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
2.99k stars 2.5k forks source link

[HOLD for payment 2023-08-07] [$8000] The message got sent but it also stayed in the compose box #10148

Closed mvtglobally closed 9 months ago

mvtglobally 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. Type something
  2. Press the send icon
  3. Right after pressing the send icon, start typing again quickly without any break. The compose box will still have the text you just sent

Expected Result:

Compose box should clear

Actual Result:

Compose box doesn't clear

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.85-8 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 related to https://github.com/Expensify/App/issues/4129

https://user-images.githubusercontent.com/43995119/181589365-7a29867b-c0cd-412f-ac2a-2db3d0c42cb8.MP4

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

View all open jobs on GitHub

melvin-bot[bot] commented 1 year ago

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

pecanoro commented 1 year ago

I was able to reproduce on my phone, so assigning external.

melvin-bot[bot] commented 1 year ago

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

mateusbra commented 1 year ago

I think its duplicate of https://github.com/Expensify/App/issues/4129

mountiny commented 1 year ago

I think its duplicate of https://github.com/Expensify/App/issues/4129

@mateusbra Thanks! We are aware of that, it is mentioned in the issue body. That issue got very long and hard to follow so we decided to make a clean start and just refer to it. Lots of time has passed since then so maybe easier and cleaner solutions will pop up.

dhairyasenjaliya commented 1 year ago

This https://github.com/Expensify/App/issues/8592#issuecomment-1152926071 proposal I have explained why this happens and also provided a solution (except mobile safari) that can solve this issue, as well as another issue, of blue opacity issue, remains in iOS after spell check https://github.com/Expensify/App/issues/8592

melvin-bot[bot] commented 1 year ago

@dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

dylanexpensify commented 1 year ago

Getting on it today

dylanexpensify commented 1 year ago

Internal: https://www.upwork.com/ab/applicants/1554462059426074624/job-details External: https://www.upwork.com/jobs/~0130f26b3941d0c325

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

rushatgabhane commented 1 year ago

This https://github.com/Expensify/App/issues/8592#issuecomment-1152926071 proposal I have explained why this happens and also provided a solution (except mobile safari) that can solve this issue, as well as another issue, of blue opacity issue, remains in iOS after spell check https://github.com/Expensify/App/issues/8592

@dhairyasenjaliya I'm sorry, I'm not sure if I understand.

Can you please write up a proposal on this issue, thanks!

dhairyasenjaliya commented 1 year ago

actually this issue I have discussed here https://github.com/Expensify/App/issues/8592#issuecomment-1152926071 before, but I have not reported this bug on slack and also I did write a proposal there in order to solve

Currently, I'm waiting for the internal team here https://github.com/Expensify/App/issues/8592#issuecomment-1203057240 they decide to split mWeb and iOS as 2 separate issues once I get to know what's next step I will notify you here as well

CC @rushatgabhane

rushatgabhane commented 1 year ago

what's next step I will notify you here as well

Cool, thanks!

hamzashakeel348 commented 1 year ago

@mountiny If it's still open, I can resolve it. Kindly let me know.

rushatgabhane commented 1 year ago

@dhairyasenjaliya can you please post a proposal here as well so that I can review it? Explaining the root cause for it is highly recommended.

rushatgabhane commented 1 year ago

@hamzashakeel348 technically, this issue is still open as no proposal is accepted yet.

It's just that your proposal might not be the first because @dhairyasenjaliya already has posted one on another issue (i could be wrong)

You could post a proposal and it'll be accepted if it turns out to be the best proposal (and different from @dhairyasenjaliya's proposal)

dhairyasenjaliya commented 1 year ago

Proposal

Solution

https://github.com/Expensify/App/blob/11a1f0debac2dfee461e6b71751c3b3bcfb9f2cf/src/pages/home/report/ReportActionCompose.js#L425-L430

Like this :

 this.props.onSubmit(trimmedComment);
 this.updateComment(''); // here we are already clearing composer textinput so applying 0 selection below this has no effect 
 this.setTextInputShouldClear(true);
 // Important to reset the selection on Submit action
 //this.textInput.setNativeProps({selection: {start: 0, end: 0}}); // comment this line solves problem in iOS (app)

Note: commenting on this line won't affect any other platform as we are already clearing the composer

Result (After commenting setNativeProps line) :

Device: iPhone 13(App)

https://user-images.githubusercontent.com/47522946/183237719-1d70829b-f1e0-4f73-9bea-300f3c13b9e0.mp4

Result (Without commenting setNativeProps line Current behavior) :

https://user-images.githubusercontent.com/47522946/183237721-c4522546-c016-4dbc-a4ca-458bd188d7ed.mp4

cc @rushatgabhane this is the same proposal I posted in another issue as this proposal solves 2 issue's of iOS (app)

rushatgabhane commented 1 year ago

@hamzashakeel348 do you have a different proposal?

hamzashakeel348 commented 1 year ago

One easy solution is that, as before entering the message the value for that input is empty string, and after submitting the message if we set the that value to empty string again by using the hooks, it can resolve the issue.

It's one of the easiest solution we can do.

swiftpipe commented 1 year ago

Proposal

I think this problem is related to asynchronous. When creating a comment, it is delayed when running and at the same time resetting the comment string to empty and calling the API so simply we just need to setTimeout for the API call to create the comment.

Solution

I tried setTimeout for onSubmit function and it worked fine

https://github.com/Expensify/App/blob/9855ad8c4abb39d9d1710a70f59f23b1dd887aef/src/pages/home/report/ReportActionCompose.js#L466-L477

Like this:

async submitForm(e) {
    if (e) {
      e.preventDefault();
    }

    const comment = this.prepareCommentAndResetComposer();
    if (!comment) {
      return;
    }

    setTimeout(() => { <---- change this
      this.props.onSubmit(comment);
    });
  }

And this is the result

https://user-images.githubusercontent.com/37079377/183067639-83f7bc24-e14c-413e-bd47-2aef3d647cc6.mp4

Please consider this proposal.

rushatgabhane commented 1 year ago

after submitting the message if we set the that value to empty string

@hamzashakeel348 I'm sure we already do that https://github.com/Expensify/App/blob/1d06ec9cdb650351d4fb3ca51026dec879975090/src/pages/home/report/ReportActionCompose.js#L440


@swiftpipe we avoid workarounds that use setTimeout. It suggests some race condition and doesn't fix the root cause

rushatgabhane commented 1 year ago

@dhairyasenjaliya i'm sorry i don't see how your proposal is related to this issue. Please see the video attached in the issue body for a better understanding

swiftpipe commented 1 year ago

@hamzashakeel348 I'm sure we already do that

Yes I have seen this but our problem is related to the submitForm function specifically the async when calling this.props.onSubmit(comment);

Try removing this function we will see that it works very well, there will be no case that the message has been sent but is still in the compose box.

https://github.com/Expensify/App/blob/9855ad8c4abb39d9d1710a70f59f23b1dd887aef/src/pages/home/report/ReportActionCompose.js#L476

@swiftpipe we avoid workarounds that use setTimeout. It suggests some race condition and doesn't fix the root cause

Sorry, this may not be the right way

hamzashakeel348 commented 1 year ago

after submitting the message if we set the that value to empty string

@hamzashakeel348 I'm sure we already do that

https://github.com/Expensify/App/blob/1d06ec9cdb650351d4fb3ca51026dec879975090/src/pages/home/report/ReportActionCompose.js#L440

@swiftpipe we avoid workarounds that use setTimeout. It suggests some race condition and doesn't fix the root cause

@rushatgabhane I checked it again. We are doing ` const comment = this.prepareCommentAndResetComposer(); if (!comment) { return; }

    this.props.onSubmit(comment);
}`

We are setting props as comment if comment is not empty, we also have to do to make them empty string after submitting data.
dhairyasenjaliya commented 1 year ago

@dhairyasenjaliya i'm sorry i don't see how your proposal is related to this issue. Please see the video attached in the issue body for a better understanding

It's same will update video

dhairyasenjaliya commented 1 year ago

@rushatgabhane updated the video here https://github.com/Expensify/App/issues/10148#issuecomment-1206003090

Note : In order to understand this issue please follow like this iOS app

  1. go to any chat
  2. type missed spell word that is actually highlighted by spell checker
  3. wait for spellcheck to turn into blue color word
  4. now send immediately with the blue color word
  5. you will notice blue color text will remain in the composer even after sending a message
dhairyasenjaliya commented 1 year ago

also am I eligible to report this issue since I already logged this issue on 10th June here on slack but somehow at that time it does not log into GitHub and recently got added https://expensify.slack.com/archives/C01GTK53T8Q/p1654860503611229

rushatgabhane commented 1 year ago

@dhairyasenjaliya you might be right. But the steps for this issue are as follows

  1. Type something
  2. Press the send icon
  3. Right after pressing the send icon, start typing again quickly without any break. The compose box will still have the text you just sent

You should be able to repro this on Android too.

mountiny commented 1 year ago

@dhairyasenjaliya Looking at the issue you reported in Slack, it is not the same as this one, but I will ping our QA team to check if they can reproduce reliably and if so, we would create a new issue for it 🙌

dhairyasenjaliya commented 1 year ago

@rushatgabhane yeah I think this issue is also different than I thought :)

dhairyasenjaliya commented 1 year ago

@dhairyasenjaliya Looking at the issue you reported in Slack, it is not the same as this one, but I will ping our QA team to check if they can reproduce reliably and if so, we would create a new issue for it 🙌

Thank you for the follow-up indeed it's another issue that just got posted in git :)

mountiny commented 1 year ago

Correct, the QA team log it for us. Thanks for keeping na eye on this 🙌

rushatgabhane commented 1 year ago

awaiting proposals!

rushatgabhane commented 1 year ago

@dylanexpensify I'd suggest that we double this issue because it's an annoying bug on arguably the most important component (compose bar).

Judging by 100+ comments in https://github.com/Expensify/App/issues/4129, the solution shouldn't be straightforward either

dylanexpensify commented 1 year ago

You got it @rushatgabhane! Thanks for the input!

MonilBhavsar commented 1 year ago

Platform: Where is this issue occurring?

iOS Android

@mvtglobally this issue mentions that issue is reproducible on Android. Could you please retest and confirm if this issue occurs on Android too. Thanks!

dylanexpensify commented 1 year ago

Price doubled

MonilBhavsar commented 1 year ago

@mvtglobally this issue mentions that issue is reproducible on Android. Could you please retest and confirm if this issue occurs on Android too. Thanks!

@mvtglobally please. In case you missed this.

dylanexpensify commented 1 year ago

Bump @mvtglobally

mdneyazahmad commented 1 year ago

This issue is reproducible in Android.

rushatgabhane commented 1 year ago

yep, it's android too.

@dylanexpensify time to double?

dylanexpensify commented 1 year ago

time to double!

dylanexpensify commented 1 year ago

Doubled twice since I somehow missed the prior double

dylanexpensify commented 1 year ago

Doubling again

rushatgabhane commented 1 year ago

@dylanexpensify I think we should pin 📌 this ticket on the issues page

dylanexpensify commented 1 year ago

Good thinking @rushatgabhane !

vladamx commented 1 year ago

Proposal

The reason why the input is not clearing is because of busy JS Thread and asynchronous nature of React Native.

After user does a submit a lot of JS state updates which creates reconciliation work and locks the JS Thread.

In the meantime, user updates the input on the main thread,since we are interacting with a native text input view, but the problem is that it can not notify JS thread and thus ignores all previous actions done to the field (in this case clear)

setTextAndSelection is what really gets called on clear Notice the condition on eventLag

Screenshot 2022-09-15 at 12 33 36

Also look at what gets sent over the bridge in the video below https://we.tl/t-CyPTYk88SP

The solution would be to minimize number of renders(state updates) with the restructure of the report screen(in part or in full), memoization, lazyness etc.

There are multiple strategies for solving perf problems on JS side.

rushatgabhane commented 1 year ago

@vladamx thanks for your investigation! In that case Fabric would help solve this issue right? https://reactnative.dev/architecture/fabric-renderer

We already have a ticket for enabling Fabric over here - https://github.com/Expensify/App/issues/8503

vladamx commented 1 year ago

Hi @rushatgabhane

Generally speaking, yes. Fabric allows synchronous updates to native views.

Now, when or how it will be applied to inputs is unknown and adoption of the new architecture is big topic, especially in projects where there are a lot of external dependencies which are not yet updated.

Anyhow, this issue is just a consequence of a greater problem which is slow ui updates. This will inevitably(with fabric or not) manifest in other ways on slower devices even if clear worked fine.

I would recommend being careful where the state lives and how often the views are updated.