Chat panel in the garden editor (#57) #71
@@ -69,6 +69,7 @@ GET,POST /gardens/:id/journal PATCH,DELETE /journal/:id (editor writes
|
||||
GET /gardens/:id/journal/counts ← entries per object, for the "has notes" indicator
|
||||
POST /agent/chat ← SSE: step events, then the finished turn (editor only)
|
||||
GET,DELETE /gardens/:id/agent/history (the actor's own thread)
|
||||
GET /capabilities ← what this instance can do, so the UI offers only what works
|
||||
GET,POST /gardens/:id/shares PATCH,DELETE /gardens/:id/shares/:userId (invite by email)
|
||||
```
|
||||
|
||||
@@ -118,7 +119,7 @@ React 19 + TypeScript + Vite + Tailwind 4 (`@tailwindcss/vite`), `@tanstack/reac
|
||||
- **Routes:** `/login`, `/register`, `/gardens` (list), `/gardens/:id` (editor, `?focus=objectId`), `/plants` (catalog). Auth guard on the router root via `/auth/me`.
|
||||
- **State:** TanStack Query for all server state (editor keyed on `gardens/:id/full`; optimistic mutations with version-conflict rollback). One small Zustand store for ephemeral editor state only: viewport, selection, focused object, active tool, in-flight drag.
|
||||
- **Editor components (`web/src/editor/`):** `GardenCanvas` (svg root + viewport g), `useViewport` (use-gesture pan/zoom/pinch), `ObjectShape`, `PlopMarker` (semantic-zoom branching), `Palette` (drag-to-place object kinds), `EditorRail` (the one side panel), `Inspector`, `HistoryPanel`, `PlantPicker`.
|
||||
- **One rail, tabs inside it.** The inspector, history, journal, and later the chat panel all want the same strip of screen; rather than each bolting on its own chrome they are tabs in `EditorRail` — so the canvas is one width instead of a different width per panel, and adding a panel is adding a tab. Selecting an object switches to the Inspector tab automatically, so the rail is never something you operate before you can edit; on a phone the same tabs render in the bottom sheet the inspector already used. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.
|
||||
- **One rail, tabs inside it.** The inspector, history, journal and assistant all want the same strip of screen; rather than each bolting on its own chrome they are tabs in `EditorRail` — so the canvas is one width instead of a different width per panel, and adding a panel is adding a tab. Selecting an object switches to the Inspector tab automatically, so the rail is never something you operate before you can edit; on a phone the same tabs render in the bottom sheet the inspector already used. Pure geometry helpers (local↔world transforms, unit formatting) in `web/src/lib/geometry.ts`, unit-tested.
|
||||
|
||||
## Roadmap
|
||||
|
||||
|
||||
@@ -53,6 +53,12 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine {
|
||||
// is set; see csrfGuard).
|
||||
v1.Use(h.csrfGuard())
|
||||
v1.GET("/healthz", healthz)
|
||||
// What this instance can actually do, so the UI offers only what works. The
|
||||
// agent routes 404 when unconfigured; without this the client would have to
|
||||
// probe for a 404 to find that out, and a dead button is worse than no button.
|
||||
v1.GET("/capabilities", func(c *gin.Context) {
|
||||
c.JSON(http.StatusOK, gin.H{"agent": cfg.Agent.Ready()})
|
||||
})
|
||||
|
||||
// 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
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
import { useEffect, useRef, useState } from 'react'
|
||||
import { Alert } from '@/components/ui/Alert'
|
||||
import { Button } from '@/components/ui/Button'
|
||||
import { TextArea } from '@/components/ui/TextArea'
|
||||
import { cn } from '@/lib/cn'
|
||||
import {
|
||||
describeStep,
|
||||
streamChat,
|
||||
useAgentHistory,
|
||||
useAgentRefresh,
|
||||
useClearAgentHistory,
|
||||
type AgentStep,
|
||||
type AgentTurn,
|
||||
} from '@/lib/agent'
|
||||
import { useUndo } from '@/lib/history'
|
||||
import { UndoButton } from './UndoButton'
|
||||
|
||||
/**
|
||||
* Talk to the garden assistant, in the editor beside the canvas.
|
||||
*
|
||||
* Here rather than on its own page because watching the garden change as the
|
||||
* agent works IS the confirmation — which is what makes acting without asking
|
||||
* first tolerable. It also means the agent never has to guess which garden you
|
||||
* mean.
|
||||
*/
|
||||
export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: boolean }) {
|
||||
const history = useAgentHistory(gardenId, true)
|
||||
|
|
||||
const clear = useClearAgentHistory(gardenId)
|
||||
const refresh = useAgentRefresh(gardenId)
|
||||
const undo = useUndo(gardenId)
|
||||
|
||||
const [input, setInput] = useState('')
|
||||
// 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 [error, setError] = useState<string | null>(null)
|
||||
const abort = useRef<AbortController | null>(null)
|
||||
const bottom = useRef<HTMLDivElement>(null)
|
||||
|
||||
// Abort an in-flight turn when the panel goes away, so a closed tab doesn't
|
||||
// leave a read hanging.
|
||||
useEffect(() => () => abort.current?.abort(), [])
|
||||
|
||||
// Follow the conversation as it grows, including mid-turn as steps arrive.
|
||||
useEffect(() => {
|
||||
bottom.current?.scrollIntoView({ behavior: 'smooth', block: 'end' })
|
||||
}, [history.data, pending])
|
||||
|
||||
const send = () => {
|
||||
const message = input.trim()
|
||||
if (!message || pending) return
|
||||
setInput('')
|
||||
setError(null)
|
||||
setPending({ message, steps: [] })
|
||||
|
||||
const controller = new AbortController()
|
||||
abort.current = controller
|
||||
|
||||
void streamChat(
|
||||
gardenId,
|
||||
message,
|
||||
{
|
||||
onStep: (step) => {
|
||||
setPending((p) => (p ? { ...p, steps: [...p.steps, step] } : p))
|
||||
// Refresh as each step lands, not just at the end: the canvas updating
|
||||
// under the conversation is the whole point of putting the chat here.
|
||||
refresh()
|
||||
},
|
||||
onDone: (turn: AgentTurn) => {
|
||||
setPending(null)
|
||||
refresh()
|
||||
if (turn.truncated) {
|
||||
setError('That turned into more steps than I should take at once — check what changed before continuing.')
|
||||
}
|
||||
},
|
||||
onError: (message) => {
|
||||
setPending(null)
|
||||
setError(message)
|
||||
// Something may still have landed before it failed.
|
||||
refresh()
|
||||
},
|
||||
},
|
||||
controller.signal,
|
||||
)
|
||||
}
|
||||
|
||||
const messages = history.data ?? []
|
||||
|
||||
return (
|
||||
<div className="flex h-full flex-col gap-3">
|
||||
<div className="flex items-center justify-between gap-2">
|
||||
<h2 className="text-sm font-semibold text-fg">Assistant</h2>
|
||||
{messages.length > 0 && (
|
||||
<button
|
||||
|
gitea-actions
commented
🟡 Clear history mutation errors silently ignored error-handling · flagged by 2 models
🪰 Gadfly · advisory 🟡 **Clear history mutation errors silently ignored**
_error-handling · flagged by 2 models_
- `web/src/editor/ChatPanel.tsx:93-100` — \"Start over\" errors are silent. `useClearAgentHistory` has no `onError` handler, and the component never reads `clear.isError`. If the DELETE fails, the button simply exits its pending state with no feedback. **Fix:** check `clear.isError` and render an alert, or at least disable the button and show the error text.
<sub>🪰 Gadfly · advisory</sub>
|
||||
type="button"
|
||||
onClick={() => clear.mutate()}
|
||||
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"
|
||||
>
|
||||
Start over
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{!canEdit && (
|
||||
<p className="rounded-md bg-border/40 px-2 py-1 text-xs text-muted">
|
||||
You can only view this garden, so the assistant can't change anything in it.
|
||||
</p>
|
||||
)}
|
||||
|
||||
<div className="flex min-h-0 flex-1 flex-col gap-2 overflow-y-auto">
|
||||
{history.isSuccess && messages.length === 0 && !pending && (
|
||||
|
gitea-actions
commented
🟠 History loading and error states silently swallowed error-handling · flagged by 1 model
🪰 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>
|
||||
<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
|
||||
north half of the west bed with beans”, “what's in here?”. Everything it does lands as one change you
|
||||
can undo.
|
||||
</p>
|
||||
)}
|
||||
|
||||
{messages.map((m) => (
|
||||
<Bubble key={m.id} role={m.role} body={m.body}>
|
||||
{/* Undo on the turn itself, so the common case never involves
|
||||
opening the History panel. Same hook #49 uses, not a second
|
||||
implementation. */}
|
||||
{m.role === 'assistant' && m.changeSetId != null && canEdit && (
|
||||
<UndoButton changeSet={{ id: m.changeSetId }} undo={undo} className="mt-1 items-start" />
|
||||
)}
|
||||
</Bubble>
|
||||
))}
|
||||
|
||||
{pending && (
|
||||
<>
|
||||
<Bubble role="user" body={pending.message} />
|
||||
<div className="rounded-lg border border-border px-2.5 py-2 text-sm">
|
||||
<ol className="flex flex-col gap-0.5">
|
||||
{pending.steps.map((s) => (
|
||||
<li key={s.index} className="text-xs text-muted">
|
||||
{describeStep(s)}…
|
||||
</li>
|
||||
))}
|
||||
</ol>
|
||||
<p className="mt-1 flex items-center gap-1.5 text-xs text-muted">
|
||||
<span className="inline-block h-1.5 w-1.5 animate-pulse rounded-full bg-accent" />
|
||||
{pending.steps.length === 0 ? 'Thinking…' : 'Working…'}
|
||||
</p>
|
||||
</div>
|
||||
</>
|
||||
)}
|
||||
|
||||
{error && <Alert>{error}</Alert>}
|
||||
<div ref={bottom} />
|
||||
</div>
|
||||
|
||||
<div className="flex flex-col gap-2 border-t border-border pt-2">
|
||||
<TextArea
|
||||
label="Message"
|
||||
name="agentMessage"
|
||||
rows={2}
|
||||
placeholder="Change the garlic bed to cucumbers this year"
|
||||
value={input}
|
||||
disabled={!!pending}
|
||||
onChange={(e) => setInput(e.target.value)}
|
||||
onKeyDown={(e) => {
|
||||
// Enter sends, Shift+Enter breaks the line — the convention every
|
||||
// chat box uses, and typing a newline by accident mid-thought is a
|
||||
// worse failure than the reverse.
|
||||
if (e.key === 'Enter' && !e.shiftKey) {
|
||||
e.preventDefault()
|
||||
send()
|
||||
}
|
||||
}}
|
||||
/>
|
||||
<div className="flex justify-end gap-2">
|
||||
{pending && (
|
||||
<Button
|
||||
variant="ghost"
|
||||
className="px-2 py-1 text-xs"
|
||||
onClick={() => {
|
||||
abort.current?.abort()
|
||||
setPending(null)
|
||||
refresh()
|
||||
}}
|
||||
>
|
||||
Stop
|
||||
</Button>
|
||||
)}
|
||||
<Button className="px-3 py-1.5 text-sm" disabled={!!pending || input.trim() === ''} onClick={send}>
|
||||
{pending ? 'Working…' : 'Send'}
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
|
||||
function Bubble({
|
||||
role,
|
||||
body,
|
||||
children,
|
||||
}: {
|
||||
role: 'user' | 'assistant'
|
||||
body: string
|
||||
children?: React.ReactNode
|
||||
|
gitea-actions
commented
🟡 Uses React.ReactNode without importing React, unlike every other component's maintainability · flagged by 2 models
🪰 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>
|
||||
}) {
|
||||
const mine = role === 'user'
|
||||
return (
|
||||
<div className={cn('flex flex-col', mine ? 'items-end' : 'items-start')}>
|
||||
<div
|
||||
className={cn(
|
||||
'max-w-[90%] whitespace-pre-wrap rounded-lg px-2.5 py-2 text-sm',
|
||||
mine ? 'bg-accent/15 text-fg' : 'border border-border text-fg',
|
||||
)}
|
||||
>
|
||||
{body}
|
||||
</div>
|
||||
{children}
|
||||
</div>
|
||||
)
|
||||
}
|
||||
@@ -1,6 +1,6 @@
|
||||
import { Button } from '@/components/ui/Button'
|
||||
import { cn } from '@/lib/cn'
|
||||
import type { ChangeSet, UndoOutcome, useUndo } from '@/lib/history'
|
||||
import type { UndoOutcome, UndoTarget, useUndo } from '@/lib/history'
|
||||
|
||||
/**
|
||||
* Undo, plus what happened. Paired with useUndo so the history list and the
|
||||
@@ -14,7 +14,7 @@ export function UndoButton({
|
||||
className,
|
||||
label = 'Undo',
|
||||
}: {
|
||||
changeSet: ChangeSet
|
||||
changeSet: UndoTarget
|
||||
undo: ReturnType<typeof useUndo>
|
||||
className?: string
|
||||
label?: string
|
||||
|
||||
@@ -0,0 +1,29 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { describeStep } from './agent'
|
||||
|
||||
describe('describeStep', () => {
|
||||
// Raw tool names tell you the agent is busy; these tell you what it's busy
|
||||
// DOING, which is the difference between the panel feeling alive and hung.
|
||||
it("names tools in the app's own vocabulary", () => {
|
||||
expect(describeStep({ index: 0, tools: ['clear_object'] })).toBe('Clearing a bed')
|
||||
expect(describeStep({ index: 1, tools: ['find_plant'] })).toBe('Looking up a plant')
|
||||
})
|
||||
|
||||
it('collapses repeats within one step', () => {
|
||||
expect(describeStep({ index: 0, tools: ['fill_region', 'fill_region'] })).toBe('Filling a bed')
|
||||
})
|
||||
|
||||
it('joins distinct tools', () => {
|
||||
expect(describeStep({ index: 0, tools: ['clear_object', 'fill_region'] })).toBe('Clearing a bed, Filling a bed')
|
||||
})
|
||||
|
||||
it('says something for a step with no tool calls', () => {
|
||||
expect(describeStep({ index: 0, tools: [] })).toBe('Thinking')
|
||||
})
|
||||
|
||||
// A tool added server-side before the client knows about it should degrade to
|
||||
// something readable rather than showing snake_case at the user.
|
||||
it('falls back readably for an unknown tool', () => {
|
||||
expect(describeStep({ index: 0, tools: ['prune_orchard'] })).toBe('prune orchard')
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,188 @@
|
||||
// The garden assistant's client (#57).
|
||||
//
|
||||
// The chat lives in the editor, not on its own page, because watching the canvas
|
||||
// change as the agent works IS the confirmation — which is what makes acting
|
||||
// without asking first tolerable. So this module's job is as much about
|
||||
// surfacing progress as it is about sending a message.
|
||||
|
||||
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'
|
||||
import { z } from 'zod'
|
||||
import { API_BASE, api } from './api'
|
||||
import { gardenFullKey } from './objects'
|
||||
import { historyKey } from './history'
|
||||
|
||||
const capabilitiesSchema = z.object({ agent: z.boolean() })
|
||||
|
||||
/** Whether this instance has the assistant configured. Without it the panel
|
||||
* isn't rendered at all — a dead button is worse than no button. */
|
||||
export function useCapabilities() {
|
||||
return useQuery({
|
||||
queryKey: ['capabilities'] as const,
|
||||
queryFn: async () => capabilitiesSchema.parse(await api.get('/capabilities')),
|
||||
staleTime: Infinity, // server config; it doesn't change under a running page
|
||||
})
|
||||
}
|
||||
|
||||
export const agentMessageSchema = z.object({
|
||||
id: z.number(),
|
||||
conversationId: z.number(),
|
||||
role: z.enum(['user', 'assistant']),
|
||||
body: z.string(),
|
||||
changeSetId: z.number().optional(),
|
||||
createdAt: z.string(),
|
||||
})
|
||||
export type AgentMessage = z.infer<typeof agentMessageSchema>
|
||||
|
||||
const historySchema = z.object({ messages: z.array(agentMessageSchema) })
|
||||
|
||||
export function agentHistoryKey(gardenId: number) {
|
||||
return ['gardens', gardenId, 'agent-history'] as const
|
||||
}
|
||||
|
||||
/** The stored conversation, so a reload doesn't lose the thread — which is
|
||||
* exactly when someone reloads, to check whether a change actually landed. */
|
||||
export function useAgentHistory(gardenId: number, enabled: boolean) {
|
||||
return useQuery({
|
||||
queryKey: agentHistoryKey(gardenId),
|
||||
enabled,
|
||||
queryFn: async (): Promise<AgentMessage[]> =>
|
||||
historySchema.parse(await api.get(`/gardens/${gardenId}/agent/history`)).messages,
|
||||
})
|
||||
}
|
||||
|
||||
export function useClearAgentHistory(gardenId: number) {
|
||||
const qc = useQueryClient()
|
||||
return useMutation({
|
||||
mutationFn: async (): Promise<void> => {
|
||||
await api.delete(`/gardens/${gardenId}/agent/history`)
|
||||
},
|
||||
onSuccess: () => qc.invalidateQueries({ queryKey: agentHistoryKey(gardenId) }),
|
||||
})
|
||||
}
|
||||
|
||||
/** One completed turn, as the server reports it. */
|
||||
export interface AgentTurn {
|
||||
reply: string
|
||||
changeSetId?: number
|
||||
steps: number
|
||||
truncated?: boolean
|
||||
}
|
||||
|
||||
/** A step the model just finished, named in the app's own vocabulary. */
|
||||
export interface AgentStep {
|
||||
index: number
|
||||
tools: string[]
|
||||
}
|
||||
|
||||
// What each tool is doing, in words. Raw tool names ("fill_region") tell you the
|
||||
// agent is busy; these tell you what it's busy DOING, which is the difference
|
||||
// between the panel feeling alive and feeling hung.
|
||||
const TOOL_LABELS: Record<string, string> = {
|
||||
list_gardens: 'Looking at your gardens',
|
||||
describe_garden: 'Reading the garden',
|
||||
create_object: 'Adding a bed',
|
||||
move_object: 'Moving a bed',
|
||||
place_planting: 'Planting',
|
||||
fill_region: 'Filling a bed',
|
||||
clear_object: 'Clearing a bed',
|
||||
find_plant: 'Looking up a plant',
|
||||
create_plant: 'Adding a plant to your catalog',
|
||||
add_journal_entry: 'Writing a journal note',
|
||||
}
|
||||
|
||||
export function describeStep(step: AgentStep): string {
|
||||
if (step.tools.length === 0) return 'Thinking'
|
||||
const labels = step.tools.map((t) => TOOL_LABELS[t] ?? t.replace(/_/g, ' '))
|
||||
// Repeated tools in one step read as one action, not a list of identical ones.
|
||||
return [...new Set(labels)].join(', ')
|
||||
}
|
||||
|
||||
export interface StreamHandlers {
|
||||
onStep: (step: AgentStep) => void
|
||||
onDone: (turn: AgentTurn) => void
|
||||
onError: (message: string) => void
|
||||
}
|
||||
|
||||
/**
|
||||
* Send a message and stream the reply.
|
||||
*
|
||||
* Hand-rolled rather than EventSource, which can only issue GETs — this needs a
|
||||
* POST body. The wire format is still SSE so a proxy that understands it doesn't
|
||||
* buffer, and so switching to EventSource later wouldn't change the server.
|
||||
*/
|
||||
export async function streamChat(
|
||||
gardenId: number,
|
||||
message: string,
|
||||
handlers: StreamHandlers,
|
||||
signal?: AbortSignal,
|
||||
): Promise<void> {
|
||||
let res: Response
|
||||
try {
|
||||
res = await fetch(`${API_BASE}/agent/chat`, {
|
||||
method: 'POST',
|
||||
headers: { 'content-type': 'application/json' },
|
||||
body: JSON.stringify({ gardenId, message }),
|
||||
credentials: 'same-origin',
|
||||
signal,
|
||||
})
|
||||
} catch {
|
||||
|
gitea-actions
commented
🔴 Abort during fetch falsely reported as network error correctness, error-handling · flagged by 5 models
🪰 Gadfly · advisory 🔴 **Abort during fetch falsely reported as network error**
_correctness, error-handling · flagged by 5 models_
- **`web/src/lib/agent.ts:128`** — The `fetch` catch block reports *"Could not reach the server"* for every thrown error, but it never checks `signal?.aborted`. If the user clicks **Stop** (or the panel unmounts) while the `fetch` is still in flight, the intentional `AbortError` is misreported as a network failure. The `reader.read()` phase later in the same function correctly guards with `if (signal?.aborted) return` (line 151); the `fetch` phase needs the same guard.
<sub>🪰 Gadfly · advisory</sub>
|
||||
handlers.onError('Could not reach the server.')
|
||||
return
|
||||
}
|
||||
if (!res.ok || !res.body) {
|
||||
handlers.onError(
|
||||
res.status === 404
|
||||
? "This instance doesn't have the assistant configured."
|
||||
: 'The assistant is not available right now.',
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
const reader = res.body.getReader()
|
||||
const decoder = new TextDecoder()
|
||||
let buffer = ''
|
||||
for (;;) {
|
||||
let chunk: ReadableStreamReadResult<Uint8Array>
|
||||
try {
|
||||
chunk = await reader.read()
|
||||
} catch {
|
||||
// An aborted read is the caller navigating away, not a failure worth
|
||||
// reporting back to them.
|
||||
if (signal?.aborted) return
|
||||
handlers.onError('The connection dropped partway through.')
|
||||
return
|
||||
}
|
||||
if (chunk.done) break
|
||||
buffer += decoder.decode(chunk.value, { stream: true })
|
||||
|
||||
// SSE frames are separated by a blank line; anything after the last one is
|
||||
// a partial frame to keep for the next chunk.
|
||||
const frames = buffer.split('\n\n')
|
||||
buffer = frames.pop() ?? ''
|
||||
for (const frame of frames) {
|
||||
const line = frame.split('\n').find((l) => l.startsWith('data:'))
|
||||
if (!line) continue
|
||||
let event: unknown
|
||||
try {
|
||||
event = JSON.parse(line.slice(5).trim())
|
||||
} catch {
|
||||
continue // a malformed frame shouldn't kill a working stream
|
||||
}
|
||||
const e = event as { step?: AgentStep; done?: AgentTurn; error?: string }
|
||||
|
gitea-actions
commented
🔴 Server Warning field silently dropped in SSE stream correctness, error-handling, maintainability · flagged by 4 models
🪰 Gadfly · advisory 🔴 **Server Warning field silently dropped in SSE stream**
_correctness, error-handling, maintainability · flagged by 4 models_
- **`web/src/lib/agent.ts:171-174`** — The client parses SSE events into `{ step?, done?, error? }` but the server also sends a `warning` field (see `internal/api/agent.go:34-43` and `agent.go:90`). When the turn succeeds but the transcript can't be saved, the server emits `{"done": {...}, "warning": "I couldn't save this exchange..."}` so the user knows the conversation will disappear on reload. The client ignores `warning` entirely, producing the exact "quiet inconsistency" the server comment…
<sub>🪰 Gadfly · advisory</sub>
|
||||
if (e.error) handlers.onError(e.error)
|
||||
else if (e.step) handlers.onStep(e.step)
|
||||
else if (e.done) handlers.onDone(e.done)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/** Refresh everything a turn may have changed: the canvas, the history list,
|
||||
* and the conversation itself. */
|
||||
export function useAgentRefresh(gardenId: number) {
|
||||
const qc = useQueryClient()
|
||||
return () => {
|
||||
void qc.invalidateQueries({ queryKey: gardenFullKey(gardenId) })
|
||||
|
gitea-actions
commented
🟠 useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn performance · flagged by 2 models
🪰 Gadfly · advisory 🟠 **useAgentRefresh invalidates agent-history and change-set-history on every step, but neither changes until the turn completes — up to ~2×(N-1) redundant refetches per turn**
_performance · flagged by 2 models_
- **`web/src/lib/agent.ts:184-186` — `useAgentRefresh` invalidates the conversation history and change-set history on every step, but neither changes mid-turn.** `onStep` calls `refresh()`, which invalidates `gardenFullKey`, `historyKey`, and `agentHistoryKey` all at once. I verified the server side: the conversation transcript is only written *after* the run completes via `RecordAgentExchange` (`internal/api/agent.go:87`), and the change set is only committed *after* the run function returns vi…
<sub>🪰 Gadfly · advisory</sub>
|
||||
void qc.invalidateQueries({ queryKey: historyKey(gardenId) })
|
||||
void qc.invalidateQueries({ queryKey: agentHistoryKey(gardenId) })
|
||||
}
|
||||
}
|
||||
@@ -113,3 +113,29 @@ describe('totalChanges', () => {
|
||||
).toBe(13)
|
||||
})
|
||||
})
|
||||
|
||||
describe('describeUndo with an unknown total', () => {
|
||||
// The chat panel offers Undo on a turn knowing only its change set id — the
|
||||
// agent's reply carries the id, not the tally. Inventing a denominator to
|
||||
// fill the sentence would be worse than not having one.
|
||||
it("doesn't invent a denominator it was never given", () => {
|
||||
const out = describeUndo(
|
||||
{ id: 5 },
|
||||
{
|
||||
changeSet: changeSet({ id: 6, counts: [{ entityType: 'planting', op: 'delete', n: 2 }] }),
|
||||
conflicts: [{ entityType: 'object', entityId: 7, reason: 'changed', name: 'North Bed' }],
|
||||
},
|
||||
)
|
||||
expect(out.tone).toBe('partial')
|
||||
expect(out.message).toBe('Partly undone — “North Bed” was edited since, so it was left alone.')
|
||||
expect(out.message).not.toMatch(/\d+ of \d+/)
|
||||
})
|
||||
|
||||
it('still reports a clean undo the same way', () => {
|
||||
const out = describeUndo(
|
||||
{ id: 5 },
|
||||
{ changeSet: changeSet({ id: 6, counts: [{ entityType: 'object', op: 'update', n: 1 }] }), conflicts: [] },
|
||||
)
|
||||
expect(out).toEqual({ tone: 'ok', message: 'Undone.' })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -171,7 +171,7 @@ export function useUndo(gardenId: number) {
|
||||
const revert = useRevertChangeSet(gardenId)
|
||||
const [outcomes, setOutcomes] = useState<Record<number, UndoOutcome>>({})
|
||||
|
||||
const undo = (cs: ChangeSet) => {
|
||||
const undo = (cs: UndoTarget) => {
|
||||
setOutcomes((prev) => ({ ...prev, [cs.id]: { tone: 'pending', message: 'Undoing…' } }))
|
||||
revert.mutate(cs.id, {
|
||||
onSuccess: (result) => {
|
||||
@@ -197,6 +197,19 @@ export function useUndo(gardenId: number) {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* What undo needs to know about a change set.
|
||||
*
|
||||
* `counts` is optional because the chat panel offers Undo on a turn knowing only
|
||||
* its change set id — the agent's reply carries the id, not the tally. Fabricating
|
||||
* counts to satisfy a type would produce a confidently wrong "1 of 1 changes
|
||||
* undone"; leaving them out lets describeUndo say what it actually knows.
|
||||
*/
|
||||
export interface UndoTarget {
|
||||
id: number
|
||||
counts?: ChangeCount[]
|
||||
}
|
||||
|
||||
export interface UndoOutcome {
|
||||
tone: 'pending' | 'ok' | 'partial' | 'error'
|
||||
message: string
|
||||
@@ -208,7 +221,7 @@ export interface UndoOutcome {
|
||||
* useful thing to report: a bare failure would be a lie about the two that did
|
||||
* apply, and a bare success would hide the one that didn't.
|
||||
*/
|
||||
export function describeUndo(target: ChangeSet, result: RevertResult): UndoOutcome {
|
||||
export function describeUndo(target: UndoTarget, result: RevertResult): UndoOutcome {
|
||||
const skipped = result.conflicts.map(describeConflict).join('; ')
|
||||
const applied = result.changeSet ? totalChanges(result.changeSet) : 0
|
||||
if (result.conflicts.length === 0) {
|
||||
@@ -221,6 +234,9 @@ export function describeUndo(target: ChangeSet, result: RevertResult): UndoOutco
|
||||
if (applied === 0) {
|
||||
return { tone: 'error', message: `Nothing was undone — ${skipped}.` }
|
||||
}
|
||||
const total = totalChanges(target)
|
||||
return { tone: 'partial', message: `${applied} of ${total} changes undone — ${skipped}.` }
|
||||
// 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.
|
||||
const total = target.counts ? target.counts.reduce((sum, c) => sum + c.n, 0) : 0
|
||||
|
gitea-actions
commented
🟡 describeUndo duplicates totalChanges' summation logic instead of sharing a helper maintainability · flagged by 4 models
🪰 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 scale = total > 0 ? `${applied} of ${total} changes undone` : 'Partly undone'
|
||||
return { tone: 'partial', message: `${scale} — ${skipped}.` }
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ import { Button } from '@/components/ui/Button'
|
||||
import { GardenCanvas } from '@/editor/GardenCanvas'
|
||||
import { EditorRail, type RailTab } from '@/editor/EditorRail'
|
||||
import { HistoryPanel } from '@/editor/HistoryPanel'
|
||||
import { ChatPanel } from '@/editor/ChatPanel'
|
||||
import { JournalPanel } from '@/editor/JournalPanel'
|
||||
import { Inspector } from '@/editor/Inspector'
|
||||
import { PlopInspector } from '@/editor/PlopInspector'
|
||||
@@ -30,6 +31,7 @@ import {
|
||||
} from '@/lib/objects'
|
||||
import { toEditorPlanting } from '@/lib/plantings'
|
||||
import type { Plant } from '@/lib/plants'
|
||||
import { useCapabilities } from '@/lib/agent'
|
||||
import { useJournalCounts } from '@/lib/journal'
|
||||
import { attributableLots, lotsByPlant, useSeedLots, type SeedLot } from '@/lib/seedLots'
|
||||
import { useSeedTray } from '@/lib/seedTray'
|
||||
@@ -64,6 +66,7 @@ export function GardenEditorPage() {
|
||||
const journalObjectId = useEditorStore((s) => s.journalObjectId)
|
||||
const setJournalObjectId = useEditorStore((s) => s.setJournalObjectId)
|
||||
const journalCounts = useJournalCounts(gid)
|
||||
const capabilities = useCapabilities()
|
||||
const journalTotal = useMemo(
|
||||
() => [...(journalCounts.data?.values() ?? [])].reduce((a, b) => a + b, 0),
|
||||
[journalCounts.data],
|
||||
@@ -385,6 +388,16 @@ export function GardenEditorPage() {
|
||||
},
|
||||
]
|
||||
|
||||
// Only when the instance actually has the assistant configured. A tab that
|
||||
// opens onto an apology is worse than no tab.
|
||||
if (capabilities.data?.agent) {
|
||||
railTabs.push({
|
||||
id: 'chat',
|
||||
label: 'Assistant',
|
||||
render: () => <ChatPanel gardenId={gid} canEdit={canEdit} />,
|
||||
})
|
||||
}
|
||||
|
||||
return (
|
||||
<div className="flex h-[calc(100vh-8rem)] flex-col gap-3 md:flex-row">
|
||||
<div className="shrink-0 md:w-40">
|
||||
@@ -422,6 +435,15 @@ export function GardenEditorPage() {
|
||||
>
|
||||
History
|
||||
</Button>
|
||||
{capabilities.data?.agent && (
|
||||
<Button
|
||||
variant="ghost"
|
||||
className="mt-1 w-full text-sm"
|
||||
onClick={() => setRailTab(railTab === 'chat' ? null : 'chat')}
|
||||
>
|
||||
💬 Assistant
|
||||
</Button>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<div className="relative flex min-h-0 flex-1 flex-col gap-2">
|
||||
|
||||
🟡 history.isError is never surfaced; a failed history load silently renders as an empty conversation
error-handling · flagged by 1 model
web/src/editor/ChatPanel.tsx:27,86,111— confirmedhistory.isErroris never referenced anywhere in the file (onlyhistory.dataandhistory.isSuccessare used). IfGET /gardens/:id/agent/historyfails,messagesfalls back to[]viahistory.data ?? [](line 86), and sinceisSuccessstays false the placeholder "Ask for what you want…" text (gated onisSuccess, line 111) doesn't render either — the panel just goes silently blank with no distinction from a loading or empty state…🪰 Gadfly · advisory