Skip to content

fix(harness): wrap isolated graph agents - #749

Merged
ynadge merged 1 commit into
mainfrom
yashnadge/sap-2966-studio-graph-view-stacks-unconnected-agents-into-one-endless-column
Aug 31, 2026
Merged

fix(harness): wrap isolated graph agents#749
ynadge merged 1 commit into
mainfrom
yashnadge/sap-2966-studio-graph-view-stacks-unconnected-agents-into-one-endless-column

Conversation

@ynadge

@ynadge ynadge commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix SAP-2966 by preserving the current layered and grouped layout for agents with detected relationships, then placing globally degree-zero agents in deterministic labelled grids beneath the connected content in their region.

Changes

  • classify isolation against the complete graph before group regions are laid out, so endpoints whose only edge crosses a group boundary are not mislabeled
  • preserve the connected component geometry and the group containers introduced on main
  • sort isolated agents by stable ID and pack them into viewport-independent landscape grids
  • render N agents · no detected relationships inside the pannable graph subject and include every label in graph and group bounds
  • route connected edges without provisional isolated cards, then keep each isolated section below every final routed primitive
  • add the 77-agent/4-edge bounded-height regression, dense-label collision coverage, grouped-layout integration coverage, and singular/plural browser coverage
  • add a patch changeset for @sapiom/harness

Testing

  • 77-agent/4-edge regression: 1,620×912, 8×9 isolate slots, and 67% fit zoom in a 1,200×800 viewport
  • pnpm vitest run web/src/lib/system-graph-layout.test.ts (21/21)
  • pnpm --filter @sapiom/harness typecheck
  • pnpm --filter @sapiom/harness build
  • targeted Playwright coverage for mixed and all-isolated graphs (2/2)
  • harness lint: zero errors; one pre-existing unused-import warning in src/server/rest.test.ts
  • CI for rebased head
  • real-installation data run: the copied-state attempt timed out when the Conductor Mac command bridge became unavailable

Scope

No collapsing, filtering, new relationship semantics, or warning-detail UX is included.

Related

Closes: SAP-2966

Checklist

  • Code follows repository design and layout conventions
  • No new design-system tokens or primitives
  • No hardcoded secrets
  • Self-reviewed

@github-actions

Copy link
Copy Markdown
Contributor

Review — PR #749 fix(harness): wrap isolated graph agents

🔒 Confidentiality: clean. The changeset, the new label copy, the CSS, and both e2e
fixtures name no company, customer, codename, internal host, or commercial arrangement.
patch for a SPA-only layout change is the right level; web/ is not in the package's
files, so SystemGraphIsolatedSection adds no published type surface.

Findings

1. Edge labels are not told the isolated-section label exists — they can be drawn on top of it

packages/harness/web/src/lib/system-graph-layout.ts:757

isolatedSection is computed in placeNodes but is never passed to routeEdges /
chooseLabelBounds. That function excludes candidates that hit nodeBounds (which now
correctly includes the isolated cards) and previously placed edge labels — the section
label is in neither set. When the six preferred candidates are all taken, the fallback
loop walks downward from componentBox.maxY + 8 + slot * (LABEL_HEIGHT + 4) with
(allNodes.length + existing.length + 1) * 6 slots, i.e. effectively unbounded, while the
section label sits at exactly lastComponentBottom + COMPONENT_GAP (64) with
height: 16. Slots 3–4 land inside that band, and the horizontal spans overlap whenever
the section grid is wider than the last component (72 isolates ⇒ 1,164px wide, so almost
always).

Failure: a dense last component that exhausts its preferred label slots renders
blocking + async stacked on 72 agents · no detected relationships, both with
pointer-events: none, so neither is readable and neither can be dismissed. Fix: thread
isolatedSection?.labelBounds into routeEdges and seed the labels accumulator with
it, or reserve the label band by advancing yCursor past the widest fallback reach.

2. The bounded-height regression test does not actually pin the fix

packages/harness/web/src/lib/system-graph-layout.test.ts:222-225

The two assertions that encode the point of the PR are
bounds.height <= bounds.width * 2 and fitSystemGraphView(...).zoom > SYSTEM_GRAPH_FLOOR_ZOOM — and SYSTEM_GRAPH_FLOOR_ZOOM is 0.1. A future change that
regressed the 77-agent map to ~3,000px tall (zoom ≈ 0.27, aspect ≈ 2.4 with the current
1,228px width — borderline) would still pass the zoom check. The PR body states the
measured result is 1,228 × 1,140 at 60% zoom; assert that: expect(zoom) .toBeGreaterThanOrEqual(0.5) and a concrete bounds.height ceiling. As written the test
proves "not catastrophic", not "bounded".

Nits

  • system-graph-layout.ts:441 writes a componentBoxes entry per isolated node; nothing
    reads them (only edge seeds look boxes up, and isolated nodes have no edges) — 72 dead
    map entries on a sparse map.
  • styles.css:4240 pins the label to height: 16px from LABEL_HEIGHT while the text is
    sized by var(--type-meta); if that token ever exceeds 16px the glyphs overflow the box
    into the 12px ISOLATED_SECTION_LABEL_GAP with no clipping.
  • The label is a bare <p> with pointer-events: none and no aria association to the
    grid it heads; screen-reader users get 72 unattributed cards.

Verdict

Approach is sound and the connected-layout invariance is well tested. Request changes for
finding 1 (a real collision, not hypothetical geometry) and tighten finding 2 before merge.


Note: I could not run vitest in this sandbox, so finding 1 is reasoned from the code
paths rather than reproduced; the geometry is arithmetic from the constants in the diff.

@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #749 (delta since bbf65ca6)

One commit moved: 6788dbbc fix(harness): keep graph section clear of labels.

🔒 Confidentiality: clean. No prose surfaces changed in the delta — the changeset,
README/JSDoc and fixtures are untouched.

Earlier findings

  • Finding 1 (edge labels drawn over the section label) — fixed, and fixed at the right
    layer.
    routeEdges now receives only connected nodes, and
    placeIsolatedSectionBelowEdges (system-graph-layout.ts:784) translates the section
    label and its cards down to max(routedBottom, …) + COMPONENT_GAP, where
    routedBottom is the max over every routed point and every labelBounds bottom. Paths
    are straight polylines (pathFromPoints), so there is no curve bulge the point scan
    misses. Isolated components are provably zero-edge (connected: componentEdges.length > 0 on a weak component ⇒ singleton), so dropping them from routeEdges cannot orphan an
    endpoint. Side benefit the new test now pins: edge geometry is byte-identical with and
    without isolates.
  • Finding 2 (regression test did not pin the fix) — fixed. bounds.height <= 1_200
    and zoom >= 0.5 replace the aspect/floor-zoom checks.
  • Nit: dead componentBoxes entries for isolated nodes — removed.

Not fixed (both were nits, both still stand)

  • styles.css:4240 still pins the label to height: 16px while sizing text with
    var(--type-meta); a token bump overflows with no clipping.
  • The section label is still a bare <p pointer-events: none> with no aria association
    to the grid it heads.

New

Nothing. I could not execute vitest in this sandbox, so the two behavioural assertions
in the new test (sparse.edges equality, label/section non-overlap) are reasoned from the
constants rather than observed green — both hold arithmetically, including the dx/dy
equality in shiftLayout that the node-equality assertion depends on.

Verdict

Both blocking findings resolved correctly; ship it.

Preserve layered connected components while packing zero-degree agents into a deterministic labeled grid below them.

Closes: SAP-2966
@ynadge
ynadge force-pushed the yashnadge/sap-2966-studio-graph-view-stacks-unconnected-agents-into-one-endless-column branch from 6788dbb to 4b122b3 Compare August 31, 2026 05:08
@ynadge
ynadge merged commit bf380c3 into main Aug 31, 2026
12 checks passed
@ynadge
ynadge deleted the yashnadge/sap-2966-studio-graph-view-stacks-unconnected-agents-into-one-endless-column branch August 31, 2026 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant