fix(design-system): resolve the full-frontend design review findings - #2210
Merged
Conversation
Applies the smallest correct fix for each finding from the 2026-08-20 read-only design review, and closes the gate gaps that let them accumulate. Live defects: - Declare `--shadow-overlay` (aliased to the elevated tier, matching the other production popovers). It was referenced with no fallback by the sidebar Appearance menu and declared nowhere, so `box-shadow` was invalid at computed-value time and the popover rendered flat. - Declare `--radius-pill`. 14 call sites worked only because each independently wrote the same `, 9999px` fallback. - Print factsheet: the "not for clinical use" banner paired the light `--danger-bg` with the DARK theme's `--danger-solid` (#b42318). Restored to the light value (#a3190f), lifting measured contrast 6.05:1 -> 7.15:1. The literals stay literals on purpose - print forces a white ground but not light token values - and a comment now records why. Accessibility: - ward-management: 12 tap targets at the forbidden 44px raised to `var(--spacing-tap)`; `.bedStates span` left alone as display-only. - ward-management `.roleSelect`: `outline: 0` removed the only focus indication with no replacement. The visible shell now owns the ring, scoped to the wrapper because the phone layout sets the select to `opacity: 0`. - Selection outlines yielded to nothing: `:where(...):focus-visible` has zero specificity, so an unconditional `[aria-pressed="true"]` outline always beat the focus ring. Both are now `:not(:focus-visible)`. - ward-management-network was the only one of four ward stylesheets with no forced-colors block; added. - patient-safety-plan: five sub-floor controls raised to `min-h-tap`. - Two decorative icons in ui-primitives given `aria-hidden`. Answer surface: - `AnswerCard` now requires a `support` prop with centrally-owned wording. `deriveTrust` returns "medium" both for ordinary answers and for a HIGH-RISK claim resting on unreviewed-authority evidence, and `weakEvidence` covers only "unsupported"/"low" - so that second case reached the reader as `ready`, visually identical to a verified answer. The distinction existed in `compactEvidenceSummary` but sat in a conditionally-rendered side card. Token layer: - `ckb-v2` is mounted unconditionally on <html> and outranks `:root`, so for any shared role the globals.css value is dead and editing it does nothing. Synced the four highest-consequence non-colour roles to the winning values and pinned them with a cross-file test. Gate gaps closed: - New `rawMarginLiterals` ratchet. Padding, radius, gap and line-height each had one; margin never did, leaving 31 literals counted by nothing. - New test: every fallback-less `var(--x)` must resolve to a declaration, ignoring conditional-theme blocks (this catches the `--shadow-overlay` class of bug at its source). - Baseline regenerated: tolerated debt 363 -> 339 with zero regressions, and the 50 units of recoverable slack are now gone. - GATES.md: qualified two rows that claimed broader enforcement than the rules actually have. - Removed 8 genuinely dead CSS classes; 5 more that the review called dead are retained - two are asserted by tests/helpers/style-contracts.ts and three are named in docs/process-hardening.md. Three others were grouped with LIVE selectors and were pruned by name rather than deleted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
Contributor
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:43 minutes Limit details: You’ve used the included review currently available. Your 87 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
…anch Immutable record, travelling with its owning product PR rather than a ledger-only branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
…iation Two real defects, and three findings that needed a decision recorded rather than a sweep. Defects: - caring-contact-shell-frame: the 250ms deferred focus hop was never cancelled, so on a fast unmount it fired against a torn-down tree and threw `document is not defined`. Held in a ref and cleared on unmount. This is the unhandled error that surfaced once during the full suite and would not reproduce in isolation. - guard-push `inFlightCiGuard` fails open when `gh` is absent, and that check ran BEFORE the injected `prViewer`/`runFetcher` were consulted - so the blocked-message test asserted nothing on any machine without the GitHub CLI, and failed there. `ghAvailable` is now injectable alongside the other two, defaulting to the real probe. The full suite now passes with no `gh` on PATH (7599 passed, exit 0), which it did not before. Decisions recorded (these three were deliberately not swept): - TOKENS.md gains §9 for the `--ward-*`/`--net-*`/`--co-*` family: 89 names across four stylesheets, 307 spacing references, 0 to the real semantic roles. §8 forbids a token without a usage rule, so leaving the family undocumented made that rule unenforceable rather than satisfied. Recorded as a scoped exception with the retirement path, because mapping a 1px-granular clinical grid onto `--gap-*`/`--pad-*` reflows the bed board and is the owner's visual call. - ckb-v2-tokens.css records how its 6-step type scale maps onto the `@theme` steps and Tailwind's defaults, including that `--text-sm` is 13px here against Tailwind's 14px. Deliberately NOT resolved by aliasing: pointing this file at `@theme` would invert the documented source-of-truth order. The `-lh`/`-tr` companions are noted as declared-but-unadopted (0 references) so their presence stops reading as evidence that leading is handled. - COMPONENTS.md §0.4 gains the per-primitive breakdown behind the existing aggregate: `Button` 12 importers against ui-primitives' 157, 112 production files still holding a raw `<button>`, and `Sheet` at 26 as the one genuinely adopted primitive. The split is not uniform and the reason is recorded - `Sheet` owns focus trap, portal and stacking, which a className cannot fake. That is the actual PR 12 question, stated rather than pre-empted. Also documents `support` as AnswerCard's third required prop in §9.13 and the behaviour list, so the binding contract matches the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
`advisory_ui_changed` stayed false when a component inside `src/components/caring-contacts/mockups/` changed, so the `@mockup` journeys - the only browser coverage those components have, since every production Playwright project excludes them by tag - ran nowhere. Two rules already existed and neither reaches this layout: one matches a TOP-LEVEL `*-mockups/` directory, the other a `*-mockups.tsx` FILE at any depth. A plain `mockups/` segment one level down, holding files with ordinary names, falls between them. That is 12 components today. This is precisely the failure the existing rule's own comment describes - "a mockup component edited on its own ... losing the only lane that runs its `@mockup` journey" - one directory level lower than it reaches. Adds a third pattern rather than widening either existing one, so their behaviour is untouched. Pinned in `selfTest` from both directions: a nested mockup file turns the lane on, and an ordinary component in the same parent directory leaves it off, so the pattern cannot quietly grow to cover real component directories. Verified against this PR's own file list: exactly one flag flips (advisory_ui_changed false -> true) and the other fifteen are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
Supersedes the earlier record, which named an ancestor SHA and predated the loose-end fixes and the nested-mockup CI scope routing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
BigSimmo
marked this pull request as ready for review
August 21, 2026 09:19
BigSimmo
enabled auto-merge
August 21, 2026 09:22
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:83507aeed3
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
Resolves a conflict in the guard-push pair, where PR #2209 landed the same fix independently: `ghAvailable` injected into `inFlightCiGuard` so the fail-open `gh --version` probe stops short-circuiting the injected fetchers. Took main's version of both files wholesale rather than hand-merging two spellings of one change. It is a strict superset of the local one: it also injects the probe into `selfTest`'s `mockBlockedGuard`, and adds a fails-open-when-gh-is-unavailable case whose fetchers throw if consulted. Nothing local was lost in substance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
Addresses the Codex P2 on the margin ratchet added earlier in this PR. The metric only ever saw the positive class utility, so three other spellings of the same defect cleared `check:design-system-contract` untouched: - `-mt-[22px]` negative utility — the regex had no optional `-` prefix - `[margin-top:22px]` arbitrary property — routes through `recordRawScaleLiteralProperty`, which had no margin branch - `margin-top: 22px` stylesheet — same missing branch Confirmed by probe before and after: all four spellings are now counted, and `margin: 0` / `margin: var(--gap-stack)` correctly remain uncounted. Wiring the detector was not enough on its own. `check-design-system-contract` records class-side and CSS-side debt through two separate `recordDebt` calls, and only the class-side one had been added — so the stylesheet half was detected and then silently dropped. Both are wired now, which is what turned the count from 31 to 74: 43 literal CSS margins that existed all along and no gate could see, 35 of them in the ward-management modules whose private spacing vocabulary TOKENS.md §9 now documents. Baseline regenerated at 382. No other metric moved. Pinned in `tests/design-system-contract-utils.test.ts` across all four spellings plus the zero and token exemptions, so a future narrowing of any one of them fails rather than quietly reducing coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014uzqrokJW55EN9vbZhFyXz
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #12781 (success). That run's conclusion is an aggregate and did not exercise Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
…scroll geometry The full-frontend design review remediation made `support` a required prop on AnswerCard, always rendering a short evidence-support label above the answer prose. Two ui-smoke.spec.ts phone geometry tests had calibrated thresholds from before that label existed, so the real added content height (a few px) tripped their strict scroll-geometry budgets in Production UI CI: - "phone short answer stays top-aligned with no phantom scroll into black": maxScrollTop 12 vs a permitted <= 8. The test already measures universal-also-matches as real content contributing to permitted overflow; the new answer-card-support label is measured the same way and added in. - "phone answer result keeps the edge dock and shared chrome synchronized on a short runway": postCollapseMaxOffset 74 vs an expected < 72. The support label's measured height is now added to that ceiling (and to the maxOffset upper bound) instead of leaving a stale pre-label constant. Both fixes measure the label's real rendered height rather than fudging a magic number, matching the existing pattern for other real content in the same tests.
Uh oh!
There was an error while loading. Please reload this page.
1 task
BigSimmo added a commit
that referenced
this pull request
Aug 21, 2026
This was referenced Aug 21, 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 freeto 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
Applies the smallest correct fix for each finding from a read-only design review of the whole frontend (240 production components, 75 routes, 834 files under the design-system contract), and closes the gate gaps that let them accumulate. Every gate was already green before this PR — the findings sat in the surfaces those gates structurally cannot parse.
Live defects
--shadow-overlaywas declared nowhere in the repository and referenced with no fallback by the sidebar Appearance menu (ClinicalSidebar.tsx:485). An unresolvedvar()with no fallback is invalid at computed-value time, sobox-shadowfell back tononeand the popover rendered flat. Declared, aliased to the elevated tier to match the other production popovers.--radius-pillwas also undeclared, working only because all 14 call sites independently wrote the same, 9999pxfallback. Declared.--danger-bg(#fef3f2) with the dark theme's--danger-solid(#b42318). Restored to the light value (#a3190f), lifting measured contrast from 6.05:1 to 7.15:1. The literals stay literals deliberately — the print stylesheet forces a white ground but not light token values, so tokenising would print the dark colour for a reader in dark mode — and a comment now records that.Accessibility
var(--spacing-tap). They were missed by the earlier 44→48 sweep because they are hand-rolled CSS-module heights rather than Tailwind utilities..bedStates spanis left at 44px — it is display-only, not a target..roleSelectremoved the only focus indication.outline: 0with no:focus-visiblereplacement anywhere meant a keyboard user tabbing to the role selector saw no change at all. The visible shell now owns the ring, scoped to the wrapper because the phone layout sets the select toopacity: 0.:where(...):focus-visiblecontributes zero specificity, so an unconditional[aria-pressed="true"]outline at (0,2,0) always won on the same property — focus was indistinguishable from selection on the routing screen. Both rules are now:not(:focus-visible).ward-management-network.module.csswas the only one of four ward stylesheets with no forced-colors block, on the screen whose routing cues most need to survive High Contrast Mode. Added.patient-safety-plan.tsxraised tomin-h-tap, including a phone-only tab switcher.ui-primitives.tsxgivenaria-hidden, matching the codebase's own 2,273-instance convention.Answer surface
AnswerCardnow requires asupportprop with centrally-owned wording.deriveTrustreturns"medium"both for an ordinary answer and for the case where a high-risk clinical claim rests on evidence whose authority was never reviewed — andweakEvidencecovers only"unsupported"/"low". That second case therefore reached the reader as{ kind: "ready" }, visually identical to a fully verified answer. The distinction already existed incompactEvidenceSummarybut lived in a conditionally-rendered side card, so it could vanish. Making the prop required is what stops it being hideable. Wording is unchanged from the existing copy; the label is text, never colour alone.Token layer
globals.cssandckb-v2-tokens.css.layout.tsx:118mountsckb-v2unconditionally on<html>, and.ckb-v2.ckb-v2(0,2,0) outranks:root(0,1,0), so the v2 value always wins and theglobals.cssvalue is dead — editing it has no visible effect, silently. The four highest-consequence non-colour roles (--text-hero,--text-hero--line-height,--leading-prose,--ease-standard, the last two differing by curve family) are synced to the winning values and pinned by a new cross-file test. Visually inert, since v2 was already what rendered.Gate gaps closed
rawMarginLiteralsratchet. Padding, radius, gap and line-height each had one; margin never did, leaving 31 literals counted by nothing.var(--x)must resolve to a declaration, ignoring conditional-theme blocks so a token declared only underforced-colorscannot satisfy it. This catches the--shadow-overlayclass of bug at its source.GATES.md: two rows qualified that claimed broader enforcement than the rules have.require-z-index-ladderwas marked unqualifiedimplemented-blockingbut only seesz-[N]literals in JS/TS —sidebar-live-shell.module.css:144carries an off-ladderz-index: var(--z-sidebar-flyout, 50)it cannot see.no-hardcoded-hexmatches onlybg-/text-/border-bracket-hex.tests/helpers/style-contracts.tsand three are named indocs/process-hardening.md. Three others turned out to be grouped with live selectors and were pruned by name rather than deleted.Verification
npm run verify:pr-local— exit 0, all 19 selected gates completed, none failed:check:runtime, check:installed-lock-parity, format:changed, sitemap:check, docs:check-index, docs:check-inventory, docs:check-scripts, docs:check-links, check:branch-review-ledger, check:outstanding-issues, check:ledger-write-discipline, lint, typecheck, test, build, eval:rag:offline, eval:rag:adversarial:offline, check:medication-interactions, check:medication-lexicon-report. Unit suiteTests 7599 passed | 4 skipped (7603), zero errors. Build generated 1871 static pages.Adversarial fixture contract passed (24 synthetic cases, 8 categories, 6 canaries).npm run verify:cheapwas used as the iteration gate.Design-system contract passed (834 production files; ... sub-floor interactive min-heights 33; ...);Scale ratchets: raw padding literals 52; raw radius literals 20; raw gap literals 25; raw margin literals 31; raw line-height literals 3.#255) in this environment —check:playwright-browser-revisionreports chromium 1234 expected, 1194 installed. PerAGENTS.md, a mismatched Chromium path must not be forced; browser proof is delegated to CI Production UI. This PR changesglobals.css, ward-management CSS modules, tap-target heights and a forced-colors block, so Production UI is the gate that matters here and should be green before merge.npm run verify:releasenot run — not a release/handoff confidence claim.npm run check:production-readinessnot run: no ingestion, privacy, environment, Supabase, source-governance or deployment behaviour changed. The clinical-output change is presentational (an additional required label on an existing card).eval:retrieval:quality,eval:rag,eval:quality) not run and not applicable: no retrieval, ranking, selection, chunking or scoring behaviour changed. The offline RAG fixture gates ran and passed as part ofverify:pr-local.New guards were each proven to fail before being trusted: reverting
--leading-prosemade the cross-layer test fail with the intended message, and removing the--shadow-overlaydeclaration made the resolution test nameClinicalSidebar.tsx:485specifically.Risk and rollout
AnswerCard, which adds a short line of text to every answer. The dead-CSS removal is the residual risk worth a reviewer's eye: three rules were grouped with live selectors and were pruned by name, so the live siblings (.medication-results-inset,.smart-search-prompt-row,.smart-search-rotating-text,[data-therapy-scroll-sm]) are asserted present but were not visually diffed.git revertof the single commit restores every surface. No migration, no data change, no config change. The regeneratedscripts/design-system-contract-baseline.jsonand.design-sync/config.jsonrevert with it.src/lib/rag/, no retrieval RPC, no ranking or selection surface, no golden fixture, and no eval-harness file is touched.classifyPullRequestFilesreportsragRanking: falsefor this diff.src/lib/answer-render-policy.tswas read during the review but is unmodified.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)On the last item specifically: the answer-surface change makes an existing, already-computed trust distinction visible rather than introducing a new clinical determination. It adds caution-relevant information (an answer resting on unreviewed-authority evidence for a high-risk claim no longer reads as fully verified) and removes none, so it moves conservatively. The
unconditional support labeloption was chosen deliberately over routingmediuminto the amberweakEvidencebanner:deriveTrustalso returns"medium"as its generic non-high-confidence fallback, so treating all"medium"as a caution would have alarmed on most answers and degraded the banners that do carry specific meaning.Notes
#b42318on#fef3f2is 6.05:1 and#a3190fon#fef3f2is 7.15:1 (both pass AA; only the corrected pair passes AAA).ui-primitives.tsxagainst 12 that import the registeredButton); the private--ward-*/--net-*/--co-*token system (~90 unregistered names, 307 spacing references); and the two overlapping type scales. Each needs a decision recorded, not a sweep.tests/guard-push.test.tsassumes theghCLI is installed —inFlightCiGuardfails open when it is absent, so the test fails on any machine withoutgh. Proven environmental by re-running with a stubghonPATH(41/41 pass). Not this PR's to fix.Generated by Claude Code