chore(CI): factor the repeated job setup into a composite action - #338
Merged
Conversation
#330 Eight CI jobs opened with the same four steps, so bumping the pnpm pin or changing the store cache was eight identical edits with one file always at risk of being left behind. `.github/actions/setup` now holds pnpm, Node and the install, and every job calls it. The checkout stays in the job. A local action is resolved from the working tree, so `uses: ./.github/actions/setup` cannot carry the checkout that puts it there — and checkout is the step that varies anyway, since ci-web.yml's `web` job needs `fetch-depth: 0` for Codecov base detection. Per-area caches stay in their jobs, moved just below the call: nothing writes the Playwright path during an install, and the Electron binary is fetched by the explicit `rebuild electron` step because the shared install runs --ignore-scripts. Both restores only have to precede the steps that read them. The install passes the manager and its flags through env and a fixed dispatch rather than splicing them into the run body — the same shape ci-editor.yml already uses for the Electron rebuild. Change detection had to learn about the new file. `buildFilters()` discovered CI files by scanning .github/workflows only, so a change to the action alone matched no filter key: every area job skipped and ci-ok green, on a change that alters how every job installs. It now goes through the `wf__` channel with its own `# ci:validates all` marker, which re-runs every job while leaving `changes.root` false — a CI edit must not deploy the site and both workers. Verified against the previous script: the only differences are the added filter key and a log line that now prints a path, since not every entry is `<name>.yml`.
The five area workflows already read `CI · <Area>`, but the entry workflow and four of the five deploys did not — `Continuous Integration`, `Continuous Deployment to GitHub Pages`, `Deploy Worker (@soroush/api)`, `Publish Packages (npm)` — so the sidebar was a list to read rather than two blocks to scan. ci.yml Continuous Integration -> CI cd-web.yml Continuous Deployment to GitHub Pages -> CD · Web (Pages + Storybook) cd-worker-api.yml Deploy Worker (@soroush/api) -> CD · Worker (api) cd-worker-bench.yml Deploy Worker (@soroush/bench-api) -> CD · Worker (bench) cd-packages.yml Publish Packages (npm) -> CD · Packages (npm) cd-editor.yml CD · Editor -> CD · Editor (release) Chromatic and the labeller stay unprefixed. Neither is part of ci-ok, and a CI prefix would say they gate a pull request when they do not. Renaming ci.yml is not a one-file edit: `workflow_run` matches on a workflow's `name:`, never its filename, so cd-web, cd-worker-api and cd-worker-bench all move from `workflows: ['Continuous Integration']` to `workflows: ['CI']` in the same change. Miss one and that deploy stops firing without an error. Thirteen doc references move with them, including the cd-packages runbook step that names the entry to click in the Actions UI. Branch protection is unaffected — it matches the job name `ci-ok`. ci.yml's concurrency group is keyed on `github.workflow`, so its value changes; the group stays internally consistent either side of the rename. The naming rule and the workflow_run coupling are written down in the workflows README and the ci-cd skill, so the next rename starts from them.
📝 WalkthroughWalkthroughThe PR adds a shared dependency setup action, updates CI jobs to use it, standardizes CI/CD workflow names and triggers, updates documentation, and extends workflow change detection. ChangesCI/CD standardization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant SetupAction
participant PackageManager
participant AreaCache
CIWorkflow->>SetupAction: pass Node version, manager, and install command
SetupAction->>PackageManager: configure tooling and install dependencies
PackageManager-->>CIWorkflow: dependencies ready
CIWorkflow->>AreaCache: restore area-specific cache
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/README.md:
- Around line 225-227: Update the bench deployment summary around the
`cd-worker-bench.yml` reference to include the `packages∋wrangler-tools`
condition alongside `worker∋bench ∥ root`, keeping it aligned with
`cd-worker-bench.md`; only change the detailed document instead if that package
trigger was intentionally removed.
In `@scripts/assemble-changes.mjs`:
- Around line 59-65: Update ciFiles() and the buildFilters()/workflowValidates()
flow so deleted .github/workflows/<name>.yml paths still produce a corresponding
wf__<name> filter, or trigger a conservative whole-workspace validation
catch-all. Ensure the fallback handles deletions that are absent from ciFiles(),
not only workflow names already detected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bf450e55-3671-4d70-9bc0-4992c3dc1e59
📒 Files selected for processing (21)
.claude/skills/ci-cd/SKILL.md.claude/skills/release-notes/SKILL.md.github/actions/setup/action.yml.github/workflows/README.md.github/workflows/cd-editor.md.github/workflows/cd-editor.yml.github/workflows/cd-packages.md.github/workflows/cd-packages.yml.github/workflows/cd-web.md.github/workflows/cd-web.yml.github/workflows/cd-worker-api.md.github/workflows/cd-worker-api.yml.github/workflows/cd-worker-bench.md.github/workflows/cd-worker-bench.yml.github/workflows/ci-editor.yml.github/workflows/ci-packages.yml.github/workflows/ci-web.yml.github/workflows/ci-worker.yml.github/workflows/ci.md.github/workflows/ci.ymlscripts/assemble-changes.mjs
…ondition Two review findings. `attributeWorkflows` built its reason strings from a template literal nested inside another one, which is hard to read at the point where it matters least. The path and the meaning are now named, and the reason is one flat template. Output is byte-identical. The workflows README summarised the bench deploy as `worker∋bench ∥ root`, but cd-worker-bench.yml has gated on `packages∋wrangler-tools` since the deploy started rendering wrangler.json with the shared bin — as cd-worker-bench.md already documented. The overview was the only place saying otherwise.
The per-file `wf__` keys are built by reading the working tree, so a deleted workflow or action generates no key: the one thing that changed matched nothing. A pull request that only removes CI files therefore ran no job at all and reported green — the same silent under-run the marker parser already refuses to make when it cannot read a claim. `ci__any` covers `.github/workflows/**` and `.github/actions/**`. When it matches and no per-file key does, the change is a file that is no longer there, and the whole workspace is validated. `changes.root` stays false, so a deletion still ships nothing. Coarse on purpose: it knows a file went, not which one, so it says "all" rather than guessing. A deletion alongside an edit needs no help, since the edit makes its own claim. Deleting only a workflow now runs both apps, all twelve packages and both workers, with root false. Editing one still validates just that file's areas, and a `cd-*` edit still validates nothing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.md (1)
203-208: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClarify the setup block context.
inputs.*is valid in the current reusable workflows, but not in a normal job such asci.yml'slintjob. Label this block as reusable-workflow-only and add theneeds.prepare.outputs.*form for normal jobs, or provide separate examples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.md around lines 203 - 208, Clarify the “Setup pnpm, Node and dependencies” example as reusable-workflow-only, and provide a separate normal-job example using needs.prepare.outputs.node_version, needs.prepare.outputs.manager, and needs.prepare.outputs.command for the lint job context.Source: Coding guidelines
🧹 Nitpick comments (1)
scripts/assemble-changes.mjs (1)
475-480: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd CI filter regression fixtures.
Assert that
ci__anyandci__deletednever enterchanges.json. Assert thatci__deletedsetsrevalidateAlland enables full CI validation. Do not require CI gating outputs to remain unchanged;revalidateAllintentionally enables them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/assemble-changes.mjs` around lines 475 - 480, Update the regression fixtures around assembleChanges to verify that ci__any and ci__deleted are excluded from changes.json, while ci__deleted sets revalidateAll and enables full CI validation. Do not assert that CI gating outputs remain unchanged when revalidateAll is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/assemble-changes.mjs`:
- Around line 99-110: The current hasUnattributedCiFile flow cannot detect mixed
changes because CHANGES contains filter names rather than paths. Update assemble
and its caller to pass ci__any_files using list-files: json, then compare the
returned paths against ciFiles() so an existing unclaimed CI file alongside an
edited workflow sets revalidateAll; add a regression case for this combination.
If path plumbing is not feasible, make every CI_ANY match trigger full
validation.
---
Outside diff comments:
In @.github/workflows/ci.md:
- Around line 203-208: Clarify the “Setup pnpm, Node and dependencies” example
as reusable-workflow-only, and provide a separate normal-job example using
needs.prepare.outputs.node_version, needs.prepare.outputs.manager, and
needs.prepare.outputs.command for the lint job context.
---
Nitpick comments:
In `@scripts/assemble-changes.mjs`:
- Around line 475-480: Update the regression fixtures around assembleChanges to
verify that ci__any and ci__deleted are excluded from changes.json, while
ci__deleted sets revalidateAll and enables full CI validation. Do not assert
that CI gating outputs remain unchanged when revalidateAll is enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e57070da-082d-492b-9916-fd7254d1b779
📒 Files selected for processing (2)
.github/workflows/ci.mdscripts/assemble-changes.mjs
Benchmark resultsBaseline case:
|
| case | avg | p75 | alloc/iter | vs fastest |
|---|---|---|---|---|
| styled-system color() :: previous | 400 ns | 400 ns | 248 B (+0.0%) | fastest |
| styled-system color() :: local-mjs | 401 ns | 402 ns | 248 B (+0.0%) | +0.4% |
| styled-system color() :: local-cjs | 404 ns | 405 ns | 248 B (least) | +1.1% |
Bundle ReportBundle size has no change ✅ |
The key heuristic went blind past the first claim: a CI file no per-file key names — an aux file inside an action, or a deleted workflow with the predicate filter gone — changed beside an ordinary workflow edit produced the same matched keys as the edit alone, and the edit's claim can be far narrower than what the masked file validated. paths-filter now hands assemble the paths ci__any matched (list-files: json); anything outside ciFiles() triggers full validation and is named in the log, a list that fails to arrive counts as everything, and ci__deleted stays as the deletion backstop. The .md companions are exempt both solo and mixed: they are documentation no job reads, and a README typo ran every job.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/assemble-changes.mjs (1)
475-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd CLI regression tests for the CI fallback cases.
Cover invalid
CI_ANY_FILES, an unclaimed CI path with a claimed workflow edit, andci__deletedwith another CI edit. Assert that all jobs revalidate andchanges.jsonkeepsroot: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/assemble-changes.mjs` around lines 475 - 500, add CLI regression tests for the CI fallback handling around unclaimedCiPaths and assembleChanges, covering invalid CI_ANY_FILES, an unclaimed CI path alongside a claimed workflow edit, and ci__deleted alongside another CI edit. For each scenario, assert that all jobs revalidate and the generated changes.json preserves root: false.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/assemble-changes.mjs`:
- Around line 475-500: add CLI regression tests for the CI fallback handling
around unclaimedCiPaths and assembleChanges, covering invalid CI_ANY_FILES, an
unclaimed CI path alongside a claimed workflow edit, and ci__deleted alongside
another CI edit. For each scenario, assert that all jobs revalidate and the
generated changes.json preserves root: false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ff666c76-04c6-41c8-9c7e-b2ac6bc2c3fe
📒 Files selected for processing (3)
.github/workflows/ci.md.github/workflows/ci.ymlscripts/assemble-changes.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/ci.md
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
1 participant
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.
Closes #330.
Two commits, each standing on its own.
1.
f9b7100— the composite actionEight CI jobs opened with the same four steps, so bumping the pnpm pin or changing the store cache was eight identical edits with one file always at risk of being left behind.
.github/actions/setupnow holds pnpm, Node and the install, and every job calls it:The checkout stays in the job, which is a correction to the issue's acceptance criteria — see the comment on #330. A local action is resolved from the working tree, so
uses: ./.github/actions/setupcannot carry the checkout that puts it there. It is also the step that varies:ci-web.yml'swebjob needsfetch-depth: 0for Codecov base detection.Per-area caches stay in their jobs, moved just below the call — Playwright binaries in three jobs, the Electron binary in two. Nothing writes either path during an install, the Electron binary is fetched by the explicit
rebuild electronstep because the shared install runs--ignore-scripts, and each restore only has to precede the steps that read it.The install passes its arguments through
envand a fixed dispatch rather than splicing them into the run body, the same shapeci-editor.ymlalready uses for the Electron rebuild.Change detection had to learn about the new file
scripts/assemble-changes.mjsbuilt its filter keys by scanning.github/workflows/only, so a PR touching just the new action matched no key: every area job skipped andci-okgreen, on a change that alters how every job installs — this issue's own failure mode in a new shape.The action now goes through the same
wf__channel as a workflow, with its own# ci:validates allmarker. That re-runs every job while leavingchanges.rootfalse, so a CI edit still deploys nothing. Verified against the previous script: the only differences are the added filter key and a log line that now prints a path.2.
2c8979e— one naming scheme for the Actions listThe five area workflows already read
CI · <Area>; the entry workflow and four of the five deploys did not, so the sidebar was a list to read rather than two blocks to scan.ci.ymlCIcd-web.ymlCD · Web (Pages + Storybook)cd-worker-api.ymlCD · Worker (api)cd-worker-bench.ymlCD · Worker (bench)cd-packages.ymlCD · Packages (npm)cd-editor.ymlCD · Editor (release)Chromatic and the labeller stay unprefixed — neither is part of
ci-ok, and a CI prefix would say they gate a pull request when they do not.The load-bearing part of this commit is the lockstep edit.
workflow_runmatches on a workflow'sname:, never its filename, socd-web,cd-worker-apiandcd-worker-benchall move toworkflows: ['CI']in the same change. Miss one and that deploy stops firing with no error. Verified every reference resolves to a workflow that exists. Thirteen doc references move with them, including thecd-packagesrunbook step naming the entry to click in the Actions UI.Branch protection is unaffected — it matches the job name
ci-ok.Review notes
ci.yml's concurrency group is keyed ongithub.workflow, so its value changes with the rename. The group stays internally consistent either side; a run in flight at merge time will not be superseded by the first run after.runsteps must declare ashell:, so the runner default cannot be kept. The arguments are all--flags, so path mangling should not apply — but this is the one thing only a real run confirms. Worth watching thefirefoxrow.ci.md, a pointer in the workflows README, and rules in theci-cdskill covering both the action and theworkflow_runnaming coupling, so the next person starts from them.Verification
pnpm lintclean; full test suite green on both commits via the pre-commit hook (232 files, 1372 tests)prettier --checkclean across everything touchedscripts/assemble-changes.mjs— it sits in no vitest project and has never had one. Verified by executing the old and new versions side by side instead. Worth its own task if we want that script covered; it is load-bearing enough to deserve it.Out of scope, worth a follow-up
The duplication this issue predicts has already drifted outside CI. Seven more setup blocks live in
cd-*.ymlandchromatic.yml, running two differentpnpm/action-setuppins (fc06bc12…v5 and0ebf4713…v6.0.9), threeactions/checkoutvariants and threeactions/setup-nodevariants. Extending the action there needsnode-version-file: .nvmrcsupport plus a pin reconciliation on deploy paths — its own task rather than riding along here.Summary by CodeRabbit
New Features
Improvements
Documentation