Fix: Open Original File button not working in Image Details panel #688.#689
Fix: Open Original File button not working in Image Details panel #688.#689NikhilPatil2005 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
WalkthroughThis pull request migrates the Tauri application from shell-based file operations to the tauri-opener plugin, adds a new ShareOptionsModal component with OneDrive integration for file sharing, updates Tauri dependencies, creates a new opener capability manifest, and includes development workflow documentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant ShareModal as ShareOptionsModal
participant Tauri as Tauri Runtime
participant Backend
User->>ShareModal: Click "Using OneDrive" or "More options"
ShareModal->>Tauri: invoke('onedrive_share', {filePath})
Tauri->>Backend: onedrive_share handler
Backend-->>Tauri: share URL
Tauri-->>ShareModal: receive share URL
rect rgb(200, 220, 255)
Note over ShareModal,Backend: OneDrive Share Flow
ShareModal->>Tauri: invoke('share_file', {shareUrl})
Tauri->>Backend: share_file handler
Backend-->>Tauri: open native share dialog
Tauri-->>ShareModal: dialog result
end
ShareModal-->>User: Share dialog displayed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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 ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 0
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaInfoPanel.tsx (1)
49-57: Frontendinvokeusage is fine; consider typing and trimming debug logsBoth call sites for
invoke('open_original_file', …)are correctly shaped for a command that takes a singlepath: Stringargument and returnsResult<(), String>once the Rust side is fixed to accept anAppHandleand useapp.opener().open_path. (v2.tauri.app)Two small optional tweaks:
- For better type-safety and editor help, you can make the return type explicit since you ignore it anyway:
await invoke<void>('open_original_file', { path: url }); // ... await invoke<void>('open_original_file', { path: currentImage.path });- The
console.log('Sending path to Tauri:', currentImage.path);line will print full local file paths (often including the username) to the dev console. Once you’re done debugging the issue, it’s safer to remove or gate this behind a debug flag so you’re not routinely logging potentially sensitive paths.If, in the future, you want to distinguish between opening URLs (map links) and filesystem paths at the Rust layer, the opener plugin also provides an
open_urlAPI that you could expose via a separate command.Also applies to: 170-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src-tauri/src/main.rs(2 hunks)frontend/src/components/Media/MediaInfoPanel.tsx(8 hunks)
🔇 Additional comments (4)
frontend/src-tauri/src/main.rs (2)
43-46: Command registration looks correct; ensure capabilities allow itRegistering
open_original_filealongsideservices::get_server_pathviatauri::generate_handler!is the right way to expose it toinvoke('open_original_file', …)on the frontend; no code changes needed here. (v2.tauri.app)Just make sure your Tauri 2 capabilities configuration allows this command and the opener plugin usage in production, otherwise the frontend may see “command not allowed” or permission errors at runtime.
10-24: Fixopen_original_fileto useAppHandle+OpenerExt::open_pathThis command requires corrections to use the tauri-plugin-opener v2.x API correctly. The
open_pathmethod must be called onapp.opener()after bringing theOpenerExttrait into scope. The command should accept anAppHandleparameter to access the plugin instance.+use tauri::AppHandle; +use tauri_plugin_opener::OpenerExt; + #[tauri::command] -async fn open_original_file(path: String) -> Result<(), String> { - println!("Tauri received path: {}", path); - - match tauri_plugin_opener::open_path(&path, None::<String>) { +async fn open_original_file(app: AppHandle, path: String) -> Result<(), String> { + println!("Tauri received path: {}", path); + + match app.opener().open_path(path, None::<&str>) { Ok(_) => { println!("File opened successfully"); Ok(()) } Err(e) => { eprintln!("Failed to open file: {}", e); Err(e.to_string()) } } }(If you never
awaitanything here, you can also make the command non-async.)frontend/src/components/Media/MediaInfoPanel.tsx (2)
2-2: Tauri v2invokeimport is correctImporting
invokefrom@tauri-apps/api/coreis the right pattern for Tauri 2 and matches the current docs. No changes needed here. (v2.tauri.app)
76-152: Sectioned “Image Details” layout changes look goodThe added inline section labels (Name, Date, Location, Tags, Position) and their associated markup are purely presentational, don’t affect behavior, and integrate cleanly with the existing props (
currentImage,currentIndex,totalImages). No functional issues spotted here.
|
Hi @rahulharpal1603, |
rahulharpal1603
left a comment
rahulharpal1603
left a comment
There was a problem hiding this comment.
Can't we just use this plugin: https://v2.tauri.app/plugin/opener/
It is much easier to use this than write Rust code.
rahulharpal1603
changed the title
NikhilPatil2005
force-pushed
the
open-image-issue
branch
from
194e71c to
0088342
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/Media/MediaInfoPanel.tsx (1)
41-55: LGTM! Improved null safety and correct API usage.The stricter null checks for coordinates and migration to
openUrlare correct. Minor note: the comment on line 47 about "Fixed URL format" is unclear—consider removing it if no specific fix was needed, or clarifying what was changed.If the comment refers to a specific issue, clarify it:
- // Fixed URL format (removed the '0' typo before latitude if it was unintentional, or kept standard) + // Standard Google Maps URL format
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/src-tauri/capabilities/opener.json(1 hunks)frontend/src-tauri/src/main.rs(1 hunks)frontend/src/components/Media/MediaInfoPanel.tsx(8 hunks)
🧰 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)
🔇 Additional comments (8)
frontend/src-tauri/src/main.rs (3)
10-10: LGTM! Clear documentation added.The comment clarifies the purpose and necessity of the opener plugin, improving code maintainability.
18-18: LGTM! Cleaner path resolution.The single-line path resolution is more concise and aligns with the pattern used in
services/mod.rs(lines 4-10).
22-24: LGTM! Improved formatting.The multi-line format with trailing comma follows Rust conventions and makes it easier to add future handlers.
frontend/src/components/Media/MediaInfoPanel.tsx (4)
1-12: LGTM! Correct migration to opener plugin.The import change from
tauri-plugin-shellto@tauri-apps/plugin-openeraligns with the capability configuration and is the correct fix for the issue.
29-39: LGTM! Clean refactoring.Both helper functions are more concise and use modern JavaScript features (optional chaining, nullish coalescing) appropriately.
122-132: LGTM! Proper null guards for coordinate formatting.The null checks ensure
toFixed()is only called on valid numbers, preventing runtime errors.
178-186: LGTM! This is the core fix for issue #688.The button now correctly calls
handleOpenOriginal, which uses the opener plugin with proper error handling. This should resolve the "Open Original File button not working" issue.frontend/src-tauri/capabilities/opener.json (1)
1-21: Verify that allowed paths align with intended use cases for opening images.The capability restricts file opening to standard home directory locations (
$HOME/**), which excludes external drives, network locations, and system directories. If the application needs to support opening images from external storage, those paths must be explicitly added to theallowlist. For example:
- macOS external drives:
"/Volumes/**"- Linux removable media:
"/media/**"or"/mnt/**"- Windows external drives:
"E:\\**","F:\\**", etc.- Network shares:
"\\\\server\\share\\**"(Windows) or mounted network pathsClarify whether the current scope is intentional (restrict to home directory for security) or whether external/network storage support is required.
| const handleOpenOriginal = async () => { | ||
| if (!currentImage?.path) { | ||
| console.warn("No path available for currentImage"); | ||
| return; | ||
| } | ||
| const path = currentImage.path; | ||
| console.log("Attempting to open file:", path); | ||
|
|
||
| try { | ||
| // This will now succeed because the path is in the allowed scope | ||
| await openPath(path); | ||
| console.log("openPath succeeded"); | ||
| } catch (err) { | ||
| console.error("openPath failed:", err); | ||
| alert(`Could not open file: ${err}`); // Optional user feedback | ||
| } | ||
| }; |
There was a problem hiding this comment.
Function works but has UX and assumption concerns.
The implementation correctly uses openPath and includes error handling, but:
-
Path scope assumption (line 66): The comment assumes the path is always in the allowed scope defined in
opener.json. If images can be stored outside$HOME/**(e.g., external drives, network shares),openPathwill fail. -
User feedback (line 71): Using
alert()for error feedback is not ideal UX—it blocks the UI and looks dated. Consider using a toast notification or inline error message instead.
Consider replacing the alert with better error feedback:
- alert(`Could not open file: ${err}`); // Optional user feedback
+ // TODO: Replace with toast notification system
+ console.error("User-facing error: Could not open file. Ensure the file exists and is accessible.");Additionally, verify that all image paths will be within the allowed directories defined in opener.json. If not, you may need to extend the capability permissions.
📝 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 handleOpenOriginal = async () => { | |
| if (!currentImage?.path) { | |
| console.warn("No path available for currentImage"); | |
| return; | |
| } | |
| const path = currentImage.path; | |
| console.log("Attempting to open file:", path); | |
| try { | |
| // This will now succeed because the path is in the allowed scope | |
| await openPath(path); | |
| console.log("openPath succeeded"); | |
| } catch (err) { | |
| console.error("openPath failed:", err); | |
| alert(`Could not open file: ${err}`); // Optional user feedback | |
| } | |
| }; | |
| const handleOpenOriginal = async () => { | |
| if (!currentImage?.path) { | |
| console.warn("No path available for currentImage"); | |
| return; | |
| } | |
| const path = currentImage.path; | |
| console.log("Attempting to open file:", path); | |
| try { | |
| // This will now succeed because the path is in the allowed scope | |
| await openPath(path); | |
| console.log("openPath succeeded"); | |
| } catch (err) { | |
| console.error("openPath failed:", err); | |
| // TODO: Replace with toast notification system | |
| console.error("User-facing error: Could not open file. Ensure the file exists and is accessible."); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In frontend/src/components/Media/MediaInfoPanel.tsx around lines 57 to 73,
remove the assumption comment about path always being in the allowed opener.json
scope and replace the blocking alert() with non-blocking UX; handle failures by
(1) using the app's toast/notification system (or render an inline error state)
to show a friendly message with the error, (2) log the full error to console for
debugging, and (3) detect permission/scope errors specifically and present
actionable guidance (e.g., "file may be outside allowed directories; move file
or update opener.json") or fallback behavior. Also add a lightweight pre-check
for allowed paths (use an existing isPathAllowed helper or compare against known
allowed prefixes like $HOME) before calling openPath and fall back to the
toast/instruction if the path is out of scope.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src-tauri/Cargo.toml (1)
26-26: Confirm dotenvy is actually needed
dotenvy = "0.15"looks fine for env loading, but from this diff alone it's not clear it's used. If no code ends up depending on it, consider dropping it to keep the Tauri binary leaner.run.txt (1)
1-75: Tighten and de‑duplicate run instructions; remove stray linesThe overall flow is helpful, but a few tweaks would reduce confusion:
- There are two nearly identical PR workflows (Lines 14–56 vs 59–72). Consider consolidating into one generic “create branch + PR” section with placeholders instead of hard‑coding
open-image-issueand then repeatingmy-branch.- Commands like
.\.venv\Scripts\activateare Windows‑specific; if contributors use macOS/Linux, a short note with the correspondingsource .venv/bin/activatevariant would help.- The trailing lines
palisaders,sugarlabs,oppisat the end look accidental and should probably be removed.None of this blocks the PR, but cleaning it up will make the doc easier for others to follow.
frontend/src/components/Media/ShareOptionsModal.tsx (3)
44-62: Align share flow behavior and disable “More options” when no pathA couple of small points around the share actions:
- The
onShareflow looks reasonable, and you correctly guard on missingfilePath. However, the “More options” button is styled as disabled when there’s nofilePathbut not actuallydisabled, so it’s still focusable/clickable (and just logs a warning). For better accessibility and consistency with the OneDrive button, also setdisabled={!filePath}.<button - onClick={onShare} - className={`inline-flex items-center gap-2 rounded border border-white/10 px-3 py-2 text-sm font-medium ${filePath ? 'hover:bg-white/5' : 'cursor-not-allowed opacity-50'}`} + onClick={onShare} + disabled={!filePath} + className={`inline-flex items-center gap-2 rounded border border-white/10 px-3 py-2 text-sm font-medium ${ + filePath ? 'hover:bg-white/5' : 'cursor-not-allowed opacity-50' + }`} aria-label="Using more options" >
shareUrlfrominvoke<string>('onedrive_share', …)is only logged. If the native side doesn’t use that URL, consider either wiring it into the sharing logic or dropping the call/log to avoid an extra round‑trip.Also applies to: 116-137
68-89: Optional: improve dialog accessibility labellingYou already set
role="dialog"andaria-modal="true", which is good. For better screen‑reader support, consider tying the dialog to its title viaaria-labelledby:- <div - className="absolute top-20 left-5 z-50 w-[360px] max-w-full rounded-lg bg-black/80 p-3 text-white backdrop-blur-md shadow-lg" - role="dialog" - aria-modal="true" - > + <div + className="absolute top-20 left-5 z-50 w-[360px] max-w-full rounded-lg bg-black/80 p-3 text-white backdrop-blur-md shadow-lg" + role="dialog" + aria-modal="true" + aria-labelledby="share-options-title" + > {/* Header */} <div className="flex items-start justify-between gap-2"> <div> - <h3 className="text-lg font-semibold">More share options</h3> + <h3 id="share-options-title" className="text-lg font-semibold"> + More share options + </h3>Not mandatory, but it makes the modal more accessible.
103-113: Clarify “Copy file” vs “Copy file path” behaviorBoth the header button and the “Copy file” tile call
copyPath, which copies the path, not the file content. That’s fine functionally, but the naming could be confusing:
- Consider renaming the tile label to “Copy path” (or similar), or
- Implement a real “copy/move file” behavior via the Tauri FS plugin for that tile and keep the existing “Copy file path” semantics for the header button.
This is mostly UX polish; the current implementation is safe.
Also applies to: 142-153
📜 Review details
Configuration used: CodeRabbit UI
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 (9)
.gitignore(1 hunks)frontend/package.json(1 hunks)frontend/src-tauri/Cargo.toml(1 hunks)frontend/src-tauri/capabilities/opener.json(1 hunks)frontend/src/components/Media/ImageCard.tsx(1 hunks)frontend/src/components/Media/MediaInfoPanel.tsx(8 hunks)frontend/src/components/Media/ShareOptionsModal.tsx(1 hunks)frontend/tsconfig.json(1 hunks)run.txt(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/tsconfig.json
- .gitignore
- frontend/src/components/Media/ImageCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/Media/MediaInfoPanel.tsx
- frontend/src-tauri/capabilities/opener.json
🔇 Additional comments (1)
frontend/package.json (1)
42-42:and
|
Please review again |
2 participants
fixes issue #688
Summary by CodeRabbit
New Features
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.