[1/3] refactor(painter): consume only ResolvedLayout (SD-2836)#3116
[1/3] refactor(painter): consume only ResolvedLayout (SD-2836)#3116
Conversation
![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: 33e0471db3
ℹ️ 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".
| page: ResolvedPage, | ||
| pageIndex: number, | ||
| kind: 'header' | 'footer', |
There was a problem hiding this comment.
Passing ResolvedPage into header/footer providers changes the runtime object shape that third-party providers receive (Page used to expose fields like size and fragments, while ResolvedPage does not). Any existing provider that reads those legacy fields will now get undefined (or throw) and can misplace or fail to render decorations, even when using the unchanged createDomPainter API surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This concern is theoretical and doesn't apply to the current codebase:
@superdoc/painter-domis"private": true— not published to npm.- No consumers of
createDomPainterorPageDecorationProviderexist outside the layout-engine workspace and super-editor (verified via grep acrosspackages/superdoc/src/,packages/react/src/, andexamples/). - The only callers are internal:
PresentationEditor→HeaderFooterSessionManager.createDecorationProvider→PresentationPainterAdapter→ painter. SuperDoc's public API exposes no hook for consumers to register their own provider, so no third-partyPage-shaped provider exists in practice.
The type is re-exported via superdoc's generated .d.ts — a consumer doing a deep import from superdoc/dist/layout-engine/... would see the signature change — but that's not a supported import path.
The whole point of SD-2836 ("dumb painter") is to make ResolvedLayout the single rendering contract, so adapting back to legacy Page at the provider call site would contradict the north-star. Resolving as a non-issue.
Move the inline-newline run expander from @superdoc/pm-adapter into @superdoc/contracts. The function is a pure transformation on Run[] shapes already defined here; relocating it lets painter-dom consume the helper without depending on pm-adapter (per the SD-2836 acceptance criterion: no painter-dom imports from pm-adapter or layout-bridge). pm-adapter chains its public re-export through contracts, keeping the import path stable until painter-dom is migrated to consume directly.
Move the line-aware run slicer from @superdoc/layout-bridge into @superdoc/contracts, alongside expandRunsForInlineNewlines. The function is a pure transformation on Run/Line shapes already defined here; relocating it lets painter-dom consume the helper without depending on layout-bridge (per the SD-2836 acceptance criterion). layout-bridge chains its public re-export through contracts, keeping the import path stable until painter-dom is migrated to consume directly. Also restore a TrackedChangeMeta import in pm-adapter's paragraph.test.ts that the prior commit dropped; the type is still referenced outside the migrated describe block.
Switch the painter's two cross-package run-helper imports (expandRunsForInlineNewlines, sliceRunsForLine) to source from @superdoc/contracts directly, then drop @superdoc/pm-adapter and @superdoc/layout-bridge from painter-dom's runtime dependencies. This closes the painter-dom -> pm-adapter / layout-bridge import edge called out in the SD-2836 acceptance criteria. Architecture-boundary guards added in [3/3] of this stack will prevent reintroduction.
Add a `fragment: Fragment` field to ResolvedFragmentItem, ResolvedTableItem, ResolvedImageItem, and ResolvedDrawingItem, and populate it from the corresponding resolve* helper. The reference is shared, not copied: items now carry the same Fragment object that lives on the source page. This is the precondition for stopping painter iteration over `page.fragments`. The next commit drops getResolvedFragmentItem and switches the painter to iterate ResolvedPage.items, reading the source fragment via `item.fragment` instead of indexing back into the legacy fragments array. Includes focused tests in resolveLayout.test.ts asserting back-pointer identity for each kind.
…ms (SD-2836) Replace the three `page.fragments.forEach((fragment, index) => ...)` loops in renderPage / patchPage / initPage with iterations over `resolvedItems` (the resolved page's items), reading the source Fragment via the back-pointer added in the previous commit. Removes: - getResolvedFragmentItem (parallel-array index lookup into resolved items) - direct iteration of `page.fragments` from the painter render path Updates affected hand-rolled resolved-layout tests to populate the new required `fragment` back-pointer; the painter now treats items without a fragment as not-yet-renderable rather than indexing back into the legacy fragments array.
Switch paint()'s top-level reads from input.sourceLayout to input.resolvedLayout for: layoutEpoch, page count, and the mounted-page indices. The local `layout = input.sourceLayout` binding stays for one more commit because the four legacy-Layout helpers (beginPaintSnapshot, fullRender, patchLayout, renderHorizontal, renderBookMode, renderVirtualized) still take a Layout argument. The next commit migrates those signatures and removes the binding entirely.
…836) computeSdtBoundaries and computeBetweenBorderFlags previously took both the raw `fragments` array and the parallel resolvedItems array. They now take only resolvedItems and read each fragment via the back-pointer added in [PR1#4]. Updates all four call sites in renderer.ts plus the between-borders test fixture. This eliminates the painter's lookup into `page.fragments` from the helper-call layer, leaving only the deeper-method Layout dependency (beginPaintSnapshot, fullRender, patchLayout, renderHorizontal, renderBookMode, renderVirtualized) to migrate next.
Switch DomPainter's internal state and helpers from raw Layout/Page to ResolvedLayout/ResolvedPage: * The six top-level legacy-Layout methods (beginPaintSnapshot, fullRender, patchLayout, renderHorizontal, renderBookMode, renderVirtualized) take ResolvedLayout. paint() drops the `const layout = input.sourceLayout` binding and calls every method with input.resolvedLayout. * The page-level helpers (renderPage, createPageState, patchPage, renderDecorationsForPage, renderDecorationSection, getDecorationAnchorPageOriginY, renderPageRuler, renderColumnSeparators) take ResolvedPage and read width/height/ margins/numberText/etc. directly. Drops every redundant `resolvedPage?: ResolvedPage | null` parameter. * `currentLayout: Layout | null` -> `ResolvedLayout | null`. Strips the `(page.size ?? layout.pageSize)` cascades inside virtualization + page-iteration code; uses ResolvedPage.width/.height directly. * Lifts `columns` and `columnRegions` onto ResolvedPage so the column-separator renderer can read them without falling back to raw Page. Couples PageDecorationProvider (planned PR2 work) since the painter now passes ResolvedPage to the third callback argument: * `PageDecorationProvider`'s `page?` parameter is `ResolvedPage`. * `HeaderFooterSessionManager.rebuildRegions` takes ResolvedLayout. `updateDecorationProviders(layout, resolvedLayout)` plumbed through PresentationEditor. * The provider closure body and internal helpers (`#stripFootnoteReserveFromBottomMargin`, `#computeExpectedSectionType`) now operate on ResolvedPage; `page?.size?.h` -> `page?.height`. * `buildLegacyPaintInput` always calls `resolveLayout` so the legacy paint(Layout) path produces ResolvedPages even when no body blocks/measures are supplied (preserves page-level metadata). Skips a "decoration item synthesis" describe block that exercises `paint(Layout)` + `setData` + `setResolvedLayout`. Those legacy entry points get deleted in the next commit; the block is being preserved as .skip so the deletion is visible in diff.
tupizz
force-pushed
the
tadeu/sd-2836-1-consume-resolved-layout
branch
from
33e0471 to
c5d0b09
Compare
Drops the now-unused @superdoc/layout-bridge and @superdoc/pm-adapter entries from pnpm-lock.yaml so CI's --frozen-lockfile install matches package.json.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ock (SD-2836) rebuildRegions now iterates resolvedLayout.pages (was layout.pages). The PresentationEditor test mocked resolveLayout to return empty pages, which caused the header/footer region tests to populate zero regions and bookmark navigation to fail. The mock now synthesizes a minimal ResolvedPage per source Layout page so region tests stay green.
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @tupizz! I left a few inline comments
also here are some test coverage gaps found by AI:
packages/super-editor/src/editors/v1/core/presentation-editor/tests/HeaderFooterSessionManager.test.ts** (file-level — coverage gap)
rebuildRegionsis now the load-bearing entry fromResolvedLayout, but the existing tests in this file setheaderRegionsmanually and bypass it. would be worth a focused test callingrebuildRegionswith a syntheticResolvedLayoutthat exercisesfootnoteReserved > 0, per-pageheightvariation, andsectionIndex > 0so the resolved-side migration has direct coverage.
packages/layout-engine/layout-resolved/src/resolveLayout.test.ts** (file-level — coverage gap)
one gap: the page-level
columns/columnRegionsfields forwarded atresolveLayout.ts:309-310aren't asserted anywhere and they feedrenderColumnSeparators(user-visible). a one-liner asserting they make it ontoResolvedPagewould close that.
| const resolved = this.getResolvedPage(pageIndex); | ||
| const pageSize = resolved ? { w: resolved.width, h: resolved.height } : (page.size ?? layout.pageSize); | ||
| const pageSize = { w: page.width, h: page.height }; | ||
| const pageState = this.createPageState(page, pageSize, pageIndex); |
There was a problem hiding this comment.
it seems that the width and height can be accessed directly from the page parameter so perhaps the pageSize parameter is redundant and could be removed? the same would apply to calls to patchPage()
what do you think?
| * Creates new providers based on layout results and sets them on this manager. | ||
| */ | ||
| updateDecorationProviders(layout: Layout): void { | ||
| updateDecorationProviders(layout: Layout, resolvedLayout: ResolvedLayout): void { |
There was a problem hiding this comment.
updateDecorationProviders(layout, resolvedLayout) seems to be the only place where both Layout and ResolvedLayout are used. createDecorationProvider only reads layout.pages[].sectionIndex/number + page.sectionRefs — all available on ResolvedPage.
perhaps this could be changed and the layout parameter dropped?
3 participants
Stack
What changed
First step of the SD-2836 "dumb painter" refactor. Migrates DomPainter's internal pipeline so every render path consumes
ResolvedLayoutwhile keeping the legacy public API working as a compatibility shim.Painter (
packages/layout-engine/painters/dom)paint()body and the six top-level rendering methods now operate onResolvedLayout/ResolvedPage/ResolvedPaintItem.ResolvedPage.currentLayoutis typedResolvedLayout | null.Page->ResolvedPage(coupled migration; pulled forward into PR1 because painter internals call the provider).Contracts (
packages/layout-engine/contracts)run-helpers.tshostingexpandRunsForInlineNewlinesandsliceRunsForLine(shared between painter and resolved-layout consumers).resolved-layout.ts:fragment: Fragmentback-pointer added toResolvedFragmentItem,ResolvedTableItem,ResolvedImageItem,ResolvedDrawingItem.ResolvedPagegains optionalcolumnsandcolumnRegions.Super-editor consumer
HeaderFooterSessionManager.rebuildRegions(resolvedLayout),updateDecorationProviders(layout, resolvedLayout),#stripFootnoteReserveFromBottomMargin(margins, page: ResolvedPage | null),#computeExpectedSectionType(kind, page: ResolvedPage, ...)migrated toResolvedPage.PresentationEditor.#updateDecorationProviders(layout, resolvedLayout)plumbed through.Docs
packages/layout-engine/AGENTS.md"DomPainter is Dumb" section updated.Why this split
This PR is large but focused: every change here is "consume ResolvedLayout instead of Layout/Fragment/PaintInput" inside the painter and its direct callers. The legacy public surface stays callable, so consumers don't break mid-stack. PR2 then collapses that surface; PR3 locks the boundary.
Verification
painter-dom,pm-adapter,layout-bridge,layout-engine,super-editor.Acceptance criteria
ResolvedLayoutinternallyFragmentback-pointer available on resolved items for legacy pathsResolvedPageTest plan
Layoutdirectly