Optimise and extend the document & image viewers#619
Conversation
…Image DocumentImage (document viewer) and SourceImage (answer/evidence surfaces) reimplemented the same signed-URL fetch + retry + 4:3 aspect frame + fade, and had drifted: SourceImage fetched eagerly on mount with no IntersectionObserver, so a long evidence gallery fired every signed-URL request and downloaded every full-resolution image up front. Extract the shared behaviour into clinical-dashboard/signed-image.tsx and make both call sites delegate to it. The gallery path now inherits the IntersectionObserver gate (rootMargin 640px), so images load only as they near the viewport. Behaviour, markup and the SourceImage export signature are preserved; the eslint-disable no-img-element directives and now-unused clearCachedSignedUrl import are dropped since the raw <img> moved to the primitive. Adds tests/signed-image.test.ts covering the deferred initial render (no <img> or fetch before intersection) and synchronous cache seeding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
The PdfCanvasViewer render effect re-rasterised the whole page on every 0.15 zoom step (its dep array included `zoom`), so rapid +/- input queued a pdf.js RenderTask per delta. holderWidth was already debounced; zoom was not. Split the immediate `zoom` from a debounced `renderZoom` (140ms) that alone drives the raster. During the settle window an interim CSS transform scales the last raster to the target zoom for instant visual feedback, resetting to 1 the moment the crisp raster paints. Fit mode is container-sized and never carries an interim scale. The existing RenderTask.cancel cleanup and scale clamping are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
…y Escape Three safe cleanups in DocumentViewer: - getInitialPdfViewerMode() read localStorage twice in back-to-back useState initializers; read it once and seed both derived states from the result. - IndexedTextPanel stays mounted in both a mobile <details> and a desktop copy, CSS-toggled, so every unrelated parent re-render (composer typing) re-rendered both. Its props are referentially stable across those renders (onSearchChange is a stable setState), so wrapping it in memo elides the wasted renders. - Escape now exits whichever fullscreen mode is active and keeps the in-app fullscreen state in sync with the browser, instead of only clearing the in-app fallback overlay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
…ard nav Move PdfCanvasViewer and NativePdfEmbed out of the 3k-line DocumentViewer monolith into src/components/document-viewer/pdf-canvas-viewer.tsx (behaviour preserved), then add the interactions a page-image viewer was missing: - Ctrl/⌘ + wheel (and trackpad pinch) zoom via a shared useViewerGestures hook, attached as a non-passive wheel listener so plain scroll is untouched. - Two-finger pinch-to-zoom and single-pointer drag-to-pan when zoomed; fit mode keeps native momentum scrolling (touch-action pan-y) and only switches to custom touch handling (touch-action none) once zoomed. - Rotate button (0/90/180/270) applied through the pdf.js viewport, so fit and canvas sizing follow the rotation automatically. - The scroll holder is now focusable (role=group, aria-label) with ArrowLeft/ Right page nav and +/-/0 zoom keys, guarded so child controls aren't hijacked. useViewerGestures is generic (zoom/pan via callbacks) so the image lightbox can reuse it. Removes the now-unused pdfjs type + icon imports from DocumentViewer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
Extracted images (document viewer source images and answer-side evidence diagrams/table crops) were static previews with no way to inspect them large. Add a fullscreen ImageLightbox built on the shared Sheet (focus trap, Escape, scroll-lock, focus return) and the shared useViewerGestures hook, so wheel/pinch zoom and drag-to-pan match the PDF canvas; zoom/pan/rotate are CSS transforms on the <img>. The lightbox zooms on any wheel (new wheelNeedsModifier option), resets on close, and gates pan to zoomed state. SignedImage gains an opt-in `zoomable` affordance that owns the lightbox in one place, so both DocumentImage and the gallery's SourceImage inherit it by passing `zoomable`. The signed-URL fetch is factored into a shared useSignedImageUrl hook used by SignedImage and the lightbox, removing the last of that duplication. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
The PDF signed URL has a 10-min TTL and pdf.js keeps a dead PDFDocumentProxy once it lapses, so a document left open past the TTL failed on the next page fetch with no recovery (image renderers self-heal via the cache skew; the pdf.js document did not). PdfCanvasViewer now detects load/render failures consistent with an expired URL (HTTP 400/401/403 / UnexpectedResponseException, not parse errors) and calls a new onUrlExpired callback — at most once per URL. DocumentViewer responds by clearing the cached signed URLs and bumping previewAttempt to re-run the fetch pipeline, minting a fresh URL that reloads the canvas. A per-document counter caps auto-refreshes at 2 so a persistently failing URL cannot loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
Non-PDF documents fell through to a single static "Source preview is available after a signed URL is generated" message even once a signed URL existed. Replace that with NonPdfSourcePreview, which branches on file_type: image/* documents render the source image inline (with an Open-full-image link for the browser's native zoom), text/* points to the already-extracted indexed text below, and other formats (DOCX/XLSX/…) get an honest Open/Download affordance. The no-signed-URL case keeps the original placeholder. Lives in its own file so DocumentViewer stays free of raw <img> elements. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR centralizes signed-image loading and retry behavior, adds zoomable image lightboxes and interactive PDF rendering, introduces non-PDF source previews, and updates ChangesViewer rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentViewer
participant PdfCanvasViewer
participant pdfjs_dist
DocumentViewer->>PdfCanvasViewer: provide signed PDF URL
PdfCanvasViewer->>pdfjs_dist: load and render PDF
pdfjs_dist-->>PdfCanvasViewer: return page or URL error
PdfCanvasViewer-->>DocumentViewer: report expired URL
DocumentViewer->>DocumentViewer: clear cache and retry preview generation
sequenceDiagram
participant SourceImage
participant SignedImage
participant ImageLightbox
SourceImage->>SignedImage: provide image endpoint
SignedImage->>SignedImage: defer loading until near viewport
SignedImage->>ImageLightbox: open loaded image
ImageLightbox->>ImageLightbox: zoom, pan, and rotate image
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
The Static PR checks CI job failed on `prettier --check` — three files in the viewer work weren't formatted to the repo's Prettier style. No behavioural change; formatting only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
![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: 06c4b80a30
ℹ️ 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 resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
…eload Addresses a Codex P2: signedUrlRefreshCountRef only reset on documentId change, so a PDF left open across more than two 10-min TTL expirations exhausted the budget and the third expiry silently skipped the refresh, dead-ending the preview even though a fresh URL could be minted. PdfCanvasViewer now reports a successful load via onLoadSuccess, and DocumentViewer resets the counter then. A genuine expiry recovers (load succeeds → budget restored), while a broken URL never loads, so it never resets and the cap still stops its loop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c4b80a30
ℹ️ 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/signed-image.test.ts (1)
22-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure/retry path.
Tests cover the deferred and cache-seeded happy paths but not
failed/retry/markFailed. SinceSignedImagenow backs bothDocumentImageandSourceImage, a regression in the failure/retry branch would affect both surfaces silently.As per path instructions, "When behavior changes, add or update the smallest relevant targeted test and run the narrowest relevant validation before broader suites."
🤖 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/signed-image.test.ts` around lines 22 - 71, Extend the SignedImage tests to cover the failure/retry flow, including the failed state, retry action, and markFailed behavior. Exercise the relevant exported symbols used by SignedImage and verify that retry reattempts loading and restores the expected image or loading state, preventing regressions for both DocumentImage and SourceImage consumers.Source: Path instructions
src/components/clinical-dashboard/image-lightbox.tsx (1)
58-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove the
setTranslateside effect out of thesetScaleupdater.Static analysis flags this: React may invoke updater functions more than once, so side effects (
setTranslate) insidesetScale's updater aren't guaranteed to run exactly once. Currently idempotent, but worth making pure to avoid surprises as this logic evolves.♻️ Proposed fix
- const zoomByFactor = useCallback((factor: number) => { - setScale((current) => { - const next = clampScale(current * factor); - if (next <= 1) setTranslate({ x: 0, y: 0 }); - return next; - }); - }, []); + const zoomByFactor = useCallback((factor: number) => { + setScale((current) => clampScale(current * factor)); + }, []); + + useEffect(() => { + if (scale <= 1) setTranslate({ x: 0, y: 0 }); + }, [scale]);🤖 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/clinical-dashboard/image-lightbox.tsx` around lines 58 - 64, Update zoomByFactor so the setScale updater remains pure: compute and return the clamped scale there without calling setTranslate. Move the reset-translation behavior to a separate effect or equivalent post-update logic keyed to the resulting scale, preserving the existing behavior when the scale is at or below 1.Source: Linters/SAST tools
🤖 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/document-viewer/non-pdf-source-preview.tsx`:
- Around line 47-63: Update the image preview branch in the non-PDF source
preview component to handle image load failures with an onError-driven failure
state. Replace the broken inline image with a clear recovery affordance
consistent with the existing SignedImage/PdfCanvasViewer behavior, allowing the
user to retry or otherwise recover from an expired or failed signed URL while
preserving the current successful image and “Open full image” paths.
In `@src/components/DocumentViewer.tsx`:
- Around line 2001-2014: The signed-URL expiry handler currently triggers the
full preview retry flow through setPreviewAttempt, causing unnecessary
document-detail reloads and viewer flicker. Extract the signed-URL-only fetch
logic from the existing detail-fetch effect into a reusable function, then call
it from handleSignedUrlExpired after clearing both cached URL entries; preserve
loadingDocument, document detail, and the active PdfCanvasViewer state while
retaining the existing refresh-attempt limit.
---
Nitpick comments:
In `@src/components/clinical-dashboard/image-lightbox.tsx`:
- Around line 58-64: Update zoomByFactor so the setScale updater remains pure:
compute and return the clamped scale there without calling setTranslate. Move
the reset-translation behavior to a separate effect or equivalent post-update
logic keyed to the resulting scale, preserving the existing behavior when the
scale is at or below 1.
In `@tests/signed-image.test.ts`:
- Around line 22-71: Extend the SignedImage tests to cover the failure/retry
flow, including the failed state, retry action, and markFailed behavior.
Exercise the relevant exported symbols used by SignedImage and verify that retry
reattempts loading and restores the expected image or loading state, preventing
regressions for both DocumentImage and SourceImage consumers.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 21b78458-0967-47d1-8ee4-f45dbec29980
📒 Files selected for processing (9)
src/components/DocumentViewer.tsxsrc/components/clinical-dashboard/answer-content.tsxsrc/components/clinical-dashboard/image-lightbox.tsxsrc/components/clinical-dashboard/signed-image.tsxsrc/components/clinical-dashboard/use-signed-image-url.tssrc/components/document-viewer/non-pdf-source-preview.tsxsrc/components/document-viewer/pdf-canvas-viewer.tsxsrc/components/document-viewer/use-viewer-gestures.tstests/signed-image.test.ts
Addresses two CodeRabbit findings: - non-pdf-source-preview: the image/* branch rendered a bare <img> with no onError, so an expired/broken signed URL showed a silently broken image. Add an InlineImagePreview with a failed state that surfaces the existing Open/Download recovery affordance; keyed by signedUrl so a fresh URL resets it. - image-lightbox: zoomByFactor called setTranslate inside the setScale updater (which React may double-invoke). Keep the updater pure and move the pan-reset to the handler body (not an effect — the repo bans set-state-in-effect), reading the projected scale from scaleRef. Deferred (follow-up): the signed-URL-only refresh refactor for expired PDFs, which would require extracting the fetch from the allSettled auth/abort pipeline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This reverts commit 78fe885. The CodeRabbit autofix implemented the signed-URL-only PDF refresh — the architectural "heavy lift" deliberately deferred for this PR (extracting the fetch out of the allSettled auth/epoch/abort pipeline). Beyond being out of the agreed scope, the generated change duplicated ~90 lines of the main data effect's fetch logic instead of extracting a shared function, left its AbortController unaborted (set-state-after-unmount risk), and failed the Static PR checks CI job. Reverting restores the reviewed, green state; the refresh refactor will be done properly as a separate follow-up if wanted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
A review-and-improve pass across the document viewer and every image-viewing surface, split into small reviewable commits (behaviour, design, optimisation):
DocumentImage(document viewer) andSourceImage(answer/evidence surfaces) reimplemented the same signed-URL fetch + retry + 4:3 frame + fade, and had drifted (SourceImagefetched eagerly on mount). Both now delegate to a sharedSignedImage, so the evidence gallery inheritsIntersectionObserverlazy-loading and stops firing every signed-URL request / downloading every full-res image up front.renderZoomdrives the raster while an interim CSS transform gives instant feedback, so+/-, wheel, and pinch no longer thrash pdf.jsRenderTasks.PdfCanvasViewerout of the 3k-line monolith and add the interactions a page viewer was missing: Ctrl/⌘-wheel + pinch zoom, drag-to-pan when zoomed, rotate (via the pdf.js viewport), and a focusable canvas with ArrowLeft/Right page nav and+/-/0zoom keys — all through a shared, reusableuseViewerGestureshook.Sheet(focus trap, Escape, scroll-lock, focus return) and the same gesture hook.file_type(image inline, text → indexed text, other → Open/Download).IndexedTextPanel, and make Escape consistently exit fullscreen.Design/tokens: all new UI reuses
ui-primitives(toolbarButton,cn, etc.), theSheetoverlay, the@themecolour/motion/tap tokens, and honoursmotion-reduce/forced-colors.Verification
npm run verify:pr-localrun individually:npm run lint(full repo,--max-warnings 0) ✅,npm run typecheck(0 errors) ✅,npm run test(vitest: 2066 passed / 3 skipped) ✅. Addedtests/signed-image.test.ts.npm run verify:pr-local/npm run verify:uias single commands — not runnable as-is in this sandbox (@sentry/nodeand@axe-core/playwrightweren't installed in the base image; typecheck/UI gates depend on them). Instead I drove the viewer end-to-end in the pre-installed Chromium (results in Notes). Please runverify:pr-local+verify:uiin CI.This change is presentation-only: it does not touch retrieval, ranking, selection, chunking, answer generation, ingestion, or document-access policy, so the eval/readiness gates below don't apply.
Clinical Governance Preflight
Change touches source rendering only (how already-authorized signed-URL content is displayed); no ingestion, answer generation, ranking, document access, privacy, or clinical-output logic changed.
Clinical KB Database(sjrfecxgysukkwxsowpy) — no Supabase changesNotes
Manual browser verification (pre-installed Chromium, demo mode):
synthetic-clozapine…, a real PDF): PDF canvas rasterised; rotate control present; keyboard focus + ArrowRight advanced page 1 → 2.synthetic-risk-flowdiagram): click-to-expand opened the fullscreen overlay; zoom + rotate controls present; the zoom control changed scale 100% → 156%; Escape closed it. Screenshots captured for both.Follow-ups intentionally out of scope (flagged in the plan): no
next/image/Supabase image-transform adoption (kept raw<img>for private signed URLs); the full monolith decomposition and splitting the core data effect so?page=changes don't refetch everything are left as separate reviewed changes.🤖 Generated with Claude Code
https://claude.ai/code/session_01DqMTk5fDNgi5GwEK5peRmj
Generated by Claude Code
Summary by CodeRabbit