feat(viewer): Phase 2c rail filmstrip syncs PDF page - #1707
Conversation
Add a compact page-linked filmstrip and clickable DocumentImage page badges that call navigateToPage, highlighting the active page without remounting PdfCanvasViewer or changing signed-image deferral.
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in:58 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe document viewer rail now supports page selection through a horizontal image filmstrip and page badges. Active-page state flows through clinical and audit images. Tests verify navigation, accessibility state, history updates, and PDF viewer mount stability. ChangesDocument page navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DocumentImageFilmstrip
participant DocumentViewerRail
participant navigateToPage
User->>DocumentImageFilmstrip: Click page button
DocumentImageFilmstrip->>DocumentViewerRail: Invoke onSelectPage(page)
DocumentViewerRail->>navigateToPage: Select page
navigateToPage-->>DocumentViewerRail: Update active page
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
Implements Phase 2c of the document viewer rail chrome by adding a compact, page-linked filmstrip for rail figures and wiring both the filmstrip and figure page badges to navigateToPage (pushState-based) so the PDF viewer does not remount on page jumps.
Changes:
- Add
DocumentImageFilmstriptoolbar and render it above the figure cards in the rail. - Extend
DocumentImageandDocumentViewerRailto acceptactivePage+onSelectPageand highlight the active PDF page. - Add DOM/performance guard tests covering the filmstrip/badges and the “no remount” contract.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/document-image-filmstrip.dom.test.tsx | Adds DOM tests for filmstrip clicks + DocumentImage page badge behavior. |
| tests/document-detail-performance.test.ts | Adds a string-based “contract/perf” test asserting navigateToPage wiring and stable canvas key. |
| src/components/DocumentViewer.tsx | Passes activePage / navigateToPage into the rail. |
| src/components/document-viewer/source-panels.tsx | Updates DocumentImage to support page badge navigation and active-page highlighting. |
| src/components/document-viewer/document-rail-panels.tsx | Renders the filmstrip + wires page-selection props through the rail. |
| src/components/document-viewer/document-image-filmstrip.tsx | New compact filmstrip toolbar component. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
CI triageCI failed on this PR. Automated classification of the 3 failed job(s):
Compared with main CI run #8945 (cancelled). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/document-viewer/document-image-filmstrip.tsx (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a theme token for the filmstrip label width.
Add
--container-filmstrip-label: 9remunder@theme, then replacemax-w-[9rem]withmax-w-filmstrip-label.🤖 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/document-image-filmstrip.tsx` at line 61, Add the --container-filmstrip-label: 9rem token under the `@theme` configuration, then update the label span in the document filmstrip to use max-w-filmstrip-label instead of the hardcoded max-w-[9rem] class.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 `@tests/document-image-filmstrip.dom.test.tsx`:
- Around line 54-55: Update the active-page assertion for page5 in the
DocumentImageFilmstrip test to expect the emitted aria-current value "page"
instead of "true", matching DocumentImageFilmstrip’s accessibility behavior.
---
Nitpick comments:
In `@src/components/document-viewer/document-image-filmstrip.tsx`:
- Line 61: Add the --container-filmstrip-label: 9rem token under the `@theme`
configuration, then update the label span in the document filmstrip to use
max-w-filmstrip-label instead of the hardcoded max-w-[9rem] class.
🪄 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: 2268d320-460b-45e4-8ad5-0be0362ada89
📒 Files selected for processing (6)
src/components/DocumentViewer.tsxsrc/components/document-viewer/document-image-filmstrip.tsxsrc/components/document-viewer/document-rail-panels.tsxsrc/components/document-viewer/source-panels.tsxtests/document-detail-performance.test.tstests/document-image-filmstrip.dom.test.tsx
Uh oh!
There was an error while loading. Please reload this page.
- Update document-image-filmstrip test to expect aria-current="page" (matches the component fix already applied for the a11y review finding); the test still asserted the old "true" value and was failing CI unit coverage. - Fix image metadata line in source-panels.tsx: filter+join the optional parts instead of prefixing each with " · ", so a missing image_type no longer leaves a leading separator (Sentry finding).
Uh oh!
There was an error while loading. Please reload this page.
Summary
DocumentImagebadges callnavigateToPage(history.pushState + local route state) — norouter.push, no PdfCanvasViewer remount (keystays documentId + viewer mode only).data-active-page,aria-current).Verification
document-image-filmstrip.dom.test.tsx,document-detail-performance.test.ts,document-section-nav.dom.test.tsx,document-viewer-shell.dom.test.tsx,document-section-index.test.ts— 34 passednpm run verify:pr-local— exit 0Test Files 522 passed (522)/Tests 5541 passed | 1 skipped (5542)Offline RAG fixture and production-contract checks passed.source-imagessection gating unchanged insection-index.ts.Risk and rollout
keyand navigateToPage pushState path.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
ui. Independent of open 2b (different files). Does not start 2d/2e.Summary by CodeRabbit
New Features
Bug Fixes
Tests