Conversation
WalkthroughThe changes introduce a new subclass, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPControllerRegister
participant InnerSSEServerTransport
participant CommonService
Client->>MCPControllerRegister: Sends JSON-RPC request (with id) via SSE
MCPControllerRegister->>InnerSSEServerTransport: Handles message, creates promise for id
MCPControllerRegister->>CommonService: Calls sayHello() in bar()
CommonService-->>MCPControllerRegister: Returns "hello world"
MCPControllerRegister->>InnerSSEServerTransport: Sends response with id
InnerSSEServerTransport->>MCPControllerRegister: Resolves promise for id
MCPControllerRegister-->>Client: Response delivered
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
plugin/controller/lib/impl/mcp/MCPControllerRegister.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eggache". (The package "eslint-plugin-eggache" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eggache" was referenced from the config file in ".eslintrc » eslint-config-egg/typescript » ./index.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
akitaSummer
force-pushed
the
fix/mcp-contextproto
branch
from
6e3f0e3 to
7438f00
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
347-365: Excellent fix for context initialization, but small improvement possibleThis change addresses a critical issue with context initialization in the MCP transport onmessage handler by creating a fresh HTTP context for each message. Instead of reusing the existing context, which led to an uninitialized ContextProto, the code now properly creates a new context with appropriate request headers.
However, there's one minor optimization:
You don't need to use
const self = thissince arrow functions inheritthisfrom their enclosing scope:- const self = this; transport.onmessage = async (...args: [ JSONRPCMessage ]) => { // 这里需要 new 一个新的 ctx，否则 ContextProto 会未被初始化 const socket = new Socket(); const req = new IncomingMessage(socket); const res = new ServerResponse(req); req.method = 'POST'; - req.url = self.mcpConfig.sseInitPath; + req.url = this.mcpConfig.sseInitPath; req.headers = { accept: 'application/json, text/event-stream', 'content-type': 'application/json', }; - const newCtx = self.app.createContext(req, res) as unknown as Context; + const newCtx = this.app.createContext(req, res) as unknown as Context; await ctx.app.ctxStorage.run(newCtx, async () => { await mw(newCtx, async () => { await messageFunc!(...args); }); }); };🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
349-349: Consider translating the comment to English for consistencyThe Chinese comment explains the purpose of the change well, but for international collaboration, consider translating it to English.
- // 这里需要 new 一个新的 ctx，否则 ContextProto 会未被初始化 + // Need to create a new ctx here, otherwise ContextProto would not be initialized🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (18)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
347-365: Improve context handling by removing unnecessarythisaliasing.The change correctly creates a new HTTP context for each incoming SSE message, ensuring proper context prototype initialization. However, the
selfvariable is unnecessary since arrow functions inheritthisfrom their enclosing scope.- const self = this; transport.onmessage = async (...args: [ JSONRPCMessage ]) => { // 这里需要 new 一个新的 ctx，否则 ContextProto 会未被初始化 const socket = new Socket(); const req = new IncomingMessage(socket); const res = new ServerResponse(req); req.method = 'POST'; - req.url = self.mcpConfig.sseInitPath; + req.url = this.mcpConfig.sseInitPath; req.headers = { accept: 'application/json, text/event-stream', 'content-type': 'application/json', }; - const newCtx = self.app.createContext(req, res) as unknown as Context; + const newCtx = this.app.createContext(req, res) as unknown as Context;🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(3 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/services/common.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/controller/test/fixtures/apps/mcp-app/app/services/common.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (18)
🔇 Additional comments (4)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (3)
17-17: Good addition of CommonService import.This import properly sets up the dependency needed for the new service integration.
36-37: LGTM! Proper dependency injection.The CommonService is properly injected using the @Inject() decorator, following the dependency injection pattern used throughout the codebase.
57-57: Good service integration in the MCP tool handler.The service call to
commonService.sayHello()is properly awaited and integrates well with the existing flow of the bar method. The call is made before returning the response, maintaining the original method behavior.plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
349-359: Great improvement to context management for SSE messages.This change properly creates a new context for each SSE message, including a new Socket, IncomingMessage, and ServerResponse. This ensures that each message has a fresh context with proper initialization of the ContextProto, addressing potential issues that could occur when reusing the same context across multiple messages.
🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
|
LG |
akitaSummer
force-pushed
the
fix/mcp-contextproto
branch
from
7438f00 to
662854b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (1)
67-67: Consider handling the return value from sayHello.The
sayHello()method returns a string, but the return value is not being used or logged.- await this.commonService.sayHello(); + const message = await this.commonService.sayHello(); + this.logger.debug(`Service said: ${message}`);plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
347-365: Good fix: Creating a fresh context ensures proper ContextProto initialization.This change addresses an important issue by creating a new HTTP context for each SSE message, ensuring that context prototypes like
CommonServiceare properly initialized.Two minor improvements can be made:
- Remove unnecessary
selfalias since arrow functions inheritthis- Add more descriptive comments explaining why this fixes the issue
- const self = this; transport.onmessage = async (...args: [ JSONRPCMessage ]) => { - // 这里需要 new 一个新的 ctx，否则 ContextProto 会未被初始化 + // Create a new context for each SSE message to ensure proper ContextProto initialization + // Without this, services decorated with @ContextProto() won't be properly initialized const socket = new Socket(); const req = new IncomingMessage(socket); const res = new ServerResponse(req); req.method = 'POST'; - req.url = self.mcpConfig.sseInitPath; + req.url = this.mcpConfig.sseInitPath; req.headers = { accept: 'application/json, text/event-stream', 'content-type': 'application/json', }; - const newCtx = self.app.createContext(req, res) as unknown as Context; + const newCtx = this.app.createContext(req, res) as unknown as Context; await ctx.app.ctxStorage.run(newCtx, async () => { await mw(newCtx, async () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
349-349: Remove unnecessarythisaliasing.Arrow functions inherit
thisfrom their enclosing scope, making theselfalias unnecessary.- const self = this;🧰 Tools
🪛 Biome (1.9.4)
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(1 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(3 hunks)plugin/controller/test/mcp/mcp.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (3)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/AppController.ts (1)
MCPController(25-68)core/controller-decorator/src/decorator/mcp/MCPController.ts (1)
MCPController(7-33)core/controller-decorator/test/fixtures/MCPController.ts (1)
MCPController(19-69)
🪛 Biome (1.9.4)
plugin/controller/test/mcp/mcp.test.ts
[error] 96-96: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 349-349: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (2)
plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts (2)
29-38: LGTM: Well-structured service with proper context decoration.The
CommonServiceclass is properly decorated with@ContextProto()and includes appropriate dependency injection for logging.
46-47: LGTM: Clean service injection in controller.The
CommonServiceis properly injected into the controller.
akitaSummer
force-pushed
the
fix/mcp-contextproto
branch
from
662854b to
1fe67b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
356-395: 🛠️ Refactor suggestionRace condition: promise stored after response may cause dead-lock
onmessagestores the resolver aftermessageFuncis invoked.
IfmessageFunc(i.e. MCP server) produces and sends the response
synchronously,InnerSSEServerTransport.send()will execute before the
resolver is registered, leading to the “missing resolver” branch and
the awaiting request never being resolved.Store the resolver before calling
messageFunc:- messageFunc!(...args); - if (isJSONRPCRequest(args[0])) { - const map = this.sseTransportsRequestMap.get(transport)!; - const wait = new Promise<null>(resolve => { - if ('id' in args[0]) { - map[args[0].id] = resolve; - } - }); - await wait; - } + if (isJSONRPCRequest(args[0])) { + const map = this.sseTransportsRequestMap.get(transport)!; + const wait = new Promise<null>(resolve => { + if ('id' in args[0]) { + map.set(args[0].id, resolve); + } + }); + messageFunc!(...args); + await wait; + } else { + messageFunc!(...args); + }This guarantees the resolver is present when the response is emitted.
🧰 Tools
🪛 Biome (1.9.4)
[error] 368-368: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
🧹 Nitpick comments (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
366-368: Remove needlessconst self = thisaliasInside arrow functions
thisis lexically scoped, so aliasing it adds noise and
triggers the BiomenoUselessThisAliaslint error. Replaceselfwith
this.- const self = this; - self.sseTransportsRequestMap.set(transport, {}); + this.sseTransportsRequestMap.set(transport, new Map());Do the same for the earlier alias at line 322.
🧰 Tools
🪛 Biome (1.9.4)
[error] 368-368: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
67-68: Minor: maketransportsMap-based for symmetry and faster lookupsYou turned
transportsintoRecord<string, InnerSSEServerTransport>; consider
usingMap<string, InnerSSEServerTransport>just like
sseTransportsRequestMap, benefiting from constant-time operations and avoiding
accidental prototype pollution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(5 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 322-322: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 368-368: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
akitaSummer
force-pushed
the
fix/mcp-contextproto
branch
from
1fe67b7 to
8574bd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
46-57:⚠️ Potential issueAdd safeguards against missing resolvers and cleanup requests properly
The
InnerSSEServerTransport.send()method has a potential issue where it assumes a resolver always exists for a message ID. If a client sends a notification (with no stored resolver) or if there's a race condition where the response arrives beforeonmessagestores the promise,resolve(null)will throw an error.Also, there's no cleanup mechanism for the transport entry in
sseTransportsRequestMapon transport close, which could lead to memory leaks.async send(message: JSONRPCMessage) { await super.send(message); // eslint-disable-next-line @typescript-eslint/no-use-before-define const map = MCPControllerRegister.instance?.sseTransportsRequestMap.get(this); if (map && 'id' in message) { - const resolve = map[message.id]; - resolve(null); - delete map[message.id]; + const resolve = map[message.id]; + if (resolve) { + resolve(null); + delete map[message.id]; + } } }
75-79: 🛠️ Refactor suggestionReplace Record with Map for improved type safety and performance
Using a plain object (
Record) forces keys to be strings and makes lookups O(n) when the key is a number. AMapwould preserve the original ID type (string | number) and improve performance.sseTransportsRequestMap = new Map< InnerSSEServerTransport, - Record<string, (value: PromiseLike<null> | null) => void - >>(); + Map<string | number, (value: PromiseLike<null> | null) => void> +>();This would also require changes in other places:
// In sseCtxStorageRun: - self.sseTransportsRequestMap.set(transport, {}); + self.sseTransportsRequestMap.set(transport, new Map()); // In onmessage handler: - map[args[0].id] = resolve; + map.set(args[0].id, resolve); // In InnerSSEServerTransport.send: - const resolve = map[message.id]; - resolve(null); - delete map[message.id]; + const resolve = map.get(message.id); + if (resolve) { + resolve(null); + map.delete(message.id); + }
🧹 Nitpick comments (2)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (2)
334-334: Use InnerSSEServerTransport instead of base classGood change to use the extended class with enhanced capabilities.
Consider addressing the static analysis warning about unnecessary this aliasing with 'self' by using arrow functions that inherit 'this'.
🧰 Tools
🪛 Biome (1.9.4)
[error] 334-334: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
378-380: Initialize request map and update context handling logicInitializing the request map for each transport is a good practice. However, there's an unnecessary aliasing of 'this' to 'self'.
- const self = this; - self.sseTransportsRequestMap.set(transport, {}); + this.sseTransportsRequestMap.set(transport, {}); // Or with the Map refactoring: + this.sseTransportsRequestMap.set(transport, new Map());🧰 Tools
🪛 Biome (1.9.4)
[error] 380-380: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(7 hunks)plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts(3 hunks)plugin/mcp-proxy/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/controller/test/fixtures/apps/mcp-app/app/controller/McpController.ts
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 334-334: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 380-380: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (18)
🔇 Additional comments (4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
67-67: Using specific type for transports record is goodChanging the type to use
InnerSSEServerTransportinstead of the base class ensures type safety in the codebase.
159-165: Good pattern for handling response closureCreating a Promise that resolves on 'close' and awaiting it ensures that the server waits for the response to be fully sent before proceeding. This prevents potential race conditions and resource cleanup issues.
266-272: Good implementation of awaiting response closureSimilar to the previous comment, this ensures proper lifecycle management for streaming responses.
plugin/mcp-proxy/index.ts (1)
158-165: Good pattern for waiting on response close eventThis change ensures the handler waits for the response to fully close before continuing. This aligns with the similar changes in MCPControllerRegister.ts and provides consistent behavior across the codebase for handling response lifecycles.
The implementation correctly:
- Creates a Promise that resolves on the 'close' event
- Awaits the Promise after handling the request
- Ensures resources are properly released
This pattern helps prevent potential race conditions and resource cleanup issues.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (5)
84-90: 🛠️ Refactor suggestionReplace Record with Map for better type safety and performance.
Using Record for storing promises by message ID is inefficient and error-prone. This was mentioned in previous review comments but not addressed.
// eslint-disable-next-line no-spaced-func sseTransportsRequestMap = new Map< InnerSSEServerTransport, - Record<string, { - resolve: (value: PromiseLike<null> | null) => void, - reject: (reason?: any) => void, - } + Map<string | number, { + resolve: (value: PromiseLike<null> | null) => void, + reject: (reason?: any) => void, + }> >>();This change allows using both string and number IDs (as per JSON-RPC spec) and provides built-in methods for safe property access and deletion.
345-345: 🛠️ Refactor suggestionUpdated transport instantiation to use the new InnerSSEServerTransport class.
This correctly instantiates the custom transport class, but there's a type mismatch issue.
There's a type mismatch between the parameter type in
sseCtxStorageRun(which expectsSSEServerTransport) and the actual type passed (InnerSSEServerTransport). This was mentioned in previous reviews but not fixed.Fix the parameter type in
sseCtxStorageRunto avoid TypeScript errors:- sseCtxStorageRun(ctx: Context, transport: SSEServerTransport) { + sseCtxStorageRun(ctx: Context, transport: InnerSSEServerTransport) {🧰 Tools
🪛 Biome (1.9.4)
[error] 345-345: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
392-393: 🛠️ Refactor suggestionInitialize resolver map but use Map instead of plain object.
When initializing the request map for the transport, use a Map instead of an empty object:
- self.sseTransportsRequestMap.set(transport, {}); + self.sseTransportsRequestMap.set(transport, new Map()); transport.onmessage = async (...args: [ JSONRPCMessage ]) => {This enables proper use of Map methods like
get,set, anddeletelater in the code.
394-417: 🛠️ Refactor suggestionImproved message handling with context creation, but lacks timeout management.
The new implementation creates a proper context for each message and adds promise-based synchronization, which is good. However, there are issues:
- No timeout for the promise resolution could lead to hanging connections if responses never arrive
- Using Record instead of Map for storing resolvers is inefficient
- No clean-up if errors occur during processing
Suggested improvements:
transport.onmessage = async (...args: [ JSONRPCMessage ]) => { // 这里需要 new 一个新的 ctx，否则 ContextProto 会未被初始化 const socket = new Socket(); const req = new IncomingMessage(socket); const res = new ServerResponse(req); req.method = 'POST'; req.url = self.mcpConfig.sseInitPath; req.headers = { accept: 'application/json, text/event-stream', 'content-type': 'application/json', }; const newCtx = self.app.createContext(req, res) as unknown as Context; await ctx.app.ctxStorage.run(newCtx, async () => { await mw(newCtx, async () => { messageFunc!(...args); if (isJSONRPCRequest(args[0])) { const map = self.sseTransportsRequestMap.get(transport)!; + let timeoutId: NodeJS.Timeout | null = null; const wait = new Promise<null>((resolve, reject) => { if ('id' in args[0]) { - map[args[0].id] = { resolve, reject }; + // Add timeout to prevent hanging connections + timeoutId = setTimeout(() => { + map.delete(args[0].id); + reject(new Error('Response timeout')); + }, 30000); // 30s timeout + map.set(args[0].id, { + resolve: (value) => { + if (timeoutId) clearTimeout(timeoutId); + resolve(value); + }, + reject: (reason) => { + if (timeoutId) clearTimeout(timeoutId); + reject(reason); + } + }); } }); - await wait; + try { + await wait; + } catch (error) { + self.app.logger.error('Error waiting for response:', error); + } } }); }); };
46-65: 🛠️ Refactor suggestionThe InnerSSEServerTransport implementation provides promise-based synchronization for responses, but has potential issues with error handling.
The implementation catches errors when sending messages and resolves/rejects promises accordingly, which is good. However, there are some concerns:
- The error handling is inconsistent - if there's no resolver for a message ID, the code silently continues.
- Consider using WeakMap or Map instead of plain objects for better memory management.
class InnerSSEServerTransport extends SSEServerTransport { async send(message: JSONRPCMessage) { let res: null | Error = null; try { await super.send(message); } catch (e) { res = e as Error; } finally { // eslint-disable-next-line @typescript-eslint/no-use-before-define const map = MCPControllerRegister.instance?.sseTransportsRequestMap.get(this); if (map && 'id' in message) { - const { resolve, reject } = map[message.id] ?? {}; - if (resolve) { + const resolver = map.get?.(message.id); + if (resolver) { + const { resolve, reject } = resolver; res ? reject(res) : resolve(res); - delete map[message.id]; + map.delete(message.id); } } } } }
🧹 Nitpick comments (3)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (3)
388-390: Improved error logging but unnecessary this aliasing.Arrow functions inherit
thisfrom their enclosing scope, making theselfalias unnecessary.transport.onerror = error => { - self.app.logger.error('session %s error %o', transport.sessionId, error); + this.app.logger.error('session %s error %o', transport.sessionId, error); };
170-177: Added response waiting mechanism, but lacks timeout protection.Good addition of waiting for the response to close before proceeding. This addresses the issue of requests potentially being terminated too early.
Consider adding a timeout to prevent hanging in case the connection never closes:
const wait = new Promise<null>(resolve => { + const timeoutId = setTimeout(() => { + resolve(null); + ctx.app.logger.warn('Request close event timed out'); + }, 60000); // 60s timeout ctx.res.once('close', () => { + clearTimeout(timeoutId); resolve(null); }); }); await self.statelessTransport.handleRequest(ctx.req, ctx.res); await wait;
277-284: Same timeout concern for stream transport request handling.Apply the same timeout mechanism here as suggested for the previous similar code:
const wait = new Promise<null>(resolve => { + const timeoutId = setTimeout(() => { + resolve(null); + ctx.app.logger.warn('Stream request close event timed out'); + }, 60000); // 60s timeout ctx.res.once('close', () => { + clearTimeout(timeoutId); resolve(null); }); }); await transport.handleRequest(ctx.req, ctx.res, body); await wait;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts
[error] 345-345: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
[error] 382-382: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (1)
plugin/controller/lib/impl/mcp/MCPControllerRegister.ts (1)
75-75: Updated transport type to match the new implementation.This change correctly updates the type of transports to use the new InnerSSEServerTransport class.
3 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Bug Fixes