Skip to content

ci: ship core's dist to the PR visual gate - #5481

Merged
nynexman4464 merged 1 commit into
mainfrom
ci-visual-gate-core-dist
Aug 25, 2026
Merged

ci: ship core's dist to the PR visual gate#5481
nynexman4464 merged 1 commit into
mainfrom
ci-visual-gate-core-dist

Conversation

@cixzhang

Copy link
Copy Markdown
Contributor

Problem

Every PR that touches a component gets a red pr-visual, outside contributors'
included — #5466,
#5462,
#5468,
#5018. The job crashes before it
shoots anything:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module
  '…/packages/themes/butter/node_modules/@astryxdesign/core/dist/theme/index.js'
  imported from …/packages/themes/butter/dist/source.mjs

Root cause: @astryxdesign/core is not built when the gate runs. A built
theme is not self-contained — dist/source.mjs keeps
import {defineTheme} from '@astryxdesign/core/theme' as an external, so
loadThemeOverrides importing it executes that import, pnpm resolves it through
the workspace link, and core's exports map sends it to
packages/core/dist/theme/index.js.
#5477 ships
packages/themes/*/dist/ and nothing else, and pr-visual never builds, so
that file is absent.

The other two candidates are both ruled out, not assumed:

  • The symlink farm is fine. pnpm install --frozen-lockfile in the job
    restores packages/themes/butter/node_modules/@astryxdesign/core../../../../core;
    Node resolved through it, which is why the error names a file inside the
    package rather than "Cannot find package".
  • The exports map is fine. "./theme" maps to ./dist/theme/index.js. A
    blocked subpath would be ERR_PACKAGE_PATH_NOT_EXPORTED, not a missing file.

Solution

  • Ship core's dist from build-storybook, which has already built it, rather
    than adding a build step to pr-visual#5477's
    reasoning, unchanged.
  • Carry it in the existing artifact instead of a second one: two search paths, so
    the artifact roots at their common ancestor packages/ and the download
    unpacks there. Renamed themes-<hash>dists-<hash> because it is no longer
    only themes.

Impact

Every component PR in the repo, including outside contributors'. pr-visual is
continue-on-error, so this was never blocking a merge — it was showing every
contributor a red check they could do nothing about.

API

None. Workflow-only.

Theme targets

None.

Breaking

None. The artifact is internal to one workflow run and consumed only by
pr-visual.

Performance & resources

One artifact, ~14 MB / ~1600 files larger; no extra job, no extra build.
Building core inside pr-visual instead would cost minutes of every component
PR's CI.

Visual evidence

None — this is CI plumbing, no pixels change.

Testing

  • Local reproduction of the exact CI failure: fresh worktree,
    pnpm install --frozen-lockfile, themes' dist/ copied in and nothing else
    (the state #5477 leaves) →
    same ERR_MODULE_NOT_FOUND on the same path. Dropping packages/core/dist/
    in and changing nothing else → all eight themes import and yield their
    component maps (butter 21 targets, probe 268, …), and
    gate.mjs plan gets past theme loading to the Storybook index it downloads
    in CI.
  • The real job on this PR. A workflow-only PR skips pr-visual by design
    (check-components finds no component change), so this branch carried a
    throwaway commit touching Spinner.tsx to arm it. That commit is dropped
    before merge; the run it produced is linked in the comments.

Importing a built theme runs its import of @astryxdesign/core/theme, which
pnpm resolves through the workspace link into packages/core/dist — absent on
the gate's runner, so every component PR died on ERR_MODULE_NOT_FOUND for
packages/themes/butter/node_modules/@astryxdesign/core/dist/theme/index.js.
build-storybook has already built core, so the existing artifact carries its
dist too rather than pr-visual paying for a build.
@vercel

vercel Bot commented Aug 25, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview Aug 25, 2026 5:06am

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 25, 2026
@cixzhang
cixzhang force-pushed the ci-visual-gate-core-dist branch from e19a16d to 0d891d3 Compare August 25, 2026 05:02
@cixzhang

Copy link
Copy Markdown
Contributor Author

Proof this fixes it, since a CI-only fix cannot be proven locally: this branch
briefly carried a throwaway commit touching Spinner.tsx, purely to arm
pr-visual (a workflow-only PR skips it by design — check-components finds no
component change). That commit is now dropped; the run it produced:

pr-visual on 6bd4892 — success,
every step green including Download built themes and core and the gate itself.
Its verdict.json:

{"status": "pass", "components": ["Spinner"],
 "counts": {"total": 8, "unchanged": 2, "changed": 0, "added": 6, "failed": 0}}

Eight shots taken and compared — the ERR_MODULE_NOT_FOUND is gone. (The six
"added" are Spinner story keys with no baseline on gh-pages yet, not failures.)

@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

No component packages changed.

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

github-actions Bot added a commit that referenced this pull request Aug 25, 2026
github-actions Bot added a commit that referenced this pull request Aug 25, 2026
@nynexman4464
nynexman4464 merged commit 4124603 into main Aug 25, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants