fix: restore split group session state safely (#653)

This commit is contained in:
Brennan Benson 2026-04-14 14:52:44 -07:00 committed by GitHub
parent 741974c59c
commit 8d63c6e4fb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 245 additions and 22 deletions

View file

@ -74,6 +74,7 @@ function App(): React.JSX.Element {
initGitHubCache: s.initGitHubCache,
refreshAllGitHub: s.refreshAllGitHub,
hydrateWorkspaceSession: s.hydrateWorkspaceSession,
hydrateTabsSession: s.hydrateTabsSession,
hydrateEditorSession: s.hydrateEditorSession,
hydrateBrowserSession: s.hydrateBrowserSession,
fetchBrowserSessionProfiles: s.fetchBrowserSessionProfiles,
@ -151,6 +152,7 @@ function App(): React.JSX.Element {
if (!cancelled) {
actions.hydratePersistedUI(persistedUI)
actions.hydrateWorkspaceSession(session)
actions.hydrateTabsSession(session)
actions.hydrateEditorSession(session)
actions.hydrateBrowserSession(session)
await actions.fetchBrowserSessionProfiles()

View file

@ -100,4 +100,16 @@ describe('buildWorkspaceSessionPayload', () => {
})
expect(payload.browserTabsByWorktree?.['wt-1'][0].loading).toBe(false)
})
it('drops transient active editor markers that do not point at restored edit files', () => {
const payload = buildWorkspaceSessionPayload(
createSnapshot({
activeFileIdByWorktree: { 'wt-1': '/tmp/demo.diff' },
activeTabTypeByWorktree: { 'wt-1': 'editor', 'wt-2': 'terminal' }
})
)
expect(payload.activeFileIdByWorktree).toEqual({})
expect(payload.activeTabTypeByWorktree).toEqual({ 'wt-2': 'terminal' })
})
})

View file

@ -40,6 +40,7 @@ export function buildEditorSessionData(
> {
const editFiles = openFiles.filter((f) => f.mode === 'edit')
const byWorktree: Record<string, PersistedOpenFile[]> = {}
const editFileIdsByWorktree: Record<string, Set<string>> = {}
for (const f of editFiles) {
const arr = byWorktree[f.worktreeId] ?? (byWorktree[f.worktreeId] = [])
arr.push({
@ -49,11 +50,48 @@ export function buildEditorSessionData(
language: f.language,
isPreview: f.isPreview || undefined
})
const ids =
editFileIdsByWorktree[f.worktreeId] ?? (editFileIdsByWorktree[f.worktreeId] = new Set())
ids.add(f.id)
}
const activeFileEntries: [string, string][] = []
for (const [worktreeId, fileId] of Object.entries(activeFileIdByWorktree)) {
if (!fileId) {
continue
}
if (editFileIdsByWorktree[worktreeId]?.has(fileId)) {
activeFileEntries.push([worktreeId, fileId])
}
}
const persistedActiveFileIdByWorktree = Object.fromEntries(activeFileEntries) as Record<
string,
string
>
const activeTabTypeEntries: [string, WorkspaceVisibleTabType][] = []
for (const [worktreeId, tabType] of Object.entries(activeTabTypeByWorktree)) {
if (tabType !== 'editor') {
activeTabTypeEntries.push([worktreeId, tabType])
continue
}
// Why: restart only restores edit-mode files. Persisting "editor" with a
// transient diff/conflict file ID creates a session payload that cannot be
// satisfied on startup and leaves the UI with no real editor tab to select.
// Only keep the editor marker when it points at a restored file.
if (persistedActiveFileIdByWorktree[worktreeId]) {
activeTabTypeEntries.push([worktreeId, tabType])
}
}
const persistedActiveTabTypeByWorktree = Object.fromEntries(activeTabTypeEntries) as Record<
string,
WorkspaceVisibleTabType
>
return {
openFilesByWorktree: byWorktree,
activeFileIdByWorktree,
activeTabTypeByWorktree
activeFileIdByWorktree: persistedActiveFileIdByWorktree,
activeTabTypeByWorktree: persistedActiveTabTypeByWorktree
}
}

View file

@ -1468,21 +1468,27 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
}
}
if (openFiles.length === 0) {
return {}
}
// Why: use the store's activeWorktreeId (set by hydrateWorkspaceSession)
// rather than the raw session value. hydrateWorkspaceSession may have
// nulled out an invalid worktree ID, and we must respect that decision.
const activeWorktreeId = s.activeWorktreeId
const activeFileId = activeWorktreeId
const fallbackActiveFileId = activeWorktreeId
? (openFiles.find((f) => f.worktreeId === activeWorktreeId)?.id ?? null)
: null
const persistedActiveFileId = activeWorktreeId
? (persistedActiveFileIdByWorktree[activeWorktreeId] ?? null)
: null
// Why: verify the persisted active file still exists in the restored set.
// The file may have been removed due to worktree validation or the
// persisted data may reference a stale path.
const activeFileExists = activeFileId ? openFiles.some((f) => f.id === activeFileId) : false
const activeFileExists = persistedActiveFileId
? openFiles.some((f) => f.id === persistedActiveFileId)
: false
// Why: if the previously active editor surface pointed at a transient
// diff/conflict tab, restart still restores any normal edit tabs for the
// worktree. Promote the first restored edit file so the UI comes back on
// a concrete file tab instead of an unselected editor surface.
const nextActiveFileId = activeFileExists ? persistedActiveFileId : fallbackActiveFileId
const activeTabType: WorkspaceVisibleTabType =
activeWorktreeId && persistedActiveTabTypeByWorktree[activeWorktreeId]
? persistedActiveTabTypeByWorktree[activeWorktreeId]
@ -1490,10 +1496,14 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
// Filter per-worktree maps to only valid worktrees with valid file references
const filteredActiveFileIdByWorktree = Object.fromEntries(
Object.entries(persistedActiveFileIdByWorktree).filter(
([wId, fileId]) =>
validWorktreeIds.has(wId) && fileId && openFiles.some((f) => f.id === fileId)
)
[...validWorktreeIds].flatMap((wId) => {
const persistedFileId = persistedActiveFileIdByWorktree[wId]
if (persistedFileId && openFiles.some((f) => f.id === persistedFileId)) {
return [[wId, persistedFileId]]
}
const fallbackFileId = openFiles.find((f) => f.worktreeId === wId)?.id
return fallbackFileId ? [[wId, fallbackFileId]] : []
})
)
const filteredActiveTabTypeByWorktree = Object.fromEntries(
Object.entries(persistedActiveTabTypeByWorktree).filter(([wId, tabType]) => {
@ -1511,11 +1521,18 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (s
})
)
// Why: restart only restores edit-mode files. If the previous active
// surface for the current worktree was a transient diff/conflict view,
// we must clear the stale "editor" marker here so startup falls back to
// browser or terminal instead of showing an empty editor surface.
const nextActiveTabType =
nextActiveFileId || activeTabType !== 'editor' ? activeTabType : 'terminal'
return {
openFiles,
activeFileId: activeFileExists ? activeFileId : null,
activeFileId: nextActiveFileId,
activeFileIdByWorktree: filteredActiveFileIdByWorktree,
activeTabType: activeFileExists ? activeTabType : 'terminal',
activeTabType: nextActiveTabType,
activeTabTypeByWorktree: filteredActiveTabTypeByWorktree
}
})

View file

@ -964,7 +964,40 @@ describe('hydrateEditorSession', () => {
expect(s.activeTabType).toBe('terminal')
})
it('falls back to terminal if persisted activeFileId is missing from restored files', () => {
it('clears stale editor markers when no edit-mode files restore for the active worktree', () => {
const store = createTestStore()
const wt = 'repo1::/path/wt1'
store.setState({
repos: [
{ id: 'repo1', path: '/repo1', displayName: 'Repo 1', badgeColor: '#000', addedAt: 0 }
],
worktreesByRepo: {
repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt1' })]
},
activeWorktreeId: wt,
activeTabType: 'editor'
})
store.getState().hydrateEditorSession({
activeRepoId: 'repo1',
activeWorktreeId: wt,
activeTabId: null,
tabsByWorktree: {},
terminalLayoutsByTabId: {},
activeFileIdByWorktree: { [wt]: `${wt}::diff::unstaged::src/index.ts` },
activeTabTypeByWorktree: { [wt]: 'editor' }
})
const s = store.getState()
expect(s.openFiles).toHaveLength(0)
expect(s.activeFileId).toBeNull()
expect(s.activeTabType).toBe('terminal')
expect(s.activeFileIdByWorktree[wt]).toBeUndefined()
expect(s.activeTabTypeByWorktree[wt]).toBeUndefined()
})
it('promotes the first restored edit file if persisted activeFileId is missing', () => {
const store = createTestStore()
const wt = 'repo1::/path/wt1'
@ -1001,9 +1034,10 @@ describe('hydrateEditorSession', () => {
const s = store.getState()
expect(s.openFiles).toHaveLength(1)
expect(s.activeFileId).toBeNull()
expect(s.activeTabType).toBe('terminal')
expect(s.activeTabTypeByWorktree[wt]).toBeUndefined()
expect(s.activeFileId).toBe('/path/wt1/src/index.ts')
expect(s.activeTabType).toBe('editor')
expect(s.activeFileIdByWorktree[wt]).toBe('/path/wt1/src/index.ts')
expect(s.activeTabTypeByWorktree[wt]).toBe('editor')
})
it('filters out files for deleted worktrees', () => {

View file

@ -137,6 +137,77 @@ describe('buildHydratedTabState unified format', () => {
expect(group.activeTabId).toBeNull()
expect(group.tabOrder).toEqual(['t1'])
})
it('collapses groups and layout when transient tabs are dropped during hydration', () => {
const session: WorkspaceSessionState = {
...makeBaseSession(),
openFilesByWorktree: {
w1: [
{
filePath: '/editor.ts',
relativePath: 'editor.ts',
worktreeId: 'w1',
language: 'typescript'
}
]
},
unifiedTabs: {
w1: [
{
id: 'diff-1',
entityId: '/diff.ts',
groupId: 'g1',
worktreeId: 'w1',
contentType: 'diff',
label: 'diff.ts',
customLabel: null,
color: null,
sortOrder: 0,
createdAt: 1
},
{
id: 'editor-1',
entityId: '/editor.ts',
groupId: 'g2',
worktreeId: 'w1',
contentType: 'editor',
label: 'editor.ts',
customLabel: null,
color: null,
sortOrder: 1,
createdAt: 2
}
]
},
tabGroups: {
w1: [
{ id: 'g1', worktreeId: 'w1', activeTabId: 'diff-1', tabOrder: ['diff-1'] },
{ id: 'g2', worktreeId: 'w1', activeTabId: 'editor-1', tabOrder: ['editor-1'] }
]
},
tabGroupLayouts: {
w1: {
type: 'split',
direction: 'horizontal',
first: { type: 'leaf', groupId: 'g1' },
second: { type: 'leaf', groupId: 'g2' },
ratio: 0.5
}
},
activeGroupIdByWorktree: { w1: 'g1' }
}
const result = buildHydratedTabState(session, new Set(['w1']))
expect(result.unifiedTabsByWorktree.w1).toEqual([
expect.objectContaining({ id: 'editor-1', groupId: 'g2', contentType: 'editor' })
])
expect(result.groupsByWorktree.w1).toEqual([
{ id: 'g2', worktreeId: 'w1', activeTabId: 'editor-1', tabOrder: ['editor-1'] }
])
expect(result.activeGroupIdByWorktree.w1).toBe('g2')
expect(result.layoutByWorktree.w1).toEqual({ type: 'leaf', groupId: 'g2' })
})
})
describe('buildHydratedTabState legacy format', () => {

View file

@ -17,6 +17,27 @@ type HydratedTabState = {
layoutByWorktree: Record<string, TabGroupLayoutNode>
}
function pruneLayoutForGroups(
root: TabGroupLayoutNode,
validGroupIds: Set<string>
): TabGroupLayoutNode | null {
if (root.type === 'leaf') {
return validGroupIds.has(root.groupId) ? root : null
}
const first = pruneLayoutForGroups(root.first, validGroupIds)
const second = pruneLayoutForGroups(root.second, validGroupIds)
if (first === null) {
return second
}
if (second === null) {
return first
}
return { ...root, first, second }
}
function hydrateUnifiedFormat(
session: WorkspaceSessionState,
validWorktreeIds: Set<string>
@ -69,18 +90,35 @@ function hydrateUnifiedFormat(
activeTabId: g.activeTabId && validTabIds.has(g.activeTabId) ? g.activeTabId : null
}
})
const hydratedGroups = validatedGroups.filter((group, index) => {
const hadTabsBeforeHydration = groups[index]?.tabOrder.length > 0
return !hadTabsBeforeHydration || group.tabOrder.length > 0
})
if (hydratedGroups.length === 0) {
if ((tabsByWorktree[worktreeId] ?? []).length === 0) {
delete tabsByWorktree[worktreeId]
}
continue
}
groupsByWorktree[worktreeId] = validatedGroups
groupsByWorktree[worktreeId] = hydratedGroups
const activeGroupId = selectHydratedActiveGroupId(
validatedGroups,
hydratedGroups,
session.activeGroupIdByWorktree?.[worktreeId]
)
if (activeGroupId) {
activeGroupIdByWorktree[worktreeId] = activeGroupId
}
layoutByWorktree[worktreeId] = session.tabGroupLayouts?.[worktreeId] ?? {
const hydratedGroupIds = new Set(hydratedGroups.map((group) => group.id))
const hydratedLayout = session.tabGroupLayouts?.[worktreeId]
? pruneLayoutForGroups(session.tabGroupLayouts[worktreeId], hydratedGroupIds)
: null
layoutByWorktree[worktreeId] = hydratedLayout ?? {
type: 'leaf',
groupId: validatedGroups[0].id
// Why: if transient-only groups were removed during hydration, the
// persisted split tree can collapse to a single surviving group. The
// fallback leaf keeps restore aligned with the remaining real tabs.
groupId: hydratedGroups[0].id
}
}

View file

@ -38,6 +38,7 @@ function createTestStore() {
unifiedTabsByWorktree: {},
groupsByWorktree: {},
activeGroupIdByWorktree: {},
layoutByWorktree: {},
openFiles: [],
editorDrafts: {},
markdownViewMode: {},
@ -304,6 +305,10 @@ describe('removeWorktree state cleanup', () => {
activeGroupIdByWorktree: {
'repo1::/path/wt1': 'group-1',
'repo1::/path/wt2': 'group-2'
},
layoutByWorktree: {
'repo1::/path/wt1': { type: 'leaf', groupId: 'group-1' },
'repo1::/path/wt2': { type: 'leaf', groupId: 'group-2' }
}
} as unknown as Partial<AppState>)
@ -321,6 +326,9 @@ describe('removeWorktree state cleanup', () => {
expect(store.getState().activeGroupIdByWorktree).toEqual({
'repo1::/path/wt2': 'group-2'
})
expect(store.getState().layoutByWorktree).toEqual({
'repo1::/path/wt2': { type: 'leaf', groupId: 'group-2' }
})
})
it('cleans up git caches for the removed worktree', async () => {

View file

@ -164,6 +164,8 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
delete nextGroupsByWorktree[worktreeId]
const nextActiveGroupIdByWorktree = { ...s.activeGroupIdByWorktree }
delete nextActiveGroupIdByWorktree[worktreeId]
const nextLayoutByWorktree = { ...s.layoutByWorktree }
delete nextLayoutByWorktree[worktreeId]
// Why: git status / compare caches are keyed by worktree and stop being
// refreshed once the worktree is deleted. Remove them here so deleted
// worktrees cannot retain stale conflict badges, branch diffs, or compare
@ -231,6 +233,7 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
unifiedTabsByWorktree: nextUnifiedTabsByWorktree,
groupsByWorktree: nextGroupsByWorktree,
activeGroupIdByWorktree: nextActiveGroupIdByWorktree,
layoutByWorktree: nextLayoutByWorktree,
editorDrafts: nextEditorDrafts,
markdownViewMode: nextMarkdownViewMode,
expandedDirs: nextExpandedDirs,