Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| beforeEach(() => { | ||
| vi.stubGlobal("parent", { postMessage: mockPostMessage }); | ||
| vi.stubGlobal("skybridge", { hostType: "mcp-app" }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| vi.resetAllMocks(); | ||
| McpAppBridge.resetInstance(); | ||
| }); | ||
|
|
||
| it("should return a function that sends ui/message request to the MCP host", () => { | ||
| const { result } = renderHook(() => useSendFollowUpMessage()); | ||
|
|
||
| const prompt = "Test message"; | ||
| result.current(prompt); | ||
|
|
||
| expect(mockPostMessage).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| jsonrpc: "2.0", | ||
| method: "ui/message", | ||
| params: expect.objectContaining({ | ||
| role: "user", | ||
| content: [ | ||
| { | ||
| type: "text", | ||
| text: prompt, | ||
| }, | ||
| ], | ||
| }), | ||
| }), | ||
| "*", | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
No test coverage for scrollToBottom in MCP app host
The MCP app host test block only covers the baseline case (no options). There is no test verifying the behaviour when scrollToBottom is passed in the MCP host context — which would immediately reveal that the option is silently dropped by McpAppAdaptor. Adding a test here would have caught the issue in mcp-app/adaptor.ts and made the gap explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/web/hooks/use-send-follow-up-message.test.ts
Line: 62-98
Comment:
**No test coverage for `scrollToBottom` in MCP app host**
The MCP app host test block only covers the baseline case (no options). There is no test verifying the behaviour when `scrollToBottom` is passed in the MCP host context — which would immediately reveal that the option is silently dropped by `McpAppAdaptor`. Adding a test here would have caught the issue in `mcp-app/adaptor.ts` and made the gap explicit.
How can I resolve this? If you propose a fix, please make it concise.| - `options?: { scrollToBottom?: boolean }` | ||
| - **Optional** | ||
| - Configuration options for the follow-up message | ||
| - `scrollToBottom`: When `true` (default), automatically scrolls to the bottom of the conversation after posting the message. Set to `false` to prevent auto-scroll. |
There was a problem hiding this comment.
Default value documented but not enforced in code
The docs state scrollToBottom defaults to true, but no explicit default is set in the implementation. The actual default behaviour falls through to whatever window.openai.sendFollowUpMessage does when scrollToBottom is absent from the payload. If the underlying API ever changes its default, the documented behaviour would silently diverge. Consider either:
- Making the default explicit in code:
options = { scrollToBottom: true }, or - Documenting this as "the host default applies" to clarify the dependency on the underlying API.
| - `scrollToBottom`: When `true` (default), automatically scrolls to the bottom of the conversation after posting the message. Set to `false` to prevent auto-scroll. | |
| - `scrollToBottom`: When `true`, automatically scrolls to the bottom of the conversation after posting the message. Set to `false` to prevent auto-scroll. Defaults to the host platform's default behavior when not specified. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/api-reference/use-send-follow-up-message.mdx
Line: 44
Comment:
**Default value documented but not enforced in code**
The docs state `scrollToBottom` defaults to `true`, but no explicit default is set in the implementation. The actual default behaviour falls through to whatever `window.openai.sendFollowUpMessage` does when `scrollToBottom` is absent from the payload. If the underlying API ever changes its default, the documented behaviour would silently diverge. Consider either:
- Making the default explicit in code: `options = { scrollToBottom: true }`, or
- Documenting this as "the host default applies" to clarify the dependency on the underlying API.
```suggestion
- `scrollToBottom`: When `true`, automatically scrolls to the bottom of the conversation after posting the message. Set to `false` to prevent auto-scroll. Defaults to the host platform's default behavior when not specified.
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
The Looking at how the analogous unsupported option is handled in If Prompt To Fix With AIThis is a comment left during a code review.
Path: packages/core/src/web/bridges/mcp-app/adaptor.ts
Line: 110-127
Comment:
**`scrollToBottom` option is silently ignored for MCP app hosts**
The `options` parameter is accepted in the function signature but never used — `scrollToBottom` is not forwarded to the `ui/message` bridge request. This means any developer calling `sendFollowUpMessage("...", { scrollToBottom: false })` in an MCP app host will get no effect silently, which is likely to cause confusion.
Looking at how the analogous unsupported option is handled in `openExternal` (line ~131 of this same file), a `console.warn` is emitted when `redirectUrl` is used on MCP. The same pattern should apply here if `scrollToBottom` is truly unsupported by the `ui/message` protocol:
```suggestion
public sendFollowUpMessage = async (
prompt: string,
options?: { scrollToBottom?: boolean },
) => {
if (options?.scrollToBottom !== undefined) {
console.warn(
"[skybridge] scrollToBottom option is not supported by the MCP ui/message protocol and will be ignored.",
);
}
const bridge = McpAppBridge.getInstance();
await bridge.request<McpUiMessageRequest, McpUiMessageResult>({
method: "ui/message",
params: {
role: "user",
content: [
{
type: "text",
text: prompt,
},
],
},
});
};
```
If `McpUiMessageRequest` does support `scrollToBottom`, it should be forwarded instead.
How can I resolve this? If you propose a fix, please make it concise. |
1 participant
Description
Extended the
useSendFollowUpMessagehook to support an optionalscrollToBottomparameter, aligning with the Apps SDK API update. This parameter allows developers to control whether the conversation view automatically scrolls to the bottom after sending a follow-up message.The implementation:
scrollToBottomparameterAdaptorinterface to accept the new options parameterAppsSdkAdaptorandMcpAppAdaptorimplementations to handle the parameteruseSendFollowUpMessagehook to pass through the options to the underlying adaptorCloses #512.
The code in this pull request was generated by GitHub Copilot with the Claude Sonnet 4.5 model.
Checklist if Applicable
pnpm test src/web/hooks/use-send-follow-up-message.test.tspnpm biome cion modified filesGreptile Summary
This PR extends
useSendFollowUpMessagewith an optionalscrollToBottomparameter, updating theAdaptorinterface, both adaptor implementations, the hook itself, tests, and documentation. The Apps SDK path is implemented correctly, but the MCP app adaptor accepts the parameter and silently discards it — unlike the existing pattern in the same file where unsupported options emit aconsole.warn. The test suite has good coverage for the Apps SDK host but misses a test for the MCP host withscrollToBottom, which would have exposed this gap. The documentation also claimstrueis the default value, but no explicit default is set in code.McpAppAdaptor:scrollToBottomis accepted but never forwarded to theui/messagebridge request. No warning is emitted (contrast withopenExternal's handling ofredirectUrl).scrollToBottomtest case is absent.scrollToBottomdefaults totrue, but the code relies on the underlyingwindow.openaiAPI's default rather than enforcing one explicitly.Confidence Score: 2/5
scrollToBottomoption is silently ignored for MCP app hosts, making the feature incomplete and potentially misleading for that platform.scrollToBottomwithout forwarding it or warning the caller. The PR description says both adaptors "handle the parameter", which is inaccurate. This constitutes a broken contract for MCP app consumers and should be resolved before merging.packages/core/src/web/bridges/mcp-app/adaptor.tsrequires a fix — either forwardscrollToBottomto the bridge request (if the MCP protocol supports it) or emit aconsole.warnthat it is unsupported (matching the existingopenExternalpattern).Last reviewed commit: 1d07428
(5/5) You can turn off certain types of comments like style here!