Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 5 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: replacing editor image popups with the shared pan and zoom viewer. Description check The description is complete and directly related to the change. It covers the user impact, implementation, rationale, linked issue, visual proof, testing, platform considerations, AI disclosure, and c… Linked Issues check Direct linked issue: #32124. Objective 1, open the editor image popup in the shared dialog: at the reviewed head, ImageViewer.tsx imports PanZoomImageDialog and renders it with open={isPopupOpen}, src…Out of Scope Changes check The changes connect to #32124. They replace the editor popup with the shared dialog, delete ImageViewerPopup and its locale keys, and update tests for the new wiring. Zoom-ownership changes are needed…Full details: Description check
-
Explanation
The description is complete and directly related to the change. It covers the user impact, implementation, rationale, linked issue, visual proof, testing, platform considerations, AI disclosure, and checklist. It also clearly documents that the full test suite and build were not run locally and that CI is expected to cover them.
Full details: Linked Issues check
-
Explanation
Direct linked issue:
#32124. Objective 1, open the editor image popup in the shared dialog: at the reviewed head, ImageViewer.tsx imports PanZoomImageDialog and renders it with open={isPopupOpen}, src={previewSrc}, title={filename}, showTitle. The inline surface onClick sets isPopupOpen to true. Met. Objective 2, remove ImageViewerPopup and its popup-only zoom state: the head ImageViewer.tsx has no popup zoom state, popup surface ref, popup wheel listener, or popup resize observer, and does not import ImageViewerPopup. The change summary reports ImageViewerPopup.tsx deleted and its three locale keys removed from all locale files. The file deletion and locale removal rest on the change summary, because the file-level diff could not be read. Objective 3, diff and conflict-review previews: they render ImageViewer, so they receive the dialog. Met. Objective 4, inline preview and footer zoom controls unchanged: the head ImageViewer.tsx keeps the footer zoom out, reset, zoom in and percentage controls, the inline wheel listener, and the inline image click. Met. Drag panning on the inline preview was not added, which matches the stated exclusion. Automated tests: the change summary lists updated ImageViewer.test.tsx, PanZoomImageDialog.test.tsx and image-viewer-zoom-ownership.test.tsx. Test bodies were not read directly. Full pnpm test was not run locally, so CI results are unverified.
Full details: Out of Scope Changes check
-
Explanation
The changes connect to
#32124. They replace the editor popup with the shared dialog, delete ImageViewerPopup and its locale keys, and update tests for the new wiring. Zoom-ownership changes are needed so the open dialog takes Cmd/Ctrl +/- before the inline image behind it. In PanZoomImageDialog.tsx, the zoom listener uses popup priority and runs only while open. This keeps the keyboard zoom from#32184working for the popup. Two side effects are connected to this zoom ownership work and are not unrelated. (1) The shared dialog's zoom steps, including toolbar zoom, now anchor at the canvas center, including when no manual view exists. This also affects chat, Markdown and Mermaid surfaces. The PR description does not list this. (2) In ImageViewer.tsx, the inline zoom listener returns early for intrinsic-layout images, as it does for PDFs and non-owners. The change summary says intrinsic-layout images no longer receive these requests. The PR description states that the inline shortcut zoom is unchanged. The code and the description disagree for this case. The base file could not be read, so the prior inline behavior for intrinsic-layout images is not confirmed. The description should be corrected or the gating reviewed. No IPC, persisted state, or platform-specific changes appear in the reviewed evidence.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
0366080 to
726a01f
Compare
|
+1, I hit this today on 1.4.240 with a tall diagram PNG opened from the file explorer. The popup has no zoom buttons and no drag-to-pan, so it's hard to read the diagram. I checked this PR's approach (reusing PanZoomImageDialog) against current main and it's what I'd want. It currently shows merge conflicts with main. Would be great to get it rebased and reviewed. |
Clicking an image in an editor tab opened a popup with no zoom controls and no drag panning. It now opens PanZoomImageDialog, the viewer chat images, Markdown images and Mermaid diagrams already use, with zoom in/out/fit buttons, drag and wheel panning, and pinch or Ctrl+wheel zoom at the cursor. The old ImageViewerPopup and its popup-only zoom state are removed. The inline preview is unchanged. Cmd/Ctrl +/- keeps zooming the open popup, as stablyai#32184 made it do: the dialog now listens for the app zoom request with the popup priority, so it answers before the inline image behind it, and Cmd/Ctrl+0 returns it to the fitted view it opened at. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ImageViewerPopup is gone, so its three catalog entries no longer have a call site and fail the runtime-catalog check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…loses A closing dialog stays mounted while it fades out, and its popup-priority listener kept taking Cmd/Ctrl +/- for those 200ms, so the pane's image missed the key. The listener now follows `open`. The test also releases its pane listener when it fails, so one failure doesn't leak into later tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0a5d9f4 to
aeaeed0
Compare
|
Heads-up: #36576 replaces |
ELI5
Clicking an image in an editor tab opened a big popup that had no zoom buttons, and you couldn't drag the picture around. Now it opens in the same viewer that chat images, Markdown images and Mermaid diagrams already use. It has zoom out, zoom in, fit and close buttons, and you can drag the image to move around it.
What Changed
Before: the editor's full-size image popup (
ImageViewerPopup) showed the image and a zoom percentage. It zoomed only with Ctrl+wheel or pinch, had no zoom buttons, and moved only with the scrollbars or the wheel. Dragging did nothing.After: the popup is
PanZoomImageDialogfrom #30154. It has zoom out, zoom in, fit-to-screen and close buttons, drag and wheel panning (Shift+wheel pans sideways), and pinch or Ctrl+wheel zoom at the cursor. It shows the file name in the corner, and Escape and the close button both close it.How:
ImageViewerrendersPanZoomImageDialogwith the preview data URI and file name.ImageViewerPopup.tsxis deleted, along with the popup's own zoom state, surface ref, wheel listener and resize observer.ImageViewerPopupstrings are removed from every locale catalog, because the runtime-catalog check fails once their call site is gone.Cmd/Ctrl +/-still zooms the open popup, as fix(editor): let Cmd/Ctrl +/- zoom an open image #32184 made it do. The dialog listens for the same app zoom request with the popup priority fix(editor): let Cmd/Ctrl +/- zoom an open image #32184 added, so it answers before the inline image behind it.Cmd/Ctrl+0returns it to the fitted view. It hands the shortcut back as soon as it closes, not after its fade-out.ImageViewernow takes the shortcut only for its inline preview.Behavior differences, both matching the other surfaces that use this dialog:
Cmd/Ctrl+0returns it there instead of to 100%.Why
The editor's popup was the last full-size image view still on the older implementation, so the same image behaved differently depending on where it was opened. Moving it onto the shared dialog fixes the missing controls and panning with a net deletion, and keeps future viewer fixes in one place.
#9557 tried to add panning to the old popup with a separate pointer controller. Reusing the dialog the app already ships avoids a second set of gestures to maintain.
Linked Issue
Fixes #32124
This also covers the popup half of #9337. Drag panning on the inline preview is left out, because a click on the inline image opens this popup.
Visual Proof
Headless Electron e2e build, 832×624 PNG opened from the explorer and clicked to open the popup. In the second row the script clicks Zoom in three times where the button exists, then drags from the center toward the top left.
Popup on open
After zooming in and dragging
Testing
mainand on this branch)ImageViewer.test.tsxnow checks that clicking the preview opensPanZoomImageDialogon the same data URI and file name, and that closing it works.PanZoomImageDialog.test.tsxchecks that the zoom request zooms the dialog's image ahead of a pane listener registered before it, that reset returns to the fitted view, and that a closing dialog still fading out leaves the request to the pane (this last check fails without theopenguard).image-viewer-zoom-ownership.test.tsxthat read the old popup's percentage now check that the open popup takes the request and the inline image behind it stays at its zoom. Making the dialog's listener never claim fails 5 tests, and giving it the pane priority fails 4.PanZoomImageDialog.test.tsxandpan-zoom-view.test.ts.ImageViewerPopup.tsx, which this PR deletes, and a test comment inImageViewer.test.tsx.pnpm lintandpnpm typecheckpass locally. The editor, hooks, native-chat and sidebar suites pass (11,955 tests). One unhandled teardown error comes fromterminal-presentation-pane-title.test.ts, which imports nothing this PR changes; it does the same when run alone.pnpm testorpnpm buildlocally, because the full suite ran out of memory on my machine. Theelectron-vitee2e build used for the screenshots succeeded before this rebase, and the screenshots predate it. I'm relying on CI for the full suite.AI Disclosure
I used Claude Code (Claude Fable 5.1 and Claude Opus 5.5) to research the existing viewers and related issues and PRs, write the change and tests, and capture the before/after screenshots.
Review
Agent skill upstream boundary
Notes
ImageViewerPopup. If this lands first, that action fits the dialog'stoolbarActionsslot. feat(editor): zoom and pan standalone mermaid diagrams #18468 (Mermaid zoom) also edits the popup.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred). Lint and typecheck pass locally; see Testing for the rest.