chore(codegen): remove unused deprecated type aliases#168
chore(codegen): remove unused deprecated type aliases#168gabrypavanello 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)
Remove FileBasedGlobalConfig and IconConfig deprecated aliases from codegen types and their re-export from the package index. These aliases were marked @deprecated pointing to GlobalConfig and Icon from @mcp-apps-kit/core, but no consumer in the codebase uses them. Removing them also eliminates one eslint-disable comment for @typescript-eslint/no-deprecated.
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a structured, environment-configurable logging system throughout the inspector and codegen packages, replacing ad-hoc console logging with a centralized logger module. Adds logger integration across 15+ modules, removes deprecated type exports from codegen, and includes comprehensive test coverage for the new logging infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: PR #168 - Remove unused deprecated type aliases + Logging improvements📊 SummaryOverall Assessment: ✅ APPROVE with minor suggestions This is a well-executed cleanup PR that achieves two goals:
Stats: 25 files changed (+453/-203 lines) ✅ Strengths1. Clean Architecture
2. Systematic Migration
3. Good Test Coverage
4. Performance Benefits
|
![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 (1)
206-219:⚠️ Potential issue | 🔴 Critical
loggeris undefined in browser context - runtime error.The
logger.info()call on line 216 is inside apage.evaluate()callback, which executes in the browser's JavaScript context, not Node.js. The module-scopedloggervariable doesn't exist there, causing aReferenceErrorat runtime.The same issue occurs at lines 255, 286, 303, 312, and 331 in this file.
🐛 Proposed fix: remove logger calls from browser context or use console directly
/* 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); + // Note: Cannot use Node.js logger here - this runs in browser context + console.info("[MCP Host] Sent ui/notifications/host-context-changed", ctx); } }, hostContext); /* eslint-enable no-undef */Apply similar fixes to all other
page.evaluate()blocks in this file.packages/inspector/src/widget-session-manager.ts (1)
400-415:⚠️ Potential issue | 🔴 Critical
loggeris undefined in browser context - runtime error.Line 412 uses
logger.info()insidepage.evaluate(), which executes in the browser context where the Node.jsloggervariable doesn't exist. This will throw aReferenceErrorat runtime.The same issue occurs at lines 442, 747, 760, and 796 in this file.
🐛 Proposed fix: use console.info in browser context
/* eslint-disable no-undef */ await page.evaluate((ctx: typeof hostContext) => { const iframe = document.getElementById("widget-frame") as HTMLIFrameElement | null; if (iframe?.contentWindow) { // Use the correct MCP Apps protocol method name // Wrap context in hostContext to match ext-apps SDK format 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 */Apply similar fixes to all other
page.evaluate()blocks.packages/inspector/src/hosts/mcp-host.ts (1)
220-281:⚠️ Potential issue | 🟠 MajorPlaywright init script will throw because
loggeris undefined in the browser context.Those
logger.info(...)calls are inside the string executed in the page;loggeronly exists in Node. This will raise aReferenceErrorand can break the emulator bootstrap.🛠️ Suggested fix: use console inside the injected script
- 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);packages/inspector/src/proxy-tools.ts (1)
38-103:⚠️ Potential issue | 🟡 MinorAvoid overwriting an existing
reasoningproperty from the target schema.If the target schema already defines
reasoning(type or requiredness), this currently overwrites it with an optional string, which can desync validation from the target.✅ Safe guard to keep existing schema
- // Add reasoning field for agent view transparency - shape.reasoning = reasoningField; + // Add reasoning field for agent view transparency (only if not already defined) + if (!("reasoning" in shape)) { + shape.reasoning = reasoningField; + }
🤖 Fix all issues with AI agents
In `@packages/inspector/src/hosts/openai-host.ts`:
- Around line 246-250: The injected Playwright init script uses Node's logger
(logger.info) inside the browser context causing a ReferenceError; update the
script code that defines requestDisplayMode (the function passed into
addInitScript / the inline script block in openai-host.ts) to use console.info
(or remove logging) instead of logger.info so it runs in the browser environment
without referencing the Node-only logger.
In `@packages/inspector/tests/logger.test.ts`:
- Around line 99-106: The test is flaky because it relies on the environment log
level; before calling createLogger("s") set process.env.MCP_APPS_LOG_LEVEL =
"warn" (or a level that allows warnings), then create the logger and run
assertions, and finally restore the original environment value (save const prev
= process.env.MCP_APPS_LOG_LEVEL; ... process.env.MCP_APPS_LOG_LEVEL = prev) so
other tests are unaffected; update the test around createLogger in
logger.test.ts to do this setup/teardown to make the test deterministic.
In `@packages/ui-react/src/hooks.ts`:
- Around line 26-32: The eslint-disable comment is placed before LEVELS but
needs to target the lines that reference process to suppress no-undef and
no-unnecessary-condition; move the comment so it immediately precedes the
envLevel declaration (the code that reads process.env?.MCP_APPS_LOG_LEVEL) or
replace the line comment with a block disable that wraps the envLevel
assignment; ensure you still limit the disable to that specific expression and
keep LEVELS and threshold unchanged (references: LEVELS, envLevel, threshold,
process).
🧹 Nitpick comments (1)
packages/inspector/src/oauth/wellknown-proxy.ts (1)
81-85: Redundant prefix in log message.The
createLogger("wellknown-proxy")already includes[wellknown-proxy]in every log line. The additional[oauth:wellknown-proxy]prefix in the message creates redundant output like:
<timestamp> [INFO] [wellknown-proxy] [oauth:wellknown-proxy] ...Consider removing the inline prefix for cleaner logs, or changing the logger source to
"oauth:wellknown-proxy"and removing the inline prefix.♻️ Suggested fix
-const logger = createLogger("wellknown-proxy"); +const logger = createLogger("oauth:wellknown-proxy");And then:
function log(message: string): void { if (debug) { - logger.info(`[oauth:wellknown-proxy] ${message}`); + logger.info(message); } }
| })); | ||
|
|
||
| console.log('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | ||
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | ||
| return { mode: mode }; | ||
| }, |
There was a problem hiding this comment.
logger is undefined in browser context - runtime error.
Line 248 uses logger.info() inside the Playwright init script string, which executes in the browser's JavaScript context when injected via addInitScript(). The Node.js logger variable doesn't exist there, causing a ReferenceError when requestDisplayMode is called.
🐛 Proposed fix: use console.info in browser script
// Dispatch set_globals event with updated sizing
window.dispatchEvent(new CustomEvent('openai:set_globals', {
detail: {
globals: {
displayMode: mode,
maxHeight: sizing.maxHeight,
viewport: window.__openaiEmulator.viewport,
},
},
}));
- logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);
+ console.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing);
return { mode: mode };
},📝 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.
| })); | |
| console.log('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| logger.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| return { mode: mode }; | |
| }, | |
| })); | |
| console.info('[OpenAI Host] requestDisplayMode:', mode, 'sizing:', sizing); | |
| return { mode: mode }; | |
| }, |
🤖 Prompt for AI Agents
In `@packages/inspector/src/hosts/openai-host.ts` around lines 246 - 250, The
injected Playwright init script uses Node's logger (logger.info) inside the
browser context causing a ReferenceError; update the script code that defines
requestDisplayMode (the function passed into addInitScript / the inline script
block in openai-host.ts) to use console.info (or remove logging) instead of
logger.info so it runs in the browser environment without referencing the
Node-only logger.
| it("passes additional arguments through", () => { | ||
| const logger = createLogger("s"); | ||
| const obj = { foo: 1 }; | ||
| logger.warn("msg", obj, 42); | ||
| const args = (console.warn as ReturnType<typeof vi.fn>).mock.calls[0]; | ||
| expect(args[1]).toBe("msg"); | ||
| expect(args[2]).toBe(obj); | ||
| expect(args[3]).toBe(42); |
There was a problem hiding this comment.
Test can flake if MCP_APPS_LOG_LEVEL is set in the environment.
This test expects warn to emit, but doesn’t set the env; if CI sets the level to error/silent, the assertion will fail.
✅ Make the test deterministic
it("passes additional arguments through", () => {
+ process.env.MCP_APPS_LOG_LEVEL = "warn";
const logger = createLogger("s");
const obj = { foo: 1 };📝 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.
| it("passes additional arguments through", () => { | |
| const logger = createLogger("s"); | |
| const obj = { foo: 1 }; | |
| logger.warn("msg", obj, 42); | |
| const args = (console.warn as ReturnType<typeof vi.fn>).mock.calls[0]; | |
| expect(args[1]).toBe("msg"); | |
| expect(args[2]).toBe(obj); | |
| expect(args[3]).toBe(42); | |
| it("passes additional arguments through", () => { | |
| process.env.MCP_APPS_LOG_LEVEL = "warn"; | |
| const logger = createLogger("s"); | |
| const obj = { foo: 1 }; | |
| logger.warn("msg", obj, 42); | |
| const args = (console.warn as ReturnType<typeof vi.fn>).mock.calls[0]; | |
| expect(args[1]).toBe("msg"); | |
| expect(args[2]).toBe(obj); | |
| expect(args[3]).toBe(42); |
🤖 Prompt for AI Agents
In `@packages/inspector/tests/logger.test.ts` around lines 99 - 106, The test is
flaky because it relies on the environment log level; before calling
createLogger("s") set process.env.MCP_APPS_LOG_LEVEL = "warn" (or a level that
allows warnings), then create the logger and run assertions, and finally restore
the original environment value (save const prev =
process.env.MCP_APPS_LOG_LEVEL; ... process.env.MCP_APPS_LOG_LEVEL = prev) so
other tests are unaffected; update the test around createLogger in
logger.test.ts to do this setup/teardown to make the test deterministic.
| 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 |
There was a problem hiding this comment.
Misplaced eslint-disable comment causes static analysis error.
The eslint-disable comment on line 27 is placed before the LEVELS constant, but the process reference that triggers the no-undef error is on lines 29-30. The comment should cover the lines that actually access process.
🐛 Suggested fix
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 =
+ // eslint-disable-next-line no-undef
typeof process !== "undefined"
+ // eslint-disable-next-line no-undef
? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase()
: "";Or use a block disable:
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`
+ /* eslint-disable no-undef */
const envLevel =
typeof process !== "undefined"
? ((process.env?.MCP_APPS_LOG_LEVEL as string | undefined) ?? "").toLowerCase()
: "";
+ /* eslint-enable no-undef */
const threshold: number = LEVELS[envLevel] ?? 1; // 1 = info🧰 Tools
🪛 ESLint
[error] 30-30: 'process' is not defined.
(no-undef)
🤖 Prompt for AI Agents
In `@packages/ui-react/src/hooks.ts` around lines 26 - 32, The eslint-disable
comment is placed before LEVELS but needs to target the lines that reference
process to suppress no-undef and no-unnecessary-condition; move the comment so
it immediately precedes the envLevel declaration (the code that reads
process.env?.MCP_APPS_LOG_LEVEL) or replace the line comment with a block
disable that wraps the envLevel assignment; ensure you still limit the disable
to that specific expression and keep LEVELS and threshold unchanged (references:
LEVELS, envLevel, threshold, process).
1 participant
Kaizen 2026-02-13 — Remove dead code.
FileBasedGlobalConfigandIconConfigwere deprecated aliases in@mcp-apps-kit/codegenpointing toGlobalConfigandIconfrom core. No consumer uses them. Removing them eliminates 2 dead type aliases and 1 eslint-disable comment.packages/codegen/src/types.tspackages/codegen/src/index.ts