Skip to content

Review 5371

Jump to bottom
Cindy Zhang edited this page Aug 29, 2026 · 6 revisions

Review 5371 — ci: publish PR previews as real GitHub deployments

PR: #5371 · author cixzhang Verdict: request changes (engineering) — posted as a comment. GitHub accepts neither an approval nor a change-request from a PR's own author, so the mechanical verdict is comment.

HEAD REVIEWED

aebf786abec11aa9b892ec6d1817d82a3513ab34

Every claim below was verified at this commit. Re-checked at the end of the run: unmoved, updatedAt 2026-08-27T03:48:28Z. Merge-base with main: 789ef5745cfd5fcf8caf7cc58b4065fba9ba3681.

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: three independent runtime decisions and ~122 non-comment added lines, over the ≤50-line / one-decision bar. Promoted at first read of the diff, not later.

PROBLEM

WHY 1: The Storybook and Sandbox previews exist only as a bot comment, so GitHub itself does not know they exist.

WHY 2: A reviewer who does not scroll the comment thread never finds the preview, and any surface that reads deployments rather than comments — the PR's own deployment box, the commit, the Deployments API — shows nothing at all.

WHY 3: The preview is the cheapest way to see what a change does to the library. When the only route to it is parsing a comment, reviewers judge the diff instead of the rendered result, which is the failure mode the previews were built to prevent.

USER-FACING PROBLEM: A reviewer opening a PR in anything that reads GitHub's deployments sees no preview, so they review the change without ever looking at what it renders.

PROBLEM SEVERITY: harmful friction — the review completes, but the preview is reachable only by hunting a comment thread, and is invisible to every deployment-aware surface. Nobody is blocked; nothing is unreachable.

Evidence: the only preview link on this PR today is inside the github-actions[bot] "PR Analysis Report" comment. GET /repos/facebook/astryx/deployments?environment=Storybook and ...=Sandbox both return 0 records; Preview (Vercel) returns 100+, which is the shape being copied.

VERDICT: clear

SOLUTION

When a preview is published, the repo now tells GitHub about it in GitHub's own vocabulary: this commit has a Storybook site here and a Sandbox site there. GitHub then shows it everywhere it shows deployments, and anything that asks GitHub what a commit deployed gets the links back instead of nothing. When the preview is later deleted because the pull request closed, the same system is told the site is gone so the links stop being offered. Responsibility stays with the jobs that already publish and delete the preview directory, so there is no second list to keep in sync.

SOLUTION (3 decisions · ~122 non-comment lines of 176 added)

  1. Publish each preview as a GitHub Deployment on the PR's head commit, in environments Storybook and Sandbox — problem: previews invisible to deployment-aware surfaces 1a. Carry the full head SHA through the CI metadata artifact so the trusted job can name a commit — enabling; ships with 1, cannot ship apart
  2. Retire those records when the closed PR's preview directory is deleted — problem: a link that outlives the site it points at
  3. Record from the manual re-deploy too, replacing the automatic run's failure — problem: a rescue re-deploy leaves a stale failure standing

More than two decisions is normally a signal to split, and this one does not split. Decision 1 alone ships the dead-link problem decision 2 exists to prevent; decision 3 alone records nothing. There is no intermediate state we would ship.

Decision 2 does not do what it says. cleanup-previews.yml:298 looks up deployments?sha=${head_sha} where head_sha is gh pr view --json headRefOid — the PR's final commit. Records are written once per push, per environment. Counted in one unit, records:

records
created across the 20 most recently closed PRs (75 head commits × 2 environments) 150
retired by cleanup (final commit of each of 20 PRs × 2 environments) 40
left success and active, still advertising a deleted pr/<n>/ directory 110

13 of the 20 PRs pushed more than once; one pushed 18 times.

Reproduction:

gh pr list --repo facebook/astryx --state closed --limit 20 --json number,headRefName \
  --jq '.[]|"\(.number) \(.headRefName)"' | while read -r n b; do
    gh api "repos/facebook/astryx/actions/workflows/ci.yml/runs?branch=$b&per_page=100" \
      --jq '[.workflow_runs[].head_sha]|unique|length'; done
# 75 head SHAs over 20 PRs; x2 environments = 150 records, 40 retirable.

BURDEN: low — no product runtime, no state, no dependency. Four extra API calls per push, two permanent repo environments, and two hand-duplicated copies of one ~30-line script.

BURDEN MATCH: proportionate — the capability is a dozen API calls and two permissions. The one real cost is the deliberate duplication, and it is where the second finding landed.

VERDICT: BLOCKS — retirement reaches 40 of the 150 records the sample would create; the other 110 keep a live link to a directory the same job deleted.

ARCHITECTURE

OWNER: the two workflows that already own the preview's lifetime — deploy-preview.yml publishes pr/<n>/, cleanup-previews.yml deletes it. The records are attached to exactly those two, plus the manual re-deploy. TIER 1: none — no component system, no shared library primitive. GitHub's Deployments API is the reused system, and the repo already runs it for Vercel's Preview environment. TIER 2: none. SEAMS: the pr-meta.json artifact (the trust boundary between untrusted PR code and the privileged workflow_run job) · the fork-PR path · the manual workflow_dispatch re-deploy · the daily cron cleanup · a metadata file written before headSha existed. BEHAVIOR UNIT: inline shell, duplicated in two workflows on purpose — deploy-preview.yml checks out nothing (that is its security property), so it cannot call a local composite action. The reasoning is stated in the diff and it is correct.

seam driven result
fork PR head SHA not driven — static lane. Source-level: ci.yml:253 is github.event.pull_request.head.sha, the real head, not the refs/pull/N/merge commit; the base repo holds it as refs/pull/N/head. Author's probe on #5370 reports it created.
old metadata without headSha not driven — static lane. Source-level: deploy-preview.yml:90-93 blanks a non-40-hex value and :202 skips the record, so the deploy still happens. Correct.
manual re-deploy fails — see BREAKING. redeploy-preview.yml:119 is a bare if: always(); deploy-preview.yml:202 guards the same script with steps.verify.outputs.ready == 'true'. The two copies have already diverged.
daily cron cleanup not driven — static lane. Source-level: the retire step is gated on steps.cleanup.outputs.deleted_prs != '', which is only written after the DELETED -eq 0 early exit and before the push; a failed push exits 1 and the step is skipped. Sound.

Placement is right and the duplication is argued. The consequence of the duplication is not a placement question — it is the behaviour defect below.

VERDICT: note — two inline copies of one contract, justified, and already disagreeing on when to record. The consequence is filed under BREAKING against the line that causes it.

IMPACT

Builders and reviewers. Everyone who opens a PR in this repo, and anything that reads deployments rather than comments. Today the preview box on a PR is Vercel's Preview only; after this it is Preview, Storybook and Sandbox, with the Storybook and Sandbox links where GitHub puts links instead of in the middle of a bot comment. Nobody has to opt in and nothing changes for an end user of the library.

What landing this newly exposes. Deployment records are permanent objects on the repo, and today there are none in these two environments. After this lands the repo starts accumulating them at 2 per push: on the 20-PR sample, 150 records of which 110 are never retired and keep offering a pr/<n>/ link that 404s. That is new — the existing dead link on a closed PR is one bot comment; this adds 5.5 more per closed PR, on the repo's Deployments page and to every unfiltered API read. It is not pre-existing debt and it does not go away on its own; a later fix does not retroactively retire what shipped in the meantime.

VERDICT: note — the change reaches every reviewer positively; the un-retired records are finding 1's cost, restated here as who pays.

API

no API change. Nothing in packages/ is touched; the diff is four files under .github/workflows/.

OSSIFICATION: the closest thing to permanent surface here is the two environment names, Storybook and Sandbox. Both already exist on the repo (GET /repos/facebook/astryx/environments lists them alongside Preview, Production, canary, github-pages, copilot), so nothing new is being named. transient_environment: true is the right marker for a per-PR site. Class: Preview, Vercel's, on this same repo — a landed member, same shape. Cost of being wrong: renaming an environment later orphans its records; low, and reversible by a sweep.

VERDICT: clear

THEMING

n/a — CI only, no styles.

STRUCTURAL ONLY — no styles, no strings, no roles.
  grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" <4 changed files>
      -> 4 hits, all issue numbers inside comments (#2941, #4290, #4530, #4672)
  grep -nE "aria-|role=|useTranslator|@astryx\." <4 changed files>
      -> 0

no new theme targets.

VERDICT: clear

BREAKING

BEHAVIOR: yes — one new defect. redeploy-preview.yml:119 records on every exit path, not only the ones that deployed something:

      - name: Record the preview as a GitHub deployment
        if: always()

STATE=failure unless steps.deploy.outcome is exactly success, so skipped and cancelled both post failure to both environments. Two reachable paths:

  • Artifacts expired. #5519 landed on main on 2026-08-26 and rewrote this workflow to download CI artifacts instead of rebuilding. Its new "Verify preview artifacts are present" step exit 1s with an actionable message — "missing or expired — re-run CI on PR #N; deploy-preview will publish the fresh preview automatically." That is a clean stop where nothing was deployed and nothing is wrong. Merging this PR on top makes that stop stamp failure on both environments. Storybook artifacts are retention-days: 30, so this is the ordinary case on any PR older than a month — exactly when someone reaches for a manual re-deploy.
  • Cancelled run. concurrency: cancel-in-progress: true on pr-preview-${{ inputs.pr_number }}; always() runs on cancellation by design. Of the last 12 Re-deploy Preview runs, 3 ended cancelled or failure (35 runs total since April).

deploy-preview.yml:202 gets this right for the same script:

        if: always() && steps.parse.outputs.head_sha != '' && steps.verify.outputs.ready == 'true'

The PR body's own claim — "A run that never had artifacts to deploy … records nothing at all" — is true of deploy-preview.yml and false of redeploy-preview.yml.

Behaviour states the diff moves through: empty — no PRs deleted, retire step skipped by its if, correct · error — push fails after 5 attempts, cleanup exits 1, retire step skipped, correct · boundary — a metadata file with no headSha blanks and skips, correct · loading / disabled / controlled: not reachable — no component, no state, no props.

API: no — nothing in packages/, no exported symbol, no signature. VISUAL: no — no rendering path is touched; four .yml files under .github/workflows/, grep above returns zero style and zero DOM hits. CI's visual-acceptance on this head reports "No stable visual scope. (PR #5371)". THEME: no — no targets, tokens or overrides exist in these files.

VERDICT: BLOCKS — the manual re-deploy posts failure for both environments on exits where nothing was deployed and the live preview is unchanged.

PERFORMANCE & RESOURCES

EFFECTS: zero. No React, no component, no hook — the diff is four GitHub Actions workflow files.

RENDER: no reachable change — nothing renders. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency, no shipped byte. jq and gh are both already on the runner and already used by these workflows.

API-call and object cost, counted rather than adjectived:

  • +4 GitHub API calls per push (2 creates + 2 statuses) in deploy-preview, and the same in a manual re-deploy.
  • +2 permanent deployment records per push. The repo records 1 today (Vercel's Preview), so this triples per-push deployment objects, 1 → 3.
  • Cleanup: 1 gh pr view + 2 list calls + N status POSTs per deleted PR, only on runs that actually deleted something.
  • The part that accumulates is finding 1's: 110 of 150 records on the 20-PR sample are never retired. A count that goes up and stays up.

Nothing here is a user-perceived cost. The record count is a cost we carry, and whether it is acceptable is a decision the retirement fix removes rather than one anyone has to make.

VERDICT: note — no runtime cost; +2 permanent records per push, of which 110 of 150 are currently never retired (finding 1).

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: genuinely non-rendering. The complete changed-path list is .github/workflows/ci.yml, .github/workflows/cleanup-previews.yml, .github/workflows/deploy-preview.yml, .github/workflows/redeploy-preview.yml (gh pr diff 5371 --name-only, 4 of 4). No component, story, style, template or doc file is touched; the structural grep in THEMING returns zero style and zero DOM/ARIA hits. CI's visual-acceptance job on this exact head returns "No stable visual scope. (PR #5371)" and pr-a11y / pr-rtl are skipping for the same reason. No frames, and none are owed.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

n/a, with the check rather than the assertion. §1: no rendered output, no interactive element, no ARIA attribute, no focus path — grep -nE "aria-|role=" over the four changed files returns 0. pr-a11y is skipping on this head because no component changed, which is the correct result and not a gap. §9: no user-visible or AT-visible string is added; the only new strings are workflow log lines and a deployment description (Preview for PR #N) that GitHub renders in its own chrome, not in the product. No direction, no logical-property, no locale surface.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION BLOCKS — retirement reaches 40 of 150 records
ARCHITECTURE note — two copies of one contract, already diverged
IMPACT note — 110 un-retired records per 20 closed PRs, newly created
API clear
THEMING clear
BREAKING BLOCKS — re-deploy posts failure where nothing deployed
PERFORMANCE note — +2 permanent records per push, 110 of 150 never retired
VISUAL clear
A11Y & I18N clear

GOAL: partly met — the recording half works: ci.yml:253 carries the true PR head (github.event.pull_request.head.sha, not the merge commit), deploy-preview.yml:226-245 creates and statuses it, and the author's live probes on #5348, #5345, #5343 and #5370 show it end to end. The retirement half reaches 40 of the 150 records the sample would create. The manual re-deploy records, but mislabels its non-deploy exits.

DISPOSITION:

  1. Retirement is keyed to the PR head, records are keyed to the commit → blocks now
  2. Manual re-deploy records failure on skipped/cancelled exits → blocks now
  3. record() runs under set -euo pipefail and calls Storybook before Sandbox, so a transient API error leaves one environment recorded, the other absent, and the run red → accepted: Deploy PR Preview runs on workflow_run and is not a PR check, so the red run is invisible on the PR; the next push re-records both. Cost is a missing box until then, paid by whoever reads the PR. Raising it would be a third finding on a two-finding review; noted here, not asked for.
  4. main moved under all four files since this head (10 commits, ci.yml +229/−9) → no finding: a local --no-commit merge of origin/main into this head is conflict-free and semantically intact — id: deploy lands on the right step, steps.pr-info.outputs.head_sha and PR_NUMBER both survive. Checked because a four-day-old PR against fast-moving CI files needs it; reported because a clean result is a result.

ADVICE: proven existing pattern, for finding 2 — deploy-preview.yml:202 already guards this exact script with steps.verify.outputs.ready == 'true', one file over, in this same PR. For finding 1, bounded outcome criteria only, no prescribed implementation.

AUTHOR CAN PROCEED: yes —

  1. After a closed PR is cleaned up, no deployment in Storybook or Sandbox for any commit of that PR is still active. (The description the diff already writes, Preview for PR #N, identifies them; walking the PR's commits also does. Author's choice.)
  2. A Re-deploy Preview run that deployed nothing — skipped or cancelled — writes no deployment status at all.

WORST OUTCOME: "Someone asks for a manual re-deploy of a month-old PR, is told to re-run CI because the artifacts expired, and the PR is left publicly claiming both previews failed while the live preview is untouched." → request changes.

JUDGEMENT NEEDED: none — the capability question is already settled by the author's own direction comment on this PR (2026-08-27): "keep the deployment integration. Storybook and Sandbox previews should be first-class GitHub Deployments with cleanup, not links discoverable only by parsing bot comments." Both findings are defects against the PR's own stated contract, not design calls.

ENGINEERING VERDICT: request changes. MECHANICAL POSTED VERDICT: comment — GitHub accepts neither an approval nor a change-request from a PR's own author. Nothing was posted: this run was read-only.

  1. [BLOCKS] Records are written per commit; retirement queries only the PR's final commit → after a closed PR is cleaned up, every earlier push still advertises a live Storybook and Sandbox link to the pr/<n>/ directory the same job deleted — 110 of 150 records across the 20 most recently closed PRs · cleanup-previews.yml:298
  2. [BLOCKS] if: always() with no deploy-outcome guard, unlike its twin → someone re-deploys a month-old PR, is told the artifacts expired and to re-run CI, and the PR is left claiming both previews failed while the live one is fine · redeploy-preview.yml:119

REVIEW

Thanks — the required_contexts and auto_inactive notes each save someone a day, and the probe table is the right evidence.

Two things before it lands.

Records are written per commit and retired per PR head. Over the last 20 closed PRs that is 150 records against 40 retirements: 110 success records keep a live link to a pr/<n>/ directory the same job just deleted.

And the manual re-deploy's always() has no equivalent of deploy-preview's verify.ready guard — the two inline copies have already drifted — so with #5519 on main its "artifacts expired, re-run CI" stop posts failure for both environments while the preview is still live.

For the retirement, does matching the description you already write feel right, or would you rather walk the PR's commits?

[Reviewed by Robohands]

INLINE (0-3, only if genuinely line-anchored)

  • .github/workflows/cleanup-previews.yml:298 — Only the head commit. Every earlier push's record stays active pointing at the directory just deleted.
  • .github/workflows/redeploy-preview.yml:119 — Hmm, always() fires on skipped and cancelled too. deploy-preview gates on verify.ready; this doesn't.

EVIDENCE I DID NOT SPEND

  • actionlint before/after on the four files: 33 findings each side. Every difference is a line-number shift of a pre-existing SC2086/SC2034, plus the SC2129 in deploy-preview.yml genuinely removed. The test plan's claim is honest, including the one new unquoted $GITHUB_OUTPUT matching its neighbours.
  • The trust boundary holds: PR_NUMBER and HEAD_SHA come from an artifact written by PR-triggered CI, and deploy-preview.yml:79-93 regex-validates both before they reach a privileged API call or a URL. deployments: write in the workflow_run context adds no reachable injection.
  • Vercel keeps every commit's Preview deployment success and active — 8 of them on closed #5601. That is fine for Vercel because its URL is per-deployment and never deleted; this PR's URL is per-PR and is deleted, which is exactly why the retirement has to match the recording's granularity.

LIMITATIONS

  • Static/CI lane. Nothing was installed, built or executed: no workflow run, no pnpm install, no browser. Evidence is source reading at the exact head, read-only GitHub API queries, two actionlint runs, and one local --no-commit merge preview.
  • What GitHub's UI renders for a stale active deployment was not checked. Finding 1 is stated at the API level, which is what the PR's own body names as the audience; no claim is made about a button.
  • Remote CI owns execution of these workflows. workflow_run workflows run from the default branch, so this PR cannot exercise its own changes — the first real run through this YAML happens after merge.

REVIEW LOOP

Full lane. Two gates: gate 1 failed on a unit mismatch between the private slots and the public text (a commit count set against a PR count in one clause) and an unfilled TIME slot; gate 2 came back clean with no violations. Reviewer, critic and evidence were all performed in one session — no pass was delegated.

TIME

TIME  total 22m
  setup       2m   dedicated worktree at the PR head; no install, no build
                   (warm main reused: yes — read-only, for origin/main and the merge check)
  reading     7m   brief + critic in full, loop version, presentation, harness, the 4 workflow files
  measuring   6m   11 read-only GitHub API queries, 1 local merge preview,
                   2 actionlint runs (base vs head), 0 re-measures
  writing     4m   presentation + critic pass + round-2 revision
  publishing  3m   fresh wiki clone, this page, the index row, lock + rebase + push
  waste       1m   one grep over the four changed files quoted its file list wrong and
                   had to be re-run; one candidate finding (that the recorded SHA
                   might be the merge commit) took a file read to kill.

Remote CI wait: 0m — CI was already complete and green on this exact head before the run started (19 checks; pr-a11y, pr-rtl and the review-clear chain correctly skipping). Nothing was queued or waited on.

What changed before posting

Posted as drafted in review 5043574438. No judgment or wording changed after the clean gate.

Round 2 — rebased maintenance review

PR

#5371 ci: publish PR previews as real GitHub deployments by cixzhang (bucket: the maintainer)

HEAD REVIEWED

b204b467ac4d850cb5518d996e63f978d3b2502f

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15

LANE

LANE: full WHY: This is a conflicted re-review of repository-wide deployment infrastructure with a prior blocking review and a privileged API permission.

PROBLEM

WHY 1: Storybook and Sandbox preview sites exist, but GitHub does not have Deployment records for them. WHY 2: Reviewers and tools that use GitHub's deployment surfaces cannot discover those previews without parsing a bot comment. WHY 3: The preview is the fastest way to inspect a change's rendered behavior, so hiding it from normal review surfaces makes visual verification less likely. USER-FACING PROBLEM: A reviewer using GitHub's deployment-aware surfaces cannot find either rendered PR preview and may review the code without inspecting the result. PROBLEM SEVERITY: harmful friction — the preview exists, but the normal deployment discovery path is empty. NEW FEATURE CASE: The maintainer publicly settled the need in this direction comment; current API reads return zero Storybook and zero Sandbox deployments, while the bot comment contains both URLs. EARLY STOP: clear — the repository owner explicitly wants this capability and the current system cannot expose it.

VERDICT: clear

SOLUTION

The existing publisher now tells GitHub whenever it publishes a PR preview, using one deployment record for Storybook and one for Sandbox. The same owner records a failed publish as failed and identifies every record for a deleted PR by its stable description. Automatic publishing, manual publishing, and cleanup therefore share one lifecycle implementation instead of copying it into each workflow.

SOLUTION (2 decisions · 251 production/workflow additions, 15 deletions)

  1. The shared publisher creates and statuses both transient deployments after an actual preview publish attempt — serves deployment discovery and truthful failure state.
  2. Cleanup returns the PR numbers whose directories it removed, and the same publisher retires every matching historical deployment — prevents links from outliving the deleted site.

cixzhang's prior review found that "Records are written per commit and retired per PR head" and that manual re-deploy's always() "has no equivalent of deploy-preview's verify.ready guard." Both blockers are resolved. Retirement now matches the stable Preview for PR #<n> description across every deployment in both environments (gh-pages-publisher.mjs:216-261), not only the final head. Manual re-deploy reaches the publisher only after its artifact verification succeeds (redeploy-preview.yml:92-113), so expired artifacts produce no false failure; both publish paths call the same implementation.

BURDEN: medium — one existing publisher gains one GitHub API adapter and two lifecycle helpers; three workflows add only permission/head wiring; 150 test lines cover payloads, failure propagation, historical retirement, cleanup output, and workflow structure. BURDEN MATCH: proportionate — real deployment lifecycle requires create, status, and inactive operations, while the prior duplicated workflow implementations and the no-longer-needed CI metadata change are removed.

VERDICT: clear

ARCHITECTURE

OWNER: .github/scripts/lib/gh-pages-publisher.mjs, the existing owner of every gh-pages publication and cleanup mutation. TIER 1: GitHub Deployments REST API, reused through one request adapter. TIER 2: none. SEAMS: automatic workflow_run publish, manual workflow_dispatch re-deploy, cleanup after a PR closes, failed gh-pages push, no-change re-publish, and multiple commits on one PR. BEHAVIOR UNIT: pure lifecycle helpers around the existing publisher functions — focused tests inject the API boundary and exercise both status paths. COMPLEXITY BUDGET: one lifecycle owner, two fixed environment projections, zero new durable maps/schemas, and three thin workflow call sites. ACTUAL BURDEN: 6 files; publisher +244/−14, tests +150/−1, workflows +7; one API adapter, one record helper, one retirement helper, zero new jobs, zero new concurrency groups, zero duplicate workflow implementations. BURDEN TREND: aebf786 → b204b46 shrank from separate recording scripts in two workflows plus cleanup-by-head to one publisher-owned lifecycle. File count grew from 4 to 6 because the centralized implementation and its tests replace workflow-local shell. RESET TRIGGER: not triggered — the rebase removed duplicate owners and representations rather than adding another one.

domain fact one authoritative writable source generated / immutable projections other writable copies
trusted PR identity Resolve trusted preview target output --pr and --head CLI arguments none
deployment lifecycle gh-pages-publisher.mjs record/retire helpers Storybook and Sandbox records none
previews deleted in this cleanup cleanupPreviews() result deletedPrs passed to retirement none
seam driven result
automatic publish workflow passes the API-validated PR number and 40-character head to the shared publisher
manual re-deploy artifact verification exits before the publisher; no deployment status is written for an expired artifact
failed publish focused test proves the publisher records failure and rethrows the original publish error
multiple PR commits focused test proves every deployment whose stable description matches the deleted PR is retired
concurrent gh-pages writers existing shared publication queue remains the only writer coordinator; no job or concurrency group is duplicated

VERDICT: clear

IMPACT

Reviewers of every non-draft PR get native Storybook and Sandbox deployment links on the commit and PR timeline. A failed publish is visible as failed, and closing the PR retires all links to the deleted pr/<n>/ site. Existing preview contents, URLs, and publishing concurrency do not change. NEW FEATURE IMPACT: reviewers can discover and open both previews through GitHub's deployment surfaces; the current failure is demonstrated by zero records in both environments and the owner-approved direction above.

VERDICT: clear

API

No package or component API changes. The repository automation adds two existing environment names, Storybook and Sandbox, to GitHub's Deployments API; the stable description is internal lifecycle identity, not consumer surface.

change public? class doc'd? verdict
+ GitHub Deployment for Storybook repository UI/API same class as Vercel Preview PR body ok — owner-approved
+ GitHub Deployment for Sandbox repository UI/API same class as Vercel Preview PR body ok — owner-approved

OSSIFICATION: the environment names were already chosen by the settled PR direction. Removing them later would orphan deployment history, so creation and retirement intentionally use the same fixed names and description function.

VERDICT: clear

THEMING

No theming change. All changed paths are repository automation or its tests; no component, token, target, CSS, or rendered DOM changes.

VERDICT: clear

BREAKING

BEHAVIOR: intentional repository-automation change only. Successful and failed preview publishes now create truthful Deployment statuses; cleanup deactivates every matching historical record. Expired manual re-deploys still fail before publishing and now write no false deployment failure. API: no package API or caller behavior changes. VISUAL: no rendered product or preview content changes; only GitHub's native deployment metadata appears. THEME: no target, token, variable, or override changes.

VERDICT: clear

PERFORMANCE & RESOURCES

EFFECTS: zero — no React code.

Effect + deps external system why render/handler cannot do it measured render cost lifetime + cleanup focused test
none GitHub Actions only n/a zero n/a n/a

RENDER: no application runtime path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or shipped-byte change. Automation cost: four Deployment API writes per publish. Cleanup performs two paginated environment reads only when it deleted a PR preview, then one inactive-status write per matching historical record.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: the six changed files are GitHub workflow, publisher, and test files. No product render, preview artifact bytes, CSS, DOM, or component behavior changes.

VERDICT: clear

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No rendered control, focus path, ARIA state, user-facing product string, locale behavior, or direction behavior changes. CI correctly skips component-only a11y/RTL work for this automation change.

VERDICT: clear

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION clear
ARCHITECTURE clear
IMPACT clear
API clear
THEMING clear
BREAKING clear
PERFORMANCE clear
VISUAL clear
A11Y & I18N clear

GOAL: met — the payload tests assert two transient deployments with required_contexts: [], truthful success/failure statuses with auto_inactive: false, and all matching historical records becoming inactive; workflow tests prove both publishers pass the trusted head through the one shared command. DISPOSITION: prior retirement blocker → fixed by description-matched pagination; prior false-failure blocker → fixed by removing the independent always() recorder and calling the shared publisher only after artifact verification; no new negative finding. ADVICE: omitted — no remaining defect. AUTHOR CAN PROCEED: yes — no implementation or design decision remains. WORST OUTCOME: none found → approve.

JUDGEMENT NEEDED: none — the capability was publicly settled by the maintainer; this round verifies the implementation and prior fixes.

approve and merge (engineering verdict; the author account can only post a comment mechanically)

REVIEW

Thanks — both blockers are fixed. Cleanup retires every matching historical record, and an expired manual re-deploy records nothing. The shared publisher keeps the paths together. Ready.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

None.

EVIDENCE I DID NOT SPEND

  • origin/main already centralizes every gh-pages writer in gh-pages-publisher.mjs through #5629, #5631, and #5674; it contains no Deployment API integration and the live Storybook and Sandbox environment queries each return zero records.
  • Rebase used rerere.enabled=false; all three conflicts kept the centralized workflows. The clean ci.yml merge was removed because trusted head identity already comes from Resolve trusted preview target.
  • Workflow structure check found one jobs block, one top-level permissions block, one job-level deployments: write, and one concurrency block in each changed workflow.

TIME

TIME total 22m setup 4m dedicated worktree, rebase with rerere disabled, fast pnpm 11 install (warm main reused: yes) reading 7m manifesto, brief, critic, harness, prior review, current main, history, workflows, publisher and tests measuring 7m 49 focused tests twice, actionlint, repository checks, API reads, structure/public guards; 1 re-measure after fixing queue-boundary attribution writing 4m PR history/body, presentation, critic passes, and review publication waste 2m broad first reads were truncated and had to be repeated in bounded chunks; one actionlint run rediscovered origin/main's existing SC2129

Remote CI wait: 16m — 19 checks passed, 6 component/visual/review-chain jobs skipped as expected; merge state is CLEAN.

WHAT I COULD NOT VERIFY

  • GitHub runs workflow_run workflow definitions from the default branch, so the real create/status calls cannot execute from this PR before merge. The exact payload shape was previously live-probed; this head preserves it in focused assertions.

What changed before posting

Posted as drafted in review 5058715449. The first critic pass required an explicit quotation of cixzhang's prior findings in the private presentation; the public 27-word review was unchanged. The second critic pass was clean.

Clone this wiki locally