feat(#65): render typed cross-scope connections + far-end proxies#72
Merged
Conversation
- Bulletproof-plan skill now documents that plans must include the full SDLC envelope: branch creation → implementation → commit/push → CodeRabbit review + self-review → autofix → commit → PR - Adds detailed implementation plan for #65 (typed cross-scope connection rendering with boundary proxies) - Quality bar updated to emphasize that plans end in a PR, not just implementation steps
Make the Canvas consume the cross-scope read shape #63 derives (ADR-0031): Connections now attach to each endpoint's on-scope representative, arrowheads derive from the interaction via a shared pure helper, and off-scope endpoints render as read-only boundary-proxy passive nodes. - `~/lib/connection-direction.ts` (`arrowEnds`): the canonical, framework-agnostic interaction→arrowheads helper (ADR-0027), shared with the exporter (#67). Canvas maps it to React Flow markers in `toRFEdge`; `@xyflow/react` stays out of `~/lib`. - Boundary proxies render as passive `boundary-proxy` nodes (re-introducing the `isPassiveNode`/`CanvasRFNode` extension point per ADR-0016, in its per-edge form without the retired boundary-group). Lineal/ingress proxies are labelled "Inbound from {ancestor}" so they don't read as the host inside itself. A "Go to {title}" affordance navigates to the off-scope Component's own scope. - Interaction picker: an inline segmented control on the selected Connection upgrades its interaction. Backed by a new `updateEdgeInteraction` mutation/service that re-checks the directional de-dupe key (mirroring `connectNodes`, with the edge's own id excluded) and returns ConflictError on a collision; draw order is preserved. `updateEdge` stays label-only. - Fix the incident-edge removal to match on reprs (not raw endpoint ids) and clean up orphaned proxies on Component delete/undo — the repr swap would otherwise strand cross-scope edges/proxies in the store. - Docs: CONTEXT.md tense flips (realized in #65); ADR-0027 amended with interaction-edit semantics; ADR-0031/0016 realized notes; stale ADR-0023 / boundary-group comments fixed. Verified: `pnpm check` green; new helper + updateEdgeInteraction service tests pass; dev-browser confirmed arrowheads (ASSOCIATION plain, REQUEST/SUBSCRIBE directional), both proxy types with correct labels, the picker round-trip, and go-to-real navigation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughCanvas now renders typed cross-scope connections with interaction-derived arrowheads, passive boundary-proxy nodes with “descend to real”, an inline per-edge interaction picker with optimistic commits and conflict-aware rollback, a server mutation for interaction updates, tests, and documentation/plan updates. ChangesTyped Cross-Scope Connection Rendering
Sequence Diagram(s)sequenceDiagram
participant ConnectionEdgeView
participant Canvas
participant ArchitectureRouter
participant EdgeService
participant DB
ConnectionEdgeView->>Canvas: onSetInteraction(edgeId, interaction)
Canvas->>ArchitectureRouter: updateEdgeInteraction mutation(input)
ArchitectureRouter->>EdgeService: updateEdgeInteraction(db, actor, input)
EdgeService->>DB: activeDuplicateWhere check / db.update
DB-->>EdgeService: Edge or uniqueness error
EdgeService-->>ArchitectureRouter: Edge | ConflictError
ArchitectureRouter-->>Canvas: mutation result
Canvas-->>ConnectionEdgeView: optimistic success or rollback/toast
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Self-review found that commitEdgeEdit and commitEdgeInteraction both restored the full CanvasEdge on rollback, so a failed label edit could clobber a concurrently-succeeded interaction change (and vice versa). Roll back only the field each path wrote, reading from the CURRENT cache row, and gate on the cache still showing that write — the field-scoped analogue of commitRename's conditional rollback. Also gate the empty-canvas hint on real Components so an inbound-proxy-only scope still shows it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/p/[slug]/_canvas/canvas.tsx (1)
1168-1184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback should merge edge fields instead of restoring whole snapshots.
Both failure paths write
prevback as a fullCanvasEdge. That means a newer successful change to the other field gets lost: a failed label save can revert a newer interaction/marker update, and a failed interaction save can revert a newer label edit. Roll back only the field owned by the failing mutation, using the current cached edge as the base.Possible fix
- if (prev) { - setEdges((es) => es.map((e) => (e.id === id ? toRFEdge(prev) : e))); - patchCanvas((c) => ({ - interiorEdges: c.interiorEdges.map((e) => (e.id === id ? prev : e)), - })); - } + const current = utils.architecture.getCanvas + .getData(canvasInput) + ?.interiorEdges.find((e) => e.id === id); + if (current?.label === label) { + const rolledBack = { ...current, label: prev.label }; + setEdges((es) => + es.map((e) => (e.id === id ? toRFEdge(rolledBack) : e)), + ); + patchCanvas((c) => ({ + interiorEdges: c.interiorEdges.map((e) => + e.id === id ? { ...e, label: prev.label } : e, + ), + })); + }- if (current?.interaction === interaction) { - setEdges((es) => es.map((e) => (e.id === id ? toRFEdge(prev) : e))); - patchCanvas((c) => ({ - interiorEdges: c.interiorEdges.map((e) => (e.id === id ? prev : e)), - })); - } + if (current?.interaction === interaction) { + const rolledBack = { ...current, interaction: prev.interaction }; + setEdges((es) => + es.map((e) => (e.id === id ? toRFEdge(rolledBack) : e)), + ); + patchCanvas((c) => ({ + interiorEdges: c.interiorEdges.map((e) => + e.id === id ? { ...e, interaction: prev.interaction } : e, + ), + })); + }Also applies to: 1205-1223
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/p/`[slug]/_canvas/canvas.tsx around lines 1168 - 1184, The rollback currently overwrites the entire CanvasEdge with the stored prev snapshot, losing concurrent updates; change the failure handlers (the block around editEdge usage and the similar block at ~1205-1223) to merge only the mutated field back into the current cached edge: on label-save failure take the current edge (from setEdges/patchCanvas or c.interiorEdges) and create merged = { ...current, label: prev.label } and use toRFEdge(merged) for setEdges and merged for patchCanvas; do the analogous merge for interaction/marker-save failures (only restore the specific interaction field from prev), ensuring you reference prev, next, setEdges, toRFEdge, patchCanvas and interiorEdges when locating and updating the code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/p/`[slug]/_canvas/canvas.tsx:
- Around line 817-827: The insertion logic in setNodes that maps incidentProxies
via toProxyRFNode currently places new proxies at y = i * 72 which can overlap
existing boundary proxies; change it to compute an offset based on the current
passive/proxy nodes in ns (e.g., count existing proxies or compute the max Y of
existing proxy nodes) and add that offset to the new node Y so each restore path
appends after existing rail entries; update both occurrences (the setNodes block
and the similar block around lines 1104-1112) to use the same offset calculation
and preserve prior rendered positions when available, referencing setNodes,
incidentProxies, toProxyRFNode, breadcrumbIds, ns and i in your fix.
In `@src/server/architecture/__tests__/edge.service.test.ts`:
- Around line 549-567: The test currently only checks that updateEdgeInteraction
throws a ConflictError; tighten it by asserting that the thrown error's
details.conflictingEdgeIds includes the pre-existing connection's id (the one
created via connectNodes) and that it does not include the upgraded edge's id
(edge.id). Locate the test using seedAssociation(), connectNodes(...), and
updateEdgeInteraction(...), capture the thrown error from updateEdgeInteraction,
and add assertions on error.details.conflictingEdgeIds (and/or its length) to
ensure the collision reports the expected conflicting edge id(s).
- Around line 569-583: The test currently calls updateEdgeInteraction on the
same seeded ASSOCIATION (seedAssociation) which converts and removes the
association row, so it never exercises an ASSOCIATION and a directional REQUEST
coexisting; change the test to keep the original association and create a new
directional edge instead of updating the existing one: after const { actor, a,
b, edge } = await seedAssociation(), call the function used to create a new edge
(e.g., createEdge or whatever helper creates an edge with interaction "REQUEST")
to insert a new A→B REQUEST row (do not call updateEdgeInteraction on the seeded
edge), then assert the newly created edge has interaction "REQUEST" and
sourceId/targetId match a.id/b.id and that the original association still exists
if needed.
---
Outside diff comments:
In `@src/app/p/`[slug]/_canvas/canvas.tsx:
- Around line 1168-1184: The rollback currently overwrites the entire CanvasEdge
with the stored prev snapshot, losing concurrent updates; change the failure
handlers (the block around editEdge usage and the similar block at ~1205-1223)
to merge only the mutated field back into the current cached edge: on label-save
failure take the current edge (from setEdges/patchCanvas or c.interiorEdges) and
create merged = { ...current, label: prev.label } and use toRFEdge(merged) for
setEdges and merged for patchCanvas; do the analogous merge for
interaction/marker-save failures (only restore the specific interaction field
from prev), ensuring you reference prev, next, setEdges, toRFEdge, patchCanvas
and interiorEdges when locating and updating the code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79875a4c-6743-46fc-8fa6-b878e7c6e0ee
📒 Files selected for processing (19)
.claude/skills/bulletproof-plan/SKILL.mdCONTEXT.mddocs/adr/0016-passive-nodes-and-boundary-group-n1-stability.mddocs/adr/0027-connection-carries-its-own-interaction.mddocs/adr/0031-cross-scope-read-derivation-and-per-edge-boundary-proxy.mddocs/plans/typed-cross-scope-connection-rendering.mdsrc/app/p/[slug]/_canvas/boundary-proxy.tsxsrc/app/p/[slug]/_canvas/canvas.tsxsrc/app/p/[slug]/_canvas/component-node.tsxsrc/app/p/[slug]/_canvas/connection-edge.tsxsrc/lib/connection-direction.test.tssrc/lib/connection-direction.tssrc/lib/connection-rules.tssrc/lib/interactions.tssrc/lib/node-kinds.tssrc/lib/schemas.tssrc/server/api/routers/architecture.tssrc/server/architecture/__tests__/edge.service.test.tssrc/server/architecture/edge.service.ts
Comment on lines
+569
to
+583
| it("allows a directional upgrade even when an ASSOCIATION shares the unordered pair", async () => { | ||
| // The seeded ASSOCIATION A↔B and a new A→B REQUEST occupy different de-dupe | ||
| // slots (unordered vs. ordered triple), so upgrading a SECOND association is | ||
| // only blocked by a matching directional row, not by the association itself. | ||
| const { actor, a, b, edge } = await seedAssociation(); | ||
|
|
||
| const updated = await updateEdgeInteraction(testDb, actor, { | ||
| id: edge.id, | ||
| interaction: "REQUEST", | ||
| }); | ||
|
|
||
| expect(updated.interaction).toBe("REQUEST"); | ||
| expect(updated.sourceId).toBe(a.id); | ||
| expect(updated.targetId).toBe(b.id); | ||
| }); |
There was a problem hiding this comment.
This test does not exercise the scenario named in its title.
After seedAssociation(), updating that same edge to "REQUEST" removes the association row, so there is never an association concurrently sharing the unordered pair. As written, this mostly duplicates the earlier happy-path upgrade test instead of proving association/directional slot separation.
Example of a case that actually keeps an association alive
- it("allows a directional upgrade even when an ASSOCIATION shares the unordered pair", async () => {
- // The seeded ASSOCIATION A↔B and a new A→B REQUEST occupy different de-dupe
- // slots (unordered vs. ordered triple), so upgrading a SECOND association is
- // only blocked by a matching directional row, not by the association itself.
- const { actor, a, b, edge } = await seedAssociation();
-
- const updated = await updateEdgeInteraction(testDb, actor, {
- id: edge.id,
- interaction: "REQUEST",
- });
-
- expect(updated.interaction).toBe("REQUEST");
- expect(updated.sourceId).toBe(a.id);
- expect(updated.targetId).toBe(b.id);
- });
+ it("allows changing a directional interaction while an ASSOCIATION shares the same pair", async () => {
+ const { actor, project, a, b } = await seedAssociation();
+ const directional = await connectNodes(testDb, actor, {
+ projectId: project.id,
+ sourceId: a.id,
+ targetId: b.id,
+ interaction: "REQUEST",
+ });
+
+ const updated = await updateEdgeInteraction(testDb, actor, {
+ id: directional.id,
+ interaction: "PUSH",
+ });
+
+ expect(updated.interaction).toBe("PUSH");
+ expect(updated.sourceId).toBe(a.id);
+ expect(updated.targetId).toBe(b.id);
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/architecture/__tests__/edge.service.test.ts` around lines 569 -
583, The test currently calls updateEdgeInteraction on the same seeded
ASSOCIATION (seedAssociation) which converts and removes the association row, so
it never exercises an ASSOCIATION and a directional REQUEST coexisting; change
the test to keep the original association and create a new directional edge
instead of updating the existing one: after const { actor, a, b, edge } = await
seedAssociation(), call the function used to create a new edge (e.g., createEdge
or whatever helper creates an edge with interaction "REQUEST") to insert a new
A→B REQUEST row (do not call updateEdgeInteraction on the seeded edge), then
assert the newly created edge has interaction "REQUEST" and sourceId/targetId
match a.id/b.id and that the original association still exists if needed.
- On Component delete-undo / rollback, re-add boundary proxies below any still-present rail entries (offset by existing proxy count) instead of restarting at y=0, so a re-added stand-in never overlaps an existing one. - Tighten the interaction-conflict test to assert details.conflictingEdgeIds names the blocking Connection (not the upgraded edge). - Refocus the coexistence test to genuinely exercise ordered-slot independence: a B→A REQUEST does not block upgrading A↔B to A→B REQUEST. (The full-snapshot rollback CodeRabbit flagged was already fixed in 40e070a — both paths now restore only the field each mutation wrote.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Run `pnpm format:write` across the repo so every file matches the Prettier config. These files predate the current config and were committed not prettier-clean, which made `pnpm format:write` churn them into every feature branch. Landing the normalization once stops that recurring noise; this commit is formatting-only (no behavioral change — `pnpm check` green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #65.
The client-render slice on top of #63's
getCanvascross-scope derivation (ADR-0031). The Canvas previously ignoredboundaryProxiesand the*Reprfields; this PR consumes them.What changed
Arrowheads (ADR-0027). New pure helper
~/lib/connection-direction.ts—arrowEnds(interaction) → { atSource, atTarget }— the single source of truth, framework-agnostic (imports only the client-safeInteractiontype; no@xyflow/reactin~/lib).toRFEdgemaps it to React FlowmarkerStart/markerEnd. The exporter (#67) will derive its→/↔glyph from the same booleans.Far-end proxies (ADR-0031, ADR-0016).
boundaryProxiesrender as passiveboundary-proxynodes, re-introducing theisPassiveNode/CanvasRFNodeextension point in its simpler per-edge form (no boundary-group). Lineal/ingress proxies (real endpoint on the breadcrumb trail) are labelled "Inbound from {ancestor}" so they don't read as the host inside itself. A "Go to {title}" affordance navigates to the off-scope Component's own scope (/n/[realEndpointId]) — nogetCanvasshape change.Interaction picker. An inline segmented control on the selected Connection upgrades its interaction. New
updateEdgeInteractionmutation/service re-checks the directional de-dupe key (mirrorsconnectNodes, excludes the edge's own id) and returnsConflictErroron collision; draw order is preserved so the arrow points the way it was drawn.updateEdgestays label-only.Repr-swap correctness. Incident-edge removal now matches on reprs (not raw endpoint ids), and orphaned proxies are cleaned up on Component delete/undo — otherwise the repr swap would strand cross-scope edges/proxies in the store (invisible to
pnpm check).Scope boundary
getCanvasshape — consumed unchanged.Docs (travel with the slice)
CONTEXT.md tense flips (realized in #65); ADR-0027 amended with interaction-edit semantics; ADR-0031/0016 realized notes; stale ADR-0023 / boundary-group comments fixed.
Verification
pnpm checkgreen.connection-direction(5 cases) +updateEdgeInteractionservice tests pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation