From e65fe04c5ca0bac0d10a02625847c76f1537cbca 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/6] 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 | 50 +++++++---- .../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(+), 129 deletions(-) diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 208707970..4e002adcd 100644 --- a/src/app/AppShell.navigation.test.tsx +++ b/src/app/AppShell.navigation.test.tsx @@ -4350,8 +4350,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" })); @@ -4359,29 +4378,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 818ee5cd4..8304133ff 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( @@ -103,7 +103,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) => @@ -121,7 +121,7 @@ describe("AgentBuilderCapability keep-save telemetry", () => { }, }), ); - apiMocks.listPersonas.mockResolvedValue([]); + apiMocks.listAgentGallery.mockResolvedValue({ personas: [], drafts: [] }); resetAgentBuilderSourceLifecycleForTests(); useAgentStore.setState({ personas: [], 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 47d1b78aa..8f81c64e4 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -19,6 +19,7 @@ import { importPersonas } from "@/shared/api/agents"; import { useAvatarLibrary } from "@/features/agents/hooks/useAvatarLibrary"; import type { AvatarLibraryState } from "@/features/agents/hooks/useAvatarLibrary"; import type { CreatePersonaRequest } from "@/shared/types/agents"; +import { placeholderAgentName } from "@/features/agents/lib/agentBuilderIdentity"; import { AgentsView } from "../AgentsView"; const mockCreatePersona = vi.hoisted(() => vi.fn()); @@ -27,7 +28,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", @@ -291,6 +292,7 @@ describe("AgentsView entry points", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], providers: [], }); useChatSessionStore.setState({ @@ -718,10 +720,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: [ { @@ -734,7 +739,6 @@ describe("AgentsView entry points", () => { targetAgentPath: "/Users/x/.agents/agents/draft-session.md", targetAgentSlug: "draft-session", targetAgentDraftState: null, - targetAgentDraftSaved: true, }, ], }); @@ -763,7 +767,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 42acd67a931171e9cab89156071e912fcadce1fb 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/6] 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 8f81c64e4..d3b541e13 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -293,6 +293,8 @@ describe("AgentsView entry points", () => { personas: [], personasLoading: false, draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -798,6 +800,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 b3df0aa6c7ab9607c536c484842dc0ed1f9ba97b 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/6] 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 8304133ff..da327de88 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); @@ -62,6 +68,7 @@ import { type ChatSession, } from "@/features/chat/stores/chatSessionStore"; import type { AgentSourceEntry } from "@/shared/api/agents"; +import type { Persona } from "@/shared/types/agents"; const existingAgentSource: AgentSourceEntry = { type: "agent", @@ -126,6 +133,9 @@ describe("AgentBuilderCapability keep-save telemetry", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -173,4 +183,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 f42609431d90d322124982aaa7e7e9ab6d5066ab 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/6] 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 () => { From 88f933979330f67f01ea549baa8d2d2153871cc4 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:34:24 -0700 Subject: [PATCH 5/6] fix(agents): restore discard-then-start order for New agent from the gallery a4dd9af2 passed `startBuilderSession` as the discard transition so the old chat would leave the screen before its file was deleted. Starting a builder is async (it resolves a provider/model before creating the chat), so the transition only began a navigation attempt and could not guarantee the new chat was active before the delete or close ran. This path is only reachable from the Agents view, where the old draft's editor is not on screen, so nothing is typed into it during the discard. Start the replacement after the untouched draft is discarded, as before. The helper's navigate -> delete -> close contract still holds for the Back guard, whose transition is a synchronous view change. Co-Authored-By: Claude --- src/features/agents/hooks/useAgentBuilderCoordinator.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/features/agents/hooks/useAgentBuilderCoordinator.ts b/src/features/agents/hooks/useAgentBuilderCoordinator.ts index bce3179ad..6fe7c1c24 100644 --- a/src/features/agents/hooks/useAgentBuilderCoordinator.ts +++ b/src/features/agents/hooks/useAgentBuilderCoordinator.ts @@ -190,14 +190,15 @@ 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. + // This path is only reachable from the Agents view, so no editor for + // the old draft is on screen while it is discarded. Starting the + // replacement builder is async (it resolves a provider/model first), + // so it runs after the untouched draft is gone rather than racing it. const outcome = await discardUntouchedDraftAgentSession(session.id, { closeSession, - onBeforeDiscard: startBuilderSession, }); if (outcome === "discarded") { + startBuilderSession(); return; } From 9d888476455426aafa63bfbd79d36098d9fafe63 Mon Sep 17 00:00:00 2001 From: morgmart <98432065+morgmart@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:33:13 -0700 Subject: [PATCH 6/6] fix(agents): close the draft-discard blind spots and wrong-target deletes found in PR review Untouched-draft detection: - The rail reports "saving" as a local edit; the write has not landed. - Any rail edit marks the session touched for its lifetime, so a draft whose only change was an avatar or model pick is kept, not discarded. - Attachment-only composer state and queued attachments count as content. - A listed-but-unreadable file is treated as having content, not as empty. - The close reaches the live backend session ID, not the provisional one. Deleting from the gallery: - discardAgentBuilderSource re-reads the path and refuses when it no longer holds a draft (AgentBuilderSourceNotDraftError); the card's delete is keyed by the card's path (deleteDraftAgentSource), never by the file its session resolves to. A stale card refreshes from disk instead of deleting a finished agent. Gallery fence: - Delete and promote run as gallery mutations inside the lifecycle helpers, so navigation-driven discards are fenced too. - refreshGallery is latest-wins: an older listing resolving after a newer one is dropped. - findAgentBuilderSource prefers the backend's moved file over a cache entry whose file is gone, and falls through after evicting one. Navigation: - Archiving the active chat only redirects Home when the chat view was showing; Settings and other surfaces stay put. Co-Authored-By: Claude --- src/app/AppShell.navigation.test.tsx | 47 +++++ src/app/AppShell.tsx | 5 +- .../lib/__tests__/agentBuilderSession.test.ts | 177 ++++++++++++++++-- .../agents/lib/agentBuilderSession.ts | 76 ++++++-- .../agents/lib/agentBuilderSourceLifecycle.ts | 90 +++++++-- .../stores/__tests__/agentStore.test.ts | 18 ++ src/features/agents/stores/agentStore.ts | 20 +- src/features/agents/ui/AgentBuilderRail.tsx | 7 +- src/features/agents/ui/AgentsView.tsx | 35 ++-- .../ui/__tests__/AgentBuilderRail.test.tsx | 18 ++ .../ui/__tests__/AgentsView.entry.test.tsx | 34 +++- 11 files changed, 464 insertions(+), 63 deletions(-) diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 4e002adcd..88bbeec3a 100644 --- a/src/app/AppShell.navigation.test.tsx +++ b/src/app/AppShell.navigation.test.tsx @@ -2063,6 +2063,53 @@ describe("AppShell global navigation", () => { expect(mockToastError).toHaveBeenCalledWith("backend down"); }); + it("stays on Settings when the chat selected underneath it is archived", async () => { + // Settings keeps the active chat selected beneath it. Archiving that chat + // (for example an untouched agent draft being discarded on the way to + // Settings) must not yank the user to Home. + const user = userEvent.setup(); + mockAcpArchiveSession.mockResolvedValueOnce(undefined); + useChatSessionStore.setState({ + sessions: [ + { + id: "session-1", + title: "Active chat", + executionTarget: { harnessId: "goose" }, + workingDir: "~/goose artifacts", + createdAt: "2026-06-09T00:00:00.000Z", + updatedAt: "2026-06-09T00:00:00.000Z", + messageCount: 1, + }, + ], + activeSessionId: null, + }); + + renderAppShell(); + + await user.click(screen.getByRole("button", { name: "Open session 1" })); + await waitFor(() => { + expect(screen.getByTestId("active-view")).toHaveTextContent("chat"); + }); + + act(() => { + window.dispatchEvent( + new CustomEvent(OPEN_SETTINGS_EVENT, { + detail: { section: "providers" }, + }), + ); + }); + await waitFor(() => { + expect(screen.getByTestId("active-view")).toHaveTextContent("settings"); + }); + expect(useChatSessionStore.getState().activeSessionId).toBe("session-1"); + + await user.click(screen.getByRole("button", { name: "Archive session 1" })); + await waitFor(() => { + expect(useChatSessionStore.getState().activeSessionId).toBeNull(); + }); + expect(screen.getByTestId("active-view")).toHaveTextContent("settings"); + }); + it("removes a pinned chat from Home only after archive succeeds", async () => { const user = userEvent.setup(); const archive = deferred(); diff --git a/src/app/AppShell.tsx b/src/app/AppShell.tsx index a208bf02f..676e8be20 100644 --- a/src/app/AppShell.tsx +++ b/src/app/AppShell.tsx @@ -3977,7 +3977,10 @@ export function AppShell({ } if (wasActiveSession) { setActiveSession(null); - setActiveView("home"); + // Only leave a view that was showing the archived chat. Settings, + // Agents, and other surfaces keep the active chat selected + // underneath them; the user is looking at those, not at the chat. + setActiveView((view) => (view === "chat" ? "home" : view)); } return cleanupFailureReason diff --git a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts index cb008ad0c..76e82e9bb 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -20,7 +20,8 @@ const chatState = vi.hoisted(() => ({ hasMoreSessions: false, messagesBySession: {} as Record, draftsBySession: {} as Record, - queuedMessageBySession: {} as Record, + queuedMessageBySession: {} as Record, + draftAttachmentsBySession: {} as Record, })); const mocks = vi.hoisted(() => ({ @@ -31,6 +32,7 @@ const mocks = vi.hoisted(() => ({ promotePersonaSource: vi.fn(), listPersonaSources: vi.fn(), readAgentSourceFile: vi.fn(), + updatePersonaSource: vi.fn(), })); const sessionListeners = new Set<() => void>(); @@ -80,6 +82,7 @@ vi.mock("@/features/chat/stores/chatStore", () => ({ messagesBySession: chatState.messagesBySession, draftsBySession: chatState.draftsBySession, queuedMessageBySession: chatState.queuedMessageBySession, + draftAttachmentsBySession: chatState.draftAttachmentsBySession, setSkillDrafts: mocks.setSkillDrafts, }), }, @@ -91,6 +94,7 @@ vi.mock("@/shared/api/agents", () => ({ promotePersonaSource: mocks.promotePersonaSource, listPersonaSources: mocks.listPersonaSources, readAgentSourceFile: mocks.readAgentSourceFile, + updatePersonaSource: mocks.updatePersonaSource, })); vi.mock("@/features/runtime-config/defaults", () => ({ @@ -99,7 +103,7 @@ vi.mock("@/features/runtime-config/defaults", () => ({ })); import { - deleteDraftAgentSession, + deleteDraftAgentSource, discardDraftAgentSession, discardUntouchedDraftAgentSession, hasAgentBuilderSessionUserContent, @@ -107,12 +111,18 @@ import { promoteDraft, recoverDraftAgent, reconcileAgentBuilderSessions, + resetAgentBuilderSessionStateForTests, saveDraftAgentSession, setAgentBuilderSessionLocalEdits, setAgentBuilderSessionSaveHandler, startAgentBuilderSession, } from "../agentBuilderSession"; -import { resetAgentBuilderSourceLifecycleForTests } from "../agentBuilderSourceLifecycle"; +import { + AgentBuilderSourceNotDraftError, + findAgentBuilderSource, + resetAgentBuilderSourceLifecycleForTests, + updateAgentBuilderSource, +} from "../agentBuilderSourceLifecycle"; import { setStoredModelPreference } from "@/features/chat/lib/modelPreferences"; import { useAgentStore } from "@/features/agents/stores/agentStore"; @@ -167,10 +177,12 @@ describe("agentBuilderSession", () => { chatState.messagesBySession = {}; chatState.draftsBySession = {}; chatState.queuedMessageBySession = {}; + chatState.draftAttachmentsBySession = {}; mocks.createPersonaSource.mockReset(); mocks.deletePersonaSource.mockReset(); mocks.promotePersonaSource.mockReset(); mocks.listPersonaSources.mockReset(); + mocks.updatePersonaSource.mockReset(); mocks.readAgentSourceFile.mockReset(); mocks.readAgentSourceFile.mockImplementation( async (_path: string, fallback: unknown) => fallback, @@ -182,7 +194,7 @@ describe("agentBuilderSession", () => { closeSession.mockClear(); navigateChat.mockClear(); resetAgentBuilderSourceLifecycleForTests(); - setAgentBuilderSessionLocalEdits("sess-1", false); + resetAgentBuilderSessionStateForTests(); window.localStorage.clear(); useAgentStore.getState().setProviders([], false); }); @@ -565,14 +577,17 @@ describe("agentBuilderSession", () => { expect(mocks.listPersonaSources).not.toHaveBeenCalled(); }); - it("deleteDraftAgentSession fails before closing when the draft cannot be deleted", async () => { + it("deleteDraftAgentSource fails before closing when the draft cannot be deleted", async () => { addBuilderSession(); mocks.listPersonaSources.mockResolvedValue([draftSource]); mocks.readAgentSourceFile.mockResolvedValue(draftSource); mocks.deletePersonaSource.mockRejectedValue(new Error("disk locked")); await expect( - deleteDraftAgentSession("sess-1", { closeSession }), + deleteDraftAgentSource(draftSource.path, { + sessionId: "sess-1", + closeSession, + }), ).rejects.toThrow("disk locked"); expect(closeSession).not.toHaveBeenCalled(); @@ -582,9 +597,9 @@ 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. + it("deleteDraftAgentSource hands an unreadable path to the backend and still closes the session", async () => { + // The card's file was moved away so reads fail. The backend decides + // whether anything is left to delete; the bound chat closes regardless. mocks.createPersonaSource.mockResolvedValue(draftSource); await startAgentBuilderSession({}, deps); await flushDraftPreparation(); @@ -592,10 +607,14 @@ describe("agentBuilderSession", () => { mocks.readAgentSourceFile.mockRejectedValue( new Error("Failed to read agent source file"), ); + mocks.deletePersonaSource.mockResolvedValue(undefined); - await deleteDraftAgentSession("sess-1", { closeSession }); + await deleteDraftAgentSource(draftSource.path, { + sessionId: "sess-1", + closeSession, + }); - expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(mocks.deletePersonaSource).toHaveBeenCalledWith(draftSource.path); expect(closeSession).toHaveBeenCalledWith("sess-1"); expect(mocks.patchSession).toHaveBeenCalledWith( "sess-1", @@ -622,6 +641,53 @@ describe("agentBuilderSession", () => { expect(closeSession).toHaveBeenCalledWith("sess-1"); }); + it("deleteDraftAgentSource refuses a path that no longer holds a draft", async () => { + // A gallery card is a snapshot. If the file at that path has since become + // a finished agent, the delete must not go through on the card's say-so. + addBuilderSession(); + const finishedAgent = { + ...draftSource, + name: "Constructive Critic", + properties: {}, + }; + mocks.readAgentSourceFile.mockResolvedValue(finishedAgent); + + await expect( + deleteDraftAgentSource(draftSource.path, { + sessionId: "sess-1", + closeSession, + }), + ).rejects.toBeInstanceOf(AgentBuilderSourceNotDraftError); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + + it("findAgentBuilderSource prefers the backend's moved file over an edited cache entry whose file is gone", async () => { + // The user edited the draft (so the cache holds a non-placeholder at A), + // then renamed the file to B outside the app. The backend lists B; A no + // longer reads. The lookup must land on B, not report the draft missing. + const editedAtA = { ...draftSource, name: "Constructive Critic" }; + mocks.updatePersonaSource.mockResolvedValue(editedAtA); + await updateAgentBuilderSource(draftSource.path, { + name: "Constructive Critic", + }); + const movedToB = { + ...editedAtA, + path: "/Users/x/.agents/agents/constructive-critic.md", + }; + mocks.listPersonaSources.mockResolvedValue([movedToB]); + mocks.readAgentSourceFile.mockImplementation(async (path: string) => { + if (path === movedToB.path) { + return movedToB; + } + throw new Error("Failed to read agent source file"); + }); + + await expect( + findAgentBuilderSource("sess-1", draftSource.path), + ).resolves.toMatchObject({ path: movedToB.path }); + }); + it("discardDraftAgentSession follows a draft moved under the same builder session id", async () => { addBuilderSession(); const movedDraft = { @@ -904,6 +970,95 @@ describe("agentBuilderSession", () => { expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); expect(closeSession).toHaveBeenCalledWith("sess-1"); }); + + it("keeps a draft whose only edit was a setup field that has since saved", async () => { + // Picking an avatar or model writes a file that still looks like the + // seeded placeholder on disk. The rail reported the edit, then the save + // landed and the unsaved flag cleared; the session is still touched. + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + setAgentBuilderSessionLocalEdits("sess-1", true); + setAgentBuilderSessionLocalEdits("sess-1", false); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + + it("keeps a draft when the composer holds only an attachment", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + chatState.draftAttachmentsBySession = { + "sess-1": [{ kind: "image", id: "att-1" }], + }; + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + }); + + it("keeps a draft when a queued message carries only an attachment", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + chatState.queuedMessageBySession = { + "sess-1": [ + { + kind: "transport-ready", + recordId: "q-1", + payload: { text: " ", attachments: [{ kind: "file", id: "a" }] }, + }, + ], + }; + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + }); + + it("keeps a draft whose file is listed but cannot be read", async () => { + // The listing says "placeholder" but the file itself is unreadable. + // Unreadable is not empty; nothing may be deleted on that evidence. + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockRejectedValue(new Error("EBUSY")); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("kept"); + expect(mocks.deletePersonaSource).not.toHaveBeenCalled(); + expect(closeSession).not.toHaveBeenCalled(); + }); + + it("closes the live backend session when the builder started under a provisional ID", async () => { + // The session was created as "sess-1" and renamed to the backend's ID + // while the check ran; the archive has to reach the live ID. + chatState.sessions = [ + { + id: "acp-1", + clientSessionId: "sess-1", + title: "New agent", + intent: "build-agent", + agentBuilderOpen: true, + targetAgentPath: draftSource.path, + targetAgentSlug: "draft-sess-1", + } as (typeof chatState.sessions)[number], + ]; + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + mocks.deletePersonaSource.mockResolvedValue(undefined); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { closeSession }), + ).resolves.toBe("discarded"); + expect(closeSession).toHaveBeenCalledWith("acp-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 b72464db6..ac4d3da07 100644 --- a/src/features/agents/lib/agentBuilderSession.ts +++ b/src/features/agents/lib/agentBuilderSession.ts @@ -62,6 +62,11 @@ interface CloseSessionDeps { } const localEditSessionIds = new Set(); +// Sessions whose rail reported an edit at any point. Unlike the unsaved flag +// this never clears on save: a user who only picked an avatar or model has +// started work even though the saved file still looks like the seeded +// placeholder, and leaving must not silently throw that away. +const touchedSessionIds = new Set(); const localSaveHandlersBySessionId = new Map< string, () => MaybePromise @@ -74,12 +79,19 @@ export function setAgentBuilderSessionLocalEdits( ): void { if (hasLocalEdits) { localEditSessionIds.add(sessionId); + touchedSessionIds.add(sessionId); return; } localEditSessionIds.delete(sessionId); } +export function resetAgentBuilderSessionStateForTests(): void { + localEditSessionIds.clear(); + touchedSessionIds.clear(); + localSaveHandlersBySessionId.clear(); +} + export function setAgentBuilderSessionSaveHandler( sessionId: string, saveHandler: (() => MaybePromise) | null, @@ -412,17 +424,23 @@ export async function discardDraftAgentSession( } } -export async function deleteDraftAgentSession( - sessionId: string, - deps: CloseSessionDeps = {}, +/** + * Deletes a draft from the gallery. The card is the file, so the delete is + * keyed by the card's path — never by whatever file the bound session would + * resolve to, which can differ when two files carry the same session tag. + * The bound session, if any, is closed afterwards. + */ +export async function deleteDraftAgentSource( + path: string, + deps: CloseSessionDeps & { sessionId?: string | null } = {}, ): Promise { - const source = await findCurrentBuilderSource(sessionId); - if (source?.properties?.draft === true) { - await discardAgentBuilderSource(source.path); - } + await discardAgentBuilderSource(path); - clearBuilderSessionState(sessionId); - await deps.closeSession?.(sessionId); + const sessionId = deps.sessionId; + if (sessionId) { + clearBuilderSessionState(sessionId); + await deps.closeSession?.(sessionId); + } } export async function promoteDraft( @@ -459,12 +477,13 @@ export async function isEmptyDraftAgentSession( } /** - * 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. + * The in-memory half of the user-content check: any rail edit this session + * (saved or not), composer text or attachments, 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)) { + if (localEditSessionIds.has(sessionId) || touchedSessionIds.has(sessionId)) { return true; } @@ -476,9 +495,18 @@ export function hasLocalAgentBuilderUserContent(sessionId: string): boolean { ) { return true; } + if ((chatState.draftAttachmentsBySession[sessionId]?.length ?? 0) > 0) { + return true; + } const queuedMessages = chatState.queuedMessageBySession[sessionId] ?? []; - if (queuedMessages.some((record) => record.payload.text.trim())) { + if ( + queuedMessages.some( + (record) => + record.payload.text.trim() || + (record.payload.attachments?.length ?? 0) > 0, + ) + ) { return true; } @@ -510,7 +538,9 @@ export async function hasAgentBuilderSessionUserContent( try { freshSource = await readFreshAgentSource(source.path, source); } catch { - return !isEmptyPlaceholderDraft(source); + // Unreadable is not the same as empty. The listed copy may be stale, so + // the only safe answer is "assume there is content". + return true; } return !isEmptyPlaceholderDraft(freshSource); @@ -572,10 +602,22 @@ export async function discardUntouchedDraftAgentSession( await discardAgentBuilderSource(source.path); } } catch (error) { - console.warn("Failed to delete agent builder draft during discard:", error); + // The chat still closes: it is empty. The file is picked up by the next + // reconcile pass, which deletes placeholders whose session is gone. + console.error( + "Failed to delete agent builder draft during discard:", + error, + ); } finally { + // A brand-new builder starts under a provisional client ID and is renamed + // to the backend's ID while it is being created. Close whichever ID is + // live now, or the archive never reaches the backend session. + const liveSessionId = resolveAgentBuilderSessionId(sessionId); clearBuilderSessionState(sessionId); - await deps.closeSession?.(sessionId); + if (liveSessionId !== sessionId) { + clearBuilderSessionState(liveSessionId); + } + await deps.closeSession?.(liveSessionId); } return "discarded"; } diff --git a/src/features/agents/lib/agentBuilderSourceLifecycle.ts b/src/features/agents/lib/agentBuilderSourceLifecycle.ts index 42edf0f83..24560787f 100644 --- a/src/features/agents/lib/agentBuilderSourceLifecycle.ts +++ b/src/features/agents/lib/agentBuilderSourceLifecycle.ts @@ -10,6 +10,7 @@ import { type CreatePersonaSourceRequest, type PersonaSourcePatch, } from "@/shared/api/agents"; +import { useAgentStore } from "@/features/agents/stores/agentStore"; import { deriveSlug, fileStem, @@ -103,9 +104,44 @@ export async function updateAgentBuilderSource( return updated; } +/** + * Thrown when a delete was asked for a path that no longer holds a draft: + * the caller was working from a stale view of the file. + */ +export class AgentBuilderSourceNotDraftError extends Error { + readonly path: string; + + constructor(path: string) { + super(`Agent source at ${path} is no longer a draft`); + this.name = "AgentBuilderSourceNotDraftError"; + this.path = path; + } +} + +/** + * The one way a builder draft leaves disk. Re-reads the file first so a + * caller holding a stale entry (a gallery card, a cached lookup) can never + * delete an agent that has since been finished or replaced at that path. A + * file that cannot be read is handed to the backend as-is; the backend is the + * authority on whether it exists. Runs as a gallery mutation so a disk + * listing that started before the delete cannot land afterwards. + */ export async function discardAgentBuilderSource(path: string): Promise { - await deletePersonaSource(path); - localDraftSourcesByPath.delete(path); + await useAgentStore.getState().mutateGallery(async () => { + let fresh: AgentSourceEntry | undefined; + try { + fresh = await readAgentSourceFile(path); + } catch { + fresh = undefined; + } + if (fresh && fresh.properties?.draft !== true) { + localDraftSourcesByPath.delete(path); + throw new AgentBuilderSourceNotDraftError(path); + } + + await deletePersonaSource(path); + localDraftSourcesByPath.delete(path); + }); } export function forgetLocalAgentBuilderSource(path: string): void { @@ -119,14 +155,18 @@ export async function promoteAgentBuilderDraftSource( return source; } - return promotePersonaSource(source.path, { - name: source.name, - description: source.description, - content: source.content, - properties: source.properties, - }).finally(() => { - localDraftSourcesByPath.delete(source.path); - }); + // A gallery mutation for the same reason as discard: the draft file is + // replaced by the promoted one, and a listing from before must not win. + return useAgentStore.getState().mutateGallery(() => + promotePersonaSource(source.path, { + name: source.name, + description: source.description, + content: source.content, + properties: source.properties, + }).finally(() => { + localDraftSourcesByPath.delete(source.path); + }), + ); } export async function findAgentBuilderSource( @@ -144,13 +184,29 @@ export async function findAgentBuilderSource( (source) => source.path !== path && !isEmptyPlaceholderDraft(source), ); - if (foundByPath && !isEmptyPlaceholderDraft(foundByPath)) { - return readListedDraftFresh(foundByPath, backendPaths); - } - - const listedSource = movedNonPlaceholder ?? foundByPath ?? sessionMatches[0]; - if (listedSource) { - return readListedDraftFresh(listedSource, backendPaths); + // Candidates in order of trust. An edited file at the known path wins only + // while the backend still lists it; otherwise a backend-listed file that + // moved under this session (external rename) beats a cache entry whose + // file is gone. Each candidate is read fresh, and a read that evicts a + // stale cache entry falls through to the next candidate instead of + // reporting the draft missing. + const editedAtPath = + foundByPath && !isEmptyPlaceholderDraft(foundByPath) + ? foundByPath + : undefined; + const ordered = + editedAtPath && backendPaths.has(editedAtPath.path) + ? [editedAtPath, movedNonPlaceholder, foundByPath, sessionMatches[0]] + : [movedNonPlaceholder, editedAtPath, foundByPath, sessionMatches[0]]; + const candidates = [ + ...new Set(ordered.filter((c): c is AgentSourceEntry => c !== undefined)), + ]; + + for (const candidate of candidates) { + const fresh = await readListedDraftFresh(candidate, backendPaths); + if (fresh) { + return fresh; + } } try { diff --git a/src/features/agents/stores/__tests__/agentStore.test.ts b/src/features/agents/stores/__tests__/agentStore.test.ts index 894b6e32a..5f19b4a55 100644 --- a/src/features/agents/stores/__tests__/agentStore.test.ts +++ b/src/features/agents/stores/__tests__/agentStore.test.ts @@ -46,6 +46,7 @@ describe("agentStore", () => { draftSources: [], galleryRevision: 0, galleryMutationsInFlight: 0, + galleryRefreshGeneration: 0, agents: [], agentsLoading: false, activeAgentId: null, @@ -270,6 +271,23 @@ describe("agentStore", () => { listing.resolve({ personas: [], drafts: [draft] }); await expect(pending).resolves.toBe(true); }); + + it("drops an older snapshot that resolves after a newer one (latest wins)", async () => { + // The draft file was removed outside the app between two refreshes. + // The newer listing (no draft) lands first; the older one (still has + // the draft) must not put the card back. + const older = deferredListing(); + const newer = deferredListing(); + const pendingOlder = useAgentStore.getState().refreshGallery(older.fetch); + const pendingNewer = useAgentStore.getState().refreshGallery(newer.fetch); + + newer.resolve({ personas: [], drafts: [] }); + await expect(pendingNewer).resolves.toBe(true); + older.resolve({ personas: [], drafts: [draft] }); + await expect(pendingOlder).resolves.toBe(false); + + expect(useAgentStore.getState().draftSources).toEqual([]); + }); }); }); diff --git a/src/features/agents/stores/agentStore.ts b/src/features/agents/stores/agentStore.ts index d1c56df53..7d413fe61 100644 --- a/src/features/agents/stores/agentStore.ts +++ b/src/features/agents/stores/agentStore.ts @@ -44,9 +44,12 @@ interface AgentStoreState { 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. + // resurrect something the user just deleted or promoted. Snapshots are also + // latest-wins: an older listing that resolves after a newer one is dropped, + // so a file removed outside the app cannot flicker back. galleryRevision: number; galleryMutationsInFlight: number; + galleryRefreshGeneration: number; // Agents agents: Agent[]; @@ -118,6 +121,7 @@ export const useAgentStore = create((set, get) => ({ draftSources: [], galleryRevision: 0, galleryMutationsInFlight: 0, + galleryRefreshGeneration: 0, agents: [], agentsLoading: false, providers: [], @@ -156,10 +160,20 @@ export const useAgentStore = create((set, get) => ({ })), refreshGallery: async (fetchGallery) => { + const generation = get().galleryRefreshGeneration + 1; + set({ galleryRefreshGeneration: generation }); const revisionAtStart = get().galleryRevision; const { personas, drafts } = await fetchGallery(); - const { galleryRevision, galleryMutationsInFlight } = get(); - if (revisionAtStart !== galleryRevision || galleryMutationsInFlight !== 0) { + const { + galleryRevision, + galleryMutationsInFlight, + galleryRefreshGeneration, + } = get(); + if ( + revisionAtStart !== galleryRevision || + galleryMutationsInFlight !== 0 || + generation !== galleryRefreshGeneration + ) { return false; } set({ personas, draftSources: drafts }); diff --git a/src/features/agents/ui/AgentBuilderRail.tsx b/src/features/agents/ui/AgentBuilderRail.tsx index 9ab66d677..143337ef8 100644 --- a/src/features/agents/ui/AgentBuilderRail.tsx +++ b/src/features/agents/ui/AgentBuilderRail.tsx @@ -262,8 +262,13 @@ export function AgentBuilderRail({ ); const isDraft = data?.properties?.draft === true; + // "saving" counts: the write has not landed, so the disk still shows the + // previous content and must not be trusted as the whole story. const hasLocalEdits = - Boolean(data) && (saveStatus === "unsaved" || saveStatus === "error"); + Boolean(data) && + (saveStatus === "unsaved" || + saveStatus === "saving" || + saveStatus === "error"); useEffect(() => { onLocalEditStateChange?.(hasLocalEdits); diff --git a/src/features/agents/ui/AgentsView.tsx b/src/features/agents/ui/AgentsView.tsx index de6bfd46e..c085102d2 100644 --- a/src/features/agents/ui/AgentsView.tsx +++ b/src/features/agents/ui/AgentsView.tsx @@ -54,11 +54,11 @@ import { } from "@/features/agents/lib/agentTelemetry"; import { runAgentViewTransition } from "@/features/agents/lib/agentViewTransitions"; import { - deleteDraftAgentSession, + deleteDraftAgentSource, fileStem, isEmptyPlaceholderDraft, } from "@/features/agents/lib/agentBuilderSession"; -import { discardAgentBuilderSource } from "@/features/agents/lib/agentBuilderSourceLifecycle"; +import { AgentBuilderSourceNotDraftError } 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"; @@ -294,22 +294,33 @@ export function AgentsView({ const handleDeleteDraft = useCallback( (draft: GalleryDraft) => { const { sessionId, source } = draft; - // Run as a gallery mutation so a disk refresh that started before the - // delete cannot land afterwards and put the card back. + // The delete itself is fenced and re-reads the file; the card removal + // rides inside the same mutation so a disk refresh that started before + // the delete cannot land between the two and put the card back. void mutateGallery(async () => { - if (sessionId) { - await deleteDraftAgentSession(sessionId, { - closeSession: onDeleteDraftSession, - }); - } else { - await discardAgentBuilderSource(source.path); - } + await deleteDraftAgentSource(source.path, { + sessionId, + closeSession: onDeleteDraftSession, + }); removeDraftSource(source.path); }).catch((error) => { + // Either way the card was out of date with disk; show what is + // actually there. A stale card over a finished agent is not an error + // the user caused, so it gets no toast. + void refreshFromDisk(); + if (error instanceof AgentBuilderSourceNotDraftError) { + return; + } toast.error(formatAgentError(error, t("view.deleteFailed"))); }); }, - [mutateGallery, onDeleteDraftSession, removeDraftSource, t], + [ + mutateGallery, + onDeleteDraftSession, + refreshFromDisk, + removeDraftSource, + t, + ], ); useEffect(() => { diff --git a/src/features/agents/ui/__tests__/AgentBuilderRail.test.tsx b/src/features/agents/ui/__tests__/AgentBuilderRail.test.tsx index a2d5f040e..c67d7fb92 100644 --- a/src/features/agents/ui/__tests__/AgentBuilderRail.test.tsx +++ b/src/features/agents/ui/__tests__/AgentBuilderRail.test.tsx @@ -620,6 +620,24 @@ describe("AgentBuilderRail", () => { expect(promoteDraft).not.toHaveBeenCalled(); }); + it("reports local edits while a save is still in flight", () => { + // Between "save pressed" and "write landed" the disk still shows the old + // content. The session must not look untouched during that window. + mockHook({ saveStatus: "saving" }); + const onLocalEditStateChange = vi.fn(); + + renderWithProviders( + , + ); + + expect(onLocalEditStateChange).toHaveBeenLastCalledWith(true); + }); + it("allows existing agents to save without draft-only required metadata", async () => { const { saveNow } = mockHook({ data: { diff --git a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx index d3b541e13..836eb37d4 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -137,12 +137,13 @@ vi.mock("@/features/agents/lib/agentTelemetry", () => ({ trackAgentDeleteCompleted: vi.fn(), })); +const mockRefreshFromDisk = vi.fn(); vi.mock("@/features/agents/hooks/usePersonas", () => ({ usePersonas: () => ({ createPersona: mockCreatePersona, updatePersona: mockUpdatePersona, deletePersona: vi.fn(), - refreshFromDisk: vi.fn(), + refreshFromDisk: mockRefreshFromDisk, }), })); @@ -840,6 +841,37 @@ describe("AgentsView entry points", () => { expect(screen.queryByText("gallery.draft")).not.toBeInTheDocument(); }); + it("does not delete a finished agent that now lives where a stale Draft card points", async () => { + // The card was photographed while the path held a draft. Since then the + // file at that path became a finished agent. Delete must re-read and + // refuse, then show what is really on disk. + const { deletePersonaSource, readAgentSourceFile } = await import( + "@/shared/api/agents" + ); + vi.mocked(readAgentSourceFile).mockResolvedValueOnce({ + ...mockDraftSource, + name: "Constructive Critic", + properties: {}, + }); + useAgentStore.setState({ + personas: [persona], + draftSources: [mockDraftSource], + }); + useChatSessionStore.setState({ sessions: [] }); + + render(); + const user = userEvent.setup(); + await user.click( + screen.getByRole("button", { name: "gallery.deleteDraftAria" }), + ); + + await waitFor(() => { + expect(mockRefreshFromDisk).toHaveBeenCalled(); + }); + expect(deletePersonaSource).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + }); + it("does not show a card for an untouched New agent placeholder", () => { useAgentStore.setState({ personas: [persona],