kaizen: replace eslint-disable with idiomatic _kind in PrimitiveDetail#177
kaizen: replace eslint-disable with idiomatic _kind in PrimitiveDetail#177gabrypavanello wants to merge 6 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)
Use the underscore-prefix convention (already configured in ESLint as varsIgnorePattern: '^_') to signal that is intentionally unused after destructuring. Removes the inline eslint-disable comment in favour of the project's established pattern. No behaviour change.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces a centralized, structured logging system across the codegen and inspector packages, replacing console.log/warn/error calls with scoped, timestamped loggers controlled by the MCP_APPS_LOG_LEVEL environment variable. The logger supports debug, info, warn, and error levels with consistent formatting. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Comment |
Code Review — PR #177Thanks for the cleanup work. The core Bug:
|
| Severity | Issue |
|---|---|
| Bug (critical) | logger used inside page.evaluate() browser-context callbacks — will ReferenceError at runtime |
| Style | Import declarations interleaved with executable statements in 6 files |
| Scope | PR description does not match the actual extent of changes |
| Design | Duplicated inline logger logic across 3 packages |
The critical issue needs to be fixed before merge. The style issues are lower priority but worth addressing in the same PR given they touch the same files.
![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/inspector/src/widget-session-manager.ts (4)
400-415:⚠️ Potential issue | 🔴 Critical
loggeris not available in browser context — will cause runtime error.The
logger.info()call at line 412 is inside apage.evaluate()callback, which executes in the browser context. The Node.jsloggervariable is not serialized or available in that context, causing aReferenceError: logger is not definedat runtime.Use
console.logfor browser-context logging, or remove the log statement since it's inside the browser and won't appear in server logs anyway.Proposed fix
iframe.contentWindow.postMessage(message, "*"); - logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); + // Browser context: use console.log (not Node logger) + console.log("[MCP Host] Sent ui/notifications/host-context-changed", ctx); }Or remove the log entirely if browser-side logging is not desired.
🤖 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 400 - 415, The browser-executed callback passed to page.evaluate references the Node-side logger (logger.info) which isn't available in the browser context and causes a ReferenceError; update the callback inside page.evaluate (the function using hostContext, iframe, and iframe.contentWindow.postMessage) to remove or replace logger.info with a browser-safe call (e.g., console.log) or omit logging entirely so the browser context does not reference the Node logger variable.
436-445:⚠️ Potential issue | 🔴 CriticalSame browser context issue —
loggerunavailable inpage.evaluate().Line 442 has the same problem:
logger.info()inside a browser-evaluated callback will fail at runtime.Proposed fix
iframe.contentWindow.postMessage(message, "*"); - logger.info("[OpenAI Host] Sent globals sync:", message.data); + console.log("[OpenAI Host] Sent globals sync:", 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 436 - 445, The call to logger.info inside page.evaluate will fail because logger is not defined in the browser context; update the block that calls page.evaluate with (message: typeof syncMessage) => { ... } so that it only posts the message from the page context and returns any needed status (e.g., true or message.data), then perform logger.info(...) in the Node context after await page.evaluate(...) completes (refer to the page.evaluate invocation, the syncMessage argument, and the logger variable) so logging runs outside the browser-evaluated function.
791-799:⚠️ Potential issue | 🔴 CriticalBrowser context
loggerusage at line 796 will fail.Same issue:
logger.info()insidepage.evaluate()runs in browser context.Proposed fix
if (iframe?.contentWindow) { iframe.contentWindow.postMessage(message, "*"); - logger.info("[OpenAI Host] Sent synced event:", message.syncType, message.data); + console.log("[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 791 - 799, The code calls logger.info(...) inside session.page.evaluate (browser context) which will fail because logger isn't available in the page scope; update the logic in widget-session-manager.ts so page.evaluate only posts the message and returns any needed status or payload (use session.page.evaluate((message)=>{...; return true}, syncMessage) or similar), then call logger.info(...) in the Node context after await session.page.evaluate resolves (reference the symbols session.page.evaluate, syncMessage, and logger) so logging happens outside the browser sandbox; alternatively, pass only serializable data back from the page and log that from the surrounding async function.
732-765:⚠️ Potential issue | 🔴 CriticalBrowser context
loggerusage at lines 747 and 760 will fail.Both
logger.info()calls inside thispage.evaluate()callback are in browser context where the Node.js logger is undefined.Proposed fix
if (storeOnHost) { const w = window as Window & { __mcpHostContextUpdates?: Record<string, unknown> }; w.__mcpHostContextUpdates = { ...(w.__mcpHostContextUpdates ?? {}), ...(p as object) }; - logger.info("[MCP Host] Stored hostContext update for ui/initialize:", p); + console.log("[MCP Host] Stored hostContext update for ui/initialize:", p); } const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { iframe.contentWindow.postMessage( { jsonrpc: "2.0", method: m, params: p, }, "*" ); - logger.info("[MCP Host] Sent synced event:", m, p); + console.log("[MCP Host] Sent synced event:", m, p); }🤖 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 732 - 765, The browser-evaluated function passed to session.page.evaluate uses the Node logger (logger.info) which is undefined in the page context; replace those calls so logging happens correctly: inside the callback (the function passed to session.page.evaluate) remove or replace logger.info(...) with console.info(...) (or collect the log messages into a returned object) and then, if you need Node-side logging with the existing logger, return the details from session.page.evaluate and call logger.info(...) in the outer scope after the await; update the function that references method, params, storeOnHost, isHostContextUpdate, iframe ("widget-frame") and __mcpHostContextUpdates accordingly so no direct use of logger occurs inside the browser context.packages/inspector/src/hosts/openai-host.ts (1)
226-250:⚠️ Potential issue | 🟠 MajorReplace
logger.info()withconsole.info()in the Playwright init script.The
logger.info()call at line 248 runs in the browser context ofaddInitScript, where theloggervariable (defined in Node.js scope on line 22) is not available. This will throw a ReferenceError and breakrequestDisplayModehandling.Suggested 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 226 - 250, The browser-side function requestDisplayMode inside the addInitScript uses logger.info which is a Node scope variable and will throw in the page context; replace the logger.info call with console.info (keeping the same message and variables: mode and sizing) so the log runs in the browser environment, e.g. update the logging statement in requestDisplayMode to use console.info(...) instead of logger.info(...).packages/inspector/src/session/session-renderer.ts (1)
206-217:⚠️ Potential issue | 🟠 MajorReplace
loggercalls withconsoleinsidepage.evaluateblocks.
page.evaluate()executes JavaScript in the browser context whereloggeris not available. Six instances found across lines 217, 256, 287, 304, 313, and 332 will throw runtime errors if executed.Fixes required
- 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] Tool response is not a valid object:", responseData); + console.info("[MCP Host] Tool response is not a valid object:", responseData);- logger.info("[MCP Host] Tool response missing valid name/toolName string:", responseData); + console.info("[MCP Host] Tool response missing valid name/toolName string:", responseData);- logger.info("[MCP Host] No pending calls for tool:", toolName); + console.info("[MCP Host] No pending calls for tool:", toolName);- logger.info("[MCP Host] Delivered synced tool response:", toolName, call.messageId); + console.info("[MCP Host] Delivered synced tool response:", toolName, call.messageId);🤖 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 - 217, Inside the page.evaluate(...) browser-executed callbacks (the function that receives ctx and manipulates the iframe), replace any use of the Node-side logger object with the browser console API (e.g., console.info/console.warn/console.error) so logging runs in the page context rather than throwing; update all occurrences inside the page.evaluate callbacks (the function passed to page.evaluate in session-renderer.ts that references ctx and iframe) to use console instead of logger while keeping the original messages and arguments.packages/inspector/src/hosts/mcp-host.ts (1)
220-305:⚠️ Potential issue | 🟠 MajorReplace logger calls with console in the Playwright init script.
The injected script runs in the browser context where
loggeris undefined. The fourlogger.info()calls in the ui/initialize, tool-result, and display-mode handlers will throw a ReferenceError. Useconsole.info()instead.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 - 305, The injected Playwright browser script uses an undefined logger and will throw ReferenceError; replace all logger.info(...) calls in the MCP host emulator message handlers with console.info(...). Specifically, update the ui/initialize handler (the block that builds and dispatches the initialization response and schedules the tool-result), the setTimeout that sends the ui/notifications/tool-result resultMessage, and the ui/requests/display-mode handler (where it logs display mode and sizing) to use console.info so the code in methods handling message (jsonrpc 'ui/initialize', 'ui/notifications/tool-result', and 'ui/requests/display-mode') runs in the browser context without errors.
🧹 Nitpick comments (1)
packages/inspector/src/oauth/wellknown-proxy.ts (1)
81-85: Redundant prefix in log message.The logger already formats output as
timestamp [INFO] [wellknown-proxy] ..., so the[oauth:wellknown-proxy]prefix in the message creates redundancy:2026-02-20T... [INFO] [wellknown-proxy] [oauth:wellknown-proxy] Upstream changed...Consider removing the inline prefix since the source is already captured by the logger.
Proposed fix
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 inline prefix "[oauth:wellknown-proxy]" in the log() helper duplicates the logger's own source tag; update the log function (log(message: string)) to stop prepending that prefix and call logger.info(message) when debug is true so messages are not double-tagged.
🤖 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/testing/src/eval/mcp/evaluator.ts`:
- Around line 37-50: Replace the custom inline evaluatorLogger with the shared
llmLogger: remove the evaluatorLogger IIFE (including the eslint-disable
comment) and instead import the exported llmLogger from the package's
debug/logger module, then use llmLogger.warn(...) wherever
evaluatorLogger.warn(...) was used; ensure any timestamp/source formatting is
preserved by using llmLogger's API rather than reimplementing logging logic.
---
Outside diff comments:
In `@packages/inspector/src/hosts/mcp-host.ts`:
- Around line 220-305: The injected Playwright browser script uses an undefined
logger and will throw ReferenceError; replace all logger.info(...) calls in the
MCP host emulator message handlers with console.info(...). Specifically, update
the ui/initialize handler (the block that builds and dispatches the
initialization response and schedules the tool-result), the setTimeout that
sends the ui/notifications/tool-result resultMessage, and the
ui/requests/display-mode handler (where it logs display mode and sizing) to use
console.info so the code in methods handling message (jsonrpc 'ui/initialize',
'ui/notifications/tool-result', and 'ui/requests/display-mode') runs in the
browser context without errors.
In `@packages/inspector/src/hosts/openai-host.ts`:
- Around line 226-250: The browser-side function requestDisplayMode inside the
addInitScript uses logger.info which is a Node scope variable and will throw in
the page context; replace the logger.info call with console.info (keeping the
same message and variables: mode and sizing) so the log runs in the browser
environment, e.g. update the logging statement in requestDisplayMode to use
console.info(...) instead of logger.info(...).
In `@packages/inspector/src/session/session-renderer.ts`:
- Around line 206-217: Inside the page.evaluate(...) browser-executed callbacks
(the function that receives ctx and manipulates the iframe), replace any use of
the Node-side logger object with the browser console API (e.g.,
console.info/console.warn/console.error) so logging runs in the page context
rather than throwing; update all occurrences inside the page.evaluate callbacks
(the function passed to page.evaluate in session-renderer.ts that references ctx
and iframe) to use console instead of logger while keeping the original messages
and arguments.
In `@packages/inspector/src/widget-session-manager.ts`:
- Around line 400-415: The browser-executed callback passed to page.evaluate
references the Node-side logger (logger.info) which isn't available in the
browser context and causes a ReferenceError; update the callback inside
page.evaluate (the function using hostContext, iframe, and
iframe.contentWindow.postMessage) to remove or replace logger.info with a
browser-safe call (e.g., console.log) or omit logging entirely so the browser
context does not reference the Node logger variable.
- Around line 436-445: The call to logger.info inside page.evaluate will fail
because logger is not defined in the browser context; update the block that
calls page.evaluate with (message: typeof syncMessage) => { ... } so that it
only posts the message from the page context and returns any needed status
(e.g., true or message.data), then perform logger.info(...) in the Node context
after await page.evaluate(...) completes (refer to the page.evaluate invocation,
the syncMessage argument, and the logger variable) so logging runs outside the
browser-evaluated function.
- Around line 791-799: The code calls logger.info(...) inside
session.page.evaluate (browser context) which will fail because logger isn't
available in the page scope; update the logic in widget-session-manager.ts so
page.evaluate only posts the message and returns any needed status or payload
(use session.page.evaluate((message)=>{...; return true}, syncMessage) or
similar), then call logger.info(...) in the Node context after await
session.page.evaluate resolves (reference the symbols session.page.evaluate,
syncMessage, and logger) so logging happens outside the browser sandbox;
alternatively, pass only serializable data back from the page and log that from
the surrounding async function.
- Around line 732-765: The browser-evaluated function passed to
session.page.evaluate uses the Node logger (logger.info) which is undefined in
the page context; replace those calls so logging happens correctly: inside the
callback (the function passed to session.page.evaluate) remove or replace
logger.info(...) with console.info(...) (or collect the log messages into a
returned object) and then, if you need Node-side logging with the existing
logger, return the details from session.page.evaluate and call logger.info(...)
in the outer scope after the await; update the function that references method,
params, storeOnHost, isHostContextUpdate, iframe ("widget-frame") and
__mcpHostContextUpdates accordingly so no direct use of logger occurs inside the
browser context.
---
Nitpick comments:
In `@packages/inspector/src/oauth/wellknown-proxy.ts`:
- Around line 81-85: The inline prefix "[oauth:wellknown-proxy]" in the log()
helper duplicates the logger's own source tag; update the log function
(log(message: string)) to stop prepending that prefix and call
logger.info(message) when debug is true so messages are not double-tagged.
| // Lightweight scoped logger (mirrors inspector's createLogger, avoids cross-package dep) | ||
| const evaluatorLogger = (() => { | ||
| const source = "testing:evaluator"; | ||
| const ts = () => new Date().toISOString(); | ||
| const LEVELS: Record<string, number> = { debug: 0, info: 1, warn: 2, error: 3, silent: 4 }; | ||
| const envLevel = (process.env.MCP_APPS_LOG_LEVEL ?? "").toLowerCase(); | ||
| const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info | ||
| const ok = (l: string): boolean => (LEVELS[l] ?? 1) >= threshold; | ||
| return { | ||
| // eslint-disable-next-line no-console | ||
| warn: (...args: unknown[]) => { | ||
| if (ok("warn")) console.warn(`${ts()} [WARN] [${source}]`, ...args); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing logger helpers in packages/testing or shared debug utilities
rg -n "createLogger|logger|logLevel|MCP_APPS_LOG_LEVEL" packages/testing src packages -g'*.ts' -g'*.tsx' --max-count=100Repository: AndurilCode/mcp-apps-kit
Length of output: 50441
🏁 Script executed:
#!/bin/bash
# Examine the packages/testing/src/debug/logger.ts file
head -100 packages/testing/src/debug/logger.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1898
🏁 Script executed:
#!/bin/bash
# Check the evaluator.ts file structure and usage around lines 789-791
sed -n '785,795p' packages/testing/src/eval/mcp/evaluator.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 377
Use the shared llmLogger instead of creating an inline evaluatorLogger with eslint-disable.
The packages/testing/src/debug/logger.ts module already exports an llmLogger specifically for LLM evaluation logging. Replace the inline evaluatorLogger (lines 37–50) with this existing utility to avoid reinventing the logging pattern and eliminate the eslint-disable suppression. Both are in the same package, so there's no cross-package dependency concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/testing/src/eval/mcp/evaluator.ts` around lines 37 - 50, Replace the
custom inline evaluatorLogger with the shared llmLogger: remove the
evaluatorLogger IIFE (including the eslint-disable comment) and instead import
the exported llmLogger from the package's debug/logger module, then use
llmLogger.warn(...) wherever evaluatorLogger.warn(...) was used; ensure any
timestamp/source formatting is preserved by using llmLogger's API rather than
reimplementing logging logic.
1 participant
Kaizen — 2026-02-20
What: Removes an
eslint-disable-next-line @typescript-eslint/no-unused-varscomment inPrimitiveDetail.tsxin favour of the idiomatic underscore-prefix convention.Before:
After:
Why: The project's ESLint config already has
varsIgnorePattern: '^_'set. Using the underscore-prefix signals intent clearly —kindis destructured out on purpose to exclude it fromprimitiveDatapassed toCopyJsonButton— without needing an inline suppression comment.Risk: None. No logic change. Build ✅ Tests ✅ (1669/1669 passing).