diff --git a/ui/dashboard/src/components/dashboards/grouping/GroupingConfig/index.tsx b/ui/dashboard/src/components/dashboards/grouping/GroupingConfig/index.tsx index 3e01c1c67..cf4737a5e 100644 --- a/ui/dashboard/src/components/dashboards/grouping/GroupingConfig/index.tsx +++ b/ui/dashboard/src/components/dashboards/grouping/GroupingConfig/index.tsx @@ -6,9 +6,15 @@ type GroupingConfigProps = { }; const GroupingConfig = ({ panelName }: GroupingConfigProps) => { - const { grouping, update } = useGroupingConfig(panelName); + const { grouping, defaultGrouping, update } = useGroupingConfig(panelName); - return ; + return ( + + ); }; export default GroupingConfig; diff --git a/ui/dashboard/src/components/dashboards/grouping/GroupingEditor/index.tsx b/ui/dashboard/src/components/dashboards/grouping/GroupingEditor/index.tsx index fb5e72e2a..ceaa3b0bd 100644 --- a/ui/dashboard/src/components/dashboards/grouping/GroupingEditor/index.tsx +++ b/ui/dashboard/src/components/dashboards/grouping/GroupingEditor/index.tsx @@ -17,6 +17,7 @@ import { useDashboardControls } from "@powerpipe/components/dashboards/layout/Da type GroupingEditorProps = { config: DisplayGroup[]; + defaultConfig: DisplayGroup[]; onApply: (newValue: DisplayGroup[]) => void; }; @@ -174,23 +175,61 @@ const GroupingEditorItem = ({ ); }; -const GroupingEditor = ({ config, onApply }: GroupingEditorProps) => { +// Rows need a stable identity of their own. +// +// Keying by content (`${type}-${value}`) collides: a newly added row is +// {type: ""}, so adding two produces two children with the key "-undefined". +// React's child map for the list is then ambiguous and later updates strand +// rows on screen - a Reset-discarded row survived, and a second Reset could not +// clear it, even though state, saved grouping and the rendered tree were right. +// +// Keying by index is worse: the react-select at a given position keeps its +// displayed value, so inserting rows made the new ones render as "Result". +// +// So each row carries an id, generated when the row is created and preserved +// across edits and reordering. It is stripped before saving - onApply writes +// straight to the URL, and this is presentation state, not part of the grouping. +let nextRowId = 0; + +const withRowIds = (groups: DisplayGroup[]): DisplayGroup[] => + groups.map((c) => ({ ...c, __rowId: `row-${nextRowId++}` })) as DisplayGroup[]; + +const stripRowIds = (groups: DisplayGroup[]): DisplayGroup[] => + groups.map((c) => { + const { __rowId, ...rest } = c as any; + return rest; + }) as DisplayGroup[]; + +const GroupingEditor = ({ + config, + defaultConfig, + onApply, +}: GroupingEditorProps) => { const [innerConfig, setInnerConfig] = useState(config); const [isDirty, setIsDirty] = useState(false); const [isValid, setIsValid] = useState({ value: false, reason: "" }); useEffect(() => { - setInnerConfig( - config.map((c) => ({ - ...c, - type: c.type, - value: c.value, - })) as any, - ); + setInnerConfig(withRowIds(config) as any); }, [config, setInnerConfig]); useEffect(() => { let reason: string = ""; + + // Every level must have a type. The switch below falls through to + // `default: return true`, so an untyped row counted as valid and Apply + // saved {"type":""} into the URL - a level that groups nothing. + const untyped = innerConfig.some((c) => !c?.type); + + // ...and each level may only appear once. A repeat cannot subdivide + // anything its twin has not already split, so it is silently inert. + // control_tag/dimension are keyed with their value, so `domain` and + // `label` remain distinct levels. + const keys = innerConfig + .filter((c) => !!c?.type) + .map((c) => `${c.type}:${c.value ?? ""}`); + const duplicated = keys.length !== new Set(keys).size; + const isValid = innerConfig.every((c, i) => { switch (c?.type) { case "benchmark": @@ -214,9 +253,16 @@ const GroupingEditor = ({ config, onApply }: GroupingEditorProps) => { return true; } }); - setIsValid({ value: isValid, reason }); + // Reported after the positional checks so the most specific message wins. + if (untyped) { + setIsValid({ value: false, reason: "Choose a type for every grouping" }); + } else if (duplicated) { + setIsValid({ value: false, reason: "Each grouping can only be used once" }); + } else { + setIsValid({ value: isValid, reason }); + } - const removeEmpty = innerConfig.map((c) => { + const removeEmpty = stripRowIds(innerConfig).map((c) => { const noEmpty = {}; for (const [k, v] of Object.entries(c)) { if (!v) { @@ -242,7 +288,8 @@ const GroupingEditor = ({ config, onApply }: GroupingEditorProps) => { (index: number, updatedItem: DisplayGroup) => setInnerConfig((existing) => [ ...existing.slice(0, index), - updatedItem, + // keep the row's id: the child rebuilds the item and would drop it + { ...updatedItem, __rowId: (existing[index] as any)?.__rowId } as any, ...existing.slice(index + 1), ]), [setInnerConfig], @@ -259,7 +306,17 @@ const GroupingEditor = ({ config, onApply }: GroupingEditorProps) => { > {innerConfig.map((c, idx) => ( { isDirty={isDirty} isValid={isValid} // @ts-ignore - onAdd={() => setInnerConfig((existing) => [...existing, { type: "" }])} - onApply={() => onApply(innerConfig)} - onClear={() => onApply([])} + onAdd={() => + // Insert BEFORE a trailing "result" rather than appending blindly. + // "result" is the leaf level - there is nothing beneath a result to + // subdivide - so the validator below requires it to be last. Appending + // after it produced the one arrangement that is always invalid, which + // greyed out Apply with the explanation hidden in a title tooltip. + setInnerConfig((existing) => { + const resultIndex = existing.findIndex((c) => c.type === "result"); + return resultIndex === -1 + ? [...existing, { type: "", __rowId: `row-${nextRowId++}` } as any] + : [ + ...existing.slice(0, resultIndex), + { type: "", __rowId: `row-${nextRowId++}` } as any, + ...existing.slice(resultIndex), + ]; + }) + } + onApply={() => onApply(stripRowIds(innerConfig))} + onClear={() => { + // Show the DEFAULT immediately, then clear the saved grouping. + // + // Two traps here, both hit in earlier attempts: + // + // - onApply([]) alone does nothing visible. It removes this panel's + // saved entry so useGroupingConfig falls back to the default, but + // when the panel is already on the default the search params do + // not change, so nothing re-renders and the useEffect that syncs + // innerConfig never fires. + // - Setting innerConfig to `config` is worse than useless: `config` + // is the grouping being discarded, so the rows being removed were + // briefly re-asserted and could survive the reset. + // + // defaultConfig is what onApply([]) will resolve to, computed by + // useGroupingConfig - which knows the control vs detection default, + // so this stays correct for detection benchmarks too. + setInnerConfig(withRowIds(defaultConfig) as any); + onApply([]); + }} addLabel="Add grouping" /> diff --git a/ui/dashboard/src/components/dashboards/grouping/common/EditorAddItem.tsx b/ui/dashboard/src/components/dashboards/grouping/common/EditorAddItem.tsx index a5f6a91a2..750ba40d5 100644 --- a/ui/dashboard/src/components/dashboards/grouping/common/EditorAddItem.tsx +++ b/ui/dashboard/src/components/dashboards/grouping/common/EditorAddItem.tsx @@ -39,6 +39,12 @@ const EditorAddItem = ({
+ {/* Say WHY Apply is disabled. The reason was only ever exposed as the + button's title attribute, so a blocked save looked like a dead + button - the user had no way to know what to change. */} + {!isValid.value && !!isValid.reason && ( + {isValid.reason} + )} { } }, [searchParams]); + // The grouping a panel falls back to when nothing is saved. Exposed so the + // editor's Reset can restore it explicitly rather than relying on a re-render + // that does not happen when the panel is already on the default. + const defaultGrouping = useMemo(() => { + if (!panel) { + return [] as DisplayGroup[]; + } + if ( + (!panel.benchmark_type || panel.benchmark_type === "control") && + (panel.panel_type === "benchmark" || panel.panel_type === "control") + ) { + return [ + { type: "benchmark" }, + { type: "control" }, + { type: "result" }, + ] as DisplayGroup[]; + } + if ( + (panel.benchmark_type === "detection" && + panel.panel_type === "benchmark") || + panel.panel_type === "detection" + ) { + return [ + { type: "benchmark" }, + { type: "detection" }, + { type: "result" }, + ] as DisplayGroup[]; + } + return [] as DisplayGroup[]; + }, [panel]); + const grouping = useMemo(() => { if (!panel) { return [] as DisplayGroup[]; @@ -84,7 +115,7 @@ const useGroupingConfig = (panelName?: string) => { }); }; - return { allGroupings, grouping, update }; + return { allGroupings, grouping, defaultGrouping, update }; }; export default useGroupingConfig;