No ESLint anywhere, so the React and SonarJS rules that would catch these problems never run #60

Closed
opened 2026-08-19 11:04:33 -05:00 by bermudalamb · 3 comments
Owner

Found during a Microsoft/React/SonarQube best-practices review.

Problem

There is no ESLint configuration in this repo — not in the root, not in backend/, not in frontend/. frontend/package.json has no lint script; its scripts are dev, build, test:e2e.

TypeScript's strict: true is set on both sides, which is good, but the compiler only checks types. Everything a linter would catch is currently unchecked.

What this specifically leaves unenforced

  • react-hooks/rules-of-hooks and react-hooks/exhaustive-deps. This is not theoretical here: PR #11 fixed "cart badge not updating after account creation due to stale closure", which is exactly what exhaustive-deps exists to prevent, and the project's own verification checklist lists "forgetting to add new dependencies to useEffect dependency arrays" as a known recurring failure. There are 10 effects with empty dependency arrays in frontend/src that nothing verifies.
  • eslint-plugin-sonarjs. SonarQube's JS/TS rules — cognitive complexity, duplicated blocks, identical sub-expressions, collapsible conditionals — run in CI after the fact, on a server, rather than in the editor while the code is being written.
  • @typescript-eslint/no-floating-promises. The Playwright guidance in this project already warns that a missing await silently passes; this rule is what catches it. It would also have flagged the unwrapped async route handlers in the sibling issue.
  • jsx-a11y rules for the storefront.

Suggested approach

Add a flat ESLint config per workspace with @typescript-eslint, react-hooks, and sonarjs, plus a lint script, and wire it into tests.yml as its own job.

Worth deciding before starting:

  • Whether to fail CI on it immediately or start as a warning. ~6,400 lines have never been linted, so the first run will produce a substantial list. Failing the build on day one blocks unrelated work; warning-only risks it being ignored permanently. A common middle path is failing only on the rules with real defect-catching value (rules-of-hooks, no-floating-promises, exhaustive-deps) and warning on stylistic ones.
  • Whether exhaustive-deps gets fixed or suppressed case by case. Some of the 10 empty dependency arrays are deliberate "run once on mount" effects and are correct; the rule cannot tell those apart, so each needs a decision rather than a blanket autofix.

Severity

Medium, but it is the multiplier behind several other findings — most of them would have been caught at authoring time rather than by a manual review.

Found during a Microsoft/React/SonarQube best-practices review. ## Problem There is no ESLint configuration in this repo — not in the root, not in `backend/`, not in `frontend/`. `frontend/package.json` has no `lint` script; its `scripts` are `dev`, `build`, `test:e2e`. TypeScript's `strict: true` is set on both sides, which is good, but the compiler only checks types. Everything a linter would catch is currently unchecked. ## What this specifically leaves unenforced - **`react-hooks/rules-of-hooks`** and **`react-hooks/exhaustive-deps`**. This is not theoretical here: PR #11 fixed "cart badge not updating after account creation due to stale closure", which is exactly what `exhaustive-deps` exists to prevent, and the project's own verification checklist lists "forgetting to add new dependencies to useEffect dependency arrays" as a known recurring failure. There are 10 effects with empty dependency arrays in `frontend/src` that nothing verifies. - **`eslint-plugin-sonarjs`**. SonarQube's JS/TS rules — cognitive complexity, duplicated blocks, identical sub-expressions, collapsible conditionals — run in CI *after* the fact, on a server, rather than in the editor while the code is being written. - **`@typescript-eslint/no-floating-promises`**. The Playwright guidance in this project already warns that a missing `await` silently passes; this rule is what catches it. It would also have flagged the unwrapped async route handlers in the sibling issue. - **`jsx-a11y`** rules for the storefront. ## Suggested approach Add a flat ESLint config per workspace with `@typescript-eslint`, `react-hooks`, and `sonarjs`, plus a `lint` script, and wire it into `tests.yml` as its own job. Worth deciding before starting: - **Whether to fail CI on it immediately or start as a warning.** ~6,400 lines have never been linted, so the first run will produce a substantial list. Failing the build on day one blocks unrelated work; warning-only risks it being ignored permanently. A common middle path is failing only on the rules with real defect-catching value (`rules-of-hooks`, `no-floating-promises`, `exhaustive-deps`) and warning on stylistic ones. - **Whether `exhaustive-deps` gets fixed or suppressed case by case.** Some of the 10 empty dependency arrays are deliberate "run once on mount" effects and are correct; the rule cannot tell those apart, so each needs a decision rather than a blanket autofix. ## Severity Medium, but it is the multiplier behind several other findings — most of them would have been caught at authoring time rather than by a manual review.
bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:37:31 -05:00
bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-19 13:20:22 -05:00
Author
Owner

Design settled and committed to docs/superpowers/specs/2026-08-19-eslint-design.md on feature/60-eslint. Decisions and the rejected alternatives below.

Everything here is measured, not estimated: a full-strength candidate config was run against backend/src and frontend/src before any config was written. It reports 435 violations across 50 files.

Two of this issue's premises did not survive measurement

exhaustive-deps flags 2, not 10. The issue inferred ten suspects from ten empty dependency arrays. The rule clears eight of them — an empty array is only wrong when the effect closes over something that changes. The two real ones are Admin.tsx:62 (missing load) and Cart.tsx:132 (missing refreshCartContext). The issue's instinct that these need case-by-case decisions rather than a blanket autofix is still right; it applies to two cases, so it is a small job rather than the open-ended one the issue implies.

The backend is already clean on the defect rules, because #59 wrapped every async route. Zero floating promises in backend/src. Its three no-misused-promises hits are all benign, each read rather than assumed: asyncRoute.ts:13 is the wrapper doing its entire job, and the two server.ts scheduled jobs already try/catch internally.

Question: which rule set?

Chosen — recommended plus two type-aware rules, not recommendedTypeChecked. The no-unsafe-* family is 325 of the 435 findings, 75% of the total, and every one traces to two untyped boundaries: pool.query() returning any rows, and fetch(...).json(). Typing those is the entire content of #65. Enabling them here produces a lint run that is three-quarters another issue's backlog, which is the reliable way to teach everyone to ignore lint output. no-floating-promises and no-misused-promises are enabled explicitly — they need the same type-aware engine, so this is a choice about which findings to surface, not about whether to pay for type-aware linting. #65 turns the rest on as it does the work that makes them green.

Rejected — enable no-unsafe-* as warnings now, to give #65 a live worklist. 325 permanent warnings would drown the ~73 warnings that this issue actually wants people to read.

Rejected — enable and fix them here. That is #65, done under #60's name.

Question: fail CI on day one?

Chosen — the middle path this issue names. Defect rules error, stylistic and advisory rules warn. ESLint exits non-zero on errors and zero on warnings, so the split needs no --max-warnings juggling.

Error, ≈37 sites: no-floating-promises 30, no-misused-promises 3, exhaustive-deps 2, jsx-a11y/alt-text 2, rules-of-hooks 0. Warn, ≈73 sites: SonarJS complexity and style, remaining jsx-a11y, React Compiler rules, no-explicit-any.

Rejected — fail on everything, which means fixing all 435 here, i.e. doing #65 first. Rejected — warn on everything, which is the outcome this issue explicitly warns against.

Question: no-misused-promises at its default?

No — checksVoidReturn: { attributes: false }. At the default it 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 docs recommend this option for React. Without it the rule is 89% noise and gets switched off within a week. With it, the remaining 3 are all signal.

Question: the React Compiler rules?

Warn. eslint-plugin-react-hooks v7 ships the compiler rule set alongside the two classic rules. This is React 18 with no compiler in the build, so they advise against a stricter model than the code was written for. They are not worthless — purity correctly catches Date.now() called during render in Cart.tsx:160 — but failing a build on nine findings would mean restructuring effects that work correctly today. Rejected turning them off entirely, which would lose that Cart.tsx finding.

Question: one root config or one per workspace?

Per workspace. CI already installs each separately, each has its own tsconfig.json that type-aware linting must point at, and React/a11y rules are meaningless in backend/src. A root config would need file-pattern overrides to express a split the directory layout already expresses, and would put React plugins in the backend's dependency tree. Costs a little duplication across two short files.

Note on the 30 void fixes

Thirty floating promises is not thirty bugs, and void can be either an honest annotation or a way to silence a rule without thinking. These are fire-and-forget load() calls in effects and handlers; the callbacks were read first, and each sets its own error state, so nothing is being swallowed. void states "deliberately not awaited", which is true here. Any site lacking internal handling gets a .catch instead — checked per site rather than applied mechanically.

Also recorded

Once no-misused-promises runs on the backend in CI it covers the same ground as routesAreWrapped.test.ts from #59, which scans source for the same defect. The test is more targeted and has a better failure message, so keeping both is a judgement call rather than an oversight — noted in the spec so it is not rediscovered as accidental duplication.

Out of scope: Prettier, linting tests/ and the Playwright specs, and replacing the SonarQube scan.

Design settled and committed to `docs/superpowers/specs/2026-08-19-eslint-design.md` on `feature/60-eslint`. Decisions and the rejected alternatives below. Everything here is measured, not estimated: a full-strength candidate config was run against `backend/src` and `frontend/src` before any config was written. It reports **435 violations across 50 files**. ## Two of this issue's premises did not survive measurement **`exhaustive-deps` flags 2, not 10.** The issue inferred ten suspects from ten empty dependency arrays. The rule clears eight of them — an empty array is only wrong when the effect closes over something that changes. The two real ones are `Admin.tsx:62` (missing `load`) and `Cart.tsx:132` (missing `refreshCartContext`). The issue's instinct that these need case-by-case decisions rather than a blanket autofix is still right; it applies to two cases, so it is a small job rather than the open-ended one the issue implies. **The backend is already clean on the defect rules**, because #59 wrapped every async route. Zero floating promises in `backend/src`. Its three `no-misused-promises` hits are all benign, each read rather than assumed: `asyncRoute.ts:13` is the wrapper doing its entire job, and the two `server.ts` scheduled jobs already `try`/`catch` internally. ## Question: which rule set? **Chosen — `recommended` plus two type-aware rules, not `recommendedTypeChecked`.** The `no-unsafe-*` family is 325 of the 435 findings, 75% of the total, and every one traces to two untyped boundaries: `pool.query()` returning `any` rows, and `fetch(...).json()`. Typing those is the entire content of #65. Enabling them here produces a lint run that is three-quarters another issue's backlog, which is the reliable way to teach everyone to ignore lint output. `no-floating-promises` and `no-misused-promises` are enabled explicitly — they need the same type-aware engine, so this is a choice about which findings to surface, not about whether to pay for type-aware linting. #65 turns the rest on as it does the work that makes them green. **Rejected — enable `no-unsafe-*` as warnings now**, to give #65 a live worklist. 325 permanent warnings would drown the ~73 warnings that this issue actually wants people to read. **Rejected — enable and fix them here.** That is #65, done under #60's name. ## Question: fail CI on day one? **Chosen — the middle path this issue names.** Defect rules error, stylistic and advisory rules warn. ESLint exits non-zero on errors and zero on warnings, so the split needs no `--max-warnings` juggling. Error, ≈37 sites: `no-floating-promises` 30, `no-misused-promises` 3, `exhaustive-deps` 2, `jsx-a11y/alt-text` 2, `rules-of-hooks` 0. Warn, ≈73 sites: SonarJS complexity and style, remaining jsx-a11y, React Compiler rules, `no-explicit-any`. **Rejected — fail on everything**, which means fixing all 435 here, i.e. doing #65 first. **Rejected — warn on everything**, which is the outcome this issue explicitly warns against. ## Question: `no-misused-promises` at its default? **No — `checksVoidReturn: { attributes: false }`.** At the default it 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 docs recommend this option for React. Without it the rule is 89% noise and gets switched off within a week. With it, the remaining 3 are all signal. ## Question: the React Compiler rules? **Warn.** `eslint-plugin-react-hooks` v7 ships the compiler rule set alongside the two classic rules. This is React 18 with no compiler in the build, so they advise against a stricter model than the code was written for. They are not worthless — `purity` correctly catches `Date.now()` called during render in `Cart.tsx:160` — but failing a build on nine findings would mean restructuring effects that work correctly today. Rejected turning them off entirely, which would lose that `Cart.tsx` finding. ## Question: one root config or one per workspace? **Per workspace.** CI already installs each separately, each has its own `tsconfig.json` that type-aware linting must point at, and React/a11y rules are meaningless in `backend/src`. A root config would need file-pattern overrides to express a split the directory layout already expresses, and would put React plugins in the backend's dependency tree. Costs a little duplication across two short files. ## Note on the 30 `void` fixes Thirty floating promises is not thirty bugs, and `void` can be either an honest annotation or a way to silence a rule without thinking. These are fire-and-forget `load()` calls in effects and handlers; the callbacks were read first, and each sets its own error state, so nothing is being swallowed. `void` states "deliberately not awaited", which is true here. Any site lacking internal handling gets a `.catch` instead — checked per site rather than applied mechanically. ## Also recorded Once `no-misused-promises` runs on the backend in CI it covers the same ground as `routesAreWrapped.test.ts` from #59, which scans source for the same defect. The test is more targeted and has a better failure message, so keeping both is a judgement call rather than an oversight — noted in the spec so it is not rediscovered as accidental duplication. Out of scope: Prettier, linting `tests/` and the Playwright specs, and replacing the SonarQube scan.
Author
Owner

Implemented on feature/60-eslint, three commits.

What landed

Flat config per workspace (backend/eslint.config.mjs, frontend/eslint.config.mjs.mjs because neither package sets "type": "module"), a lint script in each, and a lint job in tests.yml. 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 rather than inferred from four presets' defaults.

Result: backend 0 errors / 10 warnings, frontend 0 errors / 34 warnings, both exiting 0.

The 37 errors were not the mechanical fix they looked like

This is the part worth reading. The design assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, so void would be an honest annotation. That was true of the one loader read while writing the design, and false for most of the rest. Admin.tsx, Categories.tsx, Customers.tsx, Tags.tsx, Settings.tsx, Account.tsx and CustomerAuthContext.tsx had no rejection handling at all — void on those would have been precisely the disguised suppression the design warned against.

Each of those loaders now catches and surfaces the failure before the call site voids it. Concretely, the linter's first run found these:

  • CustomerAuthContext was a live bug. A rejected fetchMe() left loading true forever, so a failed session check rendered as a permanent spinner rather than a signed-out page. Now caught, with .finally clearing the flag.
  • Every admin screen failed silently. A failed refetch after a save left the table showing its previous contents, which is indistinguishable from a save that changed nothing. Each loader now reports via message.error.
  • Settings.tsx and Customers.tsx could strand a spinner the same way, both fixed.

The remaining errors:

  • Admin.tsx's load became a useCallback so its effect can name the dependency honestly instead of suppressing it — a function rebuilt each render would otherwise loop in the dependency array.
  • Categories.tsx's drop handler was split so the function antd receives returns void as its type says, rather than handing Tree a promise it never awaits.
  • Cart.tsx's effect now names refreshCartContext, which is a useCallback with an empty dependency list and therefore cannot re-run the effect or re-render the PayPal buttons.
  • Two <img> elements got alt="" — decorative, since the item name sits in the adjacent column and duplicate announcement would be worse.
  • One disable, in asyncRoute.ts, where returning a promise where Express expects void is the entire purpose of the wrapper and .catch(next) means it cannot reject.

Worth noting a trap: eslint-disable-next-line has to be the last comment line before the code. A multi-line explanation above it silently breaks the directive, and ESLint then reports both the original error and an "unused disable directive" warning.

Verification

  • npm run lint — 0 errors both workspaces
  • npm run build — clean both workspaces
  • 78 backend unit, 134 backend integration, 83 Playwright e2e — all pass

The CI gate was confirmed to fail rather than assumed to: a file with a deliberate floating promise was added, npm run lint exited 1 and reported it, and removing the file returned it to 0.

Corrections to the design doc

The spec has been updated in place, since it is the durable record: the void section now records that its premise was checked on one loader and generalised wrongly, and what the per-site check turned up instead. Status is now Implemented.

Still open, deliberately

recommendedTypeChecked and its no-unsafe-* family (~325 findings) stay off until #65 types pool.query results and fetch responses — that is the change that makes them green, so it should own them. Prettier, linting tests/ and the Playwright specs, and replacing the SonarQube scan all remain out of scope.

Not pushed — left local per the usual arrangement.

Implemented on `feature/60-eslint`, three commits. ## What landed Flat config per workspace (`backend/eslint.config.mjs`, `frontend/eslint.config.mjs` — `.mjs` because neither package sets `"type": "module"`), a `lint` script in each, and a `lint` job in `tests.yml`. 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 rather than inferred from four presets' defaults. Result: **backend 0 errors / 10 warnings, frontend 0 errors / 34 warnings**, both exiting 0. ## The 37 errors were not the mechanical fix they looked like This is the part worth reading. The design assumed the 30 floating promises were fire-and-forget loaders that already handled their own failures, so `void` would be an honest annotation. That was true of the one loader read while writing the design, and **false for most of the rest**. `Admin.tsx`, `Categories.tsx`, `Customers.tsx`, `Tags.tsx`, `Settings.tsx`, `Account.tsx` and `CustomerAuthContext.tsx` had no rejection handling at all — `void` on those would have been precisely the disguised suppression the design warned against. Each of those loaders now catches and surfaces the failure before the call site voids it. Concretely, the linter's first run found these: - **`CustomerAuthContext` was a live bug.** A rejected `fetchMe()` left `loading` true forever, so a failed session check rendered as a permanent spinner rather than a signed-out page. Now caught, with `.finally` clearing the flag. - **Every admin screen failed silently.** A failed refetch after a save left the table showing its previous contents, which is indistinguishable from a save that changed nothing. Each loader now reports via `message.error`. - **`Settings.tsx` and `Customers.tsx` could strand a spinner** the same way, both fixed. The remaining errors: - `Admin.tsx`'s `load` became a `useCallback` so its effect can name the dependency honestly instead of suppressing it — a function rebuilt each render would otherwise loop in the dependency array. - `Categories.tsx`'s drop handler was split so the function antd receives returns void as its type says, rather than handing `Tree` a promise it never awaits. - `Cart.tsx`'s effect now names `refreshCartContext`, which is a `useCallback` with an empty dependency list and therefore cannot re-run the effect or re-render the PayPal buttons. - Two `<img>` elements got `alt=""` — decorative, since the item name sits in the adjacent column and duplicate announcement would be worse. - **One disable, in `asyncRoute.ts`**, where returning a promise where Express expects void is the entire purpose of the wrapper and `.catch(next)` means it cannot reject. Worth noting a trap: `eslint-disable-next-line` has to be the last comment line before the code. A multi-line explanation above it silently breaks the directive, and ESLint then reports both the original error and an "unused disable directive" warning. ## Verification - `npm run lint` — 0 errors both workspaces - `npm run build` — clean both workspaces - 78 backend unit, 134 backend integration, 83 Playwright e2e — all pass The CI gate was **confirmed to fail rather than assumed to**: a file with a deliberate floating promise was added, `npm run lint` exited 1 and reported it, and removing the file returned it to 0. ## Corrections to the design doc The spec has been updated in place, since it is the durable record: the `void` section now records that its premise was checked on one loader and generalised wrongly, and what the per-site check turned up instead. Status is now Implemented. ## Still open, deliberately `recommendedTypeChecked` and its `no-unsafe-*` family (~325 findings) stay off until #65 types `pool.query` results and `fetch` responses — that is the change that makes them green, so it should own them. Prettier, linting `tests/` and the Playwright specs, and replacing the SonarQube scan all remain out of scope. Not pushed — left local per the usual arrangement.
bermudalamb added reference feature/60-eslint 2026-08-19 14:41:48 -05:00
bermudalamb moved this to Review in Code Quality and Hardening on 2026-08-19 14:52:45 -05:00
bermudalamb removed reference feature/60-eslint 2026-08-20 08:25:12 -05:00
Author
Owner

Closing — implemented and merged 2026-08-19

Landed in c058b3e "feat(ci): add ESLint to both workspaces with a defect-only CI gate (#60)", authored 2026-08-19 14:08 CDT, merged to main the same day as PR #68 (29e97f6) from feature/60-eslint.

Confirmed present on main today rather than taken from the implementation comment:

  • backend/eslint.config.mjs and frontend/eslint.config.mjs both exist
  • A dedicated lint job in .gitea/workflows/tests.yml, linting each workspace separately
  • npm run lint exits 0 in both workspaces

The defect-only gate this issue asked for is the shape that shipped: defect rules error, stylistic and advisory rules warn.

It has since proved itself twice

Worth recording, since the issue argued ESLint would be "the multiplier behind several other findings" and that turned out to be measurable:

  • The first run found a live bugCustomerAuthContext left loading true forever on a rejected fetchMe(), rendering a permanent spinner instead of a signed-out page — plus silent failure in every admin screen's refetch.
  • The React and SonarJS rules it turned on are what surfaced the entire #81 backlog, including four genuine React defects (a missing key, and three context providers re-rendering the whole storefront). #81 is now closed with those fixed, and the same rules confirmed the fix: 35 warnings down to 31.

Deliberately not in scope, and still not

recommendedTypeChecked and the no-unsafe-* family — roughly 325 findings — remain off. That was a decision recorded here, not an omission: those findings trace to pool.query() and fetch().json() returning any, and typing those is the whole content of #65, which stays open and owns them.

## Closing — implemented and merged 2026-08-19 Landed in `c058b3e` *"feat(ci): add ESLint to both workspaces with a defect-only CI gate (#60)"*, authored 2026-08-19 14:08 CDT, merged to `main` the same day as PR #68 (`29e97f6`) from `feature/60-eslint`. Confirmed present on `main` today rather than taken from the implementation comment: - `backend/eslint.config.mjs` and `frontend/eslint.config.mjs` both exist - A dedicated `lint` job in `.gitea/workflows/tests.yml`, linting each workspace separately - `npm run lint` exits 0 in both workspaces The defect-only gate this issue asked for is the shape that shipped: defect rules error, stylistic and advisory rules warn. ## It has since proved itself twice Worth recording, since the issue argued ESLint would be "the multiplier behind several other findings" and that turned out to be measurable: - The first run found a **live bug** — `CustomerAuthContext` left `loading` true forever on a rejected `fetchMe()`, rendering a permanent spinner instead of a signed-out page — plus silent failure in every admin screen's refetch. - The React and SonarJS rules it turned on are what surfaced the entire #81 backlog, including four genuine React defects (a missing `key`, and three context providers re-rendering the whole storefront). #81 is now closed with those fixed, and the same rules confirmed the fix: 35 warnings down to 31. ## Deliberately not in scope, and still not `recommendedTypeChecked` and the `no-unsafe-*` family — roughly 325 findings — remain off. That was a decision recorded here, not an omission: those findings trace to `pool.query()` and `fetch().json()` returning `any`, and typing those is the whole content of **#65**, which stays open and owns them.
bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-20 15:44:21 -05:00
bermudalamb moved this to Released in Code Quality and Hardening on 2026-08-21 12:12:00 -05:00
Sign in to join this conversation.
No labels
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#60