fix(core): slash menu fails in custom blocks after space#2553
Open
fix(core): slash menu fails in custom blocks after space#2553
Conversation
…h menu in custom blocks Tiptap's NodeViewWrapper hardcodes `white-space: normal` on custom block wrappers. Under this style, browsers normalize trailing spaces to NBSP, causing ProseMirror's readDOMChange to compute a replacement (from !== to) instead of a pure insertion. The SuggestionMenu plugin's handleTextInput guard then rejects the input and the menu fails to open. Fix: add `white-space: pre-wrap` to `.bn-inline-content` in Block.css, overriding the inherited `normal` and preventing NBSP normalization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a CSS rule to preserve trailing spaces in inline block content, a new end-to-end Playwright test for slash-menu behavior in a custom alert block, and a new test URL constant for the alert-block test. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts (1)
46-48: Minor: Consider combining keystrokes for brevity.The space and slash could be typed in a single call for conciseness, though keeping them separate also clearly documents the regression scenario.
// Type a space first — this is the scenario that broke the menu - await page.keyboard.type(" "); - await page.keyboard.type("/"); + await page.keyboard.type(" /");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts` around lines 46 - 48, The test types a space then a slash in two separate calls to page.keyboard.type; compress these into a single call to page.keyboard.type that sends both characters together to make the test more concise while preserving the same keystroke sequence (update the two page.keyboard.type invocations to one combined invocation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts`:
- Around line 46-48: The test types a space then a slash in two separate calls
to page.keyboard.type; compress these into a single call to page.keyboard.type
that sends both characters together to make the test more concise while
preserving the same keystroke sequence (update the two page.keyboard.type
invocations to one combined invocation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3e013af-dadc-4f7d-850f-3aba1fca2e96
📒 Files selected for processing (2)
packages/core/src/editor/Block.csstests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts
@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: |
Contributor
|
I was able to validate that we fixed #659 on firefox with this |
The e2e test was using port 5173 (local dev server) instead of port 3000 which is what CI expects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts (1)
24-30: Extract shared alert-content focus steps into a helper.Both tests duplicate the same locator/focus/caret placement flow; pulling this into a helper will make future updates safer.
Refactor sketch
+async function focusAlertInlineContent(page: import("@playwright/test").Page) { + const alertContent = page + .locator('[data-content-type="alert"]') + .first() + .locator(".bn-inline-content"); + await alertContent.click(); + await page.keyboard.press("End"); + return alertContent; +} + test.describe("Slash menu in custom (alert) block – issue `#2531`", () => { @@ - const alertContent = page - .locator('[data-content-type="alert"]') - .first() - .locator(".bn-inline-content"); - await alertContent.click(); - await page.keyboard.press("End"); + await focusAlertInlineContent(page); @@ - const alertContent = page - .locator('[data-content-type="alert"]') - .first() - .locator(".bn-inline-content"); - await alertContent.click(); - await page.keyboard.press("End"); + await focusAlertInlineContent(page);Also applies to: 39-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts` around lines 24 - 30, Extract the duplicated locator/focus/caret placement into a small async helper (e.g., focusAlertContent or getAndFocusAlertContent) that accepts the Playwright Page, finds the element via locator('[data-content-type="alert"]').first().locator('.bn-inline-content'), clicks it and presses "End" to place the caret, then returns the locator if needed; replace both occurrences (the block using alertContent at the top and the similar block around lines 39-45) with calls to this helper and await its result so future updates to the focus behavior are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts`:
- Around line 6-8: The ternary that sets ALERT_BLOCK_URL relies on string
truthiness of process.env.RUN_IN_DOCKER (using !process.env.RUN_IN_DOCKER),
which misbehaves for values like "false"; change the check to an explicit string
comparison (e.g., process.env.RUN_IN_DOCKER === 'true') and flip the ternary
branches so ALERT_BLOCK_URL selects host.docker.internal when RUN_IN_DOCKER ===
'true' and localhost otherwise; apply the same explicit comparison pattern to
other similar constants (e.g., BASE_URL, SHADCN_URL, ARIAKIT_URL) to avoid
accidental Docker URL selection.
---
Nitpick comments:
In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts`:
- Around line 24-30: Extract the duplicated locator/focus/caret placement into a
small async helper (e.g., focusAlertContent or getAndFocusAlertContent) that
accepts the Playwright Page, finds the element via
locator('[data-content-type="alert"]').first().locator('.bn-inline-content'),
clicks it and presses "End" to place the caret, then returns the locator if
needed; replace both occurrences (the block using alertContent at the top and
the similar block around lines 39-45) with calls to this helper and await its
result so future updates to the focus behavior are centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0b65165-a213-4f21-b694-606129242d2f
📒 Files selected for processing (1)
tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts
Comment on lines
+6
to
+8
| const ALERT_BLOCK_URL = !process.env.RUN_IN_DOCKER | ||
| ? `http://localhost:${PORT}/custom-schema/alert-block?hideMenu` | ||
| : `http://host.docker.internal:${PORT}/custom-schema/alert-block?hideMenu`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 '\bRUN_IN_DOCKER\b' -g '!**/node_modules/**'Repository: TypeCellOS/BlockNote
Length of output: 4428
Make RUN_IN_DOCKER evaluation explicit to avoid accidental Docker URL selection.
Using !process.env.RUN_IN_DOCKER relies on string truthiness—!"false" evaluates to false (not true), causing the wrong URL to be selected if the variable is set to the string "false". This pattern is used throughout the codebase (tests/src/utils/const.ts has 10+ instances) and is set explicitly to "true" in CI scripts (tests/package.json), but an explicit comparison guards against environment-dependent test flakiness.
Proposed fix
const PORT = 3000;
-const ALERT_BLOCK_URL = !process.env.RUN_IN_DOCKER
+const isDocker = process.env.RUN_IN_DOCKER === "true";
+const ALERT_BLOCK_URL = !isDocker
? `http://localhost:${PORT}/custom-schema/alert-block?hideMenu`
: `http://host.docker.internal:${PORT}/custom-schema/alert-block?hideMenu`;Note: This same pattern exists in tests/src/utils/const.ts for BASE_URL, SHADCN_URL, ARIAKIT_URL, and others. Consider applying the same fix across the codebase for consistency.
📝 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
| const ALERT_BLOCK_URL = !process.env.RUN_IN_DOCKER | |
| ? `http://localhost:${PORT}/custom-schema/alert-block?hideMenu` | |
| : `http://host.docker.internal:${PORT}/custom-schema/alert-block?hideMenu`; | |
| const isDocker = process.env.RUN_IN_DOCKER === "true"; | |
| const ALERT_BLOCK_URL = !isDocker | |
| ? `http://localhost:${PORT}/custom-schema/alert-block?hideMenu` | |
| : `http://host.docker.internal:${PORT}/custom-schema/alert-block?hideMenu`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts` around lines 6
- 8, The ternary that sets ALERT_BLOCK_URL relies on string truthiness of
process.env.RUN_IN_DOCKER (using !process.env.RUN_IN_DOCKER), which misbehaves
for values like "false"; change the check to an explicit string comparison
(e.g., process.env.RUN_IN_DOCKER === 'true') and flip the ternary branches so
ALERT_BLOCK_URL selects host.docker.internal when RUN_IN_DOCKER === 'true' and
localhost otherwise; apply the same explicit comparison pattern to other similar
constants (e.g., BASE_URL, SHADCN_URL, ARIAKIT_URL) to avoid accidental Docker
URL selection.
Align with the pattern used by all other Playwright tests — URL is defined once in tests/src/utils/const.ts and imported, instead of being hardcoded in the test file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The CSS fix (white-space: pre-wrap on .bn-inline-content) causes a slight visual difference in static rendering where ProseMirror styles aren't loaded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
Fix the slash menu (and other suggestion menus) not opening when the trigger character is typed after a space inside custom blocks with
isolating: true.Closes #2531
Closes #659
Rationale
Tiptap's
NodeViewWrapperhardcodeswhite-space: normalas an inline style on all React-based custom block wrappers. ProseMirror requireswhite-space: pre-wrapon editable content areas — without it, browsers (especially Chrome) normalize trailing ASCII spaces to NBSP (\u00a0) during input.When a user types
/after a trailing space inside a custom block, the browser first converts the space to NBSP. ProseMirror'sreadDOMChange→findDiffthen sees a replacement (space→NBSP+char) instead of a pure insertion, callinghandleTextInput(view, from, to, text)withfrom !== to. The SuggestionMenu plugin'sif (from === to)guard rejects this, and the menu never opens.Issue #659 (spaces eaten at end of custom blocks in Firefox) shares the same root cause —
white-space: normalcollapses trailing whitespace. While we could not reproduce #659 on current Firefox versions bundled with Playwright, the fix addresses the underlying CSS problem.Changes
packages/core/src/editor/Block.css: Addedwhite-space: pre-wrapto.bn-inline-content, overriding thenormalinherited from tiptap's NodeViewWrapper.tests/src/end-to-end/slashmenu/slashmenu-customblock.test.ts: New Playwright e2e regression test using the alert-block demo — verifies the slash menu opens both with and without a preceding space.Impact
white-spacewas alreadypre-wrapvia inheritance from ProseMirror's default styles. The explicit declaration is a no-op.NodeViewWrapper:white-spacechanges fromnormaltopre-wrap, restoring correct ProseMirror behavior. This is the intended fix.pre-wrapis what ProseMirror already uses everywhere else.Testing
Checklist
Summary by CodeRabbit
Bug Fixes
Tests