search

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.36k stars 2.78k forks source link

CRITICAL: Desktop - Chat Switcher is slow and doesn't recognize keystrokes #28071

Closed kbecciv closed 2 months ago

kbecciv 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 a message in a chat
  2. Type command+k to open chat switcher
  3. Immediately start typing the name of a person you already have chats with

Expected Result:

The characters you type show up in search

Actual Result:

Many characters are missed. ie. I typed lauren quickly and only the n showed.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.73.0 Reproducible in staging?: n/a Reproducible in production?: n/a 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

n/a

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011dba4ca10589e48c
  • Upwork Job ID: 1707643423922569216
  • Last Price Increase: 2023-10-17
Issue OwnerCurrent Issue Owner: @mallenexpensify
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~011dba4ca10589e48c

stephanieelliott commented 1 year ago

Adding relevant labels

melvin-bot[bot] commented 1 year ago

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

studentofcoding commented 1 year ago

Proposal

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

Desktop - Chat Switcher is slow and doesn't' recognize keystrokes

What is the root cause of that problem?

On src/pages/SearchPage.js we have set a debounce of 75ms this.debouncedUpdateOptions = _.debounce(this.updateOptions.bind(this), 75); meanwhile we set focus to the Input (BaseOptionsSelector.js) after 300ms on CONST.ANIMATED_TRANSITION.. This condition makes the race condition, that if the user types quickly (before the the Input isFocused) then some keystrokes will be missed.

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

  1. Change CONST.ANIMATED_TRANSITION to less than this.debouncedUpdateOptions time (in the demo below I set ANIMATED_TRANSITION = 50)

  2. We should move the call this.debouncedUpdateOptions() inside the callback function of setState(), and add the debouncedUpdateOptions more time than CONST.ANIMATED_TRANSITION, to ensure that it'll only called after ANIMATED_TRANSITION on Search Input is done & after the state update.

onChangeText(searchValue = '') {
    this.setState({searchValue}, () => {
        this.debouncedUpdateOptions();
    });
}

Note: as ANIMATED_TRANSITION is being used on another component as well, we have to decide the final value of it.

Result

https://github.com/Expensify/App/assets/20473526/3ab8b0e6-1071-46c2-b44e-c2b7a88a464b

What alternative solutions did you explore? (Optional)

None

@cubuspl42 @stephanieelliott

melvin-bot[bot] commented 12 months ago

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

cubuspl42 commented 12 months ago

@studentofcoding

This means that if the user types quickly, some keystrokes will be missed because the state update is debounced and does not happen immediately, when onChangeText function is called

Could you elaborate? I understand why it does not happen immediately, but how does debouncing explain the missing keystrokes?

studentofcoding commented 12 months ago

@studentofcoding

This means that if the user types quickly, some keystrokes will be missed because the state update is debounced and does not happen immediately, when onChangeText function is called

Could you elaborate? I understand why it does not happen immediately, but how does debouncing explain the missing keystrokes?

Based on my observations the reason is because of race conditions when debouncedUpdateOptions() is called before the state is fully updated, thus causing some keystrokes to be missed @cubuspl42.

cubuspl42 commented 12 months ago

@studentofcoding Can this race condition be observed when, for example, we add some console.log statements?

Could you provide a video with a sequence of logged events demonstrating the race? I'd like to understand better what kind of race condition we're dealing with. I believe that the solution might be working, but the root cause analysis is sometimes even more important than a fix itself.

studentofcoding commented 12 months ago

@studentofcoding Can this race condition be observed when, for example, we add some console.log statements?

Could you provide a video with a sequence of logged events demonstrating the race? I'd like to understand better what kind of race condition we're dealing with. I believe that the solution might be working, but the root cause analysis is sometimes even more important than a fix itself.

Sure @cubuspl42, let me add the demonstration tonight

stephanieelliott commented 11 months ago

Hey @studentofcoding and progress with the demonstration?

melvin-bot[bot] commented 11 months ago

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

cubuspl42 commented 11 months ago

I asked for help with figuring out this one on Slack

melvin-bot[bot] commented 11 months ago

@cubuspl42 @stephanieelliott 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!

mallenexpensify commented 11 months ago

Posted again to try to get 👀 cuz I'd love to see this bug fixed, I run into it a dozen or more times a day. https://expensify.slack.com/archives/C05LX9D6E07/p1696870380595049?thread_ts=1696418872.247919&cid=C05LX9D6E07

melvin-bot[bot] commented 11 months ago

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

stephanieelliott commented 11 months ago

Hey @studentofcoding, really interested to learn more about your theory around the race condition, can you share the video demonstrating the race?

We'd really like to get this one fixed, so doubling the bounty on this issue to $1000.

melvin-bot[bot] commented 11 months ago

Upwork job price has been updated to $1000

pradeepmdk commented 11 months ago

when I observe before input focus typing character is missing which is expected only. but after focusing there are no issues here. let me know if we are facing this issue after text focus.

studentofcoding commented 11 months ago

After another checking, The RCA I've shared only covers 30% of the problem, and the deeper root cause is found.

In that case, can I revise my proposal @stephanieelliott? If yes, the solution and demo are ready to be shown today

cubuspl42 commented 11 months ago

@studentofcoding You can update your proposal, as stated in the contributing guidelines.

studentofcoding commented 11 months ago

Sure @cubuspl42, I've updated the proposal

cc: @stephanieelliott

cubuspl42 commented 11 months ago

Well, the only issue is that you assume that we can change a constant that was (probably experimentally?) decided to be 300 ms to 50 ms, without consequences. What's the rationale here?

This is the kind of an issue where hitting a nail in one place can easily make things fall apart in another. We'll have to triple check that we're not doing exactly that.

studentofcoding commented 11 months ago

Well, the only issue is that you assume that we can change a constant that was (probably experimentally?) decided to be 300 ms to 50 ms, without consequences. What's the rationale here?

This is the kind of an issue where hitting a nail in one place can easily make things fall apart in another. We'll have to triple check that we're not doing exactly that.

I'm not assuming anything rather than, that's all the scope of the RCA, with the question for decising the constant together

And yes, that's why I shared note on the bottom of proposal that I'll make sure it's not making problem on another places (with the context that'll only related to Search Related input that get focused (after 300ms)-No data management or effect we do here)

cubuspl42 commented 11 months ago

Your root cause analysis still doesn't feel convincing to me. For example, you say:

This condition makes the race condition, that if the user types quickly (before the the Input isFocused) then some keystrokes will be missed.

  1. Change CONST.ANIMATED_TRANSITION to less than this.debouncedUpdateOptions time (in the demo below I set ANIMATED_TRANSITION = 50)

For testing purposes, I manipulated the values in this way:

ANIMATED_TRANSITION is bigger than the debounce duration, which doesn't seem to affect the solution.

I cannot see the relation between these two values, and I definitely can't see the race you're talking about.

studentofcoding commented 11 months ago

Your root cause analysis still doesn't feel convincing to me. For example, you say:

This condition makes the race condition, that if the user types quickly (before the the Input isFocused) then some keystrokes will be missed.

  1. Change CONST.ANIMATED_TRANSITION to less than this.debouncedUpdateOptions time (in the demo below I set ANIMATED_TRANSITION = 50)

For testing purposes, I manipulated the values in this way:

  • ANIMATED_TRANSITION = 50 (50 ms)
  • this.debouncedUpdateOptions = _.debounce(this.updateOptions.bind(this), 10) (10 ms)

ANIMATED_TRANSITION is bigger than the debounce duration, which doesn't seem to affect the solution.

I cannot see the relation between these two values, and I definitely can't see the race you're talking about.

I think you misunderstood my point:

In simple terms, if we type before 300ms (or ANIMATED_TRANSITION is done), then the keystroke won't catch all the characters, because when the user types it's still not focused on the Search bar Input.

Therefore the flow right now gives a race condition with the order of

  1. debouncedUpdateOptions,
  2. ANIMATED_TRANSITION,
  3. Data is rendered,

That Instead what we need is

  1. ANIMATED_TRANSITION,
  2. debouncedUpdateOptions, and
  3. Data is rendered

That's basically the reason and flow of my solution @cubuspl42

Also, your test basically just proves my point, in which you can check the after vs before to demonstrate the flow I shared above (in which I don't think it make sense to have only slight ms to check as our eyes won't catch the different)

cubuspl42 commented 11 months ago

I think you misunderstood my point

It's possible, but it's not the only explanation of our misunderstanding.

Also, we have a debouncedUpdateOptions of 75ms that will have a side effect of loading the data faster than what the user inputs on the Search bar (after 300ms)

Therefore the flow right now gives a race condition with the order of

1. debouncedUpdateOptions,

2. ANIMATED_TRANSITION,

3. Data is rendered,

How is it the case that debouncedUpdateOptions is called within these first 300 ms, when the input is not focused?

I thought that if the input is not focused, the characters can't be effectively typed-in, onChangeText can't be invoked, and in effect neither can be debouncedUpdateOptions. Could you show me a flaw in my reasoning?

melvin-bot[bot] commented 11 months ago

@cubuspl42 @stephanieelliott 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!

melvin-bot[bot] commented 11 months ago

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

cubuspl42 commented 11 months ago

The price tag has already been raised and should be enough for now.

What is expected from a good proposal here:

cubuspl42 commented 11 months ago

I advertised the issue on Slack

cubuspl42 commented 11 months ago

It was suggested on Slack that the original issue worth investigating is https://github.com/Expensify/App/issues/6069

AmjedNazzal commented 11 months ago

Proposal

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

Desktop - Chat Switcher is slow and doesn't recognize keystrokes

What is the root cause of that problem?

First of all I should correct something about this bug report, this bug is NOT exclusive to desktop and I can easily reproduce it on web chrome using the same steps

https://github.com/Expensify/App/assets/74202751/f330e9d3-e1b4-48a3-9acd-ab038ed8baeb

We are using onChangeText to update the search value, this onChangeText is passed down all the way to RNTextInput which then animates the TextInput wrapping it in AnimatedTextInput, the issue here isn't really a bug but more of how it is intended to behave, so something like onChangeText is a react event and this kind of event along with an event like onKeyPress will not be attached until the component finishes mounting and in this case finishes animating and so anything typed before the animation is done isn't being captured by the native event.

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

If debouncing the input capture even further is not desirable what we can do is make use of searchRendered in searchPage, searchRendered is attached to onLayout and if we pass onLayout to RNTextInput through BaseOptionSelector and call it in BaseTextInput then that means that searchRendered will be called upon onLayout event which in TextInput native is an event that occurs right when the component mounts, from there we can add an event listener on the window object as soon as the component mounts:

searchRendered() {
    window.addEventListener('keydown', this.handleKeyDown);

Now with that we have a listener for keydown events right when the TextInput component mounts regardless of the animation, from there we can capture any keydown events until react's own event handlers take over:

handleKeyDown(event){
    this.setState(prevState => ({ 
        tempSearchValue: prevState.tempSearchValue + event.key
    }));
};

Once onChangeText event is attached it will override the keydown event effectivly make it as if it weren't there and keydown event we added will no longer work which is good, now we can combine both search values into one and hande over the control to onChangeText's native behaviour:

onChangeText(searchValue = '') {
    ...

    if(this.state.tempSearchValue !== ''){
        this.setState({
            searchValue: this.state.tempSearchValue + searchValue,
            tempSearchValue: ''
        });
    }else {
        this.setState({searchValue}, this.debouncedUpdateOptions);
    }

lastly we would remove the event listener we added on componentWillUnmount

Result

https://github.com/Expensify/App/assets/74202751/71629caa-81c8-43f0-a946-452019d89b8f

mvtglobally commented 11 months ago

Issue not reproducible during KI retests. (First week)

melvin-bot[bot] commented 11 months 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 11 months ago

Still reproducible for me, I just typed cmd+k then Stephanie and got this

image
cubuspl42 commented 11 months ago

@AmjedNazzal Do I understand correctly that you disagree that this issue is related to the CONST.ANIMATED_TRANSITION constant?

AmjedNazzal commented 11 months ago

@cubuspl42 No I don't disagree, that duration of animation is related to my proposal, what I was trying to propose is that if manipulating the animation duration and using a debounce approach is not desirable then I was proposing an alternative solution where the animation would remain the same, again in my proposal I explain that until animations are done, react's native events won't be attached so in my opinion there is no straight forward solution as far as I can see and we could either manipulate the animation duration or we could attach a window event listener as a placeholder until react's event listeners are attached.

shubham1206agra commented 11 months ago

@cubuspl42 This is more of a performance improvement. We should not modify the CONST.ANIMATED_TRANSITION as this will break animation of screen switching.

melvin-bot[bot] commented 11 months ago

@cubuspl42 @stephanieelliott this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Thanks!

melvin-bot[bot] commented 11 months ago

Current assignee @cubuspl42 is eligible for the Internal assigner, not assigning anyone new.

cubuspl42 commented 11 months ago

@shubham1206agra

@cubuspl42 This is more of a performance improvement. We should not modify the CONST.ANIMATED_TRANSITION as this will break animation of screen switching.

Are you sure about this statement? My conclusion so far is that nothing (except maybe the title) suggests that this actually is a performance problem, even I thought that it has a performance-related aspect in the beginning.

shubham1206agra commented 11 months ago

Yes Because I tested this issue on 2 different models And the number of keystrokes missing are different

If someone gets the bug then I may be wrong but I am pretty confident that this is due to performance

shubham1206agra commented 11 months ago

@shubham1206agra

@cubuspl42 This is more of a performance improvement. We should not modify the CONST.ANIMATED_TRANSITION as this will break animation of screen switching.

Are you sure about this statement? My conclusion so far is that nothing (except maybe the title) suggests that this actually is a performance problem, even I thought that it has a performance-related aspect in the beginning.

You can't reduce the transition time as it may introduce some new problems

cubuspl42 commented 11 months ago

@shubham1206agra

Reducing ANIMATED_TRANSITION to 75 ms makes the problem disappear on my laptop. Why would it happen if this was a performance problem? Also, if this is a performance problem, then of what nature?

You can't reduce the transition time as it may introduce some new problems

Well, we definitely can do that, but I'm not sure yet whether we should. If you actually know about some part of the app that could likely break after reducing the mentioned constant, please let us know, so we can test it!

shubham1206agra commented 11 months ago

@shubham1206agra

Reducing ANIMATED_TRANSITION to 75 ms makes the problem disappear on my laptop. Why would it happen if this was a performance problem? Also, if this is a performance problem, then of what nature?

It's due to focus is called a little early.

Not sure of the nature of the performance improvement. But this may be React using some batch updates and focus is taking longer than ANIMATED_TRANSITION time (not sure)

You can't reduce the transition time as it may introduce some new problems

Well, we definitely can do that, but I'm not sure yet whether we should. If you actually know about some part of the app that could likely break after reducing the mentioned constant, please let us know, so we can test it!

Wherever we have used setTimeout without using const right now (Which is wrong). I have to double check And maybe timing was decided by the UI team. You may want to consult them before changing time

getusha commented 11 months ago

IMO reducing the ANIMATED_TRANSITION is definitely a hack and it is not fixing the root cause, additionally the issue will still be either reproducible even without the setTimeout or will cause a glitch.

cubuspl42 commented 11 months ago

So far, no proposal seems to be acceptable

@studentofcoding You didn't reply to my questions

@AmjedNazzal You seem to have focused on making your proposal different from the other, while we are seeking for a solid proposal with the best solution (or at least reasonably good...) without logical flaws

If debouncing the input capture even further is not desirable...

Before reasoning whether it's desirable, can we first determine whether it does or does not solve the problem? Because just increasing the debounce window like this...

this.debouncedUpdateOptions = _.debounce(this.updateOptions.bind(this), 375);

...doesn't seem to solve the problem, as I observe it.