Arrows between two objects, and the one object with no box - #40
Merged
Merged
Conversation
Select two objects and press "Connect": an arrow joins them, pointing from the first one selected to the second. The same control says "Disconnect" once they are joined, because with one already there that is the only thing anybody wants from a button about it. A press on the line selects it, Delete removes it, and deleting either object takes its arrows with it — in the same op, so one undo puts the whole picture back rather than the cards first and the lines after. **A connector is a relation, not a rectangle.** Its geometry is worked out from wherever its ends are at the moment it is drawn, which is what makes dragging a card free: the arrows follow it without a single op being written for them, and the document holds nothing that could disagree with where the cards are. That is also the invariant every part of this app that assumes an `x` had to be taught, and the list is exactly as long as it sounds: bounds, snapping, the marquee, nudging, dragging, the frame a PNG covers and the map. `isPlaced` is how each of them asks. Missing one writes `NaN` into the document and broadcasts it to everyone else on the board, so each has a test that fails without the guard. What copies, copies whole. A connector comes along when both the things it joins do — selected or not, the rule an envelope's contents already follow — and is left behind when they do not, since an arrow with one end in the payload has nothing to point at when it lands. Duplicating, pasting and exporting a selection all ask that same question, and the picture drawn to a PNG asks it too: the export is a second renderer, and an arrow on screen that is missing from the file is exactly the drift its comment warns about. **Drawn in one SVG at the back of the world layer**, in world coordinates, with the thickness in world units — a connector scales with the board like a card's text and unlike a selection ring, because it is content rather than an affordance. The line is two and a half units thick, which nobody can point at, so an invisible sixteen-unit stroke underneath takes the press. Two things found by building it. An element of no size with `overflow: visible` — the obvious way to draw at unbounded coordinates — is one Chrome lays out, hit-tests and paints none of; the element is moved and sized to hold what it draws now, with a `viewBox` that keeps its contents world coordinates. And "is there room between these two boxes" cannot be answered by the distance between their borders: two objects that overlap have their far borders in the wrong order, and the answer has to be measured along the direction the two lie in, or an arrow is drawn backwards through both of them. 986 tests, 96.9% lines. Both new modules at 100%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds connector objects between placed board objects. Connectors support creation, selection, movement with endpoints, deletion, copying, duplication, SVG rendering, minimap filtering, and PNG export. ChangesConnector feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/connectors.js (2)
159-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting the point list so the platform layer can reuse it.
src/platform/connectors.js(lines 96-136) rebuilds the same five zero-size rects fromconnectorGeometryoutput that this function builds. Extract a smallconnectorPoints(drawn)helper here and call it from both places. This keeps one definition of what a connector covers.♻️ Proposed extraction
+/** The points a drawn connector occupies: both line ends and the head. */ +export const connectorPoints = (drawn) => [ + { x: drawn.line.x1, y: drawn.line.y1, w: 0, h: 0 }, + { x: drawn.line.x2, y: drawn.line.y2, w: 0, h: 0 }, + ...drawn.head.map(([x, y]) => ({ x, y, w: 0, h: 0 })), +]; + export function connectorBox(from, to, options) { const drawn = connectorGeometry(from, to, options); if (!drawn) return null; - - return bbox([ - { x: drawn.line.x1, y: drawn.line.y1, w: 0, h: 0 }, - { x: drawn.line.x2, y: drawn.line.y2, w: 0, h: 0 }, - ...drawn.head.map(([x, y]) => ({ x, y, w: 0, h: 0 })), - ]); + return bbox(connectorPoints(drawn)); }🤖 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/core/connectors.js` around lines 159 - 168, Extract and export a connectorPoints(drawn) helper from connectors.js that builds the line endpoints and arrowhead zero-size rectangles currently assembled inside connectorBox. Update connectorBox and the platform connector logic to reuse this helper instead of rebuilding the point list, preserving the existing connector coverage.
145-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or cover
isHanging
isHanginghas no caller or test reference. Remove the unused export, or add the connector validation path and tests for missing endpoints.🤖 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/core/connectors.js` around lines 145 - 149, Remove the unused exported isHanging helper from the connector module, unless you integrate it into the connector validation path and add tests covering connectors with missing from or to endpoints.src/core/board.js (1)
188-221: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExclude connectors explicitly from
usable.If
OBJECT_DEFAULTSlater includesconnector,bbox(usable)will read missing dimensions and produceNaNpaste coordinates. Use the existingisPlacedpredicate.Proposed guard
- const usable = objects.filter((obj) => OBJECT_DEFAULTS[obj?.type]); + const usable = objects.filter((obj) => isPlaced(obj) && OBJECT_DEFAULTS[obj?.type]);🤖 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/core/board.js` around lines 188 - 221, Update the usable-object filtering in paste so connectors are explicitly excluded by combining the OBJECT_DEFAULTS check with the existing isPlaced predicate. Keep connector handling in the separate joins flow, ensuring bbox(usable) only receives placed objects with valid dimensions.
🤖 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/platform/renderer.js`:
- Around line 113-115: Update the placed-object insertion logic in the renderer
so the connector SVG created by createConnectorLayer() remains the layer prefix
and all placed objects are inserted after it, preserving connectors behind board
objects. Add a browser assertion that verifies the connector layer is the first
child and placed elements follow it.
In `@test/node/connectors.test.js`:
- Around line 235-247: Update the “pasting points the copies at each other” test
to capture the original connector created by the two() fixture, then assert the
pasted connector’s id differs from that original connector’s id instead of
checking only that copied.id is defined. Preserve the existing assertions that
pasted endpoints reference the new placed ids and no node retains an old id.
---
Nitpick comments:
In `@src/core/board.js`:
- Around line 188-221: Update the usable-object filtering in paste so connectors
are explicitly excluded by combining the OBJECT_DEFAULTS check with the existing
isPlaced predicate. Keep connector handling in the separate joins flow, ensuring
bbox(usable) only receives placed objects with valid dimensions.
In `@src/core/connectors.js`:
- Around line 159-168: Extract and export a connectorPoints(drawn) helper from
connectors.js that builds the line endpoints and arrowhead zero-size rectangles
currently assembled inside connectorBox. Update connectorBox and the platform
connector logic to reuse this helper instead of rebuilding the point list,
preserving the existing connector coverage.
- Around line 145-149: Remove the unused exported isHanging helper from the
connector module, unless you integrate it into the connector validation path and
add tests covering connectors with missing from or to endpoints.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a3e3dbf6-c4a8-4712-bcde-eeb91f3eb9e4
📒 Files selected for processing (15)
README.mdsrc/app.jssrc/components/FormatBar.jsxsrc/core/board.jssrc/core/connectors.jssrc/core/export.jssrc/platform/connectors.jssrc/platform/export-png.jssrc/platform/input.jssrc/platform/minimap.jssrc/platform/renderer.jssrc/styles/canvas.csstest/browser/connectors.test.jstest/browser/export.test.jstest/node/connectors.test.js
- **A connector was drawn on top of the objects it joins.** Being the layer's first child is not a place a component can hold: the renderer rewrites the order of the children on every sync to match the z-order of the objects *it* draws, and it does that by inserting each one before the previous one's next sibling — so the SVG parked at the front was pushed to the end of the list and painted over everything. It is appended now and put behind by `z-index: -1`, which says the depth once instead of relying on a position another component owns. `#layer` is transformed and therefore a stacking context, so it cannot fall out of the back of the board. This was invisible on a first look because a line is cut at both borders and usually has no card under it — and the test I wrote for it passed both ways at first, because `board.add` selects what it makes and a selected object is lifted by `z-index: 1`. Deselected, the broken version really does answer the line rather than the card at that point, which is what the test now asserts and what it fails without the fix. - **An assertion that could not fail.** `copied.id !== undefined` is true of every object `paste` returns. The original connector is captured now and the copy is compared against it, which is the claim that was meant: nothing keeps an old id, because two objects sharing one is a document where an op means two things. 987 tests, 96.9% lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
This was referenced Aug 9, 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
2 participants
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.
Select two objects and press Connect: an arrow joins them, pointing from the first one selected to the second. The same control says Disconnect once they are joined, because with one already there that is the only thing anybody wants from a button about it. A press on the line selects it,
Deleteremoves it, and deleting either object takes its arrows with it — in the same op, so one undo puts the whole picture back rather than the cards first and the lines after.A connector is a relation, not a rectangle
Its geometry is worked out from wherever its ends are at the moment it is drawn. That is what makes dragging a card free: the arrows follow without a single op being written for them, and the document holds nothing that could disagree with where the cards are.
It is also the one object with no box, which is the invariant every part of this app that assumes an
xhad to be taught. The list is exactly as long as it sounds — bounds, snapping, the marquee, nudging, dragging, the frame a PNG covers, and the map — andisPlacedis how each of them asks. Missing one writesNaNinto the document and broadcasts it to everyone else on the board, so each has a test.What copies, copies whole
A connector comes along when both the things it joins do — selected or not, the rule an envelope's contents already follow — and is left behind when they do not, since an arrow with one end in the payload has nothing to point at when it lands. Duplicating, pasting and exporting a selection all ask that same question.
The PNG asks it too. The export is a second renderer and its comment says what that costs; an arrow on screen that is missing from the file is exactly that drift, so it draws them — from the same geometry, with the stroke probed off the stylesheet rather than restated.
Drawn in one SVG, behind everything
At the back of the world layer, in world coordinates, with the thickness in world units: a connector scales with the board like a card's text and unlike a selection ring, because it is content rather than an affordance. The visible line is two and a half units thick, which nobody can point at, so an invisible sixteen-unit stroke underneath takes the press.
Two things found by building it
overflow: visibleis one Chrome lays out, hit-tests and paints none of. That is the obvious shape for drawing at unbounded, negative coordinates, and it is what this started as — the lines were provably on screen, hittable, and invisible. The element is moved and sized to hold what it draws now, with aviewBoxthat keeps its contents world coordinates.Not in this change
Straight lines only: no routing round what is in the way, no elbows, no choosing which side an arrow leaves by, and no label on the line — which is the one most likely to be missed, since half of what an arrow says on a diagram is written along it. There is also no drag-to-connect gesture: handles on an object's edge would land exactly where the resize handles already are, which is what made the two-object selection the right first move. All written into the README's limits.
Checks
986 tests, all passing, 96.9% line coverage.
core/connectors.jsandplatform/connectors.jsare both at 100%. 29 node tests for the geometry and the board rules, 11 browser tests over the real gestures — connect, disconnect, follow, overlap, select the line, the delete cascade and its undo, drag and nudge with one in the selection, the marquee, duplicate — and 3 export tests on the painted pixels.🤖 Generated with Claude Code
https://claude.ai/code/session_01PTizw7U35rC2MPquwr4Fv5
Summary by CodeRabbit