Fix scrolling to bottom list on MoneyRequestReportView by Kicu · Pull Request #59664 · Expensify/App

Kicu · 2025-04-04T14:05:51Z

Make scrolling to bottom and unread message marker work on MoneyRequestReportView.

Explanation of Change

for unread marker I extracted the common logic and reused it in MoneyRequestReport; since the list is not inverted there I needed to traverse the message in reverse order
scrolling is more complicated. In the inverse list we're always scrolling to offset 0 which works; however scrollToEnd will sometimes not work if its called before all the new items rendered, because the list will not know their size. Hence I had to use setTimeout
Extra note: the solution is tested, but my emulator + simulator were running quite slow so on the videos app looks much more slow than in reality

Fixed Issues

$ #58808
PROPOSAL:

Tests

go to Reports > Expense Reports > click on report
verify that chat scrolls to bottom to newest message when a user types a new message in
verify that unread message marker behaves the same as on chat reports (from Inbox)

Offline tests

QA Steps

go to Reports > Expense Reports > click on report
verify that chat scrolls to bottom to newest message when a user types a new message in
verify that unread message marker behaves the same as on chat reports (from Inbox)

PR Author Checklist

Screenshots/Videos

Android: Native

rec-andr.mp4

Android: mWeb Chrome

iOS: Native

rec-ios.mp4

iOS: mWeb Safari

MacOS: Chrome / Safari

rec-web.mp4

MacOS: Desktop

Kicu · 2025-04-04T14:06:24Z

src/pages/home/report/ReportActionsList.tsx

 import ListBoundaryLoader from './ListBoundaryLoader';
 import ReportActionsListItemRenderer from './ReportActionsListItemRenderer';

-type LoadNewerChats = DebouncedFunc<(params: {distanceFromStart: number}) => void>;


noticed this was unused

Kicu · 2025-04-08T13:21:22Z

src/pages/home/report/shouldDisplayNewMarkerOnReportAction.ts

+ * This function decides whether the given report action (message) should have the new marker indicator displayed
+ * It's used for the standard "chat" Report and for `MoneyRequestReport` actions lists.
+ */
+const shouldDisplayNewMarkerOnReportAction = ({


this function was extracted from ReportActionsList almost without change - I'm just passing down arguments so that it can be reused in 2 places.
No logic changes are intended for ReportActionsList

melvin-bot · 2025-04-08T13:39:58Z

@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]

Kicu · 2025-04-09T06:40:06Z

Cleaned and ready for review

shawnborton · 2025-04-09T06:40:55Z

Will run a test build 🚀

github-actions · 2025-04-09T06:41:26Z

🚧 @shawnborton has triggered a test app build. You can view the workflow run here.

shawnborton · 2025-04-09T08:54:17Z

Hmm this doesn't feel like a viable solution to me... the LHN shows a loading bar after I send a comment, and there is a delay in scrolling me down the page:

CleanShot.2025-04-09.at.10.53.24.mp4

cc @mountiny for thoughts, but this doesn't feel like smooth UX to me.

Kicu · 2025-04-09T09:05:46Z

The loading bar we can probably disable - if that is what you think makes more sense - this more of an oversight on my part.

The delay in scrolling to bottom however will be more problematic.
The main problem is that this is a dynamic list with items that have dynamic size and scrolling to bottom simply doesn't work out of the box and needs some hacks :|
I can definitely work more on making this feel smooth, but its not something that can be done super quick.

In "normal" chats we use the reverse list, and what to you is bottom is really the top of the list and so scrolling there works much better.

shawnborton · 2025-04-09T09:10:51Z

Cool, yeah I think in the very least removing the LHN loader would help it feel a little smoother

DylanDylann · 2025-04-09T11:09:16Z

New message indicator should only appear if the message come from another user. If the new action from the current user, we shouldn't display the new message indicator

Screen.Recording.2025-04-09.at.18.08.03.mov

Kicu · 2025-04-09T14:46:16Z

I will come back to this tomorrow to update and fix things

Kicu · 2025-04-10T14:25:12Z

@DylanDylann I manged to fix the unreadIndicator, I believe it now behaves same as on normal chats

rec-scrolling-web.mp4

However the request from Shawn to disable loading bar is not yet done, because this Loading bar is kinda global and unrelated to search, and I need more time ot make this work.
Will come back to this tomorrow.

Done in order to make the view feel more fluid and less jumpy.

Kicu · 2025-04-11T07:01:30Z

@DylanDylann ready for re-review.

@shawnborton at this point the Loading bar is disable on Report in Search/Reports context, however I did not modify any behaviour related to scrolling to bottom.

shawnborton · 2025-04-11T08:14:03Z

Will spin up a new testie for us 🚀

github-actions · 2025-04-11T08:14:42Z

🚧 @shawnborton has triggered a test app build. You can view the workflow run here.

Kicu · 2025-04-15T10:51:49Z

@DylanDylann confirming that I can repro this mweb error on android emulator: #59664 (comment)

Seems to be coming from here:

App/src/libs/Pusher/index.ts

Line 203 in dbd0421

throw new Error(`[Pusher] instance not found. Pusher.subscribe()

I also cannot reproduce it on main :/ not sure what happens here and why

DylanDylann · 2025-04-15T10:55:17Z

Opsss.... You mean it comes from our change.

Kicu · 2025-04-15T11:47:25Z

Ok @DylanDylann
I finally managed to reproduce this also on main, and spend some time digging around this.
Feels to me like it does not directly come from my change, but my code somehow surfaces an existing error. Perhaps we are using some kind of old way of using Browser notifications? I will try to create an issue for this later on github.

For now I think we don't have to block this PR.
Here is my repro:

rec-error.mp4

DylanDylann · 2025-04-15T17:07:19Z

@mountiny What do you think? Are we good to merge and fix above error later in another PR?

src/pages/home/report/shouldDisplayNewMarkerOnReportAction.ts

src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx

jnowakow · 2025-04-16T13:40:31Z

src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx

+    const reportActionSize = useRef(visibleReportActions.length);
+    const lastAction = visibleReportActions.at(-1);
+    const lastActionIndex = lastAction?.reportActionID;
+    const previousLastIndex = useRef(lastActionIndex);


There is usePrevious hook

thanks, I agree with you, but at this point I'd prefer to keep code the same both inMoneyRequestReportActionsList and standard ReportActionsList.

I will add this comment to another issue where I will be de-duplicating common code and fix it there.

mountiny

Small suggestion

src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx

mountiny

This is already large and complex change so I would avoid adding more complexity by the optional es lint fixes

melvin-bot · 2025-04-16T15:18:32Z

@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

mountiny · 2025-04-16T15:18:59Z

Noted above, due to the existing complexity of the pr we have skipped the optional eslint changes to the touched file

OSBotify · 2025-04-16T15:28:19Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2025-04-16T21:39:29Z

🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.29-0 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

kavimuru · 2025-04-17T15:33:16Z

Failing with original bug ID #58808 on web mweb and android
https://platform.applause.com/services/links/v1/external/786062acb0485551bc419cb3dadd68ffe1512b0b022a35b247aeecc941cb061b
https://platform.applause.com/services/links/v1/external/dfe98b5756ea8ded7da15f7373604d22413062c677f62cfc8335c077243f5c70
https://platform.applause.com/services/links/v1/external/4c15ff6aa8e0b3101369882195b774b156335e115fcc9d326f22a5dd8be817c8

mountiny · 2025-04-17T17:04:21Z

cc @Kicu

github-actions · 2025-04-18T20:31:24Z

🚀 Deployed to production by https://github.com/marcaaron in version: 9.1.29-10 🚀

platform	result
🖥 desktop 🖥	success ✅
🕸 web 🕸	success ✅
🤖 android 🤖	failure ❌
🍎 iOS 🍎	success ✅

Kicu · 2025-04-22T07:13:59Z

Looking

Kicu · 2025-04-23T13:05:27Z

sorry I in fact did not look - will do so tomorrow, since I got more work on my other PR than expected.

c3024 · 2025-08-13T06:26:55Z

src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx

+        // Scan through each visible report action until we find the appropriate action to show the unread marker
+        for (let index = startIndex; index >= endIndex; index--) {
+            const reportAction = visibleReportActions.at(index);
+            const nextAction = visibleReportActions.at(index - 1);


The logic in ReportActionsList loops up and here it is in reverse so we needed to check if index > 0 here. Fixed in #65918

Kicu commented Apr 4, 2025

View reviewed changes

Kicu force-pushed the kicu/58808-money-request-report-scroll branch from 2d77809 to 82fcf4c Compare April 8, 2025 11:13

Kicu commented Apr 8, 2025

View reviewed changes

Kicu marked this pull request as ready for review April 8, 2025 13:39

Kicu requested a review from a team as a code owner April 8, 2025 13:39

melvin-bot bot requested review from DylanDylann and removed request for a team April 8, 2025 13:39

Kicu changed the title ~~[WIP] Fix scrolling to bottom list on MoneyRequestReportView~~ Fix scrolling to bottom list on MoneyRequestReportView Apr 9, 2025

Kicu force-pushed the kicu/58808-money-request-report-scroll branch from 82fcf4c to 6335519 Compare April 9, 2025 06:38

This comment has been minimized.

Sign in to view

mountiny assigned Kicu Apr 9, 2025

Kicu added 2 commits April 10, 2025 12:36

Fix scrolling to bottom list on MoneyRequestReportView

319e605

Fix displaying unread marker correctly

d2dc97c

Kicu force-pushed the kicu/58808-money-request-report-scroll branch from 6335519 to d2dc97c Compare April 10, 2025 14:23

Kicu added 3 commits April 11, 2025 08:36

Disable LoadingBar on SearchMoneyRequestReportPage

6607831

Done in order to make the view feel more fluid and less jumpy.

Merge branch 'main' into kicu/58808-money-request-report-scroll

814f32c

Fix conflicts after merge

acf0d2f

melvin-bot bot requested a review from mountiny April 15, 2025 10:50

Merge branch 'main' into kicu/58808-money-request-report-scroll

998cbc1

mountiny requested changes Apr 16, 2025

View reviewed changes

jnowakow reviewed Apr 16, 2025

View reviewed changes

jnowakow approved these changes Apr 16, 2025

View reviewed changes

Improve code comments around MoneyRequestReportActionsList

89df2e9

Kicu mentioned this pull request Apr 16, 2025

[Due for payment 2025-05-28] [Better Expense Report View] [Followup] Deduplicate logic between ReportActionsList and MoneyRequestReportActionsList #58891

Closed

mountiny reviewed Apr 16, 2025

View reviewed changes

src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx Outdated Show resolved Hide resolved

Fix comment typo

088f516

mountiny approved these changes Apr 16, 2025

View reviewed changes

mountiny merged commit 512af49 into Expensify:main Apr 16, 2025
15 of 16 checks passed

melvin-bot bot added the Emergency label Apr 16, 2025

mountiny removed the Emergency label Apr 16, 2025

github-actions bot mentioned this pull request Apr 16, 2025

Deploy Checklist: New Expensify 2025-04-16 #60369

Closed

87 tasks

melvin-bot bot mentioned this pull request Apr 18, 2025

[Due for payment 2025-04-25] [Better Expense Report View] Reports - Chat does not scroll down after sending message #58808

Closed

8 tasks

c3024 reviewed Aug 13, 2025

View reviewed changes

blazejkustra mentioned this pull request Feb 20, 2026

[BATCH 6] Migrate scroll operations #83067

Open

Conversation

Kicu commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

Kicu Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Kicu Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

melvin-bot bot commented Apr 8, 2025

Uh oh!

Kicu commented Apr 9, 2025

Uh oh!

shawnborton commented Apr 9, 2025

Uh oh!

github-actions bot commented Apr 9, 2025

Uh oh!

This comment has been minimized.

shawnborton commented Apr 9, 2025

Uh oh!

Kicu commented Apr 9, 2025

Uh oh!

shawnborton commented Apr 9, 2025

Uh oh!

DylanDylann commented Apr 9, 2025

Uh oh!

Kicu commented Apr 9, 2025

Uh oh!

Kicu commented Apr 10, 2025

Uh oh!

Kicu commented Apr 11, 2025

Uh oh!

shawnborton commented Apr 11, 2025

Uh oh!

github-actions bot commented Apr 11, 2025

Uh oh!

Kicu commented Apr 15, 2025

Uh oh!

DylanDylann commented Apr 15, 2025

Uh oh!

Kicu commented Apr 15, 2025

Uh oh!

DylanDylann commented Apr 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnowakow Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

Kicu Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melvin-bot bot commented Apr 16, 2025

Uh oh!

mountiny commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OSBotify commented Apr 16, 2025

Uh oh!

github-actions bot commented Apr 16, 2025

Uh oh!

kavimuru commented Apr 17, 2025

Kicu commented Apr 4, 2025 •

edited

Loading

mountiny commented Apr 16, 2025 •

edited

Loading