Claude/clinical kb design system 333a69 - #1546
Conversation
Design-system scope only. Nothing is adopted by a product surface: the new components are built and tested but unimported, and the v2 token layer is class-scoped to `.ckb-v2` so importing it cannot repaint any surface that does not opt in. Correctness: - Fix a P1 browser crash. `source-metadata.ts` imported the server logger, whose `activeLevel()` reads `process.env.LOG_LEVEL` — a ReferenceError in a browser, so any off-vocabulary metadata value unmounted the whole React tree instead of falling back. Drops the import, adds a browser-safe diagnostic seam, and guards the env read in `logger.ts` as defence in depth. - Type the three source badges' `metadata` prop. It was `unknown` while the published `.d.ts` promised a shape, so the wrong key compiled cleanly. - Encode the disabled state in `controlBase` instead of `opacity-50`, which dimmed fill and label together and dropped secondary labels below 4.5:1. - Move `fieldLabel` off `--text-soft` (3.07:1) and the uppercase eyebrow treatment. - Delete `ui/card.tsx` and `ui/badge.tsx` — 7 exports, 0 importers, colliding by name with Chip and AnswerCard. Components (built, not adopted): Button, TextField, SearchField, Chip, Toast, Tabs, Tooltip, Pagination, ConfirmDialog, PageHeader, Breadcrumb, AnswerCard, DoseLine, AnswerFooter, Quantity, StatusMark, Citation, the four Link variants, Select, Checkbox, RadioGroup, Disclosure, Progress, StageList. AccessibleTable: per-column numeric alignment, `aria-controls` on the expander, sticky header when expanded, and an explicit unverified-extraction treatment. Verification: tsc pass, lint pass, prettier clean, verify:ui 344 passed, design-system contract pass. Unit 4689 passed / 1 failed / 3 skipped — the failure is `installed-lock-parity`, environmental (brace-expansion 1.1.16 vs lockfile 1.1.18), tracked as issue #149. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…all-through, leading step
C1 reconciliation per docs/design-system/TOKENS.md - the design-side token file
proved unrecoverable (design project 08d6f126 was last updated 2026-07-13; the
31 July copy was never written to it), so the repo file becomes the canonical
reconciled copy:
- Rename the two v2 --shadow-inset overrides to --shadow-well (recessed-well
role). The DS bevel --shadow-inset is inherited from globals.css again,
restoring the bevel highlight to its ~40 consumers.
- Update the token contract test in the same commit: pin --shadow-well as a
true inset and assert --shadow-inset is not redeclared in the v2 layer.
- Author the evidence-spine and status-mark role families repo-side, derived
from existing roles (spine from rule/accent/warning, status-mark from the
gutter dot and StatusMark's previously-inline stroke). Confidence-meter is
deliberately absent: no call site exists yet.
- Add --clinical-chat-document to the v2 dark block so it resolves against the
v2 dark inset; custom properties inherit computed values, so the live .dark
declaration cannot re-resolve inside the subtree.
- answer-card.tsx: use the named leading-prose step instead of the arbitrary
leading-[var(--leading-prose,1.65)], which tripped the design-token contract
('has no arbitrary leading-[...] left in production') and restated a token
value in markup.
Verified: vitest tests/ckb-v2-token-contract.test.ts tests/design-token-contract.test.ts
-> 47 passed (47). No adopted surface exists, so there is no visual change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…st, AsyncButton type Playbook PR 1+2 and the smallest slices of PR 3+4 (docs/design-system/SPEC.md §13): - Cascade: light is the default on .ckb-v2 itself; dark overrides via .dark .ckb-v2, .ckb-v2.dark. Never :not(.dark) (DS-01) - it matched a subtree inside html.dark and overrode inherited dark values. Contract test's block parser updated in the same commit (collects both .ckb-v2 blocks and the grouped dark selector). - Forced colours: v2 layer ships its own forced-colors block over all three selectors, last in file (C4/DS-02); roles map to system colours, elevation flattens to none. - Button.danger pairs --danger-solid-contrast (DS-03; dark was 2.87:1, the paired token is 6.57:1 and HCM-mapped). - AsyncButton defaults type=button so it can no longer submit a surrounding form (DS-04 adjacent; retirement to Button remains PR 4). Verified: vitest ckb-v2-token-contract + design-token-contract + ui-v2-components.dom + ui-primitives.dom -> 67 passed (67). No adopted surface exists; no visual change ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…sign-system-333a69 # Conflicts: # tests/source-metadata.test.ts
…push) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the cascade port The port made the light .ckb-v2 block match inside dark subtrees, so roles the dark block deliberately left to fall through (--text, --text-heading, --disabled, glows, backdrop, accent-soft/border, primary-soft) resolved to the LIGHT values - near-black text on the dark ramp. Re-declared in the dark block, mirroring the live .dark layer they previously inherited, and added a contract regression: dark --text/--text-heading must clear 4.5:1 on the dark surface. Found by adversarial self-review; the prior 74/74 pass had no dark body-ink assertion. Zero adopted surface, so no shipped visual change either way. Co-Authored-By: Claude Fable 5 <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. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
📝 WalkthroughWalkthroughThe PR defines dark-theme token values, adds token contract tests, updates tab and tooltip accessibility behavior, expands UI DOM tests, and records policy and branch review details. ChangesDesign-system validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
|
@cursoragent Work the current open PR end-to-end. Confirm the PR number and GitHub head first from context. If more than one open PR could apply, stop and say which one you would use and why. Fetch and start from the remote tip that matches that GitHub head. If the named branch ref is missing or stale, use the PR head ref. Preserve unrelated local WIP, including any local-only ledger commits; do not discard dirty work, and do not treat a local-ahead commit as the reviewed tip. Do not merge the PR, force-push, rebase, or arm auto-merge unless I explicitly ask. No provider-backed gates without separate approval. If you cannot push or resolve threads, diagnose and comment only; if inline replies fail, resolve when possible and put dispositions in the summary comment. If auto-merge is already armed, push only for a real blocker, and avoid pushes that would cancel in-flight required CI unless the push itself clears that blocker. If the PR is already merged or closed: confirm the head and merge commit, note required-CI outcome, post one summary, and stop. Goal: deep review plus Bugbot, fix actionable issues with the smallest correct changes, clear merge / required-CI / thread blockers, run strong local offline verification, push fixes, append the review ledger, and post one PR summary. Prefer thoroughness over speed. Regenerate large assets only when a fix requires it; then run the asset check and keep compatibility aliases byte-identical where the repo uses them. Snapshot the GitHub head SHA: tip, base, behind/ahead, mergeable state, merge-tree versus origin/main (real conflict versus behind-but-clean), required checks on that tip including Production UI when selected, advisory separately, unresolved actionable threads. Missing checks while dirty are not green. If the tip moves mid-work, re-snapshot and continue from the new head. Ledger-lookup against that GitHub head under the heavy review-and-fix scope for this PR. Already reviewed at this head with clean merge-tree, green required checks, and no new actionable threads → summarize, comment, stop unless I asked for a fresh superseding pass. Follow the repo review protocol. Unblock once: real conflict → merge origin/main (prefer main’s shared queues; keep this PR’s notes); behind-but-clean → one sync, late if required CI is in flight; clean → leave. Labels like skip-branch-sync do not block a manual sync needed to clear a real blocker. No sync thrash. After any sync or push, re-snapshot tip, merge-tree, and required checks before declaring done. Dedupe the ledger if a merge touched it. Review high-confidence delta risks only. Separate PR-introduced defects from pre-existing re-emitted debt. For generated assets, review contracts, aliases, cache, and manifests—not every generated line. If protected RAG or ranking surfaces are touched, say so before editing; fix PR-body policy text only when wrong or missing. Ignore bot noise. No nit spam or broad rewrites. Escalate verification by touched risk: clinical / RAG / privacy / migrations / auth → domain check plus production-readiness when warranted; UI / phone-chrome / routing / styling → ensure plus phone-chrome or UI gates when warranted; generated assets or docs inventory/links → asset or docs checks when warranted. Fix P0/P1 always; clear scoped P2 when locally provable; else disposition and resolve. Required-check failures on this tip only; ignore advisory. Prefer reverting a bad autofix commit. After fixes: format and commit before push; smallest targeted proof; repo cheap gate; repo PR-local gate. No release, lighthouse, live eval, or live provider gates without approval. Push only this PR’s fix commits. Append ledger for the final GitHub head under the heavy scope. Never push a ledger-only tip—include ledger in a real fix push, or leave a no-change ledger append local and unpushed. One PR comment: tip, sync/merge-tree, fixed versus dispositioned, threads, required CI, decisive local gate lines, residual risks; merge left to me. Inline only for remaining human-needed P0/P1. Stop when merge-tree is clean on the current GitHub tip, actionable threads are clear, required checks are green or clearly in progress, heavy gates for scope passed, summary posted, merge left to me. |
|
PR #1546 worked end-to-end at tip Snapshot
Fixed
Local gates
CIPolicy/mergeability green; Static/Build/Unit/Production UI in progress. Residual
Merge left to you.   |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…sign-system-333a69 Resolve DIRTY conflicts for PR #1546: - Keep this tip's dark-ink / companion-role re-declarations in ckb-v2-tokens and the matching contract regression (cascade-port fall-through fix). - Prefer main's post-#1538/#1541 component copies (type-scale, aria-hidden, z-index ladder) and main's transparent HCM overlay-backdrop rationale. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
- Tabs: stop pointing unselected tabs at missing panel IDs. - Tooltip: compose mouse/focus/keydown handlers with the child's own. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
.design-sync/previews/AsyncButton.tsx-7-21 (1)
7-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an observable action to enabled preview controls.
IdleandSecondaryrender enabled buttons without a handler, form action, or navigation. Users can activate these controls without a result. Add a focused preview action, or use the disabled-placeholder pattern.As per coding guidelines, “Every interactive
<button>must have an action, submit behavior, navigation, or the explicit disabled-placeholder pattern.”🤖 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 @.design-sync/previews/AsyncButton.tsx around lines 7 - 21, Update the Idle and Secondary AsyncButton previews to provide an observable action, such as a focused preview click handler or navigation/form action; alternatively, apply the explicit disabled-placeholder pattern. Ensure every enabled button rendered by these examples produces a result when activated.Source: Coding guidelines
.design-sync/previews/Tabs.tsx-11-29 (1)
11-29: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the tab previews update their selection.
Each
onChangecallback discards the selected tab ID. The enabled tabs do not change the active tab or panel. Store the selected ID in preview state, or present the controls as disabled placeholders.As per coding guidelines, “Every interactive
<button>must have an action, submit behavior, navigation, or the explicit disabled-placeholder pattern.”🤖 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 @.design-sync/previews/Tabs.tsx around lines 11 - 29, Update the Underlined, Segmented, and WithPanel preview components so their Tabs controls handle selection changes: store each selected tab ID in local preview state and pass that state as value, preserving the existing initial selections and panel behavior. Alternatively, explicitly mark these controls disabled if they are intended as placeholders, rather than leaving no-op onChange callbacks.Source: Coding guidelines
.design-sync/previews/PageHeader.tsx-12-15 (1)
12-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an action to the
Add notepreview.Line 13 renders an enabled
Buttonwithout a handler, submit behavior, or navigation. Add a preview-safe handler, or use the disabled-placeholder pattern if this story is intentionally noninteractive.As per coding guidelines, every interactive button must have an action, submit behavior, navigation, or the explicit disabled-placeholder pattern.
🤖 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 @.design-sync/previews/PageHeader.tsx around lines 12 - 15, The Button component with icon={Plus} and "Add note" text in the actions prop is rendered as interactive without a handler, submit behavior, or navigation, which violates the interactive-button guideline. Either add a preview-safe onClick handler to the Button that demonstrates the intended interaction, or apply the disabled-placeholder pattern by adding disabled={true} to the Button if this preview is intentionally noninteractive.Source: Coding guidelines
.design-sync/previews/IconButton.tsx-6-14 (1)
6-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd actions to enabled icon-button previews.
Lines 8–10 and 14 render enabled
IconButtoncontrols without handlers or navigation. These controls are activatable but do nothing in a browser preview. Add preview-safe handlers, or use the disabled-placeholder pattern for noninteractive examples.As per coding guidelines, every interactive button must have an action, submit behavior, navigation, or the explicit disabled-placeholder pattern.
🤖 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 @.design-sync/previews/IconButton.tsx around lines 6 - 14, The enabled IconButton instances in the Toolbar component (Copy, Download, X icons) and the Floating component (Trash2 icon) do not have handlers or the disabled-placeholder pattern applied. Add preview-safe onClick handlers to each IconButton (such as console.log statements), or apply the disabled-placeholder pattern by setting disabled={true} to mark them as non-interactive examples. This ensures compliance with the requirement that every interactive button must have an action or use the explicit disabled-placeholder pattern.Source: Coding guidelines
.design-sync/previews/Button.tsx-4-18 (1)
4-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd an action to enabled button previews.
Lines 6–16, 22–30, and 53–55 render enabled
Buttoncomponents without a handler, submit behavior, or navigation. These controls are activatable but produce no result in a browser preview. Add a preview-safe action, or use the disabled-placeholder pattern when a story must be noninteractive.As per coding guidelines, every interactive button must have an action, submit behavior, navigation, or the explicit disabled-placeholder pattern.
Also applies to: 20-32, 51-57
🤖 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 @.design-sync/previews/Button.tsx around lines 4 - 18, Add onClick handlers to each enabled Button component in the Variants export. Attach preview-safe action handlers such as console logging or a no-op function to every Button instance (the "primary" Add source button, "secondary" Review later button, "toolbar" Export button, "ghost" Cancel button, and "danger" Delete source button). Alternatively, if any of these buttons are meant to be non-interactive for preview purposes only, mark them explicitly as disabled instead. Every interactive Button must have either a handler, submit behavior, navigation, or be disabled with the placeholder pattern.Source: Coding guidelines
.design-sync/previews/SearchField.tsx-9-12 (1)
9-12: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
WithClearstateful.SearchFieldpassesvaluedirectly to the input, so line 11 renders a read-only controlled field withoutonChange. The no-oponClearalso leaves the value unchanged. Add state withonChangeandonClear, or usedefaultValueand removeonClearfor a static story.🤖 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 @.design-sync/previews/SearchField.tsx around lines 9 - 12, Update the WithClear story to use a stateful value for SearchField, wiring onChange to update that state and onClear to reset it so the controlled input remains editable and clearable.src/components/ui/tabs.tsx-95-98 (1)
95-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint
aria-controlsonly at a panel that exists.Line 97 sets
aria-controlson every tab, but Line 125 renders one panel for the selectedvalue, and only whenchildrenis supplied. Non-selected tabs therefore reference an absent element, and withchildrenomitted every reference dangles. Set the attribute only when the panel is present.♻️ Proposed fix
- aria-controls={`${baseId}-panel-${item.id}`} + aria-controls={children && selected ? `${baseId}-panel-${item.id}` : undefined}Also applies to: 122-132
🤖 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/tabs.tsx` around lines 95 - 98, Update the tab rendering around the tab element and selected panel block to set aria-controls only when the corresponding panel is actually rendered: children must be supplied and the tab’s item.id must equal the selected value. Otherwise omit the attribute while preserving the existing panel rendering behavior.src/components/ui/citation.tsx-58-64 (1)
58-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the static citation a role so
aria-labelapplies.Browsers ignore
aria-labelon a generic<span>that carries no role. In the non-interactive branch the computed status phrase from Line 54 is therefore dropped, and assistive technology reads only the visible text. Addrole="img"orrole="group", or render the accessible name assr-onlytext.🛠 Proposed fix
- <span data-testid="citation" aria-label={accessibleName} className="inline-flex w-fit items-center"> + <span data-testid="citation" role="img" aria-label={accessibleName} className="inline-flex w-fit items-center">🤖 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/citation.tsx` around lines 58 - 64, Add an explicit accessible role to the non-interactive citation `<span>` returned by the `Citation` component so its existing `aria-label={accessibleName}` is honored; use `role="img"` or `role="group"` while preserving the current markup and behavior.src/components/ui/link.tsx-113-113 (1)
113-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
LinkActionacceptstonebut ignores it.The props type extends
BaseProps, sotoneis part of the public API. The implementation hardcodes the accent styling. A caller that passestone="inherit"gets no effect and no type error. Either applytoneor omit it from the type.♻️ Proposed fix
-export function LinkAction({ href, children, className }: BaseProps & { href: string }) { +export function LinkAction({ href, children, className }: Omit<BaseProps, "tone"> & { href: string }) {🤖 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/link.tsx` at line 113, Update LinkAction to either apply the BaseProps tone value to its rendered styling or remove tone from the accepted props type; do not leave the public tone API ignored while hardcoding accent styling.src/components/ui/progress.tsx-115-115 (1)
115-115: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse an allowed z-index rung.
z-[1]is outside the repository ladder. Replace it withz-5, the lowest allowed non-zero rung, so the connector still sits under the stage marker.🛠 Proposed fix
- <span className="relative z-[1] grid w-[var(--gutter-col,1.25rem)] shrink-0 place-items-center pt-0.5"> + <span className="relative z-5 grid w-[var(--gutter-col,1.25rem)] shrink-0 place-items-center pt-0.5">🤖 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/progress.tsx` at line 115, Replace the arbitrary z-index value z-[1] in the span className with z-5, which is the allowed z-index rung from the repository's z-index ladder. This maintains the intended layering where the connector sits under the stage marker.Source: Linters/SAST tools
src/components/ui/choice.tsx-86-86 (1)
86-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
aria-checkedfrom the native checkbox input.ARIA in HTML forbids
aria-checkedoninput[type="checkbox"]. Therefcallback at Line 87 already setsnode.indeterminate, which exposes the mixed state to assistive technology through the native:indeterminatestate. The extra attribute adds a conflicting source of truth and fails ARIA validators.♻️ Proposed change
disabled={disabled} aria-describedby={descId} - aria-checked={indeterminate ? "mixed" : undefined} ref={(node) => {🤖 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/choice.tsx` at line 86, Remove the aria-checked attribute from the native checkbox input in the choice component, leaving the existing ref callback’s node.indeterminate assignment as the sole mixed-state mechanism..design-sync/conventions.md-85-90 (1)
85-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the component migration guidance consistent with the example.
This section says
Buttonsupersedes raw<button>recipes. The adjacent example still recommends a raw<button>without behavior. Replace that example with the supportedButtonusage, or remove the control from the standalone example.🤖 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 @.design-sync/conventions.md around lines 85 - 90, Update the adjacent standalone example in the component migration guidance to use the supported Button component instead of a behaviorless raw <button>, or remove that control from the example; keep the surrounding migration guidance consistent with the Button recommendation.tests/source-badges-off-vocab.dom.test.tsx-26-29 (1)
26-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlways restore stubbed environment variables in
afterEach.If the assertion on Line 68 fails, Line 70 does not run.
NODE_ENVthen remains"development"and can change later test behavior. Restore environment stubs in the existing cleanup hook.Proposed fix
afterEach(() => { + vi.unstubAllEnvs(); vi.restoreAllMocks(); });Also applies to: 62-70
🤖 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 `@tests/source-badges-off-vocab.dom.test.tsx` around lines 26 - 29, Update the existing afterEach cleanup in the source-badge test to restore stubbed environment variables, including NODE_ENV, regardless of assertion failures; use the test framework’s environment-stub restoration alongside vi.restoreAllMocks().tests/source-metadata-browser-safety.test.ts-19-25 (1)
19-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDetect relative logger imports in the regression check.
Line 24 rejects only
@/lib/logger. A future./loggeror../lib/loggerimport will restore the client dependency and still pass this test. Match all logger module specifiers.Proposed fix
- expect(source).not.toMatch(/from\s+["']@\/lib\/logger["']/); + expect(source).not.toMatch(/\b(?:from|import\s*\()\s*["'](?:[^"']*\/)?logger["']/);🤖 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 `@tests/source-metadata-browser-safety.test.ts` around lines 19 - 25, Update the regex pattern in the expect().not.toMatch() assertion on line 24 to detect all logger module imports, not just the `@/lib/logger` alias. Expand the pattern to also match relative import specifiers like ./logger and ../lib/logger so the regression test catches accidental logger imports regardless of the import path used.
🧹 Nitpick comments (8)
src/components/ui/toast.tsx (2)
87-94: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPause the auto-dismiss timer on hover and focus.
The default duration is 6000ms. The timer runs unconditionally. If a user hovers or focuses the toast to read it or to reach the dismiss control, the card still disappears, and focus moves to
bodywhen the focused button unmounts. Pause the timer while the toast has pointer or keyboard attention.♻️ Proposed fix
function ToastCard({ toast, onDismiss }: { toast: Toast; onDismiss: (id: string) => void }) { const duration = toast.duration ?? DEFAULT_DURATION; + const [paused, setPaused] = useState(false); useEffect(() => { - if (duration <= 0) return; + if (duration <= 0 || paused) return; const timer = setTimeout(() => onDismiss(toast.id), duration); return () => clearTimeout(timer); - }, [duration, onDismiss, toast.id]); + }, [duration, onDismiss, paused, toast.id]);<div data-testid="toast" data-tone={toast.tone} + onMouseEnter={() => setPaused(true)} + onMouseLeave={() => setPaused(false)} + onFocusCapture={() => setPaused(true)} + onBlurCapture={() => setPaused(false)}🤖 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/toast.tsx` around lines 87 - 94, Update ToastCard’s auto-dismiss behavior so the timer pauses while the toast is hovered or focused, including focus within its dismiss control, and resumes with the remaining duration when attention ends. Use the toast card’s pointer/focus state in the existing useEffect timer lifecycle, preserving immediate dismissal for non-positive durations and cleanup on unmount.
73-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevent two live regions when a surface places its own
ToastRegion.
ToastProvideralways renders<ToastRegion />on Line 76. The comment on Lines 128-133 tells callers that a surface owning its portal can placeToastRegionitself. If a caller does that inside the provider, two regions render the same toast list, and assistive technology announces each toast twice.Add an opt-out so the provider can skip its own region.
♻️ Proposed fix
-export function ToastProvider({ children }: { children: ReactNode }) { +export function ToastProvider({ children, renderRegion = true }: { children: ReactNode; renderRegion?: boolean }) {<ToastContext.Provider value={value}> {children} - <ToastRegion /> + {renderRegion ? <ToastRegion /> : null} </ToastContext.Provider>Also applies to: 128-137
🤖 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/toast.tsx` around lines 73 - 78, Add an opt-out prop to ToastProvider that allows callers owning their own portal to suppress the provider’s built-in ToastRegion; use this prop in the render block around ToastContext.Provider so the default region remains rendered when the opt-out is not enabled, and update the related provider API/type definition and usage documentation near ToastRegion.tests/ui-v2-components.dom.test.tsx (2)
124-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the Escape close so the test matches its name.
The test name states the tooltip opens on keyboard focus. The body verifies only that path. The component also closes on Escape, which is the documented keyboard contract and is currently untested.
♻️ Proposed addition
trigger.focus(); const tooltip = await screen.findByRole("tooltip"); expect(trigger.getAttribute("aria-describedby")).toBe(tooltip.id); + + await userEvent.keyboard("{Escape}"); + expect(screen.queryByRole("tooltip")).not.toBeInTheDocument();🤖 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 `@tests/ui-v2-components.dom.test.tsx` around lines 124 - 141, Extend the Tooltip test to dispatch an Escape key event after verifying it opens on focus, then assert the tooltip is no longer present. Keep the existing focus and aria-describedby assertions unchanged so the test covers both keyboard opening and Escape closing.
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
Select.The suite covers ten components but not
Select.Selectcombinesvalue,defaultValue, and a placeholder option, which is where the controlled/uncontrolled conflict flagged insrc/components/ui/select.tsxLines 60-61 appears. A test that renders a controlledSelectwith aplaceholderand asserts no React warning would lock that contract.I can generate the test. Do you want me to add it?
🤖 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 `@tests/ui-v2-components.dom.test.tsx` around lines 1 - 16, Add Select coverage to the component suite, importing Select from its UI module and adding a test that renders it with a controlled value and placeholder, then verifies the expected rendered state while asserting React emits no controlled/uncontrolled warning. Use the existing test conventions and spies to restore warning mocks after the test.tests/accessible-table-alignment.dom.test.tsx (2)
79-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the
matchMediastub after this test.
setMatchMedia(true)mutates global state and is never reset. Any test added after this one in the file inherits mobile mode, which silently changes the alignment expectations. Reset it in anafterEachblock.♻️ Proposed change
describe("AccessibleTable expander wiring", () => { + afterEach(() => { + setMatchMedia(false); + }); + it("points aria-controls at the dialog it opens", async () => {Add
afterEachto thevitestimport at Line 3.🤖 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 `@tests/accessible-table-alignment.dom.test.tsx` around lines 79 - 98, Add an afterEach cleanup for the global matchMedia stub used by setMatchMedia(true) in the AccessibleTable expander wiring tests, and import afterEach from vitest. Restore the default matchMedia behavior after each test so subsequent alignment tests do not inherit mobile mode.
23-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the broken checked-state selectors.
The alignment coverage here is good. The related
CheckboxandRadioGroupcomponents insrc/components/ui/choice.tsxhave no assertion that the tick or the radio dot becomes visible, which is how thepeer-checked:defect reached this PR. Add that assertion intests/ui-v2-components.dom.test.tsx.🤖 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 `@tests/accessible-table-alignment.dom.test.tsx` around lines 23 - 77, Add DOM assertions in the Checkbox and RadioGroup coverage in tests/ui-v2-components.dom.test.tsx to verify the checked-state tick and radio dot become visible when selected. Exercise the checked interaction/state and assert the relevant peer-checked visibility classes or rendered state, targeting the Checkbox and RadioGroup implementations in choice.tsx.src/components/ui/disclosure.tsx (1)
66-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider making the heading level configurable.
The trigger is always wrapped in
<h3>. A page that places a disclosure directly under an<h1>, or inside an<h3>section, then gets a broken heading outline. Add aheadingLevelprop withh3as the default.🤖 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/disclosure.tsx` around lines 66 - 89, Update the disclosure component’s heading wrapper to support a configurable headingLevel prop, defaulting to h3, and render the trigger with that selected heading level instead of always using h3. Preserve the existing trigger content and behavior while allowing callers to choose the appropriate heading outline level.src/components/ui-primitives.tsx (1)
522-539: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood fallback fix; consider a regression test.
The
body ?? descriptionfallback correctly resolves the silent-render bug described in the comment, and prioritizesbodyas documented. Since this was previously a silent failure with no type error, a small unit test assertingEmptyStaterendersdescriptionwhenbodyis omitted would guard against a future regression when the alias is eventually removed.Also applies to: 571-571
🤖 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-primitives.tsx` around lines 522 - 539, Add a focused unit test for EmptyState that omits body, supplies description, and asserts the description text renders; keep the existing body-priority behavior covered if applicable.
🤖 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 @.design-sync/conventions.md:
- Around line 36-38: Update the tap-target convention in the documented utility
contract from 44px to the repository-required 48px size, using min-h-12 and
corresponding h-tap/w-tap guidance. Preserve the distinction that the contract
applies only to interactive controls, not static chips.
In @.design-sync/previews/AnswerCard.tsx:
- Around line 21-24: Update the paragraph in AnswerCard to remove the unsourced
clozapine dosing and monitoring recommendation, replacing it with neutral
placeholder content or a validated linked citation. Preserve the preview’s
conservative behavior by ensuring the answer does not present unverifiable
clinical guidance.
In @.design-sync/previews/AsyncButton.tsx:
- Around line 7-13: Update both Upload icon instances in the AsyncButton
examples, including the idle and busy states, to mark them as decorative for
assistive technology using the icon’s supported accessibility prop. Preserve the
existing button labels and visual icon styling.
In `@docs/clinical-badge-system-guide.md`:
- Around line 65-77: Align the clinical_validation_status documentation with the
implementation in source-metadata normalization: either add unknown to the
documented supported values, or remove unknown from knownValidation and update
affected tests so it falls back to unverified with diagnostics. Keep the guide,
normalization behavior, and tests consistent under the chosen contract.
In `@src/components/ui/chip.tsx`:
- Around line 64-73: The remove control button has insufficient tap target size
at h-5 w-5 (20px) versus the required 48px minimum. Extend the clickable hit
area of the button element containing the X icon by adding padding so the total
button reaches min-h-12 and min-w-12 (48px). Keep the X icon glyph itself at h-3
w-3 to preserve the visual design, and ensure the chip's overall height does not
change by using appropriate padding values.
In `@src/components/ui/choice.tsx`:
- Around line 104-110: The peer-checked:opacity-100 selector on the Check
component and the radio dot cannot match because they are children of the visual
box span, not siblings of the peer input. Update the parent span containing the
Check icon to add peer-checked:[&>[data-mark]]:opacity-100 to its className, add
data-mark attribute to both the Check component and the radio dot element
(around line 183), remove peer-checked:opacity-100 from their classNames, and
add aria-hidden="true" to the Check component.
In `@src/components/ui/citation.tsx`:
- Around line 121-132: Update the li element that wraps each citation in the map
function to preserve list semantics for accessibility. Remove the contents
className from the li element and instead apply appropriate flex layout
properties directly to the li to maintain the visual flex layout while ensuring
screen readers can properly report the list structure and item count.
In `@src/components/ui/select.tsx`:
- Around line 57-61: Update the select element in the component’s JSX so
defaultValue is assigned only when value === undefined; preserve the
placeholder-derived empty default for uncontrolled selects while passing no
defaultValue for controlled selects.
In `@src/components/ui/tooltip.tsx`:
- Around line 30-39: Update the trigger props in the cloneElement call to
compose existing child onMouseEnter, onMouseLeave, onFocus, onBlur, and
onKeyDown handlers before applying the tooltip state changes, while preserving
Escape-to-close behavior. Merge the child’s existing aria-describedby value with
the tooltip id instead of replacing it.
In `@src/lib/source-metadata.ts`:
- Around line 14-26: Update sourceMetadataDiagnostics.warn to omit the
unvalidated value from the default console.warn payload while preserving the
test-spy seam and warning context. Revise the associated guide text to no longer
promise field-and-value diagnostics, keeping only the safe field-level
diagnostic behavior.
---
Minor comments:
In @.design-sync/conventions.md:
- Around line 85-90: Update the adjacent standalone example in the component
migration guidance to use the supported Button component instead of a
behaviorless raw <button>, or remove that control from the example; keep the
surrounding migration guidance consistent with the Button recommendation.
In @.design-sync/previews/AsyncButton.tsx:
- Around line 7-21: Update the Idle and Secondary AsyncButton previews to
provide an observable action, such as a focused preview click handler or
navigation/form action; alternatively, apply the explicit disabled-placeholder
pattern. Ensure every enabled button rendered by these examples produces a
result when activated.
In @.design-sync/previews/Button.tsx:
- Around line 4-18: Add onClick handlers to each enabled Button component in the
Variants export. Attach preview-safe action handlers such as console logging or
a no-op function to every Button instance (the "primary" Add source button,
"secondary" Review later button, "toolbar" Export button, "ghost" Cancel button,
and "danger" Delete source button). Alternatively, if any of these buttons are
meant to be non-interactive for preview purposes only, mark them explicitly as
disabled instead. Every interactive Button must have either a handler, submit
behavior, navigation, or be disabled with the placeholder pattern.
In @.design-sync/previews/IconButton.tsx:
- Around line 6-14: The enabled IconButton instances in the Toolbar component
(Copy, Download, X icons) and the Floating component (Trash2 icon) do not have
handlers or the disabled-placeholder pattern applied. Add preview-safe onClick
handlers to each IconButton (such as console.log statements), or apply the
disabled-placeholder pattern by setting disabled={true} to mark them as
non-interactive examples. This ensures compliance with the requirement that
every interactive button must have an action or use the explicit
disabled-placeholder pattern.
In @.design-sync/previews/PageHeader.tsx:
- Around line 12-15: The Button component with icon={Plus} and "Add note" text
in the actions prop is rendered as interactive without a handler, submit
behavior, or navigation, which violates the interactive-button guideline. Either
add a preview-safe onClick handler to the Button that demonstrates the intended
interaction, or apply the disabled-placeholder pattern by adding disabled={true}
to the Button if this preview is intentionally noninteractive.
In @.design-sync/previews/SearchField.tsx:
- Around line 9-12: Update the WithClear story to use a stateful value for
SearchField, wiring onChange to update that state and onClear to reset it so the
controlled input remains editable and clearable.
In @.design-sync/previews/Tabs.tsx:
- Around line 11-29: Update the Underlined, Segmented, and WithPanel preview
components so their Tabs controls handle selection changes: store each selected
tab ID in local preview state and pass that state as value, preserving the
existing initial selections and panel behavior. Alternatively, explicitly mark
these controls disabled if they are intended as placeholders, rather than
leaving no-op onChange callbacks.
In `@src/components/ui/choice.tsx`:
- Line 86: Remove the aria-checked attribute from the native checkbox input in
the choice component, leaving the existing ref callback’s node.indeterminate
assignment as the sole mixed-state mechanism.
In `@src/components/ui/citation.tsx`:
- Around line 58-64: Add an explicit accessible role to the non-interactive
citation `<span>` returned by the `Citation` component so its existing
`aria-label={accessibleName}` is honored; use `role="img"` or `role="group"`
while preserving the current markup and behavior.
In `@src/components/ui/link.tsx`:
- Line 113: Update LinkAction to either apply the BaseProps tone value to its
rendered styling or remove tone from the accepted props type; do not leave the
public tone API ignored while hardcoding accent styling.
In `@src/components/ui/progress.tsx`:
- Line 115: Replace the arbitrary z-index value z-[1] in the span className with
z-5, which is the allowed z-index rung from the repository's z-index ladder.
This maintains the intended layering where the connector sits under the stage
marker.
In `@src/components/ui/tabs.tsx`:
- Around line 95-98: Update the tab rendering around the tab element and
selected panel block to set aria-controls only when the corresponding panel is
actually rendered: children must be supplied and the tab’s item.id must equal
the selected value. Otherwise omit the attribute while preserving the existing
panel rendering behavior.
In `@tests/source-badges-off-vocab.dom.test.tsx`:
- Around line 26-29: Update the existing afterEach cleanup in the source-badge
test to restore stubbed environment variables, including NODE_ENV, regardless of
assertion failures; use the test framework’s environment-stub restoration
alongside vi.restoreAllMocks().
In `@tests/source-metadata-browser-safety.test.ts`:
- Around line 19-25: Update the regex pattern in the expect().not.toMatch()
assertion on line 24 to detect all logger module imports, not just the
`@/lib/logger` alias. Expand the pattern to also match relative import specifiers
like ./logger and ../lib/logger so the regression test catches accidental logger
imports regardless of the import path used.
---
Nitpick comments:
In `@src/components/ui-primitives.tsx`:
- Around line 522-539: Add a focused unit test for EmptyState that omits body,
supplies description, and asserts the description text renders; keep the
existing body-priority behavior covered if applicable.
In `@src/components/ui/disclosure.tsx`:
- Around line 66-89: Update the disclosure component’s heading wrapper to
support a configurable headingLevel prop, defaulting to h3, and render the
trigger with that selected heading level instead of always using h3. Preserve
the existing trigger content and behavior while allowing callers to choose the
appropriate heading outline level.
In `@src/components/ui/toast.tsx`:
- Around line 87-94: Update ToastCard’s auto-dismiss behavior so the timer
pauses while the toast is hovered or focused, including focus within its dismiss
control, and resumes with the remaining duration when attention ends. Use the
toast card’s pointer/focus state in the existing useEffect timer lifecycle,
preserving immediate dismissal for non-positive durations and cleanup on
unmount.
- Around line 73-78: Add an opt-out prop to ToastProvider that allows callers
owning their own portal to suppress the provider’s built-in ToastRegion; use
this prop in the render block around ToastContext.Provider so the default region
remains rendered when the opt-out is not enabled, and update the related
provider API/type definition and usage documentation near ToastRegion.
In `@tests/accessible-table-alignment.dom.test.tsx`:
- Around line 79-98: Add an afterEach cleanup for the global matchMedia stub
used by setMatchMedia(true) in the AccessibleTable expander wiring tests, and
import afterEach from vitest. Restore the default matchMedia behavior after each
test so subsequent alignment tests do not inherit mobile mode.
- Around line 23-77: Add DOM assertions in the Checkbox and RadioGroup coverage
in tests/ui-v2-components.dom.test.tsx to verify the checked-state tick and
radio dot become visible when selected. Exercise the checked interaction/state
and assert the relevant peer-checked visibility classes or rendered state,
targeting the Checkbox and RadioGroup implementations in choice.tsx.
In `@tests/ui-v2-components.dom.test.tsx`:
- Around line 124-141: Extend the Tooltip test to dispatch an Escape key event
after verifying it opens on focus, then assert the tooltip is no longer present.
Keep the existing focus and aria-describedby assertions unchanged so the test
covers both keyboard opening and Escape closing.
- Around line 1-16: Add Select coverage to the component suite, importing Select
from its UI module and adding a test that renders it with a controlled value and
placeholder, then verifies the expected rendered state while asserting React
emits no controlled/uncontrolled warning. Use the existing test conventions and
spies to restore warning mocks after the test.
🪄 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
Run ID: f5bfa5cb-6cf3-47b7-b0f5-ea07a433fe1b
📒 Files selected for processing (58)
.design-sync/NOTES.md.design-sync/config.json.design-sync/conventions.md.design-sync/entry.tsx.design-sync/previews/AnswerCard.tsx.design-sync/previews/AnswerFooter.tsx.design-sync/previews/AsyncButton.tsx.design-sync/previews/Breadcrumb.tsx.design-sync/previews/Button.tsx.design-sync/previews/Chip.tsx.design-sync/previews/ConfirmDialog.tsx.design-sync/previews/DoseLine.tsx.design-sync/previews/IconButton.tsx.design-sync/previews/PageHeader.tsx.design-sync/previews/Pagination.tsx.design-sync/previews/SearchField.tsx.design-sync/previews/Skeleton.tsx.design-sync/previews/SourceDesignationBadge.tsx.design-sync/previews/Tabs.tsx.design-sync/previews/TextField.tsx.design-sync/previews/ToastRegion.tsx.design-sync/previews/Tooltip.tsxdocs/branch-review-ledger.mddocs/clinical-badge-system-guide.mdscripts/design-system-contract-utils.mjssrc/app/ckb-v2-tokens.csssrc/app/globals.csssrc/components/AccessibleTable.tsxsrc/components/ui-primitives.tsxsrc/components/ui/answer-card.tsxsrc/components/ui/badge.tsxsrc/components/ui/button.tsxsrc/components/ui/card.tsxsrc/components/ui/chip.tsxsrc/components/ui/choice.tsxsrc/components/ui/citation.tsxsrc/components/ui/confirm-dialog.tsxsrc/components/ui/disclosure.tsxsrc/components/ui/link.tsxsrc/components/ui/page-header.tsxsrc/components/ui/pagination.tsxsrc/components/ui/progress.tsxsrc/components/ui/quantity.tsxsrc/components/ui/select.tsxsrc/components/ui/sheet.tsxsrc/components/ui/status-mark.tsxsrc/components/ui/tabs.tsxsrc/components/ui/text-field.tsxsrc/components/ui/toast.tsxsrc/components/ui/tooltip.tsxsrc/lib/logger.tssrc/lib/source-metadata.tstests/accessible-table-alignment.dom.test.tsxtests/ckb-v2-token-contract.test.tstests/source-badges-off-vocab.dom.test.tsxtests/source-metadata-browser-safety.test.tstests/source-metadata.test.tstests/ui-v2-components.dom.test.tsx
💤 Files with no reviewable changes (2)
- src/components/ui/card.tsx
- src/components/ui/badge.tsx
Agent identity cannot edit the PR description directly. Ship the complete policy body (summary, RAG impact, verification, risk, checked governance) so the trusted sync-pr-policy-body job can apply it. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
Heavy review+fix — PR #1546Tip: Fixed
Dispositioned (resolved, no further code)
Threads0 unresolved. Required CI on tip
Local gates (decisive)
Residual risks
Merge left to you. |
|
Sentry Tabs Tip aria-controls={children && selected ? `${baseId}-panel-${item.id}` : undefined}Unselected tabs no longer reference missing panel IDs. Thread is resolved/outdated on GitHub. Proof: Note: Sentry’s suggested “render all panels with |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
🔒 Upgrade RequiredPrivate repository analysis requires Pro or Enterprise. Upgrade: https://ecc.tools/pricing?plan=pro ECC Tools keeps the core app open, and puts private repos, team features, and enterprise controls behind paid tiers. |
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 `@PR_POLICY_BODY.md`:
- Around line 12-14: Update the verification checklist entries in
PR_POLICY_BODY.md to retain decisive evidence for each gate, including relevant
command output or a reference to an attached artifact. Replace checkbox-only
status and aggregate pass counts with evidence that directly demonstrates each
command completed successfully.
🪄 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
Run ID: b7601e84-a0ec-4584-bff8-5b95facd8766
📒 Files selected for processing (6)
PR_POLICY_BODY.mddocs/branch-review-ledger.mdsrc/app/ckb-v2-tokens.csssrc/components/ui/tabs.tsxsrc/components/ui/tooltip.tsxtests/ui-v2-components.dom.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/ui/tabs.tsx
- src/components/ui/tooltip.tsx
- docs/branch-review-ledger.md
- tests/ui-v2-components.dom.test.tsx
…ription `.github/workflows/ci.yml`'s "Sync PR policy body" job reads `PR_POLICY_BODY.md` from the PR head and overwrites the pull request description with it. The file is meant to be a per-branch scratch template, but #1546 committed its own copy to `main` (845e12e), so every PR whose head contains it — which is now every PR — has its description replaced with #1546's body. That is not cosmetic. `scripts/pr-policy.mjs` parses the description as merge-gating input, so the Clinical Governance Preflight and the `RAG impact:` line currently shown on unrelated PRs belong to a different change, and the verification evidence they assert was never run for the diff they sit on. This PR's own description was replaced with #1546's `.ckb-v2` dark-cascade notes, claiming `tests/ckb-v2-token-contract.test.ts — 35 passed` for a diff that does not touch that file. The workflow already does the right thing when the file is absent — it logs "No PR_POLICY_BODY.md template on this head; skipping PR body sync" — so deleting it restores author-written descriptions everywhere without a workflow change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
…1548) * fix(therapy-compass): stop faking multi-select with a listbox on phones Topics and availability were two native `<select>`s pretending to be multi-select. `value` was pinned to `""` so nothing was ever selected, chosen options carried a literal `"✓ "` prefix in their text, and a disabled placeholder row did the reporting ("3 topics selected"). Assistive technology was told "combobox, nothing selected" while the visible text said three things were on; choosing an already-chosen option silently deselected it; and the availability select carried "Clear filters" — an action — among its options. A listbox cannot express any of that. Both now open one sheet of `aria-pressed` toggles, which is what the wide viewport has always used, so the two breakpoints finally describe the same state the same way. Built in Therapy Compass's own idiom — `tc-btn`, `softControl`, `tc-is-selected`, and the local `SlidersIcon`/`CheckIcon` — rather than importing the documents panel's styling. The accessibility spec's therapy case is rewritten around the real defect: a topic toggle reports `aria-pressed=false`, flips to `true` when pressed, and turning on an availability filter leaves the topic on. That is precisely the state the old control could not represent. Focus-ring and 44px tap-target assertions move to the trigger. Verified: `npm run verify:cheap` exit 0, 457 files / 4780 tests passed; `tests/ui-accessibility.spec.ts` 15 passed, chromium. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(documents): address the CodeRabbit review on the filter sheet Two of the three findings were valid and are fixed: - The sheet footer's "Show N documents" button carried `sm:min-h-10`, copied from the ribbon. Ribbon controls are deliberately 40px on desktop, but this is a dialog's primary action — `mode-nav.tsx:269` shows sheet rows use `min-h-12`. Corrected to `sm:min-h-12`. - The dialog test asserted `role` and `aria-haspopup` but never the `aria-controls` linkage this PR deliberately added, so a wrong or stale panel id would have passed. It now pins `aria-expanded` and `aria-controls` against the rendered dialog's own id, and asserts both are cleared after Escape. The third — that unmounting an open Sheet when `loading` flips skips focus restore — is real in mechanism but has no observable consequence, so the suggested restructure is deliberately not made. `popSheet` runs in the same effect cleanup as the unmount (`sheet.tsx:256`), so the body scroll lock and background inert state are released either way; the only skipped step is focus restore, and its target is the trigger, which `showFilterControl` unmounts in that same render. Keeping the panel mounted to run the close path would fix nothing visible while making the panel silently reopen once the refetch ends. A test pins the invariant that does matter: the scroll lock is not leaked. Verified: `npm run verify:cheap` exit 0, 457 files / 4781 tests passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * docs: record the therapy-compass filter sheet handoff in the branch review ledger Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(therapy-compass): show Clear all when only a search query is active Phone Clear lives in the filter sheet; activeCount ignored the query so a query-only session hid Clear all. Count a trimmed query and pass it from SearchScreen so the trigger badge stays consistent. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com> * fix(therapy-compass): keep the query out of the filter badge, and finish the review The query-only Clear all fix on 42821f7 was right about the bug and wrong about where to put the count. It added the trimmed query to the trigger's `activeFilterCount`, so searching "anxiety" with nothing filtered rendered a badge reading "1" and announced "1 filter active". A search term is not a filter, and this screen's sheet exists precisely because its controls used to describe a state the page was not in. The two counts are now deliberately different, which is the point: - `clearableCount` (sheet) = topics + availability + query. Clear all appears whenever `clearSearch` has something to reset, matching the wide viewport's unconditional Clear. The query belongs here because `clearSearch` resets it. - `activeFilterCount` (trigger badge) = topics + availability. The badge is labelled "N filters active" and must not count a search term. Remaining review findings, all verified before acting: - Sheet toggles were `tc-control`, which resolves to `--spacing-tap` = 44px. Raised to `min-h-12`; a dialog has the room and these are thumb targets. - The scroll-lock test asserted only the release, so it would have passed the day the sheet stopped locking at all. It now asserts the lock is taken first. - `aria-controls` is asserted against the open panel's own id. - Module-level mocks in the therapy DOM test now reset between cases. Two findings deliberately not applied. The suggestion to share one active-filter formula between sheet and trigger is the opposite of the fix above — the formulas must differ, and a shared helper would re-merge exactly what this commit separates. And the trigger's tap-target assertion stays at 44px: it sits in the ribbon's utility row beside `ResultSortControl`, which is `min-h-tap`, so raising only this control would leave the row ragged. The repo's `min-h-12` rule exists to stop generic a11y advice pulling production down to `min-h-11`, not to override a row's own rhythm. Verified: `npm run verify:cheap` exit 0, 460 files / 4797 tests passed; `tests/ui-accessibility.spec.ts` 15 passed, chromium. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY * fix(ci): drop the stray PR_POLICY_BODY.md that rewrites every PR description `.github/workflows/ci.yml`'s "Sync PR policy body" job reads `PR_POLICY_BODY.md` from the PR head and overwrites the pull request description with it. The file is meant to be a per-branch scratch template, but #1546 committed its own copy to `main` (845e12e), so every PR whose head contains it — which is now every PR — has its description replaced with #1546's body. That is not cosmetic. `scripts/pr-policy.mjs` parses the description as merge-gating input, so the Clinical Governance Preflight and the `RAG impact:` line currently shown on unrelated PRs belong to a different change, and the verification evidence they assert was never run for the diff they sit on. This PR's own description was replaced with #1546's `.ckb-v2` dark-cascade notes, claiming `tests/ckb-v2-token-contract.test.ts — 35 passed` for a diff that does not touch that file. The workflow already does the right thing when the file is absent — it logs "No PR_POLICY_BODY.md template on this head; skipping PR body sync" — so deleting it restores author-written descriptions everywhere without a workflow change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…1609) * docs: replace the search-bar handoff with a durable decisions record `docs/handoff-search-bar.md` shipped to main in #1555. It existed to carry one unverified commit across a session boundary, and its instructions are now false: it tells the reader that `6917e732` is unverified and that no PR should be opened on it, when #1555 merged exactly that work. Leaving it in the repo means the next person to read it acts on stale gate status. Its durable content — results-bar anatomy, why the filter shelf covers two modes rather than eight, and the two things deliberately not done (the library button stays until nav can preserve the query; Sort does not move into the phone sheet from the shared band) — moves to docs/search-results-bar-decisions.md, verified against current main rather than copied forward: `appliedFilters`/`onClearFilters` still have exactly the two production consumers the doc claims, and the `Open source library` control is still there. Also records the PR-policy body defect that #1555's handoff flagged but never captured: ci.yml's body-sync job reads PR_POLICY_BODY.md from the PR head, so committing that scratch file to main (#1546) replaced every open PR's description, and pr-policy.mjs parses the body as merge-gating input. #1548 deleted the file; nothing stops the next branch adding one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(ledger): record the search-bar decisions-doc review Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: fix review findings on search-bar decisions record Reconcile the twelve results-band modes with shelf scope, name the three sheetless Sort consumers, tighten #230 to heads that contain PR_POLICY_BODY.md, and update #170 so documents/therapy sheets match code. * docs(ledger): record review-fix verification at tip --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
2 participants
Summary
.dark .ckb-v2after the cascade port so light.ckb-v2matching inside dark subtrees cannot resolve light ink on dark surfaces.origin/mainto clear DIRTY mergeability (prefer main’s post-feat(design-system): v2 token layer + 26 components — reconciled, cascade-ported, forced-colours owned #1538/fix(design-system): critique fixes + post-merge truth sweep (bundled hygiene PR) #1541 component copies; keep this tip’s dark-ink contract + HCM transparent overlay-backdrop rationale).aria-controlsonly on the selected tab when a panel is owned (unselected tabs no longer point at missing IDs).RAG impact: no retrieval behaviour change — design-system token dark-cascade fix and unadopted UI component a11y only; no file under
src/lib/rag/**, clinical-search, retrieval-selection, ranking, eval harness, golden fixtures, or retrieval RPCs is touched.Verification
npm run verify:pr-localvitesttests/ckb-v2-token-contract.test.ts+tests/ui-v2-components.dom.test.tsx— 35 passednpm run verify:cheap— 457 files / 4782 passed.ckb-v2/ these components yet; phone-chrome and visual journeys unchanged. Prefer CI Production UI on this tip.Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
Summary by CodeRabbit
Accessibility
Visual Improvements
Documentation
Tests