diff --git a/apps/desktop/src/app/command-palette/index.tsx b/apps/desktop/src/app/command-palette/index.tsx index 59f8ba0cbae3c..d53b3199dc11e 100644 --- a/apps/desktop/src/app/command-palette/index.tsx +++ b/apps/desktop/src/app/command-palette/index.tsx @@ -451,11 +451,13 @@ export function CommandPalette() { const go = useCallback((path: string) => () => navigateToWorkspacePage(navigate, path), [navigate]) - // Sessions: plain select = open in-place (focus existing tile/main, else main); - // ⌘/⌃-select / ⌘-Enter = new tab; ⇧⌘ = own window. Same door as the sidebar. + // Sessions: plain select = open beside what's already loaded (focus existing + // tile/main, else a new tab — main only when it's a blank draft); + // ⌘/⌃-select / ⌘-Enter = force a new tab; ⇧⌘ = own window. Same door as the + // sidebar, minus the sidebar's licence to spend main. const goSession = useCallback( (sessionId: string) => (event?: { ctrlKey?: boolean; metaKey?: boolean; shiftKey?: boolean }) => { - openSession(sessionId, navigate, openSessionIntentFromModifiers(event)) + openSession(sessionId, navigate, openSessionIntentFromModifiers(event, 'stack')) }, [navigate] ) diff --git a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts index 0c61eb1f795cc..d64a3a6fe79d9 100644 --- a/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts +++ b/apps/desktop/src/app/contrib/hooks/use-desktop-integrations.ts @@ -128,12 +128,14 @@ export function useDesktopIntegrations({ }, [resumeExhaustedSessionId]) // Native-notification click -> jump to the session WHERE IT ALREADY IS (open - // tile / main) instead of forcing main. Runtime id is translated to the - // stored id the chat route is keyed by; action buttons resolve in place. + // tile / main), else beside what's loaded rather than over it — the click + // came from outside the app and shouldn't cost the user the chat they left + // on screen. Runtime id is translated to the stored id the chat route is + // keyed by; action buttons resolve in place. useEffect(() => { const unsubscribe = window.hermesDesktop?.onFocusSession?.(sessionId => { if (sessionId) { - openSession(storedSessionIdForNotification(sessionId, runtimeIdByStoredSessionId.current), navigate) + openSession(storedSessionIdForNotification(sessionId, runtimeIdByStoredSessionId.current), navigate, 'stack') } }) diff --git a/apps/desktop/src/app/contrib/wiring.tsx b/apps/desktop/src/app/contrib/wiring.tsx index 190e4b4acb5a0..559ccc90e0b11 100644 --- a/apps/desktop/src/app/contrib/wiring.tsx +++ b/apps/desktop/src/app/contrib/wiring.tsx @@ -76,7 +76,7 @@ import { useGatewayRequest } from '../gateway/hooks/use-gateway-request' import { useKeybinds } from '../hooks/use-keybinds' import { ModelPickerOverlay } from '../model-picker-overlay' import { ModelVisibilityOverlay } from '../model-visibility-overlay' -import { openSession } from '../open-session' +import { mainChatOccupied, openSession } from '../open-session' import { PetGenerateOverlay } from '../pet-generate/pet-generate-overlay' import { FileActionDialogs } from '../right-sidebar/file-actions' import { RemoteFolderPicker } from '../right-sidebar/files/remote-picker' @@ -98,7 +98,7 @@ import { useRouteResume } from '../session/hooks/use-route-resume' import { useSessionActions } from '../session/hooks/use-session-actions' import { useSessionListActions } from '../session/hooks/use-session-list-actions' import { useSessionStateCache } from '../session/hooks/use-session-state-cache' -import { newSessionOpensTab, startWorkspaceSession } from '../session/workspace-session-target' +import { startWorkspaceSession } from '../session/workspace-session-target' import { useOverlayRouting } from '../shell/hooks/use-overlay-routing' import { useWindowControlsOverlayWidth } from '../shell/hooks/use-window-controls-overlay-width' import { titlebarControlsPosition } from '../shell/titlebar' @@ -493,12 +493,12 @@ export function ContribWiring({ children }: { children: ReactNode }) { // project so the new lane is visible. // // `openTab` is the sidebar "+" behavior: once a chat is loaded, stack a new - // tab instead of replacing it (see newSessionOpensTab). The composer's + // tab instead of replacing it (see mainChatOccupied). The composer's // "branch off into a new worktree" flow keeps the fresh-draft path — it // prefills the MAIN composer right after, so it has to own that surface. const startSessionInWorkspace = useCallback( (path: null | string, options?: { openTab?: boolean }) => { - if (options?.openTab && newSessionOpensTab(activeSessionIdRef.current, $selectedStoredSessionId.get())) { + if (options?.openTab && mainChatOccupied(activeSessionIdRef.current, $selectedStoredSessionId.get())) { void openNewSessionTile('center', { cwd: path, listed: false }) return diff --git a/apps/desktop/src/app/open-session.test.ts b/apps/desktop/src/app/open-session.test.ts index 5345cd8b24b5a..5f790faee018c 100644 --- a/apps/desktop/src/app/open-session.test.ts +++ b/apps/desktop/src/app/open-session.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const focusOpenSession = vi.fn() const openSessionTile = vi.fn() +const reuseBlankDraftTile = vi.fn() const openSessionInNewWindow = vi.fn() const canOpenSessionWindow = vi.fn(() => true) const workspaceIsPageGet = vi.fn(() => false) @@ -10,7 +11,8 @@ vi.mock('@/store/session-states', () => ({ focusedSessionNeedsRoute: (focused: 'main' | 'tile' | null, workspaceIsPage: boolean) => !focused || (focused === 'main' && workspaceIsPage), focusOpenSession: (...args: unknown[]) => focusOpenSession(...args), - openSessionTile: (...args: unknown[]) => openSessionTile(...args) + openSessionTile: (...args: unknown[]) => openSessionTile(...args), + reuseBlankDraftTile: (...args: unknown[]) => reuseBlankDraftTile(...args) })) vi.mock('@/store/windows', () => ({ @@ -23,7 +25,33 @@ vi.mock('./routes', () => ({ sessionRoute: (id: string) => `/c/${encodeURIComponent(id)}` })) -import { openSession, openSessionIntentFromModifiers } from './open-session' +import { $activeSessionId, $selectedStoredSessionId } from '@/store/session' + +import { mainChatOccupied, openSession, openSessionIntentFromModifiers } from './open-session' + +/** + * The question behind both the sidebar "+" and a palette open: is there a + * conversation on main that must not be discarded? A create affordance stacks a + * tab rather than replacing a chat that may still be mid-turn, and an open from + * nowhere does the same. + */ +describe('mainChatOccupied', () => { + it('is occupied once a conversation is on screen', () => { + expect(mainChatOccupied('runtime-a', 'stored-a')).toBe(true) + }) + + it('is occupied by a live runtime whose stored id has not landed yet', () => { + expect(mainChatOccupied('runtime-a', null)).toBe(true) + }) + + it('is occupied by a selected session still resuming into a runtime', () => { + expect(mainChatOccupied(null, 'stored-a')).toBe(true) + }) + + it('is free when nothing is open', () => { + expect(mainChatOccupied(null, null)).toBe(false) + }) +}) describe('openSessionIntentFromModifiers', () => { it('defaults to in-place', () => { @@ -32,12 +60,22 @@ describe('openSessionIntentFromModifiers', () => { expect(openSessionIntentFromModifiers({})).toBe('in-place') }) + it('returns the caller base for an unmodified select', () => { + expect(openSessionIntentFromModifiers(undefined, 'stack')).toBe('stack') + expect(openSessionIntentFromModifiers({}, 'stack')).toBe('stack') + }) + it('reads ⌘/⌃ as tab and ⇧+mod as window', () => { expect(openSessionIntentFromModifiers({ metaKey: true })).toBe('tab') expect(openSessionIntentFromModifiers({ ctrlKey: true })).toBe('tab') expect(openSessionIntentFromModifiers({ metaKey: true, shiftKey: true })).toBe('window') expect(openSessionIntentFromModifiers({ shiftKey: true })).toBe('in-place') }) + + it('lets modifiers override the base', () => { + expect(openSessionIntentFromModifiers({ metaKey: true }, 'stack')).toBe('tab') + expect(openSessionIntentFromModifiers({ metaKey: true, shiftKey: true }, 'stack')).toBe('window') + }) }) describe('openSession', () => { @@ -50,6 +88,9 @@ describe('openSession', () => { openSessionInNewWindow.mockReset() canOpenSessionWindow.mockReturnValue(true) workspaceIsPageGet.mockReturnValue(false) + reuseBlankDraftTile.mockReset() + $activeSessionId.set(null) + $selectedStoredSessionId.set(null) }) it('in-place focuses an existing tile and does not navigate', () => { @@ -94,6 +135,55 @@ describe('openSession', () => { expect(navigate).not.toHaveBeenCalled() }) + it('stack focuses a session that is already on screen', () => { + $selectedStoredSessionId.set('s0') + focusOpenSession.mockReturnValue('tile') + openSession('s1', navigate, 'stack') + expect(openSessionTile).not.toHaveBeenCalled() + expect(navigate).not.toHaveBeenCalled() + }) + + it('stack opens a tab rather than taking main from a loaded chat', () => { + $selectedStoredSessionId.set('s0') + focusOpenSession.mockReturnValue(null) + openSession('s1', navigate, 'stack') + expect(openSessionTile).toHaveBeenCalledWith('s1', 'center') + expect(navigate).not.toHaveBeenCalled() + }) + + it('stack spends an open blank draft tab before stacking a new one', () => { + $selectedStoredSessionId.set('s0') + focusOpenSession.mockReturnValue(null) + reuseBlankDraftTile.mockReturnValue(true) + openSession('s1', navigate, 'stack') + expect(reuseBlankDraftTile).toHaveBeenCalledWith('s1') + expect(openSessionTile).not.toHaveBeenCalled() + expect(navigate).not.toHaveBeenCalled() + }) + + it('stack prefers the session already on screen over a blank draft tab', () => { + $selectedStoredSessionId.set('s0') + focusOpenSession.mockReturnValue('tile') + reuseBlankDraftTile.mockReturnValue(true) + openSession('s1', navigate, 'stack') + expect(reuseBlankDraftTile).not.toHaveBeenCalled() + expect(openSessionTile).not.toHaveBeenCalled() + }) + + it('stack opens a tab while main is mid-turn on an unsaved session', () => { + $activeSessionId.set('runtime-a') + focusOpenSession.mockReturnValue(null) + openSession('s1', navigate, 'stack') + expect(openSessionTile).toHaveBeenCalledWith('s1', 'center') + }) + + it('stack loads into main when it holds only a blank draft', () => { + focusOpenSession.mockReturnValue(null) + openSession('s1', navigate, 'stack') + expect(navigate).toHaveBeenCalledWith('/c/s1') + expect(openSessionTile).not.toHaveBeenCalled() + }) + it('window pops out when the bridge supports it', () => { openSession('s1', navigate, 'window') expect(openSessionInNewWindow).toHaveBeenCalledWith('s1') diff --git a/apps/desktop/src/app/open-session.ts b/apps/desktop/src/app/open-session.ts index e6582fe4e9e29..94cec331f307e 100644 --- a/apps/desktop/src/app/open-session.ts +++ b/apps/desktop/src/app/open-session.ts @@ -4,28 +4,52 @@ * already a tile (or the main tab) is JUMPED TO instead of yanked into main. * * Intents: - * - `in-place` (click / Enter) — focus existing tile/main if on screen; else - * load into main (same as the left sessions sidebar). + * - `in-place` (sidebar click / Enter) — focus existing tile/main if on + * screen; else load into main (same as the left sessions sidebar). + * - `stack` (⌘K, notifications — anything that opens a chat from outside the + * workspace) — like `tab`, but may spend main or an open blank draft tab + * when either is empty. * - `tab` (⌘/⌃-click / ⌘-Enter / session refs) — focus if already on screen, * else open as a stacked session tab (never steals main from under you). * - `window` (⇧⌘-click) — pop into its own window; falls back to `tab` when * the bridge has no session-window support. */ -import { focusedSessionNeedsRoute, focusOpenSession, openSessionTile } from '@/store/session-states' +import { $activeSessionId, $selectedStoredSessionId } from '@/store/session' +import { + focusedSessionNeedsRoute, + focusOpenSession, + openSessionTile, + reuseBlankDraftTile +} from '@/store/session-states' import { canOpenSessionWindow, openSessionInNewWindow } from '@/store/windows' import { $workspaceIsPage, sessionRoute } from './routes' -export type OpenSessionIntent = 'in-place' | 'tab' | 'window' +export type OpenSessionIntent = 'in-place' | 'stack' | 'tab' | 'window' export type OpenSessionNavigate = (to: string, options?: { replace?: boolean }) => void -/** Read modifiers the way session rows do — meta OR ctrl for tab, +shift for window. */ +/** + * Is the main tab holding a conversation worth preserving? + * + * A loaded chat may still be mid-turn, so replacing it with something else + * throws away work the user can see. A blank draft has nothing to lose, which + * is what lets the sidebar "+" and a `stack` open take the cheaper main path + * instead of stacking a tab nobody asked for. + */ +export function mainChatOccupied(activeSessionId: null | string, selectedStoredSessionId: null | string): boolean { + return Boolean(activeSessionId || selectedStoredSessionId) +} + +/** Read modifiers the way session rows do — meta OR ctrl for tab, +shift for + * window. `base` is what an unmodified select means for the caller: the + * sidebar spends main (`in-place`), a palette-style open doesn't (`stack`). */ export function openSessionIntentFromModifiers( - event?: null | { ctrlKey?: boolean; metaKey?: boolean; shiftKey?: boolean } + event?: null | { ctrlKey?: boolean; metaKey?: boolean; shiftKey?: boolean }, + base: OpenSessionIntent = 'in-place' ): OpenSessionIntent { if (!event) { - return 'in-place' + return base } const mod = Boolean(event.metaKey || event.ctrlKey) @@ -38,7 +62,7 @@ export function openSessionIntentFromModifiers( return 'tab' } - return 'in-place' + return base } /** @@ -67,6 +91,17 @@ export function openSession( resolved = 'tab' } + // A `stack` open arrives from outside the workspace, so unlike a sidebar + // click it can't assume main is spendable: it behaves like `tab`, except main + // IS fair game while it's only a blank draft, and an already-open blank draft + // tab is spent before a new one is stacked. + let spendBlankDraft = false + + if (resolved === 'stack') { + spendBlankDraft = mainChatOccupied($activeSessionId.get(), $selectedStoredSessionId.get()) + resolved = spendBlankDraft ? 'tab' : 'in-place' + } + if (resolved === 'tab') { // Already on screen? Front it. openSessionTile would no-op on main without // focusing, or try to relocate an existing tile — neither is right for a @@ -75,6 +110,13 @@ export function openSession( return } + // Nothing to jump to, but an open tab may still be an empty "New session" — + // that's the tab the user would have typed into, so spend it rather than + // stacking a second blank one beside it. + if (spendBlankDraft && reuseBlankDraftTile(storedSessionId)) { + return + } + openSessionTile(storedSessionId, 'center') return diff --git a/apps/desktop/src/app/session/workspace-session-target.test.ts b/apps/desktop/src/app/session/workspace-session-target.test.ts index 614227585c054..3a83f605a321f 100644 --- a/apps/desktop/src/app/session/workspace-session-target.test.ts +++ b/apps/desktop/src/app/session/workspace-session-target.test.ts @@ -9,7 +9,7 @@ import { setNewChatWorkspaceTarget } from '@/store/session' -import { newSessionOpensTab, startWorkspaceSession } from './workspace-session-target' +import { startWorkspaceSession } from './workspace-session-target' function deferred() { let resolve!: (value: T) => void @@ -21,29 +21,6 @@ function deferred() { return { promise, resolve } } -/** - * The sidebar "+" is a create affordance, not a discard one: with a chat - * already loaded it must stack a tab (⌘T) rather than replace the surface - * (⌘N), which would throw away a conversation that may still be mid-turn. - */ -describe('newSessionOpensTab', () => { - it('opens a tab once a conversation is on screen', () => { - expect(newSessionOpensTab('runtime-a', 'stored-a')).toBe(true) - }) - - it('opens a tab for a live runtime whose stored id has not landed yet', () => { - expect(newSessionOpensTab('runtime-a', null)).toBe(true) - }) - - it('opens a tab for a selected session still resuming into a runtime', () => { - expect(newSessionOpensTab(null, 'stored-a')).toBe(true) - }) - - it('replaces the empty surface when nothing is open', () => { - expect(newSessionOpensTab(null, null)).toBe(false) - }) -}) - describe('startWorkspaceSession', () => { afterEach(() => { setCurrentBranch('') diff --git a/apps/desktop/src/app/session/workspace-session-target.ts b/apps/desktop/src/app/session/workspace-session-target.ts index 9a87d95c0095b..da5028502a112 100644 --- a/apps/desktop/src/app/session/workspace-session-target.ts +++ b/apps/desktop/src/app/session/workspace-session-target.ts @@ -17,19 +17,6 @@ interface WorkspaceSessionOptions { startFreshSessionDraft: (options?: { workspaceTarget: string }) => void } -/** - * Should the sidebar "+" open a NEW TAB rather than replace what's on screen? - * - * "+" is a create affordance, never a discard one. Once a conversation is - * loaded, replacing it with a blank draft would throw away a chat that may - * still be mid-turn, so the button stacks a tab beside it (⌘T) instead of - * taking over the surface (⌘N). With nothing open there is no tab worth - * preserving and the cheaper fresh-draft path applies. - */ -export function newSessionOpensTab(activeSessionId: null | string, selectedStoredSessionId: null | string): boolean { - return Boolean(activeSessionId || selectedStoredSessionId) -} - export function startWorkspaceSession({ activeSessionIdRef, followActiveSessionCwd: followCwd = followActiveSessionCwd, diff --git a/apps/desktop/src/store/session-states.test.ts b/apps/desktop/src/store/session-states.test.ts index 18d4142f10417..f0b4d4c4faf97 100644 --- a/apps/desktop/src/store/session-states.test.ts +++ b/apps/desktop/src/store/session-states.test.ts @@ -1,8 +1,14 @@ import { describe, expect, it } from 'vitest' +import type { ClientSessionState } from '@/app/types' import { group, split } from '@/components/pane-shell/tree/model' import type { SessionTile } from '@/store/session-states' -import { focusedSessionNeedsRoute, orderTilesByTree, selectionHomesToWorkspace } from '@/store/session-states' +import { + blankDraftTile, + focusedSessionNeedsRoute, + orderTilesByTree, + selectionHomesToWorkspace +} from '@/store/session-states' const tile = (storedSessionId: string): SessionTile => ({ storedSessionId }) const tilePane = (id: string) => `session-tile:${id}` @@ -64,3 +70,38 @@ describe('focusedSessionNeedsRoute', () => { expect(focusedSessionNeedsRoute('tile', false)).toBe(false) }) }) + +describe('blankDraftTile', () => { + const bound = (storedSessionId: string, runtimeId: string): SessionTile => ({ runtimeId, storedSessionId }) + + const state = (messages: number, busy = false) => + ({ busy, messages: Array.from({ length: messages }, (_, i) => ({ id: `m${i}` })) }) as ClientSessionState + + it('finds the open tab whose session has no messages', () => { + const tiles = [bound('a', 'run-a'), bound('b', 'run-b')] + const states = { 'run-a': state(3), 'run-b': state(0) } + + expect(blankDraftTile(tiles, states)).toEqual(tiles[1]) + }) + + it('picks the most recent blank tab when there are several', () => { + const tiles = [bound('a', 'run-a'), bound('b', 'run-b')] + const states = { 'run-a': state(0), 'run-b': state(0) } + + expect(blankDraftTile(tiles, states)).toEqual(tiles[1]) + }) + + it('leaves a blank-but-busy tab alone — its first turn is already in flight', () => { + expect(blankDraftTile([bound('a', 'run-a')], { 'run-a': state(0, true) })).toBeNull() + }) + + it('treats an unbound or unpublished tile as unknown, not empty', () => { + expect(blankDraftTile([tile('a')], {})).toBeNull() + expect(blankDraftTile([bound('a', 'run-a')], {})).toBeNull() + }) + + it('is null when every open tab holds a conversation', () => { + expect(blankDraftTile([bound('a', 'run-a')], { 'run-a': state(2) })).toBeNull() + expect(blankDraftTile([], {})).toBeNull() + }) +}) diff --git a/apps/desktop/src/store/session-states.ts b/apps/desktop/src/store/session-states.ts index 273b031587325..a44329f10d373 100644 --- a/apps/desktop/src/store/session-states.ts +++ b/apps/desktop/src/store/session-states.ts @@ -636,6 +636,42 @@ export function focusedSessionNeedsRoute(focused: 'main' | 'tile' | null, worksp return !focused || (focused === 'main' && workspaceIsPage) } +/** The open tab that's still an empty "New session" draft, if there is one. + * That tab is the one the user would have typed into, so an open-from-nowhere + * spends it instead of stacking a second blank tab beside it. Most recent + * wins; a tile whose runtime hasn't bound (or whose state hasn't published) is + * unknown rather than empty, so it's left alone. */ +export function blankDraftTile( + tiles: readonly SessionTile[], + states: Record +): null | SessionTile { + return ( + tiles.findLast(({ runtimeId }) => { + const state = runtimeId ? states[runtimeId] : undefined + + return Boolean(state && !state.busy && state.messages.length === 0) + }) ?? null + ) +} + +/** Hand an open blank draft tab over to `storedSessionId`, keeping its slot. + * False when there's no such tab, so the caller can fall back. The spent draft + * is DISCARDED rather than closed: it never held a conversation, so ⌘⇧T + * resurrecting it would just restore an empty tab. */ +export function reuseBlankDraftTile(storedSessionId: string): boolean { + const tile = blankDraftTile($sessionTiles.get(), $sessionStates.get()) + + if (!tile || tile.storedSessionId === storedSessionId) { + return false + } + + discardSessionTile(tile.storedSessionId) + openSessionTile(storedSessionId, tile.dir, tile.anchor, tile.before) + revealTreePane(`${TILE_PANE_PREFIX}${storedSessionId}`) + + return true +} + // Closed-tab stack for ⌘⇧T reopen (in-memory) — keyed PER PROFILE like the // tiles themselves, so ⌘⇧T after a profile switch never resurrects the other // profile's session. The tile's placement is remembered so it returns in place.