refactor(painter): make DomPainter measurement-clean (SD-2957)#3178
refactor(painter): make DomPainter measurement-clean (SD-2957)#3178
Conversation
…(SD-2957) Add Guard E to architecture-boundaries.test.ts. The guard fails when production source under packages/layout-engine/painters/dom/src reads DOM measurements (clientHeight, clientWidth, offsetHeight, offsetWidth, getBoundingClientRect) off rendered content. Allowlists are intentionally narrow: * ALLOWED_INTERACTION_FILES: ruler/ruler-renderer.ts (drag handle pointer mapping). New entries require reviewer sign-off. * ALLOWED_MEASUREMENT_RECEIVERS: this.mount, mount, scrollCont. Scroll container references used to detect scrollability and map pointer coords. New entries require reviewer sign-off. The guard currently fails with 4 violations in renderer.ts (pageEl page- height fallback, pageEl footer-offset fallback, elem segment-width defensive read, measureEl clone-and-measure). Subsequent commits in SD-2957 delete those four sites; the guard then passes and ratchets the invariant.
….height (SD-2957) ResolvedPage.height is non-optional in the contract (number, not number?). The painter's fallback to pageEl.clientHeight was dead by the type system and only fired if a producer skipped its own contract — which would surface as silently mismeasured footer offsets rather than an error. Replace the fallback with a direct page.height read and a fail-fast assert that mirrors the existing 'missing resolved paragraph block/measure for fragment' invariant. The unused pageEl parameter is dropped from the function signature; the sole caller is updated. Architecture-boundary Guard E now reports 3 violations (was 4).
…-2957) Tighten PageDecorationPayload.offset from optional to required and delete the painter's pageEl.clientHeight fallback. The producer (HeaderFooterSessionManager) already populates offset on every payload it returns (header path: box.offset; footer path: pageHeight - footerMargin - containerHeight; both via #computeMetrics). The optional typing was a SD-2836 transition artifact. Add a contract-shape lockdown matching the existing items-required pattern so any future regression to optional fails the type-shape suite. Architecture-boundary Guard E now reports 2 violations (was 3).
…SD-2957) LineSegment.width is required (number) by contract. The producer (measuring-dom) always emits it. Two paint-time DOM measurement fallbacks existed to rescue cases where the contract was violated: * Image segment renderer at the runSegments[0] read fell through to elem.offsetWidth when width was undefined. Replaced with a direct resolved-stage read. * Inline text segment renderer cloned the element off-screen and read measureEl.offsetWidth when segment.width was <= 0. Deleted; the painter now trusts the resolved width verbatim. If a producer emits 0 for a non-empty segment, that's a producer bug and surfaces as a layout miscalculation rather than being papered over at paint time. Architecture-boundary Guard E now reports 0 violations.
…m reads (SD-2957) The painter coalesced ~14 resolved-item field reads with the legacy fragment back-pointer (resolvedItem?.X ?? fragment.X). The resolve producer (resolveLayout) already copies these fields onto the resolved item whenever the source fragment has them, so the fragment fallback was mathematically equivalent to just resolvedItem?.X — dead code preserved from the SD-2836 transition. Sweep covers: * paragraph: continuesFromPrev, continuesOnNext, markerWidth (truthy use) * list-item content: continuesFromPrev, continuesOnNext, markerWidth (used as Math.max input — defaults to 0 when absent rather than reading fragment) * list-item wrapper frame: markerWidth (used as Math.max input — defaults to 0) * drop cap / paragraph / list marker / image: sourceAnchor * image: pmStart, pmEnd, metadata * fragment z-index: zIndex Test fixture updated: the list-item-wrapper geometry test was relying on the dead fallback (its resolved item omitted markerWidth). Populate markerWidth on both initial and updated resolved items so the test reflects the contract. Out of scope (different semantics, not the same dead-fallback shape): * block.width ?? fragment.width (image-block natural width vs fragment layout width — separate concepts). * fragment.lines ?? measure.lines.slice(...) — flagged as stretch. * fragment.scale ?? 1 — literal default with no resolved-stage equivalent.
Update painter-dom README and layout-engine AGENTS.md (CLAUDE.md is a symlink) to document the two hard invariants enforced by the architecture boundary tests: no paint-time DOM measurement (Guard E) and resolve stage as the unique source of truth (no resolvedItem?.X ?? fragment.X coalescing). Replaces aspirational 'painter is dumb' language with what's actually ratchet-protected.
![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: d5b2dc10ed
ℹ️ 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".
…(SD-2957) Add Guard F to architecture-boundaries.test.ts. The guard fails when production source under packages/layout-engine/painters/dom/src contains '?? fragment.X' or '?? (fragment as Y).X' patterns — the dead-fallback shape that the SD-2957 soft-pattern sweep targeted. Allowlist (substring match): the documented out-of-scope cases where the LHS reads from a different stage: * block.width ?? fragment.width (ImageBlock natural vs fragment layout) * block.height ?? fragment.height (same) The previous review (codex on PR #3178) caught four sites the initial sweep missed because they used the variable name 'resolvedFrag' instead of 'resolvedItem'. Without this guard, future contributors can re-introduce the pattern under any new variable name and only catch it in review. The guard fails today on those four sites; the next commit deletes them.
…rn sweep missed (SD-2957) The first sweep used variable name 'resolvedItem' / 'item' / 'resolvedZIndex' to find dead 'X ?? fragment.Y' patterns. It missed four sites in the PM attribute path that bind 'resolvedItem' to a narrower local 'resolvedFrag' and then coalesce '(fragment as ParaFragment).X': * renderer.ts:6867 — pmStart fallback * renderer.ts:6873 — pmEnd fallback * renderer.ts:6879 — continuesFromPrev fallback * renderer.ts:6885 — continuesOnNext fallback resolveLayout populates each of these on the resolved item from the source paragraph (resolveLayout.ts:286-289), making the back-pointer reads dead exactly as in the prior sweep. Replace with resolvedFrag?.X. The two truthy-check fallbacks (continuesFromPrev/continuesOnNext) collapse the 'const X = ... ; if (X)' shape to a direct 'if (resolvedFrag?.X)'. Caught by Guard F (added in the previous commit) and by codex review on PR #3178.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
LGTM
I left just a small comment about dropping a ?? 0 for a required field
…idth (SD-2957) LineSegment.width is typed 'number' (required) by contract. Inside the text-segment forEach the painter holds a non-optional LineSegment, so 'segment.width ?? 0' is dead — the producer (measuring-dom) always emits a numeric width, and the contract type system enforces it. Drop the fallback at both sites (the appendToLineGeo call and the cumulativeX advance). Behaviour identical for all healthy segments; non-finite or undefined width would surface as NaN propagation rather than being silently masked, matching the SD-2957 fail-loudly principle. Per luccas review on PR #3178.
|
Re: @luccas-harbour's comment on Done in 94a3b833c. Both Verified: Thanks for the review! |
tupizz
left a comment
tupizz
left a comment
There was a problem hiding this comment.
Updates after adresssing codex reviews:
Good catch. Both fixed in two commits:
-
b7d7f6ffc test(architecture): forbid dead fragment.X coalescing in render path— adds Guard F toarchitecture-boundaries.test.ts. The guard catches?? fragment.Xand?? (fragment as Y).Xpatterns (with a tiny allowlist for the documentedblock.width/height ?? fragment.width/heightexception, which has different semantics — image-block natural width vs fragment layout width). My initial sweep greppedresolvedItem?.X ?? fragment.Xanditem?.X ?? fragment.Xand missed the four sites because they used the variable nameresolvedFrag. Without this guard a future contributor can re-introduce the pattern under any new variable name and only catch it in review — exactly what just happened. The guard fails today on the four sites you flagged. -
852286e94 refactor(painter): delete final 4 fragment.X fallbacks the soft-pattern sweep missed— fixes the four sites (pmStart,pmEnd,continuesFromPrev,continuesOnNext) atrenderer.ts:6867/6873/6879/6885.resolveLayout.ts:286–289populates each field on the resolved item from the source paragraph, so the back-pointer reads were dead exactly as in the prior sweep. Replaced withresolvedFrag?.X; the two truthy-check fallbacks collapsed to directif (resolvedFrag?.X).
Verified after both commits:
- Architecture boundary tests: 23/23 pass (Guards A-F + 3 contract-shape lockdowns)
painter-dom: 1050/1050 pass,tsc --noEmitclean
The invariant in the docs is now true and ratchet-protected.
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.63 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.105 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.79 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.107 |
|
🎉 This PR is included in superdoc v1.30.0-next.61 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.61 |
|
🎉 This PR is included in superdoc-cli v0.9.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.32.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/mcp v0.4.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.3.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.4.0 |
3 participants
Closes SD-2957. Follow-up to SD-2836.
What
Two patterns survived SD-2836 that contradicted "the painter is dumb":
pageEl.clientHeight/elem.offsetWidth/ clone-and-measure, used as fallbacks when resolved fields were missing or zero.?? fragment.Xfallbacks —resolvedItem?.X ?? fragment.Xreads. The producer already copies these fields; the fragment fallback was equivalent toresolvedItem?.X.Both deleted.
PageDecorationPayload.offsettightened from optional to required (the producer already always sets it). New architecture-boundary Guard E ratchets the invariant — any future paint-time DOM measurement on rendered content fails CI.Commits
test(architecture): Guard E forbids paint-time DOM measurementrefactor(painter): droppageEl.clientHeightfallback forResolvedPage.heightrefactor(contracts,painter): requirePageDecorationPayload.offsetrefactor(painter): dropoffsetWidthfallbacks forLineSegment.widthrefactor(painter): delete 14 deadfragment.Xfallbacksdocs(painter): invariants in README + AGENTS/CLAUDEOut of scope
block.width ?? fragment.width(different semantics — natural vs layout width)fragment.lines ?? measure.lines.slice(...)(needsResolvedTextLineItem[]migration — separate ticket)fragment.scale ?? 1(literal default, no resolved equivalent)Verification
painter-dom; 12345/12345super-editor; 18390/18390 full repotsc --noEmitcleanTest plan
origin/main: 0 unintended changes