From b0bd70e4b7f0f463c55cea2284d23d27fa8b44ba Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Thu, 24 Sep 2026 18:41:23 -0500 Subject: [PATCH] fix(desktop): keep archived sessions out of the sidebar merge (#118156) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Archiving a session removed it optimistically, but it came back minutes later with the backend never un-archiving it (state.db still archived=1). Two renderer paths let the row back in: 1. mergeSessionPage's survivor filter honored `hidden` and the keep set, never the tombstones — and the keep set reliably names a just-archived chat, because the 30s settle grace keeps it "recently settled" and the optimistic drop only clears $sessions, not a `previous` slice a slower refresh (messaging "Load more") captured earlier. Once ANY path put the row back into $sessions, the survivor filter kept it there across every later refresh. 2. The tombstone guard on incoming rows expires: applyProjectTreePayload prunes a tombstone as soon as the mutation RPC lands and the tree payload omits the id — correct for the tree overlay, but it also removes the last guard a stale recents page has to pass, so a page read before the archive commit could re-inject the row after the prune. The survivor filter now consults $removedSessionIds directly (bare id + lineage root, the same match dropTombstoned applies to incoming rows), re-reading the set at merge time so a removal landing mid-refresh is still honored. A failed RPC untombstones immediately, so nothing is filtered on non-destructive paths. RED→GREEN on origin/main: mergeSessionPage keeps a keep-listed tombstoned row at base and drops it after; the hook-level race test (row seeded in $sessions + settle-grace keep + tombstone) passes only with the filter. Fixes #118156 --- .../hooks/use-session-list-actions.test.tsx | 30 ++++++++++++++++ apps/desktop/src/store/session.test.ts | 36 +++++++++++++++++++ apps/desktop/src/store/session.ts | 17 ++++++++- 3 files changed, 82 insertions(+), 1 deletion(-) 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 9ddf44cc24..27850a7ca0 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 @@ -302,6 +302,36 @@ describe('refreshSessions identity + loading hygiene', () => { expect($sessions.get().map(s => s.id)).toEqual(['a']) }) + it('never resurrects a just-archived row the keep set still names (#118156)', async () => { + // The archive race: the RPC landed (so the in-flight pin released and the + // projects.tree prune dropped the tombstone — hence EMPTY tombstones + // here), but the row is still inside the 30s settle grace, so + // sessionsToKeep() names it. A refresh whose `previous` still holds the + // row must not carry it back through the survivor path. The map from + // tombstone→epoch keeps the exclusion alive exactly as long as the + // tombstone stood, so it must reproduce with the tombstone still set. + removed.ids = new Set(['just-archived']) + + // Seed $sessions with the row still present, as a refresh racing the + // optimistic drop would see it. + setSessions([row('just-archived'), row('mine')]) + // And make the settle grace name it: simulate a turn that just ended. + const { getRecentlySettledSessionIds } = await import('@/store/session-states') + const settled = vi.spyOn({ getRecentlySettledSessionIds }, 'getRecentlySettledSessionIds') + + settled.mockReturnValue(['just-archived']) + + listSidebarSessions.mockResolvedValue(sidebar({ sessions: [row('mine', { message_count: 3 })] })) + + const { result } = renderHook(() => useSessionListActions({ profileScope: 'default' })) + + await act(async () => { + await result.current.refreshSessions() + }) + + expect($sessions.get().map(s => s.id)).toEqual(['mine']) + }) + it('keeps idle recents when the sidebar returns an empty page plus profile errors', async () => { // Backend contract on disk I/O / lock: HTTP 200, recents=[], errors=[{profile}]. // mergeSessionPage only keeps working/pinned/selected, so Yesterday/This-week diff --git a/apps/desktop/src/store/session.test.ts b/apps/desktop/src/store/session.test.ts index 91edde14f6..6af5e50f40 100644 --- a/apps/desktop/src/store/session.test.ts +++ b/apps/desktop/src/store/session.test.ts @@ -69,6 +69,7 @@ import { touchSessionActivity, workspaceCwdForNewSession } from './session' +import { tombstoneSessions, untombstoneSessions } from './session-removal' import { $attentionSessionIds, clearAllSessionStates, @@ -548,6 +549,41 @@ describe('mergeSessionPage', () => { expect(mergeSessionPage(previous, incoming, ['bot-chat']).map(s => s.id)).toEqual(['mine']) }) + it('never resurrects a tombstoned row through the keep set (#118156)', () => { + // Archive flow: the row is tombstoned and dropped optimistically, but it + // is still recently-settled (its turn just ended), so sessionsToKeep() + // names it. A refresh whose `previous` still holds the row (a slice + // captured before the drop, or a resurrection injected by a stale page) + // must not let the survivor path keep it alive: while the tombstone + // stands, the row is on its way out — full stop. + tombstoneSessions(['doomed']) + + try { + const previous = [session({ id: 'doomed' }), session({ id: 'mine' })] + const incoming = [session({ id: 'mine', message_count: 3 })] + + expect(mergeSessionPage(previous, incoming, ['doomed']).map(s => s.id)).toEqual(['mine']) + } finally { + untombstoneSessions(['doomed']) + } + }) + + it('matches a tombstone by lineage root, not just the live tip (#118156)', () => { + // archiveSession() tombstones the stored id AND the lineage root; the + // survivor filter must honor both, the same way dropTombstoned does for + // incoming rows. + tombstoneSessions(['root']) + + try { + const previous = [session({ id: 'tip', _lineage_root_id: 'root' }), session({ id: 'mine' })] + const incoming = [session({ id: 'mine' })] + + expect(mergeSessionPage(previous, incoming, ['root']).map(s => s.id)).toEqual(['mine']) + } finally { + untombstoneSessions(['root']) + } + }) + it('keeps a pinned session that has aged off the recent page', () => { // Repro of "loses pins until you refresh": a pinned chat falls off the // most-recent page, so the server stops returning it. A hard replace would diff --git a/apps/desktop/src/store/session.ts b/apps/desktop/src/store/session.ts index 1058b2af9d..fa1d55e9cc 100644 --- a/apps/desktop/src/store/session.ts +++ b/apps/desktop/src/store/session.ts @@ -16,7 +16,7 @@ import type { TileSessionFocusStamp } from '@/lib/session-timer-since' import { persistBoolean, persistString, readJson, storedBoolean, storedString, writeJson } from '@/lib/storage' import type { SessionInfo, UsageStats } from '@/types/hermes' -import { isSessionRemovalPending } from './session-removal' +import { $removedSessionIds, isSessionRemovalPending } from './session-removal' import type { SessionOwnerRoute, SessionOwnerScope } from './session-request-router' import { clearUnreadOnOpen } from './session-unread-remote' @@ -704,12 +704,27 @@ export function mergeSessionPage( merged.flatMap(session => (session._lineage_ids ?? []).map(id => `${profileKeyOf(session)}::${id}`)) ) + // The tombstone set is re-read here, not at the caller: optimistic removal + // can land between `previous` being captured and this merge committing (a + // messaging "Load more" holds its slice for a long time), and a row the + // user archived or deleted must not survive through the keep set — the + // settle grace keeps a just-archived chat "recently settled" for 30s, which + // is exactly the window the survivor path used to resurrect it (#118156). + // Same bare-id + lineage-root match as dropTombstoned applies to incoming + // rows; a failed RPC untombstones immediately, so the filter is only ever + // as sticky as the removal itself. + const tombstones = $removedSessionIds.get() + const tombstoned = (session: SessionInfo): boolean => + tombstones.size > 0 && + (tombstones.has(session.id) || (session._lineage_root_id != null && tombstones.has(session._lineage_root_id))) + const survivors = previous.filter( session => // The keep-list answers "live, not listed yet" — a hidden row (canonical // Bot Chat, room plumbing) is LISTED-NEVER by design, so a live turn or // open tab must not resurrect it into the sidebar (#113273). !session.hidden && + !tombstoned(session) && !incomingIds.has(identity(session)) && !incomingLineageKeys.has(lineageIdentity(session)) && !incomingLineageIdMembers.has(identity(session)) &&