feat: decouple yjs from blocknote/core#2741
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 Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (48)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (10)
📝 WalkthroughWalkthroughThis PR refactors BlockNote's Yjs collaboration architecture by introducing a ChangesCollaboration Architecture Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
docs/content/docs/features/collaboration/comments.mdxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
docs/content/docs/features/collaboration/index.mdxOops! Something went wrong! :( ESLint: 8.57.1 Error: ESLint configuration in --config is invalid:
examples/07-collaboration/01-partykit/src/App.tsxOops! Something went wrong! :( ESLint: 8.57.1 ReferenceError: Cannot read config file: /examples/.eslintrc.js
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/mantine
@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: |
nperez0111
changed the title
| selection: { | ||
| prosemirror: { | ||
| head: number; | ||
| anchor: number; |
There was a problem hiding this comment.
this was changed to not depend on yjs positions in the API. There are still ways to get it, this was just exposed in the API for some reason
nperez0111
force-pushed
the
decouple-yjs
branch
2 times, most recently
from
e44394d to
17ab49f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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.
Inline comments:
In `@packages/core/package.json`:
- Around line 131-135: Update package.json to mark the yjs-related peer deps as
optional by adding entries for "yjs", "y-prosemirror", and "y-protocols" under
peerDependenciesMeta with "optional": true; locate the existing peerDependencies
block (keys "yjs", "y-prosemirror", "y-protocols") and add a
peerDependenciesMeta object that sets each of those package names to optional so
consumers who don't use `@blocknote/core/yjs` won't be forced to install them.
In `@packages/core/src/comments/extension.ts`:
- Around line 353-356: The current call passes a ProseMirror Selection object
into threadStore.addThreadToDocument via editor.transact((tr) => tr.selection);
change this to extract the primitive positions and pass a plain object { head,
anchor } (e.g., const sel = editor.transact(tr => tr.selection); then call
threadStore.addThreadToDocument({ threadId: thread.id, selection: { head:
sel.head, anchor: sel.anchor } }) so the payload matches ThreadStore.ts's
expected { head: number; anchor: number } shape and avoids serializing the
Selection instance.
In `@packages/core/src/editor/performance.test.ts`:
- Line 40: The test suite is permanently disabled via describe.skip for
"Performance: transaction processing scales sub-linearly (`#2595`)"; instead, gate
it behind a CI/environment flag so it runs only in dedicated perf jobs. Replace
the describe.skip usage for the suite named "Performance: transaction processing
scales sub-linearly (`#2595`)" with a conditional invocation that uses an env/CI
variable (e.g., process.env.RUN_PERF_TESTS or CI_PERF_JOB) to choose between
describe and describe.skip (or wrap the body in if (flag) { describe(...) } ),
so CI/CD can enable the suite in perf runs while keeping it skipped by default
in regular jobs.
In `@packages/core/src/extensions/PositionMapping/PositionMapping.ts`:
- Around line 14-22: The current code relies on FinalizationRegistry and
numInstances to reset the shared Mapping, which is non-deterministic and causes
mapping.maps to grow; add an explicit lifecycle API instead: implement a
dispose() method on the PositionMapping/extension instance that decrements
numInstances and, when it reaches zero, resets mapping = new Mapping() (and
prunes mapping.maps used by map()), and ensure the constructor/initalizer
increments numInstances; remove or keep FinalizationRegistry as a fallback but
do not rely on it as the primary cleanup mechanism; optionally expose a
resetMapping() or maxHistory config to proactively trim mapping.maps during
map() calls.
In `@packages/core/src/yjs/extensions/FixupCreateAndFill.ts`:
- Around line 19-22: The code assumes jsonNode.content[0].content[0].attrs
exists before setting attrs.id and can throw; before mutating, add a guarded
path check on jsonNode (check jsonNode.content is array,
jsonNode.content[0].content is array, jsonNode.content[0].content[0].attrs is an
object) and only set jsonNode.content[0].content[0].attrs.id = "initialBlockId"
when those checks pass; if missing, either create the missing objects (content
arrays and attrs object) so Node.fromJSON(editor.pmSchema, jsonNode) receives a
consistent shape, or skip setting id but still proceed to assign cache from
Node.fromJSON using the safe jsonNode.
- Around line 13-16: The current cache used in the patched createAndFill always
returns the first created node regardless of arguments; change the caching logic
inside the wrapper around oldCreateAndFill so the cached value is only returned
when the wrapper is invoked with no arguments (e.g., args.length === 0 or all
args are undefined) and otherwise call oldCreateAndFill with the provided args
to produce a fresh node; update the code around oldCreateAndFill, the cache
variable, and the return path that currently does "if (cache) return cache" so
it first checks that args are the default/empty case before returning the cached
node.
In `@packages/core/src/yjs/extensions/index.ts`:
- Around line 72-83: The return currently unconditionally overwrites
initialContent with a deterministic paragraph; change it to preserve
caller-supplied content by only injecting the deterministic block when none was
provided (use options.initialContent if present, otherwise fallback to [{ type:
"paragraph", id: "initialBlockId" }]); update the object returned by
withCollaboration (the function building the extensions config) so
initialContent is set to options.initialContent ?? [{ type: "paragraph", id:
"initialBlockId" }], leaving everything else (extensions, disableExtensions)
unchanged.
In `@packages/core/src/yjs/extensions/RelativePositionMapping.test.ts`:
- Around line 80-82: The first test unmounts localEditor and remoteEditor but
fails to destroy the Yjs documents, leaving observers alive; update the test
teardown to call ydoc.destroy() and remoteYdoc.destroy() (or move both unmount
and destruction into the existing afterEach) so that the variables ydoc and
remoteYdoc used in RelativePositionMapping.test.ts are explicitly destroyed
after the test completes.
- Around line 337-339: The test "from a remote transaction" is incorrectly
performing the edit via localEditor, so it doesn't exercise remote-origin
updates; change the insertion to be performed by the remote side (use
remoteEditor._tiptapEditor.commands.insertContentAt or the remoteEditor API used
elsewhere in this test suite) instead of localEditor, ensuring the test inserts
"Test " at position 3 through remoteEditor so the local position mapping is
updated by a remote transaction.
In `@packages/core/src/yjs/extensions/RelativePositionMapping.ts`:
- Around line 27-35: The pre-sync fallback currently returns undefined when the
"positionMapping" extension is disabled, producing an invalid getter for callers
of trackPosition; update the fallback in RelativePositionMapping (the branch
that checks ySyncPluginState.binding.type.length === 0) to detect if
editor.getExtension("positionMapping") is missing and either throw a clear Error
(e.g. "positionMapping extension is disabled; cannot map position before sync")
or return a valid getter function (() => number) that maps to a safe position
(for example the document end via editor.state.doc.content.size); modify the
code around PositionMappingExtension.mapPosition so callers of
mapPosition/trackPosition always receive a function or an explicit thrown error.
In `@packages/xl-ai/src/AIExtension.ts`:
- Line 10: The import of ForkYDocExtension is currently a type-only import but
is later used with typeof (e.g., in the generic parameters referencing
ForkYDocExtension), so change the line importing ForkYDocExtension to a regular
value import by removing the `type` keyword; this ensures typeof
ForkYDocExtension is a valid value reference wherever it's used in
AIExtension.ts (references to ForkYDocExtension in the generics).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cb67314-51fc-4bd5-8003-a63ad5625713
⛔ Files ignored due to path filters (5)
packages/core/src/yjs/extensions/__snapshots__/fork-yjs-snap-editor-forked.jsonis excluded by!**/__snapshots__/**packages/core/src/yjs/extensions/__snapshots__/fork-yjs-snap-editor.jsonis excluded by!**/__snapshots__/**packages/core/src/yjs/extensions/__snapshots__/fork-yjs-snap-forked.htmlis excluded by!**/__snapshots__/**packages/core/src/yjs/extensions/__snapshots__/fork-yjs-snap.htmlis excluded by!**/__snapshots__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
docs/content/docs/features/collaboration/comments.mdxdocs/content/docs/features/collaboration/index.mdxexamples/07-collaboration/01-partykit/src/App.tsxexamples/07-collaboration/03-y-sweet/src/App.tsxexamples/07-collaboration/05-comments/src/App.tsxexamples/07-collaboration/06-comments-with-sidebar/src/App.tsxexamples/07-collaboration/07-ghost-writer/src/App.tsxexamples/07-collaboration/08-forking/src/App.tsxexamples/07-collaboration/09-comments-testing/src/App.tsxpackages/core/package.jsonpackages/core/src/api/positionMapping.test.tspackages/core/src/api/positionMapping.tspackages/core/src/comments/extension.tspackages/core/src/comments/index.tspackages/core/src/comments/threadstore/ThreadStore.tspackages/core/src/editor/BlockNoteEditor.test.tspackages/core/src/editor/BlockNoteEditor.tspackages/core/src/editor/managers/ExtensionManager/extensions.tspackages/core/src/editor/managers/StateManager.tspackages/core/src/editor/performance.test.tspackages/core/src/extensions/PositionMapping/PositionMapping.tspackages/core/src/extensions/index.tspackages/core/src/extensions/tiptap-extensions/UniqueID/UniqueID.tspackages/core/src/extensions/tiptap-extensions/index.tspackages/core/src/yjs/README.mdpackages/core/src/yjs/comments/RESTYjsThreadStore.tspackages/core/src/yjs/comments/YjsThreadStore.test.tspackages/core/src/yjs/comments/YjsThreadStore.tspackages/core/src/yjs/comments/YjsThreadStoreBase.tspackages/core/src/yjs/comments/index.tspackages/core/src/yjs/comments/yjsHelpers.tspackages/core/src/yjs/extensions/FixupCreateAndFill.tspackages/core/src/yjs/extensions/ForkYDoc.test.tspackages/core/src/yjs/extensions/ForkYDoc.tspackages/core/src/yjs/extensions/RelativePositionMapping.test.tspackages/core/src/yjs/extensions/RelativePositionMapping.tspackages/core/src/yjs/extensions/YCursorPlugin.tspackages/core/src/yjs/extensions/YSync.tspackages/core/src/yjs/extensions/YUndo.tspackages/core/src/yjs/extensions/index.tspackages/core/src/yjs/extensions/schemaMigration/SchemaMigration.tspackages/core/src/yjs/extensions/schemaMigration/migrationRules/index.tspackages/core/src/yjs/extensions/schemaMigration/migrationRules/migrationRule.tspackages/core/src/yjs/extensions/schemaMigration/migrationRules/moveColorAttributes.test.tspackages/core/src/yjs/extensions/schemaMigration/migrationRules/moveColorAttributes.tspackages/core/src/yjs/index.tspackages/xl-ai/package.jsonpackages/xl-ai/src/AIExtension.tspackages/xl-ai/src/plugins/AgentCursorPlugin.ts
💤 Files with no reviewable changes (1)
- packages/core/src/comments/index.ts
| } | ||
|
|
||
| describe("Performance: transaction processing scales sub-linearly (#2595)", () => { | ||
| describe.skip("Performance: transaction processing scales sub-linearly (#2595)", () => { |
There was a problem hiding this comment.
Avoid permanently disabling the regression suite.
Line 40 hard-disables all #2595 regression checks, removing CI guardrails for known performance regressions. Prefer conditional gating so the suite can run in dedicated perf jobs.
Proposed change
-describe.skip("Performance: transaction processing scales sub-linearly (`#2595`)", () => {
+const describePerf =
+ process.env.BN_RUN_PERF_TESTS === "1" ? describe : describe.skip;
+
+describePerf("Performance: transaction processing scales sub-linearly (`#2595`)", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe.skip("Performance: transaction processing scales sub-linearly (#2595)", () => { | |
| const describePerf = | |
| process.env.BN_RUN_PERF_TESTS === "1" ? describe : describe.skip; | |
| describePerf("Performance: transaction processing scales sub-linearly (`#2595`)", () => { |
🤖 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/core/src/editor/performance.test.ts` at line 40, The test suite is
permanently disabled via describe.skip for "Performance: transaction processing
scales sub-linearly (`#2595`)"; instead, gate it behind a CI/environment flag so
it runs only in dedicated perf jobs. Replace the describe.skip usage for the
suite named "Performance: transaction processing scales sub-linearly (`#2595`)"
with a conditional invocation that uses an env/CI variable (e.g.,
process.env.RUN_PERF_TESTS or CI_PERF_JOB) to choose between describe and
describe.skip (or wrap the body in if (flag) { describe(...) } ), so CI/CD can
enable the suite in perf runs while keeping it skipped by default in regular
jobs.
| if (cache) { | ||
| return cache; | ||
| } | ||
| const ret = oldCreateAndFill.apply(editor.pmSchema.nodes.doc, args)!; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether createAndFill is called with non-empty args in this repo.
rg -nP '\bcreateAndFill\s*\(' -C2Repository: TypeCellOS/BlockNote
Length of output: 2267
🏁 Script executed:
cat packages/core/src/yjs/extensions/FixupCreateAndFill.tsRepository: TypeCellOS/BlockNote
Length of output: 1503
Scope the cache to prevent returning it for non-initialization calls.
The cache ignores args after the first call and always returns the same node. If createAndFill is invoked later with different arguments, it will incorrectly return the initial cached node instead of creating a new one with the expected structure.
Scope the cache to only apply when no arguments are passed (the default initialization case):
Proposed fix
- editor.pmSchema.nodes.doc.createAndFill = ((...args: any) => {
- if (cache) {
+ editor.pmSchema.nodes.doc.createAndFill = ((...args: any) => {
+ const isDefaultInitCall = args.length === 0;
+ if (cache && isDefaultInitCall) {
return cache;
}
const ret = oldCreateAndFill.apply(editor.pmSchema.nodes.doc, args)!;
+ if (!isDefaultInitCall) {
+ return ret;
+ }🤖 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/core/src/yjs/extensions/FixupCreateAndFill.ts` around lines 13 - 16,
The current cache used in the patched createAndFill always returns the first
created node regardless of arguments; change the caching logic inside the
wrapper around oldCreateAndFill so the cached value is only returned when the
wrapper is invoked with no arguments (e.g., args.length === 0 or all args are
undefined) and otherwise call oldCreateAndFill with the provided args to produce
a fresh node; update the code around oldCreateAndFill, the cache variable, and
the return path that currently does "if (cache) return cache" so it first checks
that args are the default/empty case before returning the cached node.
| return { | ||
| ...options, | ||
| extensions: [ | ||
| ...(options.extensions ?? []), | ||
| CollaborationExtension(options.collaboration), | ||
| ], | ||
| // We disable the default prosemirror history plugin, since it's not compatible with yjs | ||
| disableExtensions: ["history", ...(options.disableExtensions ?? [])], | ||
| // We don't want the default initial content, since it will generate a random id for the initial block on each client, | ||
| // leading to conflicts when syncing happens afterwards. | ||
| initialContent: [{ type: "paragraph", id: "initialBlockId" }], | ||
| }; |
There was a problem hiding this comment.
Don't overwrite caller-supplied initialContent.
withCollaboration() now discards any explicit seed content and always replaces it with a single "initialBlockId" paragraph. That makes migration lossy for callers that bootstrap a brand-new shared doc with real initial content. Preserve the caller's value and only inject the deterministic block when initialContent is absent.
Proposed fix
return {
...options,
extensions: [
...(options.extensions ?? []),
CollaborationExtension(options.collaboration),
],
// We disable the default prosemirror history plugin, since it's not compatible with yjs
disableExtensions: ["history", ...(options.disableExtensions ?? [])],
// We don't want the default initial content, since it will generate a random id for the initial block on each client,
// leading to conflicts when syncing happens afterwards.
- initialContent: [{ type: "paragraph", id: "initialBlockId" }],
+ initialContent:
+ options.initialContent ??
+ [{ type: "paragraph", id: "initialBlockId" }],
};
}🤖 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/core/src/yjs/extensions/index.ts` around lines 72 - 83, The return
currently unconditionally overwrites initialContent with a deterministic
paragraph; change it to preserve caller-supplied content by only injecting the
deterministic block when none was provided (use options.initialContent if
present, otherwise fallback to [{ type: "paragraph", id: "initialBlockId" }]);
update the object returned by withCollaboration (the function building the
extensions config) so initialContent is set to options.initialContent ?? [{
type: "paragraph", id: "initialBlockId" }], leaving everything else (extensions,
disableExtensions) unchanged.
| ShowSelectionExtension, | ||
| } from "@blocknote/core/extensions"; | ||
| import { ShowSelectionExtension } from "@blocknote/core/extensions"; | ||
| import type { ForkYDocExtension } from "@blocknote/core/yjs"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration form of ForkYDocExtension and getExtension generic expectations
rg -n -C3 '\bForkYDocExtension\b' packages/core/src packages/xl-ai/src
echo "----"
rg -n -C3 '\bgetExtension\s*<' packages/core/src packages/xl-ai/srcRepository: TypeCellOS/BlockNote
Length of output: 14815
Change import type to a regular import; typeof cannot reference type-only imports.
ForkYDocExtension is imported with import type on line 10, but typeof ForkYDocExtension in the generic parameters on lines 222, 242, and 385 requires a value reference. Remove the type keyword so the import becomes a regular value import that can be used with typeof.
🤖 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/xl-ai/src/AIExtension.ts` at line 10, The import of
ForkYDocExtension is currently a type-only import but is later used with typeof
(e.g., in the generic parameters referencing ForkYDocExtension), so change the
line importing ForkYDocExtension to a regular value import by removing the
`type` keyword; this ensures typeof ForkYDocExtension is a valid value reference
wherever it's used in AIExtension.ts (references to ForkYDocExtension in the
generics).
1 participant
Summary
In preparation of the upcoming Y.js 14. Which will introduce breaking changes on the API, type, package, and possibly document level.
I'm doing what we probably should have done from the start & de-coupling BlockNote from Y.js completely.
This means that when importing from
@blocknote/core, we will no longer depend on anything inyjsory-prosemirror.Before this, certain functionality would import
yjsory-prosemirrorat runtime even if the editor didn't actually need it's methods.Now, to make your editor collaborative, you essentially just need to wrap your existing editor options with
withCollaboration, for the editor to become collaborative.This will automatically configure your editor to be collaborative, installing all of the necessary extensions to make the feature work.
Rationale
I'm fairly certain that we are going to need to support both Y.js 13 & Y.js 14 for some period of time. Since it will require document migrations at the discretion of the integrator.
So, it is better to get ahead of this & completely de-couple the editor from
yjs&y-prosemirror.I'm hoping for this to land pretty soon, before we are done with the other
y/prosemirrorchanges. Since it will reduce drift in the PR as well as reduce bundle size for existing usersChanges
This PR removes the tight coupling between Yjs and
@blocknote/coreby moving all Yjs-related code into a dedicated@blocknote/core/yjssubpath export.Breaking Changes
collaborationis no longer a direct editor option. Instead, use the newwithCollaboration()helper to wrap your editor options:Yjs-related imports have moved:
YjsThreadStore@blocknote/core/comments@blocknote/core/yjsRESTYjsThreadStore@blocknote/core/comments@blocknote/core/yjsYjsThreadStoreBase@blocknote/core/comments@blocknote/core/yjsForkYDocExtension@blocknote/core/extensions@blocknote/core/yjsYSyncExtension,YUndoExtension,YCursorExtension,SchemaMigration@blocknote/core/extensions@blocknote/core/yjsNon-Yjs exports (
CommentsExtension,DefaultThreadStoreAuth,TiptapThreadStore, etc.) remain in@blocknote/core/comments.Impact
Now,
@blocknote/coredoes not depend onyjsory-prosemirrorunless using the new@blocknote/core/yjs. Reducing bundle size & separating concerns better.Additional Notes
We should follow this up with some changes to the
@blocknote/xl-ai&@blocknote/server-editorpackages to remove dependency on theyjspackageCloses #2759
Summary by CodeRabbit
New Features
withCollaborationhelper for streamlined editor configuration with real-time collaboration.@blocknote/core/yjs.Bug Fixes & Improvements
Documentation
withCollaborationhelper pattern.