Skip to content

refactor(ci): extract composite actions and improve workflow hygiene#314

Merged
james-elicx merged 3 commits intomainfrom
opencode/playful-circuit
Mar 7, 2026
Merged

refactor(ci): extract composite actions and improve workflow hygiene#314
james-elicx merged 3 commits intomainfrom
opencode/playful-circuit

Conversation

@james-elicx
Copy link
Copy Markdown
Collaborator

@james-elicx james-elicx commented Mar 7, 2026

Summary

  • setup-node-pnpm composite action replaces the repeated 3-step pnpm/node/install triplet across all eligible workflows — every workflow that does setup and install now uses it consistently
  • Playwright browser cache added to E2E jobs — avoids re-downloading ~150MB of Chromium on every CI run
  • Concurrency group added to deploy-examples.yml — prevents deploy jobs piling up on busy branches
  • CJS → bash in create-next-app job — replaced the inline node {0} + require() script with a portable bash loop
  • CJS → ESM in ecosystem-run.yml — all inline Node heredocs converted from require() to node --input-type=module with import statements

Exceptions (intentionally kept inline)

  • bonk.yml / bigbonk.ymlissue_comment-triggered workflows; GHAS flags local composite actions reachable from these as untrusted-checkout TOCTOU
  • tip.yml notify-on-failure job — only needs plain setup-node, no pnpm or install

- Add composite actions: setup-node-pnpm, build-vinext, deploy-example, comment-preview-urls
- Cache Playwright browsers in CI to avoid ~150MB download per E2E run
- Add concurrency group to deploy-examples.yml to prevent deploy pileup
- Replace inline CJS node script in create-next-app job with portable bash
- Convert ecosystem-run.yml inline Node scripts from CJS require() to ESM
- Apply composite actions across all workflows to eliminate repeated boilerplate
…site action

Remove the build-vinext, deploy-example, and comment-preview-urls composite
actions and inline their steps back into each workflow. Keep setup-node-pnpm
as the one reusable action (pnpm install + node setup). Retain all other
improvements from the previous commit: concurrency groups, Playwright browser
cache, bash-based dev-server check, ESM node scripts in ecosystem-run.
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 7, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@314

commit: 72db039

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 7, 2026

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original

Every workflow that does pnpm/node setup + install now uses the
setup-node-pnpm composite action instead of the inline 3-step triplet.
Added optional registry-url input to support publish.yml's npm auth.

Exceptions (intentionally kept inline):
- bonk.yml / bigbonk.yml: issue_comment trigger, GHAS constraint
- tip.yml notify-on-failure: node-only, no pnpm install needed
@james-elicx
Copy link
Copy Markdown
Collaborator Author

/bigbonk review

@james-elicx james-elicx marked this pull request as ready for review March 7, 2026 13:21
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean refactor — the dedup is well done and all the conversions are functionally equivalent. Two things worth noting:

GHAS alerts on setup-node-pnpm: The rationale for keeping bonk.yml/bigbonk.yml inline is sound. Those are issue_comment-triggered with contents: write, so referencing local composite actions creates a TOCTOU vector where a malicious PR could modify the action before checkout. The existing GHAS alerts on build-vinext and deploy-example are pre-existing (from deploy-preview-command.yml, also issue_comment-triggered) — this PR doesn't make that worse.

registry-url default of "": The actions/setup-node action treats an empty string for registry-url as "don't configure a registry", so passing through the empty default works correctly. Only publish.yml sets it to the npm registry, which is the right behavior.

Specific observations on each change:

  1. Composite action — Correct 1:1 replacement. Every inline triplet used node-version: 24, cache: pnpm, and --frozen-lockfile, which is exactly what the action defaults to.

  2. publish.yml working-directory removal — The old workflow ran pnpm run build with working-directory: packages/vinext. The new version runs pnpm run build at the repo root, which delegates via pnpm --filter vinext run build in the root package.json. Functionally equivalent.

  3. create-next-app CJS→bash — The bash version using curl + kill is simpler and more readable. One behavioral difference: the old Node version used taskkill on Windows for process cleanup. Since both ubuntu-latest and windows-latest runners now use shell: bash, and kill works on Windows bash (Git Bash / MSYS2), this is fine. The kill "$SERVER_PID" 2>/dev/null || true pattern handles the case where the process already exited.

  4. ecosystem-run.yml CJS→ESMnode --input-type=module with heredoc is the correct way to run ESM from stdin. The old node - approach used CJS require() which works but is deprecated-path. The require('./packages/vinext/package.json')readFileSync + JSON.parse is necessary because JSON imports need assertions in ESM. Functionally equivalent.

  5. Playwright cache — Cache key playwright-chromium-${{ runner.os }}-${{ hashFiles('**/package.json') }} is reasonable. The playwright install chromium command is idempotent — if the cache hits and the version matches, it's a no-op. If Playwright gets bumped, package.json changes and the cache misses. The glob **/package.json is broad (includes fixtures/examples) but that's fine — better to over-invalidate than under.

  6. Deploy concurrency groupdeploy-${{ github.event_name }}-${{ github.ref }} means push-to-main and PR deploys get separate groups, and multiple pushes to the same PR cancel previous runs. Correct.

  7. Whitespace cleanup in deploy-examples.yml — trailing spaces removed in the JS script block. Purely cosmetic.

LGTM.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Mar 7, 2026

Reviewed and approved PR #314. Here's the summary:

Verdict: LGTM

The PR cleanly extracts the repeated pnpm/node/install triplet into a composite action and applies it consistently across all 7 eligible workflows. The key points:

  • Composite action is a faithful 1:1 replacement of the inline steps. The registry-url input with empty default correctly handles the publish.yml case (only workflow needing npm registry config).
  • publish.yml build step — removing working-directory: packages/vinext is correct because the root pnpm run build delegates via pnpm --filter vinext run build.
  • CJS→bash conversion in create-next-app — the bash version with curl/kill is simpler and works on both Ubuntu and Windows runners (both use Git Bash).
  • CJS→ESM conversion in ecosystem-run.ymlnode --input-type=module with readFileSync + JSON.parse is the correct ESM equivalent of require() for JSON.
  • Playwright browser cache — cache key invalidates on any package.json change, and playwright install is idempotent when the cache hits.
  • Deploy concurrency — group key separates push vs PR events, cancels stale runs on the same ref.
  • GHAS alerts — the rationale for keeping bonk.yml/bigbonk.yml inline is sound (TOCTOU risk on issue_comment-triggered workflows with write permissions).

github run

@james-elicx james-elicx merged commit 90771f7 into main Mar 7, 2026
23 checks passed
@james-elicx james-elicx deleted the opencode/playful-circuit branch March 7, 2026 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants