feat(cli): support Ctrl-Z suspension#17630
Conversation
Summary of ChangesHello @brtkwr, 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 significantly enhances the CLI's usability by enabling the standard 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 support for suspending the CLI process using Ctrl+Z. While the overall implementation is robust, a high-severity code review comment has been raised regarding a type assertion in AppContainer.tsx that bypasses TypeScript's type safety, suggesting a more robust typing approach.
packages/cli/src/ui/AppContainer.tsx
Outdated
| enableMouseEvents(); | ||
| disableLineWrapping(); | ||
| app.rerender(); | ||
| (app as unknown as { rerender: () => void }).rerender(); |
There was a problem hiding this comment.
The type assertion (app as unknown as { rerender: () => void }).rerender(); is used here to call the rerender method. While functional, this bypasses TypeScript's type safety. It would be more robust to ensure that the app object returned by useApp() is correctly typed to include the rerender method, or to import useApp from a version that includes it. This helps maintain type safety and code clarity.
1778767 to
254426d
Compare
|
Just rebased the PR and ran all the tests locally which appear to be passing. |
scidomino
left a comment
There was a problem hiding this comment.
This will probably create a giant trap for users if they are already used to hitting ctrl+z to undo. What I suggest is that do this in stages.
First, we support ctrl+z but you have to hit it twice and after you hit it the first time we pop up a message saying something like press "ctrl-Z again to suspend, use CMD-Z to undo". Then after running with that for a couple weeks we can remove the toast and just suspend on the first ctrl+z.
packages/cli/src/ui/AppContainer.tsx
Outdated
| setCtrlDPressCount((prev) => prev + 1); | ||
| return true; | ||
| } else if (keyMatchers[Command.SUSPEND](key)) { | ||
| if (process.platform !== 'win32') { |
There was a problem hiding this comment.
AppContainer is gigantic. Please move this into another file. Have Gemini do a thorough search to see if there are any files it makes sense to move this code to before deciding to create a new one.
There was a problem hiding this comment.
fixed - moved suspend behavior into a dedicated useSuspend hook to keep AppContainer smaller.
| } | ||
|
|
||
| const MAC_ALT_KEY_CHARACTER_MAP: Record<string, string> = { | ||
| const MAC_ALT_KEY_CHARACTER_MAP: Record< |
There was a problem hiding this comment.
I've broken this part out into a separate PR. Let's merge mine first and then you won't even have to touch this file #17800
There was a problem hiding this comment.
Thanks for the update, scidomino! That sounds like a good plan. If your PR is merged first, then @brtkwr can rebase this branch to remove these changes from packages/cli/src/ui/contexts/KeypressContext.tsx.
There was a problem hiding this comment.
Amazing thanks for doing that!
There was a problem hiding this comment.
updated - rebased and synced with upstream so this PR no longer carries the extra keypress-context changes from before #17800.
|
Actually, I will make the toast message part of my PR so hold off till that is merged. |
|
I merged my changes. Sorry if it's a pain but it should make your PR smaller ultimately. You just need to impl the "press ctrl+z twice" logic and the actual suspension. ping my handle in a comment when you are ready for review. |
fedb3ae to
f0b1e13
Compare
I've addressed the changes that you requested. |
|
Unfortunately, it has made the PR slightly larger, not smaller 🙃 |
18c4cf0 to
71af62c
Compare
scidomino
left a comment
There was a problem hiding this comment.
Sorry for the delay. I left some comments.
| } | ||
| if (ctrlZPressCount > 1) { | ||
| setCtrlZPressCount(0); | ||
| if (process.platform !== 'win32') { |
There was a problem hiding this comment.
Why do we only clean up if we're not in windows?
There was a problem hiding this comment.
good point - i added an explicit windows guard with a warning so cleanup/suspend logic only runs on supported platforms.
| if (ctrlZPressCount > 1) { | ||
| setCtrlZPressCount(0); | ||
| if (process.platform !== 'win32') { | ||
| // Cleanup before suspend |
There was a problem hiding this comment.
we have a private cleanupOnExit method in TerminalCapabilityManager, make it an exported function and call it here. If you need to add the "Show cursor" command to it, do it.
There was a problem hiding this comment.
fixed - exported terminal cleanup (cleanupTerminalOnExit) from TerminalCapabilityManager and reused it from suspend handling.
| } | ||
| setRawMode(true); | ||
|
|
||
| terminalCapabilityManager.enableKittyProtocol(); |
There was a problem hiding this comment.
use terminalCapabilityManager.enableSupportedModes(). We don't want to enable this if it's not supported.
There was a problem hiding this comment.
fixed - resume path now calls terminalCapabilityManager.enableSupportedModes().
| } | ||
| } | ||
|
|
||
| disableKittyProtocol() { |
There was a problem hiding this comment.
Remove these unless you have a good reason not to use enableSupportedModes
There was a problem hiding this comment.
done - removed the dedicated kitty enable/disable helpers and switched to enableSupportedModes on resume.
| @@ -0,0 +1,460 @@ | |||
| /** | |||
There was a problem hiding this comment.
There is nothing GCLI loves more than introducing new test files for classes that already have test files :D
Can you move this test into AppContainer.test.tsx? Only keep it separate if it leads to less overall code
There was a problem hiding this comment.
fixed - moved suspend coverage into AppContainer.test.tsx and removed the separate AppContainer.suspend.test.tsx file.
|
|
||
| beforeEach(() => { | ||
| vi.resetAllMocks(); | ||
| vi.stubEnv('GOOGLE_CLOUD_PROJECT_ID', ''); |
There was a problem hiding this comment.
Are these changes necessary? I can run the tests just fine without them.
There was a problem hiding this comment.
removed - dropped those setup-test env stubs to keep this PR scoped to suspend behavior.
|
When you've addressed these ping me by name with the @ symbol so I get to it right away. |
|
@scidomino i addressed all open review comments and pushed updates to this PR. ready for another pass when you have a minute. |
53e738d to
be3e79e
Compare
|
I see there are merge conflicts. I will attempt to resolve them for you. |
|
Looks like you beat me to it. |
👯♂️ |
|
Heads up: #18670 is about to be merged which duplicates some of the work you did here. I think it may have also solve some of the issues here because it has protections against registering the same listener more than once. |
|
@scidomino addressed the suspend/resume issues from your latest review in 822bfe1:
please take another look when you get a chance. |
I see the same behaviour in other terminal based coding agents. why does it need to clear the screen on suspend? |
|
Ok. Approved. I will attempt to resolve the branch conflicts |
|
I don't seem to have permission to push to your fork so you will have to resolve the merge conflicts. Sorry. |
|
@scidomino rebased |
|
Uhg. More conflicts. Ok. I am going to clone this and see if I can merge it myself. Sorry for all these delays. |
|
duplicated to #18931 |
|
merged as part of #18931 |




Restores Ctrl+Z functionality to suspend the CLI process.
Resolves keybinding conflicts and refines suspend behavior.
Fixes #5018
Summary
Details
Related Issues
How to Validate
Pre-Merge Checklist