Callstack internal/szymonzalarski/optimize transaction item row#83340
Conversation
…ymonzalarski/optimize-transaction-item-row # Conflicts: # src/components/TransactionItemRow/DataCells/TypeCell.tsx
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@codex review |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33f2c2da97
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!violations?.length && !missingFieldError) { | ||
| return null; |
There was a problem hiding this comment.
This early return hides valid red-brick-road messages for transactions that have transaction.errors (or thread action errors) but no violations and no missingFieldError. ViolationsUtils.getRBRMessages() explicitly builds messages from those error objects, so in cases like failed receipt upload/save where violations are empty, the row now renders nothing and users lose the only visible failure feedback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right, I need to modify the PR
|
PR doesn’t need product input as a performance PR. Unassigning and unsubscribing myself. |
| const [cardList] = useOnyx(ONYXKEYS.CARD_LIST); | ||
| const cardSelector = useCallback((cards: OnyxEntry<CardList>): OnyxEntry<Card> => cards?.[transactionItem.cardID ?? CONST.DEFAULT_NUMBER_ID], [transactionItem.cardID]); | ||
| const [card] = useOnyx(ONYXKEYS.CARD_LIST, {selector: cardSelector}); | ||
| const cardList = transactionItem.cardID && card ? {[transactionItem.cardID]: card} : undefined; |
There was a problem hiding this comment.
Maybe we can just update getTransactionType to pass only the necessary card there, instead of the cardList?
There was a problem hiding this comment.
Good suggestion! Changing getTransactionType to accept a single Card instead of CardList would clean this up, but it has 6+ callers across the codebase (TransactionPreviewUtils, TransactionPreview, IOU/Duplicate, SplitExpensePage, tests). I'd prefer to do that as a follow-up to keep this PR focused on the perf optimization.
| function TypeCell({transactionItem, shouldUseNarrowLayout, shouldShowTooltip}: TransactionDataCellProps) { | ||
| const {translate} = useLocalize(); | ||
| const [cardList] = useOnyx(ONYXKEYS.CARD_LIST); | ||
| const cardSelector = useCallback((cards: OnyxEntry<CardList>): OnyxEntry<Card> => cards?.[transactionItem.cardID ?? CONST.DEFAULT_NUMBER_ID], [transactionItem.cardID]); |
There was a problem hiding this comment.
let's make this compiler compliant, can we remove the selector to live outside of React? otherwise, it should be ok to not call useCallback.
There was a problem hiding this comment.
You're right, removed
|
|
||
| TransactionItemRowRBRInner.displayName = 'TransactionItemRowRBRInner'; | ||
|
|
||
| const MemoizedTransactionItemRowRBRInner = React.memo(TransactionItemRowRBRInner); |
There was a problem hiding this comment.
why use manual memo? Compiler should handle that.
There was a problem hiding this comment.
You're right, I thought React Compiler can't do memo for a whole component, asked Opus to be sure and he agreed on that, but now he has a different opinion 😂
|
|
||
| const MemoizedTransactionItemRowRBRInner = React.memo(TransactionItemRowRBRInner); | ||
|
|
||
| function TransactionItemRowRBR({transaction, violations, report, containerStyles, missingFieldError}: TransactionItemRowRBRProps) { |
There was a problem hiding this comment.
you could make this component more simple by only passing in violations and missingFieldError, and rendering null or children like that:
function RowRBRGuard({x, children}) {
if (x) {
return null;
}
return children;
}<RowRBRGuard x={}>
<RowRBR transaction={} ... />
</RowRBR>There was a problem hiding this comment.
The Guard pattern with children would also prevent mounting. However, it would require duplicating props (once to Guard for the check, once to Inner for rendering), while the current wrapper encapsulates both concerns in a single component with a cleaner call site. The wrapper also needs 1 useOnyx to check thread action errors (not available from props), which would be needed in a Guard too. Happy to refactor if you feel strongly about it though.
…d refining TypeCell logic for better error handling and performance.
…ymonzalarski/optimize-transaction-item-row # Conflicts: # src/components/TransactionItemRow/TransactionItemRowRBR.tsx
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f752e5599d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const transactionThreadId = reportActions ? getIOUActionForTransactionID(reportActions, transaction.transactionID)?.childReportID : undefined; | ||
| const [transactionThreadActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${transactionThreadId}`); |
There was a problem hiding this comment.
This guard now derives transactionThreadId only from the optional reportActions prop, so when callers do not pass that prop (I checked src/components/MoneyRequestReportView/MoneyRequestReportTransactionItem.tsx:138-158 and src/pages/UnreportedExpenseListItem.tsx:74-90), hasThreadErrors stays false and the component can return null before mounting TransactionItemRowRBRInner. In that case, thread-only report action errors are never discovered or rendered, even though they were previously shown via the inner component’s Onyx subscriptions.
Useful? React with 👍 / 👎.
| function TypeCell({transactionItem, shouldUseNarrowLayout, shouldShowTooltip}: TransactionDataCellProps) { | ||
| const {translate} = useLocalize(); | ||
| const [cardList] = useOnyx(ONYXKEYS.CARD_LIST); | ||
| const cardSelector = (cards: OnyxEntry<CardList>): OnyxEntry<Card> => cards?.[transactionItem.cardID ?? CONST.DEFAULT_NUMBER_ID]; |
There was a problem hiding this comment.
i think that selector is used in deps array for useMemo inside useOnyx
i think we need to wrap this in useCallback?
`
const selector = useMemo(() => {
if (!selectorProp || !shouldUseSnapshot) {
return selectorProp;
}
return (data: OnyxValue<OnyxKey> | undefined) => selectorProp(getKeyData(data as SearchResults, key));
}, [selectorProp, shouldUseSnapshot, key]);`
| }; | ||
|
|
||
| function TransactionItemRowRBR({transaction, violations, report, containerStyles, missingFieldError}: TransactionItemRowRBRProps) { | ||
| type TransactionItemRowRBRProps = TransactionItemRowRBRInnerProps & { |
There was a problem hiding this comment.
I understand the performance gains, do we really have to pass in this whole object though? New [] will always pop up here and there. I think this is just a violating of our clean react patterns' rules.
5 participants
Explanation of Change
TransactionItemRowRBR mounts for every transaction row and creates 6 Onyx subscriptions per instance — even when there are no violations to display. With 50 rows, that's 300 wasted Onyx subscriptions. Additionally, TypeCell subscribes to the entire CARD_LIST collection in every row, so any card change re-renders all 50 TypeCells.
This PR makes three changes:
Measured results:
491ms → 460ms (-6.3% ManualNavigateToReportsTab)
Onyx subscriptions reduced by 78% (138 → 30).
At 50 transactions (~10% with violations), projected 90% reduction in RBR-related Onyx subscriptions (300 → ~30).
Fixed Issues
$ #82865
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari