[HOLD for payment 2023-09-06] [$500] Attachments - Navigation arrows are not shown on PDF until navigated to 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!

---

Action Performed:

1. Open the app
2. Select a chat that has attached images and pdf
3. Open a PDF

Expected Result:

The navigation arrows are displayed

Actual Result:

The navigation arrows are not shown until I swipe to an image

Workaround:

Unknown

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

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

Bug6160126_Video_2023-08-10_10-17-51-1.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01373e4d2b9f6c1a7d
• Upwork Job ID: 1689735843772346368
• Last Price Increase: 2023-09-12
• Automatic offers:
• tienifr | Contributor | 26199945

[melvin-bot]

Triggered auto assignment to @puneetlath (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

[akinwale]

Proposal

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

Navigation arrows are not shown for a PDF until the carousel is swiped to an image.

What is the root cause of that problem?

The default value for isPinchGestureRunning is set to true in AttachmentCarousel/index.native.js. This combined with the value set for the shouldShowArrows prop on the CarouselButtons component results in the arrows not showing when a PDF is initially loaded by the carousel.

App/src/components/Attachments/AttachmentCarousel/index.native.js

Line 27 in b8119e9

const [isPinchGestureRunning, setIsPinchGestureRunning] = useState(true);

App/src/components/Attachments/AttachmentCarousel/index.native.js

Line 94 in b8119e9

shouldShowArrows={shouldShowArrows && !isPinchGestureRunning}

To provide further context, when an image is initially loaded in the carousel, the onPinchGestureChange event is called for the AttachmentCarouselPager component on the page, with the parameter set to false, which ends up setting the isPinchGestureRunning state value to false. This event handler does not get invoked if a PDF file is initially loaded by the pager.

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

Set the default value for the isPinchGestureRunning state variable to false.

 const [isPinchGestureRunning, setIsPinchGestureRunning] = useState(false);

This is a valid approach, as I cannot think of a reason to assume that there is an active pinch gesture running by default (taking into account the current default state) when the attachment carousel is initially loaded.

What alternative solutions did you explore? (Optional)

None.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01373e4d2b9f6c1a7d

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

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

The navigation arrows are not shown until I swipe to an image

What is the root cause of that problem?

In here, the isPinchGestureRunning default value is true, which leads to the arrows not showing as in here. If image is the first attachment in the carousel, isPinchGestureRunning will be set to false here.

However, for PDF, we're not calling the onPinchGestureChange at all, so the onPinchGestureChange remains as true and the arrows never show.

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

We need to make the onPinchGestureChange work properly for PDF

1. In here, we'll call attachmentCarouselPagerContext.onPinchGestureChange(scale !== 1);. This is similar to the logic to trigger onPinch for Image here (it checks the zoom property) and will make sure if we zoom/pinch the PDF, the arrows will also hide, similar to for Image.
2. We can set the default value of isPinchGestureRunning to false here since it makes sense for it to be false at the start

   App/src/components/Attachments/AttachmentCarousel/index.native.js

   Line 27 in ed2c287

   const [isPinchGestureRunning, setIsPinchGestureRunning] = useState(true);

   
   If there's any reason we don't want that, we can call onPinchGestureChange(false) in useEffect when rendering the AttachmentViewPdf because initially the Pdf will have scale 1.

What alternative solutions did you explore? (Optional)

NA

[puneetlath]

@rushatgabhane thoughts on the proposal?

[melvin-bot]

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

[rushatgabhane]

@tienifr does pdf zooming using pinch even work on native?

[tienifr]

@tienifr does pdf zooming using pinch even work on native?

@rushatgabhane yes, it works fine on native

[rushatgabhane]

cool, let's go with @tienifr's proposal #24375 (comment) then

C+ reviewed 🎀 👀 🎀

[melvin-bot]

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

[melvin-bot]

📣 @rushatgabhane Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.58-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix: 24375 Navigation arrows are not shown on PDF until navigated to an image #25529

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

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter n/a
• Contributor that fixed the issue. @tienifr
• Contributor+ that helped on the issue and/or PR @rushatgabhane

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

• @rushatgabhane does not require payment (Eligible for Manual Requests)
• @tienifr requires payment offer (Contributor)

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

[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:

• [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
• [@rushatgabhane] 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:
• [@rushatgabhane] 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:
• [@rushatgabhane] Determine if we should create a regression test for this bug.
• [@rushatgabhane] 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.
• [@puneetlath] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[puneetlath]

@rushatgabhane friendly reminder about the checklist so that we can pay next week. Thanks!

[puneetlath]

@rushatgabhane bump on the checklist.

[melvin-bot]

@puneetlath, @johnmlee101, @rushatgabhane, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@puneetlath, @johnmlee101, @rushatgabhane, @tienifr Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

@rushatgabhane bump again on the checklist so that we can pay this out. Thanks!

[rushatgabhane]

1. The PR that introduced the bug has been identified. Link to the PR: Fix: Find solution to low resolution large images on Android, and the Canvas crash #20798

2. 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: https://github.com/Expensify/App/pull/20798/files#r1322579339

3. 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: N.A.

4. Determine if we should create a regression test for this bug. Yes

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

   1. Login to new dot
   2. Select a chat that has PDFs (or send new ones if they don't exist)
   3. Open a PDF
   4. Verify that the navigation arrows are displayed

[rushatgabhane]

Created a manual request for $500 here - https://staging.new.expensify.com/r/4923783766023066

We had a regression - #26193 (comment)

[melvin-bot]

Upwork job price has been updated to $500

[puneetlath]

Payment summary:

• Contributor @tienifr paid $500 via Upwork
• C+ @rushatgabhane due $500 via NewDot

[puneetlath]

Issue to add regression test: https://github.com/Expensify/Expensify/issues/316113

[puneetlath]

Dropping to weekly. Just waiting on @JmillsExpensify for payment.

[rushatgabhane]

@puneetlath we can close this. The payment will be tracked in new dot

[puneetlath]

I believe the process if or @JmillsExpensify to close the issue when he pays. Is that right @JmillsExpensify or can I go ahead and close it once they've made the request?

[JmillsExpensify]

Feel free to close this issue. I don't need it open for tracking purposes.

[JmillsExpensify]

$500 payment approved for @rushatgabhane based on BZ summary.