fix(frontend): overlay/focus/motion a11y, sign-in & registry recovery, DS hardening#1036
Merged
Merged
Conversation
…, DS hardening Targeted convergence & hardening from a fresh audit of the (already mature) frontend — no redesign, no god-component refactor. All batches verified: typecheck, lint, full unit+jsdom suite (2986 passed / 1 skipped), and the design-system contract at a tightened literal-shadow baseline. - ui/sheet.tsx: module-level open-stack so only the top-most Sheet handles Escape/Tab (fixes stacked Evidence->lightbox/table double-close + focus escape), with a ref-counted body-scroll lock (ARCH-01/02; WCAG 2.1.2/2.4.3). - scroll-padding-bottom on #main-content so keyboard focus never lands under the fixed composer dock (A11Y-FOCUS-01; WCAG 2.4.11). - new resolveScrollBehavior() gates ~14 scripted smooth-scrolls on the reduced-motion preference (ANIM-01; WCAG 2.3.3). - ui-primitives: IconButton primitive with a required accessible label; adopted in InlineNotice + image lightbox. Live regions on answer skeleton / lightbox loading+failure (A11Y-LIVE-02/03). - settings-dialog: surface auth notice + OAuth errors via the shared InlineNotice (STATE-01); email-field autofill/inputmode/aria (FV-01/02). - registry hooks: refetch + Retry affordance across the record loader and the services/forms homes (STATE-02); 100vh->100dvh in global-error (RESP-03). - --shadow-rail-active token replaces 4 inlined active-rail shadows; contract literal-shadow baseline tightened 6->1 (DSD-05/01/06). - Regression tests (sheet, scroll-behavior, icon-button, mode-home-status-notice, registry-retry) + docs (component-test patterns, contribution checklist, APIs). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes reduced-motion scrolling, adds accessible UI primitives and status semantics, introduces retryable registry errors, coordinates stacked sheets, standardizes active-rail styling, and documents related design-system and testing contracts. ChangesUI behavior and accessibility contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The resolveScrollBehavior() swap lengthened a scrollTo call past the print width; apply the wrap so format:check passes. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
BigSimmo
enabled auto-merge (squash)
July 21, 2026 02:31
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ui/sheet.tsx (1)
214-231: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep focus inside the remaining top Sheet.
Line 217 pops a lower Sheet, but Lines 218-231 still restore its prior focus target. If a lower Sheet closes while an upper Sheet remains open, this moves focus behind the active modal. Restore focus only when the closing Sheet was topmost; add a regression test for this out-of-order close.
Proposed fix
return () => { window.cancelAnimationFrame(focusFrame); window.removeEventListener("keydown", onKeyDown); + const wasTopmost = isTopmostSheet(sheetId); popSheet(sheetId); + if (!wasTopmost) return; const restoreTarget = explicitReturnElement ?? previousActiveElement;🤖 Prompt for 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. In `@src/components/ui/sheet.tsx` around lines 214 - 231, Update the cleanup logic around popSheet in the Sheet component to restore focus only when the closing sheet was topmost; when another Sheet remains active, leave focus within that remaining top Sheet. Add a regression test covering out-of-order closure of a lower Sheet while an upper Sheet is still open, verifying focus is not moved behind the active modal.
🤖 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/testing.md`:
- Line 65: Update the verification checklist in docs/testing.md to include npm
run verify:pr-local before handoff and npm run ensure before browser work.
Preserve the existing order requiring npm run verify:cheap first, npm run
verify:ui for UI/routing/styling changes, and the manual dark-mode and
forced-colors spot check.
In `@tests/clinical-dashboard-merge-artifacts.test.ts`:
- Around line 145-147: Add a second assertion in the clinical dashboard artifact
test alongside the existing clinicalDashboardSource check, targeting the
non-answer mobile class combination with sm:mb-0 and the same mobile composer
reserve scroll-padding utility. Keep the existing sm:mb-24 assertion unchanged.
---
Outside diff comments:
In `@src/components/ui/sheet.tsx`:
- Around line 214-231: Update the cleanup logic around popSheet in the Sheet
component to restore focus only when the closing sheet was topmost; when another
Sheet remains active, leave focus within that remaining top Sheet. Add a
regression test covering out-of-order closure of a lower Sheet while an upper
Sheet is still open, verifying focus is not moved behind the active modal.
🪄 Autofix (Beta)
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 Plus
Run ID: 413bfde7-d86a-49bc-875e-0e0799f444cc
📒 Files selected for processing (30)
docs/design-system.mddocs/testing.mdscripts/check-type-scale.mjsscripts/design-system-contract-baseline.jsonsrc/app/global-error.tsxsrc/app/globals.csssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/account-setup-dialog.tsxsrc/components/clinical-dashboard/answer-status.tsxsrc/components/clinical-dashboard/favourites-command-library-page.tsxsrc/components/clinical-dashboard/favourites-library-nav.tsxsrc/components/clinical-dashboard/image-lightbox.tsxsrc/components/clinical-dashboard/medication-prescribing-workspace.tsxsrc/components/clinical-dashboard/settings-dialog.tsxsrc/components/forms/forms-home-page.tsxsrc/components/mode-home-template.tsxsrc/components/registry-record-loader.tsxsrc/components/services/services-home-page.tsxsrc/components/ui-primitives.tsxsrc/components/ui/sheet.tsxsrc/lib/scroll-behavior.tssrc/lib/use-registry-records.tstests/clinical-dashboard-merge-artifacts.test.tstests/favourites-demo-boundary.test.tstests/icon-button.dom.test.tsxtests/mode-home-status-notice.dom.test.tsxtests/registry-retry.dom.test.tsxtests/scroll-behavior.dom.test.tsxtests/sheet.dom.test.tsx
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #4223 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
- check:maintainability-budgets was red: the reduced-motion helper import (resolveScrollBehavior) grew ClinicalDashboard.tsx 4272->4274 and DocumentViewer.tsx 1733->1734 (import + one prettier-wrapped call). Ratchet the no-growth budgets by the exact +2/+1; no monolith complexity added, and #1034 is separately decomposing ClinicalDashboard downward. - docs/testing.md: document verify:pr-local (before handoff) and npm run ensure (before browser work) in the Verify checklist. - clinical-dashboard-merge-artifacts.test.ts: also assert the non-answer (sm:mb-0) branch carries the scroll-padding-bottom utility. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013f523gDfoBXPYT4H9vXRkE
…23366' into claude/frontend-review-improve-123366
…w-improve-123366 # Conflicts: # scripts/check-maintainability-budgets.mjs
12 tasks
3 tasks
BigSimmo
added a commit
that referenced
this pull request
Jul 21, 2026
…sions (#1048) * test(a11y): lock in reduced-motion scroll + focus-not-obscured regressions Guard the two accessibility fixes shipped in #1036 against regression: - ANIM-01 (WCAG 2.3.3): a live Chromium test intercepts the behaviour argument every scripted scrollTo/scrollIntoView receives, then triggers a known scroll ("New chat"). Reduced motion must produce instant "auto" jumps; no-preference must animate ("smooth") — verifying resolveScrollBehavior() is threaded through the call sites, not just unit-tested in isolation. - A11Y-FOCUS-01 (WCAG 2.4.11): a deterministic source pin asserts #main-content reserves scroll-padding-bottom in both mobile dock branches (plus the answer scroll-padding-top). The reservation only renders in app states impractical to reach reliably in a browser test, so it is pinned at source — the repo's established pattern for phone scroll-geometry invariants. Verified: typecheck, lint, format:check, full unit+jsdom suite (348 files / 3108 passed / 0 failed), and both targeted tests green against the local server. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(a11y): scope scroll-padding guard to #main-content and require both dock branches Address the review findings on the A11Y-FOCUS-01 source pin: - Bind every assertion to the extracted `<main id="main-content">` opening tag instead of counting source-wide occurrences, so a reservation class that drifts onto a child or a non-scrolling wrapper now fails the guard (a `>` inside a className comment means the tag is read up to its line-final `>`). - Require the *paired* `pb` + `scroll-padding-bottom` reservation in both dock branches (>=2), so one branch losing its content padding no longer passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LRZ1EyBZW1ADXrZvMxEEsC --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Targeted convergence & hardening of the (already mature) frontend, from a fresh adversarially-verified audit — no redesign, no god-component refactor. 31 findings audited (1 High, rest Medium/Low, no Critical); the High + confirmed Mediums + clear low-risk wins are fixed here (17 findings), the rest documented as follow-ups.
Accessibility
ui/sheet.tsx— module-level open-stack so only the top-most Sheet handles Escape/Tab (fixes stacked Evidence→lightbox/table double-close + focus escape), with a ref-counted body-scroll lock. (ARCH-01 High, ARCH-02; WCAG 2.1.2/2.4.3)scroll-padding-bottomon#main-contentso keyboard focus never lands under the fixed composer dock. (A11Y-FOCUS-01; WCAG 2.4.11)resolveScrollBehavior()gates ~13 scripted smooth-scrolls on the reduced-motion preference (the in-app toggle was silently broken for scrolling). (ANIM-01; WCAG 2.3.3)IconButtonprimitive with a TS-required accessible label; adopted inInlineNotice+ image lightbox. Live regions on the answer skeleton and lightbox loading/failure. (TCD-06, A11Y-LIVE-02/03)State / feedback / forms
auth.notice(the "check your email" confirmation) and OAuth errors via the sharedInlineNotice. (STATE-01)refetch+ button) across the record loader and services/forms homes, instead of a reload-only dead end. (STATE-02)inputMode/aria-invalid+aria-describedbyon Settings & account-setup;100vh→100dvhin the global error boundary. (FV-01/FV-02, RESP-03)Design system
--shadow-rail-activetoken replaces 4 hand-inlined active-rail shadows; the design-system-contract literal-shadow baseline is tightened 6→1. (DSD-05/01/06)Tests & docs
*.dom.test.tsx(stacked Sheet, reduced-motion scroll, IconButton label contract, ModeHomeStatusNotice retry, registry error-recovery);docs/testing.mdcomponent-test patterns + contribution checklist;docs/design-system.mdnew-API entries.Verification
npm run typecheck✓ ·npm run lint✓ · full unit+jsdom suitenpm run test— 344 files / 3077 passed / 1 skipped / 0 failed ✓ ·check:design-system-contract✓ (at the tightened baseline) ·check:type-scale/check:icon-scale✓ ·format:check✓. The Chromiumverify:uigate runs here in CI.Clinical-governance preflight
N/A — pure UI/frontend work. No changes to ingestion, answer generation, search/ranking, source rendering, document access, privacy, or clinical output. (One clinical-adjacent finding —
FV-03, patient-profile numeric bounds — was deliberately deferred as governance-gated.)Notes
origin/main; a pre-existing unrelated ACL migration that was parked on the branch base was excluded.🤖 Generated with Claude Code
Summary by CodeRabbit