fix(patch): cherry-pick 1611364 to release/v0.13.0-preview.1-pr-12587 to patch version v0.13.0-preview.1 and create version 0.13.0-preview.2#12601
Conversation
Co-authored-by: LayorX <yor31117@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary of ChangesHello @gemini-cli-robot, 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 focuses on enhancing the stability and reliability of the application, particularly concerning pseudo-terminal (PTY) interactions and the test suite. It introduces more resilient error handling for PTY resizing to prevent crashes, especially in race conditions where a PTY might exit unexpectedly. Additionally, it refines the testing environment by adjusting timeouts and conditionally skipping platform-specific tests, ensuring a more robust and consistent testing experience across different operating systems. This is an automated patch to update the preview release version. 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 cherry-picks a fix for a crash that can occur when resizing a pseudo-terminal (pty) that has already exited. The fix involves adding error handling to gracefully manage this race condition. While the intention is correct, the implementation introduces redundant error handling in the AppContainer component, which is already handled within the ShellExecutionService. My review focuses on removing this duplication to improve code encapsulation and maintainability. Other changes include increasing test timeouts to address flakiness and skipping some platform-specific tests, which are reasonable.
| try { | ||
| ShellExecutionService.resizePty( | ||
| activePtyId, | ||
| Math.floor(terminalWidth * SHELL_WIDTH_FRACTION), | ||
| Math.max( | ||
| Math.floor(availableTerminalHeight - SHELL_HEIGHT_PADDING), | ||
| 1, | ||
| ), | ||
| ); | ||
| } catch (e) { | ||
| // This can happen in a race condition where the pty exits | ||
| // right before we try to resize it. | ||
| if ( | ||
| !( | ||
| e instanceof Error && | ||
| e.message.includes('Cannot resize a pty that has already exited') | ||
| ) | ||
| ) { | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
This try...catch block appears to be redundant. The ShellExecutionService.resizePty method already contains logic to catch and handle this specific error, as shown in packages/core/src/services/shellExecutionService.ts. Adding another layer of error handling here couples the UI component to the implementation details of the service and duplicates the logic. The service should be responsible for handling its own internal race conditions. I recommend removing this try...catch and letting resizePty handle the error gracefully.
ShellExecutionService.resizePty(
activePtyId,
Math.floor(terminalWidth * SHELL_WIDTH_FRACTION),
Math.max(
Math.floor(availableTerminalHeight - SHELL_HEIGHT_PADDING),
1,
),
);
| describe('Shell Interaction', () => { | ||
| it('should not crash if resizing the pty fails', async () => { | ||
| const resizePtySpy = vi | ||
| .spyOn(ShellExecutionService, 'resizePty') | ||
| .mockImplementation(() => { | ||
| throw new Error('Cannot resize a pty that has already exited'); | ||
| }); | ||
|
|
||
| mockedUseGeminiStream.mockReturnValue({ | ||
| streamingState: 'idle', | ||
| submitQuery: vi.fn(), | ||
| initError: null, | ||
| pendingHistoryItems: [], | ||
| thought: null, | ||
| cancelOngoingRequest: vi.fn(), | ||
| activePtyId: 'some-pty-id', // Make sure activePtyId is set | ||
| }); | ||
|
|
||
| // The main assertion is that the render does not throw. | ||
| const { unmount } = render( | ||
| <AppContainer | ||
| config={mockConfig} | ||
| settings={mockSettings} | ||
| version="1.0.0" | ||
| initializationResult={mockInitResult} | ||
| />, | ||
| ); | ||
|
|
||
| await act(async () => { | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); | ||
| }); | ||
|
|
||
| expect(resizePtySpy).toHaveBeenCalled(); | ||
| unmount(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test suite should be removed. It validates that AppContainer doesn't crash when resizePty throws, but it does so by mocking resizePty itself. This forces the redundant try...catch block in AppContainer.tsx. The correct behavior (handling the resize error within the service) is already tested at the service level in packages/core/src/services/shellExecutionService.test.ts. By removing this test, we can also remove the duplicated error-handling logic from the AppContainer component, leading to cleaner and more maintainable code.
|
Size Change: +229 B (0%) Total Size: 20.4 MB ℹ️ View Unchanged
|
skeshive
deleted the
hotfix/v0.13.0-preview.1/0.13.0-preview.2/preview/cherry-pick-1611364/pr-12587
branch
4 participants
This PR automatically cherry-picks commit 1611364 to patch version v0.13.0-preview.1 in the preview release to create version 0.13.0-preview.2.