Skip to content

Conversation

sawka
Copy link
Member

@sawka sawka commented Sep 12, 2025

No description provided.

Copy link
Contributor

coderabbitai bot commented Sep 12, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds Mermaid diagram support to Markdown rendering. package.json adds runtime deps: mermaid and unist-util-visit. A new remark plugin (remark-mermaid-to-tag.ts) converts fenced mermaid code blocks into HTML nodes wrapped with a mermaidblock element. frontend/app/element/markdown.tsx is extended: dynamic mermaid import with one-time initialization, a Mermaid React component that normalizes text and renders via mermaid.run with loading/error state, code-path routing for language-mermaid blocks, new helper getTextContent, mermaidblock handling in remarkPlugins and rehypeSanitize, and a defensive default for text assembly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive No pull request description was provided, so there is no author-written context beyond the title and diffs; under the check rules a very vague or empty description is treated as inconclusive. While the changes clearly add Mermaid support in Markdown, the absence of a description prevents confirmation of intent, testing instructions, or migration notes. Therefore this check cannot be conclusively passed. Request that the author add a short PR description summarizing what was changed and why, list added dependencies (mermaid, unist-util-visit), and include brief testing steps or any migration/compatibility notes; a one-paragraph description with these points will allow this check to pass.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "mermaid support in markdown" directly and concisely states the primary change: adding Mermaid diagram support to the Markdown renderer. It is short, specific, and aligned with the changes in the diff (new Mermaid rendering, remark plugin, and added dependencies). This satisfies the requirement for a clear single-sentence summary of the main change.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a6bf816 and daf4314.

📒 Files selected for processing (2)
  • frontend/app/element/markdown.tsx (6 hunks)
  • frontend/app/element/remark-mermaid-to-tag.ts (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
package.json (1)

124-124: Consider lazy-loading Mermaid to avoid shipping ~1MB to all renderer views.

Keep the dependency, but import it dynamically where used to reduce initial bundle and cold-start time.

I'll propose code changes in frontend/app/element/markdown.tsx to lazy-load it.

frontend/app/element/markdown.tsx (4)

77-82: More robust normalization.

Handle lone CRs and strip multiple trailing newlines.

-        let normalizedChart = chart
-            .replace(/<br\s*\/?>/gi, "\n") // Convert <br/> and <br> to newlines
-            .replace(/\r\n/g, "\n") // Normalize \r\n to \n
-            .replace(/\n$/, ""); // Remove final newline
+        let normalizedChart = chart
+            .replace(/<br\s*\/?>/gi, "\n")      // Convert <br/> and <br> to newlines
+            .replace(/\r\n?/g, "\n")            // Normalize CRLF/LF/CR to LF
+            .replace(/\n+$/, "");               // Trim trailing newlines

85-87: Remove prod logging; add error handling.

Avoid leaking diagram contents to console and handle render failures.

-        console.log("mermaid", normalizedChart);
-        mermaid.run({ nodes: [ref.current] });
+        mermaid.run({ nodes: [ref.current] }).catch((err) =>
+            console.warn("Mermaid render failed", err)
+        );

29-36: Set explicit security level and theme source.

Explicitly configure securityLevel: "strict" (default today, but make intent clear) and consider wiring theme to the app’s theme instead of hardcoding "dark".

If you want to lock this down now:

-        mermaid.initialize({ startOnLoad: false, theme: "dark" });
+        mermaid.initialize({
+            startOnLoad: false,
+            theme: "dark", // TODO: bind to app theme
+            securityLevel: "strict",
+        });

Would you like me to wire this to your theme store/hook?


15-15: Lazy-load Mermaid to trim initial bundle and speed up first render.

Importing at module top pulls Mermaid into every Markdown view. Prefer dynamic import on first use with one-time init.

Apply this refactor (paired with initializeMermaid changes):

-import mermaid from "mermaid";
+let mermaidMod: any | null = null;
+async function getMermaid() {
+    if (!mermaidMod) {
+        const m = await import("mermaid");
+        mermaidMod = m.default ?? m;
+        // Initialize once on first load
+        if (!mermaidInitialized) {
+            mermaidMod.initialize({ startOnLoad: false, theme: "dark", securityLevel: "strict" });
+            mermaidInitialized = true;
+        }
+    }
+    return mermaidMod;
+}

Then in Mermaid’s effect:

-        mermaid.run({ nodes: [ref.current] }).catch((err) =>
+        (await getMermaid())
+            .run({ nodes: [ref.current] })
+            .catch((err) =>
             console.warn("Mermaid render failed", err)
-        );
+        );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8874774 and 4f65387.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (2)
  • frontend/app/element/markdown.tsx (3 hunks)
  • package.json (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build for TestDriver.ai
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
frontend/app/element/markdown.tsx (1)

68-90: LGTM overall on the Mermaid component.

Ref/attribute reset and node-scoped render via run() is correct for re-renders.

Comment on lines +92 to 97
const Code = ({ className = "", children }: { className?: string; children: React.ReactNode }) => {
if (/\blanguage-mermaid\b/.test(className)) {
const text = Array.isArray(children) ? children.join("") : String(children ?? "");
return <Mermaid chart={text} />;
}
return <code className={className}>{children}</code>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix: copy/execute actions return empty text for Mermaid blocks.

When Code returns a Mermaid component, CodeBlock’s getTextContent traverses React children, not DOM textContent set later; result is empty copy/execute payload.

Apply this diff so the original source is still available for copy/execute:

-    if (/\blanguage-mermaid\b/.test(className)) {
-        const text = Array.isArray(children) ? children.join("") : String(children ?? "");
-        return <Mermaid chart={text} />;
-    }
+    if (/\blanguage-mermaid\b/.test(className)) {
+        const text = Array.isArray(children) ? children.join("") : String(children ?? "");
+        return (
+            <>
+                <Mermaid chart={text} />
+                <span style={{ display: "none" }} aria-hidden="true" data-raw={text}>
+                    {text}
+                </span>
+            </>
+        );
+    }

Optionally, also teach getTextContent to read data-raw if present.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const Code = ({ className = "", children }: { className?: string; children: React.ReactNode }) => {
if (/\blanguage-mermaid\b/.test(className)) {
const text = Array.isArray(children) ? children.join("") : String(children ?? "");
return <Mermaid chart={text} />;
}
return <code className={className}>{children}</code>;
const Code = ({ className = "", children }: { className?: string; children: React.ReactNode }) => {
if (/\blanguage-mermaid\b/.test(className)) {
const text = Array.isArray(children) ? children.join("") : String(children ?? "");
return (
<>
<Mermaid chart={text} />
<span style={{ display: "none" }} aria-hidden="true" data-raw={text}>
{text}
</span>
</>
);
}
return <code className={className}>{children}</code>;
}
🤖 Prompt for AI Agents
In frontend/app/element/markdown.tsx around lines 92 to 97, Code currently
returns a Mermaid component with the chart text derived from children, which
makes the original raw source unavailable to CodeBlock.getTextContent; modify
Code so when it detects language-mermaid it preserves the original source by
attaching the raw text to the output (e.g., pass the computed text through a
prop or render a wrapper element with a data-raw attribute containing that text)
so copy/execute can read it instead of traversing children; ensure the
prop/attribute uses the same exact raw string produced now; optionally also
update getTextContent to fall back to reading data-raw when DOM textContent is
empty.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
frontend/app/element/markdown.tsx (1)

124-129: Fix: copy/execute actions return empty text for Mermaid blocks.

When Code returns a Mermaid component, the original source is no longer a child string; CodeBlock’s traversal yields empty text. Preserve the raw text alongside the rendered chart.

 const Code = ({ className = "", children }: { className?: string; children: React.ReactNode }) => {
     if (/\blanguage-mermaid\b/.test(className)) {
         const text = Array.isArray(children) ? children.join("") : String(children ?? "");
-        return <Mermaid chart={text} />;
+        return (
+            <>
+                <Mermaid chart={text} />
+                <span style={{ display: "none" }} aria-hidden="true" data-raw={text}>
+                    {text}
+                </span>
+            </>
+        );
     }
     return <code className={className}>{children}</code>;
 };

Optional (defensive): teach getTextContent to read data-raw first.

-    const getTextContent = (children: any): string => {
+    const getTextContent = (children: any): string => {
+        if (children?.props?.["data-raw"]) {
+            return String(children.props["data-raw"]);
+        }
         if (typeof children === "string") {
             return children;
         } else if (Array.isArray(children)) {
             return children.map(getTextContent).join("");
         } else if (children.props && children.props.children) {
             return getTextContent(children.props.children);
         }
         return "";
     };

I can push this change if you’d like.

🧹 Nitpick comments (2)
frontend/app/element/markdown.tsx (2)

28-38: Harden Mermaid init: type-safe instance, dedupe concurrent inits, and enforce strict security.

Prevents double-import races when multiple charts mount at once, improves typings, and explicitly sets Mermaid’s securityLevel.

-let mermaidInitialized = false;
-let mermaidInstance: any = null;
+let mermaidInitialized = false;
+let mermaidInstance: typeof import("mermaid").default | null = null;
+let mermaidInitPromise: Promise<typeof import("mermaid").default> | null = null;

 const initializeMermaid = async () => {
-    if (!mermaidInitialized) {
-        const mermaid = await import("mermaid");
-        mermaidInstance = mermaid.default;
-        mermaidInstance.initialize({ startOnLoad: false, theme: "dark" });
-        mermaidInitialized = true;
-    }
+    if (mermaidInstance) return;
+    if (!mermaidInitPromise) {
+        mermaidInitPromise = import("mermaid").then((m) => m.default);
+    }
+    const m = await mermaidInitPromise;
+    m.initialize({
+        startOnLoad: false,
+        theme: "dark",
+        securityLevel: "strict",
+    });
+    mermaidInstance = m;
+    mermaidInitialized = true;
+    mermaidInitPromise = null;
 };

Optional: wire theme to app theme (e.g., dark vs. light) instead of hard-coding "dark".


75-106: Avoid state updates after unmount; narrow catch typing.

Prevents React warnings on rapid mounts/unmounts and surfaces a safe error message.

-    useEffect(() => {
-        const renderMermaid = async () => {
+    useEffect(() => {
+        let cancelled = false;
+        const renderMermaid = async () => {
             try {
                 setIsLoading(true);
                 setError(null);
 
                 await initializeMermaid();
-                if (!ref.current || !mermaidInstance) {
+                if (cancelled || !ref.current || !mermaidInstance) {
                     return;
                 }
 
                 // Normalize the chart text
                 let normalizedChart = chart
                     .replace(/<br\s*\/?>/gi, "\n") // Convert <br/> and <br> to newlines
                     .replace(/\r\n?/g, "\n") // Normalize \r \r\n to \n
                     .replace(/\n+$/, ""); // Remove final newline
 
                 ref.current.removeAttribute("data-processed");
                 ref.current.textContent = normalizedChart;
                 // console.log("mermaid", normalizedChart);
-                await mermaidInstance.run({ nodes: [ref.current] });
-                setIsLoading(false);
+                await mermaidInstance.run({ nodes: [ref.current] });
+                if (!cancelled) setIsLoading(false);
             } catch (err) {
-                console.error("Error rendering mermaid diagram:", err);
-                setError(`Failed to render diagram: ${err.message || err}`);
-                setIsLoading(false);
+                const msg = err instanceof Error ? err.message : String(err);
+                if (!cancelled) {
+                    console.error("Error rendering mermaid diagram:", err);
+                    setError(`Failed to render diagram: ${msg}`);
+                    setIsLoading(false);
+                }
             }
         };
 
         renderMermaid();
-    }, [chart]);
+        return () => {
+            cancelled = true;
+        };
+    }, [chart]);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f65387 and 283eac8.

📒 Files selected for processing (1)
  • frontend/app/element/markdown.tsx (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
frontend/app/element/markdown.tsx (2)

92-96: Good call on clearing data-processed before run.

This avoids Mermaid skipping re-renders on updates.


329-330: LGTM: safe default for Markdown text.

Defaulting to empty string prevents undefined from leaking into transformBlocks.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/app/element/markdown.tsx (1)

412-419: Fix sanitize schema: attributes added to span instead of source (breaks <source srcset|media|type>).

rehype-sanitize will strip these from ; move them to the correct tag.

Apply:

-                        span: [
+                        span: [
                             ...(defaultSchema.attributes?.span || []),
                             // Allow all class names starting with `hljs-`.
                             ["className", /^hljs-./],
-                            ["srcset"],
-                            ["media"],
-                            ["type"],
                         ],
+                        source: [
+                            ...(defaultSchema.attributes?.source || []),
+                            ["srcset"],
+                            ["media"],
+                            ["type"],
+                        ],

Also applies to: 425-425

♻️ Duplicate comments (1)
frontend/app/element/markdown.tsx (1)

125-129: Copy/execute returns empty for Mermaid code blocks rendered as components.

Preserve the raw source in a hidden node so CodeBlock can copy/execute it.

Apply:

-    if (/\blanguage-mermaid\b/.test(className)) {
-        const text = Array.isArray(children) ? children.join("") : String(children ?? "");
-        return <Mermaid chart={text} />;
-    }
+    if (/\blanguage-mermaid\b/.test(className)) {
+        const text = Array.isArray(children) ? children.join("") : String(children ?? "");
+        return (
+            <>
+                <Mermaid chart={text} />
+                <span style={{ display: "none" }} aria-hidden="true" data-raw={text}>
+                    {text}
+                </span>
+            </>
+        );
+    }

Also teach CodeBlock.getTextContent to read data-raw (see next comment).

🧹 Nitpick comments (6)
frontend/app/element/remark-mermaid-to-tag.ts (1)

8-19: Type the plugin and avoid editing while visiting.

Minor: add proper unified/mdast types and return SKIP after replacement to avoid traversing the new node.

Apply:

-import { visit } from "unist-util-visit";
+import { visit, SKIP } from "unist-util-visit";
+import type { Root, Code } from "mdast";
+import type { Plugin } from "unified";

-export default function remarkMermaidToTag() {
-    return (tree: any) => {
-        visit(tree, "code", (node: any, index: number, parent: any) => {
+export default function remarkMermaidToTag(): Plugin<[], Root> {
+    return (tree: Root) => {
+        visit(tree, "code", (node: Code, index: number, parent: any) => {
             if (!parent || typeof index !== "number") return;
             if ((node.lang || "").toLowerCase() !== "mermaid") return;

             parent.children[index] = {
                 type: "html",
                 value: `<mermaidblock>${escapeHTML(node.value || "")}</mermaidblock>`,
             };
+            return SKIP;
         });
     };
 }
frontend/app/element/markdown.tsx (5)

32-39: Harden Mermaid config (explicit securityLevel).

Set securityLevel: "strict" explicitly. Consider theming from app theme later.

Apply:

-        mermaidInstance.initialize({ startOnLoad: false, theme: "dark" });
+        mermaidInstance.initialize({
+            startOnLoad: false,
+            theme: "dark",
+            securityLevel: "strict",
+        });

76-106: Make effect abort-safe and type-safe error handling.

Avoid state updates after unmount and handle unknown errors safely.

Apply:

-    useEffect(() => {
-        const renderMermaid = async () => {
+    useEffect(() => {
+        let cancelled = false;
+        const renderMermaid = async () => {
             try {
                 setIsLoading(true);
                 setError(null);

                 await initializeMermaid();
                 if (!ref.current || !mermaidInstance) {
-                    return;
+                    return;
                 }

                 // Normalize the chart text
                 let normalizedChart = chart
                     .replace(/<br\s*\/?>/gi, "\n") // Convert <br/> and <br> to newlines
                     .replace(/\r\n?/g, "\n") // Normalize \r \r\n to \n
                     .replace(/\n+$/, ""); // Remove final newline

                 ref.current.removeAttribute("data-processed");
                 ref.current.textContent = normalizedChart;
                 // console.log("mermaid", normalizedChart);
                 await mermaidInstance.run({ nodes: [ref.current] });
-                setIsLoading(false);
-            } catch (err) {
-                console.error("Error rendering mermaid diagram:", err);
-                setError(`Failed to render diagram: ${err.message || err}`);
-                setIsLoading(false);
+                if (!cancelled) setIsLoading(false);
+            } catch (err: unknown) {
+                console.error("Error rendering mermaid diagram:", err);
+                const msg = err instanceof Error ? err.message : String(err);
+                if (!cancelled) {
+                    setError(`Failed to render diagram: ${msg}`);
+                    setIsLoading(false);
+                }
             }
         };
 
         renderMermaid();
-    }, [chart]);
+        return () => {
+            cancelled = true;
+        };
+    }, [chart]);

Also applies to: 108-121


139-149: Teach getTextContent to prefer data-raw when present.

Enables copy/execute for Mermaid nodes rendered by Code.

Apply:

-    const getTextContent = (children: any): string => {
+    const getTextContent = (children: any): string => {
         if (typeof children === "string") {
             return children;
         } else if (Array.isArray(children)) {
             return children.map(getTextContent).join("");
-        } else if (children.props && children.props.children) {
-            return getTextContent(children.props.children);
+        } else if (children && children.props) {
+            if (children.props["data-raw"] != null) return String(children.props["data-raw"]);
+            if (children.props.children) return getTextContent(children.props.children);
         }
         return "";
     };

370-383: De-duplicate local getTextContent helper.

Prefer a single shared helper (export or lift) to avoid drift between CodeBlock and mermaidblock usage.


32-39: Optional: Theme sync.

Consider deriving Mermaid theme from the app theme or prefers-color-scheme to avoid hardcoding "dark".

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 283eac8 and a6bf816.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (3)
  • frontend/app/element/markdown.tsx (6 hunks)
  • frontend/app/element/remark-mermaid-to-tag.ts (1 hunks)
  • package.json (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • package.json
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/app/element/markdown.tsx (1)
frontend/app/element/remark-mermaid-to-tag.ts (1)
  • remarkMermaidToTag (8-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
frontend/app/element/remark-mermaid-to-tag.ts (1)

14-17: LGTM: safe HTML wrapper with escaping.

Replacing mermaid code fences with an escaped <mermaidblock> HTML node is correct and works with rehype-raw + sanitize.

@sawka sawka merged commit dc3b2c2 into main Sep 15, 2025
4 of 7 checks passed
@sawka sawka deleted the sawka/mermaid branch September 15, 2025 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant