From 8e859e58ec73744f336c1d4562a4a311ac022955 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Wed, 19 Aug 2026 13:35:33 -0500 Subject: [PATCH] docs: design for adding ESLint to both workspaces (#60) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the measurement the design rests on — 435 violations from a full-strength config, of which 325 are the no-unsafe-* family — and the decision not to enable recommendedTypeChecked, since those 325 all trace to untyped pool.query rows and fetch responses, which is the whole of #65. Also records two of the issue's premises that measurement contradicts: exhaustive-deps flags 2 rather than the 10 the issue inferred from empty dependency arrays, and the backend is already clean on the defect rules because #59 wrapped every async route. Refs #60 --- .../specs/2026-08-19-eslint-design.md | 199 ++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-19-eslint-design.md diff --git a/docs/superpowers/specs/2026-08-19-eslint-design.md b/docs/superpowers/specs/2026-08-19-eslint-design.md new file mode 100644 index 0000000..38ff094 --- /dev/null +++ b/docs/superpowers/specs/2026-08-19-eslint-design.md @@ -0,0 +1,199 @@ +# ESLint — Design + +**Issue:** [#60 — No ESLint anywhere](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/60) +**Date:** 2026-08-19 +**Status:** Approved + +## Goal + +Put a linter in front of the code that catches, at authoring time, the classes of defect this +project has already shipped and then fixed by hand. Two workspaces, one CI job, green on the day it +lands. + +The issue's own framing is the target: TypeScript's `strict: true` checks types and nothing else, so +`rules-of-hooks`, `exhaustive-deps`, `no-floating-promises` and the SonarJS rules currently run +either never or on a server after the fact. + +## What the code actually reports + +Every number below comes from running the candidate rule set against `backend/src` and +`frontend/src` before writing any config, rather than from estimating. A full-strength config — +`typescript-eslint` recommendedTypeChecked, `sonarjs`, `react-hooks` v7, `jsx-a11y` — reports **435 +violations across 50 files**. + +| Rule family | Count | Disposition | +| --- | --- | --- | +| `@typescript-eslint/no-unsafe-*` (5 rules) | 325 | **Deferred to #65** | +| `no-floating-promises` | 30 | Error — fix here | +| `no-misused-promises` | 28 | Error, but 25 disappear under `checksVoidReturn: { attributes: false }` | +| `sonarjs/prefer-read-only-props` | 16 | Warn | +| `react-hooks` compiler rules (`set-state-in-effect`, `purity`) | 9 | Warn | +| `no-explicit-any`, `no-unnecessary-type-assertion` | 11 | Warn | +| `sonarjs/no-nested-conditional`, `cognitive-complexity` | 7 | Warn | +| `react-hooks/exhaustive-deps` | 2 | Error — fix here | +| `jsx-a11y/alt-text` | 2 | Error — fix here | +| `rules-of-hooks` | 0 | Error (nothing to fix) | +| Remaining singles | 5 | Warn | + +### Two of the issue's premises do not survive measurement + +**`exhaustive-deps` flags 2, not 10.** The issue inferred ten suspects from ten empty dependency +arrays. The rule's own analysis clears eight of them — an empty array is only wrong when the effect +closes over something that changes, and most of these genuinely do not. The two real ones are +`Admin.tsx:62` (missing `load`) and `Cart.tsx:132` (missing `refreshCartContext`). The "decide case +by case rather than blanket-autofix" instinct in the issue is still right; it applies to two cases. + +**The backend is already clean on the defect rules.** #59 wrapped every async route, and it shows: +zero floating promises in `backend/src`. Its three `no-misused-promises` hits are all benign, and +each was read rather than assumed — `asyncRoute.ts:13` is the wrapper performing its entire purpose, +and the two `server.ts` scheduled jobs (`setInterval` cart sweep, `cron.schedule` reminder emails) +already `try`/`catch` internally, so no rejection escapes. + +## Decisions + +| Area | Decision | +| --- | --- | +| Config format | Flat config (`eslint.config.js`), one per workspace | +| Config location | `backend/` and `frontend/` separately — not a root config | +| Type-aware linting | On, but narrowly: `recommended`, **not** `recommendedTypeChecked` | +| `no-unsafe-*` family | Deferred to #65 | +| `no-misused-promises` | `checksVoidReturn: { attributes: false }` | +| React Compiler rules | Warn | +| CI | New `lint` job in `tests.yml` | +| Fail/warn split | Defect rules error; stylistic and advisory rules warn | + +### Why not `recommendedTypeChecked` + +It is the obvious choice and it is the wrong one here. Its `no-unsafe-*` family accounts for 325 of +the 435 findings — 75% — and every one of them traces back to two untyped boundaries: `pool.query()` +returning `any` rows on the backend, and `fetch(...).json()` on the frontend. Fixing them means +typing those boundaries, which is the entire content of +[#65](https://gitea.bermudalamb.synology.me/bermudalamb/redefined-designs/issues/65). + +Turning them on here would produce a lint run whose output is 75% another issue's backlog, which is +the reliable way to teach everyone to ignore lint output. Instead this issue enables `recommended` +plus the two type-aware rules that catch defects rather than describe type debt: + +```js +'@typescript-eslint/no-floating-promises': 'error', +'@typescript-eslint/no-misused-promises': ['error', { checksVoidReturn: { attributes: false } }], +``` + +Both need type information, so `parserOptions.projectService` is still required — the cost of +type-aware linting is paid either way. This is a choice about which findings to surface, not about +whether to run the type-aware engine. + +#65 turns the `no-unsafe-*` rules on as it types those boundaries. That is the change that makes +them green, so that is the change that should own them. + +### Why `checksVoidReturn: { attributes: false }` + +Left at its default, `no-misused-promises` flags every `onClick={async () => …}` — 25 of its 28 +hits, all antd buttons in the admin screens. Passing an async function to a React event handler is +idiomatic and safe when the function handles its own errors, which these do. typescript-eslint's own +documentation recommends this option for React codebases. Without it the rule is 89% noise and would +be switched off within a week; with it, the three remaining hits are all real signal. + +### Why the React Compiler rules only warn + +`eslint-plugin-react-hooks` v7 ships the React Compiler rule set — `set-state-in-effect`, `purity`, +`static-components` and others — alongside the two classic rules. This is React 18 with no compiler +in the build, so those rules are advising on a stricter model than the code is written against. + +They are not worthless: `purity` correctly flags `Date.now()` being called during render in +`Cart.tsx:160`. That is a genuine render-purity smell worth seeing. It is not worth failing a build +over in a codebase that never opted into the model, and fixing all nine would mean restructuring +effects that behave correctly today — behaviour change well outside what #60 asked for. + +Warn keeps the finding visible and the build honest. + +### Why per-workspace configs rather than one at the root + +CI already runs `npm install` separately in `backend` and `frontend`, and the two have genuinely +different rule needs — React and a11y rules are meaningless in `backend/src`, and each workspace has +its own `tsconfig.json` that type-aware linting must point at. A root config would need file-pattern +overrides to express a split the directory structure already expresses, and would put React plugins +in the backend's dependency tree for no reason. + +The cost is a small amount of duplication between the two config files. That is cheaper than the +alternative, and the files are short. + +## Fail/warn split + +The issue asks whether to fail CI on day one or start as warnings, and names the middle path: fail +on the rules with real defect-catching value, warn on the rest. That is what this does. + +**Error — build fails (≈37 sites to fix):** + +| Rule | Sites | Fix | +| --- | --- | --- | +| `no-floating-promises` | 30 | `void load()` | +| `no-misused-promises` | 3 | Targeted disable with a reason | +| `exhaustive-deps` | 2 | Triage individually | +| `jsx-a11y/alt-text` | 2 | Add `alt` | +| `rules-of-hooks` | 0 | — | + +**Warn — visible, does not block (≈73 sites):** SonarJS complexity and style, remaining jsx-a11y, +React Compiler rules, `no-explicit-any`. + +No `--max-warnings` flag is needed: ESLint exits non-zero on errors and zero on warnings, so the +split falls out of rule severity alone. + +### On `void load()` being a real fix rather than a disguised suppression + +Thirty floating promises sounds like thirty bugs; it is not, and the distinction matters because +`void` can be either an honest annotation or a way to silence a rule without thinking. + +These are fire-and-forget calls to `load()`-style `useCallback`s in effects and event handlers. The +callbacks were read before choosing this fix: each one sets its own error state — `App.tsx`'s `load` +calls `setFailed(true)` — so a rejection is already handled inside, and nothing is being swallowed. +`void` states "deliberately not awaited," which is exactly true here. + +Had any of them lacked internal handling, the fix for that one would be a `.catch`, not a `void`. +The implementation plan checks each site rather than applying `void` mechanically. + +## Files + +| File | Change | +| --- | --- | +| `backend/eslint.config.js` | New | +| `frontend/eslint.config.js` | New | +| `backend/package.json` | `lint` script, devDependencies | +| `frontend/package.json` | `lint` script, devDependencies | +| `.gitea/workflows/tests.yml` | New `lint` job | +| `frontend/src/**` (10 files) | ≈37 violation fixes | +| `backend/src/asyncRoute.ts`, `server.ts` | 3 targeted disables | +| `.claude/project-context.md` | Record the linter and the deferral to #65 | + +## Testing + +The linter is its own test: `npm run lint` in each workspace must exit 0. Success criteria: + +1. `npm run lint` passes in both workspaces with zero errors. +2. The warning count is reported, not zero, and that is expected. +3. `npm run build` still passes in both workspaces. +4. Backend unit and integration suites still pass — the `void` and disable edits touch frontend + files almost exclusively, but `server.ts` and `asyncRoute.ts` are on the backend. +5. Playwright e2e passes, since ≈37 edits land in rendered components. +6. The `lint` job runs in CI and fails the build when a rule in the error set is violated. + +Point 6 is verified by deliberately introducing a violation locally and confirming a non-zero exit, +not by assuming the job's configuration is correct. + +## Out of scope + +- **The `no-unsafe-*` family and the `any` backlog** — #65. +- **Prettier or any formatting tool.** The issue asks for defect detection. Formatting is a separate + argument with separate churn. +- **Linting `tests/` and Playwright specs.** Worth doing; a second step once the source is green, so + that this change's error count stays the measured one. +- **Replacing SonarQube.** The CI scan stays. This puts the same rule families in the editor, where + they are cheaper to act on. + +## Follow-ups this creates + +- **#65 owns turning on `no-unsafe-*`** as it types `pool.query` results and `fetch` responses. +- **`routesAreWrapped.test.ts` can retire** once `no-misused-promises` runs on the backend in CI. + The test predates the linter and covers the same ground by scanning source; it is more targeted + and its failure message is better, so this is a judgement call for whoever next touches either. + Recorded here so the duplication is deliberate rather than forgotten.