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
Assessment against linked issues
Suggested reviewers
Poem
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.
| { | ||
| var tempOutput = File.OpenWrite(TempFilePath); | ||
| await JsonSerializer.SerializeAsync(tempOutput, Data, | ||
| new JsonSerializerOptions | ||
| { | ||
| WriteIndented = true | ||
| }); | ||
| new JsonSerializerOptions { WriteIndented = true }); | ||
| AtomicWriteSetting(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix resource management and async/sync mixing issues.
The current implementation has several critical issues:
- The FileStream is not properly disposed
- AtomicWriteSetting is called synchronously in an async method
- The file might be left in an inconsistent state if an error occurs
Apply this fix:
public async Task SaveAsync()
{
- var tempOutput = File.OpenWrite(TempFilePath);
- await JsonSerializer.SerializeAsync(tempOutput, Data,
- new JsonSerializerOptions { WriteIndented = true });
- AtomicWriteSetting();
+ await using (var tempOutput = File.OpenWrite(TempFilePath))
+ {
+ await JsonSerializer.SerializeAsync(tempOutput, Data,
+ new JsonSerializerOptions { WriteIndented = true });
+ await tempOutput.FlushAsync();
+ }
+ await Task.Run(AtomicWriteSetting);
}📝 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.
| { | |
| var tempOutput = File.OpenWrite(TempFilePath); | |
| await JsonSerializer.SerializeAsync(tempOutput, Data, | |
| new JsonSerializerOptions | |
| { | |
| WriteIndented = true | |
| }); | |
| new JsonSerializerOptions { WriteIndented = true }); | |
| AtomicWriteSetting(); | |
| } | |
| { | |
| await using (var tempOutput = File.OpenWrite(TempFilePath)) | |
| { | |
| await JsonSerializer.SerializeAsync(tempOutput, Data, | |
| new JsonSerializerOptions { WriteIndented = true }); | |
| await tempOutput.FlushAsync(); | |
| } | |
| await Task.Run(AtomicWriteSetting); | |
| } |
@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 ...
|
fix #3011
cc helmesjo