AdHocFiltersVariable: Performance improvements

[ashharrison90]

• marks variable value select virtualized
• makes the options and input state controlled outside of select for better performance
  • extension of https://github.com/grafana/scenes/pull/763 for adhoc filters
  • also means we can revert https://github.com/grafana/grafana/pull/87843 as this is cleaner
• adds a demo page

📦 Published PR as canary version: 4.27.0--canary.766.9398430196.0

:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes-react@4.27.0--canary.766.9398430196.0 npm install @grafana/scenes@4.27.0--canary.766.9398430196.0 # or yarn add @grafana/scenes-react@4.27.0--canary.766.9398430196.0 yarn add @grafana/scenes@4.27.0--canary.766.9398430196.0 ```

[dprokop]

hahah, was just talking about this :D

[dprokop]

@ashharrison90 , i've noticed a visual regression with this PR:

Before:

After:

Mind the difference of the options being passed to the Select component, seems like this implementation adds an empty option.

[dprokop]

Also, another thing I see (in the provided demo) is perceivable performance difference between thevariables many options demo vs filters many options demo (turn the sound on for commentary):

https://github.com/grafana/scenes/assets/2376619/b72e3f6e-cabc-49d9-819f-209fbd5a77aa

[ashharrison90]

@ashharrison90 , i've noticed a visual regression with this PR:

Before:

After:

Mind the difference of the options being passed to the Select component, seems like this implementation adds an empty option.

ok this is due to getOptionSearcher adding the value to the options array if it doesn't exist in the options. maybe @torkelo knows if there's a reason why this is necessary? my guess is that without it nothing would show in the dropdown, but i think it's better to provide a custom isValidNewOption={(inputValue) => inputValue.trim().length > 0}. i've made that change anyway, can always undo if necessary.

Also, another thing I see (in the provided demo) is perceivable performance difference between thevariables many options demo vs filters many options demo (turn the sound on for commentary):

Screen.Recording.2024-06-04.at.16.30.19.mov

this isn't that surprising tbh 😅 the adhoc filter has to do more work. it always fetches options onMenuOpen, and it has to loop over the entire options array to convert from a SelectableValue to a VariableValueOption. i think that conversion is pretty fast, but when there's 10,000 options it adds up!

i've made the limit in getOptionSearcher configurable and lowered it to 1000 in AdHocFilterRenderer which seems to have helped a fair bit. think 1000 options in a select is still more than enough 😅 i've left it at 10,000 elsewhere for now, but we could probably make that change across the board

[torkelo]

@ashharrison90

ok this is due to getOptionSearcher adding the value to the options array if it doesn't exist in the options. maybe @torkelo knows if there's a reason why this is necessary? my guess is that without it nothing would show in the dropdown, but i think it's better to provide a custom isValidNewOption={(inputValue) => inputValue.trim().length > 0}. i've made that change anyway, can always undo if necessary.

I am not familiar with isValidNewOption, what does it do? no sure how it relates to showing the current value to the options? Reading the docs it seems unrelated.

But yes, adding the current value to the options I think is to make it show the current value

[ashharrison90]

@ashharrison90

ok this is due to getOptionSearcher adding the value to the options array if it doesn't exist in the options. maybe @torkelo knows if there's a reason why this is necessary? my guess is that without it nothing would show in the dropdown, but i think it's better to provide a custom isValidNewOption={(inputValue) => inputValue.trim().length > 0}. i've made that change anyway, can always undo if necessary.

I am not familiar with isValidNewOption, what does it do? no sure how it relates to showing the current value to the options? Reading the docs it seems unrelated.

But yes, adding the current value to the options I think is to make it show the current value

@torkelo the default isValidNewOption provided by react-select will not show the Use custom value: <customValue> option for the current custom value. but if you override it and just say any non-empty string is fine, it will show that option which i think negates the need for manually adding that custom option to the options array.

[torkelo]

@ashharrison90 I tried it, but could not get it to work on a non multi select. If I set isValidNewOption={() => true} and set value to a custom (non existing value) the select just shows the placeholder "Select value". But seems to work for multi select

[ashharrison90]

it's working for me on both the Many variable options and Many adhoc variable values demo pages on this branch:

[torkelo]

@ashharrison90 I tested i again on this branch and it's still not working, it does not show custom values set from URL. it seems to work just after creating a value, but it does not accept an existing value that set before mount.

Try this URL http://localhost:3000/a/grafana-scenes-app/demos/variables/many-values?var-interval=1d&var-server=AA&var-keyValue=1&var-queryVar=A&var-pod=AAA&var-handler=AAAA&var-manyOptions=custom

[dprokop]

@ashharrison90 I tested i again on this branch and it's still not working, it does not show custom values set from URL. it seems to work just after creating a value, but it does not accept an existing value that set before mount.

I observe this as well, but it works fine for the AdHocFilters (see this link http://localhost:3000/a/grafana-scenes-app/demos/variables/many-adhoc-values?var-manyAdhocOptions=a,A%7C%3D%7CusingACustom)

[ashharrison90]

@torkelo @dprokop yeah gotcha, working on it now 👍

[ashharrison90]

@torkelo @dprokop ok seems like if you pass a SelectableValue instead of just a plain string it displays correctly 😕 haven't dug into why exactly yet.

think i've also fixed the performance with the adhoc filter now as well - i was missing the no op filterOption. with that the performance seems much more in line with the normal VariableValueSelect

[torkelo]

@ashharrison90 nice!

[dprokop]

@ashharrison90 awesome, testing now

[grafanabot]

:rocket: PR was released in v4.27.0 :rocket: