Skip to content

Write one tableParaStyle entry per paragraph (#81) - #82

Merged
leogdion merged 2 commits into
v0.1.xfrom
81-multi-paragraph-formatting
Aug 3, 2026
Merged

Write one tableParaStyle entry per paragraph (#81)#82
leogdion merged 2 commits into
v0.1.xfrom
81-multi-paragraph-formatting

Conversation

@leogdion

@leogdion leogdion commented Aug 3, 2026

Copy link
Copy Markdown
Member

Fixes the bug behind both the #64 monospace failure and the dark-on-dark code panel in #78's render pass: item-level formatting reached only the first paragraph of a multi-paragraph text box.

Branches off v0.1.x directly — independent of the #78 and #65 stack.

Cause

collapsedEntries run-length collapsed adjacent identical paragraph formats into a single tableParaStyle entry at offset 0. That reads like a reasonable optimization, but Keynote treats each entry as a paragraph boundary marker, so collapsing left later paragraphs unstyled.

The archive stayed valid and nothing errored — the defect was only visible by opening a slide, which is why it survived #37's render verification (that deck is one paragraph).

What the fixture says

A human-authored 5-paragraph body storage in build_action_B.key:

STORAGE id=2651751  text=Body Level One\nBody Level Two\n…
  paraEntries=5
    char=0  -> 2651127
    char=15 -> 0
    char=30 -> 0
    char=47 -> 0
    char=63 -> 0

One entry per paragraph. The style rides entry 0; every later boundary carries identifier 0 = "same as the preceding entry". The entry must exist even though it references no record.

Fix

paragraphEntries emits one entry per paragraph, writing identifier 0 where a format repeats. Fork records are still deduped — the dedupe is on records, not entries. applyParagraphStyles filters identifier-0 entries out of the header references, since 0 isn't a record.

Two existing tests changed

itemWideAlignment and dedupeCollapsesEntries asserted the collapsing behavior — they encoded the bug. Both now assert the per-paragraph shape. Flagging explicitly since changing a green test to make a fix pass deserves scrutiny; the fixture evidence above is why I'm confident the new assertion is the correct one.

Everything uses three or more paragraphs

Deliberate. A two-paragraph case exercises only the first and last and passes while interior paragraphs stay broken — which is exactly how the per-span variant of this bug survived the first #64 probe round, where iiii and 1111 rendered correctly and MMMM didn't.

Verification

  • swift test — 70 green, including a new MultiParagraphFormattingTests suite
  • LINT_MODE=STRICT ./Scripts/lint.sh — exit 0
  • swift run AcceptanceDecksmulti_paragraph_formatting.key writes and self-checks

Structural only. This bug was invisible to structural checks by definition, so the render pass is the real gate.

Render checklist (yours)

Deck generated at /Users/leo/Downloads/para-fix/multi_paragraph_formatting.key.

  1. Slide 1 — all three lines Menlo, equal width, large. Before the fix only iiii was.
  2. Slide 2 — all five lines bold and red.
  3. Slide 3 — the middle line matches its neighbours. This is the case per-span styling missed.
  4. Slide 4 — left / centre / right / left / left alignment still honored, i.e. the fix didn't regress TextBox layout & list styling: plain-by-default, list styles, alignment/indent, rotation, columns #51.

If this renders green, #66 unblocks and I'd also restore the multi-line code sample in #78's background_fill slide 1.

Fixes #81

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c1215832-86bc-4718-aed8-a9ea37053635

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@leogdion

leogdion commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

Author — the crash was mine, and it's fixed. Thanks for catching it; a crash on open is a worse failure than the unstyled render this PR set out to fix.

What happened

The per-paragraph entries were the right idea. Writing them was not.

For a repeat entry I set entry.object.identifier = 0. In swift-protobuf that materializes the optional object message as a present-but-empty TSP.Reference. Keynote resolves it to nil and crashes — the same id-0 trap already recorded twice in agent-notes.md, once for RecordCloner remaps and once for plain character spans.

I saw the template's repeats reported "identifier 0" and generalized from that without checking how they actually serialize. The bytes are unambiguous:

template repeat:  [08 0f]         <- characterIndex only, no object field
what I wrote:     [08 0f 12 00]   <- present-but-empty reference

Reading the identifier back cannot distinguish them — absent and zeroed both report 0. Only hasObject does. That is also why my original test passed: #expect(entries[1].object.identifier == 0) is true in both the correct and the crashing shape.

The fix

  1. applyParagraphStyles assigns object only for real identifiers.
  2. UUIDMapVerifier rule 7 — no storage object-attribute entry may carry a present-but-empty reference, checked across tableParaStyle, tableCharStyle, and tableListStyle. Every deck goes through the verifier on write, so this class of crash can't ship again from any code path, not just this one.
  3. A regression test asserting hasObject is false and the entry serializes to exactly [08 04].

I confirmed the new test fails against the crashing version before restoring the fix — a test that can't fail would have been worthless here, and the previous one proved that.

Verification

  • 71 tests green, STRICT lint exit 0
  • All 12 acceptance decks regenerate clean under rule 7
  • Audited the two other object.identifier = assignments in the surgeon: both always receive real identifiers (character runs mint a style per the Support mixed formatting (runs) within a single text box #40 directive, list styles are always real), so neither can hit this

Re-render, please

Fresh deck at /Users/leo/Downloads/para-fix2/multi_paragraph_formatting.key.

  1. It should open at all — that's the new bar
  2. Slide 1 — all three lines Menlo, equal width, large
  3. Slide 2 — all five lines bold red
  4. Slide 3 — the middle line matches its neighbours
  5. Slide 4 — left / centre / right / left / left still honored

Recorded in research/findings/text_formatting.md and as a standing directive in agent-notes.md, since the distinction is invisible to the obvious assertion.

leogdion and others added 2 commits August 3, 2026 13:52
Item-level `.font`/`.fontSize`/`.foregroundColor` reached only the FIRST
paragraph of a multi-paragraph text box. Keynote rendered the rest at the
template default, silently — the archive stayed valid, so nothing failed
until a human looked at a slide.

`collapsedEntries` run-length collapsed adjacent identical paragraph formats
into one entry at offset 0. That reads like a reasonable optimization, but
Keynote treats each `tableParaStyle` entry as a paragraph boundary marker,
so collapsing left later paragraphs unstyled.

The fixture settles the intended shape. A human-authored 5-paragraph body
storage in `build_action_B.key` writes FIVE entries:

    char=0  -> 2651127
    char=15 -> 0
    char=30 -> 0
    char=47 -> 0
    char=63 -> 0

The style rides entry 0; later boundaries carry identifier 0, meaning "same
as the preceding entry". The entry must exist even though it references no
record.

`paragraphEntries` emits one entry per paragraph, using identifier 0 for a
repeat. Fork records are still deduped — the dedupe is on records, not
entries. `applyParagraphStyles` filters identifier-0 entries out of the
header references, since 0 is not a record.

Two existing tests asserted the collapsing behavior; they encoded the bug, so
they now assert the per-paragraph shape instead. Added
`MultiParagraphFormattingTests` and a `multi_paragraph_formatting`
acceptance deck.

Everything uses THREE or more paragraphs deliberately: a two-paragraph case
exercises only the first and last and passes while interior paragraphs stay
broken. That is exactly how the per-span variant of this bug survived the
first #64 probe round.

`swift test` 70 green, `LINT_MODE=STRICT ./Scripts/lint.sh` exit 0.
Structural only — the render pass is the real gate.

Refs #81, #64, #66

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Leo's render pass crashed Keynote on open. My fault, and a worse failure than
the bug being fixed.

The per-paragraph entries were right; writing them was not. I set
`entry.object.identifier = 0` for a repeat, which MATERIALIZES a
present-but-empty `TSP.Reference`. Keynote resolves that to nil and crashes —
the same id-0 trap already recorded twice in agent-notes, for `RecordCloner`
remaps and for plain character spans. I read the template's repeats as
"identifier 0" and generalized from that without checking how they serialize.

The wire bytes are unambiguous:

    template repeat:  [08 0f]         characterIndex only, no object field
    what I wrote:     [08 0f 12 00]   present-but-empty reference

Reading the identifier back cannot tell them apart — absent and zeroed both
report 0. Only `hasObject` does.

## Changes

- `applyParagraphStyles` assigns `object` only for real identifiers
- `UUIDMapVerifier` gains **rule 7**: no storage object-attribute entry may
  carry a present-but-empty reference, across `tableParaStyle`,
  `tableCharStyle`, and `tableListStyle`. Every deck now passes through it,
  so this class of crash cannot ship again from any code path.
- A regression test asserting `hasObject` is false and the entry serializes
  to exactly `[08 04]`. Verified it FAILS against the crashing version —
  asserting `identifier == 0` alone passes on both, which is precisely why
  the original test missed it.

All 12 acceptance decks regenerate clean under rule 7. 71 tests green,
STRICT lint exit 0.

Refs #81

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@leogdion
leogdion force-pushed the 81-multi-paragraph-formatting branch from 0bc36b5 to 672eb1e Compare August 3, 2026 17:53
@leogdion
leogdion merged commit cb6a2fe into v0.1.x Aug 3, 2026
5 checks passed
leogdion added a commit that referenced this pull request Aug 3, 2026
Rebase integration. #82 landed `MultiParagraphFormattingContent` and its
tests while this branch was open; they use `TextColor`, which this branch
renames to `Color`. Mechanical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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