fix: add link on commented text removes the comment highlight (2573)#2592
fix: add link on commented text removes the comment highlight (2573)#2592miadnguyen wants to merge 2 commits intoTypeCellOS:mainfrom
Conversation
|
@miadnguyen is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe LinkToolbar Changes
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 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 |
miadnguyen
force-pushed
the
fix/preserve-comment-mark-on-link-edit
branch
from
3668c41 to
9da3e59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/src/end-to-end/linktoolbar/linktoolbar.test.ts (1)
33-44: Replace hard-coded timeouts with locator-based waits to improve test reliability.Lines 32 and 43 use
waitForTimeoutwhich introduces brittle, timing-dependent behavior. Use explicit visibility assertions instead to ensure deterministic waits aligned with actual UI state changes.♻️ Suggested refactor
await page.keyboard.press("ArrowLeft"); - await page.waitForTimeout(500); + await expect(page.getByRole("button", { name: "Edit link" })).toBeVisible(); await page.keyboard.press("Enter"); - await page.waitForTimeout(300); + await expect(page.locator('a[href="https://example2.com"]')).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/linktoolbar/linktoolbar.test.ts` around lines 33 - 44, Replace the two hard-coded waitForTimeout calls with deterministic locator-based waits: remove await page.waitForTimeout(500) and instead wait for the "Edit link" locator to become visible using page.getByText("Edit link").waitFor({ state: "visible" }) (or expect(page.getByText("Edit link")).toBeVisible() if using `@playwright/test`) before clicking; after pressing Enter, remove await page.waitForTimeout(300) and wait for a concrete UI change such as the edit dialog closing or the updated link text appearing (e.g., await page.getByText("https://example2.com").waitFor({ state: "visible" }) or waiting for the edit dialog locator to be hidden) so the test no longer relies on fixed timeouts.
🤖 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/linktoolbar/linktoolbar.test.ts`:
- Around line 11-49: Add a focused regression test that mirrors "Should preserve
existing marks when editing a link" but creates a comment mark first: use
focusOnEditor to focus, type/select text, invoke the comment tooling (the
comment button/flow used in your app) to add a comment annotation, then add/edit
a link via LINK_BUTTON_SELECTOR and the existing editButton flow; finally assert
the comment highlight/association still exists (use the comment-specific locator
you have for comment highlights/associations, analogous to how boldText uses
"strong a, a strong") to ensure the comment mark is preserved after link
insert/edit.
- Around line 47-48: The current assertion using the broad selector assigned to
boldText (page.locator("strong a, a strong")) may match unrelated items; tighten
it to specifically target the edited link and assert both its href equals
"https://example2.com" and that it remains bold and linked. Update the locator
used for boldText to target the exact link (for example by link text or a more
specific ancestor/attribute) and add two expectations: one that the locator's
href/attribute equals "https://example2.com" and another that verifies the
element is wrapped in/has the strong styling (preserving bold formatting) using
the same locator and expect calls.
---
Nitpick comments:
In `@tests/src/end-to-end/linktoolbar/linktoolbar.test.ts`:
- Around line 33-44: Replace the two hard-coded waitForTimeout calls with
deterministic locator-based waits: remove await page.waitForTimeout(500) and
instead wait for the "Edit link" locator to become visible using
page.getByText("Edit link").waitFor({ state: "visible" }) (or
expect(page.getByText("Edit link")).toBeVisible() if using `@playwright/test`)
before clicking; after pressing Enter, remove await page.waitForTimeout(300) and
wait for a concrete UI change such as the edit dialog closing or the updated
link text appearing (e.g., await
page.getByText("https://example2.com").waitFor({ state: "visible" }) or waiting
for the edit dialog locator to be hidden) so the test no longer relies on fixed
timeouts.
🪄 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: ef0e1404-adec-46b7-a204-1e9b0ddd69e2
📒 Files selected for processing (1)
tests/src/end-to-end/linktoolbar/linktoolbar.test.ts
| test("Should preserve existing marks when editing a link", async ({ | ||
| page, | ||
| }) => { | ||
| await focusOnEditor(page); | ||
|
|
||
| // Type bold text | ||
| await page.keyboard.type("hello"); | ||
| await page.keyboard.press("Shift+Home"); | ||
|
|
||
| // Make it bold via formatting toolbar | ||
| await page.waitForSelector(`[data-test="bold"]`); | ||
| await page.click(`[data-test="bold"]`); | ||
|
|
||
| // Add link | ||
| await page.keyboard.press("Shift+Home"); | ||
| await page.waitForSelector(LINK_BUTTON_SELECTOR); | ||
| await page.click(LINK_BUTTON_SELECTOR); | ||
| await page.keyboard.type("https://example.com"); | ||
| await page.keyboard.press("Enter"); | ||
|
|
||
| // Move cursor back onto the linked text to trigger link toolbar | ||
| await page.keyboard.press("ArrowLeft"); | ||
| await page.waitForTimeout(500); | ||
|
|
||
| // Click Edit link button | ||
| const editButton = page.getByText("Edit link"); | ||
| await editButton.waitFor({ state: "visible" }); | ||
| await editButton.click(); | ||
|
|
||
| await page.keyboard.press("Control+A"); | ||
| await page.keyboard.type("https://example2.com"); | ||
| await page.keyboard.press("Enter"); | ||
|
|
||
| await page.waitForTimeout(300); | ||
|
|
||
| // Verify bold mark is still present on the text | ||
| const boldText = page.locator("strong a, a strong"); | ||
| await expect(boldText).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Regression test does not cover the reported comment-mark bug.
Line 11 onward validates bold preservation, but issue #2573 is specifically about preserving the comment mark (highlight + association/accessibility) when adding/editing a link. This can pass while the original bug still regresses.
Please add a scenario that creates commented text first, then adds/edits a link and verifies the comment highlight/association remains.
🧰 Tools
🪛 ESLint
[error] 36-36: Avoid destructuring queries from render result, use screen.getByText instead
(testing-library/prefer-screen-queries)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/end-to-end/linktoolbar/linktoolbar.test.ts` around lines 11 - 49,
Add a focused regression test that mirrors "Should preserve existing marks when
editing a link" but creates a comment mark first: use focusOnEditor to focus,
type/select text, invoke the comment tooling (the comment button/flow used in
your app) to add a comment annotation, then add/edit a link via
LINK_BUTTON_SELECTOR and the existing editButton flow; finally assert the
comment highlight/association still exists (use the comment-specific locator you
have for comment highlights/associations, analogous to how boldText uses "strong
a, a strong") to ensure the comment mark is preserved after link insert/edit.
| const boldText = page.locator("strong a, a strong"); | ||
| await expect(boldText).toBeVisible(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Playwright tests, what are robust selector patterns to assert nested formatted text relationships (e.g., bold + link on the same node) without broad false-positive CSS matches?
💡 Result:
Robust selector patterns in Playwright for asserting nested formatted text relationships (e.g., bold + link on the same node) prioritize precise, structure-aware locators over broad CSS to avoid false positives. Key patterns from official docs and best practices: 1. Use CSS :has pseudo-class for parent-child relationships: e.g., page.locator('p:has(strong):has(a)') selects paragraphs containing both bold () and link () children. Combine with :has-text for content: 'p:has-text("Important"):has(strong a)'. 2. Chain locators with filter: Start with a container, filter by nested elements. e.g.: const container = page.getByRole('paragraph'); const formatted = container.filter({ has: page.locator('strong:has(a)') }); await expect(formatted).toBeVisible; 3. Locator chaining with >> : e.g., 'article >> strong >> a' for bold containing link, or '*article >> strong >> a' to keep the article. Use :text or :text-is for precise text matching within: 'article :text("Click"):has(a)'. 4. Assertions for formatting: Use expect(locator).toHaveCSS('font-weight', 'bold') on links or text nodes to verify styles without class dependencies. For structure, expect(locator).toContainText or .toHaveText with regex, combined with .locator('strong').toBeVisible. 5. Best practices to avoid false positives: - Prefer role/text locators over pure CSS: page.getByRole('link', { name: /regex/ }).locator('..').locator('strong'). - Add data-testid to formatted containers: '[data-testid="bold-link"]'. - Use :visible and nth-match for specificity: 'strong:visible:nth-match(1):has(a)'. - Avoid deep CSS chains or brittle classes; test user-visible structure. These ensure assertions pass only when exact nested formatting exists, resilient to style/CSS changes.
Citations:
- 1: https://playwright.dev/docs/next/other-locators
- 2: https://playwright.dev/docs/other-locators
- 3: https://playwright.dev/docs/test-assertions
- 4: https://playwright.dev/docs/best-practices
- 5: https://playwright.dev/docs/api/class-locatorassertions
- 6: http://playwright.dev/docs/api/class-locator
🏁 Script executed:
wc -l tests/src/end-to-end/linktoolbar/linktoolbar.test.tsRepository: TypeCellOS/BlockNote
Length of output: 119
🏁 Script executed:
sed -n '1,70p' tests/src/end-to-end/linktoolbar/linktoolbar.test.tsRepository: TypeCellOS/BlockNote
Length of output: 1691
🏁 Script executed:
rg ':has\(' tests/ -A 2 -B 2 | head -50Repository: TypeCellOS/BlockNote
Length of output: 46
🏁 Script executed:
rg 'hasText|:has-text' tests/ -A 2 -B 2 | head -50Repository: TypeCellOS/BlockNote
Length of output: 431
🏁 Script executed:
rg 'page\.locator\(' tests/src/end-to-end/ -A 2 | head -80Repository: TypeCellOS/BlockNote
Length of output: 5820
🏁 Script executed:
rg 'page\.locator\([^,]+,\s*{' tests/src/end-to-end/ -A 3Repository: TypeCellOS/BlockNote
Length of output: 177
🏁 Script executed:
rg 'page\.locator.*hasText' tests/src/end-to-end/ -A 2Repository: TypeCellOS/BlockNote
Length of output: 46
🏁 Script executed:
rg 'locator\([^)]+\{' tests/src/end-to-end/basics/ -A 2 | head -30Repository: TypeCellOS/BlockNote
Length of output: 304
Tighten the assertion to verify both the updated URL and preserved formatting.
The selector strong a, a strong is too broad and could match unrelated bold links on the page. It should verify that the edited link has the new URL (https://example2.com) and the text remains bold+linked.
✅ More specific assertion
- const boldText = page.locator("strong a, a strong");
- await expect(boldText).toBeVisible();
+ const updatedLink = page.locator('a[href="https://example2.com"]', {
+ hasText: "hello",
+ });
+ await expect(updatedLink).toBeVisible();
+
+ const boldLinkedText = page.locator("strong").filter({
+ has: page.locator('a[href="https://example2.com"]'),
+ }).filter({ hasText: "hello" });
+ await expect(boldLinkedText).toBeVisible();📝 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 boldText = page.locator("strong a, a strong"); | |
| await expect(boldText).toBeVisible(); | |
| const updatedLink = page.locator('a[href="https://example2.com"]', { | |
| hasText: "hello", | |
| }); | |
| await expect(updatedLink).toBeVisible(); | |
| const boldLinkedText = page.locator("strong").filter({ | |
| has: page.locator('a[href="https://example2.com"]'), | |
| }).filter({ hasText: "hello" }); | |
| await expect(boldLinkedText).toBeVisible(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/src/end-to-end/linktoolbar/linktoolbar.test.ts` around lines 47 - 48,
The current assertion using the broad selector assigned to boldText
(page.locator("strong a, a strong")) may match unrelated items; tighten it to
specifically target the edited link and assert both its href equals
"https://example2.com" and that it remains bold and linked. Update the locator
used for boldText to target the exact link (for example by link text or a more
specific ancestor/attribute) and add two expectations: one that the locator's
href/attribute equals "https://example2.com" and another that verifies the
element is wrapped in/has the strong styling (preserving bold formatting) using
the same locator and expect calls.
1 participant
Summary
This pull request fixes an issue where adding or editing a link on commented text removes the comment highlight and comment association. After this change, a link can be added or edited on commented text while preserving both the link underline and the existing comment behavior.
Rationale
The issue was caused by
LinkToolbarExtension.editLink()(from packages/core/src/extensions/LinkToolbar/LinkToolbar.ts) replacing the selected text withinsertText(...)and then reapplying only thelinkmark. As a result, existing marks on that text, including thecommentmark, were lost. Preserving existing non-link marks makes link editing behave correctly and keeps comment functionality intact.Changes
LinkToolbarExtension.editLink()to preserve existing marks on the selected text before replacing itinsertText(...)linkmark after restoring the preserved marksImpact
This change affects the link editing flow for text that already contains other marks, especially comments. It should improve behavior by preserving comment highlighting and comment accessibility when links are added or edited. The intended behavior for normal link editing remains the same.
Testing
tests/src/end-to-end/linktoolbar/linktoolbar.test.tsverifying that bold marks are preserved when editing a link
Screenshots/Video
Below is an image showing commented text retaining the comment, highlight, and link after the fix.
Checklist
Additional Notes
Fixes #2573
Summary by CodeRabbit
Bug Fixes
Tests