perf: Migrate to Flashlist in MoneyRequestReportTransactionList#91422
perf: Migrate to Flashlist in MoneyRequestReportTransactionList#91422TMisiukiewicz wants to merge 77 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@aimane-chnaif 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b5c9bef51
ℹ️ 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".
|
|
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
8fdb7c6 to
a579798
Compare
…rt-list # Conflicts: # src/components/Search/index.tsx
|
@aimane-chnaif could you do another round of tests please, just remember about reinstalling the packages after switching the branch as you need a patch applied |
e8cf35f to
0a8bcf2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8bcf28b6
ℹ️ 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 scrollToBottom = useCallback(() => { | ||
| if (lastItemIndexRef.current < 0) { | ||
| return; | ||
| } | ||
|
|
||
| const listRef = reportScrollManager.ref; | ||
| listRef?.current?.scrollToIndex({index: lastItemIndexRef.current, animated: false}); |
There was a problem hiding this comment.
Route send-message autoscroll through indexed bottom jump
This new helper avoids scrollToEnd because large transaction+chat lists can land on a blank estimated end, but it is only used by the latest-messages pill. When the current user sends an ADD_COMMENT, scrollToBottomForCurrentUserAction still calls reportScrollManager.scrollToEnd(), so the send-message autoscroll keeps the same large-report failure mode after this migration; route that path through this indexed bottom jump as well.
Useful? React with 👍 / 👎.
| const reportActionItems: UnifiedListItem[] = visibleReportActions.map((action) => ({type: 'report-action', action})); | ||
| const data: UnifiedListItem[] = shouldInlineTransactions ? [...controller.transactionListItems, TRANSACTIONS_FOOTER_ITEM, ...reportActionItems] : reportActionItems; |
There was a problem hiding this comment.
Preserve the loading skeleton when only actions are empty
When transactions are inlined, data is populated with transaction rows and the footer even while visibleReportActions is still empty during initial action loading. That prevents the ListEmptyComponent skeleton from rendering, whereas the previous FlatList had empty action data and showed ReportActionsListLoadingSkeleton below the transaction section; reports with existing expenses but still-loading comments now show no loading state for the actions.
Useful? React with 👍 / 👎.
Review — FlashList migration for the money-request report listReviewed the whole diff, focusing on the parts the bots didn't cover: the new 🔴 Deep-link / open-at-action anchoring looks like a real regression
🟠
|
|
Please check this deeplink bug. The video shows comparison between this branch vs staging bug.mov |
|
@aimane-chnaif any specific repro steps I should follow? I cannot reproduce it unfortunately bug.mov |
|
@TMisiukiewicz what is test email? I can invite to that report |
|
@aimane-chnaif this should be fixed, could you retest please? |
|
Scroll issue fixed but there's still flicker which doesn't happen in production flicker.mov |
|
🚧 mountiny 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! 🧪🧪
|
Explanation of Change
Problem
On heavy money-request reports the RHP list was effectively non-virtualised. The inner transactions section was rendered via plain
.map()inside the parent FlatList'sListHeaderComponent, which renders eagerly. Every transaction row mounted synchronously when the report opened, pushing report open time to ~20–30s for a report with 1600 expenses and producing a noticeable jank window on every navigation.Solution
Unified list — merged the inner transactions section and the outer report-actions section into a single virtualised list. A discriminated-union
UnifiedListItem(section-header|transaction|transactions-footer|report-action) flows through onerenderItemdispatcher, so transactions and chat messages now share the same recycler window instead of one being eagerly rendered inside the other's header.FlashList — migrated the list from
react-native'sFlatList(FlatListWithScrollKey).initialScrollIndexreplaces the FlatList-specificuseFlatListScrollKeywrapperController + sub-component —
MoneyRequestReportTransactionListcontinues to own the transaction-domain state (sort, group-by, selection, columns, totals, etc.) and exposes it to the parent via a render-prop controller. The parent renders a smallMoneyRequestReportUnifiedListsub-component that assembles the FlashList from controller data plus the report-actions listFixed Issues
$ #91425
PROPOSAL:
Tests
Before testing:
npm run i-standaloneas this PR contains patch to the FlashListOffline tests
N/A
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.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov