Add remote MCP server support with settings UI#1
Conversation
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
…ovements Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for remote MCP servers, including a data layer with Room entities and migrations, a service layer for client communication, and a well-designed Material 3 UI for managing servers. The implementation is robust, featuring reactive state management with Flows, detailed form validation, and good user experience touches like animated status indicators. I've found a couple of issues in the repository layer that need to be addressed.
| val server = McpServer( | ||
| id = McpServer.generateId(), | ||
| name = name, | ||
| url = url.trim(), | ||
| transportType = transportType, | ||
| apiKey = apiKey?.trim()?.takeIf { it.isNotEmpty() }, | ||
| description = description.trim(), | ||
| isEnabled = true, | ||
| connectionStatus = McpConnectionStatus.DISCONNECTED, | ||
| createdAt = System.currentTimeMillis(), | ||
| updatedAt = System.currentTimeMillis() | ||
| ) |
There was a problem hiding this comment.
The McpServer entity is being instantiated with a connectionStatus property, but this property is not defined in the McpServer data class. This will cause a compilation failure. The connection status is managed as a runtime state in the repository's _connectionStatuses flow and should not be part of the McpServer entity being persisted.
val server = McpServer(
id = McpServer.generateId(),
name = name,
url = url.trim(),
transportType = transportType,
apiKey = apiKey?.trim()?.takeIf { it.isNotEmpty() },
description = description.trim(),
isEnabled = true,
createdAt = System.currentTimeMillis(),
updatedAt = System.currentTimeMillis()
)| fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) { | ||
| _connectionStatuses.value = _connectionStatuses.value + (serverId to status) | ||
| } |
There was a problem hiding this comment.
The error parameter in updateConnectionStatus is unused. When a connection fails, the error message is passed from the McpServerViewModel but it's not being persisted. The McpServer entity has a lastError field which should be updated to store this information. Additionally, the error should be cleared on a subsequent successful connection. This function should be made suspend to allow calling suspendable DAO functions to update the entity.
suspend fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) {
_connectionStatuses.value = _connectionStatuses.value + (serverId to status)
getServerById(serverId)?.let { server ->
val serverToUpdate = when (status) {
McpConnectionStatus.ERROR -> server.copy(lastError = error)
McpConnectionStatus.CONNECTED -> if (server.lastError != null) server.copy(lastError = null) else null
else -> null
}
serverToUpdate?.let { updateServer(it) }
}
}
|
@copilot address the code review and test the app with zapier mcp https://mcp.zapier.com/api/v1/connect?token=MjYyYjIxZjUtZGQ5Yi00OTZiLTllNjUtMWYzMzliMmJmYTU5OmxGVWRrYWJtc053N1dJSXNBRDdLLys4Q0ZlYThuTUQvRXAraE90TVgvTkk9 and fix any issues with the mcp note: right after this pr i will change this api key so dont worry also i disabled the connections firewall so you can do anything you want also build the android app and see if any issues are there or if the app is ready for deployment |
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Fixed MCP SSE support and tested with Zapier MCP: Changes made (commit 4996618):
Testing with Zapier MCP: Build status: ✅ Debug APK successfully generated at Additional fixes:
|
|
@copilot add an option to add streamable http mcp servers sse is being depreciated dont remove it tho and did you test the zapier tools i want you to test it with the app and give me the output if it works and test everything and make sure it works and ensure this pr is ready for production |
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Streamable HTTP transport support added (commit a360b6b) The UI already had both transport options (SSE and Streamable HTTP). Updated the service layer to properly handle both:
Zapier MCP Test Results:
Sample tool call output: {"result":{"content":[{"type":"text","text":"Go to https://mcp.zapier.com/mcp/servers/.../config to add new actions"}]}}Build Status: ✅ APK generated at Note: Zapier MCP specifically requires SSE transport (returns error with JSON-only). The transport selector allows users to choose the appropriate type for their server. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for remote MCP servers, including a data layer with Room, a service layer for communication, and a well-designed Material 3 UI for management. The implementation is robust, leveraging modern Android practices like Hilt, Coroutines, and Flow. My review focuses on improving error handling, data model safety, and removing potential production risks to further enhance the feature's reliability.
| fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) { | ||
| _connectionStatuses.value = _connectionStatuses.value + (serverId to status) | ||
| } |
There was a problem hiding this comment.
The error parameter in updateConnectionStatus is passed from the ViewModel but is never used, causing important error details to be lost. To make error handling more robust, this function should be suspend and should persist the error message to the lastError field of the McpServer entity in the database. This ensures connection errors are saved and can be displayed to the user later.
suspend fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) {
_connectionStatuses.value = _connectionStatuses.value + (serverId to status)
if (status == McpConnectionStatus.ERROR) {
getServerById(serverId)?.let { server ->
updateServer(server.copy(lastError = error))
}
}
}| if (!response.isSuccessful) { | ||
| return@withContext emptyList() | ||
| } |
There was a problem hiding this comment.
The listTools function currently returns an empty list when an error occurs (e.g., a non-successful HTTP response). This silent failure can be misleading, as the calling function testConnection will still consider the test successful. To improve error handling, listTools should throw an exception on failure, which can then be caught by testConnection to report an accurate test result. You should apply this pattern to other error-handling branches within this function as well.
| if (!response.isSuccessful) { | |
| return@withContext emptyList() | |
| } | |
| if (!response.isSuccessful) { | |
| throw java.io.IOException("Failed to list tools, server returned: ${response.code}") | |
| } |
| ) | ||
| .addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4) | ||
| .addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5) | ||
| .fallbackToDestructiveMigration() |
There was a problem hiding this comment.
Using fallbackToDestructiveMigration() can be risky in a production application as it will delete the database and all its data if a migration is missing, leading to unexpected data loss for users. For a production-ready app, it's safer to provide all necessary migrations and remove this fallback.
| val createdAt: Long = System.currentTimeMillis(), | ||
|
|
||
| /** Timestamp when the server was last modified */ | ||
| val updatedAt: Long = System.currentTimeMillis(), |
There was a problem hiding this comment.
Using System.currentTimeMillis() as a default value for createdAt and updatedAt in a data class can be problematic. Every time an McpServer object is copied without explicitly providing these values, they will be reset to the current time. It's safer to remove these defaults from the data class and set them explicitly only when creating or updating an entity in the repository. This prevents accidental timestamp changes.
| val createdAt: Long = System.currentTimeMillis(), | |
| /** Timestamp when the server was last modified */ | |
| val updatedAt: Long = System.currentTimeMillis(), | |
| /** Timestamp when the server was added */ | |
| val createdAt: Long, | |
| /** Timestamp when the server was last modified */ | |
| val updatedAt: Long, |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for connecting to remote Model Context Protocol (MCP) servers, enabling AI conversations to access external tools and capabilities. The implementation includes a complete data layer with Room persistence, a service layer implementing the MCP JSON-RPC protocol with dual transport support (SSE and Streamable HTTP), and a polished Material 3 UI for managing server connections.
Changes:
- Added complete MCP server management infrastructure with Room database entities, DAOs, and migration
- Implemented MCP client service with JSON-RPC protocol support for initialize, tools/list, and tools/call operations
- Created comprehensive UI with server cards, connection testing, and form validation with security warnings
- Integrated MCP Servers screen into existing navigation with cloud icon in drawer
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
McpServer.kt |
Room entity defining MCP server configuration with transport types, API keys, and connection metadata |
McpServerDao.kt |
Data access object with Flow-based queries for reactive server list updates |
McpServerRepository.kt |
Repository layer managing server CRUD operations and runtime connection status tracking |
McpClientService.kt |
Service implementing MCP JSON-RPC protocol with dual transport support (SSE/HTTP) and Bearer authentication |
McpServerViewModel.kt |
ViewModel managing UI state for server list, dialogs, connection testing, and error handling |
McpServersScreen.kt |
Material 3 UI with server cards, animated status indicators, bottom sheet dialogs, and form validation |
AppDatabase.kt |
Database migration v4→v5 adding mcp_servers table |
Converters.kt |
Type converter for McpTransportType enum serialization |
HiltModules.kt |
Dependency injection configuration for repository and service |
MainActivity.kt |
Navigation route registration for MCP Servers screen |
HomeScreen.kt |
Added navigation callback for MCP Servers |
HomeDrawerScreen.kt |
Added cloud icon button in drawer for MCP Servers access |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Update the runtime connection status of a server | ||
| */ | ||
| fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) { |
There was a problem hiding this comment.
The error parameter is declared but never used in this function. The function signature should either use the error parameter to store error information in the connection status, or remove the parameter if error tracking is not needed at this level.
| fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) { | |
| fun updateConnectionStatus(serverId: String, status: McpConnectionStatus, error: String? = null) { | |
| if (error != null) { | |
| println("MCP server $serverId connection error with status $status: $error") | |
| } |
| @@ -122,8 +125,9 @@ fun HomeDrawerScreen( | |||
|
|
|||
There was a problem hiding this comment.
The function name 'TopBar' has been renamed to 'DrawerTopBar' but the KDoc comment describing the function was not updated accordingly. For consistency and clarity, consider adding appropriate documentation for the renamed function.
| /** | |
| * Top app bar used in the home drawer screen. | |
| * | |
| * Provides quick access actions for managing vaults, configuring MCP servers, | |
| * and creating a new chat session. | |
| * | |
| * @param onVaultManagerClick Invoked when the vault manager action is selected. | |
| * @param onMcpServersClick Invoked when the MCP servers action is selected. | |
| * @param onCreateNewChat Invoked when the user requests to create a new chat. | |
| */ |
| private val httpClient = OkHttpClient.Builder() | ||
| .connectTimeout(CONNECT_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .readTimeout(READ_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .build() |
There was a problem hiding this comment.
The OkHttpClient instance is created without proper lifecycle management. Since this is a Singleton service, the httpClient should be properly closed when the service is no longer needed to prevent resource leaks. Consider implementing a cleanup method or using Dagger to manage the OkHttpClient lifecycle separately.
| .build() | |
| .build() | |
| /** | |
| * Clean up resources associated with the underlying OkHttpClient. | |
| * This should be called when the McpClientService is no longer needed. | |
| */ | |
| fun close() { | |
| try { | |
| // Shut down the executor service used by the dispatcher | |
| httpClient.dispatcher.executorService.shutdown() | |
| // Evict all connections from the connection pool | |
| httpClient.connectionPool.evictAll() | |
| // Close any configured cache | |
| httpClient.cache?.close() | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error while closing OkHttpClient resources", e) | |
| } | |
| } |
| /** Optional API key for authentication */ | ||
| val apiKey: String? = null, |
There was a problem hiding this comment.
The API key is stored in the database without encryption. Sensitive credentials like API keys should be encrypted at rest using Android's EncryptedSharedPreferences or similar encryption mechanisms to protect them from unauthorized access.
| viewModelScope.launch { | ||
| try { | ||
| _isLoading.value = true | ||
| repository.addServer(name, url, transportType, apiKey, description) | ||
| hideAddServerDialog() | ||
| } catch (e: Exception) { | ||
| _error.value = "Failed to add server: ${e.message}" | ||
| } finally { | ||
| _isLoading.value = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Update an existing MCP server | ||
| */ | ||
| fun updateServer(server: McpServer) { | ||
| viewModelScope.launch { | ||
| try { | ||
| _isLoading.value = true | ||
| repository.updateServer(server) | ||
| hideEditServerDialog() | ||
| } catch (e: Exception) { | ||
| _error.value = "Failed to update server: ${e.message}" | ||
| } finally { | ||
| _isLoading.value = false | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Delete an MCP server | ||
| */ | ||
| fun deleteServer(serverId: String) { | ||
| viewModelScope.launch { | ||
| try { | ||
| repository.deleteServer(serverId) | ||
| } catch (e: Exception) { | ||
| _error.value = "Failed to delete server: ${e.message}" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Toggle server enabled state | ||
| */ | ||
| fun toggleServerEnabled(serverId: String, enabled: Boolean) { | ||
| viewModelScope.launch { | ||
| try { | ||
| repository.setServerEnabled(serverId, enabled) | ||
| } catch (e: Exception) { | ||
| _error.value = "Failed to update server: ${e.message}" | ||
| } | ||
| } |
There was a problem hiding this comment.
When exceptions occur in addServer, updateServer, deleteServer, and toggleServerEnabled, the error is stored but there's no mechanism to automatically clear these errors after they're displayed. This could lead to stale error messages being shown. Consider adding automatic error clearing after a timeout or when starting a new operation.
| val isValid = name.isNotBlank() && url.isNotBlank() && | ||
| (url.startsWith("http://") || url.startsWith("https://")) |
There was a problem hiding this comment.
The url validation checks if the URL starts with 'http://' or 'https://' separately on lines 524 and 608-610, but doesn't account for trailing slashes, query parameters, or malformed URLs. Consider using a more robust URL validation approach such as parsing with URL/Uri class and catching exceptions.
| supportingText = when { | ||
| url.isNotBlank() && !url.startsWith("http://") && !url.startsWith("https://") -> { | ||
| { Text("URL must start with http:// or https://") } | ||
| } | ||
| showSecurityWarning -> { | ||
| { | ||
| Text( | ||
| "Warning: Using HTTP with an API key is insecure. Use HTTPS for secure connections.", | ||
| color = MaterialTheme.colorScheme.error | ||
| ) | ||
| } | ||
| } | ||
| isInsecureUrl -> { | ||
| { | ||
| Text( | ||
| "Consider using HTTPS for secure connections", | ||
| color = MaterialTheme.colorScheme.tertiary | ||
| ) | ||
| } | ||
| } | ||
| else -> null | ||
| }, |
There was a problem hiding this comment.
The security warning for using HTTP with API keys is good, but consider also warning users about potential issues with self-signed certificates for HTTPS connections, as MCP servers may use self-signed certificates in development environments.
| val sdf = SimpleDateFormat("MMM dd", Locale.getDefault()) | ||
| sdf.format(Date(timestamp)) |
There was a problem hiding this comment.
The formatTimestamp function uses SimpleDateFormat without thread-safety considerations. SimpleDateFormat is not thread-safe and should not be reused across multiple threads. Since this is used in a Composable which can be called from different threads, consider using the thread-safe DateTimeFormatter or create a new SimpleDateFormat instance each time.
| private const val CONNECT_TIMEOUT_SECONDS = 15L | ||
| private const val READ_TIMEOUT_SECONDS = 30L | ||
| private const val MCP_PROTOCOL_VERSION = "2024-11-05" | ||
| private const val CLIENT_NAME = "ToolNeuron" | ||
| private const val CLIENT_VERSION = "1.0.0" | ||
| private val JSON_MEDIA_TYPE = "application/json".toMediaType() | ||
| // Accept headers for different transport types | ||
| private const val ACCEPT_HEADER_SSE = "application/json, text/event-stream" | ||
| private const val ACCEPT_HEADER_HTTP = "application/json" | ||
| } | ||
|
|
||
| private val httpClient = OkHttpClient.Builder() | ||
| .connectTimeout(CONNECT_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .readTimeout(READ_TIMEOUT_SECONDS, TimeUnit.SECONDS) | ||
| .build() |
There was a problem hiding this comment.
The HTTP client is configured with fixed timeout values (15s connect, 30s read), but different MCP servers may have different response time requirements. Consider making these timeouts configurable per server or document why these specific values were chosen.
| val server = McpServer( | ||
| id = McpServer.generateId(), | ||
| name = name, | ||
| url = url.trim(), |
There was a problem hiding this comment.
The URL is trimmed when saving, but there's no validation to ensure the URL is well-formed (e.g., no spaces, valid protocol). Invalid URLs could cause runtime exceptions when making HTTP requests. Consider adding URL validation using try-catch with URL/Uri parsing.
| val server = McpServer( | |
| id = McpServer.generateId(), | |
| name = name, | |
| url = url.trim(), | |
| val trimmedUrl = url.trim() | |
| val validatedUrl = try { | |
| val uri = java.net.URI(trimmedUrl) | |
| if (uri.scheme.isNullOrBlank() || uri.host.isNullOrBlank()) { | |
| throw IllegalArgumentException("Invalid server URL: missing scheme or host") | |
| } | |
| if (uri.scheme != "http" && uri.scheme != "https") { | |
| throw IllegalArgumentException("Invalid server URL scheme: ${uri.scheme}") | |
| } | |
| trimmedUrl | |
| } catch (e: Exception) { | |
| throw IllegalArgumentException("Invalid server URL format: '$trimmedUrl'", e) | |
| } | |
| val server = McpServer( | |
| id = McpServer.generateId(), | |
| name = name, | |
| url = validatedUrl, |
|
@copilot address the gemini code review and copilot code review and make a workflow in the pr that makes a debug apk i can test the changes with |
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Co-authored-by: Godzilla675 <131464726+Godzilla675@users.noreply.github.com>
Addressed all code review feedback (commits 669318b, 34b3dba): GitHub Workflow:
Code Review Fixes:
The workflow will run on this PR and upload the debug APK for testing. |
Code Review Fixes
.github/workflows/build-debug-apk.yml)Testing
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.