fix: harden Domain 2 document and auth workflows#533
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates document routing and mockup boundaries, introduces a canonical search shell, adds auth-epoch request cancellation, persists private search scopes, standardizes API errors, models answer and upload lifecycles, and expands local verification and UI coverage. ChangesFrontend platform changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
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 |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@copilot resolve the merge conflicts in this pull request |
Head branch was pushed to by a user without write access
Merge conflicts resolved in commit
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/ui-accessibility.spec.ts (1)
98-108: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider exercising the scope popover under 200% zoom too.
The forced-colors test right above (lines 89-96) opens the scope command popover and checks overflow, but this new zoom test only checks the base dashboard layout. Popover/menu positioning is a common failure point at high zoom levels, so this leaves a coverage gap for the exact scenario this test is meant to protect.
♻️ Suggested addition
await expectDashboardUsable(page); + await openScopeControl(page); await expectNoPageHorizontalOverflow(page); });🤖 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` around lines 98 - 108, Extend the “dashboard remains usable at 200 percent zoom” test to open the scope command popover using the existing interaction pattern from the nearby forced-colors test, then verify the popover remains usable without horizontal overflow. Keep the existing base-dashboard assertions and perform the popover checks after applying the 200% zoom.src/lib/ward-output.ts (1)
638-666: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate table-rendering logic vs
clinicalTableToTextRows.The markdown header/separator/body-slice(6) rendering block here is copy/pasted from
clinicalTableToTextRows(lines 612-636 in this file). Consider extracting a shared helper that takes aNormalizedAccessibleTable(plus a low-confidence message string) and returns the markdown lines, then have both functions call it with their own caption/source wrapping.♻️ Suggested extraction
function renderNormalizedTableLines( normalized: NormalizedAccessibleTable, lowConfidenceMessage: string, ) { if (normalized.lowConfidence) return [lowConfidenceMessage]; return [ normalized.header.length ? `| ${normalized.header.join(" | ")} |` : "", normalized.header.length ? `| ${normalized.header.map(() => "---").join(" | ")} |` : "", ...normalized.body.slice(0, 6).map((row) => `| ${row.join(" | ")} |`), ].filter(Boolean); }Both
clinicalTableToTextRowsandformatDisplayedVisualEvidenceForClipboardcan then call this and prepend/append their own caption/source lines.🤖 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/ward-output.ts` around lines 638 - 666, Extract the duplicated normalized-table markdown rendering from clinicalTableToTextRows and formatDisplayedVisualEvidenceForClipboard into a shared helper that accepts NormalizedAccessibleTable and a low-confidence message, returning the appropriate markdown lines. Update both functions to use the helper while preserving their existing caption, source, and surrounding output lines.src/lib/supabase/client.tsx (1)
106-106: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
useRefinitializer re-evaluated every render.
useRef(createAuthRequestLifecycle())constructs a new lifecycle object (with its ownSet) on every render; only the first result is kept, so subsequent constructions are wasted. Use lazy initialization to avoid the redundant allocation.♻️ Lazy-init suggestion
- const authRequestsRef = useRef(createAuthRequestLifecycle()); + const authRequestsRef = useRef<ReturnType<typeof createAuthRequestLifecycle>>(); + if (!authRequestsRef.current) authRequestsRef.current = createAuthRequestLifecycle();🤖 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/supabase/client.tsx` at line 106, Update the authRequestsRef initialization in the client component to lazily create the lifecycle object only when the ref has no current value, avoiding createAuthRequestLifecycle() on subsequent renders while preserving the existing ref instance.src/lib/private-search-scope.ts (1)
11-32: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: orphaned scope entries accumulate within a session.
persistPrivateSearchScopemints a freshscopeRefkey on every call, and callers (executeSearch/ask) invoke it on each search that has a document selection. Previous entries are never removed until they're individually read back (or the tab closes), so a long session with many scoped searches leaves stale keys insessionStorage. Consider pruning expired/oldest entries understoragePrefixwhen persisting.🤖 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/private-search-scope.ts` around lines 11 - 32, Update persistPrivateSearchScope to prune stale entries under storagePrefix before or while writing a new scope, removing expired entries and, if necessary, the oldest entries so repeated scoped searches do not accumulate orphaned sessionStorage keys. Preserve valid active scopes and the existing maxDocumentIds and scopeRef validation behavior.
🤖 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/clinical-dashboard/DocumentManagerPanel.tsx`:
- Around line 357-374: Update the batch completion logic around the
failures.length === 0 branch so input.value is cleared and onUploaded() is
invoked whenever queued.length is greater than zero, including partial-failure
batches. Preserve the existing success and failure status messages, and avoid
refreshing when no documents were queued successfully.
In `@src/components/ClinicalDashboard.tsx`:
- Line 3575: Add aria-hidden="true" to the decorative RefreshCw icon in the “Run
again” button, matching the existing accessible RefreshCw usages in
ClinicalDashboard.
In `@src/lib/answer-render-policy.ts`:
- Around line 544-546: Update the visual evidence clipboard assembly around
formatDisplayedVisualEvidenceForClipboard to first compute the formatted rows
and add the "Displayed table evidence" heading only when that formatted output
is non-empty. Preserve the existing output for evidence containing parsable
table data and omit the heading for image-only or otherwise unparseable
evidence.
In `@src/lib/supabase/client.tsx`:
- Around line 286-296: Update the auth fingerprint in the useEffect to exclude
accessToken, keying it only on status and session?.user.id. Remove accessToken
from the effect dependencies if it is no longer referenced, while preserving
invalidateAuthRequests for genuine session changes and leaving 401
token-expiration handling unchanged.
---
Nitpick comments:
In `@src/lib/private-search-scope.ts`:
- Around line 11-32: Update persistPrivateSearchScope to prune stale entries
under storagePrefix before or while writing a new scope, removing expired
entries and, if necessary, the oldest entries so repeated scoped searches do not
accumulate orphaned sessionStorage keys. Preserve valid active scopes and the
existing maxDocumentIds and scopeRef validation behavior.
In `@src/lib/supabase/client.tsx`:
- Line 106: Update the authRequestsRef initialization in the client component to
lazily create the lifecycle object only when the ref has no current value,
avoiding createAuthRequestLifecycle() on subsequent renders while preserving the
existing ref instance.
In `@src/lib/ward-output.ts`:
- Around line 638-666: Extract the duplicated normalized-table markdown
rendering from clinicalTableToTextRows and
formatDisplayedVisualEvidenceForClipboard into a shared helper that accepts
NormalizedAccessibleTable and a low-confidence message, returning the
appropriate markdown lines. Update both functions to use the helper while
preserving their existing caption, source, and surrounding output lines.
In `@tests/ui-accessibility.spec.ts`:
- Around line 98-108: Extend the “dashboard remains usable at 200 percent zoom”
test to open the scope command popover using the existing interaction pattern
from the nearby forced-colors test, then verify the popover remains usable
without horizontal overflow. Keep the existing base-dashboard assertions and
perform the popover checks after applying the 200% zoom.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 112684a6-20cb-430e-8f61-dd010d7bd88b
📒 Files selected for processing (45)
docs/codebase-index.mddocs/frontend-architecture.mddocs/site-map.mdscripts/generate-site-map.tsscripts/verify-pr-local.mjssrc/app/api/answer/stream/route.tssrc/app/documents/search/page.tsxsrc/app/documents/source/evidence/page.tsxsrc/app/documents/source/page.tsxsrc/app/mockups/document-search/search/page.tsxsrc/app/mockups/document-search/source/page.tsxsrc/components/ClinicalDashboard.tsxsrc/components/DocumentManagementActions.tsxsrc/components/DocumentViewer.tsxsrc/components/clinical-dashboard/DocumentManagerPanel.tsxsrc/components/clinical-dashboard/global-mockup-search-shell.tsxsrc/components/clinical-dashboard/global-search-shell.tsxsrc/components/clinical-dashboard/search-utils.tssrc/components/document-search-mockups.tsxsrc/components/master-document-flow-mockups.tsxsrc/lib/answer-lifecycle.tssrc/lib/answer-render-policy.tssrc/lib/api-client-error.tssrc/lib/api-rate-limit.tssrc/lib/auth-request-lifecycle.tssrc/lib/document-flow-routes.tssrc/lib/http.tssrc/lib/private-search-scope.tssrc/lib/search-navigation-context.tssrc/lib/supabase/client.tsxsrc/lib/ward-output.tstests/answer-lifecycle.test.tstests/answer-render-policy.test.tstests/api-client-error.test.tstests/auth-request-lifecycle.test.tstests/clinical-dashboard-merge-artifacts.test.tstests/private-access-routes.test.tstests/private-search-scope.test.tstests/production-mockup-boundary.test.tstests/search-navigation-context.test.tstests/ui-accessibility.spec.tstests/ui-smoke.spec.tstests/upload-outcome.test.tstests/upload-size-contract.test.tstests/verify-pr-local.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96de116426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/DocumentViewer.tsx (2)
2332-2360: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the debounce callback after auth invalidation.
The timer at Line [2334] unconditionally sets
searchingDocument(true)and startsfetch. If the epoch is invalidated during the 220ms delay, the signal is already aborted; the request rejects, the handlers return, andfinallydoes not clear the flag. Reset the state during cleanup and guard the timer with bothsignal.abortedandisAuthEpochCurrent(...).Proposed fix
const timeout = window.setTimeout(() => { + if (controller.signal.aborted || !isAuthEpochCurrent(authRequest.epoch)) { + setSearchingDocument(false); + return; + } setSearchingDocument(true); fetch(...); }, 220); return () => { window.clearTimeout(timeout); controller.abort(); + setSearchingDocument(false); authRequest.release(); };🤖 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/DocumentViewer.tsx` around lines 2332 - 2360, Update the debounce timer around the document search request to first guard on both controller.signal.aborted and isAuthEpochCurrent(authRequest.epoch), returning without setting searchingDocument or starting fetch when invalidated. Ensure the associated cleanup resets searchingDocument during abort/auth invalidation so the loading state cannot remain stuck.
2059-2066: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBind the remaining viewer mutations to the auth lifecycle.
These hooks only protect document loading and source search. The
summarize()andreviewTableFact()paths beginning at Line [2389] and Line [2520] still use barefetchcalls and commit state after authentication changes. A request started under one session can publish stale private data into the next session; reuse an auth-bound controller/fetch and skip success, error, andfinallyupdates when aborted or stale.🤖 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/DocumentViewer.tsx` around lines 2059 - 2066, Update the summarize() and reviewTableFact() mutation paths to use the existing auth-bound request controller/fetch utilities from useAuthSession instead of bare fetch. Before committing success, error, or finally state, verify the request was not aborted and its auth epoch remains current; otherwise skip all updates, including for requests superseded by authentication changes.
🤖 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.
Outside diff comments:
In `@src/components/DocumentViewer.tsx`:
- Around line 2332-2360: Update the debounce timer around the document search
request to first guard on both controller.signal.aborted and
isAuthEpochCurrent(authRequest.epoch), returning without setting
searchingDocument or starting fetch when invalidated. Ensure the associated
cleanup resets searchingDocument during abort/auth invalidation so the loading
state cannot remain stuck.
- Around line 2059-2066: Update the summarize() and reviewTableFact() mutation
paths to use the existing auth-bound request controller/fetch utilities from
useAuthSession instead of bare fetch. Before committing success, error, or
finally state, verify the request was not aborted and its auth epoch remains
current; otherwise skip all updates, including for requests superseded by
authentication changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 281b35df-66b9-4e5f-bc5d-362b6f943fff
📒 Files selected for processing (3)
docs/site-map.mdsrc/components/DocumentViewer.tsxsrc/components/master-document-flow-mockups.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/site-map.md
|
@copilot resolve the merge conflicts in this pull request |
Merge conflicts resolved in commit
|
|
@codex fix all active comments and resolve and mark resolved please |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d781395541
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex please resolve all comments, logically fix them please and resolve |
|
Summary
Testing
|
Summary
verify:pr-local --dry-run --files/--extendedselection with testsWhy
The Domain 2 audit confirmed production fixture leakage, stale authenticated async commits, unsafe private-scope restoration, ambiguous cancellation/upload outcomes, and copy/export gaps. These changes fail closed on auth or scope loss while preserving server-enforced public/private document access.
Verification
git diff --check— passednpm run format:check— passednpm run check:type-scale— passednpm run lint— passednpm run typecheck— passednpm run build— passed; client bundle secret scan passednpm run eval:rag:offline— 36 fixture cases and 58 production-contract tests passednpm run test:e2e:critical— 8/8 Chromium tests passednpm run check:production-readiness:ciwith provider variables removed — READY with expected missing-provider warningsChecks not run / limitations
Risk and governance
This touches authentication, private document scope, uploads, answer generation, source rendering, and clinical UI. Private scope references remain opaque, owner-bound, session-only, expiring, and fail closed. Production search continues to rely on server-side access enforcement rather than client filtering.