-
Notifications
You must be signed in to change notification settings - Fork 65
feat(playground): mcp servers listed #3065
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
base: main
Are you sure you want to change the base?
feat(playground): mcp servers listed #3065
Conversation
packages/backend/src/studio.ts
Outdated
@@ -213,6 +213,7 @@ export class Studio { | |||
*/ | |||
this.#catalogManager = new CatalogManager(this.#rpcExtension, appUserDirectory); | |||
this.#catalogManager.init(); | |||
this.#extensionContext.subscriptions.push(this.#catalogManager); |
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.
This was missing, it's my understanding that it should be pushed to the subscriptions for disposal.
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.
It is now optional as every Disposable it automatically disposed when extension is stopped. Should be moved to a separate PR
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've removed my additions of what I considered to be missing entries.
However, I'm having a hard time understanding how are the Disposable
s discovered by the extension for automatic disposal.
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.
Yeah, for extension-defined disposable, it should be disposed by the extension. The disposable provided by the extension-api are disposed by the core, but I think it is always a good idea to do it, for good practice.
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.
Hi @axel7083 and @jeffmaury
This PR has been stalled for 2 weeks now.
I'm unsure of how to proceed.
It is now optional as every Disposable it automatically disposed when extension is stopped. Should be moved to a separate PR
This was addressed in a subsequent commit.
However, Axel did mention that we should dispose this in the extension.
From my side, I'm not sure on how to proceed so that we can move on.
Please, provide guidelines so that I can address them and continue with the task.
packages/backend/src/studio.ts
Outdated
@@ -385,6 +386,7 @@ export class Studio { | |||
*/ | |||
this.#snippetManager = new SnippetManager(this.#rpcExtension, this.#telemetry); | |||
this.#snippetManager.init(); | |||
this.#extensionContext.subscriptions.push(this.#snippetManager); |
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.
This was missing, it's my understanding that it should be pushed to the subscriptions for disposal.
c92bbfd
to
b240506
Compare
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.
See comment
f52c860
to
fae6f17
Compare
fae6f17
to
87e6cce
Compare
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.
Code LGTM but:
- the style for the MCP servers is not aligned with other sections titles in the left pane
- when clicking on MCP servers, the UI is displayed in the middle of the page which seems strange to me
- once the MCP servers are displayed, clicking Escape to close will close the playground and return to the list of playgrounds
LGTM codewise as well! probs needs a rebase |
Signed-off-by: Marc Nuri <[email protected]>
Signed-off-by: Marc Nuri <[email protected]>
87e6cce
to
8f41027
Compare
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.
LGTM @jeffmaury
What does this PR do?
Adds a UI element to be able to (later) activate/deactivate MCP Servers and their specific tools (even later).
This is the first iteration just to validate that the UI is aligned with the current expectation.
The next immediate iteration would be to add functionality to the checkboxes of the servers.
With this regard it would be good to know what effect we want for them.
For example, do we want them to be mapped to the file entries and update them there?
Screenshot / video of UI
podman-desktop-ai-lab-mcp-servers-ui.mp4
What issues does this PR fix or reference?
Part of #2885
How to test this PR?
~/.local/share/containers/podman-desktop/extensions-storage/redhat.ai-lab/mcp-settings.json
.MCP Servers
should be visible in the Settings pane.