Fix top React Doctor findings + representative migration samples#233
Open
Kelbie wants to merge 4 commits into
Open
Fix top React Doctor findings + representative migration samples#233Kelbie wants to merge 4 commits into
Kelbie wants to merge 4 commits into
Conversation
Address the highest-signal React Doctor findings and seed representative samples for the migration-scale rules (full sweeps deferred to follow-up). Full fixes: - Security/build-pipeline-secret-boundary: install with --ignore-scripts while the GitHub Packages token is in scope (it is only needed to auth the registry fetch), then run trusted dependency scripts (skia) and the first-party postinstall in a separate token-free step. Applied to both CI jobs. - Perf/no-json-parse-stringify-clone: structuredClone in the loggerChild test. Representative samples (recipe confirmed; remaining sites need sign-off): - react-compiler-no-manual-memoization: drop redundant useCallback in the home + notifications layouts, hoisting the closure-free handler to module scope. drawer/_layout.tsx:131 left intact: its React.memo is a load-bearing render-bailout for drawer animation timing (rule false positive). - set-state-in-effect: derive isChecking from permission in the camera hook. - js-combine-iterations: single-pass for...of in useAllSearchResults and pollParse. - no-giant-component: split PostComposer (405->300 lines) into Header, MediaTray and Toolbar sub-components, preserving the VisualLayoutProbe log taxonomy. Deferred: refs (timing-sensitive), plus the full sweeps of the above rules. Verified: react-doctor 2857->2847 with no new findings; type-check, knip, loggerChild test, and prettier (changed files) all pass.
React Compiler is enabled (app.json experiments.reactCompiler), so manual memo/useMemo/useCallback the compiler already covers is dead weight. An AST codemod unwraps ONLY React's memo/useMemo/useCallback (and removes the now-unused imports), across 125 files — 360 wrappers removed. Kept as load-bearing (verified against a fresh react-doctor run + cross-model review, NOT swept): - files where removal introduced a new compiler bailout (preserve-manual- memoization / refs / immutability / purity / no-effect-with-fresh-deps) - a side-effecting useMemo factory (NDKCacheAdapterSqlite in NostrNDKProvider) - memos with explicit human justification (stable prop identity, list-row render bailouts) The rn-no-non-native-navigator (x3) and js-length-check-first (x2) findings from the same react-doctor pass are confirmed false positives for this Expo Router setup and were intentionally left untouched: no native drawer exists in react-navigation v7, and the flagged array compares are an intentional prefix match and an already-guarded equality check. react-doctor total 2847 -> 2507; react-compiler-no-manual-memoization 1320 -> 960 (-360); all compiler-health rules flat. type-check, eslint (0 errors), and 1664 tests pass.
Audit of the repo-root config/dotfiles, cross-checked with a second model. Remove five files that no longer have a load path: - app-env.d.ts / nativewind-env.d.ts: redundant `uniwind/types` references (nativewind is no longer a dependency; the app migrated to uniwind). The reference is already provided by the uniwind-generated uniwind-types.d.ts, and both were byte-identical leftovers. - components.json / tailwind.config.js: shadcn CLI config plus the Tailwind JS config it pointed at. shadcn is not a dependency, Tailwind v4 does not auto-load a JS config without an `@config` directive (and global.css has none), and uniwind never reads it. The colors it defined already live in global.css `@theme`. - .DS_Store: macOS cruft (already gitignored, was untracked). Drop the now-dead "nativewind-env.d.ts" entry from tsconfig `include`; the `**/*.ts` glob still covers the remaining uniwind types. type-check passes; lint shows only pre-existing prettier warnings in unrelated files.
…mbine-iterations) Three genuine app-runtime list transforms over medium/large arrays — thread replies, a notifications page, and the swap-group timeline — rewritten from .map().filter() / .filter().map() to a single .flatMap() pass (the rule's own canonical fix). Identical semantics, comparable readability, one fewer traversal and intermediate allocation each. Scoped, cross-model-validated sample (me + Codex agreed): the remaining js-combine-iterations sites and ALL js-set-map-lookups sites are false-positive-dominated in this RN/Expo codebase and were left as-is — offline codereview/scripts tooling (not in the app bundle), string-receiver misfires (msg.includes / field.indexOf), and sub-10-item or already-memoized arrays where a second pass is negligible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the highest-signal React Doctor findings and seeds representative samples for the migration-scale rules. Full sweeps are deferred to follow-up pending sign-off — each sample confirms the canonical recipe holds first.
Verified against the real tool: react-doctor 2857 → 2847, with 0 newly-introduced findings.
type-check,knip, theloggerChildtest (19/19), and prettier (changed files) all pass.Full fixes
bun install --ignore-scriptswhile the GitHub Packages token is in scope (the token is only needed to auth the registry fetch, not to run lifecycle code), then run trusted dependency scripts (@shopify/react-native-skia) + the first-party postinstall in a separate token-free step.structuredCloneatloggerChild.test.ts:318.Representative samples (recipe confirmed; remaining sites need sign-off)
useCallbackin the home + notifications layouts, hoisting the closure-free handler to module scope.drawer/_layout.tsx:131left intact: itsReact.memois a load-bearing render-bailout for drawer animation timing — a rule false positive.isChecking = !permissionin the camera hook (1 verified-safe site; others would introduce stale-data bugs).for…ofinuseAllSearchResults+pollParse.PostComposer(405 → 300 lines) intoPostComposerHeader/MediaTray/Toolbar, preserving everyVisualLayoutProbelog-taxonomy string.Deferred
refs(timing-sensitive same-frame caches), plus the full sweeps of the four sampled rules.Not self-verified
The CI install behavior (
--ignore-scripts+ token-freebun pm trust --all && bun run postinstallfor skia + patches) can only be confirmed by an actual CI run — this CI is JS-only (lint/type-check/test), so it should be fine, but please watch the first run.