[HOLD for payment 2023-05-23] [$1000] image changes automatic when image preview is opened and sender sends any new image

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. click on + button and select new chat
2. create a new chat
3. from userA, send an image
4. for userB, click on that image to see full preview
5. from userA, send second image
6. check image preview on userB side

Expected Result:

Image should not changes automatically

Actual Result:

Image changes automatically

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.2
Reproducible in staging?: y
Reproducible in production?: y
Notes/Photos/Videos:

Screen.Recording.2023-04-20.at.12.08.18.PM.mov

Recording.292.mp4

Issue reported by: @gadhiyamanan
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1681972760701989

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f32126fc3213fe91
• Upwork Job ID: 1649466254320713728
• Last Price Increase: 2023-05-08

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[JmillsExpensify]

I'm not able to reproduce this one. I followed the reproduction steps using both file upload as well as drag and drop. The preview didn't switch on me. Mind confirming the bug can reproduced using these exact reproduction steps?

[gadhiyamanan]

@JmillsExpensify Please try to create a new chat, I am able to reproduce it

screen-recording-2023-04-21-at-101019-am-1_XzaoqqP6.mp4

[JmillsExpensify]

Ah ok! So creating a new chat does it for me as well. Thanks!

[MelvinBot]

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

[MelvinBot]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

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

[MelvinBot]

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

[allroundexperts]

For anyone who wants to take this on, here are my initial observations:

1. The issue happens for all the chats including old ones as well. To reproduce this, you have to open carousel and navigate to the first image. Once the first image is open, you can use the second account to send a new image. The Carousel will move to the next image. If the starting position is not the first image, the issue won't happen.
2. When the first image is open and a new one is received, the FlatList component is triggering onViewableItemsChanged method. This trigger causes the page to move forward. I'm not sure why onViewableItemsChanged gets triggered when you're on the first image only. Technically, it should not trigger even on the first image.
3. We can probably apply a patch in updatePage function here to fix this but that will be a work around IMO. The real fix would be check why the onViewableItemsChanged is getting triggered when you're on the first image and receive a new image.

@kidroca I see that you did some changes to this component a little while ago. Do you know what might be the issue here?

[akinwale]

Proposal

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

The current image being displayed in the attachment carousel automatically changes when a new image attachment is added to the report.

What is the root cause of that problem?

There are multiple things causing with the implemented logic in the AttachmentCarousel component.

In the makeStateWithReports method, the getSortedReportActions method is called with the messages sorted in descending order, however, attachments are being added to the resulting array with logic that assumes that the messages are sorted in ascending order.

App/src/components/AttachmentCarousel/index.js

Line 124 in 4947508

const actions = ReportActionsUtils.getSortedReportActions(_.values(this.props.reportActions), true);

App/src/components/AttachmentCarousel/index.js

Line 148 in 4947508

attachments.push({source, file: {name}});

When this happens on subsequent updates, the order of the attachments array changes, because the second image in the report now becomes the first in order (descending order), thereby causing the FlatList to re-render with the new image displayed.

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

Change the getSortedReportActions method call to sort the items in ascending order, then swap the checks for isForwardDisabled and isBackDisabled, and swap the parameters for the this.cycleThroughAttachments calls.

What alternative solutions did you explore? (Optional)

It's possible to keep the getSortedReportActions in descending order, and then use attachments.splice instead of attachments.push to add items to the array list in the correct order.

attachments.splice(0, 0, {source, file: {name}});

However, with this approach, there is a need to also add another loop to keep the correct page value based on the number of attachments.

if (attachments.length > 1) {
    _.forEach(attachments, (attachment, index) => {
        if (attachment.source === this.state.source) {
            page = index;
        }
    });
}

The check and parameter swaps in the proposed changes will also need to be applied. Ultimately, this will result in more expensive operations, which makes the original proposed change a better idea.

[kidroca]

@allroundexperts
onViewableItemsChanged is called because the visible image changes - we can observe the image changes even before the actual callback is triggered

The image changes because we insert an image in the list
The list knows it's at a given index, like 5. If when new attachment is added we insert it to the front (at index 0) then the images would shift up and so the image at index 5 would no longer be the same, because the image at index five now has a different key the callback onViewableItemsChanged is triggered

@akinwale If you reverse the list, you would have the same problem, just on the other side of the list

[akinwale]

@akinwale If you reverse the list, you would have the same problem, just on the other side of the list

I haven't seen the problem exhibit itself, and I don't think it happens if the list order always remains the same. We're always starting from the first item in the report, and so the page index always stays the same for the current item (if there's any). With ascending order, we're inserting the newer items at the end of the list, instead of the beginning (which is what is happening with the current descending order).

[kidroca]

Proposal

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

Observed attachment image might change if someone sends a new attachment while we have the attachment view/carousel open

What is the root cause of that problem?

The problem is we try to recalculate attachments and fit the new attachment into the list - why do we do that?
Imagine the reverse scenario - someone deletes an attachment from the chat while we're looking at it - what do we expect to happen

• do we expect to be on a different image?
• or do we expect to be able to see the deleted attachment until we close the modal?

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

A solution, like stop trying to recalculate attachments after the carousel is open, is simple and efficient
If attachment comments are added/deleted while the attachment view is open it should simply do nothing, because

• it's bad UX to be kicked out of (lose) an attachment while you're still looking at it
• new messages (adding attachments) are unread, having a new attachment added to the carousel, but not being able to distinguish it as something new is bad UX, when we receive new messages, in the chat, we can visually distinguish them as new - there's a new message indicator, in the context of the carousel there's no such thing - it would be better to exit the carousel and see what's new, instead of having it appended there automatically

The carousel should include all the attachments available at the time when we open the carousel
It should not include attachment from unread messages, because the images can't be distinguished as new
New attachments would be added once we exit, read the new message and click to see the attachment (open the carousel again)

(Just checked and WhatsApp's attachment carousel works this way - new attachment don't appear until you exit the carousel)

What alternative solutions did you explore? (Optional)

In my previous comment I've explained how content in the list might shift depending on where we insert data
If content is to change dynamically so that an image at index 5 is moved to index 6, and we're currently looking at that image, we should tell the list to scroll to index 6, when we're done with the recalculation of attachments

Simply said, any time we use the makeStateWithReports we should also tell the list to scroll to the calculated page index

(This should also cover images getting deleted and causing indexes to change, though if we were looking at a image that got deleted, we'll be moved to the neighboring image)

[allroundexperts]

@allroundexperts

onViewableItemsChanged is called because the visible image changes - we can observe the image changes even before the actual callback is triggered

The image changes because we insert an image in the list

The list knows it's at a given index, like 5. If when new attachment is added we insert it to the front (at index 0) then the images would shift up and so the image at index 5 would no longer be the same, because the image at index five now has a different key the callback onViewableItemsChanged is triggered

@akinwale If you reverse the list, you would have the same problem, just on the other side of the list

But why does it happen when you're on the first image only?

[sobitneupane]

Here are two cases:

• When user deletes a photo
• When user sends a new photo

I believe when user deletes an attachment, it should be removed from carousel as well (same is the case in WhatsApp as well). When user sends a new attachment, should it be accessible without closing the carousel and re-opening it?

@kidroca You are proposing not to remove the deleted image from the carousel until it is closed, right? It is not the case in WhatsApp. I think it makes sense to remove the image from the carousel once deleted.

@cead22 @JmillsExpensify What's your opinion on the expected output?

[akinwale]

Here are two cases:

• When user deletes a photo
• When user sends a new photo

I believe when user deletes an attachment, it should be removed from carousel as well (same is the case in WhatsApp as well). When user sends a new attachment, should it be accessible without closing the carousel and re-opening it?

For deletion, this will be a bit tricky if the image is the currently displayed image, because it will cause the FlatList to refresh, thereby resulting in confusion for a user who may be viewing the carousel at the time.

If the intent is to dynamically handle deletion, a good approach would be to only update carousel if:

1. The deleted image is not the current image, and
2. The deleted image has a higher index in the attachments list than the current image, preventing a reorder of page indexes which may cause the FlatList to flicker or refresh (this needs to be tested some more).

My proposal fixes the issue with dynamically added images changing the current displayed image fine, but not deletion. I can run some tests and update it to cover the deletion cases.

[akinwale]

@sobitneupane Another thing to note, at the moment, the current behaviour when an image attachment is deleted is that no changes happen in the carousel while it's already open. This is probably due to the fact that there is no DELETED report action. The getSortedReportActions method returns actions with type: CONST.REPORT.ACTIONS.TYPE.CREATED.

[JmillsExpensify]

Another thing to note, at the moment, the current behaviour when an image attachment is deleted is that no changes happen in the carousel while it's already open.

I'm not particularly passionate, though I agree that if we need to refresh the carousel for the case of deleted, I think that'd cause more confusion by the user – or perhaps more accurately, is jarring.

[melvin-bot]

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

[miljakljajic]

Also OOO, reapplying.

[melvin-bot]

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

[laurenreidexpensify]

@sobitneupane @kidroca @gadhiyamanan the original job expired on Upwork, I've resent you all offers on a new one - please apply here https://www.upwork.com/jobs/~01b269c8c394133c7a thanks

[gadhiyamanan]

@laurenreidexpensify Accpted, Thanks!

[laurenreidexpensify]

@sobitneupane have you had a chance to review regression steps above?

[laurenreidexpensify]

@sobitneupane @gadhiyamanan paid ✔️
waiting for @kidroca to accept job in upwork for final payment

[sobitneupane]

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:

#9279

• [@sobitneupane] 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:

#9279 (comment)

• [@sobitneupane] 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:

https://expensify.slack.com/archives/C049HHMV9SM/p1684918537590419

[sobitneupane]

Regression Test Proposal

1. Set up a chat between two users (User A and User B).
2. Have User A open the carousel and navigate to the very first image.
3. Have User B send a new attachment.
4. Verify that the currently viewed image in the carousel by User A doesn't change on receiving new attachment.

Do we agree 👍 or 👎

[kidroca]

@laurenreidexpensify am I eligable for the bonus mentioned here: #17743 (comment)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

• I was assigned for the ticket on May 9th - [HOLD for payment 2023-05-23] [$1000] image changes automatic when image preview is opened and sender sends any new image #17743 (comment)
• Create a PR which was merged on May 11th - Fix: Don't add new content an already opened carousel #18651 (comment)

The PR was merged within 72hrs of my assignment

[laurenreidexpensify]

sorry missed the bonus @kidroca @sobitneupane - fixed!

[laurenreidexpensify]

@sobitneupane regression test looks good, do you have a recommendation for which test it would live under in TestRail?

[sobitneupane]

@laurenreidexpensify I am not familiar with TestRail. Is there any document/ walkthrough video/ slack thread to familiarize with TestRail?

[laurenreidexpensify]

ah apols Sobit - Reading through https://github.com/Expensify/App/blob/main/contributingGuides/REGRESSION_TEST_BEST_PRACTICES.md I can see that you don't have access to TestRail, so ignore me!

Testing steps look good, have created an issue for it to get added to TestRail.

All steps completed here, closing. Thanks everyone!