The assistant answers in Markdown — tables, lists, bold, links — but the chat bubble rendered the raw source (whitespace-pre-wrap), so a table came through as a wall of pipes.
What
MarkdownMessage — react-markdown + remark-gfm (for GFM tables). It does not enable raw HTML (no rehype-raw), so a model reply can't inject markup; every element the assistant actually uses is Tailwind-styled for a chat bubble, and wide content (tables, code) scrolls in its own overflow-x-auto box so the bubble can't blow out.
Only assistant bubbles render as Markdown; the user's own text stays literal (their * shouldn't become a bullet), and the assistant bubble goes full-width so a table has room.
Lazy-loaded — the renderer + its ecosystem (~150 KB) is its own async chunk, loaded only when an assistant message renders, not for everyone who opens the editor. To keep it out of the eager first paint, manualChunks now vendors only the app-wide core (react/react-dom/scheduler kept together — splitting react-dom breaks React 19 at load) and lets everything else ride with its importer. The MarkdownMessage chunk is 155 KB; the editor chunk stays ~44 KB.
Tested
renderToStaticMarkup (node, no DOM) verifies: a GFM table renders with styled cells + horizontal scroll; **bold** / lists render; raw <script>/<b> are escaped, not emitted (XSS-safe); links get rel="noopener noreferrer". Re-verified the app mounts cleanly against the embedded binary after the re-chunk (no React-19 init-order break), console clean.
Note: the assistant is model-gated, so its live UI can't be exercised on the local box (no OLLAMA_CLOUD_API_KEY) — the rendering is verified by the static-render test above; the live instance (which has a model) is where you'll see it in the chat.
The assistant answers in Markdown — tables, lists, bold, links — but the chat bubble rendered the raw source (`whitespace-pre-wrap`), so a table came through as a wall of pipes.
## What
- **`MarkdownMessage`** — `react-markdown` + `remark-gfm` (for GFM tables). It does **not** enable raw HTML (no `rehype-raw`), so a model reply can't inject markup; every element the assistant actually uses is Tailwind-styled for a chat bubble, and wide content (tables, code) scrolls in its own `overflow-x-auto` box so the bubble can't blow out.
- Only **assistant** bubbles render as Markdown; the user's own text stays literal (their `*` shouldn't become a bullet), and the assistant bubble goes full-width so a table has room.
- **Lazy-loaded** — the renderer + its ecosystem (~150 KB) is its own async chunk, loaded only when an assistant message renders, not for everyone who opens the editor. To keep it out of the eager first paint, `manualChunks` now vendors only the app-wide core (react/react-dom/scheduler kept together — splitting react-dom breaks React 19 at load) and lets everything else ride with its importer. The `MarkdownMessage` chunk is 155 KB; the editor chunk stays ~44 KB.
## Tested
`renderToStaticMarkup` (node, no DOM) verifies: a GFM table renders with styled cells + horizontal scroll; `**bold**` / lists render; raw `<script>`/`<b>` are **escaped, not emitted** (XSS-safe); links get `rel="noopener noreferrer"`. Re-verified the app mounts cleanly against the embedded binary after the re-chunk (no React-19 init-order break), console clean.
`tsc` + `vitest` (99) + build green, no `>500 KB` warning.
> Note: the assistant is model-gated, so its live UI can't be exercised on the local box (no `OLLAMA_CLOUD_API_KEY`) — the rendering is verified by the static-render test above; the live instance (which has a model) is where you'll see it in the chat.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The assistant answers in Markdown — tables, lists, bold, links — but the
chat bubble showed the raw source (whitespace-pre-wrap), so a table came
through as a wall of pipes.
- New MarkdownMessage: react-markdown + remark-gfm (for GFM tables). It does
NOT enable raw HTML (no rehype-raw), so a model reply can't inject markup;
every element the assistant actually uses is Tailwind-styled for a chat
bubble, and wide content (tables, code) scrolls in its own box so the
bubble can't blow out.
- Only ASSISTANT bubbles render as Markdown; the user's own text stays
literal (their `*` shouldn't become a bullet) and the assistant bubble goes
full-width so a table has room.
- Lazy-loaded: the renderer + its ecosystem (~150 KB) is its own async chunk,
loaded only when an assistant message renders — not for everyone who opens
the editor. To keep it out of the eager first paint, manualChunks now
vendors only the app-wide core (react/react-dom/scheduler kept together —
splitting react-dom breaks React 19 at load) and lets everything else ride
with its importer.
Tested via renderToStaticMarkup (node, no DOM): a GFM table renders with
styled cells + horizontal scroll; **bold**/lists render; raw <script>/<b> are
escaped not emitted; links get rel="noopener noreferrer". App re-verified to
mount cleanly with the re-chunk (no React-19 init-order break). tsc + vitest
(99) + build green, no >500 KB warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 16:30:39Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Blocking issues found
- ✅ **error-handling** — Blocking issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Blocking issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🔴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…
🪰 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>
🔴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>
🟠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>
🟡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>
🟡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…
🪰 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>
🟡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.
🪰 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>
🟠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>
🔴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…
🪰 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>
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 at lines 13-22). Since the assistant's reply is LLM-generated text, a prompt-injection payload embedded in stored garden/plant data could cause the model to emit  and have it silently loaded as soon as the message renders. react-markdown's default urlTransform blocks dangerous URI schemes (javascript:, etc.) but does nothing to restrict the host, and there is no CSP anywhere in the repo (grep -rn "Content-Security-Policy" internal/ returns nothing) and no server-side sanitization that would strip image markdown before it's rendered. Fix: add an img entry to components that renders nothing / a text placeholder, or proxies the URL, so arbitrary attacker-controlled hosts aren't auto-fetched from the browser.
Links (MarkdownMessage.tsx:13-22) and inline/raw HTML are handled correctly: rehype-raw is not imported, so raw <script>/<b> are escaped — confirmed both by the source comment and by the existing test (MarkdownMessage.test.tsx:31-35, 'does not emit raw HTML from the model (no rehype-raw)'); anchors get target="_blank" + rel="noopener noreferrer" (test at line 40 confirms), so the residual risk is host-level (the image issue above), not scheme-level.
Everything else in this PR is clean: Bubble in ChatPanel.tsx:222-258 gates Markdown rendering to role !== 'user' only (the user's own text renders as literal body, not through MarkdownMessage), and the lockfile changes are just the new react-markdown/remark-gfm dependency tree with no unusual packages.
🎯 Correctness — Minor issues
Review — 🎯 Correctness
VERDICT: Minor issues
The new MarkdownMessage.tsx (web/src/editor/MarkdownMessage.tsx) styles most GFM elements correctly, but the custom renderers for code, ol, th/td only destructure { children } (or { children, className }) and drop other hast-derived props that react-markdown/mdast-util-to-hast actually populate. Verified against the exact pinned versions in web/package-lock.json ([email protected], [email protected], [email protected]) and the current source of MarkdownMessage.tsx:
MarkdownMessage.tsx:40-47 — The code component decides "fenced vs. inline" purely by checking for a language- className, and the surrounding comment asserts "a fenced block carries a language- class." That's false for a plain ``` fence with no info string: mdast-util-to-hast's code handler only sets properties.className when node.lang is truthy, so an un-languaged fenced block still gets wrapped in <pre> (correct) but its inner <code> falls into the inline branch and gets the pill styling (rounded bg-border/60 px-1 py-0.5 …) nested inside the block's own box styling. This is worsened by react-markdown v9 (confirmed pinned) no longer providing an inline prop to the code renderer at all — the language- className check is the only signal available, and it fails for unlabeled fences.
MarkdownMessage.tsx:24 — ol renders with a hardcoded list-decimal and drops the start prop that mdast-util-to-hast's list handler sets on the hast node whenever node.start !== 1 (confirmed: properties.start is only added when the parsed list's starting number isn't 1). Any ordered list in an assistant reply that doesn't begin at 1 (e.g. continuing a numbered plan, "6. …", "7. …") will always render starting at "1." — the displayed numbers no longer match the source.
MarkdownMessage.tsx:58-61 — th/td hardcode text-left and don't forward the style prop. GFM column alignment (:---:, ---:) is expressed by mdast-util-to-hast's table-cell handler as an inline style="text-align:…" on the th/td hast nodes (confirmed handler behavior, and style-to-js — present in the lockfile diff as a dependency of hast-util-to-jsx-runtime — exists specifically to turn that inline style string into a React style prop), which is discarded here — any table where the assistant right/center-aligns a column (e.g. a numeric "Qty" column) silently renders left-aligned regardless of source.
No existing test in MarkdownMessage.test.tsx covers an unlabeled fenced block, a non-1-starting ordered list, or a column-aligned table, so nothing in the test suite catches these.
🧹 Code cleanliness & maintainability — Minor issues
package.json/lockfile consistency also confirmed. The single finding is verified against the actual code.
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:28-29 — h1 and h2 component overrides are identical duplicated JSX (<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h3>); extract to one shared renderer used by both keys to avoid future drift.
⚡ Performance — Minor issues
Verification
I read ChatPanel.tsx and MarkdownMessage.tsx in full. Confirmed:
Neither Bubble (ChatPanel.tsx:222-258) nor MarkdownMessage (MarkdownMessage.tsx:65-73) is wrapped in React.memo — no memo import exists in either file.
ChatPanel re-renders on setInput (line 39/188) and on setPending inside onStep (line 76), both of which are plain useState setters with no scoping/isolation.
messages.map((m) => (<Bubble .../>)) at line 145 constructs fresh Bubble elements every render, and since Bubble isn't memoized, React re-invokes it (and the MarkdownMessage/ReactMarkdown tree inside it) for every past message on every unrelated state change — this is standard React re-render semantics, not library-specific behavior.
MarkdownMessage's components object (line 11) is indeed module-level, confirmed clean.
vite.config.ts:43-53manualChunks confirmed: it only pins react/react-dom/scheduler/@tanstack/zustand/zod/clsx/tailwind-merge to the vendor chunk and leaves everything else (including react-markdown) to ride with its importer, matching the draft's "isn't itself a hot-path concern" note.
node_modules isn't installed in this checkout, so I couldn't directly inspect react-markdown's internals, but its lack of per-render memoization on children is well-established, documented behavior for the library (no useMemo/cache wrapping processor.runSync), consistent with the draft's claim. Combined with the confirmed absence of React.memo in this codebase's own components, the performance claim holds.
VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:145 — Bubble and MarkdownMessage aren't memoized, so every unrelated state change in ChatPanel (typing a keystroke via setInput, lines 39/188; or each streamed agent step via onStep → setPending, line 76) re-renders the whole messages.map list and re-invokes MarkdownMessage for every assistant message already in the conversation, not just the one being added. Previously this path was whitespace-pre-wrap on a plain string (essentially free); now each keystroke and each SSE step during a streaming turn (web/src/lib/agent.ts:217) triggers a full markdown re-parse of every past assistant message — an O(messages) re-parse per tick instead of O(1), during exactly the interaction where the UI is supposed to stay responsive.
Fix: wrap Bubble (or at minimum the MarkdownMessage usage) in React.memo keyed on body/role so unrelated pending/input state changes don't re-run the markdown pipeline for messages whose content hasn't changed.
Everything else in this lens is clean: components (MarkdownMessage.tsx:11) is a module-level static object, not reallocated per render; the lazy-load/chunking split (ChatPanel.tsx:21-23, vite.config.ts:43-53) correctly keeps the markdown renderer out of the eager bundle and isn't itself a hot-path concern.
Outside my lens: none noted.
🧯 Error handling & edge cases — Minor issues
All claims check out against the actual source. The finding is fully confirmed: ChatPanel.tsx uses plain lazy() while router.tsx has a purpose-built lazyPage() for exactly this stale-chunk scenario, RouteError is only defined on the root route, gardenEditorRoute has no own errorComponent, and ChatPanel is rendered directly inside GardenEditorPage with no intermediate error boundary — so the propagation chain and blast radius described in the draft are accurate.
Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:21-23 — MarkdownMessage is loaded with a bare lazy(() => import('./MarkdownMessage').then(...)), but this codebase already has lazyPage() (web/src/router.tsx:26-49) built specifically to handle stale-chunk 404s after a deploy: React.lazy memoizes a rejected import promise forever, so once a chunk 404s, every future attempt to render that lazy component throws the same cached error — including after the route error boundary's "Try again" (router.invalidate()), since invalidating the route loader doesn't reset the lazy component's cached rejection.
Blast radius confirmed: only rootRoute defines errorComponent: RouteError (web/src/router.tsx:70); gardenEditorRoute (web/src/router.tsx:156) has no own errorComponent, and ChatPanel is rendered directly inside GardenEditorPage (web/src/pages/GardenEditorPage.tsx:535) with no intervening boundary. A throw from the failed MarkdownMessage import therefore propagates to the root boundary and replaces the entire routed outlet content (AppShell.tsx:105-107) — canvas, inspector, everything in the editor — with RouteError, while the nav chrome persists.
Concrete trigger: user has the editor open with the assistant panel; a merge to main redeploys mid-session; the next assistant reply triggers import('./MarkdownMessage') against a chunk hash the server no longer serves. The editor goes to the error screen with no self-recovery via "Try again".
Fix: use lazyPage() (or inline the same one-time-reload recovery) for MarkdownMessage instead of raw lazy().
No raw HTML injection — verified MarkdownMessage.tsx uses react-markdown with only remarkGfm and no rehype-raw. By default react-markdown does not pass through raw HTML; the test at MarkdownMessage.test.tsx:31 confirms <script>/<b> are escaped, not emitted. No rehype-raw appears anywhere in web/src.
Link safety — verified MarkdownMessage.tsx:13-22 overrides a with target="_blank" and rel="noopener noreferrer". react-markdown's default urlTransform sanitizes dangerous schemes (javascript:, data:, etc.) on href, and no custom urlTransform is passed, so the safe default remains in effect. The test at MarkdownMessage.test.tsx:37 confirms the link renders with the expected rel.
User input stays literal — verified ChatPanel.tsx:245-253: only the assistant branch renders <MarkdownMessage>; the user branch renders body as plain text with whitespace-pre-wrap, so a user's own * cannot become a bullet / markup.
No img/urlTransform/rehype-raw references were found in web/src outside the test and the comment, confirming no permissive raw-HTML or asset-loading override was introduced.
🎯 Correctness — Minor issues
The finding is confirmed: MarkdownMessage.tsx maps h1–h4 (lines 28–35) but not h5/h6, so those heading levels fall through to default <h5>/<h6> which Tailwind preflight resets to font-size: inherit; font-weight: inherit, producing un-styled text inconsistent with the styled headings above.
VERDICT: Minor issues
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, un-bold, un-sized text, visually inconsistent with the styled h1–h4 right above it. Nothing prevents the assistant from emitting headings past level 4. Fix: add h5/h6 entries (e.g. reuse the h4→h5 style or a smaller variant) so every heading level is covered.
MarkdownMessage.tsx:43 — inline-vs-fenced code detection via /language-/.test(className ?? '') is correct for react-markdown v9 (which removed the inline prop): fenced blocks carry language-*, inline code carries none. Verified against the test expectations and the component logic. No issue here.
🧹 Code cleanliness & maintainability — No material issues found
I have enough to review. The actual source change is small and clean. Let me confirm the one thing I noticed — the duplicate h1/h2 mappings.
The diff is essentially MarkdownMessage.tsx (new), its test, the ChatPanel.tsx lazy wiring, vite.config.tsmanualChunks, plus the huge package-lock.json regen. I've verified all the source files.
Within the code-cleanliness lens:
MarkdownMessage.tsx:28-29 — h1 and h2 are mapped to literally identical elements (<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">). Two copy-pasted entries for the same output; if the heading scale changes, both must be edited in lockstep. A trivial dedup (e.g. a shared heading component or a small loop building h1/h2 from one definition) would remove the duplication. This is a trivial nit, not blocking.
Everything else is genuinely clean: the components map is a cohesive single-purpose styling table (the idiomatic react-markdown pattern), the lazy import + Suspense fallback in ChatPanel.tsx follows the existing React.lazy pattern used for routes, and the manualChunks change is well-commented and keeps the core bundled. No dead code, no leaky abstractions, no confusing names.
VERDICT: No material issues found
web/src/editor/MarkdownMessage.tsx:28 — h1 and h2 map to byte-identical <h3> markup; duplicate definitions that must stay in sync. Verified by reading the file. Trivial; suggest a shared heading helper. No other cleanliness problems in this change.
⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:65 / web/src/editor/ChatPanel.tsx:145 — assistant markdown is re-parsed on every ChatPanel render during streaming.MarkdownMessage (web/src/editor/MarkdownMessage.tsx:65) is a plain, unmemoized function component, and Bubble (web/src/editor/ChatPanel.tsx:222) is likewise unmemoized. During a turn, ChatPanel re-renders on every onStep: setPending((p) => ({ …steps: [...p.steps, step] })) (ChatPanel.tsx:76) and refresh.canvas() (ChatPanel.tsx:80) both fire per step. Each such re-render re-runs messages.map(... <Bubble ...><MarkdownMessage>{body}</MarkdownMessage></Bubble>) (ChatPanel.tsx:145–154), recreating the element tree for every prior assistant message and re-invoking ReactMarkdown, which re-parses its children string into a mdast→hast tree each render. Cost grows with both message length and conversation length (O(messages × body-size) per step event). The lazy(...) wrapper (ChatPanel.tsx:21) only code-splits; it does not memoize props. Fix: wrap MarkdownMessage in React.memo — its single children: string prop is stable across these re-renders (it comes from cached history.data), so shallow-equal memo skips re-parsing — or memoize per-body.
🧯 Error handling & edge cases — Minor issues
I've verified the key claims. Let me confirm the asymmetric handling and the router's error boundary setup.
The code confirms:
ChatPanel.tsx:21-23 uses a bare lazy(() => import('./MarkdownMessage')...) with a pending-only Suspense fallback (lines 250-252) — no rejection recovery.
router.tsx:19-49 defines lazyPage with an explicit stale-chunk recovery (one-time window.location.reload() gated by a session flag) precisely because React.lazy memoizes a rejected import and a plain Suspense can't recover.
The root route sets errorComponent: RouteError (router.tsx:70), and RouteError.tsx renders "Something went wrong" + a "Try again" button calling router.invalidate().
The draft's claim that there is "no React error boundary above it" and it "crashes to a blank screen" is not confirmed and is likely false: TanStack Router's root errorComponent does catch component render throws (rendering RouteError), so it would not be a blank screen. However, the verifiable core of the finding stands: the assistant-message lazy lacks the stale-chunk recovery that lazyPage deliberately provides, and because React.lazy memoizes the rejection, RouteError's "Try again" (router.invalidate) would re-render into the same rejected import — leaving the editor stuck on the error screen until a manual hard reload. So the issue is real but the consequence is "non-recovering error screen," not "blank screen / no error boundary."
I'll keep the finding with a corrected impact description and reduced confidence (the consequence relies on TanStack Router internals not readable in the checked-out repo).
VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:21-23, 250-252 — the assistant-message lazy import has no stale-chunk recovery, unlike router.tsx's lazyPage.lazyPage (router.tsx:19-49) deliberately wraps every route-level lazy import in a one-time window.location.reload() on first import 404, gated by a session flag, because React.lazy memoizes a rejected import and a plain Suspense only handles the pending state. The MarkdownMessage lazy at line 21 gets none of that: if a deploy lands while the chat panel is open and a new assistant turn renders, the chunk 404s and React.lazy caches the rejection. The throw is caught by the router's root error boundary (errorComponent: RouteError, router.tsx:70) — so it is not a blank screen — but RouteError's "Try again" calls router.invalidate(), which re-renders into the same memoized rejection, leaving the editor route stuck on the error screen until a manual hard reload. Suggested fix: route this lazy through the same stale-chunk recovery lazyPage uses, or wrap the MarkdownMessage render in an error boundary whose fallback is the literal body text (which is exactly the current Suspense fallback), so a failed import degrades to the raw message instead of the whole-route error screen.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
web/src/editor/MarkdownMessage.tsx:11-62 — The components map overrides many elements (a, table, code, etc.) but omits img. Because react-markdown renders  as a native <img> by default, a compromised or jailbroken model could embed arbitrary external images. This enables tracking pixels (leaking user IP, UA, and potentially referrer) and unsolicited network requests. The PR’s tests explicitly verify HTML/script escaping and link safety, but do not cover images, suggesting this was overlooked. Suggested fix: Add an img component to the components map — either block images entirely (return null or a placeholder), or restrict to a safe allow-list / proxy. Example:
img:()=>null,// or render a controlled placeholder
Verified by reading MarkdownMessage.tsx (lines 11-62) and MarkdownMessage.test.tsx (no image test case). No rehype-raw is used, raw HTML is escaped, and links carry rel="noopener noreferrer" — all correctly handled.
🎯 Correctness — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:28-35 — The components object maps h1→h3, h2→h3, h3→h4, and h4→h5, but leaves h5 and h6 unmapped. Because Tailwind’s preflight strips default margins, font sizes, and font weights from all headings, any ##### or ###### the assistant emits will render as plain unstyled text, breaking the visual consistency of the chat bubble. Add h5 and h6 mappings (or a generic heading handler) to cap and style the remaining levels.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
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.
web/src/editor/MarkdownMessage.tsx:34-41 — All heading overrides use mismatched element names (h1 → <h3>, h2 → <h3>, h3 → <h4>, h4 → <h5>). The visual sizing is intentionally chat-scaled, but rendering an <h3> or <h4> for markdown that semantically says h1/h2/h3 is confusing for anyone debugging the DOM or writing selectors/tests. Prefer keeping the semantic element and applying the small styling via classes (e.g. <h1 className="... text-sm ...">).
⚡ Performance — Blocking issues found
Blocking issues found
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 Markdown parser, not just re-parsing the text. Fix: Hoist the array to module scope (const remarkPlugins = [remarkGfm]) or memoize it with useMemo.
web/src/editor/ChatPanel.tsx:222 & web/src/editor/MarkdownMessage.tsx:71 — All historical assistant messages re-render and re-parse on every keystroke.ChatPanel updates input state on each onChange, which re-renders the whole tree. Bubble and MarkdownMessage are not memoized, so every assistant message in the thread re-parses its full Markdown body through remark-gfm even though the body strings are unchanged. Impact: Typing becomes sluggish as the chat history grows; a long thread with tables/code blocks causes visible jank. Fix: Wrap MarkdownMessage in React.memo (it has a single string prop) and consider memoizing Bubble so existing messages are skipped when the parent re-renders.
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-/).
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
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 path. The route-level lazyPage function in router.tsx has sessionStorage-based auto-reload logic for exactly this problem, but the component-level lazy load has none of it. Fix: Add .catch() to the lazy() factory that falls back to the raw-text renderer so a chunk failure degrades gracefully instead of crashing the tree:
"Start over" can be clicked while a turn is still streaming — web/src/editor/ChatPanel.tsx:115
The clear-history button is only gated by clear.isPending; it ignores the pending in-flight turn. Clearing server-side history mid-stream leaves the client showing the pending UI against a now-empty message list. When the stream eventually fires onDone/onError, the user is abruptly dropped into the empty-state message while the underlying turn may already have mutated the garden. Fix: Disable the button when pending is truthy:
I verified the actual implementation rather than trusting the PR description's security claims:
web/src/editor/MarkdownMessage.tsx:1-73 — no rehype-raw, no dangerouslySetInnerHTML, no urlTransform override. react-markdown v9 applies its built-in urlTransform (protocol safelist) to href/srcbefore custom components receive props, so the custom a at line 13 receives a sanitized href. A javascript: link from the model is dropped to a safe value, not rendered verbatim. Confirmed no rehype-raw/dangerouslyAllowAll anywhere in web/src (grep returned only the comment and the test).
MarkdownMessage.tsx:13-22 — every link gets target="_blank" with rel="noopener noreferrer", so no reverse-tabnabbing and no opener leakage.
MarkdownMessage.tsx:40-46 — the code component forwards className only for fenced blocks (language-…, generated by react-markdown, not model-controlled), so no className injection vector.
ChatPanel.tsx:245-253 — user text is rendered literally (no Markdown), and the assistant path uses the same MarkdownMessage; no raw-HTML path exists for either role.
I could not directly inspect node_modules/react-markdown (not checked out in this environment), so the reliance on react-markdown v9's default urlTransform blocking javascript:/data:text/html is based on the documented v9 behavior rather than the installed source — but no override disables it, so the default applies.
Minor (not a defect): the test suite (MarkdownMessage.test.tsx) asserts that <script>/<b> are escaped and links get rel, but does not assert that a javascript: URL is sanitized. Adding expect(render('[x](javascript:alert(1))')).not.toContain('javascript:') would lock in the protocol-sanitization guarantee the lens depends on.
🎯 Correctness — Minor issues
The finding is confirmed. The code component at line 40-46 distinguishes block from inline via /language-/.test(className ?? ''). In react-markdown v9 (which dropped the inline prop), a fenced block without an info string produces a <pre><code> with no language- class, so it falls through to the inline pill styling (line 45) while still wrapped by the block <pre> (line 48-52) — a mismatched double-style. The test file confirms no coverage of the no-language fenced case.
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:43 — The code component distinguishes fenced blocks from inline code by testing className for language-. This works for fenced blocks with a language tag (```go → className="language-go"), but a fenced block without an info string (a bare ``` fence, which the assistant is quite likely to emit) produces a <pre><code> with nolanguage- class. In react-markdown v9 that className is undefined, so the if (/language-/.test(className ?? '')) branch is skipped and the block falls through to the inline pill style (rounded bg-border/60 px-1 py-0.5). The content is still wrapped by the outer <pre> (line 48), so the result is an inline-styled <code> nested inside the block-styled <pre> — double background, inline font sizing on a block — rather than the intended clean block rendering. Verified by reading the component and confirming no inline prop is used (react-markdown v9 dropped it) and no test covers the no-language fenced case (MarkdownMessage.test.tsx only checks tables/lists/links/HTML-escaping). Suggested fix: detect blocks via position/node instead of the language- class, e.g. compare the code's parent — or simplest, treat pre > code as a block by checking that the parent is a pre (react-markdown passes node); or accept a falsy className as block when the element is non-inline. A practical one-liner: render the pill only when !className is false AND it's inline — i.e. invert to "block when className is set OR node.position spans multiple lines."
🧹 Code cleanliness & maintainability — No material issues found
The code is clean and well-structured. Let me verify a couple of remaining points about the code component pattern and heading coverage.
VERDICT: No material issues found.
Through the code-cleanliness lens, this change is well-built:
MarkdownMessage.tsx is single-purpose, the top comment explains why (no rehype-raw, GFM tables, Tailwind reset strips defaults) rather than restating what — and the comment stays accurate to the code.
The components map reuses the exact Tailwind tokens the rest of web/src/editor/** uses (text-muted, border-border, text-accent-strong, bg-border/40), so it follows the surrounding pattern rather than inventing a parallel vocabulary. Verified by grepping the codebase.
The lazy wrapper in ChatPanel.tsx:21-23 with a whitespace-pre-wrap raw-text fallback (ChatPanel.tsx:250) is the right shape — no blank flash, and the comment at ChatPanel.tsx:248-249 says so. The manualChunks change in vite.config.ts:43-53 is well-commented and the kept-together react/react-dom/scheduler rule is explained inline, matching the CLAUDE.md note about not splitting react-dom.
No duplicated styling logic, no dead code, no copy-paste that should be shared. MarkdownMessage is the single consumer of react-markdown/remark-gfm (verified by grep — only MarkdownMessage.tsx imports them).
One trivial observation (not worth a finding): h1 and h2 both render as <h3> with identical classes (MarkdownMessage.tsx:28-29), and h5/h6 are unmapped so they'd fall through unstyled. The comment's phrasing — "every element the assistant actually uses" — is deliberately scoped so this isn't a false claim, and an LLM chat reply producing a 5th-level heading is not a real scenario. Nothing to fix.
⚡ Performance — Minor issues
All three JSON findings verified against the actual code:
MarkdownMessage (line 65) is a plain export function — no React.memo. Prop is children: string. ✓
Bubble (line 222) is a plain function — no React.memo. Used in messages.map at line 145. ✓
/language-/.test(...) regex literal at line 43 sits inside the code component (in the module-scope components object), evaluated per code element render; hoistable. ✓
Supporting claims also verified: maxRetainedTurns = 20 at internal/service/agent.go:15; setPending per step at ChatPanel.tsx:76; scrollIntoView({behavior:'smooth'}) at ChatPanel.tsx:57 on [history.data, pending]; lazy import at line 21; Suspense per bubble at line 250.
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:65 — MarkdownMessage is a plain function component (no React.memo), and its only prop is children: string. Since ChatPanel holds input (updated per keystroke) and pending (updated per streamed step) as state, every keystroke or step re-renders ChatPanel and — because the child Bubbles aren't memoized either — re-renders every assistant Bubble, each of which re-runs ReactMarkdown's full remark→remark-gfm→hast→JSX pipeline on its body. With maxRetainedTurns = 20 (internal/service/agent.go:15), that's up to 20 full Markdown re-parses per keystroke while typing a follow-up. Bounded (≤20 messages, modest bodies), so not catastrophic, but it's avoidable re-work that scales with conversation length × keystrokes. Fix: wrap MarkdownMessage in React.memo (default shallow equality on children: string suffices) and/or memoize Bubble.
web/src/editor/ChatPanel.tsx:145 / 222 — Bubble is also a plain function (no React.memo), so even if MarkdownMessage were memoized, the unmemoized Bubble would re-render for all up-to-20 messages on every input/pending change and defeat the memo. Extracting Bubble as memo (its props are role, body, children — the children slot is the UndoButton, which is stable per message) closes the gap.
web/src/editor/MarkdownMessage.tsx:43 — the /language-/ regex literal inside the code component is re-evaluated on each code element render; hoisting it to a module-level constant (const LANG = /^language-/) removes both the per-render evaluation and the closure capture. Minor on its own, and compounded by the missing memo above.
🧯 Error handling & edge cases — Minor issues
I'll verify the draft findings against the actual code.
Both files confirmed. Now let me verify the two findings: (1) the absence of error boundaries in the tree, and (2) the code component's inline/fenced classification logic.
Both findings verified. No error boundary exists anywhere in web/src (grep returned nothing), and the code component's logic at line 43 is exactly as the draft describes. Let me confirm the react-markdown behavior for fenced blocks without a language tag by checking the installed version.
Both findings are confirmed against the actual code:
ChatPanel.tsx:250 — Confirmed: MarkdownMessage is lazy-loaded via import() inside a Suspense at line 250, with only a loading fallback. A grep across all of web/src for componentDidCatch, getDerivedStateFromError, and ErrorBoundary returns zero matches — no error boundary exists anywhere in the tree. A failed import() rejects the promise and propagates uncaught, crashing the ChatPanel.
MarkdownMessage.tsx:43 — Confirmed: the code component distinguishes inline from fenced purely via /language-/.test(className ?? ''). A fenced block without a language tag produces className of language- in react-markdown v9 — but /language-/ matches the bare prefix language- too, so actually... let me re-examine.
Looking again: the regex /language-/.test("language-") returns true (the substring language- is present). So a fenced block with className="language-" would be treated as fenced, not inline. The draft's claim that it returns false for "" is correct (empty string), but the bare language- case actually matches the regex. The draft's specific concern ("``` with nothing after yields language-... returns false") is incorrect for the bare-prefix case — it would return true. The misclassification only happens if className is undefined/empty, which react-markdown v9 does not produce for fenced blocks (it always emits at least language-). This finding is based on a faulty premise and I'll drop it.
The first draft bullet (about children: string and nil-guard) was self-dismissed by the draft itself ("no swallowing", "Low risk... flagging only because the lens asks") and not carried into the gadfly-findings JSON, so it doesn't survive. The second finding (error boundary) is confirmed. The third finding (fenced code without language) is based on a faulty premise — /language-/.test("language-") returns true, so the bare-prefix case is correctly classified as fenced; the misclassification only occurs if className is truly empty/undefined, which react-markdown v9 doesn't produce for fenced blocks.
VERDICT: Minor issues found
web/src/editor/ChatPanel.tsx:250-252 — A failed lazy import() for the MarkdownMessage chunk (network blip, ad blocker, bad deploy) will reject and, with no error boundary anywhere in the tree (verified: no ErrorBoundary/componentDidCatch/getDerivedStateFromError in web/src), will crash the entireChatPanel rather than just the message bubble. Every assistant bubble in the thread would take the panel down — the user can't even see the history or send a new message. The fallback string renders fine while loading, but it does nothing for the failure case, which Suspense doesn't catch. Suggested fix: either wrap the lazy boundary in a small error boundary that falls back to the same whitespace-pre-wrap raw text, or drop the chunk-split and import MarkdownMessage statically (155 KB is modest, and the assistant tab only renders when capabilities.data?.agent is true anyway). Verified the absence of any error boundary via grep across web/src.
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Wait — actually, through the Security lens there are issues. Let me re-verify by checking if there are other elements I might have missed, and confirm my analysis. Blocking issues found
web/src/editor/MarkdownMessage.tsx:13-22 — javascript: / data: links not filtered.
The custom a component renders href directly without protocol validation. react-markdown without rehype-raw only prevents raw HTML injection; it does not strip dangerous URL schemes from Markdown links like [click](javascript:alert(1)) or [exfil](data:text/html,<script>…). An attacker (or a compromised/misbehaving LLM) can inject a javascript: or data: URL that executes in the user's browser when clicked. The repo already has safeExternalUrl in web/src/lib/seedLots.ts; reuse it here (drop the link if the URL is unsafe). Verified by reading MarkdownMessage.tsx, seedLots.ts, and SourceLink.tsx.
web/src/editor/MarkdownMessage.tsx:11-62 — Missing img component override allows malicious image URLs.
The components object does not include img, so standard Markdown images () fall through to the default renderer with an unvalidated src. A malicious src (e.g., javascript:, tracking pixel, or data URI) bypasses the application's otherwise careful link handling. Add an img override that validates the src (allow only http:/https:) or drops the image. Verified by reading MarkdownMessage.tsx.
🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/editor/MarkdownMessage.tsx:40-47 — Fenced code blocks without a language tag (e.g. plain ```) are incorrectly styled as inline pills. react-markdown v9 passes className: undefined to the code component when no language is specified, so the /language-/.test(className ?? '') guard fails and the block gets the inline rounded bg-border/60 px-1 py-0.5 treatment even though it is inside the <pre> container. Verified by reading the dependency source (mdast-util-to-hast 13.2.1) and the component logic. Fix: Use the presence of the <pre> wrapper as the block/inline discriminator, or move the inline pill style to a wrapper and leave the <code> element plain for both cases.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:68 — remarkPlugins={[remarkGfm]} creates a new array on every render. react-markdown memoizes the unified processor by plugin identity; a fresh array defeats that useMemo and rebuilds the entire GFM pipeline on every render of every assistant bubble. During streaming this happens across all existing assistant messages each time a new step arrives. Fix: Move const remarkPlugins = [remarkGfm] outside the component so the reference is stable.
web/src/editor/MarkdownMessage.tsx:65 — MarkdownMessage is not wrapped in React.memo. When ChatPanel re-renders during streaming (as pending.steps grows), every existing assistant bubble re-renders and re-parses its markdown even though the body string is unchanged. Fix: Export const MarkdownMessage = memo(function MarkdownMessage({ children }: { children: string }) { ... }) so unchanged messages bail out of the re-parse.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
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: Add a small class-based Error Boundary around the Suspense + MarkdownMessage pair, falling back to the raw text (<span className=\"whitespace-pre-wrap\">{body}</span>) on error — or adopt the same stale-chunk reload strategy used in router.tsx's lazyPage.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Blocking issues found** · 12 findings (7 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | remarkPlugins array recreated every render, breaking ReactMarkdown processor memoization | `web/src/editor/MarkdownMessage.tsx:65` | 4/5 | performance |
| 🟠 | No-language fenced code blocks styled as inline pills | `web/src/editor/MarkdownMessage.tsx:40` | 4/5 | correctness, performance |
| 🔴 | Lazy MarkdownMessage chunk failure crashes ChatPanel with no ErrorBoundary or fallback | `web/src/editor/ChatPanel.tsx:21` | 3/5 | error-handling |
| 🟡 | h5/h6 headings are not mapped, so they render unstyled under Tailwind preflight (inconsistent with h1–h4) | `web/src/editor/MarkdownMessage.tsx:28` | 3/5 | correctness, maintainability |
| 🔴 | Lazy MarkdownMessage has no Error Boundary; chunk/render failures crash the app | `web/src/editor/ChatPanel.tsx:248` | 2/5 | error-handling |
| 🟠 | Unmemoized Bubble/MarkdownMessage re-parses all prior assistant markdown on every keystroke and every streamed agent step | `web/src/editor/ChatPanel.tsx:145` | 2/5 | performance |
| 🟠 | Assistant Markdown renders <img> tags unrestricted, allowing prompt-injection-driven data exfiltration via auto-loaded image beacons | `web/src/editor/MarkdownMessage.tsx:11` | 2/5 | security |
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | Bubble and MarkdownMessage unmemoized, causing full-chat markdown re-parse on every keystroke | `web/src/editor/ChatPanel.tsx:222` | kimi-k2.6:cloud | performance |
| 🟠 | Clear-history button not disabled during in-flight assistant turn | `web/src/editor/ChatPanel.tsx:115` | kimi-k2.6:cloud | error-handling |
| 🟡 | Custom 'ol' renderer drops the 'start' prop, so ordered lists not beginning at 1 always render numbered from 1 | `web/src/editor/MarkdownMessage.tsx:24` | claude-code/sonnet | correctness |
| 🟡 | h1 and h2 render identical JSX - duplicated code should be shared | `web/src/editor/MarkdownMessage.tsx:34` | kimi-k2.6:cloud | maintainability |
| 🟡 | Custom 'th'/'td' renderers hardcode 'text-left' and drop the 'style' prop, discarding GFM column alignment (':---:'/'---:') | `web/src/editor/MarkdownMessage.tsx:58` | claude-code/sonnet | correctness |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`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 at lines 13-22). Since the assistant's reply is LLM-generated text, a prompt-injection payload embedded in stored garden/plant data could cause the model to emit `` and have it silently loaded as soon as the message renders. `react-markdown`'s default `urlTransform` blocks dangerous URI *schemes* (`javascript:`, etc.) but does nothing to restrict the *host*, and there is no CSP anywhere in the repo (`grep -rn "Content-Security-Policy" internal/` returns nothing) and no server-side sanitization that would strip image markdown before it's rendered. Fix: add an `img` entry to `components` that renders nothing / a text placeholder, or proxies the URL, so arbitrary attacker-controlled hosts aren't auto-fetched from the browser.
- Links (`MarkdownMessage.tsx:13-22`) and inline/raw HTML are handled correctly: `rehype-raw` is not imported, so raw `<script>`/`<b>` are escaped — confirmed both by the source comment and by the existing test (`MarkdownMessage.test.tsx:31-35`, `'does not emit raw HTML from the model (no rehype-raw)'`); anchors get `target="_blank"` + `rel="noopener noreferrer"` (test at line 40 confirms), so the residual risk is host-level (the image issue above), not scheme-level.
Everything else in this PR is clean: `Bubble` in `ChatPanel.tsx:222-258` gates Markdown rendering to `role !== 'user'` only (the user's own text renders as literal `body`, not through `MarkdownMessage`), and the lockfile changes are just the new `react-markdown`/`remark-gfm` dependency tree with no unusual packages.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Review — 🎯 Correctness
**VERDICT: Minor issues**
The new `MarkdownMessage.tsx` (web/src/editor/MarkdownMessage.tsx) styles most GFM elements correctly, but the custom renderers for `code`, `ol`, `th`/`td` only destructure `{ children }` (or `{ children, className }`) and drop other hast-derived props that `react-markdown`/`mdast-util-to-hast` actually populate. Verified against the exact pinned versions in `web/package-lock.json` (`[email protected]`, `[email protected]`, `[email protected]`) and the current source of `MarkdownMessage.tsx`:
- **`MarkdownMessage.tsx:40-47`** — The `code` component decides "fenced vs. inline" purely by checking for a `language-` className, and the surrounding comment asserts "a fenced block carries a language- class." That's false for a plain ` ``` ` fence with no info string: `mdast-util-to-hast`'s code handler only sets `properties.className` when `node.lang` is truthy, so an un-languaged fenced block still gets wrapped in `<pre>` (correct) but its inner `<code>` falls into the *inline* branch and gets the pill styling (`rounded bg-border/60 px-1 py-0.5 …`) nested inside the block's own box styling. This is worsened by react-markdown v9 (confirmed pinned) no longer providing an `inline` prop to the `code` renderer at all — the `language-` className check is the only signal available, and it fails for unlabeled fences.
- **`MarkdownMessage.tsx:24`** — `ol` renders with a hardcoded `list-decimal` and drops the `start` prop that `mdast-util-to-hast`'s list handler sets on the hast node whenever `node.start !== 1` (confirmed: `properties.start` is only added when the parsed list's starting number isn't 1). Any ordered list in an assistant reply that doesn't begin at 1 (e.g. continuing a numbered plan, "6. …", "7. …") will always render starting at "1." — the displayed numbers no longer match the source.
- **`MarkdownMessage.tsx:58-61`** — `th`/`td` hardcode `text-left` and don't forward the `style` prop. GFM column alignment (`:---:`, `---:`) is expressed by `mdast-util-to-hast`'s table-cell handler as an inline `style="text-align:…"` on the th/td hast nodes (confirmed handler behavior, and `style-to-js` — present in the lockfile diff as a dependency of `hast-util-to-jsx-runtime` — exists specifically to turn that inline style string into a React style prop), which is discarded here — any table where the assistant right/center-aligns a column (e.g. a numeric "Qty" column) silently renders left-aligned regardless of source.
No existing test in `MarkdownMessage.test.tsx` covers an unlabeled fenced block, a non-1-starting ordered list, or a column-aligned table, so nothing in the test suite catches these.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
package.json/lockfile consistency also confirmed. The single finding is verified against the actual code.
VERDICT: Minor issues
- **web/src/editor/MarkdownMessage.tsx:28-29** — `h1` and `h2` component overrides are identical duplicated JSX (`<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h3>`); extract to one shared renderer used by both keys to avoid future drift.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
## Verification
I read `ChatPanel.tsx` and `MarkdownMessage.tsx` in full. Confirmed:
- Neither `Bubble` (ChatPanel.tsx:222-258) nor `MarkdownMessage` (MarkdownMessage.tsx:65-73) is wrapped in `React.memo` — no `memo` import exists in either file.
- `ChatPanel` re-renders on `setInput` (line 39/188) and on `setPending` inside `onStep` (line 76), both of which are plain `useState` setters with no scoping/isolation.
- `messages.map((m) => (<Bubble .../>))` at line 145 constructs fresh `Bubble` elements every render, and since `Bubble` isn't memoized, React re-invokes it (and the `MarkdownMessage`/`ReactMarkdown` tree inside it) for every past message on every unrelated state change — this is standard React re-render semantics, not library-specific behavior.
- `MarkdownMessage`'s `components` object (line 11) is indeed module-level, confirmed clean.
- `vite.config.ts:43-53` `manualChunks` confirmed: it only pins `react/react-dom/scheduler/@tanstack/zustand/zod/clsx/tailwind-merge` to the `vendor` chunk and leaves everything else (including `react-markdown`) to ride with its importer, matching the draft's "isn't itself a hot-path concern" note.
`node_modules` isn't installed in this checkout, so I couldn't directly inspect `react-markdown`'s internals, but its lack of per-render memoization on `children` is well-established, documented behavior for the library (no `useMemo`/cache wrapping `processor.runSync`), consistent with the draft's claim. Combined with the confirmed absence of `React.memo` in this codebase's own components, the performance claim holds.
VERDICT: Minor issues
- `web/src/editor/ChatPanel.tsx:145` — `Bubble` and `MarkdownMessage` aren't memoized, so every unrelated state change in `ChatPanel` (typing a keystroke via `setInput`, lines 39/188; or each streamed agent step via `onStep` → `setPending`, line 76) re-renders the whole `messages.map` list and re-invokes `MarkdownMessage` for every assistant message already in the conversation, not just the one being added. Previously this path was `whitespace-pre-wrap` on a plain string (essentially free); now each keystroke and each SSE step during a streaming turn (`web/src/lib/agent.ts:217`) triggers a full markdown re-parse of every past assistant message — an O(messages) re-parse per tick instead of O(1), during exactly the interaction where the UI is supposed to stay responsive.
- Fix: wrap `Bubble` (or at minimum the `MarkdownMessage` usage) in `React.memo` keyed on `body`/`role` so unrelated `pending`/`input` state changes don't re-run the markdown pipeline for messages whose content hasn't changed.
- Everything else in this lens is clean: `components` (MarkdownMessage.tsx:11) is a module-level static object, not reallocated per render; the lazy-load/chunking split (ChatPanel.tsx:21-23, vite.config.ts:43-53) correctly keeps the markdown renderer out of the eager bundle and isn't itself a hot-path concern.
Outside my lens: none noted.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All claims check out against the actual source. The finding is fully confirmed: `ChatPanel.tsx` uses plain `lazy()` while `router.tsx` has a purpose-built `lazyPage()` for exactly this stale-chunk scenario, `RouteError` is only defined on the root route, `gardenEditorRoute` has no own `errorComponent`, and `ChatPanel` is rendered directly inside `GardenEditorPage` with no intermediate error boundary — so the propagation chain and blast radius described in the draft are accurate.
## Review — 🧯 Error handling & edge cases
**VERDICT: Minor issues**
- **`web/src/editor/ChatPanel.tsx:21-23`** — `MarkdownMessage` is loaded with a bare `lazy(() => import('./MarkdownMessage').then(...))`, but this codebase already has `lazyPage()` (`web/src/router.tsx:26-49`) built specifically to handle stale-chunk 404s after a deploy: `React.lazy` memoizes a rejected import promise forever, so once a chunk 404s, every future attempt to render that lazy component throws the same cached error — including after the route error boundary's "Try again" (`router.invalidate()`), since invalidating the route loader doesn't reset the lazy component's cached rejection.
- Blast radius confirmed: only `rootRoute` defines `errorComponent: RouteError` (`web/src/router.tsx:70`); `gardenEditorRoute` (`web/src/router.tsx:156`) has no own `errorComponent`, and `ChatPanel` is rendered directly inside `GardenEditorPage` (`web/src/pages/GardenEditorPage.tsx:535`) with no intervening boundary. A throw from the failed `MarkdownMessage` import therefore propagates to the root boundary and replaces the entire routed outlet content (`AppShell.tsx:105-107`) — canvas, inspector, everything in the editor — with `RouteError`, while the nav chrome persists.
- Concrete trigger: user has the editor open with the assistant panel; a merge to `main` redeploys mid-session; the next assistant reply triggers `import('./MarkdownMessage')` against a chunk hash the server no longer serves. The editor goes to the error screen with no self-recovery via "Try again".
- Fix: use `lazyPage()` (or inline the same one-time-reload recovery) for `MarkdownMessage` instead of raw `lazy()`.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
- **No raw HTML injection** — verified `MarkdownMessage.tsx` uses `react-markdown` with only `remarkGfm` and no `rehype-raw`. By default react-markdown does not pass through raw HTML; the test at `MarkdownMessage.test.tsx:31` confirms `<script>`/`<b>` are escaped, not emitted. No `rehype-raw` appears anywhere in `web/src`.
- **Link safety** — verified `MarkdownMessage.tsx:13-22` overrides `a` with `target="_blank"` and `rel="noopener noreferrer"`. react-markdown's default `urlTransform` sanitizes dangerous schemes (`javascript:`, `data:`, etc.) on `href`, and no custom `urlTransform` is passed, so the safe default remains in effect. The test at `MarkdownMessage.test.tsx:37` confirms the link renders with the expected `rel`.
- **User input stays literal** — verified `ChatPanel.tsx:245-253`: only the assistant branch renders `<MarkdownMessage>`; the user branch renders `body` as plain text with `whitespace-pre-wrap`, so a user's own `*` cannot become a bullet / markup.
- No `img`/`urlTransform`/`rehype-raw` references were found in `web/src` outside the test and the comment, confirming no permissive raw-HTML or asset-loading override was introduced.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed: `MarkdownMessage.tsx` maps `h1`–`h4` (lines 28–35) but not `h5`/`h6`, so those heading levels fall through to default `<h5>`/`<h6>` which Tailwind preflight resets to `font-size: inherit; font-weight: inherit`, producing un-styled text inconsistent with the styled headings above.
## VERDICT: Minor issues
- **`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, un-bold, un-sized text, visually inconsistent with the styled `h1`–`h4` right above it. Nothing prevents the assistant from emitting headings past level 4. Fix: add `h5`/`h6` entries (e.g. reuse the `h4`→`h5` style or a smaller variant) so every heading level is covered.
- `MarkdownMessage.tsx:43` — inline-vs-fenced code detection via `/language-/.test(className ?? '')` is correct for react-markdown v9 (which removed the `inline` prop): fenced blocks carry `language-*`, inline code carries none. Verified against the test expectations and the component logic. No issue here.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I have enough to review. The actual source change is small and clean. Let me confirm the one thing I noticed — the duplicate `h1`/`h2` mappings.
The diff is essentially `MarkdownMessage.tsx` (new), its test, the `ChatPanel.tsx` lazy wiring, `vite.config.ts` `manualChunks`, plus the huge `package-lock.json` regen. I've verified all the source files.
Within the code-cleanliness lens:
- `MarkdownMessage.tsx:28-29` — `h1` and `h2` are mapped to literally identical elements (`<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">`). Two copy-pasted entries for the same output; if the heading scale changes, both must be edited in lockstep. A trivial dedup (e.g. a shared `heading` component or a small loop building `h1`/`h2` from one definition) would remove the duplication. This is a trivial nit, not blocking.
- Everything else is genuinely clean: the `components` map is a cohesive single-purpose styling table (the idiomatic react-markdown pattern), the lazy import + `Suspense` fallback in `ChatPanel.tsx` follows the existing `React.lazy` pattern used for routes, and the `manualChunks` change is well-commented and keeps the core bundled. No dead code, no leaky abstractions, no confusing names.
**VERDICT: No material issues found**
- `web/src/editor/MarkdownMessage.tsx:28` — `h1` and `h2` map to byte-identical `<h3>` markup; duplicate definitions that must stay in sync. Verified by reading the file. Trivial; suggest a shared heading helper. No other cleanliness problems in this change.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`web/src/editor/MarkdownMessage.tsx:65` / `web/src/editor/ChatPanel.tsx:145` — assistant markdown is re-parsed on every ChatPanel render during streaming.** `MarkdownMessage` (`web/src/editor/MarkdownMessage.tsx:65`) is a plain, unmemoized function component, and `Bubble` (`web/src/editor/ChatPanel.tsx:222`) is likewise unmemoized. During a turn, `ChatPanel` re-renders on every `onStep`: `setPending((p) => ({ …steps: [...p.steps, step] }))` (`ChatPanel.tsx:76`) and `refresh.canvas()` (`ChatPanel.tsx:80`) both fire per step. Each such re-render re-runs `messages.map(... <Bubble ...><MarkdownMessage>{body}</MarkdownMessage></Bubble>)` (`ChatPanel.tsx:145–154`), recreating the element tree for **every** prior assistant message and re-invoking `ReactMarkdown`, which re-parses its `children` string into a mdast→hast tree each render. Cost grows with both message length and conversation length (O(messages × body-size) per step event). The `lazy(...)` wrapper (`ChatPanel.tsx:21`) only code-splits; it does not memoize props. Fix: wrap `MarkdownMessage` in `React.memo` — its single `children: string` prop is stable across these re-renders (it comes from cached `history.data`), so shallow-equal memo skips re-parsing — or memoize per-body.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified the key claims. Let me confirm the asymmetric handling and the router's error boundary setup.
The code confirms:
- `ChatPanel.tsx:21-23` uses a bare `lazy(() => import('./MarkdownMessage')...)` with a pending-only `Suspense` fallback (lines 250-252) — no rejection recovery.
- `router.tsx:19-49` defines `lazyPage` with an explicit stale-chunk recovery (one-time `window.location.reload()` gated by a session flag) precisely because `React.lazy` memoizes a rejected import and a plain Suspense can't recover.
- The root route sets `errorComponent: RouteError` (`router.tsx:70`), and `RouteError.tsx` renders "Something went wrong" + a "Try again" button calling `router.invalidate()`.
The draft's claim that there is "no React error boundary above it" and it "crashes to a blank screen" is **not** confirmed and is likely false: TanStack Router's root `errorComponent` does catch component render throws (rendering `RouteError`), so it would not be a blank screen. However, the verifiable core of the finding stands: the assistant-message lazy lacks the stale-chunk recovery that `lazyPage` deliberately provides, and because `React.lazy` memoizes the rejection, `RouteError`'s "Try again" (`router.invalidate`) would re-render into the same rejected import — leaving the editor stuck on the error screen until a manual hard reload. So the issue is real but the consequence is "non-recovering error screen," not "blank screen / no error boundary."
I'll keep the finding with a corrected impact description and reduced confidence (the consequence relies on TanStack Router internals not readable in the checked-out repo).
**VERDICT: Minor issues**
- **`web/src/editor/ChatPanel.tsx:21-23, 250-252` — the assistant-message `lazy` import has no stale-chunk recovery, unlike `router.tsx`'s `lazyPage`.** `lazyPage` (router.tsx:19-49) deliberately wraps every route-level lazy import in a one-time `window.location.reload()` on first import 404, gated by a session flag, because `React.lazy` memoizes a rejected import and a plain `Suspense` only handles the pending state. The `MarkdownMessage` lazy at line 21 gets none of that: if a deploy lands while the chat panel is open and a new assistant turn renders, the chunk 404s and `React.lazy` caches the rejection. The throw is caught by the router's root error boundary (`errorComponent: RouteError`, router.tsx:70) — so it is not a blank screen — but `RouteError`'s "Try again" calls `router.invalidate()`, which re-renders into the same memoized rejection, leaving the editor route stuck on the error screen until a manual hard reload. Suggested fix: route this lazy through the same stale-chunk recovery `lazyPage` uses, or wrap the `MarkdownMessage` render in an error boundary whose fallback is the literal `body` text (which is exactly the current `Suspense` fallback), so a failed import degrades to the raw message instead of the whole-route error screen.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:11-62`** — The `components` map overrides many elements (`a`, `table`, `code`, etc.) but omits `img`. Because `react-markdown` renders `` as a native `<img>` by default, a compromised or jailbroken model could embed arbitrary external images. This enables tracking pixels (leaking user IP, UA, and potentially referrer) and unsolicited network requests. The PR’s tests explicitly verify HTML/script escaping and link safety, but do not cover images, suggesting this was overlooked.
**Suggested fix:** Add an `img` component to the `components` map — either block images entirely (return `null` or a placeholder), or restrict to a safe allow-list / proxy. Example:
```tsx
img: () => null, // or render a controlled placeholder
```
Verified by reading `MarkdownMessage.tsx` (lines 11-62) and `MarkdownMessage.test.tsx` (no image test case). No `rehype-raw` is used, raw HTML is escaped, and links carry `rel="noopener noreferrer"` — all correctly handled.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:28-35`** — The `components` object maps `h1`→`h3`, `h2`→`h3`, `h3`→`h4`, and `h4`→`h5`, but leaves `h5` and `h6` unmapped. Because Tailwind’s preflight strips default margins, font sizes, and font weights from **all** headings, any `#####` or `######` the assistant emits will render as plain unstyled text, breaking the visual consistency of the chat bubble. Add `h5` and `h6` mappings (or a generic heading handler) to cap and style the remaining levels.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
* **`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.
* **`web/src/editor/MarkdownMessage.tsx:34-41`** — All heading overrides use mismatched element names (`h1` → `<h3>`, `h2` → `<h3>`, `h3` → `<h4>`, `h4` → `<h5>`). The visual sizing is intentionally chat-scaled, but rendering an `<h3>` or `<h4>` for markdown that semantically says `h1`/`h2`/`h3` is confusing for anyone debugging the DOM or writing selectors/tests. Prefer keeping the semantic element and applying the small styling via classes (e.g. `<h1 className="... text-sm ...">`).
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
**Blocking issues found**
- **`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 Markdown parser, not just re-parsing the text. **Fix:** Hoist the array to module scope (`const remarkPlugins = [remarkGfm]`) or memoize it with `useMemo`.
- **`web/src/editor/ChatPanel.tsx:222` & `web/src/editor/MarkdownMessage.tsx:71` — All historical assistant messages re-render and re-parse on every keystroke.** `ChatPanel` updates `input` state on each `onChange`, which re-renders the whole tree. `Bubble` and `MarkdownMessage` are not memoized, so every assistant message in the thread re-parses its full Markdown body through `remark-gfm` even though the `body` strings are unchanged. **Impact:** Typing becomes sluggish as the chat history grows; a long thread with tables/code blocks causes visible jank. **Fix:** Wrap `MarkdownMessage` in `React.memo` (it has a single `string` prop) and consider memoizing `Bubble` so existing messages are skipped when the parent re-renders.
- **`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-/`).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **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 path. The route-level `lazyPage` function in `router.tsx` has `sessionStorage`-based auto-reload logic for exactly this problem, but the component-level lazy load has none of it.
**Fix:** Add `.catch()` to the `lazy()` factory that falls back to the raw-text renderer so a chunk failure degrades gracefully instead of crashing the tree:
```tsx
const MarkdownMessage = lazy(() =>
import('./MarkdownMessage')
.then((m) => ({ default: m.MarkdownMessage }))
.catch(() => ({ default: ({ children }: { children: string }) => (
<span className="whitespace-pre-wrap">{children}</span>
) })),
)
```
- **"Start over" can be clicked while a turn is still streaming** — `web/src/editor/ChatPanel.tsx:115`
The clear-history button is only gated by `clear.isPending`; it ignores the `pending` in-flight turn. Clearing server-side history mid-stream leaves the client showing the pending UI against a now-empty message list. When the stream eventually fires `onDone`/`onError`, the user is abruptly dropped into the empty-state message while the underlying turn may already have mutated the garden.
**Fix:** Disable the button when `pending` is truthy:
```tsx
disabled={clear.isPending || !!pending}
```
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
VERDICT: No material issues found
I verified the actual implementation rather than trusting the PR description's security claims:
- **`web/src/editor/MarkdownMessage.tsx:1-73`** — no `rehype-raw`, no `dangerouslySetInnerHTML`, no `urlTransform` override. react-markdown v9 applies its built-in `urlTransform` (protocol safelist) to `href`/`src` *before* custom components receive props, so the custom `a` at line 13 receives a sanitized `href`. A `javascript:` link from the model is dropped to a safe value, not rendered verbatim. Confirmed no `rehype-raw`/`dangerouslyAllowAll` anywhere in `web/src` (grep returned only the comment and the test).
- **`MarkdownMessage.tsx:13-22`** — every link gets `target="_blank"` with `rel="noopener noreferrer"`, so no reverse-tabnabbing and no opener leakage.
- **`MarkdownMessage.tsx:40-46`** — the `code` component forwards `className` only for fenced blocks (`language-…`, generated by react-markdown, not model-controlled), so no className injection vector.
- **`ChatPanel.tsx:245-253`** — user text is rendered literally (no Markdown), and the assistant path uses the same `MarkdownMessage`; no raw-HTML path exists for either role.
I could not directly inspect `node_modules/react-markdown` (not checked out in this environment), so the reliance on react-markdown v9's default `urlTransform` blocking `javascript:`/`data:text/html` is based on the documented v9 behavior rather than the installed source — but no override disables it, so the default applies.
Minor (not a defect): the test suite (`MarkdownMessage.test.tsx`) asserts that `<script>`/`<b>` are escaped and links get `rel`, but does **not** assert that a `javascript:` URL is sanitized. Adding `expect(render('[x](javascript:alert(1))')).not.toContain('javascript:')` would lock in the protocol-sanitization guarantee the lens depends on.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed. The `code` component at line 40-46 distinguishes block from inline via `/language-/.test(className ?? '')`. In react-markdown v9 (which dropped the `inline` prop), a fenced block without an info string produces a `<pre><code>` with no `language-` class, so it falls through to the inline pill styling (line 45) while still wrapped by the block `<pre>` (line 48-52) — a mismatched double-style. The test file confirms no coverage of the no-language fenced case.
**VERDICT: Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:43`** — The `code` component distinguishes fenced blocks from inline code by testing `className` for `language-`. This works for fenced blocks *with* a language tag (```` ```go ```` → `className="language-go"`), but a fenced block *without* an info string (a bare ```` ``` ```` fence, which the assistant is quite likely to emit) produces a `<pre><code>` with **no** `language-` class. In react-markdown v9 that `className` is `undefined`, so the `if (/language-/.test(className ?? ''))` branch is skipped and the block falls through to the **inline** pill style (`rounded bg-border/60 px-1 py-0.5`). The content is still wrapped by the outer `<pre>` (line 48), so the result is an inline-styled `<code>` nested inside the block-styled `<pre>` — double background, inline font sizing on a block — rather than the intended clean block rendering. Verified by reading the component and confirming no `inline` prop is used (react-markdown v9 dropped it) and no test covers the no-language fenced case (`MarkdownMessage.test.tsx` only checks tables/lists/links/HTML-escaping). Suggested fix: detect blocks via position/`node` instead of the `language-` class, e.g. compare the `code`'s parent — or simplest, treat `pre > code` as a block by checking that the parent is a `pre` (react-markdown passes `node`); or accept a falsy className as block when the element is non-inline. A practical one-liner: render the pill only when `!className` is false AND it's inline — i.e. invert to "block when `className` is set OR `node.position` spans multiple lines."
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
The code is clean and well-structured. Let me verify a couple of remaining points about the `code` component pattern and heading coverage.
VERDICT: No material issues found.
Through the code-cleanliness lens, this change is well-built:
- `MarkdownMessage.tsx` is single-purpose, the top comment explains *why* (no `rehype-raw`, GFM tables, Tailwind reset strips defaults) rather than restating *what* — and the comment stays accurate to the code.
- The `components` map reuses the exact Tailwind tokens the rest of `web/src/editor/**` uses (`text-muted`, `border-border`, `text-accent-strong`, `bg-border/40`), so it follows the surrounding pattern rather than inventing a parallel vocabulary. Verified by grepping the codebase.
- The lazy wrapper in `ChatPanel.tsx:21-23` with a `whitespace-pre-wrap` raw-text fallback (`ChatPanel.tsx:250`) is the right shape — no blank flash, and the comment at `ChatPanel.tsx:248-249` says so. The `manualChunks` change in `vite.config.ts:43-53` is well-commented and the kept-together react/react-dom/scheduler rule is explained inline, matching the CLAUDE.md note about not splitting react-dom.
- No duplicated styling logic, no dead code, no copy-paste that should be shared. `MarkdownMessage` is the single consumer of `react-markdown`/`remark-gfm` (verified by grep — only `MarkdownMessage.tsx` imports them).
One trivial observation (not worth a finding): `h1` and `h2` both render as `<h3>` with identical classes (`MarkdownMessage.tsx:28-29`), and `h5`/`h6` are unmapped so they'd fall through unstyled. The comment's phrasing — "every element the assistant *actually uses*" — is deliberately scoped so this isn't a false claim, and an LLM chat reply producing a 5th-level heading is not a real scenario. Nothing to fix.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All three JSON findings verified against the actual code:
1. `MarkdownMessage` (line 65) is a plain `export function` — no `React.memo`. Prop is `children: string`. ✓
2. `Bubble` (line 222) is a plain `function` — no `React.memo`. Used in `messages.map` at line 145. ✓
3. `/language-/.test(...)` regex literal at line 43 sits inside the `code` component (in the module-scope `components` object), evaluated per `code` element render; hoistable. ✓
Supporting claims also verified: `maxRetainedTurns = 20` at `internal/service/agent.go:15`; `setPending` per step at `ChatPanel.tsx:76`; `scrollIntoView({behavior:'smooth'})` at `ChatPanel.tsx:57` on `[history.data, pending]`; lazy import at line 21; `Suspense` per bubble at line 250.
**VERDICT: Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:65`** — `MarkdownMessage` is a plain function component (no `React.memo`), and its only prop is `children: string`. Since `ChatPanel` holds `input` (updated per keystroke) and `pending` (updated per streamed step) as state, every keystroke or step re-renders `ChatPanel` and — because the child `Bubble`s aren't memoized either — re-renders every assistant `Bubble`, each of which re-runs `ReactMarkdown`'s full remark→remark-gfm→hast→JSX pipeline on its body. With `maxRetainedTurns = 20` (`internal/service/agent.go:15`), that's up to 20 full Markdown re-parses per keystroke while typing a follow-up. Bounded (≤20 messages, modest bodies), so not catastrophic, but it's avoidable re-work that scales with conversation length × keystrokes. Fix: wrap `MarkdownMessage` in `React.memo` (default shallow equality on `children: string` suffices) and/or memoize `Bubble`.
- **`web/src/editor/ChatPanel.tsx:145` / `222`** — `Bubble` is also a plain function (no `React.memo`), so even if `MarkdownMessage` were memoized, the unmemoized `Bubble` would re-render for all up-to-20 messages on every `input`/`pending` change and defeat the memo. Extracting `Bubble` as `memo` (its props are `role`, `body`, `children` — the `children` slot is the `UndoButton`, which is stable per message) closes the gap.
- **`web/src/editor/MarkdownMessage.tsx:43`** — the `/language-/` regex literal inside the `code` component is re-evaluated on each `code` element render; hoisting it to a module-level constant (`const LANG = /^language-/`) removes both the per-render evaluation and the closure capture. Minor on its own, and compounded by the missing memo above.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I'll verify the draft findings against the actual code.
Both files confirmed. Now let me verify the two findings: (1) the absence of error boundaries in the tree, and (2) the `code` component's inline/fenced classification logic.
Both findings verified. No error boundary exists anywhere in `web/src` (grep returned nothing), and the `code` component's logic at line 43 is exactly as the draft describes. Let me confirm the react-markdown behavior for fenced blocks without a language tag by checking the installed version.
Both findings are confirmed against the actual code:
1. **ChatPanel.tsx:250** — Confirmed: `MarkdownMessage` is lazy-loaded via `import()` inside a `Suspense` at line 250, with only a loading fallback. A grep across all of `web/src` for `componentDidCatch`, `getDerivedStateFromError`, and `ErrorBoundary` returns zero matches — no error boundary exists anywhere in the tree. A failed `import()` rejects the promise and propagates uncaught, crashing the `ChatPanel`.
2. **MarkdownMessage.tsx:43** — Confirmed: the `code` component distinguishes inline from fenced purely via `/language-/.test(className ?? '')`. A fenced block without a language tag produces `className` of `language-` in react-markdown v9 — but `/language-/` matches the bare prefix `language-` too, so actually... let me re-examine.
Looking again: the regex `/language-/.test("language-")` returns `true` (the substring `language-` is present). So a fenced block with `className="language-"` would be treated as fenced, not inline. The draft's claim that it returns `false` for `""` is correct (empty string), but the bare `language-` case actually matches the regex. The draft's specific concern ("` ``` ` with nothing after yields `language-`... returns false") is **incorrect** for the bare-prefix case — it would return true. The misclassification only happens if `className` is `undefined`/empty, which react-markdown v9 does not produce for fenced blocks (it always emits at least `language-`). This finding is based on a faulty premise and I'll drop it.
The first draft bullet (about `children: string` and nil-guard) was self-dismissed by the draft itself ("no swallowing", "Low risk... flagging only because the lens asks") and not carried into the `gadfly-findings` JSON, so it doesn't survive. The second finding (error boundary) is confirmed. The third finding (fenced code without language) is based on a faulty premise — `/language-/.test("language-")` returns `true`, so the bare-prefix case is correctly classified as fenced; the misclassification only occurs if `className` is truly empty/undefined, which react-markdown v9 doesn't produce for fenced blocks.
**VERDICT: Minor issues found**
- **`web/src/editor/ChatPanel.tsx:250-252`** — A failed lazy `import()` for the `MarkdownMessage` chunk (network blip, ad blocker, bad deploy) will reject and, with no error boundary anywhere in the tree (verified: no `ErrorBoundary`/`componentDidCatch`/`getDerivedStateFromError` in `web/src`), will crash the *entire* `ChatPanel` rather than just the message bubble. Every assistant bubble in the thread would take the panel down — the user can't even see the history or send a new message. The fallback string renders fine *while* loading, but it does nothing for the failure case, which `Suspense` doesn't catch. Suggested fix: either wrap the lazy boundary in a small error boundary that falls back to the same `whitespace-pre-wrap` raw text, or drop the chunk-split and import `MarkdownMessage` statically (155 KB is modest, and the assistant tab only renders when `capabilities.data?.agent` is true anyway). Verified the absence of any error boundary via grep across `web/src`.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
Wait — actually, through the Security lens there are issues. Let me re-verify by checking if there are other elements I might have missed, and confirm my analysis.
**Blocking issues found**
- **`web/src/editor/MarkdownMessage.tsx:13-22`** — `javascript:` / `data:` links not filtered.
The custom `a` component renders `href` directly without protocol validation. `react-markdown` without `rehype-raw` only prevents raw HTML injection; it does **not** strip dangerous URL schemes from Markdown links like `[click](javascript:alert(1))` or `[exfil](data:text/html,<script>…)`. An attacker (or a compromised/misbehaving LLM) can inject a `javascript:` or `data:` URL that executes in the user's browser when clicked. The repo already has `safeExternalUrl` in `web/src/lib/seedLots.ts`; reuse it here (drop the link if the URL is unsafe).
*Verified by reading `MarkdownMessage.tsx`, `seedLots.ts`, and `SourceLink.tsx`.*
- **`web/src/editor/MarkdownMessage.tsx:11-62`** — Missing `img` component override allows malicious image URLs.
The `components` object does not include `img`, so standard Markdown images (``) fall through to the default renderer with an unvalidated `src`. A malicious `src` (e.g., `javascript:`, tracking pixel, or data URI) bypasses the application's otherwise careful link handling. Add an `img` override that validates the `src` (allow only `http:`/`https:`) or drops the image.
*Verified by reading `MarkdownMessage.tsx`.*
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:40-47`** — Fenced code blocks without a language tag (e.g. plain ` ``` `) are incorrectly styled as inline pills. `react-markdown` v9 passes `className: undefined` to the `code` component when no language is specified, so the `/language-/.test(className ?? '')` guard fails and the block gets the inline `rounded bg-border/60 px-1 py-0.5` treatment even though it is inside the `<pre>` container. Verified by reading the dependency source (`mdast-util-to-hast` 13.2.1) and the component logic.
**Fix:** Use the presence of the `<pre>` wrapper as the block/inline discriminator, or move the inline pill style to a wrapper and leave the `<code>` element plain for both cases.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/MarkdownMessage.tsx:68`** — `remarkPlugins={[remarkGfm]}` creates a new array on every render. `react-markdown` memoizes the unified processor by plugin identity; a fresh array defeats that `useMemo` and rebuilds the entire GFM pipeline on every render of every assistant bubble. During streaming this happens across all existing assistant messages each time a new step arrives.
**Fix:** Move `const remarkPlugins = [remarkGfm]` outside the component so the reference is stable.
- **`web/src/editor/MarkdownMessage.tsx:65`** — `MarkdownMessage` is not wrapped in `React.memo`. When `ChatPanel` re-renders during streaming (as `pending.steps` grows), every existing assistant bubble re-renders and re-parses its markdown even though the `body` string is unchanged.
**Fix:** Export `const MarkdownMessage = memo(function MarkdownMessage({ children }: { children: string }) { ... })` so unchanged messages bail out of the re-parse.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`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:** Add a small class-based Error Boundary around the `Suspense` + `MarkdownMessage` pair, falling back to the raw text (`<span className=\"whitespace-pre-wrap\">{body}</span>`) on error — or adopt the same stale-chunk reload strategy used in `router.tsx`'s `lazyPage`.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Address the Gadfly review of #118:
- Forbid <img> (`disallowedElements`): a prompt-injected reply could embed
``, which the browser auto-fetches — a data
exfiltration beacon. The assistant has no reason to emit images. Tested.
- Give the lazy Markdown chunk the same stale-chunk recovery the routes get:
extract `lazyPage` to its own module and use it in ChatPanel, so a post-deploy
404 doesn't permanently break the assistant. `lazyPage` now preserves the
component's prop type instead of erasing to `{}`.
- Wrap the renderer in an error boundary that falls back to the raw text, so a
chunk that can't load (or a renderer that throws) degrades to readable text
rather than taking the editor down.
- Memoize MarkdownMessage and hoist its plugin/regex constants so typing in the
composer doesn't re-parse every message in the thread.
- Correctness: detect fenced code with no info-string (newline check, not just
the language- class); pass `start` through on ordered lists; pass GFM column
alignment (`style`) through on th/td instead of hardcoding left; de-dup the
heading renderers and style all six levels.
- Disable "Start over" while a turn is in flight — clearing mid-turn raced the
in-flight stream.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit 08d8c5e47d into main2026-07-22 16:42:12 +00:00
steve
deleted branch feat/assistant-markdown2026-07-22 16:42:13 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The assistant answers in Markdown — tables, lists, bold, links — but the chat bubble rendered the raw source (
whitespace-pre-wrap), so a table came through as a wall of pipes.What
MarkdownMessage—react-markdown+remark-gfm(for GFM tables). It does not enable raw HTML (norehype-raw), so a model reply can't inject markup; every element the assistant actually uses is Tailwind-styled for a chat bubble, and wide content (tables, code) scrolls in its ownoverflow-x-autobox so the bubble can't blow out.*shouldn't become a bullet), and the assistant bubble goes full-width so a table has room.manualChunksnow vendors only the app-wide core (react/react-dom/scheduler kept together — splitting react-dom breaks React 19 at load) and lets everything else ride with its importer. TheMarkdownMessagechunk is 155 KB; the editor chunk stays ~44 KB.Tested
renderToStaticMarkup(node, no DOM) verifies: a GFM table renders with styled cells + horizontal scroll;**bold**/ lists render; raw<script>/<b>are escaped, not emitted (XSS-safe); links getrel="noopener noreferrer". Re-verified the app mounts cleanly against the embedded binary after the re-chunk (no React-19 init-order break), console clean.tsc+vitest(99) + build green, no>500 KBwarning.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 16:30:39Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -18,1 +18,4 @@// 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(() =>🔴 Lazy MarkdownMessage chunk failure crashes ChatPanel with no ErrorBoundary or fallback
error-handling · flagged by 3 models
MarkdownMessagechunk failures —web/src/editor/ChatPanel.tsx:21Ifimport('./MarkdownMessage')fails (stale chunk hash after a deployment, network dropout, CDN 404), thelazy()Promise rejects.Suspenseonly catches the loading throw; a rejection propagates as a render error. With noErrorBoundaryanywhere in the component tree, React will unmount theChatPanel(or the wholeAppShell) and the user gets a broken chat with no recovery pa…🪰 Gadfly · advisory
@@ -235,0 +245,4 @@{mine ? (body) : (// Show the raw text until the renderer chunk arrives, so a message never🔴 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-loadedMarkdownMessageis wrapped inSuspensebut has no Error Boundary. If the chunk fails to load (network drop, stale deploy) orreact-markdownthrows during render (library bug, malformed markdown edge case), the rejected promise / thrown error bubbles past theSuspenseboundary and crashes the route or the entire app. The route-levelRouteErroronly catches loader/beforeLoad errors, not component-render errors. Fix: A…🪰 Gadfly · advisory
@@ -0,0 +8,4 @@// 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 = {🟠 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 customcomponentsmap overridesp,a,ul,code,table, etc., but never overrides (or disables)img. Confirmed by reading the full file: noimgkey exists in the object spanning lines 11–62.react-markdown's defaultimgrenderer emits a real<img src="…">, which the browser fetches unconditionally, with no click required — unlike theatag here, which needs a user click and getsrel="noopener noreferrer"(confirmed a…🪰 Gadfly · advisory
@@ -0,0 +21,4 @@</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>,🟡 Custom
olrenderer drops thestartprop, so ordered lists not beginning at 1 always render numbered from 1correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +25,4 @@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>,🟡 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—h5andh6are unmapped, so they lose all styling. Thecomponentsmap overridesh1–h4(collapsing them toh3/h4/h5withfont-semibold, sizing, etc.), buth5andh6are not mapped at all. Tailwind's preflight resets headings tofont-size: inherit; font-weight: inherit, so if the assistant emits##### Headingor###### Heading, react-markdown renders a bare<h5>/<h6>with no custom component → it shows up as plain, u…🪰 Gadfly · advisory
@@ -0,0 +31,4 @@h4: ({ children }) => (<h5 className="mb-1 mt-1.5 text-xs font-semibold uppercase tracking-wide text-muted first:mt-0">{children}</h5>🟡 h1 and h2 render identical JSX - duplicated code should be shared
maintainability · flagged by 1 model
web/src/editor/MarkdownMessage.tsx:34-35—h1andh2map 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
@@ -0,0 +37,4 @@<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 }) => {🟠 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. Thecodecomponent 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
@@ -0,0 +55,4 @@<table className="w-full border-collapse text-xs">{children}</table></div>),th: ({ children }) => (🟡 Custom
th/tdrenderers hardcodetext-leftand drop thestyleprop, discarding GFM column alignment (:---:/---:)correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +62,4 @@}/** Render one assistant message body as Markdown (GFM). */export function MarkdownMessage({ children }: { children: string }) {🔴 remarkPlugins array recreated every render, breaking ReactMarkdown processor memoization
performance · flagged by 4 models
web/src/editor/MarkdownMessage.tsx:68—remarkPluginsarray recreated on every render, invalidating processor memoization. The[remarkGfm]literal is a new array reference each render.ReactMarkdownmemoizes itsunifiedprocessor by theremarkPluginsprop; 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
🪰 Gadfly review — consensus across 5 models
Verdict: Blocking issues found · 12 findings (7 with multi-model agreement)
web/src/editor/MarkdownMessage.tsx:65web/src/editor/MarkdownMessage.tsx:40web/src/editor/ChatPanel.tsx:21web/src/editor/MarkdownMessage.tsx:28web/src/editor/ChatPanel.tsx:248web/src/editor/ChatPanel.tsx:145web/src/editor/MarkdownMessage.tsx:115 single-model findings (lower confidence)
web/src/editor/ChatPanel.tsx:222web/src/editor/ChatPanel.tsx:115web/src/editor/MarkdownMessage.tsx:24web/src/editor/MarkdownMessage.tsx:34web/src/editor/MarkdownMessage.tsx:58Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:11-62— The customcomponentsmap overridesp,a,ul,code,table, etc., but never overrides (or disables)img. Confirmed by reading the full file: noimgkey exists in the object spanning lines 11–62.react-markdown's defaultimgrenderer emits a real<img src="…">, which the browser fetches unconditionally, with no click required — unlike theatag here, which needs a user click and getsrel="noopener noreferrer"(confirmed at lines 13-22). Since the assistant's reply is LLM-generated text, a prompt-injection payload embedded in stored garden/plant data could cause the model to emitand have it silently loaded as soon as the message renders.react-markdown's defaulturlTransformblocks dangerous URI schemes (javascript:, etc.) but does nothing to restrict the host, and there is no CSP anywhere in the repo (grep -rn "Content-Security-Policy" internal/returns nothing) and no server-side sanitization that would strip image markdown before it's rendered. Fix: add animgentry tocomponentsthat renders nothing / a text placeholder, or proxies the URL, so arbitrary attacker-controlled hosts aren't auto-fetched from the browser.Links (
MarkdownMessage.tsx:13-22) and inline/raw HTML are handled correctly:rehype-rawis not imported, so raw<script>/<b>are escaped — confirmed both by the source comment and by the existing test (MarkdownMessage.test.tsx:31-35,'does not emit raw HTML from the model (no rehype-raw)'); anchors gettarget="_blank"+rel="noopener noreferrer"(test at line 40 confirms), so the residual risk is host-level (the image issue above), not scheme-level.Everything else in this PR is clean:
BubbleinChatPanel.tsx:222-258gates Markdown rendering torole !== 'user'only (the user's own text renders as literalbody, not throughMarkdownMessage), and the lockfile changes are just the newreact-markdown/remark-gfmdependency tree with no unusual packages.🎯 Correctness — Minor issues
Review — 🎯 Correctness
VERDICT: Minor issues
The new
MarkdownMessage.tsx(web/src/editor/MarkdownMessage.tsx) styles most GFM elements correctly, but the custom renderers forcode,ol,th/tdonly destructure{ children }(or{ children, className }) and drop other hast-derived props thatreact-markdown/mdast-util-to-hastactually populate. Verified against the exact pinned versions inweb/package-lock.json([email protected],[email protected],[email protected]) and the current source ofMarkdownMessage.tsx:MarkdownMessage.tsx:40-47— Thecodecomponent decides "fenced vs. inline" purely by checking for alanguage-className, and the surrounding comment asserts "a fenced block carries a language- class." That's false for a plain```fence with no info string:mdast-util-to-hast's code handler only setsproperties.classNamewhennode.langis truthy, so an un-languaged fenced block still gets wrapped in<pre>(correct) but its inner<code>falls into the inline branch and gets the pill styling (rounded bg-border/60 px-1 py-0.5 …) nested inside the block's own box styling. This is worsened by react-markdown v9 (confirmed pinned) no longer providing aninlineprop to thecoderenderer at all — thelanguage-className check is the only signal available, and it fails for unlabeled fences.MarkdownMessage.tsx:24—olrenders with a hardcodedlist-decimaland drops thestartprop thatmdast-util-to-hast's list handler sets on the hast node whenevernode.start !== 1(confirmed:properties.startis only added when the parsed list's starting number isn't 1). Any ordered list in an assistant reply that doesn't begin at 1 (e.g. continuing a numbered plan, "6. …", "7. …") will always render starting at "1." — the displayed numbers no longer match the source.MarkdownMessage.tsx:58-61—th/tdhardcodetext-leftand don't forward thestyleprop. GFM column alignment (:---:,---:) is expressed bymdast-util-to-hast's table-cell handler as an inlinestyle="text-align:…"on the th/td hast nodes (confirmed handler behavior, andstyle-to-js— present in the lockfile diff as a dependency ofhast-util-to-jsx-runtime— exists specifically to turn that inline style string into a React style prop), which is discarded here — any table where the assistant right/center-aligns a column (e.g. a numeric "Qty" column) silently renders left-aligned regardless of source.No existing test in
MarkdownMessage.test.tsxcovers an unlabeled fenced block, a non-1-starting ordered list, or a column-aligned table, so nothing in the test suite catches these.🧹 Code cleanliness & maintainability — Minor issues
package.json/lockfile consistency also confirmed. The single finding is verified against the actual code.
VERDICT: Minor issues
h1andh2component overrides are identical duplicated JSX (<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">{children}</h3>); extract to one shared renderer used by both keys to avoid future drift.⚡ Performance — Minor issues
Verification
I read
ChatPanel.tsxandMarkdownMessage.tsxin full. Confirmed:Bubble(ChatPanel.tsx:222-258) norMarkdownMessage(MarkdownMessage.tsx:65-73) is wrapped inReact.memo— nomemoimport exists in either file.ChatPanelre-renders onsetInput(line 39/188) and onsetPendinginsideonStep(line 76), both of which are plainuseStatesetters with no scoping/isolation.messages.map((m) => (<Bubble .../>))at line 145 constructs freshBubbleelements every render, and sinceBubbleisn't memoized, React re-invokes it (and theMarkdownMessage/ReactMarkdowntree inside it) for every past message on every unrelated state change — this is standard React re-render semantics, not library-specific behavior.MarkdownMessage'scomponentsobject (line 11) is indeed module-level, confirmed clean.vite.config.ts:43-53manualChunksconfirmed: it only pinsreact/react-dom/scheduler/@tanstack/zustand/zod/clsx/tailwind-mergeto thevendorchunk and leaves everything else (includingreact-markdown) to ride with its importer, matching the draft's "isn't itself a hot-path concern" note.node_modulesisn't installed in this checkout, so I couldn't directly inspectreact-markdown's internals, but its lack of per-render memoization onchildrenis well-established, documented behavior for the library (nouseMemo/cache wrappingprocessor.runSync), consistent with the draft's claim. Combined with the confirmed absence ofReact.memoin this codebase's own components, the performance claim holds.VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:145—BubbleandMarkdownMessagearen't memoized, so every unrelated state change inChatPanel(typing a keystroke viasetInput, lines 39/188; or each streamed agent step viaonStep→setPending, line 76) re-renders the wholemessages.maplist and re-invokesMarkdownMessagefor every assistant message already in the conversation, not just the one being added. Previously this path waswhitespace-pre-wrapon a plain string (essentially free); now each keystroke and each SSE step during a streaming turn (web/src/lib/agent.ts:217) triggers a full markdown re-parse of every past assistant message — an O(messages) re-parse per tick instead of O(1), during exactly the interaction where the UI is supposed to stay responsive.Bubble(or at minimum theMarkdownMessageusage) inReact.memokeyed onbody/roleso unrelatedpending/inputstate changes don't re-run the markdown pipeline for messages whose content hasn't changed.components(MarkdownMessage.tsx:11) is a module-level static object, not reallocated per render; the lazy-load/chunking split (ChatPanel.tsx:21-23, vite.config.ts:43-53) correctly keeps the markdown renderer out of the eager bundle and isn't itself a hot-path concern.Outside my lens: none noted.
🧯 Error handling & edge cases — Minor issues
All claims check out against the actual source. The finding is fully confirmed:
ChatPanel.tsxuses plainlazy()whilerouter.tsxhas a purpose-builtlazyPage()for exactly this stale-chunk scenario,RouteErroris only defined on the root route,gardenEditorRoutehas no ownerrorComponent, andChatPanelis rendered directly insideGardenEditorPagewith no intermediate error boundary — so the propagation chain and blast radius described in the draft are accurate.Review — 🧯 Error handling & edge cases
VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:21-23—MarkdownMessageis loaded with a barelazy(() => import('./MarkdownMessage').then(...)), but this codebase already haslazyPage()(web/src/router.tsx:26-49) built specifically to handle stale-chunk 404s after a deploy:React.lazymemoizes a rejected import promise forever, so once a chunk 404s, every future attempt to render that lazy component throws the same cached error — including after the route error boundary's "Try again" (router.invalidate()), since invalidating the route loader doesn't reset the lazy component's cached rejection.rootRoutedefineserrorComponent: RouteError(web/src/router.tsx:70);gardenEditorRoute(web/src/router.tsx:156) has no ownerrorComponent, andChatPanelis rendered directly insideGardenEditorPage(web/src/pages/GardenEditorPage.tsx:535) with no intervening boundary. A throw from the failedMarkdownMessageimport therefore propagates to the root boundary and replaces the entire routed outlet content (AppShell.tsx:105-107) — canvas, inspector, everything in the editor — withRouteError, while the nav chrome persists.mainredeploys mid-session; the next assistant reply triggersimport('./MarkdownMessage')against a chunk hash the server no longer serves. The editor goes to the error screen with no self-recovery via "Try again".lazyPage()(or inline the same one-time-reload recovery) forMarkdownMessageinstead of rawlazy().glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
MarkdownMessage.tsxusesreact-markdownwith onlyremarkGfmand norehype-raw. By default react-markdown does not pass through raw HTML; the test atMarkdownMessage.test.tsx:31confirms<script>/<b>are escaped, not emitted. Norehype-rawappears anywhere inweb/src.MarkdownMessage.tsx:13-22overridesawithtarget="_blank"andrel="noopener noreferrer". react-markdown's defaulturlTransformsanitizes dangerous schemes (javascript:,data:, etc.) onhref, and no customurlTransformis passed, so the safe default remains in effect. The test atMarkdownMessage.test.tsx:37confirms the link renders with the expectedrel.ChatPanel.tsx:245-253: only the assistant branch renders<MarkdownMessage>; the user branch rendersbodyas plain text withwhitespace-pre-wrap, so a user's own*cannot become a bullet / markup.img/urlTransform/rehype-rawreferences were found inweb/srcoutside the test and the comment, confirming no permissive raw-HTML or asset-loading override was introduced.🎯 Correctness — Minor issues
The finding is confirmed:
MarkdownMessage.tsxmapsh1–h4(lines 28–35) but noth5/h6, so those heading levels fall through to default<h5>/<h6>which Tailwind preflight resets tofont-size: inherit; font-weight: inherit, producing un-styled text inconsistent with the styled headings above.VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:28-35—h5andh6are unmapped, so they lose all styling. Thecomponentsmap overridesh1–h4(collapsing them toh3/h4/h5withfont-semibold, sizing, etc.), buth5andh6are not mapped at all. Tailwind's preflight resets headings tofont-size: inherit; font-weight: inherit, so if the assistant emits##### Headingor###### Heading, react-markdown renders a bare<h5>/<h6>with no custom component → it shows up as plain, un-bold, un-sized text, visually inconsistent with the styledh1–h4right above it. Nothing prevents the assistant from emitting headings past level 4. Fix: addh5/h6entries (e.g. reuse theh4→h5style or a smaller variant) so every heading level is covered.MarkdownMessage.tsx:43— inline-vs-fenced code detection via/language-/.test(className ?? '')is correct for react-markdown v9 (which removed theinlineprop): fenced blocks carrylanguage-*, inline code carries none. Verified against the test expectations and the component logic. No issue here.🧹 Code cleanliness & maintainability — No material issues found
I have enough to review. The actual source change is small and clean. Let me confirm the one thing I noticed — the duplicate
h1/h2mappings.The diff is essentially
MarkdownMessage.tsx(new), its test, theChatPanel.tsxlazy wiring,vite.config.tsmanualChunks, plus the hugepackage-lock.jsonregen. I've verified all the source files.Within the code-cleanliness lens:
MarkdownMessage.tsx:28-29—h1andh2are mapped to literally identical elements (<h3 className="mb-1 mt-2 text-sm font-semibold first:mt-0">). Two copy-pasted entries for the same output; if the heading scale changes, both must be edited in lockstep. A trivial dedup (e.g. a sharedheadingcomponent or a small loop buildingh1/h2from one definition) would remove the duplication. This is a trivial nit, not blocking.componentsmap is a cohesive single-purpose styling table (the idiomatic react-markdown pattern), the lazy import +Suspensefallback inChatPanel.tsxfollows the existingReact.lazypattern used for routes, and themanualChunkschange is well-commented and keeps the core bundled. No dead code, no leaky abstractions, no confusing names.VERDICT: No material issues found
web/src/editor/MarkdownMessage.tsx:28—h1andh2map to byte-identical<h3>markup; duplicate definitions that must stay in sync. Verified by reading the file. Trivial; suggest a shared heading helper. No other cleanliness problems in this change.⚡ Performance — Minor issues
VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:65/web/src/editor/ChatPanel.tsx:145— assistant markdown is re-parsed on every ChatPanel render during streaming.MarkdownMessage(web/src/editor/MarkdownMessage.tsx:65) is a plain, unmemoized function component, andBubble(web/src/editor/ChatPanel.tsx:222) is likewise unmemoized. During a turn,ChatPanelre-renders on everyonStep:setPending((p) => ({ …steps: [...p.steps, step] }))(ChatPanel.tsx:76) andrefresh.canvas()(ChatPanel.tsx:80) both fire per step. Each such re-render re-runsmessages.map(... <Bubble ...><MarkdownMessage>{body}</MarkdownMessage></Bubble>)(ChatPanel.tsx:145–154), recreating the element tree for every prior assistant message and re-invokingReactMarkdown, which re-parses itschildrenstring into a mdast→hast tree each render. Cost grows with both message length and conversation length (O(messages × body-size) per step event). Thelazy(...)wrapper (ChatPanel.tsx:21) only code-splits; it does not memoize props. Fix: wrapMarkdownMessageinReact.memo— its singlechildren: stringprop is stable across these re-renders (it comes from cachedhistory.data), so shallow-equal memo skips re-parsing — or memoize per-body.🧯 Error handling & edge cases — Minor issues
I've verified the key claims. Let me confirm the asymmetric handling and the router's error boundary setup.
The code confirms:
ChatPanel.tsx:21-23uses a barelazy(() => import('./MarkdownMessage')...)with a pending-onlySuspensefallback (lines 250-252) — no rejection recovery.router.tsx:19-49defineslazyPagewith an explicit stale-chunk recovery (one-timewindow.location.reload()gated by a session flag) precisely becauseReact.lazymemoizes a rejected import and a plain Suspense can't recover.errorComponent: RouteError(router.tsx:70), andRouteError.tsxrenders "Something went wrong" + a "Try again" button callingrouter.invalidate().The draft's claim that there is "no React error boundary above it" and it "crashes to a blank screen" is not confirmed and is likely false: TanStack Router's root
errorComponentdoes catch component render throws (renderingRouteError), so it would not be a blank screen. However, the verifiable core of the finding stands: the assistant-message lazy lacks the stale-chunk recovery thatlazyPagedeliberately provides, and becauseReact.lazymemoizes the rejection,RouteError's "Try again" (router.invalidate) would re-render into the same rejected import — leaving the editor stuck on the error screen until a manual hard reload. So the issue is real but the consequence is "non-recovering error screen," not "blank screen / no error boundary."I'll keep the finding with a corrected impact description and reduced confidence (the consequence relies on TanStack Router internals not readable in the checked-out repo).
VERDICT: Minor issues
web/src/editor/ChatPanel.tsx:21-23, 250-252— the assistant-messagelazyimport has no stale-chunk recovery, unlikerouter.tsx'slazyPage.lazyPage(router.tsx:19-49) deliberately wraps every route-level lazy import in a one-timewindow.location.reload()on first import 404, gated by a session flag, becauseReact.lazymemoizes a rejected import and a plainSuspenseonly handles the pending state. TheMarkdownMessagelazy at line 21 gets none of that: if a deploy lands while the chat panel is open and a new assistant turn renders, the chunk 404s andReact.lazycaches the rejection. The throw is caught by the router's root error boundary (errorComponent: RouteError, router.tsx:70) — so it is not a blank screen — butRouteError's "Try again" callsrouter.invalidate(), which re-renders into the same memoized rejection, leaving the editor route stuck on the error screen until a manual hard reload. Suggested fix: route this lazy through the same stale-chunk recoverylazyPageuses, or wrap theMarkdownMessagerender in an error boundary whose fallback is the literalbodytext (which is exactly the currentSuspensefallback), so a failed import degrades to the raw message instead of the whole-route error screen.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:11-62— Thecomponentsmap overrides many elements (a,table,code, etc.) but omitsimg. Becausereact-markdownrendersas a native<img>by default, a compromised or jailbroken model could embed arbitrary external images. This enables tracking pixels (leaking user IP, UA, and potentially referrer) and unsolicited network requests. The PR’s tests explicitly verify HTML/script escaping and link safety, but do not cover images, suggesting this was overlooked.Suggested fix: Add an
imgcomponent to thecomponentsmap — either block images entirely (returnnullor a placeholder), or restrict to a safe allow-list / proxy. Example:Verified by reading
MarkdownMessage.tsx(lines 11-62) andMarkdownMessage.test.tsx(no image test case). Norehype-rawis used, raw HTML is escaped, and links carryrel="noopener noreferrer"— all correctly handled.🎯 Correctness — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:28-35— Thecomponentsobject mapsh1→h3,h2→h3,h3→h4, andh4→h5, but leavesh5andh6unmapped. Because Tailwind’s preflight strips default margins, font sizes, and font weights from all headings, any#####or######the assistant emits will render as plain unstyled text, breaking the visual consistency of the chat bubble. Addh5andh6mappings (or a generic heading handler) to cap and style the remaining levels.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:34-35—h1andh2map 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.web/src/editor/MarkdownMessage.tsx:34-41— All heading overrides use mismatched element names (h1→<h3>,h2→<h3>,h3→<h4>,h4→<h5>). The visual sizing is intentionally chat-scaled, but rendering an<h3>or<h4>for markdown that semantically saysh1/h2/h3is confusing for anyone debugging the DOM or writing selectors/tests. Prefer keeping the semantic element and applying the small styling via classes (e.g.<h1 className="... text-sm ...">).⚡ Performance — Blocking issues found
Blocking issues found
web/src/editor/MarkdownMessage.tsx:68—remarkPluginsarray recreated on every render, invalidating processor memoization. The[remarkGfm]literal is a new array reference each render.ReactMarkdownmemoizes itsunifiedprocessor by theremarkPluginsprop; 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 Markdown parser, not just re-parsing the text. Fix: Hoist the array to module scope (const remarkPlugins = [remarkGfm]) or memoize it withuseMemo.web/src/editor/ChatPanel.tsx:222&web/src/editor/MarkdownMessage.tsx:71— All historical assistant messages re-render and re-parse on every keystroke.ChatPanelupdatesinputstate on eachonChange, which re-renders the whole tree.BubbleandMarkdownMessageare not memoized, so every assistant message in the thread re-parses its full Markdown body throughremark-gfmeven though thebodystrings are unchanged. Impact: Typing becomes sluggish as the chat history grows; a long thread with tables/code blocks causes visible jank. Fix: WrapMarkdownMessageinReact.memo(it has a singlestringprop) and consider memoizingBubbleso existing messages are skipped when the parent re-renders.web/src/editor/MarkdownMessage.tsx:43— Regex recompiled on every<code>element. Thecodecomponent 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-/).🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
No error handling for lazy-loaded
MarkdownMessagechunk failures —web/src/editor/ChatPanel.tsx:21If
import('./MarkdownMessage')fails (stale chunk hash after a deployment, network dropout, CDN 404), thelazy()Promise rejects.Suspenseonly catches the loading throw; a rejection propagates as a render error. With noErrorBoundaryanywhere in the component tree, React will unmount theChatPanel(or the wholeAppShell) and the user gets a broken chat with no recovery path. The route-levellazyPagefunction inrouter.tsxhassessionStorage-based auto-reload logic for exactly this problem, but the component-level lazy load has none of it.Fix: Add
.catch()to thelazy()factory that falls back to the raw-text renderer so a chunk failure degrades gracefully instead of crashing the tree:"Start over" can be clicked while a turn is still streaming —
web/src/editor/ChatPanel.tsx:115The clear-history button is only gated by
clear.isPending; it ignores thependingin-flight turn. Clearing server-side history mid-stream leaves the client showing the pending UI against a now-empty message list. When the stream eventually firesonDone/onError, the user is abruptly dropped into the empty-state message while the underlying turn may already have mutated the garden.Fix: Disable the button when
pendingis truthy:opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I verified the actual implementation rather than trusting the PR description's security claims:
web/src/editor/MarkdownMessage.tsx:1-73— norehype-raw, nodangerouslySetInnerHTML, nourlTransformoverride. react-markdown v9 applies its built-inurlTransform(protocol safelist) tohref/srcbefore custom components receive props, so the customaat line 13 receives a sanitizedhref. Ajavascript:link from the model is dropped to a safe value, not rendered verbatim. Confirmed norehype-raw/dangerouslyAllowAllanywhere inweb/src(grep returned only the comment and the test).MarkdownMessage.tsx:13-22— every link getstarget="_blank"withrel="noopener noreferrer", so no reverse-tabnabbing and no opener leakage.MarkdownMessage.tsx:40-46— thecodecomponent forwardsclassNameonly for fenced blocks (language-…, generated by react-markdown, not model-controlled), so no className injection vector.ChatPanel.tsx:245-253— user text is rendered literally (no Markdown), and the assistant path uses the sameMarkdownMessage; no raw-HTML path exists for either role.I could not directly inspect
node_modules/react-markdown(not checked out in this environment), so the reliance on react-markdown v9's defaulturlTransformblockingjavascript:/data:text/htmlis based on the documented v9 behavior rather than the installed source — but no override disables it, so the default applies.Minor (not a defect): the test suite (
MarkdownMessage.test.tsx) asserts that<script>/<b>are escaped and links getrel, but does not assert that ajavascript:URL is sanitized. Addingexpect(render('[x](javascript:alert(1))')).not.toContain('javascript:')would lock in the protocol-sanitization guarantee the lens depends on.🎯 Correctness — Minor issues
The finding is confirmed. The
codecomponent at line 40-46 distinguishes block from inline via/language-/.test(className ?? ''). In react-markdown v9 (which dropped theinlineprop), a fenced block without an info string produces a<pre><code>with nolanguage-class, so it falls through to the inline pill styling (line 45) while still wrapped by the block<pre>(line 48-52) — a mismatched double-style. The test file confirms no coverage of the no-language fenced case.VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:43— Thecodecomponent distinguishes fenced blocks from inline code by testingclassNameforlanguage-. This works for fenced blocks with a language tag (```go→className="language-go"), but a fenced block without an info string (a bare```fence, which the assistant is quite likely to emit) produces a<pre><code>with nolanguage-class. In react-markdown v9 thatclassNameisundefined, so theif (/language-/.test(className ?? ''))branch is skipped and the block falls through to the inline pill style (rounded bg-border/60 px-1 py-0.5). The content is still wrapped by the outer<pre>(line 48), so the result is an inline-styled<code>nested inside the block-styled<pre>— double background, inline font sizing on a block — rather than the intended clean block rendering. Verified by reading the component and confirming noinlineprop is used (react-markdown v9 dropped it) and no test covers the no-language fenced case (MarkdownMessage.test.tsxonly checks tables/lists/links/HTML-escaping). Suggested fix: detect blocks via position/nodeinstead of thelanguage-class, e.g. compare thecode's parent — or simplest, treatpre > codeas a block by checking that the parent is apre(react-markdown passesnode); or accept a falsy className as block when the element is non-inline. A practical one-liner: render the pill only when!classNameis false AND it's inline — i.e. invert to "block whenclassNameis set ORnode.positionspans multiple lines."🧹 Code cleanliness & maintainability — No material issues found
The code is clean and well-structured. Let me verify a couple of remaining points about the
codecomponent pattern and heading coverage.VERDICT: No material issues found.
Through the code-cleanliness lens, this change is well-built:
MarkdownMessage.tsxis single-purpose, the top comment explains why (norehype-raw, GFM tables, Tailwind reset strips defaults) rather than restating what — and the comment stays accurate to the code.componentsmap reuses the exact Tailwind tokens the rest ofweb/src/editor/**uses (text-muted,border-border,text-accent-strong,bg-border/40), so it follows the surrounding pattern rather than inventing a parallel vocabulary. Verified by grepping the codebase.ChatPanel.tsx:21-23with awhitespace-pre-wrapraw-text fallback (ChatPanel.tsx:250) is the right shape — no blank flash, and the comment atChatPanel.tsx:248-249says so. ThemanualChunkschange invite.config.ts:43-53is well-commented and the kept-together react/react-dom/scheduler rule is explained inline, matching the CLAUDE.md note about not splitting react-dom.MarkdownMessageis the single consumer ofreact-markdown/remark-gfm(verified by grep — onlyMarkdownMessage.tsximports them).One trivial observation (not worth a finding):
h1andh2both render as<h3>with identical classes (MarkdownMessage.tsx:28-29), andh5/h6are unmapped so they'd fall through unstyled. The comment's phrasing — "every element the assistant actually uses" — is deliberately scoped so this isn't a false claim, and an LLM chat reply producing a 5th-level heading is not a real scenario. Nothing to fix.⚡ Performance — Minor issues
All three JSON findings verified against the actual code:
MarkdownMessage(line 65) is a plainexport function— noReact.memo. Prop ischildren: string. ✓Bubble(line 222) is a plainfunction— noReact.memo. Used inmessages.mapat line 145. ✓/language-/.test(...)regex literal at line 43 sits inside thecodecomponent (in the module-scopecomponentsobject), evaluated percodeelement render; hoistable. ✓Supporting claims also verified:
maxRetainedTurns = 20atinternal/service/agent.go:15;setPendingper step atChatPanel.tsx:76;scrollIntoView({behavior:'smooth'})atChatPanel.tsx:57on[history.data, pending]; lazy import at line 21;Suspenseper bubble at line 250.VERDICT: Minor issues
web/src/editor/MarkdownMessage.tsx:65—MarkdownMessageis a plain function component (noReact.memo), and its only prop ischildren: string. SinceChatPanelholdsinput(updated per keystroke) andpending(updated per streamed step) as state, every keystroke or step re-rendersChatPaneland — because the childBubbles aren't memoized either — re-renders every assistantBubble, each of which re-runsReactMarkdown's full remark→remark-gfm→hast→JSX pipeline on its body. WithmaxRetainedTurns = 20(internal/service/agent.go:15), that's up to 20 full Markdown re-parses per keystroke while typing a follow-up. Bounded (≤20 messages, modest bodies), so not catastrophic, but it's avoidable re-work that scales with conversation length × keystrokes. Fix: wrapMarkdownMessageinReact.memo(default shallow equality onchildren: stringsuffices) and/or memoizeBubble.web/src/editor/ChatPanel.tsx:145/222—Bubbleis also a plain function (noReact.memo), so even ifMarkdownMessagewere memoized, the unmemoizedBubblewould re-render for all up-to-20 messages on everyinput/pendingchange and defeat the memo. ExtractingBubbleasmemo(its props arerole,body,children— thechildrenslot is theUndoButton, which is stable per message) closes the gap.web/src/editor/MarkdownMessage.tsx:43— the/language-/regex literal inside thecodecomponent is re-evaluated on eachcodeelement render; hoisting it to a module-level constant (const LANG = /^language-/) removes both the per-render evaluation and the closure capture. Minor on its own, and compounded by the missing memo above.🧯 Error handling & edge cases — Minor issues
I'll verify the draft findings against the actual code.
Both files confirmed. Now let me verify the two findings: (1) the absence of error boundaries in the tree, and (2) the
codecomponent's inline/fenced classification logic.Both findings verified. No error boundary exists anywhere in
web/src(grep returned nothing), and thecodecomponent's logic at line 43 is exactly as the draft describes. Let me confirm the react-markdown behavior for fenced blocks without a language tag by checking the installed version.Both findings are confirmed against the actual code:
ChatPanel.tsx:250 — Confirmed:
MarkdownMessageis lazy-loaded viaimport()inside aSuspenseat line 250, with only a loading fallback. A grep across all ofweb/srcforcomponentDidCatch,getDerivedStateFromError, andErrorBoundaryreturns zero matches — no error boundary exists anywhere in the tree. A failedimport()rejects the promise and propagates uncaught, crashing theChatPanel.MarkdownMessage.tsx:43 — Confirmed: the
codecomponent distinguishes inline from fenced purely via/language-/.test(className ?? ''). A fenced block without a language tag producesclassNameoflanguage-in react-markdown v9 — but/language-/matches the bare prefixlanguage-too, so actually... let me re-examine.Looking again: the regex
/language-/.test("language-")returnstrue(the substringlanguage-is present). So a fenced block withclassName="language-"would be treated as fenced, not inline. The draft's claim that it returnsfalsefor""is correct (empty string), but the barelanguage-case actually matches the regex. The draft's specific concern ("```with nothing after yieldslanguage-... returns false") is incorrect for the bare-prefix case — it would return true. The misclassification only happens ifclassNameisundefined/empty, which react-markdown v9 does not produce for fenced blocks (it always emits at leastlanguage-). This finding is based on a faulty premise and I'll drop it.The first draft bullet (about
children: stringand nil-guard) was self-dismissed by the draft itself ("no swallowing", "Low risk... flagging only because the lens asks") and not carried into thegadfly-findingsJSON, so it doesn't survive. The second finding (error boundary) is confirmed. The third finding (fenced code without language) is based on a faulty premise —/language-/.test("language-")returnstrue, so the bare-prefix case is correctly classified as fenced; the misclassification only occurs ifclassNameis truly empty/undefined, which react-markdown v9 doesn't produce for fenced blocks.VERDICT: Minor issues found
web/src/editor/ChatPanel.tsx:250-252— A failed lazyimport()for theMarkdownMessagechunk (network blip, ad blocker, bad deploy) will reject and, with no error boundary anywhere in the tree (verified: noErrorBoundary/componentDidCatch/getDerivedStateFromErrorinweb/src), will crash the entireChatPanelrather than just the message bubble. Every assistant bubble in the thread would take the panel down — the user can't even see the history or send a new message. The fallback string renders fine while loading, but it does nothing for the failure case, whichSuspensedoesn't catch. Suggested fix: either wrap the lazy boundary in a small error boundary that falls back to the samewhitespace-pre-wrapraw text, or drop the chunk-split and importMarkdownMessagestatically (155 KB is modest, and the assistant tab only renders whencapabilities.data?.agentis true anyway). Verified the absence of any error boundary via grep acrossweb/src.opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Wait — actually, through the Security lens there are issues. Let me re-verify by checking if there are other elements I might have missed, and confirm my analysis.
Blocking issues found
web/src/editor/MarkdownMessage.tsx:13-22—javascript:/data:links not filtered.The custom
acomponent rendershrefdirectly without protocol validation.react-markdownwithoutrehype-rawonly prevents raw HTML injection; it does not strip dangerous URL schemes from Markdown links like[click](javascript:alert(1))or[exfil](data:text/html,<script>…). An attacker (or a compromised/misbehaving LLM) can inject ajavascript:ordata:URL that executes in the user's browser when clicked. The repo already hassafeExternalUrlinweb/src/lib/seedLots.ts; reuse it here (drop the link if the URL is unsafe).Verified by reading
MarkdownMessage.tsx,seedLots.ts, andSourceLink.tsx.web/src/editor/MarkdownMessage.tsx:11-62— Missingimgcomponent override allows malicious image URLs.The
componentsobject does not includeimg, so standard Markdown images () fall through to the default renderer with an unvalidatedsrc. A malicioussrc(e.g.,javascript:, tracking pixel, or data URI) bypasses the application's otherwise careful link handling. Add animgoverride that validates thesrc(allow onlyhttp:/https:) or drops the image.Verified by reading
MarkdownMessage.tsx.🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/editor/MarkdownMessage.tsx:40-47— Fenced code blocks without a language tag (e.g. plain```) are incorrectly styled as inline pills.react-markdownv9 passesclassName: undefinedto thecodecomponent when no language is specified, so the/language-/.test(className ?? '')guard fails and the block gets the inlinerounded bg-border/60 px-1 py-0.5treatment even though it is inside the<pre>container. Verified by reading the dependency source (mdast-util-to-hast13.2.1) and the component logic.Fix: Use the presence of the
<pre>wrapper as the block/inline discriminator, or move the inline pill style to a wrapper and leave the<code>element plain for both cases.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — Minor issues
Minor issues
web/src/editor/MarkdownMessage.tsx:68—remarkPlugins={[remarkGfm]}creates a new array on every render.react-markdownmemoizes the unified processor by plugin identity; a fresh array defeats thatuseMemoand rebuilds the entire GFM pipeline on every render of every assistant bubble. During streaming this happens across all existing assistant messages each time a new step arrives.Fix: Move
const remarkPlugins = [remarkGfm]outside the component so the reference is stable.web/src/editor/MarkdownMessage.tsx:65—MarkdownMessageis not wrapped inReact.memo. WhenChatPanelre-renders during streaming (aspending.stepsgrows), every existing assistant bubble re-renders and re-parses its markdown even though thebodystring is unchanged.Fix: Export
const MarkdownMessage = memo(function MarkdownMessage({ children }: { children: string }) { ... })so unchanged messages bail out of the re-parse.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
web/src/editor/ChatPanel.tsx:248-252— The lazy-loadedMarkdownMessageis wrapped inSuspensebut has no Error Boundary. If the chunk fails to load (network drop, stale deploy) orreact-markdownthrows during render (library bug, malformed markdown edge case), the rejected promise / thrown error bubbles past theSuspenseboundary and crashes the route or the entire app. The route-levelRouteErroronly catches loader/beforeLoad errors, not component-render errors.Fix: Add a small class-based Error Boundary around the
Suspense+MarkdownMessagepair, falling back to the raw text (<span className=\"whitespace-pre-wrap\">{body}</span>) on error — or adopt the same stale-chunk reload strategy used inrouter.tsx'slazyPage.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.