GroupBy: Fetch options when opening menu #687

ashharrison90 · 2024-04-10T13:37:39Z

i think this does what we want, but leaving as draft for now to check the approach before adding tests etc

what this does:

takes the rendering logic from VariableValueSelectMulti
enhances with isFetchingOptions and isOptionsOpen logic controlled by onOpenMenu/onCloseMenu
fetches and waits for new options when opening the menu via model.validateAndUpdate()

some questions:

does this logic only apply to group by, or would we want it for any multi value variable?
- if so, could move this new logic back into VariableValueSelectMulti 🤔

Fixes #686

📦 Published PR as canary version: 4.11.2--canary.687.8737546972.0

✨ Test out this PR locally via:

npm install @grafana/scenes@4.11.2--canary.687.8737546972.0
# or 
yarn add @grafana/scenes@4.11.2--canary.687.8737546972.0

…nactive runners

dprokop · 2024-04-17T14:48:08Z

@ashharrison90 - I've made a couple of changes to this PR. In particular:

I've added isLazy to SceneVariableState. Thanks to that and the change in SceneVariableSet the GroupBy values are not fetched when the picker renders. Overall this property needs to be added to other variables as well, but this is a bit bigger endeavour and won't address it here. We are tracking it in Variables: Allow lazy loading of variable values #214
I've added tests to the code.
I've also included a followup fix to Filters/GroupBy: Resolve queries only from active query runners #685 as that one did not respect the fact, that there may be inactive runners that we still want the queries from- i.e. for panels below the fold. Overall, I have implemented sort of core dashboard specific code, that ignres inactive runners with the same key. This reflects a scenario when we clone a panel in panel edit - the key of the SceneQueryRunner is the same, but the original one is not active. It's probably not the most elegant solution, but nothing else came to my mind - I'm open for any other ideas.

@torkelo - I would appreciate your eyes on this too, as I'm touching on the isLazy we discussed and introducing it in the interface for the future use.

dprokop · 2024-04-18T08:09:31Z

packages/scenes/src/variables/types.ts

@@ -14,6 +14,7 @@ export interface SceneVariableState extends SceneObjectState {
  loading?: boolean;
  error?: any | null;
  description?: string | null;
+  isLazy?: boolean;


Thinking about putting this in the SceneVariable interface rather than in the SceneVariableState, don't think that would ever be needed to dynamically modify the nature of a variable during runtime. Thoughts?

@dprokop would we not need it in state (to make it an persisted & UI option for QueryVariable?

Yes, it would, but... I think we can lift it to the public variable API (SceneVariableState) when we work on the general problem of variables lazy loading (ref #214). I'm afraid putting it in the state right now might be confusing for consumers, as this is now an exclusive feature of Group By. I.e. configuring Query variable as lazy now makes it not usable.

torkelo · 2024-04-18T09:13:36Z

packages/scenes/src/variables/sets/SceneVariableSet.ts

@@ -67,7 +67,7 @@ export class SceneVariableSet extends SceneObjectBase<SceneVariableSetState> imp

    // Add all variables that need updating to queue
    for (const variable of this.state.variables) {
-      if (this._variableNeedsUpdate(variable)) {
+      if (!variable.state.isLazy && this._variableNeedsUpdate(variable)) {


can we move this !variable.state.isLazy condition to _variableNeedsUpdate ?

Yes, I think we can. I initially thought this is called in update code paths, but it's called only for variables initialisation so it's OK to move it there. Addressed in 4a5f0db

torkelo · 2024-04-18T09:15:50Z

packages/scenes/src/variables/groupby/GroupByVariable.tsx

+  const { value, key, maxVisibleValues, noValueOnClear } = model.useState();
+  const arrayValue = useMemo(() => (isArray(value) ? value : [value]), [value]);
+  const [isFetchingOptions, setIsFetchingOptions] = useState(false);
+  const [isOptionsOpen, setIsOptionsOpen] = useState(false);
+
+  // To not trigger queries on every selection we store this state locally here and only update the variable onBlur
+  const [uncommittedValue, setUncommittedValue] = useState(arrayValue);
+
+  // Detect value changes outside
+  useEffect(() => {
+    setUncommittedValue(arrayValue);
+  }, [arrayValue]);
+
+  const onInputChange = model.onSearchChange
+    ? (value: string, meta: InputActionMeta) => {
+        if (meta.action === 'input-change') {
+          model.onSearchChange!(value);
+        }
+      }
+    : undefined;
+
+  const placeholder = 'Select value';
+
+  return (


Any specific reason not doing this in the renderSelectForVariable? limit potential for bugs?

Similar to my comment above - i think we need to tackle this with #214 as currently the selects rendered for variables do not have onOpenMenu handler implemented. I've tried to change it so thast it uses the renderSelectForVariable but this is a bigger effort that we need to address both in SceneVariableSet and the UI for variables.

but this is a bigger effort that we need to address both in SceneVariableSet and the UI for variables.

what more changes are needed in SceneVariableSet and UI for variables?

ashharrison90

feels weird reviewing a PR where i'm marked as author 😂

think this looks good 👍

bfmatei

lgtm 🚀

grafanabot · 2024-04-22T09:16:56Z

🚀 PR was released in v4.11.2 🚀

ashharrison90 · 2024-04-22T10:23:10Z

delivered in core with grafana/grafana#86671

ashharrison90 added 2 commits April 10, 2024 14:19

refetch options onMenuOpen

ec1ce85

Merge branch 'main' into ash/686

915ad1c

ashharrison90 requested a review from dprokop April 10, 2024 13:37

ashharrison90 self-assigned this Apr 10, 2024

dprokop requested a review from torkelo April 15, 2024 10:49

dprokop added 4 commits April 17, 2024 14:40

Merge remote-tracking branch 'origin/main' into ash/686

0b742c6

Add basic interface for lazy config of a variable

9005f19

Make GroupBy variable a lazy one

89ed49b

Resolve queries passed to group by and filters correctyl, respecing i…

a9e545f

…nactive runners

Type check

da72dd4

dprokop marked this pull request as ready for review April 17, 2024 14:51

dprokop requested a review from bfmatei April 17, 2024 14:51

dprokop added patch Increment the patch version when merged release Create a release when this pr is merged labels Apr 17, 2024

dprokop reviewed Apr 18, 2024

View reviewed changes

torkelo reviewed Apr 18, 2024

View reviewed changes

ashharrison90 commented Apr 18, 2024

View reviewed changes

Review

4a5f0db

dprokop requested a review from torkelo April 18, 2024 12:06

torkelo approved these changes Apr 19, 2024

View reviewed changes

bfmatei approved these changes Apr 19, 2024

View reviewed changes

ashharrison90 merged commit f0214ee into main Apr 22, 2024
3 checks passed

ashharrison90 deleted the ash/686 branch April 22, 2024 09:12

grafanabot added the released label Apr 22, 2024

torkelo mentioned this pull request Apr 23, 2024

GroupByVariable: Sync label to URL #705

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GroupBy: Fetch options when opening menu #687

GroupBy: Fetch options when opening menu #687

ashharrison90 commented Apr 10, 2024 •

edited by grafanabot

dprokop commented Apr 17, 2024

dprokop Apr 18, 2024

torkelo Apr 18, 2024

dprokop Apr 18, 2024

torkelo Apr 18, 2024

dprokop Apr 18, 2024

torkelo Apr 18, 2024

dprokop Apr 18, 2024

torkelo Apr 19, 2024

ashharrison90 left a comment

bfmatei left a comment

grafanabot commented Apr 22, 2024

ashharrison90 commented Apr 22, 2024

GroupBy: Fetch options when opening menu #687

GroupBy: Fetch options when opening menu #687

Conversation

ashharrison90 commented Apr 10, 2024 • edited by grafanabot

dprokop commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashharrison90 left a comment

Choose a reason for hiding this comment

bfmatei left a comment

Choose a reason for hiding this comment

grafanabot commented Apr 22, 2024

ashharrison90 commented Apr 22, 2024

ashharrison90 commented Apr 10, 2024 •

edited by grafanabot