From 68649c8dec68a3f4012d0e3720a4fc40494ff770 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Mon, 26 Jan 2026 21:24:25 -0500 Subject: [PATCH] fix(cli): restore 'Modify with editor' option in external terminals (#17621) --- .../messages/ToolConfirmationMessage.test.tsx | 89 +++++++++++++++++++ .../messages/ToolConfirmationMessage.tsx | 16 ++-- .../ui/contexts/ToolActionsContext.test.tsx | 40 +++++++++ .../src/ui/contexts/ToolActionsContext.tsx | 27 ++++-- 4 files changed, 162 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx index 4b077a84dc..9489ad1d23 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.test.tsx @@ -32,6 +32,7 @@ describe('ToolConfirmationMessage', () => { vi.mocked(useToolActions).mockReturnValue({ confirm: mockConfirm, cancel: vi.fn(), + isDiffingEnabled: false, }); const mockConfig = { @@ -274,4 +275,92 @@ describe('ToolConfirmationMessage', () => { expect(lastFrame()).toContain('Allow for all future sessions'); }); }); + + describe('Modify with external editor option', () => { + const editConfirmationDetails: ToolCallConfirmationDetails = { + type: 'edit', + title: 'Confirm Edit', + fileName: 'test.txt', + filePath: '/test.txt', + fileDiff: '...diff...', + originalContent: 'a', + newContent: 'b', + onConfirm: vi.fn(), + }; + + it('should show "Modify with external editor" when NOT in IDE mode', () => { + const mockConfig = { + isTrustedFolder: () => true, + getIdeMode: () => false, + } as unknown as Config; + + vi.mocked(useToolActions).mockReturnValue({ + confirm: vi.fn(), + cancel: vi.fn(), + isDiffingEnabled: false, + }); + + const { lastFrame } = renderWithProviders( + , + ); + + expect(lastFrame()).toContain('Modify with external editor'); + }); + + it('should show "Modify with external editor" when in IDE mode but diffing is NOT enabled', () => { + const mockConfig = { + isTrustedFolder: () => true, + getIdeMode: () => true, + } as unknown as Config; + + vi.mocked(useToolActions).mockReturnValue({ + confirm: vi.fn(), + cancel: vi.fn(), + isDiffingEnabled: false, + }); + + const { lastFrame } = renderWithProviders( + , + ); + + expect(lastFrame()).toContain('Modify with external editor'); + }); + + it('should NOT show "Modify with external editor" when in IDE mode AND diffing is enabled', () => { + const mockConfig = { + isTrustedFolder: () => true, + getIdeMode: () => true, + } as unknown as Config; + + vi.mocked(useToolActions).mockReturnValue({ + confirm: vi.fn(), + cancel: vi.fn(), + isDiffingEnabled: true, + }); + + const { lastFrame } = renderWithProviders( + , + ); + + expect(lastFrame()).not.toContain('Modify with external editor'); + }); + }); }); diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 3717c2a5b0..90ecb0d8dd 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -52,7 +52,7 @@ export const ToolConfirmationMessage: React.FC< availableTerminalHeight, terminalWidth, }) => { - const { confirm } = useToolActions(); + const { confirm, isDiffingEnabled } = useToolActions(); const settings = useSettings(); const allowPermanentApproval = @@ -111,9 +111,9 @@ export const ToolConfirmationMessage: React.FC< }); } } - // We hide "Modify with external editor" if IDE mode is active, assuming - // the IDE provides a better interface (diff view) for this. - if (!config.getIdeMode()) { + // We hide "Modify with external editor" if IDE mode is active AND + // the IDE is actually capable of showing a diff (connected). + if (!config.getIdeMode() || !isDiffingEnabled) { options.push({ label: 'Modify with external editor', value: ToolConfirmationOutcome.ModifyWithEditor, @@ -210,7 +210,13 @@ export const ToolConfirmationMessage: React.FC< }); } return options; - }, [confirmationDetails, isTrustedFolder, allowPermanentApproval, config]); + }, [ + confirmationDetails, + isTrustedFolder, + allowPermanentApproval, + config, + isDiffingEnabled, + ]); const availableBodyContentHeight = useCallback(() => { if (availableTerminalHeight === undefined) { diff --git a/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx b/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx index 66b9b4f539..5ab9497106 100644 --- a/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx +++ b/packages/cli/src/ui/contexts/ToolActionsContext.test.tsx @@ -177,4 +177,44 @@ describe('ToolActionsContext', () => { throw new Error('Expected onConfirm to be present'); } }); + + it('updates isDiffingEnabled when IdeClient status changes', async () => { + let statusListener: () => void = () => {}; + const mockIdeClient = { + isDiffingEnabled: vi.fn().mockReturnValue(false), + addStatusChangeListener: vi.fn().mockImplementation((listener) => { + statusListener = listener; + }), + removeStatusChangeListener: vi.fn(), + } as unknown as IdeClient; + + vi.mocked(IdeClient.getInstance).mockResolvedValue(mockIdeClient); + vi.mocked(mockConfig.getIdeMode).mockReturnValue(true); + + const { result } = renderHook(() => useToolActions(), { wrapper }); + + // Wait for initialization + await act(async () => { + await vi.waitFor(() => expect(IdeClient.getInstance).toHaveBeenCalled()); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + expect(result.current.isDiffingEnabled).toBe(false); + + // Simulate connection change + vi.mocked(mockIdeClient.isDiffingEnabled).mockReturnValue(true); + await act(async () => { + statusListener(); + }); + + expect(result.current.isDiffingEnabled).toBe(true); + + // Simulate disconnection + vi.mocked(mockIdeClient.isDiffingEnabled).mockReturnValue(false); + await act(async () => { + statusListener(); + }); + + expect(result.current.isDiffingEnabled).toBe(false); + }); }); diff --git a/packages/cli/src/ui/contexts/ToolActionsContext.tsx b/packages/cli/src/ui/contexts/ToolActionsContext.tsx index 417625d998..b0b67ebf38 100644 --- a/packages/cli/src/ui/contexts/ToolActionsContext.tsx +++ b/packages/cli/src/ui/contexts/ToolActionsContext.tsx @@ -30,6 +30,7 @@ interface ToolActionsContextValue { payload?: ToolConfirmationPayload, ) => Promise; cancel: (callId: string) => Promise; + isDiffingEnabled: boolean; } const ToolActionsContext = createContext(null); @@ -55,12 +56,28 @@ export const ToolActionsProvider: React.FC = ( // Hoist IdeClient logic here to keep UI pure const [ideClient, setIdeClient] = useState(null); + const [isDiffingEnabled, setIsDiffingEnabled] = useState(false); + useEffect(() => { let isMounted = true; if (config.getIdeMode()) { IdeClient.getInstance() .then((client) => { - if (isMounted) setIdeClient(client); + if (!isMounted) return; + setIdeClient(client); + setIsDiffingEnabled(client.isDiffingEnabled()); + + const handleStatusChange = () => { + if (isMounted) { + setIsDiffingEnabled(client.isDiffingEnabled()); + } + }; + + client.addStatusChangeListener(handleStatusChange); + // Return a cleanup function for the listener + return () => { + client.removeStatusChangeListener(handleStatusChange); + }; }) .catch((error) => { debugLogger.error('Failed to get IdeClient instance:', error); @@ -88,12 +105,12 @@ export const ToolActionsProvider: React.FC = ( // 1. Handle Side Effects (IDE Diff) if ( details?.type === 'edit' && - ideClient?.isDiffingEnabled() && + isDiffingEnabled && 'filePath' in details // Check for safety ) { const cliOutcome = outcome === ToolConfirmationOutcome.Cancel ? 'rejected' : 'accepted'; - await ideClient.resolveDiffFromCli(details.filePath, cliOutcome); + await ideClient?.resolveDiffFromCli(details.filePath, cliOutcome); } // 2. Dispatch @@ -125,7 +142,7 @@ export const ToolActionsProvider: React.FC = ( debugLogger.warn(`ToolActions: No confirmation mechanism for ${callId}`); }, - [config, ideClient, toolCalls], + [config, ideClient, toolCalls, isDiffingEnabled], ); const cancel = useCallback( @@ -136,7 +153,7 @@ export const ToolActionsProvider: React.FC = ( ); return ( - + {children} );