perf(core): resolve block changes from changed range only - #2981
perf(core): resolve block changes from changed range only#2981nperez0111 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesTransaction change detection
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change narrows transaction processing to the affected document range and adds coverage for the related correctness fix; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Editor
participant getBlocksChangedByTransaction
participant getChangedRange
participant Document
Editor->>getBlocksChangedByTransaction: process transaction
getBlocksChangedByTransaction->>getChangedRange: compute affected range
getChangedRange-->>getBlocksChangedByTransaction: return range or null
getBlocksChangedByTransaction->>Document: map range to old document
getBlocksChangedByTransaction->>Document: collect overlapping block snapshots
Document-->>getBlocksChangedByTransaction: return changed blocks
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/diagram-block
@blocknote/mantine
@blocknote/math-block
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
getBlocksChangedByTransaction snapshotted the entire document (a nodeToBlock conversion of every block, twice) on every transaction. Since apps read getChanges() on each keystroke, typing lagged in large documents. It now diffs only the range the transaction touched. Extract a shared getChangedRange() helper that, unlike ProseMirror's changedRange(), also covers attribute-only steps (AttrStep) and mark steps, at the same O(steps) cost. PreviousBlockType now uses it too, fixing a latent bug where attribute-only changes (e.g. a heading's level) were silently missed by its ranged diff.
df2090f to
1c74355
Compare
Summary
Resolve block changes from only the range a transaction touched, instead of snapshotting the entire document on every transaction.
Rationale
getBlocksChangedByTransaction(behindeditor.getChanges()) walked the whole document and ran anodeToBlock()conversion for every block, twice, per transaction. Because apps typically readgetChanges()insideonChangeon every keystroke, this made typing lag in large documents (O(document-size) per keypress). The existing#2595perf test never subscribed toonChange, so this path was uncovered.Changes
getBlocksChangedByTransactionnow diffs only the blocks overlapping the changed range (mapped into both the old and new document) rather than snapshotting every block.getChangedRange()helper: a drop-in for ProseMirror'schangedRange()that also covers attribute-only steps (AttrStep) and mark steps, at the same O(steps) cost.PreviousBlockTypenow usesgetChangedRange(), fixing a latent bug where attribute-only changes (e.g. a heading'slevel, a numbered listindex) were silently missed by its ranged diff — breaking those CSS transitions.Impact
Behavior of
getChanges()is unchanged (all 25 snapshot tests pass as-is); only the cost model changes, from O(document-size) to O(changed-range) per transaction.PreviousBlockTypenow correctly animates attribute-only block changes it previously dropped.Testing
vp run test(full@blocknote/coresuite, 748 passing) plus lint/type-check pass. Added agetChanges()-in-onChangeperformance regression test (previously-uncovered path) and aPreviousBlockTypetest for theAttrStep(heading-level) case that fails without the fix.Screenshots/Video
N/A
Checklist
Additional Notes
Relates to #2595 — the earlier plugin optimizations closed that issue, but the
getChanges()/onChangepath (which apps hit on every keystroke) still snapshotted the whole document. This PR completes that fix and adds the regression test the original lacked.The
PreviousBlockTypechange is a correctness fix (not just perf) surfaced while investigating the same optimization; it is isolated to that file and its test.Summary by CodeRabbit
Bug Fixes
Performance
Tests