ci: verify pull requests before they reach main - #62
Conversation
Nothing checked pull requests. The only workflow was the Pages deploy, which runs on push to main, so a change that broke the build was found either by the deploy failing or by the live site changing. On the four open Dependabot PRs the sole check is DCO. That matters most for the dependency bumps: #50 and #59 move Next from 16.2.12 to 16.3.x and sharp from 0.34 to 0.35, and AGENTS.md already warns that this Next.js has breaking changes between versions. Merging those with nothing having built the site is how a broken bump reaches the live course. Runs lint, tsc --noEmit, and the production build, on Node 24 to match the deploy workflow so a PR cannot pass here and fail there. The build is the load-bearing step: this is a static export, so it renders every route, and a missing generateStaticParams or a manifest entry pointing at nothing fails here instead of in production. This is also the prerequisite for auto-approving Dependabot: without a build check, auto-merge would land bumps unverified. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
The workflow failed on its own pull request, which is the useful kind of failure: main already has three react-hooks/set-state-in-effect errors, in CatalogClient, LevelCatalogClient and ModulePlayerClient. All three are the same pattern - reading saved progress from localStorage on mount - and that effect is deliberate. localStorage does not exist during prerender, and a lazy useState initialiser would make the prerendered HTML differ from the client render and mismatch on hydration. Satisfying the rule honestly means moving to useSyncExternalStore in three components, which is a separate change with its own risk to progress behaviour. So lint runs but does not block. A required check that main itself fails would make every pull request red and teach everyone to ignore CI, which is worse than not having the check. tsc and the build stay blocking - those are the ones that catch a broken dependency bump, which is what this workflow is for. AI-Assisted-By: Claude Opus 5 (1M context) Signed-off-by: Anna Larch <[email protected]>
|
Pushed a fix after this failed on its own pull request, which is the useful kind of failure.
That effect is deliberate. So lint now runs but does not block. A required check that
Worth flipping lint to blocking once those three are resolved either way (refactor, or a documented rule disable with the prerender reasoning written down). |
There was a problem hiding this comment.
🟢 Approval recommended
The workflow is consistent with the existing Pages deploy environment (Node 24, build command) and adds low-risk, high-value PR verification.
Pull request overview
Adds a new GitHub Actions workflow to validate pull requests before merging to main, ensuring the Next.js site can install dependencies, typecheck, and successfully build (matching the existing Pages deploy environment).
Changes:
- Introduces a
pull-request.ymlworkflow triggered on PRs tomain(and manually). - Runs
npm ci, lint (non-blocking),tsc --noEmit, andnpm run buildon Node 24. - Uses concurrency to cancel superseded runs for the same PR ref.
File summaries
| File | Description |
|---|---|
| .github/workflows/pull-request.yml | New PR CI workflow that installs, lints, typechecks, and builds on Node 24 to catch failures before merge. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Why
Nothing currently checks pull requests. The only workflow is the Pages deploy, which runs on push to
main, so a change that breaks the build is discovered either by the deploy failing or by the live site changing. On the four open Dependabot PRs, the only check is DCO.That matters most for the dependency bumps sitting open right now:
16.2.12 → 16.3.0, build(deps): Bump sharp and next #59 moves it to16.3.4, both withsharp0.34 → 0.35.AGENTS.mdin this repo already warns that this Next.js has breaking changes between versions.Merging those with nothing having built the site is how a broken bump reaches the live course.
What it runs
npm ci, then lint,tsc --noEmit, then the production build — on Node 24, matchingdeploy-pages.ymlso a PR cannot pass here and then fail on deploy.The build is the load-bearing step. This is a static export, so building renders every route: a missing
generateStaticParams, a module whose markdown was deleted, or a manifest entry pointing at nothing all fail here instead of in production.tscis run separately even thoughnext buildtypechecks, because a dedicated step gives a clearer failure than a build dying mid-compile.Note
This is also the prerequisite for auto-approving Dependabot. Without a build check, auto-merge would land bumps unverified. With it, auto-approve for patch and minor updates becomes reasonable, with majors kept manual.
AI-Assisted-By: Claude Opus 5 (1M context)