chore: merge design-audit remediation to main#789
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThe changes expand sitemap discovery and documentation, remove abort-signal propagation from RAG retrieval, add Supabase cleanup and query-correction indexing, update service/forms UI behavior, and revise regression, accessibility, Playwright, and workflow-discovery tests. ChangesRoute discovery and documentation
RAG retrieval and database hardening
Dashboard and mode UI
Test and repository tooling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #3107 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
b501653 to
e4a9256
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supabase/drift-manifest.json (1)
6331-6334: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRestore
documents_sync_title_wordsin the replayed schema.The manifest omits this trigger despite the migration/schema contract requiring it. Without it, title or indexing-status changes leave
document_title_wordsstale. Fix the replay and assert trigger presence in the manifest, rather than only checking SQL text.🤖 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/drift-manifest.json` around lines 6331 - 6334, Restore the documents_sync_title_words trigger in the replayed schema manifest alongside documents_updated_at, using the migration-defined trigger configuration for public.documents. Update manifest validation to assert this trigger’s presence by name, not merely by matching SQL text, so title and indexing-status updates continue synchronizing document_title_words.src/lib/rag.ts (1)
2593-2593: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the stale
chunkLoadCachereferences.The request-local cache initialization was removed, but these three calls still reference the now-undeclared variable, causing typecheck failure.
Proposed fix
telemetry, - cache: chunkLoadCache, @@ telemetry, - cache: chunkLoadCache, @@ telemetry, - cache: chunkLoadCache,Also applies to: 2780-2780, 2796-2796
🤖 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` at line 2593, Remove the stale chunkLoadCache argument references from the three affected calls in the relevant RAG flow, including the calls near the visible cache option and the two additional locations. Keep the remaining call arguments and behavior unchanged, and ensure no undeclared chunkLoadCache references remain.
🤖 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/services/services-navigator-page.tsx`:
- Around line 496-501: Update the “Open” Link in the services navigator card
rendering so its accessible name includes the associated service name, while
preserving the visible “Open” text and existing service-specific href. Use the
service object’s existing identifying name/title field in the link’s aria-label
or equivalent accessible-name attribute.
In `@src/lib/rag.ts`:
- Line 2410: Preserve cancellation signals across retrieval fan-out: in
src/lib/rag.ts lines 1547-1554 and 2410-2410, pass args.signal to
fetchRelatedDocumentMetadata and fetchEnabledRagAliases; in
src/lib/rag-candidate-sources.ts lines 79-92, add a signal parameter to
callVersionedRetrievalRpc and forward it to every rpc call.
In `@tests/audit-content-services-regressions.test.ts`:
- Around line 209-217: Update the source-link assertions in the form-detail
regression test to match the actual implementation in form-detail-page.tsx, or
implement the expected conditional status there. Ensure the test validates the
rendered source-link structure and the correct available/unavailable behavior
without requiring a ternary that is absent from the implementation.
In `@tests/audit-navigation-auth-regressions.test.ts`:
- Around line 126-128: Update the rendered H1 in ClinicalDashboard so the root
dashboard exposes the accessible “Clinical KB” heading, then align
tests/audit-navigation-auth-regressions.test.ts:126-128,
tests/ui-accessibility.spec.ts:51, tests/ui-smoke.spec.ts:870, and
tests/ui-smoke.spec.ts:1265 with that production markup; retain the online and
offline heading expectations after the component fix.
In `@tests/site-map.test.ts`:
- Line 61: Remove the loop iterating over data.appRouteHandlers in the site-map
test, since SiteMapData no longer exposes that property and publicRouteHandlers
is already covered.
- Around line 94-95: Move the three handler assertions from the product/API
section checks to assertions against the Redirects section in the site-map test.
Update the expectations around productSection and apiSection so these routes are
validated as present in redirects, while preserving the existing route-specific
assertions.
---
Outside diff comments:
In `@src/lib/rag.ts`:
- Line 2593: Remove the stale chunkLoadCache argument references from the three
affected calls in the relevant RAG flow, including the calls near the visible
cache option and the two additional locations. Keep the remaining call arguments
and behavior unchanged, and ensure no undeclared chunkLoadCache references
remain.
In `@supabase/drift-manifest.json`:
- Around line 6331-6334: Restore the documents_sync_title_words trigger in the
replayed schema manifest alongside documents_updated_at, using the
migration-defined trigger configuration for public.documents. Update manifest
validation to assert this trigger’s presence by name, not merely by matching SQL
text, so title and indexing-status updates continue synchronizing
document_title_words.
🪄 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: cbfcde42-b767-4502-8a5e-47a168a5008b
📒 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
💤 Files with no reviewable changes (2)
- src/app/layout.tsx
- src/app/page.tsx
| if (queryAnalysis.corpusGrounding) telemetry.corpus_grounding = queryAnalysis.corpusGrounding; | ||
|
|
||
| const ragAliases = await fetchEnabledRagAliases(supabase, args.ownerId, args.accessScope, args.signal); | ||
| const ragAliases = await fetchEnabledRagAliases(supabase, args.ownerId, args.accessScope); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C3 '\.abortSignal\(' --type ts src
rg -n -C3 'fetchEnabledRagAliases\(|fetchRelatedDocumentMetadata\(|callVersionedRetrievalRpc\(' \
src/lib/rag.ts src/lib/rag-candidate-sources.ts \
src/lib/rag-retrieval-variants.ts src/lib/document-enrichment.tsRepository: BigSimmo/Database
Length of output: 18549
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '110,170p' src/lib/rag-retrieval-variants.ts
printf '\n----\n'
sed -n '730,820p' src/lib/document-enrichment.ts
printf '\n----\n'
sed -n '74,115p' src/lib/rag-candidate-sources.ts
printf '\n----\n'
sed -n '1528,1565p' src/lib/rag.ts
printf '\n----\n'
sed -n '2398,2420p' src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 10605
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("src/lib/rag-retrieval-variants.ts", 110, 170),
("src/lib/document-enrichment.ts", 730, 820),
("src/lib/rag-candidate-sources.ts", 74, 115),
("src/lib/rag.ts", 1528, 1565),
("src/lib/rag.ts", 2398, 2420),
]:
print(f"\n==== {path}:{start}-{end} ====")
lines = Path(path).read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:>5}: {lines[i-1]}")
PYRepository: BigSimmo/Database
Length of output: 12592
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
targets = {
"src/lib/rag-retrieval-variants.ts": ["fetchEnabledRagAliases", "signal", "abortSignal"],
"src/lib/document-enrichment.ts": ["fetchRelatedDocumentMetadata", "signal", "abortSignal"],
"src/lib/rag-candidate-sources.ts": ["callVersionedRetrievalRpc", "signal", "abortSignal"],
}
for path, needles in targets.items():
print(f"\n==== {path} ====")
text = Path(path).read_text().splitlines()
for needle in needles:
hits = [i+1 for i,l in enumerate(text) if needle in l]
print(f"{needle}: {hits[:20]}")
PYRepository: BigSimmo/Database
Length of output: 649
🏁 Script executed:
#!/bin/bash
set -eu
grep -nE 'export async function fetchEnabledRagAliases|export async function fetchRelatedDocumentMetadata|function callVersionedRetrievalRpc|signal\\?: AbortSignal|abortSignal\\(' \
src/lib/rag-retrieval-variants.ts src/lib/document-enrichment.ts src/lib/rag-candidate-sources.ts src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '116,150p' src/lib/rag-retrieval-variants.ts
printf '\n----\n'
sed -n '733,770p' src/lib/document-enrichment.tsRepository: BigSimmo/Database
Length of output: 3275
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
path = Path("src/lib/rag-candidate-sources.ts")
lines = path.read_text().splitlines()
# Print chunks around representative call sites and surrounding function signatures.
anchors = [185, 381, 509, 939, 1006, 1071]
for anchor in anchors:
start = max(1, anchor - 20)
end = min(len(lines), anchor + 25)
print(f"\n==== {path}:{start}-{end} (around line {anchor}) ====")
for i in range(start, end + 1):
print(f"{i:>5}: {lines[i-1]}")
PYRepository: BigSimmo/Database
Length of output: 13814
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/lib/rag-candidate-sources.ts")
text = path.read_text().splitlines()
for name in [
"fetchCandidateDocumentChunks",
"fetchCandidateDocuments",
"fetchCandidateTableFacts",
"fetchCandidateEmbeddingFieldsHybrid",
"fetchCandidateIndexUnitsHybrid",
]:
hits = [i+1 for i, line in enumerate(text) if re.search(rf'\b{name}\b', line)]
print(name, hits[:10])
PYRepository: BigSimmo/Database
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/lib/rag-candidate-sources.ts")
text = path.read_text().splitlines()
for name in [
"fetchCandidateDocumentChunks",
"fetchCandidateDocuments",
"fetchCandidateTableFacts",
"fetchCandidateEmbeddingFieldsHybrid",
"fetchCandidateIndexUnitsHybrid",
]:
hits = [i+1 for i, line in enumerate(text) if re.search(rf'\b{name}\b', line)]
print(name, hits[:10])
PYRepository: BigSimmo/Database
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/lib/rag-candidate-sources.ts")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, start=1):
if re.match(r'export async function (fetchCandidate|loadCandidate|searchCandidate|getCandidate)', line):
print(f"{i}: {line}")
PYRepository: BigSimmo/Database
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -eu
ast-grep outline src/lib/rag-candidate-sources.ts --view expandedRepository: BigSimmo/Database
Length of output: 2453
🏁 Script executed:
#!/bin/bash
set -eu
rg -n 'signal\?: AbortSignal|signal:' src/lib/rag-candidate-sources.tsRepository: BigSimmo/Database
Length of output: 155
Preserve abort signals in retrieval fan-out.
fetchEnabledRagAliases and fetchRelatedDocumentMetadata already accept AbortSignal, but src/lib/rag.ts drops args.signal when calling them. The shared RPC wrapper in src/lib/rag-candidate-sources.ts also has no abort path, so the downstream versioned/legacy Supabase RPCs keep running after cancellation.
src/lib/rag.ts#L1549: passargs.signaltofetchRelatedDocumentMetadata(...).src/lib/rag.ts#L2412: passargs.signaltofetchEnabledRagAliases(...).src/lib/rag-candidate-sources.ts#L79: addsignaltocallVersionedRetrievalRpc(...)and forward it to eachrpc(...)call.
📍 Affects 2 files
src/lib/rag.ts#L2410-L2410(this comment)src/lib/rag.ts#L1547-L1554src/lib/rag-candidate-sources.ts#L79-L92
🤖 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` at line 2410, Preserve cancellation signals across retrieval
fan-out: in src/lib/rag.ts lines 1547-1554 and 2410-2410, pass args.signal to
fetchRelatedDocumentMetadata and fetchEnabledRagAliases; in
src/lib/rag-candidate-sources.ts lines 79-92, add a signal parameter to
callVersionedRetrievalRpc and forward it to every rpc call.
| for (const pageRoute of data.pageRoutes) expectDocumentedRoute(pageRoute.route); | ||
| for (const routeHandler of data.publicRouteHandlers) expectDocumentedRoute(routeHandler.route); | ||
| for (const apiRoute of data.apiRoutes) expectDocumentedRoute(apiRoute.route); | ||
| for (const handler of data.appRouteHandlers) expectDocumentedRoute(handler.route); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the stale appRouteHandlers access.
SiteMapData no longer defines appRouteHandlers, so this test no longer type-checks. publicRouteHandlers is already covered on Line 59.
Proposed fix
- for (const handler of data.appRouteHandlers) expectDocumentedRoute(handler.route);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const handler of data.appRouteHandlers) expectDocumentedRoute(handler.route); |
🤖 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/site-map.test.ts` at line 61, Remove the loop iterating over
data.appRouteHandlers in the site-map test, since SiteMapData no longer exposes
that property and publicRouteHandlers is already covered.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/generate-site-map.ts (1)
186-196: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRedirect regex misses multi-argument
redirect()calls.The regex
/\bredirect\(\s*["']([^"']+)["']\s*\)/requires)immediately after the string argument. Next.jsredirect()can accept a secondRedirectTypeargument (e.g.,redirect("/path", RedirectType.replace)), which would not be matched. ThedocumentedRedirectTargetsmap covers the three product handler redirects, but page-level redirects relying on the regex fallback could be silently missed if they ever use the two-argument form.Consider relaxing the regex to stop matching after the string literal:
♻️ Optional refactor
- const target = - documentedRedirectTargets[route.route] ?? source.match(/\bredirect\(\s*["']([^"']+)["']\s*\)/)?.[1]; + const target = + documentedRedirectTargets[route.route] ?? source.match(/\bredirect\(\s*["']([^"']+)["']/)?.[1];🤖 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 186 - 196, Update the fallback redirect pattern in discoverRedirects so it captures the first string argument to redirect() without requiring the closing parenthesis immediately afterward, allowing optional additional arguments such as RedirectType.replace while preserving the existing target extraction and documentedRedirectTargets precedence.
🤖 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 `@tests/audit-navigation-auth-regressions.test.ts`:
- Around line 98-109: The source-contract assertions in the test must match the
current ClinicalDashboard implementation. Update the extracted ranges and
expectations around privateCapabilityContract and pollingContract to reflect the
local-mode exceptions and includeAdministrationData-based polling, or update the
corresponding production logic so those asserted contracts are restored.
---
Nitpick comments:
In `@scripts/generate-site-map.ts`:
- Around line 186-196: Update the fallback redirect pattern in discoverRedirects
so it captures the first string argument to redirect() without requiring the
closing parenthesis immediately afterward, allowing optional additional
arguments such as RedirectType.replace while preserving the existing target
extraction and documentedRedirectTargets precedence.
🪄 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: 6b49e86e-62a0-4165-b025-3ea7a867196b
📒 Files selected for processing (23)
docs/codebase-index.mddocs/site-map.mdplaywright.config.tsscripts/check-github-action-pins.mjsscripts/generate-site-map.tssrc/app/layout.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
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/ui-accessibility.spec.ts
- src/app/layout.tsx
- tests/site-map.test.ts
- scripts/check-github-action-pins.mjs
- tests/ui-smoke.spec.ts
- src/components/forms/forms-home-page.tsx
- src/components/clinical-dashboard/master-search-header.tsx
- supabase/migrations/20260714180000_patch_rag_and_corrector_scalability.sql
- src/components/services/services-navigator-page.tsx
- tests/ui-tools.spec.ts
- supabase/schema.sql
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 `@playwright.config.ts`:
- Line 58: Move reducedMotion from the use configuration into the Chromium
override’s contextOptions object in the Playwright configuration. Preserve the
"no-preference" value and leave unrelated browser settings unchanged so the
configuration conforms to Playwright’s supported types.
🪄 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: 58278ed8-db45-4a3b-963a-99145c4014f5
📒 Files selected for processing (3)
playwright.config.tstests/rag-abort-signal.test.tstests/rag-classifier-memo.test.ts
349b46c to
67c3ab1
Compare
1 participant
Summary
Verification
npm run lintnpm run test -- tests/site-map.test.ts tests/supabase-schema.test.tsnpm run typecheckfailed in this environment due V8 heap OOM unless Node memory is increased for local execution.npm run buildhits a Next.jsERR_INVALID_ARG_TYPEin this local checkout and should be validated through CI.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Risk and rollout
mainif any functional regressions are observed.Summary by CodeRabbit