-
Notifications
You must be signed in to change notification settings - Fork 37.8k
mcp: be smart about delegating scroll in mcp apps #290514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @bpaseroMatched files:
|
Implements smart scroll delegation for MCP app iframes. Instead of delegating 100% of wheel events when content is below max height (or 0% when scrollable), now detects scroll boundaries and only bubbles events when the iframe's content is scrolled to the top (scrolling up) or bottom (scrolling down). - Adds scroll boundary detection in the MCP app preamble script that checks both element-level scrolling (overflow: auto/scroll) and document-level scrolling (overflow: visible on html/body) - Introduces ui/notifications/sandbox-wheel notification to signal when scroll events should bubble from the iframe to the chat widget - Adds CustomSandboxWheelNotification interface to McpApps protocol - Removes the old canScrollWithin derived observable that made blanket delegation decisions based only on content height (Commit message generated by Copilot)
85057ff to
2254e4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements smart scroll delegation for MCP app iframes by detecting scroll boundaries instead of using a blanket delegation strategy based on content height. The changes enable wheel events to bubble from the iframe to the chat widget only when the iframe's content is scrolled to its boundaries.
Changes:
- Adds scroll boundary detection logic to the MCP app preamble script that checks both element-level and document-level scrolling
- Introduces a new CustomSandboxWheelNotification protocol message to communicate wheel events from the iframe to the host
- Replaces the old canScrollWithin observable-based approach with event-driven boundary detection
- Includes unrelated UI improvements to the agent session hover widget (adding provider names)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/vs/workbench/contrib/mcp/common/modelContextProtocolApps.ts | Adds CustomSandboxWheelNotification interface to the McpApps protocol |
| src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatMcpAppModel.ts | Removes old scroll delegation logic, adds scroll boundary detection in preamble script, implements _handleSandboxWheel handler |
| src/vs/workbench/contrib/chat/browser/agentSessions/agentSessionHoverWidget.ts | Adds provider names to agent session hover widget (unrelated to scroll delegation) |
| src/vs/workbench/contrib/chat/browser/agentSessions/media/agentSessionHoverWidget.css | Adds CSS styling for codicon display (unrelated to scroll delegation) |
Comments suppressed due to low confidence (1)
src/vs/workbench/contrib/chat/browser/widget/chatContentParts/toolInvocationParts/chatMcpAppModel.ts:663
- The deltaY value is being negated when constructing the mock IMouseWheelEvent (line 663), but StandardWheelEvent's constructor (which is called by delegateScrollFromMouseWheelEvent) will negate deltaY again when processing the wheel event (see mouseEvent.ts line 181). This double negation will cause scrolling to occur in the wrong direction. The params.deltaY value should be passed through without negation, as StandardWheelEvent handles the sign conversion internally.
Implements smart scroll delegation for MCP app iframes. Instead of
delegating 100% of wheel events when content is below max height (or 0%
when scrollable), now detects scroll boundaries and only bubbles events
when the iframe's content is scrolled to the top (scrolling up) or bottom
(scrolling down).
checks both element-level scrolling (overflow: auto/scroll) and
document-level scrolling (overflow: visible on html/body)
scroll events should bubble from the iframe to the chat widget
delegation decisions based only on content height
(Commit message generated by Copilot)