Chat panel in the garden editor (#57) #71

Merged
steve merged 3 commits from feat/chat-panel into main 2026-07-21 06:43:22 +00:00
4 changed files with 108 additions and 34 deletions
Showing only changes of commit 77bc672c19 - Show all commits
+14 -6
View File
@@ -53,12 +53,10 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
// is set; see csrfGuard). // is set; see csrfGuard).
v1.Use(h.csrfGuard()) v1.Use(h.csrfGuard())
v1.GET("/healthz", healthz) v1.GET("/healthz", healthz)
// What this instance can actually do, so the UI offers only what works. The // What this instance can actually do, so the UI offers only what works.
// agent routes 404 when unconfigured; without this the client would have to // Registered after the agent below, because it reports whether the runner
// probe for a 404 to find that out, and a dead button is worse than no button. // actually built — not merely whether it was configured to.
v1.GET("/capabilities", func(c *gin.Context) { v1.GET("/capabilities", h.capabilities)
Outdated
Review

🟠 Capabilities endpoint reports agent ready when routes may not be registered

correctness, maintainability · flagged by 4 models

  • /capabilities can falsely advertise a working assistant (internal/api/api.go:59-61). The endpoint returns cfg.Agent.Ready(), but the actual chat routes are only registered if agent.NewRunner succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), Ready() is still true yet POST /agent/chat 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden.

🪰 Gadfly · advisory

🟠 **Capabilities endpoint reports agent ready when routes may not be registered** _correctness, maintainability · flagged by 4 models_ - **`/capabilities` can falsely advertise a working assistant** (`internal/api/api.go:59-61`). The endpoint returns `cfg.Agent.Ready()`, but the actual chat routes are only registered if `agent.NewRunner` succeeds. If the model spec is unresolvable (the exact failure case noted at line 137), `Ready()` is still true yet POST `/agent/chat` 404s. The client trusts the capability and renders the chat tab, so the user lands on a button that errors instead of being hidden. <sub>🪰 Gadfly · advisory</sub>
c.JSON(http.StatusOK, gin.H{"agent": cfg.Agent.Ready()})
})
// Auth endpoints are exempt from requireAuth (you can't be logged in yet); // Auth endpoints are exempt from requireAuth (you can't be logged in yet);
// /me is the one that needs a session. Feature routers in later issues attach // /me is the one that needs a session. Feature routers in later issues attach
@@ -197,6 +195,16 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
return r return r
} }
// capabilities reports what this instance can actually do, so the UI offers only
// what works.
//
// It reports whether the runner BUILT, not whether it was configured: a
// configured-but-unresolvable model leaves the routes unregistered, and saying
// "yes" there would offer a chat tab whose first message 404s.
func (h *handlers) capabilities(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"agent": h.agent != nil})
}
// healthz is a liveness probe: always returns {"ok": true} when the server is up. // healthz is a liveness probe: always returns {"ok": true} when the server is up.
func healthz(c *gin.Context) { func healthz(c *gin.Context) {
c.JSON(http.StatusOK, gin.H{"ok": true}) c.JSON(http.StatusOK, gin.H{"ok": true})
+33 -12
View File
@@ -1,5 +1,6 @@
import { useEffect, useRef, useState } from 'react' import { useEffect, useRef, useState, type ReactNode } from 'react'
import { Alert } from '@/components/ui/Alert' import { Alert } from '@/components/ui/Alert'
import { errorMessage } from '@/lib/api'
import { Button } from '@/components/ui/Button' import { Button } from '@/components/ui/Button'
import { TextArea } from '@/components/ui/TextArea' import { TextArea } from '@/components/ui/TextArea'
import { cn } from '@/lib/cn' import { cn } from '@/lib/cn'
1
@@ -33,12 +34,17 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
// The turn in flight: what we sent, the steps so far, and how it ended. // The turn in flight: what we sent, the steps so far, and how it ended.
const [pending, setPending] = useState<{ message: string; steps: AgentStep[] } | null>(null) const [pending, setPending] = useState<{ message: string; steps: AgentStep[] } | null>(null)
const [error, setError] = useState<string | null>(null) const [error, setError] = useState<string | null>(null)
const [warning, setWarning] = useState<string | null>(null)
const abort = useRef<AbortController | null>(null) const abort = useRef<AbortController | null>(null)
const bottom = useRef<HTMLDivElement>(null) const bottom = useRef<HTMLDivElement>(null)
// Abort an in-flight turn when the panel goes away, so a closed tab doesn't // Deliberately NOT aborted on unmount. Selecting an object auto-switches the
Outdated
Review

🔴 Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown

correctness · flagged by 1 model

  • web/src/editor/ChatPanel.tsx:41 — Unmounting ChatPanel (via useEffect(() => () => abort.current?.abort(), [])) silently aborts an in-flight agent turn. EditorRail renders only the active tab lazily (web/src/editor/EditorRail.tsx:27,85), and GardenEditorPage.tsx:150-153 unconditionally calls setRailTab('inspector') on every object/plop selection — with no guard for a pending chat turn — overriding an open 'chat' tab and unmounting ChatPanel mid-stream. Server-side, `intern…

🪰 Gadfly · advisory

🔴 **Unmounting on tab switch (including auto-switch-to-inspector on selection) silently aborts an in-flight agent turn, losing the reply/history with no error shown** _correctness · flagged by 1 model_ - **`web/src/editor/ChatPanel.tsx:41`** — Unmounting `ChatPanel` (via `useEffect(() => () => abort.current?.abort(), [])`) silently aborts an in-flight agent turn. `EditorRail` renders only the active tab lazily (`web/src/editor/EditorRail.tsx:27,85`), and `GardenEditorPage.tsx:150-153` unconditionally calls `setRailTab('inspector')` on every object/plop selection — with no guard for a pending chat turn — overriding an open `'chat'` tab and unmounting `ChatPanel` mid-stream. Server-side, `intern… <sub>🪰 Gadfly · advisory</sub>
// leave a read hanging. // rail to the inspector, so aborting here would mean clicking the canvas
useEffect(() => () => abort.current?.abort(), []) // mid-turn silently killed the turn — and the canvas is exactly what you're
// meant to be watching. The request continues, the exchange is persisted
// server-side, and coming back to this tab shows it. Only Stop aborts, and
// even that only stops us READING: the turn keeps running server-side, which
// is why its work still lands in History either way.
// Follow the conversation as it grows, including mid-turn as steps arrive. // Follow the conversation as it grows, including mid-turn as steps arrive.
useEffect(() => { useEffect(() => {
@@ -50,6 +56,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
if (!message || pending) return if (!message || pending) return
setInput('') setInput('')
setError(null) setError(null)
setWarning(null)
setPending({ message, steps: [] }) setPending({ message, steps: [] })
const controller = new AbortController() const controller = new AbortController()
@@ -61,22 +68,24 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
{ {
onStep: (step) => { onStep: (step) => {
setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p)) setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p))
// Refresh as each step lands, not just at the end: the canvas updating // The canvas updating under the conversation is the whole point of
// under the conversation is the whole point of putting the chat here. // putting the chat here, so refresh it as each step lands — but only
refresh() // it: nothing else can have changed until the turn commits.
refresh.canvas()
}, },
onDone: (turn: AgentTurn) => { onDone: (turn: AgentTurn) => {
setPending(null) setPending(null)
refresh() refresh.everything()
if (turn.truncated) { if (turn.truncated) {
setError('That turned into more steps than I should take at once — check what changed before continuing.') setError('That turned into more steps than I should take at once — check what changed before continuing.')
} }
}, },
onWarning: setWarning,
onError: (message) => { onError: (message) => {
setPending(null) setPending(null)
setError(message) setError(message)
// Something may still have landed before it failed. // Something may still have landed before it failed.
refresh() refresh.everything()
}, },
}, },
controller.signal, controller.signal,
1
@@ -92,7 +101,11 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
{messages.length > 0 && ( {messages.length > 0 && (
<button <button
type="button" type="button"
onClick={() => clear.mutate()} onClick={() =>
clear.mutate(undefined, {
onError: (err) => setError(errorMessage(err, "Couldn't clear the conversation.")),
})
}
disabled={clear.isPending} disabled={clear.isPending}
className="rounded px-1.5 py-0.5 text-xs text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40" className="rounded px-1.5 py-0.5 text-xs text-muted outline-none hover:text-fg focus-visible:ring-2 focus-visible:ring-accent/40"
> >
Review

🟠 History loading and error states silently swallowed

error-handling · flagged by 1 model

  • web/src/editor/ChatPanel.tsx:111-128 — The conversation pane only renders content when history.isSuccess is true. If the history query fails (history.isError) or is still loading (history.isLoading / history.isPending), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space. - Fix: Add explicit branches for history.isPending and history.isError inside the scrol…

🪰 Gadfly · advisory

🟠 **History loading and error states silently swallowed** _error-handling · flagged by 1 model_ - **`web/src/editor/ChatPanel.tsx:111-128`** — The conversation pane only renders content when `history.isSuccess` is true. If the history query fails (`history.isError`) or is still loading (`history.isLoading` / `history.isPending`), the scroll area is simply empty—no loading spinner, no error alert, no hint that the previous conversation couldn't be retrieved. The user is left staring at blank space. - *Fix:* Add explicit branches for `history.isPending` and `history.isError` inside the scrol… <sub>🪰 Gadfly · advisory</sub>
@@ -108,6 +121,13 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
)} )}
<div className="flex min-h-0 flex-1 flex-col gap-2 overflow-y-auto"> <div className="flex min-h-0 flex-1 flex-col gap-2 overflow-y-auto">
{history.isPending && <p className="text-sm text-muted">Loading the conversation…</p>}
{/* A failed load rendering as an empty thread would look like the
conversation had been lost, which is a much worse thing to believe. */}
{history.isError && (
<Alert>{errorMessage(history.error, "Couldn't load the conversation.")}</Alert>
)}
{history.isSuccess && messages.length === 0 && !pending && ( {history.isSuccess && messages.length === 0 && !pending && (
<p className="text-sm text-muted"> <p className="text-sm text-muted">
Ask for what you want and it'll do it — “change the garlic bed to cucumbers this year”, “fill the Ask for what you want and it'll do it — “change the garlic bed to cucumbers this year”, “fill the
@@ -146,6 +166,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
</> </>
)} )}
{warning && <Alert tone="info">{warning}</Alert>}
{error && <Alert>{error}</Alert>} {error && <Alert>{error}</Alert>}
<div ref={bottom} /> <div ref={bottom} />
</div> </div>
@@ -177,7 +198,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo
onClick={() => { onClick={() => {
abort.current?.abort() abort.current?.abort()
setPending(null) setPending(null)
refresh() refresh.everything()
}} }}
Review

🟡 Uses React.ReactNode without importing React, unlike every other component's import type { ReactNode } convention

maintainability · flagged by 2 models

  • web/src/editor/ChatPanel.tsx:202children?: React.ReactNode uses the React namespace without importing it anywhere in the file (only { useEffect, useRef, useState } is imported from 'react'). Every other component in the codebase types children as ReactNode via import type { ReactNode } from 'react'. Match that convention.

🪰 Gadfly · advisory

🟡 **Uses React.ReactNode without importing React, unlike every other component's `import type { ReactNode }` convention** _maintainability · flagged by 2 models_ - `web/src/editor/ChatPanel.tsx:202` — `children?: React.ReactNode` uses the `React` namespace without importing it anywhere in the file (only `{ useEffect, useRef, useState }` is imported from `'react'`). Every other component in the codebase types children as `ReactNode` via `import type { ReactNode } from 'react'`. Match that convention. <sub>🪰 Gadfly · advisory</sub>
> >
Stop Stop
@@ -199,7 +220,7 @@ function Bubble({
}: { }: {
role: 'user' | 'assistant' role: 'user' | 'assistant'
body: string body: string
children?: React.ReactNode children?: ReactNode
}) { }) {
const mine = role === 'user' const mine = role === 'user'
return ( return (
+56 -12
View File
@@ -97,10 +97,29 @@ export function describeStep(step: AgentStep): string {
return [...new Set(labels)].join(', ') return [...new Set(labels)].join(', ')
} }
const chatEventSchema = z.object({
step: z.object({ index: z.number(), tools: z.array(z.string()) }).optional(),
done: z
.object({
reply: z.string(),
changeSetId: z.number().optional(),
steps: z.number(),
truncated: z.boolean().optional(),
})
.optional(),
error: z.string().optional(),
// The turn worked but something adjacent to it didn't — currently, the
// exchange couldn't be saved. Dropping this on the floor would recreate
// exactly the silent swallow the server added it to avoid.
warning: z.string().optional(),
})
export interface StreamHandlers { export interface StreamHandlers {
onStep: (step: AgentStep) => void onStep: (step: AgentStep) => void
onDone: (turn: AgentTurn) => void onDone: (turn: AgentTurn) => void
onError: (message: string) => void onError: (message: string) => void
/** The turn succeeded, but something alongside it didn't. */
onWarning?: (message: string) => void
} }
/** /**
1
@@ -126,6 +145,10 @@ export async function streamChat(
signal, signal,
}) })
} catch { } catch {
// An abort here is the caller's own doing — Stop, or navigating away — not a
// failure to report back to them. The read loop below already knew this; the
// request path did not.
if (signal?.aborted) return
handlers.onError('Could not reach the server.') handlers.onError('Could not reach the server.')
return return
} }
2
@@ -162,13 +185,12 @@ export async function streamChat(
for (const frame of frames) { for (const frame of frames) {
const line = frame.split('\n').find((l) => l.startsWith('data:')) const line = frame.split('\n').find((l) => l.startsWith('data:'))
if (!line) continue if (!line) continue
let event: unknown // Parsed AND validated: a malformed or unexpected frame shouldn't kill a
try { // working stream, and shouldn't be trusted into the UI either.
event = JSON.parse(line.slice(5).trim()) const parsed = chatEventSchema.safeParse(safeJson(line.slice(5).trim()))
} catch { if (!parsed.success) continue
continue // a malformed frame shouldn't kill a working stream const e = parsed.data
} if (e.warning) handlers.onWarning?.(e.warning)
const e = event as { step?: AgentStep; done?: AgentTurn; error?: string }
if (e.error) handlers.onError(e.error) if (e.error) handlers.onError(e.error)
else if (e.step) handlers.onStep(e.step) else if (e.step) handlers.onStep(e.step)
else if (e.done) handlers.onDone(e.done) else if (e.done) handlers.onDone(e.done)
@@ -176,13 +198,35 @@ export async function streamChat(
} }
} }
/** Refresh everything a turn may have changed: the canvas, the history list, function safeJson(raw: string): unknown {
* and the conversation itself. */ try {
return JSON.parse(raw)
} catch {
return null
}
}
/**
* Refreshes for the two moments that need different amounts of work.
*
* Mid-turn, only the canvas can have changed: the change set isn't written until
* the turn commits, and the exchange isn't stored until it finishes. Refetching
* those on every step would be up to 2×(N1) requests per turn for data that
* cannot have moved.
*/
export function useAgentRefresh(gardenId: number) { export function useAgentRefresh(gardenId: number) {
const qc = useQueryClient() const qc = useQueryClient()
return () => { const canvas = () => {
void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) }) void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })
void qc.invalidateQueries({ queryKey: historyKey(gardenId) }) }
void qc.invalidateQueries({ queryKey: agentHistoryKey(gardenId) }) return {
/** After a step: the garden may have changed under the conversation. */
canvas,
/** After a turn: the change set and the stored exchange exist now too. */
everything: () => {
canvas()
void qc.invalidateQueries({ queryKey: historyKey(gardenId) })
void qc.invalidateQueries({ queryKey: agentHistoryKey(gardenId) })
},
} }
} }
+5 -4
View File
@@ -113,9 +113,10 @@ export function useRevertChangeSet(gardenId: number) {
}) })
} }
/** How many rows a change set touched, for "3 changes" in the list. */ /** How many rows a change set touched, for "3 changes" in the list. Undefined
export function totalChanges(cs: ChangeSet): number { * counts total zero, which callers read as "no denominator to quote". */
return cs.counts.reduce((sum, c) => sum + c.n, 0) export function totalChanges(cs: { counts?: ChangeCount[] }): number {
return (cs.counts ?? []).reduce((sum, c) => sum + c.n, 0)
} }
const ENTITY_NOUNS: Record<ChangeCount['entityType'], [string, string]> = { const ENTITY_NOUNS: Record<ChangeCount['entityType'], [string, string]> = {
@@ -236,7 +237,7 @@ export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutc
} }
// Only claim a denominator when we have one. "2 of 3" from a caller that // Only claim a denominator when we have one. "2 of 3" from a caller that
// never knew the total would be a number invented to fill a sentence. // never knew the total would be a number invented to fill a sentence.
Review

🟡 describeUndo duplicates totalChanges' summation logic instead of sharing a helper

maintainability · flagged by 4 models

  • web/src/lib/history.ts:239describeUndo reimplements the exact summation target.counts.reduce((sum, c) => sum + c.n, 0) that totalChanges (line 117-119) already does for ChangeSet. Since UndoTarget.counts and ChangeSet.counts are both ChangeCount[], factor the reduce into a small shared helper (e.g. sumCounts(counts?: ChangeCount[])) and have both call it.

🪰 Gadfly · advisory

🟡 **describeUndo duplicates totalChanges' summation logic instead of sharing a helper** _maintainability · flagged by 4 models_ - `web/src/lib/history.ts:239` — `describeUndo` reimplements the exact summation `target.counts.reduce((sum, c) => sum + c.n, 0)` that `totalChanges` (line 117-119) already does for `ChangeSet`. Since `UndoTarget.counts` and `ChangeSet.counts` are both `ChangeCount[]`, factor the reduce into a small shared helper (e.g. `sumCounts(counts?: ChangeCount[])`) and have both call it. <sub>🪰 Gadfly · advisory</sub>
const total = target.counts ? target.counts.reduce((sum, c) => sum + c.n, 0) : 0 const total = totalChanges(target)
const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone' const scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
return { tone: 'partial', message: `${scale}${skipped}.` } return { tone: 'partial', message: `${scale}${skipped}.` }
} }