diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 208707970..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(); @@ -4350,8 +4397,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 +4425,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/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/capabilities/AgentBuilderCapability.tsx b/src/features/agents/capabilities/AgentBuilderCapability.tsx index 751a9c333..b736d5253 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"; @@ -51,37 +51,48 @@ export function AgentBuilderCapability({ const { t } = useTranslation("agents"); const patchSession = useChatSessionStore((state) => state.patchSession); - const refreshPersonas = useCallback(async () => { - const personas = await listPersonas(); - useAgentStore.getState().setPersonas(personas); - }, []); - 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); - } + const seeded = 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) => { - 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, refreshPersonas, session.id], + [onAgentBuilderCompleted, onDraftPromoted, session.id], ); const handleDraftPromoted = useCallback( diff --git a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx index 818ee5cd4..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"; @@ -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( @@ -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", @@ -103,7 +110,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,11 +128,14 @@ describe("AgentBuilderCapability keep-save telemetry", () => { }, }), ); - apiMocks.listPersonas.mockResolvedValue([]); + apiMocks.listAgentGallery.mockResolvedValue({ personas: [], drafts: [] }); resetAgentBuilderSourceLifecycleForTests(); 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/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..6fe7c1c24 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, - isDraftAgentBuilderSession, + discardUntouchedDraftAgentSession, reconcileAgentBuilderSessions, resolveAgentBuilderSessionId, saveDraftAgentSession, @@ -127,10 +126,19 @@ export function useAgentBuilderCoordinator({ } void (async () => { - const hasUserContent = await hasAgentBuilderSessionUserContent( - session.id, - ); - if (!hasUserContent) { + // 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(); return; } @@ -143,7 +151,7 @@ export function useAgentBuilderCoordinator({ return false; }, - [promptForNavigation], + [closeSession, promptForNavigation], ); const start = useCallback( @@ -182,16 +190,14 @@ export function useAgentBuilderCoordinator({ } void (async () => { - const isDraft = await isDraftAgentBuilderSession(session.id); - if ( - isDraft && - !(await hasAgentBuilderSessionUserContent(session.id)) - ) { - await discardDraftAgentSession(session.id, { closeSession }).catch( - (error) => { - console.error("Failed to discard empty agent draft:", error); - }, - ); + // 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, + }); + if (outcome === "discarded") { startBuilderSession(); return; } diff --git a/src/features/agents/hooks/usePersonas.ts b/src/features/agents/hooks/usePersonas.ts index 7ec2b3a72..22fafa47f 100644 --- a/src/features/agents/hooks/usePersonas.ts +++ b/src/features/agents/hooks/usePersonas.ts @@ -16,19 +16,18 @@ const REFRESH_INTERVAL_MS = 60_000; export function usePersonas() { const personas = useAgentStore(selectPersonas); const personasLoading = useAgentStore(selectPersonasLoading); - const setPersonas = useAgentStore((s) => s.setPersonas); + 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 ( - fetchPersonas: () => Promise, + fetchGallery: () => Promise, options: { showLoading: boolean; errorMessage: string }, ) => { if (listRequestInFlightRef.current) { @@ -36,19 +35,12 @@ export function usePersonas() { } listRequestInFlightRef.current = true; - const mutationVersionAtStart = mutationVersionRef.current; if (options.showLoading) { setPersonasLoading(true); } try { - const personas = await fetchPersonas(); - if ( - mutationVersionAtStart === mutationVersionRef.current && - mutationsInFlightRef.current === 0 - ) { - setPersonas(personas); - } + await refreshGallery(fetchGallery); } catch (error) { console.error(options.errorMessage, error); } finally { @@ -58,29 +50,18 @@ export function usePersonas() { } } }, - [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.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:", }); @@ -109,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 @@ -123,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 49211ceb6..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,19 +103,26 @@ vi.mock("@/features/runtime-config/defaults", () => ({ })); import { - deleteDraftAgentSession, + deleteDraftAgentSource, discardDraftAgentSession, + discardUntouchedDraftAgentSession, hasAgentBuilderSessionUserContent, isEmptyDraftAgentSession, 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"; @@ -166,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, @@ -181,7 +194,7 @@ describe("agentBuilderSession", () => { closeSession.mockClear(); navigateChat.mockClear(); resetAgentBuilderSourceLifecycleForTests(); - setAgentBuilderSessionLocalEdits("sess-1", false); + resetAgentBuilderSessionStateForTests(); window.localStorage.clear(); useAgentStore.getState().setProviders([], false); }); @@ -564,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(); @@ -581,6 +597,31 @@ describe("agentBuilderSession", () => { ); }); + 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(); + mocks.listPersonaSources.mockResolvedValue([]); + mocks.readAgentSourceFile.mockRejectedValue( + new Error("Failed to read agent source file"), + ); + mocks.deletePersonaSource.mockResolvedValue(undefined); + + await deleteDraftAgentSource(draftSource.path, { + sessionId: "sess-1", + closeSession, + }); + + expect(mocks.deletePersonaSource).toHaveBeenCalledWith(draftSource.path); + 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); @@ -600,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 = { @@ -740,6 +828,239 @@ 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, + ); + }); + + describe("discardUntouchedDraftAgentSession", () => { + it("discards an untouched draft: navigate, then delete, then close", async () => { + addBuilderSession(); + mocks.listPersonaSources.mockResolvedValue([draftSource]); + mocks.readAgentSourceFile.mockResolvedValue(draftSource); + 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"); + }); + + await expect( + discardUntouchedDraftAgentSession("sess-1", { + closeSession: close, + onBeforeDiscard, + }), + ).resolves.toBe("discarded"); + + expect(mocks.deletePersonaSource).toHaveBeenCalledWith(draftSource.path); + expect(close).toHaveBeenCalledWith("sess-1"); + // 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 () => { + 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("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 = { + ...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("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 () => { 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..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( @@ -458,10 +476,14 @@ export async function isEmptyDraftAgentSession( return isEmptyPlaceholderDraft(freshSource); } -export async function hasAgentBuilderSessionUserContent( - sessionId: string, -): Promise { - if (localEditSessionIds.has(sessionId)) { +/** + * 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) || touchedSessionIds.has(sessionId)) { return true; } @@ -473,25 +495,37 @@ export async function hasAgentBuilderSessionUserContent( ) { 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; } - 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; } @@ -504,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); @@ -517,6 +553,75 @@ 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 type UntouchedDraftDiscardOutcome = + | "discarded" + | "kept" + | "nothing-to-discard"; + +/** + * Discards the session's draft only if it is still untouched at the moment of + * 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. + * + * `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, + deps: CloseSessionDeps & { onBeforeDiscard?: () => void } = {}, +): Promise { + const source = await findCurrentBuilderSource(sessionId); + const isDraft = source === undefined || source.properties?.draft === true; + + 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"; + } + + deps.onBeforeDiscard?.(); + try { + if (source) { + await discardAgentBuilderSource(source.path); + } + } catch (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); + if (liveSessionId !== sessionId) { + clearBuilderSessionState(liveSessionId); + } + await deps.closeSession?.(liveSessionId); + } + return "discarded"; +} + 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..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,21 +155,27 @@ 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( 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, @@ -142,13 +184,29 @@ export async function findAgentBuilderSource( (source) => source.path !== path && !isEmptyPlaceholderDraft(source), ); - if (foundByPath && !isEmptyPlaceholderDraft(foundByPath)) { - return readListedDraftFresh(foundByPath); - } - - const listedSource = movedNonPlaceholder ?? foundByPath ?? sessionMatches[0]; - if (listedSource) { - return readListedDraftFresh(listedSource); + // 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 { @@ -262,7 +320,8 @@ function isBuilderDraftProperties( async function readListedDraftFresh( source: AgentSourceEntry, -): Promise { + backendPaths: ReadonlySet, +): Promise { if (source.properties?.draft !== true) { return source; } @@ -270,6 +329,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/__tests__/agentStore.test.ts b/src/features/agents/stores/__tests__/agentStore.test.ts index b10c56287..5f19b4a55 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,10 @@ describe("agentStore", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, + galleryRefreshGeneration: 0, agents: [], agentsLoading: false, activeAgentId: null, @@ -175,6 +183,112 @@ 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); + }); + + 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([]); + }); + }); }); describe("agentStore.setProviders", () => { diff --git a/src/features/agents/stores/agentStore.ts b/src/features/agents/stores/agentStore.ts index 511093b51..7d413fe61 100644 --- a/src/features/agents/stores/agentStore.ts +++ b/src/features/agents/stores/agentStore.ts @@ -1,6 +1,10 @@ import { create } from "zustand"; import type { Persona, Agent } from "@/shared/types/agents"; import type { AcpProvider } from "@/shared/api/acp"; +import type { + AgentGalleryListing, + AgentSourceEntry, +} from "@/shared/api/agents"; import { canEditPersona } from "@/features/agents/lib/personaPresentation"; const PROVIDER_STORAGE_KEY = "goose:defaultProvider"; @@ -36,6 +40,16 @@ interface AgentStoreState { // Personas personas: Persona[]; 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. 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[]; @@ -62,6 +76,14 @@ interface AgentStoreActions { updatePersona: (id: string, updates: Partial) => void; removePersona: (id: string) => void; 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; @@ -96,6 +118,10 @@ export const useAgentStore = create((set, get) => ({ // State personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, + galleryRefreshGeneration: 0, agents: [], agentsLoading: false, providers: [], @@ -126,6 +152,49 @@ 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), + })), + + refreshGallery: async (fetchGallery) => { + const generation = get().galleryRefreshGeneration + 1; + set({ galleryRefreshGeneration: generation }); + const revisionAtStart = get().galleryRevision; + const { personas, drafts } = await fetchGallery(); + const { + galleryRevision, + galleryMutationsInFlight, + galleryRefreshGeneration, + } = get(); + if ( + revisionAtStart !== galleryRevision || + galleryMutationsInFlight !== 0 || + generation !== galleryRefreshGeneration + ) { + 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/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 849f797f3..c085102d2 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 { + deleteDraftAgentSource, + fileStem, + isEmptyPlaceholderDraft, +} from "@/features/agents/lib/agentBuilderSession"; +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"; import { @@ -158,16 +164,33 @@ 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); + 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, + // 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 +280,47 @@ 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, + (draft: GalleryDraft) => { + const { sessionId, source } = draft; + // 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 () => { + 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"))); }); }, - [onDeleteDraftSession, t], + [ + mutateGallery, + onDeleteDraftSession, + refreshFromDisk, + removeDraftSource, + t, + ], ); useEffect(() => { @@ -669,7 +710,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__/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 47d1b78aa..836eb37d4 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", @@ -136,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, }), })); @@ -291,6 +293,9 @@ describe("AgentsView entry points", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -718,10 +723,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 +742,6 @@ describe("AgentsView entry points", () => { targetAgentPath: "/Users/x/.agents/agents/draft-session.md", targetAgentSlug: "draft-session", targetAgentDraftState: null, - targetAgentDraftSaved: true, }, ], }); @@ -763,7 +770,131 @@ 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 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 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], + 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..b6c977448 100644 --- a/src/features/chat/ui/__tests__/ChatRightRail.test.tsx +++ b/src/features/chat/ui/__tests__/ChatRightRail.test.tsx @@ -15,7 +15,15 @@ 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(), + galleryRevision: 0, + galleryMutationsInFlight: 0, + listAgentGallery: vi.fn(), recoverDraftAgent: vi.fn(), setAgentBuilderSessionLocalEdits: vi.fn(), setAgentBuilderSessionSaveHandler: vi.fn(), @@ -123,6 +131,40 @@ vi.mock("@/features/agents/stores/agentStore", () => ({ setPersonas: mocks.setPersonas, addPersona: mocks.addPersona, updatePersona: mocks.updatePersona, + draftSources: mocks.draftSources, + setDraftSources: mocks.setDraftSources, + removeDraftSource: mocks.removeDraftSource, + // 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; + }, }), }, })); @@ -141,7 +183,7 @@ vi.mock("@/shared/api/agents", () => ({ isBuiltin: false, writable: true, }), - listPersonas: () => mocks.listPersonas(), + listAgentGallery: () => mocks.listAgentGallery(), })); vi.mock("../../hooks/useGitStateAutoRefresh", () => ({ @@ -192,8 +234,13 @@ 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.galleryRevision = 0; + mocks.galleryMutationsInFlight = 0; mocks.recoverDraftAgent.mockReset(); mocks.recoverDraftAgent.mockResolvedValue({ path: "/Users/x/.agents/agents/recovered.md", @@ -713,7 +760,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 }); }