Skip to content

fix(desktop): evict archived/deleted sessions from messaging slice immediately (#87716) - #87798

Open
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/87716-session-archive-visible
Open

fix(desktop): evict archived/deleted sessions from messaging slice immediately (#87716)#87798
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/87716-session-archive-visible

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Problem

Archiving/deleting a messaging-platform session leaves the row visible for 2-4s. archiveSession/removeSession only applied optimistic removal to $sessions (recents); the messaging slice ($messagingSessions) was only cleaned on the next refresh via dropTombstoned().

Fix

Apply the same optimistic filter to the messaging slice in lockstep, with rollback on RPC failure. +88 lines of tests (4 cases: archive/drop, delete/drop, rollback).

…mediately (NousResearch#87716)

archiveSession/removeSession only applied optimistic removal to $sessions
(recents); the messaging slice ($messagingSessions) was only cleaned on the
next refresh (2-4s delay). Apply the same optimistic filter to the messaging
slice in lockstep, with rollback on RPC failure.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Aug 16, 2026
@jackulau

Copy link
Copy Markdown
Contributor

@webtecnica this and my #87774 are both for #87716, opened about six hours apart on 2026-08-16, and neither of us saw the other. Surfacing it so a maintainer picks one head rather than leaving two on use-session-actions/index.ts. I have read your diff in full.

Your rollback structure is sound and I want to say that first, because the obvious cheap criticism of it is wrong. Snapshotting removedMessagingRows / archivedMessagingRows from $messagingSessions and restoring into that same atom is correct: a real messaging row is never in $sessions, so your if (removed) setSessions(...) branch does not fire for it and nothing gets relocated into recents. You also cover archiveSession and removeSession symmetrically, which is the right scope for the issue.

Where the two heads actually differ

Both of us fix the filter. Only one of us fixes the lookup in front of it, and I think the lookup is the more damaging half.

const removed = $sessions.get().find(session => sessionMatchesStoredId(session, storedSessionId))

refreshSessions fetches recents with SIDEBAR_EXCLUDED_SOURCES, which spreads in every messaging source id, and refreshMessagingSessions fetches the inverse into $messagingSessions. So for a Feishu / Discord / Telegram row that lookup does not return undefined by accident - it returns undefined every time. Four things downstream read it:

line consequence for a messaging row
deleteSession(storedSessionId, removed?.profile) profile is undefined, so the RPC lands on whichever gateway happens to be active
removedIds = [storedSessionId, removed?.id, removed?._lineage_root_id] tombstone covers only the stored id, so a compressed row is not matched and flashes back on the next projects.tree refresh
removedPinId = removed ? sessionPinId(removed) : storedSessionId wrong pin id on a compressed row, so the pin lingers
forgetSessionUnread(removedIds, removed?.profile) wrong profile scope

That is why #87774 adds findSidebarSession (reads both live slices) rather than only mirroring the filter.

The reason this is easy to miss, and it is in the tests

Every case in your describe seeds the row into both atoms:

setSessions([row])
setMessagingSessions([row, storedSession({ id: 'feishu-2', ... })])

With the row in $sessions, the lookup succeeds and all four consequences above disappear - which is exactly why expect(deleteSession).toHaveBeenCalledWith('discord-1', 'default') passes. Drop the setSessions([row]) line so the fixture matches what refreshSessions actually produces, and that assertion becomes toHaveBeenCalledWith('discord-1', undefined) against the current code. Worth running even if this head is the one that lands - it is a one-line change to the fixture and it is the difference between the test proving the fix and the test agreeing with it.

The same substitution makes the rollback assertion expect($sessions.get().map(s => s.id)).toEqual(['telegram-1']) become toEqual([]), which is the honest expectation.

What I would like to do

I am not going to argue mine should win on rebase state, so here is the state plainly: #87774 is rebased onto a72c9ca248 as of today, green, and had to reconcile with main's new archived-slice handling in removeSession (a third disjoint atom that landed after both our PRs). Yours is showing mergeable: UNKNOWN and will very likely hit the same conflict, since it edits the same three lines.

Two ways I am happy to go:

  1. Take the lookup fix into your head. findSidebarSession and restoreSidebarSession are ~35 lines in use-session-actions/utils.ts in my diff; copy them, no attribution needed, and I will close fix(desktop): clear messaging sessions optimistically on archive and delete #87774 as a duplicate of this one.
  2. Land mine and I fold your framing in. The one thing yours does that mine does not is state the eviction rationale per call site in comments tied to refreshMessagingSessions lag, which is clearer than my single shared docstring.

Either is fine by me - say which and I will do my half.

cc @maintainers: two open PRs, same issue, same seam, six hours apart, and main has since added a third slice to the same code path. Whichever head is preferred, the other should be closed rather than left open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants