Repository navigation
[LegendList 4/7] feat: migrate report actions to chronological LegendList - #100733
chrispader wants to merge 68 commits into
Conversation
1bac411 to
c067b0c
Compare
c067b0c to
e784ebe
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f616302cdb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f616302 to
2a3323a
Compare
|
|
|
@chrispader I found some bugs here: #100733 (comment). I'm still testing on my side and will report any additional bugs I find. Sorry for not being able to report them all at once. |
Yes, I can still reproduce it. If you're still unable to reproduce it, I think we can request an adhoc build and test it on a physical device |
Clearing the linked-action route after sending remounted the report content and LegendList, briefly restoring the loading skeleton and resetting composer state. Keep the report content mounted while resetting only the link checks, and remount the list only when opening a linked action or completing initial hydration.
|
Chris's AI wingmate checking in on his behalf. 🤖 Six fixes for the latest review findings have been committed and pushed to this PR through
On October 1, all 133 focused regression tests and the required local checks passed. The linked-message send flow passed on both iOS and Android simulators without the skeleton flash, and the keyboard stayed visible. The Android delete-then-scroll flow also stayed on older messages. Linked targets below long comments and editing a long expense-chat comment passed with local test fixtures. Chris still needs to do final testing, including the full web PDF/hover flows, Concierge with substantial history, and the remaining keyboard/navigation transitions from the review recordings. Those broader reports are not all verified yet, and the performance benchmark request remains open. |
|
Still working on the remaining issues 👀 |
Suppress hover throughout scrolling in the shared list wrappers after the chronological migration. Restore report-list render benchmarks and exercise the production start-reached pagination callback.
|
Chris's AI toolbox buddy here. 🤖 First review by @dmkt9
✅ Fixed! Screen.Recording.2026-10-09.at.13.00.27.mov__
✅ Fixed! Screen.Recording.iPhone.17.Pro.09-10-2026.at.12.58.32.mp4Second review by @dmkt9
✅ Fixed! Screen_Recording_20261009_131209_Expensify.Dev.mp4
✅ Fixed! Screen.Recording.2026-10-09.at.13.00.27.mov
✅ Fixed! Screen.Recording.2026-10-09.at.13.16.00.mov
✅ Fixed! Screen_Recording_20261009_131209_Expensify.Dev.mp4
✅ Fixed! Screen_Recording_20261009_140222_Expensify.Dev.mp4
✅ Fixed! Screen_Recording_20261009_142047_Expensify.Dev.mp4
✅ Fixed! Screen_Recording_20261009_152613_Expensify.Dev.mp4
✅ Fixed! Screen.Recording.2026-10-09.at.13.39.01.mov
✅ Fixed! Screen.Recording.2026-10-09.at.13.43.17.movLatest MelvinBot review Fixed Show History pulling the reader back to the bottom, restored the New marker for tall unread final messages, and preserved footer positioning for linked final messages, with regression tests for these cases. All local fixes, including late-layout linked-message positioning and the report-switch/reconnect edge cases, are now pushed through d25f5b6.
Follow ups
|
|
@dmkt9 @roryabraham all previous issues have been addressed! Unless there are any new issues, this PR should be ready to go. @roryabraham could you please trigger an ad-hoc build on this? |
Fixed ✅
Detailsbug1.mp4
Detailshttps://github.com/user-attachments/assets/0c461e30-fcc8-450a-847b-29a837e2115d
Details**Steps to reproduce:**
bug4.mp4
Detailsbug1.mp4
Detailsbug2.mp4
Detailsbug3.mp4
Detailsbug4.mp4
Detailsbug71.mp4
Detailsbug8.mp4Remaining
|
|
@dmkt9 I cannot reproduce most of the reported issues. I've tested and recorded screen recordings for all of these issues on both a physical Samsung Galaxy S10e and Android Pixel 9 emulator running Android 17. Could you please try to reproduce these on a physical device or a different emulator? I suspect the issues are related to your specific emulator setup or your build is invalid. @dmkt9 please let me know exactly which device/model and Android version your testing on. Also this seems like it's all on an emulator, right?
❓ I still cannot reproduce this issue. I have pretty much the same list content as in your screen recording, last message is a string, before that a PDF and some more messages. Screen recordingsPixel 9 emulator android.webmSamsung Galaxy S10e Screen_Recording_20261019_152346_New.Expensify.Dev.copy.mp4
✅ Fixed! Not reproducible anymore on emulator nor physical Android device. Screen recordingsPixel 9 emulator longComment.webmSamsung Galaxy S10e Screen_Recording_20261019_165518_Expensify.Dev.mp4
❓ Also not reproducible. Some of the issues related to showing the linked message in the viewport all seem related, but they should have all been fixed. Screen recordingsPixel 9 emulator linking.webmSamsung Galaxy S10e Screen_Recording_20261019_174958_Expensify.Dev.mp4
@dmkt9 Do you mean, you could not reach this next issue, so this is an old recording? Or what issue could you not test?
❓ Not reproducible. Screen recordingsPixel 9 emulator navigateBack.webmSamsung Galaxy S10e Screen_Recording_20261019_183852_Expensify.Dev.mp4
✅ As mentioned above, all issues related to the initial scroll position should be fixed. Let me know if it is still reproducible on another emulator and physical Android device. Screen recordingsPixel 9 emulator initialExpense.webmSamsung Galaxy S10e Screen_Recording_20261019_183340_Expensify.Dev.mp4
❓ Not reproducible. In your screen recordings it looked like this might be related to the initial rendering of a chat, where the linked item might have jumped around, because newer messages were loading at that time, but i couldn't reproduce this either. I attached a screen recording for both warm and cold report open. Screen recordingsPixel 9 emulator Warm report open scrollRight.webmCold report open (after clearing Onyx) scrollFresh.webmSamsung Galaxy S10e Screen_Recording_20261019_185531_Expensify.Dev.mp4
✅ These should be fixed! Screen recordingsWeb Screen.Recording.2026-10-19.at.17.59.37.mov |
| const isInLandscapeMode = useIsInLandscapeMode(); | ||
|
|
||
| // Android keeps the input focused when the keyboard is dismissed with the back button/gesture. | ||
| useBlurOnKeyboardHide(textInputRef); |
There was a problem hiding this comment.
Could you explain why we need this change? If the composer doesn't lose focus, the keyboard will appear when we close the modal and return to the chat
Fixed ✅
Detailsbug1.mp4
Detailshttps://github.com/user-attachments/assets/0c461e30-fcc8-450a-847b-29a837e2115d
Details**Steps to reproduce:**
bug4.mp4
Detailsbug1.mp4
Detailsbug2.mp4
Detailsbug3.mp4
Detailsbug4.mp4
Detailsbug71.mp4
Detailsbug8.mp4The keyboard appears briefly: Detailsbug5.mp4The keyboard doesn't appear when editing a long comment: Detailsbug6.mp4On Android, the highlighted item is out of the viewport if there is a long comment above it: Detailsbug7.mp4Unable to test due to a new bug: The app automatically navigates back to the Home screen when "Reply in thread". Detailsbug2.mp4Android: The highlighted item scrolls down and then back up: bug1.mp4Android: Scrolling slightly right after the loading state disappears causes the list to jump: bug3.mp4Web: The "Submit" badge needs to be clicked multiple times (sometimes 3–4 times) to scroll to the highlighted item. Sometimes, the top of the highlighted item or the bottom of the last item in the list is obscured. bug4.mp4Remaining
|
@chrispader Thanks! All previously reported bugs have been resolved when testing on a physical device. |
The report-actions list currently uses inversion, so rendering order and scroll coordinates differ from chronological message order. This PR moves the main chat list to LegendList and updates pagination, unread tracking, message editing, recycling, and initial loading behavior to work with chronological data.
@roryabraham @dmkt9
@Expensify/design Please review the initial chat-loading skeleton shown below.
Explanation of Change
This is layer 4 of the seven-part LegendList and chat pagination stack. It migrates
ReportActionsListto chronological LegendList data through a direct{LegendList}import from@legendapp/list/react-native, updates the shared action-list ref contract, preserves cached pages during hydration, and includes the Android anchor patch. It also covers pagination thresholds, targeted scrolling, unread tracking, live-tail updates, badge targets, message editing, and recycled rows. Other FlashList consumers remain unchanged, and layer 5 adds the separate loading indicators and pagination-scroll behavior.Initial loading now uses one shared, bottom-padded skeleton cover across the Inbox and Search expense-report entry paths. The cover remains visible while the initial request is pending and until the current LegendList viewport reports that it loaded. LegendList also keeps newly assigned rows hidden until native measurement finishes, which prevents estimated off-screen rows such as the chat start message from briefly overlapping visible content. A terminal initial-load failure can clear the cover instead of leaving it stuck.
The review follow-up restores
onStartReachedso short chats can load older pages before the viewport becomes scrollable, names the comment-size recycling thresholds, and keeps newest-message edits in large money reports on the list's indexed bottom-scroll path. The Reassure scenario now has a LegendList-specific name because the LegendList Jest mock eagerly renders its supplied data while the previous FlashList harness used virtualization; this establishes a renderer-specific baseline instead of comparing incompatible test harnesses. Layer 1 is now merged, and this branch includes its final edge-callback latch changes plus the latestmain.Message-edit scrolling from #100883 and the shared loading-skeleton cover from #100884 are now on
main. This branch keeps the LegendList-specific integration, while the shared changes from those PRs have dropped out of this diff.Review order: 1. Foundation → 2. Message edit scrolling → 3. Loading skeleton consistency → 4. Report actions → 5. Chat pagination loading → 6. Remaining FlashList consumers → 7. Remaining FlatList consumers and restrictions. Review and merge the layers in this order.
Fixed Issues
$ #98994
$ #99202
This PR is part of the migration split from #98270. No approved proposal is linked.
Tests
Offline tests
QA Steps
Same as Tests and Offline tests.
PR Author Checklist
I reviewed every item for this PR. A checked conditional item means I verified it or determined that it does not apply. I attached the available native evidence below; Android screen recordings are still TBD, and web recordings are deferred. The checked recordings item records that review and the available evidence, not that every platform has a video.
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
iOS: Native
Cold chat open and initial loading
FlashList is on the left and LegendList is on the right. FlashList leaves the report body blank during loading; LegendList shows the report-action skeleton before messages appear.
LegendListLoadingSkeletonVsFlashList.mp4
Cold expense-chat open
FlashList is on the left and LegendList is on the right. Both progress from the loading skeleton to an expense preview and its surrounding report actions.
ExpenseChatCold.mp4
Warm expense-chat open
FlashList is on the left and LegendList is on the right. Both open the cached expense preview and report actions from the inbox.
ExpenseChatWarm.mp4
Warm chat open
Side-by-side opens of the same cached chat, ending on the same report-action history. The implementations are not labeled in this recording.
LegendListWarmReportOpen.mp4
Android: Native
Cold chat open and initial loading
FlashList is on the left and LegendList is on the right. FlashList leaves the report body blank during loading; LegendList shows the report-action skeleton before messages appear.
ColdOpenLoading.mp4
Cold expense-chat open
FlashList is on the left and LegendList is on the right. Both progress from the loading skeleton to an expense preview and its surrounding report actions.
Android screen recording TBD.
Warm expense-chat open
FlashList is on the left and LegendList is on the right. Both open the cached expense preview and report actions from the inbox.
Android screen recording TBD.
Warm chat open
Side-by-side opens of the same cached chat, ending on the same report-action history. The implementations are not labeled in this recording.
Android screen recording TBD.
Web recordings are deferred.