Repository navigation
refactor(desktop): split daemon-manager into modular files - #852
Merged
Merged
Conversation
Split the monolithic daemon-manager.ts (~1325 lines) into focused modules for better maintainability: - daemon/types.ts - SessionInfo, ColdRestoreInfo interfaces - daemon/constants.ts - Configuration constants - daemon/priority-semaphore.ts - Concurrency limiter utility - daemon/history-manager.ts - History persistence logic - daemon/daemon-manager.ts - Core session lifecycle (~740 lines) - daemon/index.ts - Exports and singleton management All imports remain backward compatible via the barrel export.
Contributor
📝 WalkthroughWalkthroughMonolithic daemon-based terminal manager file removed and functionality restructured into a new daemon/ subpackage introducing DaemonTerminalManager, HistoryManager, PrioritySemaphore, config constants, types, and a singleton accessor; root terminal import updated to use the new module. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant Manager as DaemonTerminalManager
participant Daemon as Terminal Host (daemon)
participant History as HistoryManager
participant Ports as PortManager
UI->>Manager: createOrAttach(paneId, params)
Manager->>Manager: acquire priority semaphore
alt cold restore available
Manager->>History: attemptColdRestore(paneId)
History-->>Manager: cold restore snapshot
end
Manager->>Daemon: createOrAttach (env, cwd, cols, rows)
Daemon-->>Manager: session metadata (pid, id) + stream events
Manager->>Ports: register pid for port detection
Manager->>History: initHistoryWriter(paneId, metadata, initialScrollback)
History-->>Manager: writer ready
Manager-->>UI: SessionResult (snapshot + metadata)
Daemon-->>Manager: data / exit / error events
Manager->>History: writeToHistory(paneId, data)
Manager-->>UI: emit data/exit events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts`:
- Around line 568-576: The console.warn in the signal method is missing the
required "[domain/operation]" prefix; update the warning emitted when session is
missing or not alive in the signal(params: { paneId: string; signal?: string }):
void function to include the mandated prefix (e.g. "[terminal/daemon]") before
the existing message, referencing the session variable and paneId so the log
reads like: prefix + contextual message about session not found or not alive;
ensure you update the console.warn call in daemon-manager.ts where
sessions.get(paneId) is checked.
- Around line 504-517: The catch block in getCreateOrAttachPriority silently
swallows errors; change it to catch the error as a variable (e.g., err) and log
it with context (include params.workspaceId, params.tabId, params.paneId and any
relevant appState pieces) using the project's logger (e.g., processLogger.error
or existing logger) and then return the default priority (1); keep the rest of
getCreateOrAttachPriority unchanged.
- Around line 360-373: When creating/replacing a session for a paneId you must
clear any pending cleanup timeout so the old timeout doesn't delete the
newly-created session; before calling this.sessions.set(...) (inside the code
handling new session creation where paneId is available) cancel and clear the
existing timeout for that paneId (e.g. clearTimeout on a per-pane timer stored
on the instance, such as this.sessionCleanupTimers or this.cleanupTimeouts) and
remove its entry, then proceed to set the new session object; ensure that
timeouts scheduled with SESSION_CLEANUP_DELAY_MS are stored by paneId so they
can be canceled when reusing the same paneId.
- Around line 690-695: The loop that collects IDs to terminate uses
session.paneId but the daemon kill path expects session.sessionId; update the
collection to push session.sessionId instead and rename the container variable
from paneIdsToKill to sessionIdsToKill (or update all downstream references
where paneIdsToKill is used) to keep names consistent; locate this change around
the listSessions() usage in daemon-manager (and ensure the later kill() calls
still pass the sessionIds from session.sessionId).
In `@apps/desktop/src/main/lib/terminal/daemon/index.ts`:
- Around line 15-20: Make disposeDaemonManager async and, instead of calling
daemonManager.removeAllListeners(), await daemonManager.cleanup() so
DaemonTerminalManager.cleanup() is run to close HistoryWriter file handles,
clear cleanupTimeouts, and dispose the TerminalHostClient; check daemonManager
exists before awaiting, await daemonManager.cleanup(), then set daemonManager =
null (no need to only call removeAllListeners()), and surface or log any errors
from the awaited cleanup call.
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/terminal/daemon/priority-semaphore.ts (1)
13-23: Consider documenting priority ordering convention.The implementation is correct. Lower priority values get processed first (ascending sort). Consider adding a brief JSDoc comment to clarify this convention for future maintainers:
+ /** + * Acquire a slot in the semaphore. Lower priority values are processed first. + * Returns a release function that must be called when done. + */ acquire(priority: number): Promise<() => void> {Also, sorting on every
acquireis O(n log n). For the expected small queue sizes in terminal concurrency limiting, this is fine, but if the queue grows large, a heap or sorted insertion would be more efficient.apps/desktop/src/main/lib/terminal/index.ts (1)
26-26: Duplicate constant definition—consider importing from constants module.
DEBUG_TERMINALis defined identically indaemon/constants.ts. Consider importing it to maintain DRY:-const DEBUG_TERMINAL = process.env.SUPERSET_TERMINAL_DEBUG === "1"; +import { DEBUG_TERMINAL } from "./daemon/constants";Or alternatively, export it from the daemon barrel if it's needed in both places.
apps/desktop/src/main/lib/terminal/daemon/history-manager.ts (1)
224-245: Consider consistency betweenforceCloseAllandcleanup.
forceCloseAlluses sequentialawait(Lines 225-231) whilecleanupusesPromise.all(Lines 207-218). If the sequential behavior is intentional (e.g., for ordered shutdown), consider adding a comment. Otherwise, usingPromise.allwould be more efficient.Also,
closeAllSyncsilently swallows errors (Line 240:.catch(() => {})). Per coding guidelines about not swallowing errors silently, consider at minimum logging them, though this may be intentional for synchronous shutdown contexts where logging is not guaranteed to complete.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts`:
- Around line 819-840: In forceKillAll, don’t swallow errors from
this.client.listSessions; change the catch to capture the error and log it
(e.g., this.logger.error or an existing module logger) with a clear prefix like
"Failed to list sessions in forceKillAll" before returning the fallback {
sessions: [] }. Update the call site where this.client.listSessions is used so
the catch handler receives the error variable and logs it, preserving the
current fallback behavior; reference forceKillAll and this.client.listSessions
when making the change.
♻️ Duplicate comments (2)
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts (2)
506-519: Log priority derivation failures instead of swallowing.The empty catch hides failures; log with a prefixed message and context before returning the default. As per coding guidelines, never swallow errors silently.
🐛 Proposed fix
- } catch { - return 1; - } + } catch (error) { + console.warn( + "[DaemonTerminalManager/getCreateOrAttachPriority] Failed to compute priority:", + { + workspaceId: params.workspaceId, + tabId: params.tabId, + paneId: params.paneId, + }, + error, + ); + return 1; + }
570-587: Add required log prefix for missing-session warning.The warning should include the mandated
[domain/operation]prefix. As per coding guidelines, prefix console logs.🪵 Proposed fix
- console.warn( - `Cannot signal terminal ${paneId}: session not found or not alive`, - ); + console.warn( + `[DaemonTerminalManager] Cannot signal terminal ${paneId}: session not found or not alive`, + );
Comment on lines
+819
to
+840
| async forceKillAll(): Promise<void> { | ||
| const response = await this.client.listSessions().catch(() => ({ | ||
| sessions: [], | ||
| })); | ||
| const sessionIds = response.sessions.map((s) => s.sessionId); | ||
|
|
||
| for (const timeout of this.cleanupTimeouts.values()) { | ||
| clearTimeout(timeout); | ||
| } | ||
| this.cleanupTimeouts.clear(); | ||
|
|
||
| await this.historyManager.forceCloseAll(); | ||
|
|
||
| await this.client.killAll({}); | ||
| for (const paneId of sessionIds) { | ||
| portManager.unregisterDaemonSession(paneId); | ||
| } | ||
| this.daemonAliveSessionIds.clear(); | ||
| this.daemonSessionIdsHydrated = true; | ||
| this.coldRestoreInfo.clear(); | ||
| this.sessions.clear(); | ||
| } |
Contributor
There was a problem hiding this comment.
Log listSessions failure in forceKillAll.
The catch currently swallows errors; log with prefix before falling back. As per coding guidelines, never swallow errors silently.
🐛 Proposed fix
- const response = await this.client.listSessions().catch(() => ({
- sessions: [],
- }));
+ const response = await this.client.listSessions().catch((error) => {
+ console.warn(
+ "[DaemonTerminalManager/forceKillAll] Failed to list sessions:",
+ error,
+ );
+ return { sessions: [] };
+ });🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts` around lines 819
- 840, In forceKillAll, don’t swallow errors from this.client.listSessions;
change the catch to capture the error and log it (e.g., this.logger.error or an
existing module logger) with a clear prefix like "Failed to list sessions in
forceKillAll" before returning the fallback { sessions: [] }. Update the call
site where this.client.listSessions is used so the catch handler receives the
error variable and logs it, preserving the current fallback behavior; reference
forceKillAll and this.client.listSessions when making the change.
This was referenced Jan 22, 2026
This was referenced Feb 1, 2026
This was referenced Feb 11, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
daemon-manager.ts(~1325 lines) into 6 focused modules indaemon/folderHistoryManagerclass to encapsulate history persistence logicPrioritySemaphoreutility for concurrency limitingDaemonTerminalManagerclass reduced to ~740 linesStructure
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.