refactor(desktop): Split Terminal component into smaller modules#838
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughRefactors the Terminal internals: extracts three overlay components, adds config constants and types/state, and moves terminal lifecycle, connection, restore, stream, CWD, and mode logic into multiple new hooks and state utilities while preserving the Terminal component's external signature. Changes
Sequence Diagram(s)sequenceDiagram
participant Terminal as Terminal Component
participant StreamHook as useTerminalStream
participant RestoreHook as useTerminalRestore
participant ColdHook as useTerminalColdRestore
participant XTerm as XTerm Instance
participant Main as Main Process / IPC
Terminal->>StreamHook: init(stream refs, handlers)
Terminal->>RestoreHook: init(pending state refs)
Terminal->>ColdHook: init(recovery refs)
Main-->>StreamHook: stream event (data / exit / error)
StreamHook->>XTerm: write data (or queue if not ready)
StreamHook->>Terminal: update modes / cwd refs
alt connection error
StreamHook->>Terminal: set connection error
Terminal->>ColdHook: handleRetryConnection()
else session killed
StreamHook->>Terminal: set exit/killed
Terminal->>ColdHook: handleRetryConnection()
end
ColdHook->>Main: createOrAttach (restore)
Main-->>ColdHook: CreateOrAttachResult (possibly with snapshot)
ColdHook->>RestoreHook: maybeApplyInitialState(snapshot)
RestoreHook->>XTerm: apply snapshot content & modes
RestoreHook->>XTerm: fitAddon.fit()
Terminal->>Terminal: show RestoredModeOverlay
Terminal->>ColdHook: handleStartShell()
ColdHook->>Main: acknowledge & create new session
sequenceDiagram
participant Terminal as Terminal Component
participant CwdHook as useTerminalCwd
participant ModesHook as useTerminalModes
participant StreamHook as useTerminalStream
participant Tabs as Tabs Store
Terminal->>CwdHook: init(initialCwd, workspaceCwd)
StreamHook-->>CwdHook: on data: updateCwdFromData(data)
CwdHook->>Tabs: updatePaneCwd (debounced)
StreamHook-->>ModesHook: on data: updateModesFromData(data)
ModesHook->>ModesHook: track alt-screen / bracketed-paste refs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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. 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: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts`:
- Around line 71-88: The normalization logic in useFileLinkClick treats only
POSIX absolutes and can mis-handle Windows drive (e.g., C:\...) and UNC
(\\server\share) or file:// URIs, causing false relative paths; update the
branch around workspaceCwd/filePath handling to first normalize separators and
detect Windows/UNC/file-URI absolute forms (drive-letter with colon + backslash,
leading double backslashes, and file://) before comparing to workspaceCwd, and
if such paths are outside the workspace call toast.warning (same message) and
return; ensure addFileViewerPane is only called with truly workspace-relative
filePath.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalModes.ts`:
- Around line 43-50: The code in useTerminalModes.ts currently only checks for
1049 and 47 alternate-screen sequences; update the enterAltIndex and
exitAltIndex calculations to also consider 1047 by including
combined.lastIndexOf("\x1b[?1047h") in the enterAltIndex max and
combined.lastIndexOf("\x1b[?1047l") in the exitAltIndex max so the hook
(useTerminalModes / isAlternateScreenRef logic) correctly handles terminals/apps
that toggle alternate screen with 1047 sequences.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts`:
- Around line 22-157: Two exported callbacks in useTerminalStream
(handleTerminalExit and handleStreamError) take multiple positional params;
update their signatures to accept single param objects (e.g.,
handleTerminalExit({ exitCode, xterm }) and handleStreamError({ event, xterm }))
and update all callers to pass a single object. Change the corresponding handler
type in UseTerminalStreamReturn, update useTerminalRestore's callbacks and
flushPendingEvents to enqueue/dispatch the new object-shaped events, and update
Terminal.tsx call sites to call these handlers with the params object; ensure
pendingEventsRef handling still pushes the same event objects for later
processing.
🧹 Nitpick comments (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/components/ConnectionErrorOverlay/ConnectionErrorOverlay.tsx (1)
13-28: Add dialog semantics for screen readers.
The overlay behaves like a modal but lacksrole,aria-modal, and labeling. This makes it harder for assistive tech users to understand context.♿ Proposed a11y tweak
- <div className="absolute inset-0 z-10 flex items-center justify-center bg-black/50"> + <div + className="absolute inset-0 z-10 flex items-center justify-center bg-black/50" + role="alertdialog" + aria-modal="true" + aria-labelledby="terminal-connection-error-title" + aria-describedby="terminal-connection-error-desc" + > <Card className="gap-3 py-4 px-2 max-w-xs"> <div className="flex flex-col items-center text-center gap-1.5 px-4"> <HiExclamationTriangle className="size-5 text-destructive" /> <div className="space-y-0.5"> - <p className="text-sm font-medium">Connection error</p> - <p className="text-xs text-muted-foreground"> + <p id="terminal-connection-error-title" className="text-sm font-medium"> + Connection error + </p> + <p id="terminal-connection-error-desc" className="text-xs text-muted-foreground"> Lost connection to terminal daemon </p> </div> </div>apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/components/SessionKilledOverlay/SessionKilledOverlay.tsx (1)
11-25: Add dialog semantics for screen readers.
This is a blocking overlay; add dialog semantics and label it so assistive tech can announce the context.♿ Proposed a11y tweak
- <div className="absolute inset-0 z-10 flex items-center justify-center bg-black/50"> + <div + className="absolute inset-0 z-10 flex items-center justify-center bg-black/50" + role="alertdialog" + aria-modal="true" + aria-labelledby="terminal-session-killed-title" + aria-describedby="terminal-session-killed-desc" + > <Card className="gap-3 py-4 px-2"> <div className="flex flex-col items-center text-center gap-1.5 px-4"> <LuPower className="size-5 text-muted-foreground" /> <div className="space-y-0.5"> - <p className="text-sm font-medium">Session killed</p> - <p className="text-xs text-muted-foreground"> + <p id="terminal-session-killed-title" className="text-sm font-medium"> + Session killed + </p> + <p id="terminal-session-killed-desc" className="text-xs text-muted-foreground"> You terminated this shell session </p> </div> </div>apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalModes.ts (1)
62-63: Extract the tail length magic number.
Pull32into a named constant to make intent clear and to tune in one place.♻️ Proposed refactor
+const MODE_SCAN_TAIL_CHARS = 32; + export function useTerminalModes(): UseTerminalModesReturn { @@ - modeScanBufferRef.current = combined.slice(-32); + modeScanBufferRef.current = combined.slice(-MODE_SCAN_TAIL_CHARS); }, []);As per coding guidelines, extract magic numbers to named constants.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/components/RestoredModeOverlay/RestoredModeOverlay.tsx (1)
12-30: Consider adding accessibility attributes for modal-like overlay.The overlay functions as a modal dialog blocking interaction with the terminal behind it. For improved screen reader support, consider adding
role="dialog"andaria-modal="true"to the container, andaria-labelledbypointing to the title.♿ Suggested accessibility improvement
return ( - <div className="absolute inset-0 z-10 flex items-center justify-center bg-black/50"> - <Card className="gap-3 py-4 px-2"> + <div + className="absolute inset-0 z-10 flex items-center justify-center bg-black/50" + role="dialog" + aria-modal="true" + aria-labelledby="restored-mode-title" + > + <Card className="gap-3 py-4 px-2"> <div className="flex flex-col items-center text-center gap-1.5 px-4"> <LuTerminal className="size-5 text-primary" /> <div className="space-y-0.5"> - <p className="text-sm font-medium">Session restored</p> + <p id="restored-mode-title" className="text-sm font-medium">Session restored</p>apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalCwd.ts (2)
47-51: Consider extracting the debounce delay to a named constant.The 150ms debounce delay is a magic number. Per coding guidelines, consider extracting it to
config.tsalongsideRESIZE_DEBOUNCE_MS(which is also 150ms) for consistency and discoverability.🔧 Suggested extraction
In
config.ts:+export const CWD_UPDATE_DEBOUNCE_MS = 150;In this file:
+import { CWD_UPDATE_DEBOUNCE_MS } from "../config"; + const debouncedUpdatePaneCwdRef = useRef( debounce((id: string, cwd: string | null, confirmed: boolean) => { updatePaneCwd(id, cwd, confirmed); - }, 150), + }, CWD_UPDATE_DEBOUNCE_MS), );
54-60: Unnecessary nullish coalescing on boolean state.
cwdConfirmedis initialized viauseState(false)and only ever set totrueorfalse, so it can never be nullish. The?? falseis redundant.🔧 Suggested fix
useEffect(() => { debouncedUpdatePaneCwdRef.current( paneId, terminalCwd, - cwdConfirmed ?? false, + cwdConfirmed, ); }, [terminalCwd, cwdConfirmed, paneId]);apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalColdRestore.ts (1)
22-24: ReplaceanyoncreateOrAttachRefwith a typed signature.Using
anyhere removes type safety for the most critical session-creation path. Consider exporting aCreateOrAttachFn(or similar) type fromTerminal/types.tsoruseTerminalConnectionand use it forcreateOrAttachRef.
As per coding guidelines, avoidanywhen a precise type is feasible.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts (1)
12-35: Use a params object forhandleFileLinkClickto align with the codebase guideline.Functions with 2+ parameters should accept a single params object. Update the handler signature from
(path: string, line?: number, column?: number)to accept a single options object, and update all call sites accordingly.♻️ Proposed shape for the handler
export interface UseFileLinkClickReturn { - handleFileLinkClick: (path: string, line?: number, column?: number) => void; + handleFileLinkClick: (params: { + path: string; + line?: number; + column?: number; + }) => void; } ... - const handleFileLinkClick = useCallback( - (path: string, line?: number, column?: number) => { + const handleFileLinkClick = useCallback( + ({ path, line, column }: { path: string; line?: number; column?: number }) => {Update call sites in
Terminal.tsx(line 321-322) andhelpers.ts(line 258) to pass an object instead of positional arguments.
Comment on lines
+71
to
+88
| // Normalize absolute paths to worktree-relative paths for file viewer | ||
| // File viewer expects relative paths, but terminal links can be absolute | ||
| let filePath = path; | ||
| // Use path boundary check to avoid incorrect prefix stripping | ||
| // e.g., /repo vs /repo-other should not match | ||
| if (path === workspaceCwd) { | ||
| filePath = "."; | ||
| } else if (path.startsWith(`${workspaceCwd}/`)) { | ||
| filePath = path.slice(workspaceCwd.length + 1); | ||
| } else if (path.startsWith("/")) { | ||
| // Absolute path outside workspace - show warning and don't attempt to open | ||
| toast.warning("File is outside the workspace", { | ||
| description: | ||
| "Switch to 'External editor' in Settings to open this file", | ||
| }); | ||
| return; | ||
| } | ||
| addFileViewerPane(workspaceId, { filePath, line, column }); |
Contributor
There was a problem hiding this comment.
Handle Windows/UNC absolute paths when normalizing file links.
The file-viewer normalization only checks POSIX-style absolute paths (/) and ${workspaceCwd}/. On Windows, absolute paths like C:\repo\file or \\server\share will fall through and be treated as relative, but the file viewer expects workspace‑relative paths, so these links will likely fail. Consider normalizing separators and detecting Windows/UNC/file-URI absolutes before deciding to open in the viewer.
🐛 Suggested hardening for cross-platform paths
- let filePath = path;
+ const normalizedPath = path.replace(/\\/g, "/");
+ const normalizedCwd = workspaceCwd.replace(/\\/g, "/");
+ let filePath = normalizedPath;
// Use path boundary check to avoid incorrect prefix stripping
// e.g., /repo vs /repo-other should not match
- if (path === workspaceCwd) {
+ if (normalizedPath === normalizedCwd) {
filePath = ".";
- } else if (path.startsWith(`${workspaceCwd}/`)) {
- filePath = path.slice(workspaceCwd.length + 1);
- } else if (path.startsWith("/")) {
+ } else if (normalizedPath.startsWith(`${normalizedCwd}/`)) {
+ filePath = normalizedPath.slice(normalizedCwd.length + 1);
+ } else if (
+ normalizedPath.startsWith("/") ||
+ /^[A-Za-z]:\//.test(normalizedPath) ||
+ normalizedPath.startsWith("//")
+ ) {
// Absolute path outside workspace - show warning and don't attempt to open
toast.warning("File is outside the workspace", {
description:
"Switch to 'External editor' in Settings to open this file",
});
return;
}🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useFileLinkClick.ts`
around lines 71 - 88, The normalization logic in useFileLinkClick treats only
POSIX absolutes and can mis-handle Windows drive (e.g., C:\...) and UNC
(\\server\share) or file:// URIs, causing false relative paths; update the
branch around workspaceCwd/filePath handling to first normalize separators and
detect Windows/UNC/file-URI absolute forms (drive-letter with colon + backslash,
leading double backslashes, and file://) before comparing to workspaceCwd, and
if such paths are outside the workspace call toast.warning (same message) and
return; ensure addFileViewerPane is only called with truly workspace-relative
filePath.
Comment on lines
+43
to
+50
| const enterAltIndex = Math.max( | ||
| combined.lastIndexOf("\x1b[?1049h"), | ||
| combined.lastIndexOf("\x1b[?47h"), | ||
| ); | ||
| const exitAltIndex = Math.max( | ||
| combined.lastIndexOf("\x1b[?1049l"), | ||
| combined.lastIndexOf("\x1b[?47l"), | ||
| ); |
Contributor
There was a problem hiding this comment.
Handle \x1b[?1047h/l alternate-screen sequences.
Some terminals/apps toggle alternate screen via 1047h/1047l. Missing these can leave isAlternateScreenRef stale.
🔧 Suggested fix
const enterAltIndex = Math.max(
combined.lastIndexOf("\x1b[?1049h"),
+ combined.lastIndexOf("\x1b[?1047h"),
combined.lastIndexOf("\x1b[?47h"),
);
const exitAltIndex = Math.max(
combined.lastIndexOf("\x1b[?1049l"),
+ combined.lastIndexOf("\x1b[?1047l"),
combined.lastIndexOf("\x1b[?47l"),
);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalModes.ts`
around lines 43 - 50, The code in useTerminalModes.ts currently only checks for
1049 and 47 alternate-screen sequences; update the enterAltIndex and
exitAltIndex calculations to also consider 1047 by including
combined.lastIndexOf("\x1b[?1047h") in the enterAltIndex max and
combined.lastIndexOf("\x1b[?1047l") in the exitAltIndex max so the hook
(useTerminalModes / isAlternateScreenRef logic) correctly handles terminals/apps
that toggle alternate screen with 1047 sequences.
Break down the large Terminal.tsx (~1500 lines) into focused modules: **New Components:** - SessionKilledOverlay - UI for killed session state - ConnectionErrorOverlay - UI for connection error state - RestoredModeOverlay - UI for cold restore state **New Hooks:** - useTerminalCwd - CWD state management and OSC-7 parsing - useTerminalModes - Alternate screen and bracketed paste tracking - useFileLinkClick - File link click handling - useTerminalRestore - Terminal state restoration from snapshots - useTerminalStream - Stream event handling (data, exit, error) - useTerminalColdRestore - Cold restore (reboot recovery) handling **New Files:** - state.ts - Module-level maps for pendingDetaches and coldRestoreState - config.ts - Added DEBUG_TERMINAL and FIRST_RENDER_RESTORE_FALLBACK_MS - types.ts - Added CreateOrAttachResult and ColdRestoreState types Terminal.tsx reduced from ~1500 lines to ~685 lines (~55% reduction) while maintaining the same functionality.
…l restore
- Handle exit/disconnect/error events immediately in handleStreamData
instead of queueing them, as these are critical state changes
- Add mode tracking (updateModesFromData) to flushPendingEvents for queued data
- Replace dynamic require("@superset/ui/sonner") with static ES import
- Pass stream event handlers to useTerminalRestore via refs pattern
…ream ready Reverts to original queueing behavior where ALL events (data, exit, disconnect, error) are queued when isStreamReadyRef is false, then flushed in order. This preserves the pre-refactor event sequencing.
Kitenite
force-pushed
the
refactor-terminal
branch
from
87d1912 to
fd777ee
Compare
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This was referenced Jan 22, 2026
This was referenced Jan 31, 2026
8 tasks
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.
Summary
Changes
New Components:
SessionKilledOverlay- UI overlay for killed session stateConnectionErrorOverlay- UI overlay for connection error stateRestoredModeOverlay- UI overlay for cold restore stateNew Hooks:
useTerminalCwd- CWD state management and OSC-7 parsinguseTerminalModes- Alternate screen and bracketed paste trackinguseFileLinkClick- File link click handlinguseTerminalRestore- Terminal state restoration from snapshotsuseTerminalStream- Stream event handling (data, exit, error)useTerminalColdRestore- Cold restore (reboot recovery) handlingNew/Updated Files:
state.ts- Module-level maps for pendingDetaches and coldRestoreStateconfig.ts- Added DEBUG_TERMINAL and FIRST_RENDER_RESTORE_FALLBACK_MS constantstypes.ts- Added CreateOrAttachResult and ColdRestoreState typesTest plan
Summary by CodeRabbit
New Features
Refactor
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.