-
Notifications
You must be signed in to change notification settings - Fork 127
Env port fix #35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Env port fix #35
Changes from 3 commits
ff507b5
2867edf
876acb9
ad4b08a
63622d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "Port": 8090, | ||
| "RequestTimeoutSeconds": 10, | ||
| "AutoStartServer": true, | ||
| "EnableInfoLogs": true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ import WebSocket from 'ws'; | |
| import { v4 as uuidv4 } from 'uuid'; | ||
| import { Logger } from '../utils/logger.js'; | ||
| import { McpUnityError, ErrorType } from '../utils/errors.js'; | ||
| import { execSync } from 'child_process'; | ||
| import { default as winreg } from 'winreg'; | ||
| import { promises as fs } from 'fs'; | ||
| import path from 'path'; | ||
|
|
||
| interface PendingRequest { | ||
| resolve: (value: any) => void; | ||
|
|
@@ -30,28 +30,14 @@ interface UnityResponse { | |
|
|
||
| export class McpUnity { | ||
| private logger: Logger; | ||
| private port: number; | ||
| private port: number | null = null; | ||
| private ws: WebSocket | null = null; | ||
| private pendingRequests: Map<string, PendingRequest> = new Map<string, PendingRequest>(); | ||
| private readonly REQUEST_TIMEOUT: number; | ||
| private requestTimeout = 10000; | ||
|
Comment on lines
+33
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Possible
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point, make it default to 8090 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flowchart TB
A["Set default port value in McpUnity class"]
B["Change port declaration to default to 8090"]
A --> B
@CoderGamester I'll implement that change for you. The port property will now default to 8090 instead of null, ensuring there's always a valid port value when attempting to connect to the WebSocket server. The changes will be made to ⏳ Preparing changes... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): ✏️ Server~/src/unity/mcpUnity.tsReplace the existing line Save the file and rebuild/restart the server. Verify that the WebSocket URL logged in A summary of the context that CodeRabbit is considering across the codebase:
|
||
| private retryDelay = 1000; | ||
|
|
||
| constructor(logger: Logger) { | ||
| this.logger = logger; | ||
|
|
||
| // Initialize port from environment variable or use default | ||
| const envRegistry = process.platform === 'win32' | ||
| ? this.getUnityPortFromWindowsRegistry() | ||
| : this.getUnityPortFromUnixRegistry(); | ||
|
|
||
| const envPort = process.env.UNITY_PORT || envRegistry; | ||
| this.port = envPort ? parseInt(envPort, 10) : 8090; | ||
| this.logger.info(`Using port: ${this.port} for Unity WebSocket connection`); | ||
|
|
||
| // Initialize timeout from environment variable (in seconds; it is the same as Cline) or use default (10 seconds) | ||
| const envTimeout = process.env.UNITY_REQUEST_TIMEOUT; | ||
| this.REQUEST_TIMEOUT = envTimeout ? parseInt(envTimeout, 10) * 1000 : 10000; | ||
| this.logger.info(`Using request timeout: ${this.REQUEST_TIMEOUT / 1000} seconds`); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -60,6 +46,9 @@ export class McpUnity { | |
| */ | ||
| public async start(clientName?: string): Promise<void> { | ||
| try { | ||
| this.logger.info('Attempting to read startup parameters...'); | ||
| this.parseAndSetConfig(); | ||
|
|
||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this.logger.info('Attempting to connect to Unity WebSocket...'); | ||
| await this.connect(clientName); // Pass client name to connect | ||
| this.logger.info('Successfully connected to Unity WebSocket'); | ||
|
|
@@ -77,6 +66,22 @@ export class McpUnity { | |
|
|
||
| return Promise.resolve(); | ||
| } | ||
|
|
||
| /** | ||
| * Reads our configuration file and sets parameters of the server based on them. | ||
| */ | ||
| private async parseAndSetConfig() { | ||
| const config = await this.readConfigFileAsJson(); | ||
|
|
||
| const configPort = config.Port; | ||
| this.port = configPort ? parseInt(configPort, 10) : 8090; | ||
| this.logger.info(`Using port: ${this.port} for Unity WebSocket connection`); | ||
|
|
||
| // Initialize timeout from environment variable (in seconds; it is the same as Cline) or use default (10 seconds) | ||
| const configTimeout = config.RequestTimeoutSeconds; | ||
| this.requestTimeout = configTimeout ? parseInt(configTimeout, 10) * 1000 : 10000; | ||
| this.logger.info(`Using request timeout: ${this.requestTimeout / 1000} seconds`); | ||
| } | ||
|
|
||
| /** | ||
| * Connect to the Unity WebSocket | ||
|
|
@@ -112,7 +117,7 @@ export class McpUnity { | |
| this.disconnect(); | ||
| reject(new McpUnityError(ErrorType.CONNECTION, 'Connection timeout')); | ||
| } | ||
| }, this.REQUEST_TIMEOUT); | ||
| }, this.requestTimeout); | ||
|
|
||
| this.ws.onopen = () => { | ||
| clearTimeout(connectionTimeout); | ||
|
|
@@ -249,12 +254,12 @@ export class McpUnity { | |
| // Create timeout for the request | ||
| const timeout = setTimeout(() => { | ||
| if (this.pendingRequests.has(requestId)) { | ||
| this.logger.error(`Request ${requestId} timed out after ${this.REQUEST_TIMEOUT}ms`); | ||
| this.logger.error(`Request ${requestId} timed out after ${this.requestTimeout}ms`); | ||
| this.pendingRequests.delete(requestId); | ||
| reject(new McpUnityError(ErrorType.TIMEOUT, 'Request timed out')); | ||
| } | ||
| this.reconnect(); | ||
| }, this.REQUEST_TIMEOUT); | ||
| }, this.requestTimeout); | ||
|
|
||
| // Store pending request | ||
| this.pendingRequests.set(requestId, { | ||
|
|
@@ -282,29 +287,21 @@ export class McpUnity { | |
| // Basic WebSocket connection check | ||
| return this.ws !== null && this.ws.readyState === WebSocket.OPEN; | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the UNITY_PORT value from the Windows registry (HKCU\Environment) | ||
| * @returns The port value as a string if found, otherwise an empty string | ||
| */ | ||
| private getUnityPortFromWindowsRegistry(): string { | ||
| const regKey = new winreg({hive: winreg.HKCU, key: '\\Environment'}); | ||
| let result = ''; | ||
| regKey.get('UNITY_PORT', (err: Error | null, item: any) => { | ||
| if (err) { | ||
| this.logger.error(`Error getting registry value: ${err.message}`); | ||
| } else { | ||
| result = item.value; | ||
| } | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves the UNITY_PORT value from Unix-like system environment variables | ||
| * @returns The port value as a string if found, otherwise an empty string | ||
| * Read the McpUnitySettings.json file and return its contents as a JSON object. | ||
| * @returns a JSON object with the contents of the McpUnitySettings.json file. | ||
| */ | ||
| private getUnityPortFromUnixRegistry(): string { | ||
| return execSync('printenv UNITY_PORT', { stdio: ['pipe', 'pipe', 'ignore'] }).toString().trim(); | ||
| private async readConfigFileAsJson(): Promise<any> { | ||
| const configPath = path.resolve(process.cwd(), 'build/McpUnitySettings.json'); | ||
| this.logger.debug(`Reading McpUnitySettings.json from ${configPath}`); | ||
| try { | ||
| const content = await fs.readFile(configPath, 'utf-8'); | ||
| const json = JSON.parse(content); | ||
| return json; | ||
| } catch (err) { | ||
| this.logger.debug(`McpUnitySettings.json not found or unreadable: ${err instanceof Error ? err.message : String(err)}`); | ||
| return {}; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use Path.Combine instead of string concatenation
While the implementation correctly saves settings to the server directory, string concatenation for file paths can cause issues on different platforms with varying path separators.
📝 Committable suggestion
🤖 Prompt for AI Agents