improve v2 rasmus#217
Conversation
- Recategorize Tailwind-specific rules (size/padding shorthand, space-on-flex, default palette) from design-* to tailwind-* so they're ecosystem rules, not core React rules - Tighten js-length-check-first to require actual element-wise comparison and handle IfStatement/ConditionalExpression guards with branch-awareness - Broaden async-parallel test file skip pattern to cover __tests__/ dirs, e2e/integration files, mocks, and fixtures - Make preventDefault form message framework-agnostic (remove server action suggestion) - Add web-only workspace directory detection for RN rules (apps/web/, packages/web-app/, etc.) - Add 20 tests covering all changes
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fix score formula saturation: cap per-rule log amplification at 4x - Fix async-suspense-boundaries: skip page.tsx/layout.tsx/route.tsx files entirely - Fix .prettierignore regression: filter source extension globs (*.ts etc) that would exclude all source files (fixes refinedev/refine 0-issue regression) - Port --score, --full, --annotations CLI flags from v1 - Fix JSON stdout pollution from selectProjects in --json mode - Fix parity script: normalize v2 tailwind-* rule keys back to v1 design-* keys - Add 30 regression test repos to parity script - Deep-validate all outlier fixtures (react-router, astro, vite, immich, refine) - Update progress.md and PARITY_CHECKLIST.md with Phase 7-8 results Parity: 19 exact matches across 60 repos, mean |Δ| = 1.38, max |Δ| = 9 (astro/vite — non-React repos where v2 discovers more sub-projects)
The rule only accepted literal JSXElement as a valid single child for
asChild. A variable reference like {groupContent} wrapped in a
JSXExpressionContainer was incorrectly flagged even when it resolves
to a single element at runtime.
Now also accepts JSXExpressionContainer as a valid single-child pattern.
Verified: react-review accordion.tsx went from 4 FPs to 0.
…ybook) RN rules were firing on Docusaurus documentation pages in cross-platform libraries like downshift-js/downshift (which has react-native as a devDep for cross-platform support). Extended isWebOnlyPath to also skip docusaurus/, docs/, documentation/, website/, storybook/, stories/ dirs. Verified: downshift went from 66 rn-no-raw-text errors to 0.
getMeaningfulJsxChildren was counting JSX comments ({/* ... */}) as
meaningful children. A component like:
<Trigger asChild>
{/* comment */}
<span>{element}</span>
</Trigger>
was flagged as having 2 children when it really has 1.
Fixed by filtering out JSXExpressionContainer with JSXEmptyExpression
(the AST shape for JSX comments).
Verified: PostHog went from 54 radix-aschild FPs to 0.
React Native's AppState.addEventListener returns a subscription with a .remove() method. The cleanup pattern `return () => subscription.remove()` was not recognized because 'remove' wasn't in UNSUBSCRIPTION_METHOD_NAMES. Verified: TanStack/query useAppState.ts no longer falsely flagged.
Both rules produce noise on test/storybook files: - no-nested-component-definition: inline components in stories are standard practice - no-eval: eval() in tests to verify generated script output is legitimate Tagged with test-noise so the auto-suppression system drops them on test/storybook/stories files, matching v1's behaviour. Verified: eliminates 20 no-nested-component-definition and 16 no-eval FPs across 100 repos (ant-design, MUI, adobe/react-spectrum, etc).
Documenso uses share.$slug.opengraph.tsx (Remix route convention) for OG image generation with satori. The existing pattern only matched Next.js conventions (opengraph-image.tsx). Added .opengraph.[jt]sx? suffix matching. Verified: Documenso no-inline-exhaustive-style OG FPs eliminated.
testing-no-container-query: only fire in test files — was flagging production code using container.querySelector for DOM manipulation. js-set-map-lookups: skip String.includes() via suffix-based identifier detection (spanText, routeName, etc.) and skip inline array literals with <8 elements where Set overhead exceeds linear scan. no-uncontrolled-input: recognize onInput as valid control handler alongside onChange and readOnly — React's onInput fires identically to onChange. OG_IMAGE_FILE_PATTERN: match Remix .opengraph.tsx route convention in addition to Next.js opengraph-image.tsx. Verified across react-review, expect, react-scan, react-grab: - testing-no-container-query: 2 FPs eliminated - js-set-map-lookups: ~12 FPs eliminated - no-uncontrolled-input: 1 FP eliminated - no-inline-exhaustive-style on OG: 2 FPs eliminated
client-event-listeners: skip when the addEventListener call is at module scope (not inside a React component or custom hook). Module-level event registration is intentional setup code, not a per-instance leak. Uses ancestor traversal to find enclosing component/hook functions. js-request-idle-callback: skip server/CLI/test files where requestIdleCallback doesn't exist. Detects /server/, /cli/, /bin/, /scripts/, /workers/, /commands/, /api/ path segments. Verified: react-scan module-level init FPs eliminated, expect CLI analytics FPs eliminated.
|
✅ React Review — 87/100 (+87) · Copy prompt for agentReviewed by react-review for commit c9c3cc9. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![reactreview[bot]](https://avatars.githubusercontent.com/in/2506850?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const cliValue = getCliOptionOverride(command, optionName, flagValue); | ||
| if (cliValue !== undefined) return cliValue; | ||
| return configValue === undefined ? defaultValue : undefined; | ||
| }; |
There was a problem hiding this comment.
Config boolean values silently dropped as undefined
Medium Severity
resolveBooleanInspectOption returns undefined when configValue is defined (e.g., false), instead of returning the config value itself. The intent appears to be deferring to the SDK's config fallback, but if the SDK resolves options.lint ?? true without consulting options.config?.lint, a user's explicit "lint": false in their config file will be silently ignored and lint will run anyway. The old CLI resolved config values eagerly (e.g., userConfig?.lint ?? true), so this changes the contract.
Reviewed by Cursor Bugbot for commit 2b5310e. Configure here.
…ored resolveBooleanInspectOption previously returned undefined when the config provided a value, relying on the SDK's mergeConfig fallback to recover it. That coupled the CLI to internal SDK behavior — if mergeConfig ever stopped consulting options.config.lint, a user's explicit "lint": false would silently be ignored. Resolve the value eagerly to a concrete boolean so the contract is explicit at the call site.
| silentLogs: isQuiet, | ||
| config, | ||
| loadedConfig, | ||
| }; |
There was a problem hiding this comment.
Staged mode redirects scan away from temp directory
High Severity
When rootDir is set in the config and --staged mode is used, the loadedConfig object (with its original sourceDirectory) is passed through to inspectReactProjectCore. Inside that function, getLoadedConfig returns the passed loadedConfig directly, then resolveConfigRootDirectory resolves rootDir relative to the original sourceDirectory — not the temp directory. This redirects the scan from the staged snapshot back to the original working tree, defeating the purpose of materialized staged files entirely.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 00a7cc7. Configure here.
…napshot Passing the original loadedConfig into the SDK in staged mode caused resolveConfigRootDirectory to re-resolve rootDir against loadedConfig.sourceDirectory — the original working tree — which defeated the whole point of materializing staged files. Now we pass loadedConfig: null and a copy of config without rootDir, while still forwarding ignore/ignoredTags/textComponents/adoptExistingLintConfig through the config spread so they continue to apply.
| return null; | ||
| } | ||
| currentDirectory = parentDirectory; | ||
| } |
There was a problem hiding this comment.
Config walk skips parent when start directory is boundary
Medium Severity
In loadReactDoctorConfig, the walk loop checks isProjectBoundary(currentDirectory) after failing to find a config in currentDirectory. If the starting directory is itself a project boundary (e.g. has .git), the loop returns null immediately without ever checking the starting directory's package.json for a reactDoctor key. Wait — actually loadConfigFromDirectory IS called first. Let me re-read…
Actually on re-inspection this is correct — loadConfigFromDirectory is called first, and only if it returns null does the boundary check fire. Withdrawing this.
Reviewed by Cursor Bugbot for commit 6d8fb70. Configure here.
| process.stdout.write( | ||
| `${flags.jsonCompact ? JSON.stringify(report) : JSON.stringify(report, null, 2)}\n`, | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Inconsistent JSON schema between single and multi-project output
Medium Severity
When --json is used, single-project output uses buildReactDoctorJsonReport producing a flat schema with project (singular) and no projects array, while multi-project output uses toAggregateJsonReport producing a schema with projects (array). Consumers parsing the JSON cannot rely on a stable shape — the top-level keys differ depending on whether one or multiple projects are scanned. This breaks programmatic integrations that expect a consistent JSON contract.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6d8fb70. Configure here.
| flags.full || | ||
| isJsonMode || | ||
| isNonInteractiveEnvironment() || | ||
| !process.stdin.isTTY; |
There was a problem hiding this comment.
Score-only mode missing from prompt-skip condition
Medium Severity
The shouldSkipPrompts check includes isJsonMode but not isScoreOnly (flags.score). Since isQuiet is isScoreOnly || isJsonMode, the old CLI included score-only mode in prompt suppression. Now when --score is passed without --json, the user may be prompted interactively (e.g. project selection, diff mode), which breaks script consumption of the numeric-only stdout output.
Reviewed by Cursor Bugbot for commit 880d6f2. Configure here.
| console.log(highlighter.dim("No staged source files found.")); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Score-only mode not handled for staged empty files
Low Severity
When --staged finds no staged files, the early return handles isJsonMode and !isScoreOnly branches, but never outputs anything when isScoreOnly is true. A --score --staged invocation with no staged files exits silently with no output instead of printing the perfect score (100).
Reviewed by Cursor Bugbot for commit 880d6f2. Configure here.
… and scoring notes
| const includePathsFlags: CliFlags = | ||
| wantsDiffMode && !isDiffMode ? { ...effectiveFlags, diff: false } : effectiveFlags; | ||
| includePaths = resolveIncludePaths(scanRootDirectory, includePathsFlags); | ||
| } |
There was a problem hiding this comment.
Dead guard condition after early return from staged block
Low Severity
The else if (!effectiveFlags.staged) condition on the non-diff branch is always true at this point in the code. The staged mode block (starting around line 1350) unconditionally returns early, so execution only reaches this line when effectiveFlags.staged is false. The guard adds a misleading suggestion that the staged flag could still be active here.
Reviewed by Cursor Bugbot for commit 7797985. Configure here.
| .split("\0") | ||
| .map((filePath) => filePath.trim()) | ||
| .filter((filePath) => filePath.length > 0 && isSourceFile(filePath)); | ||
| }; |
There was a problem hiding this comment.
Missing maxBuffer in getGitFiles may silently truncate results
Low Severity
getGitFiles calls spawnSync without setting maxBuffer, defaulting to Node's 1 MB limit. The sibling function getStagedFilePaths in get-staged-files.ts correctly sets maxBuffer: GIT_SHOW_MAX_BUFFER_BYTES (50 MB). In repositories with a very large number of changed files, spawnSync will set result.error, causing getGitFiles to return []. This cascades into shouldSkipSourceChecks becoming true, silently skipping all source checks and producing a misleading perfect score with no user-facing warning.
Reviewed by Cursor Bugbot for commit 7797985. Configure here.
| startedAt: report.startedAt, | ||
| completedAt: report.completedAt, | ||
| durationMilliseconds: report.durationMilliseconds, | ||
| })), |
There was a problem hiding this comment.
Aggregate JSON report missing score/scoreLabel at project level
Medium Severity
In toAggregateJsonReport, each entry in the projects array copies summary from the per-project report, but the per-project ReactDoctorJsonReport type has summary which includes score and scoreLabel. The aggregate summary at the top level (lines 818-825) computes worstScore by searching results for a matching score.value, but worstScoreLabel looks up the label via results.find(...)?.score?.label. When all projects have the same worst score, the find returns the first match — that's fine. But when scores is empty (all projects scored null), worstScore becomes null and worstScoreLabel also becomes null, which is correct. The actual issue is subtler: durationMilliseconds at line 828 sums all project durations, but if projects ran in parallel via Promise.all (line 1595), this sum overstates wall-clock time. The single-project path via buildReactDoctorJsonReport reports accurate wall-clock duration per project, but the aggregate reports an inflated total. This makes the durationMilliseconds field misleading for multi-project JSON consumers comparing timing across runs.
Reviewed by Cursor Bugbot for commit 3abd2a0. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 17 total unresolved issues (including 16 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c9c3cc9. Configure here.
| const pending: string[] = [rootDirectory]; | ||
|
|
||
| while (pending.length > 0) { | ||
| const current = pending.shift(); |
There was a problem hiding this comment.
BFS directory walk uses O(n²) array shift
Low Severity
discoverReactProjectsByFilesystem uses pending.shift() inside a while loop for a BFS traversal. Array.shift() is O(n) because it re-indexes the entire array, making the overall directory walk O(n²) in the number of directories. For large monorepos with thousands of nested directories, this quadratic behavior can become noticeable. Using an index variable to track the current position (or a proper queue) would keep it O(n).
Reviewed by Cursor Bugbot for commit c9c3cc9. Configure here.
2 participants
Preview release
Try this PR without waiting for an npm release:
Updates automatically on every push. Pin to a specific commit with
react-doctor@<sha>(e.g.@840a6d5).Note
Medium Risk
Medium risk due to a major surface-area shift (package exports, CLI rewrite, scoring/annotation behavior) that can affect consumers and CI behavior, though changes are largely additive/compat-focused with documented opt-ins.
Overview
Moves
react-doctorinto a v2 SDK-first beta: the main export now points at the SDK, legacydiagnose()lives underreact-doctor/api(with v1 error classes re-exported for compatibility), and a newreact-doctor/scoresubpath is added while thebrowser-pocexport is removed.Reworks the CLI implementation and defaults: adds new changed-file modes (
--unstaged,--changed), improves staged scanning by materializing needed config files safely, skipsinfoissues in GitHub Actions annotations, and temporarily turns off codebase graph checks (dead-code/deps/architecture) by default unless explicitly enabled.Fixes categorization/false positives by routing oxlint diagnostics through
resolveOxlintDiagnosticCategory()and adjusting codebase analyzer behavior (e.g., treatingnew URL(..., import.meta.url)asset references and ensuringvite/nextjsconfig files are treated as support—not runtime—entrypoints). Adds a preview-release GitHub workflow, bumps package version to0.2.0-beta.1, trims/adjusts dependencies, and introduces atest:regressionscript that packs a tarball and drives sandbox tests via the siblingreact-reviewrepo.Reviewed by Cursor Bugbot for commit c9c3cc9. Bugbot is set up for automated code reviews on this repo. Configure here.