feat: implement EnsureDirectoryExists method to verify and create directories as needed#84
Conversation
…ectories as needed
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 50 minutes and 50 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds EnsureDirectoryExists to IFileProvider and implements it in CommonFileProvider and ZipFileProvider. SaveLocationManager now treats directories without explicit paths as writable when appropriate and calls EnsureDirectoryExists to create/verify the target directory before finalizing the save path. Tests added for providers and builder flows. ChangesDirectory Existence and Writeability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0778cd8fec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| CanWriteDir = fileProvider.CanWriteToDirectory(p.Path) | ||
| || !fileProvider.DirectoryExists(Path.GetDirectoryName(p.Path) ?? ""), |
There was a problem hiding this comment.
Treating every non-existent directory as writable (CanWriteDir = ... || !DirectoryExists(...)) can select an unusable path ahead of a valid fallback at the same priority (the default case), because selection now prefers registration order among those true values and then immediately throws if EnsureDirectoryExists fails. In practice, if the first configured path points to a directory that does not exist and cannot be created (permissions/invalid parent) while a later path is writable, Build fails instead of choosing the later writable location, which is a regression from the previous ranking behavior.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Configuration.Writable.Core/Configure/SaveLocationManager.cs`:
- Around line 80-84: The current CanWriteDir logic in SaveLocationManager marks
non-existent paths as writable which can cause Build(...) to try creating an
uncreatable path and fail instead of trying lower-priority locations; change the
condition so CanWriteDir is true only when the directory already exists and
fileProvider.CanWriteToDirectory(p.Path) returns true (i.e., require
fileProvider.DirectoryExists(dir) && fileProvider.CanWriteToDirectory(p.Path)),
so non-existent directories are not pre-marked writable and the selection can
fall back to other configured locations.
In `@src/Configuration.Writable.Core/FileProvider/CommonFileProvider.cs`:
- Around line 278-282: The current logic in CommonFileProvider where directory
is obtained via Path.GetDirectoryName(path) incorrectly treats null/empty as
invalid (causing valid relative filenames like "file.json" to fail); update the
method (the block using Path.GetDirectoryName(path) and the subsequent
write-check) to default directory to the current directory when GetDirectoryName
returns null or empty (e.g., set directory = "." or
Directory.GetCurrentDirectory()) instead of returning false so the existing
write-permission/check logic proceeds for files in the current directory.
In `@src/Configuration.Writable.Core/FileProvider/ZipFileProvider.cs`:
- Around line 108-112: The code incorrectly treats relative targets as
unwritable when Path.GetDirectoryName(zipPath) returns null/empty; instead
normalize the path and treat the current directory as the fallback: compute var
full = Path.GetFullPath(zipPath) and then var directory =
Path.GetDirectoryName(full); if directory is null or empty set directory =
Directory.GetCurrentDirectory(); then continue the existing writability checks
using that directory (references: zipPath, directory, use Path.GetFullPath and
Directory.GetCurrentDirectory in ZipFileProvider.cs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85ccf6f1-154a-418a-bd84-e1a39b5b4ebc
📒 Files selected for processing (9)
src/Configuration.Writable.Core/Configure/SaveLocationManager.cssrc/Configuration.Writable.Core/FileProvider/CommonFileProvider.cssrc/Configuration.Writable.Core/FileProvider/IFileProvider.cssrc/Configuration.Writable.Core/FileProvider/ZipFileProvider.cstests/Configuration.Writable.Tests.PublicApi/Approvals/PublicApiCheck.Check.Configuration.Writable.Core.approved.txttests/Configuration.Writable.Tests/CommonFileWriterTests.cstests/Configuration.Writable.Tests/Utility/InMemoryFileProvider.cstests/Configuration.Writable.Tests/WritableOptionsConfigBuilder.cstests/Configuration.Writable.Tests/ZipFileProviderTests.cs
- SaveLocationManager: Only consider existing directories as writable in CanWriteDir evaluation, so Build() falls back to other locations if EnsureDirectoryExists fails for the selected path - CommonFileProvider / ZipFileProvider: Default to current directory when GetDirectoryName returns null/empty (e.g. relative filenames like 'file.json'), instead of returning false - Test: EnsureDirectoryExists_ShouldReturnFalseForInvalidPath now uses a UNC path guaranteed to be inaccessible rather than a relative path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Configuration.Writable.Core/Configure/SaveLocationManager.cs`:
- Around line 80-86: The current check sets CanWriteDir using
fileProvider.CanWriteToDirectory(".") which can false-negative; instead, when
p.Path has no directory part (filename-only), call
fileProvider.CanWriteToDirectory with a file-target probe (e.g., the filename
portion of p.Path or a generated probe filename) so the provider receives a
file-like path and can infer writability correctly; update the CanWriteDir
expression in SaveLocationManager (the block that computes CanWriteDir for
p.Path) to use Path.GetFileName(p.Path) or a probe filename rather than "." when
Path.GetDirectoryName(p.Path) is empty.
In `@tests/Configuration.Writable.Tests/CommonFileWriterTests.cs`:
- Around line 315-317: Replace the non-deterministic UNC path used in the test
with a platform-agnostic, immediately-invalid path so Directory.CreateDirectory
fails deterministically; specifically, change the invalidPath variable in
CommonFileWriterTests (the value passed into writer.EnsureDirectoryExists) to a
string containing an invalid path character (e.g., a NUL character or use
Path.GetInvalidPathChars to build an invalid name) and keep the
Assert/expectation the same so the test fails fast on all CI platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7641e1d6-dc70-4cb6-a225-416aba229e57
📒 Files selected for processing (4)
src/Configuration.Writable.Core/Configure/SaveLocationManager.cssrc/Configuration.Writable.Core/FileProvider/CommonFileProvider.cssrc/Configuration.Writable.Core/FileProvider/ZipFileProvider.cstests/Configuration.Writable.Tests/CommonFileWriterTests.cs
|
1 participant
Summary by CodeRabbit
New Features
Tests