refactor: mcp store split from settings store #502
Conversation
WalkthroughThe changes migrate the management of the MCP installation cache from the settings store to a dedicated MCP store. All cache state and operations are removed from the settings store and implemented in the MCP store, with corresponding updates to the component code to use the new MCP store API. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant McpServersComponent
participant McpStore
participant SettingsStore
User->>McpServersComponent: Interacts with MCP install cache
McpServersComponent->>McpStore: Access or clear mcpInstallCache
McpServersComponent--xSettingsStore: (No longer interacts with settingsStore for cache)
SettingsStore->>McpStore: setMcpInstallCache(mcpConfig) on MCP install deeplink
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/src/components/mcp-config/components/McpServers.vue (1)
382-383: Prop type uncertainty
default-json-confignow receivesmcpStore.mcpInstallCachewhich is typed asstring | nullin the store.
IfMcpServerFormexpects an object (JSON parsed) or a raw string but sometimes receivesnull, add explicit typing or parsing here to avoid runtime surprises.Example quick guard:
-:default-json-config="mcpStore.mcpInstallCache || undefined" +:default-json-config="mcpStore.mcpInstallCache ?? undefined"…but ideally expose
mcpInstallCacheas the correct parsed type.src/renderer/src/stores/settings.ts (1)
178-208: Reuse the store instance instead of recreating it for every deeplink eventInside the MCP-install deeplink handler you call
const mcpStore = useMcpStore()on every invocation.
useMcpStore()will always return the same Pinia instance, but paying the cost (and creating a new proxy wrapper) each time is unnecessary.-// keep this at module scope -const mcpStore = useMcpStore() -window.electron.ipcRenderer.on(DEEPLINK_EVENTS.MCP_INSTALL, async (_, data) => { - ... - mcpStore.setMcpInstallCache(mcpConfig) -}) +const mcpStore = useMcpStore() + +window.electron.ipcRenderer.on(DEEPLINK_EVENTS.MCP_INSTALL, async (_, data) => { + ... + mcpStore.setMcpInstallCache(mcpConfig) +})Hoisting also makes the handler cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/src/components/mcp-config/components/McpServers.vue(3 hunks)src/renderer/src/stores/mcp.ts(3 hunks)src/renderer/src/stores/settings.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/renderer/src/stores/mcp.ts
[error] 563-564: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/renderer/src/components/mcp-config/components/McpServers.vue (1)
44-53: Watcher now targetsmcpStore.mcpInstallCache– confirm one-shot semanticsThe immediate watcher will run once on component mount and every time
mcpInstallCachetoggles from null ➜ truthy (dialog opens) or truthy ➜ null (nothing happens).
That exactly mimics the previous behaviour, but note:
- When the dialog is closed we clear the cache (
clearMcpInstallCache) after theisAddServerDialogOpenflag flips tofalse.
Because the watch on line 44 is only formcpInstallCache, re-opening the dialog by setting the flag back totruewill not trigger unlesssetMcpInstallCacheis called again.- If multiple
McpServersinstances were ever mounted concurrently, this logic would execute multiple times.Nothing is wrong, just make sure this “open-once per cache write” contract is what you want.
Also applies to: 55-61
src/renderer/src/stores/mcp.ts (1)
111-125: ```shell
#!/bin/bash
set -eDisplay context where mcpConfig is parsed and then sent to the renderer for MCP_INSTALL
sed -n '200,260p' src/main/presenter/deeplinkPresenter/index.ts
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| onMounted(async () => { | ||
| await init() | ||
| }) | ||
| onMounted(init) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated init calls risk multiple event subscriptions
onMounted(init) runs once for every component that instantiates the store.
If two components call useMcpStore(), all IPC listeners are registered twice.
Safer pattern:
let initialised = false
const init = async () => {
if (initialised) return
initialised = true
...
}
init() // run immediately onceand drop the onMounted hook unless you really need to defer until first mount.
🤖 Prompt for AI Agents
In src/renderer/src/stores/mcp.ts at line 519, the use of onMounted(init) causes
the init function to run each time a component uses the store, leading to
multiple IPC listener registrations. To fix this, remove the onMounted(init)
call and instead modify init to track if it has already run using a boolean flag
(e.g., initialised). Call init immediately once outside of any lifecycle hooks,
and inside init, return early if initialised is true to prevent repeated
executions and duplicate event subscriptions.
Pull Request Description (中文)
你的功能请求是否与某个问题有关？请描述一下。
link #431
Summary by CodeRabbit