Fix: Maintain Center Alignment During Zoom & Reset (Issue #656) #849
Conversation
📝 WalkthroughWalkthroughThis pull request updates the OpenAPI schema documentation for query parameters and metadata properties, and refactors the ImageViewer component to improve zoom-on-scroll behavior by wrapping the TransformWrapper component, adding wheel step configuration, increasing maximum scale, and adjusting image fitting properties from contain to cover. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/backend/backend_python/openapi.json (1)
2204-2213: Address inconsistent metadata schema between ImageInCluster and ImageData.The
metadataproperty inImageInClusteris currently defined as a bare object type (or null) without any properties, whileImageData.metadatareferencesMetadataModelwhich has explicit properties. This creates:
- Schema inconsistency: Same logical field using incompatible schemas across models
- Potential breaking change: The bare object type is more restrictive and may reject metadata properties that the backend intends to return
Update
ImageInCluster.metadatato referenceMetadataModelfor consistency withImageData.metadata:"metadata": { "anyOf": [ { "$ref": "#/components/schemas/MetadataModel" }, { "type": "null" } ], "title": "Metadata" }
🧹 Nitpick comments (2)
frontend/src/components/Media/ImageViewer.tsx (1)
30-32: Potential redundant reset with key-based approach.The
useEffectthat callsresetTransform()onresetSignalchanges may be redundant since the component now remounts entirely whenimagePathchanges (due to thekeyprop on line 38). This could lead to:
- Double resets when both
imagePathandresetSignalchange simultaneously- Confusion about which mechanism is responsible for resetting
Clarify whether
resetSignalis used for scenarios other than image path changes, or consolidate the reset logic.docs/backend/backend_python/openapi.json (1)
1-2927: OpenAPI changes are unrelated to PR objectives.This PR is titled "Fix: Maintain Center Alignment During Zoom & Reset (Issue #656)" and the objectives focus entirely on ImageViewer zoom behavior. However, this OpenAPI schema file contains backend API documentation changes that are:
- Not mentioned in the PR title or objectives
- Unrelated to the zoom fix functionality
- Mixed with the frontend zoom changes
Consider separating unrelated changes into different PRs to:
- Improve reviewability and traceability
- Avoid coupling unrelated changes
- Make it easier to revert specific changes if needed
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/backend/backend_python/openapi.jsonfrontend/src/components/Media/ImageViewer.tsx
🔇 Additional comments (2)
frontend/src/components/Media/ImageViewer.tsx (1)
44-44: Verify centerOnInit behavior with key-based resets.The
centerOnInitprop centers the content on initial mount, but since you're usingkey={imagePath}to remount the component on every image change, verify that this doesn't cause unintended behavior such as:
- Brief flickers or layout shifts during the remount
- Conflicts with the imperative
resetTransform()calls from the useEffect hook (line 31)- Race conditions if
imagePathchanges while a reset is in progressConsider testing rapid image navigation to ensure smooth transitions without visual artifacts.
docs/backend/backend_python/openapi.json (1)
1120-1128: LGTM: Proper OpenAPI schema composition.Wrapping the
InputTypereference inallOfis the correct OpenAPI 3.1 pattern for adding metadata (description, default, title) to a referenced schema. The default value of"path"is reasonable and matches the description.
| <TransformWrapper | ||
| ref={transformRef} | ||
| key={imagePath} | ||
| initialScale={1} | ||
| maxScale={10} | ||
| wheel={{ | ||
| step: 0.15, | ||
| }} | ||
| centerOnInit | ||
| limitToBounds={false} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the ImageViewer.tsx file to verify the implementation
find . -name "ImageViewer.tsx" -type fRepository: AOSSIE-Org/PictoPy
Length of output: 109
🏁 Script executed:
# Read the ImageViewer.tsx file to verify the implementation details
cat -n ./frontend/src/components/Media/ImageViewer.tsxRepository: AOSSIE-Org/PictoPy
Length of output: 3028
🏁 Script executed:
# Search for issue #656 or related requirements documentation
rg -i "issue.*656|#656" --type-list | head -20
rg -i "dynamic.*scale|fit.*scale|minscale|zoom.*behavior" -A 3 -B 3Repository: AOSSIE-Org/PictoPy
Length of output: 50781
🏁 Script executed:
# Search for GitHub issues or requirements documentation about zoom behavior
find . -name "*.md" -o -name "*.txt" | xargs grep -l "656\|zoom.*scale\|fit.*screen" 2>/dev/null | head -10Repository: AOSSIE-Org/PictoPy
Length of output: 44
🌐 Web query:
react-zoom-pan-pinch library features minScale maxScale dynamic zoom capabilities
💡 Result:
Short answer: react-zoom-pan-pinch supports min/max scale and dynamic programmatic zoom. You can pass minScale and maxScale props to constrain zoom, and control zoom at runtime via the TransformWrapper render-props API (methods like zoomIn, zoomOut, setTransform, zoomToElement/zoomToPoint, zoomToCenter, etc.), so you can change scale/position dynamically or call zoom actions from buttons/handlers. The min/max bounds will clamp programmatic zoom operations (so increase maxScale if a programmatic zoom is being limited). [1][2][3]
Sources
- Library repo / README (features + control API). [1]
- npm package docs showing minScale / maxScale and other props. [2]
- Example/issue explaining minScale/maxScale limiting zoomToElement and solution (increase/remove maxScale). [3]
🌐 Web query:
issue 656 PictoPy zoom image viewer requirements
💡 Result:
I can fetch and summarize the exact requirements from PictoPy issue #656 — do you want me to open the project's GitHub issue and pull the full text now?
🏁 Script executed:
# Search for GitHub issues or PR references in the codebase
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.tsx" -o -name "*.ts" \) -exec grep -l "#656\|issue.*656" {} \; 2>/dev/null | head -5Repository: AOSSIE-Org/PictoPy
Length of output: 44
Critical: Implementation doesn't match stated requirements and library capabilities.
The code is missing essential zoom behavior features that react-zoom-pan-pinch supports:
- Missing minScale: No
minScaleprop is set, allowing infinite zoom-out and creating the "tiny/floating images" problem. - Hardcoded maxScale:
maxScale={10}is hardcoded instead of being computed dynamically based on image and viewport dimensions. - No dynamic fitScale computation: The code should compute a dynamic minimum scale—fit-to-screen when image exceeds viewport, or original resolution otherwise—but uses a hardcoded
initialScale={1}. - Missing axis-dependent zoom anchoring: No implementation of zoom behavior based on overflow state (no overflow, one axis overflowing, both axes overflowing).
- No automatic re-centering: Missing logic to snap the image to center and reset panning offsets when zooming back to minimum scale.
- objectFit: 'cover' crops images: Using
'cover'crops images instead of fitting them (use'contain'for fit-to-screen behavior).
The key={imagePath} approach resets the entire transform state when switching images, which is a workaround rather than implementing the zoom behavior system described.
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around lines 36–46, the
TransformWrapper usage lacks required zoom behavior: add a computed minScale
(fitScale) instead of omitting minScale, compute dynamic maxScale (don’t
hardcode 10) from image and viewport dimensions (e.g., based on image native
resolution and DPR or a multiple of fitScale), set initialScale to the computed
fitScale (fit-to-screen when image is larger, otherwise 1), replace
objectFit:'cover' with 'contain', remove the key={imagePath} reset workaround
and instead preserve TransformWrapper state across images, implement
axis-dependent zoom anchoring (decide anchor based on whether width/height
overflow the viewport and pass appropriate center/anchor values on zoom), and
add logic to auto-recenter/reset pan offsets when scale equals minScale so the
image snaps to center; compute image and viewport sizes on load/resize and
update these props and state accordingly.
| <div style={{ width: '100%', height: '100%' }}> | ||
| <TransformWrapper | ||
| ref={transformRef} | ||
| key={imagePath} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Using key prop to reset state is a workaround, not a proper fix.
While key={imagePath} does reset the transform state when changing images (addressing one symptom), this approach:
- Completely reinitializes the TransformWrapper component, which may cause performance issues or brief flickers
- Doesn't address the underlying zoom behavior requirements from issue BUG: Zoom-on-Scroll Behavior Does Not Match Expected Media Viewer UX #656
- Prevents intentional state preservation scenarios (e.g., maintaining zoom level when navigating between similar images)
The proper fix would compute appropriate initial zoom/pan values based on the new image dimensions rather than forcing a full component remount.
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around line 38, using
key={imagePath} to force-remount TransformWrapper is a workaround that resets
state and causes flicker; instead, compute and apply appropriate initial zoom
and pan when the image changes: detect when imagePath (or its
naturalWidth/naturalHeight) updates, calculate a fit-to-container scale and
centered translation (or reuse previous zoom when images are similar), then call
TransformWrapper/TransformComponent API or controlled props to set
initialTransform/position/scale without remounting; remove the key-based remount
and ensure updates happen only when dimensions change to preserve intentional
state and avoid performance issues.
| style={{ | ||
| maxWidth: '100%', | ||
| maxHeight: '100%', | ||
| objectFit: 'cover', |
There was a problem hiding this comment.
Critical: objectFit 'cover' will crop images instead of fitting them.
Changing from objectFit: 'contain' to objectFit: 'cover' contradicts the PR objective of "fit-to-screen" behavior. The cover value will crop the image to fill the container, potentially cutting off parts of the image. This is especially problematic for portrait images in landscape containers or vice versa.
The PR objectives specify that the minimum zoom should be "fit-to-screen when image > viewport," which aligns with contain behavior, not cover.
🔎 Recommended fix
style={{
maxWidth: '100%',
maxHeight: '100%',
- objectFit: 'cover',
+ objectFit: 'contain',
zIndex: 50,
transform: `rotate(${rotation}deg)`,
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| style={{ | |
| maxWidth: '100%', | |
| maxHeight: '100%', | |
| objectFit: 'cover', | |
| style={{ | |
| maxWidth: '100%', | |
| maxHeight: '100%', | |
| objectFit: 'contain', |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around lines 71 to 74, the
style sets objectFit: 'cover' which crops images and violates the PR's
fit-to-screen requirement; change objectFit back to 'contain' so images are
scaled to fit within the viewport without cropping, ensuring the minimum zoom
behaves as "fit-to-screen" for images larger than the viewport.
1 participant
Fix: Maintain Center Alignment During Zoom & Reset (Issue #656)
This PR fixes the behavior where images would drift off-center after zooming or switching images.
What was happening
What this PR changes
resetSignalorimagePathchangesResult
This brings the viewer behavior closer to typical image viewers and significantly improves UX.
Closes #656.
Summary by CodeRabbit
Documentation
Enhancements
✏️ Tip: You can customize this high-level summary in your review settings.