fix: Infinite loading when delete message in reply#77845
fix: Infinite loading when delete message in reply#77845truph01 wants to merge 9 commits intoExpensify:mainfrom
Conversation
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
trjExpensify
and removed request for
a team
Code Review SummaryI've reviewed PR #77845 and have the following observations: ✅ Positive Aspects
🤔 Logic ConcernThe code appears to have a redundant conditional check: if ((\!isFocused && \!isReportDetailOpenInRHP) || isInNarrowPaneModal) {
if (isInNarrowPaneModal) { // ← This condition is already true from outer if
Navigation.goBack(backTo as Route);
return;
}
return;
}Issue: When Potential improvement: Consider simplifying to: if (isInNarrowPaneModal) {
Navigation.goBack(backTo as Route);
return;
}
if (\!isFocused && \!isReportDetailOpenInRHP) {
return;
}This separates the two distinct early-return conditions and makes the logic clearer.
|
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Nobody likes an infinite skelly!
P.S Can you try to make your PR titles more reflective of what code changes the PR is making / fill out the explanation of changes section? Thanks!
|
@trjExpensify I just filled out the Explanation of Changes section. |
|
Excellent! Can you please make a concert effort to do that going forward? Thanks! |
Sure, I noted. Thanks for your reminder. |
src/pages/home/ReportScreen.tsx
Outdated
| if ((!isFocused && !isReportDetailOpenInRHP) || isInNarrowPaneModal) { | ||
| if (isInNarrowPaneModal) { | ||
| Navigation.goBack(backTo as Route); | ||
| return; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The simplified logic from the bot review makes sense.
| if ((!isFocused && !isReportDetailOpenInRHP) || isInNarrowPaneModal) { | |
| if (isInNarrowPaneModal) { | |
| Navigation.goBack(backTo as Route); | |
| return; | |
| } | |
| return; | |
| } | |
| if (isInNarrowPaneModal) { | |
| Navigation.goBack(backTo as Route); | |
| return; | |
| } | |
| if (!isFocused && !isReportDetailOpenInRHP) { | |
| return; | |
| } |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppCleanShot.2025-12-18.at.11.22.19.mp4Android: mWeb ChromeCleanShot.2025-12-18.at.11.23.30.mp4iOS: HybridAppCleanShot.2025-12-18.at.11.20.46.mp4iOS: mWeb SafariCleanShot.2025-12-18.at.11.18.42.mp4MacOS: Chrome / SafariCleanShot.2025-12-18.at.10.58.07.mp4 |
@suneox Ah, your original message in the issue confused me a bit, and I misunderstood the nature of the bug. Thanks for the video—that makes the issue much clearer. I'm working on a fix now. |
|
@suneox I just applied your suggestion #77845 (comment) Please help review again. Thanks! |
|
@garrettmknight I’d like to confirm whether we should extend the scope of this issue to also prevent infinite loading when navigating back to the inbox (instead of only dismissing the right panel as part of the expected behavior), or if this should be handled as a separate issue. CleanShot.2025-12-30.at.00.21.00.mp4 |
|
I feel it should be handled here as it sounds like same issue to me |
The behavior is similar to the issue we’re fixing here (the infinite loading), but based on a deeper RCA, they are actually different problems. From my side, I think we should create separate issue for it. @suneox What do you think about my thought? |
|
@truph01 I agree that it could be a separate issue from an RCA perspective, but I’m aligned with @MonilBhavsar that we should handle it in this issue since it’s the same context and user-facing problem (infinite loading). so I think we can extend the scope here, not only dismissing the RHP but also addressing the infinite loading when navigating back to the Inbox. @truph01 could you help troubleshoot the additional root cause for this? Thanks! |
|
I have a solution ready, but I still need to test to ensure there are no regressions. |
|
Regarding the additional bug we're addressing in #77845 (comment), here are the general steps to reproduce it:
Current behavior: In step 7, the report still shows an infinite loading indicator in the header, like this: This happens because the backend continues to return data for this report, even though both the parent and the thread messages have been deleted. My thoughts: What do you think, @suneox? |
@truph01 Which expected backend behavior defines that it “should not return data for this report anymore”? In the case where the thread is dismissed from the RHP, I think we should handle it the same way on the report page. After all messages are deleted, the app should navigate back to the parent report, and the deleted thread should no longer be shown. Could you please take another look for solution for this one? CleanShot.2026-01-17.at.18.37.49.mp4 |
|
@MonilBhavsar Regarding the issue where users try to access a deleted thread: I think this should be handled on the backend. After deleting a message, the BE returns CleanShot.2026-01-17.at.18.22.49-converted.mp4Keep using 
cc: @trjExpensify |
|
Is there a product question here for me, @truph01? |
|
Looking into this |
I don’t have any product-related questions at the moment.
I don’t have a specific reference stating this is the expected backend behavior. This was just my assumption. @MonilBhavsar I’m still awaiting your response regarding this comment and comment. |
4 participants
Explanation of Change
In the report screen, we have logic to automatically navigate the user to another report when the current one is removed from Onyx storage. This can happen due to actions like leaving a room, being removed from a room, or deleting a thread parent message (as in the case of this bug).
However, when the report screen is opened in the RHP (via the search page), this auto-navigation logic is currently skipped.
This PR adds support for that case. When the report screen is opened in the RHP, it now navigates the user back to the previous screen if the current report is removed. This prevents users from getting stuck on a removed or inaccessible report.
Fixed Issues
$ #75904
PROPOSAL: #75904 (comment)
Tests
Offline tests
QA Steps
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
Screen.Recording.2025-12-17.at.10.38.17.mov
Android: mWeb Chrome
Screen.Recording.2025-12-17.at.10.40.42.mov
iOS: Native
Screen.Recording.2025-12-17.at.10.36.11.mov
iOS: mWeb Safari
Screen.Recording.2025-12-17.at.10.39.58.mov
MacOS: Chrome / Safari
Screen.Recording.2025-12-17.at.09.58.43.mov