perf: Split TransactionItemRow into narrow and wide versions#89120
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
jmusial
changed the title
|
@codex review |
![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: 7d8ee4fda6
ℹ️ 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".
| taxAmountColumnSize={CONST.SEARCH.TABLE_COLUMN_SIZES.NORMAL} | ||
| shouldShowCheckbox={!!canSelectMultiple} | ||
| checkboxSentryLabel={CONST.SENTRY_LABEL.SEARCH.TRANSACTION_LIST_ITEM_CHECKBOX} | ||
| style={[styles.p3, styles.pv2, shouldUseNarrowLayout ? [styles.p0, styles.pt3] : [styles.noBorderRadius]]} |
There was a problem hiding this comment.
The narrow list path no longer includes styles.noBorderRadius when rendering TransactionItemRow, but TransactionItemRowNarrow still applies styles.expenseWidgetRadius internally. On small screens this reintroduces rounded inner corners in hydrated search rows, while the static placeholder list still uses square corners, causing a visible shape jump when the real list replaces the static one and breaking the stated narrow-layout visual parity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not valid. No shape jump occurs in practice. The radius would only become visible on a selected row, which isn't the scenario (inner bgActiveStyles is empty for unselected rows, so expenseWidgetRadius has no background to clip and is visually a no-op)
| isDescription={!merchant} | ||
| /> | ||
| ) : null} | ||
| <View style={[styles.flexRow, styles.alignItemsCenter, styles.gap2]}> |
There was a problem hiding this comment.
The trailing amount/comments container is always rendered without the previous conditional mlAuto, so when merchantOrDescription is empty the row has only one child in this justifyContentBetween section and it sits at the left instead of the right. This misaligns amount/comment content for transactions without merchant/description (common for partial scans or missing fields).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
True, the !merchantOrDescription && styles.mlAuto was added on main after the split of TransactionItemRow, so the merge didn't carry it over
melvin-bot
Bot
requested review from
ChavdaSachin and
heyjennahay
and removed request for
a team
|
@ChavdaSachin 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] |
|
I'll be OOO until 10 May. @OlGierd03 will be looking after this PR until then. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| return <ChatBubbleCell {...props} />; | ||
| } |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react/jsx-props-no-spreading lacks a comment explaining why the rule is disabled.
Add a justification comment:
// Deferred wrapper intentionally forwards all props to the underlying component
// eslint-disable-next-line react/jsx-props-no-spreading
return <ChatBubbleCell {...props} />;Reviewed at: 7d8ee4f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| } | ||
|
|
||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| return <TransactionItemRowRBR {...props} />; |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react/jsx-props-no-spreading lacks a comment explaining why the rule is disabled.
Add a justification comment:
// Deferred wrapper intentionally forwards all props to the underlying component
// eslint-disable-next-line react/jsx-props-no-spreading
return <TransactionItemRowRBR {...props} />;Reviewed at: 7d8ee4f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| import DeferredTransactionItemRowRBR from './DeferredTransactionItemRowRBR'; | ||
| import type {TransactionItemRowNarrowComputedData, TransactionItemRowProps} from './types'; | ||
|
|
||
| type TransactionItemRowNarrowProps = Omit<TransactionItemRowProps, 'shouldUseNarrowLayout' | 'policyForMovingExpenses'> & TransactionItemRowNarrowComputedData; |
There was a problem hiding this comment.
❌ CONSISTENCY-4 (docs)
TransactionItemRowNarrowProps is defined as Omit<TransactionItemRowProps, 'shouldUseNarrowLayout' | 'policyForMovingExpenses'> & TransactionItemRowNarrowComputedData, which pulls in many wide-layout-only props that the narrow component never destructures or uses: dateColumnSize, submittedColumnSize, approvedColumnSize, postedColumnSize, exportedColumnSize, amountColumnSize, taxAmountColumnSize, isReportItemChild, isActionColumnWide, isHover, isLargeScreenWidth, onButtonPress, isActionLoading, shouldHighlightItemWhenSelected, columns, reportActions, nonPersonalAndWorkspaceCards, and policy.
Define a narrow-specific props type using Pick that only includes props the component actually uses:
type TransactionItemRowNarrowProps = Pick<TransactionItemRowProps,
'transactionItem' | 'report' | 'isSelected' | 'shouldShowTooltip' |
'onCheckboxPress' | 'shouldShowCheckbox' | 'style' |
'isInSingleTransactionReport' | 'shouldShowRadioButton' |
'onRadioButtonPress' | 'shouldShowErrors' | 'isDisabled' |
'violations' | 'shouldShowBottomBorder' | 'onArrowRightPress' |
'shouldShowArrowRightOnNarrowLayout' | 'checkboxSentryLabel'
> & TransactionItemRowNarrowComputedData;This also means the parent dispatcher should construct narrow-only props instead of spreading all sharedProps.
Reviewed at: 7d8ee4f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I changed the narrow type to a Pick of the props that are actually used. I also dropped the now unused isDeletedTransaction from the computed data type, since neither variant reads it (the TransactionItemRowWide now computes it within the function)
|
@OlGierd03, please take a look at the AI review comments and let me know when the PR is ready for the review. |
|
Sure thing, I'll get around to it soon |
|
Sorry for the delay. I haven't had time to work on this over the last few days. I'm getting right to it now, though |
There was a problem hiding this comment.
Pull request overview
This PR optimizes large transaction search/result lists by splitting TransactionItemRow and TransactionListItem into Narrow (mobile) and Wide (table) variants, and by deferring heavier subtrees (action cell, comment bubble, and RBR row) to improve first-paint responsiveness.
Changes:
- Added viewport-specific
TransactionItemRowNarrow/TransactionItemRowWideand updated theTransactionItemRowdispatcher to compute and forward layout-specific data. - Split
TransactionListItemintoTransactionListItemNarrow/TransactionListItemWidewith a new dispatcher entrypoint. - Introduced deferred-render wrappers (
DeferredActionCell,DeferredChatBubbleCell,DeferredTransactionItemRowRBR) to delay expensive UI until after initial paint.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/TransactionItemRow/types.ts | Introduces shared and variant-specific prop/computed-data types for the split row implementations. |
| src/components/TransactionItemRow/TransactionItemRowWide.tsx | New wide/table implementation for transaction rows (columns-based rendering + deferred cells). |
| src/components/TransactionItemRow/TransactionItemRowNarrow.tsx | New narrow/mobile implementation for transaction rows (compact stacked layout + deferred cells). |
| src/components/TransactionItemRow/index.tsx | Refactors the original component into a dispatcher that computes shared data and selects narrow vs wide variants. |
| src/components/TransactionItemRow/DeferredTransactionItemRowRBR.tsx | Defers rendering of the RBR (violation/error) row. |
| src/components/TransactionItemRow/DataCells/TransactionDataCellProps.ts | Updates the TransactionWithOptionalSearchFields type import to the new types module. |
| src/components/TransactionItemRow/DataCells/DeferredChatBubbleCell.tsx | Defers chat-bubble rendering with a skeleton placeholder. |
| src/components/Search/SearchList/ListItem/TransactionListItem/types.ts | Adds shared narrow/wide prop types for split transaction list items. |
| src/components/Search/SearchList/ListItem/TransactionListItem/TransactionListItemWide.tsx | New wide/table list-item wrapper that renders a transaction row in table layout. |
| src/components/Search/SearchList/ListItem/TransactionListItem/TransactionListItemNarrow.tsx | New narrow/mobile list-item wrapper that includes the user info row + narrow transaction row. |
| src/components/Search/SearchList/ListItem/TransactionListItem/index.tsx | New dispatcher that chooses the narrow vs wide list-item implementation based on responsive layout. |
| src/components/Search/SearchList/ListItem/TransactionListItem.tsx | Removes the previous combined implementation now replaced by the dispatcher + variants. |
| src/components/Search/SearchList/ListItem/ActionCell/DeferredActionCell.tsx | Defers action button rendering with a skeleton placeholder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {!shouldShowRadioButton && ( | ||
| <Checkbox | ||
| disabled={isDisabled} | ||
| onPress={() => { | ||
| onCheckboxPress(transactionItem.transactionID); | ||
| }} | ||
| accessibilityLabel={CONST.ROLE.CHECKBOX} | ||
| isChecked={isSelected} | ||
| containerStyle={styles.m0} | ||
| wrapperStyle={styles.justifyContentCenter} | ||
| sentryLabel={checkboxSentryLabel} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
@OlGierd03 I guess this is expected, could you add an explanation comment maybe?
There was a problem hiding this comment.
Removed shouldShowCheckbox from TransactionItemRowWideProps via Omit
| case CONST.SEARCH.TABLE_COLUMNS.TO: | ||
| return ( | ||
| <View | ||
| key={column} | ||
| style={[StyleUtils.getReportTableColumnStyles(CONST.SEARCH.TABLE_COLUMNS.FROM)]} | ||
| > | ||
| {!!transactionItem.to && ( | ||
| <UserInfoCell | ||
| accountID={transactionItem.to.accountID} | ||
| avatar={transactionItem.to.avatar} | ||
| displayName={transactionItem.formattedTo ?? transactionItem.to.displayName ?? ''} | ||
| isLargeScreenWidth | ||
| /> | ||
| )} | ||
| </View> |
There was a problem hiding this comment.
Even though TO and FROM both columns resolve to the same style at the end, this suggestion is valid.
@OlGierd03 please make this change.
| styles.flex1, | ||
| animatedHighlightStyle, | ||
| styles.userSelectNone, | ||
| isLastItem && [styles.searchTableBottomRadius, styles.overflowHidden, styles.searchTableBottomRadius], |
| isFirstItem && [styles.searchTableTopRadius, styles.overflowHidden, styles.searchTableTopRadius], | ||
| isLastItem && [styles.searchTableBottomRadius, styles.overflowHidden, styles.searchTableBottomRadius], |
|
Reviewing ♻️ |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp
Android: mWeb ChromeiOS: HybridApp
iOS: mWeb SafariMacOS: Chrome / Safari
|
|
@OlGierd03 can you please address the co-pilot comments? |
|
@luacmartins addressed |
|
@jmusial performance test is failing |
|
@luacmartins all passing now |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.74-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This is an internal performance optimization that splits |
|
Deploy Blocker #90587 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.74-7 🚀
|
Explanation of Change
Split
TransactionItemRow(the inner data-row) andTransactionListItem(the outer search-list item) into viewport-specific variants — *Wide (table layout) and *Narrow (mobile layout) — so each viewport only loads the code, column renderers, and dependencies it actually needs.Also introduced three deferred-rendering wrappers.
DeferredActionCell— renders a pulsing skeleton on first paint, then swaps in the real ActionCell after a transitionDeferredChatBubbleCell— same pattern for the comment-count bubble iconDeferredTransactionItemRowRBR— defers the RBR (red-brick-road) violation/error rowThis keeps large search result lists responsive on initial render by letting React paint the visible shell immediately and hydrate heavier sub-trees in a lower-priority pass.
Note: this PR overlaps with #89083
Perf gains
Fixed Issues
$ #89123
PROPOSAL:
Tests
Offline tests
Same as 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
MacOS: Chrome / Safari
Screen.Recording.2026-04-29.at.15.56.23.mov