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 |
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