Phone Choose mode sheet: review + two large YES mockups - #1618
Conversation
Design-scratch study for the shipping phone mode switcher: review findings plus large sectioned-list and icon-deck comps using clinical Find/Diagnose/Care lanes, sticky current-mode context, and min-h-12 targets.
📝 WalkthroughWalkthroughAdds a dedicated phone mode-sheet mockup route. It includes shipping, sectioned-list, and icon-deck variants with synchronized selection, focus, scrolling, accessibility behavior, review findings, and route-specific layout handling. ChangesPhone mode sheet mockup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant PhoneModeSheetYesMockupPage
participant PhoneModeSheetYesMockups
participant SectionedModeSheet
Browser->>PhoneModeSheetYesMockupPage: Open mockup route
PhoneModeSheetYesMockupPage->>PhoneModeSheetYesMockups: Render comparison page
PhoneModeSheetYesMockups->>SectionedModeSheet: Render interactive variant
SectionedModeSheet->>PhoneModeSheetYesMockups: Update selected mode and preview state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 7 failed job(s):
Compared with main CI run #8077 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
Lead with a side-by-side large YES pair, and drop the redundant Current pill on sectioned rows so selection cues stay calm.
Fold current mode into the sheet header, keep a single sticky section layer, make selection interactive, and prove Rest/Scrolled/Switched states with every review finding closed.
Pre-push format guard requires Prettier on the committed blob.
The DSM-5 switched proof opened at Find with selection off-screen; center the active row on mount so header and selection stay visible together.
`appModeDefinitions.filter((mode) => !mode.devOnly)` does not typecheck: `devOnly` is optional on the app-mode union, so reading it off every member is TS2339. This failed `Build` on PR #1618 and took `Static PR checks`, `Production UI critical`, `Advisory UI` and `Lighthouse` down with it: ./src/components/phone-mode-sheet-yes-mockups.tsx:213:59 Type error: Property 'devOnly' does not exist on type '{ readonly id: "answer"; … }' src/lib/app-modes.ts already exports the correct guard and uses it internally at lines 400 and 406. Passing "production" reproduces this call site's intent — the shipping mode list — without duplicating the `"devOnly" in mode` check. `appModeDefinitions` stays imported: modeOf() also does a plain `.find()` by id, which never touches the optional property. Verified: tsc --noEmit exit 0, eslint clean, prettier clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
BigSimmo
commented
Aug 5, 2026
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. The only repository destination is BigSimmo/Database, and the only branch destination is the pull request head branch cursor/phone-mode-sheet-yes-05c0 at starting commit 022beba; never publish fixes to a detached or synthetic work branch and never create a stacked pull request. Use the authenticated GitHub connector to commit each approved fix to BigSimmo/Database:cursor/phone-mode-sheet-yes-05c0, then verify that the pull request head contains the published commit before reporting success. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. For a fixed thread, reply with as the first line and as the second line. For a no-code disposition, use followed by . These result markers authorize the workflow to close that exact thread only after it verifies a fixed commit is the pull request head; a local-only commit is not a fix. If publication or verification fails, do not use either result marker, do not claim success, and leave the thread open with the blocker. If human input or new authorization is required, do the same. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
Summary
Publication status
Testing
|
Keep scrolled/switched proof frames from yanking the document via scrollIntoView by setting the sheet body's scrollTop instead. Uniquify section heading ids with useId, add Arrow/Home/End roving focus so inactive modes are reachable, and lift selection into the phone frame so the top-bar mode pill tracks live picks.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The proof-frame scroll effect only needs the mount selection. Depending on selectedIndex re-ran it after every pick and yanked the Scrolled frame back to Diagnose.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/branch-review-ledger.md`:
- Around line 644-645: Correct the branch history using the permitted repair
workflow, removing the prohibited main-based updates without editing or deleting
the existing ledger rows. Preserve the append-only behavior of
docs/branch-review-ledger.md and use its established command or mechanism to
record the correction.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 347b0c4f-2a95-4514-8377-166a702b963b
📒 Files selected for processing (2)
docs/branch-review-ledger.mddocs/site-map.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/site-map.md
Uh oh!
There was an error while loading. Please reload this page.
* fix(gates): catch lint and type errors before push, not in CI Two open PRs burned full CI cycles this week on defects a single local command would have caught: #1606 on a react-hooks/set-state-in-effect lint error, #1618 on a TS2339 for `mode.devOnly` (a union member that lacks the property, where app-modes.ts already exports the correct `"devOnly" in mode` guard). Neither lint nor typecheck was in the pre-push path. Typecheck could not simply be added, because it was already unusable (outstanding-issues #210). tsconfig.json's `include` carries `.next/types/**/*.ts` and `.next/dev/types/**/*.ts` — gitignored build artifacts — so deleting a page leaves the stale generated validator importing a removed module. Reproduced rather than inferred: a planted `.next/dev/types/validator.ts` referencing a removed mockup page yields `error TS2307: Cannot find module .../mockups/deleted-mockup-route/page.js`, base config exit 2, source-only config exit 0. Full source typecheck is clean (71s cold, 8.8s warm). Red locally and green in CI is how the gate got abandoned, which is how the real type error then reached CI. - tsconfig.typecheck.json + `typecheck:source`: identical compiler options, minus the `.next` globs, with a separate tsbuildinfo so the two incremental caches cannot invalidate each other. Route-signature validation is not lost; `next build` still covers it in CI. - guard-push.mjs gains a fourth guard running eslint over the pushed files and this typecheck. Verified to reproduce both defects above with CI-identical messages. Scoped to the lint roots and to pushes that touch TS, skips loudly when node_modules is absent rather than pushing people to GUARD_PUSH_DISABLE=1, and overridable with SKIP_STATIC_GUARD=1. Also corrects a doc claim that made #1580 surprising: "mockups are exempt" was being read as blanket. Mockups are exempt from the wiring and reachability gates and nothing else — they are still typechecked, and their client chunks still count toward check:bundle-budget, which totals every built chunk rather than the initial production bundle. That the budget's scope contradicts ledger #13's "not an initial production bundle" position is a real unmade decision, now recorded as #237 rather than papered over. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy * docs(issues): capture the phone Category soft-menu fix salvaged from PR #1606#1606 is closed, but it carried the one fix nothing else in the queue provides: MobileResultFilterControl's native <select> paints a harsh system-blue highlight on phones, and #1615 keeps that native select (its change is the iOS 16px anti-zoom rule). So the fix does not survive #1615 landing. Records it as #238 with the two defects the redo must not repeat: the unresolved keyboard trap on disabled options, and the set-state-in-effect lint error that PR #1620's new pre-push guard would now catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T97Kqdj9Xh1Cubv5ms3KVy * issues: capture #239 stale Cloud acceptance pin on PR #1617, #240 remote-container browser gate drift * Tighten guard coordinator test Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(gates): shared source-typecheck lease and safer static pre-push Treat typecheck:source:internal as a shared read-only coordinator lease with a distinct per-worktree buildinfo file, drop the pinned in-repo cache path, and harden staticGuard: acquire a short exclusive lease (fail-open when busy), use a private eslint cache, escalate lint on eslint policy changes, fail closed when the push tip is not HEAD, cover eslint-rules, and add Vitest coverage. Align hook/docs wording with the fourth guard and point CLAUDE.md at #252. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(gates): keep tsBuildInfoFile when run-heavy has no npm_execpath Pre-push invokes run-heavy via plain node, so the npm_execpath spawn path was skipped and the fallback dropped effectiveForwarded — undoing the per-worktree buildinfo injection. Also warn when staticGuard passes on a dirty working tree. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * docs: refresh scripts-index for lint:changed:internal Keep docs:check-inventory green after adding the pre-push eslint wrapper script to package.json. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(gates): address Devin findings on static pre-push guard - Treat "Database focused-test capacity is full" as coordinator busy so shared typecheck slot exhaustion fails open instead of faking a type error. - Skip source typecheck when every changed .ts path is excluded by tsconfig.typecheck.json (edge functions, archive, scratch, worktrees). - Restore check-github-shell-access.mjs (and its Role notes) in the scripts index. * chore(ledger): record PR #1620 babysit * fix(gates): emit structured heavy-run admission-busy signal Prefer exit 75 + DATABASE_HEAVY_RUN_ADMISSION_BUSY over prose matching so tsc/eslint output that quotes busy strings cannot false-pass the static guard. * fix(gates): tip-check only when static work runs; isolate typecheck cache Addresses follow-up Devin on PR #1620: - Reorder staticGuard so tip-vs-HEAD fails closed only when lint/typecheck will actually read the working tree; ignore tag refs in the tip check. - Pin a distinct tsBuildInfoFile on tsconfig.typecheck.json so direct tsc does not collide with the base config cache (run-heavy still overrides). * fix(gates): keep lint failures when typecheck admission is busy Addresses Devin on PR #1620 — a prior eslint failure must still block the push if the follow-up source typecheck cannot get a coordinator slot. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Summary
/mockups/phone-mode-sheet-yesreviewing the shipping phone Choose mode bottom sheet.Currently · {mode})top-0) — no fragile offset / duplicate “Now in” cardmin-h-12rows, 44 px wells, two-line descriptions, readable inactive titlesuseId()section heading ids across multiple framesClinical Governance Preflight
/mockups/*404 in production)Test plan
npm run sitemap:check/ format guardnpx eslinton changed TSX filesnpm run ensure→/mockups/phone-mode-sheet-yesloadsaria-checkedPR requiredsuccess)RAG impact
RAG impact: no retrieval behaviour change — mockups-only design scratch; no RAG surfaces touched.
Summary by CodeRabbit
New Features
Documentation