Skip to content

Commit 6fba9e9

Browse files
committed
fix(er): give each relationship end on a crowded row its own port (#540)
## What changes Round 24's only visual major for Sonnet was saas-workspace-er s5: five relationships end on `organizations.id`. A table row holds two ends on a side cleanly, 22 apart. A third was squeezed in between them, 11 apart. Excalidraw draws each bar or crow's foot 17 across, so the three bars ran into one vertical stroke, and the reader could not tell which end belonged to which line. **A crowded row grows so each end has its own port.** - Once the placement is chosen, a row holding more than two ends on a side grows by two lines (56) for each further end. Its ends then sit a port step (22) apart around the row's middle. - Two lines keeps the outermost port as close to its own field, relative to the next row's field, as either of two ends on a plain row (about 2:3). That way the line still reads as attached to the right column. At one step (50) instead, the outer port of s5's row sat 17 from `name` and 44 from `id`. - A last row holding two ends on a side makes room below them (8), clear of the corner. Before, a one-row table's two ends were clamped 15 apart. - The tables are placed again in the same arrangement: the same columns and the same order within them. - Rows align on the ports the lines will take, so a line between level rows still runs straight (s5's `labels` line). - The layout takes optional `parentPort`/`childPort` and an `arrangement` to keep. - A grown row draws one text per cell (`text:<table>:<column>:<row>`). Empty cells are left out, since the scene schema rejects empty text. Tables without a grown row keep one text per column, and `erDrawnCheck` reads both forms. - **Guard (as #447/#524):** the grown drawing is kept only if it draws no more: - crossings, reader findings or detours (longer than twice the direct way plus 120); - ends wrapping their own table, or tables a line runs past; - marks on labels; - and no longer a long side. Otherwise main's drawing stands. **Loops nest inside lines to other columns.** Among the ends climbing (or dropping) from one row side, a loop to a table in the same column now takes the port nearest its turn. Before, rise alone decided. In s5, the `api_keys` loop took the lower port and `projects`' line, which turns far out by `projects`, crossed it. That was s5's crossing, and it is gone. Candidate selection is unchanged. Growth repairs the chosen placement and never re-ranks placements. Letting growth into selection let the first layered candidate win on its margin with long parallel runs: 20 fuzz schemas worse. ## Measured ER corpus: all 46 distinct ER specs from Oct 17–23 evals, including r24. Branch against main 42b2d80. | | main | branch | |---|---|---| | byte-identical | | 39 / 46 (the 7 saas-workspace-er specs change) | | renders worse on crossings, findings, detours, lines through a table or head, marks on labels | | 0 | | ends sharing an anchor (two ends on one side under 20 apart) | 18 | 0 | | marker overlaps (bars, feet or rings touching) | 18 | 0 | | crossings | 9 | 2 | | reader findings | 12 | 2 | | detours | 0 | 0 | | line length / drawing area | | +0.6% / +2.9% | | render time, all 46 (interleaved, min of 3×3) | 885 ms | 899 ms (+1.5%) | - **s5 spec** (also s2–s4 and 80 other corpus runs): - crossings 1 → 0, reader findings 2 → 0, shared anchors 2 → 0; - `organizations.id` grows once (3 left, 2 right); 1532×988 → 1532×1044; - 116.7 → 115.3 ms. - **The other saas variants:** - Two place `users.id` and `organizations.id` three to a side. Both rows grow once; crossings 2 → 1, shared anchors 4 → 0. - The remaining saas variants share s5's layout and change the same way. ER fuzz at full seeds (240 schemas), against main: - 0 schemas worse on crossings, findings, detours, lines through a head, marks crossed by another line, or marks on labels. 191 / 240 byte-identical. - Shared anchors 122 → 24, marks another line runs through 58 → 20. - Crossings 69 → 61, findings 171 → 150, detours 25 → 23. Line length −0.3%, area ±0. - Render time 6.68 → 6.91 s (+3.5%); +8% on the 49 changed schemas. - Class and org-chart rendering are untouched (they do not import `er.ts` or `er-layout.ts`). Stress fixtures: marketplace crossings 13 → 9 and bunched ends 6 → 0. saasWorkspace crossings 1 → 0, findings 2 → 0, bunched 2 → 0. ## Proof - Failing-first tests: - From the real s5 spec (`er.test.ts`): no bunched ends; each of the five ends inside the id row's band and nearer `id` than `name` by the 2:3 rule; `labels` straight; 0 crossings. It failed on main with 2 bunched pairs. - A one-row table's two ends a port step apart, clear of the corner (failed on main: 15 apart). - `saasWorkspace` fixture pin tightened: crossings ≤ 1 → 0, bunched ends ≤ 2 → 0. - Code Mode (`agent/er.test.ts`): a five-way key row builds artifacts that decode as the CLI and MCP read them. The per-cell texts are present, with no empty cells. A local CLI render caught this: the first cut emitted empty key cells. - `erDrawnCheck` reads per-cell tables, and still fails a renamed cell. - ER fuzz pins bunched neighbor ends: 0 in CI's 16 schemas (main 3), at most 18 over the full 240 (main 99). - Local runs, all passing: - `pnpm nx affected -t typecheck,test,build --base=origin/main` (10 projects); - `pnpm run check`; - `pnpm nx build-storybook diagram-ui`; - full `er-fuzz`. - PNGs inspected locally for s5 and f8f5 (both saas layouts). ## Open - 18 bunched neighbor pairs in full fuzz (9 schemas, 5 distinct) keep main's drawing, because growth there would add a reader finding: mostly a step that becomes a port jog, or a corridor. Schema 136 would trade two crossings for one jog. - A name band (entity-level ends) holding three ends on a side still bunches. The band does not grow.
1 parent 42b2d80 commit 6fba9e9

8 files changed

Lines changed: 676 additions & 90 deletions

File tree

‎.changeset/er-ends-own-ports.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@sketchi/cli": patch
3+
---
4+
5+
ER diagrams give every relationship its own end where several lines meet one column. A table row can hold two line ends on a side with their bars or crow's feet apart; a third used to squeeze in between, so the marks ran together into one stroke and the reader could not tell which end belonged to which line. In a SaaS workspace schema, three relationships into `organizations.id` from the left merged that way. Now a row that holds more than two ends on a side grows taller, so each end has a port of its own on that row and the line still lands on the column it references. A one-row table with two ends on a side also makes room below them, clear of its corner. Lines that loop back to a table in the same column now take the port nearest the turn, which also removes the crossing the SaaS schema drew there. A table only grows when the drawing reads no worse: no more crossings, reader findings, detours or marks on labels, and no larger drawing.

‎packages/diagram/agent/src/lib/code-mode/er.test.ts‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { validateExcalidrawScene } from "@sketchi/diagram-excalidraw";
55
import type { RenderedDiagramScene } from "@sketchi/diagram-renderer";
66

77
import { CodeModeArtifactStorage, makeMemoryArtifactStorage } from "./artifacts.js";
8-
import { BuildErDiagramResultSchema } from "./contract.js";
8+
import { BuildErDiagramResultSchema, RenderedDiagramSceneSchema } from "./contract.js";
99
import { canonicalDocumentFromDiagram } from "./documents.js";
1010
import { applyDiagramPatch, buildErDiagram, CodeModeRuntimeEnvironment } from "./runtime.js";
1111

@@ -93,6 +93,39 @@ describe("Code Mode native ER diagram", () => {
9393
]);
9494
});
9595

96+
it("builds a key row five tables reference into valid artifacts, its row a text per cell", async () => {
97+
const children = ["projects", "labels", "api_keys", "memberships", "subscriptions"];
98+
const built = await build({
99+
title: "Workspace",
100+
entities: [
101+
{ id: "organizations", fields: ["id uuid PK", "name text"] },
102+
...children.map((id) => ({ id, fields: ["id uuid PK", "organization_id uuid FK"] })),
103+
],
104+
relationships: children.map((id) => ({
105+
from: `${id}.organization_id`,
106+
to: "organizations.id",
107+
fromCardinality: "zero-or-many",
108+
toCardinality: "one",
109+
})),
110+
});
111+
if (!built.ok) throw new Error(JSON.stringify(built.issues));
112+
const formats = new Map(built.artifact.formats.map((format) => [format.format, format.inline]));
113+
// The scene decodes as the CLI and MCP read it: no empty text where a row has no key or type.
114+
const scene = Schema.decodeUnknownSync(RenderedDiagramSceneSchema)(formats.get("scene"));
115+
const cells = scene.elements.flatMap((element) =>
116+
element.type === "text" && element.id.startsWith("text:organizations:") ? [element.id] : [],
117+
);
118+
expect(cells.sort()).toEqual([
119+
"text:organizations:fields:0",
120+
"text:organizations:fields:1",
121+
"text:organizations:keys:0",
122+
"text:organizations:types:0",
123+
"text:organizations:types:1",
124+
]);
125+
const excalidraw = formats.get("excalidraw") as Parameters<typeof validateExcalidrawScene>[0];
126+
expect(validateExcalidrawScene(excalidraw)).toEqual({ ok: true, issues: [] });
127+
});
128+
96129
it("reads cardinality aliases, source/target and a combined cardinality", async () => {
97130
const built = await build({
98131
...shopSpec,

‎packages/diagram/excalidraw/src/lib/er-fuzz.test.ts‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -185,9 +185,9 @@ function routingCounts(scene: RenderedDiagramScene) {
185185
/**
186186
* Ends whose bar or foot another line runs through, other than a neighbor's
187187
* on the same row of the same table: Excalidraw draws each head min(20, run / 2)
188-
* long and 20 sin 25° either side of its line. A row holding more ends than
189-
* its band spaces a mark apart may bunch them, but no line from another row,
190-
* or passing by, may cross a mark.
188+
* long and 20 sin 25° either side of its line. A row whose growth would draw
189+
* worse may still bunch its ends, but no line from another row, or passing by,
190+
* may cross a mark.
191191
*/
192192
function foreignMarks(scene: RenderedDiagramScene): string[] {
193193
const arrows = scene.elements.flatMap((element) => (element.type === "arrow" ? [element] : []));
@@ -251,6 +251,29 @@ function foreignMarks(scene: RenderedDiagramScene): string[] {
251251
);
252252
}
253253

254+
/**
255+
* Neighboring ends on one side of a table whose heads run together: Excalidraw
256+
* draws a bar or foot 17 across, so two ports nearer than 20 read as one end.
257+
*/
258+
function bunchedEnds(scene: RenderedDiagramScene): number {
259+
const sides = new Map<string, number[]>();
260+
for (const arrow of scene.elements)
261+
if (arrow.type === "arrow")
262+
for (const [node, point] of [
263+
[arrow.sourceNodeId, arrow.points[0]],
264+
[arrow.targetNodeId, arrow.points.at(-1)!],
265+
] as const) {
266+
const key = `${node}:${Math.round(point.x)}`;
267+
sides.set(key, [...(sides.get(key) ?? []), point.y]);
268+
}
269+
let bunched = 0;
270+
for (const ports of sides.values()) {
271+
ports.sort((a, b) => a - b);
272+
bunched += ports.slice(1).filter((port, index) => port - ports[index]! < 20).length;
273+
}
274+
return bunched;
275+
}
276+
254277
/** Everything the export validator, the canvas validator or the layout guards would flag. */
255278
function defects(diagram: ErDiagram): string[] {
256279
const scene = renderDiagram(diagram);
@@ -310,6 +333,12 @@ function defects(diagram: ErDiagram): string[] {
310333
* together they may draw no more of either.
311334
*/
312335
const ER_ROUTING = "1/2 0/0 0/0 0/0 0/0 1/0 0/0 0/1 0/0 0/0 0/0 1/0 0/0 0/0 0/0 0/0";
336+
/**
337+
* Neighboring ends that bunch across the schemas checked: a row holding more
338+
* than two ends on a side grows to give each a port of its own, except where
339+
* growing would draw worse (99 and 3 before rows grew).
340+
*/
341+
const ER_BUNCHED = { ci: 0, full: 18 };
313342

314343
/** Schemas checked in CI; the full fuzz runs locally with ER_FUZZ=full (and any ER_FUZZ_SEED). */
315344
const CI_SCHEMAS = 16;
@@ -348,4 +377,10 @@ describe("ER routing fuzz", () => {
348377
expect(total(drawn, 0)).toBeLessThanOrEqual(total(pinned, 0));
349378
expect(total(drawn, 1)).toBeLessThanOrEqual(total(pinned, 1));
350379
});
380+
381+
it("bunches no more ends than pinned, each crowded row grown for its ends", () => {
382+
const bunched = schemas.reduce((sum, schema) => sum + bunchedEnds(renderDiagram(schema)), 0);
383+
if (process.env["PRINT_ROUTING"]) console.log("PIN er bunched", bunched);
384+
expect(bunched).toBeLessThanOrEqual(FULL ? ER_BUNCHED.full : ER_BUNCHED.ci);
385+
});
351386
});

‎packages/diagram/renderer/src/diagram-types/er.test.ts‎

Lines changed: 163 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
getCanvasValidationIssues,
88
orgChartEr,
99
parseErDiagram,
10+
parseErFieldShorthand,
1011
segmentCrossesBoundsInterior,
1112
segmentsFromPoints,
1213
} from "@sketchi/diagram-core";
@@ -171,6 +172,96 @@ function foreignMarks(scene: RenderedDiagramScene): string[] {
171172
const aspectOf = (scene: RenderedDiagramScene) =>
172173
Math.max(scene.width / scene.height, scene.height / scene.width);
173174

175+
/**
176+
* Each field row's middle and height from its table's top, read off the drawn
177+
* field names: one text per column holds every row 28 apart, or, where a row
178+
* grows, one text per row stands at that row's middle.
179+
*/
180+
function drawnRows(
181+
scene: RenderedDiagramScene,
182+
id: string,
183+
): readonly { readonly middle: number; readonly height: number }[] {
184+
const box = table(scene, id);
185+
const column = text(scene, `text:${id}:fields`);
186+
if (column?.type === "text")
187+
return column.text
188+
.split("\n")
189+
.map((_, row) => ({ middle: rowY(box, row) - box.y, height: 28 }));
190+
const middles: number[] = [];
191+
for (let row = 0; ; row += 1) {
192+
const cell = text(scene, `text:${id}:fields:${row}`);
193+
if (cell?.type !== "text") break;
194+
middles.push(cell.y - box.y);
195+
}
196+
let top = 46;
197+
return middles.map((middle) => {
198+
const height = 2 * (middle - top);
199+
top += height;
200+
return { middle, height };
201+
});
202+
}
203+
204+
/** Sonnet's saas-workspace-er s5 in round 24, as the model wrote it: five lines end on organizations.id. */
205+
const saasWorkspaceS5 = parseErDiagram({
206+
id: "saas-workspace-database",
207+
title: "SaaS Workspace Database",
208+
type: "er",
209+
entities: (
210+
[
211+
["organizations", ["id uuid PK", "name text"]],
212+
["users", ["id uuid PK", "email text UK", "name text"]],
213+
["memberships", ["organization_id uuid PK FK", "user_id uuid PK FK", "role text"]],
214+
["projects", ["id uuid PK", "organization_id uuid FK", "name text"]],
215+
[
216+
"tasks",
217+
["id uuid PK", "project_id uuid FK", "assignee_id uuid? FK", "title text", "due_on date?"],
218+
],
219+
["comments", ["id uuid PK", "task_id uuid FK", "author_id uuid FK", "body text"]],
220+
["labels", ["id uuid PK", "organization_id uuid FK", "name text"]],
221+
["task_labels", ["task_id uuid PK FK", "label_id uuid PK FK"]],
222+
["plans", ["id uuid PK", "name text UK", "monthly_price numeric"]],
223+
[
224+
"subscriptions",
225+
["id uuid PK", "organization_id uuid FK UK", "plan_id uuid FK", "renews_on date"],
226+
],
227+
[
228+
"invoices",
229+
["id uuid PK", "subscription_id uuid FK", "amount numeric", "paid_at timestamptz?"],
230+
],
231+
[
232+
"api_keys",
233+
["id uuid PK", "organization_id uuid FK", "created_by uuid FK", "token_hash text UK"],
234+
],
235+
] as const
236+
).map(([id, fields]) => ({ id, fields: fields.map(parseErFieldShorthand) })),
237+
relationships: (
238+
[
239+
["memberships.organization_id", "organizations.id", "one-or-many", "one"],
240+
["memberships.user_id", "users.id", "zero-or-many", "one"],
241+
["projects.organization_id", "organizations.id", "zero-or-many", "one"],
242+
["labels.organization_id", "organizations.id", "zero-or-many", "one"],
243+
["api_keys.organization_id", "organizations.id", "zero-or-many", "one"],
244+
["tasks.project_id", "projects.id", "zero-or-many", "one"],
245+
["tasks.assignee_id", "users.id", "zero-or-many", "zero-or-one"],
246+
["comments.task_id", "tasks.id", "zero-or-many", "one"],
247+
["comments.author_id", "users.id", "zero-or-many", "one"],
248+
["task_labels.task_id", "tasks.id", "zero-or-many", "one"],
249+
["task_labels.label_id", "labels.id", "zero-or-many", "one"],
250+
["subscriptions.organization_id", "organizations.id", "zero-or-one", "one"],
251+
["subscriptions.plan_id", "plans.id", "zero-or-many", "one"],
252+
["invoices.subscription_id", "subscriptions.id", "zero-or-many", "one"],
253+
["api_keys.created_by", "users.id", "zero-or-many", "one"],
254+
] as const
255+
).map(([from, to, fromCardinality, toCardinality], index) => ({
256+
id: `relationship-${index + 1}`,
257+
from,
258+
to,
259+
fromCardinality,
260+
toCardinality,
261+
})),
262+
style: { accentColor: "#8f707f", backgroundColor: "#fffdf8" },
263+
});
264+
174265
describe("ER renderer", () => {
175266
it("renders every fixture into a valid deterministic scene", () => {
176267
for (const diagram of [erFixture, orgChartEr, ...Object.values(erStressFixtures)]) {
@@ -454,14 +545,82 @@ describe("ER renderer", () => {
454545
}
455546
});
456547

457-
it("places a workspace's two hubs so its lines cross once at most, apart at their ends", () => {
548+
it("places a workspace's two hubs so no lines cross, apart at their ends", () => {
458549
const scene = renderDiagram(erStressFixtures.saasWorkspace);
459550
// The organization holds five ends on its id row and users four: spread
460-
// over both sides, two to a side stand clear and only a third bunches.
461-
expect(crossings(arrows(scene))).toBeLessThanOrEqual(1);
462-
expect(bunchedEnds(scene)).toBeLessThanOrEqual(2);
551+
// over both sides, two to a side stand clear, and the row holding a third
552+
// grows to give it a port of its own.
553+
expect(crossings(arrows(scene))).toBe(0);
554+
expect(bunchedEnds(scene)).toBe(0);
555+
expect(wraps(scene)).toBe(0);
556+
expect(aspectOf(scene)).toBeLessThanOrEqual(2);
557+
});
558+
559+
it("gives each of five lines on one key its own end, on that key's row", () => {
560+
const scene = renderDiagram(saasWorkspaceS5);
561+
// Two ends to a side stand clear in a row's band; the third on a side has
562+
// its row grow, so no two bars or feet run together.
563+
expect(bunchedEnds(scene)).toBe(0);
564+
const organizations = table(scene, "organizations");
565+
const ends = arrows(scene).flatMap((line) =>
566+
[
567+
[line.sourceNodeId, line.points[0]],
568+
[line.targetNodeId, line.points.at(-1)!],
569+
].flatMap(([node, point]) => (node === "organizations" ? [point as { y: number }] : [])),
570+
);
571+
expect(ends).toHaveLength(5);
572+
// Each still meets the id row, inside its band, and reads as id's: no
573+
// further from id's middle than two thirds of its way to name's, as
574+
// either of two ends on a plain row.
575+
const [id, name] = drawnRows(scene, "organizations");
576+
for (const end of ends) {
577+
const y = end.y - organizations.y;
578+
expect(Math.abs(y - id!.middle)).toBeLessThanOrEqual(id!.height / 2 - 3);
579+
expect(Math.abs(y - id!.middle)).toBeLessThanOrEqual((2 / 3) * Math.abs(y - name!.middle));
580+
}
581+
// labels' row lines up with the port its line takes on the grown row, so it runs straight.
582+
expect(arrow(scene, "relationship-4").points).toHaveLength(2);
583+
expect(foreignMarks(scene)).toEqual([]);
584+
// Its loop from api_keys takes the upper port, inside the line from
585+
// projects, so the two no longer cross.
586+
expect(crossings(arrows(scene))).toBe(0);
463587
expect(wraps(scene)).toBe(0);
464588
expect(aspectOf(scene)).toBeLessThanOrEqual(2);
589+
expect(getCanvasValidationIssues(scene)).toEqual([]);
590+
});
591+
592+
it("keeps two ends on a one-row table's side a port step apart, clear of its corner", () => {
593+
const diagram = parseErDiagram({
594+
id: "single-row",
595+
title: "Single row",
596+
type: "er",
597+
entities: [
598+
{ id: "accounts", fields: [{ name: "id", type: "int", pk: true }] },
599+
...["orders", "invoices"].map((id) => ({
600+
id,
601+
fields: [
602+
{ name: "id", type: "int", pk: true },
603+
{ name: "account_id", type: "int", fk: true },
604+
],
605+
})),
606+
],
607+
relationships: ["orders", "invoices"].map((id) => ({
608+
id,
609+
from: "accounts.id",
610+
to: `${id}.account_id`,
611+
fromCardinality: "one" as const,
612+
toCardinality: "zero-or-many" as const,
613+
})),
614+
});
615+
const scene = renderDiagram(diagram);
616+
const accounts = table(scene, "accounts");
617+
const starts = arrows(scene)
618+
.map((line) => line.points[0].y)
619+
.sort((a, b) => a - b);
620+
expect(starts).toHaveLength(2);
621+
expect(starts[1]! - starts[0]!).toBeGreaterThanOrEqual(20);
622+
expect(accounts.y + accounts.height - starts[1]!).toBeGreaterThanOrEqual(16);
623+
expect(bunchedEnds(scene)).toBe(0);
465624
});
466625

467626
it("keeps a small schema within two to one, stacking tables where a row runs wide", () => {

0 commit comments

Comments
 (0)