Fix PPTX connector endpoints to land on correct shape edges#260
Merged
Conversation
OOXML's `rect`/`roundRect` cxnLst is ordered T, L, B, R, but Waffle's `FOUR_CARDINAL` is N, E, S, W — i.e. T, R, B, L. parseCxnSp was passing the raw OOXML idx through as siteIndex, so every connector anchored to a left/right edge in PPTX landed on the opposite edge after import. Visible on the imported "Yorkie, 캐즘 뛰어넘기.pptx" deck (slide 24): all four MVC-diagram arrows hit the wrong side of their target shapes while the connector frames themselves stayed correct, which made the mismatch look like a routing bug instead of an index bug. Swap idx 1 ↔ 3 when materializing the attached endpoint, and add a unit test through parseSpTree covering all four cardinal indices. Per-shape cxnLst overrides (slides-connectors PR2) will need to replace this static swap with a per-ShapeKind map.
Scope-tighten the OOXML→Waffle helper comment to the T,L,B,R family (rect, roundRect, *Rect variants, plaque, bevel, flowChartTerminator); other prsts have their own cxnLst orderings that PR2's per-ShapeKind overrides will need to address. Today the swap is harmless on those shapes only because getConnectionSites() returns FOUR_CARDINAL uniformly — call that out so the next person doesn't assume the helper is universal. Also correct the fallback wording: out-of-range siteIndex falls back to sites[0] (N) in connector-frame.ts, not (0,0). Replace the two if-statements with a `[0, 3, 2, 1]` lookup table since PR2 will turn this into a per-shape map anyway. Add coverage for the `end` endpoint code path and the free-fallback path when the stCxn target id has no matching sp in the slide. Note in lessons that pre-fix imports have the buggy siteIndex persisted in Yorkie; only re-imported decks pick up the fix.
|
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 (6)
📝 WalkthroughWalkthroughFixes OOXML connector endpoint attachment site-index mapping (T,L,B,R → N,E,S,W) via a new remapping helper and integration into ChangesPPTX Connector Site Index and Flip/Rotation Fixes
Sequence Diagram(s)sequenceDiagram
participant ImportPPTX as PPTX Import
participant ParseCxn as parseCxnSp
participant ResolveEp as resolveEndpoint
participant IndexMap as ooxmlToWaffleSiteIndex
participant Output as AttachedEndpoint
ImportPPTX->>ParseCxn: connector with cxnLst idx
ParseCxn->>ResolveEp: endpoint (attached, idx)
ResolveEp->>IndexMap: OOXML idx (0–3)
IndexMap->>Output: Waffle siteIndex remapped
flowchart LR
A["Frame local coords"] --> B["Apply flipH/flipV"]
B --> C["Rotate into world space"]
C --> D["World position + angle"]
E["Outward angle"] --> F["Apply flipH/flipV to angle"]
F --> G["Add element rotation"]
G --> D
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 204.3s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`element-renderer.ts` paints flipped shapes by mirroring the path around the frame centre after rotation, but `siteWorldPos` was only applying rotation — `frame.flipH`/`frame.flipV` were silently dropped. OOXML `cxnLst` indices reference pre-flip logical sites, so an attached connector targeting a `flipH=1` shape resolved to the unmirrored edge while the shape painted as mirrored. Result: in the "Yorkie, 캐즘 뛰어넘기.pptx" deck, slide 24 looked correct after the prior idx-swap fix (no flipped shapes), but slide 26's right-side MVC group (all `flipH=1`) still routed every connector to the wrong side. Mirror `lx`/`ly` around the frame centre before rotation, and reflect the site angle (`flipH: π - a`, `flipV: -a`). Matches the paint ordering `translate(centre) → rotate → scale(flip) → translate(-w/2)` in `element-renderer.ts:50-64`. Knock-on benefit: user-flipped shapes in the editor now drag their attached connectors along to the mirrored edge, matching PowerPoint / Google Slides behaviour.
hackerwins
changed the title
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]>
3 tasks
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
(
<a:stCxn idx="1"/>oridx="3") were landing on the opposite edgeafter import. Visible on slide 24 of the "Yorkie, 캐즘 뛰어넘기.pptx"
deck: all four MVC-diagram arrows hit the wrong side while the
connector frames themselves stayed correct.
rect/roundRectcxnLstis orderedT, L, B, Rbut Waffle's
FOUR_CARDINALisN, E, S, W(i.e.T, R, B, L).parseCxnSpwas passing the raw OOXML idx through assiteIndexwithout remapping, so indices 1 and 3 silently swapped meaning.
ooxmlToWaffleSiteIndex(a[0, 3, 2, 1]lookup table) androute the attached-endpoint construction through it. Scope of the
swap is documented as the T,L,B,R family; per-
ShapeKindoverridesfor other presets remain deferred to slides-connectors PR2 (today
harmless because
getConnectionSites()is uniform).Test plan
pnpm verify:fast— green (3398 tests across sheets/slides/docs/backend/frontend).packages/slides/test/import/pptx/connector.test.ts:[0,1,2,3]→ siteIndex[0,3,2,1].endCxn.kind: 'free', not a remapped attached endpoint.Backward compatibility
Decks imported before this fix have the buggy
siteIndexvaluespersisted in Yorkie. They keep rendering at the wrong edge until
re-imported — the fix only changes the import path, not stored data.
PPTX import is one-shot so re-importing is the expected upgrade path.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation