-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[No QA] docs: Add AI code review performance rules #67647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
mountiny
merged 13 commits into
Expensify:main
from
callstack-internal:docs/ai-code-review-rules
Aug 7, 2025
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
9392f52
docs: Add AI code review performance rules
adhorodyski 7d5ecda
chore: remove redundant icons
adhorodyski d0ef42f
fix: correct PERF-1 rule description and examples
adhorodyski db37f83
chore: rm redundant OPTIONAL severity level
adhorodyski 5f5073e
chore: explain severity levels, fix code example
adhorodyski aa429b0
chore: rm rule no2 (covered by eslint)
adhorodyski c0d976c
docs: add a Reasoning block to every rule to better explain it
adhorodyski 92052f6
docs: rm the severity level property from rules
adhorodyski 55331f1
docs: rm the rule on inline jsx
adhorodyski 4a67922
docs: instruct to include a comment per rule violation
adhorodyski 8646f57
chore: remove redundant comment lines
adhorodyski 2ec8d70
chore: list very important review guidelines, including the ruleID re…
adhorodyski b6d61bc
chore: fix inline returns to follow the styleguide
adhorodyski File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| # ✅ AI Code Review Rules | ||
|
|
||
| These rules are used to conduct structured code reviews on pull request diffs. | ||
|
|
||
| Each rule includes: | ||
| - A unique **Rule ID** | ||
| - **Pass/Fail condition** | ||
| - **Reasoning**: Technical explanation of why the rule is important | ||
| - Examples of good and bad usage | ||
|
|
||
| Very important: | ||
| - Make sure you include a separate comment for every rule violation | ||
| - Every comment has to reference a **Rule ID** it violates | ||
|
|
||
| --- | ||
|
|
||
| ## Performance Rules | ||
|
|
||
| ### [PERF-1] No spread in list item's renderItem | ||
| - **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. | ||
| - **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. | ||
|
|
||
| Good: | ||
|
adhorodyski marked this conversation as resolved.
|
||
| ```tsx | ||
| <Component | ||
| item={item} | ||
| isSelected={isSelected} | ||
| shouldAnimateInHighlight={isItemHighlighted} | ||
| /> | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```tsx | ||
| <Component | ||
| item={{ | ||
| shouldAnimateInHighlight: isItemHighlighted, | ||
| isSelected: selected, | ||
| ...item, | ||
| }} | ||
| /> | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### [PERF-2] Use early returns in array iteration methods | ||
| - **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. | ||
|
adhorodyski marked this conversation as resolved.
|
||
| - **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. | ||
|
|
||
| Good: | ||
| ```ts | ||
| const areAllTransactionsValid = transactions.every((transaction) => { | ||
| if (!transaction.rawData || transaction.amount <= 0) { | ||
| return false; | ||
| } | ||
| const validation = validateTransaction(transaction); | ||
| return validation.isValid; | ||
| }); | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```ts | ||
| const areAllTransactionsValid = transactions.every((transaction) => { | ||
| const validation = validateTransaction(transaction); | ||
| return validation.isValid; | ||
| }); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem | ||
| - **Condition**: Components rendered inside `renderItem` functions should use dedicated hooks from `OnyxListItemProvider` instead of individual `useOnyx` calls. | ||
| - **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. | ||
|
|
||
| Good: | ||
| ```tsx | ||
| const personalDetails = usePersonalDetails(); | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```tsx | ||
| const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### [PERF-4] Memoize objects and functions passed as props | ||
| - **Condition**: Objects and functions passed as props should be properly memoized or simplified to primitive values to prevent unnecessary re-renders. | ||
| - **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. | ||
|
|
||
| Good: | ||
| ```tsx | ||
| const reportData = useMemo(() => ({ | ||
| reportID: report.reportID, | ||
| type: report.type, | ||
| isPinned: report.isPinned, | ||
| }), [report.reportID, report.type, report.isPinned]); | ||
|
|
||
| return <ReportActionItem report={reportData} /> | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```tsx | ||
| const [report] = useOnyx(`ONYXKEYS.COLLECTION.REPORT${iouReport.id}`); | ||
|
|
||
| return <ReportActionItem report={report} /> | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### [PERF-5] Use shallow comparisons instead of deep comparisons | ||
| - **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. | ||
|
adhorodyski marked this conversation as resolved.
|
||
| - **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. | ||
|
|
||
| Good: | ||
| ```tsx | ||
| memo(ReportActionItem, (prevProps, nextProps) => | ||
| prevProps.report.type === nextProps.report.type && | ||
| prevProps.report.reportID === nextProps.report.reportID && | ||
| prevProps.isSelected === nextProps.isSelected | ||
| ) | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```tsx | ||
| memo(ReportActionItem, (prevProps, nextProps) => | ||
| deepEqual(prevProps.report, nextProps.report) && | ||
| prevProps.isSelected === nextProps.isSelected | ||
| ) | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### [PERF-6] Use specific properties as hook dependencies | ||
| - **Condition**: In `useEffect`, `useMemo`, and `useCallback`, specify individual object properties as dependencies instead of passing entire objects. | ||
| - **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. | ||
|
|
||
| Good: | ||
| ```tsx | ||
| const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => { | ||
| return { | ||
| amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| }; | ||
| }, [transactionItem.isAmountColumnWide, transactionItem.isTaxAmountColumnWide, transactionItem.shouldShowYear]); | ||
| ``` | ||
|
|
||
| Bad: | ||
| ```tsx | ||
| const {amountColumnSize, dateColumnSize, taxAmountColumnSize} = useMemo(() => { | ||
| return { | ||
| amountColumnSize: transactionItem.isAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| taxAmountColumnSize: transactionItem.isTaxAmountColumnWide ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| dateColumnSize: transactionItem.shouldShowYear ? CONST.SEARCH.TABLE_COLUMN_SIZES.WIDE : CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL, | ||
| }; | ||
| }, [transactionItem]); | ||
| ``` | ||
|
|
||
| --- | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.