From 85babd0c387c00491867eef4c321f07320130154 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 23 Aug 2026 15:22:48 -0700 Subject: [PATCH 1/4] fix(agents): show draft cards only for drafts that exist on disk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Agents gallery used to build draft cards from open build-agent chat sessions plus an in-memory cache of draft metadata. When a draft file was moved or deleted out from under the app, the card stayed behind and Delete failed with `Source "…" not found`, leaving a card that could never be removed. Drafts are now read from disk like finished agents: `listAgentGallery()` splits a single `listAgentSources()` call into personas and drafts, the agent store keeps `draftSources`, and `AgentsView` renders a card per draft file, joining it to its builder session when one is still open. File gone -> card gone on the next refresh. Deleting a draft whose chat is gone discards the file directly instead of going through a session. Untouched drafts no longer pile up or prompt. `modelProviderId` is seeded on every new draft and was being counted as user content, so leaving a fresh "New agent" draft asked "Save this agent draft?" and kept an `untitled-agent-*.md` around. It is now exempt from the placeholder check, and the navigation guard silently discards a draft with no user content (navigating first so closing the empty chat does not redirect home). Editing an existing agent without changes still just navigates away. `findAgentBuilderSource` drops a cached draft from the in-memory cache when its file is no longer listed by the backend and cannot be read, so a missing file can't deadlock delete again. Also removes a duplicate `useVoiceConversationStore` import in the AppShell navigation test that was failing lint on main. Co-Authored-By: Claude --- src/app/AppShell.navigation.test.tsx | 51 +++++++---- .../capabilities/AgentBuilderCapability.tsx | 15 +++- .../__tests__/AgentBuilderCapability.test.tsx | 6 +- .../hooks/__tests__/usePersonas.test.ts | 87 +++++++++++++------ .../hooks/useAgentBuilderCoordinator.ts | 26 +++++- src/features/agents/hooks/usePersonas.ts | 12 +-- .../lib/__tests__/agentBuilderSession.test.ts | 70 +++++++++++++++ .../agents/lib/agentBuilderIdentity.ts | 3 + .../agents/lib/agentBuilderSession.ts | 13 +++ .../agents/lib/agentBuilderSourceLifecycle.ts | 19 +++- src/features/agents/stores/agentStore.ts | 13 +++ src/features/agents/ui/AgentsView.tsx | 79 +++++++++++------ src/features/agents/ui/PersonaGallery.tsx | 62 +++++++------ .../ui/__tests__/AgentsView.entry.test.tsx | 67 ++++++++++++-- .../chat/ui/__tests__/ChatRightRail.test.tsx | 31 +++++-- src/shared/api/agents.ts | 33 ++++++- 16 files changed, 457 insertions(+), 130 deletions(-) diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 6926f031e..e823c7055 100644 --- a/src/app/AppShell.navigation.test.tsx +++ b/src/app/AppShell.navigation.test.tsx @@ -26,7 +26,6 @@ import type { Message } from "@/shared/types/messages"; import type { GitState } from "@/shared/types/git"; import { setMultiWorkspaceEnabled } from "@/features/workspaces/multiWorkspacePreference"; import { OPEN_SETTINGS_EVENT } from "@/features/settings/lib/settingsEvents"; -import { useVoiceConversationStore } from "@/features/voice-conversation/stores/voiceConversationStore"; import { SHORTCUT_PREFERENCES_STORAGE_KEY } from "@/features/shortcuts/lib/shortcutRegistry"; import { useShortcutsDialogStore } from "@/features/shortcuts/stores/shortcutsDialogStore"; import { useProjectStore } from "@/features/projects/stores/projectStore"; @@ -4341,8 +4340,27 @@ describe("AppShell global navigation", () => { ).not.toBeInTheDocument(); }); - it("returns to agent builder mode after going back then forward", async () => { + it("discards an untouched agent draft when navigating back, without prompting", async () => { const user = userEvent.setup(); + // The placeholder really exists on disk: the backend lists it and it + // reads back unchanged. + const placeholder = { + type: "agent", + path: "/Users/test/.agents/agents/untitled-agent-created-session.md", + name: "Untitled agent created-sess", + description: "Draft", + content: "Draft in progress.", + global: true, + writable: true, + properties: { draft: true, builderSessionId: "created-session" }, + }; + mockListPersonaSources.mockResolvedValue([placeholder]); + mockReadAgentSourceFile.mockResolvedValue(placeholder); + // Once deleted, the file is no longer listed or readable. + mockDeletePersonaSource.mockImplementation(async () => { + mockListPersonaSources.mockResolvedValue([]); + mockReadAgentSourceFile.mockRejectedValue(new Error("not found")); + }); renderAppShell(); await user.click(screen.getByRole("button", { name: "Sidebar agents" })); @@ -4350,29 +4368,28 @@ describe("AppShell global navigation", () => { await waitFor(() => { expect(screen.getByTestId("active-view")).toHaveTextContent("chat"); }); - await waitFor(() => { - expect(useChatSessionStore.getState().getActiveSession()).toMatchObject({ - id: "created-session", - intent: "build-agent", - }); - }); + await waitForCreatedAgentBuilderTarget(); + // Nothing was typed or edited, so leaving is silent: no "save this + // draft?" prompt, and the placeholder file and its builder state are + // gone rather than lingering as an untitled draft. await user.click(screen.getByRole("button", { name: "Back" })); await waitFor(() => { expect(screen.getByTestId("active-view")).toHaveTextContent("agents"); }); - - await user.click(screen.getByRole("button", { name: "Forward" })); + expect( + screen.queryByText("Save this agent draft?"), + ).not.toBeInTheDocument(); await waitFor(() => { - expect(screen.getByTestId("active-view")).toHaveTextContent("chat"); + expect(mockDeletePersonaSource).toHaveBeenCalledWith( + "/Users/test/.agents/agents/untitled-agent-created-session.md", + ); }); await waitFor(() => { - expect(useChatSessionStore.getState().getActiveSession()).toMatchObject({ - id: "created-session", - intent: "build-agent", - targetAgentPath: - "/Users/test/.agents/agents/untitled-agent-created-session.md", - }); + const session = useChatSessionStore + .getState() + .getSession("created-session"); + expect(session?.intent ?? null).toBeNull(); }); }); diff --git a/src/features/agents/capabilities/AgentBuilderCapability.tsx b/src/features/agents/capabilities/AgentBuilderCapability.tsx index 751a9c333..0cc0172b6 100644 --- a/src/features/agents/capabilities/AgentBuilderCapability.tsx +++ b/src/features/agents/capabilities/AgentBuilderCapability.tsx @@ -17,7 +17,7 @@ import type { ChatSession } from "@/features/chat/stores/chatSessionStore"; import { useChatSessionStore } from "@/features/chat/stores/chatSessionStore"; import { agentSourceToPersona, - listPersonas, + listAgentGallery, type AgentSourceEntry, } from "@/shared/api/agents"; @@ -52,8 +52,10 @@ export function AgentBuilderCapability({ const patchSession = useChatSessionStore((state) => state.patchSession); const refreshPersonas = useCallback(async () => { - const personas = await listPersonas(); - useAgentStore.getState().setPersonas(personas); + const { personas, drafts } = await listAgentGallery(); + const agentStore = useAgentStore.getState(); + agentStore.setPersonas(personas); + agentStore.setDraftSources(drafts); }, []); const completeBuilder = useCallback( @@ -73,6 +75,13 @@ export function AgentBuilderCapability({ } else { agentStore.addPersona(promotedPersona); } + // The draft just became this agent; drop its card without waiting for + // the disk refresh so the gallery never shows both at once. + for (const draft of agentStore.draftSources) { + if (draft.properties?.builderSessionId === session.id) { + agentStore.removeDraftSource(draft.path); + } + } onDraftPromoted?.(source); onAgentBuilderCompleted?.(promotedPersona.id); diff --git a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx index 69a4adefe..6dc891cfc 100644 --- a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx +++ b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx @@ -15,7 +15,7 @@ const apiMocks = vi.hoisted(() => ({ listPersonaSources: vi.fn(), readAgentSourceFile: vi.fn(), updatePersonaSource: vi.fn(), - listPersonas: vi.fn(), + listAgentGallery: vi.fn(), hasRealAgentDescription: (description: string | null | undefined) => { const normalized = description?.trim().toLowerCase(); return Boolean( @@ -105,7 +105,7 @@ describe("AgentBuilderCapability keep-save telemetry", () => { apiMocks.listPersonaSources.mockReset(); apiMocks.readAgentSourceFile.mockReset(); apiMocks.updatePersonaSource.mockReset(); - apiMocks.listPersonas.mockReset(); + apiMocks.listAgentGallery.mockReset(); apiMocks.listPersonaSources.mockResolvedValue([existingAgentSource]); apiMocks.readAgentSourceFile.mockImplementation( async (_path: string, fallback?: AgentSourceEntry) => @@ -123,7 +123,7 @@ describe("AgentBuilderCapability keep-save telemetry", () => { }, }), ); - apiMocks.listPersonas.mockResolvedValue([]); + apiMocks.listAgentGallery.mockResolvedValue({ personas: [], drafts: [] }); resetAgentBuilderSourceLifecycleForTests(); setExperimentEnabled(AVATAR_COLLECTION_PAGE_EXPERIMENT_ID, false); useAgentStore.setState({ diff --git a/src/features/agents/hooks/__tests__/usePersonas.test.ts b/src/features/agents/hooks/__tests__/usePersonas.test.ts index 4b81a875f..83d5b7c8d 100644 --- a/src/features/agents/hooks/__tests__/usePersonas.test.ts +++ b/src/features/agents/hooks/__tests__/usePersonas.test.ts @@ -12,7 +12,7 @@ const avatarApiMocks = vi.hoisted(() => ({ vi.mock("@/shared/api/avatars", () => avatarApiMocks); vi.mock("@/shared/api/agents", () => ({ - listPersonas: vi.fn().mockResolvedValue([]), + listAgentGallery: vi.fn().mockResolvedValue({ personas: [], drafts: [] }), createPersona: vi.fn().mockResolvedValue({ id: "new-id", displayName: "Test", @@ -32,7 +32,7 @@ vi.mock("@/shared/api/agents", () => ({ updatedAt: "2026-01-01T00:00:00Z", }), deletePersona: vi.fn().mockResolvedValue(undefined), - refreshPersonas: vi.fn().mockResolvedValue([]), + refreshAgentGallery: vi.fn().mockResolvedValue({ personas: [], drafts: [] }), })); // Import the mocked module so we can inspect/adjust calls @@ -43,6 +43,13 @@ import { usePersonas } from "../usePersonas"; // ── helpers ────────────────────────────────────────────────────────── +function gallery( + personas: Persona[], + drafts: api.AgentGalleryListing["drafts"] = [], +): api.AgentGalleryListing { + return { personas, drafts }; +} + function makePersona(overrides: Partial = {}): Persona { return { id: crypto.randomUUID(), @@ -62,7 +69,7 @@ describe("usePersonas", () => { beforeEach(() => { // Re-establish default mock implementations (clearAllMocks would wipe them) avatarApiMocks.deleteUserAvatar.mockReset().mockResolvedValue(undefined); - vi.mocked(api.listPersonas).mockReset().mockResolvedValue([]); + vi.mocked(api.listAgentGallery).mockReset().mockResolvedValue(gallery([])); vi.mocked(api.createPersona).mockReset().mockResolvedValue({ id: "new-id", displayName: "Test", @@ -82,11 +89,14 @@ describe("usePersonas", () => { updatedAt: "2026-01-01T00:00:00Z", }); vi.mocked(api.deletePersona).mockReset().mockResolvedValue(undefined); - vi.mocked(api.refreshPersonas).mockReset().mockResolvedValue([]); + vi.mocked(api.refreshAgentGallery) + .mockReset() + .mockResolvedValue(gallery([])); useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], agents: [], agentsLoading: false, activeAgentId: null, @@ -101,25 +111,38 @@ describe("usePersonas", () => { // ── loading ──────────────────────────────────────────────────────── describe("loading personas", () => { - it("loads personas on mount via listPersonas()", async () => { + it("loads personas and drafts on mount via listAgentGallery()", async () => { const personas = [makePersona({ id: "p1" }), makePersona({ id: "p2" })]; - vi.mocked(api.listPersonas).mockResolvedValueOnce(personas); + const draft = { + type: "agent" as const, + path: "/Users/x/.agents/agents/untitled-agent-1.md", + name: "Untitled agent 1", + description: "Draft", + content: "Draft in progress.", + global: true, + writable: true, + properties: { draft: true, builderSessionId: "sess-1" }, + }; + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery(personas, [draft]), + ); const { result } = renderHook(() => usePersonas()); await waitFor(() => { - expect(api.listPersonas).toHaveBeenCalledTimes(1); + expect(api.listAgentGallery).toHaveBeenCalledTimes(1); }); await waitFor(() => { expect(result.current.personas).toEqual(personas); }); + expect(useAgentStore.getState().draftSources).toEqual([draft]); }); it("sets loading state correctly", async () => { // Create a deferred promise to control timing - let resolveList!: (value: Persona[]) => void; - vi.mocked(api.listPersonas).mockImplementationOnce( + let resolveList!: (value: api.AgentGalleryListing) => void; + vi.mocked(api.listAgentGallery).mockImplementationOnce( () => new Promise((resolve) => { resolveList = resolve; @@ -135,7 +158,7 @@ describe("usePersonas", () => { // Resolve the API call await act(async () => { - resolveList([]); + resolveList(gallery([])); }); await waitFor(() => { @@ -163,7 +186,7 @@ describe("usePersonas", () => { // Wait for initial load to fully complete await waitFor(() => { - expect(api.listPersonas).toHaveBeenCalledTimes(1); + expect(api.listAgentGallery).toHaveBeenCalledTimes(1); expect(result.current.isLoading).toBe(false); }); @@ -186,7 +209,9 @@ describe("usePersonas", () => { it("updatePersona calls API and updates store", async () => { const existing = makePersona({ id: "test-id", displayName: "Old" }); // Return existing persona from initial load so the store has it - vi.mocked(api.listPersonas).mockResolvedValueOnce([existing]); + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery([existing]), + ); const updated = { id: "test-id", @@ -229,7 +254,9 @@ describe("usePersonas", () => { id: "shared-id", avatar: "user-avatar:shared", }); - vi.mocked(api.listPersonas).mockResolvedValueOnce([existing, shared]); + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery([existing, shared]), + ); vi.mocked(api.updatePersona).mockResolvedValue({ ...existing, avatar: "user-avatar:new", @@ -256,7 +283,9 @@ describe("usePersonas", () => { it("preserves gloopies displaced by overlapping updates", async () => { const existing = makePersona({ id: "test-id", avatar: "user-avatar:a" }); - vi.mocked(api.listPersonas).mockResolvedValueOnce([existing]); + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery([existing]), + ); const first = makePersona({ id: "test-id", avatar: "user-avatar:b" }); const second = makePersona({ id: "test-id", avatar: "user-avatar:c" }); const firstResult = vi.fn<() => Promise>(); @@ -294,7 +323,9 @@ describe("usePersonas", () => { it("deletePersona calls API and removes from store", async () => { const existing = makePersona({ id: "del-id" }); // Return existing persona from initial load so the store has it - vi.mocked(api.listPersonas).mockResolvedValueOnce([existing]); + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery([existing]), + ); const { result } = renderHook(() => usePersonas()); @@ -322,7 +353,9 @@ describe("usePersonas", () => { id: "second", avatar: "user-avatar:shared", }); - vi.mocked(api.listPersonas).mockResolvedValueOnce([first, second]); + vi.mocked(api.listAgentGallery).mockResolvedValueOnce( + gallery([first, second]), + ); const { result } = renderHook(() => usePersonas()); await waitFor(() => expect(result.current.personas).toHaveLength(2)); @@ -341,9 +374,11 @@ describe("usePersonas", () => { // ── refresh ──────────────────────────────────────────────────────── describe("refresh", () => { - it("refreshFromDisk calls refreshPersonas() API", async () => { + it("refreshFromDisk calls refreshAgentGallery() API", async () => { const refreshed = [makePersona({ id: "refreshed-1" })]; - vi.mocked(api.refreshPersonas).mockResolvedValueOnce(refreshed); + vi.mocked(api.refreshAgentGallery).mockResolvedValueOnce( + gallery(refreshed), + ); const { result } = renderHook(() => usePersonas()); @@ -355,13 +390,13 @@ describe("usePersonas", () => { await result.current.refreshFromDisk(); }); - expect(api.refreshPersonas).toHaveBeenCalled(); + expect(api.refreshAgentGallery).toHaveBeenCalled(); expect(result.current.personas).toEqual(refreshed); }); it("does not start overlapping refresh requests", async () => { - let resolveRefresh!: (value: Persona[]) => void; - vi.mocked(api.refreshPersonas).mockImplementationOnce( + let resolveRefresh!: (value: api.AgentGalleryListing) => void; + vi.mocked(api.refreshAgentGallery).mockImplementationOnce( () => new Promise((resolve) => { resolveRefresh = resolve; @@ -377,10 +412,10 @@ describe("usePersonas", () => { const firstRefresh = result.current.refreshFromDisk(); const secondRefresh = result.current.refreshFromDisk(); - expect(api.refreshPersonas).toHaveBeenCalledTimes(1); + expect(api.refreshAgentGallery).toHaveBeenCalledTimes(1); await act(async () => { - resolveRefresh([]); + resolveRefresh(gallery([])); await firstRefresh; await secondRefresh; }); @@ -389,8 +424,8 @@ describe("usePersonas", () => { it("ignores stale refresh results that started before a mutation", async () => { const stalePersona = makePersona({ id: "stale" }); const createdPersona = makePersona({ id: "created" }); - let resolveRefresh!: (value: Persona[]) => void; - vi.mocked(api.refreshPersonas).mockImplementationOnce( + let resolveRefresh!: (value: api.AgentGalleryListing) => void; + vi.mocked(api.refreshAgentGallery).mockImplementationOnce( () => new Promise((resolve) => { resolveRefresh = resolve; @@ -413,7 +448,7 @@ describe("usePersonas", () => { }); await act(async () => { - resolveRefresh([stalePersona]); + resolveRefresh(gallery([stalePersona])); await refresh; }); diff --git a/src/features/agents/hooks/useAgentBuilderCoordinator.ts b/src/features/agents/hooks/useAgentBuilderCoordinator.ts index 14fcbdb96..456a144e5 100644 --- a/src/features/agents/hooks/useAgentBuilderCoordinator.ts +++ b/src/features/agents/hooks/useAgentBuilderCoordinator.ts @@ -6,7 +6,7 @@ import type { AgentBuilderLeaveDraftDialogProps } from "../ui/AgentBuilderLeaveD import { discardDraftAgentSession, hasAgentBuilderSessionUserContent, - isDraftAgentBuilderSession, + isDiscardableAgentBuilderSession, reconcileAgentBuilderSessions, resolveAgentBuilderSessionId, saveDraftAgentSession, @@ -131,7 +131,23 @@ export function useAgentBuilderCoordinator({ session.id, ); if (!hasUserContent) { + // Nothing was made here. An untouched "New agent" draft leaves no + // trace — no prompt, no file, no empty chat. Editing an existing + // agent without changes just navigates away. + const discardable = await isDiscardableAgentBuilderSession( + session.id, + ); + // Navigate first so the empty chat is no longer the active session + // when it closes; closing the active chat would redirect home and + // stomp on where the user was actually going. next(); + if (discardable) { + await discardDraftAgentSession(session.id, { closeSession }).catch( + (error) => { + console.error("Failed to discard empty agent draft:", error); + }, + ); + } return; } @@ -143,7 +159,7 @@ export function useAgentBuilderCoordinator({ return false; }, - [promptForNavigation], + [closeSession, promptForNavigation], ); const start = useCallback( @@ -182,9 +198,11 @@ export function useAgentBuilderCoordinator({ } void (async () => { - const isDraft = await isDraftAgentBuilderSession(session.id); + const isDiscardable = await isDiscardableAgentBuilderSession( + session.id, + ); if ( - isDraft && + isDiscardable && !(await hasAgentBuilderSessionUserContent(session.id)) ) { await discardDraftAgentSession(session.id, { closeSession }).catch( diff --git a/src/features/agents/hooks/usePersonas.ts b/src/features/agents/hooks/usePersonas.ts index 7ec2b3a72..72a0a8e3f 100644 --- a/src/features/agents/hooks/usePersonas.ts +++ b/src/features/agents/hooks/usePersonas.ts @@ -17,6 +17,7 @@ export function usePersonas() { const personas = useAgentStore(selectPersonas); const personasLoading = useAgentStore(selectPersonasLoading); const setPersonas = useAgentStore((s) => s.setPersonas); + const setDraftSources = useAgentStore((s) => s.setDraftSources); const addPersona = useAgentStore((s) => s.addPersona); const updatePersonaInStore = useAgentStore((s) => s.updatePersona); const removePersona = useAgentStore((s) => s.removePersona); @@ -28,7 +29,7 @@ export function usePersonas() { const replacePersonasFromApi = useCallback( async ( - fetchPersonas: () => Promise, + fetchGallery: () => Promise, options: { showLoading: boolean; errorMessage: string }, ) => { if (listRequestInFlightRef.current) { @@ -42,12 +43,13 @@ export function usePersonas() { } try { - const personas = await fetchPersonas(); + const { personas, drafts } = await fetchGallery(); if ( mutationVersionAtStart === mutationVersionRef.current && mutationsInFlightRef.current === 0 ) { setPersonas(personas); + setDraftSources(drafts); } } catch (error) { console.error(options.errorMessage, error); @@ -58,7 +60,7 @@ export function usePersonas() { } } }, - [setPersonas, setPersonasLoading], + [setDraftSources, setPersonas, setPersonasLoading], ); const trackMutation = useCallback(async (mutation: () => Promise) => { @@ -73,14 +75,14 @@ export function usePersonas() { }, []); const loadPersonas = useCallback(async () => { - await replacePersonasFromApi(api.listPersonas, { + await replacePersonasFromApi(api.listAgentGallery, { showLoading: true, errorMessage: "Failed to load personas:", }); }, [replacePersonasFromApi]); const refreshFromDisk = useCallback(async () => { - await replacePersonasFromApi(api.refreshPersonas, { + await replacePersonasFromApi(api.refreshAgentGallery, { showLoading: false, errorMessage: "Failed to refresh personas from disk:", }); diff --git a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts index 49211ceb6..524bc9d3d 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -102,6 +102,7 @@ import { deleteDraftAgentSession, discardDraftAgentSession, hasAgentBuilderSessionUserContent, + isDiscardableAgentBuilderSession, isEmptyDraftAgentSession, promoteDraft, recoverDraftAgent, @@ -581,6 +582,27 @@ describe("agentBuilderSession", () => { ); }); + it("deleteDraftAgentSession clears a draft whose file was removed outside the app", async () => { + // Creating the draft caches it locally; then the file is moved away so + // the backend stops listing it and reads fail. + mocks.createPersonaSource.mockResolvedValue(draftSource); + await startAgentBuilderSession({}, deps); + await flushDraftPreparation(); + mocks.listPersonaSources.mockResolvedValue([]); + mocks.readAgentSourceFile.mockRejectedValue( + new Error("Failed to read agent source file"), + ); + + await deleteDraftAgentSession("sess-1", { closeSession }); + + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).toHaveBeenCalledWith("sess-1"); + expect(mocks.patchSession).toHaveBeenCalledWith( + "sess-1", + expect.objectContaining({ intent: null, targetAgentPath: null }), + ); + }); + it("discardDraftAgentSession deletes the draft and clears builder mode", async () => { addBuilderSession(); mocks.deletePersonaSource.mockResolvedValue(undefined); @@ -740,6 +762,54 @@ describe("agentBuilderSession", () => { ); }); + it("does not treat the seeded model provider as user content", async () => { + // "New agent" records the stored model preference as provider + + // modelProviderId + model. None of that is something the user typed. + addBuilderSession(); + const seededDraft = { + ...draftSource, + properties: { + draft: true, + builderSessionId: "sess-1", + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "claude-sonnet-5", + avatar: "user-avatar:gloopie-1", + }, + }; + mocks.listPersonaSources.mockResolvedValue([seededDraft]); + mocks.readAgentSourceFile.mockResolvedValue(seededDraft); + + await expect(hasAgentBuilderSessionUserContent("sess-1")).resolves.toBe( + false, + ); + }); + + it("isDiscardableAgentBuilderSession is true for drafts and missing files, false for existing agents", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( + true, + ); + + const existingAgent = { + ...draftSource, + name: "Spar", + properties: { draft: false }, + }; + mocks.listPersonaSources.mockResolvedValue([existingAgent]); + mocks.readAgentSourceFile.mockResolvedValue(existingAgent); + await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( + false, + ); + + mocks.listPersonaSources.mockResolvedValue([]); + mocks.readAgentSourceFile.mockRejectedValue(new Error("missing")); + await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( + true, + ); + }); + it("treats unsaved local edits as agent builder user content", async () => { addBuilderSession(); mocks.listPersonaSources.mockResolvedValue([draftSource]); diff --git a/src/features/agents/lib/agentBuilderIdentity.ts b/src/features/agents/lib/agentBuilderIdentity.ts index f96ea62cd..c27cc31f2 100644 --- a/src/features/agents/lib/agentBuilderIdentity.ts +++ b/src/features/agents/lib/agentBuilderIdentity.ts @@ -73,11 +73,14 @@ export function isPlaceholderDraftForSession( builderSessionId: string, ): boolean { const properties = source.properties ?? {}; + // Everything "New agent" seeds on its own — identity, the stored + // provider/model preference, and a starter avatar — is not user content. const extraPropertyKeys = Object.keys(properties).filter( (key) => key !== "draft" && key !== "builderSessionId" && key !== "provider" && + key !== "modelProviderId" && key !== "model" && key !== "avatar", ); diff --git a/src/features/agents/lib/agentBuilderSession.ts b/src/features/agents/lib/agentBuilderSession.ts index 277f55159..ff5a98797 100644 --- a/src/features/agents/lib/agentBuilderSession.ts +++ b/src/features/agents/lib/agentBuilderSession.ts @@ -517,6 +517,19 @@ export async function isDraftAgentBuilderSession( return source?.properties?.draft === true; } +/** + * True when leaving this builder session with no user content should simply + * discard it: it is a draft, or the agent file it pointed at no longer exists + * (moved or removed outside the app). Editing an existing, present agent is + * never discardable. + */ +export async function isDiscardableAgentBuilderSession( + sessionId: string, +): Promise { + const source = await findCurrentBuilderSource(sessionId); + return source === undefined || source.properties?.draft === true; +} + export async function reconcileAgentBuilderSessions(): Promise { const allSources = await listAgentBuilderSources(); const draftSources = allSources.filter( diff --git a/src/features/agents/lib/agentBuilderSourceLifecycle.ts b/src/features/agents/lib/agentBuilderSourceLifecycle.ts index 222bc095b..42edf0f83 100644 --- a/src/features/agents/lib/agentBuilderSourceLifecycle.ts +++ b/src/features/agents/lib/agentBuilderSourceLifecycle.ts @@ -133,7 +133,9 @@ export async function findAgentBuilderSource( sessionId: string, path: string, ): Promise { - const sources = await listAgentBuilderSources(); + const backendSources = await listPersonaSources(); + const backendPaths = new Set(backendSources.map((source) => source.path)); + const sources = mergeLocalDraftSources(backendSources); const foundByPath = sources.find((source) => source.path === path); const sessionMatches = sources.filter( (source) => source.properties?.builderSessionId === sessionId, @@ -143,12 +145,12 @@ export async function findAgentBuilderSource( ); if (foundByPath && !isEmptyPlaceholderDraft(foundByPath)) { - return readListedDraftFresh(foundByPath); + return readListedDraftFresh(foundByPath, backendPaths); } const listedSource = movedNonPlaceholder ?? foundByPath ?? sessionMatches[0]; if (listedSource) { - return readListedDraftFresh(listedSource); + return readListedDraftFresh(listedSource, backendPaths); } try { @@ -262,7 +264,8 @@ function isBuilderDraftProperties( async function readListedDraftFresh( source: AgentSourceEntry, -): Promise { + backendPaths: ReadonlySet, +): Promise { if (source.properties?.draft !== true) { return source; } @@ -270,6 +273,14 @@ async function readListedDraftFresh( try { return await readAgentSourceFile(source.path, source); } catch { + // A draft the backend still lists may be temporarily unreadable; keep the + // listed copy. A draft only the local cache remembers has no file behind + // it anymore (moved or removed outside the app), so forget it rather than + // hand back a stale entry that later delete/save calls will trip over. + if (!backendPaths.has(source.path)) { + localDraftSourcesByPath.delete(source.path); + return undefined; + } return source; } } diff --git a/src/features/agents/stores/agentStore.ts b/src/features/agents/stores/agentStore.ts index 511093b51..97c53dc92 100644 --- a/src/features/agents/stores/agentStore.ts +++ b/src/features/agents/stores/agentStore.ts @@ -1,6 +1,7 @@ import { create } from "zustand"; import type { Persona, Agent } from "@/shared/types/agents"; import type { AcpProvider } from "@/shared/api/acp"; +import type { AgentSourceEntry } from "@/shared/api/agents"; import { canEditPersona } from "@/features/agents/lib/personaPresentation"; const PROVIDER_STORAGE_KEY = "goose:defaultProvider"; @@ -36,6 +37,8 @@ interface AgentStoreState { // Personas personas: Persona[]; personasLoading: boolean; + // Builder drafts as listed on disk; the gallery's draft cards come from here. + draftSources: AgentSourceEntry[]; // Agents agents: Agent[]; @@ -62,6 +65,8 @@ interface AgentStoreActions { updatePersona: (id: string, updates: Partial) => void; removePersona: (id: string) => void; setPersonasLoading: (loading: boolean) => void; + setDraftSources: (drafts: AgentSourceEntry[]) => void; + removeDraftSource: (path: string) => void; // Agent CRUD setAgents: (agents: Agent[]) => void; @@ -96,6 +101,7 @@ export const useAgentStore = create((set, get) => ({ // State personas: [], personasLoading: false, + draftSources: [], agents: [], agentsLoading: false, providers: [], @@ -126,6 +132,13 @@ export const useAgentStore = create((set, get) => ({ setPersonasLoading: (personasLoading) => set({ personasLoading }), + setDraftSources: (draftSources) => set({ draftSources }), + + removeDraftSource: (path) => + set((state) => ({ + draftSources: state.draftSources.filter((draft) => draft.path !== path), + })), + // Agent CRUD setAgents: (agents) => set({ agents }), diff --git a/src/features/agents/ui/AgentsView.tsx b/src/features/agents/ui/AgentsView.tsx index 849f797f3..eab5e4336 100644 --- a/src/features/agents/ui/AgentsView.tsx +++ b/src/features/agents/ui/AgentsView.tsx @@ -53,7 +53,13 @@ import { trackAgentEditCompleted, } from "@/features/agents/lib/agentTelemetry"; import { runAgentViewTransition } from "@/features/agents/lib/agentViewTransitions"; -import { deleteDraftAgentSession } from "@/features/agents/lib/agentBuilderSession"; +import { + deleteDraftAgentSession, + fileStem, + isEmptyPlaceholderDraft, +} from "@/features/agents/lib/agentBuilderSession"; +import { discardAgentBuilderSource } from "@/features/agents/lib/agentBuilderSourceLifecycle"; +import type { GalleryDraft } from "@/features/agents/ui/PersonaGallery"; import type { AppNavigationUpdateOptions } from "@/app/types/appNavigation"; import { isSafePngAvatarDataUrl } from "@/shared/lib/avatarUrl"; import { @@ -158,16 +164,32 @@ export function AgentsView({ [storedPersonas], ); const sessions = useChatSessionStore((state) => state.sessions); - const agentDraftSessions = useMemo( + const draftSources = useAgentStore((state) => state.draftSources); + const removeDraftSource = useAgentStore((state) => state.removeDraftSource); + // Draft cards come from the files on disk, like every other card in the + // gallery. An untouched "New agent" placeholder isn't something the user + // made yet, so it earns no card. The builder chat, when one is still open, + // is secondary — it lets "Continue editing" land back in the same thread. + const agentDrafts = useMemo( () => - sessions.filter( - (session) => - session.intent === "build-agent" && - session.targetAgentDraftSaved === true && - !session.archivedAt && - Boolean(session.targetAgentPath), - ), - [sessions], + draftSources + .filter((source) => !isEmptyPlaceholderDraft(source)) + .map((source) => { + const builderSessionId = source.properties?.builderSessionId; + const session = sessions.find( + (candidate) => + candidate.intent === "build-agent" && + !candidate.archivedAt && + (candidate.targetAgentPath === source.path || + candidate.id === builderSessionId), + ); + return { + source, + sessionId: session?.id ?? null, + sessionTitle: session?.title ?? null, + }; + }), + [draftSources, sessions], ); const shouldReduceMotion = useReducedMotion(); // Four or fewer agents fit in a single screen, so we float the grid in the @@ -257,29 +279,34 @@ export function AgentsView({ }, [onStartAgentBuilderSession]); const handleContinueDraft = useCallback( - (sessionId: string) => { - const session = useChatSessionStore.getState().getSession(sessionId); - if (!session?.targetAgentPath) { - return; - } - + (draft: GalleryDraft) => { + // Starting by path reopens the live builder chat when there is one and + // otherwise opens a fresh builder on the same file. onStartAgentBuilderSession?.({ - path: session.targetAgentPath, - slug: session.targetAgentSlug ?? undefined, + path: draft.source.path, + slug: fileStem(draft.source.path) || undefined, }); }, [onStartAgentBuilderSession], ); const handleDeleteDraft = useCallback( - (sessionId: string) => { - void deleteDraftAgentSession(sessionId, { - closeSession: onDeleteDraftSession, - }).catch((error) => { - toast.error(formatAgentError(error, t("view.deleteFailed"))); - }); + (draft: GalleryDraft) => { + const { sessionId, source } = draft; + const deletion = sessionId + ? deleteDraftAgentSession(sessionId, { + closeSession: onDeleteDraftSession, + }) + : discardAgentBuilderSource(source.path); + void deletion + .then(() => { + removeDraftSource(source.path); + }) + .catch((error) => { + toast.error(formatAgentError(error, t("view.deleteFailed"))); + }); }, - [onDeleteDraftSession, t], + [onDeleteDraftSession, removeDraftSource, t], ); useEffect(() => { @@ -669,7 +696,7 @@ export function AgentsView({ > void; onStartChatPersona?: (persona: Persona) => void; @@ -50,8 +60,8 @@ interface PersonaGalleryProps { onCreatePersona: () => void; onImportAgentImage?: () => void; - onContinueDraft?: (sessionId: string) => void; - onDeleteDraft?: (sessionId: string) => void; + onContinueDraft?: (draft: GalleryDraft) => void; + onDeleteDraft?: (draft: GalleryDraft) => void; onImportFile?: (fileBytes: Uint8Array, fileName: string) => void; validateImportFile?: ( file: Pick, @@ -62,11 +72,11 @@ interface PersonaGalleryProps { isLoading?: boolean; } -function draftTitle(session: ChatSession, sourceName?: string): string { - const name = sourceName?.trim(); +function draftTitle(draft: GalleryDraft): string { + const name = draft.source.name.trim(); if (name && !isPlaceholderAgentName(name)) return name; - const title = session.title.trim(); + const title = draft.sessionTitle?.trim() ?? ""; return title.length > 0 ? title : "Untitled agent draft"; } @@ -81,25 +91,23 @@ function draftAvatar(sourceAvatar: unknown): string | null { } function PersonaDraftCard({ - session, + draft, onContinue, onDelete, }: { - session: ChatSession; - onContinue?: (sessionId: string) => void; - onDelete?: (sessionId: string) => void; + draft: GalleryDraft; + onContinue?: (draft: GalleryDraft) => void; + onDelete?: (draft: GalleryDraft) => void; }) { const { t } = useTranslation("agents"); const [readyAnimatedAvatarSrc, setReadyAnimatedAvatarSrc] = useState< string | null >(null); - const { data } = usePersonaSource(session.targetAgentPath ?? null, { - builderSessionId: session.id, - }); - const title = draftTitle(session, data?.name); + const { source } = draft; + const title = draftTitle(draft); const description = - draftDescription(data?.content) ?? t("gallery.draftDescription"); - const avatar = draftAvatar(data?.properties?.avatar); + draftDescription(source.content) ?? t("gallery.draftDescription"); + const avatar = draftAvatar(source.properties?.avatar); const avatarMedia = useAvatarMedia(avatar); const staticAvatarSrc = avatarMedia?.posterSrc ?? @@ -107,9 +115,7 @@ function PersonaDraftCard({ const animatedAvatarReady = avatarMedia?.mediaType === "video" && readyAnimatedAvatarSrc === avatarMedia.src; - const fallbackIconSrc = resolveAgentIcon( - session.targetAgentPath ?? session.id, - ); + const fallbackIconSrc = resolveAgentIcon(source.path); const hoverActionsOverlay = (
onContinue?.(session.id)} + onClick={() => onContinue?.(draft)} aria-label={t("gallery.continueDraftAria", { name: title })} className="pointer-events-auto" > @@ -133,7 +139,7 @@ function PersonaDraftCard({ variant="subtle" size="sm" destructive - onClick={() => onDelete?.(session.id)} + onClick={() => onDelete?.(draft)} aria-label={t("gallery.deleteDraftAria", { name: title })} className="pointer-events-auto" > @@ -216,7 +222,7 @@ function SkeletonCard() { export function PersonaGallery({ personas, - draftSessions = [], + drafts = [], activePersonaId, onSelectPersona, onStartChatPersona, @@ -306,7 +312,7 @@ export function PersonaGallery({ ); } - if (personas.length === 0 && draftSessions.length === 0) { + if (personas.length === 0 && drafts.length === 0) { return (
))} - {draftSessions.map((session, index) => ( + {drafts.map((draft, index) => (
diff --git a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx index 082137234..e68f32d7a 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -24,6 +24,7 @@ import { EXPERIMENT_PREFERENCES_STORAGE_VERSION, } from "@/features/experiments/experimentPreferences"; import { AVATAR_COLLECTION_PAGE_EXPERIMENT_ID } from "@/features/experiments/experimentDefinitions"; +import { placeholderAgentName } from "@/features/agents/lib/agentBuilderIdentity"; import { AgentsView } from "../AgentsView"; const mockCreatePersona = vi.hoisted(() => vi.fn()); @@ -32,7 +33,7 @@ const mockTrackAgentCreateCompleted = vi.hoisted(() => vi.fn()); const mockTrackAgentEditCompleted = vi.hoisted(() => vi.fn()); const mockDraftSource = vi.hoisted(() => ({ - type: "agent", + type: "agent" as const, path: "/Users/x/.agents/agents/draft-session.md", name: "New agent", description: "Draft", @@ -309,6 +310,7 @@ describe("AgentsView entry points", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], providers: [], }); useChatSessionStore.setState({ @@ -736,10 +738,13 @@ describe("AgentsView entry points", () => { expect(onStartAgentBuilderSession).toHaveBeenCalledWith({}); }); - it("shows draft sessions at the end of the gallery and continues or deletes them", async () => { + it("shows drafts from disk at the end of the gallery and continues or deletes them", async () => { const onStartAgentBuilderSession = vi.fn(); const onDeleteDraftSession = vi.fn(); - useAgentStore.setState({ personas: [persona] }); + useAgentStore.setState({ + personas: [persona], + draftSources: [mockDraftSource], + }); useChatSessionStore.setState({ sessions: [ { @@ -752,7 +757,6 @@ describe("AgentsView entry points", () => { targetAgentPath: "/Users/x/.agents/agents/draft-session.md", targetAgentSlug: "draft-session", targetAgentDraftState: null, - targetAgentDraftSaved: true, }, ], }); @@ -781,7 +785,60 @@ describe("AgentsView entry points", () => { screen.getByRole("button", { name: "gallery.deleteDraftAria" }), ); - expect(onDeleteDraftSession).toHaveBeenCalledWith("draft-session"); + await waitFor(() => { + expect(onDeleteDraftSession).toHaveBeenCalledWith("draft-session"); + }); + expect(useAgentStore.getState().draftSources).toEqual([]); + }); + + it("shows a draft whose builder chat is gone and deletes its file directly", async () => { + const onDeleteDraftSession = vi.fn(); + const { deletePersonaSource } = await import("@/shared/api/agents"); + useAgentStore.setState({ + personas: [persona], + draftSources: [mockDraftSource], + }); + useChatSessionStore.setState({ sessions: [] }); + + render(); + + expect(screen.getByText("gallery.draft")).toBeInTheDocument(); + + const user = userEvent.setup(); + await user.click( + screen.getByRole("button", { name: "gallery.deleteDraftAria" }), + ); + + await waitFor(() => { + expect(deletePersonaSource).toHaveBeenCalledWith(mockDraftSource.path); + }); + expect(onDeleteDraftSession).not.toHaveBeenCalled(); + expect(useAgentStore.getState().draftSources).toEqual([]); + }); + + it("does not show a card for an untouched New agent placeholder", () => { + useAgentStore.setState({ + personas: [persona], + draftSources: [ + { + ...mockDraftSource, + path: "/Users/x/.agents/agents/untitled-agent-1.md", + name: placeholderAgentName("draft-session"), + properties: { + draft: true, + builderSessionId: "draft-session", + provider: "claude-acp", + modelProviderId: "claude-acp", + model: "claude-sonnet-5", + avatar: "app-avatar:gloopies-1", + }, + }, + ], + }); + + render(); + + expect(screen.queryByText("gallery.draft")).not.toBeInTheDocument(); }); it("returns from the detail page to the agents gallery", () => { diff --git a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx index c122f43c4..f2879af3d 100644 --- a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx +++ b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx @@ -15,7 +15,13 @@ const mocks = vi.hoisted(() => ({ addPersona: vi.fn(), updatePersona: vi.fn(), personas: [] as Array<{ id: string }>, - listPersonas: vi.fn(), + draftSources: [] as Array<{ + path: string; + properties?: { builderSessionId?: string }; + }>, + setDraftSources: vi.fn(), + removeDraftSource: vi.fn(), + listAgentGallery: vi.fn(), recoverDraftAgent: vi.fn(), setAgentBuilderSessionLocalEdits: vi.fn(), setAgentBuilderSessionSaveHandler: vi.fn(), @@ -123,6 +129,9 @@ vi.mock("@/features/agents/stores/agentStore", () => ({ setPersonas: mocks.setPersonas, addPersona: mocks.addPersona, updatePersona: mocks.updatePersona, + draftSources: mocks.draftSources, + setDraftSources: mocks.setDraftSources, + removeDraftSource: mocks.removeDraftSource, }), }, })); @@ -141,7 +150,7 @@ vi.mock("@/shared/api/agents", () => ({ isBuiltin: false, writable: true, }), - listPersonas: () => mocks.listPersonas(), + listAgentGallery: () => mocks.listAgentGallery(), })); vi.mock("../../hooks/useGitStateAutoRefresh", () => ({ @@ -192,8 +201,11 @@ describe("ChatRightRail", () => { mocks.setPersonas.mockReset(); mocks.addPersona.mockReset(); mocks.updatePersona.mockReset(); - mocks.listPersonas.mockReset(); - mocks.listPersonas.mockResolvedValue([]); + mocks.listAgentGallery.mockReset(); + mocks.listAgentGallery.mockResolvedValue({ personas: [], drafts: [] }); + mocks.setDraftSources.mockReset(); + mocks.removeDraftSource.mockReset(); + mocks.draftSources = []; mocks.recoverDraftAgent.mockReset(); mocks.recoverDraftAgent.mockResolvedValue({ path: "/Users/x/.agents/agents/recovered.md", @@ -713,7 +725,10 @@ describe("ChatRightRail", () => { it("refreshes agents, closes the capability, and opens the saved agent when a draft is promoted", async () => { const personas = [{ id: "/path", displayName: "Snark" }]; const onAgentBuilderCompleted = vi.fn(); - mocks.listPersonas.mockResolvedValue(personas); + mocks.listAgentGallery.mockResolvedValue({ personas, drafts: [] }); + mocks.draftSources = [ + { path: "/draft-path", properties: { builderSessionId: "s1" } }, + ]; render( { expect.objectContaining({ id: "/path" }), ); expect(onAgentBuilderCompleted).toHaveBeenCalledWith("/path"); + // The promoted draft's card leaves the gallery immediately, then the + // disk refresh replaces both lists. + expect(mocks.removeDraftSource).toHaveBeenCalledWith("/draft-path"); await waitFor(() => { expect(mocks.setPersonas).toHaveBeenCalledWith(personas); }); + expect(mocks.setDraftSources).toHaveBeenCalledWith([]); }); it("opens the promoted agent even when refreshing agents fails", async () => { const onAgentBuilderCompleted = vi.fn(); - mocks.listPersonas.mockRejectedValue(new Error("refresh unavailable")); + mocks.listAgentGallery.mockRejectedValue(new Error("refresh unavailable")); const consoleError = vi .spyOn(console, "error") .mockImplementation(() => undefined); diff --git a/src/shared/api/agents.ts b/src/shared/api/agents.ts index 7ad86a4be..c2c74646d 100644 --- a/src/shared/api/agents.ts +++ b/src/shared/api/agents.ts @@ -839,10 +839,33 @@ export async function promotePersonaSource( return promoted; } +export interface AgentGalleryListing { + personas: Persona[]; + /** In-progress builder drafts, as they exist on disk right now. */ + drafts: AgentSourceEntry[]; +} + +/** + * Single read of the agent sources, split into finished agents and builder + * drafts. The gallery renders both from this one listing so a draft card can + * only exist while its file does — same as a finished agent. + */ +export async function listAgentGallery(): Promise { + const sources = await listAgentSources(); + const personas: Persona[] = []; + const drafts: AgentSourceEntry[] = []; + for (const source of sources) { + if (source.properties?.draft === true) { + drafts.push(source); + } else { + personas.push(agentSourceToPersona(source)); + } + } + return { personas, drafts }; +} + export async function listPersonas(): Promise { - return (await listAgentSources()) - .filter((source) => source.properties?.draft !== true) - .map(agentSourceToPersona); + return (await listAgentGallery()).personas; } export async function createPersona( @@ -949,6 +972,10 @@ export async function refreshPersonas(): Promise { return listPersonas(); } +export async function refreshAgentGallery(): Promise { + return listAgentGallery(); +} + export async function repairBundledAgent(fileName: string): Promise { await invoke("repair_bundled_agent", { fileName }); } From efb8993799dc4a637897199c76d8857514742afb Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:51:56 -0700 Subject: [PATCH 2/4] fix(agents): close the two draft-lifecycle races found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Navigation guard: the "is this draft untouched?" decision was made, then another lookup awaited, then the draft deleted — anything typed in that gap was discarded silently. `discardUntouchedDraftAgentSession` now owns re-check-then-discard: the user-content check is the last step before the file goes, and a draft that picked up content returns "kept" so the caller shows the save/discard prompt instead. Both the Back guard and the New agent button use it; `isDiscardableAgentBuilderSession` is folded in. Gallery refresh: `usePersonas` fenced stale disk listings behind a private mutation counter, but draft deletion in AgentsView and the promotion writes in AgentBuilderCapability bypassed it, so a focus/interval refresh that began before Delete could land afterwards and repaint the deleted card. The fence now lives in the agent store as `refreshGallery` / `mutateGallery`; every gallery writer goes through one of the two. Tests: deferred-lookup race for the guard (type while pending → kept, no delete), store fence semantics (stale snapshot dropped, in-flight mutation blocks apply, fence released on throw), and an AgentsView test where a refresh started before Delete resolves afterwards and the card stays gone. Co-Authored-By: Claude --- .../capabilities/AgentBuilderCapability.tsx | 46 +++++---- .../hooks/useAgentBuilderCoordinator.ts | 52 ++++------ src/features/agents/hooks/usePersonas.ts | 41 ++------ .../lib/__tests__/agentBuilderSession.test.ts | 99 ++++++++++++++----- .../agents/lib/agentBuilderSession.ts | 44 ++++++++- .../stores/__tests__/agentStore.test.ts | 96 ++++++++++++++++++ src/features/agents/stores/agentStore.ts | 44 ++++++++- src/features/agents/ui/AgentsView.tsx | 27 ++--- .../ui/__tests__/AgentsView.entry.test.tsx | 42 ++++++++ .../chat/ui/__tests__/ChatRightRail.test.tsx | 14 +++ 10 files changed, 378 insertions(+), 127 deletions(-) diff --git a/src/features/agents/capabilities/AgentBuilderCapability.tsx b/src/features/agents/capabilities/AgentBuilderCapability.tsx index 0cc0172b6..28e33271d 100644 --- a/src/features/agents/capabilities/AgentBuilderCapability.tsx +++ b/src/features/agents/capabilities/AgentBuilderCapability.tsx @@ -51,46 +51,44 @@ export function AgentBuilderCapability({ const { t } = useTranslation("agents"); const patchSession = useChatSessionStore((state) => state.patchSession); - const refreshPersonas = useCallback(async () => { - const { personas, drafts } = await listAgentGallery(); - const agentStore = useAgentStore.getState(); - agentStore.setPersonas(personas); - agentStore.setDraftSources(drafts); - }, []); - const completeBuilder = useCallback( (source: AgentSourceEntry, refreshErrorMessage: string) => { clearBuilderSessionState(session.id); // Promotion is the durable source of truth. Seed the store immediately // so the destination profile exists even if the follow-up disk refresh - // fails or has not observed the promoted source yet. + // fails or has not observed the promoted source yet. Running the writes + // as a gallery mutation fences out any disk refresh that started before + // the promotion and would otherwise repaint the draft card. const promotedPersona = agentSourceToPersona(source); const agentStore = useAgentStore.getState(); - const existingPersona = agentStore.personas.find( - (persona) => persona.id === promotedPersona.id, - ); - if (existingPersona) { - agentStore.updatePersona(promotedPersona.id, promotedPersona); - } else { - agentStore.addPersona(promotedPersona); - } - // The draft just became this agent; drop its card without waiting for - // the disk refresh so the gallery never shows both at once. - for (const draft of agentStore.draftSources) { - if (draft.properties?.builderSessionId === session.id) { - agentStore.removeDraftSource(draft.path); + void agentStore.mutateGallery(() => { + const current = useAgentStore.getState(); + const existingPersona = current.personas.find( + (persona) => persona.id === promotedPersona.id, + ); + if (existingPersona) { + current.updatePersona(promotedPersona.id, promotedPersona); + } else { + current.addPersona(promotedPersona); } - } + // The draft just became this agent; drop its card without waiting + // for the disk refresh so the gallery never shows both at once. + for (const draft of current.draftSources) { + if (draft.properties?.builderSessionId === session.id) { + current.removeDraftSource(draft.path); + } + } + }); onDraftPromoted?.(source); onAgentBuilderCompleted?.(promotedPersona.id); - void refreshPersonas().catch((error) => { + void agentStore.refreshGallery(listAgentGallery).catch((error) => { console.error(refreshErrorMessage, error); }); }, - [onAgentBuilderCompleted, onDraftPromoted, refreshPersonas, session.id], + [onAgentBuilderCompleted, onDraftPromoted, session.id], ); const handleDraftPromoted = useCallback( diff --git a/src/features/agents/hooks/useAgentBuilderCoordinator.ts b/src/features/agents/hooks/useAgentBuilderCoordinator.ts index 456a144e5..7aa983cc5 100644 --- a/src/features/agents/hooks/useAgentBuilderCoordinator.ts +++ b/src/features/agents/hooks/useAgentBuilderCoordinator.ts @@ -5,8 +5,7 @@ import { useChatSessionStore } from "@/features/chat/stores/chatSessionStore"; import type { AgentBuilderLeaveDraftDialogProps } from "../ui/AgentBuilderLeaveDraftDialog"; import { discardDraftAgentSession, - hasAgentBuilderSessionUserContent, - isDiscardableAgentBuilderSession, + discardUntouchedDraftAgentSession, reconcileAgentBuilderSessions, resolveAgentBuilderSessionId, saveDraftAgentSession, @@ -127,27 +126,20 @@ export function useAgentBuilderCoordinator({ } void (async () => { - const hasUserContent = await hasAgentBuilderSessionUserContent( - session.id, - ); - if (!hasUserContent) { - // Nothing was made here. An untouched "New agent" draft leaves no - // trace — no prompt, no file, no empty chat. Editing an existing - // agent without changes just navigates away. - const discardable = await isDiscardableAgentBuilderSession( - session.id, - ); - // Navigate first so the empty chat is no longer the active session - // when it closes; closing the active chat would redirect home and - // stomp on where the user was actually going. + // An untouched "New agent" draft leaves no trace — no prompt, no + // file, no empty chat. The helper re-checks for user content right + // before deleting, so a word typed while we were looking keeps the + // draft and gets the prompt instead. + const outcome = await discardUntouchedDraftAgentSession(session.id, { + closeSession, + onBeforeDiscard: next, + }); + if (outcome === "discarded") { + return; + } + if (outcome === "nothing-to-discard") { + // Editing an existing agent without changes just navigates away. next(); - if (discardable) { - await discardDraftAgentSession(session.id, { closeSession }).catch( - (error) => { - console.error("Failed to discard empty agent draft:", error); - }, - ); - } return; } @@ -198,18 +190,10 @@ export function useAgentBuilderCoordinator({ } void (async () => { - const isDiscardable = await isDiscardableAgentBuilderSession( - session.id, - ); - if ( - isDiscardable && - !(await hasAgentBuilderSessionUserContent(session.id)) - ) { - await discardDraftAgentSession(session.id, { closeSession }).catch( - (error) => { - console.error("Failed to discard empty agent draft:", error); - }, - ); + const outcome = await discardUntouchedDraftAgentSession(session.id, { + closeSession, + }); + if (outcome === "discarded") { startBuilderSession(); return; } diff --git a/src/features/agents/hooks/usePersonas.ts b/src/features/agents/hooks/usePersonas.ts index 72a0a8e3f..22fafa47f 100644 --- a/src/features/agents/hooks/usePersonas.ts +++ b/src/features/agents/hooks/usePersonas.ts @@ -16,16 +16,14 @@ const REFRESH_INTERVAL_MS = 60_000; export function usePersonas() { const personas = useAgentStore(selectPersonas); const personasLoading = useAgentStore(selectPersonasLoading); - const setPersonas = useAgentStore((s) => s.setPersonas); - const setDraftSources = useAgentStore((s) => s.setDraftSources); + const refreshGallery = useAgentStore((s) => s.refreshGallery); + const mutateGallery = useAgentStore((s) => s.mutateGallery); const addPersona = useAgentStore((s) => s.addPersona); const updatePersonaInStore = useAgentStore((s) => s.updatePersona); const removePersona = useAgentStore((s) => s.removePersona); const setPersonasLoading = useAgentStore((s) => s.setPersonasLoading); const refreshTimerRef = useRef | null>(null); const listRequestInFlightRef = useRef(false); - const mutationVersionRef = useRef(0); - const mutationsInFlightRef = useRef(0); const replacePersonasFromApi = useCallback( async ( @@ -37,20 +35,12 @@ export function usePersonas() { } listRequestInFlightRef.current = true; - const mutationVersionAtStart = mutationVersionRef.current; if (options.showLoading) { setPersonasLoading(true); } try { - const { personas, drafts } = await fetchGallery(); - if ( - mutationVersionAtStart === mutationVersionRef.current && - mutationsInFlightRef.current === 0 - ) { - setPersonas(personas); - setDraftSources(drafts); - } + await refreshGallery(fetchGallery); } catch (error) { console.error(options.errorMessage, error); } finally { @@ -60,20 +50,9 @@ export function usePersonas() { } } }, - [setDraftSources, setPersonas, setPersonasLoading], + [refreshGallery, setPersonasLoading], ); - const trackMutation = useCallback(async (mutation: () => Promise) => { - mutationVersionRef.current += 1; - mutationsInFlightRef.current += 1; - try { - return await mutation(); - } finally { - mutationsInFlightRef.current -= 1; - mutationVersionRef.current += 1; - } - }, []); - const loadPersonas = useCallback(async () => { await replacePersonasFromApi(api.listAgentGallery, { showLoading: true, @@ -111,11 +90,11 @@ export function usePersonas() { const createPersona = useCallback( async (req: CreatePersonaRequest) => { - const persona = await trackMutation(() => api.createPersona(req)); + const persona = await mutateGallery(() => api.createPersona(req)); addPersona(persona); return persona; }, - [addPersona, trackMutation], + [addPersona, mutateGallery], ); // Custom gloopies are library citizens, not per-agent attachments: a @@ -125,21 +104,21 @@ export function usePersonas() { // happens here. const updatePersona = useCallback( async (existing: Persona, req: UpdatePersonaRequest) => { - const persona = await trackMutation(() => + const persona = await mutateGallery(() => api.updatePersona(existing, req), ); updatePersonaInStore(existing.id, persona); return persona; }, - [trackMutation, updatePersonaInStore], + [mutateGallery, updatePersonaInStore], ); const deletePersona = useCallback( async (id: string) => { - await trackMutation(() => api.deletePersona(id)); + await mutateGallery(() => api.deletePersona(id)); removePersona(id); }, - [removePersona, trackMutation], + [mutateGallery, removePersona], ); return { diff --git a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts index 524bc9d3d..b3f0ce451 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -101,8 +101,8 @@ vi.mock("@/features/runtime-config/defaults", () => ({ import { deleteDraftAgentSession, discardDraftAgentSession, + discardUntouchedDraftAgentSession, hasAgentBuilderSessionUserContent, - isDiscardableAgentBuilderSession, isEmptyDraftAgentSession, promoteDraft, recoverDraftAgent, @@ -785,29 +785,84 @@ describe("agentBuilderSession", () => { ); }); - it("isDiscardableAgentBuilderSession is true for drafts and missing files, false for existing agents", async () => { - addBuilderSession(); - mocks.listPersonaSources.mockResolvedValue([draftSource]); - await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( - true, - ); + describe("discardUntouchedDraftAgentSession", () => { + it("discards an untouched draft, navigating before the chat closes", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + mocks.deletePersonaSource.mockResolvedValue(undefined); + const order: string[] = []; + const onBeforeDiscard = vi.fn(() => order.push("navigate")); + const close = vi.fn(async () => { + order.push("close"); + }); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { + closeSession: close, + onBeforeDiscard, + }), + ).resolves.toBe("discarded"); - const existingAgent = { - ...draftSource, - name: "Spar", - properties: { draft: false }, - }; - mocks.listPersonaSources.mockResolvedValue([existingAgent]); - mocks.readAgentSourceFile.mockResolvedValue(existingAgent); - await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( - false, - ); + expect(mocks.deletePersonaSource).toHaveBeenCalledWith(draftSource.path); + expect(close).toHaveBeenCalledWith("sess-1"); + expect(order).toEqual(["navigate", "close"]); + }); - mocks.listPersonaSources.mockResolvedValue([]); - mocks.readAgentSourceFile.mockRejectedValue(new Error("missing")); - await expect(isDiscardableAgentBuilderSession("sess-1")).resolves.toBe( - true, - ); + it("keeps the draft when the user types while the lookup is in flight", async () => { + addBuilderSession(); + let releaseLookup: (sources: (typeof draftSource)[]) => void = () => {}; + mocks.listPersonaSources.mockImplementation( + () => + new Promise<(typeof draftSource)[]>((resolve) => { + releaseLookup = resolve; + }), + ); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + const onBeforeDiscard = vi.fn(); + + const pending = discardUntouchedDraftAgentSession("sess-1", { + closeSession, + onBeforeDiscard, + }); + // The decision has not been made yet; the user starts typing. + chatState.draftsBySession = { "sess-1": "make it a code reviewer" }; + releaseLookup([draftSource]); + + await expect(pending).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(onBeforeDiscard).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + + it("reports nothing to discard when editing an existing agent without changes", async () => { + addBuilderSession(); + const existingAgent = { + ...draftSource, + name: "Spar", + properties: { draft: false }, + }; + mocks.listPersonaSources.mockResolvedValue([existingAgent]); + mocks.readAgentSourceFile.mockResolvedValue(existingAgent); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("nothing-to-discard"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + + it("closes the empty chat when the draft file is already gone", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([]); + mocks.readAgentSourceFile.mockRejectedValue(new Error("missing")); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("discarded"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).toHaveBeenCalledWith("sess-1"); + }); }); it("treats unsaved local edits as agent builder user content", async () => { diff --git a/src/features/agents/lib/agentBuilderSession.ts b/src/features/agents/lib/agentBuilderSession.ts index ff5a98797..af9e60b4c 100644 --- a/src/features/agents/lib/agentBuilderSession.ts +++ b/src/features/agents/lib/agentBuilderSession.ts @@ -523,11 +523,49 @@ export async function isDraftAgentBuilderSession( * (moved or removed outside the app). Editing an existing, present agent is * never discardable. */ -export async function isDiscardableAgentBuilderSession( +export type UntouchedDraftDiscardOutcome = + | "discarded" + | "kept" + | "nothing-to-discard"; + +/** + * Discards the session's draft only if it is still untouched at the moment of + * deletion. The user-content check is the last thing that runs before the + * file goes, so anything typed while earlier lookups were in flight keeps the + * draft ("kept") instead of being silently thrown away. Editing an existing + * agent is never discardable; with no content it reports "nothing-to-discard" + * so callers can navigate freely. + * + * `onBeforeDiscard` runs once the decision is final and before the chat + * closes — callers navigate there, because closing the active chat redirects + * home and would stomp on where the user was going. + */ +export async function discardUntouchedDraftAgentSession( sessionId: string, -): Promise { + deps: CloseSessionDeps & { onBeforeDiscard?: () => void } = {}, +): Promise { const source = await findCurrentBuilderSource(sessionId); - return source === undefined || source.properties?.draft === true; + const isDraft = source === undefined || source.properties?.draft === true; + + if (await hasAgentBuilderSessionUserContent(sessionId)) { + return "kept"; + } + if (!isDraft) { + return "nothing-to-discard"; + } + + deps.onBeforeDiscard?.(); + try { + if (source) { + await discardAgentBuilderSource(source.path); + } + } catch (error) { + console.warn("Failed to delete agent builder draft during discard:", error); + } finally { + clearBuilderSessionState(sessionId); + await deps.closeSession?.(sessionId); + } + return "discarded"; } export async function reconcileAgentBuilderSessions(): Promise { diff --git a/src/features/agents/stores/__tests__/agentStore.test.ts b/src/features/agents/stores/__tests__/agentStore.test.ts index b10c56287..894b6e32a 100644 --- a/src/features/agents/stores/__tests__/agentStore.test.ts +++ b/src/features/agents/stores/__tests__/agentStore.test.ts @@ -1,6 +1,10 @@ import { afterEach, describe, it, expect, beforeEach } from "vitest"; import { useAgentStore } from "../agentStore"; import type { Persona, Agent } from "@/shared/types/agents"; +import type { + AgentGalleryListing, + AgentSourceEntry, +} from "@/shared/api/agents"; // ── fixtures ────────────────────────────────────────────────────────── @@ -39,6 +43,9 @@ describe("agentStore", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, agents: [], agentsLoading: false, activeAgentId: null, @@ -175,6 +182,95 @@ describe("agentStore", () => { expect(custom).toHaveLength(1); expect(custom[0].id).toBe("c"); }); + + // ── gallery fence ───────────────────────────────────────────────── + + describe("gallery fence", () => { + const draft: AgentSourceEntry = { + type: "agent", + path: "/agents/draft.md", + name: "Untitled agent", + description: "Draft", + content: "", + properties: { draft: true }, + writable: true, + global: true, + }; + + function deferredListing() { + let resolve: (listing: AgentGalleryListing) => void = () => {}; + const promise = new Promise((r) => { + resolve = r; + }); + return { fetch: () => promise, resolve }; + } + + it("applies a snapshot when nothing changed while it was in flight", async () => { + const listing = deferredListing(); + const pending = useAgentStore.getState().refreshGallery(listing.fetch); + listing.resolve({ + personas: [makePersona({ id: "p1" })], + drafts: [draft], + }); + + await expect(pending).resolves.toBe(true); + expect(useAgentStore.getState().personas.map((p) => p.id)).toEqual([ + "p1", + ]); + expect(useAgentStore.getState().draftSources).toEqual([draft]); + }); + + it("drops a snapshot that started before a mutation and resolved after it", async () => { + useAgentStore.setState({ draftSources: [draft] }); + const stale = deferredListing(); + const pending = useAgentStore.getState().refreshGallery(stale.fetch); + + // The user deletes the draft while the refresh is still in flight. + await useAgentStore.getState().mutateGallery(async () => { + useAgentStore.getState().removeDraftSource(draft.path); + }); + expect(useAgentStore.getState().draftSources).toEqual([]); + + // The old photo arrives, still showing the draft. It must not win. + stale.resolve({ personas: [], drafts: [draft] }); + await expect(pending).resolves.toBe(false); + expect(useAgentStore.getState().draftSources).toEqual([]); + }); + + it("drops a snapshot that resolves while a mutation is still in flight", async () => { + const listing = deferredListing(); + const pending = useAgentStore.getState().refreshGallery(listing.fetch); + + let finishMutation: () => void = () => {}; + const mutation = useAgentStore.getState().mutateGallery( + () => + new Promise((r) => { + finishMutation = r; + }), + ); + listing.resolve({ personas: [], drafts: [draft] }); + await expect(pending).resolves.toBe(false); + expect(useAgentStore.getState().draftSources).toEqual([]); + + finishMutation(); + await mutation; + expect(useAgentStore.getState().galleryMutationsInFlight).toBe(0); + }); + + it("releases the fence when a mutation throws", async () => { + await expect( + useAgentStore.getState().mutateGallery(async () => { + throw new Error("delete failed"); + }), + ).rejects.toThrow("delete failed"); + expect(useAgentStore.getState().galleryMutationsInFlight).toBe(0); + + const listing = deferredListing(); + const pending = useAgentStore.getState().refreshGallery(listing.fetch); + listing.resolve({ personas: [], drafts: [draft] }); + await expect(pending).resolves.toBe(true); + }); + }); }); describe("agentStore.setProviders", () => { diff --git a/src/features/agents/stores/agentStore.ts b/src/features/agents/stores/agentStore.ts index 97c53dc92..d1c56df53 100644 --- a/src/features/agents/stores/agentStore.ts +++ b/src/features/agents/stores/agentStore.ts @@ -1,7 +1,10 @@ import { create } from "zustand"; import type { Persona, Agent } from "@/shared/types/agents"; import type { AcpProvider } from "@/shared/api/acp"; -import type { AgentSourceEntry } from "@/shared/api/agents"; +import type { + AgentGalleryListing, + AgentSourceEntry, +} from "@/shared/api/agents"; import { canEditPersona } from "@/features/agents/lib/personaPresentation"; const PROVIDER_STORAGE_KEY = "goose:defaultProvider"; @@ -39,6 +42,11 @@ interface AgentStoreState { personasLoading: boolean; // Builder drafts as listed on disk; the gallery's draft cards come from here. draftSources: AgentSourceEntry[]; + // Gallery fence. A disk snapshot is only applied if no gallery mutation + // started or finished while it was in flight, so a slow refresh can never + // resurrect something the user just deleted or promoted. + galleryRevision: number; + galleryMutationsInFlight: number; // Agents agents: Agent[]; @@ -67,6 +75,12 @@ interface AgentStoreActions { setPersonasLoading: (loading: boolean) => void; setDraftSources: (drafts: AgentSourceEntry[]) => void; removeDraftSource: (path: string) => void; + // Every writer of the gallery goes through one of these two. Direct + // setPersonas/setDraftSources calls from a disk listing bypass the fence. + refreshGallery: ( + fetchGallery: () => Promise, + ) => Promise; + mutateGallery: (work: () => Promise | T) => Promise; // Agent CRUD setAgents: (agents: Agent[]) => void; @@ -102,6 +116,8 @@ export const useAgentStore = create((set, get) => ({ personas: [], personasLoading: false, draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, agents: [], agentsLoading: false, providers: [], @@ -139,6 +155,32 @@ export const useAgentStore = create((set, get) => ({ draftSources: state.draftSources.filter((draft) => draft.path !== path), })), + refreshGallery: async (fetchGallery) => { + const revisionAtStart = get().galleryRevision; + const { personas, drafts } = await fetchGallery(); + const { galleryRevision, galleryMutationsInFlight } = get(); + if (revisionAtStart !== galleryRevision || galleryMutationsInFlight !== 0) { + return false; + } + set({ personas, draftSources: drafts }); + return true; + }, + + mutateGallery: async (work) => { + set((state) => ({ + galleryRevision: state.galleryRevision + 1, + galleryMutationsInFlight: state.galleryMutationsInFlight + 1, + })); + try { + return await work(); + } finally { + set((state) => ({ + galleryRevision: state.galleryRevision + 1, + galleryMutationsInFlight: state.galleryMutationsInFlight - 1, + })); + } + }, + // Agent CRUD setAgents: (agents) => set({ agents }), diff --git a/src/features/agents/ui/AgentsView.tsx b/src/features/agents/ui/AgentsView.tsx index eab5e4336..de6bfd46e 100644 --- a/src/features/agents/ui/AgentsView.tsx +++ b/src/features/agents/ui/AgentsView.tsx @@ -166,6 +166,7 @@ export function AgentsView({ const sessions = useChatSessionStore((state) => state.sessions); const draftSources = useAgentStore((state) => state.draftSources); const removeDraftSource = useAgentStore((state) => state.removeDraftSource); + const mutateGallery = useAgentStore((state) => state.mutateGallery); // Draft cards come from the files on disk, like every other card in the // gallery. An untouched "New agent" placeholder isn't something the user // made yet, so it earns no card. The builder chat, when one is still open, @@ -293,20 +294,22 @@ export function AgentsView({ const handleDeleteDraft = useCallback( (draft: GalleryDraft) => { const { sessionId, source } = draft; - const deletion = sessionId - ? deleteDraftAgentSession(sessionId, { + // Run as a gallery mutation so a disk refresh that started before the + // delete cannot land afterwards and put the card back. + void mutateGallery(async () => { + if (sessionId) { + await deleteDraftAgentSession(sessionId, { closeSession: onDeleteDraftSession, - }) - : discardAgentBuilderSource(source.path); - void deletion - .then(() => { - removeDraftSource(source.path); - }) - .catch((error) => { - toast.error(formatAgentError(error, t("view.deleteFailed"))); - }); + }); + } else { + await discardAgentBuilderSource(source.path); + } + removeDraftSource(source.path); + }).catch((error) => { + toast.error(formatAgentError(error, t("view.deleteFailed"))); + }); }, - [onDeleteDraftSession, removeDraftSource, t], + [mutateGallery, onDeleteDraftSession, removeDraftSource, t], ); useEffect(() => { diff --git a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx index e68f32d7a..9eb98300a 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -311,6 +311,8 @@ describe("AgentsView entry points", () => { personas: [], personasLoading: false, draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -816,6 +818,46 @@ describe("AgentsView entry points", () => { expect(useAgentStore.getState().draftSources).toEqual([]); }); + it("does not let a disk refresh that started before Delete put the card back", async () => { + const { deletePersonaSource } = await import("@/shared/api/agents"); + useAgentStore.setState({ + personas: [persona], + draftSources: [mockDraftSource], + }); + useChatSessionStore.setState({ sessions: [] }); + + // A focus/interval refresh photographs the folder with the draft still in + // it, but the answer is slow to come back. + let resolveRefresh: (listing: { + personas: (typeof persona)[]; + drafts: (typeof mockDraftSource)[]; + }) => void = () => {}; + const staleRefresh = useAgentStore.getState().refreshGallery( + () => + new Promise((resolve) => { + resolveRefresh = resolve; + }), + ); + + render(); + const user = userEvent.setup(); + await user.click( + screen.getByRole("button", { name: "gallery.deleteDraftAria" }), + ); + await waitFor(() => { + expect(deletePersonaSource).toHaveBeenCalledWith(mockDraftSource.path); + }); + await waitFor(() => { + expect(screen.queryByText("gallery.draft")).not.toBeInTheDocument(); + }); + + // The old photo arrives after the delete. It must be ignored. + resolveRefresh({ personas: [persona], drafts: [mockDraftSource] }); + await expect(staleRefresh).resolves.toBe(false); + expect(useAgentStore.getState().draftSources).toEqual([]); + expect(screen.queryByText("gallery.draft")).not.toBeInTheDocument(); + }); + it("does not show a card for an untouched New agent placeholder", () => { useAgentStore.setState({ personas: [persona], diff --git a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx index f2879af3d..82b93254f 100644 --- a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx +++ b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx @@ -132,6 +132,20 @@ vi.mock("@/features/agents/stores/agentStore", () => ({ draftSources: mocks.draftSources, setDraftSources: mocks.setDraftSources, removeDraftSource: mocks.removeDraftSource, + // Mirror the real store's fence shape: mutations run their work + // synchronously up to the first await; refreshes apply the listing. + mutateGallery: async (work: () => Promise | T) => work(), + refreshGallery: async ( + fetchGallery: () => Promise<{ + personas: Array<{ id: string }>; + drafts: Array<{ path: string }>; + }>, + ) => { + const { personas, drafts } = await fetchGallery(); + mocks.setPersonas(personas); + mocks.setDraftSources(drafts); + return true; + }, }), }, })); From 712643969d80371064b63b5629de72cc8d774d08 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 23 Aug 2026 17:30:24 -0700 Subject: [PATCH 3/4] fix(agents): close the last async gap before discard; sequence promotion refresh Review of efb89937 found two gaps in the race fixes. The "final" user-content check still awaited a disk read after its in-memory look, so text typed during that read was invisible to it and the draft was still deleted. The in-memory look is now its own synchronous helper (`hasLocalAgentBuilderUserContent`) and `discardUntouchedDraftAgentSession` runs it once more with no await between it and the delete. Test holds the read inside the content check, types during it, and asserts "kept" with no delete/navigate/close; it fails without the re-check. `completeBuilder` started its disk refresh before the seeding mutation had released the gallery fence, so the fence (correctly) dropped every post-promotion refresh and the gallery stayed on the optimistic copy until the next timed refresh. The refresh now chains after the mutation. A capability test drives a real save through the real store and asserts the listing from disk is applied; the ChatRightRail store mock now models the fence instead of always applying, so it can no longer mask ordering bugs. Co-Authored-By: Claude --- .../capabilities/AgentBuilderCapability.tsx | 12 +++-- .../__tests__/AgentBuilderCapability.test.tsx | 50 ++++++++++++++++++- .../lib/__tests__/agentBuilderSession.test.ts | 37 ++++++++++++++ .../agents/lib/agentBuilderSession.ts | 48 +++++++++++------- .../chat/ui/__tests__/ChatRightRail.test.tsx | 27 ++++++++-- 5 files changed, 147 insertions(+), 27 deletions(-) diff --git a/src/features/agents/capabilities/AgentBuilderCapability.tsx b/src/features/agents/capabilities/AgentBuilderCapability.tsx index 28e33271d..b736d5253 100644 --- a/src/features/agents/capabilities/AgentBuilderCapability.tsx +++ b/src/features/agents/capabilities/AgentBuilderCapability.tsx @@ -62,7 +62,7 @@ export function AgentBuilderCapability({ // the promotion and would otherwise repaint the draft card. const promotedPersona = agentSourceToPersona(source); const agentStore = useAgentStore.getState(); - void agentStore.mutateGallery(() => { + const seeded = agentStore.mutateGallery(() => { const current = useAgentStore.getState(); const existingPersona = current.personas.find( (persona) => persona.id === promotedPersona.id, @@ -84,9 +84,13 @@ export function AgentBuilderCapability({ onDraftPromoted?.(source); onAgentBuilderCompleted?.(promotedPersona.id); - void agentStore.refreshGallery(listAgentGallery).catch((error) => { - console.error(refreshErrorMessage, error); - }); + // The refresh must start after the mutation releases the fence, or the + // fence would (correctly) reject it as having begun mid-mutation. + void seeded + .then(() => agentStore.refreshGallery(listAgentGallery)) + .catch((error) => { + console.error(refreshErrorMessage, error); + }); }, [onAgentBuilderCompleted, onDraftPromoted, session.id], ); diff --git a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx index 6dc891cfc..2139a5c9f 100644 --- a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx +++ b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, screen } from "@testing-library/react"; +import { act, fireEvent, screen, waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { renderWithProviders } from "@/test/render"; @@ -24,7 +24,13 @@ const apiMocks = vi.hoisted(() => ({ }, })); -vi.mock("@/shared/api/agents", () => apiMocks); +vi.mock("@/shared/api/agents", async (importOriginal) => ({ + ...apiMocks, + // Pure mapper; the real one keeps the promotion path honest. + agentSourceToPersona: ( + await importOriginal() + ).agentSourceToPersona, +})); vi.mock("@/features/agents/lib/agentTelemetry", () => telemetryMocks); @@ -64,6 +70,7 @@ import { import { setExperimentEnabled } from "@/features/experiments/experimentPreferences"; import { AVATAR_COLLECTION_PAGE_EXPERIMENT_ID } from "@/features/experiments/experimentDefinitions"; import type { AgentSourceEntry } from "@/shared/api/agents"; +import type { Persona } from "@/shared/types/agents"; const existingAgentSource: AgentSourceEntry = { type: "agent", @@ -129,6 +136,9 @@ describe("AgentBuilderCapability keep-save telemetry", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -176,4 +186,40 @@ describe("AgentBuilderCapability keep-save telemetry", () => { expect(telemetryMocks.trackAgentEditCompleted).not.toHaveBeenCalled(); expect(telemetryMocks.trackAgentCreateCompleted).not.toHaveBeenCalled(); }); + + it("applies the disk refresh that follows a save, through the real gallery fence", async () => { + // The optimistic store seed runs as a gallery mutation; the follow-up + // listing must start after that mutation releases the fence, or the fence + // would reject it and the gallery would stay on the optimistic copy. + const fromDisk: Persona = { + id: existingAgentSource.path, + displayName: "Code Reviewer (as listed on disk)", + systemPrompt: existingAgentSource.content, + isBuiltin: false, + writable: true, + createdAt: "2026-06-09T00:00:00.000Z", + updatedAt: "2026-06-09T00:00:00.000Z", + }; + apiMocks.listAgentGallery.mockResolvedValue({ + personas: [fromDisk], + drafts: [], + }); + + renderWithProviders( + , + ); + await screen.findByLabelText(/agent name/i); + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + + await waitFor(() => { + expect(apiMocks.listAgentGallery).toHaveBeenCalledTimes(1); + }); + await waitFor(() => { + expect(useAgentStore.getState().personas).toEqual([fromDisk]); + }); + expect(useAgentStore.getState().galleryMutationsInFlight).toBe(0); + }); }); diff --git a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts index b3f0ce451..bc4e4ba8d 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -835,6 +835,43 @@ describe("agentBuilderSession", () => { expect(closeSession).not.toHaveBeenCalled(); }); + it("keeps the draft when the user types during the final disk read", async () => { + // The source lookup completes, then the content check reads the file. + // Typing during that read must still be seen before anything is deleted. + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + let releaseRead: (source: typeof draftSource) => void = () => {}; + let reads = 0; + mocks.readAgentSourceFile.mockImplementation(() => { + reads += 1; + if (reads !== 2) { + // The helper's own lookup (read 1) and the content check's final + // fresh read (read 3) resolve right away. + return Promise.resolve(draftSource); + } + // The content check's lookup, after its in-memory look: hold it. + return new Promise((resolve) => { + releaseRead = resolve; + }); + }); + const onBeforeDiscard = vi.fn(); + + const pending = discardUntouchedDraftAgentSession("sess-1", { + closeSession, + onBeforeDiscard, + }); + await vi.waitFor(() => { + expect(reads).toBe(2); + }); + chatState.draftsBySession = { "sess-1": "make it a code reviewer" }; + releaseRead(draftSource); + + await expect(pending).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(onBeforeDiscard).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + it("reports nothing to discard when editing an existing agent without changes", async () => { addBuilderSession(); const existingAgent = { diff --git a/src/features/agents/lib/agentBuilderSession.ts b/src/features/agents/lib/agentBuilderSession.ts index af9e60b4c..b72464db6 100644 --- a/src/features/agents/lib/agentBuilderSession.ts +++ b/src/features/agents/lib/agentBuilderSession.ts @@ -458,9 +458,12 @@ export async function isEmptyDraftAgentSession( return isEmptyPlaceholderDraft(freshSource); } -export async function hasAgentBuilderSessionUserContent( - sessionId: string, -): Promise { +/** + * The in-memory half of the user-content check: unsaved rail edits, composer + * text, queued messages, sent messages. Synchronous on purpose — callers that + * are about to delete something re-run this with no await in between. + */ +export function hasLocalAgentBuilderUserContent(sessionId: string): boolean { if (localEditSessionIds.has(sessionId)) { return true; } @@ -479,19 +482,22 @@ export async function hasAgentBuilderSessionUserContent( return true; } - const hasUserMessage = (chatState.messagesBySession[sessionId] ?? []).some( - (message) => { - if (message.role !== "user" || message.metadata?.userVisible === false) { - return false; - } + return (chatState.messagesBySession[sessionId] ?? []).some((message) => { + if (message.role !== "user" || message.metadata?.userVisible === false) { + return false; + } - return ( - getTextContent(message).trim().length > 0 || - (message.metadata?.attachments?.length ?? 0) > 0 - ); - }, - ); - if (hasUserMessage) { + return ( + getTextContent(message).trim().length > 0 || + (message.metadata?.attachments?.length ?? 0) > 0 + ); + }); +} + +export async function hasAgentBuilderSessionUserContent( + sessionId: string, +): Promise { + if (hasLocalAgentBuilderUserContent(sessionId)) { return true; } @@ -530,9 +536,9 @@ export type UntouchedDraftDiscardOutcome = /** * Discards the session's draft only if it is still untouched at the moment of - * deletion. The user-content check is the last thing that runs before the - * file goes, so anything typed while earlier lookups were in flight keeps the - * draft ("kept") instead of being silently thrown away. Editing an existing + * deletion. The last thing before the file goes is a synchronous look at the + * in-memory user state, so anything typed while any lookup was in flight + * keeps the draft ("kept") instead of being silently thrown away. Editing an existing * agent is never discardable; with no content it reports "nothing-to-discard" * so callers can navigate freely. * @@ -550,6 +556,12 @@ export async function discardUntouchedDraftAgentSession( if (await hasAgentBuilderSessionUserContent(sessionId)) { return "kept"; } + // The check above awaited a disk read after its in-memory look. Anything + // typed during that read is invisible to it, so look once more — with no + // await between here and the delete. + if (hasLocalAgentBuilderUserContent(sessionId)) { + return "kept"; + } if (!isDraft) { return "nothing-to-discard"; } diff --git a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx index 82b93254f..b6c977448 100644 --- a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx +++ b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx @@ -21,6 +21,8 @@ const mocks = vi.hoisted(() => ({ }>, setDraftSources: vi.fn(), removeDraftSource: vi.fn(), + galleryRevision: 0, + galleryMutationsInFlight: 0, listAgentGallery: vi.fn(), recoverDraftAgent: vi.fn(), setAgentBuilderSessionLocalEdits: vi.fn(), @@ -132,16 +134,33 @@ vi.mock("@/features/agents/stores/agentStore", () => ({ draftSources: mocks.draftSources, setDraftSources: mocks.setDraftSources, removeDraftSource: mocks.removeDraftSource, - // Mirror the real store's fence shape: mutations run their work - // synchronously up to the first await; refreshes apply the listing. - mutateGallery: async (work: () => Promise | T) => work(), + // Mirror the real store fence rather than a pass-through: a refresh + // that starts while a mutation is in flight, or spans one, is dropped. + // That keeps this test able to catch a mis-sequenced refresh. + mutateGallery: async (work: () => Promise | T) => { + mocks.galleryRevision += 1; + mocks.galleryMutationsInFlight += 1; + try { + return await work(); + } finally { + mocks.galleryRevision += 1; + mocks.galleryMutationsInFlight -= 1; + } + }, refreshGallery: async ( fetchGallery: () => Promise<{ personas: Array<{ id: string }>; drafts: Array<{ path: string }>; }>, ) => { + const revisionAtStart = mocks.galleryRevision; const { personas, drafts } = await fetchGallery(); + if ( + revisionAtStart !== mocks.galleryRevision || + mocks.galleryMutationsInFlight !== 0 + ) { + return false; + } mocks.setPersonas(personas); mocks.setDraftSources(drafts); return true; @@ -220,6 +239,8 @@ describe("ChatRightRail", () => { mocks.setDraftSources.mockReset(); mocks.removeDraftSource.mockReset(); mocks.draftSources = []; + mocks.galleryRevision = 0; + mocks.galleryMutationsInFlight = 0; mocks.recoverDraftAgent.mockReset(); mocks.recoverDraftAgent.mockResolvedValue({ path: "/Users/x/.agents/agents/recovered.md", From a4dd9af2616a381361816d38e717d15240a05557 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Sun, 23 Aug 2026 21:18:03 -0700 Subject: [PATCH 4/4] fix(agents): start the next builder before deleting an untouched draft The New agent path discarded the untouched draft first and started the new builder afterwards, while the Back path navigated first. Both now use the helper's `onBeforeDiscard` transition, so the old chat is no longer the active session while its file is being deleted and closing it cannot redirect home. The helper test pins the order: navigate, delete, close. Co-Authored-By: Claude --- .../agents/hooks/useAgentBuilderCoordinator.ts | 5 ++++- .../agents/lib/__tests__/agentBuilderSession.test.ts | 10 +++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/features/agents/hooks/useAgentBuilderCoordinator.ts b/src/features/agents/hooks/useAgentBuilderCoordinator.ts index 7aa983cc5..bce3179ad 100644 --- a/src/features/agents/hooks/useAgentBuilderCoordinator.ts +++ b/src/features/agents/hooks/useAgentBuilderCoordinator.ts @@ -190,11 +190,14 @@ export function useAgentBuilderCoordinator({ } void (async () => { + // Same shape as the Back guard: the moment the discard decision is + // final, move the user into the new builder so the old editor is + // gone while its file is being deleted, then close the old chat. const outcome = await discardUntouchedDraftAgentSession(session.id, { closeSession, + onBeforeDiscard: startBuilderSession, }); if (outcome === "discarded") { - startBuilderSession(); return; } diff --git a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts index bc4e4ba8d..cb008ad0c 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -786,12 +786,14 @@ describe("agentBuilderSession", () => { }); describe("discardUntouchedDraftAgentSession", () => { - it("discards an untouched draft, navigating before the chat closes", async () => { + it("discards an untouched draft: navigate, then delete, then close", async () => { addBuilderSession(); mocks.listPersonaSources.mockResolvedValue([draftSource]); mocks.readAgentSourceFile.mockResolvedValue(draftSource); - mocks.deletePersonaSource.mockResolvedValue(undefined); const order: string[] = []; + mocks.deletePersonaSource.mockImplementation(async () => { + order.push("delete"); + }); const onBeforeDiscard = vi.fn(() => order.push("navigate")); const close = vi.fn(async () => { order.push("close"); @@ -806,7 +808,9 @@ describe("agentBuilderSession", () => { expect(mocks.deletePersonaSource).toHaveBeenCalledWith(draftSource.path); expect(close).toHaveBeenCalledWith("sess-1"); - expect(order).toEqual(["navigate", "close"]); + // The caller's transition runs before the async delete begins, so the + // old editor is already gone while the file is being removed. + expect(order).toEqual(["navigate", "delete", "close"]); }); it("keeps the draft when the user types while the lookup is in flight", async () => {