Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 6 tasks
lanitochka17 opened this issue Aug 30, 2023 · 99 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Aug 30, 2023

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?

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

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

Bug6183207_Record_Android_CB.mp4
Bug6183207_Record_iOS_CB.mp4

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
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 30, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@mallenexpensify
Copy link
Contributor

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 melvin-bot bot removed the Overdue label Sep 5, 2023
@mallenexpensify mallenexpensify added Overdue Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Sep 5, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 5, 2023

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

@mallenexpensify
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 5, 2023
@NicMendonca NicMendonca added the External Added to denote the issue can be worked on by a contributor label Sep 6, 2023
@melvin-bot melvin-bot bot changed the title Chat - The message got sent but it also stayed in the compose box [$500] Chat - The message got sent but it also stayed in the compose box Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

📣 @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
Copy link

h0u554m commented Sep 6, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

⚠️ 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
Copy link

h0u554m commented Sep 6, 2023

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

@h0u554m
Copy link

h0u554m commented Sep 6, 2023

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
Copy link

andruxnet commented Sep 6, 2023

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

Screen.Recording.2023-10-03.at.13.08.40.mp4

After Fix

Screen.Recording.2023-10-02.at.23.04.03.mp4

Contributor details

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 29, 2024
@mountiny mountiny added the Reviewing Has a PR in review label Jan 31, 2024
@melvin-bot melvin-bot bot removed the Overdue label Jan 31, 2024
@mountiny
Copy link
Contributor

Second PR got merged

Copy link

melvin-bot bot commented Feb 1, 2024

⚠️ 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.

Copy link

melvin-bot bot commented Feb 1, 2024

⚠️ 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.

Copy link

melvin-bot bot commented Feb 1, 2024

⚠️ 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.

@mountiny
Copy link
Contributor

mountiny commented Feb 2, 2024

its a tough one but we getting through it

Copy link

melvin-bot bot commented Feb 2, 2024

⚠️ 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.

@muttmuure muttmuure moved this from CRITICAL to HIGH in [#whatsnext] #quality Feb 2, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Feb 2, 2024
@melvin-bot melvin-bot bot changed the title [$2000] Chat - The message got sent but it also stayed in the compose box [HOLD for payment 2024-02-09] [$2000] Chat - The message got sent but it also stayed in the compose box Feb 2, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 2, 2024
Copy link

melvin-bot bot commented Feb 2, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Feb 2, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.35-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-02-09. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Feb 2, 2024

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:

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

@mallenexpensify
Copy link
Contributor

@mountiny, both @situchan and @aimane-chnaif have been paid $2k, Is that correct?

What regression tests do we need for this??!?!

@mountiny
Copy link
Contributor

Not sure what are the best regression steps for this. I think its one of those we would raise over time if it comes back

Has there been reliable reproduction steps?

@mallenexpensify
Copy link
Contributor

@situchan and @aimane-chnaif , from above, can you confirm this is correct?

Contributor+: @situchan paid $2000 via Upwork
Contributor+: @aimane-chnaif paid $2000 via Upwork.

I'm unaware of reliable reproduction steps. @hurali97 @kacper-mikolajczak @aimane-chnaif @situchan , any of you know?

@aimane-chnaif
Copy link
Contributor

Not exactly sure about the amount. This was very old issue and maybe applies the same amount in issue title, similar to #10148? The size of PR was also large enough.

Repro step is same as stated in OP. Our PR was supposed to fix this in most devices (#30168 (comment))
But sometimes still occurring in heavy android devices (#30168 (comment))

So I'd like to say:
Originally, reproducible 20%
After #10148 fix, reproducible 5%
After #26347 (our fix), reproducible 3%
(This is just example numbers, not correct)

@mallenexpensify
Copy link
Contributor

TestRail GH created - https://github.com/Expensify/Expensify/issues/371196

Not exactly sure about the amount.

I'm going to leave as-is then and close this. Comment/reopen if you disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
Development

No branches or pull requests