Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 150 additions & 0 deletions .claude/agents/code-inline-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,156 @@ async function submitForm(data: FormData) {

---

### [CLEAN-REACT-PATTERNS-1] Favor composition over configuration

- **Search patterns**: `shouldShow`, `shouldEnable`, `canSelect`, `enable`, `disable`, configuration props patterns

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure how AI reviewer works in this area, but I think we should be specific here that all we care about are props. For example, shouldShow can be const shouldShowSpinner = ..., user.canSelectSomething or function shouldShow2FAModal(). This probably forces to determine if it's a prop definition or not for each match

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think we could define them as should*, can* etc


- **Condition**: Flag ONLY when ALL of these are true:

- A **new feature** is being introduced OR an **existing component's API is being expanded with new props**
Comment thread
mountiny marked this conversation as resolved.
- The change adds configuration properties (flags, conditional logic)
- These configuration options control feature presence or behavior within the component
- These features could instead be expressed as composable child components

**Features that should be expressed as child components:**
- 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

**DO NOT flag if:**
- Props are narrow, stable 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

- **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".

Good (composition):

- Features expressed as composable children
- Parent stays stable; add features by adding children

```tsx
<Table data={items} columns={columns}>
<Table.SearchBar />
<Table.Header />
<Table.Body />
</Table>
```

```tsx
<SelectionList data={items}>
<SelectionList.TextInput />
<SelectionList.Body />
</SelectionList>
```

Bad (configuration):

- Features controlled by boolean flags
- Adding a new feature requires modifying the Table component's API

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to add sth like "When configuration is fine" with examples? For example

  • behavioral modifiers affecting entire component (disabled)
  • styling variants
  • accessibility props


```tsx
<Table
data={items}
columns={columns}
shouldShowSearchBar
shouldShowHeader
shouldEnableSorting
shouldShowPagination
shouldHighlightOnHover
/>

type TableProps = {
data: Item[];
columns: Column[];
shouldShowSearchBar?: boolean; // ❌ Could be <Table.SearchBar />
shouldShowHeader?: boolean; // ❌ Could be <Table.Header />
shouldEnableSorting?: boolean; // ❌ Configuration for header behavior
shouldShowPagination?: boolean; // ❌ Could be <Table.Pagination />
shouldHighlightOnHover?: boolean; // ❌ Configuration for styling behavior
};
```

```tsx
<SelectionList
data={items}
shouldShowTextInput
shouldShowTooltips
shouldScrollToFocusedIndex
shouldDebounceScrolling
shouldUpdateFocusedIndex
canSelectMultiple
disableKeyboardShortcuts
/>

type SelectionListProps = {
shouldShowTextInput?: boolean; // ❌ Could be <SelectionList.TextInput />
shouldShowConfirmButton?: boolean; // ❌ Could be <SelectionList.ConfirmButton />
textInputOptions?: {...}; // ❌ Configuration object for the above
};
```
Comment thread
adhorodyski marked this conversation as resolved.

Good (children manage their own state):

```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 (
<>
<ReportActionsView reportID={reportID} />
// other features
<Composer />
</>
);
}

// 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);
// ...
}
```

Bad (parent manages child state):

```tsx
// Parent fetches and manages state for its children
// Parent has to know child implementation details
function ReportScreen({ params: { reportID }}) {
const [reportOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {allowStaleData: true, canBeMissing: true});
const reportActions = useMemo(() => getFilteredReportActionsForReportView(unfilteredReportActions), [unfilteredReportActions]);
const [reportMetadata = defaultReportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`, {canBeMissing: true, allowStaleData: true});
const {reportActions: unfilteredReportActions, linkedAction, sortedAllReportActions, hasNewerActions, hasOlderActions} = usePaginatedReportActions(reportID, reportActionIDFromRoute);
const parentReportAction = useParentReportAction(reportOnyx);
const transactionThreadReportID = getOneTransactionThreadReportID(report, chatReport, reportActions ?? [], isOffline, reportTransactionIDs);
const isTransactionThreadView = isReportTransactionThread(report);
// other onyx connections etc

return (
<>
<ReportActionsView
report={report}
reportActions={reportActions}
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
hasNewerActions={hasNewerActions}
hasOlderActions={hasOlderActions}
parentReportAction={parentReportAction}
transactionThreadReportID={transactionThreadReportID}
isReportTransactionThread={isTransactionThreadView}
/>
// other features
<Composer />
</>
);
}
```

---

## Instructions

1. **First, get the list of changed files and their diffs:**
Expand Down
2 changes: 2 additions & 0 deletions contributingGuides/STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,8 @@ So, if a new language feature isn't something we have agreed to support it's off

## React Coding Standards

For additional React best practices and patterns that are automatically enforced during code review, see the [AI Code Reviewer Rules](../.claude/agents/code-inline-reviewer.md).

### Code Documentation

* Add descriptions to all component props using a block comment above the definition. No need to document the types, but add some context for each property so that other developers understand the intended use.
Expand Down
Loading