bermudalamb
c058b3ed2e
TypeScript's strict mode checks types and nothing else, so nothing enforced the React hook rules, the SonarJS rules, or unhandled-promise detection. Adds a flat config per workspace, a lint script in each, and a lint job in tests.yml. The rule selection is the substance of this change and is measured rather than guessed. A full-strength config reports 435 violations across 50 files, but 325 of those are the no-unsafe-* family from recommendedTypeChecked, every one downstream of pool.query() returning any rows and untyped fetch responses. Typing those boundaries is the whole of #65, so enabling the rules here would ship a linter whose output is three-quarters another issue's backlog — the reliable way to teach everyone to ignore lint output. This enables recommended plus the two type-aware rules that catch defects rather than describe type debt, which leaves 110 findings. Both configs downgrade every preset to a warning and then list the error rules explicitly at the bottom, so the CI gate is readable in one place instead of inferred from four presets' defaults. Errors are no-floating-promises, no-misused-promises, rules-of-hooks, exhaustive-deps and jsx-a11y/alt-text; everything else warns. No --max-warnings flag is needed because ESLint already exits non-zero on errors and zero on warnings. no-misused-promises runs with checksVoidReturn.attributes false, since onClick={async () => ...} is idiomatic React and safe when the handler catches its own errors — at the default it flags every antd button in the admin screens, 25 of its 28 hits, and a rule that is 89% noise gets switched off within a week. The 37 errors this surfaced were mostly not the mechanical fix they looked like. The plan assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, which was true of the one sampled when writing the design and false for most of the rest: Admin, Categories, Customers, Tags, Settings, Account and CustomerAuthContext all had no rejection handling at all, so `void` on them would have hidden real failures rather than annotated deliberate ones. Each of those loaders now catches and surfaces the failure before the call site voids it. The CustomerAuthContext one was a live bug — a rejected fetchMe left loading true forever, rendering as a permanent spinner instead of a signed-out page. Admin's load became a useCallback so its effect can name it honestly rather than suppress the dependency, Categories' drop handler was split so the function antd receives returns void as its type says, and Cart's effect now names refreshCartContext, which is a useCallback with an empty dependency list and so cannot re-run it. The only disable added is in asyncRoute, where returning a promise where Express expects void is the entire point of the wrapper and the promise cannot reject. Two of the issue's premises did not survive measurement, both recorded in the spec: exhaustive-deps flags 2 cases rather than the 10 inferred from empty dependency arrays, and the backend was already clean on the defect rules because #59 wrapped every async route. Verified: lint, build, 78 unit, 134 integration and 83 e2e all pass in both workspaces, and the CI gate was confirmed to fail by introducing a deliberate violation rather than by assuming the job is wired correctly. Closes #60
12 KiB
ESLint — Design
Issue: #60 — No ESLint anywhere Date: 2026-08-19 Status: Implemented
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.mjs), 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.
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:
'@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.
Issue #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 the files are named .mjs
Neither package.json sets "type": "module", so an eslint.config.js would be parsed as CommonJS and every import in it would fail. .mjs is the supported flat-config filename for exactly this case. Adding "type": "module" to either package instead would change how every other .js file in it is interpreted, which is a much larger change than a linter should make.
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 | .catch on six loaders, then void at the call sites |
no-misused-promises |
3 | Two restructured, one disabled 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
Corrected during implementation. The paragraph below was written after reading one loader — App.tsx's, which does catch — and generalised from it. That generalisation was wrong. Checking the other nine sites found that most of the admin loaders (Admin.tsx, Categories.tsx, Customers.tsx, Tags.tsx, Settings.tsx), plus Account.tsx and CustomerAuthContext.tsx, had no rejection handling at all. void on those would have been exactly the disguised suppression this section warns against.
So the fix was larger than planned: each of those loaders got a .catch that surfaces the failure — message.error(...) in the admin screens, and in CustomerAuthContext a .finally that clears the loading flag, since a rejected fetchMe() previously left the app on a permanent spinner rather than showing a signed-out page. Only then does void at the call site state something true.
This is the linter finding real defects on its first run, which is the outcome the issue predicted. It is recorded here because the original reasoning was sound but its premise was not checked widely enough — the per-site check is what caught it.
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 useCallbacks 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.mjs |
New |
frontend/eslint.config.mjs |
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:
npm run lintpasses in both workspaces with zero errors.- The warning count is reported, not zero, and that is expected.
npm run buildstill passes in both workspaces.- Backend unit and integration suites still pass — the
voidand disable edits touch frontend files almost exclusively, butserver.tsandasyncRoute.tsare on the backend. - Playwright e2e passes, since ≈37 edits land in rendered components.
- The
lintjob 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 theanybacklog — #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 typespool.queryresults andfetchresponses. routesAreWrapped.test.tscan retire onceno-misused-promisesruns 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.