refactor: harden RAG and dashboard boundaries#457
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe changes reorganize dashboard contracts and clinical-output rendering, extract RAG contracts and answer helpers, bound retrieval caches, centralize local-server utilities, add dependency-boundary tests, and update architecture, deployment, and review documentation. ChangesDashboard UI and client boundaries
RAG contracts, helpers, and caching
Architecture validation and local infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GlobalMockupSearchShellClient
participant ClinicalDashboard
participant GlobalMockupStandaloneSearchShellClient
GlobalMockupSearchShellClient->>GlobalMockupSearchShellClient: derive route and search state
GlobalMockupSearchShellClient->>ClinicalDashboard: lazy-load and render dashboard
GlobalMockupSearchShellClient->>GlobalMockupStandaloneSearchShellClient: render standalone shell for excluded routes
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/lib/local-server-utils.mjs`:
- Around line 7-8: Update normalizeProjectRoot to preserve case on
case-sensitive filesystems and leave POSIX backslashes unchanged; only normalize
separators and lowercase when running on a case-insensitive host. Add
platform-aware tests covering distinct roots’ generated project IDs and stable
ports to ensure they do not collide.
In `@tests/architecture-boundaries.test.ts`:
- Around line 145-153: Update the cycle detection logic in the strongly
connected component traversal to retain singleton components when the file has a
runtime self-import, instead of only pushing components with length greater than
one. Use the existing graph/runtime-edge data to distinguish genuine self-import
cycles from ordinary single-file components before appending to cycles.
- Around line 73-80: Broaden the dynamic-import handling in the visit function
to accept statically resolvable imports with an optional second options argument
and no-substitution template literals, while still rejecting non-static
expressions; add each resolved module specifier to dynamicImports so runtime
graph and boundary checks include these edges.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c67e82d3-7741-48dc-977e-69a226eda800
📒 Files selected for processing (31)
docs/branch-review-ledger.mddocs/codebase-index.mddocs/deployment-architecture.mdscripts/deployment-boot-smoke.mjsscripts/dev-free-port.mjsscripts/ensure-local-server.mjsscripts/guard-next-build.mjsscripts/playwright-base-url.tsscripts/run-playwright.mjssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/clinical-output-helpers.tsxsrc/components/clinical-dashboard/dashboard-contracts.tssrc/components/clinical-dashboard/dashboard-nav.tsxsrc/components/clinical-dashboard/document-admin.tsxsrc/components/clinical-dashboard/evidence-panels.tsxsrc/components/clinical-dashboard/global-mockup-search-shell.tsxsrc/components/clinical-dashboard/output-panel.tsxsrc/lib/bounded-ttl-cache.tssrc/lib/client-env.tssrc/lib/local-project-guard.tssrc/lib/local-server-utils.mjssrc/lib/rag-answer-support.tssrc/lib/rag-cache.tssrc/lib/rag-contracts.tssrc/lib/rag-extractive-answer.tssrc/lib/rag-query-guard.tssrc/lib/rag-retrieval-variants.tssrc/lib/rag.tstests/architecture-boundaries.test.tstests/bounded-ttl-cache.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/lib/local-server-utils.mjs`:
- Around line 7-8: Update normalizeProjectRoot to preserve case on
case-sensitive filesystems and leave POSIX backslashes unchanged; only normalize
separators and lowercase when running on a case-insensitive host. Add
platform-aware tests covering distinct roots’ generated project IDs and stable
ports to ensure they do not collide.
In `@tests/architecture-boundaries.test.ts`:
- Around line 145-153: Update the cycle detection logic in the strongly
connected component traversal to retain singleton components when the file has a
runtime self-import, instead of only pushing components with length greater than
one. Use the existing graph/runtime-edge data to distinguish genuine self-import
cycles from ordinary single-file components before appending to cycles.
- Around line 73-80: Broaden the dynamic-import handling in the visit function
to accept statically resolvable imports with an optional second options argument
and no-substitution template literals, while still rejecting non-static
expressions; add each resolved module specifier to dynamicImports so runtime
graph and boundary checks include these edges.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c67e82d3-7741-48dc-977e-69a226eda800
📒 Files selected for processing (31)
docs/branch-review-ledger.mddocs/codebase-index.mddocs/deployment-architecture.mdscripts/deployment-boot-smoke.mjsscripts/dev-free-port.mjsscripts/ensure-local-server.mjsscripts/guard-next-build.mjsscripts/playwright-base-url.tsscripts/run-playwright.mjssrc/components/ClinicalDashboard.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/clinical-output-helpers.tsxsrc/components/clinical-dashboard/dashboard-contracts.tssrc/components/clinical-dashboard/dashboard-nav.tsxsrc/components/clinical-dashboard/document-admin.tsxsrc/components/clinical-dashboard/evidence-panels.tsxsrc/components/clinical-dashboard/global-mockup-search-shell.tsxsrc/components/clinical-dashboard/output-panel.tsxsrc/lib/bounded-ttl-cache.tssrc/lib/client-env.tssrc/lib/local-project-guard.tssrc/lib/local-server-utils.mjssrc/lib/rag-answer-support.tssrc/lib/rag-cache.tssrc/lib/rag-contracts.tssrc/lib/rag-extractive-answer.tssrc/lib/rag-query-guard.tssrc/lib/rag-retrieval-variants.tssrc/lib/rag.tstests/architecture-boundaries.test.tstests/bounded-ttl-cache.test.ts
🛑 Comments failed to post (3)
src/lib/local-server-utils.mjs (1)
7-8: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve path semantics on case-sensitive systems.
normalizeProjectRootlowercases every path and rewrites backslashes even on POSIX. Distinct roots such as/work/Appand/work/appcan therefore receive the same project ID and stable port;ensure-local-server.mjsmay then treat another project's server as the requested one. Lowercase only where the host filesystem is case-insensitive, and do not reinterpret POSIX backslashes.Proposed direction
-import crypto from "node:crypto"; +import crypto from "node:crypto"; +import { resolve } from "node:path"; export function normalizeProjectRoot(projectRoot) { - return projectRoot.replaceAll("\\", "/").toLowerCase(); + const resolvedRoot = resolve(projectRoot); + return process.platform === "win32" + ? resolvedRoot.replaceAll("\\", "/").toLowerCase() + : resolvedRoot; }Add platform-aware collision tests for the generated project ID and port.
📝 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.import crypto from "node:crypto"; import { resolve } from "node:path"; export function normalizeProjectRoot(projectRoot) { const resolvedRoot = resolve(projectRoot); return process.platform === "win32" ? resolvedRoot.replaceAll("\\", "/").toLowerCase() : resolvedRoot; }🤖 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/local-server-utils.mjs` around lines 7 - 8, Update normalizeProjectRoot to preserve case on case-sensitive filesystems and leave POSIX backslashes unchanged; only normalize separators and lowercase when running on a case-insensitive host. Add platform-aware tests covering distinct roots’ generated project IDs and stable ports to ensure they do not collide.tests/architecture-boundaries.test.ts (2)
73-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -nP --glob '*.{ts,tsx,js,jsx,mjs}' 'import\s*\([^)]*,|import\s*\(`[^$`]*`\s*\)' src workerRepository: BigSimmo/Database
Length of output: 1986
Broaden dynamic-import detection. The parser only accepts
import("literal")with exactly one argument, so it misses statically resolvableimport("literal", options)and no-substitution template literals. Those edges never reach the runtime graph or boundary check.🤖 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/architecture-boundaries.test.ts` around lines 73 - 80, Broaden the dynamic-import handling in the visit function to accept statically resolvable imports with an optional second options argument and no-substitution template literals, while still rejecting non-static expressions; add each resolved module specifier to dynamicImports so runtime graph and boundary checks include these edges.
145-153: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Count self-imports as runtime cycles.
A strongly connected component containing one file is currently discarded, so a module that imports itself passes the “no runtime import cycles” test.
Proposed fix
- if (component.length > 1) cycles.push(component); + const hasSelfCycle = graph.get(file)?.includes(file) ?? false; + if (component.length > 1 || hasSelfCycle) cycles.push(component);📝 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.if (lowLinks.get(file) !== indexes.get(file)) return; const component: string[] = []; let current: string; do { current = stack.pop()!; onStack.delete(current); component.push(current); } while (current !== file); const hasSelfCycle = graph.get(file)?.includes(file) ?? false; if (component.length > 1 || hasSelfCycle) cycles.push(component);🤖 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/architecture-boundaries.test.ts` around lines 145 - 153, Update the cycle detection logic in the strongly connected component traversal to retain singleton components when the file has a runtime self-import, instead of only pushing components with length greater than one. Use the existing graph/runtime-edge data to distinguish genuine self-import cycles from ordinary single-file components before appending to cycles.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
1 participant
Summary
Why
The architecture reviews found repeatable cycle, cache-growth, client/server-boundary, lazy-loading, and duplicate-subscription risks. This branch replays the reviewed fixes onto the current
maintree without altering the already-merged PR verification or Codex autofix work.Areas touched
RAG orchestration and cache modules, dashboard/evidence components, local-server helpers, architecture documentation, and focused architecture tests.
Verification
npm run check:runtime— passednpm run check:github-actions— passednpm run sitemap:check— passednpm run lint— passednpm run typecheck— passed--testTimeout=30000— 1,433 passed, 1 skippedgit diff --check— passedNot run
Browser/UI checks, provider-backed Supabase/OpenAI checks, release gates, and deployment were not run. They remain outside the local/offline verification boundary.
Risk and governance
This touches clinical RAG internals but does not change provider credentials, database schema, production data, or deployment configuration. The existing fail-closed clinical tests remain green. The main residual risk is browser-level behavior around the dashboard lazy-loading/subscription changes.