From 77bc672c191dd1b6440c1b02562b42d582baa5a5 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 02:42:47 -0400 Subject: [PATCH] Address Gadfly review on the chat panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worst finding is one I created two commits ago: the server's `warning` field — added precisely because a failed save was being swallowed — was never read by the client. The "I couldn't save this exchange" message went nowhere, which recreated the exact silence the warning existed to break. It's wired now, and SSE frames are validated with zod rather than type-asserted, so the next field added server-side fails loudly instead of vanishing. Aborting during the initial fetch reported "Could not reach the server." The read loop already knew an abort was the caller's own doing; the request path did not, so pressing Stop looked like the network had failed. Unmount no longer aborts an in-flight turn, and that one is a design bug of my own making: selecting an object auto-switches the rail to the inspector, so clicking the canvas mid-turn silently killed the turn — while the canvas is exactly what you're meant to be watching. The request continues, the exchange is persisted server-side, and coming back to the tab shows it. Stop still aborts, but only our READ: the turn keeps running server-side either way, which is why its work still lands in History. /capabilities reported cfg.Agent.Ready() while the routes require NewRunner to have succeeded. A configured-but-unresolvable model would therefore advertise a chat tab whose first message 404s. It reports the runner's actual state now, and is a named handler like everything else in that file. A failed history load rendered as an empty conversation, which looks like the thread was lost — a much worse thing to believe than "it didn't load". Both the loading and error states are shown, and a failed "Start over" says so. Per-step refresh refetched the change history and the conversation, neither of which can move until the turn commits: up to 2×(N−1) requests per turn for data that cannot have changed. Split into a canvas-only refresh for steps and a full one for the end. describeUndo had its own copy of totalChanges' summation; it uses the helper, which now accepts an optional counts list. React.ReactNode became an imported ReactNode, matching every other component. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/api/api.go | 20 +++++++---- web/src/editor/ChatPanel.tsx | 45 +++++++++++++++++------- web/src/lib/agent.ts | 68 +++++++++++++++++++++++++++++------- web/src/lib/history.ts | 9 ++--- 4 files changed, 108 insertions(+), 34 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index c84ce0c..8777f11 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -53,12 +53,10 @@ 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()}) - }) + // What this instance can actually do, so the UI offers only what works. + // Registered after the agent below, because it reports whether the runner + // actually built — not merely whether it was configured to. + v1.GET("/capabilities", h.capabilities) // 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 @@ -197,6 +195,16 @@ func New(cfg *config.Config, svc *service.Service) *gin.Engine { 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. func healthz(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"ok": true}) diff --git a/web/src/editor/ChatPanel.tsx b/web/src/editor/ChatPanel.tsx index 709461e..3a7b486 100644 --- a/web/src/editor/ChatPanel.tsx +++ b/web/src/editor/ChatPanel.tsx @@ -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 { errorMessage } from '@/lib/api' import { Button } from '@/components/ui/Button' import { TextArea } from '@/components/ui/TextArea' import { cn } from '@/lib/cn' @@ -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. const [pending, setPending] = useState<{ message: string; steps: AgentStep[] } | null>(null) const [error, setError] = useState(null) + const [warning, setWarning] = useState(null) const abort = useRef(null) const bottom = useRef(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(), []) + // Deliberately NOT aborted on unmount. Selecting an object auto-switches the + // rail to the inspector, so aborting here would mean clicking the canvas + // 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. useEffect(() => { @@ -50,6 +56,7 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo if (!message || pending) return setInput('') setError(null) + setWarning(null) setPending({ message, steps: [] }) const controller = new AbortController() @@ -61,22 +68,24 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo { 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() + // The canvas updating under the conversation is the whole point of + // putting the chat here, so refresh it as each step lands — but only + // it: nothing else can have changed until the turn commits. + refresh.canvas() }, onDone: (turn: AgentTurn) => { setPending(null) - refresh() + refresh.everything() if (turn.truncated) { setError('That turned into more steps than I should take at once — check what changed before continuing.') } }, + onWarning: setWarning, onError: (message) => { setPending(null) setError(message) // Something may still have landed before it failed. - refresh() + refresh.everything() }, }, controller.signal, @@ -92,7 +101,11 @@ export function ChatPanel({ gardenId, canEdit }: { gardenId: number; canEdit: bo {messages.length > 0 && (