rbrooks/Quest-Board

Watch

Fork 0

fix(frontend): enforce real react-hooks lint and clear the warning backlog (#21) #180

Merged

claude-bot merged 1 commit from fix/21-real-react-hooks-lint into main

2026-07-17 01:14:14 +00:00

claude-bot commented

2026-07-17 01:08:14 +00:00

Contributor

Fixes #21. First of the v3.5.0 (Frontend Platform & PWA) milestone.

Why

eslint.config.js was deliberately dependency-free, which forced two workarounds:

A hand-maintained browser globals map — incomplete (no EventSource, FileReader, …), which is why no-undef/no-unused-vars could only be warnings.
A no-op stub plugin registered under the react-hooks name, purely so the source's // eslint-disable-next-line react-hooks/exhaustive-deps directives resolved instead of erroring on an unknown rule.

Net effect: hooks rules were never actually enforced. The tell was in the lint output itself — 3 warnings of "Unused eslint-disable directive (no problems were reported from 'react-hooks/exhaustive-deps')". The stub reported nothing, so directives suppressing real violations looked redundant.

What changed

Added @eslint/js, eslint-plugin-react-hooks, and globals as devDependencies (exact-pinned, matching the rest of the file), then:

Replaced the globals map with the globals package. This alone fixes every no-undef, and lets no-undef/no-unused-vars come from @eslint/js recommended at error — a real merge gate.
Wired up the real react-hooks plugin.
Cleared all resulting problems to 0 errors / 0 warnings.

Config decision worth reviewing

react-hooks is configured with rules-of-hooks + exhaustive-deps explicitly, rather than by spreading the plugin's recommended preset. As of eslint-plugin-react-hooks v7, recommended enables 16 rules — 13 of them React Compiler rules (static-components, use-memo, immutability, purity, set-state-in-effect, …), mostly at error. This issue asks for exactly the two rules above; adopting the compiler rule set is a separate, much larger piece of work. Rationale is documented in the config. Suggest filing that as its own issue.

The 5 real findings from enabling exhaustive-deps

Site	Finding	Fix	Behaviour change
`RecordingDashboard`	`refreshSession` re-created every render and used by the SSE effect — a naive dep-array fix would rebuild the EventSource on every render	memoise on `id`	none
`CampaignDetail`	loader effect reads `user?.id` to seed character inputs but omitted it	add dep	none — AuthGuard resolves the session first, and it's a primitive
`WikiArticle` (owners)	effect dereferenced the whole `article.entry` while listing only sub-properties	depend on the primitives it needs	none — avoids a pointless refetch when a save replaces the entry object
`WikiArticle` (save)	`handleSaveArticle` reads `isGm` for `gm_notes` but omitted it	add dep	none — primitive bool
`CampaignPlanning`	unnecessary `campaignId` dep (unused in body; `handleLoad` already re-creates on it)	drop dep	none

The RecordingDashboard one is the clearest argument for this issue: that trap was invisible while the stub was in place.

Mechanical cleanup

Unused React imports (React 19 automatic JSX runtime), dead bindings, 2 empty catch {} blocks made explicit with reasons, and 2 useless regex escapes ([^\w\s\-] → [^\w\s-], [^\)] → [^)] — both behaviour-preserving; the characters are already literal in those positions).

Verification

All run in node:20, matching CI:

npm run lint → exit 0, no output (was 31 warnings; 33 problems once the real rules were on)
npx vitest run → 69/69 passing, 11 files
npx vite build → succeeds
npm ci against the regenerated lock in a clean dir → OK (CI uses npm ci)

The build still warns "chunks larger than 500 kB" — that's #105 (route-level code splitting), next in this milestone.

🤖 Generated with Claude Code

Fixes #21. First of the v3.5.0 (Frontend Platform & PWA) milestone. ## Why `eslint.config.js` was deliberately dependency-free, which forced two workarounds: 1. A **hand-maintained browser globals map** — incomplete (no `EventSource`, `FileReader`, …), which is why `no-undef`/`no-unused-vars` could only be *warnings*. 2. A **no-op stub plugin** registered under the `react-hooks` name, purely so the source's `// eslint-disable-next-line react-hooks/exhaustive-deps` directives resolved instead of erroring on an unknown rule. Net effect: **hooks rules were never actually enforced.** The tell was in the lint output itself — 3 warnings of *"Unused eslint-disable directive (no problems were reported from 'react-hooks/exhaustive-deps')"*. The stub reported nothing, so directives suppressing real violations looked redundant. ## What changed Added `@eslint/js`, `eslint-plugin-react-hooks`, and `globals` as devDependencies (exact-pinned, matching the rest of the file), then: - Replaced the globals map with the **`globals` package**. This alone fixes *every* `no-undef`, and lets `no-undef`/`no-unused-vars` come from `@eslint/js` recommended at **error** — a real merge gate. - Wired up the **real** react-hooks plugin. - Cleared all resulting problems to **0 errors / 0 warnings**. ### Config decision worth reviewing react-hooks is configured with `rules-of-hooks` + `exhaustive-deps` **explicitly**, rather than by spreading the plugin's `recommended` preset. As of eslint-plugin-react-hooks **v7**, `recommended` enables **16 rules** — 13 of them React Compiler rules (`static-components`, `use-memo`, `immutability`, `purity`, `set-state-in-effect`, …), mostly at `error`. This issue asks for exactly the two rules above; adopting the compiler rule set is a separate, much larger piece of work. Rationale is documented in the config. **Suggest filing that as its own issue.** ## The 5 real findings from enabling exhaustive-deps | Site | Finding | Fix | Behaviour change | |---|---|---|---| | `RecordingDashboard` | `refreshSession` re-created every render and used by the SSE effect — a naive dep-array fix would **rebuild the EventSource on every render** | memoise on `id` | none | | `CampaignDetail` | loader effect reads `user?.id` to seed character inputs but omitted it | add dep | none — AuthGuard resolves the session first, and it's a primitive | | `WikiArticle` (owners) | effect dereferenced the whole `article.entry` while listing only sub-properties | depend on the primitives it needs | none — avoids a pointless refetch when a save replaces the entry object | | `WikiArticle` (save) | `handleSaveArticle` reads `isGm` for `gm_notes` but omitted it | add dep | none — primitive bool | | `CampaignPlanning` | unnecessary `campaignId` dep (unused in body; `handleLoad` already re-creates on it) | drop dep | none | The `RecordingDashboard` one is the clearest argument for this issue: that trap was invisible while the stub was in place. ## Mechanical cleanup Unused `React` imports (React 19 automatic JSX runtime), dead bindings, 2 empty `catch {}` blocks made explicit with reasons, and 2 useless regex escapes (`[^\w\s\-]` → `[^\w\s-]`, `[^\)]` → `[^)]` — both behaviour-preserving; the characters are already literal in those positions). ## Verification All run in **node:20**, matching CI: - `npm run lint` → **exit 0, no output** (was 31 warnings; 33 problems once the real rules were on) - `npx vitest run` → **69/69 passing**, 11 files - `npx vite build` → succeeds - `npm ci` against the regenerated lock in a clean dir → **OK** (CI uses `npm ci`) The build still warns "chunks larger than 500 kB" — that's #105 (route-level code splitting), next in this milestone. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

2026-07-17 01:08:14 +00:00

fix(frontend): enforce real react-hooks lint and clear the warning backlog (#21 )

CI / Backend lint (ruff) (pull_request) Successful in 59s

Details

CI / Docker image build (pull_request) Successful in 58s

Details

CI / Bot tests and audit (pull_request) Successful in 2m2s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 2m47s

Details

CI / Backend migration, tests, and audit (pull_request) Successful in 5m24s

Details

645097ad8a

eslint.config.js was deliberately dependency-free, which forced two
workarounds: a hand-maintained browser globals map, and a no-op stub plugin
registered under the `react-hooks` name so the source's
`// eslint-disable-next-line react-hooks/exhaustive-deps` directives resolved
instead of erroring on an unknown rule. Hooks rules were therefore never
actually enforced, and the incomplete globals list meant no-undef/no-unused-vars
could only be warnings.

Add @eslint/js, eslint-plugin-react-hooks, and globals as devDependencies
(exact-pinned, matching the rest of the file), then:

- Replace the globals map with the `globals` package. This alone fixes every
  no-undef (EventSource, FileReader, `global` in tests) and lets no-undef /
  no-unused-vars come from @eslint/js recommended at "error" — a real gate.
- Wire up the real react-hooks plugin.
- Clear all 33 resulting problems to 0 errors / 0 warnings.

react-hooks is configured with rules-of-hooks + exhaustive-deps explicitly
rather than by spreading the plugin's `recommended` preset: as of v7 that preset
also enables the React Compiler rule set (static-components, use-memo,
immutability, purity, set-state-in-effect, ...) mostly at "error". Those are a
separate, much larger piece of work; #21 asks for these two rules.

Turning exhaustive-deps on for the first time surfaced 5 real findings:

- RecordingDashboard: `refreshSession` was re-created every render and used by
  the SSE effect. Memoise it on `id` so the effect can depend on it without
  tearing down and rebuilding the EventSource on every render.
- CampaignDetail: the loader effect reads `user?.id` to seed the character
  inputs but omitted it. Added; AuthGuard resolves the session before this page
  renders, and the dep is a primitive, so behaviour is unchanged.
- WikiArticle: the owners effect dereferenced the whole `article.entry` object
  while listing only sub-properties. Depend on the primitives it needs so a new
  entry object (e.g. after a save) doesn't trigger a pointless refetch.
- WikiArticle: `handleSaveArticle` reads `isGm` for gm_notes but omitted it.
- CampaignPlanning: dropped an unnecessary `campaignId` dep (unused in the body;
  `handleLoad` already re-creates on it).

The remaining cleanup is mechanical: unused React imports (React 19's automatic
JSX runtime), dead bindings, two empty catch blocks made explicit, and two
useless regex escapes (both behaviour-preserving).

Verified in node:20 (matching CI): npm run lint exits 0 with no output,
69/69 vitest tests pass, vite build succeeds, and npm ci resolves the new lock.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

claude-bot merged commit 8bed9f9bd8 into main

2026-07-17 01:14:14 +00:00

claude-bot deleted branch fix/21-real-react-hooks-lint

2026-07-17 01:14:15 +00:00

claude-bot referenced this pull request from a commit

2026-07-17 01:14:15 +00:00

Merge pull request 'fix(frontend): enforce real react-hooks lint and clear the warning backlog (#21)' (#180) from fix/21-real-react-hooks-lint into main

claude-bot referenced this pull request

2026-07-17 01:32:53 +00:00

feat(frontend): shared API client with session-expiry, abort and dedup (#103) #181

claude-bot referenced this pull request

2026-07-17 02:17:44 +00:00