debt(lint): lint .playwright and .storybook, and fix what surfaced - #992
Merged
Conversation
Contributor
|
code-review-audit skipped: no files in the frontend auditor's scope, but this diff dispatches code-audit-github-workflows, which CI cannot run. GAIA-Audit is pending, not green: run the dispatched member(s) locally to clear the merge gate |
Both directories were bare entries in the lint preset's default ignore list. A
config object carrying only `ignores` is a global ignore in flat config, and a
global ignore beats any later block's `files`, so the two blocks written to lint
those directories were shadowed by the defaults they ship alongside: the
`playwright` block never matched a file, and the `storybook` block lost its
`.storybook/main.*` half. Every e2e spec, Playwright helper, and Storybook config
file went unchecked while `pnpm lint` reported a clean tree.
The root cause is fixed upstream in @gaia-react/lint 1.11.0, which drops both
entries and adds a suite that asks ESLint which paths survive the defaults. This
bumps to it and fixes the 78 findings it surfaces here, 59 by `--fix` and the
rest by hand.
Two of those were coverage, not style. `playwright/expect-expect` counts the
project's `expect*()` a11y helpers as assertions, which retires two hand-written
disable comments that never suppressed anything, and
`playwright/no-conditional-in-test` caught a hand-rolled `if (...) { throw }` in
the landmark spec, now an assertion over a projection of the violating rules.
`playwright/no-skipped-test` gains `allowConditional` upstream so the template's
own runtime-conditional skip stays legal.
The react-perf harness files take a file-level `no-underscore-dangle` disable:
`window.__renders` / `__bippyMeta` / `__PERF_NO_STRICT` are the wire contract
between the injected harness and the capture helper, not a naming choice, and
`app/entry.client.tsx` already carries the same disable for the same reason.
Closes #980
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01728LepSoRCrVZ7L2HkEoHa
`.lintstagedrc.json` gave those two directories a prettier-only entry while `app/**` got eslint alongside it. That split existed because eslint ignored them: running it on an ignored file only warns. With the ignore gone, the prettier-only entry becomes the same silent non-coverage one layer down, where a lint error in an e2e spec passes the pre-commit gate and surfaces in CI. Give the entry the same two commands `app/**` already runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01728LepSoRCrVZ7L2HkEoHa
…changes
Two follow-ons the audit raised on the previous commit.
The `{.storybook,.playwright}` lint-staged entry pointed eslint at the same
cache file as `app/**`. lint-staged runs glob task-chains concurrently, so a
commit staging both trees launches two eslint processes that each load the cache
at start and rewrite the whole file at exit, discarding one another's entries.
The failure mode is cache thrash rather than a false clean, since a missing
entry only means re-lint. Give the tooling entry its own cache file.
`tests.yml`'s path allowlist covered `.playwright/` but not `.storybook/`, so a
`.storybook`-only change reported the required check green without running.
That was defensible while the directory was unlinted and holds no tests; it is
not now that typecheck and lint both reach it, and Chromatic's Vite build strips
types rather than checking them.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01728LepSoRCrVZ7L2HkEoHa
stevensacks
force-pushed
the
debt/980-lint-playwright-storybook-dirs
branch
from
July 21, 2026 16:51
3374056 to
57e32e4
Compare
The allowlist's new comment states the rule that a path typecheck and lint reach belongs here even when it carries no tests, but the pattern only named four config files by tool. `react-router.config.ts`, `doctor.config.ts`, `knip.config.ts`, `prettier.config.mjs`, and `stylelint.config.mjs` are all in the tsconfig program and all linted, so a PR touching only one of them set `code=false` and reported the required check green without running. The react-router case is the sharpest: that file feeds the typegen step `pnpm typecheck` runs before `tsc`, so it can invalidate generated route types across `app/**`, and the e2e suite exercises the SSR-rendered pages it configures. Name every root config in the one existing alternative rather than appending a second, and say in the comment why the list is not scoped to the tools this job happens to run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01728LepSoRCrVZ7L2HkEoHa
…ationale `pnpm-workspace.yaml` and `.npmrc` were missing, and the lockfile alternative does not cover them transitively: pnpm 11 reads `allowBuilds`, `publicHoistPattern`, `minimumReleaseAge`, and `trustPolicy` from the workspace file, while the lockfile's own `settings:` block carries neither. A change to either can break `pnpm install --frozen-lockfile` for the next unrelated PR while the workspace-config PR itself merges on a green required check. Not hypothetical: #976 changed only `pnpm-workspace.yaml` and would have skipped the suite. The comment also claimed every root config is in the tsconfig program. That is true of the `.ts` ones only; `include` lists no `.mjs` glob and `allowJs` is off, so `eslint.config.mjs`, `prettier.config.mjs`, and `stylelint.config.mjs` are covered by lint alone. Attribute each half to the tool that actually runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01728LepSoRCrVZ7L2HkEoHa
GAIA-Audit: 1.6.1 4d242efea3fa90cb4ec9ecf7a76e58949f7150c4e5ef8f9209ac787861d4be8e bc0190e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #980
Problem
.playwrightand.storybookwere bare entries in the lint preset's default ignore list. A config object carrying onlyignoresis a global ignore in ESLint flat config, and a global ignore beats any later block'sfiles, so the two blocks written to lint those directories were shadowed by the defaults they ship alongside:playwrightblock scopes itself to.playwright/**/*.ts?(x)and never matched a file, taking its hand-tunedexpect-expectoptions with itstorybookblock kept its*.stories.*rules (those files live elsewhere) but lost its.storybook/main.*halfEvery e2e spec, Playwright helper, and Storybook config file went unchecked while
pnpm lintreported a clean tree and CI stayed green.Fix
Root cause is fixed upstream in
@gaia-react/lint1.11.0, which drops both entries and adds anignoressuite that asks ESLint itself which paths survive the defaults, so a future directory-level entry that shadows a scoped block fails a test instead of shipping. This bumps to it and fixes the 78 findings that surfaced here: 59 byeslint --fix, 19 by hand.Two of those findings were coverage, not style:
playwright/expect-expectcounts the project'sexpect*()a11y helpers as assertions, retiring two hand-written disable comments that never suppressed anythingplaywright/no-conditional-in-testcaught a hand-rolledif (relevant.length > 0) { attach; throw }in the landmark spec, now an assertion over a projection of the violating rulesUpstream also gained
playwright/no-skipped-test'sallowConditional, so the template's own runtime-conditional skip (a language switcher that single-language projects do not render) stays legal while a baretest.skip()stays flagged.The react-perf harness files take a file-level
no-underscore-dangledisable:window.__renders/__bippyMeta/__PERF_NO_STRICTare the wire contract between the injected harness and the capture helper, not a naming choice, andapp/entry.client.tsxalready carries the same disable for the same reason.A second commit closes the same gap one layer down:
.lintstagedrc.jsongave those directories a prettier-only entry (because running eslint on an ignored file only warns), so a lint error in an e2e spec would have passed the pre-commit gate and surfaced in CI.Verification
/and/privacyboth 200The e2e run is the load-bearing one:
capture.tsandharness-entry.tscarry real edits (apathdefault import,outputFiles.at(0), theseqcounter extraction, handler methods to arrow properties), andreact-perf-smoke.spec.tsexercises all of it.🤖 Generated with Claude Code