LOW: [$500] Unread notification in fevicon does not update when the user is in another tab

[m-natarajan]

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

---

Version Number: 1.4.5.3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @iwiznia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1701197419116139

Action Performed:

1. Open staging.new.expensify.com and login
2. Open 2nd tab
3. Send a message from another device
4. Observe the fevicon in tab 1 where account A logged in

Expected Result:

Should update the fevicon notification count

Actual Result:

Notification count updates only when clicking the tab 1

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Recording.274.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013669a17c30bfb662
• Upwork Job ID: 1729978850198351872
• Last Price Increase: 2023-12-06
• Automatic offers:
• situchan | Contributor | 28044342

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013669a17c30bfb662

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[iwiznia]

@Julesssss seems to think this was related to this change https://github.com/Expensify/App/pull/31295/files#diff-0812bc9a1458d29651dc472eddb66ec3c320b32473461e75d8dda4fbd6362523L12-R36
Thread here https://expensify.slack.com/archives/C01GTK53T8Q/p1701204065146149

@hurali97 can you take a look?

[vjurcutiu]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The favicon does not update when a message is received in another tab.

What is the root cause of that problem?

In updateUnread/index.website.js we use a setTimeout function, which is no longer necessary after the change specified by @iwiznia.

What changes do you think we should make in order to solve the problem?

Remove the setTimeout function.
POC:

POC.-.favicon.mp4

What alternative solutions did you explore? (Optional)

[iwiznia]

I am confused, I see the setTimeout, is the setTimeout call never firing? And if so, why?

[vjurcutiu]

I don't know exactly. I assumed that since InteractionManager returns a promise, setTimeout was no longer needed, and that fixed it.

[ZhenjaHorbach]

I don't think they just added it)

App/src/libs/UnreadIndicatorUpdater/updateUnread/index.website.js

Lines 15 to 16 in 7ede114

	// This setTimeout is required because due to how react rendering messes with the DOM, the document title can't be modified synchronously, and we must wait until all JS is done
	// running before setting the title.

[vjurcutiu]

Yeah, but it was added before the InteractionManager change, right? It seems like a hack to simulate an asynchronous load. Since InteractionManager returns a promise, setTimeout was no longer needed. I know that setTimeout is in a different part of the event loop than a promise, so maybe somewhere along the way it gets tangled up.

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Unread notification in fevicon does not update when the user is in another tab

What is the root cause of that problem?

When the new message come to App, we will create the new action component. This component uses BaseTooltip, and onHoverOut callback is triggered immediately because we don't hover on that component.

-> Animated.timing().start() is triggered here

In the detail implementation of RNW, we use requestAnimationFrame, and this function intentionally pauses in the background. Ref: https://stackoverflow.com/questions/46765937/run-requestanimationframe-in-background

https://github.com/necolas/react-native-web/blob/a3ea2a0a4fd346a1b32e6cef527878c8e433eef8/packages/react-native-web/src/vendor/react-native/Animated/animations/TimingAnimation.js#L129-L131

In here

App/src/libs/UnreadIndicatorUpdater/index.js

Line 29 in d565caa

InteractionManager.runAfterInteractions(() => {

we just update unread count after all animations complete

-> We can see the updated unread count when back to the main device

What changes do you think we should make in order to solve the problem?

It doesn't make sense when running hideTooltip even when it never shows up. So we can avoid that by adding didHover value in Hoverable

    const didHover = useRef(false);

update didHover in here

            if(hovered && !didHover.current){
                didHover.current=true
            }

then update this condition to

        if (onHoverOut && !isHovered && didHover.current) {

Result

Screen.Recording.2023-11-30.at.17.20.08.mp4

[adelekennedy]

@hurali97 bump on the request above!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[mananjadhav]

Apologies for the delay here. I was out sick few days. I don't think we should be removing the setTimeout and neither did the solution worked for me.

@tienifr's solution did work for me, but I am trying to understand was this something that stopped working recently and do we have a linked commit that caused this?

I also noticed that the latest change was made by this PR last week by @allroundexperts, but even after reverting that, the issue is still reproducible.

@iwiznia Are you aware if this was working previously and recently stopped working?

[tienifr]

@mananjadhav After removing runAfterInteractions in this PR, I can't reproduce, but I don't know why the author remove this . I'm asking in here

[hurali97]

@tienifr If I understand correctly, the issue is resolved after this PR but we don't know why the author removed runAfterInteractions, correct?

[tienifr]

@hurali97 Yes

[iwiznia]

@iwiznia Are you aware if this was working previously and recently stopped working?

I don't know if this was working because I've been stuck between this bug and #31276

[kaushiktd]

Please re-state the problem that we are trying to solve in this issue.

Unread notification in fevicon does not update when the user is in another tab

What is the root cause of that problem?

The issue occurs because of stemming from the condition if (onHoverOut && !isHovered) triggering onHoverOut erroneously when isHovered becomes false. This condition needs a refinement to ensure that onHoverOut is only called when a hover event had previously occurred (isHovered was true), rather than simply when isHovered becomes false.

What changes do you think we should make in order to solve the problem?

https://github.com/Expensify/App/blob/main/src/components/Hoverable/index.tsx#L54

const [hasHovered, setHasHovered] = useState(false);

replace this code 
https://github.com/Expensify/App/blob/d565caa4c62ae23004ed2e4b13b7233259d75d2e/src/components/Hoverable/index.tsx#L133

   useEffect(() => {
       

        if (disabled) {
            return;
        }

        if (onHoverIn && isHovered) {
            return onHoverIn();
        }

        if (onHoverOut && !isHovered && hasHovered) {
            return onHoverOut();
        }
    }, [disabled, isHovered, onHoverIn, onHoverOut, hasHovered]);

In essence, hasHovered acts as a flag to determine whether a hover event has occurred at least once. If hasHovered is true and the current isHovered state becomes false, it will trigger the onHoverOut() callback, indicating that a hover-out event happened after a previous hover-in event.

Video:-

https://drive.google.com/file/d/1g-GuLQ2uRzgD0PVTgOm8FgJ1WuD8QwDx/view?usp=sharing

[melvin-bot]

@adelekennedy, @situchan 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

@adelekennedy, @situchan 12 days overdue now... This issue's end is nigh!

[melvin-bot]

This issue has not been updated in over 14 days. @adelekennedy, @situchan eroding to Weekly issue.

[adelekennedy]

@situchan one new proposal to review above

[situchan]

@kaushiktd Hoverable is base component. Please do thorough testing to make sure there's no regression.

[kaushiktd]

@situchan Yes I tested it and then only sent proposal. 🙂

[quinthar]

Hi there, what's the status of this? What's next, who's doing it, and when will it be done?

[adelekennedy]

conversation started here on bringing on an agency or reassigning

[situchan]

@kaushiktd your proposal is deprecated after #33257. Can you please update proposal accordingly?

[kaushiktd]

@situchan The new code seems to be fixing this issue. It is not reproducible on my end.

[adelekennedy]

this seems like a great time to try the retest weekly label

[quinthar]

@adelekennedy can you re-test to see if this is still the case?

[adelekennedy]

can't reproduce - closing this