feat: enhance terminal functionality and settings #202
Conversation
- Added new endpoints for terminal settings: GET and PUT /settings to retrieve and update terminal configurations. - Implemented session limit checks during session creation, returning a 429 status when the limit is reached. - Introduced a new TerminalSection in settings view for customizing terminal appearance and behavior, including font family, default font size, line height, and screen reader mode. - Added support for new terminal features such as search functionality and improved error handling with a TerminalErrorBoundary component. - Updated terminal layout persistence to include session IDs for reconnection and enhanced terminal state management. - Introduced new keyboard shortcuts for terminal actions, including creating new terminal tabs. - Enhanced UI with scrollbar theming for terminal components.
|
Important Review skippedToo many files! 58 files out of 208 files are above the max files limit of 150. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds server-side terminal session limiting, settings endpoints, input/resize validation, and secure token generation. Extends terminal service lifecycle (two-stage kill) and working-dir/env handling. Introduces extensive UI terminal features: persistence, drag/drop tabs and reordering, search/links, image/large-paste handling, settings UI and editor-open integration, keyboard shortcuts, store APIs, and Electron IPC for opening files in an editor. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser UI / TerminalPanel
participant API as Server HTTP API
participant Service as TerminalService
participant WS as WebSocket
UI->>API: POST /sessions (create)
API->>Service: createSession(options)
Service->>Service: if getSessionCount() >= getMaxSessions() -> return null
alt session created
Service-->>API: TerminalSession
API-->>UI: 201 Created (session details)
UI->>WS: Open WS to session
WS->>API: WS connect
API-->>WS: WS open/ack
else capped
Service-->>API: null
API-->>UI: 429 Too Many Requests (current/max)
end
sequenceDiagram
participant UI as Terminal UI
participant Pre as Preload Bridge
participant Main as Electron Main
participant OS as OS handler
UI->>Pre: window.electron.shell.openInEditor(path,line,col)
Pre->>Main: ipcRenderer.invoke("shell:openInEditor", path, line, col)
Main->>OS: shell.openExternal("vscode://file/<encoded>?line=...&column=...")
OS-->>Main: success/failure
Main-->>Pre: { success: true } / { success: false, error }
Pre-->>UI: result
sequenceDiagram
participant UI as Terminal UI
participant Store as AppStore
participant Persist as LocalStorage
participant API as Backend API
UI->>Store: getPersistedTerminalLayout(projectPath)
Store->>Persist: read persisted layout
Persist-->>Store: PersistedTerminalState | null
alt layout found
Store-->>UI: layout
UI->>API: reconnect/create sessions per layout
API-->>UI: session statuses
else no layout
UI->>UI: init default terminals
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
SuperComboGamer
added
Testers-Requested
Summary of ChangesHello @SuperComboGamer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant overhaul of the Automaker terminal system, transforming it into a robust, feature-rich, and production-ready component. The changes span across user experience, navigation, customization, and reliability, addressing numerous bugs and introducing a wide array of modern terminal emulator functionalities. The primary goal is to provide users with a highly efficient, stable, and customizable terminal environment that seamlessly integrates with the project workflow, including project-specific layout persistence and enhanced interaction capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a massive and impressive overhaul of the terminal system, adding a wealth of new features and fixing numerous bugs. The changes significantly improve functionality, reliability, and user experience, bringing the terminal to a production-ready state. Key improvements include per-project layout persistence, robust WebSocket connection handling with heartbeats and exponential backoff, and a dedicated settings UI.
I've identified a few areas for improvement:
- The
package-lock.jsonwas updated to use an SSH URL for a dependency, which could cause build issues for other contributors and in CI environments. This should be reverted to HTTPS. - The terminal layout is saved to
localStorageon every state change, which could be frequent and impact performance. Debouncing this operation would be more efficient. - There are some hardcoded magic numbers for session limits that should be extracted into constants for better maintainability.
Overall, this is an excellent pull request that demonstrates a deep understanding of both frontend and backend development. The attention to detail in features like chunked pasting, ANSI code stripping, and clickable file paths is particularly noteworthy.
This comment was marked as spam.
This comment was marked as spam.
- Refactored session limit checks in terminal settings to use constants for minimum and maximum session values. - Enhanced terminal layout saving mechanism with debouncing to prevent excessive writes during rapid changes. - Updated error messages to reflect new session limit constants.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
apps/server/src/services/terminal-service.ts (1)
202-207: Silent failure whensetMaxSessionsreceives an invalid value.When an out-of-range value is passed, the method silently does nothing. Callers have no indication that their request was ignored. Consider throwing an error or returning a boolean to indicate success/failure.
🔎 Suggested improvement
- setMaxSessions(limit: number): void { - if (limit >= 1 && limit <= 500) { - maxSessions = limit; - console.log(`[Terminal] Max sessions limit updated to ${limit}`); - } - } + setMaxSessions(limit: number): boolean { + if (limit < 1 || limit > 500) { + console.warn(`[Terminal] Invalid max sessions limit: ${limit} (must be 1-500)`); + return false; + } + maxSessions = limit; + console.log(`[Terminal] Max sessions limit updated to ${limit}`); + return true; + }apps/server/src/routes/terminal/routes/settings.ts (2)
28-37: Consider validating thatmaxSessionsis an integer.The current validation accepts any number between 1-500, including floats like
2.5. WhilesetMaxSessionswill store it as-is, floating-point session limits are semantically incorrect and could cause unexpected behavior in comparisons.🔎 Suggested fix
if (typeof maxSessions === "number") { - if (maxSessions < 1 || maxSessions > 500) { + if (!Number.isInteger(maxSessions) || maxSessions < 1 || maxSessions > 500) { res.status(400).json({ success: false, - error: "maxSessions must be between 1 and 500", + error: "maxSessions must be an integer between 1 and 500", }); return; } terminalService.setMaxSessions(maxSessions); }
22-45: PUT with empty body returns 200 success without changes.When the request body doesn't contain
maxSessions(or it's not a number), the handler returns a success response without modifying anything. This behavior is acceptable but may surprise API consumers expecting PUT to require a body. Consider documenting this behavior or returning 400 if no valid settings are provided.apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (1)
1-1: Potential naming conflict withErrorInfoimport.React's
ErrorInfois imported here, but there's also anErrorInfointerface inapps/server/src/lib/error-handler.ts(shown in relevant code snippets). This could cause confusion if both are used in the same codebase. Consider using a named import alias if this becomes an issue, though the current usage is correct for the React error boundary pattern.-import React, { Component, ErrorInfo } from "react"; +import React, { Component, ErrorInfo as ReactErrorInfo } from "react";Then update line 32 to use
ReactErrorInfo.apps/ui/src/config/terminal-themes.ts (1)
33-55: Font options and default look good; consider DRY-ing default with store initialStateThe
TerminalFontOptiontype andTERMINAL_FONT_OPTIONS/DEFAULT_TERMINAL_FONTsetup are clear and used consistently across the UI. One small follow-up:initialState.terminalState.fontFamilyinapps/ui/src/store/app-store.tsis hard‑coded to the same Menlo stack string instead of referencingDEFAULT_TERMINAL_FONT. Wiring the store default to this constant would avoid drift if the first option ever changes.apps/ui/src/components/views/settings-view/terminal/terminal-section.tsx (1)
11-175: Settings wiring is solid; consider selector + shared component for reuseThe component correctly binds all terminal settings to
useAppStoreactions and respects the same ranges as the store clamps, and it reusesTERMINAL_FONT_OPTIONSas intended.Two optional improvements:
- Use a selector, e.g.
useAppStore(state => ({ terminalState: state.terminalState, setTerminalDefaultRunScript: state.setTerminalDefaultRunScript, ... })), to avoid re-rendering on unrelated store changes.- The font / size / line height / scrollback / run-script / screen-reader controls are now implemented here, in the toolbar popover in
TerminalView, and per-panel inTerminalPanel. Extracting a shared “TerminalSettingsContent” subcomponent would reduce duplication and keep behavior/messages in sync.apps/ui/src/components/views/terminal-view.tsx (2)
65-881: Comprehensive terminal layout, DnD, and restore logic looks correctThe added behaviors here—tab DnD/reorder, inline renaming, per-project layout persistence with reconnection + toasts, new-session tracking for
defaultRunScript, maximization, and pane navigation—are all wired consistently to the store APIs and terminal panel props. The project-switch effect correctly saves the previous layout, clears only session state, and rebuilds tabs/layouts with sensible error handling.If this grows further, consider extracting the layout restore/save logic (the
restoreLayouthelpers andrebuildLayout/checkSessionExists/createSession) into a dedicated module or store utility to keepTerminalViewreadable, but functionally this is in good shape.
1222-1325: Terminal settings popover duplicates other settings UIs; consider sharingThe popover in the toolbar exposes default font size, font family, line height, and default run script, wired to the same store actions and ranges as elsewhere. This is functionally fine, but it’s now the third implementation of similar controls (also in
TerminalSectionand the per-panel settings popover inTerminalPanel), which raises the risk of divergence in copy, ranges, or toasts later.Extracting a single shared terminal-settings content component (parametrized by scope: global vs per-panel) would reduce duplication and keep behavior aligned.
apps/ui/src/store/app-store.ts (1)
2500-2543:updateTerminalPanelSizeskey function diverges fromTerminalView’sgetPanelKeyHere
getPanelKeyfor splits is derived from child keys (split-${panel.direction}-${childKeys}), whileTerminalView’sgetPanelKeyreturnspanel.idfor splits. The comment says it “matches” the view helper, but it doesn’t, which makes the mapping betweenpanelKeyspassed fromTerminalViewand the internalsizeMapharder to reason about.It works today because only terminal nodes’
sizeis actually used fordefaultSizein the view, but to avoid subtle future bugs (and the misleading comment) it would be better to:
- Either reuse the same
getPanelKeylogic asTerminalView(based onpanel.idfor splits), or- Extract a shared helper (e.g., in a small
terminal-layout-keysmodule) used by both store and view so they stay in sync.apps/ui/src/components/views/terminal-view/terminal-panel.tsx (4)
431-597: xterm initialization, theme, addons, and link provider are well-structuredThe dynamic imports for xterm + addons, theme resolution via
getTerminalTheme, screen reader / font / scrollback options from store, and WebGL fallback handling are all set up cleanly.The custom file-path link provider is a nice touch: it:
- Parses plausible compiler-style paths with optional line/column.
- Skips URL-like strings.
- Resolves
~/and relative paths viagetElectronAPI+ current project.- Delegates to
openInEditorwith error toasts.Note: the intermediate
lineNum/colNumderived from the first regex aren’t used; that’s harmless but could be trimmed later.
1033-1115: Avoid duplicate font-size effectsYou have two effects that react to
fontSizeandisTerminalReady:
- Lines 1033–1038: update
xtermRef.current.options.fontSizeand callfit().- Lines 1098–1115: do the same, plus send a resize message to the server.
The second effect is strictly more complete and makes the first redundant. Dropping the earlier one would simplify behavior and reduce duplicate work while keeping functionality identical.
1519-1609: Per-panel settings popover is functionally correct but duplicates other settings UIsThe per-panel settings popover correctly:
- Drives local font size via
onFontSizeChange(with reset),- Configures global
defaultRunScript,fontFamily,scrollbackLines,lineHeight, andscreenReaderModewith appropriate sliders and toasts.Functionally this is fine, but it reimplements essentially the same settings as
TerminalSectionand the toolbar popover inTerminalView. Extracting a shared settings content component (parameterized for “global” vs “per-panel” context) would reduce duplication and keep messages and ranges consistent.
1600-1805: Shortcut tooltips/comments are out of sync with actual keybindingsThe
attachCustomKeyEventHandlernow uses Ctrl+Shift variants:
- Ctrl+Shift+D → split right
- Ctrl+Shift+S → split down
- Ctrl+Shift+W → close terminal
- Ctrl+Shift+T → new terminal tab
However:
- The header button tooltips still say
Split Right (Cmd+D),Split Down (Cmd+Shift+D), andClose Terminal (Cmd+W).DEFAULT_KEYBOARD_SHORTCUTSinapp-store.tsstill listsAlt+D/S/W/Tfor terminal shortcuts, andTerminalViewcomments refer to Alt-based shortcuts.This mismatch is confusing for users and for anyone reading the code.
Consider:
- Updating the tooltips here to reflect the new Ctrl/Cmd+Shift shortcuts (e.g., “Split Right (Ctrl+Shift+D / ⌘⇧D)”).
- Aligning the comments and, if those keyboardShortcut entries are still used elsewhere, updating the defaults and any docs to the new scheme.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
apps/server/src/routes/terminal/index.ts(2 hunks)apps/server/src/routes/terminal/routes/sessions.ts(1 hunks)apps/server/src/routes/terminal/routes/settings.ts(1 hunks)apps/server/src/services/terminal-service.ts(4 hunks)apps/ui/eslint.config.mjs(2 hunks)apps/ui/package.json(1 hunks)apps/ui/src/components/ui/keyboard-map.tsx(2 hunks)apps/ui/src/components/views/settings-view.tsx(2 hunks)apps/ui/src/components/views/settings-view/config/navigation.ts(2 hunks)apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts(1 hunks)apps/ui/src/components/views/settings-view/terminal/terminal-section.tsx(1 hunks)apps/ui/src/components/views/setup-view/components/terminal-output.tsx(1 hunks)apps/ui/src/components/views/terminal-view.tsx(16 hunks)apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx(1 hunks)apps/ui/src/components/views/terminal-view/terminal-panel.tsx(35 hunks)apps/ui/src/config/terminal-themes.ts(2 hunks)apps/ui/src/lib/electron.ts(1 hunks)apps/ui/src/lib/http-api-client.ts(1 hunks)apps/ui/src/main.ts(1 hunks)apps/ui/src/preload.ts(1 hunks)apps/ui/src/routes/__root.tsx(1 hunks)apps/ui/src/store/app-store.ts(23 hunks)apps/ui/src/styles/global.css(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/ui/src/components/views/settings-view.tsx (1)
apps/ui/src/components/views/settings-view/terminal/terminal-section.tsx (1)
TerminalSection(11-175)
apps/ui/src/components/views/setup-view/components/terminal-output.tsx (1)
init.mjs (1)
lines(79-79)
apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (1)
apps/server/src/lib/error-handler.ts (1)
ErrorInfo(63-70)
apps/server/src/routes/terminal/index.ts (1)
apps/server/src/routes/terminal/routes/settings.ts (2)
createSettingsGetHandler(9-20)createSettingsUpdateHandler(22-55)
apps/ui/src/components/views/settings-view/terminal/terminal-section.tsx (1)
apps/ui/src/config/terminal-themes.ts (1)
TERMINAL_FONT_OPTIONS(42-50)
apps/server/src/routes/terminal/routes/sessions.ts (1)
apps/server/src/routes/templates/common.ts (1)
logger(11-11)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (3)
apps/ui/src/config/terminal-themes.ts (3)
DEFAULT_TERMINAL_FONT(55-55)getTerminalTheme(483-493)TERMINAL_FONT_OPTIONS(42-50)apps/ui/src/lib/electron.ts (1)
getElectronAPI(560-569)apps/ui/src/lib/http-api-client.ts (1)
saveImageToTemp(365-377)
🪛 Biome (2.1.2)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 242-242: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ 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: claude-review
🔇 Additional comments (26)
apps/ui/src/components/views/setup-view/components/terminal-output.tsx (1)
7-14: LGTM! Theme-aware styling improvements.The migration from hardcoded Tailwind colors (
zinc-900,zinc-400,green-500,zinc-500) to semantic theme tokens (bg-card,text-foreground,text-primary,text-muted-foreground) aligns well with the PR's theming and accessibility objectives. This enables proper system-theme reactivity and improves consistency across the application.apps/server/src/services/terminal-service.ts (4)
228-238: Good addition of locale environment defaults.Setting
LANGandLC_ALLtoen_US.UTF-8by default ensures consistent character handling across platforms, which is important for terminal emulation. This is a solid improvement for cross-platform reliability.
213-218: Good implementation of session limit enforcement.The early return with
nullwhen the limit is reached is clean and allows callers to handle the limit condition appropriately. The error logging provides useful diagnostic information.
395-406: SIGKILL usage is appropriate for WSL reliability.Using
SIGKILLensures forceful termination when shells ignoreSIGTERM/SIGHUP. The cleanup to remove from the map even on error (lines 403-404) is a good defensive practice to prevent orphaned entries.
185-190: LGTM!Simple and correct implementation for retrieving session count.
apps/server/src/routes/terminal/index.ts (1)
24-27: LGTM!The settings routes are properly imported and registered after the
terminalAuthMiddleware, ensuring they require authentication. The pattern is consistent with other protected routes in this file.Also applies to: 46-47
apps/server/src/routes/terminal/routes/sessions.ts (1)
37-50: LGTM!Good implementation of the session limit handling:
- HTTP 429 is the correct status code for this scenario
- The response includes helpful details (current count, max limit, actionable message)
- Using
logger.warnis appropriate for an expected limit condition vs. an unexpected errorapps/server/src/routes/terminal/routes/settings.ts (1)
9-20: LGTM!Clean implementation returning the current settings. The handler correctly retrieves data from the terminal service.
apps/ui/src/routes/__root.tsx (1)
39-42: Good keyboard conflict prevention.This guard correctly prevents the streamer panel shortcut from interfering with terminal input. The selector approach using
.xtermand[data-terminal-container]is appropriate for detecting terminal focus.apps/ui/src/styles/global.css (1)
2933-2951: Terminal scrollbar styling looks good.The terminal scrollbar theming is consistent with existing scrollbar styles in the file. It correctly uses CSS variables for theme compatibility and matches the sizing (8px width, 4px border-radius) used throughout the application.
apps/ui/package.json (1)
63-64: No action needed—addon versions are current and secure.@xterm/addon-search@0.15.0 is the latest version, and @xterm/addon-web-links@0.11.0 is the latest version. Both packages maintain current stable releases with no known security vulnerabilities.
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
3-12: LGTM!The new "terminal" view ID is cleanly integrated into the existing union type, following the established pattern.
apps/ui/src/preload.ts (1)
35-36: LGTM!The preload bridge correctly exposes the new
openInEditorIPC channel with proper type annotations matching the main process handler.apps/ui/src/lib/electron.ts (1)
297-301: LGTM!The interface addition is correctly typed and appropriately marked as optional, consistent with the pattern used for other platform-specific methods in
ElectronAPI.apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (2)
22-44: Well-implemented error boundary.The implementation correctly follows React error boundary patterns with proper state management, useful logging context (including
sessionId), and a clean recovery flow. The separation ofgetDerivedStateFromErrorfor state updates andcomponentDidCatchfor side effects (logging) is correct.
46-93: Good recovery UI with accessible design.The fallback UI provides clear user feedback, WebGL-specific messaging, and an easy recovery path. The collapsible technical details section is a nice touch for debugging without cluttering the main error message.
apps/ui/src/store/app-store.ts (4)
356-365: Split-panelidaddition is appropriate for stable keysAdding
id: stringon thesplitvariant ofTerminalPanelContentis a good move for React key stability and to support persisted layouts. This aligns with howTerminalView’sgetPanelKeyusespanel.idfor split nodes.
777-821: Terminal actions extension and new persistence APIs are consistent with UI usageThe new terminal actions (
toggleTerminalMaximized, default font/run-script/font-family/line-height/scrollback setters, maxSessions, tab reordering, tab layout, panel size updates, and per-project layout persistence) line up with howTerminalViewandTerminalPanelconsume them. The clamping of numeric settings to sane ranges is also a good guardrail.
2058-2080:clearTerminalStatebehavior matches project-switch semanticsPreserving
isUnlocked,authToken, and terminal preferences while clearing tabs/layout/maximized state is exactly what you want for project switching and layout restore. This avoids re-auth prompts and keeps global settings consistent across projects.
2649-2687: Terminal settings migration and partialize wiring look correctThe v2 migration correctly hydrates
terminalStatesettings from theterminalSettingsblob while preserving any existing session/tab state. Thepartializeblock then writesterminalLayoutByProjectandterminalSettingsback out, so subsequent loads see consistent preferences and per-project layouts.This is a solid pattern for evolving terminal settings without breaking older persisted data.
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (6)
300-338: Large-paste chunking and error toasts look robustThe clipboard paste handler correctly:
- Handles empty clipboard and disconnected WebSocket with clear toasts.
- Warns the user for large pastes based on
LARGE_PASTE_WARNING_THRESHOLD.- Sends data via
sendTextInChunkswith an 8KB chunk size and small delay to avoid overwhelming the WebSocket.This is a solid improvement for stability when pasting big blobs into the terminal.
351-379: Search integration via@xterm/addon-searchis wired correctlyThe search handlers (
searchNext,searchPrevious,closeSearch,scrollToBottom) correctly delegate toSearchAddon, handle empty queries, and restore focus to the terminal. TheshowSearchstate andshowSearchRefcoordination tied to the Ctrl/Cmd+Shift+F key handler is also sound.
795-811: WebSocket heartbeat and reconnection logic are thoughtfully implementedThe WebSocket lifecycle now includes:
- A 30-second heartbeat ping with
ponghandling.- Distinct handling for auth failures (4001), normal closure (1000/4003), and “session not found” (4004) with clear toasts.
- Exponential backoff with a cap on reconnection attempts and a “Retry” toast action.
This should materially improve terminal robustness under flaky connections.
1117-1123: Theme updates for xterm on app-theme changes are correctly wiredThe effect that recomputes
getTerminalTheme(resolvedTheme)and assigns it toxtermRef.current.options.themewhenresolvedThemeor readiness changes is a clean way to keep the terminal in sync with app theme (including system dark/light when in “system” mode).
1297-1452: Image drag-and-drop flow is solid and well-defendedThe image DnD logic:
- Filters drag events to only respond when image files are present.
- Enforces a 10MB per-file size limit with clear error toasts.
- Converts to base64, saves via
saveImageToTemp(with a browser fallback path), and then inserts quoted paths into the terminal input.- Shows a friendly overlay with spinner while processing and guards against concurrent processing via
isProcessingImage.This is a nice UX improvement and appears robust.
1809-1901: Search bar and “jump to bottom” UX are well-implementedThe inline search bar:
- Ties directly into the search addon with live-updating matches.
- Provides keyboard navigation (Enter/Shift+Enter) and Escape to close.
- Keeps focus handling tidy.
The “Jump to bottom” button:
- Tracks viewport scroll to toggle visibility.
- Scrolls via xterm’s API and avoids interfering with normal interactions.
Both features are nice quality-of-life additions and appear correct.
This comment was marked as resolved.
This comment was marked as resolved.
…nation improvements - Added path validation in resolveWorkingDirectory to reject paths with null bytes and normalize paths. - Improved killSession method to attempt graceful termination with SIGTERM before falling back to SIGKILL after a delay. - Enhanced logging for session termination to provide clearer feedback on the process.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/server/src/services/terminal-service.ts (1)
16-23: CRITICAL: Resolve the inconsistency between default maxSessions and MAX_MAX_SESSIONS.The default value of
maxSessionsis 1000 (fromTERMINAL_MAX_SESSIONSenvironment variable), butsetMaxSessions()at line 207 only accepts values between 1 and 500. This creates several problems:
- The default value of 1000 cannot be set via the settings API
- Users who configure
TERMINAL_MAX_SESSIONS=750will have a value that can't be replicated through the API- The system starts with a limit that's outside the acceptable range for runtime updates
This inconsistency was flagged in a previous review and remains unresolved.
🔎 Recommended fix
Option 1 (Preferred): Align the default with the API limit:
-let maxSessions = parseInt(process.env.TERMINAL_MAX_SESSIONS || "1000", 10); +let maxSessions = parseInt(process.env.TERMINAL_MAX_SESSIONS || "500", 10);Option 2: If 1000 is truly needed as a default, increase the maximum:
-export const MAX_MAX_SESSIONS = 500; +export const MAX_MAX_SESSIONS = 1000;And update the validation in
setMaxSessions()accordingly (it already usesMAX_MAX_SESSIONS, so it will automatically accept up to 1000).apps/ui/src/components/views/terminal-view.tsx (1)
633-650: LGTM - Debouncing addresses previous review concern.The 500ms debounce prevents excessive localStorage writes during rapid terminal layout changes (e.g., panel resizing, tab switching). The cleanup function properly handles component unmount.
This resolves the performance concern raised in the previous review.
🧹 Nitpick comments (2)
apps/ui/src/components/views/terminal-view.tsx (2)
1269-1273: Consider debouncing or removing slider commit toasts.Toasts appear every time the user releases the slider (onValueCommit). If users adjust the slider multiple times to find the right value, they'll see multiple toasts in quick succession, which can be distracting.
Consider:
- Debouncing toasts to show only once per adjustment session
- Removing toasts entirely (slider visual feedback may be sufficient)
- Using a single dismissible toast that updates rather than stacking
Example: Debounced toast approach
// Add refs to track toast debouncing const fontSizeToastTimeout = useRef<NodeJS.Timeout | null>(null); const lineHeightToastTimeout = useRef<NodeJS.Timeout | null>(null); // In the Slider onValueCommit: onValueCommit={() => { if (fontSizeToastTimeout.current) { clearTimeout(fontSizeToastTimeout.current); } fontSizeToastTimeout.current = setTimeout(() => { toast.info("Font size changed", { description: "New terminals will use this size", }); }, 500); }}Also applies to: 1315-1319
208-1416: Overall assessment: Comprehensive terminal system overhaul.This PR successfully implements a feature-rich terminal interface with:
- ✅ Per-project layout persistence with reconnection
- ✅ Drag-and-drop for tabs and panels with accessibility support
- ✅ Inline tab renaming with double-click
- ✅ Keyboard navigation between panes
- ✅ Settings popover for customization
- ✅ Error boundaries for fault isolation
- ✅ Terminal maximization mode
- ✅ Resource warnings at high session counts
- ✅ Graceful error handling with user feedback
The implementation shows attention to UX details (toasts, debouncing, fallbacks) and accessibility (touch/keyboard sensors, keyboard shortcuts).
Optional refactor consideration:
At 1,400+ lines with multiple concerns (UI, state, API, persistence, DnD, keyboard nav), this component could benefit from extraction into smaller units:
- Custom hooks for layout persistence, DnD logic, and keyboard navigation
- Separate service layer for server communication
- Smaller sub-components for tab bar and settings
However, this is not blocking and can be deferred to a future refactoring PR.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
apps/server/src/routes/terminal/routes/settings.ts(1 hunks)apps/server/src/services/terminal-service.ts(4 hunks)apps/ui/src/components/views/terminal-view.tsx(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/routes/terminal/routes/settings.ts (2)
apps/server/src/services/terminal-service.ts (3)
getTerminalService(508-513)MIN_MAX_SESSIONS(17-17)MAX_MAX_SESSIONS(18-18)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: e2e
🔇 Additional comments (16)
apps/server/src/routes/terminal/routes/settings.ts (1)
9-20: LGTM!The GET handler correctly retrieves and returns current terminal settings from the service.
apps/server/src/services/terminal-service.ts (5)
26-29: LGTM!The reduced throttling (16ms → 4ms) and smaller batch size (8192 → 4096) improve input responsiveness while still preventing floods. The comment clearly explains the rationale for these values.
189-211: LGTM!The new public API methods provide clean access to session count and limits. The validation in
setMaxSessions()properly enforces bounds and logs updates.Note: The
MAX_MAX_SESSIONSlimit affects this method's behavior—see the separate comment about the default value inconsistency.
217-223: LGTM!Returning
nullwhen the session limit is reached provides a clear signal to callers and prevents resource exhaustion. The error logging helps with debugging.
233-242: LGTM!Adding default values for
LANGandLC_ALLimproves terminal consistency across platforms and prevents locale-related issues.
399-408: LGTM!Using
SIGKILLensures forceful termination, which is important for WSL where processes may ignoreSIGTERM. The defensive deletion at line 408 ensures cleanup even if the kill fails.apps/ui/src/components/views/terminal-view.tsx (10)
65-184: LGTM - Well-structured tab button with edit and drag-drop support.The component correctly combines drag and drop functionality with inline editing. The ref combination pattern, focus management, and keyboard handling are all implemented properly. The visual feedback conditions appropriately distinguish between dragging tabs vs. terminal panels.
296-302: LGTM - Accessibility improvement.Adding TouchSensor and KeyboardSensor extends drag-and-drop support to touch devices and keyboard-only users, with appropriate activation constraints to prevent accidental drags.
253-266: LGTM - Helpful resource warning.The warning logic appropriately alerts users when many terminals are open, with a sensible reset mechanism when the count drops below the threshold.
707-734: LGTM - Good error handling and session tracking.Terminal creation properly handles session limits with helpful error messages, refreshes server session count, and correctly marks new sessions for running the default script.
803-831: LGTM - Pragmatic error handling prioritizes UX.The terminal removal logic correctly prioritizes UI cleanup over strict server-client consistency. Even when the server fails to kill a session, removing it from the UI provides better UX than leaving a broken terminal visible.
932-973: LGTM - Well-implemented keyboard navigation.The pane navigation shortcuts (Ctrl/Cmd+Alt+Arrow) provide good keyboard accessibility with proper cross-platform support. The cycling behavior and fallback for no active terminal are intuitive.
983-1011: LGTM - Error boundary provides good fault isolation.Wrapping each terminal in an error boundary prevents crashes from affecting other terminals. The restart mechanism appropriately kills the crashed session and creates a new one, providing good error recovery UX.
306-382: LGTM - Clear drag-and-drop state management.The drag handlers properly distinguish between tab and terminal panel drags, with appropriate state tracking and cleanup. The separation of concerns between tab reordering and terminal movement is well implemented.
898-921: LGTM - Efficient font size lookup.The recursive search for terminal font size across tabs is necessary for the maximized terminal feature and correctly falls back to the default when a terminal isn't found in any tab.
437-628: Store action functions are inherently stable in Zustand; no memoization concern exists.Zustand's design maintains a single instance of state and actions, ensuring that all components using the same hook share the same state and actions. The functions
saveTerminalLayout,getPersistedTerminalLayout,clearTerminalState, andaddTerminalTabare defined inside the Zustand store factory and are part of that single persistent instance. They maintain stable references across renders and do not need memoization to prevent effect runs.The actual performance safeguard in this effect is the early-return guard at line 442 (
if (currentPath === prevPath || isRestoringLayoutRef.current) return;), which prevents unnecessary work. The store functions in the dependency array do not cause the effect to re-run because they are already stable references.Likely an incorrect or invalid review comment.
- Updated the maximum terminal session limit from 500 to 1000 to accommodate more concurrent sessions. - Enhanced path handling in the editor and HTTP API client to normalize file paths for both Unix and Windows systems, ensuring consistent URL encoding.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/server/src/services/terminal-service.ts (1)
17-24: Inconsistency between default (1000) and MAX_MAX_SESSIONS (500) persists.The environment variable
TERMINAL_MAX_SESSIONSdefaults to 1000, butMAX_MAX_SESSIONSis set to 500, andsetMaxSessions()only accepts values up to 500. This inconsistency was previously flagged and remains unresolved.
🧹 Nitpick comments (2)
apps/server/src/services/terminal-service.ts (2)
218-223: Consider adding validation feedback for invalid limits.
setMaxSessions()silently ignores values outside the[MIN_MAX_SESSIONS, MAX_MAX_SESSIONS]range. Adding a warning log or throwing an error would help API consumers understand why their limit wasn't applied.🔎 Proposed enhancement
setMaxSessions(limit: number): void { if (limit >= MIN_MAX_SESSIONS && limit <= MAX_MAX_SESSIONS) { maxSessions = limit; console.log(`[Terminal] Max sessions limit updated to ${limit}`); + } else { + console.warn(`[Terminal] Invalid max sessions limit ${limit}. Must be between ${MIN_MAX_SESSIONS} and ${MAX_MAX_SESSIONS}`); } }
413-432: LGTM!The two-stage shutdown (SIGTERM → SIGKILL after 1 second) is appropriate for graceful termination. The logic correctly handles race conditions and ensures cleanup even if the process doesn't exit gracefully.
Consider adding a comment clarifying that this method initiates an async kill operation and returns immediately:
🔎 Optional documentation enhancement
/** * Kill a terminal session * Attempts graceful SIGTERM first, then SIGKILL after 1 second if still alive + * Note: This method initiates the kill operation and returns immediately. + * The session will be removed from the map when the process exits or after SIGKILL. */ killSession(sessionId: string): boolean {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/src/services/terminal-service.ts(7 hunks)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (6)
apps/server/src/services/terminal-service.ts (6)
12-12: LGTM!The
pathmodule import is necessary for the path normalization logic added later in the file.
27-30: LGTM!The reduced throttle (16ms → 4ms) and batch size (8192 → 4096) align with the PR's goal to fix input lag, especially for backspace. The trade-off between responsiveness and flood prevention appears well-considered.
172-176: LGTM!Null byte validation is a good security practice to prevent potential path traversal or bypass attempts.
183-185: LGTM!Path normalization with
path.resolve()is appropriate for sanitizing user-provided working directories and handling relative paths consistently.
229-234: LGTM!Session limit enforcement is correctly implemented. Returning
nullwhen the limit is reached allows callers to handle the condition gracefully (e.g., the routes respond with HTTP 429).
251-253: LGTM!Setting
LANGandLC_ALLwith UTF-8 defaults ensures consistent character handling across platforms. The"en_US.UTF-8"fallback is widely available on modern systems.
- Added validation to ensure maxSessions is an integer before processing the request. - Responds with a 400 status and an error message if the input is not a valid integer.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/server/src/services/terminal-service.ts (3)
172-185: Consider the behavior ofpath.resolve()with relative paths.The null-byte sanitization (lines 172-176) is excellent for security. However,
path.resolve(cwd)at line 185 will resolve relative paths againstprocess.cwd()(the server's working directory), not against the user's home directory or any other expected base.If the client can send relative paths like
../../foo, the resolved path might be unexpected. Consider either:
- Rejecting relative paths explicitly
- Resolving them relative to
homeDirinstead- Documenting that relative paths are resolved against the server process CWD
🔎 Example: Ensure absolute paths only
// Normalize the path to resolve . and .. segments // This converts relative paths to absolute and cleans up the path + // Only accept absolute paths for security + if (!path.isAbsolute(cwd)) { + console.warn(`[Terminal] Rejecting relative path: ${cwd}, falling back to home`); + return homeDir; + } cwd = path.resolve(cwd);
394-439: Consider making the SIGKILL timeout configurable.The two-stage termination (SIGTERM → SIGKILL after 1 second) is a solid approach for graceful-then-forceful shutdown. The defensive cleanup at lines 436-437 ensures sessions are removed even on errors.
However, the 1-second timeout is hardcoded and might be too aggressive for shells with complex cleanup handlers or long-running child processes. Consider extracting this as a configurable constant or parameter for flexibility in different deployment scenarios.
🔎 Example: Make timeout configurable
+ // Time to wait for graceful SIGTERM before sending SIGKILL + const KILL_TIMEOUT_MS = parseInt(process.env.TERMINAL_KILL_TIMEOUT || "1000", 10); + killSession(sessionId: string): boolean { // ... existing code ... // Schedule SIGKILL fallback if process doesn't exit gracefully setTimeout(() => { // ... existing timeout logic ... - }, 1000); + }, KILL_TIMEOUT_MS);
8-8: Consider upgrading to the latest node-pty beta version or the stable 1.0.0 release.The code uses node-pty v1.1.0-beta41, which is a pre-release version. Newer beta versions are available (currently at beta34). If stability is a priority, consider either upgrading to the latest beta, or reverting to the stable 1.0.0 release and deferring the 1.1.0 upgrade until it exits beta. Ensure comprehensive testing regardless of which version is used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/services/terminal-service.ts(7 hunks)apps/ui/src/lib/http-api-client.ts(1 hunks)apps/ui/src/main.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/ui/src/main.ts
- apps/ui/src/lib/http-api-client.ts
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (6)
apps/server/src/services/terminal-service.ts (6)
12-12: LGTM!The
pathmodule import is appropriately added to support the new path normalization logic inresolveWorkingDirectory.
17-24: LGTM! Past review concern addressed.The session limit constants and default configuration are now consistent. The validation in
setMaxSessions()(line 219) now accepts values from 1 to 1000, which properly accommodates the default value of 1000 from the environment variable. This resolves the inconsistency flagged in the previous review.
27-30: LGTM! Well-justified responsiveness improvement.The reduced throttle timing (16ms → 4ms) and smaller batch size (8192 → 4096) address perceived input lag while still providing flood protection. The comment clearly explains the rationale, particularly for backspace responsiveness.
201-223: LGTM! Clean public API for session management.The getter and setter methods are well-implemented with proper bounds validation, clear naming, and useful logging. The validation range aligns with the exported constants.
229-234: LGTM! Proper session limit enforcement.The session limit check is correctly implemented at the start of
createSession(). Returningnullwhen the limit is reached provides a clear failure signal that the calling routes can handle appropriately (e.g., returning HTTP 429).
245-253: LGTM! Sensible locale defaults.The environment variable defaults ensure consistent UTF-8 terminal behavior across platforms. The fallback chain for
LC_ALL(line 253) respects existing environment configuration while providing a reasonable default.
…dation - Changed the token generation method to use slice instead of substr for better readability. - Enhanced maxSessions validation in the settings update handler to check for undefined values and ensure the input is a number before processing.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/server/src/routes/terminal/routes/settings.ts (1)
28-44: Validate invalid maxSessions types instead of silently ignoring them.When
maxSessionsis provided but is not a number (e.g.,"100",true,null,{}), the handler silently skips validation and returnssuccess: truewith unchanged settings. This provides no feedback to clients about the invalid input.Clients expecting validation will be confused when they send
{ maxSessions: "100" }and receive a success response with the old value.🔎 Proposed validation enhancement
const { maxSessions } = req.body; + if (maxSessions !== undefined && typeof maxSessions !== "number") { + res.status(400).json({ + success: false, + error: "maxSessions must be a number", + }); + return; + } + if (typeof maxSessions === "number") { if (!Number.isInteger(maxSessions)) { res.status(400).json({ success: false, error: "maxSessions must be an integer", }); return; } if (maxSessions < MIN_MAX_SESSIONS || maxSessions > MAX_MAX_SESSIONS) { res.status(400).json({ success: false, error: `maxSessions must be between ${MIN_MAX_SESSIONS} and ${MAX_MAX_SESSIONS}`, }); return; } terminalService.setMaxSessions(maxSessions); }Based on previous review feedback that identified this same issue.
apps/ui/src/components/views/terminal-view.tsx (2)
652-670: Debounced save may trigger frequently.The
saveTerminalLayoutis debounced with 500ms delay, butterminalState.tabscan change frequently (e.g., during panel resizing, tab operations). While the debouncing helps, consider if the 500ms delay is optimal for your use case.As noted in previous reviews, this could still lead to frequent localStorage writes during active terminal usage. The current implementation is functional but may benefit from further optimization if performance issues arise.
582-582: Replace deprecatedsubstrwithslice.The
substrmethod is deprecated. Usesliceinstead for future compatibility.🔎 Proposed fix
- id: persisted.id || `split-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, + id: persisted.id || `split-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`,
🧹 Nitpick comments (1)
apps/ui/src/components/views/terminal-view.tsx (1)
475-643: Consider extracting helpers to reduce function complexity.The
restoreLayoutasync function spans 168 lines and contains three nested helper functions (checkSessionExists,createSession,rebuildLayout). While the logic is correct, this high complexity reduces readability and testability.Consider extracting the helper functions to module scope or as separate utility functions. This would make the code easier to understand, test, and maintain.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/src/routes/terminal/routes/settings.ts(1 hunks)apps/ui/src/components/views/terminal-view.tsx(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/server/src/routes/terminal/routes/settings.ts (2)
apps/server/src/services/terminal-service.ts (3)
getTerminalService(537-542)MIN_MAX_SESSIONS(18-18)MAX_MAX_SESSIONS(19-19)apps/server/src/routes/common.ts (1)
getErrorMessage(370-372)
apps/ui/src/components/views/terminal-view.tsx (3)
apps/ui/src/store/app-store.ts (3)
TerminalTab(368-372)PersistedTerminalPanel(392-400)TerminalPanelContent(357-365)apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (1)
TerminalErrorBoundary(22-94)apps/ui/src/config/terminal-themes.ts (1)
TERMINAL_FONT_OPTIONS(42-50)
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: claude-review
🔇 Additional comments (3)
apps/server/src/routes/terminal/routes/settings.ts (1)
1-8: LGTM! Imports are well-structured.The imports correctly use the constants from
terminal-service.tsrather than hardcoding values, which addresses the maintainability concern raised in previous reviews.apps/ui/src/components/views/terminal-view.tsx (2)
835-849: Excellent error handling for terminal cleanup.The improved error handling always removes the terminal from the UI, even when the server returns 404 or network errors occur. This provides much better UX by avoiding broken terminal instances remaining in the interface. The graceful handling of 404 status codes (session already gone) is particularly well thought out.
1003-1031: Excellent error isolation with boundaries.Wrapping each terminal instance in a
TerminalErrorBoundaryis a robust approach that prevents a crash in one terminal from affecting others. The restart logic correctly kills the old session and creates a new one. The conditionalrunCommandOnConnectensures commands only run on new sessions, not on restored ones.
This comment was marked as resolved.
This comment was marked as resolved.
- Added validation for terminal input to ensure it is a string and limited to 1MB to prevent memory issues. - Implemented checks for terminal resize dimensions to ensure they are positive integers within specified bounds. - Updated keyboard shortcuts for terminal actions to use Alt key combinations instead of Ctrl+Shift for better accessibility.
This comment was marked as resolved.
This comment was marked as resolved.
…ce path normalization - Wrapped the terminal settings retrieval in a try-catch block to handle potential errors and respond with a 500 status and error details. - Updated path normalization logic to skip resolution for WSL UNC paths, preventing potential issues with path handling in Windows Subsystem for Linux. - Enhanced unit tests for session termination to include timer-based assertions for graceful session killing.
This comment was marked as resolved.
This comment was marked as resolved.
- Implemented spatial navigation between terminal panes using directional shortcuts (Ctrl+Alt+Arrow keys). - Improved session handling by ensuring stale sessions are automatically removed when the server indicates they are invalid. - Added customizable keyboard shortcuts for terminal actions and enhanced search functionality with dedicated highlighting colors. - Updated terminal themes to include search highlighting colors for better visibility during searches. - Refactored terminal layout saving logic to prevent incomplete state saves during project restoration.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/terminal-view.tsx (1)
305-377: Avoid treating dragged tabs as terminals when dropped on the “New Tab” drop zoneIn
handleDragEnd, when dragging a tab (activeData.type === "drag-tab") over the"new-tab"droppable, the code falls through to the terminal-panel branch and callsmoveTerminalToTabwithactive.id("drag-tab-...") as the sessionId. That lookup is a no-op today, but it’s confusing and suggests mixed semantics between tab and panel DnD.Worth short‑circuiting tab drags for non‑tab targets (including
"new-tab") so you either explicitly support “drag tab to new tab zone” or ignore that drop cleanly.Possible guard to keep behaviors separated
const handleDragEnd = useCallback((event: DragEndEvent) => { const { active, over } = event; const activeData = active.data?.current; // Reset drag states setActiveDragId(null); setActiveDragTabId(null); setDragOverTabId(null); if (!over) return; const overData = over.data?.current; // Handle tab-to-tab drag (reordering) if (activeData?.type === "drag-tab" && overData?.type === "tab") { const fromTabId = activeData.tabId as string; const toTabId = overData.tabId as string; if (fromTabId !== toTabId) { reorderTerminalTabs(fromTabId, toTabId); } return; } + // For tab drags over non-tab targets (including "new-tab"), ignore gracefully + if (activeData?.type === "drag-tab") { + return; + } + // Handle terminal panel drops const activeId = active.id as string; ...apps/ui/src/store/app-store.ts (1)
183-215: Address browser keyboard shortcut conflicts in terminal defaultsThe code comment claims using Alt modifier avoids browser shortcuts, but Alt+D moves the cursor to the browser's address bar and Alt+S can be intercepted by browsers, preventing access to the browser's history menu. Current defaults (Alt+D, Alt+S, Alt+W, Alt+T) conflict with these common browser shortcuts. Consider switching to a less conflicting pattern like Ctrl+Shift combinations, which are more commonly reserved for application-level actions, and update the code comment and documentation accordingly.
♻️ Duplicate comments (1)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
265-273: RefactorstripAnsiregex to avoid literal control characters (Biome error)
stripAnsiembeds raw control characters (\x1b,\x07, etc.) directly in the regex, which triggers Biome’snoControlCharactersInRegexand can break CI. It also recreates the regex on every call.You can precompile a shared
RegExpusing escaped Unicode code points and reuse it instripAnsi.Suggested `ANSI_ESCAPE_REGEX` helper
+// Precompiled regex for ANSI escape sequences: +// - CSI sequences: ESC [ ... letter +// - OSC sequences: ESC ] ... BEL +// - Other common escape forms used by terminals +const ANSI_ESCAPE_REGEX = new RegExp( + String.raw`\u001b\[[0-9;]*[A-Za-z]|\u001b\][^\u0007]*\u0007|\u001b[()][AB012]|\u001b[>=<]|\u001b[78HM]|\u001b#[0-9]|\u001b.`, + "g", +); + // Strip ANSI escape codes from text const stripAnsi = (text: string): string => { - // Match ANSI escape sequences: - // - CSI sequences: \x1b[...letter - // - OSC sequences: \x1b]...ST - // - Other escape sequences: \x1b followed by various characters - // eslint-disable-next-line no-control-regex - return text.replace(/\x1b\[[0-9;]*[a-zA-Z]|\x1b\][^\x07]*\x07|\x1b[()][AB012]|\x1b[>=<]|\x1b[78HM]|\x1b#[0-9]|\x1b./g, ''); + return text.replace(ANSI_ESCAPE_REGEX, ""); };
🧹 Nitpick comments (2)
apps/ui/src/store/app-store.ts (2)
403-404: Usesliceinstead of deprecatedsubstrfor split IDs
generateSplitIdstill usessubstr, which is deprecated. Behavior is easy to mirror withsliceand keeps you consistent with other updated call sites.Small modernization
-const generateSplitId = () => `split-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; +const generateSplitId = () => + `split-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;
2155-2162: Confirm whether UImaxSessionsclamp (1–500) should match server’s 1–1000 range
setTerminalMaxSessionscurrently clamps to 1–500, while the server-side constants (MIN_MAX_SESSIONS/MAX_MAX_SESSIONS) allow up to 1000. If users are expected to configure up to 1000 sessions from the UI/settings page, this clamp will silently prevent that.If the narrower UI range is intentional (with env-only overrides for higher values), consider documenting that; otherwise, bump the upper bound here to match the server.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/server/src/index.ts(1 hunks)apps/server/src/routes/terminal/common.ts(2 hunks)apps/server/src/routes/terminal/routes/settings.ts(1 hunks)apps/server/src/services/terminal-service.ts(7 hunks)apps/server/tests/unit/services/terminal-service.test.ts(2 hunks)apps/ui/src/components/views/terminal-view.tsx(16 hunks)apps/ui/src/components/views/terminal-view/terminal-panel.tsx(36 hunks)apps/ui/src/config/terminal-themes.ts(17 hunks)apps/ui/src/hooks/use-keyboard-shortcuts.ts(1 hunks)apps/ui/src/store/app-store.ts(25 hunks)docs/terminal.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/routes/terminal/routes/settings.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/hooks/use-keyboard-shortcuts.ts (1)
apps/ui/src/store/app-store.ts (1)
parseShortcut(55-87)
apps/ui/src/components/views/terminal-view.tsx (4)
apps/ui/src/store/app-store.ts (3)
TerminalTab(368-372)PersistedTerminalPanel(392-400)TerminalPanelContent(357-365)apps/server/src/services/terminal-service.ts (1)
createSession(231-339)apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (1)
TerminalErrorBoundary(22-94)apps/ui/src/config/terminal-themes.ts (1)
TERMINAL_FONT_OPTIONS(47-55)
🪛 Biome (2.1.2)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (14)
apps/server/src/routes/terminal/common.ts (1)
5-5: LGTM! Secure token generation using cryptographic randomness.The switch from pseudo-random to
crypto.randomBytes(32)provides 256 bits of cryptographic entropy. Usingbase64urlencoding is appropriate for tokens that may appear in URLs or headers without escaping issues.Also applies to: 52-57
apps/server/src/index.ts (2)
300-312: LGTM! Input validation prevents memory exhaustion and type confusion.The 1MB limit is reasonable for terminal input, and type checking prevents injection of non-string payloads. Good defensive coding.
314-356: LGTM with minor redundancy noted.The bounds validation (cols ≤1000, rows ≤500) prevents resource exhaustion from malformed resize requests. Silent ignoring is appropriate for invalid dimensions.
Note: The conditional
if (msg.cols && msg.rows)on line 329 is now redundant since the validation above guarantees both are positive integers ≥1, but this doesn't affect correctness.apps/server/tests/unit/services/terminal-service.test.ts (1)
409-429: LGTM! Test correctly validates the two-stage termination flow.The test properly verifies:
SIGTERMis sent on initial kill- After 1 second timeout,
SIGKILLis sent- Session is removed after forced termination
Good use of
vi.useFakeTimers()to control time advancement deterministically.apps/ui/src/hooks/use-keyboard-shortcuts.ts (2)
80-134: LGTM! Keyboard-layout independent key-to-code mapping.This addresses the issue where Alt+key combinations produce special characters on non-US keyboard layouts. Using
event.codeinstead ofevent.keyensures shortcuts work consistently across layouts.Good coverage of:
- Letters/digits → KeyX/DigitX
- Punctuation with shift variants (e.g.,
~and`both map toBackquote)- Special keys (arrows, Enter, Escape, etc.)
136-174: LGTM! Layout-independent shortcut matching for terminals.The function correctly:
- Parses the shortcut string using the shared
parseShortcutfunction- Converts to
event.codefor layout independence- Enforces exact modifier matching (prevents false positives from extra modifiers)
This is essential for terminal shortcuts where Alt combinations produce special characters on international layouts.
apps/ui/src/config/terminal-themes.ts (3)
31-36: LGTM! Search highlighting properties for xterm SearchAddon.The new interface properties provide complete control over search match styling:
- Background and border for regular matches
- Separate styling for the active (current) match
38-61: LGTM! Terminal font options with sensible defaults.Good selection of cross-platform monospace fonts with appropriate fallback chains. Using
TERMINAL_FONT_OPTIONS[0].valuefor the default is safe since the array is non-empty.
273-277: LGTM! Solarized contrast improvement.Changing foreground from
#839496(base0) to#93a1a1(base1) improves text readability on the#002b36background. This aligns with WCAG contrast recommendations for the Solarized palette.docs/terminal.md (1)
35-41: Documentation matches implementation—no shortcut inconsistency.The documentation correctly shows
Alt+T,Alt+D,Alt+S, andAlt+Wshortcuts, which are consistent with the actual implementation inapps/ui/src/store/app-store.tsand the UI components. There is no mismatch between documentation and codebase.Likely an incorrect or invalid review comment.
apps/ui/src/components/views/terminal-view.tsx (2)
933-956:findTerminalFontSizeimplementation looks solidThe recursive search across all tabs for a session’s
fontSize, with fallback toterminalState.defaultFontSize, is clear and handles nested splits correctly. This pairs well with the maximized-panel view’s reuse viafindTerminalFontSize.
655-685: Nice debounced layout persistence to avoidlocalStoragechurnThe 500 ms debounce plus projectPath capture and skip-while-restoring guard gives you robust per-project layout persistence without hammering storage during rapid tab/panel changes.
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (2)
307-368: Large-paste chunking and clipboard toasts are well-structuredThe
sendTextInChunkshelper plusLARGE_PASTE_WARNING_THRESHOLDand detailed error toasts give good UX for both normal and very large pastes, while protecting the WebSocket with 8 KB chunks and a small delay.
557-681: Clickable file-path link provider is robust and project-awareThe custom
filePathLinkProvidercorrectly:
- Parses a variety of absolute/relative path formats with optional
:line:col.- Skips URL-like strings.
- Resolves
~/via Electron’sgetPath('home')and relative paths viacurrentProject.path.- Delegates to
openInEditorwith proper error toasts.This is a nice integration point between terminal output and the editor.
| // Session limit constants - shared with routes/settings.ts | ||
| export const MIN_MAX_SESSIONS = 1; | ||
| export const MAX_MAX_SESSIONS = 1000; | ||
|
|
||
| // Maximum number of concurrent terminal sessions | ||
| // Can be overridden via TERMINAL_MAX_SESSIONS environment variable | ||
| // Default set to 1000 - effectively unlimited for most use cases | ||
| let maxSessions = parseInt(process.env.TERMINAL_MAX_SESSIONS || "1000", 10); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Harden TERMINAL_MAX_SESSIONS parsing to avoid NaN disabling limits
maxSessions is parsed once from TERMINAL_MAX_SESSIONS, but an invalid value (e.g. "abc") will yield NaN. That makes checks like this.sessions.size >= maxSessions always false and also exposes NaN via getMaxSessions().
Consider clamping the env value into [MIN_MAX_SESSIONS, MAX_MAX_SESSIONS] and falling back to a sane default when parsing fails.
Proposed adjustment
-// Maximum number of concurrent terminal sessions
-// Can be overridden via TERMINAL_MAX_SESSIONS environment variable
-// Default set to 1000 - effectively unlimited for most use cases
-let maxSessions = parseInt(process.env.TERMINAL_MAX_SESSIONS || "1000", 10);
+// Maximum number of concurrent terminal sessions
+// Can be overridden via TERMINAL_MAX_SESSIONS environment variable
+// Default set to 1000 - effectively unlimited for most use cases
+const envMaxSessions = parseInt(process.env.TERMINAL_MAX_SESSIONS || "1000", 10);
+let maxSessions =
+ Number.isFinite(envMaxSessions)
+ ? Math.min(
+ MAX_MAX_SESSIONS,
+ Math.max(MIN_MAX_SESSIONS, envMaxSessions),
+ )
+ : 1000;🤖 Prompt for AI Agents
In apps/server/src/services/terminal-service.ts around lines 17 to 25, the code
parses TERMINAL_MAX_SESSIONS with parseInt which can produce NaN (disabling
limits and leaking NaN via getMaxSessions); change parsing to validate the
result and clamp into [MIN_MAX_SESSIONS, MAX_MAX_SESSIONS] with a sane fallback:
parse the env var, if it's not a finite integer use the default (1000), then set
maxSessions = Math.min(MAX_MAX_SESSIONS, Math.max(MIN_MAX_SESSIONS,
parsedOrDefault)) so maxSessions is always a valid number within bounds.
| updateTerminalPanelSizes: (tabId, panelKeys, sizes) => { | ||
| const current = get().terminalState; | ||
| const tab = current.tabs.find((t) => t.id === tabId); | ||
| if (!tab || !tab.layout) return; | ||
|
|
||
| // Create a map of panel key to new size | ||
| const sizeMap = new Map<string, number>(); | ||
| panelKeys.forEach((key, index) => { | ||
| sizeMap.set(key, sizes[index]); | ||
| }); | ||
|
|
||
| // Helper to generate panel key (matches getPanelKey in terminal-view.tsx) | ||
| const getPanelKey = (panel: TerminalPanelContent): string => { | ||
| if (panel.type === "terminal") return panel.sessionId; | ||
| const childKeys = panel.panels.map(getPanelKey).join("-"); | ||
| return `split-${panel.direction}-${childKeys}`; | ||
| }; | ||
|
|
||
| // Recursively update sizes in the layout | ||
| const updateSizes = (panel: TerminalPanelContent): TerminalPanelContent => { | ||
| const key = getPanelKey(panel); | ||
| const newSize = sizeMap.get(key); | ||
|
|
||
| if (panel.type === "terminal") { | ||
| return newSize !== undefined ? { ...panel, size: newSize } : panel; | ||
| } | ||
|
|
||
| return { | ||
| ...panel, | ||
| size: newSize !== undefined ? newSize : panel.size, | ||
| panels: panel.panels.map(updateSizes), | ||
| }; | ||
| }; | ||
|
|
||
| const updatedLayout = updateSizes(tab.layout); | ||
|
|
||
| const newTabs = current.tabs.map((t) => | ||
| t.id === tabId ? { ...t, layout: updatedLayout } : t | ||
| ); | ||
|
|
||
| set({ | ||
| terminalState: { ...current, tabs: newTabs }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Panel-size persistence uses a different key scheme than terminal-view, so sizes won’t round-trip reliably
updateTerminalPanelSizes reconstructs keys via:
const getPanelKey = (panel: TerminalPanelContent): string => {
if (panel.type === "terminal") return panel.sessionId;
const childKeys = panel.panels.map(getPanelKey).join("-");
return `split-${panel.direction}-${childKeys}`;
};But terminal-view.tsx’s getPanelKey (used to build panelKeys passed into this action) returns panel.id for splits. These schemes don’t match, so sizeMap.get(key) will never find entries for split panels, and even for terminals you risk mismatches as layouts evolve, which breaks size persistence.
I’d align this helper with the view’s getPanelKey and rely on the stable split id that’s already being generated.
Align key generation with `terminal-view`
- // Helper to generate panel key (matches getPanelKey in terminal-view.tsx)
- const getPanelKey = (panel: TerminalPanelContent): string => {
- if (panel.type === "terminal") return panel.sessionId;
- const childKeys = panel.panels.map(getPanelKey).join("-");
- return `split-${panel.direction}-${childKeys}`;
- };
+ // Helper to generate panel key – must match getPanelKey in terminal-view.tsx
+ const getPanelKey = (panel: TerminalPanelContent): string => {
+ if (panel.type === "terminal") {
+ return panel.sessionId;
+ }
+ // Use stable split id so keys remain consistent across renders
+ return panel.id;
+ };🤖 Prompt for AI Agents
In apps/ui/src/store/app-store.ts around lines 2503 to 2546, the helper
getPanelKey builds keys differently than terminal-view.tsx (it composes split
keys from direction+child keys), so sizeMap lookups fail; change getPanelKey to
return the panel's stable id (the same id terminal-view uses for both terminal
and split panels) so the keys match, and remove the composed key logic; ensure
any callers/types still accept returning panel.id and update any comments
accordingly.
- Added functionality to collect and kill all terminal sessions on the server before clearing terminal state to prevent orphaned processes. - Implemented cleanup of terminal sessions during page unload using sendBeacon for reliable delivery. - Refactored terminal state clearing logic to ensure server sessions are terminated before switching projects. - Improved handling of search decorations to prevent visual artifacts during terminal disposal and content restoration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/ui/src/components/views/terminal-view.tsx (1)
751-777: Layout save debouncing is well-implemented.The 500ms debounce effectively prevents excessive localStorage writes during rapid changes (like panel resizing). The implementation correctly:
- Skips saving during restore (line 754)
- Captures project path at schedule time to handle project switches (lines 760-761)
- Validates path hasn't changed before saving (line 764)
The gemini-code-assist comment about frequent writes is already addressed by this debounce mechanism.
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
265-273: Replace raw control characters with Unicode escapes in ANSI regex.Biome's
noControlCharactersInRegexrule flags the literal control characters. Use Unicode escape sequences for better compatibility and to satisfy linters.🔎 Recommended fix
+ // Precompiled regex for ANSI escape sequences + const ANSI_ESCAPE_REGEX = new RegExp( + String.raw`\u001b\[[0-9;]*[A-Za-z]|\u001b\][^\u0007]*\u0007|\u001b[()][AB012]|\u001b[>=<]|\u001b[78HM]|\u001b#[0-9]|\u001b.`, + "g" + ); + // Strip ANSI escape codes from text const stripAnsi = (text: string): string => { - // Match ANSI escape sequences: - // - CSI sequences: \x1b[...letter - // - OSC sequences: \x1b]...ST - // - Other escape sequences: \x1b followed by various characters - // eslint-disable-next-line no-control-regex - return text.replace(/\x1b\[[0-9;]*[a-zA-Z]|\x1b\][^\x07]*\x07|\x1b[()][AB012]|\x1b[>=<]|\x1b[78HM]|\x1b#[0-9]|\x1b./g, ''); + return text.replace(ANSI_ESCAPE_REGEX, ''); };
🧹 Nitpick comments (1)
apps/ui/src/components/views/terminal-view.tsx (1)
472-511: Consider bulk session cleanup endpoint for better unload performance.The current implementation uses synchronous XHR to delete sessions individually during page unload. While functional, this can block the browser and delay navigation.
Consider adding a bulk-delete endpoint that accepts POST via
sendBeacon:const payload = JSON.stringify({ sessionIds }); const blob = new Blob([payload], { type: 'application/json' }); navigator.sendBeacon(`${serverUrl}/api/terminal/sessions/bulk-delete`, blob);This would be non-blocking and more reliable during unload. The current implementation is acceptable as a best-effort fallback.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/components/views/terminal-view.tsx(16 hunks)apps/ui/src/components/views/terminal-view/terminal-panel.tsx(35 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (4)
apps/ui/src/store/app-store.ts (2)
KeyboardShortcuts(151-180)DEFAULT_KEYBOARD_SHORTCUTS(183-215)apps/ui/src/config/terminal-themes.ts (3)
getTerminalTheme(568-578)DEFAULT_TERMINAL_FONT(60-60)TERMINAL_FONT_OPTIONS(47-55)apps/ui/src/lib/electron.ts (1)
getElectronAPI(560-569)apps/ui/src/hooks/use-keyboard-shortcuts.ts (1)
matchesShortcutWithCode(141-174)
apps/ui/src/components/views/terminal-view.tsx (4)
apps/ui/src/store/app-store.ts (2)
TerminalPanelContent(357-365)PersistedTerminalPanel(392-400)apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsx (1)
TerminalErrorBoundary(22-94)apps/ui/src/components/views/terminal-view/terminal-panel.tsx (1)
TerminalPanel(87-2119)apps/ui/src/config/terminal-themes.ts (1)
TERMINAL_FONT_OPTIONS(47-55)
🪛 Biome (2.1.2)
apps/ui/src/components/views/terminal-view/terminal-panel.tsx
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 272-272: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (18)
apps/ui/src/components/views/terminal-view.tsx (7)
42-50: LGTM! Excellent accessibility enhancement.Adding
TouchSensorandKeyboardSensorto the DnD configuration ensures that terminal tabs and panels can be reordered via touch devices and keyboard, not just mouse drag. This significantly improves accessibility.
83-140: Well-implemented inline tab renaming.The double-click-to-rename pattern is intuitive, and the implementation handles:
- Focus management and text selection on edit start
- Enter to confirm, Escape to cancel
- Trim validation to prevent empty names
- Blur to finish editing
The
stopPropagationon the close button (line 176) correctly prevents blur conflicts.
332-346: Excellent DnD sensor configuration.The activation constraints prevent accidental drags:
- 8px distance for pointer prevents accidental activation
- 200ms delay for touch prevents conflict with scrolling
- KeyboardSensor inclusion enables keyboard-only navigation
This is a well-balanced configuration for accessibility and UX.
253-266: Excellent resource management with user-friendly warnings.The 20-terminal warning proactively alerts users about resource usage before hitting hard limits. The implementation:
- Shows warning only once per session until count drops
- Provides clear explanation of resource impact
- Uses appropriate 8-second duration for important info
This helps prevent users from accidentally exhausting system resources.
1444-1547: Well-designed global terminal settings UI.The settings popover provides centralized control with:
- Appropriate ranges for font size (8-24px) and line height (1.0-2.0)
- Clear feedback via toasts for changes
- Explicit warnings when restart is required
- Default run script for automation workflows
The UX is intuitive and the implementation is clean.
1061-1146: Sophisticated spatial navigation algorithm.The
navigateToTerminalfunction correctly implements directional pane navigation:
- Maps arrow keys to appropriate split directions (horizontal for left/right, vertical for up/down)
- Traverses the layout tree to find spatially adjacent terminals
- Handles forward/backward movement within subtrees
- Gracefully handles edge cases (no active terminal, no adjacent terminal)
The algorithm provides intuitive navigation that matches the visual layout.
1553-1587: Clean implementation of terminal maximize feature.The maximized terminal rendering:
- Properly isolates the maximized terminal from the layout tree
- Uses separate error boundary with correct recovery logic
- Passes appropriate props (isMaximized=true, correct fontSize)
- Safe use of non-null assertions within the conditional check
This provides a focused full-screen terminal experience.
apps/ui/src/components/views/terminal-view/terminal-panel.tsx (11)
307-368: Well-implemented large paste chunking.The chunking mechanism prevents overwhelming the WebSocket with large pastes:
- Appropriate chunk size (8KB) and delay (10ms)
- User feedback via toast warning for >1MB pastes
- Checks connection status before each chunk
Note: If the connection closes mid-paste, remaining chunks are silently dropped (line 320 check). This is acceptable behavior for a failed connection scenario.
576-680: Excellent file path link provider implementation.The custom link provider intelligently detects and opens file paths from terminal output:
- Handles multiple path formats (absolute, relative, home directory, Windows)
- Correctly skips URLs to avoid false positives
- Integrates with editor via
openInEditorAPI- Provides helpful error messages for relative paths without project context
- Properly disposed on cleanup to prevent memory leaks
The regex pattern covers common compiler/linter output formats well. If you encounter false positives/negatives in specific tools, the pattern can be refined iteratively.
1446-1601: Robust image drag-and-drop implementation.The image handling feature is well-implemented with:
- Comprehensive validation (file type, size)
- Proper async sequencing with processing state
- Intelligent drag-leave handling to prevent flicker (lines 1523-1526)
- Shell-safe path formatting (quoted if spaces present)
- Helpful toast feedback for all outcomes
- Fallback path for browser mode
The integration with the Electron API for saving images is clean and includes error handling.
930-1094: Excellent connection management with heartbeat and exponential backoff.The WebSocket connection handling is production-ready:
- 30-second heartbeat prevents proxy/LB timeouts
- Exponential backoff (1s→16s) with 5-attempt limit
- Intelligent error code handling (auth failure, normal close, session not found)
- User-friendly retry mechanism via toast action
- Clean separation: normal closes don't retry, auth failures prompt re-auth, missing sessions trigger cleanup
The
onSessionInvalidcallback (line 1050) elegantly handles server restarts by notifying the parent component.
983-1005: Intelligent initial command execution with shell-aware delays.The command execution logic correctly:
- Detects Windows shells (PowerShell, cmd) via path and name
- Uses appropriate line endings (\r\n for Windows, \n for Unix)
- Applies longer delay (500ms) for PowerShell due to profile loading
- Prevents duplicate execution via ref flag
- Notifies parent component via callback
This handles cross-platform shell differences elegantly.
200-222: Proper system theme resolution for xterm.The implementation correctly:
- Tracks system dark mode preference via
matchMedia- Listens for system theme changes and updates accordingly
- Resolves "system" theme to concrete light/dark for xterm
- Cleans up event listener on unmount
This ensures xterm always receives a valid theme value (not "system"), which it doesn't understand.
381-415: Feature-complete search integration with excellent UX.The terminal search implementation provides:
- Theme-aware highlight colors for matches
- Real-time search as user types
- Intuitive keyboard navigation (Enter/Shift+Enter, Escape)
- Clean visual integration in header bar
- Proper focus management (input on open, terminal on close)
- Cleanup to prevent visual artifacts
The search addon is fully integrated with the terminal lifecycle and theme system.
Also applies to: 1960-2023
417-441: Dual-layer interception ensures navigation shortcuts always work.The navigation shortcuts are implemented at two levels:
- Container capture phase (lines 419-441) - intercepts before React
- Terminal custom handler (lines 748-776) - intercepts before xterm
This redundancy ensures Ctrl+Alt+Arrow navigation works reliably even if xterm attempts to handle these keys. Both handlers correctly:
- Use
event.codefor keyboard-layout independence- Support both Ctrl (Windows/Linux) and Meta (Mac)
- Prevent default and stop propagation
The comment on line 752 clearly explains this design choice.
Also applies to: 748-776
1749-1897: Comprehensive per-terminal settings popover.The settings UI provides granular control with:
- Font size slider with quick reset button
- Scrollback configuration (1k-100k lines)
- Line height adjustment (1.0-2.0)
- Font family selection
- Run-on-new-terminal command
- Screen reader accessibility toggle
- Clear restart warnings where needed
The UI is compact, informative, and includes helpful keyboard shortcut reminders. The
stopPropagationcalls prevent accidental terminal focus during settings interaction.
1216-1242: Useful scroll-to-bottom feature with smart positioning detection.The jump-to-bottom button implementation:
- Correctly queries xterm's viewport element for scroll position
- Uses 5px tolerance to handle rounding errors
- Employs passive scroll listener for performance
- Shows smooth opacity transition
- Positioned absolutely to avoid interfering with terminal content
This is a common terminal UX pattern that helps users quickly return to current output.
Also applies to: 2036-2051
1926-1943: Clean maximize/minimize toggle integration.The maximize button:
- Conditionally renders only when parent provides the callback
- Shows appropriate icon based on state (Minimize2 vs Maximize2)
- Integrates seamlessly with other header buttons
- Prevents unintended focus changes via stopPropagation
The optional nature of the feature allows terminals to exist both with and without maximize capability.
| // Handle project switching - save and restore terminal layouts | ||
| useEffect(() => { | ||
| const currentPath = currentProject?.path || null; | ||
| const prevPath = prevProjectPathRef.current; | ||
|
|
||
| // Skip if no change | ||
| if (currentPath === prevPath) { | ||
| return; | ||
| } | ||
|
|
||
| // If we're restoring a different project, that restore will be stale - let it finish but ignore results | ||
| // The path check in restoreLayout will handle this | ||
|
|
||
| // Save layout for previous project (if there was one and has terminals) | ||
| // BUT don't save if we were mid-restore for that project (would save incomplete state) | ||
| if (prevPath && terminalState.tabs.length > 0 && restoringProjectPathRef.current !== prevPath) { | ||
| saveTerminalLayout(prevPath); | ||
| } | ||
|
|
||
| // Update the previous project ref | ||
| prevProjectPathRef.current = currentPath; | ||
|
|
||
| // Helper to kill sessions and clear state | ||
| const killAndClear = async () => { | ||
| // Kill all server-side sessions first to prevent orphaned processes | ||
| await killAllSessions(); | ||
| clearTerminalState(); | ||
| }; | ||
|
|
||
| // If no current project, just clear terminals | ||
| if (!currentPath) { | ||
| killAndClear(); | ||
| return; | ||
| } | ||
|
|
||
| // ALWAYS clear existing terminals when switching projects | ||
| // This is critical - prevents old project's terminals from "bleeding" into new project | ||
| // We need to kill server sessions first to prevent orphans | ||
| killAndClear(); |
There was a problem hiding this comment.
Await killAndClear() to prevent race condition.
Line 558 calls killAndClear() without await, then immediately proceeds to restore layout. Since killAndClear is async and kills sessions via network calls, the restoration might start before cleanup completes, potentially causing conflicts or orphaned sessions.
🔎 Recommended fix
// ALWAYS clear existing terminals when switching projects
// This is critical - prevents old project's terminals from "bleeding" into new project
// We need to kill server sessions first to prevent orphans
- killAndClear();
+ await killAndClear();
// Check for saved layout for this project
const savedLayout = getPersistedTerminalLayout(currentPath);📝 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.
| // Handle project switching - save and restore terminal layouts | |
| useEffect(() => { | |
| const currentPath = currentProject?.path || null; | |
| const prevPath = prevProjectPathRef.current; | |
| // Skip if no change | |
| if (currentPath === prevPath) { | |
| return; | |
| } | |
| // If we're restoring a different project, that restore will be stale - let it finish but ignore results | |
| // The path check in restoreLayout will handle this | |
| // Save layout for previous project (if there was one and has terminals) | |
| // BUT don't save if we were mid-restore for that project (would save incomplete state) | |
| if (prevPath && terminalState.tabs.length > 0 && restoringProjectPathRef.current !== prevPath) { | |
| saveTerminalLayout(prevPath); | |
| } | |
| // Update the previous project ref | |
| prevProjectPathRef.current = currentPath; | |
| // Helper to kill sessions and clear state | |
| const killAndClear = async () => { | |
| // Kill all server-side sessions first to prevent orphaned processes | |
| await killAllSessions(); | |
| clearTerminalState(); | |
| }; | |
| // If no current project, just clear terminals | |
| if (!currentPath) { | |
| killAndClear(); | |
| return; | |
| } | |
| // ALWAYS clear existing terminals when switching projects | |
| // This is critical - prevents old project's terminals from "bleeding" into new project | |
| // We need to kill server sessions first to prevent orphans | |
| killAndClear(); | |
| useEffect(() => { | |
| const currentPath = currentProject?.path || null; | |
| const prevPath = prevProjectPathRef.current; | |
| if (currentPath === prevPath) { | |
| return; | |
| } | |
| if (prevPath && terminalState.tabs.length > 0 && restoringProjectPathRef.current !== prevPath) { | |
| saveTerminalLayout(prevPath); | |
| } | |
| prevProjectPathRef.current = currentPath; | |
| const killAndClear = async () => { | |
| await killAllSessions(); | |
| clearTerminalState(); | |
| }; | |
| if (!currentPath) { | |
| killAndClear(); | |
| return; | |
| } | |
| (async () => { | |
| // ALWAYS clear existing terminals when switching projects | |
| // This is critical - prevents old project's terminals from "bleeding" into new project | |
| // We need to kill server sessions first to prevent orphans | |
| await killAndClear(); | |
| // Check for saved layout for this project | |
| const savedLayout = getPersistedTerminalLayout(currentPath); | |
| })(); |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/terminal-view.tsx around lines 520-558, the
async helper killAndClear() is called without awaiting which can let
restoreLayout start before cleanup completes; change both places where
killAndClear() is invoked (the early-return branch for !currentPath and the
ALWAYS-clear branch) to await killAndClear() so cleanup finishes before
returning or proceeding to restore; implement this by wrapping the effect body
in an async inner function (or otherwise using .then) so you can await
killAndClear(), and propagate or handle errors from killAllSessions() to avoid
unhandled rejections.
Resolves merge conflicts: - apps/server/src/routes/terminal/common.ts: Keep randomBytes import, use @automaker/utils for createLogger - apps/ui/eslint.config.mjs: Use main's explicit globals list with XMLHttpRequest and MediaQueryListEvent additions - apps/ui/src/components/views/terminal-view.tsx: Keep our terminal improvements (killAllSessions, beforeunload, better error handling) - apps/ui/src/config/terminal-themes.ts: Keep our search highlight colors for all themes - apps/ui/src/store/app-store.ts: Keep our terminal settings persistence improvements (merge function) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…nal-upgrade feat: enhance terminal functionality and settings
Terminal System Complete Overhaul
Summary
This PR delivers a comprehensive overhaul of the Automaker terminal system, implementing 29 new features and fixing 32 bugs identified through deep analysis. The terminal is now production-ready with professional-grade features matching modern terminal emulators.
New Features (29)
User Experience Features
claude,codex)Search & Navigation
Ctrl+Shift+Fto search terminal output with prev/next navigationvscode://protocolCtrl+Alt+Arrowto move focus between terminal panesSettings & Customization
Server & Reliability
Clipboard & Input
Theming & Accessibility
Bug Fixes (32)
Critical Priority (6)
isRestoringLayoutRefnever reset on failureclearTerminalStateresets user preferencesHigh Priority (7)
Medium Priority (11)
Low Priority (8)
Additional Fixes (This Session)
Keyboard Shortcuts
alt+Talt+Dalt+Salt+Walt+FCtrl+Alt+ArrowCtrl+Plus/MinusCtrl+0EscapeFiles Changed
New Files
apps/ui/src/components/views/terminal-view/terminal-error-boundary.tsxapps/ui/src/components/views/settings-view/terminal/terminal-section.tsxapps/server/src/routes/terminal/routes/settings.tsModified Files (Major)
apps/ui/src/components/views/terminal-view.tsx- Layout, persistence, drag-dropapps/ui/src/components/views/terminal-view/terminal-panel.tsx- Features, shortcuts, WebSocketapps/ui/src/store/app-store.ts- Terminal state, persistence, actionsapps/server/src/services/terminal-service.ts- Session management, SIGKILLapps/server/src/routes/terminal/index.ts- Settings routesapps/ui/src/styles/global.css- Scrollbar themingapps/ui/src/config/terminal-themes.ts- Solarized contrast fixDependencies Added
{ "@xterm/addon-search": "^0.15.0", "@xterm/addon-web-links": "^0.11.0" }Testing Checklist
Breaking Changes
Alt+D/S/W/TtoCtrl+Shift+D/S/W/Tto avoid readline conflictsPerformance Notes
Summary by CodeRabbit
New Features
Improvements
Stability
✏️ Tip: You can customize this high-level summary in your review settings.