fix(cli): restore 'Modify with editor' option in external terminals (#17621)

Author: abhipatel12

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(
+        <ToolConfirmationMessage
+          callId="test-call-id"
+          confirmationDetails={editConfirmationDetails}
+          config={mockConfig}
+          availableTerminalHeight={30}
+          terminalWidth={80}
+        />,
+      );
+
+      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(
+        <ToolConfirmationMessage
+          callId="test-call-id"
+          confirmationDetails={editConfirmationDetails}
+          config={mockConfig}
+          availableTerminalHeight={30}
+          terminalWidth={80}
+        />,
+      );
+
+      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(
+        <ToolConfirmationMessage
+          callId="test-call-id"
+          confirmationDetails={editConfirmationDetails}
+          config={mockConfig}
+          availableTerminalHeight={30}
+          terminalWidth={80}
+        />,
+      );
+
+      expect(lastFrame()).not.toContain('Modify with external editor');
+    });
+  });
 });

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) {

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);
+  });
 });

packages/cli/src/ui/contexts/ToolActionsContext.tsx

@@ -30,6 +30,7 @@ interface ToolActionsContextValue {
     payload?: ToolConfirmationPayload,
   ) => Promise<void>;
   cancel: (callId: string) => Promise<void>;
+  isDiffingEnabled: boolean;
 }
 
 const ToolActionsContext = createContext<ToolActionsContextValue | null>(null);
@@ -55,12 +56,28 @@ export const ToolActionsProvider: React.FC<ToolActionsProviderProps> = (
 
   // Hoist IdeClient logic here to keep UI pure
   const [ideClient, setIdeClient] = useState<IdeClient | null>(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<ToolActionsProviderProps> = (
       // 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<ToolActionsProviderProps> = (
 
       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<ToolActionsProviderProps> = (
   );
 
   return (
-    <ToolActionsContext.Provider value={{ confirm, cancel }}>
+    <ToolActionsContext.Provider value={{ confirm, cancel, isDiffingEnabled }}>
       {children}
     </ToolActionsContext.Provider>
   );