-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[CP Staging] Fix blinking old type of report #62562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CP Staging] Fix blinking old type of report #62562
Conversation
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🧪🧪 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
loadNewerChats
fromMoneyRequestReportActionsList
fires and makesreportMetadata?.isLoadingNewerReportActions
true - because of that
InitialLoadingSkeleton
flashes 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we check for transactions?.length === 0
? It's possible to have an empty report
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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. |
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 Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.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