Repository navigation
fix(session): remember closed and initialized runtime tabs #21630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ff8a162
1621f83
0c7ce39
957064f
2ce07dc
e42fbfc
e0ce8c8
1857814
e1458bf
22bd6bd
bbd56bb
bbb88de
a5d5a4e
c50aff0
bc8e026
afc62a4
03f0d29
0e5b4f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,32 +1,50 @@ | ||
| import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' | ||
|
|
||
| /** | ||
| * Keeps runtime-authored session state alive across a renderer's full write. | ||
| * Keeps session fields the renderer persist snapshot does not author across a full write. | ||
| * | ||
| * A session write replaces the stored object, and the renderer builds its payload from Zustand -- | ||
| * which has no idea the runtime authority sharing this profile also persists the client-hosted | ||
| * pages it owns. Without this, every ordinary desktop session write erases them, and the loss only | ||
| * shows up a restart later when there is nothing left to rehydrate. | ||
| * A session write replaces the stored object. Zustand-built payloads omit runtime-owned | ||
| * client-hosted pages, and they omit-when-empty the write-once default-terminal-tab marker. | ||
| * Without this, those slices vanish on the next desktop write and only show up missing after | ||
| * restart. | ||
| * | ||
| * Callers do not opt in: the Store applies this inside setLocalWorkspaceSession and | ||
| * setHostWorkspaceSession, so the before-unload stage path inherits it too. Guarding the individual | ||
| * writers instead is what let the quit write -- the most common one there is -- erase the field. | ||
| * setHostWorkspaceSession, so the before-unload stage path inherits it too. | ||
| * | ||
| * A runtime clearing its own rows writes an empty map, not `undefined`, so this can never pin a | ||
| * stale set: only an author that never mentioned the field inherits the previous one. | ||
| * A runtime clearing client-hosted pages writes an empty map, not `undefined`. The default-tab | ||
| * marker is write-once: omission means "never stamped", not "never applied". | ||
| */ | ||
| function unionWriteOnceDefaultTerminalTabsApplied( | ||
| next: WorkspaceSessionState, | ||
| prior: WorkspaceSessionState | null | undefined | ||
| ): WorkspaceSessionState { | ||
| const priorMarks = prior?.defaultTerminalTabsAppliedByWorktreeId | ||
| if (!priorMarks || Object.keys(priorMarks).length === 0) { | ||
| return next | ||
| } | ||
| const nextMarks = next.defaultTerminalTabsAppliedByWorktreeId | ||
| const merged = { ...priorMarks, ...nextMarks } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Technical details# Write-once union resurrects explicitly removed markers
## Affected sites
- `src/main/persistence/runtime-authored-workspace-session-fields.ts:26` — unconditional union over prior marks.
- `src/renderer/src/store/slices/worktrees/teardown/worktree-purge-state.ts:243-245` — bulk purge drops the key (callers `fetch-all-worktrees.ts`, `fetched-worktree-merge.ts`, `worktree-event-runtime.ts`).
- `src/renderer/src/lib/workspace-session-patch.ts:189-195` — sends the whole reduced map when non-empty.
- `src/main/persistence/loading-store/session-snapshot-operations.ts:65-75` — a patch containing `tabsByWorktree` routes through `setLocalWorkspaceSession`/`setHostWorkspaceSession`, where `preserveRuntimeAuthoredWorkspaceSessionFields` runs.
- `src/main/persistence/restoring-sessions/session-worktree-ownership.ts:51` — field is `owner-keyed`, so the stale key counts as a live owner.
## Required outcome
- An explicit key removal by a writer must survive the union, while a writer that omits the field entirely still inherits prior marks.
## Suggested approach (optional)
- Only union when `next.defaultTerminalTabsAppliedByWorktreeId === undefined` (the clientHostedBrowserPages precedent), or have the purge path also clear the key through the owner-keyed prune that the explicit single-worktree removal already uses.
## Open questions for the human
- Is the permanent-write-once behaviour intended even across worktree removal, given the marker's `owner-keyed` role in metadata pruning? |
||
| if (nextMarks && Object.keys(merged).length === Object.keys(nextMarks).length) { | ||
| return next | ||
| } | ||
| return { ...next, defaultTerminalTabsAppliedByWorktreeId: merged } | ||
| } | ||
|
|
||
| export function preserveRuntimeAuthoredWorkspaceSessionFields( | ||
| next: WorkspaceSessionState, | ||
| prior: WorkspaceSessionState | null | undefined | ||
| ): WorkspaceSessionState { | ||
| let result = next | ||
| if ( | ||
| next.clientHostedBrowserPagesByWorktree !== undefined || | ||
| prior?.clientHostedBrowserPagesByWorktree === undefined | ||
| next.clientHostedBrowserPagesByWorktree === undefined && | ||
| prior?.clientHostedBrowserPagesByWorktree !== undefined | ||
| ) { | ||
| return next | ||
| } | ||
| return { | ||
| ...next, | ||
| clientHostedBrowserPagesByWorktree: prior.clientHostedBrowserPagesByWorktree | ||
| result = { | ||
| ...result, | ||
| clientHostedBrowserPagesByWorktree: prior.clientHostedBrowserPagesByWorktree | ||
| } | ||
| } | ||
| // Why union: persist snapshots omit this write-once map (empty Zustand slice, omit-when-empty | ||
| // payload). Treating omission as "never applied" re-spawns default terminals on every attach. | ||
| return unionWriteOnceDefaultTerminalTabsApplied(result, prior) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| /** | ||
| * Re-keying a workspace's session rows must not invent state the user never created. | ||
| * | ||
| * `tabsByWorktree` carries three states, not two: an absent row is "never initialized" and an | ||
| * explicit empty row is the closed-last-terminal tombstone that `shouldAutoCreateInitialTerminal` | ||
| * honours. Writing `[]` for a source that had no row turns the first into the second, and the | ||
| * workspace then never gets its initial terminal. | ||
| */ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { getDefaultWorkspaceSession } from '../../shared/constants' | ||
| import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' | ||
| import { canonicalizeTerminalSessionWorktreeId } from './workspace-session-worktree-id' | ||
|
|
||
| const SOURCE = 'repo-1::/old/path' | ||
| const TARGET = 'repo-1::/new/path' | ||
|
|
||
| function tab(id: string, worktreeId: string) { | ||
| return { | ||
| id, | ||
| ptyId: `pty-${id}`, | ||
| worktreeId, | ||
| title: id, | ||
| customTitle: null, | ||
| color: null, | ||
| sortOrder: 0, | ||
| createdAt: 1 | ||
| } | ||
| } | ||
|
|
||
| function session(overrides: Partial<WorkspaceSessionState> = {}): WorkspaceSessionState { | ||
| return { ...getDefaultWorkspaceSession(), ...overrides } | ||
| } | ||
|
|
||
| describe('canonicalizeTerminalSessionWorktreeId', () => { | ||
| it('does not fabricate a tombstone for a workspace that had no tabs row', () => { | ||
| const next = session({ tabsByWorktree: {} }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(Object.hasOwn(next.tabsByWorktree, TARGET)).toBe(false) | ||
| }) | ||
|
|
||
| it('carries a real closed-last-terminal tombstone across the re-key', () => { | ||
| // The other direction: an explicit empty row is user intent and must survive, so the guard has | ||
| // to be about the source row's presence and not about the tabs being empty. | ||
| const next = session({ tabsByWorktree: { [SOURCE]: [] } }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(Object.hasOwn(next.tabsByWorktree, TARGET)).toBe(true) | ||
| expect(next.tabsByWorktree[TARGET]).toEqual([]) | ||
| expect(Object.hasOwn(next.tabsByWorktree, SOURCE)).toBe(false) | ||
| }) | ||
|
|
||
| it('re-keys the tabs and their own worktree ids', () => { | ||
| const next = session({ tabsByWorktree: { [SOURCE]: [tab('tab-1', SOURCE)] } }) | ||
|
|
||
| canonicalizeTerminalSessionWorktreeId(next, SOURCE, TARGET) | ||
|
|
||
| expect(next.tabsByWorktree[TARGET]?.map((entry) => entry.worktreeId)).toEqual([TARGET]) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,18 @@ export function canonicalizeTerminalSessionWorktreeId( | |
| if (sourceWorktreeId === targetWorktreeId) { | ||
| return | ||
| } | ||
| const tabs = session.tabsByWorktree[sourceWorktreeId] ?? [] | ||
| delete session.tabsByWorktree[sourceWorktreeId] | ||
| session.tabsByWorktree[targetWorktreeId] = tabs.map((tab) => ({ | ||
| ...tab, | ||
| worktreeId: targetWorktreeId | ||
| })) | ||
| // Why presence and not `?? []`: an absent row means never initialized, an explicit empty one is | ||
| // the closed-last-terminal tombstone. Writing `[]` for a source that had no row invents that | ||
| // tombstone, and the workspace then never gets its initial terminal. Same guard the keyed maps | ||
| // below already use. | ||
| if (Object.hasOwn(session.tabsByWorktree, sourceWorktreeId)) { | ||
| const tabs = session.tabsByWorktree[sourceWorktreeId] ?? [] | ||
| delete session.tabsByWorktree[sourceWorktreeId] | ||
| session.tabsByWorktree[targetWorktreeId] = tabs.map((tab) => ({ | ||
| ...tab, | ||
| worktreeId: targetWorktreeId | ||
| })) | ||
| } | ||
|
Comment on lines
+15
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '1,140p' src/main/runtime/workspace-session-worktree-id.ts
rg -n 'canonicalizeTerminalSessionWorktreeId|defaultTerminalTabsAppliedByWorktreeId' src/main/runtime src/main/persistence src/renderer/src | head -200Repository: stablyai/orca Length of output: 15008 🏁 Script executed: set -eu
printf '%s\n' '--- runtime caller ---'
cat -n src/main/runtime/runtime-terminal-orphan-session-adoption.ts
printf '%s\n' '--- canonicalization tests ---'
cat -n src/main/runtime/workspace-session-worktree-id.test.ts
printf '%s\n' '--- identity migration ---'
sed -n '150,245p' src/main/persistence/tracking-repos/worktree-identity-migration.ts
printf '%s\n' '--- identity migration coverage tests ---'
sed -n '120,210p' src/main/persistence/tracking-repos/worktree-identity-migration-field-coverage.test.ts
printf '%s\n' '--- migration callers ---'
rg -n -C 5 'canonicalizeTerminalSessionWorktreeId|worktree-identity-migration|moveSessionKey' src/main src/renderer/src | head -260
printf '%s\n' '--- default terminal application ---'
cat -n src/renderer/src/lib/worktree-default-terminal-tabs.ts
printf '%s\n' '--- marker writes and relevant activation flow ---'
rg -n -C 8 'markDefaultTerminalTabsApplied|applyDefaultTerminalTabs|defaultTerminalTabsAppliedByWorktreeId' src/renderer/src/lib src/renderer/src/store | head -320Repository: stablyai/orca Length of output: 50369 🏁 Script executed: set -eu
printf '%s\n' '--- moveSessionKey definition ---'
sed -n '95,150p' src/main/persistence/tracking-repos/worktree-identity-migration.ts
printf '%s\n' '--- migration test references to default marker and collisions ---'
rg -n -C 10 'defaultTerminalTabsAppliedByWorktreeId|target|collision|existing' src/main/persistence/tracking-repos/*worktree-identity-migration*.test.ts
printf '%s\n' '--- relevant workspace session state defaults/types ---'
rg -n -C 5 'defaultTerminalTabsAppliedByWorktreeId' src/shared src/main/runtime src/main/persistence/tracking-repos/worktree-identity-migration.tsRepository: stablyai/orca Length of output: 19933 Re-key the default-terminal marker with the terminal row. This runtime migration moves If the source marker is |
||
|
|
||
| const groups = session.tabGroups?.[sourceWorktreeId] | ||
| if (groups) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { getDefaultWorkspaceSession } from '../../../shared/constants' | ||
| import { parseWorkspaceSession } from '../../../shared/workspace-session-schema' | ||
| import { useAppStore, type AppState } from '@/store' | ||
| import { applyDefaultTerminalTabs } from './worktree-default-terminal-tabs' | ||
| import { | ||
| createSessionWriteSubscriber, | ||
| type WorkspaceSessionWrite | ||
| } from './session-write-subscriber' | ||
| import { createTestStore, makeWorktree, seedStore } from '@/store/slices/store-test-helpers' | ||
|
|
||
| let initialState: AppState | ||
|
|
||
| const WORKTREE_ID = 'repo1::/wt-1' | ||
| const DEFAULT_TABS = { | ||
| runCommands: false, | ||
| tabs: [{ title: 'Claude' }] | ||
| } | ||
|
|
||
| function preserveRuntimeAuthoredWorkspaceSessionFields<T extends Record<string, unknown>>( | ||
| next: T, | ||
| prior: T | ||
| ): T { | ||
| const marks = prior.defaultTerminalTabsAppliedByWorktreeId | ||
| if (!marks || typeof marks !== 'object') { | ||
| return next | ||
| } | ||
| return { | ||
| ...next, | ||
| defaultTerminalTabsAppliedByWorktreeId: { | ||
| ...marks, | ||
| ...(next.defaultTerminalTabsAppliedByWorktreeId as Record<string, boolean> | undefined) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| describe('defaultTerminalTabsAppliedByWorktreeId persist round-trip', () => { | ||
| beforeEach(() => { | ||
| initialState = useAppStore.getState() | ||
| vi.useFakeTimers() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers() | ||
| useAppStore.setState(initialState, true) | ||
| }) | ||
|
|
||
| it('keeps the marker through mark, persist patch, omitted snapshot, and hydration', () => { | ||
| const persist = vi.fn<(payload: WorkspaceSessionWrite) => void>() | ||
| const cleanup = createSessionWriteSubscriber({ store: useAppStore, persist }) | ||
|
|
||
| useAppStore.setState({ workspaceSessionReady: true, hydrationSucceeded: true }) | ||
| vi.advanceTimersByTime(200) | ||
| persist.mockClear() | ||
|
|
||
| useAppStore.getState().markDefaultTerminalTabsApplied(WORKTREE_ID) | ||
| vi.advanceTimersByTime(200) | ||
|
|
||
| expect(persist.mock.calls[0][0].patch.defaultTerminalTabsAppliedByWorktreeId).toEqual({ | ||
| [WORKTREE_ID]: true | ||
| }) | ||
|
|
||
| const patched = { | ||
| ...getDefaultWorkspaceSession(), | ||
| ...persist.mock.calls[0][0].patch | ||
| } | ||
| // Persist snapshots omit this write-once slice (empty payload / renderer never mentioned it). | ||
| const omittedSnapshot = getDefaultWorkspaceSession() | ||
| const preserved = preserveRuntimeAuthoredWorkspaceSessionFields(omittedSnapshot, patched) | ||
| const parsed = parseWorkspaceSession(JSON.parse(JSON.stringify(preserved))) | ||
| expect(parsed.ok).toBe(true) | ||
| if (!parsed.ok) { | ||
| return | ||
| } | ||
|
|
||
| const store = createTestStore() | ||
| seedStore(store, { | ||
| worktreesByRepo: { | ||
| repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1', path: '/wt-1' })] | ||
| } | ||
| }) | ||
| store.getState().hydrateWorkspaceSession(parsed.value) | ||
|
|
||
| expect(store.getState().defaultTerminalTabsAppliedByWorktreeId[WORKTREE_ID]).toBe(true) | ||
| expect( | ||
| applyDefaultTerminalTabs( | ||
| store.getState(), | ||
| WORKTREE_ID, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| DEFAULT_TABS, | ||
| undefined, | ||
| undefined | ||
| ) | ||
| ).toBeNull() | ||
| expect(store.getState().tabsByWorktree[WORKTREE_ID]).toBeUndefined() | ||
| cleanup() | ||
| }) | ||
|
|
||
| it('does not drop in-memory markers when hydration omits the map', () => { | ||
| const store = createTestStore() | ||
| seedStore(store, { | ||
| worktreesByRepo: { | ||
| repo1: [makeWorktree({ id: WORKTREE_ID, repoId: 'repo1', path: '/wt-1' })] | ||
| } | ||
| }) | ||
| store.getState().markDefaultTerminalTabsApplied(WORKTREE_ID) | ||
|
|
||
| store.getState().hydrateWorkspaceSession({ | ||
| activeRepoId: 'repo1', | ||
| activeWorktreeId: WORKTREE_ID, | ||
| activeTabId: null, | ||
| terminalLayoutsByTabId: {}, | ||
| tabsByWorktree: {} | ||
| }) | ||
|
|
||
| expect(store.getState().defaultTerminalTabsAppliedByWorktreeId[WORKTREE_ID]).toBe(true) | ||
| expect( | ||
| applyDefaultTerminalTabs( | ||
| store.getState(), | ||
| WORKTREE_ID, | ||
| undefined, | ||
| undefined, | ||
| undefined, | ||
| DEFAULT_TABS, | ||
| undefined, | ||
| undefined | ||
| ) | ||
| ).toBeNull() | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: stablyai/orca
Length of output: 14046
🏁 Script executed:
Repository: stablyai/orca
Length of output: 35737
🏁 Script executed:
Repository: stablyai/orca
Length of output: 31497
🏁 Script executed:
Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
Repository: stablyai/orca
Length of output: 50370
Ensure the fast lane does not bypass the marker write.
evaluatePtyBindingFastLanedoes not checkdefaultTerminalTabsAppliedByWorktreeId. A durable binding with matching state can take the fast lane while its marker is absent.persistPtyBindingthen returns beforeapplyPtyBindingwrites the marker, so a later activation can apply the default terminals again.Make a missing marker force the durable write path.