fix: Clicking comment overlapping link opens link (BLO-1091)#2696
Merged
matthewlipski merged 6 commits intomainfrom May 1, 2026
Merged
fix: Clicking comment overlapping link opens link (BLO-1091)#2696matthewlipski merged 6 commits intomainfrom
matthewlipski merged 6 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCommentsExtension now runs before the link extension and registers Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant CommentsPlugin as Comments Plugin
participant Browser
User->>Editor: Left-click inline link mark
Editor->>CommentsPlugin: handleClick(event, pos, mark)
alt non-left click or no relevant node
CommentsPlugin-->>Editor: return false
Editor-->>Browser: allow default behavior (navigation)
else click on comment mark
CommentsPlugin->>CommentsPlugin: if threadId == selectedThreadId\nreturn false (no state change)
alt clicked different thread
CommentsPlugin->>CommentsPlugin: set selectedThreadId
CommentsPlugin-->>Editor: return true (event handled)
Editor-->>User: comment selected (prevent navigation)
end
Note right of Editor: second click / unhandled -> Browser navigation (popup)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 6/8 reviews remaining, refill in 11 minutes and 29 seconds.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/comments/comments.test.ts (1)
64-66: ⚡ Quick winReplace fixed sleeps with condition-based waits to reduce flakiness.
waitForTimeout(500)here can make the test brittle; Playwright assertions already auto-wait on DOM state.Proposed refactor
- await page.keyboard.press("ArrowDown"); - await page.waitForTimeout(500); - await expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0); + await page.keyboard.press("ArrowDown"); + await expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0); const link = page.locator('a[data-inline-content-type="link"]').first(); // First click selects the thread without navigating. await link.click(); - await page.waitForTimeout(500); await expect(page.locator(".bn-thread-mark-selected")).toBeVisible();Also applies to: 71-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/comments/comments.test.ts` around lines 64 - 66, After sending the ArrowDown key, remove the fixed sleep (page.waitForTimeout(500)) and replace it with a condition-based wait: rely on the Playwright auto-waiting assert (expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0)) or explicitly wait for the selector to be detached (page.waitForSelector(".bn-thread-mark-selected", { state: "detached" })); apply the same change to the similar block referenced at lines 71-74 so tests no longer use fixed sleeps and instead wait for the DOM condition.
🤖 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/comments/comments.test.ts`:
- Around line 64-66: After sending the ArrowDown key, remove the fixed sleep
(page.waitForTimeout(500)) and replace it with a condition-based wait: rely on
the Playwright auto-waiting assert
(expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0)) or explicitly
wait for the selector to be detached
(page.waitForSelector(".bn-thread-mark-selected", { state: "detached" })); apply
the same change to the similar block referenced at lines 71-74 so tests no
longer use fixed sleeps and instead wait for the DOM condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c231b05f-8164-45d8-a796-250f500cde39
📒 Files selected for processing (2)
packages/core/src/comments/extension.tstests/src/end-to-end/comments/comments.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: |
nperez0111
reviewed
May 1, 2026
nperez0111
reviewed
May 1, 2026
…e` instead of `priority`
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/src/end-to-end/comments/comments.test.ts (1)
64-73: ⚡ Quick winDrop the fixed 500 ms waits here.
These sleeps make the test slower and more timing-sensitive in CI. The
expect(...)calls already auto-wait for the selection state, so you can wait on the DOM signal directly instead of pausing twice.Suggested cleanup
await page.keyboard.press("ArrowDown"); - await page.waitForTimeout(500); await expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0); const link = page.locator('a[data-inline-content-type="link"]').first(); // First click selects the thread without navigating. await link.click(); - await page.waitForTimeout(500); await expect(page.locator(".bn-thread-mark-selected")).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/comments/comments.test.ts` around lines 64 - 73, Remove the fixed 500ms sleeps and rely on Playwright's auto-waiting assertions: delete the two await page.waitForTimeout(500) calls around the keyboard press and link.click, keep the existing awaits for expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0) and await expect(page.locator(".bn-thread-mark-selected")).toBeVisible() so the test waits on the DOM signal instead of sleeping; ensure you still call await link.click() and await page.keyboard.press("ArrowDown") as shown.
🤖 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/comments/extension.ts`:
- Line 228: Remove the debug console.log("comments") call that is firing on
every click in the comments plugin; locate the plain console.log("comments")
statement in the comments extension code (the comments plugin/extension
initialization or click handler) and delete it (or replace with a conditional
logger call such as processLogger.debug or a no-op) so browser and test output
are no longer spammed.
- Around line 249-264: The click handler currently treats a missing commentMark
as threadId === undefined and proceeds to clear selectedThreadId and return
true; change it so that if commentMark is falsy you immediately return false (do
not call store.setState or modify selectedThreadId), and only run the existing
logic (compare threadId to store.state.selectedThreadId, call store.setState to
update selectedThreadId, and return true) when commentMark exists and provides a
threadId; reference commentMark, threadId, store.setState, and selectedThreadId
to locate the code to adjust.
---
Nitpick comments:
In `@tests/src/end-to-end/comments/comments.test.ts`:
- Around line 64-73: Remove the fixed 500ms sleeps and rely on Playwright's
auto-waiting assertions: delete the two await page.waitForTimeout(500) calls
around the keyboard press and link.click, keep the existing awaits for
expect(page.locator(".bn-thread-mark-selected")).toHaveCount(0) and await
expect(page.locator(".bn-thread-mark-selected")).toBeVisible() so the test waits
on the DOM signal instead of sleeping; ensure you still call await link.click()
and await page.keyboard.press("ArrowDown") as shown.
🪄 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: 01d76749-11b6-49c8-846a-e1908f4a06e4
📒 Files selected for processing (5)
packages/core/src/comments/extension.tspackages/core/src/editor/managers/ExtensionManager/extensions.tspackages/core/src/extensions/tiptap-extensions/Link/index.tspackages/core/src/extensions/tiptap-extensions/Link/link.tstests/src/end-to-end/comments/comments.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/extensions/tiptap-extensions/Link/index.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/comments/extension.ts (1)
261-274:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard missing
threadIdbefore consuming the click.
threadIdis force-cast on Line 261, but this file already handles thread ids as optional elsewhere. If a malformed mark lacksthreadId, this path can still update state and consume the click unexpectedly. Add a runtime guard and returnfalsewhenthreadIdis absent.Suggested patch
- const threadId = commentMark.attrs.threadId as string; + const threadId = commentMark.attrs.threadId as + | string + | undefined; + + if (!threadId) { + if (store.state.selectedThreadId !== undefined) { + store.setState((prev) => ({ + ...prev, + selectedThreadId: undefined, + })); + } + return false; + } // If the clicked thread is already selected, do nothing and let // other handlers process the event (e.g. navigating a link). if (threadId === store.state.selectedThreadId) { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/comments/extension.ts` around lines 261 - 274, The click handler currently force-casts commentMark.attrs.threadId to threadId and may consume the click even when missing; add a runtime guard that checks whether commentMark.attrs.threadId is present (e.g., falsy/undefined) before proceeding, and return false immediately if it's absent; only then compare with store.state.selectedThreadId and call store.setState to update selectedThreadId when a valid threadId exists.
🤖 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/comments/extension.ts`:
- Around line 261-274: The click handler currently force-casts
commentMark.attrs.threadId to threadId and may consume the click even when
missing; add a runtime guard that checks whether commentMark.attrs.threadId is
present (e.g., falsy/undefined) before proceeding, and return false immediately
if it's absent; only then compare with store.state.selectedThreadId and call
store.setState to update selectedThreadId when a valid threadId exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55191038-7cc9-43e4-96ef-b833f5df4b24
📒 Files selected for processing (1)
packages/core/src/comments/extension.ts
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
This PR makes it so that when clicking a comment overlapping a link, the first click only shows the comment. The second click then opens the link.
Closes #2572
Rationale
This is better for UX.
Changes
Impact
N/A
Testing
Added e2e test.
Screenshots/Video
N/A
Checklist
Additional Notes
N/A
Summary by CodeRabbit
Bug Fixes
Tests
New Features
Refactor