react

# React ## Contents - Rules - Review checklist - Review detail - Section 1: Project structure (bulletproof-react) - Section 2: Components & JSX - Section 3: Data fetching (in `references/data-fetching.md`) - Section 4: Forms (in `references/forms.md`) - Section 5: State - Section 6: Render cost and layout - Sweeps ## Rules Placement is covered by the `structure` skill, which defers to bulletproof-react. This file is everything else: how components, data access, forms and state are built. ### Components and markup - A component owns its own container. It never leaves a required wrapper to the caller, and never sets root `size`, `flexGrow` or self-margins that only work under one particular parent. - Conditional element assembly is a named local component with early returns, not a `let` reassigned through `if/else` and not a multi-line ternary building markup inline. - Three or more structurally identical sibling blocks that differ only by data become a map over an array, with stable keys. - No index as a key for anything reorderable, and never mix indices taken from a filtered list with an unfiltered one. - No side-effect-only component: a `useEffect` with `return null` is a hook or an HOC, not a component. - No `renderSomething()` inline render functions, and no skipping items by returning null from a ternary inside a map. Filter first. - Assign a hook result to a variable, then derive on the next line. Never inline a selector or subscription inside a transforming expression. - Do not reach for `useMemo` or `useCallback` by default when the project runs React Compiler. Write the plain value or function. - Without React Compiler, a `useMemo` or `useCallback` has to hit: empty dependencies mean a module constant, and a dependency that changes every render (an inline object, array or function) means the memo never does. Pass stable references to memoized children. - A block of markup past about thirty lines that reads as its own unit is a named component, and a wrapper element that only repeats its parent's styling is deleted. - On React 19 and later, `ref` is a plain prop. Do not add `forwardRef`, and never declare `ref` in the props of a component that is also wrapped in `forwardRef`. ### Data access - Reads and writes live in separate hooks. A component contains no raw `fetch`, no `axios`, no direct `useQuery` or `useMutation` call; grep how the sibling features do it and follow that. - The fetch lives with the actual consumer. A page does not duplicate a fetch its child already owns, especially with different params, and nothing fetches a whole list to derive one boolean. - After a write, patch the cache first, then update surgically, and invalidate only as a last resort. - Prefer `mutate` with callbacks over `mutateAsync` with `await` when the resolved value only drives a side effect. - A request's method matches its effect: a read uses GET unless its input cannot fit in a query string. - A cache patch or invalidation targets the narrowest key that shows the change. - A write hook touches only its own resource. Another resource's writes live in that resource's hook, and the caller composes the two. - A read hook wires up every option it accepts. Accepting an option and ignoring it is dead surface. ### Forms and state - The form is the single source of truth. No `useState` shadowing a value the form already holds. - A form-state hook returns form plumbing only (the methods, submit and discard, saving state). One field's fetching, watching and lookup state live in that field's component. - Each form watch lives in the smallest component that renders from it; a child reads shared form values itself rather than taking them as props. - A form that edits a record seeds the related record its inputs render from, not just the id. - Check the validation library's own API before hand-rolling a check, a regex, or a coercion. Most of them already exist there. - New state lives at the lowest common ancestor of its actual consumers, never lifted because a parent might want it later. - Two state updates whose order matters carry a one-line comment at the call site saying why. - A child that needs two or more props derived from one hook the parent called should call the hook itself. Detection criteria and per-box review failure modes live in the `## Review checklist` of the `react` skill (`conventions:react`), with the detail under its `## Review detail`. These rules are the statement of the convention; that checklist is how a diff gets graded against it. ## Review checklist The react gate agent ticks every box against the diff. A box is FAIL if any matching construct in the new code violates the rule; the gate is FAIL if any box is FAIL. Mark a box N/A only when the diff has no matching construct. The whole gate is N/A only when the diff contains no React/JSX UI code (state that). - [ ] §R1 Bulletproof-react placement: new feature code sits under `features/<feature>/{api,components,hooks,...}` (only needed subfolders); cross-feature-consumed code sits at the shared app level. (N/A: target repo doesn't use/isn't migrating to a feature-based layout — grep for `src/features/` first — or no files added/moved) - [ ] §R2 No cross-feature imports (`features/A` importing from `features/B`'s internals); composition happens at the app/routes level. (N/A: no feature-to-feature import in diff) - [ ] §R3 React keys/indices correct: no index key for reorderable items, no mixed filtered/unfiltered indices, no redundant `indexOf` after a `find`. (N/A: no keyed list in diff) - [ ] §R4 No vestigial aliases left over from a simplification. (N/A: none in diff) - [ ] §R5 No prop drilling / too-many-props where a child could read the same hook directly (2+ props derived from one context/hook the parent fetched). (N/A: no matching prop shape) - [ ] §R6 Repeated sibling JSX (3+ structurally identical blocks differing only by data) is mapped from an array, with a stable key. (N/A: fewer than 3 repeated blocks) - [ ] §R7 Hook-call assignments are pure: a subscription/selector result is assigned to a variable, then derived on the next line, never inlined inside a transforming expression. (N/A: no such hook call) - [ ] §R8 A layout component owns its own container (doesn't leave a required wrapper to the caller); no component assumes how its parent uses it (root props/margins that only work under one specific ancestor). (N/A: no such component added) - [ ] §R9 Conditional element assembly is a named local component with early returns, not a `let`-then-`if/else` block or a multi-line/non-trivial-branch ternary building JSX inline. (N/A: no conditional element-building in diff, or only a trivial single-line ternary / `{cond && <X/>}` guard) - [ ] §R10 No side-effect-only component (`useEffect` + `return null`) — blocker; no skip-via-ternary in map callbacks; no `renderXxx()` inline render-function pattern. (N/A: none of the three constructs in diff) - [ ] §R11 Reads and writes live in separate hooks; no raw `fetch`/`axios`/`useMutation`/`useQuery` call inside a component — grep how sibling features do it. (N/A: no data call in diff) - [ ] §R12 Fetch ownership re-evaluated for every read hook added/moved: the fetch lives with the actual consumer; a parent/page doesn't duplicate a fetch its child already owns (especially with different params); no full-list fetch added only to derive a boolean. (N/A: no read hook added/moved) - [ ] §R13 Query invalidation is a last resort (cache-patch → surgical update → invalidate, in that order); `mutate` + callbacks preferred over `mutateAsync` + `await` when the resolved value only drives a side effect; mechanical sweep — run `mutate-async.sh` over the diff's added lines, give every hit a verdict, state `grepped mutateAsync: 0 hits` explicitly when none found. (N/A: no invalidation or mutation in diff) - [ ] §R14 Form is the single source of truth (no `useState` shadowing a form-held value); hand-rolled validation checks/regex/coercions are confirmed against the validation library's own API before being kept. (N/A: no form or schema in diff) - [ ] §R15 New component state lives at the lowest common ancestor of its actual consumers, never lifted because a parent "might" want it; order-dependent multi-step state mutations (`update` then `remove`, `setX` then `setY` where order matters) carry a rationale comment at the call site. (N/A: no state added or edited in diff) - [ ] §R16 Every new `useMemo`/`useCallback` earns its place: empty deps with no input from component scope means a module constant (hoist it); a dependency that changes every render (an inline object, array, or function, a fresh instance) means the cache never hits (drop the memo or stabilize the dep); the finding states which. **Enumerate with the `memo-hooks.sh` sweep** (see §R16). (N/A: `memo-hooks.sh` returns 0 hits, stated as `grepped memo hooks: 0 hits`; or the project runs React Compiler, confirmed by grepping its build config) - [ ] §R17 No wasted render work: a memo, effect, or loop whose result is unused in the current mode is guarded out; an inline object or callback passed to a memoized child, or into a dependency array, is stabilized; dependency arrays list the underlying data, not a function reference whose identity changes; a value already memoized is not recomputed elsewhere. (N/A: no hook dependency array, memoized child, or mode-dependent computation in diff) - [ ] §R18 A JSX block past roughly 30 lines that reads as its own unit is a named component; no wrapper element that only repeats its parent's styling or adds nothing to layout. (N/A: no JSX block over 30 lines and no wrapper element added) - [ ] §R19 A change to width, height, min/max sizing, flex/grid layout, or positioning is checked at 375, 600, 768, and 1024 px: the usable width per column or cell after padding is computed or measured at each, and the numbers appear in the finding; removing a minimum-size floor without a responsive fallback FAILS. (N/A: no CSS or layout change in diff) - [ ] §R20 On React 19 or later, `ref` is a plain prop: no new `forwardRef`, and no props type declaring `ref` on a component also wrapped in `forwardRef` (the declaration is unreachable). **Enumerate**: grep the changed files for `forwardRef`, one verdict per hit; a `forwardRef` in an existing file the diff touches is a FAIL at nitpick weight. (N/A: the project's React version is below 19, confirmed from its manifest, or `grepped forwardRef: 0 hits`) - [ ] §R21 A request's method matches its effect: a new request that only reads and saves nothing uses GET; a read hook whose request is POST, PUT or PATCH FAILS unless the input genuinely cannot fit in a query string, and a comment at the request says so. (N/A: no new request) - [ ] §R22 A cache patch or invalidation targets the narrowest key that shows the change: an update to one record patches that record's own key when that is where it shows, and a broad base key is used only when lists showing the same record need it too, with a comment saying so. (N/A: no cache patch or invalidation added) - [ ] §R23 A write hook touches only its own resource: it imports no other resource's requests or hooks. A write to another resource lives in that resource's write hook, and the caller composes the two, updating its own cached data in the call's `onSuccess`. (N/A: no write hook added or edited) - [ ] §R24 A form-state hook returns form plumbing only (form methods, submit and discard, saving state). One that returns field data or field handlers (options, a lookup function, a pick handler) FAILS; that logic lives in the field's own component, or the form component when small. (N/A: no form added or edited) - [ ] §R25 Each form watch (and each piece of field state) lives in the smallest component that renders from it; a child reads shared form values through the form context instead of the parent watching them and passing them down. A watch in the form or its setup hook FAILS unless that component renders from the value too. (N/A: no watch or field state added in a form) - [ ] §R26 A form that edits an existing record seeds the related record its inputs render from (`owner_id` and `owner`), not just the id, and no picker gets a label prop hand-built from a record the form already holds. (N/A: no form editing an existing record) - [ ] §R27 A read hook accepts the repo's standard read-hook options (extended with its own fields, not a re-declared subset) and wires every option it accepts; an accepted option the hook never reads FAILS. (N/A: no read hook added or edited) ## Review detail Project-structure, component/JSX, data-fetching, form, and state-placement conventions for React/JSX UI code. Covers twenty-seven independent failure modes across six areas: where code lives (bulletproof-react), how components and markup are structured, how reads/writes are split and owned, how forms hold state, where new component state itself lives, and what renders cost and how layout holds up at narrow widths. --- ### Section 1 — Project structure (bulletproof-react) Reference: https://github.com/alan2207/bulletproof-react/blob/master/docs/project-structure.md #### §R1. Feature-based layout, shared code at the app level ``` src/ ├── components/, hooks/, utils/, types/, stores/, lib/, config/ (shared, cross-feature) └── features/<feature>/ ├── api/ - data hooks / API contract for this feature ├── components/ - feature-scoped components ├── hooks/ - feature-scoped hooks ├── stores/ - feature-specific state ├── types/ - feature-specific types └── utils/ - feature-specific utils ``` Only the subfolders a feature actually needs — don't scaffold empty ones. Code consumed by 2+ features is not feature-scoped; it belongs at the shared app level (`src/hooks/`, `src/utils/`, etc.), never duplicated into each feature. **Unidirectional flow: shared → features → app.** Shared code never imports from a feature — that inverts the dependency direction and makes "shared" a lie. `app`/route-level code may import from features and shared; features may import from shared; shared imports nothing above it. **Applicability guard (this box's N/A condition):** enforce this only when the target repo already follows, or is actively migrating to, a feature-based structure — grep for `src/features/` first. In a legacy flat layout (`src/components/`, `src/hooks/` at the top level, no `features/`), don't demand a bulletproof migration inside an unrelated feature PR; flag only a placement that makes the existing layout *more* inconsistent than it already is (e.g. a new resource split across two different conventions in the same PR). ``` // Flag: new feature file placed at the shared app level for no reason src/hooks/useWidgetPricing.ts // only WidgetPanel (in features/widgets) consumes it // Better: feature-scoped src/features/widgets/hooks/useWidgetPricing.ts ``` #### §R2. No cross-feature imports `features/A` importing a component, hook, or type from inside `features/B` couples two areas that should be independently deletable/replaceable. Compose at the app/routes level instead: the page/route file imports from both features and wires them together, or the shared piece moves to the app-level shared folder. ``` // Flag import { useOtherFeatureThing } from '@/features/otherFeature/hooks/useOtherFeatureThing' // Better: promote the shared piece, or compose one level up import { useSharedThing } from '@/hooks/useSharedThing' ``` **Durable fix:** suggest an ESLint `import/no-restricted-paths` rule (bulletproof-react's own recommendation) restricting `features/*` to importing only from its own folder and the shared app level — this catches the violation at commit time instead of relying on review. --- ### Section 2 — Components & JSX #### §R3. React keys and indices Index as a React `key` for items that can reorder/insert/delete. Mixing filtered and unfiltered indices in the same component. Re-deriving an index via `arr.indexOf(...)` after already iterating with one. If items have stable IDs, use them; if the form library assigns one (e.g. a field-array id), use the form library's stable id, not the array index. #### §R4. Vestigial aliases that survived a simplification When a const just renames another const with no behavior change (`const handleA = handleB`), it's usually a leftover from an earlier version where the alias was going to do more. Two failure modes: a pure alias (`const handleA = handleB`), and a multi-line delegator with no transformation (`const handleA = (...args) => handleB(...args)`). Either inline at the call site, or rename the call-site prop to match what the function actually is. If a comment explains a constraint that *eliminated* extra work, the alias itself probably has none left and is just an explanation in disguise. #### §R5. Prop drilling + too-many-props when a hook would do Not a hard rule — sometimes prop interfaces are the right call (testability, reuse outside a provider). But often a component receives 2+ props derived entirely from a single context/hook value the parent had to fetch on its behalf, purely to hand it down. Flag when: a child takes 2+ props all derived from one context/hook the parent called; the parent computes a value purely to pass down with no other local use; a grandparent threads a value through an intermediate component that never reads it. Suggest: the child reads from the same hook/context the parent read from (caches/contexts dedupe). Keep the prop interface for inputs that genuinely vary per call site. ```tsx // Flag const allowance = ctx?.a ?? other?.b ?? 0 return <CreditsCard allowance={allowance} balance={ctx?.balance ?? 0} /> // Better return <CreditsCard /> // reads useSomeContext() itself ``` **When NOT to flag:** pure primitives that vary per call site; a component intentionally kept context-free for reuse outside the provider; only one prop being forwarded (the indirection isn't worth removing). #### §R6. Repeated sibling JSX that differs only by data — map an array When **3+ sibling blocks** are structurally identical and differ only in the data they show, build an array of the varying data and `.map` it. ```tsx // Flag: 3 near-identical blocks, only label/value differ <Row><Label>{t('a')}</Label><Value>{a}</Value></Row> <Row><Label>{t('b')}</Label><Value>{b}</Value></Row> <Row><Label>{t('c')}</Label><Value>{c}</Value></Row> // Better const rows = [{ label: t('a'), value: a }, { label: t('b'), value: b }, { label: t('c'), value: c }] {rows.map(({ label, value }) => <Row key={label}><Label>{label}</Label><Value>{value}</Value></Row>)} ``` **Why it's better:** one source of truth for the row shape (restyling is one edit, not N); add/remove/reorder becomes a data edit, not a copy-paste that risks a wrong-value bug; `rows.map(...)` reads as intent ("N rows of the same kind") instead of forcing the reader to diff blocks; a single template can't silently drift the way repeated blocks do. Use a stable `key` (label or id), never the index. Threshold is **3+**; two explicit blocks are fine and not worth the indirection. Distinct from lookup-object dispatch (see the `structure` skill, §S5) — that's branch-selection of **one** value from a fixed set of cases; this is repeated **rendered siblings**. #### §R7. Keep hook-call assignments pure — derive on the next line Don't bury a subscription/selector hook call inside a larger expression that also transforms its result. Assign the hook's return to a named variable first, derive on the next line. ```tsx // Flag const hasItems = (useWatch({ control, name: 'items' }) ?? []).length > 0 // Better const items = useWatch({ control, name: 'items' }) const hasItems = Boolean((items ?? []).length) ``` Why: the subscription reads as one thing at a glance; the watched value becomes reusable instead of forcing a second subscription; it's directly inspectable/loggable. #### §R8. A layout component owns its container; no component assumes its parent **Owns-container:** a component that renders container-dependent children (grid cells, list rows) should wrap them in its own container itself, not leave the wrapper to the caller. ```tsx // Flag: bare items; layout breaks unless the parent remembers to wrap it const FieldGroup = () => <><Cell>...</Cell></> // Better: the component owns its container const FieldGroup = () => <Row>{/* Cell items */}</Row> ``` **No-parent-assumptions:** a component's root/output must be self-contained and render correctly under any parent. Flag any assumption baked in: positioning that needs a specific ancestor (a prop that only works under a specific ancestor container, e.g. a grid cell prop, `flexGrow`, `alignSelf`; `position: 'absolute'` needing a positioned ancestor); self-applied outer margins presuming sibling rhythm (spacing *between* children is the parent's `gap`/`spacing` to set); presence/placement decisions that are really the parent's call. ```tsx // Flag: root cell-prop only works if some ancestor is the matching container const Child = () => <GridCell size={12}>...</GridCell> // Better: child renders self-contained content; parent decides the slot const Child = () => <div>...</div> // parent: <GridCell size={12}><Child /></GridCell> ``` Rule of thumb: read only the component's own return, ignoring every caller — if you can't tell it will render correctly, it's assuming its context. This is the mirror of the owns-container rule above: a child must own its **internal** container, and must not own the **external** placement that is the parent's. #### §R9. Conditional element assembly → a named local component