feat(footnote): rendering fidelity (SD-2656)#3220
Open
tupizz wants to merge 9 commits into
Open
Conversation
Make the body paginator demand-aware so footnote-heavy documents pack
body content tight to the separator instead of letting the post-hoc
reserve loop leave visible blank space above the footnote band.
Measured on Harvey NVCA Model SPA (108 footnote refs):
- BEFORE: 57 pages
- AFTER: 53 pages
- Word baseline: 51 pages (within +5%)
Mechanism
---------
PageState gains two fields:
- pageFootnoteReserve : existing per-page reserve, now exposed
to the break decision
- footnoteDemandThisPage : accumulator of measured footnote body
heights for refs anchored on this page
Paragraph layout consults a new optional callback:
- getFootnoteDemandForBlockId(blockId): number
The break decision uses an effective bottom:
additionalDemand = max(0, footnoteDemandThisPage - pageFootnoteReserve)
effectiveBottom = state.contentBottom - additionalDemand
Once the convergence loop has set a correct reserve, additionalDemand is
0 and the new code is a no-op. On pass 1 (no reserve), it provides the
tight-packing signal that prevents the body from filling the page only
to be clawed back by a later reserve relayout.
A safety cap clamps additionalDemand so the page always has room for at
least one body line - otherwise an oversized footnote would drive
effectiveBottom below cursorY and the paginator would advanceColumn
indefinitely.
The per-block demand lookup is built once per layoutDocument call. It
walks the block tree, including table cells (rows[].cells[].blocks /
.paragraph), and resolves each ref's pos to the containing top-level
block. Table-cell refs are attributed to the table block, the unit the
body paginator places on a page.
layout-bridge populates bodyHeightById from measures via
refreshBodyHeights and pre-measures every footnote on every convergence
iteration so migrating refs do not drop from the lookup mid-loop.
Tests
-----
- footnoteBodyDemand.test.ts RED-then-GREEN for block-aware break
+ no-op invariant for non-footnote docs
- footnoteContinuationDemand converged layout reserves carry-forward
demand on the continuation page
- footnoteRefMigration determinism regression: repeated runs
produce identical page counts, reserves,
and ref to page assignments
Refs: SD-2656 SD-3049 SD-3050 SD-3051
Plan: docs/plans/sd-2656-footnote-rendering-fidelity.md
Report: docs/plans/sd-2656-implementation-report.md
…986 SD-2658) Inline footnote references and the leading marker inside the footnote body now honor the OOXML number format / start configured in w:settings/w:footnotePr. Custom-mark refs (customMarkFollows="1") emit an empty marker run so the literal symbol in the next OOXML run renders as the visible mark. Supported formats: decimal, upperRoman, lowerRoman, upperLetter, lowerLetter, numberInDash. Unknown formats fall back to decimal. Single source of truth between the inline ref and the leading marker: pm-adapter/src/footnote-formatting.ts -> formatFootnoteCardinal() Used by: pm-adapter/.../converters/inline-converters/footnote-reference.ts super-editor/.../layout/FootnotesBuilder.ts The formatter switch is intentionally inlined (not imported from @superdoc/layout-engine's formatPageNumber) because pm-adapter sits upstream of layout-engine in the package graph - see Guard C in layout-engine/tests/src/architecture-boundaries.test.ts. A drift detection parity test asserts the two helpers agree on every supported format for cardinals 1..100: layout-engine/tests/src/footnote-formatter-parity.test.ts Settings readers in super-editor/document-api-adapters/document-settings: readFootnoteNumberFormat(settingsRoot): string | null readEndnoteNumberFormat(settingsRoot): string | null readFootnoteNumberStart(settingsRoot): number | null readEndnoteNumberStart(settingsRoot): number | null PresentationEditor reads all four up-front and threads the values through ConverterContext.footnoteNumberFormat / .endnoteNumberFormat and the per-doc cardinal counter is seeded with the configured start. customMarkFollows handling preserves pmStart/pmEnd on the empty marker run so click and selection continue to work at the ref position. Refs: SD-2656 SD-2986 SD-2986/B1 SD-2986/B2 SD-2658 SD-2662
End-to-end documentation for the footnote rendering fidelity epic:
docs/superdoc-feature-reports/sd-2656-plan.md
Original implementation plan: ticket inventory across the epic,
OOXML grounding (§17.11), code surface map with line numbers,
surgical approach for each slice, RED test scaffolds, falsifiable
success criteria.
docs/superdoc-feature-reports/sd-2656-implementation-report.md
What shipped, with measurements:
- Harvey NVCA: 57 -> 53 pages (Word baseline 51, +5%)
- pnpm test:layout vs superdoc@1.32.0:
535/543 docs (98.5%) byte-identical
5 unique-change docs, all NVCA-style footnote-rich legal
templates (the intended scope)
- pnpm test:visual: "no visual differences found"
- 16,649 unit tests across 5 packages, all green
Slice-by-slice walkthrough (SD-3049 / 3050 / 3051 / 2986/B1+B2 /
2658 / 2662), architecture compliance (Guard C parity test),
pr-reviewer findings + resolutions, deferred work, repro commands.
Refs: SD-2656
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… numFmt, cache key) - Re-charge block footnote demand after each advanceColumn so a paragraph that spills mid-iteration leaves the new page with the right effective bottom — previously the recharge only fired at iteration top, and a block that finished its content on the spilled-onto page never charged its demand there, letting later blocks fill into the footnote band. - Wire endnoteNumberFormat through endnoteReferenceToBlock and EndnotesBuilder via the shared formatFootnoteCardinal so documents with w:endnotePr/w:numFmt render the configured format on both the inline ref and the leading marker. - Fold numberStart and numberFormat into the FlowBlockCache invalidation signatures so settings.xml mutations that change numbering format or starting cardinal evict stale cached reference runs. - refreshBodyHeights mirrors computeFootnoteLayoutPlan: read measure.height for image and drawing footnote content so the SD-3049 tight-pack signal fires for non-text footnotes. Tests: - layout-paragraph.test.ts: demand survives advanceColumn within one iteration - endnote-reference.test.ts: numFmt cases (upperRoman, lowerRoman, fallbacks) - footnoteBodyDemand.test.ts: tight gap for image-only footnotes Refs: SD-2656
- refreshBodyHeights now handles list-kind measures (per-item paragraph line heights + spacingAfter), mirroring buildFootnoteRanges. Without it list-only footnotes contributed zero demand to the SD-3049 tight-pack signal and re-introduced the blank body-to-separator gap. - FootnotesBuilder captures customMarkFollows on the inline ref and skips the leading marker injection in the footnote body for those ids. Matches the exporter contract: custom-mark footnotes have no w:footnoteRef in note content; the literal symbol in the document body is the entire identification. Tests: - footnoteBodyDemand.test.ts: tight gap for a list-only footnote - FootnotesBuilder.test.ts: customMarkFollows ref does not inject a marker run
The footnote band already renders each id once per page via assignFootnotesToColumns. Block-aware body demand must match: when the same id is referenced multiple times on a page, contribute its body height once. Previously refByPos kept every occurrence, so two refs to the same footnote on a page reserved 2× the real height and the body paginator left phantom whitespace above the separator at convergence. The dedup keeps the first ref position per id (sufficient for the walker, which only needs to attribute demand to *some* containing block). Test: 25 body paragraphs, footnote referenced twice — page 1 must pack tight with no extra whitespace.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64f2059ef4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The block-aware break re-charged blockFootnoteDemand on every page transition. For a long paragraph that spans pages with a footnote ref on the first one, continuation pages got the demand subtracted from their effective body region even though no footnote band renders there — packing 13–15 lines per page instead of 20 and producing unnecessary extra pages. Lock the charge after the first fragment commits. The spill case (Fix 1, paragraph's first fragment lands after advanceColumn) still works because re-charging still happens until the first commit; once the fragment is on the page, the lock prevents continuation pages from seeing phantom demand. Test: 50-line paragraph with a single ref on a 20-line-per-page layout converges to 3 pages (was 4 with per-page recharge).
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
3 participants
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
w:settings/w:footnotePr/w:numFmtandw:numStart. Custom marks (customMarkFollows="1") emit an empty marker run so the literal symbol renders. Single source of truth between inline ref and leading marker.Full slice-by-slice walkthrough, measurements, architecture compliance notes, pr-reviewer findings, and repro commands in
docs/superdoc-feature-reports/sd-2656-implementation-report.md.Tickets covered
w:numFmtw:numStartcustomMarkFollows)Headline results
harvey-problem-docs/NVCA Model SPA.docx(108 footnote refs)Layout-snapshot regression (
pnpm test:layoutvs superdoc@1.32.0)lineCount,textIndentPx)Pixel diff (
pnpm test:visual)Final tool verdict: "Pixel comparison complete. No visual differences found." Report at
devtools/visual-testing/results/2026-05-09-17-27-55-v.1.32.0/webkit/report.html.Test plan
pnpm --filter @superdoc/layout-bridge test --run— 1 211 tests (incl. 3 new footnote tests)pnpm --filter @superdoc/layout-engine test— 649 testspnpm --filter @superdoc/pm-adapter test --run— 1 796 tests (incl. customMarkFollows + position preservation)pnpm --filter @superdoc/super-editor test --run— 12 699 testspnpm --filter @superdoc/layout-tests test --run— 294 tests (architecture-boundaries + new formatter parity)pnpm test:layout --reference 1.32.0— 5 unique-change docs, all in target scopepnpm test:visual— no visual differencesmainTotal unit tests: 16 649, all green.
Architecture compliance
architecture-boundaries.test.tsenforced: pm-adapter does not import@superdoc/layout-engineat runtime.pm-adapter/src/footnote-formatting.tsinlines the format switch instead of importingformatPageNumber. A drift-detection parity test inlayout-engine/tests/src/footnote-formatter-parity.test.tsasserts the two implementations agree on cardinals 1–100 for every supported format.Known limitations (documented in the report)
footnoteReservedByPageIndex. ExistingfootnoteColumnPlacement.test.tsensures correctness.lowerRoman: falls back to decimal if absent. One-line fix inPresentationEditor.tswhen corpus shows demand.w:numRestartper-page / per-section: out of scope; couples numbering to layout output. SD-2986 successor.Repro