Multi-terminal feature #1302
Multi-terminal feature #1302
Conversation
Refactors terminal data routing to use a tracked remote terminal for consistency, adds a cleanup method to TerminalInstance for resource management, and enhances TerminalManager with unique terminal naming and remote terminal tracking. Also updates TerminalTabBar for better accessibility and code organization, and removes the unused 'Go to Parent Folder' action from ExplorerFileTreeSection. Add multi-terminal support with TerminalManager Introduces TerminalManager to manage multiple terminal instances, refactors codebase to use terminalManager instead of a single terminalInstance, and adds a TerminalTabBar UI for switching between terminals. Updates related views, containers, and extensions to support multi-terminal workflows, including keyboard toolbar and local execution. Maintains backward compatibility for terminalInstance references and ensures UI updates on terminal changes. Improve terminal management and UX for busy terminals Adds confirmation dialog when killing a terminal with a running process, improves cleanup of terminal resources, and optimizes rendering to only show the active terminal. Also refactors terminal options handling and enhances data routing for remote terminals. Improve terminal management and add logging Refactored terminal options loading to use a helper for safer initialization. Added main thread assertions in TerminalManager for thread safety in debug builds. Enhanced terminal naming with localization support and improved duplicate handling. Introduced os.log-based logging for dropped remote terminal data and general terminal management events. Improve terminal naming and UI interactions TerminalManager now generates unique terminal names by reusing gaps from closed terminals, ensuring the lowest available number is used. Terminal actions in ToolbarView now target the active terminal, and MultiTerminalView adds a smooth animation for the tab bar when multiple terminals are present. Add session identifier and service provider support Executor now accepts a customizable sessionIdentifier, allowing unique identification for each terminal instance. TerminalManager tracks and propagates a TerminalServiceProvider to all TerminalInstance objects, improving remote connection handling and service management. Improve terminal management logging and error handling Adds detailed logging to terminal creation, closing, and switching in TerminalManager for better traceability. Enhances error handling in LocalExecutionExtension by providing user notifications for missing or busy executors. Refactors TerminalExtension to avoid repeated lookups of the active terminal when executing scripts. Improve terminal initialization and accessibility handling Adds a displayName property to Executor.State for better user messages, ensures terminal fitAddon is called only when ready, and posts a notification when a terminal is initialized. Refines accessibility labels for terminal tabs and improves active terminal management logic. Also prevents redundant open editor configuration in MainApp. Refactor terminal management and rendering logic Updated TerminalManager to set the terminal service provider only on the active terminal and improved remote terminal tracking. Refactored MultiTerminalView to render all terminals in a ZStack, showing only the active one, to support better view transitions and state management. Improve terminal management and accessibility handling Refactored terminal cleanup and service provider logic for better resource management and reliability. Enhanced terminal naming to avoid duplicates and added logging for failed active terminal assignments. Updated accessibility label construction in TerminalTabBar for clarity. Fixed command evaluation in LocalExecutionExtension to use the correct executor.
Refactored several long lines in MainApp.swift, TerminalManager.swift, and TerminalTabBar.swift to improve readability and maintain consistent code style. No functional changes were made.
|
Very cool! I actually have been thinking of working on this for sometime. Does your implementation support SSH? |
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive multi-terminal feature by replacing the single TerminalInstance approach with a new TerminalManager that handles multiple terminal instances simultaneously. The changes modernize terminal handling, improve code organization, and lay the foundation for better remote terminal support.
Changes:
- Introduced
TerminalManagerto orchestrate multiple terminal instances with lifecycle management, remote terminal tracking, and coordinated state updates - Added
TerminalTabBarUI component for managing and switching between terminals with accessibility support and busy-state indicators - Refactored all terminal access throughout the codebase to use the new manager pattern while maintaining backward compatibility
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| CodeApp/Managers/TerminalManager.swift | New manager class coordinating multiple terminal instances with lifecycle, options, themes, and remote connection handling |
| CodeApp/Managers/TerminalInstance.swift | Added Identifiable conformance, cleanup method, and isReady state tracking for proper multi-terminal support |
| CodeApp/Managers/MainApp.swift | Refactored to use TerminalManager with forwarded state changes, improved remote data routing, and backward compatibility property |
| CodeApp/Managers/Executor.swift | Added configurable session identifier and localized displayName for improved multi-terminal executor support |
| CodeApp/Views/TerminalTabBar.swift | New vertical tab bar component with terminal selection, closing, and busy-state confirmations |
| CodeApp/Views/TerminalKeyboardToolbar.swift | Updated to support optional terminal ID parameter for targeting specific terminals |
| Extensions/TerminalService/TerminalExtension.swift | Replaced single TerminalView with MultiTerminalView that renders all terminals in a ZStack with conditional tab bar |
| Extensions/LocalExecution/LocalExecutionExtension.swift | Enhanced error handling with terminal-specific messaging and executor state validation |
| CodeApp/Containers/RemoteContainer.swift | Updated to set terminal service provider through TerminalManager |
| CodeApp/Containers/MainScene.swift | Updated theme and options application to use manager's apply-to-all methods |
| CodeApp/Views/ActivityBar.swift | Updated focus removal to use active terminal from manager |
| Code.xcodeproj/project.pbxproj | Added TerminalManager.swift and TerminalTabBar.swift to build targets |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .onReceive( | ||
| NotificationCenter.default.publisher( | ||
| for: .terminalAltReset, | ||
| object: App.terminalInstance | ||
| object: terminal | ||
| ), |
There was a problem hiding this comment.
Similar to the previous issue, this notification observer is filtering by object (terminal), but the terminal computed property may return different instances over time. If the terminal instance changes, this observer will still be listening to the old terminal object. Consider using the terminal ID in the notification's userInfo for more robust filtering.
| webView.navigationDelegate = nil | ||
| webView.uiDelegate = nil | ||
| webView.removeFromSuperview() | ||
| webView.configuration.userContentController.removeAllScriptMessageHandlers() |
There was a problem hiding this comment.
The cleanup method calls removeAllScriptMessageHandlers(), but the initialization only adds a script message handler if !terminalMessageHandlerAdded is true. This flag is an instance variable that starts as false, so the first terminal instance will add a handler. However, removeAllScriptMessageHandlers() removes ALL handlers from the shared UserContentController, which could affect other terminal instances or components sharing the same configuration. This could lead to message handlers being unexpectedly removed from active terminals. Consider using removeScriptMessageHandler(forName:) to remove only the specific handler for this terminal instance.
| webView.configuration.userContentController.removeAllScriptMessageHandlers() |
| app.notificationManager.showWarningMessage( | ||
| "Cannot run: terminal '\(activeTerminal.name)' executor is \(executor.state.displayName) (expected idle)." | ||
| ) |
There was a problem hiding this comment.
The error message uses interpolation of executor.state.displayName to show the current state, but this might not be localized correctly in all contexts. The outer string is localized via NSLocalizedString, but the displayName itself returns localized strings. This means the resulting message structure might not make grammatical sense in all languages since it embeds a localized word into another localized sentence structure. Consider using a parameterized localized string format that can be properly translated for different languages.
| app.notificationManager.showWarningMessage( | |
| "Cannot run: terminal '\(activeTerminal.name)' executor is \(executor.state.displayName) (expected idle)." | |
| ) | |
| let message = String( | |
| format: NSLocalizedString( | |
| "LOCAL_EXECUTION_EXECUTOR_NOT_IDLE_FORMAT", | |
| comment: "Shown when trying to run code but the terminal executor is not idle. Parameters: terminal name, executor state display name." | |
| ), | |
| activeTerminal.name, | |
| executor.state.displayName | |
| ) | |
| app.notificationManager.showWarningMessage(message) |
| var monacoInstance: EditorImplementation! = nil | ||
|
|
||
| // Backward compatibility: returns the active terminal | ||
| var terminalInstance: TerminalInstance! { |
There was a problem hiding this comment.
The backward compatibility property terminalInstance returns the active terminal, which could be nil (since activeTerminal is optional), but the property is declared as an implicitly unwrapped optional TerminalInstance!. This could lead to crashes if code expects a non-nil value but no active terminal exists. While the TerminalManager ensures at least one terminal exists after initialization, there might be edge cases during cleanup or state transitions where this assumption breaks. Consider either making this a regular optional TerminalInstance? to force callers to handle the nil case explicitly, or add documentation clarifying the invariant that this will never be nil after initialization.
| var terminalInstance: TerminalInstance! { | |
| var terminalInstance: TerminalInstance? { |
|
|
||
| private func createTerminalInstance(name: String) -> TerminalInstance { | ||
| let terminal = TerminalInstance(root: rootURL, options: options, name: name) | ||
| if let provider = terminalServiceProvider, terminal.id == remoteTerminalId { |
There was a problem hiding this comment.
This logic appears incorrect. When creating a new terminal instance, it checks if terminal.id == remoteTerminalId, but at this point the terminal is newly created and its ID won't match the existing remoteTerminalId. The condition should likely check if this is being called in a context where the terminal should have the remote provider, or the logic needs to be restructured. This could prevent remote terminal service providers from being properly assigned to new terminals.
| if let provider = terminalServiceProvider, terminal.id == remoteTerminalId { | |
| if let provider = terminalServiceProvider { |
|
|
||
| private func configureOpenEditorForTerminals() { | ||
| for terminal in terminalManager.terminals where terminal.openEditor == nil { | ||
| terminal.openEditor = { [weak self] url in | ||
| if url.isDirectory { | ||
| DispatchQueue.main.async { | ||
| self?.loadFolder(url: url) | ||
| } | ||
| } else { | ||
| self?.openFile(url: url) |
There was a problem hiding this comment.
The openEditor closure in the configureOpenEditorForTerminals method captures self weakly, which is good for preventing retain cycles. However, consider that the closure is assigned to terminal.openEditor and may outlive the MainApp instance in edge cases. While the weak reference should prevent crashes, verify that the terminal cleanup properly nils out the openEditor closure to avoid unnecessary references.
| func applyOptionsToAll(_ options: TerminalOptions) { | ||
| assertMainThread() | ||
| self.options = options | ||
| for terminal in terminals { |
There was a problem hiding this comment.
The method name setTerminalServiceProviderForAll is misleading. According to the implementation and the comment on line 245, this method only sets the terminal service provider on the active terminal, not on all terminals. The method explicitly sets the provider to nil for all non-active terminals. Consider renaming to setTerminalServiceProviderForActiveTerminal to accurately reflect its behavior.
| .onReceive( | ||
| NotificationCenter.default.publisher( | ||
| for: .terminalControlReset, | ||
| object: App.terminalInstance | ||
| object: terminal | ||
| ), |
There was a problem hiding this comment.
The notification observer is filtering by object (terminal), but at the time of subscription, the terminal variable is a computed property that may return different instances over time. If the terminal instance changes (e.g., after closing and recreating), this observer will still be listening to the old terminal object. Consider using the terminal ID in the notification's userInfo for more robust filtering, or manage the lifecycle of this observer more carefully.
| onSelect() | ||
| } | ||
| .accessibilityElement(children: .ignore) | ||
| .accessibilityLabel(accessibilityLabel) |
There was a problem hiding this comment.
The accessibility hint for terminal tabs should be "Tap to switch to this terminal" rather than "Double tap to switch to this terminal" since the action is performed with a single tap (onTapGesture), not a double tap. This mismatch could confuse VoiceOver users.
| var remoteTerminal: TerminalInstance? { | ||
| if let id = remoteTerminalId { | ||
| return terminals.first { $0.id == id } | ||
| } | ||
| return terminals.first { $0.terminalServiceProvider != nil } |
There was a problem hiding this comment.
The remoteTerminal computed property has logic that could return unexpected results. When remoteTerminalId is nil, it falls back to returning the first terminal with a non-nil terminalServiceProvider. However, if multiple terminals somehow have service providers (which shouldn't happen based on setTerminalServiceProviderForAll, but could occur in edge cases), this will return an arbitrary terminal. Consider adding documentation or assertions to clarify the expected invariant that only one terminal should have a service provider at any time.
| var remoteTerminal: TerminalInstance? { | |
| if let id = remoteTerminalId { | |
| return terminals.first { $0.id == id } | |
| } | |
| return terminals.first { $0.terminalServiceProvider != nil } | |
| /// | |
| /// - Important: | |
| /// At most one `TerminalInstance` should have a non-nil `terminalServiceProvider` | |
| /// at any time. This invariant is expected to be enforced by the code that | |
| /// configures terminal service providers (e.g. `setTerminalServiceProviderForAll`). | |
| /// If multiple terminals somehow have a provider, a debug assertion will fire | |
| /// and a log message will be emitted; the first such terminal will be returned. | |
| var remoteTerminal: TerminalInstance? { | |
| // Prefer an explicitly designated remote terminal when available. | |
| if let id = remoteTerminalId { | |
| return terminals.first { $0.id == id } | |
| } | |
| // Fallback: infer the remote terminal from the presence of a service provider. | |
| let terminalsWithProvider = terminals.filter { $0.terminalServiceProvider != nil } | |
| assert( | |
| terminalsWithProvider.count <= 1, | |
| "TerminalManager.remoteTerminal invariant violated: multiple terminals have a terminalServiceProvider set simultaneously." | |
| ) | |
| if terminalsWithProvider.count > 1 { | |
| let ids = terminalsWithProvider.map { $0.id.uuidString }.joined(separator: ", ") | |
| logger.error( | |
| "Multiple terminals have terminalServiceProvider set; falling back to first. IDs: \(ids, privacy: .public)" | |
| ) | |
| } | |
| return terminalsWithProvider.first |
Yes, it does. By default, the "SSH-connected terminal" is the one that was opened at the moment the user connected to SSH. We could change this behavior though. Maybe make all new terminals SSH-connected? It's up to you. This is a big one. I tested it a lot, but I would appreciate if you could test it a little bit with your own workflow. The next pull request I intend to make is a new "go to Parent Folder" option in File Explorer context menu while connected to SSH. |
2 participants
This pull request introduces a new
TerminalManagerfor managing multiple terminal instances, refactors the codebase to use this manager throughout the app, and adds supporting infrastructure for multi-terminal workflows. The changes modernize terminal handling, improve code organization, and prepare the app for features like multiple terminals and better remote terminal support.Terminal Management Refactor:
Introduced the
TerminalManagerclass to handle multiple terminal instances, replacing the previous single-instance approach. The manager tracks active, remote, and all terminals, and provides methods for applying options, themes, and service providers across terminals. (TerminalManager.swiftand related project file changes) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Refactored
MainAppto useTerminalManagerinstead of a singleTerminalInstance, including updating all references, setting up terminal callbacks, and forwarding state changes for UI updates. (MainApp.swift) [1] [2] [3] [4] [5] [6]Terminal Instance Enhancements:
TerminalInstanceto conform toIdentifiable, addedidandnameproperties, and improved lifecycle handling for terminal service providers. (TerminalInstance.swift) [1] [2]App-wide Integration Updates:
App.terminalInstanceto use the new manager, including theme application, option changes, and focus management in UI containers. (MainScene.swift,RemoteContainer.swift) [1] [2] [3] [4]Executor Improvements:
Executorclass to accept a session identifier for better multi-terminal support and added a user-facingdisplayNameproperty for executor states. (Executor.swift) [1] [2] [3]Infrastructure and Logging:
os.loglogger toMainAppfor improved diagnostics and error reporting, especially for remote terminal data handling. (MainApp.swift) [1] [2]These changes lay the groundwork for robust multi-terminal support and improve the maintainability and scalability of the terminal subsystem.