-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Ensure adding/removing extensions refreshes extensions list #3695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import type { ExtensionConfig } from '../../../api/types.gen'; | ||
| import { toastService, ToastServiceOptions } from '../../../toasts'; | ||
| import { addToAgent, removeFromAgent } from './agent-api'; | ||
| import { addToAgent, removeFromAgent, sanitizeName } from './agent-api'; | ||
|
|
||
| interface ActivateExtensionProps { | ||
| addToConfig: (name: string, extensionConfig: ExtensionConfig, enabled: boolean) => Promise<void>; | ||
|
|
@@ -129,46 +129,121 @@ export async function addToAgentOnStartup({ | |
| interface UpdateExtensionProps { | ||
| enabled: boolean; | ||
| addToConfig: (name: string, extensionConfig: ExtensionConfig, enabled: boolean) => Promise<void>; | ||
| removeFromConfig: (name: string) => Promise<void>; | ||
| extensionConfig: ExtensionConfig; | ||
| originalName?: string; | ||
| } | ||
|
|
||
| /** | ||
| * Updates an extension configuration without changing its enabled state | ||
| * Updates an extension configuration, handling name changes | ||
| */ | ||
| export async function updateExtension({ | ||
| enabled, | ||
| addToConfig, | ||
| removeFromConfig, | ||
| extensionConfig, | ||
| originalName, | ||
| }: UpdateExtensionProps) { | ||
| if (enabled) { | ||
| // Sanitize the new name to match the behavior when adding extensions | ||
| const sanitizedNewName = sanitizeName(extensionConfig.name); | ||
| const sanitizedOriginalName = originalName ? sanitizeName(originalName) : undefined; | ||
|
|
||
| // Check if the sanitized name has changed | ||
| const nameChanged = sanitizedOriginalName && sanitizedOriginalName !== sanitizedNewName; | ||
|
|
||
| if (nameChanged) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handling this looks good as I know it was something missed before, but some new tests for it would be good! Either in this or a followup
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good will follow up with tests now that we have vitest!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| // Handle name change: remove old extension and add new one | ||
|
|
||
| // First remove the old extension from agent (using original name) | ||
| try { | ||
| // AddToAgent | ||
| await addToAgent(extensionConfig); | ||
| await removeFromAgent(originalName!, { silent: true }); // Suppress removal toast since we'll show update toast | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to add extension to agent during update:', error); | ||
| // Failed to add to agent -- show that error to user and do not update the config file | ||
| throw error; | ||
| console.error('Failed to remove old extension from agent during rename:', error); | ||
| // Continue with the process even if agent removal fails | ||
| } | ||
|
|
||
| // Then add to config | ||
| // Remove old extension from config (using original name) | ||
| try { | ||
| await addToConfig(extensionConfig.name, extensionConfig, enabled); | ||
| await removeFromConfig(originalName!); // We know originalName is not undefined here because nameChanged is true | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to update extension in config:', error); | ||
| throw error; | ||
| console.error('Failed to remove old extension from config during rename:', error); | ||
| throw error; // This is more critical, so we throw | ||
| } | ||
| } else { | ||
|
|
||
| // Create a copy of the extension config with the sanitized name | ||
| const sanitizedExtensionConfig = { | ||
| ...extensionConfig, | ||
| name: sanitizedNewName, | ||
| }; | ||
|
|
||
| // Add new extension with sanitized name | ||
| if (enabled) { | ||
| try { | ||
| // AddToAgent with silent option to avoid duplicate toasts | ||
| await addToAgent(sanitizedExtensionConfig, { silent: true }); | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to add renamed extension to agent:', error); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| // Add to config with sanitized name | ||
| try { | ||
| await addToConfig(extensionConfig.name, extensionConfig, enabled); | ||
| await addToConfig(sanitizedNewName, sanitizedExtensionConfig, enabled); | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to update disabled extension in config:', error); | ||
| console.error('[updateExtension]: Failed to add renamed extension to config:', error); | ||
| throw error; | ||
| } | ||
| // show a toast that it was successfully updated | ||
|
|
||
| toastService.configure({ silent: false }); | ||
| toastService.success({ | ||
| title: `Update extension`, | ||
| msg: `Successfully updated ${extensionConfig.name} extension`, | ||
| msg: `Successfully updated ${sanitizedNewName} extension`, | ||
| }); | ||
| } else { | ||
| // Create a copy of the extension config with the sanitized name | ||
| const sanitizedExtensionConfig = { | ||
| ...extensionConfig, | ||
| name: sanitizedNewName, | ||
| }; | ||
|
|
||
| if (enabled) { | ||
| try { | ||
| // AddToAgent with silent option to avoid duplicate toasts | ||
| await addToAgent(sanitizedExtensionConfig, { silent: true }); | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to add extension to agent during update:', error); | ||
| // Failed to add to agent -- show that error to user and do not update the config file | ||
| throw error; | ||
| } | ||
|
|
||
| // Then add to config | ||
| try { | ||
| await addToConfig(sanitizedNewName, sanitizedExtensionConfig, enabled); | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to update extension in config:', error); | ||
| throw error; | ||
| } | ||
|
|
||
| // show a toast that it was successfully updated | ||
| toastService.success({ | ||
| title: `Update extension`, | ||
| msg: `Successfully updated ${sanitizedNewName} extension`, | ||
| }); | ||
| } else { | ||
| try { | ||
| await addToConfig(sanitizedNewName, sanitizedExtensionConfig, enabled); | ||
| } catch (error) { | ||
| console.error('[updateExtension]: Failed to update disabled extension in config:', error); | ||
| throw error; | ||
| } | ||
|
|
||
| // show a toast that it was successfully updated | ||
| toastService.success({ | ||
| title: `Update extension`, | ||
| msg: `Successfully updated ${sanitizedNewName} extension`, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused on the need for this one. Doesn't:
await getExtensions(true)in fetch extensions already do a force reload?The logic for loading extensions is very fiddly and that's my fault. I just want to be sure of the need for more imperative things to get extensions to reload before we merge.