feat(mcp_store): MCP brand logos and auto-refresh after connect#1900
Merged
Conversation
Connecting an MCP server creates the installation with tool_count: 0; tools only populate once /tools/refresh/ is called. Until now that required clicking the manual Refresh button. This routes the user to the installation detail panel once the install actually succeeds, and fires a silent refresh on arrival when the tool list is empty (deduped per installation for the session).
- Move logos from apps/code/public/services/ to apps/code/src/renderer/assets/services/ and switch the BRAND_ICONS map to ES imports. Absolute /services/... URL strings only resolve in Vite's dev server; in the packaged Electron app the renderer is loaded via file:// and the assets would 404 silently. ES imports match the existing convention (33 other @renderer/assets/* imports) and give Vite compile-time path validation. - Stabilize the autoRefreshIfEmpty effect's dep array (mutate fn is stable; use tools.length and isPending instead of fresh references). - Note module-scoped dedupe set with a comment for future test setup.
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/mcp/McpInstalledRail.tsx
Line: 176-177
Comment:
**Empty `icon_key` shadows template icon via `??`**
`installation.icon_key` is typed as `string` (non-nullable), so when the API returns an empty string (as `statusBadge.test.ts` demonstrates is the default), the nullish coalescing operator evaluates to the empty string rather than falling through to `template?.icon_key`. `resolveServerIcon` then returns `undefined` and the Plugs fallback renders instead of the brand logo. Replace `??` with `||` so that an empty string also falls through to the template's icon_key.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/mcp/ServerDetailView.tsx
Line: 73
Comment:
**Same `??` vs `||` bug for `iconKey`**
`installation?.icon_key` is `string` (non-nullable per the schema), so when it is an empty string the nullish coalescing operator short-circuits and `template?.icon_key` is never reached. The Plugs fallback renders even when the template has a valid brand icon. Replace `??` with `||` on both coalesces in this line so that empty strings also fall through.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/components/sections/McpServersSettings.tsx
Line: 134-143
Comment:
**Custom install: URL mismatch leaves user stuck on the form**
`pendingCustomUrl` is matched against `installationList` by strict equality (`i.url === pendingCustomUrl`). If the backend normalises the submitted URL (e.g. strips a trailing slash, lowercases the host), no installation will ever match, `pendingCustomUrl` remains set, and the user stays on the "Add Custom Server" form indefinitely even though the install succeeded. The old `onSuccess: () => setView(...)` was immune to this. Consider adding a fallback that navigates on the next successful install even when URL matching fails, or use the most-recently-added installation after success.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/hooks/useMcpInstallationTools.ts
Line: 23
Comment:
**Module-scoped `Set` not exported for tests**
The comment on `autoRefreshedInstallations` says "Tests that exercise auto-refresh need to clear this in `beforeEach`", but the `Set` is not exported. Test files cannot clear it without either importing the module's internals via a hacky side-channel or relying on module re-registration between test suites. Consider exporting a test-only reset helper (e.g. `export function _resetAutoRefreshedInstallations()`) or moving the deduplication into a ref/context to keep it instance-scoped and inherently isolated between test renders.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(code): bundle MCP brand logos via ES..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Module-scoped on purpose: state must survive remounts of this hook so a | ||
| // detail-page revisit doesn't re-fire the auto-refresh. Tests that exercise | ||
| // auto-refresh need to clear this in beforeEach. | ||
| const autoRefreshedInstallations = new Set<string>(); |
Contributor
There was a problem hiding this comment.
Set not exported for tests
The comment on autoRefreshedInstallations says "Tests that exercise auto-refresh need to clear this in beforeEach", but the Set is not exported. Test files cannot clear it without either importing the module's internals via a hacky side-channel or relying on module re-registration between test suites. Consider exporting a test-only reset helper (e.g. export function _resetAutoRefreshedInstallations()) or moving the deduplication into a ref/context to keep it instance-scoped and inherently isolated between test renders.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/settings/hooks/useMcpInstallationTools.ts
Line: 23
Comment:
**Module-scoped `Set` not exported for tests**
The comment on `autoRefreshedInstallations` says "Tests that exercise auto-refresh need to clear this in `beforeEach`", but the `Set` is not exported. Test files cannot clear it without either importing the module's internals via a hacky side-channel or relying on module re-registration between test suites. Consider exporting a test-only reset helper (e.g. `export function _resetAutoRefreshedInstallations()`) or moving the deduplication into a ref/context to keep it instance-scoped and inherently isolated between test renders.
How can I resolve this? If you propose a fix, please make it concise.Six early-returns in one useEffect is hard to read at a glance. Each guard stops a different misfire mode; spell them out.
installation.icon_key is a non-nullable string that defaults to "" for custom installs without a template. With ??, an empty string is kept and resolveServerIcon returns undefined, so the brand logo silently falls back to the generic plug. Use || so empty strings also fall through to template?.icon_key.
URL-equality matching breaks if the backend normalises the submitted URL (trailing slash, host case, etc.) — the user would stay stuck on the Add Custom form. Snapshot the existing installation IDs at submit and navigate to whichever new id appears in the list.
cvolzer3
changed the title
joshsny
approved these changes
Apr 27, 2026
Contributor
There was a problem hiding this comment.
is there a cleaner way to do this, the ref + effect logic seems quite complicated 🤔
Contributor
There was a problem hiding this comment.
this one is getting pretty unweildy with the effect calls, I think we could simplify it a bunch for some of those by moving the things in the effect calls to whatever makes modifications to the dependency array or moving state around a bit
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
2 participants
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.
Problem
Currently, the MCP servers in the marketplace use a fallback icon.
Changes
How did you test this?