-
Notifications
You must be signed in to change notification settings - Fork 0
feat(frontend): mermaid diagrams + preview card auto-close fix #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
08f47f8
01e46b8
f4aebb4
eb12ac1
b754ab4
4596532
4c633a6
946f7ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| import { useEffect, useId, useState } from 'react'; | ||
| import { CopyButton } from './CopyButton'; | ||
|
|
||
| let mermaidInitialized = false; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: The module-level
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: Module-level
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: Module-level
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: The module-level
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: The module-level |
||
|
|
||
| /** Exported for test cleanup only. */ | ||
| export function _resetMermaidInit() { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: |
||
| mermaidInitialized = false; | ||
| } | ||
|
|
||
| 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(() => { | ||
| let cancelled = false; | ||
| const id = `mermaid-${instanceId.replace(/:/g, '')}`; | ||
|
|
||
| async function render() { | ||
| try { | ||
| // Dynamic import keeps mermaid (~1MB+ with d3/katex/cytoscape) out of | ||
| // the main bundle — only loaded when a mermaid diagram is encountered. | ||
| const { default: mermaid } = await import('mermaid'); | ||
| if (!mermaidInitialized) { | ||
| mermaidInitialized = true; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 bugs: |
||
| mermaid.initialize({ | ||
| 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', | ||
| }, | ||
| }); | ||
| } | ||
| const { svg: rendered } = await mermaid.render(id, code); | ||
| if (!cancelled) { | ||
| setSvg(rendered); | ||
| setError(null); | ||
| } | ||
| } catch { | ||
| if (!cancelled) { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: The DOM cleanup |
||
| setError('Invalid diagram'); | ||
| setSvg(null); | ||
| 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; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: When |
||
|
|
||
| return ( | ||
| <div className="mermaid-block"> | ||
| <div className="mermaid-block-svg" dangerouslySetInnerHTML={{ __html: svg }} /> | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 unsafe_assumptions:
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 unsafe_assumptions: Using |
||
| <CopyButton text={code} className="code-block-copy" label="Copy source" /> | ||
| </div> | ||
| ); | ||
| } | ||
| 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'; | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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'>) => { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The mermaid detection logic (extracting the first child, checking className against
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The mermaid-aware ). Consider extracting the shared mermaid detection + fallback pattern, or having MessageBubble compose on top of markdown-config's pre.
|
||
| 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], | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 unsafe_assumptions: useMemo deps include
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 regressions: The useMemo deps include |
||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
| className={`msg-bubble msg-bubble--assistant${streaming ? ' msg-bubble--streaming' : ''}${showCollapsed ? ' msg-bubble--collapsed' : ''}`} | ||
|
|
@@ -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> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| // @vitest-environment jsdom | ||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||
| import { createElement } from 'react'; | ||
| import { render, act, cleanup } from '@testing-library/react'; | ||
|
|
||
| const mockInitialize = vi.fn(); | ||
| const mockRender = vi.fn(); | ||
|
|
||
| // Mock the dynamic import('mermaid') that MermaidBlock uses | ||
| vi.mock('mermaid', () => ({ | ||
| default: { | ||
| initialize: mockInitialize, | ||
| render: mockRender, | ||
| }, | ||
| })); | ||
|
|
||
| import { MermaidBlock, _resetMermaidInit } from '../MermaidBlock'; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| _resetMermaidInit(); | ||
| cleanup(); | ||
| }); | ||
|
|
||
| describe('MermaidBlock', () => { | ||
| it('renders SVG and initializes with securityLevel strict', async () => { | ||
| mockRender.mockResolvedValue({ | ||
| svg: '<svg>diagram</svg>', | ||
| diagramType: 'flowchart', | ||
| bindFunctions: undefined, | ||
| }); | ||
| await act(async () => { | ||
| render(createElement(MermaidBlock, { code: 'graph TD; A-->B;' })); | ||
| }); | ||
| const block = document.querySelector('.mermaid-block-svg'); | ||
| expect(block).not.toBeNull(); | ||
| expect(block!.innerHTML).toContain('diagram'); | ||
| expect(mockInitialize).toHaveBeenCalledWith( | ||
| expect.objectContaining({ securityLevel: 'strict' }), | ||
| ); | ||
| }); | ||
|
|
||
| it('renders fallback code block on render error', async () => { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| mockRender.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{{{'); | ||
| }); | ||
|
|
||
| it('only initializes mermaid once across multiple renders', async () => { | ||
| mockRender.mockResolvedValue({ | ||
| svg: '<svg>a</svg>', | ||
| diagramType: 'flowchart', | ||
| bindFunctions: undefined, | ||
| }); | ||
| await act(async () => { | ||
| render(createElement(MermaidBlock, { code: 'graph TD; A-->B;' })); | ||
| }); | ||
| cleanup(); | ||
| await act(async () => { | ||
| render(createElement(MermaidBlock, { code: 'graph LR; X-->Y;' })); | ||
| }); | ||
| expect(mockInitialize).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| 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 }); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: Three |
||
| 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'); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
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
mermaidInitializedflag 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.