Skip to content

Commit ab9d20a

Browse files
committed
Merge branch 'main' into fix/75881
2 parents e61a1f5 + 757e1ec commit ab9d20a

1,202 files changed

Lines changed: 60425 additions & 13605 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.claude/agents/code-inline-reviewer.md

Lines changed: 208 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -143,83 +143,6 @@ const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
143143

144144
---
145145

146-
### [PERF-4] Memoize objects (including arrays) and functions passed as props
147-
148-
- **Search patterns**: `prop={{`, `prop={[`, `={() =>`, `prop={variable}` (where variable is non-memoized object/function)
149-
150-
- **Applies ONLY to**: Objects (including arrays)/functions passed directly as JSX props. Does NOT apply to:
151-
- Code inside callbacks (`.then()`, event handlers)
152-
- Code inside `useEffect`/`useMemo`/`useCallback` bodies
153-
- Primitives (strings, numbers, booleans)
154-
- Already memoized values (`useMemo`/`useCallback`)
155-
156-
- **Reasoning**: New object/function references break memoization of child components. Only matters when child IS memoized AND parent is NOT optimized by React Compiler.
157-
158-
#### Before flagging: Run optimization check
159-
160-
**YOU MUST call `checkReactCompilerOptimization.ts` (available in PATH from `.claude/scripts/`) on EVERY .tsx file from the diff.**
161-
162-
**Call the script ONCE per file, separately. DO NOT use loops or batch processing.**
163-
164-
Example usage:
165-
```bash
166-
checkReactCompilerOptimization.ts src/components/File1.tsx
167-
checkReactCompilerOptimization.ts src/components/File2.tsx
168-
```
169-
170-
**NEVER use absolute or relative paths for this script. Call it by name only:**
171-
-`checkReactCompilerOptimization.ts src/components/Example.tsx`
172-
-`/home/runner/work/App/App/.claude/scripts/checkReactCompilerOptimization.ts ...`
173-
-`./.claude/scripts/checkReactCompilerOptimization.ts ...`
174-
175-
**"File not found"** → Assume parent is optimized and skip PERF-4.
176-
177-
#### Decision flow
178-
179-
1. **Parent in `parentOptimized`?** → YES = **Skip** (compiler auto-memoizes)
180-
181-
2. **Child has custom memo comparator that PREVENTS re-render for this prop?**
182-
→ Use `sourcePath` from script output to read child's source file
183-
→ Grep for `React.memo` or `memo(`
184-
→ If custom comparator prevents re-render despite new reference for this prop → **Skip**
185-
186-
3. **Child is memoized?** (`optimized: true` OR `React.memo`)
187-
- NO → **Skip** (child re-renders anyway)
188-
- YES → **Flag PERF-4**
189-
190-
#### Examples
191-
192-
**Flag** (parent NOT optimized, child IS memoized, no custom comparator):
193-
```tsx
194-
// Script output: parentOptimized: [], child MemoizedList optimized: true
195-
// No custom comparator found
196-
return <MemoizedList options={{ showHeader: true }} />;
197-
```
198-
199-
**Skip - custom comparator** (comparator prevents re-render for this prop):
200-
```tsx
201-
// Script output: sourcePath: "src/components/PopoverMenu.tsx"
202-
// PopoverMenu.tsx has custom memo comparator that handles anchorPosition
203-
return <PopoverMenu anchorPosition={{x: 0, y: 0}} />;
204-
```
205-
206-
**Skip - parent optimized**:
207-
```tsx
208-
// Script output: parentOptimized: ["MyComponent"]
209-
// React Compiler auto-memoizes - no manual memoization needed
210-
return <MemoizedList options={{ showHeader: true }} />;
211-
```
212-
213-
**Skip - spread props with stable inner values**:
214-
```tsx
215-
// Spread is OK when inner values come from memoized sources
216-
// illustration from useMemoizedLazyIllustrations, illustrationStyle from useThemeStyles
217-
const illustration = useAboutSectionIllustration();
218-
return <Section {...illustration} />;
219-
```
220-
221-
---
222-
223146
### [PERF-5] Use shallow comparisons instead of deep comparisons
224147

225148
- **Search patterns**: `React.memo`, `deepEqual`
@@ -551,6 +474,64 @@ function TimerComponent() {
551474

552475
---
553476

477+
### [PERF-13] Avoid iterator-independent function calls in array methods
478+
479+
- **Search patterns**: `.map(`, `.reduce(`, `.filter(`, `.some(`, `.every(`, `.find(`, `.findIndex(`, `.flatMap(`, `.forEach(`
480+
481+
- **Condition**: Flag when ALL of these are true:
482+
483+
**For side-effect-free methods** (`.map`, `.reduce`, `.filter`, `.some`, `.every`, `.find`, `.findIndex`, `.flatMap`):
484+
- A function call exists inside the callback
485+
- The function call does NOT receive:
486+
- The iterator variable directly (e.g., `transform(item)`)
487+
- A property/value derived from the iterator (e.g., `format(item.name)`)
488+
- The index parameter when used meaningfully (e.g., `generateId(index)`)
489+
- The function is not a method called ON the iterator or iterator-derived value (e.g., `item.getValue()`)
490+
491+
**For `.forEach`**:
492+
- Same conditions as above, BUT also verify the side effect doesn't depend on iteration context
493+
- If the function call would produce the same effect regardless of which iteration it runs in, flag it
494+
495+
**DO NOT flag if:**
496+
497+
- Function uses iterator, its parts or derived value based on iterator (e.g. `func(item.process())`)
498+
- Function call depends on iterator (e.g. `item.value ?? getDefault()`)
499+
- Function is used when mapping to new entities (e.g. `const thing = { id: createID() }`)
500+
- Above but applied to index instead of iterator
501+
502+
- **Reasoning**: Function calls inside iteration callbacks that don't use the iterator variable execute redundantly - producing the same result. This creates O(n) overhead that scales with data size. Hoisting these calls outside the loop eliminates redundant computation and improves performance, especially on large datasets like transaction lists or report collections.
503+
504+
Good:
505+
506+
```ts
507+
// Hoist iterator-independent calls outside the loop
508+
const config = getConfig();
509+
510+
const results = items.map((item) => item.value * config.multiplier);
511+
```
512+
513+
```ts
514+
// Function receives iterator or iterator-derived value
515+
const formatted = items.map((item) => formatCurrency(item.amount));
516+
```
517+
518+
```ts
519+
// Index used meaningfully
520+
const indexed = items.map((item, index) => ({ ...item, id: generateId(index) }));
521+
```
522+
523+
Bad:
524+
525+
```ts
526+
// getConfig() called on every iteration but doesn't use item
527+
const results = items.map((item) => {
528+
const config = getConfig();
529+
return item.value * config.multiplier;
530+
});
531+
```
532+
533+
---
534+
554535
### [CONSISTENCY-1] Avoid platform-specific checks within components
555536

556537
- **Search patterns**: `Platform.OS`, `isAndroid`, `isIOS`, `Platform\.select`
@@ -844,6 +825,156 @@ async function submitForm(data: FormData) {
844825

845826
---
846827

828+
### [CLEAN-REACT-PATTERNS-1] Favor composition over configuration
829+
830+
- **Search patterns**: `shouldShow`, `shouldEnable`, `canSelect`, `enable`, `disable`, configuration props patterns
831+
832+
- **Condition**: Flag ONLY when ALL of these are true:
833+
834+
- A **new feature** is being introduced OR an **existing component's API is being expanded with new props**
835+
- The change adds configuration properties (flags, conditional logic)
836+
- These configuration options control feature presence or behavior within the component
837+
- These features could instead be expressed as composable child components
838+
839+
**Features that should be expressed as child components:**
840+
- Optional UI elements that could be composed in
841+
- New behavior that could be introduced as new children
842+
- Features that currently require parent component code changes
843+
844+
**DO NOT flag if:**
845+
- Props are narrow, stable values needed for coordination between composed parts (e.g., `reportID`, `data`, `columns`)
846+
- The component uses composition and child components for features
847+
- Parent components stay stable as features are added
848+
849+
- **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".
850+
851+
Good (composition):
852+
853+
- Features expressed as composable children
854+
- Parent stays stable; add features by adding children
855+
856+
```tsx
857+
<Table data={items} columns={columns}>
858+
<Table.SearchBar />
859+
<Table.Header />
860+
<Table.Body />
861+
</Table>
862+
```
863+
864+
```tsx
865+
<SelectionList data={items}>
866+
<SelectionList.TextInput />
867+
<SelectionList.Body />
868+
</SelectionList>
869+
```
870+
871+
Bad (configuration):
872+
873+
- Features controlled by boolean flags
874+
- Adding a new feature requires modifying the Table component's API
875+
876+
```tsx
877+
<Table
878+
data={items}
879+
columns={columns}
880+
shouldShowSearchBar
881+
shouldShowHeader
882+
shouldEnableSorting
883+
shouldShowPagination
884+
shouldHighlightOnHover
885+
/>
886+
887+
type TableProps = {
888+
data: Item[];
889+
columns: Column[];
890+
shouldShowSearchBar?: boolean; // ❌ Could be <Table.SearchBar />
891+
shouldShowHeader?: boolean; // ❌ Could be <Table.Header />
892+
shouldEnableSorting?: boolean; // ❌ Configuration for header behavior
893+
shouldShowPagination?: boolean; // ❌ Could be <Table.Pagination />
894+
shouldHighlightOnHover?: boolean; // ❌ Configuration for styling behavior
895+
};
896+
```
897+
898+
```tsx
899+
<SelectionList
900+
data={items}
901+
shouldShowTextInput
902+
shouldShowTooltips
903+
shouldScrollToFocusedIndex
904+
shouldDebounceScrolling
905+
shouldUpdateFocusedIndex
906+
canSelectMultiple
907+
disableKeyboardShortcuts
908+
/>
909+
910+
type SelectionListProps = {
911+
shouldShowTextInput?: boolean; // ❌ Could be <SelectionList.TextInput />
912+
shouldShowConfirmButton?: boolean; // ❌ Could be <SelectionList.ConfirmButton />
913+
textInputOptions?: {...}; // ❌ Configuration object for the above
914+
};
915+
```
916+
917+
Good (children manage their own state):
918+
919+
```tsx
920+
// Children are self-contained and manage their own state
921+
// Parent only passes minimal data (IDs)
922+
// Adding new features doesn't require changing the parent
923+
function ReportScreen({ params: { reportID }}) {
924+
return (
925+
<>
926+
<ReportActionsView reportID={reportID} />
927+
// other features
928+
<Composer />
929+
</>
930+
);
931+
}
932+
933+
// Component accesses stores and calculates its own state
934+
// Parent doesn't know the internals
935+
function ReportActionsView({ reportID }) {
936+
const [reportOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
937+
const reportActions = getFilteredReportActionsForReportView(unfilteredReportActions);
938+
// ...
939+
}
940+
```
941+
942+
Bad (parent manages child state):
943+
944+
```tsx
945+
// Parent fetches and manages state for its children
946+
// Parent has to know child implementation details
947+
function ReportScreen({ params: { reportID }}) {
948+
const [reportOnyx] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {allowStaleData: true, canBeMissing: true});
949+
const reportActions = useMemo(() => getFilteredReportActionsForReportView(unfilteredReportActions), [unfilteredReportActions]);
950+
const [reportMetadata = defaultReportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportIDFromRoute}`, {canBeMissing: true, allowStaleData: true});
951+
const {reportActions: unfilteredReportActions, linkedAction, sortedAllReportActions, hasNewerActions, hasOlderActions} = usePaginatedReportActions(reportID, reportActionIDFromRoute);
952+
const parentReportAction = useParentReportAction(reportOnyx);
953+
const transactionThreadReportID = getOneTransactionThreadReportID(report, chatReport, reportActions ?? [], isOffline, reportTransactionIDs);
954+
const isTransactionThreadView = isReportTransactionThread(report);
955+
// other onyx connections etc
956+
957+
return (
958+
<>
959+
<ReportActionsView
960+
report={report}
961+
reportActions={reportActions}
962+
isLoadingInitialReportActions={reportMetadata?.isLoadingInitialReportActions}
963+
hasNewerActions={hasNewerActions}
964+
hasOlderActions={hasOlderActions}
965+
parentReportAction={parentReportAction}
966+
transactionThreadReportID={transactionThreadReportID}
967+
isReportTransactionThread={isTransactionThreadView}
968+
/>
969+
// other features
970+
<Composer />
971+
</>
972+
);
973+
}
974+
```
975+
976+
---
977+
847978
## Instructions
848979

849980
1. **First, get the list of changed files and their diffs:**

.claude/commands/review-code-pr.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
---
2-
allowed-tools: Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(addPrReaction.sh:*),Bash(createInlineComment.sh:*),Bash(checkReactCompilerOptimization.ts:*)
2+
allowed-tools: Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(addPrReaction.sh:*),Bash(createInlineComment.sh:*)
33
description: Review a code contribution pull request
44
---
55

0 commit comments

Comments
 (0)