[HOLD for newdot payment after BugZero checklist is complete] [$1000] Pressing escape on the composer deletes your message, and CMD+Z doesn't bring it back

[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. Draft a message
2. Press escape
3. Press CMD+Z

Expected result:

Your draft message should come back

Actual result:

Your draft message is gone forever

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.35-5
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.5259.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018379455c4980f440
• Upwork Job ID: 1675763769888301056
• Last Price Increase: 2023-07-03

[melvin-bot]

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

[c3024]

Proposal

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

Escape in composer removes the draft and CMD Z does not restore it.

What is the root cause of that problem?

This is the part of code in ReportActionCompose for Esc key.

if (e.key === CONST.KEYBOARD_SHORTCUTS.ESCAPE.shortcutKey) {
            e.preventDefault();
            if (suggestionsExist) {
                this.resetSuggestions();
            } else if (this.comment.length > 0) {
                this.updateComment('', true);
            }
            return;
        }

This clears the draft on clicking Esc and CMD Z being a system function does not have history for this.

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

I see the point of removing mentions with Esc but I don't think it makes much sense to delete draft message on Esc
I think we should remove the else from the snippet in above RCA and not clear composer when pressing Esc.
Then CMD Z can remove the previous letter or paste as applicable.

What alternative solutions did you explore? (Optional)

[honnamkuan]

Proposal

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

After typing text into composer input, pressing Esc and then Ctrl-Z does not bring the text back.

What is the root cause of that problem?

The current logic update the comment to empty via state change instead of user interactions.
The 'clearing' of text is not included into part of the input history, hence Ctrl-Z does not bring back the deleted text

App/src/pages/home/report/ReportActionCompose.js

Lines 806 to 807 in c0ece67

	} else if (this.comment.length > 0) {
	this.updateComment('', true);

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

We need to execute browser commands selectAll and delete before the this.updateComment('', true);, that will ensure the 'clearing' get captured by browser, and Ctrl-Z will bring the cleared text back

It will be a browser specific command, so logic will be implemented in
App/src/libs/ComposerUtils/clearComment/index.js
and we will have a dummy function which does nothing in
App/src/libs/ComposerUtils/clearComment/index.native.js

clearComment then get imported to ComposerUtils, and get exported to be used in ReportActionCompose triggerHotkeyActions function.
like so:

        if (e.key === CONST.KEYBOARD_SHORTCUTS.ESCAPE.shortcutKey) {
            e.preventDefault();
            if (suggestionsExist) {
                this.resetSuggestions();
            } else if (this.comment.length > 0) {
                ComposerUtils.clearComment();
                this.updateComment('', true);
            }
            return;
        }

Results

Screen.Recording.2023-07-02.at.12.23.55.PM.mov

What alternative solutions did you explore? (Optional)

[chiragxarora]

I think this is intentional... @shawnborton

[kadiealexander]

@roryabraham just to confirm the desired behaviour, ESC should get rid of the message but you should be able to bring it back with CTRL+Z?

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018379455c4980f440

[melvin-bot]

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

[melvin-bot]

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

[c3024]

Either Esc should only remove mention and emoji suggestions and not clear composer like slack web or if Esc clears composer it must have a shortcut to restore the message. Without that it would be very annoying if someone clicks Esc unintentionally after writing a long message.

[oluwagbemigadavid]

You can store the contents of the message body in a temporal var and listen for the ctrl+z to replace the current msg body with the temporal var value

[melvin-bot]

📣 @oluwagbemigadavid! 📣
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>

[oluwagbemigadavid]

Contributor details
Your Expensify account email: koningswealth@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01c3711093b8c8aeef

[melvin-bot]

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

[narefyev91]

Proposal from @honnamkuan looks good to me #22053 (comment)
🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[dangrous]

Before assigning, I'd love to confirm the expected behavior here @shawnborton and @roryabraham.

To me, I feel like Esc shouldn't clear the input box at all - which if we decide is correct, then we would want to go with a different proposal. But I'm not sure what the original intention is so would love to hear the group's thoughts!

[narefyev91]

Guys @shawnborton @roryabraham any updates here?

[honnamkuan]

Sharing some links relevant to the earlier decision to make Esc clear composer input:

Slack: https://expensify.slack.com/archives/C049HHMV9SM/p1681480139063839

Issue: #18434
PR: #15650

[narefyev91]

Sharing some links relevant to the earlier decision to make Esc clear composer input:

Slack: https://expensify.slack.com/archives/C049HHMV9SM/p1681480139063839

Issue: #18434 PR: #15650

Thanks @honnamkuan! Based on the discussion text should be removed - when you click on Esc button, and CMD+Z should bring it back. I think we good to go @dangrous

[c3024]

The premise that since pressing Esc cancels the edit, pressing Esc in composer should reset it does not appear to be logical.

[dangrous]

I'm following up in that convo, thank you for the link! Sorry for the delay, I just am still not sold on this behavior

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

• do not clear composer when escape is pressed #22350

If no regressions arise, payment will be issued on 2023-07-20. 🎊

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

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

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

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

[kadiealexander]

@c3024 I've sent you an Upwork contract, please accept!

@narefyev91 If you're being paid via newdot please let me know, otherwise let me know your upwork details and I'll send across a contract. Also, please don't forget the checklist!

[c3024]

@kadiealexander Thank you. Accepted. PR was merged within 3 days and this is eligible for bonus. Will the bonus be paid as a separate Upwork job?

[kadiealexander]

It will be paid as a bonus on this Upwork job :) The total payment will be $1500.

[dangrous]

I think we're still waiting on the checklist, anything else needed on this one?

[kadiealexander]

Just the checklist and C+ payment :)

[kadiealexander]

Payouts due:

• Issue Reporter: n/a
• Contributor: $1000 @c3024
• Contributor+: n/a (callstack)

Eligible for 50% #urgency bonus? Yes, $500

Upwork job is here.

[dangrous]

Hey @narefyev91! Let us know when you have a chance to look at that checklist - thank you!

[narefyev91]

• The PR that introduced the bug has been identified. Link to the PR: - i will say - that this issue was not coming from any regression - it was initially implement to clean up the composer - and after some discussions new workflow was chosen
• 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 - no comments in case it was not a regression
• 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 - no discussions
• Determine if we should create a regression test for this bug - no need additional regression tests here

[dangrous]

yep i think that's all right!

[dangrous]

I think we're just waiting on the C+ payment here now, right?

[narefyev91]

I think we're just waiting on the C+ payment here now, right?

I'm from Callstack - not needs any additional payments from NewDot or Upwork :-)

[dangrous]

Ah, lovely! @kadiealexander when you have time I think we can update the payment checklist to reflect that ^ and then can close!