diff --git a/src/renderer/src/components/settings/AccountsPane.tsx b/src/renderer/src/components/settings/AccountsPane.tsx index 4188db5c831..7578112895a 100644 --- a/src/renderer/src/components/settings/AccountsPane.tsx +++ b/src/renderer/src/components/settings/AccountsPane.tsx @@ -57,7 +57,10 @@ import { ProviderHostScopeControl } from './ProviderHostScopeControl' import { SearchableSetting } from './SearchableSetting' import { SettingsRow, SettingsSegmentedControl } from './SettingsFormControls' import { matchesSettingsSearch } from './settings-search' -import { markLiveCodexSessionsForRestart } from '@/lib/codex-session-restart' +import { + markLiveCodexSessionsForRestart, + resolveCodexRestartPromptAccountLabel +} from '@/lib/codex-session-restart' import { Dialog, DialogContent, @@ -173,16 +176,6 @@ function getHostRuntimeLabel(): string { : translate('auto.components.settings.AccountsPane.9baf45d071', 'This device') } -function getCodexAccountLabel( - state: CodexRateLimitAccountsState, - accountId: string | null | undefined -): string { - if (accountId == null) { - return 'System default' - } - return state.accounts.find((account) => account.id === accountId)?.email ?? 'Codex account' -} - // Why: the system-default row has no stored identity, so surface the real // ~/.codex login live — the OAuth email when signed in, a clear custom-provider // note for env-key logins, and the generic fallback when signed out. @@ -761,8 +754,18 @@ export function AccountsPane({ // Falling back to the row is the pre-existing behaviour, not a new risk. const addedAccount = newAccounts.length === 1 ? newAccounts[0] : undefined void markLiveCodexSessionsForRestart({ - previousAccountLabel: getCodexAccountLabel(codexAccounts, previousActiveAccountId), - nextAccountLabel: getCodexAccountLabel(next, nextActiveAccountId), + previousAccountLabel: resolveCodexRestartPromptAccountLabel( + codexAccounts.accounts, + previousActiveAccountId + ), + nextAccountLabel: resolveCodexRestartPromptAccountLabel( + next.accounts, + nextActiveAccountId + ), + // Why: two accounts can share an email, so the labels alone cannot + // tell the store whether this switch lands back on the launch account. + previousAccountId: previousActiveAccountId ?? null, + nextAccountId: nextActiveAccountId ?? null, // Why: the mutation wrote this row's slot only, so panes on any other // lane still launch under the account they already had. target: addedAccount ? getProviderAccountRuntime(addedAccount) : actionRuntime, diff --git a/src/renderer/src/components/status-bar/StatusBar.tsx b/src/renderer/src/components/status-bar/StatusBar.tsx index 2fb006b34d9..2c321685ff6 100644 --- a/src/renderer/src/components/status-bar/StatusBar.tsx +++ b/src/renderer/src/components/status-bar/StatusBar.tsx @@ -65,7 +65,10 @@ import { UsageRosterPanel, getTightestUsageSection } from './UsageRosterPanel' import { getUsageProviderAccountsSectionId } from './usage-provider-settings-target' import { formatRateLimitWindowChipLabel } from '@/lib/window-label-formatter' import { useResetCountdownClock } from '@/hooks/useResetCountdownClock' -import { markLiveCodexSessionsForRestart } from '@/lib/codex-session-restart' +import { + markLiveCodexSessionsForRestart, + resolveCodexRestartPromptAccountLabel +} from '@/lib/codex-session-restart' import { UpdateStatusSegment } from './UpdateStatusSegment' import { SkillUpdateStatusSegment } from './SkillUpdateStatusSegment' import { RemoteServerUpdateStatusSegment } from './RemoteServerUpdateStatusSegment' @@ -168,16 +171,6 @@ function getHostRuntimeLabel(): string { return navigator.userAgent.includes('Windows') ? 'Windows' : 'This device' } -function getCodexAccountLabel( - state: CodexRateLimitAccountsState, - accountId: string | null | undefined -): string { - if (accountId == null) { - return 'System default' - } - return state.accounts.find((account) => account.id === accountId)?.email ?? 'Codex account' -} - function getCodexAccountDisplayLabel(account: CodexStatusAccount): string { return account.workspaceLabel ? `${account.email} (${account.workspaceLabel})` : account.email } @@ -1466,8 +1459,18 @@ export function CodexSwitcherMenu({ const nextActiveAccountId = getCodexStatusActiveId(next, target) if (previousActiveAccountId !== nextActiveAccountId) { await markLiveCodexSessionsForRestart({ - previousAccountLabel: getCodexAccountLabel(accountState, previousActiveAccountId), - nextAccountLabel: getCodexAccountLabel(next, nextActiveAccountId), + previousAccountLabel: resolveCodexRestartPromptAccountLabel( + accountState.accounts, + previousActiveAccountId + ), + nextAccountLabel: resolveCodexRestartPromptAccountLabel( + next.accounts, + nextActiveAccountId + ), + // Why: two accounts can share an email, so the labels alone cannot + // tell the store whether this switch lands back on the launch account. + previousAccountId: previousActiveAccountId ?? null, + nextAccountId: nextActiveAccountId ?? null, // Why: the mutation wrote this row's slot only, so panes on any other // lane still launch under the account they already had. target, diff --git a/src/renderer/src/lib/codex-session-restart.test.ts b/src/renderer/src/lib/codex-session-restart.test.ts index 3c9c2c816b0..7263edcd9ab 100644 --- a/src/renderer/src/lib/codex-session-restart.test.ts +++ b/src/renderer/src/lib/codex-session-restart.test.ts @@ -795,7 +795,9 @@ describe('markRestoredStaleCodexSessionsForRestart', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) diff --git a/src/renderer/src/lib/codex-session-restart.ts b/src/renderer/src/lib/codex-session-restart.ts index cf5a6f5ac2c..791fdc6ba4a 100644 --- a/src/renderer/src/lib/codex-session-restart.ts +++ b/src/renderer/src/lib/codex-session-restart.ts @@ -140,6 +140,11 @@ async function scanCodexPanes( export async function markLiveCodexSessionsForRestart(args: { previousAccountLabel: string nextAccountLabel: string + /** Ids behind the labels. Two accounts can share a label, so without these the + * store cannot tell a switch back to the launch account from a switch to a + * different account that merely reads the same. */ + previousAccountId?: string | null + nextAccountId?: string | null target?: CodexAccountSelectionTarget | null /** Set when the change cleared the selection rather than pointing it somewhere. */ clearsEveryWslDistro?: boolean @@ -162,7 +167,11 @@ export async function markLiveCodexSessionsForRestart(args: { liveCodexSessionPtyIds.map((ptyId) => ({ ptyId, previousAccountLabel: args.previousAccountLabel, - nextAccountLabel: args.nextAccountLabel + nextAccountLabel: args.nextAccountLabel, + ...(args.previousAccountId === undefined + ? {} + : { previousAccountId: args.previousAccountId }), + ...(args.nextAccountId === undefined ? {} : { nextAccountId: args.nextAccountId }) })) ) } @@ -203,28 +212,53 @@ export async function markRestoredStaleCodexSessionsForRestart(args?: { } const resolveAccountLabel = await createCodexAccountLabelResolver() - useAppStore.getState().markCodexRestartNotices( + const noticedPtyIds = useAppStore.getState().markCodexRestartNotices( stalePanes.map((pane) => ({ ptyId: pane.ptyId, previousAccountLabel: resolveAccountLabel(pane.launchAccountId), - nextAccountLabel: resolveAccountLabel(pane.activeAccountId) + nextAccountLabel: resolveAccountLabel(pane.activeAccountId), + // Why the ids: main decided staleness by id, and the labels can collide. + // Passing only labels hands the store a question it cannot answer. + previousAccountId: pane.launchAccountId, + nextAccountId: pane.activeAccountId })) ) - const notifiedPtyIds = new Set(stalePanes.map((pane) => pane.ptyId)) + // Why not every stale pane: the bind sweep suppresses a "notified" pane for the + // rest of the session, so a pane whose notice the store dropped must not claim + // one — that trades a missing prompt for a permanently missing prompt. + const notifiedPtyIds = new Set(noticedPtyIds) return scans.map((scan) => (notifiedPtyIds.has(scan.ptyId) ? { ...scan, notified: true } : scan)) } +/** + * Names an account for the restart prompt. + * + * Why the collision check: one OpenAI login added under two ChatGPT workspaces + * gives both accounts the same email, and "switch from x@y to x@y" names + * neither. The workspace is appended only when it is what tells them apart. + */ +export function resolveCodexRestartPromptAccountLabel( + accounts: readonly { id: string; email: string; workspaceLabel?: string | null }[], + accountId: string | null | undefined +): string { + if (accountId == null) { + return translate('auto.lib.codex.session.restart.4bd4a3a9c7', 'System default') + } + const account = accounts.find((entry) => entry.id === accountId) + if (!account) { + return translate('auto.lib.codex.session.restart.9f0b1c2d3e', 'Codex account') + } + const sharesEmail = accounts.some( + (entry) => entry.id !== account.id && entry.email === account.email + ) + return sharesEmail && account.workspaceLabel + ? `${account.email} (${account.workspaceLabel})` + : account.email +} + async function createCodexAccountLabelResolver(): Promise<(accountId: string | null) => string> { // Why: a failed roster read still yields usable prompts — the account ids are // already known, only their friendly emails are missing. const accounts = await window.api.codexAccounts.list().catch(() => null) - return (accountId) => { - if (accountId == null) { - return translate('auto.lib.codex.session.restart.4bd4a3a9c7', 'System default') - } - return ( - accounts?.accounts.find((account) => account.id === accountId)?.email ?? - translate('auto.lib.codex.session.restart.9f0b1c2d3e', 'Codex account') - ) - } + return (accountId) => resolveCodexRestartPromptAccountLabel(accounts?.accounts ?? [], accountId) } diff --git a/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts b/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts new file mode 100644 index 00000000000..aeeb1c674c9 --- /dev/null +++ b/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts @@ -0,0 +1,162 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { clearRuntimeCompatibilityCacheForTests } from '@/runtime/runtime-rpc-client' +import { markRestoredStaleCodexSessionsForRestart } from './codex-session-restart' + +// Why one shared email: doAddAccount has no duplicate-email check, so one OpenAI +// login used in two ChatGPT workspaces produces two accounts with equal labels. +const SHARED_EMAIL = 'shared@example.com' +const FALLBACK_LABEL = 'Codex account' + +function seedRoster( + accounts: { id: string; email: string; workspaceLabel?: string | null }[] +): void { + vi.mocked(window.api.codexAccounts.list).mockResolvedValue({ + accounts, + activeAccountId: 'account-b' + } as never) +} + +function noticeFor(ptyId: string): unknown { + return useAppStore.getState().codexRestartNoticeByPtyId[ptyId] +} + +describe('stale Codex panes are decided by account id, not label', () => { + const originalWindow = (globalThis as { window?: typeof window }).window + + beforeEach(() => { + clearRuntimeCompatibilityCacheForTests() + useAppStore.setState({ + settings: null as never, + tabsByWorktree: { + wt1: [ + { + id: 'tab-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + title: 'orca-1', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] }, + pendingCodexPaneRestartIds: {}, + codexRestartNoticeByPtyId: {} + }) + ;(globalThis as { window: typeof window }).window = { + ...originalWindow, + api: { + ...originalWindow?.api, + pty: { + ...originalWindow?.api?.pty, + getForegroundProcess: vi.fn(), + hasChildProcesses: vi.fn().mockResolvedValue(false), + inspectProcess: vi + .fn() + .mockResolvedValue({ foregroundProcess: 'codex', hasChildProcesses: false }), + confirmForegroundProcess: vi.fn().mockResolvedValue(null) + }, + codexAccounts: { + ...originalWindow?.api?.codexAccounts, + list: vi.fn(), + listStalePanes: vi.fn().mockResolvedValue([]) + } + } + } as unknown as typeof window + seedRoster([ + { id: 'account-a', email: SHARED_EMAIL }, + { id: 'account-b', email: SHARED_EMAIL } + ]) + }) + + afterEach(() => { + if (originalWindow) { + ;(globalThis as { window: typeof window }).window = originalWindow + } else { + delete (globalThis as { window?: typeof window }).window + } + }) + + it('keeps the prompt when the two accounts resolve to the same label', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(true) + }) + + it('keeps the prompt when the roster read fails and every label collapses', async () => { + vi.mocked(window.api.codexAccounts.list).mockRejectedValue(new Error('roster unavailable')) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountLabel: FALLBACK_LABEL, + nextAccountLabel: FALLBACK_LABEL, + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(true) + }) + + it('disambiguates shared emails by ChatGPT workspace', async () => { + seedRoster([ + { id: 'account-a', email: SHARED_EMAIL, workspaceLabel: 'Personal' }, + { id: 'account-b', email: SHARED_EMAIL, workspaceLabel: 'Acme' } + ]) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountLabel: `${SHARED_EMAIL} (Personal)`, + nextAccountLabel: `${SHARED_EMAIL} (Acme)` + }) + }) + + it('raises no notice and reports nothing notified for a pane on the selected account', async () => { + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + expect(scans.every((scan) => scan.notified === false)).toBe(true) + }) + + it('does not report notified for a pane whose notice the store dropped', async () => { + // The pane already carries a notice remembering account-b as its launch + // account, so re-marking it against active account-b collapses the notice. + useAppStore.setState({ + codexRestartNoticeByPtyId: { + 'pty-1': { + previousAccountLabel: SHARED_EMAIL, + nextAccountLabel: SHARED_EMAIL, + previousAccountId: 'account-b', + nextAccountId: 'account-a' + } + } + }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toBeUndefined() + // Why this matters: the sweep permanently suppresses every pane it is told + // was notified, so a dropped notice must never claim one. + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts index 251881a57de..390021b6bd1 100644 --- a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts +++ b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts @@ -91,7 +91,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -136,7 +138,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -176,7 +180,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) expect(inspectCallCountFor('pty-1')).toBe(4) }) @@ -230,7 +236,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -266,7 +274,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -288,6 +298,34 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledTimes(1) }) + it('does not suppress a pane whose notice the store dropped', async () => { + // Why: suppression is permanent for the session, so claiming a pane was + // notified when no notice survived silently strands it on the old account. + // The pane already remembers account-b as its launch account, which + // collapses a fresh notice pointing back at account-b. + useAppStore.setState({ + codexRestartNoticeByPtyId: { + 'pty-1': { + previousAccountLabel: ACCOUNT_B, + nextAccountLabel: ACCOUNT_A, + previousAccountId: 'account-b', + nextAccountId: 'account-a' + } + } + }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + expect(inspectCallCountFor('pty-1')).toBe(1) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + + expect(inspectCallCountFor('pty-1')).toBe(2) + }) + it('never marks a plain shell pane, so its input is never blocked', async () => { useAppStore.setState({ tabsByWorktree: { @@ -333,7 +371,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -350,7 +390,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -396,7 +438,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(inspectCallCountFor('remote:env-1@@term-1')).toBe(0) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) }) diff --git a/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts b/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts new file mode 100644 index 00000000000..e7a95ae8db0 --- /dev/null +++ b/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts @@ -0,0 +1,26 @@ +/** One side of a Codex restart notice: the account's id when the caller knows + * it (`null` is the system default), plus the label shown in the prompt. */ +export type CodexRestartNoticeAccount = { + id?: string | null + label: string +} + +/** + * Decides whether two restart-notice accounts are the same account. + * + * Why not the labels: an account label is an email, and nothing stops two + * accounts from sharing one — the same OpenAI login added under two ChatGPT + * workspaces, or a failed roster read collapsing every account to the same + * fallback string. Deciding on labels there erases a prompt for a pane that is + * genuinely running on the account the user switched away from. Labels remain + * the fallback only for callers that carry no ids at all. + */ +export function isSameCodexRestartNoticeAccount( + a: CodexRestartNoticeAccount, + b: CodexRestartNoticeAccount +): boolean { + if (a.id !== undefined && b.id !== undefined) { + return a.id === b.id + } + return a.label === b.label +} diff --git a/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts b/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts index 3d78a0353c1..6e8f0ede98f 100644 --- a/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts +++ b/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts @@ -111,6 +111,22 @@ describe('codex restart notice lifecycle', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId).toBe(before) }) + it('reports which panes are left holding a notice', () => { + expect( + useAppStore + .getState() + .markCodexRestartNotices([{ ptyId: 'pty-1', previousAccountLabel: A, nextAccountLabel: B }]) + ).toEqual(['pty-1']) + + // Why the caller needs this: the startup sweep suppresses a pane for the + // rest of the session once it is told a prompt went up. + expect( + useAppStore + .getState() + .markCodexRestartNotices([{ ptyId: 'pty-1', previousAccountLabel: B, nextAccountLabel: A }]) + ).toEqual([]) + }) + it('still deletes the record when the pane actually restarts', () => { switchAccount('pty-1', A, B) @@ -126,3 +142,63 @@ describe('codex restart notice lifecycle', () => { }) }) }) + +// Why: doAddAccount has no duplicate-email check, so one OpenAI login used in +// two ChatGPT workspaces gives two accounts the same label — as does a failed +// roster read, which collapses every account to 'Codex account'. +describe('codex restart notices with colliding account labels', () => { + const SHARED = 'shared@example.com' + + function switchAccountById(ptyId: string, from: string | null, to: string | null): void { + useAppStore.getState().markCodexRestartNotices([ + { + ptyId, + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: from, + nextAccountId: to + } + ]) + } + + it('raises the prompt for two different accounts that read the same', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + }) + + it('still collapses A -> B -> A when the ids match', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + switchAccountById('pty-1', 'account-b', 'account-a') + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('collapses back to the system default the pane launched under', () => { + switchAccountById('pty-1', null, 'account-b') + switchAccountById('pty-1', 'account-b', null) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('re-asks a dismissed pane when a same-labelled third account is selected', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + switchAccountById('pty-1', 'account-b', 'account-c') + + // Why: carrying the dismissal on the shared label would strand the pane on + // account-a with its prompt answered for an account it never moved to. + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: 'account-a', + nextAccountId: 'account-c' + }) + }) +}) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 2720ce9a520..b24610cbab9 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -35,6 +35,7 @@ import { } from '../../../../shared/stable-pane-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../../../shared/terminal-tab-id' import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' +import { isSameCodexRestartNoticeAccount } from './codex-restart-notice-account-identity' import { getRepoIdFromWorktreeId, splitWorktreeIdForFilesystem @@ -465,9 +466,14 @@ export type AutomaticAgentResumeClaim = { export type CodexRestartNotice = { previousAccountLabel: string nextAccountLabel: string + /** Ids behind the two labels, when the caller knows them (`null` is the system + * default). Two accounts can share a label, so only these can decide whether + * a re-mark actually points back at the account the pane launched under. */ + previousAccountId?: string | null + nextAccountId?: string | null /** Set once the user asks for the restart. The record outlives the prompt - * because `previousAccountLabel` is the only memory of the account this pane - * actually launched under, which drives the A -> B -> A collapse. */ + * because the previous-account fields are the only memory of the account this + * pane actually launched under, which drives the A -> B -> A collapse. */ restartRequested?: true /** Set when the user answers "Keep old account". Same reason the record has to * survive: deleting it erased the launch account, so re-selecting it looked @@ -650,9 +656,14 @@ export type TerminalSlice = { isPtyShutdownPending: (ptyId: string) => boolean queueCodexPaneRestarts: (ptyIds: string[]) => void consumePendingCodexPaneRestart: (ptyId: string) => boolean + /** Returns the ptyIds left holding a notice, so callers can tell a raised + * prompt from one the A -> B -> A collapse dropped. */ markCodexRestartNotices: ( - notices: { ptyId: string; previousAccountLabel: string; nextAccountLabel: string }[] - ) => void + notices: (Pick & + Partial> & { + ptyId: string + })[] + ) => string[] clearCodexRestartNotice: (ptyId: string) => void dismissCodexRestartNotices: (ptyIds: string[]) => void setTabPaneExpanded: (tabId: string, expanded: boolean) => void @@ -2862,25 +2873,39 @@ export const createTerminalSlice: StateCreator markCodexRestartNotices: (notices) => { if (notices.length === 0) { - return + return [] } + const noticedPtyIds: string[] = [] set((s) => { const next = { ...s.codexRestartNoticeByPtyId } const nextPendingCodexPaneRestartIds = { ...s.pendingCodexPaneRestartIds } for (const notice of notices) { const existing = next[notice.ptyId] - const previousAccountLabel = existing?.previousAccountLabel ?? notice.previousAccountLabel + // Why one record rather than two lookups: the label and the id of the + // launch account have to come from the same source, or they can end up + // describing two different accounts. + const launch = existing ?? notice + const target = { id: notice.nextAccountId, label: notice.nextAccountLabel } // Why: a live Codex pane keeps its original launch account until it actually restarts, so A -> B -> A must not leave a stale restart notice. - if (previousAccountLabel === notice.nextAccountLabel) { + if ( + isSameCodexRestartNoticeAccount( + { id: launch.previousAccountId, label: launch.previousAccountLabel }, + target + ) + ) { delete next[notice.ptyId] delete nextPendingCodexPaneRestartIds[notice.ptyId] continue } next[notice.ptyId] = { - previousAccountLabel, + previousAccountLabel: launch.previousAccountLabel, nextAccountLabel: notice.nextAccountLabel, + ...(launch.previousAccountId === undefined + ? {} + : { previousAccountId: launch.previousAccountId }), + ...(notice.nextAccountId === undefined ? {} : { nextAccountId: notice.nextAccountId }), // Why: a queued restart relaunches under whatever account is selected // when it runs, so a later switch does not reopen an answered prompt. ...(existing?.restartRequested ? { restartRequested: true as const } : {}), @@ -2889,16 +2914,22 @@ export const createTerminalSlice: StateCreator // so a later C reopens the prompt while adding an account or // reauthenticating the active one (both re-mark live panes with the // selection unchanged) must not resurrect it and re-mute the pane. - ...(existing?.dismissed && existing.nextAccountLabel === notice.nextAccountLabel + ...(existing?.dismissed && + isSameCodexRestartNoticeAccount( + { id: existing.nextAccountId, label: existing.nextAccountLabel }, + target + ) ? { dismissed: true as const } : {}) } + noticedPtyIds.push(notice.ptyId) } return { codexRestartNoticeByPtyId: next, pendingCodexPaneRestartIds: nextPendingCodexPaneRestartIds } }) + return noticedPtyIds }, clearCodexRestartNotice: (ptyId) => {