Questionnaire v2: fill experience rebuild (stack base)

[bodhish]

Stacked on #16618 (Questionnaire actions). Full chain:

1. Questionnaire actions #16618 — questionnaire actions (base: develop)
2. Questionnaire v2: fill experience rebuild (stack base) #16627 (this) — the v2 fill experience
3. Questionnaire v2: submit-path hardening and render-failure containment #16628 — submit-path hardening and render-failure containment
4. Questionnaire v2: fill outline as an overlay rail + panel #16629 — fill outline as an overlay rail + panel (reference design)
5. Questionnaire v2: outline overlay and submit/draft regression coverage #16630 — outline overlay and submit/draft regression coverage
6. Questionnaire v2: one header per structured question + rendering coverage for all types #16631 — one header per structured question + rendering coverage for all types

What this contains

The v2 fill experience: the successor to EncounterQuestionnaire + the legacy QuestionnaireForm, built on the module's single renderer (form/) and engine (form/engine/). The legacy fill stack is deleted; the structured QuestionTypes/* components survive behind typed adapters in structured/.

• Fill page (fill/): fullscreen shell with questionnaire canvas + embedded clinical-history tabs, patient/encounter context header, multi-questionnaire sessions (each form with its own provider/store, one batch submission), and the subject union (encounter/patient/location/device/facility mounts).
• Submission (fill/submit/): pure composeBatch per form into one atomic batch — structured requests via each type's buildRequests, plain answers via the submit endpoints, reference_id-keyed server-error mapping back to the owning form's store.
• Drafts (fill/draft/): local autosave (one localStorage entry per user/subject/entry questionnaire, debounced with pagehide/unmount flush, session-boundary sweeps) plus the deliberate server draft (form_submission, feature-flagged, encounter-subject) with the encounter overview's drafts card and ?continue_draft= resume.
• Plugin seams: namespaced structured question types registered at runtime with graceful degradation when a deployment lacks the plugin, and the fill-page wiring for the descriptor/invoke action registry from Questionnaire actions #16618.
• Playwright coverage throughout (tests/facility/patient/encounter/fill/, structuredQuestions/, resource-subject mounts) with backend E2E fixtures.

See src/components/QuestionnaireV2/README.md for the module map, frozen contracts and boundaries.

🤖 Generated with Claude Code

[github-actions]

Grumpy Review — Questionnaire v2 Fill Experience

261 changed files, 27k additions, 105 commits. I've seen smaller rewrites of entire operating systems. Fine, let's get through it.

The architecture is actually thought through: subject.ts discriminated union is clean, the store-registrar pattern for jotai isolation per form is clever, the draft cache/store split to keep fillDraftCache.ts import-free is good engineering, and catching StructuredBuildError instead of letting a plugin rejection become an unhandled promise is exactly right. The test for unsupportedDraftStructuredTypes that explicitly avoids importing registry.ts because it would blow up under node --test is the kind of discipline I wish I saw more often.

Issues worth fixing before merge:

1. PluginEngine.tsx — error log uses plugin.plugin (untrusted remote manifest field) instead of plugin.slug (trusted backend-issued identity). The same diff correctly explains why they're different and to always use slug. Then immediately uses plugin in the error message. Pay attention to your own comments.

2. useSubmitQuestionnaire.ts — MAX_BATCH_REQUESTS = 20 is a hardcoded mirror of a backend config constant with no enforcement mechanism. Silent drift.

3. composeBatch.ts — FormSubmissionCompletionBody sends fields the backend route explicitly does not document. Your own comment admits this. Untrack it properly or fix it in the backend.

4. FillOutlineOverlay.tsx — MutationObserver with subtree: true on the whole form scroll container is too broad. Will trigger on every keystroke when components re-render DOM.

5. fillDraftCache.ts — JSON.parse(... ?? "") uses exception-as-control-flow for the missing-key path. Use "null" as the fallback.

Non-blocking observations:

• QuestionnaireFillPage.tsx at 779 lines and useFillActions.ts at 519 lines are on the large side, but they've clearly already been extracted from something bigger and the logic density justifies the length.
• useFillAutosave.ts at 330 lines with six refs updated inline each render (scopeRef.current = scope etc.) is the right pattern here — better than including all of them in every effect dependency array.

The deletion of QuestionnaireForm.tsx (1203 lines) is the highlight of this PR. Good riddance.

Generated by Grumpy PR Reviewer for issue #16627 · 88.5 AIC · ⌖ 8.98 AIC · ⊞ 6.3K

[github-actions]

The error log here uses plugin.plugin — the value the remote manifest declared about itself, explicitly called out two comments above as untrusted and potentially spoofed. The backend-issued, trusted identity for the plugin is plugin.slug. Logging an attacker-controlled string in the error message is at best misleading, at worst used to obfuscate which plugin actually misbehaved.

console.error(`Invalid structured type from plugin ${plugin.slug}`, error);

[github-actions]

MAX_BATCH_REQUESTS = 20 silently mirrors MAX_REQUESTS_PER_BATCH_REQUEST on the backend with zero enforcement. When the backend changes that cap — and it will, because 20 is already thin for a multi-questionnaire session — the frontend continues sending batches "within limit" that the server now rejects with a cryptic 400. Either derive this from a backend-supplied value at load time, or at least make the coupling explicit in a comment that points to the backend setting's location so someone remembers to sync them.

[github-actions]

FormSubmissionCompletionBody ships patient and encounter that the backend route's own documented type omits, acknowledged right here. "This batch entry is a hand-built request, not a call through that route" is not a long-term excuse — it means you are relying on undocumented field acceptance and you will find out it broke only when someone touches the backend serializer. Either add these fields to the backend's FormSubmissionUpdate type (right call) or track this as a known debt with a link to the backend issue.

[github-actions]

MutationObserver with childList: true, subtree: true over the entire form scroll container fires on every DOM mutation inside every question — text input re-renders, option list opens, structured tables updating cells, the lot. The rAF guard batches the recompute so it's not catastrophic, but this is a very broad listener for a scroll-spy that only needs to know when question blocks appear or disappear. Consider narrowing to attribute mutations on [data-question-id] elements or observing only one level deep, otherwise on a 50-question form you're scheduling a rAF on every keystroke.

[github-actions]

JSON.parse(localStorage.getItem(key) ?? "") — getItem returns null when the key doesn't exist between your startsWith check and the read (race with another tab's removeItem). JSON.parse("") throws SyntaxError, which you catch and remove the key, so it works, but it's unnecessarily disguised. Use "null" instead: JSON.parse(localStorage.getItem(key) ?? "null") gives you null on the missing-key path and the type remains object | null without a silent throw-as-control-flow path.

[github-actions]

Grumpy Review — Fill Experience Rebuild

Alright. I've looked through this. It's a large, well-structured rebuild with unusually good documentation for its size. The multi-form session model is sound, the draft layering is deliberate, and the subject-mismatch guard before mount is the kind of defensive code I rarely see. So — grudging respect.

That said, four things actually bother me:

1. MAX_BATCH_REQUESTS = 20 — a frontend copy of a backend constant with no synchronization story. This is a maintenance trap.
2. setTimeout() with no delay in scrollToQuestion — React 18 concurrent mode doesn't guarantee this. Use requestAnimationFrame.
3. response_dump as { ... } cast without validation — you're trusting optional chaining to handle server-sourced malformed data. It won't catch structural problems inside responses entries. This is a clinical app; unvalidated untrusted blobs deserve a parse.
4. !import.meta.env.DEV disabling the nav prompt — prod and dev have different behavior for a safety feature. That's a bad pattern regardless of how convenient it is.

The file is 854 lines and has 8 component definitions in it, but given the gating/body split that's already in place, this is more "could be better" than "must fix." The inline comments are thorough enough that a future maintainer won't be lost.

Fix the four issues above and this is shippable.

Generated by Grumpy PR Reviewer for issue #16627 · 78.1 AIC · ⌖ 8.89 AIC · ⊞ 6.3K

[github-actions]

This MAX_BATCH_REQUESTS = 20 is a local copy of a backend setting (MAX_REQUESTS_PER_BATCH_REQUEST). When someone bumps the backend value, this frontend guard becomes either too conservative (blocks valid submissions) or irrelevant — and there's zero tooling to catch the drift. Either fetch this limit from the API or accept that the guard will silently go stale and remove it entirely; a backend 400 is already the hard stop. Having two fences with no synchronization mechanism is worse than one.

[github-actions]

setTimeout() with no delay (i.e., 0ms) to defer DOM work after a state update is a classic "works until it doesn't" pattern. React 18's concurrent renderer doesn't guarantee that a zero-delay timer fires AFTER the DOM is painted — flushSync + synchronous measurement, or requestAnimationFrame, is the correct primitive here. The comment even says "deferred a tick so the error render exists before we measure" — that's exactly what rAF is for.

[github-actions]

You're casting response_dump from the server — an untyped blob — directly to a typed structure, then trusting optional chaining to catch the bad cases. But form.responses is only narrowed to QuestionnaireResponse[] by that cast; a server that stored malformed entries (wrong shape, missing question_id) will silently produce undefined keys in record at line 195, which revive into junk later at reviveDraftResponses. This boundary deserves a proper Zod parse or at minimum a question_id guard before writing into record.

[github-actions]

Suppressing the navigation prompt in dev (!import.meta.env.DEV) means the "unsaved changes" guard is completely invisible to everyone writing or testing the fill page locally. Developers will happily navigate away with half-filled forms and never notice that clinicians in prod would be blocked by the same action. If the prompt is annoying during dev, use a more targeted escape (a specific env var, a test utility, something you control) — don't silently diverge prod and dev behavior for a safety-critical UX feature.

[github-actions]

The eslint suppression here is sweeping a real rule violation under the rug. scope and questionnaire are live objects that mutate independently of scopeKey — the "derived from stable deps" argument only holds if the derivation is tight and documented as an invariant. It isn't: scope and questionnaire both appear in the memo body but are absent from the deps array. Anyone refactoring this later has no indication this was intentional rather than a bug. At minimum, add a comment explaining the invariant; better yet, restructure so the suppression isn't needed.

[github-actions]

Grumpy Review: Questionnaire v2 Fill Experience

327 files. 42k additions. I've seen smaller codebases. Let's talk about what I actually found.

The architecture is, begrudgingly, decent. One Jotai store per form instance, clean subject union, structured type registry with graceful degradation, draft TTL + sweep — these are the right choices. Whoever designed this actually thought about the failure modes. Rare. I'll acknowledge it.

What needs fixing:

1. FilesEditor missing encounterId in deps — real potential stuck-buffer bug in the encounter-creation flow. The comment explains why list.addRows is excluded (fair), but says nothing about encounterId. Files queued while encounterId is undefined will stay stuck if nothing else re-triggers the effect after it becomes defined.

2. MAX_BATCH_REQUESTS = 20 hardcoded — a constant that must stay in sync with a backend config value and has no mechanism ensuring it does. Classic dual-maintenance trap.

3. TODO left in production code without a tracking issue — the useBatchRequest migration TODO in useSubmitFillSession. Untracked TODOs are where tech debt hides for years.

4. windowTestBridge localhost gate — intentional for Playwright, but underdocumented for the next maintainer who finds internal session handles exposed on window in a "production" environment.

The test coverage is extensive and the error-mapping/draft-merge logic is well-separated. The composeBatch + mapBatchErrors split is clean. I'm grumpy, not blind.

Generated by Grumpy PR Reviewer for issue #16627 · 76 AIC · ⌖ 6.35 AIC · ⊞ 6.3K

[github-actions]

The localhost gate enables the test bridge on ANY production build served from localhost — including a real deployment being previewed locally, a Docker container mapped to 127.0.0.1, or a developer's npm run preview against prod data.

This is by design for Playwright (the comment says so), and the bridge itself is relatively low-risk. But the gate should at minimum document this explicitly: "intentionally enabled on localhost prod builds for E2E test drivability." As written, the next person who sees window.__CARE_FILL_ASSISTANT__ in a prod environment will think something has gone wrong.

[github-actions]

encounterId is used in the effect body but excluded from deps — potential stuck-buffer bug.

If encounterId transitions from undefined → defined (encounter-creation flow) while files are queued in the buffer, the effect won't re-run because encounterId isn't a dep. The guard !encounterId blocks the drain, files sit in the buffer, encounterId becomes defined... and nothing re-triggers the effect.

The comment justifies excluding list.addRows (fair — unstable identity). It says nothing about encounterId. The disabled dep offers an incidental rescue only if the section was frozen during encounter creation and re-enables after. That's a coincidence, not a contract.

Either add encounterId to the dep array, or add a comment explaining the exact lifecycle guarantee that makes its omission safe.

[github-actions]

Hardcoded constant that must stay in sync with a backend config value — fragile.

MAX_BATCH_REQUESTS = 20 mirrors MAX_REQUESTS_PER_BATCH_REQUEST from the care backend settings. If the backend bumps that limit, this client-side guard silently becomes wrong: either it'll reject batches the backend would accept, or it'll pass batches the backend still rejects. Forty years of distributed systems have taught me: constants that exist in two places will diverge.

This should either come from a server-provided config endpoint, or at minimum live in a shared config file with a comment pointing exactly to the backend setting it mirrors so the next maintainer knows to update both.

[github-actions]

TODO left in production-bound code. "Migrate to useBatchRequest once it can..." — track this as an issue. TODOs that don't have a ticket number attached are where technical debt goes to retire undisturbed for years. The @typescript-eslint/no-deprecated disable below confirms you already know this path isn't ideal. Either fix it now or open an issue and reference the number here.