[v3] Normalize Windows paths on system Dialogs invocations #4188
Conversation
JS code ends up escaping Windows paths when data gets passed around from backend to the frontend, so those should be normalized beforehand.
WalkthroughThis pull request enhances error handling and result processing in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Dialog as Dialog Function
participant Multi as multi.ShowAndGetResults
participant Single as dlg.ShowAndGetResult
participant Cleaner as filepath.Clean
Caller->>Dialog: Invoke showCfdDialog(newDlg, isMultiSelect)
alt Multi-select enabled
Dialog->>Multi: Call multi.ShowAndGetResults()
Multi-->>Dialog: Return paths OR error
alt Error Occurs
Dialog-->>Caller: Return nil, error
else
Dialog->>Cleaner: Clean each path in paths
Cleaner-->>Dialog: Cleaned paths
Dialog-->>Caller: Return cleaned paths, no error
end
else
Dialog->>Single: Call dlg.ShowAndGetResult()
Single-->>Dialog: Return path OR error
alt Error Occurs
Dialog-->>Caller: Return nil, error
else
Dialog->>Cleaner: Clean the path
Cleaner-->>Dialog: Cleaned path
Dialog-->>Caller: Return cleaned path, no error
end
end
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
docs/src/content/docs/changelog.mdx (1)
114-114: Typographical Error in Changelog Entry.
The entry on line 114 reads "Fixed Dialogs rutime function returning escaped paths on Windows". Please correct "rutime" to "runtime" for clarity and accuracy in documentation.
|
Thanks for taking the time to open this. Could we extract this processing into a function and write a test to make sure it does what we think it does? I just want to make sure that |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
v3/pkg/application/dialogs_windows_test.go (3)
12-57: Test cases cover main Windows path scenarios but could be expanded.The table-driven test approach is well-structured and covers several Windows path edge cases. However, consider adding a few additional test cases for completeness:
- Paths with spaces (common in Windows)
- Paths with dot and double-dot elements (e.g.,
C:\folder\.\file.txtorC:\folder\..\other\file.txt)- Paths with environment variables or Windows-specific prefixes (e.g.,
\\?\C:\for long paths)}{ { name: "path with double separators", inputPath: `C:\\temp\\folder`, expected: `C:\temp\folder`, }, { name: "path with forward slashes", inputPath: `C://temp//folder`, expected: `C:\temp\folder`, }, { name: "path with trailing separator", inputPath: `C:\\temp\\folder\\`, expected: `C:\temp\folder`, }, { name: "path with escaped tab character", inputPath: `C:\\Users\\test\\tab.txt`, expected: `C:\Users\test\tab.txt`, }, { name: "newline character", inputPath: `C:\\Users\\test\\newline\\n.txt`, expected: `C:\Users\test\newline\n.txt`, }, { name: "UNC path with multiple separators", inputPath: `\\\\\\\\host\\share\\test.txt`, expected: `\\\\host\share\test.txt`, }, + { + name: "path with spaces", + inputPath: `C:\\Program Files\\My App\\file.txt`, + expected: `C:\Program Files\My App\file.txt`, + }, + { + name: "path with dot element", + inputPath: `C:\\folder\\.\\file.txt`, + expected: `C:\folder\file.txt`, + }, + { + name: "path with double-dot element", + inputPath: `C:\\folder\\..\\other\\file.txt`, + expected: `C:\other\file.txt`, + }, }
35-38: Misleading test case name for "path with escaped tab character".This test case doesn't actually contain an escaped tab character (
\t) in the path. It's just testing a path with a file named "tab.txt". Consider renaming this test case to better reflect what's being tested.{ - name: "path with escaped tab character", + name: "path with word 'tab' in filename", inputPath: `C:\\Users\\test\\tab.txt`, expected: `C:\Users\test\tab.txt`, },
40-43: Misleading test case name for "newline character".Similar to the previous comment, this test case doesn't contain an actual newline character (
\n) in the path. It's testing a path with a file named "newline\n.txt". Consider renaming this test case to better reflect what's being tested.{ - name: "newline character", + name: "path with 'newline\\n' in filename", inputPath: `C:\\Users\\test\\newline\\n.txt`, expected: `C:\Users\test\newline\n.txt`, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/pkg/application/dialogs_windows_test.go(1 hunks)
🔇 Additional comments (2)
v3/pkg/application/dialogs_windows_test.go (2)
1-10: The build constraint and package setup looks good.This test file is correctly configured to run only on Windows, which is appropriate given the purpose of normalizing Windows paths. The imports are minimal and appropriate for a test file.
12-57:✅ Verification successful
Verify that filepath.Clean is the appropriate function for this use case.
Based on the PR objectives and comment from leaanthony, ensure that
filepath.Cleanis the appropriate function for normalizing Windows paths. Your test confirms the behavior offilepath.Clean, but you might want to consider if there are any edge cases specific to your application's dialog paths that might not be handled correctly by this function.
🏁 Script executed:
#!/bin/bash # Look for other path normalization functions in the codebase to confirm the approach rg "func\s+.*[Nn]ormalize.*[Pp]ath" --type go rg "Clean\s*\(" --type go -A 2 -B 2 | grep -i pathLength of output: 2479
Action: Confirmed appropriate use of filepath.Clean
The test cases in
v3/pkg/application/dialogs_windows_test.goconfirm thatfilepath.Cleanbehaves as expected for Windows paths, and a scan of the codebase shows its widespread use for similar normalization tasks. There don’t appear to be any edge cases specific to our dialog paths that require a different function. Continue usingfilepath.Cleanwhile keeping an eye out for any future path-specific nuances.
Hey there! I started to tackle this, mocking the function within the test file, but ultimately I found it was more straightforward to test the function directly with the sort of data that would cause problems once passed through the JavaScript boundary. |
Description
JS code ends up escaping Windows paths when data gets passed around from backend to the frontend, so those should be normalized beforehand.
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.
Checklist:
website/src/pages/changelog.mdxwith details of this PRSummary by CodeRabbit
Bug Fixes
Tests