Repository navigation
fix: store --local state in the project's .cloudflare/state directory - #525
Conversation
--local commands now default to .cloudflare/state next to the nearest cloudflare.config.ts, matching where cf dev writes, and fail with a --persist-to hint outside a project.
commit: |
|
LGTM |
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 291.2 ms | 8.7 ms | 282.4 ms | 311.6 ms |
| base | 288.1 ms | 3.8 ms | 281.2 ms | 294.7 ms |
Δ (PR vs base): ↑ +3.1 ms (+1.1%)
Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 1 findings. Posted 1 inline suggestion. |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
da73a843.
This review, of 40906911
💬 Approved with comments — The code change is correct and tested. One WARNING: the changeset should be minor and should explain the old state location, because this changes pre-1.0 --local behaviour.
Findings: 1 warning (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #525.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Release] The changeset uses `patch` for a pre-1.0 behaviour change: `--local` now fails outside a project, and existing global `--local` state is no longer read by default.
- **File:** .changeset/local-project-state.md:2
🚀 Release: ⚠️ 1 warning
The changeset marks a pre-1.0 behaviour change as patch.
Recommendation: 📝 Minor Comments
- [WARNING] The changeset uses
patchfor a pre-1.0 behaviour change:--localnow fails outside a project, and existing global--localstate is no longer read by default. (.changeset/local-project-state.md:2)
View full review
.changeset/local-project-state.md:2 uses patch. The repository's changeset rules say pre-1.0 behaviour changes use minor, and this change does two things that affect existing users:
--localnow fails outside a project unless--persist-tois given.- State written by earlier
--localruns in the global config directory (<cf config dir>/state/v3) is no longer read. Inside a project, those users will see an empty store.
Use minor, and add a sentence to the body telling users how to reach their old state (for example --persist-to <cf config dir>/state).
✅ All other checks passed
🔒 Security: ✅ LGTM
The new default path comes from locating the project config and joining fixed segments. No untrusted input reaches it, and --persist-to handling is unchanged.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md update matches the new behaviour.
📋 Code Quality: ✅ LGTM
resolveProjectPersistRoot walks up to the nearest cloudflare.config.ts and joins fixed path segments. resolvePersistRoot still creates the directory tree when it is missing. The new error reaches the user through the existing local dispatch error path, and the e2e tests cover both the in-project default and the error outside a project. The orphaned global state is covered under the release section.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The docs/architecture.md and help-text updates match the new default and the error outside a project.
Reviewed commit: 40906911
There was a problem hiding this comment.
Superseded by the review of
fb6b99eb.
This review, of da73a843
✅ Approved — The changeset warning from the last review is fixed (it is now minor and explains how to reach the old state), and nothing new came up.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
The changeset now uses minor, as the repository's rules require for a pre-1.0 behaviour change. Its body says how to keep using the previous global state with --persist-to. The earlier warning is resolved.
🔒 Security: ✅ LGTM
No issues found. The default path is built from the project config's location plus fixed path segments.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md update matches the new behaviour.
📋 Code Quality: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No performance concerns.
📝 Documentation: ✅ LGTM
The docs/architecture.md and help-text updates match the new default and the error outside a project.
Reviewed commit: da73a843
|
Review: 1 findings. Posted 1 inline suggestion. |
|
LGTM |
There was a problem hiding this comment.
Superseded by the review of
4a6d59ac.
This review, of fb6b99eb
✅ Approved — The changeset and source-comment wording changes since the last review are accurate, and no new correctness, security or release issues came up.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
The changeset uses minor, as the repository's rules require for a pre-1.0 behaviour change. It says how to keep using the previous global state with --persist-to, and it only claims shared state with the Vite-plugin dev server.
🔒 Security: ✅ LGTM
No issues found. The default path is built from the project config's location plus fixed path segments.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md update matches the new behaviour.
📋 Code Quality: ✅ LGTM
resolveProjectPersistRoot finds the nearest cloudflare.config.ts and adds the fixed .cloudflare/state path to that directory. Outside a project it throws an error that suggests --persist-to, and the explicit --persist-to path works as before. The e2e test writes with the default path and reads the data back through --persist-to .cloudflare/state, which shows where the default lands.
⚡ Performance: ✅ LGTM
Each --local command does one more config lookup, which costs almost nothing.
📝 Documentation: ✅ LGTM
The docs/architecture.md and help-text updates match the new default and the error outside a project. The changeset line is now scoped to the Cloudflare Vite plugin.
Reviewed commit: fb6b99eb
|
LGTM |
There was a problem hiding this comment.
✅ Approved — The new test correctly covers --persist-to overriding the project state directory, and the rest of the change has no correctness, security or release issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
The changeset uses minor for this pre-1.0 behaviour change and says how to reach the previous global state with --persist-to. Risk: low.
🔒 Security: ✅ LGTM
No issues found. The default path is built from the project config's location plus fixed path segments.
📖 AGENTS.md: ✅ LGTM
The packages/cli/AGENTS.md update matches the new behaviour.
📋 Code Quality: ✅ LGTM
The new test lets --persist-to override the project state directory puts a cloudflare.config.ts in a temp directory and runs a --local put with --persist-to state. It then checks that state/v3 exists and .cloudflare/state does not. That matches resolvePersistRoot, where an explicit --persist-to skips the project lookup.
⚡ Performance: ✅ LGTM
Each --local command does one extra config lookup, which costs almost nothing.
📝 Documentation: ✅ LGTM
The docs/architecture.md and help-text updates match the new default and the error outside a project.
Reviewed commit: 4a6d59ac
Fix #224.
--localcommands now use.cloudflare/statenext to the nearestcloudflare.config.tsinstead of the global state directory. That's the same directorycf devwrites to through the Cloudflare Vite plugin, so--localsees the dev server's data. Outside a project they fail and suggest--persist-to <dir>, which works as before.