-
Notifications
You must be signed in to change notification settings - Fork 46
api: optional Client in AppBridge (allow custom forwarding), better protocol types (AppRequest, AppNotification, AppResult) #146
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
commit: |
71bb196 to
47258b8
Compare
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.
Overall, I think this looks good. 👍
src/app-bridge.test.ts
Outdated
| expect(receivedRequests).toHaveLength(1); | ||
| expect(result.resources).toEqual([ | ||
| { uri: "test://resource", name: "Test" }, | ||
| ]); |
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.
Missing an assertion on receivedRequests[0] here, but that's probably fine.
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.
Added.
src/app-bridge.test.ts
Outdated
| expect(receivedRequests).toHaveLength(1); | ||
| expect(result.resourceTemplates).toEqual([ | ||
| { uriTemplate: "test://{id}", name: "Test Template" }, | ||
| ]); |
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.
Missing an assertion on receivedRequests[0] here, but that's probably fine.
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.
Added.
src/app-bridge.test.ts
Outdated
| expect(receivedRequests).toHaveLength(1); | ||
| expect(result.prompts).toEqual([{ name: "test-prompt" }]); |
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.
Ditto.
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.
Added.
src/app-bridge.ts
Outdated
| * @param callback - Handler that receives tool call params and returns a result | ||
| * - params.name - Name of the tool to call | ||
| * - params.arguments - Tool arguments | ||
| * - extra - Request metadata (abort signal, session info) | ||
| * - Returns: Promise<CallToolResult> with tool execution result |
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.
I would like this to be better formatted, but I'm not sure of the proper TypeDoc syntax for it. Not a blocker — I can look into it later.
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.
Fixed! Used proper TypeDoc syntax with @param callback.params and @param callback.extra.
src/app-bridge.ts
Outdated
| * @param callback - Handler that receives list params and returns resources | ||
| * - params - Request params (may include cursor for pagination) | ||
| * - extra - Request metadata (abort signal, session info) | ||
| * - Returns: Promise<ListResourcesResult> with available resources |
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.
Likewise for this @param callback. (Not a blocker.)
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.
Fixed.
src/app-bridge.ts
Outdated
| * @param callback - Handler that receives list params and returns templates | ||
| * - params - Request params (may include cursor for pagination) | ||
| * - extra - Request metadata (abort signal, session info) | ||
| * - Returns: Promise<ListResourceTemplatesResult> with available templates |
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.
Ditto. (Not a blocker.)
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.
Fixed.
- Add oncalltool, onlistresources, onlistresourcetemplates, onreadresource, onlistprompts setters for Guest UI → Host requests - Add sendToolListChanged, sendResourceListChanged, sendPromptListChanged methods for Host → Guest UI notifications - Refactor connect() to use new setters with inlined callbacks - Update documentation to clarify optional client parameter behavior - Remove unused forwardRequest/forwardNotification helper methods - Add comprehensive tests for AppBridge without MCP client (manual handlers) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ing git-based installs
Co-authored-by: Jonathan Hefner <[email protected]>
Address PR review feedback: - Factor out test value objects (toolCall, resultContent, etc.) for clearer tests - Add missing assertions on receivedRequests[0] for list handlers - Improve test readability by defining expected values at the top of each test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…methods
- Use proper TypeDoc syntax with `@param callback.params` and `@param callback.extra`
- Add {@link ResultType} references for return types
- Replace `as Notification` with `as const` on method literals where possible
(sendResourceListChanged, sendPromptListChanged)
- Simplify sendHostContextChange cast syntax
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
- Add AppBridgeRequest, AppBridgeNotification, AppBridgeResult type unions that explicitly list all valid types for the protocol - Replace generic SDK Request/Notification/Result with our specific types - Remove all type casts by using `as const` on method literals - Add index signature to McpUiHostContext for forward compatibility This improves type safety and documents exactly what the AppBridge supports. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
9a7461f to
16fb2e4
Compare
Move protocol type unions from app-bridge.ts to types.ts where they belong with other type definitions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
021dab8 to
4653156
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary
Key change:
AppBridge'sclientconstructor parameter is now optional (Client | null). When a client is provided,connect()automatically forwards MCP requests/notifications between the Guest UI and the server. Whennullis passed, you must register handlers manually using the new setters.This enables hosts like mcp-ui to use
AppBridgewithout a direct MCP client reference, registering custom handlers instead.Changes
AppBridge'sclientconstructor parameter optional (Client | null) - hosts can now create anAppBridgewithout an MCP clientoncalltool- handlestools/callrequestsonlistresources- handlesresources/listrequestsonlistresourcetemplates- handlesresources/templates/listrequestsonreadresource- handlesresources/readrequestsonlistprompts- handlesprompts/listrequestssendToolListChanged()- forwards tool list changessendResourceListChanged()- forwards resource list changessendPromptListChanged()- forwards prompt list changesconnect()to use the new setters with inlined callbacks when a client is providedforwardRequest/forwardNotificationhelper methodsnullclient caseType Safety Improvements
AppRequest,AppNotification,AppResulttype unions that document exactly what types are valid in the MCP Apps protocolAppandAppBridgenow extendProtocol<AppRequest, AppNotification, AppResult>instead of using the generic SDK typesas conston method literals, eliminating the need foras NotificationcastsMcpUiHostContextto allow unknown propertiesDocumentation & Test Improvements
@param callback.params/@param callback.extrasyntaxUsage
With MCP client (automatic forwarding):
Without MCP client (manual handlers):
Test plan
🤖 Generated with Claude Code