fix(pagination): cache topLevelBlockElements + fix changeset language [review fixes for #434] - #436
Conversation
… changeset language - domMeasure: hoist topLevelBlockElements() outside MeasureFn closure to avoid O(N²) DOM requery (was called once per block per recompute) - changeset: rewrite from changelog-style to present-tense reference per docs standard (no 'Add an option'; use 'The option controls...') https://claude.ai/code/session_016Wodda9zsyQ9krUCZq6QxY
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the changeset documentation for the pagination 'enabled' option and optimizes the createDomMeasure function by caching top-level block elements, which improves performance by avoiding redundant DOM queries. Feedback suggests that the current implementation's reliance on index-based mapping for DOM nodes may be fragile if the document contains top-level text nodes, and recommends using a more stable identifier for better robustness.
|
|
||
| return (block) => { | ||
| const dom = topLevelBlockElements(editable)[block.path[0]]; | ||
| const dom = blocks[block.path[0]]; |
There was a problem hiding this comment.
The mapping blocks[block.path[0]] assumes that every top-level node in the Slate document is a Slate element with a corresponding DOM node found by topLevelBlockElements. If the document contains top-level text nodes or other non-element nodes, block.path[0] (the index in editor.children) will not align with the index in the filtered blocks array. While top-level text nodes are rare in many Slate configurations, using a more stable identifier (like a block ID) to find the DOM element would be more robust.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pagination/src/react/domMeasure.ts (1)
94-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider clarifying the "Re-queries on each call" phrasing.
After the caching optimization, the JSDoc comment "Re-queries on each call so it reflects edits" could be misread to suggest the returned
MeasureFnre-queries DOM per block. The blocks are now queried once percreateDomMeasureinvocation (which still reflects edits since it's called per recompute), not per block measurement.📝 Suggested clarification
* Build a {`@link` MeasureFn} that resolves the block's font + content width from * the live editable, then derives height from the number of lines pretext wraps * the block text to. Pretext — not the DOM box — owns the line count, so the * layout is line-accurate and the box's padding/margin don't perturb it. - * Re-queries on each call so it reflects edits. + * Re-queries the DOM structure once per invocation to reflect edits; the + * returned function then performs per-block measurements via cached lookups. */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pagination/src/react/domMeasure.ts` around lines 94 - 100, The JSDoc for the MeasureFn is misleading after the caching change: update the comment in domMeasure.ts to clarify that DOM re-queries happen once per createDomMeasure() invocation (which is called per recompute and thus reflects edits), not on every individual MeasureFn call per block; reference the MeasureFn type and the createDomMeasure function in the text so readers know where the caching occurs and that measurements are reused for blocks within that invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/pagination/src/react/domMeasure.ts`:
- Around line 94-100: The JSDoc for the MeasureFn is misleading after the
caching change: update the comment in domMeasure.ts to clarify that DOM
re-queries happen once per createDomMeasure() invocation (which is called per
recompute and thus reflects edits), not on every individual MeasureFn call per
block; reference the MeasureFn type and the createDomMeasure function in the
text so readers know where the caching occurs and that measurements are reused
for blocks within that invocation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bd587aa-d5d0-43ae-a65c-e766e3008bc0
📒 Files selected for processing (2)
.changeset/pagination-enabled-option.mdpackages/pagination/src/react/domMeasure.ts
2 participants
Review fixes for #434 (two
safe_autofindings from ce-review).Changes
packages/pagination/src/react/domMeasure.tscreateDomMeasurewas callingtopLevelBlockElements(editable)inside theMeasureFnclosure — once per block per recompute. For N blocks that's N querySelectorAll passes + N filter loops = O(N²) total DOM work. Hoisted the call to the outercreateDomMeasurescope so it runs once per recompute..changeset/pagination-enabled-option.mdChangeset body used changelog-style language ("Add an
enabledoption"). AGENTS.md prohibits changelog-style; docs must document current state only. Rewrote to present-tense reference.Residual findings (for #434 author)
See full review comment on #434. Key items requiring author decision:
enabled: booleanis non-optional inPaginationOptions— likely needsenabled?: boolean+ semver bump reassessmentmeasureCachenot cleared on re-enable after DOM-only changes while disabledtopLevelBlockElementsrefactor lacks unit test for the DnD-wrapper casehttps://claude.ai/code/session_016Wodda9zsyQ9krUCZq6QxY
Generated by Claude Code
Summary by CodeRabbit
Documentation
enabledoption behavior: when disabled, pagination remains inactive with no page-break overlays rendered.Performance