diff --git a/ui/goose2/src/app/AppShell.tsx b/ui/goose2/src/app/AppShell.tsx index 7ecfad684b..25c5324dc3 100644 --- a/ui/goose2/src/app/AppShell.tsx +++ b/ui/goose2/src/app/AppShell.tsx @@ -1,4 +1,6 @@ import { useCallback, useEffect, useRef, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { toast } from "sonner"; import { Sidebar } from "@/features/sidebar/ui/Sidebar"; import { CreateProjectDialog } from "@/features/projects/ui/CreateProjectDialog"; import { archiveProject } from "@/features/projects/api/projects"; @@ -8,12 +10,21 @@ import type { SectionId } from "@/features/settings/ui/SettingsModal"; import { OPEN_SETTINGS_EVENT } from "@/features/settings/lib/settingsEvents"; import { TopBar } from "./ui/TopBar"; import { useChatStore } from "@/features/chat/stores/chatStore"; +import { selectMessagesBySession } from "@/features/chat/stores/chatSelectors"; import { type ChatSession, useChatSessionStore, } from "@/features/chat/stores/chatSessionStore"; +import { + selectActiveSessionId, + selectHasHydratedSessions, + selectSessions, + selectSessionsLoading, +} from "@/features/chat/stores/chatSessionSelectors"; import { useAgentStore } from "@/features/agents/stores/agentStore"; +import { selectSelectedProvider } from "@/features/agents/stores/agentSelectors"; import { useProjectStore } from "@/features/projects/stores/projectStore"; +import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { findExistingDraft } from "@/features/chat/lib/newChat"; import { DEFAULT_CHAT_TITLE } from "@/features/chat/lib/sessionTitle"; import { useAppStartup } from "./hooks/useAppStartup"; @@ -23,6 +34,10 @@ import { resolveSupportedSessionModelPreference } from "./lib/resolveSupportedSe import { useCreatePersonaNavigation } from "./hooks/useCreatePersonaNavigation"; import { AppShellContent } from "./ui/AppShellContent"; import { acpPrepareSession, acpSetModel } from "@/shared/api/acp"; +import { + updateSessionProject, + updateSessionTitle, +} from "@/features/chat/stores/chatSessionOperations"; import { clearReplayBuffer, getAndDeleteReplayBuffer, @@ -103,6 +118,7 @@ async function syncWindowMinimumSize() { } export function AppShell({ children }: { children?: React.ReactNode }) { + const { t } = useTranslation("chat"); const [sidebarCollapsed, setSidebarCollapsed] = useState(false); const [sidebarWidth, setSidebarWidth] = useState(SIDEBAR_DEFAULT_WIDTH); const [isResizing, setIsResizing] = useState(false); @@ -120,10 +136,21 @@ export function AppShell({ children }: { children?: React.ReactNode }) { loadStoredHomeSessionId(), ); - const chatStore = useChatStore(); - const sessionStore = useChatSessionStore(); - const agentStore = useAgentStore(); - const projectStore = useProjectStore(); + const messagesBySession = useChatStore(selectMessagesBySession); + const setChatActiveSession = useChatStore((s) => s.setActiveSession); + const cleanupChatSession = useChatStore((s) => s.cleanupSession); + const sessions = useChatSessionStore(selectSessions); + const activeSessionId = useChatSessionStore(selectActiveSessionId); + const hasHydratedSessions = useChatSessionStore(selectHasHydratedSessions); + const sessionsLoading = useChatSessionStore(selectSessionsLoading); + const createSession = useChatSessionStore((s) => s.createSession); + const patchSession = useChatSessionStore((s) => s.patchSession); + const setActiveSession = useChatSessionStore((s) => s.setActiveSession); + const archiveSession = useChatSessionStore((s) => s.archiveSession); + const selectedProvider = useAgentStore(selectSelectedProvider); + const projects = useProjectStore(selectProjects); + const fetchProjects = useProjectStore((s) => s.fetchProjects); + const reorderProjects = useProjectStore((s) => s.reorderProjects); const providerInventoryEntries = useProviderInventoryStore((s) => s.entries); const startup = useAppStartup(); const onboardingGate = useOnboardingGate(startup.ready); @@ -182,10 +209,8 @@ export function AppShell({ children }: { children?: React.ReactNode }) { }, []); useEffect(() => { - projectStore.fetchProjects(); - }, [projectStore.fetchProjects]); - - const { activeSessionId } = sessionStore; + fetchProjects(); + }, [fetchProjects]); useEffect(() => { if (activeView === "chat" && activeSessionId) { @@ -194,23 +219,23 @@ export function AppShell({ children }: { children?: React.ReactNode }) { }, [activeSessionId, activeView]); const activeSession = activeSessionId - ? sessionStore.getSession(activeSessionId) + ? sessions.find((session) => session.id === activeSessionId) : undefined; const homeSession = homeSessionId - ? sessionStore.getSession(homeSessionId) + ? sessions.find((session) => session.id === homeSessionId) : undefined; useHomeSessionStateSync({ homeSessionId, homeSession, - messagesBySession: chatStore.messagesBySession, - hasHydratedSessions: sessionStore.hasHydratedSessions, - isLoading: sessionStore.isLoading, + messagesBySession, + hasHydratedSessions, + isLoading: sessionsLoading, setHomeSessionId, }); const ensureHomeSession = useCallback(async () => { - if (!sessionStore.hasHydratedSessions || sessionStore.isLoading) { + if (!hasHydratedSessions || sessionsLoading) { return null; } @@ -226,11 +251,11 @@ export function AppShell({ children }: { children?: React.ReactNode }) { ) { const sessionModelPreference = await resolveSupportedSessionModelPreference( - agentStore.selectedProvider ?? "goose", + selectedProvider ?? "goose", providerInventoryEntries, ); const project = homeSession.projectId - ? (projectStore.projects.find( + ? (projects.find( (candidate) => candidate.id === homeSession.projectId, ) ?? null) : null; @@ -243,14 +268,14 @@ export function AppShell({ children }: { children?: React.ReactNode }) { const shouldClearHomeModel = sessionModelPreference.providerId !== homeSession.providerId || !sessionModelPreference.modelId; - sessionStore.updateSession(homeSession.id, { + patchSession(homeSession.id, { providerId: sessionModelPreference.providerId, modelId: shouldClearHomeModel ? undefined : homeSession.modelId, modelName: shouldClearHomeModel ? undefined : homeSession.modelName, }); if (sessionModelPreference.modelId) { await acpSetModel(homeSession.id, sessionModelPreference.modelId); - sessionStore.updateSession(homeSession.id, { + patchSession(homeSession.id, { modelId: sessionModelPreference.modelId, modelName: sessionModelPreference.modelName, }); @@ -261,10 +286,10 @@ export function AppShell({ children }: { children?: React.ReactNode }) { const workingDir = await resolveSessionCwd(null); const sessionModelPreference = await resolveSupportedSessionModelPreference( - agentStore.selectedProvider ?? "goose", + selectedProvider ?? "goose", providerInventoryEntries, ); - const session = await sessionStore.createSession({ + const session = await createSession({ title: DEFAULT_CHAT_TITLE, providerId: sessionModelPreference.providerId, workingDir, @@ -284,13 +309,14 @@ export function AppShell({ children }: { children?: React.ReactNode }) { } } }, [ - agentStore.selectedProvider, + selectedProvider, + createSession, + hasHydratedSessions, homeSession, providerInventoryEntries, - projectStore.projects, - sessionStore.hasHydratedSessions, - sessionStore, - sessionStore.isLoading, + projects, + sessionsLoading, + patchSession, ]); useEffect(() => { @@ -309,7 +335,7 @@ export function AppShell({ children }: { children?: React.ReactNode }) { `[perf:newtab] createNewTab start (project=${project?.id ?? "none"})`, ); const providerId = - project?.preferredProvider ?? agentStore.selectedProvider ?? "goose"; + project?.preferredProvider ?? selectedProvider ?? "goose"; const sessionModelPreference = await resolveSupportedSessionModelPreference( providerId, @@ -330,9 +356,9 @@ export function AppShell({ children }: { children?: React.ReactNode }) { }); if (existingDraft) { - sessionStore.setActiveSession(existingDraft.id); + setActiveSession(existingDraft.id); setActiveView("chat"); - chatStore.setActiveSession(existingDraft.id); + setChatActiveSession(existingDraft.id); perfLog( `[perf:newtab] ${existingDraft.id.slice(0, 8)} reused draft in ${(performance.now() - tStart).toFixed(1)}ms`, ); @@ -340,7 +366,7 @@ export function AppShell({ children }: { children?: React.ReactNode }) { } const workingDir = await resolveSessionCwd(project); - const session = await sessionStore.createSession({ + const session = await createSession({ title, projectId: project?.id, providerId: sessionModelPreference.providerId, @@ -348,19 +374,20 @@ export function AppShell({ children }: { children?: React.ReactNode }) { modelId: sessionModelPreference.modelId, modelName: sessionModelPreference.modelName, }); - sessionStore.setActiveSession(session.id); + setActiveSession(session.id); setActiveView("chat"); - chatStore.setActiveSession(session.id); + setChatActiveSession(session.id); perfLog( `[perf:newtab] ${session.id.slice(0, 8)} created session in ${(performance.now() - tStart).toFixed(1)}ms`, ); return session; }, [ - agentStore.selectedProvider, - chatStore, + selectedProvider, + createSession, providerInventoryEntries, - sessionStore, + setActiveSession, + setChatActiveSession, ], ); @@ -374,7 +401,7 @@ export function AppShell({ children }: { children?: React.ReactNode }) { const handleStartChatWithSkill = useCallback( (skill: SkillInfo, projectId?: string | null) => { const project = projectId - ? projectStore.projects.find((candidate) => candidate.id === projectId) + ? projects.find((candidate) => candidate.id === projectId) : undefined; void createNewTab(DEFAULT_CHAT_TITLE, project) @@ -387,38 +414,38 @@ export function AppShell({ children }: { children?: React.ReactNode }) { console.error("Failed to start chat with skill:", error); }); }, - [createNewTab, projectStore.projects], + [createNewTab, projects], ); const handleNewChatInProject = useCallback( (projectId: string) => { - const project = projectStore.projects.find((p) => p.id === projectId); + const project = projects.find((p) => p.id === projectId); if (project) { void createNewTab(DEFAULT_CHAT_TITLE, project); } }, - [createNewTab, projectStore.projects], + [createNewTab, projects], ); const handleArchiveProject = useCallback( async (projectId: string) => { try { await archiveProject(projectId); - projectStore.fetchProjects(); + fetchProjects(); } catch { // best-effort } }, - [projectStore.fetchProjects], + [fetchProjects], ); const clearActiveSession = useCallback( (sessionId: string) => { - chatStore.cleanupSession(sessionId); - sessionStore.setActiveSession(null); + cleanupChatSession(sessionId); + setActiveSession(null); setActiveView("home"); }, - [chatStore, sessionStore], + [cleanupChatSession, setActiveSession], ); const openSettings = useCallback((section: SectionId = "appearance") => { setSettingsInitialSection(section); @@ -456,43 +483,43 @@ export function AppShell({ children }: { children?: React.ReactNode }) { const wasActiveSession = currentActiveSessionId === sessionId; try { - await sessionStore.archiveSession(sessionId); - chatStore.cleanupSession(sessionId); + await archiveSession(sessionId); + cleanupChatSession(sessionId); if (!wasActiveSession) { return; } - sessionStore.setActiveSession(null); + setActiveSession(null); setActiveView("home"); } catch { // best-effort } }, - [chatStore, sessionStore], + [archiveSession, cleanupChatSession, setActiveSession], ); const handleEditProject = useCallback( (projectId: string) => { - const project = projectStore.projects.find((p) => p.id === projectId); + const project = projects.find((p) => p.id === projectId); if (project) { setEditingProject(project); setCreateProjectOpen(true); } }, - [projectStore.projects], + [projects], ); const handleMoveToProject = useCallback( (sessionId: string, projectId: string | null) => { - sessionStore.updateSession(sessionId, { projectId }); - const session = useChatSessionStore.getState().getSession(sessionId); if (!session) { return; } void (async () => { + await updateSessionProject(sessionId, projectId); + const nextProject = projectId == null ? null @@ -505,27 +532,25 @@ export function AppShell({ children }: { children?: React.ReactNode }) { } await acpPrepareSession( sessionId, - session.providerId ?? agentStore.selectedProvider ?? "goose", + session.providerId ?? selectedProvider ?? "goose", workingDir, ); })().catch((error) => { - console.error( - "Failed to update ACP session project working directory:", - error, - ); + console.error("Failed to move chat to project:", error); + toast.error(t("notifications.moveError")); }); }, - [agentStore.selectedProvider, sessionStore], + [selectedProvider, t], ); const handleRenameChat = useCallback( (sessionId: string, nextTitle: string) => { - sessionStore.updateSession(sessionId, { - title: nextTitle, - userSetName: true, + void updateSessionTitle(sessionId, nextTitle).catch((error) => { + console.error("Failed to rename session:", error); + toast.error(t("notifications.renameError")); }); }, - [sessionStore], + [t], ); const openCreateProjectDialog = useCallback( @@ -546,23 +571,23 @@ export function AppShell({ children }: { children?: React.ReactNode }) { if (homeSessionId === sessionId) { setHomeSessionId(null); } - sessionStore.setActiveSession(sessionId); + setActiveSession(sessionId); setActiveView("chat"); - chatStore.setActiveSession(sessionId); + setChatActiveSession(sessionId); useChatStore.getState().markSessionRead(sessionId); }, - [chatStore, homeSessionId, sessionStore], + [homeSessionId, setActiveSession, setChatActiveSession], ); const handleSelectSession = useCallback( (id: string) => { - sessionStore.setActiveSession(id); + setActiveSession(id); setActiveView("chat"); - chatStore.setActiveSession(id); + setChatActiveSession(id); useChatStore.getState().markSessionRead(id); loadSessionMessages(id); }, - [sessionStore, chatStore, loadSessionMessages], + [setActiveSession, setChatActiveSession, loadSessionMessages], ); const handleSelectSearchResult = useCallback( @@ -580,11 +605,11 @@ export function AppShell({ children }: { children?: React.ReactNode }) { const handleNavigate = useCallback( (view: AppView) => { if (view !== "chat") { - sessionStore.setActiveSession(null); + setActiveSession(null); } setActiveView(view); }, - [sessionStore], + [setActiveSession], ); const handleCreatePersona = useCreatePersonaNavigation(() => @@ -717,13 +742,13 @@ export function AppShell({ children }: { children?: React.ReactNode }) { // Cmd+N opens new conversation screen if (e.key === "n" && e.metaKey) { e.preventDefault(); - sessionStore.setActiveSession(null); + setActiveSession(null); setActiveView("home"); } }; window.addEventListener("keydown", handler); return () => window.removeEventListener("keydown", handler); - }, [clearActiveSession, sessionStore, toggleSidebar]); + }, [clearActiveSession, setActiveSession, toggleSidebar]); if (!startup.ready) { return ( @@ -768,7 +793,7 @@ export function AppShell({ children }: { children?: React.ReactNode }) { onNavigate={handleNavigate} onNewChatInProject={handleNewChatInProject} onNewChat={() => { - sessionStore.setActiveSession(null); + setActiveSession(null); setActiveView("home"); }} onCreateProject={() => openCreateProjectDialog()} @@ -777,12 +802,12 @@ export function AppShell({ children }: { children?: React.ReactNode }) { onArchiveChat={handleArchiveChat} onRenameChat={handleRenameChat} onMoveToProject={handleMoveToProject} - onReorderProject={projectStore.reorderProjects} + onReorderProject={reorderProjects} onSelectSession={handleSelectSession} onSelectSearchResult={handleSelectSearchResult} activeView={activeView} activeSessionId={activeSessionId} - projects={projectStore.projects} + projects={projects} className="h-full rounded-xl" /> @@ -832,7 +857,7 @@ export function AppShell({ children }: { children?: React.ReactNode }) { pendingProjectCreatedRef.current = null; }} onCreated={(project) => { - projectStore.fetchProjects(); + fetchProjects(); pendingProjectCreatedRef.current?.(project.id); pendingProjectCreatedRef.current = null; setCreateProjectInitialWorkingDir(null); diff --git a/ui/goose2/src/features/agents/hooks/usePersonas.ts b/ui/goose2/src/features/agents/hooks/usePersonas.ts index 4bbadd8dc3..a7bf4b00f6 100644 --- a/ui/goose2/src/features/agents/hooks/usePersonas.ts +++ b/ui/goose2/src/features/agents/hooks/usePersonas.ts @@ -1,5 +1,9 @@ import { useEffect, useCallback, useRef } from "react"; import { useAgentStore } from "../stores/agentStore"; +import { + selectPersonas, + selectPersonasLoading, +} from "../stores/agentSelectors"; import type { CreatePersonaRequest, UpdatePersonaRequest, @@ -9,32 +13,36 @@ import * as api from "@/shared/api/agents"; const REFRESH_INTERVAL_MS = 60_000; export function usePersonas() { - const store = useAgentStore(); + const personas = useAgentStore(selectPersonas); + const personasLoading = useAgentStore(selectPersonasLoading); + const setPersonas = useAgentStore((s) => s.setPersonas); + 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); - // biome-ignore lint/correctness/useExhaustiveDependencies: store is stable and should not trigger re-creation const loadPersonas = useCallback(async () => { - store.setPersonasLoading(true); + setPersonasLoading(true); try { const personas = await api.listPersonas(); - store.setPersonas(personas); + setPersonas(personas); } catch (error) { console.error("Failed to load personas:", error); // Fall back to empty list - builtins will come from backend } finally { - store.setPersonasLoading(false); + setPersonasLoading(false); } - }, []); + }, [setPersonas, setPersonasLoading]); - // biome-ignore lint/correctness/useExhaustiveDependencies: store is stable and should not trigger re-creation const refreshFromDisk = useCallback(async () => { try { const personas = await api.refreshPersonas(); - store.setPersonas(personas); + setPersonas(personas); } catch (error) { console.error("Failed to refresh personas from disk:", error); } - }, []); + }, [setPersonas]); useEffect(() => { loadPersonas(); @@ -57,32 +65,35 @@ export function usePersonas() { }; }, [refreshFromDisk]); - // biome-ignore lint/correctness/useExhaustiveDependencies: store is stable and should not trigger re-creation - const createPersona = useCallback(async (req: CreatePersonaRequest) => { - const persona = await api.createPersona(req); - store.addPersona(persona); - return persona; - }, []); + const createPersona = useCallback( + async (req: CreatePersonaRequest) => { + const persona = await api.createPersona(req); + addPersona(persona); + return persona; + }, + [addPersona], + ); - // biome-ignore lint/correctness/useExhaustiveDependencies: store is stable and should not trigger re-creation const updatePersona = useCallback( async (id: string, req: UpdatePersonaRequest) => { const persona = await api.updatePersona(id, req); - store.updatePersona(id, persona); + updatePersonaInStore(id, persona); return persona; }, - [], + [updatePersonaInStore], ); - // biome-ignore lint/correctness/useExhaustiveDependencies: store is stable and should not trigger re-creation - const deletePersona = useCallback(async (id: string) => { - await api.deletePersona(id); - store.removePersona(id); - }, []); + const deletePersona = useCallback( + async (id: string) => { + await api.deletePersona(id); + removePersona(id); + }, + [removePersona], + ); return { - personas: store.personas, - isLoading: store.personasLoading, + personas, + isLoading: personasLoading, createPersona, updatePersona, deletePersona, diff --git a/ui/goose2/src/features/agents/hooks/useProviderSelection.ts b/ui/goose2/src/features/agents/hooks/useProviderSelection.ts index 060547eafd..9342e19fd6 100644 --- a/ui/goose2/src/features/agents/hooks/useProviderSelection.ts +++ b/ui/goose2/src/features/agents/hooks/useProviderSelection.ts @@ -1,10 +1,11 @@ import { useCallback } from "react"; import { useAgentStore } from "../stores/agentStore"; +import { selectSelectedProvider } from "../stores/agentSelectors"; export function useProviderSelection() { const providers = useAgentStore((s) => s.providers); const providersLoading = useAgentStore((s) => s.providersLoading); - const selectedProvider = useAgentStore((s) => s.selectedProvider); + const selectedProvider = useAgentStore(selectSelectedProvider); const storeSetSelectedProvider = useAgentStore((s) => s.setSelectedProvider); const setSelectedProvider = useCallback( diff --git a/ui/goose2/src/features/agents/stores/agentSelectors.ts b/ui/goose2/src/features/agents/stores/agentSelectors.ts new file mode 100644 index 0000000000..524538b2ea --- /dev/null +++ b/ui/goose2/src/features/agents/stores/agentSelectors.ts @@ -0,0 +1,9 @@ +import type { AgentStore } from "./agentStore"; + +export const selectPersonas = (state: AgentStore) => state.personas; + +export const selectPersonasLoading = (state: AgentStore) => + state.personasLoading; + +export const selectSelectedProvider = (state: AgentStore) => + state.selectedProvider; diff --git a/ui/goose2/src/features/agents/ui/AgentsView.tsx b/ui/goose2/src/features/agents/ui/AgentsView.tsx index 9b0f47b19a..23abe723a2 100644 --- a/ui/goose2/src/features/agents/ui/AgentsView.tsx +++ b/ui/goose2/src/features/agents/ui/AgentsView.tsx @@ -16,6 +16,10 @@ import { AlertDialogTitle, } from "@/shared/ui/alert-dialog"; import { useAgentStore } from "@/features/agents/stores/agentStore"; +import { + selectPersonas, + selectPersonasLoading, +} from "@/features/agents/stores/agentSelectors"; import { PersonaGallery } from "@/features/agents/ui/PersonaGallery"; import { PersonaEditor } from "@/features/agents/ui/PersonaEditor"; import { @@ -41,8 +45,8 @@ export function AgentsView() { const [search, setSearch] = useState(""); const [deletingPersona, setDeletingPersona] = useState(null); - const personas = useAgentStore((s) => s.personas); - const personasLoading = useAgentStore((s) => s.personasLoading); + const personas = useAgentStore(selectPersonas); + const personasLoading = useAgentStore(selectPersonasLoading); const personaEditorOpen = useAgentStore((s) => s.personaEditorOpen); const editingPersona = useAgentStore((s) => s.editingPersona); const personaEditorMode = useAgentStore((s) => s.personaEditorMode); diff --git a/ui/goose2/src/features/chat/hooks/__tests__/useChatSessionController.compaction.test.ts b/ui/goose2/src/features/chat/hooks/__tests__/useChatSessionController.compaction.test.ts index fb00e14235..2b5e1dc4b7 100644 --- a/ui/goose2/src/features/chat/hooks/__tests__/useChatSessionController.compaction.test.ts +++ b/ui/goose2/src/features/chat/hooks/__tests__/useChatSessionController.compaction.test.ts @@ -246,7 +246,7 @@ describe("useChatSessionController compaction behavior", () => { useChatStore .getState() .replaceTokenState("session-1", mockTokenState, true); - useChatSessionStore.getState().updateSession("session-1", { + useChatSessionStore.getState().patchSession("session-1", { providerId: "goose", }); @@ -300,7 +300,7 @@ describe("useChatSessionController compaction behavior", () => { useChatStore .getState() .replaceTokenState("session-1", mockTokenState, true); - useChatSessionStore.getState().updateSession("session-1", { + useChatSessionStore.getState().patchSession("session-1", { providerId: "goose", personaId: "persona-b", }); diff --git a/ui/goose2/src/features/chat/hooks/useChat.ts b/ui/goose2/src/features/chat/hooks/useChat.ts index 1d7829cd57..a35bdb8759 100644 --- a/ui/goose2/src/features/chat/hooks/useChat.ts +++ b/ui/goose2/src/features/chat/hooks/useChat.ts @@ -4,10 +4,12 @@ import { useChatSessionStore } from "../stores/chatSessionStore"; import { clearReplayBuffer, getAndDeleteReplayBuffer } from "./replayBuffer"; import { type ChatAttachmentDraft, + type Message, createSystemNotificationMessage, createUserMessage, } from "@/shared/types/messages"; import type { ChatState, TokenState } from "@/shared/types/chat"; +import { INITIAL_SESSION_CHAT_RUNTIME } from "@/shared/types/chat"; import { acpSendMessage, acpCancelSession, @@ -18,7 +20,6 @@ import { getSessionTitleFromDraft, isDefaultChatTitle, } from "../lib/sessionTitle"; -import { findLastIndex } from "@/shared/lib/arrays"; import { perfLog } from "@/shared/lib/perfLog"; import { appendAttachmentPaths, @@ -28,10 +29,10 @@ import { import { sanitizeReplayMessages } from "../lib/replaySanitizer"; import { i18n } from "@/shared/i18n"; import type { ChatSendOptions } from "../types"; -import { buildSkillRetryOptions } from "../lib/skillSendPayload"; // TODO: Remove this fallback once goose2 has first-class /-commands. const MANUAL_COMPACT_TRIGGER = "/compact"; +const EMPTY_MESSAGES: Message[] = []; type CompactConversationResult = "completed" | "failed" | "skipped"; function createCompactionConfirmationMessage() { @@ -104,12 +105,28 @@ export function useChat( ensurePrepared?: (personaId?: string) => Promise; }, ) { - const store = useChatStore(); const abortRef = useRef(null); - const messages = store.messagesBySession[sessionId] ?? []; - const { chatState, tokenState, error, streamingMessageId } = - store.getSessionRuntime(sessionId); + const messages = useChatStore( + (s) => s.messagesBySession[sessionId] ?? EMPTY_MESSAGES, + ); + const runtime = useChatStore( + (s) => s.sessionStateById[sessionId] ?? INITIAL_SESSION_CHAT_RUNTIME, + ); + const setActiveSession = useChatStore((s) => s.setActiveSession); + const addMessage = useChatStore((s) => s.addMessage); + const setMessages = useChatStore((s) => s.setMessages); + const clearMessages = useChatStore((s) => s.clearMessages); + const setChatState = useChatStore((s) => s.setChatState); + const setError = useChatStore((s) => s.setError); + const setStreamingMessageId = useChatStore((s) => s.setStreamingMessageId); + const setPendingAssistantProvider = useChatStore( + (s) => s.setPendingAssistantProvider, + ); + const clearDraft = useChatStore((s) => s.clearDraft); + const setSessionLoading = useChatStore((s) => s.setSessionLoading); + + const { chatState, tokenState, error, streamingMessageId } = runtime; const isStreaming = chatState === "streaming" || streamingMessageId !== null; const resolvePersonaInfo = useCallback( @@ -166,8 +183,8 @@ export function useChat( systemPromptOverride ?? agent?.systemPrompt ?? undefined; // Ensure active session - store.setActiveSession(sessionId); - store.setPendingAssistantProvider(sessionId, providerId); + setActiveSession(sessionId); + setPendingAssistantProvider(sessionId, providerId); // Create and add user message const userMessage = createUserMessage( @@ -195,9 +212,9 @@ export function useChat( }); } } - store.addMessage(sessionId, userMessage); - store.setChatState(sessionId, "thinking"); - store.setError(sessionId, null); + addMessage(sessionId, userMessage); + setChatState(sessionId, "thinking"); + setError(sessionId, null); const sessionStore = useChatSessionStore.getState(); const session = sessionStore.getSession(sessionId); @@ -208,19 +225,19 @@ export function useChat( // A better backend-generated title will overwrite this if it arrives // via the acp:session_info event. if (session && isDefaultChatTitle(session.title)) { - sessionStore.updateSession(sessionId, { + sessionStore.patchSession(sessionId, { title: getSessionTitleFromDraft(text, attachments), updatedAt: new Date().toISOString(), }); } else { - sessionStore.updateSession(sessionId, { + sessionStore.patchSession(sessionId, { updatedAt: new Date().toISOString(), }); } options?.onMessageAccepted?.(sessionId); - store.clearDraft(sessionId); + clearDraft(sessionId); const abort = new AbortController(); abortRef.current = abort; @@ -228,7 +245,7 @@ export function useChat( try { await options?.ensurePrepared?.(effectivePersonaInfo?.id); - store.setChatState(sessionId, "streaming"); + setChatState(sessionId, "streaming"); const promptWithPaths = appendAttachmentPaths(text.trim(), attachments); const acpPrompt = promptWithPaths || (images?.length ? " " : promptWithPaths); @@ -251,11 +268,11 @@ export function useChat( `[perf:send] ${sid} acpSendMessage returned after ${(performance.now() - tAcp).toFixed(1)}ms (total sendMessage ${(performance.now() - tSendStart).toFixed(1)}ms)`, ); - store.setChatState(sessionId, "idle"); - store.setStreamingMessageId(sessionId, null); + setChatState(sessionId, "idle"); + setStreamingMessageId(sessionId, null); } catch (err) { if (err instanceof DOMException && err.name === "AbortError") { - store.setChatState(sessionId, "idle"); + setChatState(sessionId, "idle"); } else { const errorMessage = getErrorMessage(err); const liveStore = useChatStore.getState(); @@ -278,18 +295,24 @@ export function useChat( sessionId, createSystemNotificationMessage(errorMessage, "error"), ); - store.setError(sessionId, errorMessage); - store.setChatState(sessionId, "idle"); - store.setStreamingMessageId(sessionId, null); + setError(sessionId, errorMessage); + setChatState(sessionId, "idle"); + setStreamingMessageId(sessionId, null); } - store.setPendingAssistantProvider(sessionId, null); + setPendingAssistantProvider(sessionId, null); } finally { abortRef.current = null; } }, [ sessionId, - store, + setActiveSession, + setPendingAssistantProvider, + addMessage, + setChatState, + setError, + clearDraft, + setStreamingMessageId, providerOverride, systemPromptOverride, resolvePersonaInfo, @@ -303,9 +326,9 @@ export function useChat( .getState() .getSessionRuntime(sessionId).streamingMessageId; - store.setChatState(sessionId, "idle"); - store.setStreamingMessageId(sessionId, null); - store.setPendingAssistantProvider(sessionId, null); + setChatState(sessionId, "idle"); + setStreamingMessageId(sessionId, null); + setPendingAssistantProvider(sessionId, null); // Cancel the backend ACP session to stop orphaned streaming events acpCancelSession(sessionId) .then((wasCancelled) => { @@ -316,50 +339,26 @@ export function useChat( .catch(() => { // Best-effort cancellation — ignore errors }); - }, [store, sessionId]); - - const retryLastMessage = useCallback(async () => { - const sessionMessages = store.messagesBySession[sessionId] ?? []; - // Find the last user message - const lastUserIndex = findLastIndex( - sessionMessages, - (m) => m.role === "user", - ); - if (lastUserIndex === -1) return; - - const lastUserMessage = sessionMessages[lastUserIndex]; - - // Remove all messages after (and including) the last assistant response - const messagesToKeep = sessionMessages.slice(0, lastUserIndex); - store.setMessages(sessionId, messagesToKeep); - - // Extract the text and resend - const textContent = lastUserMessage.content.find((c) => c.type === "text"); - if (textContent && "text" in textContent) { - const targetPersonaId = lastUserMessage.metadata?.targetPersonaId; - const targetPersonaName = lastUserMessage.metadata?.targetPersonaName; - const retryOptions = buildSkillRetryOptions( - textContent.text, - lastUserMessage.metadata?.chips, - ); - await sendMessage( - textContent.text || (retryOptions ? " " : ""), - targetPersonaId - ? { id: targetPersonaId, name: targetPersonaName } - : undefined, - undefined, - retryOptions, - ); - } - }, [sessionId, store, sendMessage]); + }, [ + setChatState, + setPendingAssistantProvider, + setStreamingMessageId, + sessionId, + ]); const clearChat = useCallback(() => { abortRef.current?.abort(); - store.clearMessages(sessionId); - store.setChatState(sessionId, "idle"); - store.setStreamingMessageId(sessionId, null); - store.setPendingAssistantProvider(sessionId, null); - }, [sessionId, store]); + clearMessages(sessionId); + setChatState(sessionId, "idle"); + setStreamingMessageId(sessionId, null); + setPendingAssistantProvider(sessionId, null); + }, [ + sessionId, + clearMessages, + setChatState, + setStreamingMessageId, + setPendingAssistantProvider, + ]); const getWorkingDir = useCallback( () => @@ -381,25 +380,25 @@ export function useChat( overridePersona?.name, ); - store.setActiveSession(sessionId); - store.setChatState(sessionId, "compacting"); - store.setStreamingMessageId(sessionId, null); - store.setError(sessionId, null); + setActiveSession(sessionId); + setChatState(sessionId, "compacting"); + setStreamingMessageId(sessionId, null); + setError(sessionId, null); try { await options?.ensurePrepared?.(effectivePersonaInfo?.id); } catch (err) { const errorMessage = getErrorMessage(err); - store.addMessage( + addMessage( sessionId, createSystemNotificationMessage(errorMessage, "error"), ); - store.setError(sessionId, errorMessage); - store.setChatState(sessionId, "idle"); + setError(sessionId, errorMessage); + setChatState(sessionId, "idle"); return "failed" as CompactConversationResult; } - store.setSessionLoading(sessionId, true); + setSessionLoading(sessionId, true); clearReplayBuffer(sessionId); try { @@ -415,37 +414,50 @@ export function useChat( const workingDir = getWorkingDir(); await acpLoadSession(sessionId, workingDir); - store.setSessionLoading(sessionId, false); + setSessionLoading(sessionId, false); const buffer = getAndDeleteReplayBuffer(sessionId); if (buffer) { - store.setMessages(sessionId, [ + setMessages(sessionId, [ ...sanitizeReplayMessages(buffer), createCompactionConfirmationMessage(), ]); } else { - store.addMessage(sessionId, createCompactionConfirmationMessage()); + addMessage(sessionId, createCompactionConfirmationMessage()); } return "completed" as CompactConversationResult; } catch (err) { clearReplayBuffer(sessionId); - store.setSessionLoading(sessionId, false); + setSessionLoading(sessionId, false); const errorMessage = getErrorMessage(err); - store.addMessage( + addMessage( sessionId, createSystemNotificationMessage(errorMessage, "error"), ); - store.setError(sessionId, errorMessage); + setError(sessionId, errorMessage); return "failed" as CompactConversationResult; } finally { - store.setChatState(sessionId, "idle"); - store.setStreamingMessageId(sessionId, null); - store.setPendingAssistantProvider(sessionId, null); - store.setSessionLoading(sessionId, false); + setChatState(sessionId, "idle"); + setStreamingMessageId(sessionId, null); + setPendingAssistantProvider(sessionId, null); + setSessionLoading(sessionId, false); } }, - [getWorkingDir, options, resolvePersonaInfo, sessionId, store], + [ + getWorkingDir, + options, + resolvePersonaInfo, + sessionId, + setActiveSession, + setChatState, + setStreamingMessageId, + setError, + addMessage, + setSessionLoading, + setMessages, + setPendingAssistantProvider, + ], ); const stopStreaming = stopGeneration; @@ -459,7 +471,6 @@ export function useChat( sendMessage, stopGeneration, stopStreaming, - retryLastMessage, clearChat, compactConversation, isStreaming, diff --git a/ui/goose2/src/features/chat/hooks/useChatSessionController.ts b/ui/goose2/src/features/chat/hooks/useChatSessionController.ts index f2acb12871..041b7cf736 100644 --- a/ui/goose2/src/features/chat/hooks/useChatSessionController.ts +++ b/ui/goose2/src/features/chat/hooks/useChatSessionController.ts @@ -8,8 +8,10 @@ import { useMessageQueue } from "./useMessageQueue"; import { useChatStore } from "../stores/chatStore"; import { useChatSessionStore } from "../stores/chatSessionStore"; import { useAgentStore } from "@/features/agents/stores/agentStore"; +import { selectPersonas } from "@/features/agents/stores/agentSelectors"; import { useProviderSelection } from "@/features/agents/hooks/useProviderSelection"; import { useProjectStore } from "@/features/projects/stores/projectStore"; +import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { resolveAgentProviderCatalogIdStrictFromEntries } from "@/features/providers/providerCatalog"; import { useProviderCatalogStore } from "@/features/providers/stores/providerCatalogStore"; import { @@ -25,6 +27,7 @@ import { } from "../lib/autoCompact"; import { resolveSessionCwd } from "@/features/projects/lib/sessionCwdSelection"; import { acpPrepareSession, acpSetModel } from "@/shared/api/acp"; +import { updateSessionProject } from "../stores/chatSessionOperations"; import { useResolvedAgentModelPicker, type PreferredModelSelection, @@ -51,7 +54,7 @@ export function useChatSessionController({ selectedProvider: globalSelectedProvider, setSelectedProvider: setGlobalSelectedProvider, } = useProviderSelection(); - const personas = useAgentStore((s) => s.personas); + const personas = useAgentStore(selectPersonas); const session = useChatSessionStore((s) => sessionId ? s.sessions.find((candidate) => candidate.id === sessionId) @@ -63,7 +66,7 @@ export function useChatSessionController({ const clearActiveWorkspace = useChatSessionStore( (s) => s.clearActiveWorkspace, ); - const projects = useProjectStore((s) => s.projects); + const projects = useProjectStore(selectProjects); const projectsLoading = useProjectStore((s) => s.loading); const catalogEntries = useProviderCatalogStore((s) => s.entries); const [pendingPersonaId, setPendingPersonaId] = useState(); @@ -184,7 +187,7 @@ export function useChatSessionController({ } await acpSetModel(sessionId, modelSelection.id); - sessionStore.updateSession(sessionId, { + sessionStore.patchSession(sessionId, { modelId: modelSelection.id, modelName: modelSelection.name, }); @@ -315,16 +318,18 @@ export function useChatSessionController({ .projects.find((candidate) => candidate.id === projectId) ?? null); - useChatSessionStore.getState().updateSession(sessionId, { projectId }); - if (!selectedProvider) { - return; - } - void prepareCurrentSession( - selectedProvider, - nextProject, - activeWorkspace?.path, - effectiveModelSelection, - ).catch((error) => { + void (async () => { + await updateSessionProject(sessionId, projectId); + if (!selectedProvider) { + return; + } + await prepareCurrentSession( + selectedProvider, + nextProject, + activeWorkspace?.path, + effectiveModelSelection, + ); + })().catch((error) => { console.error("Failed to update ACP session working directory:", error); }); }, @@ -374,7 +379,7 @@ export function useChatSessionController({ } useChatSessionStore .getState() - .updateSession(sessionId, { personaId: personaId ?? undefined }); + .patchSession(sessionId, { personaId: personaId ?? undefined }); }, [ handleProviderChange, @@ -395,7 +400,7 @@ export function useChatSessionController({ if (sessionId) { useChatSessionStore .getState() - .updateSession(sessionId, { personaId: undefined }); + .patchSession(sessionId, { personaId: undefined }); } else { setPendingPersonaId(undefined); } @@ -703,7 +708,6 @@ export function useChatSessionController({ const patch: { providerId?: string; personaId?: string | undefined; - projectId?: string | null; modelId?: string | undefined; modelName?: string | undefined; } = {}; @@ -716,13 +720,14 @@ export function useChatSessionController({ if (hasPendingPersona) { patch.personaId = nextPersonaId; } - if (hasPendingProject) { - patch.projectId = nextProjectId ?? null; + if (Object.keys(patch).length > 0) { + useChatSessionStore.getState().patchSession(sessionId, patch); } - useChatSessionStore.getState().updateSession(sessionId, patch); - try { + if (hasPendingProject) { + await updateSessionProject(sessionId, nextProjectId ?? null); + } await prepareCurrentSession( nextProviderId, nextProject, diff --git a/ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts b/ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts index 81509350a5..180b75cd44 100644 --- a/ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts +++ b/ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts @@ -310,7 +310,7 @@ export function useResolvedAgentModelPicker({ setGlobalSelectedProvider(nextProviderId); } - useChatSessionStore.getState().updateSession(sessionId, { + useChatSessionStore.getState().patchSession(sessionId, { modelId, modelName, }); @@ -335,7 +335,7 @@ export function useResolvedAgentModelPicker({ } else { clearStoredModelPreference(selectedAgentId); } - useChatSessionStore.getState().updateSession(sessionId, { + useChatSessionStore.getState().patchSession(sessionId, { providerId: previousProviderId, modelId: previousModelId, modelName: previousModelName, diff --git a/ui/goose2/src/features/chat/lib/skillSendPayload.ts b/ui/goose2/src/features/chat/lib/skillSendPayload.ts index de40c52af4..28b51bd0ce 100644 --- a/ui/goose2/src/features/chat/lib/skillSendPayload.ts +++ b/ui/goose2/src/features/chat/lib/skillSendPayload.ts @@ -2,7 +2,6 @@ import { formatSkillInstructionPrompt, type SkillCommandMatch, } from "@/features/skills/lib/skillChatPrompt"; -import type { MessageChip } from "@/shared/types/messages"; import type { ChatSendOptions, ChatSkillDraft } from "../types"; interface SkillSendPayload { @@ -50,19 +49,3 @@ export function buildSkillSendPayload( }, }; } - -export function buildSkillRetryOptions( - text: string, - chips?: MessageChip[], -): ChatSendOptions | undefined { - const skillChips = chips?.filter((chip) => chip.type === "skill") ?? []; - if (skillChips.length === 0) return undefined; - - return { - displayText: text, - assistantPrompt: formatSkillInstructionPrompt( - skillChips.map((chip) => ({ name: chip.label })), - ), - chips: skillChips, - }; -} diff --git a/ui/goose2/src/features/chat/stores/__tests__/chatSessionOperations.test.ts b/ui/goose2/src/features/chat/stores/__tests__/chatSessionOperations.test.ts new file mode 100644 index 0000000000..36b658dc10 --- /dev/null +++ b/ui/goose2/src/features/chat/stores/__tests__/chatSessionOperations.test.ts @@ -0,0 +1,114 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { useChatSessionStore, type ChatSession } from "../chatSessionStore"; +import { + updateSessionProject, + updateSessionTitle, +} from "../chatSessionOperations"; + +const mockRenameSession = vi.fn(); +const mockUpdateSessionProject = vi.fn(); + +vi.mock("@/shared/api/acpApi", () => ({ + renameSession: (...args: unknown[]) => mockRenameSession(...args), + updateSessionProject: (...args: unknown[]) => + mockUpdateSessionProject(...args), +})); + +function resetStore() { + useChatSessionStore.setState({ + sessions: [], + activeSessionId: null, + isLoading: false, + hasHydratedSessions: false, + contextPanelOpenBySession: {}, + activeWorkspaceBySession: {}, + }); +} + +function seedSession(overrides: Partial = {}) { + useChatSessionStore.setState({ + sessions: [ + { + id: "session-1", + title: "Original Title", + createdAt: "2026-04-01T00:00:00.000Z", + updatedAt: "2026-04-01T00:00:00.000Z", + messageCount: 0, + ...overrides, + }, + ], + }); +} + +describe("chatSessionOperations", () => { + beforeEach(() => { + resetStore(); + vi.clearAllMocks(); + }); + + describe("updateSessionTitle", () => { + it("renames in backend before patching local state", async () => { + seedSession({ userSetName: false }); + mockRenameSession.mockResolvedValue(undefined); + + await updateSessionTitle("session-1", "Manual Title"); + + expect(mockRenameSession).toHaveBeenCalledWith( + "session-1", + "Manual Title", + ); + expect( + useChatSessionStore.getState().getSession("session-1"), + ).toMatchObject({ + title: "Manual Title", + userSetName: true, + }); + }); + + it("does not patch local state when backend rename fails", async () => { + seedSession({ userSetName: false }); + mockRenameSession.mockRejectedValue(new Error("rename failed")); + + await expect( + updateSessionTitle("session-1", "Manual Title"), + ).rejects.toThrow("rename failed"); + + expect( + useChatSessionStore.getState().getSession("session-1"), + ).toMatchObject({ + title: "Original Title", + userSetName: false, + }); + }); + }); + + describe("updateSessionProject", () => { + it("updates project in backend before patching local state", async () => { + seedSession({ projectId: "project-old" }); + mockUpdateSessionProject.mockResolvedValue(undefined); + + await updateSessionProject("session-1", "project-new"); + + expect(mockUpdateSessionProject).toHaveBeenCalledWith( + "session-1", + "project-new", + ); + expect( + useChatSessionStore.getState().getSession("session-1")?.projectId, + ).toBe("project-new"); + }); + + it("does not patch local state when backend project update fails", async () => { + seedSession({ projectId: "project-old" }); + mockUpdateSessionProject.mockRejectedValue(new Error("project failed")); + + await expect( + updateSessionProject("session-1", "project-new"), + ).rejects.toThrow("project failed"); + + expect( + useChatSessionStore.getState().getSession("session-1")?.projectId, + ).toBe("project-old"); + }); + }); +}); diff --git a/ui/goose2/src/features/chat/stores/__tests__/chatSessionStore.test.ts b/ui/goose2/src/features/chat/stores/__tests__/chatSessionStore.test.ts index 735be14993..d7f708eb22 100644 --- a/ui/goose2/src/features/chat/stores/__tests__/chatSessionStore.test.ts +++ b/ui/goose2/src/features/chat/stores/__tests__/chatSessionStore.test.ts @@ -225,54 +225,32 @@ describe("chatSessionStore", () => { }); }); - describe("updateSession", () => { - it("updates session properties", () => { + describe("patchSession", () => { + it("patches session properties while preserving updatedAt when omitted", () => { const session = seedSession(); + const originalUpdatedAt = session.updatedAt; - useChatSessionStore.getState().updateSession(session.id, { + useChatSessionStore.getState().patchSession(session.id, { title: "Updated Title", projectId: "new-project", }); const updated = useChatSessionStore.getState().getSession(session.id); - expect(updated?.title).toBe("Updated Title"); - expect(updated?.projectId).toBe("new-project"); - }); - - it("preserves updatedAt when not explicitly provided in patch", () => { - const session = seedSession(); - const originalUpdatedAt = session.updatedAt; - - vi.useFakeTimers(); - vi.advanceTimersByTime(1000); - - useChatSessionStore.getState().updateSession(session.id, { - title: "New Title", + expect(updated).toMatchObject({ + title: "Updated Title", + projectId: "new-project", + updatedAt: originalUpdatedAt, }); - - vi.useRealTimers(); - - const updated = useChatSessionStore.getState().getSession(session.id); - expect(updated?.updatedAt).toBe(originalUpdatedAt); }); it("updates updatedAt when explicitly provided in patch", () => { const session = seedSession(); - const originalUpdatedAt = session.updatedAt; - - vi.useFakeTimers(); - vi.advanceTimersByTime(1000); - - const newTimestamp = new Date().toISOString(); - useChatSessionStore.getState().updateSession(session.id, { - title: "New Title", + const newTimestamp = "2026-04-01T00:01:00.000Z"; + useChatSessionStore.getState().patchSession(session.id, { updatedAt: newTimestamp, }); - vi.useRealTimers(); - const updated = useChatSessionStore.getState().getSession(session.id); - expect(updated?.updatedAt).not.toBe(originalUpdatedAt); expect(updated?.updatedAt).toBe(newTimestamp); }); }); diff --git a/ui/goose2/src/features/chat/stores/chatSelectors.ts b/ui/goose2/src/features/chat/stores/chatSelectors.ts new file mode 100644 index 0000000000..a86bb67d0a --- /dev/null +++ b/ui/goose2/src/features/chat/stores/chatSelectors.ts @@ -0,0 +1,7 @@ +import type { ChatStore } from "./chatStore"; + +export const selectMessagesBySession = (state: ChatStore) => + state.messagesBySession; + +export const selectSessionStateById = (state: ChatStore) => + state.sessionStateById; diff --git a/ui/goose2/src/features/chat/stores/chatSessionOperations.ts b/ui/goose2/src/features/chat/stores/chatSessionOperations.ts new file mode 100644 index 0000000000..e0fab0c7ce --- /dev/null +++ b/ui/goose2/src/features/chat/stores/chatSessionOperations.ts @@ -0,0 +1,28 @@ +import { + renameSession, + updateSessionProject as updateSessionProjectApi, +} from "@/shared/api/acpApi"; +import { useChatSessionStore } from "./chatSessionStore"; + +export async function updateSessionTitle( + sessionId: string, + title: string, +): Promise { + await renameSession(sessionId, title); + + useChatSessionStore.getState().patchSession(sessionId, { + title, + userSetName: true, + }); +} + +export async function updateSessionProject( + sessionId: string, + projectId: string | null, +): Promise { + await updateSessionProjectApi(sessionId, projectId); + + useChatSessionStore.getState().patchSession(sessionId, { + projectId, + }); +} diff --git a/ui/goose2/src/features/chat/stores/chatSessionSelectors.ts b/ui/goose2/src/features/chat/stores/chatSessionSelectors.ts new file mode 100644 index 0000000000..7104691702 --- /dev/null +++ b/ui/goose2/src/features/chat/stores/chatSessionSelectors.ts @@ -0,0 +1,12 @@ +import type { ChatSessionStore } from "./chatSessionStore"; + +export const selectSessions = (state: ChatSessionStore) => state.sessions; + +export const selectActiveSessionId = (state: ChatSessionStore) => + state.activeSessionId; + +export const selectHasHydratedSessions = (state: ChatSessionStore) => + state.hasHydratedSessions; + +export const selectSessionsLoading = (state: ChatSessionStore) => + state.isLoading; diff --git a/ui/goose2/src/features/chat/stores/chatSessionStore.ts b/ui/goose2/src/features/chat/stores/chatSessionStore.ts index a421d6a0df..d49e2b1dbc 100644 --- a/ui/goose2/src/features/chat/stores/chatSessionStore.ts +++ b/ui/goose2/src/features/chat/stores/chatSessionStore.ts @@ -12,8 +12,6 @@ import { import { archiveSession as acpArchiveSession, unarchiveSession as acpUnarchiveSession, - renameSession as acpRenameSession, - updateSessionProject, } from "@/shared/api/acpApi"; export interface ChatSession { @@ -77,7 +75,7 @@ interface CreateSessionOpts { interface ChatSessionStoreActions { createSession: (opts?: CreateSessionOpts) => Promise; loadSessions: () => Promise; - updateSession: (id: string, patch: Partial) => void; + patchSession: (id: string, patch: Partial) => void; addSession: (session: ChatSession) => void; archiveSession: (id: string) => Promise; unarchiveSession: (id: string) => Promise; @@ -195,7 +193,7 @@ export const useChatSessionStore = create((set, get) => ({ } }, - updateSession: (id, patch) => { + patchSession: (id, patch) => { set((state) => ({ sessions: state.sessions.map((session) => session.id === id @@ -207,29 +205,6 @@ export const useChatSessionStore = create((set, get) => ({ : session, ), })); - - const updatedSession = get().sessions.find((session) => session.id === id); - - // Persist title rename to backend - if ( - "title" in patch && - "userSetName" in patch && - patch.userSetName && - updatedSession && - patch.title - ) { - acpRenameSession(updatedSession.id, patch.title).catch((err: unknown) => - console.error("Failed to rename session in backend:", err), - ); - } - - // Persist projectId change to backend - if ("projectId" in patch && updatedSession) { - updateSessionProject(updatedSession.id, patch.projectId ?? null).catch( - (err: unknown) => - console.error("Failed to update session project in backend:", err), - ); - } }, addSession: (session) => { diff --git a/ui/goose2/src/features/projects/stores/projectSelectors.ts b/ui/goose2/src/features/projects/stores/projectSelectors.ts new file mode 100644 index 0000000000..485597d3e4 --- /dev/null +++ b/ui/goose2/src/features/projects/stores/projectSelectors.ts @@ -0,0 +1,3 @@ +import type { ProjectStore } from "./projectStore"; + +export const selectProjects = (state: ProjectStore) => state.projects; diff --git a/ui/goose2/src/features/projects/stores/projectStore.ts b/ui/goose2/src/features/projects/stores/projectStore.ts index 931b7a02f3..a0bdfa28b2 100644 --- a/ui/goose2/src/features/projects/stores/projectStore.ts +++ b/ui/goose2/src/features/projects/stores/projectStore.ts @@ -34,7 +34,7 @@ function persistProjects(projects: ProjectInfo[]): void { } } -interface ProjectState { +export interface ProjectStore { projects: ProjectInfo[]; loading: boolean; activeProjectId: string | null; @@ -70,7 +70,7 @@ interface ProjectState { getActiveProject: () => ProjectInfo | null; } -export const useProjectStore = create((set, get) => ({ +export const useProjectStore = create((set, get) => ({ projects: loadCachedProjects(), loading: false, activeProjectId: null, diff --git a/ui/goose2/src/features/sessions/ui/SessionHistoryView.tsx b/ui/goose2/src/features/sessions/ui/SessionHistoryView.tsx index 6b001b4881..d9d2197bb6 100644 --- a/ui/goose2/src/features/sessions/ui/SessionHistoryView.tsx +++ b/ui/goose2/src/features/sessions/ui/SessionHistoryView.tsx @@ -12,8 +12,11 @@ import { getVisibleSessions, useChatSessionStore, } from "@/features/chat/stores/chatSessionStore"; +import { selectSessions } from "@/features/chat/stores/chatSessionSelectors"; import { useChatStore } from "@/features/chat/stores/chatStore"; +import { selectMessagesBySession } from "@/features/chat/stores/chatSelectors"; import { useProjectStore } from "@/features/projects/stores/projectStore"; +import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { acpDuplicateSession, acpExportSession, @@ -41,8 +44,8 @@ export function SessionHistoryView({ onArchiveChat, }: SessionHistoryViewProps) { const { t, i18n } = useTranslation(["sessions", "common"]); - const sessions = useChatSessionStore((s) => s.sessions); - const messagesBySession = useChatStore((s) => s.messagesBySession); + const sessions = useChatSessionStore(selectSessions); + const messagesBySession = useChatStore(selectMessagesBySession); const loadSessions = useChatSessionStore((s) => s.loadSessions); const activeSessions = useMemo( () => @@ -59,7 +62,7 @@ export function SessionHistoryView({ [], ); - const projects = useProjectStore((s) => s.projects); + const projects = useProjectStore(selectProjects); const getProjectName = useCallback( (projectId: string) => projects.find((p) => p.id === projectId)?.name, [projects], diff --git a/ui/goose2/src/features/sidebar/ui/Sidebar.tsx b/ui/goose2/src/features/sidebar/ui/Sidebar.tsx index 2e600d34ad..ae0a459ae5 100644 --- a/ui/goose2/src/features/sidebar/ui/Sidebar.tsx +++ b/ui/goose2/src/features/sidebar/ui/Sidebar.tsx @@ -17,13 +17,20 @@ import { cn } from "@/shared/lib/cn"; import type { AppView } from "@/app/AppShell"; import type { ProjectInfo } from "@/features/projects/api/projects"; import { useChatStore } from "@/features/chat/stores/chatStore"; +import { + selectMessagesBySession, + selectSessionStateById, +} from "@/features/chat/stores/chatSelectors"; +import { INITIAL_SESSION_CHAT_RUNTIME } from "@/shared/types/chat"; import { getVisibleSessions, useChatSessionStore, } from "@/features/chat/stores/chatSessionStore"; +import { selectSessions } from "@/features/chat/stores/chatSessionSelectors"; import { isSessionRunning } from "@/features/chat/lib/sessionActivity"; import { useAgentStore } from "@/features/agents/stores/agentStore"; import { useProjectStore } from "@/features/projects/stores/projectStore"; +import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { Button } from "@/shared/ui/button"; import { useSessionSearch } from "@/features/sessions/hooks/useSessionSearch"; import { SidebarProjectsSection } from "./SidebarProjectsSection"; @@ -101,12 +108,12 @@ export function Sidebar({ } }); - const chatStore = useChatStore(); - const { sessions } = useChatSessionStore(); - const visibleSessions = getVisibleSessions( - sessions, - chatStore.messagesBySession, - ); + const messagesBySession = useChatStore(selectMessagesBySession); + const sessionStateById = useChatStore(selectSessionStateById); + const sessions = useChatSessionStore(selectSessions); + const getPersonaById = useAgentStore((s) => s.getPersonaById); + const projectStoreProjects = useProjectStore(selectProjects); + const visibleSessions = getVisibleSessions(sessions, messagesBySession); const activeSessions = visibleSessions.filter( (session) => !session.archivedAt, ); @@ -162,7 +169,8 @@ export function Sidebar({ const standalone: SessionItem[] = []; for (const session of visibleSessions) { if (session.archivedAt) continue; - const runtime = chatStore.getSessionRuntime(session.id); + const runtime = + sessionStateById[session.id] ?? INITIAL_SESSION_CHAT_RUNTIME; const item: SessionItem = { id: session.id, title: session.title, @@ -194,15 +202,11 @@ export function Sidebar({ return { byProject, standalone: limitedStandalone }; })(); - const agentStoreState = useAgentStore(); - const projectStoreState = useProjectStore(); - const sidebarResolvers = { getPersonaName: (personaId: string) => - agentStoreState.getPersonaById(personaId)?.displayName, + getPersonaById(personaId)?.displayName, getProjectName: (projectId: string) => - projectStoreState.projects.find((p: { id: string }) => p.id === projectId) - ?.name, + projectStoreProjects.find((p) => p.id === projectId)?.name, }; const sidebarSearch = useSessionSearch({ sessions: activeSessions, diff --git a/ui/goose2/src/features/sidebar/ui/__tests__/Sidebar.test.tsx b/ui/goose2/src/features/sidebar/ui/__tests__/Sidebar.test.tsx index db8eee27c9..1525a45aa0 100644 --- a/ui/goose2/src/features/sidebar/ui/__tests__/Sidebar.test.tsx +++ b/ui/goose2/src/features/sidebar/ui/__tests__/Sidebar.test.tsx @@ -13,33 +13,34 @@ const mockSessions: Array<{ }> = []; vi.mock("@/features/chat/stores/chatStore", () => ({ - useChatStore: () => ({ - messagesBySession: {}, - getSessionRuntime: () => ({ - chatState: "idle", - hasUnread: false, + useChatStore: (selector: (state: unknown) => unknown) => + selector({ + messagesBySession: {}, + sessionStateById: {}, }), - }), })); vi.mock("@/features/chat/stores/chatSessionStore", () => ({ getVisibleSessions: (sessions: typeof mockSessions) => sessions.filter((session) => session.messageCount > 0), - useChatSessionStore: () => ({ - sessions: mockSessions, - }), + useChatSessionStore: (selector: (state: unknown) => unknown) => + selector({ + sessions: mockSessions, + }), })); vi.mock("@/features/agents/stores/agentStore", () => ({ - useAgentStore: () => ({ - getPersonaById: () => undefined, - }), + useAgentStore: (selector: (state: unknown) => unknown) => + selector({ + getPersonaById: () => undefined, + }), })); vi.mock("@/features/projects/stores/projectStore", () => ({ - useProjectStore: () => ({ - projects: [], - }), + useProjectStore: (selector: (state: unknown) => unknown) => + selector({ + projects: [], + }), })); describe("Sidebar", () => { diff --git a/ui/goose2/src/features/skills/ui/SkillsView.tsx b/ui/goose2/src/features/skills/ui/SkillsView.tsx index 846d59104a..128c5a2a09 100644 --- a/ui/goose2/src/features/skills/ui/SkillsView.tsx +++ b/ui/goose2/src/features/skills/ui/SkillsView.tsx @@ -3,6 +3,7 @@ import { useTranslation } from "react-i18next"; import { Plus, Upload } from "lucide-react"; import { toast } from "sonner"; import { useProjectStore } from "@/features/projects/stores/projectStore"; +import { selectProjects } from "@/features/projects/stores/projectSelectors"; import { Button } from "@/shared/ui/button"; import { PageHeader, PageShell } from "@/shared/ui/page-shell"; import { revealInFileManager } from "@/shared/lib/fileManager"; @@ -32,7 +33,7 @@ interface SkillsViewProps { export function SkillsView({ onStartChatWithSkill }: SkillsViewProps) { const { t } = useTranslation(["skills", "common"]); - const projects = useProjectStore((state) => state.projects); + const projects = useProjectStore(selectProjects); const [search, setSearch] = useState(""); const [activeFilter, setActiveFilter] = useState("all"); const [dialogOpen, setDialogOpen] = useState(false); diff --git a/ui/goose2/src/shared/api/acpNotificationHandler.ts b/ui/goose2/src/shared/api/acpNotificationHandler.ts index 2f60a91413..5ca3bc589e 100644 --- a/ui/goose2/src/shared/api/acpNotificationHandler.ts +++ b/ui/goose2/src/shared/api/acpNotificationHandler.ts @@ -506,7 +506,7 @@ function handleShared(sessionId: string, update: SessionUpdate): void { currentModelId; const sessionStore = useChatSessionStore.getState(); - sessionStore.updateSession(sessionId, { + sessionStore.patchSession(sessionId, { modelId: currentModelId, modelName: currentModelName, }); diff --git a/ui/goose2/src/shared/api/acpSessionInfoUpdate.ts b/ui/goose2/src/shared/api/acpSessionInfoUpdate.ts index 252a5ce4cb..e7c23b075e 100644 --- a/ui/goose2/src/shared/api/acpSessionInfoUpdate.ts +++ b/ui/goose2/src/shared/api/acpSessionInfoUpdate.ts @@ -24,7 +24,7 @@ export function handleSessionInfoUpdate( } const meta = isRecord(info._meta) ? info._meta : {}; - const patch: Parameters[1] = {}; + const patch: Parameters[1] = {}; if (typeof info.title === "string" && info.title && !session.userSetName) { patch.title = info.title; @@ -40,6 +40,6 @@ export function handleSessionInfoUpdate( } if (Object.keys(patch).length > 0) { - sessionStore.updateSession(sessionId, patch); + sessionStore.patchSession(sessionId, patch); } } diff --git a/ui/goose2/src/shared/i18n/locales/en/chat.json b/ui/goose2/src/shared/i18n/locales/en/chat.json index 58e9a3f0f8..976f26d884 100644 --- a/ui/goose2/src/shared/i18n/locales/en/chat.json +++ b/ui/goose2/src/shared/i18n/locales/en/chat.json @@ -119,7 +119,9 @@ "filesTitle": "Files" }, "notifications": { - "compactionComplete": "Conversation compacted. Older context was summarized." + "compactionComplete": "Conversation compacted. Older context was summarized.", + "moveError": "Failed to move chat", + "renameError": "Failed to rename chat" }, "message": { "copied": "Copied", diff --git a/ui/goose2/src/shared/i18n/locales/es/chat.json b/ui/goose2/src/shared/i18n/locales/es/chat.json index e710e116cc..16e6b70ede 100644 --- a/ui/goose2/src/shared/i18n/locales/es/chat.json +++ b/ui/goose2/src/shared/i18n/locales/es/chat.json @@ -119,7 +119,9 @@ "filesTitle": "Archivos" }, "notifications": { - "compactionComplete": "Conversacion compactada. El contexto anterior se resumio." + "compactionComplete": "Conversacion compactada. El contexto anterior se resumio.", + "moveError": "No se pudo mover el chat", + "renameError": "No se pudo cambiar el nombre del chat" }, "message": { "copied": "Copiado", diff --git a/ui/goose2/src/shared/lib/arrays.ts b/ui/goose2/src/shared/lib/arrays.ts deleted file mode 100644 index 6d5f406e1e..0000000000 --- a/ui/goose2/src/shared/lib/arrays.ts +++ /dev/null @@ -1,10 +0,0 @@ -/** Find the last index in an array matching a predicate. */ -export function findLastIndex( - arr: readonly T[], - predicate: (item: T) => boolean, -): number { - for (let i = arr.length - 1; i >= 0; i--) { - if (predicate(arr[i])) return i; - } - return -1; -} diff --git a/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-improvement-plan.md b/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-improvement-plan.md new file mode 100644 index 0000000000..9218e199fa --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-improvement-plan.md @@ -0,0 +1,464 @@ +**Goose2 Zustand State Management Improvement Plan** + +This plan translates the findings from [goose2-zustand-state-management-review.md](/Users/lifei/Development/goose/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-review.md) into a concrete implementation sequence. + +The plan is optimized for: +- improving maintainability +- reducing future bug risk +- minimizing unnecessary churn +- respecting code dependencies and migration order + +This is a single master plan. It is intentionally sequenced so that higher-leverage, lower-risk changes happen before larger store refactors. + +**Goals** + +- Establish selector-first Zustand usage in React code. +- Reduce coupling between consumers and broad store shapes. +- Remove hidden backend side effects from generic store actions. +- Improve store responsibility boundaries. +- Standardize persistence and mutation policy. +- Improve test reset discipline and coverage for risky flows. +- Only use `useShallow` and Immer where they materially improve maintainability. + +**Sequencing Principles** + +- Fix read patterns before splitting stores. +- Separate side effects from generic store actions before redesigning store boundaries. +- Split the clearest UI/domain boundaries first. +- Standardize persistence only after the intended state boundaries are clearer. +- Improve tests alongside each change, not only at the end. +- Treat Immer as a late readability improvement, not an architectural fix. + +**High-Level Phase Order** + +1. Remove whole-store subscriptions and stabilize store consumption. +2. Introduce a selector-first read layer and add `useShallow` where appropriate. +3. Separate backend side effects from generic store actions. +4. Split the broadest stores by responsibility. +5. Decide chat-session workflow failure policy. +6. Refactor `projectStore` into clearer layers. +7. Standardize persistence boundaries. +8. Standardize store reset/testing patterns and close coverage gaps. +9. Optionally adopt Immer for nested update-heavy stores. + +**Priority Snapshot** + +**Highest priority** +- Phase 1: whole-store subscriptions +- Phase 2: selector-first read layer +- Phase 3: hidden backend side effects in `chatSessionStore` + +**Medium priority** +- Phase 4: split `agentStore` UI state and `chatSessionStore` UI state +- Phase 5: decide chat-session workflow failure policy +- Phase 6: refactor `projectStore` +- Phase 8: testing/reset cleanup for touched stores + +**Lower priority but useful** +- Phase 7: persistence standardization rollout +- Phase 9: selective Immer adoption + +**Dependency Logic** + +- Phases 1 and 2 come first because they reduce consumer coupling and make later store splits safer. +- Phase 3 comes before major store splits because current action semantics are one of the biggest bug risks. +- Phase 4 is safer after read patterns and action semantics are clearer. +- Phase 5 follows Phase 4 so archive/unarchive workflow policy can be decided after session-store boundaries are clearer. +- Phase 6 depends on lessons from earlier store cleanup and should not be the first major refactor. +- Phase 7 depends on clearer decisions about what state is truly durable. +- Phase 8 should happen incrementally, but a dedicated cleanup pass is still needed. +- Phase 9 is intentionally last because it improves update ergonomics, not architecture. + +**Phase 1: Remove Whole-Store Subscriptions** + +**Goal** +- Replace broad bound-store subscriptions with explicit selectors. + +**Why first** +- This is the highest-leverage, lowest-risk improvement. +- It reduces rerender noise and decouples components/hooks from full store shapes. +- It makes later store changes less invasive. + +**Primary files** +- [AppShell.tsx](/Users/lifei/Development/goose/ui/goose2/src/app/AppShell.tsx:80) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:103) +- [usePersonas.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/usePersonas.ts:12) +- [useChat.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChat.ts:108) + +**Detailed tasks** +- `AppShell.tsx` + - Replace `useChatStore()`, `useChatSessionStore()`, `useAgentStore()`, and `useProjectStore()` broad subscriptions with specific selectors. + - Avoid selecting whole store objects just to access a few fields or actions. + - Keep legitimate `getState()` usage in async helper callbacks where necessary. +- `Sidebar.tsx` + - Replace whole `chatStore`, `agentStoreState`, and `projectStoreState` subscriptions with targeted selectors. + - Avoid broad subscription combined with heavy render-time derivation. +- `usePersonas.ts` + - Replace `const store = useAgentStore()` with explicit state/action selectors. + - Stop relying on the “store object is stable” assumption in hook callbacks. +- `useChat.ts` + - Replace whole-store subscription with session-scoped selectors for messages/runtime plus specific actions. + +**Success criteria** +- No high-level React component or hook uses `useSomeStore()` with no selector. +- Components subscribe only to the fields/actions they actually need. + +**Phase 2: Introduce a Selector-First Read Layer** + +**Goal** +- Standardize how React code reads Zustand state. + +**Why second** +- After Phase 1, repeated selector logic will become visible. +- This is the right time to define reusable selectors and add `useShallow` only where needed. + +**Primary files to add or refactor** +- `src/features/chat/stores/chatSelectors.ts` +- `src/features/chat/stores/chatSessionSelectors.ts` +- `src/features/agents/stores/agentSelectors.ts` +- `src/features/projects/stores/projectSelectors.ts` +- [useProviderInventory.ts](/Users/lifei/Development/goose/ui/goose2/src/features/providers/hooks/useProviderInventory.ts:29) + +**Detailed tasks** +- Create selector helpers for repeated reads: + - session lists and active session id + - session runtime by id + - visible messages for session + - project list and project lookup helpers + - agent/persona/provider simple reads + - provider selection state +- Keep active session, Home session, active project, and similar composed values derived from existing state. Do not add duplicate store attributes just to make reads easier. +- Introduce grouped selectors with `useShallow` where components need multiple values together. +- Keep primitive selectors simple and do not wrap them in `useShallow`. +- Refactor any repetitive inline lookup logic into selector helpers or pure `lib/` helpers if it is not inherently store-specific. + +**Where `useShallow` should be used** +- grouped object selectors in high-level consumers +- derived array/object selectors where reference churn would otherwise trigger unnecessary rerenders + +**Where `useShallow` should not be used** +- primitive selectors +- as a substitute for selector discipline +- to compensate for weak store boundaries + +**Success criteria** +- Common reads have reusable selector helpers. +- `useShallow` appears only on object/array selectors where it adds value. +- React read patterns are visibly more consistent across features. + +**Phase 3: Separate Backend Side Effects From Generic Store Actions** + +**Goal** +- Make mutation semantics explicit and reduce hidden side effects. + +**Why third** +- This is one of the highest bug-risk areas. +- It is easier to split stores safely after action semantics are cleaned up. + +**Primary file** +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:195) + +**Current problem** +- `updateSession` looks like a local patch action but can also trigger backend rename/project-update side effects. + +**Detailed tasks** +- Split local state patching from backend persistence behavior. +- Keep a truly local session patch action in the store. +- Move remote mutation behavior into explicit orchestration functions or hooks. +- Introduce clearly named operations for: + - rename session and persist + - update session project and persist + - archive session + - unarchive session +- Audit current call sites that assume `updateSession` is the main entry point: + - [useChat.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChat.ts:215) + - [useResolvedAgentModelPicker.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts:279) + - [useChatSessionController.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChatSessionController.ts:314) + - [useChatSessionController.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChatSessionController.ts:711) + +**Possible target modules** +- `src/features/chat/api/` for backend-facing operations +- `src/features/chat/hooks/` for orchestration wrappers + +**Success criteria** +- Generic local patch actions are local only. +- Backend writes happen through explicitly named orchestration functions. +- Mutation semantics are easier to reason about at call sites. + +**Phase 4: Split the Broadest Stores by Responsibility** + +**Goal** +- Reduce mixed responsibilities inside stores and isolate UI-only state from domain state where practical. + +**Why fourth** +- Consumer coupling and action semantics should be cleaner before changing boundaries. + +**AgentStore Boundary Work** + +**Primary file** +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:34) + +**Current mix** +- personas +- agents +- providers +- selected provider +- active agent +- persona editor modal state + +**Detailed tasks** +- Keep catalog/domain concerns together initially: + - personas + - agents + - providers + - selected provider + - active agent if still truly shared +- Move editor/modal concerns out: + - `personaEditorOpen` + - `editingPersona` + - `personaEditorMode` +- Update affected consumers: + - [AgentsView.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/agents/ui/AgentsView.tsx:44) + - [useChatSessionController.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChatSessionController.ts:601) + +**Possible target files** +- `agentCatalogStore.ts` +- `agentUiStore.ts` + +**ChatSessionStore Boundary Work** + +**Primary file** +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:55) + +**Current mix** +- session records +- active session selection +- context panel open state +- active workspace UI state + +**Detailed tasks** +- Keep session data in the session store. +- Move UI-only keyed state out: + - `contextPanelOpenBySession` + - `activeWorkspaceBySession` +- Update affected consumers: + - [ChatView.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/chat/ui/ChatView.tsx:30) + - [ContextPanel.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/chat/ui/ContextPanel.tsx:44) + - [useChatSessionController.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChatSessionController.ts:56) + - [useChat.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChat.ts:386) + +**Possible target files** +- `chatSessionStore.ts` +- `chatSessionUiStore.ts` + +**ChatStore Re-Evaluation** + +**Primary file** +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:39) + +**Current mix** +- messages +- runtime +- queue +- drafts +- connection state +- loading/replay state +- scroll targeting +- cleanup + +**Detailed tasks** +- Do not split this immediately unless the earlier Phase 4 boundary work is already complete. +- Reassess whether the app needs a separation between: + - message state + - runtime state + - composer/draft state +- Base the split on actual selector usage after cleanup, not on theoretical purity. + +**Success criteria for Phase 4** +- UI-only state is no longer co-located with unrelated domain-heavy stores where that separation is clear and useful. +- Store boundaries reflect actual shared-state responsibilities more closely. + +**Phase 5: Decide Chat Session Workflow Failure Policy** + +**Goal** +- Make chat-session workflow failure behavior explicit after Phase 4 clarifies session-store boundaries. + +**Why this phase matters** +- Phase 3 removes hidden side effects from generic session patching. +- Phase 4 clarifies which session state should remain in `chatSessionStore`. +- Archive/unarchive still have API calls in the store and currently use optimistic-without-rollback behavior. +- Failure-policy changes are user-visible, so they should be handled separately from the mechanical side-effect extraction. + +**Detailed tasks** +- Review title/project workflows introduced in Phase 3. +- Revisit archive/unarchive after Phase 4. +- Decide policy per workflow: + - backend-first + - optimistic without rollback + - optimistic with rollback + - refresh-on-failure + - optimistic with user-visible error +- Add focused tests for selected success/failure behavior. + +**Success criteria** +- Each chat-session workflow has one clear implementation point. +- Local/backend consistency behavior is intentional and tested. + +**Phase 6: Refactor `projectStore` Into Clearer Layers** + +**Goal** +- Reduce architectural overreach in the project feature state layer. + +**Why this phase matters** +- `projectStore` currently combines state, cache hydration, persistence, CRUD orchestration, and optimistic mutation policy. +- That makes project behavior harder to reason about and harder to test than the other feature stores. +- This phase should happen after the earlier selector and store-boundary cleanup so the project refactor is not fighting broad consumer coupling at the same time. + +**Primary file** +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:73) + +**Current mix** +- project data +- loading state +- local cache hydration/persistence +- CRUD orchestration +- optimistic reorder behavior +- active selection + +**Detailed tasks** +- Keep project state transitions in the store. +- Move API orchestration into explicit project commands/hooks. +- Make reorder policy explicit. +- Decide and document whether reorder is: + - optimistic with rollback + - optimistic with refresh + - pessimistic +- Reduce direct persistence coupling in the store so persistence can later be standardized. + +**Likely support files** +- `src/features/projects/hooks/useProjectCommands.ts` +- `src/features/projects/stores/projectSelectors.ts` + +**Likely consumer files to revisit** +- [ProjectsView.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/projects/ui/ProjectsView.tsx:89) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:192) +- [SettingsModal.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/settings/ui/SettingsModal.tsx:107) + +**Success criteria** +- Clearer source-of-truth behavior for projects. +- Explicit mutation policy for reorder and CRUD flows. +- Less orchestration logic living in the store itself. + +**Phase 7: Standardize Persistence Boundaries** + +**Goal** +- Replace ad hoc per-store durability choices with a clearer persistence policy. + +**Why this phase matters** +- Persistence choices currently live in scattered store-local helpers and are not governed by a shared rule. +- That makes hydration behavior, migration behavior, and “what should survive reloads” decisions inconsistent. +- This phase comes after boundary cleanup because persistence should reflect the intended final ownership of state, not the current mixed-responsibility shape. + +**Primary files** +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:11) +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:5) +- [draftPersistence.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/draftPersistence.ts:1) + +**Detailed tasks** +- Decide what state is truly durable. +- Move eligible store-backed durability to Zustand `persist` where appropriate. +- Use `partialize` and versioning for persisted stores. +- Keep non-store persistence explicit if it truly belongs outside Zustand. + +**Good persistence candidates** +- selected provider +- chat drafts if intentionally durable +- possibly project cache if cached bootstrap remains a product decision + +**Avoid persisting by default** +- connection state +- transient runtime state +- temporary UI interaction state + +**Success criteria** +- durability rules are explicit +- persisted state is minimal and intentional +- persistence code is no longer scattered ad hoc through stores + +**Phase 8: Standardize Test Reset Patterns and Close Coverage Gaps** + +**Goal** +- Make store tests safer and add coverage to the riskiest state-management paths. + +**Why this phase matters** +- The current tests prove some important store behavior, but reset discipline is inconsistent and some of the riskiest stores are still under-covered. +- As the earlier phases change boundaries and mutation semantics, test quality becomes more important, not less. +- This phase ensures the refactor leaves the state layer easier to trust and easier to evolve. + +**Primary files** +- [agentStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/__tests__/agentStore.test.ts:37) +- [usePersonas.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/__tests__/usePersonas.test.ts:53) +- [chatStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/__tests__/chatStore.test.ts:22) +- additional chat hook tests that use partial `setState(...)` setup patterns + +**Detailed tasks** +- Introduce initial-state reset helpers for each store. +- Stop relying on partial shallow-merge resets as the standard setup pattern. +- Apply the same reset discipline to hook tests that manipulate stores directly. + +**Coverage gaps to address** +- `projectStore` + - cache hydration + - reorder semantics + - mutation failure/reconciliation policy +- `providerInventoryStore` + - merge semantics + - loading behavior +- session mutation orchestration introduced in Phase 3 + +**Success criteria** +- tests reset stores from a known initial state +- risky persistence and mutation behaviors have direct coverage + +**Phase 9: Optional Immer Adoption for Nested Update Ergonomics** + +**Goal** +- Improve readability where nested immutable updates remain noisy after structural cleanup. + +**Why this phase matters** +- Immer is not a structural fix, so it should not be introduced before the higher-value boundary and side-effect problems are addressed. +- After the earlier phases, it becomes much easier to judge whether nested update boilerplate is still a real maintainability cost. +- Keeping this phase late prevents the team from mistaking update ergonomics for architectural improvement. + +**Primary candidate** +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:104) + +**Secondary candidate** +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:137) + +**Detailed tasks** +- Reassess nested update-heavy store actions after earlier phases. +- Only adopt Immer if update readability remains a real pain point. +- Do not use Immer as a justification to keep a broad or mixed-responsibility store intact. + +**Success criteria** +- update logic is easier to read without obscuring ownership boundaries + +**Suggested PR Breakdown** + +1. PR 1: selector cleanup in `AppShell`, `Sidebar`, `usePersonas`, `useChat` +2. PR 2: selector helpers + selective `useShallow` +3. PR 3: split `chatSessionStore` local patching from backend side effects +4. PR 4: extract `agentUiStore` from `agentStore` +5. PR 5: extract `chatSessionUiStore` from `chatSessionStore` +6. PR 6: refactor `projectStore` orchestration and mutation policy +7. PR 7: persistence standardization +8. PR 8: reset helpers + coverage expansion +9. PR 9: optional Immer cleanup for nested update-heavy stores + +**Plan Usage** + +- Use this as the master sequencing document. +- Do not try to execute every phase in a single refactor. +- For implementation, create smaller phase-specific execution notes only when a phase is about to start. +- Re-check the plan after Phase 3 and again after Phase 6, because store boundaries and actual consumer patterns may change enough to adjust later phases. diff --git a/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-review.md b/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-review.md new file mode 100644 index 0000000000..6f44447912 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/goose2-zustand-state-management-review.md @@ -0,0 +1,384 @@ +**Goose2 Zustand Review: Code Quality + Architecture** + +If the bar is “easy to maintain later” and “low future bug surface,” the current Zustand usage in `ui/goose2` does **not** meet that bar yet. + +This review combines two perspectives: +- strict Zustand usage and code-quality review +- higher-level architectural review of how Zustand is being used in the frontend + +The conclusions are based on reading the current `ui/goose2` stores, major consumers, and store tests. + +Primary reference points from the official Zustand docs: +- Intro and selector usage: https://zustand.docs.pmnd.rs/getting-started/introduction +- Flux-inspired practice: https://zustand.docs.pmnd.rs/learn/guides/flux-inspired-practice +- Slices pattern: https://zustand.docs.pmnd.rs/learn/guides/slices-pattern +- Auto-generating selectors: https://zustand.docs.pmnd.rs/learn/guides/auto-generating-selectors +- `useShallow`: https://zustand.docs.pmnd.rs/learn/guides/prevent-rerenders-with-use-shallow +- `persist`: https://zustand.docs.pmnd.rs/reference/middlewares/persist +- Reset pattern: https://zustand.docs.pmnd.rs/learn/guides/how-to-reset-state +- `Map` / `Set` usage: https://zustand.docs.pmnd.rs/learn/guides/maps-and-sets-usage + +**Overall Verdict** + +`ui/goose2` uses Zustand well enough to ship features, but not strictly enough for long-term maintainability. + +The biggest problems are: +1. broad subscriptions to entire stores in React +2. stores with weak responsibility boundaries +3. state stores doubling as workflow and backend side-effect layers +4. ad hoc persistence and inconsistent durability rules +5. insufficiently explicit mutation/reconciliation policy +6. inconsistent testing/reset discipline + +This is not a case of “Zustand is wrong.” It is a case of “the current architecture makes Zustand too easy to use as a global bucket for state, persistence, and orchestration.” + +**What Is Good** + +- State updates mostly go through `set` and are implemented immutably. +- `Map` and `Set` usage is correct where present, because updates create new instances. Examples: [providerInventoryStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/providers/stores/providerInventoryStore.ts:19), [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:452). +- Some consumers already use narrow selectors correctly, for example [useProviderSelection.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/useProviderSelection.ts:1). +- `getState()` usage in non-React async/event code is acceptable and not a problem by itself. +- Important stores like chat and agent stores already have meaningful tests. + +**Detailed Findings** + +1. **Whole-store subscriptions are the clearest Zustand misuse** + +In React code, bound store hooks should not be called without selectors. + +Concrete examples: +- [AppShell.tsx](/Users/lifei/Development/goose/ui/goose2/src/app/AppShell.tsx:80) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:103) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:191) +- [usePersonas.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/usePersonas.ts:12) +- [useChat.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChat.ts:108) + +Why this is a problem: +- unrelated store updates can rerender large components/hooks +- consumers become tightly coupled to the full store shape +- store refactors become expensive because consumers depend on broad internals + +Strict standard: +- no `useSomeStore()` without a selector in React components or hooks + +2. **`useShallow` is underused where it matters** + +There is effectively no `useShallow` usage in the `src/` tree. + +That is not the primary problem, but it is a real gap after selector discipline is fixed. + +Where it would matter: +- grouped selectors returning objects +- computed selectors returning arrays/objects +- high-level components that need a few store fields together + +Good target pattern: +```ts +const { sessions, activeSessionId } = useChatSessionStore( + useShallow((s) => ({ + sessions: s.sessions, + activeSessionId: s.activeSessionId, + })), +); +``` + +Strict standard: +- first adopt selector-first consumption +- then use `useShallow` on object/array selectors +- do not use `useShallow` on primitive selectors + +More specifically, `useShallow` is good for: +- replacing whole-store subscriptions with grouped object selectors in high-level consumers such as [AppShell.tsx](/Users/lifei/Development/goose/ui/goose2/src/app/AppShell.tsx:80), [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:103), [usePersonas.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/usePersonas.ts:12), and [useChat.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/hooks/useChat.ts:108) +- derived object/array selectors in places like [useProviderInventory.ts](/Users/lifei/Development/goose/ui/goose2/src/features/providers/hooks/useProviderInventory.ts:29) + +`useShallow` is not for: +- primitive selectors like `loading`, `activeSessionId`, or `selectedProvider` +- compensating for weak store boundaries or hidden store side effects +- replacing selector discipline with a blanket optimization + +Maintainability conclusion: +- `useShallow` is a good tactical improvement after selector cleanup +- it does not replace the higher-value fixes in this review: selector-first reads, clearer store boundaries, explicit side-effect boundaries, and standardized persistence/reconciliation rules + +3. **Stores are too broad in responsibility** + +Separate stores are not automatically wrong, but several individual stores are carrying too many concerns. + +Examples: +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:39) +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:34) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:55) +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:37) + +What that looks like: +- `chatStore` mixes messages, runtime, drafts, queue, connection state, loading/replay state, scroll targeting, and cleanup +- `agentStore` mixes personas, agents, providers, selected provider, active agent, and persona editor UI state +- `chatSessionStore` mixes session records, active session selection, context-panel state, and workspace UI state +- `projectStore` mixes project records, loading state, persistence, CRUD orchestration, optimistic reordering, and active selection + +Why this is a problem: +- unrelated state changes share subscriber surfaces +- tests become broader than necessary +- small changes require too much knowledge of one store’s internals + +Immer is relevant here only as an update-readability tool, not as a boundary fix. It may improve maintainability in nested update-heavy stores such as [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:104), where updates to `messagesBySession`, `sessionStateById`, `draftsBySession`, and `scrollTargetMessageBySession` currently require substantial object spread boilerplate, and to a lesser extent in [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:137). It would not solve the higher-value problems in this review: broad store responsibilities, workflow/state mixing, hidden backend side effects, or ad hoc persistence. It is also unlikely to add much value to already-flat stores like [providerInventoryStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/providers/stores/providerInventoryStore.ts:19). + +4. **Domain state and UI state are mixed together** + +This is one of the clearest architectural problems. + +Examples: +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:56) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:60) + +Specifically: +- `agentStore` holds persona/agent/provider data and also persona editor modal state +- `chatSessionStore` holds session data and also context panel open state plus per-session workspace UI state + +Why this is a problem: +- UI interaction state and domain data are forced into the same lifecycle +- unrelated UI churn can affect store consumers interested in durable feature data +- state boundaries are harder to reason about + +Strict standard: +- if state is only for one screen or interaction, prefer local state or a focused UI store +- if state is shared across views or sessions, use Zustand +- do not default to putting modal/open-state UI into domain-heavy stores + +5. **Store actions hide backend and persistence side effects** + +The clearest example is `chatSessionStore.updateSession`. + +Code: +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:195) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:211) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:224) + +Why this is a problem: +- `updateSession` sounds like a local patch, but it may also rename a backend session or update the backend project association +- action names stop being truthful about side effects +- rollback, retry, and reconciliation responsibilities become unclear + +Strict standard: +- local patch actions should be local only +- side-effectful operations should be explicit, for example `renameSessionAndPersist` +- transport and orchestration should stay in `api/` modules or dedicated hooks/commands + +6. **Zustand is being used as a workflow layer, not just a state layer** + +This is the main architectural synthesis. + +Examples: +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:73) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:195) +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:415) + +What to look for: +- local storage helpers embedded in stores +- API calls inside store actions +- optimistic update behavior living in the store +- store methods that are really workflow/orchestration entry points + +Architectural conclusion: +- Zustand should primarily own shared client state +- hooks and command-style modules should own workflow/orchestration +- `api/` should own backend transport + +7. **Persistence is hand-rolled where `persist` would provide a cleaner boundary** + +Examples: +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:11) +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:5) +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:18) + +Why this is a problem: +- repeated `localStorage` code +- each store invents its own hydration semantics +- no versioning, migrations, or `partialize` +- persistence rules are not centralized or explicit + +Architectural conclusion: +- durability should be a standard boundary, not a per-store ad hoc decision + +8. **Selectors are not a stable architectural read API yet** + +The codebase often relies on imperative helper methods instead of a selector-oriented read layer. + +Examples: +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:188) +- [agentStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/agentStore.ts:213) +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:181) +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:184) +- [chatStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatStore.ts:191) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:329) + +These helpers are acceptable for `getState()` usage in non-React code, but in React they push the app toward imperative reads instead of reactive selectors. + +Architectural conclusion: +- the app needs a selector-first read API +- common selectors should be promoted into helper hooks +- React components should not depend on imperative store helper methods by default + +9. **Some render paths perform heavy derived work while broadly subscribed** + +The clearest example is the sidebar. + +Code: +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:103) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:145) +- [Sidebar.tsx](/Users/lifei/Development/goose/ui/goose2/src/features/sidebar/ui/Sidebar.tsx:194) + +Why this is a problem: +- broad subscription plus per-render grouping/sorting/runtime lookups compounds rerender cost +- derivation logic is harder to isolate, test, and reuse + +Architectural conclusion: +- central derived state patterns should live in selector helpers or pure `lib/` utilities +- avoid render-time traversal of large store structures when the same logic is core to the feature + +10. **`projectStore` is the strongest example of architectural overreach** + +Code: +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:73) +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:78) +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:157) + +It currently acts as: +- project state store +- local cache owner +- CRUD orchestration layer +- optimistic reorder owner +- active selection owner + +Why this is a problem: +- one module owns too many failure modes +- source-of-truth rules are muddy +- harder to add retries, rollback, or refresh policies later + +11. **Async mutation and reconciliation policy is not explicit enough** + +Examples: +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:157) +- [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:251) + +What to look for: +- local state updated first +- backend call later +- failures mostly logged +- no explicit rollback or refresh strategy encoded in the abstraction + +Architectural conclusion: +- each mutation should clearly be one of: + - pessimistic + - optimistic with rollback + - optimistic with guaranteed refresh +- that policy should be explicit and consistent + +12. **Test reset discipline is not strict enough** + +Examples: +- [agentStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/__tests__/agentStore.test.ts:37) +- [usePersonas.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/__tests__/usePersonas.test.ts:53) +- [chatStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/__tests__/chatStore.test.ts:22) + +Why this is a problem: +- `setState` shallow-merges +- tests sometimes omit fields when resetting state +- stale state can leak across tests + +Strict standard: +- reset each store from its initial state or `getInitialState()` pattern + +13. **Coverage is uneven for side-effectful stores** + +Well-covered: +- [agentStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/stores/__tests__/agentStore.test.ts:1) +- [chatStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/__tests__/chatStore.test.ts:1) +- [chatSessionStore.test.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/__tests__/chatSessionStore.test.ts:1) + +Noticeably under-covered: +- [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:1) +- [providerInventoryStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/providers/stores/providerInventoryStore.ts:1) + +Missing high-value coverage areas: +- project cache hydration +- reorder semantics +- optimistic failure policy +- inventory merge semantics + +14. **There are no clear house rules for what belongs in Zustand** + +This is the meta architectural issue. + +Code evidence comes from inconsistency across modules: +- broad whole-store consumer: [AppShell.tsx](/Users/lifei/Development/goose/ui/goose2/src/app/AppShell.tsx:80) +- narrow selector consumer: [useProviderSelection.ts](/Users/lifei/Development/goose/ui/goose2/src/features/agents/hooks/useProviderSelection.ts:1) +- store-embedded persistence: [projectStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/projects/stores/projectStore.ts:11) +- store-embedded backend side effects: [chatSessionStore.ts](/Users/lifei/Development/goose/ui/goose2/src/features/chat/stores/chatSessionStore.ts:195) +- local UI state still handled in component: [AppShell.tsx](/Users/lifei/Development/goose/ui/goose2/src/app/AppShell.tsx:58) + +What this means: +- the codebase does not yet have a shared rule set for: + - local vs shared state + - ephemeral vs durable state + - store state vs orchestration logic + - selector API expectations + - persistence boundaries + +**Clarifications So We Don’t Overstate** + +- Multiple separate stores are not automatically wrong. +- `Map` and `Set` are not wrong here; they are used correctly. +- `getState()` is not wrong in non-React async/event code. +- Colocating actions with state is a recommended Zustand pattern. The issue is not action location by itself, but unclear responsibilities and hidden side effects. +- The architecture recommendations below are a synthesis of the current findings. They are not all direct “code defects,” but they are the most defensible target design responses to the issues in the current code. + +**Architectural Target Direction** + +The clean target architecture is: +- React local state for local view concerns +- Zustand for shared client state needed by multiple consumers +- `api/` modules for backend transport +- hooks or command-style modules for orchestration and reconciliation +- pure `lib/` utilities for derivation and transformation + +Zustand should be a small, predictable shared-state layer, not a catch-all workflow layer. + +A healthier direction would be to separate concerns more explicitly, for example: +- session data state vs session UI state +- agent/provider catalog state vs editor/modal UI state +- chat message state vs chat composer/runtime state +- workflow commands/hooks for backend mutations and reconciliation + +This exact split does not need to be implemented all at once, but the codebase should move in that direction. + +**Recommended Standard For `ui/goose2`** + +- Never call a bound Zustand hook without a selector in React components or hooks. +- Use `useShallow` only when a selector returns an object or array. +- Keep store actions either: + - pure local state transitions, or + - explicitly named async commands with clear side-effect semantics. +- Do not mix domain data state and modal/open-state UI unless truly necessary. +- Use `persist` for durable state, with `partialize` and `version`. +- Prefer selector hooks/helpers for React reads. +- Use `getState()` mainly in non-React async/event code. +- Reset stores in tests from initial state, not partial merge patches. +- When a store grows large, split it by responsibility or by slices. +- For every backend mutation, make the optimistic/pessimistic/reconciliation policy explicit. +- Establish a team rule for what belongs in Zustand vs local state vs hooks vs `api/`. + +**Bottom Line** + +Strictly judged against Zustand good practices and from an architectural perspective, `ui/goose2` is **adequate for shipping features quickly, but not disciplined enough for long-term maintainability**. + +The highest-value issues to fix are: +1. whole-store subscriptions +2. broad mixed-responsibility stores +3. domain state and UI state mixed in the same stores +4. hidden backend side effects in generic store actions +5. Zustand being used as a workflow layer instead of primarily a state layer +6. ad hoc persistence +7. weak selector and test-reset discipline + +These are the areas most likely to create future bugs and make the code harder to refactor, reason about, and test. diff --git a/ui/goose2/ui_improvements/state_management/phase-1-selector-cleanup.md b/ui/goose2/ui_improvements/state_management/phase-1-selector-cleanup.md new file mode 100644 index 0000000000..646b5efa90 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-1-selector-cleanup.md @@ -0,0 +1,61 @@ +**Phase 1: Remove Whole-Store Subscriptions** + +**Status** +- Not started. + +**Goal** +- Replace broad bound-store subscriptions with explicit selectors. +- Keep behavior unchanged. +- Reduce coupling before any store boundary changes. + +**Scope** +- `ui/goose2/src/app/AppShell.tsx` +- `ui/goose2/src/features/sidebar/ui/Sidebar.tsx` +- `ui/goose2/src/features/agents/hooks/usePersonas.ts` +- `ui/goose2/src/features/chat/hooks/useChat.ts` + +**Out Of Scope** +- Do not split stores. +- Do not move backend API calls. +- Do not introduce Zustand `persist`. +- Do not introduce Immer. +- Do not create a broad selector framework unless repeated selectors become obvious. + +**Execution Steps** + +1. Establish the current baseline. + - Run `rg "use[A-Za-z0-9]+Store\\(\\)" ui/goose2/src`. + - Confirm broad subscriptions are limited to the known Phase 1 targets or document any additional targets. + +2. Refactor `AppShell.tsx`. + - Replace `useChatStore()` with selectors for only the used fields/actions. + - Replace `useChatSessionStore()` with selectors for only the used fields/actions. + - Replace `useAgentStore()` with selectors for only the used fields/actions. + - Replace `useProjectStore()` with selectors for only the used fields/actions. + - Keep `useStore.getState()` inside async callbacks where the code intentionally needs latest state at call time. + +3. Refactor `Sidebar.tsx`. + - Replace `useChatStore()` with selectors for `messagesBySession` and any action/helper actually needed during render. + - Replace `useChatSessionStore()` broad destructuring with selectors. + - Replace `useAgentStore()` and `useProjectStore()` broad reads with targeted selectors or narrow `getState()` usage in callbacks. + - Keep render output and sorting behavior unchanged. + +4. Refactor `usePersonas.ts`. + - Replace `const store = useAgentStore()` with selected actions/state. + - Use selected actions in callbacks. + - Avoid adding dependencies that recreate timers unnecessarily. + - If a callback needs latest store state rather than reactive state, use `useAgentStore.getState()` explicitly. + +5. Refactor `useChat.ts`. + - Replace `const store = useChatStore()` with selectors for session-specific messages/runtime and selected actions. + - Preserve existing use of `getState()` where callbacks intentionally read current runtime state. + - Avoid changing prompt, queue, compaction, or message mutation semantics. + +**Validation** +- `rg "use[A-Za-z0-9]+Store\\(\\)" ui/goose2/src` +- `cd ui/goose2 && pnpm test -- useChat usePersonas Sidebar` + +**Success Criteria** +- No high-level React component or hook in scope calls a bound Zustand hook without a selector. +- Behavior remains unchanged. +- No store boundary or persistence changes are included. diff --git a/ui/goose2/ui_improvements/state_management/phase-2-selector-read-layer.md b/ui/goose2/ui_improvements/state_management/phase-2-selector-read-layer.md new file mode 100644 index 0000000000..9c381788db --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-2-selector-read-layer.md @@ -0,0 +1,95 @@ +**Phase 2: Introduce A Selector-First Read Layer** + +**Status** +- Complete. + +**Goal** +- Standardize repeated Zustand reads after Phase 1 exposes selector duplication. +- Add `useShallow` only where object or array selector results benefit from shallow comparison. +- Use selectors to reveal state boundaries, not hide weak store or component boundaries. +- Treat selector files as the current read-layer map, not as a final architecture. Later phases must revisit selector files when state moves across store boundaries. + +**Scope** +- `ui/goose2/src/features/chat/stores/chatSelectors.ts` +- `ui/goose2/src/features/chat/stores/chatSessionSelectors.ts` +- `ui/goose2/src/features/agents/stores/agentSelectors.ts` +- `ui/goose2/src/features/projects/stores/projectSelectors.ts` +- `ui/goose2/src/features/providers/hooks/useProviderInventory.ts` +- Any Phase 1 call sites that now duplicate selector logic. + +**Out Of Scope** +- Do not split stores. +- Do not change action semantics. +- Do not move backend calls. +- Do not add `useShallow` around primitive selectors. +- Do not introduce selectors for one-off reads unless they improve clarity. +- Do not create selector files mechanically for every store field. +- Do not add stored attributes for values that can be safely derived. + +**Execution Steps** + +1. Review and classify selectors from Phase 1. + - Look for repeated active session, active project, active agent, message, runtime, and provider-selection reads. + - Classify each candidate as one of: + - common simple read + - derived value + - symptom of a store with too many responsibilities + - symptom of a component doing too much orchestration + - one-off inline selector + - Keep one-off selectors inline unless abstraction clearly improves readability. + - Record store-boundary and component-boundary issues as follow-ups instead of hiding them behind selectors. + +2. Add selector helpers only for common simple reads. + - Start with pure selector functions. + - Prefer names that describe the value, not the component that consumes it. + - Keep selector modules close to the store they read. + - When a later phase moves state out of a store, move or delete the related selectors in the same phase so selector files do not preserve old boundaries by accident. + - Selector fallbacks must be stable. Do not return fresh `[]` or `{}` values directly from selectors; use module-level constants or derive outside the selector. + - Good candidates: + - chat: `selectMessagesBySession`, `selectSessionStateById` + - chat sessions: `selectSessions`, `selectActiveSessionId`, `selectHasHydratedSessions`, `selectSessionsLoading` + - agents: `selectPersonas`, `selectPersonasLoading`, `selectSelectedProvider` + - projects: `selectProjects`, `selectProjectsLoading` + +3. Prefer pure derived helpers for derived values. + - Active session should be derived from `sessions + activeSessionId`; do not store a second `activeSession` attribute unless there is a strong reason. + - Project lookup by id can be a pure helper over `projects`. + - Sidebar session grouping may belong in a pure helper or dedicated selector-like utility, but should be designed carefully because it combines sessions, messages, runtime state, and project ids. + - Sidebar: evaluate whether derived sidebar session items should be produced by selector/helper code instead of selecting the whole `sessionStateById` record during render. + - `AppShell` active/home session derivation should remain derived from selected session state for now; do not add duplicate store attributes. + +4. Revisit hooks that mix read state and command functions. + - `usePersonas()` currently returns `personas` and `isLoading` as well as command functions. + - `AgentsView` already reads `personas` and `personasLoading` directly with store selectors and only uses `usePersonas()` for command functions. + - Decide whether to keep the current contract or split toward a clearer read-selector plus command-hook pattern. + - Do not make this contract change in Phase 1. + +5. Introduce `useShallow` selectively. + - Use it for grouped object selectors in high-level consumers. + - Use it for derived array/object selectors where reference churn causes unnecessary rerenders. + - Do not use it for primitive values or single function selectors. + - Do not use `useShallow` to compensate for a broad store or overly broad component dependency. + - Re-check `useProviderInventory` during this phase. It currently reads `entries` and `loading` separately; only group them with `useShallow` if that improves clarity without introducing object churn. + - Phase 2 assessment: keep `useProviderInventory` as separate `entries` and `loading` selectors for now. Grouping them with `useShallow` would mostly reduce two store subscriptions to one, but would not narrow rerenders or simplify consumers. + +6. Refactor consumers to use selector helpers where they remove duplication. + - Keep call sites readable. + - Avoid turning every selector into an abstraction. + - If a consumer needs many selectors, decide whether that indicates a later component decomposition or store-boundary task. + - Start with small chat-session selector helpers in `AppShell.tsx` and `Sidebar.tsx`; do not refactor every selector candidate at once. + - Record `AppShell` as a later component-decomposition candidate rather than trying to hide its orchestration breadth behind selector helpers. + - Record `Sidebar` derived session grouping as a possible pure helper extraction, not as store state. + - For `agentStore`, add selectors only for stable repeated domain reads such as personas. Do not add selector helpers for persona editor UI state in this phase, because that state is a Phase 4 extraction candidate. + - If selector extraction makes a feature look syntactically cleaner but still leaves many store dependencies, record that as a component or store boundary problem instead of adding more selectors. + - Before closing Phase 2, list selector files created and the later phases that must revisit them. + +**Validation** +- `rg "useShallow|zustand/shallow" ui/goose2/src` +- `rg "use[A-Za-z0-9]+Store\\(\\)" ui/goose2/src` +- `cd ui/goose2 && pnpm test -- useChat usePersonas Sidebar useProviderInventory` + +**Success Criteria** +- Common read patterns have reusable selector helpers. +- `useShallow` appears only on object or array selectors where it adds value. +- React store consumption is more consistent without changing behavior. +- Phase 3-5 follow-ups are clearer because selector audit findings distinguish simple read cleanup from deeper architecture problems. diff --git a/ui/goose2/ui_improvements/state_management/phase-3-session-side-effects.md b/ui/goose2/ui_improvements/state_management/phase-3-session-side-effects.md new file mode 100644 index 0000000000..4401150f28 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-3-session-side-effects.md @@ -0,0 +1,96 @@ +**Phase 3: Separate Chat Session Side Effects From Generic Store Actions** + +**Status** +- Complete. + +**Goal** +- Make session mutation semantics explicit. +- Ensure generic local patch actions do not hide backend writes. +- Use backend-first operations for persisted session fields so local state is patched only after backend success. +- Keep this phase focused on side-effect semantics even though Phase 2 identified broader `chatSessionStore` responsibility overload. +- Create clearly named operation functions so backend failure policy is centralized and visible. + +**Scope** +- `ui/goose2/src/features/chat/stores/chatSessionStore.ts` +- `ui/goose2/src/features/chat/hooks/useChat.ts` +- `ui/goose2/src/features/chat/hooks/useResolvedAgentModelPicker.ts` +- `ui/goose2/src/features/chat/hooks/useChatSessionController.ts` +- New `ui/goose2/src/features/chat/stores/chatSessionOperations.ts` + +**Out Of Scope** +- Do not split `chatSessionStore` UI state yet. +- Do not refactor `projectStore`. +- Do not change ACP API behavior. +- Do not change title generation, archive, unarchive, or project assignment UX. +- Do not solve `chatSessionStore` context-panel or workspace UI ownership here; that belongs in Phase 4. +- Do not introduce rollback, refresh-on-failure, or broad user-visible error policy in this phase. +- Do not use `commands` naming for the new module, because Tauri backend commands already use that term. + +**Execution Steps** + +1. Identify all `updateSession` call sites. + - Classify each call as local-only, title rename, project update, timestamp update, provider/model update, archive-related, or other. + - Keep a note of calls that also reveal broad `chatSessionStore` ownership, but do not expand this phase beyond mutation semantics. + - Current classification: + - manual title update: `AppShell.tsx` should move to `updateSessionTitle`. + - project assignment: `AppShell.tsx` and `useChatSessionController.ts` should move to `updateSessionProject`. + - backend/session-info title metadata: `acpSessionInfoUpdate.ts` should be local-only `patchSession`. + - generated local title, timestamps, provider/model/persona metadata, ACP notification model updates, and test setup should be local-only `patchSession`. + - mixed pending patch in `useChatSessionController.ts` should split `projectId` into `updateSessionProject` and keep provider/persona/model fields as `patchSession`. + +2. Add a local-only action. + - Introduce `patchSession` in `chatSessionStore`. + - It should only update Zustand state. + - It should not inspect patch fields for backend meaning. + - Keep the existing shallow-merge local update behavior exactly the same for now. + +3. Keep compatibility while migrating. + - Temporarily keep `updateSession`. + - Have `updateSession` delegate to `patchSession` only after persisted call sites have moved to operations. + - Remove `updateSession` when no production or test call sites remain. + +4. Add explicit operation functions. + - Add `chatSessionOperations.ts`. + - Add `updateSessionTitle(sessionId, title)`. + - Call backend rename first. + - After success, call `patchSession(sessionId, { title, userSetName: true })`. + - Let errors throw so UI call sites can decide how to surface failure. + - Add `updateSessionProject(sessionId, projectId)`. + - Call backend project update first. + - After success, call `patchSession(sessionId, { projectId })`. + - Let errors throw so UI call sites can decide how to surface failure. + - Import backend API functions with aliases if needed to avoid name conflicts, for example `renameSession as updateSessionTitleApi`. + - Leave archive/unarchive in the store initially unless migration remains simple after title/project cleanup. + +5. Migrate call sites one category at a time. + - Timestamp-only and UI/local patches should use `patchSession`. + - Manual title rename should call `updateSessionTitle`. + - Project assignment should call `updateSessionProject`. + - Backend/session-info updates should use `patchSession` only, even when the patch includes `title` or `userSetName`. + - `userSetName` alone should never imply a backend call. + - Archive flows can stay on existing explicit `archiveSession` / `unarchiveSession` until title/project ambiguity is gone. + +6. Remove hidden side effects from `updateSession`. + - Once call sites are migrated, remove `updateSession`. + - Avoid leaving two names with different implied semantics. + +7. Add focused tests. + - `patchSession` does not call backend APIs. + - `updateSessionTitle` calls backend rename before patching local state. + - `updateSessionTitle` does not patch local state if backend rename rejects. + - `updateSessionProject` calls backend project update before patching local state. + - `updateSessionProject` does not patch local state if backend project update rejects. + - Backend/session-info updates with `title` and/or `userSetName` are local-only. + +8. Record failure-policy follow-up. + - After this phase, evaluate whether archive and unarchive should also move to operation functions and whether persisted operations need user-visible error handling. + - Track that decision in `phase-5-session-workflow-failure-policy.md`. + +**Validation** +- `rg "updateSession\\(" ui/goose2/src` +- `cd ui/goose2 && pnpm test -- chatSessionStore useChat useChatSessionController` + +**Success Criteria** +- Generic local session patching is local-only. +- Backend writes happen through explicitly named operations. +- Existing chat rename, project update, archive, and unarchive flows still work. diff --git a/ui/goose2/ui_improvements/state_management/phase-4-store-boundaries.md b/ui/goose2/ui_improvements/state_management/phase-4-store-boundaries.md new file mode 100644 index 0000000000..9f2e1a0fa9 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-4-store-boundaries.md @@ -0,0 +1,101 @@ +**Phase 4: Split The Clearest Store Boundaries** + +**Status** +- Not started. + +**Goal** +- Move clearly UI-only state out of domain-heavy stores. +- Keep store splits narrow and behavior-preserving. +- Use the Phase 2 selector audit to distinguish UI-state extraction from deeper domain-model cleanup. +- Revisit component responsibility after UI state moves, especially where Phase 2 identified components that still depend on many store concerns. + +**Scope** +- `ui/goose2/src/features/agents/stores/agentStore.ts` +- `ui/goose2/src/features/agents/stores/agentSelectors.ts` if Phase 2 adds it +- New `ui/goose2/src/features/agents/stores/agentUiStore.ts` +- `ui/goose2/src/features/chat/stores/chatSessionStore.ts` +- `ui/goose2/src/features/chat/stores/chatSessionSelectors.ts` +- `ui/goose2/src/features/chat/stores/chatSelectors.ts` if `chatStore` boundaries are revisited +- New `ui/goose2/src/features/chat/stores/chatSessionUiStore.ts` +- Consumers of persona editor state, context panel state, and active workspace state. + +**Out Of Scope** +- Do not split personas, agents, providers, or selected provider yet. +- Do not move `activeSessionId` unless this phase's boundary review proves it is safe. +- Do not split `chatStore` immediately. +- Do not refactor project orchestration in this phase. +- Do not solve the full persona vs agent vs provider domain model in the first store split; record the model problem and keep the initial extraction narrow. + +**Execution Steps** + +1. Split `agentStore` UI state first. + - Phase 2 identified `agentStore` as mixing personas, agents, providers, selected provider, active agent, and persona editor UI state. + - Treat persona editor state as the safest first extraction, not as the full solution to the domain model. + - Move these fields to `agentUiStore`: + - `personaEditorOpen` + - `editingPersona` + - `personaEditorMode` + - Move these actions to `agentUiStore`: + - `openPersonaEditor` + - `closePersonaEditor` + - Keep catalog/provider state in `agentStore`. + +2. Update agent UI consumers. + - Update `AgentsView.tsx` and related components to read editor state from `agentUiStore`. + - Update any controller code that opens or closes the persona editor. + - Keep persona CRUD and provider selection on `agentStore`. + - Update any Phase 2 selector files so `agentSelectors.ts` contains only state that remains in `agentStore`. If UI selectors were avoided in Phase 2, this should be a small verification step. + +3. Validate the `agentStore` split. + - Tests for domain state should not need editor fields. + - Add minimal `agentUiStore` tests if behavior is not trivially covered by UI tests. + - Recheck `agentStore.isLoading`. Phase 2 found it may be a legacy/general loading flag because personas, agents, and providers already have specific loading fields. Remove or defer it only after confirming consumers and tests do not depend on it. + - Recheck derived helper methods such as `getActiveAgent`, `getAgentsByPersona`, `getBuiltinPersonas`, and `getCustomPersonas`. Keep them for imperative `getState()` usage if useful, but React consumers should prefer selectors or pure helpers. + +4. Split `chatSessionStore` UI state second. + - Phase 2 identified `chatSessionStore` as mixing session records, hydration/loading, active selection, creation/archive/mutation actions, context-panel state, and workspace UI state. + - This phase should extract only the clearly UI-only context panel and workspace state. + - Move these fields to `chatSessionUiStore`: + - `contextPanelOpenBySession` + - `activeWorkspaceBySession` + - Move these actions to `chatSessionUiStore`: + - `setContextPanelOpen` + - `setActiveWorkspace` + - `clearActiveWorkspace` + - Keep session records and active session selection in `chatSessionStore`. + +5. Update chat UI consumers. + - Update `ChatView.tsx`, `ContextPanel.tsx`, `useChatSessionController.ts`, and `useChat.ts` if they read the moved fields/actions. + - Keep session creation, loading, archive, and rename behavior unchanged. + - Update `chatSessionSelectors.ts` so selectors for moved UI state live with `chatSessionUiStore` or are removed. Keep session record selectors with `chatSessionStore`. + - Recheck session-derived values after the split. Active session and Home session should still be derived from `sessions + activeSessionId` or `sessions + homeSessionId`; do not add duplicate stored session objects. + - Recheck sidebar session grouping after UI state moves. If grouping remains complex, prefer a pure helper or selector-like utility over adding grouped sidebar rows to store state. + +6. Re-evaluate `chatStore` only after the two safe splits. + - Phase 2 identified `chatStore` as mixing messages, runtime, drafts, queue, loading, scroll targets, and cleanup. + - Phase 2 also found duplicated active-session selection: both `chatStore` and `chatSessionStore` store `activeSessionId`. Decide which store should own active session selection, or explicitly document why both must remain synchronized. + - Do not split message/runtime/draft state in this phase unless the selector data from Phases 1 and 2 shows a very clear boundary. + - If any `chatStore` state moves later, update `chatSelectors.ts` in the same change so selector files continue to reflect real store boundaries. + - Record any follow-up observations in the progress tracker. + +7. Record remaining domain-model questions. + - Clarify whether personas, agents, providers, selected provider, and active agent need separate stores or clearer naming. + - Defer any larger domain-model split to a later focused plan after the UI-only state extraction is complete. + - Recheck store helper methods that return derived values, including `chatStore.getActiveMessages`, `chatSessionStore.getActiveSession`, and `chatSessionStore.getArchivedSessions`. Prefer pure helpers/selectors for React reads, but keep imperative helpers if they are still useful in callbacks or non-React code. + +8. Recheck component responsibility after the UI-store splits. + - `AgentsView` should be simpler after persona editor UI state moves out of `agentStore`; if it still mixes list filtering, editor orchestration, import/export, delete confirmation, and mutation flows too heavily, record a focused component-decomposition follow-up. + - `useChatSessionController`, `ChatView`, and context-panel consumers should be rechecked after context panel and workspace UI state move out of `chatSessionStore`. + - Recheck chat/session actions that are owned too high in `AppShell` and prop-drilled through multiple surfaces, especially rename from `SidebarChatRow` and `SessionCard`. + - Recheck whether rename/archive/move actions need a surface-level action hook or controller after state boundaries are clearer. + - Do not decompose components in this phase unless the split requires it for safe wiring; record the follow-up with concrete files and responsibilities. + +**Validation** +- `rg "personaEditorOpen|editingPersona|personaEditorMode|openPersonaEditor|closePersonaEditor" ui/goose2/src` +- `rg "contextPanelOpenBySession|activeWorkspaceBySession|setContextPanelOpen|setActiveWorkspace|clearActiveWorkspace" ui/goose2/src` +- `cd ui/goose2 && pnpm test -- agentStore chatSessionStore useChatSessionController` + +**Success Criteria** +- Persona editor UI state is no longer in `agentStore`. +- Context panel and active workspace UI state are no longer in `chatSessionStore`. +- Domain store behavior remains unchanged. diff --git a/ui/goose2/ui_improvements/state_management/phase-5-session-workflow-failure-policy.md b/ui/goose2/ui_improvements/state_management/phase-5-session-workflow-failure-policy.md new file mode 100644 index 0000000000..9945e9a897 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-5-session-workflow-failure-policy.md @@ -0,0 +1,85 @@ +**Phase 5: Decide Chat Session Workflow Failure Policy** + +**Status** +- Not started. + +**Goal** +- Decide whether chat-session workflow actions should keep optimistic-without-rollback behavior or move to a stronger consistency policy. +- Keep this as a separate behavior-change decision after Phase 3 makes title/project workflow actions explicit. +- Revisit archive/unarchive after Phase 4 clarifies store boundaries, because those workflows still live in `chatSessionStore` today. + +**Why This Is Separate** +- Phase 3 removes hidden side effects without changing behavior. +- Failure-policy changes are user-visible and need intentional product/UX decisions. +- Once workflows such as `renameSessionAndPersist` and `updateSessionProjectAndPersist` exist, each policy can be changed in one place. + +**Scope** +- Chat-session workflow actions created in Phase 3: + - rename session + - update session project +- Chat-session workflow actions to revisit after Phase 4: + - archive session + - unarchive session +- User-visible error handling for those workflows, if policy changes require it. +- Pending/error UI for direct user actions such as rename, if needed. +- Focused tests for success and failure behavior. + +**Out Of Scope** +- Do not refactor `projectStore`; that remains Phase 6. +- Do not standardize all persistence; that remains Phase 7. +- Do not change unrelated chat send, replay, or model-selection behavior. + +**Policy Options To Evaluate** + +1. Keep optimistic without rollback. + - Local state updates immediately. + - Backend failure is logged. + - Lowest UI friction, weakest consistency. + +2. Backend first. + - Backend succeeds before local state changes. + - Strong consistency, slower perceived UI. + +3. Optimistic with rollback. + - Local state updates immediately. + - Backend failure restores previous local state. + - Better consistency, more rollback complexity. + +4. Optimistic with refresh on failure. + - Local state updates immediately. + - Backend failure reloads sessions from backend. + - Simpler than targeted rollback, but may be heavier. + +5. Optimistic with user-visible error. + - Local state updates immediately. + - Backend failure leaves local state but tells the user. + - Makes failure visible without solving consistency. + +**Execution Steps** + +1. Review Phase 3 workflow actions and call sites. + - Confirm each workflow has one clear implementation point. + - Confirm current optimistic-without-rollback behavior is covered by tests. + +2. Choose policy per workflow. + - Rename may be able to use backend-first or rollback. + - Project assignment may need extra care because it affects sidebar grouping. + - Archive/unarchive should be evaluated after Phase 4. Current behavior is optimistic without rollback: local visibility changes immediately, backend failures are logged, and local state is not restored. Moving these workflows out of `chatSessionStore` should happen after store boundaries are clearer. + +3. Implement one workflow policy at a time. + - Keep each behavior change small. + - Add focused failure tests before moving to the next workflow. + +4. Add user-visible error handling only if the chosen policy needs it. + - Use existing toast/error patterns. + - Avoid noisy errors for background refresh-style recovery. + - Recheck rename UX. Today `SidebarChatRow` and `SessionCard` close inline edit immediately and invoke a void callback; backend-first rename failure can only be surfaced as a toast unless those components gain pending/error state. + +**Validation** +- `cd ui/goose2 && pnpm test -- chatSessionStore useChatSessionController Sidebar` +- Manual smoke: rename chat, move chat to project, archive/unarchive chat, force or mock backend failure where feasible. + +**Success Criteria** +- Each chat-session workflow has an explicit failure policy. +- Failure behavior is tested. +- Local/backend consistency behavior is intentional rather than inherited from the old generic `updateSession`. diff --git a/ui/goose2/ui_improvements/state_management/phase-6-project-store.md b/ui/goose2/ui_improvements/state_management/phase-6-project-store.md new file mode 100644 index 0000000000..951bd0aeb4 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-6-project-store.md @@ -0,0 +1,82 @@ +**Phase 6: Refactor Project Store Into Clearer Layers** + +**Status** +- Not started. + +**Goal** +- Reduce `projectStore` responsibility overload. +- Make API orchestration, local state transitions, cache behavior, and mutation policy easier to reason about. +- Address Phase 2 findings that project reads are common but the bigger issue is project workflow ownership, not selector syntax. +- Revisit project-heavy component responsibility after project-store ownership is clearer. + +**Scope** +- `ui/goose2/src/features/projects/stores/projectStore.ts` +- New project command or hook module, such as `useProjectCommands.ts` or `projectCommands.ts` +- Project consumers such as `ProjectsView.tsx`, `Sidebar.tsx`, and `SettingsModal.tsx` + +**Out Of Scope** +- Do not standardize all persistence yet. +- Do not change project API contracts. +- Do not redesign project UI. +- Do not change reorder UX unless the current behavior is clearly buggy. + +**Execution Steps** + +1. Document the current mutation policy. + - Include the fact that `AppShell`, `Sidebar`, and project views depend on project list lookup by id, while `projectStore` also owns API orchestration and cache persistence. + - `fetchProjects`: pessimistic load with cached bootstrap. + - `addProject`, `editProject`, `removeProject`: backend first, local update after success. + - `reorderProjects`: local optimistic reorder, persist cache, fire backend reorder without rollback. + +2. Add local state actions. + - Introduce local-only store actions with explicit names, for example: + - `setProjectsLocal` + - `upsertProjectLocal` + - `removeProjectLocal` + - `reorderProjectsLocal` + - `setProjectsLoading` + - `setActiveProject` + - Keep old public actions temporarily if migration needs to be incremental. + +3. Add project commands. + - Move backend workflows into a command-style module or hook. + - Commands can call project API modules and then local store actions. + - Keep UI call sites simple. + +4. Migrate consumers gradually. + - Move `fetchProjects`, `addProject`, `editProject`, and `removeProject` consumers to commands. + - Migrate reorder last because it has the least explicit failure behavior today. + +5. Recheck project-related component responsibility. + - `AppShell` currently orchestrates project fetch, project creation/edit/archive callbacks, project selection for new chats, sidebar project actions, and chat/session setup. After project-store policy is clarified, decide whether some project orchestration should move into a project controller hook or smaller app-shell child component. + - `Sidebar` currently combines project display, session grouping by project, expanded-project persistence, runtime badges, and search resolver support. After project selectors/helpers are settled, decide whether project/session grouping belongs in a pure helper or a smaller sidebar controller. + - Recheck project lookup derived values. Project lookup by id should remain derived from `projects + projectId`; if repeated lookup logic is noisy, use a pure helper over the project list instead of storing duplicate active/current project objects. + - Recheck `projectStore.getActiveProject`. It is an imperative derived helper over `projects + activeProjectId`; keep it only if useful for `getState()`/callback code, and prefer selectors or pure helpers for React reads. + - Record concrete follow-ups instead of doing a broad component refactor inside this phase unless the project-store change requires it. + +6. Make reorder policy explicit. + - Preserve current optimistic behavior unless product expectations require a change. + - Add a clear comment or function name indicating whether reorder is optimistic without rollback, optimistic with refresh, or pessimistic. + - Prefer adding failure handling in a separate small change if needed. + +7. Reduce persistence coupling, but do not standardize persistence yet. + - Keep cache behavior working. + - Isolate cache reads/writes behind small helper functions if they remain in this phase. + +8. Add focused tests. + - Project cache bootstrap. + - Add/edit/remove success behavior. + - Reorder local ordering and order fields. + - Reorder backend call shape. + - Failure behavior documented by tests where feasible. + +**Validation** +- `cd ui/goose2 && pnpm test -- project` +- `rg "useProjectStore\\(\\)" ui/goose2/src` +- Manual smoke: create, edit, reorder, and delete a project. + +**Success Criteria** +- `projectStore` no longer owns every project workflow directly. +- Mutation policy is explicit. +- Cache behavior still works. +- Project UI behavior is unchanged. diff --git a/ui/goose2/ui_improvements/state_management/phase-7-persistence.md b/ui/goose2/ui_improvements/state_management/phase-7-persistence.md new file mode 100644 index 0000000000..a6abba0e0f --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-7-persistence.md @@ -0,0 +1,61 @@ +**Phase 7: Standardize Persistence Boundaries** + +**Status** +- Not started. + +**Goal** +- Make durable state intentional and consistent. +- Reduce ad hoc localStorage handling where Zustand `persist` or isolated persistence helpers provide a cleaner boundary. + +**Scope** +- `ui/goose2/src/features/projects/stores/projectStore.ts` +- `ui/goose2/src/features/agents/stores/agentStore.ts` +- `ui/goose2/src/features/chat/stores/draftPersistence.ts` +- Any persistence introduced or clarified in earlier phases. + +**Out Of Scope** +- Do not persist transient runtime state. +- Do not persist UI-only modal/open state by default. +- Do not convert isolated, well-tested persistence helpers just for consistency. +- Do not change product decisions about what survives reload without recording the decision. + +**Execution Steps** + +1. Inventory durable state. + - Selected provider. + - Chat drafts. + - Project cache. + - Home session id if relevant to this area. + - Model preferences if relevant to this area. + +2. Classify each durable value. + - Product preference. + - Cache for faster bootstrap. + - Draft/user input recovery. + - Runtime state that should not be persisted. + - UI state that should not be persisted. + +3. Decide persistence mechanism per value. + - Use Zustand `persist` only when the persisted data is clearly store-owned. + - Keep separate helper modules for persistence that is intentionally outside a store or already isolated. + - Use `partialize` and versioning when adopting `persist`. + +4. Migrate one persistence area at a time. + - Selected provider is the smallest candidate. + - Project cache should wait until project store ownership is clear. + - Draft persistence can remain as-is if the helper is clearer than `persist`. + +5. Add tests around migration behavior. + - Hydration/default behavior. + - Invalid stored data fallback. + - Version/migration behavior if `persist` is used. + - Non-persistence of transient state. + +**Validation** +- `rg "localStorage|getItem|setItem|persist\\(" ui/goose2/src/features` +- `cd ui/goose2 && pnpm test -- chatStore agentStore project` + +**Success Criteria** +- Durable state choices are documented and minimal. +- Persistence code has clear ownership. +- No runtime or temporary UI state is accidentally persisted. diff --git a/ui/goose2/ui_improvements/state_management/phase-8-test-reset-coverage.md b/ui/goose2/ui_improvements/state_management/phase-8-test-reset-coverage.md new file mode 100644 index 0000000000..f60debdb6c --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-8-test-reset-coverage.md @@ -0,0 +1,66 @@ +**Phase 8: Standardize Test Reset Patterns And Close Coverage Gaps** + +**Status** +- Not started. + +**Goal** +- Make store and hook tests safer after the state boundaries have stabilized. +- Replace partial shallow-merge resets with known full-state resets. +- Add coverage for risky persistence and mutation policy paths. + +**Scope** +- Store tests for chat, chat sessions, agents, projects, providers, and any UI stores introduced in Phase 4. +- Hook tests that directly manipulate Zustand stores. +- Coverage gaps discovered in Phases 3 through 6. + +**Out Of Scope** +- Do not rewrite all tests for style. +- Do not add broad snapshot-style coverage. +- Do not introduce a large test utility framework unless duplication is clearly painful. + +**Execution Steps** + +1. Add full initial-state reset helpers. + - Prefer one helper per store. + - Keep helpers easy to update when store shape changes. + - Include actions only if the store's reset approach requires them. + +2. Replace partial resets in existing tests. + - `agentStore.test.ts` + - `usePersonas.test.ts` + - `chatStore.test.ts` + - Chat hook tests that call `setState` directly. + - New store tests from earlier phases. + +3. Add project store coverage. + - Cache hydration. + - Add/edit/remove local state results. + - Reorder semantics. + - Reorder failure policy if implemented or documented. + +4. Add provider inventory store coverage if still missing. + - `setEntries` replaces inventory. + - `mergeEntries` preserves existing providers and updates overlapping providers. + - Loading behavior. + +5. Add session mutation orchestration coverage. + - Local patch does not call backend APIs. + - Rename operation calls backend rename. + - Project update operation calls backend project update. + - Archive/unarchive operation behavior. + +6. Remove stale assumptions from tests. + - Tests should not rely on fields that moved to UI stores. + - Tests should reset every store touched by the scenario. + - Component-test mocks for Zustand bound hooks should support selector-style calls. Phase 1 moved production code from `useStore()` to `useStore((state) => ...)`, so mocks should accept an optional selector and invoke it with the mocked state shape. + - Tests that import Phase 2 selector files should still match the final store boundaries after Phases 4 and 5. Remove or move selector imports if the selected state moved to a new store. + - Recheck test reset assumptions around legacy/general fields such as `agentStore.isLoading`. If Phase 4 removes or narrows those fields, reset helpers should not preserve stale state shape. + +**Validation** +- `cd ui/goose2 && pnpm test` +- `rg "setState\\(\\{" ui/goose2/src/features ui/goose2/src/shared` + +**Success Criteria** +- Store tests reset from known initial state. +- Risky mutation and persistence paths have direct coverage. +- Test setup matches the final store boundaries. diff --git a/ui/goose2/ui_improvements/state_management/phase-9-optional-immer.md b/ui/goose2/ui_improvements/state_management/phase-9-optional-immer.md new file mode 100644 index 0000000000..6593558e5c --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/phase-9-optional-immer.md @@ -0,0 +1,51 @@ +**Phase 9: Optional Immer Adoption** + +**Status** +- Not started. + +**Goal** +- Improve nested update readability only where structural cleanup did not solve the pain. +- Avoid using Immer as a substitute for better store boundaries. + +**Scope** +- `ui/goose2/src/features/chat/stores/chatStore.ts` +- Possibly `ui/goose2/src/features/chat/stores/chatSessionStore.ts` + +**Out Of Scope** +- Do not add Immer before earlier phases are complete. +- Do not use Immer to justify keeping a broad store. +- Do not convert flat stores where object spread is already clear. +- Do not make dependency changes manually in `Cargo.toml`; for frontend packages follow the repo's package workflow. + +**Execution Steps** + +1. Reassess update complexity after Phases 1 through 7. + - Check whether `chatStore` nested updates are still hard to read. + - Check whether store splits or selectors already reduced the pain enough. + +2. Decide whether Immer is worth the dependency and pattern change. + - Adopt only if it materially improves readability. + - Skip this phase if the remaining updates are acceptable. + +3. If adopting, start with one store. + - Prefer `chatStore` first. + - Convert a small group of related nested updates. + - Keep behavior exactly the same. + +4. Add or update focused tests. + - Messages update correctly. + - Runtime state updates correctly. + - Drafts and cleanup still work. + - No accidental mutation leaks between sessions. + +5. Reassess before expanding. + - Only convert `chatSessionStore` if there is still meaningful nested update boilerplate. + +**Validation** +- `cd ui/goose2 && pnpm test -- chatStore chatSessionStore useChat` +- Focused code review for accidental mutation or changed reference behavior. + +**Success Criteria** +- Update logic is easier to read. +- Behavior is unchanged. +- Immer is used selectively and not as an architectural workaround. diff --git a/ui/goose2/ui_improvements/state_management/refactor-progress.md b/ui/goose2/ui_improvements/state_management/refactor-progress.md new file mode 100644 index 0000000000..75f30b58d8 --- /dev/null +++ b/ui/goose2/ui_improvements/state_management/refactor-progress.md @@ -0,0 +1,154 @@ +**Goose2 Zustand Refactor Progress** + +**Current Status** +- Phase 1 implementation complete. +- Phase 2 implementation complete. +- Phase 3 implementation complete. + +**Documents** +- Added master review: `goose2-zustand-state-management-review.md` +- Added master plan: `goose2-zustand-state-management-improvement-plan.md` +- Added per-phase execution plans: + - `phase-1-selector-cleanup.md` + - `phase-2-selector-read-layer.md` + - `phase-3-session-side-effects.md` + - `phase-4-store-boundaries.md` + - `phase-5-session-workflow-failure-policy.md` + - `phase-6-project-store.md` + - `phase-7-persistence.md` + - `phase-8-test-reset-coverage.md` + - `phase-9-optional-immer.md` + +**Decisions** +- Keep simple numbered phases without `A`/`B` sub-phases. +- Do not make test reset cleanup Phase 1. +- Clean up test reset patterns opportunistically when a refactor PR already touches those tests. +- Keep the dedicated test reset and coverage pass as Phase 8. +- Create per-phase execution files so each phase has scope, guardrails, validation, and success criteria. +- In Phase 1, keep existing hook/component public contracts unchanged. For example, `usePersonas()` should keep returning `personas` and `isLoading` even though its current production consumer only uses command functions. +- For Phase 3, use explicit `chatSessionOperations.ts` functions for backend-aware chat-session workflows. Use `patchSession` for local-only store updates. For title and project updates, call backend first and patch local state only after success. + +**Phase Status** + +| Phase | Name | Status | Notes | +| --- | --- | --- | --- | +| 1 | Remove whole-store subscriptions | Complete | Replaced broad subscriptions in `usePersonas.ts`, `useChat.ts`, `Sidebar.tsx`, and `AppShell.tsx`. | +| 2 | Introduce selector-first read layer | Complete | Added initial `chatSessionStore`, `chatStore`, `agentStore`, and `projectStore` selector helpers. Deferred derived values, store-boundary issues, and component-boundary issues to later phases. | +| 3 | Separate session side effects | Complete | Removed generic `updateSession`; local patches use `patchSession`, and title/project persistence uses explicit operations. | +| 4 | Split clearest store boundaries | Pending | Split `agentStore` UI state, then `chatSessionStore` UI state. | +| 5 | Decide session workflow failure policy | Pending | Behavior-policy follow-up after Phase 4 clarifies session-store boundaries. | +| 6 | Refactor project store | Pending | Make mutation policy explicit. | +| 7 | Standardize persistence | Pending | Do after boundaries are clearer. | +| 8 | Test reset and coverage | Pending | Dedicated cleanup after boundary changes stabilize. | +| 9 | Optional Immer | Pending | Skip unless update readability still justifies it. | + +**Known Current-Code Findings To Recheck Before Phase 1** +- Broad subscription scan is clean after Phase 1: + - `rg "use[A-Za-z0-9]+Store\\(\\)" ui/goose2/src` +- `useShallow` is currently unused in `ui/goose2/src`. +- Phase 3 removed `chatSessionStore.updateSession`; local-only session changes now use `patchSession`. +- `projectStore` currently owns local cache, API orchestration, state, active selection, and reorder behavior. +- `agentStore` currently mixes catalog/provider state with persona editor UI state. + +**Next Step** +- Start Phase 4 using `phase-4-store-boundaries.md`. + +**Validation Log** +- Phase 1 broad subscription scan: clean. +- Phase 1 TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 1 focused tests: `cd ui/goose2 && pnpm test -- useChat usePersonas Sidebar` passed. +- Phase 2 Step 1 TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 Step 2 TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 Step 3A TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 Step 3B TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 Step 4A TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 Step 4B TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 2 focused tests: `cd ui/goose2 && pnpm test -- useChat usePersonas Sidebar useProviderInventory` passed. +- Phase 2 `useProviderInventory` / `useShallow` assessment complete: no code change recommended. +- Phase 2 final broad subscription scan: `rg "use[A-Za-z0-9]+Store\\(\\)" ui/goose2/src` found no matches. +- Phase 2 final `useShallow` scan: `rg "useShallow|zustand/shallow" ui/goose2/src` found no matches, as expected. +- Phase 3 TypeScript check: `cd ui/goose2 && pnpm exec tsc --noEmit` passed. +- Phase 3 focused tests: `cd ui/goose2 && pnpm test -- chatSessionOperations chatSessionStore` passed; Vitest ran 109 files / 632 tests. +- Phase 3 final generic `updateSession` scan: no `chatSessionStore.updateSession` action or call sites remain; only explicit `updateSessionTitle` / `updateSessionProject` operation/API names remain. + +**Follow-Ups To Revisit Later** +- Revisit whether `usePersonas()` should remain both a read hook and command hook. Today `AgentsView` already reads `personas` and `personasLoading` through direct store selectors and only uses `usePersonas()` for `createPersona`, `updatePersona`, `deletePersona`, and `refreshFromDisk`. Do not change that contract during Phase 1. +- Revisit the sidebar runtime selector in Phase 2. Phase 1 selects the whole `sessionStateById` record to avoid a complex derived selector, which is still narrower than the whole `chatStore`; Phase 2 should evaluate whether visible sidebar session items need a dedicated selector/helper. +- Keep selector fallbacks stable. Phase 1 found that returning a fresh `[]` from a selector fallback in `useChat` can trigger React snapshot churn; use module-level constants or derive fallback values outside the selector. +- In Phase 8, keep component-test store mocks selector-aware. Phase 1 production code now calls bound hooks with selectors, so mocks need to support `useStore((state) => ...)` as well as any remaining direct mock patterns. +- Phase 1 removed dead code exposed by the selector cleanup: `retryLastMessage`, `buildSkillRetryOptions`, and the unused `findLastIndex` helper. +- After Phase 3 creates explicit title/project workflow actions, revisit backend failure policy for rename and project assignment. Options include backend-first, optimistic with rollback, refresh-on-failure, or optimistic with user-visible error. +- After Phase 4 clarifies `chatSessionStore` boundaries, revisit archive/unarchive. They currently remain store actions with API calls and optimistic-without-rollback behavior: local visibility changes immediately, backend failures are logged, and local state is not restored. + +**Phase 2 Audit Notes** +- Completed Step 1: + - Added `chatSessionSelectors.ts` with `selectSessions`, `selectActiveSessionId`, `selectHasHydratedSessions`, and `selectSessionsLoading`. + - Updated `AppShell.tsx` and `Sidebar.tsx` to use those simple field selectors. +- Completed Step 2: + - Added `chatSelectors.ts` with `selectMessagesBySession` and `selectSessionStateById`. + - Updated `AppShell.tsx` and `Sidebar.tsx` to use those simple field selectors. +- Completed Step 3A: + - Added `agentSelectors.ts` with `selectPersonas` and `selectPersonasLoading`. + - Updated `usePersonas.ts`, `AgentsView.tsx`, and `useChatSessionController.ts` to use persona domain selectors. + - Left persona editor UI state selectors inline so Phase 4 extraction remains visible. +- Completed Step 3B: + - Added `selectSelectedProvider` to `agentSelectors.ts`. + - Updated `AppShell.tsx` and `useProviderSelection.ts` to use the selected-provider selector. + - Left provider lists/loading/actions inline pending later `agentStore` boundary work. +- Completed Step 4A: + - Renamed the internal `ProjectState` type to exported `ProjectStore` because the type includes state and actions. + - Added `projectSelectors.ts` with `selectProjects`. + - Updated direct `projects` field reads in `AppShell.tsx`, `Sidebar.tsx`, `SkillsView.tsx`, `SessionHistoryView.tsx`, and `useChatSessionController.ts`. + - Left project actions, loading, and derived project lookup reads inline pending Phase 6 project-store ownership work. +- Completed Step 4B: + - Updated `SessionHistoryView.tsx` to use existing `selectSessions` and `selectMessagesBySession`. + - A scan found no remaining exact inline reads for selector helpers already created. + - Left derived/id-dependent reads, UI-state reads, one-off loading reads, and action selectors inline. +- Completed `useProviderInventory` / `useShallow` assessment: + - `useProviderInventory` reads `entries` and `loading` with separate narrow selectors. + - Grouping these into an object selector with `useShallow` would not meaningfully reduce rerenders because consumers should still update when either value changes. + - Kept the hook unchanged. +- Simple selector candidates: + - chat: `selectMessagesBySession`, `selectSessionStateById` + - chat sessions: `selectSessions`, `selectActiveSessionId`, `selectHasHydratedSessions`, `selectSessionsLoading` + - agents: `selectPersonas`, `selectPersonasLoading`, `selectSelectedProvider` + - projects: `selectProjects`, `selectFetchProjects`, `selectReorderProjects` +- Derived values should stay derived, not become stored attributes: + - active session from `sessions + activeSessionId` + - Home session from `sessions + homeSessionId` + - project lookup by id from `projects + projectId` + - sidebar grouped session rows from `sessions + messagesBySession + sessionStateById + project ids` + - Phase 4 should recheck active/Home session derivation and sidebar grouping after chat/session UI state moves. + - Phase 6 should recheck project lookup helpers after project-store ownership is clearer. +- Duplicated or legacy state to revisit later: + - `chatStore.activeSessionId` and `chatSessionStore.activeSessionId` duplicate active-session selection. Track for Phase 4 boundary review. + - `agentStore.isLoading` may be a legacy/general loading flag now that personas, agents, and providers have specific loading fields. Track for Phase 4 and test-reset cleanup in Phase 8. +- Imperative derived helper methods to revisit later: + - `chatStore.getActiveMessages` + - `chatSessionStore.getActiveSession` + - `chatSessionStore.getArchivedSessions` + - `projectStore.getActiveProject` + - `agentStore.getActiveAgent` + - `agentStore.getAgentsByPersona` + - `agentStore.getBuiltinPersonas` + - `agentStore.getCustomPersonas` + - Keep these only where useful for `getState()`/callback code; prefer selectors or pure helpers for React reads. +- Store-boundary signals to carry into later phases: + - `agentStore` mixes personas, providers, active agent, selected provider, and persona editor UI. Track for Phase 4. + - `chatStore` mixes messages, runtime, drafts, queue, loading, scroll targets, and cleanup. Track for Phase 4 / Phase 9 reassessment. + - `chatSessionStore` mixes session records, hydration/loading, active selection, creation, archive, mutation, context panel state, and workspace UI. Track for Phase 3 and Phase 4. +- Phase 2 guardrail: + - Selectors should not hide real feature complexity. For `agentStore`, use selectors only for stable repeated domain reads such as personas and selected provider. + - Do not add selectors for persona editor UI state in Phase 2; keep that visible for Phase 4 UI-store extraction. + - If a feature still needs many store dependencies after selector cleanup, track it as a component/store boundary issue instead of adding more selectors. + - Selector files created in Phase 2 are part of the read layer, not proof that the current store boundaries are final. + - When Phase 4 or Phase 6 moves state across stores, update, move, or delete the corresponding Phase 2 selectors in the same change. +- Component-boundary signals to carry into later phases: + - `AppShell` is orchestration-heavy: projects, sessions, Home setup, provider/model choice, keyboard shortcuts, dialogs, sidebar wiring, and chat activation. Track as a later component decomposition concern. + - `Sidebar` performs visible-session filtering, project grouping, runtime badge derivation, search resolver creation, and expanded-project persistence. Track derived helpers during Phase 2 and possible component decomposition later. + - Rename behavior is owned high in `AppShell` and prop-drilled through `Sidebar`/sidebar sections to `SidebarChatRow`, and through `SessionHistoryView` to `SessionCard`. Track for later component/action-controller cleanup. + - Rename row components close edit mode immediately and expose void callbacks, so backend-first rename failures cannot currently show inline pending/error state. Track UX policy in Phase 5. +- Phase 2 implementation plan: + - Start with small chat-session selector helpers because they are simple field selectors. + - Refactor only `AppShell.tsx` and `Sidebar.tsx` first. + - Do not use selectors to hide the larger Phase 3-5 architecture problems.