Add Playwright production-build end-to-end coverage - #29
Conversation
|
Hi @XonkelX, thanks, had a quick look and it looks good so far. The actions SHA versions should be updated to the latest release versions though. Can you please look into that? Did you use Codex for this? I might do a Claude Code + Human review on this after I finish work. |
|
Yes! I used Codex as a coding assistant for the implementation and testing, and I reviewed the changes before submitting. The branch name probably gave it away. I'll update the three pinned actions to their latest release SHAs. Thanks for taking a look. |
|
Updated the three pins in c26f6cf: actions/checkout v7.0.1, pnpm/action-setup v6.0.10, and actions/setup-node v7.0.0. I also reran lint and the Playwright suite; all 18 tests pass. I kept the update scoped to the new E2E workflow. |
Cool cool, will take a look in an hour or two. If you use the same model family to do reviews, the biases can carry through each step. Like a mistake in the generation might not be flagged by the model since it would make the same mistake. I use claude code mainly and use codex to review. Will use Claude instead for this PR. |
There was a problem hiding this comment.
Playwright end-to-end coverage
Worth landing. Right targets — the menu overlay's focus management and the contact form
are the only real behaviour on this site — and role-based locators that won't break on
styling changes.
Five comments, all small. The two worth real attention are the focus-containment loop and
the contact-form assertion: both currently pass for reasons unrelated to what they check.
Before merge: the suite has never actually run in CI — both End-to-end tests runs are
action_required at 0s, held by the approval gate. It's confirmed passing locally against
the production build, so approving one runner pass is worth more than this review.
Optional later: upload the traces the config already generates on failure, and give the 404
test a real not-found.tsx to assert against.
Review by Claude Code (Opus 5); Codex did not see these changes.
…30) Came out of running `/pr-review` on #29, which produced eight inline comments and a several-hundred-word summary that mostly restated them. The skill had no length guidance anywhere, and no way to tell a defect from a preference — so everything arrived at the same weight, and trimming meant dropping real findings rather than ranking them. Three changes to `SKILL.md`: **Severity labels.** Every inline comment now opens with `**major**`, `**minor**` or `**nit**` before the claim, each defined by what the author is expected to do about it. Ranking is what earns the right to raise everything: a nit labelled as a nit costs two seconds, while the same nit sitting unlabelled beside a real defect makes the author weigh both equally. Includes a warning against inflating a nit to get it fixed, which is how the labels stop being believed. **Length, on the summary only.** Inline comments are explicitly unbounded — a finding needs enough words to be verifiable, or the author has to re-derive it. The ~150-word budget applies to the summary, reframed as a landing page rather than a recap, with an ordered list of what to cut first that starts with anything the inline comments already say. **Attribution is now required, and leads the summary.** It was previously described as "fine and useful". Position matters: knowing a model produced a finding changes how it is weighed, so it has to arrive before the findings, not as a footnote. The section also spells out the contrast with commit messages — which carry no tool attribution at all per `AGENTS.md` — because the two rules point opposite ways and are easy to conflate. Docs only; no code or workflow changes.
c26f6cf to
483e2cb
Compare
|
Approving @XonkelX. Thank you for your contribution. I look forward to getting more. |
2 participants
Closes #6
What changed
/,/contact, and the generated 404 route@axe-core/playwright; existing palette contrast debt remains outside this harness changepnpm test:e2eWhy
Most routes are Server Components, so they need browser-level coverage. This gives the project a production-build smoke gate without overlapping the general build/lint workflow in #26.
Verification
pnpm lintpnpm buildpnpm test:e2e— 18 passed across desktop and mobile Chromiumpnpm install --frozen-lockfile --lockfile-onlygit diff --check