fix: resolve recent PR review findings#551
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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
WalkthroughThis PR strengthens workflow validation and readiness checks, hardens authentication and answer caching, updates upload and navigation behavior, improves CLI resolution, and refreshes privacy, review, and staging documentation with corresponding tests. ChangesWorkflow and developer tooling
Runtime request, cache, and answer handling
UI navigation and upload flows
Review and operational documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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: f33fbc6cc5
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/components/ClinicalDashboard.tsx (1)
1456-1497: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing error handling in
loadMoreDocumentsforauthBoundFetchabort/failure.Unlike every other
authBoundFetchconsumer touched in this PR (retryJob,reindexDocument,mutateDocumentLabel,bulkReindexSelected,bulkUpdateMetadata),loadMoreDocumentshas nocatchblock.authBoundFetchnow deliberately throwsDOMException("Stale authentication epoch", "AbortError")when the epoch goes stale after fetch resolves, andresponse.json()here has no.catch()fallback either. Since this is invoked as a fire-and-forget UI callback (onLoadMoreDocuments={loadMoreDocuments}), any such throw becomes an unhandled promise rejection instead of being silently discarded like the sibling handlers.🔧 Proposed fix
const loadMoreDocuments = useCallback(async () => { if (!documentsPagination?.hasMore || loadingMoreDocuments || !canUsePrivateApis) { return; } setLoadingMoreDocuments(true); try { const protectedHeaders = clientDemoMode ? undefined : authorizationHeader; const { response, requestEpoch } = await authBoundFetch( `/api/documents?limit=${documentPageSize}&offset=${documentsPagination.nextOffset}`, { headers: protectedHeaders }, ); if (response.status === 401) { markSessionExpired(); return; } if (!response.ok) { if (!isAuthEpochCurrent(requestEpoch)) return; setApiUnavailable(true); return; } - const payload = await response.json(); + const payload = await response.json().catch(() => null); + if (!payload) return; if (!isAuthEpochCurrent(requestEpoch)) return; const nextDocuments = (payload.documents ?? []) as ClinicalDocument[]; setDocuments((current) => { const seen = new Set(current.map((document) => document.id)); return [...current, ...nextDocuments.filter((document) => !seen.has(document.id))]; }); setDocumentsPagination(payload.pagination ?? null); + } catch (error) { + if (!isAbortError(error)) setApiUnavailable(true); } finally { setLoadingMoreDocuments(false); } }, [...]);🤖 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/ClinicalDashboard.tsx` around lines 1456 - 1497, Add catch handling to loadMoreDocuments for authBoundFetch, response.json(), and other asynchronous failures so fire-and-forget invocation does not produce unhandled promise rejections. Silently discard stale-authentication AbortError failures, matching the behavior of sibling handlers such as retryJob and reindexDocument, while preserving the existing loadingMoreDocuments cleanup in finally.
🧹 Nitpick comments (2)
scripts/check-github-action-pins.mjs (1)
75-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded version duplicates
expectedSupabaseCliVersion.Line 75 embeds the literal
2\.108\.0in the regex instead of building it fromexpectedSupabaseCliVersion(declared at Line 38). Bumping the constant without updating this regex (or a typo in either place) desynchronizes the pin check from its own source of truth.♻️ Proposed fix to derive the regex from the constant
-if (!/^ SUPABASE_CLI_VERSION: 2\.108\.0$/m.test(ciWorkflow)) { +const pinnedVersionPattern = expectedSupabaseCliVersion.replace(/\./g, "\\."); +if (!new RegExp(`^ SUPABASE_CLI_VERSION: ${pinnedVersionPattern}$`, "m").test(ciWorkflow)) { failures.push(`ci.yml: global SUPABASE_CLI_VERSION must remain pinned to ${expectedSupabaseCliVersion}.`); }🤖 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 75 - 77, Update the SUPABASE_CLI_VERSION validation in the ci workflow check to derive the regex from expectedSupabaseCliVersion instead of hardcoding 2.108.0. Escape or otherwise safely interpolate the constant so version bumps keep the check synchronized, while preserving the existing failure message and validation behavior.src/lib/supabase/client.tsx (1)
289-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: tighten
authSessionFingerprint'sstatusparam type.
authSessionFingerprint(status: string, ...)accepts a barestring; usingAuthStatus(imported from this file) instead would let the type checker catch drift if the status union changes.🤖 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 289, Update the authSessionFingerprint function’s status parameter from string to the imported AuthStatus type, while preserving its existing behavior and call sites.
🤖 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/ClinicalDashboard.tsx`:
- Around line 1456-1497: Add catch handling to loadMoreDocuments for
authBoundFetch, response.json(), and other asynchronous failures so
fire-and-forget invocation does not produce unhandled promise rejections.
Silently discard stale-authentication AbortError failures, matching the behavior
of sibling handlers such as retryJob and reindexDocument, while preserving the
existing loadingMoreDocuments cleanup in finally.
---
Nitpick comments:
In `@scripts/check-github-action-pins.mjs`:
- Around line 75-77: Update the SUPABASE_CLI_VERSION validation in the ci
workflow check to derive the regex from expectedSupabaseCliVersion instead of
hardcoding 2.108.0. Escape or otherwise safely interpolate the constant so
version bumps keep the check synchronized, while preserving the existing failure
message and validation behavior.
In `@src/lib/supabase/client.tsx`:
- Line 289: Update the authSessionFingerprint function’s status parameter from
string to the imported AuthStatus type, while preserving its existing behavior
and call sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 026c2f3d-7001-47a8-add9-6c0372f45c79
📒 Files selected for processing (33)
.github/workflows/sast.ymldocs/branch-review-ledger.mddocs/privacy-impact-assessment.mddocs/staging-setup.mdscripts/check-github-action-pins.mjsscripts/generate-drift-manifest.tsscripts/resolve-tsx-cli.mjsscripts/run-tsx.mjsscripts/yaml-contract.mjssrc/components/ClinicalDashboard.tsxsrc/components/clinical-dashboard/DocumentManagerPanel.tsxsrc/components/differentials/differentials-home-page.tsxsrc/components/tools-page-mockups/tools-page-mockup-page.tsxsrc/lib/answer-render-policy.tssrc/lib/auth-request-lifecycle.tssrc/lib/observability/answer-slo.tssrc/lib/private-search-scope.tssrc/lib/rag-cache.tssrc/lib/rag.tssrc/lib/supabase/client.tsxsrc/proxy.tstests/anonymous-answer-cache-policy.test.tstests/answer-lifecycle.test.tstests/answer-render-policy.test.tstests/answer-slo.test.tstests/auth-request-lifecycle.test.tstests/private-search-scope.test.tstests/production-mockup-boundary.test.tstests/proxy.test.tstests/rag-answer-fallback.test.tstests/resolve-tsx-cli.test.tstests/upload-outcome.test.tstests/yaml-contract.test.ts
💤 Files with no reviewable changes (1)
- src/proxy.ts
|
Addressed the CodeRabbit follow-ups in 4b86105: load-more now absorbs expected auth aborts and surfaces genuine failures, and the Actions policy check derives its version regex from the single pinned Supabase CLI constant. The optional auth status typing suggestion was left unchanged because the helper intentionally stays independent of the provider-local AuthStatus type. Focused tests, Actions policy validation, typecheck, formatting, and lint pass. |
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 `@tests/answer-slo.test.ts`:
- Around line 16-18: Update the test double’s is() method in the answer SLO
tests to record both the column and comparison value, then assert { column:
"metadata->>event_type", value: null } for each of the three queries. Apply the
same change to the additional is() occurrence referenced in the comment while
preserving the existing query assertions.
🪄 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: 8dcfe61d-01f4-495b-8676-4154861e25fb
📒 Files selected for processing (2)
src/lib/observability/answer-slo.tstests/answer-slo.test.ts
1 participant
Summary
tsxresolution in sibling worktrees; correct privacy and staging documentationVerification
npm run verify:pr-local: passed (1,762 tests passed, 1 skipped; production build and client-bundle secret scan passed; offline RAG 60/60)npm run verify:ui: the full Chromium runner hung without reporting a failed test. Rannpm run test:e2e:critical: passed (8/8); hosted advisory UI remains enabled.npm run verify:release: it includes broader provider/governance release gates; required PR checks are running hosted.npm run eval:retrieval:quality: not applicable because no retrieval ranking, selection, chunking, or scoring behavior changed.npm run eval:rag -- --limit 15ornpm run eval:quality -- --rag-only: not applicable because no generation prompt or clinical answer post-processing changed.npm run check:production-readiness: passed as READY with the configured Clinical KB Database project.npm run check:deployment-readiness: not applicable because no deployment startup or hosting behavior changed.Additional evidence:
npm run test -- tests/answer-slo.test.ts tests/production-mockup-boundary.test.ts tests/auth-request-lifecycle.test.ts: passed (10/10).npm run check:github-actions: passed.npm run typecheck: passed.node --env-file=C:\Dev\Apps\Database\.env.local scripts/run-tsx.mjs scripts/check-drift.ts: completed with five expected differences, all explained by the unapplied migrations merged in fix: remediate repository review and reconcile Supabase drift #540; no unexplained drift and no live mutation.git diff --checkand pre-commit staged diff checks: passed.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
main, duplicate migration/drift work was dropped, and only unique fixes remain.