Conversation
Contributor
📝 WalkthroughWalkthroughRefactors Terminal internals into three hooks—useTerminalLifecycle, useTerminalHotkeys, and useTerminalRefs—moving xterm instance management, event wiring, restoration, and hotkey logic out of the Terminal component while preserving its public API. Changes
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal Component
participant Lifecycle as useTerminalLifecycle
participant XTerm as XTerm Instance
participant Backend as createOrAttach / Session
participant Overlay as SessionKilledOverlay
Terminal->>Lifecycle: mount + options (paneId, tabId, refs)
Lifecycle->>XTerm: create instance + load addons
Lifecycle->>Backend: createOrAttach(session params)
Backend-->>Lifecycle: session attached / wasRecovered
Lifecycle->>XTerm: pipe data, set title, register input handlers
XTerm->>Lifecycle: user input / resize / focus events
Lifecycle->>Overlay: on session killed -> expose restartTerminal
Overlay->>Lifecycle: user triggers restartTerminal
Lifecycle->>Backend: createOrAttach (restart flow)
Backend-->>Lifecycle: new session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 512-521: Replace the misleading biome-ignore comment near the
effect that omits tabId from the dependency array with a concise clarifying
comment stating that tabId is a plain prop (not a ref) and is architecturally
stable for the life of this component because any change to tabId causes the
component to remount, so omitting tabId from the useEffect dependency array in
useTerminalLifecycle is intentional and safe; update the comment near the effect
(where tabId is used but not listed) to explicitly document this reasoning.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (2)
492-496: Extract magic number to named constant.The
50ms timeout for deferred detach is a magic number. Consider extracting it to a named constant at module level for clarity.Proposed fix
Add at the top of the file near other config imports:
const DETACH_DELAY_MS = 50;Then update the usage:
- const detachTimeout = setTimeout(() => { + const detachTimeout = setTimeout(() => { detachRef.current({ paneId }); pendingDetaches.delete(paneId); coldRestoreState.delete(paneId); - }, 50); + }, DETACH_DELAY_MS);
199-204: Consider adding error handling for dynamic import.The dynamic import of
SearchAddondoesn't handle potential import failures. While search functionality is non-critical, an unhandled rejection could clutter error logs in edge cases.Proposed fix
import("@xterm/addon-search").then(({ SearchAddon }) => { if (isUnmounted) return; const searchAddon = new SearchAddon(); xterm.loadAddon(searchAddon); searchAddonRef.current = searchAddon; - }); + }).catch((err) => { + console.warn("[Terminal] Failed to load search addon:", err); + });
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 241-246: The dynamic import of the SearchAddon in
useTerminalLifecycle.ts currently has no error handling; update the
import("@xterm/addon-search").then(...) chain to add a .catch(...) that logs
failures using the prefixed pattern (e.g. console.error("[terminal/search-addon]
failed to load SearchAddon:", err)), and ensure any error does not crash the app
(do not swallow—log with context). Keep the existing isUnmounted check and the
assignment to searchAddonRef.current and xterm.loadAddon(SearchAddon) inside the
success handler; only handle and log errors in the .catch block referencing
SearchAddon, searchAddonRef, xterm.loadAddon and isUnmounted.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (3)
34-83: Use params objects for helper functions with 2+ args.
markAttachInFlight,clearAttachInFlight, andwaitForAttachClearshould take a single params object; consider aligningRegisterCallback/UnregisterCallbacksimilarly (will require matching changes inuseTerminalRefs).As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.♻️ Suggested refactor (update call sites accordingly)
@@ -function markAttachInFlight(paneId: string, attachId: number): void { +function markAttachInFlight({ + paneId, + attachId, +}: { paneId: string; attachId: number }): void { @@ -function clearAttachInFlight(paneId: string, attachId?: number): void { +function clearAttachInFlight({ + paneId, + attachId, +}: { paneId: string; attachId?: number }): void { @@ -function waitForAttachClear(paneId: string, waiter: () => void): () => void { +function waitForAttachClear({ + paneId, + waiter, +}: { paneId: string; waiter: () => void }): () => void { @@ - cancelAttachWait = waitForAttachClear(paneId, () => { + cancelAttachWait = waitForAttachClear({ paneId, waiter: () => { @@ - }); + }});
192-195: Standardize log prefixes to[domain/operation].
Please align the log prefix here (and other console.* in this hook) with the required format.As per coding guidelines: Use prefixed console logging with pattern `[domain/operation] message` for all logging.♻️ Example adjustment
- console.log(`[Terminal] Mount: ${paneId}`); + console.log(`[terminal/lifecycle] Mount: ${paneId}`);
570-575: Extract the detach delay magic number.
Make the delay explicit via a named constant at module scope.As per coding guidelines: Avoid magic numbers by extracting them to named constants at module top.♻️ Suggested refactor
@@ -const attachWaitersByPane = new Map<string, Set<() => void>>(); +const attachWaitersByPane = new Map<string, Set<() => void>>(); +const DETACH_DELAY_MS = 50; @@ - const detachTimeout = setTimeout(() => { + const detachTimeout = setTimeout(() => { detachRef.current({ paneId }); pendingDetaches.delete(paneId); coldRestoreState.delete(paneId); - }, 50); + }, DETACH_DELAY_MS);
Comment on lines
+241
to
+246
| import("@xterm/addon-search").then(({ SearchAddon }) => { | ||
| if (isUnmounted) return; | ||
| const searchAddon = new SearchAddon(); | ||
| xterm.loadAddon(searchAddon); | ||
| searchAddonRef.current = searchAddon; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | sed -n '235,250p'Repository: superset-sh/superset
Length of output: 659
🏁 Script executed:
# Check for surrounding context and see if there's any error handling wrapping this code
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | sed -n '220,260p'Repository: superset-sh/superset
Length of output: 1512
🏁 Script executed:
# Get the function signature and broader context
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts | sed -n '180,250p'Repository: superset-sh/superset
Length of output: 2717
🏁 Script executed:
# Search for other import() or error handling patterns in the file to understand conventions
rg -n "import\(" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts -A 5Repository: superset-sh/superset
Length of output: 297
🏁 Script executed:
# Check for logging patterns and console.error usage in the renderer directory to verify convention compliance
rg -n "console\.(error|log)" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsRepository: superset-sh/superset
Length of output: 375
🏁 Script executed:
# Check other terminal-related files for logging patterns
rg -n "console\.(error|log)" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ -t ts -t tsx | head -20Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check other terminal-related files for logging patterns
rg -n "console\.(error|log)" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/ | head -20Repository: superset-sh/superset
Length of output: 3381
Add error handling for the search addon dynamic import.
The dynamic import can fail; add a .catch() block to log errors explicitly.
Suggested fix
import("@xterm/addon-search").then(({ SearchAddon }) => {
if (isUnmounted) return;
const searchAddon = new SearchAddon();
xterm.loadAddon(searchAddon);
searchAddonRef.current = searchAddon;
- });
+ }).catch((error) => {
+ console.error(
+ "[terminal/lifecycle] Failed to load search addon",
+ error,
+ );
+ });Per coding guidelines: Never swallow errors silently; at minimum log them with context, and use prefixed console logging with pattern [domain/operation] message for all logging.
📝 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.
Suggested change
| import("@xterm/addon-search").then(({ SearchAddon }) => { | |
| if (isUnmounted) return; | |
| const searchAddon = new SearchAddon(); | |
| xterm.loadAddon(searchAddon); | |
| searchAddonRef.current = searchAddon; | |
| }); | |
| import("@xterm/addon-search").then(({ SearchAddon }) => { | |
| if (isUnmounted) return; | |
| const searchAddon = new SearchAddon(); | |
| xterm.loadAddon(searchAddon); | |
| searchAddonRef.current = searchAddon; | |
| }).catch((error) => { | |
| console.error( | |
| "[terminal/lifecycle] Failed to load search addon", | |
| error, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`
around lines 241 - 246, The dynamic import of the SearchAddon in
useTerminalLifecycle.ts currently has no error handling; update the
import("@xterm/addon-search").then(...) chain to add a .catch(...) that logs
failures using the prefixed pattern (e.g. console.error("[terminal/search-addon]
failed to load SearchAddon:", err)), and ensure any error does not crash the app
(do not swallow—log with context). Keep the existing isUnmounted check and the
assignment to searchAddonRef.current and xterm.loadAddon(SearchAddon) inside the
success handler; only handle and log errors in the .catch block referencing
SearchAddon, searchAddonRef, xterm.loadAddon and isUnmounted.
This was referenced Feb 1, 2026
This was referenced Feb 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.