From cb9c5dda598fd2b49860e606cf1f001b9d947baf Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 20:12:22 +0300 Subject: [PATCH 1/2] perf(comment): static comment renderer + lazy editors + memoized list (#340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment panel lagged for seconds on open and stuttered on every resolve/apply with many comments (real case: 30 open + 326 resolved ≈ 356 threads), because each comment body mounted a full TipTap/ProseMirror editor, both tabs mounted at once, and any mutation re-rendered the whole list. - CommentContentView: static recursive renderer of comment ProseMirror JSON (no editor instance) for the read-only body — supports exactly CommentEditor's node set (doc/paragraph/text/hardBreak/mention) + marks (bold/italic/strike/code/ link), reproducing the 3-level DOM nesting for pixel-identical CSS. Unknown node/mark or unparseable content degrades that one comment to the read-only CommentEditor; legacy non-JSON strings render as plain text. SECURITY: link hrefs are protocol-allowlisted (safeHref, mirroring @tiptap/extension-link) so a stored comment with a `javascript:`/`data:` href cannot XSS — the old TipTap read-only path sanitized this; the static renderer must too. Control-char smuggling (java\tscript:) is stripped before the check. - MentionContent extracted from MentionView, shared by the TipTap NodeView and the static renderer (identical user/page-mention behavior). - keepMounted={false} on the tabs: the inactive tab no longer mounts its editors. - Lazy reply editor: a stub until click/focus, then the real editor (kept mounted so the draft survives thread re-renders). - React.memo(CommentListItem) + a childrenByParent map (replaces the per-thread O(n^2) filter) + localized reply-send pending state: resolve/apply/reply now re-render only the touched thread. - Progressive first paint: useCommentsQuery no longer blocks on hasNextPage. Gate: client comment+mention suites 22/22 passed, tsc --noEmit 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../components/comment-content-view.test.tsx | 241 ++++++++++++++++++ .../components/comment-content-view.tsx | 194 ++++++++++++++ .../components/comment-list-item.test.tsx | 32 +++ .../comment/components/comment-list-item.tsx | 22 +- .../components/comment-list-with-tabs.tsx | 111 +++++--- .../features/comment/queries/comment-query.ts | 5 +- .../components/mention/mention-view.tsx | 26 +- 7 files changed, 583 insertions(+), 48 deletions(-) create mode 100644 apps/client/src/features/comment/components/comment-content-view.test.tsx create mode 100644 apps/client/src/features/comment/components/comment-content-view.tsx diff --git a/apps/client/src/features/comment/components/comment-content-view.test.tsx b/apps/client/src/features/comment/components/comment-content-view.test.tsx new file mode 100644 index 00000000..597c60d0 --- /dev/null +++ b/apps/client/src/features/comment/components/comment-content-view.test.tsx @@ -0,0 +1,241 @@ +import { describe, it, expect, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import { MemoryRouter } from "react-router-dom"; + +// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts. + +// The fallback path renders the full TipTap editor; stub it so we can assert the +// safety valve fired without pulling in the editor stack. +vi.mock("@/features/comment/components/comment-editor", () => ({ + default: () =>
, +})); + +// Mention rendering hits react-query; stub the page/share queries so the mention +// case renders in isolation. +vi.mock("@/features/page/queries/page-query.ts", () => ({ + usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }), +})); +vi.mock("@/features/share/queries/share-query.ts", () => ({ + useSharePageQuery: () => ({ data: undefined }), +})); + +import { CommentContentView } from "./comment-content-view"; + +function renderView(content: string | object) { + return render( + + + + + , + ); +} + +const doc = (content: any[]) => JSON.stringify({ type: "doc", content }); +const para = (content: any[]) => ({ type: "paragraph", content }); +const text = (t: string, marks?: any[]) => ({ type: "text", text: t, marks }); + +describe("CommentContentView", () => { + it("renders paragraphs as

with text", () => { + const { container } = renderView(doc([para([text("Hello world")])])); + expect(screen.getByText("Hello world")).toBeDefined(); + expect(container.querySelector("p")).not.toBeNull(); + }); + + it("reproduces the read-only CommentEditor DOM nesting for CSS parity", () => { + const { container } = renderView(doc([para([text("x")])])); + // outer .commentEditor > .ProseMirror (module) > .ProseMirror (global) > p + const globalPm = container.querySelector("div.ProseMirror > p"); + expect(globalPm).not.toBeNull(); + }); + + it("renders the bold mark as ", () => { + const { container } = renderView( + doc([para([text("bold", [{ type: "bold" }])])]), + ); + const el = container.querySelector("strong"); + expect(el?.textContent).toBe("bold"); + }); + + it("renders the italic mark as ", () => { + const { container } = renderView( + doc([para([text("it", [{ type: "italic" }])])]), + ); + expect(container.querySelector("em")?.textContent).toBe("it"); + }); + + it("renders the strike mark as ", () => { + const { container } = renderView( + doc([para([text("st", [{ type: "strike" }])])]), + ); + expect(container.querySelector("s")?.textContent).toBe("st"); + }); + + it("renders the code mark as ", () => { + const { container } = renderView( + doc([para([text("co", [{ type: "code" }])])]), + ); + expect(container.querySelector("code")?.textContent).toBe("co"); + }); + + it("renders the link mark as an anchor with safe rel/target", () => { + const { container } = renderView( + doc([ + para([ + text("click", [ + { type: "link", attrs: { href: "https://example.com" } }, + ]), + ]), + ]), + ); + const a = container.querySelector("a"); + expect(a?.getAttribute("href")).toBe("https://example.com"); + expect(a?.getAttribute("target")).toBe("_blank"); + expect(a?.getAttribute("rel")).toBe("noopener noreferrer nofollow"); + expect(a?.textContent).toBe("click"); + }); + + it("neutralizes a javascript: link href (stored XSS) while keeping the text", () => { + const { container } = renderView( + doc([ + para([ + text("click", [ + { type: "link", attrs: { href: "javascript:alert(1)" } }, + ]), + ]), + ]), + ); + const a = container.querySelector("a"); + expect(a).not.toBeNull(); + // No navigable javascript: href — attribute is absent (or empty). + expect(a?.getAttribute("href")).toBeFalsy(); + // The link text is still rendered. + expect(a?.textContent).toBe("click"); + }); + + it("neutralizes a control-char-obfuscated javascript: href", () => { + const { container } = renderView( + doc([ + para([ + text("x", [ + { type: "link", attrs: { href: "java\tscript:alert(1)" } }, + ]), + ]), + ]), + ); + expect(container.querySelector("a")?.getAttribute("href")).toBeFalsy(); + }); + + it("neutralizes a data: link href", () => { + const { container } = renderView( + doc([ + para([ + text("x", [ + { + type: "link", + attrs: { href: "data:text/html," }, + }, + ]), + ]), + ]), + ); + expect(container.querySelector("a")?.getAttribute("href")).toBeFalsy(); + }); + + it("preserves a mailto: link href (allowlisted scheme)", () => { + const { container } = renderView( + doc([ + para([ + text("mail", [ + { type: "link", attrs: { href: "mailto:a@b.com" } }, + ]), + ]), + ]), + ); + expect(container.querySelector("a")?.getAttribute("href")).toBe( + "mailto:a@b.com", + ); + }); + + it("preserves a relative link href (no scheme, not a script vector)", () => { + const { container } = renderView( + doc([ + para([ + text("rel", [{ type: "link", attrs: { href: "/some/path" } }]), + ]), + ]), + ); + expect(container.querySelector("a")?.getAttribute("href")).toBe( + "/some/path", + ); + }); + + it("nests multiple marks on one text node", () => { + const { container } = renderView( + doc([para([text("x", [{ type: "bold" }, { type: "italic" }])])]), + ); + // bold wraps italic (or vice versa) — both elements exist around the text. + expect(container.querySelector("strong")).not.toBeNull(); + expect(container.querySelector("em")).not.toBeNull(); + expect(screen.getByText("x")).toBeDefined(); + }); + + it("renders hardBreak as
", () => { + const { container } = renderView( + doc([para([text("a"), { type: "hardBreak" }, text("b")])]), + ); + expect(container.querySelector("br")).not.toBeNull(); + }); + + it("renders a user mention as a styled span", () => { + const { container } = renderView( + doc([ + para([ + { + type: "mention", + attrs: { label: "Alice", entityType: "user", entityId: "u1" }, + }, + ]), + ]), + ); + expect(screen.getByText("@Alice")).toBeDefined(); + // No fallback to the editor. + expect(screen.queryByTestId("comment-editor-fallback")).toBeNull(); + }); + + it("renders a page mention as a link", () => { + const { container } = renderView( + doc([ + para([ + { + type: "mention", + attrs: { + label: "Some Page", + entityType: "page", + slugId: "pg1", + }, + }, + ]), + ]), + ); + expect(container.querySelector("a")).not.toBeNull(); + expect(screen.getByText("Some Page")).toBeDefined(); + }); + + it("renders a legacy plain-text (non-JSON) string as plain text", () => { + renderView("just a legacy string"); + expect(screen.getByText("just a legacy string")).toBeDefined(); + expect(screen.queryByTestId("comment-editor-fallback")).toBeNull(); + }); + + it("falls back to CommentEditor for an unknown node type", () => { + renderView(doc([{ type: "codeBlock", content: [text("x")] }])); + expect(screen.getByTestId("comment-editor-fallback")).toBeDefined(); + }); + + it("falls back to CommentEditor for malformed JSON", () => { + renderView('{"type":"doc","content":['); + expect(screen.getByTestId("comment-editor-fallback")).toBeDefined(); + }); +}); diff --git a/apps/client/src/features/comment/components/comment-content-view.tsx b/apps/client/src/features/comment/components/comment-content-view.tsx new file mode 100644 index 00000000..7e6feea9 --- /dev/null +++ b/apps/client/src/features/comment/components/comment-content-view.tsx @@ -0,0 +1,194 @@ +import React from "react"; +import classes from "./comment.module.css"; +import { MentionContent } from "@/features/editor/components/mention/mention-view"; +import CommentEditor from "@/features/comment/components/comment-editor"; + +// Static, editor-free renderer of a comment body (ProseMirror JSON). It walks the +// document and emits plain DOM, avoiding the cost of a full TipTap/ProseMirror +// instance per comment (the panel used to spin up 400+ editors on mount). +// +// The supported node/mark set MUST mirror what CommentEditor enables +// (StarterKit + Mention + LinkExtension). Anything outside that set makes the +// whole comment degrade to the read-only CommentEditor via the fallback below, +// so we never show a half-rendered comment. + +// Sentinel thrown when we hit a node/mark we don't know how to render statically. +// Caught at the top level to trigger the CommentEditor fallback for the whole comment. +class UnknownNodeError extends Error {} + +// Protocol allowlist mirroring @tiptap/extension-link's default (the read-only +// CommentEditor path relies on it to blank javascript:/data: hrefs). The static +// renderer must apply the SAME sanitization because the backend stores comment +// content verbatim and React does not neutralize javascript: in an href. +const ALLOWED_URI_SCHEMES = /^(?:https?|ftps?|mailto|tel|callto|sms|cid|xmpp):/i; + +function safeHref(href: unknown): string | undefined { + if (typeof href !== "string") return undefined; + // Strip control chars/whitespace that could smuggle a scheme past the test + // (e.g. "java\tscript:"). + const cleaned = href.replace(/[\u0000-\u0020]/g, "").trim(); + // Allow relative/anchor/protocol-relative links (no scheme) — not script vectors. + if (!/^[a-z][a-z0-9+.-]*:/i.test(cleaned)) return href; + return ALLOWED_URI_SCHEMES.test(cleaned) ? href : undefined; +} + +interface PMMark { + type: string; + attrs?: Record; +} + +interface PMNode { + type: string; + attrs?: Record; + content?: PMNode[]; + text?: string; + marks?: PMMark[]; +} + +// Wrap a text node's string in its marks (marks nest, e.g. bold + italic). +function renderMarks( + text: React.ReactNode, + marks: PMMark[] | undefined, + keyPrefix: string, +): React.ReactNode { + if (!marks || marks.length === 0) return text; + + return marks.reduce((acc, mark, i) => { + const key = `${keyPrefix}-m${i}`; + switch (mark.type) { + case "bold": + return {acc}; + case "italic": + return {acc}; + case "strike": + return {acc}; + case "code": + return {acc}; + case "link": { + // LinkExtension (TiptapLink) opens links in a new tab; keep the same + // safe rel semantics the editor produces. Sanitize the href against the + // extension's protocol allowlist — a disallowed scheme (javascript:, + // data:) yields undefined so the anchor is non-navigable but still shows + // its text, matching how extension-link blanks a bad href. + const href = safeHref(mark.attrs?.href); + return ( + + {acc} + + ); + } + default: + throw new UnknownNodeError(`Unknown mark type: ${mark.type}`); + } + }, text); +} + +function renderNode(node: PMNode, key: string): React.ReactNode { + switch (node.type) { + case "paragraph": + return

{renderChildren(node.content, key)}

; + case "text": + return ( + + {renderMarks(node.text ?? "", node.marks, key)} + + ); + case "hardBreak": + return
; + case "mention": + return ( + + + + ); + default: + throw new UnknownNodeError(`Unknown node type: ${node.type}`); + } +} + +function renderChildren( + content: PMNode[] | undefined, + keyPrefix: string, +): React.ReactNode { + if (!content) return null; + return content.map((child, i) => renderNode(child, `${keyPrefix}-${i}`)); +} + +// Reproduce the exact DOM nesting the read-only CommentEditor renders so the +// scoped CSS in comment.module.css (which targets +// `.commentEditor .ProseMirror :global(.ProseMirror)` and `.ProseMirror p`) +// applies pixel-for-pixel. Read-only => no data-editable / data-surface attrs. +function Shell({ children }: { children: React.ReactNode }) { + return ( +
+
+
{children}
+
+
+ ); +} + +interface CommentContentViewProps { + content: string | object; +} + +export function CommentContentView({ content }: CommentContentViewProps) { + // Degrade this single comment to the old editor-based render (safety valve). + const fallback = () => { + if (import.meta.env.DEV) { + console.warn( + "CommentContentView: unsupported comment content, falling back to editor", + ); + } + return ; + }; + + let doc: unknown = content; + + if (typeof content === "string") { + try { + doc = JSON.parse(content); + } catch { + const trimmed = content.trim(); + // Looks like it was meant to be JSON but is malformed -> safety-valve fallback. + if (trimmed.startsWith("{") || trimmed.startsWith("[")) { + return fallback(); + } + // Otherwise it's a legacy plain-text comment: render as a single paragraph. + return ( + +

{content}

+ + ); + } + } + + // Double-stringified / legacy plain-text stored as a JSON string. + if (typeof doc === "string") { + return ( + +

{doc}

+ + ); + } + + try { + const pmDoc = doc as PMNode; + if (!pmDoc || typeof pmDoc !== "object" || pmDoc.type !== "doc") { + throw new UnknownNodeError("Not a ProseMirror doc"); + } + return {renderChildren(pmDoc.content, "n")}; + } catch (err) { + if (err instanceof UnknownNodeError) { + return fallback(); + } + throw err; + } +} + +export default CommentContentView; diff --git a/apps/client/src/features/comment/components/comment-list-item.test.tsx b/apps/client/src/features/comment/components/comment-list-item.test.tsx index 0a343bf7..f41f952c 100644 --- a/apps/client/src/features/comment/components/comment-list-item.test.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.test.tsx @@ -28,6 +28,16 @@ vi.mock("@/features/comment/components/comment-editor", () => ({ default: () =>
, })); +// CommentContentView (used for the read-only body) imports the mention view, +// which pulls page-query -> main.tsx (createRoot). Stub the queries so the item +// renders in isolation without the app entry side-effect. +vi.mock("@/features/page/queries/page-query.ts", () => ({ + usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }), +})); +vi.mock("@/features/share/queries/share-query.ts", () => ({ + useSharePageQuery: () => ({ data: undefined }), +})); + import CommentListItem from "./comment-list-item"; import { canShowApply, @@ -286,3 +296,25 @@ describe("canShowDismiss predicate", () => { expect(canShowDismiss(c({ parentCommentId: "p" }), true, true)).toBe(false); }); }); + +describe("CommentListItem — read-only body renders statically", () => { + it("renders the comment body as static text without a TipTap editor", () => { + renderItem( + baseComment({ + content: JSON.stringify({ + type: "doc", + content: [ + { + type: "paragraph", + content: [{ type: "text", text: "Hello static world" }], + }, + ], + }), + }), + ); + // Body text is present... + expect(screen.getByText("Hello static world")).toBeDefined(); + // ...and it did NOT go through the (mocked) CommentEditor instance. + expect(screen.queryByTestId("comment-editor")).toBeNull(); + }); +}); diff --git a/apps/client/src/features/comment/components/comment-list-item.tsx b/apps/client/src/features/comment/components/comment-list-item.tsx index aa9b253a..543ed1a1 100644 --- a/apps/client/src/features/comment/components/comment-list-item.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.tsx @@ -1,10 +1,11 @@ import { Group, Text, Box, Badge, Button } from "@mantine/core"; import { AgentAvatarStack } from "@/components/ui/agent-avatar-stack.tsx"; -import React, { useEffect, useMemo, useRef, useState } from "react"; +import React, { useMemo, useRef, useState } from "react"; import classes from "./comment.module.css"; import { useAtom, useAtomValue } from "jotai"; import { useTimeAgo } from "@/hooks/use-time-ago"; import CommentEditor from "@/features/comment/components/comment-editor"; +import CommentContentView from "@/features/comment/components/comment-content-view"; import { pageEditorAtom } from "@/features/editor/atoms/editor-atoms"; import CommentActions from "@/features/comment/components/comment-actions"; import CommentMenu from "@/features/comment/components/comment-menu"; @@ -50,7 +51,6 @@ function CommentListItem({ const [isEditing, setIsEditing] = useState(false); const [isLoading, setIsLoading] = useState(false); const editor = useAtomValue(pageEditorAtom); - const [content, setContent] = useState(comment.content); const editContentRef = useRef(null); const updateCommentMutation = useUpdateCommentMutation(); const deleteCommentMutation = useDeleteCommentMutation(comment.pageId); @@ -78,22 +78,16 @@ function CommentListItem({ const isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId || userSpaceRole === "admin"; - useEffect(() => { - setContent(comment.content); - }, [comment]); async function handleUpdateComment() { try { setIsLoading(true); const commentToUpdate = { commentId: comment.id, - content: JSON.stringify(editContentRef.current ?? content), + content: JSON.stringify(editContentRef.current ?? comment.content), }; await updateCommentMutation.mutateAsync(commentToUpdate); - if (editContentRef.current) { - setContent(editContentRef.current); - editContentRef.current = null; - } + editContentRef.current = null; setIsEditing(false); } catch (error) { console.error("Failed to update comment:", error); @@ -350,11 +344,11 @@ function CommentListItem({ )} {!isEditing ? ( - + ) : ( <> { editContentRef.current = newContent; }} onSave={handleUpdateComment} @@ -374,4 +368,6 @@ function CommentListItem({ ); } -export default CommentListItem; +// Memoized so a resolve/apply/reply cache update (which only replaces the touched +// comment's object identity) re-renders that one thread, not all ~356 items. +export default React.memo(CommentListItem); diff --git a/apps/client/src/features/comment/components/comment-list-with-tabs.tsx b/apps/client/src/features/comment/components/comment-list-with-tabs.tsx index af9d0783..5ee19a3e 100644 --- a/apps/client/src/features/comment/components/comment-list-with-tabs.tsx +++ b/apps/client/src/features/comment/components/comment-list-with-tabs.tsx @@ -23,7 +23,6 @@ import CommentActions from "@/features/comment/components/comment-actions"; import { useFocusWithin } from "@mantine/hooks"; import { IComment } from "@/features/comment/types/comment.types.ts"; import { usePageQuery } from "@/features/page/queries/page-query.ts"; -import { IPagination } from "@/lib/types.ts"; import { extractPageSlugId } from "@/lib"; import { useTranslation } from "react-i18next"; import { useGetSpaceBySlugQuery } from "@/features/space/queries/space-query.ts"; @@ -46,7 +45,9 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { isError, } = useCommentsQuery({ pageId: page?.id }); const createCommentMutation = useCreateCommentMutation(); - const [isLoading, setIsLoading] = useState(false); + // mutateAsync is a stable reference across renders; depend on it (not the + // mutation object) so the reply/comment callbacks stay stable. + const createCommentAsync = createCommentMutation.mutateAsync; const { data: space } = useGetSpaceBySlugQuery(page?.space?.slug); const canEdit = page?.permissions?.canEdit ?? false; @@ -75,13 +76,28 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { return { activeComments: active, resolvedComments: resolved }; }, [comments]); + // Index replies by their parent once, instead of an O(n^2) filter per thread. + // The map ref changes on any comments update, so MemoizedChildComments re-runs + // (cheap) and re-looks-up, while memoized CommentListItems skip unchanged items. + const childrenByParent = useMemo(() => { + const m = new Map(); + for (const c of comments?.items ?? []) { + if (c.parentCommentId) { + const arr = m.get(c.parentCommentId); + if (arr) arr.push(c); + else m.set(c.parentCommentId, [c]); + } + } + return m; + }, [comments?.items]); + const [isPageCommentLoading, setIsPageCommentLoading] = useState(false); const handleAddPageComment = useCallback( async (_commentId: string, content: string) => { try { setIsPageCommentLoading(true); - const createdComment = await createCommentMutation.mutateAsync({ + const createdComment = await createCommentAsync({ pageId: page?.id, content: JSON.stringify(content), }); @@ -100,27 +116,26 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { setIsPageCommentLoading(false); } }, - [createCommentMutation, page?.id], + [createCommentAsync, page?.id], ); const handleAddReply = useCallback( async (commentId: string, content: string) => { + // Pending state lives inside CommentEditorWithActions so sending a reply + // does not churn renderComments and re-render the whole list. try { - setIsLoading(true); const commentData = { pageId: page?.id, parentCommentId: commentId, content: JSON.stringify(content), }; - await createCommentMutation.mutateAsync(commentData); + await createCommentAsync(commentData); } catch (error) { console.error("Failed to post comment:", error); - } finally { - setIsLoading(false); } }, - [createCommentMutation, page?.id], + [createCommentAsync, page?.id], ); const renderComments = useCallback( @@ -143,7 +158,7 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { userSpaceRole={space?.membership?.role} /> )} ), [ - comments, + childrenByParent, handleAddReply, - isLoading, + page?.id, space?.membership?.role, canComment, canEdit, @@ -203,6 +217,9 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { ; + childrenByParent: Map; parentId: string; pageId: string; canComment: boolean; @@ -315,24 +332,18 @@ interface ChildCommentsProps { userSpaceRole?: string; } const ChildComments = ({ - comments, + childrenByParent, parentId, pageId, canComment, canEdit, userSpaceRole, }: ChildCommentsProps) => { - const getChildComments = useCallback( - (parentId: string) => - comments.items.filter( - (comment: IComment) => comment.parentCommentId === parentId, - ), - [comments.items], - ); + const children = childrenByParent.get(parentId) ?? []; return (
- {getChildComments(parentId).map((childComment) => ( + {children.map((childComment) => (
{ + const { t } = useTranslation(); + // Lazily mount the TipTap reply editor: until the user interacts with the + // stub, no editor instance is created for this thread. Once mounted it stays + // mounted so the draft is preserved. + const [mounted, setMounted] = useState(false); const [content, setContent] = useState(""); + const [isSending, setIsSending] = useState(false); const { ref, focused } = useFocusWithin(); const commentEditorRef = useRef(null); - const handleSave = useCallback(() => { - onSave(commentId, content); - setContent(""); - commentEditorRef.current?.clearContent(); + const activate = useCallback(() => setMounted(true), []); + + const handleSave = useCallback(async () => { + try { + setIsSending(true); + await onSave(commentId, content); + setContent(""); + commentEditorRef.current?.clearContent(); + } finally { + setIsSending(false); + } }, [commentId, content, onSave]); + if (!mounted) { + return ( +
{ + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + activate(); + } + }} + style={{ + padding: "6px", + fontSize: "var(--mantine-font-size-sm)", + lineHeight: 1.4, + color: "var(--mantine-color-placeholder)", + cursor: "text", + borderRadius: "var(--mantine-radius-sm)", + }} + > + {placeholder || t("Reply...")} +
+ ); + } + return (
- {focused && } + {focused && }
); }; diff --git a/apps/client/src/features/comment/queries/comment-query.ts b/apps/client/src/features/comment/queries/comment-query.ts index 97783841..6065cbec 100644 --- a/apps/client/src/features/comment/queries/comment-query.ts +++ b/apps/client/src/features/comment/queries/comment-query.ts @@ -53,7 +53,10 @@ export function useCommentsQuery(params: ICommentParams) { return { data, - isLoading: query.isLoading || query.hasNextPage, + // Paint the first page as soon as it arrives instead of blocking until every + // page has loaded; the background effect above keeps streaming the rest + // (tab counts grow as pages arrive). + isLoading: query.isLoading, isError: query.isError, }; } diff --git a/apps/client/src/features/editor/components/mention/mention-view.tsx b/apps/client/src/features/editor/components/mention/mention-view.tsx index 561f3e0f..fa0b6d52 100644 --- a/apps/client/src/features/editor/components/mention/mention-view.tsx +++ b/apps/client/src/features/editor/components/mention/mention-view.tsx @@ -11,9 +11,19 @@ import { import { extractPageSlugId } from "@/lib"; import classes from "./mention.module.css"; -export default function MentionView(props: NodeViewProps) { - const { node } = props; - const { label, entityType, entityId, slugId, anchorId } = node.attrs; +interface MentionAttrs { + label?: string; + entityType?: string; + entityId?: string; + slugId?: string; + anchorId?: string; +} + +// Presentational mention renderer (no NodeViewWrapper). Shared by the editor +// NodeView (MentionView) and the static comment renderer (CommentContentView) +// so mention click/nav/icon behavior stays identical outside of an editor. +export function MentionContent({ attrs }: { attrs: MentionAttrs }) { + const { label, entityType, slugId, anchorId } = attrs; const isPageMention = entityType === "page"; const { spaceSlug, pageSlug } = useParams(); const { shareId } = useParams(); @@ -56,7 +66,7 @@ export default function MentionView(props: NodeViewProps) { }); return ( - + <> {entityType === "user" && ( @{label} @@ -139,6 +149,14 @@ export default function MentionView(props: NodeViewProps) { )} + + ); +} + +export default function MentionView(props: NodeViewProps) { + return ( + + ); } -- 2.52.0 From a4fc6c7f6401718c95c69740ddaf7332f2fce2f7 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 4 Jul 2026 20:48:21 +0300 Subject: [PATCH 2/2] fix(comment): underline mark + draft-surviving tabs + test coverage (#349 review F1-F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - F1: render the `underline` mark statically (StarterKit v3 enables Underline; comment-editor does not disable it) — an underlined comment no longer degrades the whole comment to the read-only editor fallback. renderMarks gains a `case "underline" -> `, mirroring the other marks (+ test). - F2: keep the Open tab panel mounted (`Tabs.Panel value="open" keepMounted`) while the heavy Resolved panel still unmounts (`Tabs keepMounted={false}`). A per-panel keepMounted overrides the parent's `false` (Mantine 8 TabsPanel), so an in-progress reply draft / edit in the Open panel survives an Open->Resolved->Open switch, keeping the micro-opt of not mounting the large Resolved list. - F3: cover edit->save->re-render in comment-list-item.test.tsx — save calls mutateAsync with JSON.stringify(editContentRef) and a new comment.content prop updates the visible body; cancel restores the static body without mutating; clearing editContentRef after cancel. - F4: extract childrenByParent grouping into an exported pure `buildChildrenByParent(items)` (unit-tested: nesting, orphan reply, sibling order) + new comment-list-with-tabs.test.tsx covering the lazy reply-editor activation (stub -> click/focus/Enter mounts the editor). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../components/comment-content-view.test.tsx | 9 ++ .../components/comment-content-view.tsx | 5 + .../components/comment-list-item.test.tsx | 152 +++++++++++++++++- .../comment-list-with-tabs.test.tsx | 108 +++++++++++++ .../components/comment-list-with-tabs.tsx | 46 ++++-- 5 files changed, 299 insertions(+), 21 deletions(-) create mode 100644 apps/client/src/features/comment/components/comment-list-with-tabs.test.tsx diff --git a/apps/client/src/features/comment/components/comment-content-view.test.tsx b/apps/client/src/features/comment/components/comment-content-view.test.tsx index 597c60d0..96e01961 100644 --- a/apps/client/src/features/comment/components/comment-content-view.test.tsx +++ b/apps/client/src/features/comment/components/comment-content-view.test.tsx @@ -72,6 +72,15 @@ describe("CommentContentView", () => { expect(container.querySelector("s")?.textContent).toBe("st"); }); + it("renders the underline mark as (not the editor fallback)", () => { + const { container } = renderView( + doc([para([text("un", [{ type: "underline" }])])]), + ); + expect(container.querySelector("u")?.textContent).toBe("un"); + // Underline is a supported mark, so no degrade to the editor fallback. + expect(screen.queryByTestId("comment-editor-fallback")).toBeNull(); + }); + it("renders the code mark as ", () => { const { container } = renderView( doc([para([text("co", [{ type: "code" }])])]), diff --git a/apps/client/src/features/comment/components/comment-content-view.tsx b/apps/client/src/features/comment/components/comment-content-view.tsx index 7e6feea9..3255cd24 100644 --- a/apps/client/src/features/comment/components/comment-content-view.tsx +++ b/apps/client/src/features/comment/components/comment-content-view.tsx @@ -62,6 +62,11 @@ function renderMarks( return {acc}; case "strike": return {acc}; + case "underline": + // StarterKit enables the Underline extension by default (Mod-u) and + // CommentEditor does not disable it, so real comments can carry this + // mark. Render it here rather than degrading the whole comment. + return {acc}; case "code": return {acc}; case "link": { diff --git a/apps/client/src/features/comment/components/comment-list-item.test.tsx b/apps/client/src/features/comment/components/comment-list-item.test.tsx index f41f952c..5b217520 100644 --- a/apps/client/src/features/comment/components/comment-list-item.test.tsx +++ b/apps/client/src/features/comment/components/comment-list-item.test.tsx @@ -1,5 +1,5 @@ -import { describe, it, expect, vi } from "vitest"; -import { render, screen, fireEvent } from "@testing-library/react"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor } from "@testing-library/react"; import { MantineProvider } from "@mantine/core"; import { IComment } from "@/features/comment/types/comment.types"; @@ -9,10 +9,11 @@ import { IComment } from "@/features/comment/types/comment.types"; // component renders in isolation. We only assert the AI-badge rendering branch. const applyMutateAsync = vi.fn(); const dismissMutateAsync = vi.fn(); +const updateMutateAsync = vi.fn(); vi.mock("@/features/comment/queries/comment-query", () => ({ useDeleteCommentMutation: () => ({ mutateAsync: vi.fn() }), useResolveCommentMutation: () => ({ mutateAsync: vi.fn() }), - useUpdateCommentMutation: () => ({ mutateAsync: vi.fn() }), + useUpdateCommentMutation: () => ({ mutateAsync: updateMutateAsync }), useApplySuggestionMutation: () => ({ mutateAsync: applyMutateAsync, isPending: false, @@ -23,10 +24,42 @@ vi.mock("@/features/comment/queries/comment-query", () => ({ }), })); +// The document the mocked editor emits via onUpdate when the edit form is open. +// Duplicated inside the mock factory (below) to keep the factory self-contained. +const EDITED_DOC = { + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "edited via editor" }] }, + ], +}; + // CommentEditor pulls in the full TipTap editor stack; replace it with a stub. -vi.mock("@/features/comment/components/comment-editor", () => ({ - default: () =>
, -})); +// In edit mode the stub exposes buttons that fire the real onUpdate/onSave props +// so the edit->save/cancel flow can be driven without a live editor. +vi.mock("@/features/comment/components/comment-editor", () => { + const doc = { + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "edited via editor" }] }, + ], + }; + return { + default: ({ onUpdate, onSave }: any) => ( +
+
+ ), + }; +}); // CommentContentView (used for the read-only body) imports the mention view, // which pulls page-query -> main.tsx (createRoot). Stub the queries so the item @@ -297,6 +330,113 @@ describe("canShowDismiss predicate", () => { }); }); +describe("CommentListItem — edit -> save/cancel flow (#340 F3)", () => { + const body = (t: string) => + JSON.stringify({ + type: "doc", + content: [{ type: "paragraph", content: [{ type: "text", text: t }] }], + }); + + // The edit menu item is gated on the viewer owning the comment + // (currentUser.id === creatorId). currentUserAtom is atomWithStorage-backed, + // so seed localStorage to make the viewer the owner (creatorId "user-1"). + beforeEach(() => { + updateMutateAsync.mockClear(); + localStorage.setItem( + "currentUser", + JSON.stringify({ user: { id: "user-1", name: "Owner" } }), + ); + }); + afterEach(() => { + localStorage.clear(); + }); + + async function openEditor() { + // Open the comment menu, then click "Edit comment" to toggle into edit mode. + fireEvent.click(screen.getByLabelText("Comment menu")); + fireEvent.click(await screen.findByText("Edit comment")); + // Edit form (mocked editor + actions) is now mounted. + await screen.findByTestId("comment-editor"); + } + + it("saves the edited content and, on cache update, shows the new body", async () => { + const { rerender } = renderItem( + baseComment({ content: body("original body") }), + ); + // Static body first. + expect(screen.getByText("original body")).toBeDefined(); + + await openEditor(); + + // Editor emits an update (populates editContentRef), then Save is clicked. + fireEvent.click(screen.getByTestId("editor-emit-update")); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + + // mutateAsync is called with the stringified edited doc. + expect(updateMutateAsync).toHaveBeenCalledWith({ + commentId: "c-1", + content: JSON.stringify(EDITED_DOC), + }); + + // On success the form closes (isEditing -> false); the static body renders + // from the comment.content prop again. + await waitFor(() => + expect(screen.queryByTestId("comment-editor")).toBeNull(), + ); + + // Simulate the cache invalidation swapping in a new comment object with the + // updated content — the static body reflects it. + rerender( + + + , + ); + expect(screen.getByText("updated body after save")).toBeDefined(); + expect(screen.queryByText("original body")).toBeNull(); + }); + + it("cancel restores the static body and does not call the update mutation", async () => { + renderItem(baseComment({ content: body("original body") })); + await openEditor(); + + // Type something (editContentRef set), then cancel. + fireEvent.click(screen.getByTestId("editor-emit-update")); + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + + // Editor unmounts, static body restored, no save happened. + await waitFor(() => + expect(screen.queryByTestId("comment-editor")).toBeNull(), + ); + expect(screen.getByText("original body")).toBeDefined(); + expect(updateMutateAsync).not.toHaveBeenCalled(); + }); + + it("saving without editing sends the existing content (editContentRef cleared after cancel)", async () => { + renderItem(baseComment({ content: body("original body") })); + + // Cancel path clears editContentRef... + await openEditor(); + fireEvent.click(screen.getByTestId("editor-emit-update")); + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + await waitFor(() => + expect(screen.queryByTestId("comment-editor")).toBeNull(), + ); + + // ...so re-opening and saving WITHOUT an update falls back to comment.content. + await openEditor(); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + expect(updateMutateAsync).toHaveBeenCalledWith({ + commentId: "c-1", + content: JSON.stringify(body("original body")), + }); + }); +}); + describe("CommentListItem — read-only body renders statically", () => { it("renders the comment body as static text without a TipTap editor", () => { renderItem( diff --git a/apps/client/src/features/comment/components/comment-list-with-tabs.test.tsx b/apps/client/src/features/comment/components/comment-list-with-tabs.test.tsx new file mode 100644 index 00000000..cdc3bcb4 --- /dev/null +++ b/apps/client/src/features/comment/components/comment-list-with-tabs.test.tsx @@ -0,0 +1,108 @@ +import { describe, it, expect, vi } from "vitest"; +import { render, screen, fireEvent } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; +import { IComment } from "@/features/comment/types/comment.types"; + +// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts. + +// CommentEditor pulls in the full TipTap editor stack; replace it with a stub so +// the lazy reply editor's mount transition can be observed without the editor. +vi.mock("@/features/comment/components/comment-editor", () => ({ + default: () =>
, +})); + +// page-query -> main.tsx (createRoot) is a module side effect; stub the queries +// pulled in transitively so importing the module is side-effect free. +vi.mock("@/features/page/queries/page-query.ts", () => ({ + usePageQuery: () => ({ data: undefined, isLoading: false, isError: false }), +})); +vi.mock("@/features/share/queries/share-query.ts", () => ({ + useSharePageQuery: () => ({ data: undefined }), +})); +// space-query -> main.tsx (createRoot) is another module side effect; stub it. +vi.mock("@/features/space/queries/space-query.ts", () => ({ + useGetSpaceBySlugQuery: () => ({ data: undefined }), +})); + +import { + buildChildrenByParent, + CommentEditorWithActions, +} from "./comment-list-with-tabs"; + +const c = (id: string, parentCommentId: string | null = null): IComment => + ({ id, parentCommentId }) as IComment; + +describe("buildChildrenByParent (childrenByParent grouping)", () => { + it("returns an empty map for undefined or empty input", () => { + expect(buildChildrenByParent(undefined).size).toBe(0); + expect(buildChildrenByParent([]).size).toBe(0); + }); + + it("does not index a top-level comment (parentCommentId null)", () => { + const map = buildChildrenByParent([c("p1", null)]); + expect(map.size).toBe(0); + expect(map.has("p1")).toBe(false); + }); + + it("groups replies under the correct parent, including reply-to-reply nesting", () => { + const p1 = c("p1", null); + const r1 = c("r1", "p1"); + const r2 = c("r2", "r1"); // a reply to a reply + const map = buildChildrenByParent([p1, r1, r2]); + expect(map.get("p1")).toEqual([r1]); + expect(map.get("r1")).toEqual([r2]); + // The top-level comment itself is never a key. + expect(map.has("p1") && map.get("p1")?.length).toBe(1); + }); + + it("still groups a reply whose parent is not present in items", () => { + const orphan = c("o1", "missing-parent"); + const map = buildChildrenByParent([orphan]); + expect(map.get("missing-parent")).toEqual([orphan]); + }); + + it("preserves insertion order among sibling replies", () => { + const map = buildChildrenByParent([ + c("a", "p1"), + c("b", "p1"), + c("d", "p1"), + ]); + expect(map.get("p1")?.map((x) => x.id)).toEqual(["a", "b", "d"]); + }); +}); + +function renderReplyEditor() { + return render( + + + , + ); +} + +describe("CommentEditorWithActions — lazy reply editor activation", () => { + it("shows only the stub initially (no editor instance mounted)", () => { + renderReplyEditor(); + expect(screen.getByRole("button")).toBeDefined(); + expect(screen.queryByTestId("comment-editor")).toBeNull(); + }); + + it("mounts the real editor when the stub is clicked and keeps it mounted", () => { + renderReplyEditor(); + fireEvent.click(screen.getByRole("button")); + expect(screen.getByTestId("comment-editor")).toBeDefined(); + // The stub button is replaced by the editor subtree. + expect(screen.queryByRole("button")).toBeNull(); + }); + + it("mounts the editor when the stub receives focus", () => { + renderReplyEditor(); + fireEvent.focus(screen.getByRole("button")); + expect(screen.getByTestId("comment-editor")).toBeDefined(); + }); + + it("mounts the editor on Enter keydown of the stub", () => { + renderReplyEditor(); + fireEvent.keyDown(screen.getByRole("button"), { key: "Enter" }); + expect(screen.getByTestId("comment-editor")).toBeDefined(); + }); +}); diff --git a/apps/client/src/features/comment/components/comment-list-with-tabs.tsx b/apps/client/src/features/comment/components/comment-list-with-tabs.tsx index 5ee19a3e..b5e62e8e 100644 --- a/apps/client/src/features/comment/components/comment-list-with-tabs.tsx +++ b/apps/client/src/features/comment/components/comment-list-with-tabs.tsx @@ -35,6 +35,24 @@ interface CommentListWithTabsProps { onClose?: () => void; } +// Index replies by their parent id once (O(n)), instead of an O(n^2) filter per +// thread. Replies whose parent is not in `items` are still grouped under their +// parentCommentId (they simply won't be reached by the top-level walk). +// Exported for unit testing. +export function buildChildrenByParent( + items: IComment[] | undefined, +): Map { + const m = new Map(); + for (const c of items ?? []) { + if (c.parentCommentId) { + const arr = m.get(c.parentCommentId); + if (arr) arr.push(c); + else m.set(c.parentCommentId, [c]); + } + } + return m; +} + function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { const { t } = useTranslation(); const { pageSlug } = useParams(); @@ -79,17 +97,10 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { // Index replies by their parent once, instead of an O(n^2) filter per thread. // The map ref changes on any comments update, so MemoizedChildComments re-runs // (cheap) and re-looks-up, while memoized CommentListItems skip unchanged items. - const childrenByParent = useMemo(() => { - const m = new Map(); - for (const c of comments?.items ?? []) { - if (c.parentCommentId) { - const arr = m.get(c.parentCommentId); - if (arr) arr.push(c); - else m.set(c.parentCommentId, [c]); - } - } - return m; - }, [comments?.items]); + const childrenByParent = useMemo( + () => buildChildrenByParent(comments?.items), + [comments?.items], + ); const [isPageCommentLoading, setIsPageCommentLoading] = useState(false); @@ -217,8 +228,10 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { Resolved -> Open switch. keepMounted={false} style={{ flex: "1 1 auto", @@ -278,7 +291,10 @@ function CommentListWithTabs({ onClose }: CommentListWithTabsProps) { type="scroll" >
- + {/* keepMounted keeps the Open panel alive even while Resolved is + active, so a lazily-mounted reply editor's draft (and an + in-progress edit) is not discarded on tab switch. */} + {activeComments.length === 0 ? (
@@ -368,7 +384,7 @@ const ChildComments = ({ const MemoizedChildComments = memo(ChildComments); -const CommentEditorWithActions = ({ +export const CommentEditorWithActions = ({ commentId, onSave, placeholder = undefined, -- 2.52.0