[$250] Search - Workspace chat preview, flickers between last message and information message

[IuliiaHerets]

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: 9.0.69-1 Reproducible in staging?: Y Reproducible in production?: N If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Y Issue reported by: Applause Internal Team

Action Performed:

1. Open the staging.new.expensify.com website.
2. Tap on "Settings" and select "Workspaces"
3. Open any workspace and tap on "Members"
4. Tap on "Invite Member"
5. Invite any member to workspace.
6. Return to LHN.
7. Open the workspace individual chat created for this invited member.
8. Send any message on it.
9. Tap on the search icon.
10. Start writing a search related to this last chat.
11. Verify that with every letter added on the search input, this chat preview keeps displaying the last sent message.

Expected Result:

The preview of the individual chat created for an invited workspace member, should keep displaying the last sent message when writing on the search input.

Actual Result:

Chat preview of individual chat created for an invited workspace member, flickers between the last sent message and the workspace information message with every letter added on the search input.

Workaround:

Unknown

Platforms:

• [x] Android: Standalone
• [x] Android: HybridApp
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/b99fa083-f588-49e1-91ab-138716a56f76

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021863580183549143544
• Upwork Job ID: 1863580183549143544
• Last Price Increase: 2024-12-09

Issue Owner

Current Issue Owner: @rojiphil

[melvin-bot[bot]]

Triggered auto assignment to @isabelastisser (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[bot]]

Triggered auto assignment to @Beamanator (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot[bot]]

💬 A slack conversation has been started in #expensify-open-source

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[mountiny]

This is a minor ux issue, feels like the state is changing between optimistic offline and the response from the server

Demoted

[mountiny]

cc @luacmartins @lakchote you might know the best about changes related to this area of code

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Beamanator]

Demoting makes sense to me 👍 making this external since the flicker sounds like a UI-only bug

[bernhardoj]

Proposal

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

Workspace chat subtitle text flickers when typing on search modal.

What is the root cause of that problem?

The flicker changes between the last message and the workspace name. When we get the option list subtitle, we mutate the original report object. https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L1196

This report object comes from OptionsListContextProvider which shares the options list between multiple components. In our case, when we open the search page, there are 3 usages that calls getOptions, two of the usages pass forcePolicyNamePreview as true and the 3rd one passes forcePolicyNamePreview as false, and showChatPreviewLine as true.

https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L435-L437

This happens after https://github.com/Expensify/App/pull/53293 which adds the showChatPreviewLine to fix https://github.com/Expensify/App/issues/53291. But from my testing, even if I remove the changes from that PR, the issue doesn't happen anymore. I'm not sure what causes that issue (53291), but the changes from PR (53293) add the showChatPreviewLine to the participantsAutocompleteList section, while the issue happens on the recent reports/chats section.

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

We can probably revert https://github.com/Expensify/App/pull/53293, but I think we should avoid mutating the original object in the first place. To do that, we need to convert this into a const first. https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L1196

const alternateText = getAlternateText(reportOption, {showChatPreviewLine, forcePolicyNamePreview});

And instead of using the reportOption, we need to clone the object first. https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L1244-L1248

const newReportOption = {
    ...reportOption,
    alternateText,
}

We can do the same for this: https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L1238-L1239 https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L1261

But I think the reason we mutate it so we don't affect the performance from the cloning operation. If we don't want to do the cloning, we can do the hard way, that is by changing the item structure to be like this:

{
option: reportOption,
alternateText,
isSelected,
...
}

There will be a lot of reactors and it actually looks similar to this. https://github.com/Expensify/App/blob/892cef17c0b6ed1cdd9a8bc5de437bc2dfb5c921/src/libs/OptionsListUtils.ts#L49-L51

[aldo-expensify]

but I think we should avoid mutating the original object in the first place. To do that, we need to convert this into a const first.

I think these mutations are very prone to introducing hard to find bugs so they should be removed.

But I think the reason we mutate it so we don't affect the performance from the cloning operation.

I'm pretty sure we can avoid performance issues without introducing mutations. How many options are we processing here?

We can do the same for this: reportOption.isSelected = isReportSelected(reportOption, selectedOptions); reportOption.isBold = shouldBoldTitleByDefault || shouldUseBoldText(reportOption);

Yes, lets just avoid cloning the same object each time a key is changed :P

[luacmartins]

I agree that we should remove the mutations.

I'm pretty sure we can avoid performance issues without introducing mutations. How many options are we processing here?

This function is used by the LHN as well, so we are processing the full LHN results for users. Not sure how large that data set is though.

[UmairMukhtar]

https://github.com/user-attachments/assets/6659b40d-f930-4c60-bd45-0de608e5f763

This issue does not exist anymore and may be closed

[melvin-bot[bot]]

📣 @UmairMukhtar! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[UmairMukhtar]

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~0102d6a693d924b83b

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[isabelastisser]

@rojiphil, any updates here?

[melvin-bot[bot]]

@rojiphil, @Beamanator, @isabelastisser Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[rojiphil]

@isabelastisser Sorry for the delay here. I have picked this for review now and will update sometime today.

[rojiphil]

@bernhardoj I am not able to reproduce this issue with the latest main. Can you reproduce?

However, I do find the mutation problem interesting. If mutations are causing problems, they must be manifested elsewhere, too. Did you find any other issues due to these mutations?

https://github.com/user-attachments/assets/45fee654-dbe1-4e34-9822-c97e4797e621

[bernhardoj]

Yeah, I can't see the flickering anymore too, but you can see that it still shows the last message of the policy expense chat even though we set forcePolicyNamePreview as true. https://github.com/Expensify/App/blob/7925c9b555af60156c62947ae63b517d6c31587f/src/libs/OptionsListUtils.ts#L1297-L1306

If we don't mutate the object, then we will see the policy name as the subtitle.

This is another issue caused by object mutation: https://github.com/Expensify/App/issues/52975

[isabelastisser]

@rojiphil, please provide an update. Thanks!

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@rojiphil, @Beamanator, @isabelastisser Whoops! This issue is 2 days overdue. Let's get this updated quick!

[rojiphil]

Yeah, I can't see the flickering anymore too, but you can see that it still shows the last message of the policy expense chat even though we set forcePolicyNamePreview as true.

@bernhardoj The expected result here is to display the last message text which is what it does. So, I am not sure what the issue is here. Can you please explain a bit more on this?

This is another issue caused by object mutation: #52975

That is another issue and you are already handling it there. But, I am curious to know if we can reproduce a problem in our context here with any other variable e.g. isSelected, isBold as referred in your proposal.

[bernhardoj]

The expected result here is to display the last message text which is what it does. So, I am not sure what the issue is here. Can you please explain a bit more on this?

Based on the code, the expected result is to show the policy name as explained in my previous comment. You can revert https://github.com/Expensify/App/pull/53293 and see it will show the policy name.

But, I am curious to know if we can reproduce a problem in our context here with any other variable e.g. isSelected, isBold as referred in your proposal.

You can change this to true https://github.com/Expensify/App/blob/3982ed03ff644e7a723e8d4131535a2cceb9488d/src/components/Search/SearchRouter/SearchRouterList.tsx#L159

and search for a DM and the title will be bold even though not bold on the LHN.

[melvin-bot[bot]]

@rojiphil @Beamanator @isabelastisser this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[isabelastisser]

@rojiphil, please share an update. Thanks!

[rojiphil]

Based on the code, the expected result is to show the policy name as explained in my previous comment. You can revert #53293 and see it will show the policy name.

@bernhardoj The mentioned PR has been in production for the last 15 days and seems acceptable to all. So, I am still not sure why a revert is needed. Can you please point out the relevant PR/issue where the expected result is to show the policy name?

[bernhardoj]

So, I am still not sure why a revert is needed.

I'm not saying that the solution here is to revert, but fix the mutation issue. I just mentioned that you can revert and see the behavior before that PR to prove the issue that is caused by mutating an object.

Can you please point out the relevant PR/issue where the expected result is to show the policy name?

I'm not aware of this, but the code was added in https://github.com/Expensify/App/pull/3540.

If the current behavior is acceptable, then we can close this and wait until another unexpected issue caused by the mutation side-effect comes up.