[HOLD for payment 2022-11-29] [$250] Deleting a comment unresponsive after right click to copy - reported by @gadhiyamanan

[mvtglobally]

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. Go to any chat > long press on any message
2. click copy to clipboard or mark as read
3. before closing popup click on delete comment

Expected Result:

Message should be deleted

Actual Result:

Message is not deleted

Workaround:

unknown

Platform:

Where is this issue occurring?

• Android

Version Number: 1.1.99-0 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43995119/190951224-a0801fcd-ec5e-4b04-a2be-c6eaa0c26ba1.mp4

Expensify/Expensify Issue URL: Issue reported by: @gadhiyamanan Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661673321608069

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @RachCHopkins (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[RachCHopkins]

Working on v1.2.0-8

[gadhiyamanan]

@RachCHopkins crash was fixed but still not able to delete message . https://user-images.githubusercontent.com/54790231/191170377-072d998c-1c20-490d-ad30-50ea1fcf640d.mp4

[RachCHopkins]

I'm unable to reproduce this - has anyone else reported the same problem?

[melvin-bot[bot]]

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

[deetergp]

@RachCHopkins I am also unable to reproduce this 🤷 and my message deleted fine. I'm inclined to close it…

[gadhiyamanan]

strange... but I am still not able to delete the message by following the steps Version(v1.2.3-0)

Go to any chat > long press on any message click copy to clipboard or mark as read before closing popup click on delete comment

https://user-images.githubusercontent.com/54790231/191418914-0698a4c3-45ca-4d33-87ef-8b6a69c6bd61.mov

cc: @deetergp

[melvin-bot[bot]]

@deetergp Eep! 4 days overdue now. Issues have feelings too...

[gadhiyamanan]

@deetergp still not able to delete message with latest version after mark as read or copy to clipboard https://user-images.githubusercontent.com/54790231/192721948-a1e0dcf8-bcc5-4e7e-bbe8-0389ddaef997.mp4

[muttmuure]

I've reproduced this on web and iOS.

Reproduction steps are correct.

Adding external label

https://user-images.githubusercontent.com/46995600/200344400-998f3352-eac9-4da0-8b32-d0e3cfc6d38c.mp4

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[muttmuure]

Job is here

[Pujan92]

Proposal

On hideContextMenu method of the PopoverReportActionContextMenu component, we can remove the resetting of the report fields. The issue occurs due to reportID 0 passed when for the same popover if we try to delete the comment after Copy To Clipboard/Mark as Unread.
Line number 200 in PopoverReportActionContextMenu.js file

hideContextMenu(onHideActionCallback) {
        // debugger
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
            isPopoverVisible: false,
        });
    }

So to me no need to reset report fields there as we are allowed to delay the popover to be closed, for each PopoverReportActionContextMenu it will create the new state anyway from the constructor.

[mananjadhav]

Okay I understood the problem and the reproduction steps.

@Pujan92 I tried your proposal and it didn't work for me. Just removing the reportID: 0 from setState can cause problems as PopoverReportActionContextMenu is a shared component and might already be mounted for other components.

@deetergp @muttmuure This is. happening only for Copy... menus because we wait until the text changes to Copied. Should we disable the other items when we're in this state?

[Pujan92]

@mananjadhav Not only reportID: 0, we have to take out reportAction: {} also as it might contain the action for deletion and resetting causes the issue. So removing these 2 fields from setState within hideContextMenu works for me.

[mananjadhav]

Okay thanks for pointing out. Let's just wait for @deetergp to confirm the expected behavior.

[mananjadhav]

@deetergp Quick bump on this

[deetergp]

Should we disable the other items when we're in this state?

I think we should just not wait for the Copied message. If the popup menu is there to receive the message, then great, but if the user has chosen to delete the message—and we have a modal to prevent accidental deletions—then we should do what the user wants and delete the message.

Thoughts, @muttmuure?

[muttmuure]

I agree with @deetergp - the button should be doing what it says it does, and we give a fair warning.

[mananjadhav]

Okay thanks for the comment. In that case I am fine with @Pujan92's proposal here.

@deetergp All yours.

🎀 👀 🎀  C+ reviewed

[Pujan92]

Do I need to raise PR for this or still need to wait for any approval?

[muttmuure]

If @mananjadhav and @deetergp are both cool with it, @Pujan92 apply for the job and we can get going

[deetergp]

@Pujan92's proposal looks good to me 👍

[mananjadhav]

PR is being reviewed, @deetergp can you assign the job to @Pujan92 on the issue here?

[melvin-bot[bot]]

📣 @Pujan92 You have been assigned to this job by @muttmuure! Please apply to this job in Upwork 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 📖

[deetergp]

Not overdue; PR is ~being reviewed~ done 🎉 .

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[0xmiros]

Proposal

Accepted proposal caused regression - https://github.com/Expensify/App/issues/12889

Root cause: https://github.com/Expensify/App/issues/12889#issuecomment-1322353170

According to this comment, I am proposing new solution here Solution: https://github.com/Expensify/App/issues/12889#issuecomment-1322356108

So final code looks like this:

    /**
     * After Popover hides, call the registered onPopoverHide & onPopoverHideActionCallback callback and reset it
     */
    runAndResetOnPopoverHide() {
+       this.state.reportID = '0';
+       this.state.reportAction = {};
        this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
        this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
    }

    /**
     * Hide the ReportActionContextMenu modal popover.
     * @param {Function} onHideActionCallback Callback to be called after popover is completely hidden
     */
    hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
-           reportID: '0',
-           reportAction: {},
            selection: '',
            reportActionDraftMessage: '',
            isPopoverVisible: false,
        });
    }

[aldo-expensify]

Proposal

Accepted proposal caused regression - #12889

Root cause: #12889 (comment)

According to this comment, I am proposing new solution here Solution: #12889 (comment)

So final code looks like this:

    /**
     * After Popover hides, call the registered onPopoverHide & onPopoverHideActionCallback callback and reset it
     */
    runAndResetOnPopoverHide() {
+       this.state.reportID = '0';
+       this.state.reportAction = {};
        this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
        this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
    }

    /**
     * Hide the ReportActionContextMenu modal popover.
     * @param {Function} onHideActionCallback Callback to be called after popover is completely hidden
     */
    hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
        }
        this.setState({
-           reportID: '0',
-           reportAction: {},
            selection: '',
            reportActionDraftMessage: '',
            isPopoverVisible: false,
        });
    }

This proposal is mutating the state. From my knowledge of React, this is something that should be completely avoided and can cause hard to track bugs, are there cases where mutations of the state is acceptable? No, as far as I know, but please let me know if I'm wrong about this.

SO about mutating state: https://stackoverflow.com/a/40309023/16434681

[aldo-expensify]

What if to fix this issue we just close the context menu once we click the first action. If you want to delete the comment after, you would just have to open the context menu again. This for me is the normal behaviour of a context menu, they usually close as soon as you press something.

[0xmiros]

@aldo-expensify it's same as below in this component since shouldComponentUpdate is still false regardless of reportID, reportAction state values change

this.setState({reportID: '0', reportAction: {}}, () => {
        this.onPopoverHide = this.runAndResetCallback(this.onPopoverHide);
        this.onPopoverHideActionCallback = this.runAndResetCallback(this.onPopoverHideActionCallback);
})

[0xmiros]

This proposal is mutating the state. From my knowledge of React, this is something that should be completely avoided and can cause hard to track bugs, are there cases where mutations of the state is acceptable? No, as far as I know, but please let me know if I'm wrong about this.

This is usually the case when no need to re-render because of state update. Of course we can use class member variable (i.e. this.reportID) instead of state variable (i.e. this.state.reportID) to avoid re-render but in this component, it's fine and safe to mutate those directly.

[mananjadhav]

@Pujan92 We have a regression on this issue, that we'll have to fix. Please create a fresh PR to fix the issue and then we can review.

[aldo-expensify]

This proposal is mutating the state. From my knowledge of React, this is something that should be completely avoided and can cause hard to track bugs, are there cases where mutations of the state is acceptable? No, as far as I know, but please let me know if I'm wrong about this.

This is usually the case when no need to re-render because of state update. Of course we can use class member variable (i.e. this.reportID) instead of state variable (i.e. this.state.reportID) to avoid re-render but in this component, it's fine and safe to mutate those directly.

I understand the point that it may be safe in this specific case at this time, but this type of code removes the assumption that the state is immutable. Removing that assumptions open the door for future bugs because other developers won't expect this, also, simple references check are not longer enough for checking equality. In my opinion it is similar to storing your state in window, yes, it may work, but at some point you will have hard to follow buggy code that breaks all the time because it is outside of what we can expect in react.

I'm not very familiarized with class component, but yes, I think we use class members variable for the cases. In functional react, this can be done with useRef because the reference.current is mutable.

Found this in the react documentation that talks about this: https://reactjs.org/docs/react-component.html#state Seems like the way to go is to use class member variables if you want to do something like that.

[Pujan92]

Sorry for the regression issue. I think updating the hideContextMenu with the check of the callback function which gets passed when there is a delay in context menu action can solve the issue. So if the action is deleting the comment we won't update the state from hideContextMenu method otherwise it(delete modal) won't have the reportID and reportAction.

hideContextMenu(onHideActionCallback) {
        if (_.isFunction(onHideActionCallback)) {
            this.onPopoverHideActionCallback = onHideActionCallback;
            if (!this.state.isDeleteCommentConfirmModalVisible) {
                this.setState({
                    reportID: '0',
                    reportAction: {},
                    selection: '',
                    reportActionDraftMessage: '',
                    isPopoverVisible: false,
                });
            }
        } else {
            this.setState({
                reportID: '0',
                reportAction: {},
                selection: '',
                reportActionDraftMessage: '',
                isPopoverVisible: false,
            });
        }
    }

Recording_1669097982090.webm

[mananjadhav]

@Pujan92 thanks for the quick PR, I'll try to get this one before tomorrow.

[mananjadhav]

@muttmuure Payment date to be removed, as it caused a regression.

[Pujan92]

@Pujan92 thanks for the quick PR, I'll try to get this one before tomorrow.

@mananjadhav Could you plz review once you get some time

[mananjadhav]

Apologies for the delay here. I'll review it by EOD today!

[0xmiros]

I think I am eligible for compensation since my solution was picked up and merged ref: https://github.com/Expensify/App/pull/12917#issuecomment-1331669521

[mananjadhav]

This was deployed to production but title for the payment date isn't updated.

@muttmuure please note this had a regression so C+ payment has to be adjusted accordingly.

[mananjadhav]

@deetergp @muttmuure quick bump

[Pujan92]

@muttmuure Are we fine to complete this?

[muttmuure]

Thanks for the bump guys - getting this sorted today

[muttmuure]

OK by the logic of Stack Overflow:

@Pujan92 = $250 @0xmiroslav = $250 @mananjadhav = $125

The job is here:

https://www.upwork.com/jobs/~012e1e9aa00357fd02

Please could you apply and we'll get you paid and close this out

[Pujan92]

https://www.upwork.com/jobs/~012e1e9aa00357fd02 Please could you apply and we'll get you paid and close this out

I have already applied for the earlier posted job(https://www.upwork.com/jobs/~01bc6b990f835122e4)

[0xmiros]

@muttmuure applied

[gadhiyamanan]

@muttmuure applied for reporting