fix: Allow multiple loads in useLoadReportActions hook
#76104
Conversation
chrispader
force-pushed
the
@chrispader/allow-multiple-loads-in-useLoadReportActions-hook
branch
from
42e8ae6 to
752818e
Compare
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.
|
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
@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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/hooks/useLoadReportActions.ts
Outdated
| ], | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Logic Error
There's a logic error in this useEffect. When !isTransactionThreadReport is true (line 191), you're checking if both isNewestReportActionLoaded AND isNewestTransactionThreadReportActionLoaded are loaded before setting isLoadingNewerChats.current = false (lines 194-197).
However, if it's NOT a transaction thread report, you shouldn't need to check the transaction thread's newest action at all. The code on lines 192-197 appears unreachable/incorrect because:
- Line 192 sets
isLoadingNewerChats.current = falseunconditionally - Lines 193-197 declare a variable and then check both reports, which contradicts the
!isTransactionThreadReportcondition
This should probably be:
if (!isTransactionThreadReport) {
const isNewestReportActionLoaded = isReportActionLoaded(currentReportNewest?.reportActionID);
if (isNewestReportActionLoaded) {
isLoadingNewerChats.current = false;
}
return;
}
const isNewestReportActionLoaded = isReportActionLoaded(currentReportNewest?.reportActionID);
const isNewestTransactionThreadReportActionLoaded = isReportActionLoaded(transactionThreadNewest?.reportActionID);
if (isNewestReportActionLoaded && isNewestTransactionThreadReportActionLoaded) {
isLoadingNewerChats.current = false;
}
src/hooks/useLoadReportActions.ts
Outdated
|
|
||
| return reportActions.some((action) => action.reportActionID === actionID); | ||
| }, | ||
| [reportActions], |
There was a problem hiding this comment.
Missing useEffect Import
The diff shows that useEffect is imported at line 2, but this comment is about line 92 in the new code. Looking at the logic of the new isReportActionLoaded callback:
This callback iterates through ALL reportActions using .some() to check if a specific action exists. For large reports with thousands of actions, this could be expensive and is called multiple times in the useEffect hooks.
Performance Suggestion: Consider memoizing a Set of reportActionIDs to make lookups O(1) instead of O(n):
const reportActionIDsSet = useMemo(() => {
return new Set(reportActions.map((action) => action.reportActionID));
}, [reportActions]);
const isReportActionLoaded = useCallback(
(actionID: string | undefined) => {
if (!actionID) {
return true;
}
return reportActionIDsSet.has(actionID);
},
[reportActionIDsSet],
);There was a problem hiding this comment.
Good point. Thanks for the catch!
src/hooks/useLoadReportActions.ts
Outdated
| ); | ||
|
|
||
| useEffect(() => { | ||
| if (!isLoadingOlderChats.current) { |
There was a problem hiding this comment.
Potential Race Condition
The new implementation uses useEffect to reset the isLoadingOlderChats.current flag when new actions are loaded. However, there's a potential race condition:
- User triggers
loadOlderChats()which setsisLoadingOlderChats.current = trueand callsgetOlderActions() - The API call is in-flight
- User triggers
loadOlderChats()again before the first request completes - The check at line 102 (
isLoadingOlderChats.current) blocks the second call - First request completes, useEffect at line 124 runs and sets
isLoadingOlderChats.current = false - But what if the actions from the first request arrive in Onyx BEFORE the useEffect runs?
The timing between when Onyx updates reportActions and when the useEffect detects the change could cause issues. Consider adding error handling or using a more robust loading state management pattern (e.g., tracking request IDs or using a proper state machine).
|
In order to prevent duplicate concurrent With that in mind, i changed the implementation to prevent further API calls until the report action is loaded and found in Onyx, but this raises some problems:
Therefore, i'd like to know if we can make an exception for the rule above for this purpose and we just go with the previous promise-based implementation. We are not awaiting any data from a previous action or API request, we just want to check whether it has finished. (Success or failure don't make a difference here) |
@ishpaul777 i'm not able to reproduce this one. Maybe because i just merged main as well? Tried both with existing report actions and pages in cache, and after clearing cache. Screen.Recording.2025-12-05.at.12.52.36.movScreen.Recording.2025-12-05.at.12.51.37.mov |
@ishpaul777 also not able to reproduce 🥴 Screen.Recording.2025-12-05.at.12.54.58.mov |
Screen.Recording.2025-12-06.at.12.20.02.AM.mov
Screen.Recording.2025-12-06.at.12.18.57.AM.movBoth issue still reprodicble consistently 😕 |
Okay @ishpaul777 so i'm able to reproduce this now, but this is also present on main atm. Could you please confirm? 👀
Still not able to reproduce this one unfortunately. I tried on both Chrome and Arc (both Chromium based), tried logging in and out a couple of times, clearing (Onyx) cache, both from within DevTools and the Troubleshoot menu. Also, i think you were either on a slow network connection or enabled Network throttling, so i also tried with 3G network throttling. Do you have any further repro steps for me? I'm suspecting this is to some illegal/stale state in your Expensify instance. Could you try logging out completely maybe? Screen.Recording.2025-12-09.at.11.22.55.mov |
|
Hey @ishpaul777 were you able to check this out? #76104 (comment) |
ahh i confirm its reproducible on staging and prod also Screen.Recording.2025-12-15.at.8.22.33.PM.mov
@chrispader would you like to try testing with my test account, i am able to reproduce this very reliably, please check below recording. on first opening chat it doesn't scroll below 150th message(i just this is also reproducible on staging also,) later when reopening chat it scroll directly to last message (not reproducbile on staging) Screen.Recording.2025-12-15.at.8.32.03.PM.mov |
Yes, that would be awesome. thanks! |
|
Hey @chrispader were you able to test it? |
|
Yes, so i was talking with @ishpaul777 and these is the issue in more details according to him:
After testing on my and @ishpaul777's account i was able to reproduce parts of the issue:
I was able to reproduce this, but i can also repro on main. This is something that should be fixed by #51366, because there we change the initial rendering of the
and
I am still not able to reproduce this one. Here's a screen recording of the whole reproduction steps flow after clearing the cache. Screen.Recording.2025-12-18.at.10.21.57.movI can even see that an issue currently on main was fixed by this PR. Previously, we would only allow one Screen.Recording.2025-12-18.at.10.50.23.movSo if the jumps to the end of the chat are not reproducible, i think we should be good with merging this. |
|
finishing this up today! |
ishpaul777
left a comment
ishpaul777
left a comment
There was a problem hiding this comment.
So if the jumps to the end of the chat are not reproducible, i think we should be good with merging this.
this was still reproducible for me on chrome everytime chat is open first time after sign in but it could be me only thing 🤷
rest LGTM! Thanks!
|
Hey @chrispader I am facing the same issue that Ishpaul is. Maybe we can screenshare and get this sorted? Screen.Recording.2025-12-29.at.10.24.14.mov |
|
@rlinoz @ishpaul777 going to look into this again. That's a nasty one 😅 |
|
@chrispader let me know if I can help debug this 😄 |
|
Hey @chrispader any updates on this? |
@rlinoz sorry for the delay, i will try to debug this again today and hopefully find a fix over the weekend, otherwise ask you to debug it for me too 🙌🏼 |
|
@rlinoz @ishpaul777 i'm looking into this again now. Sorry for the delay |
|
Okay, so it seems both @rlinoz and @ishpaul777 you're able to reproduce the issue where no messages after the 150th message are loaded when scrolling down, right? The issue where the chat jumps to the end of the chat on scroll seems to be resolved right? |
|
@rlinoz @ishpaul777 i've retested on Chrome, but i'm still not able to reproduce 🥴 I've cleared cache and then went to the chat and clicked on a link to somewhere in the middle of the chat. I first scrolled up, causing two triggers of Screen.Recording.2026-01-12.at.13.02.41.movSometimes in between the content stalled, but that's mainly due to the network request taking some time and me continuing to scroll. (we should probably implement some sort of loading spinner to show while the request is loading, i've proposed that before in Slack, maybe i can work on this in a follow-up) |
|
@rlinoz I have just tried to repro in Firefox as well and encountered the same issue that we've fixed before, where scrolling down stops at some point due to the content being cut off. We have fixed that before in this commit: import type ChatContentScrollViewPlatformStyles from './types';
const chatContentScrollViewPlatformStyles: ChatContentScrollViewPlatformStyles = {
- overflow: 'hidden',
+ overflowY: 'clip',
};
export default chatContentScrollViewPlatformStyles;I'all add this change to this PR as well, @rlinoz could you re-test or do you have time to screenshare later today, so we can figure this out? |
|
Fixed for me! 🎉 Can you double check @ishpaul777, please? |
|
Acknowledging i saw this, will retest ASAP! |
|
Reviewing now! |
|
Apologies i shifted focus on other issues. it seems like now i cant even scroll after the 51st message demo.movi reverted this overflow back to 'hidden' and its fixed but still has this #76104 (comment) issue this is also repro on Safari browser, but in this case it tries scrolls up while user is scrolling down and then auto scroll to last Screen.Recording.2026-01-16.at.5.16.05.AM.mov |
3 participants
@rlinoz @ishpaul777
Explanation of Change
This PR changes the behavior of
useLoadReportActionsso thatGetOlderActionsandGetNewerActionscan be triggered multiple times. Previously, this could only be triggered once, i'm not sure if this was by design or a bug, but for comment linking this definitely has to be possible.Fixed Issues
$ #35011
PROPOSAL:
Tests
Before these tests, you might want to log in freshly or clear the
reportActions_andreportActionsPages_Onyx stores for a given report:Load older chats
Load older/newer chats for linked messages
/r/:reportID/:reportActionID)Offline tests
None needed.
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as in Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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: mWeb Safari
MacOS: Chrome / Safari