Clear the 85 minutes of technical debt — four of the fourteen "smells" are real React defects #81

New Issue

2026-08-20T13:52:30-05:00

bermudalamb commented

2026-08-20 13:52:30 -05:00

SonarQube reports 85 minutes of technical debt across 14 code smells, zero bugs and zero vulnerabilities. Debt ratio is 0.1%, so the maintainability rating is A and none of this is failing the gate. Cleaning it up anyway, because reading through the fourteen turned up that several are not stylistic at all.

Four of these are genuine defects, not style

Worth separating from the rest, because "code smell" undersells them.

Rule	Location	What actually happens
`S6477`	`frontend/src/cart/Cart.tsx:160`	The `List.Item` `actions` array holds a `<Button>` with no `key`. React cannot match elements across renders, so the Remove button is torn down and rebuilt on every cart render.
`S6481`	`frontend/src/cart/CartContext.tsx:38`	`value={{ items, itemIds, refresh }}` allocates a new object every render, so every consumer of the cart context re-renders whenever the provider does, whether or not the cart changed. `itemIds` is a fresh `Set` each time too.
`S6481`	`frontend/src/customer/CustomerAuthContext.tsx:53`	Same shape.
`S6481`	`frontend/src/customer/FavoritesContext.tsx:45`	Same shape, plus the same per-render `Set`.

These three contexts wrap the whole storefront, so the re-render fan-out is not theoretical. This is precisely the class of problem #60 filed ESLint to catch, now that the rules are actually running.

Four are cognitive complexity, and want real refactoring

Location	Complexity	Shape
`backend/src/itemFilters.ts:84` — `parseItemFilters`	22 / 15	A linear run of per-field parse blocks; splits cleanly into one helper per field. Well covered by unit tests, so the refactor has a safety net.
`frontend/src/App.tsx:29`	22 / 15	Mostly the nested-ternary render chain, so this overlaps with the two `S3358`s below — fixing those should take most of it out.
`backend/src/routes/adminCategories.ts:82` — `PUT /:id`	19 / 15	The parent-id validation (existence, then the cycle check) is the bulk; extracting it into a resolver leaves a thin handler.
`frontend/src/admin/Customers.tsx:12`	16 / 15	One over the line. Small extraction.

Five are mechanical

Four nested ternaries (S3358) in App.tsx:183, App.tsx:188, admin/Admin.tsx:199, admin/Categories.tsx:129, plus one unnecessary type assertion (S4325) at main.tsx:91 — backdrop as Location, where backdrop is already Location on every branch of its definition.

One looks like a false positive

S6478 at frontend/src/admin/CategoryTreeSelect.tsx:82 — "do not define components during render". The flagged code is antd's popupRender prop, which is a render prop: antd calls it as a function and splices the returned JSX in. It is never mounted as a component type, so the destroy-the-subtree failure the rule warns about cannot occur. The rule's own message names the escape hatch (allowAsProps).

Intending to mark this one False positive in SonarQube rather than contorting idiomatic antd around a rule that has misread it — but flagging the reasoning here rather than doing it quietly, same as #74.

Verification

Frontend has no unit suite (see #72), so the safety net for the React refactors is the TypeScript build, ESLint, and the 83 end-to-end tests. Backend changes are covered by the unit and integration suites. All of it runs before this is called done.

Relationship to other issues

#60 — filed the ESLint work whose React and SonarJS rules surface exactly these. This is the backlog that work exposed.
#79 — the coverage gate. Unrelated except that both touch the same files.

Severity

Low as scored, higher than scored in practice. Nothing is failing and no user is blocked, but three context providers re-rendering the entire storefront unnecessarily is a real cost that a 5-minute-per-smell estimate does not convey.

SonarQube reports **85 minutes of technical debt across 14 code smells**, zero bugs and zero vulnerabilities. Debt ratio is 0.1%, so the maintainability rating is A and none of this is failing the gate. Cleaning it up anyway, because reading through the fourteen turned up that several are not stylistic at all. ## Four of these are genuine defects, not style Worth separating from the rest, because "code smell" undersells them. | Rule | Location | What actually happens | | --- | --- | --- | | `S6477` | `frontend/src/cart/Cart.tsx:160` | The `List.Item` `actions` array holds a `<Button>` with no `key`. React cannot match elements across renders, so the Remove button is torn down and rebuilt on every cart render. | | `S6481` | `frontend/src/cart/CartContext.tsx:38` | `value={{ items, itemIds, refresh }}` allocates a new object every render, so **every consumer of the cart context re-renders whenever the provider does**, whether or not the cart changed. `itemIds` is a fresh `Set` each time too. | | `S6481` | `frontend/src/customer/CustomerAuthContext.tsx:53` | Same shape. | | `S6481` | `frontend/src/customer/FavoritesContext.tsx:45` | Same shape, plus the same per-render `Set`. | These three contexts wrap the whole storefront, so the re-render fan-out is not theoretical. This is precisely the class of problem #60 filed ESLint to catch, now that the rules are actually running. ## Four are cognitive complexity, and want real refactoring | Location | Complexity | Shape | | --- | --- | --- | | `backend/src/itemFilters.ts:84` — `parseItemFilters` | 22 / 15 | A linear run of per-field parse blocks; splits cleanly into one helper per field. Well covered by unit tests, so the refactor has a safety net. | | `frontend/src/App.tsx:29` | 22 / 15 | Mostly the nested-ternary render chain, so this overlaps with the two `S3358`s below — fixing those should take most of it out. | | `backend/src/routes/adminCategories.ts:82` — `PUT /:id` | 19 / 15 | The parent-id validation (existence, then the cycle check) is the bulk; extracting it into a resolver leaves a thin handler. | | `frontend/src/admin/Customers.tsx:12` | 16 / 15 | One over the line. Small extraction. | ## Five are mechanical Four nested ternaries (`S3358`) in `App.tsx:183`, `App.tsx:188`, `admin/Admin.tsx:199`, `admin/Categories.tsx:129`, plus one unnecessary type assertion (`S4325`) at `main.tsx:91` — `backdrop as Location`, where `backdrop` is already `Location` on every branch of its definition. ## One looks like a false positive `S6478` at `frontend/src/admin/CategoryTreeSelect.tsx:82` — "do not define components during render". The flagged code is antd's `popupRender` prop, which is a **render prop**: antd calls it as a function and splices the returned JSX in. It is never mounted as a component type, so the destroy-the-subtree failure the rule warns about cannot occur. The rule's own message names the escape hatch (`allowAsProps`). Intending to mark this one **False positive** in SonarQube rather than contorting idiomatic antd around a rule that has misread it — but flagging the reasoning here rather than doing it quietly, same as #74. ## Verification Frontend has no unit suite (see #72), so the safety net for the React refactors is the TypeScript build, ESLint, and the 83 end-to-end tests. Backend changes are covered by the unit and integration suites. All of it runs before this is called done. ## Relationship to other issues - **#60** — filed the ESLint work whose React and SonarJS rules surface exactly these. This is the backlog that work exposed. - **#79** — the coverage gate. Unrelated except that both touch the same files. ## Severity Low as scored, higher than scored in practice. Nothing is failing and no user is blocked, but three context providers re-rendering the entire storefront unnecessarily is a real cost that a 5-minute-per-smell estimate does not convey.

bermudalamb commented

2026-08-20 14:40:58 -05:00

Done on `feature/81-technical-debt` — 85 minutes to 5, 14 smells to 1

Verified by scanning the working tree against the scratch key, not by reasoning about what the rules would say:

	Before	After
`sqale_index`	85 min	5 min
`code_smells`	14	1
`bugs` / `vulnerabilities`	0 / 0	0 / 0

The one remaining is the S6478 false positive, now marked False positive in SonarQube with the render-prop reasoning recorded on it. Its 5 minutes is the whole remaining balance.

The four real defects

Fixed as described above — the missing key in Cart's actions array, and useMemo on all three context provider values plus the two derived Sets that were being rebuilt every render.

The complexity findings, and where I got it wrong twice

Worth recording, because two plausible-sounding fixes did nothing and the scan is the only reason I know that.

parseItemFilters and adminCategories' PUT both split cleanly, with parse order preserved exactly in the former — a query wrong in two ways reports the first field, so reordering the helper calls would change which error a caller sees. App became a Catalogue component, which took the two nested ternaries with it.

Customers took three attempts:

Hoisting the confirm dialog to module level. The reported line moved from 12 to 60 and I read that as the finding having moved to the hoisted function. It had not — line 60 was Customers() itself, still scoring exactly 16. I claimed this fixed in the first commit; it was not, and the follow-up commit says so.
Collapsing five branches on disabling into one copy object. Fixed the hoisted function. Customers() stayed at 16.
Extracting ten ternaries from the table's cell renderers. Still 16. The ternaries inside a render callback were never the weight.

What actually carried the score was the drawer body and the reserved-items dialog body — two JSX blocks whose loading, empty and populated states are each a branch nested several levels down. Extracting them as CustomerDetailPanel and ReservedItemsBody took it under.

The cell-renderer extraction is kept even though it moved no metric, because BooleanTag removes a repetition the Status column was open-coding differently from Verified and Subscribed.

Verification

Backend build clean, unit 78 pass, integration 134 pass
Frontend build clean, e2e 83 pass
ESLint 35 → 31 warnings, 0 errors, no file newly warning. The React and SonarJS rules from #60 are what surfaced this backlog, so it is fitting that they also confirm it

Something found on the way, worth its own issue

The e2e suite is not idempotent against a persistent database. Two consecutive runs against a database that had already served three produced two different pairs of failures — auth.spec.ts:73 and favorites.spec.ts:100 on one run, favorites.spec.ts:44 on the next. Recreating the database produced a clean 83.

CI is unaffected, because its Postgres service is fresh per run. Locally it matters: a developer re-running the suite gets failures that look like regressions in whatever they just changed. I nearly attributed these to the context memoization for exactly that reason.

This belongs with #72's theme — a pipeline that can be wrong in ways that look right. Say the word and I will file it.

Not pushed — left local per the usual arrangement.

## Done on `feature/81-technical-debt` — 85 minutes to 5, 14 smells to 1 Verified by scanning the working tree against the scratch key, not by reasoning about what the rules would say: | | Before | After | | --- | --- | --- | | `sqale_index` | 85 min | **5 min** | | `code_smells` | 14 | **1** | | `bugs` / `vulnerabilities` | 0 / 0 | 0 / 0 | The one remaining is the `S6478` false positive, now marked **False positive** in SonarQube with the render-prop reasoning recorded on it. Its 5 minutes is the whole remaining balance. ## The four real defects Fixed as described above — the missing `key` in Cart's actions array, and `useMemo` on all three context provider values plus the two derived `Set`s that were being rebuilt every render. ## The complexity findings, and where I got it wrong twice Worth recording, because two plausible-sounding fixes did nothing and the scan is the only reason I know that. `parseItemFilters` and `adminCategories`' PUT both split cleanly, with parse order preserved exactly in the former — a query wrong in two ways reports the first field, so reordering the helper calls would change which error a caller sees. `App` became a `Catalogue` component, which took the two nested ternaries with it. `Customers` took three attempts: 1. **Hoisting the confirm dialog to module level.** The reported line moved from 12 to 60 and I read that as the finding having moved to the hoisted function. It had not — line 60 was `Customers()` itself, still scoring exactly 16. I claimed this fixed in the first commit; it was not, and the follow-up commit says so. 2. **Collapsing five branches on `disabling` into one copy object.** Fixed the hoisted function. `Customers()` stayed at 16. 3. **Extracting ten ternaries from the table's cell renderers.** Still 16. The ternaries inside a render callback were never the weight. What actually carried the score was the drawer body and the reserved-items dialog body — two JSX blocks whose loading, empty and populated states are each a branch nested several levels down. Extracting them as `CustomerDetailPanel` and `ReservedItemsBody` took it under. The cell-renderer extraction is kept even though it moved no metric, because `BooleanTag` removes a repetition the Status column was open-coding differently from Verified and Subscribed. ## Verification - Backend build clean, **unit 78 pass**, **integration 134 pass** - Frontend build clean, **e2e 83 pass** - **ESLint 35 → 31 warnings**, 0 errors, no file newly warning. The React and SonarJS rules from #60 are what surfaced this backlog, so it is fitting that they also confirm it ## Something found on the way, worth its own issue **The e2e suite is not idempotent against a persistent database.** Two consecutive runs against a database that had already served three produced two *different* pairs of failures — `auth.spec.ts:73` and `favorites.spec.ts:100` on one run, `favorites.spec.ts:44` on the next. Recreating the database produced a clean 83. CI is unaffected, because its Postgres service is fresh per run. Locally it matters: a developer re-running the suite gets failures that look like regressions in whatever they just changed. I nearly attributed these to the context memoization for exactly that reason. This belongs with #72's theme — a pipeline that can be wrong in ways that look right. Say the word and I will file it. Not pushed — left local per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-20 15:06:41 -05:00

refactor: clear the 85 minutes of technical debt (#81)

bermudalamb referenced this issue from a commit

2026-08-20 15:06:41 -05:00

refactor(frontend): actually reduce Customers' complexity rather than relocate it (#81)

bermudalamb self-assigned this 2026-08-20 15:22:16 -05:00

bermudalamb added this to the Code Quality and Hardening project 2026-08-20 15:22:22 -05:00

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-20 15:22:27 -05:00

bermudalamb closed this issue

2026-08-20 15:22:41 -05:00

bermudalamb referenced this issue