Fix 905 Added duplicates and best shot feature #967
Conversation
📝 WalkthroughWalkthroughThis PR implements duplicate image detection and "best shot" selection. It adds perceptual hashing to the database layer, introduces image similarity analysis using ORB features and dHash clustering in a new utility module, and provides REST API endpoints and a comprehensive UI for viewing duplicate image groups and deleting selected duplicates. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend UI
participant API as Backend API
participant Detector as Image Processor
participant DB as Database
participant FS as File System
rect rgb(100, 150, 200)
Note over User,FS: Duplicate Detection & Retrieval Flow
User->>Frontend: Request duplicates
Frontend->>API: GET /duplicates/
API->>DB: db_get_all_images_with_phash()
DB-->>API: Images with phash
API->>Detector: group_similar_images(images)
Detector->>Detector: Compute dHash for each<br/>Estimate sharpness<br/>Cluster by hash proximity
Detector->>Detector: Geometric verification<br/>of similar pairs
Detector->>Detector: identify_best_shot()<br/>per group
Detector-->>API: Grouped images + best_shot_id
API-->>Frontend: Duplicate groups
Frontend-->>User: Display groups with thumbnails
end
rect rgb(150, 100, 100)
Note over User,FS: Deletion Flow
User->>Frontend: Select images & confirm delete
Frontend->>API: POST /duplicates/delete<br/>(image_ids)
API->>DB: db_get_images_by_ids(ids)
DB-->>API: Image records with paths
API->>DB: Delete records
API->>FS: Delete image files<br/>& thumbnails
FS-->>API: File deletion complete
API-->>Frontend: {deleted_count, deleted_files_count}
Frontend->>Frontend: Refresh duplicate groups
Frontend-->>User: Updated view
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 6
🤖 Fix all issues with AI Agents
In @backend/app/routes/duplicates.py:
- Around line 70-72: The handler in delete_duplicates currently logs the
exception message and returns str(e) in the HTTPException detail, which may leak
sensitive internals; change the code to log the full exception internally (use
logger.error(..., exc_info=True) or similar) and raise
HTTPException(status_code=500, detail="Internal server error") or another
generic, non-sensitive message instead of str(e), keeping the logger call
referencing the delete_duplicates context and the raised HTTPException for the
same error path.
In @backend/app/utils/images.py:
- Around line 22-24: There are two logger definitions: logger =
get_logger(__name__) and later logger = logging.getLogger(__name__), which
shadows the custom get_logger; remove the redundant logging.getLogger assignment
so the module consistently uses logger from get_logger(__name__) (ensure any
imports still used like get_logger remain and that references to logger
elsewhere in the file use the single definition).
In @backend/requirements.txt:
- Line 74: Update the ImageHash dependency line in requirements.txt from
"ImageHash==4.3.1" to "ImageHash==4.3.2": locate the ImageHash entry (the
"ImageHash==4.3.1" token) and change the pinned version to 4.3.2, then run your
dependency install/lock workflow (pip install -r requirements.txt or regenerate
lockfile) to ensure the new version is applied.
In @frontend/src/pages/DuplicatePage/DuplicatePage.tsx:
- Around line 200-202: The filename extraction using img.path.split('/').pop()
is brittle on Windows; change it to a cross-platform splitter or utility (e.g.,
replace the split with img.path.split(/[/\\]/).pop() or create a
getFilename(path) helper used where img.path is rendered at the two spots in
DuplicatePage) so backslashes are handled correctly and both locations (the
img.path usage around the thumbnail captions) use the same robust function.
- Around line 97-116: The effect uses navigateImage but doesn't include it in
the dependency array; wrap navigateImage in useCallback (so it preserves
identity unless its own dependencies change) and then add navigateImage to the
useEffect dependency array alongside viewingImage and groups; ensure the
useCallback's dependency list includes any values navigateImage uses (e.g.,
viewingImage, groups, setViewingImage) to avoid stale closures.
- Around line 208-262: The image preview modal JSX (the block that checks
viewingImage and renders the overlay, including handlers handlePrev/handleNext
and the close button that calls setViewingImage) is currently inside the
groups.map() callback and is therefore duplicated and nested incorrectly; move
that entire modal block so it lives once at the component root (outside the
groups.map), remove the stray closing </div> that currently closes the group
container incorrectly, and ensure the modal still references viewingImage,
convertFileSrc(viewingImage.path), handlePrev, handleNext and setViewingImage so
its behavior is unchanged.
🧹 Nitpick comments (6)
frontend/src/pages/DuplicatePage/DuplicatePage.tsx (1)
8-13: Consider reusing the existingImageinterface.A similar
Imageinterface exists infrontend/src/types/Media.ts. Consider extending or reusing it to maintain type consistency across the codebase, adding thephashfield if needed.🔎 Proposed approach
import { Image as BaseImage } from '@/types/Media'; interface DuplicateImage extends Pick<BaseImage, 'id' | 'path' | 'thumbnailPath'> { phash: string; }docs/backend/backend_python/openapi.json (1)
1334-1382: Consider adding maxItems to the request body array.The request accepts an unbounded array of image IDs. While this is a low-risk internal API, adding a reasonable
maxItemslimit (e.g., 1000) would provide some protection against accidental or malicious large payloads.backend/app/routes/duplicates.py (1)
25-25: Hardcoded threshold differs from module constant.The threshold is hardcoded as
5here, butHASH_THRESHOLDinduplicate_detector.pyis8. Consider using the constant for consistency, or document why a different value is intentional.🔎 Proposed fix
+from app.utils.duplicate_detector import identify_best_shot, group_similar_images, HASH_THRESHOLD ... - groups = group_similar_images(all_images, threshold=5) + groups = group_similar_images(all_images, threshold=HASH_THRESHOLD)Or if
5is intentional for stricter matching, add a comment explaining the choice.backend/app/utils/duplicate_detector.py (3)
1-10: Unused import:json.The
jsonmodule is imported but never used in this file.🔎 Proposed fix
import imagehash from PIL import Image import os -import json import cv2
80-91: Use context manager for PIL Image.The image is opened but not explicitly closed, which could lead to resource leaks when processing many images.
🔎 Proposed fix
def calculate_phash(image_path: str) -> Optional[str]: """ Calculate perceptual hash for an image. """ try: - img = Image.open(image_path) - # phash is generally good for finding duplicates including resized/compressed ones - hash_obj = imagehash.phash(img) - return str(hash_obj) + with Image.open(image_path) as img: + # phash is generally good for finding duplicates including resized/compressed ones + hash_obj = imagehash.phash(img) + return str(hash_obj) except Exception as e: logger.error(f"Error calculating pHash for {image_path}: {e}") return None
124-145: Clean up confusing comments and use context manager for PIL Image.The comments at lines 125-130 discuss whether to use pHash or dHash but don't reach a clear conclusion. The code then computes dHash regardless. Additionally, the PIL Image should use a context manager.
🔎 Proposed fix
for img in images: - if img.get('phash'): # We are technically using the pHash from DB if available, or calculating on fly - # If we want to switch to dHash strictly we might need to re-compute. - # For now let's reuse the stored hash as a first pass filter if possible, - # OR strictly compute dHash now for better burst mode detection. - # Given the context, let's calculate dHash on fly for high accuracy as requested. - pass - path = img.get('path') if not path or not os.path.exists(path): continue try: - # Calculate dHash on the fly for heavy logic mode - pil_img = Image.open(path) - dhash = imagehash.dhash(pil_img, hash_size=HASH_SIZE) - img['hash_obj'] = dhash - # Compute sharpness now to save time later - img['sharpness_score'] = get_image_sharpness(path) - processed_images.append(img) + # Calculate dHash on-the-fly for burst mode detection accuracy + with Image.open(path) as pil_img: + dhash = imagehash.dhash(pil_img, hash_size=HASH_SIZE) + img['hash_obj'] = dhash + # Compute sharpness now to save time later + img['sharpness_score'] = get_image_sharpness(path) + processed_images.append(img) except Exception as e: logger.warning(f"Error processing image {path}: {e}")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/app/database/images.pybackend/app/routes/duplicates.pybackend/app/utils/duplicate_detector.pybackend/app/utils/images.pybackend/main.pybackend/requirements.txtdocs/backend/backend_python/openapi.jsonfrontend/src/api/apiEndpoints.tsfrontend/src/components/Navigation/Sidebar/AppSidebar.tsxfrontend/src/constants/routes.tsfrontend/src/pages/DuplicatePage/DuplicatePage.tsxfrontend/src/routes/AppRoutes.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/src/pages/DuplicatePage/DuplicatePage.tsx (2)
frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/api/apiEndpoints.ts (1)
duplicatesEndpoints(34-37)
frontend/src/routes/AppRoutes.tsx (2)
frontend/src/constants/routes.ts (1)
ROUTES(1-13)frontend/src/pages/DuplicatePage/DuplicatePage.tsx (1)
DuplicatePage(20-266)
backend/app/utils/duplicate_detector.py (1)
frontend/src/types/Media.ts (1)
Image(13-23)
backend/app/routes/duplicates.py (2)
backend/app/database/images.py (3)
db_get_all_images_with_phash(424-457)db_delete_images_by_ids(367-399)db_get_images_by_ids(459-472)backend/app/utils/duplicate_detector.py (2)
identify_best_shot(93-114)group_similar_images(116-185)
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
frontend/src/constants/routes.ts (1)
ROUTES(1-13)
backend/app/utils/images.py (1)
backend/app/utils/duplicate_detector.py (1)
calculate_phash(80-91)
🪛 Checkov (3.2.334)
docs/backend/backend_python/openapi.json
[medium] 1321-1327: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (14)
backend/main.py (1)
28-28: LGTM! Duplicates router properly integrated.The duplicates router is correctly imported and registered following the established pattern of other routers in the application.
Also applies to: 134-134
frontend/src/routes/AppRoutes.tsx (1)
12-12: LGTM! Duplicates route properly configured.The new duplicates route is correctly implemented following the existing routing pattern, properly nested within the Layout wrapper for authenticated pages.
Also applies to: 27-27
frontend/src/constants/routes.ts (1)
12-12: LGTM! Route constant properly defined.The DUPLICATES route constant follows the established naming convention and is properly integrated into the ROUTES object.
frontend/src/components/Navigation/Sidebar/AppSidebar.tsx (1)
19-19: LGTM! Sidebar menu item properly implemented.The Duplicates menu item is correctly added with appropriate icon (Copy from lucide-react) and follows the established pattern for sidebar navigation items.
Also applies to: 56-56
frontend/src/api/apiEndpoints.ts (1)
33-37: LGTM!The new
duplicatesEndpointsobject follows the established pattern and aligns with the backend API routes.backend/app/utils/images.py (1)
169-179: LGTM!The phash integration is well-placed after successful thumbnail generation. The nullable
phashfield handles cases where hash computation fails gracefully.backend/app/database/images.py (5)
30-30: LGTM!The
phashfield is correctly typed asUnion[str, None]to match the nullable database column.
68-68: Verify migration strategy for existing images.New images will have phash computed during insertion, but existing images in the database will have
phash = NULL. The duplicates feature will only detect duplicates among images with non-null phash values.Consider adding a migration or background task to compute phash for existing images, or document this limitation.
102-112: LGTM!The INSERT statement correctly includes phash, and the ON CONFLICT clause properly updates the phash value during upserts while preserving the conditional isTagged logic.
424-457: LGTM!The function correctly retrieves all images with non-null phash values, has proper error handling, and ensures connection cleanup in the
finallyblock.
459-472: LGTM!The function follows established patterns with parameterized queries, proper error handling, and connection cleanup.
docs/backend/backend_python/openapi.json (1)
1308-1332: LGTM!The GET /duplicates/ endpoint is correctly documented. The response schema could benefit from a more detailed definition (like other endpoints use
$refto named schemas), but the current auto-generated schema is functional.backend/app/routes/duplicates.py (1)
11-39: LGTM for the endpoint structure.The endpoint correctly fetches images, groups them, identifies best shots, and returns structured results with proper error handling.
backend/app/utils/duplicate_detector.py (1)
116-185: Algorithm is well-designed with appropriate safeguards.The greedy clustering approach with hash-based fast filtering and ORB verification for accuracy is a good balance. The sharpness pre-sort ensures high-quality images become cluster representatives. Error handling and cleanup of temporary attributes are properly implemented.
Note: For very large image collections, the O(n × clusters × ORB_cost) complexity could become slow. Consider adding a comment about expected performance characteristics or adding batch size limits if this becomes an issue in production.
| except Exception as e: | ||
| logger.error(f"Error in delete_duplicates: {e}") | ||
| raise HTTPException(status_code=500, detail=str(e)) |
There was a problem hiding this comment.
Avoid exposing raw exception details in API response.
Using str(e) in the error detail could expose internal paths, database errors, or other sensitive information to API consumers.
🔎 Proposed fix
except Exception as e:
logger.error(f"Error in delete_duplicates: {e}")
- raise HTTPException(status_code=500, detail=str(e))
+ raise HTTPException(status_code=500, detail="Failed to delete images")📝 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.
| except Exception as e: | |
| logger.error(f"Error in delete_duplicates: {e}") | |
| raise HTTPException(status_code=500, detail=str(e)) | |
| except Exception as e: | |
| logger.error(f"Error in delete_duplicates: {e}") | |
| raise HTTPException(status_code=500, detail="Failed to delete images") |
🤖 Prompt for AI Agents
In @backend/app/routes/duplicates.py around lines 70 - 72, The handler in
delete_duplicates currently logs the exception message and returns str(e) in the
HTTPException detail, which may leak sensitive internals; change the code to log
the full exception internally (use logger.error(..., exc_info=True) or similar)
and raise HTTPException(status_code=500, detail="Internal server error") or
another generic, non-sensitive message instead of str(e), keeping the logger
call referencing the delete_duplicates context and the raised HTTPException for
the same error path.
| from app.utils.duplicate_detector import calculate_phash | ||
|
|
||
| logger = get_logger(__name__) |
There was a problem hiding this comment.
Duplicate logger definition detected.
Line 24 defines logger = get_logger(__name__), but line 30 redefines it as logger = logging.getLogger(__name__). This shadows the first definition and may cause inconsistent logging behavior if get_logger provides custom formatting or handlers.
🔎 Proposed fix
Remove the duplicate definition at line 30:
logger = get_logger(__name__)
# GPS EXIF tag constant
GPS_INFO_TAG = 34853
-logger = logging.getLogger(__name__)
-Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @backend/app/utils/images.py around lines 22 - 24, There are two logger
definitions: logger = get_logger(__name__) and later logger =
logging.getLogger(__name__), which shadows the custom get_logger; remove the
redundant logging.getLogger assignment so the module consistently uses logger
from get_logger(__name__) (ensure any imports still used like get_logger remain
and that references to logger elsewhere in the file use the single definition).
| psutil>=5.9.5 | ||
| pytest-asyncio>=1.0.0 | ||
| setuptools==66.1.1 | ||
| ImageHash==4.3.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check ImageHash package version information and security advisories
echo "=== Checking available ImageHash versions (last 10) ==="
curl -s https://pypi.org/pypi/ImageHash/json | jq -r '.releases | keys[]' | sort -V | tail -10
echo -e "\n=== Latest version ==="
curl -s https://pypi.org/pypi/ImageHash/json | jq -r '.info.version'
echo -e "\n=== Release date of version 4.3.1 ==="
curl -s https://pypi.org/pypi/ImageHash/4.3.1/json | jq -r '.releases' 2>/dev/null || curl -s https://pypi.org/pypi/ImageHash/json | jq -r '.releases."4.3.1"[0].upload_time' 2>/dev/null
echo -e "\n=== Check if version 4.3.1 exists ==="
curl -s https://pypi.org/pypi/ImageHash/4.3.1/json | jq -r '.info.version' 2>/dev/nullRepository: AOSSIE-Org/PictoPy
Length of output: 288
🌐 Web query:
ImageHash security vulnerabilities CVE
💡 Result:
Short answer: There are no public CVEs that specifically name the Python ImageHash package. Snyk’s vulnerability DB lists no direct vulnerabilities for ImageHash, and PyPI shows the project but no CVE entries. [1][2]
Context / related notes:
- Image-processing libraries commonly do have CVEs (e.g., ImageMagick and others), so risks often come from image decoders or native libs you use alongside ImageHash (not ImageHash itself). Examples: ImageMagick CVEs tracked by Ubuntu, and a recent stb_image CVE. [3][4]
If you want, I can:
- search more exhaustively (NVD, GitHub advisories, OSS‑DB) and return any CVEs that mention ImageHash by name; or
- scan a project that uses ImageHash for known-vulnerable dependencies. Which would you prefer?
🌐 Web query:
ImageHash package security advisories
💡 Result:
Short answer: The ImageHash project itself has no published security advisories; its GitHub security page shows no advisories and no SECURITY.md. However, ImageHash depends on image libraries (notably Pillow), which have had several CVEs (e.g., CVE-2022-22817, CVE-2021-28675). You should track Pillow (and other imaging deps) for security fixes and keep ImageHash and its dependencies up to date. [1][2][3][4]
Sources:
- ImageHash GitHub security overview (no advisories). [1]
- Snyk package page for ImageHash (no direct vulnerabilities recorded). [2]
- CVE-2022-22817 (Pillow ImageMath.eval arbitrary expression). [3]
- CVE-2021-28675 (Pillow PSD plugin issue). [4]
Update ImageHash to version 4.3.2 (latest available).
Version 4.3.1 is valid and free of known vulnerabilities, but version 4.3.2 is now available. ImageHash itself has no published CVEs; however, its dependency on Pillow means security updates to image libraries should be monitored and kept current.
🤖 Prompt for AI Agents
In @backend/requirements.txt at line 74, Update the ImageHash dependency line in
requirements.txt from "ImageHash==4.3.1" to "ImageHash==4.3.2": locate the
ImageHash entry (the "ImageHash==4.3.1" token) and change the pinned version to
4.3.2, then run your dependency install/lock workflow (pip install -r
requirements.txt or regenerate lockfile) to ensure the new version is applied.
| useEffect(() => { | ||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (!viewingImage) return; | ||
|
|
||
| switch (e.key) { | ||
| case 'ArrowLeft': | ||
| navigateImage('prev'); | ||
| break; | ||
| case 'ArrowRight': | ||
| navigateImage('next'); | ||
| break; | ||
| case 'Escape': | ||
| setViewingImage(null); | ||
| break; | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('keydown', handleKeyDown); | ||
| return () => window.removeEventListener('keydown', handleKeyDown); | ||
| }, [viewingImage, groups]); |
There was a problem hiding this comment.
navigateImage should be in the useEffect dependency array.
The navigateImage function is called inside the effect but not listed as a dependency. This can cause stale closure issues where viewingImage or groups values become outdated.
🔎 Proposed fix
Wrap navigateImage in useCallback and add it to dependencies:
+import React, { useEffect, useState, useCallback } from 'react';
...
- const navigateImage = (direction: 'prev' | 'next') => {
+ const navigateImage = useCallback((direction: 'prev' | 'next') => {
if (!viewingImage) return;
for (const group of groups) {
const idx = group.images.findIndex(img => img.id === viewingImage.id);
if (idx !== -1) {
let newIdx;
if (direction === 'prev') {
newIdx = idx === 0 ? group.images.length - 1 : idx - 1;
} else {
newIdx = (idx + 1) % group.images.length;
}
setViewingImage(group.images[newIdx]);
return;
}
}
- };
+ }, [viewingImage, groups]);
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
...
};
window.addEventListener('keydown', handleKeyDown);
return () => window.removeEventListener('keydown', handleKeyDown);
- }, [viewingImage, groups]);
+ }, [viewingImage, navigateImage]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @frontend/src/pages/DuplicatePage/DuplicatePage.tsx around lines 97 - 116,
The effect uses navigateImage but doesn't include it in the dependency array;
wrap navigateImage in useCallback (so it preserves identity unless its own
dependencies change) and then add navigateImage to the useEffect dependency
array alongside viewingImage and groups; ensure the useCallback's dependency
list includes any values navigateImage uses (e.g., viewingImage, groups,
setViewingImage) to avoid stale closures.
| <div className="absolute bottom-0 left-0 right-0 bg-black/60 p-2 text-xs text-white truncate"> | ||
| {img.path.split('/').pop()} | ||
| </div> |
There was a problem hiding this comment.
Path splitting may fail on Windows.
Using path.split('/').pop() assumes Unix-style paths. On Windows, paths use backslashes and this will return the entire path string instead of the filename.
🔎 Proposed fix
Use a cross-platform approach:
- {img.path.split('/').pop()}
+ {img.path.split(/[\\/]/).pop()}Or extract to a utility function for reuse at lines 201 and 240.
📝 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 className="absolute bottom-0 left-0 right-0 bg-black/60 p-2 text-xs text-white truncate"> | |
| {img.path.split('/').pop()} | |
| </div> | |
| <div className="absolute bottom-0 left-0 right-0 bg-black/60 p-2 text-xs text-white truncate"> | |
| {img.path.split(/[\\/]/).pop()} | |
| </div> |
🤖 Prompt for AI Agents
In @frontend/src/pages/DuplicatePage/DuplicatePage.tsx around lines 200 - 202,
The filename extraction using img.path.split('/').pop() is brittle on Windows;
change it to a cross-platform splitter or utility (e.g., replace the split with
img.path.split(/[/\\]/).pop() or create a getFilename(path) helper used where
img.path is rendered at the two spots in DuplicatePage) so backslashes are
handled correctly and both locations (the img.path usage around the thumbnail
captions) use the same robust function.
| {/* Image Preview Modal */} | ||
| {viewingImage && ( | ||
| <div | ||
| className="fixed inset-0 z-50 flex items-center justify-center bg-black/95 p-4 animate-in fade-in duration-200" | ||
| onClick={() => setViewingImage(null)} | ||
| > | ||
| <div className="relative max-w-[95vw] max-h-[95vh] flex flex-col items-center justify-center" onClick={(e) => e.stopPropagation()}> | ||
| <div className="absolute top-2 left-2 z-50 bg-black/60 text-white px-3 py-1 rounded-full text-sm font-medium backdrop-blur-sm shadow-lg pointer-events-none"> | ||
| {(() => { | ||
| const group = groups.find(g => g.images.some(i => i.id === viewingImage.id)); | ||
| if (!group) return ''; | ||
| const index = group.images.findIndex(i => i.id === viewingImage.id) + 1; | ||
| return `${index} / ${group.images.length}`; | ||
| })()} | ||
| </div> | ||
|
|
||
| {/* Navigation Buttons */} | ||
| <button | ||
| className="absolute left-2 md:-left-16 top-1/2 -translate-y-1/2 text-white hover:text-gray-300 transition-colors bg-white/10 hover:bg-white/20 p-2 rounded-full z-10" | ||
| onClick={handlePrev} | ||
| title="Previous Image" | ||
| > | ||
| <ChevronLeft size={32} /> | ||
| </button> | ||
|
|
||
| <img | ||
| src={convertFileSrc(viewingImage.path)} | ||
| className="max-w-full max-h-[85vh] object-contain rounded-md shadow-2xl" | ||
| alt="Preview" | ||
| /> | ||
|
|
||
| <div className="mt-4 text-white text-center bg-black/60 px-4 py-2 rounded-full backdrop-blur-sm"> | ||
| <p className="font-medium truncate max-w-2xl">{viewingImage.path.split('/').pop()}</p> | ||
| <p className="text-xs text-gray-300 truncate max-w-2xl">{viewingImage.path}</p> | ||
| </div> | ||
|
|
||
| <button | ||
| className="absolute right-2 md:-right-16 top-1/2 -translate-y-1/2 text-white hover:text-gray-300 transition-colors bg-white/10 hover:bg-white/20 p-2 rounded-full z-10" | ||
| onClick={handleNext} | ||
| title="Next Image" | ||
| > | ||
| <ChevronRight size={32} /> | ||
| </button> | ||
|
|
||
| <button | ||
| className="absolute -top-12 right-0 md:-right-12 md:-top-0 text-white hover:text-gray-300 transition-colors bg-white/10 hover:bg-white/20 p-2 rounded-full" | ||
| onClick={() => setViewingImage(null)} | ||
| > | ||
| <X size={32} /> | ||
| </button> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
Modal is rendered inside the map loop causing incorrect DOM structure.
The image preview modal (lines 208-260) is rendered inside the groups.map() callback, which means:
- The modal is nested inside each group's
<div>instead of at the component root - It will be rendered multiple times (once per group)
- The closing
</div>at line 261 closes the group container, not the space container
🔎 Proposed fix
Move the modal outside the map loop to render it once at the component root:
<div className="space-y-8">
{groups.map((group, index) => (
<div key={index} className="bg-gray-800/50 p-4 rounded-lg border border-gray-700">
...
<div className="grid grid-cols-2 md:grid-cols-3 lg:grid-cols-4 gap-4">
{group.images.map((img) => {
...
})}
</div>
-
- {/* Image Preview Modal */}
- {viewingImage && (
- <div ...>
- ...
- </div>
- )}
</div>
))}
</div>
+
+ {/* Image Preview Modal */}
+ {viewingImage && (
+ <div
+ className="fixed inset-0 z-50 flex items-center justify-center bg-black/95 p-4 animate-in fade-in duration-200"
+ onClick={() => setViewingImage(null)}
+ >
+ ...
+ </div>
+ )}
</div>
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @frontend/src/pages/DuplicatePage/DuplicatePage.tsx around lines 208 - 262,
The image preview modal JSX (the block that checks viewingImage and renders the
overlay, including handlers handlePrev/handleNext and the close button that
calls setViewingImage) is currently inside the groups.map() callback and is
therefore duplicated and nested incorrectly; move that entire modal block so it
lives once at the component root (outside the groups.map), remove the stray
closing </div> that currently closes the group container incorrectly, and ensure
the modal still references viewingImage, convertFileSrc(viewingImage.path),
handlePrev, handleNext and setViewingImage so its behavior is unchanged.
1 participant
Fixes #905
Relevant Changes
Additional changes
Video - https://res.cloudinary.com/db6cpegxg/video/upload/v1767806671/Screen_Recording_2026-01-07_at_10.48.35_PM_bfxczl.mov
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.