Skip to content

ci: add format, lint, type-check and PR title workflows#33

Merged
jhb-dev merged 12 commits into
mainfrom
claude/add-github-ci-workflows-MOgSh
Apr 23, 2026
Merged

ci: add format, lint, type-check and PR title workflows#33
jhb-dev merged 12 commits into
mainfrom
claude/add-github-ci-workflows-MOgSh

Conversation

@jhb-dev
Copy link
Copy Markdown
Contributor

@jhb-dev jhb-dev commented Apr 23, 2026

Summary

This PR establishes automated CI/CD pipelines and adds formatting validation to the project. It introduces GitHub Actions workflows for continuous integration and adds npm scripts for checking code formatting without modifying files.

Key Changes

  • CI Workflow (.github/workflows/ci.yml): Added comprehensive GitHub Actions workflow that runs on every push and pull request to main, including:

    • Format checking for both cms and web packages
    • Linting for both cms and web packages
    • Type checking for cms and Astro checking for web
    • Concurrency controls to cancel in-progress runs when new commits are pushed
  • PR Title Validation (.github/workflows/pr-title.yml): Added workflow to enforce semantic commit conventions on pull request titles with scopes for web and cms

  • Format Check Scripts: Added format:check npm scripts to both packages:

    • cms/package.json: Added format:check script using Prettier in check mode
    • web/package.json: Added format:check script using Prettier in check mode
  • Type Check Script: Added check script to cms/package.json for TypeScript type checking without emitting files

Implementation Details

  • Uses pnpm v9 with Node.js 22 for CI environments
  • Implements workflow concurrency to prevent redundant runs
  • Format and lint checks run in parallel as separate jobs for faster feedback
  • PR title validation is optional (scopes not required) but encourages semantic versioning practices

https://claude.ai/code/session_018Jbn2fhYkgnRgdJ1sreXPG

@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 23, 2026

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

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
website Ignored Ignored Apr 23, 2026 8:52am
website-cms Ignored Ignored Apr 23, 2026 8:52am

Request Review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR establishes CI/CD foundations by adding GitHub Actions workflows for format, lint, and type-check validation, plus PR title enforcement. Overall a solid addition — the structure is clean and follows common conventions.


Potential Bugs / Issues

  • cancel-in-progress: true on push to main: If two commits land in quick succession on main, the first CI run will be cancelled before completing. For a branch that represents "shipped" state, it may be preferable to only cancel in-progress runs for PRs (i.e., conditionally apply cancel-in-progress: ${{ github.event_name == 'pull_request' }}). This prevents a situation where CI on main never has a chance to go fully green.

  • cms type-check uses bare tsc --noEmit: Next.js performs its own type checking during next build with additional context (e.g., auto-generated types, .next/types). Running tsc --noEmit directly can miss Next.js-specific errors or, conversely, produce spurious errors for Next.js-generated declarations. Consider using next build --no-lint (which type-checks as a side effect) or verify that tsc --noEmit is reliable for this setup.

  • No CI trigger on feature branches: The workflow only triggers on PRs targeting main and direct pushes to main. Developers working on feature branches get no CI feedback until they open a PR. Consider adding push: branches-ignore: [main] or removing the branch filter from push altogether, if early feedback is desired.


Performance Considerations

  • Redundant installs across jobs: All three jobs independently run pnpm install --frozen-lockfile. While cache: 'pnpm' caches the pnpm store, the node_modules are reinstalled each time. For a monorepo, consider uploading node_modules as a build artifact from a shared setup job and using needs: to avoid repeating the install step — this can meaningfully cut total CI time.

  • No build job: A build step is currently absent. Build failures (e.g., Payload import map issues, Astro static generation errors) won't be caught in CI. Adding build jobs would give stronger correctness guarantees.


Security Concerns

  • Third-party action pinned to a mutable tag: amannn/action-semantic-pull-request@v5 is referenced by a floating major-version tag. A tag can be force-pushed to point at a different (potentially malicious) commit. Best practice is to pin to an immutable commit SHA, e.g.:

    uses: amannn/action-semantic-pull-request@0303f0f8b0d09059f56f6dac63e5ce72c1ca498  # v5.5.3

    The first-party actions (actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4) carry the same risk but are lower priority given their provenance.

  • permissions block only on pr-title.yml: The CI workflow does not declare explicit permissions. Depending on the repository's default token permissions, this may grant broader access than needed. Consider adding permissions: read-all (or specific minimal permissions) to ci.yml as well.


Minor Notes

  • The format:check scripts correctly omit cross-env NODE_OPTIONS=--no-deprecation — Prettier doesn't need it, so this is intentional and fine.
  • Concurrency group key ci-${{ github.workflow }}-${{ github.ref }} is correct and clear.
  • requireScope: false for PR titles is a sensible default to ease adoption.

Summary

The CI foundation is well-structured. The main items worth addressing before merge are the cancel-in-progress behavior on main, the tsc --noEmit reliability for Next.js, and pinning the third-party action to a commit SHA for supply-chain safety.

Allows astro check to run in CI where CMS_URL/CMS_API_KEY aren't set.
@jhb-dev jhb-dev changed the title Add CI/CD workflows and format check scripts ci: add format, lint, type-check and PR title workflows Apr 23, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

Preview Deployment

Project URL
CMS https://website-7hd2ongwl-jhb-software.vercel.app
Web https://website-19nuoqqr7-jhb-software.vercel.app

The Web preview uses the CMS preview URL for content fetching.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds GitHub Actions CI workflows (format, lint, type-check, PR title validation) and the supporting format:check / check npm scripts. It also fixes a latent bug in getRedirects.ts where missing env vars could cause a runtime crash during type-checking.


Potential Bugs / Issues

getRedirects.ts early-exit placement - The guard is correct and the non-null assertion removals are consistent with it. Minor nit: bypassSecret is still read from process.env before the guard, which is harmless but slightly inconsistent ordering.

format:check / format only target src - Config files at the package root (e.g. astro.config.mjs, eslint.config.js) are excluded. This is inherited from the pre-existing format script, not introduced here, but worth noting since CI now enforces the narrower scope.

cms check script uses bare tsc --noEmit - Next.js projects sometimes require a prior next build to produce generated types. If type errors only surface after a build step, this check may pass in CI while the build would fail. Worth verifying tsc --noEmit alone catches all real type errors in the CMS package.


Performance Considerations

Three independent pnpm install runs - Each job does a full checkout + install. The cache: pnpm on setup-node helps, but a shared setup job uploading node_modules as an artifact would reduce redundant work as the dependency tree grows.

Unpinned tool versions - pnpm/action-setup@v4 with version: 9 and Node 22 are not pinned to patch versions. Pinning (e.g. pnpm 9.12.3, Node 22.14.0) improves reproducibility across CI runs.


Security Concerns

amannn/action-semantic-pull-request@v5 uses a floating tag - A compromised release under this tag could execute arbitrary code in your workflow. Pinning to a full commit SHA is the recommended mitigation (e.g. SHA for v5.5.3).

Missing permissions in ci.yml - The pr-title.yml workflow correctly limits permissions to pull-requests: read. The CI workflow has no explicit permissions block; adding permissions: read-all (or the specific minimal set) follows the principle of least privilege.


Summary

Solid CI foundation and a correct getRedirects.ts fix. Top actionable items:

  1. Pin amannn/action-semantic-pull-request to a commit SHA
  2. Add explicit permissions to ci.yml
  3. Verify tsc --noEmit covers all CMS type errors without a preceding build step

- prettier --write on drifted files
- use set:html template literal for analytics script in 404/500 pages
  so prettier-plugin-astro can parse the surrounding JSX expression
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds automated CI/CD pipelines (format, lint, type-check, PR title validation) and a few formatting-only fixes across the frontend. Solid addition overall.


Potential Bugs / Issues

getRedirects.ts — silent failure in misconfigured production
The new early return when CMS_URL or CMS_API_KEY are missing silently returns {} with only a console.warn. In a production environment where env vars are accidentally unset, this would silently disable all redirects with no hard failure. Consider throwing an error when a production context is detected (e.g. process.env.VERCEL_ENV === 'production'), and reserving the soft skip for CI/dev.

set:html on inline analytics scripts (404 / 500 pages)
Converting <script is:inline> to <script is:inline set:html={...}> is the correct Astro pattern for conditional script blocks, so this is fine for static content. Just note that if dynamic values were ever interpolated into those template literals in the future, it would open an XSS vector — keep the content purely static as it is now.


Performance Considerations

Redundant pnpm install across CI jobs
Each of the three jobs (format, lint, type-check) runs a full pnpm install --frozen-lockfile independently. While cache: 'pnpm' on setup-node mitigates this somewhat, a common pattern to avoid the overhead is a shared install job that the others needs::

install:
  runs-on: ubuntu-latest
  steps:
    - uses: actions/checkout@v4
    - uses: pnpm/action-setup@v4
      with:
        version: 9
    - uses: actions/setup-node@v4
      with:
        node-version: 22
        cache: 'pnpm'
    - run: pnpm install --frozen-lockfile

format:
  needs: install
  ...

This is a minor optimization and the current approach is perfectly functional.


Security Concerns

Third-party action pinned to a mutable tag
amannn/action-semantic-pull-request@v5 is pinned to a major version tag rather than a specific commit SHA. If the upstream maintainer overwrites the v5 tag, the updated code would run in CI without any review. GitHub's security hardening guide recommends pinning to a full SHA:

- uses: amannn/action-semantic-pull-request@0723387faaf9b38adef4775cd42cfd5155ed6017 # v5.5.3

The official GitHub-maintained actions (actions/checkout@v4, actions/setup-node@v4) are lower risk but the same principle applies if stricter supply-chain security is desired.

Missing explicit permissions in ci.yml
The ci.yml workflow doesn't declare a top-level permissions block, so it inherits the repository's default token permissions. Adding permissions: contents: read (or a full explicit block) follows the principle of least privilege and is a low-effort hardening step:

permissions:
  contents: read

Minor Notes

  • Formatting-only changes (import reordering, Tailwind class sorting) are consistent with what the new format:check CI job will enforce going forward — good hygiene.
  • Removing ! non-null assertions in getRedirects.ts after adding the guard is correct and removes a TypeScript safety smell.

jhb-dev added 2 commits April 23, 2026 10:38
CollectionSlug from 'payload' widens to string in this project's setup;
payloadSDK.find requires keyof Config['collections'].
- Merge format/lint/type-check into a single job so pnpm install and
  Node setup run once instead of three times.
- Add top-level permissions: contents: read per least-privilege.
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds CI/CD infrastructure (format check, lint, type check, PR title validation workflows) and fixes several code quality issues (import ordering, Tailwind class ordering, inline script formatting). Mostly straightforward and well-scoped.


Potential Bugs / Issues

Sequential CI steps vs. parallel jobs
All checks are sequential steps inside a single checks job. If the CMS format check fails, you get no lint or type-check results — you have to fix and re-push to see the next failure. Splitting into parallel jobs (e.g., format, lint, type-check) would give complete failure information per run. The trade-off is that pnpm install would run multiple times; this can be mitigated with a setup job that caches node_modules or by accepting the extra install time for richer feedback.

set:html inline scripts in 404.astro / 500.astro
Switching from <script is:inline> to <script is:inline set:html={...}> works correctly here since the content is hardcoded. Just be aware that set:html bypasses Astro's HTML escaping — if any interpolated variable were ever introduced inside those template literals, it would be an XSS vector. No immediate issue, but worth noting as a future caution.

DOMContentLoaded in Plausible scripts (pre-existing, surfaced by the reformatting)
With Astro's <ClientRouter /> (View Transitions), DOMContentLoaded only fires on the initial hard load — not on client-side navigations. Per CLAUDE.md, scripts should use astro:page-load instead. The 404/500 pages are static and likely not navigated to via client routing, so this is low risk in practice, but it's worth aligning with the project convention.


Security Concerns

Third-party action not pinned to a commit SHA

# pr-title.yml
- uses: amannn/action-semantic-pull-request@v5

@v5 is a mutable tag — a maintainer (or a compromised account) could push a new commit under that tag. The security best practice for third-party Actions is to pin to a full commit SHA:

- uses: amannn/action-semantic-pull-request@0135e35e22f8b5a64b7c40d34d5a3f9dd4bee5eb  # v5.5.3

The first-party actions (actions/checkout@v4, pnpm/action-setup@v4, actions/setup-node@v4) carry the same risk; Anthropic/GitHub's own security guidance recommends SHA-pinning all actions.


Performance Considerations

pnpm/node version pinning

version: 9          # pnpm
node-version: 22    # node

Major-only pins mean silent upgrades if pnpm releases 10.x or Node releases 22.x patches with breaking changes. Consider:

  • Using node-version-file: '.node-version' (or .nvmrc) to keep versions DRY
  • Pinning pnpm to a minor: version: "9.x" or reading from packageManager in package.json

No build step in CI
The workflow validates format/lint/types but doesn't run pnpm build. Type errors that only manifest during Astro's full build (e.g., in page templates) would pass CI and only be caught at deploy time. Adding a build step (even behind a condition or separate workflow) would catch this class of issue earlier.


Minor Positives

  • Minimal permissions on both workflows (contents: read, pull-requests: read) — good practice.
  • Concurrency cancellation is correctly scoped to avoid cross-workflow interference.
  • Removing the ! non-null assertions in getRedirects.ts is correct; the new early-return guard is cleaner.
  • keyof Config['collections'] in getPageData.ts is a nice narrowing over the looser CollectionSlug — better type safety with no runtime cost.

Summary

The CI foundation is solid. The two most actionable items before merge are: pin action-semantic-pull-request to a commit SHA (security) and consider whether sequential vs. parallel jobs meets your feedback-loop needs (DX). Everything else is polish-level.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds CI/CD infrastructure: a GitHub Actions workflow for format/lint/type checks, PR title validation enforcing semantic commits, and format:check scripts to both packages. It also includes a defensive env-var guard in getRedirects.ts and various Prettier-driven formatting fixes.


Potential Bugs

DOMContentLoaded vs astro:page-load in 404/500 analytics scripts
web/src/pages/404.astro and web/src/pages/[lang]/500.astro still use DOMContentLoaded inside is:inline scripts. Per CLAUDE.md conventions, scripts that need to fire on every page visit should use astro:page-load because DOMContentLoaded does not re-fire during client-side View Transitions navigation. If a user ever navigates to an error page via the client-side router, analytics would silently be skipped.

- document.addEventListener('DOMContentLoaded', function () {
+ document.addEventListener('astro:page-load', function () {

set:html for inline scripts
Converting <script is:inline>...</script> to <script is:inline set:html={...} /> works, but set:html is Astro's escape hatch for raw HTML — it's semantically intended for HTML content, not script bodies. The correct pattern for inline scripts in Astro is <script is:inline> with the content inside the tags (which Astro preserves as-is). The set:html approach works today but is unconventional and could be confusing.


Performance Considerations

CI steps are sequential, not parallel
The PR description says format and lint checks run "in parallel as separate jobs" — but in the actual ci.yml they are sequential steps inside a single job. A failure in format:check (cms) will block all downstream steps. Splitting into parallel jobs would give faster feedback and allow all checks to report independently:

jobs:
  format:
    runs-on: ubuntu-latest
    steps: [checkout, setup, install, format:check cms, format:check web]
  lint:
    runs-on: ubuntu-latest
    steps: [checkout, setup, install, lint cms, lint web]
  typecheck:
    runs-on: ubuntu-latest
    steps: [checkout, setup, install, check cms, check web]

Security Considerations

getRedirects.ts silently returns {} on missing credentials
The new guard is a good defensive measure for CI/local dev, but it only logs a console.warn. In a production deployment where CMS_URL or CMS_API_KEY is accidentally unset, the site would silently serve no redirects — potentially breaking SEO-critical redirect chains — with no alerting. Consider throwing an error (or at minimum logging at error level) when VERCEL_ENV === 'production':

if (!process.env.CMS_URL || !process.env.CMS_API_KEY) {
  if (process.env.VERCEL_ENV === 'production') {
    throw new Error('CMS_URL and CMS_API_KEY must be set in production')
  }
  console.warn('[getRedirects] CMS credentials not set — skipping redirects fetch.')
  return {}
}

pnpm version: 9 — unpinned major
CI uses version: 9 for pnpm, which will float across all minor/patch releases. For reproducible builds, pin to the exact version that matches the packageManager field in your root package.json (e.g. 9.15.4). This prevents unexpected breakage if a pnpm minor introduces a change in --frozen-lockfile behavior.


Minor / Positive Notes

  • keyof Config['collections'] in getPageData.ts is a nice improvement over the generic CollectionSlug — more type-safe and scoped to the actual project collections.
  • permissions: contents: read and permissions: pull-requests: read on the workflows are correctly minimal.
  • Import reordering and trailing-comma fixes are clean and consistent with Prettier config.

jhb-dev added 2 commits April 23, 2026 10:43
Removes the Node.js 20 deprecation warning on GitHub Actions runners.
… failure

Running format/lint/type-check as separate jobs surfaces all category
failures in one CI run and produces independent PR status checks.
Within each job, the web step runs even if the cms step fails so both
package results are visible in a single run.
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR adds CI/CD automation (format check, lint, type-check, PR title validation) and a handful of formatting fixes. The overall approach is solid — parallel jobs, minimal permissions, and sensible format:check scripts.


Potential Bugs / Issues

1. Action versions likely do not exist (ci.yml)
All three jobs use actions/checkout@v6, pnpm/action-setup@v6, and actions/setup-node@v6. As of late 2025, the latest stable releases were all at v4. If v6 does not exist the workflows will fail on first run. Recommend verifying and using @v4 (or pinning to commit SHAs for security).

2. cancel-in-progress: true applies to pushes to main
The concurrency group is scoped to github.ref, so back-to-back pushes to main will cancel each other's CI runs. If branch protection requires a green CI status, a fast follow-up push could leave the earlier merge without a completed check. A common fix is to only cancel in-progress runs for PRs:

concurrency:
  group: ci-${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: ${{ github.event_name == 'pull_request' }}

3. format:check only covers the src/ directory
Both prettier --check src scripts skip root-level config files (astro.config.mjs, eslint.config.js, tailwind.config.js, etc.). If those files drift from Prettier style, CI will not catch it. Consider prettier --check . with an appropriate .prettierignore.

4. DOMContentLoaded in 404/500 scripts (CLAUDE.md convention)
CLAUDE.md states client-side scripts should use astro:page-load instead of DOMContentLoaded to fire correctly after View Transition navigations. The reformatted inline scripts in 404/500 still use DOMContentLoaded. Low risk in practice (these pages are likely full navigations), but inconsistent with the project convention.


Performance Considerations

Each of the three CI jobs independently runs pnpm install --frozen-lockfile. The cache: 'pnpm' option restores the pnpm store cache, but node_modules is still reinstalled per job. This is the standard trade-off and fine here; a shared install job with artifact upload would speed things up if build times become a concern.


Security Considerations

  • Action SHA pinning: Using major-version tags rather than full commit SHAs means a compromised tag rewrite could execute arbitrary code in CI. For higher assurance, pin to SHAs (e.g. actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4).
  • Permissions: Both workflows declare minimal permissions (contents: read, pull-requests: read) — well done.
  • set:html scripts: The template literals in 404/500 contain no user-supplied data, so there is no XSS risk from that change.
  • getRedirects env-var guard: The new early-return silently skips the redirects fetch when CMS_URL/CMS_API_KEY are absent (intentional for CI). However, if these vars are accidentally unset in production, redirects will silently stop working with only a console.warn. Worth asserting these vars are present in the production build step.

Minor / Formatting

The remaining changes (import reordering, Tailwind class ordering, trailing commas, script tag reformatting) are clean Prettier/ESLint fixes with no behavioural impact.


Summary: The main actionable items are the action version numbers (likely broken as-is), the concurrency behaviour on main, and the narrow scope of format:check. Everything else is low-severity.

- web/404.astro: tailwind class order shuffled
- web/ci-test-violations.ts: unused var + number/string mismatch
- cms/ci-test-violations.ts: unsorted imports + formatting + type error

To be reverted after CI confirms all 6 sub-checks fail.
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR establishes CI/CD pipelines with format checking, linting, and type checking workflows, plus format:check / check scripts in both packages. The bulk of the diff is automated Prettier reformatting (import order, Tailwind class order, inline script style).


🔴 Must Fix Before Merging

Test violation files left in the PR
Both cms/src/ci-test-violations.ts and web/src/ci-test-violations.ts are explicitly marked "To be reverted" but are still present. They contain intentional type errors (number assigned a string literal) and unused variables that will fail the very CI checks this PR introduces. These must be removed before merging.

Action versions likely incorrect
All three setup actions use versions that don't exist:

uses: actions/checkout@v6        # latest stable is @v4
uses: actions/setup-node@v6      # latest stable is @v4
uses: pnpm/action-setup@v6       # latest stable is @v4

Using non-existent action versions will cause every CI run to fail at the runner setup step. Pin to the current latest (@v4).


🟡 Potential Issues

No build job in CI
Format, lint, and type checks can all pass while the actual build fails (e.g. missing env vars at build time, Astro config errors). Consider adding a build step — even a dry-run — so broken builds are caught on PRs.

DOMContentLoaded vs astro:page-load in error pages
The inline scripts in 404.astro and 500.astro use DOMContentLoaded. Per CLAUDE.md, scripts that need to fire on every page visit should use astro:page-load because of View Transitions. Error pages may be navigated to via client-side routing, so the Plausible events could silently fail to fire on those navigations. Worth aligning with the documented convention.


🟢 Positive Changes

  • getRedirects.ts guard: The early-return when CMS_URL / CMS_API_KEY are unset is a clean fix that avoids crashes during CI type checks and local dev without a CMS. Removing the ! non-null assertions after the guard is correct.
  • getPageData.ts type improvement: keyof Config['collections'] is more precise than the generic CollectionSlug from Payload — it catches invalid collection names at compile time.
  • Concurrency controls: Cancelling in-progress runs on new pushes is the right default for a monorepo CI.
  • Separate jobs for format/lint/type-check: Running as parallel jobs gives faster feedback and cleaner failure isolation.
  • set:html for inline scripts: Avoids Astro's template literal interpolation and is consistent with how the analytics script is handled in Layout.astro.

Minor

  • The ci- prefix in the concurrency group key is redundant since github.workflow already scopes uniqueness. Not a bug, just noise.
  • The Prettier-reordered Tailwind classes in 404.astro (flex-col flex at the end) look odd but are correct — this is what the Tailwind Prettier plugin produces.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR establishes a CI/CD foundation with format, lint, and type-check workflows plus PR title validation. The bulk of the non-workflow changes are Prettier-driven formatting fixes (import ordering, Tailwind class ordering, trailing commas).


Potential Bugs / Issues

Critical: Invalid action versions in ci.yml

The workflow references versions that do not exist:

- uses: actions/checkout@v6        # latest stable: v4
- uses: pnpm/action-setup@v6       # latest stable: v4
- uses: actions/setup-node@v6      # latest stable: v4

These will fail at runtime (or resolve unpredictably). Change all three to @v4.

getRedirects.ts – silent skip during full builds

The new guard:

if (!process.env.CMS_URL || !process.env.CMS_API_KEY) {
  console.warn('[getRedirects] CMS_URL or CMS_API_KEY not set — skipping redirects fetch.')
  return {}
}

is correct for CI type-checks, but if these env vars are accidentally missing in a real deployment the site would silently serve no redirects. Consider throwing in non-CI environments, or at least making the log message clearly distinguishable (e.g. console.error) so monitoring picks it up.


Performance Considerations

  • All three CI jobs (format, lint, type-check) independently run pnpm install --frozen-lockfile. The cache: 'pnpm' setting in setup-node helps, but the installs still run three times. Extracting a shared setup job with outputs + needs, or using a composite action, would reduce cold-cache install time.
  • Jobs run in parallel, which is good. Within each job, both packages are checked sequentially with if: ${{ !cancelled() }} so both results are always reported even on first failure — that's the right call.

Security Considerations

  • Third-party actions (amannn/action-semantic-pull-request@v5) are pinned to a mutable major-version tag, not a SHA digest. For supply-chain hardening, pin to a full SHA (e.g. amannn/action-semantic-pull-request@0aeff8913784aeed24d42a3b84f46c776b7cc6d).
  • The set:html conversion on the inline <script> tags in 404.astro and 500.astro is safe — content is fully static with no user-controlled values.
  • ci.yml correctly uses permissions: contents: read (least-privilege). The PR title workflow correctly uses permissions: pull-requests: read.

Minor

  • The concurrency group ci-${{ github.workflow }}-${{ github.ref }} includes ${{ github.workflow }} but the group is already prefixed with a literal ci-. Since you only have one CI workflow, this is fine, but the prefix is redundant — ${{ github.workflow }}-${{ github.ref }} would be cleaner.
  • The format:check scripts in both packages only check the src directory (matching the format scripts). This is intentional and consistent.
  • The type narrowing change in getPageData.ts (CollectionSlugkeyof Config['collections']) is a good improvement — it restricts the parameter to slugs that actually exist in this project.

Summary

The action version mismatch is a blocker — the CI workflows will fail immediately on first run. Everything else is solid; the formatting fixes and getRedirects guard are clean improvements.

@jhb-dev jhb-dev merged commit 3f5bd3d into main Apr 23, 2026
10 checks passed
@claude
Copy link
Copy Markdown

claude Bot commented Apr 23, 2026

Code Review

Overview

This PR establishes CI/CD pipelines (format, lint, type-check, PR title validation) and adds related npm scripts. It also includes several formatting-only fixes and a defensive guard in getRedirects.ts.


Potential Bugs / Issues

Action versions may not exist
actions/checkout@v6, pnpm/action-setup@v6, and actions/setup-node@v6 are referenced across all three jobs. As of the last stable releases for these actions (v4 for each), @v6 tags do not exist and will cause workflow failures. Verify these tags exist or downgrade to @v4.

DOMContentLoaded vs astro:page-load in 404/500 scripts
Per CLAUDE.md, client-side scripts should wrap code in document.addEventListener('astro:page-load', ...) to handle Astro's View Transitions correctly. The 404 and 500 page scripts still use DOMContentLoaded (the diff is formatting-only here, not a regression introduced by this PR — but worth tracking as a follow-up).

No build job in CI
Format, lint, and type-check are validated, but a broken build won't be caught. Consider adding a build step or noting this as a known gap.


Performance Considerations

Repeated dependency installs across jobs
Each of the three CI jobs (format, lint, type-check) runs pnpm install --frozen-lockfile independently. Using actions/cache for the pnpm store and sharing the node_modules layer (e.g., via an upload/download artifact or a single install job) would speed up total CI time noticeably.


Security Considerations

Actions pinned to tags, not SHAs
Using @v5/@v6 floating tags means a compromised tag could silently change what runs in CI. For security-sensitive workflows, pinning to a full commit SHA is the recommended practice.

set:html for inline analytics scripts
The 404 and 500 pages now use set:html to inject the Plausible scripts. Since the content is a static string literal (not user input), this is safe. However, set:html bypasses Astro's built-in escaping — a short comment explaining that the content is intentionally static would help future readers avoid cargo-culting this pattern with dynamic data.


Positive Notes

  • The getRedirects.ts guard against missing env vars is a solid defensive fix — removes ! non-null assertions and prevents CI type-check failures from missing credentials.
  • keyof Config['collections'] in getPageData.ts is a more precise type than the generic CollectionSlug from Payload.
  • Concurrency controls (cancel-in-progress: true) are correctly scoped per branch/ref.
  • if: ${{ !cancelled() }} on the web steps ensures both packages are always checked, even if the cms step fails.

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.

1 participant