Codex/pagination enabled option - #437
Conversation
…lay) (#357) * feat(pagination): scaffold @platejs/pagination variant A (render-overlay) — refs #353 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(template): add OpenNext deploy + pagination toolbar button placeholder Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(pagination): apply inspector + CR feedback — refs #357 - move base bundle (header, footer, page break) to BasePaginationPlugin.plugins (Slate base) - BasePaginationConfig key uses typeof KEYS.pagination - drop degenerate <_V> generic on BasePaginationOptions - add includeFootnoteSubPlugins option (default true) — opt-out for footnote coupling - usePretextMeasurer uses useState so future ready flip re-renders - drop @chenglou/pretext from dependencies until measurer lands Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(template): CR feedback — refs #365 - lint script: add explicit `eslint .` target - lint:fix: chain `eslint . --fix` after biome - pagination-toolbar-button: drop internal #357/#358 PR refs from user-facing toast Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(pagination): implement variant A — paginate, measurer, overlay, footnotes — refs #353 #357 Replace TODO stubs with full variant A: - paginate(): bin-pack with manual page-break, oversized-block, header/footer/footnote-def exclusion - allocateFootnotes(): per-page allocation by reference walk - DOM-backed measurer with bounded LRU cache keyed by (nodeId, marksFingerprint, font, width) - usePageLayout, PageOverlay (afterEditable, pointer-events: none), PageFrame chrome - FootnotePortal hides in-flow definitions via injected CSS - Editor API: getPages, getPageOf, getFootnotes - Editor transforms: insertPageBreak, setHeader, setFooter - 9 unit tests passing Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(template): wire real @platejs/pagination via vendored dist — refs #357 #365 Drop placeholder pagination kit. Vendor @platejs/pagination's built dist under templates/plate-playground-template/vendor/platejs-pagination so the template can consume the real plugin without npm publish: - pagination-kit: PaginationPlugin.configure({ A4, 96px margins, 48 header/footer, 96 footnote well }) - pagination-toolbar-button: clicks editor.tf.pagination.insertPageBreak() - vendor/.gitignore overrides repo **/dist so committed artifacts ship Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(pagination): visible page panel + resilient toolbar — refs #357 #365 PageOverlay: replace overlay-on-editor (z-index conflict, white-on-white) with a fixed top-right card showing "Pages: N" plus PageFrame thumbnails. Always visible regardless of editor theme. Toolbar button: try editor.tf.pagination.insertPageBreak() first; fall back to editor.tf.insertNodes({type:'pageBreak'}) so the action works even when the plugin's transforms aren't bound (e.g. during HMR or kit ordering). Drop the "plugin not loaded" toast. Refresh vendored dist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(pagination): KEYS resolution + reactive overlay + toggle preview — refs #353 #357 #365 Two bugs uncovered via dogfooding the deploy: 1. Plugin silently dropped from editor: source referenced KEYS.pagination and KEYS.pageBreak; both undefined in published platejs@53.0.3 → key resolved to undefined → editor filtered the plugin out. Fixed by replacing with local PAGINATION_KEY / PAGE_BREAK_KEY / etc. constants in lib/internal/keys.ts. 2. Overlay never re-rendered on edits: useEditorRef + useMemo([editor.children]) misses Slate in-place mutations. Switched to useEditorValue + usePluginOption. Also: - previewVisible option + togglePreview() transform; toolbar button now toggles the side panel instead of inserting a break. - PageFrame now renders mini-content per block (h1-h6 / blockquote / code / paragraph) so the preview is content-aware, not just empty page chrome. - Refresh vendored dist + .gitignore dogfood-output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(pagination): page config + header/footer toggles + settings dropdown — refs #357 Adds the next layer per the CR plan + agreed roadmap: Plugin (lib): - PageSize: union of preset key | { width, height } literal — registry escape hatch - BasePaginationOptions: + headerVisible, footerVisible (defaults false) - BasePaginationTransforms: + setPageSize, setMargins, toggleHeader, toggleFooter - toggleHeader/toggleFooter inserts a top-level header/footer block at index 0 / N with placeholder text ("Header" / "Footer") and flips the visibility option; removeByType dedupes if normalization produced duplicates - resolvePageSize handles both preset and literal forms; resolvePageRect clamps contentHeight/contentWidth to >= 0 Overlay (react): - usePluginOption subscribes to pageSize, margins, headerVisible, footerVisible so the panel re-renders when any of them changes via toggle/setMargins/etc. Template: - pagination-toolbar-button: replaces the single click handler with a Radix DropdownMenu — sections for Display (preview/header/footer toggles), Page size (A4/Letter/Legal radio), Margins (narrow/default/wide presets) - vendor:pagination script — copies packages/pagination/dist into the template's vendored package; addresses CR's "Vendored dist refresh" follow-up Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(pagination): apply CR/Gemini review — derived header/footer, normalizeNode, content cache, pt scaling — refs #357 Implements the agreed subset of the CR plan-conejo (comment 11) and the Gemini + CR review feedback: Architecture: - Move page-state.ts from react/internal/ to lib/internal/ so the base Slate plugin's API can read pages without depending on React. Keeps imports flowing lib → react, never the other way. Header/footer model: - Drop headerVisible / footerVisible options. Derive presence from the doc itself (editor.children.some(n => n.type === HEADER_KEY)). Removes the option-vs-Slate-history asymmetry CR risk-flagged: undo of a toggleHeader call now correctly restores the header without leaving a stale option flag. - Add api.pagination.hasHeader / hasFooter for consumers that don't want to walk children themselves. - Toolbar dropdown checkboxes derive state via useEditorValue + same scan. Invariants: - Add overrideEditor → normalizeNode that runs at root path: enforceHeaderFooterInvariants drops duplicate header/footer blocks and re-positions the survivor (header at [0], footer at last index). Defends against paste/undo producing duplicates. Measurement correctness: - MeasureCacheKey gains a contentHash field; usePretextMeasurer hashes type + plain text per node and feeds it in. Cache now invalidates on in-place edits (Gemini + CR concern). - collectPlainText stops inserting a space between adjacent leaves — bold-then-plain runs ("He" + "llo") were measured as "He llo", over- counting line breaks. Fixed. - scaleFont uses /(\d+)(px|pt)/ and re-emits the matched unit so pt-sized fonts scale correctly for headings. - resolvePageRect clamps contentWidth/Height to ≥ 0 (already in place, preserved when accepting the new PageSize union). UI: - PageOverlay's THUMB_SCALE is now computed via computeThumbScale(width) = min(0.18, 196/pageWidth) — scales custom landscape page sizes down to the panel width. - React keys for the page list use `page-${pageIndex}` (stable). Cleanup: - marks-fingerprint: rename `sorted` → `segments` (CR nitpick — name reflected traversal order, not sort). Tests: 11 pass (added contentHash + hashString cases). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 📝 CodeRabbit Chat: Implement requested code changes * Update packages/pagination/src/static/page-break-element-static.tsx Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> * refactor(pagination): split helpers into queries/transforms; KEYS via getType; plugin spec Apply footnote-plugin best practices to BasePaginationPlugin: - Extract queries (lib/queries) and transforms (lib/transforms) modules. - Move BasePaginationConfig/Api/Transforms types to lib/types. - Use editor.getType(KEYS.x) so consumer .configure({ node: { type } }) overrides flow through. - Fix enforceHeaderFooterInvariants stale-index bug (collect → sort desc → remove → re-scan → move). - Add base-pagination-plugins.spec covering plugin schema, API/transforms surface, toggle/set/insert behavior, and normalize invariants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Replace packages/pagination with the version from origin/pagination. Brings BasePaginationPlugin, PaginationCoordinator, PageElement, YjsIntegration, leaderElection, reflowEngine, runtime, registry, and example_visualization_with_toggle. Amp-Thread-ID: https://ampcode.com/threads/T-019e2aaa-8068-77e8-8c96-7aed37adf1fe Co-authored-by: Amp <amp@ampcode.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Type editor as SlateEditor; route through editor.api / editor.tf to satisfy platejs typings without slate Editor casts. Use TextApi.isText and ElementApi.isElement. Narrow Operation via 'path' in op / 'newPath' in op. Drop dynamic getType?/getOption?/hasEditableTarget accesses.
Extract PaginationPlugin into its own file so src/index.ts is purely the brl-generated barrel. Move example_visualization_with_toggle/ under internal/ so brl skips it; T8 removes it entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move YjsIntegration into src/yjs/ subfolder so the main barrel no longer transitively depends on @platejs/yjs. Mark @platejs/yjs peer as optional. Extend tsdown + brl tooling to discover the yjs subpath alongside existing react/static lanes.
drop dead example dir + drop tsconfig exclude paths for it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ug logs - runtime.notify: queueMicrotask coalesces multiple markDirty in same tick - internal/scheduleIdle: SSR-safe ric+setTimeout fallback, drops window-as-any - ReflowOptions.debug (default false) gates splitOversizedBlock console.error - splitOversizedBlock: bag last two params for useMaxParams compliance - tests: async-aware notify expectations + new microtask coalescing test
Supply-chain defense: packages must be 7+ days old before install. Adds [test] root="./packages" in monorepos. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PaginationPlugin now mounts PaginationRegistryProvider (aboveEditable) and the reflow PaginationCoordinator (afterEditable) itself, so registering the plugin is enough to render + reflow pages. Also repair the grafted package: fix PageElement composed-ref import (@udecode/react-utils), stop the coordinator spec's global module mock from stripping exports other specs need, drop vitest/stale assertions, lint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Use literal 'pagination' key, not KEYS.pagination (unreleased in published @platejs/utils → undefined key → content never wrapped into pages). - Mount registry provider + reflow coordinator in one shared aboveEditable subtree (PaginationAboveEditable) so the coordinator reads the pages that PageElement registers; the split provider/coordinator gave separate registries. - Render the page number in each page's bottom margin (paginated mode). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Foundation of the premirror-inspired pagination rewrite (derived layout; the document model never changes). Pure, DOM-free, deterministic: - layout/types.ts: layout contract (PageSpec/margins/policies, Unmeasured + Measured snapshots, LayoutOutput = pages → frames → BlockFragments + breakReason). - layout/compose.ts: composeLayout — block-level page fill with widow/orphan, keep-with-next, manual breaks, splittable-block fragmenting, oversized overflow. - layout/snapshot.ts: buildSnapshot — Slate value → flat block snapshot with stable content-based ids + atomic/keepWithNext/breakBefore hints. 16 tests, typecheck + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
measure/measure.ts: measureSnapshot turns an UnmeasuredSnapshot into a MeasuredSnapshot. The DOM read is injected (MeasureFn) so the assembly + cache layer is pure/unit-tested; caching is keyed by stable content id + content width (measure-once, reuse unchanged blocks). 6 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
react/domMeasure.ts: the MeasureFn that reads a top-level block's rendered height (incl. vertical margins) + computed line height from the live editor DOM via ReactEditor.toDOMNode. The only DOM touch in the measurement path; feeds measureSnapshot. Resilient fallbacks for line height. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
P4 of the rewrite: render derived pages without mutating the document. - react/geometry.ts: getPageGeometry / getBlockPlacements (pure, tested) — stack pages + map blocks to page-frame positions. - react/domMeasure.ts: pure-DOM MeasureFn (top-level [data-slate-node=element] children); no slate-react dependency. - react/alignContent.ts: page-start CSS spacers align a single continuous Editable's content to page frames (no model change). - react/index.ts: clean @platejs/pagination/react entry re-exporting the slate-react-free pipeline (snapshot/measure/compose + geometry/measure/align). - apps/www dev/pagination2: demo (white A4 chrome + Editable overlay + page numbers). Verified live in agent-browser via the playground template: 4 A4 pages, content flows across page boxes with clean boundaries + page numbers. 149 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The data layer both P0s (split-block rendering + caret/selection mapping) require — locate where a block/line lands and project it to screen coords. - layout/mapping.ts: buildMappingIndex — fragmentsOfBlock / pageOfBlock / pageOfBlockLine / fragmentOfBlockLine / isSplit (block→page/fragment). - layout/projection.ts: fragmentRects (absolute stack rects per fragment of a split block) + blockLinePosition (caret line → absolute stack position). Pure, deterministic. 8 tests; 24 layout tests green; typecheck + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A block taller than a page now renders correctly across page boxes without mutating the document (approach #1). - react/splitClones.ts: computeSplitPlan (pure, layout-level) + renderSplitClones (DOM). One live Editable stays editable, clipped to the slice that fits its page; each later page gets a read-only clipped clone of the next slice. Slicing is real-pixel (live block's measured top/height + page geometry) with line-boundary snapping via Range.getClientRects() — seamless live→clone and clone→clone junctions (no overlap/gap/half-line). - react/index.ts: export mapping, projection, splitClones. Verified live (agent-browser): a block ~7× page height splits cleanly across pages. 161 tests; typecheck + lint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… derived projection Scorch the mutator path: reflowEngine, page-node wrapping (BasePaginationPlugin), coordinator, registry, leader election, yjs bridge + the dead clone renderer (splitClones, option C). Package is now a self-contained pure layout pipeline (snapshot/measure/compose/geometry/align). Drop ./yjs export + dead deps. Neutralize template pagination usage (waiver). Add pretext gate to AGENTS.md. Plan: docs/plans/2026-05-22-pagination-rewrite-v2.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
F-006: cache was keyed by block id alone, so the same block measured at two
widths overwrote one slot — alternating widths (resize, side-by-side) defeated
the cache. Key by `${id}@${widthPx}`; drop the redundant inner key field.
TDD: alternating-width test was RED (3 measures), now GREEN (2).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add @chenglou/pretext + measure/pretext.ts → measureTextLines(text, font, widthPx, lineHeightPx): wrapped lines with text, width, and segment/grapheme cursor range. Real word/grapheme/CJK-aware breaking — the seed for line-level pagination + caret mapping (F-003). pretext measures via canvas; tests inject a deterministic monospace stub so line breaking is exact and headless. Barrels regenerated (pretext exported from '.'); fix scratch demo to import pure pipeline from '.' and DOM helpers from '/react' per the convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UnmeasuredBlock gains a required `text` field (concatenated leaf text), so the measure pass can feed pretext. TDD: snapshot-text test RED→GREEN. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…OM font) Option 3: pretext is the canonical layout measurement. measureBlockHeight(text, font, widthPx, lineHeightPx) = wrapped line count * lineHeight (>=1 line). createDomMeasure resolves the editor's real font + content width from the DOM and feeds pretext, replacing offsetHeight — so layout height is line-accurate and decoupled from box padding/margin. pretext gate satisfied (layout uses it). TDD: measureBlockHeight RED→GREEN (canvas stub). DOM rewire verified at host (PR7). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove the line-splitting + widow/orphan path. Each block is one whole fragment: fits-or-next-page, oversized overflows its page. Matches the chosen render (option C, no clones). Drops linesToPlace; compose is now ~20 lines simpler. TDD: compose.spec rewritten to the place-whole contract, RED→GREEN. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… LayoutOutput audit P5: projection.ts rebuilt buildMappingIndex per call. Now composeLayout builds it once and returns it on LayoutOutput.mapping; fragmentRects/ blockLinePosition consume it. buildMappingIndex now takes PageLayout[]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pply)
lib/registry.ts: WeakMap<editor,{output,dirty,measureCache}>, lazy rebuild via
injected compute, shouldInvalidateLayout predicate (content ops yes, selection
no). Footnote registry pattern. apply-override wiring lands with the plugin.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…invalidation) createTSlatePlugin<PaginationConfig> with page/margins/policies/viewMode/atomic/ keepWithNext options (A4, continuous default); overrideEditor wraps apply to mark the layout registry dirty on content ops (not selection). React useHooks host deferred to the first render PR (needs observable output to verify). Also: salvage useful algorithms from an external DOCX paginator into the impl plan (margin-collapsing, header/footer effective-height, footnote area, token offset model); reject its DOM-clone architecture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rrels continuous.ts: getContinuousBreakYs(layout) → continuous-flow Y at each interior page boundary (cumulative page heights, N pages → N-1 break Ys). Drives the continuous view's advisory break-lines (no spacers, content stays in flow). pnpm brl: expose lib/ (registry, BasePaginationPlugin) + layout/continuous from the package barrels. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r page) pretext owns the break decision; the continuous overlay anchors its advisory rule to the boundary block's live DOM top instead of a text-only pixel sum that ignores DOM margins. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… overlay (scaffold) toPlatePlugin(BasePaginationPlugin): useHooks runs snapshot→pretext-measure→compose against the live editable when the registry is dirty (rAF-batched), stores layout in the registry, and sets a reactive breakYs option. afterEditable overlay paints dashed advisory page-break rules (pointer-events:none) at getContinuousBreakYs. SCAFFOLD: typecheck 8/8 + build 8/8, but overlay DOM positioning + recompute cadence are NOT dev-browser verified yet. Do not merge until verified in-browser. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- PaginationPlugin useHooks moved into plugin config (was .extend) so the React host's recompute/overlay actually run. - Overlay consumes getContinuousBreaks: each advisory rule anchors to the boundary block's live DOM top (no text-only pixel sum) → lands on a real block edge, never mid-paragraph. - Page N label moved to the right margin gutter (was overlapping body text); dashed rule bumped to visible slate-500. - ResizeObserver recomputes + re-anchors on width change. - Wire /dev/pagination2 demos (apps/www + template) to register the plugin; template gets the @chenglou/pretext runtime peer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-001..004) - 001: compose packs by flow height (text + DOM box spacing via flowHeightPx), so page count + breaks match real DOM flow (demo: 3→4 pages, correct for 2965px content). heightPx/lineCount stay text-only for line mapping. - 003: overlay shows 'Page N of M' + a 'Page 1 of M' marker. - 004: labels moved to the left margin gutter so they stay on-screen when a narrow viewport overflows the page width. - 002: recompute runs in a layout effect (pre-paint) instead of post-paint rAF; content→lines gap ~604ms→~308ms (residual is editor hydration). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`enabled` (default true) on BasePaginationPlugin. When false, the React layer skips layout recompute and renders no overlay; document never affected. Toggle via editor.setOption(BasePaginationPlugin,'enabled',next). TDD: base option contract (default true, configurable). Changeset: minor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Warning Review limit reached
Your plan includes 5 reviews of capacity. Refill in 5 minutes and 55 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (118)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request implements a complete rewrite of the pagination system, transitioning from a model-mutating engine to a derived projection architecture that uses @chenglou/pretext for high-fidelity text measurement. The new pipeline is deterministic and avoids document pollution. Feedback focuses on critical performance and correctness refinements, including optimizing DOM queries in the measurement pass to avoid O(N^2) complexity, hardening line-height resolution to handle non-numeric CSS values, ensuring safer line-count estimation using ceiling logic to prevent overflow, and debouncing resize observers to maintain responsiveness during active window resizing.
| return (block) => { | ||
| const dom = topLevelBlockElements(editable)[block.path[0]]; | ||
| if (!dom) return null; |
There was a problem hiding this comment.
This implementation is inefficient as it performs a DOM query (topLevelBlockElements) for every block during the measurement pass, leading to O(N^2) complexity. Querying the elements once before returning the measurement function significantly improves performance.
export function createDomMeasure(editable: HTMLElement): MeasureFn {
const elements = topLevelBlockElements(editable);
return (block) => {
const dom = elements[block.path[0]];| const lineHeight = | ||
| Number.parseFloat(getComputedStyle(el).lineHeight) || 0; |
There was a problem hiding this comment.
Number.parseFloat returns NaN if the lineHeight is "normal", which results in a resolved lineHeight of 0. This will cause incorrect positioning for break lines when lineStart > 0. Consider using a more robust resolution logic that accounts for the font size.
const style = getComputedStyle(el);
const lineHeight =
Number.parseFloat(style.lineHeight) ||
Number.parseFloat(style.fontSize) * 1.5 ||
20;
| function lineCountFrom(heightPx: number, lineHeightPx: number): number { | ||
| if (lineHeightPx <= 0) return 1; | ||
|
|
||
| return Math.max(1, Math.round(heightPx / lineHeightPx)); |
There was a problem hiding this comment.
Using Math.ceil instead of Math.round is safer for line count estimation to ensure that partial lines are accounted for and do not cause unexpected content overflow during pagination.
| return Math.max(1, Math.round(heightPx / lineHeightPx)); | |
| return Math.max(1, Math.ceil(heightPx / lineHeightPx)); |
| const observer = new ResizeObserver(() => { | ||
| invalidateLayoutRegistry(editor); | ||
| forceRecompute((n) => n + 1); | ||
| }); |
arthrod
left a comment
There was a problem hiding this comment.
Code Review Results
Scope: 604ca5d → codex/pagination-enabled-option (130 files, ~6 756 additions)
Intent: Add enabled runtime toggle to BasePaginationPlugin/PaginationPlugin; deliver the full pretext-driven pagination rewrite (pure-projection, no document mutation).
Pretext gate: ✅ satisfied — @chenglou/pretext ^0.0.6 is the measurement backend; old offsetHeight-based code deleted; height comes from prepareWithSegments + layoutWithLines line counts.
Reviewers: correctness, testing, maintainability, project-standards, performance, api-contract, reliability, adversarial, kieran-typescript, julik-frontend-races, learnings-researcher, agent-native (12/12)
P1 — High
| # | File | Issue | Reviewer(s) | Conf | Route |
|---|---|---|---|---|---|
| 1 | PaginationPlugin.tsx:151 |
useIsomorphicLayoutEffect has no dependency array — runs full snapshot→measure→compose pipeline on every render; combined with getContinuousBreaks always returning a new array reference, if setOption does shallow-equal this creates an infinite loop: forceRecompute → render → effect → setOption(newArray) → render → … |
correctness, maintainability, performance, adversarial, kieran-typescript, julik-frontend-races | 0.98 | gated_auto → downstream-resolver |
| 2 | registry.ts:54 |
ensureLayout sets dirty = false unconditionally — if compute() throws, dirty becomes false but layout is null/stale; next reader sees a clean registry and returns broken layout with no retry |
reliability | 0.92 | gated_auto → review-fixer |
| 3 | PaginationPlugin.tsx:151 |
No try/catch in useLayoutEffect body — any throw from buildSnapshot, measureSnapshot, createDomMeasure, or composeLayout unmounts the entire PaginationPlugin |
reliability | 0.95 | gated_auto → review-fixer |
| 4 | PaginationPlugin.tsx:1 |
Zero test coverage for the React component — useIsomorphicLayoutEffect wiring, overlay rendering, DOM measurements, and the enabled toggle in React context are all untested |
testing | 1.00 | manual → human |
| 5 | domMeasure.ts:1 |
createDomMeasure has no tests — CSS unit parsing, getComputedStyle extraction, clientWidth fallback; silent errors produce wrong page breaks |
testing | 0.95 | manual → human |
| 6 | alignContent.ts:1 |
alignContent (CSS spacer injection) has no tests |
testing | 0.90 | manual → human |
| 7 | PaginationPlugin.tsx:1 |
enabled: false path untested at the React layer — BasePaginationPlugin.spec.ts covers the plugin layer only |
testing | 0.85 | manual → human |
| 8 | plans/*.md |
Three plan files added to root plans/ — AGENTS.md requires plans under docs/plans/ |
project-standards | 1.00 | manual → human |
| 9 | registry.ts:25 |
measureCache not cleared when enabled → false — content edits during disabled period mark dirty=true but leave stale cached heights; on re-enable, wrong heights produce wrong break positions |
correctness, adversarial | 0.90 | manual → downstream-resolver |
P2 — Moderate
| # | File | Issue | Reviewer(s) | Conf | Route |
|---|---|---|---|---|---|
| 10 | registry.ts:35 |
measureCache is an unbounded Map with no eviction; deleted-block entries accumulate indefinitely |
maintainability, performance | 0.92 | manual → downstream-resolver |
| 11 | PaginationPlugin.tsx:162 |
No guard on content-area width ≤ 0 — misconfigured margins or SSR clientWidth=0 causes every word to wrap to its own line; wrong inflated heights are cached under ${id}@0 and persist |
reliability, adversarial | 0.82 | gated_auto → review-fixer |
| 12 | pretext.ts:34 |
No OffscreenCanvas guard — prepareWithSegments throws in Node.js / old browsers; the null-return fallback in measureSnapshot doesn't fire because the error propagates past it |
reliability | 0.75 | gated_auto → review-fixer |
| 13 | packages/pagination/src/index.ts |
Barrel exports changed (new layout/ + measure/ subpaths) — confirm pnpm brl output matches committed barrel |
project-standards | 0.85 | gated_auto → release |
| 14 | domMeasure.ts |
getComputedStyle called per-block per-render with no style caching; properties are constant per element type |
performance | 0.88 | advisory → human |
| 15 | PaginationPlugin.tsx:176 |
setOption('breaks', …) inside useLayoutEffect triggers a second render for PaginationBreakLines; each break block gets getBoundingClientRect in the render phase — paint-stall risk on large documents |
julik-frontend-races | 0.68 | advisory → release |
| 16 | compose.spec.ts |
Missing edge cases: keepWithNext on last block; overflow block + following block; multi-block keepWithNext chain |
testing, adversarial | 0.85 | manual → downstream-resolver |
| 17 | PaginationPlugin.tsx:183 |
ResizeObserver destroyed and recreated on every enabled toggle; repeated toggling accumulates create/destroy cycles |
adversarial | 0.72 | manual → downstream-resolver |
P3 — Low
| # | File | Issue | Conf | Route |
|---|---|---|---|---|
| 18 | pretext.spec.ts |
No non-ASCII, CJK, or RTL text tests; monospace stub obscures proportional-font behaviour | 0.75 | manual → downstream-resolver |
| 19 | .changeset/*.md |
One changeset uses single quotes for package name; others use double quotes | 0.65 | safe_auto → review-fixer |
| 20 | measure.ts:32 |
MeasureFn JSDoc doesn't document that returning null activates fallbackLineHeightPx |
0.60 | advisory → human |
Suggested fixes for the P1 blockers
#1 — dep array + loop guard (PaginationPlugin.tsx:151)
- useIsomorphicLayoutEffect(() => {
+ useIsomorphicLayoutEffect(() => {
if (!enabled) return;
// ... measure/compose ...
- setOption('breaks', getContinuousBreaks(layout));
+ const next = getContinuousBreaks(layout);
+ // avoid re-render when breaks are structurally identical
+ const prev = editor.getOption(PaginationPlugin, 'breaks');
+ if (JSON.stringify(next) !== JSON.stringify(prev)) {
+ setOption('breaks', next);
+ }
- });
+ }, [editor, enabled]);#2 — ensureLayout error recovery (registry.ts)
export function ensureLayout(editor, compute) {
const entry = getLayoutRegistry(editor);
if (entry.dirty || !entry.output) {
- entry.output = compute();
- entry.dirty = false;
+ try {
+ entry.output = compute();
+ entry.dirty = false;
+ } catch (err) {
+ // keep dirty=true so the next call retries
+ throw err;
+ }
}
return entry.output;
}#3 — effect error boundary (PaginationPlugin.tsx)
useIsomorphicLayoutEffect(() => {
if (!enabled) return;
+ try {
const editable = ...;
// ... full pipeline ...
setOption('breaks', ...);
+ } catch (err) {
+ console.error('[PaginationPlugin] layout pipeline failed:', err);
+ }
}, [editor, enabled]);#8 — plan file locations
git mv plans/2026-05-15-fix-pagination-plugin-v1.md docs/plans/
git mv plans/2026-05-15-fix-pagination-plugin-tdd-v2.md docs/plans/
git mv plans/2026-05-16-pagination-end-to-end-fix-v1.md docs/plans/
Pre-existing (not blocking)
| File | Note |
|---|---|
compose.ts:99 |
keepWithNext on last block is silently a no-op — undocumented but correct |
PaginationPlugin.tsx |
StrictMode runs useLayoutEffect twice in dev — deterministic by design |
Learnings & Past Solutions
No prior docs/solutions/ entries for pagination. Key known-open items from docs/plans/2026-05-23-pagination-impl-plan.md (confirmed not blocking this PR): Point↔LayoutPoint mapping (P1), dirty-range incremental invalidation (P3), line-split compose with widow/orphan (gated on mapping), print/static path.
Agent-Native
✅ Fully headless pipeline accessible (buildSnapshot → measureSnapshot → composeLayout → MappingIndex). enabled option well-documented with @default true. Gap: measureSnapshot() lacks JSDoc on null-return fallback; no README quick-start.
Coverage
- Suppressed: 4 findings below 0.60 confidence
- Residual risks: no rAF batching (known future work);
measureCachenot keyed by font metrics (zoom/theme change gives stale heights until next width change)
Verdict:
⚠️ Ready with fixesArchitecture is solid — pretext gate satisfied, no document mutation, pure composition, correct WeakMap registry,
MappingIndexembedded inLayoutOutput. Pure-layer test coverage (compose, mapping, snapshot, registry, pretext primitives) is good.Must fix (P1):
useIsomorphicLayoutEffectdep array +getContinuousBreaksmemoize (potential infinite loop)ensureLayout: preservedirty=truewhencompute()throwstry/catchinuseLayoutEffectbody- Move
plans/*.md→docs/plans/Should fix (P1–P2):
5–9. React-layer test coverage (PaginationPlugin.tsx,domMeasure.ts,alignContent.ts,enabled=falsepath)
10. Width ≤ 0 guard beforemeasureSnapshot
11.OffscreenCanvasenvironment guard inpretext.tsFix order: dep-array + loop → registry error recovery → effect try/catch → plan paths → width guard → OffscreenCanvas guard → React tests
Generated by Claude Code
Checklist
pnpm typecheckpnpm lint:fixbun testpnpm brlpnpm changeset