From 0777cc2c7666a96ce9ae40388170fe7a281140bd Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Mon, 27 Apr 2026 01:17:23 -0700 Subject: [PATCH] fix: prevent UI data loss and blank window on startup hydration failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two gaps in the #1158 error path that mirrored the session writer bug the PR fixes: - Guard defaults-hydration in App.tsx catch with a uiHydrated flag so a late-stage startup failure can't overwrite already-loaded ui.json values (sidebar width, sort, filters) with hardcoded defaults. - Wrap reconnectPersistedTerminals() in the catch with a try/catch and force workspaceSessionReady=true via setState on failure, so a crash in the recovery step can't leave the user staring at a blank window. Also adds async-path coverage for the 1-hour backup rotation gate — existing tests only exercised the sync flush path. Co-authored-by: Orca --- 00-review-context.md | 63 --------------------------- src/main/persistence.test.ts | 84 ++++++++++++++++++++++++++++++++++++ src/renderer/src/App.tsx | 69 +++++++++++++++++++++-------- 3 files changed, 134 insertions(+), 82 deletions(-) delete mode 100644 00-review-context.md diff --git a/00-review-context.md b/00-review-context.md deleted file mode 100644 index 477f2810d1b..00000000000 --- a/00-review-context.md +++ /dev/null @@ -1,63 +0,0 @@ -# Review Context - -## Branch Info - -- Base: origin/main (8e44541c4c53cfdf6bb149dc7dc4524eff24405a) -- Current: brennanb2025/fix-session-hydration-data-loss - -## Changed Files Summary - -- M src/main/persistence.test.ts -- M src/main/persistence.ts -- M src/renderer/src/App.tsx -- M src/renderer/src/lib/workspace-session.test.ts -- M src/renderer/src/lib/workspace-session.ts -- M src/renderer/src/store/slices/terminals-hydration.test.ts -- M src/renderer/src/store/slices/terminals.ts - -## Changed Line Ranges (PR Scope) - - - -| File | Changed Lines | -| --------------------------------------------------------- | ------------------------------------------------------ | -| src/main/persistence.test.ts | 5, 585-712 | -| src/main/persistence.ts | 5-14, 57-68, 88-93, 99-154, 269-277, 308-316 | -| src/renderer/src/App.tsx | 31-34, 98, 188-192, 283-296, 316-320, 355, 389, 399-403| -| src/renderer/src/lib/workspace-session.test.ts | 1-2, 143-243 | -| src/renderer/src/lib/workspace-session.ts | 11-27 | -| src/renderer/src/store/slices/terminals-hydration.test.ts | 269-309 | -| src/renderer/src/store/slices/terminals.ts | 98-106, 230-233 | - -## Review Standards Reference - -- Follow /review-code standards -- Focus on: correctness, security, performance, maintainability -- Priority levels: Critical > High > Medium > Low - -## File Categories - -### Electron/Main -- src/main/persistence.ts -- src/main/persistence.test.ts - -### Frontend/UI -- src/renderer/src/App.tsx -- src/renderer/src/lib/workspace-session.ts -- src/renderer/src/lib/workspace-session.test.ts -- src/renderer/src/store/slices/terminals.ts -- src/renderer/src/store/slices/terminals-hydration.test.ts - -## Skipped Issues (Do Not Re-validate) - - - - - -[None yet] - -## Iteration State - -Current iteration: 1 -Last completed phase: Setup -Files fixed this iteration: [] diff --git a/src/main/persistence.test.ts b/src/main/persistence.test.ts index d38c810b48b..9a7e18b2819 100644 --- a/src/main/persistence.test.ts +++ b/src/main/persistence.test.ts @@ -709,5 +709,89 @@ describe('Store', () => { vi.useRealTimers() } }) + + // Why (issue #1158): scheduleSave → writeToDiskAsync → rotateBackupsAsync + // is the hot path during normal use (every mutation debounces through it); + // flush() is only used at shutdown. The 1-hour gate must hold on the async + // path too, or routine edits would burn the 5-slot ring in minutes. + it('does not rotate on the async write path within the 1-hour window', async () => { + vi.useFakeTimers() + try { + // Seed disk so the first async write has something to snapshot. + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: 'seed' })], + worktreeMeta: {}, + settings: {}, + ui: {}, + githubCache: { pr: {}, issue: {} }, + workspaceSession: {} + }) + + const store = await createStore() + + // First async write (debounced) — rotates because .bak.0 doesn't exist + // yet, so shouldRotateBackups returns true on statSync ENOENT. + store.addRepo(makeRepo({ id: 'first-async' })) + vi.advanceTimersByTime(300) + await store.waitForPendingWrite() + + const bak0AfterFirst = readBackup(0) + expect(bak0AfterFirst.repos.map((r) => r.id)).toEqual(['seed']) + + // Advance only 5 minutes, then trigger another async write. The 1-hour + // gate must block rotation: .bak.0 should still reflect the seed state, + // NOT the "first-async" state from the previous write. + vi.setSystemTime(new Date(Date.now() + 5 * 60 * 1000)) + store.addRepo(makeRepo({ id: 'within-hour-async', path: '/within-async' })) + vi.advanceTimersByTime(300) + await store.waitForPendingWrite() + + const bak0AfterSecond = readBackup(0) + expect(bak0AfterSecond.repos.map((r) => r.id)).toEqual(['seed']) + } finally { + vi.useRealTimers() + } + }) + + it('rotates on the async write path after the 1-hour window elapses', async () => { + vi.useFakeTimers() + try { + // Seed disk so the first async write has something to snapshot. + writeDataFile({ + schemaVersion: 1, + repos: [makeRepo({ id: 'seed' })], + worktreeMeta: {}, + settings: {}, + ui: {}, + githubCache: { pr: {}, issue: {} }, + workspaceSession: {} + }) + + const store = await createStore() + + // First async write: snapshots "seed" into .bak.0. + store.addRepo(makeRepo({ id: 'first-async' })) + vi.advanceTimersByTime(300) + await store.waitForPendingWrite() + + expect(readBackup(0).repos.map((r) => r.id)).toEqual(['seed']) + + // Jump past BACKUP_MIN_INTERVAL_MS (1h). The next async write should + // rotate: the previous on-disk state (seed + first-async) lands in + // .bak.0, and the prior .bak.0 ("seed" alone) shifts to .bak.1. + vi.setSystemTime(new Date(Date.now() + 61 * 60 * 1000)) + store.addRepo(makeRepo({ id: 'after-hour-async', path: '/after-async' })) + vi.advanceTimersByTime(300) + await store.waitForPendingWrite() + + const bak0After = readBackup(0) + expect(bak0After.repos.map((r) => r.id).sort()).toEqual(['first-async', 'seed']) + expect(existsSync(backupFile(1))).toBe(true) + expect(readBackup(1).repos.map((r) => r.id)).toEqual(['seed']) + } finally { + vi.useRealTimers() + } + }) }) }) diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 39c80b6776e..187d182b5f4 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -167,6 +167,13 @@ function App(): React.JSX.Element { // without this, the first (unmounted) pass would keep spawning PTYs. const abortController = new AbortController() + // Why (issue #1158): track whether the real persisted UI hydration + // succeeded so the catch path below doesn't overwrite freshly-loaded + // ui.json values with hardcoded defaults. The debounced UI writer is + // gated only on persistedUIReady, so a default-hydration after a real + // one would serialize those defaults back to disk — silently erasing + // sidebar width, sort order, filters, etc. + let uiHydrated = false void (async () => { try { await actions.fetchRepos() @@ -181,6 +188,7 @@ function App(): React.JSX.Element { await actions.fetchSettings() if (!cancelled) { actions.hydratePersistedUI(persistedUI) + uiHydrated = true actions.hydrateWorkspaceSession(session) actions.hydrateTabsSession(session) actions.hydrateEditorSession(session) @@ -298,30 +306,53 @@ function App(): React.JSX.Element { error ) if (!cancelled) { - actions.hydratePersistedUI({ - lastActiveRepoId: null, - lastActiveWorktreeId: null, - sidebarWidth: 280, - rightSidebarWidth: 350, - groupBy: 'none', - sortBy: 'name', - showActiveOnly: false, - filterRepoIds: [], - collapsedGroups: [], - uiZoomLevel: 0, - editorFontZoomLevel: 0, - worktreeCardProperties: [...DEFAULT_WORKTREE_CARD_PROPERTIES], - statusBarItems: [...DEFAULT_STATUS_BAR_ITEMS], - statusBarVisible: true, - dismissedUpdateVersion: null, - lastUpdateCheckAt: null - }) + // Why (issue #1158): only hydrate UI with defaults if the real + // hydration never ran. If fetchRepos/fetchAllWorktrees/session.get/ + // fetchSettings threw before line 183, persistedUIReady is still + // false and the UI cannot mount until we hydrate something. But if + // the real hydrate already ran and a later step threw, overwriting + // with defaults would flow through the debounced UI writer and + // clobber ui.json (sidebar width, sort, filters, etc.). + if (!uiHydrated) { + actions.hydratePersistedUI({ + lastActiveRepoId: null, + lastActiveWorktreeId: null, + sidebarWidth: 280, + rightSidebarWidth: 350, + groupBy: 'none', + sortBy: 'name', + showActiveOnly: false, + filterRepoIds: [], + collapsedGroups: [], + uiZoomLevel: 0, + editorFontZoomLevel: 0, + worktreeCardProperties: [...DEFAULT_WORKTREE_CARD_PROPERTIES], + statusBarItems: [...DEFAULT_STATUS_BAR_ITEMS], + statusBarVisible: true, + dismissedUpdateVersion: null, + lastUpdateCheckAt: null + }) + } // Why: reconnectPersistedTerminals flips workspaceSessionReady so the // UI mounts; auto-tab-creation becomes unblocked. hydrationSucceeded // is intentionally NOT set — the session writer must stay a no-op // until the user gets a clean restart, so we don't overwrite the // on-disk file we failed to load. - await actions.reconnectPersistedTerminals() + try { + await actions.reconnectPersistedTerminals() + } catch (reconnectErr) { + console.error( + '[startup] reconnectPersistedTerminals failed in error path:', + reconnectErr + ) + // Why (issue #1158): this is already the recovery path from a + // failed hydration. If the recovery itself throws, the async IIFE + // rejects as an unhandled promise and workspaceSessionReady never + // flips — leaving the user staring at a blank window. Forcing the + // flag true lets the app shell mount with an empty session, which + // is strictly better than a non-functional UI. + useAppStore.setState({ workspaceSessionReady: true }) + } } } void actions.initGitHubCache()