Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2024-02-09] [$2000] Chat - The message got sent but it also stayed in the compose box #26347

Closed lanitochka17 closed 8 months ago

lanitochka17 commented 1 year ago

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. Open New Expensify app
  2. Open any conversation
  3. Quickly enter any text in the compose box
  4. Tap the icon to send a message
  5. Quickly repeat steps 3-4 several times

Expected Result:

The compose box should be scrubbed after each click on the send message icon

Actual Result:

The compose box doesn't have time to scrub after each click on the send icon and new text is added to the old text already sent

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.59

Reproducible in staging?: Yes

Reproducible in production?: Yes

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

https://github.com/Expensify/App/assets/78819774/3f44a226-4faa-4495-96e8-219185702ea1

https://github.com/Expensify/App/assets/78819774/1f004dc6-31e4-4452-a70d-fcde3d7443a0

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bb269a50709f3aa2
  • Upwork Job ID: 1699460288474398720
  • Last Price Increase: 2023-10-02
  • Automatic offers:
    • aimane-chnaif | Reviewer | 27043430
    • situchan | Contributor | 27503921
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

mallenexpensify commented 1 year ago

I feel like this is an age-old issue that we used to run into a LOT.
https://github.com/Expensify/App/assets/22508304/13ddd9cc-dc69-4a57-a4fd-d4f7e358ec66

Checking on in #expensify-open-source https://expensify.slack.com/archives/C01GTK53T8Q/p1693872831763299

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

mallenexpensify commented 1 year ago

@NicMendonca I'm off this week, can you please keep 👀 on this then I'll snag it back on Monday? Thx

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignees @mallenexpensify and @NicMendonca are eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

📣 @h0u554m! 📣 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. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
h0u554m commented 1 year ago

Contributor details Your Expensify account email: houssammarrakchimk@gmail.com Upwork Profile Link: https://www.upwork.com/en-gb/freelancers/~01d68a22e5d906bca6

melvin-bot[bot] commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

h0u554m commented 1 year ago

Contributor details Your Expensify account email: https://new.expensify.com/a/15606409 Upwork Profile Link: https://www.upwork.com/en-gb/freelancers/~01d68a22e5d906bca6

h0u554m commented 1 year ago

You can either implement a brief delay (e.g., 1-2 seconds) to prevent the client from clicking the button while it's sending or change the button's icon to a loading indicator to indicate that the action is in progress.

andruxnet commented 1 year ago

Proposal

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

When a user rapidly enters text and tap the send message icon multiple times in quick succession, the compose box does not clear as expected. This results in the new message getting concatenated with the previously sent message text, creating a messy overlap in the compose box.

What is the root cause of that problem?

The root cause of this problem seems to be related to the asynchronous nature of the text handling and API call. As the user types, the updateComment function is debounced, delaying the processing time. When the user quickly types and hits the send button, it triggers the submitForm method, which also takes time to complete due to its interaction with the API.

Because of this delayed processing, when the user types and hits send rapidly, multiple calls to updateComment and submitForm can overlap. This overlapping leads to messages being concatenated, not cleared properly, or not sent to the API as expected.

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

After some extensive analysis of the relevant codebase, I explored a couple of options to solve this issue, but wasn't able to implement them properly since I'm still not very familiar with the app, and, my simulator does not work as fast as I see the one in the video showing the bug. I was able to reproduce the bug, though, and I did come up with a simple solution that hopefully you consider accepting.

I implemented a "versioning" strategy when rendering the ComposerWithSuggestions component. The versioning strategy helps because it forcibly remounts the component right after submitting the comment, clearing its internal state. However, this does not solve the root of the problem, which is the overlap of asynchronous operations (updateComment and submitForm). It does work, though, and can be used in the meantime and until a more robust solution can be implemented by a developer who's more experienced in Expensify's codebase.

Before Fix

https://github.com/Expensify/App/assets/3628909/07b4e20e-9cd8-4597-8e5b-ca1c00eacb2a

After Fix

https://github.com/Expensify/App/assets/3628909/4afa2c31-9878-4f3c-8cb3-a012e7853cdd

Contributor details

Your Expensify account email: aolvera@andrux.net Upwork Profile Link: https://www.upwork.com/freelancers/~01634d5f25c3c5b849?viewMode=1

melvin-bot[bot] commented 1 year ago

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

h0u554m commented 1 year ago

https://github.com/Expensify/App/blob/0e836df546939a9d905fee03b31879242de38a3b/src/pages/home/ReportScreen.js#L241


/**
 * @param {String} text
 */
const onSubmitComment = useCallback(
  (text) => {
    // Disable the function temporarily
    setSubmitDisabled(true);

    // Wait for 1 second (you can adjust the delay as needed)
    setTimeout(() => {
      // Re-enable the function
      setSubmitDisabled(false);

      // Now you can perform the comment submission
      Report.addComment(getReportID(route), text);
    }, 1000); // 1000 milliseconds = 1 second
  },
  [route]
);

// In your component, you can track whether submission is disabled
const [submitDisabled, setSubmitDisabled] = useState(false);

// You can conditionally disable the button based on submitDisabled
// For example:
{/* <button onClick={() => onSubmitComment('Your comment')} disabled={submitDisabled}>Submit Comment</button> */}

Contributor details Your Expensify account email: https://new.expensify.com/a/15606409 Upwork Profile Link: https://www.upwork.com/en-gb/freelancers/~01d68a22e5d906bca6

mallenexpensify commented 1 year ago

Thanks @NicMendonca. @aimane-chnaif, can you review the @h0u554m 's proposal above? I'm hoping this is a new discrete issue and not related to a bug from a year ago 🤞

h0u554m commented 1 year ago

Is it new

h0u554m commented 1 year ago

@mallenexpensify

melvin-bot[bot] commented 1 year ago

@mallenexpensify @aimane-chnaif this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

aimane-chnaif commented 1 year ago

@h0u554m please review our contributing guideline and follow proposal template

h0u554m commented 1 year ago

Problem Statement: We need to address an issue where, upon multiple rapid clicks, the message is not reset to its default empty state when sending a message.

Root Cause Analysis: The problem arises due to multiple presses on the send message button in quick succession.

Proposed Solution: To solve this issue, we can implement a brief delay of 1-2 seconds to prevent the client from clicking the button while the message is being sent.

Alternative Solution Considered: Another alternative solution is to change the button's icon to a loading indicator to indicate that the action is in progress.

Solution Implementation: You can find the code implementation for this solution at this GitHub link.

Here's the refactored code for the solution:

/**
 * @param {String} text
 */
const onSubmitComment = useCallback(
  (text) => {
    // Disable the function temporarily
    setSubmitDisabled(true);

    // Wait for 1 second (you can adjust the delay as needed)
    setTimeout(() => {
      // Re-enable the function
      setSubmitDisabled(false);

      // Now you can perform the comment submission
      Report.addComment(getReportID(route), text);
    }, 1000); // 1000 milliseconds = 1 second
  },
  [route]
);

// In your component, you can track whether submission is disabled
const [submitDisabled, setSubmitDisabled] = useState(false);

// You can conditionally disable the button based on submitDisabled
// For example:
{/* <button onClick={() => onSubmitComment('Your comment')} disabled={submitDisabled}>Submit Comment</button> */}

Contributor details Your Expensify account email: houssammarrakchimk@gmail.com Upwork Profile Link: https://www.upwork.com/en-gb/freelancers/~01d68a22e5d906bca6

melvin-bot[bot] commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

h0u554m commented 1 year ago

@aimane-chnaif

aimane-chnaif commented 1 year ago

@h0u554m both root cause and solution are incorrect

h0u554m commented 1 year ago

@aimane-chnaif can you explain more

aimane-chnaif commented 1 year ago

multiple presses on the send message button in quick succession

This is not the root cause.

setTimeout is not acceptable here. And please don't post code diff as solution.

Please check other contributors' proposals on other issues.

h0u554m commented 1 year ago

Okay 1s i will fix it

h0u554m commented 1 year ago

Problem Statement: We need to address an issue where, upon multiple rapid clicks, the message is not reset to its default empty state when sending a message.

Root Cause Analysis: The root cause of the issue is multiple rapid clicks on the "send message" button. These rapid clicks can interfere with the expected behavior, preventing the message from being reset to its default empty state.

Proposed Solution: disable the button when onSubmitComment is called, perform the comment submission asynchronously, handle any errors, and then re-enable the button using the try...catch...finally pattern.

Solution Implementation: You can find the code implementation for this solution at this GitHub link.

Here's the refactored code for the solution:


const onSubmitComment = useCallback(
  async (text) => {
    // Disable the function temporarily
    setSubmitDisabled(true);

    try {
      // Now you can perform the comment submission
      await Report.addComment(getReportID(route), text);

      // Comment submitted successfully, you can add any success handling here if needed
    } catch (error) {
      // Handle any errors here
      console.error("Comment submission error:", error);
    } finally {
      // Re-enable the function
      setSubmitDisabled(false);
    }
  },
  [route]
);

Contributor Details:

h0u554m commented 1 year ago

@aimane-chnaif i think that better what is your opinion please

aimane-chnaif commented 1 year ago

Awaiting proposals

melvin-bot[bot] commented 1 year ago

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

mallenexpensify commented 1 year ago

@aimane-chnaif can you review @h0u554m 's updated proposal and provide feedback? https://github.com/Expensify/App/issues/26347#issuecomment-1717316009

andruxnet commented 1 year ago

Proposal

Updated

melvin-bot[bot] commented 1 year ago

@mallenexpensify @aimane-chnaif 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!

aimane-chnaif commented 1 year ago

@aimane-chnaif can you review @h0u554m 's updated proposal and provide feedback? #26347 (comment)

@h0u554m has still incorrect root cause.

aimane-chnaif commented 1 year ago

@andruxnet thanks for the proposal. Please explain the root cause in more detail. i.e. which lines cause delay of clearing draft message?

aimane-chnaif commented 1 year ago

For contributors: please share demo videos before and after applying your fixes with the same repro step.

h0u554m commented 1 year ago

Problem Statement: We need to address an issue where, upon multiple rapid clicks, the message is not reset to its default empty state when sending a message.

Root Cause: The root cause of this problem stems from the inefficiency of the existing compose box clearing process. It fails to execute quickly enough to match the pace of rapid, successive input and sending of messages. As a result, the compose box clearance process lags behind, leading to the unintended overlap of old and new message text

Proposed Solution: disable the button when onSubmitComment is called, perform the comment submission asynchronously, handle any errors, and then re-enable the button using the try...catch...finally pattern.

Solution Implementation: You can find the code implementation for this solution at this GitHub link.

Here's the refactored code for the solution:


const onSubmitComment = useCallback(
  async (text) => {
    // Disable the function temporarily
    setSubmitDisabled(true);

    try {
      // Now you can perform the comment submission
      await Report.addComment(getReportID(route), text);

      // Comment submitted successfully, you can add any success handling here if needed
    } catch (error) {
      // Handle any errors here
      console.error("Comment submission error:", error);
    } finally {
      // Re-enable the function
      setSubmitDisabled(false);
    }
  },
  [route]
);

Contributor Details:

melvin-bot[bot] commented 1 year ago

📣 @h0u554m! 📣 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. Make sure you've read and understood the contributing guidelines.
  2. 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.
  3. 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.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
aimane-chnaif commented 1 year ago

@h0u554m please refer to https://github.com/Expensify/App/issues/26347#issuecomment-1727585857 and https://github.com/Expensify/App/issues/26347#issuecomment-1727586690

melvin-bot[bot] commented 1 year ago

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

aimane-chnaif commented 1 year ago

No new proposals

andruxnet commented 1 year ago

Proposal

Updated

Dag deeper into the code so hopefully my assessment is more accurate now.

aimane-chnaif commented 1 year ago

@andruxnet please check https://github.com/Expensify/App/issues/26347#issuecomment-1727586690 The proposed solution should be "should", not "might"

melvin-bot[bot] commented 1 year ago

Upwork job price has been updated to $1000

mallenexpensify commented 1 year ago

Doubled to get more 👀 and proposals

melvin-bot[bot] commented 1 year ago

@mallenexpensify @aimane-chnaif this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 1 year ago

Current assignee @aimane-chnaif is eligible for the Internal assigner, not assigning anyone new.