Import + render PPTX <a:bodyPr anchor> with menu control#307
Merged
Conversation
Add a todo plan for parsing PPTX <a:bodyPr anchor> into a slides TextElement.verticalAnchor field and rendering the offset so imported title placeholders sit at the bottom of their oversized frames as the source decks intend. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Mirrors OOXML <a:bodyPr anchor>. Sparse field — absent preserves existing top-anchored behavior, so old documents and tests remain valid without migration. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Mirrors the AutofitMode pattern so the PPTX importer and canvas renderer can reference a single type symbol instead of duplicating the inline union. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Source decks anchor title placeholders at the bottom of an oversized frame (anchor="b"). Without parsing this, imported titles render ~2" above where the source shows them. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Treat empty-string anchor="" as absent rather than silently overriding inheritance with 'top'. Tighten the null guard to match attr()'s actual return type. Cover 'dist' alongside 'just' in the unsupported-fallback test, and drop a redundant import alias in the test file. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
drawText now measures laid-out content height and translates the paint origin so text sits at the top/middle/bottom of the frame. Imported PPTX titles with anchor="b" finally render at the bottom of their oversized placeholder boxes instead of floating ~2" above. Clamps to >= 0 so overflow content stays visible at the top edge rather than getting clipped above the frame. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Notes the shrink-path padding assumption that keeps originY non- negative, and the deliberate decision to paint the empty-placeholder hint at the top regardless of verticalAnchor. Tightens the overflow test so the assertion catches a spurious offset rather than just checking the line baseline is inside the frame. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Adds verticalAnchor to the model-support matrix and records the in-place editor parity gap that Chunk 3 will close. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Mirrors the slides canvas-renderer offset so paintLayout, the caret position, the selection rectangles, and the click hit-test all align with the configured anchor. Without this, editing a bottom-anchored title snaps the text to the top of the frame. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Closes a narrow click-race window: TextEditor wires its pointer handler at construction, but currentOriginY was only set inside renderNow's first rAF. A click before that frame would resolve hit-test as top-anchored regardless of the actual verticalAnchor. Initialise eagerly at construction and also after setContentHeight so the closure read always reflects the current frame. Also tightens the middle-anchor test bound and marks the deferred pointer-event hit-test gap. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Closes the "text snaps to top while editing" gap left by the prior commits. The slides wrapper forwards verticalAnchor to initializeTextBox so the editing surface applies the same paint and hit-test offsets as the committed slide canvas. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Chunk 3 of the verticalAnchor work is now in: docs editor honors the offset and the slides wrapper threads element.data.verticalAnchor through, so the read-only render and the in-place editor agree. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The getCanvasOffsetTop callback was returning -(pageGap+originY)*scale, which double-applies the anchor offset: a click on a bottom-anchored title's visible text resolved to a layout-y far past the last line, sending the cursor to the wrong block. Correct formula is (originY-pageGap)*scale: a click at host-y = originY*scale produces py = pageGap, so localY = 0 — the very start of the layout. Reduces to -pageGap*scale when originY = 0, preserving the default-path behavior for docs/sheets callers. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Stage 1 of the verticalAnchor UI exposure — expose the field via three "Align text top/middle/bottom" items in the existing slides context menu, reusing Store.updateElementData. Toolbar and side panel surfaces remain follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Prefixes the active choice in radio-group-style menus with a check-mark glyph (and a matching spacer on non-selected items so labels stay column-aligned). No effect on menus that don't set the new field. Used by the upcoming "Align text" items. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Exposes TextElement.data.verticalAnchor for user editing. Items appear only when a single TextElement is selected; multi-select and non-text selections hide them so the action target is unambiguous. The current value is marked with the new ContextMenuItem.selected indicator — "Top" reads as active for unset (default-top) text boxes. Closes Stage 1 of the verticalAnchor UI rollout. Toolbar and side panel surfaces are tracked separately. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Items that leave selected=undefined no longer get the 3-space spacer when a sibling radio group is present — the column was leaking onto Copy/Cut/Paste/etc. when a single text element was selected. Also short-circuits a no-op verticalAnchor write so clicking the currently-selected anchor doesn't add a spurious undo entry. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Code-review polish: the skipped pointer-event spec's body never ran and only inflated the file; collapse to a one-liner with the rationale on top. Surface the drawHint placeholder-ghost-text top-anchor carve-out in the design doc so the limitation is visible without re-deriving it from source. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 21 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 (8)
📝 WalkthroughWalkthroughThis PR implements end-to-end PPTX vertical text anchoring support: parsing ChangesVertical Text Anchor Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Verification: verify:selfResult: ✅ PASS in 248.7s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Capture lessons from the import + render + editor parity work and the Stage 1 menu rollout: click hit-test math is the real failure point (not paint), paintLayout propagates originY internally, first-render closure variables need eager init, and radio-group indicators must be per-item opt-in. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/slides/test/view/editor/editor.test.ts (2)
2400-2403: ⚡ Quick winConsider importing the actual ContextMenuItem type instead of duplicating it.
The local
ContextItemLiketype duplicates the interface definition and could diverge from the actualContextMenuItemtype if the implementation adds or changes fields. Importing and using the real type would catch interface changes at compile time.♻️ Refactor to use the actual type
+import type { ContextMenuItem } from '../../../src/view/editor/context-menu'; + - type ContextItemLike = { label: string; selected?: boolean; run: () => void }; type EditorWithContextItems = { - elementContextItems(slideId: string): ReadonlyArray<ContextItemLike>; + elementContextItems(slideId: string): ReadonlyArray<ContextMenuItem>; }; - function getContextItems(ed: SlidesEditor, slideId: string): ReadonlyArray<ContextItemLike> { + function getContextItems(ed: SlidesEditor, slideId: string): ReadonlyArray<ContextMenuItem> { return (ed as unknown as EditorWithContextItems).elementContextItems(slideId); }🤖 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/test/view/editor/editor.test.ts` around lines 2400 - 2403, Replace the duplicated local type with the real exported type: import ContextMenuItem and change the local declarations — remove ContextItemLike and update EditorWithContextItems.elementContextItems to return ReadonlyArray<ContextMenuItem> (and adjust any usage that referenced the local run/selected fields to the actual ContextMenuItem shape). Ensure the import is added at the top of the test file and update any mock objects to satisfy the real ContextMenuItem interface.
2465-2465: ⚡ Quick winUse
.toBe(false)for consistency with other boolean assertions.This assertion uses
.toBeFalsy()while similar checks in this suite (lines 2438, 2464) use.toBe(true). For boolean flags,.toBe(false)is more precise and maintains consistency.♻️ Suggested fix
- expect(items.find((it) => it.label === 'Align text top')?.selected).toBeFalsy(); + expect(items.find((it) => it.label === 'Align text top')?.selected).toBe(false);🤖 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/test/view/editor/editor.test.ts` at line 2465, Replace the loose falsy assertion with an explicit boolean check to match the rest of the suite: change the assertion that checks items.find((it) => it.label === 'Align text top')?.selected from using .toBeFalsy() to .toBe(false) so the boolean flag is asserted precisely (target the test in editor.test.ts where the items.find(... 'Align text top')?.selected is asserted).
🤖 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-themes-layouts-import.md`:
- Line 438: The table row for `<a:bodyPr anchor>` is missing a third cell (it's
a 2-cell entry in a 3-column table), which breaks Markdown table alignment;
update that row in docs/design/slides/slides-themes-layouts-import.md to include
the missing third column (either an appropriate support/status value or an
explicit empty placeholder like an em-dash) so the row has three pipe-separated
cells, keeping the existing text referencing TextElement.data.verticalAnchor,
detectVerticalAnchor, and computeVerticalOriginY intact.
In `@packages/slides/test/view/editor/editor.test.ts`:
- Around line 2590-2618: The test currently checks that elements don't change
after clicking the already-selected anchor but doesn't assert the undo stack
remained unchanged; capture the undo state before running the menu action (e.g.,
const beforeCanUndo = store.canUndo(), beforeCanRedo = store.canRedo()), run the
no-op via items.find(...).run(), then assert the undo/redo state is unchanged
(expect(store.canUndo()).toBe(beforeCanUndo) and
expect(store.canRedo()).toBe(beforeCanRedo)); add these checks around the
existing before/after element assertions in the same test where editor, store,
getContextItems, and items.find(...).run() are used.
---
Nitpick comments:
In `@packages/slides/test/view/editor/editor.test.ts`:
- Around line 2400-2403: Replace the duplicated local type with the real
exported type: import ContextMenuItem and change the local declarations — remove
ContextItemLike and update EditorWithContextItems.elementContextItems to return
ReadonlyArray<ContextMenuItem> (and adjust any usage that referenced the local
run/selected fields to the actual ContextMenuItem shape). Ensure the import is
added at the top of the test file and update any mock objects to satisfy the
real ContextMenuItem interface.
- Line 2465: Replace the loose falsy assertion with an explicit boolean check to
match the rest of the suite: change the assertion that checks items.find((it) =>
it.label === 'Align text top')?.selected from using .toBeFalsy() to .toBe(false)
so the boolean flag is asserted precisely (target the test in editor.test.ts
where the items.find(... 'Align text top')?.selected is asserted).
🪄 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: f9645dcc-93c1-4196-9d58-da910ee72704
📒 Files selected for processing (17)
docs/design/slides/slides-themes-layouts-import.mddocs/tasks/active/20260529-slides-pptx-text-vertical-anchor-todo.mddocs/tasks/active/20260529-slides-text-vertical-align-menu-todo.mdpackages/docs/src/view/text-box-editor.tspackages/docs/test/view/text-box-editor.test.tspackages/slides/src/import/pptx/shape.tspackages/slides/src/import/pptx/text.tspackages/slides/src/index.tspackages/slides/src/model/element.tspackages/slides/src/view/canvas/text-renderer.tspackages/slides/src/view/editor/context-menu.tspackages/slides/src/view/editor/editor.tspackages/slides/src/view/editor/text-box-editor.tspackages/slides/test/import/pptx/text.test.tspackages/slides/test/view/canvas/text-renderer.test.tspackages/slides/test/view/editor/context-menu.test.tspackages/slides/test/view/editor/editor.test.ts
- Split the verticalAnchor support-matrix row into the table's three columns (was 2 cells in a 3-column table → MD056 lint warning, could misalign rendering). - Strengthen the idempotent-click test to also assert canUndo/canRedo stay unchanged so a future regression in the short-circuit guard fails the spec instead of silently pushing no-op undo entries. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
May 31, 2026
PRs #257/#301/#303/#304/#305/#306/#307/#309/#315/#316/#317/#321/#322 shipped between v0.4.2 and the v0.4.3 cut, leaving their paired todo files in active with the process checkboxes still open. Flip the remaining boxes for the work that landed, then move the paired todo/lessons into docs/tasks/archive/2026/05/ via pnpm tasks:archive. Active count drops from 17 to 6; archive count grows by 21 files. Patch release covering 29 commits since v0.4.2. Headline theme is Slides editing & UX polish toward Google Slides / PowerPoint parity: smart guides (equal-spacing / distance / size), Shift drag modifiers, text inside shapes, a Format options right panel, tier-1 universal toolbar controls, text autofit (shrink + grow), group-selection visuals, and several smaller text-box / selection fixes. PPTX import gained paragraph-level bullet styles, body anchor (vertical alignment), and stretched blipFill cover-crop. Docs added a pending-inline-style at collapsed caret, a caret that tracks the resolved text color, bullet indent on Tab / Shift+Tab, font / size / line-spacing / clear formatting toolbar controls, an inline-style-attribute clear that actually lands in the CRDT, and a polished toolbar trigger / color swatch pass shared across docs / sheets / slides. Sheets gained .xlsx import. Infrastructure: Docker images now build on native arm64 runners, fixing the release hang seen on v0.4.2. No Prisma migration, no new backend env vars — the devops bump is a routine image-tag change. 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:bodyPr anchor="t |ctr|b">into a new sparseTextElement.data.verticalAnchorfield (top/middle/bottom). Absent preserves the previous top-anchored behavior — no migration for existing decks.computeVerticalOriginYintext-renderer.ts) and in the docs in-place text-box editor so paint, caret, selection rectangles, and click hit-test all align with the configured anchor. Imported titles inYorkie, 캐즘 뛰어넘기.pptxslide 1 (and similar PPTX decks that bottom-anchor a tall placeholder) now render at the bottom of the frame instead of floating ~2" above.verticalAnchorvia three "Align text top / middle / bottom" items in the slides context menu when a singleTextElementis selected — Stage 1 of the UI rollout. Items mark the current value with a check-mark column (via a new opt-inContextMenuItem.selectedfield), short-circuit idempotent clicks, and write throughstore.batchso each change is a single undo step. Toolbar / side-panel surfaces remain explicit follow-ups.Test plan
pnpm verify:fast— lint + 803 docs + 1420 slides unit tests, 1 documented skip (jsdom canvas-geometry limitation; algebra verified in JSDoc + Playwright handoff noted)detectVerticalAnchor): coverst/ctr/b/just/dist, missingbodyPr, missing/emptyanchorattr; integration spec round-trips throughparseSpTreedrawText+computeVerticalOriginY): all three anchors land in the expected band on a 200 px frame, overflow clamps to top, default (absent) path unchangedcurrentOriginYeagerly initialized beforeTextEditoris constructed (no first-render race);setContentHeightresyncs; click hit-test math(currentOriginY - Theme.pageGap) * scalereduces to-pageGap * scalefor the default case (proven by walk-through + existing tests)selected; multi-select / non-text selections hide the items; idempotent click guard verifiedverticalAnchorrender identically; existing menus withoutContextMenuItem.selectedrender identically (existing specs unchanged)Yorkie, 캐즘 뛰어넘기.pptxviapnpm dev, confirm slide 1 title sits at the bottom of its placeholder, enter edit mode and verify the caret stays aligned🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation