feat: support for split terminal ui #652
Conversation
WalkthroughThis PR introduces a multi-pane, session-based terminal UI with resizable split panes. The terminal component is substantially refactored to support multiple sessions and splits per session, with per-session active pane tracking. Terminal utilities are enhanced with robust dimension-readiness checks and safe WebSocket messaging guards. The API version metadata is also updated. Changes
Sequence DiagramsequenceDiagram
participant User
participant Terminal Component
participant TerminalSession
participant isContainerReady
participant ResizeObserver
participant WebSocket
participant XTerm
User->>Terminal Component: Mount component
Terminal Component->>TerminalSession: Create session instance
TerminalSession->>isContainerReady: Check if container has valid dimensions
isContainerReady->>isContainerReady: Measure element/parent sizes
alt Dimensions not ready
isContainerReady->>isContainerReady: Schedule requestAnimationFrame retry
end
isContainerReady-->>TerminalSession: Container ready signal
TerminalSession->>WebSocket: safeSendMessage (await readiness)
alt WebSocket ready
WebSocket-->>TerminalSession: Send initial terminal data
else WebSocket not ready
TerminalSession->>TerminalSession: Queue/defer message
end
TerminalSession->>ResizeObserver: Observe pane dimensions
ResizeObserver->>ResizeObserver: Detect resize events
ResizeObserver->>WebSocket: safeSendMessage (resize dimensions)
TerminalSession->>XTerm: Initialize terminal with dimensions
XTerm-->>TerminalSession: Terminal ready
User->>Terminal Component: Add split pane / Switch session
Terminal Component->>ResizablePanelGroup: Update layout / visibility
Terminal Component->>TerminalSession: Trigger initialization for new pane
TerminalSession->>isContainerReady: Re-check new pane dimensions
isContainerReady-->>TerminalSession: New pane ready
TerminalSession->>XTerm: Initialize new terminal instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
view/app/terminal/utils/isContainerReady.ts (1)
18-22: Unbounded retry loop may spin indefinitely.If the ref is attached but dimensions never become positive (e.g., CSS display:none, or parent with zero size), the
requestAnimationFrameretry at lines 20, 58 will loop forever, consuming CPU.Consider adding a max retry count or timeout to bail out gracefully.
+const MAX_RETRIES = 50; // ~50 frames ≈ 800ms at 60fps +const retryCountRef = useRef(0); const checkSize = () => { if (!isTerminalOpen) { setIsContainerReady(false); + retryCountRef.current = 0; return; } if (!terminalRef?.current) { + if (retryCountRef.current++ < MAX_RETRIES) { rafRef.current = requestAnimationFrame(checkSize); + } return; } // ... dimension checks ... if (height > 0 && width > 0) { setIsContainerReady(true); + retryCountRef.current = 0; } else { + if (retryCountRef.current++ < MAX_RETRIES) { rafRef.current = requestAnimationFrame(checkSize); + } } };view/app/terminal/utils/useTerminal.ts (3)
46-50: Silent message drop may hide initialization issues.When
isReadyRef.currentis false, messages are silently dropped. This could cause the terminal to fail to initialize properly without any indication. Consider logging dropped messages in development or queueing critical initialization messages.const safeSendMessage = useCallback((data: any) => { if (isReadyRef.current) { sendJsonMessageRef.current(data); + } else if (process.env.NODE_ENV === 'development') { + console.debug('WebSocket not ready, message dropped:', data.action); } }, []);
52-60: Remove unusedterminalIdfrom dependency array.
terminalIdis listed in dependencies but not used indestroyTerminal. This won't cause bugs but unnecessarily recreates the callback.- }, [terminalInstance, terminalId]); + }, [terminalInstance]);
220-220:terminalRefis a stable ref and doesn't need to be in dependencies.Refs returned by
useRefhave a stable identity and shouldn't trigger callback recreation. Remove it from the dependency array.- }, [safeSendMessage, terminalRef, terminalInstance, allowInput, terminalId]); + }, [safeSendMessage, terminalInstance, allowInput, terminalId]);view/app/terminal/terminal.tsx (5)
207-215: Clearing timeouts inside the loop doesn't prevent remaining iterations.When
attemptInitialization()succeeds, clearingtimeoutsarray doesn't stop already-scheduledsetTimeoutcallbacks for later delays since the loop has already scheduled all of them. Theinitializedflag guards re-initialization, but the timeouts still fire unnecessarily.+ let cancelled = false; retryDelays.forEach((delay) => { const timeout = setTimeout(() => { - if (attemptInitialization()) { - // Clear remaining timeouts if initialization succeeds - timeouts.forEach((t) => clearTimeout(t)); + if (!cancelled && attemptInitialization()) { + cancelled = true; + timeouts.forEach(clearTimeout); } }, delay); timeouts.push(timeout); }); return () => { + cancelled = true; timeouts.forEach((timeout) => clearTimeout(timeout)); };
516-524: Hardcoded string should use translation."Split Terminal" should use the translation function for consistency with other UI strings. Add a translation key like
terminal.split.- title="Split Terminal" + title={t('terminal.split')}
87-133: Consider consolidating dimension-check logic.The dimension detection pattern here (immediate check, ResizeObserver, multiple delayed checks) duplicates similar logic in
useContainerReady. Consider extracting a shared utility hook that handles both use cases to reduce maintenance burden.
6-6: Unused imports:Maximize2,Minimize2.These icons are imported but not used in the component. Remove them or implement the maximize/minimize functionality they were intended for.
-import { Plus, X, SplitSquareVertical, Maximize2, Minimize2 } from 'lucide-react'; +import { Plus, X, SplitSquareVertical } from 'lucide-react';
54-65:containerRefprop is unused.The
containerRefprop is defined in the component signature and passed from the parent, but it's never used insideTerminalSession. Either remove it or implement its intended use.const TerminalSession: React.FC<{ isActive: boolean; isTerminalOpen: boolean; canCreate: boolean; canUpdate: boolean; setFitAddonRef: React.Dispatch<React.SetStateAction<any | null>>; terminalId: string; onFocus: () => void; - containerRef?: React.RefObject<HTMLDivElement>; }> = ({ isActive, isTerminalOpen, canCreate, canUpdate, setFitAddonRef, terminalId, - onFocus, - containerRef + onFocus }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/api/versions.json(1 hunks)view/app/terminal/terminal.tsx(8 hunks)view/app/terminal/utils/isContainerReady.ts(1 hunks)view/app/terminal/utils/useTerminal.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
view/app/terminal/terminal.tsx (6)
view/app/terminal/utils/useTerminal.ts (1)
useTerminal(21-230)view/app/terminal/utils/isContainerReady.ts (1)
useContainerReady(3-84)view/hooks/use-translation.ts (1)
translationKey(16-16)view/lib/rbac.ts (1)
useRBAC(29-146)view/hooks/features_provider.tsx (1)
useFeatureFlags(59-59)view/components/ui/resizable.tsx (3)
ResizablePanelGroup(51-51)ResizablePanel(51-51)ResizableHandle(51-51)
🔇 Additional comments (6)
api/api/versions.json (1)
6-6: LGTM!Version metadata update for the new release date.
view/app/terminal/utils/isContainerReady.ts (1)
73-80: LGTM!Proper cleanup of timeout and animation frame references prevents memory leaks.
view/app/terminal/utils/useTerminal.ts (1)
222-229: LGTM!Good addition of
isWebSocketReadyto the return object, enabling consumers to coordinate initialization with WebSocket state.view/app/terminal/terminal.tsx (3)
281-315: LGTM!Clean implementation with proper
stopPropagationto prevent click events from bubbling through.
383-401: LGTM!Session close logic correctly handles active session switching and cleans up the
activePaneBySessiontracking state.
535-599: LGTM!Good architectural decision to keep inactive sessions mounted with
visibility: hiddenrather than unmounting, preserving terminal state across session switches. The ResizablePanelGroup integration with dynamic defaultSize calculation works well.
Issue
Link to related issue(s):
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.