Repository navigation
ci: cut redundant installs, desktop rebuild, and vite build from the test job - #590
Merged
Merged
Conversation
Astro-Han
changed the title
Make the ripgrep and xvfb install steps conditional (`command -v X || install`): a no-op where the tool is already present, still installs on images that lack it. No assumption about what any image ships.
Move the vite renderer bundle to `dist-renderer` (sibling of `dist`) so `build:main`'s tsc side-files under `dist/renderer` (helpers imported by main/__tests__) and the vite bundle no longer collide. `emptyOutDir` clears only `dist-renderer`; the tsc side-files stay intact, so desktop no longer needs `clean:main && build:main` before testing. - vite outDir -> `../../dist-renderer`; main-window `loadFile` updated; check-stale-dist renderer pair updated (its header comment already described this two-output layout). - desktop `test:dist` = `test:checks` (console/a11y/copy, factored out of `pretest` so the list is not duplicated) + `node --test dist/main`, no rebuild. - root `test:dist` calls desktop `test:dist` instead of `test`.
`test:dist` only consumes tsc outputs (`dist/main` + `dist/renderer` side-files). The vite bundle is never loaded by node tests: the index.html test reads `src/`, the hygiene checks skip `dist/`, and e2e builds its own renderer in a separate job. Add `build:test` (builds everything except the vite renderer bundle) and use it in the test job instead of the full build.
Astro-Han
force-pushed
the
ci/conditional-tool-installs
branch
from
July 6, 2026 20:28
df62d4e to
4f7e976
Compare
Astro-Han
changed the title
The vite renderer bundle moved to apps/desktop/dist-renderer; .gitignore only covered `dist`, so the new output showed as untracked. Add `dist-renderer` alongside `dist`. (Codex review P2.)
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three CI cuts on the test/e2e jobs (the bottleneck after #587), one per commit:
command -v X || install. No-op where the tool is preinstalled, installs where it isn't.dist-renderer(sibling ofdist), sobuild:main's tsc side-files underdist/rendererand the vite bundle no longer collide. New desktoptest:dist(test:checks+node --test dist/main) needs no rebuild.test:checksis factored out ofpretestso the hygiene-check list lives in one place.test:distonly consumes tsc outputs; the vite bundle is never loaded by node tests (the index.html test readssrc/, the hygiene checks skipdist/, e2e builds its own renderer in a separate job). Addbuild:test(everything except the vite renderer) and use it in the test job.Why
After #587 the test job was ~167s. This PR's first iteration used
emptyOutDir:false+ a manual clean, which worked but was fragile. This revision switches to thedist-renderersplit (whichcheck-stale-dist.mjs's own header comment already described as the intended layout) and adds thebuild:testcut that the first iteration missed entirely — vite was building a renderer bundle that nothing in the test job consumes. Build-artifact sharing, dist caching, test sharding, larger runners, and build-script parallelism were evaluated (Codex + Claude consult + internal review) and rejected as over-engineering for a ~2-3 min CI on free 2-core runners.Measured
Local:
build:test9.5s (skips vite),test:distall suites pass / 0 fail (4863 tests incl. 2138 desktop),check:stalefresh, full build produces thedist-rendererbundle. CI will confirm the test-job wall (expected ~95-110s, approaching typecheck's ~90s floor).Scope
Three commits, one per concern (independently reviewable and revertable):
ci: install ripgrep/xvfb only when missingci(test): skip desktop rebuild via dist-renderer split + test:distci(test): skip the vite renderer build in the test jobFiles:
.github/workflows/ci.yml,apps/desktop/{vite.config.ts,package.json,src/main/main-window.ts},package.json,scripts/check-stale-dist.mjs.User-facing impact
None (CI + build scripts only).
npm run dev(vite dev server) is unaffected. The renderer bundle now lives atdist-rendererinstead ofdist/renderer; the only code that loaded the bundle is the oneloadFileinmain-window.ts(updated), andcheck-stale-dist.mjs's renderer pair (updated to match its own header comment).Reviewer notes
dist-renderermove is the cross-cutting change; its runtime effect is covered by the e2e job, which loads the window vialoadFile.emptyOutDirnow safely clears onlydist-renderer; tsc side-files indist/rendererare never touched by vite — this removes the prior iteration's clean-list fragility and the incremental-tsc transition footgun.test:checksis shared bypretestandtest:dist, so the console/a11y/copy check list has a single source.