Skip to content

chore(web): add ESLint with react-hooks rules and bulk suppressions - #1155

Merged
mforce merged 3 commits into
mainfrom
chore/web-lint
Oct 10, 2026
Merged

mforce merged 3 commits into
mainfrom
chore/web-lint

Conversation

@mforce

@mforce mforce commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Adds ESLint to the SPA so hook misuse and unawaited promises fail CI instead of reaching review.

Closes #1156

What it enforces

  • react-hooks/rules-of-hooks and react-hooks/exhaustive-deps at error.
  • Type-aware @typescript-eslint/no-floating-promises.
  • @eslint/js and typescript-eslint recommended, minus prefer-const and preserve-caught-error (style) and no-unused-vars (tsc's noUnusedLocals/noUnusedParameters own it).
  • Unused eslint-disable comments are errors. Six stale react-refresh/only-export-components comments for a plugin that was never installed are deleted.

Why the linter has its own install (web/lint/)

typescript-eslint 8.71 declares typescript >=4.8.4 <6.1.0, and TypeScript 7.0.2 ships no classic compiler API (only tsc and unstable/sync). npm refuses the peer conflict in web/, and a nested override does not resolve it. So web/lint/package.json pins typescript ~6.0.3 for the linter. The app still builds with 7, and the Dockerfile's npm ci is untouched. ESLint 10 lints only files under its config's directory, so web/eslint.config.js is a one-line re-export of web/lint/eslint.config.js. Dependabot gets a /web/lint entry that ignores TypeScript majors until typescript-eslint supports 7.

Suppressions may only shrink

ESLint 10.12.0 (bulk suppressions need 9.24 or later). Pre-existing violations are in web/lint/eslint-suppressions.json, counted per file and rule: 21 no-floating-promises and 9 exhaustive-deps. None were auto-fixed.

  • A new violation fails, because it pushes the file's count above the suppressed count.
  • A stale suppression for an existing file fails too. Verified locally: raising a count above the real number, or adding an entry for a clean file, makes ESLint print "There are suppressions left that do not occur anymore" and exit 2. Only --pass-on-unpruned-suppressions skips that, and nothing passes it.
  • After a fix, npm run lint -- --prune-suppressions shrinks the file. Entries for deleted files must be deleted by hand; see Review round 1 fixes.

Wiring

  • npm run lint in web/package.json. Run npm ci --prefix lint once first.
  • CI: a Lint step in the web job, after the audits and before the tests.
  • .githooks/pre-commit lints only staged web/src .ts/.tsx files (about 4 s; the full run takes about 18 s type-aware over 293 files).
  • Rule documented in web/AGENTS.md.

Checks run locally

  • npm run lint passes. npm run typecheck passes.
  • npm audit --omit=dev is clean in web/ (the CI: dependency-vulnerability and SAST gates #146 gate) and in web/lint/. Every new package is a devDependency.
  • The commit went through .githooks/pre-commit, which ran typecheck and the staged-file lint.
  • SchemaDocsTests *ImagePin_IsOneIdenticalStringAcrossEveryTrackedFile (2 tests) pass.

Review round 1 fixes

ESLint alone does not make the suppressions file shrink-only. It accepts any count the same PR writes, and it never checks an entry whose file is gone. web/lint/check-suppressions.mjs covers both cases and runs in CI as Check lint suppressions, after Lint:

  • 64208a50: fails when an entry names a file that does not exist. Runs on every event.
  • bc0f604e: on a pull request, fails when a file/rule entry is new or its count is higher than at the PR base (pull_request.base.sha). Removals and decreases pass. The step fetches the base at depth 1 and fails if the fetch fails. If the base has no suppressions file (this PR), only the path check runs.
  • On push and workflow_dispatch only the path check runs. There is no PR base, and main takes changes only through PRs, which already ran the growth check.
  • A count that is not an integer fails the script, so an ESLint format change cannot pass silently.
  • Renaming a file that has suppressions now means fixing its violations, because the new path counts as a new entry. web/AGENTS.md says so.

--prune-suppressions does not close finding 2 on ESLint 10.12.0. With an entry for src/components/Deleted.tsx, npm run lint -- --prune-suppressions exited 0 and left the file unchanged. So the check reads the file system directly.

Mutation proof. Ran locally in web/ at bc0f604e. "Before fix" is the gate as of a42b294e (npm run lint). "After fix" is npm run lint followed by the CI step's own run: block, extracted from ci.yml with BASE_SHA=a42b294e…. Locally the step's git fetch --depth=1 was replaced by a no-op, because the base commit was already present and a depth-1 fetch makes a full clone shallow. Each row was reverted before the next.

Mutation Exit before fix (lint only) Exit after fix (lint + check) Check output
Conditional hook in StockBar + added rules-of-hooks entry 0 1 src/components/StockBar.tsx react-hooks/rules-of-hooks: count 0 -> 1
Missing effect dep in BrandSplash + count raised 1 to 2 0 1 src/components/BrandSplash.tsx react-hooks/exhaustive-deps: count 1 -> 2
Entry for a deleted path 0 1 src/components/Deleted.tsx react-hooks/exhaustive-deps: file does not exist
Entry for a deleted path, no base (the push path) 0 1 same file does not exist line
Control: SalesPage entry removed (check only) n/a 0 none
Control: all reverted 0 0 none

Proof

Scratch commits on top of a42b294e, never pushed. The branch was reset to a42b294e afterwards. Absolute paths in the output are shortened to repo-relative.

1. A hook inside an if fails. Added if (data.segments.length === 0) { useState(0); } to StockBar, a file with no suppressions:

### scratch 1: eab5def5 scratch: conditional hook
$ npm run lint

web/src/components/StockBar.tsx
  16:5  error  React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render  react-hooks/rules-of-hooks

✖ 1 problem (1 error, 0 warnings)

exit=1

2. Reverted, it passes:

### reverted: f99835a5 Revert "scratch: conditional hook"
$ npm run lint
exit=0

3. A new exhaustive-deps violation fails, even in a file that already has a suppression for that rule. BrandSplash.tsx has one suppressed exhaustive-deps violation. Added useEffect(() => { document.title = farmName; }, []);:

### scratch 2: 8f1c8c93 scratch: missing effect dependency
$ npm run lint

web/src/components/BrandSplash.tsx
  27:53  error  React Hook useMemo has an unnecessary dependency: 'bannerContentHash'. Either exclude it or remove the dependency array  react-hooks/exhaustive-deps
  35:6   error  React Hook useEffect has a missing dependency: 'farmName'. Either include it or remove the dependency array              react-hooks/exhaustive-deps

✖ 2 problems (2 errors, 0 warnings)

exit=1

Once the count goes over the suppressed number, ESLint reports every violation of that rule in the file, including the one already suppressed. Line 27 is also why nothing was auto-fixed: the bannerContentHash dependency is deliberate, because it re-reads the farm binding token when the banner changes. Removing it as the rule suggests would change behaviour.

typescript-eslint cannot load TypeScript 7, so the linter has its own
install in web/lint with TypeScript 6. Existing violations go into a
suppressions file that CI only lets shrink.
@mforce mforce added this to the Platform hardening milestone Oct 10, 2026
@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Review round 1 (Codex gpt-6-astra, head a42b294e): two findings, both P2

  1. CI does not enforce "suppressions may only shrink" (.github/workflows/ci.yml:416, web/package.json:12). CI lints against the suppression counts the PR itself supplies, and nothing compares them with the base. A PR can add a conditional hook and a matching react-hooks/rules-of-hooks: { "count": 1 } entry, or raise an existing count, and lint passes. The reviewer reproduced this through SuppressionsService.applySuppressions: one error became zero errors with zero unused suppressions. Fix: in CI, compare each file/rule count with the base revision. Fail on an added entry or a higher count. Allow removals and reductions. Compare per entry, not the total.
  2. Entries for deleted files stay green (web/package.json:12, ci.yml:419). ESLint 10.12.0 checks for unused suppressions only on files it linted. An entry for a deleted or renamed path is never checked. If that path is reused later, the old allowance hides a new violation. Reproduced: an entry for a nonexistent src/components/DeletedReviewComponent.tsx exited 0. Fix: run --prune-suppressions in CI and fail if it changes the committed file, or reject entries whose paths were not linted.

The reviewer also checked:

  • The TypeScript 6 parser loads all 287 source roots with zero diagnostics.
  • Probes for conditional hooks, missing effect deps, floating promises and unused disable comments each fail the CLI.
  • The lint step cannot be skipped by the docs-only gate.
  • Dependabot covers /web/lint.
  • Removing the react-refresh comments changes no runtime code.

No quality or over-engineering findings.

@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Review round 1 (Codex gpt-6-astra, head a42b294e): two findings, both P2

  1. CI does not enforce "suppressions may only shrink"
  2. Entries for deleted files stay green

Both are fixed with one new CI step, Check lint suppressions, which runs web/lint/check-suppressions.mjs after Lint.

  1. Fixed in bc0f604e. On a pull request it compares each file/rule entry with pull_request.base.sha. A new entry or a higher count fails; removals and decreases pass. On push and dispatch there is no base, so only the path check runs; main takes changes only through PRs.
  2. Fixed in 64208a50. Every run fails on an entry whose file does not exist. The suggested --prune-suppressions route does not work on ESLint 10.12.0: with an entry for a nonexistent src/components/Deleted.tsx, prune exited 0 and left the file unchanged.

Mutation proof, run locally at bc0f604e against base a42b294e with the step's own run: block:

Mutation Before fix After fix
Conditional hook + added rules-of-hooks entry 0 1 (count 0 -> 1)
Missing effect dep + count raised 1 to 2 0 1 (count 1 -> 2)
Entry for a deleted path 0 1 (file does not exist)
Same, no base (push path) 0 1
Control: an entry removed n/a 0
Control: all reverted 0 0

The full table and method are in the PR body under "Review round 1 fixes".

@mforce

mforce commented Oct 10, 2026

Copy link
Copy Markdown
Owner Author

Review round 2 (Codex gpt-6-astra, head bc0f604e): no findings.

Both round 1 defects are fixed. The reviewer ran check-suppressions.mjs against the committed baseline:

  • Exit 1 (fails): a new allowance, a new rule under an existing file, a raised count, a missing path (with or without a base), a ./, ../, absolute or backslash path alias, and a string count.
  • Exit 0 (passes): a decreased count, a removed entry, and the unchanged baseline.

The workflow run: block itself, executed in a shallow checkout, exits 128 when the base fetch fails.

Accepted boundaries, as documented:

  • This PR: its base has no suppression file yet, so it skips the growth check. The check starts with the next PR that touches the file.
  • Editing the workflow: removing BASE_SHA turns the growth check off silently. That, and any edit to the checker itself, is left to review.

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.

Add a web linter (ESLint + react-hooks)

1 participant