Fix invalid path for windows dialogs #4019
Conversation
WalkthroughThis pull request introduces several modifications to the go-common-file-dialog package, focusing on error handling, UUID generation, and resource management. The changes include renaming error variables, updating import statements, modifying error handling in various methods, and simplifying deferred resource release. The modifications span multiple files within the internal package, addressing potential issues with file dialogs, particularly on Windows platforms. Changes
Assessment against linked issues
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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 (2)
v2/internal/go-common-file-dialog/cfdutil/CFDUtil.go (2)
13-13: Consider handling Release errors to prevent resource leaks.While the simplified defer is cleaner, silently ignoring Release errors could mask cleanup issues that might contribute to resource leaks on Windows.
Consider logging or handling Release errors:
-defer dialog.Release() +defer func() { + if err := dialog.Release(); err != nil { + // Log or handle cleanup error + } +}()
Line range hint
7-7: Add documentation for dialog functions.The TODO comments indicate missing documentation. Since these functions handle Windows dialogs, proper documentation of their behavior, parameters, and potential platform-specific considerations would be valuable.
Would you like me to help generate comprehensive documentation for these functions?
Also applies to: 17-17, 27-27, 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
v2/internal/go-common-file-dialog/cfd/CommonFileDialog_nonWindows.go(1 hunks)v2/internal/go-common-file-dialog/cfd/DialogConfig.go(3 hunks)v2/internal/go-common-file-dialog/cfd/errors.go(1 hunks)v2/internal/go-common-file-dialog/cfd/iFileOpenDialog.go(4 hunks)v2/internal/go-common-file-dialog/cfd/iFileSaveDialog.go(1 hunks)v2/internal/go-common-file-dialog/cfd/iShellItemArray.go(2 hunks)v2/internal/go-common-file-dialog/cfd/vtblCommonFunc.go(1 hunks)v2/internal/go-common-file-dialog/cfdutil/CFDUtil.go(4 hunks)website/src/pages/changelog.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- v2/internal/go-common-file-dialog/cfd/CommonFileDialog_nonWindows.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (13)
v2/internal/go-common-file-dialog/cfdutil/CFDUtil.go (2)
23-23: Consistent pattern applied across all dialog functions.The same simplified defer pattern is applied to all dialog functions. Consider applying the Release error handling suggestion from above consistently across all these functions.
Also applies to: 33-33, 43-43
Line range hint
1-44: Verify complete implementation of invalid path handling.While the resource cleanup improvements are valuable, I don't see direct handling of invalid default paths, which was the primary objective of this PR. Could you verify if:
- Path validation is handled in the imported
cfdpackage- These cleanup changes are part of a larger solution
Let's check for path validation in the cfd package:
✅ Verification successful
Path validation is properly implemented through Windows Shell API.
The implementation correctly handles invalid paths by:
- Utilizing Windows Shell API's built-in path validation
- Properly propagating errors from invalid paths
- Ensuring resource cleanup on validation failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for path validation logic in the cfd package rg -t go "path|directory|folder.*valid" ./v2/internal/go-common-file-dialog/cfd/Length of output: 1206
Script:
#!/bin/bash # Check implementation of newIShellItem and related error handling ast-grep --pattern 'func newIShellItem($path string) ($ret, $err error) { $$$ }' # Look for recent changes in error handling rg -B 2 -A 2 "error" ./v2/internal/go-common-file-dialog/cfd/Length of output: 38203
v2/internal/go-common-file-dialog/cfd/errors.go (1)
5-7: LGTM! Improved error variable naming.The rename from
ErrCancelledtoErrorCancelledfollows Go's naming convention for error variables.v2/internal/go-common-file-dialog/cfd/iShellItemArray.go (1)
61-61: LGTM! Improved error message clarity.The error message now accurately describes the failure condition instead of incorrectly indicating user cancellation.
v2/internal/go-common-file-dialog/cfd/iFileSaveDialog.go (1)
79-79: LGTM! Simplified package dependencies.Moved UUID generation to local package, reducing external dependencies while maintaining functionality.
v2/internal/go-common-file-dialog/cfd/DialogConfig.go (2)
18-20: LGTM! Added type safety for FileFilter.Preventing FileFilter type obfuscation ensures consistent behavior across different build configurations.
79-82: Verify network path handling in the codebase.Let's check if there are any existing uses of UNC paths that might be affected by the current validation.
Also applies to: 90-93
v2/internal/go-common-file-dialog/cfd/iFileOpenDialog.go (4)
8-8: LGTM: Package dependency updateReplacing the internal util package with the widely-used google/uuid package is a good choice for UUID generation.
109-109: LGTM: Updated SetRole implementationThe method now uses the new StringToUUID function, maintaining consistency with the package's UUID handling approach.
180-180: LGTM: Improved error handlingChanged error return from ErrCancelled to ErrorCancelled for consistency with the error naming convention.
198-200: LGTM: New UUID generation functionThe new StringToUUID function provides a standardized way to generate UUIDs using SHA1 hashing, which is more reliable than the previous implementation.
v2/internal/go-common-file-dialog/cfd/vtblCommonFunc.go (1)
171-171: LGTM: Enhanced error reportingThe error message is now more descriptive, helping with debugging when shellItem is nil.
website/src/pages/changelog.mdx (1)
35-35: LGTM: Changelog updateThe changelog entry properly documents the Windows SaveDialog crash fix and correctly attributes it to the author.
| _, err = os.Stat(config.Folder) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Path validation might be too strict for network paths.
The current implementation using os.Stat might fail for valid Windows UNC paths or network shares that are temporarily unavailable. This could prevent users from accessing network locations.
Consider using a more flexible path validation approach:
- _, err = os.Stat(config.Folder)
- if err != nil {
- return
- }
+ if !isValidPath(config.Folder) {
+ return fmt.Errorf("invalid folder path: %s", config.Folder)
+ }Add this helper function:
func isValidPath(path string) bool {
// Basic path validation
if path == "" {
return false
}
// Check if it's a Windows UNC path
if strings.HasPrefix(path, "\\\\") {
return true
}
// For local paths, check if they exist
_, err := os.Stat(path)
return err == nil
}Also applies to: 90-93
Deploying wails with
|
| Latest commit: |
613971d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f6c88229.wails.pages.dev |
| Branch Preview URL: | https://v2-bugfix-3804-windows-dialo.wails.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
.github/workflows/pr.yml (1)
Line range hint
28-41: Verify the impact of removing the linting jobThe removal of the entire linting job eliminates important code quality checks. This change seems unrelated to the PR's objective of fixing invalid path handling for Windows dialogs.
Could you please clarify:
- Why is the linting job being removed?
- Is there an alternative linting solution in place?
- How will code quality be maintained without these checks?
| @@ -56,7 +56,7 @@ jobs: | |||
| strategy: | |||
| matrix: | |||
| os: [ubuntu-22.04, windows-latest, macos-latest, ubuntu-24.04] | |||
| go-version: ['1.21'] | |||
| go-version: ['1.23'] | |||
There was a problem hiding this comment.
Invalid Go version specified
The specified Go version '1.23' is not yet released and may not be available. This could cause the workflow to fail.
Use a currently available Go version:
- go-version: ['1.23']
+ go-version: ['1.21']📝 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.
| go-version: ['1.23'] | |
| go-version: ['1.21'] |
Description
Updates the file dialog code so that invalid default paths don't crash the app.
Fixes #3804
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Chores
1.21to1.23