fix: finalize design audit regression hardening#806
Closed
BigSimmo wants to merge 3 commits into
Closed
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
📝 WalkthroughWalkthroughThe pull request updates sitemap generation and documentation, refactors RAG abort and visual-enrichment handling, adds Supabase cleanup and query-correction logic, enhances service navigation and forms UI, and realigns regression and browser tests. ChangesRoute documentation and discovery
RAG and database scalability
UI behavior and regression coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RouteFiles
participant collectSiteMapData
participant renderSiteMapRaw
RouteFiles->>collectSiteMapData: discover page and handler routes
collectSiteMapData->>collectSiteMapData: classify API and public handlers
collectSiteMapData->>renderSiteMapRaw: pass route groups and redirects
renderSiteMapRaw->>RouteFiles: render sitemap sections
sequenceDiagram
participant searchChunksWithTelemetry
participant ragCandidateSources
participant attachPageVisualEvidence
participant selectRankedRetrievalResults
searchChunksWithTelemetry->>ragCandidateSources: fetch retrieval candidates
ragCandidateSources->>selectRankedRetrievalResults: provide candidate results
selectRankedRetrievalResults->>attachPageVisualEvidence: enrich ranked results
attachPageVisualEvidence->>searchChunksWithTelemetry: return visual evidence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CI triageCI failed on this PR. Automated classification of the 6 failed job(s):
Compared with main CI run #3145 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/rag-candidate-sources.ts (1)
185-207: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRestore caller cancellation through candidate retrieval.
The text-candidate path and the downstream lookup/hydration helpers still dropsignal, so cancelled answer requests keep waiting on Supabase RPCs and reads. Threadsignalthrough these helpers and pass it tocallVersionedRetrievalRpc/query builders across the retrieval chain.🤖 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 185 - 207, Thread the caller’s AbortSignal through searchTextChunkCandidates and its downstream candidate lookup/hydration helpers, preserving it across every retrieval-chain call. Pass the signal to callVersionedRetrievalRpc and all Supabase query builders so cancellation stops RPCs and reads instead of being dropped.Source: Coding guidelines
🤖 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 `@playwright.config.ts`:
- Line 58: Move the reducedMotion setting from the top-level Playwright use
configuration into the contextOptions object, preserving its "no-preference"
value so the configuration matches Playwright’s typed schema.
In `@scripts/generate-site-map.ts`:
- Around line 394-405: Restore product redirect handlers to the “Main product
routes” section by appending handlers identified by productRouteHandlerPaths to
the product route list used by the sitemap renderer in
scripts/generate-site-map.ts lines 394-405. Keep scripts/generate-site-map.ts
lines 481-484 excluding those handlers from the utility-handler section. Update
the product-section assertion in tests/site-map.test.ts lines 90-97 from
toNotContain to toContain.
In `@supabase/migrations/20260714190000_document_table_facts_trgm_idx.sql`:
- Around line 2-11: Remove the document_table_facts_text_trgm_idx creation from
this migration because 20260717010000_harden_rag_scalability_patch.sql drops it
immediately afterward. Keep the committed migration chain free of this temporary
GIN index build; if needed, apply the index separately outside the transaction.
In `@tests/ui-accessibility.spec.ts`:
- Line 51: Align the dashboard H1 expectations with the canonical rendered
heading by changing the locator name from “Clinical KB” to “Clinical Guide” in
tests/ui-accessibility.spec.ts:51-51, tests/ui-smoke.spec.ts:870-870, and
tests/ui-smoke.spec.ts:1265-1265; keep the existing heading role and count
assertions unchanged.
In `@tests/ui-tools.spec.ts`:
- Line 1623: Update mockAnswerDashboardApi to handle service detail requests in
addition to kind === "form": return the appropriate service detail record for
the detail route instead of falling through to a 404. Preserve the existing
form-record behavior and apply the updated mock to the listed service-detail
test setups.
---
Outside diff comments:
In `@src/lib/rag-candidate-sources.ts`:
- Around line 185-207: Thread the caller’s AbortSignal through
searchTextChunkCandidates and its downstream candidate lookup/hydration helpers,
preserving it across every retrieval-chain call. Pass the signal to
callVersionedRetrievalRpc and all Supabase query builders so cancellation stops
RPCs and reads instead of being dropped.
🪄 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: 1f0bfbcc-61f9-4b81-bab2-263fbaad977f
📒 Files selected for processing (24)
docs/codebase-index.mddocs/site-map.mdplaywright.config.tsscripts/check-github-action-pins.mjsscripts/generate-site-map.tssrc/app/layout.tsxsrc/app/page.tsxsrc/components/clinical-dashboard/master-search-header.tsxsrc/components/forms/forms-home-page.tsxsrc/components/services/services-navigator-page.tsxsrc/lib/rag-candidate-sources.tssrc/lib/rag.tssupabase/drift-manifest.jsonsupabase/migrations/20260714180000_patch_rag_and_corrector_scalability.sqlsupabase/migrations/20260714190000_document_table_facts_trgm_idx.sqlsupabase/migrations/20260717010000_harden_rag_scalability_patch.sqlsupabase/schema.sqltests/audit-content-services-regressions.test.tstests/audit-navigation-auth-regressions.test.tstests/site-map.test.tstests/supabase-schema.test.tstests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/ui-tools.spec.ts
| grepInvert: mockupTag, | ||
| use: { | ||
| ...devices["Desktop Chrome"], | ||
| reducedMotion: "no-preference", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n '"(`@playwright/test`|playwright)"|reducedMotion|contextOptions' \
playwright.config.ts package.json package-lock.json pnpm-lock.yaml yarn.lock 2>/dev/null || trueRepository: BigSimmo/Database
Length of output: 668
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n--- playwright.config.ts (excerpt) ---\n'
cat -n playwright.config.ts | sed -n '35,70p'
printf '\n--- package.json Playwright entries ---\n'
cat -n package.json | sed -n '205,225p'
printf '\n--- local Playwright docs/type hints if present ---\n'
fd -a 'contextOptions*|playwright*' node_modules/next/dist/docs node_modules 2>/dev/null | sed -n '1,80p' || trueRepository: BigSimmo/Database
Length of output: 4025
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n--- Playwright type references for reducedMotion/contextOptions ---\n'
rg -n "reducedMotion|contextOptions" node_modules/playwright node_modules/playwright-core -g '*.d.ts' -g '*.md' -g '*.js' | sed -n '1,200p'
printf '\n--- Playwright test config type definitions around use options ---\n'
FILE="$(fd -a 'test.d.ts' node_modules/playwright node_modules/playwright-core | head -n 1)"
echo "Using: $FILE"
if [ -n "$FILE" ]; then
rg -n "reducedMotion|contextOptions|PlaywrightTestOptions|UseOptions" "$FILE" | sed -n '1,120p'
fiRepository: BigSimmo/Database
Length of output: 26293
🏁 Script executed:
#!/bin/bash
set -eu
FILE="node_modules/playwright/types/test.d.ts"
printf '\n--- PlaywrightTestOptions / use-option section ---\n'
rg -n "interface PlaywrightTestOptions|type PlaywrightTestOptions|reducedMotion\\?|contextOptions: BrowserContextOptions|BrowserContextOptions" "$FILE" | sed -n '1,120p'
printf '\n--- excerpt around contextOptions / reducedMotion references ---\n'
for n in 7528 7532 7536 7540 7544 7548; do
sed -n "${n},$((n+20))p" "$FILE"
printf '\n---\n'
done
printf '\n--- BrowserContextOptions definition around reducedMotion ---\n'
FILE2="node_modules/playwright-core/types/types.d.ts"
line=$(rg -n "^export interface BrowserContextOptions" "$FILE2" | head -n 1 | cut -d: -f1)
if [ -n "${line:-}" ]; then
sed -n "$((line-5)),$((line+120))p" "$FILE2" | rg -n "reducedMotion|contextOptions|colorScheme|interface BrowserContextOptions" -n
fiRepository: BigSimmo/Database
Length of output: 4831
Move reducedMotion into contextOptions. Playwright’s use schema exposes this setting under contextOptions; the top-level reducedMotion entry here doesn’t match the config type and blocks config type-checking.
🧰 Tools
🪛 GitHub Check: Static PR checks
[failure] 58-58:
No overload matches this call.
🤖 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 `@playwright.config.ts` at line 58, Move the reducedMotion setting from the
top-level Playwright use configuration into the contextOptions object,
preserving its "no-preference" value so the configuration matches Playwright’s
typed schema.
Source: Linters/SAST tools
Comment on lines
+394
to
405
| const publicUtilityRouteHandlers = data.publicRouteHandlers.filter( | ||
| (route) => !productRouteHandlerPaths.has(route.route), | ||
| ); | ||
|
|
||
| const lines = [ | ||
| "# Clinical KB Site Map", | ||
| "", | ||
| "This file is generated by `npm run sitemap:update`. Run `npm run sitemap:check` to verify it is current.", | ||
| "", | ||
| ...section( | ||
| "Main product pages", | ||
| "Main product routes", | ||
| productRoutes.map((route) => routeLine(route, routeDescriptions)), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore product redirect handlers to “Main product routes.”
The renderer drops product handlers from that section, and the regression test incorrectly codifies their absence.
scripts/generate-site-map.ts#L394-L405: append handlers inproductRouteHandlerPathsto the main product route list.scripts/generate-site-map.ts#L481-L484: continue excluding those product handlers from the utility-handler section.tests/site-map.test.ts#L90-L97: change the product-section assertion totoContain.
📍 Affects 2 files
scripts/generate-site-map.ts#L394-L405(this comment)scripts/generate-site-map.ts#L481-L484tests/site-map.test.ts#L90-L97
🤖 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 `@scripts/generate-site-map.ts` around lines 394 - 405, Restore product
redirect handlers to the “Main product routes” section by appending handlers
identified by productRouteHandlerPaths to the product route list used by the
sitemap renderer in scripts/generate-site-map.ts lines 394-405. Keep
scripts/generate-site-map.ts lines 481-484 excluding those handlers from the
utility-handler section. Update the product-section assertion in
tests/site-map.test.ts lines 90-97 from toNotContain to toContain.
Comment on lines
+2
to
+11
| create index if not exists document_table_facts_text_trgm_idx | ||
| on public.document_table_facts using gin ( | ||
| lower( | ||
| coalesce(table_title, '') || ' ' || | ||
| coalesce(row_label, '') || ' ' || | ||
| coalesce(clinical_parameter, '') || ' ' || | ||
| coalesce(threshold_value, '') || ' ' || | ||
| coalesce(action, '') | ||
| ) extensions.gin_trgm_ops | ||
| ); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'document_table_facts_text_trgm_idx|CONCURRENTLY' supabase scripts package.json
rg -n -C3 'transaction|migration' supabase/config.toml package.json README.md 2>/dev/null || trueRepository: BigSimmo/Database
Length of output: 16963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the two migrations and the migration-reconciliation guidance.
wc -l supabase/migrations/20260714190000_document_table_facts_trgm_idx.sql \
supabase/migrations/20260717010000_harden_rag_scalability_patch.sql \
docs/supabase-migration-reconciliation.md \
docs/process-hardening.md
printf '\n--- 20260714190000_document_table_facts_trgm_idx.sql ---\n'
cat -n supabase/migrations/20260714190000_document_table_facts_trgm_idx.sql
printf '\n--- 20260717010000_harden_rag_scalability_patch.sql (relevant slice) ---\n'
sed -n '1,180p' supabase/migrations/20260717010000_harden_rag_scalability_patch.sql
printf '\n--- docs/supabase-migration-reconciliation.md (relevant slice) ---\n'
sed -n '1,220p' docs/supabase-migration-reconciliation.md
printf '\n--- docs/process-hardening.md (relevant slice) ---\n'
sed -n '1,220p' docs/process-hardening.mdRepository: BigSimmo/Database
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any explicit guidance about CONCURRENTLY, transactional migrations, or replay policy.
rg -n -C2 'CONCURRENTLY|transactional|db push|migration replay|Supabase migrations|write-blocking|lock' \
supabase docs README.md package.json scripts 2>/dev/null || trueRepository: BigSimmo/Database
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for the table/index usage around the migration window.
rg -n -C2 'document_table_facts|document_table_facts_text_trgm_idx|trgm' \
supabase docs scripts src 2>/dev/null || true
printf '\n--- migration list around July 2026 ---\n'
git ls-files 'supabase/migrations/202607*.sql' | sortRepository: BigSimmo/Database
Length of output: 50376
Remove the dead index build. 20260717010000_harden_rag_scalability_patch.sql drops document_table_facts_text_trgm_idx again, so this migration just adds a blocking GIN build that fresh replays immediately throw away. If the index is only temporary scaffolding, keep it out of the committed migration chain and apply it separately outside the transaction.
🧰 Tools
🪛 Squawk (2.59.0)
[warning] 2-11: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 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 `@supabase/migrations/20260714190000_document_table_facts_trgm_idx.sql` around
lines 2 - 11, Remove the document_table_facts_text_trgm_idx creation from this
migration because 20260717010000_harden_rag_scalability_patch.sql drops it
immediately afterward. Keep the committed migration chain free of this temporary
GIN index build; if needed, apply the index separately outside the transaction.
Source: Linters/SAST tools
|
|
||
| async function expectDashboardUsable(page: Page) { | ||
| await expect(page.getByRole("heading", { level: 1, name: "Clinical Guide" })).toHaveCount(1); | ||
| await expect(page.getByRole("heading", { level: 1, name: "Clinical KB" })).toHaveCount(1); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the browser H1 expectations with the rendered dashboard heading.
tests/audit-navigation-auth-regressions.test.ts still verifies that the production sr-only H1 is Clinical Guide, so these Clinical KB locators target a heading the dashboard does not render.
tests/ui-accessibility.spec.ts#L51-L51: expectClinical Guide, or update the production heading and source regression together.tests/ui-smoke.spec.ts#L870-L870: use the same canonical H1 name.tests/ui-smoke.spec.ts#L1265-L1265: use the same canonical H1 name.
📍 Affects 2 files
tests/ui-accessibility.spec.ts#L51-L51(this comment)tests/ui-smoke.spec.ts#L870-L870tests/ui-smoke.spec.ts#L1265-L1265
🤖 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-accessibility.spec.ts` at line 51, Align the dashboard H1
expectations with the canonical rendered heading by changing the locator name
from “Clinical KB” to “Clinical Guide” in tests/ui-accessibility.spec.ts:51-51,
tests/ui-smoke.spec.ts:870-870, and tests/ui-smoke.spec.ts:1265-1265; keep the
existing heading role and count assertions unchanged.
| { name: "desktop", width: 1280, height: 900 }, | ||
| ] as const) { | ||
| test(`13YARN service detail is usable at ${viewport.name}`, async ({ page }) => { | ||
| await mockAnswerDashboardApi(page); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Extend mockAnswerDashboardApi to serve service detail records.
Its detail route only resolves records for kind === "form" and returns 404 for services. These new calls therefore break the service-detail tests they are intended to stabilize.
Proposed direction
- const record = kind === "form" ? formRecords.find((form) => form.slug === slug) : undefined;
+ const record =
+ kind === "form"
+ ? formRecords.find((form) => form.slug === slug)
+ : serviceRecords.find((service) => service.slug === slug);Also applies to: 1645-1645, 1677-1677, 1688-1688
🤖 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` at line 1623, Update mockAnswerDashboardApi to handle
service detail requests in addition to kind === "form": return the appropriate
service detail record for the detail route instead of falling through to a 404.
Preserve the existing form-record behavior and apply the updated mock to the
listed service-detail test setups.
9 tasks
8 tasks
21 tasks
This was referenced Jul 18, 2026
This was referenced Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves the remaining design/accessibility audit regressions and associated regression-source guard updates, including test and RAG recovery hardening.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation