Repository navigation
feat(image-viewer): add drag-to-pan gestures - #9557
OnlyYu1996 wants to merge 3 commits into
Conversation
|
Inline preview after Space + left-button pan at 244%: 
Popup preview after Space + left-button pan at 244%: 
|
📝 WalkthroughWalkthroughAdds image viewer panning with middle-button and Space-plus-left-button gestures. The implementation includes threshold detection, scroll updates, pointer capture, cleanup handling, cursor state, click suppression, and focus-loss behavior. Shared pan bindings are connected to inline and popup viewer surfaces, and native image dragging is disabled. New tests cover helper calculations, DOM gesture lifecycle, hook behavior, and rendered surface bindings. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/src/components/editor/image-viewer-dom-pan.ts (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComments are in Chinese, inconsistent with the rest of the PR's English "Why:" comments.
Every other explanatory comment in this cohort (e.g.
ImageViewer.tsx's// Why: Chromium exposes trackpad pinch...) is in English. Consider translating these two rationale comments for consistency and to keep them accessible to the whole team.♻️ Suggested translation
} catch { - // 元素可能已卸载，清理仍需继续移除窗口监听器。 + // Why: the surface may already be unmounted; keep removing window + // listeners regardless so cleanup always completes. }try { surface.setPointerCapture(pointerId) } catch { - // 捕获失败时窗口监听器仍能完成拖动和清理。 + // Why: if capture fails, the window-level listeners still allow the + // drag and cleanup to complete correctly. }Also applies to: 110-114
src/renderer/src/components/editor/use-image-viewer-pan.ts (1)
34-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a "why" comment for the modifier-key and pointer-inside guards.
The reasons for excluding Alt/Ctrl/Meta+Space and requiring
isPointerInsideRef.current(scoping Space-triggered panning to hover, avoiding collision with OS/browser Space shortcuts) aren't obvious from the code alone.As per path instructions: "For non-obvious TypeScript code, add a concise comment explaining why the constraint exists, not how the code works."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f0332c3-8769-4321-9666-88eb0e231851
📥 Commits
Reviewing files that changed from the base of the PR and between 0989128 and 2184a9076b4790cfa24e9509b1e7465926e6dc1e.
📒 Files selected for processing (9)
src/renderer/src/components/editor/ImageViewer.test.tsxsrc/renderer/src/components/editor/ImageViewer.tsxsrc/renderer/src/components/editor/ImageViewerPopup.tsxsrc/renderer/src/components/editor/image-viewer-dom-pan.test.tssrc/renderer/src/components/editor/image-viewer-dom-pan.tssrc/renderer/src/components/editor/image-viewer-pan.test.tssrc/renderer/src/components/editor/image-viewer-pan.tssrc/renderer/src/components/editor/use-image-viewer-pan.test.tsxsrc/renderer/src/components/editor/use-image-viewer-pan.ts
|
Addressed the Space-handler rationale note in 8fe6f2c3f by documenting why the guard is scoped to the hovered image surface without system modifiers. Focused image-viewer tests (27), web typecheck, oxlint, and React Doctor all pass. |
8fe6f2c to
aa6955e
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR adds drag-to-pan gestures (middle-button and Space+left-button) to the inline and popup image viewer surfaces. It introduces three new modules — a pure math/trigger helper (
Confidence Score: 4/5Safe to merge with a one-line guard added to the click-suppression condition. The pan controller, pointer-capture lifecycle, and cursor logic are all correct. The only defect is in the onEnd callback in use-image-viewer-pan.ts: suppressClickRef is armed whenever didDrag && trigger === 'space-left', regardless of how the drag ended. When the popup is closed by Escape mid-drag, or when the window loses focus during a space-left pan, the stale flag silently eats the user's next click on the inline image surface, preventing the popup from opening until a second attempt. Adding reason === 'pointerup' to the guard fully contains the issue. Files Needing Attention: src/renderer/src/components/editor/use-image-viewer-pan.ts — the onEnd callback inside onPointerDown
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/editor/use-image-viewer-pan.ts | New hook that manages Space/middle-button pan state; contains a click-suppression logic gap where non-pointerup endings (blur, lostpointercapture) incorrectly arm suppressClickRef. |
| src/renderer/src/components/editor/image-viewer-dom-pan.ts | New DOM-level pan controller; pointer capture, threshold detection, and cleanup paths are all correct and well-guarded. |
| src/renderer/src/components/editor/image-viewer-pan.ts | Pure utility functions for trigger detection, threshold check, and scroll-position math; all correct and well-tested. |
| src/renderer/src/components/editor/ImageViewer.tsx | Correctly gates inline pan with isIntrinsicLayout check (inlinePan = null) while sharing the same controller with the popup. |
| src/renderer/src/components/editor/ImageViewerPopup.tsx | Straightforward consumer of pan bindings; always uses scroll-based overflow so panning is always valid here. |
Sequence Diagram
sequenceDiagram
participant User
participant Surface as Inline/Popup Surface
participant Hook as useImageViewerPanSurface
participant DOM as beginImageViewerPan
User->>Surface: onPointerEnter
Note over Hook: isPointerInsideRef = true
User->>Surface: keydown(Space)
Note over Hook: isSpacePressed = true, cursor-grab
User->>Surface: "onPointerDown(button=0)"
Hook->>DOM: beginImageViewerPan(surface, pointerId, 'space-left')
DOM->>Surface: setPointerCapture(pointerId)
Note over Hook: isDragging = true, cursor-grabbing
User->>DOM: pointermove (window, crosses 4px threshold)
DOM->>Surface: scrollLeft/scrollTop updated
User->>DOM: pointerup (window)
DOM->>Hook: "onEnd({didDrag, reason:'pointerup', trigger:'space-left'})"
Note over Hook: suppressClickRef = true (only for pointerup)
User->>Surface: click event
Surface->>Hook: onClickCapture → stopPropagation
Note over Hook: suppressClickRef = false, openPopup blocked
Note over Hook,DOM: Middle-button follows same path without click suppression
Reviews (2): Last reviewed commit: "fix(image-viewer): limit panning to scro..." | Re-trigger Greptile
aa6955e to
abae3f6
Compare
Summary
grab/grabbingcursors.Closes #9337
Screenshots
Inline and popup screenshots at 244% zoom are attached in a follow-up PR comment.
Testing
pnpm lint(the full command is currently blocked by pre-existing switch-exhaustiveness errors inskill-freshness-group.tsxanddaemon-server.ts; changed-file oxlint, React Doctor, and the remaining lint gates pass)pnpm typecheckpnpm test(the focused image-viewer suite passes; the local full suite hit unrelated integration-test timeouts)pnpm buildAdditional verification:
AI Review Report
The review checked pointer event ordering, click suppression, capture loss, pointer cancellation, window blur, Space release, listener cleanup, and native image drag behavior. It flagged duplicate global keyboard listeners when inline and popup used separate controller instances; the implementation was changed to share one controller per image viewer and to update Space state only while a surface is hovered.
Cross-platform review covered macOS, Linux, and Windows. The implementation uses standard Pointer Events,
KeyboardEvent.code, button values, and existing Tailwind cursor classes without platform-specific shortcut labels, filesystem paths, shell behavior, or Electron-only APIs. The behavior is renderer-local and unchanged for local, SSH, and remote worktrees. It does not interact with agents, integrations, or git providers. Performance review confirmed one controller per mounted viewer and deterministic listener cleanup. UI review confirmed no layout or token changes and verified inline and popup screenshots.Security Audit
The change processes only local pointer and keyboard events. It adds no command execution, path handling, network or IPC calls, authentication, secrets, dependencies, or persisted input.
preventDefaultis scoped to supported pan gestures or Space while the pointer is over an image surface, and all temporary window listeners are removed on every terminal path. No follow-up security work is needed.Notes
ELI5
You can pan around zoomed images by dragging with the middle mouse button, or holding Space and dragging with the left button. It works in both the inline image view and the popup, with grab cursors so it feels like a normal image viewer.