[$2000] BUG: unread indicator count is not reset after logout reported by @parasharrajat

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Open the app on web.
2. Login with any account that has some unread messages. (to get unread messages send some messages from a different account to the account being logged in).
3. Now wait a couple seconds.
4. Observe there are unread message count on the Tab.
5. Do not go to the chat that has unread messages.
6. Now Sign out.
7. Wait a couple seconds.
8. Unread count is shown back after log out.

Optional:

9. If count is not shown by step 8, login back with the same account without refreshing the page.
10. Wait for the count to show up.
11. Log out now.
12. Wait a couple seconds, observe the Unread count on the tab bar.

Expected Result:

No unread count and indicator is shown after logout

Actual Result:

Unread count and indicator are shown after logout on login page.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Desktop App

Version Number: 1.2.12-2 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: https://user-images.githubusercontent.com/43996225/194615034-efb34a05-7dfd-4644-b813-d49dbc39827d.mp4

https://user-images.githubusercontent.com/43996225/194615559-d16582d9-ae1e-42e0-bd79-1e13f9e25b55.mp4

Expensify/Expensify Issue URL: Issue reported by: @parasharrajat Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1665066610938259

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @tjferriss (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[tjferriss]

I'm going to pass this over to engineering as I think it's a pass. However, I am on the fence as to whether this issue really hits the value part of the review:

Value — in other words, why is this particular issue important? Are there reasonable workarounds? Is the value of resolution greater than the resources required to reach it?

This seems like a polish item and if it requires more than a super small amount of work then I'd suggest we pass for now.

[melvin-bot[bot]]

Triggered auto assignment to @justicea83 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Justicea83]

This can be external, we set the unreadCount on the Web App here.

[melvin-bot[bot]]

Triggered auto assignment to @CortneyOfstad (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

[melvin-bot[bot]]

Triggered auto assignment to @marcaaron (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tienifr]

Proposal

Problem

Now, we're using listenForReportChanges function to listen the changing of the reports and then using function throttledUpdatePageTitleAndUnreadCount to update the pageTitle and unreadCount. In function listenForReportChanges we check if (!report || !report.reportID) we do nothing. That why when we logout we don't reset the reports.

Solution

we'll create new function resetReportAfterLogout to reset the reports and use UnreadIndicatorUpdater.resetReportAfterLogout() in componentDidMount of SignInPage

In https://github.com/Expensify/App/blob/main/src/libs/UnreadIndicatorUpdater/index.js

+function resetReportAfterLogout(){
+    Object.keys(reports).forEach(k => delete reports[k])
+}

export default {
    listenForReportChanges,
    stopListeningForReportChanges,
    throttledUpdatePageTitleAndUnreadCount,
+    resetReportAfterLogout
};

In https://github.com/Expensify/App/blob/main/src/pages/signin/SignInPage.js#L48

-        setTimeout(() => updateUnread(0), 0);
+        setTimeout(() => {
+            UnreadIndicatorUpdater.resetReportAfterLogout()
+            updateUnread(0)
+        }, 0);

https://user-images.githubusercontent.com/113963320/195080834-62a726f6-4c1d-44b1-98f1-ada7a3a06e5d.mp4

[marcaaron]

@Justicea83 why did we unassign @CortneyOfstad? @sobitneupane why did we add the Help Wanted label?

Did I miss a process change? Seems like we need to wait for the Upwork job before the Help Wanted label gets added (which should happen automatically when the Exported label is added).

[CortneyOfstad]

Sounds good — just about to head out for the day, but will tackle this ASAP in the morning to get the Upwork job created 👍

[Justicea83]

@Justicea83 why did we unassign @CortneyOfstad? @sobitneupane why did we add the Help Wanted label?

Did I miss a process change? Seems like we need to wait for the Upwork job before the Help Wanted label gets added (which should happen automatically when the Exported label is added).

That must be a Github bug, I remember only unassigning myself 😁

[CortneyOfstad]

Posted job here — https://www.upwork.com/ab/applicants/1580165477000413184/job-details

[sobitneupane]

@marcaaron

Did I miss a process change?

Here is the slack discussion regarding the process update.

[fedirjh]

Proposal

Problem

the UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount() is being invoked on onStateChange on the NavigationContainer . the throttledUpdatePageTitleAndUnreadCount execute the updateUnread function after 100 ms . so when user logs out this happen:

1. State Updated -> call to UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount()
2. SignInPage is mounted and updateUnread is executed immediately and update the title
3. UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount() , after 100 ms , execute updateUnread and update the title again

Solutions

Solution 1

Delay the updateUnread on the SignInPage for 100 ms

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/pages/signin/SignInPage.js#L48

-          setTimeout(() => updateUnread(0), 0);
+          setTimeout(() => updateUnread(0), 1);

Solution 2

Call UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount() only when user is authenticated

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/Navigation/NavigationRoot.js#L35 https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/Navigation/NavigationRoot.js#L49 https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/Navigation/NavigationRoot.js#L62

edited

+               if (this.props.authenticated) {
                   UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount();
+               }
-                onStateChange={this.parseAndLogRoute}
+               onStateChange={this.parseAndLogRoute.bind(this)}

Solution 3

Remove Reports when stopListeningForReportChanges is executed

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L7 https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L42-L48

-          const reports = {};
+         let reports = {};
...

function stopListeningForReportChanges() {
    if (!connectionID) {
        return;
    }
+       reports = {}

    Onyx.disconnect(connectionID);
}

[melvin-bot[bot]]

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

[sobitneupane]

All solutions proposed by @fedirjh in his proposal solves this issue. I prefer the third solution because it also solves below mentioned issue.

While testing above proposals I found that after signing out from one account and signing in to another account, the unread indicator count from previously signed in account is displayed. It is displayed for few seconds if the unread indicator count for newly signed in account is non-zero and if the unread indicator count for newly signed in account is zero, it is displayed until the page is reload.

cc: @marcaaron

[tienifr]

@sobitneupane I think u missed some things here, I retested my proposal and It works correctly. Maybe u haven't add the import UnreadIndicatorUpdater from '../../UnreadIndicatorUpdater' in https://github.com/Expensify/App/blob/main/src/pages/signin/SignInPage.js. If there is, please help recheck. My evidence:

https://user-images.githubusercontent.com/113963320/195655273-3b74ea03-d000-46da-b00f-1c96da6efa0e.mp4

https://user-images.githubusercontent.com/113963320/195655306-8d90b795-53f9-4c8d-a90e-a0dab8f75a32.mp4

By the way, the third solution of @fedirjh seems to be flicked.

https://user-images.githubusercontent.com/113963320/195658147-e888b4f2-c718-4783-8382-a41093491fb2.mp4

[marcaaron]

Sorry I don't really like any of the solutions proposed by @fedirjh

Solution 1 - setTimeout is hacky in this case as it's not clear what we are waiting for Solution 2 - It feels like the wrong place to apply this logic. Also we are passing a prop to a method that can access these props already because it's a class method. Solution 3 - It's not the job of that listener to remove the reports. This should happen when Onyx.clear() is called.

@sobitneupane I think we can wait for a higher quality solution. The one's I've seen are not really acceptable. Though the second solution related to authentication feels like the closest to what we might want - I think we'd want a different implementation.

[fedirjh]

@marcaaron thanks for the feedback , i have updated both solution 1 and 2

for solution 1 : when SignInPage is mounted it execute updateUnread(0) immediately , but it should be deferred until UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); is . setting timeout to 1ms will resolve this issue

setTimeout(() => updateUnread(0), 1);

for solution 3 : the reports const is used on the subscription callback , the job of stopListeningForReportChanges is to remove the subscription callback , so the reports should be reset when we remove the subscription callback , if it's not reseted , the next login will use the old reports value (even when user is changed) . check demo

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L39-L42

Screencast from 10-13-2022 08:25:35 PM.webm

[marcaaron]

for solution 1 : when SignInPage is mounted it execute updateUnread(0) immediately , but it should be deferred until UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); is . setting timeout to 1ms will resolve this issue

We're still using a setTimeout so it's a no from me.

for solution 3 : the reports const is used on the subscription callback , the job of stopListeningForReportChanges is to remove the subscription callback , so the reports should be rest when we remove the subscription callback , if it's not reseted , the next login will use the old reports value . check demo

I'm not seeing how this is a different solution from the one before. As mentioned already, the reports value should be reset when Onyx.clear() is called.

[fedirjh]

@marcaaron we are talking about reports in the memory

const reports = {};

not that one from indexedDB

We're still using a setTimeout so it's a no from me.

the actual code uses setTimeout , i just suggested a change of the delay

[marcaaron]

the actual code uses setTimeout , i just suggested a change of the delay

Apologies, in that case, I would entertain a proposal that removes the setTimeout(), but not one that modifies it without explaining why it's there to begin with.

we are talking about reports in the memory

I understood this. Thanks.

[tienifr]

@marcaaron @sobitneupane What do you think about my proposal here https://github.com/Expensify/App/issues/11671#issuecomment-1274549195

[sobitneupane]

What do you think about my proposal here #11671 (comment)

@tienifr Thank you for the proposal. But we are looking for proposals that removes the use of setTimeout(). And we believe there must be a way to tackle this issue without the use of setTimeout()

[fedirjh]

While testing above proposals I found that after signing out from one account and signing in to another account, the unread indicator count from previously signed in account is displayed.

@sobitneupane this issue exists in staging & production , it displays the sum of unread reports from previous and active account , you can reproduce it by using two accounts whit unread count :

1. Open the app on web.
2. Login with any account that has some unread messages.
3. Observe there are unread message count on the Tab.
4. Do not go to the chat that has unread messages.
5. Now Sign out.
6. Login with another account that has some unread messages.
7. Now you see that the count is the sum of unique unread reports of previous and active account

this is due the reports persists in memory , new logged in user's reports are being merged with previous logged in account as @marcaaron mentioned :

the reports value should be reset when Onyx.clear() is called

here is solution : https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L7

-        const reports = {};
+        let reports = {}
+        function clearReports(){
+            reports = {}
+        }
...
export default {
    listenForReportChanges,
    stopListeningForReportChanges,
    throttledUpdatePageTitleAndUnreadCount,
+   clearReports
};

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/actions/SignInRedirect.js#L36-L38

+   import UnreadIndicatorUpdater from "../UnreadIndicatorUpdater";
...

+   UnreadIndicatorUpdater.clearReports();
    Onyx.clear()
        .then(() => {

[marcaaron]

@fedirjh In the correct design the reports should be "cleared" already - there should be no reason to manually clear them before calling Onyx.clear()

[marcaaron]

@tienifr your solution has the same problem as @fedirjh - my expectation is that if we clear data with Onyx that any subscribers that set local values should have those values cleared automatically.

[fedirjh]

my expectation is that if we clear data with Onyx that any subscribers that set local values should have those values cleared automatically.

@marcaaron thanks for clarification , the actual reports const is out of scope for the subscriber callback , then i would suggest those changes https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L14 https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L25-L37

-   const reports = {};
...
-   const throttledUpdatePageTitleAndUnreadCount = _.throttle(() => {
+   const throttledUpdatePageTitleAndUnreadCount = _.throttle((reports) => {
...
function listenForReportChanges() {
    connectionID = Onyx.connect({
        key: ONYXKEYS.COLLECTION.REPORT,
+        waitForCollectionCallback:true, 
-        callback: (report) => {
+        callback: (reports) => {
-            if (!report || !report.reportID) {
+            if (!reports) {
                return;
            }

-            reports[report.reportID] = report;
-            throttledUpdatePageTitleAndUnreadCount();
+            throttledUpdatePageTitleAndUnreadCount(_.filter(reports, null));
        },
    });
}
...

function stopListeningForReportChanges() {
    if (!connectionID) {
        return;
    }

-    Onyx.disconnect(connectionID);
+    // When Onyx.clear() i called , there is a last time change to report where 
+    // all reports are cleared (nulled) we need to wait for this change to happen , 
+    // then we disconnect , if Onyx.disconnect is called immediately , then it will be executed 
+    // after Onyx.clear()  without waiting for that last change 
+    setTimeout(() => Onyx.disconnect(connectionID), 0)
}

then no need to call UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); in NavigationRoot.js https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/Navigation/NavigationRoot.js#L49 we just remove it

-   UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount();

For SignInPage we can remove the setTimeout

https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/pages/signin/SignInPage.js#L48

componentDidMount() {
        // Always reset the unread counter to zero on this page
        // NOTE: We need to wait for the next tick to ensure that the unread indicator is updated
-        setTimeout(() => updateUnread(0), 0);
    }

[sobitneupane]

@fedirjh Why do we need to force update unread in signinpage? https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/pages/signin/SignInPage.js#L48

I believe it should be updated when onyx is cleared. We might have stopped listening for changes in report before Onyx is cleared.

[sobitneupane]

@fedirjh I agree with your reason below

this is due the reports persists in memory , new logged in user's reports are being merged with previous logged in account

Cause might be:

• reports is stored as global variable.
• When onyx is cleared and start listening for change in report (listenForReportChanges()), !report is true in callback function and the callback function returns. We are not clearing reports stored in global variable in any place. https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L28-L36

[fedirjh]

@sobitneupane , @marcaaron When Onyx.clear() i called , there is a last time change to report where all reports are cleared (nulled) we need to wait for this change to happen , then we disconnect , if Onyx.disconnectis called immediately , it will be executed after Onyx.clear() without waiting for that last change , in this case setTimeout will ensure enqueuing :

Onyx.js#L908

notifySubscribersOnNextTick(key, resetValue);

[fedirjh]

@sobitneupane i have updated my proposal , you can test it https://github.com/Expensify/App/issues/11671#issuecomment-1279341434

[marcaaron]

I'm going OOO for a week so we will need to find a new CME for this one. @sobitneupane please ping in Slack if you can.

[sobitneupane]

@fedirjh Suggestion: It is better to add updated proposal in new comment rather than editing previous comment.

Rather than setTimeout, why not wait for onyx to be cleared before calling stopListeningForReportChanges().

We might have stopped listening for changes in report before Onyx is cleared.

Can you please explain the line below?

the actual reports const is out of scope for the subscriber callback

Also, next point to consider is performance. We need to make our code better in performance? Your suggested changes does not look good performance wise? Because in each update You are suggesting to pass all the available reports to throttledUpdatePageTitleAndUnreadCount which is not the case now(only updated report are passed to throttledUpdatePageTitleAndUnreadCount). It might hinder performance when users have lot of reports.

[tienifr]

@sobitneupane what do you think about my updated proposal below: In https://github.com/Expensify/App/blob/363d98fd3a3567c102c5359ba5257c67f95c60d0/src/libs/UnreadIndicatorUpdater/index.js#L42 I will reset reports and then call function throttledUpdatePageTitleAndUnreadCount to update unread indicator

-    const reports = {};

function stopListeningForReportChanges() {
    if (!connectionID) {
        return;
    }
+    reports={}
+    throttledUpdatePageTitleAndUnreadCount();
    Onyx.disconnect(connectionID);
}

And then in https://github.com/Expensify/App/blob/363d98fd3a3567c102c5359ba5257c67f95c60d0/src/pages/signin/SignInPage.js#L45 I can remove the logic using setTimeout

-    componentDidMount() {
-        // Always reset the unread counter to zero on this page
-        // NOTE: We need to wait for the next tick to ensure that the unread indicator is updated
-        setTimeout(() => updateUnread(0), 0);
-    }

[fedirjh]

the actual reports const is out of scope for the subscriber callback

@sobitneupane i meant that reports variable isn't declared inside the subscriber , so it persists in-memory

Why do we need to force update unread in signinpage ?

when Onyx is cleared , the subscriber callback doesn't update reports because it become null , reports[report.reportID] = report; here we add report to reports variable , but when report become null (cleared) we can't remove it from reports as we don't have any reference to it , if Onyx.clear return the id instead of null then we can update the above code to remove report from reports , I can suggest a solution for this but it's not a good practice for performance , I think it's just better to force update count to zero updateUnread(0)

here when report become null nothing happen : https://github.com/Expensify/App/blob/299f07bfa29c0dd81a6126691aae29e161097baa/src/libs/UnreadIndicatorUpdater/index.js#L29-L31

Proposal

• We have to declare const reports = {}; inside the subscriber to ensure that data is cleared when subscriber is removed
• When we stop stopListeningForReportChanges we just rest total count to zero

1. Changes in UnreadIndicatorUpdater/index.js

-   const reports = {};
...
-   const throttledUpdatePageTitleAndUnreadCount = _.throttle(() => {
+   const throttledUpdatePageTitleAndUnreadCount = _.throttle((reports) => {
...
function listenForReportChanges() {
+   const reports = {};
    connectionID = Onyx.connect({
        key: ONYXKEYS.COLLECTION.REPORT,
        callback: (report) => {
           if (!report || !report.reportID) {
                return;
            }

            reports[report.reportID] = report;
            throttledUpdatePageTitleAndUnreadCount();
+           throttledUpdatePageTitleAndUnreadCount(reports);
        },
    });
}
...

function stopListeningForReportChanges() {
    if (!connectionID) {
        return;
    }
+   updateUnread(0);
    Onyx.disconnect(connectionID);
}

2. Changes in SignInRedirect.js

To Avoid the use of setTimeout we should ensure that onyx.clear is executed then we execute stopListeningForReportChanges which will disconnect and reset counter to zero

+        import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater';

    Onyx.clear()
        .then(() => {
            ...
-        });
+        }).finally(() => UnreadIndicatorUpdater.stopListeningForReportChanges())

3. Changes in Session/index.js

we have to remove `UnreadIndicatorUpdater.stopListeningForReportChanges()` from `cleanupSession`
function cleanupSession() {
    // We got signed out in this tab or another so clean up any subscriptions and timers
    NetworkConnection.stopListeningForReconnect();
-   UnreadIndicatorUpdater.stopListeningForReportChanges();
    PushNotification.deregister();
    PushNotification.clearNotifications();
    Pusher.disconnect();
    Timers.clearAll();
    Welcome.resetReadyCheck();
}

4. Changes in NavigationRoot.js

there is no need for this line

-      UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount();

5. Changes in SignInPage.js

there is no need anymore to force reset on this page , because we reset on

class SignInPage extends Component {
-    componentDidMount() {
-        // Always reset the unread counter to zero on this page
-        // NOTE: We need to wait for the next tick to ensure that the unread indicator is updated
-        setTimeout(() => updateUnread(0), 0);
-    }

[sobitneupane]

@tjferriss As suggested earlier, we want listenForReportChanges to handle updating of the read/unread count. Since report is being changed, throttledUpdatePageTitleAndUnreadCount(); should be called through listenForReportChanges.

[sobitneupane]

@fedirjh Your proposal steps 2-5 look convincing.

function stopListeningForReportChanges() {
   if (!connectionID) {
       return;
   }
+   updateUnread(0);
   Onyx.disconnect(connectionID);
}

But in step 1, It is better to find a way to update unread updateUnread through listenForReportChanges > throttledUpdatePageTitleAndUnreadCount > updateUnread if possible.

[fedirjh]

@sobitneupane here is an updated Proposal

Proposal

1. Changes in UnreadIndicatorUpdater/index.js

we can get the key already , we can remove the report from reports when it's cleared

...
const throttledUpdatePageTitleAndUnreadCount = _.throttle(() => {
-   const totalCount = _.filter(reports, ReportUtils.isUnread).length;
+   const totalCount = _.keys(reports).length;
    updateUnread(totalCount);
}, 100, {leading: false});

function listenForReportChanges() {
    connectionID = Onyx.connect({
        key: ONYXKEYS.COLLECTION.REPORT,
-        callback: (report) => {
+        callback: (report,key) => {
-           if (!report || !report.reportID) {
+           if (!key) {
                return;
            }
+           if (!report) {
+               delete reports[key]
+            } else {
+                // push only unread reports
+                if (ReportUtils.isUnread(report))
+                    reports[key] = report;
+                else
+                   delete reports[key]
+            }

-           reports[report.reportID] = report;
            throttledUpdatePageTitleAndUnreadCount();
        },
    });
}

2. Changes in SignInRedirect.js

To Avoid the use of setTimeout we should ensure that onyx.clear is executed then we execute stopListeningForReportChanges which will disconnect and reset counter to zero

+        import UnreadIndicatorUpdater from '../UnreadIndicatorUpdater';

    Onyx.clear()
        .then(() => {
            ...
-        });
+        }).finally(() => UnreadIndicatorUpdater.stopListeningForReportChanges())

3. Changes in Session/index.js

we have to remove `UnreadIndicatorUpdater.stopListeningForReportChanges()` from `cleanupSession`
function cleanupSession() {
    // We got signed out in this tab or another so clean up any subscriptions and timers
    NetworkConnection.stopListeningForReconnect();
-   UnreadIndicatorUpdater.stopListeningForReportChanges();
    PushNotification.deregister();
    PushNotification.clearNotifications();
    Pusher.disconnect();
    Timers.clearAll();
    Welcome.resetReadyCheck();
}

4. Changes in NavigationRoot.js

there is no need for this line

-      UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount();

5. Changes in SignInPage.js

there is no need anymore to force reset on this page , because we reset on

class SignInPage extends Component {
-    componentDidMount() {
-        // Always reset the unread counter to zero on this page
-        // NOTE: We need to wait for the next tick to ensure that the unread indicator is updated
-        setTimeout(() => updateUnread(0), 0);
-    }

[sobitneupane]

@fedirjh Your latest proposal looks promising. Have you tested it on other different scenarios where unread indicator is updated?

Can you also clarify why UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); was needed before and how your proposed solution removes it's need?

@CortneyOfstad As Marc is OoO for a week (https://github.com/Expensify/App/issues/11671#issuecomment-1280072219), we need new CME here.

[fedirjh]

Can you also clarify why UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); was needed before and how your proposed solution removes it's need?

@sobitneupane this change was made by @marcaaron https://github.com/Expensify/App/pull/10739/commits/211bd86236e5ccea86102cb25a8e452e86f5186a , i think the correct design is that title indicator is updated on report change only from the subscriber callback , not on state change , this will cause performance issue as it will fire on every state change (including route change)

[CortneyOfstad]

Since it has been over a week, doubled the price to $500. Also applying new CME now 👍

[CortneyOfstad]

Hmm, GH is being screwy with the labels. Trying that again!

[melvin-bot[bot]]

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Current assignee @sobitneupane is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[tsa321]

Proposal

The problem is throttledUpdatePageTitleAndUnreadCount callback function is being called after user sign out and reports variable is persist in memory. As for reports variable that persist in memory after user sign out, this variable must be reset with reports = {} because it will cause wrong total unread count if user login with different account. So adding reports = {} and setting it as let variable.

.../UnreadIndicatorUpdater/index.js
...
- const reports = {};
+ let reports = {};
....
function stopListeningForReportChanges() {
    if (!connectionID) {
        return;
    }

      Onyx.disconnect(connectionID);
+   reports = {}; 
}
....

Actually setting reports = {} is enough to solve this issue. This is similar to @tienifr https://github.com/Expensify/App/issues/11671#issuecomment-1280411982. But I don't know why he use additional throttledUpdatePageTitleAndUnreadCount(); Also we can remove setTimeout(() => updateUnread(0), 0); in SignIn page

[sobitneupane]

Proposal from @fedirjh looks good to me. I am not sure why UnreadIndicatorUpdater.throttledUpdatePageTitleAndUnreadCount(); was added in NavigationRoot.js. It was added in this commit by @marcaaron. @fedirjh is proposing to remove it too. Changes test well.

cc: @puneetlath

[tsa321]

@sobitneupane any advise for my proposal? Is it bad? since the changes are minimal. .

[sobitneupane]

@sobitneupane any advise for my proposal? Is it bad? since the changes are minimal. .

@tsa321 Your proposal is similar to 3rd solution in this proposal. You can find advise for your proposal in https://github.com/Expensify/App/issues/11671#issuecomment-1277944934 comment and comments following it.