diff --git a/src/renderer/src/lib/ensure-hooks-confirmed.ts b/src/renderer/src/lib/ensure-hooks-confirmed.ts index 181be47e34d..0a24c776c62 100644 --- a/src/renderer/src/lib/ensure-hooks-confirmed.ts +++ b/src/renderer/src/lib/ensure-hooks-confirmed.ts @@ -129,6 +129,14 @@ async function confirmScriptContent( const previouslyApproved = Boolean(existingHash) return new Promise<'run' | 'skip'>((resolve) => { + let settled = false + const settle = (decision: 'run' | 'skip'): void => { + if (settled) { + return + } + settled = true + resolve(decision) + } state.openModal('confirm-orca-yaml-hooks', { repoId, repoName, @@ -136,10 +144,9 @@ async function confirmScriptContent( scriptContent, contentHash, previouslyApproved, - onResolve: (decision: 'run' | 'skip') => resolve(decision), - // Why: the modal slot holds one entry; losing it must not strand the - // caller awaiting this decision. Dismissal is the same as declining. - [MODAL_DISMISSED_KEY]: () => resolve('skip') + onResolve: settle, + // Why: eviction must fail closed without stranding the singleton trust queue. + [MODAL_DISMISSED_KEY]: () => settle('skip') }) }) } diff --git a/src/renderer/src/lib/orca-yaml-trust-prompt-slot-eviction.test.ts b/src/renderer/src/lib/orca-yaml-trust-prompt-slot-eviction.test.ts index a910bf67d72..56bd061af6b 100644 --- a/src/renderer/src/lib/orca-yaml-trust-prompt-slot-eviction.test.ts +++ b/src/renderer/src/lib/orca-yaml-trust-prompt-slot-eviction.test.ts @@ -13,8 +13,7 @@ const hooksCheckMock = vi.fn() const readIssueCommandMock = vi.fn() const runtimeEnvironmentTransportCallMock = vi.fn() -// Why: the real UI slice owns the single modal slot, so the eviction under test -// has to come from it — a hand-rolled openModal double would decide the outcome. +// Why: a hand-rolled openModal double would decide the eviction outcome under test. function createStateBackedByRealModalSlot(): { store: StoreApi state: AppState @@ -59,13 +58,21 @@ describe('orca.yaml trust prompt evicted from the modal slot', () => { it('resolves the pending decision as skip when another modal takes the slot', async () => { const { store, state } = createStateBackedByRealModalSlot() + const runSetup = vi.fn() const decision = ensureHooksConfirmed(state, 'repo-1', 'setup') await vi.waitFor(() => expect(store.getState().activeModal).toBe('confirm-orca-yaml-hooks')) store.getState().openModal('worktree-palette') - await expect(settleOrReport(decision)).resolves.toBe('skip') + const result = await settleOrReport(decision) + if (result === 'run') { + runSetup() + } + + expect(result).toBe('skip') + expect(runSetup).not.toHaveBeenCalled() + expect(store.getState().trustedOrcaHooks).toEqual({}) }) it('resolves the pending decision as skip when the slot is closed outright', async () => { @@ -101,7 +108,7 @@ describe('orca.yaml trust prompt evicted from the modal slot', () => { const decision = ensureHooksConfirmed(state, 'repo-1', 'setup') await vi.waitFor(() => expect(store.getState().activeModal).toBe('confirm-orca-yaml-hooks')) - // Mirrors OrcaYamlTrustDialog: resolve first, then vacate the slot. + // Mirrors the trust dialog: resolve first, then vacate the slot. ;(store.getState().modalData.onResolve as (d: 'run' | 'skip') => void)('run') store.getState().closeModal() diff --git a/src/renderer/src/store/slices/modal-slot-dismissal.ts b/src/renderer/src/store/slices/modal-slot-dismissal.ts index 540f963cc80..d98a7e50f56 100644 --- a/src/renderer/src/store/slices/modal-slot-dismissal.ts +++ b/src/renderer/src/store/slices/modal-slot-dismissal.ts @@ -1,8 +1,4 @@ -/** - * The app has one modal slot, so opening a modal evicts whatever held it. A modal - * that owns an unsettled promise (a trust prompt awaiting a decision) hands over - * this callback so eviction settles it instead of stranding the awaiting caller. - */ +// Modal owners with pending work use this hook to settle when the singleton slot evicts them. export const MODAL_DISMISSED_KEY = 'onModalDismissed' export function settleEvictedModalData(evicted: Record): void { diff --git a/src/renderer/src/store/slices/ui-modal-slot-dismissal.test.ts b/src/renderer/src/store/slices/ui-modal-slot-dismissal.test.ts new file mode 100644 index 00000000000..c858e0d507e --- /dev/null +++ b/src/renderer/src/store/slices/ui-modal-slot-dismissal.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it, vi } from 'vitest' +import { createUIStore } from './ui-slice-test-harness' +import { MODAL_DISMISSED_KEY } from './modal-slot-dismissal' + +function dismissalData(onDismissed: () => void): Record { + return { [MODAL_DISMISSED_KEY]: onDismissed } +} + +describe('UI modal slot dismissal', () => { + it('dismisses replaced and closed entries once', () => { + const store = createUIStore() + const firstDismissed = vi.fn() + const secondDismissed = vi.fn() + + store.getState().openModal('worktree-palette', dismissalData(firstDismissed)) + store.getState().openModal('quick-open', dismissalData(secondDismissed)) + store.getState().closeModal() + store.getState().closeModal() + + expect(firstDismissed).toHaveBeenCalledOnce() + expect(secondDismissed).toHaveBeenCalledOnce() + }) + + it('dismisses each entry across consecutive replacements', () => { + const store = createUIStore() + const firstDismissed = vi.fn() + const secondDismissed = vi.fn() + const thirdDismissed = vi.fn() + + store.getState().openModal('worktree-palette', dismissalData(firstDismissed)) + store.getState().openModal('quick-open', dismissalData(secondDismissed)) + store.getState().openModal('add-repo', dismissalData(thirdDismissed)) + store.getState().closeModal() + + expect(firstDismissed).toHaveBeenCalledOnce() + expect(secondDismissed).toHaveBeenCalledOnce() + expect(thirdDismissed).toHaveBeenCalledOnce() + }) + + it('updates the slot before notifying the evicted entry', () => { + const store = createUIStore() + const observedModal = vi.fn(() => store.getState().activeModal) + + store.getState().openModal('worktree-palette', dismissalData(observedModal)) + store.getState().openModal('quick-open') + + expect(observedModal).toHaveReturnedWith('quick-open') + }) + + it('ignores modal data without a callable dismissal hook', () => { + const store = createUIStore() + + store.getState().openModal('worktree-palette', { [MODAL_DISMISSED_KEY]: 'invalid' }) + + expect(() => store.getState().openModal('quick-open')).not.toThrow() + expect(store.getState().activeModal).toBe('quick-open') + }) + + it('settles reentrant replacement and close paths once', () => { + const store = createUIStore() + const replacementDismissed = vi.fn() + const reentrantDismissed = vi.fn() + const firstDismissed = vi.fn(() => { + store.getState().openModal('quick-open', dismissalData(reentrantDismissed)) + }) + + store.getState().openModal('worktree-palette', dismissalData(firstDismissed)) + store.getState().openModal('add-repo', dismissalData(replacementDismissed)) + store.getState().closeModal() + + expect(firstDismissed).toHaveBeenCalledOnce() + expect(replacementDismissed).toHaveBeenCalledOnce() + expect(reentrantDismissed).toHaveBeenCalledOnce() + expect(store.getState().activeModal).toBe('none') + }) + + it('performs one bounded notification per transition under repeated replacement', () => { + const store = createUIStore() + const dismissed = new Set() + const transitionCount = 1_000 + + for (let id = 0; id < transitionCount; id += 1) { + store.getState().openModal( + 'worktree-palette', + dismissalData(() => { + expect(dismissed.has(id)).toBe(false) + dismissed.add(id) + }) + ) + } + store.getState().closeModal() + + expect(dismissed.size).toBe(transitionCount) + expect(store.getState().modalData).toEqual({}) + }) +})