fix: action buttons of left column in toggle block hover (#2526)#2671
fix: action buttons of left column in toggle block hover (#2526)#2671yamiletpineda wants to merge 3 commits intoTypeCellOS:mainfrom
Conversation
|
@Rn123456789 is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughColumn detection and side-menu positioning logic were changed to recognize Changes
Sequence Diagram(s)sequenceDiagram
participant Mouse
participant EditorView as Editor View
participant SideMenu as SideMenu Extension
participant DOM as Column DOM Element
Mouse->>EditorView: mousemove at (x,y)
EditorView->>SideMenu: onMouseMove event
SideMenu->>DOM: elementFromPoint(x,y) -> check `.bn-side-menu`
alt cursor over side menu
SideMenu-->>EditorView: bail out (no update)
else not over side menu
SideMenu->>DOM: find nearest column element (`data-node-type=column`)
SideMenu->>EditorView: resolve referenceBlock from (clampedX, y)
EditorView-->>SideMenu: referenceBlock
SideMenu->>SideMenu: compute menu position using columnRect.left/right
SideMenu-->>EditorView: update side-menu position / show hover
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/SideMenu/SideMenu.ts`:
- Around line 109-122: The current early return when a column is found uses
getBlockFromCoords with a fixed left offset and skips the nested-block
correction logic, causing parent blocks to be selected instead of indented
children; replace the early return with assigning the result of
getBlockFromCoords to a local variable (e.g., candidate =
getBlockFromCoords(view, {...})) and then fall through to the existing
nested-block correction logic that appears after this block so that
indented/nested blocks inside a column are still resolved (use candidate only if
the nested/block-correction step doesn't find a better match). Ensure you
reference the same symbols (referenceBlock, view, columnRect,
getBlockFromCoords) and preserve the original coordinate calculation but do not
return before running the nested resolution.
- Around line 626-630: The code in SideMenu.ts currently casts event.target to
HTMLElement | null and calls target.closest(".bn-side-menu"), which can throw if
event.target is a non-Element (e.g., Text node); modify the logic in the event
handler where `const target = event.target as HTMLElement | null` and the
subsequent `target?.closest(...)` check to first guard with `if (!(event.target
instanceof Element)) return;` or similar, then assign a properly typed `const
target: Element = event.target` and call `target.closest(".bn-side-menu")`—this
ensures `closest` is only invoked on an Element and prevents runtime TypeErrors.
In `@tests/src/end-to-end/draghandle/draghandle.test.ts`:
- Around line 185-196: The test currently grabs the first global
".bn-block-column" which can pass even if the columns are siblings of the
toggle; change the locator/assertion so the column is proved to be inside the
toggle before hovering. Specifically, in the "Hovering over column nested in
toggle block should show draghandle" test (the test function name) either scope
leftColumn to a toggle ancestor (e.g., find the toggle block created by
executeSlashCommand("togglelist") and then use its
locator().locator(".bn-block-column").first()) or add an explicit assertion that
the selected leftColumn is a descendant of the toggle (using a toggle locator
and an .contains/.getBy* or assertion that
toggle.locator(".bn-block-column").count() > 0) before calling
moveMouseOverElement, then proceed to waitForSelector(DRAG_HANDLE_SELECTOR) and
click as before.
🪄 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: 6bbe7cf1-5d9e-4289-97dc-5da44f7d7758
📒 Files selected for processing (2)
packages/core/src/extensions/SideMenu/SideMenu.tstests/src/end-to-end/draghandle/draghandle.test.ts
| const column = referenceBlock.node.closest("[data-node-type=column]"); | ||
|
|
||
| if (column) { | ||
| const columnRect = column.getBoundingClientRect(); | ||
|
|
||
| return getBlockFromCoords( | ||
| view, | ||
| { | ||
| left: Math.min(columnRect.left + 20, columnRect.right - 10), | ||
| top: mousePos.y, | ||
| }, | ||
| false, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Preserve nested-block resolution inside columns.
This early return skips the nested-block correction below Line 124. For indented/nested blocks inside a column, probing at columnRect.left + 20 can resolve the parent block instead of the nested block under the cursor.
Suggested fix
if (column) {
const columnRect = column.getBoundingClientRect();
+ const referenceBlocksBoundingBox =
+ referenceBlock.node.getBoundingClientRect();
return getBlockFromCoords(
view,
{
- left: Math.min(columnRect.left + 20, columnRect.right - 10),
+ left: Math.min(
+ Math.max(referenceBlocksBoundingBox.right - 10, columnRect.left + 10),
+ columnRect.right - 10,
+ ),
top: mousePos.y,
},
false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/extensions/SideMenu/SideMenu.ts` around lines 109 - 122,
The current early return when a column is found uses getBlockFromCoords with a
fixed left offset and skips the nested-block correction logic, causing parent
blocks to be selected instead of indented children; replace the early return
with assigning the result of getBlockFromCoords to a local variable (e.g.,
candidate = getBlockFromCoords(view, {...})) and then fall through to the
existing nested-block correction logic that appears after this block so that
indented/nested blocks inside a column are still resolved (use candidate only if
the nested/block-correction step doesn't find a better match). Ensure you
reference the same symbols (referenceBlock, view, columnRect,
getBlockFromCoords) and preserve the original coordinate calculation but do not
return before running the nested resolution.
| const target = event.target as HTMLElement | null; | ||
|
|
||
| if (target?.closest(".bn-side-menu")) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "SideMenu.ts" -type f | head -20Repository: TypeCellOS/BlockNote
Length of output: 115
🏁 Script executed:
cat -n ./packages/core/src/extensions/SideMenu/SideMenu.ts | sed -n '610,640p'Repository: TypeCellOS/BlockNote
Length of output: 1208
🌐 Web query:
DOM event.target type MouseEvent .closest method Element
💡 Result:
In the DOM, for a MouseEvent (e.g., from a 'click' handler), event.target is an EventTarget referencing the innermost element that dispatched the event, typically an Element like an input or button. Since Element.closest(selectors) is a standard method that traverses up the DOM tree to find the nearest ancestor (or self) matching the CSS selectors, you can safely call event.target.closest(selectors) after ensuring it's an Element. This pattern is commonly used for event delegation: attach a single listener to a parent, then use event.target.closest to identify the relevant child element. Example: document.addEventListener('click', function(event) { const button = (event.target as Element).closest('button.special'); if (button) { // Handle click on special button or its children } }); TypeScript requires casting event.target to Element or HTMLElement since EventTarget lacks closest. MouseEvent generics specify the expected target type (e.g., MouseEvent). No changes or deprecations as of 2026 per MDN docs.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
- 2: https://adactio.medium.com/event-target-closest-536c7498a897
- 3: https://stackoverflow.com/questions/63046477/how-to-get-an-element-from-event-target-closestname
- 4: https://developer.mozilla.org/en-US/docs/DOM/event.target
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Event/target
- 6: https://developer.mozilla.org/en-US/docs/Web/API/Element.closest
🌐 Web query:
Text node Comment node .closest() method throws TypeError
💡 Result:
The .closest method throws a TypeError when called on a Text node or Comment node because it is defined only on the Element interface, not on Node. Text nodes (nodeType 3) and Comment nodes (nodeType 8) are not Elements, so they do not have the closest method. To find the closest ancestor element matching a selector from a Text or Comment node, first access its parent Element using node.parentElement (or node.parentNode if you need to check the type), then call closest on that Element. Example: const textNode = someElement.firstChild; // Text node if (textNode.nodeType === Node.TEXT_NODE) { const closestDiv = textNode.parentElement.closest('div'); } This is a current DOM specification behavior as of 2026; an open WHATWG issue proposes extending closest to more node types like Text, but it remains Element-only.
Citations:
- 1: Adding .closest() to more nodes whatwg/dom#161
- 2: https://developer.mozilla.org/en/DOM/Node.nodeType
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
Narrow event.target before calling .closest.
The cast to HTMLElement | null does not guarantee the runtime target is an Element. Since event.target has type EventTarget, it could be a non-Element node (e.g., Text or Comment), and calling .closest() on such nodes throws a TypeError. Use an instanceof Element guard to safely narrow the type.
Suggested fix
- const target = event.target as HTMLElement | null;
+ const target = event.target;
- if (target?.closest(".bn-side-menu")) {
- return;
+ if (target instanceof Element && target.closest(".bn-side-menu")) {
+ return;
}📝 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.
| const target = event.target as HTMLElement | null; | |
| if (target?.closest(".bn-side-menu")) { | |
| return; | |
| } | |
| const target = event.target; | |
| if (target instanceof Element && target.closest(".bn-side-menu")) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/extensions/SideMenu/SideMenu.ts` around lines 626 - 630,
The code in SideMenu.ts currently casts event.target to HTMLElement | null and
calls target.closest(".bn-side-menu"), which can throw if event.target is a
non-Element (e.g., Text node); modify the logic in the event handler where
`const target = event.target as HTMLElement | null` and the subsequent
`target?.closest(...)` check to first guard with `if (!(event.target instanceof
Element)) return;` or similar, then assign a properly typed `const target:
Element = event.target` and call `target.closest(".bn-side-menu")`—this ensures
`closest` is only invoked on an Element and prevents runtime TypeErrors.
| test("Hovering over column nested in toggle block should show draghandle", async () => { | ||
| await executeSlashCommand(page, "togglelist"); | ||
| await page.keyboard.type("Toggle heading"); | ||
| await page.keyboard.press("Enter"); | ||
| await executeSlashCommand(page, "two"); | ||
| await page.keyboard.type("Left column content"); | ||
| const leftColumn = await page.locator(".bn-block-column").first(); | ||
| await moveMouseOverElement(page, leftColumn); | ||
| await page.waitForSelector(DRAG_HANDLE_SELECTOR); | ||
| await page.click(DRAG_HANDLE_SELECTOR); | ||
| await page.waitForSelector(DRAG_HANDLE_MENU_SELECTOR); | ||
| }); |
There was a problem hiding this comment.
Make the regression test prove the column is inside the toggle.
Right now the test selects the first column globally, so it can still pass if Enter creates the columns as a sibling of the toggle instead of a child. Scope the locator to a toggle ancestor, or assert that relationship before hovering.
Suggested test hardening
- const leftColumn = await page.locator(".bn-block-column").first();
+ const leftColumn = page
+ .locator('[data-node-type*="toggle" i] [data-node-type="column"]')
+ .first();
+ await expect(leftColumn).toBeVisible();
await moveMouseOverElement(page, leftColumn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/end-to-end/draghandle/draghandle.test.ts` around lines 185 - 196,
The test currently grabs the first global ".bn-block-column" which can pass even
if the columns are siblings of the toggle; change the locator/assertion so the
column is proved to be inside the toggle before hovering. Specifically, in the
"Hovering over column nested in toggle block should show draghandle" test (the
test function name) either scope leftColumn to a toggle ancestor (e.g., find the
toggle block created by executeSlashCommand("togglelist") and then use its
locator().locator(".bn-block-column").first()) or add an explicit assertion that
the selected leftColumn is a descendant of the toggle (using a toggle locator
and an .contains/.getBy* or assertion that
toggle.locator(".bn-block-column").count() > 0) before calling
moveMouseOverElement, then proceed to waitForSelector(DRAG_HANDLE_SELECTOR) and
click as before.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/api/clipboard/toClipboard/copyExtension.ts (1)
132-135: Nit: formatting / scope of unwrap.Indentation of the
ifblock is off (extra indent inside the brace), and the unwrap triggers on any single-childblockGroup, including when an explicitNodeSelectionof ablockGroupis copied. If that case should retain the outer wrapper, guard with e.g.!("node" in view.state.selection). Otherwise safe givendoc's content spec is a singleblockGroup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/api/clipboard/toClipboard/copyExtension.ts` around lines 132 - 135, The if-block wrongly indents and unconditionally unwraps a single-child blockGroup, which also unwraps when the user has a NodeSelection of that blockGroup; fix by normalizing indentation for the if and add a selection-type guard so you only unwrap when the current selection is not a node selection (e.g. check that !"node" in view.state.selection or similar) before assigning selectedFragment = selectedFragment.firstChild.content; references: selectedFragment, blockGroup, and view.state.selection/NodeSelection.packages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts (1)
956-966: Scope: unrelated to PR objective.This
Mod-ashortcut and the clipboardSlicechanges incopyExtension.tsare unrelated to the stated fix (side-menu action buttons on toggle-nested columns, issue#2526). Consider splitting them into a separate PR to keep the change set focused and simplify review/revert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts` around lines 956 - 966, The Mod-a keyboard shortcut handler (the "Mod-a" case in KeyboardShortcutsExtension.ts that sets an AllSelection) and the clipboard Slice changes in copyExtension.ts are unrelated to the side-menu toggle-nested columns fix; revert or remove these unrelated edits from this branch/commit and put them into a separate focused PR: locate the "Mod-a" case in KeyboardShortcutsExtension.ts and restore the previous behavior (or remove the added AllSelection logic), and revert the Slice/clipboard changes in copyExtension.ts, then create a new branch/PR containing only those two changes so this PR only contains the toggle-nested columns fixes.
🤖 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/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts`:
- Around line 956-966: The "Mod-a" key handler currently uses optional chaining
on this.options.editor.prosemirrorView so it may return true even when no
dispatch occurs; replace the direct dispatch with the TipTap command pattern to
ensure consistent pipeline and only swallow the event when dispatch happened:
inside the "Mod-a" handler call this.editor.commands.command(({ tr, dispatch })
=> { const { doc } = this.options.editor.prosemirrorState; if (dispatch)
dispatch(tr.setSelection(new AllSelection(doc))); return !!dispatch; }); or
alternatively use this.editor.view.dispatch if you must dispatch directly, but
ensure you check view exists and return false when no dispatch occurred. Ensure
references include the existing AllSelection,
this.options.editor.prosemirrorState, and the handler name "Mod-a".
---
Nitpick comments:
In `@packages/core/src/api/clipboard/toClipboard/copyExtension.ts`:
- Around line 132-135: The if-block wrongly indents and unconditionally unwraps
a single-child blockGroup, which also unwraps when the user has a NodeSelection
of that blockGroup; fix by normalizing indentation for the if and add a
selection-type guard so you only unwrap when the current selection is not a node
selection (e.g. check that !"node" in view.state.selection or similar) before
assigning selectedFragment = selectedFragment.firstChild.content; references:
selectedFragment, blockGroup, and view.state.selection/NodeSelection.
In
`@packages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts`:
- Around line 956-966: The Mod-a keyboard shortcut handler (the "Mod-a" case in
KeyboardShortcutsExtension.ts that sets an AllSelection) and the clipboard Slice
changes in copyExtension.ts are unrelated to the side-menu toggle-nested columns
fix; revert or remove these unrelated edits from this branch/commit and put them
into a separate focused PR: locate the "Mod-a" case in
KeyboardShortcutsExtension.ts and restore the previous behavior (or remove the
added AllSelection logic), and revert the Slice/clipboard changes in
copyExtension.ts, then create a new branch/PR containing only those two changes
so this PR only contains the toggle-nested columns fixes.
🪄 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: 4d2607dd-15ae-4deb-bfed-7465630e8cb6
📒 Files selected for processing (2)
packages/core/src/api/clipboard/toClipboard/copyExtension.tspackages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts
| // Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that | ||
| // TextSelection would skip. | ||
| "Mod-a": () => { | ||
| const { doc } = this.options.editor.prosemirrorState; | ||
| this.options.editor.prosemirrorView?.dispatch( | ||
| this.options.editor.prosemirrorState.tr.setSelection( | ||
| new AllSelection(doc) | ||
| ) | ||
| ); | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
Mod-a returns true even when dispatch is skipped.
prosemirrorView is accessed via optional chaining, so if it's undefined the selection is never updated but the handler still returns true, swallowing the shortcut and preventing the browser's native Select-All fallback. Also, dispatching directly bypasses the TipTap command pipeline used elsewhere in this file — prefer this.editor.commands.command(({ tr, dispatch }) => { ... }) or this.editor.view.dispatch for consistency with the rest of the keymap (which relies on this.editor, not this.options.editor.prosemirrorView).
♻️ Suggested change
- "Mod-a": () => {
- const { doc } = this.options.editor.prosemirrorState;
- this.options.editor.prosemirrorView?.dispatch(
- this.options.editor.prosemirrorState.tr.setSelection(
- new AllSelection(doc)
- )
- );
- return true;
- },
+ "Mod-a": () => {
+ const view = this.options.editor.prosemirrorView;
+ if (!view) {
+ return false;
+ }
+ view.dispatch(
+ view.state.tr.setSelection(new AllSelection(view.state.doc)),
+ );
+ return true;
+ },📝 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.
| // Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that | |
| // TextSelection would skip. | |
| "Mod-a": () => { | |
| const { doc } = this.options.editor.prosemirrorState; | |
| this.options.editor.prosemirrorView?.dispatch( | |
| this.options.editor.prosemirrorState.tr.setSelection( | |
| new AllSelection(doc) | |
| ) | |
| ); | |
| return true; | |
| }, | |
| // Forces AllSelection from pos 0 to include non-editable blocks (e.g. images) that | |
| // TextSelection would skip. | |
| "Mod-a": () => { | |
| const view = this.options.editor.prosemirrorView; | |
| if (!view) { | |
| return false; | |
| } | |
| view.dispatch( | |
| view.state.tr.setSelection(new AllSelection(view.state.doc)), | |
| ); | |
| return true; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.ts`
around lines 956 - 966, The "Mod-a" key handler currently uses optional chaining
on this.options.editor.prosemirrorView so it may return true even when no
dispatch occurs; replace the direct dispatch with the TipTap command pattern to
ensure consistent pipeline and only swallow the event when dispatch happened:
inside the "Mod-a" handler call this.editor.commands.command(({ tr, dispatch })
=> { const { doc } = this.options.editor.prosemirrorState; if (dispatch)
dispatch(tr.setSelection(new AllSelection(doc))); return !!dispatch; }); or
alternatively use this.editor.view.dispatch if you must dispatch directly, but
ensure you check view exists and return false when no dispatch occurred. Ensure
references include the existing AllSelection,
this.options.editor.prosemirrorState, and the handler name "Mod-a".
2 participants
Summary
Fixes #2526. Action buttons of the left column in a multi-column block not appearing or being clickable when column block is nested in a toggle list block.
Rationale
When a multi-column block is nested inside a toggle list block, the side menu action buttons of the leftmost column were not appearing or clickable on hover. This was due to incorrect column detection logic and a hardcoded x-coordinate offset in
SideMenu.ts.Changes
getBlockFromCoordsfrom[data-node-type=columnList]to[data-node-type=column].left: coords.left + 50offset with a bounds-aware clamp using the column's actualgetBoundingClientRect().getBlockFromMousePosto handle columns nested inside toggle list blocks.onMouseMoveto return early if the cursor is over a.bn-side-menuelement, preventing the side menu from disappearing when hovered.updateStateFromMousePosto usecolumn.getBoundingClientRect().xdirectly.Impact
Side menu action buttons now correctly appear and are clickable when hovering over columns nested inside toggle blocks.
Testing
Screenshots/Video
multi-column-toggle-side-menu.copy.mov
Checklist
Additional Notes
The added test is a Playwright end-to-end test, not a unit test.
Summary by CodeRabbit