diff --git a/src/app/AppShell.navigation.test.tsx b/src/app/AppShell.navigation.test.tsx index 6926f031e..e823c7055 100644 --- a/src/app/AppShell.navigation.test.tsx +++ b/src/app/AppShell.navigation.test.tsx @@ -26,7 +26,6 @@ import type { Message } from "@/shared/types/messages"; import type { GitState } from "@/shared/types/git"; import { setMultiWorkspaceEnabled } from "@/features/workspaces/multiWorkspacePreference"; import { OPEN_SETTINGS_EVENT } from "@/features/settings/lib/settingsEvents"; -import { useVoiceConversationStore } from "@/features/voice-conversation/stores/voiceConversationStore"; import { SHORTCUT_PREFERENCES_STORAGE_KEY } from "@/features/shortcuts/lib/shortcutRegistry"; import { useShortcutsDialogStore } from "@/features/shortcuts/stores/shortcutsDialogStore"; import { useProjectStore } from "@/features/projects/stores/projectStore"; @@ -4341,8 +4340,27 @@ describe("AppShell global navigation", () => { ).not.toBeInTheDocument(); }); - it("returns to agent builder mode after going back then forward", async () => { + it("discards an untouched agent draft when navigating back, without prompting", async () => { const user = userEvent.setup(); + // The placeholder really exists on disk: the backend lists it and it + // reads back unchanged. + const placeholder = { + type: "agent", + path: "/Users/test/.agents/agents/untitled-agent-created-session.md", + name: "Untitled agent created-sess", + description: "Draft", + content: "Draft in progress.", + global: true, + writable: true, + properties: { draft: true, builderSessionId: "created-session" }, + }; + mockListPersonaSources.mockResolvedValue([placeholder]); + mockReadAgentSourceFile.mockResolvedValue(placeholder); + // Once deleted, the file is no longer listed or readable. + mockDeletePersonaSource.mockImplementation(async () => { + mockListPersonaSources.mockResolvedValue([]); + mockReadAgentSourceFile.mockRejectedValue(new Error("not found")); + }); renderAppShell(); await user.click(screen.getByRole("button", { name: "Sidebar agents" })); @@ -4350,29 +4368,28 @@ describe("AppShell global navigation", () => { await waitFor(() => { expect(screen.getByTestId("active-view")).toHaveTextContent("chat"); }); - await waitFor(() => { - expect(useChatSessionStore.getState().getActiveSession()).toMatchObject({ - id: "created-session", - intent: "build-agent", - }); - }); + await waitForCreatedAgentBuilderTarget(); + // Nothing was typed or edited, so leaving is silent: no "save this + // draft?" prompt, and the placeholder file and its builder state are + // gone rather than lingering as an untitled draft. await user.click(screen.getByRole("button", { name: "Back" })); await waitFor(() => { expect(screen.getByTestId("active-view")).toHaveTextContent("agents"); }); - - await user.click(screen.getByRole("button", { name: "Forward" })); + expect( + screen.queryByText("Save this agent draft?"), + ).not.toBeInTheDocument(); await waitFor(() => { - expect(screen.getByTestId("active-view")).toHaveTextContent("chat"); + expect(mockDeletePersonaSource).toHaveBeenCalledWith( + "/Users/test/.agents/agents/untitled-agent-created-session.md", + ); }); await waitFor(() => { - expect(useChatSessionStore.getState().getActiveSession()).toMatchObject({ - id: "created-session", - intent: "build-agent", - targetAgentPath: - "/Users/test/.agents/agents/untitled-agent-created-session.md", - }); + const session = useChatSessionStore + .getState() + .getSession("created-session"); + expect(session?.intent ?? null).toBeNull(); }); }); diff --git a/src/features/agents/capabilities/AgentBuilderCapability.tsx b/src/features/agents/capabilities/AgentBuilderCapability.tsx index 751a9c333..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 69a4adefe..2139a5c9f 100644 --- a/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx +++ b/src/features/agents/capabilities/__tests__/AgentBuilderCapability.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, screen } from "@testing-library/react"; +import { act, fireEvent, screen, waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { renderWithProviders } from "@/test/render"; @@ -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); @@ -64,6 +70,7 @@ import { import { setExperimentEnabled } from "@/features/experiments/experimentPreferences"; import { AVATAR_COLLECTION_PAGE_EXPERIMENT_ID } from "@/features/experiments/experimentDefinitions"; import type { AgentSourceEntry } from "@/shared/api/agents"; +import type { Persona } from "@/shared/types/agents"; const existingAgentSource: AgentSourceEntry = { type: "agent", @@ -105,7 +112,7 @@ describe("AgentBuilderCapability keep-save telemetry", () => { apiMocks.listPersonaSources.mockReset(); apiMocks.readAgentSourceFile.mockReset(); apiMocks.updatePersonaSource.mockReset(); - apiMocks.listPersonas.mockReset(); + apiMocks.listAgentGallery.mockReset(); apiMocks.listPersonaSources.mockResolvedValue([existingAgentSource]); apiMocks.readAgentSourceFile.mockImplementation( async (_path: string, fallback?: AgentSourceEntry) => @@ -123,12 +130,15 @@ describe("AgentBuilderCapability keep-save telemetry", () => { }, }), ); - apiMocks.listPersonas.mockResolvedValue([]); + apiMocks.listAgentGallery.mockResolvedValue({ personas: [], drafts: [] }); resetAgentBuilderSourceLifecycleForTests(); setExperimentEnabled(AVATAR_COLLECTION_PAGE_EXPERIMENT_ID, false); useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -176,4 +186,40 @@ describe("AgentBuilderCapability keep-save telemetry", () => { expect(telemetryMocks.trackAgentEditCompleted).not.toHaveBeenCalled(); expect(telemetryMocks.trackAgentCreateCompleted).not.toHaveBeenCalled(); }); + + it("applies the disk refresh that follows a save, through the real gallery fence", async () => { + // The optimistic store seed runs as a gallery mutation; the follow-up + // listing must start after that mutation releases the fence, or the fence + // would reject it and the gallery would stay on the optimistic copy. + const fromDisk: Persona = { + id: existingAgentSource.path, + displayName: "Code Reviewer (as listed on disk)", + systemPrompt: existingAgentSource.content, + isBuiltin: false, + writable: true, + createdAt: "2026-06-09T00:00:00.000Z", + updatedAt: "2026-06-09T00:00:00.000Z", + }; + apiMocks.listAgentGallery.mockResolvedValue({ + personas: [fromDisk], + drafts: [], + }); + + renderWithProviders( + , + ); + await screen.findByLabelText(/agent name/i); + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + + await waitFor(() => { + expect(apiMocks.listAgentGallery).toHaveBeenCalledTimes(1); + }); + await waitFor(() => { + expect(useAgentStore.getState().personas).toEqual([fromDisk]); + }); + expect(useAgentStore.getState().galleryMutationsInFlight).toBe(0); + }); }); diff --git a/src/features/agents/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..bce3179ad 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,17 +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); - }, - ); - startBuilderSession(); + // 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") { 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..cb008ad0c 100644 --- a/src/features/agents/lib/__tests__/agentBuilderSession.test.ts +++ b/src/features/agents/lib/__tests__/agentBuilderSession.test.ts @@ -101,6 +101,7 @@ vi.mock("@/features/runtime-config/defaults", () => ({ import { deleteDraftAgentSession, discardDraftAgentSession, + discardUntouchedDraftAgentSession, hasAgentBuilderSessionUserContent, isEmptyDraftAgentSession, promoteDraft, @@ -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,150 @@ 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("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..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; } @@ -517,6 +523,63 @@ 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) { + 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 { 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/__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 511093b51..d1c56df53 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,13 @@ 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. + galleryRevision: number; + galleryMutationsInFlight: number; // Agents agents: Agent[]; @@ -62,6 +73,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 +115,9 @@ export const useAgentStore = create((set, get) => ({ // State personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, agents: [], agentsLoading: false, providers: [], @@ -126,6 +148,39 @@ 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 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 849f797f3..de6bfd46e 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,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,36 @@ 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; + // 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, + }); + } else { + await discardAgentBuilderSource(source.path); + } + removeDraftSource(source.path); }).catch((error) => { toast.error(formatAgentError(error, t("view.deleteFailed"))); }); }, - [onDeleteDraftSession, t], + [mutateGallery, onDeleteDraftSession, removeDraftSource, t], ); useEffect(() => { @@ -669,7 +699,7 @@ export function AgentsView({ > void; onStartChatPersona?: (persona: Persona) => void; @@ -50,8 +60,8 @@ interface PersonaGalleryProps { onCreatePersona: () => void; onImportAgentImage?: () => void; - onContinueDraft?: (sessionId: string) => void; - onDeleteDraft?: (sessionId: string) => void; + onContinueDraft?: (draft: GalleryDraft) => void; + onDeleteDraft?: (draft: GalleryDraft) => void; onImportFile?: (fileBytes: Uint8Array, fileName: string) => void; validateImportFile?: ( file: Pick, @@ -62,11 +72,11 @@ interface PersonaGalleryProps { isLoading?: boolean; } -function draftTitle(session: ChatSession, sourceName?: string): string { - const name = sourceName?.trim(); +function draftTitle(draft: GalleryDraft): string { + const name = draft.source.name.trim(); if (name && !isPlaceholderAgentName(name)) return name; - const title = session.title.trim(); + const title = draft.sessionTitle?.trim() ?? ""; return title.length > 0 ? title : "Untitled agent draft"; } @@ -81,25 +91,23 @@ function draftAvatar(sourceAvatar: unknown): string | null { } function PersonaDraftCard({ - session, + draft, onContinue, onDelete, }: { - session: ChatSession; - onContinue?: (sessionId: string) => void; - onDelete?: (sessionId: string) => void; + draft: GalleryDraft; + onContinue?: (draft: GalleryDraft) => void; + onDelete?: (draft: GalleryDraft) => void; }) { const { t } = useTranslation("agents"); const [readyAnimatedAvatarSrc, setReadyAnimatedAvatarSrc] = useState< string | null >(null); - const { data } = usePersonaSource(session.targetAgentPath ?? null, { - builderSessionId: session.id, - }); - const title = draftTitle(session, data?.name); + const { source } = draft; + const title = draftTitle(draft); const description = - draftDescription(data?.content) ?? t("gallery.draftDescription"); - const avatar = draftAvatar(data?.properties?.avatar); + draftDescription(source.content) ?? t("gallery.draftDescription"); + const avatar = draftAvatar(source.properties?.avatar); const avatarMedia = useAvatarMedia(avatar); const staticAvatarSrc = avatarMedia?.posterSrc ?? @@ -107,9 +115,7 @@ function PersonaDraftCard({ const animatedAvatarReady = avatarMedia?.mediaType === "video" && readyAnimatedAvatarSrc === avatarMedia.src; - const fallbackIconSrc = resolveAgentIcon( - session.targetAgentPath ?? session.id, - ); + const fallbackIconSrc = resolveAgentIcon(source.path); const hoverActionsOverlay = (
onContinue?.(session.id)} + onClick={() => onContinue?.(draft)} aria-label={t("gallery.continueDraftAria", { name: title })} className="pointer-events-auto" > @@ -133,7 +139,7 @@ function PersonaDraftCard({ variant="subtle" size="sm" destructive - onClick={() => onDelete?.(session.id)} + onClick={() => onDelete?.(draft)} aria-label={t("gallery.deleteDraftAria", { name: title })} className="pointer-events-auto" > @@ -216,7 +222,7 @@ function SkeletonCard() { export function PersonaGallery({ personas, - draftSessions = [], + drafts = [], activePersonaId, onSelectPersona, onStartChatPersona, @@ -306,7 +312,7 @@ export function PersonaGallery({ ); } - if (personas.length === 0 && draftSessions.length === 0) { + if (personas.length === 0 && drafts.length === 0) { return (
))} - {draftSessions.map((session, index) => ( + {drafts.map((draft, index) => (
diff --git a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx index 082137234..9eb98300a 100644 --- a/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx +++ b/src/features/agents/ui/__tests__/AgentsView.entry.test.tsx @@ -24,6 +24,7 @@ import { EXPERIMENT_PREFERENCES_STORAGE_VERSION, } from "@/features/experiments/experimentPreferences"; import { AVATAR_COLLECTION_PAGE_EXPERIMENT_ID } from "@/features/experiments/experimentDefinitions"; +import { placeholderAgentName } from "@/features/agents/lib/agentBuilderIdentity"; import { AgentsView } from "../AgentsView"; const mockCreatePersona = vi.hoisted(() => vi.fn()); @@ -32,7 +33,7 @@ const mockTrackAgentCreateCompleted = vi.hoisted(() => vi.fn()); const mockTrackAgentEditCompleted = vi.hoisted(() => vi.fn()); const mockDraftSource = vi.hoisted(() => ({ - type: "agent", + type: "agent" as const, path: "/Users/x/.agents/agents/draft-session.md", name: "New agent", description: "Draft", @@ -309,6 +310,9 @@ describe("AgentsView entry points", () => { useAgentStore.setState({ personas: [], personasLoading: false, + draftSources: [], + galleryRevision: 0, + galleryMutationsInFlight: 0, providers: [], }); useChatSessionStore.setState({ @@ -736,10 +740,13 @@ describe("AgentsView entry points", () => { expect(onStartAgentBuilderSession).toHaveBeenCalledWith({}); }); - it("shows draft sessions at the end of the gallery and continues or deletes them", async () => { + it("shows drafts from disk at the end of the gallery and continues or deletes them", async () => { const onStartAgentBuilderSession = vi.fn(); const onDeleteDraftSession = vi.fn(); - useAgentStore.setState({ personas: [persona] }); + useAgentStore.setState({ + personas: [persona], + draftSources: [mockDraftSource], + }); useChatSessionStore.setState({ sessions: [ { @@ -752,7 +759,6 @@ describe("AgentsView entry points", () => { targetAgentPath: "/Users/x/.agents/agents/draft-session.md", targetAgentSlug: "draft-session", targetAgentDraftState: null, - targetAgentDraftSaved: true, }, ], }); @@ -781,7 +787,100 @@ 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 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 }); }