DashboardVariables: Use Combobox behind toggle #1011

tskarhed · 2024-12-19T13:47:49Z

Implements variable rendering with single Combobox
Adds events for reporting changed variables
Handles custom onSearch in Combobox rather than variable state

Closes: #96099

Related to #96091 and #98261

Followup issues:

Identify custom value creation in Combobox

packages/scenes/src/variables/components/VariableValueSelect.tsx

tskarhed · 2025-01-22T14:38:54Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+
+  return (
+    <Combobox
+      id={'var-' + key}


Not sure where this var- is added for the Select

packages/scenes/src/variables/components/VariableValueSelect.tsx

tskarhed · 2025-01-31T09:51:07Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+  const onInputChange = useMemo(() => {
+    if (!model.onSearchChange) {
+      return;
+    }
+    return async (newInputValue: string) => {
+      model.onSearchChange!(newInputValue);
+
+      // Same functionality as in _updateOptionsBasedOnSearchFilter, although deboucning is managed in Combobox
+      return (await lastValueFrom(model.getValueOptions({ searchFilter: newInputValue }))).map((o) => ({
+        value: o.value.toString(),
+        label: o.label,
+      }));
+    };
+  }, [model]);


I don't think this is working properly, and I lack the insight into how value updates are made in Scenes to make an informed decision

Any reason why this looks different from the one on line 73?

packages/scenes/src/variables/variants/query/QueryVariable.tsx

tskarhed · 2025-02-03T12:13:52Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+    // If we don't use searchFilter in QueryVariable, don't bother to make a query. TODO: Proper typing
+    // @ts-ignore
+    if ('query' in model.state && !containsSearchFilter(model.state.query)) {
+      return;
+    }


Some feedback to do some proper typing on this would be nice. query only exists on a subclass. I guess this breaks the class isolation somewhat.

Would setting this in the state instead make sense? Like model.state.async or something similar, and set it in the QueryVariable constructor. By default QueryVariable only runs the initial query and the rest is filtered client side.

You can simply call the function and use the same logic we have above (for the old select )

tskarhed · 2025-02-03T12:13:53Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+      const res = model.onSearchChange!(newInputValue).then((result) => {
+        return result.map((o) => ({
+          value: o.value.toString(),
+          label: o.value.toString() === ALL_VARIABLE_VALUE ? ALL_VARIABLE_TEXT : o.label,


Unclear how All should be treated in this case.

packages/scenes/src/variables/components/VariableValueSelect.tsx

torkelo · 2025-01-31T13:33:02Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+  const onInputChange = useMemo(() => {
+    if (!model.onSearchChange) {
+      return;
+    }
+    return async (newInputValue: string) => {
+      model.onSearchChange!(newInputValue);
+
+      // Same functionality as in _updateOptionsBasedOnSearchFilter, although deboucning is managed in Combobox
+      return (await lastValueFrom(model.getValueOptions({ searchFilter: newInputValue }))).map((o) => ({
+        value: o.value.toString(),
+        label: o.label,
+      }));
+    };
+  }, [model]);


Any reason why this looks different from the one on line 73?

torkelo · 2025-01-31T13:37:24Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+      return (await lastValueFrom(model.getValueOptions({ searchFilter: newInputValue }))).map((o) => ({
+        value: o.value.toString(),
+        label: o.label,
+      }));


you can remove this part

torkelo · 2025-02-03T13:52:29Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+    // If we don't use searchFilter in QueryVariable, don't bother to make a query. TODO: Proper typing
+    // @ts-ignore
+    if ('query' in model.state && !containsSearchFilter(model.state.query)) {
+      return;
+    }


You can simply call the function and use the same logic we have above (for the old select )

torkelo · 2025-02-03T13:53:08Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+      return;
+    }
+
+    return async (newInputValue: string) => {


Think you can remove this is you simply called the function, like the current impl

torkelo · 2025-02-03T14:02:55Z

packages/scenes/src/variables/variants/query/QueryVariable.tsx

+      return this._getNewOptions(searchFilter);
+    }
+
+    // Undefined for first call


What is undefined?

THe return value. This is because of the debounce. It can be fixed by setting { leading: true }

torkelo

It seems hang / crash a browser when there are many options (many values) returned by the variable query:

In this demo example:
http://localhost:3000/a/grafana-scenes-app/demos/variables/many-values?var-interval=1d&from=now-6h&to=now&timezone=browser&var-server=AA&var-pod=AAA&var-handler=pod&var-keyValue=1&var-queryVar=A&var-namespace=broken&__feature.templateVariablesUsesCombobox&var-manyOptions=CEeAKU5LETNWXReRPQY7IddhyHDK5eYQi2kdATodj6m4AYPHJ8

Tested this a bit now with a prometheus variable query that uses __searchFilter,

With this query:

  new QueryVariable({
                  name: 'namespace',
                  query: {
                    qryType: 1,
                    query: 'label_values({__name__=~"${__searchFilter}.*"},__name__)',
                    refId: 'PrometheusVariableQueryEditor-VariableQuery',
                  },
                  datasource: { uid: 'se-demo' },
                }),

The call to onSearchChange and getValueOptions causes double queries (at least on first open).

Dan we really need to move the loading state from the label to input as well causing the input to jump around (not related to this PR, just an observation)

tskarhed · 2025-02-04T09:32:37Z

It seems hang / crash a browser when there are many options (many values) returned by the variable query:

There is a possibility this is fixed with the latest changes to Combobox. This seems to only be an issue when first opened...
Something is creating event listeners, as they are close to 100k, looking into it.

tskarhed · 2025-02-04T16:26:47Z

It seems hang / crash a browser when there are many options (many values) returned by the variable query:

In this demo example: http://localhost:3000/a/grafana-scenes-app/demos/variables/many-values?var-interval=1d&from=now-6h&to=now&timezone=browser&var-server=AA&var-pod=AAA&var-handler=pod&var-keyValue=1&var-queryVar=A&var-namespace=broken&__feature.templateVariablesUsesCombobox&var-manyOptions=CEeAKU5LETNWXReRPQY7IddhyHDK5eYQi2kdATodj6m4AYPHJ8

A ton of event listeners are being created. I have no idea why. The basic use case is handled without any issues directly in Combobox.

torkelo · 2025-02-05T07:34:30Z

@tskarhed yea, it's strange, the combobox looks to recursive be rendering itself or something.

thought it has something to do with passing options via the onInputChange but commented and it still hangs the same way

tskarhed · 2025-02-05T15:26:47Z

The issue is that when the options are loaded the list virtualization isn't on somehow. No idea why. Maybe I'll test directly with 10k+ items in a dashboard. This issue doesn't appear in Prometheus or Storybook.

tskarhed · 2025-02-06T12:49:29Z

I still don't know exactly why this issue appears here and nowhere else. However, it seems like the initial render left the maxHeight undefined, hence the list virtualization never kicked in. I fixed this by setting arbitrary initial dimensions (which are overridden soon after measuring things).

grafana/grafana#100188

tskarhed · 2025-02-06T13:10:42Z

With this query:

  new QueryVariable({
                  name: 'namespace',
                  query: {
                    qryType: 1,
                    query: 'label_values({__name__=~"${__searchFilter}.*"},__name__)',
                    refId: 'PrometheusVariableQueryEditor-VariableQuery',
                  },
                  datasource: { uid: 'se-demo' },
                }),

The call to onSearchChange and getValueOptions causes double queries (at least on first open).

Posting this recording for reference.

Screen.Recording.2025-02-06.at.14.09.04.mov

It doesn't seem to happen when having an empty value. I'm looking into it.

tskarhed · 2025-02-06T16:43:35Z

@torkelo A call in onOpen in Combobox isn't debounced. A PR is open for it now grafana/grafana#100219

Other than that, is there anything blocking this PR, or do you feel okay approving it so we can start testing it in our instances? I assume the debounce PR will be in order soon enough.

Co-authored-by: Josh Hunt <[email protected]>

torkelo · 2025-02-07T07:46:19Z

packages/scenes/src/variables/components/VariableValueSelect.tsx

+      //@ts-ignore
+      model.onSearchChange && 'query' in model.state && containsSearchFilter(model.state.query),


We just need to fix this now, and the empty promise return of onSearchChange.

wonder how though, need to align this impl with the other and change how options are returned a bit

How about adding an isAsync getter on MutliVariableValue that defaults to false, but for QueryVariable has this statement?

Although it is a bit misnamed, as QueryVariable always makes at least one async request.

@tskarhed yea, we need a true async mode for QueryVariable where it will always have a default value, but needs some bigger changes

Template Variables: Use Combobox behind toggle

0e91863

tskarhed mentioned this pull request Dec 19, 2024

DashboardVariables: Use Combobox behind feature flag grafana/grafana#98261

Merged

3 tasks

Variables: Add renderWithCombobox prop

775d397

Sergej-Vlasov reviewed Jan 17, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

tskarhed and others added 2 commits January 22, 2025 15:36

Variables: Improve Combobox implementation

6b590f4

Merge branch 'main' into tskarhed/combobox-template-variable

19dcedc

tskarhed commented Jan 22, 2025

View reviewed changes

tskarhed added 2 commits January 22, 2025 15:44

Restore dependencies from main

5878c48

Restore files from main

cf583bd

tskarhed marked this pull request as ready for review January 22, 2025 15:28

tskarhed requested a review from Sergej-Vlasov January 22, 2025 15:28

tskarhed commented Jan 24, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

tskarhed commented Jan 27, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

Add loading state and memozie options

fd4e249

tskarhed changed the title ~~Template Variables: Use Combobox behind toggle~~ DashboardVariables: Use Combobox behind toggle Jan 30, 2025

tskarhed added 2 commits January 30, 2025 13:27

Merge branch 'main' into tskarhed/combobox-template-variable

c2007dc

VariableValueSelect: Support onSearchCahnge for Combobox

08e1d66

tskarhed commented Jan 30, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

Add reporting for value selection

b816567

torkelo reviewed Jan 31, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

tskarhed added 4 commits January 31, 2025 10:04

Identify ALL option for multi

c299893

Cleanup

0acb219

Use feature toggle directly from the config

80c2256

Make tests pass from config check

332e698

tskarhed commented Jan 31, 2025

View reviewed changes

tskarhed added 5 commits January 31, 2025 11:40

Report if combobox

5f14e95

Track custom value

c9b9f26

Remove comment

1832cf8

Update custom value check

59d1508

Fix async

a59696b

tskarhed added 3 commits February 3, 2025 12:17

Add comment

427ccf5

Fix async

8b8f6d4

Fix failing test

2f34215

tskarhed commented Feb 3, 2025

View reviewed changes

Remove leading from debounce

6c7ed5f

tskarhed commented Feb 3, 2025

View reviewed changes

packages/scenes/src/variables/components/VariableValueSelect.tsx Outdated Show resolved Hide resolved

Use const for ALL

78a6c9d

torkelo reviewed Feb 3, 2025

View reviewed changes

torkelo reviewed Feb 4, 2025

View reviewed changes

tskarhed mentioned this pull request Feb 6, 2025

Combobox: Fix list not being virtualized initially in some cases grafana/grafana#100188

Merged

3 tasks

tskarhed mentioned this pull request Feb 6, 2025

Combobox: Debounce async requests onOpen grafana/grafana#100219

Closed

3 tasks

Refactor functions for options

07eaa71

Co-authored-by: Josh Hunt <[email protected]>

torkelo reviewed Feb 7, 2025

View reviewed changes

Add isAsync getter and placeholder

0002681

		//@ts-ignore
		model.onSearchChange && 'query' in model.state && containsSearchFilter(model.state.query),

DashboardVariables: Use Combobox behind toggle #1011

Are you sure you want to change the base?

DashboardVariables: Use Combobox behind toggle #1011

Uh oh!

Conversation

tskarhed commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tskarhed Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torkelo left a comment

Choose a reason for hiding this comment

Uh oh!

tskarhed commented Feb 4, 2025

Uh oh!

tskarhed commented Feb 4, 2025

Uh oh!

torkelo commented Feb 5, 2025

Uh oh!

tskarhed commented Feb 5, 2025

Uh oh!

tskarhed commented Feb 6, 2025

Uh oh!

tskarhed commented Feb 6, 2025

Uh oh!

tskarhed commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tskarhed commented Dec 19, 2024 •

edited

Loading

tskarhed Feb 3, 2025 •

edited

Loading