[HOLD for payment 2024-06-28] [$250] [CRITICAL] [UX Reliability] The chat finder page shows duplicated result for a moment

[mountiny]

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:
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: @quinthar
Slack conversation: https://expensify.slack.com/archives/C05LX9D6E07/p1717508731782149

Action Performed:

Break down in numbered steps

1. Create Workspace as Alice
2. Add couple members including Bob
3. Create couple rooms on that workspace
4. Sign into an account off Bob
5. Turn on the focus mode
6. Sign out and sign back in as bob
7. In chat search page search for the name of the room on the policy
8. Open it the room from the Search page

Expected Result:

Describe what you think should've happened

There should only be one row for that chat

Actual Result:

Describe what actually happened

The specific chat is duplicated in the search results

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0157768929ce5b2314
• Upwork Job ID: 1798292023932084224
• Last Price Increase: 2024-06-05
• Automatic offers:
• ahmedGaber93 | Reviewer | 102621261

Issue Owner

Current Issue Owner: @dylanexpensify

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0157768929ce5b2314

[melvin-bot]

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot]

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

[dragnoir]

I think it's a dupe of #43049

[gedu]

Hey, I'm Edu from Callstack, I can take this issue

[tienifr]

@gedu I already had the proposal in #43049 (comment). cc @ahmedGaber93

[mountiny]

Nice @gedu can you check out the proposal if it matches your findings?

[ahmedGaber93]

@tienifr your solution is works, but I think your RCA is not correct, log missingReportIds before your fix seem correct, and after applied your fix it always logs empty array. Maybe we need to find the correct root cause first.

[ahmedGaber93]

This proposal from 2 min ago #43049 (comment) in the other issue add RTC about what I am thinking. But I think we no longer accept new proposals now after the agency member requests to work on this. Also, I'm still not sure if this is the desired fix

[gedu]

@mountiny

Proposal

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

After creating a room and searching for it, the room is displayed twice in the search results (no cache).

What is the root cause of that problem?

There are two useEffect hooks listening to the same object reports. The first hook updates the options with the new list of options inside setOptions using the state provided by the function. The second hook uses prevReports to find a missing reportID (which in this case is the new report), and updates the same setOptions state, adding the "missing" report to the already updated options (added by the first useEffect), cause the duplication.

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

Since the first useEffect always recreates the options when reports change, the second useEffect detects a difference and updates the state over an already updated one. To solve this, unify the two useEffect hooks to update the report options whenever the reports object changes, avoiding extra logic and potential re-renders.

What alternative solutions did you explore? (Optional)

Before pushing the missing report ID found by the filter, in the second useEffect, check if the reportID is already in the option list.

I tested some older issues, like the #39705 and the main fix doesn't cause a regression on that side, I'm not sure if there is another bug to address

[dragnoir]

@ahmedGaber93 pls check mine #43049 (comment)

[ahmedGaber93]

@dragnoir Sorry, We no longer accept new proposals after the agency member requests to work on this. I just will review proposals before that.

[dragnoir]

@ahmedGaber93 thank you 🥰

[ahmedGaber93]

I think this issue is a regression from #41636 which delete the return early on the first useEffect, and this make the new reports are handling in two useEffect

- const lastUpdatedReport = ReportUtils.getLastUpdatedReport();

- if (!lastUpdatedReport) {
-       return;
- }

To solve this, unify the two useEffect hooks to update the report options whenever the reports object changes

@gedu I agree, we need to make sure the new report is handled once, but not necessarily we need to unify the two useEffect, we can also keep them separator and prevent interference between them. @gedu Can you share how you do that?

Before pushing the missing report ID found by the filter, in the second useEffect, check if the reportID is already in the option list.

I think this will always cancel the second useEffect work, as the first useEffect always add it first.

[tienifr]

@ahmedGaber93

Maybe we need to find the correct root cause first.

Since we have 2 useEffects and the first useEffect already update the newest reports. But we are not sure about the reports of second useEffect is up-to-date that why we have the logic to find the missingReportIDs before.

[ahmedGaber93]

Yeah, but missingReportIDs will be always empty array as the first useEffect add them first, so it seems we completely remove the second useEffect

[ahmedGaber93]

The RCA here is we have two useEffect will add the new report to the list:

• the second useEffect job is handling the new items and it works fine

• the first one job is updating an existing item when a report is updated. And it shouldn't handle adding new items, but PR fix: deleted workspace room display in task share somewhere #41636 removed the return early when no existing item need to update, and this let it adding the new items and cause this regression

@dylanexpensify As no proposal is accepted yet, let's assign @gedu from callstack.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[melvin-bot]

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

Offer link
Upwork job

[ahmedGaber93]

but the createOptionList is always getting call

@gedu Yes, but this is part of the regression by #41636 that we need to fix it here

This regression remove the return early in the first useEffect and this caused the current issue and also the performance issue, so I think after fix it, the both issues should be fixed.

Can you please take a look into #41636 changes to understand what I mean.

[gedu]

Yes, I will take a look, sorry for the ping pong question, trying to wrap my head on this because with just one useEffect fix all the issues, will dig deeper

[gedu]

@ahmedGaber93 I'm trying to find a way to prevent the first useEffect executes the createOptionList but there is always a case that doesn't work.
The last thing I tried was, just listening to the getLastUpdatedReport just to check if it was in the options, remove it and let the second useEffect create that option when checking for the missingReportID, but the update doesn't propagate to all reports, I updated the name of a workspace (Callstack3 to Callstack4) and the main one did change but the rooms not, because the getLastUpdatedReport can't send all the changes.

I will try tomorrow other possibilities, probably check if we can improve the performance of createOptionList

[ahmedGaber93]

@gedu Great investigation 👍, I think it would be great if we can find a solution that updates the affected reports only and not the whole list.

[gedu]

@ahmedGaber93 I created a new function to listen when a report changes, I listen to them individually so I can save them into a Set and when I use the updated IDs I clear the Set. That way when a report changes I can know which one change, I delete it from the prevReports, and the second useEffect handle the missingReportID.
The issue that I'm having is, when an update changes in this case changing the name of the Workspace, it seems that all the rooms depending on it aren't sent via Onyx, same issue as above, but in this case the room changed but not the Workspace.
I'm not finding a predictable way to know which report changes so I can remove it from the list and later create the optionItem just for that items.

I would start by considering my proposal to unify and update the list continuously. Additionally, I will work on a follow-up PR to improve the performance of the createOptionList, given that the only way to know when a report changes is through Onyx, and I can't get reliable values.

[gedu]

@ahmedGaber93 I make my PR ready for review #43164

[dylanexpensify]

waiting for regression period

[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 9.0.0-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Search result duplication #43164

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

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

• @gedu does not require payment (Contractor)
• @ahmedGaber93 requires payment automatic offer (Reviewer)

[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:

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

[ahmedGaber93]

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:

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR: fix: deleted workspace room display in task share somewhere #41636
• [@ahmedGaber93] 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/41636/files#r1660214150
• [@ahmedGaber93] 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
• [@ahmedGaber93] Determine if we should create a regression test for this bug.
• [@ahmedGaber93] 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 Proposal

1. Open the App
2. Click Account Settings > Workspaces > and create new workspace
3. Click fab icon > Start chat > Room tab
4. Fill room name and select the workspace you create it in step 1 then click create room
5. Open Search page and Verify that the room you created in step 4 appear once in search result

Do we agree 👍 or 👎

[dylanexpensify]

Payment summary:

• contributor+ @ahmedGaber93 $250

Please apply, request!

[dylanexpensify]

Done!