diff --git a/dashboards/src/components/Variables/Variable.test.ts b/dashboards/src/components/Variables/Variable.test.ts index 90f13d25..f52edb40 100644 --- a/dashboards/src/components/Variables/Variable.test.ts +++ b/dashboards/src/components/Variables/Variable.test.ts @@ -59,6 +59,25 @@ interface TestParams { } describe('useListVariableState', () => { + it('keeps normalized All selections stable across renders', () => { + const spec = { + name: 'example', + plugin: { kind: 'StaticListVariable', spec: {} }, + allowMultiple: true, + allowAllValue: true, + }; + const state = { value: '$__all', loading: false }; + const query = { data: [option('hello')], isFetching: false }; + const { result, rerender } = renderHook(() => useListVariableState(spec, state, query)); + const previous = result.current; + + rerender(); + + expect(result.current.value).toBe(previous.value); + expect(result.current.selectedOptions).toBe(previous.selectedOptions); + expect(result.current.value).toEqual(['$__all']); + }); + it.each([ { description: '[!ALL][!MULTIPLE] is fetching', diff --git a/dashboards/src/components/Variables/Variable.tsx b/dashboards/src/components/Variables/Variable.tsx index 269ebc0d..c921273f 100644 --- a/dashboards/src/components/Variables/Variable.tsx +++ b/dashboards/src/components/Variables/Variable.tsx @@ -81,12 +81,13 @@ export function useListVariableState( const loading = useMemo(() => variablesOptionsQuery.isFetching ?? false, [variablesOptionsQuery.isFetching]); const options = useMemo(() => variablesOptionsQuery.data ?? [], [variablesOptionsQuery.data]); - let initialValue = state?.value; - - // Make sure initialValue is an array if allowMultiple is true - if (allowMultiple && !Array.isArray(initialValue)) { - initialValue = typeof initialValue === 'string' ? [initialValue] : []; - } + const initialValue = useMemo(() => { + const currentValue = state?.value; + if (allowMultiple && !Array.isArray(currentValue)) { + return typeof currentValue === 'string' ? [currentValue] : []; + } + return currentValue; + }, [allowMultiple, state?.value]); // Sort the provided list of options according to the method defined const sortedOptions = useMemo((): VariableOption[] => { @@ -138,9 +139,8 @@ export function useListVariableState( // In the case Autocomplete.multiple equals false, Autocomplete.value expects a single object, not // an array, hence this conditional if (Array.isArray(value)) { - return viewOptions.filter((o) => { - return value?.includes(o.value); - }); + const selectedValues = new Set(value); + return viewOptions.filter((option) => selectedValues.has(option.value)); } else { return ( viewOptions.find((o) => { @@ -157,6 +157,8 @@ const StyledPopper = (props: PopperProps): ReactElement => ( ); +const filterOptions = createFilterOptions(); + const LETTER_HSIZE = 8; // approximation const ARROW_OFFSET = 40; // right offset for list variables (= take into account the dropdown toggle size) const getWidthPx = (inputValue: string, kind: 'list' | 'text'): number => { @@ -188,11 +190,9 @@ function ListVariable({ name, source }: VariableProps): ReactElement { const allowMultiple = definition?.spec.allowMultiple === true; const allowAllValue = definition?.spec.allowAllValue === true; - const filterOptions = createFilterOptions({}); - const filteredOptions = useMemo( () => filterOptions(viewOptions, { inputValue, getOptionLabel: (o) => o.label }), - [inputValue, viewOptions, filterOptions], + [inputValue, viewOptions], ); // Update value when changed @@ -320,7 +320,6 @@ function ListVariable({ name, source }: VariableProps): ReactElement { }, [ allowAllValue, allowMultiple, - filterOptions, inputValue, inputWidth, loading, diff --git a/dashboards/src/context/VariableProvider/VariableProvider.performance.test.tsx b/dashboards/src/context/VariableProvider/VariableProvider.performance.test.tsx new file mode 100644 index 00000000..ec1dd3cb --- /dev/null +++ b/dashboards/src/context/VariableProvider/VariableProvider.performance.test.tsx @@ -0,0 +1,197 @@ +// Copyright The Perses Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import type * as PluginSystem from '@perses-dev/plugin-system'; +import { TimeRangeProviderBasic, useVariableValues } from '@perses-dev/plugin-system'; +import type { VariableDefinition } from '@perses-dev/spec'; +import { DEFAULT_ALL_VALUE } from '@perses-dev/spec'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { act, fireEvent, render, renderHook, screen } from '@testing-library/react'; +import type { ReactElement, ReactNode } from 'react'; +import { Suspense, useState } from 'react'; + +import { Variable } from '../../components/Variables/Variable'; +import type * as QueryParams from './query-params'; +import { + useVariableDefinitionStoreCtx, + useVariableDefinitionActions, + useVariableDefinitionAndState, + useVariableDefinitionStates, + VariableProvider, + VariableProviderWithQueryParams, +} from './VariableProvider'; + +const options = [ + { value: 'dev', label: 'Development' }, + { value: 'prod', label: 'Production' }, +]; +const variableQuery = { data: options, isFetching: false }; +vi.mock('@perses-dev/plugin-system', async (importOriginal) => ({ + ...(await importOriginal()), + useListVariablePluginValues: (): typeof variableQuery => variableQuery, +})); + +const setQueryParams = vi.fn(); +const emptyQueryValues = {}; +vi.mock('./query-params', async (importOriginal) => ({ + ...(await importOriginal()), + useVariableQueryParams: (): ReturnType => [ + emptyQueryValues, + setQueryParams, + ], +})); +const chartFallback =
Loading chart
; + +const definitions: VariableDefinition[] = [ + { kind: 'TextVariable', spec: { name: 'region', value: 'west' } }, + { + kind: 'ListVariable', + spec: { + name: 'environment', + defaultValue: 'dev', + allowAllValue: true, + allowMultiple: false, + plugin: { kind: 'StaticListVariable', spec: {} }, + }, + }, +]; +const initialTimeRange = { pastDuration: '30m' } as const; + +function Wrapper({ children, queryParams = false }: { children: ReactNode; queryParams?: boolean }): ReactElement { + const Provider = queryParams ? VariableProviderWithQueryParams : VariableProvider; + const [queryClient] = useState(() => new QueryClient()); + return ( + + + {children} + + + ); +} + +function QueryParamsWrapper({ children }: { children: ReactNode }): ReactElement { + return {children}; +} + +function SlowVariableConsumer({ read }: { read: (value: string) => void }): ReactElement { + const values = useVariableValues(); + const value = values.environment?.value as string; + read(value); + return
Chart: {value}
; +} + +it('does not notify subscribers or rewrite the URL for identical normalized values', () => { + setQueryParams.mockClear(); + const { result } = renderHook(() => useVariableDefinitionStoreCtx(), { wrapper: QueryParamsWrapper }); + const store = result.current; + const onChange = vi.fn(); + store.subscribe(onChange); + const actions = store.getState(); + + act(() => actions.setVariableValue('environment', 'dev')); + expect(onChange).not.toHaveBeenCalled(); + expect(setQueryParams).not.toHaveBeenCalled(); + + act(() => actions.setVariableValue('environment', ['prod'])); + act(() => actions.setVariableValue('environment', ['prod'])); + expect(onChange).toHaveBeenCalledTimes(1); + expect(setQueryParams).toHaveBeenCalledTimes(1); + + act(() => actions.setVariableValue('environment', ['prod', DEFAULT_ALL_VALUE])); + act(() => actions.setVariableValue('environment', [DEFAULT_ALL_VALUE])); + expect(store.getState().variableState.get({ name: 'environment' })?.value).toBe(DEFAULT_ALL_VALUE); + expect(onChange).toHaveBeenCalledTimes(2); + expect(setQueryParams).toHaveBeenCalledTimes(2); + + act(() => actions.setVariableValue('environment', [DEFAULT_ALL_VALUE, 'prod'])); + expect(store.getState().variableState.get({ name: 'environment' })?.value).toEqual(['prod']); + expect(setQueryParams).toHaveBeenLastCalledWith({ 'var-environment': ['prod'] }); +}); + +it('does not publish identical options or loading state again', () => { + const { result } = renderHook(() => useVariableDefinitionStoreCtx(), { wrapper: Wrapper }); + const store = result.current; + const actions = store.getState(); + act(() => actions.setVariableOptions('environment', options)); + const previous = store.getState(); + const onChange = vi.fn(); + store.subscribe(onChange); + + act(() => + actions.setVariableOptions( + 'environment', + options.map((option) => Object.assign({}, option)), + ), + ); + act(() => actions.setVariableLoading('environment', false)); + expect(store.getState()).toBe(previous); + expect(onChange).not.toHaveBeenCalled(); + + act(() => actions.setVariableOptions('environment', [{ value: 'dev', label: 'Renamed development' }])); + expect(onChange).toHaveBeenCalledTimes(1); +}); + +it('keeps unrelated input and variable-state subscriptions unchanged', () => { + const { result } = renderHook( + () => ({ + variable: useVariableDefinitionAndState('environment'), + states: useVariableDefinitionStates(['environment']), + actions: useVariableDefinitionActions(), + }), + { wrapper: Wrapper }, + ); + const previous = result.current; + + act(() => result.current.actions.setVariableValue('region', 'east')); + expect(result.current).toBe(previous); + + act(() => result.current.actions.setVariableValue('environment', 'prod')); + expect(result.current.variable.state?.value).toBe('prod'); + expect(result.current.states.environment?.value).toBe('prod'); +}); + +it('updates the selected input while a chart is still rendering the new variable value', async () => { + let ready = false; + let finish!: () => void; + const pending = new Promise((resolve) => { + finish = resolve; + }); + const read = vi.fn((value: string): void => { + if (value === 'prod' && !ready) throw pending; + }); + render( + + + + + + , + ); + const input = screen.getByRole('combobox', { name: 'environment' }); + expect(input).toHaveValue('Development'); + const initialReads = read.mock.calls.filter(([value]) => value === 'dev').length; + fireEvent.mouseDown(input); + fireEvent.click(screen.getByRole('option', { name: 'Production' })); + + expect(input).toHaveValue('Production'); + expect(read.mock.calls.filter(([value]) => value === 'dev')).toHaveLength(initialReads); + expect(screen.getByText('Chart: dev')).toBeInTheDocument(); + expect(screen.queryByText('Loading chart')).not.toBeInTheDocument(); + + await act(async () => { + ready = true; + finish(); + await pending; + }); + expect(screen.getByText('Chart: prod')).toBeInTheDocument(); +}); diff --git a/dashboards/src/context/VariableProvider/VariableProvider.tsx b/dashboards/src/context/VariableProvider/VariableProvider.tsx index fa983f4b..239b8403 100644 --- a/dashboards/src/context/VariableProvider/VariableProvider.tsx +++ b/dashboards/src/context/VariableProvider/VariableProvider.tsx @@ -30,7 +30,7 @@ import type { import { DEFAULT_ALL_VALUE as ALL_VALUE, formatDuration, intervalToDuration } from '@perses-dev/spec'; import { produce } from 'immer'; import type { ReactElement, ReactNode } from 'react'; -import { createContext, useContext, useEffect, useMemo, useState } from 'react'; +import { createContext, useContext, useDeferredValue, useEffect, useMemo, useState } from 'react'; import type { StoreApi } from 'zustand'; import { createStore, useStore } from 'zustand'; import { devtools } from 'zustand/middleware'; @@ -150,9 +150,7 @@ export function useVariableDefinitionStates(variableNames?: string[]): VariableS return varStates; }, - (left, right) => { - return JSON.stringify(left) === JSON.stringify(right); - }, + shallow, ); } @@ -201,15 +199,19 @@ export function useVariableDefinitionAndState( state: VariableState | undefined; } { const store = useVariableDefinitionStoreCtx(); - return useStore(store, (s) => { - const state = s.variableState.get({ name, source }); - const definitions = source - ? s.externalVariableDefinitions.find((v) => v.source === source)?.definitions - : s.variableDefinitions; - const definition = (definitions || []).find((v) => v.spec.name === name); - - return { state, definition }; - }); + return useStoreWithEqualityFn( + store, + (s) => { + const state = s.variableState.get({ name, source }); + const definitions = source + ? s.externalVariableDefinitions.find((v) => v.source === source)?.definitions + : s.variableDefinitions; + const definition = (definitions || []).find((v) => v.spec.name === name); + + return { state, definition }; + }, + shallow, + ); } export function useVariableDefinitionActions(): { @@ -253,7 +255,8 @@ interface PluginProviderProps { } function PluginProvider({ children, builtinVariables }: PluginProviderProps): ReactElement { - const originalValues = useVariableDefinitionStates(); + // Inputs read the store immediately; chart and query consumers can render in the background. + const originalValues = useDeferredValue(useVariableDefinitionStates()); const definitions = useVariableDefinitions(); const externalDefinitions = useExternalVariableDefinitions(); const { absoluteTimeRange } = useTimeRange(); @@ -356,9 +359,12 @@ function PluginProvider({ children, builtinVariables }: PluginProviderProps): Re return result; }, [absoluteTimeRange, builtinVariables]); + const builtinContext = useMemo(() => ({ variables: allBuiltinVariables }), [allBuiltinVariables]); + const variableContext = useMemo(() => ({ state: values }), [values]); + return ( - - {children} + + {children} ); } @@ -423,6 +429,16 @@ function createVariableDefinitionStore({ ); }, setVariableOptions(name, options, source?: string): void { + const currentOptions = get().variableState.get({ name, source })?.options; + if (currentOptions === options) return; + if ( + currentOptions?.length === options.length && + currentOptions.every( + (option, index) => option.value === options[index]?.value && option.label === options[index]?.label, + ) + ) { + return; + } set( (state) => { const varState = state.variableState.get({ name, source }); @@ -436,6 +452,7 @@ function createVariableDefinitionStore({ ); }, setVariableLoading(name, loading, source?: string): void { + if (get().variableState.get({ name, source })?.loading === loading) return; set( (state) => { const varState = state.variableState.get({ name, source }); @@ -448,32 +465,31 @@ function createVariableDefinitionStore({ '[Variables] setVariableLoading', ); }, - setVariableValue: (name, value, source?: string): void => + setVariableValue: (name, value, source?: string): void => { + const currentState = get().variableState.get({ name, source }); + if (!currentState) return; + + let nextValue = value; + // Normalize the All option before comparing, including repeated multi-select updates. + if (Array.isArray(nextValue) && nextValue.includes(ALL_VALUE)) { + nextValue = nextValue.at(-1) === ALL_VALUE ? ALL_VALUE : nextValue.filter((v) => v !== ALL_VALUE); + } + if (areVariableValuesEqual(currentState.value, nextValue)) return; + set( (state) => { - let val = value; const varState = state.variableState.get({ name, source }); if (!varState) { return; } - // Make sure there is only one all value - if (Array.isArray(val) && val.includes(ALL_VALUE)) { - if (val.at(-1) === ALL_VALUE) { - val = ALL_VALUE; - } else { - val = val.filter((v) => v !== ALL_VALUE); - } - } - if (queryParams) { - const setQueryParams = queryParams[1]; - setQueryParams({ [getURLQueryParamName(name)]: val }); - } - varState.value = val; + varState.value = nextValue; }, false, '[Variables] setVariableValue', - ), + ); + queryParams?.[1]({ [getURLQueryParamName(name)]: nextValue }); + }, setVariableDefaultValues: (): VariableDefinition[] => { const variableDefinitions = get().variableDefinitions; const variableState = get().variableState; @@ -543,12 +559,14 @@ export interface VariableProviderProps { initialVariableValues?: Record; } +const EMPTY_BUILTIN_VARIABLE_DEFINITIONS: BuiltinVariableDefinition[] = []; + // TODO: merge the different providers related to Variables under a single one (and keep "VariableProvider" as a name) export function VariableProvider({ children, initialVariableDefinitions = [], externalVariableDefinitions = [], - builtinVariableDefinitions = [], + builtinVariableDefinitions = EMPTY_BUILTIN_VARIABLE_DEFINITIONS, initialVariableValues, }: VariableProviderProps): ReactElement { const [store] = useState(() => @@ -566,7 +584,7 @@ export function VariableProviderWithQueryParams({ children, initialVariableDefinitions = [], externalVariableDefinitions = [], - builtinVariableDefinitions: builtinVariables = [], + builtinVariableDefinitions: builtinVariables = EMPTY_BUILTIN_VARIABLE_DEFINITIONS, }: VariableProviderProps): ReactElement { const allVariableDefs = mergeVariableDefinitions(initialVariableDefinitions, externalVariableDefinitions); const queryParams = useVariableQueryParams(allVariableDefs);