site: regenerate from static-tool-page-template#24
Conversation
Rebuilds the Astro/Starlight site on the static-tool-page-template baseline: centralizes project metadata in src/site.config.mjs, replaces the per-module component CSS files with a generated theme.css plus rewritten landing.css, converts docs pages from .md to .mdx, and adds Playwright/axe tests, cspell, Prettier, and tsconfig. Splits the single pages.yml workflow into a site-check PR validator and a site-publish deploy workflow scoped to site/** paths, and updates AGENTIC_NAVIGATION_GUIDE.md to match the new tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code ReviewOverviewThis PR rebuilds the IssuesSecurity — action pins regressed to floating tagsThe deleted uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1Both new workflows use floating version tags: uses: actions/checkout@v5
uses: actions/setup-node@v5Floating tags can be moved by an account compromise or a force-push. Consider pinning to SHAs as the old workflow did — or at minimum add a comment explaining the deliberate policy change (e.g. Dependabot will keep them updated). Performance — npm cache dropped from CIThe old - uses: actions/setup-node@...
with:
node-version: 22
cache: npm
cache-dependency-path: site/package-lock.jsonNeither Maintainability —
|
| File | Line |
|---|---|
site/src/site.config.mjs |
"basePath": "/agentic-navigation-guide" |
site/playwright.config.ts |
const basePath: string = "/agentic-navigation-guide" |
site/tests/site.spec.ts |
const basePath: string = "/agentic-navigation-guide" |
If the deploy path changes, all three need to be updated in sync. playwright.config.ts and site.spec.ts could import from site.config.mjs:
import { siteConfig } from "../src/site.config.mjs";
const basePath = siteConfig.site.basePath;The same applies to projectTitle and projectDescription in site.spec.ts — they are copied verbatim from siteConfig.project.*.
Minor observations
-
docs.sidebar↔docs.pagesdata duplication insite.config.mjs: these two arrays overlap on title/slug for the same four pages. A future addition would require touching both. Not a blocker, but worth a note in a comment that they must stay in sync. -
package.jsonsemver floor lowered:@astrojs/starlightdrops from^0.39.2→^0.39.0andastrofrom^6.3.1→^6.0.0. The lock file pins actual resolved versions so this won't cause a runtime change today, but it's a wider allowable window. Likely intentional for template compatibility. -
Missing
astroscript alias: the oldpackage.jsonhad"astro": "astro"as a shorthand; the new one doesn't. Minor, butnpx astro ...still works.
What's well done
- Accessibility improvements: skip-to-content link,
aria-controls="mobile-nav",aria-expandedtoggling, and Escape-key close for mobile nav are all implemented correctly. - The
landing.tscopy-button handler correctly clears the reset timer before starting a new one, avoiding race conditions. tsconfig.jsonwithastro/tsconfigs/strict+checkJs: truegives good coverage of the.mjsconfig file.- The Playwright test suite is thorough: viewport overflow, link validity (including hash-target existence), and axe audits on key pages.
- The
site-check.yml/site-publish.ymlsplit is a clean separation of concerns.
🤖 Generated with Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 995abe8d40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "typescript": "^6.0.0" | ||
| }, | ||
| "engines": { | ||
| "node": ">=22" |
There was a problem hiding this comment.
Raise the advertised Node floor to 22.12
For local or engine-strict installs on Node 22.0–22.11, this package now advertises support even though the committed lock resolves astro@6.3.1, whose own engines entry requires node >=22.12.0; those environments can fail npm ci/build despite satisfying this top-level constraint. Please align this to >=22.12.0 so contributors and tooling reject unsupported Node versions up front.
Useful? React with 👍 / 👎.
Apply the local half of plx/static-tool-page-template#3 ahead of that PR landing: - SHA-pin every third-party action in site-check.yml and site-publish.yml; restores the security posture the old pages.yml workflow had before regeneration. - Add `cache: npm` + `cache-dependency-path: site/package-lock.json` to setup-node so the new devDependencies (Playwright, axe, cspell, Prettier, TypeScript) reuse cached installs. - Have site/playwright.config.ts and site/tests/site.spec.ts import basePath, projectTitle, projectDescription, and docsPages from src/site.config.mjs instead of duplicating them. Eliminates three places where the deploy path could drift. - Add an "astro": "astro" script alias to site/package.json. After plx/static-tool-page-template#3 lands and we re-render, this patch becomes a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code ReviewOverviewThis PR regenerates the Astro/Starlight site from a
The overall direction is good — separating CI concerns, adding accessibility testing, and centralizing site config are all meaningful improvements. IssuesDead branch in both CI workflows In run: |
if [ -f package-lock.json ]; then
npm ci
else
npm install
fi
Missing
Dependency version ranges downgraded "@astrojs/starlight": "^0.39.0" // was ^0.39.2
"astro": "^6.0.0" // was ^6.3.1The lock file pins the actual versions, so this won't matter day-to-day, but the declared ranges now allow older patch releases that the project apparently wasn't using before. If the downgrade is intentional to match the template baseline, a comment explaining the decision would help. Minor Notes
// Generated by static-tool-page-template. Edit this file and docs content before changing
// stock landing page components.The file is both generated and the canonical source of truth going forward. The comment direction ("edit this file … before changing stock components") is correct, but "Generated" implies it can be regenerated and overwritten. Consider rewording to make clear this file owns project configuration.
Playwright audit scope ( What's Good
|
Astro 6.3.1 (the locked version) requires node >=22.12.0, so advertising >=22 here let engine-strict installs on Node 22.0–22.11 pass the top-level check then fail later during npm ci/build. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review: site — regenerate from static-tool-page-templateOverall this is a clean, well-structured rebuild. The split of a single workflow into validation vs. deploy, the centralized Bugs / Potential Issues1. -*,
-*::before,
-*::after {
+* {
box-sizing: border-box;Removing 2. .sl-markdown-content pre {
overflow-x: visible;
overflow-wrap: anywhere;
white-space: pre-wrap;
}Per the CSS spec, setting 3. External link label check misses if (!link.label) {
failures.push(`${pagePath}: external link ${link.href} has no text label`);
}
Minor / Style4. if [ -f package-lock.json ]; then npm ci; else npm install; fi
5. Redundant
6. Both new workflows pin to commit SHAs labeled 7. const pagesToAudit = ["/", docsPages[0]?.href].filter(Boolean);Accessibility violations on the remaining 3 docs pages won't be caught. Even at the cost of a few extra seconds, running axe on all 8. -"@astrojs/starlight": "^0.39.2",
-"astro": "^6.3.1"
+"@astrojs/starlight": "^0.39.0",
+"astro": "^6.0.0"The lock file pins the actual installed versions, so this has no runtime impact now. But the range is wider than the version the code was developed against, which could surprise someone doing a fresh install from the manifest without a lock file. Consider keeping the ranges at the versions actually used. Positives Worth Calling Out
🤖 Generated with Claude Code |
…g comment) - Drop the unreachable `if [ -f package-lock.json ]` branch in both site workflows; the lock file is committed, so just `npm ci`. - Add prettier-plugin-astro as a devDep and a `.prettierrc` that registers the plugin and the `.astro` parser override. Format the existing Astro landing page accordingly; without this, `format:check` was silently skipping `.astro` files. - Bump declared dependency floors to `@astrojs/starlight ^0.39.2` and `astro ^6.3.1` to match the lockfile (the prior `^0.39.0` / `^6.0.0` ranges allowed older patch releases the project already moved past). - Re-word the `site.config.mjs` leading comment so it's clear this file is the canonical project configuration, not a regenerable artifact. - Track the new `.prettierrc` in the navigation guide. Upstream template fixes filed at plx/static-tool-page-template#5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — PR #24: site: regenerate from static-tool-page-templateOverviewThis PR rebuilds the Astro/Starlight site from a
The overall direction is solid. The config centralization, workflow split, and Playwright suite are all clear improvements over the prior state. Potential Issues1. Escape key: focus not returned to toggle after mobile nav closes ( document.addEventListener("keydown", (event) => {
if (event.key === "Escape") {
setNavOpen(false); // closes panel but does not move focus
}
});When a keyboard user presses Escape to dismiss the nav panel, focus stays wherever it was inside the panel (likely on a nav link), which can disorient screen-reader users. The toggle button is the logical focus target after dismiss. Adding document.addEventListener("keydown", (event) => {
if (event.key === "Escape") {
setNavOpen(false);
navToggle?.focus();
}
});The Playwright test for this flow ( 2. CI Playwright tests run against the dev server, not the production build ( webServer: {
command: `npm run dev -- --host 127.0.0.1 --port ${testPort}`,In Consider switching the webServer: {
command: `npm run preview -- --host 127.0.0.1 --port ${testPort}`,
// requires 'npm run build' to have run first3. Both arrays describe the same four pages with largely the same fields ( 4. The old Minor Notes
SummaryThe structural changes are well thought out. The |
- landing.css: restore `::before`/`::after` in the box-sizing reset so pseudo-elements (e.g. `.command-card::before` overlay) inherit border-box sizing. - starlight.css: drop `overflow-x: visible` on `.sl-markdown-content pre`; `white-space: pre-wrap` + `overflow-wrap: anywhere` already wrap long lines, and `visible` would get promoted to `auto` once Starlight sets `overflow-y` to anything else. - site.spec.ts: read `aria-label` alongside `textContent` for external links and only flag links missing both. Rename failure message to "no accessible label". - site.spec.ts: expand `pagesToAudit` to cover every page in `pagesToCheck`, so axe runs against the docs pages too (27 tests vs 18 before). - site-check.yml: drop the redundant `push: branches: [main]` trigger; `site-publish.yml` already validates the build on merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review: site: regenerate from static-tool-page-templateOverviewThis PR rebuilds the Astro/Starlight site on a new template baseline, centralises all project metadata in What Works Well
Issues and Suggestions1. Playwright browser cache not configured in CI (performance)
- name: Cache Playwright browsers
uses: actions/cache@...
with:
path: ~/.cache/ms-playwright
key: playwright-${{ runner.os }}-${{ hashFiles('site/package-lock.json') }}
- name: Install Playwright browsers
run: npx playwright install --with-deps chromium2.
|
Summary
static-tool-page-templatebaseline: project metadata is centralized insrc/site.config.mjs, the per-module*.components.cssfiles are replaced by a generatedtheme.cssplus a rewrittenlanding.css, and docs pages move from.mdto.mdx.tsconfig.json,justfile,env.d.ts, favicon/tool-mark assets, and a stock siteREADME.md.pages.ymlworkflow into asite-check.ymlPR validator (format, spell, typecheck, build, Playwright) and asite-publish.ymldeploy workflow, both scoped tosite/**paths.AGENTIC_NAVIGATION_GUIDE.mdto match the newsite/tree.Test plan
site-checkworkflow passes on the PRsite-publishdeploys cleanly on merge tomain🤖 Generated with Claude Code