Add endpoint-driven connectors with snap-on-draw to slides#241
Merged
Conversation
The bbox-based line/arrow shapes can't represent attachment to other elements, so adding connection points means redesigning the geometry. Switch lines to an endpoint-driven Connector element type with a per-shape connection-site registry, auto routing, and snap-on-draw UX, matching Google Slides' four connector tools. Land in three PRs; this commit only captures the design and the PR1 task plan.
Removing 'line' / 'arrow' from ShapeKind immediately breaks several existing production sites (shape-renderer / shape-icon / insert) and test files that reference them via string literals. The cleanup belongs together with the call-site removals, so move it from Task 1 into Task 10 alongside the rest of the line/arrow shape teardown. Task 1 is now purely additive — new Connector type, union extension, and exhaustive-switch stubs.
Introduces the connector-insert mode for Slides — `'connector:line'` and `'connector:arrow'` insert-mode keys, the pure-function snap helpers (`findSnapTarget`, `snappedEndpoint`, `finalizeInsert`), and the editor wiring that drags a connector ghost on the canvas until mouseup. The drag flow mirrors the existing shape-insert pattern: capture mousedown, render a translucent live ghost via `forceRender`, commit on mouseup, and revert to select mode. ESC cancels with capture-phase pre-emption so the keyboard-rule's own Escape handler does not double-fire. Sub-threshold drags are dropped by `finalizeInsert` to avoid stray zero-length connectors. Endpoints snap to the nearest connection site within 12 slide-logical units; otherwise they land as `free` endpoints at the cursor. The new `editor.isConnectorMode()` getter is exposed so Task 13's connection-points overlay can subscribe through `onInsertModeChange`. `buildInsertElement` is now typed `ShapeOrTextInsertKind` (no connector branches reach it), and the new `ConnectorInsertKind` is re-exported from `@wafflebase/slides` for toolbar wiring in Task 14.
`finalizeInsert` already short-circuits on drag distance below `MIN_DRAG_DISTANCE` and returns null without calling `store.addElement`. But the editor's `onUp` handler wrapped the call in `store.batch(...)`, and `MemSlidesStore.batch` unconditionally pushes an undo snapshot when entered at depth 0. A stray click in connector-arm mode therefore recorded a no-op undo entry, so the user's next Cmd+Z did nothing visible. Move the `store.batch(...)` wrap inside `finalizeInsert`, around the `store.addElement` call only. Now the threshold check and the transaction boundary live together: sub-threshold drags skip both the mutation and the batch, leaving the undo stack untouched. The editor `onUp` becomes a straight call to `finalizeConnectorInsert`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
`SHAPE_HOVER_RADIUS` (24) and `SITE_SNAP_RADIUS` (12) are documented as screen-pixel constants (DPR-corrected), but `findSnapTarget` compared `dx² + dy²` (slide-logical units squared) directly against `SITE_SNAP_RADIUS²`, treating 12 as slide-logical. The overlay's `renderConnectionPointsOverlay` already divides by zoom — so at zoom=2 the highlight ring covered 6 logical units while the snap window remained 12 logical units. The dot stopped highlighting but the connector still snapped (and vice versa at zoom < 1). Thread the editor's `this.scale()` through `findSnapTarget`, `snappedEndpoint`, `buildConnectorInit`, `finalizeInsert`, and `dragEndpoint` so the snap rule divides by zoom the same way the highlight rule does. Update JSDoc on both constants to make the screen-pixel intent explicit, and add regression tests at zoom=2 and zoom=0.5 that lock in the new semantics from both call paths.
The Lines category in the slides shape picker now arms the new connector insert pipeline (connector:line / connector:arrow) instead of the removed line/arrow ShapeKinds. The picker icon for connectors is drawn inline (a diagonal stroke, plus a small head for arrows) since they live outside PATH_BUILDERS. Cleans up the harness visual scenarios that still listed line/arrow in SHAPE_CATALOG / LINE_KINDS — these no longer typecheck against the slides .d.ts. Connector-rendering visual coverage is deferred to a follow-up PR.
PR1 shipped connectors against MemSlidesStore but pnpm dev uses the Yorkie-backed store, which silently dropped every connector-specific field on read (start, end, routing, arrowheads, stroke, elbowBend) and omitted the two new store methods entirely — so the overlay crashed with "Cannot read properties of undefined (reading 'kind')" as soon as a user selected a connector. This mirrors the MemSlidesStore connector implementation against the Yorkie proxy model: read() now branches on `type === 'connector'` and unwraps each field via `yorkieToPlain`; rebuildSlide/duplicateSlide/ reorderElement unwrap the whole element so no fields are lost; the two new SlidesStore methods `updateConnectorEndpoint` and `updateConnectorArrowheads` are added with the cached-frame recompute on endpoint changes; removeElement/removeElements cascade-sweep attached endpoints to free at their last world position (Q4 c1 policy); updateElementFrame refreshes dependent connector frames so selection bbox / hit-testing tracks the moved source. Also exports `ConnectorElement`, `Endpoint`, `ArrowheadStyle`, `ConnectorRouting`, `ArrowheadKind`, `computeConnectorFrame`, and `resolveEndpoint` from `@wafflebase/slides` so the frontend can reuse the frame helpers instead of duplicating them. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Line insertion is endpoint-anchored (snap-to-shape) — fundamentally different UX from shape drag-to-size — so burying the two connector entries inside the Shapes picker confused the affordance during manual smoke. Match the Google Slides toolbar where Line is a separate tool. - Shape picker: drop the "Lines" category, tighten `CategoryEntry.kind` back to `ShapeKind`, remove the connector branch + `drawConnectorIcon` from `shape-picker.tsx`. Catalogue shrinks 117 → 115. - New `<LinePicker />` dropdown: two entries (Line, Arrow) rendered as labelled rows with inline canvas previews; trigger uses the Tabler `IconLine` glyph and mirrors the Shape button's pressed visual. - Toolbar wires both pickers side-by-side; `isLinePickerKind` splits the editor's `InsertKind` between the two `activeKind` props. - Tests: shape-picker invariants updated (8 cats / 115 entries / no lines); new `line-picker.test.ts` asserts the two-entry catalogue and the `isLinePickerKind` guard. `.tsx` is still stubbed at test load so the testable surface lives in `line-picker-helpers.ts`.
Self code review found: 1. duplicateSlide silently corrupted attached connector references — regenerated element ids weren't rewritten on connector endpoints. 2. MIN_DRAG_DISTANCE was in slide-logical units while sibling radii were screen-pixel — inconsistent click-vs-drag at non-1 zoom. 3. addElement trusted caller frame, so future paste/import paths could persist a degenerate bbox on connectors. 4. detachConnectorsTargeting rebuilt the lookup Map per mutated connector when the outer Map was already valid (free endpoints don't consult it).
Record the four landed fixes (connector hygiene) and the three deferred polish items (hover-radius semantics, editor.ts size, read() migration cost) flagged during the senior self code review.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (54)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR implements PR1 of the phased connector architecture: adds endpoint-driven connector elements with free/attached endpoints, connection-site anchoring, straight routing, triangle arrowheads, store-layer CRUD with cascade detachment semantics, drag-to-place insertion with snapping and deadband, endpoint dragging with live snap feedback, overlay affordances, and toolbar UI. Removes legacy line/arrow shape kinds. ChangesConnector Architecture & Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Verification: verify:selfResult: ✅ PASS in 184.5s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/slides/src/store/memory.ts (1)
258-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject direct
updateElementFramepatches for connectors.
frameis derived cache for connectors, but this path still lets callers persist arbitrary frame patches onto a connector. That can leave selection/hit-testing out of sync with the actual endpoints until some later connector-specific mutation recomputes the cache. Either throw for connector targets here, or translate endpoints and then recompute the frame instead of merging the patch.Suggested fix
updateElementFrame( slideId: string, elementId: string, frame: Partial<Frame>, ): void { this.requireBatch(); const slide = this.requireSlide(slideId); const e = slide.elements[this.requireElementIndex(slide, elementId)]; + if (e.type === 'connector') { + throw new Error( + `Element ${elementId} is a connector; update its endpoints instead of its frame`, + ); + } e.frame = { ...e.frame, ...frame }; const lookup = this.elementsLookup(slideId); for (const el of slide.elements) {🤖 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 `@packages/slides/src/store/memory.ts` around lines 258 - 280, The updateElementFrame method currently allows callers to patch the frame of any element, which lets connectors receive direct frame writes; change updateElementFrame so it rejects (throw) when the target element is a connector (detect via slide.elements[this.requireElementIndex(slide, elementId)].type === 'connector') OR, alternatively, when the element is a connector translate the supplied frame patch into endpoint updates and then recompute the connector cache instead of merging: i.e., do not assign e.frame = {...} for connectors; call this.elementsLookup(slideId) and computeConnectorFrame(connector, lookup) to refresh the cached frame after applying endpoint changes (use requireSlide, requireElementIndex to locate the element), and ensure callers get an error if they attempt to directly mutate connector.frame.packages/frontend/src/app/slides/yorkie-slides-store.ts (1)
752-768:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock direct frame updates for connectors here too.
This has the same invariant hole as the memory store:
updateElementFramecan still write arbitrary cached bbox values onto a connector even though connector geometry is endpoint-driven. Once that happens, Yorkie can persist stale hit-test/selection bounds until another connector-specific update repairs them.Suggested fix
updateElementFrame( slideId: string, elementId: string, frame: Partial<Frame>, ): void { this.requireBatch(); this.doc.update((r) => { const s = r.slides.find((s) => s.id === slideId); if (!s) throw new Error(`Slide not found: ${slideId}`); const e = s.elements.find((e) => e.id === elementId); if (!e) throw new Error(`Element not found: ${elementId}`); + if (e.type === 'connector') { + throw new Error( + `Element ${elementId} is a connector; update its endpoints instead of its frame`, + ); + } e.frame = { ...e.frame, ...frame }; this.recomputeDependentConnectorFrames(s, elementId); }); }🤖 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 `@packages/frontend/src/app/slides/yorkie-slides-store.ts` around lines 752 - 768, updateElementFrame must not write cached bbox values onto connector elements; add a guard in updateElementFrame that detects connector elements (e.g., check e.type === 'connector' or use the project’s isConnector predicate) and skip merging/assigning into e.frame for those elements (do not perform e.frame = { ...e.frame, ...frame } for connectors); keep the existing recomputeDependentConnectorFrames call for non-connector moves, and for connectors either return early or delegate to the connector-specific recompute logic instead so connector geometry stays endpoint-driven.
🧹 Nitpick comments (3)
packages/frontend/src/app/slides/line-picker.tsx (1)
52-57: ⚡ Quick winConsider using
fill()for the arrowhead instead ofstroke().The comment at line 18 describes "a filled arrowhead," but line 57 uses
ctx.stroke()instead ofctx.fill(). This renders the arrowhead as an outline rather than a solid filled triangle.🎨 Proposed fix to fill the arrowhead
ctx.lineTo(baseX + px * headHalf, baseY + py * headHalf); ctx.lineTo(baseX - px * headHalf, baseY - py * headHalf); ctx.closePath(); - ctx.stroke(); + ctx.fill();🤖 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 `@packages/frontend/src/app/slides/line-picker.tsx` around lines 52 - 57, The arrowhead path is being stroked (ctx.stroke()) which produces an outline, but the comment and intent call for a filled arrowhead: change the final call in the arrowhead drawing block (the sequence starting with ctx.beginPath(); ctx.moveTo(...); ctx.lineTo(...); ctx.lineTo(...); ctx.closePath();) to use ctx.fill() (or ctx.fill() followed by ctx.stroke() if you want an outline) and ensure the desired color is set via ctx.fillStyle before calling ctx.fill().packages/slides/src/view/editor/overlay.test.ts (1)
217-247: ⚡ Quick winType these connector fixtures directly instead of double-casting them.
as unknown as Elementdisables compile-time drift detection on the new connector shape, so these tests can keep compiling after a connector contract change with stale fixtures. ReturningConnectorElementfrom the helpers keeps the suite honest.As per coding guidelines, "Use type annotations in TypeScript to improve code clarity and catch errors early".Suggested cleanup
-import type { Element, ShapeElement } from '../../model/element'; +import type { ConnectorElement } from '../../model/connector'; +import type { Element, ShapeElement } from '../../model/element'; - function freeConnector(): Element { + function freeConnector(): ConnectorElement { return { id: 'c1', type: 'connector', @@ - frame: { x: 100, y: 100, w: 200, h: 0, rotation: 0 }, - } as unknown as Element; + frame: { x: 100, y: 100, w: 200, h: 0, rotation: 0 }, + }; } - function attachedToRect( - site: number, - ): { connector: Element; host: Element } { + function attachedToRect( + site: number, + ): { connector: ConnectorElement; host: Element } { @@ - const connector: Element = { + const connector: ConnectorElement = { @@ - frame: { x: 0, y: 0, w: 0, h: 0, rotation: 0 }, - } as unknown as Element; + frame: { x: 0, y: 0, w: 0, h: 0, rotation: 0 }, + }; return { connector, host }; } - function connectorAt(): Element { + function connectorAt(): ConnectorElement { return { @@ - frame: { x: 1000, y: 1000, w: 100, h: 100, rotation: 0 }, - } as unknown as Element; + frame: { x: 1000, y: 1000, w: 100, h: 100, rotation: 0 }, + }; }Also applies to: 359-368
🤖 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 `@packages/slides/src/view/editor/overlay.test.ts` around lines 217 - 247, The fixtures freeConnector() and attachedToRect() are being double-cast with "as unknown as Element", which hides type mismatches; change their return types and local connector variables to the concrete ConnectorElement type (and the host to the appropriate ShapeElement/Element subtype) instead of using "as unknown as Element", update the function signatures (e.g. make freeConnector(): ConnectorElement and attachedToRect(site: number): { connector: ConnectorElement; host: ShapeElement } or the correct host type) and remove the unsafe casts so the compiler will enforce the connector contract; adjust any property shapes to satisfy ConnectorElement/ShapeElement definitions.packages/frontend/src/types/slides-document.ts (1)
114-118: ⚡ Quick winConsider removing
placeholderReffrom YorkieConnectorElement.The comment on lines 111-112 explicitly states that connectors "do not appear as layout placeholders." If connectors can never be placeholders, the
placeholderRef?: PlaceholderReffield on line 118 appears unnecessary and could cause confusion.If this field is included for structural consistency with other element types or for future-proofing, consider adding a clarifying comment.
🤖 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 `@packages/frontend/src/types/slides-document.ts` around lines 114 - 118, The YorkieConnectorElement interface currently includes an optional placeholderRef which contradicts the comment that connectors "do not appear as layout placeholders"; remove the placeholderRef?: PlaceholderRef property from the YorkieConnectorElement definition (or, if you intend to keep it for structural reasons, add a clarifying comment above YorkieConnectorElement explaining why it remains) so the interface and its intent are consistent; locate the YorkieConnectorElement declaration and either delete placeholderRef or add the explanatory comment to avoid confusion.
🤖 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 `@docs/design/slides/slides-connectors.md`:
- Around line 127-131: The fenced code blocks showing the
index.ts/getConnectionSites, defaults.ts/fourCardinal, and
overrides.ts/CONNECTION_SITES examples lack language identifiers and cause
MD040; update each triple-backtick fence around those snippets to include an
appropriate language tag (e.g., "ts" for TypeScript examples or "text" where not
code) so the blocks are lint-clean; apply the same fix to the other unlabeled
fences referenced in the review (the additional ranges noted) to eliminate the
MD040 warnings.
- Around line 51-53: The doc currently states Yorkie support is deferred; update
the "Yorkie schema migration" paragraph and any other occurrences (e.g., the
sections referencing deferred support around lines 344-346) to reflect that the
Yorkie connector implementation (yorkie-slides-store.ts) is present in the
current PR stack and not postponed—remove the deferment language, state that
Yorkie-backed store is implemented and shipping with the Phase 4 collaboration
workstream, and ensure the architecture/status wording across the file is
consistent.
In `@docs/tasks/active/20260515-slides-connectors-pr1-todo.md`:
- Around line 1651-1656: The task text is out of date: it instructs adding
Line/Arrow into the ShapePicker dropdown and references ShapeKind, but the app
uses a dedicated LinePicker component next to ShapePicker; update the task
wording to match current UI architecture by replacing references to adding
entries into ShapePicker/ShapeKind with instructions to add two options to the
existing LinePicker (or confirm they exist), and update the click behavior to
call editor.setInsertMode('connector:line') and
editor.setInsertMode('connector:arrow') as described (reference LinePicker,
ShapePicker, ShapeKind, and editor.setInsertMode in the updated text so
reviewers can locate code).
In `@packages/slides/src/view/editor/interactions/insert-connector.ts`:
- Around line 65-74: Validate zoom before dividing in findSnapTarget (and the
other block using MIN_DRAG_DISTANCE around symbols at lines ~167-173): ensure
Number.isFinite(zoom) && zoom > 0; if not, throw a clear RangeError or return
null with a descriptive message that includes the invalid zoom value and
function name (e.g. "findSnapTarget: invalid zoom = <value>"), so the code never
computes SITE_SNAP_RADIUS/zoom or MIN_DRAG_DISTANCE/zoom when zoom is non-finite
or <= 0.
In `@packages/slides/src/view/editor/overlay.ts`:
- Around line 410-428: The current hover selection uses centre distance (cx/cy)
to pick nearest elements; replace that with point-to-element distance: for each
non-connector element in elements compute either the minimum distance from
cursor to the element's connection sites (if the element exposes a
sites/connections array) or the distance to the closest point on el.frame
(clamping cursor to rect edges) to get dx/dy and d2, then compare against bestD2
(derived from hoverR2) and set nearest accordingly (update the loop that sets
nearest and bestD2). Keep the zoom/hoverR2 logic and skip connectors as before,
and add a regression test asserting the hover chooses the element when cursor is
exactly on a connection site (covering the blue-dot/endpoint case).
---
Outside diff comments:
In `@packages/frontend/src/app/slides/yorkie-slides-store.ts`:
- Around line 752-768: updateElementFrame must not write cached bbox values onto
connector elements; add a guard in updateElementFrame that detects connector
elements (e.g., check e.type === 'connector' or use the project’s isConnector
predicate) and skip merging/assigning into e.frame for those elements (do not
perform e.frame = { ...e.frame, ...frame } for connectors); keep the existing
recomputeDependentConnectorFrames call for non-connector moves, and for
connectors either return early or delegate to the connector-specific recompute
logic instead so connector geometry stays endpoint-driven.
In `@packages/slides/src/store/memory.ts`:
- Around line 258-280: The updateElementFrame method currently allows callers to
patch the frame of any element, which lets connectors receive direct frame
writes; change updateElementFrame so it rejects (throw) when the target element
is a connector (detect via slide.elements[this.requireElementIndex(slide,
elementId)].type === 'connector') OR, alternatively, when the element is a
connector translate the supplied frame patch into endpoint updates and then
recompute the connector cache instead of merging: i.e., do not assign e.frame =
{...} for connectors; call this.elementsLookup(slideId) and
computeConnectorFrame(connector, lookup) to refresh the cached frame after
applying endpoint changes (use requireSlide, requireElementIndex to locate the
element), and ensure callers get an error if they attempt to directly mutate
connector.frame.
---
Nitpick comments:
In `@packages/frontend/src/app/slides/line-picker.tsx`:
- Around line 52-57: The arrowhead path is being stroked (ctx.stroke()) which
produces an outline, but the comment and intent call for a filled arrowhead:
change the final call in the arrowhead drawing block (the sequence starting with
ctx.beginPath(); ctx.moveTo(...); ctx.lineTo(...); ctx.lineTo(...);
ctx.closePath();) to use ctx.fill() (or ctx.fill() followed by ctx.stroke() if
you want an outline) and ensure the desired color is set via ctx.fillStyle
before calling ctx.fill().
In `@packages/frontend/src/types/slides-document.ts`:
- Around line 114-118: The YorkieConnectorElement interface currently includes
an optional placeholderRef which contradicts the comment that connectors "do not
appear as layout placeholders"; remove the placeholderRef?: PlaceholderRef
property from the YorkieConnectorElement definition (or, if you intend to keep
it for structural reasons, add a clarifying comment above YorkieConnectorElement
explaining why it remains) so the interface and its intent are consistent;
locate the YorkieConnectorElement declaration and either delete placeholderRef
or add the explanatory comment to avoid confusion.
In `@packages/slides/src/view/editor/overlay.test.ts`:
- Around line 217-247: The fixtures freeConnector() and attachedToRect() are
being double-cast with "as unknown as Element", which hides type mismatches;
change their return types and local connector variables to the concrete
ConnectorElement type (and the host to the appropriate ShapeElement/Element
subtype) instead of using "as unknown as Element", update the function
signatures (e.g. make freeConnector(): ConnectorElement and attachedToRect(site:
number): { connector: ConnectorElement; host: ShapeElement } or the correct host
type) and remove the unsafe casts so the compiler will enforce the connector
contract; adjust any property shapes to satisfy ConnectorElement/ShapeElement
definitions.
🪄 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: 202aa373-cd2f-4bd5-8537-339947fb5ebc
📒 Files selected for processing (50)
docs/design/README.mddocs/design/slides/slides-connectors.mddocs/tasks/active/20260515-slides-connectors-pr1-lessons.mddocs/tasks/active/20260515-slides-connectors-pr1-todo.mdpackages/frontend/src/app/harness/visual/slides-scenarios.tsxpackages/frontend/src/app/slides/line-picker-helpers.tspackages/frontend/src/app/slides/line-picker.tsxpackages/frontend/src/app/slides/shape-picker-helpers.tspackages/frontend/src/app/slides/shape-picker.tsxpackages/frontend/src/app/slides/slides-formatting-toolbar.tsxpackages/frontend/src/app/slides/yorkie-slides-store.tspackages/frontend/src/types/slides-document.tspackages/frontend/tests/app/slides/line-picker.test.tspackages/frontend/tests/app/slides/shape-picker.test.tspackages/slides/src/index.tspackages/slides/src/model/connection-site.tspackages/slides/src/model/connector.tspackages/slides/src/model/element.tspackages/slides/src/store/memory.test.tspackages/slides/src/store/memory.tspackages/slides/src/store/store.tspackages/slides/src/view/canvas/arrowhead-renderer.test.tspackages/slides/src/view/canvas/arrowhead-renderer.tspackages/slides/src/view/canvas/connection-sites/connection-sites.test.tspackages/slides/src/view/canvas/connection-sites/defaults.tspackages/slides/src/view/canvas/connection-sites/index.tspackages/slides/src/view/canvas/connector-frame.test.tspackages/slides/src/view/canvas/connector-frame.tspackages/slides/src/view/canvas/connector-renderer.test.tspackages/slides/src/view/canvas/connector-renderer.tspackages/slides/src/view/canvas/element-renderer.tspackages/slides/src/view/canvas/routing.test.tspackages/slides/src/view/canvas/routing.tspackages/slides/src/view/canvas/shape-icon.test.tspackages/slides/src/view/canvas/shape-icon.tspackages/slides/src/view/canvas/shape-renderer.test.tspackages/slides/src/view/canvas/shape-renderer.tspackages/slides/src/view/canvas/shape-special.tspackages/slides/src/view/canvas/slide-renderer.tspackages/slides/src/view/editor/editor.test.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/hit-test.tspackages/slides/src/view/editor/interactions/connector-endpoint-drag.test.tspackages/slides/src/view/editor/interactions/connector-endpoint-drag.tspackages/slides/src/view/editor/interactions/insert-connector.test.tspackages/slides/src/view/editor/interactions/insert-connector.tspackages/slides/src/view/editor/interactions/insert.test.tspackages/slides/src/view/editor/interactions/insert.tspackages/slides/src/view/editor/overlay.test.tspackages/slides/src/view/editor/overlay.ts
💤 Files with no reviewable changes (4)
- packages/slides/src/view/canvas/shape-icon.test.ts
- packages/slides/src/view/editor/interactions/insert.test.ts
- packages/slides/src/view/canvas/shape-special.ts
- packages/slides/src/view/canvas/shape-renderer.test.ts
CodeRabbit + senior self-review surfaced two real invariant holes plus minor polish: - updateElementFrame on a connector left its derived bbox cache out of sync with the endpoints. Reject the call instead so callers get a clear error and use updateConnectorEndpoint. - Hover affordance was gated on element-center distance, so the connection-points overlay disappeared the moment the cursor sat ON a connection site of a larger shape (the very position where snapping was about to commit). Switch to nearest-site distance so the overlay agrees with the snap. Plus guards for zoom <= 0, doc drift fixes for the design and plan files, an arrowhead fill/stroke nit on the picker icon, type narrowing in test fixtures, and trimming a never-used placeholderRef field from the Yorkie connector type. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This was referenced May 16, 2026
4 tasks
hackerwins
added a commit
that referenced
this pull request
May 18, 2026
These seven slides + PPTX tasks shipped (PRs #238, #240, #241, #255, #259, #260, #261) but their checklist boxes were never ticked during execution, so the archive script left them in active/. Sweep the checkboxes to reflect what actually landed, add lessons files for the two that were missing (keyboard-shortcuts, presentation-mode), and convert the keyboard-shortcuts onLinkRequest popover bullet from a checkbox to a "Deferred (follow-up)" note so the gate stops blocking on work that is intentionally out of scope. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
7 tasks
This was referenced Jun 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
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.
Summary
Replace bbox-based
kind: 'line'/'arrow'shapes in the slides package witha new endpoint-driven
Connectorelement type, matching Google Slides'Line/Arrow tools.
ConnectorElement(sibling of Shape/Text/Image): twoEndpoints(
{ kind: 'free', x, y }or{ kind: 'attached', elementId, siteIndex }),routing field (
'straight'only in PR1; elbow/curved arrive in PR2),per-endpoint arrowheads (filled
triangleonly in PR1; full set in PR3),derived
framebbox cache maintained bycomputeConnectorFrame.ConnectionSite[]registrywith default 4-cardinal (N/E/S/W mid-edge) for every kind (per-
ShapeKindoverrides arrive in PR2).
siteWorldPosresolves a normalized local sitethrough the source's
frametransform.updateConnectorEndpoint,updateConnectorArrowheads,cascade sweep on source delete (attached → free at last-rendered world
position), dependent-frame recompute on source move. Wired in both
MemSlidesStoreandYorkieSlidesStore.duplicateSliderewritesattached connector ids to point at the copy's elements.
endpoint drag (free⇄attached transition), connection-points DOM overlay,
ESC cancel, sub-threshold drag skips undo.
LinePickerdropdown right ofShapePickerso theendpoint-anchored insertion UX gets its own affordance.
Design doc:
docs/design/slides/slides-connectors.mdPlan:
docs/tasks/active/20260515-slides-connectors-pr1-todo.mdLessons:
docs/tasks/active/20260515-slides-connectors-pr1-lessons.mdPR2 (elbow + curved routing + per-shape sites) and PR3 (arrowhead variants
Test plan
pnpm verify:fastgreen on every commit (804 slides tests, 239frontend tests, 764 docs tests, all packages green)
pnpm --filter @wafflebase/frontend exec tsc --noEmitgreencascade on delete, undo, no resize/rotate handles on connectors,
LinePicker dropdown UX)
pnpm verify:browser:docker(shape catalog drops from 117→115; newLine dropdown adds a toolbar button — diffs expected)
the 4 fixes that landed post-review and the 3 deferrals (hover-radius
semantics, editor.ts size,
read()migration cost) marked asfollow-ups
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes