perf: Split ExpenseReportListItemRow, defer ActionCell, extract Avatar#89083
Conversation
|
@codex review |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ffed6fae7
ℹ️ 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".
| <PulsingView | ||
| shouldPulse | ||
| style={styles.w100} | ||
| > | ||
| <View style={[styles.w100, sizeStyle, {backgroundColor: theme.skeletonLHNIn}]} /> | ||
| </PulsingView> |
There was a problem hiding this comment.
While DeferredActionCell shows the pulsing placeholder, the action column is no longer an interactive control, so clicks/taps in that area fall through to the enclosing list-row press handler (BaseListItem’s onPress) and open/select the row instead of running the row action. This is most visible on slower devices or busy initial renders where the transition keeps the skeleton visible longer, and it can trigger the wrong user action (e.g., opening the report when the user intended to approve/pay).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
When skeleton shows there is no Action showed on a button just skeleton, we can assume that row click is with intention to see more details.
|
@DylanDylann 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. |
| isHovered={hovered} | ||
| isFocused={isFocused} | ||
| isPendingDelete={isPendingDelete} | ||
| isLargeScreenWidth={isLargeScreenWidth} |
There was a problem hiding this comment.
NIT: Let’s remove the isLargeScreenWidth prop and handle the subscription inside the ExpenseReportListItemRow component.
|
|
||
| return ( | ||
| <View | ||
| style={[styles.flexRow, styles.alignItemsCenter, styles.gap3, styles.pt3]} |
There was a problem hiding this comment.
Why do we need to add styles.pt3? This style seems new compared to before.
There was a problem hiding this comment.
Before this PR pt3 was applied by the parent ExpenseReportListItem via a wrapping <View style={styles.pt3}> around the narrow render (1c4c652). Spacing is unchanged
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efbf5ab16d
ℹ️ 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".
| <PulsingView | ||
| shouldPulse | ||
| style={styles.w100} | ||
| > | ||
| <View style={[styles.w100, sizeStyle, {backgroundColor: theme.skeletonLHNIn}]} /> |
There was a problem hiding this comment.
Returning a non-interactive skeleton here removes the action button’s press handler during the initial deferred window. In BaseListItem, taps inside the row bubble to the row onPress, so clicking the action column before ActionCell mounts will navigate/select the row instead of performing the intended action. This creates a user-visible behavior regression on first paint (and each virtualized remount) for anyone who taps quickly.
Useful? React with 👍 / 👎.
|
@jmusial Let’s discuss the use of DeferredActionCell. While it helps improve list rendering performance, it will cause flickering when users switch between lists. I’d like to hear other thoughts on this change. cc @luacmartins @Expensify/design Before Screen.Recording.2026-05-03.at.17.56.14.movAfter Screen.Recording.2026-05-03.at.17.55.43.mov |
|
Yeah same as design. If there's a benefit with going with 1 over 2, then that makes sense to me. |
|
@jmusial Your solution looks great. Could you merge the latest main and complete the PR and ping me for the final review? |
There was a problem hiding this comment.
Pull request overview
This PR refactors the expense report search list row to improve perceived performance in large lists by separating wide vs. narrow row implementations, extracting the avatar rendering, and deferring the heavier action-button cell behind an initial placeholder.
Changes:
- Split
ExpenseReportListItemRowinto a dispatcher (index.tsx) withExpenseReportListItemRowWideandExpenseReportListItemRowNarrow. - Extracted the avatar rendering into
ExpenseReportListItemAvatar. - Added
DeferredActionCelland wired it into the wide layout to defer rendering the realActionCelluntil after initial paint.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Search/SearchList/ListItem/ExpenseReportListItemRow/types.ts | Introduces shared prop types for the new narrow/wide row split. |
| src/components/Search/SearchList/ListItem/ExpenseReportListItemRow/index.tsx | Dispatcher component that selects wide vs. narrow layout based on responsive breakpoint. |
| src/components/Search/SearchList/ListItem/ExpenseReportListItemRow/ExpenseReportListItemRowWide.tsx | Wide/table implementation; uses extracted avatar component and DeferredActionCell. |
| src/components/Search/SearchList/ListItem/ExpenseReportListItemRow/ExpenseReportListItemRowNarrow.tsx | New narrow/mobile row implementation with compact summary/accessibility label. |
| src/components/Search/SearchList/ListItem/ExpenseReportListItemRow/ExpenseReportListItemAvatar.tsx | Extracted avatar rendering + border-color logic. |
| src/components/Search/SearchList/ListItem/ExpenseReportListItem.tsx | Simplifies rendering to always use ExpenseReportListItemRow (dispatcher handles layout). |
| src/components/Search/SearchList/ListItem/ActionCell/DeferredActionCell.tsx | Adds deferred rendering wrapper with an initial disabled placeholder button. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ExpenseReportListItemRowNarrow from './ExpenseReportListItemRowNarrow'; | ||
| import ExpenseReportListItemRowWide from './ExpenseReportListItemRowWide'; | ||
| import type {ExpenseReportListItemRowProps} from './types'; | ||
|
|
||
| function ExpenseReportListItemRow(props: ExpenseReportListItemRowProps) { | ||
| const {isLargeScreenWidth} = useResponsiveLayout(); | ||
|
|
||
| if (isLargeScreenWidth) { | ||
| return ( | ||
| <ExpenseReportListItemRowWide | ||
| item={props.item} | ||
| reportActions={props.reportActions} | ||
| showTooltip={props.showTooltip} | ||
| canSelectMultiple={props.canSelectMultiple} | ||
| isActionLoading={props.isActionLoading} | ||
| onButtonPress={props.onButtonPress} | ||
| onCheckboxPress={props.onCheckboxPress} | ||
| containerStyle={props.containerStyle} | ||
| isSelectAllChecked={props.isSelectAllChecked} | ||
| isIndeterminate={props.isIndeterminate} | ||
| isDisabledCheckbox={props.isDisabledCheckbox} | ||
| isHovered={props.isHovered} | ||
| isFocused={props.isFocused} | ||
| isPendingDelete={props.isPendingDelete} | ||
| columns={props.columns} | ||
| /> | ||
| ); | ||
| } | ||
| return ( | ||
| <ExpenseReportListItemRowNarrow | ||
| item={props.item} | ||
| onCheckboxPress={props.onCheckboxPress} | ||
| canSelectMultiple={props.canSelectMultiple} | ||
| isSelectAllChecked={props.isSelectAllChecked} | ||
| isIndeterminate={props.isIndeterminate} | ||
| isDisabledCheckbox={props.isDisabledCheckbox} | ||
| /> |
There was a problem hiding this comment.
Bundle splitting is not used for App and AFAIK we do not want to do this
| function ExpenseReportListItemAvatar({item, showTooltip, isHovered = false, isFocused = false, isLargeScreenWidth = false}: ExpenseReportListItemAvatarProps) { | ||
| const StyleUtils = useStyleUtils(); | ||
| const styles = useThemeStyles(); | ||
| const theme = useTheme(); | ||
|
|
There was a problem hiding this comment.
React compiler handles memoization
|
@jmusial Could you check the failed Reassure Performance Tests? Is this intentional? |
|
@DylanDylann checking, but I think there weren't failing before latest merge |
This is the error And I think this is on |
|
@mountiny Could you trigger again the Reassure Performance Tests? |
|
@DylanDylann pushed empty commit to retrigger 😄 |
|
@MelvinBot Could you investigate why the Reassure Performance Tests failed? I suspect that using useDeferredValue is causing a change in the render count. |
|
Your suspicion is correct — What failed: The Why: Investigation detailsThe Reassure validation at The const shouldRender = useDeferredValue(true, false);
// First render: shouldRender = false → skeleton button
// Deferred render: shouldRender = true → real ActionCellThis is the only new Options to fix:
|
|
@MelvinBot I doubt :
Also getting green on local, re-merged latest main, possibly was still out of date before. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-12.at.17.09.39.movAndroid: mWeb ChromeScreen.Recording.2026-05-12.at.17.05.51.moviOS: HybridAppScreen.Recording.2026-05-12.at.17.06.26.moviOS: mWeb SafariScreen.Recording.2026-05-12.at.17.05.10.movMacOS: Chrome / SafariScreen.Recording.2026-05-12.at.17.02.43.mov |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.73-0 🚀
Bundle Size Analysis (Sentry): |
|
This PR is a purely internal performance refactoring — it splits There are no user-facing feature changes, new UI text, behavioral changes, or navigation flow updates. No help site changes are required. |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.73-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.73-4 🚀
|
Explanation of Change
Split
ExpenseReportListItemRow.tsxinto a thin dispatcher that picks betweenExpenseReportListItemRowWide(table layout) andExpenseReportListItemRowNarrow(mobile layout) so each viewport only loads the code and dependencies it needs. Also extracted the avatar into a memoizedExpenseReportListItemAvatar] and introducedDeferredActionCell, which renders a skeleton on first paint and swaps in the realActionCellafter a transition to keep large lists responsive on initial render.Perf gains
Big percived gain for web at a cost of button being behind skeleton on initial render.
Fixed Issues
$ #89100
PROPOSAL:
Tests
Spend/ExpensesSpend/ReportsOffline 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-28.at.13.08.47.mov