Skip to content

Commit 7bc7cea

Browse files
Refine reviewer rules based on automated tracker feedback
- CLEAN-REACT-PATTERNS-4: correct the React Compiler memoization claim and scope firing to helpers that contain hooks/side-effects/business logic, build deeply nested JSX inline, or close over mutable parent state. Add carve-outs for switch/conditional delegation helpers and .map() over already-extracted child components. - PERF-9: require the intermediate state to have no render-driving role. Add carve-out for state consumed in JSX for conditional rendering, prop pass-through to a rendered child, or gating a subtree. - CLEAN-REACT-PATTERNS-1: add a Case 3 carve-out for structurally detectable coordination props (same state setter or callback consumed by two or more sibling children).
1 parent 6a4bf92 commit 7bc7cea

3 files changed

Lines changed: 52 additions & 27 deletions

File tree

.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ Flag when ANY of these are true:
436436
- A component receives a large set of props that collectively configure its appearance and behavior (e.g., dialog with `isOpen`, `title`, `tabs`, `activeTab`, `onTabChange`, `onSave`, `onReset`, `values`)
437437
- These props could be broken into independent composable blocks: Provider manages state, sub-components render independently
438438
- The component becomes a "configuration object consumer" rather than a composition of building blocks
439+
- When counting props for this case, exclude **coordination props** that match the Case 3 carve-out below (state setters or callbacks structurally shared across multiple sibling children). Count only props that configure the component's own appearance or behavior. If the remaining prop count no longer reads as monolithic, do not flag.
439440

440441
**Case 4 — Config-array driven rendering:**
441442
- Statically-known items (finite, fixed at development time) are encoded as a data array of config objects
@@ -468,6 +469,10 @@ In all cases, the rule applies to: **new components**, **new features added to e
468469
- The `ReactNode` prop is `children` itself — `children` is the foundation of composition, not configuration
469470
- The render function is a **list component callback** (`renderItem` on `FlatList`, `DraggableList`) — these are framework requirements
470471
- The render function receives **per-item runtime data** from a dynamic collection (e.g., `renderSuggestionMenuItem(item, index)`) — this is list-style rendering, not slot configuration
472+
- **(Case 3 only)** Props exist to **coordinate shared state or callbacks across sibling children** whose state must live in the common parent. This is legitimate state lifting, not configuration. The coordination role must be **structurally detectable** in the diff or surrounding code — apply ONLY when ONE of these is true:
473+
- The **same state setter** (e.g., `setSelectedId`) is passed to two or more sibling children, OR
474+
- The **same callback** (e.g., `onItemPress`) is consumed by two or more sibling children
475+
A prop passed to a single child, or a prop merely described as "shared" without a visible second consumer, does NOT qualify — this qualifier exists to prevent the carve-out from becoming an argument-shaped escape hatch for any prop.
471476

472477
**Search Patterns** (hints for reviewers):
473478
- **Case 1**: `should\w+`, `can\w+`, `enable`, `disable` (boolean flag prefixes in prop types)

.claude/skills/coding-standards/rules/clean-react-4-no-side-effect-spaghetti.md

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -93,36 +93,50 @@ In this example:
9393

9494
#### Incorrect (internal render helper functions)
9595

96-
- Internal `render*` functions signal the component owns multiple rendering responsibilities that should be separate components
97-
- They close over the entire component scope — the React Compiler cannot independently memoize them
98-
- They hide the component's render tree — the return statement doesn't show what actually renders
99-
- The fix is to extract each helper into its own component with explicit props
96+
- Internal render helpers become a problem when they carry hooks, side effects, or non-trivial business logic that belong in a dedicated component
97+
- They become a problem when they build deeply nested JSX inline, hiding the real render tree from the return statement
98+
- They become a problem when they close over mutable parent state (e.g., values from `useState`/`useReducer`) and bake that state into the JSX they produce
99+
- React Compiler handles thin delegation wrappers (e.g., a function that picks a child component based on a prop) without issue — the rule targets helpers that are doing real work, not ones that just route to already-extracted children
100+
- The fix for the problematic cases is to extract the helper into its own component with explicit props
100101

101102
```tsx
102-
function ForYouSection() {
103-
const theme = useTheme();
103+
function ReportDetailsPage({report, policy}: Props) {
104+
const [draftNote, setDraftNote] = useState('');
104105
const {translate} = useLocalize();
105-
const {submitCount, approveCount} = useTodos();
106-
107-
// ❌ Internal render helper — closes over everything, hides structure
108-
const renderTodoItems = () => (
109-
<View style={styles.todoContainer}>
110-
{todoItems.map(({key, icon, ...rest}) => (
111-
<BaseWidgetItem key={key} icon={icon} {...rest} />
112-
))}
113-
</View>
114-
);
115106

116-
// ❌ Another render helper — branching logic buried in a function
117-
const renderContent = () => {
118-
if (isLoadingApp) {
119-
return <ActivityIndicator size="large" />;
120-
}
121-
return hasAnyTodos ? renderTodoItems() : <EmptyState />;
107+
// ❌ Heavy internal helper — closes over mutable state, runs business logic,
108+
// and builds deeply nested JSX inline. This is what the rule targets.
109+
const renderNoteSection = () => {
110+
const trimmed = draftNote.trim();
111+
const isValid = trimmed.length > 0 && trimmed.length <= CONST.NOTE.MAX_LENGTH;
112+
const participants = ReportUtils.getParticipantsList(report, policy);
113+
const canEdit = ReportUtils.canEditReportNote(report, participants);
114+
115+
return (
116+
<View style={styles.section}>
117+
<View style={styles.sectionHeader}>
118+
<Text style={styles.sectionTitle}>{translate('notes.title')}</Text>
119+
{canEdit && (
120+
<View style={styles.sectionActions}>
121+
<Button
122+
text={translate('common.save')}
123+
isDisabled={!isValid}
124+
onPress={() => Report.saveNote(report.reportID, trimmed)}
125+
/>
126+
</View>
127+
)}
128+
</View>
129+
<TextInput
130+
value={draftNote}
131+
onChangeText={setDraftNote}
132+
editable={canEdit}
133+
multiline
134+
/>
135+
</View>
136+
);
122137
};
123138

124-
// The return statement hides the actual render tree
125-
return <WidgetContainer>{renderContent()}</WidgetContainer>;
139+
return <ScreenWrapper>{renderNoteSection()}</ScreenWrapper>;
126140
}
127141
```
128142

@@ -189,9 +203,9 @@ Flag when a component, hook, or utility aggregates multiple unrelated responsibi
189203
- Unrelated state variables are interdependent or updated together
190204
- Logic mixes data fetching, navigation, UI state, and lifecycle behavior in one place
191205
- Removing one piece of functionality requires careful untangling from others
192-
- Component defines internal `render*` functions or arrow functions that return JSX and calls them in its return statement (e.g., `const renderContent = () => ...`, `{renderContent()}`)
193-
- These functions close over the component's entire scope, preventing the React Compiler from independently memoizing them
194-
- The component's return statement calls these helpers instead of showing the render tree directly
206+
- An internal function that returns JSX contains hooks, side effects, or non-trivial business logic (data shaping, permission checks, validation, etc.) that should live in its own component
207+
- An internal function that returns JSX builds deeply nested JSX inline, obscuring the component's render tree
208+
- An internal function that returns JSX closes over mutable parent state (e.g., values from `useState`/`useReducer`) and embeds that state in the JSX it produces
195209

196210
**What counts as "unrelated":**
197211
- Group by responsibility (what the code does), NOT by timing (when it runs)
@@ -203,6 +217,8 @@ Flag when a component, hook, or utility aggregates multiple unrelated responsibi
203217
- Effects are extracted into focused custom hooks with single responsibilities (e.g., `useDebugShortcut`, `usePriorityMode`) — inline `useEffect` calls are a code smell and should be named hooks
204218
- The internal function is a **callback or event handler** (e.g., `handlePress`, `onSubmit`), not a render helper — only functions that return JSX qualify
205219
- The internal function is a **single early return** for a guard clause (e.g., `if (!data) return <EmptyState />;` at the top of the component) — simple guards in the component body are not render helpers
220+
- The internal function is a **switch/conditional helper that delegates to already-extracted child components** (e.g., `const renderItem = () => type === 'foo' ? <FooItem /> : <BarItem />`) — React Compiler memoizes these thin wrappers correctly
221+
- The internal function (or inline `.map()` callback) **iterates over data and renders an already-extracted child component** per item (e.g., `items.map((item) => <Row key={item.id} item={item} />)`) — the per-item work happens inside the extracted child
206222

207223
**Search Patterns** (hints for reviewers):
208224
- `useEffect`

.claude/skills/coding-standards/rules/perf-9-no-useeffect-chains.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ title: Avoid useEffect chains
99

1010
Chains of effects create complex dependencies, timing issues, and unnecessary renders. Logic should be restructured to avoid interdependent effects.
1111

12+
Chains are wasteful only when the bridge state has no observable render role. If the intermediate state drives UI - conditional rendering, props passed to a rendered child, or gating a subtree - the re-render is the purpose of the state, not waste, and PERF-9 does not apply.
13+
1214
### Incorrect
1315

1416
```tsx
@@ -58,10 +60,12 @@ Flag ONLY when ALL of these are true:
5860

5961
- Multiple `useEffect` hooks exist
6062
- One effect's state update triggers another effect
63+
- The intermediate state has no render-driving role - it is not referenced in JSX for conditional rendering, prop pass-through to a rendered child, or gating a subtree
6164
- Logic could be combined or computed instead
6265

6366
**DO NOT flag if:**
6467

68+
- The intermediate state is consumed as a render-driving dependency in JSX - e.g., conditional rendering (`{state && <X />}`), prop pass-through to a child the user observes, or gating an entire subtree. In those cases the re-render is the purpose of the state, not waste.
6569
- Effects handle different concerns (e.g., one for setup, one for event listening)
6670
- Effects depend on external events that can't be combined
6771
- Separation of concerns requires multiple effects

0 commit comments

Comments
 (0)