feat: enhance terminal with WebGL, ligatures, images, and themes - #1
Conversation
- Add WebGL renderer addon for GPU-accelerated rendering - Add ligatures addon for programming font support (Fira Code, JetBrains Mono) - Add image addon for sixel/iTerm2 inline image support - Add 11 terminal theme presets (VS Code, Dracula, Nord, Monokai, etc.) - Add theme picker UI in terminal toolbar - Add performance optimizations (fast scroll, optimized settings) - Fix React StrictMode double-mount issue in PTY spawning - Persist theme preference to localStorage
📝 WalkthroughWalkthroughAdds terminal theming and a theme picker, persists theme in the editor store, refactors terminal init to load WebGL/Ligatures/Image addons and use ResizeObserver, wires PTY I/O to the terminal, deduplicates PTY spawn and ensures kill-on-remove, and adds three xterm addon dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TerminalTabs as TerminalTabs (UI)
participant EditorStore as EditorStore
participant useTerminal as useTerminal Hook
participant XTerm as XTerm Instance
rect rgb(240,248,255)
Note over User,XTerm: Theme selection flow
User->>TerminalTabs: Open dropdown & select theme
TerminalTabs->>EditorStore: setTerminalTheme(name)
EditorStore->>useTerminal: notify subscription (theme changed)
useTerminal->>XTerm: terminal.setOption(theme: ITheme)
XTerm->>XTerm: apply theme options
end
sequenceDiagram
participant React as Component
participant useTerminal as useTerminal Hook
participant EditorStore as EditorStore
participant PTY as PTY Backend
participant XTerm as XTerm Instance
rect rgb(240,248,255)
Note over React,XTerm: Terminal init, addons, PTY wiring
React->>useTerminal: initTerminal(containerId)
useTerminal->>EditorStore: read terminalTheme
useTerminal->>XTerm: create instance with theme/options
useTerminal->>XTerm: load addons (WebGL, Ligatures, Image, Fit, WebLinks, Search)
useTerminal->>useTerminal: attach ResizeObserver → fit + sync size
useTerminal->>PTY: spawn_shell (entry check)
PTY-->>useTerminal: stream events
useTerminal->>XTerm: on PTY data → terminal.write()
XTerm->>PTY: on user input → PTY.write()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/commands/pty.rs (1)
154-162: Explicitly terminate the child process before removing the PTY instance.
kill_ptyremoves the instance from the HashMap but does not explicitly callchild.kill()orchild.wait(). Theportable_ptycrate does not guarantee process termination on Drop—you must explicitly call these methods to reap or terminate the spawned shell process. Without this, child processes become orphaned and accumulate over time.Implement a custom Drop for
PtyInstancethat callschild.kill(), or modifykill_ptyto explicitly terminate the child before removal:pub fn kill_pty(id: String) -> Result<(), String> { let mut instances = PTY_INSTANCES.lock().unwrap(); if let Some(mut instance) = instances.remove(&id) { // Explicitly kill the child process let _ = instance.child.kill(); Ok(()) } else { Err(format!("PTY instance not found: {}", id)) } }
🧹 Nitpick comments (3)
src/lib/terminalThemes.ts (1)
281-298: LGTM! Well-structured theme system.The theme infrastructure is clean:
getThemeprovides a safe fallback for invalid theme namesthemeDisplayNamesprovides user-friendly labels for the UIthemeNamesenables iteration over available themesFor stronger type safety, you could derive the theme name type from the themes object to ensure
themeDisplayNamesstays in sync:🔎 Optional type-safe enhancement
// At the top of the file, after themes definition: export type ThemeName = keyof typeof themes; export const themeDisplayNames: Record<ThemeName, string> = { // ... existing entries }; export const getTheme = (name: string): ITheme => themes[name as ThemeName] ?? themes['vscode-dark']; export const themeNames = Object.keys(themes) as ThemeName[];src/components/Terminal/TerminalTabs.tsx (1)
119-167: Theme picker UI looks good; consider accessibility improvements.The dropdown implementation is solid with proper click-outside handling and keyboard support. For improved accessibility, consider adding ARIA attributes.
🔎 Optional accessibility enhancements
<button ref={themeButtonRef} className={`h-full px-3 transition-colors ${...}`} onClick={() => setThemeDropdownOpen(!themeDropdownOpen)} title="Terminal Theme" + aria-haspopup="listbox" + aria-expanded={themeDropdownOpen} ><div ref={themeDropdownRef} className="absolute right-0 bottom-full mb-1 w-48 ..." + role="listbox" + aria-label="Terminal themes" > {themeNames.map((name) => ( <button key={name} className={`w-full px-3 py-1.5 text-left text-xs ...`} onClick={() => {...}} + role="option" + aria-selected={terminalTheme === name} >src-tauri/src/commands/pty.rs (1)
86-110: Consider coordination between reader thread cleanup and kill_pty.The reader thread cleans up
PTY_INSTANCESon exit (line 109), andkill_ptyalso removes entries (line 157). Ifkill_ptyis called while the reader thread is active, there's no explicit coordination:
kill_ptyremoves the entry- Reader thread continues reading until it encounters an error
- Reader thread attempts to remove an already-removed entry (which is safe, but redundant)
This is not a bug—
HashMap::removeis idempotent and the read loop will eventually terminate. However, for cleaner shutdown, you might consider signaling the reader thread to exit whenkill_ptyis called.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc-tauri/src/commands/pty.rssrc/components/Terminal/TerminalTabs.tsxsrc/hooks/useTerminal.tssrc/lib/terminalThemes.tssrc/stores/editorStore.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/Terminal/TerminalTabs.tsx (1)
src/lib/terminalThemes.ts (2)
themeNames(298-298)themeDisplayNames(281-293)
src/stores/editorStore.ts (2)
src/types/index.ts (5)
RemoteConnection(17-25)FileEntry(2-8)GitStatus(87-92)GitBranches(107-111)GitCommit(94-99)src/components/Git/GitBranches.tsx (1)
GitBranches(35-89)
🔇 Additional comments (5)
src/stores/editorStore.ts (1)
31-31: LGTM!The terminal theme state management is well-structured:
- State field properly typed as
string- Setter action is straightforward
- Persistence via
partializeensures theme preference survives page reloads- Default
"vscode-dark"aligns with the fallback ingetTheme()Also applies to: 86-86, 136-136, 416-416, 693-693
src/hooks/useTerminal.ts (3)
38-51: LGTM on terminal configuration.The configuration is well-thought-out:
- Font stack with ligature-capable fonts
allowProposedApi: trueenables ligatures addon- Performance options (
fastScrollModifier,smoothScrollDuration: 0) align with PR goals
67-76: Good defensive handling for WebGL fallback.The try/catch with context loss handler ensures graceful degradation to canvas rendering when WebGL isn't available or fails.
145-153: Theme subscription correctly integrated.The store subscription pattern is correct, and adding the unsubscribe function to
unlistenersRefensures proper cleanup.src-tauri/src/commands/pty.rs (1)
9-18: LGTM!The global state management using
Arc<Mutex<HashMap>>is the standard Rust pattern for thread-safe shared state. ThePtyInstancestruct correctly retains the child process handle for lifetime management, even though it's not directly accessed (hence#[allow(dead_code)]). This ensures the child process isn't prematurely dropped.
- Fix ResizeObserver memory leak by storing in ref and disconnecting on cleanup - Add setThemeDropdownOpen to useEffect dependency array - Downgrade xterm addon versions for compatibility with xterm 5.5.0: - addon-webgl: 0.19.0 → 0.18.0 - addon-ligatures: 0.10.0 → 0.9.0 - addon-image: 0.9.0 → 0.8.0 - Fix Escape key handler to only close one element at a time - Reset initializedRef on unmount to allow proper re-initialization - Clean up all refs properly in cleanup effect
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai - have all reported bugs been addressed? is it safe to merge? |
Replace separate check-then-insert with HashMap::entry() API for atomic check-and-insert. Prevents potential resource leaks if two threads call spawn_shell with the same id concurrently.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 24265 @gl11tchy - Based on the review, there's one critical issue that should be addressed before merging: 🚨 Outstanding IssueChild Process Termination in The Recommended fix: pub fn kill_pty(id: String) -> Result<(), String> {
let mut instances = PTY_INSTANCES.lock().unwrap();
if let Some(mut instance) = instances.remove(&id) {
// Explicitly kill the child process
let _ = instance.child.kill();
Ok(())
} else {
Err(format!("PTY instance not found: {}", id))
}
}✅ What's Good
📝 Optional Improvements (Nitpicks)There are 3 optional suggestions for improvements (type safety, accessibility, thread coordination), but these aren't blocking issues. RecommendationNot safe to merge yet - I'd recommend fixing the child process termination issue first to prevent potential resource leaks in production. Once that's addressed, the PR should be good to go! 🚀 |
The portable_pty crate doesn't guarantee process termination on Drop, so explicitly call child.kill() when removing PTY instances to prevent orphaned shell processes from accumulating.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src-tauri/src/commands/pty.rs (1)
22-23: Consider moving import to module scope.The
Entryimport inside the function works but is unconventional. Moving it to the top-levelusestatement improves discoverability.🔎 Proposed change
use portable_pty::{native_pty_system, CommandBuilder, PtySize}; -use std::collections::HashMap; +use std::collections::{hash_map::Entry, HashMap}; use std::io::{Read, Write};Then remove line 22:
pub fn spawn_shell(app: AppHandle, id: String) -> Result<(), String> { - use std::collections::hash_map::Entry; - let pty_system = native_pty_system();src/components/Terminal/TerminalTabs.tsx (3)
51-56: Remove stable setter from dependency array.
setThemeDropdownOpenis a ReactuseStatesetter, which is stable across renders. Including it in the dependency array is unnecessary (though harmless). Same applies totoggleTerminalSearchif it's from a stable store action.🔎 Proposed change
}, [ terminalSearchVisible, toggleTerminalSearch, themeDropdownOpen, - setThemeDropdownOpen, ]);
138-148: Theme icon may not be intuitive.The current icon resembles a clock. A palette or paint-brush icon would better convey "theme" functionality. This is a minor UX suggestion.
🔎 Alternative palette icon
<svg className="w-4 h-4" viewBox="0 0 16 16" fill="none" stroke="currentColor" strokeWidth="1.5" > - <circle cx="8" cy="8" r="5" /> - <path d="M8 3v5l3 3" /> + <circle cx="8" cy="8" r="6" /> + <circle cx="6" cy="6" r="1.5" fill="currentColor" /> + <circle cx="10" cy="6" r="1.5" fill="currentColor" /> + <circle cx="6" cy="10" r="1.5" fill="currentColor" /> </svg>
150-173: Dropdown lacks keyboard navigation.The theme dropdown only supports mouse selection. For accessibility, consider adding:
role="listbox"on the dropdown- Arrow key navigation between options
- Enter/Space to select the focused option
aria-activedescendantfor screen readersThis is optional for an internal tool but recommended for accessibility compliance.
src/hooks/useTerminal.ts (1)
133-143: Consider debouncing resize callbacks.The
ResizeObservercallback triggers bothfitAddon.fit()and an IPC call on every resize event. During window resizing, this can fire many times per second, potentially flooding the backend with resize commands.Consider debouncing to reduce IPC overhead.
🔎 Proposed debounced resize
+ let resizeTimeout: ReturnType<typeof setTimeout> | null = null; const resizeObserver = new ResizeObserver(() => { - fitAddon.fit(); - const { rows, cols } = terminal; - if (isRemote && remote) { - invoke("ssh_resize_shell", { ptyId: id, rows, cols }).catch( - console.error, - ); - } else { - invoke("resize_pty", { id, rows, cols }).catch(console.error); - } + if (resizeTimeout) clearTimeout(resizeTimeout); + resizeTimeout = setTimeout(() => { + fitAddon.fit(); + const { rows, cols } = terminal; + if (isRemote && remote) { + invoke("ssh_resize_shell", { ptyId: id, rows, cols }).catch( + console.error, + ); + } else { + invoke("resize_pty", { id, rows, cols }).catch(console.error); + } + }, 100); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc-tauri/src/commands/pty.rssrc/components/Terminal/TerminalTabs.tsxsrc/hooks/useTerminal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (1)
src/hooks/useTerminal.ts (3)
src/stores/editorStore.ts (1)
useEditorStore(120-703)src/lib/terminalThemes.ts (1)
getTheme(295-296)src/components/Terminal/Terminal.tsx (1)
Terminal(11-45)
🔇 Additional comments (6)
src-tauri/src/commands/pty.rs (1)
66-83: TOCTOU race condition properly fixed with atomic Entry API.The fix correctly uses
HashMap::entry()for atomic check-and-insert, addressing the previously flagged race condition. If a PTY already exists for the givenid, the newly allocated resources (pair,child,writer,reader) are dropped automatically when returning early.One minor observation: resources are allocated before the atomic check, so in the double-mount scenario, a PTY is briefly opened and immediately closed. This is acceptable given React StrictMode double-mounts are infrequent and the simplicity gained.
src/components/Terminal/TerminalTabs.tsx (1)
39-46: Escape key handling properly fixed.The if/else-if pattern correctly ensures only one element closes at a time, addressing the previously flagged issue.
src/hooks/useTerminal.ts (4)
172-199: Comprehensive cleanup addresses all previously flagged issues.The cleanup effect now properly:
- Disconnects
ResizeObserverand nulls the ref- Calls all unlisteners (including theme subscription)
- Kills the PTY
- Disposes the terminal and clears refs
- Resets
initializedRefto allow remount re-initialization
68-77: WebGL addon with fallback handling is well implemented.The try/catch properly handles environments where WebGL isn't supported, and the
onContextLosshandler disposes the addon gracefully. xterm's canvas renderer activates automatically as fallback.
147-155: Theme subscription correctly wired with cleanup.The subscription uses a local variable to track the current theme, avoiding redundant updates. The unsubscribe function is properly pushed to
unlistenersReffor cleanup.
164-170: Search functions are correctly implemented.
| const initTerminal = useCallback( | ||
| async (container: HTMLDivElement) => { | ||
| // Prevent double initialization (React StrictMode) | ||
| if (initializedRef.current) return; | ||
| initializedRef.current = true; | ||
|
|
There was a problem hiding this comment.
Async initialization may race with cleanup on rapid mount/unmount.
If the component unmounts while initTerminal is still executing (e.g., between await listen() and await invoke("spawn_shell")), the cleanup effect runs but the async function continues. This could result in:
- Writing to a disposed terminal
- Pushing to a cleared
unlistenersRef - Setting refs that were nulled
Consider adding an abort signal or mounted flag to bail out of the async function if cleanup has started.
🔎 Proposed fix using AbortController or mounted flag
export function useTerminal({ id, onExit, remote }: UseTerminalOptions) {
const terminalRef = useRef<Terminal | null>(null);
const fitAddonRef = useRef<FitAddon | null>(null);
const searchAddonRef = useRef<SearchAddon | null>(null);
const resizeObserverRef = useRef<ResizeObserver | null>(null);
const unlistenersRef = useRef<UnlistenFn[]>([]);
const initializedRef = useRef(false);
+ const mountedRef = useRef(true);
const isRemote = !!remote;
const initTerminal = useCallback(
async (container: HTMLDivElement) => {
// Prevent double initialization (React StrictMode)
if (initializedRef.current) return;
initializedRef.current = true;
// ... terminal setup ...
+ // Check if still mounted before proceeding with async operations
+ if (!mountedRef.current) return;
+
// Listen for PTY output
const dataUnlisten = await listen<string>(`pty-data-${id}`, (event) => {
terminal.write(event.payload);
});
+ if (!mountedRef.current) {
+ dataUnlisten();
+ return;
+ }
+
// ... rest of setup ...
},
[id, onExit, isRemote, remote],
);
// Cleanup
useEffect(() => {
+ mountedRef.current = true;
return () => {
+ mountedRef.current = false;
// ... existing cleanup ...
};
}, [id, isRemote, remote]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/hooks/useTerminal.ts around lines 29-34, initTerminal is async and may
continue after the component unmounts, causing writes to a disposed terminal and
pushes to cleared refs; fix by adding an abort signal or mounted flag checked
after each await (or pass an AbortController.signal into async operations like
listen/invoke), bail out early if aborted/unmounted, and avoid mutating refs or
terminal instance when aborted; ensure cleanup sets the abort flag/signal before
awaiting pending work and that initTerminal checks it (and returns) before
performing side effects such as writing to the terminal or pushing into
unlistenersRef.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @gl11tchy. * #1 (comment) The following files were modified: * `src-tauri/src/commands/pty.rs` * `src/components/Terminal/TerminalTabs.tsx` * `src/hooks/useTerminal.ts`
1 participant
Summary
Upgrades the terminal with GPU acceleration, font ligatures, inline image support, and a theme system.
Changes
New Features
UI
Performance
Bug Fixes
Files Changed
package.json- Added 3 new xterm addon dependenciessrc/lib/terminalThemes.ts- New theme definitions filesrc/stores/editorStore.ts- Added theme state managementsrc/hooks/useTerminal.ts- Integrated new addons and theme supportsrc/components/Terminal/TerminalTabs.tsx- Added theme picker UIsrc-tauri/src/commands/pty.rs- Fixed double-spawn issueTesting
npm run tauri dev=> !== ->with a ligature font to see ligaturesFix remaining terminal-enhancements issues and add verification/tests:
Apply changes with minimal API surface disruption and include the brief tests/manual steps above.