[$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 contributors@expensify.com 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?