Fix performance issues and remove non-persisting terminal#1105
Conversation
📝 WalkthroughWalkthroughConsolidates terminal handling to a daemon-backed model: removes in-process TerminalManager and persistence settings, adds killed-session tombstones and exit reasons ("killed" | "exited" | "error"), updates router and renderer APIs to propagate reason, and simplifies daemon-related public APIs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 293-297: The Promise.allSettled call that runs terminal.kill for
each paneId is silently ignoring failures; change the code around the
Promise.allSettled calls (where beforeIds.map(...) is passed to
Promise.allSettled and the similar block at the other occurrence) to await the
settled results, iterate over them, and for any result.status === "rejected"
call the module's logger (or processLogger) to log an error with context
including the paneId and the rejection reason so failed kills are not swallowed.
Ensure you reference the terminal.kill call and include paneId in the log
message.
In `@apps/desktop/src/main/lib/terminal/index.ts`:
- Around line 33-47: The catch block in tryListExistingDaemonSessions currently
only logs errors when DEBUG_TERMINAL is set, which swallows failures in
production; update the catch to always emit a warning-level log with context and
the error (e.g., using console.warn or the module's logger) and retain the debug
log when DEBUG_TERMINAL is true; reference getTerminalHostClient and
client.listSessions so the message clearly states "Failed to list existing
daemon sessions" and includes the error details.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx (1)
744-750: Extract the session-count threshold into a named constant.
Avoid a hard-coded20in UI logic.As per coding guidelines: Avoid magic numbers by extracting them to named constants at module top.♻️ Suggested refactor
+const SESSION_WARNING_THRESHOLD = 20; ... - {aliveSessions.length >= 20 && ( + {aliveSessions.length >= SESSION_WARNING_THRESHOLD && (apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
6-7: Consider extractingTerminalExitReasonto a shared types location.This type is duplicated identically in
apps/desktop/src/main/lib/terminal/types.ts(line 28). While the duplication is small, it creates a maintenance burden and potential for drift if one definition changes without the other.Consider moving this to a shared types file that both renderer and main process can import, or re-exporting from a common location.
apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts (1)
62-65: Consider adding a test for theMAX_KILLED_SESSION_TOMBSTONESlimit.The tombstone eviction logic (removing oldest when exceeding 1000 entries) is not covered by tests. A test verifying that the oldest tombstone is evicted when the limit is reached would improve confidence in the bounded memory behavior.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
135-137: Prefer a params object for the exit handler signature.With three positional args, consider
{ exitCode, xterm, reason }and update call sites accordingly to reduce positional confusion.As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts (1)
22-26: Prefer a params object forhandleTerminalExit.Now that the handler takes three positional args, consider a single
{ exitCode, xterm, reason }object and update call sites to match.As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
Also applies to: 58-66
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 350-375: The returned killedCount currently uses toKill.length
(attempts) rather than counting successful kills; after awaiting
Promise.allSettled on paneIds.map(paneId => terminal.kill({ paneId })), compute
the number of results with status === "fulfilled" (or filter results by
fulfilled) and return that value as killedCount; keep the existing logging for
rejected results but change the final return to use the computed successfulCount
(referencing terminal.management.listSessions, terminal.kill, paneIds, results,
and killedCount).
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts`:
- Around line 74-83: The session fixture used in daemon-manager.test.ts is
missing the sessionId property and must mirror the real invariant (sessionId ===
paneId); update the object passed to sessions.set (the entry created with paneId
and workspaceId) to include sessionId set to paneId so tests reflect the actual
session shape used by the daemon code (look for sessions.set and paneId in the
test).
In `@apps/desktop/src/main/lib/workspace-runtime/local.ts`:
- Around line 29-35: Update the adapter doc comment to reflect that management
is always provided (remove or change the conditional "only when available
(daemon mode)" wording); edit the comment that describes Adapts
DaemonTerminalManager to the TerminalRuntime interface (the adapter for
DaemonTerminalManager/TerminalRuntime) so bullet 2 states that management is
exposed (always) rather than conditionally, ensuring the three-bullet list
matches current behavior.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/pane-guards.ts (1)
3-6: Use a params object forisPaneDestroyedto meet the 2+ args guideline.This keeps call sites self-documenting and aligns with the project rule.
♻️ Suggested refactor
-export const isPaneDestroyed = ( - panes: Record<string, Pane> | undefined, - paneId: string, -): boolean => !panes?.[paneId]; +export const isPaneDestroyed = ({ + panes, + paneId, +}: { + panes?: Record<string, Pane>; + paneId: string; +}): boolean => !panes?.[paneId];apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalRestore.ts (1)
22-26: Prefer a params object foronExitEvent(now 3 args).This aligns with the 2+ args guideline and makes call sites clearer as the signature grows.
♻️ Suggested refactor
- onExitEvent: ( - exitCode: number, - xterm: XTerm, - reason?: TerminalExitReason, - ) => void; + onExitEvent: (params: { + exitCode: number; + xterm: XTerm; + reason?: TerminalExitReason; + }) => void;- onExitEventRef.current(event.exitCode, xterm, event.reason); + onExitEventRef.current({ + exitCode: event.exitCode, + xterm, + reason: event.reason, + });Also applies to: 100-100
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (1)
571-576: Extract the 50ms detach delay to a named constant.Magic numbers reduce readability and are harder to tune consistently.
♻️ Suggested refactor
+const DETACH_TIMEOUT_MS = 50;- const detachTimeout = setTimeout(() => { + const detachTimeout = setTimeout(() => { detachRef.current({ paneId }); pendingDetaches.delete(paneId); coldRestoreState.delete(paneId); - }, 50); + }, DETACH_TIMEOUT_MS);apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts (1)
93-95: Extract exit code/signal literals into named constants.
This makes intent explicit (e.g., SIGTERM vs. generic numbers) and keeps tests consistent.As per coding guidelines, avoid magic numbers by extracting them to named constants at module top.
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx (1)
739-746: Extract the session warning threshold into a named constant.
This avoids a hard-coded value in the UI logic.♻️ Suggested refactor
+const SESSION_WARNING_THRESHOLD = 20;- {aliveSessions.length >= 20 && ( + {aliveSessions.length >= SESSION_WARNING_THRESHOLD && (As per coding guidelines: Avoid magic numbers by extracting them to named constants at module top.
| const { sessions } = await terminal.management.listSessions(); | ||
| const toKill = sessions.filter( | ||
| (session) => session.workspaceId === input.workspaceId, | ||
| ); | ||
|
|
||
| for (const session of toKill) { | ||
| userKilledSessions.add(session.sessionId); | ||
| await client.kill({ sessionId: session.sessionId }); | ||
| if (toKill.length > 0) { | ||
| const paneIds = toKill.map((session) => session.sessionId); | ||
| const results = await Promise.allSettled( | ||
| paneIds.map((paneId) => terminal.kill({ paneId })), | ||
| ); | ||
| for (const [index, result] of results.entries()) { | ||
| if (result.status === "rejected") { | ||
| const paneId = paneIds[index]; | ||
| logger.error( | ||
| `[killDaemonSessionsForWorkspace] terminal.kill failed for paneId=${paneId}`, | ||
| { | ||
| paneId, | ||
| workspaceId: input.workspaceId, | ||
| reason: result.reason, | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return { daemonModeEnabled: true, killedCount: toKill.length }; | ||
| return { killedCount: toKill.length }; |
There was a problem hiding this comment.
killedCount should reflect successful kills, not attempts.
Right now it returns toKill.length even if some kills fail, which can overstate what actually happened.
🛠️ Proposed fix
const toKill = sessions.filter(
(session) => session.workspaceId === input.workspaceId,
);
- if (toKill.length > 0) {
- const paneIds = toKill.map((session) => session.sessionId);
- const results = await Promise.allSettled(
- paneIds.map((paneId) => terminal.kill({ paneId })),
- );
- for (const [index, result] of results.entries()) {
- if (result.status === "rejected") {
- const paneId = paneIds[index];
- logger.error(
- `[killDaemonSessionsForWorkspace] terminal.kill failed for paneId=${paneId}`,
- {
- paneId,
- workspaceId: input.workspaceId,
- reason: result.reason,
- },
- );
- }
- }
- }
-
- return { killedCount: toKill.length };
+ let killedCount = 0;
+ if (toKill.length > 0) {
+ const paneIds = toKill.map((session) => session.sessionId);
+ const results = await Promise.allSettled(
+ paneIds.map((paneId) => terminal.kill({ paneId })),
+ );
+ for (const [index, result] of results.entries()) {
+ if (result.status === "rejected") {
+ const paneId = paneIds[index];
+ logger.error(
+ `[killDaemonSessionsForWorkspace] terminal.kill failed for paneId=${paneId}`,
+ {
+ paneId,
+ workspaceId: input.workspaceId,
+ reason: result.reason,
+ },
+ );
+ }
+ }
+ killedCount = results.filter(
+ (result) => result.status === "fulfilled",
+ ).length;
+ }
+
+ return { killedCount };🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts` around lines 350 -
375, The returned killedCount currently uses toKill.length (attempts) rather
than counting successful kills; after awaiting Promise.allSettled on
paneIds.map(paneId => terminal.kill({ paneId })), compute the number of results
with status === "fulfilled" (or filter results by fulfilled) and return that
value as killedCount; keep the existing logging for rejected results but change
the final return to use the computed successfulCount (referencing
terminal.management.listSessions, terminal.kill, paneIds, results, and
killedCount).
| sessions.set(paneId, { | ||
| paneId, | ||
| workspaceId: "ws-1", | ||
| isAlive: true, | ||
| lastActive: Date.now(), | ||
| cwd: "", | ||
| pid: 123, | ||
| cols: 80, | ||
| rows: 24, | ||
| }); |
There was a problem hiding this comment.
Add sessionId to the session fixture to preserve the daemon invariant.
Without it, the test diverges from real session shape and can mask issues that read sessionId directly.
✅ Suggested fix
sessions.set(paneId, {
paneId,
+ sessionId: paneId,
workspaceId: "ws-1",
isAlive: true,
lastActive: Date.now(),
cwd: "",
pid: 123,
cols: 80,
rows: 24,
});Based on learnings: In the terminal daemon code under apps/desktop/src/main/lib/terminal/daemon, sessionId is intentionally set equal to paneId, so tests should reflect that invariant.
📝 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.
| sessions.set(paneId, { | |
| paneId, | |
| workspaceId: "ws-1", | |
| isAlive: true, | |
| lastActive: Date.now(), | |
| cwd: "", | |
| pid: 123, | |
| cols: 80, | |
| rows: 24, | |
| }); | |
| sessions.set(paneId, { | |
| paneId, | |
| sessionId: paneId, | |
| workspaceId: "ws-1", | |
| isAlive: true, | |
| lastActive: Date.now(), | |
| cwd: "", | |
| pid: 123, | |
| cols: 80, | |
| rows: 24, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/terminal/daemon/daemon-manager.test.ts` around
lines 74 - 83, The session fixture used in daemon-manager.test.ts is missing the
sessionId property and must mirror the real invariant (sessionId === paneId);
update the object passed to sessions.set (the entry created with paneId and
workspaceId) to include sessionId set to paneId so tests reflect the actual
session shape used by the daemon code (look for sessions.set and paneId in the
test).
| * Adapts DaemonTerminalManager to the TerminalRuntime interface. | ||
| * | ||
| * This adapter: | ||
| * 1. Wraps the underlying manager with the common interface | ||
| * 2. Exposes management capabilities only when available (daemon mode) | ||
| * 3. Provides capability flags for UI feature detection | ||
| */ |
There was a problem hiding this comment.
Update adapter doc: management is no longer conditional.
The comment says management is exposed “only when available (daemon mode)”, but this adapter now always provides management. Please update the bullet to match current behavior.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/workspace-runtime/local.ts` around lines 29 - 35,
Update the adapter doc comment to reflect that management is always provided
(remove or change the conditional "only when available (daemon mode)" wording);
edit the comment that describes Adapts DaemonTerminalManager to the
TerminalRuntime interface (the adapter for
DaemonTerminalManager/TerminalRuntime) so bullet 2 states that management is
exposed (always) rather than conditionally, ensuring the three-bullet list
matches current behavior.
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.