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

Server Side Search Deletes Search Text #1081

Closed
mhmcdonald opened this issue Aug 6, 2022 · 19 comments · Fixed by #1348 · May be fixed by #1332
Closed

Server Side Search Deletes Search Text #1081

mhmcdonald opened this issue Aug 6, 2022 · 19 comments · Fixed by #1348 · May be fixed by #1332
Labels
wontfix This will not be worked on

Comments

@mhmcdonald
Copy link

Describe the bug
I've noticed that when I use server side search and type in search text at a reasonable pace, some of the most recent text will get deleted. This behavior can be seen in this gif. This results in mistakes while entering your search phrase because the text you're typing in gets changed so easily/unexpectedly.

I've noticed this bug on my own projects as well as on the server-side search example on the grid.js documentation site.

To Reproduce
Steps to reproduce the behavior:

  1. Go to the server side search example on grid.js website: https://gridjs.io/docs/examples/server-side-search/
  2. Scroll down to the search box
  3. Type in the word 'empire' at a normal typing pace (if you type very slow or very fast you may not be able to reproduce this issue).
  4. Notice how the last letter (or couple of letters) that you've typed in gets deleted from the search box. Similarly, if you've made a mistake and you click delete on your key board, your delete may not take effect. This results in incorrect search terms being entered into the search box.

Expected behavior
The server-side search feature returns filtered table records as soon as you start typing. The text that I type into the search box should not be altered/edited.

Screenshots
You can see this bug in this gif.

In this gif, I'm typing in the word 'empire' into the search bar. After typing in the first three letters, 'emp' the table returns results for 'em'. However the in the search box, the 'p' has been deleted from my search text (you can see the 'emp' for a split second before the 'p' gets removed). It accepts my fourth letter typed in ('i') so then I have the incorrect search text of 'emi'. Towards the end of the gif you can see I have the same buggy experience trying to type in the letter 'r' (it gets deleted from the search text box very quickly after I've typed it in).

Desktop (please complete the following information):

  • OS: Monterey
  • Browser: chrome
  • Version: 103.0.5060
@abitbetterthanyesterday
Copy link
Collaborator

abitbetterthanyesterday commented Aug 15, 2022

@mhmcdonald Same issue here.

There's is a 'racing' condition when typing:

Typing triggers a server call.
Depending on how fast/slow it responds, you will experience the issue.

This does not occur when typing fast since the debouncing takes care of it.
If typing 'slow', then the server has the time to respond, and overwrite the current input value.

Not that if you have a slow connection, it would make the matter even worse - as in you will have to type fast, or very slow.

But if you hit the 'wrong' speed when typing, it gets out of hand 😃

Notice on the video the server calls on the right. When they return, they override the input value.

Note: I'm typing 'hope' and then 'keyword'

grid-js-server-search-issue.mp4

I believe the way to work around that would be to decouple the searching from the rendering.

I'm happy to open a PR for that, I found the issue, just give me a day or two 👍

@abitbetterthanyesterday
Copy link
Collaborator

abitbetterthanyesterday commented Aug 16, 2022

Made a bit of progress here - not as much as expected.
The library is heavily relying on inheritance and events, it took me a while to find my way around 😄

So, it turns out it is an issue related to the way we debounce the user input - there might be no racing conditions with the server as I initially thought.

As a quick fix, you can disable the debouncing until I fix this properly:
src > view > plugin > search > line 102 > comment out : onInput = debounce(onInput, this.props.debounceTimeout);

This will get rid of the bug.
However, depending on your use case you might overload the server as it disables any debouncing on the user input.

I'll try to keep you updated soon once I've done more digging 🔧 👍

@mhmcdonald
Copy link
Author

Thanks for the update @Aloysb - I really appreciate you digging into this

@abitbetterthanyesterday
Copy link
Collaborator

abitbetterthanyesterday commented Aug 17, 2022

There you go! 🍻 @mhmcdonald

Feel free to pull on that branch until the PR is merged.

It seems that sometimes the answer is just KISS Keep it simple, stupid.
Look at how small the code changes I've made are 😄

There is no need to have the search input controlled (at least, at this stage) - and it does create a racing condition.

Rather than having the current input and the current search stored separately, I've simply turned the input into a plain ol' uncontrolled component since it doesn't HAVE to be controlled and ... tada! 🎉 no more racing condition 👍

@mhmcdonald
Copy link
Author

Amazing, thank you @Aloysb! You rock! I will be watching the PR.

@abitbetterthanyesterday
Copy link
Collaborator

Hey @mhmcdonald ,

Just to let you know that we'll aim to get this reviewed soon, maybe next week 👍

@mhmcdonald
Copy link
Author

thanks again for the update @Aloysb!

@nitinmalik1997
Copy link

There you go! 🍻 @mhmcdonald

Feel free to pull on that branch until the PR is merged.

It seems that sometimes the answer is just KISS Keep it simple, stupid. Look at how small the code changes I've made are 😄

There is no need to have the search input controlled (at least, at this stage) - and it does create a racing condition.

Rather than having the current input and the current search stored separately, I've simply turned the input into a plain ol' uncontrolled component since it doesn't HAVE to be controlled and ... tada! 🎉 no more racing condition 👍

Could you please elaborate how to turn the input into a plain of uncontrolled component?

@abitbetterthanyesterday
Copy link
Collaborator

abitbetterthanyesterday commented Oct 2, 2022 via email

@hrvoje-grabusic
Copy link

hrvoje-grabusic commented Nov 26, 2022

But if you type at the wrong speed, you create this racing condition.

Increasing the wrong speed is quick fix from the config side meanwhile.
debounceTimeout:1000,
Works for me.

@newmediacrew
Copy link

But if you type at the wrong speed, you create this racing condition.

Increasing the wrong speed is quick fix from the config side meanwhile. debounceTimeout:1000, Works for me.

Where did you set the value of debounceTimeout:1000 ?

@hrvoje-grabusic
Copy link

hrvoje-grabusic commented Nov 26, 2022

Where did you set the value of debounceTimeout:1000 ?

In the search block
search: { debounceTimeout:1000, server: { url: (prev, keyword) =>${prev}?search=${keyword} } },

It's in the docs:
gridjs.io/docs/config/search

@newmediacrew
Copy link

Where did you set the value of debounceTimeout:1000 ?

In the search block search: { debounceTimeout:1000, server: { url: (prev, keyword) =>prev?search={keyword} } },

It's in the docs: gridjs.io/docs/config/search

Just a matter for RTFM, thank you for that.

@afshinm
Copy link
Member

afshinm commented Jan 10, 2023

#1267 can potentially fix this issue. Can someone run some tests on that branch please?

@mnsrulz
Copy link

mnsrulz commented Feb 15, 2023

#1267 can potentially fix this issue. Can someone run some tests on that branch please?

The issue still persists.

@mnsrulz
Copy link

mnsrulz commented Mar 8, 2023

@abitbetterthanyesterday i feel like your solution is optimal but there's a small catch which is not handled and that's why we have to keep the search keyword in the state. Like if you want to pass an initial value to the grid than your solution would not honor that and behave inappropriately. I have a small fix for that which I am thinking to create a PR. It's more like persist the input keyword in the state using useState and in addition to that call the debounce method of the text input as is.

@afshinm let me know your thoughts on the solution.

@bruno-buiret
Copy link

I'm interested in this, I'll increase the debounce timeout for now but a better solution is appreciated, thanks to everyone involved!

@adamhwang
Copy link
Contributor

adamhwang commented Apr 2, 2023

This happens for client side as well with a reasonable amount of data (~800 total rows, 100 rows/page, 10+ cols)

ETA: Not sure if the client side problem is related to the same root cause as the server side, but it seems to be mostly mitigated by removing the debounce altogether:

search={{
  debounceTimeout: 0,
}}

I think the "next event cycle" is still causing a bit of an issue with removing text if you type really fast, but its mostly fine now. Ideally we can no-op the debounce if the entire grid is loaded in the client

@stale
Copy link

stale bot commented Jun 10, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Jun 10, 2023
@stale stale bot closed this as completed Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
9 participants