The composite action ran the score step as `react-doctor <dir> --score`
without forwarding any `--fail-on` value, so the CLI's default
(`--fail-on error`) applied. Combined with `set -e -o pipefail`, that
killed the job whenever the score landed in the "Needs work" band even
though the diff scan had already satisfied the user's configured
`fail-on` and the score itself is the only signal the step exists for.
Two complementary fixes:
1. action.yml: the score step now always runs with `--fail-on none`
and tolerates the pipeline failing (`|| true`). The score's value
is the only signal — its exit code never gates the workflow.
2. CLI: `--score` is an output-collection mode. Skip the
`shouldFailForDiagnostics` exit-code branch when `--score` is set,
matching the documented contract that `--score` outputs only the
score.
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(catalog): resolve react version from Bun grouped catalogs (#191)
Adds support for `workspaces.catalogs.<group>` lookups in package.json,
mirroring the existing top-level `catalogs` handling. Without this,
`catalog:<group>` references in workspace packages resolved to null and
react-doctor failed with the misleading "No React dependency found"
error against repos using Bun grouped catalogs.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(catalog): thread leaf catalog ref through monorepo-root resolution
Bugbot caught a real bug in the original PR: when resolveCatalogVersion
runs against the MONOREPO ROOT package.json (which typically has no
`react` dep of its own), the catalogName was being derived from the
root's own dependencies and was therefore always null — so named-catalog
lookups silently fell through to the `Object.values(workspaces.catalogs)`
scan and returned an arbitrary group.
When multiple Bun grouped catalogs define the same package at different
versions (e.g. `react18` and `react19`), `catalog:react19` from a leaf
package was getting resolved to whatever group iterated first — usually
`react18`.
Fix: capture the leaf's catalog reference (`catalog:react19`) once in
`discoverProject`, and thread it as an explicit `explicitCatalogReference`
parameter through every fallback that resolves against the root or a
sibling root package.json. The same plumbing also applies to
`findDependencyInfoFromMonorepoRoot`, which had the identical bug.
New regression fixture (`bun-multiple-grouped-catalogs`) defines both
`react18` and `react19` groups; the leaf points at `react19`. The new
test asserts the resolver returns `19.2.0`, not `18.3.1` — without
this fix, it returned the wrong version.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(catalog): fall through to root's own catalog reference when leaf has none
Bugbot caught the inverse-direction edge case: when
`explicitCatalogReference` is `null` (the leaf has no `react`
dependency at all), the previous `!== undefined` check treated
`null` as an intentional override and suppressed the
`extractCatalogName(rawVersion)` derivation from the package.json
the resolver was actually looking at.
In practice that meant: monorepos where the root package.json itself
declares `react: "catalog:<group>"` lost their own catalog name
hint as soon as we recursed into the root with a leaf that didn't
carry react. Replace the discriminating check with `??` so the
caller's name wins when present and the local package.json's own
reference fills in otherwise.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* feat(react-doctor): add rawTextWrapperComponents config (#183)
Adds a narrow escape hatch for rn-no-raw-text aimed at components
that are not themselves text elements but safely route string-only
children through an internal <Text> (e.g. heroui-native's <Button>,
which stringifies its children and renders them through ButtonLabel).
- types.ts: new ReactDoctorConfig.rawTextWrapperComponents field.
- filter-diagnostics.ts: suppress rn-no-raw-text inside listed
wrappers ONLY when the wrapper's children are entirely
stringifiable. A wrapper with a sibling JSX child (e.g.
<Button>Save<Icon /></Button>) still reports.
- README.md: document the new config key alongside textComponents.
- tests/regressions/rn-and-motion.test.ts: positive coverage for
string-only children, template-literal children, and member-
expression wrappers; negative coverage for mixed-children
wrappers and non-listed wrappers.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): skip closed-sibling openers when resolving wrapper
Deep review caught a false-negative in the wrapper-enclosure
heuristic. Walking back from the diagnostic to find the immediate
JSX opener could land on a CLOSED sibling element instead of the
real enclosing parent. With config rawTextWrapperComponents: ['Button']
and source like:
<View>
<Button>Inner</Button>
Save
</View>
the diagnostic for 'Save' (whose real parent is <View>) walked back
to <Button> on the previous line, found Button-with-only-string-
children, and incorrectly suppressed.
Refactored isInsideStringOnlyWrapper to iterate openers nearest-
above outward and verify enclosure by checking that the matching
closing tag falls AFTER the diagnostic position (not before). When
the closer is before the diagnostic, the opener is a closed
sibling — skip and walk further up. Also resolved JSX range now
returns the closer position alongside the body text in a single
helper.
- filter-diagnostics.ts: refactor extractWrapperBodyText into
resolveJsxRange (returns closer position + body), pass
diagnostic.column through to the wrapper check, iterate openers
outward skipping closed siblings.
- tests/regressions/rn-and-motion.test.ts:
- new closed-sibling regression test for the bug above
- new end-to-end test that runs real oxlint on a React Native
project and verifies rawTextWrapperComponents suppresses the
actual diagnostics emitted (validates that real oxlint
line/column values flow correctly through the new filter)
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
- Show diagnostics by rule name (plugin/rule) with accordion details
- Update copy command to `npx react-doctor@latest`
- Add description and framework support line to header
- Use side-by-side score header layout matching CLI branding
- Remove detection steps, category breakdown, and hidden summary
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(react-doctor): forward reactMajorVersion in programmatic diagnose() + cleanups
H1 (regression introduced by #172) — `diagnose()` in `src/index.ts`
forgot to forward `reactMajorVersion` to `runOxlint`. After the
directional version-gating change in #172, that meant every
"prefer-newer-api" rule (today: `prefer-use-effect-event`) was silently
skipped for every programmatic API consumer, even on React 19+
projects. The CLI (`scan.ts`) was unaffected because it always passed
the version explicitly.
Fix is one line + one import — mirror what `scan.ts` already does.
Added `tests/diagnose.test.ts` with a regression test that asserts
`prefer-use-effect-event` fires on a React 19 fixture, plus a
symmetric guard that it stays skipped when the React version can't
be parsed (e.g. a github: range).
H2 — updated the stale docstring on `runOxlint`'s `reactMajorVersion`
field. The doc still claimed "`null` means unknown — leave those
rules enabled" but after #172 the null branch is directional
(deprecation-warning rules stay on, prefer-newer-api rules go off).
M1 — `SUB_HANDLER_DIRECT_CALLEE_NAMES` was just an alias of
`TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES`. Both names existed for
narrative reasons in different files; knip flagged the duplicate
export. Collapsed to the canonical name and updated the one consumer
(`isCallExpressionWithSubHandlerCallee` in state-and-effects).
L1 — moved `walkInsideStatementBlocks` from inline in
`state-and-effects.ts` to `plugin/helpers.ts` next to its sibling
`walkAst`. It already had four call sites and is the natural
"synchronous-only" walker for any rule asking what runs inside an
effect's own body — colocating with `walkAst` makes future rules
discover it.
L2 (audit finding) — proposed receiver-gating
`post`/`put`/`patch` in `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES`
INTENTIONALLY NOT TAKEN. The canonical "You Might Not Need an Effect"
§6 example is `post(jsonToSubmit)` as a bare callee, so removing
those names breaks textbook detection (3 existing tests / fixtures).
Documented the trade-off in the constants.ts docstring.
Validation
- 642 tests passing (640 baseline + 2 new diagnose-API regressions)
- typecheck / lint / format clean
Co-authored-by: Cursor <cursoragent@cursor.com>
* chore: format scripts/benchmark-scores.ts
Pre-existing formatting issue introduced in 6afdc04 — the script was
committed unformatted, breaking `pnpm format:check` on every PR
branched off main since. Picked up by rebasing this PR.
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Audit pass on `main` after the consolidation/AGENTS.md cleanups (#167, #169).
Targets eight specific FP/FN patterns surfaced by re-reading the merged code
end-to-end and checking each rule against its own intent docstring.
High
- H1 unify release detection between `effectHasCleanupRelease` and
`cleanupReleasesSubscription`: both now share `isCleanupReturn` /
`isReleaseLikeCall` / `containsReleaseLikeCall`. Previously
`prefer-use-sync-external-store` and `prefer-use-effect-event` could
disagree on whether a cleanup with a generic teardown verb (`cleanup()`,
`dispose()`) counted as a release, producing inconsistent diagnostics.
- H2 require function-shaped return for `isExternalSyncEffect`: any
`return <expr>` previously qualified the effect as "external sync" and
silently disabled chain detection, so `return null` / `return 42` /
`return condition && cleanup` would mask real chains. We now only
treat function-shaped returns (Arrow, FnExpr, Call, Identifier) as
cleanup.
- H3 directional version gating: `prefer-use-effect-event` is a
"prefer-newer-api" rule and was firing on projects where the React
major couldn't be detected. Gating now records whether each rule is
"prefer-newer-api" (skip when version is unknown) or
"deprecation-warning" (keep firing when version is unknown).
Medium
- M1 receiver-gate ambiguous direct callees: `track`, `logEvent`, `del`
removed from `EVENT_TRIGGERED_SIDE_EFFECT_CALLEES` — they were too
generic as bare identifiers and produced FPs on user-defined helpers.
Receiver-gated member calls (`analytics.track(...)`) still fire.
- M2 extend `findSubHandlerForEnclosingFunction` to FunctionDeclaration
and AssignmentExpression shapes — `function handler() {}` and
`let h; h = (e) => {}` are now recognized alongside `const h = ...`.
- M3 deep-walk for `noPropCallbackInEffect`: the rule previously only
scanned top-level effect statements and missed the very common
`if (didChange) onChange(state)` shape. Walk now descends through
control-flow blocks but stops at function boundaries (so deferred
sub-handlers like `setTimeout(() => onChange(state))` stay the
domain of `prefer-use-effect-event`).
- M4 `collectWrittenStateNamesInEffect` no longer counts setter calls
inside nested functions for chain detection — deferred writes
(`setTimeout(() => setX(...))`) are not synchronous chain triggers.
Low
- L1 `noMirrorPropEffect` now accepts multi-element deps as long as the
mirrored prop's root is in the deps array. The prior
"exactly one dep" requirement missed legitimate mirror shapes with an
unrelated extra dep.
- L2 (paired with H1/H2) `effectHasCleanupRelease` "return Identifier"
now narrows to known bound release names so a stray non-function
identifier return doesn't silently look like a release.
- L3 extend `prop-stack-barrier.test.ts` to cover all four
`createComponentPropStackTracker` consumers (`no-derived-useState`,
`no-prop-callback-in-effect`, `no-mirror-prop-effect`,
`prefer-use-effect-event`) so the empty-frame barrier semantic is
regression-tested for every rule that depends on it.
- L4 extract a sibling `createComponentBindingStackTracker` and migrate
`noEffectEventInDeps` onto it — the third inlined push/pop scaffold
was effectively a copy-paste of the prop-stack one specialised to
binding sets.
Tests
- 634 passing (up from 619). New regressions cover each behavior change
above plus FP guards (e.g. mirror shape with prop root NOT in deps
must not fire, sub-handler reads must not fire `no-prop-callback-in-effect`).
- `tests/run-oxlint.test.ts` now passes `reactMajorVersion: 19` for the
basic / Next / TanStack Start fixtures so they exercise the
prefer-newer-api rules under a known React major.
Co-authored-by: Cursor <cursoragent@cursor.com>
* refactor(react-doctor): AGENTS.md compliance pass on state-and-effects
Round 2 of the post-merge audit. Found one direct AGENTS.md violation,
two duplicate constant sets, two duplicated AST walks, one duplicate
helper function, and a 4-rule duplicated machinery pattern (~120 LOC).
**AGENTS.md violations**
- `interface MirrorBinding` was declared inside `checkComponent` —
AGENTS.md requires "Keep all types in the global scope." Hoisted
to module scope.
- Five `(n) =>` parameters in `NONDETERMINISTIC_RENDER_PATTERNS` —
AGENTS.md requires descriptive variable names. Renamed to `node`.
- Abbreviated locals `stmts`, `lastStmt` inside
`findSubscribeLikeUsages` — renamed to `callbackStatements` and
`lastCallbackStatement`. `stmt` loop variable in
`noPropCallbackInEffect` renamed to `bodyStatement`.
**Duplicate constants — single source of truth**
- `SUBSCRIBE_LIKE_METHOD_NAMES` (state-and-effects.ts) was identical
to `SUBSCRIPTION_METHOD_NAMES` (constants.ts). Removed the local
copy.
- `UNSUBSCRIBE_LIKE_METHOD_NAMES` was identical to
`UNSUBSCRIPTION_METHOD_NAMES`. Removed.
- `TIMER_CALLEE_NAMES_REQUIRING_CLEANUP` and
`TIMER_CLEANUP_CALLEE_NAMES` moved from rule scope to constants.ts
alongside the existing `TIMER_AND_SCHEDULER_DIRECT_CALLEE_NAMES`.
- The hardcoded
`/^(unsubscribe|unsub|removeListener|...|cleanup|dispose|destroy|teardown)$/i`
regex is now `CLEANUP_LIKE_RELEASE_CALLEE_NAMES` — a Set spread
from `UNSUBSCRIPTION_METHOD_NAMES` plus the four generic teardown
verbs. Adding a new unsubscribe verb in one place propagates to
both rules.
- Inline `/^on[A-Z]/` regex in `preferUseEffectEvent` is now
`REACT_HANDLER_PROP_PATTERN` in `constants.ts`.
**Duplicate AST walks — use shared helpers**
- Two manual `while (cursor?.type === "MemberExpression")` chain
walks (`findMutableDepIssue`, `findTriggeredSideEffectCalleeName`)
now use the shared `getRootIdentifierName` helper.
- Local `arePropExpressionsStructurallyEqual` was a strict subset of
`areExpressionsStructurallyEqual` in helpers.ts. Removed and
switched the one caller. As a side benefit `noMirrorPropEffect`
now recognizes mirror patterns through method calls (e.g.
`useState(value.toUpperCase())` paired with
`setX(value.toUpperCase())`) — `getPropRootName` was extended to
follow call chains, and the structural equality check handles
CallExpression. Two regression tests pin both shapes.
**Duplicated prop-stack machinery — extracted shared helper**
Four rules — `noDerivedUseState`, `noPropCallbackInEffect`,
`noMirrorPropEffect`, `preferUseEffectEvent` — each implemented the
same component-prop-stack pattern: push the destructured param set
on FunctionDeclaration / VariableDeclarator entry, push an empty
barrier for non-component nested helpers, pop on exit, and provide a
barrier-aware `isPropName(name)` lookup. Extracted as
`createComponentPropStackTracker` in `plugin/helpers.ts` with an
optional `onComponentEnter(componentBody)` callback for rules that
run their analysis eagerly on entry. `isFunctionLikeVariableDeclarator`
is now exported from helpers.ts (it was an inline helper used by all
four rules).
**Net diff**: −27 LOC (288 added / 315 removed across 5 files).
All checks green: 619/619 tests (617 prior + 2 new), typecheck, lint
0/0, format.
Co-authored-by: Cursor <cursoragent@cursor.com>
* chore(react-doctor): tighten audit pass — drop redundant logic, un-export helpers
- server.ts: remove `expressionDerivesFromIdentifier` — its result is
provably the same as the `getRootIdentifierName(expression, {followCallChains})`
computed one line earlier in `serverDedupProps`. Inline the single
use, delete the helper.
- state-and-effects.ts: collapse `getPropRootName` — `getRootIdentifierName`
already short-circuits on bare `Identifier`, so the manual fast path
is dead code.
- helpers.ts: un-export `extractDestructuredPropNames` and
`isFunctionLikeVariableDeclarator` — both are now only used inside
`createComponentPropStackTracker` after the prop-stack consolidation.
- README.md: re-run formatter to match `vp fmt --check` (trailing space
introduced by GitHub-UI edit on main was failing CI).
Co-authored-by: Cursor <cursoragent@cursor.com>
---------
Co-authored-by: Cursor <cursoragent@cursor.com>
Cleanup pass over the recently-merged state-and-effects rules from
#153–#157, #162 surfacing six audit findings.
1. **De-duplicate constant sets.** Four overlapping allowlists
(`SUBSCRIPTION_METHOD_NAMES` ⊂ `EXTERNAL_SYNC_MEMBER_METHOD_NAMES`,
`FETCH_MEMBER_OBJECTS` ⊂ `EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS`,
`FETCH_CALLEE_NAMES` ⊂ `EXTERNAL_SYNC_DIRECT_CALLEE_NAMES` &
`EVENT_TRIGGERED_SIDE_EFFECT_CALLEES`, and the timer/scheduler
list shared between `prefer-use-effect-event` and
`no-effect-chain`) now spread from a single base set so adding a
new HTTP client / subscribe verb in one place propagates
everywhere.
2. **Unify `getRootIdentifierName`.** `server.ts` had a private copy
that walked through CallExpression chains; the shared helper in
`helpers.ts` did not. Added an opt-in `followCallChains` option
and deleted the duplicate.
3. **Remove dead branch in `isExternalSyncEffect`.** A redundant
`else if (effectCallback.body?.type !== "BlockStatement")` whose
body was a comment-only no-op.
4. **Migrate `noDerivedUseState` to the shared root walker.** Drops
the inline MemberExpression chain walk and uses
`getRootIdentifierName` for consistency.
5. **Drop `noEffectEventHandler`'s deference to `noEventTriggerState`.**
The deference predicate only checked two of the five preconditions
`noEventTriggerState` actually requires (single dep,
handler-only writes, not render-reachable were not verified),
so when those failed the narrower rule didn't fire AND this rule
deferred — a silent drop. Both rules now fire independently; the
two messages frame the same code differently and a duplicate
diagnostic is strictly better than a missing one. Also removes
~60 lines of useState-name-tracking machinery that only existed
to support the deference. Existing test was updated and a new
regression pinning the double-warn was added.
6. **Receiver-gate `push` / `replace`.** Both names live in
`EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS` but also denote
`Array.prototype.push` and `String.prototype.replace`. Moved
them to a separate `EVENT_TRIGGERED_NAVIGATION_METHOD_NAMES`
set that's only counted when the receiver root is in
`NAVIGATION_RECEIVER_NAMES` (`router`, `navigation`, `navigator`,
`history`, `location`). Two regressions added — `arr.push(x)`
does not fire, `router.push(path)` still does.
All checks green: 617/617 tests, typecheck, lint, format.
Co-authored-by: Cursor <cursoragent@cursor.com>
Rebased onto main after #153 / #163 merged. Test file reconstructed
by appending PR's describe block to main's version + the
reactMajorVersion helper-arg modification. All 555 tests pass.
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* feat(react-doctor): add no-effect-chain rule
New rule (severity: warn) flagging the §7 anti-pattern from React's
'You Might Not Need an Effect' guide:
useEffect(() => { if (card.gold) setGoldCount(c => c + 1); }, [card]);
useEffect(() => { if (goldCount > 3) setRound(r => r + 1); }, [goldCount]);
useEffect(() => { if (round > 5) setIsGameOver(true); }, [round]);
Each link adds an extra render to the tree below the component.
The chain is also rigid — setting `card` to a value from the past
re-fires every downstream effect.
Detector (per component body):
1. Collect every top-level useEffect call and, for each:
- depNames: Identifier names in the dep array
- writtenStateNames: state names whose setter is called in body
- isExternalSync: body returns cleanup OR contains a recognized
external-system call (subscribe / addEventListener / fetch /
setInterval / new MutationObserver / etc.) OR mutates a ref
2. For every ordered pair (A, B) of distinct effects:
edge iff (writes(A) ∩ deps(B)) ≠ ∅
AND ¬isExternalSync(A) AND ¬isExternalSync(B)
3. Report on every reader effect B with the chained state name.
The article calls out one legitimate 'chain' — a multi-step network
cascade where each effect re-fetches based on the previous step's
result. Those effects all have isExternalSync=true (they contain
fetch), so the rule won't fire.
Complements the existing no-cascading-set-state rule (intra-effect
multi-setter detector) without overlapping.
Tests: 5 regression cases.
flags:
- article §7 Game-style three-effect chain
does NOT flag:
- single effect with multiple setters (covered by no-cascading-set-state)
- article's GOOD network-cascade exception (each effect calls fetch)
- chat-connection effect that shares deps with state-reset effect
(createConnection().connect() / .disconnect() is external sync)
- two effects whose written/read state sets are disjoint
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: add changeset for no-effect-chain
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: drop changeset
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-effect-chain — recognize axios.get() etc. as external sync
Bugbot found that EXTERNAL_SYNC_MEMBER_METHOD_NAMES included `post`,
`put`, `patch`, `delete`, `fetch` but omitted `get`. Read-only
network calls inside an effect (`axios.get(...)`, `ky.get(...)`,
`api.get(...)`) were therefore classified as internal-only sync,
producing false-positive chain warnings on the article's exact
'cascade of network fetches' exception.
Added `get`, `head`, and `options` to round out the standard
HTTP method set. Now an `axios.get`-bearing effect correctly has
`isExternalSync = true` and the rule won't flag it as a chain.
Regression test mirrors the article's ShippingForm example but with
`axios.get` instead of `fetch`, asserting zero hits.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-effect-chain — restrict ambiguous HTTP verbs to known client receivers
Bugbot caught: round 1 added `get` to EXTERNAL_SYNC_MEMBER_METHOD_NAMES
to support `axios.get` cascades, but `get` is also a universal
data-structure method name (`Map.get`, `URLSearchParams.get`,
`FormData.get`, `Headers.get`, `WeakMap.get`). Effects whose only
'external' call was `params.get('id')` were misclassified as external
sync, causing false-NEGATIVE chain detection.
Two new constants split the ambiguity:
EXTERNAL_SYNC_MEMBER_METHOD_NAMES — unambiguous methods (subscribe,
addEventListener, post, put, patch, delete, fetch, …). Receiver
is irrelevant; the method name alone marks the call as external.
EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES — the HTTP-vs-data-structure
overlapping verbs (get, head, options). Only count as external
when paired with a recognized HTTP-client receiver name.
EXTERNAL_SYNC_HTTP_CLIENT_RECEIVERS — axios, ky, got, wretch,
ofetch, api, client, http, request, fetcher.
Regression test asserts that a chain where one effect calls
`params.get('theme')` (URLSearchParams) still produces the chain
warning, verifying that data-structure-shape `.get` no longer
exempts the effect.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-effect-chain — `delete` is also ambiguous (Bugbot #156 round 3)
Bugbot caught: round-2 left `delete` in EXTERNAL_SYNC_MEMBER_METHOD_NAMES
(unambiguous external-sync) while moving `get`, `head`, `options`
to the ambiguous set. But `delete` has the same problem — it's the
name of standard JS APIs on Map, Set, URLSearchParams, Headers,
FormData, and WeakMap. An effect whose only 'external' call was
`set.delete(item)` got falsely classified as external sync,
silently exempting it from chain detection.
Move `delete` to EXTERNAL_SYNC_AMBIGUOUS_HTTP_METHOD_NAMES so it's
only counted as external when paired with a recognized HTTP-client
receiver (axios, ky, …). `set.delete()`, `map.delete()`, etc. now
correctly stay classified as internal-only.
Regression test: a chain where one effect calls `next.delete(...)`
on a Set still produces the chain warning.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* test(react-doctor): no-effect-chain — fix tests so they actually exercise isExternalSync (Bugbot #156 round 4)
Bugbot caught: the cascade / ShippingForm / chat tests previously
had no actual write→dep state overlap between their effects, so
they passed regardless of whether `isExternalSync` worked correctly.
Reverting the isExternalSync exemption would NOT have caused those
tests to fail — they were vacuously green.
All three rewritten so the second effect depends on a state that
the first effect writes:
ShippingForm: writes(A) = {cities}, deps(B) = [cities]
axios cascade: writes(A) = {cities}, deps(B) = [cities]
chat: writes(A) = {status}, deps(B) = [status]
In every case both effects are external-sync (fetch / axios.get /
createConnection().connect() / addEventListener), so the chain
detector exempts them. If the exemption breaks, all three tests
will fail — the chain detection itself fires correctly per the
existing intra-effect tests.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* feat(react-doctor): add no-event-trigger-state rule
New rule (severity: warn) flagging the §6 anti-pattern from React's
'You Might Not Need an Effect' guide:
const [jsonToSubmit, setJsonToSubmit] = useState(null);
useEffect(() => {
if (jsonToSubmit !== null) {
post('/api/register', jsonToSubmit);
}
}, [jsonToSubmit]);
function handleSubmit(event) {
event.preventDefault();
setJsonToSubmit({ firstName, lastName });
}
The state variable exists only to schedule an effect to run on click.
The fix is to call `post('/api/register', { firstName, lastName })`
directly inside handleSubmit and delete the state — and that's exactly
what the rule's diagnostic recommends.
Detector pre-conditions (all four must hold) — chosen to keep
real-world false positives near zero:
(1) useEffect with a single-identifier dep array, where the dep is
a useState binding declared in this component
(2) effect body is exactly one IfStatement guarding on that state
with one of: bare truthy, !== null/undefined, === Literal,
.length, or !X
(3) IfStatement.consequent contains a CallExpression whose callee
is in EVENT_TRIGGERED_SIDE_EFFECT_CALLEES (fetch, post, navigate,
showNotification, alert, track, ...) OR a MemberExpression
whose property is in EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS
(`axios.post`, `router.push`, `analytics.track`, etc.)
(4) every setStateX call site in the component is inside a JSX
`on*` handler (or a function bound to one) — i.e. the trigger
is set only by user interactions
(4) is the strongest signal that the state exists *only* to schedule
the effect, and is what distinguishes this rule from §5 (handled by
the existing no-effect-event-handler).
Reuses existing helpers `collectHandlerBindingNames` /
`isInsideEventHandler` from the same file.
Tests: 7 regression cases.
flags:
- canonical post-trigger shape
- axios.post member-call shape
- bare truthy guard with navigate(...)
does NOT flag:
- article's GOOD analytics-on-mount example (empty deps, no trigger)
- state also written outside handlers (mixed reactive logic)
- guard with a non-side-effect callee (compute(seed))
- guard on a prop (no useState binding present)
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: add changeset for no-event-trigger-state
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: drop changeset
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-event-trigger-state — don't flag state also read in render
Bugbot found that the rule's four pre-conditions don't include a
render-reachability guard. State that's BOTH a controlled-input
value AND a trigger for the effect (e.g. `query` driving both
`<input value={query}>` and `useEffect(() => track('search', query),
[query])`) satisfied all four conditions and produced a false
positive — the message told the user to 'delete the state', which
would break the input.
Added the render-reachability check used by rerender-state-only-in-
handlers: collect return expressions → expand transitive
dependencies → check if the trigger state is render-reachable. If
yes, skip — it's dual-purpose, not a pure trigger.
Regression test mirrors the Bugbot example.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-event-trigger-state \+ no-effect-event-handler — round 2 Bugbot follow-ups
Two further issues Bugbot caught after round 1:
1. `getTriggerGuardRootName` returned the first Identifier on
either side of a BinaryExpression. Since `undefined` is parsed
as Identifier (not Literal like `null`), the reversed
ordering `if (undefined !== pendingPayload)` returned
`"undefined"` and silently dropped the violation. Now skip
a curated SENTINEL_IDENTIFIER_NAMES set
(undefined / NaN / null) so the actual state Identifier
wins regardless of operand order.
2. The bare-truthy guard `if (destination) navigate(destination)`
produced TWO warnings on the same line: one from
`no-effect-event-handler` (its original §5 detector) and one
from the new `no-event-trigger-state`. The two messages overlap.
Solution: `no-effect-event-handler` now defers to
`no-event-trigger-state` when the dep is a useState value
(which is what `no-event-trigger-state` was built to handle).
For the canonical §5 case (prop-driven trigger like
`if (isOpen) document.body...`), `no-effect-event-handler`
still fires.
Two regression tests cover both fixes.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): no-effect-event-handler defer only when no-event-trigger-state would actually fire (Bugbot #155 round 3)
Round 2 made no-effect-event-handler defer whenever the trigger
guard's dep was a useState value. But no-event-trigger-state has
narrower preconditions — single dep, handler-only writes, and
specifically a callee in EVENT_TRIGGERED_SIDE_EFFECT_CALLEES (or
EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS for member calls). For a
state-typed trigger guard whose consequent calls a custom function
NOT in those allowlists (`if (trigger) customAction()`), the
round-2 deference silently dropped the warning — neither rule fired.
Tighten the deference: only skip when BOTH predicates hold —
state-typed dep AND the consequent contains a recognized event-
triggered side-effect callee. Otherwise no-effect-event-handler
keeps owning the diagnostic, preserving the previous behavior for
custom-callee shapes.
Pulled the side-effect-callee detection into a reusable
`consequentCallsTriggeredSideEffect` helper that mirrors what
no-event-trigger-state's `findTriggeredSideEffectCalleeName`
already does. The two rules now stay aligned via the same shared
constants (EVENT_TRIGGERED_SIDE_EFFECT_CALLEES,
EVENT_TRIGGERED_SIDE_EFFECT_MEMBER_METHODS).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* refactor(react-doctor): dedupe consequentCallsTriggeredSideEffect into findTriggeredSideEffectCalleeName (Bugbot #155 round 4)
Round 3 introduced `consequentCallsTriggeredSideEffect` for the
deference logic, which duplicated the AST walk + constant lookups
from `findTriggeredSideEffectCalleeName` declared later in the
file. Bugbot pointed out that the boolean function is exactly
`findTriggeredSideEffectCalleeName(node) !== null`.
Drop the duplicate. Reuse the existing helper directly. Forward
reference works because the call site sits inside a closure
(noEffectEventHandler.create()'s returned visitor) — at the time
that visitor runs, all module-level functions are initialized.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* feat(react-doctor): add prefer-use-sync-external-store rule
New rule (severity: warn) flagging the §11 anti-pattern from React's
'You Might Not Need an Effect' guide:
const [snapshot, setSnapshot] = useState(getSnapshot());
useEffect(() => {
const unsub = store.subscribe(() => setSnapshot(getSnapshot()));
return unsub;
}, []);
The hand-rolled subscribe pattern reimplements useSyncExternalStore in
user space — incorrectly. The hook handles tearing during concurrent
renders and SSR snapshots; the manual pattern doesn't. The rule
suggests:
const snapshot = useSyncExternalStore(store.subscribe, getSnapshot);
Detector requires a four-vertex AST match before firing:
(1) useEffect with empty deps `[]`
(2) body declares `const u = X.subscribe(handler)` OR
directly invokes a subscription method X.addEventListener(...)
(3) cleanup is a `return` that returns the unsubscribe binding
directly OR returns a closure that unsubscribes
(4) handler is a single `setY(<getter>)` whose <getter> is
structurally equal to the matching useState's initializer
Subscription method names recognized: subscribe, addEventListener,
addListener, on, watch, listen, sub. Covers zustand, Redux vanilla,
Valtio, Effector, Jotai store API, RxJS observables, and the browser
`addEventListener` shape (matchMedia, navigator.onLine).
Resolves Identifier-passed handlers like
const onChange = () => setIsOnline(navigator.onLine);
window.addEventListener('online', onChange);
by walking earlier const declarations in the same effect body.
Adds:
- SUBSCRIPTION_METHOD_NAMES + UNSUBSCRIPTION_METHOD_NAMES to constants
(replaces the local 4-element set in advancedEventHandlerRefs;
same shape, broader allowlist).
- areExpressionsStructurallyEqual helper for the four-vertex
structural match.
Tests: 7 regression cases — canonical store-subscribe shape, browser
addEventListener shape, lazy-initializer variant, and four
no-flag cases (legitimate chat connection with non-empty deps,
subscribe-as-side-effect with no useState pair, mismatched setter
arg, missing cleanup).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: add changeset for prefer-use-sync-external-store
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: drop changeset
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(react-doctor): widen no-effect-event-handler test predicate to MemberExpression root
Aligns the detector with the literal §5 example from React's
'You Might Not Need an Effect' guide. The previous detector
required the IfStatement test to be a bare Identifier, so
'if (product.isInCart)' (a MemberExpression) didn't match. Walks
the test down to its root identifier so both shapes match when
the root appears in the deps array.
Adds a small 'getRootIdentifierName' helper to plugin/helpers.ts
(used here, exportable for upcoming detectors).
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* feat(react-doctor): split no-derived-state-effect message on memo-worthy derivations
Aligns the diagnostic with §2 (Caching expensive calculations) of
React's 'You Might Not Need an Effect' guide. A setter argument
that contains a user-defined CallExpression — e.g.
`setVisibleTodos(getFilteredTodos(todos, filter))` — now gets:
Derived state in useEffect — wrap the calculation in
useMemo([deps]) (or compute it directly during render if it
isn't expensive)
Pure data shaping (`firstName + ' ' + lastName`) still gets the
'compute during render' message. Coercion / parsing helpers
(Number, parseInt, Math.*, Boolean, etc.) count as trivial.
As part of this change the detector switched to an AST-aware
walker (collectValueIdentifierNames) that no longer over-counts
the callee identifier of a CallExpression or the static property
of a non-computed MemberExpression as a 'reactive value read' —
those are function references and static names, not values that
need to be in deps. This also fixes a pre-existing under-detection
bug where setX(getDerivedValue(propA, propB)) would short-circuit
the rule because getDerivedValue (a stable module-scoped helper)
wasn't in deps.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: add changeset for effect-rule extensions
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* chore: drop changeset
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* refactor(react-doctor): drop unnecessary type casts in collectValueIdentifierNames walker
EsTreeNode is declared with `[key: string]: any` index signature,
so `node[key]` already returns `any` — the four
`as Record<string, unknown>` / `as EsTreeNode` casts were
redundant and violated AGENTS.md 'Do not type cast unless
absolutely necessary'.
Behavior unchanged. Tests + typecheck still green.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* fix(no-derived-state-effect): handle Math/Date members + zero-arg fn calls (Bugbot #153 round 2)
Three findings raised by Bugbot's fresh re-review of PR #153:
1. `Math.floor(raw)` (and any `Math.*` / `Date.*` / `JSON.*` call)
was unreachable: `collectValueIdentifierNames` collected the
chain root ("Math") as a reactive read, the dep-mismatch check
then bailed the rule entirely. Separately, the expensive-
derivation walk read the *property* name ("floor") for member
callees, so the "Math" / "Date" entries in
`TRIVIAL_DERIVATION_CALLEE_NAMES` could never match. New
`BUILTIN_GLOBAL_NAMESPACE_NAMES` set is consulted at the chain
root in both helpers — `Math.floor(raw)` now correctly emits the
trivial "compute during render" diagnostic.
2. New `getRootIdentifierName` helper duplicated the private
`getMemberRootName` in state-and-effects.ts. Removed the
duplicate; both call sites now use the shared helper.
3. `setFilteredItems(applyCurrentFilters())` (zero-arg user
function) produced an empty `valueIdentifierNames` list, both
`.some()` checks vacuously returned false, and the rule fired
with the wrong "state reset" message. Now: when an expensive
derivation is detected, `hasAnyDependencyReference` is forced
true so the message routes to the `useMemo` branch (a user
function call closes over reactive values implicitly, so calling
it is a derivation, not a literal reset).
Also stripped the now-dead "Math" / "Date" entries from
TRIVIAL_DERIVATION_CALLEE_NAMES (they couldn't match `Namespace.method()`
patterns).
Regression tests added for findings #1 and #3. All 489 tests pass;
lint, typecheck, format clean.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
* refactor: consolidate getMemberExpressionRootName into shared getRootIdentifierName (Bugbot #153 round 3)
The new `getMemberExpressionRootName` I added in state-and-effects.ts
was identical to the imported `getRootIdentifierName` from helpers.ts
— exactly the kind of duplication Bugbot's prior round flagged on
`getMemberRootName`. Drop the second copy and use the shared one
everywhere.
All 489 tests pass; lint, typecheck, format clean.
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
The visitor scaffolding shared by no-prop-callback-in-effect and
no-derived-useState pushes an empty Set when entering a non-component
FunctionDeclaration / ArrowFunctionExpression. The intent (per the
existing comment) is to act as a barrier so identifiers inside the
helper don't resolve against an outer component's props.
The implementation walked the entire stack regardless, so a helper
nested inside a component would inherit the component's prop set and
produce false positives:
function Outer({ value, onChange }) { // outer props
function inner() {
const [draft, setDraft] = useState(value);
useEffect(() => {
onChange(isOn);
}, [isOn, onChange]);
}
inner();
}
The closed-over `value` and `onChange` are NOT props of `inner`;
they're lexically captured from `Outer`. Both rules incorrectly
flagged this shape.
Fix: stop the lookup walk at the first empty frame so the barrier
the visitor pushes actually behaves as one.
Two regression cases per rule (real prop → still flags; nested helper
→ no longer flags) live in tests/regressions/prop-stack-barrier.test.ts.
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Follow-up to #165. Addresses everything flagged during the post-merge audit.
1. Audit-mode asymmetry. `respectInlineDisables: false` was documented as
neutralizing every inline suppression, but `mergeAndFilterDiagnostics` ran
`filterInlineSuppressions` unconditionally, so `// react-doctor-disable*`
comments still hid diagnostics in audit mode. Thread the option through
`combineDiagnostics → mergeAndFilterDiagnostics` and skip the inline
filter when false. Config-level `ignore.rules`, `ignore.files`, and
`ignore.overrides` stay honored, matching the existing `.oxlintignore`
carve-out.
2. `ignore.overrides` config-shape footgun. Malformed entries (e.g.
`rules: "react/no-danger"` instead of an array) silently became
"ignore everything for these files". Validate each entry, emit stderr
warnings (mirroring `validate-config-types`), preserve `files` while
dropping malformed `rules`.
3. `--explain <file:line>` monorepo + ergonomics. Previously crashed in
monorepos and silently ignored `--project` / `--no-lint` /
`--no-dead-code`. New `findOwningProjectDirectory` walks workspace
packages and picks the longest-prefix match for the requested file;
`--project` is honored when set; resolved `ScanOptions` propagate.
Output is severity-aware (warnings yellow, errors red) and shows the
diagnostic's `category`.
4. TypeScript generic JSX components. `<List<Item>` followed by a
multi-line attribute list is now correctly recognized via an
`innerAngleDepth` counter — each `<` followed by an alpha char at
brace depth 0 increments, each matching `>` decrements, the OUTER `>`
closes the opener. Nested generic constraints
(`<Form<Schema<Item>>>`) work too.
5. Line-comment skipping in the JSX opener scanner.
`isOpenerMatchInsideLineComment` walks the line up to the regex match
position (string-aware) and rejects matches that come after `//`,
eliminating a false-positive class where a `disable-next-line` above
a `// docs example: <Foo>` line could extend coverage to a real
diagnostic below. Block-comment edge case remains a documented
limitation.
6. Single-pass suppression evaluator. `filterInlineSuppressions` used to
call both `isRuleSuppressedAt` and `classifySuppressionNearMiss` for
every surviving diagnostic, each independently looking up
`findEnclosingMultilineJsxOpenerStart` and
`findStackedDisableCommentsAbove`. New `evaluate-suppression.ts`
consolidates both into one pass; the original utility files remain
as thin one-line wrappers so external test imports keep working.
7. README polish. `--explain` section calls out monorepo
auto-resolution, the `--project` override, and steers users to
`--verbose` when they want hints across many sites.
`respectInlineDisables` documentation now lists
`// react-doctor-disable*` alongside the eslint / oxlint forms in
both prose and the config table.
Cleanup. `findOpenerTagOnLine` now uses `String.prototype.matchAll`
instead of `exec()` against a stateful module-scope `g`-flag regex.
`findChainSuppressor` was using `.find()` to retrieve an entry but
the caller only checked existence — renamed to `hasChainSuppressor`
and switched to `.some()`.
Tests: 17 new (527 total)
- find-jsx-opener-span: 7 added (line-comment containing fake `<Tag`,
single-generic component, nested generic constraints, generic +
self-close, generic + immediate close, string-attribute angle,
code-after-//-on-same-line)
- inline-suppressions regressions: 2 added (line-comment fake-opener
doesn't cause a false-positive suppression; generic-typed JSX
component attribute-line suppression works end-to-end)
- merge-and-filter-diagnostics: 3 (default respects, audit bypasses,
audit still honors config-level ignores)
- filter-diagnostics: 1 (stderr warning when overrides[i].rules is a
string)
- find-owning-project: 4 (single-project, monorepo dispatch, orphan
fallback, relative-path resolution)
Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Squashes work that closes#143 plus follow-on cleanup of the browser
surface and dead-code.
**`adoptExistingLintConfig` (default-on)** — when a project has a
JSON-format `.oxlintrc.json` or `.eslintrc.json` at the scanned
directory or any ancestor up to the nearest project boundary, that
config is folded into the same scan via oxlint's `extends` field.
Diagnostics from the user's existing rules count toward the 0–100
health score alongside the curated react-doctor rule set; if oxlint
can't load the user config, react-doctor logs the reason on stderr
and retries the scan once without `extends`. JS / TS configs are
silently skipped (oxlint's `extends` can't evaluate them). Coverage
broadened to `.ts` and `.js` files (previously the parser dropped
non-JSX). Closes#143.
**`react-doctor/eslint-plugin` flat-config export** — the same
react-doctor rule set is shipped as an ESLint flat-config plugin so
projects that already run ESLint can wire it up without depending on
the CLI. Includes presets (`recommended`, `next`, `react-native`,
`tanstack-start`, `tanstack-query`, `all`) plus cherry-picking.
**Removed browser entrypoints, browser CLI, and `react-doctor-browser`
package** — the in-browser diagnostics pipeline (`react-doctor/browser`
+ `/worker`), the `react-doctor browser …` CLI subcommand
(`start` / `stop` / `status` / `snapshot` / `screenshot` /
`playwright`), and the standalone `react-doctor-browser` workspace
package (Playwright + CDP + cookie extraction) are gone. Nothing in
the monorepo consumed them and they pulled in a heavy dep footprint
(`playwright`, `libsql`). Source is preserved on the
`archive/browser` branch.
**`diagnose-core` engine inlined into `index.ts::diagnose`** —
the dependency-injection shape was designed for browser sharing.
Only one caller now, so collapsed into a flat orchestrator. Score
helpers moved from `src/core/` to `src/utils/`; `src/core/` directory
removed.
**Smaller cleanups** — drop unused `matchGlobPattern` wrapper,
`isMemberProperty` import in nextjs rule, `fileContainsPattern`
helper, and various dead test locals; ignore `tests/fixtures/**`
from workspace lint and add `.oxlintignore` so `lint --fix` against
fixture paths can't strip the `debugger;` statements that
`adoptExistingLintConfig` tests assert on.
Tests: 452 / 452 pass on Node 22 + 24. Build smoke green.
Aiden Bai