Conversation
✅ No New Circular DependenciesNo new circular dependencies detected. Current count: 0 |
📦 Alpha Package Version PublishedUse Use |
🔍 Visual review for your branch is published 🔍Here are the links to: |
Coverage Report for packages/react
File Coverage
|
||||||||||||||||||||||||||||||||||||||
xavikp
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes a RichTextEditor @mention regression where inserting the 4th mention in the same paragraph could replace an existing mention, by making insertion rely on TipTap’s tracked suggestion range and by adding regressions to prevent future breakage.
Changes:
- Switch mention insertion to
insertContentAt(range, ...)using the latest suggestion props/range for deterministic behavior. - Add regression tests covering 4+ mentions in the same paragraph and mentions across paragraphs/consecutive mentions.
- Minor import re-order in an existing RichTextEditor test.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/react/src/components/RichText/CoreEditor/Extensions/Mention/suggestion.tsx | Updates mention suggestion insertion logic to use the latest suggestion range and an atomic chain. |
| packages/react/src/components/RichText/CoreEditor/Extensions/Mention/suggestion.test.tsx | Adds unit coverage ensuring the mention insertion uses the latest range/editor state. |
| packages/react/src/components/RichText/RichTextEditor/mentions.test.tsx | Adds integration regressions for multiple mentions, including the “4th mention” scenario. |
| packages/react/src/components/RichText/RichTextEditor/index.spec.tsx | Minor import reordering in an existing test file. |
| }) | ||
|
|
||
| import { RichTextEditor } from "./index" | ||
| import React from "react" |
There was a problem hiding this comment.
import React from "react" is unused in this test file (JSX runtime doesn't require it here). With oxlint no-unused-vars enabled, this will fail lint; remove the import or use it for a referenced type/value.
| import React from "react" |
| import { waitFor } from "@testing-library/react" | ||
| import { afterEach, describe, expect, it, vi } from "vitest" | ||
|
|
||
| import { screen, userEvent, zeroRender } from "../../../testing/test-utils" |
There was a problem hiding this comment.
The test utilities are imported via a long relative path. The repo consistently uses the @/ alias for internal imports (e.g. @/testing/test-utils), which is less brittle if folders move.
| import { screen, userEvent, zeroRender } from "../../../testing/test-utils" | |
| import { screen, userEvent, zeroRender } from "@/testing/test-utils" |
|
|
||
| const editor = document.querySelector(".ProseMirror") as HTMLElement | ||
|
|
There was a problem hiding this comment.
document.querySelector(".ProseMirror") as HTMLElement will throw a confusing runtime error if the editor isn't rendered. Prefer asserting the element exists (or querying via Testing Library) before using it, to make failures actionable.
| import { render, screen } from "@testing-library/react" | ||
| import userEvent from "@testing-library/user-event" | ||
| import { expect, test, vi } from "vitest" |
There was a problem hiding this comment.
This test uses render/userEvent directly from Testing Library. In packages/react, the established convention is to use zeroRender (and the preconfigured userEvent) from @/testing/test-utils for consistent providers/setup.
| let component: ReactRenderer | null = null | ||
| let popoverRoot: Root | null = null | ||
| let container: HTMLDivElement | null = null | ||
| let latestProps: SuggestionRenderProps | null = null | ||
|
|
There was a problem hiding this comment.
latestProps is retained in a closure and never cleared. Consider resetting it in onExit (and/or when handling Escape) to avoid holding stale editor references longer than needed.
Description
Screenshots (if applicable)
[Link to Figma Design](Figma URL here)
Implementation details