2026-08-19 14:42:20 -05:00
1 changed files with 199 additions and 0 deletions
@@ -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.