From 3e21cfdebbf790e3b2a89b01084a8aa86914c9a5 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Thu, 2 Jul 2026 21:20:57 -0500 Subject: [PATCH] fix(desktop): clear stale active todos on turn end AND on rehydration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A turn that ends without a final `todo` update left the composer "Tasks N/M" panel pinned with its last item stuck pending/in_progress, and it survived restarts because the panel is read back from stored session history. Two coupled fixes (the first alone is undone by the second path): - Turn end: clear a still-active todo list on `message.complete` and on a terminal `error` (new `clearActiveSessionTodos` — active lists only; a finished list keeps its short linger so the last checkmark still lands). - Rehydration: `hydrateFromStoredSession` runs *after* a turn completes, so an "active" stored list is stale, not in-flight. It now restores only a *finished* list (via new `todosForHydration`) and drops anything still active — otherwise it re-pinned the panel right after the turn-end clear and resurrected it on every restart. Salvages #52996 (@0disoft): the fix shape (clearActiveSessionTodos on turn completion, preserving the finished-list linger) is carried forward and ported onto the current use-message-stream/ folder split (gateway-event.ts), then extended to the rehydration path per review. Co-authored-by: 0disoft --- apps/desktop/src/app/desktop-controller.tsx | 18 ++-- .../hooks/use-message-stream/gateway-event.ts | 6 ++ .../use-message-stream/todo-cleanup.test.tsx | 93 +++++++++++++++++++ apps/desktop/src/store/todos.test.ts | 60 +++++++++++- apps/desktop/src/store/todos.ts | 26 ++++++ 5 files changed, 195 insertions(+), 8 deletions(-) create mode 100644 apps/desktop/src/app/session/hooks/use-message-stream/todo-cleanup.test.tsx diff --git a/apps/desktop/src/app/desktop-controller.tsx b/apps/desktop/src/app/desktop-controller.tsx index b4a1c863e..f3a07183e 100644 --- a/apps/desktop/src/app/desktop-controller.tsx +++ b/apps/desktop/src/app/desktop-controller.tsx @@ -76,7 +76,7 @@ import { setRememberedSessionId } from '../store/session' import { onSessionsChanged } from '../store/session-sync' -import { clearSessionTodos, setSessionTodos, todoListActive } from '../store/todos' +import { clearSessionTodos, setSessionTodos, todosForHydration } from '../store/todos' import { openUpdatesWindow, startUpdatePoller, stopUpdatePoller } from '../store/updates' import { isSecondaryWindow } from '../store/windows' @@ -484,13 +484,17 @@ export function DesktopController() { storedSessionId ) - // Seed the status stack's todo group from history — but only while - // the plan is still in flight, so reopening an old chat doesn't pin - // its finished todo list above the composer forever. - const todos = latestSessionTodos(messages) + // Rehydration runs *after* a turn completes, so an "active" stored + // list (last `todo` still pending/in_progress) means the turn ended + // without a final update — it's stale, not in-flight. Re-seeding it + // would re-pin "Tasks N/M" above the composer and undo the turn-end + // clear (and survive restarts, since it's read back from history). + // todosForHydration restores only a *finished* list (its short linger + // shows the last checkmark); anything still active is dropped. + const restored = todosForHydration(latestSessionTodos(messages)) - if (todos && todoListActive(todos)) { - setSessionTodos(runtimeSessionId, todos) + if (restored) { + setSessionTodos(runtimeSessionId, restored) } else { clearSessionTodos(runtimeSessionId) } diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts index 8bb401093..f36d74ad7 100644 --- a/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts +++ b/apps/desktop/src/app/session/hooks/use-message-stream/gateway-event.ts @@ -37,6 +37,7 @@ import { setYoloActive } from '@/store/session' import { clearSessionSubagents, pruneDelegateFallbackSubagents, upsertSubagent } from '@/store/subagents' +import { clearActiveSessionTodos } from '@/store/todos' import { recordToolDiff } from '@/store/tool-diffs' import { notifyWorkspaceChanged, toolMayMutateFiles } from '@/store/workspace-events' import type { RpcEvent } from '@/types/hermes' @@ -308,6 +309,10 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) { // prompt, and vice versa. clearAllPrompts(sessionId) clearClarifyRequest(undefined, sessionId) + // Turn ended without a final `todo` update — drop a still-unfinished + // list so "Tasks N/M" doesn't stay pinned above the composer with the + // last item stuck pending/in_progress. Finished lists keep their linger. + clearActiveSessionTodos(sessionId) setSessionCompacting(sessionId, false) flushQueuedDeltas(sessionId) @@ -588,6 +593,7 @@ export function useGatewayEventHandler(deps: GatewayEventDeps) { if (sessionId) { clearAllPrompts(sessionId) clearClarifyRequest(undefined, sessionId) + clearActiveSessionTodos(sessionId) setSessionCompacting(sessionId, false) compactedTurnRef.current.delete(sessionId) } diff --git a/apps/desktop/src/app/session/hooks/use-message-stream/todo-cleanup.test.tsx b/apps/desktop/src/app/session/hooks/use-message-stream/todo-cleanup.test.tsx new file mode 100644 index 000000000..6b676976e --- /dev/null +++ b/apps/desktop/src/app/session/hooks/use-message-stream/todo-cleanup.test.tsx @@ -0,0 +1,93 @@ +import { QueryClient } from '@tanstack/react-query' +import { act, cleanup, render, waitFor } from '@testing-library/react' +import { useEffect, useRef } from 'react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import type { ClientSessionState } from '@/app/types' +import { createClientSessionState } from '@/lib/chat-runtime' +import type { TodoItem } from '@/lib/todos' +import { $todosBySession, clearSessionTodos, setSessionTodos } from '@/store/todos' +import type { RpcEvent } from '@/types/hermes' + +import { useMessageStream } from './index' + +const SID = 'session-1' +const todo = (id: string, status: TodoItem['status']): TodoItem => ({ content: `task ${id}`, id, status }) + +let handleEvent: ((event: RpcEvent) => void) | null = null + +function Harness() { + const activeSessionIdRef = useRef(SID) + const sessionStateByRuntimeIdRef = useRef(new Map()) + const queryClientRef = useRef(new QueryClient()) + + const stream = useMessageStream({ + activeSessionIdRef, + hydrateFromStoredSession: vi.fn(async () => undefined), + queryClient: queryClientRef.current, + refreshHermesConfig: vi.fn(async () => undefined), + refreshSessions: vi.fn(async () => undefined), + sessionStateByRuntimeIdRef, + updateSessionState: (sessionId, updater) => { + const current = sessionStateByRuntimeIdRef.current.get(sessionId) ?? createClientSessionState() + const next = updater(current) + sessionStateByRuntimeIdRef.current.set(sessionId, next) + + return next + } + }) + + useEffect(() => { + handleEvent = stream.handleGatewayEvent + }, [stream.handleGatewayEvent]) + + return null +} + +async function mountStream() { + render() + await waitFor(() => expect(handleEvent).not.toBeNull()) +} + +const complete = () => act(() => handleEvent!({ payload: { text: 'done' }, session_id: SID, type: 'message.complete' })) + +describe('useMessageStream turn-end todo cleanup', () => { + beforeEach(() => { + handleEvent = null + clearSessionTodos(SID) + }) + + afterEach(() => { + cleanup() + clearSessionTodos(SID) + vi.restoreAllMocks() + }) + + it('drops a still-active task list when the turn completes', async () => { + await mountStream() + setSessionTodos(SID, [todo('a', 'completed'), todo('b', 'in_progress')]) + + complete() + + expect($todosBySession.get()[SID]).toBeUndefined() + }) + + it('keeps a finished list on completion so its linger shows the final checkmarks', async () => { + await mountStream() + setSessionTodos(SID, [todo('a', 'completed')]) + + complete() + + // Not cleared immediately — the finished-list linger still owns it. + expect($todosBySession.get()[SID]).toHaveLength(1) + }) + + it('drops a still-active task list when the turn errors out', async () => { + await mountStream() + setSessionTodos(SID, [todo('a', 'in_progress')]) + + act(() => handleEvent!({ payload: { message: 'boom' }, session_id: SID, type: 'error' })) + + expect($todosBySession.get()[SID]).toBeUndefined() + }) +}) diff --git a/apps/desktop/src/store/todos.test.ts b/apps/desktop/src/store/todos.test.ts index 544706df9..1f1abf2e1 100644 --- a/apps/desktop/src/store/todos.test.ts +++ b/apps/desktop/src/store/todos.test.ts @@ -2,7 +2,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { TodoItem } from '@/lib/todos' -import { $todosBySession, clearSessionTodos, setSessionTodos } from './todos' +import { + $todosBySession, + clearActiveSessionTodos, + clearSessionTodos, + setSessionTodos, + todosForHydration +} from './todos' const todo = (id: string, status: TodoItem['status']): TodoItem => ({ content: `task ${id}`, id, status }) @@ -45,3 +51,55 @@ describe('setSessionTodos finished-list auto-clear', () => { expect($todosBySession.get().s1).toHaveLength(2) }) }) + +describe('clearActiveSessionTodos (turn-end cleanup)', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + clearSessionTodos('s1') + vi.useRealTimers() + }) + + it('drops a still-active list when the turn has ended', () => { + setSessionTodos('s1', [todo('a', 'completed'), todo('b', 'in_progress')]) + + clearActiveSessionTodos('s1') + + expect($todosBySession.get().s1).toBeUndefined() + }) + + it('leaves a finished list to its normal linger instead of clearing immediately', () => { + setSessionTodos('s1', [todo('a', 'completed')]) + + clearActiveSessionTodos('s1') + + expect($todosBySession.get().s1).toHaveLength(1) + vi.advanceTimersByTime(5_000) + expect($todosBySession.get().s1).toBeUndefined() + }) + + it('is a no-op when the session has no todos', () => { + clearActiveSessionTodos('s1') + + expect($todosBySession.get().s1).toBeUndefined() + }) +}) + +describe('todosForHydration (stale-active guard on restore)', () => { + it('does not restore an active list (stale after a completed turn)', () => { + expect(todosForHydration([todo('a', 'completed'), todo('b', 'in_progress')])).toBeNull() + expect(todosForHydration([todo('a', 'pending')])).toBeNull() + }) + + it('restores a finished list so its linger shows the final checkmarks', () => { + const finished = [todo('a', 'completed'), todo('b', 'cancelled')] + + expect(todosForHydration(finished)).toEqual(finished) + }) + + it('returns null when there is nothing stored', () => { + expect(todosForHydration(null)).toBeNull() + }) +}) diff --git a/apps/desktop/src/store/todos.ts b/apps/desktop/src/store/todos.ts index 20228bb91..31aed642f 100644 --- a/apps/desktop/src/store/todos.ts +++ b/apps/desktop/src/store/todos.ts @@ -16,6 +16,17 @@ export const $todosBySession = atom>({}) export const todoListActive = (todos: readonly TodoItem[]) => todos.some(t => t.status === 'pending' || t.status === 'in_progress') +// Decide which todo list to restore when rehydrating a session from stored +// history. Rehydration runs *after* a turn completes, so an active list (last +// item still pending/in_progress) is stale — the turn ended without a final +// `todo` update — and must NOT be re-pinned (that would undo the turn-end +// clear and, because it's read back from history, resurrect on restart). Only +// a finished list is restored, so its short linger shows the last checkmark. +// Returns null when there's nothing to restore (caller should clear). +export function todosForHydration(todos: readonly TodoItem[] | null): TodoItem[] | null { + return todos && !todoListActive(todos) ? [...todos] : null +} + // Once a list finishes (every item completed/cancelled), the final state // lingers just long enough to see the last checkmark land, then the group // drops out of the stack on its own. @@ -62,3 +73,18 @@ export function clearSessionTodos(sid: string) { const { [sid]: _drop, ...rest } = map $todosBySession.set(rest) } + +// Drop a still-active todo list (any pending/in_progress item) — used at turn +// end, when an unfinished list means the turn stopped without a final `todo` +// update, so the "Tasks N/M" panel would otherwise stay pinned above the +// composer forever. A finished list is left untouched so its short linger +// still shows the last checkmark landing. +export function clearActiveSessionTodos(sid: string) { + const todos = $todosBySession.get()[sid] + + if (!todos || !todoListActive(todos)) { + return + } + + clearSessionTodos(sid) +}