Repository navigation
Conversation
…d drop stale chunks (--clean) The js step is two Rollup passes: the view pass (bootstrap and every client.js) and the Kiln pass (every model.js, kiln.js and plugin). A full build runs both, about 15 s and 2.2 GB in a branch pod; the view pass alone is 6.5 s and 1.4 GB, the Kiln pass alone 9.5 s and 1.7 GB. After each pass the build now records the project modules it bundled and the files it wrote in .clay/vite-deps.json. clay vite --only js --changed <files> looks each file up and runs only the pass or passes that bundled it, keeping the other pass's manifest entries and client-env names. A file no pass bundled, a missing index or a missing manifest rebuilds both. --clean deletes files in public/js/chunks and public/js/.clay that neither pass's last build wrote; it is opt-in because production relies on old hashed files staying.
|
coverage: 86.537% (+1.0%) from 85.553% — jordan/vite-js-changed into master |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: b3c9269527
ℹ️ 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".
…--changed Two review findings on --changed were real: - client-env.json was the union of the last file and the names the rebuilt pass found, so a name whose reference was removed never left until a full build. Each pass now has its own env collector and its names are kept in .clay/vite-deps.json; a partial build replaces the pass it ran and keeps the other's, and client-env.json is the union taken from the index. - A partial build trusted the index even when the options had changed (for example --minify), mixing output built under different settings. The index now stores a fingerprint of the resolved bundler config, NODE_ENV and the claycli version, and both passes run when it differs, is missing, or the index has no env names (one from an earlier build). The third finding (--clean would delete the sourcemaps of live chunks) does not reproduce: Rollup lists the .map files it writes among the output assets, so the index already has them. A test now runs a real build in a child process and checks that pruneStale keeps every map, so a bundler upgrade that stops listing them fails the suite.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9941ce08c9
ℹ️ 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".
…hat no longer matches the output Two more problems in the --changed / --clean index, found in review: - With kilnSplit, which builds everything in one pass, the index had only a view entry and --clean refuses to prune unless both passes are recorded, so it silently deleted nothing. The index now names the passes that make up a whole build (`passes`: both, or only the view pass under kilnSplit) and --clean prunes once all of them are recorded. - The old index stayed on disk while the manifest and client-env.json were rewritten, so a build that stopped halfway (or whose index could not be written) left an index describing the previous build. The next --changed build could then skip a pass whose output had changed, and --clean could delete a live file. The index is now removed just before results are written and written last, so one on disk always describes a finished build.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5855257eae
ℹ️ 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".
| * `partial` (whether that is fewer than a whole build). | ||
| */ | ||
| function planJsBuild(options, viteCfg) { | ||
| const index = incremental.readIndex(); |
There was a problem hiding this comment.
After a one-shot build creates this index, watch() can rewrite chunks, _manifest.json, and client-env.json at lines 1607–1608 and 1754–1755 without clearing or updating .clay/vite-deps.json. If the watcher is then stopped and a --changed build selects only one pass, this stale index is trusted for the skipped pass; its old env names replace the watch result, and adding --clean can delete the skipped pass's currently live chunks because only their pre-watch filenames are recorded. Clear the index when watch mode starts writing results, or maintain it from watch outputs.
Useful? React with 👍 / 👎.
…nv and installed dependencies Found by a second review of the whole --changed / --clean change, run against real builds in separate processes rather than only the mocked tests: - clay vite --watch rewrites chunks, _manifest.json and client-env.json without keeping .clay/vite-deps.json up to date, so a later --changed build trusted an index describing an older build (and --clean could delete live chunks). watch() now removes the index when it starts; the next build runs both passes. - CLAYCLI_COMPILE_MINIFIED makes the Vue plugin compile in production mode and is not part of the bundler config, so a partial build under a different value mixed modes. It is now part of the build fingerprint, with NODE_ENV. - A reinstall changes what the skipped pass would bundle, and the docs said it was not tracked, which made "only skips work that cannot change the output" false. The fingerprint now includes a hash of the install record in node_modules (npm's .package-lock.json, Yarn's .yarn-integrity, pnpm's .modules.yaml). Each new test fails without its change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cca24c12f6
ℹ️ 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".
| const whole = viteCfg.kilnSplit ? ['view'] : incremental.PASSES; | ||
| const passes = viteCfg.kilnSplit | ||
| ? whole | ||
| : incremental.choosePasses(options.changed, index, fs.existsSync(MANIFEST_FILE), config); |
There was a problem hiding this comment.
When _manifest.json exists but is malformed or lacks entries from the pass that would be skipped, this existence check still permits a partial build. readJsonOr() then returns null (or the incomplete object), and mergeManifest() writes only the rebuilt pass's entries, so the resulting manifest silently loses the skipped view or Kiln assets. Parse and validate the existing manifest before selecting a partial plan, falling back to both passes when it cannot preserve every skipped pass entry.
Useful? React with 👍 / 👎.
…k the kept output before skipping a pass Two more gaps in --changed, found by building a project with real .vue files and by the fourth Codex review: - public/css/_kiln-plugins.css is written by the Vue plugin from the style blocks its own process transformed. A partial build runs one pass, so it rewrote the file with only that pass's CSS: a view-only build dropped the Kiln plugin styles and a Kiln-only build dropped the view styles. Sites has 61 .vue files with styles across both passes. The index now records each pass's blocks (plugins/kiln-css.js, collected by pass at buildEnd), and a partial build puts the skipped pass's blocks back before the plugin writes the file. - The shortcut only checked that _manifest.json existed. A manifest that is unreadable or lacks the skipped pass's entry made mergeManifest drop that pass's assets, and a deleted file of the skipped pass went unnoticed. A pass is now skipped only if the manifest parses and holds its entry and every file the index recorded for it is on disk; otherwise both passes run. Each new test fails without its change; verified with real builds in separate processes.
jjpaulino
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a08aa0dc8
ℹ️ 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".
| // The output changes from here on, so the old index stops describing it: drop it now, write the new one last. | ||
| incremental.clearIndex(); |
There was a problem hiding this comment.
When one parallel Rollup pass writes its output but the other pass rejects, execution never reaches this clearIndex(), so the previous dependency index remains trusted despite the interrupted build. A subsequent --changed run for only the repaired pass can therefore skip the successful-but-uncommitted pass and preserve its old manifest and environment entries, silently omitting source changes from the deployed assets; clear the index after planning but before runJsPasses() begins writing output.
Useful? React with 👍 / 👎.
…ss cannot leave a trusted one The index was removed in writeJsResults, after both passes had finished. The two passes run in parallel, so one can write its output while the other fails; the build then rejected before reaching the removal and the previous index stayed on disk. A later --changed run for only the repaired pass skipped the pass that had written (but never committed to the manifest), silently leaving its source changes out of the deployed assets. The index is now removed right before the passes run (after the plan has read it) and still written last, so one on disk always describes a finished build, whether the build fails in a pass or stops halfway. A test makes the view pass throw after the Kiln pass wrote its output and checks that no index is left and the next build runs both passes; verified with real builds as well.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc2e84f45f
ℹ️ 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".
| for (const name of await fs.readdir(path.join(dest, folder))) { | ||
| const relative = `${folder}/${name}`; | ||
|
|
||
| if ((await fs.stat(path.join(dest, relative))).isFile()) onDisk.push(relative); |
There was a problem hiding this comment.
When Rollup or a configured plugin emits a nested filename such as chunks/vendor/foo-[hash].js or .clay/generated/foo-[hash].js, readdir() returns the child directory and the isFile() check skips it, so neither that file nor stale files from earlier builds are ever considered by staleFiles(). Walk these two output trees recursively so --clean fulfills its documented behavior for nested outputs.
Useful? React with 👍 / 👎.
What does this PR do?
Issue/Story
Adds
clay vite --only js --changed <files>: it rebuilds only the Rollup pass(es) that bundle the changed files, and--cleanto delete the previous builds' leftover chunks.Why are we doing this? Any context or related work?
The js step is two independent Rollup passes. The view pass bundles the bootstrap and every component and layout
client.js; the Kiln pass bundles everymodel.js,kiln.jsand Kiln plugin into one file. A full build runs both, though most edits feed only one. Measured on the sites repo, in a feature-branch pod (Node 26,--only js, three runs each):This is for sites' live updates to feature branches (a PR that copies a push's changed files into the running pod and rebuilds just those, nymag/sites#13761). With claycli it can ask for the pass it needs instead of running both. The memory peak matters there: the two passes together reached 2.76 GB at the container.
It is per pass, not per chunk. Chunk names carry content hashes that depend on the whole module graph, and Vite does not hand back Rollup's cache, so a single chunk cannot be rebuilt on its own from a separate process.
Where should a reviewer start?
lib/cmd/vite/incremental.js→choosePasses: the decision. A file is looked up in the index of what each pass bundled. Unknown file, no index, an incomplete index, a manifest or output that is not all there, or a different build config (bundler config,NODE_ENV,CLAYCLI_COMPILE_MINIFIED, installed dependencies, claycli version) means both passes, so the shortcut only skips work that cannot change the output.lib/cmd/vite/scripts.js→buildJSand the helpers it was split into (planJsBuild,runJsPasses,writeJsResults): the same build, with the manifest,client-env.jsonand index merged instead of replaced after a partial build.lib/cmd/vite/plugins/kiln-css.js(the Vue plugin's CSS state, moved out ofvue2.js), andkeepsOutput/skippedCssinincremental.js: what a partial build keeps from the pass it skips (its CSS, and a check that its output is still all there).Manual testing steps?
Run in a built site (so that
.clay/vite-deps.jsonexists after a first fullclay vite):Checked in a real pod with probe strings: a view edit appears only in the view output (Kiln bundle untouched), a model edit only in the Kiln bundle, the manifest keeps all its entries, every file it references exists, the site answers, and
--cleanremoved only stale files.Additional Context
--cleanis opt-in. Production never prunes hashed files on purpose (older HTML keeps referencing them), see CLAY-VITE.md. It only touchespublic/js/chunksandpublic/js/.clay, and does nothing unless every pass of the build is recorded (both passes, or the one pass underkilnSplit).client-env.jsonis written from the env names each pass recorded in the index: the pass that ran replaces its own names (a name nothing reads any more drops out) and the other pass keeps its names.public/css/_kiln-plugins.cssholds the style blocks of the.vuefiles of both passes, and the Vue plugin writes it from what its own process transformed. The index keeps each pass's blocks and a partial build puts the skipped pass's back before the file is written, so it ends up the same as after a full build.clay vite --watchremoves it too, because watch mode rewrites outputs without keeping it up to date.kilnSplit: trueis a single pass, so--changedchanges nothing there;--cleanstill works.incremental.test.jsandplugins/kiln-css.test.js(new), andbuildJScases inscripts.test.jsfor each of the above;npm testpasses (33 suites, 723 tests). Beyond the mocked tests it was run for real, in separate processes, on a small project with real Vite builds and.vuefiles with styles in both passes.--filesfor the styles step, and writing.clay/template-signalafter a templates build. Sites would then call oneclay vite --changedfor any change set.🤖 Generated with Claude Code