Import paragraph-level PPTX bullet style for consistent markers#317
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 8 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 (1)
📝 WalkthroughWalkthroughAdds Block.marker to docs model; prefers block.marker in renderListMarker; parses PPTX paragraph buFont/buSzPts/buClr and master <p:txStyles> into txStylesMarkers, merges defaults into paragraph markers, threads marker defaults through import pipeline, preserves marker through editor paste, scales marker.fontSize in autofit, and adds tests/docs. ChangesList Bullet Marker Styling
🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 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 |
Verification: verify:selfResult: ✅ PASS in 262.6s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
078f6b7 to
f0bb527
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tasks/active/20260530-pptx-paragraph-bullet-style-todo.md`:
- Around line 66-73: The "Out of scope" note incorrectly claims
layout/master-level bullet style inheritance isn't implemented; update the text
to reflect the shipped behavior by noting that
packages/slides/src/import/pptx/master.ts parses txStyles and
packages/slides/src/import/pptx/text.ts merges level defaults with paragraph
overrides (so master/layout-level inheritance is supported), and adjust the
checklist items accordingly to avoid misleading future readers.
In `@packages/slides/src/import/pptx/text.ts`:
- Around line 228-242: The mergeMarkers function currently returns the passed-in
BlockMarker objects directly (base or overrides), which allows shared objects
(e.g., markerDefaults) to be mutated by callers; change mergeMarkers so every
return of a BlockMarker returns a fresh shallow-cloned object: when base or
overrides is returned alone, return a clone (e.g., {...base} or {...overrides}),
and when merging use the existing out object as the new combined clone; keep
returning undefined when no properties exist. Ensure you update references in
mergeMarkers to always produce new BlockMarker objects to avoid sibling
mutation.
🪄 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: 81c5084b-7480-47a0-99a8-8925cf2192cd
📒 Files selected for processing (15)
docs/tasks/active/20260530-pptx-paragraph-bullet-style-todo.mdpackages/docs/src/index.tspackages/docs/src/model/types.tspackages/docs/src/view/paint-layout.tspackages/docs/src/view/text-editor.tspackages/docs/test/view/render-list-marker.test.tspackages/slides/src/import/pptx/index.tspackages/slides/src/import/pptx/master.tspackages/slides/src/import/pptx/shape.tspackages/slides/src/import/pptx/slide.tspackages/slides/src/import/pptx/text.tspackages/slides/src/model/autofit.tspackages/slides/test/import/pptx/master.test.tspackages/slides/test/import/pptx/text.test.tspackages/slides/test/model/autofit.test.ts
f0bb527 to
845fb24
Compare
On a Slides deck with the Simple Dark theme (and any future deck theme that paints light text on a dark background), the text-edit caret was rendered with the docs `Theme.cursorColor`. That theme knows about light/dark mode of the docs package itself but not about per-deck slide backgrounds, so the caret painted in dark ink and was effectively invisible against the slide background. Match Google Docs / Slides behaviour: the caret picks up the resolved text color at the cursor position — light text yields a light caret, red text yields a red caret. Adds `resolveColorAtPosition` next to the existing `defaultColorResolver`, widens the three caret paint sites (`paintLayout`, `DocCanvas` body / header / footer) with an optional `cursor.color` field, and threads the resolved color from the slides text-box editor and the docs full editor. Falls back to `Theme.cursorColor` when no color is supplied so existing callers keep their current rendering. Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
PowerPoint stores bullet font/size/color on `<a:pPr>` via
`<a:buFont>`/`<a:buSzPts>`/`<a:buClr>` so the marker glyph stays
consistent regardless of the run's font. Wafflebase ignored those
paragraph attributes, so `renderListMarker` had to inherit from
the first inline — and the Hangul fallback in the importer makes
that first inline vary across bullets within one text box.
In the Yorkie "캐즘 뛰어넘기" deck this surfaced on slide 2: bullets
1–3 begin with English ("April, 2019: ") so their first inline stays
on Arial, while bullet 4 starts with "Oct, 2022: Yorkie, 캐즘 뛰어넘기"
and gets Noto Sans KR. At the same 18 pt the Noto `●` is ~2x taller
than Arial's, so bullet 4's orange marker visibly dwarfs the others
even though every paragraph authors the same `<a:buSzPts val="1800"/>`.
Add an optional `Block.marker` field (`BlockMarker`) for authored
marker style, populate it from `<a:buFont>`/`<a:buSzPts>`/`<a:buClr>`
during PPTX import (only on list-items), and teach the docs marker
painter to prefer it per-axis, falling back to the first inline when
unset so existing DOCX-imported and native docs continue rendering
unchanged. Existing Yorkie documents need a re-import to pick up
the new field; new imports get correct markers automatically.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Self-review uncovered three downstream paths that ignored the new optional Block.marker, leaving subtle gaps once it carries meaningful data: - scaleBlocks() scaled inline fontSize but left marker.fontSize alone, so a shrink-autofit text box would render the marker glyph at the authored 18pt while body text dropped to the fit scale — the inverse of the bug the previous commit fixed. The "Yorkie 캐즘" deck's body shape uses normAutofit, so this is reachable. - getSelectedBlocks() enumerated cloned block metadata by field name and didn't copy marker, so a copy/paste round-trip silently dropped authored markers on the head and tail blocks (middle blocks were spread-cloned and survived). - YorkieDocStore doesn't (de)serialize marker through Yorkie Tree node attributes. Today no docs feature authors marker style, so this is latent; document the gap on the Block.marker jsdoc so a future docs feature or DOCX round-trip doesn't lose data silently. Also tightens the renderer's color fallback to `??` for axis-uniform style (StoredColor has no falsy legal value). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
The previous commit read paragraph-level <a:buFont>/<a:buSzPts>/<a:buClr>
into Block.marker, but live-imported decks still showed the marker
glyph drift. Root cause: the "Yorkie 캐즘" deck (and most PowerPoint
exports) inlines only <a:buSzPts>/<a:buChar> on the paragraph while
the marker typeface (<a:buFont typeface="Arial"/>) lives exclusively
in the master's <p:txStyles><p:bodyStyle>. The importer ignored that
slot, so marker.fontFamily stayed undefined and renderListMarker fell
back to inlines[0]'s font — drifting back to Noto Sans KR on the
Hangul-starting bullet 4.
Parse <p:txStyles> in the master importer into a slot × level map
(`TxStylesMarkers`), thread it through ParseSlideOptions and
SlideParseContext, look up the right slot inside buildTextElement from
the host shape's <p:ph type>, and merge the level default *under* the
paragraph's own marker so per-paragraph overrides still win per-axis.
With this change, all four bullets of slide 2 import as
`{ fontFamily: 'Arial', fontSize: 18, color: ... }`, matching what
PowerPoint and Google Slides render.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Second-pass code review caught three small items on top of the master inheritance fix: - The task doc's "Out of scope" section claimed master/layout inheritance was deferred, but commit 3 of this PR landed master inheritance. Rewrite the section as a "Re-validated gap" that explains why the original assumption was wrong, and narrow the remaining out-of-scope to layout-placeholder bullet defaults plus <a:buSzPct>. - Paste's middle-block clone in text-editor's pasteBlocks path was the only branch that didn't deep-copy Block.marker (head and tail blocks did). Mirror the spread treatment so mutations to a pasted block can't leak into the clipboard payload. - Add a comment on placeholderTypeToTxStylesSlot's `big-number` / `caption` branches explaining they're Waffle-only placeholder kinds the importer never produces from PPTX today — kept for forward routing rather than dead code. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
CodeRabbit caught a subtle aliasing bug in mergeMarkers: when one of its inputs was undefined it returned the other one verbatim. Since the `base` argument is the master-level `markerDefaults` map entry shared across every paragraph that resolves to the same slot × level, a downstream mutation on the resulting `Block.marker` (clearFormatting, theme remap, persisted-snapshot round-trip, …) would silently corrupt every other list item that inherits the same default. Clone in the single-input branches too. Marginal alloc cost — one small object per list paragraph — against a real correctness hazard. Regression test asserts two paragraphs that share a master default end up with distinct marker references and that mutating one doesn't bleed into the other or into the shared default. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
845fb24 to
eb13ab2
Compare
The browser visual harness intermittently captured the system-fallback serif glyph for previewed families like "Roboto" and the theme card "aA" labels instead of the actual Google web font. The existing `document.fonts.load()` + `fonts.ready` block was racing the slides ThemedFontPicker, which injects the Google Fonts CSS link from a child mount effect — so the first `fonts.load()` call ran before the link was in the DOM and resolved against the system family. The race used to manifest only on `mobile.dark`; small bundle perturbations (e.g. adding a type export) can flip the timing on `desktop` too, surfacing as a baseline mismatch on the top-level `harness-visual.browser.png` capture while every individual scenario stays stable. Stop relying on `fonts.ready` alone: - First pass loads / drains the queue the page already knows about. - A second `networkidle` wait gives any late-injected link time to fetch. - Second pass loads again, then polls `document.fonts.check()` until every family is ready for synchronous rendering. `check()` is strictly stronger than `ready`, which can resolve before late-mounted families register. Refresh the `harness-visual.browser.png` baseline (desktop / light) from the CI-captured actual so the locked render is the fonts-loaded one, not the fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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]>
Summary
<a:buFont>,<a:buSzPts>,<a:buClr>during PPTX import into a new optionalBlock.marker(BlockMarker) field, and teachrenderListMarkerto prefer it per-axis (with first-inline fallback for non-PPTX paths).Block.markerthroughscaleBlocks(so shrink-autofit scales the bullet glyph in lockstep with body text) and throughgetSelectedBlocks+ paste head/tail blocks (so clipboard round-trips keep the authored marker).Block.markerjsdoc thatYorkieDocStoredoes not persist the field today — slides round-trip it via element-level JSON; docs editor doesn't yet author markers, so the gap is latent.Why
In the "Yorkie, 캐즘 뛰어넘기" PPTX deck (slide 2), the fourth
●bullet renders much larger than the first three even though every paragraph authors the same<a:buSzPts val="1800"/>. Root cause: the importer dropped paragraph-level bullet style, sorenderListMarkerinherited font/size/color fromblock.inlines[0].style— and the HangulfontFamilyfallback (which only fires on runs whose own text contains Hangul) leaves the English-prefix first run on Arial for bullets 1-3 while the all-Korean bullet 4 gets Noto Sans KR. At 18 pt Noto Sans KR's●glyph is ~2× taller than Arial's, hence the visible mismatch. PowerPoint and Google Slides render the marker through paragraph bullet attributes, independent of run font.See
docs/tasks/active/20260530-pptx-paragraph-bullet-style-todo.mdfor the full investigation and approach.Test plan
pnpm verify:fastgreen (locally green at 053/53 files, 839/840 tests)parseTextBodypopulatesBlock.markerfrom<a:buFont>/<a:buSzPts>/<a:buClr>and routes scheme colors throughclrMapparseTextBodydoes NOT attachmarkeron non-list paragraphs (e.g.<a:buNone/>)renderListMarkerprefersblock.markerand falls back per-axis to the first inlinescaleBlocksscalesmarker.fontSizein lockstep with inline runs and preserves color-only markers untouchedYorkie, 캐즘 뛰어넘기.pptx, open slide 2, verify all four●markers render at the same glyph size and only bullet 4 is colored orange. (Existing Yorkie documents imported before this PR will continue to show the bug until re-imported — flagged in the task todo.)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation