From f3ac5363e107d3283d61b460f8096135ad62f3b1 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Tue, 23 Apr 2024 13:53:19 +0300 Subject: [PATCH 1/7] GroupByVariable: Sync label to URL --- .../AdHocFiltersVariableUrlSyncHandler.ts | 47 ++---------- .../src/variables/groupby/GroupByVariable.tsx | 39 +++++++--- .../groupby/GroupByVariableUrlSyncHandler.ts | 76 +++++++++++++++++++ packages/scenes/src/variables/utils.ts | 38 ++++++++++ 4 files changed, 146 insertions(+), 54 deletions(-) create mode 100644 packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts diff --git a/packages/scenes/src/variables/adhoc/AdHocFiltersVariableUrlSyncHandler.ts b/packages/scenes/src/variables/adhoc/AdHocFiltersVariableUrlSyncHandler.ts index 2172ac5c7..f33f9a9ea 100644 --- a/packages/scenes/src/variables/adhoc/AdHocFiltersVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/adhoc/AdHocFiltersVariableUrlSyncHandler.ts @@ -1,5 +1,6 @@ import { SceneObjectUrlSyncHandler, SceneObjectUrlValue, SceneObjectUrlValues } from '../../core/types'; import { AdHocFiltersVariable, AdHocFilterWithLabels, isFilterComplete } from './AdHocFiltersVariable'; +import { escapeUrlPipeDelimiters, toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils'; export class AdHocFiltersVariableUrlSyncHandler implements SceneObjectUrlSyncHandler { public constructor(private _variable: AdHocFiltersVariable) {} @@ -19,7 +20,7 @@ export class AdHocFiltersVariableUrlSyncHandler implements SceneObjectUrlSyncHan return { [this.getKey()]: [''] }; } - const value = filters.filter(isFilterComplete).map((filter) => toArray(filter).map(escapePipeDelimiters).join('|')); + const value = filters.filter(isFilterComplete).map((filter) => toArray(filter).map(escapeUrlPipeDelimiters).join('|')); return { [this.getKey()]: value }; } @@ -45,52 +46,14 @@ function deserializeUrlToFilters(value: SceneObjectUrlValue): AdHocFilterWithLab return filter === null ? [] : [filter]; } -function escapePipeDelimiters(value: string | undefined): string { - if (value === null || value === undefined) { - return ''; - } - - // Replace the pipe due to using it as a filter separator - return (value = /\|/g[Symbol.replace](value, '__gfp__')); -} - -function escapeCommaDelimiters(value: string | undefined): string { - if (value === null || value === undefined) { - return ''; - } - - // Replace the comma due to using it as a value/label separator - return /,/g[Symbol.replace](value, '__gfc__'); -} - -function unescapeDelimiters(value: string | undefined): string { - if (value === null || value === undefined) { - return ''; - } - - value = /__gfp__/g[Symbol.replace](value, '|'); - value = /__gfc__/g[Symbol.replace](value, ','); - - return value; -} - function toArray(filter: AdHocFilterWithLabels): string[] { return [ - toCommaDelimitedString(filter.key, filter.keyLabel), + toUrlCommaDelimitedString(filter.key, filter.keyLabel), filter.operator, - toCommaDelimitedString(filter.value, filter.valueLabel), + toUrlCommaDelimitedString(filter.value, filter.valueLabel), ]; } -function toCommaDelimitedString(key: string, label?: string): string { - // Omit for identical key/label or when label is not set at all - if (!label || key === label) { - return escapeCommaDelimiters(key); - } - - return [key, label].map(escapeCommaDelimiters).join(','); -} - function toFilter(urlValue: string | number | boolean | undefined | null): AdHocFilterWithLabels | null { if (typeof urlValue !== 'string' || urlValue.length === 0) { return null; @@ -105,7 +68,7 @@ function toFilter(urlValue: string | number | boolean | undefined | null): AdHoc return acc; }, []) - .map(unescapeDelimiters); + .map(unescapeUrlDelimiters); return { key, diff --git a/packages/scenes/src/variables/groupby/GroupByVariable.tsx b/packages/scenes/src/variables/groupby/GroupByVariable.tsx index 064293141..62ab6d2ed 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariable.tsx +++ b/packages/scenes/src/variables/groupby/GroupByVariable.tsx @@ -1,17 +1,18 @@ import React, { useEffect, useMemo, useState } from 'react'; -import { AdHocVariableFilter, DataSourceApi, MetricFindValue } from '@grafana/data'; +import { AdHocVariableFilter, DataSourceApi, MetricFindValue, SelectableValue } from '@grafana/data'; import { allActiveGroupByVariables } from './findActiveGroupByVariablesByUid'; import { DataSourceRef, VariableType } from '@grafana/schema'; -import { SceneComponentProps, ControlsLayout } from '../../core/types'; +import { SceneComponentProps, ControlsLayout, SceneObjectUrlSyncHandler } from '../../core/types'; import { sceneGraph } from '../../core/sceneGraph'; import { ValidateAndUpdateResult, VariableValueOption, VariableValueSingle } from '../types'; import { MultiValueVariable, MultiValueVariableState, VariableGetOptionsArgs } from '../variants/MultiValueVariable'; import { from, lastValueFrom, map, mergeMap, Observable, of, take } from 'rxjs'; import { getDataSource } from '../../utils/getDataSource'; import { InputActionMeta, MultiSelect } from '@grafana/ui'; -import { isArray } from 'lodash'; +import { isArray, unionBy } from 'lodash'; import { getQueriesForVariables } from '../utils'; import { OptionWithCheckbox } from '../components/VariableValueSelect'; +import { GroupByVariableUrlSyncHandler } from './GroupByVariableUrlSyncHandler'; export interface GroupByVariableState extends MultiValueVariableState { /** Defaults to "Group" */ @@ -19,6 +20,8 @@ export interface GroupByVariableState extends MultiValueVariableState { /** The visible keys to group on */ // TODO review this type and name (naming is hard) defaultOptions?: MetricFindValue[]; + /** Options obtained through URL sync */ + urlOptions?: MetricFindValue[]; /** Base filters to always apply when looking up keys */ baseFilters?: AdHocVariableFilter[]; /** Datasource to use for getTagKeys and also controls which scene queries the group by should apply to */ @@ -57,6 +60,8 @@ export class GroupByVariable extends MultiValueVariable { static Component = GroupByVariableRenderer; isLazy = true; + protected _urlSync: SceneObjectUrlSyncHandler = new GroupByVariableUrlSyncHandler(this); + public validateAndUpdate(): Observable { return this.getValueOptions({}).pipe( map((options) => { @@ -80,10 +85,12 @@ export class GroupByVariable extends MultiValueVariable { } public getValueOptions(args: VariableGetOptionsArgs): Observable { + const urlOptions = this.state.urlOptions ?? []; + // When default dimensions are provided, return the static list if (this.state.defaultOptions) { return of( - this.state.defaultOptions.map((o) => ({ + unionBy(this.state.defaultOptions, urlOptions, 'value').map((o) => ({ label: o.text, value: String(o.value), })) @@ -101,7 +108,7 @@ export class GroupByVariable extends MultiValueVariable { return from(this._getKeys(ds)).pipe( take(1), mergeMap((data: MetricFindValue[]) => { - const a: VariableValueOption[] = data.map((i) => { + const a: VariableValueOption[] = unionBy(data, urlOptions, 'value').map((i) => { return { label: i.text, value: i.value ? String(i.value) : i.text, @@ -182,19 +189,27 @@ export class GroupByVariable extends MultiValueVariable { } } export function GroupByVariableRenderer({ model }: SceneComponentProps) { - const { value, key, maxVisibleValues, noValueOnClear } = model.useState(); - const arrayValue = useMemo(() => (isArray(value) ? value : [value]), [value]); + const { value, text, key, maxVisibleValues, noValueOnClear } = model.useState(); + const values = useMemo>>(() => { + const arrayValue = isArray(value) ? value : [value]; + const arrayText = isArray(text) ? text : [text]; + + return arrayValue.map((value, idx) => ({ + value, + label: String(arrayText[idx] ?? value), + })); + }, [value, text]); const [isFetchingOptions, setIsFetchingOptions] = useState(false); const [isOptionsOpen, setIsOptionsOpen] = useState(false); const [inputValue, setInputValue] = useState(''); // To not trigger queries on every selection we store this state locally here and only update the variable onBlur - const [uncommittedValue, setUncommittedValue] = useState(arrayValue); + const [uncommittedValue, setUncommittedValue] = useState(values); // Detect value changes outside useEffect(() => { - setUncommittedValue(arrayValue); - }, [arrayValue]); + setUncommittedValue(values); + }, [values]); const onInputChange = (value: string, { action }: InputActionMeta) => { if (action === 'input-change') { @@ -237,13 +252,13 @@ export function GroupByVariableRenderer({ model }: SceneComponentProps { - model.changeValueTo(uncommittedValue); + model.changeValueTo(uncommittedValue.map((x) => x.value)); }} onChange={(newValue, action) => { if (action.action === 'clear' && noValueOnClear) { model.changeValueTo([]); } - setUncommittedValue(newValue.map((x) => x.value!)); + setUncommittedValue(newValue); }} onOpenMenu={async () => { setIsFetchingOptions(true); diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts new file mode 100644 index 000000000..8a43167dc --- /dev/null +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -0,0 +1,76 @@ +import { unzip, zip } from 'lodash'; +import { SceneObjectUrlSyncHandler, SceneObjectUrlValues } from '../../core/types'; +import { GroupByVariable } from './GroupByVariable'; +import { toUrlCommaDelimitedString } from '../utils'; + +export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler { + public constructor(private _sceneObject: GroupByVariable) {} + + private getKey(): string { + return `var-${this._sceneObject.state.name}`; + } + + public getKeys(): string[] { + if (this._sceneObject.state.skipUrlSync) { + return []; + } + + return [this.getKey()]; + } + + public getUrlState(): SceneObjectUrlValues { + if (this._sceneObject.state.skipUrlSync) { + return {}; + } + + let { value: values, text: texts } = this._sceneObject.state; + + values = Array.isArray(values) ? values : [values]; + texts = Array.isArray(texts) ? texts : [texts]; + + const urlValue = zip(values, texts).map(toUrlValues); + + return { [this.getKey()]: urlValue }; + } + + public updateFromUrl(values: SceneObjectUrlValues): void { + let urlValue = values[this.getKey()]; + + if (urlValue != null) { + /** + * Initial URL Sync happens before scene objects are activated. + * We need to skip validation in this case to make sure values set via URL are maintained. + */ + if (!this._sceneObject.isActive) { + this._sceneObject.skipNextValidation = true; + } + + urlValue = Array.isArray(urlValue) ? urlValue : [urlValue]; + let [values, labels] = unzip(urlValue.map((value) => (value ? value.split(',') : [value]))); + + // Fail-safe as this cannot be type-checked properly + values = values ?? []; + labels = labels ?? []; + + this._sceneObject.setState({ + urlOptions: values.map((value, idx) => ({ + value, + text: labels[idx], + })), + }); + + this._sceneObject.changeValueTo(values, labels); + } + } +} + +function toUrlValues([value, label]: [string, string]) { + if (value === undefined || value === null) { + return value; + } + + value = String(value); + label = label === undefined || label === null ? value : String(label); + + return toUrlCommaDelimitedString(value, label); +} diff --git a/packages/scenes/src/variables/utils.ts b/packages/scenes/src/variables/utils.ts index 9f76d1763..1f8897aea 100644 --- a/packages/scenes/src/variables/utils.ts +++ b/packages/scenes/src/variables/utils.ts @@ -135,3 +135,41 @@ function filterOutInactiveRunnerDuplicates(runners: SceneQueryRunner[]) { return activeItems; }); } + +export function escapeUrlPipeDelimiters(value: string | undefined): string { + if (value === null || value === undefined) { + return ''; + } + + // Replace the pipe due to using it as a filter separator + return (value = /\|/g[Symbol.replace](value, '__gfp__')); +} + +export function escapeUrlCommaDelimiters(value: string | undefined): string { + if (value === null || value === undefined) { + return ''; + } + + // Replace the comma due to using it as a value/label separator + return /,/g[Symbol.replace](value, '__gfc__'); +} + +export function unescapeUrlDelimiters(value: string | undefined): string { + if (value === null || value === undefined) { + return ''; + } + + value = /__gfp__/g[Symbol.replace](value, '|'); + value = /__gfc__/g[Symbol.replace](value, ','); + + return value; +} + +export function toUrlCommaDelimitedString(key: string, label?: string): string { + // Omit for identical key/label or when label is not set at all + if (!label || key === label) { + return escapeUrlCommaDelimiters(key); + } + + return [key, label].map(escapeUrlCommaDelimiters).join(','); +} From d22490ae96809eb479b3c1894f9528a141685c76 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Tue, 23 Apr 2024 14:52:55 +0300 Subject: [PATCH 2/7] Unescape delimiters --- .../variables/groupby/GroupByVariableUrlSyncHandler.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts index 8a43167dc..7fb98b485 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -1,7 +1,7 @@ import { unzip, zip } from 'lodash'; import { SceneObjectUrlSyncHandler, SceneObjectUrlValues } from '../../core/types'; import { GroupByVariable } from './GroupByVariable'; -import { toUrlCommaDelimitedString } from '../utils'; +import { toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils'; export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler { public constructor(private _sceneObject: GroupByVariable) {} @@ -46,11 +46,11 @@ export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler } urlValue = Array.isArray(urlValue) ? urlValue : [urlValue]; - let [values, labels] = unzip(urlValue.map((value) => (value ? value.split(',') : [value]))); + const valuesLabelsPairs = urlValue.map((value) => (value ? value.split(',') : [value])); + let [values, labels] = unzip(valuesLabelsPairs); - // Fail-safe as this cannot be type-checked properly - values = values ?? []; - labels = labels ?? []; + values = (values ?? []).map(unescapeUrlDelimiters); + labels = (labels ?? []).map(unescapeUrlDelimiters); this._sceneObject.setState({ urlOptions: values.map((value, idx) => ({ From 906b6a0f940222f1dee93aa56e6edb9d35d595c9 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Tue, 23 Apr 2024 15:15:18 +0300 Subject: [PATCH 3/7] Fix typechecking --- packages/scenes/src/variables/groupby/GroupByVariable.tsx | 2 +- .../src/variables/groupby/GroupByVariableUrlSyncHandler.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariable.tsx b/packages/scenes/src/variables/groupby/GroupByVariable.tsx index 62ab6d2ed..21f3aa7be 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariable.tsx +++ b/packages/scenes/src/variables/groupby/GroupByVariable.tsx @@ -252,7 +252,7 @@ export function GroupByVariableRenderer({ model }: SceneComponentProps { - model.changeValueTo(uncommittedValue.map((x) => x.value)); + model.changeValueTo(uncommittedValue.map((x) => x.value!)); }} onChange={(newValue, action) => { if (action.action === 'clear' && noValueOnClear) { diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts index 7fb98b485..f91fb6d31 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -2,6 +2,7 @@ import { unzip, zip } from 'lodash'; import { SceneObjectUrlSyncHandler, SceneObjectUrlValues } from '../../core/types'; import { GroupByVariable } from './GroupByVariable'; import { toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils'; +import { VariableValueSingle } from '../types'; export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler { public constructor(private _sceneObject: GroupByVariable) {} @@ -64,9 +65,11 @@ export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler } } -function toUrlValues([value, label]: [string, string]) { +function toUrlValues([value, label]: [VariableValueSingle | undefined, VariableValueSingle | undefined]): + | string + | undefined { if (value === undefined || value === null) { - return value; + return undefined; } value = String(value); From e284000d91831a674a3cfbf3e3b61cc16f30d064 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Tue, 23 Apr 2024 16:34:32 +0300 Subject: [PATCH 4/7] Fix typechecking --- .../src/variables/groupby/GroupByVariableUrlSyncHandler.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts index f91fb6d31..a3c43934f 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -65,11 +65,9 @@ export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler } } -function toUrlValues([value, label]: [VariableValueSingle | undefined, VariableValueSingle | undefined]): - | string - | undefined { +function toUrlValues([value, label]: [VariableValueSingle | undefined, VariableValueSingle | undefined]): string { if (value === undefined || value === null) { - return undefined; + return ''; } value = String(value); From d2b74a93817130df0047638f8468c3908b8ee901 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Fri, 26 Apr 2024 14:28:28 +0300 Subject: [PATCH 5/7] Add tests --- .../groupby/GroupByVariable.test.tsx | 110 +++++++++++++++--- 1 file changed, 96 insertions(+), 14 deletions(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx b/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx index 973855280..c74e21d6b 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx +++ b/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx @@ -46,27 +46,109 @@ describe('GroupByVariable', () => { }); }); - it('url sync works', async () => { - const { variable } = setupTest({ - defaultOptions: [ - { text: 'a', value: 'a' }, - { text: 'b', value: 'b' }, - ], - }); + describe('url sync', () => { + it('should work with default options', () => { + const { variable } = setupTest({ + defaultOptions: [ + { text: 'a', value: 'a' }, + { text: 'b', value: 'b' }, + ], + }); + + expect(variable.state.value).toEqual(''); - expect(variable.state.value).toEqual(''); + act(() => { + variable.changeValueTo(['a']); + }); - act(() => { - variable.changeValueTo(['a']); + expect(locationService.getLocation().search).toBe('?var-test=a'); + + act(() => { + locationService.push('/?var-test=a&var-test=b'); + }); + + expect(variable.state.value).toEqual(['a', 'b']); + + act(() => { + locationService.push('/?var-test=a&var-test=b&var-test=c'); + }); + + expect(variable.state.value).toEqual(['a', 'b', 'c']); }); - expect(locationService.getLocation().search).toBe('?var-test=a'); + it('should work with received options', async () => { + const { variable } = setupTest({ + getTagKeysProvider: () => { + return Promise.resolve({ + replace: true, + values: [ + { text: 'A', value: 'a' }, + { text: 'b', value: 'b' }, + { text: 'C', value: 'c' }, + ], + }); + }, + }); + + await act(async () => { + await lastValueFrom(variable.validateAndUpdate()); + }); + + expect(variable.state.value).toEqual(''); + + act(() => { + variable.changeValueTo(['a']); + }); + + expect(locationService.getLocation().search).toBe('?var-test=a,A'); + + act(() => { + locationService.push('/?var-test=a,A&var-test=b'); + }); - act(() => { - locationService.push('/?var-test=a&var-test=b'); + expect(variable.state.value).toEqual(['a', 'b']); + + act(() => { + locationService.push('/?var-test=a,A&var-test=b&var-test=c'); + }); + + expect(variable.state.value).toEqual(['a', 'b', 'c']); }); - expect(variable.state.value).toEqual(['a', 'b']); + it('should work with commas', async () => { + const { variable } = setupTest({ + defaultOptions: [ + { text: 'A,something', value: 'a' }, + { text: 'B', value: 'b,something' }, + ], + }); + + expect(variable.state.value).toEqual(''); + + await act(async () => { + await lastValueFrom(variable.validateAndUpdate()); + }); + + act(() => { + variable.changeValueTo(['a']); + }); + + expect(locationService.getLocation().search).toBe('?var-test=a,A__gfc__something'); + + act(() => { + locationService.push('/?var-test=a,A__gfc__something&var-test=b__gfc__something'); + }); + + expect(variable.state.value).toEqual(['a', 'b,something']); + + act(() => { + locationService.push( + '/?var-test=a,A__gfc__something&var-test=b__gfc__something&var-test=c__gfc__something,C__gfc__something' + ); + }); + + expect(variable.state.value).toEqual(['a', 'b,something', 'c,something']); + }); }); it('Can override and replace getTagKeys', async () => { From 91edd051a992f6150aed35b82848efa22a807f40 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Fri, 10 May 2024 13:31:47 +0300 Subject: [PATCH 6/7] Remove urlOptions --- .../groupby/GroupByVariable.test.tsx | 9 +++ .../src/variables/groupby/GroupByVariable.tsx | 15 ++--- .../groupby/GroupByVariableUrlSyncHandler.ts | 66 ++++++++++--------- 3 files changed, 52 insertions(+), 38 deletions(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx b/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx index c74e21d6b..409f6dd26 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx +++ b/packages/scenes/src/variables/groupby/GroupByVariable.test.tsx @@ -56,6 +56,7 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(''); + expect(variable.state.text).toEqual(''); act(() => { variable.changeValueTo(['a']); @@ -68,12 +69,14 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(['a', 'b']); + expect(variable.state.text).toEqual(['a', 'b']); act(() => { locationService.push('/?var-test=a&var-test=b&var-test=c'); }); expect(variable.state.value).toEqual(['a', 'b', 'c']); + expect(variable.state.text).toEqual(['a', 'b', 'c']); }); it('should work with received options', async () => { @@ -95,6 +98,7 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(''); + expect(variable.state.text).toEqual(''); act(() => { variable.changeValueTo(['a']); @@ -107,12 +111,14 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(['a', 'b']); + expect(variable.state.text).toEqual(['A', 'b']); act(() => { locationService.push('/?var-test=a,A&var-test=b&var-test=c'); }); expect(variable.state.value).toEqual(['a', 'b', 'c']); + expect(variable.state.text).toEqual(['A', 'b', 'c']); }); it('should work with commas', async () => { @@ -124,6 +130,7 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(''); + expect(variable.state.text).toEqual(''); await act(async () => { await lastValueFrom(variable.validateAndUpdate()); @@ -140,6 +147,7 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(['a', 'b,something']); + expect(variable.state.text).toEqual(['A,something', 'b,something']); act(() => { locationService.push( @@ -148,6 +156,7 @@ describe('GroupByVariable', () => { }); expect(variable.state.value).toEqual(['a', 'b,something', 'c,something']); + expect(variable.state.text).toEqual(['A,something', 'b,something', 'C,something']); }); }); diff --git a/packages/scenes/src/variables/groupby/GroupByVariable.tsx b/packages/scenes/src/variables/groupby/GroupByVariable.tsx index 21f3aa7be..ecb70aa78 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariable.tsx +++ b/packages/scenes/src/variables/groupby/GroupByVariable.tsx @@ -9,7 +9,7 @@ import { MultiValueVariable, MultiValueVariableState, VariableGetOptionsArgs } f import { from, lastValueFrom, map, mergeMap, Observable, of, take } from 'rxjs'; import { getDataSource } from '../../utils/getDataSource'; import { InputActionMeta, MultiSelect } from '@grafana/ui'; -import { isArray, unionBy } from 'lodash'; +import { isArray } from 'lodash'; import { getQueriesForVariables } from '../utils'; import { OptionWithCheckbox } from '../components/VariableValueSelect'; import { GroupByVariableUrlSyncHandler } from './GroupByVariableUrlSyncHandler'; @@ -20,8 +20,6 @@ export interface GroupByVariableState extends MultiValueVariableState { /** The visible keys to group on */ // TODO review this type and name (naming is hard) defaultOptions?: MetricFindValue[]; - /** Options obtained through URL sync */ - urlOptions?: MetricFindValue[]; /** Base filters to always apply when looking up keys */ baseFilters?: AdHocVariableFilter[]; /** Datasource to use for getTagKeys and also controls which scene queries the group by should apply to */ @@ -85,12 +83,10 @@ export class GroupByVariable extends MultiValueVariable { } public getValueOptions(args: VariableGetOptionsArgs): Observable { - const urlOptions = this.state.urlOptions ?? []; - // When default dimensions are provided, return the static list if (this.state.defaultOptions) { return of( - unionBy(this.state.defaultOptions, urlOptions, 'value').map((o) => ({ + this.state.defaultOptions.map((o) => ({ label: o.text, value: String(o.value), })) @@ -108,7 +104,7 @@ export class GroupByVariable extends MultiValueVariable { return from(this._getKeys(ds)).pipe( take(1), mergeMap((data: MetricFindValue[]) => { - const a: VariableValueOption[] = unionBy(data, urlOptions, 'value').map((i) => { + const a: VariableValueOption[] = data.map((i) => { return { label: i.text, value: i.value ? String(i.value) : i.text, @@ -252,7 +248,10 @@ export function GroupByVariableRenderer({ model }: SceneComponentProps { - model.changeValueTo(uncommittedValue.map((x) => x.value!)); + model.changeValueTo( + uncommittedValue.map((x) => x.value!), + uncommittedValue.map((x) => x.label!) + ); }} onChange={(newValue, action) => { if (action.action === 'clear' && noValueOnClear) { diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts index a3c43934f..ef936fb04 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -1,8 +1,7 @@ -import { unzip, zip } from 'lodash'; import { SceneObjectUrlSyncHandler, SceneObjectUrlValues } from '../../core/types'; import { GroupByVariable } from './GroupByVariable'; import { toUrlCommaDelimitedString, unescapeUrlDelimiters } from '../utils'; -import { VariableValueSingle } from '../types'; +import { VariableValue } from '../types'; export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler { public constructor(private _sceneObject: GroupByVariable) {} @@ -24,14 +23,7 @@ export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler return {}; } - let { value: values, text: texts } = this._sceneObject.state; - - values = Array.isArray(values) ? values : [values]; - texts = Array.isArray(texts) ? texts : [texts]; - - const urlValue = zip(values, texts).map(toUrlValues); - - return { [this.getKey()]: urlValue }; + return { [this.getKey()]: toUrlValues(this._sceneObject.state.value, this._sceneObject.state.text) }; } public updateFromUrl(values: SceneObjectUrlValues): void { @@ -46,32 +38,46 @@ export class GroupByVariableUrlSyncHandler implements SceneObjectUrlSyncHandler this._sceneObject.skipNextValidation = true; } - urlValue = Array.isArray(urlValue) ? urlValue : [urlValue]; - const valuesLabelsPairs = urlValue.map((value) => (value ? value.split(',') : [value])); - let [values, labels] = unzip(valuesLabelsPairs); + const { values, texts } = fromUrlValues(urlValue); - values = (values ?? []).map(unescapeUrlDelimiters); - labels = (labels ?? []).map(unescapeUrlDelimiters); + this._sceneObject.changeValueTo(values, texts); + } + } +} - this._sceneObject.setState({ - urlOptions: values.map((value, idx) => ({ - value, - text: labels[idx], - })), - }); +function toUrlValues(values: VariableValue, texts: VariableValue): string[] { + values = Array.isArray(values) ? values : [values]; + texts = Array.isArray(texts) ? texts : [texts]; - this._sceneObject.changeValueTo(values, labels); + return values.map((value, idx) => { + if (value === undefined || value === null) { + return ''; } - } + + value = String(value); + + let text = texts[idx]; + text = text === undefined || text === null ? value : String(text); + + return toUrlCommaDelimitedString(value, text); + }); } -function toUrlValues([value, label]: [VariableValueSingle | undefined, VariableValueSingle | undefined]): string { - if (value === undefined || value === null) { - return ''; - } +function fromUrlValues(urlValues: string | string[]): { values: string[]; texts: string[] } { + urlValues = Array.isArray(urlValues) ? urlValues : [urlValues]; + + return urlValues.reduce( + (acc, urlValue) => { + const [value, label] = (urlValue ?? '').split(','); - value = String(value); - label = label === undefined || label === null ? value : String(label); + acc.values.push(unescapeUrlDelimiters(value)); + acc.texts.push(unescapeUrlDelimiters(label ?? value)); - return toUrlCommaDelimitedString(value, label); + return acc; + }, + { + values: [], + texts: [], + } + ); } From c861f37450c0636b915715f6a4fa59f8eb1bae02 Mon Sep 17 00:00:00 2001 From: Bogdan Matei Date: Fri, 10 May 2024 13:42:15 +0300 Subject: [PATCH 7/7] Fix typechecking --- .../src/variables/groupby/GroupByVariableUrlSyncHandler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts index ef936fb04..f5f148040 100644 --- a/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts +++ b/packages/scenes/src/variables/groupby/GroupByVariableUrlSyncHandler.ts @@ -66,7 +66,7 @@ function toUrlValues(values: VariableValue, texts: VariableValue): string[] { function fromUrlValues(urlValues: string | string[]): { values: string[]; texts: string[] } { urlValues = Array.isArray(urlValues) ? urlValues : [urlValues]; - return urlValues.reduce( + return urlValues.reduce<{ values: string[]; texts: string[] }>( (acc, urlValue) => { const [value, label] = (urlValue ?? '').split(',');