From 8b1c38815c2fcdbb814865c82ace38acaedcc443 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 16 Feb 2026 15:13:49 +0100 Subject: [PATCH 1/6] Apply PR #82443 reviewer feedback to CLEAN-REACT-PATTERNS-1 rule - Broaden Case 1 search patterns to should\w+ and can\w+ (VickyStash) - Generalize Case 1 condition to reference search patterns (VickyStash) - Remove onChange from Case 3 monolithic prop example (VickyStash) - Add DO NOT flag for thin platform primitive wrappers (TMisiukiewicz) Co-Authored-By: Claude Opus 4.6 --- .../clean-react-1-composition-over-config.md | 277 ++++++++++++------ 1 file changed, 186 insertions(+), 91 deletions(-) diff --git a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md index d059dc4bdbc7..d8b2ab8d185c 100644 --- a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md +++ b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md @@ -7,14 +7,16 @@ title: Favor composition over configuration ### Reasoning -When new features are implemented by adding configuration (props, flags, conditional logic) to existing components, if requirements change, then those components must be repeatedly modified, increasing coupling, surface area, and regression risk. Composition ensures features scale horizontally, limits the scope of changes, and prevents components from becoming configuration-driven "mega components". +When features are implemented by adding configuration to components — whether boolean flags, optional content props, or large prop interfaces — the component must be modified every time a consumer needs different behavior. This increases coupling, surface area, and regression risk at scale. Composition treats features as independent building blocks: a Provider manages shared state, sub-components (blocks) render independently via context or direct props, and consumers add/remove features by including or excluding blocks. The component never changes. This applies equally to simple widgets and complex multi-feature UIs. + +Reference: [Composition Pattern Guide](https://composition-pattern-starter.vercel.app/comparison) ### Incorrect -#### Incorrect (configuration) +#### Incorrect (configuration — boolean flags) - Features controlled by boolean flags -- Adding a new feature requires modifying the Table component's API +- Adding a new feature requires modifying the component's API and internals ```tsx -type SelectionListProps = { - shouldShowTextInput?: boolean; // Could be - shouldShowConfirmButton?: boolean; // Could be - textInputOptions?: {...}; // Configuration object for the above -}; +// Inside the component — conditional rendering controlled by props: +function BaseWidgetItem({icon, iconBackgroundColor, title, subtitle, ctaText, onCtaPress, iconFill}: BaseWidgetItemProps) { + return ( + + + + + + {!!subtitle && {subtitle}} // ❌ Prop exists solely for this conditional + {title} + +
``` -```tsx - - - - -``` +#### Correct (composition — compound component) -#### Correct (children manage their own state) +- UI elements are composable children the consumer includes or omits +- Adding a new element (e.g., Subtitle, Badge) never changes existing sub-components +- Each sub-component is a small, focused function with its own styles ```tsx -// Children are self-contained and manage their own state -// Parent only passes minimal data (IDs) -// Adding new features doesn't require changing the parent -function ReportScreen({ params: { reportID }}) { - return ( - <> - - // other features - - - ); +// Implementation — each sub-component owns its behavior: + +function Container({children, onPress, accessibilityLabel}: {children: React.ReactNode; onPress?: () => void; accessibilityLabel?: string}) { + const styles = useThemeStyles(); + const content = {children}; + + if (onPress) { + return ( + + {content} + + ); + } + return content; +} + +function WidgetIcon({src, backgroundColor, fill}: {src: IconAsset; backgroundColor: string; fill?: string}) { + const styles = useThemeStyles(); + const theme = useTheme(); + return ( + + + + ); +} + +function Content({children}: {children: React.ReactNode}) { + const styles = useThemeStyles(); + return {children}; +} + +function Title({children, numberOfLines}: {children: React.ReactNode; numberOfLines?: number}) { + const styles = useThemeStyles(); + return {children}; } -// Component accesses stores and calculates its own state -// Parent doesn't know the internals -function ReportActionsView({ reportID }) { - const [reportOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`); - const reportActions = getFilteredReportActionsForReportView(unfilteredReportActions); - // ... +function Subtitle({children}: {children: React.ReactNode}) { + const styles = useThemeStyles(); + return {children}; } + +function Action({onPress, isLoading, children}: {onPress: () => void; isLoading?: boolean; children: string}) { + const styles = useThemeStyles(); + return + + + + + + + {/* Form content */} + + + + + + + + + +// No props between blocks — context manages everything +// Adding a new section doesn't change the Provider or other blocks ``` --- @@ -144,28 +226,41 @@ function ReportActionsView({ reportID }) { #### Condition -Flag ONLY when ALL of these are true: +Flag when ANY of these are true: + +**Case 1 — Boolean flag configuration:** +- A component uses boolean/flag props (matching the Case 1 search patterns) that cause `if/else` or ternary branching inside the component body +- These flags control feature presence, layout strategy, or behavior within the component +- These features could instead be expressed as composable child components + +**Case 2 — Content prop configuration:** +- An optional content prop's **sole purpose** is to conditionally render a UI element +- The test: if removing the prop would only remove a `{!!prop && }` or `{prop ? : null}` block and nothing else, the element should be a composable child instead +- This applies when **adding new optional content props** to new or existing components — not when modifying existing conditional rendering during a bug fix + +**Detection steps for Case 2:** +1. In the diff, search for conditional rendering patterns: `{!!prop &&`, `{prop &&`, `{prop ? <...> : null}` +2. For each match, identify the variable used in the condition (e.g., `subtitle` in `{!!subtitle && ...`) +3. Check the component's type definition — is this prop optional (`subtitle?: string`)? +4. Search the entire component body for other uses of this prop — is the conditional render the ONLY place it appears? +5. If yes to both (optional + sole purpose is conditional render) → flag as Case 2 violation -- Any of these scenarios apply: - - A **new feature** is being introduced - - An **existing component's API** is being expanded with new props - - A **refactoring** creates a new component that still has boolean configuration props matching the search patterns controlling branching logic — refactoring is an opportunity to eliminate configuration flags, not preserve them -- The component contains boolean props matching the search patterns that cause `if/else` or ternary branching inside the component body -- These configuration options control feature presence, layout strategy, or behavior within the component +**Case 3 — Monolithic prop interface:** +- 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`) +- These props could be broken into independent composable blocks: Provider manages state, sub-components render independently +- The component becomes a "configuration object consumer" rather than a composition of building blocks -**Features that should NOT be controlled by boolean flags:** -- Optional UI elements that could be composed in -- New behavior that could be introduced as new children -- Features that currently require parent component code changes -- Layout strategy variants +In all cases, the rule applies to: **new components**, **new features added to existing components**, and **refactorings that create new components still following configuration patterns** **DO NOT flag if:** -- Props are non-boolean data values needed for coordination between composed parts (e.g., `reportID`, `data`, `columns`). -- The component uses composition and child components for features -- Parent components stay stable as features are added +- Props are domain identifiers used for data fetching (e.g., `reportID`, `policyID`, `transactionID`) +- Props are event handlers for abstract actions (e.g., `onPress`, `onChange`, `onSelectRow`) +- Props are structural/presentational (e.g., `style`, `testID`) +- The component already uses composition and child components for features +- The optional prop is used for logic beyond just conditional rendering (e.g., computing derived values, passed to callbacks, used in multiple places within the component) +- The component is a thin wrapper around a platform primitive (e.g., wrapping `TextInput`, `ScrollView`, `Pressable`) — these naturally pass through configuration props **Search Patterns** (hints for reviewers): -- `should\w+` (any prop starting with `should`) -- `canSelect` -- `enable` -- `disable` +- **Case 1**: `should\w+`, `can\w+`, `enable`, `disable` (boolean flag prefixes in prop types) +- **Case 2**: `{!!`, `&&\s*<`, `?\s*<`, `: null`, combined with optional prop markers (`?:` in type definitions) +- **Case 3**: Components with 8+ props in their type definition, especially mixing state/handler/content props From 61f254e52b86d99a06c2d937a195a259372f7440 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Tue, 17 Feb 2026 17:08:04 +0100 Subject: [PATCH 2/6] Clarify presentational prop exception in CLEAN-REACT-PATTERNS-1 Explicitly note that booleans selecting between layout/styling strategies on focused components (e.g., shouldUseAspectRatio) fall under the existing presentational prop exception and should not be flagged as composition-over-configuration violations. Co-Authored-By: Claude Opus 4.6 --- .../rules/clean-react-1-composition-over-config.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md index d8b2ab8d185c..b806250aba59 100644 --- a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md +++ b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md @@ -255,7 +255,7 @@ In all cases, the rule applies to: **new components**, **new features added to e **DO NOT flag if:** - Props are domain identifiers used for data fetching (e.g., `reportID`, `policyID`, `transactionID`) - Props are event handlers for abstract actions (e.g., `onPress`, `onChange`, `onSelectRow`) -- Props are structural/presentational (e.g., `style`, `testID`) +- Props are structural/presentational (e.g., `style`, `testID`) — this includes booleans that select between layout or styling strategies on a focused component (e.g., `shouldUseAspectRatio` toggling between fixed-height and aspect-ratio styles) - The component already uses composition and child components for features - The optional prop is used for logic beyond just conditional rendering (e.g., computing derived values, passed to callbacks, used in multiple places within the component) - The component is a thin wrapper around a platform primitive (e.g., wrapping `TextInput`, `ScrollView`, `Pressable`) — these naturally pass through configuration props From c027b645e8662729f3a8b0820c8cd42926a737eb Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Tue, 17 Feb 2026 18:04:10 +0100 Subject: [PATCH 3/6] Add Case 4 (config-array driven rendering) to CLEAN-REACT-PATTERNS-1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds detection for statically-known items encoded as data arrays and .map()'d through generic components — a pattern missed during PR #80829 review. Includes incorrect/correct examples, review metadata conditions, DO NOT flag exceptions for runtime data and list components, and search patterns. Co-Authored-By: Claude Opus 4.6 --- .../clean-react-1-composition-over-config.md | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md index b806250aba59..4e483828762e 100644 --- a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md +++ b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md @@ -97,6 +97,46 @@ function BaseWidgetItem({icon, iconBackgroundColor, title, subtitle, ctaText, on /> ``` +#### Incorrect (configuration — config-array driven rendering) + +- Statically-known items encoded as data instead of declared as JSX +- The generic component must handle every possible config shape +- Business logic leaks into data declarations (filters, conditionals) +- Adding behavior means expanding the config schema, not adding a component + +```tsx +// A) Basic: statically-known actions encoded as a config array + +``` + +```tsx +// B) With embedded conditionals (worse): business logic mixed into data declarations +const todoItems = [ + { + key: 'submit', + count: submitCount, + icon: Send, + translationKey: '...', + handler: handleSubmit, + }, + { + key: 'approve', + count: approveCount, + icon: ThumbsUp, + translationKey: '...', + handler: handleApprove, + }, +].filter((item) => item.count > 0); + +{todoItems.map(({key, icon, ...rest}) => ( + +))} +``` + ### Correct #### Correct (composition — boolean features) @@ -220,6 +260,37 @@ export default WidgetItem; // Adding a new section doesn't change the Provider or other blocks ``` +#### Correct (composition — declarative JSX over config arrays) + +- Each item is explicit JSX — visible in the component tree +- Conditional rendering is standard JSX (`{count > 0 && ...}`), not data-level filtering +- No generic component needs to interpret a config schema +- Adding/removing items means adding/removing JSX, not expanding a data structure + +```tsx +// Each item declared as JSX — self-contained, type-safe, independently modifiable + + {submitCount > 0 && ( + + + + {translate('homePage.forYouSection.submit', {count: submitCount})} + + {beginText} + + )} + {approveCount > 0 && ( + + + + {translate('homePage.forYouSection.approve', {count: approveCount})} + + {beginText} + + )} + +``` + --- ### Review Metadata @@ -250,6 +321,12 @@ Flag when ANY of these are true: - These props could be broken into independent composable blocks: Provider manages state, sub-components render independently - The component becomes a "configuration object consumer" rather than a composition of building blocks +**Case 4 — Config-array driven rendering:** +- Statically-known items (finite, fixed at development time) are encoded as a data array of config objects +- The array is `.map()`'d through a generic component to produce JSX +- Each item has distinct behavior or props that could be expressed as individual JSX elements or dedicated components +- Business logic is mixed into the array (conditional entries via `&&`, `.filter()`) + In all cases, the rule applies to: **new components**, **new features added to existing components**, and **refactorings that create new components still following configuration patterns** **DO NOT flag if:** @@ -259,8 +336,13 @@ In all cases, the rule applies to: **new components**, **new features added to e - The component already uses composition and child components for features - The optional prop is used for logic beyond just conditional rendering (e.g., computing derived values, passed to callbacks, used in multiple places within the component) - The component is a thin wrapper around a platform primitive (e.g., wrapping `TextInput`, `ScrollView`, `Pressable`) — these naturally pass through configuration props +- Items come from **runtime data** (API responses, user-generated content, Onyx collections) — dynamic data must be mapped +- The array is used with **list components** (e.g., `FlatList`, `SectionList`, or custom wrappers) — these require data arrays by design +- Items are truly **homogeneous** (same shape, same behavior, only values differ) and the count is **unbounded** (e.g., list of chat messages, search results) +- The array is a **framework requirement** (e.g., React Navigation screen config, form validation rules) **Search Patterns** (hints for reviewers): - **Case 1**: `should\w+`, `can\w+`, `enable`, `disable` (boolean flag prefixes in prop types) - **Case 2**: `{!!`, `&&\s*<`, `?\s*<`, `: null`, combined with optional prop markers (`?:` in type definitions) - **Case 3**: Components with 8+ props in their type definition, especially mixing state/handler/content props +- **Case 4**: `\.map\(` combined with array literal `= \[` in the same component; `actions={[`, `items={[`, `options={[` (config arrays passed as props); `.filter(` on array literals (conditional items) From aee01c45421099a83063cf1554e7130f7e746ec6 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 18 Feb 2026 11:43:04 +0100 Subject: [PATCH 4/6] Add Cases 5-6, close loopholes in CLEAN-REACT-PATTERNS-1 - Add ReactNode slot props as Case 2 variant (leftComponent, rightComponent, etc.) - Add Case 5: render function props (renderTitle, renderSubtitle, etc.) - Add correct examples: compound component slots, Suspense over isLoading - Fix Case 2 detection step 4: test "only for conditional rendering" not "only one place" - Close Case 4 cosmetic compliance loophole (compound components inside .map) - Replace Case 3 numeric threshold with qualitative guideline - Add DO NOT flag exceptions for children, list callbacks, per-item runtime data - Add search patterns for Case 5 Co-Authored-By: Claude Opus 4.6 --- .../clean-react-1-composition-over-config.md | 133 +++++++++++++++++- 1 file changed, 130 insertions(+), 3 deletions(-) diff --git a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md index 4e483828762e..17c0d1f9f93c 100644 --- a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md +++ b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md @@ -74,6 +74,41 @@ function BaseWidgetItem({icon, iconBackgroundColor, title, subtitle, ctaText, on } ``` +#### Incorrect (configuration — ReactNode slot props) + +- Passing JSX via named props is still configuration — the component must know about each slot +- Adding a new slot (e.g., `badgeComponent`) requires modifying the component's props AND internals +- Each slot prop often drags along associated style/behavior props (`leftComponentStyle`, `shouldShowRightComponent`) +- The consumer can't reorder, wrap, or compose slots — the component controls layout + +```tsx +} + rightComponent={} + furtherDetailsComponent={} +/> + +type MenuItemProps = { + title: string; + leftComponent?: ReactNode; // Positional slot configured via prop + rightComponent?: ReactNode; // Another positional slot + furtherDetailsComponent?: ReactElement; // Yet another slot +}; + +// Inside the component — each slot is a conditional render: +function MenuItem({title, leftComponent, rightComponent, furtherDetailsComponent}: MenuItemProps) { + return ( + + {!!leftComponent && {leftComponent}} + {title} + {!!rightComponent && rightComponent} + {!!furtherDetailsComponent && {furtherDetailsComponent}} + + ); +} +``` + #### Incorrect (configuration — monolithic prop interface) - All features threaded through props @@ -137,6 +172,38 @@ const todoItems = [ ))} ``` +#### Incorrect (configuration — render function props) + +- Render functions are configuration disguised as flexibility — the component still owns each slot +- Each `render*` prop is an alternative to a composable child component +- The component must call each function and manage the fallback logic +- Adding a new renderable area means adding a new `render*` prop to the interface + +```tsx +
( + + Advanced features for your workspace + + + )} + overlayContent={() => isLoading && } +/> + +// Inside the component — render functions called inline: +function Section({title, renderSubtitle, renderTitle, overlayContent}: SectionProps) { + return ( + + {renderTitle ? renderTitle() : {title}} + {renderSubtitle ? renderSubtitle() : null} + {children} + {overlayContent?.()} + + ); +} +``` + ### Correct #### Correct (composition — boolean features) @@ -291,6 +358,53 @@ export default WidgetItem; ``` +#### Correct (composition — compound component slots over ReactNode props) + +- Each slot is a composable child — visible in the JSX tree, independently testable +- Adding a new slot (e.g., ``) never changes existing sub-components +- No associated style props needed — each sub-component owns its own styling +- The consumer has full control over composition, ordering, and conditional rendering + +```tsx +// Consumer controls what appears and where — each slot is explicit JSX + + + + + + Settings + Manage your preferences + + + + + +``` + +#### Correct (composition — children slots over render functions) + +- Each area declared as JSX — the consumer decides what renders, not the component +- Conditional rendering is standard JSX, not function call fallback chains +- Adding a new area means creating a new sub-component, not expanding the parent's prop interface +- Each sub-component is an independent render unit — better for React Compiler memoization + +```tsx +// Each area is a composable child — no render functions needed +
+ Features + + Advanced features for your workspace + + + + {children} + + }> + + +
+``` + --- ### Review Metadata @@ -308,12 +422,14 @@ Flag when ANY of these are true: - An optional content prop's **sole purpose** is to conditionally render a UI element - The test: if removing the prop would only remove a `{!!prop && }` or `{prop ? : null}` block and nothing else, the element should be a composable child instead - This applies when **adding new optional content props** to new or existing components — not when modifying existing conditional rendering during a bug fix +- A named `ReactNode` or `ReactElement` prop whose purpose is to render UI in a specific position within the component (e.g., `leftComponent?: ReactNode`, `footerContent?: ReactNode`, `titleComponent?: ReactElement`) +- The test: the component wraps the prop in a conditional render (`{!!prop && {prop}}`) or renders it directly — the slot could instead be a compound component child **Detection steps for Case 2:** 1. In the diff, search for conditional rendering patterns: `{!!prop &&`, `{prop &&`, `{prop ? <...> : null}` 2. For each match, identify the variable used in the condition (e.g., `subtitle` in `{!!subtitle && ...`) 3. Check the component's type definition — is this prop optional (`subtitle?: string`)? -4. Search the entire component body for other uses of this prop — is the conditional render the ONLY place it appears? +4. Search the entire component body for other uses of this prop — is the prop used **only for conditional rendering**? (Note: a prop may appear in multiple conditional render blocks and still be a violation — the test is whether ALL uses are solely `{!!prop && }` or `{prop ? : null}` patterns, not whether it appears in only one place.) 5. If yes to both (optional + sole purpose is conditional render) → flag as Case 2 violation **Case 3 — Monolithic prop interface:** @@ -326,6 +442,13 @@ Flag when ANY of these are true: - The array is `.map()`'d through a generic component to produce JSX - Each item has distinct behavior or props that could be expressed as individual JSX elements or dedicated components - Business logic is mixed into the array (conditional entries via `&&`, `.filter()`) +- **Important**: Using compound components inside `.map()` over a static array does not resolve the violation — the fix is to inline each item as distinct JSX, not to improve the mapped component's API. The anti-pattern is the static config array itself, not the component being mapped. + +**Case 5 — Render function props:** +- A component accepts `render*` function props (e.g., `renderTitle`, `renderSubtitle`, `renderFooter`) that return JSX +- The component calls these functions to fill specific areas of its layout +- Each render function corresponds to an area that could be a compound component child instead +- The component manages fallback logic between the render function and a default prop (e.g., `renderTitle ? renderTitle() : {title}`) In all cases, the rule applies to: **new components**, **new features added to existing components**, and **refactorings that create new components still following configuration patterns** @@ -340,9 +463,13 @@ In all cases, the rule applies to: **new components**, **new features added to e - The array is used with **list components** (e.g., `FlatList`, `SectionList`, or custom wrappers) — these require data arrays by design - Items are truly **homogeneous** (same shape, same behavior, only values differ) and the count is **unbounded** (e.g., list of chat messages, search results) - The array is a **framework requirement** (e.g., React Navigation screen config, form validation rules) +- The `ReactNode` prop is `children` itself — `children` is the foundation of composition, not configuration +- The render function is a **list component callback** (`renderItem` on `FlatList`, `SectionList`, `DraggableList`) — these are framework requirements +- 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 **Search Patterns** (hints for reviewers): - **Case 1**: `should\w+`, `can\w+`, `enable`, `disable` (boolean flag prefixes in prop types) -- **Case 2**: `{!!`, `&&\s*<`, `?\s*<`, `: null`, combined with optional prop markers (`?:` in type definitions) -- **Case 3**: Components with 8+ props in their type definition, especially mixing state/handler/content props +- **Case 2**: `{!!`, `&&\s*<`, `?\s*<`, `: null`, combined with optional prop markers (`?:` in type definitions); named `ReactNode` or `ReactElement` optional props (`\w+\?:\s*(React\.)?React(Node|Element)`) +- **Case 3**: Components where props collectively configure appearance AND behavior — look for a mix of state props, handler props, and content/slot props in the same type definition - **Case 4**: `\.map\(` combined with array literal `= \[` in the same component; `actions={[`, `items={[`, `options={[` (config arrays passed as props); `.filter(` on array literals (conditional items) +- **Case 5**: `render\w+\??\s*:` in type definitions (render function props); `render\w+\s*\??\s*\(` in component bodies (render function calls) From c4e20e35cb4773e34574f6220f41550a121d8d4b Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 18 Feb 2026 11:54:43 +0100 Subject: [PATCH 5/6] Add internal render helper detection to CLEAN-REACT-PATTERNS-4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Render helpers (renderContent, renderTodoItems, etc.) are a single-responsibility violation — they hide the render tree and prevent React Compiler memoization. Added incorrect example, detection conditions, exceptions, and search patterns. Co-Authored-By: Claude Opus 4.6 --- .../clean-react-4-no-side-effect-spaghetti.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/.claude/skills/coding-standards/rules/clean-react-4-no-side-effect-spaghetti.md b/.claude/skills/coding-standards/rules/clean-react-4-no-side-effect-spaghetti.md index 817ededb2e35..706e5809f894 100644 --- a/.claude/skills/coding-standards/rules/clean-react-4-no-side-effect-spaghetti.md +++ b/.claude/skills/coding-standards/rules/clean-react-4-no-side-effect-spaghetti.md @@ -91,6 +91,41 @@ In this example: - Effects could be extracted to focused hooks: `useTelemetrySpans`, `useDeepLinking`, `useAudioMode`, etc. - Entry points don't get special treatment — extracting effects into named hooks improves clarity and makes it possible to understand what each effect does and how to safely modify it +#### Incorrect (internal render helper functions) + +- Internal `render*` functions signal the component owns multiple rendering responsibilities that should be separate components +- They close over the entire component scope — the React Compiler cannot independently memoize them +- They hide the component's render tree — the return statement doesn't show what actually renders +- The fix is to extract each helper into its own component with explicit props + +```tsx +function ForYouSection() { + const theme = useTheme(); + const {translate} = useLocalize(); + const {submitCount, approveCount} = useTodos(); + + // ❌ Internal render helper — closes over everything, hides structure + const renderTodoItems = () => ( + + {todoItems.map(({key, icon, ...rest}) => ( + + ))} + + ); + + // ❌ Another render helper — branching logic buried in a function + const renderContent = () => { + if (isLoadingApp) { + return ; + } + return hasAnyTodos ? renderTodoItems() : ; + }; + + // The return statement hides the actual render tree + return {renderContent()}; +} +``` + ### Correct #### Correct (separated concerns) @@ -154,6 +189,9 @@ Flag when a component, hook, or utility aggregates multiple unrelated responsibi - Unrelated state variables are interdependent or updated together - Logic mixes data fetching, navigation, UI state, and lifecycle behavior in one place - Removing one piece of functionality requires careful untangling from others +- Component defines internal `render*` functions or arrow functions that return JSX and calls them in its return statement (e.g., `const renderContent = () => ...`, `{renderContent()}`) +- These functions close over the component's entire scope, preventing the React Compiler from independently memoizing them +- The component's return statement calls these helpers instead of showing the render tree directly **What counts as "unrelated":** - Group by responsibility (what the code does), NOT by timing (when it runs) @@ -163,7 +201,11 @@ Flag when a component, hook, or utility aggregates multiple unrelated responsibi **DO NOT flag if:** - Component is a thin orchestration layer that ONLY composes child components (no business logic, no effects beyond rendering) - 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 +- The internal function is a **callback or event handler** (e.g., `handlePress`, `onSubmit`), not a render helper — only functions that return JSX qualify +- The internal function is a **single early return** for a guard clause (e.g., `if (!data) return ;` at the top of the component) — simple guards in the component body are not render helpers **Search Patterns** (hints for reviewers): - `useEffect` - `useOnyx` +- `const render\w+\s*=` or `function render\w+` inside a component body (internal render helpers) +- `{render\w+\(\)}` in JSX return statements (helper invocations) From 782db44f7aecd2ddfd9382ab1fb3a9ab582bcb9c Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 18 Feb 2026 18:27:15 +0100 Subject: [PATCH 6/6] Close consumer-vs-creator loophole, tighten presentational exception in CLEAN-REACT-PATTERNS-1 Co-Authored-By: Claude Opus 4.6 --- .../rules/clean-react-1-composition-over-config.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md index 17c0d1f9f93c..65bd48e1a31a 100644 --- a/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md +++ b/.claude/skills/coding-standards/rules/clean-react-1-composition-over-config.md @@ -450,12 +450,14 @@ Flag when ANY of these are true: - Each render function corresponds to an area that could be a compound component child instead - The component manages fallback logic between the render function and a default prop (e.g., `renderTitle ? renderTitle() : {title}`) -In all cases, the rule applies to: **new components**, **new features added to existing components**, and **refactorings that create new components still following configuration patterns** +In all cases, the rule applies to: **new components**, **new features added to existing components**, **refactorings that create new components still following configuration patterns**, and **new consumers of existing config-heavy components**. + +**Consumer vs. Creator:** New code that consumes a component with a known configuration-heavy API (many props controlling what/how to render) SHOULD be flagged. Each new consumer cements the config pattern and makes future refactoring harder. The fix is to advocate for a compositional wrapper or refactor — not to silently adopt the old pattern. Flag new consumers at the same severity as the component creator. **DO NOT flag if:** - Props are domain identifiers used for data fetching (e.g., `reportID`, `policyID`, `transactionID`) - Props are event handlers for abstract actions (e.g., `onPress`, `onChange`, `onSelectRow`) -- Props are structural/presentational (e.g., `style`, `testID`) — this includes booleans that select between layout or styling strategies on a focused component (e.g., `shouldUseAspectRatio` toggling between fixed-height and aspect-ratio styles) +- Props are **purely presentational** (e.g., `style`, `testID`, `numberOfLines`, `fill`, `iconFill`). A prop is presentational ONLY if removing it would change appearance but NOT structure, content, or layout strategy. Props that select between rendering strategies (e.g., `shouldUseAspectRatio` toggling layout modes, `shouldShowX` toggling element visibility) or control which content appears are NOT presentational — they are behavioral flags (Case 1). - The component already uses composition and child components for features - The optional prop is used for logic beyond just conditional rendering (e.g., computing derived values, passed to callbacks, used in multiple places within the component) - The component is a thin wrapper around a platform primitive (e.g., wrapping `TextInput`, `ScrollView`, `Pressable`) — these naturally pass through configuration props