|
| 1 | +# ✅ AI Code Review Rules |
| 2 | + |
| 3 | +These rules are used to conduct structured code reviews on pull request diffs. |
| 4 | + |
| 5 | +Each rule includes: |
| 6 | +- A unique **Rule ID** |
| 7 | +- **Pass/Fail condition** |
| 8 | +- **Reasoning**: Technical explanation of why the rule is important |
| 9 | +- Examples of good and bad usage |
| 10 | + |
| 11 | +Very important: |
| 12 | +- Make sure you include a separate comment for every rule violation |
| 13 | +- Every comment has to reference a **Rule ID** it violates |
| 14 | + |
| 15 | +--- |
| 16 | + |
| 17 | +## Performance Rules |
| 18 | + |
| 19 | +### [PERF-1] No spread in list item's renderItem |
| 20 | +- **Condition**: When passing data to components in renderItem functions, avoid using spread operators to extend objects. Instead, pass the base object and additional properties as separate props to prevent unnecessary object creation on each render. |
| 21 | +- **Reasoning**: `renderItem` functions execute for every visible list item on each render. Creating new objects with spread operators forces React to treat each item as changed, preventing reconciliation optimizations and causing unnecessary re-renders of child components. |
| 22 | + |
| 23 | +Good: |
| 24 | +```tsx |
| 25 | +<Component |
| 26 | + item={item} |
| 27 | + isSelected={isSelected} |
| 28 | + shouldAnimateInHighlight={isItemHighlighted} |
| 29 | +/> |
| 30 | +``` |
| 31 | + |
| 32 | +Bad: |
| 33 | +```tsx |
| 34 | +<Component |
| 35 | + item={{ |
| 36 | + shouldAnimateInHighlight: isItemHighlighted, |
| 37 | + isSelected: selected, |
| 38 | + ...item, |
| 39 | + }} |
| 40 | +/> |
| 41 | +``` |
| 42 | + |
| 43 | +--- |
| 44 | + |
| 45 | +### [PERF-2] Use early returns in array iteration methods |
| 46 | +- **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. |
| 47 | +- **Reasoning**: Expensive operations can be any long-running synchronous tasks (like complex calculations) and should be avoided when simple property checks can eliminate items early. This reduces unnecessary computation and improves iteration performance, especially on large datasets. |
| 48 | + |
| 49 | +Good: |
| 50 | +```ts |
| 51 | +const areAllTransactionsValid = transactions.every((transaction) => { |
| 52 | + if (!transaction.rawData || transaction.amount <= 0) { |
| 53 | + return false; |
| 54 | + } |
| 55 | + const validation = validateTransaction(transaction); |
| 56 | + return validation.isValid; |
| 57 | +}); |
| 58 | +``` |
| 59 | + |
| 60 | +Bad: |
| 61 | +```ts |
| 62 | +const areAllTransactionsValid = transactions.every((transaction) => { |
| 63 | + const validation = validateTransaction(transaction); |
| 64 | + return validation.isValid; |
| 65 | +}); |
| 66 | +``` |
| 67 | + |
| 68 | +--- |
| 69 | + |
| 70 | +### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem |
| 71 | +- **Condition**: Components rendered inside `renderItem` functions should use dedicated hooks from `OnyxListItemProvider` instead of individual `useOnyx` calls. |
| 72 | +- **Reasoning**: Individual `useOnyx` calls in renderItem create separate subscriptions for each list item, causing memory overhead and update cascades. `OnyxListItemProvider` hooks provide optimized data access patterns specifically designed for list rendering performance. |
| 73 | + |
| 74 | +Good: |
| 75 | +```tsx |
| 76 | +const personalDetails = usePersonalDetails(); |
| 77 | +``` |
| 78 | + |
| 79 | +Bad: |
| 80 | +```tsx |
| 81 | +const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); |
| 82 | +``` |
| 83 | + |
| 84 | +--- |
| 85 | + |
| 86 | +### [PERF-4] Memoize objects and functions passed as props |
| 87 | +- **Condition**: Objects and functions passed as props should be properly memoized or simplified to primitive values to prevent unnecessary re-renders. |
| 88 | +- **Reasoning**: React uses referential equality to determine if props changed. New object/function instances on every render trigger unnecessary re-renders of child components, even when the actual data hasn't changed. Memoization preserves referential stability. |
| 89 | + |
| 90 | +Good: |
| 91 | +```tsx |
| 92 | +const reportData = useMemo(() => ({ |
| 93 | + reportID: report.reportID, |
| 94 | + type: report.type, |
| 95 | + isPinned: report.isPinned, |
| 96 | +}), [report.reportID, report.type, report.isPinned]); |
| 97 | + |
| 98 | +return <ReportActionItem report={reportData} /> |
| 99 | +``` |
| 100 | + |
| 101 | +Bad: |
| 102 | +```tsx |
| 103 | +const [report] = useOnyx(`ONYXKEYS.COLLECTION.REPORT${iouReport.id}`); |
| 104 | + |
| 105 | +return <ReportActionItem report={report} /> |
| 106 | +``` |
| 107 | + |
| 108 | +--- |
| 109 | + |
| 110 | +### [PERF-5] Use shallow comparisons instead of deep comparisons |
| 111 | +- **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. |
| 112 | +- **Reasoning**: Deep equality checks recursively compare all nested properties, creating performance overhead that often exceeds the re-render cost they aim to prevent. Shallow comparisons of specific relevant properties provide the same optimization benefits with minimal computational cost. |
| 113 | + |
| 114 | +Good: |
| 115 | +```tsx |
| 116 | +memo(ReportActionItem, (prevProps, nextProps) => |
| 117 | + prevProps.report.type === nextProps.report.type && |
| 118 | + prevProps.report.reportID === nextProps.report.reportID && |
| 119 | + prevProps.isSelected === nextProps.isSelected |
| 120 | +) |
| 121 | +``` |
| 122 | + |
| 123 | +Bad: |
| 124 | +```tsx |
| 125 | +memo(ReportActionItem, (prevProps, nextProps) => |
| 126 | + deepEqual(prevProps.report, nextProps.report) && |
| 127 | + prevProps.isSelected === nextProps.isSelected |
| 128 | +) |
| 129 | +``` |
| 130 | + |
| 131 | +--- |
| 132 | + |
| 133 | +### [PERF-6] Use specific properties as hook dependencies |
| 134 | +- **Condition**: In `useEffect`, `useMemo`, and `useCallback`, specify individual object properties as dependencies instead of passing entire objects. |
| 135 | +- **Reasoning**: Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability. |
| 136 | + |
| 137 | +Good: |
| 138 | +```tsx |
| 139 | +const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => { |
| 140 | + return { |
| 141 | + amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 142 | + taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 143 | + dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 144 | + }; |
| 145 | +}, [transactionItem.isAmountColumnWide, transactionItem.isTaxAmountColumnWide, transactionItem.shouldShowYear]); |
| 146 | +``` |
| 147 | + |
| 148 | +Bad: |
| 149 | +```tsx |
| 150 | +const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => { |
| 151 | + return { |
| 152 | + amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 153 | + taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 154 | + dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, |
| 155 | + }; |
| 156 | +}, [transactionItem]); |
| 157 | +``` |
| 158 | + |
| 159 | +--- |
0 commit comments