refactor(studio): one validateFlowDraft pass — derive red-error highlight from the unified problem list

.changeset/flow-problems-single-source.md

@@ -0,0 +1,18 @@
+---
+"@object-ui/app-shell": patch
+---
+
+refactor(studio): derive the flow red-error highlight from the unified problem list (one validateFlowDraft pass)
+
+Follow-up to #1972 (Problems panel + badges) and #1976 (clickable banner). The
+flow preview still ran `validateFlowDraft` twice per render — once in
+`buildFlowProblems` (badges / banner / panel) and again in a separate memo that
+derived the red node/edge ring/stroke — with the cycle-highlight logic duplicated
+between them.
+
+`buildFlowProblems` is now the single validation pass: a new
+`deriveInvalidElements(problems)` produces the red error set (errors only; a
+cycle paints its whole loop via a per-problem `highlight` set while its badge +
+reveal stay on the closing edge). The preview drops its second `validateFlowDraft`
+call. The clickable banner (#1976), badges, and panel are unchanged — all four
+surfaces now derive from one list, so they cannot drift.

packages/app-shell/src/views/metadata-admin/previews/FlowPreview.tsx

@@ -39,10 +39,8 @@ import { FlowCanvas } from './FlowCanvas';
 import { edgeKey } from './flow-canvas-layout';
 import { FlowSimulatorPanel } from './FlowSimulatorPanel';
 import { FlowRunsPanel } from './FlowRunsPanel';
-import { validateFlowDraft } from './simulator/flow-sim-validate';
-import type { SimEdge, SimNode } from './simulator/flow-sim-types';
 import { ProblemsPanel } from './ProblemsPanel';
-import { buildFlowProblems, type FlowProblem } from './flow-problems';
+import { buildFlowProblems, deriveInvalidElements, type FlowProblem } from './flow-problems';
 
 interface FlowNode {
   id: string;
@@ -95,35 +93,21 @@ export function FlowPreview({ draft, editing, selection, onSelectionChange, onPa
     traversedEdgeIds: string[];
   } | null>(null);
 
-  // Continuous structural validation paints the offending edges/nodes red on the
-  // canvas (ADR-0044) — so an un-declared cycle is visible without opening Debug.
-  // Same `validateFlowDraft` the simulator preflight uses; the inline banner and
-  // per-element badges are driven by the unified `problems` list below.
-  const { invalidNodeIds, invalidEdges } = React.useMemo(() => {
-    const v = validateFlowDraft(nodes as unknown as SimNode[], edges as unknown as SimEdge[]);
-    const nodeSet = new Set<string>();
-    const edgeSet = new Set<string>();
-    for (const diag of v.errors) {
-      if (diag.nodeId) nodeSet.add(diag.nodeId);
-      // A cycle error carries its closing node path → mark each hop's edge red.
-      if (diag.cycle) {
-        for (let i = 0; i < diag.cycle.length - 1; i++) {
-          nodeSet.add(diag.cycle[i]);
-          edgeSet.add(`${diag.cycle[i]}->${diag.cycle[i + 1]}`);
-        }
-      }
-    }
-    return { invalidNodeIds: [...nodeSet], invalidEdges: edgeSet };
-  }, [nodes, edges]);
-
-  // Unified problem list (structural + server `_diagnostics`) shared by the
-  // on-canvas badges and the Problems panel. Recomputed from the live draft so
-  // badges + rows clear as the author fixes each issue.
+  // Unified problem list (structural + server `_diagnostics`) is the SINGLE
+  // source for every validation surface — the clickable inline banner, the
+  // per-element badges, the red error ring/stroke, and the Problems panel.
+  // Recomputed from the live draft so they all clear as the author fixes each issue.
   const problems = React.useMemo<FlowProblem[]>(
     () => buildFlowProblems({ nodes, edges, serverDiagnostics: diagnostics }),
     [nodes, edges, diagnostics],
   );
   const errorCount = problems.filter((p) => p.level === 'error').length;
+  // Red error ring/stroke derived from the same list (errors only; a cycle
+  // paints its whole loop) — no second validateFlowDraft pass.
+  const { invalidNodeIds, invalidEdges } = React.useMemo(
+    () => deriveInvalidElements(problems),
+    [problems],
+  );
 
   // "Reveal" handshake with the canvas: a changing nonce pans to the element.
   const [reveal, setReveal] = React.useState<{ target: FlowProblem['target']; nonce: number } | null>(null);

packages/app-shell/src/views/metadata-admin/previews/flow-problems.test.ts

@@ -2,7 +2,7 @@
 
 import { describe, it, expect } from 'vitest';
 import { validateFlowDraft } from './simulator/flow-sim-validate';
-import { buildFlowProblems, indexProblemBadges } from './flow-problems';
+import { buildFlowProblems, indexProblemBadges, deriveInvalidElements } from './flow-problems';
 
 describe('validateFlowDraft — per-element keying', () => {
   it('attaches the edge endpoints to a dangling-edge error', () => {
@@ -161,3 +161,35 @@ describe('indexProblemBadges', () => {
     expect(byNode.get('d')?.count).toBeGreaterThanOrEqual(2);
   });
 });
+
+describe('deriveInvalidElements', () => {
+  it('flags every cycle hop (node + edge) for the red error set', () => {
+    const problems = buildFlowProblems({
+      nodes: [
+        { id: 'a', type: 'decision' },
+        { id: 'b', type: 'create_record' },
+      ],
+      edges: [
+        { source: 'a', target: 'b' },
+        { source: 'b', target: 'a' },
+      ],
+    });
+    const { invalidNodeIds, invalidEdges } = deriveInvalidElements(problems);
+    expect(new Set(invalidNodeIds)).toEqual(new Set(['a', 'b']));
+    expect(invalidEdges.has('a->b')).toBe(true);
+    expect(invalidEdges.has('b->a')).toBe(true);
+  });
+
+  it('excludes warning-only elements from the red set', () => {
+    const problems = buildFlowProblems({
+      nodes: [
+        { id: 's', type: 'start' },
+        { id: 'd', type: 'decision' }, // no outgoing → warning only
+      ],
+      edges: [{ source: 's', target: 'd' }],
+    });
+    const { invalidNodeIds, invalidEdges } = deriveInvalidElements(problems);
+    expect(invalidNodeIds).not.toContain('d');
+    expect(invalidEdges.size).toBe(0);
+  });
+});

packages/app-shell/src/views/metadata-admin/previews/flow-problems.ts

@@ -39,6 +39,11 @@ export interface FlowProblem {
   message: string;
   target: FlowProblemTarget;
   source: FlowProblemSource;
+  /**
+   * Extra elements to flag with the red error ring/stroke beyond `target` —
+   * e.g. every hop of a cycle. The badge + click-reveal still use `target`.
+   */
+  highlight?: { nodeIds: string[]; edges: Array<{ source: string; target: string }> };
 }
 
 /** A server diagnostic entry (subset of the layered record's `_diagnostics`). */
@@ -71,23 +76,40 @@ function pathSegments(path: ServerDiagnostic['path']): Array<string | number> {
   });
 }
 
+/** A structural diagnostic mapped to its badge target + optional red-highlight set. */
+interface StructuralMapping {
+  target: FlowProblemTarget;
+  highlight?: { nodeIds: string[]; edges: Array<{ source: string; target: string }> };
+}
+
 /**
  * Map a structural diagnostic's optional anchors (`edge`, `cycle`, `nodeId`)
- * onto a single canvas target. A cycle points at the *closing* hop — the edge
- * the author marks as a back-edge to resolve it.
+ * onto a badge target. A cycle points its badge at the *closing* hop — the edge
+ * the author marks as a back-edge to resolve it — but flags EVERY hop (nodes +
+ * edges) for the red error highlight so the whole loop reads as the problem.
  */
-function structuralTarget(diag: Diagnostic, edges: FlowEdge[]): FlowProblemTarget {
+function structuralMapping(diag: Diagnostic, edges: FlowEdge[]): StructuralMapping {
   if (diag.edge) {
     const { source, target } = diag.edge;
-    return { kind: 'edge', source, target, edgeKey: resolveEdgeKey(edges, source, target) };
+    return { target: { kind: 'edge', source, target, edgeKey: resolveEdgeKey(edges, source, target) } };
   }
   if (diag.cycle && diag.cycle.length >= 2) {
-    const source = diag.cycle[diag.cycle.length - 2];
-    const target = diag.cycle[diag.cycle.length - 1];
-    return { kind: 'edge', source, target, edgeKey: resolveEdgeKey(edges, source, target) };
+    const c = diag.cycle;
+    const source = c[c.length - 2];
+    const target = c[c.length - 1];
+    const nodeIds: string[] = [];
+    const hops: Array<{ source: string; target: string }> = [];
+    for (let i = 0; i < c.length - 1; i++) {
+      nodeIds.push(c[i]);
+      hops.push({ source: c[i], target: c[i + 1] });
+    }
+    return {
+      target: { kind: 'edge', source, target, edgeKey: resolveEdgeKey(edges, source, target) },
+      highlight: { nodeIds, edges: hops },
+    };
   }
-  if (diag.nodeId) return { kind: 'node', nodeId: diag.nodeId };
-  return { kind: 'flow' };
+  if (diag.nodeId) return { target: { kind: 'node', nodeId: diag.nodeId } };
+  return { target: { kind: 'flow' } };
 }
 
 /** Map a server diagnostic's JSON path onto a node/edge/flow target. */
@@ -129,13 +151,14 @@ export function buildFlowProblems({ nodes, edges, serverDiagnostics }: BuildFlow
   const v = validateFlowDraft(nodes as unknown as SimNode[], edges as unknown as SimEdge[]);
   const pushStructural = (level: DiagnosticLevel, list: Diagnostic[]) => {
     list.forEach((diag, i) => {
-      const target = structuralTarget(diag, edges);
+      const { target, highlight } = structuralMapping(diag, edges);
       problems.push({
         id: `structural:${level}:${i}:${targetKey(target)}`,
         level,
         message: diag.message,
         target,
         source: 'structural',
+        ...(highlight ? { highlight } : {}),
       });
     });
   };
@@ -204,3 +227,26 @@ export function indexProblemBadges(problems: FlowProblem[]): ProblemIndex {
   for (const [k, list] of edgeLists) byEdge.set(k, foldBadge(list));
   return { byNode, byEdge };
 }
+
+/**
+ * Error elements to paint with the red ring/stroke, derived from the unified
+ * problem list. ERRORS ONLY — warnings get an amber badge but no ring. Includes
+ * each error's `highlight` set, so a cycle paints its whole loop (every hop node
+ * + edge) red while its badge still sits on the closing edge. Lets the preview
+ * derive the red sets from `problems` instead of a second validateFlowDraft pass.
+ */
+export function deriveInvalidElements(problems: FlowProblem[]): {
+  invalidNodeIds: string[];
+  invalidEdges: Set<string>;
+} {
+  const nodeSet = new Set<string>();
+  const edgeSet = new Set<string>();
+  for (const p of problems) {
+    if (p.level !== 'error') continue;
+    if (p.target.kind === 'node') nodeSet.add(p.target.nodeId);
+    else if (p.target.kind === 'edge') edgeSet.add(edgeProblemKey(p.target.source, p.target.target));
+    for (const id of p.highlight?.nodeIds ?? []) nodeSet.add(id);
+    for (const e of p.highlight?.edges ?? []) edgeSet.add(edgeProblemKey(e.source, e.target));
+  }
+  return { invalidNodeIds: [...nodeSet], invalidEdges: edgeSet };
+}