From ec9acb1da8e64e4755789387f6b2e3240a0f31b0 Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Thu, 19 Feb 2026 18:48:48 +0800 Subject: [PATCH 1/6] feat: wire up HTTP definition headers orthogonally from auth, remove uninstall confirm - Headers from server definitions are now always applied as default_headers on the reqwest::Client regardless of auth strategy (OAuth, PAT, or no-auth) - If headers contain an Authorization header, OAuth is skipped entirely - If headers have no Authorization, they ride alongside OAuth tokens - Add build_default_headers() and build_http_client() helpers - Remove standalone connect_with_headers() in favor of orthogonal design - Add 19 unit tests for HttpTransport (requires_oauth, header building, connect routing logic, transport_type, description) - Remove broken confirm() dialog from server uninstall action Signed-off-by: Mohammod Al Amin Ashik --- .../src/features/servers/ServersPage.tsx | 10 +- .../mcpmux-gateway/src/pool/transport/http.rs | 582 +++++++++++++++++- 2 files changed, 559 insertions(+), 33 deletions(-) diff --git a/apps/desktop/src/features/servers/ServersPage.tsx b/apps/desktop/src/features/servers/ServersPage.tsx index 07ad188e..599e2ff0 100644 --- a/apps/desktop/src/features/servers/ServersPage.tsx +++ b/apps/desktop/src/features/servers/ServersPage.tsx @@ -709,15 +709,9 @@ export function ServersPage() { }; const handleUninstall = async (server: ServerViewModel) => { - // Import source-aware helpers - const { getUninstallConfirmMessage, getUninstallLabel } = await import('@/components/SourceBadge'); - const confirmMsg = getUninstallConfirmMessage(server.name, server.installation_source); + const { getUninstallLabel } = await import('@/components/SourceBadge'); const actionLabel = getUninstallLabel(server.installation_source); - - if (!confirm(confirmMsg)) { - return; - } - + setActionLoading(`uninstall-${server.id}`); try { const { uninstallServer } = await import('@/lib/api/registry'); diff --git a/crates/mcpmux-gateway/src/pool/transport/http.rs b/crates/mcpmux-gateway/src/pool/transport/http.rs index d1e99ba8..250f0046 100644 --- a/crates/mcpmux-gateway/src/pool/transport/http.rs +++ b/crates/mcpmux-gateway/src/pool/transport/http.rs @@ -29,7 +29,6 @@ use crate::pool::credential_store::DatabaseCredentialStore; /// automatically refreshed by RMCP on every request when needed. pub struct HttpTransport { url: String, - #[allow(dead_code)] // Reserved for future custom headers headers: HashMap, space_id: Uuid, server_id: String, @@ -99,7 +98,7 @@ impl HttpTransport { oauth_indicators.iter().any(|s| error_lower.contains(s)) } - /// Connect with OAuth using DatabaseCredentialStore. + /// Connect with OAuth using DatabaseCredentialStore (with definition headers if any). /// /// RMCP's AuthClient will automatically: /// - Load tokens from the credential store @@ -107,11 +106,18 @@ impl HttpTransport { /// - Save refreshed tokens back to the store /// - Add auth header to every request /// + /// Definition headers are applied as default_headers on the underlying reqwest::Client, + /// so they're sent alongside OAuth tokens on every request. + /// /// If RMCP's metadata discovery fails (non-spec-compliant servers), we use /// stored metadata from the initial OAuth flow. - async fn connect_with_auth(&self) -> TransportConnectResult { + async fn connect_with_auth( + &self, + header_map: reqwest::header::HeaderMap, + ) -> TransportConnectResult { debug!( server_id = %self.server_id, + header_count = header_map.len(), "Connecting with OAuth via CredentialStore" ); @@ -119,8 +125,9 @@ impl HttpTransport { LogLevel::Info, LogSource::HttpRequest, format!( - "Connecting to {} with OAuth (auto-refresh enabled)", - self.url + "Connecting to {} with OAuth (auto-refresh enabled, {} custom header(s))", + self.url, + header_map.len() ), ) .await; @@ -234,12 +241,17 @@ impl HttpTransport { ) .await; - return self.connect_with_manual_token().await; + return self.connect_with_manual_token(header_map).await; } } - // Create AuthClient - this wraps reqwest::Client with automatic token injection & refresh - let auth_client = AuthClient::new(reqwest::Client::default(), auth_manager); + // Create AuthClient - wraps reqwest::Client with automatic token injection & refresh. + // Definition headers are baked into the client so they're sent on every request. + let base_client = match self.build_http_client(header_map) { + Ok(c) => c, + Err(err) => return TransportConnectResult::Failed(err), + }; + let auth_client = AuthClient::new(base_client, auth_manager); let transport_config = StreamableHttpClientTransportConfig::with_uri(self.url.as_str()); let transport = StreamableHttpClientTransport::with_client(auth_client, transport_config); @@ -303,8 +315,12 @@ impl HttpTransport { /// /// Some servers (like Cloudflare) don't serve OAuth metadata at the standard location /// that RMCP expects. In this case, we manually inject the stored token into requests. + /// Definition headers are merged in (token Authorization header takes precedence). /// NOTE: Auto-refresh won't work in this mode - tokens must be refreshed manually. - async fn connect_with_manual_token(&self) -> TransportConnectResult { + async fn connect_with_manual_token( + &self, + mut header_map: reqwest::header::HeaderMap, + ) -> TransportConnectResult { debug!( server_id = %self.server_id, "Connecting with manual token injection (RMCP metadata failed)" @@ -341,12 +357,11 @@ impl HttpTransport { ) .await; - // Build HTTP client with Authorization header - let mut headers = reqwest::header::HeaderMap::new(); + // Add Authorization header to the definition headers (overrides if already present) let auth_value = format!("Bearer {}", access_token); match reqwest::header::HeaderValue::from_str(&auth_value) { Ok(val) => { - headers.insert(reqwest::header::AUTHORIZATION, val); + header_map.insert(reqwest::header::AUTHORIZATION, val); } Err(e) => { let err = format!("Invalid token format: {}", e); @@ -355,13 +370,9 @@ impl HttpTransport { } } - let client = match reqwest::Client::builder().default_headers(headers).build() { + let client = match self.build_http_client(header_map) { Ok(c) => c, - Err(e) => { - let err = format!("Failed to build HTTP client: {}", e); - error!(server_id = %self.server_id, "{}", err); - return TransportConnectResult::Failed(err); - } + Err(err) => return TransportConnectResult::Failed(err), }; let transport_config = StreamableHttpClientTransportConfig::with_uri(self.url.as_str()); @@ -423,21 +434,74 @@ impl HttpTransport { } } - /// Try connecting without authentication - async fn connect_without_auth(&self) -> TransportConnectResult { + /// Build a reqwest HeaderMap from definition-provided headers. + /// + /// These headers (resolved from `${input:ID}` placeholders) are always applied + /// to the HTTP client regardless of auth strategy. Returns an empty map if no + /// definition headers are configured. + fn build_default_headers(&self) -> Result { + let mut header_map = reqwest::header::HeaderMap::new(); + for (key, value) in &self.headers { + let header_name = + reqwest::header::HeaderName::from_bytes(key.as_bytes()).map_err(|e| { + let err = format!("Invalid header name '{}': {}", key, e); + error!(server_id = %self.server_id, "{}", err); + err + })?; + let header_value = reqwest::header::HeaderValue::from_str(value).map_err(|e| { + let err = format!("Invalid header value for '{}': {}", key, e); + error!(server_id = %self.server_id, "{}", err); + err + })?; + header_map.insert(header_name, header_value); + } + Ok(header_map) + } + + /// Build a reqwest::Client with definition headers as default_headers. + fn build_http_client( + &self, + header_map: reqwest::header::HeaderMap, + ) -> Result { + reqwest::Client::builder() + .default_headers(header_map) + .build() + .map_err(|e| { + let err = format!("Failed to build HTTP client: {}", e); + error!(server_id = %self.server_id, "{}", err); + err + }) + } + + /// Try connecting without authentication (but with definition headers if any) + async fn connect_without_auth( + &self, + header_map: reqwest::header::HeaderMap, + ) -> TransportConnectResult { debug!( server_id = %self.server_id, + header_count = header_map.len(), "Trying connection without auth" ); self.log( LogLevel::Info, LogSource::HttpRequest, - format!("Connecting to {} without auth", self.url), + format!( + "Connecting to {} without auth ({} custom header(s))", + self.url, + header_map.len() + ), ) .await; - let transport = StreamableHttpClientTransport::from_uri(self.url.as_str()); + let client = match self.build_http_client(header_map) { + Ok(c) => c, + Err(err) => return TransportConnectResult::Failed(err), + }; + + let transport_config = StreamableHttpClientTransportConfig::with_uri(self.url.as_str()); + let transport = StreamableHttpClientTransport::with_client(client, transport_config); let client_handler = create_client_handler( &self.server_id, self.space_id, @@ -519,7 +583,33 @@ impl Transport for HttpTransport { return TransportConnectResult::Failed(err); } - // Check if we have stored credentials for this server + // Build definition headers (always applied regardless of auth strategy) + let header_map = match self.build_default_headers() { + Ok(h) => h, + Err(err) => return TransportConnectResult::Failed(err), + }; + + if !header_map.is_empty() { + info!( + server_id = %self.server_id, + header_count = header_map.len(), + "Applying definition-provided headers to connection" + ); + } + + // Check if definition headers already include an Authorization header. + // If so, skip OAuth — the user explicitly provided auth via the definition (e.g., PAT). + let has_explicit_auth = header_map.contains_key(reqwest::header::AUTHORIZATION); + + if has_explicit_auth { + info!( + server_id = %self.server_id, + "Definition includes Authorization header, skipping OAuth" + ); + return self.connect_without_auth(header_map).await; + } + + // No explicit auth in headers — check for stored OAuth credentials let has_credentials = self .credential_repo .get( @@ -537,13 +627,13 @@ impl Transport for HttpTransport { server_id = %self.server_id, "Found stored credentials, connecting with OAuth (auto-refresh enabled)" ); - self.connect_with_auth().await + self.connect_with_auth(header_map).await } else { debug!( server_id = %self.server_id, "No stored credentials, trying without auth" ); - self.connect_without_auth().await + self.connect_without_auth(header_map).await } } @@ -555,3 +645,445 @@ impl Transport for HttpTransport { format!("http:{}", self.url) } } + +#[cfg(test)] +mod tests { + use super::*; + use mcpmux_core::{Credential, CredentialType, OutboundOAuthRegistration}; + + // ── Mock repos (minimal, sufficient for HttpTransport unit tests) ── + + #[derive(Clone)] + struct MockCredentialRepo { + credentials: Arc>>, + } + + impl MockCredentialRepo { + fn new() -> Self { + Self { + credentials: Arc::new(tokio::sync::RwLock::new(Vec::new())), + } + } + + fn with_credential(cred: Credential) -> Self { + Self { + credentials: Arc::new(tokio::sync::RwLock::new(vec![cred])), + } + } + } + + #[async_trait] + impl CredentialRepository for MockCredentialRepo { + async fn get( + &self, + space_id: &Uuid, + server_id: &str, + credential_type: &CredentialType, + ) -> anyhow::Result> { + let creds = self.credentials.read().await; + Ok(creds + .iter() + .find(|c| { + c.space_id == *space_id + && c.server_id == server_id + && c.credential_type == *credential_type + }) + .cloned()) + } + + async fn get_all( + &self, + space_id: &Uuid, + server_id: &str, + ) -> anyhow::Result> { + let creds = self.credentials.read().await; + Ok(creds + .iter() + .filter(|c| c.space_id == *space_id && c.server_id == server_id) + .cloned() + .collect()) + } + + async fn save(&self, credential: &Credential) -> anyhow::Result<()> { + let mut creds = self.credentials.write().await; + creds.retain(|c| { + !(c.space_id == credential.space_id + && c.server_id == credential.server_id + && c.credential_type == credential.credential_type) + }); + creds.push(credential.clone()); + Ok(()) + } + + async fn delete( + &self, + space_id: &Uuid, + server_id: &str, + credential_type: &CredentialType, + ) -> anyhow::Result<()> { + let mut creds = self.credentials.write().await; + creds.retain(|c| { + !(c.space_id == *space_id + && c.server_id == server_id + && c.credential_type == *credential_type) + }); + Ok(()) + } + + async fn delete_all(&self, space_id: &Uuid, server_id: &str) -> anyhow::Result<()> { + let mut creds = self.credentials.write().await; + creds.retain(|c| !(c.space_id == *space_id && c.server_id == server_id)); + Ok(()) + } + + async fn clear_tokens(&self, space_id: &Uuid, server_id: &str) -> anyhow::Result { + let mut creds = self.credentials.write().await; + let before = creds.len(); + creds.retain(|c| { + !(c.space_id == *space_id + && c.server_id == server_id + && c.credential_type.is_oauth()) + }); + Ok(creds.len() < before) + } + + async fn list_for_space(&self, space_id: &Uuid) -> anyhow::Result> { + let creds = self.credentials.read().await; + Ok(creds + .iter() + .filter(|c| c.space_id == *space_id) + .cloned() + .collect()) + } + } + + #[derive(Clone)] + struct MockOAuthRepo; + + #[async_trait] + impl OutboundOAuthRepository for MockOAuthRepo { + async fn get( + &self, + _space_id: &Uuid, + _server_id: &str, + ) -> anyhow::Result> { + Ok(None) + } + + async fn save(&self, _registration: &OutboundOAuthRegistration) -> anyhow::Result<()> { + Ok(()) + } + + async fn delete(&self, _space_id: &Uuid, _server_id: &str) -> anyhow::Result<()> { + Ok(()) + } + + async fn list_for_space( + &self, + _space_id: &Uuid, + ) -> anyhow::Result> { + Ok(vec![]) + } + } + + /// Helper to create an HttpTransport with given headers and credential repo. + fn make_transport( + headers: HashMap, + credential_repo: Arc, + ) -> HttpTransport { + HttpTransport::new( + "https://example.com/mcp".to_string(), + headers, + Uuid::new_v4(), + "test-server".to_string(), + credential_repo, + Arc::new(MockOAuthRepo), + None, + Duration::from_secs(10), + None, + ) + } + + fn make_transport_with_space( + headers: HashMap, + credential_repo: Arc, + space_id: Uuid, + server_id: &str, + ) -> HttpTransport { + HttpTransport::new( + "https://example.com/mcp".to_string(), + headers, + space_id, + server_id.to_string(), + credential_repo, + Arc::new(MockOAuthRepo), + None, + Duration::from_secs(10), + None, + ) + } + + // ── requires_oauth tests ── + + #[test] + fn test_requires_oauth_401() { + assert!(HttpTransport::requires_oauth("HTTP 401 Unauthorized")); + } + + #[test] + fn test_requires_oauth_bearer() { + assert!(HttpTransport::requires_oauth("Missing Bearer token")); + } + + #[test] + fn test_requires_oauth_www_authenticate() { + assert!(HttpTransport::requires_oauth("WWW-Authenticate: Bearer")); + } + + #[test] + fn test_requires_oauth_channel_closed() { + assert!(HttpTransport::requires_oauth("transport channel closed")); + } + + #[test] + fn test_requires_oauth_false_for_unrelated() { + assert!(!HttpTransport::requires_oauth("connection refused")); + assert!(!HttpTransport::requires_oauth("DNS lookup failed")); + assert!(!HttpTransport::requires_oauth("timeout")); + } + + // ── build_default_headers tests ── + + #[test] + fn test_build_default_headers_empty() { + let transport = make_transport(HashMap::new(), Arc::new(MockCredentialRepo::new())); + let headers = transport.build_default_headers().unwrap(); + assert!(headers.is_empty()); + } + + #[test] + fn test_build_default_headers_single() { + let mut h = HashMap::new(); + h.insert("Authorization".to_string(), "Bearer token123".to_string()); + let transport = make_transport(h, Arc::new(MockCredentialRepo::new())); + let headers = transport.build_default_headers().unwrap(); + + assert_eq!(headers.len(), 1); + assert_eq!( + headers.get(reqwest::header::AUTHORIZATION).unwrap(), + "Bearer token123" + ); + } + + #[test] + fn test_build_default_headers_multiple() { + let mut h = HashMap::new(); + h.insert("Authorization".to_string(), "Bearer pat_xxx".to_string()); + h.insert("X-Custom-Header".to_string(), "custom-value".to_string()); + let transport = make_transport(h, Arc::new(MockCredentialRepo::new())); + let headers = transport.build_default_headers().unwrap(); + + assert_eq!(headers.len(), 2); + assert_eq!( + headers.get(reqwest::header::AUTHORIZATION).unwrap(), + "Bearer pat_xxx" + ); + assert_eq!(headers.get("x-custom-header").unwrap(), "custom-value"); + } + + #[test] + fn test_build_default_headers_invalid_name() { + let mut h = HashMap::new(); + h.insert("Invalid Header\n".to_string(), "value".to_string()); + let transport = make_transport(h, Arc::new(MockCredentialRepo::new())); + let result = transport.build_default_headers(); + assert!(result.is_err()); + } + + #[test] + fn test_build_default_headers_invalid_value() { + let mut h = HashMap::new(); + h.insert("X-Header".to_string(), "bad\nvalue".to_string()); + let transport = make_transport(h, Arc::new(MockCredentialRepo::new())); + let result = transport.build_default_headers(); + assert!(result.is_err()); + } + + // ── build_http_client tests ── + + #[test] + fn test_build_http_client_empty_headers() { + let transport = make_transport(HashMap::new(), Arc::new(MockCredentialRepo::new())); + let client = transport.build_http_client(reqwest::header::HeaderMap::new()); + assert!(client.is_ok()); + } + + #[test] + fn test_build_http_client_with_headers() { + let transport = make_transport(HashMap::new(), Arc::new(MockCredentialRepo::new())); + let mut header_map = reqwest::header::HeaderMap::new(); + header_map.insert( + reqwest::header::AUTHORIZATION, + reqwest::header::HeaderValue::from_static("Bearer token"), + ); + let client = transport.build_http_client(header_map); + assert!(client.is_ok()); + } + + // ── connect() routing logic tests ── + + #[tokio::test] + async fn test_connect_invalid_url_fails() { + let transport = HttpTransport::new( + "not a valid url".to_string(), + HashMap::new(), + Uuid::new_v4(), + "test-server".to_string(), + Arc::new(MockCredentialRepo::new()), + Arc::new(MockOAuthRepo), + None, + Duration::from_secs(5), + None, + ); + + let result = transport.connect().await; + match result { + TransportConnectResult::Failed(msg) => { + assert!(msg.contains("Invalid URL"), "Got: {}", msg); + } + _ => panic!("Expected Failed for invalid URL"), + } + } + + #[tokio::test] + async fn test_connect_with_explicit_auth_header_skips_oauth_check() { + // When headers include Authorization, connect should NOT check credential_repo + // for OAuth tokens — it should go straight to connect_without_auth with headers. + // We verify this by giving it an unreachable server URL (connection will fail) + // and checking that the error is a connection failure, NOT OAuthRequired. + let mut h = HashMap::new(); + h.insert( + "Authorization".to_string(), + "Bearer ghp_testtoken123".to_string(), + ); + + let transport = HttpTransport::new( + "https://127.0.0.1:1/mcp".to_string(), // unreachable + h, + Uuid::new_v4(), + "test-server".to_string(), + Arc::new(MockCredentialRepo::new()), + Arc::new(MockOAuthRepo), + None, + Duration::from_secs(2), + None, + ); + + let result = transport.connect().await; + // Should be Failed (connection error) or timeout — NOT OAuthRequired + match result { + TransportConnectResult::Failed(_) => {} // expected + TransportConnectResult::OAuthRequired { .. } => { + panic!("Should not trigger OAuth when Authorization header is present") + } + _ => {} // Connected would be surprising but not wrong + } + } + + #[tokio::test] + async fn test_connect_no_headers_no_credentials_tries_no_auth() { + // No headers, no stored credentials → connect_without_auth + // Will fail to connect to unreachable server + let transport = HttpTransport::new( + "https://127.0.0.1:1/mcp".to_string(), + HashMap::new(), + Uuid::new_v4(), + "test-server".to_string(), + Arc::new(MockCredentialRepo::new()), + Arc::new(MockOAuthRepo), + None, + Duration::from_secs(2), + None, + ); + + let result = transport.connect().await; + // Should be Failed (connection error/timeout) or OAuthRequired (if 401 detected) + match result { + TransportConnectResult::Failed(_) | TransportConnectResult::OAuthRequired { .. } => {} + TransportConnectResult::Connected(_) => { + panic!("Should not connect to unreachable server") + } + } + } + + #[tokio::test] + async fn test_connect_with_stored_credentials_routes_to_oauth() { + // When stored credentials exist and no explicit Authorization header, + // connect should route to connect_with_auth (which will fail on unreachable server, + // but we verify it doesn't go to connect_without_auth by observing the error path). + let space_id = Uuid::new_v4(); + let server_id = "test-server"; + + let cred = Credential::access_token(space_id, server_id, "stored_token", None); + let cred_repo = Arc::new(MockCredentialRepo::with_credential(cred)); + + let transport = make_transport_with_space(HashMap::new(), cred_repo, space_id, server_id); + + let result = transport.connect().await; + // connect_with_auth will fail (can't reach server / AuthorizationManager::new fails) + // but it should NOT be OAuthRequired from the no-auth path's 401 detection + match result { + TransportConnectResult::Failed(_) | TransportConnectResult::OAuthRequired { .. } => {} + TransportConnectResult::Connected(_) => { + panic!("Should not connect to example.com MCP endpoint") + } + } + } + + #[tokio::test] + async fn test_connect_custom_headers_always_applied_with_credentials() { + // Even when we have stored OAuth credentials, custom (non-auth) headers + // from the definition should be applied. We can verify this indirectly: + // the transport should route to connect_with_auth (not connect_without_auth) + // because there's no Authorization in the custom headers. + let space_id = Uuid::new_v4(); + let server_id = "test-server"; + + let cred = Credential::access_token(space_id, server_id, "stored_token", None); + let cred_repo = Arc::new(MockCredentialRepo::with_credential(cred)); + + let mut h = HashMap::new(); + h.insert("X-MCP-Toolsets".to_string(), "tools-only".to_string()); + + let transport = make_transport_with_space(h, cred_repo, space_id, server_id); + + // Verify headers are built correctly (non-auth header present, no Authorization) + let headers = transport.build_default_headers().unwrap(); + assert_eq!(headers.len(), 1); + assert!(!headers.contains_key(reqwest::header::AUTHORIZATION)); + assert_eq!(headers.get("x-mcp-toolsets").unwrap(), "tools-only"); + + // connect() should route to connect_with_auth since no explicit Authorization header + let result = transport.connect().await; + match result { + TransportConnectResult::Failed(_) | TransportConnectResult::OAuthRequired { .. } => {} + TransportConnectResult::Connected(_) => { + panic!("Should not connect to example.com MCP endpoint") + } + } + } + + // ── transport_type / description tests ── + + #[test] + fn test_transport_type() { + let transport = make_transport(HashMap::new(), Arc::new(MockCredentialRepo::new())); + assert!(matches!(transport.transport_type(), TransportType::Http)); + } + + #[test] + fn test_description() { + let transport = make_transport(HashMap::new(), Arc::new(MockCredentialRepo::new())); + assert_eq!(transport.description(), "http:https://example.com/mcp"); + } +} From f43879a4ac873fbc003667a917807994dfbf2983 Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Tue, 24 Feb 2026 18:04:25 +0800 Subject: [PATCH 2/6] feat: add post-action UX guidance for install, approval, and empty states Improve user experience after key actions by guiding users to the next step instead of leaving them without direction. - My Servers empty state: replace plain text with gradient "Discover MCP Servers" button that navigates to the Discover tab - Discover page post-install: success toast now includes "Go to My Servers to enable" action link (servers install disabled by default) - OAuth consent post-approval: new success screen with "Manage Permissions" button guiding users to assign FeatureSets on Clients page - Move activeNav to Zustand store for cross-component navigation - Extend Toast component to support optional action buttons (backward compatible) Signed-off-by: Mohammod Al Amin Ashik --- apps/desktop/src/App.tsx | 23 ++-- .../src/components/OAuthConsentModal.tsx | 58 +++++++++- .../src/features/registry/RegistryPage.tsx | 11 +- .../src/features/servers/ServersPage.tsx | 13 ++- apps/desktop/src/stores/appStore.ts | 7 ++ apps/desktop/src/stores/selectors.ts | 2 + apps/desktop/src/stores/types.ts | 8 ++ packages/ui/src/components/common/Toast.tsx | 16 +++ packages/ui/src/hooks/useToast.ts | 9 +- packages/ui/src/index.ts | 2 +- tests/e2e/specs/post-action-guidance.spec.ts | 107 ++++++++++++++++++ tests/ts/components/ToastAction.test.tsx | 72 ++++++++++++ tests/ts/hooks/useToast.test.ts | 42 ++++++- tests/ts/stores/appStore.test.ts | 21 ++++ 14 files changed, 367 insertions(+), 24 deletions(-) create mode 100644 tests/e2e/specs/post-action-guidance.spec.ts create mode 100644 tests/ts/components/ToastAction.test.tsx diff --git a/apps/desktop/src/App.tsx b/apps/desktop/src/App.tsx index d98841ff..9521773f 100644 --- a/apps/desktop/src/App.tsx +++ b/apps/desktop/src/App.tsx @@ -35,7 +35,7 @@ import { ConnectIDEs } from '@/components/ConnectIDEs'; import { useDataSync } from '@/hooks/useDataSync'; import { useAnalytics } from '@/hooks/useAnalytics'; import { initAnalytics, capture, optIn, optOut } from '@/lib/analytics'; -import { useAppStore, useActiveSpace, useViewSpace, useTheme, useAnalyticsEnabled } from '@/stores'; +import { useAppStore, useActiveSpace, useViewSpace, useTheme, useAnalyticsEnabled, useActiveNav, useNavigateTo } from '@/stores'; import { RegistryPage } from '@/features/registry'; import { FeatureSetsPage } from '@/features/featuresets'; import { ClientsPage } from '@/features/clients'; @@ -80,13 +80,12 @@ function McpMuxGlyph({ className }: { className?: string }) { ); } -type NavItem = 'home' | 'registry' | 'servers' | 'spaces' | 'featuresets' | 'clients' | 'settings'; - function AppContent() { // Sync data from backend on mount useDataSync(); - const [activeNav, setActiveNav] = useState('home'); + const activeNav = useActiveNav(); + const navigateTo = useNavigateTo(); const [availableUpdate, setAvailableUpdate] = useState<{ version: string } | null>(null); // Auto-check for updates on startup (silent check after 5 seconds) @@ -199,21 +198,21 @@ function AppContent() { icon={} label="Dashboard" active={activeNav === 'home'} - onClick={() => setActiveNav('home')} + onClick={() => navigateTo('home')} data-testid="nav-dashboard" /> } label="My Servers" active={activeNav === 'servers'} - onClick={() => setActiveNav('servers')} + onClick={() => navigateTo('servers')} data-testid="nav-my-servers" /> } label="Discover" active={activeNav === 'registry'} - onClick={() => setActiveNav('registry')} + onClick={() => navigateTo('registry')} data-testid="nav-discover" /> @@ -223,14 +222,14 @@ function AppContent() { icon={} label="Spaces" active={activeNav === 'spaces'} - onClick={() => setActiveNav('spaces')} + onClick={() => navigateTo('spaces')} data-testid="nav-spaces" /> } label="FeatureSets" active={activeNav === 'featuresets'} - onClick={() => setActiveNav('featuresets')} + onClick={() => navigateTo('featuresets')} data-testid="nav-featuresets" /> @@ -240,7 +239,7 @@ function AppContent() { icon={} label="Clients" active={activeNav === 'clients'} - onClick={() => setActiveNav('clients')} + onClick={() => navigateTo('clients')} data-testid="nav-clients" /> @@ -250,7 +249,7 @@ function AppContent() { icon={} label="Settings" active={activeNav === 'settings'} - onClick={() => setActiveNav('settings')} + onClick={() => navigateTo('settings')} data-testid="nav-settings" /> @@ -316,7 +315,7 @@ function AppContent() { + + + + + + ); + } + // Consent state - show approval modal const { details } = modalState; const scopes = details.scope?.split(' ').filter(Boolean) || ['mcp']; diff --git a/apps/desktop/src/features/registry/RegistryPage.tsx b/apps/desktop/src/features/registry/RegistryPage.tsx index ebb5322f..dda4e672 100644 --- a/apps/desktop/src/features/registry/RegistryPage.tsx +++ b/apps/desktop/src/features/registry/RegistryPage.tsx @@ -10,7 +10,7 @@ import { useToast, ToastContainer } from '@mcpmux/ui'; import { useRegistryStore } from '../../stores/registryStore'; import { ServerCard } from './ServerCard'; import { ServerDetailModal } from './ServerDetailModal'; -import { useViewSpace } from '@/stores'; +import { useViewSpace, useNavigateTo } from '@/stores'; import { capture } from '@/lib/analytics'; export function RegistryPage() { @@ -39,6 +39,7 @@ export function RegistryPage() { const [localSearch, setLocalSearch] = useState(''); const viewSpace = useViewSpace(); + const navigateTo = useNavigateTo(); const { toasts, success, error: showToastError, dismiss } = useToast(); const itemsPerPage = uiConfig?.items_per_page ?? 24; @@ -105,7 +106,13 @@ export function RegistryPage() { const serverName = server?.name || 'Server'; try { await installServer(id, viewSpace?.id); - success('Server installed', `"${serverName}" has been installed`); + success('Server installed', `"${serverName}" has been installed`, { + duration: 6000, + action: { + label: 'Go to My Servers to enable →', + onClick: () => navigateTo('servers'), + }, + }); } catch { showToastError('Install failed', `Failed to install "${serverName}"`); } diff --git a/apps/desktop/src/features/servers/ServersPage.tsx b/apps/desktop/src/features/servers/ServersPage.tsx index 599e2ff0..0986dc89 100644 --- a/apps/desktop/src/features/servers/ServersPage.tsx +++ b/apps/desktop/src/features/servers/ServersPage.tsx @@ -26,7 +26,7 @@ import type { ServerFeature } from '@/lib/api/serverFeatures'; import { listServerFeaturesByServer } from '@/lib/api/serverFeatures'; import type { ConnectionStatus, ServerStatusResponse } from '@/lib/api/serverManager'; import { getServerStatuses as fetchServerStatuses } from '@/lib/api/serverManager'; -import { useViewSpace } from '@/stores'; +import { useViewSpace, useNavigateTo } from '@/stores'; import { useServerManager } from '@/hooks/useServerManager'; import { useGatewayEvents, useDomainEvents } from '@/hooks/useDomainEvents'; import type { GatewayChangedPayload, ServerChangedPayload } from '@/hooks/useDomainEvents'; @@ -192,7 +192,8 @@ export function ServersPage() { const [editConfigSpace, setEditConfigSpace] = useState<{ id: string; name: string } | null>(null); const viewSpace = useViewSpace(); - + const navigateTo = useNavigateTo(); + // Event-driven server status management const { statuses: serverStatuses, @@ -890,7 +891,13 @@ export function ServersPage() {
📦

No servers installed

-

Visit Discover to install MCP servers

+
) : (
diff --git a/apps/desktop/src/stores/appStore.ts b/apps/desktop/src/stores/appStore.ts index 7f5b1618..d9811d4e 100644 --- a/apps/desktop/src/stores/appStore.ts +++ b/apps/desktop/src/stores/appStore.ts @@ -7,6 +7,7 @@ const initialState: AppState = { spaces: [], activeSpaceId: null, viewSpaceId: null, + activeNav: 'home', sidebarCollapsed: false, theme: 'system', analyticsEnabled: true, @@ -85,6 +86,12 @@ export const useAppStore = create()( } }), + // Navigation + navigateTo: (nav) => + set((state) => { + state.activeNav = nav; + }), + // UI toggleSidebar: () => set((state) => { diff --git a/apps/desktop/src/stores/selectors.ts b/apps/desktop/src/stores/selectors.ts index 756b3219..2478c900 100644 --- a/apps/desktop/src/stores/selectors.ts +++ b/apps/desktop/src/stores/selectors.ts @@ -5,6 +5,8 @@ import { Space } from '@/lib/api/spaces'; export const useSpaces = () => useAppStore((state) => state.spaces); export const useActiveSpaceId = () => useAppStore((state) => state.activeSpaceId); export const useViewSpaceId = () => useAppStore((state) => state.viewSpaceId); +export const useActiveNav = () => useAppStore((state) => state.activeNav); +export const useNavigateTo = () => useAppStore((state) => state.navigateTo); export const useTheme = () => useAppStore((state) => state.theme); export const useSidebarCollapsed = () => useAppStore((state) => state.sidebarCollapsed); export const useAnalyticsEnabled = () => useAppStore((state) => state.analyticsEnabled); diff --git a/apps/desktop/src/stores/types.ts b/apps/desktop/src/stores/types.ts index 48686aae..6e87de87 100644 --- a/apps/desktop/src/stores/types.ts +++ b/apps/desktop/src/stores/types.ts @@ -1,11 +1,16 @@ import { Space } from '@/lib/api/spaces'; +export type NavItem = 'home' | 'registry' | 'servers' | 'spaces' | 'featuresets' | 'clients' | 'settings'; + export interface AppState { // Spaces spaces: Space[]; activeSpaceId: string | null; viewSpaceId: string | null; + // Navigation + activeNav: NavItem; + // UI state sidebarCollapsed: boolean; theme: 'light' | 'dark' | 'system'; @@ -27,6 +32,9 @@ export interface AppActions { removeSpace: (id: string) => void; updateSpace: (id: string, updates: Partial) => void; + // Navigation + navigateTo: (nav: NavItem) => void; + // UI toggleSidebar: () => void; setTheme: (theme: 'light' | 'dark' | 'system') => void; diff --git a/packages/ui/src/components/common/Toast.tsx b/packages/ui/src/components/common/Toast.tsx index a6cfac4e..d8c00c8d 100644 --- a/packages/ui/src/components/common/Toast.tsx +++ b/packages/ui/src/components/common/Toast.tsx @@ -4,12 +4,18 @@ import { cn } from '../../lib/cn'; export type ToastType = 'success' | 'error' | 'warning' | 'info'; +export interface ToastAction { + label: string; + onClick: () => void; +} + export interface ToastProps { id: string; type: ToastType; title: string; message?: string; duration?: number; + action?: ToastAction; onClose: (id: string) => void; } @@ -33,6 +39,7 @@ export function Toast({ title, message, duration = 3000, + action, onClose, }: ToastProps) { const Icon = iconMap[type]; @@ -62,6 +69,15 @@ export function Toast({ {message && (

{message}

)} + {action && ( + + )}
- -
+ +
- + {!isValidJson && ( {validationErrors.length > 0 ? 'Schema Error' : 'Invalid JSON'} )} + + + Ctrl+S save · Ctrl+Shift+F format +
{/* Editor Area */} diff --git a/apps/desktop/src/components/OAuthConsentModal.tsx b/apps/desktop/src/components/OAuthConsentModal.tsx index 87b371a2..1108b36c 100644 --- a/apps/desktop/src/components/OAuthConsentModal.tsx +++ b/apps/desktop/src/components/OAuthConsentModal.tsx @@ -16,7 +16,7 @@ import { listen } from '@tauri-apps/api/event'; import { Check, X, AlertCircle, Loader2, Globe, Lock } from 'lucide-react'; import { Button, Card, CardHeader, CardTitle, CardDescription, CardContent } from '@mcpmux/ui'; import { listSpaces, type Space } from '@/lib/api/spaces'; -import { useAppStore } from '@/stores'; +import { useNavigateTo } from '@/stores'; import { resolveKnownClientKey } from '@/lib/clientIcons'; import cursorIcon from '@/assets/client-icons/cursor.svg'; import vscodeIcon from '@/assets/client-icons/vscode.png'; @@ -123,6 +123,7 @@ export function OAuthConsentModal() { const [processError, setProcessError] = useState(null); /** 2-second cooldown before the Approve button becomes active */ const [approveReady, setApproveReady] = useState(false); + const navigateTo = useNavigateTo(); // Load spaces when modal opens useEffect(() => { @@ -293,7 +294,6 @@ export function OAuthConsentModal() { // Approved state - show success with next-step guidance if (modalState.type === 'approved') { - const navigateTo = useAppStore.getState().navigateTo; return (
@@ -320,21 +320,21 @@ export function OAuthConsentModal() {
diff --git a/apps/desktop/src/components/ServerLogViewer.tsx b/apps/desktop/src/components/ServerLogViewer.tsx index 5942f329..7905496d 100644 --- a/apps/desktop/src/components/ServerLogViewer.tsx +++ b/apps/desktop/src/components/ServerLogViewer.tsx @@ -1,6 +1,6 @@ import { useEffect, useState, useRef } from 'react'; import { X, Download, Trash2, RefreshCw } from 'lucide-react'; -import { useToast, ToastContainer } from '@mcpmux/ui'; +import { useToast, ToastContainer, useConfirm } from '@mcpmux/ui'; import { getServerLogs, clearServerLogs, getServerLogFile, type ServerLogEntry } from '@/lib/api/logs'; interface ServerLogViewerProps { @@ -41,6 +41,7 @@ export function ServerLogViewer({ serverId, serverName, onClose }: ServerLogView const scrollContainerRef = useRef(null); const shouldScrollRef = useRef(true); const { toasts, success, error: showError, dismiss } = useToast(); + const { confirm, ConfirmDialogElement } = useConfirm(); const loadLogs = async () => { try { @@ -93,7 +94,12 @@ export function ServerLogViewer({ serverId, serverName, onClose }: ServerLogView }; const handleClearLogs = async () => { - if (!confirm('Clear all logs for this server? This cannot be undone.')) { + if (!await confirm({ + title: 'Clear logs', + message: `Clear all logs for "${serverName}"? This cannot be undone.`, + confirmLabel: 'Clear', + variant: 'danger', + })) { return; } @@ -135,6 +141,7 @@ export function ServerLogViewer({ serverId, serverName, onClose }: ServerLogView return (
+ {ConfirmDialogElement}
{/* Header */}
diff --git a/apps/desktop/src/features/clients/ClientsPage.tsx b/apps/desktop/src/features/clients/ClientsPage.tsx index 30b22139..69fb55e6 100644 --- a/apps/desktop/src/features/clients/ClientsPage.tsx +++ b/apps/desktop/src/features/clients/ClientsPage.tsx @@ -32,6 +32,7 @@ import { Button, useToast, ToastContainer, + useConfirm, } from '@mcpmux/ui'; import type { OAuthClient, UpdateClientRequest } from '@/lib/api/gateway'; import { listOAuthClients, updateOAuthClient, deleteOAuthClient } from '@/lib/api/gateway'; @@ -120,7 +121,8 @@ export default function ClientsPage() { const [selectedClient, setSelectedClient] = useState(null); const { toasts, success, error: showError, info, dismiss } = useToast(); - + const { confirm, ConfirmDialogElement } = useConfirm(); + // Edit state const [editAlias, setEditAlias] = useState(''); const [editMode, setEditMode] = useState('follow_active'); @@ -383,9 +385,14 @@ export default function ClientsPage() { }; const handleDelete = async (clientId: string) => { - if (!confirm('Remove this client? All tokens will be revoked.')) return; - const deletedClient = oauthClients.find(c => c.client_id === clientId); + const name = deletedClient?.client_alias || deletedClient?.client_name || 'this client'; + if (!await confirm({ + title: 'Remove client', + message: `Remove "${name}"? All tokens will be revoked.`, + confirmLabel: 'Remove', + variant: 'danger', + })) return; const clientName = deletedClient?.client_alias || deletedClient?.client_name || 'Client'; try { @@ -1321,6 +1328,7 @@ export default function ClientsPage() { )} + {ConfirmDialogElement}
); } diff --git a/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx b/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx index 6ea56841..d2664b22 100644 --- a/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx +++ b/apps/desktop/src/features/featuresets/FeatureSetPanel.tsx @@ -20,7 +20,7 @@ import { Shield, Save, } from 'lucide-react'; -import { Button, useToast, ToastContainer } from '@mcpmux/ui'; +import { Button, useToast, ToastContainer, useConfirm } from '@mcpmux/ui'; import type { FeatureSet, AddMemberInput } from '@/lib/api/featureSets'; import { setFeatureSetMembers } from '@/lib/api/featureSets'; import type { ServerFeature } from '@/lib/api/serverFeatures'; @@ -49,6 +49,7 @@ export function FeatureSetPanel({ featureSet, spaceId, onClose, onDelete, onUpda const [error, setError] = useState(null); const [expandedServers, setExpandedServers] = useState>(new Set()); const { toasts, success, error: showError, dismiss } = useToast(); + const { confirm, ConfirmDialogElement } = useConfirm(); // Collapsible sections - only one expanded at a time, features by default const [expandedSections, setExpandedSections] = useState({ @@ -279,6 +280,7 @@ export function FeatureSetPanel({ featureSet, spaceId, onClose, onDelete, onUpda return (
+ {ConfirmDialogElement} {/* Panel Header */}
@@ -598,8 +600,13 @@ export function FeatureSetPanel({ featureSet, spaceId, onClose, onDelete, onUpda )} @@ -893,10 +893,11 @@ export function ServersPage() {

No servers installed

) : ( diff --git a/apps/desktop/src/features/spaces/SpacesPage.tsx b/apps/desktop/src/features/spaces/SpacesPage.tsx index 7e9ca240..67e3e45f 100644 --- a/apps/desktop/src/features/spaces/SpacesPage.tsx +++ b/apps/desktop/src/features/spaces/SpacesPage.tsx @@ -16,6 +16,7 @@ import { Button, useToast, ToastContainer, + useConfirm, } from '@mcpmux/ui'; import { useAppStore, @@ -39,6 +40,7 @@ export function SpacesPage() { const [searchQuery, setSearchQuery] = useState(''); const [error, setError] = useState(null); const [isActionLoading, setIsActionLoading] = useState(null); // ID of space being acted on + const { confirm, ConfirmDialogElement } = useConfirm(); const { toasts, success, error: showError, dismiss } = useToast(); // Create Modal State @@ -69,7 +71,13 @@ export function SpacesPage() { }; const handleDelete = async (id: string) => { - if (!confirm('Are you sure you want to delete this space? This action cannot be undone.')) return; + const spaceName = spaces.find(s => s.id === id)?.name || 'this space'; + if (!await confirm({ + title: 'Delete workspace', + message: `Are you sure you want to delete "${spaceName}"? This action cannot be undone.`, + confirmLabel: 'Delete', + variant: 'danger', + })) return; setIsActionLoading(id); setError(null); @@ -117,6 +125,7 @@ export function SpacesPage() { return ( <> + {ConfirmDialogElement}
{/* Header */}
diff --git a/packages/ui/src/components/common/ConfirmDialog.tsx b/packages/ui/src/components/common/ConfirmDialog.tsx new file mode 100644 index 00000000..7a3e3f58 --- /dev/null +++ b/packages/ui/src/components/common/ConfirmDialog.tsx @@ -0,0 +1,134 @@ +import { useCallback, useState, useRef } from 'react'; +import { AlertCircle } from 'lucide-react'; + +export interface ConfirmDialogState { + open: boolean; + title: string; + message: string; + confirmLabel?: string; + variant?: 'danger' | 'default'; +} + +export interface ConfirmDialogProps extends ConfirmDialogState { + onConfirm: () => void; + onCancel: () => void; +} + +export function ConfirmDialog({ + open, + title, + message, + confirmLabel = 'Confirm', + variant = 'default', + onConfirm, + onCancel, +}: ConfirmDialogProps) { + if (!open) return null; + + const isDanger = variant === 'danger'; + + return ( +
+
e.stopPropagation()} + data-testid="confirm-dialog" + > +
+ {isDanger && ( +
+ +
+ )} +
+

{title}

+

{message}

+
+
+
+ + +
+
+
+ ); +} + +/** + * Hook that provides a promise-based confirm dialog. + * + * Usage: + * ```tsx + * const { confirm, ConfirmDialogElement } = useConfirm(); + * + * const handleDelete = async () => { + * if (!await confirm({ title: 'Delete?', message: 'This cannot be undone.' })) return; + * // proceed with delete + * }; + * + * return <>{ConfirmDialogElement}; + * ``` + */ +export function useConfirm() { + const [state, setState] = useState({ + open: false, + title: '', + message: '', + key: 0, + }); + const resolveRef = useRef<((value: boolean) => void) | null>(null); + + const confirm = useCallback( + (options: Omit) => { + return new Promise((resolve) => { + resolveRef.current = resolve; + setState((prev) => ({ ...options, open: true, key: prev.key + 1 })); + }); + }, + [] + ); + + const handleConfirm = useCallback(() => { + setState((prev) => ({ ...prev, open: false })); + resolveRef.current?.(true); + resolveRef.current = null; + }, []); + + const handleCancel = useCallback(() => { + setState((prev) => ({ ...prev, open: false })); + resolveRef.current?.(false); + resolveRef.current = null; + }, []); + + const { key: dialogKey, ...dialogState } = state; + const ConfirmDialogElement = ( + + ); + + return { confirm, ConfirmDialogElement }; +} diff --git a/packages/ui/src/index.ts b/packages/ui/src/index.ts index 14ef87ab..ffcf9745 100644 --- a/packages/ui/src/index.ts +++ b/packages/ui/src/index.ts @@ -16,6 +16,8 @@ export { Card, CardHeader, CardTitle, CardDescription, CardContent } from './com export { Switch } from './components/common/Switch'; export { Toast, ToastContainer } from './components/common/Toast'; export type { ToastProps, ToastType, ToastAction } from './components/common/Toast'; +export { ConfirmDialog, useConfirm } from './components/common/ConfirmDialog'; +export type { ConfirmDialogState, ConfirmDialogProps } from './components/common/ConfirmDialog'; // Hooks export { useToast } from './hooks/useToast'; diff --git a/tests/ts/components/ConfirmDialog.test.tsx b/tests/ts/components/ConfirmDialog.test.tsx new file mode 100644 index 00000000..d6355b69 --- /dev/null +++ b/tests/ts/components/ConfirmDialog.test.tsx @@ -0,0 +1,202 @@ +import { describe, it, expect, vi } from 'vitest'; +import { render, screen, act } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ConfirmDialog, useConfirm } from '../../../packages/ui/src/components/common/ConfirmDialog'; + +describe('ConfirmDialog', () => { + it('should not render when closed', () => { + render( + + ); + + expect(screen.queryByTestId('confirm-dialog')).not.toBeInTheDocument(); + }); + + it('should render when open', () => { + render( + + ); + + expect(screen.getByTestId('confirm-dialog')).toBeInTheDocument(); + expect(screen.getByText('Delete item')).toBeInTheDocument(); + expect(screen.getByText('This cannot be undone.')).toBeInTheDocument(); + }); + + it('should show custom confirm label', () => { + render( + + ); + + expect(screen.getByTestId('confirm-dialog-confirm')).toHaveTextContent('Yes, delete'); + }); + + it('should show danger icon for danger variant', () => { + render( + + ); + + expect(screen.getByTestId('confirm-dialog')).toBeInTheDocument(); + }); + + it('should call onConfirm when confirm is clicked', async () => { + const user = userEvent.setup(); + const onConfirm = vi.fn(); + + render( + + ); + + await user.click(screen.getByTestId('confirm-dialog-confirm')); + expect(onConfirm).toHaveBeenCalledTimes(1); + }); + + it('should call onCancel when cancel is clicked', async () => { + const user = userEvent.setup(); + const onCancel = vi.fn(); + + render( + + ); + + await user.click(screen.getByTestId('confirm-dialog-cancel')); + expect(onCancel).toHaveBeenCalledTimes(1); + }); + + it('should call onCancel when overlay is clicked', async () => { + const user = userEvent.setup(); + const onCancel = vi.fn(); + + render( + + ); + + await user.click(screen.getByTestId('confirm-dialog-overlay')); + expect(onCancel).toHaveBeenCalledTimes(1); + }); +}); + +describe('useConfirm', () => { + function TestComponent({ onResult }: { onResult: (v: boolean) => void }) { + const { confirm, ConfirmDialogElement } = useConfirm(); + + return ( +
+ + {ConfirmDialogElement} +
+ ); + } + + it('should resolve true when confirmed', async () => { + const user = userEvent.setup(); + const onResult = vi.fn(); + + render(); + + // Dialog should not be visible initially + expect(screen.queryByTestId('confirm-dialog')).not.toBeInTheDocument(); + + // Open dialog + await user.click(screen.getByTestId('trigger')); + + // Dialog should be visible + expect(screen.getByTestId('confirm-dialog')).toBeInTheDocument(); + expect(screen.getByText('Confirm action')).toBeInTheDocument(); + expect(screen.getByText('Do you want to proceed?')).toBeInTheDocument(); + expect(screen.getByTestId('confirm-dialog-confirm')).toHaveTextContent('Proceed'); + + // Click confirm + await user.click(screen.getByTestId('confirm-dialog-confirm')); + + expect(onResult).toHaveBeenCalledWith(true); + // Dialog should close + expect(screen.queryByTestId('confirm-dialog')).not.toBeInTheDocument(); + }); + + it('should resolve false when cancelled', async () => { + const user = userEvent.setup(); + const onResult = vi.fn(); + + render(); + + await user.click(screen.getByTestId('trigger')); + expect(screen.getByTestId('confirm-dialog')).toBeInTheDocument(); + + await user.click(screen.getByTestId('confirm-dialog-cancel')); + + expect(onResult).toHaveBeenCalledWith(false); + expect(screen.queryByTestId('confirm-dialog')).not.toBeInTheDocument(); + }); + + it('should resolve false when overlay is clicked', async () => { + const user = userEvent.setup(); + const onResult = vi.fn(); + + render(); + + await user.click(screen.getByTestId('trigger')); + expect(screen.getByTestId('confirm-dialog')).toBeInTheDocument(); + + await user.click(screen.getByTestId('confirm-dialog-overlay')); + + expect(onResult).toHaveBeenCalledWith(false); + expect(screen.queryByTestId('confirm-dialog')).not.toBeInTheDocument(); + }); +}); From 813f142d8750835b8a76d0f71fdb282f5df7e751 Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Wed, 25 Feb 2026 16:42:45 +0800 Subject: [PATCH 4/6] fix: refresh clients list when navigating from Manage Permissions button After OAuth approval, clicking "Manage Permissions" navigates to the Clients page but the list could be stale. Emit oauth-client-changed event after a short delay so the ClientsPage picks up the new client after mounting and subscribing to events. Signed-off-by: mcpmux Signed-off-by: Mohammod Al Amin Ashik --- apps/desktop/src/components/OAuthConsentModal.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/components/OAuthConsentModal.tsx b/apps/desktop/src/components/OAuthConsentModal.tsx index 1108b36c..24169c49 100644 --- a/apps/desktop/src/components/OAuthConsentModal.tsx +++ b/apps/desktop/src/components/OAuthConsentModal.tsx @@ -12,7 +12,7 @@ import { useState, useEffect } from 'react'; import { invoke } from '@tauri-apps/api/core'; -import { listen } from '@tauri-apps/api/event'; +import { listen, emit } from '@tauri-apps/api/event'; import { Check, X, AlertCircle, Loader2, Globe, Lock } from 'lucide-react'; import { Button, Card, CardHeader, CardTitle, CardDescription, CardContent } from '@mcpmux/ui'; import { listSpaces, type Space } from '@/lib/api/spaces'; @@ -328,8 +328,13 @@ export function OAuthConsentModal() { variant="primary" className="flex-1 whitespace-nowrap" onClick={() => { - navigateTo('clients'); handleDismiss(); + navigateTo('clients'); + // Emit event after a short delay so ClientsPage has time to mount + // and subscribe to the event before it fires + setTimeout(() => { + emit('oauth-client-changed', { action: 'approved' }); + }, 300); }} data-testid="go-to-clients-btn" > From ace86f1b6c03b8069dd4b53f31699ab7052b46db Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Wed, 25 Feb 2026 17:46:49 +0800 Subject: [PATCH 5/6] fix: auto-open client panel when navigating from Manage Permissions After OAuth approval, clicking "Manage Permissions" now sets a pendingClientId in the store. When ClientsPage mounts and loads data, it detects the pending ID and auto-opens that client's detail panel so the user can configure permissions immediately. Also adds e2e tests for the ConfirmDialog component verifying that delete prompts appear, cancel dismisses without action, and overlay click dismisses. Signed-off-by: mcpmux Signed-off-by: Mohammod Al Amin Ashik --- .../src/components/OAuthConsentModal.tsx | 8 +- .../src/features/clients/ClientsPage.tsx | 14 ++- apps/desktop/src/stores/appStore.ts | 6 + apps/desktop/src/stores/selectors.ts | 2 + apps/desktop/src/stores/types.ts | 3 + tests/e2e/specs/confirm-dialog.spec.ts | 113 ++++++++++++++++++ tests/ts/stores/appStore.test.ts | 13 ++ 7 files changed, 155 insertions(+), 4 deletions(-) create mode 100644 tests/e2e/specs/confirm-dialog.spec.ts diff --git a/apps/desktop/src/components/OAuthConsentModal.tsx b/apps/desktop/src/components/OAuthConsentModal.tsx index 24169c49..0a022187 100644 --- a/apps/desktop/src/components/OAuthConsentModal.tsx +++ b/apps/desktop/src/components/OAuthConsentModal.tsx @@ -16,7 +16,7 @@ import { listen, emit } from '@tauri-apps/api/event'; import { Check, X, AlertCircle, Loader2, Globe, Lock } from 'lucide-react'; import { Button, Card, CardHeader, CardTitle, CardDescription, CardContent } from '@mcpmux/ui'; import { listSpaces, type Space } from '@/lib/api/spaces'; -import { useNavigateTo } from '@/stores'; +import { useNavigateTo, useSetPendingClientId } from '@/stores'; import { resolveKnownClientKey } from '@/lib/clientIcons'; import cursorIcon from '@/assets/client-icons/cursor.svg'; import vscodeIcon from '@/assets/client-icons/vscode.png'; @@ -74,7 +74,7 @@ type ModalState = | { type: 'loading'; requestId: string } | { type: 'error'; requestId: string; error: ConsentError } | { type: 'consent'; details: ConsentRequestDetails } - | { type: 'approved'; clientName: string }; + | { type: 'approved'; clientName: string; clientId: string }; /** Open a URL using the backend open command (handles custom protocols like cursor://) */ async function openRedirectUrl(url: string): Promise { @@ -124,6 +124,7 @@ export function OAuthConsentModal() { /** 2-second cooldown before the Approve button becomes active */ const [approveReady, setApproveReady] = useState(false); const navigateTo = useNavigateTo(); + const setPendingClientId = useSetPendingClientId(); // Load spaces when modal opens useEffect(() => { @@ -199,7 +200,7 @@ export function OAuthConsentModal() { if (response.success && response.redirect_url) { console.log('[OAuth] Approved, redirecting to:', response.redirect_url); await openRedirectUrl(response.redirect_url); - setModalState({ type: 'approved', clientName: clientAlias || details.clientName }); + setModalState({ type: 'approved', clientName: clientAlias || details.clientName, clientId: details.clientId }); } else { setProcessError(response.error || 'Failed to approve consent'); } @@ -328,6 +329,7 @@ export function OAuthConsentModal() { variant="primary" className="flex-1 whitespace-nowrap" onClick={() => { + setPendingClientId(modalState.clientId); handleDismiss(); navigateTo('clients'); // Emit event after a short delay so ClientsPage has time to mount diff --git a/apps/desktop/src/features/clients/ClientsPage.tsx b/apps/desktop/src/features/clients/ClientsPage.tsx index 69fb55e6..475d5770 100644 --- a/apps/desktop/src/features/clients/ClientsPage.tsx +++ b/apps/desktop/src/features/clients/ClientsPage.tsx @@ -38,7 +38,7 @@ import type { OAuthClient, UpdateClientRequest } from '@/lib/api/gateway'; import { listOAuthClients, updateOAuthClient, deleteOAuthClient } from '@/lib/api/gateway'; import type { Space } from '@/lib/api/spaces'; import { listSpaces } from '@/lib/api/spaces'; -import { useViewSpace } from '@/stores'; +import { useViewSpace, usePendingClientId, useSetPendingClientId } from '@/stores'; import type { FeatureSet } from '@/lib/api/featureSets'; import { listFeatureSetsBySpace } from '@/lib/api/featureSets'; import { @@ -122,6 +122,8 @@ export default function ClientsPage() { const { toasts, success, error: showError, info, dismiss } = useToast(); const { confirm, ConfirmDialogElement } = useConfirm(); + const pendingClientId = usePendingClientId(); + const setPendingClientId = useSetPendingClientId(); // Edit state const [editAlias, setEditAlias] = useState(''); @@ -267,6 +269,16 @@ export default function ClientsPage() { loadData(); }, []); + // Auto-open a client panel when navigated from "Manage Permissions" + useEffect(() => { + if (!pendingClientId || isLoading) return; + const client = oauthClients.find(c => c.client_id === pendingClientId); + if (client) { + openPanel(client); + setPendingClientId(null); + } + }, [pendingClientId, isLoading, oauthClients]); + useEffect(() => { setActiveSpace(viewSpace); }, [viewSpace?.id]); diff --git a/apps/desktop/src/stores/appStore.ts b/apps/desktop/src/stores/appStore.ts index d9811d4e..91e554dd 100644 --- a/apps/desktop/src/stores/appStore.ts +++ b/apps/desktop/src/stores/appStore.ts @@ -8,6 +8,7 @@ const initialState: AppState = { activeSpaceId: null, viewSpaceId: null, activeNav: 'home', + pendingClientId: null, sidebarCollapsed: false, theme: 'system', analyticsEnabled: true, @@ -92,6 +93,11 @@ export const useAppStore = create()( state.activeNav = nav; }), + setPendingClientId: (id) => + set((state) => { + state.pendingClientId = id; + }), + // UI toggleSidebar: () => set((state) => { diff --git a/apps/desktop/src/stores/selectors.ts b/apps/desktop/src/stores/selectors.ts index 2478c900..02ed4cb4 100644 --- a/apps/desktop/src/stores/selectors.ts +++ b/apps/desktop/src/stores/selectors.ts @@ -7,6 +7,8 @@ export const useActiveSpaceId = () => useAppStore((state) => state.activeSpaceId export const useViewSpaceId = () => useAppStore((state) => state.viewSpaceId); export const useActiveNav = () => useAppStore((state) => state.activeNav); export const useNavigateTo = () => useAppStore((state) => state.navigateTo); +export const usePendingClientId = () => useAppStore((state) => state.pendingClientId); +export const useSetPendingClientId = () => useAppStore((state) => state.setPendingClientId); export const useTheme = () => useAppStore((state) => state.theme); export const useSidebarCollapsed = () => useAppStore((state) => state.sidebarCollapsed); export const useAnalyticsEnabled = () => useAppStore((state) => state.analyticsEnabled); diff --git a/apps/desktop/src/stores/types.ts b/apps/desktop/src/stores/types.ts index 6e87de87..15b55d3c 100644 --- a/apps/desktop/src/stores/types.ts +++ b/apps/desktop/src/stores/types.ts @@ -10,6 +10,8 @@ export interface AppState { // Navigation activeNav: NavItem; + /** Client ID to auto-select when navigating to Clients page */ + pendingClientId: string | null; // UI state sidebarCollapsed: boolean; @@ -34,6 +36,7 @@ export interface AppActions { // Navigation navigateTo: (nav: NavItem) => void; + setPendingClientId: (id: string | null) => void; // UI toggleSidebar: () => void; diff --git a/tests/e2e/specs/confirm-dialog.spec.ts b/tests/e2e/specs/confirm-dialog.spec.ts new file mode 100644 index 00000000..ba1c01e5 --- /dev/null +++ b/tests/e2e/specs/confirm-dialog.spec.ts @@ -0,0 +1,113 @@ +import { test, expect } from '@playwright/test'; +import { DashboardPage, SpacesPage, ClientsPage } from '../pages'; + +// Helper to click Spaces in sidebar (avoids space switcher button) +async function goToSpaces(page: import('@playwright/test').Page) { + await page.locator('nav button:has-text("Spaces")').last().click(); +} + +test.describe('ConfirmDialog – Spaces', () => { + test('should show confirm dialog when clicking delete on a non-default space', async ({ page }) => { + const dashboard = new DashboardPage(page); + await dashboard.navigate(); + await goToSpaces(page); + await expect(page.locator('h1:has-text("Workspaces")')).toBeVisible(); + + // Look for a delete button + const deleteBtn = page.locator('[data-testid^="delete-space-"]').first(); + if (await deleteBtn.isVisible().catch(() => false)) { + await deleteBtn.click(); + + // Confirm dialog should appear + await expect(page.getByTestId('confirm-dialog')).toBeVisible(); + await expect(page.getByTestId('confirm-dialog-confirm')).toBeVisible(); + await expect(page.getByTestId('confirm-dialog-cancel')).toBeVisible(); + + // Title should mention delete + await expect(page.getByTestId('confirm-dialog').locator('h3')).toContainText(/[Dd]elete/); + } + }); + + test('should dismiss confirm dialog on cancel without deleting', async ({ page }) => { + const dashboard = new DashboardPage(page); + await dashboard.navigate(); + await goToSpaces(page); + await expect(page.locator('h1:has-text("Workspaces")')).toBeVisible(); + + const deleteBtn = page.locator('[data-testid^="delete-space-"]').first(); + if (await deleteBtn.isVisible().catch(() => false)) { + // Count spaces before + const spaceBefore = await page.locator('[data-testid^="space-card-"]').count(); + + await deleteBtn.click(); + await expect(page.getByTestId('confirm-dialog')).toBeVisible(); + + // Click cancel + await page.getByTestId('confirm-dialog-cancel').click(); + + // Dialog should close + await expect(page.getByTestId('confirm-dialog')).not.toBeVisible(); + + // Space count should be the same (nothing was deleted) + const spaceAfter = await page.locator('[data-testid^="space-card-"]').count(); + expect(spaceAfter).toBe(spaceBefore); + } + }); + + test('should dismiss confirm dialog when clicking overlay', async ({ page }) => { + const dashboard = new DashboardPage(page); + await dashboard.navigate(); + await goToSpaces(page); + await expect(page.locator('h1:has-text("Workspaces")')).toBeVisible(); + + const deleteBtn = page.locator('[data-testid^="delete-space-"]').first(); + if (await deleteBtn.isVisible().catch(() => false)) { + await deleteBtn.click(); + await expect(page.getByTestId('confirm-dialog')).toBeVisible(); + + // Click overlay (outside the dialog) + await page.getByTestId('confirm-dialog-overlay').click({ position: { x: 5, y: 5 } }); + + // Dialog should close + await expect(page.getByTestId('confirm-dialog')).not.toBeVisible(); + } + }); +}); + +test.describe('ConfirmDialog – Clients', () => { + test('should show confirm dialog when clicking Remove Client', async ({ page }) => { + const dashboard = new DashboardPage(page); + await dashboard.navigate(); + await page.locator('nav button:has-text("Clients")').click(); + await expect(page.getByRole('heading', { name: 'Connected Clients' })).toBeVisible(); + + // Click the first client card to open the detail panel + const clientCards = page.locator('[data-testid^="client-card-"]'); + const count = await clientCards.count(); + + if (count > 0) { + await clientCards.first().click(); + + // Wait for panel to open + await page.waitForTimeout(300); + + // Find the Remove Client button in the panel + const removeBtn = page.getByRole('button', { name: /Remove Client/i }); + if (await removeBtn.isVisible().catch(() => false)) { + await removeBtn.click(); + + // Confirm dialog should appear + await expect(page.getByTestId('confirm-dialog')).toBeVisible(); + await expect(page.getByTestId('confirm-dialog-confirm')).toHaveText(/Remove/i); + + // Cancel should dismiss without removing + await page.getByTestId('confirm-dialog-cancel').click(); + await expect(page.getByTestId('confirm-dialog')).not.toBeVisible(); + + // Client should still be there + const countAfter = await clientCards.count(); + expect(countAfter).toBe(count); + } + } + }); +}); diff --git a/tests/ts/stores/appStore.test.ts b/tests/ts/stores/appStore.test.ts index 87e2bfb0..0866a077 100644 --- a/tests/ts/stores/appStore.test.ts +++ b/tests/ts/stores/appStore.test.ts @@ -10,6 +10,7 @@ describe('appStore', () => { activeSpaceId: null, viewSpaceId: null, activeNav: 'home', + pendingClientId: null, sidebarCollapsed: false, theme: 'system', loading: { spaces: false, servers: false }, @@ -299,6 +300,18 @@ describe('appStore', () => { }); }); + describe('setPendingClientId', () => { + it('should set and clear pending client id', () => { + expect(useAppStore.getState().pendingClientId).toBeNull(); + + useAppStore.getState().setPendingClientId('client-123'); + expect(useAppStore.getState().pendingClientId).toBe('client-123'); + + useAppStore.getState().setPendingClientId(null); + expect(useAppStore.getState().pendingClientId).toBeNull(); + }); + }); + describe('setLoading', () => { it('should set spaces loading state', () => { useAppStore.getState().setLoading('spaces', true); From 0bcab394c74e2eeb124b39ee5bedf51ff5dcca75 Mon Sep 17 00:00:00 2001 From: Mohammod Al Amin Ashik Date: Wed, 25 Feb 2026 18:12:47 +0800 Subject: [PATCH 6/6] fix: remove unused imports flagged by code quality Remove unused `act` import from ConfirmDialog.test.tsx and unused `SpacesPage`/`ClientsPage` imports from confirm-dialog.spec.ts. Signed-off-by: mcpmux Signed-off-by: Mohammod Al Amin Ashik --- tests/e2e/specs/confirm-dialog.spec.ts | 2 +- tests/ts/components/ConfirmDialog.test.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/specs/confirm-dialog.spec.ts b/tests/e2e/specs/confirm-dialog.spec.ts index ba1c01e5..32c05083 100644 --- a/tests/e2e/specs/confirm-dialog.spec.ts +++ b/tests/e2e/specs/confirm-dialog.spec.ts @@ -1,5 +1,5 @@ import { test, expect } from '@playwright/test'; -import { DashboardPage, SpacesPage, ClientsPage } from '../pages'; +import { DashboardPage } from '../pages'; // Helper to click Spaces in sidebar (avoids space switcher button) async function goToSpaces(page: import('@playwright/test').Page) { diff --git a/tests/ts/components/ConfirmDialog.test.tsx b/tests/ts/components/ConfirmDialog.test.tsx index d6355b69..5e7186da 100644 --- a/tests/ts/components/ConfirmDialog.test.tsx +++ b/tests/ts/components/ConfirmDialog.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi } from 'vitest'; -import { render, screen, act } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { ConfirmDialog, useConfirm } from '../../../packages/ui/src/components/common/ConfirmDialog';