feat(core): Add MCP server icon support - #85
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds icon support: new Icon/IconTheme types and config fields, iconFromFile utility (file→data URI), icon validation/normalize helpers in server, propagation of icon(s) through createApp/version merges, example usage, comprehensive unit tests, and a small float-generator tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CLI
participant FS as Filesystem
participant Util as iconFromFile
participant App as createApp/mergeVersionConfig
participant Server as MCP Server Init
participant Client as MCP Client
Dev ->> Util: call iconFromFile("./src/logo.png")
Util ->> FS: read file bytes
Util ->> Util: validate size & MIME -> base64-encode -> build Icon
Util -->> Dev: return Icon (data URI/src)
Dev ->> App: call createApp({ ..., icon or icons })
App ->> App: mergeVersionConfig(..., globalIcon, globalIcons) -> attach icons to AppConfig
Dev ->> Server: start server with AppConfig
Server ->> Server: normalizeIcons(config.icon, config.icons) -> validated icons[]
Server ->> Client: expose icons metadata in server info response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✏️ Tip: You can disable this entire section by setting Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b915d2e51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Review: MCP Server Icon SupportSummaryThis PR implements server icon configuration following the MCP specification (2025-11-25). The implementation adds icon support with both shorthand ( ✅ Strengths1. Excellent Type Safety & Documentation
2. Good API Design
3. Strong Test Coverage
4. Follows Project Conventions
🔍 Issues & Recommendations1. Missing Integration Tests for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/core/src/utils/icons.ts:
- Around line 83-88: The iconFromFile function currently accepts unvalidated
filePath and uses fs.readFileSync, enabling path traversal and blocking IO; fix
by normalizing and resolving the input (use path.resolve and path.normalize),
ensure the resolved path is within an allowed base directory (compare with
path.relative to reject traversal outside the intended root), validate the file
exists and its size against a configurable max (reject oversized files to avoid
memory issues), and replace fs.readFileSync with an async read using
fs.promises.readFile (and make iconFromFile async or provide an async variant)
so that IconFromFileOptions can include baseDir and maxSize settings and you
throw clear errors on invalid paths or sizes.
In @packages/core/tests/unit/icons.test.ts:
- Around line 145-261: The tests lack a check that normalized icons are actually
passed into the server constructor; add a unit that mocks the McpServer export
from "@modelcontextprotocol/sdk/server/mcp.js", calls normalizeIcons (the
function under test) and then verifies the McpServer constructor was called with
an options object containing the expected icons value (e.g., expect(new
McpServer).toHaveBeenCalledWith(expect.objectContaining({ icons: ... }))).
Ensure you import or reference normalizeIcons and the mocked McpServer symbol,
set up the mock before invoking normalizeIcons, and restore/clear mocks after
the test.
🧹 Nitpick comments (5)
examples/minimal/src/index.ts (1)
121-124: Example icon URL is fine, but consider showingicons(mimeType/sizes/theme) or avoiding a brittle remote dependency.
This example may fail in environments where GitHub raw URLs are blocked/rate-limited; also it doesn’t demonstratesizes/theme.packages/core/tests/unit/icons.test.ts (1)
1-143: Nice coverage forIcon/IconThemeandAppConfigacceptance tests.packages/core/src/server/index.ts (1)
41-63:normalizeIconsprecedence behavior is clear and matches the PR intent.packages/core/src/types/config.ts (1)
10-83: Icon types are straightforward and well-documented against the spec.packages/core/src/utils/icons.ts (1)
25-43: Useexport typeinstead ofexport interface.Per the coding guidelines: "Use
export typefor type-only exports to prevent runtime imports of types." Consider changing fromexport interfacetoexport typefor consistency.♻️ Suggested refactor
-export interface IconFromFileOptions { +export type IconFromFileOptions = { /** * Icon sizes in "WxH" format. * Use `["any"]` for scalable formats like SVG. * * @example ["48x48", "96x96"] */ sizes?: string[]; /** * Theme this icon is designed for. */ theme?: IconTheme; /** * Override the auto-detected MIME type. */ mimeType?: string; -} +};Based on coding guidelines.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/minimal/src/index.tspackages/core/src/index.tspackages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.tspackages/core/tests/unit/icons.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript - noanytype allowed; useunknownwith type narrowing instead
Middleware must alwaysawait next()or the middleware chain will break - follow Koa-style middleware patterns
Useexport typefor type-only exports to prevent runtime imports of types
Files:
packages/core/tests/unit/icons.test.tspackages/core/src/index.tsexamples/minimal/src/index.tspackages/core/src/utils/icons.tspackages/core/src/types/config.tspackages/core/src/server/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
defineToolanddefineUI(ordefineReactUIfor React components) for type inference instead of manual definitions
Files:
packages/core/tests/unit/icons.test.tspackages/core/src/index.tsexamples/minimal/src/index.tspackages/core/src/utils/icons.tspackages/core/src/types/config.tspackages/core/src/server/index.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export only through
index.tsfiles to keep the public API clean and make refactoring safe
Files:
packages/core/src/index.tsexamples/minimal/src/index.tspackages/core/src/server/index.ts
🧠 Learnings (2)
📚 Learning: 2026-01-11T02:21:51.116Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T02:21:51.116Z
Learning: Applies to **/index.ts : Export only through `index.ts` files to keep the public API clean and make refactoring safe
Applied to files:
packages/core/src/index.ts
📚 Learning: 2026-01-11T02:21:51.116Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T02:21:51.116Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `defineTool` and `defineUI` (or `defineReactUI` for React components) for type inference instead of manual definitions
Applied to files:
packages/core/src/server/index.ts
🧬 Code graph analysis (2)
packages/core/tests/unit/icons.test.ts (2)
packages/core/src/index.ts (4)
Icon(65-65)IconTheme(66-66)ToolDefs(39-39)AppConfig(58-58)packages/core/src/types/config.ts (3)
Icon(41-83)IconTheme(17-17)AppConfig(588-668)
packages/core/src/types/config.ts (1)
packages/core/src/index.ts (2)
IconTheme(66-66)Icon(65-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: test-and-lint
🔇 Additional comments (7)
packages/core/src/index.ts (1)
53-67: Public API exports look clean and TS-safe (export typeused appropriately).
Based on learnings, this keeps the public surface centralized inpackages/core/src/index.ts.Also applies to: 182-185
packages/core/src/types/config.ts (1)
542-562: Config surface additions (icon/icons) look good—please ensure any runtime config validation is updated too.
IfcreateApp/validation code strips unknown keys, these fields could be ignored unless explicitly allowed.Also applies to: 633-667
packages/core/src/server/index.ts (1)
110-118: TheMcpServerconstructor in@modelcontextprotocol/sdkdoes support aniconsoption. The option accepts an array of Icon objects with the shape{ src: string, mimeType?: string, sizes?: string[], theme?: string }. ThenormalizeIconsfunction correctly returnsIcon[] | undefined, which matches the SDK's expected format. When aniconstring is provided, it is properly converted to[{ src: icon }], and when aniconsarray is provided, it is passed through as-is. The code's use of...(icons && { icons })is the correct pattern for conditionally spreading optional properties.packages/core/src/utils/icons.ts (4)
1-7: LGTM - Clean imports.The imports follow best practices with the
node:protocol for built-in modules and proper type imports.
9-20: LGTM - Comprehensive MIME type mapping.The MIME type mappings cover common image formats and are correctly defined. The design allows for extension via the
mimeTypeoption override.
90-106: LGTM - Solid MIME type detection with helpful error messages.The MIME type detection logic correctly handles auto-detection from extensions and allows override. The error message is descriptive and guides users on how to handle unsupported formats.
108-122: LGTM - Clean Icon object construction.The Icon object is built correctly with conditional properties. The explicit approach makes the logic clear and avoids adding undefined fields.
| describe("iconFromFile", () => { | ||
| const mockReadFileSync = vi.mocked(readFileSync); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("should convert PNG file to data URI", () => { | ||
| const pngData = Buffer.from([0x89, 0x50, 0x4e, 0x47]); // PNG magic bytes | ||
| mockReadFileSync.mockReturnValue(pngData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.png"); | ||
|
|
||
| expect(icon.src).toMatch(/^data:image\/png;base64,/); | ||
| expect(icon.mimeType).toBe("image/png"); | ||
| expect(icon.sizes).toBeUndefined(); | ||
| expect(icon.theme).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("should convert JPEG file to data URI", () => { | ||
| const jpegData = Buffer.from([0xff, 0xd8, 0xff, 0xe0]); // JPEG magic bytes | ||
| mockReadFileSync.mockReturnValue(jpegData); | ||
|
|
||
| const icon = iconFromFile("/path/to/photo.jpg"); | ||
|
|
||
| expect(icon.src).toMatch(/^data:image\/jpeg;base64,/); | ||
| expect(icon.mimeType).toBe("image/jpeg"); | ||
| }); | ||
|
|
||
| it("should convert SVG file to data URI", () => { | ||
| const svgContent = Buffer.from('<svg xmlns="http://www.w3.org/2000/svg"></svg>'); | ||
| mockReadFileSync.mockReturnValue(svgContent); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.svg"); | ||
|
|
||
| expect(icon.src).toMatch(/^data:image\/svg\+xml;base64,/); | ||
| expect(icon.mimeType).toBe("image/svg+xml"); | ||
| }); | ||
|
|
||
| it("should handle .jpeg extension", () => { | ||
| const jpegData = Buffer.from([0xff, 0xd8, 0xff, 0xe0]); | ||
| mockReadFileSync.mockReturnValue(jpegData); | ||
|
|
||
| const icon = iconFromFile("/path/to/photo.jpeg"); | ||
|
|
||
| expect(icon.mimeType).toBe("image/jpeg"); | ||
| }); | ||
|
|
||
| it("should handle WebP files", () => { | ||
| const webpData = Buffer.from("RIFF....WEBP"); | ||
| mockReadFileSync.mockReturnValue(webpData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.webp"); | ||
|
|
||
| expect(icon.mimeType).toBe("image/webp"); | ||
| }); | ||
|
|
||
| it("should include sizes option when provided", () => { | ||
| const pngData = Buffer.from([0x89, 0x50, 0x4e, 0x47]); | ||
| mockReadFileSync.mockReturnValue(pngData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.png", { sizes: ["48x48", "96x96"] }); | ||
|
|
||
| expect(icon.sizes).toEqual(["48x48", "96x96"]); | ||
| }); | ||
|
|
||
| it("should include theme option when provided", () => { | ||
| const pngData = Buffer.from([0x89, 0x50, 0x4e, 0x47]); | ||
| mockReadFileSync.mockReturnValue(pngData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon-dark.png", { theme: "dark" }); | ||
|
|
||
| expect(icon.theme).toBe("dark"); | ||
| }); | ||
|
|
||
| it("should allow mimeType override", () => { | ||
| const customData = Buffer.from("custom format"); | ||
| mockReadFileSync.mockReturnValue(customData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.custom", { mimeType: "image/x-custom" }); | ||
|
|
||
| expect(icon.src).toMatch(/^data:image\/x-custom;base64,/); | ||
| expect(icon.mimeType).toBe("image/x-custom"); | ||
| }); | ||
|
|
||
| it("should throw for unsupported extension without mimeType override", () => { | ||
| const unknownData = Buffer.from("unknown format"); | ||
| mockReadFileSync.mockReturnValue(unknownData); | ||
|
|
||
| expect(() => iconFromFile("/path/to/icon.xyz")).toThrow("Unsupported image format: .xyz"); | ||
| }); | ||
|
|
||
| it("should correctly encode file content as base64", () => { | ||
| const testContent = Buffer.from("Hello, World!"); | ||
| mockReadFileSync.mockReturnValue(testContent); | ||
|
|
||
| const icon = iconFromFile("/path/to/test.png"); | ||
|
|
||
| // "Hello, World!" in base64 is "SGVsbG8sIFdvcmxkIQ==" | ||
| expect(icon.src).toBe("data:image/png;base64,SGVsbG8sIFdvcmxkIQ=="); | ||
| }); | ||
|
|
||
| it("should handle all options together", () => { | ||
| const pngData = Buffer.from([0x89, 0x50, 0x4e, 0x47]); | ||
| mockReadFileSync.mockReturnValue(pngData); | ||
|
|
||
| const icon = iconFromFile("/path/to/icon.png", { | ||
| sizes: ["48x48"], | ||
| theme: "light", | ||
| }); | ||
|
|
||
| expect(icon.src).toMatch(/^data:image\/png;base64,/); | ||
| expect(icon.mimeType).toBe("image/png"); | ||
| expect(icon.sizes).toEqual(["48x48"]); | ||
| expect(icon.theme).toBe("light"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find packages/core -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 824
🏁 Script executed:
wc -l packages/core/tests/unit/icons.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 110
🏁 Script executed:
cat -n packages/core/tests/unit/icons.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 10117
🏁 Script executed:
find packages/core -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "createServerInstance" | head -10Repository: AndurilCode/mcp-apps-kit
Length of output: 132
🏁 Script executed:
find packages/core -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "McpServer" | head -10Repository: AndurilCode/mcp-apps-kit
Length of output: 265
🏁 Script executed:
cat -n packages/core/src/createApp.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 50382
🏁 Script executed:
cat -n packages/core/src/server/index.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 46510
🏁 Script executed:
find packages/core/tests -name "*.test.ts" -o -name "*.test.tsx" | xargs grep -l "createServerInstance\|McpServer" | head -10Repository: AndurilCode/mcp-apps-kit
Length of output: 106
🏁 Script executed:
grep -r "createServerInstance" packages/core/tests --include="*.ts" --include="*.tsx"Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
grep -r "new McpServer" packages/core/tests --include="*.ts" --include="*.tsx"Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
cat -n packages/core/tests/unit/stdio.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 3559
🏁 Script executed:
find packages/core/tests -type f \( -name "*.test.ts" -o -name "*.test.tsx" \) | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 1073
🏁 Script executed:
cat packages/core/tests/integration/server.test.ts | head -200Repository: AndurilCode/mcp-apps-kit
Length of output: 5924
🏁 Script executed:
grep -A 10 -B 5 "icons" packages/core/tests/integration/server.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
grep -n "icon" packages/core/tests/integration/server.test.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
grep -rn "McpServer\|icons" packages/core/tests/contract packages/core/tests/integration --include="*.ts" | grep -i iconRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
cat -n packages/core/src/server/index.ts | sed -n '110,120p'Repository: AndurilCode/mcp-apps-kit
Length of output: 494
Add a test that asserts server wiring (normalizeIcons → new McpServer({ icons })) to prevent SDK/API drift.
Right now the suite doesn't verify the integration between icon config and server initialization. Mocking @modelcontextprotocol/sdk/server/mcp.js and asserting the McpServer constructor receives the correct icons option would catch breakage early when the SDK API changes.
🤖 Prompt for AI Agents
In @packages/core/tests/unit/icons.test.ts around lines 145 - 261, The tests
lack a check that normalized icons are actually passed into the server
constructor; add a unit that mocks the McpServer export from
"@modelcontextprotocol/sdk/server/mcp.js", calls normalizeIcons (the function
under test) and then verifies the McpServer constructor was called with an
options object containing the expected icons value (e.g., expect(new
McpServer).toHaveBeenCalledWith(expect.objectContaining({ icons: ... }))).
Ensure you import or reference normalizeIcons and the mocked McpServer symbol,
set up the mock before invoking normalizeIcons, and restore/clear mocks after
the test.
Code Review: MCP Server Icon SupportSummaryThis PR adds icon configuration support following the MCP specification (2025-11-25). The implementation is well-structured with comprehensive tests and good documentation. Overall quality is high, but I've identified several issues that should be addressed. Critical Issues1. Missing Tests for
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/tests/unit/icons.test.ts (1)
145-287: Excellent test coverage foriconFromFile!The test suite comprehensively covers:
- Multiple image formats (PNG, JPEG, SVG, WebP)
- All configuration options (sizes, theme, mimeType)
- Error scenarios (unsupported formats, file size limits, read failures)
- Boundary conditions (exactly 1MB file)
- Base64 encoding correctness with known input
The mock setup and assertions are precise and well-structured.
💡 Optional: Consider testing path resolution behavior
You could add a test to verify that relative paths are resolved correctly to absolute paths:
it("should resolve relative paths to absolute", () => { const pngData = Buffer.from([0x89, 0x50, 0x4e, 0x47]); mockReadFileSync.mockReturnValue(pngData); iconFromFile("./relative/icon.png"); // Verify readFileSync was called with an absolute path expect(mockReadFileSync).toHaveBeenCalledWith( expect.stringMatching(/^\/.*\/relative\/icon\.png$/) ); });This would verify the path resolution logic mentioned in the implementation, but it's not essential given the current comprehensive coverage.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/server/index.tspackages/core/src/utils/icons.tspackages/core/tests/unit/icons.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/server/index.ts
- packages/core/src/utils/icons.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript - noanytype allowed; useunknownwith type narrowing instead
Middleware must alwaysawait next()or the middleware chain will break - follow Koa-style middleware patterns
Useexport typefor type-only exports to prevent runtime imports of types
Files:
packages/core/tests/unit/icons.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
defineToolanddefineUI(ordefineReactUIfor React components) for type inference instead of manual definitions
Files:
packages/core/tests/unit/icons.test.ts
🧬 Code graph analysis (1)
packages/core/tests/unit/icons.test.ts (3)
packages/core/src/index.ts (5)
Icon(65-65)IconTheme(66-66)ToolDefs(39-39)AppConfig(58-58)iconFromFile(183-183)packages/core/src/types/config.ts (3)
Icon(41-83)IconTheme(17-17)AppConfig(588-668)packages/core/src/utils/icons.ts (1)
iconFromFile(93-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-lint
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/core/tests/unit/icons.test.ts (3)
12-18: LGTM! Proper mocking setup.The mock is correctly configured before importing
iconFromFile, which ensures the module uses the mocked file system functions during tests.
20-65: LGTM! Comprehensive Icon interface tests.The tests cover minimal configurations, fully specified icons, data URIs, and theme values. The type-level assertion on line 61 is a nice touch for verifying the IconTheme union type.
67-142: LGTM! Thorough AppConfig icon tests.The test suite validates all icon configuration patterns: shorthand strings, arrays, theme-specific icons, and data URIs. The test at line 103 correctly verifies that both
iconandiconsfields can coexist in the configuration (the precedence logic is tested in the server implementation).
Code Review: MCP Server Icon SupportStrengthsExcellent implementation overall! The code is well-structured, thoroughly tested, and follows MCP specification closely.
Critical IssuesIssue 1: Empty String Icon Handling In normalizeIcons, an empty string for icon parameter returns undefined instead of throwing an error. The check if (icon) evaluates to false for empty strings, so they are silently ignored. This is tested and intentional (line 309), but inconsistent with the error message. Recommendation: Either document this behavior (empty string = no icon) or change line 79 to if (icon !== undefined) to catch empty strings. Issue 2: Sparse Array Handling If someone passes a sparse array, undefined entries are skipped without validation. This could lead to runtime errors in the MCP SDK. Fix: Add validation for null/undefined entries before calling validateIcon. Issue 3: MIME Type Always Included If mimeType is an empty string, it is still added to the Icon object. Per the Icon interface, mimeType is optional, so it should be omitted if not detected. Though realistically this cannot happen due to validation at line 119-125. Best Practices & SuggestionsSecurity: Path Traversal Performance: Synchronous File Reading Test Coverage: Missing Integration Test Export Consistency Test CoverageThe test file is excellent with 396 lines covering type validation, all image formats, size limits, error cases, icon normalization logic, and edge cases. This easily exceeds the 50% coverage requirement! Code Quality
Before MergingPlease verify:
Overall, this is high-quality work with just a few minor issues to address. The feature is well-designed and will be a great addition to the framework! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/server/index.ts (1)
67-76: Rename the loop variable to avoid shadowing the function parameter.The variable
iconon line 70 shadows the function parametericon(string | undefined). This reduces code clarity and could lead to confusion.♻️ Proposed fix
// icons array takes precedence if (icons && icons.length > 0) { // Validate each icon for (let i = 0; i < icons.length; i++) { - const icon = icons[i]; - if (icon) { - validateIcon(icon, i); + const iconItem = icons[i]; + if (iconItem) { + validateIcon(iconItem, i); } } return icons; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/index.tspackages/core/src/server/index.tspackages/core/tests/unit/icons.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/tests/unit/icons.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript - noanytype allowed; useunknownwith type narrowing instead
Middleware must alwaysawait next()or the middleware chain will break - follow Koa-style middleware patterns
Useexport typefor type-only exports to prevent runtime imports of types
Files:
packages/core/src/server/index.tspackages/core/src/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
defineToolanddefineUI(ordefineReactUIfor React components) for type inference instead of manual definitions
Files:
packages/core/src/server/index.tspackages/core/src/index.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export only through
index.tsfiles to keep the public API clean and make refactoring safe
Files:
packages/core/src/server/index.tspackages/core/src/index.ts
🧠 Learnings (2)
📚 Learning: 2026-01-11T02:21:51.116Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T02:21:51.116Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `defineTool` and `defineUI` (or `defineReactUI` for React components) for type inference instead of manual definitions
Applied to files:
packages/core/src/server/index.ts
📚 Learning: 2026-01-11T02:21:51.116Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T02:21:51.116Z
Learning: Applies to **/index.ts : Export only through `index.ts` files to keep the public API clean and make refactoring safe
Applied to files:
packages/core/src/index.ts
🧬 Code graph analysis (1)
packages/core/src/server/index.ts (1)
packages/core/src/types/config.ts (1)
Icon(41-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-lint
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/core/src/index.ts (1)
65-66: LGTM! Icon exports follow TypeScript best practices.The new icon-related exports are properly structured:
- Type exports use
export type(Icon, IconTheme, IconFromFileOptions)- Runtime function exports correctly omit
export type(iconFromFile, normalizeIcons)- Exports are organized logically with clear section comments
Also applies to: 182-185
packages/core/src/server/index.ts (2)
45-52: LGTM! Validation logic is correct and focused.The validation correctly enforces the only required field (
src) and provides clear error messages for debugging.
134-142: Icon integration with McpServer is properly implemented.The icon normalization and conditional spreading pattern are correct. MCP SDK version 1.25.1 supports the
iconsfield onMcpServer, and the implementation properly handles both shorthandiconstrings and fulliconsarrays with appropriate validation.
Code Review - PR #85: MCP Server Icon SupportGreat work implementing MCP server icon configuration! The implementation is well-structured, thoroughly tested, and follows the MCP specification. Here's my detailed review: ✅ Strengths
🔍 Issues & Recommendations1. Potential Type Safety Issue in
|
Code Review: MCP Server Icon SupportOverviewThis PR successfully implements server icon configuration following the MCP specification (2025-11-25). The implementation is well-structured with comprehensive type definitions, validation, and test coverage. ✅ Strengths1. Excellent Type Safety & Documentation
2. Robust Validation
3. Developer Experience
4. Comprehensive Test Coverage
🔍 Potential Issues & Suggestions1. Security: Insufficient Data URI ValidationSeverity: Medium The Recommendation: Add basic data URI validation to ensure MIME type starts with 2. Edge Case: Empty Icons Array BehaviorSeverity: Low When Test case missing: normalizeIcons("https://example.com/fallback.png", []) // Returns undefined3. Code Quality: Redundant MIME Type AssignmentSeverity: Very Low In 4. Maintainability: Exported Internal FunctionSeverity: Low
📋 Testing ObservationsExcellent coverage overall. Additional test cases to consider:
🎯 Performance & Best PracticesFile Size Limit ✅ 1MB limit well-justified with clear messaging Synchronous File Reading Project Conventions ✅ Follows all CLAUDE.md requirements 🔒 Security AssessmentNo critical security issues. The data URI validation gap should be addressed to prevent potential content injection if icons are rendered in untrusted contexts. ✅ Final RecommendationAPPROVE with minor changes recommended This is a solid implementation following MCP specifications and project conventions. Well-tested, well-documented, and provides excellent developer experience. Before merging:
Nice work! The dual API and 📦 Checklist
|
Code Review: MCP Server Icon SupportSummaryThis PR implements MCP server icon configuration following the MCP specification (2025-11-25). The implementation is well-structured with comprehensive tests and good documentation. Overall, this is high-quality work with only minor concerns. Strengths1. Excellent Test Coverage
2. Strong Type Safety
3. Great Documentation
4. Good API Design
Issues and Suggestions1. Potential Security Concern: Data URI Size Limit (Medium) The 1MB file size limit is enforced for local files, but there is no validation for data URIs passed directly via the icon or icons config. An attacker or careless developer could pass a multi-megabyte base64 data URI that would bloat the server configuration. Recommendation: Add data URI size validation in normalizeIcons() for data URIs to prevent configuration bloat. 2. Missing Validation Test (Low) 3. Export Inconsistency (Low) The normalizeIcons() function is exported but marked as @internal. Internal functions should generally not be exported unless there is a specific reason. 4. Error Message Consistency (Minor) 5. Missing File Extension Edge Case (Low) 6. Performance: Synchronous File I/O (Low) Code Quality NotesFollowing CLAUDE.md Guidelines:
Best Practices:
Before Merge ChecklistAccording to CLAUDE.md, all four must pass: pnpm build && pnpm test && pnpm lint && pnpm typecheckPlease confirm these all pass before merging. VerdictAPPROVE with suggestions - This is solid work that adds valuable functionality. The security concern about data URI size validation should be addressed, but it is not a blocker. The other items are nice-to-haves that would improve robustness. Great job on the comprehensive testing and documentation! Review by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/tests/unit/versioning.test.ts (1)
290-327: Consider strengthening icon propagation verification.The tests successfully verify that app creation doesn't fail with global icons, but they don't actually verify that icons are propagated to the version configurations or server instances. The tests would pass even if icon propagation was completely broken.
While this pattern is consistent with similar tests in this file (e.g., the plugin merging test at lines 263-288), you could strengthen these tests by:
- Accessing version apps via
app.getVersion()and verifying icon configuration- Checking if icons are passed to server initialization
- Using the server's info/metadata endpoint (if available) to verify icons are exposed
Example: Potential verification approach
it("should propagate global icon to all versions", () => { const app = createApp({ name: "test-app", icon: "https://example.com/icon.png", versions: { v1: { version: "1.0.0", tools: {} }, v2: { version: "2.0.0", tools: {} }, }, }); expect(app).toBeDefined(); // If there's a way to access the config or verify server initialization: // const v1App = app.getVersion("v1"); // const v1Server = v1App?.getServer(); // expect(v1Server.serverInfo.icon).toBe("https://example.com/icon.png"); });Note: This example assumes there's a way to access the icon through the API, which may not be currently exposed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript - noanytype allowed; useunknownwith type narrowing instead
Middleware must alwaysawait next()or the middleware chain will break - follow Koa-style middleware patterns
Useexport typefor type-only exports to prevent runtime imports of types
Files:
packages/core/tests/unit/versioning.test.tspackages/core/src/createApp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
defineToolanddefineUI(ordefineReactUIfor React components) for type inference instead of manual definitions
Files:
packages/core/tests/unit/versioning.test.tspackages/core/src/createApp.ts
🧬 Code graph analysis (2)
packages/core/tests/unit/versioning.test.ts (2)
packages/core/src/createApp.ts (1)
createApp(540-550)packages/core/src/index.ts (1)
createApp(190-190)
packages/core/src/createApp.ts (1)
packages/core/src/types/config.ts (1)
Icon(41-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test-and-lint
- GitHub Check: claude-review
🔇 Additional comments (2)
packages/core/src/createApp.ts (2)
430-491: LGTM! Icon propagation implementation is correct.The implementation correctly propagates global
iconandiconsto all versions. The design choice to only support global icons (no per-version overrides) aligns with the PR objectives which state "Icons provided in the app configuration are passed to McpServer during initialization."Note: If per-version icon overrides are needed in the future, the signature would need to accept
versionConfig.iconandversionConfig.iconswith precedence logic similar to how plugins are merged.
817-824: LGTM! Multi-version icon propagation is correctly implemented.The call to
mergeVersionConfigproperly passes the globaliconandiconsfields, ensuring they're propagated to each version's AppConfig.
3bf42b8 to
53b5754
Compare
Code Review for PR #85: Add MCP Server Icon SupportSummaryThis PR implements MCP server icon configuration following the MCP specification (2025-11-25). The implementation is well-structured with comprehensive testing and good error handling. ✅ Strengths
🔍 Code Quality Issues1. Potential Security Concern
|
53b5754 to
ab2a237
Compare
Code Review: MCP Server Icon SupportSummaryThis PR implements MCP server icon configuration following the MCP specification (2025-11-25). The implementation is well-designed with excellent test coverage and proper type safety. ✅ Strengths1. Excellent Type Safety
2. Comprehensive Test Coverage
3. Well-Documented API
4. Follows Codebase Conventions
🔍 Issues & Suggestions1. Missing Integration Test (Minor)The icon configuration is passed to
Location: Suggested addition: describe("MCP integration", () => {
it("should expose icons via MCP server info", async () => {
const app = createApp({
name: "test-app",
version: "1.0.0",
icon: "https://example.com/icon.png",
tools: {},
});
await app.start({ port: 3200 });
const transport = new StreamableHTTPClientTransport(new URL("http://localhost:3200/mcp"));
const client = new Client({ name: "test", version: "1.0.0" }, { capabilities: {} });
await client.connect(transport);
// Verify server implementation includes icons
// (requires accessing server info from MCP SDK)
await client.close();
await app.stop();
});
});2. Potential Runtime Error with Invalid MIME Type (Minor)In const mimeType = options.mimeType ?? detectedMimeType ?? "";However, this is actually safe because of the validation on line 119-125 that would throw before this point if both are undefined. Consider adding a comment or assertion to make this more explicit. 3. Version-Specific Icon Override Not Supported (Design Question)Currently, icons are only configurable at the global level and propagated to all versions. There's no way to specify different icons per version. Is this intentional? Location: If version-specific icons are desired in the future, you'd need to update 4. URL Validation Not Performed (Minor Security)The
Example: function validateIconSrc(src: string): void {
if (src.startsWith('data:')) {
// Validate data URI format
if (!/^data:image\/[\w+.-]+;base64,/.test(src)) {
throw new Error('Invalid data URI format');
}
} else if (src.startsWith('http://') || src.startsWith('https://')) {
try {
new URL(src);
} catch {
throw new Error('Invalid URL format');
}
} else {
throw new Error('Icon src must be a valid HTTP(S) URL or data URI');
}
}5. Example Uses External Icon URL (Minor)In icon: "https://raw.githubusercontent.com/anthropics/claude-code/main/assets/icon.svg",This references an external repository. Consider:
🎯 Performance Considerations
🔒 Security Considerations
📝 Code Quality
✅ Verification ChecklistPer
🎉 ConclusionThis is a high-quality implementation that follows the codebase conventions and provides a great developer experience. The test coverage is excellent, and the API design is clean and intuitive. Recommendation: Approve with minor suggestions The suggestions above are all minor improvements that could be addressed in follow-up PRs. The core implementation is solid and ready to merge. Priority of Suggestions:
Great work! 🎨 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/utils/icons.ts (1)
127-135: Consider the unreachable empty string case on Line 128.The fallback to empty string (
?? "") on Line 128 is defensive but unreachable due to the validation on Lines 119-125 which throws if bothdetectedMimeTypeandoptions.mimeTypeare falsy. The subsequentstartsWith("image/")check on Line 131 would also catch an empty string, so this is safe but slightly redundant.♻️ Optional: Simplify with non-null assertion
- // mimeType is guaranteed to be defined after validation above - const mimeType = options.mimeType ?? detectedMimeType ?? ""; + // mimeType is guaranteed to be defined after validation above + const mimeType = (options.mimeType ?? detectedMimeType)!;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/minimal/src/index.tspackages/core/src/createApp.tspackages/core/src/index.tspackages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.tspackages/core/tests/unit/icons.test.tspackages/core/tests/unit/versioning.test.tspackages/testing/src/eval/property/generators.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/minimal/src/index.ts
- packages/core/tests/unit/versioning.test.ts
- packages/core/src/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Express 5 (not 4) - async error handling works differently
Files:
packages/core/tests/unit/icons.test.tspackages/core/src/createApp.tspackages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.tspackages/testing/src/eval/property/generators.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use strict TypeScript - noanytypes, useunknownwith type narrowing instead
UsedefineToolanddefineUI(ordefineReactUIfor React components) for type inference
Useexport typefor type-only exports to prevent runtime imports of types
Files:
packages/core/tests/unit/icons.test.tspackages/core/src/createApp.tspackages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.tspackages/testing/src/eval/property/generators.ts
packages/core/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
packages/core/src/**/*.{ts,tsx}: UsedefineToolanddefineUIfor type inference instead of inline objects
Ensure Zod 4 APIs are used, not Zod 3 APIs which have breaking changes
Always validate input with Zod schema before processing, never trust raw input
Files:
packages/core/src/createApp.tspackages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.ts
**/index.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Export only through
index.tsfiles to keep public API clean and enable safe refactoring
Files:
packages/core/src/server/index.ts
packages/core/src/**/index.ts
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
All public API exports must go through
src/index.ts, not individual subdirectory barrel files
Files:
packages/core/src/server/index.ts
packages/core/src/{adapters,debug,events,middleware,plugins,server,server/oauth}/index.ts
📄 CodeRabbit inference engine (packages/core/AGENTS.md)
Subdirectory barrel files in
adapters/,debug/,events/,middleware/,plugins/,server/, andserver/oauth/should only contain local re-exports
Files:
packages/core/src/server/index.ts
🧠 Learnings (7)
📚 Learning: 2026-01-11T17:08:24.463Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/testing/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:24.463Z
Learning: Applies to packages/testing/**/*.test.{ts,tsx,js,jsx} : Use the fluent assertion API with `expectToolResult()` for validating tool execution results
Applied to files:
packages/core/tests/unit/icons.test.ts
📚 Learning: 2026-01-11T17:08:24.463Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/testing/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:24.463Z
Learning: Applies to packages/testing/**/*.test.{ts,tsx,js,jsx} : Always call `await env.cleanup()` after test completion to ensure the server is properly shut down
Applied to files:
packages/core/tests/unit/icons.test.ts
📚 Learning: 2026-01-11T17:08:42.176Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/ui/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:42.176Z
Learning: Applies to packages/ui/**/*.{ts,tsx} : Provide type parameter to `createClient<typeof app.tools()>()` for proper typed tool calls
Applied to files:
packages/core/src/createApp.tspackages/core/src/server/index.ts
📚 Learning: 2026-01-11T17:08:42.176Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/ui/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:42.176Z
Learning: Applies to packages/ui/**/*.{ts,tsx,js,jsx} : Handle both MCP and OpenAI response formats when calling tools
Applied to files:
packages/core/src/server/index.ts
📚 Learning: 2026-01-11T17:08:13.651Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/core/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:13.651Z
Learning: Applies to packages/core/src/**/*.{ts,tsx} : Use `defineTool` and `defineUI` for type inference instead of inline objects
Applied to files:
packages/core/src/server/index.tspackages/core/src/types/config.tspackages/core/src/utils/icons.ts
📚 Learning: 2026-01-11T17:08:03.538Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:08:03.538Z
Learning: Applies to **/*.{ts,tsx} : Use `defineTool` and `defineUI` (or `defineReactUI` for React components) for type inference
Applied to files:
packages/core/src/server/index.ts
📚 Learning: 2026-01-11T17:08:19.638Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: packages/create-app/AGENTS.md:0-0
Timestamp: 2026-01-11T17:08:19.638Z
Learning: Applies to packages/create-app/**/*.{ts,tsx,js} : Use path.join() instead of string concatenation for cross-platform file path operations in the mcp-apps-kit/create-app scaffolding tool
Applied to files:
packages/core/src/server/index.tspackages/core/src/utils/icons.ts
🔇 Additional comments (18)
packages/testing/src/eval/property/generators.ts (1)
132-144: LGTM! Consistent NaN exclusion across all float generator branches.The addition of
noNaN: trueto all branches is a sensible default for property testing, as NaN values often cause unexpected test failures. This is a behavioral change worth noting in release notes if consumers were relying on NaN generation.packages/core/src/types/config.ts (3)
10-83: Well-documented Icon types following the MCP specification.The
Iconinterface is well-structured with comprehensive JSDoc examples covering URL references, data URIs, and theme-specific icons. The optional fields (mimeType,sizes,theme) provide flexibility while keepingsrcas the only required field.
541-562: Global icon configuration for multi-version apps looks good.The
iconshorthand andiconsarray fields onVersionsConfigenable global icon configuration that applies to all versions. The documentation clearly explains the relationship between the two fields.
632-668: Consistent icon configuration for single-version apps.The icon fields mirror the
VersionsConfigstructure, maintaining API consistency. The examples are helpful and cover both URL and data URI use cases.packages/core/src/utils/icons.ts (3)
1-26: Good foundation for icon utilities.The 1MB size limit is reasonable, and the MIME type mappings cover common image formats. The comment about base64 encoding increasing size by ~33% is helpful context.
93-104: Robust file reading with helpful error messages.The error handling wraps the underlying error with context about which file failed, making debugging easier. Using
path.isAbsolutecheck before resolving is correct.
141-156: Clean Icon object construction with optional field handling.The conditional addition of
sizesandthemeonly when provided keeps the output clean and avoids unnecessary undefined properties.packages/core/tests/unit/icons.test.ts (4)
12-19: Good mock setup for file system operations.Mocking
node:fsbefore importingiconFromFileensures the mock is in place when the module loads. The import order (mock → iconFromFile → readFileSync) is correct.
68-76: Consider usingdefineToolfor test tool definitions.Per coding guidelines,
defineToolshould be used for type inference instead of inline objects. However, since this is a test file focused on icon configuration rather than tool behavior, the inline definition is acceptable for test simplicity.
257-271: Thorough boundary testing for file size limits.Testing both the "exceeds limit" (1MB + 1 byte) and "exactly at limit" (1MB) cases ensures the boundary condition is correctly handled with
>rather than>=.
416-445: Comprehensive sizes format validation tests.Good coverage of valid formats (
WxH,any) and invalid formats (48,48x,large). This ensures the regex pattern innormalizeIconsis correctly enforced.packages/core/src/server/index.ts (3)
41-64: Well-structured icon validation helpers.The
SIZES_PATTERNregex correctly validatesWxHformat, andvalidateIconSizesproperly handles the special"any"value for scalable formats. Error messages include the index for easier debugging.
76-107: Clarify the export status ofnormalizeIcons.The function is marked
@internalin the JSDoc but is exported publicly. If it's truly internal, remove theexportkeyword or remove the@internaltag if it's intentionally part of the public API. Additionally, verify that this export aligns with the guideline that subdirectory barrel files likeserver/index.tsshould only contain local re-exports and that all public API exports must go throughpackages/core/src/index.ts.
154-162: Verify icon parameter placement in McpServer constructor.The code passes
iconsas a top-level parameter to McpServer, but the SDK documentation indicates icons should be nested within animplementationobject structure. Verify that thenormalizeIconsfunction and McpServer instantiation are correctly handling the icon structure per the @modelcontextprotocol/sdk specification.packages/core/src/createApp.ts (4)
15-15: LGTM!The
Icontype is correctly imported usingimport type, ensuring no runtime import overhead. As per coding guidelines, this is the proper approach for type-only imports.
430-436: LGTM!Clean signature extension with optional parameters maintains backward compatibility. The function signature clearly distinguishes between the shorthand
icon(string URL or data URI) and the fulliconsarray, aligning with the MCP specification's dual approach.
487-490: LGTM!Global icons are correctly propagated to each version's merged config. The implementation aligns with the PR objective of passing app-level icons to McpServer during initialization. The comment clearly documents the intent.
817-824: LGTM!The call site correctly passes both
config.iconandconfig.iconsfrom the globalVersionsConfigto the merge function. This ensures icon configuration defined at the app level is propagated to all version instances.
ab2a237 to
c1f7992
Compare
Code Review: MCP Server Icon SupportSummaryThis PR implements server icon configuration following the MCP specification. The implementation is well-structured with good test coverage and proper error handling. Overall, this is a high-quality PR with thoughtful design decisions. Strengths
Issues and Suggestions1. Missing MIME Type Validation (Medium Priority)Location: packages/core/src/server/index.ts:69-74 The validateIcon function validates src and sizes, but NOT mimeType. This creates inconsistency because iconFromFile validates MIME types must start with image/ but manually created icons bypass this check. Recommendation: Add MIME type validation to validateIcon to ensure all icons have valid image MIME types. 2. normalizeIcons Export (Low Priority)The function is marked @internal but exported from server/index.ts. Consider making it truly internal or documenting why it needs to be exported. 3. Icon/Icons Precedence (Low Priority)When both icon and icons are provided, icons takes precedence and icon is silently ignored. Consider adding a debug warning for this case. 4. Security: URL Scheme ValidationUsers can provide arbitrary URLs with any scheme (file://, javascript:, etc.). Consider validating that only http, https, and data schemes are allowed. 5. Performance: Synchronous File ReadingiconFromFile uses fs.readFileSync which blocks the event loop. Add JSDoc note that this is intended for initialization only, not runtime. Security ConsiderationsGood practices:
Concerns:
Test CoverageComprehensive coverage including:
Missing: MIME type validation tests for manual icons, URL scheme tests VerdictAPPROVE with suggestions Well-implemented feature with excellent tests and documentation. Issues are minor and mostly involve additional validation for robustness. Before Merge:
Great work! |
Implement server icon configuration following the MCP specification
(2025-11-25). Developers can now provide icons for their MCP servers
that will be displayed in MCP client UIs.
Features:
- Add Icon and IconTheme types
- Support both shorthand `icon` (string URL/data URI) and full `icons` array
- Icons are passed to McpServer during initialization
- Export Icon and IconTheme types from package
Usage:
```typescript
createApp({
name: "my-app",
version: "1.0.0",
icon: "https://example.com/icon.png", // or data:image/...
// OR for multiple icons:
icons: [
{ src: "https://example.com/icon.png", mimeType: "image/png", sizes: ["48x48"] },
{ src: "https://example.com/dark.png", theme: "dark" }
],
tools: { ... }
});
```
Add a convenience function to convert local image files to base64 data
URIs for use with server icon configuration.
Features:
- Reads local files and converts to base64 data URI
- Auto-detects MIME type from file extension
- Supports PNG, JPEG, SVG, WebP, GIF, ICO formats
- Allows custom mimeType override for unsupported formats
- Accepts optional sizes and theme parameters
Usage:
```typescript
import { createApp, iconFromFile } from "@mcp-apps-kit/core";
const app = createApp({
name: "my-app",
version: "1.0.0",
icons: [
iconFromFile("./assets/icon.png"),
iconFromFile("./assets/icon-48.png", { sizes: ["48x48"] }),
iconFromFile("./assets/icon-dark.png", { theme: "dark" }),
],
tools: { ... }
});
```
- Add 1MB file size limit to iconFromFile to prevent excessive memory usage - Add error handling wrapper for file read operations with descriptive messages - Add icon validation in normalizeIcons to ensure src is non-empty string - Add comprehensive tests for new validation and error scenarios
- Export normalizeIcons for testability - Add unit tests for icon shorthand, icons array, and precedence - Add validation tests for empty/whitespace icon values - Test theme-specific icons and edge cases
The fast-check float generator can produce NaN values by default, which caused the "should generate floats within bounds" test to fail intermittently. Adding noNaN: true ensures only valid numeric floats are generated.
- Use forEach with block body instead of for loop (type safety) - Add sizes format validation (WxH pattern or "any") - Add MIME type validation (must be image/*) - Make empty string icon throw error (consistent with whitespace) - Remove normalizeIcons from public exports (internal only) - Add comprehensive tests for new validation logic
Global icon/icons from VersionsConfig were not being applied to individual versions. Updated mergeVersionConfig to accept and forward these fields, ensuring icons configured at the global level are properly propagated to all versions.
Replaced the server icon URL with a local image file using the new iconFromFile helper. This change enhances the flexibility of icon configuration by allowing local image embedding for server icons.
84d1479 to
d2df9d3
Compare
PR Review: Add MCP Server Icon SupportSummaryThis PR implements server icon configuration following the MCP specification (2025-11-25). The implementation is well-structured and production-ready with excellent test coverage and documentation. ✅ Strengths1. Excellent Code Quality
2. Robust Validation
3. Strong Test Coverage
4. Developer Experience
🔍 Issues Found1. Security: Path Traversal Vulnerability (Medium Severity)Location: const absolutePath = path.isAbsolute(filePath) ? filePath : path.resolve(process.cwd(), filePath);Issue: No validation that the resolved path stays within expected boundaries. An attacker could use Recommendation: While this is likely build-time only, add defensive checks: const absolutePath = path.isAbsolute(filePath)
? filePath
: path.resolve(process.cwd(), filePath);
// Ensure the resolved path doesn't escape expected directories
const normalized = path.normalize(absolutePath);
if (normalized.includes('..')) {
throw new Error(`Invalid file path: path traversal not allowed`);
}Mitigation: Document that 2. Bug: Incorrect Empty Array HandlingLocation: if (icons && icons.length > 0) {
icons.forEach((ic, i) => {
validateIcon(ic, i);
});
return icons;
}Issue: Empty arrays Recommendation: Be explicit about the contract: // Option 1: Treat empty array as undefined (current behavior - document it)
// Option 2: Preserve empty array (be explicit that it means "no icons")
if (icons !== undefined) {
if (icons.length === 0) {
return undefined; // Document: empty array = no icons
}
// ... validation
}3. Type Safety:
|
Code Review: MCP Server Icon SupportSummaryThis PR implements server icon configuration following the MCP specification (2025-11-25). The implementation is well-structured with good test coverage and comprehensive documentation. ✅ Strengths
🔍 Issues Found1. Security: Potential Path Traversal Vulnerability (High Priority)Location: The function accepts user-provided file paths without validation, allowing path traversal attacks like Recommendation: Add path validation to restrict file access to the project directory. 2. Exported Internal Function (Medium Priority)Location: The 3. Example Usage Issue (Medium Priority)Location: The example extracts 4. Missing Dimension Validation (Low Priority)Location: The 📊 Test CoverageThe new code appears well-tested with 446 lines of comprehensive unit tests. Per CLAUDE.md, verify 50% coverage minimum passes. 🔧 Pre-Merge ChecklistPer CLAUDE.md, confirm all four pass: pnpm build && pnpm test && pnpm lint && pnpm typecheck🎯 Priority Actions
✨ ConclusionThis is a solid implementation with excellent testing and documentation. The main concern is the path traversal vulnerability which should be addressed before merge. The other issues are minor improvements. Recommendation: Request changes for the security issue, then approve after fix. Great work overall! 🚀 |
2 participants
Implement server icon configuration following the MCP specification
(2025-11-25). Developers can now provide icons for their MCP servers
that will be displayed in MCP client UIs.
Features:
icon(string URL/data URI) and fulliconsarrayUsage: