-
Notifications
You must be signed in to change notification settings - Fork 75
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
chore(ui): debounce server requests while typing in filter bar #3470
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=de7cdad5d1fc3db3c562d7e68177d3997dc704a6 |
|
||
const debouncedSetFilterModel = useMemo( | ||
() => | ||
_.debounce((newModel: GridFilterModel) => setFilterModel(newModel), 300), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and I would increase the rate to more like 1 sec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that adds 1 second of latency to every user though, and 99% probably aren't running into this perf stuff. what about 500ms? @tssweeney
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think 500 is fine, sure. I general this live-filter effect is pretty rare in most applications. Instead there is a moment of "applying" the filter b/c this is so expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah true, i'll get the featurebee and you can play with 500
const newModel2 = {...filterModel, items: [item]}; | ||
setFilterModel(newModel2); | ||
const newModel2 = {...localFilterModel, items: [item]}; | ||
setLocalFilterModel(newModel2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would instead create a callback: updateFilterModel
which internally sets the "local" working copy AND calls the debounced one. Seems weird to have the dual call here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this feels wrong, but it would only ever be used in this one place, the other times we setLocalFilterModel
we want to either not update the original filter or directly set it instead of using the debounced.
I will try to appease the style gods while not being verbose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with comments
Description
WB-22888
Debounce typing in the trace table filter bar.
Testing
Prod:
Branch: