diff --git a/ui/src/components/MarkdownEditor.test.tsx b/ui/src/components/MarkdownEditor.test.tsx index ebe0de0de2..6d3ff02e92 100644 --- a/ui/src/components/MarkdownEditor.test.tsx +++ b/ui/src/components/MarkdownEditor.test.tsx @@ -11,6 +11,7 @@ import { isSameAutocompleteSession, issueMentionTitle, MarkdownEditor, + type MarkdownEditorRef, type MentionOption, placeCaretAfterMentionAnchor, shouldAcceptAutocompleteKey, @@ -22,12 +23,28 @@ const mdxEditorMockState = vi.hoisted(() => ({ emitMountParseError: false, emitMountSilentEmptyState: false, throwOnRender: false, + /** Markdown the mock emits through `onChange` once mounted, as the real editor would export it. */ + emitMountChange: null as string | null, + /** Milliseconds the mock waits before painting its content, to model a slow import. */ + populateDelayMs: 0, markdownValues: [] as string[], + /** Every string handed to the editor's imperative `insertMarkdown`. */ + insertedMarkdownValues: [] as string[], suppressHtmlProcessingValues: [] as boolean[], })); -function containsHtmlLikeTag(markdown: string) { - return /<\/?[A-Za-z][A-Za-z0-9:-]*(?:\s[^>]*)?\/?>/.test(markdown); +/** + * Stand-in for the real importer's HTML tokenizer. MDXEditor runs with + * `suppressHtmlProcessing`, which *removes* the HTML visitor — so a bare `<` + * that opens an HTML construct is what throws + * `UnrecognizedMarkdownConstructError`, regardless of the flag. Only a + * backslash escape (`\<`) hides the bracket from the parser, which is exactly + * what `escapeUnsupportedAngleBrackets` produces. + */ +function containsUnescapedHtmlLikeTag(markdown: string) { + // Consume escape pairs first so `\<` counts as literal text, then look for a + // surviving `<` that would open an HTML construct. + return /(?:^|[^\\])(?:\\\\)*<[A-Za-z!/?]/.test(markdown); } vi.mock("@mdxeditor/editor", async () => { @@ -59,7 +76,11 @@ vi.mock("@mdxeditor/editor", async () => { suppressHtmlProcessing?: boolean; className?: string; }, - forwardedRef: React.ForwardedRef<{ setMarkdown: (value: string) => void; focus: () => void } | null>, + forwardedRef: React.ForwardedRef<{ + setMarkdown: (value: string) => void; + insertMarkdown: (value: string) => void; + focus: (callback?: () => void) => void; + } | null>, ) { if (mdxEditorMockState.throwOnRender) { throw new Error("Rich editor render crashed"); @@ -68,22 +89,44 @@ vi.mock("@mdxeditor/editor", async () => { mdxEditorMockState.suppressHtmlProcessingValues.push(Boolean(suppressHtmlProcessing)); const [content, setContent] = React.useState(markdown); const editableRef = React.useRef(null); + const onErrorRef = React.useRef(onError); + onErrorRef.current = onError; const handle = React.useMemo(() => ({ setMarkdown: (value: string) => setContent(value), - focus: () => editableRef.current?.focus(), + insertMarkdown: (value: string) => { + mdxEditorMockState.insertedMarkdownValues.push(value); + // Inserted markdown goes through the same importer as the mounted + // document, so an unescaped tag fails here in exactly the same way. + if (containsUnescapedHtmlLikeTag(value)) { + onErrorRef.current?.({ error: "Unrecognized markdown construct: html", source: value }); + return; + } + setContent((previous) => `${previous}${value}`); + }, + // The real `focus` runs its callback once a selection exists. + focus: (callback?: () => void) => { + editableRef.current?.focus(); + callback?.(); + }, }), []); React.useEffect(() => { - if (!suppressHtmlProcessing && containsHtmlLikeTag(markdown)) { + if (containsUnescapedHtmlLikeTag(markdown)) { setContent(""); onError?.({ - error: "Error parsing markdown: HTML-like formatting requires suppressHtmlProcessing", + error: "Unrecognized markdown construct: html", source: markdown, }); return; } + if (mdxEditorMockState.populateDelayMs > 0) { + // Model an import that paints its content some time after mount. + setContent(""); + const timer = window.setTimeout(() => setContent(markdown), mdxEditorMockState.populateDelayMs); + return () => window.clearTimeout(timer); + } setContent(markdown); - }, [markdown, onError, suppressHtmlProcessing]); + }, [markdown, onError]); React.useEffect(() => { setForwardedRef(forwardedRef, null); @@ -103,6 +146,9 @@ vi.mock("@mdxeditor/editor", async () => { source: markdown, }); } + if (mdxEditorMockState.emitMountChange !== null) { + onChange?.(mdxEditorMockState.emitMountChange); + } }, 0); return () => { window.clearTimeout(timer); @@ -118,7 +164,8 @@ vi.mock("@mdxeditor/editor", async () => { contentEditable suppressContentEditableWarning > - {content || placeholder || ""} + {/* The real editor paints resolved text, never the escapes that carried it in. */} + {content.replace(/\\ ); }); @@ -167,6 +214,28 @@ async function flush() { }); } +function clickRetryRichEditor(scope: HTMLElement) { + const button = Array.from(scope.querySelectorAll("button")).find((candidate) => + candidate.textContent?.includes("Retry rich editor"), + ); + if (!button) throw new Error('"Retry rich editor" button not found'); + button.dispatchEvent(new MouseEvent("click", { bubbles: true })); +} + +function fallbackCode(scope: HTMLElement) { + return scope.querySelector('[data-testid="markdown-editor-fallback-code"]')?.textContent; +} + +/** + * The DOM-empty heuristic checks at 100ms and confirms 200ms later, so a test + * that wants to prove no fallback happened has to outlast both phases. + */ +async function waitPastEmptyHeuristic() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 400)); + }); +} + function createFileDragEvent(type: string) { const event = ( typeof DragEvent === "function" @@ -244,7 +313,10 @@ describe("MarkdownEditor", () => { mdxEditorMockState.emitMountParseError = false; mdxEditorMockState.emitMountSilentEmptyState = false; mdxEditorMockState.throwOnRender = false; + mdxEditorMockState.emitMountChange = null; + mdxEditorMockState.populateDelayMs = 0; mdxEditorMockState.markdownValues = []; + mdxEditorMockState.insertedMarkdownValues = []; mdxEditorMockState.suppressHtmlProcessingValues = []; }); @@ -464,6 +536,7 @@ describe("MarkdownEditor", () => { expect(textarea).not.toBeNull(); expect(textarea?.value).toBe("Affected versions: <= v0.3.1"); expect(container.textContent).toContain("Rich editor unavailable for this markdown"); + expect(fallbackCode(container)).toBe("MDE-PARSE"); expect(handleChange).not.toHaveBeenCalled(); await act(async () => { @@ -494,6 +567,7 @@ describe("MarkdownEditor", () => { expect(textarea).not.toBeNull(); expect(textarea?.value).toBe("5. python3 circleback/sync_insights.py --input -- writes insights//*.md"); expect(container.textContent).toContain("Rich editor unavailable for this markdown"); + expect(fallbackCode(container)).toBe("MDE-RENDER"); expect(consoleError).toHaveBeenCalledWith( "Markdown rich editor failed; falling back to raw textarea", expect.objectContaining({ @@ -532,6 +606,7 @@ describe("MarkdownEditor", () => { expect(textarea).not.toBeNull(); expect(textarea?.value).toBe("Affected versions: <= v0.3.1"); expect(container.textContent).toContain("Rich editor unavailable for this markdown"); + expect(fallbackCode(container)).toBe("MDE-EMPTY"); expect(handleChange).not.toHaveBeenCalled(); await act(async () => { @@ -539,6 +614,293 @@ describe("MarkdownEditor", () => { }); }); + it("keeps prose with bare angle brackets in the rich editor", async () => { + const value = "Affected versions: <= v0.3.1\n\nRename to the real name."; + const handleChange = vi.fn(); + const root = createRoot(container); + + await act(async () => { + root.render( + , + ); + }); + + await flush(); + await waitPastEmptyHeuristic(); + + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).not.toContain("Rich editor unavailable for this markdown"); + + // The editor is handed the escaped form: the placeholder is hidden from the + // HTML tokenizer, while `<=` cannot open a tag and is left exactly as typed. + const received = mdxEditorMockState.markdownValues.at(-1); + expect(received).toContain("Rename \\ to the real name."); + expect(received).toContain("Affected versions: <= v0.3.1"); + + // What the user sees is still the markdown they wrote. + expect(container.textContent).toContain("Rename to the real name."); + expect(handleChange).not.toHaveBeenCalled(); + + await act(async () => { + root.unmount(); + }); + }); + + it("returns editor output to the parent with the transport escaping removed", async () => { + const value = "Affected versions: <= v0.3.1\n\nRename to the real name."; + // What the real exporter emits for this document: `<` re-escaped, `<=` bare. + mdxEditorMockState.emitMountChange = + "Affected versions: <= v0.3.1\n\nRename \\ to the real name."; + const handleChange = vi.fn(); + const root = createRoot(container); + + await act(async () => { + root.render( + , + ); + }); + + await flush(); + + // The parent stores the clean form — these fields feed agent prompts, so a + // stray `\<` would leak into a prompt. + expect(handleChange).toHaveBeenCalledWith(value); + expect(handleChange.mock.calls.every(([next]) => !String(next).includes("\\<"))).toBe(true); + + await act(async () => { + root.unmount(); + }); + }); + + it("does not notify the parent when a prop sync echoes escaped markdown back", async () => { + const handleChange = vi.fn(); + const root = createRoot(container); + + await act(async () => { + root.render( + , + ); + }); + + // The editor echoes an imperative `setMarkdown` back through `onChange` in + // editor space, while the component compares in stored space. + mdxEditorMockState.emitMountChange = "Rename \\ here"; + + await act(async () => { + root.render( + , + ); + }); + + await flush(); + await waitPastEmptyHeuristic(); + + expect(handleChange).not.toHaveBeenCalled(); + expect(container.querySelector("textarea")).toBeNull(); + + await act(async () => { + root.unmount(); + }); + }); + + it("recovers the rich editor on retry after a transient parse failure", async () => { + mdxEditorMockState.emitMountParseError = true; + const handleChange = vi.fn(); + const root = createRoot(container); + + await act(async () => { + root.render( + , + ); + }); + + await flush(); + await vi.waitFor(() => { + expect(container.querySelector("textarea")).not.toBeNull(); + }); + expect(fallbackCode(container)).toBe("MDE-PARSE"); + + // The failure was transient; the next mount imports the same markdown fine. + mdxEditorMockState.emitMountParseError = false; + + await act(async () => { + clickRetryRichEditor(container); + }); + await flush(); + await waitPastEmptyHeuristic(); + + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).not.toContain("Rich editor unavailable for this markdown"); + expect(container.textContent).toContain("Rename to the real name."); + expect(handleChange).not.toHaveBeenCalledWith(""); + + await act(async () => { + root.unmount(); + }); + }); + + it("re-arms the empty-onChange guard when the editor is retried", async () => { + // The sequence this guards: the editor mounts, reports an edit (spending the + // one-shot mount guard), then fails. The user retries, and the fresh mount + // emits the empty onChange that the guard exists to swallow — which would + // otherwise wipe the parent's value. + mdxEditorMockState.emitMountChange = "Rename \\ to the real name."; + mdxEditorMockState.emitMountParseError = true; + const handleChange = vi.fn(); + const root = createRoot(container); + + await act(async () => { + root.render( + , + ); + }); + + await flush(); + await vi.waitFor(() => { + expect(container.querySelector("textarea")).not.toBeNull(); + }); + + mdxEditorMockState.emitMountChange = null; + mdxEditorMockState.emitMountParseError = false; + mdxEditorMockState.emitMountEmptyReset = true; + + await act(async () => { + clickRetryRichEditor(container); + }); + await flush(); + await waitPastEmptyHeuristic(); + + expect(handleChange).not.toHaveBeenCalledWith(""); + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).toContain("Rename to the real name."); + + await act(async () => { + root.unmount(); + }); + }); + + it("does not fall back again while a retried editor is still populating", async () => { + mdxEditorMockState.emitMountParseError = true; + const root = createRoot(container); + + await act(async () => { + root.render( + {}} + placeholder="Markdown body" + />, + ); + }); + + await flush(); + await vi.waitFor(() => { + expect(container.querySelector("textarea")).not.toBeNull(); + }); + + // The retried editor paints its content between the first check and the + // confirming re-check, so the heuristic must not call it empty. + mdxEditorMockState.emitMountParseError = false; + mdxEditorMockState.populateDelayMs = 150; + + await act(async () => { + clickRetryRichEditor(container); + }); + await flush(); + await waitPastEmptyHeuristic(); + + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).not.toContain("Rich editor unavailable for this markdown"); + expect(container.textContent).toContain("Rename to the real name."); + + await act(async () => { + root.unmount(); + }); + }); + + it("escapes angle brackets in markdown inserted through the ref", async () => { + const editorRef = { current: null as MarkdownEditorRef | null }; + const root = createRoot(container); + + await act(async () => { + root.render( + {}} + placeholder="Markdown body" + />, + ); + }); + await flush(); + + await act(async () => { + editorRef.current?.insertMarkdown("\n\nRename to the real name."); + }); + await flush(); + await waitPastEmptyHeuristic(); + + expect(mdxEditorMockState.insertedMarkdownValues).toEqual([ + "\n\nRename \\ to the real name.", + ]); + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).not.toContain("Rich editor unavailable for this markdown"); + + await act(async () => { + root.unmount(); + }); + }); + + it("escapes angle brackets in pasted markdown", async () => { + const root = createRoot(container); + + await act(async () => { + root.render( + {}} placeholder="Markdown body" />, + ); + }); + await flush(); + + const scope = container.querySelector('[data-testid="mdx-editor"]')?.parentElement; + const pasted = "## Setup\n\n- Rename to the real name\n"; + const event = new Event("paste", { bubbles: true, cancelable: true }); + Object.defineProperty(event, "clipboardData", { + configurable: true, + value: { + types: ["text/plain"], + getData: (type: string) => (type === "text/plain" ? pasted : ""), + }, + }); + + await act(async () => { + scope?.dispatchEvent(event); + }); + await flush(); + await waitPastEmptyHeuristic(); + + expect(mdxEditorMockState.insertedMarkdownValues).toEqual([ + "## Setup\n\n- Rename \\ to the real name\n", + ]); + expect(container.querySelector("textarea")).toBeNull(); + expect(container.textContent).not.toContain("Rich editor unavailable for this markdown"); + + await act(async () => { + root.unmount(); + }); + }); + it("shows the editor-scoped dropzone by default when files are dragged over it", async () => { const root = createRoot(container); diff --git a/ui/src/components/MarkdownEditor.tsx b/ui/src/components/MarkdownEditor.tsx index 897097e9fb..34bcb9d947 100644 --- a/ui/src/components/MarkdownEditor.tsx +++ b/ui/src/components/MarkdownEditor.tsx @@ -51,6 +51,10 @@ import { MentionAwareLinkNode, mentionAwareLinkNodeReplacement } from "../lib/me import { mentionDeletionPlugin } from "../lib/mention-deletion"; import { looksLikeMarkdownPaste } from "../lib/markdownPaste"; import { normalizeMarkdown } from "../lib/normalize-markdown"; +import { + escapeUnsupportedAngleBrackets, + unescapeAngleBracketEscapes, +} from "../lib/angle-bracket-markdown"; import { unescapeBlockquoteMarkers } from "../lib/blockquote-markdown"; import { pasteNormalizationPlugin } from "../lib/paste-normalization"; import { cn } from "../lib/utils"; @@ -149,12 +153,33 @@ function convertHtmlImagesToMarkdown(text: string): string { }); } +/** + * Convert a stored value into the exact markdown handed to MDXEditor. + * + * The angle-bracket escape runs last, after the `` rewrite, because that + * rewrite has to match a bare ``) so `>`-prefixed content renders as a real // blockquote in the editor as well as on display (keeps import/export in sync). const withBlockquotes = unescapeBlockquoteMarkers(normalizedLineEndings); - return convertHtmlImagesToMarkdown(withBlockquotes); + const withImages = convertHtmlImagesToMarkdown(withBlockquotes); + // The editor runs with `suppressHtmlProcessing`, so a bare `<` that opens an + // HTML construct would throw and drop the whole component to raw source. + // Escaping those brackets keeps ordinary prose — ``, `` — in the + // rich editor. `toStoredMarkdown` reverses it on the way out. + return escapeUnsupportedAngleBrackets(withImages); +} + +/** + * Convert markdown exported by MDXEditor back into the form this product + * stores. Inverse of the rewrites `prepareMarkdownForEditor` applies, so the + * stored value never carries the editor's transport escaping — these fields + * feed agent prompts and must stay in the clean, human-authored form. + */ +function toStoredMarkdown(markdown: string): string { + return unescapeAngleBracketEscapes(unescapeBlockquoteMarkers(markdown)); } function escapeRegExp(value: string): string { @@ -222,6 +247,30 @@ function richEditorErrorMessage(error: unknown): string { return "Rich editor failed to render"; } +/** + * Why the rich editor was abandoned for this value. Surfaced in the fallback + * header so a bug report names the path that failed rather than describing the + * same generic message for three unrelated causes. + * + * - `MDE-PARSE` the markdown importer rejected the source + * - `MDE-RENDER` the editor threw while rendering + * - `MDE-EMPTY` the editor mounted but never painted the content + */ +type RichEditorErrorCode = "MDE-PARSE" | "MDE-RENDER" | "MDE-EMPTY"; + +interface RichEditorError { + code: RichEditorErrorCode; + message: string; +} + +/** + * The editor populates its DOM asynchronously, so "looks empty" is only + * trustworthy after it has had time to settle and then stayed empty. The first + * check debounces behind mutations; the second confirms the verdict once. + */ +const RICH_EDITOR_EMPTY_CHECK_MS = 100; +const RICH_EDITOR_EMPTY_CONFIRM_MS = 200; + /* ---- Mention detection helpers ---- */ interface MentionState { @@ -663,7 +712,7 @@ export const MarkdownEditor = forwardRef const echoIgnoreMarkdownRef = useRef(null); const [uploadError, setUploadError] = useState(null); const [isDragOver, setIsDragOver] = useState(false); - const [richEditorError, setRichEditorError] = useState(null); + const [richEditorError, setRichEditorError] = useState(null); const dragDepthRef = useRef(0); // Stable ref for imageUploadHandler so plugins don't recreate on every render @@ -735,7 +784,10 @@ export const MarkdownEditor = forwardRef // (an editor that was never focused). Focus first — the callback runs // once focus (and a selection: caret kept, else rootEnd) is in place. const editor = ref.current; - editor.focus(() => editor.insertMarkdown(markdown), { defaultSelection: "rootEnd" }); + // Inserted markdown reaches the same importer as the mounted document, so + // it needs the same angle-bracket escaping to survive it. + const editorMarkdown = escapeUnsupportedAngleBrackets(markdown); + editor.focus(() => editor.insertMarkdown(editorMarkdown), { defaultSelection: "rootEnd" }); return; } const textarea = fallbackTextareaRef.current; @@ -781,22 +833,44 @@ export const MarkdownEditor = forwardRef const container = containerRef.current; if (!container) return; - let timeoutId = 0; + let checkTimeoutId = 0; + let confirmTimeoutId = 0; + let mutationCount = 0; + + const looksEmpty = () => { + const editable = container.querySelector('[contenteditable="true"]'); + if (!(editable instanceof HTMLElement)) return false; + const activeElement = document.activeElement; + // A focused editor is the user's, not ours to second-guess. + if (activeElement === editable || editable.contains(activeElement)) return false; + return isRichEditorDomEmpty(editable, editorValue, placeholder); + }; + + // Two phases. Mounting (and especially remounting after "Retry rich + // editor", where focus sits on the button rather than the editor) leaves + // the editable momentarily empty, so a single immediate check reads a + // still-populating editor as a broken one. Wait, then confirm once; any + // mutation in between means content arrived and restarts the whole thing. const scheduleCheck = () => { - window.clearTimeout(timeoutId); - timeoutId = window.setTimeout(() => { - const editable = container.querySelector('[contenteditable="true"]'); - if (!(editable instanceof HTMLElement)) return; - const activeElement = document.activeElement; - if (activeElement === editable || editable.contains(activeElement)) return; - if (isRichEditorDomEmpty(editable, editorValue, placeholder)) { - setRichEditorError("Rich editor failed to load content"); - } - }, 0); + window.clearTimeout(checkTimeoutId); + window.clearTimeout(confirmTimeoutId); + checkTimeoutId = window.setTimeout(() => { + if (!looksEmpty()) return; + const mutationsAtCheck = mutationCount; + confirmTimeoutId = window.setTimeout(() => { + if (mutationCount !== mutationsAtCheck) return; + if (!looksEmpty()) return; + setRichEditorError({ + code: "MDE-EMPTY", + message: "Rich editor failed to load content", + }); + }, RICH_EDITOR_EMPTY_CONFIRM_MS); + }, RICH_EDITOR_EMPTY_CHECK_MS); }; scheduleCheck(); const observer = new MutationObserver(() => { + mutationCount += 1; scheduleCheck(); }); observer.observe(container, { @@ -806,7 +880,8 @@ export const MarkdownEditor = forwardRef }); return () => { - window.clearTimeout(timeoutId); + window.clearTimeout(checkTimeoutId); + window.clearTimeout(confirmTimeoutId); observer.disconnect(); }; }, [editorValue, placeholder, richEditorError]); @@ -836,7 +911,9 @@ export const MarkdownEditor = forwardRef latestValueRef.current = updated; echoIgnoreMarkdownRef.current = updated; ref.current?.setMarkdown(updated); - onChange(updated); + // `updated` derives from `latestValueRef`, which is editor + // space; the parent only ever sees stored space. + onChange(toStoredMarkdown(updated)); requestAnimationFrame(() => { ref.current?.focus(undefined, { defaultSelection: "rootEnd" }); }); @@ -1065,7 +1142,9 @@ export const MarkdownEditor = forwardRef latestValueRef.current = next; echoIgnoreMarkdownRef.current = next; ref.current?.setMarkdown(next); - onChange(next); + // `next` derives from `latestValueRef`, which is editor space; the + // parent only ever sees stored space. + onChange(toStoredMarkdown(next)); } const restoreSelection = (attemptsRemaining: number) => { @@ -1163,11 +1242,15 @@ export const MarkdownEditor = forwardRef if (!looksLikeMarkdownPaste(rawText)) return; event.preventDefault(); - ref.current.insertMarkdown(normalizeMarkdown(rawText)); + ref.current.insertMarkdown(escapeUnsupportedAngleBrackets(normalizeMarkdown(rawText))); }, []); - const handleRichEditorError = useCallback((error: unknown) => { - setRichEditorError(richEditorErrorMessage(error)); + const handleRichEditorRenderError = useCallback((error: unknown) => { + setRichEditorError({ code: "MDE-RENDER", message: richEditorErrorMessage(error) }); + }, []); + + const handleRichEditorParseError = useCallback((error: unknown) => { + setRichEditorError({ code: "MDE-PARSE", message: richEditorErrorMessage(error) }); }, []); const mentionMenuPosition = mentionState @@ -1189,11 +1272,20 @@ export const MarkdownEditor = forwardRef )} >
-

Rich editor unavailable for this markdown. Showing raw source instead.

+

+ Rich editor unavailable for this markdown. Showing raw source instead.{" "} + + {richEditorError.code} + +