Skip to content

feat(harness): the project map draws the groups the rail already has [SAP-2983] - #745

Merged
gwitwer merged 5 commits into
mainfrom
georgewitwer/sap-2983-map-groups
Aug 31, 2026
Merged

feat(harness): the project map draws the groups the rail already has [SAP-2983]#745
gwitwer merged 5 commits into
mainfrom
georgewitwer/sap-2983-map-groups

Conversation

@gwitwer

@gwitwer gwitwer commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Primary change type

  • Bug fix
  • Documentation
  • Feature
  • Tests
  • Dependency update
  • Maintenance or refactor

Problem and motivation

A project map drew every agent the project contained as one flat set, ignoring the
sub-structure the rail was showing six inches to its left. Opening a root that holds
several systems produced one undifferentiated column of nodes.

Measured on a real 9-system, 76-agent tree (~/sapiom/agents), before this change:

subject 712 × 9,104 px · 0 containers · 76 nodes · 3 distinct x, 74 distinct y

That is 68 single-agent weak components stacked vertically. Fit resolves it to 10–17%,
where nothing is legible — the map is a place you cannot read.

The mechanism to fix it already existed. web/src/lib/agent-groups.ts implements
the Group axis: union-find over launch edges gives one group per connected component,
groups are user-nameable and editable, and the arrangement persists to a committable
.sapiom/studio-rail.json per project root. The map simply never read it. This PR is
mostly rendering an existing model.

Summary and scope

In

  • web/src/lib/system-graph-groups.ts (new) — joins the rail's GroupNode[] to graph
    nodes and returns an exhaustive partition. It reuses the same navigation join the
    drill-in uses, so a node you can open is a node whose group is known. Group
    membership is many-to-many by design, so a shared subagent is drawn once, under the
    first group the rail names it in. A group whose members this graph has none of is
    dropped — chrome around nothing. A node no row resolved falls to Ungrouped rather
    than off the map.
  • web/src/lib/system-graph-layout.tslayoutSystemGraph(graph, groups?). Each
    container is laid out in its own coordinates and measured after routing, so its
    box is the union of its cards, its connectors and its connector labels. Sizing from
    the cards alone is not enough: measured, a six-way fan-in already pushes two labels
    outside cards + 48px, and an edge drawn across the border it belongs inside reads as
    an edge leaving a system it never leaves. Weak components inside a container now
    shelf-pack instead of stacking, which is what stops the column reappearing inside
    Ungrouped. Containers pack the same way, in the rail's order.
  • Connectors whose ends sit in different containers are still drawn, dotted and
    dimmer, and crossing the border rather than clipped by it. A group is editable, so half
    a detected system can be pulled out; dropping the edge between the halves would make the
    map claim two systems never touch.
  • web/src/lib/use-rail-groups.ts — one arrangement per root, held at module scope.
    Two hook instances holding two copies is exactly how the rail and the map come to
    disagree: the file is the only shared medium and nothing re-reads it, so a rail edit
    would leave the map drawing what it read on mount. The groups: null / groups: []
    distinction, "derived until touched", and "a write is a consequence of an edit, never
    of an effect" are all unchanged.
  • The container label counter-scales against the view zoom, by font-size. Measured on
    the 76-agent tree, it rendered 3.65px tall at the map's own arrival zoom and 2.96px
    after Fit. The name is the entire thing a container adds; shipping it illegible at the
    altitude you zoom out to read the shape from is shipping nothing. Clamped, and 1:1 at
    70%+.

Deliberately out of scope

  • Any change to how groups are detected or persisted. agent-groups.ts is consumed,
    not modified. The map is a new reader; it never writes an arrangement.
  • The fourfold mock's other ideas: additional edge kinds, non-agent nodes, per-node
    state (cost, running dot, schedule badge), and the LENS switcher.

Related work

Related issue or discussion: SAP-2983. Supersedes the framing of SAP-2966
("graph view stacks unconnected agents into one endless column") — the fix is not better
layout for 76 orphans, it is not drawing 76 orphans. Independent of SAP-2979.

Evidence

The defect, and the fix, on the same data

/tmp/agents-2983 (a copy of the real 9-project / 76-agent tree) with a materialized
arrangement. Left: main. Right: this branch.

before — 248 × 9,728 px, 0 containers, fit to 10% after — 2,800 × 1,678 px, 9 containers
before after

The honest case: a project with nothing detected

The real ~/sapiom/agents tree, where the Group axis detects no launch edges at all (see
Found on the way below). One labelled Ungrouped container, wrapped — not a silent flat
list, and not a column.

before — 712 × 9,104 px, 0 containers after — 2,328 × 1,394 px, 1 container, 76 cards
before after

The rail and the map are one arrangement

Renaming murderbox to THE LOOP in the rail, on a real server. The map's container
follows with no reload, and the rename reaches .sapiom/studio-rail.json on disk.

https://raw.githubusercontent.com/sapiom/sapiom-js/pr-media/media/745/rail-edit-follows.webm

Review round 1 — all four findings fixed, each proven by a mutation

The automated review found four real problems. Every fix below is pinned by a test that
was confirmed to fail when the fix is reverted.

finding fix proven by
Confidentiality — comments and the changeset named a private design artifact and a third-party company The rules are stated directly instead; the changeset's measurement is generalised. A changeset cannot be edited after publish, so that one mattered most. n/a
1. The Ungrouped bucket was matched by its label text. renameGroup only trims — a user can name a real system "Ungrouped", and unresolved cards would have been filed inside it and the group moved to the end. isUngrouped is carried from the rail through SystemGraphNodeGroup and Region; the label is only ever a label now. does not mistake a group the user NAMED 'Ungrouped' for the bucket — the rail genuinely renders two rows called Ungrouped in that fixture, which is the shape the map has to survive. Reverting to the label lookup fails it.
2. A failed getRailState left the map flat and unlabelled forever while the rail kept showing the derived groups — the exact divergence this feature removes. Split the signals: isReady stays the WRITE gate (false forever after a failed read, deliberately); the new hasSettled is the DRAW gate (the read finished, either way). The map uses hasSettled. e2e a project whose arrangement cannot be READ still draws its groups, via a new mock-only __MOCK_RAIL_STATE_FAIL__ knob beside the existing __MOCK_SYSTEM_GRAPH_FAIL_ONCE__. It also asserts the rail is correctly not editable in that state — which is why the two gates cannot be one. Reverting to isReady fails it.
3. The rail file was read once per page, ever — a failed read was never retried, and a committable file was never re-read after a branch switch or hand edit. "Have I asked for this root yet" goes back to a per-surface ref; only the DATA is shared. A failed read also clears its own latch. Two surfaces now issue two GETs of a cheap idempotent read. e2e a read that failed is tried again when the map is reopened — fails the read, drills into an agent and back out, and asserts the map picks up the stored arrangement. Re-sharing the latch fails it.
4. The counter-scaled label overflowed its own container. transform: scale() does not affect layout, so the label's on-screen width stayed constant as the container's shrank; below ~70% a long name drew over its neighbour, invisible to a check that measures boxes only. Grows by font-size instead, so the line re-lays out and max-width + ellipsis still apply. GROUP_HEADER is sized for the largest line the clamp can produce. e2e a container's name stays inside its own box at every zoom, asserted at the far end of the clamp against both the container edges and every card rect. Reverting to transform fails it.

Nits fixed too: the layout test's docblock named a file that does not exist, and every
container had the same data-testid (now system-graph-group-<id>, unique).

Re-verified after the fixes: typecheck clean, 2652 unit tests pass (same one pre-existing
macOS failure), 471 e2e pass, and both real servers re-measured —
76/76 cards inside exactly one container, 0 homeless, 0 double-claimed, 0 overlapping
on the 9-container tree and on the single-Ungrouped one.

Review round 2 — both new findings fixed

Round 1's fix split the arrangement (shared, so the two surfaces agree) from the request
latch (per surface, so a remount re-reads). The follow-up review found both consequences
of that split. Each fix is again pinned by a test confirmed to fail when reverted.

finding fix proven by
A mount-time read could clobber an in-flight edit. Opening the map issues its own GET, which races a 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 reverts, and the next edit then materializes from the reverted state and persists it, losing the edit on disk too. A root this page has WRITTEN to is never re-read. Once we have written it, the in-memory arrangement is the newer answer and a read can only be older. e2e opening the map cannot undo an edit the rail just made — it holds the write open so the race is deterministic rather than a matter of who wins, renames a group, opens the map, then releases. Removing the guard fails it.
The whole-tree grep was re-issued on every mount. Round 1's finding was about the rail file; moving the launch-edge latch per-instance too meant every drill-in re-scanned every registered agent's sources. The edge latch goes back to module scope — its answer is install-wide and there is nothing per-root to re-read — and is released on failure so a later mount still retries. The per-root file reads stay per surface. Measured on the real 76-agent server by counting HTTP calls across 3 drill-in/out cycles: 4 → 1 GET /api/studio-rail/launch-edges, while GET /api/studio-rail?root= stays at 4 (one per mount, as intended).

Nits fixed: a comment describing a transform-origin removed in the previous push, and
toRegions now has its own test for recognising the bucket by identity rather than by the
label "Ungrouped" — only the mapper half of that fix was pinned before.

Also measured, since the edge grep now gates the containers: cold GET /api/studio-rail/launch-edges over 76 agents is 170ms (106ms warm), against a graph
projection that takes seconds — so the containers land on the same frame as the cards
(measured: 2,844ms and 2,844ms, gap 0ms). There is no card-then-container reflow.

Validation

Driven against a real harness on its own port and state root — ~/sapiom/agents
(76 agents / 9 projects) on :5466 and a node_modules-free copy on :5467 — with
Playwright reading computed geometry out of the DOM, not inferred from source.

# real server, ~/sapiom/agents — before (main) → after (this branch)
subject 712 × 9104, groups 0, nodes 76        →  subject 2328 × 1394, groups 1, nodes 76
                                                 76/76 cards inside exactly one container
                                                 0 homeless, 0 double-claimed, 0 overlapping

# real server, materialized 9-group arrangement
map container labels  == rail group row labels, same order:
  probes murderbox team-bots outreach property-ops research social-content support devtools
subject 2800 × 1678 · 9 containers in 3 rows × 6 columns · 0 px horizontal overflow
76/76 cards inside exactly one container · 0 homeless · 0 double-claimed · 0 overlapping

# rail rename, live, no reload
map before: [probes, murderbox, ...]   map after: [probes, THE LOOP, ...]
rail after: [probes, THE LOOP, ...]    file on disk: [probes, THE LOOP, ...]

# container label legibility, measured at the map's own arrival zoom
before: 3.65 px tall (2.96 px after Fit)  →  after: 11.83 px

pnpm --filter @sapiom/harness typecheck   — clean
pnpm --filter @sapiom/harness lint        — 1 pre-existing warning in src/server/rest.test.ts
pnpm --filter @sapiom/harness test        — 2651 passed, 1 failed
    src/server/system-graph-freshness.test.ts fails identically on clean main on macOS
    (filesystem-watcher test from #724); Linux CI reports the same commit green.
pnpm terminology:check                    — passed (457 files)
pnpm examples:check                       — passed
E2E_PORT=5468 pnpm --filter @sapiom/harness test:ui  — 468 / 471 passed on this machine
    while it was quiet (one run: 467 + 1 rich-step-detail failure, green in isolation
    and on re-run — the known flake band).

A note on the local e2e numbers, so they are not read as more than they are. Later runs
on this machine degraded badly — 460/12 in 17 minutes, then 445/27 in 51 minutes — at load
averages of 42–49, with failures scattered across templates, unrooted-agents,
snippet-panel, step-macros and the command palette, none of them in this diff and none
reproducible in isolation. The machine was running several suites at once. CI is the
signal, and playwright-mock is green on this exact commit.
This PR's own nine specs
were re-run under that same load and passed in 8.9s.

Mutation testing

A count-only assertion passes when nothing happened. Every guard added here was mutated
and confirmed to fail, then restored:

mutation caught by
layout ignores the groups it is handed 5 layout tests + all 5 e2e specs
container sized from cards only (no edge/label union) keeps a group's own wiring and labels inside its border
shelf packing reverted to a vertical stack wraps a container of unconnected agents instead of stacking them
cross-container edges dropped draws an edge whose ends the user split across two groups, e2e an edge whose ends the user split across groups is still drawn
unclaimed nodes silently dropped still draws a node no group claimed
mapper ignores the stored arrangement 6 systemGraphNodeGroups tests
container boxes left out of the layout bounds e2e none is drawn outside the map's own bounds
use-rail-groups reverted to the pre-fix per-instance state e2e a rail edit moves the map, with no reload + the cross-group spec
Ungrouped bucket found by label text again does not mistake a group the user NAMED 'Ungrouped' for the bucket
the map gated on the write gate again e2e a project whose arrangement cannot be READ still draws its groups
the request latch shared again (no retry, no re-read) e2e a read that failed is tried again when the map is reopened
container label back to transform: scale() e2e a container's name stays inside its own box at every zoom
the write guard removed from the load effect e2e opening the map cannot undo an edit the rail just made
toRegions back to the label lookup does not file an unclaimed node into a group merely NAMED Ungrouped

Two mutations that did NOT fail, reported rather than rounded up:

  1. My first version of keeps a group's own wiring inside its border used a 3-node cycle
    and survived the card-sized-container mutation — 48px of padding already covered a
    cycle's 44px gutter, so the assertion was not proving its own claim. I measured for a
    shape that actually overflows (a six-way fan-in pushes labels past cards + 48), and
    the rewritten test now fails the mutation. The original assertion is kept separately
    and is honestly labelled as the weaker one.
  2. Dropping railGroups from the useMemo deps in WorkspaceGraphView survived the
    e2e suite — the memo is recomputed anyway because navigation depends on workflows,
    whose identity changes on state updates. So that dep is belt-and-braces, not the
    mechanism. The mechanism is the shared store, and reverting use-rail-groups.ts to
    its pre-fix per-instance form does fail the propagation specs. Worth recording that
    with per-instance state the first three specs still pass: each surface reads the file
    itself on mount, so only live propagation is what sharing buys.

Not covered by a browser test

WorkspaceGraphView passes undefined (not []) while the arrangement is still in
flight, so the map cannot flash one Ungrouped container over the whole project and then
rearrange. VITE_MOCK=1 serves rail state from a local method with no network request, so
there is no way to hold it open from a spec without adding a mock-only knob to api.ts
(a file another epic owns this week). The undefined branch itself is pinned by
draws no container at all when it was given no groups; the wiring is not.

Found on the way — a real divergence, not fixed here

The rail's Group axis and the system map read different edge sets, despite comments in
both files asserting they read one graph:

  • src/server/studio-rail.ts:detectLaunchEdges calls detectWorkflowLaunches, which is
    scanWorkflowSources(...).launches — and that filters to invocation.mode === "async"
    (src/core/canvas-interconnections.ts:626-628), i.e. agents.launch only.
  • The system graph uses all invocations, blocking included.

On the real 76-agent install every cross-agent call is ctx.sapiom.agents.run (blocking),
so GET /api/studio-rail/launch-edges returns 0 edges while the map's own projection
has 8. The rail therefore detects no groups at all there, and the map — correctly —
matches it with a single Ungrouped container.

Changing that is explicitly out of scope for this issue ("any change to how groups are
detected"), and it is a one-line filter in a file this PR does not touch. Flagging it for
a follow-up issue: the Group axis is currently blind to the launch shape a real install
actually uses.

Tests and documentation

  • web/src/lib/system-graph-groups.test.ts (new, 8 cases) — the rail→map join, including
    a case that fails if groups: null and groups: [] are ever collapsed, and one that
    fails if the map re-derives instead of reading the stored arrangement.
  • web/src/lib/system-graph-layout.test.ts (+8 cases) — container geometry, containment,
    non-overlap, wrapping, cross-group edges, determinism.
  • web/e2e/project-map-groups.spec.ts (new, 5 specs) — the claims only a browser can
    settle, all measured from computed geometry.
  • No doc changes: docs/workspace-system-graph.md describes projection, which is
    unchanged; grouping rules stay documented where they live, in agent-groups.ts.

Compatibility and release impact

  • Breaking or externally visible changes: None to any API or wire format. The map's
    rendering changes; no route, payload, or stored-file shape does. The map never writes
    .sapiom/studio-rail.json.
  • Changeset: Added — .changeset/project-map-groups.md.

Security

  • I have not included secrets, credentials, private data, or unsanitized logs.
  • This pull request does not publicly disclose a suspected vulnerability. I
    will follow the
    Security Policy for
    private reporting.

AI assistance

  • I did not use AI assistance for this change.
  • I used AI assistance and have described it below.

Claude Code (Opus 5) wrote the implementation, tests and this description. Verification was
not taken on trust: every guard was mutation-tested (table above, including the two that
failed to fail), and every geometric claim was measured in a browser driven against a real
harness on a real 76-agent tree rather than against VITE_MOCK fixtures.

Checklist

  • I read CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.
  • This pull request addresses one focused problem and contains no unrelated cleanup.
  • I added or updated tests, or explained above why tests are not applicable.
  • I ran the relevant build, typecheck, lint, and test commands, or explained
    any N/A checks above.
  • I updated documentation for user-facing changes, or marked it N/A above.
  • I added a Changeset for a published-package change, or explained why it is not applicable.
  • I can explain and maintain every submitted change, including any AI-assisted work.

gwitwer and others added 3 commits August 30, 2026 09:36
…[SAP-2983]

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91
…rom [SAP-2983]

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91
…hangeset [SAP-2983]

`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91
@github-actions

Copy link
Copy Markdown
Contributor

Review — PR #745 (feat(harness): the project map draws the groups the rail already has)

🔒 CONFIDENTIALITY — internal design reference and a third-party company name in published prose

Three new comments describe a private design artifact and name a company that is not Sapiom and
is not in the pinned public-provider vocabulary (scripts/provider-neutral-copy-check.mjs):

  • packages/harness/web/src/lib/system-graph-layout.ts, routeCrossGroupEdges docblock:
    "the design reference draws them the same way — the connector out of "THE LOOP" to TikTok
    crosses its border."
  • packages/harness/web/src/styles.css, .system-graph-group: "the shape the fourfold map
    reference draws around "THE LOOP""
    .
  • SystemGraphCanvas.tsx: "the shape the design reference draws".

This repo is public. Cite the rule, not the artifact: "a connector between two containers is drawn
crossing the border rather than clipped by it." Drop THE LOOP, fourfold, and TikTok.

Related, lower confidence but same root cause: styles.css cites "a real project … measured on a
9-system, 76-agent tree"
and the changeset opens with "a nine-system, 76-agent folder came out
9,700 pixels tall"
. If that measurement came from anything other than a Sapiom-owned repo, the
changeset is the one surface that can never be edited after publish. Generalise to
"a project with dozens of agents".

Correctness

1. The Ungrouped bucket is matched by label text, not by identity.
system-graph-groups.ts looks the bucket up with container.label === SYSTEM_GRAPH_UNGROUPED_LABEL,
then splices it out and re-appends it last with the unresolved nodes merged in. GroupNode already
carries isUngrouped (agent-groups.ts:57) and UNGROUPED_ID, and renameGroup only trims — it
does not reject the string "Ungrouped".
Failure: a user renames (or creates) a group "Ungrouped" in the rail. Agents that failed the
navigation join are drawn inside that named system, and the group jumps to the end of the map —
breaking the exact rail-order invariant this PR's own tests assert. Match on group.isUngrouped
here, and on the group id in toRegions (system-graph-layout.ts), which has the same label-string
lookup.

2. A failed getRailState silently un-groups the map while the rail stays grouped.
The .catch in use-rail-groups.ts sets states but deliberately not loaded, so isReady is
false forever. WorkspaceGraphView then passes groups: undefined permanently and the map falls
back to the pre-PR flat layout — while WorkflowsRail.tsx:1211 renders groupsFor regardless of
isReady and still shows the derived groups.
Failure: read-only checkout or any 5xx on that route → rail shows nine named systems, map shows
one unlabelled blob, no error anywhere. That is the divergence the ticket exists to remove. The map
needs a separate readiness signal (edges landed + read settled), not the write-safety gate.

3. The rail file is now read once per page, ever.
requested/loaded/states moved from per-hook refs to a module-level store that is never
invalidated. Previously unmounting and remounting the rail re-read .sapiom/studio-rail.json; now
nothing does.
Failure: the file is explicitly committable — git checkout to another branch, or editing it by
hand, leaves both surfaces drawing the old arrangement until a full reload. A failed read is
likewise never retried.

4. The counter-scaled container label overflows its own container.
.system-graph-group-label uses transform: scale(clamp(1, calc(0.7 / var(--system-graph-zoom)), 4))
with transform-origin: 0 0. Transform does not affect layout, so the label's on-screen width is
0.7 × naturalWidth(layout px) independent of zoom, while its container's on-screen width is
W × zoom. Below ~70% zoom the two diverge.
Failure: at the ~20% arrival zoom this comment itself cites, a group name wider than ~0.29 × W
draws past its own box and over the neighbour — GROUP_GAP (80 layout px) is ~16 screen px there.
The new e2e overlap check measures .system-graph-group rects only, so it cannot see this. Clamp
against the container width, or scale the label's reserved box too.

Nits

  • system-graph-layout.test.ts's new docblock points at project-map.spec.ts; the file added is
    project-map-groups.spec.ts.
  • Every container renders data-testid="system-graph-group", so getByTestId is a strict-mode
    violation for anyone who reaches for it; the specs work around it with a class selector.

Verdict: request changes — remove the design-reference/company names from the public prose, and
fix the label-string identity of the Ungrouped bucket (1) and the silent map/rail divergence on a
failed read (2). No published API surface, dependency, or tarball change; the minor changeset
level is right.

…f its own [SAP-2983]

- 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91
@github-actions

Copy link
Copy Markdown
Contributor

Follow-up review — PR #745 (delta since 76865ad)

All four round-1 findings are fixed, and the confidentiality issue is gone from every
published surface in the diff (changeset, styles.css, SystemGraphCanvas.tsx,
system-graph-layout.ts). Two new problems come out of the fix to round-1 finding 3.

New — a mount-time read can clobber an in-flight rail edit

use-rail-groups.ts: states is now module-level (shared) while the request latch is
per instance, so every mount re-reads and the result overwrites shared state
unconditionally (store.states.set(root, readRailState(raw, …))), including an
optimistic edit commit just made. Before this PR the two were per instance together,
so a remount's read only landed in a fresh, unedited instance.

Failure: drag an agent between groups in the rail (optimistic state + queued PUT),
then click a project to open the map. The map's GET races the PUT; if it is served
first, the read replaces the arrangement with the pre-edit file — the rail visibly
reverts, and the next edit materializes from the reverted state and persists it, so
the drag is lost on disk too. Skip (or discard) a read for a root with an entry in
writeChain.

New — the launch-edge grep is now re-issued on every mount

The same commit moved edgesRequested from the store to a per-instance ref, but the
round-1 finding was about the rail file, not the edges. listLaunchEdges greps every
registered agent's sources (per its own comment) and its result is install-wide and
already cached in store.edges, so the refetch buys nothing. WorkspaceGraphView
unmounts on every drill-in — as the new retry spec demonstrates — so each navigation
into a project triggers a full source scan of the install. Keep this latch module-level,
or skip when store.edges !== null.

Nits

  • styles.css:4272 still says "transform-origin pins it to the corner it already sits
    in"; the transform/transform-origin pair was removed in this push.
  • system-graph-layout.test.ts's group() helper sets isUngrouped: label === "Ungrouped", so the layout half of the round-1 finding-1 fix (toRegions matching on
    identity) is not independently pinned — only the mapper half is.

Verdict: request changes — the round-1 fixes hold, but the shared-state/per-instance-latch
split introduces a lost-edit race and an unnecessary repeated whole-tree scan.

…per page [SAP-2983]

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YESzo9dE2sqMWX9z73PQ91
@gwitwer
gwitwer merged commit 5745707 into main Aug 31, 2026
12 checks passed
@gwitwer
gwitwer deleted the georgewitwer/sap-2983-map-groups branch August 31, 2026 03:33
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