Fix the Filter & Group editor: add-after-result, Reset, row identity, validation

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 <GroupingEditor config={grouping} onApply={update} />;
+  return (
+    <GroupingEditor
+      config={grouping}
+      defaultConfig={defaultGrouping}
+      onApply={update}
+    />
+  );
 };
 
 export default GroupingConfig;

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<DisplayGroup[]>(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) => (
           <GroupingEditorItem
-            key={`${c.type}-${c.value}`}
+            // Key by POSITION, not by content. A newly added row is {type: ""},
+            // so a content key is `-undefined` - add two rows and both children
+            // share one key. React's child map for the list is then ambiguous
+            // and later updates cannot reliably unmount the right rows: after
+            // Reset a discarded row survived on screen (and a second Reset could
+            // not clear it) even though innerConfig, the saved grouping and the
+            // rendered tree were all correct. Only a remount fixed it.
+            //
+            // Reorder identity is carried by Reorder.Item's `value={item}` prop,
+            // not by the React key, so dragging is unaffected.
+            key={(c as any).__rowId ?? idx}
             config={innerConfig}
             item={c}
             index={idx}
@@ -272,9 +329,44 @@ const GroupingEditor = ({ config, onApply }: GroupingEditorProps) => {
         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"
       />
     </div>

ui/dashboard/src/components/dashboards/grouping/common/EditorAddItem.tsx

@@ -39,6 +39,12 @@ const EditorAddItem = ({
         </div>
       </div>
       <div className="flex items-center justify-end space-x-3">
+        {/* 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 && (
+          <span className="text-sm text-alert">{isValid.reason}</span>
+        )}
         <span
           className="text-sm text-foreground-lighter cursor-pointer hover:text-link mr-2"
           onClick={onClear}

ui/dashboard/src/hooks/useGroupingConfig.ts

@@ -20,6 +20,37 @@ const useGroupingConfig = (panelName?: string) => {
     }
   }, [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;