Fix production zoom scroll behavior#1295
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughZoomableImage now uses a ChangesDynamic Zoom Wheel Interceptor Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
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 `@frontend/src/components/Media/__tests__/ZoomableImage.test.tsx`:
- Around line 97-127: Add two new tests in ZoomableImage.test.tsx alongside the
existing "stable container intercept" case: (1) an axis-dependent anchoring test
that sets up mockElementRect so only one axis overflows (e.g., width larger than
container but height fits), fires a wheel event and asserts mockSetTransform was
called with the expected anchor coordinates and scale change (use the same
pattern as the existing test referencing wheelArea, image, fireEvent.wheel, and
mockSetTransform), and (2) a recenter-on-min-scale test that starts at >min
scale, simulates a zoom-out to the minimum scale via wheel events, and asserts
that the transform was reset to the centered coordinates (relying on
ZoomableImage, mockElementRect, and mockSetTransform to verify recentering).
🪄 Autofix (Beta)
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: cf69a53e-2307-4afe-88f9-84a1a37efe16
📒 Files selected for processing (2)
frontend/src/components/Media/ZoomableImage.tsxfrontend/src/components/Media/__tests__/ZoomableImage.test.tsx
|
@VanshajPoonia can you create a test release in your fork for me to test the implementation? |
|
Sure, I created a test release in my fork: https://github.com/VanshajPoonia/PictoPy/releases/tag/v1.1.0-pr1295-test.1 The release is for testing the zoom on scroll implementation from this PR |
|
Not exactly what I meant. To create a release for testing, you'll need to manually trigger the For the workflow to run successfully, you'll also need to remove a few Please refer to the link in the Discord server for more details on how to get the workflow running on your fork: |
|
Hi, just confirming before I run the test release workflow. I’m planning to use a separate temporary branch in my fork with my PR changes plus the workflow changes from the Discord thread you referred me to. The temporary branch would include:
I’ll keep my actual PR branch unchanged with only the zoom fix. The temporary branch will only be used to run build-and-release.yml from my fork, and I’ll share the generated release link once it finishes. Does that match what you expected? |
Yes, that's exactly the right approach |
|
I ran the test release workflow from a temporary branch in the fork using the fork safe workflow changes. Release link: Workflow run: |
The Zoom on Scroll functionality is still the same (broken) in the release 🤔 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/Media/ZoomableImage.tsx (1)
154-159: ⚡ Quick winUse a single owned viewport element (wheelAreaRef) for all geometry math
getViewportElement()falls back totransformRef.current?.instance?.wrapperComponent, butwrapperComponentis an internal (undocumented, non-public) implementation detail inreact-zoom-pan-pinchv3.7.0; this can couple rect/overflow/wheel calculations to library internals and change behavior across lifecycle phases whenwrapperComponentbecomes available. Prefer always measuring againstwheelAreaRef.currentforgetBoundingClientRect()/overflow math (and remove thewrapperComponentfallback).Also applies to: 248-255, 268-271
🤖 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 `@frontend/src/components/Media/ZoomableImage.tsx` around lines 154 - 159, Replace the logic in getViewportElement so it always returns the owned wheelAreaRef.current (remove any use of transformRef.current?.instance?.wrapperComponent), and update any other places that currently branch to wrapperComponent (referenced as transformRef, wrapperComponent and getViewportElement) to use wheelAreaRef.current for all getBoundingClientRect/overflow/wheel geometry math; ensure wheelAreaRef is non-null where used or guard with a clear early return to avoid accessing undefined.
🤖 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 `@frontend/src/components/Media/ZoomableImage.tsx`:
- Around line 114-150: getScaledDimensions and getOverflowState assume scale===1
is the fit-to-view floor; instead compute a true minimum scale from the measured
viewer size and the image’s natural dimensions (use
imageRef.current.naturalWidth/naturalHeight and the current viewportRect) and
use that as the baseline. Update getScaledDimensions (which currently calls
getEffectiveDimensions) to accept or compute minScale and return dimensions
based on Math.max(scale, minScale); update getOverflowState to compare
scaledDimensions (using that minScale) against viewportWidth/viewportHeight; and
change the wheel-recenter path to clamp the target scale to that computed
minScale so recentering never produces a scale smaller than the measured
minimum. Ensure references to getScaledDimensions, getOverflowState,
getEffectiveDimensions, imageRef, and viewportRect are used to locate and
implement these changes.
---
Nitpick comments:
In `@frontend/src/components/Media/ZoomableImage.tsx`:
- Around line 154-159: Replace the logic in getViewportElement so it always
returns the owned wheelAreaRef.current (remove any use of
transformRef.current?.instance?.wrapperComponent), and update any other places
that currently branch to wrapperComponent (referenced as transformRef,
wrapperComponent and getViewportElement) to use wheelAreaRef.current for all
getBoundingClientRect/overflow/wheel geometry math; ensure wheelAreaRef is
non-null where used or guard with a clear early return to avoid accessing
undefined.
🪄 Autofix (Beta)
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: 69f802e3-5394-4450-b1ad-78f06ec4f375
📒 Files selected for processing (2)
frontend/src/components/Media/ZoomableImage.tsxfrontend/src/components/Media/__tests__/ZoomableImage.test.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/Media/__tests__/ZoomableImage.test.tsx (1)
159-183: ⚡ Quick winDocument the intentionally-null wrapper to protect the regression scenario.
This test deliberately leaves
mockWrapperComponentasnull(reset inbeforeEach), so it exercises thegetViewportElement()fallback towheelArea— i.e. the exact "internal transform wrapper not ready" path this PR fixes. Without a note, a future maintainer could addmockWrapperComponent = ...here and silently invalidate the regression coverage.📝 Suggested clarifying comment
test('keeps zoom centered while the image still fits in the viewport', () => { + // Intentionally do NOT set mockWrapperComponent: this verifies the + // getViewportElement() fallback when the library wrapper isn't ready (`#1293`). const { container } = renderZoomableImage();🤖 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 `@frontend/src/components/Media/__tests__/ZoomableImage.test.tsx` around lines 159 - 183, Add a short explanatory comment in the test "keeps zoom centered while the image still fits in the viewport" stating that mockWrapperComponent is intentionally left null (it’s reset in beforeEach) so the test exercises the getViewportElement() fallback to wheelArea; place the comment near the call to renderZoomableImage()/mockWrapperComponent declaration so future maintainers don't set mockWrapperComponent and inadvertently remove this regression coverage for getViewportElement(), wheelArea and the internal transform wrapper path.
🤖 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.
Nitpick comments:
In `@frontend/src/components/Media/__tests__/ZoomableImage.test.tsx`:
- Around line 159-183: Add a short explanatory comment in the test "keeps zoom
centered while the image still fits in the viewport" stating that
mockWrapperComponent is intentionally left null (it’s reset in beforeEach) so
the test exercises the getViewportElement() fallback to wheelArea; place the
comment near the call to renderZoomableImage()/mockWrapperComponent declaration
so future maintainers don't set mockWrapperComponent and inadvertently remove
this regression coverage for getViewportElement(), wheelArea and the internal
transform wrapper path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8deb3f05-4d25-444b-8724-95f95d6c46a8
📒 Files selected for processing (2)
frontend/src/components/Media/ZoomableImage.tsxfrontend/src/components/Media/__tests__/ZoomableImage.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Media/ZoomableImage.tsx
|
I am still working on this issue |
|
@rohan-pandeyy Thank you for testing the earlier build. I have pushed an updated implementation that removes the hidden zoom lifecycle dependency on react-zoom-pan-pinch for this viewer and now controls the image transform directly. The public ImageViewer API is unchanged. I also updated the temporary fork-only workflow branch and ran the build-and-release workflow successfully from there. Root cause: the custom zoom behavior depended on react-zoom-pan-pinch internals being initialized at the right time. In production, the image/wrapper sizing and transform lifecycle could initialize differently than in dev, so the viewer sometimes fell back into stale/default transform behavior. Fix: ZoomableImage now owns the image transform directly instead of layering custom behavior over react-zoom-pan-pinch. Wheel zoom, fit-to-view, panning, clamping, image switch reset, rotation reset, and recentering are handled in one controlled path. Workflow run: Release link: The real PR branch is still focused on the zoom fix only. The temporary release branch contains the fork-safe workflow changes needed to generate test artifacts. |
rohan-pandeyy
left a comment
rohan-pandeyy
left a comment
There was a problem hiding this comment.
@VanshajPoonia, thanks for the test release. I verified the changes, and they do work. However, there is a subtle behavioral difference that I'd like to point out. Please read the comment mentioned within ZoomableImage component.
|
@coderabbitai Please review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== ZoomableImage.tsx ==="
cat frontend/src/components/Media/ZoomableImage.tsx
echo ""
echo "=== ZoomableImage.test.tsx ==="
cat "frontend/src/components/Media/__tests__/ZoomableImage.test.tsx"
echo ""
echo "=== Media folder structure ==="
tree frontend/src/components/Media/ 2>/dev/null || find frontend/src/components/Media -type f | sort
echo ""
echo "=== Other Media components ==="
for f in $(find frontend/src/components/Media -name "*.tsx" ! -name "ZoomableImage*" | sort); do
echo "--- $f ---"
cat "$f"
echo ""
doneLength of output: 73708
🐛 Potential Bugs & Edge Cases1. Dual pointer + mouse handlers fire on the same gestureThe component registers both While this happens to be safe today, 2. Double viewport measurement in
|
| Convention | Other components | ZoomableImage |
|---|---|---|
| Complex logic → custom hook | MediaView uses useImageViewControls, useSlideshow, useKeyboardNavigation |
All logic inline in one 420-line component |
React.FC<Props> for presentation |
MediaViewControls, NavigationButtons, ZoomControls, MediaInfoPanel |
Uses forwardRef (correct for imperative ref, but inner logic could still be extracted) |
displayName on forwardRef |
ZoomableImage sets it |
ImageViewer does not |
Pure utils in utils/ |
dateUtils.ts, etc. |
No zoomUtils.ts exists |
ResizeObserver without feature-check |
ChronologicalGallery uses it unconditionally |
ZoomableImage guards with typeof ResizeObserver === 'function' — good, more defensive |
| Named exports | All components | ✅ Consistent |
The biggest gap is that MediaView and other complex components extract their stateful logic into named custom hooks, while ZoomableImage inlines everything. A useZoomTransform(viewportRef, imageRef, rotation, imagePath, resetSignal) hook would bring this into alignment with the project's own patterns and make the JSX in ZoomableImage trivially readable.
Summary
| Priority | Issue |
|---|---|
| 🔴 Bug | Dual pointer+mouse handlers — isPanning can stick if pointer capture fails |
| 🔴 Bug | Double getElementSize in zoomBy — divergent left/top vs dimensions |
| 🟡 Edge case | isOverImage guard causes snap-to-center when cursor is in padding |
| 🟡 Edge case | readRawDimensions prefers clientWidth over fallbackDimensions pre-load |
| 🟡 Edge case | LINE_HEIGHT_MULTIPLIER undocumented; causes 33× faster zoom in line-mode |
| 🟡 Edge case | fitRetryCountRef not reset in clearScheduledFit |
| 🔵 Maintainability | Pure math functions should be in zoomUtils.ts |
| 🔵 Maintainability | zoomBy should be split into a testable pure function |
| 🔵 Convention | Logic should be extracted to useZoomTransform hook per project pattern |
| 🔵 Convention | ImageViewer missing displayName |
| 🔵 Cosmetic | Redundant cursor style on 3 elements |
|
Both bugs are valid. The edge cases will need to be tested thoroughly. I agree with the maintainability concern regarding the math functions. The file is becoming quite large, and separating them out would make it easier to manage and maintain. The convention-related comments are mandatory and should be addressed. The cosmetic suggestions are good to have, but they are lower priority. |
|
Hey @VanshajPoonia, |
|
I will hopefully be able to hop on. I do we are going back and forth a lot on this particular issue. |
|
(Refer to the next comment) |
|
Thanks for the earlier feedback. I pushed the follow-up zoom updates. This now includes both changes:
The existing fit-to-view, edge-before-anchoring, axis-dependent anchoring, and minimum-recenter behavior are kept intact. I tested this locally on macOS with an external mouse, and the zoom-in behavior feels much smoother now, especially on high-resolution images. Commits: Verification: I also merged the same changes into my temporary test-release workflow branch and ran the fork release workflow again. Workflow run: New test release: Could you please verify this build on Windows as well, especially with a physical mouse wheel and the zoom buttons? |
rohan-pandeyy
left a comment
rohan-pandeyy
left a comment
There was a problem hiding this comment.
Great work!
And thanks for your patience with all the test releases 😄
2 participants
Fixes #1293
Screenshots/Recordings:
This PR addresses the zoom-on-scroll behavior described in issue #1293 and the linked reference issue #656.
Before:
After:
Additional Notes:
The regression was caused by the custom wheel listener depending on
react-zoom-pan-pinch's internalwrapperComponentbeing available during the first effect run. In production builds, that timing could fail, causing the viewer to use the library's default wheel behavior.This PR attaches the custom wheel listener to a stable container owned by
ZoomableImageand disables the library's default wheel handling. A focused regression test was added to verify the custom wheel behavior still works when the internal transform wrapper is not ready.Local verification completed:
AI Usage Disclosure:
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: Codex
Checklist
Summary by CodeRabbit
Refactor
Tests