Open
Conversation
…nx routing
Nginx rewrites {alias}.domain.com/* → /mcp/{alias}/* but auth/discovery
routes lived under /auth/{alias}/* and /.well-known/*, causing 404s for
all OAuth flows through subdomains.
Add McpAuthController under /mcp/:alias with matching OAuth + RFC 9728/8414
discovery routes, subdomain-aware metadata URLs, and configurable login
form action. Existing base-domain routes unchanged.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds subdomain-aware OAuth discovery and endpoints: a URL builder, subdomain-specific discovery metadata methods, a new McpAuthController (authorize/login/token/register) mounted at Changes
Sequence DiagramsequenceDiagram
participant Client
participant McpAuthCtrl as McpAuthController
participant DiscoverySvc as DiscoveryService
participant OAuthSvc as OAuthService
participant AuthSvc as AuthService
rect rgba(173,216,230,0.5)
Note over Client,McpAuthCtrl: Discovery metadata request
Client->>McpAuthCtrl: GET /mcp/:alias/.well-known/oauth-protected-resource
McpAuthCtrl->>DiscoverySvc: getSubdomainResourceMetadata(alias)
DiscoverySvc-->>McpAuthCtrl: resource metadata
McpAuthCtrl-->>Client: JSON metadata
end
rect rgba(144,238,144,0.5)
Note over Client,OAuthSvc: Authorization & login
Client->>McpAuthCtrl: GET /mcp/:alias/authorize?client_id=...
McpAuthCtrl-->>Client: HTML login form (generateLoginPage)
Client->>McpAuthCtrl: POST /mcp/:alias/login (credentials)
McpAuthCtrl->>OAuthSvc: handleLogin(alias, credentials)
OAuthSvc->>AuthSvc: authenticate user
AuthSvc-->>OAuthSvc: auth code
OAuthSvc-->>McpAuthCtrl: auth code
McpAuthCtrl-->>Client: Redirect with code & state
end
rect rgba(255,182,193,0.5)
Note over Client,OAuthSvc: Token exchange
Client->>McpAuthCtrl: POST /mcp/:alias/token (code or refresh_token)
McpAuthCtrl->>OAuthSvc: exchangeAuthorizationCode / refreshToken
OAuthSvc-->>McpAuthCtrl: access token / refresh token response
McpAuthCtrl-->>Client: token JSON
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/auth/services/discovery.service.ts (1)
3-3: Use path alias for internal imports.As per coding guidelines, internal imports should use the
@/*path alias.Suggested fix
-import { buildSubdomainUrl } from '../../common/utils/url.utils'; +import { buildSubdomainUrl } from '@/common/utils/url.utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/services/discovery.service.ts` at line 3, Replace the relative import of buildSubdomainUrl with the project path-alias form: update the import in discovery.service.ts that currently references '../../common/utils/url.utils' so it imports buildSubdomainUrl from '@/common/utils/url.utils' (i.e., use the `@/` path alias for internal imports).src/mcp/mcp-auth.controller.ts (1)
17-24: Use path aliases for internal imports.As per coding guidelines, internal imports should use the
@/*path alias pointing tosrc/*.Suggested fix
-import { OAuthService } from '../auth/services/oauth.service'; -import { DiscoveryService } from '../auth/services/discovery.service'; -import { AuthorizeQueryDto } from '../auth/dto/authorize.dto'; -import { TokenRequestDto } from '../auth/dto/token-request.dto'; -import { TokenResponseDto } from '../auth/dto/token-response.dto'; -import { generateLoginPage } from '../auth/templates/login-page.template'; -import { PartnerMetaService } from '../alias/partner-meta.service'; -import { AliasService } from '../alias/alias.service'; +import { OAuthService } from '@/auth/services/oauth.service'; +import { DiscoveryService } from '@/auth/services/discovery.service'; +import { AuthorizeQueryDto } from '@/auth/dto/authorize.dto'; +import { TokenRequestDto } from '@/auth/dto/token-request.dto'; +import { TokenResponseDto } from '@/auth/dto/token-response.dto'; +import { generateLoginPage } from '@/auth/templates/login-page.template'; +import { PartnerMetaService } from '@/alias/partner-meta.service'; +import { AliasService } from '@/alias/alias.service';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp-auth.controller.ts` around lines 17 - 24, Replace the relative internal imports in mcp-auth.controller.ts with the project path alias (`@/`*) so internal modules use '@/...' instead of '../...'; update imports that reference OAuthService, DiscoveryService, AuthorizeQueryDto, TokenRequestDto, TokenResponseDto, generateLoginPage, PartnerMetaService, and AliasService to their corresponding '@/' paths (e.g. '@/auth/services/oauth.service') so they resolve via the configured path alias.src/mcp/mcp.controller.ts (1)
20-20: Use path alias for internal imports.As per coding guidelines, internal imports should use the
@/*path alias pointing tosrc/*.Suggested fix
-import { buildSubdomainUrl } from '../common/utils/url.utils'; +import { buildSubdomainUrl } from '@/common/utils/url.utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` at line 20, Replace the relative internal import with the project path-alias: change the import that references buildSubdomainUrl (currently "import { buildSubdomainUrl } from '../common/utils/url.utils';") to use the "@/..." alias pointing at the same module under src (so the symbol buildSubdomainUrl is imported from the aliased path). Ensure the new import follows the same named import syntax and that any linter/TS config recognizes the alias so mcp.controller.ts continues to compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp/mcp-auth.controller.ts`:
- Around line 147-158: The tokenOptions method sets Access-Control-Allow-Origin
to '*' while also setting Access-Control-Allow-Credentials to 'true', which
violates CORS; update tokenOptions to either remove the
Access-Control-Allow-Credentials header (if credentialed requests are not
needed) or replace the wildcard origin with a concrete origin value (e.g., read
from config/env and set that value in the Access-Control-Allow-Origin header) so
that credentials are only allowed with a specific origin; ensure the chosen
approach updates the headers set in tokenOptions(`@Res`() res: Response) and keep
the rest of the preflight headers unchanged.
- Around line 77-78: The form action currently passed to generateLoginPage is an
absolute '/login' which breaks subdomain-scoped routes; update the call in
mcp-auth.controller.ts (generateLoginPage(alias, query, meta ?? undefined,
undefined, '/login')) to use a relative path 'login' so the browser posts to the
current /mcp/:alias/authorize context, or construct the proper absolute path
using the alias (e.g., `/mcp/${alias}/login`) so the form submits to the correct
/mcp/:alias/login endpoint.
- Around line 136-142: The generateLoginPage call in the error path currently
passes a hardcoded '/login' as the form action; change that to use the same
action value used for the normal (non-error) render so the re-rendered form
posts to the correct endpoint—replace the '/login' argument in the
generateLoginPage(...) invocation with the same variable/constant used in the
successful render (e.g., the login action variable or oauthParams.action /
LOGIN_POST_PATH used elsewhere) so both paths use the identical form action.
---
Nitpick comments:
In `@src/auth/services/discovery.service.ts`:
- Line 3: Replace the relative import of buildSubdomainUrl with the project
path-alias form: update the import in discovery.service.ts that currently
references '../../common/utils/url.utils' so it imports buildSubdomainUrl from
'@/common/utils/url.utils' (i.e., use the `@/` path alias for internal imports).
In `@src/mcp/mcp-auth.controller.ts`:
- Around line 17-24: Replace the relative internal imports in
mcp-auth.controller.ts with the project path alias (`@/`*) so internal modules use
'@/...' instead of '../...'; update imports that reference OAuthService,
DiscoveryService, AuthorizeQueryDto, TokenRequestDto, TokenResponseDto,
generateLoginPage, PartnerMetaService, and AliasService to their corresponding
'@/' paths (e.g. '@/auth/services/oauth.service') so they resolve via the
configured path alias.
In `@src/mcp/mcp.controller.ts`:
- Line 20: Replace the relative internal import with the project path-alias:
change the import that references buildSubdomainUrl (currently "import {
buildSubdomainUrl } from '../common/utils/url.utils';") to use the "@/..." alias
pointing at the same module under src (so the symbol buildSubdomainUrl is
imported from the aliased path). Ensure the new import follows the same named
import syntax and that any linter/TS config recognizes the alias so
mcp.controller.ts continues to compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4996e5ed-b611-4d86-bded-067b9d549c49
📒 Files selected for processing (6)
src/auth/services/discovery.service.tssrc/auth/templates/login-page.template.tssrc/common/utils/url.utils.tssrc/mcp/mcp-auth.controller.tssrc/mcp/mcp.controller.tssrc/mcp/mcp.module.ts
Comment on lines
+77
to
+78
| const html = generateLoginPage(alias, query, meta ?? undefined, undefined, '/login'); | ||
| res.status(HttpStatus.OK).contentType('text/html').send(html); |
There was a problem hiding this comment.
Incorrect form action URL will break the login flow.
The loginActionUrl is set to '/login' which is an absolute path pointing to the root /login endpoint, not the subdomain-scoped /mcp/:alias/login. The form submission will fail with a 404.
Proposed fix
Use a relative path 'login' (without leading slash) so the browser resolves it relative to the current /mcp/:alias/authorize path:
- const html = generateLoginPage(alias, query, meta ?? undefined, undefined, '/login');
+ const html = generateLoginPage(alias, query, meta ?? undefined, undefined, 'login');Alternatively, construct the absolute path:
- const html = generateLoginPage(alias, query, meta ?? undefined, undefined, '/login');
+ const html = generateLoginPage(alias, query, meta ?? undefined, undefined, `/mcp/${alias}/login`);📝 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.
Suggested change
| const html = generateLoginPage(alias, query, meta ?? undefined, undefined, '/login'); | |
| res.status(HttpStatus.OK).contentType('text/html').send(html); | |
| const html = generateLoginPage(alias, query, meta ?? undefined, undefined, 'login'); | |
| res.status(HttpStatus.OK).contentType('text/html').send(html); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp-auth.controller.ts` around lines 77 - 78, The form action
currently passed to generateLoginPage is an absolute '/login' which breaks
subdomain-scoped routes; update the call in mcp-auth.controller.ts
(generateLoginPage(alias, query, meta ?? undefined, undefined, '/login')) to use
a relative path 'login' so the browser posts to the current
/mcp/:alias/authorize context, or construct the proper absolute path using the
alias (e.g., `/mcp/${alias}/login`) so the form submits to the correct
/mcp/:alias/login endpoint.
Comment on lines
+136
to
+142
| const html = generateLoginPage( | ||
| alias, | ||
| oauthParams, | ||
| meta ?? undefined, | ||
| err.message || 'Login failed', | ||
| '/login', | ||
| ); |
There was a problem hiding this comment.
Same incorrect form action URL in the error path.
Apply the same fix here to ensure the re-rendered login form posts to the correct endpoint.
Proposed fix
const html = generateLoginPage(
alias,
oauthParams,
meta ?? undefined,
err.message || 'Login failed',
- '/login',
+ 'login',
);📝 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.
Suggested change
| const html = generateLoginPage( | |
| alias, | |
| oauthParams, | |
| meta ?? undefined, | |
| err.message || 'Login failed', | |
| '/login', | |
| ); | |
| const html = generateLoginPage( | |
| alias, | |
| oauthParams, | |
| meta ?? undefined, | |
| err.message || 'Login failed', | |
| 'login', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp-auth.controller.ts` around lines 136 - 142, The generateLoginPage
call in the error path currently passes a hardcoded '/login' as the form action;
change that to use the same action value used for the normal (non-error) render
so the re-rendered form posts to the correct endpoint—replace the '/login'
argument in the generateLoginPage(...) invocation with the same
variable/constant used in the successful render (e.g., the login action variable
or oauthParams.action / LOGIN_POST_PATH used elsewhere) so both paths use the
identical form action.
Comment on lines
+147
to
+158
| @Options('token') | ||
| tokenOptions(@Res() res: Response): void { | ||
| res.header('Access-Control-Allow-Origin', '*'); | ||
| res.header('Access-Control-Allow-Methods', 'POST, OPTIONS'); | ||
| res.header( | ||
| 'Access-Control-Allow-Headers', | ||
| 'Content-Type, Authorization, Accept, x-admin-api-key, x-project-api-key, mcp-protocol-version', | ||
| ); | ||
| res.header('Access-Control-Allow-Credentials', 'true'); | ||
| res.header('Access-Control-Max-Age', '86400'); | ||
| res.status(HttpStatus.NO_CONTENT).send(); | ||
| } |
There was a problem hiding this comment.
Invalid CORS configuration: wildcard origin with credentials.
Setting Access-Control-Allow-Origin: '*' together with Access-Control-Allow-Credentials: 'true' violates the CORS specification. Browsers will reject this configuration and block the preflight response.
Either:
- Remove
Access-Control-Allow-Credentialsif you don't need credentialed requests, or - Use a specific origin instead of
'*'when credentials are required
Proposed fix (option 1: remove credentials)
`@Options`('token')
tokenOptions(`@Res`() res: Response): void {
res.header('Access-Control-Allow-Origin', '*');
res.header('Access-Control-Allow-Methods', 'POST, OPTIONS');
res.header(
'Access-Control-Allow-Headers',
'Content-Type, Authorization, Accept, x-admin-api-key, x-project-api-key, mcp-protocol-version',
);
- res.header('Access-Control-Allow-Credentials', 'true');
res.header('Access-Control-Max-Age', '86400');
res.status(HttpStatus.NO_CONTENT).send();
}📝 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.
Suggested change
| @Options('token') | |
| tokenOptions(@Res() res: Response): void { | |
| res.header('Access-Control-Allow-Origin', '*'); | |
| res.header('Access-Control-Allow-Methods', 'POST, OPTIONS'); | |
| res.header( | |
| 'Access-Control-Allow-Headers', | |
| 'Content-Type, Authorization, Accept, x-admin-api-key, x-project-api-key, mcp-protocol-version', | |
| ); | |
| res.header('Access-Control-Allow-Credentials', 'true'); | |
| res.header('Access-Control-Max-Age', '86400'); | |
| res.status(HttpStatus.NO_CONTENT).send(); | |
| } | |
| `@Options`('token') | |
| tokenOptions(`@Res`() res: Response): void { | |
| res.header('Access-Control-Allow-Origin', '*'); | |
| res.header('Access-Control-Allow-Methods', 'POST, OPTIONS'); | |
| res.header( | |
| 'Access-Control-Allow-Headers', | |
| 'Content-Type, Authorization, Accept, x-admin-api-key, x-project-api-key, mcp-protocol-version', | |
| ); | |
| res.header('Access-Control-Max-Age', '86400'); | |
| res.status(HttpStatus.NO_CONTENT).send(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp-auth.controller.ts` around lines 147 - 158, The tokenOptions
method sets Access-Control-Allow-Origin to '*' while also setting
Access-Control-Allow-Credentials to 'true', which violates CORS; update
tokenOptions to either remove the Access-Control-Allow-Credentials header (if
credentialed requests are not needed) or replace the wildcard origin with a
concrete origin value (e.g., read from config/env and set that value in the
Access-Control-Allow-Origin header) so that credentials are only allowed with a
specific origin; ensure the chosen approach updates the headers set in
tokenOptions(`@Res`() res: Response) and keep the rest of the preflight headers
unchanged.
Replace static client_id stub with real DCR backed by Redis storage. Clients are registered per-alias with scrypt-hashed secrets, exact redirect_uri matching, and configurable per-tenant limits. - POST /auth/:alias/register returns 201 with client_id + optional secret - Token endpoint validates registered clients (Basic Auth + client_secret_post) - Backward compatible: REQUIRE_CLIENT_REGISTRATION defaults to false - Discovery metadata cleaned up to only advertise implemented endpoints
…91)" This reverts commit 1c27dcb.
…ce_simple Brightness now accepts 0-100 (percent) and converts to 0-1000 server-side. All set_* actions validate ranges with clear error messages (BadRequestException): - set_brightness: 0-100% - set_kelvin: 0-65000 - set_temperature: 15-30°C - set_mode: 0-4 (AUTO/COOL/DRY/HEAT/FAN)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/services/tool-executor.service.ts`:
- Around line 131-142: requireValue currently only checks for null/undefined and
lets non-numeric inputs (''/false/NaN) through, causing downstream
Math.round/validateRange to operate on invalid numbers; update
requireValue(value: number | null | undefined, action: string) to perform
runtime numeric validation by verifying typeof value === 'number' and
Number.isFinite(value) (reject NaN/Infinity and non-number primitives) and throw
BadRequestException with a clear message referencing the action when validation
fails; keep validateRange as-is but rely on the strengthened requireValue to
ensure callers (e.g., places that call Math.round or validateRange) never
receive invalid numbers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5b31d19-901b-4b77-ac6a-8b1cd802c895
📒 Files selected for processing (2)
src/tools/definitions/control-device-simple.tool.tssrc/tools/services/tool-executor.service.ts
✅ Files skipped from review due to trivial changes (1)
- src/tools/definitions/control-device-simple.tool.ts
Comment on lines
+131
to
+142
| private requireValue(value: number | null | undefined, action: string): number { | ||
| if (value == null) { | ||
| throw new BadRequestException(`value is required for ${action} action`); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| private validateRange(value: number, min: number, max: number, label: string): void { | ||
| if (value < min || value > max) { | ||
| throw new BadRequestException(`${label} must be ${min}-${max}. Got: ${value}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the current @modelcontextprotocol/sdk Node server validate incoming tool-call arguments against a tool's inputSchema before invoking the handler, or must servers perform their own runtime type validation?
💡 Result:
No, the @modelcontextprotocol/sdk Node server does not automatically validate incoming tool-call arguments against a tool's inputSchema before invoking the handler. Servers must perform their own runtime type validation, typically using Zod schemas provided during tool registration via methods like server.tool or McpServer.registerTool. The SDK uses Zod (peer dependency) to parse and type-check arguments in the handler function parameters, but developers implement validation logic in handlers or rely on Zod's type inference. Official spec requires servers to "Validate all tool inputs" (security consideration), and tutorials show manual Zod validation in handlers. GitHub issues discuss schema inconsistencies and validation bugs, confirming it's developer responsibility.
Citations:
- 1: https://modelcontextprotocol.io/specification/2025-06-18/server/tools
- 2: https://github.com/modelcontextprotocol/typescript-sdk
- 3: https://modelcontextprotocol.info/specification/2024-11-05/server/tools
- 4: https://oneuptime.com/blog/post/2025-12-17-build-mcp-server-nodejs/view
- 5: Inconsistent handling of
inputSchemawhen schema is{}vsundefinedmodelcontextprotocol/typescript-sdk#458 - 6: InvalidParams error when all fields in the tool schema are optional modelcontextprotocol/typescript-sdk#400
- 7: https://modelcontextprotocol.github.io/typescript-sdk/documents/protocol.html
- 8: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_server.server_mcp.McpServer.html
- 9:
registerTool: accept ZodType<object> for input and output schema modelcontextprotocol/typescript-sdk#816
Add runtime type validation to requireValue() helper.
The @modelcontextprotocol/sdk does not validate incoming tool arguments against inputSchema before invoking handlers—servers must perform their own runtime validation. The current requireValue() only rejects null/undefined, allowing malformed non-numeric values like '', false, or NaN to bypass validation. These coerce silently in validateRange() and Math.round(), sending 0/NaN commands to devices instead of rejecting the request.
Fix
- private requireValue(value: number | null | undefined, action: string): number {
+ private requireValue(value: unknown, action: string): number {
if (value == null) {
throw new BadRequestException(`value is required for ${action} action`);
}
+ if (typeof value !== 'number' || !Number.isFinite(value)) {
+ throw new BadRequestException(`value for ${action} must be a finite number`);
+ }
return value;
}📝 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.
Suggested change
| private requireValue(value: number | null | undefined, action: string): number { | |
| if (value == null) { | |
| throw new BadRequestException(`value is required for ${action} action`); | |
| } | |
| return value; | |
| } | |
| private validateRange(value: number, min: number, max: number, label: string): void { | |
| if (value < min || value > max) { | |
| throw new BadRequestException(`${label} must be ${min}-${max}. Got: ${value}`); | |
| } | |
| } | |
| private requireValue(value: unknown, action: string): number { | |
| if (value == null) { | |
| throw new BadRequestException(`value is required for ${action} action`); | |
| } | |
| if (typeof value !== 'number' || !Number.isFinite(value)) { | |
| throw new BadRequestException(`value for ${action} must be a finite number`); | |
| } | |
| return value; | |
| } | |
| private validateRange(value: number, min: number, max: number, label: string): void { | |
| if (value < min || value > max) { | |
| throw new BadRequestException(`${label} must be ${min}-${max}. Got: ${value}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/services/tool-executor.service.ts` around lines 131 - 142,
requireValue currently only checks for null/undefined and lets non-numeric
inputs (''/false/NaN) through, causing downstream Math.round/validateRange to
operate on invalid numbers; update requireValue(value: number | null |
undefined, action: string) to perform runtime numeric validation by verifying
typeof value === 'number' and Number.isFinite(value) (reject NaN/Infinity and
non-number primitives) and throw BadRequestException with a clear message
referencing the action when validation fails; keep validateRange as-is but rely
on the strengthened requireValue to ensure callers (e.g., places that call
Math.round or validateRange) never receive invalid numbers.
Replace ChatGPT-specific window.openai.* API with standard MCP Apps JSON-RPC postMessage protocol. Widget now works on ChatGPT, Claude Desktop, and GitHub Copilot (VS Code 1.109+). - Add minimal _bridge object speaking JSON-RPC 2.0 over postMessage - Replace all window.openai.callTool → _bridge.callServerTool (7 sites) - Replace initialization from window.openai.toolOutput → bridge connect - Add flat _meta["ui/resourceUri"] to tool defs for Claude compat - Keep window.openai.* as fallback for legacy ChatGPT versions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@views/widgets/device-app.html`:
- Around line 1433-1435: The widget caches the current language in _lang inside
t(), so if the host locale arrives asynchronously (e.g., after connect()
resolves or later via host context) the UI stays in the old language; update the
onhostcontextchanged handler (the _bridge.on('onhostcontextchanged', ...)
listener) to invalidate the translation cache and re-render: clear or reset the
cached _lang used by t() (e.g., set _lang = undefined/null) and trigger the same
render/update path you use after changing <html lang> so translations are
recomputed when the host locale arrives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 195cb588-f3f9-4b69-ae69-df48d982f389
📒 Files selected for processing (3)
src/tools/definitions/get-device.tool.tssrc/tools/definitions/list-devices.tool.tsviews/widgets/device-app.html
✅ Files skipped from review due to trivial changes (2)
- src/tools/definitions/get-device.tool.ts
- src/tools/definitions/list-devices.tool.ts
Comment on lines
+1433
to
+1435
| _bridge.on('onhostcontextchanged', function (ctx) { | ||
| if (ctx && ctx.theme) applyTheme(ctx.theme); | ||
| }); |
There was a problem hiding this comment.
Invalidate translations when locale arrives asynchronously.
Line 2646 updates <html lang>, but t() caches _lang on first use. If the 200 ms fallback at Lines 2651-2655 renders before connect() resolves, or the host changes locale later, the widget stays in the previous language until reload.
🌐 Suggested fix
_bridge.on('onhostcontextchanged', function (ctx) {
if (ctx && ctx.theme) applyTheme(ctx.theme);
+ if (ctx && ctx.locale && ctx.locale !== document.documentElement.lang) {
+ document.documentElement.lang = ctx.locale;
+ _lang = null;
+ if (_currentView && _currentData) renderView(_currentView, _currentData);
+ }
});
@@
_bridge.connect().then(function (ctx) {
if (ctx && ctx.theme) applyTheme(ctx.theme);
if (ctx && ctx.locale) {
document.documentElement.lang = ctx.locale;
+ _lang = null;
+ if (_currentView && _currentData) renderView(_currentView, _currentData);
}
});Also applies to: 2643-2655
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/widgets/device-app.html` around lines 1433 - 1435, The widget caches
the current language in _lang inside t(), so if the host locale arrives
asynchronously (e.g., after connect() resolves or later via host context) the UI
stays in the old language; update the onhostcontextchanged handler (the
_bridge.on('onhostcontextchanged', ...) listener) to invalidate the translation
cache and re-render: clear or reset the cached _lang used by t() (e.g., set
_lang = undefined/null) and trigger the same render/update path you use after
changing <html lang> so translations are recomputed when the host locale
arrives.
The ext-apps SDK sends { method: 'tools/call' } for callServerTool(),
not 'ui/callServerTool'. The host proxies standard MCP methods directly
to the server. Wrong method name caused tool calls from the widget to
hang on GitHub Copilot (no response to unknown method).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
views/widgets/device-app.html (1)
2641-2646:⚠️ Potential issue | 🟡 MinorLocale set in
connect()may not apply to already-cached translations.Setting
document.documentElement.langhere won't affect translations ift()was already called (e.g., during the 200ms fallback render or initial loading text). Reset_lang = nulland consider re-rendering if a view was already displayed.🌐 Suggested fix
_bridge.connect().then(function (ctx) { if (ctx && ctx.theme) applyTheme(ctx.theme); if (ctx && ctx.locale) { document.documentElement.lang = ctx.locale; + _lang = null; + if (_currentView && _currentData) renderView(_currentView, _currentData); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@views/widgets/device-app.html` around lines 2641 - 2646, The locale set in the _bridge.connect() callback by assigning document.documentElement.lang won't refresh already-cached translations used by t(); reset the internal cache variable (_lang = null) after setting the lang and trigger a re-render of the current view if one is displayed so translations are recomputed; locate the connect callback where _bridge.connect().then(...) and applyTheme(ctx.theme) are used, set document.documentElement.lang = ctx.locale, then clear _lang and call the view re-render function (or invoke the existing render/update routine) to update displayed text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@views/widgets/device-app.html`:
- Around line 1228-1237: The _send function leaves pending JSON-RPC requests in
the _pending map indefinitely; add a per-request timeout inside _send that
rejects the Promise and deletes _pending[id] if no response arrives within
(e.g.) 30s, and ensure you clear that timer when the response handler
resolves/rejects the same id; reference the _send function, the _pending map,
the generated id (_id) and the existing response handling path so the timer is
stored per-request and cleaned up to avoid memory leaks.
---
Duplicate comments:
In `@views/widgets/device-app.html`:
- Around line 2641-2646: The locale set in the _bridge.connect() callback by
assigning document.documentElement.lang won't refresh already-cached
translations used by t(); reset the internal cache variable (_lang = null) after
setting the lang and trigger a re-render of the current view if one is displayed
so translations are recomputed; locate the connect callback where
_bridge.connect().then(...) and applyTheme(ctx.theme) are used, set
document.documentElement.lang = ctx.locale, then clear _lang and call the view
re-render function (or invoke the existing render/update routine) to update
displayed text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2aeee9-af6e-41dc-a012-98368189cce0
📒 Files selected for processing (1)
views/widgets/device-app.html
Comment on lines
+1228
to
+1237
| function _send(method, params) { | ||
| var id = ++_id; | ||
| return new Promise(function (resolve, reject) { | ||
| _pending[id] = { resolve: resolve, reject: reject }; | ||
| window.parent.postMessage( | ||
| { jsonrpc: '2.0', method: method, params: params || {}, id: id }, | ||
| '*', | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing timeout for pending JSON-RPC requests.
The _send function stores callbacks in _pending indefinitely. If the parent frame never responds (e.g., host doesn't support the protocol, network issues, or parent crashes), the Promise never resolves and the callback leaks memory.
Consider adding a timeout to reject stale requests:
🛡️ Suggested fix
function _send(method, params) {
var id = ++_id;
return new Promise(function (resolve, reject) {
- _pending[id] = { resolve: resolve, reject: reject };
+ var timeoutId = setTimeout(function () {
+ if (_pending[id]) {
+ delete _pending[id];
+ reject({ code: -32000, message: 'Request timeout' });
+ }
+ }, 10000);
+ _pending[id] = {
+ resolve: function (r) { clearTimeout(timeoutId); resolve(r); },
+ reject: function (e) { clearTimeout(timeoutId); reject(e); }
+ };
window.parent.postMessage(
{ jsonrpc: '2.0', method: method, params: params || {}, id: id },
'*',
);
});
}📝 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.
Suggested change
| function _send(method, params) { | |
| var id = ++_id; | |
| return new Promise(function (resolve, reject) { | |
| _pending[id] = { resolve: resolve, reject: reject }; | |
| window.parent.postMessage( | |
| { jsonrpc: '2.0', method: method, params: params || {}, id: id }, | |
| '*', | |
| ); | |
| }); | |
| } | |
| function _send(method, params) { | |
| var id = ++_id; | |
| return new Promise(function (resolve, reject) { | |
| var timeoutId = setTimeout(function () { | |
| if (_pending[id]) { | |
| delete _pending[id]; | |
| reject({ code: -32000, message: 'Request timeout' }); | |
| } | |
| }, 10000); | |
| _pending[id] = { | |
| resolve: function (r) { clearTimeout(timeoutId); resolve(r); }, | |
| reject: function (e) { clearTimeout(timeoutId); reject(e); } | |
| }; | |
| window.parent.postMessage( | |
| { jsonrpc: '2.0', method: method, params: params || {}, id: id }, | |
| '*', | |
| ); | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@views/widgets/device-app.html` around lines 1228 - 1237, The _send function
leaves pending JSON-RPC requests in the _pending map indefinitely; add a
per-request timeout inside _send that rejects the Promise and deletes
_pending[id] if no response arrives within (e.g.) 30s, and ensure you clear that
timer when the response handler resolves/rejects the same id; reference the
_send function, the _pending map, the generated id (_id) and the existing
response handling path so the timer is stored per-request and cleaned up to
avoid memory leaks.
Model-visible tool that opens the widget control UI when the user wants to control a device without specifying exact actions. Reuses the same executeWidgetControlDevice handler (_view: 'control'). AI description guides tool selection: vague requests → interactive_device widget, specific commands → control_device_simple.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation