Questionnaire v2: submit-path hardening and render-failure containment

[bodhish]

Stack 3/5 — chain: #16618 ← #16627 ← this ← #16629 ← #16630.

Closes the structured render-failure containment end to end and hardens the submit path.

• composeBatch now honours structuredRenderFailedAtom: a structured section showing the error-boundary notice was already skipped by both validators, but rows recorded before the component broke still composed and submitted with their type's validate never run. Compose now shares the same skip, and a recovered slot un-marks itself via a probe that only mounts when its subtree actually rendered — so a live required input never stays exempt for the session.
• Double-submit guard across the async compose window: structured buildRequests are awaited before the batch mutation starts, so the mutation's isPending was still false and a second click composed a second identical batch.
• A repeats answer whose first row is cleared in place no longer drops its later rows — the leaf gate is now serializeResponseValues' own content rule instead of a values[0] check, matching what the required check counts as answered.
• Scroll-to-first-error picks the first failing question in reading order (the error array concatenates required failures before structured ones) and respects prefers-reduced-motion.

🤖 Generated with Claude Code

[github-actions]

⚠️ Merge Checklist Incomplete

Thank you for your contribution! To help us review your PR efficiently, please complete the merge checklist in your PR description.

Your PR will be reviewed once you have marked the appropriate checklist items.

To update the checklist:

• Change - [ ] to - [x] for completed items
• Only check items that are relevant to your PR
• Leave items unchecked if they don't apply

The checklist helps ensure code quality, testing coverage, and documentation are properly addressed.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: df0dd8a0-e816-4ca5-8851-9271f914dd48

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cloudflare-workers-and-pages]

Deploying care-preview with    Cloudflare Pages

Latest commit:	66827ab
Status:	✅  Deploy successful!
Preview URL:	https://0ad47c58.care-preview-a7w.pages.dev
Branch Preview URL:	https://bodhi-qv2-submit-hardening.care-preview-a7w.pages.dev

View logs

[github-actions]

Fine. I've seen worse.

The ClearRenderFailedOnMount probe pattern is actually clever — placing it inside the boundary means the effect only commits if the component didn't throw, which is exactly the invariant you need. The double-submit guard using both a ref (sync gate) and state (UI disable) is correct reasoning. The values[0] removal is safe because the post-serialization filter catches all-empty responses before they hit the server.

Three gripes above: atoms in useMemo instead of atomFamily, a cross-reference gap between two related code blocks in composeBatch.ts, and a recursive closure that should be a module-level helper. None of these are blockers — they're just the kind of thing that bites the next developer who has to touch this code.

Generated by Grumpy PR Reviewer for issue #16628 · 44.6 AIC · ⌖ 6.11 AIC · ⊞ 6.3K

[github-actions]

Creating write-only atoms inside useMemo works but it's non-standard Jotai. You're synthesizing a new atom class per questionId instead of using atomFamily from jotai/utils, which is the intended tool for exactly this pattern. The useMemo dependency is correct and it won't blow up — it's just going to confuse the next person who reads it and wonders why atoms are being manufactured in render.

[github-actions]

The safety of this removal depends entirely on the .filter((result) => result.values.length > 0) 20-odd lines below. The comment here says "the content decision is serializeResponseValues' alone" but doesn't mention the filter is the actual guard against submitting values: [] to the server (which your own comment below calls "a server error rather than an omission"). These two comments should reference each other — right now the reader has to connect the dots across two separate code blocks to verify correctness.

[github-actions]

inTreeOrder is a recursive closure redeclared inside runSubmit on every submit-with-errors call. For large questionnaires with many nested groups, this walks the full tree once per form that fails. It's not a hot path so it won't crater performance, but this function belongs outside the callback — it takes only questions and failing, has no other dependencies on the closure, and would be cleaner as a module-level helper.

[github-actions]

Grumpy ReviewTM — Submit Hardening & Render-Failure Containment

Alright. I've read this. All 45 files of it. My coffee is cold.

The good: The core ideas here are sound. The double-submit guard with a ref (synchronous) + state (reactive) pair is the correct pattern — state alone leaves a same-tick hole and people have been burned by that for years. The render-failed atom approach — mark on componentDidCatch, clear via a probe that only survives a successful commit — is genuinely clever and architecturally honest. The scroll-to-first-error fix (tree order, not validator array order) is the obvious right answer that took someone actually thinking about it to notice. The prefers-reduced-motion additions are appreciated.

The not-so-good (see inline comments):

1. FillOutlineOverlay.tsx line ~90: MutationObserver with subtree: true on the full canvas container. Will fire constantly during typing. The rAF debounce helps but doesn't fully contain it. Should be narrowed.

2. composeBatch.ts line ~190: Dropped values.length > 0 entirely. Fix was correct for the repeats bug but broader than needed — now pushes zero-value responses into serialize unconditionally. Needs verification that the serializer returns nothing for empty arrays, not an empty object that confuses the backend.

3. useSubmitQuestionnaire.ts line ~192: inTreeOrder closure defined inline is testable only by triggering a full submit. Module-level pure function, please.

4. FillOutline.tsx line ~59: Four-level ternary with two redundant tree walks. Extract and simplify.

5. StructuredSlot.tsx line ~42: useEffect for the recovery probe has a paint-cycle window; useLayoutEffect would be strictly more correct.

Nothing here is a ship-stopper — the submit hardening is real and the edge cases are real. But the MutationObserver scope and the composeBatch empty-values question deserve a look before the next stack lands on top of this.

Generated by Grumpy PR Reviewer for issue #16628 · 78.5 AIC · ⌖ 6.6 AIC · ⊞ 6.3K

[github-actions]

mutationObserver.observe(container, { childList: true, subtree: true });

Oh lovely — a MutationObserver on the entire scroll container with subtree: true. Every keystroke in a text field mutates the DOM (cursor blinking? nope, but value attributes, aria-live regions, validation messages), every conditional render, every checkbox tick — ALL of them fire schedule() and queue a requestAnimationFrame that walks every [data-question-id] element.

The raf-debounce helps, but it only collapses consecutive triggers within the same frame. A form being filled will saturate this continuously.

This should be watching only for nodes with data-question-id being added/removed — attributeFilter won't help here, but you could hoist the [data-question-id] anchors into a shallower wrapper and observe ONLY that, not the entire canvas subtree. At minimum, document this explicitly so the next person doesn't wonder why profiling shows MutationObserver callbacks everywhere.

[github-actions]

if (!response.structured_type) {
  answeredLeaves.push(response);
}

The old code checked response.values.length > 0 && response.values[0]?.value !== "". Now you're pushing responses with an EMPTY values array to answeredLeaves. Yes, you say serializeResponseValues handles it — but that means you're now calling serialize on every leaf response regardless, including ones with no data at all. Whether that's a silent no-op or eventually becomes a backend validation error depends entirely on what serializeResponseValues does with an empty array and whether the API rejects empty leaf objects in the batch.

This fix is correct for the multi-row repeats bug, but dropping the values.length > 0 guard entirely is broader than the fix requires. At minimum you should be checking response.values.length > 0 still, and letting serializeResponseValues decide content; the original bug was gating on values[0] not on length.

[github-actions]

const inTreeOrder = (questions: Question[]): string | undefined => {
  for (const question of questions) {
    if (failing.has(question.id)) return question.id;
    const found = inTreeOrder(question.questions ?? []);
    if (found) return found;
  }
  return undefined;
};

This recursive helper is defined INSIDE useCallback, which means it's redefined every time runSubmit is called (which is fine — it's transient). But it's also closing over failing which is itself constructed fresh each call. No correctness bug here, but inTreeOrder could trivially be a module-level utility taking questions and failing as args — it has no need to be a closure. As-is it's a bit hard to test in isolation.

Also: this walks the tree for EVERY form's errors, but you only need it when clientErrors.length > 0. Nit, since forms are small, but still.

[github-actions]

const activeRowId =
  activeQuestionId === null
    ? null
    : hasRow(activeQuestionId)
      ? activeQuestionId
      : findFirstQuestion(questionnaire.questions, (q) => q.id === activeQuestionId)
        ? questionnaire.questions[findTopLevelIndex(questionnaire.questions, activeQuestionId)]?.id
        : null;

This calls findFirstQuestion purely to check if the id exists in the tree AT ALL, and then separately calls findTopLevelIndex to get the actual ancestor — two tree walks to answer one question. findTopLevelIndex presumably already returns -1 or a sentinel when not found; just use that result directly and you can drop the findFirstQuestion call entirely.

Also: four levels of ternary nesting. This is why if statements exist. The reader has to mentally expand this left-to-right like a zip file. Extract it into a named function.

[github-actions]

The ClearRenderFailedOnMount probe is clever — it exploits the fact that a thrown render discards effects before they run. But there's a subtle gap: useEffect runs AFTER paint, not synchronously at commit. Between "the component successfully mounted" and "the effect fires and clears the mark," there's a render cycle where structuredRenderFailedAtom still has the old question ID in it.

In practice this race matters if something triggers a submit in that window — unlikely in normal use, but not impossible with a fast autosave or keyboard shortcut. A useLayoutEffect would close that window since it fires synchronously after DOM mutations before paint. The probe doesn't touch the DOM, so useLayoutEffect is safe here and would be strictly more correct.