fix - Dev : Report chat is not visible in LHN if we open concierge chat in another tab#74195
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
There was a problem hiding this comment.
This seems to make sense to me. We shouldn't lose/change your chat position in another tab if you simply open a new one and click into another chat. 👍
|
@FitseTLT I'm not sure if I am missing a step here, but I'm unable to reproduce the issue in staging. 73457-not-reproducible-002.mp4 |
Yes @rojiphil you are indeed missing steps. First you should test it on different tabs not windows and you should be on the expense report not policy chat just like the OP. I know the title mislead you I have updated it. Thx |
|
Ah.. Now I get it.. Thanks @FitseTLT for pointing it out. 73457-issue-001.mp4 |
|
Are you reproducing the bug now? I don't reproduce it after merging main 2025-11-11.19-58-39.mp4 |
@FitseTLT Sorry for the delay here. Can you please merge again? Will check with the latest. |
|
Done @rojiphil |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp74195-android-hybrid-001.mp4Android: mWeb Chrome74195-mweb-chrome-001.mp4iOS: HybridApp74195-ios-hybrid-001.mp4iOS: mWeb Safari74195-mweb-safari-001.mp4MacOS: Chrome / Safari74195-web-chrome-001.mp474195-web-chrome-002.mp4MacOS: Desktop74195-desktop-001.mp4 |
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @FitseTLT.
@arosiclair Changes LGTM and works well too.
Over to you. Thanks
| useEffect(() => { | ||
| currentReportIDValue?.updateCurrentReportID(currentReportID); | ||
|
|
||
| // eslint-disable-next-line react-compiler/react-compiler | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- we only want to run the effect on currentReportID change. | ||
| }, [currentReportID]); |
There was a problem hiding this comment.
Why move this code from the NavigationRoot to the AppNavigator? Does this fix not work without this change?
There was a problem hiding this comment.
Yes useNavigationState should be used under a NavigationContainer.
There was a problem hiding this comment.
Previously we were using the NavigationContainer's onStateChange. Can we just keep using that to update the current reportID? I think adding additional hooks here might trigger more full app re-renders and impact performance.
There was a problem hiding this comment.
What about the memoization of AuthScreens
wouldn't that immediately stop re-renders and performance degradation as it is the direct child of AppNavigator
the disadvantage of using the onStateChange callback is that it is too slow to be called an causes a flickering problem #56523
There was a problem hiding this comment.
Can you show a video of that flickering problem in this context?
There was a problem hiding this comment.
Here is the flickering.
2025-12-01.19-42-22.mp4
But when removing setTimeout delay here
App/src/libs/Navigation/NavigationRoot.tsx
Lines 252 to 255 in 1f7efee
the flickering becomes almost unrecognizable
2025-12-01.19-44-00.mp4
But working on the pr they claimed it was a little jumpy here and considered removing the delay as not sufficient I am afraid that we would cause a regression if we only remove the delay.
There was a problem hiding this comment.
Here is the flickering.
https://github.com/user-attachments/assets/0b7cf110-6160-42d1-8f78-2bc1ce2f8c88
I don't really see an issue 😕. Can you push changes to use onStateChange and @rojiphil can you retest for any flickering issues?
There was a problem hiding this comment.
Agree with you. Implemented. @rojiphil U can continue 👍
There was a problem hiding this comment.
LGTM from the flickering perspective.
74195-web-chrome-003.mp4
rojiphil
left a comment
There was a problem hiding this comment.
@arosiclair Changes LGTM.
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.2.79-0 🚀
|
|
🚀 Deployed to staging by https://github.com/arosiclair in version: 9.2.81-0 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.2.81-5 🚀
|
Explanation of Change
Fixed Issues
$ #73457
PROPOSAL: #73457 (comment)
Tests
Prerequisite: can only be tested in Web (MacOs/windows)
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
MacOS: Chrome / Safari
2025-11-04.16-34-02.mp4
MacOS: Desktop