Signed-URL-only refresh for expired PDFs (shared fetch, abortable)#643
Conversation
… abortable) Follow-up to #619. An expired PDF signed URL previously recovered by bumping previewAttempt, which re-ran the whole detail-fetch effect — resetting loadingDocument/signedUrl, hiding the canvas behind the loading screen, and re-fetching document detail (pages/images/chunks/tableFacts) plus both signed URLs — just to re-mint one URL on an otherwise-unchanged document. Extract the signed-URL fetch/apply into shared units used by both paths, so the logic lives in exactly one place (not duplicated): - requestSignedUrlPayload / fetchSignedUrlPair: one signed-URL GET and the preview+download pair (cache-first when allowed), at module scope. - applySignedUrlResults: applies the settled results to state (cache write, previewError, download fallback) — used by the initial load and the refresh. handleSignedUrlExpired now calls refreshSignedUrls, which re-issues only the signed URLs in place (no detail refetch, no loadingDocument reset, no flicker), guarded by the auth epoch and its own AbortController — aborted on the next refresh and on unmount. The refresh-attempt cap and reset-on-successful-load are retained. Verified: lint, typecheck (0 errors), format, full vitest (2265 passing), and a Chromium drive of the initial load (detail + PDF canvas + download link, no preview error). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesSigned URL lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DocumentViewer
participant fetchSignedUrlPair
participant SignedUrlAPI
participant URLCache
DocumentViewer->>fetchSignedUrlPair: Request preview and download URLs
fetchSignedUrlPair->>URLCache: Read cached URLs when enabled
fetchSignedUrlPair->>SignedUrlAPI: Fetch missing signed URLs
SignedUrlAPI-->>fetchSignedUrlPair: Return settled URL results
fetchSignedUrlPair-->>DocumentViewer: Return preview and download results
DocumentViewer->>URLCache: Cache fulfilled URLs
DocumentViewer->>DocumentViewer: Apply URL state and errors
DocumentViewer->>fetchSignedUrlPair: Refresh URLs after expiry
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Follow-up to #619 (now merged), addressing the one CodeRabbit finding deliberately deferred there: when a PDF's signed URL expires mid-session, recovery reused
setPreviewAttempt, which re-ran the whole detail-fetch effect — resettingloadingDocument/signedUrl, hiding the canvas behind the loading screen, and re-fetching document detail (pages/images/chunks/tableFacts) plus both signed URLs — all to re-mint one URL on an otherwise-unchanged document (jarring reload/flicker + wasted payload).This does it properly (not the way CodeRabbit's reverted autofix did — that duplicated ~90 lines and left an un-aborted controller):
requestSignedUrlPayload(a single signed-URL GET: JSON parse, 401 →markSessionExpired, throw on failure) andfetchSignedUrlPair(the preview + download pair, cache-first when allowed) at module scope, andapplySignedUrlResults(applies the settled results to state — cache write,previewError, download fallback). The initial-load effect and the expiry refresh now take the exact same code path.handleSignedUrlExpiredcallsrefreshSignedUrls, which re-issues only the signed URLs — no detail refetch, noloadingDocumentreset, no flicker; the fresh URL flows toPdfCanvasViewerand reloads the page.refreshSignedUrlsowns anAbortControllerthat is aborted on the next refresh and on unmount, and is guarded by the auth epoch (isAuthEpochCurrent) so a stale response can't write state. The refresh-attempt cap (≤2) and reset-on-successful-load from Optimise and extend the document & image viewers #619 are retained.Verification
npm run verify:pr-localrun individually:npm run lint(repo,--max-warnings 0) ✅,npm run typecheck(0 errors) ✅,npm run test(vitest: 2265 passed / 3 skipped) ✅,prettier --check✅.fetchSignedUrlPairand applied viaapplySignedUrlResults), the Download link is present, and no preview error surfaces.npm run verify:pr-local/npm run verify:uias single commands — CI runs these. (The expiry-refresh path itself can't be forced in a quick drive — a 10-min TTL lapse — but it reuses the exact shared fetch/apply units verified above, with the same epoch/abort guards as the initial-load effect.)Presentation/auth-plumbing only — no change to retrieval, ranking, ingestion, answer generation, or document-access policy.
Clinical Governance Preflight
Change touches source rendering only (how an already-authorized signed URL is re-issued and displayed); no ingestion, answer generation, ranking, document access, privacy, or clinical-output logic changed.
Clinical KB Database(sjrfecxgysukkwxsowpy) — no Supabase changesNotes
Depends on #619 (merged). Net diff is
src/components/DocumentViewer.tsxonly: the shared fetch/apply helpers, therefreshSignedUrlscallback (+ unmount-abort effect), and rewiringhandleSignedUrlExpired; the initial-load effect now consumes the same helpers so there is no duplicated signed-URL fetch logic.🤖 Generated with Claude Code
https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
Generated by Claude Code
Summary by CodeRabbit