From 9392f52af9bca6829fa3d85e8db87ea0bff230b1 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 1 Aug 2025 14:29:27 +0200 Subject: [PATCH 01/13] docs: Add AI code review performance rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add 8 performance rules for AI code review: - PERF-1: No spread in list item's renderItem (MUST) - PERF-2: Use Set for O(1) lookups instead of array methods (SHOULD) - PERF-3: Use early returns in array iteration methods (SHOULD) - PERF-4: Use OnyxListItemProvider hooks instead of useOnyx in renderItem (MUST) - PERF-5: Memoize objects and functions passed as props (MUST) - PERF-6: Avoid inline object/array/function creation in JSX (SHOULD) - PERF-7: Use shallow comparisons instead of deep comparisons (SHOULD) - PERF-8: Use specific properties as hook dependencies (SHOULD) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- contributingGuides/review/RULES.md | 203 +++++++++++++++++++++++++++++ 1 file changed, 203 insertions(+) create mode 100644 contributingGuides/review/RULES.md diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md new file mode 100644 index 000000000000..3bb3bcbd5449 --- /dev/null +++ b/contributingGuides/review/RULES.md @@ -0,0 +1,203 @@ +# ✅ AI Code Review Rules + +These rules are used to conduct structured code reviews on pull request diffs. Each rule includes: + +- A unique **Rule ID** +- **Severity level**: MUST / SHOULD / OPTIONAL +- **Pass/Fail condition** +- Examples of good and bad usage + +--- + +## �� Performance Rules + +### [PERF-1] No spread in list item's renderItem +- **Severity**: MUST +- **Condition**: Instead of extending the object with spread operator while passing it to the child component in the renderItem function, pass additional properties as props of the child component, and propagate it on its children. + +Good: +```tsx + +``` + +Bad: +```tsx + +``` + +--- + +### [PERF-2] Use Set for O(1) lookups instead of array methods +- **Severity**: SHOULD +- **Condition**: When performing repeated lookups or checking existence in collections, convert arrays to Sets for O(1) performance instead of using O(n) methods like `.includes()`. + +Good: +```tsx +const reportIdFromRoute = report.reportId; +const reportUserIdSet = new Set(Object.values(reports)); +const reportExists = reportUserIdSet.has(reportIdFromRoute); +``` + +Bad: +```tsx +const reportIdFromRoute = report.reportId; +const reportExists = Object.values(reports).includes(reportIdFromRoute); +``` + +--- + +### [PERF-3] Use early returns in array iteration methods +- **Severity**: SHOULD +- **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. + +Good: +```tsx +const areAllTransactionsValid = transactions.every((transaction) => { + if (!transaction.rawData || transaction.amount <= 0) return false; + const validation = validateTransaction(transaction); + return validation.isValid; +}); +``` + +Bad: +```tsx +const areAllTransactionsValid = transactions.every((transaction) => { + const validation = validateTransaction(transaction); + return validation.isValid; +}); +``` + +--- + +### [PERF-4] Use OnyxListItemProvider hooks instead of useOnyx in renderItem +- **Severity**: MUST +- **Condition**: Components rendered inside `renderItem` functions should use dedicated hooks from `OnyxListItemProvider` instead of individual `useOnyx` calls. + +Good: +```tsx +// ReportActionItem.tsx +const personalDetails = usePersonalDetails(); +``` + +Bad: +```tsx +// ReportActionItem.tsx +const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); +``` + +--- + +### [PERF-5] Memoize objects and functions passed as props +- **Severity**: MUST +- **Condition**: Objects and functions passed as props should be properly memoized or simplified to primitive values to prevent unnecessary re-renders. + +Good: +```tsx +const reportData = useMemo(() => ({ + reportID: report.reportID, + type: report.type, + isPinned: report.isPinned, +}), [report.reportID, report.type, report.isPinned]); + +return +``` + +Bad: +```tsx +const [report] = useOnyx(`ONYXKEYS.COLLECTION.REPORT${iouReport.id}`); + +return +``` + +--- + +### [PERF-6] Avoid inline object/array/function creation in JSX +- **Severity**: SHOULD +- **Condition**: Objects, arrays, and functions should not be created inline in JSX. Use `useMemo` or `useCallback` to memoize them when there's actual performance benefit. + +Good: +```tsx +const reportActionItemStyle = useMemo(() => [styles.container, styles.flex], []); +const handleSelect = useCallback(() => { + onSelectRow(item); +}, [onSelectRow, item]); + + +``` + +Bad: +```tsx + onSelectRow(item)} + report={report} +/> +``` + +--- + +### [PERF-7] Use shallow comparisons instead of deep comparisons +- **Severity**: SHOULD +- **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. + +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-8] Use specific properties as hook dependencies +- **Severity**: SHOULD +- **Condition**: In `useEffect`, `useMemo`, and `useCallback`, specify individual object properties as dependencies instead of passing entire objects. + +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]); +``` + +--- From 7d5ecda2ea10420c491399e96ab3ce8e279b0bd3 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 1 Aug 2025 14:32:54 +0200 Subject: [PATCH 02/13] chore: remove redundant icons --- contributingGuides/review/RULES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 3bb3bcbd5449..7db2653f3173 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -9,7 +9,7 @@ These rules are used to conduct structured code reviews on pull request diffs. E --- -## �� Performance Rules +## Performance Rules ### [PERF-1] No spread in list item's renderItem - **Severity**: MUST From d0ef42fdf94f07f3ebfa9f841455b719884c8f2d Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 1 Aug 2025 16:17:39 +0200 Subject: [PATCH 03/13] fix: correct PERF-1 rule description and examples MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule description contradicted the examples - fixed to properly show that spread operators should be avoided in renderItem functions for performance reasons. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- contributingGuides/review/RULES.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 7db2653f3173..4e2184399092 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -13,25 +13,25 @@ These rules are used to conduct structured code reviews on pull request diffs. E ### [PERF-1] No spread in list item's renderItem - **Severity**: MUST -- **Condition**: Instead of extending the object with spread operator while passing it to the child component in the renderItem function, pass additional properties as props of the child component, and propagate it on its children. +- **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. Good: ```tsx ``` Bad: ```tsx ``` From db37f83811e309a0e79513a30517d5c3c4b37504 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Fri, 1 Aug 2025 17:24:00 +0200 Subject: [PATCH 04/13] chore: rm redundant OPTIONAL severity level --- contributingGuides/review/RULES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 4e2184399092..405ed57bfc07 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -3,7 +3,7 @@ These rules are used to conduct structured code reviews on pull request diffs. Each rule includes: - A unique **Rule ID** -- **Severity level**: MUST / SHOULD / OPTIONAL +- **Severity level**: MUST / SHOULD - **Pass/Fail condition** - Examples of good and bad usage From 5f5073efd9ebe469ee25ca8fa30f567c93d8cc05 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 4 Aug 2025 19:34:07 +0200 Subject: [PATCH 05/13] chore: explain severity levels, fix code example --- contributingGuides/review/RULES.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 405ed57bfc07..52cabba93cda 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -3,7 +3,9 @@ These rules are used to conduct structured code reviews on pull request diffs. Each rule includes: - A unique **Rule ID** -- **Severity level**: MUST / SHOULD +- **Severity level**: + - MUST: Mandatory to update this as it causes issues. + - SHOULD: Strong recommendation to consider changing this and think about future implications. - **Pass/Fail condition** - Examples of good and bad usage @@ -136,7 +138,6 @@ const handleSelect = useCallback(() => { style={reportActionItemStyle} onSelect={handleSelect} reportID={report.reportID} - isPinned={report.isPinned} /> ``` @@ -145,7 +146,7 @@ Bad: onSelectRow(item)} - report={report} + reportID={report.reportID} /> ``` From aa429b08ff1e38f66ec89bedcc9bb07a4cb86c4f Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 4 Aug 2025 19:43:38 +0200 Subject: [PATCH 06/13] chore: rm rule no2 (covered by eslint) --- contributingGuides/review/RULES.md | 31 ++++++------------------------ 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 52cabba93cda..a6b3c91f38c0 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -39,26 +39,7 @@ Bad: --- -### [PERF-2] Use Set for O(1) lookups instead of array methods -- **Severity**: SHOULD -- **Condition**: When performing repeated lookups or checking existence in collections, convert arrays to Sets for O(1) performance instead of using O(n) methods like `.includes()`. - -Good: -```tsx -const reportIdFromRoute = report.reportId; -const reportUserIdSet = new Set(Object.values(reports)); -const reportExists = reportUserIdSet.has(reportIdFromRoute); -``` - -Bad: -```tsx -const reportIdFromRoute = report.reportId; -const reportExists = Object.values(reports).includes(reportIdFromRoute); -``` - ---- - -### [PERF-3] Use early returns in array iteration methods +### [PERF-2] Use early returns in array iteration methods - **Severity**: SHOULD - **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. @@ -81,7 +62,7 @@ const areAllTransactionsValid = transactions.every((transaction) => { --- -### [PERF-4] Use OnyxListItemProvider hooks instead of useOnyx in renderItem +### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem - **Severity**: MUST - **Condition**: Components rendered inside `renderItem` functions should use dedicated hooks from `OnyxListItemProvider` instead of individual `useOnyx` calls. @@ -99,7 +80,7 @@ const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); --- -### [PERF-5] Memoize objects and functions passed as props +### [PERF-4] Memoize objects and functions passed as props - **Severity**: MUST - **Condition**: Objects and functions passed as props should be properly memoized or simplified to primitive values to prevent unnecessary re-renders. @@ -123,7 +104,7 @@ return --- -### [PERF-6] Avoid inline object/array/function creation in JSX +### [PERF-5] Avoid inline object/array/function creation in JSX - **Severity**: SHOULD - **Condition**: Objects, arrays, and functions should not be created inline in JSX. Use `useMemo` or `useCallback` to memoize them when there's actual performance benefit. @@ -152,7 +133,7 @@ Bad: --- -### [PERF-7] Use shallow comparisons instead of deep comparisons +### [PERF-6] Use shallow comparisons instead of deep comparisons - **Severity**: SHOULD - **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. @@ -175,7 +156,7 @@ memo(ReportActionItem, (prevProps, nextProps) => --- -### [PERF-8] Use specific properties as hook dependencies +### [PERF-7] Use specific properties as hook dependencies - **Severity**: SHOULD - **Condition**: In `useEffect`, `useMemo`, and `useCallback`, specify individual object properties as dependencies instead of passing entire objects. From c0d976c2b106f4e508b3d2ed0ec58101df42ef1e Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Mon, 4 Aug 2025 19:54:53 +0200 Subject: [PATCH 07/13] docs: add a Reasoning block to every rule to better explain it --- contributingGuides/review/RULES.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index a6b3c91f38c0..49e2d5b8cb33 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -7,6 +7,7 @@ These rules are used to conduct structured code reviews on pull request diffs. E - MUST: Mandatory to update this as it causes issues. - SHOULD: Strong recommendation to consider changing this and think about future implications. - **Pass/Fail condition** +- **Reasoning**: Technical explanation of why the rule is important - Examples of good and bad usage --- @@ -16,6 +17,7 @@ These rules are used to conduct structured code reviews on pull request diffs. E ### [PERF-1] No spread in list item's renderItem - **Severity**: MUST - **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: ```tsx @@ -42,6 +44,7 @@ Bad: ### [PERF-2] Use early returns in array iteration methods - **Severity**: SHOULD - **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. +- **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: ```tsx @@ -65,6 +68,7 @@ const areAllTransactionsValid = transactions.every((transaction) => { ### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem - **Severity**: MUST - **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 @@ -83,6 +87,7 @@ const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); ### [PERF-4] Memoize objects and functions passed as props - **Severity**: MUST - **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 @@ -107,6 +112,7 @@ return ### [PERF-5] Avoid inline object/array/function creation in JSX - **Severity**: SHOULD - **Condition**: Objects, arrays, and functions should not be created inline in JSX. Use `useMemo` or `useCallback` to memoize them when there's actual performance benefit. +- **Reasoning**: Inline creation generates new instances on every render, breaking React's reconciliation optimizations and forcing child component re-renders. Memoization moves the creation cost outside the render cycle and enables proper optimization. Good: ```tsx @@ -136,6 +142,7 @@ Bad: ### [PERF-6] Use shallow comparisons instead of deep comparisons - **Severity**: SHOULD - **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. +- **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 @@ -159,6 +166,7 @@ memo(ReportActionItem, (prevProps, nextProps) => ### [PERF-7] Use specific properties as hook dependencies - **Severity**: SHOULD - **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 From 92052f6a0bb76f298f952f09056c1dbcc68b4615 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 6 Aug 2025 10:56:13 +0200 Subject: [PATCH 08/13] docs: rm the severity level property from rules --- contributingGuides/review/RULES.md | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 49e2d5b8cb33..6619301d8a74 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -3,9 +3,6 @@ These rules are used to conduct structured code reviews on pull request diffs. Each rule includes: - A unique **Rule ID** -- **Severity level**: - - MUST: Mandatory to update this as it causes issues. - - SHOULD: Strong recommendation to consider changing this and think about future implications. - **Pass/Fail condition** - **Reasoning**: Technical explanation of why the rule is important - Examples of good and bad usage @@ -15,7 +12,6 @@ These rules are used to conduct structured code reviews on pull request diffs. E ## Performance Rules ### [PERF-1] No spread in list item's renderItem -- **Severity**: MUST - **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. @@ -42,7 +38,6 @@ Bad: --- ### [PERF-2] Use early returns in array iteration methods -- **Severity**: SHOULD - **Condition**: When using `.every()`, `.some()`, or similar methods, perform simple checks first with early returns before expensive operations. - **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. @@ -66,7 +61,6 @@ const areAllTransactionsValid = transactions.every((transaction) => { --- ### [PERF-3] Use OnyxListItemProvider hooks instead of useOnyx in renderItem -- **Severity**: MUST - **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. @@ -85,7 +79,6 @@ const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); --- ### [PERF-4] Memoize objects and functions passed as props -- **Severity**: MUST - **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. @@ -110,7 +103,6 @@ return --- ### [PERF-5] Avoid inline object/array/function creation in JSX -- **Severity**: SHOULD - **Condition**: Objects, arrays, and functions should not be created inline in JSX. Use `useMemo` or `useCallback` to memoize them when there's actual performance benefit. - **Reasoning**: Inline creation generates new instances on every render, breaking React's reconciliation optimizations and forcing child component re-renders. Memoization moves the creation cost outside the render cycle and enables proper optimization. @@ -140,7 +132,6 @@ Bad: --- ### [PERF-6] Use shallow comparisons instead of deep comparisons -- **Severity**: SHOULD - **Condition**: In `React.memo` and similar optimization functions, compare only specific relevant properties instead of using deep equality checks. - **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. @@ -164,7 +155,6 @@ memo(ReportActionItem, (prevProps, nextProps) => --- ### [PERF-7] Use specific properties as hook dependencies -- **Severity**: SHOULD - **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. From 55331f1022e21b35f58b5737210646859462a456 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 6 Aug 2025 11:08:48 +0200 Subject: [PATCH 09/13] docs: rm the rule on inline jsx --- contributingGuides/review/RULES.md | 33 ++---------------------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 6619301d8a74..4ae2984b3316 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -102,36 +102,7 @@ return --- -### [PERF-5] Avoid inline object/array/function creation in JSX -- **Condition**: Objects, arrays, and functions should not be created inline in JSX. Use `useMemo` or `useCallback` to memoize them when there's actual performance benefit. -- **Reasoning**: Inline creation generates new instances on every render, breaking React's reconciliation optimizations and forcing child component re-renders. Memoization moves the creation cost outside the render cycle and enables proper optimization. - -Good: -```tsx -const reportActionItemStyle = useMemo(() => [styles.container, styles.flex], []); -const handleSelect = useCallback(() => { - onSelectRow(item); -}, [onSelectRow, item]); - - -``` - -Bad: -```tsx - onSelectRow(item)} - reportID={report.reportID} -/> -``` - ---- - -### [PERF-6] Use shallow comparisons instead of deep comparisons +### [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. - **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. @@ -154,7 +125,7 @@ memo(ReportActionItem, (prevProps, nextProps) => --- -### [PERF-7] Use specific properties as hook dependencies +### [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. From 4a67922a8b839a80b36de98ca4ee3f8d337743b4 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 6 Aug 2025 11:36:19 +0200 Subject: [PATCH 10/13] docs: instruct to include a comment per rule violation --- contributingGuides/review/RULES.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index 4ae2984b3316..bf5452cc0c5b 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -1,7 +1,9 @@ # ✅ AI Code Review Rules -These rules are used to conduct structured code reviews on pull request diffs. Each rule includes: +These rules are used to conduct structured code reviews on pull request diffs. +Very important: in your review, make sure you include a separate comment for every rule violation. +Each rule includes: - A unique **Rule ID** - **Pass/Fail condition** - **Reasoning**: Technical explanation of why the rule is important From 8646f575afdad71187eb6aa13fbfbe12e7e5b05c Mon Sep 17 00:00:00 2001 From: Adam Horodyski <42047036+adhorodyski@users.noreply.github.com> Date: Wed, 6 Aug 2025 18:28:31 +0200 Subject: [PATCH 11/13] chore: remove redundant comment lines --- contributingGuides/review/RULES.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index bf5452cc0c5b..f6c27aeef9e5 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -68,13 +68,11 @@ const areAllTransactionsValid = transactions.every((transaction) => { Good: ```tsx -// ReportActionItem.tsx const personalDetails = usePersonalDetails(); ``` Bad: ```tsx -// ReportActionItem.tsx const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST); ``` From 2ec8d709a65e80d4cb2347126581ae563312fd48 Mon Sep 17 00:00:00 2001 From: Adam Horodyski <42047036+adhorodyski@users.noreply.github.com> Date: Wed, 6 Aug 2025 18:32:51 +0200 Subject: [PATCH 12/13] chore: list very important review guidelines, including the ruleID requirement --- contributingGuides/review/RULES.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index f6c27aeef9e5..b3eaa844c755 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -1,7 +1,6 @@ # ✅ AI Code Review Rules These rules are used to conduct structured code reviews on pull request diffs. -Very important: in your review, make sure you include a separate comment for every rule violation. Each rule includes: - A unique **Rule ID** @@ -9,6 +8,10 @@ Each rule includes: - **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 From b6d61bcfffb46ab28608bda7566d35c3a1a76347 Mon Sep 17 00:00:00 2001 From: Adam Horodyski Date: Wed, 6 Aug 2025 18:41:55 +0200 Subject: [PATCH 13/13] chore: fix inline returns to follow the styleguide --- contributingGuides/review/RULES.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/contributingGuides/review/RULES.md b/contributingGuides/review/RULES.md index b3eaa844c755..6d075e268be2 100644 --- a/contributingGuides/review/RULES.md +++ b/contributingGuides/review/RULES.md @@ -47,16 +47,18 @@ Bad: - **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: -```tsx +```ts const areAllTransactionsValid = transactions.every((transaction) => { - if (!transaction.rawData || transaction.amount <= 0) return false; + if (!transaction.rawData || transaction.amount <= 0) { + return false; + } const validation = validateTransaction(transaction); return validation.isValid; }); ``` Bad: -```tsx +```ts const areAllTransactionsValid = transactions.every((transaction) => { const validation = validateTransaction(transaction); return validation.isValid;