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.
There was a problem hiding this comment.
The previous ZoomableImage implementation softened zoom behavior in two ways:
- The wheel configuration used a zoom step of
0.1with a smoothing factor of0.001, which naturally eased the motion. - The image position was blended back toward the center using
safeRatio, making the transition away from the current anchor gradual rather than immediate.
Your implementation is more piecewise in nature: if the image still fits, it remains centered; if it overflows, it switches to edge or cursor anchoring and then clamps the final position. Because of this, when testing the same mouse-wheel movement on an image that starts at or above the viewport size and is being zoomed out, the interaction feels noticeably harsher.
TL;DR
Before: The wheel delta was smoothed, and image positioning was continuously interpolated or anchored with additional smoothing.
Current implementation: The wheel change is applied more directly, and the anchoring rule switches as soon as the overflow state changes. As a result, larger images that start at or above viewport resolution feel less smooth when zooming out.
The resulting values are correct, but the perceived motion changes because the transition between positioning modes is more abrupt.
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