A pass with React Doctor

The diagnosis

One command, 494 findings, a score of 36/100 (“Critical”). First job was separating signal from noise: 487 of those were in src/, and the other 7 were things like a Python virtualenv that happened to live in the repo and a CI YAML file — not React, not mine to fix here. The very first “top error” it showed me was a command-injection warning inside pip’s source code. Good reminder that a scanner scans everything you point it at.

I pulled the structured JSON report and ranked the rules by severity and file spread. That ranking, not the pretty terminal output, drove everything after.

Triage before touching anything

React Doctor’s own guidance is sensible and I followed it: treat findings as hypotheses, read the code before confirming, prefer behavior-preserving fixes, and sample before you sweep when a single rule spans dozens of files. So I split the work by ROI:

• Do now: real bugs and cheap mechanical wins — effect cleanups, side effects inside state updaters, missing button type, and fetches that read the body without checking the status.
• Defer: the migration-scale rules — a full Framer Motion import in 53 files, 40 “giant components,” array-index keys in 29 files. Each is a project on its own and the wrong thing to sweep in one unreviewable pass.

Fixes that landed

Effect cleanup. An auto-save effect kicked off a fetch and a setTimeout with no cleanup, so an unmount mid-flight could set state on a dead component. Fixed with an AbortController and a cleared timer, plus ignoring the resulting abort error.

Side effects in state updaters. The core rule here is that React may call an updater function more than once, so setX(prev => { doSideEffect(); return next }) is a trap. The clean fixes moved the side effect to where it belongs: the game demo does its round transition in the interval callback via a ref instead of inside setProgress; the weather and fleet toggles persist to localStorage in an effect keyed on the value; the calendar’s infinite scroll captures scroll height before the prepend rather than during it.

The toggle case is even simpler — the updater just computes the next value, and persistence moves to an effect keyed on it:

Two more from the same batch. The landing GraphQL typewriter cleared its interval from inside the updater; now the updater is pure and a small effect stops the interval at the end — and that effect only calls clearInterval, never setState, so it stays clear of the rule that bit the stepper:

And the infinite calendar scroll wrote a ref (the scroll height to restore after a prepend) from inside its updater; that read moves out ahead of the state update, using a ref that mirrors the current periods:

Button types. 48 buttons across 30 files had no explicit type, which defaults to submit. Before mass-editing I checked that none of those 30 files even contained a <form>, so type="button" was unambiguously correct — a genuine submit button would have wanted type="submit" instead. Then a small codemod added the attribute to exactly the flagged lines.

Fetch status checks. fetch() does not reject on a 4xx/5xx — it resolves, and .json() then happily parses an error body. Added if (!res.ok) throw before the reads that lacked it.

One in this batch was a proxy, not a client read: the graphql route forwards the upstream status, so the fix isn’t to throw on a bad status — it’s to guard the parse so a non-JSON upstream error can’t throw:

The fix that fought back

The biggest cluster by count was the same stepper pattern copied across ten algorithm-visualizer pages: a play/advance control that called stop() (clear the interval, set playing false) from inside the setStepIdx updater. Textbook impure updater.

My first instinct: make the updater pure and move the stop into a small “when we reach the last step, stop” effect. It read cleanly and typechecked. Then React Doctor flagged the fix with a different rule: calling setState synchronously inside an effect body causes cascading renders. I had traded one finding for another — whack-a-mole.

The genuinely correct fix is neither the updater nor an effect: the side effect belongs in the event that drives the change (the interval tick / the click handler), reading the current step from a synced ref. That’s right, but it’s a per-file restructure across ten files — and critically, these particular side effects are idempotent (clearing an already-clear interval and setting a boolean false twice are both no-ops), so the real-world harm is close to zero. This is exactly the “sample before you sweep” case. I reverted the whole stepper batch and left it as a focused follow-up rather than bloat this PR with a risky, low-value ten-file rewrite.

The lesson that stuck: a “fix” that only relocates a side effect from one disallowed place to another isn’t a fix. And a true positive is not automatically worth fixing now — idempotent impurity in a demo is a different priority than a real leak in a save path.

Update — came back and did it. In a follow-up I applied the correct fix across all ten steppers: a ref mirrors the current step (a ref write in an effect, which is allowed), and play/advance check that ref and stop() from the interval or the click handler, leaving the updater pure. That cleared every impure-updater finding in learn/ (30 to 0, twice over) with zero new setState-in-effect — the proof the recipe was right all along, just in the wrong place the first time. Playback, single-step, and reset behave exactly as before.

Then the tests caught it out. Writing a test that clicks Play and advances fake timers past the end — the exact thing you’d write to lock the behavior in — blew up with Cannot read properties of undefined. The guard reads stepIdxRef.current, but that ref is only refreshed by an effect after React commits. At a real 800ms cadence the effect always flushes between ticks, so it looked correct. Batched timers fire many ticks in one go with no commit in between, so the ref stays stale, the guard never trips, and stepIdx runs off the end of the array.

The fix is to stop leaning on the effect for the value the loop depends on: write the ref synchronously in the same callback that advances the step. The effect stays (it still catches Step and Reset), but the interval no longer races it. Lesson: a fix that “works” because of a timing gap isn’t done until a test closes the gap — and the test is what found it.

Reading before fixing: the false positives

Two flagged items were false positives once I read them. A “side effect in a GET handler” (CSRF risk) pointed at Query.create() in the TCG route — but that’s a read-only query builder and the endpoint is idempotent and CDN-cached, no state mutation anywhere. And a “fetch used without a status check” in the vitals proxy was already resilient: it forwards the upstream status and parses with .catch(() => null). Both left as-is, documented. The tool says as much itself: don’t suppress without evidence from the file.

Deferred by design

The migration-scale rules are real and worth doing — the full framer-motion import inflates the bundle, giant components are hard to change, array-index keys bite on reorder — but they’re each a deliberate, reviewable effort with their own trade-offs. Sweeping 53 files of motion imports or splitting 40 components in a “react-doctor fixes” PR would be unreviewable and exactly the failure mode the tool warns about. Those get their own PRs, a sampled recipe first.

A second pass: render & layout work

Coming back for a second batch, this time performance rules that were real and safe (as opposed to the migration-scale ones).

Unmemoized context values. Two providers built their context value inline, so a brand-new object every render — which makes every consumer of that context re-render even when nothing it cares about changed. The fix is a useMemo keyed on the values that actually change (the store functions are already stable module-level refs).

toLocaleString() in render. Two operator components formatted a timestamp with toLocaleString() during render — but locale and timezone differ between the server and the browser, so that’s a hydration mismatch. The interesting part is the fix hit the same tension as the stepper: the obvious “format after mount” needs a setState in an effect, which React Doctor flags. The clean answer that satisfies both rules is useSyncExternalStore with a server snapshot — render an empty string on the server, the formatted value on the client, no effect and no mismatch.

<img> to next/image. The “use next/image” rule is mostly a defer — but the email-studio demo’s image block is a real case worth doing. It renders a data: URL from a local file import, so there’s nothing for the optimizer to actually do; the fix is next/image with fill and unoptimized, which clears the lint and keeps the exact same output.

Framer Motion, sampled. The biggest deferred item is the full framer-motion import across ~53 files — the fix is LazyMotion plus the lighter m components. Per the tool’s own advice I did a sample first: mount the provider once and convert three files, to prove the recipe before sweeping the rest. Two things it forced me to get right — the bundle must be domMax (not the smaller domAnimation) because the app animates layout and drag, and it has to stay non-strict so the ~50 files still on motion kept working while the migration was in flight. That sweep has since landed — all 50 remaining files are now on m.

Then I measured it, and the honest number is small. A production build before and after the sweep moves the total client JS by about 2.8 KB gzipped — roughly 0.1%. That is not the win it looks like on paper, and the reason is the interesting part: domMax is a static import in providers.tsx, so the full feature set (layout projection, drag, gestures — about 38 KB gzipped of Framer code) ships app-wide no matter what. Swapping motion for m only drops the redundant component wrappers, not the features.

So the sweep is not the payoff — it is the prerequisite. As long as a single file statically imports the full motion, domMax cannot move to a dynamic import. Now that nothing does, the real next step — async-loading domMax so that ~38 KB loads only when an animated view actually mounts — is finally unblocked. That is where the measurable win lands, and it gets its own PR.

Following up on a deferred rule: the array-index keys

One of the deferred rules was no-array-index-as-key — 65 hits across the codebase. Rather than sweep-edit or keep ignoring it, I actually read all 65. Every single one falls into a safe bucket: a static or append-only list that never reorders, an idiomatic recharts <Cell> inside a .map, or a pure-render list with no local state to mis-associate. The lists that do reorder (drag-and-drop boards, editable rows) were already keyed by stable ids. So the rule was firing 65 times with a real-world harm of zero.

Index keys only bite when a list is reordered or has items inserted in the middle: React reuses the wrong DOM node and any per-item local state (focus, an open menu, an animation) attaches to the wrong row. None of that applies to an append-only or never-changing list — there the index is a stable identity.

So the right move was to mute the rule, not edit 65 files. React Doctor takes a doctor.config.json with per-rule severities. One gotcha that cost me a few runs: the rule key has to be the fully-qualified react-doctor/no-array-index-as-key — the bare short name loads without error but silently doesn’t match, so the score never moves. With the prefix, the 65 findings drop out and the score ticked up.

Config-as-suppression only earns its keep because the audit came first. Muting a rule you haven’t read is how you turn a scanner into decoration.

What React Doctor got right, and wrong

Right: the bug rules are high-signal — impure updators, missing effect cleanup, unchecked fetches, and missing button types are all real. The JSON report is the actual product; ranking rules by severity and spread is what makes it usable. And its meta-advice (“sample before you sweep,” “split broad work into separate PRs”) is genuinely good process.

Wrong / careful: it scans everything you point it at, so a repo with a stray venv gets you findings inside pip. Some rules are context-blind (the read-only GET handler, the already-resilient proxy). Severity isn’t priority: 73 idempotent updater findings in demos ranked above a single real leak. And one of its own rules can flag the naive fix for another — you have to understand the underlying React model, not just chase the score down.

Net: a good hypothesis generator, a bad autopilot. Every finding got read before it got fixed, deferred, or dismissed — which is the only way to use a tool like this.