kaizen: remove eslint-disable for no-unused-vars in PrimitiveDetail#166
kaizen: remove eslint-disable for no-unused-vars in PrimitiveDetail#166gabrypavanello 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)
…iveDetail Rename destructured `kind` to `_kind` to leverage the existing varsIgnorePattern (^_) in eslint config, eliminating the suppression comment.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces a structured, environment-configurable logging system with level-based filtering and timestamp formatting. It adds a new logger module, exposes public logging utilities, and migrates all console-based logging across multiple packages to use the standardized logger instead. 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 Review: PR #166 - Kaizen: Remove eslint-disable for no-unused-vars in PrimitiveDetailSummaryThis PR introduces a structured logging system across the codebase and makes a clean code improvement by eliminating an unnecessary ESLint suppression comment. The overall implementation is solid and aligns well with the repository's standards. ✅ Strengths1. Excellent Structured Logging Design
2. Comprehensive Test Coverage
3. Consistent Migration Pattern
4. Clean Code Win (PrimitiveDetail.tsx)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/inspector/src/session/session-renderer.ts (3)
206-217:⚠️ Potential issue | 🟠 MajorUse
console.infoinstead oflogger.infoinsidepage.evaluate(browser context).The
loggerobject is defined at the module level in Node.js and is not accessible withinpage.evaluate, which runs in the browser context. Callinglogger.info()will throw aReferenceError, breaking the host-context update. Useconsole.infoinstead.Proposed fix
- logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); + console.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx);
282-332:⚠️ Potential issue | 🟠 MajorReplace
logger.info()calls withconsole.info()insidepage.evaluate().The
loggerobject is defined in server-side code and is not available in the browser context wherepage.evaluate()executes. Attempting to calllogger.info()will throw a ReferenceError. Useconsole.info()instead, which is always available in browser context.Proposed fix (use console in page context)
- 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);
250-256:⚠️ Potential issue | 🟠 MajorUse
console.info()instead oflogger.info()in browser context.The
loggerobject is scoped to the Node.js renderer process and is not available withinpage.evaluate(), which executes code in an isolated browser context. This will throw aReferenceErrorat runtime.Proposed fix
- logger.info("[OpenAI Host] Sent globals sync:", message.data); + console.info("[OpenAI Host] Sent globals sync:", message.data);Note: This pattern occurs multiple times throughout this file (at least 5 other instances). Consider a broader audit to replace all
loggercalls withinpage.evaluate()blocks withconsolemethods.packages/inspector/src/hosts/mcp-host.ts (1)
220-281:⚠️ Potential issue | 🟠 MajorReplace
loggercalls withconsolein Playwright init script.The init script executes in the browser page context where
loggeris undefined, causing runtime errors. Useconsole.info()instead.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);
🤖 Fix all issues with AI agents
In `@packages/inspector/src/hosts/openai-host.ts`:
- Line 248: Inside the template returned by getPlaywrightInitScript(), replace
the page-context log call that uses logger.info(...) with console.info(...) so
requestDisplayMode() will not throw in the browser page; update any occurrences
inside the getPlaywrightInitScript() string (including the requestDisplayMode
function body) to use console.info and ensure no other references to the
module-scoped logger remain in that injected script.
In `@packages/inspector/src/widget-session-manager.ts`:
- Around line 412-413: The logger.info calls inside page.evaluate are running in
the browser VM and will throw ReferenceError; locate each page.evaluate(...)
invocation in widget-session-manager.ts (the calls that currently contain
logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx) and
the other similar messages at the affected locations) and move the
logger.info(...) out of the evaluate callback to run in Node context instead —
modify the evaluate functions to return a success flag or result (e.g.,
true/false or the ctx) and then call logger.info(...) after await
page.evaluate(...) using that returned value; ensure all occurrences listed (the
blocks around the shown log strings) follow this pattern so no logger is
referenced inside the page.evaluate scope.
In `@packages/ui-react/src/hooks.ts`:
- Around line 22-33: The eslint disable is placed on the wrong line and
`process` is used as a free global; update the uiReactLogger initialization to
check for process via globalThis (e.g., guard with typeof globalThis.process !==
"undefined") and derive envLevel from globalThis.process.env safely, remove the
incorrect eslint-disable comment, and keep the rest of the logic in
uiReactLogger (references: uiReactLogger, envLevel, LEVELS, threshold, ok, ts)
so ESLint no-undef no longer flags `process`.
| })); | ||
|
|
||
| console.log('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | ||
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and the specific file mentioned
fd -t f "openai-host.ts" packages/inspector/src/Repository: AndurilCode/mcp-apps-kit
Length of output: 111
🏁 Script executed:
# Read the file to understand the context around line 248
wc -l packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 115
🏁 Script executed:
# Read the file with context around line 248
sed -n '240,260p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 640
🏁 Script executed:
# Read a broader context to understand the function structure
sed -n '220,280p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2157
🏁 Script executed:
# Check the imports at the top of the file
head -30 packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 795
🏁 Script executed:
# Look for more context - see the broader structure and how this object is used
sed -n '180,220p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1247
🏁 Script executed:
# Check if there's any Playwright or page context setup
grep -n "page\|context\|addInitScript\|evaluate" packages/inspector/src/hosts/openai-host.ts | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 141
🏁 Script executed:
# Look for where this object is being used/returned
grep -n "requestDisplayMode\|window\.__openaiEmulator" packages/inspector/src/hosts/openai-host.ts | head -20Repository: AndurilCode/mcp-apps-kit
Length of output: 1390
🏁 Script executed:
# Look at the function that generates the init script
sed -n '95,155p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2615
🏁 Script executed:
# Search for how this script is being injected into the page
grep -n "addInitScript\|evaluateHandle\|evaluate\|JSON.stringify" packages/inspector/src/hosts/openai-host.ts | head -30Repository: AndurilCode/mcp-apps-kit
Length of output: 1150
🏁 Script executed:
# Let's see the return statement and how the script string is constructed
sed -n '128,160p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 1238
🏁 Script executed:
# Check the end of the function to see how the script is returned
sed -n '295,310p' packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 654
🏁 Script executed:
# Check where this init script is actually used/injected
grep -n "getPlaywrightInitScript\|addInitScript" packages/inspector/src/hosts/openai-host.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 183
🏁 Script executed:
# Also search in other files to see how this is being used
rg "getPlaywrightInitScript\|addInitScript" packages/inspector/src/ -B 2 -A 2Repository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
# Let's verify that logger is indeed a Node.js logger not available in page context
sed -n '1,25p' packages/inspector/src/hosts/openai-host.ts | grep -A 2 "createLogger"Repository: AndurilCode/mcp-apps-kit
Length of output: 215
🏁 Script executed:
# Let's confirm the exact structure - check if logger.info is within the template literal string
sed -n '128,310p' packages/inspector/src/hosts/openai-host.ts | grep -n "logger\|console" -B 2 -A 2Repository: AndurilCode/mcp-apps-kit
Length of output: 240
🏁 Script executed:
# Check the debug/logger module to understand what createLogger does
cat packages/inspector/src/debug/logger.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 2974
🏁 Script executed:
# Let me verify the exact line numbers and structure
# Show the full template literal to understand the scope
sed -n '128,310p' packages/inspector/src/hosts/openai-host.ts | head -100Repository: AndurilCode/mcp-apps-kit
Length of output: 3191
🏁 Script executed:
# Count backticks to verify we're in a template literal
sed -n '128,310p' packages/inspector/src/hosts/openai-host.ts | grep -c '`'Repository: AndurilCode/mcp-apps-kit
Length of output: 69
🏁 Script executed:
# Search for where the opening backtick is and closing backtick
sed -n '128,310p' packages/inspector/src/hosts/openai-host.ts | tail -20Repository: AndurilCode/mcp-apps-kit
Length of output: 761
🏁 Script executed:
# Let's check if there are other logger calls within the template literal
sed -n '128,305p' packages/inspector/src/hosts/openai-host.ts | grep "logger\."Repository: AndurilCode/mcp-apps-kit
Length of output: 154
🏁 Script executed:
# Check if logger is used anywhere else in the init script string
sed -n '128,310p' packages/inspector/src/hosts/openai-host.ts | grep -n "logger" -B 1 -A 1Repository: AndurilCode/mcp-apps-kit
Length of output: 202
Replace logger.info() with console.info() in page context.
The getPlaywrightInitScript() method returns a template literal string that will be injected into the browser page via Playwright. The logger variable is not available in the page context—it's defined only in the Node.js module scope. When requestDisplayMode() is called, this will throw ReferenceError: logger is not defined and break the emulator flow. Use console.info() instead, which is natively available in the page.
Proposed fix
- logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);
+ console.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);📝 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.
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| console.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); |
🤖 Prompt for AI Agents
In `@packages/inspector/src/hosts/openai-host.ts` at line 248, Inside the template
returned by getPlaywrightInitScript(), replace the page-context log call that
uses logger.info(...) with console.info(...) so requestDisplayMode() will not
throw in the browser page; update any occurrences inside the
getPlaywrightInitScript() string (including the requestDisplayMode function
body) to use console.info and ensure no other references to the module-scoped
logger remain in that injected script.
| logger.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Playwright page.evaluate execution context Node variables access browser
💡 Result:
page.evaluate() runs inside the browser page’s JavaScript VM, while your Playwright test code runs in Node.js. These environments don’t intersect, so variables/functions from Node (e.g., const x = ...) are not directly accessible inside evaluate() unless you explicitly send them in. (playwright.dev)
1) Pass Node values into the page via the argument
const data = { startDate: '2026-02-11' };
await page.evaluate(({ startDate }) => {
// runs in the browser
window.localStorage.setItem('startDate', startDate);
}, data);Playwright evaluate() takes a single optional argument (serializable values and/or handles). (playwright.dev)
(Stack Overflow explanation of the same pitfall + fix.) (stackoverflow.com)
2) If you need to call back into Node from the page, expose a function/binding
Use exposeFunction/exposeBinding to install a function on window that executes in Node when called from the page. (playwright.dev)
await page.context().exposeFunction('sha256', (text: string) => /* node code */ text);
await page.evaluate(async () => {
const hash = await (window as any).sha256('hello'); // calls Node
return hash;
});If you tell me your exact goal (read a Node variable? call a Node function? access filesystem/env?), I can suggest the cleanest pattern.
Citations:
- 1: https://playwright.dev/docs/next/evaluating?utm_source=openai
- 2: https://playwright.dev/docs/next/evaluating?utm_source=openai
- 3: https://stackoverflow.com/questions/70263320/pass-parameters-to-inner-arrow-function-used-with-playwrights-evaluate?utm_source=openai
- 4: https://playwright.dev/docs/api/class-browsercontext?utm_source=openai
🏁 Script executed:
wc -l packages/inspector/src/widget-session-manager.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 121
🏁 Script executed:
cat -n packages/inspector/src/widget-session-manager.ts | sed -n '400,430p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1699
🏁 Script executed:
cat -n packages/inspector/src/widget-session-manager.ts | sed -n '430,460p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1286
🏁 Script executed:
cat -n packages/inspector/src/widget-session-manager.ts | sed -n '735,775p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1773
🏁 Script executed:
cat -n packages/inspector/src/widget-session-manager.ts | sed -n '785,810p'Repository: AndurilCode/mcp-apps-kit
Length of output: 1281
Remove logger.info() calls from inside page.evaluate() blocks — they will throw ReferenceError in browser context.
page.evaluate() executes JavaScript in the browser page VM, where Node-side variables like logger are undefined. Move logging outside the evaluate blocks, optionally returning a boolean to signal whether the operation succeeded.
Affected locations: 412-413, 442-443, 747-748, 760-761, 796-797
✅ Recommended pattern
- await page.evaluate((ctx: typeof hostContext) => {
+ const didPost = 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);
+ return true;
}
+ return false;
}, hostContext);
+ if (this.debug && didPost) {
+ logger.info("[MCP Host] Sent ui/notifications/host-context-changed", hostContext);
+ }🤖 Prompt for AI Agents
In `@packages/inspector/src/widget-session-manager.ts` around lines 412 - 413, The
logger.info calls inside page.evaluate are running in the browser VM and will
throw ReferenceError; locate each page.evaluate(...) invocation in
widget-session-manager.ts (the calls that currently contain logger.info("[MCP
Host] Sent ui/notifications/host-context-changed", ctx) and the other similar
messages at the affected locations) and move the logger.info(...) out of the
evaluate callback to run in Node context instead — modify the evaluate functions
to return a success flag or result (e.g., true/false or the ctx) and then call
logger.info(...) after await page.evaluate(...) using that returned value;
ensure all occurrences listed (the blocks around the shown log strings) follow
this pattern so no logger is referenced inside the page.evaluate scope.
| // Lightweight scoped logger (mirrors inspector's createLogger, avoids cross-package dep) | ||
| const uiReactLogger = (() => { | ||
| const source = "ui-react:hooks"; | ||
| const ts = () => new Date().toISOString(); | ||
| const LEVELS: Record<string, number> = { debug: 0, info: 1, warn: 2, error: 3, silent: 4 }; | ||
| // eslint-disable-next-line no-undef, @typescript-eslint/no-unnecessary-condition | ||
| const envLevel = | ||
| typeof process !== "undefined" | ||
| ? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase() | ||
| : ""; | ||
| const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info | ||
| const ok = (l: string): boolean => (LEVELS[l] ?? 1) >= threshold; |
There was a problem hiding this comment.
Fix no-undef on process (disable is on the wrong line).
Line 27 disables only the next line, but process is referenced on Line 29–30, so ESLint can still error. Consider avoiding process as a free global via globalThis and drop the disable.
✅ Suggested fix (no `any`, no eslint-disable)
- // eslint-disable-next-line no-undef, `@typescript-eslint/no-unnecessary-condition`
- const envLevel =
- typeof process !== "undefined"
- ? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase()
- : "";
+ const processEnv =
+ typeof globalThis !== "undefined" && "process" in globalThis
+ ? (
+ globalThis as {
+ process?: { env?: Record<string, string | undefined> };
+ }
+ ).process?.env
+ : undefined;
+ const envLevel = (processEnv?.MCP_APPS_LOG_LEVEL ?? "").toLowerCase();📝 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.
| // Lightweight scoped logger (mirrors inspector's createLogger, avoids cross-package dep) | |
| const uiReactLogger = (() => { | |
| const source = "ui-react:hooks"; | |
| const ts = () => new Date().toISOString(); | |
| const LEVELS: Record<string, number> = { debug: 0, info: 1, warn: 2, error: 3, silent: 4 }; | |
| // eslint-disable-next-line no-undef, @typescript-eslint/no-unnecessary-condition | |
| const envLevel = | |
| typeof process !== "undefined" | |
| ? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase() | |
| : ""; | |
| const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info | |
| const ok = (l: string): boolean => (LEVELS[l] ?? 1) >= threshold; | |
| // Lightweight scoped logger (mirrors inspector's createLogger, avoids cross-package dep) | |
| const uiReactLogger = (() => { | |
| const source = "ui-react:hooks"; | |
| const ts = () => new Date().toISOString(); | |
| const LEVELS: Record<string, number> = { debug: 0, info: 1, warn: 2, error: 3, silent: 4 }; | |
| const processEnv = | |
| typeof globalThis !== "undefined" && "process" in globalThis | |
| ? ( | |
| globalThis as { | |
| process?: { env?: Record<string, string | undefined> }; | |
| } | |
| ).process?.env | |
| : undefined; | |
| const envLevel = (processEnv?.MCP_APPS_LOG_LEVEL ?? "").toLowerCase(); | |
| const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info | |
| const ok = (l: string): boolean => (LEVELS[l] ?? 1) >= threshold; |
🧰 Tools
🪛 ESLint
[error] 30-30: 'process' is not defined.
(no-undef)
🤖 Prompt for AI Agents
In `@packages/ui-react/src/hooks.ts` around lines 22 - 33, The eslint disable is
placed on the wrong line and `process` is used as a free global; update the
uiReactLogger initialization to check for process via globalThis (e.g., guard
with typeof globalThis.process !== "undefined") and derive envLevel from
globalThis.process.env safely, remove the incorrect eslint-disable comment, and
keep the rest of the logic in uiReactLogger (references: uiReactLogger,
envLevel, LEVELS, threshold, ok, ts) so ESLint no-undef no longer flags
`process`.
1 participant
Rename destructured
kindto_kindto leverage the existingvarsIgnorePattern: '^_'in eslint config, removing an unnecessary suppression comment.File:
packages/inspector/src/dashboard/react/components/PrimitiveDetail.tsxBuild ✅ | Tests ✅ (74/74)