Repository navigation
Conversation
…e it waited for The test polls until the rebuilt module carries the new token, re-touching the theme on each poll. A re-touch made before the rebuild showed up queues one more rebuild, and the final assertion read the module from disk a second time; landing inside that rewrite (writeFileSync truncates first), it saw an empty module and the import line read null. The assertion now reads the snapshot the poll saw, which a rebuild wrote, so it still pins that the watch loop forwards --icons-specifier.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the snapshot doesn't stop this failure, because the module the poll accepts can itself lack the import. theme build renames its outputs into place, so no read sees a half-written module; instead, a rebuild whose second read of the theme lands inside the test's in-place re-touch (fs.writeFileSync(themeFile, touched) truncates first) exits 0 with #0a0b0c and no import line, because the testIcons registry is empty, and the snapshot keeps that module. Replaying the test's watch flow with a wider gap between that truncate and write reproduces expected null to be 'import { testIcons } from "./icons.mjs"' at the new assertion, and staging the re-touch in a sibling file and renaming it over the theme makes it pass (the watcher already follows renames). Please make the re-touch atomic and update the comment above rebuiltModule to describe this race.
[Automated review]
| // The rebuilt module as read when the new token showed up. The | ||
| // assertion below reads this snapshot, not the file again: a re-touch | ||
| // in this loop can queue one more rebuild, and a second read can land | ||
| // inside that rewrite (writeFileSync truncates first) and see an empty | ||
| // module. |
There was a problem hiding this comment.
This mechanism can't occur at this head. theme build writes each output to a sibling temporary file and renames it over the old one (packages/cli/api/integration/add-helpers.mjs:357-388), so a read always sees a complete module.
The import-less module comes from the rebuild the poll observed. Its second read of the theme (packages/cli/api/theme/build/build.mjs:2688) can land inside the in-place re-touch below, and with the empty testIcons registry that build still exits 0, without the import.
There was a problem hiding this comment.
You're right: theme build renames its outputs into place, so no read of the module was ever half-written. The import-less module came from a rebuild whose second read of the theme landed inside the test's in-place re-touch. b033699 makes each touch atomic, and the comment above rebuiltModule now describes that race.
| try { | ||
| if (fs.readFileSync(builtFile, 'utf-8').includes('#0a0b0c')) | ||
| const generated = fs.readFileSync(builtFile, 'utf-8'); | ||
| if (generated.includes('#0a0b0c')) { |
There was a problem hiding this comment.
The token is the only gate here, so this accepts the import-less module described above, and rebuiltModule keeps it. Staging the re-touch and renaming it over the theme removes the race, and the watcher already follows renames (packages/cli/clients/cli/commands/theme.mjs:206-219):
const staged = `${themeFile}.tmp`;
fs.writeFileSync(staged, touched);
fs.renameSync(staged, themeFile);There was a problem hiding this comment.
Done in b033699, as suggested: each touch is staged in a sibling .tmp file and renamed over the theme. A scratch copy of the test whose in-place re-touch holds its truncate open for 300 ms fails 5 of 5 runs with the CI error. The same copy with the atomic touch, its staged file held open the same 300 ms, passes 5 of 5.
…omically A rebuild reads the theme twice: it loads the module, then scans its source for the icon import. The test's re-touch was an in-place writeFileSync, which truncates first, so a re-touch landing between those two reads gave a rebuild an empty source to scan; with the test's empty icon registry that build exits 0 with the new token and no import line, and the poll accepted it. Each touch is now staged in a sibling file and renamed over the theme, as an editor's atomic save does (the watcher follows renames). The comment above the snapshot names the real race: theme build renames its outputs into place, so a read of the module was never half-written.
There was a problem hiding this comment.
This change fixes the flake: staging each touch in a sibling file and renaming it over the theme means a rebuild can no longer read a truncated theme and publish a module without the icon import, and the updated comments describe that race accurately. The assertion still fails if the watch loop stops forwarding --icons-specifier to its rebuilds, so the #4620 guard is intact. No further changes are needed.
[Automated review]
The failure this test showed is the build's second read of the theme, fixed in #8598; closing in its favor. The branch stays.
Depends on nothing (based on main).
Maintainer impact
build-theme.icons-specifier.test.mjs › watch mode forwards the flag to every rebuildfails intermittently in CI'stest-nodeshard, on pull requests that change no CLI code. It fails at the final assertion:expected null to be 'import { testIcons } from "./icons.mjs"'.Root cause (corrected in review; the first commit's diagnosis was wrong):
writeFileSync, which truncates the file before it writes. A re-touch landing between a rebuild's two reads gives that rebuild an empty source to scan. With the test's empty icon registry, the build still exits 0, with the new token and no import line, and the poll accepts that module.theme buildrenames its outputs into place, so no read of the module is ever half-written.Intended invariant
Every theme the watcher sees is whole, so every rebuild the poll can accept was built from the touched theme, import included.
Change
Evidence
expected null to be 'import { testIcons } from "./icons.mjs"'.Scope
Testing
pnpm exec vitest run packages/cli/clients/cli/commands/build-theme.icons-specifier.test.mjs: 8 passed, three runs.ASTRYX_STRICT_LINT=1 eslinton the file,pnpm check:changesets: clean.