Render the assistant's replies as Markdown #118

Merged
steve merged 2 commits from feat/assistant-markdown into main 2026-07-22 16:42:12 +00:00
6 changed files with 1626 additions and 23 deletions
Showing only changes of commit ae3d52db31 - Show all commits
+1469 -5
View File
File diff suppressed because it is too large Load Diff
+2
View File
@@ -22,6 +22,8 @@
"clsx": "^2.1.1",
"react": "^19.0.0",
"react-dom": "^19.0.0",
"react-markdown": "^9.1.0",
"remark-gfm": "^4.0.1",
"tailwind-merge": "^2.6.0",
"zod": "^3.24.1",
"zustand": "^5.0.14"
+23 -4
View File
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState, type ReactNode } from 'react'
import { Suspense, lazy, useEffect, useRef, useState, type ReactNode } from 'react'
import { Alert } from '@/components/ui/Alert'
import { errorMessage } from '@/lib/api'
import { Button } from '@/components/ui/Button'
@@ -16,6 +16,12 @@ import {
import { useUndo } from '@/lib/history'
import { UndoButton } from './UndoButton'
// Lazy so the markdown renderer + its ecosystem (~150 KB) loads only when an
// assistant message actually renders, not for everyone who opens the editor.
const MarkdownMessage = lazy(() =>
Review

🔴 Lazy MarkdownMessage chunk failure crashes ChatPanel with no ErrorBoundary or fallback

error-handling · flagged by 3 models

  • No error handling for lazy-loaded MarkdownMessage chunk failuresweb/src/editor/ChatPanel.tsx:21 If import('./MarkdownMessage') fails (stale chunk hash after a deployment, network dropout, CDN 404), the lazy() Promise rejects. Suspense only catches the loading throw; a rejection propagates as a render error. With no ErrorBoundary anywhere in the component tree, React will unmount the ChatPanel (or the whole AppShell) and the user gets a broken chat with no recovery pa…

🪰 Gadfly · advisory

🔴 **Lazy MarkdownMessage chunk failure crashes ChatPanel with no ErrorBoundary or fallback** _error-handling · flagged by 3 models_ - **No error handling for lazy-loaded `MarkdownMessage` chunk failures** — `web/src/editor/ChatPanel.tsx:21` If `import('./MarkdownMessage')` fails (stale chunk hash after a deployment, network dropout, CDN 404), the `lazy()` Promise rejects. `Suspense` only catches the *loading* throw; a *rejection* propagates as a render error. With no `ErrorBoundary` anywhere in the component tree, React will unmount the `ChatPanel` (or the whole `AppShell`) and the user gets a broken chat with no recovery pa… <sub>🪰 Gadfly · advisory</sub>
import('./MarkdownMessage').then((m) => ({ default: m.MarkdownMessage })),
)
/**
* Talk to the garden assistant, in the editor beside the canvas.
*
@@ -227,11 +233,24 @@ function Bubble({
<div className={cn('flex flex-col', mine ? 'items-end' : 'items-start')}>
<div
className={cn(
'max-w-[90%] whitespace-pre-wrap rounded-lg px-2.5 py-2 text-sm',
mine ? 'bg-accent/15 text-fg' : 'border border-border text-fg',
'rounded-lg px-2.5 py-2 text-sm',
// The user's own text is literal (their `*` shouldn't become a bullet)
// and hugs the right; the assistant's Markdown is rendered and gets the
// full width so a table has room.
mine
? 'max-w-[90%] whitespace-pre-wrap bg-accent/15 text-fg'
: 'w-full border border-border text-fg',
)}
>
{body}
{mine ? (
body
) : (
// Show the raw text until the renderer chunk arrives, so a message never
Outdated
Review

🔴 Lazy MarkdownMessage has no Error Boundary; chunk/render failures crash the app

error-handling · flagged by 2 models

  • web/src/editor/ChatPanel.tsx:248-252 — The lazy-loaded MarkdownMessage is wrapped in Suspense but has no Error Boundary. If the chunk fails to load (network drop, stale deploy) or react-markdown throws during render (library bug, malformed markdown edge case), the rejected promise / thrown error bubbles past the Suspense boundary and crashes the route or the entire app. The route-level RouteError only catches loader/beforeLoad errors, not component-render errors. Fix: A…

🪰 Gadfly · advisory

🔴 **Lazy MarkdownMessage has no Error Boundary; chunk/render failures crash the app** _error-handling · flagged by 2 models_ - **`web/src/editor/ChatPanel.tsx:248-252`** — The lazy-loaded `MarkdownMessage` is wrapped in `Suspense` but has **no Error Boundary**. If the chunk fails to load (network drop, stale deploy) or `react-markdown` throws during render (library bug, malformed markdown edge case), the rejected promise / thrown error bubbles past the `Suspense` boundary and crashes the route or the entire app. The route-level `RouteError` only catches loader/beforeLoad errors, not component-render errors. **Fix:** A… <sub>🪰 Gadfly · advisory</sub>
// flashes blank.
<Suspense fallback={<span className="whitespace-pre-wrap">{body}</span>}>
<MarkdownMessage>{body}</MarkdownMessage>
</Suspense>
)}
</div>
{children}
</div>
+42
View File
@@ -0,0 +1,42 @@
import { describe, expect, it } from 'vitest'
import { renderToStaticMarkup } from 'react-dom/server'
import { MarkdownMessage } from './MarkdownMessage'
// renderToStaticMarkup needs no DOM, so this runs in the default (node) env and
// proves the assistant's Markdown — GFM tables in particular — actually renders.
function render(md: string): string {
return renderToStaticMarkup(<MarkdownMessage>{md}</MarkdownMessage>)
}
describe('MarkdownMessage', () => {
it('renders a GFM table with styled cells', () => {
const html = render('| Bed | Plant |\n| --- | --- |\n| North | Garlic |')
expect(html).toContain('<table')
expect(html).toContain('border-collapse')
expect(html).toContain('<th')
expect(html).toContain('<td')
expect(html).toContain('Garlic')
// Wide tables scroll inside their own box rather than blowing out the bubble.
expect(html).toContain('overflow-x-auto')
})
it('renders inline formatting and lists', () => {
const html = render('**bold** and *italic*\n\n- one\n- two')
expect(html).toContain('<strong')
expect(html).toContain('<em')
expect(html).toContain('<ul')
expect(html).toContain('<li')
})
it('does not emit raw HTML from the model (no rehype-raw)', () => {
const html = render('Hi <script>alert(1)</script> <b>x</b>')
expect(html).not.toContain('<script>')
expect(html).not.toContain('<b>x</b>') // the literal tag is escaped, not rendered
})
it('opens links safely in a new tab', () => {
const html = render('[seeds](https://example.com)')
expect(html).toContain('href="https://example.com"')
expect(html).toContain('rel="noopener noreferrer"')
})
})
+73
View File
@@ -0,0 +1,73 @@
import ReactMarkdown, { type Components } from 'react-markdown'
import remarkGfm from 'remark-gfm'
// The assistant answers in Markdown — including GFM tables — so render it as such
// instead of showing the raw source. react-markdown does NOT emit raw HTML (no
// rehype-raw), so an LLM reply can't inject markup; remark-gfm adds tables,
// strikethrough and task lists. Tailwind's reset strips default list/table
// styling, so every element the assistant actually uses is restyled here, scaled
// for a chat bubble. Wide content (tables, code) scrolls inside its own box so
// the bubble never blows out.
const components: Components = {
Outdated
Review

🟠 Assistant Markdown renders tags unrestricted, allowing prompt-injection-driven data exfiltration via auto-loaded image beacons

security · flagged by 2 models

  • web/src/editor/MarkdownMessage.tsx:11-62 — The custom components map overrides p, a, ul, code, table, etc., but never overrides (or disables) img. Confirmed by reading the full file: no img key exists in the object spanning lines 11–62. react-markdown's default img renderer emits a real <img src="…">, which the browser fetches unconditionally, with no click required — unlike the a tag here, which needs a user click and gets rel="noopener noreferrer" (confirmed a…

🪰 Gadfly · advisory

🟠 **Assistant Markdown renders <img> tags unrestricted, allowing prompt-injection-driven data exfiltration via auto-loaded image beacons** _security · flagged by 2 models_ - **`web/src/editor/MarkdownMessage.tsx:11-62`** — The custom `components` map overrides `p`, `a`, `ul`, `code`, `table`, etc., but never overrides (or disables) `img`. Confirmed by reading the full file: no `img` key exists in the object spanning lines 11–62. `react-markdown`'s default `img` renderer emits a real `<img src="…">`, which the browser fetches unconditionally, with no click required — unlike the `a` tag here, which needs a user click and gets `rel="noopener noreferrer"` (confirmed a… <sub>🪰 Gadfly · advisory</sub>
p: ({ children }) => <p className="my-1.5 first:mt-0 last:mb-0">{children}</p>,
a: ({ href, children }) => (
<a
href={href}
target="_blank"
rel="noopener noreferrer"
className="text-accent-strong underline underline-offset-2"
>
{children}
</a>
),
ul: ({ children }) => <ul className="my-1.5 list-disc pl-5">{children}</ul>,
ol: ({ children }) => <ol className="my-1.5 list-decimal pl-5">{children}</ol>,
Outdated
Review

🟡 Custom ol renderer drops the start prop, so ordered lists not beginning at 1 always render numbered from 1

correctness · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Custom `ol` renderer drops the `start` prop, so ordered lists not beginning at 1 always render numbered from 1** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
li: ({ children }) => <li className="my-0.5">{children}</li>,
strong: ({ children }) => <strong className="font-semibold">{children}</strong>,
em: ({ children }) => <em className="italic">{children}</em>,
h1: ({ children }) => <h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h3>,
Outdated
Review

🟡 h5/h6 headings are not mapped, so they render unstyled under Tailwind preflight (inconsistent with h1–h4)

correctness, maintainability · flagged by 3 models

  • web/src/editor/MarkdownMessage.tsx:28-35h5 and h6 are unmapped, so they lose all styling. The components map overrides h1h4 (collapsing them to h3/h4/h5 with font-semibold, sizing, etc.), but h5 and h6 are not mapped at all. Tailwind's preflight resets headings to font-size: inherit; font-weight: inherit, so if the assistant emits ##### Heading or ###### Heading, react-markdown renders a bare <h5>/<h6> with no custom component → it shows up as plain, u…

🪰 Gadfly · advisory

🟡 **h5/h6 headings are not mapped, so they render unstyled under Tailwind preflight (inconsistent with h1–h4)** _correctness, maintainability · flagged by 3 models_ - **`web/src/editor/MarkdownMessage.tsx:28-35` — `h5` and `h6` are unmapped, so they lose all styling.** The `components` map overrides `h1`–`h4` (collapsing them to `h3`/`h4`/`h5` with `font-semibold`, sizing, etc.), but `h5` and `h6` are not mapped at all. Tailwind's preflight resets headings to `font-size: inherit; font-weight: inherit`, so if the assistant emits `##### Heading` or `###### Heading`, react-markdown renders a bare `<h5>`/`<h6>` with no custom component → it shows up as plain, u… <sub>🪰 Gadfly · advisory</sub>
h2: ({ children }) => <h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h3>,
h3: ({ children }) => <h4 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h4>,
h4: ({ children }) => (
<h5 className="mb-1 mt-1.5 text-xs font-semibold uppercase tracking-wide text-muted first:mt-0">
{children}
</h5>
Review

🟡 h1 and h2 render identical JSX - duplicated code should be shared

maintainability · flagged by 1 model

  • web/src/editor/MarkdownMessage.tsx:34-35h1 and h2 map to literally identical JSX (<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">). This is duplicated code that should be shared via a single styled-heading component or constant. When the chat-bubble heading style needs to change, a maintainer has to remember to update two identical lines.

🪰 Gadfly · advisory

🟡 **h1 and h2 render identical JSX - duplicated code should be shared** _maintainability · flagged by 1 model_ * **`web/src/editor/MarkdownMessage.tsx:34-35`** — `h1` and `h2` map to literally identical JSX (`<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">`). This is duplicated code that should be shared via a single styled-heading component or constant. When the chat-bubble heading style needs to change, a maintainer has to remember to update two identical lines. <sub>🪰 Gadfly · advisory</sub>
),
blockquote: ({ children }) => (
<blockquote className="my-1.5 border-l-2 border-border pl-2 text-muted">{children}</blockquote>
),
hr: () => <hr className="my-2 border-border" />,
code: ({ className, children }) => {
Review

🟠 No-language fenced code blocks styled as inline pills

correctness, performance · flagged by 4 models

  • web/src/editor/MarkdownMessage.tsx:43 — Regex recompiled on every <code> element. The code component literal /language-/ is compiled fresh on every invocation. Impact: Tiny but unnecessary overhead every time a code span or block appears. Fix: Hoist to module scope (const languageRe = /language-/).

🪰 Gadfly · advisory

🟠 **No-language fenced code blocks styled as inline pills** _correctness, performance · flagged by 4 models_ - **`web/src/editor/MarkdownMessage.tsx:43` — Regex recompiled on every `<code>` element.** The `code` component literal `/language-/` is compiled fresh on every invocation. **Impact:** Tiny but unnecessary overhead every time a code span or block appears. **Fix:** Hoist to module scope (`const languageRe = /language-/`). <sub>🪰 Gadfly · advisory</sub>
// A fenced block carries a language- class and is wrapped by <pre> (styled
// below); inline code is a bare <code> and gets the pill treatment here.
if (/language-/.test(className ?? '')) return <code className={className}>{children}</code>
return (
<code className="rounded bg-border/60 px-1 py-0.5 font-mono text-[0.85em]">{children}</code>
)
},
pre: ({ children }) => (
<pre className="my-1.5 overflow-x-auto rounded-md bg-border/40 p-2 font-mono text-xs">
{children}
</pre>
),
table: ({ children }) => (
<div className="my-1.5 overflow-x-auto">
<table className="w-full border-collapse text-xs">{children}</table>
</div>
),
th: ({ children }) => (
Review

🟡 Custom th/td renderers hardcode text-left and drop the style prop, discarding GFM column alignment (:---:/---:)

correctness · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Custom `th`/`td` renderers hardcode `text-left` and drop the `style` prop, discarding GFM column alignment (`:---:`/`---:`)** _correctness · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
<th className="border border-border px-2 py-1 text-left font-semibold">{children}</th>
),
td: ({ children }) => <td className="border border-border px-2 py-1 align-top">{children}</td>,
}
/** Render one assistant message body as Markdown (GFM). */
export function MarkdownMessage({ children }: { children: string }) {
Outdated
Review

🔴 remarkPlugins array recreated every render, breaking ReactMarkdown processor memoization

performance · flagged by 4 models

  • web/src/editor/MarkdownMessage.tsx:68remarkPlugins array recreated on every render, invalidating processor memoization. The [remarkGfm] literal is a new array reference each render. ReactMarkdown memoizes its unified processor by the remarkPlugins prop; a new array forces it to rebuild the entire plugin pipeline (remark-parse → remark-gfm and its micromark extensions) on every keystroke. Impact: Every render of every assistant bubble pays the cost of reconstructing the M…

🪰 Gadfly · advisory

🔴 **remarkPlugins array recreated every render, breaking ReactMarkdown processor memoization** _performance · flagged by 4 models_ - **`web/src/editor/MarkdownMessage.tsx:68` — `remarkPlugins` array recreated on every render, invalidating processor memoization.** The `[remarkGfm]` literal is a new array reference each render. `ReactMarkdown` memoizes its `unified` processor by the `remarkPlugins` prop; a new array forces it to rebuild the entire plugin pipeline (remark-parse → remark-gfm and its micromark extensions) on every keystroke. **Impact:** Every render of every assistant bubble pays the cost of reconstructing the M… <sub>🪰 Gadfly · advisory</sub>
return (
<div className="leading-relaxed">
<ReactMarkdown remarkPlugins={[remarkGfm]} components={components}>
{children}
</ReactMarkdown>
</div>
)
}
+17 -14
View File
@@ -31,22 +31,25 @@ export default defineConfig(({ mode }) => {
sourcemap: true,
rollupOptions: {
output: {
// One vendor chunk for ALL node_modules: it's rarely-changing, so it
// caches across app deploys while the tiny app chunk churns. Kept as a
// SINGLE chunk on purpose — splitting react-dom/scheduler into their own
// chunk reorders their module init across chunk boundaries and breaks
// React 19 at load ("Cannot set 'Activity' of undefined"). The heavy
// routes are code-split separately via React.lazy (router.tsx), which is
// where the real first-paint win is.
// The app-wide core goes in ONE cached vendor chunk (rarely changes, so
// it survives deploys while the tiny app chunk churns). react + react-dom
// + scheduler MUST stay together — splitting react-dom into its own chunk
// reorders module init across chunk boundaries and breaks React 19 at
// load ("Cannot set 'Activity' of undefined"). Everything else — the
// gesture engine, the assistant's markdown renderer + its ecosystem —
// rides with whatever imports it, so a lazily-loaded route/feature keeps
// it out of the eager first paint. The routes are code-split via
// React.lazy (router.tsx), where the real first-paint win is.
manualChunks(id) {
if (!id.includes('node_modules')) return undefined
// Editor-only libs (the gesture engine) ride with the lazy editor
// chunk instead of the eager vendor one, so the first paint doesn't
// pay for code only the canvas needs. Everything else — react, tanstack
// and the rest — stays in ONE vendor chunk; splitting react-dom out
// reorders its init across chunks and breaks React 19 at load.
if (id.includes('@use-gesture')) return undefined
return 'vendor'
if (
/[/\\]node_modules[/\\](react|react-dom|scheduler|@tanstack|zustand|zod|clsx|tailwind-merge)[/\\]/.test(
id,
)
) {
return 'vendor'
}
return undefined
},
},
},