Questionnaire v2: fill outline as an overlay rail + panel - #16629
Conversation
…the reference design The outline no longer reserves a fixed column. A slim tick rail hugs the canvas' left edge (one hairline per visible top-level question, the one in view longer and indigo, distributed minimap-style so long sessions compress instead of clipping); the 320px panel floats OVER the full-width canvas on hover, focus or click. Rows are numbered with live completion adornments, the active row tracks a scroll-spy over the data-question-id anchors, and a clicked row pins active until the user scrolls. Each form of a multi-questionnaire session portals its rows and ticks into the shared overlay, keeping the per-form store architecture. Interaction model: hover governs mice (a mouse click on the open rail is deliberately a no-op), tap toggles for touch (judged by the state at pointerdown, so the tap's own focus-open can't flash it shut), Escape and focus-out close for keyboards with focus restored to the rail. Dismissal rides NATIVE listeners on the shell — the portaled rows never propagate React synthetic events through the overlay's tree, and some embedded browsers don't synthesize React's enter/leave pair at all. Ride-along fill fixes from the same review pass: - Draft safety: resume retains added-form snapshots through the re-fetch window, merges into a form the clinician already re-added by hand, the picker seeds from a retained snapshot instead of displacing it, Discard drops only the STORED draft (typed work stays and persists), a resumed record must still be status "draft", and a background refetch of the draft query no longer unmounts the live session. - Context fetch failures show an error page instead of a headerless clinical form; the loading skeleton uses the real fullscreen shell. - A11y: aria-required on every engine input (the asterisk was visual-only), date/dateTime pickers get named groups, keyboard row activation moves focus to the question, tab strip and action band no longer clip at phone widths, Save Changes gains a pending spinner and primary-token styling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Entire-Checkpoint: 190e20e46e89
Deploying care-preview with
|
| Latest commit: |
8da11a8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6105eb97.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://bodhi-qv2-outline-overlay.care-preview-a7w.pages.dev |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8da11a8dd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // show a headerless page with no patient identity, blood group or | ||
| // allergy badges — safety-relevant context — while the clinician types | ||
| // clinical data into it. | ||
| if ((encounterId && isEncounterError) || isPatientError) { |
There was a problem hiding this comment.
When the encounter or patient is already cached, this page mounts FillPageBody immediately while React Query performs its normal stale-data refetch; if that request then fails, isError becomes true even though encounter/fetchedPatient still contains valid data. This branch consequently unmounts every form store after the clinician may already have entered answers, and a resumed server-draft session does not locally autosave those edits. Gate the error page on the corresponding data being absent rather than on isError alone.
Useful? React with 👍 / 👎.
| scrollContainer.addEventListener("wheel", release, { passive: true }); | ||
| scrollContainer.addEventListener("touchstart", release, { passive: true }); |
There was a problem hiding this comment.
After any outline-row selection sets pinnedId, the pin is released only for wheel and touch gestures. If the user subsequently scrolls the canvas with Page Down/arrow keys or by dragging the scrollbar, the scroll-spy continues computing new IDs but pinnedId ?? spyActiveId permanently masks them, so both the highlighted row and rail tick remain on the old question until a wheel/touch gesture occurs. Manual scroll paths beyond these two events need to clear the pin while excluding only the navigation's own smooth scroll.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The overlay architecture is solid — portaling per-form rows/ticks into shared hosts while keeping them inside their form providers is the right call. Draft safety fixes (isLoading vs isFetching, status guard, retained snapshots) are clearly thought through. The a11y work is genuinely good intent.\n\nFour real issues worth fixing before this ships:\n\n1. MutationObserver subtree perf — fires on every keystroke/render inside the canvas, scheduling querySelectorAll+getBoundingClientRect on every animation frame during editing. Filter mutations or use a more targeted observation target.\n2. Double aria-labelledby on the time input — the group already propagates the label; the explicit attribute on the child input will double-announce it.\n3. activeRowId undefined creep — questionnaire.questions[findTopLevelIndex(...)]?.id can return undefined not null when the index is -1, and the type says string | null. Won't crash but is fragile; same issue in FillOutlineRail.\n4. Toggle button and aside share the same accessible name — screen readers can't distinguish the control from the landmark it opens.
Generated by Grumpy PR Reviewer for issue #16629 · 59.7 AIC · ⌖ 6.38 AIC · ⊞ 6.3K
| children, | ||
| }: { | ||
| /** The canvas' scrolling element — anchors are queried inside it. */ | ||
| scrollContainer: HTMLElement | null; |
There was a problem hiding this comment.
MutationObserver with subtree: true is a performance footgun. Every React render, every keystroke, every conditional re-render fires this observer. You've got the if (!frame) RAF guard so it won't stack up, but you're still scheduling a full querySelectorAll("[data-question-id]") + getBoundingClientRect() loop every animation frame during active editing. You only care about [data-question-id] elements appearing or disappearing (enable_when blocks). Use { attributes: false, characterData: false, childList: true, subtree: true } and filter inside the callback to bail early if no [data-question-id] element was added or removed. As-is this will hammer the layout thread on long questionnaires.
| @@ -69,7 +78,8 @@ export function DateTimeQuestionInput({ | |||
| /> | |||
| <Input | |||
There was a problem hiding this comment.
The time <Input> has aria-labelledby={labelId} AND lives inside a role="group" aria-labelledby={labelId}. ARIA label inheritance from the group already names child controls — adding an explicit aria-labelledby on the input itself means screen readers will announce the question label twice for the time field. Pick one: either the group names everything inside it, or each control is labeled individually. Not both.
| ) : ( | ||
| <Dot className="size-4 shrink-0 text-gray-500" /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
activeRowId can silently evaluate to undefined here. findFirstQuestion confirms the question exists in the tree, then findTopLevelIndex is called separately on the same tree — two full traversals where one would do. More importantly: if findTopLevelIndex ever returns -1 (question not in the top-level array, e.g. it's deeper than one level of nesting), questionnaire.questions[-1] is undefined, so ?.id gives undefined, not null. The callers do activeRowId === item.question.id which silently stays false. You've added a type annotation of string | null but the actual runtime value can be string | null | undefined. Either unify the two traversals, or add ?? null after the bracket access.
| const { t } = useTranslation(); | ||
| const [open, setOpen] = useState(false); | ||
| const closeTimer = useRef<ReturnType<typeof setTimeout>>(undefined); | ||
| const rootRef = useRef<HTMLDivElement>(null); |
There was a problem hiding this comment.
The toggle button and the <aside> landmark share the exact same aria-label value ("questions_outline"). A screen reader user navigating landmarks will find two things with identical names — one a button, one a navigation region. The button should say something like "Toggle questions outline" or "Open questions outline" so it's distinguishable from the landmark it controls. Right now aria-controls points at the aside, but the button's own announced name is a noun, not an action.
| const activeTopId = | ||
| activeQuestionId !== null && | ||
| findFirstQuestion( | ||
| questionnaire.questions, |
There was a problem hiding this comment.
Same double-traversal problem as FillOutline: findFirstQuestion walks the tree to check existence, then findTopLevelIndex walks it again to get the index. There's no memoization here so this runs on every render. Combine them — findTopLevelIndex returning -1 already encodes "not found". Also: the result is never null-coalesced, so questionnaire.questions[-1]?.id can silently return undefined and activeTopId becomes falsy rather than null, which happens to work because of the || comparison downstream but it's fragile.
There was a problem hiding this comment.
Grumpy review 🔥
The concept is sound. Overlay rail instead of a reserved column — fine, more canvas space. The draft safety fix (isLoading not isFetching) is the kind of thing that bites you three months later in an incident review, so good catch. The serverDraft.status !== "draft" guard should have been there from day one. The aria-required additions are overdue.
Three things that need attention (inline comments above):
-
MutationObserver({childList: true, subtree: true})on the entire form container — chatty by design; will fire aquerySelectorAll+getBoundingClientRectloop per rAF bucket during any typing. On a 60-question form with fast data-entry this is measurable overhead. -
scrollContainer ?? documentfallback —documentis the wrong escape hatch here. A missing container should be a no-op, not a document-wide search that might find an element from a different component. -
Quadruple ternary for
activeRowId— technically correct (probably), but too clever for its own good. Named helper, please.
The rest of the PR — the portal architecture, the interaction model, the pin-then-spy pattern — is thoughtfully done. Begrudgingly acknowledged. 🔥
Generated by Grumpy PR Reviewer for issue #16629 · 68 AIC · ⌖ 5 AIC · ⊞ 6.3K
| const scrollToQuestion = useCallback( | ||
| (questionId: string, options?: { focus?: boolean }) => { | ||
| setPinnedId(questionId); | ||
| const root: ParentNode = scrollContainer ?? document; |
There was a problem hiding this comment.
scrollContainer ?? document as the querySelector root means that if scrollContainer is null at the moment of a keyboard-activated row click (edge case during mount), the query falls through to the entire document — it could accidentally scroll a [data-question-id] element from a different component that happens to be in the DOM at the same time. The early-return on !block won't save you if the wrong block is found. Use scrollContainer?.querySelector(...) and return early if !scrollContainer instead.
| item.question.id === questionId || | ||
| item.children.some((child) => child.question.id === questionId), | ||
| ); | ||
| const activeRowId = |
There was a problem hiding this comment.
Four-level nested ternary for activeRowId. I've been reading code for 40 years and this still made me re-read it twice. Also a subtle type hole: questionnaire.questions[findTopLevelIndex(...)]?.id can be string | undefined if findTopLevelIndex returns an out-of-bounds index, but the variable is inferred as string | null | undefined — the active = activeRowId === item.question.id comparison still works correctly, but this is exactly the kind of thing that sneaks bugs in later. Extract it into a named helper with a return type annotation.
| const resizeObserver = new ResizeObserver(schedule); | ||
| resizeObserver.observe(container); | ||
| const mutationObserver = new MutationObserver(schedule); | ||
| mutationObserver.observe(container, { childList: true, subtree: true }); |
There was a problem hiding this comment.
The MutationObserver with childList: true, subtree: true on the entire scroll container will fire on every React DOM mutation inside the form — each keystroke, validation run, and conditional show/hide all add/remove nodes. The requestAnimationFrame debounce helps, but you'll still burn a compute() call every frame that has any DOM activity. Given that compute() does a full querySelectorAll("[data-question-id]") + getBoundingClientRect() loop, this can get expensive on long questionnaires under fast typing.
attributeFilter: ["data-question-id"] plus a separate observer just for childList on the direct children of the container (new form sections appearing) would be more surgical. Or accept the tradeoff and document it.
1 participant
Stack 4/5 — chain: #16618 ← #16627 ← #16628 ← this ← #16630.
Implements the reference design for the fill page outline (Care Master, node 43604-21509): the outline no longer reserves a fixed 288px column — the canvas gets the full width.
navlandmark per form in multi-questionnaire sessions (each form portals its rows and ticks in, preserving the per-form store architecture).data-question-idanchors; a clicked row pins active until the user scrolls (near the scroll floor the target can never top the viewport, so pure spy would highlight the wrong question).Ride-along fill fixes from the same review pass:
isLoading, notisFetching); aform_submissionthat is no longer statusdraftrefuses to resume; Resume retains added-form snapshots through the re-fetch window and merges into a form the clinician already re-added by hand; the picker seeds from a retained snapshot instead of displacing it; Discard drops only the stored draft — answers typed while the prompt was pending stay and persist as a fresh draft.aria-requiredon every engine input (the asterisk was visual-only), named groups for the date/dateTime pickers, keyboard outline-row activation moves focus to the question, tab strip and action band no longer clip at phone widths, Save Changes gains a pending spinner and primary-token styling.🤖 Generated with Claude Code