Skip to content

Review 5519

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 1 revision

#5519 — ci: re-deploy previews from CI artifacts instead of rebuilding

bhamodi · open, approve posted · reviewed at 5966fb188be · view on GitHub

Verdict: approve — the loop merges it

Problem

Re-deploy Preview is a manual button a maintainer presses to republish a PR's preview. To do it, the job checked out the PR's head and ran pnpm build — while holding contents: write on the base repo. So a maintainer pressing it on a fork PR ran that contributor's install and build scripts with a token that can push to any branch. The PR's body frames the problem as duplicate build pipelines; the file's own new comment (redeploy-preview.yml:11) states the safety one.

Solution

(2 decisions + 1 trivial · 65 added lines, 1 file)

  1. Republish CI's artifacts instead of rebuilding — the fix.
  2. Absent artifacts fail loudly, where deploy-preview.yml skips quietly.
  3. Require the operator-typed PR number to be digits — trivial, recorded.

All three trace to something stated in the body, and the runner drop to ubuntu-slim is a consequence of 1 rather than a fourth decision. CI builds every PR's Storybook and Sandbox and files them under a name made from the PR's head commit; the button now looks that filing up, downloads it, and copies it into the published site. Nothing belonging to the contributor is executed, because nothing is built.

The change lives with its owner — .github/workflows/redeploy-preview.yml, the manual twin of deploy-preview.yml — and reuses ci.yml's storybook-<hash> / sandbox-<hash> artifact contract rather than rebuilding part of the system. The deploy step is now byte-identical to deploy-preview.yml's, which runs in production on every PR, so the riskiest half of this change is code already proven.

Impact

Nobody using Astryx sees anything. The people this reaches are maintainers and contributors:

  • A maintainer pressing Re-deploy Preview gets the same preview in well under a minute instead of a ~12-minute rebuild, and no longer runs a stranger's build scripts with a write token to get it.
  • A contributor whose preview is republished gets byte-identical bytes to the ones CI already showed them, rather than a second build that could differ.

What landing this newly exposes: nothing. The old path's failure mode was silent — nobody knew the button ran fork code — and the new path's failure modes are loud.

API

No API change — one CI workflow file, no package surface, no export, no prop.

Theme targets

n/a — CI workflow only, no styles. The style grep over the diff (#hex, rgba(, hsla(, boxShadow, light-dark(, stylex., xstyle) returns 0. No new theme targets.

Ossification

Nothing ossifies. There is no published surface to freeze; the only new internal coupling is on ci.yml's artifact names, which three other workflows already depend on.

Breaking

  • API — no. No package surface is touched.
  • Visual — no. Nothing renders; the bytes published are the same artifacts CI built.
  • Theme — no. No targets, tokens or overrides.
  • Behaviour — one change, and it is the one finding:
state before after
artifacts present rebuilds, deploys downloads, deploys — same bytes
artifacts expired (>30d) rebuilds, deploys fails; "re-run CI" advice is correct and works
CI never ran for this head builds anyway fails at :66 with the right advice
docsite-only PR built and published a preview fails at :101 with advice that cannot work
PR number non-numeric proceeded rejected at :53

The commit type ci: is honest — one workflow file, no runtime code.

Performance & resources

No React, so no effects, listeners or layout (useEffect|useLayoutEffect|useState → 0). This is a measured improvement, not a cost: the work removed is a full monorepo build. From this PR's own CI run, build-storybook took 3m32s and build-sandbox 8m17s — the old job did both on a 2-core runner before it could push anything. The new job downloads two artifacts on ubuntu-slim. Nothing goes up: no new dependency, no new action beyond download-artifact@v8, which three sibling workflows already use, and the runner gets smaller.

Visual evidence

None, and the reason is that nothing here is renderable: the diff is one CI workflow YAML file. No geometry, no colour, no DOM, no component — confirmed by the greps returning 0 for styles and 0 for ARIA and strings. Breaking · Visual is "no", so this slot and that answer agree.

A11y & i18n

n/a — no rendered output and no strings (aria-|role=|useTranslator|t('@astryx → 0). The only human-readable strings the diff adds are two ::error:: lines for a maintainer reading an Actions log. One of them is wrong for a case it can reach, which is the behaviour finding, not an i18n one; workflow log output does not go through the catalog and should not.

Judgement

approve and merge. No design question and no package surface, so nothing here needs a human ruling.

1. On a docsite-only PR, CI uploads no preview artifacts at all — the Storybook
   upload is gated on `docsite_only != 'true'` (ci.yml:251) and the whole
   build-sandbox job is gated the same way (ci.yml:323-325, whose own comment
   reads "docsite-only PRs skip preview builds entirely"). So the new failure
   message tells the operator to re-run CI, which skips both uploads again.
   → a maintainer re-deploying a docs-only PR's preview runs CI a second time,
     waits, retries, and gets the identical error, with nothing saying the PR
     has no preview by design            · redeploy-preview.yml:101

Not blocking. Two candidate blocking findings were raised and killed by a second check, both of which looked solid on a first read:

  • "the job never granted actions: read, so every download 403s." Dead. gh pr diff's three lines of context cut off line 40; actions: read is already on main at :31 and this PR does not touch it.
  • "ubuntu-slim has no gh CLI, so the first step dies." Dead. cleanup-previews.yml runs gh pr list on ubuntu-slim in production today (:57, :130).

Both of the PR's load-bearing claims were verified for both artifacts: the short hash is the first 7 of pull_request.head.sha in the Storybook job (ci.yml:240-241) and in the build-sandbox job (ci.yml:345-346), which is what the new workflow computes at :58-59; and CI builds the sandbox with /<repo>/pr/<n>/sandbox (ci.yml:350), matching the base path the deleted build step set. Vercel is red and it is not this PR's — #5518, #5517, #5520 and #5521 all show the same failure. 20 checks, everything else green.

Not verified: the workflow was not run. Every claim is read from the YAML at head and from ci.yml / deploy-preview.yml on main; the strongest evidence that the mechanism works is that deploy-preview.yml already does exactly this in production. ubuntu-slim's image contents were not confirmed directly, only that another production workflow uses gh on it.

The review, as posted

Thanks — dropping the fork-head checkout from a contents: write job is the bigger win. Merging as is. Nit: docsite-only PRs never upload artifacts, so the error's advice can't work there.

Full review

[Reviewed by Robohands]

Inline: .github/workflows/redeploy-preview.yml:101 — Hmm, docsite-only PRs never upload these (ci.yml:251, :325), so re-running CI won't help.

Rounds

One review, three gate passes.

  • Gate 1 — failed. The review never said what happens to the PR: its closing line was the nit, under an approve-and-merge verdict, so a contributor who cannot self-merge was left unsure whether to push a fix or wait. And the finding cited redeploy-preview.yml:101 but anchored its cause nowhere — the claim is about ci.yml's behaviour, so ci.yml's lines belong in it.
  • Gate 2 — one incomplete citation. "Merging as is." was in, and the finding now carried ci.yml:251 and ci.yml:323-325, both opened at the cited revision — stronger than asked for, since the job's own comment carries the claim. The remaining miss: the short-hash evidence cited only the build-sandbox job's computation, while the workflow downloads two artifacts and the Storybook one's hash is computed in a different job.
  • Gate 3 — clean. Both jobs now cited and both opened at head; full re-check of every other rule passed, since a rewrite that fixes one thing routinely breaks another.

What changed before posting

Posted as drafted, with a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5519) line appended before the attribution.

Clone this wiki locally