[HOLD for payment 2024-01-28] CRITICAL: [$500] Switching from most recent to #focus shows already-read chats in the LHN

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

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

Action Performed:

Precondition: User must have several conversations and no unread chats. Priority mode is set to Most Recent

1. Refresh the page
2. Without taking other actions - Navigate to preferences and enable #focus in priority mode

Expected Result:

Only the current report should be visible in the LHN

Actual Result:

The LHN remains unchanged (or at least, many unread chats remain visible in the LHN)

Workaround:

If the user takes other actions before refreshing, the issue is not reproduced.

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

Reprod.in.prod.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0111ebc2af8eb5ed14
• Upwork Job ID: 1745951096565080064
• Last Price Increase: 2024-01-12
• Automatic offers:
• bernhardoj | Contributor | 28106614

Issue Owner

Current Issue Owner: @zanyrenney

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0111ebc2af8eb5ed14

[melvin-bot]

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

[melvin-bot]

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

[bernhardoj]

Proposal

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

Switching from the most recent fo #focus mode still shows all read chats

What is the root cause of that problem?

When we change the priority mode, it will recalculate the chats that should be shown on the LHN.

App/src/pages/home/sidebar/SidebarLinksData.js

Lines 75 to 88 in 753ce92

	const optionListItems = useMemo(() => {
	const reportIDs = SidebarUtils.getOrderedReportIDs(null, chatReports, betas, policies, priorityMode, allReportActions);
	if (deepEqual(reportIDsRef.current, reportIDs)) {
	return reportIDsRef.current;
	}
	// We need to update existing reports only once while loading because they are updated several times during loading and causes this regression: https://github.com/Expensify/App/issues/24596#issuecomment-1681679531
	// However, if the user is offline, we need to update the reports unconditionally, since the loading of report data might be stuck in this case.
	if (!isLoading || !reportIDsRef.current || network.isOffline) {
	reportIDsRef.current = reportIDs;
	}
	return reportIDsRef.current || [];
	}, [allReportActions, betas, chatReports, policies, priorityMode, isLoading, network.isOffline]);

It will first compare with the existing list whether something has changed or not, then, we will only get the updated list IF the app is not loading (!isLoading).

However, we have an issue where the isLoading onyx data is stuck at true. This happened before because we are ignoring the successData and failureData if the onyx updates from the server haven't changed, then we fix it by making sure both successData or failureData is always applied even when that case happen.

But this problem arose again because, in #33530, we are adding a new property called finallyData that we haven't included the case yet for the onyx update ignore fix.

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

We should include finallyData to be processed even when the onyx updates from the server haven't changed.

App/src/libs/actions/OnyxUpdates.ts

Lines 87 to 95 in 753ce92

	// In this case, we're already received the OnyxUpdate included in the response, so we don't need to apply it again.
	// However, we do need to apply the successData and failureData from the request
	if (type === CONST.ONYX_UPDATE_TYPES.HTTPS && request && response && (!isEmptyObject(request.successData) || !isEmptyObject(request.failureData))) {
	Log.info('[OnyxUpdateManager] Applying success or failure data from request without onyxData from response');
	// We use a spread here instead of delete because we don't want to change the response for other middlewares
	const {onyxData, ...responseWithoutOnyxData} = response;
	return applyHTTPSOnyxUpdates(request, responseWithoutOnyxData);
	}

|| !isEmptyObject(request.finallyData)

---

Another issue:

But another thing I notice with this switching mode is that the list could be late to be updated because we ignore the updates if isLoading is true which I think makes sense when we load the app for the first time because we want to wait for the OpenApp API to be fully complete first.

App/src/pages/home/sidebar/SidebarLinksData.js

Lines 84 to 86 in 753ce92

	if (!isLoading || !reportIDsRef.current || network.isOffline) {
	reportIDsRef.current = reportIDs;
	}

However, this becomes a problem when we switch from focus to most recent because it will also call OpenApp, so if we switch from focus -> most recent -> focus, we will see all read chats in the 2nd focus mode for a few seconds before the OpenApp completes.

Screen.Recording.2024-01-13.at.15.50.52.mov

We can fix this by adding another condition that checks if the priority mode is updated, then we will update the list.

const prevPriorityMode = usePrevious(priorityMode)

if (... || (prevPriorityMode !== priorityMode))
    // update the list
}

[zanyrenney]

@parasharrajat please review the proposal and let me know what you think.

[quinthar]

Added to #vip-vsb, thanks so much for taking this on! It's blocking #33619, which is a pretty important one.

[zanyrenney]

asked here - https://expensify.slack.com/archives/C01GTK53T8Q/p1705494618062199

[parasharrajat]

Reviewing...

[parasharrajat]

@bernhardoj's proposal is spot on.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[dangrous]

This sounds great! Assigning.

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[quinthar]

What's the ETA for this?

[quinthar]

What's the ETA for review?

[bernhardoj]

The PR is merged

[dangrous]

Yep! This is on staging now, being QAd

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.28-0 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix switching priority mode to focus still shows read chats #34702

If no regressions arise, payment will be issued on 2024-01-28. 🎊

For reference, here are some details about the assignees on this issue:

• @parasharrajat requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
• [@parasharrajat] 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:
• [@parasharrajat] 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:
• [@parasharrajat] Determine if we should create a regression test for this bug.
• [@parasharrajat] 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.
• [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[parasharrajat]

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:

• [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: Add finallyData as alternative to successData and failureData #33530
• [@parasharrajat] 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: Add finallyData as alternative to successData and failureData #33530 (comment)
• [@parasharrajat] 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: Not needed.
• [@parasharrajat] Determine if we should create a regression test for this bug. Yes
• [@parasharrajat] 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.

Regression Test Steps

1. Open the app
2. Go to priority mode settings
3. Switch the priority mode between Most Recent and #focus multiple times
4. Verify the LHN list is updated correctly (Most Recent should show all chats, #focus should hide read chats)

Do you agree 👍 or 👎 ?

[garrettmknight]

@parasharrajat I'm trying to figure out whether the PR that caused this is right. #34466 was discovered in v1.4.24-7 and the deploy blocker slack thread shows it in production. #33530 was deployed to staging in 1.4.24-4, to production in 1.4.24-8. Are you sure that's the right one?

[garrettmknight]

@parasharrajat bump on the above

[parasharrajat]

Hmm, I was also a little confused by this. I tried to pinpoint the PR where I finallyData was added. IMO, PR should have similarly handled the other cases like they were done for successData or failureData.

It might be possible that logic related to this issue was refactored later in some other PR which triggered this issue.

I could use some help here if we want to know the exact PR that made the bug noticeable.

[dangrous]

So based on the solve, as well as this being a user (Expensify employee) reported bug, I think this may be one of those bugs that's been there pretty much since the beginning, we maybe just didn't notice it? We didn't appear to have regression tests that would catch it so it'll be unclear when it started.

That might be a cop out, though - I didn't investigate too long - but it feels like it might be the case

[dangrous]

let me know what you think of that ^ @garrettmknight and @parasharrajat.

[parasharrajat]

It is possible. I think we are fine with the above-linked PR. IMO PR could have solved this issue if the finallyData case was implemented as well.

[dangrous]

Yeah I think it's kind of a mix - the bug existed likely since the beginning, and then that linked PR could have fixed it but didn't since it didn't apply the onyx updates for finallydata

[dangrous]

Following up on this @garrettmknight @zanyrenney should we investigate this further, or are we good to move forward on payment?

[garrettmknight]

Hey @dangrous - no need to investigate further. We're good to pay out here.

[dangrous]

For reference, here are some details about the assignees on this issue:

• @parasharrajat requires payment through NewDot Manual Requests
• @bernhardoj requires payment automatic offer (Contributor)

Great! @zanyrenney we're ready to go here - thanks!

[zanyrenney]

Thanks @danieldoglas

[zanyrenney]

payment summary:

@parasharrajat requires payment through NewDot Manual Requests - please request $500 for this issue
@bernhardoj requires payment automatic offer (Contributor)**** paid $500 via upwork!

[parasharrajat]

Payment requested as per #34466 (comment)

[JmillsExpensify]

$500 approved for @parasharrajat