[HOLD for payment 2024-06-06] [$2000] Wrong Image opens on HT Report in Safari

[kavimuru]

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

---

Action Performed:

1.Go to PROD.
2. Join the following channel https://new.expensify.com/r/8292963527436014
3. Scroll up and open shared images by users
4. After scrolling a little up again open some other image
5. Wrong image is displayed

Expected Result:

Correct image should be displayed

Actual Result:

Wrong image is displayed

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 / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number:
Reproducible in staging?: needs reproduction
Reproducible in production?: needs reproduction
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
Notes/Photos/Videos: Any additional supporting documentation

wrong-image-ht.mov

Expensify/Expensify Issue URL:
Issue reported by: @Talha345
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1690065844572829

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012f0ed142cb12018c
• Upwork Job ID: 1685003651569020928
• Last Price Increase: 2023-09-11
• Automatic offers:
• Talha345 | Reporter | 26772303

Issue Owner

Current Issue Owner: @CortneyOfstad

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[michaelhaxhiu]

Reproduced easily on Safari for me 👍

[michaelhaxhiu]

Not overdue, fielding proposals still

[mikesmnx]

Proposal

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

A wrong image (or a series of wrong images) is displayed if a user clicks on one of the attachments in the chat. I was able to reproduce this issue in both Safari and Chrome.

What is the root cause of that problem?

The FlatList settings that are being used in the AttachmentCarousel Component. The combination of these settings leads to the updatePage function being called with wrong parameters on carousel initialization and first scroll (via onViewableItemsChanged={updatePage}).

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

I changed viewabilityConfig, initialNumToRender, windowSize, maxToRenderPerBatch, bounces and pagingEnabled properties. The pagingEnabled and minimumViewTime parameters are mandatory, while the others are optional.

The carousel now works properly in Safari, on both desktop and mobile.

Explanation:

• minimumViewTime=300 (was none)
  Minimum amount of time (in milliseconds) that an item must be physically viewable before the viewability callback will be fired

Adding this to decrease the likelihood of triggering the viewability callback, which would lead to an updatePage call.

• initialNumToRender=1 (was 3)
  The initial amount of items to render

Since we're displaying only one attachment at a time, there's no need for a larger value.

• windowSize=3 (was 5)
  The number passed here is a measurement unit where 1 is equivalent to your viewport height. The default value is 21 (10 viewports above, 10 below, and one in between)

Since we're only displaying the previous or next attachment, there's no need for a larger window.

• maxToRenderPerBatch=1 (was 3)
  This controls the amount of items rendered per batch, which is the next chunk of items rendered on every scroll

Since we're displaying only one attachment at a time, there's no need for a larger value.

• bounces=true (was false), ios only
  When true, the scroll view bounces when it reaches the end of the content if the content is larger than the scroll view along the axis of the scroll direction.

Setting this to true will enhance the stability of the viewable items callback on iOS devices.

• pagingEnabled=false (was true)
  When true, the scroll view stops on multiples of the scroll view's size when scrolling

If the list contains items of inconsistent heights or sizes, enabling pagingEnabled can lead to awkward scroll stop positions and may inaccurately determine which items are viewable.

What alternative solutions did you explore? (Optional)

react-native-snap-carousel

[parasharrajat]

Thanks for the proposal @mikesmnx but it sounds incomplete. What settings of FlatList especially?

What did you change and why? How will it impact the App? Which flat List as we talking about?

[mikesmnx]

Hi @parasharrajat

FlatList has complex logic for determining viewable items. The carousel contains a FlatList, where each attachment is a FlatList item (all are hidden except for the selected one). With the previous settings, it incorrectly determined that one or more neighboring attachments were also viewable. As a result, either the wrong image was displayed, or all the attachments were displayed one by one.

Code changes:
src/components/AttachmentCarousel/index.js

const viewabilityConfig = {
    // To facilitate paging through the attachments, we want to consider an item "viewable" when it is
    // more than 90% visible. When that happens we update the page index in the state.
    itemVisiblePercentThreshold: 95,
    minimumViewTime: 300, 
};

<FlatList
                    keyboardShouldPersistTaps="handled"
                    listKey="AttachmentCarousel"
                    horizontal
                    decelerationRate="fast"
                    showsHorizontalScrollIndicator={false}
                    bounces={true}
                    // Scroll only one image at a time no matter how fast the user swipes
                    disableIntervalMomentum
                    pagingEnabled={false}
                    snapToAlignment="start"
                    snapToInterval={containerWidth}
                    // Enable scrolling by swiping on mobile (touch) devices only
                    // disable scroll for desktop/browsers because they add their scrollbars
                    // Enable scrolling FlatList only when PDF is not in a zoomed state
                    scrollEnabled={canUseTouchScreen && !isZoomed}
                    ref={scrollRef}
                    initialScrollIndex={page}
                    initialNumToRender={1}
                    windowSize={3}
                    maxToRenderPerBatch={1}
                    data={attachments}
                    CellRendererComponent={renderCell}
                    renderItem={renderItem}
                    getItemLayout={getItemLayout}
                    keyExtractor={(item) => item.source}
                    viewabilityConfig={viewabilityConfig}
                    onViewableItemsChanged={updatePage}
                />

[parasharrajat]

Reviewing this today. Need to reproduce the bug first.

[mikesmnx]

It's easier to reproduce the issue in Safari, especially on mobile devices, but Chrome will work too. Just open and close the attachment multiple times.

click-click.mp4

[melvin-bot]

Upwork job price has been updated to $2000

[CortneyOfstad]

Hi @parasharrajat — just checking to see if we're still on pace to have this merged early this week? Thanks!

[parasharrajat]

As I am back from vacay, I will continue the review and try to get this merged ASAP.

[CortneyOfstad]

Coming from this comment here are we still looking to merge today @parasharrajat? Thanks!

[deetergp]

The PR has been approved but we are currently under a merge freeze. It will likely not be merged until Thursday.

[parasharrajat]

Same as above.

[CortneyOfstad]

Merge freeze should end today 🤞

[deetergp]

Looks like the freeze is thawing slightly to a slush. Not yet sure what that means for a practical standpoint. This is a pretty big change, so we may need to thaw further before merging it.

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

• Enhance Carousel Scrolling and Interaction in AttachmentCarousel #39930

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

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

• @kidroca does not require payment (Contractor)
• @mikesmnx requires payment (Needs manual offer from BZ)
• @parasharrajat requires payment through NewDot Manual Requests
• @Talha345 requires payment automatic offer (Reporter)

[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.
• [@CortneyOfstad] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/402695

[CortneyOfstad]

@parasharrajat can you complete the checklist before tomorrow so there is no delay in payment? Thanks!

[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: None, it is an issue with FlatList on Safari.
• [@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: NA
• [@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: NA
• [@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 a chat with multiple attachments.
2. Open and close several attachments randomly.
3. Confirm that the attachment carousel always opens with the attachment that was selected.

Do you agree 👍 or 👎 ?

[CortneyOfstad]

@parasharrajat can you complete the checklist before tomorrow so there is no delay in payment?

Also, at @mikesmnx can you link your Upwork profile so I can send you a payment contract? After discussing it with the team, it was decided to offer you 25% for your initial work on this. Thanks!

[parasharrajat]

@CortneyOfstad I already did. Is there something else that I can help with?

[CortneyOfstad]

Sorry, there was a glitch that caused my edited comment to then post on it's own — sorry for the confusion!

[parasharrajat]

No worries. Thanks.

[mikesmnx]

Also, at @mikesmnx can you link your Upwork profile so I can send you a payment contract? After discussing it with the team, it was decided to offer you 25% for your initial work on this. Thanks!

@CortneyOfstad https://www.upwork.com/freelancers/~01ae736e03e7dce34c

[parasharrajat]

@mikesmnx Please close your old PR. Thanks.

[CortneyOfstad]

@mikesmnx and @Talha345 — I've sent you both offers in Upwork. Please let me know once you accept and I'll get those paid ASAP. Thanks!

[Talha345]

@mikesmnx and @Talha345 — I've sent you both offers in Upwork. Please let me know once you accept and I'll get those paid ASAP. Thanks!

@CortneyOfstad Offer accepted!

[CortneyOfstad]

Regression test created (here) so doing the payment summary now!

[CortneyOfstad]

Payment Summary

@kidroca — contracted
@parasharrajat — to be paid $2,000 via NewDot
@mikesmnx — paid $500 via Upwork (decided on 25% payout due to original PR creation and review process)
@Talha345 — paid $250 for reporting (GH was created pre-pricing change)

Thanks all and great work!

[parasharrajat]

Payment requested as per #23546 (comment)

[JmillsExpensify]

$2,000 approved for @parasharrajat