feat(core): de-duplicate and order extensions by key in ExtensionManager#2872
Open
nperez0111 wants to merge 1 commit into
Open
feat(core): de-duplicate and order extensions by key in ExtensionManager#2872nperez0111 wants to merge 1 commit into
nperez0111 wants to merge 1 commit into
Conversation
De-duplication: when an extension with a given key is already registered, skip registering another one with the same key (the first registration wins). This lets an extension declare a dependency on another via `blockNoteExtensions` without conflicting when that same extension is also registered directly by the user. Ordering: a sub-extension declared via `blockNoteExtensions` is a dependency of the extension that declares it, so it now runs before its parent. The dependency is recorded as the sub is resolved (before the de-duplication check), so it holds even when multiple parents declare the same sub-extension and when a parent has a higher base priority via `runsBefore`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthrough
ExtensionManager dependency-aware ordering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/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: |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/src/editor/managers/ExtensionManager/ExtensionManager.test.ts`:
- Around line 22-28: The pluginIndex helper currently returns -1 when a plugin
is missing, which can let ordering assertions pass even if registration failed.
Update pluginIndex in ExtensionManager.test.ts to fail fast by asserting that
the requested PluginKey exists in editor.prosemirrorState.plugins before
returning the index, so tests using pluginIndex(editor, subKey) cannot silently
succeed on absent plugins.
In `@packages/core/src/editor/managers/ExtensionManager/index.ts`:
- Around line 211-217: Avoid recording self-dependencies in ExtensionManager
when registering dependents: in the block that adds parentKey to
blockNoteExtensionDependents for instance.key, skip the add if parentKey equals
instance.key. Update the logic around getTiptapExtensions/dependent tracking so
a parent extension cannot create a self-edge that later reaches the dependency
sorter.
- Around line 364-378: The dependency sort in ExtensionManager’s getPriority
only applies to BlockNote extensions, so raw tiptapExtensions can still be
emitted in the wrong order. Update the ordering logic in
ExtensionManager/index.ts so the same dependency-based priority used for
wrappers/input rules also determines the final tiptapExtensions sequence,
preserving child-before-parent behavior for parent/sub-extension pairs exposed
through addExtension().
🪄 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: 6588c705-cf03-44f3-996c-bdb4a321e37b
📒 Files selected for processing (2)
packages/core/src/editor/managers/ExtensionManager/ExtensionManager.test.tspackages/core/src/editor/managers/ExtensionManager/index.ts
Comment on lines
+22
to
+28
| function pluginIndex( | ||
| editor: BlockNoteEditor<any, any, any>, | ||
| key: PluginKey, | ||
| ): number { | ||
| return editor.prosemirrorState.plugins.findIndex( | ||
| (plugin) => (plugin as any).spec?.key === key, | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail fast when the plugin is missing.
findIndex returns -1; assertions like expect(pluginIndex(editor, subKey)).toBeLessThan(...) can pass if the expected earlier plugin was never registered. Make the helper assert presence before returning.
Proposed fix
function pluginIndex(
editor: BlockNoteEditor<any, any, any>,
key: PluginKey,
): number {
- return editor.prosemirrorState.plugins.findIndex(
+ const index = editor.prosemirrorState.plugins.findIndex(
(plugin) => (plugin as any).spec?.key === key,
);
+ expect(index).not.toBe(-1);
+ return index;
}📝 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.
Suggested change
| function pluginIndex( | |
| editor: BlockNoteEditor<any, any, any>, | |
| key: PluginKey, | |
| ): number { | |
| return editor.prosemirrorState.plugins.findIndex( | |
| (plugin) => (plugin as any).spec?.key === key, | |
| ); | |
| function pluginIndex( | |
| editor: BlockNoteEditor<any, any, any>, | |
| key: PluginKey, | |
| ): number { | |
| const index = editor.prosemirrorState.plugins.findIndex( | |
| (plugin) => (plugin as any).spec?.key === key, | |
| ); | |
| expect(index).not.toBe(-1); | |
| return index; | |
| } |
🤖 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/managers/ExtensionManager/ExtensionManager.test.ts`
around lines 22 - 28, The pluginIndex helper currently returns -1 when a plugin
is missing, which can let ordering assertions pass even if registration failed.
Update pluginIndex in ExtensionManager.test.ts to fail fast by asserting that
the requested PluginKey exists in editor.prosemirrorState.plugins before
returning the index, so tests using pluginIndex(editor, subKey) cannot silently
succeed on absent plugins.
Comment on lines
+211
to
+217
| if (parentKey) { | ||
| let dependents = this.blockNoteExtensionDependents.get(instance.key); | ||
| if (!dependents) { | ||
| dependents = new Set(); | ||
| this.blockNoteExtensionDependents.set(instance.key, dependents); | ||
| } | ||
| dependents.add(parentKey); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid recording self-dependencies.
If a parent accidentally declares a sub-extension with the same key, this records instance.key -> parentKey where both keys are equal, and getTiptapExtensions later feeds a self-edge into the dependency sorter.
Proposed fix
- if (parentKey) {
+ if (parentKey && parentKey !== instance.key) {
let dependents = this.blockNoteExtensionDependents.get(instance.key);
if (!dependents) {
dependents = new Set();
this.blockNoteExtensionDependents.set(instance.key, dependents);📝 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.
Suggested change
| if (parentKey) { | |
| let dependents = this.blockNoteExtensionDependents.get(instance.key); | |
| if (!dependents) { | |
| dependents = new Set(); | |
| this.blockNoteExtensionDependents.set(instance.key, dependents); | |
| } | |
| dependents.add(parentKey); | |
| if (parentKey && parentKey !== instance.key) { | |
| let dependents = this.blockNoteExtensionDependents.get(instance.key); | |
| if (!dependents) { | |
| dependents = new Set(); | |
| this.blockNoteExtensionDependents.set(instance.key, dependents); | |
| } | |
| dependents.add(parentKey); |
🤖 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/managers/ExtensionManager/index.ts` around lines 211
- 217, Avoid recording self-dependencies in ExtensionManager when registering
dependents: in the block that adds parentKey to blockNoteExtensionDependents for
instance.key, skip the add if parentKey equals instance.key. Update the logic
around getTiptapExtensions/dependent tracking so a parent extension cannot
create a self-edge that later reaches the dependency sorter.
Comment on lines
+364
to
+378
| const getPriority = sortByDependencies( | ||
| this.extensions.map((extension) => { | ||
| // A sub-extension declared via `blockNoteExtensions` must run before the | ||
| // extension(s) that declared it, so we merge those parents into its | ||
| // `runsBefore`. | ||
| const dependents = this.blockNoteExtensionDependents.get(extension.key); | ||
| if (!dependents?.size) { | ||
| return extension; | ||
| } | ||
| return { | ||
| key: extension.key, | ||
| runsBefore: [...(extension.runsBefore ?? []), ...dependents], | ||
| }; | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/core/src/editor/managers/ExtensionManager/index.ts'
echo '--- outline ---'
ast-grep outline "$FILE" --view expanded || true
echo '--- relevant ranges ---'
sed -n '200,430p' "$FILE" | cat -nRepository: TypeCellOS/BlockNote
Length of output: 11138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/core/src/editor/managers/ExtensionManager/index.ts'
echo '--- matches ---'
rg -n "sortByDependencies|tiptapExtensions|inputRules|this\.extensions|blockNoteExtensionDependents" "$FILE"
echo '--- selected context ---'
sed -n '220,420p' "$FILE" | cat -nRepository: TypeCellOS/BlockNote
Length of output: 11249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/core/src/editor/managers/ExtensionManager/index.ts'
python3 - <<'PY'
from pathlib import Path
p = Path("packages/core/src/editor/managers/ExtensionManager/index.ts")
text = p.read_text()
for needle in ["sortByDependencies", "tiptapExtensions", "inputRules", "blockNoteExtensionDependents", "this.extensions"]:
print(f"--- {needle} ---")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-6)
end = min(len(text.splitlines()), i+10)
for j in range(start, end+1):
print(f"{j:4d}: {text.splitlines()[j-1]}")
print()
break
PYRepository: TypeCellOS/BlockNote
Length of output: 4594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/core/src/editor/managers/ExtensionManager/index.ts'
sed -n '430,500p' "$FILE" | cat -nRepository: TypeCellOS/BlockNote
Length of output: 3013
Apply dependency ordering to raw tiptapExtensions too. They’re still appended in this.extensions order, and addExtension() inserts a parent before its blockNoteExtensions child. A parent/sub-extension pair that both expose Tiptap extensions can still run parent-first even though wrappers and input rules use the dependency sort.
🤖 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/managers/ExtensionManager/index.ts` around lines 364
- 378, The dependency sort in ExtensionManager’s getPriority only applies to
BlockNote extensions, so raw tiptapExtensions can still be emitted in the wrong
order. Update the ordering logic in ExtensionManager/index.ts so the same
dependency-based priority used for wrappers/input rules also determines the
final tiptapExtensions sequence, preserving child-before-parent behavior for
parent/sub-extension pairs exposed through addExtension().
4 tasks
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
1 participant
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.
De-duplication: when an extension with a given key is already registered,
skip registering another one with the same key (the first registration
wins). This lets an extension declare a dependency on another via
blockNoteExtensionswithout conflicting when that same extension is alsoregistered directly by the user.
Ordering: a sub-extension declared via
blockNoteExtensionsis a dependencyof the extension that declares it, so it now runs before its parent. The
dependency is recorded as the sub is resolved (before the de-duplication
check), so it holds even when multiple parents declare the same sub-extension
and when a parent has a higher base priority via
runsBefore.Summary
Rationale
Changes
Impact
Testing
Screenshots/Video
Checklist
Additional Notes
Summary by CodeRabbit