From b99f4ef345bf277499af62f721fcfc3dfa38a8c5 Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sun, 30 Aug 2026 09:36:02 -0700 Subject: [PATCH 1/5] feat(harness): the project map draws the groups the rail already has [SAP-2983] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A project map drew every contained agent as one flat set, ignoring the sub-structure the rail was showing beside it: one root holding nine systems and 76 agents came out as a single ~70-node column. The mechanism already existed. `lib/agent-groups.ts` derives groups from launch edges, lets the user edit them, and persists the arrangement per project root. The map simply never read it. - `lib/system-graph-groups.ts` joins the rail's rows to graph nodes, as an exhaustive partition — first claim wins for a shared subagent, and a node no row resolved falls to Ungrouped rather than off the map. - `lib/system-graph-layout.ts` lays each container out in its own coordinates and measures it AFTER routing, so a cycle gutter or a displaced label can never cross the border drawn around its system. Weak components inside a container now shelf-pack instead of stacking, which is what stops the column coming back inside `Ungrouped`. - `use-rail-groups.ts` holds one arrangement per root at module scope: two copies is how the rail and the map come to disagree after an edit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91 --- .../web/src/components/SystemGraphCanvas.tsx | 43 +- .../web/src/components/WorkspaceGraphView.tsx | 41 ++ .../web/src/lib/system-graph-groups.test.ts | 220 +++++++ .../web/src/lib/system-graph-groups.ts | 96 +++ .../web/src/lib/system-graph-layout.test.ts | 230 +++++++ .../web/src/lib/system-graph-layout.ts | 559 ++++++++++++++++-- .../harness/web/src/lib/use-rail-groups.ts | 159 +++-- packages/harness/web/src/styles.css | 37 ++ 8 files changed, 1275 insertions(+), 110 deletions(-) create mode 100644 packages/harness/web/src/lib/system-graph-groups.test.ts create mode 100644 packages/harness/web/src/lib/system-graph-groups.ts diff --git a/packages/harness/web/src/components/SystemGraphCanvas.tsx b/packages/harness/web/src/components/SystemGraphCanvas.tsx index 96b14c1a3..841ba730a 100644 --- a/packages/harness/web/src/components/SystemGraphCanvas.tsx +++ b/packages/harness/web/src/components/SystemGraphCanvas.tsx @@ -19,6 +19,7 @@ import { layoutSystemGraph, systemGraphNodeById, type SystemGraphLayoutNode, + type SystemGraphNodeGroup, } from "../lib/system-graph-layout"; import { SYSTEM_GRAPH_DEFAULT_MIN_ZOOM, @@ -46,6 +47,12 @@ interface SystemGraphCanvasProps { graph: SystemGraph; workspaceKey: WorkspaceKey; navigableAgentKeys: ReadonlySet; + /** + * The containers to draw, from the rail's Group axis. `undefined` while the + * project's stored arrangement is still in flight — NOT an empty list, which + * would be a real answer ("nothing is grouped") and would flash a wrong one. + */ + groups: readonly SystemGraphNodeGroup[] | undefined; onOpenAgent: (agentKey: AgentKey) => void; } @@ -66,15 +73,16 @@ export function SystemGraphCanvas({ graph, workspaceKey, navigableAgentKeys, + groups, onOpenAgent, }: SystemGraphCanvasProps): JSX.Element { const computed = useMemo(() => { try { - return { layout: layoutSystemGraph(graph), failed: false } as const; + return { layout: layoutSystemGraph(graph, groups), failed: false } as const; } catch { return { layout: null, failed: true } as const; } - }, [graph]); + }, [graph, groups]); const layout = computed.layout; const graphNodes = useMemo(() => systemGraphNodeById(graph), [graph]); const [view, setView] = useState( @@ -314,6 +322,32 @@ export function SystemGraphCanvas({ role="group" aria-label="Workspace dependency graph" > + {/* Behind the connectors and the cards, so an edge that leaves its + system reads as crossing the boundary rather than being clipped by + it — the shape the design reference draws. */} + {layout.groups.map((group) => ( +
+ + {group.label} + +
+ ))} + diff --git a/packages/harness/web/src/components/WorkspaceGraphView.tsx b/packages/harness/web/src/components/WorkspaceGraphView.tsx index 553ba1d6a..41813832e 100644 --- a/packages/harness/web/src/components/WorkspaceGraphView.tsx +++ b/packages/harness/web/src/components/WorkspaceGraphView.tsx @@ -11,7 +11,9 @@ import type { BusMessage, WorkflowInfo } from "@shared/types"; import type { HarnessApi } from "../lib/api"; import { systemGraphLoader } from "../lib/system-graph-loader"; +import { systemGraphNodeGroups } from "../lib/system-graph-groups"; import { mapSystemGraphNavigation } from "../lib/system-graph-navigation"; +import { useRailGroups } from "../lib/use-rail-groups"; import { trackingAttrs } from "../lib/analytics/tracking-attrs"; import { EmptyState } from "./EmptyState"; import { Icon } from "./Icon"; @@ -139,6 +141,44 @@ export function WorkspaceGraphView({ [graph, workspaceKey, workflows, workspaceScopes], ); + /* THE MAP READS THE RAIL'S GROUPS (SAP-2983). + The Group axis is stored per project ROOT, and a workspace scope is the one + thing that joins this opaque key back to one — the graph payload carries no + filesystem path on purpose. Sorted by name because a container's order on + the map is its own (`shelfPack` keeps the rail's group order); this only + settles the order of agents inside one, which the layout re-decides from + the topology anyway. */ + const projectRoot = useMemo( + () => + workspaceScopes.find((scope) => scope.workspaceKey === workspaceKey) + ?.cwd ?? null, + [workspaceKey, workspaceScopes], + ); + const railRoots = useMemo( + () => (projectRoot === null ? [] : [projectRoot]), + [projectRoot], + ); + const railGroups = useRailGroups( + railRoots, + workflows, + "name", + projectRoot !== null, + ); + const groups = useMemo(() => { + // `isReady` is BOTH halves — the stored arrangement and the launch edges. + // Drawing before either lands would put every agent in one `Ungrouped` + // container for a beat, and that is a real arrangement, not a placeholder: + // it would read as this project's answer and then silently rearrange. + if (!graph || projectRoot === null || !railGroups.isReady(projectRoot)) { + return undefined; + } + return systemGraphNodeGroups( + graph.nodes, + railGroups.groupsFor(projectRoot, railGroups.agentsIn(projectRoot)), + navigation, + ); + }, [graph, navigation, projectRoot, railGroups]); + return ( /* The MAP altitude of the right pane (`lib/canvas-altitude.ts`) — a project's agents and the edges between them, drawn beside the @@ -268,6 +308,7 @@ export function WorkspaceGraphView({ graph={graph} workspaceKey={workspaceKey} navigableAgentKeys={new Set(navigation.keys())} + groups={groups} onOpenAgent={(agentKey) => { const workflow = navigation.get(agentKey); if (workflow) onOpenAgent(workflow.path); diff --git a/packages/harness/web/src/lib/system-graph-groups.test.ts b/packages/harness/web/src/lib/system-graph-groups.test.ts new file mode 100644 index 000000000..5827aa088 --- /dev/null +++ b/packages/harness/web/src/lib/system-graph-groups.test.ts @@ -0,0 +1,220 @@ +import { describe, expect, it } from "vitest"; +import type { AgentKey, SystemGraphNode } from "@shared/system-graph"; +import type { WorkflowInfo } from "@shared/types"; + +import { + EMPTY_RAIL_STATE, + deriveOrStored, + materialize, + parseRailState, + type LaunchEdge, + type RailState, +} from "./agent-groups"; +import { systemGraphNodeGroups } from "./system-graph-groups"; + +const ROOT = "/repo"; + +const agent = (name: string): WorkflowInfo => ({ + name, + path: `${ROOT}/${name}`, + definitionId: null, + definitionSlug: name, + activeBuildRunId: null, + activeBuildRunStatus: null, + source: "scan", +}); + +const node = (name: string): SystemGraphNode => ({ + id: `agent:${name}`, + agentKey: name, + label: name, +}); + +/** The join `WorkspaceGraphView` hands over: agent key to registry row. */ +const navigationFor = ( + workflows: readonly WorkflowInfo[], +): ReadonlyMap => + new Map(workflows.map((workflow) => [workflow.name, workflow])); + +/** gateway launches queue and worker; mailer launches sender; loner nothing. */ +const WORKFLOWS = [ + agent("gateway"), + agent("queue"), + agent("worker"), + agent("mailer"), + agent("sender"), + agent("loner"), +]; +const NODES = WORKFLOWS.map((workflow) => node(workflow.name)); +const EDGES: LaunchEdge[] = [ + { parent: "gateway", child: "queue" }, + { parent: "gateway", child: "worker" }, + { parent: "mailer", child: "sender" }, +]; + +/** What the RAIL renders for a state — the map is handed exactly this. */ +const railRows = (state: RailState) => + deriveOrStored(WORKFLOWS, state, EDGES, "name"); + +const containers = (state: RailState) => + systemGraphNodeGroups(NODES, railRows(state), navigationFor(WORKFLOWS)); + +const shape = (state: RailState) => + containers(state).map((container) => [container.label, container.nodeIds]); + +describe("systemGraphNodeGroups", () => { + it("draws one container per rail row, in the rail's order, labelled identically", () => { + // The whole ticket in one assertion: the sub-structure the rail shows is + // the sub-structure the map draws. Two names for one group is the failure. + expect(shape(EMPTY_RAIL_STATE)).toEqual([ + ["gateway", ["agent:gateway", "agent:queue", "agent:worker"]], + ["mailer", ["agent:mailer", "agent:sender"]], + ["Ungrouped", ["agent:loner"]], + ]); + expect(containers(EMPTY_RAIL_STATE).map((c) => c.label)).toEqual( + railRows(EMPTY_RAIL_STATE).map((row) => row.label), + ); + }); + + it("keeps `groups: null` and `groups: []` different answers", () => { + /* THE REGRESSION THIS PROJECT KEEPS HAVING. `null` is "nothing stored, + detection owns this"; `[]` is "the user materialized groups and then + deleted every one". Collapsing them dumped every agent into Ungrouped, + permanently, in a reference prototype — and the map is a NEW read path + for the same file, so the distinction has to survive this module too. + + Fails if the map ever reaches past `deriveOrStored` for its own opinion + of the edges: an implementation that re-derived from launch edges would + return the detected containers for BOTH states. */ + const nothingStored = parseRailState( + JSON.stringify({ version: 1, groups: null, renames: {} }), + ); + const allDeleted = parseRailState( + JSON.stringify({ version: 1, groups: [], renames: {} }), + ); + expect(nothingStored.groups).toBeNull(); + expect(allDeleted.groups).toEqual([]); + + expect(shape(nothingStored)).toEqual(shape(EMPTY_RAIL_STATE)); + expect(shape(allDeleted)).toEqual([ + [ + "Ungrouped", + // Name order: the rail sorts an Ungrouped bucket, and the map carries + // its rows through untouched. + [ + "agent:gateway", + "agent:loner", + "agent:mailer", + "agent:queue", + "agent:sender", + "agent:worker", + ], + ], + ]); + expect(shape(allDeleted)).not.toEqual(shape(nothingStored)); + }); + + it("follows the user's edited groups rather than re-detecting", () => { + // Derived until touched: once materialized and renamed, the map must say + // what the rail says, not what a fresh scan would. + const edited = materialize(EMPTY_RAIL_STATE, WORKFLOWS, EDGES, "name"); + const renamed: RailState = { + ...edited, + groups: edited.groups.map((group) => + group.label === "gateway" ? { ...group, label: "Ingest" } : group, + ), + }; + expect(shape(renamed)).toEqual([ + ["Ingest", ["agent:gateway", "agent:queue", "agent:worker"]], + ["mailer", ["agent:mailer", "agent:sender"]], + ["Ungrouped", ["agent:loner"]], + ]); + }); + + it("draws a shared agent once, under the first group that names it", () => { + // Group membership is many-to-many by design — a shared subagent belongs to + // every system that calls it — and the rail prints it in each. A map has + // one card per agent, so a second mention must not draw a second card in a + // second container. + const shared: RailState = { + version: 1, + renames: {}, + groups: [ + { id: "g_one", label: "One", members: [`${ROOT}/gateway`, `${ROOT}/queue`] }, + { id: "g_two", label: "Two", members: [`${ROOT}/queue`, `${ROOT}/worker`] }, + ], + }; + const drawn = containers(shared); + expect(drawn.map((c) => [c.label, c.nodeIds])).toEqual([ + ["One", ["agent:gateway", "agent:queue"]], + ["Two", ["agent:worker"]], + ["Ungrouped", ["agent:loner", "agent:mailer", "agent:sender"]], + ]); + expect(drawn.flatMap((c) => c.nodeIds)).toHaveLength(NODES.length); + }); + + it("drops a container whose members this graph has none of", () => { + // Chrome around nothing: a group naming only agents the projection did not + // produce would draw an empty labelled box. + const stale: RailState = { + version: 1, + renames: {}, + groups: [{ id: "g_gone", label: "Gone", members: [`${ROOT}/deleted`] }], + }; + expect(containers(stale).map((c) => c.label)).toEqual(["Ungrouped"]); + }); + + it("files a node no row resolved into Ungrouped rather than losing it", () => { + /* Two agents sharing a `definitionSlug` are dropped from the navigation + join, because neither can be told from the other — which a real install + has. Their CARDS still exist, and a card outside every container is a + card the layout has nowhere to put. */ + const ambiguous = navigationFor( + WORKFLOWS.filter((workflow) => workflow.name !== "sender"), + ); + const drawn = systemGraphNodeGroups( + NODES, + railRows(EMPTY_RAIL_STATE), + ambiguous, + ); + expect(drawn.map((c) => [c.label, c.nodeIds])).toEqual([ + ["gateway", ["agent:gateway", "agent:queue", "agent:worker"]], + ["mailer", ["agent:mailer"]], + ["Ungrouped", ["agent:loner", "agent:sender"]], + ]); + expect(drawn.flatMap((c) => c.nodeIds).sort()).toEqual( + NODES.map((n) => n.id).sort(), + ); + }); + + it("covers every node exactly once for every arrangement", () => { + // The layout is handed this as a partition. A node claimed twice draws two + // cards; a node claimed by nobody vanishes from the map. + for (const state of [ + EMPTY_RAIL_STATE, + materialize(EMPTY_RAIL_STATE, WORKFLOWS, EDGES, "name"), + { version: 1 as const, groups: [], renames: {} }, + ]) { + const claimed = containers(state).flatMap((c) => c.nodeIds); + expect([...claimed].sort()).toEqual(NODES.map((n) => n.id).sort()); + } + }); + + it("renders a single-group project as that group, not as an extra frame", () => { + const onlyOne: RailState = { + version: 1, + renames: {}, + groups: [ + { + id: "g_all", + label: "Everything", + members: WORKFLOWS.map((workflow) => workflow.path), + }, + ], + }; + const drawn = containers(onlyOne); + expect(drawn).toHaveLength(1); + expect(drawn[0]!.label).toBe("Everything"); + expect(drawn[0]!.nodeIds).toHaveLength(NODES.length); + }); +}); diff --git a/packages/harness/web/src/lib/system-graph-groups.ts b/packages/harness/web/src/lib/system-graph-groups.ts new file mode 100644 index 000000000..550f3fc69 --- /dev/null +++ b/packages/harness/web/src/lib/system-graph-groups.ts @@ -0,0 +1,96 @@ +import type { AgentKey, SystemGraphNode } from "@shared/system-graph"; +import type { WorkflowInfo } from "@shared/types"; + +import { UNGROUPED_ID, type GroupNode } from "./agent-groups"; +import { + SYSTEM_GRAPH_UNGROUPED_LABEL, + type SystemGraphNodeGroup, +} from "./system-graph-layout"; + +/** + * The join between the rail's GROUP axis and the project map. + * + * The map used to draw every agent a project contains as one flat set, ignoring + * the sub-structure the rail was showing six inches to its left: one root + * holding nine systems and 76 agents came out as a single ~70-node column. The + * mechanism to fix that already existed — `lib/agent-groups.ts` derives groups + * from launch edges, lets the user edit them, and persists the arrangement to a + * committable `.sapiom/studio-rail.json`. The map simply never read it. + * + * So this module invents nothing. It takes the rows the rail renders and + * answers one question: which graph node is which row's. Everything about what + * a group IS — derived until touched, `groups: null` is not `groups: []`, + * membership is many-to-many — stays in `agent-groups.ts`, unmodified, and + * reaches the map only through the `GroupNode[]` it is handed. + */ + +/** + * Containers for one graph, in the rail's own order, covering every node. + * + * `navigation` is the SAME map the drill-in uses (`system-graph-navigation.ts`): + * public graph nodes carry no filesystem path, so an agent key is joined to a + * registry row only where exactly one row claims it. Reusing it rather than + * writing a second join keeps one invariant true — a node you can open is a + * node whose group is known — and puts the ambiguous ones (two agents sharing a + * `definitionSlug`, which a real install has) in `Ungrouped` rather than in a + * guess. + */ +export function systemGraphNodeGroups( + nodes: readonly SystemGraphNode[], + groups: readonly GroupNode[], + navigation: ReadonlyMap, +): SystemGraphNodeGroup[] { + const nodeIdByAgentKey = new Map(nodes.map((node) => [node.agentKey, node.id])); + const nodeIdByPath = new Map(); + for (const [agentKey, workflow] of navigation) { + const nodeId = nodeIdByAgentKey.get(agentKey); + if (nodeId !== undefined) nodeIdByPath.set(workflow.path, nodeId); + } + + const claimed = new Set(); + const containers: SystemGraphNodeGroup[] = []; + for (const group of groups) { + const nodeIds: string[] = []; + for (const agent of group.agents) { + const nodeId = nodeIdByPath.get(agent.workflow.path); + // Claimed already: a shared subagent is a member of every system that + // calls it, and the rail prints it once per group. The map has one card + // for it, filed under the first group that names it. + if (nodeId === undefined || claimed.has(nodeId)) continue; + claimed.add(nodeId); + nodeIds.push(nodeId); + } + // A group whose members are all agents this graph does not have would draw + // an empty box with a name on it — chrome around nothing. + if (nodeIds.length > 0) { + containers.push({ id: group.id, label: group.label, nodeIds }); + } + } + + // Nodes no row claimed. Registry rows and graph nodes are two projections of + // one directory and they can disagree — an agent registered a moment ago, one + // whose key two rows both claim. Those are still on the map, and a bucket + // named for what it means beats a card floating outside every container. + const rest = nodes + .map((node) => node.id) + .filter((nodeId) => !claimed.has(nodeId)); + if (rest.length > 0) { + const index = containers.findIndex( + (container) => container.label === SYSTEM_GRAPH_UNGROUPED_LABEL, + ); + if (index === -1) { + containers.push({ + id: UNGROUPED_ID, + label: SYSTEM_GRAPH_UNGROUPED_LABEL, + nodeIds: rest, + }); + } else { + // Re-appended rather than edited in place: Ungrouped is last in the rail + // and stays last here, so the two read in the same order. + const bucket = containers[index]!; + containers.splice(index, 1); + containers.push({ ...bucket, nodeIds: [...bucket.nodeIds, ...rest] }); + } + } + return containers; +} diff --git a/packages/harness/web/src/lib/system-graph-layout.test.ts b/packages/harness/web/src/lib/system-graph-layout.test.ts index 3cb92984c..cb3fed61b 100644 --- a/packages/harness/web/src/lib/system-graph-layout.test.ts +++ b/packages/harness/web/src/lib/system-graph-layout.test.ts @@ -10,6 +10,7 @@ import { SYSTEM_GRAPH_NODE_WIDTH, layoutSystemGraph, type SystemGraphLayout, + type SystemGraphNodeGroup, } from "./system-graph-layout"; const node = (id: string) => ({ id, agentKey: id, label: id.toUpperCase() }); @@ -282,3 +283,232 @@ describe("layoutSystemGraph", () => { ).toThrow("Invalid system graph layout input"); }); }); + +/** + * SAP-2983 — the map draws the groups the rail already has. + * + * The defect was structural, not cosmetic: one project root holding nine + * systems and 76 agents rendered as a single ~70-node column, because every + * unconnected agent was its own weak component and components were STACKED. + * These pin the two halves of the fix — containers, and packing that wraps — + * in geometry, because "it looks better" is not a rule anything can hold. + * + * Geometry only. That containers carry the RAIL's labels is + * `system-graph-groups.test.ts`; that they reach the DOM is `project-map.spec.ts`. + */ +describe("layoutSystemGraph with groups", () => { + const group = ( + id: string, + label: string, + nodeIds: string[], + ): SystemGraphNodeGroup => ({ id, label, nodeIds }); + + /** The box a container claims, by label. */ + function boxOf(layout: SystemGraphLayout, label: string) { + const found = layout.groups.find((candidate) => candidate.label === label); + if (!found) throw new Error(`Missing container ${label}`); + return found; + } + + const contains = ( + outer: { x: number; y: number; width: number; height: number }, + inner: { x: number; y: number; width: number; height: number }, + ): boolean => + inner.x >= outer.x && + inner.y >= outer.y && + inner.x + inner.width <= outer.x + outer.width && + inner.y + inner.height <= outer.y + outer.height; + + it("draws no container at all when it was given no groups", () => { + // `undefined` is "no grouping information", which is NOT "nothing is + // grouped". Drawing a bucket for it would put a label on the whole map + // while the project's arrangement was still loading. + const layout = layoutSystemGraph(graph(["a", "b"], [edge("a", "b")])); + expect(layout.groups).toEqual([]); + expect(layout.nodes.every((node) => node.groupId === null)).toBe(true); + }); + + it("puts every card inside its own container and no card inside another", () => { + const layout = layoutSystemGraph( + graph( + ["a", "b", "x", "y", "loner"], + [edge("a", "b"), edge("x", "y", "async")], + ), + [ + group("g:one", "One", ["a", "b"]), + group("g:two", "Two", ["x", "y"]), + group("group:ungrouped", "Ungrouped", ["loner"]), + ], + ); + + expect(layout.groups.map((candidate) => candidate.label)).toEqual([ + "One", + "Two", + "Ungrouped", + ]); + for (const [label, ids] of [ + ["One", ["a", "b"]], + ["Two", ["x", "y"]], + ["Ungrouped", ["loner"]], + ] as const) { + const box = boxOf(layout, label); + expect(box.nodeCount).toBe(ids.length); + for (const id of ids) { + expect(contains(box, byId(layout, id)), `${id} inside ${label}`).toBe( + true, + ); + expect(byId(layout, id).groupId).toBe(box.id); + } + } + // The containers themselves must not overlap, or "inside" means nothing. + for (let left = 0; left < layout.groups.length; left += 1) { + for (let right = left + 1; right < layout.groups.length; right += 1) { + expect( + rectanglesOverlap(layout.groups[left]!, layout.groups[right]!), + `${layout.groups[left]!.label} overlaps ${layout.groups[right]!.label}`, + ).toBe(false); + } + } + expectNodesNotToOverlap(layout); + }); + + it("keeps a group's own wiring and labels inside its border", () => { + /* A container measured from its CARDS is not big enough. A cycle gutter + runs 44px past the right edge of its component, a rank-skipping corridor + 28px above the top of one, and — the case that actually escapes any fixed + padding — a connector label that finds no free slot beside the cards is + pushed into a fallback stack that grows without bound. Measured: a + six-way fan-in already puts two labels outside cards + 48px. + + So the box is the union of everything the group DRAWS, computed after + routing. Anything less and an edge appears to leave a system it never + leaves. */ + const sources = Array.from({ length: 8 }, (_, index) => `s${index}`); + const layout = layoutSystemGraph( + graph( + ["hub", ...sources, "solo"], + sources.map((id) => edge(id, "hub")), + ), + [ + group("g:fan", "Fan", ["hub", ...sources]), + group("group:ungrouped", "Ungrouped", ["solo"]), + ], + ); + const box = boxOf(layout, "Fan"); + const cards = layout.nodes.filter((placed) => placed.id !== "solo"); + // The fixture is only evidence while it still overflows a card-sized box. + const cardsBox = { + x: Math.min(...cards.map((placed) => placed.x)), + y: Math.min(...cards.map((placed) => placed.y)), + width: 0, + height: 0, + }; + cardsBox.width = + Math.max(...cards.map((placed) => placed.x + placed.width)) - cardsBox.x; + cardsBox.height = + Math.max(...cards.map((placed) => placed.y + placed.height)) - cardsBox.y; + expect( + layout.edges.some((routed) => !contains(cardsBox, routed.labelBounds)), + ).toBe(true); + + for (const routed of layout.edges) { + for (const point of routed.points) { + expect(point.x).toBeGreaterThanOrEqual(box.x); + expect(point.x).toBeLessThanOrEqual(box.x + box.width); + expect(point.y).toBeGreaterThanOrEqual(box.y); + expect(point.y).toBeLessThanOrEqual(box.y + box.height); + } + expect(contains(box, routed.labelBounds)).toBe(true); + } + expectEdgesNotToCrossCards(layout); + }); + + it("keeps a cyclic group's gutters inside its border too", () => { + const layout = layoutSystemGraph( + graph( + ["a", "b", "c", "solo"], + [edge("a", "b"), edge("b", "c"), edge("c", "a", "async"), edge("a", "c")], + ), + [ + group("g:cyclic", "Cyclic", ["a", "b", "c"]), + group("group:ungrouped", "Ungrouped", ["solo"]), + ], + ); + const box = boxOf(layout, "Cyclic"); + for (const routed of layout.edges) { + for (const point of routed.points) { + expect(point.x).toBeGreaterThanOrEqual(box.x); + expect(point.x).toBeLessThanOrEqual(box.x + box.width); + expect(point.y).toBeGreaterThanOrEqual(box.y); + expect(point.y).toBeLessThanOrEqual(box.y + box.height); + } + expect(contains(box, routed.labelBounds)).toBe(true); + } + expectEdgesNotToCrossCards(layout); + }); + + it("draws an edge whose ends the user split across two groups", () => { + // A group is editable, so half a detected system can be pulled out. The + // edge between the halves is still real; dropping it would make the map + // claim two systems never touch. + const layout = layoutSystemGraph( + graph(["a", "b"], [edge("a", "b")]), + [group("g:one", "One", ["a"]), group("g:two", "Two", ["b"])], + ); + expect(layout.edges).toHaveLength(1); + expect(layout.edges[0]).toMatchObject({ + from: "a", + to: "b", + crossesGroup: true, + }); + expect(layout.edges[0]!.path).not.toContain("NaN"); + }); + + it("wraps a container of unconnected agents instead of stacking them", () => { + /* THE DEFECT, in numbers. 40 agents with no edges used to be 40 stacked + components: 40 * (64 + 64) = 5,120px tall and one card wide. The + assertion is on the SHAPE — taller than it is wide is the column coming + back — and on distinct rows and columns, which a stack has exactly one + of. */ + const ids = Array.from({ length: 40 }, (_, index) => `n${index}`); + const layout = layoutSystemGraph(graph(ids, []), [ + group("group:ungrouped", "Ungrouped", ids), + ]); + + expect(layout.groups).toHaveLength(1); + expect(layout.bounds.height).toBeLessThan(5120 / 2); + expect(layout.bounds.width).toBeGreaterThan(layout.bounds.height); + expect(new Set(layout.nodes.map((node) => node.x)).size).toBeGreaterThan(1); + expect(new Set(layout.nodes.map((node) => node.y)).size).toBeGreaterThan(1); + expectNodesNotToOverlap(layout); + expect(boxOf(layout, "Ungrouped").nodeCount).toBe(40); + }); + + it("still draws a node no group claimed", () => { + // The caller hands over an exhaustive partition, so this is a backstop — + // and it is deliberately not a throw. A card that silently disappears is + // worse than a card in the bucket that means "nothing claims this". + const layout = layoutSystemGraph(graph(["a", "orphan"], []), [ + group("g:one", "One", ["a"]), + ]); + expect(layout.nodes.map((node) => node.id).sort()).toEqual(["a", "orphan"]); + const box = boxOf(layout, "Ungrouped"); + expect(contains(box, byId(layout, "orphan"))).toBe(true); + }); + + it("is deterministic for grouped input too", () => { + const groups = [ + group("g:one", "One", ["a", "b"]), + group("group:ungrouped", "Ungrouped", ["z"]), + ]; + const forward = layoutSystemGraph( + graph(["a", "b", "z"], [edge("a", "b")]), + groups, + ); + const reversed = layoutSystemGraph( + graph(["z", "b", "a"], [edge("a", "b")]), + groups, + ); + expect(reversed).toEqual(forward); + }); +}); diff --git a/packages/harness/web/src/lib/system-graph-layout.ts b/packages/harness/web/src/lib/system-graph-layout.ts index f3e3f92af..3ccc6b5fa 100644 --- a/packages/harness/web/src/lib/system-graph-layout.ts +++ b/packages/harness/web/src/lib/system-graph-layout.ts @@ -20,11 +20,73 @@ const PORT_LIMIT = 24; const PORT_STEP = 8; const LABEL_HEIGHT = 16; +/** + * Inside a container, around everything it holds. + * + * Not a taste number: a cycle gutter reaches `8 + 9 * 4 = 44px` past the right + * edge of its component and a rank-skipping corridor reaches `12 + 4 * 4 = 28px` + * above the top of one. Anything smaller and a group's own wiring would be + * drawn crossing the border drawn around it, which reads as an edge leaving the + * system when it never did. + */ +const GROUP_PADDING = 48; +/** The label strip along the top of a container, above its content. */ +const GROUP_HEADER = 26; +/** Between containers. Wider than `COMPONENT_GAP` so the boundary between two + * systems reads as a bigger break than the boundary between two components of + * one system. */ +const GROUP_GAP = 80; +/** + * Target width:height for a packed region. + * + * The defect this whole file's packing exists to fix is a project of 76 agents + * with 8 edges rendering as a single ~70-node column roughly 8,700px tall. + * Shelf packing needs a width to wrap at, and the pane it lands in is wide, so + * aim landscape rather than square. + */ +const SHELF_ASPECT = 2.2; + +/** + * The label the Group axis gives agents no group claims (`agent-groups.ts`). + * + * Repeated here rather than imported because the dependency runs the other way: + * `system-graph-groups.ts` maps the rail's model onto this one. The e2e spec + * asserts the map's container labels against the RAIL's rows, so the two + * spellings cannot drift apart unnoticed. + */ +export const SYSTEM_GRAPH_UNGROUPED_LABEL = "Ungrouped"; + export interface SystemGraphPoint { x: number; y: number; } +/** + * One container to draw: a named set of node ids. + * + * The map does not decide these. They come from the Group axis the rail already + * renders (`lib/agent-groups.ts`, mapped by `lib/system-graph-groups.ts`) — two + * views of one arrangement, which is the whole point of SAP-2983. A second + * opinion about which agents belong together would be a second answer to a + * question the user has already answered. + */ +export interface SystemGraphNodeGroup { + id: string; + label: string; + nodeIds: readonly string[]; +} + +/** A drawn container: the box, and the label that names it. */ +export interface SystemGraphLayoutGroup { + id: string; + label: string; + x: number; + y: number; + width: number; + height: number; + nodeCount: number; +} + export interface SystemGraphLayoutNode { id: string; x: number; @@ -32,6 +94,9 @@ export interface SystemGraphLayoutNode { width: number; height: number; componentId: string; + /** The container this card sits inside, or null when the graph was laid out + * without groups. */ + groupId: string | null; } export interface SystemGraphLabelBounds { @@ -52,11 +117,18 @@ export interface SystemGraphLayoutEdge { labelY: number; labelBounds: SystemGraphLabelBounds; route: "forward" | "cycle"; + /** True when the two ends sit in different containers. Drawn differently, + * because "these two systems touch" is a different claim from "this system + * is wired like this". */ + crossesGroup: boolean; } export interface SystemGraphLayout { nodes: SystemGraphLayoutNode[]; edges: SystemGraphLayoutEdge[]; + /** Empty when laid out without groups — the map draws no chrome it was not + * given a reason to draw. */ + groups: SystemGraphLayoutGroup[]; bounds: { width: number; height: number }; } @@ -96,6 +168,7 @@ interface EdgeSeed { targetOffset: number; cycleLane: number; forwardLane: number; + crossesGroup: boolean; } interface RoutedEdge extends EdgeSeed { @@ -330,19 +403,73 @@ export function analyzeSystemGraph(graph: SystemGraph): SystemGraphTopology { return { components }; } -function placeNodes(topology: SystemGraphTopology): { +interface Sized { + width: number; + height: number; +} + +/** + * Left-to-right shelves, wrapping at a width derived from the total area. + * + * Boxes keep their given ORDER — for containers that order is the rail's, and a + * map that reshuffles the rail's rows is a map you have to re-read. Wrapping is + * what stops a list of boxes becoming a column: stacking 68 single-agent + * components produced a subject 8,700px tall, which no amount of fitting makes + * legible. + */ +function shelfPack( + sizes: readonly Sized[], + gap: number, +): { offsets: SystemGraphPoint[]; width: number; height: number } { + if (sizes.length === 0) return { offsets: [], width: 0, height: 0 }; + const widest = Math.max(...sizes.map((size) => size.width)); + // Each box is counted WITH its gutter, so the estimate holds for the many + // small boxes case — which is the shape that produced the column. + const area = sizes.reduce( + (total, size) => total + (size.width + gap) * (size.height + gap), + 0, + ); + const target = Math.max(widest, Math.sqrt(area * SHELF_ASPECT)); + const offsets: SystemGraphPoint[] = []; + let shelfTop = 0; + let shelfHeight = 0; + let cursorX = 0; + let width = 0; + for (const size of sizes) { + if (cursorX > 0 && cursorX + size.width > target) { + shelfTop += shelfHeight + gap; + shelfHeight = 0; + cursorX = 0; + } + offsets.push({ x: cursorX, y: shelfTop }); + cursorX += size.width + gap; + width = Math.max(width, cursorX - gap); + shelfHeight = Math.max(shelfHeight, size.height); + } + return { offsets, width, height: shelfTop + shelfHeight }; +} + +/** + * Every component of one region, ranked internally and then shelf-packed. + * + * Components are laid out at their own origin first because packing needs each + * box's size up front — the vertical stack this replaces never had to know one. + */ +function placeRegionNodes( + topology: SystemGraphTopology, + groupId: string | null, +): { nodes: SystemGraphLayoutNode[]; componentBoxes: Map; componentByNode: Map; strongByNode: Map; + width: number; + height: number; } { - const nodes: SystemGraphLayoutNode[] = []; - const componentBoxes = new Map(); const componentByNode = new Map(); const strongByNode = new Map(); - let yCursor = 0; - for (const component of topology.components) { + const laid = topology.components.map((component) => { const byRank = new Map(); for (const strong of component.stronglyConnected) { const rankNodes = byRank.get(strong.rank) ?? []; @@ -362,41 +489,56 @@ function placeNodes(topology: SystemGraphTopology): { Math.max(0, count - 1) * SYSTEM_GRAPH_SLOT_GAP ); }; - const componentHeight = Math.max( + const height = Math.max( SYSTEM_GRAPH_NODE_HEIGHT, ...ranks.map(rankHeight), ); + const local: SystemGraphLayoutNode[] = []; for (const rank of ranks) { const rankNodes = byRank.get(rank)!; - const startY = yCursor + (componentHeight - rankHeight(rank)) / 2; + const startY = (height - rankHeight(rank)) / 2; rankNodes.forEach((id, row) => { - nodes.push({ + local.push({ id, x: rank * (SYSTEM_GRAPH_NODE_WIDTH + SYSTEM_GRAPH_RANK_GAP), y: startY + row * (SYSTEM_GRAPH_NODE_HEIGHT + SYSTEM_GRAPH_SLOT_GAP), width: SYSTEM_GRAPH_NODE_WIDTH, height: SYSTEM_GRAPH_NODE_HEIGHT, componentId: component.id, + groupId, }); }); } - const componentNodes = nodes.filter( - (candidate) => candidate.componentId === component.id, - ); - componentBoxes.set(component.id, { - minX: Math.min(...componentNodes.map((candidate) => candidate.x)), - minY: Math.min(...componentNodes.map((candidate) => candidate.y)), - maxX: Math.max( - ...componentNodes.map((candidate) => candidate.x + candidate.width), - ), - maxY: Math.max( - ...componentNodes.map((candidate) => candidate.y + candidate.height), - ), + const width = Math.max(...local.map((node) => node.x + node.width)); + return { component, local, width, height }; + }); + + const packed = shelfPack(laid, COMPONENT_GAP); + const nodes: SystemGraphLayoutNode[] = []; + const componentBoxes = new Map(); + laid.forEach((entry, index) => { + const at = packed.offsets[index]!; + for (const node of entry.local) { + nodes.push({ ...node, x: node.x + at.x, y: node.y + at.y }); + } + // The ranked block spans its full box: the tallest rank starts at the top + // and reaches the bottom, and rank 0 starts at the left. + componentBoxes.set(entry.component.id, { + minX: at.x, + minY: at.y, + maxX: at.x + entry.width, + maxY: at.y + entry.height, }); - yCursor += componentHeight + COMPONENT_GAP; - } + }); nodes.sort((left, right) => compareIds(left.id, right.id)); - return { nodes, componentBoxes, componentByNode, strongByNode }; + return { + nodes, + componentBoxes, + componentByNode, + strongByNode, + width: packed.width, + height: packed.height, + }; } function spreadPortOffsets( @@ -531,14 +673,15 @@ function chooseLabelBounds( } function routeEdges( - graph: SystemGraph, - nodes: SystemGraphLayoutNode[], + visible: readonly VisibleSystemGraphEdge[], + nodes: readonly SystemGraphLayoutNode[], componentBoxes: ReadonlyMap, componentByNode: ReadonlyMap, strongByNode: ReadonlyMap, + labels: SystemGraphLabelBounds[], ): RoutedEdge[] { const nodeById = new Map(nodes.map((node) => [node.id, node])); - const seeds: EdgeSeed[] = groupSystemGraphEdges(graph.edges).map((edge) => { + const seeds: EdgeSeed[] = visible.map((edge) => { const componentId = componentByNode.get(edge.from)!; return { edge, @@ -551,6 +694,7 @@ function routeEdges( targetOffset: 0, cycleLane: 0, forwardLane: 0, + crossesGroup: false, }; }); spreadPortOffsets(seeds, nodeById, "source"); @@ -598,7 +742,6 @@ function routeEdges( }); } - const labels: SystemGraphLabelBounds[] = []; return seeds.map((seed): RoutedEdge => { const source = nodeById.get(seed.edge.from)!; const target = nodeById.get(seed.edge.to)!; @@ -689,6 +832,97 @@ function routeEdges( }); } +/** How far outside a card a cross-container connector runs before it turns. */ +const CROSS_GROUP_GUTTER = 16; +const CROSS_GROUP_LANE = 6; + +/** + * Connectors whose two ends sit in DIFFERENT containers. + * + * They exist because a group is editable: pull half a detected system into a + * group of its own and the edge between the halves is still real. Dropping it + * would make the map claim two systems never touch, which is the one thing an + * edge is for. + * + * Routed after the containers are packed, in global coordinates, and + * deliberately NOT confined to a gutter: a corridor wide enough to skirt every + * container between two ends would dominate the drawing for the rarest edge on + * it. They pass BEHIND cards (the edge layer sits under the node layer) and the + * design reference draws them the same way — the connector out of "THE LOOP" to + * TikTok crosses its border. + */ +function routeCrossGroupEdges( + visible: readonly VisibleSystemGraphEdge[], + nodes: readonly SystemGraphLayoutNode[], + labels: SystemGraphLabelBounds[], +): RoutedEdge[] { + const nodeById = new Map(nodes.map((node) => [node.id, node])); + return visible.map((edge, index): RoutedEdge => { + const source = nodeById.get(edge.from)!; + const target = nodeById.get(edge.to)!; + const lane = index % 6; + const start = { + x: source.x + source.width, + y: source.y + source.height / 2, + }; + const end = { x: target.x - 1, y: target.y + target.height / 2 }; + let points: SystemGraphPoint[]; + if (end.x - start.x > SYSTEM_GRAPH_RANK_GAP) { + const elbowX = start.x + (end.x - start.x) * 0.5 + lane * CROSS_GROUP_LANE; + points = [ + start, + { x: elbowX, y: start.y }, + { x: elbowX, y: end.y }, + end, + ]; + } else { + // The target is level with or behind the source: leave to the right, run + // above both cards, and come back down into the target's left edge. + const outX = start.x + CROSS_GROUP_GUTTER + lane * CROSS_GROUP_LANE; + const inX = end.x - CROSS_GROUP_GUTTER - lane * CROSS_GROUP_LANE; + const overY = + Math.min(source.y, target.y) - + CROSS_GROUP_GUTTER - + lane * CROSS_GROUP_LANE; + points = [ + start, + { x: outX, y: start.y }, + { x: outX, y: overY }, + { x: inX, y: overY }, + { x: inX, y: end.y }, + end, + ]; + } + points = points.map((point) => ({ x: round(point.x), y: round(point.y) })); + const label = labelForModes(edge.modes); + const partial = { + edge, + route: "forward" as const, + componentId: "", + sourceOffset: 0, + targetOffset: 0, + cycleLane: 0, + forwardLane: 0, + crossesGroup: true, + points, + label, + }; + const labelBounds = chooseLabelBounds(partial, nodeById, nodes, labels, { + minX: Math.min(source.x, target.x), + minY: Math.min(source.y, target.y), + maxX: Math.max(source.x + source.width, target.x + target.width), + maxY: Math.max(source.y + source.height, target.y + target.height), + }); + labels.push(labelBounds); + return { + ...partial, + labelBounds, + labelX: round(labelBounds.x + labelBounds.width / 2), + labelY: round(labelBounds.y + labelBounds.height - 3), + }; + }); +} + function pathFromPoints(points: readonly SystemGraphPoint[]): string { if (points.length === 0) return ""; const parts = [`M ${round(points[0]!.x)} ${round(points[0]!.y)}`]; @@ -702,9 +936,177 @@ function pathFromPoints(points: readonly SystemGraphPoint[]): string { return parts.join(" "); } +const translateNode = ( + node: SystemGraphLayoutNode, + dx: number, + dy: number, +): SystemGraphLayoutNode => ({ + ...node, + x: round(node.x + dx), + y: round(node.y + dy), +}); + +const translateRouted = ( + edge: RoutedEdge, + dx: number, + dy: number, +): RoutedEdge => ({ + ...edge, + points: edge.points.map((point) => ({ + x: round(point.x + dx), + y: round(point.y + dy), + })), + labelX: round(edge.labelX + dx), + labelY: round(edge.labelY + dy), + labelBounds: { + ...edge.labelBounds, + x: round(edge.labelBounds.x + dx), + y: round(edge.labelBounds.y + dy), + }, +}); + +interface Region { + id: string; + /** null for the single implicit region of an ungrouped layout — the one case + * where nothing is drawn around the content. */ + label: string | null; + nodeIds: string[]; +} + +/** + * The containers to draw, as an exhaustive partition of the graph's nodes. + * + * `undefined` groups means "no grouping information" — the graph is laid out as + * one unlabelled region, exactly as before this existed. That is not the same + * as an EMPTY group list, and the caller must keep them apart: the map is + * handed groups only once the project's stored arrangement AND the launch edges + * have landed, so a project mid-load never flashes an "Ungrouped" container + * that then turns out to be wrong. + */ +function toRegions( + graph: SystemGraph, + groups: readonly SystemGraphNodeGroup[] | undefined, +): Region[] { + const order = graph.nodes.map((node) => node.id); + if (!groups) return [{ id: "", label: null, nodeIds: order }]; + const known = new Set(order); + const claimed = new Set(); + const regions: Region[] = []; + for (const group of groups) { + const nodeIds: string[] = []; + for (const id of group.nodeIds) { + // Group membership is MANY-to-many — a shared subagent genuinely belongs + // to every system that calls it — but a map draws each agent once. First + // claim wins, in the rail's own order, so the card sits where the rail + // first mentions it rather than in whichever container drew last. + if (!known.has(id) || claimed.has(id)) continue; + claimed.add(id); + nodeIds.push(id); + } + // A group whose every member resolved to nothing is chrome around nothing. + if (nodeIds.length > 0) { + regions.push({ id: group.id, label: group.label, nodeIds }); + } + } + const leftover = order.filter((id) => !claimed.has(id)); + if (leftover.length > 0) { + // `systemGraphNodeGroups` hands over an exhaustive partition, so this is a + // backstop rather than a path — and deliberately not a throw. A node that + // silently disappears from the map is worse than a node filed in the bucket + // that means "nothing claims this". + const bucket = regions.find( + (region) => region.label === SYSTEM_GRAPH_UNGROUPED_LABEL, + ); + if (bucket) bucket.nodeIds.push(...leftover); + else { + regions.push({ + id: "group:unclaimed", + label: SYSTEM_GRAPH_UNGROUPED_LABEL, + nodeIds: leftover, + }); + } + } + return regions; +} + +interface LaidRegion extends Sized { + id: string; + label: string | null; + nodes: SystemGraphLayoutNode[]; + edges: RoutedEdge[]; + insetX: number; + insetY: number; +} + +/** + * One container, laid out in its OWN coordinates and measured afterwards. + * + * Measuring after routing is what makes the container honest: its box is the + * union of its cards, its connectors and its connector labels, so nothing a + * group draws can end up outside the border drawn around it. Sizing the box + * from the cards alone would let a cycle gutter or a displaced label spill into + * the neighbouring system. + */ +function layoutRegion(graph: SystemGraph, region: Region): LaidRegion { + const members = new Set(region.nodeIds); + const subgraph: SystemGraph = { + ...graph, + nodes: graph.nodes.filter((node) => members.has(node.id)), + edges: graph.edges.filter( + (edge) => members.has(edge.from) && members.has(edge.to), + ), + }; + const placed = placeRegionNodes( + analyzeSystemGraph(subgraph), + region.label === null ? null : region.id, + ); + const labels: SystemGraphLabelBounds[] = []; + const routed = routeEdges( + groupSystemGraphEdges(subgraph.edges), + placed.nodes, + placed.componentBoxes, + placed.componentByNode, + placed.strongByNode, + labels, + ); + + const xs: number[] = []; + const ys: number[] = []; + for (const node of placed.nodes) { + xs.push(node.x, node.x + node.width); + ys.push(node.y, node.y + node.height); + } + for (const edge of routed) { + for (const point of edge.points) { + xs.push(point.x); + ys.push(point.y); + } + xs.push(edge.labelBounds.x, edge.labelBounds.x + edge.labelBounds.width); + ys.push(edge.labelBounds.y, edge.labelBounds.y + edge.labelBounds.height); + } + const minX = Math.min(...xs); + const minY = Math.min(...ys); + const contentWidth = round(Math.max(...xs) - minX); + const contentHeight = round(Math.max(...ys) - minY); + const labelled = region.label !== null; + return { + id: region.id, + label: region.label, + nodes: placed.nodes.map((node) => translateNode(node, -minX, -minY)), + edges: routed.map((edge) => translateRouted(edge, -minX, -minY)), + insetX: labelled ? GROUP_PADDING : 0, + insetY: labelled ? GROUP_PADDING + GROUP_HEADER : 0, + width: labelled ? contentWidth + GROUP_PADDING * 2 : contentWidth, + height: labelled + ? contentHeight + GROUP_PADDING * 2 + GROUP_HEADER + : contentHeight, + }; +} + function shiftLayout( nodes: SystemGraphLayoutNode[], edges: RoutedEdge[], + groups: SystemGraphLayoutGroup[], ): SystemGraphLayout { const xs: number[] = []; const ys: number[] = []; @@ -712,6 +1114,10 @@ function shiftLayout( xs.push(node.x, node.x + node.width); ys.push(node.y, node.y + node.height); } + for (const group of groups) { + xs.push(group.x, group.x + group.width); + ys.push(group.y, group.y + group.height); + } for (const edge of edges) { for (const point of edge.points) { xs.push(point.x); @@ -721,7 +1127,7 @@ function shiftLayout( ys.push(edge.labelBounds.y, edge.labelBounds.y + edge.labelBounds.height); } if (xs.length === 0 || ys.length === 0) { - return { nodes: [], edges: [], bounds: { width: 0, height: 0 } }; + return { nodes: [], edges: [], groups: [], bounds: { width: 0, height: 0 } }; } const minX = Math.min(...xs); const minY = Math.min(...ys); @@ -729,37 +1135,32 @@ function shiftLayout( const maxY = Math.max(...ys); const dx = LAYOUT_PADDING - minX; const dy = LAYOUT_PADDING - minY; - const shiftedNodes = nodes.map((node) => ({ - ...node, - x: round(node.x + dx), - y: round(node.y + dy), + const shiftedNodes = nodes.map((node) => translateNode(node, dx, dy)); + const shiftedGroups = groups.map((group) => ({ + ...group, + x: round(group.x + dx), + y: round(group.y + dy), })); const shiftedEdges = edges.map((edge): SystemGraphLayoutEdge => { - const points = edge.points.map((point) => ({ - x: round(point.x + dx), - y: round(point.y + dy), - })); - const labelBounds = { - ...edge.labelBounds, - x: round(edge.labelBounds.x + dx), - y: round(edge.labelBounds.y + dy), - }; + const moved = translateRouted(edge, dx, dy); return { - from: edge.edge.from, - to: edge.edge.to, - modes: [...edge.edge.modes], - path: pathFromPoints(points), - points, - label: edge.label, - labelX: round(edge.labelX + dx), - labelY: round(edge.labelY + dy), - labelBounds, - route: edge.route, + from: moved.edge.from, + to: moved.edge.to, + modes: [...moved.edge.modes], + path: pathFromPoints(moved.points), + points: moved.points, + label: moved.label, + labelX: moved.labelX, + labelY: moved.labelY, + labelBounds: moved.labelBounds, + route: moved.route, + crossesGroup: moved.crossesGroup, }; }); return { nodes: shiftedNodes, edges: shiftedEdges, + groups: shiftedGroups, bounds: { width: round(maxX - minX + LAYOUT_PADDING * 2), height: round(maxY - minY + LAYOUT_PADDING * 2), @@ -767,20 +1168,54 @@ function shiftLayout( }; } -export function layoutSystemGraph(graph: SystemGraph): SystemGraphLayout { - const topology = analyzeSystemGraph(graph); +export function layoutSystemGraph( + graph: SystemGraph, + groups?: readonly SystemGraphNodeGroup[], +): SystemGraphLayout { + validateGraph(graph); if (graph.nodes.length === 0) { - return { nodes: [], edges: [], bounds: { width: 0, height: 0 } }; + return { nodes: [], edges: [], groups: [], bounds: { width: 0, height: 0 } }; } - const placed = placeNodes(topology); - const edges = routeEdges( - graph, - placed.nodes, - placed.componentBoxes, - placed.componentByNode, - placed.strongByNode, + const laid = toRegions(graph, groups).map((region) => + layoutRegion(graph, region), ); - return shiftLayout(placed.nodes, edges); + const packed = shelfPack(laid, GROUP_GAP); + + const nodes: SystemGraphLayoutNode[] = []; + const routed: RoutedEdge[] = []; + const labels: SystemGraphLabelBounds[] = []; + const boxes: SystemGraphLayoutGroup[] = []; + laid.forEach((region, index) => { + const at = packed.offsets[index]!; + const dx = at.x + region.insetX; + const dy = at.y + region.insetY; + for (const node of region.nodes) nodes.push(translateNode(node, dx, dy)); + for (const edge of region.edges) { + const moved = translateRouted(edge, dx, dy); + routed.push(moved); + labels.push(moved.labelBounds); + } + if (region.label !== null) { + boxes.push({ + id: region.id, + label: region.label, + x: round(at.x), + y: round(at.y), + width: region.width, + height: region.height, + nodeCount: region.nodes.length, + }); + } + }); + nodes.sort((left, right) => compareIds(left.id, right.id)); + + const groupOfNode = new Map(nodes.map((node) => [node.id, node.groupId])); + const crossing = groupSystemGraphEdges(graph.edges).filter( + (edge) => groupOfNode.get(edge.from) !== groupOfNode.get(edge.to), + ); + routed.push(...routeCrossGroupEdges(crossing, nodes, labels)); + + return shiftLayout(nodes, routed, boxes); } export function systemGraphNodeById( diff --git a/packages/harness/web/src/lib/use-rail-groups.ts b/packages/harness/web/src/lib/use-rail-groups.ts index 99018f3a8..3450756fc 100644 --- a/packages/harness/web/src/lib/use-rail-groups.ts +++ b/packages/harness/web/src/lib/use-rail-groups.ts @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useSyncExternalStore } from "react"; import type { WorkflowInfo } from "@shared/types"; import { createApi } from "./api"; @@ -27,6 +27,69 @@ const api = createApi(); * path the settings file or a session cwd produced, a space can. */ const ROOTS_SEP = "\n"; +/** + * ONE arrangement per project root, shared by every consumer on the page. + * + * The rail and the project MAP are two views of the same groups (SAP-2983), and + * two hook instances holding two copies is exactly how they come to disagree: + * an edit in the rail would leave the map still drawing the arrangement from + * before it, with no event to tell it otherwise — the file is the only shared + * medium, and nothing re-reads it. So the cache is module-level for the same + * reason the file is per project: there is one answer, and both surfaces read + * it. + * + * `requested` lives here too, so a second surface mounting does not re-issue a + * read the first one already has in flight. + */ +interface RailGroupsStore { + /** A property of the INSTALL, not of a root, so one read serves every + * project. Null until it lands. */ + edges: LaunchEdge[] | null; + edgesRequested: boolean; + states: Map; + loaded: Set; + requested: Set; + listeners: Set<() => void>; + /** Bumped on every mutation. `useSyncExternalStore` compares it by identity, + * and every accessor below takes it as a dependency — a Map mutated in place + * cannot be compared, and a version number can. */ + version: number; +} + +const store: RailGroupsStore = { + edges: null, + edgesRequested: false, + states: new Map(), + loaded: new Set(), + requested: new Set(), + listeners: new Set(), + version: 0, +}; + +function notify(): void { + store.version += 1; + for (const listener of [...store.listeners]) listener(); +} + +function subscribe(listener: () => void): () => void { + store.listeners.add(listener); + return () => { + store.listeners.delete(listener); + }; +} + +const snapshot = (): number => store.version; + +/** Roots are compared canonically — the rail holds what the user typed and the + * map holds what the server resolved, and `/` and `` are one + * directory. Storing under the raw spelling would give one project two + * arrangements. */ +const canonicalRoot = (root: string): string => + root.replace(/\\/g, "/").replace(/(.)\/+$/, "$1"); + +/** Writes in flight, per canonical root. See `commit` below. */ +const writeChain = new Map>(); + export interface RailGroups { /** The rows to render for one project root: the stored groups if the user has * any, the derived ones until then, `Ungrouped` last either way. */ @@ -43,6 +106,10 @@ export interface RailGroups { * arrangement that is still in flight; materializing before the edges land * would freeze an EMPTY derived set as the user's own, which is the stuck * state `Reset to detected` exists to escape — reached by clicking fast. + * + * The map reads it for a different reason and the same one: drawing before + * both halves land would show every agent in `Ungrouped` for a beat, which is + * a real arrangement and would read as the answer. */ isReady: (root: string) => boolean; /** The stored state, for the reset control's copy ("Discards 3 groups"). */ @@ -88,9 +155,7 @@ export function useRailGroups( sort: RailSort, enabled: boolean, ): RailGroups { - const [edges, setEdges] = useState(null); - const [states, setStates] = useState>({}); - const [loaded, setLoaded] = useState>({}); + const version = useSyncExternalStore(subscribe, snapshot, snapshot); // The registry changes as agents are scanned, and the load effect below must // not re-run for that — it would re-read every project's file. It reads the @@ -98,39 +163,34 @@ export function useRailGroups( const workflowsRef = useRef(workflows); workflowsRef.current = workflows; - /** Roots a read has already been started for. A ref, not state, so it updates - * synchronously and a re-render mid-flight cannot start a second read. */ - const requested = useRef(new Set()); - // Launch edges are a property of the INSTALL, not of a root, so one read // serves every project. Fetched only once the axis is in use: it greps every // registered agent's sources, and the Project axis has no use for the answer. useEffect(() => { - if (!enabled) return; - let live = true; + if (!enabled || store.edgesRequested) return; + store.edgesRequested = true; void api .listLaunchEdges() .then((next) => { - if (live) setEdges(next); + store.edges = next; + notify(); }) .catch(() => { // No edges is a truthful degradation: every agent shows in `Ungrouped`, // which is what a repo with no launch calls looks like anyway. Recorded // as an empty ARRAY rather than left null so the axis becomes editable — // hand-grouping is the whole point when detection finds nothing. - if (live) setEdges([]); + store.edges = []; + notify(); }); - return () => { - live = false; - }; }, [enabled]); - const rootsKey = roots.join(ROOTS_SEP); + const rootsKey = roots.map(canonicalRoot).join(ROOTS_SEP); useEffect(() => { if (!enabled) return; for (const root of rootsKey.split(ROOTS_SEP).filter(Boolean)) { - if (requested.current.has(root)) continue; - requested.current.add(root); + if (store.requested.has(root)) continue; + store.requested.add(root); void api .getRailState(root) .then((raw) => { @@ -138,27 +198,31 @@ export function useRailGroups( // member path belonging to a neighbouring project is still a real // agent, and pruning it here would silently drop it from a file the // next edit rewrites. - setStates((prev) => ({ ...prev, [root]: readRailState(raw, workflowsRef.current) })); - setLoaded((prev) => ({ ...prev, [root]: true })); + store.states.set(root, readRailState(raw, workflowsRef.current)); + store.loaded.add(root); + notify(); }) .catch(() => { // An older server with no such route, or an unreadable project. Both // read as "nothing stored", which shows the derived groups — but the // root stays NOT loaded, so nothing can be edited into a file we were // unable to read. - setStates((prev) => ({ ...prev, [root]: EMPTY_RAIL_STATE })); + store.states.set(root, EMPTY_RAIL_STATE); + notify(); }); } }, [enabled, rootsKey]); const stateFor = useCallback( - (root: string): RailState => states[root] ?? EMPTY_RAIL_STATE, - [states], + (root: string): RailState => store.states.get(canonicalRoot(root)) ?? EMPTY_RAIL_STATE, + // `version` stands in for the store: a Map mutated in place cannot be a + // dependency, and the counter it bumps can. + [version], ); const isReady = useCallback( - (root: string): boolean => edges !== null && loaded[root] === true, - [edges, loaded], + (root: string): boolean => store.edges !== null && store.loaded.has(canonicalRoot(root)), + [version], ); /** @@ -169,23 +233,21 @@ export function useRailGroups( * here: the reset's DELETE finishing before the drag's PUT leaves the file * holding the arrangement the reset was meant to erase, which is the stuck * state all over again. Chaining makes the file end where the user left off. - */ - const writeChain = useRef(new Map>()); - - /** - * The ONE place a rail-state file is written. `railStateWrite` returns the - * DECISION — write this text, or remove the file — so the null/empty - * distinction cannot be lost at a call site. + * + * Module-level with the rest of the store, because two SURFACES editing one + * root are the same race as two edits from one. */ const commit = useCallback((root: string, next: RailState): void => { - setStates((prev) => ({ ...prev, [root]: next })); + const key = canonicalRoot(root); + store.states.set(key, next); + notify(); const write = railStateWrite(next); - const previous = writeChain.current.get(root) ?? Promise.resolve(); + const previous = writeChain.get(key) ?? Promise.resolve(); const persisted = previous.then(() => write.kind === "write" ? api.saveRailState(root, write.raw) : api.clearRailState(root), ); - writeChain.current.set( - root, + writeChain.set( + key, persisted.catch(() => { // A read-only checkout or an older server. The arrangement still applies // for this session; it simply will not be there next time — and the @@ -200,19 +262,21 @@ export function useRailGroups( rootWorkflows: readonly WorkflowInfo[], fn: (state: MaterializedRailState) => MaterializedRailState, ): void => { - if (edges === null || !loaded[root]) return; - const current = states[root] ?? EMPTY_RAIL_STATE; - commit(root, fn(materialize(current, rootWorkflows, edges, sort))); + const key = canonicalRoot(root); + if (store.edges === null || !store.loaded.has(key)) return; + const current = store.states.get(key) ?? EMPTY_RAIL_STATE; + commit(root, fn(materialize(current, rootWorkflows, store.edges, sort))); }, - [commit, edges, loaded, sort, states], + [commit, sort], ); const reset = useCallback( (root: string): void => { - if (!loaded[root]) return; - commit(root, resetToDetected(states[root] ?? EMPTY_RAIL_STATE)); + const key = canonicalRoot(root); + if (!store.loaded.has(key)) return; + commit(root, resetToDetected(store.states.get(key) ?? EMPTY_RAIL_STATE)); }, - [commit, loaded, states], + [commit], ); const agentsIn = useCallback( @@ -223,8 +287,13 @@ export function useRailGroups( const groupsFor = useCallback( (root: string, rootWorkflows: readonly WorkflowInfo[]): GroupNode[] => - deriveOrStored(rootWorkflows, states[root] ?? EMPTY_RAIL_STATE, edges ?? [], sort), - [edges, sort, states], + deriveOrStored( + rootWorkflows, + store.states.get(canonicalRoot(root)) ?? EMPTY_RAIL_STATE, + store.edges ?? [], + sort, + ), + [sort, version], ); return useMemo( diff --git a/packages/harness/web/src/styles.css b/packages/harness/web/src/styles.css index e421049bd..88d426cd5 100644 --- a/packages/harness/web/src/styles.css +++ b/packages/harness/web/src/styles.css @@ -4223,6 +4223,16 @@ button.rail-footer-card:hover { stroke-dasharray: 5 5; } +/* Two systems that touch. Dotted and lighter than the wiring inside a + container, because "these two are connected" is a weaker claim than "this is + how this system is built" — and these are the only connectors that cross a + border, so they must not read as that border being wrong. */ +.system-graph-edge.is-cross-group { + stroke: var(--text-faint); + stroke-dasharray: 2 4; + opacity: 0.75; +} + .system-graph-arrow { fill: var(--text-faint); } @@ -4237,6 +4247,33 @@ button.rail-footer-card:hover { pointer-events: none; } +/* A named, bounded container around one system's cards — the shape the + fourfold map reference draws around "THE LOOP". Behind everything (the edge + svg and the cards are later siblings), and NOT interactive: a group is + edited in the rail, which is the surface that owns the arrangement. */ +.system-graph-group { + position: absolute; + box-sizing: border-box; + padding: var(--sp2) var(--sp3); + border: 1px solid var(--ui-line); + border-radius: calc(var(--radius) * 2); + background: var(--surface-inset); + pointer-events: none; +} + +.system-graph-group-label { + display: block; + max-width: 100%; + overflow: hidden; + color: var(--text-faint); + font-family: var(--font-mono); + font-size: var(--type-meta); + letter-spacing: 0.08em; + text-overflow: ellipsis; + text-transform: uppercase; + white-space: nowrap; +} + .system-graph-node { position: absolute; display: flex; From 6f52863ba51403b4b2fae4af6e17b824adddf4d4 Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sun, 30 Aug 2026 09:43:33 -0700 Subject: [PATCH 2/5] fix(harness): keep a container named at the zoom you read its shape from [SAP-2983] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on the 9-system, 76-agent tree: the container label rendered 3.65px tall at the map's own arrival zoom and 2.96px after Fit. The name is the whole thing a container adds, so it counter-scales against the view — clamped, and 1:1 once the cards are legible on their own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91 --- packages/harness/geometry.probe.mjs | 71 +++++++++++++++++++ packages/harness/label-size.probe.mjs | 29 ++++++++ packages/harness/probe-map.probe.mjs | 43 +++++++++++ packages/harness/rail-vs-map.probe.mjs | 34 +++++++++ .../web/src/components/SystemGraphCanvas.tsx | 6 +- packages/harness/web/src/styles.css | 11 +++ 6 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 packages/harness/geometry.probe.mjs create mode 100644 packages/harness/label-size.probe.mjs create mode 100644 packages/harness/probe-map.probe.mjs create mode 100644 packages/harness/rail-vs-map.probe.mjs diff --git a/packages/harness/geometry.probe.mjs b/packages/harness/geometry.probe.mjs new file mode 100644 index 000000000..5dcf803fb --- /dev/null +++ b/packages/harness/geometry.probe.mjs @@ -0,0 +1,71 @@ +import { chromium } from "@playwright/test"; +const [url, shot, fit] = process.argv.slice(2); +const b = await chromium.launch(); +const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); +p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); +await p.goto(url, { waitUntil: "networkidle" }); +await p.waitForTimeout(1500); +await p.keyboard.press("Escape"); +await p.waitForTimeout(400); +await p.locator('[data-testid^="project-select-"]').first().click(); +await p.waitForTimeout(7000); +if (fit === "fit") { + await p.getByTestId("system-graph-fit").click(); + await p.waitForTimeout(700); +} + +const report = await p.evaluate(() => { + const num = (v) => parseFloat(v); + const groups = [...document.querySelectorAll(".system-graph-group")].map((el) => ({ + id: el.getAttribute("data-group-id"), + label: el.getAttribute("data-group-label"), + count: Number(el.getAttribute("data-group-nodes")), + x: num(el.style.left), y: num(el.style.top), + w: num(el.style.width), h: num(el.style.height), + // measured, not declared: what the browser actually laid out + rect: el.getBoundingClientRect().toJSON(), + labelText: el.querySelector(".system-graph-group-label")?.textContent ?? null, + bg: getComputedStyle(el).backgroundColor, + border: getComputedStyle(el).borderTopColor, + })); + const nodes = [...document.querySelectorAll(".system-graph-node")].map((el) => ({ + key: el.getAttribute("data-agent-key"), + x: num(el.style.left), y: num(el.style.top), + w: num(el.style.width), h: num(el.style.height), + rect: el.getBoundingClientRect().toJSON(), + })); + const inside = (g, n) => + n.x >= g.x && n.y >= g.y && n.x + n.w <= g.x + g.w && n.y + n.h <= g.y + g.h; + const homeless = nodes.filter((n) => !groups.some((g) => inside(g, n))); + const doubled = nodes.filter((n) => groups.filter((g) => inside(g, n)).length > 1); + const overlapping = []; + for (let i = 0; i < groups.length; i++) + for (let j = i + 1; j < groups.length; j++) { + const a = groups[i], c = groups[j]; + if (!(a.x + a.w <= c.x || c.x + c.w <= a.x || a.y + a.h <= c.y || c.y + c.h <= a.y)) + overlapping.push([a.label, c.label]); + } + const counted = groups.reduce((s, g) => s + nodes.filter((n) => inside(g, n)).length, 0); + const subject = document.querySelector(".system-graph-subject"); + const viewport = document.querySelector(".system-graph-viewport").getBoundingClientRect(); + // Nothing may overflow the pane horizontally. + const overflowRight = Math.max(0, ...groups.map((g) => g.rect.x + g.rect.width - viewport.right)); + return { + subject: { w: num(subject.style.width), h: num(subject.style.height) }, + groupCount: groups.length, + nodeCount: nodes.length, + countedInsideContainers: counted, + homeless: homeless.map((n) => n.key), + doubled: doubled.map((n) => n.key), + overlappingContainers: overlapping, + labelsRendered: groups.map((g) => g.labelText), + declaredCounts: groups.map((g) => [g.label, g.count]), + firstContainerStyle: groups[0] ? { bg: groups[0].bg, border: groups[0].border } : null, + overflowRightPx: Math.round(overflowRight), + distinctRows: new Set(groups.map((g) => g.y)).size, + distinctCols: new Set(groups.map((g) => g.x)).size, + }; +}); +console.log(JSON.stringify(report, null, 1)); +if (shot) await p.screenshot({ path: shot }); +await b.close(); diff --git a/packages/harness/label-size.probe.mjs b/packages/harness/label-size.probe.mjs new file mode 100644 index 000000000..5dfe64915 --- /dev/null +++ b/packages/harness/label-size.probe.mjs @@ -0,0 +1,29 @@ +import { chromium } from "@playwright/test"; +const [url] = process.argv.slice(2); +const b = await chromium.launch(); +const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); +await p.goto(url, { waitUntil: "networkidle" }); +await p.waitForTimeout(1500); +await p.keyboard.press("Escape"); +await p.waitForTimeout(400); +await p.locator('[data-testid^="project-select-"]').first().click(); +await p.waitForTimeout(7000); +const read = async (tag) => { + const r = await p.evaluate(() => { + const zoom = document.querySelector('[data-testid="system-graph-zoom-reset"]').textContent; + const el = document.querySelector(".system-graph-group-label"); + const node = document.querySelector(".system-graph-node-label"); + return { + zoom, + groupLabelPx: el ? +el.getBoundingClientRect().height.toFixed(2) : null, + groupLabelWidthPx: el ? +el.getBoundingClientRect().width.toFixed(2) : null, + nodeLabelPx: node ? +node.getBoundingClientRect().height.toFixed(2) : null, + }; + }); + console.log(tag, JSON.stringify(r)); +}; +await read("arrival "); +await p.getByTestId("system-graph-fit").click(); +await p.waitForTimeout(600); +await read("fitted "); +await b.close(); diff --git a/packages/harness/probe-map.probe.mjs b/packages/harness/probe-map.probe.mjs new file mode 100644 index 000000000..022a2e998 --- /dev/null +++ b/packages/harness/probe-map.probe.mjs @@ -0,0 +1,43 @@ +import { chromium } from "@playwright/test"; +const url = process.argv[2]; +const shot = process.argv[3]; +const b = await chromium.launch(); +const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); +p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); +p.on("console", (m) => { if (m.type() === "error") console.log("CONSOLEERR", m.text()); }); +await p.goto(url, { waitUntil: "networkidle" }); +await p.waitForTimeout(2000); + +// A first-run help overlay covers the rail; dismiss it. +await p.keyboard.press("Escape"); +await p.waitForTimeout(500); + +// Select the project row in the rail so the map altitude is on. +const project = p.locator('[data-testid^="project-select-"]').first(); +if (await project.count()) await project.click(); +await p.waitForTimeout(6000); + +const data = await p.evaluate(() => { + const subject = document.querySelector(".system-graph-subject"); + const groups = [...document.querySelectorAll(".system-graph-group")].map((el) => ({ + label: el.getAttribute("data-group-label"), + nodes: Number(el.getAttribute("data-group-nodes")), + box: { x: parseFloat(el.style.left), y: parseFloat(el.style.top), w: parseFloat(el.style.width), h: parseFloat(el.style.height) }, + })); + const nodes = [...document.querySelectorAll(".system-graph-node")].map((el) => ({ + key: el.getAttribute("data-agent-key"), + x: parseFloat(el.style.left), y: parseFloat(el.style.top), + })); + return { + hasMap: !!document.querySelector('[data-testid="workspace-graph-view"]'), + subject: subject ? { w: subject.style.width, h: subject.style.height } : null, + groups, + nodeCount: nodes.length, + distinctX: new Set(nodes.map((n) => n.x)).size, + distinctY: new Set(nodes.map((n) => n.y)).size, + warning: document.querySelector('[data-testid="system-graph-warning"]')?.textContent ?? null, + }; +}); +console.log(JSON.stringify(data, null, 1)); +if (shot) await p.screenshot({ path: shot }); +await b.close(); diff --git a/packages/harness/rail-vs-map.probe.mjs b/packages/harness/rail-vs-map.probe.mjs new file mode 100644 index 000000000..8f4223b3b --- /dev/null +++ b/packages/harness/rail-vs-map.probe.mjs @@ -0,0 +1,34 @@ +import { chromium } from "@playwright/test"; +const url = process.argv[2]; +const b = await chromium.launch(); +const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); +p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); +p.on("response", (r) => { if (r.status() >= 400) console.log("HTTP", r.status(), r.url()); }); +await p.goto(url, { waitUntil: "networkidle" }); +await p.waitForTimeout(1500); +await p.keyboard.press("Escape"); +await p.waitForTimeout(400); + +await p.locator('[data-testid^="project-select-"]').first().click(); +await p.waitForTimeout(6000); + +const mapLabels = await p.$$eval(".system-graph-group", (els) => + els.map((el) => el.getAttribute("data-group-label")), +); + +// Switch the RAIL to the Group axis and read its rows. +await p.getByTestId("history-trigger").click(); +await p.getByTestId("filing-group-by").selectOption("group"); +await p.keyboard.press("Escape"); +await p.waitForTimeout(2500); +const railLabels = await p.$$eval( + '[data-testid^="group-row-"] .tree-row-label', + (els) => els.map((el) => el.textContent.trim()), +); +const mapAfterAxis = await p.$$eval(".system-graph-group", (els) => + els.map((el) => el.getAttribute("data-group-label")), +); + +console.log(JSON.stringify({ mapLabels, railLabels, mapAfterAxis, match: JSON.stringify(mapLabels) === JSON.stringify(railLabels) }, null, 1)); +await p.screenshot({ path: "/tmp/shots-2983/real-rail-and-map.png" }); +await b.close(); diff --git a/packages/harness/web/src/components/SystemGraphCanvas.tsx b/packages/harness/web/src/components/SystemGraphCanvas.tsx index 841ba730a..20b5bb828 100644 --- a/packages/harness/web/src/components/SystemGraphCanvas.tsx +++ b/packages/harness/web/src/components/SystemGraphCanvas.tsx @@ -317,7 +317,11 @@ export function SystemGraphCanvas({ width: layout.bounds.width, height: layout.bounds.height, transform: `translate(-50%, -50%) translate(${view.x}px, ${view.y}px) scale(${view.zoom})`, - } satisfies CSSProperties + // Published for the container labels, which counter-scale against + // it so a system stays NAMED at the altitude you zoom out to read + // its shape from (see .system-graph-group-label). + "--system-graph-zoom": view.zoom, + } as CSSProperties } role="group" aria-label="Workspace dependency graph" diff --git a/packages/harness/web/src/styles.css b/packages/harness/web/src/styles.css index 88d426cd5..6d998fbe5 100644 --- a/packages/harness/web/src/styles.css +++ b/packages/harness/web/src/styles.css @@ -4261,10 +4261,21 @@ button.rail-footer-card:hover { pointer-events: none; } +/* Counter-scaled against the view zoom. + + A container's NAME is the whole thing the container adds, and the map's own + arrival zoom on a real project is around 20% — measured on a 9-system, + 76-agent tree: the label rendered 3.65px tall, and 2.96px after Fit. That is + not a label. Growing it as the view shrinks keeps the systems named at + exactly the altitude you zoom out to read them from, and the clamp stops + there: at 70% and above the cards are legible on their own and the label is + 1:1. transform-origin pins it to the corner it already sits in. */ .system-graph-group-label { display: block; max-width: 100%; overflow: hidden; + transform: scale(clamp(1, calc(0.7 / var(--system-graph-zoom, 1)), 4)); + transform-origin: 0 0; color: var(--text-faint); font-family: var(--font-mono); font-size: var(--type-meta); From 76865adf104dd043fe058b78a42fcbb3879ea841 Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sun, 30 Aug 2026 09:53:25 -0700 Subject: [PATCH 3/5] test(harness): prove the map's containers in a browser, and add the changeset [SAP-2983] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `project-map-groups.spec.ts` asserts what no unit test can see: that the map reads the RAIL's arrangement, that its container labels equal the rail's rows on screen, and that a rename in the rail moves the map with no reload. Geometry is measured — cards are absolutely positioned siblings of the boxes, not their children, so "inside" is only settled by measurement. Also drops the throwaway probe scripts that were committed by accident. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91 --- .changeset/project-map-groups.md | 30 +++ packages/harness/geometry.probe.mjs | 71 ------- packages/harness/label-size.probe.mjs | 29 --- packages/harness/probe-map.probe.mjs | 43 ---- packages/harness/rail-vs-map.probe.mjs | 34 ---- .../web/e2e/project-map-groups.spec.ts | 191 ++++++++++++++++++ 6 files changed, 221 insertions(+), 177 deletions(-) create mode 100644 .changeset/project-map-groups.md delete mode 100644 packages/harness/geometry.probe.mjs delete mode 100644 packages/harness/label-size.probe.mjs delete mode 100644 packages/harness/probe-map.probe.mjs delete mode 100644 packages/harness/rail-vs-map.probe.mjs create mode 100644 packages/harness/web/e2e/project-map-groups.spec.ts diff --git a/.changeset/project-map-groups.md b/.changeset/project-map-groups.md new file mode 100644 index 000000000..de4099d08 --- /dev/null +++ b/.changeset/project-map-groups.md @@ -0,0 +1,30 @@ +--- +"@sapiom/harness": minor +--- + +Studio: the project map draws your groups as named containers, instead of one +endless column of agents. + +A project map used to draw every agent it contained as one flat set, ignoring +the structure the rail was showing right beside it. Open a folder holding +several systems and you got a single vertical column — a nine-system, 76-agent +folder came out 9,700 pixels tall and one card wide, which no amount of zooming +out makes readable. + +- **One labelled container per group.** The map now reads the same groups the + rail does, so a system you named in the rail is a system you can see on the + map, under exactly that name and in the same order. Rename or regroup in the + rail and the map follows immediately. +- **Ungrouped is a container too.** "No connections detected" is a real answer + about a project, not an absence, so it gets a labelled box rather than being + scattered loose. A project with one group renders as one group. +- **Agents with no connections wrap instead of stacking.** Inside a container, + unconnected agents fill the width and wrap, which is what replaces the column. +- **A connector between two containers is still drawn.** Splitting a system + across two groups does not make the link between the halves disappear; it is + drawn dotted, as a link between systems rather than wiring inside one. +- Container names stay readable as you zoom out, which is the moment you are + looking at the whole project and need to know which system is which. + +Nothing about how groups are detected, edited, or stored has changed. The map is +a new reader of the arrangement your project already has; it never writes one. diff --git a/packages/harness/geometry.probe.mjs b/packages/harness/geometry.probe.mjs deleted file mode 100644 index 5dcf803fb..000000000 --- a/packages/harness/geometry.probe.mjs +++ /dev/null @@ -1,71 +0,0 @@ -import { chromium } from "@playwright/test"; -const [url, shot, fit] = process.argv.slice(2); -const b = await chromium.launch(); -const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); -p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); -await p.goto(url, { waitUntil: "networkidle" }); -await p.waitForTimeout(1500); -await p.keyboard.press("Escape"); -await p.waitForTimeout(400); -await p.locator('[data-testid^="project-select-"]').first().click(); -await p.waitForTimeout(7000); -if (fit === "fit") { - await p.getByTestId("system-graph-fit").click(); - await p.waitForTimeout(700); -} - -const report = await p.evaluate(() => { - const num = (v) => parseFloat(v); - const groups = [...document.querySelectorAll(".system-graph-group")].map((el) => ({ - id: el.getAttribute("data-group-id"), - label: el.getAttribute("data-group-label"), - count: Number(el.getAttribute("data-group-nodes")), - x: num(el.style.left), y: num(el.style.top), - w: num(el.style.width), h: num(el.style.height), - // measured, not declared: what the browser actually laid out - rect: el.getBoundingClientRect().toJSON(), - labelText: el.querySelector(".system-graph-group-label")?.textContent ?? null, - bg: getComputedStyle(el).backgroundColor, - border: getComputedStyle(el).borderTopColor, - })); - const nodes = [...document.querySelectorAll(".system-graph-node")].map((el) => ({ - key: el.getAttribute("data-agent-key"), - x: num(el.style.left), y: num(el.style.top), - w: num(el.style.width), h: num(el.style.height), - rect: el.getBoundingClientRect().toJSON(), - })); - const inside = (g, n) => - n.x >= g.x && n.y >= g.y && n.x + n.w <= g.x + g.w && n.y + n.h <= g.y + g.h; - const homeless = nodes.filter((n) => !groups.some((g) => inside(g, n))); - const doubled = nodes.filter((n) => groups.filter((g) => inside(g, n)).length > 1); - const overlapping = []; - for (let i = 0; i < groups.length; i++) - for (let j = i + 1; j < groups.length; j++) { - const a = groups[i], c = groups[j]; - if (!(a.x + a.w <= c.x || c.x + c.w <= a.x || a.y + a.h <= c.y || c.y + c.h <= a.y)) - overlapping.push([a.label, c.label]); - } - const counted = groups.reduce((s, g) => s + nodes.filter((n) => inside(g, n)).length, 0); - const subject = document.querySelector(".system-graph-subject"); - const viewport = document.querySelector(".system-graph-viewport").getBoundingClientRect(); - // Nothing may overflow the pane horizontally. - const overflowRight = Math.max(0, ...groups.map((g) => g.rect.x + g.rect.width - viewport.right)); - return { - subject: { w: num(subject.style.width), h: num(subject.style.height) }, - groupCount: groups.length, - nodeCount: nodes.length, - countedInsideContainers: counted, - homeless: homeless.map((n) => n.key), - doubled: doubled.map((n) => n.key), - overlappingContainers: overlapping, - labelsRendered: groups.map((g) => g.labelText), - declaredCounts: groups.map((g) => [g.label, g.count]), - firstContainerStyle: groups[0] ? { bg: groups[0].bg, border: groups[0].border } : null, - overflowRightPx: Math.round(overflowRight), - distinctRows: new Set(groups.map((g) => g.y)).size, - distinctCols: new Set(groups.map((g) => g.x)).size, - }; -}); -console.log(JSON.stringify(report, null, 1)); -if (shot) await p.screenshot({ path: shot }); -await b.close(); diff --git a/packages/harness/label-size.probe.mjs b/packages/harness/label-size.probe.mjs deleted file mode 100644 index 5dfe64915..000000000 --- a/packages/harness/label-size.probe.mjs +++ /dev/null @@ -1,29 +0,0 @@ -import { chromium } from "@playwright/test"; -const [url] = process.argv.slice(2); -const b = await chromium.launch(); -const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); -await p.goto(url, { waitUntil: "networkidle" }); -await p.waitForTimeout(1500); -await p.keyboard.press("Escape"); -await p.waitForTimeout(400); -await p.locator('[data-testid^="project-select-"]').first().click(); -await p.waitForTimeout(7000); -const read = async (tag) => { - const r = await p.evaluate(() => { - const zoom = document.querySelector('[data-testid="system-graph-zoom-reset"]').textContent; - const el = document.querySelector(".system-graph-group-label"); - const node = document.querySelector(".system-graph-node-label"); - return { - zoom, - groupLabelPx: el ? +el.getBoundingClientRect().height.toFixed(2) : null, - groupLabelWidthPx: el ? +el.getBoundingClientRect().width.toFixed(2) : null, - nodeLabelPx: node ? +node.getBoundingClientRect().height.toFixed(2) : null, - }; - }); - console.log(tag, JSON.stringify(r)); -}; -await read("arrival "); -await p.getByTestId("system-graph-fit").click(); -await p.waitForTimeout(600); -await read("fitted "); -await b.close(); diff --git a/packages/harness/probe-map.probe.mjs b/packages/harness/probe-map.probe.mjs deleted file mode 100644 index 022a2e998..000000000 --- a/packages/harness/probe-map.probe.mjs +++ /dev/null @@ -1,43 +0,0 @@ -import { chromium } from "@playwright/test"; -const url = process.argv[2]; -const shot = process.argv[3]; -const b = await chromium.launch(); -const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); -p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); -p.on("console", (m) => { if (m.type() === "error") console.log("CONSOLEERR", m.text()); }); -await p.goto(url, { waitUntil: "networkidle" }); -await p.waitForTimeout(2000); - -// A first-run help overlay covers the rail; dismiss it. -await p.keyboard.press("Escape"); -await p.waitForTimeout(500); - -// Select the project row in the rail so the map altitude is on. -const project = p.locator('[data-testid^="project-select-"]').first(); -if (await project.count()) await project.click(); -await p.waitForTimeout(6000); - -const data = await p.evaluate(() => { - const subject = document.querySelector(".system-graph-subject"); - const groups = [...document.querySelectorAll(".system-graph-group")].map((el) => ({ - label: el.getAttribute("data-group-label"), - nodes: Number(el.getAttribute("data-group-nodes")), - box: { x: parseFloat(el.style.left), y: parseFloat(el.style.top), w: parseFloat(el.style.width), h: parseFloat(el.style.height) }, - })); - const nodes = [...document.querySelectorAll(".system-graph-node")].map((el) => ({ - key: el.getAttribute("data-agent-key"), - x: parseFloat(el.style.left), y: parseFloat(el.style.top), - })); - return { - hasMap: !!document.querySelector('[data-testid="workspace-graph-view"]'), - subject: subject ? { w: subject.style.width, h: subject.style.height } : null, - groups, - nodeCount: nodes.length, - distinctX: new Set(nodes.map((n) => n.x)).size, - distinctY: new Set(nodes.map((n) => n.y)).size, - warning: document.querySelector('[data-testid="system-graph-warning"]')?.textContent ?? null, - }; -}); -console.log(JSON.stringify(data, null, 1)); -if (shot) await p.screenshot({ path: shot }); -await b.close(); diff --git a/packages/harness/rail-vs-map.probe.mjs b/packages/harness/rail-vs-map.probe.mjs deleted file mode 100644 index 8f4223b3b..000000000 --- a/packages/harness/rail-vs-map.probe.mjs +++ /dev/null @@ -1,34 +0,0 @@ -import { chromium } from "@playwright/test"; -const url = process.argv[2]; -const b = await chromium.launch(); -const p = await b.newPage({ viewport: { width: 1500, height: 950 } }); -p.on("pageerror", (e) => console.log("PAGEERROR", e.message)); -p.on("response", (r) => { if (r.status() >= 400) console.log("HTTP", r.status(), r.url()); }); -await p.goto(url, { waitUntil: "networkidle" }); -await p.waitForTimeout(1500); -await p.keyboard.press("Escape"); -await p.waitForTimeout(400); - -await p.locator('[data-testid^="project-select-"]').first().click(); -await p.waitForTimeout(6000); - -const mapLabels = await p.$$eval(".system-graph-group", (els) => - els.map((el) => el.getAttribute("data-group-label")), -); - -// Switch the RAIL to the Group axis and read its rows. -await p.getByTestId("history-trigger").click(); -await p.getByTestId("filing-group-by").selectOption("group"); -await p.keyboard.press("Escape"); -await p.waitForTimeout(2500); -const railLabels = await p.$$eval( - '[data-testid^="group-row-"] .tree-row-label', - (els) => els.map((el) => el.textContent.trim()), -); -const mapAfterAxis = await p.$$eval(".system-graph-group", (els) => - els.map((el) => el.getAttribute("data-group-label")), -); - -console.log(JSON.stringify({ mapLabels, railLabels, mapAfterAxis, match: JSON.stringify(mapLabels) === JSON.stringify(railLabels) }, null, 1)); -await p.screenshot({ path: "/tmp/shots-2983/real-rail-and-map.png" }); -await b.close(); diff --git a/packages/harness/web/e2e/project-map-groups.spec.ts b/packages/harness/web/e2e/project-map-groups.spec.ts new file mode 100644 index 000000000..613ea161a --- /dev/null +++ b/packages/harness/web/e2e/project-map-groups.spec.ts @@ -0,0 +1,191 @@ +/** + * SAP-2983 — the project map draws the groups the rail already has. + * + * The unit tests pin the two pure halves: `lib/system-graph-groups.test.ts` + * decides which node belongs to which container, `lib/system-graph-layout.test.ts` + * decides where the container goes. Neither can see the thing the ticket is + * about — that the map READS the rail's arrangement at all, and that the two + * surfaces agree on screen. A layout rule is not proven by a unit test, and a + * map drawing a second opinion of the same groups would pass every one of them. + * + * `?mockFixtures=deep` is the fixture with a real group axis: `MOCK_LAUNCH_EDGES` + * produces a three-member component (gateway), a two-member one (mailer), an + * edge to an agent this install lacks, and agents no edge reaches — plus + * `MOCK_POLSIA_GRAPH_EDGES`, whose connectors run BETWEEN those groups, which is + * the cross-container case. + * + * Every assertion here was mutation-tested; what each mutation was, and which + * assertion caught it, is on the PR. + */ +import { expect, test } from "@playwright/test"; +import type { Page } from "@playwright/test"; + +/** The container labels the map draws, in DOM order. */ +const mapContainers = (page: Page): Promise<(string | null)[]> => + page + .locator(".system-graph-group") + .evaluateAll((els) => + els.map((el) => el.getAttribute("data-group-label")), + ); + +/** The group rows the RAIL draws for polsia, in DOM order. */ +const railGroups = (page: Page): Promise => + page + .getByTestId("workspace-group-polsia") + .locator('[data-testid^="group-row-"] .tree-row-label') + .allInnerTexts(); + +/** Switch the rail to the Group axis and wait for it to be editable. */ +async function openGroupAxis(page: Page): Promise { + await page.getByTestId("history-trigger").click(); + await page.getByTestId("filing-group-by").selectOption("group"); + await page.keyboard.press("Escape"); + await expect(page.getByTestId("group-create-polsia")).toBeVisible(); +} + +test.beforeEach(async ({ page }) => { + await page.goto("/?mockFixtures=deep"); + await expect(page.locator(".rail-workflows")).toBeVisible(); + await page.getByTestId("project-select-polsia").click(); + await expect(page.getByTestId("workspace-graph-view")).toBeVisible(); + await expect(page.locator(".system-graph-group").first()).toBeVisible(); +}); + +test("one labelled container per group, named exactly as the rail names it", async ({ + page, +}) => { + // The whole ticket. Two names for one group is the failure it prevents, and + // it is only visible with both surfaces on screen at once. + await expect(page.locator(".system-graph-group")).toHaveCount(3); + expect(await mapContainers(page)).toEqual(["gateway", "mailer", "Ungrouped"]); + + await openGroupAxis(page); + expect(await railGroups(page)).toEqual(await mapContainers(page)); +}); + +test("every card sits inside exactly one container, measured", async ({ + page, +}) => { + /* GEOMETRY, not counts. A container assertion that still passes when the + cards are drawn outside their boxes is worthless — and the boxes are + absolutely positioned siblings of the cards, not their DOM parents, so + "inside" is a claim only measurement can settle. */ + const placement = await page.evaluate(() => { + const box = (el: Element) => el.getBoundingClientRect(); + const groups = [...document.querySelectorAll(".system-graph-group")].map( + (el) => ({ label: el.getAttribute("data-group-label"), rect: box(el) }), + ); + const contains = (outer: DOMRect, inner: DOMRect) => + inner.left >= outer.left - 0.5 && + inner.top >= outer.top - 0.5 && + inner.right <= outer.right + 0.5 && + inner.bottom <= outer.bottom + 0.5; + return [...document.querySelectorAll(".system-graph-node")].map((el) => ({ + key: el.getAttribute("data-agent-key"), + in: groups + .filter((group) => contains(group.rect, box(el))) + .map((group) => group.label), + })); + }); + + expect(placement.length).toBeGreaterThan(0); + for (const card of placement) { + expect(card.in, `${card.key} is in exactly one container`).toHaveLength(1); + } + expect( + placement.filter((card) => card.in[0] === "gateway").map((c) => c.key).sort(), + ).toEqual(["ads-worker", "gateway", "queue"]); + expect( + placement.filter((card) => card.in[0] === "mailer").map((c) => c.key).sort(), + ).toEqual(["mailer", "sender"]); +}); + +test("containers do not overlap, and none is drawn outside the map's own bounds", async ({ + page, +}) => { + /* The subject box IS the layout's bounds, and the viewport's fit, its zoom + floor and its "did the stored view still show anything" check all read + them. A container drawn outside them is a container Fit cannot bring on + screen — and it is invisible to any assertion that only counts boxes, + which is how a row overflowing its rail by 17px shipped. */ + const measured = await page.evaluate(() => { + const rects = [...document.querySelectorAll(".system-graph-group")].map( + (el) => el.getBoundingClientRect(), + ); + const overlaps: string[] = []; + for (let left = 0; left < rects.length; left += 1) { + for (let right = left + 1; right < rects.length; right += 1) { + const a = rects[left]!; + const b = rects[right]!; + if ( + !( + a.right <= b.left || + b.right <= a.left || + a.bottom <= b.top || + b.bottom <= a.top + ) + ) { + overlaps.push(`${left}/${right}`); + } + } + } + const subject = document + .querySelector(".system-graph-subject")! + .getBoundingClientRect(); + const escaping = rects.filter( + (rect) => + rect.left < subject.left - 0.5 || + rect.top < subject.top - 0.5 || + rect.right > subject.right + 0.5 || + rect.bottom > subject.bottom + 0.5, + ).length; + return { count: rects.length, overlaps, escaping }; + }); + expect(measured.count).toBe(3); + expect(measured.overlaps).toEqual([]); + expect(measured.escaping).toBe(0); +}); + +test("a rail edit moves the map, with no reload", async ({ page }) => { + /* The rail and the map are two views of ONE arrangement. Two copies of the + state is exactly how they come to disagree: the file is the only shared + medium and nothing re-reads it, so an edit in the rail would leave the map + drawing what it read on mount. */ + await openGroupAxis(page); + expect(await mapContainers(page)).toContain("gateway"); + + await page.getByTestId("group-rename-gateway").click(); + await page.getByTestId("group-rename-input").fill("Ingest"); + await page.keyboard.press("Enter"); + + await expect + .poll(() => mapContainers(page)) + .toEqual(["Ingest", "mailer", "Ungrouped"]); + expect(await railGroups(page)).toEqual(await mapContainers(page)); +}); + +test("an edge whose ends the user split across groups is still drawn", async ({ + page, +}) => { + /* Pull one member out of a detected system and the connector between the + halves is still real. Dropping it would make the map claim two systems + never touch, which is the one thing an edge is for. */ + await openGroupAxis(page); + const before = await page + .locator('[data-testid^="system-graph-edge-"]') + .count(); + expect(before).toBeGreaterThan(0); + + // `queue` leaves every group — the drop-on-Ungrouped gesture, applied + // through the rail's own delete of the group that holds it. + await page.getByTestId("group-delete-mailer").click(); + await expect.poll(() => mapContainers(page)).toEqual(["gateway", "Ungrouped"]); + + const after = await page + .locator('[data-testid^="system-graph-edge-"]') + .count(); + expect(after).toBe(before); + await expect(page.locator(".system-graph-edge.is-cross-group")).not.toHaveCount( + 0, + ); +}); From cbfb14f7901e728f4c123907db2baf39ecb42951 Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sun, 30 Aug 2026 10:14:42 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(harness):=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20identity=20over=20label,=20and=20a=20draw=20gate=20?= =?UTF-8?q?of=20its=20own=20[SAP-2983]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The `Ungrouped` bucket is carried by `isUngrouped` from the rail, not matched on the string "Ungrouped". Nothing stops a user naming a real system that, and the label lookup would have filed unresolved cards inside it and moved it to the end of the map — breaking the rail order this feature is about. - The map draws on `hasSettled` (the read finished, either way), not on `isReady` (the WRITE gate, which stays false forever after a failed read). Gated on the write gate, a read-only checkout left the map flat and unlabelled while the rail beside it named every system — the exact divergence this is meant to remove. - "Have I asked for this root yet" goes back to a per-surface ref. Shared, one bad response was permanent, and this committable file was never re-read after a branch switch or a hand edit. - The container label grows by font-size, not `transform: scale()`. A transform does not re-lay the line out, so below ~70% zoom a long group name drew past its own box and over its neighbour, where neither `max-width` nor the ellipsis could see it. `GROUP_HEADER` is sized for the largest line. - Drops the design-artifact and third-party names from comments and the changeset: this repo is public, and a changeset cannot be edited after publish. The rules they illustrated are stated directly instead. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91 --- .changeset/project-map-groups.md | 6 +- .../web/e2e/project-map-groups.spec.ts | 151 +++++++++++++++++- .../web/src/components/SystemGraphCanvas.tsx | 4 +- .../web/src/components/WorkspaceGraphView.tsx | 30 ++-- packages/harness/web/src/lib/api.ts | 12 ++ .../web/src/lib/system-graph-groups.test.ts | 46 ++++++ .../web/src/lib/system-graph-groups.ts | 15 +- .../web/src/lib/system-graph-layout.test.ts | 9 +- .../web/src/lib/system-graph-layout.ts | 67 +++++--- .../harness/web/src/lib/use-rail-groups.ts | 63 ++++++-- packages/harness/web/src/styles.css | 24 +-- 11 files changed, 357 insertions(+), 70 deletions(-) diff --git a/.changeset/project-map-groups.md b/.changeset/project-map-groups.md index de4099d08..6cbe617b7 100644 --- a/.changeset/project-map-groups.md +++ b/.changeset/project-map-groups.md @@ -7,9 +7,9 @@ endless column of agents. A project map used to draw every agent it contained as one flat set, ignoring the structure the rail was showing right beside it. Open a folder holding -several systems and you got a single vertical column — a nine-system, 76-agent -folder came out 9,700 pixels tall and one card wide, which no amount of zooming -out makes readable. +several systems and you got a single vertical column — a folder holding a few +dozen agents came out thousands of pixels tall and one card wide, which no +amount of zooming out makes readable. - **One labelled container per group.** The map now reads the same groups the rail does, so a system you named in the rail is a system you can see on the diff --git a/packages/harness/web/e2e/project-map-groups.spec.ts b/packages/harness/web/e2e/project-map-groups.spec.ts index 613ea161a..0a619bf43 100644 --- a/packages/harness/web/e2e/project-map-groups.spec.ts +++ b/packages/harness/web/e2e/project-map-groups.spec.ts @@ -35,11 +35,17 @@ const railGroups = (page: Page): Promise => .locator('[data-testid^="group-row-"] .tree-row-label') .allInnerTexts(); -/** Switch the rail to the Group axis and wait for it to be editable. */ -async function openGroupAxis(page: Page): Promise { +/** Switch the rail to the Group axis. */ +async function selectGroupAxis(page: Page): Promise { await page.getByTestId("history-trigger").click(); await page.getByTestId("filing-group-by").selectOption("group"); await page.keyboard.press("Escape"); +} + +/** …and wait for it to be EDITABLE: the create row appears only once the + * arrangement and the launch edges have both loaded. */ +async function openGroupAxis(page: Page): Promise { + await selectGroupAxis(page); await expect(page.getByTestId("group-create-polsia")).toBeVisible(); } @@ -189,3 +195,144 @@ test("an edge whose ends the user split across groups is still drawn", async ({ 0, ); }); + +test("a container's name stays inside its own box at every zoom", async ({ + page, +}) => { + /* The name counter-scales against the view zoom, because at the map's own + arrival zoom it renders under 4px tall. A `transform: scale()` would do + that WITHOUT re-laying the line out — the label's on-screen width would + then stay constant while its container's shrank, so below ~70% a long group + name draws past its own box and over its neighbour, invisible to any check + that measures the boxes alone. It grows by font-size instead, so the + ellipsis still applies. + + Asserted at the far end of the clamp, where a transform would be worst. */ + await page.getByTestId("system-graph-zoom-out").click({ clickCount: 8 }); + + const measured = await page.evaluate(() => { + const groups = [...document.querySelectorAll(".system-graph-group")]; + const cards = [...document.querySelectorAll(".system-graph-node")].map( + (el) => el.getBoundingClientRect(), + ); + const hits = (a: DOMRect, b: DOMRect) => + !(a.right <= b.left || b.right <= a.left || a.bottom <= b.top || b.bottom <= a.top); + return { + zoom: document.querySelector('[data-testid="system-graph-zoom-reset"]')! + .textContent, + labelHeight: Math.round( + groups[0]! + .querySelector(".system-graph-group-label")! + .getBoundingClientRect().height, + ), + escaping: groups.filter((group) => { + const outer = group.getBoundingClientRect(); + const label = group + .querySelector(".system-graph-group-label")! + .getBoundingClientRect(); + return ( + label.right > outer.right + 0.5 || label.bottom > outer.bottom + 0.5 + ); + }).length, + overCards: groups.filter((group) => { + const label = group + .querySelector(".system-graph-group-label")! + .getBoundingClientRect(); + return cards.some((card) => hits(label, card)); + }).length, + }; + }); + + // The fixture is only evidence while the label is actually being grown. + expect(Number.parseInt(measured.zoom!, 10)).toBeLessThan(70); + expect(measured.labelHeight).toBeGreaterThan(6); + expect(measured.escaping).toBe(0); + expect(measured.overCards).toBe(0); +}); + +test("a project whose arrangement cannot be READ still draws its groups", async ({ + page, +}) => { + /* The write gate and the draw gate are different questions. A read that fails + answers "nothing stored", which shows the DERIVED groups — the rail renders + those, because a group axis you cannot write to is still one you can look + at. Gating the map on the write gate instead would leave it flat and + unlabelled on a read-only checkout while the rail six inches away named + every system, which is the divergence this whole feature removes. */ + await page.goto("/?mockFixtures=deep"); + await expect(page.locator(".rail-workflows")).toBeVisible(); + await page.evaluate(() => { + (window as unknown as { __MOCK_RAIL_STATE_FAIL__?: boolean }).__MOCK_RAIL_STATE_FAIL__ = + true; + }); + await page.getByTestId("project-select-polsia").click(); + await expect(page.getByTestId("workspace-graph-view")).toBeVisible(); + + await expect + .poll(() => mapContainers(page)) + .toEqual(["gateway", "mailer", "Ungrouped"]); + + // The rail draws the same rows — read-only, which is why the map cannot be + // gated on the same signal: `group-create-polsia` is deliberately absent. + await selectGroupAxis(page); + await expect + .poll(() => railGroups(page)) + .toEqual(["gateway", "mailer", "Ungrouped"]); + await expect(page.getByTestId("group-create-polsia")).toHaveCount(0); + expect(await railGroups(page)).toEqual(await mapContainers(page)); +}); + +test("a read that failed is tried again when the map is reopened", async ({ + page, +}) => { + /* The arrangement cache is shared by both surfaces so an edit in one moves + the other. What must NOT be shared is "have I asked for this yet": a + module-level request latch would mean one bad response is permanent, and + that this committable file is never re-read after a branch switch or a hand + edit either. So the latch stays per surface, and a remount re-reads. */ + await page.goto("/?mockFixtures=deep"); + await expect(page.locator(".rail-workflows")).toBeVisible(); + await page.evaluate(() => { + // A stored arrangement, so a successful read is distinguishable from a + // failed one by more than timing. + window.localStorage.setItem( + "sapiom-mock-studio-rail:/Users/demo/polsia", + JSON.stringify({ + version: 1, + renames: {}, + groups: [ + { + id: "g_custom", + label: "Custom", + members: [ + "/Users/demo/polsia/services/gateway", + "/Users/demo/polsia/services/workers/queue", + ], + }, + ], + }), + ); + (window as unknown as { __MOCK_RAIL_STATE_FAIL__?: boolean }).__MOCK_RAIL_STATE_FAIL__ = + true; + }); + + await page.getByTestId("project-select-polsia").click(); + await expect(page.getByTestId("workspace-graph-view")).toBeVisible(); + // The read failed, so the map falls back to the DERIVED groups. + await expect + .poll(() => mapContainers(page)) + .toEqual(["gateway", "mailer", "Ungrouped"]); + + await page.evaluate(() => { + (window as unknown as { __MOCK_RAIL_STATE_FAIL__?: boolean }).__MOCK_RAIL_STATE_FAIL__ = + false; + }); + + // Drill into an agent and back out: the map remounts, and the remount reads. + await page.locator(".system-graph-node.is-navigable").first().click(); + await expect(page.getByTestId("workspace-graph-view")).toHaveCount(0); + await page.getByTestId("project-select-polsia").click(); + await expect(page.getByTestId("workspace-graph-view")).toBeVisible(); + + await expect.poll(() => mapContainers(page)).toEqual(["Custom", "Ungrouped"]); +}); diff --git a/packages/harness/web/src/components/SystemGraphCanvas.tsx b/packages/harness/web/src/components/SystemGraphCanvas.tsx index 20b5bb828..69f1ec4d8 100644 --- a/packages/harness/web/src/components/SystemGraphCanvas.tsx +++ b/packages/harness/web/src/components/SystemGraphCanvas.tsx @@ -328,12 +328,12 @@ export function SystemGraphCanvas({ > {/* Behind the connectors and the cards, so an edge that leaves its system reads as crossing the boundary rather than being clipped by - it — the shape the design reference draws. */} + it. */} {layout.groups.map((group) => (
workspaceScopes.find((scope) => scope.workspaceKey === workspaceKey) @@ -165,11 +169,17 @@ export function WorkspaceGraphView({ projectRoot !== null, ); const groups = useMemo(() => { - // `isReady` is BOTH halves — the stored arrangement and the launch edges. - // Drawing before either lands would put every agent in one `Ungrouped` - // container for a beat, and that is a real arrangement, not a placeholder: - // it would read as this project's answer and then silently rearrange. - if (!graph || projectRoot === null || !railGroups.isReady(projectRoot)) { + /* `hasSettled`, NOT `isReady`. Both need the launch edges and the stored + arrangement, but `isReady` is the WRITE gate and stays false forever on a + read that failed — so a map gated on it would fall back to an unlabelled + flat layout on a read-only checkout while the rail beside it kept showing + the systems by name. Settled means both surfaces have the same answer. + + Something has to gate it, though: drawing before the edges land would put + every agent in one `Ungrouped` container for a beat, and that is a real + arrangement rather than a placeholder — it would read as this project's + answer and then silently rearrange. */ + if (!graph || projectRoot === null || !railGroups.hasSettled(projectRoot)) { return undefined; } return systemGraphNodeGroups( diff --git a/packages/harness/web/src/lib/api.ts b/packages/harness/web/src/lib/api.ts index 03be8d981..9f4041a92 100644 --- a/packages/harness/web/src/lib/api.ts +++ b/packages/harness/web/src/lib/api.ts @@ -2155,6 +2155,18 @@ class MockApi implements HarnessApi { async getRailState(projectRoot: string): Promise { await delay(60); + // Test-only, mock mode only, matching __MOCK_SYSTEM_GRAPH_FAIL_ONCE__: a + // read-only checkout or a 5xx on this route is the one case where "safe to + // write" and "safe to draw" have different answers, and getting that wrong + // leaves the rail naming every system while the map shows an unlabelled + // blob. Reachable only by throwing the read. + if ( + typeof window !== "undefined" && + (window as unknown as { __MOCK_RAIL_STATE_FAIL__?: boolean }) + .__MOCK_RAIL_STATE_FAIL__ + ) { + throw new ApiError(500, "Rail state unreadable", "Rail state unreadable"); + } try { return window.localStorage.getItem(this.railStateKey(projectRoot)); } catch { diff --git a/packages/harness/web/src/lib/system-graph-groups.test.ts b/packages/harness/web/src/lib/system-graph-groups.test.ts index 5827aa088..0cc9a1ec7 100644 --- a/packages/harness/web/src/lib/system-graph-groups.test.ts +++ b/packages/harness/web/src/lib/system-graph-groups.test.ts @@ -217,4 +217,50 @@ describe("systemGraphNodeGroups", () => { expect(drawn[0]!.label).toBe("Everything"); expect(drawn[0]!.nodeIds).toHaveLength(NODES.length); }); + + it("does not mistake a group the user NAMED `Ungrouped` for the bucket", () => { + /* `renameGroup` only trims — nothing stops a user calling a real system + `Ungrouped`. Recognising the bucket by its LABEL would then file every + card that failed the navigation join inside that system, and move it to + the end of the map, breaking the rail order this feature is about. + `isUngrouped` is carried from the rail instead. */ + const collision: RailState = { + version: 1, + renames: {}, + groups: [ + { + id: "g_named", + label: "Ungrouped", + members: [`${ROOT}/gateway`, `${ROOT}/queue`], + }, + { id: "g_second", label: "Second", members: [`${ROOT}/worker`] }, + ], + }; + const rows = railRows(collision); + // The rail itself draws two rows called `Ungrouped`: the user's, and the + // real bucket. That is the shape the map has to survive. + expect(rows.map((row) => [row.label, row.isUngrouped])).toEqual([ + ["Ungrouped", false], + ["Second", false], + ["Ungrouped", true], + ]); + + const drawn = systemGraphNodeGroups( + NODES, + rows, + // `sender` drops out of the navigation join, so its card is unclaimed. + navigationFor(WORKFLOWS.filter((workflow) => workflow.name !== "sender")), + ); + expect(drawn.map((c) => [c.label, c.isUngrouped, c.nodeIds])).toEqual([ + ["Ungrouped", false, ["agent:gateway", "agent:queue"]], + ["Second", false, ["agent:worker"]], + [ + "Ungrouped", + true, + ["agent:loner", "agent:mailer", "agent:sender"], + ], + ]); + // The user's group keeps its position and its exact membership. + expect(drawn[0]!.nodeIds).not.toContain("agent:sender"); + }); }); diff --git a/packages/harness/web/src/lib/system-graph-groups.ts b/packages/harness/web/src/lib/system-graph-groups.ts index 550f3fc69..563d23c00 100644 --- a/packages/harness/web/src/lib/system-graph-groups.ts +++ b/packages/harness/web/src/lib/system-graph-groups.ts @@ -63,7 +63,15 @@ export function systemGraphNodeGroups( // A group whose members are all agents this graph does not have would draw // an empty box with a name on it — chrome around nothing. if (nodeIds.length > 0) { - containers.push({ id: group.id, label: group.label, nodeIds }); + containers.push({ + id: group.id, + label: group.label, + nodeIds, + // Carried from the rail, never inferred from the label: a user may name + // a group of their own "Ungrouped", and that group is a real system, not + // the bucket for what nothing claims. + isUngrouped: group.isUngrouped, + }); } } @@ -75,14 +83,13 @@ export function systemGraphNodeGroups( .map((node) => node.id) .filter((nodeId) => !claimed.has(nodeId)); if (rest.length > 0) { - const index = containers.findIndex( - (container) => container.label === SYSTEM_GRAPH_UNGROUPED_LABEL, - ); + const index = containers.findIndex((container) => container.isUngrouped); if (index === -1) { containers.push({ id: UNGROUPED_ID, label: SYSTEM_GRAPH_UNGROUPED_LABEL, nodeIds: rest, + isUngrouped: true, }); } else { // Re-appended rather than edited in place: Ungrouped is last in the rail diff --git a/packages/harness/web/src/lib/system-graph-layout.test.ts b/packages/harness/web/src/lib/system-graph-layout.test.ts index cb3fed61b..32122e15e 100644 --- a/packages/harness/web/src/lib/system-graph-layout.test.ts +++ b/packages/harness/web/src/lib/system-graph-layout.test.ts @@ -294,14 +294,19 @@ describe("layoutSystemGraph", () => { * in geometry, because "it looks better" is not a rule anything can hold. * * Geometry only. That containers carry the RAIL's labels is - * `system-graph-groups.test.ts`; that they reach the DOM is `project-map.spec.ts`. + * `system-graph-groups.test.ts`; that they reach the DOM is `project-map-groups.spec.ts`. */ describe("layoutSystemGraph with groups", () => { const group = ( id: string, label: string, nodeIds: string[], - ): SystemGraphNodeGroup => ({ id, label, nodeIds }); + ): SystemGraphNodeGroup => ({ + id, + label, + nodeIds, + isUngrouped: label === "Ungrouped", + }); /** The box a container claims, by label. */ function boxOf(layout: SystemGraphLayout, label: string) { diff --git a/packages/harness/web/src/lib/system-graph-layout.ts b/packages/harness/web/src/lib/system-graph-layout.ts index 3ccc6b5fa..3774a14ce 100644 --- a/packages/harness/web/src/lib/system-graph-layout.ts +++ b/packages/harness/web/src/lib/system-graph-layout.ts @@ -30,8 +30,16 @@ const LABEL_HEIGHT = 16; * system when it never did. */ const GROUP_PADDING = 48; -/** The label strip along the top of a container, above its content. */ -const GROUP_HEADER = 26; +/** + * The label strip along the top of a container, above its content. + * + * Sized for the BIGGEST line the label can produce, not for its natural one: + * `.system-graph-group-label` grows its type up to 4x as the view zooms out, so + * at the clamp it is `4 * --type-meta * 1.2` plus the container's own top + * padding. Sized for the natural line instead, the name of a system would sit + * across the first row of its cards at exactly the zoom it becomes readable. + */ +const GROUP_HEADER = 64; /** Between containers. Wider than `COMPONENT_GAP` so the boundary between two * systems reads as a bigger break than the boundary between two components of * one system. */ @@ -47,12 +55,15 @@ const GROUP_GAP = 80; const SHELF_ASPECT = 2.2; /** - * The label the Group axis gives agents no group claims (`agent-groups.ts`). + * The label for the bucket this module synthesizes when a node reaches it that + * no container claimed — see `toRegions`. It matches the rail's own spelling + * (`agent-groups.ts`), repeated rather than imported because the dependency + * runs the other way: `system-graph-groups.ts` maps the rail's model onto this + * one. The e2e spec asserts the map's labels against the RAIL's rows, so the + * two spellings cannot drift apart unnoticed. * - * Repeated here rather than imported because the dependency runs the other way: - * `system-graph-groups.ts` maps the rail's model onto this one. The e2e spec - * asserts the map's container labels against the RAIL's rows, so the two - * spellings cannot drift apart unnoticed. + * It is a LABEL, never an identity test: `isUngrouped` is how the bucket is + * recognised. */ export const SYSTEM_GRAPH_UNGROUPED_LABEL = "Ungrouped"; @@ -74,6 +85,14 @@ export interface SystemGraphNodeGroup { id: string; label: string; nodeIds: readonly string[]; + /** + * The bucket for agents no group claims, carried by IDENTITY rather than + * inferred from the label. Nothing stops a user creating or renaming a group + * to "Ungrouped" in the rail, and matching on the string would then file + * unresolved cards inside that named system and move it to the end of the + * map — breaking the rail-order agreement this whole feature is about. + */ + isUngrouped: boolean; } /** A drawn container: the box, and the label that names it. */ @@ -463,8 +482,6 @@ function placeRegionNodes( componentBoxes: Map; componentByNode: Map; strongByNode: Map; - width: number; - height: number; } { const componentByNode = new Map(); const strongByNode = new Map(); @@ -531,14 +548,7 @@ function placeRegionNodes( }); }); nodes.sort((left, right) => compareIds(left.id, right.id)); - return { - nodes, - componentBoxes, - componentByNode, - strongByNode, - width: packed.width, - height: packed.height, - }; + return { nodes, componentBoxes, componentByNode, strongByNode }; } function spreadPortOffsets( @@ -847,9 +857,9 @@ const CROSS_GROUP_LANE = 6; * Routed after the containers are packed, in global coordinates, and * deliberately NOT confined to a gutter: a corridor wide enough to skirt every * container between two ends would dominate the drawing for the rarest edge on - * it. They pass BEHIND cards (the edge layer sits under the node layer) and the - * design reference draws them the same way — the connector out of "THE LOOP" to - * TikTok crosses its border. + * it. They pass BEHIND cards, because the edge layer sits under the node layer + * — a connector between two containers is drawn CROSSING the border rather than + * clipped by it, which is what makes it read as a link out of the system. */ function routeCrossGroupEdges( visible: readonly VisibleSystemGraphEdge[], @@ -971,6 +981,7 @@ interface Region { * where nothing is drawn around the content. */ label: string | null; nodeIds: string[]; + isUngrouped: boolean; } /** @@ -988,7 +999,9 @@ function toRegions( groups: readonly SystemGraphNodeGroup[] | undefined, ): Region[] { const order = graph.nodes.map((node) => node.id); - if (!groups) return [{ id: "", label: null, nodeIds: order }]; + if (!groups) { + return [{ id: "", label: null, nodeIds: order, isUngrouped: false }]; + } const known = new Set(order); const claimed = new Set(); const regions: Region[] = []; @@ -1005,7 +1018,12 @@ function toRegions( } // A group whose every member resolved to nothing is chrome around nothing. if (nodeIds.length > 0) { - regions.push({ id: group.id, label: group.label, nodeIds }); + regions.push({ + id: group.id, + label: group.label, + nodeIds, + isUngrouped: group.isUngrouped, + }); } } const leftover = order.filter((id) => !claimed.has(id)); @@ -1014,15 +1032,14 @@ function toRegions( // backstop rather than a path — and deliberately not a throw. A node that // silently disappears from the map is worse than a node filed in the bucket // that means "nothing claims this". - const bucket = regions.find( - (region) => region.label === SYSTEM_GRAPH_UNGROUPED_LABEL, - ); + const bucket = regions.find((region) => region.isUngrouped); if (bucket) bucket.nodeIds.push(...leftover); else { regions.push({ id: "group:unclaimed", label: SYSTEM_GRAPH_UNGROUPED_LABEL, nodeIds: leftover, + isUngrouped: true, }); } } diff --git a/packages/harness/web/src/lib/use-rail-groups.ts b/packages/harness/web/src/lib/use-rail-groups.ts index 3450756fc..29dde29d2 100644 --- a/packages/harness/web/src/lib/use-rail-groups.ts +++ b/packages/harness/web/src/lib/use-rail-groups.ts @@ -38,17 +38,23 @@ const ROOTS_SEP = "\n"; * reason the file is per project: there is one answer, and both surfaces read * it. * - * `requested` lives here too, so a second surface mounting does not re-issue a - * read the first one already has in flight. + * What deliberately does NOT live here is "have I asked for this yet". That + * stays per hook, so mounting a surface re-reads the file — it is a committable + * file that a branch switch or a hand edit can change under the app, and a + * module-level request latch would mean the page never looked again, and never + * retried a read that failed. Two surfaces mounting therefore issue two GETs of + * the same file, which is a cheap read and idempotent. */ interface RailGroupsStore { /** A property of the INSTALL, not of a root, so one read serves every * project. Null until it lands. */ edges: LaunchEdge[] | null; - edgesRequested: boolean; states: Map; + /** Roots whose file READ SUCCEEDED. Gates writes. */ loaded: Set; - requested: Set; + /** Roots whose read has SETTLED, successfully or not. Gates drawing. + * See `hasSettled` — the two must stay different sets. */ + settled: Set; listeners: Set<() => void>; /** Bumped on every mutation. `useSyncExternalStore` compares it by identity, * and every accessor below takes it as a dependency — a Map mutated in place @@ -58,10 +64,9 @@ interface RailGroupsStore { const store: RailGroupsStore = { edges: null, - edgesRequested: false, states: new Map(), loaded: new Set(), - requested: new Set(), + settled: new Set(), listeners: new Set(), version: 0, }; @@ -112,6 +117,21 @@ export interface RailGroups { * a real arrangement and would read as the answer. */ isReady: (root: string) => boolean; + /** + * Whether this root's arrangement is safe to DRAW: the edges landed and the + * read has settled — successfully or not. + * + * Deliberately weaker than `isReady`, and the difference is a real failure. + * A read that fails is answered as "nothing stored", which shows the DERIVED + * groups; the rail renders those, because a group axis you cannot write to is + * still a group axis you can look at. `isReady` stays false for that root + * forever, on purpose, so nothing can be edited into a file we were unable to + * read. A map gated on `isReady` would therefore fall back to an unlabelled + * flat layout on a read-only checkout or a 5xx, while the rail six inches away + * showed the systems by name — which is the exact divergence this feature + * exists to remove. + */ + hasSettled: (root: string) => boolean; /** The stored state, for the reset control's copy ("Discards 3 groups"). */ stateFor: (root: string) => RailState; /** Apply a pure operation to one root's arrangement and persist the result. @@ -157,6 +177,12 @@ export function useRailGroups( ): RailGroups { const version = useSyncExternalStore(subscribe, snapshot, snapshot); + /** Roots and edges this INSTANCE has already asked for. Refs, not state, so + * they update synchronously and a re-render mid-flight cannot start a second + * read; per instance, so a remount re-reads. */ + const requested = useRef(new Set()); + const edgesRequested = useRef(false); + // The registry changes as agents are scanned, and the load effect below must // not re-run for that — it would re-read every project's file. It reads the // latest registry through a ref instead of depending on it. @@ -167,8 +193,8 @@ export function useRailGroups( // serves every project. Fetched only once the axis is in use: it greps every // registered agent's sources, and the Project axis has no use for the answer. useEffect(() => { - if (!enabled || store.edgesRequested) return; - store.edgesRequested = true; + if (!enabled || edgesRequested.current) return; + edgesRequested.current = true; void api .listLaunchEdges() .then((next) => { @@ -189,8 +215,8 @@ export function useRailGroups( useEffect(() => { if (!enabled) return; for (const root of rootsKey.split(ROOTS_SEP).filter(Boolean)) { - if (store.requested.has(root)) continue; - store.requested.add(root); + if (requested.current.has(root)) continue; + requested.current.add(root); void api .getRailState(root) .then((raw) => { @@ -200,14 +226,20 @@ export function useRailGroups( // next edit rewrites. store.states.set(root, readRailState(raw, workflowsRef.current)); store.loaded.add(root); + store.settled.add(root); notify(); }) .catch(() => { // An older server with no such route, or an unreadable project. Both // read as "nothing stored", which shows the derived groups — but the // root stays NOT loaded, so nothing can be edited into a file we were - // unable to read. + // unable to read. It IS settled: both surfaces now have the same + // answer to draw, which is the point of the two sets being different. store.states.set(root, EMPTY_RAIL_STATE); + store.settled.add(root); + // Forget the request so a later mount tries the read again; a + // permanent latch would make one bad response permanent. + requested.current.delete(root); notify(); }); } @@ -225,6 +257,11 @@ export function useRailGroups( [version], ); + const hasSettled = useCallback( + (root: string): boolean => store.edges !== null && store.settled.has(canonicalRoot(root)), + [version], + ); + /** * Writes in flight per root, chained. * @@ -297,7 +334,7 @@ export function useRailGroups( ); return useMemo( - () => ({ groupsFor, agentsIn, isReady, stateFor, edit, reset }), - [groupsFor, agentsIn, isReady, stateFor, edit, reset], + () => ({ groupsFor, agentsIn, isReady, hasSettled, stateFor, edit, reset }), + [groupsFor, agentsIn, isReady, hasSettled, stateFor, edit, reset], ); } diff --git a/packages/harness/web/src/styles.css b/packages/harness/web/src/styles.css index 6d998fbe5..8be6d61c5 100644 --- a/packages/harness/web/src/styles.css +++ b/packages/harness/web/src/styles.css @@ -4247,9 +4247,8 @@ button.rail-footer-card:hover { pointer-events: none; } -/* A named, bounded container around one system's cards — the shape the - fourfold map reference draws around "THE LOOP". Behind everything (the edge - svg and the cards are later siblings), and NOT interactive: a group is +/* A named, bounded container around one system's cards. Behind everything (the + edge svg and the cards are later siblings), and NOT interactive: a group is edited in the rail, which is the surface that owns the arrangement. */ .system-graph-group { position: absolute; @@ -4264,9 +4263,9 @@ button.rail-footer-card:hover { /* Counter-scaled against the view zoom. A container's NAME is the whole thing the container adds, and the map's own - arrival zoom on a real project is around 20% — measured on a 9-system, - 76-agent tree: the label rendered 3.65px tall, and 2.96px after Fit. That is - not a label. Growing it as the view shrinks keeps the systems named at + arrival zoom on a project with dozens of agents is around 20% — measured + there, the label rendered 3.65px tall, and 2.96px after Fit. That is not a + label. Growing it as the view shrinks keeps the systems named at exactly the altitude you zoom out to read them from, and the clamp stops there: at 70% and above the cards are legible on their own and the label is 1:1. transform-origin pins it to the corner it already sits in. */ @@ -4274,12 +4273,19 @@ button.rail-footer-card:hover { display: block; max-width: 100%; overflow: hidden; - transform: scale(clamp(1, calc(0.7 / var(--system-graph-zoom, 1)), 4)); - transform-origin: 0 0; color: var(--text-faint); font-family: var(--font-mono); - font-size: var(--type-meta); + /* FONT-SIZE, not `transform: scale()`. A transform does not affect layout, so + a scaled label's on-screen width stays constant as the view shrinks while + its container's does not — below ~70% a long group name draws straight past + its own box and over its neighbour, and neither `max-width` nor the ellipsis + can see it happen. Growing the type re-lays the line out, so both still + hold. `GROUP_HEADER` is sized for the largest line this can produce. */ + font-size: calc( + var(--type-meta) * clamp(1, calc(0.7 / var(--system-graph-zoom, 1)), 4) + ); letter-spacing: 0.08em; + line-height: 1.2; text-overflow: ellipsis; text-transform: uppercase; white-space: nowrap; From 4d152c0b2e23c07bcb97a3508cfbd5ef00e8f16a Mon Sep 17 00:00:00 2001 From: David Witwer Date: Sun, 30 Aug 2026 10:53:40 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(harness):=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20no=20lost=20edit,=20and=20one=20whole-tree=20grep?= =?UTF-8?q?=20per=20page=20[SAP-2983]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings fall out of round 1's split between a shared arrangement and a per-surface request latch. - A root this page has WRITTEN to is never re-read. Opening the map issued its own GET, which raced any PUT still in flight from an edit a moment earlier: served first, it replaced the optimistic arrangement with the pre-edit file, the rail visibly reverted, and the next edit then materialized from the reverted state and persisted it — losing the edit on disk too. - The launch-edge latch goes back to module scope. That grep walks every registered agent's sources and its answer is install-wide, so a per-surface latch re-scanned the whole tree on every drill-in. Measured on a 76-agent tree: 4 greps across 3 drill-in/out cycles, now 1. Released on failure so a later mount still retries. The per-root file reads stay per surface, which is what round 1's finding was about. - `toRegions` now has its own test for recognising the bucket by identity rather than by the label "Ungrouped"; only the mapper half was pinned. - Drops a comment line describing a `transform-origin` that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91 --- .../web/e2e/project-map-groups.spec.ts | 57 +++++++++++++++++++ .../web/src/lib/system-graph-groups.ts | 3 +- .../web/src/lib/system-graph-layout.test.ts | 18 ++++++ .../harness/web/src/lib/use-rail-groups.ts | 40 ++++++++++--- packages/harness/web/src/styles.css | 2 +- 5 files changed, 111 insertions(+), 9 deletions(-) diff --git a/packages/harness/web/e2e/project-map-groups.spec.ts b/packages/harness/web/e2e/project-map-groups.spec.ts index 0a619bf43..f6af3e26c 100644 --- a/packages/harness/web/e2e/project-map-groups.spec.ts +++ b/packages/harness/web/e2e/project-map-groups.spec.ts @@ -336,3 +336,60 @@ test("a read that failed is tried again when the map is reopened", async ({ await expect.poll(() => mapContainers(page)).toEqual(["Custom", "Ungrouped"]); }); + +test("opening the map cannot undo an edit the rail just made", async ({ + page, +}) => { + /* The arrangement is shared across surfaces but the request latch is per + surface, so opening the map issues its OWN read of the file — and that read + races any write still in flight from an edit a moment earlier. Served + first, it would replace the optimistic arrangement with the pre-edit file: + the rail visibly reverts, and the next edit then materializes from the + reverted state and persists it, losing the edit on disk as well as on + screen. A root this page has written to is never re-read. + + The write is held open here so the race is deterministic rather than a + matter of who happens to win. */ + await page.goto("/?mockFixtures=deep"); + await expect(page.locator(".rail-workflows")).toBeVisible(); + await openGroupAxis(page); + + let releaseWrite = (): void => {}; + const writeHeld = new Promise((resolve) => { + releaseWrite = resolve; + }); + await page.exposeFunction("__holdRailWrite", () => writeHeld); + await page.evaluate(() => { + const store = window.localStorage; + const original = store.setItem.bind(store); + store.setItem = (key: string, value: string) => { + if (key.startsWith("sapiom-mock-studio-rail:")) { + void (window as unknown as { __holdRailWrite: () => Promise }) + .__holdRailWrite() + .then(() => original(key, value)); + return; + } + original(key, value); + }; + }); + + // The edit: optimistic in memory, its write parked. + await page.getByTestId("group-rename-gateway").click(); + await page.getByTestId("group-rename-input").fill("Ingest"); + await page.keyboard.press("Enter"); + await expect + .poll(() => railGroups(page)) + .toEqual(["Ingest", "mailer", "Ungrouped"]); + + // Opening the map is what issues the second read. + await page.getByTestId("project-select-polsia").click(); + await expect(page.getByTestId("workspace-graph-view")).toBeVisible(); + await expect + .poll(() => mapContainers(page)) + .toEqual(["Ingest", "mailer", "Ungrouped"]); + + releaseWrite(); + // Still the edit, on both surfaces, after the write lands. + await expect.poll(() => railGroups(page)).toEqual(["Ingest", "mailer", "Ungrouped"]); + expect(await mapContainers(page)).toEqual(["Ingest", "mailer", "Ungrouped"]); +}); diff --git a/packages/harness/web/src/lib/system-graph-groups.ts b/packages/harness/web/src/lib/system-graph-groups.ts index 563d23c00..5a432e2d0 100644 --- a/packages/harness/web/src/lib/system-graph-groups.ts +++ b/packages/harness/web/src/lib/system-graph-groups.ts @@ -12,7 +12,8 @@ import { * * The map used to draw every agent a project contains as one flat set, ignoring * the sub-structure the rail was showing six inches to its left: one root - * holding nine systems and 76 agents came out as a single ~70-node column. The + * holding several systems and a few dozen agents came out as a single, endless + * column of unconnected nodes, thousands of pixels tall and one card wide. The * mechanism to fix that already existed — `lib/agent-groups.ts` derives groups * from launch edges, lets the user edit them, and persists the arrangement to a * committable `.sapiom/studio-rail.json`. The map simply never read it. diff --git a/packages/harness/web/src/lib/system-graph-layout.test.ts b/packages/harness/web/src/lib/system-graph-layout.test.ts index 32122e15e..b192fa117 100644 --- a/packages/harness/web/src/lib/system-graph-layout.test.ts +++ b/packages/harness/web/src/lib/system-graph-layout.test.ts @@ -489,6 +489,24 @@ describe("layoutSystemGraph with groups", () => { expect(boxOf(layout, "Ungrouped").nodeCount).toBe(40); }); + it("does not file an unclaimed node into a group merely NAMED Ungrouped", () => { + /* The layout half of the same identity rule the mapper carries: the bucket + is `isUngrouped`, never the string. A user may name a real system + "Ungrouped", and matching on the label would drop the cards nothing + claimed inside it and move it to the end of the map. */ + const layout = layoutSystemGraph(graph(["a", "b", "orphan"], []), [ + { id: "g:named", label: "Ungrouped", nodeIds: ["a", "b"], isUngrouped: false }, + ]); + expect(layout.groups.map((candidate) => candidate.nodeCount)).toEqual([2, 1]); + expect(layout.groups[0]!.id).toBe("g:named"); + expect(byId(layout, "a").groupId).toBe("g:named"); + expect(byId(layout, "b").groupId).toBe("g:named"); + // The synthesized bucket is a SECOND box, after the user's group. + expect(layout.groups[1]!.label).toBe("Ungrouped"); + expect(layout.groups[1]!.id).not.toBe("g:named"); + expect(byId(layout, "orphan").groupId).toBe(layout.groups[1]!.id); + }); + it("still draws a node no group claimed", () => { // The caller hands over an exhaustive partition, so this is a backstop — // and it is deliberately not a throw. A card that silently disappears is diff --git a/packages/harness/web/src/lib/use-rail-groups.ts b/packages/harness/web/src/lib/use-rail-groups.ts index 29dde29d2..dde29aedc 100644 --- a/packages/harness/web/src/lib/use-rail-groups.ts +++ b/packages/harness/web/src/lib/use-rail-groups.ts @@ -92,9 +92,15 @@ const snapshot = (): number => store.version; const canonicalRoot = (root: string): string => root.replace(/\\/g, "/").replace(/(.)\/+$/, "$1"); -/** Writes in flight, per canonical root. See `commit` below. */ +/** Writes in flight, per canonical root. See `commit` below. Its KEYS double as + * "this page has edited that root", which is what stops a later mount's read + * from overwriting an edit — see the load effect. */ const writeChain = new Map>(); +/** Whether the install-wide launch-edge grep has been asked for. Module scope + * on purpose; see the effect that reads it. */ +let edgesRequested = false; + export interface RailGroups { /** The rows to render for one project root: the stored groups if the user has * any, the derived ones until then, `Ungrouped` last either way. */ @@ -177,11 +183,11 @@ export function useRailGroups( ): RailGroups { const version = useSyncExternalStore(subscribe, snapshot, snapshot); - /** Roots and edges this INSTANCE has already asked for. Refs, not state, so - * they update synchronously and a re-render mid-flight cannot start a second - * read; per instance, so a remount re-reads. */ + /** Roots this INSTANCE has already asked for. A ref, not state, so it updates + * synchronously and a re-render mid-flight cannot start a second read; per + * instance, so a remount re-reads the file. (The launch edges are latched at + * module scope instead — see `edgesRequested`.) */ const requested = useRef(new Set()); - const edgesRequested = useRef(false); // The registry changes as agents are scanned, and the load effect below must // not re-run for that — it would re-read every project's file. It reads the @@ -192,9 +198,15 @@ export function useRailGroups( // Launch edges are a property of the INSTALL, not of a root, so one read // serves every project. Fetched only once the axis is in use: it greps every // registered agent's sources, and the Project axis has no use for the answer. + // + // Latched at MODULE scope, unlike the per-root reads below — and the + // difference is the cost. That grep walks every registered agent's sources, + // the answer is install-wide, and the map unmounts on every drill-in, so a + // per-instance latch would scan the whole tree again each time someone + // clicked an agent and came back. There is nothing per-root to re-read here. useEffect(() => { - if (!enabled || edgesRequested.current) return; - edgesRequested.current = true; + if (!enabled || edgesRequested) return; + edgesRequested = true; void api .listLaunchEdges() .then((next) => { @@ -207,6 +219,9 @@ export function useRailGroups( // as an empty ARRAY rather than left null so the axis becomes editable — // hand-grouping is the whole point when detection finds nothing. store.edges = []; + // Released, so a later mount tries once more: this one is a request + // that failed, not an install with nothing in it. + edgesRequested = false; notify(); }); }, [enabled]); @@ -216,6 +231,17 @@ export function useRailGroups( if (!enabled) return; for (const root of rootsKey.split(ROOTS_SEP).filter(Boolean)) { if (requested.current.has(root)) continue; + /* NEVER re-read a root this page has already WRITTEN to. + + The arrangement is shared across surfaces but the request latch is per + surface, so opening the map issues its own GET — and that GET races any + PUT still in flight. Served first, its result would replace the + optimistic state with the pre-edit file: the rail visibly reverts, and + the next edit then materializes from the reverted state and persists + it, losing the drag on disk as well as on screen. Once this page has + written a root, the in-memory arrangement IS the newer answer and a + read can only tell us something older. */ + if (writeChain.has(root)) continue; requested.current.add(root); void api .getRailState(root) diff --git a/packages/harness/web/src/styles.css b/packages/harness/web/src/styles.css index 8be6d61c5..8a5c0e44c 100644 --- a/packages/harness/web/src/styles.css +++ b/packages/harness/web/src/styles.css @@ -4268,7 +4268,7 @@ button.rail-footer-card:hover { label. Growing it as the view shrinks keeps the systems named at exactly the altitude you zoom out to read them from, and the clamp stops there: at 70% and above the cards are legible on their own and the label is - 1:1. transform-origin pins it to the corner it already sits in. */ + 1:1. */ .system-graph-group-label { display: block; max-width: 100%;