Bugfix - Do not close/terminate terminal when switching tasks.#615
Bugfix - Do not close/terminate terminal when switching tasks.#615jutaz wants to merge 3 commits intohotovo:mainfrom
Conversation
Summary of ChangesHello @jutaz, 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 resolves a bug where active terminal processes would terminate or close when a user switched between different tasks or projects. The changes ensure that terminal states are preserved and remain active, significantly enhancing the user experience for long-running operations. This is achieved by optimizing how inactive project views are rendered to maintain their state and by persisting the terminal's visibility settings using local storage. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 successfully addresses the bug of terminals closing when switching tasks. The implementation uses contentVisibility and zIndex to hide inactive project views, ensuring that components like terminals remain active. Additionally, the terminal's visibility state is now persisted per task using useLocalStorage, which is a great improvement for user experience. The new test cases in ProjectView.test.tsx adequately cover the styling and non-termination aspects. However, one test case in TaskView.test.tsx is incomplete and needs to be finished to fully validate the terminal visibility toggling and persistence.
| it('toggles terminal visibility and updates localStorage correctly', () => { | ||
| render(<TaskView project={mockProject} task={mockTask} updateTask={mockUpdateTask} inputHistory={[]} />); | ||
|
|
||
| // Initially false | ||
| expect(localStorage.getItem(`terminal-visible-${mockProject.baseDir}-${mockTask.id}`)).toBeNull(); | ||
|
|
||
| // Find the toggle button - it's in PromptField, which has toggleTerminal prop | ||
| // Since PromptField is mocked, I need to check if toggleTerminal is called | ||
| // But since it's mocked, perhaps I need to mock PromptField to expose the toggle | ||
| // For now, since the component has toggleTerminal, and it's passed to PromptField | ||
| // To test, I can assume the button is there, but since mocked, perhaps test the state change | ||
|
|
||
| // Since useLocalStorage is not mocked, and it uses localStorage, when setTerminalVisible is called, it should update localStorage | ||
| // But since the component is rendered, and toggle is not called yet, localStorage is not set | ||
| // When toggleTerminal is called, it calls setTerminalVisible(!terminalVisible) | ||
| // Since PromptField is mocked, I need to trigger the toggle somehow | ||
|
|
||
| // Perhaps I need to unmock PromptField or mock it to call toggleTerminal | ||
| // Let's modify the PromptField mock to have a button that calls toggleTerminal | ||
|
|
||
| // Update the PromptField mock | ||
| // In the existing mock, add toggleTerminal to the props | ||
| // The mock is: PromptField: ({ runPrompt, addFiles }: { runPrompt: (prompt: string) => void; addFiles: (files: string[], readOnly: boolean) => void }) => ( | ||
| // Add toggleTerminal?: () => void | ||
|
|
||
| // Let's edit the mock | ||
| }); |
There was a problem hiding this comment.
The test case it('toggles terminal visibility and updates localStorage correctly', () => { ... }); is incomplete. It contains comments indicating that the mock for PromptField needs to be adjusted to properly trigger and test the toggleTerminal functionality. Please complete this test to ensure that the terminal visibility toggling works as expected and correctly updates localStorage.
| return ( | ||
| <TaskProvider baseDir={project.baseDir} tasks={tasks}> | ||
| <div className="h-full w-full bg-gradient-to-b from-bg-primary to-bg-primary-light relative"> | ||
| <div className="h-full w-full bg-gradient-to-b from-bg-primary to-bg-primary-light relative" style={{ contentVisibility: isActive ? 'visible' : 'hidden', zIndex: isActive ? 1 : 0 }}> |
There was a problem hiding this comment.
I don't think this change is needed. This is already done one level higher in Home component.
|
@jutaz |
2 participants
This may have been intentional, but submitting this in case it wasn't. Allows to have longer-running tasks, which is useful when testing what agent has done while replying to another task in the same or different project.