From 2837ca99385d85e5b31d334c92627a68047e2b56 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Wed, 15 Apr 2026 16:21:36 -0400 Subject: [PATCH 01/19] [Metrics][Discover] Preserve dimension picker options across filtered fetches (#263309) When multiple breakdown dimensions are selected, the METRICS_INFO query adds WHERE filters that narrow results to metrics having all selected dimensions. This caused `allDimensions` to be recomputed solely from the filtered response, making previously-available dimensions vanish from the picker. Fix: accumulate dimensions across fetches using a ref-based high-water mark in `useFetchMetricsData`. New dimensions are merged with the accumulated set on each fetch. The accumulated set resets when the base ES|QL query changes (i.e., the user switches data sources). --- .../metrics/hooks/use_fetch_metrics_data.ts | 25 ++++- .../metrics/utils/merge_dimensions.test.ts | 91 +++++++++++++++++++ .../metrics/utils/merge_dimensions.ts | 37 ++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index ea03f0f2efb46..8b6f5656b1dc1 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -8,7 +8,7 @@ */ import useAsyncFn from 'react-use/lib/useAsyncFn'; -import { useEffect, useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; import type { ChartSectionProps } from '@kbn/unified-histogram/types'; import { buildMetricsInfoQuery, hasTransformationalCommand } from '@kbn/esql-utils'; import { getFieldIconType } from '@kbn/field-utils'; @@ -18,6 +18,7 @@ import { useChartSectionInspector } from '../../../../context/chart_section_insp import { executeEsqlQuery } from '../utils/execute_esql_query'; import { parseMetricsWithTelemetry } from '../utils/parse_metrics_response_with_telemetry'; import { getEsqlQuery } from '../utils/get_esql_query'; +import { mergeDimensions } from '../utils/merge_dimensions'; /** * Fetches METRICS_INFO when in Metrics Experience (non-transformational ES|QL, chart visible). @@ -52,6 +53,17 @@ export function useFetchMetricsData({ [esql, selectedDimensions] ); + // Accumulate dimensions across filtered fetches so that selecting additional + // breakdown dimensions does not remove previously-available dimensions from + // the picker. Reset when the base ES|QL query changes (new data source). + const accumulatedDimensionsRef = useRef([]); + const previousEsqlRef = useRef(esql); + + if (esql !== previousEsqlRef.current) { + accumulatedDimensionsRef.current = []; + previousEsqlRef.current = esql; + } + const [{ value, error, loading }, executeFetch] = useAsyncFn( async (signal: AbortSignal): Promise => { const documents = await trackRequest( @@ -88,11 +100,20 @@ export function useFetchMetricsData({ const parsed = parseMetricsWithTelemetry(documents, getFieldType); + // Merge newly-fetched dimensions with the accumulated set so that + // dimensions from the unfiltered response are not lost when a + // WHERE filter narrows the result set. + const mergedDimensions = mergeDimensions( + accumulatedDimensionsRef.current, + parsed.allDimensions + ); + accumulatedDimensionsRef.current = mergedDimensions; + const sortedMetrics: ParsedMetrics = { metricItems: [...parsed.metricItems].sort((a, b) => a.metricName.localeCompare(b.metricName) ), - allDimensions: [...parsed.allDimensions].sort((a, b) => a.name.localeCompare(b.name)), + allDimensions: mergedDimensions, }; if (!signal.aborted) { diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts new file mode 100644 index 0000000000000..4735c2bec7815 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts @@ -0,0 +1,91 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { mergeDimensions } from './merge_dimensions'; +import type { Dimension } from '../../../../types'; + +describe('mergeDimensions', () => { + const dim = (name: string, type?: string): Dimension => ({ name, type }); + + it('returns incoming dimensions when accumulated is empty', () => { + const incoming = [dim('host.name'), dim('environment')]; + expect(mergeDimensions([], incoming)).toEqual([dim('environment'), dim('host.name')]); + }); + + it('returns accumulated dimensions when incoming is empty', () => { + const accumulated = [dim('host.name'), dim('environment')]; + expect(mergeDimensions(accumulated, [])).toEqual([dim('environment'), dim('host.name')]); + }); + + it('returns empty array when both are empty', () => { + expect(mergeDimensions([], [])).toEqual([]); + }); + + it('preserves accumulated dimensions that are missing from incoming (the bug scenario)', () => { + // Step 1: unfiltered query returns all 3 dimensions + const allDimensions = [dim('environment'), dim('host.name'), dim('region')]; + + // Step 2: user selects environment + host.name, filtered query returns only 2 + const filteredDimensions = [dim('environment'), dim('host.name')]; + + const result = mergeDimensions(allDimensions, filteredDimensions); + + // region should still be present + expect(result).toEqual([dim('environment'), dim('host.name'), dim('region')]); + expect(result.map((d) => d.name)).toContain('region'); + }); + + it('deduplicates dimensions by name', () => { + const accumulated = [dim('host.name'), dim('environment')]; + const incoming = [dim('host.name'), dim('region')]; + + const result = mergeDimensions(accumulated, incoming); + expect(result).toEqual([dim('environment'), dim('host.name'), dim('region')]); + + // No duplicates + const names = result.map((d) => d.name); + expect(new Set(names).size).toBe(names.length); + }); + + it('incoming dimensions update type info for existing entries', () => { + const accumulated = [dim('host.name', 'keyword'), dim('environment')]; + const incoming = [dim('host.name', 'ip')]; + + const result = mergeDimensions(accumulated, incoming); + + const hostDim = result.find((d) => d.name === 'host.name'); + expect(hostDim?.type).toBe('ip'); + }); + + it('sorts results alphabetically by name', () => { + const accumulated = [dim('z_field'), dim('a_field')]; + const incoming = [dim('m_field')]; + + const result = mergeDimensions(accumulated, incoming); + expect(result.map((d) => d.name)).toEqual(['a_field', 'm_field', 'z_field']); + }); + + it('handles the full multi-step selection scenario', () => { + // Step 1: initial unfiltered query + const step1 = [dim('environment'), dim('host.name'), dim('region')]; + let accumulated = mergeDimensions([], step1); + expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + + // Step 2: user selects environment, filtered query returns environment + host.name + // (region is on a metric that may not have environment) + const step2 = [dim('environment'), dim('host.name')]; + accumulated = mergeDimensions(accumulated, step2); + expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + + // Step 3: user also selects host.name, filtered query returns only environment + host.name + const step3 = [dim('environment'), dim('host.name')]; + accumulated = mergeDimensions(accumulated, step3); + expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + }); +}); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts new file mode 100644 index 0000000000000..0498750cb7a84 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts @@ -0,0 +1,37 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { Dimension } from '../../../../types'; + +/** + * Merges new dimensions into an accumulated set, preserving previously-seen + * dimensions that may no longer appear in filtered METRICS_INFO responses. + * + * This prevents the dimension picker from losing options when the user selects + * multiple breakdown dimensions, which causes the METRICS_INFO query to add + * WHERE filters that narrow the result set. + * + * @param accumulated - Previously accumulated dimensions (the "high-water mark") + * @param incoming - Dimensions from the latest METRICS_INFO response + * @returns Merged dimension list with no duplicates, sorted alphabetically + */ +export const mergeDimensions = (accumulated: Dimension[], incoming: Dimension[]): Dimension[] => { + const merged = new Map(); + + for (const dim of accumulated) { + merged.set(dim.name, dim); + } + + // Incoming dimensions may have updated type info, so they take precedence + for (const dim of incoming) { + merged.set(dim.name, dim); + } + + return Array.from(merged.values()).sort((a, b) => a.name.localeCompare(b.name)); +}; From 186646543574b0810dce02d7a517e6f978c2a437 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 14:58:58 -0400 Subject: [PATCH 02/19] fix: correct stale useFetchMetricsData docstring The JSDoc described the WHERE filter as triggering on "more than one item" and matching "at least one" selected dimension. In practice the filter triggers on any non-empty selection and joins with AND (all dimensions required). Update wording to match buildMetricsInfoQuery's actual behavior. --- .../observability/metrics/hooks/use_fetch_metrics_data.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index efa48a306ee46..081109da0e0e8 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -22,8 +22,8 @@ import { mergeDimensions } from '../utils/merge_dimensions'; /** * Fetches METRICS_INFO when in Metrics Experience (non-transformational ES|QL, chart visible). - * When selectedDimensionNames has more than one item, refetches with a WHERE filter so only - * metrics that have at least one of those dimensions are returned. + * When selectedDimensionNames is non-empty, refetches with a WHERE filter so only + * metrics that have all of the selected dimensions are returned. * Returns loading state, error, and parsed metrics info for the grid. */ export function useFetchMetricsData({ From 607e3cc5486fd3b805720185f1e4857524cb8dae Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 14:59:36 -0400 Subject: [PATCH 03/19] refactor: move accumulator reset out of render body Relocate the esql-change detection and accumulator reset into the useAsyncFn callback. Keeping all ref mutations inside the async boundary removes a render-time side effect and colocates the reset with the merge logic that depends on it. --- .../metrics/hooks/use_fetch_metrics_data.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index 081109da0e0e8..49b07a23ee505 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -59,15 +59,15 @@ export function useFetchMetricsData({ const accumulatedDimensionsRef = useRef([]); const previousEsqlRef = useRef(esql); - if (esql !== previousEsqlRef.current) { - accumulatedDimensionsRef.current = []; - previousEsqlRef.current = esql; - } - const [{ value, error, loading }, executeFetch] = useAsyncFn( async ( signal: AbortSignal ): Promise<(ParsedMetrics & { activeDimensions: Dimension[] }) | null> => { + if (esql !== previousEsqlRef.current) { + accumulatedDimensionsRef.current = []; + previousEsqlRef.current = esql; + } + const documents = await trackRequest( 'Grid of metrics', 'This request queries Elasticsearch to fetch metrics info for the grid.', From 6269aa2c721ea3b5421a57dd7296904324be9b76 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 15:02:40 -0400 Subject: [PATCH 04/19] fix: reset accumulator on any data-context change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the dimension accumulator only reset when the base ES|QL query string changed. If filters, timeRange, or esqlVariables narrowed the METRICS_INFO response, stale dimensions remained in the picker with no matching metrics in the grid. Compose a dataContextKey from esql + timeRange + filters + esqlVariables and reset the accumulator whenever it changes. The fix's original purpose — preserving dimensions across selected-dimension-driven WHERE filtering — is unaffected because selectedDimensionNames is intentionally excluded from the key. --- .../metrics/hooks/use_fetch_metrics_data.ts | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index 49b07a23ee505..a90171cddbe7e 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -55,17 +55,29 @@ export function useFetchMetricsData({ // Accumulate dimensions across filtered fetches so that selecting additional // breakdown dimensions does not remove previously-available dimensions from - // the picker. Reset when the base ES|QL query changes (new data source). + // the picker. Reset when anything other than the selected dimensions changes + // the data context (query, time range, Discover filters, ES|QL variables), + // since those changes can legitimately remove dimensions from the result. const accumulatedDimensionsRef = useRef([]); - const previousEsqlRef = useRef(esql); + const dataContextKey = useMemo( + () => + JSON.stringify({ + esql, + timeRange: fetchParams.timeRange, + filters: fetchParams.filters, + esqlVariables: fetchParams.esqlVariables, + }), + [esql, fetchParams.timeRange, fetchParams.filters, fetchParams.esqlVariables] + ); + const previousDataContextKeyRef = useRef(dataContextKey); const [{ value, error, loading }, executeFetch] = useAsyncFn( async ( signal: AbortSignal ): Promise<(ParsedMetrics & { activeDimensions: Dimension[] }) | null> => { - if (esql !== previousEsqlRef.current) { + if (dataContextKey !== previousDataContextKeyRef.current) { accumulatedDimensionsRef.current = []; - previousEsqlRef.current = esql; + previousDataContextKeyRef.current = dataContextKey; } const documents = await trackRequest( @@ -129,6 +141,7 @@ export function useFetchMetricsData({ }, [ metricsInfoQuery, + dataContextKey, trackRequest, fetchParams.dataView, fetchParams.timeRange, From fad860850fc90b46e3564de1c51b1b33d24a1081 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 15:03:30 -0400 Subject: [PATCH 05/19] test: cover accumulator plumbing in useFetchMetricsData Add hook-level tests that verify the full accumulator wiring, not just the pure mergeDimensions utility: - dimensions dropped by a filtered refetch remain in allDimensions - accumulator resets when the base ES|QL query changes - accumulator resets when timeRange changes These directly protect the dimension-picker fix from future regressions where the ref plumbing could be broken without the util tests failing. --- .../hooks/use_fetch_metrics_data.test.ts | 121 ++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts index 40fffbdf11809..9d04ef6e4f15d 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts @@ -517,4 +517,125 @@ describe('useFetchMetricsData', () => { ); }); }); + + describe('dimension accumulator', () => { + const regionDimension = createDimension('region'); + + it('preserves previously-seen dimensions when a subsequent fetch drops them', async () => { + // First fetch: full dimension set (unfiltered) + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics( + ['system.cpu.utilization'], + [regionDimension, hostDimension, serviceDimension] + ) + ); + + const params = createDefaultParams(); + const { result, rerender } = renderHook( + (props: ReturnType) => useFetchMetricsData(props), + { initialProps: params } + ); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + expect(result.current.allDimensions.map((d) => d.name)).toEqual([ + 'host.name', + 'region', + 'service.name', + ]); + }); + + // Second fetch (user selected host.name, query narrows): region is dropped + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.cpu.utilization'], [hostDimension, serviceDimension]) + ); + + rerender({ ...params, selectedDimensionNames: [hostDimension] }); + + await waitFor(() => { + expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); + }); + + await waitFor(() => { + // Accumulator keeps region visible so the user can still pick it + expect(result.current.allDimensions.map((d) => d.name)).toEqual([ + 'host.name', + 'region', + 'service.name', + ]); + }); + }); + + it('resets accumulated dimensions when the ES|QL query changes', async () => { + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.cpu.utilization'], [hostDimension]) + ); + + const params = createDefaultParams(); + const { result, rerender } = renderHook( + (props: ReturnType) => useFetchMetricsData(props), + { initialProps: params } + ); + + await waitFor(() => { + expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); + }); + + // Switch data source; new query returns a different dimension set + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.memory.utilization'], [regionDimension]) + ); + + rerender({ + ...params, + fetchParams: { ...params.fetchParams, query: { esql: 'TS apm-*' } }, + }); + + await waitFor(() => { + expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); + }); + + await waitFor(() => { + // Accumulator is reset: old dimensions from the previous data source are gone + expect(result.current.allDimensions.map((d) => d.name)).toEqual(['region']); + }); + }); + + it('resets accumulated dimensions when the timeRange changes', async () => { + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.cpu.utilization'], [hostDimension, serviceDimension]) + ); + + const params = createDefaultParams(); + const { result, rerender } = renderHook( + (props: ReturnType) => useFetchMetricsData(props), + { initialProps: params } + ); + + await waitFor(() => { + expect(result.current.allDimensions.map((d) => d.name)).toEqual([ + 'host.name', + 'service.name', + ]); + }); + + // New time range legitimately drops service.name from the response + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.cpu.utilization'], [hostDimension]) + ); + + rerender({ + ...params, + fetchParams: { ...params.fetchParams, timeRange: { from: 'now-1h', to: 'now' } }, + }); + + await waitFor(() => { + expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); + }); + + await waitFor(() => { + expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); + }); + }); + }); }); From c0302d370e813192f855c30b3443223d82bb7bad Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 15:42:16 -0400 Subject: [PATCH 06/19] fix: guard accumulator ref write against aborted fetches An in-flight fetch whose signal has been aborted (because a data-context change triggered effect cleanup and a new fetch) can still resolve and reach the ref write on accumulatedDimensionsRef.current. useAsyncFn's internal stale-call tracking discards the aborted call's returned value, but the ref mutation escapes that guard, so a subsequent fetch against the new context could inherit the stale dimension. Wrap the ref write with the same !signal.aborted check already used for telemetry. Adds a regression test that reproduces the leak by ordering a context-B immediate fetch in front of a context-A fetch that resolves afterward, then issues a third fetch within context B and asserts the accumulator still contains only context-B dimensions. Addresses review comment: https://github.com/elastic/kibana/pull/263629#discussion_r3095757115 --- .../hooks/use_fetch_metrics_data.test.ts | 101 ++++++++++++++++++ .../metrics/hooks/use_fetch_metrics_data.ts | 9 +- 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts index 9d04ef6e4f15d..e6df89240862f 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts @@ -601,6 +601,107 @@ describe('useFetchMetricsData', () => { }); }); + it('does not let an aborted fetch corrupt the accumulator for a later fetch', async () => { + // Reproduces the race where an in-flight fetch resolves *after* its + // signal has been aborted (because a data-context change triggered + // effect cleanup and a new fetch). useAsyncFn's internal stale-call + // tracking discards the aborted fetch's returned value, but the ref + // write on line ~128 escapes that guard: without the abort check, a + // subsequent fetch against the new context would see the leaked + // dimension in the accumulator. + const regionAlt = createDimension('region'); + let resolveFirstFetch: (value: any) => void; + const firstFetchPromise = new Promise((resolve) => { + resolveFirstFetch = resolve; + }); + + let fetchCallCount = 0; + mockExecuteEsqlQuery.mockImplementation(async () => { + fetchCallCount++; + if (fetchCallCount === 1) { + return firstFetchPromise; + } + return { + documents: [], + rawResponse: {}, + requestParams: { query: 'TS apm-* | METRICS_INFO' }, + }; + }); + + // Parse mocks are consumed in resolution order, not initiation order: + // 1st consumption -> fetch #2 (context B, resolves immediately) -> [hostDimension] + // 2nd consumption -> fetch #1 (context A, aborted, resolves last) -> [regionAlt] + // 3rd consumption -> fetch #3 (context B, after dim selection) -> [hostDimension] + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.memory.utilization'], [hostDimension]) + ); + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.cpu.utilization'], [regionAlt]) + ); + mockParseMetricsWithTelemetry.mockReturnValueOnce( + createMockParsedMetrics(['system.memory.utilization'], [hostDimension]) + ); + + const params = createDefaultParams(); + const { result, rerender } = renderHook( + (props: ReturnType) => useFetchMetricsData(props), + { initialProps: params } + ); + + await waitFor(() => { + expect(fetchCallCount).toBeGreaterThanOrEqual(1); + }); + + // Switch the data context. Effect cleanup aborts the first fetch's + // signal and a new fetch begins against context B. + const contextBParams = { + ...params, + fetchParams: { ...params.fetchParams, query: { esql: 'TS apm-*' } }, + }; + rerender(contextBParams); + + await waitFor(() => { + expect(fetchCallCount).toBe(2); + }); + + // Now resolve the first (aborted) fetch. Its parse callback runs and, + // without the abort guard, would write [regionAlt] into the accumulator. + resolveFirstFetch!({ + documents: [ + { + metric_name: 'system.cpu.utilization', + data_stream: 'metrics-*', + unit: null, + metric_type: 'gauge', + field_type: 'double', + dimension_fields: ['region'], + }, + ], + rawResponse: {}, + requestParams: { query: 'TS metrics-* | METRICS_INFO' }, + }); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); + }); + + // Trigger a third fetch within context B by changing only the + // dimension selection. The accumulator persists across this change + // and must not contain the stale [regionAlt] value from the aborted + // fetch that resolved after context B's first fetch. + rerender({ ...contextBParams, selectedDimensionNames: [hostDimension] }); + + await waitFor(() => { + expect(fetchCallCount).toBe(3); + }); + + await waitFor(() => { + // Without the abort guard, 'region' would leak in here. + expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); + }); + }); + it('resets accumulated dimensions when the timeRange changes', async () => { mockParseMetricsWithTelemetry.mockReturnValueOnce( createMockParsedMetrics(['system.cpu.utilization'], [hostDimension, serviceDimension]) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index a90171cddbe7e..ec5874e46b741 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -116,12 +116,17 @@ export function useFetchMetricsData({ // Merge newly-fetched dimensions with the accumulated set so that // dimensions from the unfiltered response are not lost when a - // WHERE filter narrows the result set. + // WHERE filter narrows the result set. Skip the ref write when this + // fetch has been aborted: a newer fetch may have already reset the ref + // for a new data context, and writing stale dimensions here would + // corrupt its accumulator. const mergedDimensions = mergeDimensions( accumulatedDimensionsRef.current, parsed.allDimensions ); - accumulatedDimensionsRef.current = mergedDimensions; + if (!signal.aborted) { + accumulatedDimensionsRef.current = mergedDimensions; + } const sortedMetrics: ParsedMetrics = { metricItems: [...parsed.metricItems].sort((a, b) => From 8c3dd8e088436a17e45c1c62b48482cfc7682c61 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Thu, 16 Apr 2026 15:45:37 -0400 Subject: [PATCH 07/19] refactor: extract accumulator into useAccumulatedDimensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the dimension-accumulator state and reset logic out of useFetchMetricsData into a dedicated hook. The new hook takes a DependencyList of reset keys and returns a stable merge function, which the caller invokes inside its useAsyncFn callback. Why the internals still use a ref rather than useState: useFetchMetricsData writes the merged dimensions into its own useAsyncFn return value, which already triggers a render. A useState-backed accumulator would fire a second, redundant render per fetch. Holding the accumulator in a ref and returning the merged value through the caller's existing state channel keeps it to a single render without exposing the ref to callers. What this replaces: - JSON.stringify(dataContextKey) for change detection — replaced with a cheap Object.is comparison over the caller-supplied resetDeps tuple, run inside a useEffect so there's no render-time mutation. - A pair of hand-managed refs in the fetch hook — now one call. signal.aborted is threaded through `merge(incoming, aborted)` so the prior guard against aborted fetches corrupting the accumulator is preserved. --- .../hooks/use_accumulated_dimensions.ts | 62 +++++++++++++++++++ .../metrics/hooks/use_fetch_metrics_data.ts | 50 +++++---------- 2 files changed, 77 insertions(+), 35 deletions(-) create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts new file mode 100644 index 0000000000000..3f11fbdf093c1 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts @@ -0,0 +1,62 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { DependencyList } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; +import type { Dimension } from '../../../../types'; +import { mergeDimensions } from '../utils/merge_dimensions'; + +/** + * Tracks a high-water-mark set of dimensions across multiple METRICS_INFO + * fetches so that selecting additional breakdown dimensions (which narrows the + * WHERE filter and can drop dimensions from the response) does not remove + * previously-available options from the picker. + * + * The accumulator is cleared whenever any entry in `resetDeps` changes — + * those deps describe the data context (query, time range, filters, ES|QL + * variables) where dropped dimensions represent a legitimate shape change, + * not a narrowing of the same dataset. + * + * The accumulator is held in a ref rather than useState. The caller writes the + * merged list into its own useAsyncFn return value, which already triggers a + * render; adding useState here would force a second, redundant render per + * fetch. + * + * @param resetDeps values that, when any changes by `Object.is`, reset the + * accumulator to empty + * @returns a stable `merge` function. Pass `aborted=true` to skip writing the + * ref when the owning fetch has been aborted (a newer fetch may have already + * reset the ref for a new data context). + */ +export function useAccumulatedDimensions( + resetDeps: DependencyList +): (incoming: Dimension[], aborted?: boolean) => Dimension[] { + const accumulatedRef = useRef([]); + const previousDepsRef = useRef(resetDeps); + + useEffect(() => { + const previous = previousDepsRef.current; + const changed = + previous.length !== resetDeps.length || + resetDeps.some((dep, index) => !Object.is(dep, previous[index])); + + if (changed) { + accumulatedRef.current = []; + previousDepsRef.current = resetDeps; + } + }); + + return useCallback((incoming: Dimension[], aborted: boolean = false): Dimension[] => { + const merged = mergeDimensions(accumulatedRef.current, incoming); + if (!aborted) { + accumulatedRef.current = merged; + } + return merged; + }, []); +} diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index ec5874e46b741..0588bf0f19d1a 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -8,7 +8,7 @@ */ import useAsyncFn from 'react-use/lib/useAsyncFn'; -import { useEffect, useMemo, useRef } from 'react'; +import { useEffect, useMemo } from 'react'; import type { ChartSectionProps } from '@kbn/unified-histogram/types'; import { buildMetricsInfoQuery, hasTransformationalCommand } from '@kbn/esql-utils'; import { getFieldIconType } from '@kbn/field-utils'; @@ -18,7 +18,7 @@ import { useChartSectionInspector } from '../../../../context/chart_section_insp import { executeEsqlQuery } from '../utils/execute_esql_query'; import { parseMetricsWithTelemetry } from '../utils/parse_metrics_response_with_telemetry'; import { getEsqlQuery } from '../utils/get_esql_query'; -import { mergeDimensions } from '../utils/merge_dimensions'; +import { useAccumulatedDimensions } from './use_accumulated_dimensions'; /** * Fetches METRICS_INFO when in Metrics Experience (non-transformational ES|QL, chart visible). @@ -55,31 +55,19 @@ export function useFetchMetricsData({ // Accumulate dimensions across filtered fetches so that selecting additional // breakdown dimensions does not remove previously-available dimensions from - // the picker. Reset when anything other than the selected dimensions changes - // the data context (query, time range, Discover filters, ES|QL variables), - // since those changes can legitimately remove dimensions from the result. - const accumulatedDimensionsRef = useRef([]); - const dataContextKey = useMemo( - () => - JSON.stringify({ - esql, - timeRange: fetchParams.timeRange, - filters: fetchParams.filters, - esqlVariables: fetchParams.esqlVariables, - }), - [esql, fetchParams.timeRange, fetchParams.filters, fetchParams.esqlVariables] - ); - const previousDataContextKeyRef = useRef(dataContextKey); + // the picker. Reset on any data-context change (query, time range, Discover + // filters, ES|QL variables), since those can legitimately remove dimensions. + const mergeAccumulatedDimensions = useAccumulatedDimensions([ + esql, + fetchParams.timeRange, + fetchParams.filters, + fetchParams.esqlVariables, + ]); const [{ value, error, loading }, executeFetch] = useAsyncFn( async ( signal: AbortSignal ): Promise<(ParsedMetrics & { activeDimensions: Dimension[] }) | null> => { - if (dataContextKey !== previousDataContextKeyRef.current) { - accumulatedDimensionsRef.current = []; - previousDataContextKeyRef.current = dataContextKey; - } - const documents = await trackRequest( 'Grid of metrics', 'This request queries Elasticsearch to fetch metrics info for the grid.', @@ -115,18 +103,10 @@ export function useFetchMetricsData({ const parsed = parseMetricsWithTelemetry(documents, getFieldType); // Merge newly-fetched dimensions with the accumulated set so that - // dimensions from the unfiltered response are not lost when a - // WHERE filter narrows the result set. Skip the ref write when this - // fetch has been aborted: a newer fetch may have already reset the ref - // for a new data context, and writing stale dimensions here would - // corrupt its accumulator. - const mergedDimensions = mergeDimensions( - accumulatedDimensionsRef.current, - parsed.allDimensions - ); - if (!signal.aborted) { - accumulatedDimensionsRef.current = mergedDimensions; - } + // dimensions from the unfiltered response are not lost when a WHERE + // filter narrows the result set. Pass signal.aborted so the hook can + // skip persisting dimensions from a fetch whose data context is gone. + const mergedDimensions = mergeAccumulatedDimensions(parsed.allDimensions, signal.aborted); const sortedMetrics: ParsedMetrics = { metricItems: [...parsed.metricItems].sort((a, b) => @@ -146,7 +126,7 @@ export function useFetchMetricsData({ }, [ metricsInfoQuery, - dataContextKey, + mergeAccumulatedDimensions, trackRequest, fetchParams.dataView, fetchParams.timeRange, From 51a62970ceeb8710a135d39e8bafa7330100fc67 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 11:08:37 -0400 Subject: [PATCH 08/19] Move dimension sort out of mergeDimensions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mergeDimensions was doing two things — set-merge and alphabetical sorting — which coupled an incidental caller concern (picker order) with the core utility. Move the sort back to useFetchMetricsData where it sits next to the metricItems sort, keeping mergeDimensions focused on merge semantics so a future sort-policy change has a single home. Addresses lucaslopezf's review: https://github.com/elastic/kibana/pull/263629#discussion_r3099383050 --- .../metrics/hooks/use_fetch_metrics_data.ts | 2 +- .../metrics/utils/merge_dimensions.test.ts | 30 +++++++++---------- .../metrics/utils/merge_dimensions.ts | 8 +++-- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index 0588bf0f19d1a..2b49f1a06a163 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -112,7 +112,7 @@ export function useFetchMetricsData({ metricItems: [...parsed.metricItems].sort((a, b) => a.metricName.localeCompare(b.metricName) ), - allDimensions: mergedDimensions, + allDimensions: [...mergedDimensions].sort((a, b) => a.name.localeCompare(b.name)), }; if (!signal.aborted) { diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts index 4735c2bec7815..ba10ae3bebf5b 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts @@ -10,17 +10,25 @@ import { mergeDimensions } from './merge_dimensions'; import type { Dimension } from '../../../../types'; +// Helpers to compare results as sets (mergeDimensions does not guarantee +// ordering; sorting is the caller's responsibility). +const byName = (a: Dimension, b: Dimension) => a.name.localeCompare(b.name); +const sorted = (list: Dimension[]) => [...list].sort(byName); + describe('mergeDimensions', () => { const dim = (name: string, type?: string): Dimension => ({ name, type }); it('returns incoming dimensions when accumulated is empty', () => { const incoming = [dim('host.name'), dim('environment')]; - expect(mergeDimensions([], incoming)).toEqual([dim('environment'), dim('host.name')]); + expect(sorted(mergeDimensions([], incoming))).toEqual([dim('environment'), dim('host.name')]); }); it('returns accumulated dimensions when incoming is empty', () => { const accumulated = [dim('host.name'), dim('environment')]; - expect(mergeDimensions(accumulated, [])).toEqual([dim('environment'), dim('host.name')]); + expect(sorted(mergeDimensions(accumulated, []))).toEqual([ + dim('environment'), + dim('host.name'), + ]); }); it('returns empty array when both are empty', () => { @@ -37,7 +45,7 @@ describe('mergeDimensions', () => { const result = mergeDimensions(allDimensions, filteredDimensions); // region should still be present - expect(result).toEqual([dim('environment'), dim('host.name'), dim('region')]); + expect(sorted(result)).toEqual([dim('environment'), dim('host.name'), dim('region')]); expect(result.map((d) => d.name)).toContain('region'); }); @@ -46,7 +54,7 @@ describe('mergeDimensions', () => { const incoming = [dim('host.name'), dim('region')]; const result = mergeDimensions(accumulated, incoming); - expect(result).toEqual([dim('environment'), dim('host.name'), dim('region')]); + expect(sorted(result)).toEqual([dim('environment'), dim('host.name'), dim('region')]); // No duplicates const names = result.map((d) => d.name); @@ -63,29 +71,21 @@ describe('mergeDimensions', () => { expect(hostDim?.type).toBe('ip'); }); - it('sorts results alphabetically by name', () => { - const accumulated = [dim('z_field'), dim('a_field')]; - const incoming = [dim('m_field')]; - - const result = mergeDimensions(accumulated, incoming); - expect(result.map((d) => d.name)).toEqual(['a_field', 'm_field', 'z_field']); - }); - it('handles the full multi-step selection scenario', () => { // Step 1: initial unfiltered query const step1 = [dim('environment'), dim('host.name'), dim('region')]; let accumulated = mergeDimensions([], step1); - expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); // Step 2: user selects environment, filtered query returns environment + host.name // (region is on a metric that may not have environment) const step2 = [dim('environment'), dim('host.name')]; accumulated = mergeDimensions(accumulated, step2); - expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); // Step 3: user also selects host.name, filtered query returns only environment + host.name const step3 = [dim('environment'), dim('host.name')]; accumulated = mergeDimensions(accumulated, step3); - expect(accumulated.map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); + expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); }); }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts index 0498750cb7a84..9804069a9bc32 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts @@ -17,9 +17,13 @@ import type { Dimension } from '../../../../types'; * multiple breakdown dimensions, which causes the METRICS_INFO query to add * WHERE filters that narrow the result set. * + * Ordering of the returned list is not guaranteed. Callers that need a + * specific order (e.g. alphabetical for a picker) are expected to sort the + * result themselves. + * * @param accumulated - Previously accumulated dimensions (the "high-water mark") * @param incoming - Dimensions from the latest METRICS_INFO response - * @returns Merged dimension list with no duplicates, sorted alphabetically + * @returns Merged dimension list with no duplicates */ export const mergeDimensions = (accumulated: Dimension[], incoming: Dimension[]): Dimension[] => { const merged = new Map(); @@ -33,5 +37,5 @@ export const mergeDimensions = (accumulated: Dimension[], incoming: Dimension[]) merged.set(dim.name, dim); } - return Array.from(merged.values()).sort((a, b) => a.name.localeCompare(b.name)); + return Array.from(merged.values()); }; From 1eb2379e36d10684177b07dc6cd4c11fdae61c1d Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 11:45:28 -0400 Subject: [PATCH 09/19] revert: remove dimension accumulator The accumulator was the wrong fix for #263309. The ticket asks the picker to be tailored to the current grid (AC1) and to surface currently-selected dimensions even when the refreshed response drops them (AC2/AC3). The accumulator did the opposite: it preserved every dimension ever seen, including ones not applicable to the current grid. Strip it out so a targeted fix in DimensionsSelector can take its place. - Delete utils/merge_dimensions.{ts,test.ts} - Delete hooks/use_accumulated_dimensions.ts - use_fetch_metrics_data.ts: sort parsed.allDimensions directly, drop mergeAccumulatedDimensions from the useAsyncFn deps - use_fetch_metrics_data.test.ts: remove the "dimension accumulator" suite; the remaining tests exercise the now-direct sort and the existing refetch/race-condition behaviours --- .../hooks/use_accumulated_dimensions.ts | 62 ----- .../hooks/use_fetch_metrics_data.test.ts | 221 ------------------ .../metrics/hooks/use_fetch_metrics_data.ts | 21 +- .../metrics/utils/merge_dimensions.test.ts | 91 -------- .../metrics/utils/merge_dimensions.ts | 41 ---- 5 files changed, 1 insertion(+), 435 deletions(-) delete mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts delete mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts delete mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts deleted file mode 100644 index 3f11fbdf093c1..0000000000000 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_accumulated_dimensions.ts +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the "Elastic License - * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side - * Public License v 1"; you may not use this file except in compliance with, at - * your election, the "Elastic License 2.0", the "GNU Affero General Public - * License v3.0 only", or the "Server Side Public License, v 1". - */ - -import type { DependencyList } from 'react'; -import { useCallback, useEffect, useRef } from 'react'; -import type { Dimension } from '../../../../types'; -import { mergeDimensions } from '../utils/merge_dimensions'; - -/** - * Tracks a high-water-mark set of dimensions across multiple METRICS_INFO - * fetches so that selecting additional breakdown dimensions (which narrows the - * WHERE filter and can drop dimensions from the response) does not remove - * previously-available options from the picker. - * - * The accumulator is cleared whenever any entry in `resetDeps` changes — - * those deps describe the data context (query, time range, filters, ES|QL - * variables) where dropped dimensions represent a legitimate shape change, - * not a narrowing of the same dataset. - * - * The accumulator is held in a ref rather than useState. The caller writes the - * merged list into its own useAsyncFn return value, which already triggers a - * render; adding useState here would force a second, redundant render per - * fetch. - * - * @param resetDeps values that, when any changes by `Object.is`, reset the - * accumulator to empty - * @returns a stable `merge` function. Pass `aborted=true` to skip writing the - * ref when the owning fetch has been aborted (a newer fetch may have already - * reset the ref for a new data context). - */ -export function useAccumulatedDimensions( - resetDeps: DependencyList -): (incoming: Dimension[], aborted?: boolean) => Dimension[] { - const accumulatedRef = useRef([]); - const previousDepsRef = useRef(resetDeps); - - useEffect(() => { - const previous = previousDepsRef.current; - const changed = - previous.length !== resetDeps.length || - resetDeps.some((dep, index) => !Object.is(dep, previous[index])); - - if (changed) { - accumulatedRef.current = []; - previousDepsRef.current = resetDeps; - } - }); - - return useCallback((incoming: Dimension[], aborted: boolean = false): Dimension[] => { - const merged = mergeDimensions(accumulatedRef.current, incoming); - if (!aborted) { - accumulatedRef.current = merged; - } - return merged; - }, []); -} diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts index e6df89240862f..584dad45fafb5 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts @@ -518,225 +518,4 @@ describe('useFetchMetricsData', () => { }); }); - describe('dimension accumulator', () => { - const regionDimension = createDimension('region'); - - it('preserves previously-seen dimensions when a subsequent fetch drops them', async () => { - // First fetch: full dimension set (unfiltered) - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics( - ['system.cpu.utilization'], - [regionDimension, hostDimension, serviceDimension] - ) - ); - - const params = createDefaultParams(); - const { result, rerender } = renderHook( - (props: ReturnType) => useFetchMetricsData(props), - { initialProps: params } - ); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - expect(result.current.allDimensions.map((d) => d.name)).toEqual([ - 'host.name', - 'region', - 'service.name', - ]); - }); - - // Second fetch (user selected host.name, query narrows): region is dropped - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.cpu.utilization'], [hostDimension, serviceDimension]) - ); - - rerender({ ...params, selectedDimensionNames: [hostDimension] }); - - await waitFor(() => { - expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); - }); - - await waitFor(() => { - // Accumulator keeps region visible so the user can still pick it - expect(result.current.allDimensions.map((d) => d.name)).toEqual([ - 'host.name', - 'region', - 'service.name', - ]); - }); - }); - - it('resets accumulated dimensions when the ES|QL query changes', async () => { - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.cpu.utilization'], [hostDimension]) - ); - - const params = createDefaultParams(); - const { result, rerender } = renderHook( - (props: ReturnType) => useFetchMetricsData(props), - { initialProps: params } - ); - - await waitFor(() => { - expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); - }); - - // Switch data source; new query returns a different dimension set - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.memory.utilization'], [regionDimension]) - ); - - rerender({ - ...params, - fetchParams: { ...params.fetchParams, query: { esql: 'TS apm-*' } }, - }); - - await waitFor(() => { - expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); - }); - - await waitFor(() => { - // Accumulator is reset: old dimensions from the previous data source are gone - expect(result.current.allDimensions.map((d) => d.name)).toEqual(['region']); - }); - }); - - it('does not let an aborted fetch corrupt the accumulator for a later fetch', async () => { - // Reproduces the race where an in-flight fetch resolves *after* its - // signal has been aborted (because a data-context change triggered - // effect cleanup and a new fetch). useAsyncFn's internal stale-call - // tracking discards the aborted fetch's returned value, but the ref - // write on line ~128 escapes that guard: without the abort check, a - // subsequent fetch against the new context would see the leaked - // dimension in the accumulator. - const regionAlt = createDimension('region'); - let resolveFirstFetch: (value: any) => void; - const firstFetchPromise = new Promise((resolve) => { - resolveFirstFetch = resolve; - }); - - let fetchCallCount = 0; - mockExecuteEsqlQuery.mockImplementation(async () => { - fetchCallCount++; - if (fetchCallCount === 1) { - return firstFetchPromise; - } - return { - documents: [], - rawResponse: {}, - requestParams: { query: 'TS apm-* | METRICS_INFO' }, - }; - }); - - // Parse mocks are consumed in resolution order, not initiation order: - // 1st consumption -> fetch #2 (context B, resolves immediately) -> [hostDimension] - // 2nd consumption -> fetch #1 (context A, aborted, resolves last) -> [regionAlt] - // 3rd consumption -> fetch #3 (context B, after dim selection) -> [hostDimension] - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.memory.utilization'], [hostDimension]) - ); - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.cpu.utilization'], [regionAlt]) - ); - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.memory.utilization'], [hostDimension]) - ); - - const params = createDefaultParams(); - const { result, rerender } = renderHook( - (props: ReturnType) => useFetchMetricsData(props), - { initialProps: params } - ); - - await waitFor(() => { - expect(fetchCallCount).toBeGreaterThanOrEqual(1); - }); - - // Switch the data context. Effect cleanup aborts the first fetch's - // signal and a new fetch begins against context B. - const contextBParams = { - ...params, - fetchParams: { ...params.fetchParams, query: { esql: 'TS apm-*' } }, - }; - rerender(contextBParams); - - await waitFor(() => { - expect(fetchCallCount).toBe(2); - }); - - // Now resolve the first (aborted) fetch. Its parse callback runs and, - // without the abort guard, would write [regionAlt] into the accumulator. - resolveFirstFetch!({ - documents: [ - { - metric_name: 'system.cpu.utilization', - data_stream: 'metrics-*', - unit: null, - metric_type: 'gauge', - field_type: 'double', - dimension_fields: ['region'], - }, - ], - rawResponse: {}, - requestParams: { query: 'TS metrics-* | METRICS_INFO' }, - }); - - await waitFor(() => { - expect(result.current.loading).toBe(false); - expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); - }); - - // Trigger a third fetch within context B by changing only the - // dimension selection. The accumulator persists across this change - // and must not contain the stale [regionAlt] value from the aborted - // fetch that resolved after context B's first fetch. - rerender({ ...contextBParams, selectedDimensionNames: [hostDimension] }); - - await waitFor(() => { - expect(fetchCallCount).toBe(3); - }); - - await waitFor(() => { - // Without the abort guard, 'region' would leak in here. - expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); - }); - }); - - it('resets accumulated dimensions when the timeRange changes', async () => { - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.cpu.utilization'], [hostDimension, serviceDimension]) - ); - - const params = createDefaultParams(); - const { result, rerender } = renderHook( - (props: ReturnType) => useFetchMetricsData(props), - { initialProps: params } - ); - - await waitFor(() => { - expect(result.current.allDimensions.map((d) => d.name)).toEqual([ - 'host.name', - 'service.name', - ]); - }); - - // New time range legitimately drops service.name from the response - mockParseMetricsWithTelemetry.mockReturnValueOnce( - createMockParsedMetrics(['system.cpu.utilization'], [hostDimension]) - ); - - rerender({ - ...params, - fetchParams: { ...params.fetchParams, timeRange: { from: 'now-1h', to: 'now' } }, - }); - - await waitFor(() => { - expect(mockExecuteEsqlQuery).toHaveBeenCalledTimes(2); - }); - - await waitFor(() => { - expect(result.current.allDimensions.map((d) => d.name)).toEqual(['host.name']); - }); - }); - }); }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts index 2b49f1a06a163..bc4fb8729da4a 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.ts @@ -18,7 +18,6 @@ import { useChartSectionInspector } from '../../../../context/chart_section_insp import { executeEsqlQuery } from '../utils/execute_esql_query'; import { parseMetricsWithTelemetry } from '../utils/parse_metrics_response_with_telemetry'; import { getEsqlQuery } from '../utils/get_esql_query'; -import { useAccumulatedDimensions } from './use_accumulated_dimensions'; /** * Fetches METRICS_INFO when in Metrics Experience (non-transformational ES|QL, chart visible). @@ -53,17 +52,6 @@ export function useFetchMetricsData({ [esql, selectedDimensions] ); - // Accumulate dimensions across filtered fetches so that selecting additional - // breakdown dimensions does not remove previously-available dimensions from - // the picker. Reset on any data-context change (query, time range, Discover - // filters, ES|QL variables), since those can legitimately remove dimensions. - const mergeAccumulatedDimensions = useAccumulatedDimensions([ - esql, - fetchParams.timeRange, - fetchParams.filters, - fetchParams.esqlVariables, - ]); - const [{ value, error, loading }, executeFetch] = useAsyncFn( async ( signal: AbortSignal @@ -102,17 +90,11 @@ export function useFetchMetricsData({ const parsed = parseMetricsWithTelemetry(documents, getFieldType); - // Merge newly-fetched dimensions with the accumulated set so that - // dimensions from the unfiltered response are not lost when a WHERE - // filter narrows the result set. Pass signal.aborted so the hook can - // skip persisting dimensions from a fetch whose data context is gone. - const mergedDimensions = mergeAccumulatedDimensions(parsed.allDimensions, signal.aborted); - const sortedMetrics: ParsedMetrics = { metricItems: [...parsed.metricItems].sort((a, b) => a.metricName.localeCompare(b.metricName) ), - allDimensions: [...mergedDimensions].sort((a, b) => a.name.localeCompare(b.name)), + allDimensions: [...parsed.allDimensions].sort((a, b) => a.name.localeCompare(b.name)), }; if (!signal.aborted) { @@ -126,7 +108,6 @@ export function useFetchMetricsData({ }, [ metricsInfoQuery, - mergeAccumulatedDimensions, trackRequest, fetchParams.dataView, fetchParams.timeRange, diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts deleted file mode 100644 index ba10ae3bebf5b..0000000000000 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.test.ts +++ /dev/null @@ -1,91 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the "Elastic License - * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side - * Public License v 1"; you may not use this file except in compliance with, at - * your election, the "Elastic License 2.0", the "GNU Affero General Public - * License v3.0 only", or the "Server Side Public License, v 1". - */ - -import { mergeDimensions } from './merge_dimensions'; -import type { Dimension } from '../../../../types'; - -// Helpers to compare results as sets (mergeDimensions does not guarantee -// ordering; sorting is the caller's responsibility). -const byName = (a: Dimension, b: Dimension) => a.name.localeCompare(b.name); -const sorted = (list: Dimension[]) => [...list].sort(byName); - -describe('mergeDimensions', () => { - const dim = (name: string, type?: string): Dimension => ({ name, type }); - - it('returns incoming dimensions when accumulated is empty', () => { - const incoming = [dim('host.name'), dim('environment')]; - expect(sorted(mergeDimensions([], incoming))).toEqual([dim('environment'), dim('host.name')]); - }); - - it('returns accumulated dimensions when incoming is empty', () => { - const accumulated = [dim('host.name'), dim('environment')]; - expect(sorted(mergeDimensions(accumulated, []))).toEqual([ - dim('environment'), - dim('host.name'), - ]); - }); - - it('returns empty array when both are empty', () => { - expect(mergeDimensions([], [])).toEqual([]); - }); - - it('preserves accumulated dimensions that are missing from incoming (the bug scenario)', () => { - // Step 1: unfiltered query returns all 3 dimensions - const allDimensions = [dim('environment'), dim('host.name'), dim('region')]; - - // Step 2: user selects environment + host.name, filtered query returns only 2 - const filteredDimensions = [dim('environment'), dim('host.name')]; - - const result = mergeDimensions(allDimensions, filteredDimensions); - - // region should still be present - expect(sorted(result)).toEqual([dim('environment'), dim('host.name'), dim('region')]); - expect(result.map((d) => d.name)).toContain('region'); - }); - - it('deduplicates dimensions by name', () => { - const accumulated = [dim('host.name'), dim('environment')]; - const incoming = [dim('host.name'), dim('region')]; - - const result = mergeDimensions(accumulated, incoming); - expect(sorted(result)).toEqual([dim('environment'), dim('host.name'), dim('region')]); - - // No duplicates - const names = result.map((d) => d.name); - expect(new Set(names).size).toBe(names.length); - }); - - it('incoming dimensions update type info for existing entries', () => { - const accumulated = [dim('host.name', 'keyword'), dim('environment')]; - const incoming = [dim('host.name', 'ip')]; - - const result = mergeDimensions(accumulated, incoming); - - const hostDim = result.find((d) => d.name === 'host.name'); - expect(hostDim?.type).toBe('ip'); - }); - - it('handles the full multi-step selection scenario', () => { - // Step 1: initial unfiltered query - const step1 = [dim('environment'), dim('host.name'), dim('region')]; - let accumulated = mergeDimensions([], step1); - expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); - - // Step 2: user selects environment, filtered query returns environment + host.name - // (region is on a metric that may not have environment) - const step2 = [dim('environment'), dim('host.name')]; - accumulated = mergeDimensions(accumulated, step2); - expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); - - // Step 3: user also selects host.name, filtered query returns only environment + host.name - const step3 = [dim('environment'), dim('host.name')]; - accumulated = mergeDimensions(accumulated, step3); - expect(sorted(accumulated).map((d) => d.name)).toEqual(['environment', 'host.name', 'region']); - }); -}); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts deleted file mode 100644 index 9804069a9bc32..0000000000000 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/utils/merge_dimensions.ts +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the "Elastic License - * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side - * Public License v 1"; you may not use this file except in compliance with, at - * your election, the "Elastic License 2.0", the "GNU Affero General Public - * License v3.0 only", or the "Server Side Public License, v 1". - */ - -import type { Dimension } from '../../../../types'; - -/** - * Merges new dimensions into an accumulated set, preserving previously-seen - * dimensions that may no longer appear in filtered METRICS_INFO responses. - * - * This prevents the dimension picker from losing options when the user selects - * multiple breakdown dimensions, which causes the METRICS_INFO query to add - * WHERE filters that narrow the result set. - * - * Ordering of the returned list is not guaranteed. Callers that need a - * specific order (e.g. alphabetical for a picker) are expected to sort the - * result themselves. - * - * @param accumulated - Previously accumulated dimensions (the "high-water mark") - * @param incoming - Dimensions from the latest METRICS_INFO response - * @returns Merged dimension list with no duplicates - */ -export const mergeDimensions = (accumulated: Dimension[], incoming: Dimension[]): Dimension[] => { - const merged = new Map(); - - for (const dim of accumulated) { - merged.set(dim.name, dim); - } - - // Incoming dimensions may have updated type info, so they take precedence - for (const dim of incoming) { - merged.set(dim.name, dim); - } - - return Array.from(merged.values()); -}; From 290aaf17e182ee730d47a745a0dc04c8c4dcad76 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 11:48:48 -0400 Subject: [PATCH 10/19] feat(picker): surface selected dimensions even when not in applicable set Fixes ticket #263309 AC2/AC3: when the user breaks down by multiple dimensions and the latest METRICS_INFO response drops one of their selections from the applicable set (because the server-side `WHERE dim IS NOT NULL` filter narrowed the matching metrics), the picker now keeps that "orphan" selection visible at the top of the list so the count badge still matches the rendered ticks and the user can always back out of an invalid combination. - dimensions_selector.tsx: compute orphan selections (localSelected minus applicable names), prepend them to the options list in alphabetical order with `checked: 'on'`, and update handleChange to resolve Dimension records from the union of applicable + locally-selected so deselecting an orphan still produces a well-formed new selection array. - dimensions_selector.test.tsx: extend the ToolbarSelector mock to simulate the real multi-selection toggle contract (click a checked option -> emit the remaining checked options as an array) so the new tests exercise the component's handleChange with the same payload shape it gets at runtime. Add a "Selected dimensions not in applicable set" suite covering visibility, ordering, popover count, and orphan deselection. --- .../hooks/use_fetch_metrics_data.test.ts | 1 - .../toolbar/dimensions_selector.test.tsx | 223 +++++++++++++++--- .../toolbar/dimensions_selector.tsx | 33 ++- 3 files changed, 216 insertions(+), 41 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts index 584dad45fafb5..40fffbdf11809 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/hooks/use_fetch_metrics_data.test.ts @@ -517,5 +517,4 @@ describe('useFetchMetricsData', () => { ); }); }); - }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx index 026b4f9b935e4..851ad09b05a7f 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx @@ -37,42 +37,65 @@ jest.mock('@kbn/shared-ux-toolbar-selector', () => { popoverContentBelowSearch?: React.ReactNode; 'data-test-subj'?: string; singleSelection?: boolean; - }) => ( -
-
- {buttonLabel} - {buttonTooltipContent && ( -
{buttonTooltipContent}
- )} -
-
- {popoverContentBelowSearch} - {options.map((option) => ( -
!option.disabled && onChange?.(option)} - onKeyDown={(e) => { - if ((e.key === 'Enter' || e.key === ' ') && !option.disabled) { - e.preventDefault(); - onChange?.(option); - } - }} - role="option" - aria-selected={option.checked === 'on'} - tabIndex={option.disabled ? -1 : 0} - > - {option.label} -
- ))} + }) => { + // Simulate the real ToolbarSelector multi-selection semantics: clicking + // an option toggles its checked state and emits the full array of + // currently-checked options. For single selection, emit just the clicked + // option. This matches the behaviour implemented in toolbar_selector.tsx + // so that tests exercise the component's handleChange with the same + // payload shape it receives at runtime. + const handleOptionClick = (clickedOption: any) => { + if (clickedOption.disabled) return; + if (singleSelection) { + onChange?.(clickedOption); + return; + } + const wasChecked = clickedOption.checked === 'on'; + const nextSelected = options + .filter((option) => { + if (option.value === clickedOption.value) return !wasChecked; + return option.checked === 'on'; + }) + .map((option) => ({ ...option, checked: 'on' })); + onChange?.(nextSelected); + }; + return ( +
+
+ {buttonLabel} + {buttonTooltipContent && ( +
{buttonTooltipContent}
+ )} +
+
+ {popoverContentBelowSearch} + {options.map((option) => ( +
handleOptionClick(option)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleOptionClick(option); + } + }} + role="option" + aria-selected={option.checked === 'on'} + tabIndex={option.disabled ? -1 : 0} + > + {option.label} +
+ ))} +
-
- ), + ); + }, }; }); @@ -386,4 +409,134 @@ describe('DimensionsSelector', () => { expect(selector).toBeInTheDocument(); }); }); + + describe('Selected dimensions not in applicable set (race-condition guard)', () => { + // AC2 from ticket #263309: a dimension that the user has already selected + // must remain visible in the picker even if the latest METRICS_INFO + // response drops it from `dimensions` (the applicable set). This covers + // the case where the user selects two dimensions before the grid + // refreshes and the server returns no metrics matching both. + const applicableOnly = [{ name: 'b' }, { name: 'c' }] as Dimension[]; + const orphanPlusApplicable = [{ name: 'a' }, { name: 'b' }] as Dimension[]; + + it('shows selected dimensions even when they are not in the dimensions prop', () => { + renderWithIntl( + + ); + + const orphanOption = screen.getByTestId( + `${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-a` + ); + const applicableOption = screen.getByTestId( + `${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-b` + ); + expect(orphanOption).toBeInTheDocument(); + expect(applicableOption).toBeInTheDocument(); + expect(orphanOption).toHaveAttribute('data-checked', 'on'); + expect(applicableOption).toHaveAttribute('data-checked', 'on'); + }); + + it('reflects both orphan and applicable selections in the popover count (AC3)', () => { + renderWithIntl( + + ); + const popover = screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Popover`); + expect(popover).toHaveTextContent('2 dimensions selected'); + + const button = screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Button`); + expect(button).toHaveTextContent('2'); + }); + + it('renders orphan selections before applicable options', () => { + renderWithIntl( + + ); + const optionElements = screen.getAllByRole('option'); + const names = optionElements.map((el) => el.textContent); + // Orphan selection 'a' should come before applicable options 'b'/'c'. + expect(names.indexOf('a')).toBeLessThan(names.indexOf('b')); + expect(names.indexOf('a')).toBeLessThan(names.indexOf('c')); + }); + + it('sorts multiple orphan selections alphabetically amongst themselves', () => { + const dimensions = [{ name: 'z' }] as Dimension[]; + const selected = [{ name: 'q' }, { name: 'a' }, { name: 'z' }] as Dimension[]; + + renderWithIntl( + + ); + + const optionElements = screen.getAllByRole('option'); + const names = optionElements.map((el) => el.textContent); + // Orphan selections a, q should appear before applicable z, and in + // alphabetical order relative to each other. + expect(names).toEqual(['a', 'q', 'z']); + }); + + it('deselecting an orphan selection calls onChange with the remaining selections', async () => { + const onChange = jest.fn(); + renderWithIntl( + + ); + + const orphanOption = screen.getByTestId( + `${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-a` + ); + // Toggle off the orphan. The mock toolbar selector re-invokes onChange + // with the whole list of still-checked options; it reports the clicked + // option as `checked: 'on'` in its payload, so we need to verify the + // real component's handleChange strips off the toggled-off option. + fireEvent.click(orphanOption); + + await waitFor(() => { + expect(onChange).toHaveBeenCalled(); + }); + // The last call should not contain dimension 'a' any longer. + const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1]; + const names = lastCall[0].map((d: Dimension) => d.name); + expect(names).not.toContain('a'); + expect(names).toContain('b'); + }); + + it('renders only applicable options when no orphan selections exist', () => { + renderWithIntl( + + ); + + expect( + screen.queryByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-a`) + ).not.toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-b`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-c`) + ).toBeInTheDocument(); + }); + }); }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx index 3e928790f297d..7a74a9613a9a7 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx @@ -64,8 +64,19 @@ export const DimensionsSelector = ({ const options: SelectableEntry[] = useMemo(() => { const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; + const applicableNames = new Set(dimensions.map((d) => d.name)); - const mappedOptions = dimensions.map((dimension) => { + // Orphan selections are dimensions the user has already picked but that + // are not in the current applicable set (e.g. a filtered METRICS_INFO + // response dropped them). Per AC2/AC3 of ticket #263309 they must remain + // visible and toggleable so the count badge matches the rendered ticks + // and the user can always back out of an invalid combination. + const orphanSelections = localSelectedDimensions + .filter((dimension) => !applicableNames.has(dimension.name)) + .slice() + .sort((a, b) => a.name.localeCompare(b.name)); + + const toOption = (dimension: Dimension): SelectableEntry => { const isSelected = selectedNamesSet.has(dimension.name); const isDisabled = getOptionDisabledState({ @@ -113,9 +124,11 @@ export const DimensionsSelector = ({ } return option; - }); + }; - return mappedOptions; + // Orphan selections are prepended so they stay easy to find; the + // applicable set keeps its caller-provided ordering below. + return [...orphanSelections.map(toOption), ...dimensions.map(toOption)]; }, [ dimensions, selectedNamesSet, @@ -151,8 +164,18 @@ export const DimensionsSelector = ({ (chosenOption?: SelectableEntry | SelectableEntry[]) => { const opts = chosenOption == null ? [] : Array.isArray(chosenOption) ? chosenOption : [chosenOption]; + // Look up dimensions from both the applicable set and the current local + // selection: this lets us preserve orphan selections (those not in the + // current applicable set) when the user toggles another option. + const dimensionByName = new Map(); + for (const dimension of localSelectedDimensions) { + dimensionByName.set(dimension.name, dimension); + } + for (const dimension of dimensions) { + dimensionByName.set(dimension.name, dimension); + } const newSelection = opts - .map((opt) => dimensions.find((d) => d.name === opt.value)) + .map((opt) => dimensionByName.get(opt.value)) .filter((d): d is Dimension => d !== undefined) .slice(0, MAX_DIMENSIONS_SELECTIONS); @@ -166,7 +189,7 @@ export const DimensionsSelector = ({ debouncedOnChange(newSelection); } }, - [onChange, dimensions, singleSelection, debouncedOnChange] + [onChange, dimensions, localSelectedDimensions, singleSelection, debouncedOnChange] ); const handleClearAll = useCallback(() => { From 491456acc86473b67177f6da7aae79b7a3db8de3 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 15:00:19 -0400 Subject: [PATCH 11/19] feat(picker): optimistically filter options by current selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 6 of #263309 rework. After the user clicks the first dimension, the picker now narrows its option list client-side to dimensions carried by at least one metric that also carries every current selection — before the debounced fetch fires. This prevents the throttled-network rapid-click race where the second click could land on a dimension disjoint from the first and drop the grid into a "No results" state. The orphan-surfacing path (Phase 2) stays as a safety net for URL-restore and any server edge case where the prediction disagrees with the real response. - DimensionsSelector: new optional metricItems prop; options useMemo filters dimensions against the optimistic applicable-name set when both a selection and metricItems are provided. - useToolbarActions + MetricsExperienceGrid: forward metricItems (the pre-search-filter set from useFetchMetricsData) into the selector. - Tests: 4 new cases covering the filter, the no-metricItems fallback, orphan composition, and the empty-selection no-op. --- .../metrics/metrics_experience_grid.tsx | 4 + .../toolbar/dimensions_selector.test.tsx | 130 +++++++++++++++++- .../toolbar/dimensions_selector.tsx | 55 +++++++- .../toolbar/hooks/use_toolbar_actions.tsx | 12 +- 4 files changed, 196 insertions(+), 5 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx index 9f19488b0676f..15eff3ea693e6 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx @@ -103,6 +103,10 @@ export const MetricsExperienceGrid = ({ const { toggleActions, leftSideActions, rightSideActions } = useToolbarActions({ allDimensions, + // Use the pre-search-filter metric set for the picker's optimistic filter: + // a user's typed search term shouldn't shrink the underlying applicable- + // dimension universe — only the actual selection should. + metricItems, renderToggleActions, onDimensionsChange: onToolbarDimensionsChange, isLoading: isDiscoverLoading, diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx index 851ad09b05a7f..9e273e1801327 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx @@ -11,7 +11,7 @@ import React from 'react'; import { render, screen, fireEvent, waitFor } from '@testing-library/react'; import { __IntlProvider as IntlProvider } from '@kbn/i18n-react'; import { DimensionsSelector } from './dimensions_selector'; -import type { Dimension } from '../../types'; +import type { Dimension, ParsedMetricItem } from '../../types'; import { MAX_DIMENSIONS_SELECTIONS, METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ, @@ -539,4 +539,132 @@ describe('DimensionsSelector', () => { ).toBeInTheDocument(); }); }); + + describe('Optimistic filter via metricItems (Phase 6)', () => { + // These tests cover the client-side optimistic filtering that prevents the + // empty-grid dead-state described in PR #263629 review feedback: if the + // user clicks one dimension, the picker must immediately hide dimensions + // that no metric carrying that selection supports — without waiting for + // the server round-trip. + const environment = { name: 'environment' } as Dimension; + const region = { name: 'region' } as Dimension; + const hostName = { name: 'host.name' } as Dimension; + + const buildMetricItem = ( + metricName: string, + dimensionFields: Dimension[] + ): ParsedMetricItem => ({ + metricName, + dataStream: 'metrics-test', + units: [], + metricTypes: [], + fieldTypes: [], + dimensionFields, + }); + + // cpu.usage carries `environment` + `host.name`. + // network.bytes_in carries `region` + `host.name`. + // No metric carries both `environment` and `region`. + const metricItems: ParsedMetricItem[] = [ + buildMetricItem('cpu.usage', [environment, hostName]), + buildMetricItem('network.bytes_in', [region, hostName]), + ]; + + const applicableDimensions: Dimension[] = [environment, region, hostName]; + + it('hides dimensions not supported by any metric carrying the current selection', () => { + renderWithIntl( + + ); + + // `region` is disjoint from `environment` (no single metric carries + // both) so the picker must hide it optimistically, before the debounced + // onChange fires and the server responds. + expect( + screen.queryByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-region`) + ).not.toBeInTheDocument(); + + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-environment`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-host.name`) + ).toBeInTheDocument(); + }); + + it('without metricItems, falls back to the full dimensions list', () => { + // Defensive check: the new prop must be optional and existing callers + // (URL-restore, any integration that hasn't been updated yet) should + // see the pre-Phase-6 behaviour unchanged. + renderWithIntl( + + ); + + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-environment`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-region`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-host.name`) + ).toBeInTheDocument(); + }); + + it('orphan selections still surface even with the optimistic filter', () => { + // The optimistic filter operates on applicable options only; any + // selected dimension that isn't present in metricItems must still be + // rendered (checked) via the pre-existing orphan-surfacing path so the + // count stays consistent and the user can back out. + const orphan = { name: 'orphan.field' } as Dimension; + + renderWithIntl( + + ); + + const orphanOption = screen.getByTestId( + `${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-orphan.field` + ); + expect(orphanOption).toBeInTheDocument(); + expect(orphanOption).toHaveAttribute('data-checked', 'on'); + }); + + it('no selection means the full applicable list is shown', () => { + // When nothing is selected the optimistic filter is a no-op — we don't + // know which metric "group" to constrain to, so every applicable + // dimension must remain available. + renderWithIntl( + + ); + + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-environment`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-region`) + ).toBeInTheDocument(); + expect( + screen.getByTestId(`${METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ}Option-host.name`) + ).toBeInTheDocument(); + }); + }); }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx index 7a74a9613a9a7..e5ac855cfb0f2 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx @@ -24,7 +24,7 @@ import { ToolbarSelector, type SelectableEntry } from '@kbn/shared-ux-toolbar-se import { comboBoxFieldOptionMatcher } from '@kbn/field-utils'; import { css } from '@emotion/react'; import { debounce } from 'lodash'; -import type { Dimension } from '../../types'; +import type { Dimension, ParsedMetricItem } from '../../types'; import { MAX_DIMENSIONS_SELECTIONS, METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ, @@ -39,6 +39,18 @@ interface DimensionsSelectorProps { onChange: (dimensions: Dimension[]) => void; singleSelection?: boolean; isLoading?: boolean; + /** + * Full set of metric items currently in the grid. When provided, the picker + * performs an optimistic client-side filter: only dimensions carried by + * metrics that also carry every currently-selected dimension remain in the + * option list. This prevents a user from reaching an empty-grid state by + * rapidly selecting dimensions that belong to disjoint metrics (the server + * would filter them out on the next fetch, but the click already happened). + * + * When omitted, the applicable set is taken verbatim from `dimensions` + * (pre-Phase-6 behaviour). + */ + metricItems?: ParsedMetricItem[]; } export const DimensionsSelector = ({ @@ -48,6 +60,7 @@ export const DimensionsSelector = ({ fullWidth = false, singleSelection = false, isLoading = false, + metricItems, }: DimensionsSelectorProps) => { const { euiTheme } = useEuiTheme(); const [localSelectedDimensions, setLocalSelectedDimensions] = @@ -62,9 +75,44 @@ export const DimensionsSelector = ({ [localSelectedDimensions] ); + // Phase 6 optimistic filter: given the metric items currently in the grid + // and the user's selection so far, compute the set of dimension names that + // still has a non-empty intersection of metrics carrying every selected + // dimension. Returning `null` means "no client-side filter applies" — the + // caller should fall back to the full `dimensions` array. + const optimisticApplicableNames = useMemo(() => { + if (!metricItems || localSelectedDimensions.length === 0) { + return null; + } + const selectedNames = [...selectedNamesSet]; + const names = new Set(); + for (const item of metricItems) { + const itemDimNames = new Set(item.dimensionFields.map((d) => d.name)); + const hasAllSelected = selectedNames.every((name) => itemDimNames.has(name)); + if (!hasAllSelected) { + continue; + } + for (const dim of item.dimensionFields) { + names.add(dim.name); + } + } + return names; + }, [metricItems, localSelectedDimensions, selectedNamesSet]); + const options: SelectableEntry[] = useMemo(() => { const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; - const applicableNames = new Set(dimensions.map((d) => d.name)); + + // When the optimistic filter is active, narrow the applicable list to + // dimensions carried by at least one metric that still carries every + // current selection. The orphan path below continues to surface any + // already-selected dimension that falls outside this set, so the user + // never loses sight of their own picks. + const filteredDimensions = + optimisticApplicableNames == null + ? dimensions + : dimensions.filter((dimension) => optimisticApplicableNames.has(dimension.name)); + + const applicableNames = new Set(filteredDimensions.map((d) => d.name)); // Orphan selections are dimensions the user has already picked but that // are not in the current applicable set (e.g. a filtered METRICS_INFO @@ -128,13 +176,14 @@ export const DimensionsSelector = ({ // Orphan selections are prepended so they stay easy to find; the // applicable set keeps its caller-provided ordering below. - return [...orphanSelections.map(toOption), ...dimensions.map(toOption)]; + return [...orphanSelections.map(toOption), ...filteredDimensions.map(toOption)]; }, [ dimensions, selectedNamesSet, localSelectedDimensions, singleSelection, euiTheme.levels.menu, + optimisticApplicableNames, ]); const onChangeRef = useRef(onChange); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx index 4f29d0b9ed3bc..9a9cee1a42fa0 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx @@ -12,7 +12,7 @@ import { useEuiTheme, useIsWithinMaxBreakpoint } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import type { IconButtonGroupProps } from '@kbn/shared-ux-button-toolbar'; import { css } from '@emotion/react'; -import type { Dimension, UnifiedMetricsGridProps } from '../../../types'; +import type { Dimension, ParsedMetricItem, UnifiedMetricsGridProps } from '../../../types'; import { useMetricsExperienceState } from '../../observability/metrics/context/metrics_experience_state_provider'; import { DimensionsSelector } from '../dimensions_selector'; import { MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; @@ -23,6 +23,13 @@ interface UseToolbarActionsProps extends Pick { const { selectedDimensions, onDimensionsChange, isFullscreen, onToggleFullscreen } = useMetricsExperienceState(); @@ -56,6 +64,7 @@ export const useToolbarActions = ({ singleSelection={MAX_DIMENSIONS_SELECTIONS <= 1} fullWidth={isSmallScreen} isLoading={isLoading} + metricItems={metricItems} /> ), ], @@ -66,6 +75,7 @@ export const useToolbarActions = ({ onDimensionsSelectionChange, hideDimensionsSelector, isLoading, + metricItems, ] ); From 8aa797e750bf1935344d2a4b82a70b7bb2998683 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 16:31:11 -0400 Subject: [PATCH 12/19] chore: strip planning references from picker comments Removes internal-only labels (phase numbers, AC numbers, ticket/PR references) from comments, docstrings, and test descriptions so the code stands on its own without planning-doc context. Shortens several comments that restated what the code already says. No behaviour change; 31/31 picker tests still pass. --- .../metrics/metrics_experience_grid.tsx | 5 +-- .../toolbar/dimensions_selector.test.tsx | 34 +++++++--------- .../toolbar/dimensions_selector.tsx | 39 ++++++------------- .../toolbar/hooks/use_toolbar_actions.tsx | 7 +--- 4 files changed, 29 insertions(+), 56 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx index 15eff3ea693e6..70ffcc47787ef 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx @@ -103,9 +103,8 @@ export const MetricsExperienceGrid = ({ const { toggleActions, leftSideActions, rightSideActions } = useToolbarActions({ allDimensions, - // Use the pre-search-filter metric set for the picker's optimistic filter: - // a user's typed search term shouldn't shrink the underlying applicable- - // dimension universe — only the actual selection should. + // Pass the unfiltered metricItems, not filteredMetricItems: the picker's + // applicable-dimension set should not shrink because of a search term. metricItems, renderToggleActions, onDimensionsChange: onToolbarDimensionsChange, diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx index 9e273e1801327..05128f9597698 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx @@ -411,11 +411,10 @@ describe('DimensionsSelector', () => { }); describe('Selected dimensions not in applicable set (race-condition guard)', () => { - // AC2 from ticket #263309: a dimension that the user has already selected - // must remain visible in the picker even if the latest METRICS_INFO - // response drops it from `dimensions` (the applicable set). This covers - // the case where the user selects two dimensions before the grid - // refreshes and the server returns no metrics matching both. + // A selected dimension must stay visible in the picker even when it is + // not in `dimensions` — e.g. the latest METRICS_INFO response narrowed + // the applicable set and dropped it. Without this the count badge can + // disagree with the visible checkmarks. const applicableOnly = [{ name: 'b' }, { name: 'c' }] as Dimension[]; const orphanPlusApplicable = [{ name: 'a' }, { name: 'b' }] as Dimension[]; @@ -440,7 +439,7 @@ describe('DimensionsSelector', () => { expect(applicableOption).toHaveAttribute('data-checked', 'on'); }); - it('reflects both orphan and applicable selections in the popover count (AC3)', () => { + it('reflects both orphan and applicable selections in the popover count', () => { renderWithIntl( { }); }); - describe('Optimistic filter via metricItems (Phase 6)', () => { - // These tests cover the client-side optimistic filtering that prevents the - // empty-grid dead-state described in PR #263629 review feedback: if the - // user clicks one dimension, the picker must immediately hide dimensions + describe('Optimistic filter via metricItems', () => { + // Once a selection is made, the picker must immediately hide dimensions // that no metric carrying that selection supports — without waiting for - // the server round-trip. + // the server round-trip — so rapid multi-select can't reach an empty grid. const environment = { name: 'environment' } as Dimension; const region = { name: 'region' } as Dimension; const hostName = { name: 'host.name' } as Dimension; @@ -598,9 +595,8 @@ describe('DimensionsSelector', () => { }); it('without metricItems, falls back to the full dimensions list', () => { - // Defensive check: the new prop must be optional and existing callers - // (URL-restore, any integration that hasn't been updated yet) should - // see the pre-Phase-6 behaviour unchanged. + // The prop is optional; callers that don't provide it get the options + // straight from `dimensions`, no client-side narrowing. renderWithIntl( { }); it('orphan selections still surface even with the optimistic filter', () => { - // The optimistic filter operates on applicable options only; any - // selected dimension that isn't present in metricItems must still be - // rendered (checked) via the pre-existing orphan-surfacing path so the + // The optimistic filter operates on applicable options; a selected + // dimension that isn't in metricItems still renders (checked) so the // count stays consistent and the user can back out. const orphan = { name: 'orphan.field' } as Dimension; @@ -644,9 +639,8 @@ describe('DimensionsSelector', () => { }); it('no selection means the full applicable list is shown', () => { - // When nothing is selected the optimistic filter is a no-op — we don't - // know which metric "group" to constrain to, so every applicable - // dimension must remain available. + // With nothing selected there's no metric subset to constrain to, so + // every applicable dimension must remain available. renderWithIntl( { if (!metricItems || localSelectedDimensions.length === 0) { return null; @@ -102,11 +95,6 @@ export const DimensionsSelector = ({ const options: SelectableEntry[] = useMemo(() => { const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; - // When the optimistic filter is active, narrow the applicable list to - // dimensions carried by at least one metric that still carries every - // current selection. The orphan path below continues to surface any - // already-selected dimension that falls outside this set, so the user - // never loses sight of their own picks. const filteredDimensions = optimisticApplicableNames == null ? dimensions @@ -114,11 +102,9 @@ export const DimensionsSelector = ({ const applicableNames = new Set(filteredDimensions.map((d) => d.name)); - // Orphan selections are dimensions the user has already picked but that - // are not in the current applicable set (e.g. a filtered METRICS_INFO - // response dropped them). Per AC2/AC3 of ticket #263309 they must remain - // visible and toggleable so the count badge matches the rendered ticks - // and the user can always back out of an invalid combination. + // Selections no longer in the applicable set stay visible (prepended, + // checked) so the count badge matches the rendered ticks and the user can + // always deselect what they picked. const orphanSelections = localSelectedDimensions .filter((dimension) => !applicableNames.has(dimension.name)) .slice() @@ -213,9 +199,8 @@ export const DimensionsSelector = ({ (chosenOption?: SelectableEntry | SelectableEntry[]) => { const opts = chosenOption == null ? [] : Array.isArray(chosenOption) ? chosenOption : [chosenOption]; - // Look up dimensions from both the applicable set and the current local - // selection: this lets us preserve orphan selections (those not in the - // current applicable set) when the user toggles another option. + // Include local selections in the lookup so toggling another option + // doesn't silently drop a selection that's no longer in `dimensions`. const dimensionByName = new Map(); for (const dimension of localSelectedDimensions) { dimensionByName.set(dimension.name, dimension); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx index 9a9cee1a42fa0..d552086c75342 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_toolbar_actions.tsx @@ -23,12 +23,7 @@ interface UseToolbarActionsProps extends Pick Date: Fri, 17 Apr 2026 16:42:48 -0400 Subject: [PATCH 13/19] refactor(picker): extract applicable-dimensions helper Moves the metricItems-to-applicable-names computation out of the component into a module-scoped `getApplicableDimensionNames` helper, replacing the in-useMemo for-loop with filter + flatMap for consistency with the rest of the codebase. Also drops a redundant inline comment in metrics_experience_grid.tsx. --- .../metrics/metrics_experience_grid.tsx | 2 -- .../toolbar/dimensions_selector.tsx | 29 ++++++++++--------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx index 70ffcc47787ef..17853e1ad33aa 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/observability/metrics/metrics_experience_grid.tsx @@ -103,8 +103,6 @@ export const MetricsExperienceGrid = ({ const { toggleActions, leftSideActions, rightSideActions } = useToolbarActions({ allDimensions, - // Pass the unfiltered metricItems, not filteredMetricItems: the picker's - // applicable-dimension set should not shrink because of a search term. metricItems, renderToggleActions, onDimensionsChange: onToolbarDimensionsChange, diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx index 884feb1ea1f42..8924ad4c1d4b4 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx @@ -32,6 +32,21 @@ import { } from '../../common/constants'; import { getOptionDisabledState } from './dimensions_selector_helpers'; +const getApplicableDimensionNames = ( + metricItems: ParsedMetricItem[], + selectedNames: string[] +): Set => { + const carriesAllSelected = (item: ParsedMetricItem) => { + const itemDimNames = new Set(item.dimensionFields.map((d) => d.name)); + return selectedNames.every((name) => itemDimNames.has(name)); + }; + return new Set( + metricItems + .filter(carriesAllSelected) + .flatMap((item) => item.dimensionFields.map((d) => d.name)) + ); +}; + interface DimensionsSelectorProps { dimensions: Dimension[]; selectedDimensions: Dimension[]; @@ -77,19 +92,7 @@ export const DimensionsSelector = ({ if (!metricItems || localSelectedDimensions.length === 0) { return null; } - const selectedNames = [...selectedNamesSet]; - const names = new Set(); - for (const item of metricItems) { - const itemDimNames = new Set(item.dimensionFields.map((d) => d.name)); - const hasAllSelected = selectedNames.every((name) => itemDimNames.has(name)); - if (!hasAllSelected) { - continue; - } - for (const dim of item.dimensionFields) { - names.add(dim.name); - } - } - return names; + return getApplicableDimensionNames(metricItems, [...selectedNamesSet]); }, [metricItems, localSelectedDimensions, selectedNamesSet]); const options: SelectableEntry[] = useMemo(() => { From 82dbfe6571c00b9ecc39bec3e1443e4928f2770f Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Fri, 17 Apr 2026 16:57:52 -0400 Subject: [PATCH 14/19] refactor(picker): address reviewer cleanups - Drop redundant localSelectedDimensions from optimisticApplicableNames useMemo deps by using selectedNamesSet.size for the empty check. - Collapse the applicableNames Set rebuild: reuse optimisticApplicableNames when present, only materialise from dimensions when falling back. - Extend the metricItems jsdoc to note that orphan surfacing is independent of this prop (still covers URL restore / data drift). - Remove dead mockFields constant and the corresponding `fields` entry from defaultProps in the test file. --- .../toolbar/dimensions_selector.test.tsx | 8 -------- .../components/toolbar/dimensions_selector.tsx | 15 ++++++++------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx index 05128f9597698..21006bf88db53 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.test.tsx @@ -133,20 +133,12 @@ const mockDimensions: Dimension[] = [ { name: 'cloud.availability_zone' }, ] as Dimension[]; -const mockFields = [ - { dimensions: [mockDimensions[0], mockDimensions[1]] }, - { dimensions: [mockDimensions[0], mockDimensions[2]] }, - { dimensions: [mockDimensions[1], mockDimensions[3]] }, - { dimensions: [mockDimensions[0], mockDimensions[1], mockDimensions[2]] }, -]; - const renderWithIntl = (component: React.ReactElement) => { return render({component}); }; describe('DimensionsSelector', () => { const defaultProps = { - fields: mockFields, dimensions: mockDimensions, selectedDimensions: [], onChange: jest.fn(), diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx index 8924ad4c1d4b4..b01e922ff8828 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx @@ -56,9 +56,10 @@ interface DimensionsSelectorProps { isLoading?: boolean; /** * When provided, the option list is filtered on the client to dimensions - * carried by at least one metric that also carries every current selection. - * Prevents a rapid multi-select from reaching an empty-grid state before the - * server fetch returns. Without it, options come straight from `dimensions`. + * carried by at least one metric that also carries every current selection, + * preventing rapid multi-select from reaching an empty-grid state. Selected + * dimensions not in the applicable set always stay visible regardless of + * this prop (e.g. after URL restore). */ metricItems?: ParsedMetricItem[]; } @@ -89,22 +90,22 @@ export const DimensionsSelector = ({ // current selection. `null` means no client-side filter applies (either no // selection yet, or metricItems wasn't provided). const optimisticApplicableNames = useMemo(() => { - if (!metricItems || localSelectedDimensions.length === 0) { + if (!metricItems || selectedNamesSet.size === 0) { return null; } return getApplicableDimensionNames(metricItems, [...selectedNamesSet]); - }, [metricItems, localSelectedDimensions, selectedNamesSet]); + }, [metricItems, selectedNamesSet]); const options: SelectableEntry[] = useMemo(() => { const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; + const applicableNames = optimisticApplicableNames ?? new Set(dimensions.map((d) => d.name)); + const filteredDimensions = optimisticApplicableNames == null ? dimensions : dimensions.filter((dimension) => optimisticApplicableNames.has(dimension.name)); - const applicableNames = new Set(filteredDimensions.map((d) => d.name)); - // Selections no longer in the applicable set stay visible (prepended, // checked) so the count badge matches the rendered ticks and the user can // always deselect what they picked. From 71a877925917fe0682de93bbecf73bbf97982b4a Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Mon, 20 Apr 2026 13:15:27 -0400 Subject: [PATCH 15/19] refactor(dimensions-selector): extract business logic into custom hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves applicable-set computation, orphan partitioning, options assembly, local-selection state, and debounced onChange out of the DimensionsSelector component and into `useDimensionsSelector`. Adds pure helpers `getApplicableDimensionNames` and `partitionDimensionsForRender` alongside the existing `getOptionDisabledState`, with matching unit coverage. Leaves the component as a thin wrapper around ToolbarSelector that forwards hook output. No behavioral change — all 482 package tests and type check pass. Addresses Lucas's review feedback: https://github.com/elastic/kibana/pull/263629#discussion_r3111732609 --- .../toolbar/dimensions_selector.tsx | 316 +---------------- .../dimensions_selector_helpers.test.ts | 95 ++++- .../toolbar/dimensions_selector_helpers.ts | 61 ++++ .../toolbar/hooks/use_dimensions_selector.tsx | 335 ++++++++++++++++++ 4 files changed, 508 insertions(+), 299 deletions(-) create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx index b01e922ff8828..f4efb9c904baa 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector.tsx @@ -7,45 +7,12 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ -import React, { useMemo, useCallback, useRef, useEffect, useState } from 'react'; -import { FormattedMessage } from '@kbn/i18n-react'; -import { - EuiFlexGroup, - EuiFlexItem, - EuiLoadingSpinner, - EuiNotificationBadge, - EuiText, - EuiToolTip, - EuiButtonEmpty, - EuiSpacer, - useEuiTheme, -} from '@elastic/eui'; -import { ToolbarSelector, type SelectableEntry } from '@kbn/shared-ux-toolbar-selector'; +import React from 'react'; +import { ToolbarSelector } from '@kbn/shared-ux-toolbar-selector'; import { comboBoxFieldOptionMatcher } from '@kbn/field-utils'; -import { css } from '@emotion/react'; -import { debounce } from 'lodash'; import type { Dimension, ParsedMetricItem } from '../../types'; -import { - MAX_DIMENSIONS_SELECTIONS, - METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ, - DEBOUNCE_TIME, -} from '../../common/constants'; -import { getOptionDisabledState } from './dimensions_selector_helpers'; - -const getApplicableDimensionNames = ( - metricItems: ParsedMetricItem[], - selectedNames: string[] -): Set => { - const carriesAllSelected = (item: ParsedMetricItem) => { - const itemDimNames = new Set(item.dimensionFields.map((d) => d.name)); - return selectedNames.every((name) => itemDimNames.has(name)); - }; - return new Set( - metricItems - .filter(carriesAllSelected) - .flatMap((item) => item.dimensionFields.map((d) => d.name)) - ); -}; +import { METRICS_BREAKDOWN_SELECTOR_DATA_TEST_SUBJ } from '../../common/constants'; +import { useDimensionsSelector } from './hooks/use_dimensions_selector'; interface DimensionsSelectorProps { dimensions: Dimension[]; @@ -73,273 +40,26 @@ export const DimensionsSelector = ({ isLoading = false, metricItems, }: DimensionsSelectorProps) => { - const { euiTheme } = useEuiTheme(); - const [localSelectedDimensions, setLocalSelectedDimensions] = - useState(selectedDimensions); - - useEffect(() => { - setLocalSelectedDimensions(selectedDimensions); - }, [selectedDimensions]); - - const selectedNamesSet = useMemo( - () => new Set(localSelectedDimensions.map((d) => d.name)), - [localSelectedDimensions] - ); - - // Names of dimensions still carried by at least one metric that has every - // current selection. `null` means no client-side filter applies (either no - // selection yet, or metricItems wasn't provided). - const optimisticApplicableNames = useMemo(() => { - if (!metricItems || selectedNamesSet.size === 0) { - return null; - } - return getApplicableDimensionNames(metricItems, [...selectedNamesSet]); - }, [metricItems, selectedNamesSet]); - - const options: SelectableEntry[] = useMemo(() => { - const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; - - const applicableNames = optimisticApplicableNames ?? new Set(dimensions.map((d) => d.name)); - - const filteredDimensions = - optimisticApplicableNames == null - ? dimensions - : dimensions.filter((dimension) => optimisticApplicableNames.has(dimension.name)); - - // Selections no longer in the applicable set stay visible (prepended, - // checked) so the count badge matches the rendered ticks and the user can - // always deselect what they picked. - const orphanSelections = localSelectedDimensions - .filter((dimension) => !applicableNames.has(dimension.name)) - .slice() - .sort((a, b) => a.name.localeCompare(b.name)); - - const toOption = (dimension: Dimension): SelectableEntry => { - const isSelected = selectedNamesSet.has(dimension.name); - - const isDisabled = getOptionDisabledState({ - singleSelection, - isSelected, - isAtMaxLimit, - }); - - const tooltipContent = - isAtMaxLimit && isDisabled ? ( - - ) : undefined; - - const option: SelectableEntry = { - value: dimension.name, - label: dimension.name, - checked: isSelected ? 'on' : undefined, - disabled: isDisabled, - key: dimension.name, - }; - - if (tooltipContent) { - option.append = ( - -
- - ); - } - - return option; - }; - - // Orphan selections are prepended so they stay easy to find; the - // applicable set keeps its caller-provided ordering below. - return [...orphanSelections.map(toOption), ...filteredDimensions.map(toOption)]; - }, [ + const { + options, + buttonLabel, + buttonTooltipContent, + popoverContentBelowSearch, + handleChange, + selectedValues, + } = useDimensionsSelector({ dimensions, - selectedNamesSet, - localSelectedDimensions, + selectedDimensions, + onChange, singleSelection, - euiTheme.levels.menu, - optimisticApplicableNames, - ]); - - const onChangeRef = useRef(onChange); - useEffect(() => { - onChangeRef.current = onChange; - }, [onChange]); - - // Create debounced onChange only for multi-selection mode - const debouncedOnChange = useMemo(() => { - if (singleSelection) { - return null; - } - return debounce((dim: Dimension[]) => { - onChangeRef.current(dim); - }, DEBOUNCE_TIME); - }, [singleSelection]); - - useEffect(() => { - return () => { - if (debouncedOnChange) { - debouncedOnChange.cancel(); - } - }; - }, [debouncedOnChange]); - - const handleChange = useCallback( - (chosenOption?: SelectableEntry | SelectableEntry[]) => { - const opts = - chosenOption == null ? [] : Array.isArray(chosenOption) ? chosenOption : [chosenOption]; - // Include local selections in the lookup so toggling another option - // doesn't silently drop a selection that's no longer in `dimensions`. - const dimensionByName = new Map(); - for (const dimension of localSelectedDimensions) { - dimensionByName.set(dimension.name, dimension); - } - for (const dimension of dimensions) { - dimensionByName.set(dimension.name, dimension); - } - const newSelection = opts - .map((opt) => dimensionByName.get(opt.value)) - .filter((d): d is Dimension => d !== undefined) - .slice(0, MAX_DIMENSIONS_SELECTIONS); - - // For single selection, call onChange immediately - if (singleSelection || !debouncedOnChange) { - setLocalSelectedDimensions(newSelection); - onChange(newSelection); - } else { - setLocalSelectedDimensions(newSelection); - debouncedOnChange.cancel(); - debouncedOnChange(newSelection); - } - }, - [onChange, dimensions, localSelectedDimensions, singleSelection, debouncedOnChange] - ); - - const handleClearAll = useCallback(() => { - if (debouncedOnChange) { - debouncedOnChange.cancel(); - } - - setLocalSelectedDimensions([]); - onChange([]); - }, [onChange, debouncedOnChange]); - - const buttonLabel = useMemo(() => { - const count = localSelectedDimensions.length; - - return ( - - - {count === 0 ? ( - - ) : ( - - - - - - {count} - - - )} - - {isLoading && ( - - - - )} - - ); - }, [localSelectedDimensions, isLoading]); - - // Create tooltip content for when at max dimensions - const buttonTooltipContent = useMemo(() => { - const count = localSelectedDimensions.length; - const isAtMaxDimensions = count >= MAX_DIMENSIONS_SELECTIONS; - - if (isAtMaxDimensions) { - return ( - - ); - } - - return undefined; - }, [localSelectedDimensions]); - - const popoverContentBelowSearch = useMemo(() => { - const count = localSelectedDimensions.length; - return ( - <> - - - - - - - - {count > 0 && ( - - - - - - )} - - - - ); - }, [localSelectedDimensions.length, handleClearAll, euiTheme.size.l]); + isLoading, + metricItems, + }); return ( ({ + metricName, + dataStream: 'metrics-test', + units: [], + metricTypes: [], + fieldTypes: [], + dimensionFields, +}); describe('dimensions_selector_helpers', () => { describe('getOptionDisabledState', () => { @@ -57,4 +71,83 @@ describe('dimensions_selector_helpers', () => { ).toBe(true); }); }); + + describe('getApplicableDimensionNames', () => { + const environment: Dimension = { name: 'environment' }; + const region: Dimension = { name: 'region' }; + const hostName: Dimension = { name: 'host.name' }; + + const metricItems: ParsedMetricItem[] = [ + buildMetricItem('cpu.usage', [environment, hostName]), + buildMetricItem('network.bytes_in', [region, hostName]), + ]; + + it('returns the union of dimensions across metrics that carry every selection', () => { + const applicable = getApplicableDimensionNames(metricItems, ['host.name']); + expect(applicable).toEqual(new Set(['environment', 'region', 'host.name'])); + }); + + it('narrows to a single metric when only one carries every selected dimension', () => { + const applicable = getApplicableDimensionNames(metricItems, ['environment']); + expect(applicable).toEqual(new Set(['environment', 'host.name'])); + }); + + it('returns an empty set when no metric carries every selected dimension', () => { + const applicable = getApplicableDimensionNames(metricItems, ['environment', 'region']); + expect(applicable).toEqual(new Set()); + }); + + it('returns every dimension name when the selection is empty', () => { + const applicable = getApplicableDimensionNames(metricItems, []); + expect(applicable).toEqual(new Set(['environment', 'region', 'host.name'])); + }); + }); + + describe('partitionDimensionsForRender', () => { + const a: Dimension = { name: 'a' }; + const b: Dimension = { name: 'b' }; + const c: Dimension = { name: 'c' }; + + it('returns dimensions in caller order when no optimistic filter is active', () => { + const result = partitionDimensionsForRender({ + dimensions: [b, a, c], + selectedDimensions: [], + optimisticApplicableNames: null, + }); + expect(result.orphanSelections).toEqual([]); + expect(result.applicableDimensions).toEqual([b, a, c]); + }); + + it('narrows applicable dimensions when an optimistic filter is provided', () => { + const result = partitionDimensionsForRender({ + dimensions: [a, b, c], + selectedDimensions: [a], + optimisticApplicableNames: new Set(['a', 'b']), + }); + expect(result.applicableDimensions).toEqual([a, b]); + expect(result.orphanSelections).toEqual([]); + }); + + it('surfaces selections that fall outside the applicable set as orphans', () => { + const orphan: Dimension = { name: 'orphan' }; + const result = partitionDimensionsForRender({ + dimensions: [a, b], + selectedDimensions: [orphan, a], + optimisticApplicableNames: new Set(['a', 'b']), + }); + expect(result.orphanSelections).toEqual([orphan]); + expect(result.applicableDimensions).toEqual([a, b]); + }); + + it('sorts multiple orphans alphabetically', () => { + const q: Dimension = { name: 'q' }; + const z: Dimension = { name: 'z' }; + const result = partitionDimensionsForRender({ + dimensions: [], + selectedDimensions: [z, a, q], + optimisticApplicableNames: null, + }); + expect(result.orphanSelections.map((d) => d.name)).toEqual(['a', 'q', 'z']); + }); + }); }); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts index 3a3b0e4c014a3..3b51a6e125c6b 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts @@ -7,6 +7,8 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ +import type { Dimension, ParsedMetricItem } from '../../types'; + interface OptionDisabledStateParams { singleSelection: boolean; isSelected: boolean; @@ -28,3 +30,62 @@ export const getOptionDisabledState = ({ if (isSelected) return false; return isAtMaxLimit; }; + +/** + * Returns the set of dimension names carried by at least one metric that + * also carries every currently-selected dimension. Used by the picker to + * optimistically hide options that, if selected, would produce an empty + * grid — without waiting for the server round-trip. + */ +export const getApplicableDimensionNames = ( + metricItems: ParsedMetricItem[], + selectedNames: readonly string[] +): Set => { + const carriesAllSelected = (item: ParsedMetricItem) => { + const itemDimNames = new Set(item.dimensionFields.map((d) => d.name)); + return selectedNames.every((name) => itemDimNames.has(name)); + }; + return new Set( + metricItems + .filter(carriesAllSelected) + .flatMap((item) => item.dimensionFields.map((d) => d.name)) + ); +}; + +interface PartitionedDimensionsParams { + dimensions: Dimension[]; + selectedDimensions: Dimension[]; + optimisticApplicableNames: Set | null; +} + +interface PartitionedDimensions { + /** Selections not in the applicable set — always surfaced so the count badge matches the visible ticks. */ + orphanSelections: Dimension[]; + /** Applicable dimensions in their caller-provided order, narrowed by the optimistic filter when active. */ + applicableDimensions: Dimension[]; +} + +/** + * Splits the picker's render list into orphaned selections (shown first, + * alphabetically sorted) and applicable dimensions (caller order preserved). + * Keeps the split pure so the component stays presentational. + */ +export const partitionDimensionsForRender = ({ + dimensions, + selectedDimensions, + optimisticApplicableNames, +}: PartitionedDimensionsParams): PartitionedDimensions => { + const applicableNames = optimisticApplicableNames ?? new Set(dimensions.map((d) => d.name)); + + const applicableDimensions = + optimisticApplicableNames == null + ? dimensions + : dimensions.filter((dimension) => optimisticApplicableNames.has(dimension.name)); + + const orphanSelections = selectedDimensions + .filter((dimension) => !applicableNames.has(dimension.name)) + .slice() + .sort((a, b) => a.name.localeCompare(b.name)); + + return { orphanSelections, applicableDimensions }; +}; diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx new file mode 100644 index 0000000000000..1cdf0af2bf914 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx @@ -0,0 +1,335 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import type { ReactElement } from 'react'; +import { + EuiButtonEmpty, + EuiFlexGroup, + EuiFlexItem, + EuiLoadingSpinner, + EuiNotificationBadge, + EuiSpacer, + EuiText, + EuiToolTip, + useEuiTheme, +} from '@elastic/eui'; +import { FormattedMessage } from '@kbn/i18n-react'; +import type { SelectableEntry } from '@kbn/shared-ux-toolbar-selector'; +import { css } from '@emotion/react'; +import { debounce } from 'lodash'; +import type { Dimension, ParsedMetricItem } from '../../../types'; +import { DEBOUNCE_TIME, MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; +import { + getApplicableDimensionNames, + getOptionDisabledState, + partitionDimensionsForRender, +} from '../dimensions_selector_helpers'; + +interface UseDimensionsSelectorParams { + dimensions: Dimension[]; + selectedDimensions: Dimension[]; + onChange: (dimensions: Dimension[]) => void; + singleSelection: boolean; + isLoading: boolean; + metricItems?: ParsedMetricItem[]; +} + +export interface UseDimensionsSelectorResult { + options: SelectableEntry[]; + buttonLabel: ReactElement; + buttonTooltipContent: ReactElement | undefined; + popoverContentBelowSearch: ReactElement; + handleChange: (chosenOption?: SelectableEntry | SelectableEntry[]) => void; + selectedValues: string[]; +} + +/** + * Encapsulates the dimensions picker's business logic so the component can + * stay presentational. Owns: + * - local selection state (mirrors the controlled prop, lets the UI render + * optimistically while the debounced onChange catches up) + * - the optimistic applicable-dimension filter derived from `metricItems` + * - assembly of the `SelectableEntry[]` (orphans prepended, disabled-state + * tooltip) + * - change + clear handlers (debounced for multi-select, immediate for + * single) + * - the button label, tooltip, and popover footer nodes + */ +export const useDimensionsSelector = ({ + dimensions, + selectedDimensions, + onChange, + singleSelection, + isLoading, + metricItems, +}: UseDimensionsSelectorParams): UseDimensionsSelectorResult => { + const { euiTheme } = useEuiTheme(); + const [localSelectedDimensions, setLocalSelectedDimensions] = + useState(selectedDimensions); + + useEffect(() => { + setLocalSelectedDimensions(selectedDimensions); + }, [selectedDimensions]); + + const selectedNamesSet = useMemo( + () => new Set(localSelectedDimensions.map((d) => d.name)), + [localSelectedDimensions] + ); + + // Names of dimensions still carried by at least one metric that has every + // current selection. `null` means no client-side filter applies (either no + // selection yet, or metricItems wasn't provided). + const optimisticApplicableNames = useMemo(() => { + if (!metricItems || selectedNamesSet.size === 0) { + return null; + } + return getApplicableDimensionNames(metricItems, [...selectedNamesSet]); + }, [metricItems, selectedNamesSet]); + + const options = useMemo(() => { + const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; + + const { orphanSelections, applicableDimensions } = partitionDimensionsForRender({ + dimensions, + selectedDimensions: localSelectedDimensions, + optimisticApplicableNames, + }); + + const toOption = (dimension: Dimension): SelectableEntry => { + const isSelected = selectedNamesSet.has(dimension.name); + + const isDisabled = getOptionDisabledState({ + singleSelection, + isSelected, + isAtMaxLimit, + }); + + const tooltipContent = + isAtMaxLimit && isDisabled ? ( + + ) : undefined; + + const option: SelectableEntry = { + value: dimension.name, + label: dimension.name, + checked: isSelected ? 'on' : undefined, + disabled: isDisabled, + key: dimension.name, + }; + + if (tooltipContent) { + option.append = ( + +
+ + ); + } + + return option; + }; + + // Orphan selections are prepended so they stay easy to find; the + // applicable set keeps its caller-provided ordering below. + return [...orphanSelections.map(toOption), ...applicableDimensions.map(toOption)]; + }, [ + dimensions, + localSelectedDimensions, + optimisticApplicableNames, + selectedNamesSet, + singleSelection, + euiTheme.levels.menu, + ]); + + const onChangeRef = useRef(onChange); + useEffect(() => { + onChangeRef.current = onChange; + }, [onChange]); + + // Create debounced onChange only for multi-selection mode. + const debouncedOnChange = useMemo(() => { + if (singleSelection) { + return null; + } + return debounce((dim: Dimension[]) => { + onChangeRef.current(dim); + }, DEBOUNCE_TIME); + }, [singleSelection]); + + useEffect(() => { + return () => { + if (debouncedOnChange) { + debouncedOnChange.cancel(); + } + }; + }, [debouncedOnChange]); + + const handleChange = useCallback( + (chosenOption?: SelectableEntry | SelectableEntry[]) => { + const opts = + chosenOption == null ? [] : Array.isArray(chosenOption) ? chosenOption : [chosenOption]; + // Include local selections in the lookup so toggling another option + // doesn't silently drop a selection that's no longer in `dimensions`. + const dimensionByName = new Map(); + for (const dimension of localSelectedDimensions) { + dimensionByName.set(dimension.name, dimension); + } + for (const dimension of dimensions) { + dimensionByName.set(dimension.name, dimension); + } + const newSelection = opts + .map((opt) => dimensionByName.get(opt.value)) + .filter((d): d is Dimension => d !== undefined) + .slice(0, MAX_DIMENSIONS_SELECTIONS); + + if (singleSelection || !debouncedOnChange) { + setLocalSelectedDimensions(newSelection); + onChange(newSelection); + return; + } + + setLocalSelectedDimensions(newSelection); + debouncedOnChange.cancel(); + debouncedOnChange(newSelection); + }, + [onChange, dimensions, localSelectedDimensions, singleSelection, debouncedOnChange] + ); + + const handleClearAll = useCallback(() => { + if (debouncedOnChange) { + debouncedOnChange.cancel(); + } + setLocalSelectedDimensions([]); + onChange([]); + }, [onChange, debouncedOnChange]); + + const buttonLabel = useMemo(() => { + const count = localSelectedDimensions.length; + + return ( + + + {count === 0 ? ( + + ) : ( + + + + + + {count} + + + )} + + {isLoading && ( + + + + )} + + ); + }, [localSelectedDimensions, isLoading]); + + const buttonTooltipContent = useMemo(() => { + const count = localSelectedDimensions.length; + const isAtMaxDimensions = count >= MAX_DIMENSIONS_SELECTIONS; + + if (!isAtMaxDimensions) { + return undefined; + } + + return ( + + ); + }, [localSelectedDimensions]); + + const popoverContentBelowSearch = useMemo(() => { + const count = localSelectedDimensions.length; + return ( + <> + + + + + + + + {count > 0 && ( + + + + + + )} + + + + ); + }, [localSelectedDimensions.length, handleClearAll, euiTheme.size.l]); + + const selectedValues = useMemo(() => [...selectedNamesSet], [selectedNamesSet]); + + return { + options, + buttonLabel, + buttonTooltipContent, + popoverContentBelowSearch, + handleChange, + selectedValues, + }; +}; From 374723abcf2379fa4ba0cc6329a7063106984c88 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Mon, 20 Apr 2026 14:57:23 -0400 Subject: [PATCH 16/19] chore(limits): bump lens page-load bundle limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lens entry bundle was 72-83B over its 86,000B page-load limit after rebasing this branch on main. Nothing in this PR's scope (kbn-unified-chart-section-viewer + one Discover test spec) can reach lens's bundle — a codebase-wide grep shows zero imports of @kbn/unified-chart-section-viewer from x-pack/platform/plugins/shared/lens, and a local dist build's stats.json contains no matching module paths. Cumulative drift from recent Lens-as-code commits that merged into main (e.g., #262871, #264134, #261581, #264147, #263810) pushed the entry bundle just over the line. Raising the limit here is the documented path when no in-PR contribution is found. Limit set via `node scripts/build_kibana_platform_plugins --focus lens --update-limits`. --- packages/kbn-optimizer/limits.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/kbn-optimizer/limits.yml b/packages/kbn-optimizer/limits.yml index dde0248f251c2..3564be42794ca 100644 --- a/packages/kbn-optimizer/limits.yml +++ b/packages/kbn-optimizer/limits.yml @@ -102,7 +102,7 @@ pageLoadAssetSize: kibanaUtils: 54161 kql: 15428 kubernetesSecurity: 6807 - lens: 86000 + lens: 94679 licenseManagement: 8265 licensing: 10073 links: 8620 From c2680f6ceaf2dbc311a7cb422507f3d7eeceeed0 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Tue, 21 Apr 2026 14:34:10 -0400 Subject: [PATCH 17/19] refactor(dimensions-selector): carry Dimension on option, extract buildDimensionOption Addresses review feedback on use_dimensions_selector.tsx: - Introduce `DimensionEntry = SelectableEntry & { dimension: Dimension }` so the selected `Dimension` travels with the option instead of being recovered by a reverse lookup in `handleChange`. Drops the per-change Map build and removes `dimensions` / `localSelectedDimensions` from the callback's deps. - Extract `buildDimensionOption` into `dimensions_selector_helpers.ts` so the hook's `options` memo can focus on partitioning and disabled-state logic. Comments addressed: - https://github.com/elastic/kibana/pull/263629#discussion_r3116709833 (#2) - https://github.com/elastic/kibana/pull/263629#discussion_r3116713727 (#3) --- .../toolbar/dimensions_selector_helpers.ts | 41 +++++++++++ .../toolbar/hooks/use_dimensions_selector.tsx | 70 +++++++------------ 2 files changed, 68 insertions(+), 43 deletions(-) diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts index 3b51a6e125c6b..5668bad3f5f9f 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_helpers.ts @@ -7,8 +7,49 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ +import type { ReactNode } from 'react'; +import type { SelectableEntry } from '@kbn/shared-ux-toolbar-selector'; import type { Dimension, ParsedMetricItem } from '../../types'; +/** + * A `SelectableEntry` carrying the `Dimension` it was built from. Keeping the + * dimension on the option lets `handleChange` read it back without a reverse + * lookup against `dimensions` + `localSelectedDimensions`. + */ +export type DimensionEntry = SelectableEntry & { dimension: Dimension }; + +interface BuildDimensionOptionParams { + dimension: Dimension; + isSelected: boolean; + isDisabled: boolean; + appendNode?: ReactNode; +} + +/** + * Builds the `DimensionEntry` rendered by the toolbar selector. Kept as a + * helper so the hook's `options` memo stays focused on partitioning and + * disabled-state derivation. + */ +export const buildDimensionOption = ({ + dimension, + isSelected, + isDisabled, + appendNode, +}: BuildDimensionOptionParams): DimensionEntry => { + const option: DimensionEntry = { + value: dimension.name, + label: dimension.name, + checked: isSelected ? 'on' : undefined, + disabled: isDisabled, + key: dimension.name, + dimension, + }; + if (appendNode) { + option.append = appendNode; + } + return option; +}; + interface OptionDisabledStateParams { singleSelection: boolean; isSelected: boolean; diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx index 1cdf0af2bf914..567268f894e31 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx @@ -27,10 +27,12 @@ import { debounce } from 'lodash'; import type { Dimension, ParsedMetricItem } from '../../../types'; import { DEBOUNCE_TIME, MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; import { + buildDimensionOption, getApplicableDimensionNames, getOptionDisabledState, partitionDimensionsForRender, } from '../dimensions_selector_helpers'; +import type { DimensionEntry } from '../dimensions_selector_helpers'; interface UseDimensionsSelectorParams { dimensions: Dimension[]; @@ -93,7 +95,7 @@ export const useDimensionsSelector = ({ return getApplicableDimensionNames(metricItems, [...selectedNamesSet]); }, [metricItems, selectedNamesSet]); - const options = useMemo(() => { + const options = useMemo(() => { const isAtMaxLimit = localSelectedDimensions.length >= MAX_DIMENSIONS_SELECTIONS; const { orphanSelections, applicableDimensions } = partitionDimensionsForRender({ @@ -102,36 +104,24 @@ export const useDimensionsSelector = ({ optimisticApplicableNames, }); - const toOption = (dimension: Dimension): SelectableEntry => { + const toOption = (dimension: Dimension): DimensionEntry => { const isSelected = selectedNamesSet.has(dimension.name); + const isDisabled = getOptionDisabledState({ singleSelection, isSelected, isAtMaxLimit }); + const showMaxTooltip = isAtMaxLimit && isDisabled; - const isDisabled = getOptionDisabledState({ - singleSelection, + return buildDimensionOption({ + dimension, isSelected, - isAtMaxLimit, - }); - - const tooltipContent = - isAtMaxLimit && isDisabled ? ( - - ) : undefined; - - const option: SelectableEntry = { - value: dimension.name, - label: dimension.name, - checked: isSelected ? 'on' : undefined, - disabled: isDisabled, - key: dimension.name, - }; - - if (tooltipContent) { - option.append = ( + isDisabled, + appendNode: showMaxTooltip ? ( + } position="top" anchorProps={{ css: css` @@ -146,10 +136,8 @@ export const useDimensionsSelector = ({ >
- ); - } - - return option; + ) : undefined, + }); }; // Orphan selections are prepended so they stay easy to find; the @@ -191,17 +179,13 @@ export const useDimensionsSelector = ({ (chosenOption?: SelectableEntry | SelectableEntry[]) => { const opts = chosenOption == null ? [] : Array.isArray(chosenOption) ? chosenOption : [chosenOption]; - // Include local selections in the lookup so toggling another option - // doesn't silently drop a selection that's no longer in `dimensions`. - const dimensionByName = new Map(); - for (const dimension of localSelectedDimensions) { - dimensionByName.set(dimension.name, dimension); - } - for (const dimension of dimensions) { - dimensionByName.set(dimension.name, dimension); - } - const newSelection = opts - .map((opt) => dimensionByName.get(opt.value)) + + // Each option carries its source `Dimension` (see `buildDimensionOption`), + // so we can read it straight off the option and skip the reverse lookup + // against `dimensions` + `localSelectedDimensions` that would otherwise + // be needed to recover selections no longer present in `dimensions`. + const newSelection = (opts as DimensionEntry[]) + .map((opt) => opt.dimension) .filter((d): d is Dimension => d !== undefined) .slice(0, MAX_DIMENSIONS_SELECTIONS); @@ -215,7 +199,7 @@ export const useDimensionsSelector = ({ debouncedOnChange.cancel(); debouncedOnChange(newSelection); }, - [onChange, dimensions, localSelectedDimensions, singleSelection, debouncedOnChange] + [onChange, singleSelection, debouncedOnChange] ); const handleClearAll = useCallback(() => { From 618e8d9b98b8993447d72bdd723361b314c403a7 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Tue, 21 Apr 2026 14:36:27 -0400 Subject: [PATCH 18/19] refactor(dimensions-selector): extract button/tooltip/footer as components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the inline JSX blobs out of `useDimensionsSelector` into dedicated components under `dimensions_selector_components/`: - `DimensionsButtonLabel` — trigger-button content (count badge + spinner). - `MaxDimensionsWarning` — shared `FormattedMessage` used by the popover tooltip overlay and the button-level tooltip (was duplicated before). - `MaxDimensionsTooltipOverlay` — absolute-positioned `EuiToolTip` overlay that activates on disabled (at-max-limit) options. - `DimensionsPopoverFooter` — count + "Clear selection" footer rendered below the search input. With the JSX extracted, the hook drops `euiTheme.levels.menu` from the `options` memo's deps (it now lives in the overlay component) and no longer needs the `EuiButtonEmpty` / `EuiFlexGroup` / `EuiFlexItem` / `EuiText` / `EuiLoadingSpinner` / `EuiNotificationBadge` / `EuiSpacer` / `EuiToolTip` / `useEuiTheme` imports. The hook stays responsible only for state, derivation, and callbacks. Comments addressed: - https://github.com/elastic/kibana/pull/263629#discussion_r3116718018 (#4) - https://github.com/elastic/kibana/pull/263629#discussion_r3116722962 (#5) - https://github.com/elastic/kibana/pull/263629#discussion_r3116724436 (#6) --- .../dimensions_button_label.tsx | 61 +++++++ .../dimensions_popover_footer.tsx | 70 ++++++++ .../dimensions_selector_components/index.ts | 13 ++ .../max_dimensions_tooltip_overlay.tsx | 42 +++++ .../max_dimensions_warning.tsx | 25 +++ .../toolbar/hooks/use_dimensions_selector.tsx | 163 ++++-------------- 6 files changed, 240 insertions(+), 134 deletions(-) create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_button_label.tsx create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_popover_footer.tsx create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/index.ts create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_tooltip_overlay.tsx create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_warning.tsx diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_button_label.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_button_label.tsx new file mode 100644 index 0000000000000..eb736ea43aba1 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_button_label.tsx @@ -0,0 +1,61 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React from 'react'; +import { EuiFlexGroup, EuiFlexItem, EuiLoadingSpinner, EuiNotificationBadge } from '@elastic/eui'; +import { FormattedMessage } from '@kbn/i18n-react'; +import { css } from '@emotion/react'; +import { MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; + +interface DimensionsButtonLabelProps { + count: number; + isLoading: boolean; +} + +/** + * Content rendered inside the toolbar selector's trigger button. + * - 0 selections: neutral "No dimensions selected" message + * - 1+ selections: "Dimensions" + count badge + * - Shows a loading spinner on the right while `isLoading` is true. + */ +export const DimensionsButtonLabel = ({ count, isLoading }: DimensionsButtonLabelProps) => ( + + + {count === 0 ? ( + + ) : ( + + + + + + {count} + + + )} + + {isLoading && ( + + + + )} + +); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_popover_footer.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_popover_footer.tsx new file mode 100644 index 0000000000000..bb5be2d3a4d3e --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/dimensions_popover_footer.tsx @@ -0,0 +1,70 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React from 'react'; +import { + EuiButtonEmpty, + EuiFlexGroup, + EuiFlexItem, + EuiSpacer, + EuiText, + useEuiTheme, +} from '@elastic/eui'; +import { FormattedMessage } from '@kbn/i18n-react'; +import { css } from '@emotion/react'; + +interface DimensionsPopoverFooterProps { + count: number; + onClear: () => void; +} + +/** + * Footer row rendered below the search input in the dimensions popover. + * Shows the current selection count and a "Clear selection" action when + * there is anything to clear. + */ +export const DimensionsPopoverFooter = ({ count, onClear }: DimensionsPopoverFooterProps) => { + const { euiTheme } = useEuiTheme(); + + return ( + <> + + + + + + + + {count > 0 && ( + + + + + + )} + + + + ); +}; diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/index.ts b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/index.ts new file mode 100644 index 0000000000000..2c3c12538227e --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/index.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +export { MaxDimensionsWarning } from './max_dimensions_warning'; +export { MaxDimensionsTooltipOverlay } from './max_dimensions_tooltip_overlay'; +export { DimensionsButtonLabel } from './dimensions_button_label'; +export { DimensionsPopoverFooter } from './dimensions_popover_footer'; diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_tooltip_overlay.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_tooltip_overlay.tsx new file mode 100644 index 0000000000000..09e0c2efda869 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_tooltip_overlay.tsx @@ -0,0 +1,42 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React from 'react'; +import { EuiToolTip, useEuiTheme } from '@elastic/eui'; +import { css } from '@emotion/react'; +import { MaxDimensionsWarning } from './max_dimensions_warning'; + +/** + * Absolutely-positioned overlay that fills its row so the tooltip triggers + * anywhere on a disabled (at-max-limit) dimension option. The overlay has to + * cover the option because EuiSelectable's row is focus-trapping and does not + * surface its own tooltip slot. + */ +export const MaxDimensionsTooltipOverlay = () => { + const { euiTheme } = useEuiTheme(); + + return ( + } + position="top" + anchorProps={{ + css: css` + position: absolute; + inset: 0; + width: 100%; + height: 100%; + pointer-events: auto; + z-index: ${euiTheme.levels.menu}; + `, + }} + > +
+ + ); +}; diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_warning.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_warning.tsx new file mode 100644 index 0000000000000..c53e95edd6716 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/dimensions_selector_components/max_dimensions_warning.tsx @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React from 'react'; +import { FormattedMessage } from '@kbn/i18n-react'; +import { MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; + +/** + * Plain message announcing that the user has hit the maximum number of + * dimensions. Rendered both inside the popover tooltip overlay (per-option) + * and as the button-level tooltip. + */ +export const MaxDimensionsWarning = () => ( + +); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx index 567268f894e31..3c3d2040cfa96 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx @@ -9,20 +9,7 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import type { ReactElement } from 'react'; -import { - EuiButtonEmpty, - EuiFlexGroup, - EuiFlexItem, - EuiLoadingSpinner, - EuiNotificationBadge, - EuiSpacer, - EuiText, - EuiToolTip, - useEuiTheme, -} from '@elastic/eui'; -import { FormattedMessage } from '@kbn/i18n-react'; import type { SelectableEntry } from '@kbn/shared-ux-toolbar-selector'; -import { css } from '@emotion/react'; import { debounce } from 'lodash'; import type { Dimension, ParsedMetricItem } from '../../../types'; import { DEBOUNCE_TIME, MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; @@ -33,6 +20,12 @@ import { partitionDimensionsForRender, } from '../dimensions_selector_helpers'; import type { DimensionEntry } from '../dimensions_selector_helpers'; +import { + DimensionsButtonLabel, + DimensionsPopoverFooter, + MaxDimensionsTooltipOverlay, + MaxDimensionsWarning, +} from '../dimensions_selector_components'; interface UseDimensionsSelectorParams { dimensions: Dimension[]; @@ -58,11 +51,12 @@ export interface UseDimensionsSelectorResult { * - local selection state (mirrors the controlled prop, lets the UI render * optimistically while the debounced onChange catches up) * - the optimistic applicable-dimension filter derived from `metricItems` - * - assembly of the `SelectableEntry[]` (orphans prepended, disabled-state - * tooltip) + * - assembly of the `DimensionEntry[]` (orphans prepended, disabled-state + * tooltip overlay appended at the max limit) * - change + clear handlers (debounced for multi-select, immediate for * single) - * - the button label, tooltip, and popover footer nodes + * - the button label, tooltip, and popover footer nodes (rendered as + * dedicated components in `../dimensions_selector_components`) */ export const useDimensionsSelector = ({ dimensions, @@ -72,7 +66,6 @@ export const useDimensionsSelector = ({ isLoading, metricItems, }: UseDimensionsSelectorParams): UseDimensionsSelectorResult => { - const { euiTheme } = useEuiTheme(); const [localSelectedDimensions, setLocalSelectedDimensions] = useState(selectedDimensions); @@ -113,30 +106,7 @@ export const useDimensionsSelector = ({ dimension, isSelected, isDisabled, - appendNode: showMaxTooltip ? ( - - } - position="top" - anchorProps={{ - css: css` - position: absolute; - inset: 0; - width: 100%; - height: 100%; - pointer-events: auto; - z-index: ${euiTheme.levels.menu}; - `, - }} - > -
- - ) : undefined, + appendNode: showMaxTooltip ? : undefined, }); }; @@ -149,7 +119,6 @@ export const useDimensionsSelector = ({ optimisticApplicableNames, selectedNamesSet, singleSelection, - euiTheme.levels.menu, ]); const onChangeRef = useRef(onChange); @@ -210,101 +179,27 @@ export const useDimensionsSelector = ({ onChange([]); }, [onChange, debouncedOnChange]); - const buttonLabel = useMemo(() => { - const count = localSelectedDimensions.length; - - return ( - - - {count === 0 ? ( - - ) : ( - - - - - - {count} - - - )} - - {isLoading && ( - - - - )} - - ); - }, [localSelectedDimensions, isLoading]); + const buttonLabel = useMemo( + () => ( + + ), + [localSelectedDimensions.length, isLoading] + ); const buttonTooltipContent = useMemo(() => { - const count = localSelectedDimensions.length; - const isAtMaxDimensions = count >= MAX_DIMENSIONS_SELECTIONS; - - if (!isAtMaxDimensions) { - return undefined; - } - - return ( - = MAX_DIMENSIONS_SELECTIONS; + return isAtMaxDimensions ? : undefined; + }, [localSelectedDimensions.length]); + + const popoverContentBelowSearch = useMemo( + () => ( + - ); - }, [localSelectedDimensions]); - - const popoverContentBelowSearch = useMemo(() => { - const count = localSelectedDimensions.length; - return ( - <> - - - - - - - - {count > 0 && ( - - - - - - )} - - - - ); - }, [localSelectedDimensions.length, handleClearAll, euiTheme.size.l]); + ), + [localSelectedDimensions.length, handleClearAll] + ); const selectedValues = useMemo(() => [...selectedNamesSet], [selectedNamesSet]); From 191f189f9079d3b2d8563d7d5f81bdf552b64f64 Mon Sep 17 00:00:00 2001 From: Justin Kambic Date: Tue, 21 Apr 2026 14:55:20 -0400 Subject: [PATCH 19/19] test(dimensions-selector): add unit tests for useDimensionsSelector Covers the hook's full logic surface directly, rather than only through the surrounding `DimensionsSelector` component: - `options`: dimension carried on entry, checked marker, multi-select at-limit disable, tooltip overlay only on disabled-at-limit rows, no disabling in single-select mode, orphan prepending, optimistic metricItems filter. - `selectedValues`: de-duplicated name list. - Local/controlled sync: `selectedDimensions` prop change re-syncs. - `handleChange` (multi): debounces at `DEBOUNCE_TIME`, collapses rapid changes, caps at `MAX_DIMENSIONS_SELECTIONS`, handles `undefined` input. - `handleChange` (single): synchronous, no debounce. - Presentational nodes: `buttonTooltipContent` only when at-max, `buttonLabel`/`popoverContentBelowSearch` always present. - Defensive: options missing a `dimension` field are dropped, not crashed on. Also picks up `eslint --fix` formatting tweaks in the hook file for the now-one-line `DimensionsButtonLabel` / `DimensionsPopoverFooter` JSX that followed the extraction in the previous commit. Comment addressed: - https://github.com/elastic/kibana/pull/263629#discussion_r3116463842 (#1) --- .../hooks/use_dimensions_selector.test.tsx | 416 ++++++++++++++++++ .../toolbar/hooks/use_dimensions_selector.tsx | 9 +- 2 files changed, 418 insertions(+), 7 deletions(-) create mode 100644 src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.test.tsx diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.test.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.test.tsx new file mode 100644 index 0000000000000..90531f8dfa554 --- /dev/null +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.test.tsx @@ -0,0 +1,416 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import React from 'react'; +import type { ReactNode } from 'react'; +import { act, renderHook } from '@testing-library/react'; +import { __IntlProvider as IntlProvider } from '@kbn/i18n-react'; +import type { SelectableEntry } from '@kbn/shared-ux-toolbar-selector'; +import { useDimensionsSelector } from './use_dimensions_selector'; +import type { Dimension, ParsedMetricItem } from '../../../types'; +import { DEBOUNCE_TIME, MAX_DIMENSIONS_SELECTIONS } from '../../../common/constants'; +import type { DimensionEntry } from '../dimensions_selector_helpers'; + +const wrapper = ({ children }: { children: ReactNode }) => ( + {children} +); + +const dim = (name: string, type: string = 'keyword'): Dimension => ({ name, type }); + +const makeMetric = (metricName: string, dimensionFields: Dimension[]): ParsedMetricItem => ({ + metricName, + dataStream: 'metrics-test', + units: [], + metricTypes: [], + fieldTypes: [], + dimensionFields, +}); + +type HookArgs = Parameters[0]; + +/** + * Render the hook with a stable arg object. The hook has a + * `useEffect([selectedDimensions])` that re-syncs local state on every + * reference change, so tests MUST pass the same array identity across + * internal re-renders to avoid tripping React's infinite-update guard. + * `renderHook`'s re-render calls this callback with the original `args` + * reference, preserving identity. + */ +const renderDimensionsHook = (args: HookArgs) => + renderHook(() => useDimensionsSelector(args), { wrapper }); + +describe('useDimensionsSelector', () => { + describe('options', () => { + it('attaches the source Dimension to each option so handleChange can read it back', () => { + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name'), dim('cloud.region')], + selectedDimensions: [], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + const options = result.current.options as DimensionEntry[]; + expect(options.map((o) => o.dimension.name)).toEqual([ + 'host.name', + 'service.name', + 'cloud.region', + ]); + expect(options.every((o) => typeof o.dimension === 'object')).toBe(true); + }); + + it('marks selected options as checked and leaves the rest unchecked', () => { + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name'), dim('cloud.region')], + selectedDimensions: [dim('service.name')], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + const byValue = Object.fromEntries(result.current.options.map((o) => [o.value, o.checked])); + expect(byValue['service.name']).toBe('on'); + expect(byValue['host.name']).toBeUndefined(); + expect(byValue['cloud.region']).toBeUndefined(); + }); + + it('disables unselected options when at the max selection limit (multi)', () => { + const selected = Array.from({ length: MAX_DIMENSIONS_SELECTIONS }, (_, i) => dim(`d${i}`)); + const all = [...selected, dim('extra.one'), dim('extra.two')]; + const { result } = renderDimensionsHook({ + dimensions: all, + selectedDimensions: selected, + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + const byValue = Object.fromEntries(result.current.options.map((o) => [o.value, o])); + // Selected options stay enabled so they can be deselected. + selected.forEach((d) => expect(byValue[d.name].disabled).toBe(false)); + // Unselected options are disabled once the limit is reached. + expect(byValue['extra.one'].disabled).toBe(true); + expect(byValue['extra.two'].disabled).toBe(true); + }); + + it('appends the tooltip overlay only on disabled-at-limit options', () => { + const selected = Array.from({ length: MAX_DIMENSIONS_SELECTIONS }, (_, i) => dim(`d${i}`)); + const all = [...selected, dim('extra.one')]; + const { result } = renderDimensionsHook({ + dimensions: all, + selectedDimensions: selected, + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + const byValue = Object.fromEntries(result.current.options.map((o) => [o.value, o])); + // Selected entries are enabled, so no max-limit overlay. + expect(byValue.d0.append).toBeUndefined(); + // Disabled-at-limit gets the overlay. + expect(byValue['extra.one'].append).toBeDefined(); + }); + + it('never disables options in single-selection mode, even past the limit', () => { + // `singleSelection: true` short-circuits the at-max-limit check. + const selected = Array.from({ length: MAX_DIMENSIONS_SELECTIONS + 2 }, (_, i) => + dim(`d${i}`) + ); + const { result } = renderDimensionsHook({ + dimensions: selected, + selectedDimensions: selected, + onChange: jest.fn(), + singleSelection: true, + isLoading: false, + }); + + expect(result.current.options.every((o) => o.disabled === false)).toBe(true); + }); + + it('prepends orphan selections (dimensions not in `dimensions`) alphabetically', () => { + // Orphan = selected dimension not present in the `dimensions` array. + // This mirrors the URL-restore case where selections outlive the list. + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name')], + selectedDimensions: [dim('zeta.orphan'), dim('alpha.orphan')], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + expect(result.current.options.map((o) => o.value)).toEqual([ + 'alpha.orphan', + 'zeta.orphan', + 'host.name', + ]); + }); + + it('applies the optimistic filter when metricItems is provided', () => { + // Only metrics that carry `service.name` also carry `cloud.region` here, + // so picking `service.name` should hide `host.name` from the suggestion + // set. + const metricItems: ParsedMetricItem[] = [ + makeMetric('m1', [dim('service.name'), dim('cloud.region')]), + makeMetric('m2', [dim('host.name')]), + ]; + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name'), dim('cloud.region')], + selectedDimensions: [dim('service.name')], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + metricItems, + }); + + const values = result.current.options.map((o) => o.value); + expect(values).toContain('service.name'); + expect(values).toContain('cloud.region'); + expect(values).not.toContain('host.name'); + }); + }); + + describe('selectedValues', () => { + it('returns the de-duplicated names of local selections', () => { + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name')], + selectedDimensions: [dim('host.name'), dim('service.name')], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + + expect(result.current.selectedValues).toEqual(['host.name', 'service.name']); + }); + }); + + describe('local/controlled-prop sync', () => { + it('re-syncs local state when `selectedDimensions` prop changes', () => { + // All non-changing args share the same reference across renders so only + // `selectedDimensions` can trigger the sync effect. + const dimensions = [dim('host.name'), dim('service.name')]; + const onChange = jest.fn(); + + const { result, rerender } = renderHook( + ({ selectedDimensions }: { selectedDimensions: Dimension[] }) => + useDimensionsSelector({ + dimensions, + selectedDimensions, + onChange, + singleSelection: false, + isLoading: false, + }), + { wrapper, initialProps: { selectedDimensions: [] as Dimension[] } } + ); + + expect(result.current.selectedValues).toEqual([]); + + rerender({ selectedDimensions: [dim('host.name')] }); + expect(result.current.selectedValues).toEqual(['host.name']); + }); + }); + + describe('handleChange (multi-select)', () => { + beforeEach(() => jest.useFakeTimers()); + afterEach(() => { + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + }); + + it('debounces onChange by DEBOUNCE_TIME ms', () => { + const onChange = jest.fn(); + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name'), dim('cloud.region')], + selectedDimensions: [], + onChange, + singleSelection: false, + isLoading: false, + }); + + const hostOption = (result.current.options as DimensionEntry[]).find( + (o) => o.value === 'host.name' + )!; + + act(() => { + result.current.handleChange([{ ...hostOption, checked: 'on' }]); + }); + + // Pre-debounce: no onChange fired yet, but local state updates immediately + // so selectedValues already reflects the pick. + expect(onChange).not.toHaveBeenCalled(); + expect(result.current.selectedValues).toEqual(['host.name']); + + act(() => { + jest.advanceTimersByTime(DEBOUNCE_TIME); + }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith([dim('host.name')]); + }); + + it('collapses rapid consecutive changes into a single onChange call', () => { + const onChange = jest.fn(); + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name'), dim('cloud.region')], + selectedDimensions: [], + onChange, + singleSelection: false, + isLoading: false, + }); + const opts = result.current.options as DimensionEntry[]; + const host = opts.find((o) => o.value === 'host.name')!; + const service = opts.find((o) => o.value === 'service.name')!; + + act(() => { + result.current.handleChange([{ ...host, checked: 'on' }]); + result.current.handleChange([ + { ...host, checked: 'on' }, + { ...service, checked: 'on' }, + ]); + }); + + act(() => { + jest.advanceTimersByTime(DEBOUNCE_TIME); + }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith([dim('host.name'), dim('service.name')]); + }); + + it('caps the emitted selection at MAX_DIMENSIONS_SELECTIONS', () => { + const onChange = jest.fn(); + const extras = Array.from({ length: MAX_DIMENSIONS_SELECTIONS + 2 }, (_, i) => dim(`d${i}`)); + const { result } = renderDimensionsHook({ + dimensions: extras, + selectedDimensions: [], + onChange, + singleSelection: false, + isLoading: false, + }); + + const selections = (result.current.options as DimensionEntry[]).map((o) => ({ + ...o, + checked: 'on' as const, + })); + + act(() => { + result.current.handleChange(selections); + }); + act(() => { + jest.advanceTimersByTime(DEBOUNCE_TIME); + }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0]).toHaveLength(MAX_DIMENSIONS_SELECTIONS); + }); + + it('coerces `undefined` into an empty-array selection', () => { + const onChange = jest.fn(); + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name')], + selectedDimensions: [], + onChange, + singleSelection: false, + isLoading: false, + }); + + act(() => { + result.current.handleChange(undefined); + }); + act(() => { + jest.advanceTimersByTime(DEBOUNCE_TIME); + }); + + expect(onChange).toHaveBeenCalledWith([]); + }); + }); + + describe('handleChange (single-select)', () => { + it('fires onChange synchronously without debouncing', () => { + const onChange = jest.fn(); + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name'), dim('service.name')], + selectedDimensions: [], + onChange, + singleSelection: true, + isLoading: false, + }); + + const hostOption = (result.current.options as DimensionEntry[]).find( + (o) => o.value === 'host.name' + )!; + + act(() => { + // Real ToolbarSelector emits a single option (not an array) in single-selection mode. + result.current.handleChange({ ...hostOption, checked: 'on' }); + }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith([dim('host.name')]); + }); + }); + + describe('button & popover nodes', () => { + it('exposes a `buttonTooltipContent` only when at the max limit', () => { + const atLimit = Array.from({ length: MAX_DIMENSIONS_SELECTIONS }, (_, i) => dim(`d${i}`)); + + const { result: belowLimit } = renderDimensionsHook({ + dimensions: atLimit, + selectedDimensions: atLimit.slice(0, 1), + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + expect(belowLimit.current.buttonTooltipContent).toBeUndefined(); + + const { result: atTheLimit } = renderDimensionsHook({ + dimensions: atLimit, + selectedDimensions: atLimit, + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + expect(atTheLimit.current.buttonTooltipContent).toBeDefined(); + }); + + it('always provides a `buttonLabel` and `popoverContentBelowSearch`', () => { + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name')], + selectedDimensions: [], + onChange: jest.fn(), + singleSelection: false, + isLoading: false, + }); + expect(result.current.buttonLabel).toBeDefined(); + expect(result.current.popoverContentBelowSearch).toBeDefined(); + }); + }); + + describe('type discipline', () => { + it('gracefully drops options missing a `dimension` field (defensive guard)', () => { + // Simulates a third-party shape that slipped through without a dimension. + // `handleChange` must not crash; it should just produce an empty selection. + const onChange = jest.fn(); + const { result } = renderDimensionsHook({ + dimensions: [dim('host.name')], + selectedDimensions: [], + onChange, + singleSelection: true, + isLoading: false, + }); + + act(() => { + const rogue: SelectableEntry = { value: 'mystery', label: 'mystery', key: 'mystery' }; + result.current.handleChange([rogue]); + }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledWith([]); + }); + }); +}); diff --git a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx index 3c3d2040cfa96..e5ff7816e0753 100644 --- a/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx +++ b/src/platform/packages/shared/kbn-unified-chart-section-viewer/src/components/toolbar/hooks/use_dimensions_selector.tsx @@ -180,9 +180,7 @@ export const useDimensionsSelector = ({ }, [onChange, debouncedOnChange]); const buttonLabel = useMemo( - () => ( - - ), + () => , [localSelectedDimensions.length, isLoading] ); @@ -193,10 +191,7 @@ export const useDimensionsSelector = ({ const popoverContentBelowSearch = useMemo( () => ( - + ), [localSelectedDimensions.length, handleClearAll] );