Resolve grouped connector endpoints to world coords#320
Conversation
|
Warning Review limit reached
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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Verification: verify:selfResult: ✅ PASS in 246.7s
Verification: verify:integrationResult: ✅ PASS |
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]>
c4af207 to
cff3051
Compare
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.
Summary
<p:cxnSp>connector targeted an element inside a<p:grpSp>(e.g. slide 24 ofYorkie, 캐즘 뛰어넘기.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:siteWorldPosreadsel.frame.x/yas slide-world coords, but group children are stored with frames in their parent group's local space, andflattenElementsflattened the tree without lifting those frames.buildElementWorldLookup(elements)inpackages/slides/src/model/group.tswalks the tree DFS while composing ancestorgroupToTransforms and emits each non-top-level entry with its frame rewritten throughapplyMatrix. Replaces theflattenElements-based lookup at every connector-resolver callsite (slide-renderer, two editor sites, overlay, store).flattenElementsis kept for callers that only need tree topology (iterating connectors by id/type). The design-doc section indocs/design/slides/slides-connectors.mddocuments which lookup belongs where.Test plan
pnpm verify:fastgreenconnector-frame.test.tscover: 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