[$2000] Wrong Image opens on HT Report in Safari

[kavimuru]

trafficstars

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
• [x] iOS / Safari
• [x] 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

https://github.com/Expensify/App/assets/43996225/e50e0735-8339-4df3-af36-f551d29bd479

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [x] 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
• [x] 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.
• [x] 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.
• [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[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.

https://github.com/Expensify/App/assets/12779176/df201468-8d77-4fdc-8c5a-ffe4bcbfc388

[melvin-bot[bot]]

Upwork job price has been updated to $2000

[melvin-bot[bot]]

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

[michaelhaxhiu]

Note: I'm preparing to go OOO for ~2 weeks and need a BZ buddy to watch over this in the meantime. 🙏

Next step: I just doubled the price. Please ensure that we push this forward and double price if no winning proposal in the next week.

Thanks in advance @laurenreidexpensify, I can take this back when I get back if it's not complete before then.

[suneox]

Proposal

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

Wrong Image opens on HT Report in Safari

What is the root cause of that problem?

The Flatlist fire the incorrect event onViewableItemsChanged when rendering the list without user action, it happens when device rendering low-performance on any device not only Safari

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

Remove the legacy attribute pagingEnabled (this attribute not found on "react-native": "0.72.1", and inherit virtualizedlist) ~~and use the attribute removeClippedSubviews to optimize the flatlist following official document In this case, we can use this attribute because each item on flatlist has full width so it doesn't affect in the note for this attribute~~

What alternative solutions did you explore? (Optional)

Option 2: We can prevent an unexpected events from callback onViewableItemsChanged by checking the condition current page with viewableItems, changed

https://github.com/Expensify/App/blob/4230f289341c8d6d9a8b780d54da0725d4764b2b/src/components/AttachmentCarousel/index.js#L175C5-L190C16

~~and update initialNumToRender, maxToRenderPerBatch to 1 get more perform~~

Result

This issue can reproduce easier by uploading > 30 images Look at the result before the change: bug-on web After change: after-fix

[suneox]

Actually, we can combine 2 solutions I have proposed, the first for performance and the second to prevent an unexpected event

[suneox]

Hi @michaelhaxhiu how about my proposal ? I have provided the way to reproduce and solution to prevent incorrect event. And we don’t need to update initialNumToRender, maxToRenderPerBatch to 1

[mikesmnx]

I believe there's no reason to resort to the second option. If the flatlist approach works, then the second method is absolutely unnecessary. If the flatlist approach doesn't work, the second method only masks the problem instead of truly resolving it.

The first option is identical to my approach, except for the use of removeClippedSubviews, which is even discouraged by the official documentation.

[suneox]

Option 1 still happens when you scroll faster and open image, so we need to apply more option 2 to prevent unexpected event, and an attribute removeclippedsubviews is available on official Optimizing Flatlist Configuration. If we don’t using this attribute it ok because when I perform rendering it faster a little bit if we scare missing content in spite of we have only 1 item full width

[laurenreidexpensify]

@parasharrajat bump for review. Thanks!

[melvin-bot[bot]]

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

[melvin-bot[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

[laurenreidexpensify]

@bfitzexpensify I am OOO From tomorrow afternoon for rest of week + some of next week, so reassigning as I can't cover for @michaelhaxhiu on this one. thanks

[melvin-bot[bot]]

@michaelhaxhiu @parasharrajat @bfitzexpensify 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!

[bfitzexpensify]

@parasharrajat another friendly bump to review - thank you!

[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? 💸

[bfitzexpensify]

@parasharrajat do you have capacity to review these proposals or should I assign another C+?

[parasharrajat]

Thanks for both your proposals. Can you please add explanations for each change? For example, why did you change a particular option and what is the effect of it?

[mikesmnx]

• 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.

I chose not to use removeClippedSubviews as it is still in experimental phase. The official documentation also cautions that this implementation can exhibit issues, such as missing content, especially on iOS.