OpenFileManager for opening with the native file manager and optional file selection support #3937
Conversation
WalkthroughThe pull request introduces significant updates to the project's changelog and core functionality. A new method, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
mkdocs-website/docs/en/changelog.md (1)
23-23: Add Space Between Contributor Mentions for Correct FormattingIn line 23, there is no space between the two contributor mentions, which may affect Markdown rendering and make it harder to read.
Apply this diff to add a space:
23 - `app.OpenFileManager(path string)` to open file manager to the path `path` with optional highlighting via `selectFile` by [@Krzysztofz01](https://github.com/Krzysztofz01)[@rcalixte](https://github.com/rcalixte) + - `app.OpenFileManager(path string)` to open file manager to the path `path` with optional highlighting via `selectFile` by [@Krzysztofz01](https://github.com/Krzysztofz01) [@rcalixte](https://github.com/rcalixte)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/pkg/application/application.go(2 hunks)
rcalixte
force-pushed
the
file_manager
branch
5 times, most recently
from
adc6a94 to
dc9056c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
v3/internal/fileexplorer/fileexplorer.go (3)
20-21: Improve error message clarityThe error message includes redundant "stat" text which doesn't add value to the error context.
- return fmt.Errorf("failed to access the specified path stat: %w", err) + return fmt.Errorf("failed to access the specified path: %w", err)
50-58: Consider adding context for command executionThe command execution could benefit from context to handle potential hanging processes, especially when dealing with external file managers.
+import "context" + func OpenFileManager(path string, selectFile bool) error { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + - cmd := exec.Command(explorerBin, explorerArgs...) + cmd := exec.CommandContext(ctx, explorerBin, explorerArgs...)
120-123: Consider supporting file selection in fallback modeThe fallback implementation currently forces directory opening and ignores the
selectFileparameter. Consider implementing file selection support using alternative methods likedbus-sendto communicate with file managers that support it.var linuxFallbackExplorerBinArgs explorerBinArgs = func(path string, selectFile bool) (string, []string, error) { - // NOTE: The linux fallback explorer opening is not supporting file selection - path = filepath.Dir(path) + if !selectFile { + path = filepath.Dir(path) + } + // Try dbus-send first for file managers that support it + if selectFile { + if _, err := exec.LookPath("dbus-send"); err == nil { + return "dbus-send", []string{ + "--session", + "--dest=org.freedesktop.FileManager1", + "--type=method_call", + "/org/freedesktop/FileManager1", + "org.freedesktop.FileManager1.ShowItems", + fmt.Sprintf("array:string:file://%s", path), + "string:''", + }, nil + } + } return "xdg-open", []string{path}, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mkdocs-website/docs/en/changelog.md
- v3/pkg/application/application.go
🔇 Additional comments (4)
v3/internal/fileexplorer/fileexplorer.go (4)
62-70: LGTM! Windows implementation is correct
The implementation properly handles both directory opening and file selection scenarios using the appropriate explorer.exe flags.
72-80: LGTM! macOS implementation is correct
The implementation correctly uses the open command with the -R flag for file selection.
90-94: Possible Incorrect Path Construction When XDG_DATA_HOME Is Unset
This is a previously identified issue that still needs to be addressed.
106-113:
Fix duplicate path argument and improve desktop entry handling
There are two issues in this section:
- The path is being appended twice (once through %f/%F replacement and again explicitly)
- The desktop entry Exec field handling needs improvement (previously identified)
exec := cfg.Section("Desktop Entry").Key("Exec").String()
exec = strings.ReplaceAll(exec, "%f", path)
exec = strings.ReplaceAll(exec, "%F", path)
exec = strings.ReplaceAll(exec, "%u", path)
exec = strings.ReplaceAll(exec, "%U", path)
args := strings.Fields(exec)
- args = append(args, path)
return args[0], args[1:], nilLikely invalid or redundant comment.
rcalixte
force-pushed
the
file_manager
branch
2 times, most recently
from
7d2559a to
683d9bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
v3/internal/fileexplorer/fileexplorer.go (4)
21-21: Consider a longer timeout for slow systems or network paths.The 5-second timeout might be insufficient when dealing with slow systems, network paths, or when the file manager is slow to start.
-ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
24-29: Enhance path validation.The current path validation could be more robust by:
- Converting to absolute path
- Cleaning the path to remove any
.or..- Checking for symlink resolution
-path = os.ExpandEnv(path) -if pathInfo, err := os.Stat(path); err != nil { +path = os.ExpandEnv(path) +path = filepath.Clean(path) +absPath, err := filepath.Abs(path) +if err != nil { + return fmt.Errorf("failed to resolve absolute path: %w", err) +} +path = absPath +if pathInfo, err := os.Stat(path); err != nil {
47-47: Improve error message for unsupported platforms.The error message could be more specific by including the current platform.
-return errors.New("unsupported platform") +return fmt.Errorf("platform %q is not supported", runtime.GOOS)
127-130: Consider Supporting File Selection in Fallback ModeThe fallback implementation could try alternative file managers that support file selection (e.g., nautilus, dolphin, nemo) before falling back to xdg-open.
var linuxFallbackExplorerBinArgs explorerBinArgs = func(path string, selectFile bool) (string, []string, error) { - // NOTE: The linux fallback explorer opening is not supporting file selection - path = filepath.Dir(path) - return "xdg-open", []string{path}, nil + if !selectFile { + return "xdg-open", []string{path}, nil + } + + // Try common file managers that support selection + fileManagers := []struct { + bin string + args []string + }{ + {"nautilus", []string{"--select"}}, + {"dolphin", []string{"--select"}}, + {"nemo", []string{"--no-desktop"}}, + } + + for _, fm := range fileManagers { + if _, err := exec.LookPath(fm.bin); err == nil { + args := append(fm.args, path) + return fm.bin, args, nil + } + } + + // If no file manager supports selection, fall back to opening the directory + return "xdg-open", []string{filepath.Dir(path)}, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/pkg/application/application.go
- mkdocs-website/docs/en/changelog.md
🔇 Additional comments (5)
v3/internal/fileexplorer/fileexplorer.go (5)
18-18: Well-designed type for platform-specific implementations!
The explorerBinArgs function type provides a clean interface for implementing platform-specific file manager opening logic.
67-75: LGTM! Proper handling of Windows explorer.exe quirks.
The implementation correctly:
- Handles paths with spaces using quotes
- Implements file selection using the
/selectflag - Ignores exit codes due to known Windows explorer.exe behavior
77-85: LGTM! Clean macOS implementation.
The implementation correctly uses the open command with the -R flag for file selection, following macOS conventions.
95-99: 🛠️ Refactor suggestion
Incorrect Path Construction When XDG_DATA_HOME Is Unset
The path construction should use filepath.Join for better cross-platform compatibility.
-xdgPath := strings.TrimSpace(os.Getenv("XDG_DATA_HOME"))
-if xdgPath == "" {
- xdgPath = os.Getenv("HOME") + "/.local/share"
-}
-desktopFile = xdgPath + "/applications/" + strings.TrimSpace((buf.String()))
+xdgPath := strings.TrimSpace(os.Getenv("XDG_DATA_HOME"))
+if xdgPath == "" {
+ xdgPath = filepath.Join(os.Getenv("HOME"), ".local", "share")
+}
+desktopFile = filepath.Join(xdgPath, "applications", strings.TrimSpace(buf.String()))111-119: 🛠️ Refactor suggestion
Enhance Desktop Entry Field Code Handling
While the code handles basic field codes, it should also:
- Handle field codes with URIs (
%u,%U) properly by converting paths to URIs - Handle other field codes like
%d,%D,%n,%N,%v,%m - Strip out any remaining unhandled field codes
+func pathToURI(path string) string {
+ absPath, err := filepath.Abs(path)
+ if err != nil {
+ return path
+ }
+ return "file://" + absPath
+}
+
exec := cfg.Section("Desktop Entry").Key("Exec").String()
-exec = strings.ReplaceAll(exec, "%f", path)
-exec = strings.ReplaceAll(exec, "%F", path)
-exec = strings.ReplaceAll(exec, "%u", path)
-exec = strings.ReplaceAll(exec, "%U", path)
+exec = strings.ReplaceAll(exec, "%f", path)
+exec = strings.ReplaceAll(exec, "%F", path)
+exec = strings.ReplaceAll(exec, "%u", pathToURI(path))
+exec = strings.ReplaceAll(exec, "%U", pathToURI(path))
+// Strip other field codes
+exec = strings.NewReplacer(
+ "%d", "",
+ "%D", "",
+ "%n", "",
+ "%N", "",
+ "%v", "",
+ "%m", "",
+).Replace(exec)Likely invalid or redundant comment.
rcalixte
force-pushed
the
file_manager
branch
from
683d9bf to
1b282aa
Compare
I've added some tests but it would help to have paths that are known at the time the tests are running. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
v3/internal/fileexplorer/fileexplorer_test.go (1)
1-40: Consider adding integration tests.While unit tests are valuable, the current implementation would benefit from integration tests that verify the actual interaction with the native file manager.
Consider:
- Adding integration test markers (e.g.,
//+build integration)- Implementing platform-specific process verification
- Adding user interaction simulation tests
- Documenting manual test scenarios
Would you like me to help create a separate integration test file with these improvements?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/internal/fileexplorer/fileexplorer_test.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v3/pkg/application/application.go
- mkdocs-website/docs/en/changelog.md
- v3/internal/fileexplorer/fileexplorer.go
🔇 Additional comments (1)
v3/internal/fileexplorer/fileexplorer_test.go (1)
1-8: LGTM! Clean package structure and imports.
The test package name and imports are correctly defined.
rcalixte
force-pushed
the
file_manager
branch
from
1b282aa to
17abf0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
v3/internal/fileexplorer/fileexplorer.go (2)
20-35: Consider increasing the timeout duration for slow systems.The 10-second timeout might be insufficient for slow systems or network paths. Consider making this configurable or increasing it to handle edge cases.
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
31-35: Enhance path validation for edge cases.While the current path validation is good, consider adding checks for:
- Path length limits on Windows
- Special characters that might cause issues with shell execution
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/internal/fileexplorer/fileexplorer_test.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- v3/internal/fileexplorer/fileexplorer_test.go
- v3/pkg/application/application.go
- mkdocs-website/docs/en/changelog.md
🔇 Additional comments (2)
v3/internal/fileexplorer/fileexplorer.go (2)
4-15: LGTM! Well-structured imports and type definition.
The new imports and the explorerBinArgs function type are well-organized and appropriate for the functionality. The type definition provides a clean interface for platform-specific implementations.
Also applies to: 18-19
20-71: Verify input validation for security concerns.
The code executes commands based on user input, which could potentially be exploited.
✅ Verification successful
Let me analyze the code and search results to verify the security concerns.
The code in fileexplorer.go shows proper input validation and path sanitization:
- Uses
os.ExpandEnv()to safely expand environment variables - Uses
filepath.Clean()to normalize the path and remove any.or..components - Uses
filepath.Abs()to resolve to absolute path - Validates path existence with
os.Stat()before proceeding - The command execution uses platform-specific predefined binaries and arguments (via
explorerBinArgs) - Command execution is constrained with a timeout context
Let's verify the platform-specific command execution:
Let's check the platform-specific implementations to ensure they handle the command arguments securely:
Let's try a different search pattern to find the platform-specific implementations:
Input validation and command execution are properly secured
The code implements multiple layers of security:
-
Path sanitization:
- Uses
os.ExpandEnv(),filepath.Clean(), andfilepath.Abs()to safely handle paths - Validates path existence with
os.Stat()
- Uses
-
Command execution safety:
- Uses platform-specific predefined binaries (explorer.exe, open, xdg-open)
- Arguments are properly constructed without shell interpolation
- Command execution is constrained with a timeout context
- Windows: Properly quotes paths in arguments
- Darwin: Uses safe
-Rflag for selection - Linux: Safely handles desktop entries and falls back to
xdg-open
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential command injection vulnerabilities
# Test: Search for command execution without proper escaping
rg -A 5 'exec\.Command|os\.StartProcess'
# Test: Check for path traversal protection
rg -A 5 'filepath\.Clean|filepath\.Abs'
Length of output: 66158
Script:
#!/bin/bash
# Check platform-specific command execution implementations
rg -A 10 'type explorerBinArgs'
Length of output: 894
Script:
#!/bin/bash
# Check platform-specific command execution implementations
rg -A 15 'func \w+ExplorerBinArgs'
Length of output: 36
Script:
#!/bin/bash
# Search for platform-specific explorer implementations
rg -A 15 'var \w+ExplorerBinArgs'
# Also search for any file with 'explorer' in the name to find the implementations
fd explorer
Length of output: 3824
rcalixte
force-pushed
the
file_manager
branch
from
17abf0b to
8484c61
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
v3/internal/fileexplorer/fileexplorer.go (3)
21-23: Consider increasing the timeout duration.The 10-second timeout might be too short for slow systems or network drives. Consider making this configurable or increasing it to 30 seconds.
- ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
62-64: Consider logging command output on error.While suppressing command output is generally good, it might be helpful to capture and log it when an error occurs to aid in debugging.
- cmd.Stdout = nil - cmd.Stderr = nil + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderrThen in the error handling:
if err := cmd.Run(); err != nil { if !ignoreExitCode { - return fmt.Errorf("failed to open the file explorer: %w", err) + return fmt.Errorf("failed to open the file explorer: %w (stdout: %s, stderr: %s)", + err, stdout.String(), stderr.String()) } }
140-144: Consider warning about disabled file selection in fallback mode.When falling back to xdg-open, if selectFile is true, consider returning a warning in the error message.
- return "xdg-open", []string{path}, nil + if selectFile { + return "xdg-open", []string{path}, fmt.Errorf("file selection not supported in fallback mode") + } + return "xdg-open", []string{path}, nilv3/internal/fileexplorer/fileexplorer_test.go (1)
20-29: Consider adding more error test cases.The test cases could be expanded to include:
- Invalid paths with special characters
- Network paths
- Paths with insufficient permissions
tests := []struct { name string path string selectFile bool expectedErr error }{ {"Open Existing File", tempDir, false, nil}, {"Select Existing File", tempDir, true, nil}, {"Non-Existent Path", "/path/does/not/exist", false, fmt.Errorf("failed to access the specified path: /path/does/not/exist")}, + {"Path With Special Chars", filepath.Join(tempDir, "test space.txt"), true, nil}, + {"No Permission Path", "/root/test.txt", false, fmt.Errorf("failed to access the specified path")}, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/internal/fileexplorer/fileexplorer_test.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- v3/pkg/application/application.go
- mkdocs-website/docs/en/changelog.md
🔇 Additional comments (4)
v3/internal/fileexplorer/fileexplorer.go (4)
25-36: LGTM! Robust path handling implementation.
The path handling is thorough with:
- Environment variable expansion
- Path cleaning and normalization
- Absolute path resolution
- Existence validation
- Smart handling of selectFile based on path type
74-82: LGTM! Clean Windows implementation.
The Windows implementation correctly handles:
- File selection with /select flag
- Path quoting
- Command argument construction
84-92: LGTM! Clean macOS implementation.
The macOS implementation correctly uses the 'open' command with proper handling of the -R flag for file selection.
146-168: LGTM! Well-implemented helper functions.
Both pathToURI and findDesktopFile are well-implemented:
- Proper URL encoding
- Following XDG specification
- Good error handling
rcalixte
force-pushed
the
file_manager
branch
from
8484c61 to
7958150
Compare
I think it's ready for review. Let me know if anything else should be changed. |
rcalixte
force-pushed
the
file_manager
branch
from
7958150 to
2ff4665
Compare
file selection support Closes #3197
rcalixte
force-pushed
the
file_manager
branch
from
2ff4665 to
48157e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
v3/internal/fileexplorer/fileexplorer.go (2)
74-82: Consider enhancing path escaping for Windows.While the current implementation works, it could be improved to handle special characters in paths more robustly.
- args = append(args, fmt.Sprintf("/select,\"%s\"", path)) + args = append(args, fmt.Sprintf("/select,%s", strings.ReplaceAll(path, "\"", "\"\"")))
94-138: Consider adding error logging and improving path handling.The Linux implementation could benefit from:
- Logging errors before falling back
- More robust path handling in args
if err := fileManagerQuery.Run(); err != nil { + a.Logger.Debug("Failed to query default file manager", "error", err) return linuxFallbackExplorerBinArgs(path, selectFile) } desktopFile, err := findDesktopFile(strings.TrimSpace((buf.String()))) if err != nil { + a.Logger.Debug("Failed to find desktop file", "error", err) return linuxFallbackExplorerBinArgs(path, selectFile) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
mkdocs-website/docs/en/changelog.md(1 hunks)v3/internal/fileexplorer/fileexplorer.go(1 hunks)v3/internal/fileexplorer/fileexplorer_test.go(1 hunks)v3/pkg/application/application.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/internal/fileexplorer/fileexplorer_test.go
🔇 Additional comments (6)
v3/internal/fileexplorer/fileexplorer.go (4)
19-72: Well-structured implementation with proper error handling and path validation!
The implementation includes:
- Appropriate use of context with timeout
- Thorough path validation and error handling
- Clear separation of platform-specific logic
84-92: Clean implementation following macOS conventions!
The implementation correctly uses the 'open' command with appropriate flags.
140-144: Appropriate fallback implementation with clear documentation!
The fallback mechanism is well-documented and provides basic functionality when the primary method fails.
146-168: Well-implemented helper functions following standards!
Both helper functions are properly implemented:
pathToURIhandles path escaping correctlyfindDesktopFilefollows XDG specification for desktop entry locations
mkdocs-website/docs/en/changelog.md (1)
22-22: Well-documented changelog entry!
The changelog properly documents the new feature with clear attribution to contributors.
v3/pkg/application/application.go (1)
1050-1054: Clean implementation with proper synchronization!
The OpenFileManager method is well-implemented:
- Uses
InvokeSyncWithErrorfor thread safety - Properly forwards parameters to the fileexplorer package
Closes #3197
cc @Krzysztofz01
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please select the option that is relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor.If you checked Linux, please specify the distro and version.
Test Configuration
Please paste the output of
wails doctor. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation