Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@xterm/addon-web-links": "^0.11.0",
"@xterm/xterm": "^5.5.0",
"highlight.js": "^11.11.1",
"mermaid": "^11.16.0",
"react": "^19.0.0",
"react-dom": "^19.0.0",
"react-markdown": "^10.1.0",
Expand Down
86 changes: 86 additions & 0 deletions frontend/src/components/MermaidBlock.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { useEffect, useId, useState } from 'react';
import mermaid from 'mermaid';

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 style: Static import mermaid from 'mermaid' pulls mermaid (plus d3, cytoscape, katex, roughjs, dompurify, etc.) into the main bundle. Since MermaidBlock is imported by MessageBubble.tsx (a core chat component), all these dependencies load on initial page load. For a mobile-first app this is a significant hit. Consider dynamically importing mermaid inside the useEffect (const { default: mermaid } = await import('mermaid')) so Vite can code-split it into a separate chunk loaded only when a mermaid diagram is actually encountered. [fixable]

import { CopyButton } from './CopyButton';

let mermaidInitialized = false;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: The module-level mermaidInitialized flag works in production but can leak state between test runs in Vitest (which reuses the module cache by default). If MermaidBlock tests are added later, they may see stale initialization. Not a production issue, but worth noting for testability.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: The module-level mermaidInitialized flag means mermaid.initialize() is only ever called once with the hardcoded dark theme. If theme support is added later (light/dark toggle), re-initialization would be silently skipped. This is fine for now but worth noting as a design constraint.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 style: Module-level let mermaidInitialized is mutable shared state. If tests reset modules or if HMR reloads the component without reloading the module, the flag could get out of sync. Consider using mermaid.initialize idempotency or checking mermaid internal state instead.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: Module-level mermaidInitialized flag won't reset during Vite HMR — if mermaid config (theme, security) needs updating during development, a full page reload is required. Minor DX issue; not a production concern.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: The module-level mermaidInitialized flag never resets. In tests this creates ordering dependence — test 3 (renders SVG...) sets it to true, so any later test expecting mermaid.initialize to be called again will fail silently. Consider exporting a resetMermaidInit() for test use, or using vi.resetModules() in the test's beforeEach. [fixable]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: The module-level mermaidInitialized flag is never reset between tests. The MermaidBlock tests work because renderToStaticMarkup (tests 1-2) doesn't trigger useEffect, so test 3 is the first to call ensureMermaidInit(). If test ordering changes or new tests are added that use render() before test 3, the mermaid.initialize assertion in test 3 would fail. Consider exporting a resetMermaidInit for test use, or moving the flag into a ref/context. [fixable]


function ensureMermaidInit() {
if (mermaidInitialized) return;
mermaidInitialized = true;
mermaid.initialize({

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 unsafe_assumptions: Mermaid's securityLevel is not explicitly set. While mermaid v11 defaults to 'strict' (which uses DOMPurify), relying on a library default for security is fragile — a future upgrade could change the default. Explicitly set securityLevel: 'strict' in the mermaid.initialize() call to make the security posture intentional and auditable. [fixable]

startOnLoad: false,
securityLevel: 'strict',
theme: 'dark',
themeVariables: {
darkMode: true,
background: '#1e1e2e',
primaryColor: '#7c3aed',
primaryTextColor: '#e2e8f0',
primaryBorderColor: '#6366f1',
lineColor: '#94a3b8',
secondaryColor: '#374151',
tertiaryColor: '#1f2937',
noteBkgColor: '#374151',
noteTextColor: '#e2e8f0',
fontFamily: 'inherit',
},
});
}

interface MermaidBlockProps {
code: string;
}

export function MermaidBlock({ code }: MermaidBlockProps) {
const instanceId = useId();
const [error, setError] = useState<string | null>(null);
const [svg, setSvg] = useState<string | null>(null);

useEffect(() => {
ensureMermaidInit();
let cancelled = false;
const id = `mermaid-${instanceId.replace(/:/g, '')}`;

async function render() {
try {
const { svg: rendered } = await mermaid.render(id, code);
if (!cancelled) {
setSvg(rendered);
setError(null);
}
} catch {
if (!cancelled) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: The DOM cleanup document.getElementById(d${id})?.remove() assumes mermaid's internal temp element naming convention (d prefix). This is an undocumented implementation detail of mermaid that could change across versions. Consider wrapping the mermaid render call in a try/finally that queries by the known container pattern, or document the dependency on mermaid internals with a version-pinned comment.

setError('Invalid diagram');
setSvg(null);
// Clean up any leftover element mermaid may have created on failure
document.getElementById(`d${id}`)?.remove();
}
}
}

render();
return () => {
cancelled = true;
};
}, [code, instanceId]);

if (error) {
return (
<div className="code-block-wrapper">
<pre>
<code>{code}</code>
</pre>
<CopyButton text={code} className="code-block-copy" label="Copy code" />
</div>
);
}

if (!svg) return null;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 style: When svg is null and there's no error (initial render / loading state), the component returns null — a blank gap in the message. Consider rendering a lightweight loading indicator (e.g., a skeleton or the raw code block) so the user sees something while mermaid loads (~1MB async import + render). [fixable]


return (
<div className="mermaid-block">
<div className="mermaid-block-svg" dangerouslySetInnerHTML={{ __html: svg }} />

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 unsafe_assumptions: dangerouslySetInnerHTML={{ __html: svg }} relies entirely on mermaid's securityLevel: 'strict' (which uses DOMPurify internally) for XSS safety. This is the standard mermaid integration pattern and is safe as long as the mermaid library itself isn't compromised. However, if the mermaid dependency were supply-chain attacked, SVG would be injected unsanitized. Consider an explicit DOMPurify pass on the SVG output as defense-in-depth, or at minimum a comment documenting the safety invariant. [fixable]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 unsafe_assumptions: Using dangerouslySetInnerHTML={{ __html: svg }} with mermaid's rendered SVG. While mermaid v11 with securityLevel: 'strict' uses DOMPurify internally to sanitize output (DOMPurify is a transitive dependency visible in the lockfile), this is an implicit safety guarantee tied to mermaid's internals. If mermaid ever changes its sanitization behavior, this becomes an XSS vector. Consider adding a brief comment noting the DOMPurify reliance, or adding an explicit DOMPurify.sanitize() call on the SVG output for defense in depth. [fixable]

<CopyButton text={code} className="code-block-copy" label="Copy source" />
</div>
);
}
134 changes: 70 additions & 64 deletions frontend/src/components/MessageBubble.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useState, useEffect, useRef } from 'react';
import React, { useState, useEffect, useRef, useMemo } from 'react';
import ReactMarkdown, { defaultUrlTransform } from 'react-markdown';
import remarkGfm from 'remark-gfm';
import rehypeHighlight from 'rehype-highlight';
Expand All @@ -10,7 +10,9 @@ import { CopyButton } from './CopyButton';
import { ShareButton } from './ShareButton';
import { ReadAloudButton } from './ReadAloudButton';
import { extractText } from '../lib/extractText';
import { getMermaidCode } from '../lib/mermaid-detect';
import { MarkdownPreviewCard } from './MarkdownPreviewCard';
import { MermaidBlock } from './MermaidBlock';

const COLLAPSE_HEIGHT = 300;

Expand Down Expand Up @@ -101,6 +103,72 @@ export function TextBubble({ content, streaming = false, timestamp, readAloud }:

const showCollapsed = isLong && collapsed && !streaming;

// Memoize components so react-markdown preserves component instances
// (e.g. MarkdownPreviewCard expanded state) across parent re-renders.
const mdComponents = useMemo(
() => ({
table: ({ children, ...props }: React.ComponentProps<'table'>) => (
<div className="table-scroll-wrapper">
<table {...props}>{children}</table>
</div>
),
pre: ({ children, ...props }: React.ComponentProps<'pre'>) => {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 style: The mermaid detection logic (extracting the first child, checking className against /language-mermaid/) is duplicated verbatim between MessageBubble.tsx:114-123 and markdown-config.tsx:29-37. Extract a shared helper (e.g., isMermaidCodeBlock(children): string | null) to keep the two call sites in sync. [fixable]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 style: The mermaid-aware pre component is duplicated between MessageBubble.tsx (line 115-124) and markdown-config.tsx (line 28-32). Both call getMermaidCode and return MermaidBlock identically; only the non-mermaid fallback differs (CopyButton wrapper vs. plain

). Consider extracting the shared mermaid detection + fallback pattern, or having MessageBubble compose on top of markdown-config's pre. [fixable]

const mermaidCode = getMermaidCode(children);
if (mermaidCode !== null) return <MermaidBlock code={mermaidCode} />;
const text = extractText(children);
return (
<div className="code-block-wrapper">
<pre {...props}>{children}</pre>
<CopyButton text={text} className="code-block-copy" label="Copy code" />
</div>
);
},
p: ({ children }: React.ComponentProps<'p'>) => {
const childArray = React.Children.toArray(children);
if (childArray.length === 1 && React.isValidElement(childArray[0])) {
const el = childArray[0] as React.ReactElement<Record<string, unknown>>;
const href = el.props?.href as string | undefined;
if (href?.startsWith(FILE_SCHEME)) {
const filePath = decodeURIComponent(href.slice(FILE_SCHEME.length));
if (/\.mdx?$/i.test(filePath)) {
return <MarkdownPreviewCard filePath={filePath} />;
}
}
}
return <p>{children}</p>;
},
a: ({ href, children }: React.ComponentProps<'a'>) => {
if (href?.startsWith(FILE_SCHEME)) {
const filePath = decodeURIComponent(href.slice(FILE_SCHEME.length));
return (
<span className="file-path-group">
<a
href="#"
className="file-path-link"
data-file-path={filePath}
onClick={(e) => {
e.preventDefault();
navigate(
`/files?path=${encodeURIComponent(filePath)}&from=${encodeURIComponent(currentPath)}`,
);
}}
>
{children}
</a>
<ShareButton filePath={filePath} className="file-path-share" />
</span>
);
}
return (
<a href={href} target="_blank" rel="noopener noreferrer">
{children}
</a>
);
},
}),
[navigate, currentPath],

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🟡 unsafe_assumptions: useMemo deps include navigate from useNavigate(). In React Router v7 declarative mode (BrowserRouter), navigate's reference stability is not guaranteed across renders — it depends on internal useCallback deps including context values. If navigate changes identity on re-renders that don't involve navigation (e.g. during streaming updates), the memoization is defeated and MarkdownPreviewCard expanded state will still reset — the exact bug this PR aims to fix. Consider using a ref for navigate (const navRef = useRef(navigate); navRef.current = navigate;) and referencing navRef.current inside the memo, removing navigate from the dep array. [fixable]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 regressions: The useMemo deps include currentPath, so when the URL changes (e.g. navigating to FileViewer and back), the entire components object is recreated, which would reset MarkdownPreviewCard expanded state. This is the same behavior as before the fix (pre-useMemo), so it's not a regression per se, but it limits the fix: the auto-close bug is only prevented during same-page re-renders (streaming updates), not across navigation. Worth noting in case this was intended to be fully fixed.

);

return (
<div
className={`msg-bubble msg-bubble--assistant${streaming ? ' msg-bubble--streaming' : ''}${showCollapsed ? ' msg-bubble--collapsed' : ''}`}
Expand All @@ -110,69 +178,7 @@ export function TextBubble({ content, streaming = false, timestamp, readAloud }:
remarkPlugins={[remarkGfm]}
rehypePlugins={[rehypeHighlight]}
urlTransform={(url) => (url.startsWith(FILE_SCHEME) ? url : defaultUrlTransform(url))}
components={{
table: ({ children, ...props }) => (
<div className="table-scroll-wrapper">
<table {...props}>{children}</table>
</div>
),
pre: ({ children, ...props }) => {
const text = extractText(children);
return (
<div className="code-block-wrapper">
<pre {...props}>{children}</pre>
<CopyButton text={text} className="code-block-copy" label="Copy code" />
</div>
);
},
// When a paragraph contains a single file-path link to a .md/.mdx
// file, promote it to an inline preview card instead of a plain link.
// In ReactMarkdown v10, children are unrendered component instances —
// the `a` handler hasn't run yet — so we check `href` (the prop
// ReactMarkdown passes) rather than rendered DOM attributes.
p: ({ children }) => {
const childArray = React.Children.toArray(children);
if (childArray.length === 1 && React.isValidElement(childArray[0])) {
const el = childArray[0] as React.ReactElement<Record<string, unknown>>;
const href = el.props?.href as string | undefined;
if (href?.startsWith(FILE_SCHEME)) {
const filePath = decodeURIComponent(href.slice(FILE_SCHEME.length));
if (/\.mdx?$/i.test(filePath)) {
return <MarkdownPreviewCard filePath={filePath} />;
}
}
}
return <p>{children}</p>;
},
a: ({ href, children }) => {
if (href?.startsWith(FILE_SCHEME)) {
const filePath = decodeURIComponent(href.slice(FILE_SCHEME.length));
return (
<span className="file-path-group">
<a
href="#"
className="file-path-link"
data-file-path={filePath}
onClick={(e) => {
e.preventDefault();
navigate(
`/files?path=${encodeURIComponent(filePath)}&from=${encodeURIComponent(currentPath)}`,
);
}}
>
{children}
</a>
<ShareButton filePath={filePath} className="file-path-share" />
</span>
);
}
return (
<a href={href} target="_blank" rel="noopener noreferrer">
{children}
</a>
);
},
}}
components={mdComponents}
>
{processed}
</ReactMarkdown>
Expand Down
62 changes: 62 additions & 0 deletions frontend/src/components/__tests__/MermaidBlock.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// @vitest-environment jsdom
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { createElement } from 'react';
import { renderToStaticMarkup } from 'react-dom/server';
import { render, act, cleanup } from '@testing-library/react';

// Mock mermaid before importing the component
vi.mock('mermaid', () => ({
default: {
initialize: vi.fn(),
render: vi.fn(),
},
}));

import mermaid from 'mermaid';
import { MermaidBlock } from '../MermaidBlock';

beforeEach(() => {
vi.clearAllMocks();
cleanup();
});

describe('MermaidBlock', () => {
it('renders null initially while waiting for mermaid.render', () => {
vi.mocked(mermaid.render).mockReturnValue(new Promise(() => {}));
const html = renderToStaticMarkup(createElement(MermaidBlock, { code: 'graph TD; A-->B;' }));
expect(html).toBe('');
});

it('does not call mermaid.initialize at import time (deferred to useEffect)', () => {
vi.mocked(mermaid.render).mockReturnValue(new Promise(() => {}));
renderToStaticMarkup(createElement(MermaidBlock, { code: 'graph TD; A-->B;' }));
expect(mermaid.initialize).not.toHaveBeenCalled();
});

it('renders SVG and initializes with securityLevel strict', async () => {
vi.mocked(mermaid.render).mockResolvedValue({
svg: '<svg>diagram</svg>',
});
await act(async () => {
render(createElement(MermaidBlock, { code: 'graph TD; A-->B;' }));
});
// Verify rendered output
const block = document.querySelector('.mermaid-block-svg');
expect(block).not.toBeNull();
expect(block!.innerHTML).toContain('diagram');
// Verify security config on first initialization
expect(mermaid.initialize).toHaveBeenCalledWith(
expect.objectContaining({ securityLevel: 'strict' }),
);
});

it('renders fallback code block on render error', async () => {
vi.mocked(mermaid.render).mockRejectedValue(new Error('parse error'));
await act(async () => {
render(createElement(MermaidBlock, { code: 'invalid{{{' }));
});
const wrapper = document.querySelector('.code-block-wrapper');
expect(wrapper).not.toBeNull();
expect(wrapper!.textContent).toContain('invalid{{{');
});
});
20 changes: 20 additions & 0 deletions frontend/src/components/__tests__/MessageBubble.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,26 @@ describe('TextBubble code block CopyButton', () => {
});
});

describe('TextBubble mermaid code blocks', () => {
it('renders MermaidBlock for language-mermaid code blocks', () => {
renderToStaticMarkup(createElement(TextBubble, { content: 'test' }));
const pre = capturedComponents!.pre;
const codeEl = createElement('code', { className: 'language-mermaid' }, 'graph TD; A-->B;');
const result = pre({ children: codeEl });
// Should render MermaidBlock, not code-block-wrapper
expect(result.type).not.toBe('div');
expect(result.props.code).toBe('graph TD; A-->B;');
});

it('renders normal code block for non-mermaid languages', () => {
renderToStaticMarkup(createElement(TextBubble, { content: 'test' }));
const pre = capturedComponents!.pre;
const codeEl = createElement('code', { className: 'language-python' }, 'print("hi")');
const html = renderToStaticMarkup(pre({ children: codeEl }));
expect(html).toContain('code-block-wrapper');
});
});

describe('TextBubble markdown preview card promotion', () => {
it('provides a custom p component to ReactMarkdown', () => {
renderToStaticMarkup(createElement(TextBubble, { content: 'test' }));
Expand Down
29 changes: 29 additions & 0 deletions frontend/src/lib/__tests__/markdown-config.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { describe, it, expect } from 'vitest';
import { createElement } from 'react';
import { markdownComponents } from '../markdown-config';

describe('markdownComponents.pre', () => {
const pre = markdownComponents.pre!;

it('renders MermaidBlock for language-mermaid code blocks', () => {
const codeEl = createElement('code', { className: 'language-mermaid' }, 'graph TD; A-->B;');
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result = (pre as any)({ children: codeEl });

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

🔵 style: Three as any casts are used to call pre as a function. Since markdownComponents is typed as Components (from react-markdown), the pre property is typed as a component rather than a plain function. Consider extracting the pre handler into a named function with explicit types in markdown-config.tsx, or use a type assertion once at the pre binding rather than repeating as any in each test. [fixable]

expect(result.type).not.toBe('pre');
expect(result.props.code).toBe('graph TD; A-->B;');
});

it('renders normal pre for non-mermaid code blocks', () => {
const codeEl = createElement('code', { className: 'language-python' }, 'print("hi")');
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result = (pre as any)({ children: codeEl });
expect(result.type).toBe('pre');
});

it('renders normal pre when code has no className', () => {
const codeEl = createElement('code', null, 'plain text');
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const result = (pre as any)({ children: codeEl });
expect(result.type).toBe('pre');
});
});
Loading
Loading