feat(viewer): Phase 2a DocumentFrame owns PDF zoom/fit chrome - #1687
Conversation
Phase 2a: wire shared viewer zoom contract into DocumentFrame controls, control PdfCanvasViewer fit/zoom from the parent, and demote duplicate pdf-toolbar zoom/fit chrome while keeping page/rotate/fullscreen.
Adjust fit/zoom/viewing-aid during render when documentId changes so the static guard's react-hooks/set-state-in-effect rule stays green.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 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)
📝 WalkthroughWalkthrough
ChangesPDF viewer controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentViewer
participant DocumentFrame
participant PdfCanvasViewer
DocumentViewer->>DocumentFrame: pass PDF controls
DocumentFrame->>PdfCanvasViewer: pass controlled fit-width and zoom
PdfCanvasViewer->>DocumentViewer: report fit-width and zoom changes
DocumentViewer->>DocumentFrame: update control state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
@cursoragent please resolve all comments and issues for this PR |
|
Resolved the only actionable review thread on #1687. Sentry HIGH (stale Other PR comments: Informational only (Supabase no-op, CodeRabbit rate limit, Codex review quota) — nothing to change. Evidence:   |
Pinch/wheel use functional setZoom updaters; resolving against a closed-over zoom dropped intermediate steps. Compose via zoomRef (Frame-owned) and React state updaters (internal), plus resolveViewerZoomUpdate tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/components/document-viewer/pdf-canvas-viewer.tsx`:
- Around line 106-107: Move the fitWidthRef and zoomRef assignments out of the
render body and into a commit-time layout effect so discarded renders cannot
update refs; retain immediate ref writes in the setFitWidth and setZoom event
handlers before propagating changes to DocumentViewer.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44cd04b4-9efb-46e0-a982-07dba58cfc19
📒 Files selected for processing (9)
src/components/DocumentViewer.tsxsrc/components/document-viewer/pdf-canvas-viewer.tsxsrc/components/document-viewer/viewer-zoom.tssrc/components/ui/document-frame.tsxtests/document-frame-contract.test.tstests/document-frame.dom.test.tsxtests/property-document-frame.dom.test.tsxtests/ui-smoke.spec.tstests/viewer-zoom.test.ts
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Pull request overview
Implements Phase 2a of the “unified viewer chrome” plan by moving PDF zoom/fit/viewing-aid chrome ownership to DocumentFrame for the canvas-based PDF path, while keeping PdfCanvasViewer responsible for page navigation, rotation, and fullscreen.
Changes:
- Introduces a shared zoom contract (
viewer-zoom.ts) and updates bothDocumentFrameandPdfCanvasViewerto use the same clamp/step/defaults. - Makes
PdfCanvasVieweroptionally controlled forfitWidth/zoom, and demotes its duplicate toolbar zoom/fit controls whenDocumentFrameowns the chrome. - Updates contract + DOM + Playwright smoke tests to enforce the new ownership and updated accessible labels.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/viewer-zoom.test.ts | Adds unit coverage for shared clamp/functional zoom update composition. |
| tests/ui-smoke.spec.ts | Updates fullscreen button accessible name expectation for the PDF canvas toolbar. |
| tests/property-document-frame.dom.test.tsx | Adjusts minimum zoom expectation to align with the shared zoom contract (55%). |
| tests/document-frame.dom.test.tsx | Updates expected zoom step deltas (±0.15) for frame-owned controls. |
| tests/document-frame-contract.test.ts | Tightens contract to require controls={pdfFrameControls} and shared zoom imports/behavior. |
| src/components/ui/document-frame.tsx | Uses shared viewer zoom constants as default min/max/step for frame controls. |
| src/components/DocumentViewer.tsx | Adds controlled PDF viewing state (fit/zoom/viewing-aid) and wires it into DocumentFrame + PdfCanvasViewer. |
| src/components/document-viewer/viewer-zoom.ts | Defines shared viewer zoom constants plus clamp/functional-update resolver. |
| src/components/document-viewer/pdf-canvas-viewer.tsx | Accepts controlled fit/zoom, composes functional zoom updates safely, and demotes duplicate zoom/fit toolbar when frame-owned. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Prettier reported style issues on the Phase 2a frame chrome file, failing format:changed in CI.
Autopilot + Bugbot — PR #1687Head: Snapshot
Tip history this pass
Review
Actions
BlockersNone for required CI. Human merge/approval still yours — not merged. |
Summary
docs/plans/document-viewer-phase2-unified-chrome.md): DocumentFrame owns zoom / fit-width / viewing-aid for canvas PDF.viewer-zoom.ts(0.55–4, step0.15) used by Frame defaults andPdfCanvasViewer.PdfCanvasVieweraccepts controlled fit/zoom; when Frame owns chrome, demotes duplicate toolbar zoom/fit while keeping page nav, rotate, and fullscreen.controls={pdfFrameControls}for the canvas PDF path.Verification
tests/document-frame-contract.test.ts,document-frame.dom.test.tsx,property-document-frame.dom.test.tsx,document-viewer-shell.dom.test.tsx,document-viewer-pdf-reader-lazy.test.ts,client-performance-boundaries.test.ts,document-detail-performance.test.ts— 7 passed (32 tests)npm run verify:pr-local— exit 0Test Files 520 passed (520)/Tests 5532 passed | 1 skipped (5533)Offline RAG fixture and manifest validation passed (36 golden cases, 23 suites).npm run ensure+ manual PDF smoke before widening toverify:ui.Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
ui; treated as UI + clinical document surface for governance.Summary by CodeRabbit
New Features
Bug Fixes