ci(visual-gate): build the theme dists the gate needs instead of crashing - #5514
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No 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 |
…hing The gate reads each theme's built source, which reaches the runner as an artifact from the job that already built it. Three times in twenty-four hours a change to how that artifact is named or filled left the gate with nothing to read, and it died on a missing-file error naming a path no PR had touched — on 15 of the 21 open PRs where it ran. It builds what it cannot import now: core first, since every built theme imports its dist, then only the themes that are actually missing. A bare tree recovers in 21s and an intact one pays nothing. The download step is non-fatal for the same reason, so an artifact rename can no longer kill the job before the gate is reached.
8106c2d to
765b432
Compare
Proved on CI, not locally
1. The recovery path, run On 2. The normal path, run One thing the diagnosis changedThe failures are not about a stale base — they are about a stale event. A |
Problem
pr-visualis red on 15 of the 21 open PRs where it ran (71%), and none ofthe failures are about anything those PRs changed. Two different errors, both
naming a file the contributor never touched:
It is a soft gate, so people merge through it — which is worse than the gate not
existing, because a red check nobody reads is a red check nobody reads when it is
right.
Why, exactly
The gate needs two build products it does not produce: each theme's
dist/source.mjs(a theme's component map is whatdefineThemereturns, not aliteral in its source) and
packages/core/dist, which a built theme's ownimport '@astryxdesign/core/theme'resolves into through the workspace link.They reach the runner as an artifact from
build-storybook, and how thatartifact is named and filled changed three times in twenty-four hours
(#5477,
#5481,
#5482). Each intermediate shape
left the gate with nothing to read, and the gate's response to that was to
throw.
A PR does not run the
ci.ymlonmain. It runs the one on its own mergecommit, and that merge commit is frozen when the
pull_requestevent fires.Re-running the job replays the same payload, so a PR whose last push predates a
fix re-runs the pre-fix CI forever, and the error tells you which snapshot it is
stuck on. This is not base staleness:
#5455 is 25 commits behind main
and green, because it was pushed after the last fix landed; the failing PRs are
10–14 behind. Any push refreshes the snapshot — merging main is just one kind of
push.
So the backlog drains itself as people push. The defect worth fixing is that a
wiring change to a CI artifact can redden a gate at all.
The two
digest-mismatch: errorlines are a red herringThey are
actions/download-artifact@v8echoing its owndigest-mismatchinput(default
error) in thewith:block it prints before every download. Nothingmismatched — both digests verified. The lines are worth exactly one thing:
count them. Two means the run is on the pre-#5477 workflow; three means
post-#5477. Not ours to fix, and nothing in this PR touches it.
Solution
Four decisions:
since every built theme imports it; then only the themes actually missing.
pnpm build. The gate needs eight small packages,not the whole workspace, and it says which one it is building and why.
a broken build from a missing one, which the old message could not.
continue-on-error. A crash there happens beforethe gate runs, so the recovery would never get its turn — that is precisely how
a rename reddened every open component PR.
The artifact stays: it is the fast path. It is now an optimisation rather than a
correctness dependency.
Impact
Every component PR, contributor and maintainer alike. The gate stops being able
to fail for a reason that is not about the PR.
Already-red PRs are not retroactively fixed — nothing landed on
maincanreach a run frozen to an older merge commit. They go green on their next push.
API
None.
loadThemeOverridesgained an optional third parameter as a test seam;it is an internal CI script with two call sites, both in
gate.mjs.Usage
Theme targets
None.
Ossification
The build seam is a third positional parameter on a module under
.github/scripts/, imported by one file in this repo and published nowhere. Ifit is wrong, changing it costs one call site and a test. Not a class decision —
"inject the expensive side effect" is what the existing
ensureCoreBuilthelperdoes for the same build, and I deliberately did not import that one: it is
declared a test helper and locks against parallel Vitest workers, a hazard the
gate does not have.
Breaking
and continues. That is the point of the change.
Performance & resources
Effects: one
pnpm -F <pkg> buildper missing package, on the failure path only.Measured on this branch, from a fully bare tree (a state CI never actually
reaches, since
build-storybook's artifact or the job's own core build normallycovers it):
No cost on the happy path — the first import succeeds and nothing is spawned.
Visual evidence
None; this is CI plumbing and changes no rendered pixel. The evidence is the job
log, in a comment below.
Judgement
What I could not verify
build-storybook's core dist in the artifact is now dead weight.#5482 rebuilds core in
pr-visualunconditionally, which makes#5481's core half redundant —
~12s and an artifact's worth of bytes on every component PR. Flagging rather
than removing: dropping a safety step in a PR about not depending on that step
is the wrong trade, and it is a clean follow-up.