Skip to content
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

[Bug] [$2000] The browser tab shows (1) New Expensify even though there are no unread messages in the LHN #8005

Closed
mvtglobally opened this issue Mar 4, 2022 · 98 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2 Planning Changes still in the thought process

Comments

@mvtglobally
Copy link

mvtglobally commented Mar 4, 2022

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 app in Web
  2. Check browser tab

Expected Result:

When user has no unread messages it should not display any counter.

Actual Result:

The browser tab shows (1) New Expensify even though there are no unread messages in the LHN

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.1.41-0
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: Any additional supporting documentation

Screen Shot 2022-03-04 at 12 06 18 AM

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

View all open jobs on GitHub

@MelvinBot
Copy link

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

@NikkiWines
Copy link
Contributor

Made a snippet to show the keyvaluepairs containing an unreadActionCount greater than 0

let connection = indexedDB.open('OnyxDB', 2);
let unreadReportKVPs;

connection.onsuccess = (e) => {
    const database = e.target.result;
    const transaction = database.transaction(['keyvaluepairs']);
    const objectStore = transaction.objectStore('keyvaluepairs');
    const keyvaluepairsPromise = objectStore.getAll();

    keyvaluepairsPromise.onsuccess = (e) => {
        let keyvaluepairs = e.target.result;
        unreadReportKVPs = keyvaluepairs.filter((object) => {
            if (typeof(object) === 'object') {
                return object.unreadActionCount > 0;
            }
            return false;
        });
        console.log(unreadReportKVPs);
    };
};

@NikkiWines
Copy link
Contributor

I think this can probably be an external issue? I'm not personally experiencing this so it's a bit difficult to debug.

@NikkiWines NikkiWines added Improvement Item broken or needs improvement. External Added to denote the issue can be worked on by a contributor labels Mar 4, 2022
@MelvinBot
Copy link

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

@NikkiWines NikkiWines removed their assignment Mar 4, 2022
@adelekennedy
Copy link

Internal
External

@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 5, 2022
@MelvinBot
Copy link

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

@adelekennedy
Copy link

@Santhosh-Sellavel @Beamanator it looks like the slack convo is still evolving but I wanted to get the job posted this week

@K4tsuki

This comment was marked as outdated.

@Santhosh-Sellavel
Copy link
Collaborator

The problem is there is (are) report(s) that has unread message but the report is not displayed in LHN.
LHN doesn't display all reports retrieved from Get chatList, it filters out some reports.

@K4tsuki Can you answer the following questions.

  1. What reports are filtered out?
  2. Why its get filtered?

@K4tsuki

This comment was marked as outdated.

@Santhosh-Sellavel
Copy link
Collaborator

The problem is likely from -1 inside unreadActionCounts. In my local reports data there is a report with notification prefecerence set to daily, it is a policyRoom.

@K4tsuki Can you clearly make a new proposal, with the changes you are proposing.

@K4tsuki

This comment was marked as outdated.

@Santhosh-Sellavel
Copy link
Collaborator

@Beamanator Room feature comes out of beta, addressing the issue would be unnecessary. What do you say?

@Beamanator
Copy link
Contributor

@Santhosh-Sellavel to be clear, are you thinking this issue will be resolved once the room features are out of beta? If yes can you explain why?

Also @K4tsuki thanks so much for your ideas so far, are you able to reproduce the issue regularly? If so, I assume you've tested your proposal with the reproduction steps and you're not seeing the issue anymore with your proposed changes?

@K4tsuki

This comment was marked as outdated.

@Santhosh-Sellavel
Copy link
Collaborator

@Santhosh-Sellavel to be clear, are you thinking this issue will be resolved once the room features are out of beta? If yes can you explain why?
Yes, I should have explained why, sorry I missed out earlier.

The issue occurs due to messages received in default or user-created rooms. Look at the attachments below with/without beta for the room feature. Notification count in the header doesn’t make sense only when the user does not have the beta. So I believe when the room features are out of beta, this issue becomes invalid.

If room beta is available

Screenshot 2022-03-16 at 7 42 01 PM

if room beta is not available

Screenshot 2022-03-16 at 7 32 50 PM

cc: @Beamanator

@iwiznia
Copy link
Contributor

iwiznia commented Jul 27, 2022

I know this is on hold, but the bug is VERY annoying

@adelekennedy
Copy link

still holding

@Beamanator
Copy link
Contributor

Agreed it's annoying, luckily the Unread Indicator Improvements doc is now out! (thanks to Marc)

@melvin-bot melvin-bot bot added the Overdue label Aug 29, 2022
@adelekennedy
Copy link

still on hold (despite how annoying it is 😂 )

@marcaaron
Copy link
Contributor

Ok so back with an update here and I think this one can come off HOLD.

We have changed how this feature works and squashed most of the bugs related to Unread Indicators - but this one is a bit elusive as it seems related to be a browser bug. It's not reproducible on Safari AFAICT - yet it is very consistently reproducible on Chrome. All you have to do is...

  1. Have an unread chat
  2. Navigate to the report (see the count clear from the title)
  3. Navigate back to the LHN (see the count return)

What's weird is that if you log out the "count" when we programmatically set it then shows 0, but the title still reverts back to (1).

Somewhat recent SO post about this here: https://stackoverflow.com/questions/72982365/setting-document-title-doesnt-change-the-tabs-text-after-pressing-back-in-the

So it seems related to how Chrome is handling the document.title. There is a workaround mentioned in the stack overflow which is basically to do this:

    document.title = '';
    document.title = hasUnread ? `(${totalCount}) ${CONFIG.SITE_TITLE}` : CONFIG.SITE_TITLE;

I tried it and it worked (kind of) the count will clear but navigating back will make the old count flash briefly. IMO this is better than telling someone that they have an unread message when they do not, but not totally ideal.

Haven't found much info on this bug so I suspect it is relatively new, but maybe it will just go away with time (or if we complain to the Chome team).

@marcaaron
Copy link
Contributor

I'm going to open a PR with the change since I think it is super annoying to see the incorrect count and would rather have a brief incorrect flash than totally incorrect.

@marcaaron marcaaron changed the title [HOLD] [$2000] The browser tab shows (1) New Expensify even though there are no unread messages in the LHN [$2000] The browser tab shows (1) New Expensify even though there are no unread messages in the LHN Sep 21, 2022
@JmillsExpensify JmillsExpensify changed the title [$2000] The browser tab shows (1) New Expensify even though there are no unread messages in the LHN [Bug] [$2000] The browser tab shows (1) New Expensify even though there are no unread messages in the LHN Sep 22, 2022
@adelekennedy
Copy link

Are we opening this back up for any proposals? @Beamanator @Santhosh-Sellavel

@Beamanator
Copy link
Contributor

Hmm are we able to consistently reproduce anymore? I don't have the (1) always there where it used to be, so let's figure out if it's still a problem & occurring for anyone first?

@marcaaron
Copy link
Contributor

Yes, it still happens.

@marcaaron
Copy link
Contributor

I have a PR to fix it if you want to look @Beamanator #11160

It's hacky but the issue is a browser bug and I have no better way to work around it.

@melvin-bot melvin-bot bot closed this as completed Oct 6, 2022
@adelekennedy
Copy link

Slack convo on payment here

@adelekennedy
Copy link

as discussed in Slack compensation is due for this job! @Santhosh-Sellavel @thesahindia sent you offers

@Santhosh-Sellavel
Copy link
Collaborator

Accepted thanks @adelekennedy!

@xtealer
Copy link

xtealer commented Nov 9, 2022

as discussed in Slack compensation is due for this job! @Santhosh-Sellavel @thesahindia sent you offers

Did you get a solution?

@cryptosmiler
Copy link

As discussed in Slack compensation is due for this job! @jliexpensify @UpworkBartkoski sent you offers

@xtealer
Copy link

xtealer commented Nov 9, 2022

I propose 2 changes. We must update the file updateUnread and we must call this function on back navigation events detected to force the stale document.title reference to update.

Reference on StackOverflow about Stale Navigation Title

updateUnread.js on Line 16 should change from document.title = ''; to history.replaceState(null, ''); so file looks like:

/**
 * Web browsers have a tab title and favicon which can be updated to show there are unread comments
 */
import CONFIG from '../../../CONFIG';

/**
 * Set the page title on web
 *
 * @param {Number} totalCount
 */
function updateUnread(totalCount) {
    const hasUnread = totalCount !== 0;

    // There is a Chrome browser bug that causes the title to revert back to the previous when we are navigating back. Setting the title to an empty string
    // seems to improve this issue.
    history.replaceState(null, '');
    document.title = hasUnread ? `(${totalCount}) ${CONFIG.SITE_TITLE}` : CONFIG.SITE_TITLE;
    document.getElementById('favicon').href = hasUnread ? CONFIG.FAVICON.UNREAD : CONFIG.FAVICON.DEFAULT;
}

export default updateUnread;

For the second change, we can implement a event listener to detect when user navigates back or forward so we can trigger a update when no new changes are received / detected by the Onyx listeners. This would be implemented on src/libs/UnreadIndicatorUpdater/index.js

@melvin-bot
Copy link

melvin-bot bot commented Nov 9, 2022

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.

@thesahindia
Copy link
Member

Accepted, thanks!

@thesahindia
Copy link
Member

@xtealer, this doesn't need a solution anymore. Please check others issues with the help wanted label.

@thesahindia
Copy link
Member

Accepted, thanks!

Bump @adelekennedy

@Santhosh-Sellavel
Copy link
Collaborator

@adelekennedy bump!

@adelekennedy
Copy link

These closed issues always get me - both have been paid out 2k

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Monthly KSv2 Planning Changes still in the thought process
Projects
None yet
Development

No branches or pull requests