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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoves prosemirror-dropcursor, introduces a BlockNote DropCursorExtension and utilities with exclusion support, adds multi-column edge-detection and drop-handling, adds a drag‑and‑drop example using Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Editor as BlockNote Editor
participant DropExt as DropCursorExtension
participant MultiHandler as MultiColumnHandler
participant Utils as DropCursor Utils
participant DOM as Overlay/DOM
User->>Editor: drag item over editor
Editor->>DropExt: dragstart / dragover events
activate DropExt
DropExt->>Utils: hasExclusionClassname?
alt exclusion detected
DropExt-->>Editor: ignore (no cursor)
else visible area
DropExt->>Utils: computeDropPosition(context)
Utils-->>DropExt: position + orientation
DropExt->>DOM: create/update overlay at coords
end
deactivate DropExt
User->>Editor: drop item
Editor->>MultiHandler: handleDrop invoked (plugin)
activate MultiHandler
MultiHandler->>Utils: detectEdgePosition(event)
alt edge drop (column)
MultiHandler->>Editor: normalize/move/create columnList, apply transaction
else regular drop
MultiHandler-->>Editor: allow default drop handling
end
deactivate MultiHandler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
examples/03-ui-components/18-drag-n-drop/src/App.tsx (2)
29-34: Extract duplicated initial items to a constant.The initial items array is duplicated between the
useStatecall andhandleReset. Consider extracting to reduce duplication and ensure consistency.♻️ Proposed fix
+const INITIAL_ITEMS = [ + { id: "1", color: "#FF6B6B", label: "Red Item" }, + { id: "2", color: "#4ECDC4", label: "Teal Item" }, + { id: "3", color: "#45B7D1", label: "Blue Item" }, + { id: "4", color: "#FFA07A", label: "Orange Item" }, +]; + export default function App() { // ... - const [draggedItems, setDraggedItems] = useState([ - { id: "1", color: "#FF6B6B", label: "Red Item" }, - { id: "2", color: "#4ECDC4", label: "Teal Item" }, - { id: "3", color: "#45B7D1", label: "Blue Item" }, - { id: "4", color: "#FFA07A", label: "Orange Item" }, - ]); + const [draggedItems, setDraggedItems] = useState(INITIAL_ITEMS); // ... const handleReset = () => { - setDraggedItems([ - { id: "1", color: "#FF6B6B", label: "Red Item" }, - { id: "2", color: "#4ECDC4", label: "Teal Item" }, - { id: "3", color: "#45B7D1", label: "Blue Item" }, - { id: "4", color: "#FFA07A", label: "Orange Item" }, - ]); + setDraggedItems(INITIAL_ITEMS); setDroppedItems([]); };Also applies to: 61-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/03-ui-components/18-drag-n-drop/src/App.tsx` around lines 29 - 34, Extract the duplicated initial items array into a shared constant (e.g., INITIAL_ITEMS) and use that constant as the initial value for useState (where draggedItems is declared) and inside the handleReset function to reset state; update references to draggedItems/setDraggedItems and handleReset to use INITIAL_ITEMS so the initial items are defined once and kept consistent.
78-78: Use the exported constant instead of the string literal.The README demonstrates importing
DRAG_EXCLUSION_CLASSNAMEfrom@blocknote/core, but this example uses the literal string"bn-drag-exclude". For consistency and to demonstrate the recommended approach:♻️ Proposed fix
+import { DRAG_EXCLUSION_CLASSNAME } from "@blocknote/core"; import "@blocknote/core/fonts/inter.css";Then update line 78:
- <div className={`drag-demo-section bn-drag-exclude`}> + <div className={`drag-demo-section ${DRAG_EXCLUSION_CLASSNAME}`}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/03-ui-components/18-drag-n-drop/src/App.tsx` at line 78, Replace the hard-coded string "bn-drag-exclude" with the exported constant DRAG_EXCLUSION_CLASSNAME from `@blocknote/core`: import DRAG_EXCLUSION_CLASSNAME at the top of App.tsx (if not already imported) and use that constant in the div's className (e.g., className={`drag-demo-section ${DRAG_EXCLUSION_CLASSNAME}`}) so the example uses the exported symbol rather than a literal.packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts (1)
7-11: Consider stronger typing fornodefield.Using
anyloses type safety. Consider using ProseMirror'sNodetype for better IDE support and compile-time checks.+import type { Node } from "prosemirror-model"; + export interface EdgeDropPosition { position: "left" | "right" | "regular"; posBeforeNode: number; - node: any; + node: Node; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts` around lines 7 - 11, The EdgeDropPosition interface currently types node as any; update it to use ProseMirror's Node type for stronger typing: change EdgeDropPosition.node from any to import and use the Node type from prosemirror-model (e.g., Node) so EdgeDropPosition, and any code using multiColumnDropCursor.ts, benefits from IDE/compile-time checks; ensure you add the appropriate import for Node at the top of the file and update any related places that construct or inspect EdgeDropPosition to satisfy the stronger type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/03-ui-components/18-drag-n-drop/index.html`:
- Around line 1-14: The document is missing the HTML doctype which causes
quirks-mode rendering; add a <!DOCTYPE html> declaration as the very first line
before the <html lang="en"> tag in the file so the page renders in standards
mode and the drag-and-drop example behaves correctly.
In `@examples/03-ui-components/18-drag-n-drop/vite.config.ts`:
- Around line 14-30: The alias block in the Vite config (resolve.alias) uses
incorrect relative paths ("../../packages/...") so the
fs.existsSync(path.resolve(__dirname, "../../packages/core/src")) check always
fails and aliases for `@blocknote/core` and `@blocknote/react` never enable; update
the paths used in both the fs.existsSync check and the alias targets
(referencing resolve, conf.command, fs.existsSync, path.resolve, and the alias
keys "@blocknote/core" and "@blocknote/react") to use
"../../../packages/core/src" and "../../../packages/react/src" so they correctly
point to the repository root and the aliases activate during development.
In `@packages/core/src/editor/BlockNoteEditor.ts`:
- Around line 123-128: The change narrows the public API by replacing the
previous plugin-factory shape with DropCursorOptions on
BlockNoteEditor.dropCursor; restore compatibility by accepting both shapes:
detect if the provided dropCursor is a function (the old
plugin-factory/custom-plugin integration) and, in that case, call it and
adapt/unwrap its result into the new DropCursorOptions shape (or wrap it into
the expected plugin chain), otherwise treat it as a DropCursorOptions object;
update the BlockNoteEditor constructor/initializer where dropCursor is consumed
to perform this feature-detecting adapter so existing consumers continue to work
without code changes and add a short deprecation note on the function shape.
In `@packages/core/src/extensions/DropCursor/DropCursor.ts`:
- Around line 256-259: The abort handler currently only calls setCursor(null)
and neglects to clear any pending removal timeout; update the DropCursor
unmount/abort logic so it also clears the scheduled timeout used by
scheduleRemoval (the timeout id variable created when calling setTimeout inside
scheduleRemoval), e.g. ensure the timeout id is stored in a scoped variable
(e.g. removalTimeout or timeoutId) and call clearTimeout(timeoutId) before/when
handling signal.addEventListener("abort", ...) so the pending timeout cannot
later call setCursor(null) after cleanup.
In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts`:
- Around line 45-46: view.nodeDOM(posInfo.posBeforeNode) can return null so
casting to HTMLElement and calling getBoundingClientRect() on blockElement may
throw; update the code in multiColumnDropCursor.ts to null-check the result of
view.nodeDOM(posInfo.posBeforeNode) (e.g., check blockElement !== null) before
casting and calling getBoundingClientRect(), and handle the null case by
returning early or computing fallback geometry for blockRect so subsequent logic
using blockRect does not run on a null value.
- Around line 27-29: The current check in multiColumnDropCursor.ts throws when
view.posAtCoords() yields null (eventPos), which crashes drag/drop; change the
behavior to return null instead of throwing and update the containing function's
return type to include null (e.g., DropCursorResult | null) so callers can
handle a missing position; then update all callers of that function to
gracefully handle a null return (skip drop handling or use a fallback position)
rather than relying on an exception.
In
`@packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts`:
- Around line 89-95: The new column insertion currently constructs the column
node with props: {} which omits width and triggers NaN in the column width
normalization in multiColumnHandleDropPlugin.ts; update the insertion that calls
.toSpliced(...) to include a default width (e.g. props: { width: 1 }) so the
newly inserted column has a numeric props.width, ensuring downstream sum/average
in the normalization logic (the block that reads column.props.width) operates
correctly; locate the insertion near where UniqueID.options.generateID() is used
and add the width to the props for the new column node.
- Around line 31-34: The code unsafely calls slice.content.child(0) which throws
when the slice is empty; update the logic in multiColumnHandleDropPlugin to
guard on slice.content.childCount (or equivalent) before calling nodeToBlock and
handle the empty case (e.g., abort the drop, return early, or skip processing)
so you never call nodeToBlock with a non-existent child; specifically check
slice.content.childCount > 0, then call nodeToBlock(slice.content.child(0),
editor.pmSchema) and proceed, otherwise return/skip the drop handling.
- Around line 22-29: The call to detectEdgePosition inside handleDrop can throw
if the drop is outside editor bounds; wrap the detectEdgePosition(view, event,
view.state) call in a try-catch inside handleDrop (the function in
multiColumnHandleDropPlugin) and on error return false so ProseMirror can handle
the drop (optionally log the error via your logger). Ensure the rest of
handleDrop only runs when detectEdgePosition succeeds and keep existing behavior
of returning false for "regular" positions.
---
Nitpick comments:
In `@examples/03-ui-components/18-drag-n-drop/src/App.tsx`:
- Around line 29-34: Extract the duplicated initial items array into a shared
constant (e.g., INITIAL_ITEMS) and use that constant as the initial value for
useState (where draggedItems is declared) and inside the handleReset function to
reset state; update references to draggedItems/setDraggedItems and handleReset
to use INITIAL_ITEMS so the initial items are defined once and kept consistent.
- Line 78: Replace the hard-coded string "bn-drag-exclude" with the exported
constant DRAG_EXCLUSION_CLASSNAME from `@blocknote/core`: import
DRAG_EXCLUSION_CLASSNAME at the top of App.tsx (if not already imported) and use
that constant in the div's className (e.g., className={`drag-demo-section
${DRAG_EXCLUSION_CLASSNAME}`}) so the example uses the exported symbol rather
than a literal.
In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts`:
- Around line 7-11: The EdgeDropPosition interface currently types node as any;
update it to use ProseMirror's Node type for stronger typing: change
EdgeDropPosition.node from any to import and use the Node type from
prosemirror-model (e.g., Node) so EdgeDropPosition, and any code using
multiColumnDropCursor.ts, benefits from IDE/compile-time checks; ensure you add
the appropriate import for Node at the top of the file and update any related
places that construct or inspect EdgeDropPosition to satisfy the stronger type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d96cc6c-9efc-4e39-9d61-c6031219e0f5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.github/dependabot.ymlexamples/03-ui-components/18-drag-n-drop/.bnexample.jsonexamples/03-ui-components/18-drag-n-drop/README.mdexamples/03-ui-components/18-drag-n-drop/index.htmlexamples/03-ui-components/18-drag-n-drop/main.tsxexamples/03-ui-components/18-drag-n-drop/package.jsonexamples/03-ui-components/18-drag-n-drop/src/App.tsxexamples/03-ui-components/18-drag-n-drop/src/styles.cssexamples/03-ui-components/18-drag-n-drop/tsconfig.jsonexamples/03-ui-components/18-drag-n-drop/vite.config.tspackages/core/package.jsonpackages/core/src/editor/BlockNoteEditor.tspackages/core/src/extensions/DropCursor/DropCursor.tspackages/core/src/extensions/DropCursor/utils.tspackages/core/src/index.tspackages/xl-multi-column/src/blocks/Columns/index.tspackages/xl-multi-column/src/extensions/DropCursor/MultiColumnDropCursorPlugin.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.tspackages/xl-multi-column/src/index.tsplayground/src/examples.gen.tsxplayground/src/main.tsx
💤 Files with no reviewable changes (3)
- .github/dependabot.yml
- packages/core/package.json
- packages/xl-multi-column/src/extensions/DropCursor/MultiColumnDropCursorPlugin.ts
packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts
Show resolved
Hide resolved
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts
Show resolved
Hide resolved
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts
Show resolved
Hide resolved
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts (1)
97-103:⚠️ Potential issue | 🟠 MajorNew column missing
widthproperty causes NaN in width normalization.The newly inserted column has
props: {}without awidthproperty. When this column is later processed by the normalization logic (lines 63-79),column.props.widthwill beundefined, causing arithmetic operations to produceNaN.This was flagged in a previous review and remains unaddressed.
🐛 Proposed fix - add default width
.toSpliced(edgePos.position === "left" ? index : index + 1, 0, { type: "column", children: [draggedBlock], - props: {}, + props: { width: 1 }, content: undefined, id: UniqueID.options.generateID(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts` around lines 97 - 103, The new column object inserted in the array (the toSpliced call that creates an object with type: "column", children: [draggedBlock], props: {}, content: undefined, id: UniqueID.options.generateID()) lacks a default width, which later causes NaN in the column width normalization; update the inserted object in multiColumnHandleDropPlugin (the toSpliced insertion) to include a sane default width (e.g., props: { width: <defaultNumber> }) so column.props.width is defined before normalization (lines referenced in the review: the column normalization logic).
🧹 Nitpick comments (1)
packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts (1)
41-44: Consider defensive check fornodeAfter.The non-null assertion on
resolved.nodeAfter!relies ongetNearestBlockPosalways returning a valid position with a node. While this should hold, a defensive check would be more robust.♻️ Optional defensive approach
const posInfo = { posBeforeNode: resolved.pos, - node: resolved.nodeAfter!, + node: resolved.nodeAfter ?? blockPos.node, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts` around lines 41 - 44, The code uses a non-null assertion on resolved.nodeAfter when building posInfo (posInfo, resolved, getNearestBlockPos, nodeAfter), so add a defensive guard that checks if resolved.nodeAfter is undefined before creating posInfo; if undefined, handle gracefully (e.g., return early/null, skip the drop-cursor logic, or throw a clear error) and log or surface context as needed so the rest of the function never relies on a possibly missing nodeAfter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/extensions/DropCursor/DropCursor.ts`:
- Around line 116-124: The code assumes view.dom.offsetParent is non-null and
immediately uses it as parent to append the drop-cursor element; when
offsetParent is null this throws. Add a null-guard: if view.dom.offsetParent is
null, fall back to using document.body (or another safe container) or skip
creating the element until a valid parent exists, i.e., check
view.dom.offsetParent before using parent.appendChild in the DropCursor creation
code (the block referencing view.dom.offsetParent, element, and config.color in
DropCursor.ts) and only call appendChild when parent is non-null, applying the
same style/backgroundColor setup afterward.
In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts`:
- Around line 46-54: The code assumes view.nodeDOM(posInfo.posBeforeNode)
returns an HTMLElement but it can return other Node types (e.g., Text), so
before calling getBoundingClientRect() on blockElement add an instanceof
HTMLElement check; if blockElement is not an HTMLElement (or is null) return the
same fallback object as currently returned, otherwise cast to HTMLElement and
call getBoundingClientRect(); update any references to blockRect accordingly in
multiColumnDropCursor.ts around view.nodeDOM, blockElement, and
getBoundingClientRect to avoid calling the method on non-elements.
In
`@packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts`:
- Around line 136-141: In the DropCursor handling code inside
multiColumnHandleDropPlugin.ts where new column nodes are built from blocks (the
children: blocks.map(...) block), add a width property to each created "column"
object (e.g., width: DEFAULT_COLUMN_WIDTH or compute from the normalized
columnList) so columns are created with a defined numeric width to match the
normalized schema and avoid NaN in later normalization passes; update the map
that returns { type: "column", children: [b] } to return { type: "column",
width: <defaultWidth>, children: [b] } (use the same default/constant used
elsewhere for columns).
---
Duplicate comments:
In
`@packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts`:
- Around line 97-103: The new column object inserted in the array (the toSpliced
call that creates an object with type: "column", children: [draggedBlock],
props: {}, content: undefined, id: UniqueID.options.generateID()) lacks a
default width, which later causes NaN in the column width normalization; update
the inserted object in multiColumnHandleDropPlugin (the toSpliced insertion) to
include a sane default width (e.g., props: { width: <defaultNumber> }) so
column.props.width is defined before normalization (lines referenced in the
review: the column normalization logic).
---
Nitpick comments:
In `@packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts`:
- Around line 41-44: The code uses a non-null assertion on resolved.nodeAfter
when building posInfo (posInfo, resolved, getNearestBlockPos, nodeAfter), so add
a defensive guard that checks if resolved.nodeAfter is undefined before creating
posInfo; if undefined, handle gracefully (e.g., return early/null, skip the
drop-cursor logic, or throw a clear error) and log or surface context as needed
so the rest of the function never relies on a possibly missing nodeAfter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95ed2c3f-d3ef-4288-9800-6bad9b252385
📒 Files selected for processing (5)
examples/03-ui-components/18-drag-n-drop/README.mdpackages/core/src/extensions/DropCursor/DropCursor.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.tspackages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.tsplayground/src/examples.gen.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- playground/src/examples.gen.tsx
packages/xl-multi-column/src/extensions/DropCursor/multiColumnDropCursor.ts
Show resolved
Hide resolved
packages/xl-multi-column/src/extensions/DropCursor/multiColumnHandleDropPlugin.ts
Show resolved
Hide resolved
| import type { BlockNoteEditor } from "../../editor/BlockNoteEditor.js"; | ||
| import { createExtension } from "../../editor/BlockNoteExtension.js"; | ||
|
|
||
| export const DRAG_EXCLUSION_CLASSNAME = "bn-drag-exclude"; |
There was a problem hiding this comment.
do we want to export this as a default classname? And have a global export that we need to document, etc? Or rather just document options.exclude (without default)?
There was a problem hiding this comment.
I think it's easier to describe that you can just add a classname than having to configure it
There was a problem hiding this comment.
export I could lose
| * Refactored to use BlockNote extension pattern with mount callback and AbortSignal | ||
| * for lifecycle management instead of ProseMirror PluginView. | ||
| */ | ||
| export const DropCursorExtension = createExtension< |
There was a problem hiding this comment.
I assume the logic itself is 95% the same as it was right? i.e. we don't need to review this extensively?
(except for the hooks and "exclude"?
There was a problem hiding this comment.
Yep, is the same, obv want to check behavior but my testing makes me think this is solid
@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: |
Summary
The goal of this refactor was to add the ability to exclude certain classnames from being considered for drag & drop into the editor. This proved to be difficult with the existing
prosemirror-dropcursorextension. We have already run into issues with this extension in the past (e.g. with the multi-column dropping), so I decided to just refactor this into a BlockNote extension. Now, the default extension can handle the use cases of both the multi-column extension & the base drop-cursor needs, by exposing options in a similar way to how we already exposed replacing the plugin entirely.What is cool, is that while the underlying options & interface have changed, this should be a zero code change for consumers to upgrade (assuming they bump both the editor & multi-column at the same time).
Rationale
Changes
Impact
Testing
Screenshots/Video
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation
Chores