Feat/add request display mode support #1131
Conversation
dosubot
bot
added
the
size:M
WalkthroughUpdates add CONTRIBUTING.md guidance to copy 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)client/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
🔍 Remote MCP Context7Additional context found (concise, relevant to code review)
Sources used:
⏰ 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)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
CONTRIBUTING.mdclient/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧬 Code graph analysis (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
client/src/stores/ui-playground-store.ts (1)
DisplayMode(30-30)
🔍 Remote MCP Context7
Summary of Relevant Context for PR Review
Project Context
The MCP Inspector is a developer tool for testing and debugging MCP servers, featuring a React-based web UI and a Node.js proxy for seamless browser interaction. The project is organized as a monorepo with workspaces, with the client/ directory containing the React frontend built with Vite, TypeScript, and Tailwind CSS, and the server/ directory housing the Express backend.
Development Environment Changes
The CONTRIBUTING.md updates are relevant to the development setup:
- The development process involves using
npm run prettier-check,npm run prettier-fix,npm test, andnpm run test:e2eto validate changes before submission. - The port change from 3000 to 5173 aligns with standard Vite development server configuration (Vite defaults to port 5173).
Component Context: MCPAppsRenderer
The PR modifies mcp-apps-renderer.tsx to support a new JSON-RPC method ui/request-display-mode. This component is part of the client-side UI rendering system responsible for displaying widgets/apps from MCP servers with different display modes (inline, pip/picture-in-picture, fullscreen).
Key design considerations for the review:
- The new
onRequestPipcallback prop enables parent components to respond when child widgets request PIP mode - Display mode adaptation logic accounts for mobile constraints (devices <768px cannot display PIP mode, forcing fullscreen instead)
- The implementation maintains backward compatibility by making the new prop optional
- The component communicates state changes back to the host via the existing
ui/notifications/host-context-changedmechanism
Code Standards
Adhering to code style guidelines includes using TypeScript with proper type annotations, following React functional component patterns with hooks, using Prettier for code formatting, maintaining consistent naming conventions (camelCase for variables/functions, PascalCase for components, kebab-case for filenames), and utilizing Tailwind CSS for styling.,
🔇 Additional comments (2)
CONTRIBUTING.md (1)
47-61: Documentation improvements look good.The environment file setup instruction and corrected port reference (5173 for Vite) align with standard development practices and improve clarity for contributors.
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
60-60: Prop addition follows existing patterns.The new
onRequestPipcallback is properly typed as optional and consistent with sibling callbacks likeonExitPip.
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)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
724-750: Critical: MissingviewportHeightin dependency array.The
handleMessagecallback usesviewportHeight(line 527 in theui/initializehandler) but it's absent from the dependency array. This creates a stale closure where the callback may reference an outdated viewport height value if the device type or custom viewport changes.🔎 Required fix
}, [ themeMode, effectiveDisplayMode, locale, timeZone, platform, viewportWidth, + viewportHeight, deviceCapabilities, safeAreaInsets, toolName, toolMetadata, onCallTool, onSendFollowUp, serverId, toolCallId, toolInput, toolOutput, toolState, sendResponse, sendNotification, addUiLog, addCspViolation, setDisplayMode, onRequestPip, onExitPip, pipWidgetId, ], );
🧹 Nitpick comments (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
634-660: Handler logic is sound; consider removing redundant notification.The display mode request handling correctly normalizes
piptofullscreenon narrow viewports and manages state/callbacks appropriately.However, the explicit notification on lines 656–658 is likely redundant. The
useEffectat lines 772–861 already emitshost-context-changednotifications whenevereffectiveDisplayModechanges. SincesetDisplayMode(actualMode)triggers a state update, theuseEffectwill fire and send a notification automatically. The immediate notification here may result in duplicate messages to the guest.🔎 Suggested simplification
Remove the explicit notification; let the
useEffecthandle it:sendResponse(id, { displayMode: actualMode }); - - sendNotification("ui/notifications/host-context-changed", { - displayMode: actualMode, - }); break;Optionally, extract the viewport breakpoint to a named constant:
+ const MOBILE_VIEWPORT_THRESHOLD = 768; + case "ui/request-display-mode": { const requestedMode = (params as { mode?: DisplayMode })?.mode || "inline"; - const isMobile = viewportWidth < 768; + const isMobile = viewportWidth < MOBILE_VIEWPORT_THRESHOLD;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧬 Code graph analysis (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
client/src/stores/ui-playground-store.ts (1)
DisplayMode(30-30)
🔍 Remote MCP Context7
Additional context found (concise)
- Resolved repository to Context7 library ID: /modelcontextprotocol/inspector — "MCP Inspector", React + Node.js developer tool for MCP servers.
- Source reputation: High; Benchmark Score: 79.3; Code snippets: 85. (Useful to prioritize authoritative docs/examples in review.)
No other external file-level diffs or docs were returned by the resolver; further file-level or code-search queries can be run against /modelcontextprotocol/inspector if you want specific changed files, diffs, or usage examples for mcp-apps-renderer.tsx.
🔇 Additional comments (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
60-60: LGTM - Clean prop addition.The
onRequestPipprop follows the established pattern for display mode callbacks. Type signature and destructuring are correct.Also applies to: 92-92
|
Hey there! Thanks for your contribution. Looks good to me, pushing a quick fix to use |
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)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
724-750: Critical: Dependency array still incomplete from prior review.While the new dependencies (
setDisplayMode,onRequestPip,onExitPip,pipWidgetId) were correctly added,viewportHeightandmaxHeightremain missing despite being flagged in the previous review. Both are referenced in theui/initializehandler (lines 527-528) and their absence creates stale closures—viewport changes won't be reflected in initialization responses sent to the guest UI.🔎 Add missing dependencies
[ themeMode, effectiveDisplayMode, locale, timeZone, platform, viewportWidth, + viewportHeight, deviceCapabilities, safeAreaInsets, toolName, toolMetadata, onCallTool, onSendFollowUp, serverId, toolCallId, toolInput, toolOutput, toolState, sendResponse, sendNotification, addUiLog, addCspViolation, setDisplayMode, onRequestPip, onExitPip, pipWidgetId, + maxHeight, ],
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx
🧬 Code graph analysis (1)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (1)
client/src/stores/ui-playground-store.ts (1)
DisplayMode(30-30)
🔍 Remote MCP Context7
Additional context found (concise, relevant to code review)
-
Resolved repository to Context7 library ID: /modelcontextprotocol/inspector — MCP Inspector (React + Node.js dev tool). Chosen because name matches repo and it has high reputation and many code snippets, making it the authoritative docs/source for this codebase.
-
Key PR facts (from provided PR context):
- Adds support for a JSON-RPC method "ui/request-display-mode" in client/src/components/chat-v2/thread/mcp-apps-renderer.tsx:
- Parses requested mode (default "inline"), maps "pip" → "fullscreen" on mobile (viewportWidth < 768).
- Calls setDisplayMode(actualMode), responds to caller with { mode: actualMode }.
- Calls onRequestPip(toolCallId) when actualMode === "pip".
- Calls onExitPip(toolCallId) when leaving pip to inline/fullscreen if current widget is pip.
- Notifies host context change via ui/notifications/host-context-changed with new displayMode.
- Public API change: adds optional prop onRequestPip?: (toolCallId: string) => void to MCPAppsRendererProps (breaking/public API surface change).
- Commit changed response key from "displayMode" to "mode".
- Non-code changes: CONTRIBUTING.md updated (copy .env.local → .env.development; dev server port reference changed to 5173) and package-lock updated from npm install.
- PR references issue #1130 and is targeted at main (feat/add-request-display-mode-support → main). (PR URL: #1131)
- Adds support for a JSON-RPC method "ui/request-display-mode" in client/src/components/chat-v2/thread/mcp-apps-renderer.tsx:
Sources used:
- Repository resolver: Context7_resolve-library-id (result: /modelcontextprotocol/inspector)
⏰ 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: Cursor Bugbot
🔇 Additional comments (2)
client/src/components/chat-v2/thread/mcp-apps-renderer.tsx (2)
92-92: Public API addition looks good.The new
onRequestPipcallback follows the established pattern for display mode lifecycle management and integrates cleanly with the existingonExitPiphandler.
634-660: Display mode handler implementation is sound.The logic correctly maps
piptofullscreenon narrow viewports, properly sequences state updates before callbacks and notifications, and uses the component's computedviewportWidth(respecting playground device simulation). Response format aligns with the protocol update noted in the commit message.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| case "ui/request-display-mode": { | ||
| const requestedMode = | ||
| (params as { mode?: DisplayMode })?.mode || "inline"; | ||
| const isMobile = viewportWidth < 768; |
There was a problem hiding this comment.
Mobile detection uses wrong viewport causing incorrect pip fallback
The mobile detection isMobile = viewportWidth < 768 uses viewportWidth, which defaults to 400 when not in playground mode. Since 400 is always less than 768, all pip mode requests outside the playground will be incorrectly converted to fullscreen. The similar handler in chatgpt-app-renderer.tsx correctly uses window.innerWidth < 768 to check the actual browser viewport instead.
Co-authored-by: Andrew Khadder <54488379+khandrew1@users.noreply.github.com>
2 participants
Solving #1130
Note
MCP Apps display mode support
ui/request-display-modehandling inMCPAppsRenderer, settingdisplayMode(inline/pip/fullscreen), invokingonRequestPip/onExitPip, falling back tofullscreenon mobile whenpipis requested, replying with{ mode }, and sendingui/notifications/host-context-changed.onRequestPipprop and wires dependencies (setDisplayMode,onRequestPip,onExitPip,pipWidgetId) in handlers.Docs and misc
CONTRIBUTING.md: add.envcopy step and change client dev port to:5173.package-lock.jsonversion to1.4.0.Written by Cursor Bugbot for commit e9d8498. This will update automatically on new commits. Configure here.