Skip to content

fix(desktop): dismiss file preview tabs when switching conversations (#39657) - #39712

Closed
maxmilian wants to merge 1 commit into
NousResearch:mainfrom
maxmilian:fix/desktop-preview-tab-session-scope
Closed

fix(desktop): dismiss file preview tabs when switching conversations (#39657)#39712
maxmilian wants to merge 1 commit into
NousResearch:mainfrom
maxmilian:fix/desktop-preview-tab-session-scope

Conversation

@maxmilian

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the cross-conversation persistence half of #39657: opening an image/file preview in one conversation left the preview tab visible after switching to a different conversation that does not contain that attachment.

Related Issue

Addresses #39657 (the persistence issue). The top-right overlap with the window controls reported in the same issue is a separate titlebar/layout concern and is intentionally left as a follow-up — see Scope below.

Root cause

File preview tabs live in $filePreviewTabs (apps/desktop/src/store/preview.ts), a flat global atom whose FilePreviewTab records carried no session association. The live preview ($previewTarget) is already re-scoped per session — usePreviewRouting's effect reads the per-session $sessionPreviewRegistry on every session change and resets the target. But $filePreviewTabs was never touched on a session change, so a file tab opened in session A stayed mounted when you switched to session B.

Clicking an image/file attachment routes through setCurrentSessionPreviewTarget(..., 'manual', ...)tryOpenFilePreview$filePreviewTabs, which is exactly the leaking path.

Fix

  • Tag each FilePreviewTab with the sessionId it was opened under (threaded through tryOpenFilePreview/openFilePreviewTarget from the sessionId the caller already passes).
  • Add syncFilePreviewTabsForSession(sessionId): keeps only tabs belonging to the given session, and re-selects the live-preview tab if the active file tab was dropped. Re-syncing the current session is a no-op, so it's safe to call on every render.
  • usePreviewRouting's session effect now calls it alongside the existing $previewTarget reset — clearing other-session tabs on a real session switch, without clobbering same-session tabs when the effect re-runs on registry updates.

Net production change is ~8 lines of logic; no new dependencies, no behavior change for the same-session case.

How to Test

  1. Open the Desktop app, open a conversation that has an image/file attachment.
  2. Click the attachment to open its preview tab (top-right rail).
  3. Switch to a different conversation that does not contain that attachment.
  4. Before: the file preview tab persists. After: the tab is dismissed and the rail falls back to the live-preview tab.

Automated coverage (npm run test:ui, run from apps/desktop):

  • store/preview.test.ts — drops other-session tabs on switch; keeps same-session tabs on re-sync.
  • app/session/hooks/use-preview-routing.test.tsx — file tab is dismissed when the routed session changes. This test fails without the effect-level call, so it pins the regression.

All preview/routing tests green; tsc -b and eslint clean. (Pre-existing unrelated failures in model-settings/toolset/skills/streaming/pane-shell suites reproduce on a clean main checkout too.)

Scope — intentionally not changed

  • Top-right overlap (the other half of [Bug]: Desktop app image preview tab or title bar overlaps with top-right UI buttons #39657). The preview tab bar visually colliding with the window controls is a titlebar/z-index layout problem (app/shell --titlebar-tools-right/--titlebar-tools-width reservations), not a state bug. It needs a packaged Desktop build to repro and verify a CSS fix with before/after evidence, so it's deliberately out of scope here to keep this PR a small, fully unit-tested state fix. Happy to follow up with a dedicated layout PR.
  • Live preview ($previewTarget) behavior is unchanged — it was already correctly per-session.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

@maxmilian
maxmilian force-pushed the fix/desktop-preview-tab-session-scope branch from 111797c to c4759ce Compare June 5, 2026 09:46
@maxmilian
maxmilian marked this pull request as ready for review June 5, 2026 09:51
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 5, 2026
@maxmilian
maxmilian force-pushed the fix/desktop-preview-tab-session-scope branch from c4759ce to 5d55be2 Compare June 24, 2026 22:53
@maxmilian
maxmilian force-pushed the fix/desktop-preview-tab-session-scope branch from 5d55be2 to 0535408 Compare July 9, 2026 14:01

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the state half of #39657; current main does retain file tabs globally (apps/desktop/src/store/preview.ts:147-155) while usePreviewRouting only re-scopes the live target (apps/desktop/src/app/session/hooks/use-preview-routing.ts:58-68).

Problems

  • The new routing call syncs from currentPreviewSessionId(), but the hook chooses its visible session through a different precedence chain: selectedStoredSessionId || routedSessionId || activeSessionIdRef.current (use-preview-routing.ts:34-40; preview.ts:292-294). Pass previewSessionId into the sync function and cover divergent route/runtime IDs; the current test changes both together.
  • sessionId is required by the new FilePreviewTab, but the persisted-tab validator still accepts legacy { id, target } rows (preview.ts:197-205). Version/migrate the storage or reject legacy rows explicitly, with a decode regression test.

Suggested changes

  • Scope sync to the hook's resolved session identity.
  • Define and test the localStorage upgrade behavior.

Automated hermes-sweeper review.

@@ -62,6 +63,8 @@ export function usePreviewRouting({
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass previewSessionId into this sync operation. The hook resolves it as selectedStoredSessionId || routedSessionId || activeSessionIdRef.current, whereas the new helper reads only global selected/active atoms; a route/runtime mismatch can retain tabs for the wrong session. Add a regression test with divergent IDs.

@@ -57,6 +57,7 @@ type SessionPreviewRegistry = Record<string, SessionPreviewRecord[]>

export interface FilePreviewTab {
id: `file:${string}`
sessionId: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sessionId mandatory, but the persisted-tab decoder's isFilePreviewTab guard is unchanged and still accepts legacy { id, target } rows. Version/migrate the storage or reject rows without a string sessionId, and test the selected upgrade behavior.

@maxmilian
maxmilian force-pushed the fix/desktop-preview-tab-session-scope branch from 0535408 to c3fa19c Compare July 14, 2026 02:57
@maxmilian

Copy link
Copy Markdown
Contributor Author

Thanks for the review — both problems were real. Addressed on the new head (also rebased onto latest main, so the earlier conflict is gone).

1. Sync now scopes to the routing identity, not a re-derived one. syncFilePreviewTabsForSession takes an explicit sessionId, and the routing effect passes previewSessionId — the exact id it uses for getSessionPreviewRecord(...) — instead of the function re-deriving currentPreviewSessionId() internally. So the filter can no longer diverge from the routed session when the runtime session id lags a mid-switch route. The old routing test moved $activeSessionId and routedSessionId together; the replacement drives them apart — it tags a tab under session-1, advances the runtime $activeSessionId store to session-2, then routes to session-1, and asserts the tab is kept (re-deriving from the store would have wrongly dropped it).

2. Legacy/blank rows rejected on decode. sessionId is now required on FilePreviewTab, so the persisted-tab validator rejects both legacy { id, target } rows and blank session ids rather than restoring unattributable tabs. The decode step is extracted as decodeFilePreviewTabs(raw) and covered by a regression test that feeds it a scoped row + a legacy row + a blank-session row and asserts only the scoped one survives.

Verification: tsc -p . --noEmit clean; vitest run --project ui on both files 14/14 green. I also reverted the production changes and re-ran the suite to confirm the four new/changed tests actually fail on the pre-fix code (they're regressions, not tautologies).

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
…ousResearch#39657)

File preview tabs (`$filePreviewTabs`) were a flat global atom with no
session association, so opening an image/file preview in one conversation
left the tab visible after switching to a conversation that does not
contain that attachment. Unlike `$previewTarget` — which the routing
effect re-scopes per session via the preview registry — file tabs were
never reset on a session change.

Tag each `FilePreviewTab` with the session it was opened under and add
`syncFilePreviewTabsForSession(sessionId)`, which the session-routing
effect calls to drop tabs that don't belong to the conversation being
restored. The sync scopes tabs to the id the effect passes it —
`previewSessionId`, the exact id it routes with — rather than
re-deriving `currentPreviewSessionId()` internally, so the filter can't
diverge from the routed session when the runtime session id lags a
mid-switch route. Syncing with the tab's own session id is a no-op, so
the effect can call it on every registry update without clobbering
same-session tabs.

`sessionId` is now required on `FilePreviewTab`, so the persisted-tab
decoder rejects legacy `{ id, target }` rows (and blank session ids)
that predate scoping instead of restoring unattributable tabs; the
decode step is extracted as `decodeFilePreviewTabs` to be tested
directly.

Tests: store tests cover dropping other-session tabs, the same-session
no-op, argument-scoped filtering, and rejecting legacy/blank rows on
decode; a routing test proves the tab is scoped to the routed session
even when the runtime session store diverges. All fail without the
corresponding fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@maxmilian
maxmilian force-pushed the fix/desktop-preview-tab-session-scope branch from c3fa19c to 30e71d3 Compare July 26, 2026 15:20
@maxmilian

Copy link
Copy Markdown
Contributor Author

Closing this — the premise it was built on no longer holds on main.

#72963 ("Unify the preview rail onto one tab list", merged 2026-07-27) rewrote the rail. $filePreviewTabs, the flat global atom this PR tagged with a sessionId, no longer exists, and the commit is explicit that "the session registry and its reconciler are gone." More to the point, apps/desktop/src/store/preview.ts now documents the behavior this PR set out to change as the intended one:

Tabs are global and outlive the session that created them, like tabs anywhere else — they close when you close them.

So this isn't upstream drift to rebase past. This PR argued that a file tab surviving a conversation switch was a bug; the rail's design has since answered that question the other way, deliberately. Rebasing it would mean re-litigating a design decision through a merge conflict, which isn't what a bugfix PR is for.

Worth saying that #72963 also fixed something real in this area that I had read as a separate concern — the set-then-immediately-cleared pane flash caused by the tab list and the session-keyed registry disagreeing about which session id was current. Collapsing the two lists was a better cut than scoping one of them harder, which is what this PR did.

The half of #39657 that remains genuinely open is the one I scoped out of this PR originally: the image preview tab / title bar overlapping the top-right window controls. I've left a note on the issue so it doesn't read as partly fixed.

@maxmilian maxmilian closed this Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

3 participants