resolve link before using File.Replace #3177
Conversation
|
🥷 Code experts: jjw24 jjw24 has most 🧠 knowledge in the files. See details
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
This comment has been minimized.
This comment has been minimized.
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThe changes in the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
✨ Finishing Touches
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
🔭 Outside diff range comments (1)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (1)
Add test coverage for JsonStorage class
The JsonStorage class lacks test coverage for critical functionality:
- JSON file operations (read/write)
- Backup and restore mechanisms
- Atomic write operations
- New symbolic link handling
Please add tests in
Flow.Launcher.Testdirectory covering these scenarios to ensure reliability.🔗 Analysis chain
Line range hint
34-211: Verify test coverage for symbolic link scenarios.The changes handle symbolic links, but we should ensure proper test coverage exists.
Run this script to check for test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for test files and symbolic link test scenarios echo "Searching for JsonStorage test files..." rg -l "JsonStorage.*Test" echo "Checking for symbolic link test scenarios..." rg -i "symlink|linktar?get" --type csLength of output: 390
Script:
#!/bin/bash # Broader search for related test files echo "Searching for storage/settings related test files..." rg -l "Storage.*Test|Settings.*Test" --type cs echo "Searching for file system test utilities..." rg -l "TestFile|FileSystem.*Test|Mock.*File" --type cs echo "Checking test directory structure..." fd -t d "test|Test"Length of output: 442
Script:
#!/bin/bash # Examine test directory content echo "Listing test directory files..." fd . Flow.Launcher.Test/ -t f -e cs echo "Searching test files for relevant content..." rg -l "JsonStorage|Infrastructure.*Test|Settings|Json.*Test" Flow.Launcher.Test/ echo "Searching for JSON deserialization test scenarios..." rg -C2 "JsonSerializer|Deserialize|WriteAllText|ReadAllText" Flow.Launcher.Test/Length of output: 1722
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (2)
34-41: LGTM! Consider improving the error message.The constructor properly validates the file path and directory. However, the error message could be more specific.
- DirectoryPath = Path.GetDirectoryName(filePath) ?? throw new ArgumentException("Invalid file path"); + DirectoryPath = Path.GetDirectoryName(filePath) ?? throw new ArgumentException("File path must include a valid directory", nameof(filePath));
Line range hint
200-211: Enhance symbolic link handling robustness.While the changes correctly handle basic symbolic link scenarios, consider adding additional safeguards and logging.
private void AtomicWriteSetting() { if (!File.Exists(FilePath)) { File.Move(TempFilePath, FilePath); } else { - var finalFilePath = new FileInfo(FilePath).LinkTarget ?? FilePath; + var fileInfo = new FileInfo(FilePath); + var finalFilePath = fileInfo.LinkTarget ?? FilePath; + + if (fileInfo.LinkTarget != null) + { + Log.Debug($"Resolving symbolic link: {FilePath} -> {finalFilePath}"); + + if (!File.Exists(finalFilePath)) + throw new FileNotFoundException($"Symbolic link target not found: {finalFilePath}"); + } + File.Replace(TempFilePath, finalFilePath, BackupFilePath); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
🔇 Additional comments (1)
Flow.Launcher.Infrastructure/Storage/JsonStorage.cs (1)
184-190: LGTM! The Save method looks good.The serialization options are properly configured, and the method correctly uses atomic write operations.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
taooceros
requested review from
Jack251970,
VictoriousRaptor,
Yusyuriv and
jjw24
fix #3011
cc helmesjo