feat(viewer): Phase 2b unify whole-document images with ImageLightbox - #1704
Conversation
Extend ImageLightbox with parent-owned URL mode that never touches the signed-URL LRU, and open NonPdf image previews through the shared immersive Sheet while keeping Open/Download as secondary actions.
📝 WalkthroughWalkthroughChangesImage lightbox source handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InlineImagePreview
participant ImageLightbox
participant DirectImage
InlineImagePreview->>ImageLightbox: Open with direct URL
ImageLightbox->>DirectImage: Render image source
DirectImage-->>ImageLightbox: Report load success or failure
ImageLightbox-->>InlineImagePreview: Close and restore focus
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/document-viewer/non-pdf-source-preview.tsx (1)
158-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the image height into a theme token.
max-h-[min(70vh,36rem)]introduces a hardcoded design value. Define a named Tailwind 4@themetoken insrc/app/globals.cssand consume that token here.As per coding guidelines, “Use Tailwind 4
@themetokens in src/app/globals.css … rather than introducing hardcoded design values.”🤖 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/document-viewer/non-pdf-source-preview.tsx` around lines 158 - 166, Replace the hardcoded max-height value on the image in the non-PDF preview with a named Tailwind 4 theme token. Define the token in the `@theme` section of globals.css, then reference that token from the image class while preserving the existing sizing behavior.Source: Coding guidelines
🤖 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/image-lightbox.tsx`:
- Around line 98-105: Update the lightbox component’s open-state effect to
detect a transition from open to closed and reset directFailed when that
transition occurs. Keep handleClose’s existing reset behavior and avoid clearing
the failure state on initial render or while the lightbox remains open.
In `@src/components/document-viewer/non-pdf-source-preview.tsx`:
- Around line 168-181: Update the lightbox opening logic shared by the image
overlay button and the “View immersive” button to record the invoking element in
a shared ref before setting lightboxOpen. Pass that shared ref as returnFocusRef
so closing the lightbox restores focus to whichever control opened it, including
the “View immersive” button.
---
Nitpick comments:
In `@src/components/document-viewer/non-pdf-source-preview.tsx`:
- Around line 158-166: Replace the hardcoded max-height value on the image in
the non-PDF preview with a named Tailwind 4 theme token. Define the token in the
`@theme` section of globals.css, then reference that token from the image class
while preserving the existing sizing behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59777cce-3571-4a36-9379-debcf353442d
📒 Files selected for processing (4)
src/components/clinical-dashboard/image-lightbox.tsxsrc/components/document-viewer/non-pdf-source-preview.tsxtests/document-viewer-non-pdf-preview.dom.test.tsxtests/image-lightbox-url-mode.dom.test.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Unifies whole-document image previews with the shared immersive lightbox while preserving parent-owned signed-URL handling.
Changes:
- Adds direct-URL mode to
ImageLightbox. - Adds lightbox controls to non-PDF image previews.
- Adds URL-mode, retry, and auth-clear tests.
Review finding (P2):non-pdf-source-preview.tsx:210 always restores focus to the image-overlay button. If opened via “View immersive,” closing moves focus to a different control. Track the activated opener and test focus restoration for both buttons.
Static review only; reported tests were not rerun.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/components/clinical-dashboard/image-lightbox.tsx | Adds parent-owned URL mode. |
src/components/document-viewer/non-pdf-source-preview.tsx | Integrates immersive image viewing. |
tests/image-lightbox-url-mode.dom.test.tsx | Tests URL isolation, clearing, and retry. |
tests/document-viewer-non-pdf-preview.dom.test.tsx | Tests preview integration and URL clearing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
ImageLightboxas rail crops.ImageLightboxaccepts eitherendpoint(existing signed-URL fetch) or parent-ownedurl(document signed URL). URL mode never reads or writes the signed-URL LRU.NonPdfSourcePreviewprimary action opens the lightbox; Open/Download remain secondary. Card-like preview chrome flattened into the frame surround.Verification
tests/image-lightbox-url-mode.dom.test.tsx,document-viewer-non-pdf-preview.dom.test.tsx,signed-image.dom.test.tsx,auth-signed-url-cache.dom.test.tsx,use-viewer-gestures.dom.test.tsx— 25 passednpm run verify:pr-local— exit 0Test Files 522 passed (522)/Tests 5543 passed | 1 skipped (5544)Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).verify:pr-localattempt hit an unrelateddesign-system-adoption30s timeout under suite load; that test passed alone and the full gate passed on retry.npm run ensure+ photo QA before widening.Risk and rollout
useSignedImageUrlfetch path whenurlis provided, plus blank/unmount tests.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
ui, privacy. Does not start 2c (filmstrip), 2d, or 2e.Summary by CodeRabbit
New Features
Bug Fixes
Tests