Enhancement: Advanced Zoom on Scroll & Panning Logic #835
Conversation
refactored ImageViewer component to separate zoom logic from image viewing logic. Introduces 'smart zoom' logic, refined panning logic, and robust centering behaviors
|
|
📝 WalkthroughWalkthroughOpenAPI schema adjusted for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageViewer
participant ZoomableImage
participant TransformWrapper
participant Image
rect rgb(240,248,255)
Note over User,ImageViewer: User interactions (wheel, drag, toolbar buttons)
end
User->>ImageViewer: wheel / drag / click zoom buttons
ImageViewer->>ZoomableImage: call ref.zoomIn/zoomOut/reset or pass events
ZoomableImage->>TransformWrapper: apply scale/translate (clamp bounds, center logic)
TransformWrapper->>Image: render transformed image (rotation via CSS)
Image-->>TransformWrapper: load / error events
TransformWrapper-->>ZoomableImage: transform/state updates (overflow, limits)
ZoomableImage-->>ImageViewer: optional callbacks (onReset/onError)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (9)
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 |
|
Hi @rahulharpal1603, |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/components/Media/ImageViewer.tsx (1)
19-19: Consider usingZoomableImageReftype for the ref.The ref is typed as
ImageViewerRef, butZoomableImageexports its ownZoomableImageRefinterface. While both interfaces are structurally identical, using the actual exported type improves maintainability—ifZoomableImageRefchanges in the future, TypeScript would catch the mismatch.🔎 Suggested change
+import { ZoomableImage, ZoomableImageRef } from './ZoomableImage'; -import { ZoomableImage } from './ZoomableImage'; ... - const zoomableImageRef = useRef<ImageViewerRef>(null); + const zoomableImageRef = useRef<ZoomableImageRef>(null);frontend/src/components/Media/ZoomableImage.tsx (2)
34-74: Consider memoizinghandleResetwithuseCallback.The
handleResetfunction is used as a dependency in theuseEffecton line 82-84, but since it's not memoized, including it in the dependency array would cause the effect to re-run on every render. The current implementation omits it from dependencies, which will trigger an ESLintreact-hooks/exhaustive-depswarning.🔎 Suggested approach
Wrap
handleResetinuseCallback:+import { useCallback } from 'react'; ... - const handleReset = () => { + const handleReset = useCallback(() => { if ( !transformRef.current?.instance?.wrapperComponent || !imageRef.current ) return; // ... rest of function setIsOverflowing(false); - }; + }, []);Then update the effect:
useEffect(() => { handleReset(); - }, [resetSignal]); + }, [resetSignal, handleReset]);
85-125: Consider extracting shared centering logic.The centering calculation (lines 94-116) duplicates the logic in
handleReset(lines 41-72). Extracting this into a shared helper function would improve maintainability and reduce the risk of the two implementations diverging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/backend/backend_python/openapi.jsonfrontend/src/components/Media/ImageViewer.tsxfrontend/src/components/Media/ZoomableImage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Media/ImageViewer.tsx (1)
frontend/src/components/Media/ZoomableImage.tsx (1)
ZoomableImage(28-370)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
🔇 Additional comments (13)
docs/backend/backend_python/openapi.json (3)
1120-1128: Verify intent and client compatibility of input_type parameter schema change.The input_type parameter has been refactored from a direct
$refto anallOfwrapper. While this is valid OpenAPI 3.1.0 syntax, it may affect code generation and API client behavior. Confirm that:
- The allOf wrapper is intentional and not a side effect of schema generation.
- Existing API clients or generated SDKs that depend on this parameter handle the schema change correctly.
- This change is necessary for the frontend zoom/pan feature described in the PR objectives, or if it's an incidental backend change.
2204-2213: Clarify metadata additionalProperties constraint removal in ImageInCluster.The metadata field in ImageInCluster has been changed from a schema with explicit
additionalPropertiescontrol to an implicit object type. By default in OpenAPI 3.1.0, objects allow additional properties unless explicitly set tofalse. This change may affect:
- API validation strictness on clients and servers.
- Documentation clarity for API consumers about what metadata fields are allowed.
Confirm whether this relaxation of validation is intentional, and ensure downstream consumers (including the frontend components in this PR) are aware of this schema evolution.
1-2927: Scope mismatch: Backend schema changes in PR focused on frontend components.The PR objectives describe frontend enhancements (ZoomableImage component extraction, zoom/pan logic), but this file contains backend OpenAPI schema modifications. Clarify whether:
- These backend schema changes are dependencies for the frontend work, or
- The file selection is incidental, and the frontend changes (ZoomableImage.tsx, ImageViewer.tsx) are the primary focus.
If the schema changes are necessary, ensure they're justified in the PR description.
frontend/src/components/Media/ImageViewer.tsx (1)
17-37: Clean delegation pattern.The refactoring effectively extracts zoom/pan complexity into
ZoomableImagewhile preserving the public API ofImageViewer. The imperative handle correctly proxies all methods.frontend/src/components/Media/ZoomableImage.tsx (9)
1-13: LGTM on imports.The imports are appropriate for the component's functionality.
15-26: Well-defined interfaces.The prop and ref interfaces clearly define the component's API contract.
76-80: LGTM on imperative handle.The imperative methods are correctly exposed and safely handle potentially null refs.
127-145: LGTM on overflow calculation.The overflow state calculation is correct and handles the null image ref case safely.
147-246: Empty dependency array may cause linting warnings.The effect runs only on mount with
[], but referencesgetOverflowStatewhich is recreated each render. This works correctly because:
- The refs (
imageRef,transformRef) are stablegetOverflowStateonly reads fromimageRef.currentHowever, ESLint's
react-hooks/exhaustive-depsrule will flag this. If you want to suppress the warning explicitly, consider adding a lint disable comment or memoizinggetOverflowStatewithuseCallback.
357-364: Rotation may cause incorrect centering and panning bounds.The rotation is applied via CSS
transform: rotate(${rotation}deg), but the centering and panning calculations usenaturalWidth/naturalHeightandclientWidth/clientHeight. When the image is rotated 90° or 270°:
getBoundingClientRect()returns the rotated bounding box (swapped width/height)- But aspect ratio calculations in
handleResetusenaturalWidth/naturalHeightwhich don't changeThis can cause the image to be incorrectly centered or have wrong panning limits when rotated.
Consider testing with 90° rotation to verify the zoom/pan behavior is acceptable, or account for rotation in the dimension calculations:
const isSwapped = rotation === 90 || rotation === 270; const effectiveW = isSwapped ? baseH : baseW; const effectiveH = isSwapped ? baseW : baseH;
284-331: Well-implemented panning boundary clamping.The loose panning bounds (≈20px buffer) implementation is solid and prevents the image from being dragged completely off-screen. The clamping logic correctly handles all four edges.
346-365: Good error handling for image loading.The
onErrorhandler correctly prevents an infinite loop by nullifyingonerrorbefore setting the fallback source. Thedraggable={false}andselect-noneclass appropriately prevent default drag behavior that would interfere with custom panning.
372-372: Good practice setting displayName.Setting
displayNameon forwardRef components improves debugging experience in React DevTools.
Viewport-centered zoom now preserves existing pan offsets
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
frontend/src/components/Media/ZoomableImage.tsx (6)
29-29: Consider using React 19's ref-as-prop pattern.React 19 allows
refas a regular prop, makingforwardRefoptional. While your current implementation works correctly, you could simplify by acceptingrefdirectly in the props interface.Modern React 19 pattern
-export const ZoomableImage = forwardRef<ZoomableImageRef, ZoomableImageProps>( - ({ imagePath, alt, rotation, resetSignal }, ref) => { +interface ZoomableImagePropsWithRef extends ZoomableImageProps { + ref?: React.Ref<ZoomableImageRef>; +} + +export const ZoomableImage = ({ imagePath, alt, rotation, resetSignal, ref }: ZoomableImagePropsWithRef) => {Based on React 19 documentation provided.
47-47: Extract hardcoded scale to a named constant.The reset scale value is hardcoded. Define it as a constant (e.g.,
RESET_SCALE) for better maintainability and clarity, especially if this value needs to align withminScalein the TransformWrapper config.Proposed refactor
At the top of the component:
+const RESET_SCALE = 1; + export const ZoomableImage = forwardRef<ZoomableImageRef, ZoomableImageProps>( ({ imagePath, alt, rotation, resetSignal }, ref) => {Then use it:
- const scale = 1; + const scale = RESET_SCALE;
147-154: Extract magic numbers to named constants.Lines 148-149 and 154 contain magic numbers that should be defined as constants for better maintainability and clarity.
Proposed refactor
At the top of the component:
+const WHEEL_LINE_MODE_MULTIPLIER = 33; +const ZOOM_FACTOR = 0.001; +const MIN_SCALE = 1; +const MAX_SCALE = 8; + export const ZoomableImage = forwardRef<ZoomableImageRef, ZoomableImageProps>(Then use them:
- const multiplier = isLineMode ? 33 : 1; - const factor = 0.001; + const multiplier = isLineMode ? WHEEL_LINE_MODE_MULTIPLIER : 1; + const factor = ZOOM_FACTOR; - const newScale = Math.max(1, Math.min(8, currentScale + zoomChange)); + const newScale = Math.max(MIN_SCALE, Math.min(MAX_SCALE, currentScale + zoomChange));Also update TransformWrapper config on lines 253-254 to use the same constants.
267-284: Refactor duplicate overflow state logic.The
onTransformedandonZoomhandlers contain identical overflow state calculation logic. Extract this to a helper function to reduce duplication.Proposed refactor
+const updateOverflowState = (ref: ReactZoomPanPinchRef) => { + const scale = ref.state.scale; + const wrapper = ref.instance.wrapperComponent; + if (!wrapper) return; + const rect = wrapper.getBoundingClientRect(); + const overflow = getOverflowState(scale, rect.width, rect.height); + setIsOverflowing(overflow.width || overflow.height); +}; + onTransformed={(ref) => { - const scale = ref.state.scale; - const wrapper = ref.instance.wrapperComponent; - if (!wrapper) return; - const rect = wrapper.getBoundingClientRect(); - const overflow = getOverflowState(scale, rect.width, rect.height); - setIsOverflowing(overflow.width || overflow.height); + updateOverflowState(ref); }} onZoom={(ref) => { - const scale = ref.state.scale; - const wrapper = ref.instance.wrapperComponent; - if (!wrapper) return; - const rect = wrapper.getBoundingClientRect(); - const overflow = getOverflowState(scale, rect.width, rect.height); - setIsOverflowing(overflow.width || overflow.height); + updateOverflowState(ref); }}
304-307: Extract panning buffer to a named constant.The 20px panning buffer is hardcoded. Define it as a constant (e.g.,
PANNING_BUFFER_PX) for better maintainability, especially if this value needs tuning.Proposed refactor
At the top of the component:
+const PANNING_BUFFER_PX = 20; + export const ZoomableImage = forwardRef<ZoomableImageRef, ZoomableImageProps>(Then use it:
- const limitLeft = -scaledW + 20; - const limitRight = viewW - 20; - const limitTop = -scaledH + 20; - const limitBottom = viewH - 20; + const limitLeft = -scaledW + PANNING_BUFFER_PX; + const limitRight = viewW - PANNING_BUFFER_PX; + const limitTop = -scaledH + PANNING_BUFFER_PX; + const limitBottom = viewH - PANNING_BUFFER_PX;
335-365: Consider extracting cursor logic to reduce duplication.The cursor styling is set in three places (lines 339, 344, 364) based on
isOverflowing. While this works correctly, extracting it to a variable would improve maintainability.Proposed refactor
+const cursorStyle = isOverflowing ? 'move' : 'default'; + <TransformComponent wrapperStyle={{ width: '100%', height: '100%', overflow: 'visible', - cursor: isOverflowing ? 'move' : 'default', + cursor: cursorStyle, }} contentStyle={{ width: 'fit-content', height: 'fit-content', - cursor: isOverflowing ? 'move' : 'default', + cursor: cursorStyle, }} > <img ... style={{ ... - cursor: isOverflowing ? 'move' : 'default', + cursor: cursorStyle, }} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/ImageViewer.tsxfrontend/src/components/Media/ZoomableImage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Media/ImageViewer.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
🔇 Additional comments (2)
frontend/src/components/Media/ZoomableImage.tsx (2)
86-106: LGTM! Clean effect handling.Both effects are well-implemented:
- Reset effect properly depends on
resetSignalandhandleReset- Image load effect correctly handles both already-loaded and loading states with proper cleanup
353-357: LGTM! Proper error handling.The image error handler correctly sets
onerror = nullto prevent infinite loops before falling back to the placeholder. This is a best practice for handling image load failures.
+ improve overflow detection
1 participant
closes #656
Overview
Refactors the
ImageViewercomponent by separating the zoom logic into a separateZoomableImagecomponent to implement a professional-grade image viewer experience. This update introduces "smart zoom" logic, refined panning logic, and robust centering behaviors to address imprecise/obscure zoom-ins & outs and unresponsive controls.Changes
frontend/src/components/Media/ZoomableImage.tsx,frontend/src/components/Media/ImageViewer.tsxwheelevent interceptors and manual transform calculations.useReffor performant access to transform state anduseStatefor tracking overflow status.Key Features
1. Smart Zoom Logic
2. Advanced Panning Logic
3. UX Polish & Visuals
Dynamic Cursor:
default(Arrow): When image fits or cannot be panned.move(Hand/Crosshair): Immediately appears when the image overflows and is pannable.Smooth Zoom-Out: Zooming out now interpolates towards the visual center of the viewport, eliminating the glitch where the image would drift away entirely out of viewport.
Video Demonstration
2025-12-24.15-36-48.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.