chore: apply 51278a70d remediations onto main lineage#850
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe change updates RAG retrieval and enrichment, Supabase cleanup and query correction, sitemap generation, service/form interfaces, Playwright behavior, and regression tests. ChangesApplication and data-platform updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
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 (3)
playwright.config.ts (1)
46-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemove the Chromium reduced-motion override.
Playwright mergesuseshallowly, socontextOptions: { reducedMotion: "no-preference" }replaces the suite-widereducedMotion: "reduce"for Chromium production specs. Drop this override or scope it only to the motion-specific test.🤖 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` around lines 46 - 58, Remove the Chromium project’s contextOptions reducedMotion override from the chromium project in projects, allowing the suite-wide reducedMotion: "reduce" setting to apply to production specs. Do not alter unrelated device or test-matching configuration.supabase/drift-manifest.json (1)
6331-6334: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRegenerate the manifest with
documents_sync_title_wordspresent.The July 14 migration creates this trigger, and no later migration drops it, but the regenerated snapshot omits it. This masks real drift and leaves newly indexed or renamed documents absent from the correction vocabulary.
🤖 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, Regenerate supabase/drift-manifest.json from the current migrations so it includes the documents_sync_title_words trigger alongside documents_updated_at. Verify the regenerated snapshot preserves both triggers and reflects the July 14 migration without manually editing the manifest.src/lib/rag.ts (1)
1640-1660: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope direct visual-evidence lookups to the authorized documents.
The admin-client query fetches
sourceImageIdswithout a document filter, then attaches matches by ID alone. A malformed cross-document reference can expose another tenant’s image metadata. Add.in("document_id", documentIds)and validate the image document before attachment.Proposed query fix
.select(selectColumns) .in("id", sourceImageIds) + .in("document_id", documentIds) .eq("searchable", true)🤖 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 1640 - 1660, Restrict the direct visual-evidence query in the Promise.all flow to the authorized documentIds by adding the document filter alongside the existing sourceImageIds filter. Before attaching each direct match, validate that its document_id belongs to documentIds, using the existing attachment logic’s relevant symbol, and skip any cross-document result.
🤖 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 `@scripts/check-github-action-pins.mjs`:
- Around line 13-15: Update discoverGitHubActionFiles to throw a descriptive
error when the .github/workflows directory is absent instead of returning an
empty array, preserving fail-closed behavior for invalid or incomplete
repository roots.
In `@src/components/services/services-navigator-page.tsx`:
- Around line 497-502: Update the comparison link rendered in the services
navigator to include the service name in its accessible label, using the
available service identity such as service.name or service.slug while keeping
the visible “Open” text unchanged. Ensure each Link in the service comparison
list has a distinct screen-reader name.
In `@supabase/migrations/20260717010000_harden_rag_scalability_patch.sql`:
- Around line 31-114: Update correct_clinical_query_terms to accept
owner/include-public scope parameters, restrict rag_aliases candidates by
owner_id and public visibility, and source document_title_words through a scoped
documents join. Update every RPC caller to pass the appropriate scope, then
regenerate the database types and adjust callers to match the new function
signature.
In `@tests/audit-navigation-auth-regressions.test.ts`:
- Around line 98-101: Update the assertions for privateCapabilityContract to
match the actual private-API gate in ClinicalDashboard.tsx: either remove
localNoAuthMode and localDevCanAttemptPrivateApis from the implementation so the
authenticated-only expression is exact, or change the test to assert the
intended full expression including those symbols. Adjust the negative regex
accordingly so it checks the implementation’s real behavior.
In `@tests/site-map.test.ts`:
- Around line 100-132: Remove the duplicate test named “keeps public redirect
handlers in product routes and API handlers in the API section” from the later
block, preserving the earlier test with the correct assertions and avoiding
duplicate coverage.
In `@tests/supabase-schema.test.ts`:
- Around line 1208-1214: Update the assertions for default_privileges_status in
the schema test to extract the final function body using finalSqlSegment before
checking ACL expressions. Run the existing toContain probes against that scoped
segment rather than the whole sql string, preserving all current ACL
expectations.
---
Outside diff comments:
In `@playwright.config.ts`:
- Around line 46-58: Remove the Chromium project’s contextOptions reducedMotion
override from the chromium project in projects, allowing the suite-wide
reducedMotion: "reduce" setting to apply to production specs. Do not alter
unrelated device or test-matching configuration.
In `@src/lib/rag.ts`:
- Around line 1640-1660: Restrict the direct visual-evidence query in the
Promise.all flow to the authorized documentIds by adding the document filter
alongside the existing sourceImageIds filter. Before attaching each direct
match, validate that its document_id belongs to documentIds, using the existing
attachment logic’s relevant symbol, and skip any cross-document result.
In `@supabase/drift-manifest.json`:
- Around line 6331-6334: Regenerate supabase/drift-manifest.json from the
current migrations so it includes the documents_sync_title_words trigger
alongside documents_updated_at. Verify the regenerated snapshot preserves both
triggers and reflects the July 14 migration without manually editing the
manifest.
🪄 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: 2493d6d9-d6d0-4626-9e88-c31abe31ddff
📒 Files selected for processing (27)
docs/branch-review-ledger.mddocs/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/rag-variant-early-exit.test.tstests/site-map.test.tstests/supabase-schema.test.tstests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/ui-tools.spec.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/app/page.tsx
| function discoverGitHubActionFiles(workflowRoot) { | ||
| const workflowDir = path.join(workflowRoot, ".github", "workflows"); | ||
| if (!existsSync(workflowDir)) return []; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when the workflows directory is missing.
Returning [] makes this pin-policy check succeed from the wrong repository root or an incomplete checkout. Preserve the previous failure behavior by throwing a descriptive error when .github/workflows is absent.
🤖 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/check-github-action-pins.mjs` around lines 13 - 15, Update
discoverGitHubActionFiles to throw a descriptive error when the
.github/workflows directory is absent instead of returning an empty array,
preserving fail-closed behavior for invalid or incomplete repository roots.
| <Link | ||
| href={`/services/${service.slug}`} | ||
| className="inline-flex min-h-tap items-center text-xs font-bold text-[color:var(--clinical-accent)] sm:min-h-0" | ||
| > | ||
| Open | ||
| </Link> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Give each comparison link a service-specific accessible name.
Every link is announced as “Open”, making the destinations indistinguishable in a screen-reader link list.
Proposed fix
<Link
href={`/services/${service.slug}`}
+ aria-label={`Open ${service.title}`}
className="inline-flex min-h-tap items-center text-xs font-bold text-[color:var(--clinical-accent)] sm:min-h-0"
>📝 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.
| <Link | |
| href={`/services/${service.slug}`} | |
| className="inline-flex min-h-tap items-center text-xs font-bold text-[color:var(--clinical-accent)] sm:min-h-0" | |
| > | |
| Open | |
| </Link> | |
| <Link | |
| href={`/services/${service.slug}`} | |
| aria-label={`Open ${service.title}`} | |
| className="inline-flex min-h-tap items-center text-xs font-bold text-[color:var(--clinical-accent)] sm:min-h-0" | |
| > | |
| Open | |
| </Link> |
🤖 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/services/services-navigator-page.tsx` around lines 497 - 502,
Update the comparison link rendered in the services navigator to include the
service name in its accessible label, using the available service identity such
as service.name or service.slug while keeping the visible “Open” text unchanged.
Ensure each Link in the service comparison list has a distinct screen-reader
name.
| create or replace function public.correct_clinical_query_terms(input_query text, min_sim real default 0.45) | ||
| returns text | ||
| language plpgsql | ||
| stable security definer | ||
| set search_path to 'public', 'extensions', 'pg_temp' | ||
| set pg_trgm.similarity_threshold = 0.3 | ||
| as $$ | ||
| declare | ||
| tokens text[]; | ||
| tok text; | ||
| best text; | ||
| best_sim real; | ||
| corrected text[] := array[]::text[]; | ||
| changed boolean := false; | ||
| begin | ||
| if min_sim is null or min_sim < 0.3 or min_sim > 1 then | ||
| raise exception 'min_sim must be between 0.3 and 1.0' using errcode = '22023'; | ||
| end if; | ||
|
|
||
| if input_query is null or length(trim(input_query)) = 0 then | ||
| return input_query; | ||
| end if; | ||
|
|
||
| tokens := regexp_split_to_array(lower(input_query), '\s+'); | ||
| foreach tok in array tokens loop | ||
| if length(tok) < 4 then | ||
| corrected := corrected || tok; | ||
| continue; | ||
| end if; | ||
|
|
||
| best := null; | ||
| best_sim := 0; | ||
| select candidate.term, similarity(candidate.term, tok) | ||
| into best, best_sim | ||
| from ( | ||
| ( | ||
| select lower(alias) as term | ||
| from public.rag_aliases | ||
| where enabled | ||
| and length(alias) between 4 and 40 | ||
| and lower(alias) % tok | ||
| order by similarity(lower(alias), tok) desc, lower(alias) | ||
| limit 32 | ||
| ) | ||
| union all | ||
| ( | ||
| select lower(canonical) as term | ||
| from public.rag_aliases | ||
| where enabled | ||
| and length(canonical) between 4 and 40 | ||
| and lower(canonical) % tok | ||
| order by similarity(lower(canonical), tok) desc, lower(canonical) | ||
| limit 32 | ||
| ) | ||
| union all | ||
| ( | ||
| select word as term | ||
| from public.document_title_words | ||
| where length(word) between 4 and 40 | ||
| and word % tok | ||
| order by similarity(word, tok) desc, word | ||
| limit 32 | ||
| ) | ||
| ) candidate | ||
| order by similarity(candidate.term, tok) desc, candidate.term | ||
| limit 1; | ||
|
|
||
| if best is not null and best_sim >= min_sim and best <> tok and length(best) >= length(tok) then | ||
| corrected := corrected || best; | ||
| changed := true; | ||
| else | ||
| corrected := corrected || tok; | ||
| end if; | ||
| end loop; | ||
|
|
||
| if not changed then | ||
| return input_query; | ||
| end if; | ||
| return array_to_string(corrected, ' '); | ||
| end; | ||
| $$; | ||
|
|
||
| revoke execute on function public.correct_clinical_query_terms(text, real) from public, anon, authenticated; | ||
| grant execute on function public.correct_clinical_query_terms(text, real) to service_role; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Scope query-correction vocabulary by retrieval access.
This SECURITY DEFINER function searches every tenant’s aliases and document-title words. Because callers use the service-role client and pass no owner scope, one tenant’s private vocabulary can alter or reveal corrections for another tenant.
Add owner/include-public parameters, filter rag_aliases.owner_id, and join document_title_words to scoped documents; then update both RPC callers and generated types.
🤖 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/20260717010000_harden_rag_scalability_patch.sql` around
lines 31 - 114, Update correct_clinical_query_terms to accept
owner/include-public scope parameters, restrict rag_aliases candidates by
owner_id and public visibility, and source document_title_words through a scoped
documents join. Update every RPC caller to pass the appropriate scope, then
regenerate the database types and adjust callers to match the new function
signature.
| expect(privateCapabilityContract).toContain( | ||
| 'const canUsePrivateApis = localProjectReady && authStatus === "authenticated";', | ||
| ); | ||
| expect(privateCapabilityContract).not.toMatch(/localNoAuth|clientDemoMode/); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Align this assertion with the actual private-API gate.
ClinicalDashboard.tsx still includes localNoAuthMode and localDevCanAttemptPrivateApis, so the exact authenticated-only assertion and negative regex will fail. Either change the implementation or assert its intended full expression.
🤖 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/audit-navigation-auth-regressions.test.ts` around lines 98 - 101,
Update the assertions for privateCapabilityContract to match the actual
private-API gate in ClinicalDashboard.tsx: either remove localNoAuthMode and
localDevCanAttemptPrivateApis from the implementation so the authenticated-only
expression is exact, or change the test to assert the intended full expression
including those symbols. Adjust the negative regex accordingly so it checks the
implementation’s real behavior.
| it("keeps public redirect handlers in product routes and API handlers in the API section", () => { | ||
| const data = collectSiteMapData(); | ||
| const productSection = siteMap.slice( | ||
| siteMap.indexOf("## Main product routes"), | ||
| siteMap.indexOf("## Mode/query routes"), | ||
| ); | ||
| const apiSection = siteMap.slice(siteMap.indexOf("## API routes"), siteMap.indexOf("## Redirects")); | ||
| const expectedProductHandlers = [ | ||
| ["/applications", "src/app/applications/route.ts", "/tools"], | ||
| [ | ||
| "/differentials/presentations", | ||
| "src/app/differentials/presentations/route.ts", | ||
| "/differentials/presentations/[workflow-slug]", | ||
| ], | ||
| ["/medications", "src/app/medications/route.ts", "/?mode=prescribing"], | ||
| ] as const; | ||
|
|
||
| expect(data.apiRoutes.every((route) => route.route === "/api" || route.route.startsWith("/api/"))).toBe(true); | ||
| expect(data.publicRouteHandlers.some((route) => route.route === "/auth/callback")).toBe(true); | ||
| expect(data.publicRouteHandlers).toContainEqual({ | ||
| route: "/icons/[variant]", | ||
| file: "src/app/icons/[variant]/route.tsx", | ||
| }); | ||
| expect(apiSection).not.toContain("`/icons/[variant]`"); | ||
|
|
||
| for (const [route, file, target] of expectedProductHandlers) { | ||
| expect(data.publicRouteHandlers).toContainEqual({ route, file }); | ||
| expect(data.apiRoutes).not.toContainEqual({ route, file }); | ||
| expect(data.redirects).toContainEqual({ route, file, target }); | ||
| expect(productSection).toContain(`\`${route}\``); | ||
| expect(apiSection).not.toContain(`\`${route}\``); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Duplicate test with incorrect assertions will fail.
Lines 100-132 duplicate the test at lines 64-98 with the same it() title. The critical difference is line 129: expect(productSection).toContain(\${route}`) asserts that product handler routes (/applications, /differentials/presentations, /medications) appear in the "Main product routes" section. However, these are route.tshandler files, notpage.tsxfiles — the product section only rendersdata.pageRoutes`, so these routes are absent from it. This assertion will fail for all three expected handlers.
Test 1 (line 96) correctly asserts not.toContain. Test 2 should be removed or its line 129 assertion corrected to match Test 1.
🐛 Proposed fix: remove the duplicate test
- it("keeps public redirect handlers in product routes and API handlers in the API section", () => {
- const data = collectSiteMapData();
- const productSection = siteMap.slice(
- siteMap.indexOf("## Main product routes"),
- siteMap.indexOf("## Mode/query routes"),
- );
- const apiSection = siteMap.slice(siteMap.indexOf("## API routes"), siteMap.indexOf("## Redirects"));
- const expectedProductHandlers = [
- ["/applications", "src/app/applications/route.ts", "/tools"],
- [
- "/differentials/presentations",
- "src/app/differentials/presentations/route.ts",
- "/differentials/presentations/[workflow-slug]",
- ],
- ["/medications", "src/app/medications/route.ts", "/?mode=prescribing"],
- ] as const;
-
- expect(data.apiRoutes.every((route) => route.route === "/api" || route.route.startsWith("/api/"))).toBe(true);
- expect(data.publicRouteHandlers.some((route) => route.route === "/auth/callback")).toBe(true);
- expect(data.publicRouteHandlers).toContainEqual({
- route: "/icons/[variant]",
- file: "src/app/icons/[variant]/route.tsx",
- });
- expect(apiSection).not.toContain("`/icons/[variant]`");
-
- for (const [route, file, target] of expectedProductHandlers) {
- expect(data.publicRouteHandlers).toContainEqual({ route, file });
- expect(data.apiRoutes).not.toContainEqual({ route, file });
- expect(data.redirects).toContainEqual({ route, file, target });
- expect(productSection).toContain(`\`${route}\``);
- expect(apiSection).not.toContain(`\`${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.
| it("keeps public redirect handlers in product routes and API handlers in the API section", () => { | |
| const data = collectSiteMapData(); | |
| const productSection = siteMap.slice( | |
| siteMap.indexOf("## Main product routes"), | |
| siteMap.indexOf("## Mode/query routes"), | |
| ); | |
| const apiSection = siteMap.slice(siteMap.indexOf("## API routes"), siteMap.indexOf("## Redirects")); | |
| const expectedProductHandlers = [ | |
| ["/applications", "src/app/applications/route.ts", "/tools"], | |
| [ | |
| "/differentials/presentations", | |
| "src/app/differentials/presentations/route.ts", | |
| "/differentials/presentations/[workflow-slug]", | |
| ], | |
| ["/medications", "src/app/medications/route.ts", "/?mode=prescribing"], | |
| ] as const; | |
| expect(data.apiRoutes.every((route) => route.route === "/api" || route.route.startsWith("/api/"))).toBe(true); | |
| expect(data.publicRouteHandlers.some((route) => route.route === "/auth/callback")).toBe(true); | |
| expect(data.publicRouteHandlers).toContainEqual({ | |
| route: "/icons/[variant]", | |
| file: "src/app/icons/[variant]/route.tsx", | |
| }); | |
| expect(apiSection).not.toContain("`/icons/[variant]`"); | |
| for (const [route, file, target] of expectedProductHandlers) { | |
| expect(data.publicRouteHandlers).toContainEqual({ route, file }); | |
| expect(data.apiRoutes).not.toContainEqual({ route, file }); | |
| expect(data.redirects).toContainEqual({ route, file, target }); | |
| expect(productSection).toContain(`\`${route}\``); | |
| expect(apiSection).not.toContain(`\`${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` around lines 100 - 132, Remove the duplicate test
named “keeps public redirect handlers in product routes and API handlers in the
API section” from the later block, preserving the earlier test with the correct
assertions and avoiding duplicate coverage.
| expect(sql).toContain("create or replace function public.default_privileges_status("); | ||
| expect(sql).toContain("pg_catalog.acldefault(ot.object_code, v_role_oid)"); | ||
| expect(sql).toContain("pg_catalog.aclexplode(ea.acl)"); | ||
| expect(sql).toContain("bool_or(grantee not in (p_role_name, 'postgres', 'service_role'))"); | ||
| expect(sql).toContain("entry like 'table:PUBLIC:%'"); | ||
| expect(sql).toContain("entry like 'sequence:PUBLIC:%'"); | ||
| expect(sql).toContain("entry = 'function:PUBLIC:execute'"); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope the ACL probes to the final function definition.
Whole-file toContain checks can pass if these probes survive in comments or an obsolete definition while the effective default_privileges_status function regresses. Extract its final body with finalSqlSegment before asserting the ACL logic.
🤖 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 1208 - 1214, Update the
assertions for default_privileges_status in the schema test to extract the final
function body using finalSqlSegment before checking ACL expressions. Run the
existing toContain probes against that scoped segment rather than the whole sql
string, preserving all current ACL expectations.
Merges design-audit remediation changes from commit 51278a7 and resolves merge context to main. Includes targeted fixes in rag call safety, source validation, and source-status assertions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation