Repository navigation
Conversation
📝 WalkthroughWalkthroughIntroduces a terminal instance caching mechanism that preserves xterm instances, addons, and DOM elements across component mount/unmount cycles. Terminals are stored by paneId, retrieved on mount to avoid recreation, and cleaned up when no longer needed. Also removes suppression of focus-related CSI sequences. Changes
Sequence Diagram(s)sequenceDiagram
participant Comp as Terminal Component
participant Cache as Terminal Cache
participant DOM as DOM Wrapper
participant XTerm as XTerm Instance
participant Addon as Search Addon
rect rgb(200, 220, 255)
note over Comp,Addon: First Mount (paneId = "pane-1")
Comp->>Cache: getCachedTerminal("pane-1")
Cache-->>Comp: undefined
Comp->>XTerm: new XTerm()
Comp->>Addon: load SearchAddon
Addon-->>Comp: addon instance
Comp->>DOM: attach terminal element
Comp->>Cache: setCachedTerminal("pane-1", {xterm, fitAddon, searchAddon, element, cleanup})
end
rect rgb(200, 255, 220)
note over Comp,DOM: Unmount
Comp->>DOM: detach terminal element from wrapper
Comp->>Cache: (terminal remains cached)
end
rect rgb(255, 220, 220)
note over Comp,Cache: Second Mount (paneId = "pane-1")
Comp->>Cache: getCachedTerminal("pane-1")
Cache-->>Comp: {xterm, fitAddon, searchAddon, element, cleanup}
Comp->>DOM: attach cached element to wrapper
note over Comp: reuse existing xterm, addons, and state
end
rect rgb(255, 240, 200)
note over Comp,Cache: Tab Close / Cleanup
Comp->>Cache: removeCachedTerminal("pane-1")
Cache->>XTerm: dispose()
Cache->>Addon: (cleanup)
Cache->>Cache: delete from internal Map
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.tsapps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
One component per file - do not create multi-component files
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (10)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/suppressQueryResponses.ts (1)
18-18: LGTM! Focus reports now enabled for TUI apps.The removal of CSI I/O suppression allows focus-reporting modes (like mode 1004) to work correctly, which is essential for TUI applications such as opencode that depend on focus events to trigger redraws.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/terminalCache.ts (1)
5-43: LGTM! Clean cache implementation following Hyper's pattern.The module-level cache with a minimal public API (get/set/remove/has) provides a clean abstraction for terminal instance persistence. The disposal order in
removeCachedTerminal(cleanup → dispose → delete) is correct and ensures proper resource cleanup.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (7)
38-45: LGTM! Clean separation of concerns.Introducing
terminalWrapperReffor DOM attachment and separate tracking ofdisposableListenersRefandcleanupFunctionsRefenables the Hyper-like pattern where listeners are disposed on unmount while the terminal instance persists in cache.
315-337: LGTM! Cache reattachment logic is sound.When a cached terminal exists, the implementation correctly reattaches the preserved DOM element to the new wrapper, restores refs, and calls
fit()to adapt to the potentially new container dimensions.
338-361: LGTM! New terminal creation is correct.The creation flow properly:
- Sets
isNewTerminalflag to control PTY creation- Creates a persistent terminal element
- Initializes xterm with correct options
- Attaches to the wrapper
- Updates refs
464-489: LGTM! PTY creation correctly gated by isNewTerminal.Only creating/attaching the backend PTY for new terminals is essential to the Hyper pattern. This ensures that when reattaching a cached terminal, the existing PTY connection is reused, preserving terminal modes, alternate buffer state, and preventing TUI app disruption.
491-556: LGTM! Event listener setup and tracking is thorough.The implementation correctly:
- Registers xterm event listeners (onData, onKey, onTitleChange)
- Stores disposables in
disposableListenersReffor cleanup- Sets up keyboard, click, focus, resize, and paste handlers
- Stores cleanup functions in
cleanupFunctionsRefThis enables selective cleanup on unmount without disposing the terminal instance.
560-587: LGTM! Cleanup correctly follows Hyper's pattern.The unmount cleanup properly:
- Disposes event listeners without destroying the terminal
- Runs helper cleanup functions
- Unregisters callbacks
- Detaches the terminal element from the wrapper (with safe conditional check)
- Clears refs while preserving the terminal in cache
This ensures the terminal instance can be reattached on subsequent mounts, preserving all state.
631-631: LGTM! Render updated for new wrapper pattern.Using
terminalWrapperRefas the container for attaching/detaching the persistent terminal element aligns with the caching implementation.apps/desktop/src/renderer/stores/tabs/utils/terminal-cleanup.ts (1)
8-14: LGTM! Cleanup order is correct.All pane deletion code paths properly invoke
killTerminalForPanewith appropriate terminal type checks. The frontend state (listeners, xterm instance) is disposed viaremoveCachedTerminalbefore signaling termination to the backend, preventing race conditions between frontend disposal and backend PTY cleanup. The type checking optimization (only killing terminal sessions fortype === "terminal"panes) avoids unnecessary IPC overhead for file-viewer panes.
| import("@xterm/addon-search").then(({ SearchAddon }) => { | ||
| const searchAddon = new SearchAddon(); | ||
| xterm.loadAddon(searchAddon); | ||
| searchAddonRef.current = searchAddon; | ||
|
|
||
| // Update cache with search addon | ||
| const currentCached = getCachedTerminal(paneId); | ||
| if (currentCached) { | ||
| currentCached.searchAddon = searchAddon; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Add error handling and consider race condition with terminal disposal.
Two issues:
-
Missing error handling: The async import has no
.catch()handler. If the import fails orloadAddonthrows (e.g., if xterm was disposed viaremoveCachedTerminalduring the async operation), errors will go unhandled. -
Minor race condition: If the pane is deleted (calling
removeCachedTerminal→xterm.dispose()) between caching the terminal (line 377) and the addon loading (line 366),loadAddonmay be called on a disposed terminal. While line 370-373 prevents cache corruption, the disposed terminal could still cause an error.
🛡️ Proposed fix
// Load search addon asynchronously
import("@xterm/addon-search").then(({ SearchAddon }) => {
- const searchAddon = new SearchAddon();
- xterm.loadAddon(searchAddon);
- searchAddonRef.current = searchAddon;
-
- // Update cache with search addon
- const currentCached = getCachedTerminal(paneId);
- if (currentCached) {
- currentCached.searchAddon = searchAddon;
- }
+ // Check if terminal still exists in cache before loading
+ const currentCached = getCachedTerminal(paneId);
+ if (currentCached) {
+ const searchAddon = new SearchAddon();
+ xterm.loadAddon(searchAddon);
+ searchAddonRef.current = searchAddon;
+ currentCached.searchAddon = searchAddon;
+ }
+}).catch((error) => {
+ console.error("[Terminal] Failed to load search addon for pane", paneId, error);
});This checks cache existence before loading the addon and adds error handling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import("@xterm/addon-search").then(({ SearchAddon }) => { | |
| const searchAddon = new SearchAddon(); | |
| xterm.loadAddon(searchAddon); | |
| searchAddonRef.current = searchAddon; | |
| // Update cache with search addon | |
| const currentCached = getCachedTerminal(paneId); | |
| if (currentCached) { | |
| currentCached.searchAddon = searchAddon; | |
| } | |
| }); | |
| import("@xterm/addon-search").then(({ SearchAddon }) => { | |
| // Check if terminal still exists in cache before loading | |
| const currentCached = getCachedTerminal(paneId); | |
| if (currentCached) { | |
| const searchAddon = new SearchAddon(); | |
| xterm.loadAddon(searchAddon); | |
| searchAddonRef.current = searchAddon; | |
| currentCached.searchAddon = searchAddon; | |
| } | |
| }).catch((error) => { | |
| console.error("[Terminal] Failed to load search addon for pane", paneId, error); | |
| }); |
Summary
Changes
terminalCache.ts- module-level cache for terminal instancesTerminal.tsxto reuse cached terminals on remount instead of creating new onesterminal-cleanup.tsto properly dispose cached terminals when panes are deletedWhy this fixes the issue
Previously, switching tabs caused:
This lost terminal modes like focus reporting (mode 1004). TUI apps like opencode enable this mode to know when focus changes, but after tab switch the mode was lost, so opencode didn't know to redraw.
Now (following Hyper's approach):
This preserves all terminal state including modes, alternate buffer, etc.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.