-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix: Infinite Loading When Member Is Removed from Workspace Chat #74045
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
base: main
Are you sure you want to change the base?
Fix: Infinite Loading When Member Is Removed from Workspace Chat #74045
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ 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✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
|
@allroundexperts, what is the expected behavior for the last issue you discovered? Should the “Not Found” page be displayed when a user is removed from the workspace? Since the member isn’t performing any action at that moment, it doesn’t seem necessary to redirect them back. What do you think?
|
I think the behaviour should be the same as it is here. |
…backbutton-pressed
|
@allroundexperts, my changes work as expected and redirect the user back. However, the following line navigates the user back to the unavailable report again. This issue already existed, but it became visible after we fixed the issue in OR Post App/src/pages/home/ReportScreen.tsx Lines 738 to 741 in 1ffa9e4
I don’t have enough context on when this line is expected to run and trigger navigation to the previous report. Could you please provide more details on the intended behavior and how the code should be updated without breaking it? |
|
@allroundexperts, @JmillsExpensify could you please check this comment when you get a chance. thanks |
|
@allroundexperts, @JmillsExpensify kindly bump on this comment |
…backbutton-pressed
|
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 |


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.