diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index d46c58d677..4005d44b43 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -392,7 +392,10 @@ describe('editor utils', () => { ); }); - it(`should reject if ${editor} exits with non-zero code`, async () => { + it(`should resolve and log warning if ${editor} exits with non-zero code`, async () => { + const warnSpy = vi + .spyOn(debugLogger, 'warn') + .mockImplementation(() => {}); const mockSpawnOn = vi.fn((event, cb) => { if (event === 'close') { cb(1); @@ -400,9 +403,73 @@ describe('editor utils', () => { }); (spawn as Mock).mockReturnValue({ on: mockSpawnOn }); + await openDiff('old.txt', 'new.txt', editor); + expect(warnSpy).toHaveBeenCalledWith(`${editor} exited with code 1`); + }); + + it(`should emit ExternalEditorClosed when ${editor} exits successfully`, async () => { + const emitSpy = vi.spyOn(coreEvents, 'emit'); + const mockSpawnOn = vi.fn((event, cb) => { + if (event === 'close') { + cb(0); + } + }); + (spawn as Mock).mockReturnValue({ on: mockSpawnOn }); + + await openDiff('old.txt', 'new.txt', editor); + expect(emitSpy).toHaveBeenCalledWith(CoreEvent.ExternalEditorClosed); + }); + + it(`should emit ExternalEditorClosed when ${editor} exits with non-zero code`, async () => { + vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); + const emitSpy = vi.spyOn(coreEvents, 'emit'); + const mockSpawnOn = vi.fn((event, cb) => { + if (event === 'close') { + cb(1); + } + }); + (spawn as Mock).mockReturnValue({ on: mockSpawnOn }); + + await openDiff('old.txt', 'new.txt', editor); + expect(emitSpy).toHaveBeenCalledWith(CoreEvent.ExternalEditorClosed); + }); + + it(`should emit ExternalEditorClosed when ${editor} spawn errors`, async () => { + const emitSpy = vi.spyOn(coreEvents, 'emit'); + const mockError = new Error('spawn error'); + const mockSpawnOn = vi.fn((event, cb) => { + if (event === 'error') { + cb(mockError); + } + }); + (spawn as Mock).mockReturnValue({ on: mockSpawnOn }); + await expect(openDiff('old.txt', 'new.txt', editor)).rejects.toThrow( - `${editor} exited with code 1`, + 'spawn error', ); + expect(emitSpy).toHaveBeenCalledWith(CoreEvent.ExternalEditorClosed); + }); + + it(`should only emit ExternalEditorClosed once when ${editor} fires both error and close`, async () => { + const emitSpy = vi.spyOn(coreEvents, 'emit'); + const callbacks: Record void> = {}; + const mockSpawnOn = vi.fn( + (event: string, cb: (arg: unknown) => void) => { + callbacks[event] = cb; + }, + ); + (spawn as Mock).mockReturnValue({ on: mockSpawnOn }); + + const promise = openDiff('old.txt', 'new.txt', editor); + // Simulate Node.js behavior: error fires first, then close. + callbacks['error'](new Error('spawn error')); + callbacks['close'](1); + + await expect(promise).rejects.toThrow('spawn error'); + const editorClosedEmissions = emitSpy.mock.calls.filter( + (call) => call[0] === CoreEvent.ExternalEditorClosed, + ); + expect(editorClosedEmissions).toHaveLength(1); }); } diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index cdc1e1d4a5..29dc78fc49 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -323,15 +323,30 @@ export async function openDiff( shell: process.platform === 'win32', }); + // Guard against both 'error' and 'close' firing for a single failure, + // which would emit ExternalEditorClosed twice and attempt to settle + // the promise twice. + let isSettled = false; + childProcess.on('close', (code) => { - if (code === 0) { - resolve(); - } else { - reject(new Error(`${editor} exited with code ${code}`)); + if (isSettled) return; + isSettled = true; + + if (code !== 0) { + // GUI editors (VS Code, Zed, etc.) can exit with non-zero codes + // under normal circumstances (e.g., window closed while loading). + // Log a warning instead of crashing the CLI process. + debugLogger.warn(`${editor} exited with code ${code}`); } + coreEvents.emit(CoreEvent.ExternalEditorClosed); + resolve(); }); childProcess.on('error', (error) => { + if (isSettled) return; + isSettled = true; + + coreEvents.emit(CoreEvent.ExternalEditorClosed); reject(error); }); });