Conversation
WalkthroughThis change updates the conditional logic for registering the MCP controller and connecting to the stateless stream transport in the application lifecycle. Instead of checking for the presence of Changes
Sequence Diagram(s)sequenceDiagram
participant NodeRuntime
participant App
participant MCPControllerRegister
participant MCPProxyHook
participant EggContext
NodeRuntime->>App: Start (check Node.js version)
App->>App: If majorVersion >= 18
App->>MCPControllerRegister: Register controller and middleware
App->>MCPControllerRegister: Connect stateless stream transport (async)
MCPProxyHook->>EggContext: Create new context for each request
MCPProxyHook->>MCPControllerRegister: Call preProxy hooks with new context
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/mcp-proxy/index.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. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/controller/app.ts (1)
66-91: Version-based conditional replaces property checkThe code now uses Node.js version (>=18) as a condition rather than checking for
this.app.mcpProxy. This makes the requirement more explicit and direct, which is a good improvement.However, for better maintainability, consider extracting this magic number into a named constant.
const majorVersion = parseInt(process.versions.node.split('.')[0], 10); +const MCP_REQUIRED_NODE_VERSION = 18; // ... -if (majorVersion >= 18) { +if (majorVersion >= MCP_REQUIRED_NODE_VERSION) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/controller/app.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (20)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (16)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
plugin/controller/app.ts (2)
20-20: Good approach for extracting Node.js major versionThe code extracts the major version number from Node.js runtime version string and converts it to an integer. This is a simple and effective approach.
117-119: Consistent version check approachUsing the same Node.js version check here maintains consistency with the earlier condition. This is good for code maintainability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
change_tag.sh (4)
1-1: Use a portable shebang for BashRecommend switching to
#!/usr/bin/env bashto improve portability across environments where Bash may not reside at/bin/bash.Diff:
-#!/bin/bash +#!/usr/bin/env bash
3-4: Parameterize version and tag variablesHardcoding
TARGET_VERSIONandTAGrequires editing the script for each release. Consider accepting them as command-line arguments or environment variables, for example:TARGET_VERSION="${1:-3.53.0}" TAG="${2:-latest}"This allows invoking the script like:
./change_tag.sh 3.54.0 beta
21-29: Consistent language and logging for missing package nameError messages and comments mix English and Chinese. For clarity in a global codebase, pick one language. For example:
- echo "错误: $project_dir 中未找到有效的 package.json name 字段" + echo "Error: no valid 'name' field in $project_dir/package.json"
32-42: Track failures and proper exit statusCurrently, errors are logged but the script always exits with code 0. Consider:
- Enabling strict mode:
set -euo pipefail- Accumulating failures to exit non-zero if any tagging operation fails:
+# Enable strict mode at the top +set -euo pipefail + +FAILURE=0 ... if ! npm dist-tag add "$package_name@$TARGET_VERSION" "$TAG"; then echo "Failed to update tag in $project_dir" + FAILURE=1 fi ... done echo "All done!" +exit $FAILUREThis ensures downstream automation can detect failures correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
change_tag.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Runner-macos (16)
- GitHub Check: Runner-macos (20)
🔇 Additional comments (1)
change_tag.sh (1)
6-7: Directory list declaration looks goodThe
DIRSarray clearly enumerates the intended directories and requires no changes.
akitaSummer
force-pushed
the
fix/mcp-node-version
branch
from
eec2589 to
4325e39
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugin/mcp-proxy/index.ts (1)
62-72: Improved context isolation for pre-proxy hooks.This change properly isolates the context for each request by creating a new
EggContextinstance instead of potentially reusing a global context. This is a good practice that prevents potential data leakage between requests and aligns with the PR's focus on fixing MCP version check behavior.Consider exploring if there's a way to avoid the TypeScript ignore comments by properly typing the app or using a safer pattern for assigning the request and response classes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugin/mcp-proxy/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Runner-ubuntu (18)
- GitHub Check: Analyze (javascript)
- GitHub Check: Runner-ubuntu (20)
- GitHub Check: Runner-ubuntu (16)
- GitHub Check: Analyze (typescript)
- GitHub Check: Runner-macos (20)
- GitHub Check: Runner-ubuntu (22)
- GitHub Check: Runner-macos (18)
- GitHub Check: Runner-macos (16)
2 participants
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit