fix(viewer): Phase 0 gesture INP, preview a11y, image decode - #1660
Conversation
Make PDF wheel listeners passive by default and attach non-passive only for unmodified wheel-zoom or active Ctrl/⌘ pinch (#214). Announce canvas/non-PDF preview failures with role=alert and LiveAnnouncer (#219). Add lightbox decoding=async and SignedImage priority for above-fold loads (partial #215). Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Close the order gap so the recommended execution queue no longer points at resolved viewer gesture/a11y work. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Avoid assigning read-only observer fields in the jsdom mock so typecheck stays green under the Phase 0 priority coverage. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
|
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: 12 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 (1)
📝 WalkthroughWalkthroughThe pull request updates image loading, document preview accessibility, wheel gesture listeners, related tests, smoke-test locators, and the outstanding-issues ledger. ChangesViewer updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant useViewerGestures
participant PassiveWheelListener
participant NonPassiveWheelListener
User->>PassiveWheelListener: Send ordinary wheel event
PassiveWheelListener->>useViewerGestures: Handle unmodified scrolling
User->>useViewerGestures: Send Ctrl/⌘ wheel or pinch event
useViewerGestures->>NonPassiveWheelListener: Promote listener for blocking zoom
NonPassiveWheelListener->>useViewerGestures: Prevent default and calculate zoom
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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}
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/clinical-dashboard/signed-image.tsx (1)
35-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate
shouldLoadwhenprioritychanges.A mounted
SignedImagedoes not start loading whenprioritychanges fromfalsetotrue. The current consumer derivespriorityfrom answer state, so this transition can occur during rerenders. Add a rerender test and make the load gate respond to the current prop, or document mount-only behavior.🤖 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/signed-image.tsx` around lines 35 - 61, The shouldLoad state in SignedImage is initialized from priority but does not react when that prop changes. Update the SignedImage load gate so a false-to-true priority transition sets shouldLoad to true, and add a rerender test in tests/signed-image.dom.test.tsx covering that transition.Source: MCP tools
🧹 Nitpick comments (5)
src/components/clinical-dashboard/signed-image.tsx (1)
166-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMap
priorityto the supported Next.js 16 image API.The repository uses Next.js 16.2.12. Next.js 16 deprecates the
next/imagepriorityprop in favor ofpreload; this line still passes the deprecated prop. KeepSignedImage.priorityas the public contract if needed, but map it to the supported loading API that fits a client-resolved signed URL. Verify the installed Next.js documentation before merge. (nextjs.org)🤖 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/signed-image.tsx` at line 166, Update the Image props in SignedImage to map the public SignedImage.priority value to Next.js 16’s supported preload API, removing the deprecated priority prop. Preserve the existing behavior for client-resolved signed URLs and keep the public priority contract unchanged.Source: MCP tools
tests/signed-image.dom.test.tsx (2)
90-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
priorityprop update path.The new test covers only
priority={true}on the initial render. It does not catch the false-to-true update failure inSignedImageat Lines 35-61. Start withoutpriority, rerender withpriority, and assert that no observer is used and loading starts immediately.🤖 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.dom.test.tsx` at line 90, Add a test case covering the false-to-true priority update path in SignedImage: render without priority, rerender with priority enabled, then assert loading begins immediately and IntersectionObserver is not used. Keep the existing initial-render priority test unchanged.
111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the priority test independent of the signed-URL cache.
SignedImageseeds its initial load state fromgetCachedSignedUrl(endpoint)as well aspriority. If an earlier test has cached the sharedENDPOINT,observecan remain unused even if the priority wiring regresses. Use a unique endpoint or reset the signed-URL cache in test setup.🤖 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.dom.test.tsx` around lines 111 - 118, Update the priority test around SignedImage so it cannot reuse a signed URL cached by earlier tests: use a unique endpoint for this test or clear the signed-URL cache during setup. Preserve the assertions verifying that priority skips observe while still fetching and rendering the expected image.tests/document-viewer-non-pdf-preview.dom.test.tsx (1)
34-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not require two announcement channels unless they are intentional.
The new assertion adds
role="alert"while the test already verifies the assertive LiveAnnouncer region at Line 33. This locks in both channels for one failure. After selecting one production path, assert that path and add a no-duplicate regression check if both remain.🤖 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/document-viewer-non-pdf-preview.dom.test.tsx` at line 34, Update the failure assertion in the document preview test to use only the intentionally selected announcement channel, rather than requiring both the LiveAnnouncer region and the role="alert" element. If both production channels remain, retain the existing primary assertion and add a check that prevents duplicate announcements.tests/document-frame-contract.test.ts (1)
49-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep the contract test aligned with one announcement contract.
This test requires both
role="alert"andannounce(error)for the PDF owner. If one path is removed to avoid duplicate speech, these assertions will preserve the old dual-channel contract. Assert the selected announcement path instead.🤖 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/document-frame-contract.test.ts` around lines 49 - 56, Update the test case “announces canvas-level PDF preview failures after the frame is already ready” to assert only the chosen single PDF error announcement mechanism, removing the conflicting requirement for both role="alert" and announce(error). Keep the assertions for the other owners unchanged unless they use the same consolidated contract.
🤖 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 `@docs/outstanding-issues.md`:
- Around line 137-172: Add the missing open ledger rows `#256` and `#257` to the
recommended queue using the established /issues flow, preserving their existing
priorities and details so the queue remains complete. If the intended queue is a
subset instead, revise its contract and enforce that scope consistently rather
than leaving the omissions implicit.
In `@src/components/document-viewer/pdf-canvas-viewer.tsx`:
- Around line 107-117: Use a single announcement contract for preview failures:
in src/components/document-viewer/pdf-canvas-viewer.tsx lines 107-117, remove
either the LiveAnnouncer path or the visible role="alert" path; in
src/components/document-viewer/non-pdf-source-preview.tsx line 123, retain one
image announcement path and scope the alert to the message text. Update
tests/document-frame-contract.test.ts lines 49-56 and
tests/document-viewer-non-pdf-preview.dom.test.tsx line 34 to assert the
selected paths and prevent duplicate announcements.
- Around line 107-117: Update the error announcement effect in the PDF canvas
viewer to associate each error with the URL that produced it, then announce only
when that error URL matches the current url. Preserve the existing assertive
priority and eventId behavior for current-URL errors, preventing stale messages
from being announced after navigation.
In `@src/components/document-viewer/use-viewer-gestures.ts`:
- Around line 114-135: The wheel blocking logic in onWheelPassive must prevent
browser page zoom on every modifier-wheel event, including the first event and
subsequent events while Ctrl/Meta remains pressed. Keep blockingHandler attached
until the corresponding keyup instead of detaching after 180 ms, explicitly
define the behavior for unkeyed trackpad pinch, and update
tests/use-viewer-gestures.dom.test.tsx lines 94-106 with Chromium coverage for
both modifier-wheel and unkeyed pinch behavior; the existing
listener-registration assertion should be expanded to verify event cancellation.
---
Outside diff comments:
In `@src/components/clinical-dashboard/signed-image.tsx`:
- Around line 35-61: The shouldLoad state in SignedImage is initialized from
priority but does not react when that prop changes. Update the SignedImage load
gate so a false-to-true priority transition sets shouldLoad to true, and add a
rerender test in tests/signed-image.dom.test.tsx covering that transition.
---
Nitpick comments:
In `@src/components/clinical-dashboard/signed-image.tsx`:
- Line 166: Update the Image props in SignedImage to map the public
SignedImage.priority value to Next.js 16’s supported preload API, removing the
deprecated priority prop. Preserve the existing behavior for client-resolved
signed URLs and keep the public priority contract unchanged.
In `@tests/document-frame-contract.test.ts`:
- Around line 49-56: Update the test case “announces canvas-level PDF preview
failures after the frame is already ready” to assert only the chosen single PDF
error announcement mechanism, removing the conflicting requirement for both
role="alert" and announce(error). Keep the assertions for the other owners
unchanged unless they use the same consolidated contract.
In `@tests/document-viewer-non-pdf-preview.dom.test.tsx`:
- Line 34: Update the failure assertion in the document preview test to use only
the intentionally selected announcement channel, rather than requiring both the
LiveAnnouncer region and the role="alert" element. If both production channels
remain, retain the existing primary assertion and add a check that prevents
duplicate announcements.
In `@tests/signed-image.dom.test.tsx`:
- Line 90: Add a test case covering the false-to-true priority update path in
SignedImage: render without priority, rerender with priority enabled, then
assert loading begins immediately and IntersectionObserver is not used. Keep the
existing initial-render priority test unchanged.
- Around line 111-118: Update the priority test around SignedImage so it cannot
reuse a signed URL cached by earlier tests: use a unique endpoint for this test
or clear the signed-URL cache during setup. Preserve the assertions verifying
that priority skips observe while still fetching and rendering the expected
image.
🪄 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: cfdede50-430d-4dc2-8a32-3e5bd2c45988
📒 Files selected for processing (11)
docs/outstanding-issues.mdsrc/components/clinical-dashboard/image-lightbox.tsxsrc/components/clinical-dashboard/signed-image.tsxsrc/components/document-viewer/non-pdf-source-preview.tsxsrc/components/document-viewer/pdf-canvas-viewer.tsxsrc/components/document-viewer/use-viewer-gestures.tstests/document-frame-contract.test.tstests/document-viewer-non-pdf-preview.dom.test.tsxtests/signed-image.dom.test.tsxtests/ui-smoke.spec.tstests/use-viewer-gestures.dom.test.tsx
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
2 participants
Summary
docs/plans/document-viewer-redesign-plan.mdon docs: PDF and photo viewer redesign plan #1659).role=alert+ LiveAnnouncer; non-PDF image failure panel also exposesrole=alert(DocumentFrame already alerted shell errors).decoding=async;SignedImagegains optionalpriority(skips IntersectionObserver deferral). Remaining: PWA lifecycle decode, demo WebP/AVIF, call-site wiring.#214/#219indocs/outstanding-issues.mdand drops them from the recommended queue; updates#215detail.Verification
npx vitest runon Phase 0 files — 21 passednpm run test— 5450 passed | 1 skipped (515 files)npm run typecheck— exit 0npm run verify:pr-local -- --files <phase0 paths>— exit 0 (format, docs guards, lint, typecheck, build/client-bundle scan, RAG fixtures)Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
DocumentViewer.tsx, then Phase 2 unified DocumentFrame / photo chrome..local/workflow-evidence/2026-08-06T17-33-14-225Z-flightplan.json(classifiedui; treated as clinical document surface + privacy-adjacent for governance).Summary by CodeRabbit
New Features
Bug Fixes
Documentation