Skip to content

Resolve grouped connector endpoints to world coords#320

Merged
hackerwins merged 1 commit into
mainfrom
slides-grouped-connector-endpoint-world-coords
May 31, 2026
Merged

Resolve grouped connector endpoints to world coords#320
hackerwins merged 1 commit into
mainfrom
slides-grouped-connector-endpoint-world-coords

Conversation

@hackerwins

Copy link
Copy Markdown
Collaborator

Summary

  • Imported PPTX decks where a <p:cxnSp> connector targeted an element inside a <p:grpSp> (e.g. slide 24 of Yorkie, 캐즘 뛰어넘기.pptx — arrows pointing at a user avatar nested in a group with its background ellipse) rendered arrows near the slide origin instead of at the target. Cause: siteWorldPos reads el.frame.x/y as slide-world coords, but group children are stored with frames in their parent group's local space, and flattenElements flattened the tree without lifting those frames.
  • New buildElementWorldLookup(elements) in packages/slides/src/model/group.ts walks the tree DFS while composing ancestor groupToTransforms and emits each non-top-level entry with its frame rewritten through applyMatrix. Replaces the flattenElements-based lookup at every connector-resolver callsite (slide-renderer, two editor sites, overlay, store).
  • flattenElements is kept for callers that only need tree topology (iterating connectors by id/type). The design-doc section in docs/design/slides/slides-connectors.md documents which lookup belongs where.

Test plan

  • pnpm verify:fast green
  • New unit tests in connector-frame.test.ts cover: shape nested in a group, group children with world frames, nested groups composing transforms, group scaling propagating to children, nested group elements themselves lifted to world frames
  • Manual smoke on the affected deck (user-verified) — slide 24 arrows now land on the avatar

@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@hackerwins, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 23 minutes and 36 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 98ab830b-33c7-4506-96f0-85cc7fb6fda0

📥 Commits

Reviewing files that changed from the base of the PR and between 91bf275 and cff3051.

📒 Files selected for processing (13)
  • docs/design/slides/slides-connectors.md
  • docs/tasks/README.md
  • docs/tasks/archive/2026/05/20260531-slides-group-nested-connector-lessons.md
  • docs/tasks/archive/2026/05/20260531-slides-group-nested-connector-todo.md
  • docs/tasks/archive/README.md
  • packages/slides/src/index.ts
  • packages/slides/src/model/group.ts
  • packages/slides/src/node.ts
  • packages/slides/src/store/memory.ts
  • packages/slides/src/view/canvas/slide-renderer.ts
  • packages/slides/src/view/editor/editor.ts
  • packages/slides/src/view/editor/overlay.ts
  • packages/slides/test/view/canvas/connector-frame.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch slides-grouped-connector-endpoint-world-coords

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 31, 2026

Copy link
Copy Markdown
Contributor

Verification: verify:self

Result: ✅ PASS in 246.7s

Lane Status Duration
tokens:build ✅ pass 2.2s
sheets:build ✅ pass 13.4s
docs:build ✅ pass 12.1s
slides:build ✅ pass 14.0s
verify:fast ✅ pass 162.4s
frontend:build ✅ pass 18.7s
verify:frontend:chunks ✅ pass 0.3s
backend:build ✅ pass 4.8s
cli:build ✅ pass 2.1s
verify:entropy ✅ pass 16.7s

Verification: verify:integration

Result: ✅ PASS

@codecov

codecov Bot commented May 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

`siteWorldPos` reads `el.frame.x/y` as slide-world coordinates, but
group children are stored with frames in their parent group's local
space. `flattenElements` flattened the tree without lifting those
frames, so a connector whose `attached` endpoint targeted an element
inside a `<p:grpSp>` rendered near the slide origin instead of at
the actual on-screen position. Imported PPTX decks with diagrams
where arrows attach to grouped shapes (e.g. avatars wrapped in a
group with their background ellipse) showed arrows pointing far
away from their targets.

`buildElementWorldLookup` walks the tree DFS while composing
ancestor `groupToTransform`s and emits each non-top-level entry
with its frame rewritten through `applyMatrix`. Replaces the
`flattenElements`-based lookup at every connector-resolver
callsite (slide-renderer, two editor sites, overlay, store).

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@hackerwins
hackerwins force-pushed the slides-grouped-connector-endpoint-world-coords branch from c4af207 to cff3051 Compare May 31, 2026 07:17
@hackerwins
hackerwins merged commit 41c6341 into main May 31, 2026
1 check passed
@hackerwins
hackerwins deleted the slides-grouped-connector-endpoint-world-coords branch May 31, 2026 07:18
hackerwins added a commit that referenced this pull request Jun 11, 2026
Three slides-editor bugs surfaced while testing groups; they travel
together because the same `/shared/<id>` deck exercises them in
sequence.

1. Lines drift after grouping their endpoints (the reported bug).
   `MemSlidesStore.group()` keeps a connector as a group child when
   both endpoints reference candidates, but `drawElement`'s connector
   branch skipped the per-element frame transform — `drawConnector`
   expects world-coord endpoints and `buildElementWorldLookup` lifts
   grouped targets to world, so the group transform compounded on top
   of already-world endpoints and the line jumped by the group's
   offset.

   `drawElement` now threads a cumulative `parentTransform`; on a
   connector child it inverts the parent transform onto the ctx and
   hands `drawConnector` the lookup's view of the connector — free
   endpoints are world-coord via `walkWorld`, attached endpoints
   already resolve through the lookup, so both kinds agree. The
   "v1 group() never includes connectors" NOTE in
   `element-renderer.ts` was stale since #263; #320 then bolted the
   world-coord lookup on top, which is exactly what broke this.

2. Rotate angle tooltip drifts by the pasteboard offset.
   `acquireRotateTooltip` appends to `overlay.parentElement` so
   `renderOverlay`'s innerHTML reset can't wipe it mid-drag, but
   `showTooltip` was computing coords against `overlay`'s rect.
   These containers shared an origin until #353 gave the overlay a
   non-zero `slideOffsetCssX/Y` inside `canvasWrap`. Measure against
   the tooltip's actual parent rect instead.

3. Rotate tooltip flickers at the previous drag's last position on
   re-acquire. `acquireRotateTooltip` flipped `display: block`
   immediately while `transform` still held the previous drag's
   terminal coords. Acquire now returns the cached element hidden;
   `startRotate` calls `showTooltip(clientX, clientY, 0)` once
   immediately so `transform` and `display: block` land in the same
   paint frame. Bonus: a 0° readout pops at the click position with
   no input latency.

Pre-merge code review surfaced one blocking concern and seven
non-blocking follow-ups (tracked in the task doc).

Fixed before merge: `invertGroupTransform` now returns `null`
instead of throwing on a singular matrix, and the connector branch
skips the paint. A degenerate group (frame.w = 0 with refSize > 0,
reachable via PPTX import) used to throw, escape `drawElement`'s
try/finally, and blank every later element on the slide.

Deferred follow-ups: flip blindness in `groupToTransform`;
reference-equality identity gate; ghost connector inside a group
ghost; `tooltipContainer` cached once at `startRotate`;
`overlay.parentElement` null fallback inside `acquireRotateTooltip`;
three copies of the 6-coef affine inverse math
(`element-renderer.ts` + `model/group.ts`); acquire-then-show
invariant naming.

Regression coverage:
- `slide-renderer.test.ts` — attached connector: `ctx.transform`
  runs before `moveTo` with the expected inverse coefficients; free
  connector: pre/post-group `moveTo`/`lineTo` args match (the lookup
  swap); singular parent transform: skips that connector but keeps
  painting the rest of the slide.
- `editor.test.ts` — pasteboard layout: tooltip transform encodes
  parent-frame coords, not overlay-relative; two-cycle drag: second
  pointerdown's transform reflects the new click position, not the
  previous drag's last move.
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