wip - apply feedback

sourcegraph · Jan 6, 2025 · 9583a67 · 9583a67
1 parent 3ac1ef3
commit 9583a67
Show file tree

Hide file tree

Showing 6 changed files with 34 additions and 36 deletions.
diff --git a/vscode/src/chat/agentic/CodyTool.test.ts b/vscode/src/chat/agentic/CodyTool.test.ts
@@ -6,7 +6,7 @@ import { mockLocalStorage } from '../../services/LocalStorageProvider'
 import type { ContextRetriever } from '../chat-view/ContextRetriever'
 import { CodyTool, OpenCtxTool } from './CodyTool'
 import { CodyToolProvider, ToolFactory, type ToolStatusCallback } from './CodyToolProvider'
-import { toolboxSettings } from './ToolboxManager'
+import { toolboxManager } from './ToolboxManager'
 
 const mockCallback: ToolStatusCallback = {
     onStart: vi.fn(),
@@ -210,7 +210,7 @@ describe('CodyTool', () => {
 
         it('should register all default tools based on toolbox settings', () => {
             const mockedToolboxSettings = { agent: 'mock-agent', shell: true }
-            vi.spyOn(toolboxSettings, 'getSettings').mockReturnValue(mockedToolboxSettings)
+            vi.spyOn(toolboxManager, 'getSettings').mockReturnValue(mockedToolboxSettings)
             const localStorageData: { [key: string]: unknown } = {}
             mockLocalStorage({
                 get: (key: string) => localStorageData[key],
@@ -227,7 +227,7 @@ describe('CodyTool', () => {
 
             // Disable shell and check if terminal tool is removed.
             mockedToolboxSettings.shell = false
-            vi.spyOn(toolboxSettings, 'getSettings').mockReturnValue(mockedToolboxSettings)
+            vi.spyOn(toolboxManager, 'getSettings').mockReturnValue(mockedToolboxSettings)
             const newTools = CodyToolProvider.getTools()
             expect(newTools.some(t => t.config.title.includes('Terminal'))).toBeFalsy()
             expect(newTools.length).toBe(tools.length - 1)

diff --git a/vscode/src/chat/agentic/CodyToolProvider.test.ts b/vscode/src/chat/agentic/CodyToolProvider.test.ts
@@ -6,7 +6,7 @@ import { mockLocalStorage } from '../../services/LocalStorageProvider'
 import type { ContextRetriever } from '../chat-view/ContextRetriever'
 import { CodyTool, type CodyToolConfig } from './CodyTool'
 import { CodyToolProvider, type ToolConfiguration, ToolFactory } from './CodyToolProvider'
-import { toolboxSettings } from './ToolboxManager'
+import { toolboxManager } from './ToolboxManager'
 
 const localStorageData: { [key: string]: unknown } = {}
 mockLocalStorage({
@@ -89,13 +89,13 @@ describe('CodyToolProvider', () => {
     })
 
     it('should not include CLI tool if shell is disabled', () => {
-        vi.spyOn(toolboxSettings, 'getSettings').mockReturnValue({ agent: 'deep-cody', shell: false })
+        vi.spyOn(toolboxManager, 'getSettings').mockReturnValue({ agent: 'deep-cody', shell: false })
         const tools = CodyToolProvider.getTools()
         expect(tools.some(tool => tool.config.title === 'Terminal')).toBe(false)
     })
 
     it('should include CLI tool if shell is enabled', () => {
-        vi.spyOn(toolboxSettings, 'getSettings').mockReturnValue({ agent: 'deep-cody', shell: true })
+        vi.spyOn(toolboxManager, 'getSettings').mockReturnValue({ agent: 'deep-cody', shell: true })
         const tools = CodyToolProvider.getTools()
         expect(tools.some(tool => tool.config.title === 'Terminal')).toBe(true)
     })

diff --git a/vscode/src/chat/agentic/CodyToolProvider.ts b/vscode/src/chat/agentic/CodyToolProvider.ts
@@ -2,14 +2,15 @@ import {
     type ContextMentionProviderMetadata,
     PromptString,
     type Unsubscribable,
+    isDefined,
     openCtx,
     openCtxProviderMetadata,
     ps,
 } from '@sourcegraph/cody-shared'
 import { map } from 'observable-fns'
 import type { ContextRetriever } from '../chat-view/ContextRetriever'
 import { type CodyTool, type CodyToolConfig, OpenCtxTool, TOOL_CONFIGS } from './CodyTool'
-import { toolboxSettings } from './ToolboxManager'
+import { toolboxManager } from './ToolboxManager'
 import { OPENCTX_TOOL_CONFIG } from './config'
 
 /**
@@ -61,15 +62,15 @@ export class ToolFactory {
     public getInstances(): CodyTool[] {
         // Create fresh instances of all registered tools
         return Array.from(this.tools.entries())
-            .filter(([name]) => name !== 'CliTool' || toolboxSettings.getSettings()?.shell)
+            .filter(([name]) => name !== 'CliTool' || toolboxManager.getSettings()?.shell)
             .map(([_, config]) => config.createInstance(config, this.contextRetriever))
-            .filter(Boolean) as CodyTool[]
+            .filter(isDefined) as CodyTool[]
     }
 
     public createDefaultTools(contextRetriever?: Pick<ContextRetriever, 'retrieveContext'>): CodyTool[] {
         return Object.entries(TOOL_CONFIGS)
             .map(([name]) => this.createTool(name, contextRetriever))
-            .filter(Boolean) as CodyTool[]
+            .filter(isDefined) as CodyTool[]
     }
 
     public createOpenCtxTools(providers: ContextMentionProviderMetadata[]): CodyTool[] {
@@ -137,7 +138,7 @@ export class ToolFactory {
  */
 export namespace CodyToolProvider {
     export let factory: ToolFactory
-    let openCtxSubscription: Unsubscribable
+    let openCtxSubscription: Unsubscribable | undefined
 
     export function initialize(contextRetriever: Pick<ContextRetriever, 'retrieveContext'>): void {
         factory = new ToolFactory(contextRetriever)
@@ -174,4 +175,11 @@ export namespace CodyToolProvider {
             })
         }
     }
+
+    export function dispose(): void {
+        if (openCtxSubscription) {
+            openCtxSubscription.unsubscribe()
+            openCtxSubscription = undefined
+        }
+    }
 }
diff --git a/vscode/src/chat/agentic/ToolboxManager.ts b/vscode/src/chat/agentic/ToolboxManager.ts
@@ -27,12 +27,16 @@ type StoredToolboxSettings = {
     readonly shell: boolean
 }
 
-export class ToolboxManager {
+/**
+ * ToolboxManager manages the toolbox settings for the Cody chat agents.
+ * NOTE: This is a Singleton class.
+ */
+class ToolboxManager {
     private static readonly STORAGE_KEY = 'CODY_CHATAGENTS_TOOLBOX_SETTINGS'
     private static instance?: ToolboxManager
 
     private constructor() {
-        // Using private constructor for singleton pattern
+        // Using private constructor for Singleton pattern
     }
 
     private isEnabled = false
@@ -99,10 +103,6 @@ export class ToolboxManager {
             return this.getSettings()
         })
     )
-
-    public get changes(): Observable<void> {
-        return this.changeNotifications
-    }
 }
 
 export const toolboxManager = ToolboxManager.getInstance()
diff --git a/vscode/src/chat/chat-view/ChatController.ts b/vscode/src/chat/chat-view/ChatController.ts
@@ -105,7 +105,7 @@ import {
 import { openExternalLinks } from '../../services/utils/workspace-action'
 import { TestSupport } from '../../test-support'
 import type { MessageErrorType } from '../MessageProvider'
-import { toolboxSettings } from '../agentic/ToolboxManager'
+import { toolboxManager } from '../agentic/ToolboxManager'
 import { getMentionMenuData } from '../context/chatContext'
 import type { ChatIntentAPIClient } from '../context/chatIntentAPIClient'
 import { observeDefaultContext } from '../initialContext'
@@ -662,7 +662,7 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
                     editorState,
                     intent: detectedIntent,
                     manuallySelectedIntent: manuallySelectedIntent ? detectedIntent : undefined,
-                    agent: toolboxSettings.getSettings()?.agent,
+                    agent: toolboxManager.getSettings()?.agent,
                 })
                 this.postViewTranscript({ speaker: 'assistant' })
 
@@ -1534,10 +1534,10 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
                         userProductSubscription.pipe(
                             map(value => (value === pendingOperation ? null : value))
                         ),
-                    toolboxSettings: () => toolboxSettings.settings,
+                    toolboxSettings: () => toolboxManager.settings,
                     updateToolboxSettings: settings => {
                         return promiseFactoryToObservable(async () => {
-                            await toolboxSettings.updatetoolboxSettings(settings)
+                            await toolboxManager.updateToolboxSettings(settings)
                         })
                     },
                 }

diff --git a/vscode/webviews/chat/cells/messageCell/human/editor/toolbar/ToolboxButton.tsx b/vscode/webviews/chat/cells/messageCell/human/editor/toolbar/ToolboxButton.tsx
@@ -1,5 +1,5 @@
 import type { AgentToolboxSettings, WebviewToExtensionAPI } from '@sourcegraph/cody-shared'
-import { BotIcon, BotOffIcon } from 'lucide-react'
+import { BrainIcon } from 'lucide-react'
 import { type FC, memo, useCallback, useEffect, useState } from 'react'
 import { Badge } from '../../../../../../components/shadcn/ui/badge'
 import { Button } from '../../../../../../components/shadcn/ui/button'
@@ -151,22 +151,12 @@ export const ToolboxButton: FC<ToolboxButtonProps> = memo(({ settings, api }) =>
                     },
                 }}
             >
-                <Button variant="ghost" className="!tw-p-1 !tw-opacity-100 tw-relative">
+                <Button variant="ghost" className="!tw-p-2 tw-relative">
+                    <BrainIcon size={16} strokeWidth={1.25} className="tw-w-8 tw-h-8" />
                     {settings.agent ? (
-                        <>
-                            <BotIcon
-                                size={16}
-                                strokeWidth={1.25}
-                                className="!tw-opacity-100 tw-w-8 tw-h-8"
-                            />
-                            <span className="tw-absolute tw-top-0 tw-right-0 tw-w-2 tw-h-2 tw-rounded-full tw-bg-green-500 tw-animate-[pulse_5s_ease-in-out_infinite]" />
-                        </>
+                        <span className="tw-absolute tw-top-0 tw-right-0 tw-w-2 tw-h-2 tw-rounded-full tw-bg-green-500 tw-animate-[pulse_5s_ease-in-out_infinite]" />
                     ) : (
-                        <BotOffIcon
-                            size={16}
-                            strokeWidth={1.25}
-                            className="!tw-opacity-100 tw-w-8 tw-h-8"
-                        />
+                        <span className="tw-absolute tw-top-0 tw-right-0 tw-w-2 tw-h-2 tw-rounded-full tw-bg-red-500 tw-animate-[pulse_5s_ease-in-out_infinite]" />
                     )}
                 </Button>
             </ToolbarPopoverItem>