[CP Staging] Fix blinking old type of report #62562
Conversation
|
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
mountiny
changed the title
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@marcaaron 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.
In general once we get rid of the table report view beta, I think we should take another look at this if the logic can be simplified. for now its very fragile and not easy to follow cc @Kicu
Thanks for a quick fix and seems to work well for me, video here: https://expensify.slack.com/archives/C01GTK53T8Q/p1747916319905749?thread_ts=1747859807.859029&cid=C01GTK53T8Q
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
@SzymczakJ @martasudol also tested so I will move this one ahead and ask QA for a retest |
…pe-of-report [CP Staging] Fix blinking old type of report (cherry picked from commit e48ca86) (cherry-picked to staging by mountiny)
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| // Prevent flash by ensuring transaction data is fully loaded before deciding which view to render | ||
| // We need to wait for both the selector to finish AND ensure we're not in a loading state where transactions could still populate | ||
| const isTransactionDataReady = transactions !== undefined; | ||
| const isStillLoadingData = !!isLoadingInitialReportActions || !!reportMetadata?.isLoadingOlderReportActions || !!reportMetadata?.isLoadingNewerReportActions; |
There was a problem hiding this comment.
Why do we check for reportMetadata?.isLoadingOlderReportActions and reportMetadata?.isLoadingNewerReportActions?
I've found one case(it was really hard to reproduce it 😄 ) when:
- We are already on empty report which is loaded
- for some reason
loadNewerChatsfromMoneyRequestReportActionsListfires and makesreportMetadata?.isLoadingNewerReportActionstrue - because of that
InitialLoadingSkeletonflashes for a moment
|
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 9.1.49-6 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.1.50-0 🚀
|
| // We need to wait for both the selector to finish AND ensure we're not in a loading state where transactions could still populate | ||
| const isTransactionDataReady = transactions !== undefined; | ||
| const isStillLoadingData = !!isLoadingInitialReportActions || !!reportMetadata?.isLoadingOlderReportActions || !!reportMetadata?.isLoadingNewerReportActions; | ||
| const shouldWaitForData = (!isTransactionDataReady || (isStillLoadingData && transactions?.length === 0)) && !isTransactionThreadView; |
There was a problem hiding this comment.
Why do we check for transactions?.length === 0? It's possible to have an empty report
There was a problem hiding this comment.
Initially, when transactions come from Onyx, they are an empty array - not undefined. So the check for transactions?.length === 0 helps us distinguish between "data is ready but empty" vs "data is still loading".
|
I think this PR causes infinite loading after creating a report while offline |
@dmkt9 could you help me reproduce it? Take a look on my recording 👇 Nagranie.z.ekranu.2025-05-27.o.14.33.15.mov |
|
@martasudol Sure. Below is the video I reproduced. The error seems to originate here, with App/src/components/MoneyRequestReportView/MoneyRequestReportView.tsx Lines 140 to 141 in 33f1586 msedge_327LHLY1cl.mp4 |
|
@dmkt9 thanks! I'm on it! |
|
@dmkt9 I've added 
|
|
@martasudol it seems like I also forgot to add |
|
@SzymczakJ no problem, just let me know what should change in app behavior so I could test if it works well 🤞 Also @SzymczakJ maybe you're able to help me understand what's the expected behavior here? :D |
|
I think our expected behaviour for offline mode is to not show any loading indicator, since nothing is being loaded. So in our case it would be showing just "This report has no expenses" empty state. cc @shawnborton for confirmation 🙏 |
|
You mean ? If so, unfortunately it doesn't change anything in the flow described by @dmkt9 👇 Nagranie.z.ekranu.2025-05-28.o.10.28.09.mov |
|
Hmm but in this case, do we actually know for sure that the report has no expenses? If we aren't sure if the report has no expenses or not, we should use a more generic skeleton loader - otherwise that empty state might be very misleading. |
We don't. IMO in this case we should show skeleton loader as we do for other reports (static one; without animation). What do you think @shawnborton? |
|
Yup, that makes sense to me. |
|
Cool. PR created. Waiting for pipes to make it ready for review. @SzymczakJ looks like your missing flag doesn't break anything :D |
There is one case in which we know that the report has no expenses: when we create and empty report in offline mode. Screen.Recording.2025-05-28.at.12.21.49.movIf we block it by big skeleton then the user won't have an option to create any expenses offline in this report. Isn't that a problem? |
@martasudol @SzymczakJ @shawnborton I think so too. I see that there are quite a few flows running in offline mode. And the issue has been reported here #62677 |
|
Do you think it should be handled in #62677? |
I agree we wouldn't want to block it in that case. The whole point of the "Report is empty" empty state isn't to be any kind of loading state, it's truly only to be shown when the report is empty. So if you create a report while offline and you haven't added any expenses to it, we should definitely show it. |
|
Okay, I think this discussion got a little messy and we should just define the UI states for each edge case:
Currently we are just showing the generic loading page, which is blocking users from making expenses in offline mode.
But I suggest we get rid of that infinite loading state, while we are offline
WDYT @shawnborton |
cc @Expensify/design @trjExpensify for a gut check too. |
|
Agree on those.
Additionally though, did you create the report and add these two expenses all while offline in this case? 
|
|
See that this merged, but agree with the comments above |
I created the report and added these two expenses online and then switched to offline mode before opening Report. @martasudol do you mind, if we take over the #62677 and finish that empty state in there? We will probably have extra bandwidth today, so we will be looking for something to work on 😄 |
|
No problem, @SzymczakJ - go ahead! I also think it's a good idea, since you've been working on this codebase and have more context. If I can help with anything, just let me know! |
Cool, all good then. 👍 |
Explanation of Change
This PR fixes an empty state or expenses preview shown briefly when opening transaction thread.
Fixed Issues
$ #62514
Tests
Offline tests
n/a
QA Steps
Same as 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))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
Nagranie.z.ekranu.2025-05-22.o.12.59.38.mov
MacOS: Chrome / Safari
Nagranie.z.ekranu.2025-05-22.o.12.56.23.mov
MacOS: Desktop