-
Notifications
You must be signed in to change notification settings - Fork 47
[MCP Apps] Update viewport type
#153
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
base: main
Are you sure you want to change the base?
Conversation
commit: |
ac1d83c to
33fa759
Compare
|
@claude please review this PR in depth, and give your assessment / recommendations wrt/ backwards compatibility |
jonathanhefner
left a comment
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.
Agreed this makes much more sense. It makes it easier for MCP Apps to determine their dimensions because one dimension is essentially fixed, and it simplifies the responsibilities of the host application.
33fa759 to
046bef4
Compare
|
Yes - we also discussed this earlier in the SEP process, since ChatGPT Apps and MCP-UI were treating this question in different ways. Two points here:
|
|
@liady for both of those two points, can you give some specific examples where those would be true? I'm having a hard time imagining a situation for both of these Also for 2, what are you saying is the implication of that? And what is the way that we would account for that future? |
04b60b9 to
8614792
Compare
|
@martinalong for #1 there are several use cases like rendering the UI in a modal (where it can dynamically expand vertically and horizontally), or in a sidebar where it can affect the width, etc. Since we're not targeting only inlined UIs inside chats - and perhaps more display modes in the future - the horizontal axis size changes inside the UI might still be relevant. For #2 - there is no concrete example other than rendering the UI not inside an iframe (for example inside a modal that's not an iframe). In that case media queries / js apis won't work. However that's not something we currently allow in the SEP, so it's not an issue. If we'll add render targets that are not web iframes, we can re-introduce passing the host's dimensions in the future. |
8614792 to
189f525
Compare
Ahh I see what you mean. Like basically any view where the app doesn't have to take up the full width. Hmm yeah, I was trying to match the OpenAI spec here which just passes We should probably go back to passing The viewport has two independent dimension pairs:
These pairs are independent—you can have a fixed height with flexible width, or any other combination.
Example usage:
Oh I was imagining apps would be measuring "full width" through just |
|
@martinalong sounds good, just few points:
So for our purposes I think:
So all in all I think that keeping |
|
Thanks @martinalong! |
|
Yep what you said is my understanding of it as well! I'll make the updates |
189f525 to
c47b970
Compare
viewport with maxHeightviewport type
specification/draft/apps.mdx
Outdated
| // Handle height | ||
| if ("height" in viewport) { | ||
| // Fixed height: fill the container | ||
| document.body.style.height = "100%"; |
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.
Not sure we need to add reference implementations here, or just let apps handle it their own way like they handle any browser window. (some apps might want scrolling content like a scrolling carousel).
If we do want - we should make some adjustments -
- Use
document.documentElementeverywhere instead ofdocument.body- since this is the element that actually takes the iframe size. It also fixes issues if the HTML has paddings, the body has margins, etc. - Use
vh/vwunits since for the app the iframe is the actual viewport - Clarify why we're limiting the app size - if we need the app to at least fill the container it might need to be
style.minHeight/minWidth. If we want to force the app to have fixed size no matter what then.widthis ok.
Again - we can also leave it to the internal implementation of the app.
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.
some apps might want scrolling content like a scrolling carousel
I think it's informative to have an example, even if that example doesn't cover all the ways you can use it!
But overall 1 and 2 sound good to me! Will update
On the topic of minHeight: height/maxHeight to refer to the iframe height, not the app height. Apps can do whatever they want within that height (scroll or not)
b35a4e2 to
010ff33
Compare
| width: number; | ||
| height: number; | ||
| /** Viewport width (if fixed). Only pass width or maxWidth, not both. */ | ||
| width?: number; |
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.
@ochafik I realized that no matter what, we have to make this change (making these fields optional), which is is a minor breaking change
I don't think there's really a way around it if we want to adopt this new shape. Wdyt should be the plan here? I think we may as well just update it to the type i originally proposed and announce the next version will be a breaking change
96f017b to
aed8cbe
Compare
Motivation and Context
viewport.widthandviewport.maxWidthwere unnecessary because the width of the app always needs to be 100%viewport.heightis unnecessary because the app decides the heightviewport.maxHeight. Thus,viewporthas been simplified to justmaxHeightonsizechangecan also be updated toonheightchangesince we don't do anything with the width change notificationsHow Has This Been Tested?
Manually tested with examples
Breaking Changes
Yes, any users using the
viewportproperty will have to update their behaviorTypes of changes
Checklist
Additional context