Zoom-on-Scroll Support in Image Preiew #530
Conversation
WalkthroughImageViewer was refactored to use react-zoom-pan-pinch (TransformWrapper/TransformComponent) and now exposes an imperative ImageViewerRef (zoomIn/zoomOut/reset) via forwardRef and a resetSignal prop; MediaView now holds a ref and uses new zoom/reset handlers; OpenAPI schemas added additionalProperties; dependency added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MediaView
participant ImageViewer as ImageViewer (forwardRef)
participant Transform as TransformWrapper/TransformComponent
participant IMG as <img>
User->>MediaView: open image / interact
MediaView->>ImageViewer: render with `ref` and `resetSignal`
ImageViewer->>Transform: mount wrapper/component
Transform->>IMG: render image with transform styles
alt User zooms/pans
User->>IMG: scroll/pinch/drag
IMG->>Transform: input events
Transform->>IMG: update scale/translate
end
alt Parent calls methods
MediaView->>ImageViewer: call zoomIn/zoomOut/reset
ImageViewer->>Transform: invoke control methods via transformRef
Transform->>IMG: update transform
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/components/Media/MediaView.tsx (2)
⏰ 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 (1)
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: 2
🧹 Nitpick comments (4)
frontend/src/components/Media/ImageCard.tsx (1)
35-37: Drop debug console.log in click handler.Avoids noisy logs and accidental PII leakage.
Apply this diff:
- console.log(imageIndex); dispatch(setCurrentViewIndex(imageIndex));frontend/src/components/Media/MediaView.tsx (1)
107-107: Remove commented-out dead code.Keeps the file clean.
- // const [mouselocation, setMouselocation] = useState({x:0,y:0});frontend/src/components/Media/ImageViewer.tsx (1)
52-59: Transform-origin and containment updates look good.This aligns with wheel-zoom math. Consider adding willChange: 'transform' for smoother animations on capable devices.
Optional:
transition: isDragging ? 'none' : 'transform 0.2s ease-in-out', cursor: isDragging ? 'grabbing' : 'grab', + willChange: 'transform', maxWidth: '100%', maxHeight: '100%', userSelect: 'none', objectFit: 'contain',frontend/src/pages/Home/Home.tsx (1)
32-32: Remove stray console.log (frontend/src/pages/Home/Home.tsx:32)This log is noisy and can trigger react-hooks/exhaustive-deps warnings; remove it or move it into a useEffect keyed on
displayImages.Apply this diff:
- console.log(displayImages);Or keep for debugging:
useEffect(() => { if (process.env.NODE_ENV !== 'production') { // eslint-disable-next-line no-console console.log(displayImages); } }, [displayImages]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/components/Media/ImageCard.tsx(1 hunks)frontend/src/components/Media/ImageViewer.tsx(1 hunks)frontend/src/components/Media/MediaView.tsx(2 hunks)frontend/src/hooks/useImageViewControls.ts(2 hunks)frontend/src/pages/Home/Home.tsx(1 hunks)
| onWheel={(e) => { | ||
| const rect = e.currentTarget.getBoundingClientRect(); | ||
| handlers.handleWheelZoom(e, rect); | ||
| }} |
There was a problem hiding this comment.
Pass the image element’s rect to wheel-zoom to keep the cursor anchored.
Using the container’s rect causes anchor drift with centered layout. Query the child img’s rect.
Apply this diff:
- onWheel={(e) => {
- const rect = e.currentTarget.getBoundingClientRect();
- handlers.handleWheelZoom(e, rect);
- }}
+ onWheel={(e) => {
+ const img =
+ (e.currentTarget as HTMLElement).querySelector('img') ?? undefined;
+ const rect =
+ img?.getBoundingClientRect() ?? e.currentTarget.getBoundingClientRect();
+ handlers.handleWheelZoom(e, rect);
+ }}Note: This avoids ref pluming. If you prefer a stricter approach, forward a ref from ImageViewer to the img and use that rect directly.
📝 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.
| onWheel={(e) => { | |
| const rect = e.currentTarget.getBoundingClientRect(); | |
| handlers.handleWheelZoom(e, rect); | |
| }} | |
| onWheel={(e) => { | |
| const img = | |
| (e.currentTarget as HTMLElement).querySelector('img') ?? undefined; | |
| const rect = | |
| img?.getBoundingClientRect() ?? e.currentTarget.getBoundingClientRect(); | |
| handlers.handleWheelZoom(e, rect); | |
| }} |
🤖 Prompt for AI Agents
In frontend/src/components/Media/MediaView.tsx around lines 128-131, the onWheel
handler currently grabs the container's bounding rect which causes anchor drift;
change it to get the child img element's bounding rect (e.g., find the img via
e.currentTarget.querySelector('img') and call getBoundingClientRect() on that
node) and pass that rect to handlers.handleWheelZoom; if the img is not found,
fall back to the container rect. Alternatively, if you prefer stricter typing
and no DOM queries, forward a ref from ImageViewer to the img and use that
ref.current.getBoundingClientRect() instead.
|
@Vaibhaviitian Please fix the linting errors |
|
@rahulharpal1603 done sir |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
The current approach doesn't seem very intuitive. When I have my mouse pointer on the door handle, it should zoom near that thing, not anywhere else (See the attached video)
2025-10-07.00-44-31.mov
sir i had tried to make as ideal zoom work but what happens when user scrolled for first time then the x,y coordinate for the user cursor get changed and then scaling happening with respect to that new x,y so this was the issue i had faced , |
Yes, you have to improve it. |
ok sir |
Sir i had updated the logic and i have added new video in pr please revview |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/Media/MediaView.tsx (1)
143-143: Anchor drift issue already flagged in previous review.A previous review comment already identified that using the container's rect causes anchor drift during wheel zoom. The suggested fix is to query the child img element's rect instead. This aligns with the zoom positioning issues you mentioned in the PR comments.
Please refer to the previous review comment for the suggested solution.
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaView.tsx (1)
106-106: Remove commented-out code.Commented-out code should be removed to keep the codebase clean. If this state is needed in the future, it can be retrieved from version control.
Apply this diff to remove the commented-out code:
- // const [mouselocation, setMouselocation] = useState({x:0,y:0});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src/components/Media/ImageViewer.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(3 hunks)frontend/src/hooks/useImageViewControls.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Media/ImageViewer.tsx
- frontend/src/hooks/useImageViewControls.ts
⏰ 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/MediaView.tsx (2)
140-142: LGTM!The simplified onClick handler is cleaner and maintains the same functionality.
149-149: NavigationButtons signature includes onNext prop Verified that NavigationButtonsProps defines onNext and the component destructures it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/hooks/useImageViewControls.ts (1)
20-45: Zoom anchor still drifts; container rect ≠ image rect when image is centered.The previous review flagged that using the container's bounding rect causes zoom drift when the image is centered (flex layout). The current implementation still retrieves the parent element's rect (line 26) and computes mouse coordinates relative to it (lines 27-28), which doesn't account for the image's actual position within that container.
Why this causes drift:
- The image is centered in its container (likely via flex layout)
transformOrigin: 'top left'means transforms are anchored to the image's top-left corner- When you compute
mouseX/Yrelative to the container's top-left, you're not accounting for the gap between the container edge and the image edge- Each zoom adjusts
position.x/y, but the baseline is wrong, so the cursor "slips" after the first scrollThe fix: Pass the image element's
getBoundingClientRect()(not the parent's) to the handler, either as a parameter or by directly accessing the image ref. The previous review provides a comprehensive solution with the correct math.
🧹 Nitpick comments (1)
frontend/src/hooks/useImageViewControls.ts (1)
24-26: Consider passing the rect explicitly for robustness.Traversing the DOM via
e.currentTarget.parentElementassumes a specific DOM structure. If the component layout changes (e.g., wrapper div added), this breaks.Instead, consider passing the image element's rect as a parameter to
handleWheelZoom, or use a ref to the image directly. This makes the code more resilient and easier to test.Example signature:
const handleWheelZoom = useCallback((e: React.WheelEvent, imageRect: DOMRect) => { e.preventDefault(); e.stopPropagation(); // Use imageRect directly... }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/ImageViewer.tsx(2 hunks)frontend/src/hooks/useImageViewControls.ts(2 hunks)
🚧 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: Backend Tests
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
|
@rahulharpal1603 sir please review this |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Vertical images are being displayed as zoomed in by default:
Current behaviour:
Expected behaviour:
Image link: https://unsplash.com/photos/brown-gondola-on-body-of-water-XG391m6rH_8
| {/* Main viewer area */} | ||
| <div | ||
| className="relative flex h-full w-full items-center justify-center" | ||
| className="relativ flex h-full w-full" |
There was a problem hiding this comment.
It should be relative
|
@rahulharpal1603 sir i opened some vertical images but seems no issue see the video i had attached |
Recording.2025-10-19.131926.mp4see it sir @rahulharpal1603 , and i corrected relative |
Don't take a screenshot of the boat image. Download its full resolution version and then show how it's being displayed. |
|
so sir i think it is not a problem that boat image have large dimension that is the issue if you makes it dimension shorter then it would be correct |
|
let me know sir for further moves |
If you open a branch without all your 9 commits and then test with the boat image, then the correct behaviour is there. After your commits, this issue is occuring. |
okay sir let me check |
sir i had tried to fix it but not able to figure out it. 😞😞😞😞😞😞😞😞😞😞 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/components/Media/ImageViewer.tsx (1)
53-63: Consider extracting static styles to CSS classes.The inline
styleobject has grown significantly with many static properties (maxWidth,maxHeight,userSelect,objectFit,display). For better maintainability, consider moving the static styles to a CSS class and keeping only the dynamic transform-related properties inline.For example, create a CSS class:
.image-viewer-img { max-width: 100%; max-height: 100%; user-select: none; object-fit: contain; display: block; }Then simplify the inline styles to only dynamic properties:
<img + className="image-viewer-img" src={convertFileSrc(imagePath) || '/placeholder.svg'} alt={alt} draggable={false} onError={(e) => { const img = e.target as HTMLImageElement; img.onerror = null; img.src = '/placeholder.svg'; }} onWheel={onWheel} style={{ transform: `translate(${position.x}px, ${position.y}px) scale(${scale}) rotate(${rotation}deg)`, transformOrigin: '0 0', transition: isDragging ? 'none' : 'transform 0.2s ease-in-out', cursor: isDragging ? 'grabbing' : 'grab', - maxWidth: '100%', - maxHeight: '100%', - userSelect: 'none', - objectFit: 'contain', - display: 'block', }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/ImageViewer.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Media/MediaView.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). (5)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Tauri Build Check (ubuntu-22.04)
- GitHub Check: Frontend Tests
- GitHub Check: Linting
🔇 Additional comments (1)
frontend/src/components/Media/ImageViewer.tsx (1)
16-16: LGTM: Event handler prop addition is correct.The
onWheelprop follows the same pattern as other event handlers in the interface and is appropriately typed.
| <div | ||
| id="zoomable-image" | ||
| className="relative" | ||
| onClick={onClick} | ||
| onMouseDown={onMouseDown} | ||
| onMouseMove={onMouseMove} | ||
| onMouseUp={onMouseUp} | ||
| onMouseLeave={onMouseLeave} | ||
| > | ||
| <img | ||
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | ||
| alt={alt} | ||
| draggable={false} | ||
| onError={(e) => { | ||
| const img = e.target as HTMLImageElement; | ||
| img.onerror = null; | ||
| img.src = '/placeholder.svg'; | ||
| }} | ||
| onWheel={onWheel} |
There was a problem hiding this comment.
Move onWheel to the parent div for consistent cursor position handling.
The onWheel handler is attached to the img element (line 52), while all other interaction handlers (onClick, onMouseDown, onMouseMove, onMouseUp, onMouseLeave) are attached to the parent div (lines 37-41). This inconsistency creates coordination problems because cursor position calculations will be relative to different elements. For zoom-to-cursor functionality to work correctly, the wheel event must capture the cursor position relative to the same element as the other handlers.
Apply this diff to move onWheel to the parent div:
<div
id="zoomable-image"
className="relative"
onClick={onClick}
onMouseDown={onMouseDown}
onMouseMove={onMouseMove}
onMouseUp={onMouseUp}
onMouseLeave={onMouseLeave}
+ onWheel={onWheel}
>
<img
src={convertFileSrc(imagePath) || '/placeholder.svg'}
alt={alt}
draggable={false}
onError={(e) => {
const img = e.target as HTMLImageElement;
img.onerror = null;
img.src = '/placeholder.svg';
}}
- onWheel={onWheel}
style={{📝 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.
| <div | |
| id="zoomable-image" | |
| className="relative" | |
| onClick={onClick} | |
| onMouseDown={onMouseDown} | |
| onMouseMove={onMouseMove} | |
| onMouseUp={onMouseUp} | |
| onMouseLeave={onMouseLeave} | |
| > | |
| <img | |
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | |
| alt={alt} | |
| draggable={false} | |
| onError={(e) => { | |
| const img = e.target as HTMLImageElement; | |
| img.onerror = null; | |
| img.src = '/placeholder.svg'; | |
| }} | |
| onWheel={onWheel} | |
| <div | |
| id="zoomable-image" | |
| className="relative" | |
| onClick={onClick} | |
| onMouseDown={onMouseDown} | |
| onMouseMove={onMouseMove} | |
| onMouseUp={onMouseUp} | |
| onMouseLeave={onMouseLeave} | |
| onWheel={onWheel} | |
| > | |
| <img | |
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | |
| alt={alt} | |
| draggable={false} | |
| onError={(e) => { | |
| const img = e.target as HTMLImageElement; | |
| img.onerror = null; | |
| img.src = '/placeholder.svg'; | |
| }} |
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around lines 34 to 52, the
onWheel handler is attached to the <img> while all other mouse handlers are on
the parent <div>, causing inconsistent cursor position references for
zoom-to-cursor; move the onWheel prop from the <img> to the parent <div> so it
sits alongside onClick/onMouseDown/onMouseMove/onMouseUp/onMouseLeave, remove
onWheel from the <img>, and ensure event types/handler signature remain
unchanged (pass the same handler reference and update any refs/logic if it
relied on event.target being the image).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/Media/ImageViewer.tsx (1)
69-78: Remove inline transform that overrides library behavior.The inline
transformstyle at line 75 directly conflicts withreact-zoom-pan-pinch's internal transform management. When the user zooms or pans, the library calculates the correct transform, but this inline style immediately overrides it with stale values from props, breaking the zoom-to-cursor and pan functionality you're trying to implement.The library already handles:
- ✓ Scale (zoom) via mouse wheel
- ✓ Translation (pan) via mouse drag
- ✓ Smooth transitions
- ✓ Cursor states (grab/grabbing)
Remove the custom transform and let the library handle it. If you need rotation, apply it separately and carefully to avoid interfering with zoom/pan math (see previous comment for approach).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
frontend/package.json(1 hunks)frontend/src/components/Media/ImageViewer.tsx(2 hunks)frontend/src/components/Media/MediaView.tsx(2 hunks)frontend/src/hooks/useImageViewControls.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/hooks/useImageViewControls.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/Media/MediaView.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 (ubuntu-22.04)
- GitHub Check: Tauri Build Check (windows-latest)
- GitHub Check: Tauri Build Check (macos-latest, --target aarch64-apple-darwin)
- GitHub Check: Backend Tests
🔇 Additional comments (1)
frontend/package.json (1)
64-64: Good library choice for zoom/pan functionality.The
react-zoom-pan-pinchlibrary (v3.7.0) is well-suited for implementing zoom-on-scroll with proper zoom-to-cursor behavior. This version includes fixes for mobile pinch and Safari rendering, which aligns well with the PR's goals. However, the current implementation inImageViewer.tsxdoesn't fully leverage this library and creates conflicts with existing custom transform code (see comments in that file).Based on learnings.
| <TransformWrapper | ||
| initialScale={1} | ||
| minScale={0.1} | ||
| maxScale={8} | ||
| centerOnInit={true} | ||
| limitToBounds={false} | ||
| > | ||
| <img | ||
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | ||
| alt={alt} | ||
| draggable={false} | ||
| className="h-full w-full object-contain select-none" | ||
| onError={(e) => { | ||
| const img = e.target as HTMLImageElement; | ||
| img.onerror = null; | ||
| img.src = '/placeholder.svg'; | ||
| <TransformComponent | ||
| wrapperStyle={{ | ||
| width: '100%', | ||
| height: '100%', | ||
| overflow: 'visible', | ||
| }} | ||
| style={{ | ||
| transform: `translate(${position.x}px, ${position.y}px) scale(${scale}) rotate(${rotation}deg)`, | ||
| transition: isDragging ? 'none' : 'transform 0.2s ease-in-out', | ||
| cursor: isDragging ? 'grabbing' : 'grab', | ||
| contentStyle={{ | ||
| width: '100%', | ||
| height: '100%', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }} | ||
| /> | ||
| </div> | ||
| > | ||
| <img | ||
| onClick={onClick} | ||
| onMouseDown={onMouseDown} | ||
| onMouseMove={onMouseMove} | ||
| onMouseUp={onMouseUp} | ||
| onMouseLeave={onMouseLeave} | ||
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | ||
| alt={alt} | ||
| draggable={false} | ||
| className="select-none" | ||
| onError={(e) => { | ||
| const img = e.target as HTMLImageElement; | ||
| img.onerror = null; | ||
| img.src = '/placeholder.svg'; | ||
| }} | ||
| style={{ | ||
| maxWidth: '100%', | ||
| maxHeight: '100%', | ||
| objectFit: 'contain', | ||
| // transform: `rotate(${rotation}deg)`, | ||
| zIndex: 50, | ||
| transform: `translate(${position.x}px, ${position.y}px) scale(${scale}) rotate(${rotation}deg)`, | ||
| transition: isDragging ? 'none' : 'transform 0.2s ease-in-out', | ||
| cursor: isDragging ? 'grabbing' : 'grab', | ||
| }} | ||
| /> | ||
| </TransformComponent> | ||
| </TransformWrapper> | ||
| ); |
There was a problem hiding this comment.
Critical: Conflicting transform systems break zoom/pan functionality.
The component uses react-zoom-pan-pinch (TransformWrapper/TransformComponent) but also applies custom transforms via props (scale, position) as inline styles (line 75). These two transform systems conflict with each other:
- react-zoom-pan-pinch manages zoom/pan transforms internally and applies them to the content
- Inline style transform at line 75 uses prop values to override those transforms
When a user scrolls to zoom, the library calculates and applies a transform, but then the inline style immediately overrides it with the prop values. This explains the erratic behavior described in the PR comments (zoom not centering on cursor, issues with large images, etc.).
Additionally, the component receives custom mouse handlers (onMouseDown, onMouseMove, etc.) and isDragging state for manual pan/drag, which duplicates the library's built-in pan functionality and creates further conflicts.
Recommended refactoring approach:
Remove the custom transform logic and let react-zoom-pan-pinch handle all zoom and pan:
interface ImageViewerProps {
imagePath: string;
alt: string;
- scale: number;
- position: { x: number; y: number };
rotation: number;
- isDragging: boolean;
- onMouseDown: (e: React.MouseEvent) => void;
- onMouseMove: (e: React.MouseEvent) => void;
- onMouseUp: () => void;
- onMouseLeave: () => void;
onClick?: (e: React.MouseEvent) => void;
}
export const ImageViewer: React.FC<ImageViewerProps> = ({
imagePath,
alt,
- scale,
- position,
rotation,
- isDragging,
- onMouseDown,
- onMouseMove,
- onMouseUp,
- onMouseLeave,
onClick,
}) => {
return (
<TransformWrapper
initialScale={1}
minScale={0.1}
maxScale={8}
centerOnInit={true}
limitToBounds={false}
+ wheel={{ smoothStep: 0.01 }}
>
<TransformComponent
wrapperStyle={{
width: '100%',
height: '100%',
- overflow: 'visible',
}}
contentStyle={{
width: '100%',
height: '100%',
display: 'flex',
alignItems: 'center',
justifyContent: 'center',
}}
>
<img
onClick={onClick}
- onMouseDown={onMouseDown}
- onMouseMove={onMouseMove}
- onMouseUp={onMouseUp}
- onMouseLeave={onMouseLeave}
src={convertFileSrc(imagePath) || '/placeholder.svg'}
alt={alt}
draggable={false}
className="select-none"
onError={(e) => {
const img = e.target as HTMLImageElement;
img.onerror = null;
img.src = '/placeholder.svg';
}}
style={{
maxWidth: '100%',
maxHeight: '100%',
objectFit: 'contain',
- // transform: `rotate(${rotation}deg)`,
- zIndex: 50,
- transform: `translate(${position.x}px, ${position.y}px) scale(${scale}) rotate(${rotation}deg)`,
- transition: isDragging ? 'none' : 'transform 0.2s ease-in-out',
- cursor: isDragging ? 'grabbing' : 'grab',
+ transform: `rotate(${rotation}deg)`,
+ cursor: 'grab',
}}
/>
</TransformComponent>
</TransformWrapper>
);
};For rotation: If you need dynamic rotation control, use the library's programmatic API via render props or useControls hook to apply rotation in coordination with zoom/pan transforms.
For the parent hook: Update useImageViewControls.ts to remove custom zoom/pan state and handlers since the library manages them internally. If you need to respond to transform changes, use the library's onTransformed callback or useTransformEffect hook.
Would you like me to generate a complete refactored implementation showing how to properly integrate rotation with react-zoom-pan-pinch?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
frontend/src/components/Media/ImageViewer.tsx lines 33-82: the component is
conflicting with react-zoom-pan-pinch because it applies an inline CSS transform
(translate/scale/rotate) from props (position, scale, rotation) while
TransformWrapper/TransformComponent already applies transforms; similarly,
custom mouse handlers and isDragging duplicate library pan logic. Remove the
inline transform and any custom zoom/pan state/handlers so the image element
only receives rotation (if needed) via the library API or combined with the
library transform, and wire rotation through react-zoom-pan-pinch programmatic
API (render props or useControls/useTransformEffect) and onTransformed
callbacks; update useImageViewControls.ts to stop managing scale/position/mouse
events and instead expose hooks/callbacks that call the library controls for
rotation and to observe transform changes.
|
@rahulharpal1603 sir review |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/src/components/Media/MediaView.tsx (1)
142-158: Remove custom drag handler props passed to ImageViewer.The
isDraggingprop (line 147) and mouse handler props (lines 148-151) should be removed from the ImageViewer call, as they conflict with the library's built-in pan functionality (see the critical issue in ImageViewer.tsx review).Apply this diff:
<ImageViewer ref={imageViewerRef} imagePath={currentImagePath} alt={currentImageAlt} rotation={viewState.rotation} - isDragging={viewState.isDragging} - onMouseDown={handlers.handleMouseDown} - onMouseMove={handlers.handleMouseMove} - onMouseUp={handlers.handleMouseUp} - onMouseLeave={handlers.handleMouseUp} onClick={(e) => { if (e.target === e.currentTarget) { handleClose(); } }} resetSignal={resetSignal} />After this change, verify that
useImageViewControlshook no longer needs to manageisDragging,handleMouseDown,handleMouseMove, andhandleMouseUpstate/handlers.frontend/src/components/Media/ImageViewer.tsx (1)
5-16: Critical: Custom drag handlers conflict with react-zoom-pan-pinch's built-in pan functionality.The library provides built-in pan/drag support that works seamlessly with zoom and wheel events. The custom drag handlers (
isDragging,onMouseDown,onMouseMove,onMouseUp,onMouseLeave) attached to the<img>element (lines 79-82) will interfere with the library's pan gestures, causing unpredictable behavior. This likely explains the erratic zoom/pan issues described in the PR comments.Remove the custom drag handlers from the interface and let the library handle all pan interactions:
interface ImageViewerProps { imagePath: string; alt: string; rotation: number; - isDragging: boolean; - onMouseDown: (e: React.MouseEvent) => void; - onMouseMove: (e: React.MouseEvent) => void; - onMouseUp: () => void; - onMouseLeave: () => void; onClick?: (e: React.MouseEvent) => void; resetSignal?: number; }Then remove the handler props from the component signature (lines 30-34) and from the
<img>element (lines 79-82, 99).
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaView.tsx (1)
77-77: Optional: Consider memoizing derived values.
currentImagePathandcurrentImageAltare derived fromcurrentImage, which is already memoized. While not critical (since they'll only recompute whencurrentImagechanges), wrapping them inuseMemowould make the optimization explicit and prevent potential issues if the component structure changes.- const currentImagePath = currentImage.path; - const currentImageAlt = `image-${currentViewIndex}`; + const currentImagePath = useMemo(() => currentImage.path, [currentImage]); + const currentImageAlt = useMemo(() => `image-${currentViewIndex}`, [currentViewIndex]);Also applies to: 100-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/Media/ImageViewer.tsx(1 hunks)frontend/src/components/Media/MediaView.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/Media/MediaView.tsx (1)
frontend/src/components/Media/ImageViewer.tsx (2)
ImageViewerRef(18-22)ImageViewer(24-106)
⏰ 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). (1)
- GitHub Check: Backend Tests
🔇 Additional comments (7)
frontend/src/components/Media/ImageViewer.tsx (4)
18-22: LGTM: Clean imperative API for zoom controls.The ref interface properly exposes zoom controls to the parent component, enabling programmatic zoom management.
49-52: LGTM: Valid pattern for prop-driven reset.The useEffect correctly triggers a transform reset when the
resetSignalprop changes, providing a declarative way for the parent to reset the view.
54-62: LGTM: Appropriate TransformWrapper configuration.The wrapper configuration provides a reasonable zoom range (0.1× to 8×) and
centerOnInitensures proper initial positioning. ThelimitToBounds={false}setting allows free panning, which is appropriate for image viewing.
63-76: Verify thatoverflow: 'visible'is necessary.Setting
overflow: 'visible'on the wrapper (line 67) can interfere with the library's bounds calculations and clipping behavior. This may contribute to the zoom positioning issues described in the PR comments. The library typically expects contained overflow for proper transform calculations.Unless there's a specific requirement for content to overflow the container, consider removing this override and using the library's default overflow handling:
wrapperStyle={{ width: '100%', height: '100%', - overflow: 'visible', }}If overflow is intentional, please test thoroughly with images of varying sizes, especially the large boat image (~3000×4500) mentioned in the PR discussion.
frontend/src/components/Media/MediaView.tsx (3)
1-1: LGTM: Proper ref setup with correct typing.The
ImageViewerRefis correctly imported and the ref is properly typed.Also applies to: 18-18, 39-39
169-177: LGTM: ZoomControls properly wired to new handlers.The zoom controls now correctly invoke the new
handleZoomIn,handleZoomOut, andhandleResetZoomhandlers that operate through the library's imperative API.Note: The
onRotatehandler (line 173) still useshandlers.handleRotate. Verify that rotation works correctly with the library's transform system. As noted in the ImageViewer.tsx review, rotation should ideally be applied to a wrapper element rather than directly on the<img>to avoid transform conflicts.
136-136: LGTM: ClassName typo fixed.The className is now correct with proper spacing and includes the centering classes (
items-center justify-center), addressing the past review comment.Note: The
overflow-visibleclass here, combined withoverflow: 'visible'in ImageViewer's wrapperStyle, may contribute to the zoom positioning issues. See the related concern in the ImageViewer.tsx review.
| const transformRef = useRef<any>(null); | ||
|
|
||
| // Expose zoom functions to parent | ||
| useImperativeHandle(ref, () => ({ | ||
| zoomIn: () => transformRef.current?.zoomIn(), | ||
| zoomOut: () => transformRef.current?.zoomOut(), | ||
| reset: () => transformRef.current?.resetTransform(), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any type with the library's proper ref type.
Using any defeats TypeScript's type safety. The react-zoom-pan-pinch library exports a ReactZoomPanPinchRef type for the TransformWrapper ref.
Apply this diff to use proper typing:
-import React, { useRef, useImperativeHandle, forwardRef } from 'react';
-import { TransformWrapper, TransformComponent } from 'react-zoom-pan-pinch';
+import React, { useRef, useImperativeHandle, forwardRef } from 'react';
+import {
+ TransformWrapper,
+ TransformComponent,
+ ReactZoomPanPinchRef
+} from 'react-zoom-pan-pinch';- const transformRef = useRef<any>(null);
+ const transformRef = useRef<ReactZoomPanPinchRef>(null);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around lines 40 to 47, the
transformRef is typed as any which disables TypeScript safety; import the
ReactZoomPanPinchRef type from 'react-zoom-pan-pinch' and use it as the generic
for useRef (e.g. useRef<ReactZoomPanPinchRef | null>(null)), and ensure any
forwardRef/imperative handle types align with that ref type so
zoomIn/zoomOut/reset calls are properly typed.
| <img | ||
| onClick={onClick} | ||
| onMouseDown={onMouseDown} | ||
| onMouseMove={onMouseMove} | ||
| onMouseUp={onMouseUp} | ||
| onMouseLeave={onMouseLeave} | ||
| src={convertFileSrc(imagePath) || '/placeholder.svg'} | ||
| alt={alt} | ||
| draggable={false} | ||
| className="select-none" | ||
| onError={(e) => { | ||
| const img = e.target as HTMLImageElement; | ||
| img.onerror = null; | ||
| img.src = '/placeholder.svg'; | ||
| }} | ||
| style={{ | ||
| maxWidth: '100%', | ||
| maxHeight: '100%', | ||
| objectFit: 'contain', | ||
| zIndex: 50, | ||
| transform: `rotate(${rotation}deg)`, | ||
| transition: isDragging ? 'none' : 'transform 0.2s ease-in-out', | ||
| cursor: isDragging ? 'grabbing' : 'grab', | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Critical: Rotation transform conflicts with library transforms.
Applying transform: rotate(${rotation}deg) directly on the image (line 97) conflicts with the library's own transform. The library applies zoom/pan transforms to the content, and adding an additional CSS transform creates a composition where transforms combine in unpredictable ways, potentially breaking zoom-to-cursor positioning.
Additionally, the transition and cursor styles (lines 98-99) depend on isDragging, which should be removed along with the custom drag handlers.
Solution: Use the library's programmatic API to compose rotation with zoom/pan transforms. Here's the recommended approach:
<img
onClick={onClick}
- onMouseDown={onMouseDown}
- onMouseMove={onMouseMove}
- onMouseUp={onMouseUp}
- onMouseLeave={onMouseLeave}
src={convertFileSrc(imagePath) || '/placeholder.svg'}
alt={alt}
draggable={false}
className="select-none"
onError={(e) => {
const img = e.target as HTMLImageElement;
img.onerror = null;
img.src = '/placeholder.svg';
}}
style={{
maxWidth: '100%',
maxHeight: '100%',
objectFit: 'contain',
- zIndex: 50,
transform: `rotate(${rotation}deg)`,
- transition: isDragging ? 'none' : 'transform 0.2s ease-in-out',
- cursor: isDragging ? 'grabbing' : 'grab',
}}
/>For rotation, wrap the <img> in a container <div> that applies rotation, and let the library transform the container:
<TransformComponent ...>
<div style={{ transform: `rotate(${rotation}deg)` }}>
<img ... style={{ maxWidth: '100%', maxHeight: '100%', objectFit: 'contain' }} />
</div>
</TransformComponent>This separates rotation (on the wrapper) from zoom/pan (managed by the library), preventing transform conflicts.
🤖 Prompt for AI Agents
In frontend/src/components/Media/ImageViewer.tsx around lines 77 to 101, the
image is being rotated via inline CSS transform and using transition/cursor tied
to isDragging while the library also applies its own zoom/pan transforms, which
causes transform composition bugs; remove the inline transform, transition and
cursor from the <img> element and stop using custom drag handlers/isDragging for
styling, and instead wrap the <img> in a container <div> that you rotate using
style={{ transform: `rotate(${rotation}deg)` }} (keep the image styles like
maxWidth/maxHeight/objectFit on the img) and let the library’s
TransformComponent manage zoom/pan on the rotated wrapper so rotation and
library transforms are composed correctly via the library API.
| const handleZoomIn = useCallback(() => { | ||
| imageViewerRef.current?.zoomIn(); | ||
| }, []); | ||
|
|
||
| const handleZoomOut = useCallback(() => { | ||
| imageViewerRef.current?.zoomOut(); | ||
| }, []); | ||
|
|
||
| const handleResetZoom = useCallback(() => { | ||
| imageViewerRef.current?.reset(); | ||
| handlers.resetZoom(); | ||
| setResetSignal((s) => s + 1); | ||
| }, [handlers]); |
There was a problem hiding this comment.
Redundant reset logic and unclear responsibilities.
handleResetZoom has three reset calls:
- Line 111:
imageViewerRef.current?.reset()- resets the library transform - Line 112:
handlers.resetZoom()- unclear what this resets inuseImageViewControls - Line 113:
setResetSignal((s) => s + 1)- triggers the useEffect in ImageViewer (lines 50-52) which callsresetTransform()again
This results in calling reset twice on the library (lines 111 and via the effect), which is redundant.
Since the library now manages all zoom/pan state, handlers.resetZoom() likely manages obsolete state. Consider removing it:
const handleResetZoom = useCallback(() => {
imageViewerRef.current?.reset();
- handlers.resetZoom();
setResetSignal((s) => s + 1);
}, [handlers]);Or, if you prefer using only the prop-driven reset pattern:
const handleResetZoom = useCallback(() => {
- imageViewerRef.current?.reset();
- handlers.resetZoom();
setResetSignal((s) => s + 1);
-}, [handlers]);
+}, []);Similarly, review whether handlers.resetZoom() calls in handleNextImage (line 51), handlePreviousImage (line 56), and handleThumbnailClick (line 67) should be replaced with setResetSignal.
📝 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.
| const handleZoomIn = useCallback(() => { | |
| imageViewerRef.current?.zoomIn(); | |
| }, []); | |
| const handleZoomOut = useCallback(() => { | |
| imageViewerRef.current?.zoomOut(); | |
| }, []); | |
| const handleResetZoom = useCallback(() => { | |
| imageViewerRef.current?.reset(); | |
| handlers.resetZoom(); | |
| setResetSignal((s) => s + 1); | |
| }, [handlers]); | |
| const handleResetZoom = useCallback(() => { | |
| imageViewerRef.current?.reset(); | |
| setResetSignal((s) => s + 1); | |
| }, [handlers]); |
Removed dragging and click handlers from MediaView component.
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Thanks @Vaibhaviitian
It's my pleasure sir |
2 participants
Adding feature regarding this issue #528 - Add Zoom-on-Scroll Support in Media View
This PR adds zoom in and out on scroll down and up
Wherever mouse is, the zoom or scale factor will be large for that area (as shown in the video)
after adding this feature
https://github.com/user-attachments/assets/6c3b437e-59b2-427b-ad5e-66db844a378c
sir requested to correct the logic so after logic correctiion
Recording.2025-10-07.120644.1.mp4
Summary by CodeRabbit
New Features
Style
Bug Fixes