Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
85 changes: 85 additions & 0 deletions frontend/src/components/MermaidBlock.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
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);
}
document.getElementById(`d${id}`)?.remove();

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.

🔵 bugs: The DOM cleanup document.getElementById(d${id})?.remove() runs even when cancelled is true (it's outside the if (!cancelled) block). If the component unmounts during a failed render, this removes a DOM element that may have already been cleaned up or belong to a new render cycle. Move it inside the if (!cancelled) guard for consistency, or document why it must always run. [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.

🟡 bugs: The cleanup document.getElementById(d${id})?.remove() is only called inside the catch block, but mermaid.render() also creates a temporary DOM element on success. If the component unmounts while mermaid.render() is in-flight (cancelled = true), the temporary element created by mermaid may leak in the DOM. Consider also cleaning up the element in the effect's cleanup function. [fixable]

}
}

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
51 changes: 51 additions & 0 deletions frontend/src/components/__tests__/MermaidBlock.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { createElement } from 'react';
import { renderToStaticMarkup } from 'react-dom/server';

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

// Mock useId since renderToStaticMarkup doesn't fully support it
vi.mock('react', async () => {
const actual = await vi.importActual<typeof import('react')>('react');
return { ...actual, useId: () => ':test-id:' };
});

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

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

describe('MermaidBlock', () => {
it('renders null initially while waiting for mermaid.render', () => {
// mermaid.render returns a never-resolving promise (simulates pending)
vi.mocked(mermaid.render).mockReturnValue(new Promise(() => {}));
const html = renderToStaticMarkup(createElement(MermaidBlock, { code: 'graph TD; A-->B;' }));
// Should render nothing while svg is null and no error
expect(html).toBe('');
});

it('renders fallback code block on render error', async () => {

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.

🟡 missing_tests: The test titled 'renders fallback code block on render error' is misleading — it only verifies the initial render (empty string) before the async error resolves, not the actual error fallback UI. renderToStaticMarkup doesn't run useEffect, so none of the three tests exercise the successful SVG render path or the error fallback code-block path. The core behavior of this component (rendering SVG output and falling back on errors) is entirely untested. Consider using @testing-library/react with act() to test async state updates. [fixable]

vi.mocked(mermaid.render).mockRejectedValue(new Error('parse error'));

// We can't easily test async state updates with renderToStaticMarkup,
// but we can verify the component doesn't crash
const html = renderToStaticMarkup(createElement(MermaidBlock, { code: 'invalid{{{' }));
expect(html).toBe(''); // Initial render before error resolves
});

it('does not call mermaid.initialize at import time (deferred to first render)', () => {
// Verify the deferred initialization pattern — initialize is called inside
// useEffect, not at module scope. renderToStaticMarkup skips effects.
vi.mocked(mermaid.render).mockReturnValue(new Promise(() => {}));
renderToStaticMarkup(createElement(MermaidBlock, { code: 'graph TD; A-->B;' }));
expect(mermaid.initialize).not.toHaveBeenCalled();
});
});
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');
});
});
10 changes: 9 additions & 1 deletion frontend/src/lib/markdown-config.tsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import React from 'react';
import remarkGfm from 'remark-gfm';
import rehypeRaw from 'rehype-raw';
import rehypeSanitize, { defaultSchema } from 'rehype-sanitize';
import type { Components } from 'react-markdown';
import type { PluggableList } from 'unified';
import { MermaidBlock } from '../components/MermaidBlock';
import { getMermaidCode } from './mermaid-detect';

const sanitizeSchema = {
...defaultSchema,
attributes: {
...defaultSchema.attributes,
img: [...(defaultSchema.attributes?.img ?? []), 'width', 'height'],
// Only allow language-* classes (set by rehype-highlight) — not arbitrary classNames

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: Comment says 'Only allow language-* classes (set by rehype-highlight)' but rehype-highlight is not used in this rendering path (MarkdownPreviewCard/FileViewer use rehypeRaw + rehypeSanitize, not rehypeHighlight). The classes come from the markdown parser's fenced code block syntax. Misleading comment could confuse future readers. [fixable]

code: [...(defaultSchema.attributes?.code ?? []), ['className', /^language-/]],
},
};

Expand All @@ -22,4 +25,9 @@ export const markdownComponents: Components = {
<table {...props}>{children}</table>
</div>
),
pre: ({ children, ...props }) => {

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.

🔵 missing_tests: The new pre component in markdownComponents (used by FileViewer and MarkdownPreviewCard) has mermaid detection logic duplicated from MessageBubble, but no test coverage. If this logic diverges or breaks, there's no safety net. Consider adding a test for markdownComponents.pre similar to the MessageBubble tests. [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 pre handler in markdown-config.tsx (used by FileViewer and MarkdownPreviewCard) renders a bare <pre> for non-mermaid code blocks, while the pre handler in MessageBubble.tsx wraps them in a code-block-wrapper div with a CopyButton. This means code blocks rendered via markdown-config (file viewer, preview cards) lost their copy button and wrapper styling in this PR, since the pre handler was changed from a passthrough to one that actively returns <pre> without the wrapper. Verify this is intentional — if those contexts previously had copy buttons, this is a regression. [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 pre handler in markdownComponents (used by FileViewer and MarkdownPreviewCard) does not wrap non-mermaid code blocks in code-block-wrapper with a CopyButton, unlike the identical handler in TextBubble. This creates an inconsistency: code blocks in chat have copy buttons, but code blocks in the file viewer and preview cards do not. [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 pre handler here renders a plain <pre> for non-mermaid code blocks, while the pre handler in MessageBubble.tsx wraps them with a CopyButton. This is intentional (markdown-config serves MarkdownPreviewCard and FileViewer where CopyButton isn't needed), but the behavioral difference between two nearly identical pre handlers in the same codebase could surprise a future contributor.

const mermaidCode = getMermaidCode(children);

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 pre handler in markdown-config.tsx (used by MarkdownPreviewCard/FileViewer) does not include a CopyButton in its non-mermaid fallback, while MessageBubble's pre handler does. This means code blocks in file previews lose their copy button. If this was intentional, consider adding a brief comment; if not, add CopyButton here too for consistency. [fixable]

if (mermaidCode !== null) return <MermaidBlock code={mermaidCode} />;

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 pre handler in markdownComponents (used by MarkdownPreviewCard and FileViewer) does not include a CopyButton for non-mermaid code blocks, unlike the equivalent handler in MessageBubble.tsx which wraps code blocks with code-block-wrapper + CopyButton. This means code blocks inside MarkdownPreviewCard and FileViewer won't have copy buttons. This appears intentional (the old version had no pre handler at all), but worth confirming since the two pre handlers now diverge in behavior. [fixable]

return <pre {...props}>{children}</pre>;
},
};
Loading
Loading