refactor(schema): address PR #62/#63 code-review follow-ups

src/core/dot-layout.js

@@ -20,45 +20,38 @@ export function nodeWidth(label) {
   return Math.max(MIN_W, String(label).length * CHAR_W + PAD_X);
 }
 
+// Box size for a node: honor an explicit w/h when it carries one (the rich schema
+// cards pre-compute w/h from their content via cardSize); otherwise fall back to
+// the label-based width + fixed height (pipeline + inline schema boxes).
+const sizeOf = (n) => ({ width: n.w != null ? n.w : nodeWidth(n.label), height: n.h != null ? n.h : NODE_H });
+// `kind`/`db`/`name`/`external` (node) and `label` (edge) pass through for the
+// schema graph's colouring, external-dimming + click-to-SHOW-CREATE (so the UI
+// need not re-split the id or keep a side-channel for these).
+const carry = (n) => ({ id: n.id, label: n.label, kind: n.kind, db: n.db, name: n.name, external: n.external });
+
 /**
+ * Lay out a graph with dagre. Generic (pipeline + schema lineage): every node is
+ * ranked top→bottom and edges routed. Returns `{ nodes, edges, width, height }`
+ * with node x/y as top-left.
  * @param dagre  the injected dagre module (`{ graphlib, layout }`)
  * @param graph  parsed `{ nodes:[{id,label}], edges:[{from,to}] }`
  */
-export function dagreLayout(dagre, graph, opts = {}) {
+export function dagreLayout(dagre, graph) {
   const nodes = graph.nodes || [];
   if (!nodes.length) return { nodes: [], edges: [], width: 0, height: 0 };
   const ids = new Set(nodes.map((n) => n.id));
   // Keep edges between declared processors; drop self-loops (a Resize feedback
   // would just loop onto its own box).
   const edges = (graph.edges || []).filter((e) => ids.has(e.from) && ids.has(e.to) && e.from !== e.to);
 
-  // Schema views opt into `isolatedLast`: lay out only the connected nodes with
-  // dagre and pack the edge-less "single" tables into a grid *below* the lineage,
-  // so a whole-DB graph reads as "relationships first, loose tables after" rather
-  // than dagre ranking the orphans across the top. Other callers (the pipeline
-  // graph) keep every node in the dagre pass.
-  const connected = new Set();
-  for (const e of edges) { connected.add(e.from); connected.add(e.to); }
-  const singles = opts.isolatedLast ? nodes.filter((n) => !connected.has(n.id)) : [];
-  const ranked = opts.isolatedLast ? nodes.filter((n) => connected.has(n.id)) : nodes;
-
-  // Honor a node's explicit size when it carries one (the rich schema cards
-  // pre-compute w/h from their content via cardSize); otherwise fall back to the
-  // label-based width + fixed height (pipeline + inline schema boxes).
-  const sizeOf = (n) => ({ width: n.w != null ? n.w : nodeWidth(n.label), height: n.h != null ? n.h : NODE_H });
-  // `kind`/`db`/`name`/`external` (node) and `label` (edge) pass through for the
-  // schema graph's colouring, external-dimming + click-to-SHOW-CREATE (so the UI
-  // need not re-split the id or keep a side-channel for these).
-  const carry = (n) => ({ id: n.id, label: n.label, kind: n.kind, db: n.db, name: n.name, external: n.external });
-
   const g = new dagre.graphlib.Graph();
   g.setGraph({ rankdir: 'TB', nodesep: NODESEP, ranksep: RANKSEP, marginx: MARGIN, marginy: MARGIN });
   g.setDefaultEdgeLabel(() => ({}));
-  for (const n of ranked) g.setNode(n.id, sizeOf(n));
+  for (const n of nodes) g.setNode(n.id, sizeOf(n));
   for (const e of edges) g.setEdge(e.from, e.to);
-  if (ranked.length) dagre.layout(g);
+  dagre.layout(g);
 
-  const outNodes = ranked.map((n) => {
+  const outNodes = nodes.map((n) => {
     const dn = g.node(n.id);
     return { ...carry(n), x: dn.x - dn.width / 2, y: dn.y - dn.height / 2, w: dn.width, h: dn.height };
   });
@@ -67,26 +60,46 @@ export function dagreLayout(dagre, graph, opts = {}) {
     points: g.edge(e.from, e.to).points.map((p) => ({ x: p.x, y: p.y })),
   }));
   const gg = g.graph();
-  let width = ranked.length ? gg.width : 0;
-  let height = ranked.length ? gg.height : 0;
+  return { nodes: outNodes, edges: outEdges, width: gg.width, height: gg.height };
+}
+
+/**
+ * Schema-graph layout: dagre the connected lineage, then grid-pack the edge-less
+ * "single" tables *below* it — so a whole-DB graph reads as "relationships first,
+ * loose tables after" rather than dagre ranking the orphans across the top. The
+ * grid is a roughly-square block of uniform cells (widest/tallest single),
+ * left-aligned at the margin, one ranksep below the lineage (or at the top when
+ * there is no lineage at all). Same `{ nodes, edges, width, height }` shape.
+ */
+export function schemaLayout(dagre, graph) {
+  const nodes = graph.nodes || [];
+  if (!nodes.length) return { nodes: [], edges: [], width: 0, height: 0 };
+  const ids = new Set(nodes.map((n) => n.id));
+  const edges = (graph.edges || []).filter((e) => ids.has(e.from) && ids.has(e.to) && e.from !== e.to);
+  const connected = new Set();
+  for (const e of edges) { connected.add(e.from); connected.add(e.to); }
+  const singles = nodes.filter((n) => !connected.has(n.id));
+  if (!singles.length) return dagreLayout(dagre, graph); // no orphans → plain dagre
 
-  // Grid-pack the singles beneath the connected layout: a roughly-square block of
-  // uniform cells (widest/tallest single), left-aligned at the margin, sitting one
-  // ranksep below the lineage (or at the top when there's no lineage at all).
-  if (singles.length) {
-    const cells = singles.map(sizeOf);
-    const colW = Math.max(...cells.map((c) => c.width));
-    const rowH = Math.max(...cells.map((c) => c.height));
-    const cols = Math.max(1, Math.ceil(Math.sqrt(singles.length)));
-    const top = height ? height + RANKSEP : MARGIN;
-    singles.forEach((n, i) => {
-      const col = i % cols, row = Math.floor(i / cols);
-      outNodes.push({ ...carry(n), x: MARGIN + col * (colW + NODESEP), y: top + row * (rowH + NODESEP), w: cells[i].width, h: cells[i].height });
-    });
-    const usedCols = Math.min(cols, singles.length);
-    const rows = Math.ceil(singles.length / cols);
-    width = Math.max(width, MARGIN * 2 + usedCols * colW + (usedCols - 1) * NODESEP);
-    height = top + rows * rowH + (rows - 1) * NODESEP + MARGIN;
-  }
-  return { nodes: outNodes, edges: outEdges, width, height };
+  // Lay the lineage out with dagre (connected nodes only, so the orphans don't
+  // reserve a rank-0 row across the top), then append the grid beneath it.
+  const base = dagreLayout(dagre, { nodes: nodes.filter((n) => connected.has(n.id)), edges });
+  const cells = singles.map(sizeOf);
+  const colW = Math.max(...cells.map((c) => c.width));
+  const rowH = Math.max(...cells.map((c) => c.height));
+  const cols = Math.max(1, Math.ceil(Math.sqrt(singles.length)));
+  const top = base.height ? base.height + RANKSEP : MARGIN;
+  const gridded = singles.map((n, i) => ({
+    ...carry(n),
+    x: MARGIN + (i % cols) * (colW + NODESEP),
+    y: top + Math.floor(i / cols) * (rowH + NODESEP),
+    w: cells[i].width, h: cells[i].height,
+  }));
+  const rows = Math.ceil(singles.length / cols);
+  return {
+    nodes: [...base.nodes, ...gridded],
+    edges: base.edges,
+    width: Math.max(base.width, MARGIN * 2 + cols * colW + (cols - 1) * NODESEP),
+    height: top + rows * rowH + (rows - 1) * NODESEP + MARGIN,
+  };
 }

src/core/schema-graph.js

@@ -241,7 +241,7 @@ export function buildSchemaGraph(rows, focus) {
   // friendly ·inner and external-source labels are left alone — and ids/edges
   // (which key everything, incl. click-to-SHOW-CREATE) are unaffected.
   const curDb = focus && focus.db;
-  if (curDb) for (const n of outNodes) { if (n.db === curDb && n.label === n.id) n.label = n.name; }
+  if (curDb) for (const n of outNodes) { if (n.db === curDb && n.name && n.label === n.id) n.label = n.name; }
   return { nodes: outNodes, edges: outEdges };
 }
 

src/net/ch-client.js

@@ -237,7 +237,13 @@ export async function loadLineageTransitive(ctx, focus, opts = {}) {
       dictionaries = dictionaries.concat(part.dictionaries);
     }
     const graph = buildSchemaGraph({ tables, dictionaries });
-    if (graph.nodes.length >= nodeCap) { truncated = true; break; }
+    // Cap on the *lineage* size — count only nodes that participate in an edge.
+    // Standalone tables are cheap to render and never drive cross-DB expansion, so
+    // they must not trip the cap (a single big DB of mostly-unrelated tables would
+    // otherwise truncate on the first round, before its few links are followed).
+    const linked = new Set();
+    for (const e of graph.edges) { linked.add(e.from); linked.add(e.to); }
+    if (linked.size >= nodeCap) { truncated = true; break; }
     frontier = externalDbs(graph, loaded);
   }
   return { rows: { tables, dictionaries }, truncated };

src/ui/app.js

@@ -4,7 +4,7 @@
 // window, location, fetch, crypto, sessionStorage) is injected so the whole
 // controller is testable under happy-dom with stubs.
 
-import { h, zoomScale } from './dom.js';
+import { h, zoomScale, fixedAnchor } from './dom.js';
 import { Icon } from './icons.js';
 import {
   createState, activeTab, KEYS, recordHistory, saveQuery, savedForTab, tabChart,
@@ -730,13 +730,11 @@ export function createApp(env = {}) {
     };
     app.dom[refKey] = node;
     const r = anchorEl.getBoundingClientRect();
-    // Bridge the shipped html{zoom}: getBoundingClientRect is post-zoom px, but a
-    // fixed element's top/right are re-scaled by zoom on paint — divide by scale so
-    // the popover anchors under the button (same as the File menu / editor popovers).
-    const scale = zoomScale(anchorEl);
+    // Right-align under the button, bridging html{zoom} (see fixedAnchor / zoomScale).
+    const a = fixedAnchor(r, zoomScale(anchorEl), { viewportW: win.innerWidth || 0 });
     node.style.position = 'fixed';
-    node.style.top = (r.bottom / scale + 6) + 'px';
-    node.style.right = Math.max(8, ((win.innerWidth || 0) - r.right) / scale) + 'px';
+    node.style.top = a.top + 'px';
+    node.style.right = a.right + 'px';
     doc.body.appendChild(node);
     doc.addEventListener('keydown', onKey, true);
     doc.addEventListener('mousedown', onOutside, true);
@@ -890,9 +888,10 @@ export function renderApp(app, helpers) {
   const dragCtx = {
     state,
     rectFor,
-    // Only the px-based 'col' axis needs the html{zoom} bridge (the '%' axes use a
-    // zoom-cancelling ratio); measure the sidebar, which lives in the zoomed tree.
-    scale: (axis) => (axis === 'col' ? zoomScale(sidebar) : 1),
+    // The px-based 'col' axis divides clientX by the page zoom; the '%' axes use a
+    // zoom-cancelling ratio and ignore `scale` (dragValue's default), so we needn't
+    // special-case the axis here — just report the page zoom from the sidebar.
+    scale: () => zoomScale(sidebar),
     apply: (axis, value) => {
       if (axis === 'col') sidebar.style.width = value + 'px';
       else if (axis === 'sideRow') sidebar.firstElementChild.style.height = value + '%';

src/ui/dom.js

@@ -60,10 +60,28 @@ export function s(tag, props, ...children) {
 // post-zoom px while layout (offsetWidth) is pre-zoom CSS px, so their ratio is
 // the zoom. The single source of truth for bridging `html{zoom}` when mapping
 // between client coords and CSS px (editor popovers, results column-resize).
+// `zoom` is a page-global html{} property, so the element measured is immaterial
+// — pass any laid-out element near the work; the ratio is the same everywhere.
 // Falls back to 1 for any non-positive/non-finite ratio — an unlaid-out element
 // gives 0/0 → NaN, and offsetWidth 0 with a non-zero rect gives Infinity; both
 // (and a degenerate 0-width) must read as "no zoom", not blow up a divisor.
 export function zoomScale(el) {
   const s = el.getBoundingClientRect().width / el.offsetWidth;
   return Number.isFinite(s) && s > 0 ? s : 1;
 }
+
+// Place a fixed-position popover anchored under a button, bridging `html{zoom}`:
+// getBoundingClientRect coords are post-zoom px but a fixed element's top/left/right
+// are re-scaled by zoom on paint, so divide by `scale` (from zoomScale). Returns
+// `{ top, left }`, or `{ top, right }` when `viewportW` is given (right-align to
+// the anchor's right edge). `gap` is the px below the anchor; `min` floors the
+// side inset. Pure arithmetic on a DOMRect-like — the single recipe for the File
+// menu, the Save popover and the user menu.
+export function fixedAnchor(rect, scale, opts = {}) {
+  const gap = opts.gap != null ? opts.gap : 6;
+  const min = opts.min != null ? opts.min : 8;
+  const top = rect.bottom / scale + gap;
+  return opts.viewportW != null
+    ? { top, right: Math.max(min, (opts.viewportW - rect.right) / scale) }
+    : { top, left: Math.max(min, rect.left / scale) };
+}

src/ui/explain-graph.js

@@ -8,7 +8,7 @@
 import { h, s, withDocument } from './dom.js';
 import { Icon } from './icons.js';
 import { parseDot } from '../core/dot.js';
-import { dagreLayout } from '../core/dot-layout.js';
+import { dagreLayout, schemaLayout } from '../core/dot-layout.js';
 import { buildCardModel, cardSize, CARD } from '../core/schema-cards.js';
 import { qualifyIdent } from '../core/format.js';
 import { fitBox, fitWidthBox, zoomBox, panBox, viewBoxStr } from '../core/panzoom.js';
@@ -180,7 +180,7 @@ export function buildPipelineSvg(rawText, dagre) {
 
 /** Build the schema-lineage SVG from a `{nodes,edges}` graph (kind-coloured). */
 export function buildSchemaSvg(graph, dagre, onNode) {
-  return renderGraphSvg(dagreLayout(dagre, graph || { nodes: [], edges: [] }, { isolatedLast: true }), {
+  return renderGraphSvg(schemaLayout(dagre, graph || { nodes: [], edges: [] }), {
     nodeClass: (n) => 'eg-node eg-node--' + (n.kind || 'table'),
     edgeClass: (e) => 'eg-edge eg-edge--' + (e.kind || 'feeds'),
     edgeLabel: (e) => e.kind,
@@ -248,7 +248,7 @@ export function buildRichSchemaSvg(graph, dagre, onNode) {
   });
   // `external` rides through dagreLayout (like kind/db/name), so the node class can
   // read it off the laid node — no side-channel needed.
-  const laid = dagreLayout(dagre, { nodes: sized, edges: g.edges || [] }, { isolatedLast: true });
+  const laid = schemaLayout(dagre, { nodes: sized, edges: g.edges || [] });
   // Overlay any manually-moved positions remembered for this result, then
   // straighten the edges touching a moved node so they still connect on first draw.
   const positions = g.savedPositions;

src/ui/file-menu.js

@@ -5,7 +5,7 @@
 // effect goes through an injected seam (app.saveJSON / app.saveStr /
 // app.downloadFile / app.FileReader / app.document), so it is fully testable.
 
-import { h, zoomScale } from './dom.js';
+import { h, zoomScale, fixedAnchor } from './dom.js';
 import { Icon } from './icons.js';
 import { flashToast } from './toast.js';
 import { renderSavedHistory } from './saved-history.js';
@@ -104,13 +104,11 @@ export function openFileMenu(app) {
   app.dom.fileMenu = menu;
   doc.body.appendChild(overlay);
   const r = app.dom.fileBtn.getBoundingClientRect();
-  // Bridge the shipped html{zoom}: getBoundingClientRect is post-zoom px, but a
-  // fixed element's top/left are re-scaled by zoom on paint — divide by scale so
-  // the menu anchors under the button (same as the editor popovers via zoomScale).
-  const scale = zoomScale(app.dom.fileBtn);
+  // Anchor under the button, bridging html{zoom} (see fixedAnchor / zoomScale).
+  const a = fixedAnchor(r, zoomScale(app.dom.fileBtn));
   menu.style.position = 'fixed';
-  menu.style.top = (r.bottom / scale + 6) + 'px';
-  menu.style.left = Math.max(8, r.left / scale) + 'px';
+  menu.style.top = a.top + 'px';
+  menu.style.left = a.left + 'px';
   doc.body.appendChild(menu);
   doc.addEventListener('keydown', onKey, true);
 }

tests/unit/ch-client.test.js

@@ -461,6 +461,25 @@ describe('loadLineageTransitive', () => {
     expect(out.truncated).toBe(true);
   });
 
+  it('counts only linked nodes toward the cap — standalone tables do not truncate the cross-DB walk', async () => {
+    // 'a' has one cross-DB link (a.t → b.mv) plus 4 standalone tables: 6 total nodes
+    // after round 1, but only 2 linked. nodeCap 3 must NOT truncate, and 'b' must load.
+    const ctx = ctxWith((url, init) => {
+      const sql = init.body;
+      if (/EXPLAIN AST/.test(sql) || /system\.dictionaries/.test(sql)) return jsonResp({ data: [] });
+      if (/database = 'a'/.test(sql)) return jsonResp({ data: [
+        tbl('a', 't', 'MergeTree', { dependencies_database: ['b'], dependencies_table: ['mv'] }),
+        tbl('a', 's1', 'MergeTree'), tbl('a', 's2', 'MergeTree'),
+        tbl('a', 's3', 'MergeTree'), tbl('a', 's4', 'MergeTree'),
+      ] });
+      if (/database = 'b'/.test(sql)) return jsonResp({ data: [tbl('b', 'mv', 'MaterializedView')] });
+      return jsonResp({ data: [] });
+    });
+    const out = await loadLineageTransitive(ctx, { db: 'a' }, { nodeCap: 3 });
+    expect(out.truncated).toBe(false);                                            // 2 linked < cap 3
+    expect(new Set(out.rows.tables.map((t) => t.database)).has('b')).toBe(true);  // cross-DB walk reached b
+  });
+
   it('loads a multi-database frontier concurrently in one round', async () => {
     const ctx = ctxWith((url, init) => {
       const sql = init.body;

tests/unit/dom.test.js

@@ -1,5 +1,5 @@
 import { describe, it, expect, vi } from 'vitest';
-import { h, s, withDocument, zoomScale } from '../../src/ui/dom.js';
+import { h, s, withDocument, zoomScale, fixedAnchor } from '../../src/ui/dom.js';
 
 const SVG_NS = 'http://www.w3.org/2000/svg';
 
@@ -56,6 +56,26 @@ describe('zoomScale', () => {
   });
 });
 
+describe('fixedAnchor', () => {
+  it('left-aligns under the anchor, dividing client coords by the zoom scale', () => {
+    const a = fixedAnchor({ bottom: 40, left: 100 }, 1.2);
+    expect(a.top).toBeCloseTo(40 / 1.2 + 6); // r.bottom/scale + default 6px gap
+    expect(a.left).toBeCloseTo(100 / 1.2);
+    expect(a.right).toBeUndefined();
+  });
+  it('right-aligns to the anchor when viewportW is given', () => {
+    const a = fixedAnchor({ bottom: 40, right: 1180 }, 1.2, { viewportW: 1200 });
+    expect(a.top).toBeCloseTo(40 / 1.2 + 6);
+    expect(a.right).toBeCloseTo((1200 - 1180) / 1.2);
+    expect(a.left).toBeUndefined();
+  });
+  it('floors the side inset at `min` (default 8) and honors a custom gap', () => {
+    expect(fixedAnchor({ bottom: 0, left: 0 }, 1)).toMatchObject({ top: 6, left: 8 });
+    expect(fixedAnchor({ bottom: 0, right: 1200 }, 1, { viewportW: 1200 })).toMatchObject({ right: 8 });
+    expect(fixedAnchor({ bottom: 10, left: 50 }, 1, { gap: 0, min: 0 })).toMatchObject({ top: 10, left: 50 });
+  });
+});
+
 describe('s (SVG namespace)', () => {
   it('creates elements in the SVG namespace with attrs, style, events, and children', () => {
     const onclick = vi.fn();