diff --git a/packages/ui/markdown/Markdown.tsx b/packages/ui/markdown/Markdown.tsx index 8c9651e5454..beaa5d53e85 100644 --- a/packages/ui/markdown/Markdown.tsx +++ b/packages/ui/markdown/Markdown.tsx @@ -1,8 +1,8 @@ import * as React from 'react' -import ReactMarkdown, { type Components, defaultUrlTransform } from 'react-markdown' +import ReactMarkdown, { type Components } from 'react-markdown' import rehypeKatex from 'rehype-katex' import rehypeRaw from 'rehype-raw' -import rehypeSanitize, { defaultSchema } from 'rehype-sanitize' +import rehypeSanitize from 'rehype-sanitize' import remarkBreaks from 'remark-breaks' import remarkGfm from 'remark-gfm' import remarkMath from 'remark-math' @@ -14,6 +14,7 @@ import { isAllowedFileCardHref, preprocessFileCards } from './file-cards' import { preprocessLinks } from './linkify' import { preprocessIssueIdentifiers } from './issue-identifiers' import { preprocessMentionShortcodes } from './mentions' +import { markdownSanitizeSchema, markdownUrlTransform } from './sanitize' import 'katex/dist/katex.min.css' import './markdown.css' @@ -87,64 +88,6 @@ export interface MarkdownProps { autolinkIssueIdentifiers?: boolean } -// Sanitization schema — extends GitHub defaults to allow code highlighting classes -// and Multica's internal mention/slash protocols. -const sanitizeSchema = { - ...defaultSchema, - protocols: { - ...defaultSchema.protocols, - href: [...(defaultSchema.protocols?.href ?? []), 'mention', 'slash'], - // Permit inline data-URI images (QR codes, charts, base64 screenshots). - // The scheme gate only allows `data:` through here; attributes.img below - // narrows it to image/* so non-image data URIs are still rejected. - src: [...(defaultSchema.protocols?.src ?? []), 'data'], - }, - attributes: { - ...defaultSchema.attributes, - div: [ - ...(defaultSchema.attributes?.div ?? []), - 'dataType', - 'dataHref', - 'dataFilename', - ], - code: [ - ...(defaultSchema.attributes?.code ?? []), - ['className', /^language-/], - ['className', /^math-/], - ['className', /^hljs/], - ], - img: [ - // Drop the default plain `src` entry so the value allow-list below is the - // one findDefinition resolves — it returns the first match by name, so a - // bare `src` string would otherwise shadow (and disable) the allow-list. - ...(defaultSchema.attributes?.img ?? []).filter( - (attr) => (typeof attr === 'string' ? attr : attr[0]) !== 'src', - ), - 'alt', - // Allow inline data:image/* URIs while leaving every other src form - // (http/https/site-relative) exactly as before: the negative lookahead - // keeps all non-data values, and data: is narrowed to images only. - ['src', /^data:image\//i, /^(?!data:)/i], - ], - }, -} - -/** - * Custom URL transform that allows Multica internal protocols while keeping - * the default security for all other URLs. - */ -function urlTransform(url: string): string { - if (url.startsWith('mention://')) return url - if (url.startsWith('slash://skill/')) return url - // Allow inline data:image/* URIs — defaultUrlTransform strips every data: URL - // to '', which would blank the src even after rehype-sanitize keeps it. Kept - // in sync with the image/* narrowing in sanitizeSchema (protocols.src + - // attributes.img) so both gates agree on what a valid inline image is. - if (/^data:image\//i.test(url)) return url - return defaultUrlTransform(url) -} - - // File path detection regex - matches paths starting with /, ~/, or ./ const FILE_PATH_REGEX = /^(?:\/|~\/|\.\/)[\w\-./@]+\.(?:ts|tsx|js|jsx|mjs|cjs|md|json|yaml|yml|py|go|rs|css|scss|less|html|htm|txt|log|sh|bash|zsh|swift|kt|java|c|cpp|h|hpp|rb|php|xml|toml|ini|cfg|conf|env|sql|graphql|vue|svelte|astro|prisma)$/i @@ -483,8 +426,8 @@ export function Markdown({ remarkBreaks, [remarkGfm, { singleTilde: false }], ]} - rehypePlugins={[rehypeRaw, [rehypeSanitize, sanitizeSchema], rehypeKatex]} - urlTransform={urlTransform} + rehypePlugins={[rehypeRaw, [rehypeSanitize, markdownSanitizeSchema], rehypeKatex]} + urlTransform={markdownUrlTransform} components={components} > {processedContent} diff --git a/packages/ui/markdown/index.ts b/packages/ui/markdown/index.ts index ce609eb5d5f..a800eb5091c 100644 --- a/packages/ui/markdown/index.ts +++ b/packages/ui/markdown/index.ts @@ -8,6 +8,7 @@ export { ISSUE_IDENTIFIER_PATTERN, } from './issue-identifiers' export { preprocessMentionShortcodes } from './mentions' +export { markdownSanitizeSchema, markdownUrlTransform } from './sanitize' export { preprocessFileCards, isCdnUrl, diff --git a/packages/ui/markdown/sanitize.ts b/packages/ui/markdown/sanitize.ts new file mode 100644 index 00000000000..bc828d1042d --- /dev/null +++ b/packages/ui/markdown/sanitize.ts @@ -0,0 +1,75 @@ +import { defaultUrlTransform } from 'react-markdown' +import { defaultSchema, type Options } from 'rehype-sanitize' + +/** + * Canonical sanitize schema + URL transform for every product-level Markdown + * renderer (Chat via ui/markdown/Markdown.tsx, Issue/Comment via + * views/editor/readonly-content.tsx). + * + * This is the ONLY copy. Both renderers previously carried a verbatim fork of + * this schema, which had already drifted — the readonly fork whitelisted + * for `==highlight==` and the chat fork did not. A security-relevant allow-list + * maintained in two places means any future XSS fix has to land twice, and + * missing one is a hole. Adding a surface means importing this, never copying + * it (MUL-4922). + * + * The two gates below must agree on what a valid inline image is: + * `protocols.src` + `attributes.img` (sanitize) and `markdownUrlTransform` + * (react-markdown). Change them together. + */ +export const markdownSanitizeSchema: Options = { + ...defaultSchema, + // Allow (text highlight) — emitted by highlightToHtml from `==text==`. + // It carries no attributes, so only the tag name needs whitelisting. + tagNames: [...(defaultSchema.tagNames ?? []), 'mark'], + protocols: { + ...defaultSchema.protocols, + href: [...(defaultSchema.protocols?.href ?? []), 'mention', 'slash'], + // Permit inline data-URI images (QR codes, charts, base64 screenshots). + // The scheme gate only allows `data:` through here; attributes.img below + // narrows it to image/* so non-image data URIs are still rejected. + src: [...(defaultSchema.protocols?.src ?? []), 'data'], + }, + attributes: { + ...defaultSchema.attributes, + div: [ + ...(defaultSchema.attributes?.div ?? []), + 'dataType', + 'dataHref', + 'dataFilename', + ], + code: [ + ...(defaultSchema.attributes?.code ?? []), + ['className', /^language-/], + ['className', /^math-/], + ['className', /^hljs/], + ], + img: [ + // Drop the default plain `src` entry so the value allow-list below is the + // one findDefinition resolves — it returns the first match by name, so a + // bare `src` string would otherwise shadow (and disable) the allow-list. + ...(defaultSchema.attributes?.img ?? []).filter( + (attr) => (typeof attr === 'string' ? attr : attr[0]) !== 'src', + ), + 'alt', + // Allow inline data:image/* URIs while leaving every other src form + // (http/https/site-relative) exactly as before: the negative lookahead + // keeps all non-data values, and data: is narrowed to images only. + ['src', /^data:image\//i, /^(?!data:)/i], + ], + }, +} + +/** + * Allows Multica internal protocols and inline images through react-markdown's + * URL gate while keeping default security for everything else. + */ +export function markdownUrlTransform(url: string): string { + if (url.startsWith('mention://')) return url + if (url.startsWith('slash://skill/')) return url + // defaultUrlTransform strips every data: URL to '', which would blank the src + // even after rehype-sanitize keeps it. Kept in sync with the image/* narrowing + // in markdownSanitizeSchema so both gates agree on what a valid inline image is. + if (/^data:image\//i.test(url)) return url + return defaultUrlTransform(url) +} diff --git a/packages/views/chat/components/chat-message-list.test.tsx b/packages/views/chat/components/chat-message-list.test.tsx index ef281515ad2..e5bbb42e9eb 100644 --- a/packages/views/chat/components/chat-message-list.test.tsx +++ b/packages/views/chat/components/chat-message-list.test.tsx @@ -1,11 +1,46 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { act, render, screen } from "@testing-library/react"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { I18nProvider } from "@multica/core/i18n/react"; import { chatKeys } from "@multica/core/chat/queries"; import type { TaskMessagePayload } from "@multica/core/types"; +import type { ReactElement } from "react"; import enChat from "../../locales/en/chat.json"; +// The live timeline is a real list row rather than Virtuoso chrome (MUL-4922), +// so it shares one identity with the persisted assistant row and keeps its +// Mermaid/HTML blocks mounted across task completion. Real react-virtuoso +// renders its Footer but NO data rows under jsdom's zero-height viewport, so +// these tests must stub it to see rows at all. computeItemKey is still applied +// here — it is what expresses the live -> persisted identity. +vi.mock("react-virtuoso", () => ({ + Virtuoso: ({ + data, + itemContent, + computeItemKey, + components, + context, + }: { + data: unknown[]; + itemContent: (i: number, item: unknown) => ReactElement; + computeItemKey: (i: number, item: unknown) => string; + components?: { Footer?: (p: { context?: unknown }) => ReactElement | null }; + context?: unknown; + }) => { + const Footer = components?.Footer; + return ( +
+ {data.map((item, i) => ( +
+ {itemContent(i, item)} +
+ ))} + {Footer ?
: null} +
+ ); + }, +})); + import { ChatMessageList } from "./chat-message-list"; const TEST_RESOURCES = { en: { chat: enChat } }; diff --git a/packages/views/chat/components/chat-message-list.tsx b/packages/views/chat/components/chat-message-list.tsx index 1ed5f24aa0c..cec109038c0 100644 --- a/packages/views/chat/components/chat-message-list.tsx +++ b/packages/views/chat/components/chat-message-list.tsx @@ -1,6 +1,6 @@ "use client"; -import { memo, useCallback, useMemo, useRef, useState } from "react"; +import { memo, useCallback, useEffect, useMemo, useRef, useState } from "react"; import { toast } from "sonner"; import { useQuery } from "@tanstack/react-query"; import { Virtuoso, type Components } from "react-virtuoso"; @@ -20,7 +20,8 @@ import { import { ChevronRight, ChevronDown, Brain, AlertCircle, AlertTriangle, Copy } from "lucide-react"; import { useScrollFade } from "@multica/ui/hooks/use-scroll-fade"; import { isTaskMessageTaskId, taskMessagesOptions } from "@multica/core/chat/queries"; -import { MemoizedMarkdown } from "@multica/views/common/markdown"; +import { RichContent } from "../../rich-content"; +import { RichContentScrollRootProvider } from "../../rich-content/scroll-root"; import { copyText } from "@multica/ui/lib/clipboard"; import { AttachmentList } from "../../issues/components/comment-card"; import type { AgentAvailability } from "@multica/core/agents"; @@ -70,14 +71,34 @@ interface ChatMessageListProps { interface ChatListContext { isFetchingOlderMessages: boolean; - hasLive: boolean; - liveTimeline: ChatTimelineItem[]; showStatusPill: boolean; pendingTask: ChatPendingTask | null | undefined; liveTaskMessages: readonly TaskMessagePayload[] | undefined; availability: AgentAvailability | undefined; } +/** + * One Virtuoso row. A live (still-streaming) task and the persisted assistant + * message it becomes share ONE key — `task:` — so the handoff replaces + * this item's data in place instead of unmounting a Footer subtree and mounting + * a different row (MUL-4922). That identity is what keeps an already-rendered + * Mermaid diagram or HTML iframe mounted across task completion. + */ +type ChatRenderItem = + | { key: string; kind: "message"; message: ChatMessage; taskId: string | null } + | { key: string; kind: "live"; taskId: string }; + +/** + * Row key for a persisted message. Assistant turns carrying a task_id key on + * the task so they can inherit the live row; everything else keys on its own + * id. + */ +function messageRowKey(message: ChatMessage): string { + return message.role === "assistant" && message.task_id + ? `task:${message.task_id}` + : message.id; +} + function ChatListHeader({ context }: { context?: ChatListContext }) { const { t } = useT("chat"); return ( @@ -91,27 +112,24 @@ function ChatListHeader({ context }: { context?: ChatListContext }) { ); } +// The Footer now carries only the status pill — task chrome, not content. The +// live timeline moved into a real row so it can keep its identity when the +// task completes (see ChatRenderItem). function ChatListFooter({ context }: { context?: ChatListContext }) { if (!context) return null; + if (!context.showStatusPill || !context.pendingTask) return null; return (
- {context.hasLive && ( -
- -
- )} - {context.showStatusPill && context.pendingTask && ( - - )} +
); } -const LIST_COMPONENTS: Components = { +const LIST_COMPONENTS: Components = { Header: ChatListHeader, Footer: ChatListFooter, }; @@ -148,30 +166,39 @@ export function ChatMessageList({ ); // Live timeline for the in-flight task. useRealtimeSync keeps this cache - // current via setQueryData on task:message events. + // current via setQueryData on task:message events. Only used here to decide + // whether the live row exists and to feed the status pill — the row itself + // reads the same cache entry through AssistantMessage. const showLiveTimeline = !!pendingTaskId && !pendingAlreadyPersisted; const canFetchLiveTimeline = isTaskMessageTaskId(pendingTaskId) && !pendingAlreadyPersisted; const { data: liveTaskMessages } = useQuery({ ...taskMessagesOptions(pendingTaskId ?? ""), enabled: canFetchLiveTimeline, }); - // Memoized on the cache array identity: mergeTaskMessagesBySeq preserves - // the array reference when a duplicate event arrives, so this recomputes - // only when a genuinely new message lands — not on unrelated re-renders. - const liveTimeline: ChatTimelineItem[] = useMemo( - () => transformTimeline(buildTimeline(liveTaskMessages ?? []), transformContent), - [liveTaskMessages, transformContent], - ); - const hasLive = showLiveTimeline && liveTimeline.length > 0; + const hasLive = showLiveTimeline && (liveTaskMessages?.length ?? 0) > 0; const showStatusPill = !!pendingTaskId && !pendingAlreadyPersisted && !!pendingTask; - const totalCount = messages.length + (hasLive || showStatusPill ? 1 : 0); - const firstIndex = totalCount > 0 ? firstItemIndex : 0; + // Persisted messages plus, while a task is in flight, one synthetic trailing + // row for it. When the assistant message persists, `hasLive` goes false and + // the message takes the SAME key at the SAME position — an in-place data + // swap, not a remount. + const renderItems: ChatRenderItem[] = useMemo(() => { + const items: ChatRenderItem[] = messages.map((message) => ({ + key: messageRowKey(message), + kind: "message" as const, + message, + taskId: message.task_id ?? null, + })); + if (hasLive && pendingTaskId) { + items.push({ key: `task:${pendingTaskId}`, kind: "live", taskId: pendingTaskId }); + } + return items; + }, [messages, hasLive, pendingTaskId]); + + const firstIndex = renderItems.length > 0 ? firstItemIndex : 0; const listContext: ChatListContext = { isFetchingOlderMessages, - hasLive, - liveTimeline, showStatusPill, pendingTask, liveTaskMessages, @@ -190,9 +217,13 @@ export function ChatMessageList({ ) : ( + // Chat scrolls inside its own element, so rich blocks must measure + // "near-viewport" against that element rather than the browser viewport — + // otherwise a diagram only starts loading once it is already on screen. + msg.id} + computeItemKey={(_, item) => item.key} context={listContext} components={LIST_COMPONENTS} - itemContent={(_, msg) => ( + itemContent={(_, item) => (
)} /> +
)} ); @@ -266,25 +298,44 @@ export function ChatMessageSkeleton() { // memo skips reconciling rows the stream didn't touch — the persisted // history stays inert while only the live footer updates. const MessageBubble = memo(function MessageBubble({ - message, + item, isPending, transformContent, }: { - message: ChatMessage; + item: ChatRenderItem; isPending: boolean; transformContent?: (content: string) => string; }) { + // The live row and the persisted assistant row both land here under one key, + // and both render — same component type, same position — + // so React reconciles rather than remounts at task completion. + if (item.kind === "live") { + return ( + + ); + } + + const { message } = item; + if (message.role === "user") { return (
- {/* User messages are authored as markdown in ContentEditor, so - * render them through the same pipeline as assistant replies. - * Neutralise prose's leading/trailing margin so single-line - * bubbles stay as compact as the plain-text version used to. */} -
- {message.content} -
+ {/* User messages are authored as markdown in ContentEditor, so they + * render through the SAME RichContent as assistant replies and as + * Issue/Comment — a Mermaid fence a user pastes is a diagram here + * too. `compact` trims the leading/trailing block margins so a + * single-line bubble stays as tight as the plain-text version. */} + `), so the live → persisted + * handoff is a prop change rather than an unmount: the RichContent subtree and + * any Mermaid diagram / HTML iframe inside it stay mounted, keep their pan-zoom + * state, and never re-run their expensive render. Before this, the live + * timeline lived in Virtuoso's Footer and the persisted row keyed on + * `message.id`, so every completed task tore down and rebuilt its diagrams. + * + * The timeline itself comes from `taskMessagesOptions(taskId)` in both forms — + * the same cache entry useRealtimeSync seeds during execution — so no refetch + * and no data discontinuity happens at the handoff either. + */ function AssistantMessage({ + taskId, message, isPending, transformContent, }: { - message: ChatMessage; + taskId: string | null; + message?: ChatMessage; isPending: boolean; transformContent?: (content: string) => string; }) { - const taskId = message.task_id; const canFetchTaskMessages = isTaskMessageTaskId(taskId); // Use the shared taskMessagesOptions so this cache entry is the same one @@ -324,17 +394,23 @@ function AssistantMessage({ enabled: canFetchTaskMessages, }); - // Same memoization rationale as the live timeline in ChatMessageList. + // Memoized on the cache array identity: mergeTaskMessagesBySeq preserves the + // array reference when a duplicate event arrives, so this recomputes only + // when a genuinely new message lands. const timeline: ChatTimelineItem[] = useMemo( () => transformTimeline(buildTimeline(taskMessages ?? []), transformContent), [taskMessages, transformContent], ); + // Content is settled once the persisted message exists; until then text is + // still arriving and a trailing fence may be half-written. + const phase: "streaming" | "settled" = message ? "settled" : "streaming"; + // Failure bubble path: when the server's FailTask wrote a failure // chat_message (failure_reason set), render a destructive bubble with the // human-readable reason label + collapsible raw errMsg + the same timeline // so the user can see exactly where the run broke. - if (message.failure_reason) { + if (message?.failure_reason) { return ( {timeline.length > 0 && ( - + )} {isNoResponse ? ( - ) : timeline.length === 0 ? ( -
- {message.content} -
+ ) : message && timeline.length === 0 ? ( + ) : null} - - + {message && ( + <> + + + + )}
); } @@ -584,35 +673,42 @@ function TimelineView({ items, isStreaming, attachments, + phase = "settled", }: { items: ChatTimelineItem[]; isStreaming?: boolean; attachments?: import("@multica/core/types").Attachment[]; + phase?: "streaming" | "settled"; }) { const { preface, middle, final } = splitTimeline(items); return ( <> {preface.length > 0 && ( -
- - {preface.map((t) => t.content ?? "").join("")} - -
+ t.content ?? "").join("")} + attachments={attachments} + density="compact" + phase={phase} + className="leading-relaxed" + /> )} {middle.length > 0 && ( )} {final.length > 0 && ( -
- - {final.map((t) => t.content ?? "").join("")} - -
+ t.content ?? "").join("")} + attachments={attachments} + density="compact" + phase={phase} + className="leading-relaxed" + /> )} ); @@ -620,20 +716,27 @@ function TimelineView({ function OuterProcessFold({ items, - defaultOpen, + isStreaming, attachments, + phase = "settled", }: { items: ChatTimelineItem[]; - defaultOpen?: boolean; + isStreaming?: boolean; attachments?: import("@multica/core/types").Attachment[]; + phase?: "streaming" | "settled"; }) { const { t } = useT("chat"); - // useState seeds once at mount — subsequent renders never overwrite the - // user's manual toggle. The streaming → completed transition unmounts - // the live and mounts the persisted AssistantMessage's - // own , so the persisted instance starts closed (default) - // even if the live one was open. That's the desired collapsed-default. - const [open, setOpen] = useState(defaultOpen ?? false); + // Open while the task streams (so the user watches progress), collapsed once + // it settles. This used to fall out of a remount: the live TimelineView was + // torn down and the persisted one mounted closed. The row is now stable + // across that handoff (MUL-4922) — which is the point, it keeps Mermaid and + // HTML blocks alive — so the collapse has to be expressed directly. + const [open, setOpen] = useState(!!isStreaming); + const wasStreaming = useRef(!!isStreaming); + useEffect(() => { + if (wasStreaming.current && !isStreaming) setOpen(false); + wasStreaming.current = !!isStreaming; + }, [isStreaming]); const stepCount = items.length; return ( @@ -646,7 +749,12 @@ function OuterProcessFold({
{items.map((item) => item.type === "text" ? ( - + ) : ( ), @@ -664,13 +772,20 @@ function OuterProcessFold({ function MiddleTextRow({ item, attachments, + phase = "settled", }: { item: ChatTimelineItem; attachments?: import("@multica/core/types").Attachment[]; + phase?: "streaming" | "settled"; }) { return ( -
- {item.content ?? ""} +
+
); } diff --git a/packages/views/common/markdown.test.tsx b/packages/views/common/markdown.test.tsx deleted file mode 100644 index 0e3450e33f9..00000000000 --- a/packages/views/common/markdown.test.tsx +++ /dev/null @@ -1,182 +0,0 @@ -import type { ReactNode } from "react"; -import { render, screen } from "@testing-library/react"; -import { afterEach, describe, expect, it, vi } from "vitest"; -import { Markdown as MarkdownBase } from "@multica/ui/markdown"; -import { Markdown } from "./markdown"; - -const { resolveMock } = vi.hoisted(() => ({ resolveMock: vi.fn() })); - -vi.mock("../issues/hooks", () => ({ - useResolveIssueIdentifier: (identifier: string) => resolveMock(identifier), -})); - -vi.mock("@multica/core/config", () => ({ - useConfigStore: (selector: (state: { cdnDomain: string }) => unknown) => - selector({ cdnDomain: "" }), -})); - -vi.mock("../issues/components/issue-mention-card", () => ({ - IssueMentionCard: ({ issueId }: { issueId: string }) => ( - {issueId} - ), -})); - -vi.mock("@multica/core/paths", async (importOriginal) => { - const actual = await importOriginal(); - return { - ...actual, - useWorkspaceSlug: () => "acme", - useRequiredWorkspaceSlug: () => "acme", - useWorkspacePaths: () => ({ - ...actual.paths.workspace("acme"), - projectDetail: (projectId: string) => `/projects/${projectId}`, - }), - }; -}); - -vi.mock("../navigation", () => ({ - AppLink: ({ - href, - children, - className, - }: { - href: string; - children: ReactNode; - className?: string; - }) => ( - - {children} - - ), -})); - -vi.mock("../projects/components/project-chip", () => ({ - ProjectChip: ({ projectId }: { projectId: string }) => ( - {projectId} - ), -})); - -const ligatureClasses = [ - "[font-variant-ligatures:none]", - "[font-feature-settings:'liga'_0]", -]; - -describe("Markdown", () => { - it("disables ligatures inside raw code tags", () => { - render({"uv run --extra dev pytest -q"}); - - expect(screen.getByText("uv run --extra dev pytest -q")).toHaveClass(...ligatureClasses); - }); - - it("disables ligatures inside fenced code blocks", () => { - render({"```sh\nuv run --extra dev pytest -q\n```"}); - - expect(screen.getByText("uv run --extra dev pytest -q")).toHaveClass(...ligatureClasses); - }); - - it("disables ligatures in terminal-mode code", () => { - render({"uv run --extra dev pytest -q"}); - - expect(screen.getByText("uv run --extra dev pytest -q")).toHaveClass(...ligatureClasses); - }); - - it("renders slash skill links as slash command pills", () => { - const { container } = render( - [/deploy](slash://skill/abc-123), - ); - - const pill = container.querySelector(".slash-command"); - expect(pill).not.toBeNull(); - expect(pill?.textContent).toBe("/deploy"); - }); - - it("renders project mention links as project chips", () => { - render({"[Roadmap](mention://project/project-123)"}); - - expect(screen.getByTestId("project-chip")).toHaveTextContent("project-123"); - expect(screen.getByRole("link")).toHaveAttribute("href", "/projects/project-123"); - }); -}); - -describe("Markdown bare issue identifier autolink", () => { - afterEach(() => { - resolveMock.mockReset(); - }); - - it("renders a resolved bare identifier as an issue chip", () => { - resolveMock.mockImplementation((id: string) => - id === "MUL-1" ? { id: "issue-1", identifier: "MUL-1" } : null, - ); - - render({"Related to MUL-1 today"}); - - expect(screen.getByTestId("issue-mention-card")).toHaveTextContent("issue-1"); - expect(resolveMock).toHaveBeenCalledWith("MUL-1"); - }); - - it("leaves an unresolved bare identifier as plain text", () => { - resolveMock.mockReturnValue(null); - - render({"Related to MUL-999 today"}); - - expect(screen.queryByTestId("issue-mention-card")).toBeNull(); - expect(screen.getByText(/Related to/)).toHaveTextContent("MUL-999"); - }); - - it("does not autolink identifiers inside inline code", () => { - resolveMock.mockReturnValue(null); - - render({"use `MUL-1` here"}); - - expect(resolveMock).not.toHaveBeenCalled(); - expect(screen.queryByTestId("issue-mention-card")).toBeNull(); - }); - - it("routes a canonical UUID mention straight to the chip (no identifier resolve)", () => { - render( - - {"[MUL-2](mention://issue/00000000-0000-0000-0000-000000000002)"} - , - ); - - expect(screen.getByTestId("issue-mention-card")).toHaveTextContent( - "00000000-0000-0000-0000-000000000002", - ); - expect(resolveMock).not.toHaveBeenCalled(); - }); -}); - -// The base renderer uses a plain ; exercising it here (instead of the -// views wrapper, which swaps in ) lets us assert the sanitized -// `src` directly. Covers the two gates that used to blank data-URI images: -// rehype-sanitize's protocols.src and react-markdown's urlTransform. -describe("Markdown inline data-URI images", () => { - const PNG_1X1 = - "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=="; - - it("preserves the src of an inline data:image/png image", () => { - render({`![demo](${PNG_1X1})`}); - - const img = screen.getByAltText("demo"); - expect(img.tagName).toBe("IMG"); - expect(img).toHaveAttribute("src", PNG_1X1); - }); - - it("keeps regular http(s) images working", () => { - render({"![cat](https://cdn.example.com/cat.png)"}); - - expect(screen.getByAltText("cat")).toHaveAttribute( - "src", - "https://cdn.example.com/cat.png", - ); - }); - - it("strips non-image data URIs (data:text/html)", () => { - render( - {"![x](data:text/html,)"}, - ); - - const img = screen.getByAltText("x"); - expect(img.getAttribute("src") ?? "").toBe(""); - }); -}); diff --git a/packages/views/common/markdown.tsx b/packages/views/common/markdown.tsx deleted file mode 100644 index 920efda33c1..00000000000 --- a/packages/views/common/markdown.tsx +++ /dev/null @@ -1,140 +0,0 @@ -"use client"; - -import * as React from "react"; -import { - Markdown as MarkdownBase, - type MarkdownProps as MarkdownBaseProps, - type RenderMode, - isIssueIdentifier, -} from "@multica/ui/markdown"; -import { useConfigStore } from "@multica/core/config"; -import type { Attachment as AttachmentRecord } from "@multica/core/types"; -import { useWorkspacePaths } from "@multica/core/paths"; -import { IssueMentionCard } from "../issues/components/issue-mention-card"; -import { useResolveIssueIdentifier } from "../issues/hooks"; -import { ProjectChip } from "../projects/components/project-chip"; -import { AppLink } from "../navigation"; -import { - Attachment as AttachmentRenderer, - AttachmentDownloadProvider, -} from "../editor"; - -export type { RenderMode }; - -export interface MarkdownProps extends MarkdownBaseProps { - /** - * Attachments associated with the surrounding entity (chat message, skill - * file). When passed, the renderer resolves inline image / file-card URLs - * to full attachment records via AttachmentDownloadProvider, unlocking the - * unified hover toolbar / lightbox / preview-modal behavior used in - * editor surfaces. - */ - attachments?: AttachmentRecord[]; -} - -/** - * Default renderMention that delegates to entity chips for issue/project mentions - * and renders a styled span for other mention types. - */ -function ProjectMentionCard({ projectId }: { projectId: string }): React.ReactNode { - const p = useWorkspacePaths(); - return ( - - - - ); -} - -/** - * Autolinked bare identifier (e.g. `MUL-123`) routed through - * `mention://issue/`. Resolves the identifier to a real issue in - * the current workspace; renders a navigable chip on a hit, plain text on a - * miss / while loading / cross-workspace. - */ -function AutolinkedIssueMention({ identifier }: { identifier: string }): React.ReactNode { - const issue = useResolveIssueIdentifier(identifier); - if (!issue) return identifier; - return ; -} - -function defaultRenderMention({ - type, - id, -}: { - type: string; - id: string; -}): React.ReactNode { - if (type === "issue") { - // A bare identifier (from the autolink preprocessor) is carried as the id - // segment; a real mention carries a UUID. Dispatch on the id shape. - if (isIssueIdentifier(id)) { - return ; - } - return ; - } - if (type === "project") { - return ; - } - return null; -} - -function renderImage({ src, alt }: { src: string; alt: string }): React.ReactNode { - return ( - - ); -} - -function renderFileCard({ - href, - filename, -}: { - href: string; - filename: string; -}): React.ReactNode { - return ( - - ); -} - -/** - * App-level Markdown wrapper. Injects: - * - entity chips for issue/project mentions - * - cdnDomain from the config store (drives fileCard preprocessing) - * - unified as the image / file-card renderer - * - AttachmentDownloadProvider so url → record resolution works inside - * the injected components - */ -export function Markdown(props: MarkdownProps): React.JSX.Element { - const cdnDomain = useConfigStore((s) => s.cdnDomain); - const { attachments, ...rest } = props; - return ( - - - - ); -} - -export const MemoizedMarkdown = React.memo(Markdown); -MemoizedMarkdown.displayName = "MemoizedMarkdown"; diff --git a/packages/views/common/rich-content-sanitize-contract.test.tsx b/packages/views/common/rich-content-sanitize-contract.test.tsx new file mode 100644 index 00000000000..12a4de56101 --- /dev/null +++ b/packages/views/common/rich-content-sanitize-contract.test.tsx @@ -0,0 +1,184 @@ +/** + * Cross-surface sanitize contract (MUL-4922). + * + * The two product-level Markdown chains — Chat (ui/markdown/Markdown.tsx) and + * Issue/Comment (views/editor/readonly-content.tsx) — used to carry a verbatim + * fork of the sanitize schema and urlTransform each, and had already drifted: + * readonly whitelisted , chat did not. Both now import the single + * canonical base from @multica/ui/markdown. + * + * This suite runs one set of security fixtures through BOTH surfaces and + * asserts the same outcome. It is the mechanism that stops a third fork from + * growing back: a schema change that lands on only one chain fails here. + */ +import { describe, expect, it, vi } from "vitest"; +import { render } from "@testing-library/react"; +import { + Markdown as MarkdownBase, + markdownSanitizeSchema, +} from "@multica/ui/markdown"; + +vi.mock("@multica/core/config", () => ({ + useConfigStore: (selector: (state: { cdnDomain: string }) => unknown) => + selector({ cdnDomain: "" }), + configStore: { getState: () => ({ cdnDomain: "" }) }, +})); + +vi.mock("../issues/hooks", () => ({ + useResolveIssueIdentifier: () => null, +})); + +vi.mock("../i18n", async () => { + const editor = (await import("../locales/en/editor.json")).default; + return { + useT: () => ({ + t: (select: (bundle: typeof editor) => string) => select(editor), + }), + useTimeAgo: () => "just now", + }; +}); + +vi.mock("@multica/core/api", () => ({ + api: { getAttachmentTextContent: vi.fn() }, + PreviewTooLargeError: class extends Error {}, + PreviewUnsupportedError: class extends Error {}, +})); + +vi.mock("@multica/core/paths", () => ({ + useWorkspacePaths: () => ({ + issueDetail: (id: string) => `/test/issues/${id}`, + projectDetail: (id: string) => `/test/projects/${id}`, + }), + useWorkspaceSlug: () => "test", +})); + +vi.mock("../navigation", () => ({ + useNavigation: () => ({ push: vi.fn(), openInNewTab: vi.fn() }), + AppLink: ({ href, children }: { href: string; children: React.ReactNode }) => ( + {children} + ), +})); + +vi.mock("../issues/components/issue-mention-card", () => ({ + IssueMentionCard: ({ issueId }: { issueId: string }) => {issueId}, +})); + +vi.mock("../projects/components/project-chip", () => ({ + ProjectChip: ({ projectId }: { projectId: string }) => {projectId}, +})); + +vi.mock("../editor/link-hover-card", () => ({ + useLinkHover: () => ({}), + LinkHoverCard: () => null, +})); + +vi.mock("../editor/utils/link-handler", () => ({ + openLink: vi.fn(), + isMentionHref: (href?: string) => Boolean(href?.startsWith("mention://")), +})); + +// Expose the sanitized url/filename that survived the schema, so image +// fixtures can be asserted identically on both surfaces (readonly routes +// images through , the ui base renders a plain ). +vi.mock("../editor/attachment", () => ({ + Attachment: ({ + attachment, + }: { + attachment: { url: string; filename: string }; + }) => {attachment.filename}, +})); + +import { ReadonlyContent } from "../editor/readonly-content"; + +const PNG_1X1 = + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=="; + +const SURFACES: ReadonlyArray<{ + name: string; + render: (markdown: string) => HTMLElement; +}> = [ + { + name: "Chat (ui/markdown)", + render: (markdown) => render({markdown}).container, + }, + { + name: "Issue/Comment (ReadonlyContent)", + render: (markdown) => render().container, + }, +]; + +describe.each(SURFACES)("sanitize contract — $name", ({ render: renderSurface }) => { + it("strips "); + + expect(container.querySelector("script")).toBeNull(); + expect(container.textContent).not.toContain("alert(1)"); + }); + + it("strips event-handler attributes from raw HTML", () => { + const container = renderSurface(''); + + expect(container.querySelector("[onerror]")).toBeNull(); + expect(container.innerHTML).not.toContain("onerror"); + }); + + it("neutralizes javascript: hrefs", () => { + const container = renderSurface("[click](javascript:alert(1))"); + + const href = container.querySelector("a")?.getAttribute("href") ?? ""; + expect(href.toLowerCase()).not.toContain("javascript:"); + }); + + it("preserves an inline data:image/* src", () => { + const container = renderSurface(`![demo](${PNG_1X1})`); + + expect(container.querySelector("img")).toHaveAttribute("src", PNG_1X1); + }); + + it("blanks a non-image data: URI", () => { + const container = renderSurface("![x](data:text/html,)"); + + expect(container.querySelector("img")?.getAttribute("src") ?? "").toBe(""); + }); + + it("keeps http(s) image src intact", () => { + const container = renderSurface("![cat](https://cdn.example.com/cat.png)"); + + expect(container.querySelector("img")).toHaveAttribute( + "src", + "https://cdn.example.com/cat.png", + ); + }); + + // The drift this sweep closes: was whitelisted on readonly only, so + // the same content highlighted in a comment and rendered in chat disagreed. + it("allows (==highlight== lowering target)", () => { + const container = renderSurface("hi"); + + expect(container.querySelector("mark")?.textContent).toBe("hi"); + }); + + it("allows the slash:// protocol", () => { + const container = renderSurface("[/deploy](slash://skill/abc-123)"); + + expect(container.querySelector(".slash-command")?.textContent).toBe("/deploy"); + }); +}); + +// Code-block *rendering* is deliberately not asserted cross-surface yet: chat +// highlights with Shiki and readonly with lowlight, so the emitted class tokens +// still differ. Converging them is the RichCodeBlock phase of MUL-4922; until +// the highlight engine is picked, only the schema-level allow-list is shared. +describe("canonical sanitize schema", () => { + it("permits language-/math-/hljs class tokens on ", () => { + const codeAttrs = markdownSanitizeSchema.attributes?.code ?? []; + const patterns = codeAttrs + .filter((attr): attr is [string, RegExp] => Array.isArray(attr)) + .filter(([name]) => name === "className") + .map(([, pattern]) => pattern.source); + + expect(patterns).toEqual( + expect.arrayContaining(["^language-", "^math-", "^hljs"]), + ); + }); +}); diff --git a/packages/views/editor/attachment.tsx b/packages/views/editor/attachment.tsx index c85d9abb2cc..534579d0575 100644 --- a/packages/views/editor/attachment.tsx +++ b/packages/views/editor/attachment.tsx @@ -14,9 +14,9 @@ * Call sites: * - extensions/file-card.tsx FileCardView (Tiptap NodeView) * - extensions/image-view.tsx ImageView (Tiptap NodeView) - * - readonly-content.tsx (markdown img + fileCard div renderers) + * - rich-content/rich-content.tsx (markdown img + fileCard div renderers, + * serving Chat, Issue descriptions and Comments through one renderer) * - issues/components/comment-card.tsx AttachmentList (standalone fallback) - * - common/markdown.tsx (chat / skill viewer Markdown wrapper) * * The component owns its own preview modal and download dispatcher — callers * just pass `attachment` and (for editor surfaces) optional editor chrome diff --git a/packages/views/editor/content-editor.tsx b/packages/views/editor/content-editor.tsx index 3ee8e4ba79b..327615a6703 100644 --- a/packages/views/editor/content-editor.tsx +++ b/packages/views/editor/content-editor.tsx @@ -58,6 +58,7 @@ import type { MentionItem } from "./extensions/mention-suggestion"; import type { IssueIdentifierResolver } from "./extensions/issue-identifier-autolink"; import { createEditorExtensions } from "./extensions"; import { uploadAndInsertFile } from "./extensions/file-upload"; +import { configStore } from "@multica/core/config"; import { preprocessMarkdown } from "./utils/preprocess"; import { repairEmptyListItems } from "./utils/repair-list-items"; import { openLink, isMentionHref } from "./utils/link-handler"; @@ -439,7 +440,13 @@ const ContentEditor = forwardRef( const initialMarkdown = value ?? defaultValue ?? ""; const initialContent = initialMarkdown - ? preprocessMarkdown(initialMarkdown) + ? // One-shot read: the editor preprocesses its initial document once at + // load. Unlike the readonly renderer it does not need to re-run when + // the CDN config lands later — the user's own edits drive the document + // from here on. + preprocessMarkdown(initialMarkdown, { + cdnDomain: configStore.getState().cdnDomain, + }) : ""; // With `immediatelyRender: false` the Tiptap instance is created after // mount, so an imperative `focus()` fired on the same tick (e.g. chat @@ -623,7 +630,11 @@ const ContentEditor = forwardRef( // example `dev.de` used to become a link during a debounce pause). if (normalizeMarkdown(markdown) === before) return; - const incoming = markdown ? preprocessMarkdown(markdown) : ""; + const incoming = markdown + ? preprocessMarkdown(markdown, { + cdnDomain: configStore.getState().cdnDomain, + }) + : ""; const incomingNormalized = normalizeMarkdown(incoming); // Normalized-equal short-circuit. Avoids a no-op transaction when the // preprocessed input serializes to the document already on screen. diff --git a/packages/views/editor/html-block-preview.tsx b/packages/views/editor/html-block-preview.tsx index ccf4be50f3f..8659f689a19 100644 --- a/packages/views/editor/html-block-preview.tsx +++ b/packages/views/editor/html-block-preview.tsx @@ -36,6 +36,14 @@ import { HtmlPreviewBody } from "./html-preview-body"; const CODE_BLOCK_IFRAME_HEIGHT = "h-[480px]"; +/** + * Pixel twin of CODE_BLOCK_IFRAME_HEIGHT. The preview iframe is a fixed height, + * so the near-viewport lazy shell (rich-content/lazy-rich-block.tsx) can + * reserve exactly the space this component will occupy and mount with zero + * layout shift. Keep the two in sync. + */ +export const HTML_BLOCK_PREVIEW_HEIGHT_PX = 480; + // Label shown in the code-block header. Not a translatable string — it's a // language identifier (matches the `lang === "html"` token below). const HTML_LANGUAGE_LABEL = "html"; diff --git a/packages/views/editor/mermaid-diagram.tsx b/packages/views/editor/mermaid-diagram.tsx index 892f475b8ac..3f14b69dec5 100644 --- a/packages/views/editor/mermaid-diagram.tsx +++ b/packages/views/editor/mermaid-diagram.tsx @@ -147,7 +147,7 @@ function getMermaidLayout(svg: string): Size | null { // in this session. Picked to absorb most issue-detail diagrams without // excessive empty space; web.dev's CLS guidance recommends reserving any // such space upfront so async content doesn't shift surrounding layout. -const MERMAID_SKELETON_HEIGHT_PX = 280; +export const MERMAID_SKELETON_HEIGHT_PX = 280; const MERMAID_LAYOUT_CACHE_PREFIX = "multica:mermaid:layout:"; // DJB2 — small, fast, sufficient for sessionStorage cache keys. The chart @@ -161,6 +161,23 @@ function hashChart(chart: string): string { return (hash >>> 0).toString(36); } +/** + * Height to reserve for a diagram that has not rendered yet: the real height + * when this exact chart already rendered in this session, otherwise the + * skeleton default. Exported so the near-viewport lazy shell + * (rich-content/lazy-rich-block.tsx) reserves the SAME space this component + * would, instead of maintaining a second guess at the size. + * + * NOT safe to call during render: it reads sessionStorage, which does not exist + * on the server, so the value differs between the server frame and the browser's + * hydration frame whenever the cache is warm. Callers must use the skeleton + * default for the first frame and call this from an effect (see + * useReservedMermaidHeightPx in rich-content/rich-code-block.tsx). + */ +export function reservedMermaidHeightPx(chart: string): number { + return readCachedLayout(chart)?.height ?? MERMAID_SKELETON_HEIGHT_PX; +} + function readCachedLayout(chart: string): Size | null { if (typeof window === "undefined") return null; try { diff --git a/packages/views/editor/readonly-content.tsx b/packages/views/editor/readonly-content.tsx index 703dcb5842b..0fb60c7af0e 100644 --- a/packages/views/editor/readonly-content.tsx +++ b/packages/views/editor/readonly-content.tsx @@ -1,501 +1,46 @@ "use client"; /** - * ReadonlyContent — lightweight markdown renderer for readonly content display. + * ReadonlyContent — compatibility wrapper over the canonical . * - * Replaces for comment cards and other - * read-only surfaces. Uses react-markdown instead of a full Tiptap/ProseMirror - * instance, eliminating EditorView, Plugin, and NodeView overhead. + * The renderer itself moved to packages/views/rich-content/ (MUL-4922) so Chat, + * Issue descriptions and Comments all share ONE implementation of Markdown + * parsing, sanitize, fenced-code dispatch, mentions, links and attachments. + * This file stays only so existing document-density callers (comment cards, + * issue detail, autopilot detail, Markdown attachment preview) keep their + * import path and props. * - * Visual parity with ContentEditor is achieved by: - * - Wrapping output in
so the same - * styles/index.css rules apply to standard HTML tags - * - Using the same preprocessMarkdown pipeline (mention shortcodes + linkify) - * - Using lowlight for code highlighting (same engine as Tiptap's CodeBlockLowlight) - * so .hljs-* CSS rules from styles/code.css produce identical colors - * - Rendering mentions with the same IssueMentionCard component and .mention class + * Do not reintroduce rendering logic here. New behaviour belongs in + * RichContent, where every surface picks it up at once. */ -import { isValidElement, memo, useMemo, useRef, useState } from "react"; -import ReactMarkdown, { - defaultUrlTransform, - type Components, -} from "react-markdown"; -import type { ReactNode } from "react"; -import rehypeKatex from "rehype-katex"; -import remarkBreaks from "remark-breaks"; -import remarkGfm from "remark-gfm"; -import remarkMath from "remark-math"; -import rehypeRaw from "rehype-raw"; -import rehypeSanitize, { defaultSchema } from "rehype-sanitize"; -import { toHtml } from "hast-util-to-html"; -import { Check, Copy } from "lucide-react"; -import { cn } from "@multica/ui/lib/utils"; -import { copyText } from "@multica/ui/lib/clipboard"; -import { useWorkspacePaths, useWorkspaceSlug } from "@multica/core/paths"; +import { memo } from "react"; import type { Attachment } from "@multica/core/types"; -import { useT } from "../i18n"; -import { useNavigation } from "../navigation"; -import { IssueMentionCard } from "../issues/components/issue-mention-card"; -import { useResolveIssueIdentifier } from "../issues/hooks"; -import { ProjectChip } from "../projects/components/project-chip"; -import { useLinkHover, LinkHoverCard } from "./link-hover-card"; -import { openLink, isMentionHref } from "./utils/link-handler"; -import { isAllowedFileCardHref, isIssueIdentifier } from "@multica/ui/markdown"; -import { preprocessMarkdown } from "./utils/preprocess"; -import { highlightToHtml } from "./utils/highlight-markdown"; -import { MermaidDiagram } from "./mermaid-diagram"; -import { HtmlBlockPreview } from "./html-block-preview"; -import { AttachmentDownloadProvider } from "./attachment-download-context"; -import { Attachment as AttachmentRenderer } from "./attachment"; -import { highlightCode } from "./syntax-highlight"; -import "katex/dist/katex.min.css"; -import "./styles/index.css"; - -// Code fences that the `code` renderer returns as a non- React element -// (Mermaid diagram, HTML preview iframe). The `pre` renderer below unwraps -// these so the default
 envelope doesn't clamp their styles.
-// Anchored to whole class tokens so `language-htmlbars` / `language-mermaidx`
-// don't accidentally match and lose their 
 wrapper.
-const PRE_UNWRAP_RE = /(^|\s)language-(html|mermaid)(\s|$)/;
-
-// ---------------------------------------------------------------------------
-// Sanitization schema — extends GitHub defaults to allow file-card data attrs
-// ---------------------------------------------------------------------------
-
-const sanitizeSchema = {
-  ...defaultSchema,
-  // Allow  (text highlight) — emitted by highlightToHtml from `==text==`.
-  // It carries no attributes, so only the tag name needs whitelisting.
-  tagNames: [...(defaultSchema.tagNames ?? []), "mark"],
-  protocols: {
-    ...defaultSchema.protocols,
-    href: [...(defaultSchema.protocols?.href ?? []), "mention", "slash"],
-    // Permit inline data-URI images (QR codes, charts, base64 screenshots).
-    // The scheme gate only allows `data:` through here; attributes.img below
-    // narrows it to image/* so non-image data URIs are still rejected.
-    src: [...(defaultSchema.protocols?.src ?? []), "data"],
-  },
-  attributes: {
-    ...defaultSchema.attributes,
-    div: [
-      ...(defaultSchema.attributes?.div ?? []),
-      "dataType",
-      "dataHref",
-      "dataFilename",
-    ],
-    code: [
-      ...(defaultSchema.attributes?.code ?? []),
-      ["className", /^language-/],
-      ["className", /^math-/],
-      ["className", /^hljs/],
-    ],
-    img: [
-      // Drop the default plain `src` entry so the value allow-list below is the
-      // one findDefinition resolves — it returns the first match by name, so a
-      // bare `src` string would otherwise shadow (and disable) the allow-list.
-      ...(defaultSchema.attributes?.img ?? []).filter(
-        (attr) => (typeof attr === "string" ? attr : attr[0]) !== "src",
-      ),
-      "alt",
-      // Allow inline data:image/* URIs while leaving every other src form
-      // (http/https/site-relative) exactly as before: the negative lookahead
-      // keeps all non-data values, and data: is narrowed to images only.
-      ["src", /^data:image\//i, /^(?!data:)/i],
-    ],
-  },
-};
-
-// ---------------------------------------------------------------------------
-// URL transform — allow mention:// protocol through react-markdown's sanitizer
-// ---------------------------------------------------------------------------
-
-function urlTransform(url: string): string {
-  if (url.startsWith("mention://")) return url;
-  if (url.startsWith("slash://skill/")) return url;
-  // Allow inline data:image/* URIs — defaultUrlTransform strips every data: URL
-  // to '', which would blank the src even after rehype-sanitize keeps it. Kept
-  // in sync with the image/* narrowing in sanitizeSchema (protocols.src +
-  // attributes.img) so both gates agree on what a valid inline image is.
-  if (/^data:image\//i.test(url)) return url;
-  return defaultUrlTransform(url);
-}
-
-// ---------------------------------------------------------------------------
-// Custom react-markdown components
-// ---------------------------------------------------------------------------
-
-/**
- * Issue mention chip. Navigation — plain click, modifier click, and the
- * "open issue links in new tab" preference — is owned by the AppLink inside
- * IssueMentionCard; the wrapper only shields surrounding click handlers
- * (e.g. collapsed-comment expanders) from mention clicks.
- */
-function IssueMentionLink({ issueId, label }: { issueId: string; label?: string }) {
-  return (
-     e.stopPropagation()}>
-      
-    
-  );
-}
-
-/**
- * Autolinked bare identifier (e.g. `MUL-123`) routed through
- * `mention://issue/` by the readonly preprocessor. Resolves to a
- * real issue in the current workspace; renders a navigable mention on a hit,
- * plain text on a miss / while loading / cross-workspace.
- */
-function AutolinkedIssueMentionLink({ identifier }: { identifier: string }) {
-  const issue = useResolveIssueIdentifier(identifier);
-  if (!issue) return <>{identifier};
-  return ;
-}
-
-function ProjectMentionLink({ projectId, label }: { projectId: string; label?: string }) {
-  const { push, openInNewTab } = useNavigation();
-  const p = useWorkspacePaths();
-  const path = p.projectDetail(projectId);
-  return (
-     {
-        e.preventDefault();
-        e.stopPropagation();
-        if (e.metaKey || e.ctrlKey || e.shiftKey) {
-          if (openInNewTab) {
-            openInNewTab(path, label);
-          }
-          return;
-        }
-        push(path);
-      }}
-    >
-      
-    
-  );
-}
-
-function getTextContent(node: ReactNode): string {
-  if (node == null || typeof node === "boolean") return "";
-  if (typeof node === "string" || typeof node === "number") return String(node);
-  if (Array.isArray(node)) return node.map(getTextContent).join("");
-  if (isValidElement(node)) {
-    const props = node.props as { children?: ReactNode };
-    return getTextContent(props.children);
-  }
-  return "";
-}
-
-function ReadonlyCodeBlock({
-  children,
-  language,
-}: {
-  children: ReactNode;
-  language?: string;
-}) {
-  const { t } = useT("editor");
-  const [copied, setCopied] = useState(false);
-  const code = useMemo(
-    () => getTextContent(children).replace(/\n$/, ""),
-    [children],
-  );
-  const copyLabel = t(($) => $.code_block.copy_code) || "Copy code";
-
-  const handleCopy = async () => {
-    if (!code) return;
-    if (await copyText(code)) {
-      setCopied(true);
-      setTimeout(() => setCopied(false), 2000);
-    }
-  };
-
-  return (
-    
-
- {/* Same hover chrome as the editable code block's header - (code-block-view.tsx): language label + copy. */} - {language && ( - - {language} - - )} - -
- {/* No extra right padding: `.rich-text-editor pre` outranks utility - padding classes anyway, and the editable NodeView uses the same - 1rem — keeping them identical keeps line wrapping identical. */} -
{children}
-
- ); -} - -// Named component so it can call useWorkspaceSlug() — arrow function inlined -// inside `components` below would still work, but extracting it keeps the -// hook usage explicit and avoids hook-in-object-literal surprises. -function ReadonlyLink({ - href, - children, -}: { - href?: string; - children?: React.ReactNode; -}) { - const slug = useWorkspaceSlug(); - - if (href?.startsWith("slash://skill/")) { - return {children}; - } - - if (isMentionHref(href)) { - const match = href.match(/^mention:\/\/(member|agent|issue|project|all)\/(.+)$/); - if (match?.[1] === "issue" && match[2]) { - // A bare identifier (from the autolink preprocessor) is carried as the id - // segment; a real mention carries a UUID. Dispatch on the id shape. - if (isIssueIdentifier(match[2])) { - return ; - } - const label = - typeof children === "string" - ? children - : Array.isArray(children) - ? children.join("") - : undefined; - return ; - } - if (match?.[1] === "project" && match[2]) { - const label = - typeof children === "string" - ? children - : Array.isArray(children) - ? children.join("") - : undefined; - return ; - } - // Member / agent / all mentions - return {children}; - } - - // Regular links — open directly on click - return ( - { - e.preventDefault(); - if (href) openLink(href, slug); - }} - > - {children} - - ); -} - -function buildComponents(): Partial { - return { - // Links — route mention:// to mention components, others show preview card - a: ReadonlyLink, - - // Images — unified through . The resolver context provided - // by AttachmentDownloadProvider (mounted in ReadonlyContent below) turns - // a CDN URL into a full record when possible; external URLs render as - // plain images with lightbox-via-preview-modal. forceKind is mandatory - // here because markdown `![]()` carries no content-type and alt is - // commonly empty or descriptive — without it images fall through to - // the file-card chrome. - img: ({ src, alt }) => ( - - ), - - // FileCard — intercept
from preprocessMarkdown - div: ({ node, children, ...props }) => { - const dataType = node?.properties?.dataType as string | undefined; - if (dataType === "fileCard") { - const rawHref = (node?.properties?.dataHref as string) || ""; - const href = isAllowedFileCardHref(rawHref) ? rawHref : ""; - const filename = (node?.properties?.dataFilename as string) || ""; - return ( - - ); - } - return
{children}
; - }, - - // Tables — wrap in tableWrapper div for border/radius/scroll (matches Tiptap) - table: ({ children }) => ( -
- {children}
-
- ), - - // Code — lowlight highlighting for blocks, plain render for inline - code: ({ className, children, node, ...props }) => { - const lang = /language-(\w+)/.exec(className || "")?.[1]; - const isBlock = - node?.position && - node.position.start.line !== node.position.end.line; - - if (isBlock && lang === "mermaid") { - return ; - } - if (isBlock && lang === "html") { - // Like Mermaid, return the React element directly here and rely on - // the `pre` renderer below to unwrap it — react-markdown otherwise - // wraps `code` children in a `
` whose monospace + overflow
-        // styles would clamp the preview iframe.
-        return ;
-      }
-
-      if (!isBlock && !lang) {
-        // Inline code — CSS handles styling via .rich-text-editor code
-        return {children};
-      }
-
-      // Block code — highlight with lowlight, output hljs classes
-      const code = String(children).replace(/\n$/, "");
-      try {
-        const tree = highlightCode(code, lang);
-        const html = toHtml(tree);
-        if (html) {
-          return (
-            
-          );
-        }
-      } catch {
-        // fall through to plain render
-      }
-      return (
-        
-          {children}
-        
-      );
-    },
-
-    // Pre — wrap regular code fences with copy chrome.
-    // Special-case Mermaid / HtmlBlockPreview returned from the `code`
-    // renderer above so the outer `
` does not wrap them — this is the
-    // standard two-layer pattern used to escape react-markdown's default
-    // `
` envelope.
-    pre: ({ children }) => {
-      // react-markdown calls `pre` BEFORE invoking the `code` renderer —
-      // `children` is the unrendered `` element from the AST. So we
-      // identify "this block was meant to be unwrapped" by inspecting the
-      // child's className (`language-mermaid`, `language-html`), not by
-      // checking `children.type === MermaidDiagram`, which never matches.
-      //
-      // Match by exact class token: a substring `includes("language-html")`
-      // would also fire on neighboring languages like `language-htmlbars`
-      // and silently strip their 
 wrapper.
-      let language: string | undefined;
-      if (isValidElement(children)) {
-        const childProps = children.props as { className?: string };
-        if (PRE_UNWRAP_RE.test(childProps.className ?? "")) {
-          return <>{children};
-        }
-        language = /language-(\w+)/.exec(childProps.className ?? "")?.[1];
-      }
-      return {children};
-    },
-  };
-}
-
-// ---------------------------------------------------------------------------
-// Component
-// ---------------------------------------------------------------------------
+import { RichContent } from "../rich-content";
 
 interface ReadonlyContentProps {
   content: string;
   className?: string;
   /**
-   * Attachments associated with the surrounding entity (comment / issue
-   * body). When the markdown contains an inline `` or file card whose
-   * URL matches one of these attachments, the download button re-signs the
-   * URL at click time via `useDownloadAttachment` instead of opening the
-   * potentially stale link embedded in the markdown.
-   *
-   * Callers SHOULD pass a stable reference (e.g. the field on a memoized
-   * timeline entry); a fresh array on every parent render busts the memo.
+   * Attachments associated with the surrounding entity (comment / issue body).
+   * Callers SHOULD pass a stable reference; a fresh array on every parent
+   * render busts the memo.
    */
   attachments?: Attachment[];
 }
 
-// Memoized so a long timeline of comments (Inbox + IssueDetail) does not
-// re-run the full react-markdown + rehype-* + lowlight pipeline on every
-// parent re-render. Props are `content`/`className`/`attachments`, all
-// shallow-comparable; stability is the caller's responsibility for the
-// array.
 export const ReadonlyContent = memo(function ReadonlyContent({
   content,
   className,
   attachments,
 }: ReadonlyContentProps) {
-  const processed = useMemo(
-    () =>
-      highlightToHtml(
-        preprocessMarkdown(content, { autolinkIssueIdentifiers: true }),
-      ),
-    [content],
-  );
-  const wrapperRef = useRef(null);
-  const hover = useLinkHover(wrapperRef);
-
-  // Components map is now static — all attachment-aware logic lives in
-  // , which reads the surrounding AttachmentDownloadProvider.
-  const components = useMemo(() => buildComponents(), []);
-
-  // Memoize the whole react-markdown subtree on its only real inputs
-  // (`processed` + `components`). Unrelated parent re-renders (e.g. a sibling
-  // agent task streaming over WebSocket fires one every ~100ms) would otherwise
-  // re-run react-markdown, which hands `` a fresh `dangerouslySetInnerHTML`
-  // object each time; React then rewrites the highlighted innerHTML even though
-  // the HTML string is byte-identical, tearing down and rebuilding every hljs
-  //  — which collapses any active text selection inside a code block
-  // (MUL-3621). A stable element reference lets React bail out of the subtree.
-  const markdown = useMemo(
-    () => (
-      
-        {processed}
-      
-    ),
-    [processed, components],
-  );
-
   return (
-    
-      
- {markdown} - -
-
+ ); }); diff --git a/packages/views/editor/utils/preprocess.ts b/packages/views/editor/utils/preprocess.ts index 203aaf0a7ef..61f5aefa7e7 100644 --- a/packages/views/editor/utils/preprocess.ts +++ b/packages/views/editor/utils/preprocess.ts @@ -4,7 +4,6 @@ import { preprocessFileCards, preprocessIssueIdentifiers, } from "@multica/ui/markdown"; -import { configStore } from "@multica/core/config"; /** * Preprocess a markdown string before loading into Tiptap via contentType: 'markdown'. @@ -28,13 +27,22 @@ import { configStore } from "@multica/core/config"; * identifier string (not a real UUID) and corrupt the saved markdown. Only the * readonly renderer (which resolves the identifier to a UUID at render time) * passes it. + * + * `cdnDomain` is an explicit parameter rather than an imperative + * `configStore.getState()` read inside this function. The CDN config arrives + * asynchronously after auth, so a one-shot read made this transform silently + * time-dependent: content rendered before the config landed kept its legacy CDN + * links as plain anchors forever, because nothing re-ran the transform. Passing + * the value in lets a reactive caller (RichContent, which subscribes to the + * store) put it in its memo dependencies, while a one-shot caller (the Tiptap + * editor, which preprocesses once at load) can still read the store itself. */ export function preprocessMarkdown( markdown: string, - opts?: { autolinkIssueIdentifiers?: boolean }, + opts: { cdnDomain: string; autolinkIssueIdentifiers?: boolean }, ): string { if (!markdown) return ""; - const cdnDomain = configStore.getState().cdnDomain; + const { cdnDomain } = opts; const step1 = preprocessMentionShortcodes(markdown); const step2 = opts?.autolinkIssueIdentifiers ? preprocessIssueIdentifiers(step1) diff --git a/packages/views/package.json b/packages/views/package.json index 9855631213b..dd63d1649fe 100644 --- a/packages/views/package.json +++ b/packages/views/package.json @@ -11,7 +11,6 @@ "exports": { "./navigation": "./navigation/index.ts", "./common/actor-avatar": "./common/actor-avatar.tsx", - "./common/markdown": "./common/markdown.tsx", "./editor": "./editor/index.ts", "./issues/components": "./issues/components/index.ts", "./issues/hooks": "./issues/hooks/index.ts", @@ -89,6 +88,7 @@ "hast-util-to-html": "^9.0.5", "katex": "catalog:", "lowlight": "^3.3.0", + "mdast-util-from-markdown": "^2.0.3", "mermaid": "catalog:", "motion": "^12.38.0", "pinyin-pro": "3.26.0", diff --git a/packages/views/rich-content/cdn-config-reactivity.test.tsx b/packages/views/rich-content/cdn-config-reactivity.test.tsx new file mode 100644 index 00000000000..e3684a66a0e --- /dev/null +++ b/packages/views/rich-content/cdn-config-reactivity.test.tsx @@ -0,0 +1,128 @@ +/** + * Late CDN config must reprocess already-rendered content (MUL-4922). + * + * The CDN domain is fetched asynchronously after auth. File-card detection + * happens in the markdown preprocess step, which used to read the domain once, + * imperatively, at render time — so content that rendered before the config + * landed kept its legacy CDN links as plain anchors permanently. Nothing + * re-ran the transform, because the memo only depended on `content`. + * + * This drives the real store: render with an empty domain, then publish the + * config the way AuthInitializer does, and assert the block upgrades. + */ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { act, render } from "@testing-library/react"; +import { configStore } from "@multica/core/config"; + +vi.mock("../issues/hooks", () => ({ + useResolveIssueIdentifier: () => null, +})); + +vi.mock("../i18n", async () => { + const editor = (await import("../locales/en/editor.json")).default; + return { + useT: () => ({ + t: (select: (bundle: typeof editor) => string) => select(editor), + }), + useTimeAgo: () => "just now", + }; +}); + +vi.mock("@multica/core/api", () => ({ + api: { getAttachmentTextContent: vi.fn() }, + PreviewTooLargeError: class extends Error {}, + PreviewUnsupportedError: class extends Error {}, +})); + +vi.mock("@multica/core/paths", () => ({ + useWorkspacePaths: () => ({ + issueDetail: (id: string) => `/acme/issues/${id}`, + projectDetail: (id: string) => `/acme/projects/${id}`, + }), + useWorkspaceSlug: () => "acme", +})); + +vi.mock("../navigation", () => ({ + useNavigation: () => ({ push: vi.fn(), openInNewTab: vi.fn() }), + AppLink: ({ href, children }: { href: string; children: React.ReactNode }) => ( + {children} + ), +})); + +vi.mock("../issues/components/issue-mention-card", () => ({ + IssueMentionCard: ({ issueId }: { issueId: string }) => {issueId}, +})); + +vi.mock("../projects/components/project-chip", () => ({ + ProjectChip: ({ projectId }: { projectId: string }) => {projectId}, +})); + +vi.mock("../editor/link-hover-card", () => ({ + useLinkHover: () => ({}), + LinkHoverCard: () => null, +})); + +// Surface what the file-card renderer received, so the assertion is about the +// emitted attachment rather than incidental chrome. +vi.mock("../editor/attachment", () => ({ + Attachment: ({ attachment }: { attachment: { url: string; filename: string } }) => ( + + {attachment.filename} + + ), +})); + +import { RichContent } from "./rich-content"; + +const CDN_DOMAIN = "multica-static.example.com"; +const FILE_URL = `https://${CDN_DOMAIN}/workspaces/w1/files/report.pdf`; +// Legacy file-card syntax is a link ON ITS OWN LINE (FILE_LINK_LINE is +// anchored); an inline link is deliberately not a card. +const CONTENT = `Attached:\n\n[report.pdf](${FILE_URL})`; + +beforeEach(() => { + configStore.setState({ cdnDomain: "", cdnSigned: false }); +}); + +describe("RichContent CDN config reactivity", () => { + it("upgrades a legacy CDN link to a file card when the config arrives late", () => { + const { container } = render(); + + // Config has not landed: the CDN host is unknown, so this is still a link. + expect(container.querySelector("[data-testid='file-card']")).toBeNull(); + expect(container.querySelector(`a[href="${FILE_URL}"]`)).not.toBeNull(); + + // AuthInitializer publishes the config after its background fetch resolves. + act(() => { + configStore.getState().setCdnConfig({ cdnDomain: CDN_DOMAIN }); + }); + + const card = container.querySelector("[data-testid='file-card']"); + expect(card).not.toBeNull(); + expect(card?.getAttribute("data-url")).toBe(FILE_URL); + }); + + it("renders the file card immediately when the config is already present", () => { + configStore.setState({ cdnDomain: CDN_DOMAIN }); + + const { container } = render(); + + expect(container.querySelector("[data-testid='file-card']")).not.toBeNull(); + }); + + it("leaves non-CDN links alone when the config arrives", () => { + // Same shape as CONTENT — only the hostname differs — so the assertion is + // about CDN matching, not about link placement. + const external = "Attached:\n\n[the spec](https://example.com/spec.pdf)"; + const { container } = render(); + + act(() => { + configStore.getState().setCdnConfig({ cdnDomain: CDN_DOMAIN }); + }); + + expect(container.querySelector("[data-testid='file-card']")).toBeNull(); + expect( + container.querySelector('a[href="https://example.com/spec.pdf"]'), + ).not.toBeNull(); + }); +}); diff --git a/packages/views/rich-content/index.ts b/packages/views/rich-content/index.ts new file mode 100644 index 00000000000..b1bbd9eca00 --- /dev/null +++ b/packages/views/rich-content/index.ts @@ -0,0 +1,12 @@ +export { + RichContent, + type RichContentProps, + type RichContentDensity, + type RichContentPhase, +} from "./rich-content"; +export { + isRichFenceLanguage, + shouldUpgradeFence, + type RichFenceLanguage, +} from "./rich-code-block"; +export { computeClosedFenceOffsets } from "./streaming-fence"; diff --git a/packages/views/rich-content/lazy-rich-block.test.tsx b/packages/views/rich-content/lazy-rich-block.test.tsx new file mode 100644 index 00000000000..6cb6958e128 --- /dev/null +++ b/packages/views/rich-content/lazy-rich-block.test.tsx @@ -0,0 +1,395 @@ +/** + * Near-viewport lazy shell (MUL-4922 performance contract). + * + * jsdom has no IntersectionObserver, so these tests install a controllable + * fake: it records observed elements and lets a test decide when a block + * becomes near-viewport. That makes the deferral, the latch and the size + * reservation observable rather than assumed. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, render, screen } from "@testing-library/react"; +import { renderToString } from "react-dom/server"; +import { createRoot, hydrateRoot } from "react-dom/client"; +import { flushSync } from "react-dom"; +import { LazyRichBlock } from "./lazy-rich-block"; +import { RichContentScrollRootProvider } from "./scroll-root"; +import { resetMountedBlocks } from "./mounted-block-registry"; + +type IOCallback = (entries: { isIntersecting: boolean }[]) => void; + +interface FakeObserver { + callback: IOCallback; + options?: IntersectionObserverInit; + observed: Element[]; + disconnected: boolean; +} + +let observers: FakeObserver[] = []; + +beforeEach(() => { + observers = []; + resetMountedBlocks(); + class FakeIntersectionObserver { + private readonly self: FakeObserver; + constructor(callback: IOCallback, options?: IntersectionObserverInit) { + this.self = { callback, options, observed: [], disconnected: false }; + observers.push(this.self); + } + observe(el: Element) { + this.self.observed.push(el); + } + disconnect() { + this.self.disconnected = true; + } + unobserve() {} + takeRecords() { + return []; + } + } + vi.stubGlobal("IntersectionObserver", FakeIntersectionObserver); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +/** Fire the near-viewport signal on every live observer. */ +function enterViewport() { + act(() => { + for (const o of observers) o.callback([{ isIntersecting: true }]); + }); +} + +const Expensive = () =>
diagram
; + +describe("LazyRichBlock", () => { + it("does not mount its child until the block is near the viewport", () => { + render( + + + , + ); + + expect(screen.queryByTestId("expensive")).toBeNull(); + expect(observers).toHaveLength(1); + expect(observers[0]?.observed).toHaveLength(1); + }); + + it("mounts the child once it becomes near-viewport", () => { + render( + + + , + ); + expect(screen.queryByTestId("expensive")).toBeNull(); + + enterViewport(); + + expect(screen.getByTestId("expensive")).toBeInTheDocument(); + }); + + // The latch: re-running Mermaid / rebuilding an iframe on every scroll pass + // would be worse than mounting eagerly once. + it("stops observing after mounting so the block never unmounts", () => { + render( + + + , + ); + enterViewport(); + + expect(observers[0]?.disconnected).toBe(true); + + // A later "left the viewport" signal must not tear the block down. + act(() => { + observers[0]?.callback([{ isIntersecting: false }]); + }); + expect(screen.getByTestId("expensive")).toBeInTheDocument(); + }); + + // Howard's stated risk: lazy mounting must not disturb Virtuoso's height + // measurement. The shell therefore reserves the same space before and after. + it("reserves the same height before and after mount", () => { + const { container } = render( + + + , + ); + const shell = container.querySelector("[data-rich-block-shell]") as HTMLElement; + + expect(shell.style.minHeight).toBe("480px"); + expect(shell.hasAttribute("data-mounted")).toBe(false); + + enterViewport(); + + expect(shell.style.minHeight).toBe("480px"); + expect(shell.hasAttribute("data-mounted")).toBe(true); + }); + + it("watches an area larger than the viewport so blocks are ready on arrival", () => { + render( + + + , + ); + + const margin = observers[0]?.options?.rootMargin ?? ""; + const topPx = Number.parseInt(margin.split(" ")[0] ?? "0", 10); + // Must exceed the chat list's own overscan (600px bottom) or a block would + // still be blank when Virtuoso has already rendered its row. + expect(topPx).toBeGreaterThan(600); + }); + + // Chat scrolls inside its own element. With the default (viewport) root, + // rootMargin expands the wrong box, so a block only loads once it is already + // visible — the preloading is silently dead in the surface that needs it most. + it("observes against the surface's scroll root when one is provided", () => { + const scrollRoot = document.createElement("div"); + document.body.appendChild(scrollRoot); + + render( + + + + + , + ); + + expect(observers[0]?.options?.root).toBe(scrollRoot); + scrollRoot.remove(); + }); + + it("falls back to the viewport root for page-scrolled surfaces", () => { + // Issue description / Comment scroll with the page; a null root is correct. + render( + + + , + ); + + expect(observers[0]?.options?.root ?? null).toBeNull(); + }); + + it("mounts via an effect when IntersectionObserver is unavailable", () => { + vi.unstubAllGlobals(); + vi.stubGlobal("IntersectionObserver", undefined); + + render( + + + , + ); + + // Degrading to eager mount is correct; rendering nothing would not be. + // The mount happens in an effect (not in the initial state) so the first + // committed frame still matches what a server render would produce — see + // the SSR suite below. + expect(screen.getByTestId("expensive")).toBeInTheDocument(); + }); +}); + +/** + * Virtualized-row recycling. + * + * Chat's list unmounts rows that scroll far enough away. A `mounted` flag held + * only in component state disappears with the row, so scrolling back would + * re-run Mermaid, rebuild the sandboxed iframe and drop the viewer's pan/zoom — + * turning the one-time mount cost into a per-pass cost. The latch therefore + * lives outside the component, keyed by content. + */ +describe("LazyRichBlock across row recycling", () => { + const SOURCE = "flowchart LR\n A --> B"; + + beforeEach(() => { + resetMountedBlocks(); + }); + + it("re-mounts a recycled block immediately, without waiting to be seen again", () => { + const first = render( + + + , + ); + enterViewport(); + expect(first.getByTestId("expensive")).toBeInTheDocument(); + + // Virtuoso recycles the row: the whole subtree is unmounted. + first.unmount(); + observers = []; + + // The row comes back. No intersection is reported this time — the block was + // already built once, so it must not wait to be observed again. + const second = render( + + + , + ); + + expect(second.getByTestId("expensive")).toBeInTheDocument(); + }); + + it("does not resurrect a different block that was never mounted", () => { + const first = render( + + + , + ); + enterViewport(); + first.unmount(); + observers = []; + + // Different content => different key => still deferred. + const other = render( + + + , + ); + + expect(other.queryByTestId("expensive")).toBeNull(); + }); + + it("keeps deferring when no sourceKey is supplied", () => { + // Without an identity there is nothing to remember; the block must not be + // treated as already-mounted. + const first = render( + + + , + ); + enterViewport(); + first.unmount(); + observers = []; + + const second = render( + + + , + ); + + expect(second.queryByTestId("expensive")).toBeNull(); + }); +}); + +/** + * Server/client determinism. + * + * The initial `mounted` state must not be derived from feature detection. If it + * were, the server (no `window`) and the browser (has IntersectionObserver) + * would disagree on the very first frame: React would find different markup + * during hydration, and the server render would silently bypass the lazy gate + * it exists to enforce. `"use client"` does not exempt a component from Next's + * server render, so this has to hold. + */ +describe("LazyRichBlock SSR", () => { + /** + * Render the way a server would. + * + * jsdom always provides `window`, so simply calling renderToString here would + * take the SAME feature-detection branch as the browser and could not observe + * the mismatch at all. Removing IntersectionObserver for the duration + * reproduces the real asymmetry: on a server the detection says + * "unsupported", in the browser it says "supported". A component whose first + * frame depends on that answer renders differently in the two environments — + * which is precisely the bug. + */ + function serverRender(ui: Parameters[0]): string { + const browserObserver = globalThis.IntersectionObserver; + vi.stubGlobal("IntersectionObserver", undefined); + try { + return renderToString(ui); + } finally { + vi.stubGlobal("IntersectionObserver", browserObserver); + } + } + + it("renders the placeholder, not the block, on the server", () => { + const html = serverRender( + + + , + ); + + expect(html).toContain("data-rich-block-shell"); + // The expensive subtree must not be in server output: SSR has no viewport, + // so nothing is near it. + expect(html).not.toContain("expensive"); + expect(html).not.toContain("data-mounted"); + }); + + it("hydrates the server markup without a mismatch", async () => { + const html = serverRender( + + + , + ); + + const container = document.createElement("div"); + container.innerHTML = html; + document.body.appendChild(container); + + const errors: string[] = []; + const errorSpy = vi.spyOn(console, "error").mockImplementation((...args) => { + errors.push(args.map(String).join(" ")); + }); + + let root: ReturnType | undefined; + await act(async () => { + root = hydrateRoot( + container, + + + , + ); + }); + + const hydrationErrors = errors.filter((e) => + /hydrat|did not match|mismatch/i.test(e), + ); + expect(hydrationErrors).toEqual([]); + + errorSpy.mockRestore(); + await act(async () => { + root?.unmount(); + }); + container.remove(); + }); + + it("produces the same first frame on server and client", () => { + // Same component, both environments, IntersectionObserver present on the + // client — the frames must be identical before any effect runs. + const serverHtml = serverRender( + + + , + ); + + const container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + // No act(): flushing effects is exactly what we must NOT do here, because + // the comparison is against the first committed frame. + flushSync(() => { + root.render( + + + , + ); + }); + const clientFirstFrame = container.innerHTML; + + // Normalize inline-style serialization only: React's server renderer emits + // `min-height:280px` while the browser's CSSOM round-trips it as + // `min-height: 280px;`. That difference is cosmetic — React hydration does + // not string-compare markup — and the hydration test above is the + // authoritative check. Everything else is compared verbatim. + const normalize = (html: string) => + html.replace(/style="([^"]*)"/g, (_, css: string) => + `style="${css.replace(/\s*;\s*$/, "").replace(/:\s+/g, ":")}"`, + ); + + expect(normalize(clientFirstFrame)).toBe(normalize(serverHtml)); + + act(() => root.unmount()); + container.remove(); + }); +}); diff --git a/packages/views/rich-content/lazy-rich-block.tsx b/packages/views/rich-content/lazy-rich-block.tsx new file mode 100644 index 00000000000..605aa28c853 --- /dev/null +++ b/packages/views/rich-content/lazy-rich-block.tsx @@ -0,0 +1,172 @@ +"use client"; + +/** + * LazyRichBlock — near-viewport, stable-size mount gate for rich blocks + * (MUL-4922 performance contract). + * + * A long chat session or a long comment thread can contain dozens of Mermaid + * diagrams and sandboxed HTML iframes. Instantiating them all at once costs a + * Mermaid render (async, layout-heavy) and an iframe (its own document + + * script execution) per block, whether or not the user ever scrolls to them. + * This shell defers each rich leaf until it is near the viewport. + * + * Two properties make this safe inside Virtuoso's virtualized list: + * + * 1. STABLE SIZE. The shell reserves the block's expected height BEFORE mounting + * and keeps that reservation as a `min-height` afterwards. Without it, a + * block would measure 0px while off-screen and jump to its real height on + * mount — which is precisely the measurement churn that makes a virtualized + * list mis-estimate item sizes and lose scroll position. The reservation is + * not a guess local to this file: it comes from the leaf components + * themselves (a session-cached real diagram height when available, else their + * documented skeleton/iframe height), so a cache hit mounts with zero shift. + * + * 2. MOUNT-ONCE LATCH. Once mounted the block is never unmounted, even when + * scrolled far away. Unmounting would re-run Mermaid and rebuild the iframe + * on every pass, and would discard the viewer's pan/zoom state — trading a + * one-time cost for a repeated one. Memory is bounded by "blocks the user + * actually scrolled past", not by the whole transcript. + */ + +import { + useEffect, + useLayoutEffect, + useRef, + useState, + type ReactNode, +} from "react"; +import { useRichContentScrollRoot } from "./scroll-root"; +import { + hasBlockMounted, + markBlockMounted, + mountedBlockKey, +} from "./mounted-block-registry"; + +// Restoring an already-mounted block must happen before paint, or a recycled +// Virtuoso row would flash its placeholder on the way back into view. +// useLayoutEffect does not run on the server (and warns if called there), so the +// server takes the plain-effect branch — neither runs during SSR, so the first +// frame is unaffected either way. +const useIsomorphicLayoutEffect = + typeof window !== "undefined" ? useLayoutEffect : useEffect; + +/** + * How far outside the viewport a block starts mounting. Sized to cover + * Virtuoso's own overscan (`increaseViewportBy` is 400px top / 600px bottom in + * the chat list) so a block is ready by the time it is scrolled into view, + * without eagerly building the whole transcript. + */ +const NEAR_VIEWPORT_ROOT_MARGIN = "800px 0px"; + +function supportsIntersectionObserver(): boolean { + return typeof window !== "undefined" && typeof window.IntersectionObserver === "function"; +} + +export function LazyRichBlock({ + reservedHeightPx, + sourceKey, + children, +}: { + /** Expected height of the mounted block; reserved before and after mount. */ + reservedHeightPx: number; + /** + * Stable identity for this block's content. Used to remember that it has + * already been mounted, so a virtualized row that gets recycled and later + * re-created does not pay the mount cost again. + */ + sourceKey?: string; + children: ReactNode; +}) { + const ref = useRef(null); + const scrollRoot = useRichContentScrollRoot(); + const registryKey = sourceKey == null ? null : mountedBlockKey("rich", sourceKey); + // ALWAYS false on the first render, on both server and client. + // + // Deriving this from feature detection (`typeof window`, IntersectionObserver + // presence) would make the first frame environment-dependent: the server, with + // no `window`, would render the full Mermaid/HTML subtree while the browser's + // hydration pass renders a placeholder — a markup mismatch, and an SSR that + // silently bypasses the lazy gate it is supposed to honour. `"use client"` + // does not opt a component out of Next's server render, so the only safe + // initial state is the one both environments can agree on. + // + // Everything environment-specific happens in the effect below, which never + // runs on the server. + const [mounted, setMounted] = useState(false); + + // Restore a block this session has already built. Kept out of the initial + // state for the same reason as everything else here: the registry is empty on + // the server, so seeding from it would make the first frame differ between + // server and client. + useIsomorphicLayoutEffect(() => { + if (mounted || registryKey == null) return; + if (hasBlockMounted(registryKey)) setMounted(true); + }, [mounted, registryKey]); + + useEffect(() => { + if (mounted) return; + + // No IntersectionObserver (jsdom, older webviews): fall back to mounting + // eagerly. This runs in an effect rather than in the initial state so the + // first committed frame still matches the server's. + if (!supportsIntersectionObserver()) { + setMounted(true); + return; + } + + const el = ref.current; + if (!el) return; + + const observer = new IntersectionObserver( + (entries) => { + if (entries.some((entry) => entry.isIntersecting)) { + // Latch: stop observing so the block never unmounts on scroll-away. + setMounted(true); + observer.disconnect(); + } + }, + { + // Clip against the surface's own scroll container when there is one. + // With the default (viewport) root, rootMargin would expand the wrong + // box and a Chat block would only load once already visible. + root: scrollRoot, + rootMargin: NEAR_VIEWPORT_ROOT_MARGIN, + }, + ); + observer.observe(el); + return () => observer.disconnect(); + }, [mounted, scrollRoot]); + + // Record the mount so a recycled row can skip straight to the built block. + useEffect(() => { + if (mounted && registryKey != null) markBlockMounted(registryKey); + }, [mounted, registryKey]); + + return ( +
+ {mounted ? children : } +
+ ); +} + +/** + * Pre-mount placeholder. Deliberately inert and unlabelled-as-loading: it fills + * reserved space rather than announcing work, because nothing is loading yet — + * the block simply has not been scrolled near. + */ +function RichBlockPlaceholder() { + return ( +