Fix: Infinite Loading When Member Is Removed from Workspace Chat#74045
Fix: Infinite Loading When Member Is Removed from Workspace Chat#74045nabi-ebrahimi wants to merge 11 commits intoExpensify:mainfrom
Conversation
|
@allroundexperts 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] |
| }, [reportOnyx?.reportID, route.params.backTo, reportIDFromRoute]); | ||
|
|
||
| useFocusEffect(redirectBackOnDeletedReport); | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
The useCallback dependency array includes the entire reportOnyx?.reportID object property, but you're only accessing it in conditional checks. Consider specifying the individual property more explicitly if reportOnyx is an object that changes frequently.
Suggested fix: Since reportOnyx?.reportID is already a primitive value (string), this is acceptable. However, ensure that reportIDFromRoute and route.params.backTo are also primitives and not extracted from complex objects that could cause unnecessary re-renders.
|
LGTM 👍 Thank you for your hard work! |
|
@allroundexperts, friendly bump on review. thanks. |
|
@nabi-ebrahimi Tests are failing. |
…backbutton-pressed
|
@allroundexperts Thanks for checking. Although those failures weren’t related to my changes, I’ve merged main to rerun the tests. If they fail again, I’ll investigate further. |
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.
|
|
@allroundexperts, tests are all passed, please take another look. thanks |
|
Hi @nabi-ebrahimi! Your PR still does not seem to resolve the issue. Screen.Recording.2025-12-01.at.3.04.17.AM.mov |
|
@allroundexperts, I will check it asap and update here. thanks |
…backbutton-pressed
|
@allroundexperts, could you please re-test it? I’ve tested it again on my side, and it’s working. Screen.Recording.1404-09-10.at.8.52.45.AM.mp4Could you please share any specific steps in your test that caused this issue to appear? Thank you. |
|
@allroundexperts, the failing test appears to be a flaky one and isn’t related to my changes. It should be resolved after re-running the tests. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Change makes sense from a product/permissions perspective.
|
@allroundexperts, can you please check this again. thanks. |
|
Hi @nabi-ebrahimi! This still does not work. Do the following to reproduce:
You'll observe that user B is redirected to a forever loading screen. Screen.Recording.2025-12-08.at.2.12.13.AM.mov |
|
@allroundexperts Thanks for the review. The issue reported in the OP only occurs when a user is redirected back after being removed from the chat. The issue you're referring to is related, but it wasn't part of the original scope and doesn’t require a redirect to reproduce — the user simply needs to be a workspace admin. Would you prefer that I address this in the current PR, or should I open a separate PR since it wasn’t originally included or handled? |
|
I think it is related and should be handled as part of this PR. |
|
For me, browser back button keeps the current page (expense report) with just scrolling up. Screen.Recording.2026-01-08.at.1.48.03.PM.mov |
|
https://dev.new.expensify.com:8082/r/7280722323369493/?backTo=%2Fr%2F6061172335000489 Screen.Recording.2026-01-08.at.1.53.47.PM.mov |
This comment was marked as off-topic.
This comment was marked as off-topic.
@nabi-ebrahimi you can git bisect yourself. App/src/pages/home/ReportScreen.tsx Line 757 in a0bb986 This line was added in #28702 to fix #28087 And later changed a bit (wrapped with isNavigationReady) in #63032 to fix #62568. App/src/pages/home/ReportScreen.tsx Lines 756 to 758 in a0bb986 |
|
When tap on browser back button, Screen.Recording.2026-01-08.at.2.25.17.PM.movIn this video: |
|
We already have logic to redirect to Concierge chat when report doesn't exist: App/src/pages/home/ReportScreen.tsx Lines 762 to 764 in a0bb986 But failed because this condition is not met: App/src/pages/home/ReportScreen.tsx Lines 708 to 715 in a0bb986 Because prevReport is undefined.
This works perfectly when member is on workspace chat (not expense report) at the moment member is removed by admin. |
|
@nabi-ebrahimi based on above analysis, can you please find solution to fix the real root cause? |
|
@situchan, thanks, i will check and update today. |
|
@situchan, thanks for the review, and check. I have investigated it further. And i believe this block is useles now and should be removed. App/src/pages/home/ReportScreen.tsx Lines 749 to 755 in d335917 This block originally added in this pr to fix this issue. But this logic handles that now. and that block never runs after App/src/libs/actions/Report.ts Lines 3684 to 3700 in d335917 Which is called in both Since then we had two changes to that code block this pr to fix this issue, and this PR to fix this issue I have tested my solution with that code block removed. all the issues are solved. 1. Leaving threads lands you in concierge chat issue leave-the-thread.mov2. Chat - LHN disappears after leaving a thread issue Screen.Recording.1404-10-22.at.2.20.25.PM.mov3. Chat - After deleting an expense, it navigates to different chat instead of the main chat issue Screen.Recording.1404-10-22.at.2.26.37.PM.mov |
|
ok, if removing the changes doesn't reintroduce linked issue which that PR fixed, we can go with that. |
@situchan thanks for the review, i have tested, it won't reintroduce those issues. |
…backbutton-pressed
|
@situchan, kindly bump, thanks |
|
@situchan, quick bump. |
|
I am still doing regression test since we're touching the core component. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3181d366f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (reportOnyx?.reportID) { | ||
| return; | ||
| } | ||
|
|
||
| if (reportIdRef.current.routeReportId === reportIDFromRoute) { | ||
| Navigation.goBack(route.params.backTo ?? undefined); |
There was a problem hiding this comment.
Add fallback when deleted report has no back stack
This redirect relies solely on Navigation.goBack, which is a no-op when there is no history (Navigation.goBack returns early if canGoBack() is false). If a user opens a child report via deep link/push (so there is no back stack or backTo param) and then gets removed from the room, the report will be deleted but this handler won’t navigate anywhere, leaving the user stuck on a deleted report/infinite loader. Previously the flow navigated to the parent report when parentReportID existed, which worked without history; consider restoring that or providing an explicit fallback (e.g., LHP) when goBack can’t pop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Can you please test the flow bot concerns?
There was a problem hiding this comment.
@situchan, the bot is right. i am trying to fix that. thanks.
|
And please pull main, fix conflict |
|
@nabi-ebrahimi any update? |
This seems like a fragile area of the codebase with a history of regressions so I want to respect that and tread lightly. If the proposed fix in 80306 is going to address the root cause more cleanly then I would rather wait for that instead of trying to force a less-holistic approach here. (Also just to add, if we don't end up merging a PR here, we can still compensate for the time invested on both of your parts. So let's just make sure we're promoting the most optimal solution regardless of who will implement.) |
Explanation of Change
Fixed Issues
$ #64627
PROPOSAL: #64627 (comment)
Tests
Offline tests
Same as Tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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))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.1404-04-19.at.3.03.46.PM.mov
Android: mWeb Chrome
Screen.Recording.1404-04-19.at.3.24.18.PM.mov
iOS: Native
Screen.Recording.1404-04-19.at.3.19.47.PM.mov
iOS: mWeb Safari
Screen.Recording.1404-04-19.at.3.25.27.PM.mov
MacOS: Chrome / Safari
Screen.Recording.1404-04-19.at.2.57.57.PM.mov
MacOS: Desktop
Not applicable.