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 introduces a significant upgrade to the application's integrated terminal by replacing the existing 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
|
There was a problem hiding this comment.
Code Review
This pull request replaces xterm.js with ghostty-web for the terminal component, which is a significant and welcome improvement for reliability and usability. The refactoring also includes performance enhancements for terminal tab switching. While the changes are generally positive, I've identified a critical bug that causes an infinite render loop, a high-severity security issue with the Content Security Policy, and a couple of medium-severity issues related to error handling and potential performance optimizations. Please review the comments for details.
Good points, addressed 👍 |
|
@jutaz |
|
Taking a look in an hour or so - sorry about that! Focused on layout too much 😂 |
|
@jutaz |
| ghostty.onResize(({ cols, rows }) => { | ||
| void api.resizeTerminal(terminalId, cols, rows); | ||
| }); | ||
| }, [terminalId, api, ghosttyTerminal]); |
There was a problem hiding this comment.
CRITICAL: Memory leak - event handlers not cleaned up
The onData and onResize event handlers registered on the ghostty terminal are never removed. This will cause memory leaks when the component unmounts or when dependencies change.
The effect should return a cleanup function that removes these handlers. Most terminal libraries provide an unsubscribe mechanism or disposable pattern for event handlers.
|
|
||
| void createTerminal(); | ||
| }, [baseDir, terminalId, visible, api, taskId]); | ||
| }, [baseDir, terminalId, visible, api, taskId, ghosttyTerminal]); |
There was a problem hiding this comment.
WARNING: Missing dependency in useEffect
The fitAddon is used inside the effect (line 294) but is not included in the dependency array. This could cause the effect to use a stale reference to fitAddon.
Add fitAddon to the dependency array: [baseDir, terminalId, visible, api, taskId, ghosttyTerminal, fitAddon]
| } | ||
|
|
||
| .xterm-viewport { | ||
| @apply scrollbar-thin scrollbar-track-bg-primary scrollbar-thumb-bg-secondary-light hover:scrollbar-thumb-bg-tertiary; |
There was a problem hiding this comment.
WARNING: Stale CSS selector after library migration
The .xterm-viewport class was specific to the xterm.js library which has been replaced with ghostty-web. This selector will no longer match any elements and should be updated to target ghostty-web's viewport class or removed if no longer needed.
Check ghostty-web documentation for the correct class names.
|
|
||
| .xterm-screen { | ||
| @apply px-3; | ||
| @apply pl-3 pr-0; |
There was a problem hiding this comment.
WARNING: Stale CSS selector after library migration
The .xterm-screen class was specific to the xterm.js library which has been replaced with ghostty-web. This selector will no longer match any elements and should be updated to target ghostty-web's screen class or removed if no longer needed.
Check ghostty-web documentation for the correct class names.
Code Review SummaryStatus: 2 New Issues Found | Recommendation: Address before merge Overview
New Issue Details (click to expand)CRITICAL
WARNING
Previously Reported Issues (8 active)
Files Reviewed (4 files)
|
| } | ||
| }; | ||
| }, [terminalId, api]); | ||
| }, [taskId, visible, fitAddon]); |
There was a problem hiding this comment.
CRITICAL: Circular dependency causing infinite re-renders
The useLayoutEffect at line 124 includes fitAddon in its dependency array (line 272), but this same effect creates and sets fitAddon via setFitAddon(fitAddonInstance) at line 219. This creates a circular dependency:
- Effect runs → creates new
fitAddon→ callssetFitAddon fitAddonstate changes → triggers effect again- Loop repeats infinitely
Impact: Infinite re-renders, memory leaks from repeatedly creating/disposing terminal instances, poor performance.
Solution: Remove fitAddon from the dependency array. The effect should only run when taskId or visible changes, not when fitAddon changes (since the effect itself creates it).
| }, [taskId, visible, fitAddon]); | |
| }, [taskId, visible]); |
|
|
||
| void createTerminal(); | ||
| }, [baseDir, terminalId, visible, api, taskId]); | ||
| }, [baseDir, terminalId, visible, api, taskId, ghosttyTerminal]); |
There was a problem hiding this comment.
WARNING: Missing fitAddon dependency in useLayoutEffect
The effect uses fitAddon at line 293-295 but doesn't include it in the dependency array. This violates the React hooks exhaustive-deps rule and could cause the effect to use a stale fitAddon reference.
Solution: Add fitAddon to the dependency array:
| }, [baseDir, terminalId, visible, api, taskId, ghosttyTerminal]); | |
| }, [baseDir, terminalId, visible, api, taskId, ghosttyTerminal, fitAddon]); |




Adds
ghosttyviaghostty-webinstead of xterm. This should increase reliability and usability of the inbuilt terminal.