feat: Allow expanding MCP call arguments #11207
Conversation
That is, during confirmation, the arguments to the MCP call can now be expanded using CTRL+s. Ref google-gemini#11031.
Summary of ChangesHello @janEbert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the CLI's MCP tool call confirmation process by allowing users to inspect the arguments passed to an MCP server. By pressing "Ctrl+S", users can now toggle the detailed JSON view of the arguments, providing greater transparency and control before confirming a tool's execution. This addresses issue #11031, improving the user experience for tool invocation. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature for expanding MCP call arguments during confirmation. Your concern about the keybinding conflict with SHOW_MORE_LINES is valid, as the current implementation introduces a side effect where expanding arguments also expands the entire history view. I've included review comments with suggestions to resolve this issue and improve the overall consistency of the implementation.
packages/cli/src/ui/AppContainer.tsx
Outdated
| (keyMatchers[Command.SHOW_MORE_LINES](key) || | ||
| keyMatchers[Command.TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION](key)) && |
There was a problem hiding this comment.
You were right to be concerned about the interplay with SHOW_MORE_LINES. As it's currently implemented, when a user presses Ctrl+S in the MCP confirmation dialog, it will correctly toggle the arguments' visibility, but it will also trigger this global key handler, causing setConstrainHeight(false) to be called. This expands the main history view, which is likely an unwanted side effect.
The key press should be handled exclusively by the ToolConfirmationMessage component when it is focused. To fix this, we can remove the check for TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION from this global handler. The useKeypress hook in ToolConfirmationMessage will still capture and handle the event correctly for toggling the arguments.
keyMatchers[Command.SHOW_MORE_LINES](key) &&
| if (!isFocused) return; | ||
| if (key.name === 'escape' || (key.ctrl && key.name === 'c')) { | ||
| handleConfirm(ToolConfirmationOutcome.Cancel); | ||
| } else if (key.ctrl && key.name === 's') { |
There was a problem hiding this comment.
To improve consistency and maintainability, it's better to use the centralized keyMatchers for keybinding checks instead of hardcoding the key combination. This ensures that if the keybinding for TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION is ever changed in keyBindings.ts, it will be automatically reflected here. This also makes the purpose of defining the new Command and its keybinding clearer.
You'll also need to add import { keyMatchers, Command } from '../../keyMatchers.js'; at the top of the file.
| } else if (key.ctrl && key.name === 's') { | |
| } else if (keyMatchers[Command.TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION](key)) { |
Summary of ChangesHello @janEbert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature to the CLI that allows users to inspect the arguments passed to an MCP server during the tool call confirmation process. By pressing "Ctrl+S", users can toggle the display of these arguments, providing greater transparency and control before executing a tool. This enhancement aims to improve the user experience by offering more detailed information at a critical decision point. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature for expanding MCP tool call arguments. My review focuses on the keybinding implementation. You correctly identified a potential conflict with the shared CTRL+s shortcut. I've found a logic issue in how this conflict is handled in AppContainer.tsx and a consistency issue in ToolConfirmationMessage.tsx. The suggested changes should resolve the conflict cleanly by isolating the new keybinding's effect to its component and adhering to the project's centralized key-matching pattern.
| } else if (key.ctrl && key.name === 's') { | ||
| setIsExpanded((prev) => !prev); | ||
| } |
There was a problem hiding this comment.
The keypress for toggling argument expansion is hardcoded as key.ctrl && key.name === 's'. This is inconsistent with the rest of the application, which uses a centralized system for managing keybindings (keyBindings.ts) and matching them (keyMatchers.ts). You've already added the TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION command to this system, and it should be used here to maintain consistency and configurability.
You will need to import keyMatchers and Command from ../../keyMatchers.js.
| } else if (key.ctrl && key.name === 's') { | |
| setIsExpanded((prev) => !prev); | |
| } | |
| } else if (keyMatchers[Command.TOGGLE_TOOL_CALL_ARGUMENTS_EXPANSION](key)) { | |
| setIsExpanded((prev) => !prev); | |
| } |
There was a problem hiding this comment.
This is in line with the if-query in the lines right above the change, which also checks for (key.name === 'escape' || (key.ctrl && key.name === 'c')). Should both occurrences be adapted? If yes, what should the other command(s) be?
1 participant
TLDR
During confirmation, the arguments to the MCP call can now be expanded using CTRL+s. I'm a bit worried about the interplay with the
Command.SHOW_MORE_LINES(inkeyBindings.ts), which shares the same keybinding, but was unable to produce a test case.Reviewer Test Plan
Ask the model to execute any of your configured MCP servers that takes arguments; then press CTRL+s to toggle display of the arguments.
Testing Matrix
Linked issues / bugs