fix(pagination): margin-aware packing + continuous-overlay polish (dogfood fixes)

[arthrod]

Fixes the four issues from the /dogfood pass on /dev/pagination2. Stacked on #418.

ISSUE-001 (medium) — break lines drifted past the true page boundary

Compose now packs pages by a block's flow height = pretext text height + the DOM box spacing (margins/padding/borders) the measurer supplies as a new flowHeightPx (falls back to text height when absent). heightPx/lineCount stay text-only, so line-level mapping is untouched.

• Page count is now correct: the demo content (2965px) needs 4 pages (3×931 < 2965), where it previously showed 3.
• Per-line error dropped and flipped to the safe (early) direction; verified delta-to-block-top stays 0–1px.
• Residual drift is pretext text-measurement precision vs the browser (a deeper F-item), not box spacing.
• TDD: new compose.spec cases — packs by flowHeightPx when present, falls back when absent.

ISSUE-003 (low) — no Page 1 / no total

Labels now read Page N of M with a Page 1 of M marker at the top.

ISSUE-004 (low) — labels off-screen on narrow viewports

Labels moved to the left margin gutter. Verified at a 600px viewport: all labels on-screen (left:49, right:112) vs off-screen before.

ISSUE-002 (low) — ~600ms load flash

Recompute runs in an isomorphic layout effect (pre-paint) instead of post-paint requestAnimationFrame. Content→lines gap ~604ms → ~308ms; the residual is the editor's hydration time (SSR content is on screen before the client can measure the DOM — inherent to a client-measured overlay).

Verification

• @platejs/pagination build 8/8, typecheck 8/8, 49 tests pass (47 + 2 new), biome clean.
• dev-browser verified all four on localhost:3000/dev/pagination2 (default + 600px viewports); editing/selection still native (pointer-events:none); no console errors.

🤖 Generated with Claude Code

[arthrod]

@coderabbitai review very carefully and fix

[arthrod]

@gemini review very carefully and fix

[arthrod]

@Kilo review very carefully and fix

[gemini-code-assist]

Code Review

This pull request introduces margin-aware page packing by incorporating DOM box spacing (margins, padding, and borders) into layout calculations via a new flowHeightPx property. It also enhances the pagination overlay by displaying 'Page N of M' labels, adding a marker for the first page, and moving labels to the left margin gutter for better visibility on narrow viewports. Additionally, the layout recomputation was moved to a layout effect to ensure advisory lines appear simultaneously with content. Feedback was provided regarding the line-height resolution in the overlay, suggesting a more robust fallback than 0 when the computed style is 'normal' to prevent positioning errors.

[gemini-code-assist]

The current line height resolution falls back to 0 if the computed line-height is normal (which parseFloat returns as NaN). This will cause incorrect positioning of advisory lines if lineStart > 0 (e.g., in future line-split modes). It's better to fall back to a heuristic like fontSize * 1.5 or a constant, similar to the resolveLineHeight logic used in domMeasure.ts.

        const style = getComputedStyle(el);
        const lineHeight =
          Number.parseFloat(style.lineHeight) ||
          Number.parseFloat(style.fontSize) * 1.5 ||
          20;

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll start by fetching the PR details and diff to review the changes carefully.

---

🏁 Script executed:

#!/bin/bash
# Fetch PR details
gh pr view 421 --repo cicero-im/plate --json title,body,files,commits,baseRefName,headRefName

# Fetch the diff
gh pr diff 421 --repo cicero-im/plate 2>&1 | head -2000

Length of output: 20067

---

Good — I have the full diff. Let me pull more context on the key files before writing up the review.

---

🏁 Script executed:

#!/bin/bash
# Get the full compose.ts to understand PageFragment type usage
cat packages/pagination/src/layout/compose.ts

echo "===TYPES==="
cat packages/pagination/src/layout/types.ts

echo "===MEASURE==="
cat packages/pagination/src/measure/measure.ts

echo "===PAGINATION_PLUGIN==="
cat packages/pagination/src/react/PaginationPlugin.tsx

echo "===DOM_MEASURE==="
cat packages/pagination/src/react/domMeasure.ts

Length of output: 22881

---

@arthrod — here is a careful review of the seven changed files. I found one confirmed bug, one partially-unaddressed issue from your own issue list, and one low-severity concern.

---

🔴 Bug — heightPx: flow stored in BlockFragment contradicts stated intent

File: packages/pagination/src/layout/compose.ts

The PR description and the inline comment both state:

heightPx/lineCount stay text-only, so line-level mapping is unaffected.

But the fragment is pushed with:

push({
  ...
  heightPx: flow,      // ← flow = b.flowHeightPx ?? b.heightPx (includes box spacing)
  lineCount: b.lineCount,
  ...
});
currentY += flow;

currentY += flow is correct for packing (the page coordinate must advance by the full DOM flow height). But the fragment's heightPx field should stay b.heightPx (pure text height) if the documented guarantee is to hold. Any consumer that reads fragment.heightPx expecting the text-only height (e.g. paged-view block container sizing, or a future heightPx / lineHeightPx re-derivation of line counts) will silently receive the inflated value. lineCount being correct won't save it.

Suggested fix:

-    push({
-      blockId: b.id,
-      fragmentIndex: 0,
-      heightPx: flow,        // was: b.heightPx
-      lineCount: b.lineCount,
-      lineStart: 0,
-      path: b.path,
-      y: currentY,
-    });
-    currentY += flow;
+    push({
+      blockId: b.id,
+      fragmentIndex: 0,
+      heightPx: b.heightPx,  // text-only, matches documented contract
+      lineCount: b.lineCount,
+      lineStart: 0,
+      path: b.path,
+      y: currentY,
+    });
+    currentY += flow;        // advance by full DOM flow height for correct packing

---

🟡 ISSUE-003 only half-fixed — single-page documents still show no label

File: packages/pagination/src/react/PaginationPlugin.tsx — Line 78

if (!editable || breaks.length === 0) return null;

When the document fits on one page, breaks is [], the component returns null, and the "Page 1 of 1" marker that was added later in the same render is never reached. ISSUE-003 ("no Page 1 / no total") remains unresolved for single-page documents.

Suggested fix:

-  if (!editable || breaks.length === 0) return null;
+  if (!editable) return null;
+  if (breaks.length === 0 && !blocks[0]) return null; // truly empty doc

This lets the "Page 1 of 1" marker render even when there are no break lines. The {breaks.map(...)} call over an empty array is a no-op, so no extra DOM nodes appear.

---

🔵 Low — useIsomorphicLayoutEffect fires before every paint (no dependency array)

File: packages/pagination/src/react/PaginationPlugin.tsx — Line 147

useIsomorphicLayoutEffect(() => {
  const registry = getLayoutRegistry(editor);
  if (!registry.dirty && registry.output) return;
  ...
  setOption('breaks', getContinuousBreaks(layout));
});   // ← no dependency array

Without a dependency array the effect runs synchronously (blocking paint) before every render. The early-return keeps it cheap, but getLayoutRegistry(editor) is still called before every single paint. The original useEffect + requestAnimationFrame also had no deps, so this is a pre-existing pattern — but layout effects are synchronous, making the omission more impactful.

Adding [editor] as a dependency would restrict runs to the editor instance changing (mount/unmount), letting the registry.dirty guard handle content-change re-runs via the forceRecompute trigger from the ResizeObserver effect:

-  useIsomorphicLayoutEffect(() => {
+  useIsomorphicLayoutEffect(() => {  // eslint-disable-line react-hooks/exhaustive-deps
     const registry = getLayoutRegistry(editor);
     if (!registry.dirty && registry.output) return;
     ...
     setOption('breaks', getContinuousBreaks(layout));
-  });
+  }); // intentionally no dep-array: must re-run after every forceRecompute tick

(If the forceRecompute counter is the intended re-render trigger, the comment above should be added so future readers understand the omission is deliberate.)

---

✅ Everything else looks good

Area	Status
flowOf() helper and overflow / keep-with-next packing logic	Correct
verticalBoxSpacing margin-collapse over-count	Acknowledged and safe (conservative direction)
flowHeightPx only set when boxSpacingPx > 0 (clean fallback)	Correct
useIsomorphicLayoutEffect SSR guard	Standard pattern ✓
labelStyle extracted to module scope (no inline object churn)	Good refactor
topOf() helper de-duplicated from break rendering	Clean ✓
New compose.spec cases cover both branches of flowOf	Good coverage
Changeset entry present and accurate	✓

Changes needed

• Fix heightPx: flow → heightPx: b.heightPx in compose.ts
• Fix breaks.length === 0 early-return guard in PaginationPlugin.tsx to allow single-page "Page 1 of 1" label
• Add explanatory comment on the intentional missing dep-array for useIsomorphicLayoutEffect

---

The requested changes have been implemented and a pull request has been created: View PR