Fix: Leave thread chat when has no comment #27849
Conversation
melvin-bot
bot
requested review from
fedirjh
and removed request for
a team
|
@dukenv0307 you have conflicts here |
|
@roryabraham @dukenv0307 I would like to point out that this solution will not fix the previous thread that you opened without commenting , it will just prevent the bug from occurring in the future. The user should re-open all these threads again to fix the unread count completely. However there is a challenge as these reports are not visible in the LHN, making it difficult for the user to locate them. |
|
@fedirjh Not sure if any actions should I take from my side based on your comment? Or should we wait for the internal team to confirm |
|
Hi! Can we fix conflicts on this and get it merged? At least for me, I think it's fine that it won't fix old threads, they won't be super active (and if people follow the other methods to get to them) it'll be either a non-issue or fixed eventually. |
|
Reviewing shortly. |
src/pages/home/ReportScreen.js
Outdated
| return; | ||
| } | ||
|
|
||
| if (ReportUtils.isThread(report) && report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { |
There was a problem hiding this comment.
Let's add a comment explaining this hook.
There was a problem hiding this comment.
should prefer early return here:
if (!ReportUtils.isThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
return;
}
Report.leaveRoom(report.reportID, false);
src/pages/home/ReportScreen.js
Outdated
| Report.leaveRoom(report.reportID, false); | ||
| } | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Why didn’t we pass all deps , basically the report ?
There was a problem hiding this comment.
I think we should trigger this useEffect once the report loses focus
There was a problem hiding this comment.
Agree we don't want to run this whenever any property on the report changes. However, I think we probably want to make sure that the correct report data is used and want to avoid suppressing the lint rule.
So maybe we can workaround the lint rule with something like this:
const isThread = ReportUtils.isThread(report);
const isNotificationPreferenceHidden = report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const reportID = report.reportID;
// When the report screen is unmounted or no longer in focus, and the user has no comments on that thread, call LeaveRoom
useEffect(() => {
if (isFocused || !isThread || !isNotificationPreferenceHidden) {
return;
}
Report.leaveRoom(reportID, false);
}, [isFocused, isThread, isNotificationPreferenceHidden, reportID]);| @@ -350,6 +351,18 @@ function ReportScreen({ | |||
| [report, isLoading, shouldHideReport, isDefaultReport, isOptimisticDelete, userLeavingStatus], | |||
| ); | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
Will this hook runs when component unmounts ?
src/libs/actions/Report.js
Outdated
| if (report.parentReportID) { | ||
| Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report.parentReportID), CONST.NAVIGATION.TYPE.FORCED_UP); | ||
| return; | ||
| if (shouldNavigate) { |
There was a problem hiding this comment.
Let's early return before dismissing the modal and keep the other part as it is .
if (!shouldNavigate) {
return
}
|
Excited to review this one as soon as it's ready |
src/pages/home/ReportScreen.js
Outdated
| return; | ||
| } | ||
|
|
||
| if (ReportUtils.isThread(report) && report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) { |
There was a problem hiding this comment.
should prefer early return here:
if (!ReportUtils.isThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
return;
}
Report.leaveRoom(report.reportID, false);
src/pages/home/ReportScreen.js
Outdated
| Report.leaveRoom(report.reportID, false); | ||
| } | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Agree we don't want to run this whenever any property on the report changes. However, I think we probably want to make sure that the correct report data is used and want to avoid suppressing the lint rule.
So maybe we can workaround the lint rule with something like this:
const isThread = ReportUtils.isThread(report);
const isNotificationPreferenceHidden = report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN;
const reportID = report.reportID;
// When the report screen is unmounted or no longer in focus, and the user has no comments on that thread, call LeaveRoom
useEffect(() => {
if (isFocused || !isThread || !isNotificationPreferenceHidden) {
return;
}
Report.leaveRoom(reportID, false);
}, [isFocused, isThread, isNotificationPreferenceHidden, reportID]);
|
@roryabraham Just updated the code. Please help review again. |
| if (!shouldNavigate) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Let's move this line above the modal dismissal .
Bug: User is navigated to concierge after leaving the thread
- Open report
- Navigate to thread
- Click back button
- User should go back to main report
- User is navigate to concierge
CleanShot.2023-09-26.at.16.07.37.mp4
There was a problem hiding this comment.
@fedirjh I see the problem, this PR changed the logic
so I don't think just moving that IF condition will work since when we call LeaveRoom, we set the optimistic data of that report statusNum to CLOSED
App/src/libs/actions/Report.js
Line 1868 in fff2dd4
In ReportScreen, we have a logic to navigate to Concierge once statusNum is CLOSED
App/src/pages/home/ReportScreen.js
Lines 312 to 321 in fff2dd4
Working on this to find a solution
There was a problem hiding this comment.
I think one simple solution for this bug is that we add an optimistic prop let's say shouldNavigate and use it to prevent redirection inside ReportScreen.
optimisticData: [
{
onyxMethod: Onyx.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
stateNum: CONST.REPORT.STATE_NUM.SUBMITTED,
statusNum: CONST.REPORT.STATUS.CLOSED,
shouldNavigate, // Add this prop
},
},
],
// Update the condition to
if (
// non-optimistic case
(!prevUserLeavingStatus && userLeavingStatus) ||
// optimistic case
(... && report.shouldNavigate)
) {
|
@dukenv0307 any update ? |
|
@fedirjh The bug
will be successfully resolved using your suggestion However, I just encountered a new bug.
Screen.Recording.2023-09-28.at.15.21.58.movThe reason is that isFocused now will be false, therefore LeaveRoom will be triggered. I need a little bit more time to find a better alternative solution. Will ping back if any update |
@dukenv0307 Why do we prefer leaving the room when the component loses focus, why we didn’t just leave the room when the component unmounts? That way LeaveRoom will be called once. useEffect(() => () => {
if (!isThread || !isNotificationPreferenceHidden) {
return;
}
Report.leaveRoom(reportID, false);
}, [isThread, isNotificationPreferenceHidden, reportID]); |
|
@fedirjh |
|
@roryabraham / @fedirjh can you take a look at the failed checks on this one? |
|
Looks like we have conflicts here, and the last failing check is just the reviewer checklist. Currently awaiting approval + checklist from @fedirjh |
|
@fedirjh @roryabraham After implementing the proposal, I found out that many regressions are likely to happen if the LeaveRoom solution is applied
I would suggest applying a new solution to update the logic here App/src/libs/UnreadIndicatorUpdater/index.js Lines 11 to 12 in 6c7ee53 to leave out all the report that notificationPreference is hidden. |
Those are very bad regressions, worse than having wrong indicator 😕 .
@dukenv0307 Do you suggest we exclude the hidden notificationPreference in the count, But we will still have data in Onyx for those reports. The solution looks better than having wrong count, but doesn’t seems ideal. Curious about your thoughts @roryabraham |
|
@roryabraham Friendly bump. Any thoughts on this #27849 (comment) |
|
My thoughts are that the solution we are working on here is still logically sound, but we need to add some additional logic to correctly handle edge cases. psuedocode like this: In plain English, if you navigate away from a thread that you have not commented on, and are not navigating to a child thread, then leave the thread and recursively do the same thing for all parent threads. thoughts? |
@roryabraham Actually, these are not really few edge cases; the report screen will lose focus in different ways, Opening any RHN modal including deeplink will make the report screen lose focus (and trigger the leave action). I am not sure if we can cover all of them with the suggested approach. |
|
@roryabraham @dukenv0307 We have this PR #30013 merged, It addresses the same issue we are fixing. What is our next plan of action for this PR? Should we proceed with closing it? |
|
Yes, I suppose we should close this now |
Details
New messages on the threads that I can't see on my LHN are not counting toward the unread counter on the app icon
Fixed Issues
$ #27512
PROPOSAL: #27512 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
web.7.mp4
Mobile Web - Chrome
chrome.3.mp4
Mobile Web - Safari
safari.1.mp4
Desktop
desktop.5.mp4
iOS
ios.3.mp4
Android
android.6.mp4