Table-safe diff rendering + id-aware block pairing - #2990
Draft
YousefED wants to merge 5 commits into
Draft
Conversation
prosemirror-tables' tableEditing() appends a normalizing fixTables transaction whenever it sees an inconsistent table — and a rendered diff shows inconsistent tables on purpose (deleted/inserted row and column copies). Letting it run while a diff is displayed rewrote the very diff being shown, wrote the normalization back into the suggestion Y-doc through the read-only merged viewer's binding (the phantom empty cells in the concurrent-table snapshots), and could crash the merge outright (the 'Delete row vs add column' gallery scenario: fixTables fed y-prosemirror a delta Yjs can't apply). Read-only editors shouldn't self-normalize at all: block fixTables transactions while the editor isn't editable, mirroring AIExtension's ai-writing gate.
… rule Two containers with different block ids are different blocks, no matter how similar their content — pairing them diffed one block into the other in place, which rendered e.g. a full-document replacement as edits inside the first deleted block instead of a separately inserted one. Only enforced when both sides carry an id (delta attrs are op-wrapped; compare values), so id-less external content still pairs by shape. The table-dimension pairing rule (replace the table when both dimensions changed) is removed: with fixTables gated in diff mode the rendered inconsistent tables it guarded against are displayed as-is, and the rule produced no observable behavior differences in the suite. The DiffVersioning test fixture now gives its before/after docs a stable id — they model one block at two points in time, which is the only shape real versioning inputs have (snapshots preserve ids across versions).
- 'Delete a whole document': resolved note removed (the replacement block now renders as its own insert below the deleted document). - 'Merge cells': phantom-deleted-column note removed (it was the fixTables write-back, now gated). - 'Delete row vs add column': no longer crashes with the fixTables gate (verified in-browser) — knownCrash flag and note removed. - All six concurrent table scenarios share one low-severity note: table merges aren't conflict-free (pre-existing collaboration limitation, not diff-specific; a dedicated table CRDT is the follow-up).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 ignored due to path filters (2)
📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe PR filters automatic table-fixing transactions in read-only editors, matches diff blocks by stable IDs, updates diff fixtures, and consolidates feedback for concurrent table scenarios. ChangesCore behavior and collaboration scenarios
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR changes diff rendering and block pairing while reporting focused browser and unit validation; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #2989.
Fix 1: tables in diff mode
fixTablesis now blocked while the editor is read-only (afilterTransactiononfixTablesKeymeta inTableExtension, mirroring AIExtension's ai-writing gate). A rendered diff shows inconsistent tables on purpose (deleted/inserted row and column copies); letting prosemirror-tables "repair" them:tables.concurrentsnapshots were exactly these artifacts), andlib0 'Unexpected case') no longer crashes, verified in-browser; itsknownCrashflag is removed.blockMatchNodes(replace-when-both-dims-changed) is removed along with its helper; with the gate in place it guarded nothing, and removing it produced zero behavior changes in the suite.Fix 2: pair blocks by id in
blockMatchNodesContainers with different block ids never pair in place (enforced only when both sides carry ids, so id-less external content still pairs by shape; delta attrs are op-wrapped — the values are compared). Pairing different blocks diffed one into the other, which rendered e.g. a full-document replacement as edits inside the first deleted block. Now the replacement renders as its own inserted block below the deleted document — resolving the documented issue on the "Delete a whole document" scenario (note removed).
The
DiffVersioningExtensiontest fixture gives its before/after docs a stable id: they model one block at two points in time, the only shape real versioning inputs have (snapshots preserve ids).Gallery notes
The six concurrent table scenarios now share one low-severity note: concurrent structural table edits aren't conflict-free — a pre-existing collaboration limitation (not diff-specific); a dedicated table CRDT in Yjs + BlockNote is the follow-up. The resolved "Merge cells" phantom-column and "Delete a whole document" placement notes are removed.
Validation
🤖 Generated with Claude Code
Summary by CodeRabbit