[$500] User can unpin a report and mark it as unread/read simultaneously.

[kavimuru]

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. Right click on any report in LHS
2. Unpin/pin & Mark as read/unread quickly

Expected Result:

User should be able to select one at a time.

Actual Result:

User is able to select both at a time.

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?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.65-0
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

bug_demo.mp4

web.mov

Expensify/Expensify Issue URL:
Issue reported by: @Krishna2323
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1692160684023549

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c0f25974eee0955c
• Upwork Job ID: 1699841676339986432
• Last Price Increase: 2023-09-28

[melvin-bot]

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

User can unpin a report and mark it as unread/read simultaneously.

What is the root cause of that problem?

The shouldDelay parameter passed to the closePopover is set to true, which results in a delay when closing the popover.

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 255 in 1170d0e

hideContextMenu(true, ReportActionComposeFocusManager.focus);

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

Solution 1
Set the shouldDelay parameter to false for the closePopover method. This should also be done for the "Mark as read" and "Pin" / "Unpin" menu items.

However, it looks like the delay may be required when the context menu is displayed elsewhere.

1. So we should add a new parameter for the onPress handler of the menu items in the ContextMenuActions file for the corresponding menu items that need to be displayed in the LHN. This parameter can be called shouldDelayClose.

2. Add an optional parameter to the ReportActionContextMenu#showContextMenu method called shouldDelayClose with its default value set to true to maintain backward compatibility. When calling this method in OptionRowLHN, set the value for this parameter to false, and then pass the value to onPress event handler for each of the corresponding items.

App/src/components/LHNOptionsList/OptionRowLHN.js

Line 118 in d60c13e

ReportActionContextMenu.showContextMenu(

3. Add the shouldDelayClose prop to BaseReportActionContextMenu and the corresponding prop type. Pass the prop as a member of the payload object when rendering the context menu actions.

shouldDelayClose: props.shouldDelayClose,

4. Update PopoverReportActionContextMenu with shouldDelayClose set in the state. Add the parameter to the showContextMenu method with a default value set to true. Pass the state value as a prop to BaseReportActionContextMenu.

Solution 2
Use a boolean ref to track if a menu item has been clicked in BaseReportActionContextMenu and handle accordingly.

Why not a useState hook here? It is possible for a user to click faster than React updates the state, so two actions will still end up being registered in that scenario.

1. Add the itemPressed ref.

const itemPressed = useRef(false);

2. Update the onPress prop for the ContextMenuItem.

onPress={() => interceptAnonymousUser(() => {
    if (itemPressed.current) {
        return;
    }
    if (closePopup) {
        // only one menu item should register for a press if the popup should close afterwards
        itemPressed.current = true;
    }
    contextAction.onPress(closePopup, payload);
}, contextAction.isAnonymousAction)}

Solution 3 (not working)
Use the useSingleExecution hook to prevent multiple taps/clicks from registering meaning only one click will be executed at a time. This hook will be available once this PR is merged into main. Explanation.

To achieve this, the following changes will be made in BaseReportActionContextMenu.

1. Add the useSingleExecution hook.

const {singleExecution} = useSingleExecution();

2. Update the onPress prop for ContextMenuItem.

-onPress={() => interceptAnonymousUser(() => contextAction.onPress(closePopup, payload), contextAction.isAnonymousAction)}
+onPress={singleExecution(() => interceptAnonymousUser(() => contextAction.onPress(closePopup, payload), contextAction.isAnonymousAction))}

After testing this approach, it does not work as intended. While it works on native platforms, there seems to be no latency on web and the internal isExecuting state of the hook returns false before the next user click is registered.

What alternative solutions did you explore? (Optional)

None.

[DylanDylann]

Proposal

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

User can unpin a report and mark it as unread/read simultaneously.

What is the root cause of that problem?

• Currently, when clicking on the "Mark as unread" (or "Mark as read") button in the context menu, it will delay for 800ms before hiding the context menu, like below:
  

  App/src/pages/home/report/ContextMenu/ContextMenuActions.js

  Line 254 in f68755c

  hideContextMenu(true, ReportActionComposeFocusManager.focus);

• The hideContextMenu with the first params called "shouldDelay":
  

  App/src/pages/home/report/ContextMenu/ReportActionContextMenu.js

  Lines 11 to 32 in f68755c

  	function hideContextMenu(shouldDelay, onHideCallback = () => {}) {
  	if (!contextMenuRef.current) {
  	return;
  	}
  	if (!shouldDelay) {
  	contextMenuRef.current.hideContextMenu(onHideCallback);
  	return;
  	}
  	// Save the active instanceID for which hide action was called.
  	// If menu is being closed with a delay, check that whether the same instance exists or a new was created.
  	// If instance is not same, cancel the hide action
  	const instanceID = contextMenuRef.current.instanceID;
  	setTimeout(() => {
  	if (contextMenuRef.current.instanceID !== instanceID) {
  	return;
  	}
  	contextMenuRef.current.hideContextMenu(onHideCallback);
  	}, 800);
  	}

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

• We clicking on "Mark as unread" button, we can call the hideContextMenu with shouldDelay param is false like:

   hideContextMenu(false, ReportActionComposeFocusManager.focus);

What alternative solutions did you explore? (Optional)

• We can just disable the others button after clicking on "Mark as unread" button to make sure that others' logic is not broken by creating a state disableClickOnContextMenuItem in PopoverReportActionContextMenu that used to decide if we should disable context button item or not
• The hideContextMenu function will become:

function hideContextMenu(shouldDelay, onHideCallback = () => {}) {
  ...
    contextMenuRef.current.disableClickOnContextMenuItem();
    setTimeout(() => {
        if (contextMenuRef.current.instanceID !== instanceID) {
            return;
        }
        contextMenuRef.current.hideContextMenu(onHideCallback);
    }, 800);
}

• In there, the disableClickOnContextMenuItem is:

  disableClickOnContextMenuItem() {
        this.setState({
            disableClickOnContextMenuItem: true,
        });
    }

Result

• Solution 1:

Screencast.from.24-08-2023.22.59.14.webm

• Solution 2:

Screencast.from.25-08-2023.02.26.25.webm

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[parasharrajat]

Why is this an issue?

Does one action prevent the other? or Does one action create issues for the other?

[DylanDylann]

@parasharrajat I think the expected is: User cannot click two items at once

[parasharrajat]

Technically, they are being clicked one by one, not at a time.

[DylanDylann]

@parasharrajat Yeah, I mean that once the ContextMenu is open, user can click on one item

[parasharrajat]

Ok. It might be but IMO, it is fine.

[abekkala]

It was decided in the Slack discussion that we should ensure the modal is closed when a context item is selected. Rather than staying open after the first selection

[melvin-bot]

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abekkala]

@abdulrahuman5196 have you had a chance to review the proposals that have been provided?

[melvin-bot]

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

[abekkala]

@abdulrahuman5196 have you reviewed the proposals for the fix?

[DylanDylann]

@abekkala @abdulrahuman5196 Currently, we are using the animation when selecting Mark as unread or Pin so I think we should not close the modal immediately after selecting item

Screencast.from.15-09-2023.11.57.21.webm

[melvin-bot]

@abekkala @abdulrahuman5196 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!

[abdulrahuman5196]

Reviewing now

[abdulrahuman5196]

@abekkala Sorry for the delay.

Currently, we are using the animation when selecting Mark as unread or Pin so I think we should not close the modal immediately after selecting item

I agree on this. We can't close immediately.

What should be the expected behaviour here? Do we make the other options not clickable?

[DylanDylann]

@abdulrahuman5196 my proposal will make other options not clickable #26917 (comment)

[melvin-bot]

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

[abdulrahuman5196]

@abdulrahuman5196 my proposal will make other options not clickable #26917 (comment)

But I am doubtful if we want it. Because if its not clickable how can the UI be? It would be better to confirm what exact behaviour we want? Like I have asked here #26917 (comment)

cc: @abekkala

[DylanDylann]

@abdulrahuman5196 Yeah we need to confirm. In my opinion, my proposal will disable other options, and when hovering over these options, it will display as {cursor: not-allowed} and the color will change to any "none-actvie" color

[melvin-bot]

@abekkala, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[abdulrahuman5196]

@abdulrahuman5196 my proposal will make other options not clickable #26917 (comment)

But I am doubtful if we want it. Because if its not clickable how can the UI be? It would be better to confirm what exact behaviour we want? Like I have asked here #26917 (comment)

cc: @abekkala

@abekkala Gentle ping on this.

And another point is, I am also skeptical about changing this behaviour. Because there are multiple other popovers using the same behaviour and we would end up changing others as well.

[abekkala]

@Julesssss pulling you in here as you were on the Slack conversation.

Should the modal close and not keep open to click back and forth both options?

[Julesssss]

Currently, we are using the animation when selecting Mark as unread or Pin so I think we should not close the modal immediately after selecting item

I don't think the animation is necessary tbh. The user doesn't need to see the state change to understand that it has occurred, plus it is causing this bug and looks like UI lag. Having said that, if the animation is desired then let's just do nothing here.

Let's ask @Expensify/design for their thoughts.

[melvin-bot]

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

[shawnborton]

I'm actually curious why we think this is a bug? I mean if the menu is open, and the options are selectable, why can't the user select either one as quickly as they want?

I mean obviously this is an edge case and we try to close the popover menu after one is selected. But I don't know that this is a "bug" personally.

[DylanDylann]

@shawnborton You can check this one #24261. In that, we also prevent users from selecting two emojis at once

[shawnborton]

Sure, but this is different - these aren't the same action (adding an emoji) - they are two distinct actions.

[melvin-bot]

@dannymcclain @Julesssss @abekkala @abdulrahuman5196 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]

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

[abekkala]

Ok, so it sounds like initially in the slack thread we decided this was something to "fix" and now after further reflection we decided this is not a bug. @Julesssss we good to just close this then?

[Julesssss]

Yeah, I agree. While not ideal let's not fix something that isn't broken