[$250] Profile - Zoom slider does not change position when moving to zoom an image

[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: 9.0.6-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): gatantm+528@gmail.com Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/44788

Action Performed:

1. Navigate to account Settings > Profile
2. Click on the avatar
3. Click on upload photo and upload any JPEG image
4. Hover over the zoom slider and move to the right

Expected Result:

The zoom slider is moving to show how an image is zoomed

Actual Result:

The zoom slider stays on the same position when moving it to zoom uploaded image

Workaround:

Unknown

Platforms:

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

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/fd7a8c59-f557-4379-882d-2f15fd7edbd0

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0130ac939be52a2a80
• Upwork Job ID: 1811335809081764317
• Last Price Increase: 2024-09-05

[melvin-bot[bot]]

Triggered auto assignment to @Julesssss (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@Julesssss 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

[lanitochka17]

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

[Julesssss]

Cute cat.

This needs to be fixed, but I'm demoting it from being a blocker.

[Julesssss]

My best guess so far is that this PR introduced the bug

[ishpaul777]

I can reproduce the issue even after reverting https://github.com/Expensify/App/pull/44788

[Julesssss]

Thanks for looking

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~0130ac939be52a2a80

[melvin-bot[bot]]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

📣 @harshit078! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[QichenZhu]

Proposal

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

The zoom slider doesn't work.

What is the root cause of that problem?

In versions below 2.16.0, react-native-gesture-handler registers pointermove listeners but never unregisters them. This causes issues in development mode since StrictMode renders components twice. This has been fixed by https://github.com/software-mansion/react-native-gesture-handler/pull/2815.

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

1. Wait until #45162 is resolved. Currently I'm using the work-in-progress solution from #45314.
2. Upgrade react-native-gesture-handler to version 2.16.0 or newer.

What alternative solutions did you explore? (Optional)

N/A

[hungvu193]

I'll take a look this weekend

[Ahmed-Abdella]

Please check my Proposal here, it fixes the Crash when trying to upload the Avatar. I believe it fixes this issue also.

[hungvu193]

Please check my Proposal here, it fixes the Crash when trying to upload the Avatar. I believe it fixes this issue also.

Your proposal did fix the crashes but didn't fix this issue, also in your RCA's proposal, you mentioned the code that was added 14 months ago, I don't think that's correct RCA.

[hungvu193]

@QichenZhu Your proposal looks promising but I'm facing a crash while testing it on mobile. Did you test it with mobile platform?

[QichenZhu]

@hungvu193 Yes, I came across the same problem on mobile, so I suggested holding for #45162 for now.

[hungvu193]

@hungvu193 Yes, I came across the same problem on mobile, so I suggested holding for #45162 for now.

I actually applied your proposal from that issue along with your mentioned PR + your suggestion from this issue, but it still crashed

[QichenZhu]

@hungvu193 Thanks for testing and pointing out the problem. I'll revisit #45162 in a few hours.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[alexpensify]

I think that this notice is an error, but correct me if I'm wrong @hungvu193. Thanks!

[hungvu193]

I think that this notice is an error, but correct me if I'm wrong @hungvu193. Thanks!

Yeah. This issue has similar root cause with a DB, so a contributor mentioned in DB that his proposal will fix this issue too (but it doesn't as I explained above in my comment)that's why we saw this alert. We can ignore that 😄

[QichenZhu]

@hungvu193 The fix for #45162 has been merged. Could you retest on the latest main?

[hungvu193]

@hungvu193 The fix for #45162 has been merged. Could you retest on the latest main?

Sure. I'll retest it tomorrow in my time.

[QichenZhu]

@hungvu193 I am withdrawing my proposal for two reasons:

1. react-native-gesture-handler 2.16.0, despite its README claiming to support React Native 0.68.0+, does not support React Native 0.73.4 as mentioned in this issue:

Our current policy regarding the new architecture support is to support only the most recent version, which at the time of writing is 0.74. The crash you're describing is caused by a breaking change between RN 0.73 and 0.74.

2. The PR https://github.com/Expensify/App/pull/40548 already includes the react-native-gesture-handler 2.16.0 upgrade I suggested.

[hungvu193]

@QichenZhu Thanks for your update!

Still waiting for proposals.

[hungvu193]

I re-test it today, I think this issue is fixed.

https://github.com/user-attachments/assets/8276fb23-f321-4b0b-9289-10dfe8c03a52

[kavimuru]

Bug is fixed

https://github.com/user-attachments/assets/25783ca4-96f8-4d19-829a-39430bfb67fd

[Ahmed-Abdella]

Why I am still able to reproduce it on the latest main v9.0.8-2

https://github.com/user-attachments/assets/003acbf5-e1da-459d-a858-90be8717c0d1

[QichenZhu]

@hungvu193 @kavimuru @Ahmed-Abdella The bug is reproducible in development mode, but not in production mode.

Development mode

https://github.com/user-attachments/assets/53c96227-6f4e-4d3f-8ca8-01f92f89138d

Production mode

https://github.com/user-attachments/assets/52fc381e-d371-43ad-b29a-a32195c360ac

[hungvu193]

Oops I think I disabled StrictMode during test this issue

[Ahmed-Abdella]

So this is still need to be fixed?

[Ahmed-Abdella]

Proposal

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

The zoom slider doesn't work.

What is the root cause of that problem?

In the slider component

https://github.com/Expensify/App/blob/7f66ea3632c787bce4ab8b6dc12c252c1ddc7685/src/components/AvatarCropModal/Slider.tsx#L41-L53

The minDistance(5) make sure that the gesture doesn't get active before travelling a distance of 5

minDistance(value: number) Minimum distance the finger (or multiple finger) need to travel before the gesture activates. Expressed in points.

So we lose the control of gesture handler before the activation, and now we can't active the gesture anymore and the onChange method (event) that's responsible to update the sllider will never run.

Also the tooltipIsVisible state change to true in the onFinalize method before activating the gesture that make the tooltip visible again which prevent us to control the gesture handler.

So we need to active the gesture before reaching the onFinalize method that set the tooltipIsVisible to true

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

We should call the minDistance() method with 0 instead of 5

Gesture.Pan()
        .minDistance(0)

By changing the value to 0 we ensure the gesture active immediately

Result

https://github.com/user-attachments/assets/6f1c64b1-386b-47b6-971c-b4a1356bb922

What alternative solutions did you explore? (Optional)

N/A

[Ahmed-Abdella]

Proposal updated for more explanation.

[hungvu193]

Thanks for your update. I'll review it in my morning.

[hungvu193]

So we lose the control of gesture handler before the activation, and now we can't active the gesture anymore and the onChange method (event) that's responsible to update the sllider will never run.

Hey can you give me more detailed about this one right here?

Also please notice that this issue is now only reproduced when StrictMode is enabled.

[Ahmed-Abdella]

Hey can you give me more detailed about this one right here?

Maybe I complicated things a little bit, all I mean is that the onChange callback in Gesture.pan() never got invoked because we delayd the activation for a distance of 5 by using minDistance(5).

the onBegin callback got invoke, and immediately after start moving the onFinalize got invoked, means that the gesture ended. And the onChange callback is skipped. So we need to active the gesture immediately to trigger the onChange callback and apply the updates inside it.

Also please notice that this issue is now only reproduced when StrictMode is enabled.

Yes, Maybe because the fact that the StrictMode double invoke the component to identify the side effects and also enforce synchronous state updates that cause immediate rerender.

Maybe this is causing problems in how the state got managed and updated in the gesture handler after Delaying the activation using minDistance, which make the onChange not being invoked and the onFinalize called before it's time.

Setting the minDistance to 0 ensure that the gesture activate immediatey and the onChange that update the slider got invoked.

[hungvu193]

I've been under the water for past few days, I'll take another look today

[hungvu193]

Reviewing shortly

[hungvu193]

Maybe I complicated things a little bit, all I mean is that the onChange callback in Gesture.pan() never got invoked because we delayd the activation for a distance of 5 by using minDistance(5).

I don't think this is correct and setting minDistance to 0 feels like a workaround to me.

Upgrade react-native-gesture-handler to 2.16.0 will fix this issue, however we already had a PR to do that: https://github.com/Expensify/App/pull/40548

@alexpensify Please put this PR on hold for https://github.com/Expensify/App/pull/40548, this bug's only reproduced on StrictMode, so I think that's fine to hold it.

[alexpensify]

Thanks for the update! I've updated this GH.

[alexpensify]

Weekly Update: On hold for https://github.com/Expensify/App/pull/40548

[alexpensify]

Weekly Update: hold for https://github.com/Expensify/App/pull/40548

[alexpensify]

Weekly Update: https://github.com/Expensify/App/pull/40548

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @alexpensify, @Julesssss, @hungvu193 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!

[alexpensify]

Weekly update: The other GH we have been tracking states - Closing this in favor of the 0.75 upgrade

I need to figure out what we should do here. @roryabraham any feedback? Thanks!

[roryabraham]

Should HOLD on https://github.com/Expensify/App/issues/37374 instead

[alexpensify]

Weekly Update: Thanks, Rory! Still on hold for https://github.com/Expensify/App/issues/37374

[roryabraham]

We are on RN 0.75 now, so I think this can come off hold. That issue isn't closed yet but is in the cleanup phase.