[HOLD for payment 2023-12-06] [$1000] Web - Pressing ESC on attachment preview glitches the preview

[kbecciv]

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. Open the app
2. Click on any chat
3. Click '+' add attachment
4. Attach multiple images
5. Open first uploaded image
6. Click on ESC multiple times, observe that preview closes, reopens with different image and closes

Expected Result:

App should close preview on ESC click (note you need to press Esc multiple times)

Actual Result:

App glitches on closing preview on ESC click

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.61-1
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

Recording.4284.mp4

preview.glitches.on.ESC.close.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ebd6c095f0ad277c
• Upwork Job ID: 1701562714140925952
• Last Price Increase: 2023-10-03

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ebd6c095f0ad277c

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

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

[melvin-bot]

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

[zukilover]

Proposal

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

App glitches on closing preview on ESC click

What is the root cause of that problem?

AttachmentModal is using unnecessary useCallback for closeModal function. This causes React to not throw away the cached function.

App/src/components/AttachmentModal.js

Lines 307 to 309 in 5f668c6

	const closeModal = useCallback(() => {
	setIsModalOpen(false);
	}, []);

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

Remove the useCallback wrapper for closeModal, e.g.:

    const closeModal = () => {
        setIsModalOpen(false);
    };

This is the safest and optimal way since the modal visibility is controlled by a local state.

What alternative solutions did you explore? (Optional)

N/A

[ishpaul777]

While my solution is solving the issue I think the root cause is correctly identified by @zukilover, the solution proposed by me is solving the issue of Navigation issue when CMD+K or CMD+SHIFT+K is used for navigation (reported by a contributor on slack already) I am marking my off-topic (unless we decide to solve this issue along with this).

[Christinadobrzyn]

Not sure why @adelekennedy and I are both assigned but I did the testing and can reproduce this only in Chrome. Looks like External is already added for some proposals.

Ah this is why we were both added - https://expensify.slack.com/archives/C01SKUP7QR0/p1694216537643229

[astrohunter62]

Proposal

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

Pressing or double pressing ESC on the attachment preview glitches the preview

What is the root cause of that problem?

This issue is caused by the onViewableItemsChanged props.
The onViewableItemsChanged is still being invoked even after navigating to a different screen in the React Native FlatList.

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

We can add and change the focused boolean value with navigation eventListeners 'blur' and 'focus'.

For example:
src/components/Attachments/AttachmentCarousel/index.js

const [focused, setFocused] = useState();
const navigation = useNavigation();
useEffect(() => {
    const subscribeFocusEvent = navigation.addListener('focus', () => {
        setFocused(true);
    });
    const subscribeBlurEvent = navigation.addListener('blur', () => {
        setFocused(false);
    });
    return () => {
        subscribeFocusEvent();
        subscribeBlurEvent();
    };
}, [focused, navigation]);    
  .....
{containerWidth > 0 && focused && (
  <FlatList
      ....

What alternative solutions did you explore? (Optional)

const renderItem = useCallback(
    ({item}) =>
        focused ? (
            <CarouselItem
                item={item}
                isFocused={activeSource === item.source}
                onPress={canUseTouchScreen ? () => setShouldShowArrows(!shouldShowArrows) : undefined}
            />
        ) : (
            <></>
        ),
    [activeSource, focused, canUseTouchScreen, setShouldShowArrows, shouldShowArrows],
);

• Result

Safari.webm

Chrome.webm

[melvin-bot]

📣 @astrohunter62! 📣
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. 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.
2. 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.
3. 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>

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[Christinadobrzyn]

I think we're reviewing proposals!

[melvin-bot]

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

[Christinadobrzyn]

hi @abdulrahuman5196! Can you let me know what you think of the current proposals and if you'd like to see more?

[Christinadobrzyn]

Hey @abdulrahuman5196 would you like to see more proposals?

[Christinadobrzyn]

This PR is in review so moving with GH to weekly.

[paultsimura]

@Christinadobrzyn the PR is already approved, merged, and deployed to staging 🤔

[Christinadobrzyn]

Sorry that was my lazy way of writing, "we're waiting for the PR to go into production so this can be paid".

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

• fix: Attachments reopen on multiple 'esc' click and hang on browser's navigation back #31524

If no regressions arise, payment will be issued on 2023-12-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
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @paultsimura requires payment
• @abdulrahuman5196 requires payment
• @dhanashree-sawant requires payment

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

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

[Christinadobrzyn]

prepping for payment

Payouts due:

Issue Reporter: $50 @dhanashree-sawant (PAID in Upwork)
Contributor: $750 @paultsimura (PAID in Upwork) & $250 @tsa321 (PAID in Upwork)
Contributor+: $1000 @abdulrahuman5196 (PAID in Upwork)

Eligible for 50% #urgency bonus? N

Upwork job is here.

@abdulrahuman5196 do we need a regression for this?

[paultsimura]

Thank you @Christinadobrzyn
Based on this comment: #27237 (comment), could you please split my bounty into $750 for me, and $250 for @tsa321?

[abdulrahuman5196]

The PR that introduced the bug has been identified. Link to the PR:
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:
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:

Not a regression.

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

Yes.

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. Attach multiple images in a chat
2. Open the first uploaded image
3. Click on ESC multiple times
4. Verify that the attachment modal doesn't reopen
5. Open an attachment again
6. Click the "Back" button in the browser
7. Verify that the composer gets focused
8. Open an attachment again
9. Click the "X" button on the modal
10. Verify that the composer gets focused

@Christinadobrzyn

[Christinadobrzyn]

Awesome! Thanks @abdulrahuman5196!

I paid you and @paultsimura in Upwork based on this payment structure #27237 (comment)

@tsa321 and @dhanashree-sawant can you accept the Upwork offer and I'll pay you?

[dhanashree-sawant]

Hi @Christinadobrzyn, sorry for the delay, the job was actually raised after 30 August but you have sent me 250$ offer, can you rectify it once to 50$?

[tsa321]

@Christinadobrzyn I have accepted the offers, thank you...

[Christinadobrzyn]

Thanks @tsa321! I paid you in Upwork based on this payment summary.

Thanks for catching that @dhanashree-sawant - I got my dates mixed up - I sent you a modified offer, would you mind accepting so I can pay you?

[dhanashree-sawant]

That's fine, I have accepted the offer.

[Christinadobrzyn]

Thanks @dhanashree-sawant! I paid you in Upwork based on our payment summary.

This is good to close - thanks everyone!