Right-Click Context Menu Actions for Images *(Favorite, Copy, View Details)* #900
Conversation
|
|
📝 WalkthroughWalkthroughAdds a Radix-based context menu UI and right-click actions to image cards (including copy-to-clipboard), a Tauri command to copy images to the system clipboard via arboard, a MediaInfoPanel modal flow in Home, and small UI/type updates (InfoDialog success variant, MediaInfoPanel className). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageCard
participant ContextMenu
participant Frontend as UI
participant TauriBackend as Tauri
participant Arboard
User->>ImageCard: Right-click image
ImageCard->>ContextMenu: open()
User->>ContextMenu: Select "Copy image"
ContextMenu->>Frontend: invoke('copy_image_to_clipboard', path)
Frontend->>Tauri: forward invoke
Tauri->>Arboard: load file, convert RGBA, write clipboard
alt write success
Arboard-->>Tauri: Ok
Tauri-->>Frontend: Ok
Frontend->>UI: showInfoDialog('success')
UI->>User: Success notification
else write error
Arboard-->>Tauri: Err(msg)
Tauri-->>Frontend: Err(msg)
Frontend->>UI: showInfoDialog('error')
UI->>User: Error notification
end
sequenceDiagram
participant User
participant ImageCard
participant ChronologicalGallery
participant Home
participant MediaInfoPanel
User->>ImageCard: Click "View Info" (or context action)
ImageCard->>ChronologicalGallery: onViewInfo(image, index)
ChronologicalGallery->>Home: forward onViewInfo(image, index)
Home->>Home: handleOpenInfo (set state)
Home->>MediaInfoPanel: render modal with image/index
MediaInfoPanel->>User: display details
User->>MediaInfoPanel: Close
MediaInfoPanel->>Home: onClose → handleCloseInfo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 5
🧹 Nitpick comments (2)
frontend/src/components/Media/ImageCard.tsx (2)
64-100: Consider wrapping handlers inuseCallbackfor consistency.
handleToggleFavouriteusesuseCallback, buthandleCopyandhandleViewInfodo not. While these handlers are used only in local event handlers (so performance impact is minimal), usinguseCallbackconsistently improves code predictability and would matter if these handlers were ever passed to memoized children.🔎 Suggested refactor
- const handleCopy = async () => { + const handleCopy = useCallback(async () => { try { await invoke('copy_image_to_clipboard', { path: image.path, }); dispatch( showInfoDialog({ title: 'Success', message: 'Image copied to clipboard', variant: 'success', }), ); } catch (err) { console.error(err); dispatch( showInfoDialog({ title: 'Error', message: 'Failed to copy image to clipboard', variant: 'error', }), ); } - }; + }, [dispatch, image.path]); - const handleViewInfo = () => { + const handleViewInfo = useCallback(() => { if (onViewInfo) { onViewInfo(image, imageIndex); } else if (onClick) { onClick(); } - }; + }, [onViewInfo, image, imageIndex, onClick]);
36-47: PropsonViewInfoandimageIndexare correctly paired in all current usages.Verification shows that every usage of
ImageCardwith theonViewInfoprop also providesimageIndexexplicitly. InChronologicalGallery, both props are consistently passed together with the correct index value. While the prop signature allows them to be used independently—which could theoretically lead toonViewInforeceiving a defaultimageIndexof0—this pattern does not occur in practice.The suggestions to enforce prop pairing (e.g., via TypeScript discriminated unions or runtime warnings) remain valid as preventative design measures, but are not currently necessary given how the component is used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
frontend/package.jsonfrontend/src-tauri/Cargo.tomlfrontend/src-tauri/src/main.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src/components/Dialog/InfoDialog.tsxfrontend/src/components/Media/ChronologicalGallery.tsxfrontend/src/components/Media/ImageCard.tsxfrontend/src/components/Media/MediaInfoPanel.tsxfrontend/src/components/ui/context-menu.tsxfrontend/src/pages/Home/Home.tsxfrontend/src/types/infoDialog.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
frontend/src/components/Media/MediaInfoPanel.tsx (2)
frontend/src/types/Media.ts (1)
Image(13-23)frontend/src/lib/utils.ts (1)
cn(5-7)
frontend/src/components/Media/ChronologicalGallery.tsx (1)
frontend/src/types/Media.ts (1)
Image(13-23)
🔇 Additional comments (13)
frontend/src/components/Media/MediaInfoPanel.tsx (2)
13-13: LGTM! Good extensibility pattern.Adding the optional
classNameprop and applying it via thecnutility enables external styling while maintaining default classes. This is a clean implementation that follows React best practices.Also applies to: 21-21, 30-30, 67-67
111-112: AI summary inconsistency detected.The AI summary claims "Changes the Location button rendering to rely solely on the presence of longitude," but the code still checks both
latitudeandlongitudeon lines 111-112. The condition remainscurrentImage?.metadata?.latitude && currentImage?.metadata?.longitude, unchanged from the original logic.frontend/src/components/ui/context-menu.tsx (1)
1-199: LGTM! Well-structured context menu implementation.This is a clean, well-structured wrapper around Radix UI Context Menu primitives that follows best practices:
- Proper use of
forwardReffor all interactive components- Consistent
displayNameassignments for debugging- Type-safe prop spreading with TypeScript
- Appropriate use of the
cnutility for className composition- Support for advanced features (submenus, checkboxes, radio items, shortcuts)
- Accessible markup with proper focus states and data attributes
The implementation aligns with the shadcn/ui component pattern and integrates well with the Tailwind styling system.
frontend/package.json (1)
28-28: @radix-ui/[email protected] is compatible with React 19. The package was part of a Radix Primitives release (June 19, 2024) that added official React 19 support. Note: Some console warnings related to React 19 deprecation (element.ref issues) may appear in certain setups, but these are not blockers.frontend/src-tauri/Cargo.toml (1)
19-20: Dependencies are valid and safe.Verification confirms:
arboard3.4.0 and 3.4.1 are available on crates.ioimage0.25 has versions 0.25.5–0.25.9 available- No active security advisories (RustSec advisories RUSTSEC-2019-0014 and RUSTSEC-2020-0073 are patched in 0.25+)
- Code uses only stable APIs (
image::open()andto_rgba8()) not affected by the breaking changes in 0.25frontend/src/types/infoDialog.ts (1)
1-1: LGTM!The addition of the 'success' variant to the InfoDialogVariant union is appropriate for providing positive feedback to users (e.g., successful copy-to-clipboard operations).
frontend/src/pages/Home/Home.tsx (2)
27-30: LGTM!The modal state management is clean and follows React best practices. The separation of concerns (open state, current image, and index) is appropriate for this use case.
60-69: LGTM!The modal handlers are well-structured. The cleanup in
handleCloseInfoproperly resets all modal-related state.frontend/src-tauri/src/main.rs (1)
55-58: LGTM with dependencies on path validation fix.The command registration correctly includes both the existing
get_server_pathand the newcopy_image_to_clipboardcommand. Once the path validation is implemented in the command function, this registration is complete.frontend/src/components/Media/ChronologicalGallery.tsx (2)
23-23: LGTM!The addition of the
onViewInfocallback prop is well-typed and follows the existing prop pattern. The signature(image: Image, index: number) => voidprovides all necessary context for the consumer to display image details.
171-172: LGTM!The prop threading is correct. Both
imageIndex(computed from theimageIndexMap) andonViewInfoare properly passed toImageCard, enabling the per-image context menu functionality.frontend/src/components/Media/ImageCard.tsx (2)
102-169: LGTM!The context menu integration is well-structured. The
ContextMenuTriggercorrectly wraps the card, hover state management is preserved, ande.stopPropagation()on the favorite button prevents unintended card clicks.
171-200: LGTM!Context menu actions are well-implemented with proper event propagation handling. The dynamic favorite/unfavorite text and icon styling provide good user feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src-tauri/src/main.rs (1)
29-37: LGTM! Size limit effectively prevents memory exhaustion.The maximum pixel check (30M pixels ≈ 120MB RGBA) prevents excessive memory usage from very large images while allowing typical photos and 4K images. The limit is well-documented and the error message is clear.
Consider making the limit configurable for different deployment environments:
// At file/module level const DEFAULT_MAX_PIXELS: u32 = 30_000_000; // In function, could read from config or use default let max_pixels = DEFAULT_MAX_PIXELS; // Could be read from app config if width * height > max_pixels { return Err(format!( "Image too large to copy to clipboard ({}x{} = {} pixels, max: {})", width, height, width * height, max_pixels ).into()); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/src-tauri/src/main.rsfrontend/src-tauri/tauri.conf.jsonfrontend/src/components/Dialog/InfoDialog.tsxfrontend/src/pages/Home/Home.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/components/Dialog/InfoDialog.tsx
- frontend/src-tauri/tauri.conf.json
- frontend/src/pages/Home/Home.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src-tauri/src/main.rs (2)
backend/app/routes/images.py (1)
ImageData(29-37)frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
🔇 Additional comments (3)
frontend/src-tauri/src/main.rs (3)
9-16: LGTM! Imports are well-organized.The clipboard-related imports are clearly grouped and all necessary for the command implementation. The organization with comment headers improves code readability.
39-53: LGTM! Clipboard operation is correctly implemented.The image conversion to RGBA8, ImageData construction, and clipboard operation are properly implemented with appropriate error handling. The use of
Cow::Ownedis correct for the owned RGBA data, andarboardhandles cross-platform clipboard differences.
76-79: LGTM! Command registration is correct.The new
copy_image_to_clipboardcommand is properly registered alongside the existingget_server_pathcommand using Tauri'sgenerate_handler!macro.
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JaYRaNa213. * #900 (comment) The following files were modified: * `frontend/src-tauri/src/main.rs` * `frontend/src/components/Media/ImageCard.tsx`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src-tauri/src/main.rs (2)
9-10: Remove duplicate comment line.The comment "// -------- Clipboard imports --------" appears twice consecutively.
🔎 Proposed fix
-// -------- Clipboard imports -------- // -------- Clipboard imports -------- use tauri::command;
19-65: Add defense-in-depth: canonicalize paths and validate against allowed directories.The current security checks (absolute path requirement, parent dir rejection, file existence validation) provide basic protection against obvious traversal attacks. However, they don't prevent symlink-based attacks.
The gap:
- Symlinks are not resolved: A path like
/home/user/images/link.jpgcan pass all current checks (absolute, no.., exists, is_file) but may be a symlink to/etc/passwdor other sensitive files outside the intended directory.Recommended additions:
- Use
path.canonicalize()to resolve symlinks before validation- Validate the canonicalized path is within allowed base directories (e.g., the app's data directory or user's image library folder)
Enhanced security approach
#[command] -fn copy_image_to_clipboard(path: String) -> Result<(), String> { +fn copy_image_to_clipboard(path: String, app_handle: tauri::AppHandle) -> Result<(), String> { let path = PathBuf::from(&path); // 🔐 Security checks if !path.is_absolute() { return Err("Expected absolute file path".into()); } if path.components().any(|c| matches!(c, std::path::Component::ParentDir)) { return Err("Invalid path traversal detected".into()); } + + // Resolve symlinks and normalize path + let canonical_path = path.canonicalize() + .map_err(|e| format!("Failed to resolve path: {}", e))?; + + // TODO: Define allowed base directories for your app + // Example: validate against app data directory or configured image library path + // let allowed_dirs = vec![ + // app_handle.path().app_data_dir()?, + // PathBuf::from("/path/to/image/library"), + // ]; + // + // if !allowed_dirs.iter().any(|base| canonical_path.starts_with(base)) { + // return Err("Path outside allowed directories".into()); + // } - if !path.exists() { + if !canonical_path.exists() { return Err("File does not exist".into()); } - if !path.is_file() { + if !canonical_path.is_file() { return Err("Path is not a file".into()); } // Load image - let img = image::open(&path) + let img = image::open(&canonical_path) .map_err(|e| format!("Failed to open image: {}", e))?;Note: If you add the
app_handleparameter, update the command invocation in the frontend accordingly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src-tauri/src/main.rs
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src-tauri/src/main.rs (1)
frontend/src-tauri/src/services/mod.rs (1)
get_server_path(5-11)
1 participant
This PR introduces a right-click context menu for image cards in PictoPy, enabling quick and intuitive image actions without affecting existing workflows.
The feature improves usability by allowing users to interact directly with images from the gallery view.
Issue :
Fixes #802
✨ Features Added
🖱️ Right-click Context Menu on Image Cards
📁 Files Changed
Frontend
Backend (Tauri)
Screenshots
View Info button :
Screenshots
- >
Summary by CodeRabbit
New Features
UI/UX Improvements
Other
✏️ Tip: You can customize this high-level summary in your review settings.