feat: add MCP server for agent access to curated repos (closes #164)#247
Closed
AmintaCCCP wants to merge 11 commits into
Closed
feat: add MCP server for agent access to curated repos (closes #164)#247AmintaCCCP wants to merge 11 commits into
AmintaCCCP wants to merge 11 commits into
Conversation
Implement a read-only MCP server (Streamable HTTP at /mcp + SSE fallback at /mcp/sse) mounted only when enabled and a token is configured. Adds the mcp_configs table, GET/PUT /api/configs/mcp (token encrypted like vector search), SqliteMcpProvider, tool registration (conditional semantic search), and the Express mount. Token comparison uses constant-time timingSafeEqual.
Add McpConfig type + store actions/normalization, backendAdapter and autoSync push/pull for mcpConfig, a gated 'MCP服务' settings tab with enable toggle, endpoint display, token reset+copy and one-click client JSON copy. Electron renderer bridge pushes data snapshots and delegates semantic search to the renderer's vector index.
Rewrite build-desktop.js to install @modelcontextprotocol/sdk + zod into electron/, embed a self-contained mcpServer.mjs (Streamable HTTP + SSE fallback, snapshot-based provider, semantic search delegated to the renderer), and wire IPC (mcp:start/stop/snapshot/semanticSearch). SSE endpoints now require the bearer token; MCP auto-starts on launch when previously enabled.
Publish the backend container port to the host (default 3000) so local MCP clients (e.g. Claude Desktop, Cursor) can reach http://localhost:3000/mcp, while keeping the internal expose for the frontend.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable MCP access across the Express backend, Electron desktop runtime, and frontend, including authenticated transports, repository tools, semantic search, persisted configuration, synchronization, and settings UI. It also makes persisted snapshot selection freshness-aware across IndexedDB and localStorage. ChangesMCP integration
Storage resilience
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant ElectronMain
participant McpServer
participant McpClient
Renderer->>ElectronMain: Start MCP with port and token
ElectronMain->>McpServer: Start MCP service
McpClient->>McpServer: Send authenticated request
McpServer->>ElectronMain: Request semantic search
ElectronMain->>Renderer: Execute renderer search bridge
Renderer-->>ElectronMain: Return ranked repositories
ElectronMain-->>McpServer: Return search results
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 Model Context Protocol (MCP) server support to the GitHub Stars Manager, enabling AI agents to query starred repositories, releases, categories, tags, and statistics via Streamable HTTP or SSE. The implementation spans Docker Compose configurations, an Electron main-process integration, a SQLite-backed Express MCP server, and a frontend settings panel with a data-pushing bridge. The code review highlights several critical issues, including pagination bugs caused by post-query in-memory tag filtering, a crash risk from accessing non-existent properties on Cohere embeddings, performance degradation due to redundant heavy snapshot builds on every UI state change, unhandled decryption exceptions, resetting of creation timestamps via INSERT OR REPLACE, and a lack of upper-bound validation on the TCP port range.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+124
to
+129
| if (filter.tags && filter.tags.length) { | ||
| const tags = filter.tags.map((t) => t.toLowerCase()); | ||
| results = results.filter((r) => | ||
| [...r.customTags, ...r.aiTags, ...r.topics].some((t) => tags.includes(t.toLowerCase())) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Filtering by tags in memory after applying LIMIT and OFFSET in the SQL query will cause pagination issues. If the matching repositories with the specified tags are not within the first limit rows returned by the database, they will be completely missed, resulting in incomplete or empty results.
To fix this, the tag filtering should be moved into the SQL query using LIKE clauses on topics, custom_tags, and ai_tags before the query is executed (around line 101).
return Promise.resolve(results);| apiType === 'cohere' ? { model, texts: [text], input_type: 'search_query' } : { input: [text], model }, | ||
| { headers: { Authorization: `Bearer ${apiKey}`, 'Content-Type': 'application/json' }, timeout: 15000 } | ||
| ); | ||
| if (apiType === 'cohere') return (res.data?.embeddings?.[0]?.values as number[]) ?? null; |
There was a problem hiding this comment.
The Cohere /v1/embed API returns a 2D array of floats directly under the embeddings key (i.e., number[][]). It does not wrap the embedding vector in an object with a values property. Accessing .values on the first element of embeddings will evaluate to undefined, causing the semantic search to fail.
Suggested change
| if (apiType === 'cohere') return (res.data?.embeddings?.[0]?.values as number[]) ?? null; | |
| if (apiType === 'cohere') return (res.data?.embeddings?.[0] as number[]) ?? null; |
Comment on lines
+155
to
+165
| useAppStore.subscribe((state, prev) => { | ||
| if (state.mcpConfig.enabled && (!prev.mcpConfig.enabled || state.mcpConfig.token !== prev.mcpConfig.token)) { | ||
| // 刚启用或令牌变更，立即推送一次 | ||
| window.electronAPI?.startMcp?.({ port: state.mcpConfig.port, token: state.mcpConfig.token }); | ||
| window.electronAPI?.pushMcpSnapshot?.(buildSnapshot()); | ||
| return; | ||
| } | ||
| if (!state.mcpConfig.enabled) return; | ||
| if (debounceTimer) clearTimeout(debounceTimer); | ||
| debounceTimer = setTimeout(push, 2000); | ||
| }); |
There was a problem hiding this comment.
The store subscription currently triggers on any state change in useAppStore (including UI state, theme, search filters, etc.). Since buildSnapshot() is a heavy operation that serializes all repositories and releases, debouncing and executing this on every minor UI interaction can cause noticeable performance degradation and UI lag.
We should add a reference equality check to only trigger the update when the actual data (repositories, releases, categories, or MCP config) has changed.
useAppStore.subscribe((state, prev) => {
const hasDataChanged =
state.repositories !== prev.repositories ||
state.releases !== prev.releases ||
state.customCategories !== prev.customCategories ||
state.mcpConfig !== prev.mcpConfig;
if (!hasDataChanged) return;
if (state.mcpConfig.enabled && (!prev.mcpConfig.enabled || state.mcpConfig.token !== prev.mcpConfig.token)) {
// 刚启用或令牌变更，立即推送一次
window.electronAPI?.startMcp?.({ port: state.mcpConfig.port, token: state.mcpConfig.token });
window.electronAPI?.pushMcpSnapshot?.(buildSnapshot());
return;
}
if (!state.mcpConfig.enabled) return;
if (debounceTimer) clearTimeout(debounceTimer);
debounceTimer = setTimeout(push, 2000);
});| .get(v.embeddingConfigId) as Record<string, unknown> | undefined; | ||
| if (!embRow) return []; | ||
|
|
||
| const apiKey = embRow.api_key_encrypted ? decrypt(embRow.api_key_encrypted as string, config.encryptionKey) : ''; |
There was a problem hiding this comment.
The decrypt call for apiKey is not wrapped in a try-catch block. If decryption fails (e.g., due to a corrupted key or configuration mismatch), it will throw an unhandled exception and reject the promise, failing the entire semantic search tool execution.
Suggested change
| const apiKey = embRow.api_key_encrypted ? decrypt(embRow.api_key_encrypted as string, config.encryptionKey) : ''; | |
| let apiKey = ''; | |
| if (embRow.api_key_encrypted) { | |
| try { | |
| apiKey = decrypt(embRow.api_key_encrypted as string, config.encryptionKey); | |
| } catch { | |
| return []; | |
| } | |
| } |
Comment on lines
+813
to
+816
| db.prepare(` | ||
| INSERT OR REPLACE INTO mcp_configs (id, enabled, port, token_encrypted, updated_at) | ||
| VALUES (?, ?, ?, ?, datetime('now')) | ||
| `).run('default', isEnabled ? 1 : 0, mcpPort, encryptedToken); |
There was a problem hiding this comment.
Using INSERT OR REPLACE in SQLite performs a DELETE followed by an INSERT on conflict. This causes any columns not explicitly specified in the query (such as created_at) to revert to their default values (which is datetime('now')), resetting the creation timestamp every time the configuration is updated.
Using INSERT INTO ... ON CONFLICT(id) DO UPDATE (upsert) preserves the original created_at timestamp.
db.prepare("INSERT INTO mcp_configs (id, enabled, port, token_encrypted, updated_at) VALUES (?, ?, ?, ?, datetime('now')) ON CONFLICT(id) DO UPDATE SET enabled = excluded.enabled, port = excluded.port, token_encrypted = excluded.token_encrypted, updated_at = datetime('now')").run('default', isEnabled ? 1 : 0, mcpPort, encryptedToken);| } | ||
|
|
||
| const isEnabled = enabled === true || enabled === 1; | ||
| const mcpPort = typeof port === 'number' && port > 0 ? port : 0; |
There was a problem hiding this comment.
The port number is only validated to be > 0. If a user enters an invalid port number greater than 65535, the server will fail to bind and crash. We should validate that the port is within the valid TCP port range (1 to 65535).
Suggested change
| const mcpPort = typeof port === 'number' && port > 0 ? port : 0; | |
| const mcpPort = typeof port === 'number' && port > 0 && port <= 65535 ? port : 0; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/services/autoSync.ts (1)
437-452: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
mcpConfigis missing from the store subscription change detector.The
changedexpression instartAutoSyncchecks every other synced slice (repositories,releases,aiConfigs, etc.) but omitsstate.mcpConfig !== prevState.mcpConfig. Local MCP changes therefore never trigger the debouncedsyncToBackend, and_lastHash.mcpis never updated after a local change. Consequences:
- Every 5s poll after a local MCP change sees a hash mismatch, causing an unnecessary
setMcpConfigre-set.- If the direct
backend.syncMcpConfiginMcpSettings.applyConfigfails (silently caught with.catch(() => undefined)), there is no retry path — the next poll overwrites the local change with stale backend data.🔧 Proposed fix: add mcpConfig to the subscription check
const changed = state.repositories !== prevState.repositories || state.releases !== prevState.releases || state.aiConfigs !== prevState.aiConfigs || state.webdavConfigs !== prevState.webdavConfigs || state.embeddingConfigs !== prevState.embeddingConfigs || state.vectorSearchConfig !== prevState.vectorSearchConfig || + state.mcpConfig !== prevState.mcpConfig || state.activeAIConfig !== prevState.activeAIConfig ||🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/autoSync.ts` around lines 437 - 452, Add state.mcpConfig !== prevState.mcpConfig to the changed expression in startAutoSync, alongside the other synced store slices, so local MCP configuration changes trigger the existing debounced syncToBackend flow.src/App.tsx (1)
145-158: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDelay
initMcpRendererBridge()until after backend syncstartMcp()is called from the bridge with the locally hydratedmcpConfig, and the subscription only restarts on enable/token changes. A backend-only port update can leave MCP bound to the old port while the store/UI shows the new one.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/App.tsx` around lines 145 - 158, Move initMcpRendererBridge() in initBackend to run only after the backend sync completes and the cancellation check passes, ensuring the bridge starts with the locally hydrated mcpConfig and current backend port. Keep backend initialization and sync error handling unchanged.
🧹 Nitpick comments (5)
src/services/mcpRendererBridge.ts (1)
112-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
initMcpRendererBridgeleaks its store subscription and debounce timer.The function creates a
useAppStore.subscribelistener (line 155) and adebounceTimer(line 141) but returnsvoid— there is no way to unsubscribe or clear the timer. If the caller invokes this function more than once (e.g., a React re-mount in development or a hot reload), multiple subscriptions accumulate, causing duplicate snapshot pushes andstartMcpcalls.♻️ Proposed refactor: return a cleanup function
-export function initMcpRendererBridge(): void { +export function initMcpRendererBridge(): () => void { if (!isElectron()) return; // ... existing setup ... - useAppStore.subscribe((state, prev) => { + const unsubscribe = useAppStore.subscribe((state, prev) => { // ... existing logic ... }); + + return () => { + unsubscribe(); + if (debounceTimer) clearTimeout(debounceTimer); + }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/mcpRendererBridge.ts` around lines 112 - 166, Update initMcpRendererBridge to return a cleanup function instead of void. Capture the useAppStore.subscribe unsubscribe handle and, on cleanup, unsubscribe the listener, clear any pending debounceTimer, and reset the timer reference so repeated initialization or hot reload does not accumulate subscriptions or duplicate MCP actions.src/store/useAppStore.ts (1)
674-693: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
setMcpConfigbypasses port sanity-checking done innormalizeMcpConfig.
mergeMcpConfigjust spreads the patch over current state without re-validatingport(must be a positive integer).normalizeMcpConfigenforces this only during hydration/migration, so any caller ofsetMcpConfig(e.g. a bad UI input) can persist an invalid port (0, negative, non-integer, or out of the 1–65535 TCP range) that later gets pushed to the backend and potentially used to bind a real listener in Electron.♻️ Suggested fix
- setMcpConfig: (config) => set((state) => ({ - mcpConfig: mergeMcpConfig(state.mcpConfig, config) - })), + setMcpConfig: (config) => set((state) => ({ + mcpConfig: normalizeMcpConfig(mergeMcpConfig(state.mcpConfig, config)) + })),Also applies to: 1540-1544
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/useAppStore.ts` around lines 674 - 693, Update mergeMcpConfig so every patched port is revalidated using the same positive-integer rules as normalizeMcpConfig, including the 1–65535 TCP range, and invalid values fall back to DEFAULT_MCP_PORT. Ensure setMcpConfig cannot persist an invalid port while preserving existing values when port is omitted.server/src/mcp/tools.ts (2)
17-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
offsetisn't exposed, so agents can't page past the firstlimitresults.
McpListFilter.offsetexists and is honored bySqliteMcpProvider.listRepositories, butsearch_repositories'sinputSchemanever exposes it, capping any single query atlimit(max 200) results with no way to retrieve subsequent pages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/mcp/tools.ts` around lines 17 - 28, The search_repositories inputSchema is missing the pagination offset. Add an optional numeric offset field to the schema, matching the existing McpListFilter.offset type and behavior, so it is passed through to SqliteMcpProvider.listRepositories and allows retrieving subsequent pages beyond the limit.
54-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNot-found responses should set
isError: trueper MCP tool-result convention.
get_repository/get_repository_commentsreturnjson({ error: ... })as a normal (non-error) tool result. Per the MCP spec, tool execution failures should be surfaced viaCallToolResult.isError, so clients/LLMs can reliably detect and react to failures instead of having to parse response text for an "error" key.♻️ Suggested fix
async (args) => { const repo = await provider.getRepository(args.fullName); - if (!repo) return json({ error: `Repository not found: ${args.fullName}` }); + if (!repo) { + return { ...json({ error: `Repository not found: ${args.fullName}` }), isError: true }; + } return json(repo); }Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/mcp/tools.ts` around lines 54 - 59, Update the not-found branches in the get_repository and get_repository_comments tool handlers to return an MCP tool result with isError set to true alongside the existing error payload. Keep successful repository responses unchanged.server/src/mcp/index.ts (1)
12-26: 🔒 Security & Privacy | 🔵 TrivialNo rate limiting on the bearer-token check.
checkTokenperforms a constant-time comparison, but there's no throttling/backoff on repeated failed attempts against/mcpor/mcp/sse, and the port is published for local Docker access per the PR summary. Consider basic rate limiting on repeated 401s for defense-in-depth, especially since a leaked/guessed token exposes all starred-repo data and notes.Also applies to: 69-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/mcp/index.ts` around lines 12 - 26, Add basic rate limiting for repeated failed bearer-token checks in checkToken, covering both /mcp and /mcp/sse requests. Track failed attempts by an appropriate client identifier, throttle or temporarily block excessive 401 responses, and preserve tokenMatches constant-time comparison and normal successful-request behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 20-23: Update the backend service port mapping under ports to bind
the host side explicitly to 127.0.0.1, while preserving the configurable
BACKEND_HOST_PORT value and container port 3000.
In `@scripts/build-desktop.js`:
- Around line 378-385: Update the http.createServer request handler to
authenticate POST/PUT requests before reading their bodies, then buffer chunks
only up to a defined byte limit and immediately return status 413 when exceeded.
Move stream consumption inside appropriate error handling so aborted or failed
requests are handled without an unhandled rejection, while preserving existing
JSON parsing behavior for accepted bodies.
- Around line 439-462: Update the desktop build configuration around
electronPackageJson and its generated package.json to declare pinned
devDependencies for both electron and electron-builder, using the versions
expected by the existing npx electron-builder build flow. Keep the MCP SDK and
zod entries under dependencies, and ensure npm install in electronDir installs
the declared build toolchain rather than relying on an implicit on-demand
install.
- Around line 125-157: Register the `mcp:start`, `mcp:stop`, and `mcp:snapshot`
IPC handlers before calling `createWindow()` in the `app.whenReady()` startup
flow. Ensure the async `mcpServer.mjs` import completes and handlers are
installed before renderer initialization, so the persisted MCP state’s initial
snapshot and startup request cannot race handler registration.
- Around line 377-401: Move the sseTransports map from startMcp’s local scope to
module scope so stopMcp can access active /mcp/sse sessions. Update stopMcp to
close every stored SSE transport before awaiting httpServer.close(), and clear
the map afterward while preserving the existing session cleanup behavior.
- Around line 424-425: Update the server startup flow in startMcp around
httpServerInstance.listen to attach a one-time error listener that rejects the
startup promise, while resolving on successful listening. Ensure asynchronous
errors such as EADDRINUSE are propagated so mcp:start surfaces the failure
cleanly, and preserve assigning httpServer after successful startup.
- Around line 358-375: Update handleRequest to instantiate and configure a fresh
McpServer for each transport request before calling server.connect(transport),
rather than sharing the server created in startMcp across requests or endpoints.
Remove the redundant transport.start() call if present, relying on connect() to
start the transport.
In `@server/src/mcp/index.ts`:
- Around line 33-65: Update initMcp so MCP endpoint behavior reads the current
database configuration at request time, rather than capturing enabled,
token_encrypted, and vector search settings during startup. Ensure toggling MCP,
rotating the token, or changing vector_search_configs through PUT
/api/configs/mcp takes effect without restarting or remounting the application.
- Around line 67-68: Update the MCP request/connection handling around
createMcpServer so each /mcp request and /mcp/sse connection constructs a fresh
McpServer instance with the current MCP enable, token, and vector settings. Do
not reuse a startup-level server or provider across transports; retain the
existing database/provider setup as needed while ensuring settings are reloaded
for every connection.
In `@server/src/mcp/sqliteProvider.ts`:
- Around line 72-131: Update listRepositories so filter.tags is applied before
pagination: when tags are present, retrieve the candidate rows without SQL
LIMIT/OFFSET, map and filter them by customTags, aiTags, and topics, then apply
the requested offset and limit to the filtered results; preserve the existing
SQL pagination path when no tags are specified.
In `@src/components/settings/McpSettings.tsx`:
- Around line 177-181: Update the port input and its onChange handler in
McpSettings to enforce the valid TCP port range of 1–65535: add matching input
constraints and clamp or reject values outside that range before applyConfig
receives them. Preserve the existing 18789 fallback for empty or invalid input.
- Around line 47-63: Update the MCP startup calls in applyConfig and the initial
restart path in mcpRendererBridge to use the shared fallback port when the
normalized configuration port is 0 or otherwise unset. Keep the UI and backend
configuration values unchanged while ensuring startMcp receives the same
fallback port displayed or expected by the application.
In `@src/services/autoSync.ts`:
- Around line 252-255: Update the fulfilled MCP branch in autoSync around
fetchMcpConfig and setMcpConfig to preserve the existing local token when the
backend token is empty or equals decrypt_failed. Before applying the result,
merge in the current MCP token for those cases, while retaining the backend
token for valid values and preserving the existing hash update behavior.
---
Outside diff comments:
In `@src/App.tsx`:
- Around line 145-158: Move initMcpRendererBridge() in initBackend to run only
after the backend sync completes and the cancellation check passes, ensuring the
bridge starts with the locally hydrated mcpConfig and current backend port. Keep
backend initialization and sync error handling unchanged.
In `@src/services/autoSync.ts`:
- Around line 437-452: Add state.mcpConfig !== prevState.mcpConfig to the
changed expression in startAutoSync, alongside the other synced store slices, so
local MCP configuration changes trigger the existing debounced syncToBackend
flow.
---
Nitpick comments:
In `@server/src/mcp/index.ts`:
- Around line 12-26: Add basic rate limiting for repeated failed bearer-token
checks in checkToken, covering both /mcp and /mcp/sse requests. Track failed
attempts by an appropriate client identifier, throttle or temporarily block
excessive 401 responses, and preserve tokenMatches constant-time comparison and
normal successful-request behavior.
In `@server/src/mcp/tools.ts`:
- Around line 17-28: The search_repositories inputSchema is missing the
pagination offset. Add an optional numeric offset field to the schema, matching
the existing McpListFilter.offset type and behavior, so it is passed through to
SqliteMcpProvider.listRepositories and allows retrieving subsequent pages beyond
the limit.
- Around line 54-59: Update the not-found branches in the get_repository and
get_repository_comments tool handlers to return an MCP tool result with isError
set to true alongside the existing error payload. Keep successful repository
responses unchanged.
In `@src/services/mcpRendererBridge.ts`:
- Around line 112-166: Update initMcpRendererBridge to return a cleanup function
instead of void. Capture the useAppStore.subscribe unsubscribe handle and, on
cleanup, unsubscribe the listener, clear any pending debounceTimer, and reset
the timer reference so repeated initialization or hot reload does not accumulate
subscriptions or duplicate MCP actions.
In `@src/store/useAppStore.ts`:
- Around line 674-693: Update mergeMcpConfig so every patched port is
revalidated using the same positive-integer rules as normalizeMcpConfig,
including the 1–65535 TCP range, and invalid values fall back to
DEFAULT_MCP_PORT. Ensure setMcpConfig cannot persist an invalid port while
preserving existing values when port is omitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28746a8b-b7e1-4ccd-9b99-8067e7ea9ea7
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
docker-compose.ymlscripts/build-desktop.jsserver/package.jsonserver/src/db/schema.tsserver/src/index.tsserver/src/mcp/index.tsserver/src/mcp/server.tsserver/src/mcp/sqliteProvider.tsserver/src/mcp/tools.tsserver/src/mcp/types.tsserver/src/routes/configs.tssrc/App.tsxsrc/components/SettingsPanel.tsxsrc/components/settings/McpSettings.tsxsrc/components/settings/index.tssrc/services/autoSync.tssrc/services/backendAdapter.tssrc/services/electronProxy.tssrc/services/mcpRendererBridge.tssrc/store/useAppStore.tssrc/types/index.ts
- sqliteProvider: filter tags before pagination, fix Cohere embeddings shape, guard decrypt(apiKey) failure - mcp/index: read config per request, fresh McpServer per connection - configs: UPSERT preserves created_at; validate port 1-65535 - mcpRendererBridge: skip snapshot rebuild when data unchanged - autoSync: preserve local MCP token on decrypt_failed/empty - McpSettings: default port fallback + clamp input 1-65535 - build-desktop: close SSE transports on stop, auth before body buffer with 1MiB cap (413), handle listen EADDRINUSE, restore electron/electron-builder install step Refs #247
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/mcp/index.ts`:
- Around line 131-133: Remove the redundant transport.start() call from the SSE
handler after server.connect(transport) in the server setup flow. Keep await
server.connect(transport) as the sole transport initialization step so /mcp/sse
requests do not attempt to start the transport twice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23143ec0-f8c9-4362-a5b5-27c88c7b9efa
📒 Files selected for processing (7)
scripts/build-desktop.jsserver/src/mcp/index.tsserver/src/mcp/sqliteProvider.tsserver/src/routes/configs.tssrc/components/settings/McpSettings.tsxsrc/services/autoSync.tssrc/services/mcpRendererBridge.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/src/routes/configs.ts
- src/services/mcpRendererBridge.ts
- src/services/autoSync.ts
- src/components/settings/McpSettings.tsx
- server/src/mcp/sqliteProvider.ts
- index.ts / mcpServer.mjs: drop redundant SSE transport.start() after server.connect() (avoids ERR_HTTP_HEADERS_SENT / 500s on /mcp/sse) - mcpRendererBridge: use 18789 port fallback for startMcp calls; include mcpConfig in snapshot rebuild reference-equality guard - build-desktop: register MCP IPC handlers before createWindow() so persisted enabled-state can't race handler registration; pin electron + electron-builder as devDependencies (drop implicit on-demand install) - docker-compose: bind backend host port to 127.0.0.1 (loopback) Refs #247
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build-desktop.js (1)
186-477: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInstantiate
McpServerinside each transport handler
startMcp()still creates oneMcpServerand reuses it for both/mcpand/mcp/sse; mirrorserver/src/mcp/index.tsand build a fresh server per request so one transport can’t interfere with the next. Thetransport.start()call is already gone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-desktop.js` around lines 186 - 477, Update startMcp so it no longer creates or shares one McpServer across requests. In each /mcp and /mcp/sse transport handler, instantiate a fresh McpServer and call registerTools on it before connecting the transport; keep the existing transport lifecycle and leave the already-removed transport.start() behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/build-desktop.js`:
- Around line 186-477: Update startMcp so it no longer creates or shares one
McpServer across requests. In each /mcp and /mcp/sse transport handler,
instantiate a fresh McpServer and call registerTools on it before connecting the
transport; keep the existing transport lifecycle and leave the already-removed
transport.start() behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 267e33f1-3f6a-4766-a8e4-e0e6e077df63
📒 Files selected for processing (5)
docker-compose.ymlpackage.jsonscripts/build-desktop.jsserver/src/mcp/index.tssrc/services/mcpRendererBridge.ts
💤 Files with no reviewable changes (1)
- server/src/mcp/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/services/mcpRendererBridge.ts
- docker-compose.yml
Regenerate lock so CI `npm ci` passes after pinning electron + electron-builder as devDependencies in package.json. Refs #247
…pshot, unload-safe mirror)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 128-143: Replace the snapshotRichness-based selection in the
IndexedDB/localStorage restore flow with explicit write generation or timestamp
metadata persisted alongside every snapshot, validate that metadata, and select
the newest valid snapshot regardless of collection sizes. Use snapshotRichness
only for one-time legacy recovery when neither copy has freshness metadata, then
repair the older or divergent store from the selected snapshot in the logic
surrounding isNonEmptySnapshot, the restore selection block, and persistence
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac339bf5-1b9d-44f8-9b40-44197fc17898
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/services/indexedDbStorage.ts
…e (CodeRabbit r3569671054)
…ty-write guard Root cause: zustand v4 hydration is async and races with syncFromBackend() (called in App.tsx useEffect). Before hydration finishes, state.repositories is the empty initial value, which defeats the isBootstrapEmpty guard in autoSync and lets an empty setRepositories() overwrite good IndexedDB data on the next persist write (the 1s debounce makes the empty write land after the good-data write). This is a pre-existing race on main; this build was the one that hit the window. Fixes: - F1 (root cause): App.tsx waits for hasHydrated before calling syncFromBackend(); autoSync.syncFromBackend() early-returns if !hasHydrated (defense in depth). - F2 (backstop): indexedDbStorage.setItem refuses to overwrite a non-empty snapshot with an empty/invalid one. - F3 (durability): latestValue + pagehide/visibilitychange synchronous localStorage mirror already keeps the freshest value safe across teardown. - F4 (regression lock): tests for the empty-write guard and the sync hydration gate. Verified: tsc --noEmit clean, eslint clean, full vitest 114/114.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
src/services/indexedDbStorage.test.ts (1)
37-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the actual freshness, repair, and unload paths.
Disabling IndexedDB means this suite never exercises timestamp selection or cross-store repair. Add tests for:
- A newer, smaller snapshot winning.
- localStorage-to-IndexedDB repair preserving the exact timestamp.
- A rejected empty write followed by
visibilitychange/pagehide.- IndexedDB failure with localStorage fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/indexedDbStorage.test.ts` around lines 37 - 67, Extend the indexedDBStorage tests beyond the forced localStorage-only setup to cover freshness and fallback behavior: verify a newer smaller snapshot is selected, localStorage-to-IndexedDB repair preserves the exact timestamp, a rejected empty write remains protected across visibilitychange and pagehide, and IndexedDB failures use localStorage fallback. Reuse the existing indexedDBStorage, readState, mockLocalStorage, and mem setup, and ensure each test exercises the corresponding event or storage failure path.src/components/settings/McpSettings.tsx (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider memoizing
isElectronModefor stability.
isElectron()is called on every render to produceisElectronMode, but the result never changes within a session. Moving it to auseMemoor module-level constant avoids redundant calls and makes the intent clearer.♻️ Suggested refactor
- const isElectronMode = isElectron(); + const isElectronMode = useMemo(() => isElectron(), []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/settings/McpSettings.tsx` at line 45, Update the isElectronMode initialization in McpSettings to avoid calling isElectron() on every render, using a module-level constant or useMemo with stable dependencies. Preserve the existing boolean behavior throughout the component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/db/schema.ts`:
- Line 141: The MCP configuration port default in the schema must match the
API’s accepted range and synchronization behavior. Update the port default near
the MCP configuration schema to use 18789, and ensure the missing-row GET
fallback uses the same default so GET and PUT remain consistent.
In `@server/src/mcp/index.ts`:
- Around line 90-100: Move loadMcpState(), the missing-state response, and token
authorization checks inside the existing try blocks for the async handlers
app.all('/mcp'), app.get('/mcp/sse'), and app.post('/mcp/sse/messages'). Ensure
synchronous SQLite/configuration failures are caught by those handlers and reach
the existing JSON 500 error response, preserving current 404 and unauthorized
behavior.
In `@server/src/mcp/sqliteProvider.ts`:
- Around line 169-251: Update semanticSearch and embedQuery so embedding and
vector-worker exceptions are logged and propagated as sanitized tool failures
instead of being converted to [] or null. Preserve [] only for valid searches
that produce no matches, while retaining the existing precondition checks and
result filtering.
In `@server/src/mcp/tools.ts`:
- Line 27: Update the schemas for search_repositories.limit,
list_releases.limit, and semantic_search_repositories.topK to require integers
and enforce their documented minimum and maximum bounds using the existing Zod
number validation chain. Preserve the optional/default behavior while preventing
fractional or out-of-range values from reaching SQLite LIMIT or the vector
search payload.
- Around line 17-42: Expose an optional offset parameter in the repository-list
tool’s inputSchema, then pass args.offset into the provider.listRepositories
call alongside the existing filters. Use the existing McpListFilter-supported
offset behavior without changing other pagination parameters.
In `@server/src/routes/configs.ts`:
- Around line 799-828: After computing isEnabled in the configuration update
flow, reject requests where isEnabled is true and encryptedToken is empty by
returning a 400 response before the database upsert. Include an appropriate
error payload, and keep the existing validation and persistence behavior
unchanged for valid enabled configurations and disabled configurations.
In `@src/services/backendAdapter.ts`:
- Around line 669-678: Update the MCP configuration request in the backend
adapter method containing the `/configs/mcp?decrypt=true` fetch so its decrypted
response body is not emitted by fetchWithTimeout debug logging. Mark this
request as sensitive using the existing fetchWithTimeout sensitivity mechanism,
or redact the token field before response-body logging while preserving the
returned plaintext token for callers.
In `@src/services/indexedDbStorage.ts`:
- Around line 272-275: Update the IndexedDB repair branch in the
selected-versus-raw reconciliation flow to pass selected directly to idbSet
instead of wrapping it with stamp. Preserve the selected snapshot’s original
timestamp while leaving the existing timeout and error handling unchanged.
- Around line 291-310: Update the empty-snapshot rejection path around
latestValue and registerFlushListeners so a rejected write is not retained for
later lifecycle flushes. Clear or restore latestValue before returning when
existing non-empty data is detected, and ensure visibilitychange, pagehide, and
beforeunload cannot persist the rejected snapshot.
In `@src/services/mcpRendererBridge.ts`:
- Around line 155-175: Update the useAppStore subscription to invoke the MCP
stop lifecycle IPC when mcpConfig.enabled changes from true to false before
returning. Extend the existing start/restart condition to include a port change,
ensuring the running server is restarted with the new port while preserving the
immediate snapshot push for enable and token changes.
- Around line 124-138: Update the semantic-search try/catch around
EmbeddingClient, VectorSearchService, and their embed/query calls to stop
converting delegated failures into successful empty results. In the catch block,
reject by throwing a sanitized error that exposes no sensitive details, while
preserving the existing empty-array return for valid zero-result searches.
In `@src/store/useAppStore.ts`:
- Around line 2236-2237: Update the partialize persistence logic around
mcpConfig so snapshots retain only the enabled and port settings, excluding the
bearer token entirely. Ensure runtime configuration still retrieves the token
from encrypted backend storage or OS-backed secret storage in desktop mode,
rather than from persisted state.
---
Nitpick comments:
In `@src/components/settings/McpSettings.tsx`:
- Line 45: Update the isElectronMode initialization in McpSettings to avoid
calling isElectron() on every render, using a module-level constant or useMemo
with stable dependencies. Preserve the existing boolean behavior throughout the
component.
In `@src/services/indexedDbStorage.test.ts`:
- Around line 37-67: Extend the indexedDBStorage tests beyond the forced
localStorage-only setup to cover freshness and fallback behavior: verify a newer
smaller snapshot is selected, localStorage-to-IndexedDB repair preserves the
exact timestamp, a rejected empty write remains protected across
visibilitychange and pagehide, and IndexedDB failures use localStorage fallback.
Reuse the existing indexedDBStorage, readState, mockLocalStorage, and mem setup,
and ensure each test exercises the corresponding event or storage failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d417b609-64ce-4405-8494-4a7ef107cd39
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonserver/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
docker-compose.ymlpackage.jsonscripts/build-desktop.jsserver/package.jsonserver/src/db/schema.tsserver/src/index.tsserver/src/mcp/index.tsserver/src/mcp/server.tsserver/src/mcp/sqliteProvider.tsserver/src/mcp/tools.tsserver/src/mcp/types.tsserver/src/routes/configs.tssrc/App.tsxsrc/components/SettingsPanel.tsxsrc/components/settings/McpSettings.tsxsrc/components/settings/index.tssrc/services/autoSync.test.tssrc/services/autoSync.tssrc/services/backendAdapter.tssrc/services/electronProxy.tssrc/services/indexedDbStorage.test.tssrc/services/indexedDbStorage.tssrc/services/mcpRendererBridge.tssrc/store/useAppStore.tssrc/types/index.ts
- indexedDbStorage: 修复巡检(repair)保留原时间戳；空写闸门拒绝后不再缓存 latestValue，避免 pagehide/visibilitychange 绕过闸门写入空快照 - server mcp: 每请求/连接创建独立 McpServer；loadMcpState 与鉴权移入 try；语义搜索失败显式抛出而非返回空结果；tools 暴露 offset 并将 limit/topK 约束为整数 - configs: MCP 默认端口统一为 18789；启用 MCP 但缺少 token 时拒绝(400) - backendAdapter: MCP token 拉取请求标记为敏感，debug 日志脱敏 - mcpRendererBridge: 语义搜索失败抛出；禁用/改端口时正确停止或重启内嵌服务 - useAppStore: 快照不再持久化 MCP bearer token 明文 - schema: mcp_configs 端口默认值 18789
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the "add a CLI for agents" idea from #164 with a seamless in-app MCP (Model Context Protocol) server so AI agents (Claude Desktop, Cursor, etc.) can read-only access the user's curated starred repositories — no manual CLI/third-party install.
/mcp) + SSE fallback (/mcp/sse), mounted only when enabled and a token is set. Bearer-token auth (constant-time compare). Tools:search_repositories,get_repository,get_repository_comments,list_categories,list_tags,list_releases,get_stats, andsemantic_search_repositories(registered only when vector search is configured). Token stored encrypted (mcp_configstable), mirrored on the existing vector-search encryption pattern.type: "http"). Hidden in pure-frontend mode.build-desktop.jsinstalls the SDK intoelectron/, embeds a self-containedmcpServer.mjs(snapshot-based provider; semantic search delegated to the renderer's vector index), and wires IPC. Binds127.0.0.1only; auto-starts on launch when previously enabled.docker-compose.ymlnow publishes the backend port to the host so local MCP clients can reachhttp://localhost:3000/mcp.Related issue
Closes #164 (original request was a CLI for agent access; MCP chosen for a seamless, no-install experience).
Audit fixes (this branch)
crypto.timingSafeEqual) in both backend and Electron, matching existing API auth.Type of change
Testing
npx tsc --noEmit(server) — cleannpx tsc -p tsconfig.app.json --noEmit(web) — clean (only a pre-existingautoSync.ts:243vector-search error unrelated to this change remains)eslinton changed files — cleanvite build— succeedsmcpServer.mjsextracted and syntax-validatedNotes
Summary by CodeRabbit