refactor: remove no-this-alias suppressions in test mocks#175
refactor: remove no-this-alias suppressions in test mocks#175gabrypavanello wants to merge 7 commits intomainfrom
Conversation
…SK-037-02) Replace all raw console.log/warn/error calls across 17 inspector/src files with scoped logger instances using createLogger(). Each module gets a descriptive source name for log filtering. - 17 files migrated to structured logger - Stale eslint-disable no-console comments removed - Tests updated to match new console.info output format - bin/mcp-inspector.ts and widget-server-templates.ts untouched - proxy-resources.ts skipped (console call is in client-side template)
…efineMiddleware Remove 3 eslint-disable comments for @typescript-eslint/no-confusing-void-expression in defineMiddleware.ts. The next() function returns Promise<void>, so capturing or returning the void result was unnecessary. Now we simply await next() without capturing the result. Kaizen: daily tech debt reduction
… capture Replace pattern with direct access inside arrow functions passed to setTimeout. Arrow functions lexically bind , making the alias unnecessary. Removes 2 eslint-disable comments.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces a centralized, structured logging system with environment-driven log levels across packages/inspector, packages/codegen, and packages/testing, replacing direct console output with timestamped, level-gated logger calls. Additionally modifies middleware to stop propagating return values from next() calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Code ReviewThanks for this cleanup PR. The intent is clear and the changes are in the right direction. I have a few observations across the different areas touched.
|
| Area | Assessment |
|---|---|
Core goal (remove no-this-alias) |
Correct |
| New structured logger | Good direction — log level snapshot caveat worth addressing |
| Double-prefix in call sites | Noisy output; inline tags should be removed from message strings |
| Logger exported from public API | Needs an explicit decision |
| Inline IIFE logger duplication | Acknowledged in comments; consider extracting if the pattern keeps spreading |
Middleware return result removal |
Correct |
| Test coverage | Good; minor gap on env-var snapshot behaviour |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/core/src/middleware/defineMiddleware.ts (1)
185-195:⚠️ Potential issue | 🟡 MinorUpdate stale comment about returning next()’s result.
The note still says we return next()’s result, but the code now discards it. Please update/remove it to avoid misleading behavior assumptions.
Proposed update
- // Note: We return the result from next() even though Middleware type says Promise<void>. - // This allows void-based middleware to work seamlessly with MiddlewareChain<TResult>. - // The type assertion is safe because JavaScript doesn't enforce return types at runtime. + // Note: We intentionally do not return next()'s result for void-based middleware. + // This keeps Middleware<void> semantics explicit and avoids mixing result-passing chains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/middleware/defineMiddleware.ts` around lines 185 - 195, The existing comment block above the returned middleware incorrectly states that the middleware returns next()'s result; in reality the implementation awaits next() and discards its result. Update or remove that comment to accurately describe behavior in the closure returned by defineMiddleware (the async (context, next) => { ... } function): state that we await next() for sequencing but do not return its value (middleware effectively returns void), and adjust wording around compatibility with MiddlewareChain<TResult> if needed so it no longer claims we propagate next()'s return value.packages/inspector/src/widget-session-manager.ts (2)
733-761:⚠️ Potential issue | 🟠 MajorReplace
logger.*calls withconsole.*insidepage.evaluate()andframe.evaluate()callbacks.The
loggerobject is not available in the browser context wherepage.evaluate()executes, causing runtime failures. Useconsole.info()instead for logging within evaluated functions.This affects multiple locations:
- Line ~745:
logger.info("[MCP Host] Stored hostContext update for ui/initialize:", p);- Line ~752:
logger.info("[MCP Host] Sent synced event:", m, p);- Line ~795:
logger.info("[OpenAI Host] Sent synced event:", message.syncType, message.data);- Additional instances also found in similar
page.evaluate()andframe.evaluate()callbacks throughout the file.Replace with
console.info()in all evaluated functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/widget-session-manager.ts` around lines 733 - 761, Inside the evaluated browser contexts (calls to session.page.evaluate and frame.evaluate) replace uses of the node-side logger (logger.info / logger.error / logger.debug) with the browser console (console.info / console.error / console.debug) because logger is undefined in the page/frame context; locate the evaluated functions that reference logger (e.g., the callback passed to session.page.evaluate that stores __mcpHostContextUpdates and posts to iframe.contentWindow, and the callback that posts OpenAI Host sync messages using message.syncType/message.data) and update all logger.* invocations to the equivalent console.* calls so logging occurs in the page/frame context without throwing.
401-414:⚠️ Potential issue | 🟠 MajorRemove
logger.infocalls insidepage.evaluateblocks (Node logger undefined in browser context).
page.evaluateexecutes in the browser, where the Node.jsloggerinstance is undefined and will throw a ReferenceError. Replace withconsole.infoor log outside the evaluate call.Affected locations:
- Line 412 (updateSessionGlobals, MCP protocol)
- Line 442 (updateSessionGlobals, OpenAI protocol)
- Lines 747, 760 (deliverMcpEvent)
- Line 796 (deliverOpenAIEvent)
🔧 Proposed fix
- logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); + console.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx);- logger.info("[OpenAI Host] Sent globals sync:", message.data); + console.info("[OpenAI Host] Sent globals sync:", message.data);- logger.info("[MCP Host] Stored hostContext update for ui/initialize:", p); + console.info("[MCP Host] Stored hostContext update for ui/initialize:", p);- logger.info("[MCP Host] Sent synced event:", m, p); + console.info("[MCP Host] Sent synced event:", m, p);- logger.info("[OpenAI Host] Sent synced event:", message.syncType, message.data); + console.info("[OpenAI Host] Sent synced event:", message.syncType, message.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/widget-session-manager.ts` around lines 401 - 414, The page.evaluate callbacks (used in updateSessionGlobals and in deliverMcpEvent / deliverOpenAIEvent) are running in the browser context where the Node.js logger is undefined; remove or replace logger.info calls inside those page.evaluate blocks with console.info (or move the logger calls to after page.evaluate returns) — update the logger usage in the evaluate call inside updateSessionGlobals (MCP and OpenAI protocol), and the evaluate blocks inside deliverMcpEvent and deliverOpenAIEvent to use console.info or perform Node-side logging immediately after await page.evaluate.packages/inspector/src/hosts/mcp-host.ts (1)
220-281:⚠️ Potential issue | 🟠 MajorReplace
logger.info()withconsole.info()in the Playwright init script.The template string returned by
getPlaywrightInitScript()is executed in the browser context wherelogger(a Node.js variable) is undefined. This causes a ReferenceError at runtime whenui/initializeor display-mode handlers execute. Useconsole.info()instead for all logging inside the template string (lines 222, 247, 251, 280).🔧 Proposed fix
- logger.info('[MCP Host Emulator] Received ui/initialize, responding...'); + console.info('[MCP Host Emulator] Received ui/initialize, responding...');- logger.info('[MCP Host Emulator] Sent ui/initialize response'); + console.info('[MCP Host Emulator] Sent ui/initialize response');- logger.info('[MCP Host Emulator] Sending ui/notifications/tool-result...'); + console.info('[MCP Host Emulator] Sending ui/notifications/tool-result...');- logger.info('[MCP Host Emulator] Display mode changed to:', requestedMode, 'sizing:', sizing); + console.info('[MCP Host Emulator] Display mode changed to:', requestedMode, 'sizing:', sizing);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/hosts/mcp-host.ts` around lines 220 - 281, The template script emitted by getPlaywrightInitScript() uses a Node-only logger variable inside the browser-executed template (see the ui/initialize and ui/requests/display-mode message handlers and the tool-result send block); replace all logger.info(...) calls inside that template string with console.info(...) so the code runs in the browser context without ReferenceError (update occurrences in the ui/initialize response dispatch, the tool-result timeout block, and the display-mode handler).packages/inspector/src/session/session-renderer.ts (3)
282-334:⚠️ Potential issue | 🔴 CriticalCritical:
loggeris not available in browser context.All
logger.infocalls inside thispage.evaluate()callback (lines 286, 303, 312, 331) will fail at runtime becauseloggeris a Node.js module variable not available in the browser context.🐛 Proposed fix
/* eslint-disable no-undef */ await page.evaluate((responseData: unknown) => { // Validate responseData is a non-null object before accessing properties if (typeof responseData !== "object" || responseData === null) { - logger.info("[MCP Host] Tool response is not a valid object:", responseData); + console.info("[MCP Host] Tool response is not a valid object:", responseData); return; } const d = responseData as Record<string, unknown>; const nameValue = d.name; const toolNameValue = d.toolName; // Validate that name or toolName exists and is a string const toolName = typeof nameValue === "string" && nameValue ? nameValue : typeof toolNameValue === "string" && toolNameValue ? toolNameValue : null; if (!toolName) { - logger.info("[MCP Host] Tool response missing valid name/toolName string:", responseData); + console.info("[MCP Host] Tool response missing valid name/toolName string:", responseData); return; } type PendingCall = { messageId: number | string; args: unknown; timestamp: number }; const w = window as Window & { __pendingToolCalls?: Record<string, PendingCall[]> }; const pending = w.__pendingToolCalls?.[toolName]; if (!pending || pending.length === 0) { - logger.info("[MCP Host] No pending calls for tool:", toolName); + console.info("[MCP Host] No pending calls for tool:", toolName); return; } // Get the oldest pending call (FIFO) const call = pending.shift(); if (!call) return; // Send response to widget iframe const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { iframe.contentWindow.postMessage( { jsonrpc: "2.0", id: call.messageId, result: d.result ?? { content: [{ type: "text", text: JSON.stringify(d) }] }, }, "*" ); - logger.info("[MCP Host] Delivered synced tool response:", toolName, call.messageId); + console.info("[MCP Host] Delivered synced tool response:", toolName, call.messageId); } }, data); /* eslint-enable no-undef */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/session/session-renderer.ts` around lines 282 - 334, Inside the page.evaluate callback the Node.js variable logger is not available, so replace all uses of logger.info inside the page.evaluate((responseData) => { ... }) block with browser-safe console calls (e.g., console.info) or serialize a logging function into the evaluate args; update the four occurrences referenced in the snippet (the logger.info calls near the checks for responseData, missing toolName, no pending calls, and after delivering the response) so they use console.* or a passed-in log callback, leaving the rest of logic (page.evaluate, the pending lookup via window.__pendingToolCalls, pending.shift(), iframe.postMessage payload) unchanged.
206-220:⚠️ Potential issue | 🔴 CriticalCritical:
loggeris not available in browser context.
page.evaluate()executes code in the browser (Playwright's page context), not in Node.js. Theloggervariable is a Node.js module-scoped object and is not accessible inside the browser. This will throwReferenceError: logger is not definedat runtime.These calls should use
console.log(orconsole.info) since they run in the browser.🐛 Proposed fix
/* eslint-disable no-undef */ await page.evaluate((ctx: typeof hostContext) => { const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { const message = { jsonrpc: "2.0", method: "ui/notifications/host-context-changed", params: { hostContext: ctx }, }; iframe.contentWindow.postMessage(message, "*"); - logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); + console.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); } }, hostContext); /* eslint-enable no-undef */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/session/session-renderer.ts` around lines 206 - 220, The code inside page.evaluate uses the Node-only logger variable which is not available in the browser context (causing ReferenceError); update the callback passed to page.evaluate (the function referencing iframe and sending the "ui/notifications/host-context-changed" message) to replace logger.info with console.info (or console.log) so the log runs inside the page context, keeping the message payload and the use of hostContext, iframe id "widget-frame", and the jsonrpc method intact; alternatively, if you want Node-side logging, move any logger.info calls outside the page.evaluate call and log after evaluate returns.
250-259:⚠️ Potential issue | 🔴 CriticalCritical:
loggeris not available in browser context.Same issue as above —
page.evaluate()runs in the browser whereloggeris undefined.🐛 Proposed fix
/* eslint-disable no-undef */ await page.evaluate((message: typeof syncMessage) => { const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { iframe.contentWindow.postMessage(message, "*"); - logger.info("[OpenAI Host] Sent globals sync:", message.data); + console.info("[OpenAI Host] Sent globals sync:", message.data); } }, syncMessage); /* eslint-enable no-undef */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/session/session-renderer.ts` around lines 250 - 259, The evaluated function passed to page.evaluate references logger (which is unavailable in the browser) — remove any use of logger inside the page.evaluate callback and perform logging in Node context instead; specifically, change the block using page.evaluate(syncMessage) so the callback only finds the iframe and posts message (frame.contentWindow.postMessage(message, "*")), then after await page.evaluate(...) call logger.info("[OpenAI Host] Sent globals sync:", syncMessage.data) in the surrounding Node code (session-renderer / the function that calls page.evaluate) so no browser-side references to logger remain.
🧹 Nitpick comments (2)
packages/inspector/src/oauth/provider.ts (1)
632-636: Consider removing the redundant prefix.Similar to
wellknown-proxy.ts, the logger source"oauth-provider"already provides context. The additional[oauth:provider]prefix in the message creates redundant labeling in the output.🔧 Suggested simplification
private log(message: string): void { if (this._debug) { - logger.info(`[oauth:provider] ${message}`); + logger.info(message); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/oauth/provider.ts` around lines 632 - 636, The log method in provider.ts currently prefixes messages with "[oauth:provider]" even though the logger source already identifies this module; update the private log(message: string) method to remove the redundant prefix so it simply calls logger.info(message) when this._debug is true, preserving the existing this._debug check and method signature (private log) so all callers of log continue to work unchanged.packages/inspector/src/oauth/wellknown-proxy.ts (1)
81-85: Consider removing the redundant prefix in the log helper.The logger already includes the source
"wellknown-proxy"in its formatted output (e.g.,[INFO] [wellknown-proxy]). The additional[oauth:wellknown-proxy]prefix in the log message creates redundant labeling.🔧 Suggested simplification
function log(message: string): void { if (debug) { - logger.info(`[oauth:wellknown-proxy] ${message}`); + logger.info(message); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/inspector/src/oauth/wellknown-proxy.ts` around lines 81 - 85, The helper function log currently prepends "[oauth:wellknown-proxy]" to messages causing redundant labels; update the log function (log(message: string)) to call logger.info(message) when debug is true instead of prefixing, so messages use the logger's existing "[wellknown-proxy]" source formatting; keep the debug gating and no other callers need changing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/inspector/src/hosts/openai-host.ts`:
- Around line 248-249: The injected Playwright script in openai-host.ts uses the
Node-only symbol logger (see the requestDisplayMode snippet), which will throw
in the browser; replace logger.info('[OpenAI Host] requestDisplayMode:', mode,
'sizing:', sizing); with a browser-safe call (console.info) inside the script
passed to page.addInitScript() or the requestDisplayMode injection function so
the message logs in the page context without referencing Node modules.
---
Outside diff comments:
In `@packages/core/src/middleware/defineMiddleware.ts`:
- Around line 185-195: The existing comment block above the returned middleware
incorrectly states that the middleware returns next()'s result; in reality the
implementation awaits next() and discards its result. Update or remove that
comment to accurately describe behavior in the closure returned by
defineMiddleware (the async (context, next) => { ... } function): state that we
await next() for sequencing but do not return its value (middleware effectively
returns void), and adjust wording around compatibility with
MiddlewareChain<TResult> if needed so it no longer claims we propagate next()'s
return value.
In `@packages/inspector/src/hosts/mcp-host.ts`:
- Around line 220-281: The template script emitted by getPlaywrightInitScript()
uses a Node-only logger variable inside the browser-executed template (see the
ui/initialize and ui/requests/display-mode message handlers and the tool-result
send block); replace all logger.info(...) calls inside that template string with
console.info(...) so the code runs in the browser context without ReferenceError
(update occurrences in the ui/initialize response dispatch, the tool-result
timeout block, and the display-mode handler).
In `@packages/inspector/src/session/session-renderer.ts`:
- Around line 282-334: Inside the page.evaluate callback the Node.js variable
logger is not available, so replace all uses of logger.info inside the
page.evaluate((responseData) => { ... }) block with browser-safe console calls
(e.g., console.info) or serialize a logging function into the evaluate args;
update the four occurrences referenced in the snippet (the logger.info calls
near the checks for responseData, missing toolName, no pending calls, and after
delivering the response) so they use console.* or a passed-in log callback,
leaving the rest of logic (page.evaluate, the pending lookup via
window.__pendingToolCalls, pending.shift(), iframe.postMessage payload)
unchanged.
- Around line 206-220: The code inside page.evaluate uses the Node-only logger
variable which is not available in the browser context (causing ReferenceError);
update the callback passed to page.evaluate (the function referencing iframe and
sending the "ui/notifications/host-context-changed" message) to replace
logger.info with console.info (or console.log) so the log runs inside the page
context, keeping the message payload and the use of hostContext, iframe id
"widget-frame", and the jsonrpc method intact; alternatively, if you want
Node-side logging, move any logger.info calls outside the page.evaluate call and
log after evaluate returns.
- Around line 250-259: The evaluated function passed to page.evaluate references
logger (which is unavailable in the browser) — remove any use of logger inside
the page.evaluate callback and perform logging in Node context instead;
specifically, change the block using page.evaluate(syncMessage) so the callback
only finds the iframe and posts message
(frame.contentWindow.postMessage(message, "*")), then after await
page.evaluate(...) call logger.info("[OpenAI Host] Sent globals sync:",
syncMessage.data) in the surrounding Node code (session-renderer / the function
that calls page.evaluate) so no browser-side references to logger remain.
In `@packages/inspector/src/widget-session-manager.ts`:
- Around line 733-761: Inside the evaluated browser contexts (calls to
session.page.evaluate and frame.evaluate) replace uses of the node-side logger
(logger.info / logger.error / logger.debug) with the browser console
(console.info / console.error / console.debug) because logger is undefined in
the page/frame context; locate the evaluated functions that reference logger
(e.g., the callback passed to session.page.evaluate that stores
__mcpHostContextUpdates and posts to iframe.contentWindow, and the callback that
posts OpenAI Host sync messages using message.syncType/message.data) and update
all logger.* invocations to the equivalent console.* calls so logging occurs in
the page/frame context without throwing.
- Around line 401-414: The page.evaluate callbacks (used in updateSessionGlobals
and in deliverMcpEvent / deliverOpenAIEvent) are running in the browser context
where the Node.js logger is undefined; remove or replace logger.info calls
inside those page.evaluate blocks with console.info (or move the logger calls to
after page.evaluate returns) — update the logger usage in the evaluate call
inside updateSessionGlobals (MCP and OpenAI protocol), and the evaluate blocks
inside deliverMcpEvent and deliverOpenAIEvent to use console.info or perform
Node-side logging immediately after await page.evaluate.
---
Nitpick comments:
In `@packages/inspector/src/oauth/provider.ts`:
- Around line 632-636: The log method in provider.ts currently prefixes messages
with "[oauth:provider]" even though the logger source already identifies this
module; update the private log(message: string) method to remove the redundant
prefix so it simply calls logger.info(message) when this._debug is true,
preserving the existing this._debug check and method signature (private log) so
all callers of log continue to work unchanged.
In `@packages/inspector/src/oauth/wellknown-proxy.ts`:
- Around line 81-85: The helper function log currently prepends
"[oauth:wellknown-proxy]" to messages causing redundant labels; update the log
function (log(message: string)) to call logger.info(message) when debug is true
instead of prefixing, so messages use the logger's existing "[wellknown-proxy]"
source formatting; keep the debug gating and no other callers need changing.
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | ||
| return { mode: mode }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate logger usage and context around Playwright init script
rg -n "getPlaywrightInitScript|logger\.info" packages/inspector/src/hosts/openai-host.ts -A 5 -B 5Repository: AndurilCode/mcp-apps-kit
Length of output: 2198
🏁 Script executed:
sed -n '240,260p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 640
🏁 Script executed:
cat -n packages/inspector/src/hosts/openai-host.ts | grep -E "getPlaywrightInitScript|addInitScript" -A 10 -B 2Repository: AndurilCode/mcp-apps-kit
Length of output: 898
🏁 Script executed:
sed -n '101,360p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 9500
🏁 Script executed:
head -50 packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1543
Replace logger.info with console.info inside the Playwright injected script.
The logger module is Node.js-scoped and unavailable in the browser context where this code runs via addInitScript(). This will throw a ReferenceError at runtime.
🔧 Proposed fix
- logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);
+ console.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/inspector/src/hosts/openai-host.ts` around lines 248 - 249, The
injected Playwright script in openai-host.ts uses the Node-only symbol logger
(see the requestDisplayMode snippet), which will throw in the browser; replace
logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); with
a browser-safe call (console.info) inside the script passed to
page.addInitScript() or the requestDisplayMode injection function so the message
logs in the page context without referencing Node modules.
1 participant
Kaizen 🧹
Removes 2 eslint-disable @typescript-eslint/no-this-alias suppressions in test-client.test.ts.
Arrow functions lexically bind this, so the const self = this pattern inside class constructors is unnecessary when the callback is already an arrow function.
Changes: 2 suppressions removed, 0 behavior changes.