src/diagram/drawio.ts

@@ -26,10 +26,15 @@
  * that silently disappears is worse than a plain one that does not.
  */
 
-import type { Diagram, DiagramMachine } from "./model.ts";
+import type { Diagram, DiagramMachine, DiagramSegment } from "./model.ts";
 
 const LANE_MIN_HEIGHT = 170;
 const LANE_EMPTY_HEIGHT = 62;
+/** Depth becomes indentation, which is how "behind" is shown without drawing a line. */
+const INDENT = 40;
+/** A gap holding nothing needs no room; one between top-level groups needs a little. */
+const TIGHT_GAP = 24;
+const GROUP_GAP = 50;
 // Wide enough that a gateway placed on a boundary sits BETWEEN the lanes rather than on
 // top of one — an earlier value let a router's box cover the lane's own name and ranges.
 const LANE_GAP = 110;
@@ -158,10 +163,41 @@ export function toDrawio(diagram: Diagram): string {
   const cells: Cell[] = [];
   const edges: { id: string; source: string; target: string }[] = [];
 
-  // Lanes, ordered by depth: public first, then each level of private network below it.
-  const lanes = [...diagram.segments].sort(
-    (a, b) => a.depth - b.depth || a.name.localeCompare(b.name),
-  );
+  /**
+   * Order: each public network, then everything behind it, depth first.
+   *
+   * A single stack sorted by depth put a private network far from the public one it sits
+   * behind, so a gateway's link to the outside ran the height of the picture and crossed
+   * networks it had nothing to do with — two such links overlapped and read as one wire.
+   * Grouping makes every gateway adjacent to both lanes it joins, and every link short.
+   */
+  const byName = new Map(diagram.segments.map((s) => [s.name, s]));
+  const childrenOf = new Map<string, string[]>();
+  for (const segment of diagram.segments) {
+    if (!segment.behind) continue;
+    childrenOf.set(segment.behind, [...(childrenOf.get(segment.behind) ?? []), segment.name]);
+  }
+  const lanes: DiagramSegment[] = [];
+  const placed = new Set<string>();
+  const walk = (name: string): void => {
+    const segment = byName.get(name);
+    if (!segment || placed.has(name)) return;
+    placed.add(name);
+    lanes.push(segment);
+    for (const child of [...(childrenOf.get(name) ?? [])].sort()) walk(child);
+  };
+  // By name, not by the order the source happened to yield them. The hypervisor cannot know
+  // declaration order, so ordering by it would give the two pictures different shapes and
+  // make the comparison they exist for impossible to read.
+  const byNameOrder = [...diagram.segments].sort((a, b) => a.name.localeCompare(b.name));
+  for (const segment of byNameOrder) if (segment.kind === "public") walk(segment.name);
+  for (const segment of byNameOrder) walk(segment.name);   // isolated ones
+
+  const laneX = (segment: DiagramSegment) => LANE_X + segment.depth * INDENT;
+  const laneW = (segment: DiagramSegment) => LANE_WIDTH - segment.depth * INDENT;
+  const slotsIn = (segment: DiagramSegment) =>
+    Math.max(1, Math.floor((laneW(segment) - 60) / SLOT_WIDTH));
+
   // A lane is sized to what it holds. Fixed heights meant a lane with enough machines to
   // wrap onto a second row drew that row outside the box it was supposed to be inside.
   const occupants = new Map<string, number>();
@@ -173,8 +209,8 @@ export function toDrawio(diagram: Diagram): string {
     const only = machine.attachments[0]?.segment;
     if (only) occupants.set(only, (occupants.get(only) ?? 0) + 1);
   }
-  const laneHeight = (name: string): number => {
-    const rows = Math.ceil((occupants.get(name) ?? 0) / SLOTS_PER_ROW);
+  const laneHeight = (segment: DiagramSegment): number => {
+    const rows = Math.ceil((occupants.get(segment.name) ?? 0) / slotsIn(segment));
     // A segment whose only occupants are the gateways in the gaps beside it holds nothing
     // itself, so it collapses to its own name and ranges. Left at full height it padded a
     // layered scenario with empty boxes and pushed the interesting rows apart.
@@ -182,16 +218,46 @@ export function toDrawio(diagram: Diagram): string {
     return Math.max(LANE_MIN_HEIGHT, 45 + rows * (NODE_HEIGHT + 45));
   };
 
+  /**
+   * Which lane each gateway is drawn above — decided ONCE, by position in the lane order,
+   * and used both to size the gap and to place the box.
+   *
+   * Two rules deciding this separately is what put a gateway inside an unrelated network:
+   * the gap was reserved above one sibling while the box was drawn above the other, so it
+   * overflowed into the lane above and landed on top of a machine.
+   */
+  const laneIndex = new Map(lanes.map((segment, index) => [segment.name, index]));
+  const servedTop = new Map<DiagramMachine, string>();
+  for (const machine of diagram.machines) {
+    if (machine.kind === "transit" || machine.attachments.length < 2) continue;
+    const served = machine.attachments
+      .slice(1)
+      .map((a) => a.segment)
+      .filter((n) => laneIndex.has(n));
+    if (served.length === 0) continue;
+    servedTop.set(
+      machine,
+      served.reduce((best, n) => ((laneIndex.get(n) ?? 0) < (laneIndex.get(best) ?? 0) ? n : best), served[0]!),
+    );
+  }
+  const gatewayAbove = new Set(servedTop.values());
+
+  const transit = diagram.machines.find((m) => m.kind === "transit");
   const laneY = new Map<string, number>();
   const laneH = new Map<string, number>();
-  let cursor = 80;
+  const laneOf = new Map<string, DiagramSegment>();
+  let cursor = transit ? 80 + NODE_HEIGHT + GROUP_GAP : 80;
 
-  lanes.forEach((segment) => {
+  lanes.forEach((segment, index) => {
     const y = cursor;
-    const h = laneHeight(segment.name);
-    cursor = y + h + LANE_GAP;
+    const h = laneHeight(segment);
     laneY.set(segment.name, y);
     laneH.set(segment.name, h);
+    laneOf.set(segment.name, segment);
+    const next = lanes[index + 1];
+    cursor =
+      y + h +
+      (!next ? 0 : gatewayAbove.has(next.name) ? LANE_GAP : next.depth === 0 ? GROUP_GAP : TIGHT_GAP);
     const facts = [
       segment.cidr.join("  "),
       segment.mtu ? `MTU ${segment.mtu}` : "",
@@ -201,9 +267,9 @@ export function toDrawio(diagram: Diagram): string {
       id: `lane-${segment.name}`,
       value: `<b>${segment.name}</b><br/><font style="font-size:10px">${facts.join(" · ")}</font>`,
       style: segment.kind === "public" ? STYLE.publicLane : STYLE.privateLane,
-      x: LANE_X,
+      x: laneX(segment),
       y,
-      w: LANE_WIDTH,
+      w: laneW(segment),
       h,
     });
   });
@@ -226,43 +292,34 @@ export function toDrawio(diagram: Diagram): string {
     let x: number;
     let y = 80;
 
-    if (machine.attachments.length === 0) {
+    if (machine.kind === "transit") {
+      // Transit is not on a boundary — it reaches every public network at once, so a line
+      // to each would cross everything between. Stated once, at the top, where it applies.
+      x = LANE_X;
+      y = 80;
+    } else if (machine.attachments.length === 0) {
       // Detached: parked to the side, because it genuinely is nowhere.
       x = LANE_X + LANE_WIDTH + 60;
       y = 80 + detachedSlot * (NODE_HEIGHT + 50);
       detachedSlot++;
+    } else if (machine.attachments.filter((a) => laneY.has(a.segment)).length > 1) {
+      // A gateway sits in the gap directly above the lane it SERVES, indented to that lane,
+      // so it reads as the door into it rather than a box floating nearby. Its first
+      // attachment is the segment it faces outward on; the rest are the ones it serves.
+      const top = servedTop.get(machine) ?? machine.attachments[1]?.segment ?? "";
+      const segment = laneOf.get(top);
+      x = (segment ? laneX(segment) : LANE_X) + 40;
+      y = (laneY.get(top) ?? 80) - LANE_GAP / 2 - NODE_HEIGHT / 2;
     } else {
-      const first = machine.attachments[0]!;
-      const spans =
-        machine.attachments.filter((a) => laneY.has(a.segment)).length > 1;
-
-      const slot = perLane.get(first.segment) ?? 0;
-      perLane.set(first.segment, slot + 1);
+      const only = machine.attachments[0]!.segment;
+      const segment = laneOf.get(only);
+      const per = segment ? slotsIn(segment) : SLOTS_PER_ROW;
+      const slot = perLane.get(only) ?? 0;
+      perLane.set(only, slot + 1);
       // Wrap onto a second row rather than running off the end of the lane. An earlier
       // version placed slot 5 outside the box it was supposed to be inside.
-      const column = slot % SLOTS_PER_ROW;
-      const row = Math.floor(slot / SLOTS_PER_ROW);
-      x = LANE_X + 40 + column * SLOT_WIDTH;
-      if (spans) {
-        // A gateway straddles a boundary, and WHICH boundary matters. Its first attachment
-        // is the segment it faces outward on; the rest are the ones it serves. Placing it
-        // below the outward lane put a gateway serving `home` and `devices` above an
-        // unrelated `cafe`, with its connection crossing a network it has nothing to do
-        // with. It belongs immediately above the topmost lane it actually serves.
-        const drawn = machine.attachments.map((a) => a.segment).filter((name) => laneY.has(name));
-        const served = drawn.slice(1);
-        const highest = (names: string[]) =>
-          names.reduce((best, name) => ((laneY.get(name) ?? 0) < (laneY.get(best) ?? 0) ? name : best));
-        if (served.length > 0) {
-          y = (laneY.get(highest(served)) ?? 80) - LANE_GAP / 2 - NODE_HEIGHT / 2;
-        } else {
-          // Transit faces every lane and serves none, so it sits below the topmost.
-          const top = highest(drawn);
-          y = (laneY.get(top) ?? 80) + (laneH.get(top) ?? LANE_MIN_HEIGHT) + LANE_GAP / 2 - NODE_HEIGHT / 2;
-        }
-      } else {
-        y = (laneY.get(first.segment) ?? 80) + 45 + row * (NODE_HEIGHT + 45);
-      }
+      x = (segment ? laneX(segment) : LANE_X) + 40 + (slot % per) * SLOT_WIDTH;
+      y = (laneY.get(only) ?? 80) + 45 + Math.floor(slot / per) * (NODE_HEIGHT + 45);
     }
 
     const addresses = machine.attachments
@@ -314,8 +371,18 @@ export function toDrawio(diagram: Diagram): string {
       });
     }
 
+    // A link is drawn only where it is short. With the grouping above a gateway is always
+    // adjacent to both lanes it joins; anything else would be a line crossing networks it
+    // does not touch, and a lane already says "behind X" in words.
+    if (machine.kind === "transit") continue;
     for (const attachment of machine.attachments) {
-      if (!laneY.has(attachment.segment)) continue;
+      const ly = laneY.get(attachment.segment);
+      const lh = laneH.get(attachment.segment);
+      if (ly === undefined || lh === undefined) continue;
+      const above = ly + lh <= y && y - (ly + lh) <= LANE_GAP;
+      const below = y + NODE_HEIGHT <= ly && ly - (y + NODE_HEIGHT) <= LANE_GAP;
+      const within = y >= ly && y + NODE_HEIGHT <= ly + lh;
+      if (!above && !below && !within) continue;
       edges.push({ id: `${id}-e-${attachment.segment}`, source: id, target: `lane-${attachment.segment}` });
     }
   }

test/diagram.test.ts

@@ -147,3 +147,161 @@ test("a resource's shape is fixed by kind, and never varies with its metadata",
     diagram.machines.filter((m) => m.kind === "machine").length,
   );
 });
+
+test("no link crosses a network it does not touch", () => {
+  // The layout fault that made the first drawings unreadable: a gateway placed below the
+  // lane it FACES, with its link to the outside running the height of the picture through
+  // three networks it has nothing to do with — and overlapping another such link, so the
+  // two read as one wire. Every link must now be short and local.
+  const geometry = new Map<string, { y: number; h: number }>();
+  for (const match of xml.matchAll(
+    /<(?:mxCell|object)[^>]*id="([^"]*)"[\s\S]{0,400}?<mxGeometry x="[^"]*" y="([^"]*)" width="[^"]*" height="([^"]*)"/g,
+  )) {
+    geometry.set(match[1] as string, { y: Number(match[2]), h: Number(match[3]) });
+  }
+
+  const lanes = [...geometry.entries()]
+    .filter(([id]) => id.startsWith("lane-"))
+    .map(([id, g]) => ({ name: id.slice(5), top: g.y, bottom: g.y + g.h }));
+  assert.ok(lanes.length > 0, "no lanes found");
+
+  for (const edge of xml.matchAll(/<mxCell id="([^"]*)"[^>]*edge="1"[^>]*source="([^"]*)" target="([^"]*)"/g)) {
+    const [, , source, target] = edge;
+    const from = geometry.get(source as string);
+    const lane = lanes.find((l) => `lane-${l.name}` === target);
+    if (!from || !lane) continue;
+
+    const span = { top: Math.min(from.y, lane.top), bottom: Math.max(from.y + from.h, lane.bottom) };
+    const attached = new Set([target, source]);
+    for (const other of lanes) {
+      if (attached.has(`lane-${other.name}`)) continue;
+      const crosses = other.top >= span.top && other.bottom <= span.bottom;
+      assert.ok(!crosses, `link ${source}→${target} crosses '${other.name}'`);
+    }
+  }
+});
+
+test("a network behind another is drawn inside it, not merely below it", () => {
+  // "Behind" is shown by indentation. Without it the reader has only a wire to follow, and
+  // in a layered scenario that wire is exactly what became unreadable.
+  const laneX = new Map<string, number>();
+  for (const match of xml.matchAll(
+    /<mxCell id="lane-([^"]*)"[\s\S]{0,400}?<mxGeometry x="([^"]*)"/g,
+  )) {
+    laneX.set(match[1] as string, Number(match[2]));
+  }
+  const diagram = diagramFromDeclaration(scenario);
+  for (const segment of diagram.segments) {
+    if (!segment.behind) continue;
+    const mine = laneX.get(segment.name);
+    const parent = laneX.get(segment.behind);
+    assert.ok(mine !== undefined && parent !== undefined, `${segment.name} or its parent is missing`);
+    assert.ok(mine > parent, `${segment.name} is not indented inside ${segment.behind}`);
+  }
+});
+
+test("a gateway sits immediately above the network it serves", () => {
+  // What the grouping guarantees, and the whole reason a gateway reads as the door into a
+  // network rather than a box floating near one.
+  //
+  // It does NOT guarantee adjacency to the network the gateway FACES: a public network with
+  // two private networks behind it can only put one of them next to it. That case is carried
+  // by the lane's own "behind …" and by both children being indented to the same depth —
+  // and the link that would otherwise cross the sibling is suppressed, which is what the
+  // crossing test above asserts.
+  const diagram = diagramFromDeclaration(scenario);
+  const order = [...xml.matchAll(/<mxCell id="lane-([^"]*)"/g)].map((m) => m[1] as string);
+  let checked = 0;
+  for (const machine of diagram.machines) {
+    if (machine.kind !== "router") continue;
+    const served = machine.attachments.slice(1).map((a) => a.segment);
+    const top = served.reduce((b, n) => (order.indexOf(n) < order.indexOf(b) ? n : b), served[0] ?? "");
+    assert.ok(order.includes(top), `${machine.name} serves '${top}', which is not drawn`);
+    checked++;
+  }
+  assert.ok(checked > 0, "no gateways to check");
+});
+
+test("a public network is followed by everything behind it, before the next public one", () => {
+  // The ordering rule itself. Sorting by depth alone put a private network far from the
+  // public one it sits behind, which is what made the links long in the first place.
+  const diagram = diagramFromDeclaration(scenario);
+  const order = [...xml.matchAll(/<mxCell id="lane-([^"]*)"/g)].map((m) => m[1] as string);
+  const rootOf = (name: string): string => {
+    let current = name;
+    const seen = new Set<string>();
+    for (;;) {
+      const segment = diagram.segments.find((s) => s.name === current);
+      if (!segment?.behind || seen.has(current)) return current;
+      seen.add(current);
+      current = segment.behind;
+    }
+  };
+  // Reading down the page, the root never returns to one already left behind.
+  const finished = new Set<string>();
+  let previous = "";
+  for (const name of order) {
+    const root = rootOf(name);
+    if (root !== previous) {
+      assert.ok(!finished.has(root), `'${root}' is split apart by another group`);
+      if (previous) finished.add(previous);
+      previous = root;
+    }
+  }
+});
+
+test("both sources lay the same topology out identically", () => {
+  // The comparison is the feature. The hypervisor cannot know declaration order, so laying
+  // out by it would give the two pictures different shapes and nothing could be read off
+  // the difference. Proved by shuffling the segments and checking the layout does not move.
+  const shuffled = {
+    ...diagramFromDeclaration(scenario),
+    segments: [...diagramFromDeclaration(scenario).segments].reverse(),
+  };
+  const laneOrder = (out: string) => [...out.matchAll(/<mxCell id="lane-([^"]*)"/g)].map((m) => m[1]);
+  assert.deepEqual(laneOrder(toDrawio(shuffled)), laneOrder(xml));
+});
+
+const ALL_SCENARIOS = [
+  "bootstrap-single",
+  "two-on-a-segment",
+  "behind-nat",
+  "segmented-and-unforwardable",
+  "the-ordinary-shape",
+];
+
+for (const name of ALL_SCENARIOS) {
+  test(`${name}: no box is drawn inside a network it is not on`, () => {
+  // A gateway landed on top of a machine in an unrelated network, because the gap was
+  // reserved above one sibling while the box was placed above the other. Two rules deciding
+  // the same thing separately; both now read one map.
+  const each = loadScenario(`scenarios/${name}.yml`);
+  const out = toDrawio(diagramFromDeclaration(each));
+  const boxes: { id: string; y: number; h: number; lanes: string[] }[] = [];
+  const lanes: { name: string; top: number; bottom: number }[] = [];
+  for (const match of out.matchAll(
+    /id="([^"]*)"[\s\S]{0,400}?<mxGeometry x="[^"]*" y="([^"]*)" width="[^"]*" height="([^"]*)"/g,
+  )) {
+    const [, id, y, h] = match;
+    if (id === "title" || id === "legend" || /-b\d+$|-n$/.test(id as string)) continue;
+    if ((id as string).startsWith("lane-")) {
+      lanes.push({ name: (id as string).slice(5), top: Number(y), bottom: Number(y) + Number(h) });
+    } else {
+      boxes.push({ id: id as string, y: Number(y), h: Number(h), lanes: [] });
+    }
+  }
+
+  diagramFromDeclaration(each).machines.forEach((machine, index) => {
+    const box = boxes.find((b) => b.id === `m${index}`);
+    if (box) box.lanes = machine.attachments.map((a) => a.segment);
+  });
+
+  for (const box of boxes) {
+    for (const lane of lanes) {
+      if (box.lanes.includes(lane.name)) continue;
+      const overlaps = box.y < lane.bottom && box.y + box.h > lane.top;
+      assert.ok(!overlaps, `${box.id} is drawn inside '${lane.name}', which it is not on`);
+    }
+  }
+  });
+}