From a3097f5250355ba9671c3dfca2ca6692f7e117e7 Mon Sep 17 00:00:00 2001 From: Amed Rodriguez Date: Tue, 26 Aug 2025 13:28:10 -0700 Subject: [PATCH] Refactor Extensions Install Modal (#4328) --- ui/desktop/src/App.tsx | 23 +- .../components/ExtensionInstallModal.test.tsx | 161 +++++++++++++ .../ExtensionInstallModal.tsx} | 150 +++++++++--- .../modals/ExtensionInstallModal.tsx | 88 ------- .../hooks/useExtensionInstallModal.test.ts | 224 ------------------ ui/desktop/src/types/extension.ts | 30 --- 6 files changed, 279 insertions(+), 397 deletions(-) create mode 100644 ui/desktop/src/components/ExtensionInstallModal.test.tsx rename ui/desktop/src/{hooks/useExtensionInstallModal.ts => components/ExtensionInstallModal.tsx} (66%) delete mode 100644 ui/desktop/src/components/modals/ExtensionInstallModal.tsx delete mode 100644 ui/desktop/src/hooks/useExtensionInstallModal.test.ts delete mode 100644 ui/desktop/src/types/extension.ts diff --git a/ui/desktop/src/App.tsx b/ui/desktop/src/App.tsx index 2c1402b1..5b1c2f85 100644 --- a/ui/desktop/src/App.tsx +++ b/ui/desktop/src/App.tsx @@ -2,8 +2,7 @@ import { useEffect, useRef, useState } from 'react'; import { IpcRendererEvent } from 'electron'; import { HashRouter, Routes, Route, useNavigate, useLocation } from 'react-router-dom'; import { ErrorUI } from './components/ErrorBoundary'; -import { ExtensionInstallModal } from './components/modals/ExtensionInstallModal'; -import { useExtensionInstallModal } from './hooks/useExtensionInstallModal'; +import { ExtensionInstallModal } from './components/ExtensionInstallModal'; import { ToastContainer } from 'react-toastify'; import { GoosehintsModal } from './components/GoosehintsModal'; import AnnouncementModal from './components/AnnouncementModal'; @@ -366,8 +365,6 @@ export default function App() { const { getExtensions, addExtension, read } = useConfig(); const initAttemptedRef = useRef(false); - const { modalState, modalConfig, dismissModal, confirmInstall } = - useExtensionInstallModal(addExtension); // Create a setView function for useChat hook - we'll use window.history instead of navigate const setView = (view: View, viewOptions: ViewOptions = {}) => { @@ -664,15 +661,6 @@ export default function App() { }; }, []); - const handleExtensionConfirm = async () => { - const result = await confirmInstall(); - if (result.success) { - console.log('Extension installation completed successfully'); - } else { - console.error('Extension installation failed:', result.error); - } - }; - if (fatalError) { return ; } @@ -704,14 +692,7 @@ export default function App() { closeOnClick pauseOnHover /> - +
diff --git a/ui/desktop/src/components/ExtensionInstallModal.test.tsx b/ui/desktop/src/components/ExtensionInstallModal.test.tsx new file mode 100644 index 00000000..ca2d54e5 --- /dev/null +++ b/ui/desktop/src/components/ExtensionInstallModal.test.tsx @@ -0,0 +1,161 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, act } from '@testing-library/react'; +import { ExtensionInstallModal } from './ExtensionInstallModal'; +import { addExtensionFromDeepLink } from './settings/extensions/deeplink'; + +vi.mock('./settings/extensions/deeplink', () => ({ + addExtensionFromDeepLink: vi.fn(), +})); + +const mockElectron = { + getConfig: vi.fn(), + getAllowedExtensions: vi.fn(), + logInfo: vi.fn(), + on: vi.fn(), + off: vi.fn(), +}; + +(window as any).electron = mockElectron; + +describe('ExtensionInstallModal', () => { + const mockAddExtension = vi.fn(); + + const getAddExtensionEventHandler = () => { + const addExtensionCall = mockElectron.on.mock.calls.find((call) => call[0] === 'add-extension'); + expect(addExtensionCall).toBeDefined(); + return addExtensionCall![1]; + }; + + beforeEach(() => { + vi.clearAllMocks(); + mockElectron.getConfig.mockReturnValue({ + GOOSE_ALLOWLIST_WARNING: false, + }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe('Extension Request Handling', () => { + it('should handle trusted extension (default behaviour, no allowlist)', async () => { + mockElectron.getAllowedExtensions.mockResolvedValue([]); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler({}, 'goose://extension?cmd=npx&arg=test-extension&name=TestExt'); + }); + + expect(screen.getByRole('dialog')).toBeInTheDocument(); + expect(screen.getByText('Confirm Extension Installation')).toBeInTheDocument(); + expect(screen.getByText(/TestExt extension/)).toBeInTheDocument(); + expect(screen.getAllByRole('button')).toHaveLength(3); + }); + + it('should handle trusted extension (from allowlist)', async () => { + mockElectron.getAllowedExtensions.mockResolvedValue(['npx test-extension']); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler({}, 'goose://extension?cmd=npx&arg=test-extension&name=AllowedExt'); + }); + + expect(screen.getByText('Confirm Extension Installation')).toBeInTheDocument(); + expect(screen.getAllByRole('button')).toHaveLength(3); + }); + + it('should handle warning mode', async () => { + mockElectron.getConfig.mockReturnValue({ + GOOSE_ALLOWLIST_WARNING: true, + }); + mockElectron.getAllowedExtensions.mockResolvedValue(['uvx allowed-package']); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler( + {}, + 'goose://extension?cmd=npx&arg=untrusted-extension&name=UntrustedExt' + ); + }); + + expect(screen.getByText('Install Untrusted Extension?')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Install Anyway' })).toBeInTheDocument(); + expect(screen.getAllByRole('button')).toHaveLength(3); + }); + + it('should handle blocked extension', async () => { + mockElectron.getAllowedExtensions.mockResolvedValue(['uvx allowed-package']); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler({}, 'goose://extension?cmd=npx&arg=blocked-extension&name=BlockedExt'); + }); + + expect(screen.getByText('Extension Installation Blocked')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'OK' })).toBeInTheDocument(); + expect(screen.getAllByRole('button')).toHaveLength(2); + expect(screen.getByText(/Contact your administrator/)).toBeInTheDocument(); + }); + }); + + describe('Modal Actions', () => { + it('should dismiss modal correctly', async () => { + mockElectron.getAllowedExtensions.mockResolvedValue([]); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler({}, 'goose://extension?cmd=npx&arg=test&name=Test'); + }); + + expect(screen.getByRole('dialog')).toBeInTheDocument(); + + await act(async () => { + screen.getByRole('button', { name: 'No' }).click(); + }); + + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + + it('should handle successful extension installation', async () => { + vi.mocked(addExtensionFromDeepLink).mockResolvedValue(undefined); + mockElectron.getAllowedExtensions.mockResolvedValue([]); + + render(); + + const eventHandler = getAddExtensionEventHandler(); + + await act(async () => { + await eventHandler({}, 'goose://extension?cmd=npx&arg=test&name=Test'); + }); + + await act(async () => { + screen.getByRole('button', { name: 'Yes' }).click(); + }); + + expect(addExtensionFromDeepLink).toHaveBeenCalledWith( + 'goose://extension?cmd=npx&arg=test&name=Test', + mockAddExtension, + expect.any(Function) + ); + + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + }); + }); +}); diff --git a/ui/desktop/src/hooks/useExtensionInstallModal.ts b/ui/desktop/src/components/ExtensionInstallModal.tsx similarity index 66% rename from ui/desktop/src/hooks/useExtensionInstallModal.ts rename to ui/desktop/src/components/ExtensionInstallModal.tsx index fafb0bb2..2ac3bea0 100644 --- a/ui/desktop/src/hooks/useExtensionInstallModal.ts +++ b/ui/desktop/src/components/ExtensionInstallModal.tsx @@ -1,15 +1,47 @@ import { useState, useCallback, useEffect } from 'react'; import { IpcRendererEvent } from 'electron'; -import { extractExtensionName } from '../components/settings/extensions/utils'; -import { addExtensionFromDeepLink } from '../components/settings/extensions/deeplink'; -import type { ExtensionConfig } from '../api/types.gen'; import { - ExtensionModalState, - ExtensionInfo, - ModalType, - ExtensionModalConfig, - ExtensionInstallResult, -} from '../types/extension'; + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from './ui/dialog'; +import { Button } from './ui/button'; +import { extractExtensionName } from './settings/extensions/utils'; +import { addExtensionFromDeepLink } from './settings/extensions/deeplink'; +import type { ExtensionConfig } from '../api/types.gen'; + +type ModalType = 'blocked' | 'untrusted' | 'trusted'; + +interface ExtensionInfo { + name: string; + command?: string; + remoteUrl?: string; + link: string; +} + +interface ExtensionModalState { + isOpen: boolean; + modalType: ModalType; + extensionInfo: ExtensionInfo | null; + isPending: boolean; + error: string | null; +} + +interface ExtensionModalConfig { + title: string; + message: string; + confirmLabel: string; + cancelLabel: string; + showSingleButton: boolean; + isBlocked: boolean; +} + +interface ExtensionInstallModalProps { + addExtension?: (name: string, config: ExtensionConfig, enabled: boolean) => Promise; +} function extractCommand(link: string): string { const url = new URL(link); @@ -23,9 +55,7 @@ function extractRemoteUrl(link: string): string | null { return url.searchParams.get('url'); } -export const useExtensionInstallModal = ( - addExtension?: (name: string, config: ExtensionConfig, enabled: boolean) => Promise -) => { +export function ExtensionInstallModal({ addExtension }: ExtensionInstallModalProps) { const [modalState, setModalState] = useState({ isOpen: false, modalType: 'trusted', @@ -44,14 +74,12 @@ export const useExtensionInstallModal = ( const config = window.electron.getConfig(); const ALLOWLIST_WARNING_MODE = config.GOOSE_ALLOWLIST_WARNING === true; - // If warning mode is enabled, always show warning but allow installation if (ALLOWLIST_WARNING_MODE) { return 'untrusted'; } const allowedCommands = await window.electron.getAllowedExtensions(); - // If no allowlist configured if (!allowedCommands || allowedCommands.length === 0) { return 'trusted'; } @@ -158,9 +186,9 @@ export const useExtensionInstallModal = ( setPendingLink(null); }, []); - const confirmInstall = useCallback(async (): Promise => { + const confirmInstall = useCallback(async (): Promise => { if (!pendingLink) { - return { success: false, error: 'No pending extension to install' }; + return; } setModalState((prev) => ({ ...prev, isPending: true })); @@ -168,17 +196,16 @@ export const useExtensionInstallModal = ( try { console.log(`Confirming installation of extension from: ${pendingLink}`); - dismissModal(); - if (addExtension) { await addExtensionFromDeepLink(pendingLink, addExtension, () => { console.log('Extension installation completed, navigating to extensions'); }); } else { - throw new Error('addExtension function not provided to hook'); + throw new Error('addExtension function not provided to component'); } - return { success: true }; + // Only dismiss modal after successful installation + dismissModal(); } catch (error) { const errorMessage = error instanceof Error ? error.message : 'Installation failed'; console.error('Extension installation failed:', error); @@ -188,16 +215,9 @@ export const useExtensionInstallModal = ( error: errorMessage, isPending: false, })); - - return { success: false, error: errorMessage }; } }, [pendingLink, dismissModal, addExtension]); - const getModalConfig = (): ExtensionModalConfig | null => { - if (!modalState.extensionInfo) return null; - return generateModalConfig(modalState.modalType, modalState.extensionInfo); - }; - useEffect(() => { console.log('Setting up extension install modal handler'); @@ -213,11 +233,73 @@ export const useExtensionInstallModal = ( }; }, [handleExtensionRequest]); - return { - modalState, - modalConfig: getModalConfig(), - handleExtensionRequest, - dismissModal, - confirmInstall, + const getModalConfig = (): ExtensionModalConfig | null => { + if (!modalState.extensionInfo) return null; + return generateModalConfig(modalState.modalType, modalState.extensionInfo); }; -}; + + const config = getModalConfig(); + if (!config) return null; + + const getConfirmButtonVariant = () => { + switch (modalState.modalType) { + case 'blocked': + return 'outline'; + case 'untrusted': + return 'destructive'; + case 'trusted': + default: + return 'default'; + } + }; + + const getTitleClassName = () => { + switch (modalState.modalType) { + case 'blocked': + return 'text-red-600 dark:text-red-400'; + case 'untrusted': + return 'text-yellow-600 dark:text-yellow-400'; + case 'trusted': + default: + return ''; + } + }; + + return ( + !open && dismissModal()}> + + + {config.title} + + {config.message} + + + + + {config.showSingleButton ? ( + + ) : ( + <> + + + + )} + + + + ); +} diff --git a/ui/desktop/src/components/modals/ExtensionInstallModal.tsx b/ui/desktop/src/components/modals/ExtensionInstallModal.tsx deleted file mode 100644 index 86f63219..00000000 --- a/ui/desktop/src/components/modals/ExtensionInstallModal.tsx +++ /dev/null @@ -1,88 +0,0 @@ -import { - Dialog, - DialogContent, - DialogDescription, - DialogFooter, - DialogHeader, - DialogTitle, -} from '../ui/dialog'; -import { Button } from '../ui/button'; -import { ModalType, ExtensionModalConfig } from '../../types/extension'; - -interface ExtensionInstallModalProps { - isOpen: boolean; - modalType: ModalType; - config: ExtensionModalConfig | null; - onConfirm: () => void; - onCancel: () => void; - isSubmitting?: boolean; -} - -export function ExtensionInstallModal({ - isOpen, - modalType, - config, - onConfirm, - onCancel, - isSubmitting = false, -}: ExtensionInstallModalProps) { - if (!config) return null; - - const getConfirmButtonVariant = () => { - switch (modalType) { - case 'blocked': - return 'outline'; - case 'untrusted': - return 'destructive'; - case 'trusted': - default: - return 'default'; - } - }; - - const getTitleClassName = () => { - switch (modalType) { - case 'blocked': - return 'text-red-600 dark:text-red-400'; - case 'untrusted': - return 'text-yellow-600 dark:text-yellow-400'; - case 'trusted': - default: - return ''; - } - }; - - return ( - !open && onCancel()}> - - - {config.title} - - {config.message} - - - - - {config.showSingleButton ? ( - - ) : ( - <> - - - - )} - - - - ); -} diff --git a/ui/desktop/src/hooks/useExtensionInstallModal.test.ts b/ui/desktop/src/hooks/useExtensionInstallModal.test.ts deleted file mode 100644 index 884da36b..00000000 --- a/ui/desktop/src/hooks/useExtensionInstallModal.test.ts +++ /dev/null @@ -1,224 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; -import { useExtensionInstallModal } from './useExtensionInstallModal'; -import { addExtensionFromDeepLink } from '../components/settings/extensions/deeplink'; - -const mockElectron = { - getConfig: vi.fn(), - getAllowedExtensions: vi.fn(), - logInfo: vi.fn(), - processExtensionLink: vi.fn(), - on: vi.fn(), - off: vi.fn(), -}; - -vi.mock('../components/settings/extensions/utils', () => ({ - extractExtensionName: vi.fn((link: string) => { - const url = new URL(link); - return url.searchParams.get('name') || 'Unknown Extension'; - }), -})); - -vi.mock('../components/settings/extensions/deeplink', () => ({ - addExtensionFromDeepLink: vi.fn(), -})); - -beforeEach(() => { - Object.defineProperty(globalThis, 'window', { - value: { - electron: mockElectron, - }, - writable: true, - }); - - mockElectron.getConfig.mockReturnValue({ - GOOSE_ALLOWLIST_WARNING: false, - }); -}); - -afterEach(() => { - vi.clearAllMocks(); -}); - -describe('useExtensionInstallModal', () => { - const mockAddExtension = vi.fn(); - - describe('Initial State', () => { - it('should initialize with correct default state', () => { - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - expect(result.current.modalState).toEqual({ - isOpen: false, - modalType: 'trusted', - extensionInfo: null, - isPending: false, - error: null, - }); - expect(result.current.modalConfig).toBeNull(); - }); - }); - - describe('Extension Request Handling', () => { - it('should handle trusted extension (default behaviour, no allowlist)', async () => { - mockElectron.getAllowedExtensions.mockResolvedValue([]); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest( - 'goose://extension?cmd=npx&arg=test-extension&name=TestExt' - ); - }); - - expect(result.current.modalState.isOpen).toBe(true); - expect(result.current.modalState.modalType).toBe('trusted'); - expect(result.current.modalState.extensionInfo?.name).toBe('TestExt'); - expect(result.current.modalConfig?.title).toBe('Confirm Extension Installation'); - }); - - it('should handle trusted extension (from allowlist)', async () => { - mockElectron.getAllowedExtensions.mockResolvedValue(['npx test-extension']); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest( - 'goose://extension?cmd=npx&arg=test-extension&name=AllowedExt' - ); - }); - - expect(result.current.modalState.modalType).toBe('trusted'); - expect(result.current.modalConfig?.title).toBe('Confirm Extension Installation'); - }); - - it('should handle warning mode', async () => { - mockElectron.getConfig.mockReturnValue({ - GOOSE_ALLOWLIST_WARNING: true, - }); - - mockElectron.getAllowedExtensions.mockResolvedValue(['uvx allowed-package']); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest( - 'goose://extension?cmd=npx&arg=untrusted-extension&name=UntrustedExt' - ); - }); - - expect(result.current.modalState.modalType).toBe('untrusted'); - expect(result.current.modalConfig?.title).toBe('Install Untrusted Extension?'); - expect(result.current.modalConfig?.confirmLabel).toBe('Install Anyway'); - expect(result.current.modalConfig?.showSingleButton).toBe(false); - }); - - it('should handle blocked extension', async () => { - mockElectron.getAllowedExtensions.mockResolvedValue(['uvx allowed-package']); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest( - 'goose://extension?cmd=npx&arg=blocked-extension&name=BlockedExt' - ); - }); - - expect(result.current.modalState.modalType).toBe('blocked'); - expect(result.current.modalConfig?.title).toBe('Extension Installation Blocked'); - expect(result.current.modalConfig?.confirmLabel).toBe('OK'); - expect(result.current.modalConfig?.showSingleButton).toBe(true); - expect(result.current.modalConfig?.isBlocked).toBe(true); - }); - }); - - describe('Modal Actions', () => { - it('should dismiss modal correctly', async () => { - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest('goose://extension?cmd=npx&arg=test&name=Test'); - }); - - expect(result.current.modalState.isOpen).toBe(true); - - act(() => { - result.current.dismissModal(); - }); - - expect(result.current.modalState.isOpen).toBe(false); - expect(result.current.modalState.extensionInfo).toBeNull(); - }); - - it('should handle successful extension installation', async () => { - vi.mocked(addExtensionFromDeepLink).mockResolvedValue(undefined); - mockElectron.getAllowedExtensions.mockResolvedValue([]); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest('goose://extension?cmd=npx&arg=test&name=Test'); - }); - - let installResult; - await act(async () => { - installResult = await result.current.confirmInstall(); - }); - - expect(installResult).toEqual({ success: true }); - expect(addExtensionFromDeepLink).toHaveBeenCalledWith( - 'goose://extension?cmd=npx&arg=test&name=Test', - mockAddExtension, - expect.any(Function) - ); - expect(result.current.modalState.isOpen).toBe(false); - }); - - it('should handle failed extension installation', async () => { - const error = new Error('Installation failed'); - vi.mocked(addExtensionFromDeepLink).mockRejectedValue(error); - mockElectron.getAllowedExtensions.mockResolvedValue([]); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest('goose://extension?cmd=npx&arg=test&name=Test'); - }); - - let installResult; - await act(async () => { - installResult = await result.current.confirmInstall(); - }); - - expect(installResult).toEqual({ - success: false, - error: 'Installation failed', - }); - expect(result.current.modalState.error).toBe('Installation failed'); - }); - - it('should not install blocked extensions', async () => { - mockElectron.getAllowedExtensions.mockResolvedValue(['uvx allowed-package']); - - const { result } = renderHook(() => useExtensionInstallModal(mockAddExtension)); - - await act(async () => { - await result.current.handleExtensionRequest( - 'goose://extension?cmd=npx&arg=blocked&name=Blocked' - ); - }); - - expect(result.current.modalState.modalType).toBe('blocked'); - - let installResult; - await act(async () => { - installResult = await result.current.confirmInstall(); - }); - - expect(installResult).toEqual({ - success: false, - error: 'No pending extension to install', - }); - expect(addExtensionFromDeepLink).not.toHaveBeenCalled(); - }); - }); -}); diff --git a/ui/desktop/src/types/extension.ts b/ui/desktop/src/types/extension.ts deleted file mode 100644 index 01a2f265..00000000 --- a/ui/desktop/src/types/extension.ts +++ /dev/null @@ -1,30 +0,0 @@ -export type ModalType = 'blocked' | 'untrusted' | 'trusted'; - -export interface ExtensionInfo { - name: string; - command?: string; - remoteUrl?: string; - link: string; -} - -export interface ExtensionModalState { - isOpen: boolean; - modalType: ModalType; - extensionInfo: ExtensionInfo | null; - isPending: boolean; - error: string | null; -} - -export interface ExtensionModalConfig { - title: string; - message: string; - confirmLabel: string; - cancelLabel: string; - showSingleButton: boolean; - isBlocked: boolean; -} - -export interface ExtensionInstallResult { - success: boolean; - error?: string; -}