Repository navigation
[v3-linux] fileexplorer fix opening dir on Linux - #4521
Conversation
dosubot
Bot
added
the
size:S
WalkthroughReplaces the Linux file-explorer implementation with a self-contained version that stat-checks the target, uses a file's parent directory when needed, resolves desktop entries to build Exec arguments (with field-code substitution), restores helper functions, and falls back to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant FE as FileExplorer (Linux)
participant Stat as os.Stat
participant Finder as DesktopFileFinder
participant Resolver as explorerBinArgs
participant Exec as Process (Exec / xdg-open)
Caller->>FE: Open(path)
FE->>Stat: stat(path)
alt stat error
Stat-->>FE: error
FE-->>Caller: wrapped error
else stat ok
Stat-->>FE: fileInfo
alt target is file
FE->>FE: path = parent directory
end
FE->>Finder: findDesktopFile(path)
alt desktop file found
Finder-->>FE: desktopFile
FE->>Resolver: build Exec command & args
Resolver-->>FE: execCmd, args
FE->>Exec: run execCmd with args
else none found
Finder-->>FE: nil
FE->>Exec: run `xdg-open` with path
end
Exec-->>FE: result
FE-->>Caller: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
v3/internal/fileexplorer/fileexplorer_linux.go (1)
72-74: Error message clarity and idiomatic nil for args sliceEven if you keep the current behavior, include the offending path in the error and prefer returning a nil slice over an allocated empty slice.
- if err != nil { - return "", []string{}, fmt.Errorf("stat path: %w", err) - } + if err != nil { + return "", nil, fmt.Errorf("stat path %q: %w", path, err) + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
v3/internal/fileexplorer/fileexplorer_linux.go(1 hunks)
⏰ 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: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: semgrep-cloud-platform/scan
| stat, err := os.Stat(path) | ||
| if err != nil { | ||
| return "", []string{}, fmt.Errorf("stat path: %w", err) | ||
| } | ||
|
|
||
| // If the path is a file, we want to open the directory containing the file | ||
| if !stat.IsDir() { | ||
| path = filepath.Dir(path) | ||
| } |
There was a problem hiding this comment.
Handle non-existent file paths by opening the parent directory instead of erroring
Today, a path that doesn't yet exist (common when pointing to a file that will be created, e.g., a “Save As” target) causes os.Stat to fail and bubbles an error, which prevents opening the containing folder. That regresses the stated PR goal for file paths that aren't on disk yet.
Proposed fix: on Stat failure, attempt Stat on filepath.Dir(path) and open it if it exists; also normalize the path first. While here, improve the error message to include the path and return nil for the args slice on error.
func fallbackExplorerBinArgs(path string, selectFile bool) (string, []string, error) {
// NOTE: The linux fallback explorer opening does not support file selection
- stat, err := os.Stat(path)
- if err != nil {
- return "", []string{}, fmt.Errorf("stat path: %w", err)
- }
+ cleaned := filepath.Clean(path)
+ info, err := os.Stat(cleaned)
+ if err != nil {
+ // If the path doesn't exist, try opening its parent directory (e.g. a file that hasn't been created yet).
+ parent := filepath.Dir(cleaned)
+ if parent != cleaned {
+ if pinfo, err2 := os.Stat(parent); err2 == nil && pinfo.IsDir() {
+ return "xdg-open", []string{parent}, nil
+ }
+ }
+ return "", nil, fmt.Errorf("stat path %q: %w", cleaned, err)
+ }
// If the path is a file, we want to open the directory containing the file
- if !stat.IsDir() {
- path = filepath.Dir(path)
+ if !info.IsDir() {
+ path = filepath.Dir(cleaned)
+ } else {
+ path = cleaned
}
return "xdg-open", []string{path}, nil
}Manual test matrix to validate after the change:
- Existing directory: pass a dir path -> opens that directory.
- Existing file: pass a file path -> opens its parent directory.
- Non-existent file: pass a file path that doesn’t exist but whose parent does -> opens the parent directory.
- Non-existent parent: pass a path whose parent doesn’t exist -> returns an error that includes the path.
📝 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.
| stat, err := os.Stat(path) | |
| if err != nil { | |
| return "", []string{}, fmt.Errorf("stat path: %w", err) | |
| } | |
| // If the path is a file, we want to open the directory containing the file | |
| if !stat.IsDir() { | |
| path = filepath.Dir(path) | |
| } | |
| func fallbackExplorerBinArgs(path string, selectFile bool) (string, []string, error) { | |
| // NOTE: The linux fallback explorer opening does not support file selection | |
| cleaned := filepath.Clean(path) | |
| info, err := os.Stat(cleaned) | |
| if err != nil { | |
| // If the path doesn't exist, try opening its parent directory (e.g. a file that hasn't been created yet). | |
| parent := filepath.Dir(cleaned) | |
| if parent != cleaned { | |
| if pinfo, err2 := os.Stat(parent); err2 == nil && pinfo.IsDir() { | |
| return "xdg-open", []string{parent}, nil | |
| } | |
| } | |
| return "", nil, fmt.Errorf("stat path %q: %w", cleaned, err) | |
| } | |
| // If the path is a file, we want to open the directory containing the file | |
| if !info.IsDir() { | |
| path = filepath.Dir(cleaned) | |
| } else { | |
| path = cleaned | |
| } | |
| return "xdg-open", []string{path}, nil | |
| } |
🤖 Prompt for AI Agents
In v3/internal/fileexplorer/fileexplorer_linux.go around lines 71-79, normalize
the input path first (e.g., filepath.Clean), then call os.Stat; if Stat fails,
try os.Stat on filepath.Dir(path) and if that succeeds use the parent directory,
otherwise return an error that includes the original path and return nil for the
args slice; if Stat succeeds but is a file, set path = filepath.Dir(path) as
before; ensure the error message includes the path for clarity.
leaanthony
added
the
awaiting feedback
leaanthony
left a comment
leaanthony
left a comment
There was a problem hiding this comment.
Need an updated changelog 👍
Head branch was pushed to by a user without write access
github-actions
Bot
added
the
Documentation
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
v3/internal/fileexplorer/fileexplorer_linux.go (1)
68-82: Address the past review feedback on non-existent path handling.The past review comment correctly identifies that this implementation doesn't handle non-existent paths (e.g., "Save As" targets that don't exist yet). When
os.Statfails, the function returns an error instead of attempting to open the parent directory.Additionally, on line 73, the error case returns an empty slice
[]string{}instead ofnil, which is inconsistent with Go conventions.Please apply the diff suggested in the past review comment to:
- Normalize the path with
filepath.Clean- Handle non-existent paths by falling back to the parent directory
- Return
nilinstead of empty slice on error- Include the path in the error message
🧹 Nitpick comments (3)
v3/UNRELEASED_CHANGELOG.md (1)
51-51: Clarify wording and add contributor credit.As written, the bullet reads like the bug still occurs. Please rephrase to make the fix explicit and append the requested contributor credit, e.g. “Fix Linux file explorer so file paths open their parent directory (#4521, thanks @atterpac)”.
v3/internal/fileexplorer/fileexplorer_linux.go (2)
57-59: Fragile path existence check may incorrectly match substrings.The check
!strings.Contains(strings.Join(args, " "), path)can matchpathas a substring within other arguments or paths. For example, ifpathis"/tmp"and an arg contains"/tmp/cache", it would incorrectly detect the path as present.Consider iterating through args and using exact equality or path comparison:
- if !strings.Contains(strings.Join(args, " "), path) { - args = append(args, path) - } + pathFound := false + for _, arg := range args { + if arg == path { + pathFound = true + break + } + } + if !pathFound { + args = append(args, path) + }
18-62: Document or utilize theselectFileparameter.The
selectFileparameter is declared but never used inexplorerBinArgs. Either implement file selection support or document why it's not supported on Linux.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.md(1 hunks)v3/internal/fileexplorer/fileexplorer_linux.go(1 hunks)
🔇 Additional comments (2)
v3/internal/fileexplorer/fileexplorer_linux.go (2)
1-16: LGTM!The imports are appropriate for the Linux file explorer implementation. The use of
gopkg.in/ini.v1constrains to the v1 major version, which is suitable for parsing desktop entry files.
64-66: LGTM!The empty
SysProcAttris appropriate for the Linux platform in this context.
| exec := cfg.Section("Desktop Entry").Key("Exec").String() | ||
| for fieldCode, replacement := range fieldCodes { | ||
| exec = strings.ReplaceAll(exec, fieldCode, replacement) | ||
| } | ||
| args := strings.Fields(exec) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid shadowing the exec package import.
The variable name exec on line 52 shadows the exec package imported at line 10. This reduces code clarity and could lead to confusion.
Apply this diff to rename the variable:
- exec := cfg.Section("Desktop Entry").Key("Exec").String()
+ execCmd := cfg.Section("Desktop Entry").Key("Exec").String()
for fieldCode, replacement := range fieldCodes {
- exec = strings.ReplaceAll(exec, fieldCode, replacement)
+ execCmd = strings.ReplaceAll(execCmd, fieldCode, replacement)
}
- args := strings.Fields(exec)
+ args := strings.Fields(execCmd)📝 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.
| exec := cfg.Section("Desktop Entry").Key("Exec").String() | |
| for fieldCode, replacement := range fieldCodes { | |
| exec = strings.ReplaceAll(exec, fieldCode, replacement) | |
| } | |
| args := strings.Fields(exec) | |
| execCmd := cfg.Section("Desktop Entry").Key("Exec").String() | |
| for fieldCode, replacement := range fieldCodes { | |
| execCmd = strings.ReplaceAll(execCmd, fieldCode, replacement) | |
| } | |
| args := strings.Fields(execCmd) |
🤖 Prompt for AI Agents
In v3/internal/fileexplorer/fileexplorer_linux.go around lines 52 to 56, the
local variable named `exec` shadows the imported `exec` package; rename the
variable (e.g., `execStr`, `execCmd` or `execLine`) and update its subsequent
uses in this block so the package import is no longer shadowed, preserving the
same logic (replace field codes, then split into args with strings.Fields).
| func pathToURI(path string) string { | ||
| absPath, err := filepath.Abs(path) | ||
| if err != nil { | ||
| return path | ||
| } | ||
| return "file://" + url.PathEscape(absPath) | ||
| } |
There was a problem hiding this comment.
Fix URI encoding to preserve path separators.
url.PathEscape escapes all special characters including forward slashes, which breaks the file URI. For example, /tmp/file.txt becomes %2Ftmp%2Ffile.txt, which is invalid.
Use url.URL to properly construct the URI:
func pathToURI(path string) string {
absPath, err := filepath.Abs(path)
if err != nil {
return path
}
- return "file://" + url.PathEscape(absPath)
+ u := url.URL{
+ Scheme: "file",
+ Path: absPath,
+ }
+ return u.String()
}🤖 Prompt for AI Agents
In v3/internal/fileexplorer/fileexplorer_linux.go around lines 84 to 90, the
current pathToURI uses url.PathEscape which encodes slashes and breaks file
URIs; replace that logic by constructing a url.URL with Scheme "file" and Path
set to the absolute path converted to forward-slash form (use
filepath.ToSlash(absPath)) and return url.String(); this preserves separators
while still percent-encoding characters like spaces.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
v3/internal/fileexplorer/fileexplorer_linux.go (4)
52-56: Avoid shadowing theexecpackage import.The variable name
execshadows the importedexecpackage, reducing clarity.Apply this diff to rename the variable:
- exec := cfg.Section("Desktop Entry").Key("Exec").String() + execCmd := cfg.Section("Desktop Entry").Key("Exec").String() for fieldCode, replacement := range fieldCodes { - exec = strings.ReplaceAll(exec, fieldCode, replacement) + execCmd = strings.ReplaceAll(execCmd, fieldCode, replacement) } - args := strings.Fields(exec) + args := strings.Fields(execCmd)
71-79: Handle non-existent file paths by opening the parent directory.The
os.Statcall will fail for paths that don't exist yet (e.g., "Save As" targets), which prevents opening the containing folder. This regresses the PR's goal for non-existent file paths.Apply this diff to handle non-existent paths:
func fallbackExplorerBinArgs(path string, selectFile bool) (string, []string, error) { // NOTE: The linux fallback explorer opening does not support file selection - stat, err := os.Stat(path) - if err != nil { - return "", []string{}, fmt.Errorf("stat path: %w", err) - } + cleaned := filepath.Clean(path) + info, err := os.Stat(cleaned) + if err != nil { + // If the path doesn't exist, try opening its parent directory + parent := filepath.Dir(cleaned) + if parent != cleaned { + if pinfo, err2 := os.Stat(parent); err2 == nil && pinfo.IsDir() { + return "xdg-open", []string{parent}, nil + } + } + return "", nil, fmt.Errorf("stat path %q: %w", cleaned, err) + } // If the path is a file, we want to open the directory containing the file - if !stat.IsDir() { - path = filepath.Dir(path) + if !info.IsDir() { + path = filepath.Dir(cleaned) + } else { + path = cleaned } return "xdg-open", []string{path}, nil }
84-90: Fix URI encoding to preserve path separators.
url.PathEscapeescapes forward slashes, which breaks the file URI. For example,/tmp/file.txtbecomesfile://%2Ftmp%2Ffile.txt, which is invalid.Use
url.URLto properly construct the URI:func pathToURI(path string) string { absPath, err := filepath.Abs(path) if err != nil { return path } - return "file://" + url.PathEscape(absPath) + u := url.URL{ + Scheme: "file", + Path: absPath, + } + return u.String() }
92-107: Handle empty environment variables to avoid invalid paths.If
XDG_DATA_HOMEorHOMEare empty,filepath.Joincreates invalid paths like/applications. Whileos.Statwill fail, it's cleaner to skip them.Apply this diff:
func findDesktopFile(xdgFileName string) (string, error) { - paths := []string{ - filepath.Join(os.Getenv("XDG_DATA_HOME"), "applications"), - filepath.Join(os.Getenv("HOME"), ".local", "share", "applications"), + var paths []string + + if xdgDataHome := os.Getenv("XDG_DATA_HOME"); xdgDataHome != "" { + paths = append(paths, filepath.Join(xdgDataHome, "applications")) + } + if home := os.Getenv("HOME"); home != "" { + paths = append(paths, filepath.Join(home, ".local", "share", "applications")) + } + paths = append(paths, "/usr/share/applications", - } + ) for _, path := range paths { desktopFile := filepath.Join(path, xdgFileName) if _, err := os.Stat(desktopFile); err == nil { return desktopFile, nil } } err := fmt.Errorf("desktop file not found: %s", xdgFileName) return "", err }
🧹 Nitpick comments (3)
v3/internal/fileexplorer/fileexplorer_linux.go (3)
35-35: Remove redundantStderrassignment.Setting
Stderr = nilis redundant since it's already the zero value for*os.File.Apply this diff:
fileManagerQuery.Stdout = buf - fileManagerQuery.Stderr = nil
41-41: Remove unnecessary parentheses.The expression has redundant double parentheses.
Apply this diff:
- desktopFile, err := findDesktopFile(strings.TrimSpace((buf.String()))) + desktopFile, err := findDesktopFile(strings.TrimSpace(buf.String()))
57-59: Consider more robust path checking.The current logic joins all args with spaces and checks if the path is a substring. This could produce false positives if
pathappears as a substring within another argument.Consider checking each argument individually:
- if !strings.Contains(strings.Join(args, " "), path) { + pathFound := false + for _, arg := range args { + if arg == path { + pathFound = true + break + } + } + if !pathFound { args = append(args, path) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.md(1 hunks)v3/internal/fileexplorer/fileexplorer_linux.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:15:37.785Z
Learnt from: APshenkin
PR: wailsapp/wails#4484
File: v2/internal/frontend/utils/urlValidator_test.go:152-155
Timestamp: 2025-08-08T13:15:37.785Z
Learning: In v2/internal/frontend/utils/urlValidator_test.go, the project accepts a literal '|' in the URL path (e.g., "https://example.com/user/123|admin") as valid for BrowserOpenURL. Maintainer (APshenkin) clarified that browsers will escape it automatically; thus, tests should not require percent-encoding '%7C' nor expect an error from utils.ValidateURL for this case. Avoid suggesting rejection or forced encoding of '|' in path in future reviews.
Applied to files:
v3/internal/fileexplorer/fileexplorer_linux.go
🔇 Additional comments (2)
v3/UNRELEASED_CHANGELOG.md (1)
51-51: Changelog entry looks good.The entry clearly describes the fix and references the PR number appropriately.
v3/internal/fileexplorer/fileexplorer_linux.go (1)
64-66: LGTM!The empty
SysProcAttris appropriate for this use case.
|
I noticed that the file had crlf line endings so I changed it to lf like the other files. |
|
Thanks @atterpac 🙏 |
* convert file to use lf line-endings * fileexplorer fix opening dir on Linux * include update in unreleased changelog --------- Co-authored-by: Angus Dippenaar <angusdippenaar@gmail.com>
This pull request improves the behavior of the Linux fallback file explorer by ensuring that when a file path is provided, the explorer opens the containing directory instead of the file itself. It also adds error handling for invalid paths.
File explorer behavior improvements:
fallbackExplorerBinArgsinfileexplorer_linux.goto check if the given path is a file, and if so, open its containing directory instead of the file itself.os.Statfails on the provided path, returning a wrapped error.Summary by CodeRabbit