Added click handling and HTML attribute link options to the editor#2656
Open
matthewlipski wants to merge 2 commits intomainfrom
Open
Added click handling and HTML attribute link options to the editor#2656matthewlipski wants to merge 2 commits intomainfrom
matthewlipski wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
actor User
participant Editor as BlockNote Editor
participant Plugin as Link Click Plugin
participant Handler as Custom onClick / window.open
User->>Editor: click on link
Editor->>Plugin: dispatch click to plugin
Plugin->>Plugin: check left-button & editable
Plugin->>Plugin: find nearest <a> within editor root
alt options.links.onClick provided
Plugin->>Handler: invoke options.links.onClick(event)
Handler->>Plugin: handler handles event
else no onClick
Plugin->>Handler: extract href/target and call window.open(href, target)
Handler->>Plugin: navigation performed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
packages/core/src/editor/managers/ExtensionManager/extensions.ts (2)
145-152: Remove commented-outenableClickSelectionblock.Dead commented-out code referring to an option that is "always disabled" is noise — if it's not coming back in this PR, prefer deleting it (it'll still be in git history). If it's a planned follow-up, a
TODOreferencing an issue is clearer than a paragraph of commented code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts` around lines 145 - 152, Remove the dead commented-out block that references enableClickSelection and the extendMarkRange call (the commented lines containing "if (options.enableClickSelection) {", "tiptapEditor.commands.extendMarkRange", and "markType.name"); delete those commented lines rather than leaving them in place, or if you want to keep a note add a single-line TODO referencing the issue number, but do not keep the multi-line commented code in ExtensionManager/extensions.ts.
96-174: Missing test coverage for the new click handler.The PR checklist claims "Unit tests covering the new feature have been added", but this handler has non-trivial branching (left-button check, editable check, ancestor walk bounded to editor root,
onClickoverride vs.window.openfallback) and several of the bugs above would be caught by straightforward tests. Consider adding unit tests that at minimum cover:
- Non-left-button click (should be ignored).
- Click on non-anchor descendant inside an anchor (ancestor lookup).
- Anchor outside editor root (should not be handled).
onClickprovided →onClickinvoked andwindow.opennot called.onClicknot provided →window.opencalled with correcthref/target, including when those come from mark attrs rather than DOM attributes.Happy to sketch out a test file for this handler if you'd like.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts` around lines 96 - 174, Add unit tests for the click handler produced by ExtensionManager.addProseMirrorPlugins: exercise the Plugin.props.handleClick branching (check non-left-button returns false; clicking a non-anchor descendant inside an anchor resolves via closest; clicks on anchors outside the tiptapEditor.view.dom root are ignored), verify options.links.onClick is invoked when present (and window.open is not called), and verify window.open is called with correct href/target when onClick is absent, including when href/target are provided via getAttributes(view.state, markType.name) rather than the DOM anchor; target the handleClick logic by constructing a mock view (tiptapEditor.view), stub getAttributes, and spy on window.open and options.links.onClick to assert behavior.docs/content/docs/features/blocks/inline-content.mdx (1)
115-127: ClarifyonClicksemantics in the docs.The description states that providing
onClickdisables the default open-in-new-window behavior, but it does not mention a few subtleties that users are likely to hit:
- The handler receives only the raw
MouseEvent; to gethref/target, the consumer has to walkevent.targetup to the nearest<a>themselves. Exposing at least the anchor element (orhref/target) in the callback signature would make this much more usable.- The default (no
onClick) currently opens viawindow.open(href, target)with nonoopener/noreferrer— worth documenting so consumers know when to supply their own handler for security-sensitive contexts.- It would help to mention that the callback is only invoked on primary-button (left) clicks inside an editable view.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/features/blocks/inline-content.mdx` around lines 115 - 127, Update the onClick docs for BlockNoteEditor.links to clarify behavior: state that links.onClick receives only the raw MouseEvent (not the anchor or href/target) so consumers must walk event.target up to the nearest <a> to read href/target, note the default behavior when onClick is undefined is window.open(href, target) without noopener/noreferrer (so recommend supplying a custom handler for security-sensitive contexts), and mention the callback is only invoked for primary-button (left) clicks inside an editable view; reference the onClick option and BlockNoteEditor.links in the text so readers can locate the setting.packages/core/src/editor/BlockNoteEditor.ts (1)
143-161: Consider lettingonClicksignal whether it handled the event.The current signature
(event: MouseEvent) => voidforces the editor to treat everyonClickinvocation as fully handling the click (seeextensions.tslines 154–165), which is fine for the documented "custom routing" use case but makes it impossible for a consumer to fall through to the default open-in-new-window behavior conditionally (e.g. ignore modifier-clicks, or only intercept same-origin links). Returningvoid | boolean— wheretruemeans "handled, skip default" and falsy means "fall through to default" — would be a more forward-compatible API. Not blocking, just easier to evolve than widening the return type later.- onClick?: (event: MouseEvent) => void; + onClick?: (event: MouseEvent) => void | boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/editor/BlockNoteEditor.ts` around lines 143 - 161, Update the links.onClick handler to return a boolean-ish value so consumers can signal whether they handled the event: change the documented signature from (event: MouseEvent) => void to (event: MouseEvent) => boolean | void (or boolean | undefined), update the implementation that invokes onClick (the caller in extensions.ts that currently treats any invocation as handled) to only suppress the default open-on-click behavior when onClick returns a truthy value, and update the JSDoc comment for links.HTMLAttributes/onClick to explain that returning true prevents the default open-in-new-window behavior while falsy allows fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/features/blocks/inline-content.mdx`:
- Around line 86-98: The docs show using the protected constructor (new
BlockNoteEditor(...)) which is not public; update the three examples to call the
public factory instead by replacing new BlockNoteEditor({...}) with
BlockNoteEditor.create({...}) so they match the editor's public API (constructor
is protected in BlockNoteEditor.ts) and will type-check for users.
In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts`:
- Around line 143-168: The handleClick branch that calls options.links?.onClick
currently leaves handled as false so the DOM click may still trigger navigation;
update the onClick branch in the handleClick implementation to prevent default
browser navigation and mark the click as handled — e.g., call
event.preventDefault() (and optionally event.stopPropagation()) and set handled
= true after invoking options.links.onClick(event) so the function returns true
and the click is consumed; keep the existing fallback behavior (using
getAttributes/href/window.open) unchanged.
- Around line 130-131: Fix the typo and complete the truncated comment in
extensions.ts: change "Tntentionally" to "Intentionally" and finish the sentence
so it reads clearly (e.g., "Intentionally limit the lookup to the editor root.
Using tag names like DIV as boundaries breaks with custom NodeViews, so we must
scope to the editor root to avoid incorrect boundary detection."). Update the
comment near the ExtensionManager or extensions.ts lookup logic (the block that
mentions editor root and NodeViews) so it is grammatically correct and conveys
the full rationale.
- Around line 161-163: The current call window.open(href, target) is vulnerable
to tabnabbing; update the logic where href/target are used (the branch that sets
handled = true) to call window.open with the feature string
"noopener,noreferrer" (e.g., window.open(href, target, "noopener,noreferrer"))
so the opened page cannot access window.opener; ensure this is applied whenever
opening external links from the editor (the code path using href, target,
handled) and keep handled = true unchanged.
- Around line 158-164: Replace uses of the DOM IDL properties so fallback values
work: read href and target via link.getAttribute('href') and
link.getAttribute('target') (e.g., const hrefAttr = link.getAttribute('href') ??
attrs.href; const linkTarget = link.getAttribute('target') ?? attrs.target)
instead of link.href / link.target; rename the local target variable to avoid
shadowing the DOM property (e.g., linkTarget) and pass that to window.open;
ensure that when options.links.onClick is called you set handled = true (or call
event.preventDefault()) so ProseMirror doesn't allow default navigation; also
correct the typo "Tntentionally" to "Intentionally" in the surrounding
comment/strings.
---
Nitpick comments:
In `@docs/content/docs/features/blocks/inline-content.mdx`:
- Around line 115-127: Update the onClick docs for BlockNoteEditor.links to
clarify behavior: state that links.onClick receives only the raw MouseEvent (not
the anchor or href/target) so consumers must walk event.target up to the nearest
<a> to read href/target, note the default behavior when onClick is undefined is
window.open(href, target) without noopener/noreferrer (so recommend supplying a
custom handler for security-sensitive contexts), and mention the callback is
only invoked for primary-button (left) clicks inside an editable view; reference
the onClick option and BlockNoteEditor.links in the text so readers can locate
the setting.
In `@packages/core/src/editor/BlockNoteEditor.ts`:
- Around line 143-161: Update the links.onClick handler to return a boolean-ish
value so consumers can signal whether they handled the event: change the
documented signature from (event: MouseEvent) => void to (event: MouseEvent) =>
boolean | void (or boolean | undefined), update the implementation that invokes
onClick (the caller in extensions.ts that currently treats any invocation as
handled) to only suppress the default open-on-click behavior when onClick
returns a truthy value, and update the JSDoc comment for
links.HTMLAttributes/onClick to explain that returning true prevents the default
open-in-new-window behavior while falsy allows fallback.
In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts`:
- Around line 145-152: Remove the dead commented-out block that references
enableClickSelection and the extendMarkRange call (the commented lines
containing "if (options.enableClickSelection) {",
"tiptapEditor.commands.extendMarkRange", and "markType.name"); delete those
commented lines rather than leaving them in place, or if you want to keep a note
add a single-line TODO referencing the issue number, but do not keep the
multi-line commented code in ExtensionManager/extensions.ts.
- Around line 96-174: Add unit tests for the click handler produced by
ExtensionManager.addProseMirrorPlugins: exercise the Plugin.props.handleClick
branching (check non-left-button returns false; clicking a non-anchor descendant
inside an anchor resolves via closest; clicks on anchors outside the
tiptapEditor.view.dom root are ignored), verify options.links.onClick is invoked
when present (and window.open is not called), and verify window.open is called
with correct href/target when onClick is absent, including when href/target are
provided via getAttributes(view.state, markType.name) rather than the DOM
anchor; target the handleClick logic by constructing a mock view
(tiptapEditor.view), stub getAttributes, and spy on window.open and
options.links.onClick to assert behavior.
🪄 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: c07edf04-54b9-4e24-985b-228535edc5ce
📒 Files selected for processing (3)
docs/content/docs/features/blocks/inline-content.mdxpackages/core/src/editor/BlockNoteEditor.tspackages/core/src/editor/managers/ExtensionManager/extensions.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: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/editor/managers/ExtensionManager/extensions.ts (1)
158-164:⚠️ Potential issue | 🟠 MajorFix the default-open path before relying on fallbacks.
link.href ?? attrs.hrefandlink.target ?? attrs.targetstill won’t fall back when the DOM properties return"", andwindow.open(href, target)should includenoopener,noreferrerfor user-authored links.🐛 Proposed fix
- const href = link.href ?? attrs.href; - const target = link.target ?? attrs.target; + const href = link.getAttribute("href") ?? attrs.href; + const linkTarget = + link.getAttribute("target") ?? attrs.target; if (href) { - window.open(href, target); + window.open( + href, + linkTarget || "_blank", + "noopener,noreferrer", + ); handled = true; }Verify HTMLAnchorElement.href and HTMLAnchorElement.target behavior when the corresponding attributes are absent, and verify that window.open with "noopener,noreferrer" prevents opener access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts` around lines 158 - 164, The current fallback uses link.href ?? attrs.href which treats empty string as valid — change to use a truthy check so href = (link.href ?? "") || attrs.href and target = (link.target ?? "") || attrs.target, then only call window.open if href is non-empty; when opening user links include the noopener,noreferrer feature string in the third argument to window.open to prevent opener access (use the same conditional branch that currently calls window.open and reference getAttributes, markType.name, link.href/target, attrs.href/target, and window.open).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/core/src/editor/managers/ExtensionManager/extensions.ts`:
- Around line 158-164: The current fallback uses link.href ?? attrs.href which
treats empty string as valid — change to use a truthy check so href = (link.href
?? "") || attrs.href and target = (link.target ?? "") || attrs.target, then only
call window.open if href is non-empty; when opening user links include the
noopener,noreferrer feature string in the third argument to window.open to
prevent opener access (use the same conditional branch that currently calls
window.open and reference getAttributes, markType.name, link.href/target,
attrs.href/target, and window.open).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f62267d0-cfad-4274-b9e0-3adbea9769d5
📒 Files selected for processing (2)
docs/content/docs/features/blocks/inline-content.mdxpackages/core/src/editor/managers/ExtensionManager/extensions.ts
✅ Files skipped from review due to trivial changes (1)
- docs/content/docs/features/blocks/inline-content.mdx
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.
Summary
This PR adds a new editor option:
These do basically what they say -
HTMLAttributesadds HTML attributes to rendered link elements andonClickreplaces the default click behaviour (which opens the link in a new tab).Closes #1539
Rationale
Some users are finding it annoying that links open a new tab on click when they're just trying to move the selection.
HTML attributes allow for slight customization for link rendering. It's the best we can do atm, but really more of a stopgap solution as consumers should ideally be able to override the default link rendering with whatever they want.
Changes
See above.
Impact
N/A
Testing
N/A (example needed?)
Screenshots/Video
N/A
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation