Skip to content

fix(desktop): clear messaging sessions optimistically on archive and delete - #87774

Open
jackulau wants to merge 3 commits into
NousResearch:mainfrom
jackulau:fix/messaging-session-archive-optimistic-clear-87716
Open

fix(desktop): clear messaging sessions optimistically on archive and delete#87774
jackulau wants to merge 3 commits into
NousResearch:mainfrom
jackulau:fix/messaging-session-archive-optimistic-clear-87716

Conversation

@jackulau

Copy link
Copy Markdown
Contributor

What does this PR do?

Archiving or deleting a messaging-platform session (Telegram, Discord, Feishu, Slack, ...) left the row in the sidebar until the next refresh landed, the 2-4s of stale UI in the report.

The sidebar is two disjoint atoms, not one. refreshSessions fetches recents with SIDEBAR_EXCLUDED_SOURCES, which spreads in every id from MESSAGING_SESSION_SOURCE_IDS, and refreshMessagingSessions fetches the inverse into $messagingSessions. archiveSession and removeSession optimistically filtered $sessions only, so for a messaging row the optimistic clear was a no-op.

The lookup in front of that filter has the same blind spot, and it is the more damaging half. Both actions start with

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

and then derive three separate things from it:

  • removed?.profile, which routes the RPC (deleteSession(id, profile) / setSessionArchived(id, true, profile))
  • removed?._lineage_root_id, which the tombstone needs to match a compressed row
  • removed itself, which is the snapshot the catch restores from

For a messaging row that lookup does not return undefined by accident, it returns undefined always. So the delete goes out with no profile, the tombstone covers only the stored id, and if (removed) { ... } in the rollback is dead code: a failed archive tears the row out of the UI and never puts it back. Adding the optimistic $messagingSessions clear on its own would have made that last one worse, since it would start actually removing the row it cannot restore.

So the fix is two small helpers in utils.ts rather than a second setSessions call:

  • findSidebarSession(storedSessionId) reads $sessions and falls back to $messagingSessions. resolveStoredSession in the same file already reads all three slices for exactly this reason, so this follows the local precedent.
  • restoreSidebarSession(session) puts a row back in the slice it came from, keyed on isMessagingSource(session.source). Restoring into $sessions unconditionally would relocate a messaging row into recents, where it does not render and where the next refreshSessions would drop it again: a rollback that looks like it worked and then silently loses the row a second time.

Answering the two things I said I would check on the issue

deleteSession has the identical omission. Confirmed and fixed here. removeSession is that path (session-actions-menu and the tile menu both route delete through it), and it had all four symptoms above, not just the visible one.

Does the tombstone cover messaging ingestion, or only recents? It covers all of it, so the optimistic clear is safe and no re-add race remains. dropTombstoned is applied at every ingestion point in use-session-list-actions.ts: lines 139 and 192 on the messaging paths, 276 and 321 on the recents paths. That was the condition I flagged as the reason a clear alone might not be enough; it does not apply.

Related Issue

Fixes #87716

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/session/hooks/use-session-actions/utils.ts: added findSidebarSession (both-slice lookup) and restoreSidebarSession (slice-preserving rollback), with comments explaining why the two atoms are disjoint.
  • apps/desktop/src/app/session/hooks/use-session-actions/index.ts: removeSession and archiveSession now look the row up with findSidebarSession, also filter $messagingSessions optimistically, and roll back through restoreSidebarSession. The wasSelected re-lookup in removeSession's catch uses the same helper, so a messaging row now restores its usage counters too.
  • apps/desktop/src/app/session/hooks/use-session-actions.test.tsx: 4 regression tests.

How to Test

  1. Connect a messaging platform, so the sidebar shows a section fed by $messagingSessions (Feishu in the report).
  2. Archive or delete one of those rows. Before: it stays visible until the next refresh. After: it disappears on click.
  3. To see the rollback half, make the RPC fail (stop the gateway, or point the profile at a dead backend) and archive a messaging row. Before: the row vanishes from the UI and the error toast fires, but it is never restored. After: it comes back into its own section, not into recents.

The 4 new tests cover the same ground:

  • archiving drops the row from the messaging slice, not only from recents fails on main with the row still in $messagingSessions.
  • deleting routes the RPC through the profile of the row being deleted fails on main with deleteSession called as (id, undefined) instead of (id, 'work'). This is the silent half of the bug, and the reason it is worth a test rather than a comment.
  • rolls a failed archive back into the messaging slice, not into recents pins restoreSidebarSession's routing.
  • still rolls a failed archive of a local row back into recents is the non-regression guard for the desktop/CLI path.

Note on verification: I could not run vitest locally in this checkout, so CI is the gate on these. scripts/check-windows-footguns.py --all passes (973 files scanned).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 16, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(desktop): clear messaging sessions optimistically on archive and delete

  • apps/desktop/src/app/session/hooks/use-session-actions/utils.ts: findSidebarSession prefers $sessions then $messagingSessions, while restoreSidebarSession decides the target slice via isMessagingSource(session.source). The two helpers encode the "which slice" decision differently; they agree given the documented disjointness, but a single helper returning (row, slice) would keep the invariant in one place and survive a future case where a row legitimately appears in both slices.
  • restoreSidebarSession unconditionally prepends the restored row, losing its original position in the messaging slice. For a recency-sorted messaging list a stale row can surface at the top after a failed archive. Minor — it matches the previous $sessions rollback behavior, but it now also applies to messaging lists.
  • Tests cover archive success/failure across both slices well, but there is no test for the removeSession failure rollback path (the restoreSidebarSession(removed) call in the delete catch) — the success-path routing is tested, the rollback routing only for archive.

@jackulau
jackulau force-pushed the fix/messaging-session-archive-optimistic-clear-87716 branch from 3731569 to 11c7a0f Compare August 17, 2026 02:40
@jackulau

Copy link
Copy Markdown
Contributor Author

Took the third point, and it was the right catch. Pushed as 11c7a0f26 (branch also rebased onto current upstream/main, 67 commits, clean).

3. Missing delete-rollback coverage. Correct, and the gap was worse than "one path untested": delete rolls back through the same restoreSidebarSession helper but from a different catch, so any regression that dropped or misrouted that call would have left the archive tests green and told me nothing. Added two tests mirroring the archive pair, so the routing is pinned in both directions rather than only for the messaging slice.

Mutation-verified, two independent mutations because they prove different things:

  • Forcing restoreSidebarSession to always write $sessions (the original bug): 2 failed, 4 passed, the messaging test of each pair.
  • Deleting the restoreSidebarSession(removed) call from the delete catch: 2 failed, 4 passed, and the two failures are exactly the two new tests while the archive pair stays green. That is the specific thing the old suite could not detect.

Full file 60 passed, tsc --noEmit and eslint clean at the rebased head.

2. Unconditional prepend losing position. This one I checked and I do not think it is reachable for the messaging slice, so I have left it alone rather than adding a sort I cannot justify. The atom order is not the display order there: messagingGroups in chat/sidebar/index.tsx re-sorts every platform bucket at render with [...list].sort((a, b) => sessionTime(b) - sessionTime(a)) (sessionTime is sessionRecency), and then sorts the sections themselves by their head row's recency. So a restored row lands back at its recency position no matter where in the array it was reinserted, and the pinned rows are resolved through sessionByAnyId with order coming from pinnedSessionIds, not from slice order either. The prepend does survive in recents, but that is the pre-existing $sessions behavior this PR does not change, and it is out of scope here.

If the messaging sections ever stop re-sorting at render, this becomes real, so it is worth knowing it is currently load-bearing.

1. Two helpers encoding the slice decision differently. I read this one as deliberate rather than accidental, and I would rather not unify them. The two helpers are answering different questions:

  • findSidebarSession is a tolerant lookup: find the row wherever it happens to be. Preferring $sessions and falling through is the right shape for that.
  • restoreSidebarSession is an authoritative placement: put the row where the fetchers will keep it. isMessagingSource(session.source) is not a second opinion about which slice the row is in, it is the exact predicate the fetchers partition on (use-session-list-actions.ts:139 and :321 both filter with isMessagingSource, and recents excludes MESSAGING_SESSION_SOURCE_IDS via SIDEBAR_EXCLUDED_SOURCES). So restore agrees with the next refresh by construction, not by coincidence.

That is also why the "future case where a row legitimately appears in both slices" cuts the other way for me. A combined (row, slice) helper would restore into whichever slice the lookup found it in, which applies the tolerant answer to the authoritative decision. If a row were ever in both, the fetcher would still put it in exactly one on the next refresh, and the lookup-derived slice could disagree with it. The duplication here is one predicate call, and the cost of removing it is that the placement stops being derivable from the row itself.

Happy to be argued out of this if you would rather have the single helper anyway.

@jackulau
jackulau force-pushed the fix/messaging-session-archive-optimistic-clear-87716 branch from 11c7a0f to 47018c7 Compare August 17, 2026 23:58
@jackulau

Copy link
Copy Markdown
Contributor Author

The red check:test:ui:shard-2of3 on the current head is not from this PR. It is a
time-of-day flake in src/app/chat/sidebar/session-row.test.tsx, a file this PR does not
touch:

AssertionError: expected '5m, Yesterday at 11:56 PM' to match /^5m, Today at /
 Tests  1 failed | 1401 passed (1402)

That test builds its fixture from Date.now() - 5 * 60, then asserts the label starts with
Today at. The shard ran at 00:01 UTC, so five minutes earlier was the previous day and
formatMessageTimestamp correctly returned the yesterday label. Any PR whose UI shard lands
between 00:00 and 00:05 local hits it.

I have opened #88876 to pin the clock in that test. It is independent of this PR, so this one
should go green on its next CI run either way; I do not have rerun rights on the job.

The three files this PR does change are covered by use-session-actions.test.tsx, which is at
60/60 locally on the current rebase, with tsc -p . --noEmit clean.

@jackulau
jackulau force-pushed the fix/messaging-session-archive-optimistic-clear-87716 branch 2 times, most recently from f75ea85 to 1c8d200 Compare August 20, 2026 04:36
@jackulau

Copy link
Copy Markdown
Contributor Author

Rebased onto main (a72c9ca248). This one had gone CONFLICTING, and the conflict is worth a paragraph because resolving it meant rewriting three lines that landed on main after this PR opened.

What collided

main independently added the archived view's own store to removeSession's lookup / eviction / rollback triple:

const removedFromMain = $sessions.get().find(...)
const removed = removedFromMain ?? $archivedSessions.get().find(...)

That is the same class of bug this PR fixes, for a different slice. The sidebar is not two atoms, it is three, and none of them is a superset of another: recents are fetched with SIDEBAR_EXCLUDED_SOURCES, messaging rows go to $messagingSessions, archived rows are excluded from both by design.

So I unioned rather than picked a side. The lookup now resolves across all three, and all three are evicted:

const removedFromLive = findSidebarSession(storedSessionId)   // recents ?? messaging

const removed =
  removedFromLive ?? $archivedSessions.get().find(session => sessionMatchesStoredId(session, storedSessionId))

The one place it cannot be unioned

The rollback. restoreSidebarSession routes a failed mutation back into the live slice it came from and knows nothing about the archived store, which is restored from its own previousArchived snapshot two lines further down. Running both would put an archived row back into recents as well: it would show up twice until the next refresh, then vanish from the Archived filter it was deleted from - a rollback that looks like it worked and then loses the row anyway, which is exactly the failure this PR exists to remove.

Hence removedFromLive for the restore and removed for the profile/lineage lookups. main's if (removedFromMain) guard was doing the same job; this keeps it and widens it to cover messaging rows too.

Third commit

That distinction is invisible in the diff and easy to erase later, and the archived path had no test in this file, so I pinned it: a failed delete of an archived row must restore in exactly one place. Passing restoreSidebarSession the wider removed fails it:

× rolls a failed delete of an archived row back into the archived view only
AssertionError: expected [ 'stored-archived-1' ] to deeply equal []

I would rather that line be guarded by a test than by a comment, given I am the one who rewrote it during a rebase rather than the person who wrote it originally.

Verification

  • vitest run --project ui src/app/session/hooks/use-session-actions.test.tsx - 61 passed, including main's existing coverage on this path.
  • tsc -p tsconfig.json --noEmit - clean.
  • eslint on all three touched files - clean.
  • Scope unchanged: still only use-session-actions/index.ts, use-session-actions/utils.ts, and the test file.

If a maintainer would rather keep main's two-slice shape and have this PR rebase to fit around it instead, say so and I will restructure - but then the messaging row still needs a home in that lookup, and I do not see a way to give it one without the three-way resolve above.

@jackulau

Copy link
Copy Markdown
Contributor Author

Cross-reference for whoever triages this: #87798 by @webtecnica is the same fix for the same issue, opened about six hours after this one on 2026-08-16. My backward duplicate sweep only surfaced it today. Full comparison is on that PR rather than duplicated here.

Short version, so nobody has to click: both heads fix the optimistic filter. This one also fixes the lookup in front of it, which is the half that leaks past the UI - for a messaging row $sessions.get().find(...) returns undefined every time, so the delete RPC goes out with no profile, the tombstone misses _lineage_root_id on a compressed row, and the pin id is wrong. I offered @webtecnica either direction: they take the two helpers from utils.ts and I close this, or this one lands and I fold in their per-call-site framing.

Whichever head is preferred, the other should be closed rather than left open - main has since added a third disjoint slice ($archivedSessions) to the same three lines, so two open heads here is now a three-way conflict waiting to happen.

…delete

The sidebar keeps recents and messaging rows in two disjoint atoms.
refreshSessions fetches recents with SIDEBAR_EXCLUDED_SOURCES, which
spreads in every messaging source id, and refreshMessagingSessions
fetches the inverse into $messagingSessions. archiveSession and
removeSession only filtered $sessions, so a Telegram, Discord or Feishu
row stayed on screen until the next refresh landed.

The lookup in front of that filter had the same blind spot, and it is
the more damaging half. Both actions read the row to derive the profile
that routes the RPC, the _lineage_root_id the tombstone needs in order
to match a compressed row, and the snapshot the rollback restores from.
For a messaging row that lookup did not return undefined by accident, it
returned undefined always: the delete went out with no profile, the
tombstone covered only the stored id, and the failure rollback was
unreachable.

findSidebarSession reads both slices, and restoreSidebarSession puts a
failed mutation back in the slice it came from, so a rollback cannot
quietly relocate a messaging row into recents where the next
refreshSessions would drop it a second time.

Fixes NousResearch#87716
The archive path had rollback coverage in both directions, the delete
path had none: only its success-path routing was pinned. Delete rolls
back through the same restoreSidebarSession helper but from a different
catch, so a regression that dropped or misrouted that call would have
left the archive tests green.

Two tests, mirroring the archive pair so the routing is pinned in both
directions rather than only for the messaging slice.

Verified by mutation:

* forcing restoreSidebarSession to always write $sessions fails the
  messaging test of each pair (2 failed, 4 passed).
* deleting the restoreSidebarSession(removed) call from the delete
  catch fails exactly the two new tests and leaves the archive pair
  green (2 failed, 4 passed).

Full file: 60 passed. tsc --noEmit and eslint clean.

Refs NousResearch#87716
…slices

The rebase onto main had to reconcile this branch with an independent fix that
added the archived view's own store to the same lookup / eviction / rollback
triple in removeSession. Both changes are the same class of bug for a different
slice, so the resolution unions them rather than picking a side: the lookup now
resolves across recents, messaging and archived, and all three are evicted.

The rollback cannot be unioned the same way. restoreSidebarSession routes a
failed mutation back into the LIVE slice it came from and knows nothing about
the archived store, which is restored from its own previousArchived snapshot.
Running both would put an archived row back into recents as well: it would
appear twice until the next refresh, then vanish from the Archived filter it
was deleted from. So the live restore is keyed on removedFromLive, and the
archived case is left to the snapshot.

That distinction is invisible in the diff and easy to erase later, and the
archived path had no test here, so pin it: a failed delete of an archived row
must restore in exactly one place. Passing restoreSidebarSession the wider
`removed` instead fails this test with the row leaked into recents.
@jackulau
jackulau force-pushed the fix/messaging-session-archive-optimistic-clear-87716 branch from 1c8d200 to 8e44f0a Compare August 21, 2026 23:02
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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Archiving/deleting a messaging-platform session leaves the row visible for 2-4s

3 participants