search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

[$1000] Zoom view in the attachment preview is lost with 4 or more attachments #18614

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

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. Open the app
  2. Open report with 4 or more image attachments
  3. Open first image and zoom in
  4. Navigate back 4 images and again navigate back to first image and observe that image is zoomed out
  5. Repeat step 3 and 4 with 3 images in step 4 and observe that image is zoomed in

Expected Result:

App should remember zoomed in position throughout the attachment navigations as it does when we only navigate back 3 images

Actual Result:

Image is zoomed out when we navigate back 4 images in attachment preview

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?

Version Number: 1.3.11.2 Reproducible in staging?: y Reproducible in production?: y 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://user-images.githubusercontent.com/43996225/236950717-3feb43b9-3263-4535-b4c9-a565cc919411.mp4

https://user-images.githubusercontent.com/43996225/236950730-c7c1c16f-e69a-48fe-a7e5-2db537b21442.mp4

Expensify/Expensify Issue URL: Issue reported by: @dhanashree-sawant Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1683535771247579

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0185edcdfdafdda6aa
  • Upwork Job ID: 1657031802775678976
  • Last Price Increase: 2023-05-26
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

therealsujitk commented 1 year ago

Proposal

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

Zoom gets reset after navigating 3 attachments before or after.

What is the root cause of that problem?

This isn't a bug, it's intended behaviour. To prevent devices from lagging/crashing it is limited to load three attachments at the same time, after which the furthest attachment gets unloaded to clear up cache. This is done using the maxToRenderPerBatch attribute in line 375 in the code shown below.

https://github.com/Expensify/App/blob/bc8cc52162dca556db0a9b4a4719d09682e49745/src/components/AttachmentCarousel/index.js#L347-L383

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

To keep behaviour consistent, we could store data related to the state of the attachment (such as position, scroll, etc.) in an array. This way we can restore it's state when it's reloaded. For this an attribute (ex: initialState) can be created in the AttachmentView component to specify what the view should look like when initially loaded.

In our makeInitialState() function where we create an array of attachments, we also create a local variable to store attachment states.

this.attachmentStates = new Array(attachments.length).fill({});

Note: We don't need to store this in the react state because we only need to use it initially when the AttachmentView component mounts.

We then create a function to update this state whenever required.

/**
 * Updates the attachment state to restore it later
 * @param {Object} state current attachment state
 * @param {Number} index index of the attachment to update
 */
updateAttachmentState(state, index) {
    this.attachmentStates[index] = state;
}

Our AttachmentView component will then look like this. These props will be then passed down to ImageView and PDFView.

<AttachmentView
    source={authSource}
    file={item.file}
    initialState={this.attachmentStates[index]}
    onStateUpdate={(state) => this.updateAttachmentState(state, index)}
/>

Finally in the ImageView and PDFView we read the initial state in the constructor.

this.state = {
    isLoading: true,
    containerHeight: 0,
    ...
    ...this.props.initialState,
};

And in the componentWillUnmount we use the this.props.onStateUpdate() function since it will only be required if the component is unmounted.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

michaelhaxhiu commented 1 year ago

small note: re-assigning as I'm OOO till next week

maddylewis commented 1 year ago

i am OOO starting tomorrow but will try to repro this / push this along

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

maddylewis commented 1 year ago

whew, i am so sorry to do this @miljakljajic. i was reassigned this issue when i was about to take OOO and am confused by the repro steps / didn't have a chance to get to this one today.

if you have a chance Thurs/Fri, could you try reproducing this one and adding the external or internal label for someone to start working on this one?

then, once im back on Monday, i can take over triaging the issue from there.

sorry again. i don't like pawning GHs off to others, but this one has already been sitting for 2 days so wanted to get it assigned to someone. thx!

miljakljajic commented 1 year ago

I was able to reproduce this:

https://github.com/Expensify/App/assets/28715117/e9b05c0c-d452-47c9-aee2-88daf4639222

No problem, Maddy. I'll OOO travelling to EC3 on Monday but you can pick it up from there.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

Thanks for the proposal @therealsujitk, I will be testing this later today , given that you said this is not a bug and that's the intended behavior

This isn't a bug, it's intended behaviour. To prevent devices from lagging/crashing it is limited to load three attachments at the same time, after which the furthest attachment gets unloaded to clear up cache.

I don't think storing data related to the state of the attachment is performance wise approach . I am more aligned with your alternative solution, which looks simple and optimal

Another solution would be to reset zoom if navigated to keep behaviour consistent.

miljakljajic commented 1 year ago

Hey @maddylewis - travelling to EC3 so will unassign myself. Seems like you're back on today. Thank you!

maddylewis commented 1 year ago

thanks, @miljakljajic !

maddylewis commented 1 year ago

@fedirjh - when you have a chance, will you provide an update on which solution you're thinking we should go with? thx!

fedirjh commented 1 year ago

I was looking for similar issues , Just found this PR https://github.com/Expensify/App/pull/17647 which disable swiping the PDFView when it's zoomed.

One solution would be to disable navigation while an attachment is zoomed in like WhatsApp does. Another solution would be to reset zoom if navigated to keep behaviour consistent.

cc @therealsujitk, could you please provide more details regarding both solutions? Specifically, I would like to know which components will be affected by the changes and how these modifications will impact the overall performance. I have reviewed the components myself, and It seems that every solution require different implementation/approach . Could you kindly update your proposal with this information?

therealsujitk commented 1 year ago

@fedirjh I've updated my proposal with the required information.

One solution would be to disable navigation while an attachment is zoomed in like WhatsApp does.

This solution is by far the simplest, I haven't written much on it as it's already being used (explained in proposal).

fedirjh commented 1 year ago

@therealsujitk Thanks for the update, I agree with you on disabling navigation is the simplest solution and given that we implemented something similar for PDFView in #17647 I think we should follow that approach and keep it consistent and optimal.

cc @MonilBhavsar I think we should proceed with the first option which disable the carousel navigation while the attachment is zoomed.

🎀👀🎀 C+ reviewed

Edit: The solution doesn’t address a similar case for PDF scrolling.

therealsujitk commented 1 year ago

@fedirjh there's another thing that should be considered here, PDFs will have something similar to this issue for scrolling as well i.e. the PDF scroll will get reset after navigating past 3 attachments.

Considering this issue what is the approach to take?

fedirjh commented 1 year ago

PDFs will have something similar to this issue for scrolling as well i.e. the PDF scroll will get reset after navigating past 3 attachments.

I checked that and your concerns are correct, PDF scrolling is reset after passing 3 attachments similar to image zoom

Apply this fix only for zoom and ignore scrolling for now

I think we should address both cases , they have the same root cause.

Something else (ex: reset scroll on navigation, might be inconvenient for the user)

I believe that if we want to address this issue, we should implement the same fix. Considering that we cannot prevent carousel navigation when the PDF is scrolled, which results in a poor user experience, I think we should reset both the zoom and the scrolling when the user navigates to a different attachment. BUT this solution doesn’t look good to me given it's complexity and the impact it may have on performance of the carousel.

Don't fix either issue (Drop this issue completely)

This is by far the best option.

therealsujitk commented 1 year ago

The best way to fix this would be to save the state of each attachment, but you might be right about it affecting performance. I don't think storing an array of the states in the state will affect performance as it would still take O(n) space since the attachments itself are also stored in an array, HOWEVER the problem arises while updating this state, react doesn't allow updating just a single element in the array, the whole array will have to be updated resulting in O(n) time for every single state update (ideally it would be O(1)). One way would be to store the attachment states individually with unique keys ({'attachment_state_1': {}, 'attachment_state_2': {}} instead of {'attachment_states': [{}, {}]}) but that seems too messy and not right.

If someone can provide some insight on this that would be great, otherwise dropping this might be the best option.

therealsujitk commented 1 year ago

@fedirjh I just realised, we don't need to store the attachment states in the react state. Since we'll only be using it when the component mounts, we can store it as a normal variable. I've updated my proposal with this solution.

Note: I've not tested this for PDFs yet, but it should work depending on if react-pdf allows it.

melvin-bot[bot] commented 1 year ago

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

fedirjh commented 1 year ago

Thanks for the update, @therealsujitk. The new proposal makes more sense. Does that apply to PDFView as well? How will we restore the PDF scrolling position?

therealsujitk commented 1 year ago

@fedirjh their GitHub repository says it's possible to scroll to a page so I was thinking of saving the scrollHeight or something similar (will have to experiment), I can try it this weekend and let you know for sure how does that sound?

fedirjh commented 1 year ago

That sounds good. As for the implementation , can’t we use Set instead of Array ?

therealsujitk commented 1 year ago

It won't be possible to get an element easily from a Set (plus we'll need the previous state, and there can be other issues), but we can use an Object with the index as key (or a Map, not sure which is better), this would take lesser space too. Unless I've misunderstood what you're suggesting?

fedirjh commented 1 year ago

Okay, arrays sound good too. I was considering the scenario where we open the carousel and then another attachment is added to the chat. It seems that the most recently added attachment won't be displayed (pushed to the carousel) until the carousel is closed and reopened. Therefore, I believe using an array with an index would work fine. However, using a map is also a viable option.

therealsujitk commented 1 year ago

I'll test for that case too (add and delete attachment while the carousel is open), I remember seeing some issue open about the carousel getting closed when an attachment is deleted earlier today.

Anyway, I'll create the branch, if everything goes well it should be ready for you by Monday.

michaelhaxhiu commented 1 year ago

@maddylewis if you like I can take this back since I am back from OOO (in light of the new internal process for re-assigning GHs during/after OOO)

therealsujitk commented 1 year ago

@fedirjh the branch is ready! - fix-18614. It works quite well from what I've tested, one tiny issue is that the react-native-pdf package we use only has a setPage(), you can't scroll to a point so on native devices it'll scroll to the page that the user was viewing which may not be the exact scroll position but it'll be pretty close.

fedirjh commented 1 year ago

Thanks for the update @therealsujitk . I will test this out and give you feedbacks.

melvin-bot[bot] commented 1 year ago

@MonilBhavsar @fedirjh @maddylewis 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!

fedirjh commented 1 year ago

Thanks @therealsujitk , the changes looks good. Testing now.

fedirjh commented 1 year ago

I have tested the changes from https://github.com/Expensify/App/compare/main...therealsujitk:expensify-app:fix-18614 , the results looks good to me. The PDF doesn’t scroll the exact position, but it scrolls to the to the page that the user was viewing. I think we can proceed with that since the user can't navigate if the PDF is zoomed.

@MonilBhavsar The latest proposal looks good to me. I think we should proceed with it. I may also ping @kidroca to ask for his opinion since he implemented the carousel.

kidroca commented 1 year ago

Hi @fedirjh,

Thanks for looping me into this discussion. You've correctly pinpointed the issue to be with the FlatList virtualization. However, I'd like to clarify that maxToRenderPerBatch merely decides the number of images to render per unit of time (imagine rendering 3 images no more often than every 100ms). The actual cause of the problem lies in the windowSize={5} setting, which permits a maximum of 5 attachments to be mounted—two each on either side of the current page.

From what I understand, the proposed solution aims to preserve the state of the attachment, which I see could be beneficial especially for document attachments with multiple pages—allowing users to return to the last viewed page. For image attachments, however, I'm wondering about the user experience of returning to a previously zoomed-in state. Could you shed some light on any particular use-cases where this might be beneficial?

If maintaining the attachment state is indeed a key requirement, allow me to suggest an alternative that I believe is simpler. It wouldn't necessitate altering the individual attachment components to satisfy the Carousel.

The windowSize={5} setting has always been more about network performance (reducing simultaneous loading) rather than rendering performance. We certainly wouldn't want to initiate multiple attachment loads as soon as the Carousel is opened. However, progressively increasing the number as we navigate through the carousel images should pose no issues.

Why not consider maintaining a Set of unique items that have already been rendered, and then using this Set to determine the windowSize?

renderedItems = new Set();

renderItem({ item }) {
  this.renderedItems.add(item);
  return <Item ... />
}

render() {
  return <FlatList ... windowSize={Math.max(5, this.renderedItems.size)} />
}

As we render more images, the window size would gradually increase, keeping the already loaded attachments from unmounting and hence preserving their state. What do you think of this approach?

therealsujitk commented 1 year ago

Thanks for looping me into this discussion. You've correctly pinpointed the issue to be with the FlatList virtualization. However, I'd like to clarify that maxToRenderPerBatch merely decides the number of images to render per unit of time (imagine rendering 3 images no more often than every 100ms). The actual cause of the problem lies in the windowSize={5} setting, which permits a maximum of 5 attachments to be mounted—two each on either side of the current page.

Ah, I was mistaken. I said maxToRenderPerBatch based on the the number 3 :sweat_smile:.

For image attachments, however, I'm wondering about the user experience of returning to a previously zoomed-in state. Could you shed some light on any particular use-cases where this might be beneficial?

This was done mainly to keep it consistent, but there could be a case where the attachment has some text written on it (say a receipt) and the user would like it to be zoomed in while they're navigating through attachments.

Why not consider maintaining a Set of unique items that have already been rendered, and then using this Set to determine the windowSize?

This is a good idea, I never thought of that. However, I have a few concerns.

  1. Will there be a case where the FlatList component ignores the windowSize prop due to lack of system resources, I feel there's a possibility of this so the application wouldn't crash.
  2. This approach could slow done the system as more and more attachments are added especially if they are heavy. And again there may be a possibility that FlatList destroys attachments to prevent crashes.
kidroca commented 1 year ago
  1. Will there be a case where the FlatList component ignores the windowSize prop due to lack of system resources, I feel there's a possibility of this so the application wouldn't crash.

While it's a valid consideration, it's important to note that the FlatList component isn't designed to manage system resources. Instead, it will simply attempt to perform the task we've specified. If we start to notice any performance degradation, it will be our responsibility to revisit and refine our solution accordingly.

2. This approach could slow down the system as more and more attachments are added especially if they are heavy. And again there may be a possibility that FlatList destroys attachments to prevent crashes.

Indeed, an excessive number of image attachments could pose a performance challenge. However, this would only be an issue if the user scrolls past hundreds of images. We can manage this effectively by setting an upper limit.

const desiredSize = Math.max(5, this.renderedItems.size);
const windowSize = Math.min(100, desiredSize);

This boundary ensures that the window doesn't grow beyond a manageable size. While technically the issue could still occur, we have to ask: would a user realistically zoom into an attachment, scroll 100 attachments away, and then scroll back?

A potential complementary or standalone solution to the virtual window size could involve using react-freeze. This package preserves the component state while replacing off-screen items with a low-cost placeholder. It's a method we're already employing in a different part of the app. We could render items outside the immediate view area as 'frozen' with the following:

renderItem({ item, index }) {
  const diff = Math.abs(index - this.state.page);
  if (diff > 2) // return FreezedItem
  else // return RegularItem
}

This approach would ensure that off-screen items don't overly tax system resources while maintaining user experience continuity.

fedirjh commented 1 year ago

Hey @kidroca Thank you for clarifying the issue with the windowSize and for providing an alternative approach. Your suggestion of maintaining a Set of rendered items and dynamically adjusting the windowSize based on the number of rendered items is an interesting solution. The only concern is that we may have more memory consumption. I think we can limit the max windowSize to just 20 or maybe 10. It doesn’t looks realistic for a user to scroll 10 attachments away, and then scroll back and expects to get the maintained zoom state.

fedirjh commented 1 year ago

Thank you for the engaging and insightful discussion, @kidroca and @therealsujitk! 🙌🏼 . Taking into account the concerns raised, it prompts me to question whether addressing this issue is truly necessary. Upon evaluating the current behavior, it seems that the actual result is quite satisfactory. Users are experiencing a good and reasonable interaction, with the attachment state (including zoom and scroll position) being successfully preserved for the last 3 visited attachments.

I would appreciate hearing @MonilBhavsar's thoughts on this issue. Your input would be valuable in determining whether further action is required.

melvin-bot[bot] commented 1 year ago

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

MonilBhavsar commented 1 year ago

The only concern is that we may have more memory consumption. I think we can limit the max windowSize to just 20 or maybe 10. It doesn’t looks realistic for a user to scroll 10 attachments away, and then scroll back and expects to get the maintained zoom state.

I agree with this 👍

MonilBhavsar commented 1 year ago

Users are experiencing a good and reasonable interaction, with the attachment state (including zoom and scroll position) being successfully preserved for the last 3 visited attachments.

Well, I personally think it is fair to maintain zoomed state for last 3 visited attachments. cc Bug-zero team - @maddylewis @miljakljajic @michaelhaxhiu If we agree, we can :donothing: and consider this as "a feature and not a bug"

melvin-bot[bot] commented 1 year ago

@MonilBhavsar @fedirjh @maddylewis this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

maddylewis commented 1 year ago

@MonilBhavsar - i think we can consider this a feature and not a bug!