[$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut #28241

izarutskaya · 2023-09-26T09:01:29Z

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:

Open the app
Open any report
Send any attachment
Open the attachment
Reload
Click CTRL+K/CMD+K to open search and observe that URL changes but app still displays image
Close the attachment and observe that URL still shows attachment URL but app displays the report

Expected Result:

App should close the image attachment when we open search page using shortcut

Actual Result:

App does not close the image attachment when we open attachment, reload and open search page using shortcut and it breaks the further navigation

Workaround:

Unknown

Platforms:

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

Version Number: Build v1.3.74-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

attachment.preview.search.breaks.navigation.windows.chrome.mp4

Recording.1641.mp4

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695575577105459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01d3cee7fe8b187729
Upwork Job ID: 1706594842812719104
Last Price Increase: 2023-10-03
Automatic offers:

dukenv0307 | Contributor | 27004795
dhanashree-sawant | Reporter | 27004799

melvin-bot · 2023-09-26T09:01:34Z

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

melvin-bot · 2023-09-26T09:01:35Z

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

melvin-bot · 2023-09-26T09:01:37Z

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 · 2023-09-26T09:01:40Z

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

dukenv0307 · 2023-09-26T09:35:36Z

Proposal

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

App does not close the image attachment when we open attachment, reload and open search page using shortcut and it breaks the further navigation

What is the root cause of that problem?

Here, when the PopoverWithoutOverlay is rendered the first time, it always set the closeModal function to null, which overrides the closeModal function that is set by the attachment modal when the attachment modal is first rendered. So it fails to close the attachment modal when using the shortcut here.

This is not correct since this useEffect block should only trigger when isVisible change, not at the first render. This is also explained in the comment here.

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

We should make sure this line is only triggered when isVisible changes.

Store previous value of isVisible by using the usePrevious

const prevIsVisible = usePrevious(props.isVisible);

Make sure isVisible changes before triggering Modal.setCloseModal above this line, by early return if isVisible did not change.

if (prevIsVisible === props.isVisible) {
    return;
}

or that can be put here if we want to exclude the full logic of the useEffect

What alternative solutions did you explore? (Optional)

An alternative is to use isFirstRender approach to not run the useEffect if it's the first render of the PopoverWithoutOverlay component.

parasharrajat · 2023-09-26T15:19:31Z

This Modal.setCloseModal logic has been always confusing for me. Can we somehow remove it?

May be the use of a Context to trigger close to all modals not just the latest. @dukenv0307 ideas?

About the proposal:

I am curious wouldn't the useEffect itself detect changes to passed dependency? IMO, this this useEffect should only run isVisible changes. So there should be no need to manually check that.

dukenv0307 · 2023-09-27T07:45:31Z

I am curious wouldn't the useEffect itself detect changes to passed dependency? IMO, this this useEffect should only run isVisible changes. So there should be no need to manually check that.

@parasharrajat yes, but useEffect will always run on first render as well. So by default it will run:

on first render
after any dependency changes

(This is different from componentDidUpdate that we uses earlier which only runs after dependency changes, this has caused quite a number of issues since we migrated to hooks, usePrevious was added to fix those issues)

Also we have the same fix using wasVisible for the BaseModal as well here

dukenv0307 · 2023-09-27T07:56:58Z

May be the use of a Context to trigger close to all modals not just the latest. @dukenv0307 ideas?

@parasharrajat Yes we can maybe use a Context to pass a shouldForceClose boolean to all Modals and if we want to close them we just set shouldForceClose to true. But we only have 1 modal opens at a time so I think closing all modals are the same as closing the latest, so maybe the isVisible check if sufficient for this issue.

LiaqatSaeed · 2023-09-27T11:00:51Z

@dukenv0307 what if we close this modal on Pressing "Ctrl +K/cmd+K". We put a check if Modal is visible Close it before opening Search. thoughts?

dukenv0307 · 2023-09-28T04:44:50Z

@dukenv0307 what if we close this modal on Pressing "Ctrl +K/cmd+K". We put a check if Modal is visible Close it before opening Search. thoughts?

@LiaqatSaeed we're already trying to do that here. The problem is that currently doesn't work, which is being addressed in my proposal.

parasharrajat · 2023-09-28T12:48:11Z

Ok, I see. Now, it is clear to me. Thanks for explaining.

@dukenv0307's proposal looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2023-09-28T12:48:18Z

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

parasharrajat · 2023-09-28T12:49:22Z

@dukenv0307 Can you please create the PR today? I will be OOO from 29 Sept to 4 Oct so I will try to get it done by tomorrow.

SergeyHTML · 2023-09-28T15:26:45Z

Proposal

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

Navigation breaks on opening attachment, reload and opening search by shortcut

What is the root cause of that problem?

Here when the PopoverWithoutOverlay is rendered the first time, it always sets the closeModal function to null, which overrides the closeModal function that is set by the attachment modal when the attachment modal is first rendered. So it fails to close the attachment modal when using the shortcut here.

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

We can check the Modal is open and will not override the method closeModal.

We can export closeModal from the Modal and in the component PopoverWithoutOverlay add the condition.

if (!Modal.closeModal) {
            Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);
        }

What alternative solutions did you explore? (Optional)

N/A

kaushiktd · 2023-09-29T12:08:42Z

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

when users open an image attachment within the application, reload the application, and then attempt to open the search page using a keyboard shortcut, the application fails to close the previously opened image attachment. This behavior disrupts further navigation within the application.

What is the root cause of that problem?

The root cause of this problem appears to be related to how the application manages the state of opened attachments and how it handles navigation. Specifically, when the attachment is initially rendered, it doesn't close properly when switching to the search page using a keyboard shortcut.

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

file path: https://github.com/Expensify/App/blob/main/src/components/PopoverWithoutOverlay/index.js

Create a Ref for isVisible: Instead of relying on the previous value of isVisible stored in a state variable, we can use a ref to keep track of the previous value. Refs persist across renders and can be helpful for comparing previous and current values without causing re-renders.

const prevIsVisibleRef = React.useRef(props.isVisible);

// Update the ref when isVisible changes
React.useEffect(() => {
  prevIsVisibleRef.current = props.isVisible;
}, [props.isVisible]);

Modify the useEffect: In the useEffect block, you can now directly compare the current isVisible prop with the value stored in the ref to determine if it has changed.
React.useEffect(() => {
  // Check if isVisible has changed
  if (prevIsVisibleRef.current !== props.isVisible) {
    if (props.isVisible) {
      props.onModalShow();
      onOpen({
        ref,
        close: props.onClose,
        anchorRef: props.anchorRef,
      });
    } else {
      props.onModalHide();
      close(props.anchorRef);
      Modal.onModalDidClose();
    }
    Modal.willAlertModalBecomeVisible(props.isVisible);
    Modal.setCloseModal(props.isVisible ? () => props.onClose(props.anchorRef) : null);
  }
}, [props.isVisible]);

By using this approach, you maintain a reference to the previous value of isVisible without causing additional re-renders. This can help ensure that the Modal.setCloseModal logic is only executed when there is a change in the isVisible prop.

Video:-

https://drive.google.com/file/d/1j6K3FmCYpXD-zXvj5smSgu2RhRZuK5YR/view?usp=sharing

dukenv0307 · 2023-09-29T13:50:01Z

@deetergp Can you please assign me? So I can raise a PR to @parasharrajat review before he is OOO.

parasharrajat · 2023-09-29T15:08:30Z

It's too late. I will be OOO in about 2 hours. So it's fine, we can take time to assign this. I would say let's start this on Monday so we don't start the countdown while I am away.

kaushiktd · 2023-09-29T15:15:48Z

@parasharrajat, I am wondering if you checked my proposal earlier today! Would love your feedback. Feedback will help me either way! 🙂

NicMendonca · 2023-10-02T20:19:41Z

bump @deetergp - #28241 (comment)

melvin-bot · 2023-10-03T16:00:12Z

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

parasharrajat · 2023-10-17T14:56:07Z

The final revert PR is here #29784. After this is merged let's retest existence your issue.

NicMendonca · 2023-10-18T19:38:02Z

@parasharrajat are we holding payment here? 👀

parasharrajat · 2023-10-18T19:44:01Z

Yes @NicMendonca

Next steps:

The final revert is merged so I will see if Fix report action context menu stuck when open search page with keyboard shortcut #29152 can solve this issue. If so, we don't need the next step.
Find a new solution without bugs cc: @dukenv0307

parasharrajat · 2023-10-20T06:55:40Z

OK, I checked #29152 solves this issue as well. So no other action is left here. Good work @tsa321.

the final payment date for this issue will depend on #27244.

melvin-bot · 2023-10-23T12:27:48Z

@deetergp, @parasharrajat, @NicMendonca, @dukenv0307 Whoops! This issue is 2 days overdue. Let's get this updated quick!

deetergp · 2023-10-23T17:17:11Z

I believe we are still waiting on the resolution of #27244?

melvin-bot · 2023-10-24T12:52:04Z

@deetergp @parasharrajat @NicMendonca @dukenv0307 this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot · 2023-10-24T12:52:11Z

Current assignee @parasharrajat is eligible for the Internal assigner, not assigning anyone new.

deetergp · 2023-10-25T07:04:50Z

Looks like #27244 has been merged and is in the regression period. We should know by Tuesday.

melvin-bot · 2023-10-30T12:52:13Z

@deetergp, @parasharrajat, @NicMendonca, @dukenv0307 Eep! 4 days overdue now. Issues have feelings too...

NicMendonca · 2023-10-31T21:50:25Z

@deetergp @parasharrajat what's the next steps for this one? Are we paying out now?

deetergp · 2023-11-01T00:57:43Z

Would someone be willing to retest this to see if #27244 resolved our issue?

parasharrajat · 2023-11-01T06:03:12Z

I already did. We are just waiting for payments here for the work done on this issue.

NicMendonca · 2023-11-02T19:16:09Z

Oh sorry, the hold for payment automation didn't click in

NicMendonca · 2023-11-02T19:20:30Z

Reporter: @dhanashree-sawant - $50
Contributor: @dukenv0307 - $500
C+: @parasharrajat $500 - paid via Expensify

NicMendonca · 2023-11-02T19:21:55Z

@dhanashree-sawant, @dukenv0307 you've been paid!

@parasharrajat don't forget to request payment via Expensify!

🎉

parasharrajat · 2024-04-05T18:05:00Z

Payment requested as per #28241 (comment)

JmillsExpensify · 2024-04-08T03:02:02Z

$500 approved for @parasharrajat

izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 26, 2023

melvin-bot bot changed the title ~~Web - Navigation breaks on opening attachment, reload and opening search by shortcut~~ [$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut Sep 26, 2023

melvin-bot bot assigned NicMendonca Sep 26, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 26, 2023

melvin-bot bot assigned parasharrajat Sep 26, 2023

melvin-bot bot assigned deetergp Sep 28, 2023

melvin-bot bot added the Overdue label Oct 2, 2023

melvin-bot bot removed the Overdue label Oct 2, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 17, 2023

tsa321 mentioned this issue Oct 17, 2023

Revert "Fix navigation breaks on opening search page after reloading the attachment" #29784

Merged

59 tasks

NicMendonca changed the title ~~[HOLD for payment 2023-10-18] [$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut~~ [$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut Oct 18, 2023

melvin-bot bot added the Overdue label Oct 23, 2023

melvin-bot bot removed the Overdue label Oct 23, 2023

melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor labels Oct 24, 2023

melvin-bot bot added the Overdue label Oct 27, 2023

melvin-bot bot removed the Overdue label Oct 31, 2023

NicMendonca closed this as completed Nov 2, 2023

aimane-chnaif mentioned this issue Dec 7, 2023

[HOLD for payment 2024-02-07] [$500] Search Page opens in Background when delete Reciept Confirm Modal is visible #29511

Closed

6 tasks

[$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut #28241

[$500] Web - Navigation breaks on opening attachment, reload and opening search by shortcut #28241

Comments

izarutskaya commented Sep 26, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 26, 2023

melvin-bot bot commented Sep 26, 2023

melvin-bot bot commented Sep 26, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 26, 2023

dukenv0307 commented Sep 26, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

parasharrajat commented Sep 26, 2023 • edited Loading

dukenv0307 commented Sep 27, 2023 • edited Loading

dukenv0307 commented Sep 27, 2023

LiaqatSaeed commented Sep 27, 2023

dukenv0307 commented Sep 28, 2023

parasharrajat commented Sep 28, 2023 • edited Loading

melvin-bot bot commented Sep 28, 2023

parasharrajat commented Sep 28, 2023

SergeyHTML commented Sep 28, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

kaushiktd commented Sep 29, 2023

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

What is the root cause of that problem?

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

Video:-

dukenv0307 commented Sep 29, 2023

parasharrajat commented Sep 29, 2023 • edited Loading

kaushiktd commented Sep 29, 2023

NicMendonca commented Oct 2, 2023

melvin-bot bot commented Oct 3, 2023

parasharrajat commented Oct 17, 2023 • edited Loading

NicMendonca commented Oct 18, 2023

parasharrajat commented Oct 18, 2023 • edited Loading

parasharrajat commented Oct 20, 2023

melvin-bot bot commented Oct 23, 2023

deetergp commented Oct 23, 2023

melvin-bot bot commented Oct 24, 2023

melvin-bot bot commented Oct 24, 2023

deetergp commented Oct 25, 2023

melvin-bot bot commented Oct 30, 2023

NicMendonca commented Oct 31, 2023

deetergp commented Nov 1, 2023

parasharrajat commented Nov 1, 2023 • edited Loading

NicMendonca commented Nov 2, 2023

NicMendonca commented Nov 2, 2023

NicMendonca commented Nov 2, 2023

parasharrajat commented Apr 5, 2024

JmillsExpensify commented Apr 8, 2024

izarutskaya commented Sep 26, 2023 •

edited by melvin-bot bot

Loading

dukenv0307 commented Sep 26, 2023 •

edited

Loading

parasharrajat commented Sep 26, 2023 •

edited

Loading

dukenv0307 commented Sep 27, 2023 •

edited

Loading

parasharrajat commented Sep 28, 2023 •

edited

Loading

parasharrajat commented Sep 29, 2023 •

edited

Loading

parasharrajat commented Oct 17, 2023 •

edited

Loading

parasharrajat commented Oct 18, 2023 •

edited

Loading

parasharrajat commented Nov 1, 2023 •

edited

Loading