Add ui/notifications/request-close notification for UI to initiate termination#215
Add ui/notifications/request-close notification for UI to initiate termination#215fredericbarthelet wants to merge 3 commits intomodelcontextprotocol:mainfrom
Conversation
@modelcontextprotocol/ext-apps
@modelcontextprotocol/server-basic-react
@modelcontextprotocol/server-basic-vanillajs
@modelcontextprotocol/server-budget-allocator
@modelcontextprotocol/server-cohort-heatmap
@modelcontextprotocol/server-customer-segmentation
@modelcontextprotocol/server-map
@modelcontextprotocol/server-pdf
@modelcontextprotocol/server-scenario-modeler
@modelcontextprotocol/server-shadertoy
@modelcontextprotocol/server-sheet-music
@modelcontextprotocol/server-system-monitor
@modelcontextprotocol/server-threejs
@modelcontextprotocol/server-transcript
@modelcontextprotocol/server-video-resource
@modelcontextprotocol/server-wiki-explorer
commit: |
df4c168 to
52fff75
Compare
949f84a to
95bd5ec
Compare
|
@fredericbarthelet thanks for this! several points:
What do you think? |
|
Hey @liady, thanks for your comments. I'll add the spec changes and tests to the PR.
while, given it was at the guest initiative, it could have been much simpler:
For the sake of clarity (and steering away from this conversation analogy 😅), here are the 2 options as sequences. Option A: ui/close-resource (current PR)Simpler. sequenceDiagram
participant Host
participant Guest
note right of Guest: App performs cleanup first
Guest->>Guest: saveState(), cleanup()
Guest--)Host: ui/close-resource (request)
note left of Host: Host unmounts immediately
Host->>Host: iframe.remove()
Option B: ui/request-close → ui/resource-teardownPiggy-back on existing implementation of sequenceDiagram
participant Host
participant Guest
Guest--)Host: ui/request-close (notification)
note left of Host: Host decides whether to close
alt Host approves close
Host->>Guest: ui/resource-teardown (request)
note right of Guest: App performs cleanup
Guest->>Guest: saveState(), cleanup()
Guest-->>Host: {} (success response)
Host->>Host: iframe.remove()
else Host denies close
note left of Host: Host ignores or defers
end
I can go with option B if you deem it more appropriate here. Let me know :) |
|
Hey @liady, happy to go with option B if you prefer it this way. Just let me know and I'll update my PR, documentation and add tests accordingly :) Thanks ! |
|
Thanks @fredericbarthelet, awesome contribution! View removal may be deferred by the Host or outright blocked, and the view needs to know that it's still running business-as-usual. In that time, data might have mutated (e.g., the user continued to interact with it). That data would now need to be handled gracefully. This leads me down the path of |
|
@fredericbarthelet yes, I think for now option B might be preferrable, since it builds on top of the current mechanism, and still leaves the lifecycle control in the hands of the host, preventing possibly surprising edge cases. The view still gains the ability to initiate this process. So I'd recommend implementing B for now. Do you see a specific use-case in favor of option A - where the view "has" to first initiate its own termination and only then notify the host? |
b844892 to
179843f
Compare
Hey @fredericbarthelet, sorry about the delay, completely missed this! Will review tomorrow (hopefully we can merge this ASAP). Thanks for the awesome work 🙏 |
There was a problem hiding this comment.
Pull request overview
Adds a new app→host notification that lets a View request its own termination, enabling a bidirectional teardown flow that reuses the existing ui/resource-teardown cleanup mechanism.
Changes:
- Introduces
ui/notifications/request-closein the spec/types and generated schemas. - Adds
App.requestClose()(fire-and-forget) andAppBridge.onrequestclosehandler wiring. - Updates documentation and adds an integration test covering request-close → teardown → cleanup.
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Re-exports the new notification type/schema and adds it to AppNotification. |
| src/spec.types.ts | Defines McpUiRequestCloseNotification in the protocol spec types. |
| src/generated/schema.ts | Adds Zod schema for McpUiRequestCloseNotification. |
| src/generated/schema.test.ts | Adds compile-time type/schema inference assertions for the new schema. |
| src/generated/schema.json | Adds JSON Schema entry for McpUiRequestCloseNotification. |
| src/app.ts | Adds App.requestClose() API to emit the notification. |
| src/app-bridge.ts | Adds onrequestclose setter to handle the notification host-side. |
| src/app-bridge.test.ts | Adds integration test for the close-request initiating teardown flow. |
| specification/draft/apps.mdx | Documents the new notification and updates lifecycle/cleanup diagram. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a2e175e to
289a568
Compare
|
Hey @idosal, thanks for triggering copilot review on this PR. Implemented feedbacks from its comments. |
Fixes #203
Motivation and Context
Implement bi-directionality on UI termination.
Until now
ui/resource-teardownwas the only way for Host to initiate UI teardown.This methods enable UI to initiate its own teardown
Items that might require discussion in this PR:
open-link,message,request-display-mode, the name of the request should be action-first/verb-first. Even if we implement a fire and forget pattern (see next point), I would not includenotificationsin the name. What do you think ofui/close-resource?ui/close-resourcemessage implements a request/response pattern or a fire and forget type of pattern (where UI fires the event like it would fire a notification without ever expecting an answer). I'm leaning towards fire and forget because the host won't be able to send a response if it proceeds accordingly and does indeed terminates the UIui/close-resourcerequests be fulfilled? this can be achieved only if we go for a request/response pattern, so I'd not account for this scenario just yetTypes of changes
Checklist
Additional context