[$500] Android - An extra space is created at the top of the chat list when pinning and unpinning

[lanitochka17]

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.45.0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/runs/view/20070&group_by=cases:section_id&group_order=asc&group_id=296775
Email or phone of affected tester (no customers): nathanmulugetatesting@gmail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Start a conversation between two accounts, account A (main testing device) account B on another device
2. Send an IOU from account B to account A (to the main testing device)
3. Pay the IOU from account A (on android)
4. Go to the LHN and unpin any other conversation you have and let the conversation you had with account B be at the top
5. Hold on the chat with account B and pin it
6. Hold on the chat with account B again and unpin it right away

Expected Result:

There should be no extra space between the top chat and the title on the LHN

Actual Result:

An extra space gets created between the first chat at the top and the title

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0110a8f38476d1ad2f
• Upwork Job ID: 1763342127580393472
• Last Price Increase: 2024-03-07
• Automatic offers:
• aimane-chnaif | Reviewer | 0
• dukenv0307 | Contributor | 0

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp
CC @quinthar

[lanitochka17]

@johncschuster FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[melvin-bot]

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

[melvin-bot]

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

[dukenv0307]

Proposal

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

An extra space gets created between the first chat at the top and the title

What is the root cause of that problem?

In Step 4

Go to the LHN and unpin any other conversation you have and let the conversation you had with account B be at the top

The conversation with B is at the top, but actually in the reportIDs list here, there's another reportID of the IOU that's ordered above this report. The reason why that IOU report does not show is because its notificationPreference is hidden and will early return here.

When the user pin/unpin the chat report, the orders or that chat report and the IOU report will be switched, although the IOU report is hidden, it will still sometimes occupy space when the FlashList is rerendered because we always have the same estimatedItemSize here regardless of if we'll display the item or not.

That's why the blank space will show.

To quickly validate that this root cause is correct, we can simply add the below to here to make sure there's no "item included in list but does not render" and we'll see that the problem no longer occurs

if (report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
    return false;
}

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

We should consolidate the logic of "should we show this report in LHN?" in once place rather than two currently:

• In the filtering logic here
• In the item rendering here

We should move the logic in the item rendering to the reportIDs filtering logic, ideally here. So that if an item is not to be rendered, it will not appear in the reportIDs list passed into the FlashList at all. FlashList also expects the items passed to it to be rendered (because of the estimatedItemSize), not return null as we're doing.

So in this particular issue, the reportIDs list passed to FlashList will remain consistent when we pin/unpin the chat report, and the list will stay the same.

What alternative solutions did you explore? (Optional)

NA

[aimane-chnaif]

Looks like this is marked as android only issue.
@dukenv0307 can you please confirm? If so, please explain why this happens only on android.

[melvin-bot]

@johncschuster, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[johncschuster]

@dukenv0307 bump on @aimane-chnaif's question above!

[dukenv0307]

I'll post an update tomorrow, thanks for the patience!

[melvin-bot]

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

[dukenv0307]

Looks like this is marked as android only issue.
@dukenv0307 can you please confirm?

@aimane-chnaif This issue happens on iOS as well, it's not Android-only.

[dukenv0307]

To quickly validate that this root cause is correct, we can simply add the below to here to make sure there's no "item included in list but does not render" and we'll see that the problem no longer occurs

if (report.notificationPreference === CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
    return false;
}

@aimane-chnaif I've added the above to the proposal, hope it makes it easier to validate the RCA is correct 👍

[melvin-bot]

@johncschuster, @aimane-chnaif Huh... This is 4 days overdue. Who can take care of this?

[johncschuster]

Bumped in Slack

[aimane-chnaif]

@dukenv0307's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[aimane-chnaif]

@lanitochka17 repro video is missing in OP

[Julesssss]

can you share a video of the behavior if you're unable to reproduce?

Instead, could you please share a video of the reproduction so I can make sure I'm following the steps correctly

[dukenv0307]

Instead, could you please share a video of the reproduction so I can make sure I'm following the steps correctly

@Julesssss Sure, here you go:
https://github.com/Expensify/App/assets/129500732/1e12350d-187c-4302-bc1e-302a70f1d01e

I just followed the steps in the OP, I used an account with ~10 existing reports.

[Julesssss]

Ah, I forgot to unpin my chats.

[melvin-bot]

📣 @aimane-chnaif 🎉 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

[melvin-bot]

📣 @dukenv0307 🎉 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 📖

[melvin-bot]

@johncschuster, @Julesssss, @aimane-chnaif, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[johncschuster]

This was just assigned out, Melvin. Chill.

[melvin-bot]

This issue has not been updated in over 15 days. @johncschuster, @Julesssss, @aimane-chnaif, @dukenv0307 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[aimane-chnaif]

Payment is remaining

[johncschuster]

Thanks for the comment, @aimane-chnaif! I'm having a hard time finding the PRs that fix this issue. Can you point me in the right direction?

Is it #38670, or is it something else?

I'm specifically looking for the merge date so I can calculate the payment date. Thanks!

[aimane-chnaif]

#38656 is PR that fixes this issue.

[johncschuster]

Payment has been issued to @aimane-chnaif and @dukenv0307. Thanks for your contributions! 🎉

[johncschuster]

@Julesssss is there a BZ Checklist that needs to be completed?

[aimane-chnaif]

No need new regression test. Already exists - https://expensify.testrail.io/index.php?/runs/view/20070&group_by=cases:section_id&group_order=asc&group_id=296775

[johncschuster]

Great! It sounds like we can go ahead and close this issue then. Thanks, everyone!