diff --git a/.planning/research/v1427-r4-senior-dev.md b/.planning/research/v1427-r4-senior-dev.md new file mode 100644 index 000000000..4c058773b --- /dev/null +++ b/.planning/research/v1427-r4-senior-dev.md @@ -0,0 +1,619 @@ +# R4 senior-dev architectural review — v1.4.27 + +Read-only review of the v1.4.27 MB1-MB7 surface on `develop` (HEAD +`617d4518`). Scope frame from the dispatch directive; severity tiers +follow `BLOCKER` → `HIGH` → `MED` → `LOW` → `NIT` with the v1.4.28 +backlog as the deferral target for non-blocking items. + +Conventions observed end-to-end: no forbidden vocabulary, Marc-voice +English, no PII. The review prose follows the same conventions. + +## Summary + +The MB1-MB7 work is structurally sound. The new primitives compose with +the existing shadcn vocabulary, the SSR-safe hooks degrade correctly, +the migration is idempotent, and the context-based Coach launch model +removes the sub-page unmount bug cleanly. The findings below are +**zero blockers, two HIGH, four MED, six LOW**, plus one annotation +that should land in the CHANGELOG. + +The single most consequential finding is **HIGH-1 (ResponsiveSheet +footer slot under-used)** — every call site outside `export-section` +inlines the form footer into the body instead of feeding it through the +`footer` prop, so the sticky-pinned bottom rail the primitive promises +on the Sheet branch only paints empty. The fix is a 5-minute swap per +call site and lifts the actual UX promise to where the primitive's +JSDoc and tests already claim it lives. + +--- + +## BLOCKER + +(none) + +--- + +## HIGH + +### HIGH-1 — ``'s sticky footer rail is dead code on every consumer except `export-section` + +**Where:** every ResponsiveSheet call site: +- `src/app/page.tsx:523, 533` +- `src/app/measurements/page.tsx:92` +- `src/app/mood/page.tsx:51` +- `src/app/medications/page.tsx:261` +- `src/components/medications/SideEffectsSection.tsx:327` (inline footer at line 462) +- `src/components/medications/inventory-section.tsx:429` (inline footer at line 512) +- `src/components/medications/intake-history-list.tsx:539, 687` + +**Symptom:** the primitive's docstring (`responsive-sheet.tsx:71-75`) and +smoke suite both pin a sticky-pinned `` on the Sheet +branch so primary CTAs stay reachable when the keyboard collapses the +viewport. That's the entire point of the bottom-sheet bias on mobile. +None of the call sites listed above pass a `footer={…}` prop. They +inline the Save / Cancel row inside the `children` body. The body has +`overflow-y-auto` (line 138 of the primitive), so on phones the CTA row +scrolls **away** under the keyboard exactly like the pre-MB1 baseline. + +`grep -rn "footer=" src --include="*.tsx"` in this tree returns four +hits, all in `src/components/settings/export-section.tsx` — and that +file does not use `` at all (the `footer` token there +binds to a different component). + +**Fix:** the form-owner moves the footer JSX out of the form body and +passes it through `footer={…}`. The `
` tag stays in the body +because the `Save` button has `type="submit"` and form-association +crosses sticky-pinning fine when the submit button shares the same form +ancestor. Either move the `` to wrap the whole `` +or use `form=""` on the submit button. Five call sites; ~80 lines +moved total. + +**Impact:** the WCAG 2.5.5 floor work + the rest of MB1's mobile +narrative banks on this. Today the floor is correct but the rail is +unreachable when the keyboard is up — same UX class the lift was meant +to retire. + +**Tier:** HIGH because the primitive ships a contract its consumers +silently violate. The fix is mechanical, not architectural; v1.4.28 +W1a candidate. + +### HIGH-2 — `` mounts Sheet AND Dialog at the breakpoint boundary, losing focus + scroll position on viewport rotation + +**Where:** `src/components/ui/responsive-sheet.tsx:96, 158` + +**Symptom:** the primitive's render branch is a hard `if (isMobile) +return …; return …;`. Both branches are full Radix +portal mounts. When the viewport crosses 768 px (tablet rotation, browser +window resize, devtools toggle) `useIsMobile` flips and React unmounts +one tree and mounts the other. + +For an open ResponsiveSheet that means: +- input focus is lost (the focused element is unmounted) +- a partially-scrolled body resets to the top +- a half-typed `