fix: rebase design-audit regression hardening onto current main - #846
Closed
BigSimmo wants to merge 2 commits into
Closed
fix: rebase design-audit regression hardening onto current main#846BigSimmo wants to merge 2 commits into
BigSimmo wants to merge 2 commits into
Conversation
Updates to Preview Branch (codex/merge-safe-819-rebased) ↗︎
Tasks are run on every commit but only new migration files are pushed.
❌ Branch Error • Sat, 18 Jul 2026 12:38:12 UTC View logs for this Workflow Run ↗︎. |
Contributor
📝 WalkthroughWalkthroughThe pull request adds abort-aware retrieval and classifier handling, refines answer-coalescing metrics, changes dashboard rendering and focus behavior, updates query parameter typing and Playwright motion settings, and refreshes UI, regression, schema, and manifest expectations. ChangesRAG pipeline updates
Dashboard and browser behavior
Regression and schema validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant rag
participant ClassifierFallback
participant SupabaseRPC
Request->>rag: start retrieval with AbortSignal
rag->>ClassifierFallback: analyze query with caller signal
ClassifierFallback-->>rag: classified query or AbortError
rag->>SupabaseRPC: execute versioned or legacy RPC with signal
SupabaseRPC-->>rag: retrieval data or error
rag-->>Request: return result or cancellation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/components/route-error-boundary.tsx`:
- Around line 59-62: Update the focused heading in the route error boundary,
identified by headingRef and its h1 className, to preserve a visible outline
when focused imperatively. Add a focus-based outline style while retaining the
existing focus-visible styling, or explicitly request focus-visible behavior
through headingRef.current?.focus({ focusVisible: true }).
In `@src/lib/rag-candidate-sources.ts`:
- Around line 91-99: Forward the caller’s args.signal through both vector RPC
retrieval call sites in rag.ts, including the fallback path, so the executeRpc
cancellation logic in rag-candidate-sources.ts receives it. Update the calls
near the existing retrieval paths without changing their other arguments or
behavior.
In `@src/lib/rag.ts`:
- Around line 1393-1399: Update the error handling in awaitWithCallerSignal so
caller-triggered aborts are re-thrown when opts?.signal?.aborted or error equals
opts?.signal?.reason, including custom abort reasons. Perform this check before
the existing AbortError transport check and preserve the current fallback that
returns analysis for non-abort failures.
In `@tests/supabase-schema.test.ts`:
- Around line 1372-1375: Update the assertions around cleanup to normalize SQL
formatting without lowercasing quoted values: use a separately normalized
representation for structural checks, while matching the raw cleanup SQL for the
case-sensitive literals 'clinical_registry_records' and 'kind'. Preserve the
existing checks for metadata fields and CASE structure.
In `@tests/ui-tools.spec.ts`:
- Around line 1021-1033: Update the transition inspection in the dock test
around the evaluate callback so it parses transitionProperty and
transitionDuration as paired lists and asserts the duration corresponding to the
transform transition, while treating all as the applicable fallback. Remove the
current maximum-duration assertion so unrelated transitions such as opacity
cannot satisfy the dock animation check.
🪄 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: 1cc2668c-b565-4a40-80d7-417241cda42f
📒 Files selected for processing (13)
playwright.config.tssrc/app/page.tsxsrc/components/ClinicalDashboard.tsxsrc/components/route-error-boundary.tsxsrc/lib/rag-candidate-sources.tssrc/lib/rag.tssupabase/drift-manifest.jsontests/audit-content-services-regressions.test.tstests/audit-navigation-auth-regressions.test.tstests/supabase-schema.test.tstests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
Comment on lines
59
to
+62
| <h1 | ||
| ref={headingRef} | ||
| tabIndex={-1} | ||
| className="mt-4 text-lg font-semibold tracking-tight text-[color:var(--text-heading)] focus:outline focus:outline-2 focus:outline-offset-2 focus:outline-[color:var(--focus-ring,Highlight)]" | ||
| className="mt-4 text-lg font-semibold tracking-tight text-[color:var(--text-heading)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus-ring,Highlight)]" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'\n== route-error-boundary.tsx ==\n'
sed -n '1,220p' src/components/route-error-boundary.tsx
printf'\n== focus-related usages ==\n'
rg -n "focus-visible:outline|focus:outline|headingRef|tabIndex=\{-1\}|focus\(\)" src/components src -g '!**/*.map'||trueRepository: BigSimmo/Database
Length of output: 50374
🌐 Web query:
CSS :focus-visible programmatic focus non-interactive element browser behavior
💡 Result:
When using the programmatic .focus() method on a non-interactive element (e.g., a
with
tabindex="-1"), the :focus-visible pseudo-class does not typically match by default [1][2]. This occurs because the browser's heuristic for :focus-visible generally suppresses the focus indicator when the focus change is triggered by a pointer interaction or a script, aiming to match native behavior where focus rings are often hidden unless a keyboard-driven navigation path is detected [3][4][5]. Key behaviors and workarounds include: Programmatic Focus Limitations: By default, calling element.focus() does not trigger :focus-visible because the user agent's heuristic often classifies programmatic focus as a non-keyboard-driven event [2][6]. Forcing Visibility: To ensure a focus indicator appears regardless of the browser's internal heuristics, you can use the focusVisible option in the HTMLElement.focus() method [6][7]: element.focus({ focusVisible: true }); This explicitly tells the browser to treat the focus as "visible," causing the :focus-visible pseudo-class to match [2][6]. Browser Inconsistencies: While the focusVisible option is now widely supported in modern browsers (e.g., Firefox 104+, Chrome, Safari), behavior for non-interactive elements without this option can still vary slightly across engines [1][7]. Non-interactive elements (like those with tabindex="-1") that are focusable via script but not by keyboard navigation may not consistently show a focus ring depending on how the browser's focus management heuristics classify the triggering event [1][5]. Recommendations: Use the { focusVisible: true } option if you want to guarantee that your :focus-visible styles apply to an element receiving programmatic focus [2][6]. Avoid relying on implicit browser heuristics to show focus on non-interactive elements that are not part of your natural keyboard tab order, as this behavior is not guaranteed to be consistent across different browsers or future spec updates [1][4].Citations:
- 1: [selectors] :focus-visible matching heuristic unclear for non-interactive elements with tabindex="-1" w3c/csswg-drafts#12127
- 2: https://stackoverflow.com/questions/79914412/focus-visible-not-triggered-on-parent-has-selector-when-focus-is-applied-pro
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:focus-visible
- 4: Should :focus-visible match when returning focus or programmatically focusing? WICG/focus-visible#88
- 5: [selectors] :focus-visible matches on initial programmatic focus w3c/csswg-drafts#5885
- 6: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus
- 7: Ff104 HTMLElement.focus() - add focusVisible option mdn/content#19311
Preserve the focus outline on the imperatively focused heading.headingRef.current?.focus() can land on this h1 without matching :focus-visible in some browsers, so sighted users may lose the visible focus location. Keep focus:outline here, or switch to focus({ focusVisible: true }) if you want to rely on focus-visible styling.
🤖 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/route-error-boundary.tsx` around lines 59 - 62, Update the
focused heading in the route error boundary, identified by headingRef and its h1
className, to preserve a visible outline when focused imperatively. Add a
focus-based outline style while retaining the existing focus-visible styling, or
explicitly request focus-visible behavior through headingRef.current?.focus({
focusVisible: true }).
Comment on lines
+91
to
99
| signal?: AbortSignal, | ||
| ): Promise<{ data: T | null; error: SupabaseRpcError }> { | ||
| const client = supabase as unknown as { | ||
| rpc: (name: string, rpcArgs: Record<string, unknown>) => Promise<{ data: T | null; error: SupabaseRpcError }>; | ||
| const client = supabase as unknown as SupabaseRpcClient; | ||
| const executeRpc = async (name: string, rpcArgs: Record<string, unknown>) => { | ||
| const pending = client.rpc(name, rpcArgs) as AbortableRpc<T>; | ||
| const pendingWithAbort = | ||
| signal && typeof pending.abortSignal === "function" ? pending.abortSignal(signal) : pending; | ||
| return await pendingWithAbort; | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Forward args.signal at both retrieval call sites.
Lines 91-99 only apply cancellation when a signal is supplied, but src/lib/rag.ts Lines 2837-2849 and 2945-2956 omit it. Both vector RPC paths therefore continue after caller abort.
Proposed fix
const { data, error } = await callVersionedRetrievalRpc(
supabase,
"match_document_chunks_hybrid_v2",
"match_document_chunks_hybrid",
{
// ...
},
+ args.signal,
);Apply the same fifth argument to the fallback call at src/lib/rag.ts Lines 2945-2956.
🤖 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/lib/rag-candidate-sources.ts` around lines 91 - 99, Forward the caller’s
args.signal through both vector RPC retrieval call sites in rag.ts, including
the fallback path, so the executeRpc cancellation logic in
rag-candidate-sources.ts receives it. Update the calls near the existing
retrieval paths without changing their other arguments or behavior.
Comment on lines
+1393
to
+1399
| } catch (error) { | ||
| if ( | ||
| error && | ||
| (error instanceof DOMException || typeof error === "object") && | ||
| (error as { name?: string }).name === "AbortError" | ||
| ) | ||
| throw error; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'const controller = new AbortController();const reason = new Error("caller cancelled");controller.abort(reason);console.log(controller.signal.aborted, controller.signal.reason === reason, controller.signal.reason.name);NODERepository: BigSimmo/Database
Length of output: 171
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Inspect the relevant region around the reported lines
sed -n '1365,1415p' src/lib/rag.ts
echoecho'---'echo# Find the helper that awaits with the caller signal
rg -n "awaitWithCallerSignal|signal\.reason|AbortError|opts\?\.signal\?\.aborted" src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 2872
Propagate caller aborts heresrc/lib/rag.ts:1393-1399 — awaitWithCallerSignal() can reject with signal.reason, so an AbortController.abort(new Error(...)) falls through to return analysis instead of cancelling. Re-throw when opts?.signal?.aborted (or when error === opts?.signal?.reason) before treating it as a transport failure.
🤖 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/lib/rag.ts` around lines 1393 - 1399, Update the error handling in
awaitWithCallerSignal so caller-triggered aborts are re-thrown when
opts?.signal?.aborted or error equals opts?.signal?.reason, including custom
abort reasons. Perform this check before the existing AbortError transport check
and preserve the current fallback that returns analysis for non-abort failures.
Comment on lines
+1372
to
+1375
| const cleanupLower = cleanup.toLowerCase(); | ||
| expect(cleanupLower).toContain("metadata->>'registry_record_id' = old.id::text"); | ||
| expect(cleanupLower).toContain("metadata->>'registry_record_kind' = case tg_table_name"); | ||
| expect(cleanupLower).toMatch(/when 'clinical_registry_records' then (pg_catalog\.)?to_jsonb\(old\)->>'kind'/); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve case-sensitive registry literals in this assertion.
cleanup.toLowerCase() also lowercases quoted string values. A migration that changes 'clinical_registry_records' or 'kind' to the wrong case would still satisfy these checks, even though PostgreSQL compares those metadata values case-sensitively. Normalize SQL formatting separately, but assert the quoted registry literals from the raw SQL.
🤖 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/supabase-schema.test.ts` around lines 1372 - 1375, Update the
assertions around cleanup to normalize SQL formatting without lowercasing quoted
values: use a separately normalized representation for structural checks, while
matching the raw cleanup SQL for the case-sensitive literals
'clinical_registry_records' and 'kind'. Preserve the existing checks for
metadata fields and CASE structure.
Comment on lines
+1021
to
+1033
| const transition = await dock.evaluate((node) => { | ||
| const style = window.getComputedStyle(node); | ||
| const durationMs = Math.max( | ||
| ...style.transitionDuration.split(",").map((value) => { | ||
| const normalized = value.trim(); | ||
| const duration = Number.parseFloat(normalized); | ||
| return normalized.endsWith("ms") ? duration : duration * 1000; | ||
| }), | ||
| ); | ||
| return { durationMs, property: style.transitionProperty }; | ||
| }); | ||
| expect(transition.property).toMatch(/transform|all/); | ||
| expect(transition.durationMs).toBeGreaterThanOrEqual(100); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the matched transition’s own duration.tests/ui-tools.spec.ts:1021-1033 uses the maximum duration across all transitions, so a transform 0ms, opacity 200ms case can still pass even though the dock animation is instant. Pair transform/all with its corresponding duration instead.
🤖 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-tools.spec.ts` around lines 1021 - 1033, Update the transition
inspection in the dock test around the evaluate callback so it parses
transitionProperty and transitionDuration as paired lists and asserts the
duration corresponding to the transform transition, while treating all as the
applicable fallback. Remove the current maximum-duration assertion so unrelated
transitions such as opacity cannot satisfy the dock animation check.
11 tasks
Contributor
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3269 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
enabled auto-merge
July 18, 2026 12:37
11 tasks
9 tasks
auto-merge was automatically disabled
July 18, 2026 13:01
Pull request was closed
This was referenced Jul 18, 2026
3 tasks
This was referenced Jul 31, 2026
This was referenced Aug 9, 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
1 participant
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
main.Verification
Verification not run:
npm run verify:pr-local(local verification environment missingprettierfrom this worktree; can rerun after reinstall).UI verification not run: no local browser run started in this workflow.
Verification not run:
npm run eval:retrieval:quality(provider-backed command).Verification not run:
npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only(provider-backed command).Verification not run:
npm run check:production-readiness(provider-backed command).Verification not run:
npm run check:deployment-readiness(not requested for this change set).Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)