[HOLD for payment 2024-04-03] HIGH: [Reliability] [$500] Unread fevicon does not show for new messages / unread messages

[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.51-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: @AndrewGable
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1710193817230399

Action Performed:

1. Sign in to a account as user A
2. Sign in to another account in another device as B
3. Send a message from B to A

Expected Result:

Fevicon should display unread icon

Actual Result:

Does not show

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.2850.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~011928bada81510714
• Upwork Job ID: 1768019508229373952
• Last Price Increase: 2024-03-13
• Automatic offers:
• situchan | Reviewer | 0
• gijoe0295 | Contributor | 0

[melvin-bot]

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

[gijoe0295]

Proposal

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

Unread fevicon does not show for new messages / unread messages.

What is the root cause of that problem?

This is about module importing in JS. In UnreadIndicatorUpdater, we used export default to export function:

App/src/libs/UnreadIndicatorUpdater/index.ts

Line 12 in 34930fe

export default function getUnreadReportsForUnreadIndicator(reports: OnyxCollection<Report>, currentReportID: string) {

When we used import UnreadIndicatorUpdater from './libs/UnreadIndicatorUpdater' in Expensify.tsx, it only imported the exported function, not executed the top-level code in the module including Onyx.connect:

App/src/Expensify.tsx

Line 33 in 34930fe

import UnreadIndicatorUpdater from './libs/UnreadIndicatorUpdater';

This has happened since we migrated Expensify to TS.

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

We should use import './libs/UnreadIndicatorUpdater' just like we did here:

App/src/Expensify.tsx

Line 29 in 34930fe

import './libs/Notification/PushNotification/subscribePushNotification';

What alternative solutions did you explore? (Optional)

NA

[situchan]

I'd like to review this as C+ since I have context while reviewing similar issues:

• MEDIUM: [$500] Unread indicator is showing in the fevicon even though there is nothing in LHN #31276
• LOW: [$500] Unread notification in fevicon does not update when the user is in another tab #32225

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~011928bada81510714

[melvin-bot]

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

[askavyblr]

Proposal

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

Unread favicon does not show for new messages

What is the root cause of that problem?

We need to update unreadReports and sync

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

App/src/libs/UnreadIndicatorUpdater/index.ts

Line 42 in 13f8bc0

const unreadReports = getUnreadReportsForUnreadIndicator(allReports, currentReportID);

What alternative solutions did you explore? (Optional)

N/A

[jeremy-croff]

Proposal

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

The favicon for unread is missing! Great catch btw

What is the root cause of that problem?

This is a regression from the typescript migration: #25231
There is functional difference between this and javascript. Changing to the previous javascript file mitigates this issue.

I believe this is a case of "Side effect imports" to understand the issue in depth.
Typescript has to be configured in order to preserve unused side effects, but without that, it will clean it up unused imports as an optimization. This is different from JS.

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

I agree with the previous post that we should import everything with import './libs/UnreadIndicatorUpdater' in the Expensify.tsx file to execute all the side effects. I just wanted to mention the Root Cause Analysis (RCA) and link the regression. We can also eliminate the lint exclusions // eslint-disable-next-line @typescript-eslint/no-unused-vars.

Confirmed this solution with the docs from https://www.typescriptlang.org/docs/handbook/2/modules.html

What alternative solutions did you explore? (Optional)

[Santhosh-Sellavel]

I'd like to review this as C+ since I have context while reviewing similar issues:

• MEDIUM: [$500] Unread indicator is showing in the fevicon even though there is nothing in LHN #31276
• LOW: [$500] Unread notification in fevicon does not update when the user is in another tab #32225

@garrettmknight Please assign @situchan as C+ here, I'll unassign myself here.

[melvin-bot]

❌ There was an error making the offer to @situchan for the Contributor role. The BZ member will need to manually hire the contributor.

[situchan]

Thanks

[situchan]

Thanks for the proposals.

While @jeremy-croff's root cause is more accurate, I think it's not fair for @gijoe0295 to ignore their efforts to find offending PR and solution first. Pointing out the offending PR itself mostly covers the root cause.
That being said, I recommend to assign @gijoe0295.
Proposal: #38256 (comment)

@jeremy-croff please feel free to comment with the reason if you disagree.
Assigned engineer will give the final call.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[jeremy-croff]

Thanks for the proposals.

While @jeremy-croff's root cause is more accurate, I think it's not fair for @gijoe0295 to ignore their efforts to find offending PR and solution first. Pointing out the offending PR itself mostly covers the root cause. That being said, I recommend to assign @gijoe0295. Proposal: #38256 (comment)

@jeremy-croff please feel free to comment with the reason if you disagree. Assigned engineer will give the final call. 🎀 👀 🎀 C+ reviewed

I know this situation is a bit complicated because I also feel bad for suggesting the same solution. The only improvement I could find was to remove the unnecessary lint rule.

However, based on your explanation, I would like to clarify a few things. Firstly, I was the one who identified the problematic PR and linked it to TypeScript migration, not @gijoe0295. Secondly, it appears that @gijoe0295 only edited the PR to indicate that the issue was caused by TypeScript migration after the fact."er I posted this root cause

[situchan]

However, based on your explanation, I would like to clarify a few things. Firstly, I was the one who identified the problematic PR and linked it to TypeScript migration, not @gijoe0295. Secondly, it appears that @gijoe0295 only edited the PR to indicate that the issue was caused by TypeScript migration after the fact."er I posted this root cause

As we know the offending line, the PR could have been identified in a few seconds using git bisect or git lens

[gijoe0295]

I was the one who identified the problematic PR and linked it to TypeScript migration

@jeremy-croff I think you might be mistaken here. I edited my proposal to point to TS migration before you did. And we all know which PR it is without explicitly link the PR 😄.

[jeremy-croff]

@situchan I see what you mean.

His root cause states specifically it's a wrong import, and that's it.But that import hadn't changed in over a year and that is why I wanted to point out the real root cause. He then posted he posted 3 lines of code all with different git history. For example it could've been the default export that occured during writing of the unit test of 8c3c4e2 also 2 months ago. since the import never changed right.

When I wrote my proposal I thought it would provide the actual clarity. As it brought in the specifics and the history the bug. Not leave you to assume based on your own usage of gitlens on 1 of the refererred lines of code.

But now I also see that @gijoe0295 edited his proposal to mention typescript, but I don't have the granular timestamp to know where it was me or him that posted it first. As both were 16 hours ago.
@gijoe0295 Maybe we found it at the same time, but the only reason I posted was because you missed this at first and I knew I could add value even if I don't actually do the work and you do 👍

[situchan]

@gijoe0295 updated proposal 25min before @jeremy-croff's post

[jeremy-croff]

@gijoe0295 I hope you understand where I came from, I drafted the proposal for far over 25 minutes so I was not trying to duplicate your proposal. I actually did not know you edited to add typescript before my post so my statement of me pointing it out first was genuine, but a misunderstanding.

Thanks for all the clarifications and @situchan there's no disagreement from me.

[situchan]

PR reached staging

[garrettmknight]

On prod, updating for payment

[gijoe0295]

@garrettmknight Payment due ytd.

[garrettmknight]

All paid out.

@situchan can you post the bug zero checklist when you get a chance?

[deetergp]

Bump @situchan

[deetergp]

@situchan can you post the bug zero checklist when you get a chance?

Another friendly bump, @situchan

[garrettmknight]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@situchan] The PR that introduced the bug has been identified. Link to the PR:
• [@situchan] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@situchan] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@situchan] Determine if we should create a regression test for this bug.
• [@situchan] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@garrettmknight] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[garrettmknight]

@situchan bumpity bump

[garrettmknight]

@situchan bump - please complete the checklist.

[deetergp]

@situchan Bump! Please finish the checklist. Thanks!

[situchan]

Sorry I am back from vacay

[situchan]

• The PR that introduced the bug has been identified. Link to the PR: [TS migration] Migrate 'Expensify.js' file to TypeScript #35296
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/35296/files#r1609288483
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A

Regression Test Proposal

1. Login to any account with several unread chats
2. [Web] Verify that browser's tab favicon shows unread status and tab title shows unread count
3. [iOS/Desktop] Verify that app's icon shows unread status

[deetergp]

Thanks @situchan and welcome back 🎉