Skip to content

Conversation

@lily-de
Copy link
Contributor

@lily-de lily-de commented Mar 24, 2025

Implements these flows for adding, removing, and updating extensions:
flows drawio

Follow up PRs will unify this with index.ts

console.error(errorMessage);

if (toastId) toast.dismiss(toastId);
toast(<ErrorMsg name={extensionName} message={data.message} />, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have ToastError I think you can remove the custom ErrorMsg component and just use the reusable version.


try {
toastId = toast.loading(`${actionVerb} ${extensionName} extension...`, {
position: 'top-center',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to go with top-center for more things? I'm just thinking it might feel inconsistent as other things use top right. I am good with either - perhaps we should just be consistent across the board!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top right was annoying and covered the more menu :D top left covered the exit button lol

const errorMessage = `Failed to ${actionType === 'adding' ? 'add' : 'remove'} ${extensionName} extension: ${error instanceof Error ? error.message : 'Unknown error'}`;
console.error(errorMessage);
if (toastId) toast.dismiss(toastId);
toast.error(errorMessage, { autoClose: false });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also nicely be ToastError

// First add to the config system
await addExtensionFn(nameToKey(name), config, true);

if (config.type != 'stdio') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The V1 implementation supported the other kinds as well so this is a reminder to myself to go in very quickly after this and refactor to support HTTP+SSE ones as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants