Auto-resize-mcp-apps - #72
Conversation
gabe4coding
commented
Jan 9, 2026
gabe4coding
commented
- Updated McpAdapterOptions from an interface to a type definition for improved clarity and consistency in type usage.
- Adjusted the syntax to use a type alias, enhancing readability and aligning with modern TypeScript practices.
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds an optional autoResize flag and threads it from UI definitions through builder/entry generation into AppsProvider/createClient/createAdapter and the MCP adapter to control automatic size-change notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant Entry as Generated EntryPoint
participant Provider as AppsProvider
participant ClientFactory as createClient
participant AdapterFactory as createAdapter
participant Mcp as McpAdapter
participant App as MCP App (runtime)
Entry->>Provider: render <AppsProvider autoResize=...>
Provider->>ClientFactory: createClient({ autoResize })
ClientFactory->>AdapterFactory: createAdapter(protocol, { mcp: { autoResize } })
AdapterFactory->>Mcp: new McpAdapter({ autoResize })
Mcp->>App: connect(..., autoResize)
App->>App: (if autoResize) observe body/documentElement size -> send size events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Comment |
d27483e to
120f5ed
Compare
120f5ed to
2f763d6
Compare
…mponents - Introduced `autoResize` option in MCP adapter to enable or disable automatic size change notifications. - Updated `createAdapter` function to accept adapter-specific options, including `autoResize`. - Enhanced `AppsProvider` and related components to support passing the `autoResize` prop. - Updated documentation and tests to reflect the new functionality and ensure proper integration.
…nforce fixed width - Removed the `autoResize` option from both Greeting Widget V1 and V2 to standardize behavior. - Updated CSS to set a fixed width of 432px for the body, preventing layout shifts and ensuring consistent presentation across different screen sizes.
…tion - Updated McpAdapterOptions from an interface to a type definition for improved clarity and consistency in type usage. - Adjusted the syntax to use a type alias, enhancing readability and aligning with modern TypeScript practices.
…plugin files - Streamlined destructuring of options in `generateEntryPoint` and `buildDiscoveredUIs` functions for improved readability. - Removed unnecessary line breaks to enhance code clarity and maintainability.
2f763d6 to
7c07b52
Compare
Pull Request Review: Auto-resize MCP AppsOverviewThis PR adds an ✅ Strengths
🔍 Issues & Recommendations1. Missing Test Coverage (High Priority)The PR lacks tests for the core functionality in critical areas: packages/ui/tests/unit/mcp-adapter.test.ts: // Missing tests for:
describe('McpAdapter with autoResize option', () => {
it('should pass autoResize option to App constructor', async () => {
const adapter = new McpAdapter({ autoResize: false });
await adapter.connect();
// Verify that App was instantiated with { autoResize: false }
});
it('should default autoResize to true when not specified', async () => {
const adapter = new McpAdapter();
await adapter.connect();
// Verify that App was instantiated with { autoResize: true }
});
});packages/ui/tests/unit/detection.test.ts (or new integration test): // Missing tests for createClient with autoResize option
describe('createClient with autoResize', () => {
it('should pass autoResize to MCP adapter', async () => {
const client = await createClient({
forceAdapter: 'mcp',
autoResize: false
});
// Verify adapter received the option
});
});packages/ui-react/tests/unit/context.test.tsx: // Missing tests for AppsProvider with autoResize
describe('AppsProvider with autoResize', () => {
it('should pass autoResize to createClient', async () => {
render(
<AppsProvider forceAdapter="mock" autoResize={false}>
<TestComponent />
</AppsProvider>
);
// Verify the option is passed through
});
});packages/ui-react-builder/tests/unit/ast-parser.test.ts: it('should extract autoResize property from defineReactUI', async () => {
const content = `
import { defineReactUI } from "@mcp-apps-kit/ui-react-builder";
import { Widget } from "./Widget";
defineReactUI({
component: Widget,
name: "Widget",
autoResize: false,
});
`;
const results = await parseReactUIDefinitions(content);
expect(results[0].autoResize).toBe(false);
});2. Type Safety Issue (Medium Priority)In packages/ui/src/index.ts:183-189, the adapter options construction could be improved: // Current implementation:
const adapterOptions: AdapterOptions = {
mcp: {
autoResize: options?.autoResize, // Could be undefined
},
};Issue: This always creates an object with Recommendation: Only include the option when explicitly provided: const adapterOptions: AdapterOptions = {};
if (options?.autoResize !== undefined) {
adapterOptions.mcp = { autoResize: options.autoResize };
}Or more cleanly: const adapterOptions: AdapterOptions = options?.autoResize !== undefined
? { mcp: { autoResize: options.autoResize } }
: {};3. Documentation Gap (Low Priority)The PR description mentions changing
4. Example CSS Hard-Coding (Low Priority)In examples/minimal/src/ui/styles.css:13-15, the width is hard-coded to prevent auto-resize width changes: /* Fix width to prevent auto-resize width changes */
width: 432px; /* 400px content + 32px padding */
min-width: 432px;Question: Is this the recommended pattern when 🔐 Security ConsiderationsNo security concerns identified. The feature is purely client-side configuration. ⚡ Performance ConsiderationsThe implementation looks efficient. The
However, consider documenting:
📊 Code QualityPositive:
Minor Issues:
✅ Checklist Before Merge
📝 Overall AssessmentThis is a well-implemented feature that follows the project's conventions and maintains backward compatibility. The main concern is insufficient test coverage for the core functionality. Once tests are added and the minor type safety issue is addressed, this will be ready to merge. Recommendation: Request changes to add comprehensive test coverage before merging. Additional Questions
Great work on maintaining consistency across the codebase! 🎉 |
Code Review: Auto-resize MCP AppsSummaryThis PR adds an autoResize option to control automatic size change notifications for MCP Apps. The implementation is well-structured and follows the framework's patterns, but there are several concerns that need to be addressed. Critical Issues1. CSS-Based Width Constraint in Minimal ExampleFile: examples/minimal/src/ui/styles.css:13-15 The hardcoded fixed width defeats the purpose of auto-resize. This is confusing and potentially harmful if users copy this pattern. Recommendation: Remove the fixed width constraint or add clear documentation explaining why it's needed. High Priority Issues2. Missing Test CoverageFiles: packages/ui/tests/unit/mcp-adapter.test.ts No tests verify that the autoResize option is correctly passed through the stack or that it defaults to true. Recommendation: Add test cases for McpAdapter constructor with/without options, and createClient with autoResize parameter. 3. Type ConsistencyFile: packages/ui/src/adapters/mcp.ts:52 McpAdapterOptions uses type instead of interface. The codebase uses interfaces extensively for similar options objects. Recommendation: Consider using interface for consistency with AppsProviderProps, CreateClientOptions, etc. Medium Priority Issues4. Missing Use Case DocumentationThe feature doesn't explain when/why developers should disable auto-resize. Recommendation: Enhance documentation with examples (fixed-size UIs, manual management, performance). 5. React Effect DependencyFile: packages/ui-react/src/context.tsx:136 If autoResize changes after mount, the effect will re-run and create a new client. Recommendation: Document that autoResize should not change during component lifetime. Positive Aspects
Verdict: Needs WorkThe implementation is structurally sound but needs:
Estimated effort: 2-3 hours Great work on maintaining type safety! 🎉 |
Code Review: Auto-resize MCP Apps FeatureThank you for this well-structured PR! This adds an important feature for controlling automatic resize behavior in MCP apps. Here's my detailed review: ✅ Strengths
🔍 Issues & Recommendations1. Interface to Type Conversion (Critical - Design Decision)File: // Changed from interface to type
export type McpAdapterOptions = {
autoResize?: boolean;
};Issue: The PR description states this change improves "clarity and consistency" and aligns with "modern TypeScript practices," but:
Recommendation:
Suggested: export interface McpAdapterOptions {
/** Enable automatic size change notifications */
autoResize?: boolean;
}2. Missing Test Coverage for MCP Adapter OptionsFile: Issue: No tests verify that the Recommendation: Add tests like: describe('autoResize option', () => {
it('should default to true when not specified', async () => {
const adapter = new McpAdapter();
await adapter.connect();
// Verify App was created with autoResize: true
});
it('should pass autoResize: false when specified', async () => {
const adapter = new McpAdapter({ autoResize: false });
await adapter.connect();
// Verify App was created with autoResize: false
});
it('should pass autoResize: true when specified', async () => {
const adapter = new McpAdapter({ autoResize: true });
await adapter.connect();
// Verify App was created with autoResize: true
});
});3. React useEffect Dependency ArrayFile: useEffect(() => {
// ...
}, [providedClient, _forceAdapter, autoResize]);Issue: While technically correct, changing Recommendation:
4. AdapterOptions Type CouplingFile: interface AdapterOptions {
/** Options specific to MCP adapter */
mcp?: McpAdapterOptions;
}Issue: This local interface isn't exported and only supports MCP options. If OpenAI adapter needs options later, this will need refactoring. Recommendation:
interface AdapterOptions {
mcp?: McpAdapterOptions;
openai?: OpenAIAdapterOptions; // Future-proof
mock?: MockAdapterOptions;
}
5. AST Parser Only Handles Literal BooleansFile: } else if (keyName === "autoResize") {
// Extract autoResize boolean
if (prop.value.type === AST_NODE_TYPES.Literal && typeof prop.value.value === "boolean") {
autoResize = prop.value.value;
}
}Issue: This only handles literal const AUTO_RESIZE = false;
defineReactUI({
component: MyWidget,
name: "Widget",
autoResize: AUTO_RESIZE // Won't be detected
});Recommendation:
6. CSS Fixed Width ApproachFile: /* Fix width to prevent auto-resize width changes */
width: 432px; /* 400px content + 32px padding */
min-width: 432px;Concerns:
Recommendation:
7. Missing Integration TestsIssue: No end-to-end test verifying the full flow:
Recommendation: Add an integration test in 8. Potential Runtime ErrorFile: this.app = new App(
{ name: "@mcp-apps-kit/ui", version: "0.0.0" },
{ tools: {} },
{ autoResize: this.autoResize }
);Question: Does the Recommendation: Verify this against the ext-apps SDK version (check package.json). If this is a newly added parameter, ensure the minimum version is documented. 🎯 Minor Issues
✅ Testing ChecklistBefore merging, please verify:
📚 Documentation Suggestions
🎬 VerdictOverall Assessment: Good implementation with minor issues to address. Blocking Issues: None (all issues are recommendations) Suggested Action:
Once these are addressed, this should be ready to merge. Great work on the consistent threading of this option through all layers! 🚀 |
Pull Request Review: Auto-resize MCP Apps FeatureOverviewThis PR adds an ✅ Strengths
🔴 Critical Issues1. Conflicting CSS in Minimal ExampleFile: /* Fix width to prevent auto-resize width changes */
width: 432px; /* 400px content + 32px padding */
min-width: 432px;Issue: This hardcoded width constraint seems to contradict the purpose of the auto-resize feature. If this is a workaround for a specific issue, it should be documented. Otherwise, it's confusing. Recommendation: Either remove these lines or add clear documentation explaining when/why fixed widths are needed with auto-resize disabled. 🟡 High Priority Issues2. Missing Test Coverage for Core FunctionalityThe new Missing in describe('McpAdapter with autoResize option', () => {
it('should pass autoResize option to App constructor', async () => {
const adapter = new McpAdapter({ autoResize: false });
await adapter.connect();
// Verify App instantiation with correct options
});
it('should default autoResize to true when not specified', async () => {
const adapter = new McpAdapter();
// Should use default true
});
});Missing in
Missing in
Missing in it('should extract autoResize property from defineReactUI', () => {
const content = `
defineReactUI({
component: Widget,
name: "Widget",
autoResize: false,
});
`;
const results = parseReactUIDefinitions(content);
expect(results[0].autoResize).toBe(false);
});Target: Project requires 80% minimum test coverage 3. Type Safety in Options ConstructionFile: const adapterOptions: AdapterOptions = {
mcp: {
autoResize: options?.autoResize, // Always sets property even when undefined
},
};Issue: This creates Recommendation: Only include when explicitly provided: const adapterOptions: AdapterOptions = options?.autoResize !== undefined
? { mcp: { autoResize: options.autoResize } }
: {};4. Unclear PR DescriptionThe PR description focuses on "Updated McpAdapterOptions from interface to type" which is an implementation detail, not the actual feature. Recommendation: Update description to explain:
🔵 Medium Priority Issues5. React Effect Dependency WarningFile: The Recommendation: Document that 6. Missing DocumentationNo documentation explains when/why developers should disable auto-resize. Recommendations:
🟢 Code QualityPositive:
Minor:
🔐 Security & PerformanceSecurity: ✅ No concerns identified - purely client-side configuration Performance: ✅ Efficient implementation
✅ Pre-Merge Checklist
📋 Overall AssessmentRating: This is a well-architected feature that follows project conventions and maintains backward compatibility. The implementation pattern is solid, but it lacks sufficient test coverage for the core functionality and has a confusing example that needs clarification. Estimated effort to address: 2-3 hours Recommendation: Request changes to add test coverage and clarify the CSS example before merging. Great work maintaining consistency across the monorepo! Once tests are added, this will be ready to ship. 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui-react-builder/src/define.ts (1)
74-90: DefaultautoResizetotrueindefineReactUI()to match documented behavior.The JSDoc for
ReactUIInput.autoResizeexplicitly states@default true, but the current code passesundefinedwhenautoResizeis not provided. Since bothautoResize?: boolean(input) and__autoResize?: boolean(internal) are optional types, this default is not enforced. Update line 89 to:- __autoResize: autoResize, + __autoResize: autoResize ?? true,
🧹 Nitpick comments (6)
examples/minimal/src/ui/styles.css (1)
13-15: Fixedbodywidth may cause horizontal overflow on narrow hostsLocking
bodytowidth/min-width: 432pxwill prevent width-driven layout shifts, but can introduce horizontal scrolling/clipping if the host viewport/container is < 432px wide.Consider a narrow-host fallback (e.g., media query) so the example remains usable in smaller containers while still defaulting to 432px where possible.
packages/ui-react/src/context.tsx (1)
59-69: Guard against stale asynccreateClient()results when deps change / unmountNow that
autoResizeparticipates in the effect dependency list, it’s easier to trigger multiple inits; consider adding anisCurrent/cleanup guard so an older promise can’t win the race and callsetClientafter a newer init (or after unmount).Proposed tweak (minimal race/unmount guard)
useEffect(() => { if (providedClient) { setClient(providedClient); setIsConnecting(false); return; } + let isCurrent = true; + // Initialize client with auto-detection or forced adapter const initClient = async () => { try { const newClient = await createClient<T>({ forceAdapter: _forceAdapter, autoResize, }); - setClient(newClient); - setIsConnecting(false); + if (!isCurrent) return; + setClient(newClient); + setIsConnecting(false); } catch (err) { - setError(err instanceof Error ? err : new Error(String(err))); - setIsConnecting(false); + if (!isCurrent) return; + setError(err instanceof Error ? err : new Error(String(err))); + setIsConnecting(false); } }; void initClient(); + return () => { + isCurrent = false; + }; }, [providedClient, _forceAdapter, autoResize]);Also applies to: 101-108, 123-136
packages/ui-react-builder/src/html.ts (1)
113-118: Consider TS overloads forgenerateEntryPoint(cleaner typing for the new options-object API)Current union signature works, but overloads would improve IntelliSense and prevent accidentally passing a second arg with the options-object form.
Example overloads (no runtime change)
export function generateEntryPoint( + componentPath: string, + defaultProps?: Record<string, unknown> +): string; +export function generateEntryPoint(options: EntryPointOptions): string; +export function generateEntryPoint( componentPathOrOptions: string | EntryPointOptions, defaultProps?: Record<string, unknown> ): string { // Handle backward compatibility with string-only signature const options: EntryPointOptions = typeof componentPathOrOptions === "string" ? { componentPath: componentPathOrOptions, defaultProps } : componentPathOrOptions;Also applies to: 151-172, 184-185
packages/ui-react-builder/src/ast-parser.ts (2)
13-22: Consider switchingParsedReactUIfromexport interface→export typeto match repo conventions.Coding guidelines request
export typefor exported type shapes in.tsfiles; this is a good candidate.
91-118: Support quoted keys (string-literal property names) when parsingautoResize(and existing keys).Right now
keyNameis only derived fromIdentifierkeys, so{ "autoResize": false }(or{ "name": "..." }) will be ignored.Proposed patch
- const key = prop.key; - const keyName = key.type === AST_NODE_TYPES.Identifier ? key.name : null; + const key = prop.key; + const keyName = + key.type === AST_NODE_TYPES.Identifier + ? key.name + : key.type === AST_NODE_TYPES.Literal && typeof key.value === "string" + ? key.value + : null; if (!keyName) continue;Also applies to: 129-134
packages/ui-react-builder/src/vite-plugin.ts (1)
190-192: Nice: conditionalAppsProviderprop injection matches the builder’s entry-point behavior.Optional follow-up: consider reusing
generateEntryPoint()here to avoid maintaining two parallel entry templates.Also applies to: 319-325, 364-380
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/minimal/src/ui/styles.csspackages/ui-react-builder/src/ast-parser.tspackages/ui-react-builder/src/build.tspackages/ui-react-builder/src/define.tspackages/ui-react-builder/src/html.tspackages/ui-react-builder/src/types.tspackages/ui-react-builder/src/vite-plugin.tspackages/ui-react-builder/tests/unit/html.test.tspackages/ui-react/src/context.tsxpackages/ui/src/adapters/mcp.tspackages/ui/src/index.tspackages/ui/src/types.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/ui-react-builder/src/types.tspackages/ui-react-builder/src/html.tspackages/ui-react-builder/tests/unit/html.test.tspackages/ui-react-builder/src/ast-parser.tspackages/ui/src/index.tspackages/ui/src/types.tspackages/ui-react-builder/src/vite-plugin.tspackages/ui-react/src/context.tsxpackages/ui-react-builder/src/build.tspackages/ui/src/adapters/mcp.tspackages/ui-react-builder/src/define.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/ui-react-builder/src/types.tspackages/ui-react-builder/src/html.tspackages/ui-react-builder/tests/unit/html.test.tspackages/ui-react-builder/src/ast-parser.tspackages/ui/src/index.tspackages/ui/src/types.tspackages/ui-react-builder/src/vite-plugin.tspackages/ui-react/src/context.tsxpackages/ui-react-builder/src/build.tspackages/ui/src/adapters/mcp.tspackages/ui-react-builder/src/define.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/ui-react-builder/src/types.tspackages/ui-react-builder/src/html.tspackages/ui-react-builder/tests/unit/html.test.tspackages/ui-react-builder/src/ast-parser.tspackages/ui/src/index.tspackages/ui/src/types.tspackages/ui-react-builder/src/vite-plugin.tspackages/ui-react-builder/src/build.tspackages/ui/src/adapters/mcp.tspackages/ui-react-builder/src/define.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/ui-react-builder/tests/unit/html.test.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
packages/ui/src/index.ts
🧠 Learnings (3)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
packages/ui-react-builder/src/types.tspackages/ui-react-builder/tests/unit/html.test.tspackages/ui-react-builder/src/ast-parser.tspackages/ui-react-builder/src/vite-plugin.tspackages/ui-react-builder/src/build.tspackages/ui-react-builder/src/define.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to **/tests/**/*.{test,spec}.{ts,tsx} : Mirror test file structure to source: tests in `tests/` directory with `unit/`, `integration/`, and `contract/` subdirectories
Applied to files:
packages/ui-react-builder/tests/unit/html.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/ui-react-builder/src/build.tspackages/ui-react-builder/src/define.ts
🧬 Code graph analysis (3)
packages/ui-react-builder/tests/unit/html.test.ts (1)
packages/ui-react-builder/src/html.ts (1)
generateEntryPoint(151-191)
packages/ui-react-builder/src/build.ts (1)
packages/ui-react-builder/src/html.ts (1)
generateEntryPoint(151-191)
packages/ui/src/adapters/mcp.ts (2)
packages/ui/src/index.ts (1)
McpAdapterOptions(97-97)packages/core/src/types/tools.ts (1)
App(416-544)
🔇 Additional comments (13)
packages/ui/src/types.ts (1)
731-741: autoResize implementation correctly defaults and is MCP-onlyThe implementation correctly defaults
autoResizetotruevia the nullish coalescing operator inMcpAdapterconstructor (line 92 of packages/ui/src/adapters/mcp.ts), and the option is properly isolated to MCP adapter only through dedicatedMcpAdapterOptionstype. Non-MCP adapters are unaffected. Documentation is accurate.packages/ui-react-builder/src/types.ts (1)
62-73: Verified: "@default true" semantics are correctly enforced through consistent implicit delegationThe
autoResizedefault behavior is properly maintained across the entire pipeline. WhenautoResizeis undefined, both code generators (vite-plugin.ts and html.ts) omit the prop entirely, delegating the default-true behavior to AppsProvider. This pattern is consistent end-to-end:defineReactUI()preserves the input value, the AST parser maintains undefined when absent, build serializes it through__autoResize, and both output generators conditionally omit the prop. The documentation "@default true" correctly reflects the user-facing behavior.packages/ui/src/adapters/mcp.ts (2)
49-63:McpAdapterOptions+ defaulting behavior looks consistent and easy to use.Default
autoResizetotrueis clear, and the type alias aligns with the PR goal of preferringtypeoverinterface.Also applies to: 88-93
123-130: Code correctly usesautoResizeoption inAppconstructor.The
@modelcontextprotocol/ext-appsv0.2.2 constructor signature isnew App(appInfo, capabilities, options?), andautoResizeis a valid boolean option in the third parameter. The code will work as intended.packages/ui-react-builder/tests/unit/html.test.ts (1)
170-195: Good coverage for the threeautoResizestates (false/true/undefined).packages/ui-react-builder/src/build.ts (2)
154-154: LGTM: Consistent property extraction.The extraction of
__autoResizefollows the same pattern as the existing__componentand__defaultPropsproperties, maintaining consistency in the codebase.
159-163: LGTM: Clean refactor to options object.The refactor from positional arguments to a structured options object improves code maintainability and makes the API more extensible. The change integrates correctly with the updated
generateEntryPointfunction signature.packages/ui/src/index.ts (6)
97-97: LGTM: Proper type-only export syntax.The inline
typekeyword correctly marksMcpAdapterOptionsas a type-only export, which complies with the coding guideline to useexport typefor type-only exports. This is the appropriate syntax when exporting both values and types from the same module.Also applies to: 114-114
121-127: LGTM: Extensible adapter options design.The
AdapterOptionsinterface provides a clean, extensible structure for adapter-specific options. Keeping it internal is appropriate since it's implementation detail for the adapter creation flow.
132-138: LGTM: Safe and backward-compatible signature update.The addition of the optional
optionsparameter maintains backward compatibility while enabling adapter-specific configuration. The options are correctly propagated to the MCP adapter while other adapters appropriately ignore them.
165-167: LGTM: Clear documentation example.The documentation example clearly demonstrates how to use the new
autoResizeoption, helping developers understand this feature.
184-189: LGTM: Straightforward options propagation.The code correctly propagates the
autoResizeoption fromCreateClientOptionsto the MCP adapter. While themcpoptions object is created for all adapter types (including non-MCP), this is harmless and maintains a consistent structure that supports future extensibility.
192-192: LGTM: Correct integration with createAdapter.The adapter creation correctly passes the built options, completing the flow from user-provided options to adapter initialization.
…ated components - Added `autoResize` option to the MCP adapter, allowing users to enable or disable automatic size change notifications. - Updated `createClient` and `AppsProvider` to support the new `autoResize` prop. - Enhanced documentation across README files to reflect the new functionality and usage examples. - Added unit tests to ensure proper handling of the `autoResize` option in various scenarios.
- Improved the formatting of the options table in the `defineReactUI` section of the README for better readability. - Ensured consistent alignment and spacing across the table entries to enhance clarity for users.
Code Review: Auto-resize MCP AppsSummaryThis PR adds an Critical Issues1. Interface to Type Change Not DescribedThe PR description mentions:
However, in the actual code ( export type McpAdapterOptions = {
autoResize?: boolean;
};This appears to be a new type, not a conversion. The PR description is misleading. Either update the description to reflect that this is a new type, or if there was an interface before, show that change in the diff. 2. Potential Re-initialization Issue in React ContextIn useEffect(() => {
// ... initialization code
void initClient();
}, [providedClient, _forceAdapter, autoResize]);This means changing Recommendation: Remove Code Quality Issues3. Incomplete Adapter Options PatternIn const adapterOptions: AdapterOptions =
options?.autoResize \!== undefined ? { mcp: { autoResize: options.autoResize } } : {};This creates an empty object const adapterOptions: AdapterOptions | undefined =
options?.autoResize \!== undefined ? { mcp: { autoResize: options.autoResize } } : undefined;4. Test Coverage ConcernsWhile tests are added for the new functionality, there's a notable gap:
Recommendation: Add integration tests that verify:
Documentation Issues5. Inconsistent DocumentationThe documentation is comprehensive but has minor inconsistencies:
Recommendation: Clarify cross-platform behavior explicitly. Positive Aspects ✅
Minor Observations
Recommendations SummaryMust Fix:
Should Fix: Nice to Have: VerdictOnce fixed, this will be a valuable addition that gives developers fine-grained control over resize behavior. Great work on maintaining consistency across the entire stack! 🎉 |
Code Review SummaryThis PR introduces auto-resize functionality for MCP Apps, allowing widgets to automatically report size changes to the host. Strengths
Key ObservationsReact Effect Dependency (context.tsx:136) - Including autoResize in dependency array means prop changes will reinitialize the client. Document if this is intentional or ensure proper cleanup. Test Naming (context.test.tsx:145) - Test named should not re-initialize but comment says will actually re-initialize. Update test name or remove autoResize from dependencies. Future-Proofing - AdapterOptions structure suggests support for OpenAI/Mock adapter options. Consider documenting this pattern. Security and PerformanceNo security concerns - MCP-only feature with no new attack surfaces RecommendationApprove with minor suggestions Solid implementation following best practices. Main improvements needed:
Great work maintaining monorepo consistency and test coverage! Review by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/ui-react/tests/unit/context.test.tsx:
- Around line 113-178: Mock the module-level createClient and assert its calls
instead of only checking DOM: update the tests around AppsProvider to jest.mock
the client creator (createClient) so each render verifies createClient was
called with the correct autoResize value (true, false, and undefined for the
default case), replace the misleading test name/intent so the reinitialize test
reflects reality (e.g., "should re-initialize when autoResize changes") and
assert createClient was called twice across the rerender, and ensure you
reset/clear mocks between tests.
🧹 Nitpick comments (6)
packages/ui-react-builder/tests/unit/ast-parser.test.ts (1)
807-863: Auto-resize parsing tests look good; consider adding a non-literal value case.The new cases cover
autoResize: false,autoResize: true, and “absent → undefined” nicely. Optionally add a test for an unsupported value (e.g.,autoResize: someVarorautoResize: condition ? true : false) to lock in the parser’s behavior for non-boolean-literals.packages/ui/README.md (1)
91-116: Docs are clear; consider one extra “MCP only” callout nearsendSizeChanged.The section reads well and matches the intended behavior (autoResize default enabled, disable via
autoResize: false). Minor: it may help to label the manualsendSizeChangedsnippet as MCP-only as well, to avoid implying cross-adapter support.Also applies to: 155-158
packages/ui-react/README.md (1)
120-132: LGTM; optionally document what happens ifautoResizechanges after mount.Given the provider’s current behavior (autoResize affecting initialization), a short note on whether changing
autoResizetriggers reconnection vs being ignored after connect would prevent surprises.Also applies to: 162-168
packages/ui/src/index.ts (1)
121-127: Confirm whetherAdapterOptionsis meant to be public; align with “type alias” direction.PR objectives mention
AdapterOptionsas a public API, but this file currently defines it as a privateinterface. If it’s intended to be public, export it as a type-only export; if not, rename it to reduce confusion (e.g.,InternalAdapterOptions).Proposed change (public + type alias)
-/** - * Options passed to adapter creation - */ -interface AdapterOptions { - /** Options specific to MCP adapter */ - mcp?: McpAdapterOptions; -} +/** + * Options passed to adapter creation + */ +export type AdapterOptions = { + /** Options specific to MCP adapter */ + mcp?: McpAdapterOptions; +};Also applies to: 132-139
packages/ui/tests/unit/mcp-adapter.test.ts (1)
226-253: Consider enhancing test coverage to verify autoResize behavior.The tests successfully verify that the constructor accepts the
autoResizeoption and thatconnect()succeeds, but they don't verify that the autoResize value is actually stored or used correctly. Based on the implementation, the default value should betruewhen not provided.Consider adding assertions to verify:
- The default autoResize value (true) when options are omitted
- That the autoResize value is correctly stored and accessible (if there's a way to introspect this)
- That the autoResize setting affects behavior during connect() (e.g., by mocking the App constructor to verify it receives the correct autoResize parameter)
Example enhanced test
it("should default autoResize to true when not specified", async () => { const adapter = new McpAdapter(); // If there's a way to access the internal autoResize value: // expect(adapter['autoResize']).toBe(true); // Or verify behavior by mocking App instantiation await adapter.connect(); // Verify App was created with autoResize: true });packages/ui/tests/unit/create-client.test.ts (1)
76-83: Clarify the test name to reflect actual behavior.The test name suggests that
autoResizeis passed to the mock adapter, but based on the implementation inpackages/ui/src/index.ts(lines 172-197), theautoResizeoption is only propagated to the MCP adapter viaadapterOptions:const adapterOptions: AdapterOptions = options?.autoResize !== undefined ? { mcp: { autoResize: options.autoResize } } : {};The mock adapter does not receive the
autoResizesetting. Consider renaming this test to better reflect what it actually verifies (e.g., "should accept autoResize option with mock adapter" or "should not error when autoResize is provided with mock adapter").
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/core/README.mdpackages/ui-react-builder/README.mdpackages/ui-react-builder/tests/unit/ast-parser.test.tspackages/ui-react/README.mdpackages/ui-react/tests/unit/context.test.tsxpackages/ui/README.mdpackages/ui/src/index.tspackages/ui/tests/unit/create-client.test.tspackages/ui/tests/unit/mcp-adapter.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/README.md
- packages/ui-react-builder/README.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/ui/tests/unit/mcp-adapter.test.tspackages/ui/tests/unit/create-client.test.tspackages/ui-react-builder/tests/unit/ast-parser.test.tspackages/ui/src/index.tspackages/ui-react/tests/unit/context.test.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/ui/tests/unit/mcp-adapter.test.tspackages/ui/tests/unit/create-client.test.tspackages/ui-react-builder/tests/unit/ast-parser.test.tspackages/ui/src/index.tspackages/ui-react/tests/unit/context.test.tsx
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/ui/tests/unit/mcp-adapter.test.tspackages/ui/tests/unit/create-client.test.tspackages/ui-react-builder/tests/unit/ast-parser.test.tspackages/ui-react/tests/unit/context.test.tsx
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/ui/tests/unit/mcp-adapter.test.tspackages/ui/tests/unit/create-client.test.tspackages/ui-react-builder/tests/unit/ast-parser.test.tspackages/ui/src/index.ts
**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API only in
index.tsfiles
Files:
packages/ui/src/index.ts
🧠 Learnings (1)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
packages/ui-react-builder/tests/unit/ast-parser.test.ts
🧬 Code graph analysis (5)
packages/ui/tests/unit/mcp-adapter.test.ts (1)
packages/ui/src/adapters/mcp.ts (1)
McpAdapter(73-682)
packages/ui/tests/unit/create-client.test.ts (1)
packages/ui/src/index.ts (1)
createClient(173-198)
packages/ui-react-builder/tests/unit/ast-parser.test.ts (1)
packages/ui-react-builder/src/ast-parser.ts (1)
parseReactUIDefinitions(30-76)
packages/ui/src/index.ts (2)
packages/ui/src/adapters/mcp.ts (2)
McpAdapterOptions(52-63)McpAdapter(73-682)packages/core/src/adapters/index.ts (1)
createAdapter(28-30)
packages/ui-react/tests/unit/context.test.tsx (2)
packages/ui-react/src/context.tsx (1)
AppsProvider(101-157)packages/ui-react/src/index.ts (1)
AppsProvider(23-23)
🔇 Additional comments (2)
packages/ui/src/index.ts (2)
97-98: Exportingtype McpAdapterOptionsis the right shape.
184-190: autoResize threading increateClientlooks correct.Only including
{ mcp: { autoResize } }when explicitly provided preserves the MCP adapter’s default (true) and avoids changing behavior for callers that don’t pass the flag.
| describe("autoResize option", () => { | ||
| it("should pass autoResize: false to createClient", async () => { | ||
| render( | ||
| <AppsProvider forceAdapter="mock" autoResize={false} fallback={<div>Loading...</div>}> | ||
| <div data-testid="child">Ready</div> | ||
| </AppsProvider> | ||
| ); | ||
|
|
||
| // Wait for client to connect | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId("child")).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should pass autoResize: true to createClient", async () => { | ||
| render( | ||
| <AppsProvider forceAdapter="mock" autoResize={true} fallback={<div>Loading...</div>}> | ||
| <div data-testid="child">Ready</div> | ||
| </AppsProvider> | ||
| ); | ||
|
|
||
| // Wait for client to connect | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId("child")).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should work without autoResize option (default behavior)", async () => { | ||
| render( | ||
| <AppsProvider forceAdapter="mock" fallback={<div>Loading...</div>}> | ||
| <div data-testid="child">Ready</div> | ||
| </AppsProvider> | ||
| ); | ||
|
|
||
| // Wait for client to connect | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId("child")).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should not re-initialize when autoResize changes", async () => { | ||
| const { rerender } = render( | ||
| <AppsProvider forceAdapter="mock" autoResize={true} fallback={<div>Loading...</div>}> | ||
| <div data-testid="child">Ready</div> | ||
| </AppsProvider> | ||
| ); | ||
|
|
||
| // Wait for initial connection | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId("child")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| // Re-render with different autoResize value | ||
| // Note: This will actually re-initialize the client since autoResize is in the dependency array | ||
| rerender( | ||
| <AppsProvider forceAdapter="mock" autoResize={false} fallback={<div>Loading...</div>}> | ||
| <div data-testid="child">Ready</div> | ||
| </AppsProvider> | ||
| ); | ||
|
|
||
| // Component should still render | ||
| await waitFor(() => { | ||
| expect(screen.getByTestId("child")).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
find . -name "context.test.tsx" -type fRepository: AndurilCode/mcp-apps-kit
Length of output: 115
🏁 Script executed:
# Also find the AppsProvider implementation
find . -name "*.tsx" -o -name "*.ts" | xargs rg -l "export.*AppsProvider" | head -5Repository: AndurilCode/mcp-apps-kit
Length of output: 136
🏁 Script executed:
# Read the test file
cat -n ./packages/ui-react/tests/unit/context.test.tsxRepository: AndurilCode/mcp-apps-kit
Length of output: 8464
🏁 Script executed:
# Read the AppsProvider implementation
cat -n ./packages/ui-react/src/context.tsxRepository: AndurilCode/mcp-apps-kit
Length of output: 6145
🌐 Web query:
Vitest vi.mock module-level ESM imports best practices
💡 Result:
Concise best-practices for mocking ESM module-level imports with Vitest (vi.mock)
- Use vi imported from 'vitest' (or enable globals) so hoisting works. vi.mock calls are hoisted and run before imports. [2]
- Mock the module by passing a Promise import: vi.mock(import('./mod'), () => { ... }) — Vitest transforms static imports to dynamic ones so the mock is registered before module evaluation. Prefer the Promise form for explicit ESM behavior. [1][2]
- Keep vi.mock at top-level in the test file (or let Vitest hoist it); avoid calling vi.mock from files that aren’t statically analyzable by Vitest. Use vi.hoisted to share values needed by the hoisted mock. [1][2]
- Don’t import the target module in a setup file before mocking — modules imported in setupFiles are cached and won’t be mocked later. If necessary, call vi.resetModules() inside vi.hoisted to clear cache before test file runs. [2]
- If you need the real implementation inside a factory, use vi.importActual (factory can be async). [1][3]
- For live bindings: when mocking named exports, access them via the module namespace so Vitest can preserve live bindings (Vitest’s transform handles this). Prefer mocking whole module or using spies when you need original behavior. [1]
- Watch browser mode limitations: mocking is more constrained (Vitest uses transformed served modules). Some mock patterns that work in Node/JSDOM may behave differently in browser mode. [1]
- Troubleshooting: if mocks don’t take effect (esp. in containers/CI), check Node/Vite/Vitest versions and ensure files are not pre-bundled or pre-imported; confirm vi is directly imported so static analysis can hoist the calls. [2][5]
Primary sources
- Vitest docs — Mocking modules (how vi.mock is transformed/hoisted, dynamic import form, browser mode). [1]
- Vitest API — vi.mock notes, hoisting, resetModules, setupFiles warning. [2]
- Vitest API / docs — vi.importActual and async factory support. [3]
If you want, I can show a minimal example test demonstrating these patterns.
🏁 Script executed:
# Check if the test file uses beforeEach or afterEach for mock cleanup
rg "beforeEach|afterEach|beforeAll|afterAll" ./packages/ui-react/tests/unit/context.test.tsxRepository: AndurilCode/mcp-apps-kit
Length of output: 50
🏁 Script executed:
# Check the full structure of the autoResize describe block to see if mock resets are needed
sed -n '113,178p' ./packages/ui-react/tests/unit/context.test.tsxRepository: AndurilCode/mcp-apps-kit
Length of output: 2247
🏁 Script executed:
# Check if there are any other test files that show mocking patterns
rg -A5 "beforeEach" ./packages/ui-react/tests --type tsx | head -30Repository: AndurilCode/mcp-apps-kit
Length of output: 95
🏁 Script executed:
# Check the @mcp-apps-kit/ui module to understand what createClient is
find . -path "*/node_modules" -prune -o -name "*.ts" -o -name "*.tsx" | xargs rg -l "export.*createClient" | grep -v node_modules | head -5Repository: AndurilCode/mcp-apps-kit
Length of output: 94
🏁 Script executed:
# Check the createClient export
rg -A10 "export.*createClient" ./packages/ui/src/index.tsRepository: AndurilCode/mcp-apps-kit
Length of output: 442
🏁 Script executed:
# Also check what the coding guidelines say about test structure
cat ./coding_guidelines 2>/dev/null || echo "No coding_guidelines file found"Repository: AndurilCode/mcp-apps-kit
Length of output: 99
These tests lack assertions validating autoResize propagation, and one test name contradicts its actual behavior.
The tests only verify that the child renders eventually, which won't catch regressions where autoResize is silently ignored. Also, "should not re-initialize when autoResize changes" contradicts the implementation: autoResize is in the effect dependency array (line 136 of context.tsx), so the client will re-initialize when the prop changes. The test's own comment at line 166 acknowledges this contradiction.
Update the test setup to mock createClient at module level and assert:
- Each test verifies the correct
autoResizevalue (orundefinedfor the default case) is passed tocreateClient - The re-initialize test (renamed to match actual behavior) asserts
createClientis called twice across the rerender
Proposed test adjustments
import { describe, it, expect, vi } from "vitest";
import { render, screen, waitFor } from "@testing-library/react";
import { AppsProvider, useAppsClient } from "../../src";
import type { AppsClient, ToolDefs } from "@mcp-apps-kit/ui";
-import { MockAdapter, createAppsClient } from "@mcp-apps-kit/ui";
+import { MockAdapter, createAppsClient, createClient } from "@mcp-apps-kit/ui";
+
+vi.mock("@mcp-apps-kit/ui", async (importOriginal) => {
+ const original = await importOriginal<typeof import("@mcp-apps-kit/ui")>();
+ return {
+ ...original,
+ createClient: vi.fn(),
+ };
+});
+
+function stubClient(): AppsClient<ToolDefs> {
+ return {} as unknown as AppsClient<ToolDefs>;
+}
// Create a mock client for testing
async function createMockClient(): Promise<AppsClient<ToolDefs>> {
const adapter = new MockAdapter();
await adapter.connect();
return createAppsClient(adapter);
}
describe("AppsProvider", () => {
// ... existing tests ...
describe("autoResize option", () => {
it("should pass autoResize: false to createClient", async () => {
+ vi.mocked(createClient).mockResolvedValueOnce(stubClient());
render(
<AppsProvider forceAdapter="mock" autoResize={false} fallback={<div>Loading...</div>}>
<div data-testid="child">Ready</div>
</AppsProvider>
);
+ expect(vi.mocked(createClient)).toHaveBeenCalledWith({
+ forceAdapter: "mock",
+ autoResize: false,
+ });
await waitFor(() => {
expect(screen.getByTestId("child")).toBeInTheDocument();
});
});
it("should pass autoResize: true to createClient", async () => {
+ vi.mocked(createClient).mockResolvedValueOnce(stubClient());
render(
<AppsProvider forceAdapter="mock" autoResize={true} fallback={<div>Loading...</div>}>
<div data-testid="child">Ready</div>
</AppsProvider>
);
+ expect(vi.mocked(createClient)).toHaveBeenCalledWith({
+ forceAdapter: "mock",
+ autoResize: true,
+ });
await waitFor(() => {
expect(screen.getByTestId("child")).toBeInTheDocument();
});
});
it("should work without autoResize option (default behavior)", async () => {
+ vi.mocked(createClient).mockResolvedValueOnce(stubClient());
render(
<AppsProvider forceAdapter="mock" fallback={<div>Loading...</div>}>
<div data-testid="child">Ready</div>
</AppsProvider>
);
+ expect(vi.mocked(createClient)).toHaveBeenCalledWith({
+ forceAdapter: "mock",
+ autoResize: undefined,
+ });
await waitFor(() => {
expect(screen.getByTestId("child")).toBeInTheDocument();
});
});
- it("should not re-initialize when autoResize changes", async () => {
+ it("should re-initialize when autoResize changes", async () => {
+ vi.mocked(createClient).mockResolvedValue(stubClient());
const { rerender } = render(
<AppsProvider forceAdapter="mock" autoResize={true} fallback={<div>Loading...</div>}>
<div data-testid="child">Ready</div>
</AppsProvider>
);
await waitFor(() => {
expect(screen.getByTestId("child")).toBeInTheDocument();
});
rerender(
<AppsProvider forceAdapter="mock" autoResize={false} fallback={<div>Loading...</div>}>
<div data-testid="child">Ready</div>
</AppsProvider>
);
+
+ expect(vi.mocked(createClient)).toHaveBeenCalledTimes(2);
});
});
});🤖 Prompt for AI Agents
In @packages/ui-react/tests/unit/context.test.tsx around lines 113 - 178, Mock
the module-level createClient and assert its calls instead of only checking DOM:
update the tests around AppsProvider to jest.mock the client creator
(createClient) so each render verifies createClient was called with the correct
autoResize value (true, false, and undefined for the default case), replace the
misleading test name/intent so the reinitialize test reflects reality (e.g.,
"should re-initialize when autoResize changes") and assert createClient was
called twice across the rerender, and ensure you reset/clear mocks between
tests.
Code Review: Auto-resize MCP Apps FeatureThank you for this PR! I've reviewed the implementation of the auto-resize feature for MCP Apps. Here's my detailed feedback: ✅ Strengths
🔍 Issues & Concerns1. Critical: Test Comment Inconsistency// packages/ui-react/tests/unit/context.test.tsx:153-166
it("should not re-initialize when autoResize changes", async () => {
// ...
// Note: This will actually re-initialize the client since autoResize is in the dependency array
// ...
});Issue: The test name says "should not re-initialize" but the comment admits it will re-initialize. This is misleading. Recommendation:
Question: Is it intentional that changing autoResize prop triggers a full client reconnection? This seems like an expensive operation for a property that typically wouldn't change at runtime. 2. Missing Verification: autoResize Not Actually TestedThe tests verify that the option is accepted by various components, but they don't verify that it actually does anything. For example: // packages/ui/tests/unit/mcp-adapter.test.ts:242-245
it("should connect successfully with autoResize: false", async () => {
const adapterWithAutoResize = new McpAdapter({ autoResize: false });
await adapterWithAutoResize.connect();
expect(adapterWithAutoResize.isConnected()).toBe(true);
});This only tests that the option doesn't break connection, not that it disables auto-resize behavior. Recommendation: Add integration tests that verify:
3. Inconsistent Option Propagation// packages/ui/src/index.ts:185-186
const adapterOptions: AdapterOptions =
options?.autoResize !== undefined ? { mcp: { autoResize: options.autoResize } } : {};Issue: The
Recommendation: Either:
4. Type Definition Location// packages/ui/src/index.ts:114-117
interface AdapterOptions {
/** Options specific to MCP adapter */
mcp?: McpAdapterOptions;
}Issue: This internal interface is defined in Recommendation: Consider moving this to 5. Missing Edge Case: What happens if autoResize changes in ext-apps API?// packages/ui/src/adapters/mcp.ts:125-129
this.app = new App(
{ name: "@mcp-apps-kit/ui", version: "0.0.0" },
{ tools: {} },
{ autoResize: this.autoResize }
);Question: Is the third parameter Recommendation:
📝 Minor Issues6. Documentation: Default Value Not Always ClearIn some places you use "default: true", in others "(default: true)", and sometimes it's in the description text. Recommendation: Use consistent JSDoc /**
* Enable automatic size change notifications
* @default true
*/
autoResize?: boolean;7. README Example Comments// packages/core/README.md:95-96
// Optional: Disable automatic size notifications (default: true, MCP Apps only)
// autoResize: false,Minor: The commented-out example is good, but consider adding a full working example showing when you'd actually disable it (e.g., fixed-size widget). 🎯 Performance ConsiderationsNo significant performance concerns. The autoResize feature:
🔒 Security ConsiderationsNo security concerns identified. This feature:
✅ Recommendations SummaryMust Fix:
Should Fix: Nice to Have: 📊 Test CoverageGood test coverage across packages, but missing behavioral verification (see issue #2 above). 🎨 Code QualityOverall code quality is excellent:
✨ ConclusionThis is a solid feature implementation with good test coverage and documentation. The main concerns are:
With these issues addressed, this PR will be ready to merge. Great work on the comprehensive implementation across all packages! 🚀 |
Critical fixes:
1. Remove autoResize from useEffect dependency array in AppsProvider
- Prevents unnecessary client re-initialization when prop changes
- autoResize should only be applied during initial mount
- Added documentation and eslint-disable comment
2. Use undefined instead of empty object for adapter options
- More explicit when no options are provided
- Aligns with AdapterOptions | undefined type
3. Update test to verify no re-initialization on autoResize change
- Test now correctly validates that changing autoResize doesn't
cause the client to reconnect
These fixes address performance concerns and improve code clarity
as identified in code review.
Code Review: Auto-resize MCP Apps FeatureSummaryThis PR adds an ✅ Strengths
🔍 Code Quality ObservationsMcpAdapter Implementation (packages/ui/src/adapters/mcp.ts)Good:
Question:
createClient Function (packages/ui/src/index.ts)Good:
Minor:
AppsProvider React Component (packages/ui-react/src/context.tsx)Good:
Excellent:
AST Parser (packages/ui-react-builder/src/ast-parser.ts)Good:
🐛 Potential IssuesCritical: Verify MCP SDK CompatibilityThe key concern is line 128 in this.app = new App(
{ name: "@mcp-apps-kit/ui", version: "0.0.0" },
{ tools: {} },
{ autoResize: this.autoResize } // ⚠️ Is this supported by the SDK?
);Action Required:
Minor: Type SafetyIn 🎯 Performance Considerations
🔒 Security Concerns
📋 Test CoverageExcellent coverage across packages:
Coverage includes:
📝 Documentation QualityStrengths:
Suggestions:
🔄 Suggested Changes1. Verify SDK Compatibility (Required)Check if the 2. Add Integration Test (Recommended)Consider adding an integration test that verifies the actual ResizeObserver behavior when 3. Clarify Documentation (Optional)In the README examples, clarify that ✅ Checklist
🎬 ConclusionThis is a well-implemented feature with excellent test coverage and documentation. The code quality is high and follows the project's conventions closely. The main blocker is verifying that the Once the SDK compatibility is confirmed, this PR is ready to merge! 🚀 |
Documentation improvements: 1. Clarify that autoResize is only supported in MCP Apps (Claude Desktop) and ignored in ChatGPT across all package READMEs 2. Remove incorrect autoResize example from core/README.md - autoResize is a client-side option, not a server-side UIDef property - defineUI() does not accept autoResize parameter 3. Add note that autoResize only applies during initial mount - Changing the prop at runtime has no effect - Documented in ui-react context and provider props 4. Expand ui-react README with more details about ResizeObserver behavior These changes address documentation inconsistencies identified in code review.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui-react/src/context.tsx (1)
126-143: autoResize not frozen at initial mount value.The documentation and comments state that autoResize is "only applied during initial mount," but the current implementation uses the closure-captured value. If the effect re-runs because
providedClientor_forceAdapterchange, the currentautoResizevalue is used, not the initial one.Edge case: User mounts with
autoResize={true}, later updates toautoResize={false}and simultaneously changes_forceAdapter. The effect re-runs and creates a client withautoResize=false, contradicting the documentation.📌 Proposed fix: freeze autoResize at initial value
Capture the initial
autoResizevalue in a ref to ensure it remains stable even when the effect re-runs:}: AppsProviderProps<T>): React.JSX.Element { const [client, setClient] = useState<AppsClient<T> | null>(providedClient ?? null); const [isConnecting, setIsConnecting] = useState(!providedClient); const [error, setError] = useState<Error | null>(null); + const autoResizeRef = useRef(autoResize); useEffect(() => { if (providedClient) { setClient(providedClient); setIsConnecting(false); return; } // Initialize client with auto-detection or forced adapter const initClient = async () => { try { const newClient = await createClient<T>({ forceAdapter: _forceAdapter, - autoResize, + autoResize: autoResizeRef.current, }); setClient(newClient); setIsConnecting(false);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ui-react/src/context.tsxpackages/ui-react/tests/unit/context.test.tsxpackages/ui/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ui-react/tests/unit/context.test.tsx
- packages/ui/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/ui-react/src/context.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/ui-react/src/context.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (2)
packages/ui-react/src/context.tsx (2)
59-71: LGTM! Clear documentation for the autoResize prop.The documentation clearly explains the behavior, scope (MCP adapter only), timing (initial mount), and default value. Well done.
108-108: LGTM!Simple prop destructuring, correctly integrated.
Features:
1. Add autoResize property to UIDef interface in core package
- Enables automatic size change notifications (MCP Apps only)
- Defaults to true when not specified
- Properly documented with JSDoc
2. Update core README.md with autoResize example
- Shows usage in defineUI() with restaurant list example
- Clarifies MCP Apps only behavior
3. Add comprehensive test coverage for autoResize in UIDef
- Test that autoResize can be explicitly set to false
- Test default behavior when not specified (undefined)
- Test autoResize alongside all other UIDef properties
This provides full consistency across the API:
- Server-side: defineUI({ autoResize })
- React builder: defineReactUI({ autoResize })
- Client-side: createClient({ autoResize })
- Provider: <AppsProvider autoResize={...} />
The autoResize property flows through the protocol metadata automatically
via the existing metadata generation system.
- Improved alignment and spacing in the options table for better readability. - Enhanced clarity of descriptions for each option, ensuring consistent presentation across the documentation.
PR Review: Auto-resize MCP Apps FeatureOverviewThis PR adds an ✅ StrengthsArchitecture & Design
Type Safety
Test CoverageComprehensive test coverage across all affected packages:
The tests validate both Documentation
🔍 Issues & Recommendations1. Intentional Dependency Array Exclusion Needs VerificationLocation: // eslint-disable-next-line react-hooks/exhaustive-deps
}, [providedClient, _forceAdapter]);Issue:
Recommendation: Consider one of these approaches:
Example dev warning: const prevAutoResize = useRef(autoResize);
useEffect(() => {
if (process.env.NODE_ENV !== 'production' && prevAutoResize.current !== autoResize) {
console.warn('[AppsProvider] autoResize changed but will be ignored. Restart the app to apply changes.');
}
prevAutoResize.current = autoResize;
}, [autoResize]);2. Default Value Inconsistency in TestsLocation: const ui = app.tools.default.ui as { autoResize?: boolean };
// autoResize is optional, so it may be undefined when not specified
// The default is applied at the client-side adapter level
expect(ui.autoResize).toBeUndefined();Issue: The comment states "The default is applied at the client-side adapter level," but looking at the code:
Question: Should the default ( Current behavior: If a user doesn't specify Recommendation: This is actually correct and intentional - you're avoiding embedding defaults in the tool definition. However, the test comment could be clearer: // autoResize is optional and undefined when not specified.
// The default (true) is applied by the MCP adapter at runtime.
expect(ui.autoResize).toBeUndefined();3. Missing Type Narrowing in AdapterOptionsLocation: const adapterOptions: AdapterOptions | undefined =
options?.autoResize !== undefined ? { mcp: { autoResize: options.autoResize } } : undefined;Issue: This creates
Recommendation: Simplify to always pass options (type system allows it): const adapter = createAdapter(protocol, { mcp: { autoResize: options?.autoResize } });The 4. HTML Generation Template InconsistencyLocation: Multiple files generate entry points with the autoResize prop
Issue: Both files have similar but separate entry point generation logic: const providerProps = autoResize === undefined ? "" : ` autoResize={${autoResize}}`;Observation: This is correct but duplicated. If the logic changes, both places need updates. Recommendation: Extract to a shared helper function: function buildProviderProps(autoResize?: boolean): string {
return autoResize === undefined ? "" : ` autoResize={${autoResize}}`;
}Or use the 5. PR Description Minor IssuePR Description states:
Actual change: Created a new type Recommendation: Update PR description for accuracy:
🛡️ Security & Performance
📊 Test Coverage AssessmentScore: 9/10 - Excellent coverage The PR adds:
What's covered:
Minor gap: No integration test verifying end-to-end flow from ✨ Code Quality
📝 Suggestions Summary
VerdictApproved with minor suggestions ✅ This is a well-implemented feature that:
The identified issues are minor and could be addressed in follow-up PRs if desired. The core implementation is solid and ready to merge. Great work on maintaining consistency across the monorepo! The colocated UI pattern with autoResize integration is particularly clean. |
Code Review: Auto-resize MCP Apps FeatureSummaryThis PR adds automatic size change notification functionality to the MCP Apps adapter, allowing widgets to automatically report their size changes to the host. The implementation is well-designed and follows the project's conventions. ✅ Strengths1. Clean API Design
2. Type Safety
3. Excellent Test Coverage
4. Documentation
5. Consistent Implementation
🔍 Code Quality ObservationsGood: McpAdapter Implementation (packages/ui/src/adapters/mcp.ts:90-92, 125-129)
Excellent: React Context (packages/ui-react/src/context.tsx:139-142)
Good: createClient Options Flow (packages/ui/src/index.ts:184-186)
🔒 Security & PerformanceSecurity: No concerns identified
Performance: Good characteristics
📝 RecommendationsRequired before merge: None - the PR is ready to merge Nice to have:
✨ Overall AssessmentGrade: A This is a well-crafted PR with:
Recommendation: Approve and merge ✅ Great work on this feature! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/core/src/types/ui.ts:
- Around line 119-130: The JSDoc for the autoResize property is misleading: it
currently says "@default true" but the type does not apply a runtime default
(the client-side adapter applies true when the property is undefined). Update
the comment for autoResize to state that the type itself does not set a default
and that the client-side adapter will treat an omitted/undefined autoResize as
true (or, if you prefer, remove the @default tag and add a sentence clarifying
that the default behavior is enforced by the client adapter).
🧹 Nitpick comments (1)
packages/core/tests/unit/ui-resources.test.ts (1)
132-155: Consider renaming this test to match its actual behavior.The test name states "should default autoResize to true when not specified," but the test actually verifies that
autoResizeisundefinedwhen not specified (line 154). The comment on line 153 clarifies that "The default is applied at the client-side adapter level."This creates confusion between the test name and its implementation. Consider renaming the test to more accurately reflect what it verifies.
💡 Suggested test name
- it("should default autoResize to true when not specified", () => { + it("should leave autoResize undefined when not specified", () => { const defaultTool = defineTool({Or alternatively:
- it("should default autoResize to true when not specified", () => { + it("should allow client-side adapter to apply autoResize default when not specified", () => { const defaultTool = defineTool({
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/README.mdpackages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.tspackages/ui-react-builder/README.mdpackages/ui-react/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ui-react/README.md
- packages/core/README.md
- packages/ui-react-builder/README.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript with no
anytypes - useunknownand narrow instead
Files:
packages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Remove unused variables or prefix with underscore (
_)
Files:
packages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.ts
{packages/core,examples}/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
{packages/core,examples}/**/*.ts: Always usedefineToolanddefineUIfor type inference when defining tools and UI components
Use Koa-style async/await middleware pattern withawait next()to chain middleware execution
UseAppErrorandErrorCodefrom@mcp-apps-kit/corefor error handling
Implement plugins using thePlugininterface with hooks:onInit,onStart,onShutdown,beforeToolCall,afterToolCall,onToolError
Useapp.events.on()andapp.events.once()for event subscription with event types likeapp:init,tool:call,app:start
Use Zod schemas withdefineToolfor input/output validation
Colocate UI definitions near tool definitions usingdefineUIwithhtmlproperty pointing to compiled UI assets
Files:
packages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
export typefor type-only exports
Files:
packages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.ts
**/tests/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mirror test file structure to source: tests in
tests/directory withunit/,integration/, andcontract/subdirectories
Files:
packages/core/tests/unit/ui-resources.test.ts
🧠 Learnings (2)
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Colocate UI definitions near tool definitions using `defineUI` with `html` property pointing to compiled UI assets
Applied to files:
packages/core/src/types/ui.tspackages/core/tests/unit/ui-resources.test.ts
📚 Learning: 2025-12-29T20:58:59.376Z
Learnt from: CR
Repo: AndurilCode/mcp-apps-kit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T20:58:59.376Z
Learning: Applies to {packages/core,examples}/**/*.ts : Always use `defineTool` and `defineUI` for type inference when defining tools and UI components
Applied to files:
packages/core/tests/unit/ui-resources.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (2)
packages/core/tests/unit/ui-resources.test.ts (2)
108-130: LGTM!The test correctly verifies that
autoResize: falseis preserved in the UI definition. The implementation uses properdefineToolanddefineUIhelpers as per coding guidelines.
618-618: LGTM!The addition of
autoResize: falseto the comprehensive test is consistent and correct. The type assertion and expectation properly validate the new property.Also applies to: 643-643, 658-658
| /** | ||
| * Enable automatic size change notifications. | ||
| * | ||
| * When enabled, the UI automatically reports its size changes to the host | ||
| * using a ResizeObserver on document.body and document.documentElement. | ||
| * The host can then resize the UI container accordingly. | ||
| * | ||
| * MCP Apps only - ignored in ChatGPT. | ||
| * | ||
| * @default true | ||
| */ | ||
| autoResize?: boolean; |
There was a problem hiding this comment.
Clarify the @default documentation for autoResize.
The JSDoc states @default true, but the test on line 154 of packages/core/tests/unit/ui-resources.test.ts shows that when autoResize is not specified, it remains undefined, with the comment stating "The default is applied at the client-side adapter level."
This creates ambiguity about whether:
- The property defaults to
trueat this type level, or - The property remains
undefinedwhen unspecified, and the client-side adapter applies a default oftrue
Consider revising the JSDoc to clarify this behavior more accurately.
📝 Suggested JSDoc revision
/**
* Enable automatic size change notifications.
*
* When enabled, the UI automatically reports its size changes to the host
* using a ResizeObserver on document.body and document.documentElement.
* The host can then resize the UI container accordingly.
*
* MCP Apps only - ignored in ChatGPT.
*
- * @default true
+ * @default true (applied by client-side adapter when undefined)
*/
autoResize?: boolean;Or alternatively, if the server-side definition truly doesn't have a default:
/**
* Enable automatic size change notifications.
*
* When enabled, the UI automatically reports its size changes to the host
* using a ResizeObserver on document.body and document.documentElement.
* The host can then resize the UI container accordingly.
*
* MCP Apps only - ignored in ChatGPT.
*
- * @default true
+ * When undefined, the client-side adapter applies a default of true.
*/
autoResize?: boolean;📝 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.
| /** | |
| * Enable automatic size change notifications. | |
| * | |
| * When enabled, the UI automatically reports its size changes to the host | |
| * using a ResizeObserver on document.body and document.documentElement. | |
| * The host can then resize the UI container accordingly. | |
| * | |
| * MCP Apps only - ignored in ChatGPT. | |
| * | |
| * @default true | |
| */ | |
| autoResize?: boolean; | |
| /** | |
| * Enable automatic size change notifications. | |
| * | |
| * When enabled, the UI automatically reports its size changes to the host | |
| * using a ResizeObserver on document.body and document.documentElement. | |
| * The host can then resize the UI container accordingly. | |
| * | |
| * MCP Apps only - ignored in ChatGPT. | |
| * | |
| * @default true (applied by client-side adapter when undefined) | |
| */ | |
| autoResize?: boolean; |
🤖 Prompt for AI Agents
In @packages/core/src/types/ui.ts around lines 119 - 130, The JSDoc for the
autoResize property is misleading: it currently says "@default true" but the
type does not apply a runtime default (the client-side adapter applies true when
the property is undefined). Update the comment for autoResize to state that the
type itself does not set a default and that the client-side adapter will treat
an omitted/undefined autoResize as true (or, if you prefer, remove the @default
tag and add a sentence clarifying that the default behavior is enforced by the
client adapter).
Remove eslint-disable comment for react-hooks/exhaustive-deps rule which is not configured in the ESLint setup. The explanatory comment about why autoResize is intentionally not in the dependency array remains and is sufficient documentation.
Code Review: Auto-resize MCP AppsSummaryThis PR adds automatic size change notifications for MCP Apps (Claude Desktop) UIs using ResizeObserver. The implementation spans across multiple packages ( Note: The PR description doesn't match the actual changes. It mentions "Updated McpAdapterOptions from an interface to a type definition" but the PR actually implements the auto-resize feature. Consider updating the description to accurately reflect the changes. ✅ Strengths
🔍 Issues & Concerns1. PR Description Mismatch (High Priority)The PR body talks about changing Recommendation: Update the PR description to accurately describe the auto-resize feature. 2. Missing Implementation Details (Medium Priority)The PR adds the this.app = new App(
{ name: "@mcp-apps-kit/ui", version: "0.0.0" },
{ tools: {} },
{ autoResize: this.autoResize } // packages/ui/src/adapters/mcp.ts:128
);Questions:
3. React useEffect Dependency Array (Medium Priority)In void initClient();
// Note: autoResize is intentionally not in the dependency array.
// It should only be set during initial mount. Changing it at runtime
// would cause unnecessary client re-initialization.While the reasoning makes sense, this creates a subtle behavior:
Recommendation: Consider either:
4. Default Value Inconsistency (Low Priority)The default handling is slightly inconsistent:
The test at line 149 expects Recommendation: Consider adding a test that verifies the default behavior at the adapter level, not just the definition level. 5. Entry Point Generation (Low Priority)In const providerProps = autoResize === undefined ? "" : \` autoResize=\${autoResize}\`;This generates Recommendation: Consider only adding the prop when it's explicitly const providerProps = autoResize === false ? " autoResize={false}" : "";This would reduce bundle size slightly and make the default case cleaner. 6. Test Coverage Gaps (Low Priority)The tests verify that the option is accepted and passed through, but don't test:
These might be hard to test without the actual MCP Apps SDK being present, but consider integration tests or mocking if possible. 🔒 Security ConsiderationsNo security concerns identified. The feature is purely additive and doesn't introduce any new attack vectors. ⚡ Performance ConsiderationsResizeObserver overhead: ResizeObserver is efficient, but observing both
📝 Code QualityStrengths:
Minor Style Issues:
🎯 Recommendations
✅ Approval StatusConditional Approval pending:
The code quality is excellent and the feature is well-implemented. The main concern is ensuring the underlying SDK actually supports this option as expected. 📚 Additional NotesFollowing the project's CLAUDE.md guidelines:
Great work on maintaining consistency across the monorepo! 🎉 |
2 participants
