Keep the cache root out of git on the build path, and document it - #3606
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change documents generated ChangesCache directory support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5512b80928
ℹ️ 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".
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 `@docs/guides/project-structure.md`:
- Around line 194-198: Update the `.cache/.gitignore` documentation to state
that Veryfront creates the self-ignoring file only when it is absent and
preserves any existing file without adding `*` to it; clarify this behavior for
both commands described in the section.
In `@src/build/production-build/build/build-orchestrator.ts`:
- Around line 88-96: Update the build orchestration around
ensureCacheDirIgnored() and generateLocalReleaseAssetManifest() to resolve one
cache root and reuse it for both the ignore marker and generated build caches,
including when options.projectDir differs from cwd(). Preserve explicit cache
overrides, and add coverage for the default path without an explicit cache
override.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e99f20b-62cc-4e80-b200-c766e1d9cacf
📒 Files selected for processing (4)
docs/guides/project-structure.mdsrc/build/production-build/build/build-orchestrator.tstests/docs/guide-content.test.tstests/integration/server/build/build.test.ts
Outside production the cache root is `<project>/.cache`, so `veryfront build` drops generated bundles into the user's project. #3581 wrote a self-ignoring `.cache/.gitignore` from `clearAllLocalCaches()`, which only `veryfront dev`, `start`, and `serve` call, so the build path was left uncovered: a project that adopted Veryfront into an existing tree still saw ten untracked `.mjs` files after one `veryfront build`, and `git add -A` committed them. Mark the cache root from `buildProduction()` instead of from another entry point's startup, so the CLI build, the MCP build tool, and a direct API call are all covered. It goes there rather than in `setupBuildDirectories()` because a dry run skips that step and still populates the cache root. Also document `.cache/` in the project-structure guide. Until now the only explanation a developer got for the directory appearing in their tree was the comment inside the generated `.gitignore`; the guide now names both cache subdirectories, states that the directory ignores itself, and documents `VERYFRONT_CACHE_DIR` for moving it out of the project. Verified against the published 0.1.1229 repro: a hand-built adopt-path project (own .gitignore listing only node_modules/, dist/, .env) is left with a clean `git status --porcelain -uall` after `veryfront build` and after `veryfront build --dry-run`, where 0.1.1229 leaves ten untracked bundles.
The cwd-relative-test-reads audit ratchets per file, and reading the guide by cwd-relative path added a 30th racy read to tests/docs/guide-content.test.ts.
Review follow-up: `ensureCacheDirIgnored()` deliberately leaves an existing `.cache/.gitignore` alone, so the guide must not promise that every run writes a catch-all there.
0d52ec3 to
2d2b242
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
kwakayama
left a comment
There was a problem hiding this comment.
- [P3] Correct the production cache-location guidance —
docs/guides/project-structure.md:185says the CLI writes generated directories into the project root, and:191-200says both commands create project.cache/. Butsrc/utils/cache-dir.ts:85-102selects$HOME/.cache/veryfrontwhenNODE_ENVorVERYFRONT_MODEisproduction(unless overridden). The documentation test attests/docs/guide-content.test.ts:568-580forces a cache context and only checks directory names, so it cannot catch this default-production mismatch. Document the production location and test both modes.
| Area | Score |
|---|---|
| Correctness | 40/40 |
| Tests | 19/20 |
| Reliability/security | 15/15 |
| Maintainability | 15/15 |
| Scope/docs | 5/10 |
Review-Gate:
Reviewer: Codex
Reviewed-SHA: 2d2b242
Score: 94/100
Actionable-Findings: 1
Verdict: COMMENT
The guide described one cache location, the project's `.cache/`. There are two: `getDefaultCacheBaseDir()` (src/utils/cache-dir.ts) returns `$HOME/.cache/veryfront` when `NODE_ENV` or `VERYFRONT_MODE` is `production` and `HOME` is set, so every deployed run wrote somewhere the guide did not name. The documentation test could not have caught that. It forced a cache context with `runWithCacheDir()` and then asserted on `basename()` of the subdirectory paths, which answers with the forced root in both modes -- the default resolution the guide describes was never exercised. It now calls the real `getCacheBaseDir()` once per mode with the overrides cleared, asserts the two roots actually differ before asserting anything about the prose, and reads the trigger variables out of `getDefaultCacheBaseDir()` so a renamed trigger fails here instead of leaving readers checking a variable the code stopped consulting.
|
[P3] cache-location guidance — fixed in 9a553d2. Citation confirmed. Doc. Test. You were right about why the old one could not fail. It wrapped everything in The replacement drops the forced context and calls the real
Evidence it distinguishes the modes — three runs of
Both mutations were reverted; the commit touches only the guide and the test, and Resolving. Leaving the PR out of the merge queue for you. |
ensureCacheDirIgnored() resolves through getCacheBaseDir(), so it leaves an existing marker alone in the home cache root exactly as it does in the project's. Saying so only about `.cache/.gitignore` invited a production reader to assume the opposite.
|
Addendum, d5b2bc3: while re-reading the section I found one more instance of the same species of error and fixed it rather than leaving it. The marker paragraph asserted "a |
The validator this PR adds found three em dashes that reached main while no authoring-time check existed: two from #3606 in project-structure.md and one from #3616 in deploy-project.md. Rewritten as sentences and parentheses with the meaning unchanged. The rebase also took main's 'Verify it worked' section rather than this branch's older copy, so #3616's reviewed route-probing guidance survives.
The backfill replayed a "Generated directories" section from the docs mirror into project-structure.md, but this repo already had a newer one: #3606 landed the production cache root and VF_CACHE_DIR here after the mirror copy was written. The result was two H2 sections with the same heading giving different answers, the later one claiming the CLI writes .cache/ into the project root with no mention of production. Drop the stale copy; the retained section is a strict superset of it. Also rewords the summary line above that section: "`.cache/` is too during development" elided its verb across a sentence boundary. coding-agents.md still stated the MCP port as `--port + 2` in two places. The earlier fix only corrected the verification section, and one of the two misses was the "Port already in use" heading, which is precisely the case where the requested port and the bound port differ. mcpPort is computed from started.port (cli/commands/dev/command.ts:240-243), so a --port 4000 that falls forward to 4001 serves MCP on 4003.
What the round-2 verification found
Finding 16 was filed as "
veryfront devwrites an undocumented.cache/tree into the host project root and never adds a.gitignoreentry" and marked partially-fixed: the git-hygiene half looked fixed, the documentation half did not.Reproducing on the published 0.1.1229 showed the git-hygiene half is only fixed on the path the verification happened to test.
Adopt-path project (hand-built per the Installation doc, own
.gitignorelisting onlynode_modules/,dist/,.env,git init+ commit), publishedveryfront@0.1.1229:git status --porcelain -uallveryfront dev+ one page loadveryfront build.cache/**.mjsfilesveryfront build --dry-run.cache/**.mjsfiles.cache/.gitignoreis simply absent after a build.Why the previous fix missed it
#3581 diagnosed the problem correctly but hooked the repair to the wrong lifecycle. It calls
ensureCacheDirIgnored()fromclearAllLocalCaches(), and onlyveryfront dev,veryfront start, andveryfront servecall that — they are the commands that boot a server.veryfront buildnever boots a server, so it writesveryfront-mdx-esm/andveryfront-http-bundle/into<project>/.cachewith no marker in sight. The fix was verified through the dev path only, which is exactly the path it covered.The fix
buildProduction()marks the cache root itself. That is the single library entry the CLI build, the MCP build tool, and a direct API call all funnel through. It is not insetupBuildDirectories(), because a dry run returns from that step early and still populates the cache root (verified above).Documentation half
The project-structure guide gains a Generated directories section: what
.cache/holds (named after the real subdirectories, pinned by a test), that it ignores itself so an adopted project never has to edit its own.gitignore, that deleting it is safe, andVERYFRONT_CACHE_DIRfor keeping generated bundles out of the project tree entirely. Every claim was checked against published-CLI behaviour, including thatVERYFRONT_CACHE_DIRrelocates the whole root and leaves the project clean.Live URL that must show this after the docs sync lands: https://veryfront.com/code/guides/project-structure (section "Generated directories").
docs/code/guides/in veryfront-docs is overwritten wholesale from this repo by.github/workflows/sync-docs.yml→update-reference.yml, so this repo is the source of truth; veryfront-docs#375 carries the same text so the live page does not wait on the sync run.Tests
tests/integration/server/build/build.test.ts— new case runsbuildProduction()insiderunWithCacheDir()and asserts the cache root ends up with a.gitignorecontaining*. Confirmed red before the fix (AssertionError: Values are not equal) and green after.tests/docs/guide-content.test.ts— pins the new guide section to the real cache layout viabasename(getMdxEsmCacheDir())/basename(getHttpBundleCacheDir()), so renaming a cache subdirectory fails here instead of silently stranding the guide.Proof the original symptom is gone
Same sandbox project, same commands, this branch's build instead of 0.1.1229:
Summary by CodeRabbit
Documentation
.cache/anddist/directories, including their contents, regeneration, safe deletion, and cache-directory configuration.Bug Fixes
Tests
.gitignorecreation during production builds.