Support demos build with npm - #4
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🟡 Changes recommended
The new npm-build path should handle/communicate missing lockfiles more clearly (or adjust install strategy) and the documentation should be unambiguous about the entrypoint/publishing expectations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds support for building and publishing demo apps to GitHub Pages via an npm build step, enabling demos that use tooling like Vite/TypeScript to be deployed as static dist/ output.
Changes:
- Documented how Pages publishing works for demos with
package.jsonandnpm run build. - Updated the Pages assembly script to run
npm ci && npm run buildand publishdist/when a demo containspackage.json. - Updated the Pages workflow to install Node.js before assembling the site.
File summaries
| File | Description |
|---|---|
| demos/README.md | Documents publishing rules for static vs npm-built demos and expected build output (dist/). |
| demos/assemble-pages.sh | Adds npm build support for demos with package.json and publishes dist/ output. |
| .github/workflows/pages-demos.yml | Ensures Node is available in CI so demo builds can run. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The workflow now runs npm builds on pull_request, which should be gated to avoid executing arbitrary scripts for forked PRs, and the new npm ci path should clearly enforce/document lockfile requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
demos/README.md:26
- This text implies any demo with
package.jsonwill build vianpm ci, butnpm cifails without a lockfile. Consider documenting the lockfile requirement explicitly so contributors don’t discover it only via CI failures.
Official static host: [https://robotemi.github.io/openapi/](https://robotemi.github.io/openapi/) (index of every client-only demo). A folder is published when it has `index.html` and no `vercel.json`. If the folder has `package.json`, Pages runs `npm ci && npm run build` and publishes `dist/`.
demos/assemble-pages.sh:74
npm cirequires a lockfile (package-lock.jsonornpm-shrinkwrap.json). As written, any demo that addspackage.jsonwithout a lockfile will fail with a less clear npm error; it’s better to fail fast with a targeted message (and optionally disable audit/fund network calls for CI speed/noise).
if [ -f "${dir}package.json" ]; then
echo "assemble-pages: building $name"
(cd "$dir" && npm ci && npm run build)
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
e0ddf9f to
7230503
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The workflow likely lacks required pages permissions for the configure-pages step in the assemble job, which may break master deployments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
7230503 to
5d389dd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new documentation/rules text contains a few mismatches/overstatements relative to the actual assembly behavior and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
demos/README.md:42
- The contributing checklist for Vite/TypeScript demos requires
package-lock.json, but the actual assemble script also acceptsnpm-shrinkwrap.json. The README should mention shrinkwrap here too so the guidance matches the implementation.
4. Client-only static demos (no build): add `index.html` at the folder root, omit `package.json` and `vercel.json`, and use a URL-safe folder name (`A–Z a–z 0–9 . _ -`). Merge to `master` and the Pages workflow copies the folder to `https://robotemi.github.io/openapi/<name>/`.
5. Client-only demos that use Vite/TypeScript: commit `package.json` **and** `package-lock.json`, with `npm run build` writing `dist/index.html`. Set Vite `base` to `./`. Omit `vercel.json`. Do not commit `node_modules/` or `dist/`. Pages runs `npm ci && npm run build` and publishes `dist/`.
6. Serverless demos: include `vercel.json`. Deploy as a separate Vercel project.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
5d389dd to
d0ae8de
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated workflow’s job-level permissions omit actions: read, which can break artifact upload/download and prevent deployment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/pages-demos.yml:53
actions/download-artifact@v4requiresactions: readwhen permissions are locked down per-job; currently the deploy job permissions omit it, which can prevent downloading thesiteartifact and break deployment.
runs-on: ubuntu-latest
permissions:
contents: read
pages: write
id-token: write
steps:
- uses: actions/download-artifact@v4
with:
name: site
path: _site
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
d0ae8de to
9932b87
Compare
9932b87 to
22a5759
Compare
This pull request updates the process and documentation for publishing OpenAPI demos to GitHub Pages, focusing on improved safety, clearer rules for demo publishing, and stricter CI/CD controls. The changes clarify how demo folders are selected and built, introduce safety checks in the assemble script, and update the workflow to ensure only trusted code is built and deployed.
Demo publishing and build process improvements:
vercel.jsonand either (a) a rootindex.htmlwith nopackage.json(entire folder copied), or (b) apackage.jsonand a lockfile, in which case CI builds and publishes thedist/directory. Builds without a lockfile now fail fast. [1] [2] [3] [4]assemble-pages.shscript now uses canonical path normalization for the output directory and refuses to operate on unsafe paths, improving safety.Workflow and CI/CD controls:
assemble(builds site artifacts) anddeploy(publishes to Pages), with strict conditions: only same-repo PRs and themasterbranch are built or deployed, and fork PRs are excluded from running arbitrary builds..nojekyllis preserved for Pages.Documentation and rules:
.cursor/rules/demos-pages.mdc) specifies token safety, demo folder naming, assemble script rules, and workflow trust boundaries, aligning the README and script.README.mdis updated to match the new publishing and build rules, with clear instructions for different demo types and new guidance for PR contributors. [1] [2]These changes collectively ensure a safer, more predictable, and well-documented process for demo publishing and CI/CD.