Standalone mcp apps #6458
Standalone mcp apps #6458
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to launch standalone MCP apps from UI resources provided by MCP servers. Apps can be browsed in a new Apps view and launched in separate windows, with HTML content cached locally for faster loading.
Changes:
- Adds backend caching and API endpoint for listing MCP apps from UI resources
- Implements new Apps view for browsing available apps and a StandaloneAppView component for rendering apps in separate windows
- Integrates app launching functionality into the Electron main process with IPC handlers
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/preload.ts | Adds IPC bridge method for launching apps |
| ui/desktop/src/main.ts | Implements Electron handler to create standalone app windows |
| ui/desktop/src/components/apps/StandaloneAppView.tsx | New component for rendering apps in standalone windows with cached HTML support |
| ui/desktop/src/components/apps/AppsView.tsx | New view for browsing and launching available MCP apps |
| ui/desktop/src/components/McpApps/McpAppRenderer.tsx | Adds fullscreen mode and cached HTML support for app rendering |
| ui/desktop/src/components/GooseSidebar/AppSidebar.tsx | Adds Apps menu item to sidebar navigation |
| ui/desktop/src/hooks/useChatStream.ts | Pre-populates apps cache on session load |
| ui/desktop/src/App.tsx | Adds routes for apps view and standalone apps |
| ui/desktop/src/api/types.gen.ts | Generated types for GooseApp and list_apps API |
| ui/desktop/src/api/sdk.gen.ts | Generated SDK method for list_apps endpoint |
| ui/desktop/openapi.json | OpenAPI schema for list_apps endpoint |
| ui/desktop/package-lock.json | Peer dependency reorganization (no new dependencies) |
| crates/goose/src/goose_apps/mod.rs | Implements app caching to disk and listing functionality |
| crates/goose-server/src/routes/agent.rs | Adds list_apps HTTP endpoint with cache support |
| crates/goose-server/src/openapi.rs | Registers new API types and endpoint in OpenAPI schema |
Files not reviewed (1)
- ui/desktop/package-lock.json: Language not supported
| fn cache_key(extension_name: &str, resource_uri: &str) -> String { | ||
| use std::collections::hash_map::DefaultHasher; | ||
| use std::hash::{Hash, Hasher}; | ||
|
|
||
| let mut hasher = DefaultHasher::new(); | ||
| format!("{}::{}", extension_name, resource_uri).hash(&mut hasher); | ||
| format!("{}_{:x}", extension_name, hasher.finish()) | ||
| } |
There was a problem hiding this comment.
The cache key uses DefaultHasher which is not guaranteed to be stable across process restarts or Rust versions. This could lead to cache misses for the same app after updates. Consider using a stable hash function like SHA256 or SipHasher.
| let agent = state | ||
| .get_agent_for_route(params.session_id.unwrap()) |
There was a problem hiding this comment.
Using unwrap() on a checked Option could panic if session_id was checked earlier but the value changes. The session_id is checked for None on line 958 but then unwrap() is called on line 968. While this appears safe in the current code flow, consider using if-let or expect with a clear message for better error handling clarity.
| let agent = state | |
| .get_agent_for_route(params.session_id.unwrap()) | |
| let session_id = match params.session_id { | |
| Some(id) => id, | |
| None => { | |
| return Err(ErrorResponse { | |
| message: "Missing session_id for list_apps request".to_string(), | |
| status: StatusCode::BAD_REQUEST, | |
| }); | |
| } | |
| }; | |
| let agent = state | |
| .get_agent_for_route(session_id) |
| const apps = response.data?.apps || []; | ||
| setApps(apps); |
There was a problem hiding this comment.
The loadApps callback is defined but only used in the error state Retry button. However, it shadows the apps variable from state with a local variable on line 84. This could lead to confusion. Consider renaming the local variable to avoid shadowing.
| const apps = response.data?.apps || []; | |
| setApps(apps); | |
| const fetchedApps = response.data?.apps || []; | |
| setApps(fetchedApps); |
| useEffect(() => { | ||
| if (cachedHtml) { | ||
| setResourceHtml(cachedHtml); | ||
| } | ||
| }, [cachedHtml]); |
There was a problem hiding this comment.
There are two separate useEffect hooks that both set resourceHtml with cachedHtml (lines 54-58 and 78-80). The first effect is redundant since resourceHtml is already initialized with cachedHtml in the useState declaration on line 49. Remove the first useEffect to avoid unnecessary updates.
| const standaloneUrl = | ||
| `${baseUrl}/#/standalone-app?` + | ||
| `resourceUri=${encodeURIComponent(gooseApp.resourceUri)}` + | ||
| `&extensionName=${encodeURIComponent(gooseApp.mcpServer || '')}` + | ||
| `&appName=${encodeURIComponent(gooseApp.name)}` + | ||
| `&workingDir=${encodeURIComponent(workingDir)}`; |
There was a problem hiding this comment.
The URL is constructed using string concatenation with encodeURIComponent for query parameters. While the encoding is present, the baseUrl comes from the current window URL which could potentially be manipulated. Consider validating that baseUrl matches expected origins before constructing the standalone URL.
| } | ||
|
|
||
| initSession(); | ||
| }, [resourceUri, extensionName, workingDir, cachedHtml]); |
There was a problem hiding this comment.
The second useEffect depends on cachedHtml which creates an unnecessary dependency. When cachedHtml is set by the first effect, it will trigger the second effect to re-run initSession, which could create duplicate sessions. The cachedHtml should only be used to conditionally set the error state, not as a trigger for the effect.
| }, [resourceUri, extensionName, workingDir, cachedHtml]); | |
| }, [resourceUri, extensionName, workingDir]); |
| const startResponse = await startAgent({ | ||
| body: { working_dir: workingDir }, | ||
| throwOnError: true, | ||
| }); | ||
|
|
||
| const sid = startResponse.data.id; | ||
|
|
||
| await resumeAgent({ | ||
| body: { | ||
| session_id: sid, | ||
| load_model_and_extensions: true, | ||
| }, | ||
| throwOnError: true, | ||
| }); | ||
|
|
||
| setSessionId(sid); | ||
| setLoading(false); |
There was a problem hiding this comment.
Each standalone app window creates a new agent session but doesn't clean it up when the window closes. The session should be stopped when the window closes to avoid accumulating orphaned sessions and their resources. Consider tracking the session ID and calling a cleanup function when the component unmounts.
| if ( | ||
| !resourceUri || | ||
| !extensionName || | ||
| !workingDir || |
There was a problem hiding this comment.
The check for workingDir on line 59 doesn't handle the case where workingDir is null (only handles undefined and 'undefined' string). Add null check: !workingDir || workingDir === 'undefined' || workingDir === 'null' to be consistent.
| !workingDir || | |
| !workingDir || | |
| workingDir === 'undefined' || | |
| workingDir === 'null' || |
| }; | ||
|
|
||
| checkApps(); | ||
| }, [currentPath]); |
There was a problem hiding this comment.
The Apps sidebar item visibility is checked on every currentPath change. Since the cache rarely changes, this causes unnecessary API calls. Consider moving this check outside the dependency on currentPath, or polling less frequently.
| }, [currentPath]); | |
| }, []); |
| appWindow.on('close', () => { | ||
| goosedClients.delete(appWindow.id); | ||
| }); |
There was a problem hiding this comment.
The window close handler only removes the client reference but doesn't stop the agent session created in StandaloneAppView. While StandaloneAppView has a cleanup effect, if the window is closed before the component unmounts properly, the session may leak. Consider adding session cleanup here or ensuring the cleanup effect always runs.
| <GridLayout> | ||
| {apps.map((app, index) => ( | ||
| <div | ||
| key={index} |
There was a problem hiding this comment.
Using array index as React key is an anti-pattern that can cause rendering issues when the list changes. Use a stable unique identifier like ${app.resourceUri}-${app.mcpServer} instead.
| key={index} | |
| key={`${app.resourceUri}-${app.mcpServer}`} |
crates/goose/src/goose_apps/mod.rs
Outdated
| if let Some(cache) = cache { | ||
| let active_extensions: HashSet<String> = ui_resources | ||
| .iter() | ||
| .map(|(ext_name, _)| ext_name.clone()) | ||
| .collect(); | ||
|
|
||
| for extension_name in active_extensions { | ||
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | ||
| warn!( | ||
| "Failed to clean cache for extension {}: {}", | ||
| extension_name, e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This cache cleanup logic deletes cache entries for all active extensions before immediately re-fetching and re-caching them. This defeats the caching optimization. The cleanup should instead delete entries for extensions that are NO LONGER active (i.e., not in the active_extensions set).
| if let Some(cache) = cache { | |
| let active_extensions: HashSet<String> = ui_resources | |
| .iter() | |
| .map(|(ext_name, _)| ext_name.clone()) | |
| .collect(); | |
| for extension_name in active_extensions { | |
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | |
| warn!( | |
| "Failed to clean cache for extension {}: {}", | |
| extension_name, e | |
| ); | |
| } | |
| } | |
| } |
|
|
||
| return ( | ||
| <div | ||
| style={{ | ||
| width: '100vw', | ||
| height: '100vh', | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| }} | ||
| > | ||
| <p style={{ color: 'var(--text-muted, #6b7280)' }}>Initializing app...</p> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The final return statement (lines 167-179) is unreachable code. At this point in the logic, either cachedHtml or sessionId must be set (since loading would be false), so the condition on line 153 would catch it.
| return ( | |
| <div | |
| style={{ | |
| width: '100vw', | |
| height: '100vh', | |
| display: 'flex', | |
| alignItems: 'center', | |
| justifyContent: 'center', | |
| }} | |
| > | |
| <p style={{ color: 'var(--text-muted, #6b7280)' }}>Initializing app...</p> | |
| </div> | |
| ); |
ui/desktop/src/main.ts
Outdated
| const standaloneUrl = | ||
| `${baseUrl}/#/standalone-app?` + | ||
| `resourceUri=${encodeURIComponent(gooseApp.resourceUri)}` + | ||
| `&extensionName=${encodeURIComponent(gooseApp.mcpServer || '')}` + |
There was a problem hiding this comment.
If gooseApp.mcpServer is empty/null, this will encode an empty string in the URL. The receiver code checks for 'undefined' string (line 24 in StandaloneAppView.tsx), but won't handle empty string. Consider checking mcpServer before launching or handling empty strings consistently.
| const standaloneUrl = | |
| `${baseUrl}/#/standalone-app?` + | |
| `resourceUri=${encodeURIComponent(gooseApp.resourceUri)}` + | |
| `&extensionName=${encodeURIComponent(gooseApp.mcpServer || '')}` + | |
| const extensionName = | |
| gooseApp.mcpServer === undefined || | |
| gooseApp.mcpServer === null || | |
| gooseApp.mcpServer === '' | |
| ? 'undefined' | |
| : gooseApp.mcpServer; | |
| const standaloneUrl = | |
| `${baseUrl}/#/standalone-app?` + | |
| `resourceUri=${encodeURIComponent(gooseApp.resourceUri)}` + | |
| `&extensionName=${encodeURIComponent(extensionName)}` + |
| goosedClients.delete(appWindow.id); | ||
| }); | ||
|
|
||
| const workingDir = app.getPath('home'); |
There was a problem hiding this comment.
Using the home directory as the working directory for all standalone apps is incorrect. The working directory should come from the launching window's session, not default to the user's home directory. This could cause apps to operate in the wrong directory context.
| useEffect(() => { | ||
| return () => { | ||
| if (sessionId) { | ||
| stopAgent({ | ||
| body: { session_id: sessionId }, | ||
| throwOnError: false, | ||
| }).catch((err: unknown) => { | ||
| console.warn('Failed to stop agent on unmount:', err); | ||
| }); | ||
| } | ||
| }; | ||
| }, [sessionId]); |
There was a problem hiding this comment.
The cleanup effect creates a new session on every render when sessionId changes, potentially leaving multiple dangling sessions. Each standalone app window creates its own session, but if the sessionId changes, the old session won't be cleaned up by this effect since it only has access to the current sessionId value.
| if (error) { | ||
| return ( | ||
| <MainPanelLayout> | ||
| <div className="flex flex-col items-center justify-center h-64 text-center"> | ||
| <p className="text-red-500 mb-4">Error loading apps: {error}</p> | ||
| <Button onClick={loadApps}>Retry</Button> | ||
| </div> | ||
| </MainPanelLayout> | ||
| ); |
There was a problem hiding this comment.
The error UI shows even when there are successfully loaded cached apps, because the error state is set independently of the apps state. If an app launch fails or a session refresh fails, it will hide all apps and only show the error message, even though cached apps are available and could be displayed.
| let session_id = params.session_id.ok_or_else(|| ErrorResponse { | ||
| message: "Missing session_id for list_apps request".to_string(), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; |
There was a problem hiding this comment.
The error handling is unreachable. Line 967 checks if session_id is None after line 958 already returned early when session_id is None, making the error case impossible to reach.
|
@DOsinga got a demo app to poke around with you like to show? |
ui/desktop/src/api/types.gen.ts
Outdated
| export type GooseApp = { | ||
| description?: string | null; | ||
| height?: number | null; | ||
| html: string; | ||
| mcpServer?: string | null; | ||
| name: string; | ||
| resizable?: boolean | null; | ||
| resourceUri: string; | ||
| width?: number | null; | ||
| }; |
There was a problem hiding this comment.
For awareness, MCP Apps are conforming to MCP Resources schema via UIResource, see UIResource documented here.
Is GooseApp a superset of UIResource? If so, can we align our type definition to conform?
I think properties like height, mcpServer (can we infer this?), width, resizable probably belong under a _meta.ui.goose key... or defined in a smart way that work well with UIResource.
... and properties like description, name, html (as text or blob) are already standard fields in the MCP resource schema.
There was a problem hiding this comment.
this is a good point. the GooseApp struct is inherited from a previous version of GooseApps that predates the McpApp story. it does need the mcpServer so it can make toolcalls back (an embedded mcp app is inside a tool result, so it knows what mcp server created it). I would like a future where an MCP app can have more than one mcp server actually
|
@michaelneale, once you connect to an MCP server with apps (I used https://mcp-app-bench.onrender.com/mcp), their app resources appear. See screenshot — I have one MCP server that I've set up as a bench testing tool, and all the relevant UI resources show up as standalone apps. It’s not exactly my mental model of what an MCP App is, but it’s still pretty cool! This raises the question: should we ship a very simple goose app so users can try it out? Maybe a clock app? 
|
|
I have a standard stock quote app that I use. it currently has the access token baked in, so need to clean that up, but it allows you to get, eh, stock quotes. we could quickly publish a little clock thing too - it wouldn't really do all that much in terms of toolcalling of course (timezone?) but how we store that is still left as an exercise for the reader here our we do this as is and quickly follow up with a goose platform MPC that handles this elegantly. including a clock in that would work easy |
just whipped up some HTML, clock.html goose-app-clock.mov |
would be pretty sweet as well |
| }; | ||
|
|
||
| checkApps(); | ||
| }, [currentPath]); |
There was a problem hiding this comment.
The dependency on currentPath for the checkApps effect (line 133) doesn't make sense. The effect checks if apps are available to decide whether to show the Apps menu item, but it re-runs every time the path changes. This should either run once on mount, or have logic that updates when apps are actually added/removed (e.g., when extensions change). Re-running on every path change is inefficient.
| }, [currentPath]); | |
| }, []); |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [sessionId]); |
There was a problem hiding this comment.
The refreshApps effect (lines 50-73) has a missing dependency on apps.length but uses it in the error handling logic (line 65). The eslint-disable comment suppresses this warning, but the effect could reference stale values of apps. Either add apps.length to dependencies or restructure to avoid the closure issue.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [sessionId]); | |
| }, [sessionId, apps.length]); |
| let session_id = params.session_id.ok_or_else(|| ErrorResponse { | ||
| message: "Missing session_id for list_apps request".to_string(), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; |
There was a problem hiding this comment.
The logic at lines 958-964 checks if params.session_id.is_none() and returns cached apps, but then at line 966 tries to unwrap params.session_id with ok_or_else. Since we already returned if session_id is None, this error case at line 966-969 is unreachable dead code and should be removed.
| let session_id = params.session_id.ok_or_else(|| ErrorResponse { | |
| message: "Missing session_id for list_apps request".to_string(), | |
| status: StatusCode::BAD_REQUEST, | |
| })?; | |
| let session_id = params.session_id.unwrap(); |
crates/goose/src/goose_apps/mod.rs
Outdated
| pub async fn list_mcp_apps( | ||
| extension_manager: &ExtensionManager, | ||
| ) -> Result<Vec<GooseApp>, ErrorData> { | ||
| list_mcp_apps_with_cache(extension_manager, None).await | ||
| } | ||
|
|
||
| pub async fn list_mcp_apps_with_cache( | ||
| extension_manager: &ExtensionManager, | ||
| cache: Option<&McpAppCache>, | ||
| ) -> Result<Vec<GooseApp>, ErrorData> { | ||
| let mut apps = Vec::new(); | ||
|
|
||
| let ui_resources = extension_manager.get_ui_resources().await?; | ||
|
|
||
| if let Some(cache) = cache { | ||
| let active_extensions: HashSet<String> = ui_resources | ||
| .iter() | ||
| .map(|(ext_name, _)| ext_name.clone()) | ||
| .collect(); | ||
|
|
||
| for extension_name in active_extensions { | ||
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | ||
| warn!( | ||
| "Failed to clean cache for extension {}: {}", | ||
| extension_name, e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (extension_name, resource) in ui_resources { | ||
| match extension_manager | ||
| .read_resource(&resource.uri, &extension_name, CancellationToken::default()) | ||
| .await | ||
| { | ||
| Ok(read_result) => { | ||
| let mut html = String::new(); | ||
| for content in read_result.contents { | ||
| if let rmcp::model::ResourceContents::TextResourceContents { text, .. } = | ||
| content | ||
| { | ||
| html = text; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if !html.is_empty() { | ||
| let mcp_resource = McpAppResource { | ||
| uri: resource.uri.clone(), | ||
| name: format_resource_name(resource.name.clone()), | ||
| description: resource.description.clone(), | ||
| mime_type: "text/html;profile=mcp-app".to_string(), | ||
| text: Some(html), | ||
| blob: None, | ||
| meta: None, | ||
| }; | ||
|
|
||
| let app = GooseApp { | ||
| resource: mcp_resource, | ||
| mcp_server: Some(extension_name), | ||
| window_props: Some(WindowProps { | ||
| width: 800, | ||
| height: 600, | ||
| resizable: true, | ||
| }), | ||
| }; | ||
|
|
||
| if let Some(cache) = cache { | ||
| if let Err(e) = cache.cache_app(&app) { | ||
| warn!("Failed to cache app {}: {}", app.resource.name, e); | ||
| } | ||
| } | ||
|
|
||
| apps.push(app); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| warn!( | ||
| "Failed to read resource {} from {}: {}", | ||
| resource.uri, extension_name, e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(apps) | ||
| } | ||
|
|
||
| fn format_resource_name(name: String) -> String { | ||
| name.replace('_', " ") | ||
| .split_whitespace() | ||
| .map(|word| { | ||
| let mut chars = word.chars(); | ||
| match chars.next() { | ||
| None => String::new(), | ||
| Some(first) => first.to_uppercase().chain(chars).collect(), | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| } |
There was a problem hiding this comment.
The new goose_apps module lacks test coverage. The codebase has comprehensive test coverage for other modules in the goose crate, but this new functionality for listing, caching, and managing MCP apps has no tests. Consider adding tests for cache operations (caching, retrieval, deletion), the cache key generation logic, and the list_mcp_apps_with_cache function.
crates/goose/src/goose_apps/mod.rs
Outdated
| if let Some(cache) = cache { | ||
| let active_extensions: HashSet<String> = ui_resources | ||
| .iter() | ||
| .map(|(ext_name, _)| ext_name.clone()) | ||
| .collect(); | ||
|
|
||
| for extension_name in active_extensions { | ||
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | ||
| warn!( | ||
| "Failed to clean cache for extension {}: {}", | ||
| extension_name, e | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The cache cleanup logic is inverted. Currently, when listing apps, the code deletes cache entries for active extensions (lines 159-166). This means apps from currently-running extensions get their cache cleared, while stale cache from removed/inactive extensions persists indefinitely. The logic should delete cache entries for extensions that are NOT in the active set.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [resourceUri, extensionName, workingDir]); |
There was a problem hiding this comment.
The dependency array for the initSession effect is missing cachedHtml but the effect uses it in the error handling (line 88). This could cause the error state logic to reference a stale value of cachedHtml. Either add cachedHtml to the dependency array or restructure the logic.
ui/desktop/src/main.ts
Outdated
| const extensionName = | ||
| gooseApp.mcpServer === undefined || gooseApp.mcpServer === null || gooseApp.mcpServer === '' | ||
| ? 'undefined' | ||
| : gooseApp.mcpServer; |
There was a problem hiding this comment.
The condition gooseApp.mcpServer === '' is redundant because empty strings are falsy, and checking for empty string after checking for undefined/null provides no additional value. Simplify to !gooseApp.mcpServer.
| const extensionName = | |
| gooseApp.mcpServer === undefined || gooseApp.mcpServer === null || gooseApp.mcpServer === '' | |
| ? 'undefined' | |
| : gooseApp.mcpServer; | |
| const extensionName = !gooseApp.mcpServer ? 'undefined' : gooseApp.mcpServer; |
crates/goose/src/goose_apps/mod.rs
Outdated
| if let Some(cache) = cache { | ||
| let active_extensions: HashSet<String> = ui_resources | ||
| .iter() | ||
| .map(|(ext_name, _)| ext_name.clone()) | ||
| .collect(); | ||
|
|
||
| for extension_name in active_extensions { | ||
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | ||
| warn!( | ||
| "Failed to clean cache for extension {}: {}", | ||
| extension_name, e | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The cache cleaning logic deletes apps for active extensions, but should delete apps for inactive extensions. This will clear cached apps right before fetching fresh ones, defeating the cache optimization. Store previously cached extensions and only delete those no longer present.
| if let Some(cache) = cache { | |
| let active_extensions: HashSet<String> = ui_resources | |
| .iter() | |
| .map(|(ext_name, _)| ext_name.clone()) | |
| .collect(); | |
| for extension_name in active_extensions { | |
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | |
| warn!( | |
| "Failed to clean cache for extension {}: {}", | |
| extension_name, e | |
| ); | |
| } | |
| } | |
| } |
| }; | ||
|
|
||
| refreshApps(); | ||
| }, [sessionId, apps.length]); |
There was a problem hiding this comment.
Including apps.length in dependency array causes infinite re-fetch loop - refreshApps updates apps, which changes apps.length, triggering another refresh. Remove apps.length from dependencies.
| }, [sessionId, apps.length]); | |
| }, [sessionId]); |
ui/desktop/src/main.ts
Outdated
| }); | ||
|
|
||
| const workingDir = app.getPath('home'); | ||
| const extensionName = !gooseApp.mcpServer ? 'undefined' : gooseApp.mcpServer; |
There was a problem hiding this comment.
Setting extensionName to string 'undefined' when mcpServer is missing creates ambiguous state - the string 'undefined' !== undefined. Use nullish coalescing instead or handle the null case explicitly in StandaloneAppView validation.
| const extensionName = !gooseApp.mcpServer ? 'undefined' : gooseApp.mcpServer; | |
| const extensionName = gooseApp.mcpServer ?? ''; |
| <McpAppRenderer | ||
| resourceUri={resourceUri!} | ||
| extensionName={extensionName!} | ||
| sessionId={sessionId || 'loading'} |
There was a problem hiding this comment.
Passing string 'loading' as sessionId is a sentinel value that McpAppRenderer must handle specially (line 64). Consider using null/undefined and making sessionId optional in the component props instead.
| width: gooseApp.width ?? 800, | ||
| height: gooseApp.height ?? 600, | ||
| resizable: gooseApp.resizable ?? true, |
There was a problem hiding this comment.
The GooseApp type is defined as McpAppResource & (WindowProps | null), which means width, height, and resizable could be undefined when WindowProps is null. Accessing these fields directly without optional chaining could lead to runtime errors.
Consider updating the access pattern to handle the nested structure properly, or add validation that WindowProps exists before accessing.
| // Fetch fresh apps from MCP servers | ||
| let session_id = params.session_id.ok_or_else(|| ErrorResponse { | ||
| message: "Missing session_id for list_apps request".to_string(), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; |
There was a problem hiding this comment.
The session_id is first used in the conditional at line 973, then extracted again with ok_or_else at line 982. This second extraction is unreachable because if params.session_id.is_none() is true, the function returns early at line 978. Consider simplifying by using unwrap() or restructuring the logic to avoid the redundant error handling.
| // Fetch fresh apps from MCP servers | |
| let session_id = params.session_id.ok_or_else(|| ErrorResponse { | |
| message: "Missing session_id for list_apps request".to_string(), | |
| status: StatusCode::BAD_REQUEST, | |
| })?; | |
| // Fetch fresh apps from MCP servers; at this point session_id must be present | |
| let session_id = params.session_id.unwrap(); |
| await resumeAgent({ | ||
| body: { | ||
| session_id: sid, | ||
| load_model_and_extensions: true, |
There was a problem hiding this comment.
Loading model and extensions for every standalone app window creates unnecessary overhead. Consider a lightweight session mode that only loads the required extension for the specific app being launched, or reuse an existing session from the parent window.
| load_model_and_extensions: true, | |
| load_model_and_extensions: false, |
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [sessionId, apps.length]); |
There was a problem hiding this comment.
The apps.length dependency causes this useCallback to be recreated every time the apps array changes, defeating the purpose of memoization. Remove apps.length from dependencies and check it inside the function body if needed.
| if let Some(ref extension_name) = app.mcp_server { | ||
| let cache_key = Self::cache_key(extension_name, &app.resource.uri); | ||
| let app_path = self.cache_dir.join(format!("{}.json", cache_key)); | ||
| let json = serde_json::to_string_pretty(app).map_err(std::io::Error::other)?; |
There was a problem hiding this comment.
Using to_string_pretty for cache storage wastes disk space. Use serde_json::to_string instead since cache files don't need human readability.
| let json = serde_json::to_string_pretty(app).map_err(std::io::Error::other)?; | |
| let json = serde_json::to_string(app).map_err(std::io::Error::other)?; |
| let active_extensions: std::collections::HashSet<String> = apps | ||
| .iter() | ||
| .filter_map(|app| app.mcp_server.clone()) | ||
| .collect(); | ||
|
|
||
| for extension_name in active_extensions { | ||
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | ||
| warn!( | ||
| "Failed to clean cache for extension {}: {}", |
There was a problem hiding this comment.
This logic deletes all cached apps for extensions that have active apps, then immediately re-caches them. This clears the cache even when nothing changed. Only delete cached apps for extensions that are no longer present, or compare cached vs fresh apps to update selectively.
| let active_extensions: std::collections::HashSet<String> = apps | |
| .iter() | |
| .filter_map(|app| app.mcp_server.clone()) | |
| .collect(); | |
| for extension_name in active_extensions { | |
| if let Err(e) = cache.delete_extension_apps(&extension_name) { | |
| warn!( | |
| "Failed to clean cache for extension {}: {}", | |
| // Determine which extensions are currently active based on freshly fetched apps. | |
| let active_extensions: std::collections::HashSet<String> = apps | |
| .iter() | |
| .filter_map(|app| app.mcp_server.clone()) | |
| .collect(); | |
| // Determine which extensions are present in the cache. | |
| let cached_apps = cache.list_apps().unwrap_or_default(); | |
| let cached_extensions: std::collections::HashSet<String> = cached_apps | |
| .iter() | |
| .filter_map(|app| app.mcp_server.clone()) | |
| .collect(); | |
| // Only delete cached apps for extensions that are no longer active. | |
| for extension_name in cached_extensions.difference(&active_extensions) { | |
| if let Err(e) = cache.delete_extension_apps(extension_name) { | |
| warn!( | |
| "Failed to clean cache for stale extension {}: {}", |
| return () => { | ||
| if (sessionId) { | ||
| stopAgent({ | ||
| body: { session_id: sessionId }, | ||
| throwOnError: false, | ||
| }).catch((err: unknown) => { | ||
| console.warn('Failed to stop agent on unmount:', err); | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Session cleanup happens asynchronously but component unmounts immediately, potentially leaving orphaned sessions if the window closes quickly. The stopAgent call should be awaited, or the window close should be prevented until cleanup completes.
| return () => { | |
| if (sessionId) { | |
| stopAgent({ | |
| body: { session_id: sessionId }, | |
| throwOnError: false, | |
| }).catch((err: unknown) => { | |
| console.warn('Failed to stop agent on unmount:', err); | |
| }); | |
| } | |
| }; | |
| if (!sessionId) { | |
| return; | |
| } | |
| const cleanupSession = async () => { | |
| try { | |
| await stopAgent({ | |
| body: { session_id: sessionId }, | |
| throwOnError: false, | |
| }); | |
| } catch (err: unknown) { | |
| console.warn('Failed to stop agent on unmount:', err); | |
| } | |
| }; | |
| return () => { | |
| void cleanupSession(); | |
| }; |
| fn format_resource_name(name: String) -> String { | ||
| name.replace('_', " ") | ||
| .split_whitespace() | ||
| .map(|word| { | ||
| let mut chars = word.chars(); | ||
| match chars.next() { | ||
| None => String::new(), | ||
| Some(first) => first.to_uppercase().chain(chars).collect(), | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(" ") | ||
| } |
There was a problem hiding this comment.
This function implements title case conversion but doesn't handle edge cases like acronyms or already-capitalized words well. Consider using a library like heck for more robust case conversion, or document the expected input format.
|
I refactored this a bit, taking the mcp resource thing in as @aharvard suggested - my preference would be to merge this soon-ish and quickly follow up with step 4 of the plan. thoughts? we should also think about how we do state management for these. I'm thinking something like, we associate a session with each app and it always renders the last toolcall in that session and then there is a config box which all it does is send a message to the agent asking for a toolcall. that way if you had like @aharvard 's clock example, the toocall could set the type of clock and if you hit configure you could just tell goose, show me a swiss clock type of thing |
@DOsinga, agreed — and I would like to consider us refining how the apps page presents stuff to users... we can iterate on that layout easily enough over time
I think i follow, last tool call makes sense... also FYI context mutation is something I'd like to land for basic MCP Apps soonish... might want to leverage that as well somehow #6472
yeah, worth exploring statefullness w/ local storage as well (how I did it in my clock HTML) |
* 'main' of github.com:block/goose: (28 commits) chore(deps): bump aiohttp from 3.13.0 to 3.13.3 in /scripts/provider-error-proxy (#6539) chore(deps): bump brotli from 1.1.0 to 1.2.0 in /scripts/provider-error-proxy (#6538) docs: temp correction for agent directory (#6544) chore: upgrade rmcp (#6516) docs: clarify directory in /documentation readme (#6541) Release 1.20.0 Standalone mcp apps (#6458) don't add escaping to the command field (#6519) Fix popular topics not starting chat when clicked (#6508) fix[desktop]: deeplink ui repeat on refresh (#6469) fixed test compilation on main branch (#6512) fix: correctly parse extension name from tool call for MCP apps (#6482) fixed 0 token in openrouter steaming (#6493) feat(goose-acp): enable parallel sessions with isolated agent state (#6392) copilot instruction to flag prelease docs (#6504) docs: acp mcp support (#6491) feat: add flatpak support for linux (#6387) fix(code_execution): serialize record_result output as JSON (#6495) perf(google): avoid accumulating thoughtSignatures across conversation history (#6462) fix(openai): make tool_call arguments optional and fix silent stream termination (#6309) ...
Summary
makes it possible to launch standalone apps that are declared as MCP servers