feat((versioning): Implemented versioned route apps - #63
Conversation
… tools - Updated README to reflect versioning support and added API endpoint details. - Refactored index.ts to define separate greet tools for v1 (name only) and v2 (name + surname). - Removed deprecated GreetingWidget component and created version-specific UI components. - Enhanced styles.css to support version badges and improved UI layout. - Updated core to support multi-version app configurations and validation.
- Enhanced README with clearer API endpoint descriptions and input/output specifications. - Improved formatting in GreetingWidget components for better readability. - Refactored createApp.ts and server/index.ts for consistent code style and lazy initialization of JWKS client. - Updated OAuth middleware to support JWKS client as a getter function.
- Introduced a new configuration option for the app to specify the protocol as "openai". - Updated createApp.ts to manage a shared HTTP server for multi-version applications, enhancing server instance accessibility. - Improved comments for clarity regarding server instance handling and debug logger configuration.
- Added a new section on API versioning, explaining how to expose multiple versions from a single app. - Included code examples demonstrating version-specific tools, configuration overrides, and middleware. - Updated existing examples to reflect API versioning capabilities.
…ormatting - Added blank lines for better readability in README and core documentation. - Reformatted tool definitions in the core README for consistency. - Ensured consistent code style across examples in the documentation.
|
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 API versioning: new multi-version config types and AppConfigInput union, branching createApp into single- and multi-version flows, per-version routing with lazy JWKS init, server changes for versioned endpoints, App.getVersion/getVersions methods, updated examples/docs, UI changes for minimal example, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Express as Shared Express App
participant Router as Version Router
participant V1 as V1 App Instance
participant V2 as V2 App Instance
participant JWKS as JWKS Client (lazy)
participant Tools as Per-version Tools
Note over Client,Express: Multi-version request dispatch
Client->>+Express: POST /v1/mcp
Express->>+Router: route /v1
Router->>+V1: forward request
alt OAuth required
V1->>+JWKS: resolve/init JWKS (lazy, per-version)
JWKS-->>-V1: jwks client
V1->>V1: verify JWT via middleware
end
V1->>+Tools: invoke v1 tool
Tools-->>-V1: return result
V1-->>-Router: respond
Router-->>-Express: response
Express-->>-Client: 200 OK
Client->>+Express: POST /v2/mcp
Express->>+Router: route /v2
Router->>+V2: forward request
alt OAuth required
V2->>+JWKS: resolve/init JWKS (lazy, per-version)
JWKS-->>-V2: jwks client
V2->>V2: verify JWT via middleware
end
V2->>+Tools: invoke v2 tool
Tools-->>-V2: return result
V2-->>-Router: respond
Router-->>-Express: response
Express-->>-Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Code Review: API Versioning FeatureThis PR introduces a comprehensive API versioning system. Overall, this is a well-implemented feature with excellent test coverage and documentation. ✅ StrengthsArchitecture & Design
Test Coverage
Documentation
🔍 Critical Issues1. OAuth Config Hashing Not Deterministic (createApp.ts:720) Recommendation: Use sorted keys before stringifying or use a proper hash function. 2. Missing JWKS Client Cleanup (createApp.ts:665-918) Recommendation: Add cleanup in stop() method or new shutdown() method.
|
![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 @examples/minimal/src/ui/GreetingWidgetV2.tsx:
- Line 92: In GreetingWidgetV2, the name input's onKeyDown currently checks
`e.key === "Enter" && surname && handleGreet()` which blocks Enter when surname
is empty; remove the `surname` guard so pressing Enter in the name field invokes
handleGreet regardless of surname (e.g. change the handler to `e.key === "Enter"
&& handleGreet()`), and ensure any required validation stays inside handleGreet
so optional surname doesn't prevent submission.
In @packages/core/src/createApp.ts:
- Around line 944-957: The current code only assigns sharedHttpServer to the
first ServerInstance (firstVersion.httpServer) causing getServer().httpServer to
be undefined on other versions; update the resolve callback to iterate over
versionServerInstances.values() and assign sharedHttpServer to each
ServerInstance.httpServer so every version instance gets the same HTTP server
(keep using the existing sharedHttpServer, ServerInstance shape, and preserve
resolve/reject behavior).
♻️ Duplicate comments (1)
packages/core/tests/unit/versioning.test.ts (1)
8-8: Remove unused imports:beforeEachandAddressInfo.
beforeEachis imported but never used. Additionally,AddressInfo(line 11) is imported but not referenced anywhere in the file. As per coding guidelines, unused imports should be removed.Proposed fix
-import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, afterEach } from "vitest"; import { z } from "zod"; import { createApp, type AppConfigInput, type VersionsConfig } from "../../src/index"; -import type { AddressInfo } from "node:net";
🧹 Nitpick comments (7)
packages/core/tests/unit/versioning.test.ts (2)
15-31: Unusedserversarray — cleanup logic never populates it.The
serversarray is declared and cleaned up inafterEach, but tests never push servers to it. Instead, each test manually closesapp.getServer().httpServer. Either remove the unused tracking array or refactor tests to use it consistently.Option A: Remove unused code
-// Track servers for cleanup -const servers: Array<{ close: () => void }> = []; - -afterEach(async () => { - // Close all servers after each test - for (const server of servers) { - await new Promise<void>((resolve) => { - try { - server.close(); - resolve(); - } catch { - resolve(); - } - }); - } - servers.length = 0; -});Option B: Use the array for consistent cleanup
Refactor tests to push servers to the array and rely on
afterEachfor cleanup, removing the manualhttpServer.close()calls in each test.
340-390: Hardcoded ports may cause flaky tests in parallel execution.Tests use fixed ports (3100–3105). If tests run in parallel or another process occupies these ports, tests will fail. Consider using port 0 to let the OS assign an available port, then retrieve it from the server's address.
Example approach
await app.start({ port: 0 }); const httpServer = app.getServer().httpServer; const address = httpServer?.address(); const port = typeof address === 'object' && address ? address.port : 0; // Use `port` for client connectionspackages/core/src/types/config.ts (1)
313-340: Consider branded/template literal types for stricter version key validation.The
versionsfield usesRecord<string, VersionConfig<T>>, relying on runtime validation for thev\d+pattern. For stronger compile-time safety, you could use a template literal type:type VersionKey = `v${number}`; versions: Record<VersionKey, VersionConfig<T>>;This is optional since runtime validation catches invalid keys.
packages/core/src/createApp.ts (4)
336-364: Clarify shallow merge behavior for nested config objects.The merge logic uses nullish coalescing (
??) for nested objects likeoauth,cors, etc., which means version-specific config entirely replaces global config rather than deep-merging. This is likely intentional but worth documenting explicitly in the JSDoc comment to avoid confusion for users expecting deep merge behavior.🔎 Suggested documentation enhancement
/** * Merge global config with version-specific config * Version-specific config takes precedence over global config + * Note: Nested objects (oauth, cors, openai, debug, protocol) are replaced entirely + * by version-specific values, not deep-merged. */ function mergeVersionConfig<T extends ToolDefs>(
287-295: Dead code: version route can never conflict with/health.The check
if (versionRoute === "/health")is unreachable. SinceversionRouteis constructed as/${versionKey}/mcpandversionKeymust match/^v\d+$/, the resulting route (e.g.,/v1/mcp) can never equal/health. This appears to be copy-paste from single-version validation.🔎 Suggested removal
validateVersionConfig(versionKey, versionConfig); - - // Validate that version route doesn't conflict with reserved routes - const versionRoute = `/${versionKey}/mcp`; - if (versionRoute === "/health") { - throw new AppError( - ErrorCode.INVALID_CONFIG, - `Version "${versionKey}" route conflicts with health check endpoint` - ); - } }
719-727: Consider stable cache key generation for OAuth config.Using
JSON.stringifydirectly is sensitive to property order. If the same OAuth configuration is defined with different property ordering, separate JWKS clients would be created unnecessarily. While this is unlikely in practice, a stable serialization approach would be more robust.🔎 Suggested stable key generation
+ // Helper to create a stable cache key from OAuth config + function stableStringify(obj: unknown): string { + return JSON.stringify(obj, Object.keys(obj as object).sort()); + } + // Create version-specific OAuth JWKS client key (for reuse if config is identical) const oauthConfigKey = normalizedVersionConfig.config?.oauth - ? JSON.stringify(normalizedVersionConfig.config.oauth) + ? stableStringify(normalizedVersionConfig.config.oauth) : "no-oauth";
925-927: Clarify behavior:mainApp.toolsreturns first version's tools only.For multi-version apps,
mainApp.toolsreturns the first version's tool definitions, which may not represent all available tools across versions. Consider adding a JSDoc comment to clarify this or providing a method to access all versions' tools if needed.🔎 Suggested documentation
// Create main app instance that delegates to version apps const mainApp: App<T> = { - // Use tools from first version (for type inference) + // Use tools from first version (for type inference). + // To access a specific version's tools, use getVersion(key).tools tools: (Object.values(config.versions)[0] as VersionConfig<T> | undefined)?.tools as T,
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.mdexamples/minimal/README.mdexamples/minimal/src/index.tsexamples/minimal/src/ui/GreetingWidgetV1.tsxexamples/minimal/src/ui/GreetingWidgetV2.tsxexamples/minimal/src/ui/styles.csspackages/core/README.mdpackages/core/src/createApp.tspackages/core/src/server/index.tspackages/core/src/server/oauth/middleware.tspackages/core/src/types/config.tspackages/core/src/types/tools.tspackages/core/tests/unit/versioning.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
examples/minimal/src/ui/GreetingWidgetV2.tsxpackages/core/src/types/tools.tspackages/core/src/types/config.tspackages/core/tests/unit/versioning.test.tspackages/core/src/server/index.tsexamples/minimal/src/index.tsexamples/minimal/src/ui/GreetingWidgetV1.tsxpackages/core/src/server/oauth/middleware.tspackages/core/src/createApp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
examples/minimal/src/ui/GreetingWidgetV2.tsxpackages/core/src/types/tools.tspackages/core/src/types/config.tspackages/core/tests/unit/versioning.test.tspackages/core/src/server/index.tsexamples/minimal/src/index.tsexamples/minimal/src/ui/GreetingWidgetV1.tsxpackages/core/src/server/oauth/middleware.tspackages/core/src/createApp.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
packages/core/src/types/tools.tspackages/core/src/types/config.tspackages/core/tests/unit/versioning.test.tspackages/core/src/server/index.tsexamples/minimal/src/index.tspackages/core/src/server/oauth/middleware.tspackages/core/src/createApp.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/core/src/types/tools.tspackages/core/src/types/config.tspackages/core/tests/unit/versioning.test.tspackages/core/src/server/index.tsexamples/minimal/src/index.tspackages/core/src/server/oauth/middleware.tspackages/core/src/createApp.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/core/tests/unit/versioning.test.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
packages/core/src/server/index.tsexamples/minimal/src/index.ts
🧠 Learnings (8)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
README.mdpackages/core/src/types/tools.tspackages/core/src/types/config.tsexamples/minimal/src/index.tsexamples/minimal/README.mdpackages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
README.mdexamples/minimal/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Implement plugins using the `Plugin` interface with hooks: `onInit`, `onStart`, `onShutdown`, `beforeToolCall`, `afterToolCall`, `onToolError`
Applied to files:
packages/core/src/types/config.tspackages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/core/tests/unit/versioning.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Remove unused variables or prefix with underscore (`_`)
Applied to files:
packages/core/tests/unit/versioning.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
examples/minimal/src/index.tsexamples/minimal/README.mdpackages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Koa-style async/await middleware pattern with `await next()` to chain middleware execution
Applied to files:
packages/core/src/server/oauth/middleware.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
Applied to files:
packages/core/src/createApp.ts
🧬 Code graph analysis (7)
examples/minimal/src/ui/GreetingWidgetV2.tsx (1)
examples/minimal/src/index.ts (1)
AppClientToolsV2(173-173)
packages/core/src/types/tools.ts (1)
packages/core/src/index.ts (1)
App(41-41)
packages/core/src/types/config.ts (4)
packages/core/src/types/tools.ts (1)
ToolDefs(332-332)packages/core/src/index.ts (4)
ToolDefs(39-39)UIDefs(50-50)Plugin(75-75)AppConfig(58-58)packages/core/src/types/ui.ts (1)
UIDefs(141-141)packages/core/src/plugins/types.ts (1)
Plugin(163-275)
packages/core/tests/unit/versioning.test.ts (1)
packages/core/src/createApp.ts (1)
createApp(413-423)
packages/core/src/server/index.ts (2)
packages/core/src/server/oauth/middleware.ts (1)
createOAuthMiddleware(166-225)packages/core/src/utils/errors.ts (1)
wrapError(204-217)
examples/minimal/src/ui/GreetingWidgetV1.tsx (1)
examples/minimal/src/index.ts (1)
AppClientToolsV1(167-167)
packages/core/src/server/oauth/middleware.ts (2)
packages/core/src/server/oauth/errors.ts (2)
OAuthError(7-128)ErrorCode(133-151)packages/core/src/server/oauth/jwt-verifier.ts (1)
verifyJWT(50-155)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (33)
README.md (2)
65-65: LGTM! Clear feature addition.The API Versioning feature is appropriately highlighted in the features list, aligning with the PR objectives.
185-248: Comprehensive API versioning documentation with clear examples.The new section provides excellent guidance on multi-version API support. The TypeScript example clearly demonstrates:
- Shared configuration across versions
- Per-version tool definitions with distinct schemas
- Version-specific config overrides
- Programmatic access via
getVersionandgetVersions- Dedicated route exposure
The example accurately reflects the multi-version architecture introduced in this PR.
examples/minimal/src/ui/styles.css (3)
18-39: LGTM! Well-structured version badge UI.The addition of
position: relativeto.containerenables proper absolute positioning of the version badge. The badge styling with backdrop-filter and distinct v2 variant provides clear visual differentiation between API versions.
60-64: Clean styling for the full-name display.The subtle typography and opacity create appropriate visual hierarchy within the greeting component.
136-149: Improved form layout with input-group.The flexbox column layout with proper spacing and placeholder styling enhances the modal input experience for both V1 and V2 UIs.
packages/core/README.md (3)
217-267: Excellent foundational versioning documentation.The basic usage section clearly demonstrates:
- Shared config inheritance across versions
- Per-version tool definitions with distinct schemas (v1 takes only
name, v2 adds optionalsurname)- Proper route mapping documentation
The example effectively illustrates the core versioning capability.
391-403: Clear version key validation requirements.The explicit pattern
/^v\d+$/with valid and invalid examples prevents confusion. This regex-based constraint ensures consistent route structure and parsing.
413-431: Important backward compatibility documentation.Clearly documenting that
getVersions()returns an empty array andgetVersion()returnsundefinedfor single-version apps ensures existing code continues to work without modification.packages/core/src/types/tools.ts (1)
505-543: Well-defined versioning API surface.The addition of
getVersionandgetVersionsmethods to theAppinterface provides clean programmatic access to version-specific app instances:
- Return types are correct:
getVersionreturnsApp<T> | undefined(undefined for single-version apps or missing versions), andgetVersionsreturnsstring[](empty for single-version apps).- JSDoc is comprehensive: Examples clearly demonstrate usage patterns and return value expectations.
- Type preservation: Both methods maintain the generic
T extends ToolDefsparameter, ensuring type safety across version boundaries.These additions align perfectly with the multi-version architecture documented across the PR.
packages/core/src/server/oauth/middleware.ts (1)
166-189: Solid implementation of lazy JWKS client initialization.The enhanced OAuth middleware correctly supports lazy initialization through a getter function pattern:
- Type-safe resolution: The function type guard
typeof jwksClient === "function"safely identifies and resolves getters.- Proper error handling: Throws
OAuthErrorwithINVALID_REQUESTcode when the resolved client isnull, providing a clear diagnostic message.- Clean integration: The resolved client is correctly passed to
verifyJWTon line 189, maintaining the existing verification flow.This pattern enables per-version JWKS clients in multi-version deployments, as described in the PR context. The implementation follows the Koa-style async/await middleware pattern per coding guidelines.
packages/core/tests/unit/versioning.test.ts (1)
33-709: Comprehensive test coverage for versioning feature.The test suite thoroughly covers multi-version app creation, version key validation, config merging, route isolation, tool execution isolation, backward compatibility, shared Express app references, and version-specific middleware. This provides strong confidence in the versioning implementation.
packages/core/src/types/config.ts (2)
240-278: Well-structured version configuration type.The
VersionConfiginterface properly captures version-specific tools, UI, config overrides, and plugins with appropriate generics and documentation.
412-417: Clean union type for backward-compatible API.
AppConfigInput<T>elegantly supports both single-version (AppConfig) and multi-version (VersionsConfig) configurations, enabling seamless migration for existing users.examples/minimal/src/ui/GreetingWidgetV2.tsx (1)
12-57: Clean component implementation with proper state management.Good separation of concerns with local state for modal control, form inputs, loading/error states, and tool result caching. The
greetOutputfallback pattern (greetResult ?? result?.greet) correctly prioritizes local state over hook result.examples/minimal/README.md (1)
1-145: Comprehensive documentation update for versioning feature.The README clearly explains the new versioning capabilities with:
- Feature overview distinguishing v1 and v2 APIs
- API endpoints table
- Working curl examples for both versions
- Updated Claude Desktop configuration
- Project structure reflecting versioned UI components
- Versioning configuration code example
packages/core/src/server/index.ts (3)
77-82: Well-designed API extension for versioned servers.The updated signature supports:
- Lazy JWKS client initialization via getter function
- Custom version routes for multi-version deployments
Backward compatible — existing single-version apps work without changes.
147-152: Good lazy initialization pattern for JWKS client.Normalizing the JWKS client to a getter function (
getJwksClient) enables deferred initialization, which is useful when the client isn't needed immediately or when sharing across versions.
244-278: Correct conditional mounting of global endpoints.Versioned servers delegate health checks, domain verification, and 404 handling to the shared parent Express app. This avoids duplicate endpoints and ensures consistent behavior across versions.
examples/minimal/src/ui/GreetingWidgetV1.tsx (1)
12-15: Clean version-specific refactoring.Component correctly renamed to
GreetingWidgetV1with proper type narrowing toAppClientToolsV1. The version badge at line 54 provides clear visual identification.examples/minimal/src/index.ts (4)
30-54: V1 tool follows coding guidelines.Uses
defineToolwith Zod schemas for input/output validation anddefineReactUIfor colocated UI definition. Handler correctly returns structured output with_textfor model narration.
71-97: V2 tool properly extends V1 capabilities.Adds optional
surnamefield andfullNameoutput while maintaining the same patterns. Good demonstration of backward-compatible API evolution.
126-135: V2 uses different protocol — verify this is intentional.V2 overrides
protocol: "openai"while V1 uses the sharedprotocol: "mcp". This means V2 will use snake_case metadata format. If this is intentional for demonstration purposes, consider adding a code comment explaining why the protocols differ.
165-175: Well-organized type exports for UI consumers.Separate exports for V1 and V2 types (
AppToolsV1/AppClientToolsV1,AppToolsV2/AppClientToolsV2) enable type-safe UI development for each version.packages/core/src/createApp.ts (10)
7-31: LGTM!Imports are well-organized and appropriate for the new multi-version functionality. The separation of type imports using
import typefollows TypeScript best practices.
72-91: LGTM!The type guard
isVersionsConfigand version key validation are well-implemented. The regex pattern/^v\d+$/clearly enforces the expected format (v1, v2, etc.), and error messages are descriptive.
93-128: LGTM!Comprehensive validation of version-specific configuration with proper delegation to
validateGlobalConfigfor nested config validation. The error messages clearly identify which version has the issue.
457-522: LGTM!The lazy OAuth initialization pattern is well-implemented with proper idempotency handling. The promise-based approach correctly handles concurrent initialization attempts by having subsequent callers await the in-progress promise.
639-652: LGTM!The versioning method stubs for single-version apps correctly return
undefined/empty array, maintaining a consistent API surface while indicating versioning is not applicable. The underscore prefix for_versionKeyfollows the coding guidelines for unused parameters.
1022-1063: LGTM!The
onceimplementation correctly ensures the handler fires only once across all versions, even with concurrent event emissions. Settingfired = truebefore calling the handler prevents race conditions, and the immediate unsubscribe loop prevents other versions from triggering duplicate calls.
979-999: LGTM!The request routing logic correctly parses the URL path and delegates to the appropriate version app based on the
/vN/mcpprefix pattern. The fallback to 404 for unmatched routes is consistent with the Express catch-all handler.
600-606: LGTM!The
handleRequestsignature correctly usesglobalThis.Request/globalThis.Responsefor Web API types andunknownfor the optionalenvparameter, following strict TypeScript guidelines.
903-922: LGTM!The shared Express app correctly adds:
- Health endpoint with version listing
- OpenAI domain verification challenge (when configured)
- Catch-all 404 handler
These endpoints are appropriately placed after version-specific routes are mounted.
960-971: No action required; type casting works as designed.The return statement safely casts
ServerInstancetoMcpServerwithout runtime issues. SinceMcpServeris an opaque interface accepting any properties ([key: string]: unknown), consumers can accesshttpServerand otherServerInstanceproperties without errors. Tests throughout the codebase confirm this pattern works correctly.Likely an incorrect or invalid review comment.
…App comments - Removed unnecessary surname check in GreetingWidgetV2 onKeyDown event. - Updated comments in createApp.ts for clarity on version-specific config merging and server instance handling. - Cleaned up versioning test file by removing unused server cleanup logic.
gabrypavanello
changed the title
| * and backward compatibility with single-version apps. | ||
| */ | ||
|
|
||
| import { describe, it, expect, afterEach } from "vitest"; |
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository |
|
@cursor review |
Code Review: API Versioning FeatureI've completed a thorough review of PR #63 which adds API versioning support to mcp-apps-kit. This is a well-architected feature with solid implementation. ✅ Strengths1. Excellent Architecture & Design
2. Robust Validation
3. Strong Test Coverage
4. Documentation Quality
🔍 Issues & ConcernsCRITICAL: Event Handler Memory Leak (High Priority) Location: createApp.ts:1015-1056 The once() implementation for multi-version apps has a race condition that can cause memory leaks. If version A fires the event before version B registers, version B's wrapper stays attached but will never execute (due to fired flag). The handler stays in memory indefinitely. Fix: Unsubscribe from all versions BEFORE calling the handler to prevent race conditions. MAJOR: Missing Version Validation (Medium Priority) Location: createApp.ts:972-992 The handleRequest() method for serverless doesn't validate that the version key exists before routing. Valid version pattern (v999) but non-existent version returns generic 404. Should return explicit Version not found error with available versions. MINOR Issues:
📊 Testing AssessmentCoverage: Excellent (691 new test lines) Missing Tests:
🔒 Security Review✅ No security vulnerabilities detected
📝 Recommendation Summary
✅ Approval StatusConditional Approval - Fix the CRITICAL issue before merge. This is a well-designed feature that adds significant value to the framework. The implementation is solid, documentation is excellent, and test coverage is strong. However, the event handler race condition in once() must be addressed before merging to prevent memory leaks in production. After fixing the critical issue, this will be a great addition to mcp-apps-kit! 🎉 Great work overall! The versioning architecture is clean and maintainable. 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @packages/core/tests/unit/versioning.test.ts:
- Line 8: The test file imports afterEach but never uses it; remove the unused
symbol by updating the import statement that currently reads "import { describe,
it, expect, afterEach } from 'vitest';" to drop afterEach so it becomes "import
{ describe, it, expect } from 'vitest';" and ensure no other references to
afterEach exist in versioning.test.ts.
🧹 Nitpick comments (2)
packages/core/src/createApp.ts (1)
712-716: Consider stable cache key generation for OAuth config.Using
JSON.stringifyfor the OAuth config cache key (line 714) could theoretically produce different keys if property ordering varies, though this is unlikely in practice since configs come from the same source.Alternative approach using sorted keys
// Helper function to create stable cache key function createOAuthCacheKey(oauthConfig: OAuthConfig): string { const keys = Object.keys(oauthConfig).sort(); const parts = keys.map(k => `${k}:${JSON.stringify(oauthConfig[k as keyof OAuthConfig])}`); return parts.join('|'); } // Usage const oauthConfigKey = normalizedVersionConfig.config?.oauth ? createOAuthCacheKey(normalizedVersionConfig.config.oauth) : "no-oauth";packages/core/tests/unit/versioning.test.ts (1)
365-370: Consider extracting server cleanup into a helper.The manual server cleanup pattern is repeated across multiple tests. While explicit and correct, extracting it into a helper function would reduce duplication.
Example helper function
async function closeServer(app: ReturnType<typeof createApp>): Promise<void> { const httpServer = app.getServer().httpServer; if (httpServer) { await new Promise<void>((resolve) => { httpServer.close(() => resolve()); }); } } // Usage in tests: await closeServer(app);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/minimal/src/ui/GreetingWidgetV2.tsxpackages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/minimal/src/ui/GreetingWidgetV2.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/core/tests/unit/versioning.test.ts
🧠 Learnings (6)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Implement plugins using the `Plugin` interface with hooks: `onInit`, `onStart`, `onShutdown`, `beforeToolCall`, `afterToolCall`, `onToolError`
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/core/tests/unit/versioning.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Remove unused variables or prefix with underscore (`_`)
Applied to files:
packages/core/tests/unit/versioning.test.ts
🧬 Code graph analysis (1)
packages/core/tests/unit/versioning.test.ts (1)
packages/core/src/createApp.ts (1)
createApp(406-416)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (12)
packages/core/src/createApp.ts (9)
8-31: LGTM!The type imports properly support the multi-version architecture. All imports are necessary for the new functionality.
75-79: LGTM!The type guard correctly discriminates between single-version and multi-version configurations using a property check.
84-321: LGTM!The validation functions provide comprehensive runtime checks for both single-version and multi-version configurations. Proper use of
AppErrorandErrorCodefor error handling, and good integration with Zod schemas for OAuth validation.
329-357: LGTM!The config merging logic correctly prioritizes version-specific config over global config. The shallow merge approach for nested objects (line 339-343) and plugin array concatenation (line 347) align with the documented behavior.
406-416: LGTM!The entry point correctly delegates to version-specific implementations based on config shape. Clean separation of concerns.
421-653: LGTM!The single-version implementation maintains backward compatibility while extending the API with stub implementations of
getVersionandgetVersions(lines 636-645). The lazy OAuth initialization pattern (line 520) is properly documented.
940-943: Past issue resolved: HTTP server now attached to all version instances.The shared HTTP server is correctly attached to all version
ServerInstanceobjects, addressing the previous review feedback. This ensuresgetServer().httpServerworks consistently across all versions.
1015-1056: LGTM: Complex but correctonce()implementation.The multi-version
once()implementation correctly ensures the handler fires exactly once across all versions by using shared state (firedandisUnsubscribedflags) and immediately unsubscribing from all versions after the first execution. The complexity is justified by the multi-version coordination requirement.
658-1081: LGTM: Well-structured multi-version orchestration.The implementation provides:
- Shared Express app with per-version routing and middleware isolation (lines 660, 811)
- OAuth JWKS client caching and reuse across versions with identical configs (lines 672-674)
- Lazy OAuth initialization with proper promise management (lines 724-788)
- Comprehensive multi-version event handling (lines 1002-1069)
- Shared health and OpenAI challenge endpoints (lines 896-910)
packages/core/tests/unit/versioning.test.ts (3)
14-440: LGTM: Comprehensive test coverage for versioning features.The tests thoroughly exercise multi-version app creation, version APIs, validation, config merging, and route isolation. Each test properly cleans up HTTP servers to avoid resource leaks.
442-526: LGTM: Tool execution isolation properly tested.The test validates that tools with the same name but different signatures execute independently per version, confirming proper isolation.
528-691: LGTM: Backward compatibility and middleware isolation well tested.The tests ensure:
- Single-version apps maintain backward compatibility (lines 528-568)
- All versions share the same Express app instance (lines 570-595)
- Version-specific middleware executes in isolation (lines 597-690)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…eApp - Implemented a health check endpoint that returns the app status and available versions. - Added support for handling OpenAI domain verification challenge requests. - Updated routing logic to return 404 for unmatched routes, ensuring consistency with Express behavior. - Enhanced comments for clarity on event handling and memory management in the request processing flow.
Code Review: API Versioning FeatureI've completed a thorough review of this PR. Overall, this is a well-designed and thoughtfully implemented feature that adds significant value to the framework. The code quality is high, tests are comprehensive, and documentation is excellent. ✅ StrengthsArchitecture & Design
Code Quality
Testing
Documentation
🔍 Issues & Concerns1. Memory Management - HTTP Server Leak Risk
|
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @packages/core/tests/unit/versioning.test.ts:
- Around line 75-84: The test name is misleading: it says "should return
undefined for getVersions()" but the assertion checks for an empty array; update
the test description to reflect expected behavior. Change the it(...) string in
the versioning.test.ts case that calls app.getVersions() to something like
"should return an empty array for getVersions() in single-version mode" so the
test name matches the assertion and implementation (refer to the test invoking
createApp(...) and app.getVersions()).
♻️ Duplicate comments (1)
packages/core/tests/unit/versioning.test.ts (1)
8-8: Remove unusedafterEachimport.The
afterEachimport is never used in this test file. As per coding guidelines, unused imports should be removed.🔎 Suggested fix
-import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect } from "vitest";
🧹 Nitpick comments (1)
packages/core/tests/unit/versioning.test.ts (1)
210-289: Config merging tests lack behavioral verification.These tests create apps with merged configurations but only assert that the app is defined. The comments indicate intent to verify config override behavior (e.g., "Version-specific config should override global"), but there's no actual verification. Consider adding assertions that confirm the merged config is applied correctly, for example by checking observable behavior affected by the config.
📜 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 (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/core/tests/unit/versioning.test.ts
🧠 Learnings (6)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Implement plugins using the `Plugin` interface with hooks: `onInit`, `onStart`, `onShutdown`, `beforeToolCall`, `afterToolCall`, `onToolError`
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/core/tests/unit/versioning.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Remove unused variables or prefix with underscore (`_`)
Applied to files:
packages/core/tests/unit/versioning.test.ts
🧬 Code graph analysis (1)
packages/core/tests/unit/versioning.test.ts (1)
packages/core/src/createApp.ts (1)
createApp(406-416)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (10)
packages/core/tests/unit/versioning.test.ts (1)
1-806: Comprehensive test coverage for versioning feature.The test suite thoroughly covers the versioning functionality including multi-version creation, version key validation, route isolation, tool execution isolation, backward compatibility, and serverless deployment scenarios. The test structure is clean and follows good practices.
packages/core/src/createApp.ts (9)
84-91: LGTM!The version key validation correctly enforces the
/^v\d+$/pattern with a clear error message.
329-357: LGTM!The config merging logic correctly handles version-specific overrides with well-documented shallow merge behavior for nested objects. The empty name is appropriately set from global config at the call site.
421-653: LGTM!Single-version app creation properly maintains backward compatibility with lazy OAuth initialization and correct handling of
getVersion/getVersionsmethods.
712-788: OAuth client caching is well-implemented.The lazy OAuth initialization with per-config caching via
JSON.stringifyis a pragmatic approach that correctly handles:
- Reusing clients when configs are identical
- Waiting for in-progress initialization
- Cleaning up on failure to allow retry
805-811: LGTM!The Express app mounting correctly composes version-specific routers onto the shared app, maintaining route isolation between versions.
972-1020: Shared endpoint handling in handleRequest is complete.The implementation correctly handles
/healthand/.well-known/openai-apps-challengeendpoints in the serverlesshandleRequestpath, addressing the concern from the previous review.
1043-1086: Well-designedonce()implementation with proper race condition handling.The implementation correctly handles the "fire only once across all versions" semantics with:
- Early
firedflag set before unsubscribing to prevent races- Cleanup before handler execution to prevent memory leaks
- Guard against duplicate unsubscribe calls
The comments clearly explain the design rationale.
939-944: HTTP server correctly attached to all version instances.The implementation now properly attaches the shared HTTP server to all version
ServerInstanceobjects, addressing the previous review concern about only attaching to the first version.
658-1111: Solid multi-version app implementation.The
createMultiVersionAppfunction is well-structured with:
- Proper sharing of Express app and HTTP server across versions
- Efficient OAuth client caching by config hash
- Correct event propagation semantics for
on,once, andonAny- Comprehensive
handleRequestsupport for serverless deployments- Clear documentation of design decisions in comments
| it("should return undefined for getVersions() in single-version mode", () => { | ||
| const app = createApp({ | ||
| name: "test-app", | ||
| version: "1.0.0", | ||
| tools: {}, | ||
| }); | ||
|
|
||
| const versions = app.getVersions(); | ||
| expect(versions).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
Fix misleading test name.
The test name says "should return undefined for getVersions()" but the assertion expects an empty array [], not undefined. The assertion is correct based on the implementation, but the test name is misleading.
🔎 Suggested fix
- it("should return undefined for getVersions() in single-version mode", () => {
+ it("should return empty array for getVersions() in single-version mode", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should return undefined for getVersions() in single-version mode", () => { | |
| const app = createApp({ | |
| name: "test-app", | |
| version: "1.0.0", | |
| tools: {}, | |
| }); | |
| const versions = app.getVersions(); | |
| expect(versions).toEqual([]); | |
| }); | |
| it("should return empty array for getVersions() in single-version mode", () => { | |
| const app = createApp({ | |
| name: "test-app", | |
| version: "1.0.0", | |
| tools: {}, | |
| }); | |
| const versions = app.getVersions(); | |
| expect(versions).toEqual([]); | |
| }); |
🤖 Prompt for AI Agents
In @packages/core/tests/unit/versioning.test.ts around lines 75-84, The test
name is misleading: it says "should return undefined for getVersions()" but the
assertion checks for an empty array; update the test description to reflect
expected behavior. Change the it(...) string in the versioning.test.ts case that
calls app.getVersions() to something like "should return an empty array for
getVersions() in single-version mode" so the test name matches the assertion and
implementation (refer to the test invoking createApp(...) and
app.getVersions()).
- Updated `validateGlobalConfig` to accept `VersionSpecificConfig`, allowing null values to disable properties. - Introduced `deepMerge` function for recursively merging global and version-specific configurations, with support for null to remove properties. - Updated `mergeVersionConfig` to utilize deep merging for nested objects, ensuring proper handling of undefined and null values. - Added tests for deep merging behavior, including scenarios for overriding, disabling, and inheriting configurations. - Enhanced type definitions for better clarity on configuration structures.
Code Review: API Versioning ImplementationSummaryThis PR implements a comprehensive API versioning feature for ✅ Strengths1. Architecture & Design
2. Type Safety
3. Testing
4. Documentation
5. Error Handling
🔍 Issues & ConcernsCritical Issues1. Version Key Constraint Too RestrictiveLocation: function validateVersionKey(versionKey: string): void {
if (!/^v\d+$/.test(versionKey)) {
throw new AppError(
ErrorCode.INVALID_CONFIG,
`Version key must match pattern /^v\\d+$/, got: "${versionKey}"`
);
}
}Issue: The pattern
Impact: Limits flexibility for real-world API versioning strategies. Recommendation: Consider relaxing to 2. OAuth JWKS Client Shared Across VersionsLocation: // Create version-specific OAuth JWKS client key (for reuse if config is identical)
const oauthConfigKey = normalizedVersionConfig.config?.oauth
? JSON.stringify(normalizedVersionConfig.config.oauth)
: "no-oauth";Issue: Using
Example failure case: // These should be identical but create different keys:
{ authorizationServer: "https://auth.com", scopes: ["read"] }
{ scopes: ["read"], authorizationServer: "https://auth.com" }Recommendation: Use a stable hash function or compare normalized config objects: const oauthConfigKey = normalizedVersionConfig.config?.oauth
? crypto
.createHash('sha256')
.update(
JSON.stringify(
Object.keys(config.oauth)
.sort()
.reduce((acc, key) => ({ ...acc, [key]: config.oauth[key] }), {})
)
)
.digest('hex')
: "no-oauth";3. Missing Validation for serverRoute in Versioned ConfigLocation: if (serverRoute === "/health") {
throw new AppError(
ErrorCode.INVALID_CONFIG,
`${prefix}.serverRoute cannot be "/health" as it conflicts with the health check endpoint`
);
}Issue: The validation prevents Recommendation: Add validation or warning when if (versionConfig.config?.serverRoute !== undefined) {
throw new AppError(
ErrorCode.INVALID_CONFIG,
`Version "${versionKey}".config.serverRoute is not supported in multi-version apps. Routes are automatically assigned as /{versionKey}/mcp`
);
}Medium Issues4. Debug Logger Not Configured Per-VersionLocation: // Note: Debug logger is configured once with global config (line 681-683).
// We don't reconfigure it per-version because it's a global singleton.Issue: This is documented but may lead to unexpected behavior. If v1 has Recommendation: Either:
5. Type Inference Issue for Multi-Version AppsLocation: tools: (Object.values(config.versions)[0] as VersionConfig<T> | undefined)?.tools as T,Issue: The main app's
Recommendation: Document this clearly or provide a discriminated union type for multi-version tool access: // Better API:
const v1App = app.getVersion("v1");
const result = await v1App.tools.greet({ name: "Alice" }); // Type-safe6. Event Deduplication in
|
…ation - Eliminated example curl commands from the index.ts file to streamline the documentation. - Focused on providing a clearer overview of available endpoints without cluttering the content.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/createApp.ts (1)
1-1: Fix Prettier formatting to resolve pipeline failure.The CI pipeline is failing due to formatting issues. Run
prettier --write packages/core/src/createApp.tsto fix.
♻️ Duplicate comments (2)
packages/core/tests/unit/versioning.test.ts (2)
8-8: Remove unused import.The
afterEachimport is unused. Tests perform manual cleanup inline rather than using anafterEachhook.As per coding guidelines, unused imports should be removed.
75-84: Fix misleading test name.The test name says "should return undefined for getVersions()" but the assertion expects an empty array
[], notundefined. The assertion is correct based on the implementation, but the test name is misleading.🔎 Suggested fix
- it("should return undefined for getVersions() in single-version mode", () => { + it("should return empty array for getVersions() in single-version mode", () => {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/createApp.tspackages/core/src/index.tspackages/core/src/types/config.tspackages/core/tests/unit/versioning.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/core/src/index.tspackages/core/src/types/config.tspackages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/core/src/index.tspackages/core/src/types/config.tspackages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
packages/core/src/index.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
packages/core/src/index.tspackages/core/src/types/config.tspackages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/core/src/index.tspackages/core/src/types/config.tspackages/core/src/createApp.tspackages/core/tests/unit/versioning.test.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/core/tests/unit/versioning.test.ts
🧠 Learnings (7)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/core/src/types/config.tspackages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
packages/core/src/types/config.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Implement plugins using the `Plugin` interface with hooks: `onInit`, `onStart`, `onShutdown`, `beforeToolCall`, `afterToolCall`, `onToolError`
Applied to files:
packages/core/src/types/config.tspackages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use `AppError` and `ErrorCode` from `mcp-apps-kit/core` for error handling
Applied to files:
packages/core/src/createApp.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/core/tests/unit/versioning.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Remove unused variables or prefix with underscore (`_`)
Applied to files:
packages/core/tests/unit/versioning.test.ts
🧬 Code graph analysis (1)
packages/core/src/types/config.ts (3)
packages/core/src/types/tools.ts (1)
ToolDefs(332-332)packages/core/src/types/ui.ts (1)
UIDefs(141-141)packages/core/src/plugins/types.ts (1)
Plugin(163-275)
🪛 GitHub Actions: PR Check
packages/core/src/createApp.ts
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in packages/core/src/createApp.ts. Step: 'pnpm format' (prettier --check .) exited with code 1.
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (10)
packages/core/src/types/config.ts (1)
248-449: LGTM! Well-structured type definitions for multi-version support.The new types are well-designed and thoroughly documented:
DeepPartialWithNull<T>enables fine-grained control over config inheritance with clear null vs undefined semanticsVersionSpecificConfig,VersionConfig, andVersionsConfigprovide a clean API for multi-version appsAppConfigInputunion maintains backward compatibility while enabling versioning- Documentation includes helpful examples and clear explanations
packages/core/src/createApp.ts (7)
73-79: LGTM! Simple and effective type guard.The
isVersionsConfigtype guard correctly checks for theversionsproperty to distinguish multi-version configs from single-version configs.
81-324: LGTM! Comprehensive validation with proper type narrowing.The validation functions are well-structured:
validateVersionKeyenforces the/^v\d+$/patternvalidateVersionConfigvalidates all required fields and optional overridesvalidateGlobalConfigproperly handles null values for deep merge semanticsvalidateConfigsafely narrows fromunknownand validates both single and multi-version configs- All use
AppErrorandErrorCodeas per coding guidelines- Error messages are clear and helpful
As per coding guidelines, proper error handling with
AppErrorandErrorCodeis used throughout.
326-448: LGTM! Correct deep merge implementation.The deep merge logic correctly implements the documented semantics:
nullexplicitly disables/removes propertiesundefinedinherits from global config- Objects are recursively merged
- Arrays and primitives are replaced (not merged)
- Type safety is maintained with
Record<string, unknown>and appropriate castingThe
mergeVersionConfigfunction properly applies deep merge to nested config objects while handling primitive properties separately.
497-507: LGTM! Clean dispatching logic.The updated
createAppsignature acceptsAppConfigInput<T>and uses theisVersionsConfigtype guard to dispatch to the appropriate implementation, maintaining backward compatibility while enabling multi-version support.
509-744: LGTM! Backward-compatible single-version implementation.The
createSingleVersionAppfunction maintains backward compatibility while supporting the new API:
- Lazy OAuth JWKS initialization is idempotent and handles both server and serverless scenarios
- Middleware chain follows Koa-style async/await pattern as per coding guidelines
- Error handling uses
AppErrorandErrorCodeas requiredgetVersion()andgetVersions()return appropriate values for single-version appsAs per coding guidelines, Koa-style async/await middleware pattern is used.
746-1002: LGTM! Well-architected multi-version implementation.The first part of
createMultiVersionAppsets up the shared infrastructure:
- Shared Express app for all versions with per-version sub-apps
- OAuth JWKS clients cached by config hash for efficient reuse across versions
- Per-version lazy OAuth initialization is idempotent and handles race conditions
- Version-specific server instances properly mounted with version routes
- Shared endpoints (
/healthand/.well-known/openai-apps-challenge) correctly added to shared app
1008-1214: LGTM! Comprehensive multi-version app orchestration.The main multi-version app instance correctly:
- Delegates all operations to version-specific apps
- Starts all versions and the shared HTTP server
- Handles shared endpoints (
/health,/.well-known/openai-apps-challenge) inhandleRequest()for serverless deployments- Applies middleware to all version apps via
use()- Delegates event subscriptions to all versions
- Implements
once()with proper shared state to ensure handler fires only once across all versions- Provides
getVersion()andgetVersions()for programmatic accessThe
once()implementation (lines 1146-1189) is notably complex but correct—it properly handles race conditions and ensures cleanup even if the handler throws.packages/core/src/index.ts (1)
61-62: LGTM! Proper type-only exports.The new type exports (
VersionSpecificConfigandDeepPartialWithNull) are correctly added to the public API surface usingexport typeas per coding guidelines.As per coding guidelines,
export typeis used for type-only exports.packages/core/tests/unit/versioning.test.ts (1)
14-967: Excellent test coverage for multi-version functionality.The test suite comprehensively covers:
- Multi-version app creation and version retrieval
- Version key validation and error handling
- Config merging (global + version-specific) with deep merge semantics
- Route isolation across versions
- Backward compatibility with single-version apps
- Per-version middleware behavior
- Serverless
handleRequest()scenarios including shared endpoints- Tool execution isolation across versions
Well-structured and follows testing guidelines by placing tests in the
unit/subdirectory.As per coding guidelines, test file structure mirrors source with tests in
tests/unit/subdirectory.
Code Review: API Versioning ImplementationThank you for this substantial feature addition! This PR implements API versioning support for MCP AppsKit, allowing multiple API versions to be exposed from a single application. Here's my detailed review: ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Documentation
3. Robust Testing
4. Security & Best Practices
🔍 Issues & ConcernsCritical Issues1. Resource Leak in Multi-Version Apps // packages/core/src/createApp.ts:899
// Express strips the mount path, so routes registered at serverRoute on the version appThe comment ends mid-sentence, suggesting incomplete implementation. More importantly, I don't see cleanup logic for:
Recommendation: Implement a 2. Debug Logger Singleton Issue 🐛 // packages/core/src/createApp.ts:788-791
// Note: Debug logger is configured once with global config (line 681-683).
// We don't reconfigure it per-version because it's a global singleton.This is explicitly acknowledged but creates a limitation:
Recommendation: Either:
3. Missing Health Check Version Information 📋 // packages/core/src/server/index.ts:250-252
expressApp.get("/health", (_req: Request, res: Response) => {
res.json({ status: "ok", name: config.name, version: config.version });
});For single-version apps, this works fine. But for multi-version apps, the shared health endpoint should return ALL available versions, not just one. According to the PR description, it should "return all available versions". Expected behavior: {
"status": "ok",
"name": "my-app",
"versions": [
{ "key": "v1", "version": "1.0.0", "route": "/v1/mcp" },
{ "key": "v2", "version": "2.0.0", "route": "/v2/mcp" }
]
}4. Incomplete Route Implementation 🔧 // Note: Express strips the mount path, so routes registered at serverRoute on the version appThis needs to be completed or removed. Medium Priority Issues5. Inconsistent OAuth Audience Handling // packages/core/src/server/index.ts:139-145
const protectedResourceUrl = new URL(serverRoute, config.config.oauth.protectedResource);
const oauthConfigWithAudience = {
...config.config.oauth,
audience: config.config.oauth.audience ?? protectedResourceUrl.href,
};For multi-version apps with different OAuth configs per version, the audience should include the version route ( Recommendation: Ensure version-specific OAuth audiences are properly constructed when OAuth configs differ per version. 6. Potential Memory Issue with JWKS Client Caching // packages/core/src/createApp.ts:804-806
const oauthConfigKey = normalizedVersionConfig.config?.oauth
? JSON.stringify(normalizedVersionConfig.config.oauth)
: "no-oauth";Using
Recommendation: Consider using a hash function (crypto.createHash) or implement proper cache key normalization. 7. Missing Validation for Conflicting Version Routes
Recommendation: Add validation in Low Priority / Code Quality8. Type Assertions Could Be Improved // packages/core/src/createApp.ts:414-434
const mergedConfig: GlobalConfig = {
oauth: deepMerge(...) as GlobalConfig["oauth"],
cors: deepMerge(...) as GlobalConfig["cors"],
// ...
};Multiple 9. Test Coverage Gaps
Recommendation: Add integration tests that start a server and make actual HTTP calls. 10. Example Code Quality // examples/minimal/src/index.ts:53
},
});
// =============================================================================
// V2: Enhanced greet tool (name + optional surname)
// =============================================================================The example is well-structured, but could be enhanced:
📝 Minor Issues
🎯 RecommendationsMust Fix Before Merge:
Should Fix:
Nice to Have:
🚀 Overall AssessmentThis is a well-designed and implemented feature that adds significant value to the framework. The core architecture is sound, backward compatibility is maintained, and the documentation is comprehensive. Score: 8/10 The main issues are:
Once the critical issues are addressed, this will be an excellent addition to the framework. Recommendation: Request Changes - The issues are fixable and not fundamental flaws. 💡 Additional Suggestions
Great work overall! Looking forward to seeing this feature land. 🎉 |
- Reformatted conditional checks in validateGlobalConfig for better clarity. - Updated mergeVersionConfig to enhance readability by using parentheses for nested expressions. - Ensured consistent handling of null and undefined values in configuration merging logic.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @examples/minimal/src/index.ts:
- Around line 131-133: The v2 handler currently overrides the client config with
protocol: "openai" (the config object containing protocol: "openai"), which
conflicts with docs that expect MCP for both v1 and v2; remove that protocol
override from the v2 config so v2 uses the default MCP protocol (or if OpenAI
protocol support is intended, update routing/docs to reflect POST /v2/openai and
adjust the v2 endpoint to deliberately set protocol: "openai" and handle any
OpenAI-specific request shapes). Ensure the change targets the config object
where protocol is set for v2.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/minimal/src/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
examples/minimal/src/index.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
examples/minimal/src/index.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
examples/minimal/src/index.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
examples/minimal/src/index.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
examples/minimal/src/index.ts
🧠 Learnings (3)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
examples/minimal/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Use Zod schemas with `defineTool` for input/output validation
Applied to files:
examples/minimal/src/index.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
examples/minimal/src/index.ts
🧬 Code graph analysis (1)
examples/minimal/src/index.ts (6)
packages/core/src/index.ts (3)
defineTool(181-181)createApp(181-181)ClientToolsFromCore(46-46)packages/core/src/createApp.ts (2)
defineTool(1217-1217)createApp(497-507)packages/ui-react-builder/src/define.ts (1)
defineReactUI(74-90)packages/ui-react-builder/src/index.ts (1)
defineReactUI(87-87)examples/minimal/src/ui/GreetingWidgetV1.tsx (1)
GreetingWidgetV1(12-99)examples/minimal/src/ui/GreetingWidgetV2.tsx (1)
GreetingWidgetV2(12-116)
🔇 Additional comments (4)
examples/minimal/src/index.ts (4)
1-15: LGTM! Excellent adherence to coding guidelines.The file structure, imports, and setup correctly follow the established patterns:
- Uses
defineToolanddefineReactUIfor type inference as per guidelines- Imports Zod for schema validation
- Properly imports versioned UI components
- Clear documentation of versioning feature
21-54: LGTM! V1 tool implementation is clean and follows best practices.The V1 greet tool correctly:
- Uses Zod schemas for input/output validation as per guidelines
- Leverages
defineToolfor proper type inference- Includes UI component binding with
defineReactUI- Returns structured output with the
_textfield for display purposes
60-97: LGTM! V2 tool demonstrates proper version evolution.The V2 implementation correctly:
- Extends V1 schema with optional
surnamefield (Zod 4 compatible)- Computes
fullNameappropriately, handling the optional surname case- Maintains consistent structure with V1 for clarity
- Uses a dedicated V2 UI component for version-specific behavior
154-168: LGTM! Type exports follow TypeScript best practices.The type exports correctly:
- Use
export typefor type-only exports as per coding guidelines- Create isolated type namespaces (V1 and V2) preventing cross-version type mixing
- Export both tool types (
AppTools*) and inferred schema types (GreetInput*,GreetOutput*)- Provide strong type safety for the UI components consuming these types
| config: { | ||
| protocol: "openai", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the actual code at the specified lines
cat -n examples/minimal/src/index.ts | sed -n '100,140p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1060
🏁 Script executed:
#!/bin/bash
# Search for documentation about protocol and versioning
rg -n -A 3 -B 3 'protocol' examples/minimal/src/index.ts | head -50Repository: AndurilCode/mcp-apps-kit
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Check if there are other examples in the repository
find examples -name '*.ts' -type f | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Look for any documentation about mixed protocol patterns
rg -i 'mixed.*protocol|protocol.*version' --type md -A 3Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Check the restaurant-finder example to see if it also uses mixed protocols
cat -n examples/restaurant-finder/src/index.ts | sed -n '1,50p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1705
🏁 Script executed:
#!/bin/bash
# Search for how createApp handles config merging and versioning
rg -n 'createApp' --type ts -A 10 | head -100Repository: AndurilCode/mcp-apps-kit
Length of output: 6742
🏁 Script executed:
#!/bin/bash
# Look for README or documentation in examples directory
find examples -name 'README*' -o -name '*.md' | xargs cat 2>/dev/nullRepository: AndurilCode/mcp-apps-kit
Length of output: 4978
🏁 Script executed:
#!/bin/bash
# Check core implementation to understand how config overrides work
rg -n 'versions|protocol' packages/core/src --type ts -B 2 -A 2 | head -80Repository: AndurilCode/mcp-apps-kit
Length of output: 5094
Remove the protocol: "openai" override from v2 or clarify the intended behavior.
The code overrides v2's protocol to "openai" (lines 131-133), but the README documentation and all provided examples show both v1 and v2 endpoints use MCP protocol (POST /v1/mcp and POST /v2/mcp). This inconsistency between the code configuration and documented endpoints should be resolved—either remove the protocol override to keep both versions on MCP, or update the documentation and routing if OpenAI protocol support is actually intended for v2.
🤖 Prompt for AI Agents
In @examples/minimal/src/index.ts around lines 131-133, The v2 handler currently
overrides the client config with protocol: "openai" (the config object
containing protocol: "openai"), which conflicts with docs that expect MCP for
both v1 and v2; remove that protocol override from the v2 config so v2 uses the
default MCP protocol (or if OpenAI protocol support is intended, update
routing/docs to reflect POST /v2/openai and adjust the v2 endpoint to
deliberately set protocol: "openai" and handle any OpenAI-specific request
shapes). Ensure the change targets the config object where protocol is set for
v2.
Code Review: API Versioning ImplementationSummaryThis PR implements API versioning support for mcp-apps-kit, allowing a single application to expose multiple API versions at different routes (e.g., ✅ Strengths1. Excellent Architecture & Design
2. Strong Type Safety
3. Robust Testing
4. Backward Compatibility
5. Documentation
|
…y handling - Updated deepMerge function to build the result object without null properties. - Replaced direct property deletion with Reflect.deleteProperty to comply with ESLint rules. - Enhanced comments for clarity on the merging process and runtime validation in versioning tests.
Code Review: API Versioning ImplementationThis PR introduces a comprehensive API versioning feature to mcp-apps-kit, enabling multiple API versions from a single application. Overall, this is a well-designed and thoroughly implemented feature with excellent test coverage. ✅ Strengths1. Excellent Architecture & Design
2. Outstanding Test Coverage
3. Documentation Quality
4. Backward Compatibility
🔧 Issues & Recommendations1. Critical: OAuth Lazy Initialization Race Condition
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Excellent design with clean separation of concerns |
| Code Quality | ⭐⭐⭐⭐ | High quality, some type safety improvements needed |
| Test Coverage | ⭐⭐⭐⭐⭐ | Outstanding - 967 lines of comprehensive tests |
| Documentation | ⭐⭐⭐⭐ | Very good, would benefit from migration guide |
| Security | ⭐⭐⭐⭐ | OAuth implementation solid, minor race condition |
| Performance | ⭐⭐⭐⭐⭐ | Smart resource sharing, minimal overhead |
| Backward Compat | ⭐⭐⭐⭐⭐ | Perfect - single-version apps unchanged |
✅ Approval Recommendation
LGTM with minor fixes recommended
This is a high-quality implementation that adds significant value to the framework. The critical OAuth race condition should be addressed before merge, but overall this is excellent work.
Pre-merge Checklist:
- Fix OAuth lazy initialization race condition (Critical)
- Improve type safety in mergeVersionConfig (Recommended)
- Add OAuth failure test cases (Recommended)
- Clarify serverRoute behavior in multi-version mode (Documentation)
Great job on this feature! 🎉
1 participant
Note
API versioning (core)
createAppnow supports multi-version apps viaversions, exposing per-version endpoints like/v1/mcp,/v2/mcpapp.getVersion(key)andapp.getVersions()for programmatic accessGET /healthDocs and examples
packages/core/README.mddocument versioning usage and patternsexamples/minimalconverted to a versioned app withv1/v2greet tools and version-specific React widgetsTests
Written by Cursor Bugbot for commit 0233cd6. Configure here.