fix(blend): store remembered checkout in state file - #23
Conversation
Reviewer's GuideStores the remembered blend checkout directory in a new JSON state file alongside snapshots, prefers this state over the legacy config when resolving blend_dir, and simplifies the generated Nickel starter config to only manage sandbox settings while keeping legacy config readable for backward compatibility. Flow diagram for resolving blend_dir using state.json and legacy configflowchart TD
A[resolve_blend_dir] --> B{cli.blend_dir set?}
B -- yes --> C[use cli.blend_dir]
B -- no --> D{command is Init?}
D -- yes --> E{find_blend_dir_from_current_dir}
E -- found --> F[choice_from_current_dir]
E -- not found --> G{find_blend_dir_from_state_or_config}
D -- no --> H[find_blend_dir]
H --> I{find_blend_dir_from_current_dir}
I -- found --> F
I -- not found --> G
G --> J{state.read_blend_dir}
J -- Some --> K[use blend_dir from state.json]
J -- None --> L{find_blend_dir_from_config}
L -- Some --> M[use blend_dir from config]
L -- None --> N[bail: no blend_dir found]
F --> O[BlendDirChoice with update_config_after_success]
C --> P[BlendDirChoice from cli]
K --> Q[BlendDirChoice from state]
M --> R[BlendDirChoice from config]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The atomic write logic for the state file (tmp path construction, write, rename, cleanup on failure) duplicates similar patterns elsewhere in
StateStore; consider extracting a shared helper to reduce repetition and keep the behavior consistent. - The test
from_env_for_home_falls_back_to_supplied_homemanipulatesXDG_STATE_HOMEusingunsafeand restores it only on the happy path, so a panic before the restore will leak environment changes into other tests; consider using a scoped env helper/guard to ensure restoration happens even on failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The atomic write logic for the state file (tmp path construction, write, rename, cleanup on failure) duplicates similar patterns elsewhere in `StateStore`; consider extracting a shared helper to reduce repetition and keep the behavior consistent.
- The test `from_env_for_home_falls_back_to_supplied_home` manipulates `XDG_STATE_HOME` using `unsafe` and restores it only on the happy path, so a panic before the restore will leak environment changes into other tests; consider using a scoped env helper/guard to ensure restoration happens even on failure.
## Individual Comments
### Comment 1
<location path="blend/src/state.rs" line_range="147" />
<code_context>
+ };
+ std::fs::write(&tmp, raw)
+ .with_context(|| format!("failed to write state temp file {}", tmp.display()))?;
+ let rename_result = std::fs::rename(&tmp, &path).with_context(|| {
+ format!(
+ "failed to rename state file {} -> {}",
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `std::fs::rename` may fail on Windows if the target file already exists.
Because `std::fs::rename` errors on Windows when the destination already exists, repeated or concurrent calls to `write_blend_dir` may fail even though the operation is meant to be idempotent. To avoid this, consider explicitly removing the existing `path` before renaming (e.g. `std::fs::remove_file(&path)`), or introducing a platform-specific `replace_file` helper behind a `cfg` that provides atomic "replace" semantics across platforms.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }; | ||
| std::fs::write(&tmp, raw) | ||
| .with_context(|| format!("failed to write state temp file {}", tmp.display()))?; | ||
| let rename_result = std::fs::rename(&tmp, &path).with_context(|| { |
There was a problem hiding this comment.
issue (bug_risk): Using std::fs::rename may fail on Windows if the target file already exists.
Because std::fs::rename errors on Windows when the destination already exists, repeated or concurrent calls to write_blend_dir may fail even though the operation is meant to be idempotent. To avoid this, consider explicitly removing the existing path before renaming (e.g. std::fs::remove_file(&path)), or introducing a platform-specific replace_file helper behind a cfg that provides atomic "replace" semantics across platforms.
9695db0 to
ca7c69a
Compare
ca7c69a to
abcc8d2
Compare
Summary
state.jsonbeside snapshots instead of generated configblend_dirconfig readable as a runtime fallbackBlendOrderNickel contract soorders/blend/order.nclrejects stale config fields likeblend_dirTest
cargo fmt --checkcargo clippy -- -D warningscargo test