Eight setState-in-effect sites: triage which are derived state and which are legitimate #99

New Issue

2026-08-21T16:01:40-05:00

bermudalamb commented

2026-08-21 16:01:40 -05:00

react-hooks/set-state-in-effect fires eight times. They are not all the same problem, and a blanket "fix all eight" would make some of them worse, so this is a triage rather than a task list.

The eight

Site	Shape
`customer/VerifyEmail.tsx:33`	`setStatus('failed')` when there is no token
`cart/CartContext.tsx:31`	`setItems([])` when there is no customer
`customer/FavoritesContext.tsx:38`	`setFavorites([])` when there is no customer
`customer/CustomerAuthContext.tsx:36`	same shape
`App.tsx:167`	`setLoading(true)` before a debounced fetch
`cart/Cart.tsx:72`	load-on-mount
`admin/Categories.tsx:81`	load-on-mount
`admin/Tags.tsx:39`	load-on-mount

They fall into three groups

Genuinely derived state, worth fixing. VerifyEmail is the clearest: whether the link is missing its token is known during render, from the URL. No request is needed to find that out, so routing it through an effect and a state update means the component renders once in a state that was never true. This is exactly the case React's "You Might Not Need an Effect" is about.

A reset that could be derived but is defensible. The three context providers all do the same thing — clear their collection when the customer becomes null. That is real synchronisation with an external system (the session), and the alternative shapes are not obviously better: deriving would mean every consumer handles "signed out" itself, and the key remount trick is more indirection than the problem deserves. Worth deciding deliberately and writing the decision down rather than leaving three identical warnings that each future reader re-investigates.

Legitimate, flagged conservatively. The load-on-mount cases and App.tsx's setLoading(true) are fetches with a pending flag. The rule cannot distinguish "synchronising with a server" from "computing something I already knew", and this is the former.

What this issue should produce

Not eight fixes. A decision per site: fix the ones that are derived, and for the ones that stay, either a targeted eslint-disable-next-line with the reason or — better — a note in the file saying why the effect is the right tool. Eight standing warnings that nobody can distinguish from each other is how a lint gate stops being read, which is the outcome #60 explicitly designed against.

Note the trap #60 already recorded: eslint-disable-next-line must 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 warning.

Verification

No behaviour change is intended for the sites that stay. VerifyEmail has end-to-end coverage for both the missing-token and invalid-token paths, so that one is checkable; the context providers are covered by the auth and favorites specs.

Severity

Low. No defect is known to result from any of these. The value is in the codebase not carrying eight identical-looking warnings of which only some matter.

Found during a React best-practices review.

`react-hooks/set-state-in-effect` fires eight times. They are **not all the same problem**, and a blanket "fix all eight" would make some of them worse, so this is a triage rather than a task list. ## The eight | Site | Shape | | --- | --- | | `customer/VerifyEmail.tsx:33` | `setStatus('failed')` when there is no token | | `cart/CartContext.tsx:31` | `setItems([])` when there is no customer | | `customer/FavoritesContext.tsx:38` | `setFavorites([])` when there is no customer | | `customer/CustomerAuthContext.tsx:36` | same shape | | `App.tsx:167` | `setLoading(true)` before a debounced fetch | | `cart/Cart.tsx:72` | load-on-mount | | `admin/Categories.tsx:81` | load-on-mount | | `admin/Tags.tsx:39` | load-on-mount | ## They fall into three groups **Genuinely derived state, worth fixing.** `VerifyEmail` is the clearest: whether the link is missing its token is known during render, from the URL. No request is needed to find that out, so routing it through an effect and a state update means the component renders once in a state that was never true. This is exactly the case React's "You Might Not Need an Effect" is about. **A reset that could be derived but is defensible.** The three context providers all do the same thing — clear their collection when the customer becomes null. That is real synchronisation with an external system (the session), and the alternative shapes are not obviously better: deriving would mean every consumer handles "signed out" itself, and the `key` remount trick is more indirection than the problem deserves. Worth *deciding* deliberately and writing the decision down rather than leaving three identical warnings that each future reader re-investigates. **Legitimate, flagged conservatively.** The load-on-mount cases and `App.tsx`'s `setLoading(true)` are fetches with a pending flag. The rule cannot distinguish "synchronising with a server" from "computing something I already knew", and this is the former. ## What this issue should produce Not eight fixes. A decision per site: fix the ones that are derived, and for the ones that stay, either a targeted `eslint-disable-next-line` with the reason or — better — a note in the file saying why the effect is the right tool. Eight standing warnings that nobody can distinguish from each other is how a lint gate stops being read, which is the outcome #60 explicitly designed against. Note the trap #60 already recorded: `eslint-disable-next-line` must 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 warning. ## Verification No behaviour change is intended for the sites that stay. `VerifyEmail` has end-to-end coverage for both the missing-token and invalid-token paths, so that one is checkable; the context providers are covered by the auth and favorites specs. ## Severity Low. No defect is known to result from any of these. The value is in the codebase not carrying eight identical-looking warnings of which only some matter. Found during a React best-practices review.

bermudalamb referenced this issue

2026-08-21 16:02:14 -05:00

Seventeen components declare mutable props, and one antd prop is deprecated #100

bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-21 16:08:38 -05:00

bermudalamb self-assigned this 2026-08-21 16:09:03 -05:00

bermudalamb referenced this issue from a commit

2026-08-24 12:00:40 -05:00

refactor(frontend): declare props read-only, and drop the deprecated antd prop (#100)

bermudalamb referenced this issue

2026-08-24 12:01:14 -05:00

refactor(frontend): declare props read-only, and drop the deprecated antd prop (#100) #157

bermudalamb referenced this issue from a commit