From 4db2f78ec7f44d00a00b37ffee53c3519be20bd7 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Wed, 22 Jul 2026 22:54:25 -0500 Subject: [PATCH] fix(desktop): honor optimistic session tombstones on list & tree refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refresh racing an in-flight delete/archive resurrected the just-removed row: refreshSessions repopulated $sessions straight from the backend page ignoring tombstones, and the projects.tree prune dropped a tombstone the moment its id left scoped_session_ids — so the grouped lane un-filtered it too. The row only vanished on a later refresh once the RPC finally landed. refreshSessions now filters tombstoned rows (and their lineage tip) before merging. The prune keeps a tombstone while its mutation is still in flight locally via $sessionMutationsInFlight, then hands it back to the normal scoped-based prune once settled. --- .../hooks/use-session-list-actions.test.tsx | 31 +++++++++++ .../session/hooks/use-session-list-actions.ts | 13 ++++- apps/desktop/src/store/projects.test.ts | 53 ++++++++++++++++++- apps/desktop/src/store/projects.ts | 32 ++++++++++- 4 files changed, 126 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/app/session/hooks/use-session-list-actions.test.tsx b/apps/desktop/src/app/session/hooks/use-session-list-actions.test.tsx index 0147d45e4a8b..b5c3ef07217f 100644 --- a/apps/desktop/src/app/session/hooks/use-session-list-actions.test.tsx +++ b/apps/desktop/src/app/session/hooks/use-session-list-actions.test.tsx @@ -62,9 +62,18 @@ vi.mock('@/hermes', async importOriginal => ({ listSidebarSessions: (...args: unknown[]) => listSidebarSessions(...args) })) +// The refresh only reads the optimistic tombstone set; stub it so we don't pull +// the whole projects store (gateway / fs / git) into this hook's test. +const removed = vi.hoisted(() => ({ ids: new Set() })) + +vi.mock('@/store/projects', () => ({ + $removedSessionIds: { get: () => removed.ids } +})) + beforeEach(() => { listSidebarSessions.mockReset() listAllProfileSessions.mockReset() + removed.ids = new Set() setSessions([]) setCronSessions([]) setMessagingSessions([]) @@ -146,6 +155,28 @@ describe('refreshSessions identity + loading hygiene', () => { expect(loadingStates).toEqual([false]) }) + it('drops rows the user just deleted, even when the backend page still lists them', async () => { + // A delete RPC is in flight: the row is tombstoned optimistically but the + // batched refresh still carries it (and a lineage-tip variant). Both must be + // filtered so the optimistic removal never flashes back. + removed.ids = new Set(['b', 'root-c']) + listSidebarSessions.mockResolvedValue( + sidebar({ + sessions: [row('a'), row('b'), row('c', { _lineage_root_id: 'root-c' } as Partial)], + total: 3, + profile_totals: {} + }) + ) + + const { result } = renderHook(() => useSessionListActions({ profileScope: 'default' })) + + await act(async () => { + await result.current.refreshSessions() + }) + + expect($sessions.get().map(s => s.id)).toEqual(['a']) + }) + it('still shows loading for the initial (empty-list) fetch', async () => { listSidebarSessions.mockResolvedValue(sidebar({ sessions: [row('a')], total: 1, profile_totals: {} })) const { result } = renderHook(() => useSessionListActions({ profileScope: 'default' })) diff --git a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts index 38850cad3a77..2031b6806179 100644 --- a/apps/desktop/src/app/session/hooks/use-session-list-actions.ts +++ b/apps/desktop/src/app/session/hooks/use-session-list-actions.ts @@ -11,6 +11,7 @@ import { import { setCronJobs } from '@/store/cron' import { $pinnedSessionIds, $sessionsLimit, bumpSessionsLimit, SIDEBAR_SESSIONS_PAGE_SIZE } from '@/store/layout' import { ALL_PROFILES, normalizeProfileKey } from '@/store/profile' +import { $removedSessionIds } from '@/store/projects' import { $messagingSessions, $selectedStoredSessionId, @@ -180,12 +181,22 @@ export function useSessionListActions({ profileScope }: UseSessionListActionsArg if (refreshSessionsRequestRef.current === requestId) { const recents = result.recents + // Drop rows the user just deleted/archived: a refresh can race an + // in-flight mutation and the backend page still carries the doomed row. + // Honoring the optimistic tombstone keeps the removal from flashing back + // (the tombstone self-clears once projects.tree confirms the delete). + const tombstones = $removedSessionIds.get() + + const incoming = tombstones.size + ? recents.sessions.filter(s => !tombstones.has(s.id) && !(s._lineage_root_id && tombstones.has(s._lineage_root_id))) + : recents.sessions + // Signature-gate the swap (same pattern as cron/messaging): a refresh // that returns content-identical rows must keep the previous array // identity, or every sidebar memo keyed on $sessions recomputes and the // whole list re-renders once per turn/broadcast for nothing. setSessions(prev => { - const next = mergeSessionPage(prev, recents.sessions, sessionsToKeep()) + const next = mergeSessionPage(prev, incoming, sessionsToKeep()) return sameCronSignature(prev, next) ? prev : next }) diff --git a/apps/desktop/src/store/projects.test.ts b/apps/desktop/src/store/projects.test.ts index db19c1d5c6fe..86927b0e9d68 100644 --- a/apps/desktop/src/store/projects.test.ts +++ b/apps/desktop/src/store/projects.test.ts @@ -10,9 +10,13 @@ import { $projectScope, $projectsRpcAvailable, $projectTree, + $removedSessionIds, + $sessionMutationsInFlight, $worktreeRefreshToken, ALL_PROJECTS, + beginSessionMutation, createProject, + endSessionMutation, enterProject, exitProjectScope, openProjectCreate, @@ -21,7 +25,8 @@ import { refreshProjects, refreshProjectTree, refreshWorktrees, - scanAndRecordRepos + scanAndRecordRepos, + tombstoneSessions } from './projects' vi.mock('@/i18n', () => ({ @@ -408,3 +413,49 @@ describe('project tree profile isolation', () => { expect($projectTree.get().map(project => project.id)).toEqual(['profile-b']) }) }) + +describe('tombstone pruning', () => { + const openGatewayReturning = (scopedIds: string[]) => { + const gateway = { + connectionState: 'open', + request: vi.fn().mockResolvedValue({ active_id: null, projects: [], scoped_session_ids: scopedIds }) + } + + activeGateway.mockImplementation(() => gateway as never) + gatewayAtom.set(gateway as never) + + return gateway + } + + beforeEach(() => { + $removedSessionIds.set(new Set()) + $sessionMutationsInFlight.set(new Set()) + }) + + it('keeps an in-flight delete tombstone even when the backend snapshot omits it', async () => { + // Optimistic delete: hide the row, mark the RPC as in flight. + tombstoneSessions(['sess-1']) + beginSessionMutation(['sess-1']) + + // A projects.tree refresh races the pending delete: the id is already gone + // from scope, but the RPC hasn't landed — the tombstone must survive so the + // row doesn't flash back. + openGatewayReturning([]) + await refreshProjectTree() + + expect($removedSessionIds.get().has('sess-1')).toBe(true) + }) + + it('prunes the tombstone once the mutation settles and scope no longer lists it', async () => { + tombstoneSessions(['sess-1']) + beginSessionMutation(['sess-1']) + openGatewayReturning([]) + await refreshProjectTree() + + // Delete RPC settled; the next refresh with the id absent from scope drops it. + endSessionMutation(['sess-1']) + await refreshProjectTree() + + expect($removedSessionIds.get().has('sess-1')).toBe(false) + }) +}) diff --git a/apps/desktop/src/store/projects.ts b/apps/desktop/src/store/projects.ts index 29af6fb91234..20acf671dd25 100644 --- a/apps/desktop/src/store/projects.ts +++ b/apps/desktop/src/store/projects.ts @@ -94,6 +94,32 @@ export function untombstoneSessions(ids: Array): void } } +// Ids whose delete/archive RPC is still in flight. Their tombstones are pinned +// against the projects.tree prune below: a refresh whose snapshot predates the +// mutation completing must NOT drop the tombstone, or the row flashes back until +// the backend catches up. Keyed by id, so concurrent deletes stay independent. +export const $sessionMutationsInFlight = atom>(new Set()) + +function mutateInFlight(ids: Array, add: boolean): void { + const current = $sessionMutationsInFlight.get() + const next = new Set(current) + + for (const id of ids) { + const trimmed = id?.trim() + + if (trimmed) { + add ? next.add(trimmed) : next.delete(trimmed) + } + } + + if (next.size !== current.size) { + $sessionMutationsInFlight.set(next) + } +} + +export const beginSessionMutation = (ids: Array): void => mutateInFlight(ids, true) +export const endSessionMutation = (ids: Array): void => mutateInFlight(ids, false) + // True while the disk scan is in flight (drives the "finding repos" hint). export const $reposScanning = atom(false) @@ -341,7 +367,11 @@ async function refreshProjectTreeOn(gateway: HermesGateway): Promise { const tombstones = $removedSessionIds.get() if (tombstones.size) { - const pending = new Set([...tombstones].filter(id => scoped.has(id))) + // Keep a tombstone while the backend still lists the id (delete pending on + // its side) OR while its mutation is still in flight locally — dropping it + // early flashes the row back until the RPC lands. + const inFlight = $sessionMutationsInFlight.get() + const pending = new Set([...tombstones].filter(id => scoped.has(id) || inFlight.has(id))) if (pending.size !== tombstones.size) { $removedSessionIds.set(pending)