feat(desktop): add restart prompt for terminal persistence toggle and show sessions regardless of mode #809
Conversation
📝 WalkthroughWalkthroughAdds a TRPC Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Settings UI
participant TRPC as TRPC Server
participant Main as Desktop Main
participant TerminalHost as Terminal Host Daemon
participant App as Electron App
User->>UI: Confirm persistence change
UI->>TRPC: confirmPersistenceChange mutation
TRPC->>Main: update persistence setting
alt production
UI->>TRPC: restartApp mutation
TRPC->>Main: restart request
Main->>App: app.relaunch()
Main->>App: quitWithoutConfirmation()
App->>App: Restart
else development
TRPC-->>UI: return success (show toast)
end
Note over UI,TerminalHost: Terminal session operations
UI->>TRPC: listDaemonSessions / killSession requests
TRPC->>TerminalHost: getTerminalHostClient() -> tryConnectAndAuthenticate()
alt connected
TerminalHost-->>TRPC: listSessions / kill success
TRPC-->>UI: return sessions / success
else disconnected
TerminalHost-->>TRPC: respond daemonRunning=false / empty sessions
TRPC-->>UI: return disabled/empty result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Kitenite
force-pushed
the
switch-restart-app
branch
from
45c53ae to
8974fcc
Compare
… show sessions regardless of mode - Add confirmation dialog when toggling terminal persistence that restarts app (production) or shows toast (dev mode) - Show background sessions in tray and settings regardless of persistence setting - Remove automatic daemon shutdown on startup to allow managing existing sessions - Remove daemon mode caching to read fresh from DB
Kitenite
force-pushed
the
switch-restart-app
branch
from
aec54ac to
6907634
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
301-307: Track killed sessions bypaneIdto prevent unintended reattach.
userKilledSessionsis checked againstpaneIdincreateOrAttach, but these paths addsessionId. If they diverge, killed sessions can resurrect. Track bypaneIdwhile still killing bysessionId.🐛 Suggested fix
- const beforeIds = before.sessions.map((s) => s.sessionId); - for (const id of beforeIds) { - userKilledSessions.add(id); - } + const beforeIds = before.sessions.map((s) => s.sessionId); + for (const session of before.sessions) { + userKilledSessions.add(session.paneId); + } ... - for (const session of toKill) { - userKilledSessions.add(session.sessionId); - await client.kill({ sessionId: session.sessionId }); - } + for (const session of toKill) { + userKilledSessions.add(session.paneId); + await client.kill({ sessionId: session.sessionId }); + }Also applies to: 362-369
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/terminal/index.ts`:
- Around line 136-156: In tryListExistingDaemonSessions, don't swallow errors
when DEBUG_TERMINAL is false: inside the catch block (around
getTerminalHostClient/tryConnectAndAuthenticate/listSessions calls) always emit
a warning-level log that includes the error details and context (e.g.,
"[TerminalManager] Failed to list existing daemon sessions") using the existing
logger or console.warn if no logger is available, while still returning {
daemonRunning: false, sessions: [] }; ensure DEBUG_TERMINAL controls the more
verbose debug console.log but a warning is always emitted for observability.
In `@apps/desktop/src/main/lib/tray/index.ts`:
- Around line 130-136: The tray killSession function is passing the pane
identifier incorrectly to the host client's kill method; update killSession (and
any callers) so client.kill is invoked with an object containing sessionId
(e.g., client.kill({ sessionId: paneId })) rather than passing paneId directly;
locate the killSession function and the getTerminalHostClient / client.kill
usage to make this change so the host receives the correct sessionId field.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/tray/index.ts (1)
166-169: Prefer params object for multi-arg helpers.
This helper now has 2 positional args; switching to a params object aligns with repo conventions. As per coding guidelines, use an object parameter for 2+ arguments.♻️ Suggested refactor
-function buildSessionsSubmenu( - sessions: ListSessionsResponse["sessions"], - daemonRunning: boolean, -): MenuItemConstructorOptions[] { +function buildSessionsSubmenu({ + sessions, + daemonRunning, +}: { + sessions: ListSessionsResponse["sessions"]; + daemonRunning: boolean; +}): MenuItemConstructorOptions[] {- const sessionsSubmenu = buildSessionsSubmenu(sessions, daemonRunning); + const sessionsSubmenu = buildSessionsSubmenu({ sessions, daemonRunning });Also applies to: 249-250
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx (1)
56-111: Handle restart failures for user feedback.
restartAppmutation errors are currently silent, which can leave users stuck if the relaunch fails. Add anonErrortoast (or similar) so failures are visible. As per coding guidelines, avoid silent failures.🔧 Suggested fix
-const restartApp = electronTrpc.settings.restartApp.useMutation(); +const restartApp = electronTrpc.settings.restartApp.useMutation({ + onError: (error) => { + toast.error("Failed to restart app", { + description: error.message, + }); + }, +});
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/tray/index.ts (1)
138-151: Usesession.sessionIdinstead ofsession.paneIdwhen killing sessions.The
killSessionfunction receivespaneIdas a parameter but passes it toclient.kill()which expectssessionId. SinceListSessionsResponsecontains distinctsessionIdandpaneIdfields (seeapps/desktop/src/main/lib/terminal-host/types.tslines 233-235), this mismatch causes kill operations to target the wrong session.Change line 138 to accept
sessionId: stringas the parameter, and line 213 to passsession.sessionIdinstead ofsession.paneId. This matches the correct usage pattern inapps/desktop/src/lib/trpc/routers/terminal/terminal.tsline 369.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/tray/index.ts (1)
181-231: "Terminal Settings..." is only visible when there are active sessions.The menu item at lines 223-226 is inside the
elseblock, so it only appears when sessions exist. Users with no active sessions cannot access Terminal Settings from this submenu, while "Restart Daemon" is always visible (line 233).Consider moving "Terminal Settings..." outside the conditional block for consistent access:
♻️ Suggested fix
if (aliveSessions.length === 0) { menuItems.push({ label: "No active sessions", enabled: false }); - } else { + } + + if (aliveSessions.length > 0) { const byWorkspace = new Map<string, ListSessionsResponse["sessions"]>(); // ... workspace grouping logic ... menuItems.push({ type: "separator" }); - menuItems.push({ - label: "Terminal Settings...", - click: openTerminalSettings, - }); menuItems.push({ label: "Kill All Sessions", click: killAllSessions, }); } + menuItems.push({ + label: "Terminal Settings...", + click: openTerminalSettings, + }); menuItems.push({ label: "Restart Daemon", enabled: daemonRunning, click: restartDaemon, });
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/tray/index.ts (1)
133-146: Fix the sessionId mismatch in the killSession function—passsession.sessionIdinstead ofsession.paneId.The Terminal Host daemon stores sessions by
sessionIdand thekill()method usessessionIdto look up the session. However, the function currently passespaneIdas thesessionIdparameter toclient.kill(), which will fail to find and kill the session. At line 208, change the call tokillSession(session.sessionId).
🧹 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: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/tray/index.ts`:
- Around line 115-121: In killAllSessions (and similarly in killSession) add a
diagnostic log when tryConnectAndAuthenticate() returns false so the function
doesn't silently no-op: after calling const client = getTerminalHostClient() and
awaiting client.tryConnectAndAuthenticate(), if connected is false call
processLogger.warn or console.warn with a clear message (e.g., "[Tray] Terminal
host unavailable, cannot kill all sessions" and include any identifying context)
and then return; keep the existing success log path unchanged and reuse the same
pattern in killSession to mirror behavior.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/tray/index.ts (1)
130-136: UsesessionIdinstead ofpaneIdwhen killing sessions.
client.killexpectssessionId, but the tray path still passespaneId, which can break kills when IDs differ.🐛 Proposed fix
-async function killSession(paneId: string): Promise<void> { +async function killSession(sessionId: string): Promise<void> { try { const client = getTerminalHostClient(); const connected = await client.tryConnectAndAuthenticate(); if (connected) { - await client.kill({ sessionId: paneId }); - console.log(`[Tray] Killed session: ${paneId}`); + await client.kill({ sessionId }); + console.log(`[Tray] Killed session: ${sessionId}`); } } catch (error) { - console.error(`[Tray] Failed to kill session ${paneId}:`, error); + console.error(`[Tray] Failed to kill session ${sessionId}:`, error); }- click: () => killSession(session.paneId), + click: () => killSession(session.sessionId),Also applies to: 205-206
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.stream.test.ts (1)
91-99: LGTM!The mock provides minimal stubs that isolate the tests from Electron dependencies. The default return values (
falsefor auth, empty sessions, empty objects for kill operations) are sensible defaults for test isolation.Consider adding explicit return types to the mock methods if the actual
TerminalHostClientinterface evolves, to catch mismatches at compile time. This is optional given this is test scaffolding.apps/desktop/src/main/lib/tray/index.ts (1)
166-169: Use an object param forbuildSessionsSubmenuto avoid boolean blindness.
Two positional params (especially a boolean) are discouraged by the repo guidelines. As per coding guidelines, please switch to an object parameter.♻️ Suggested refactor
-function buildSessionsSubmenu( - sessions: ListSessionsResponse["sessions"], - daemonRunning: boolean, -): MenuItemConstructorOptions[] { +function buildSessionsSubmenu({ + sessions, + daemonRunning, +}: { + sessions: ListSessionsResponse["sessions"]; + daemonRunning: boolean; +}): MenuItemConstructorOptions[] {- const sessionsSubmenu = buildSessionsSubmenu(sessions, daemonRunning); + const sessionsSubmenu = buildSessionsSubmenu({ sessions, daemonRunning });Also applies to: 254-255
| async function killAllSessions(): Promise<void> { | ||
| try { | ||
| const manager = getActiveTerminalManager(); | ||
| if (manager instanceof DaemonTerminalManager) { | ||
| await manager.forceKillAll(); | ||
| const client = getTerminalHostClient(); | ||
| const connected = await client.tryConnectAndAuthenticate(); | ||
| if (connected) { | ||
| await client.killAll({}); | ||
| console.log("[Tray] Killed all daemon sessions"); |
There was a problem hiding this comment.
Log when terminal host is unavailable to avoid silent no-ops.
Right now, if tryConnectAndAuthenticate() returns false, the action silently does nothing. Consider logging (and apply the same pattern in killSession) so failures are diagnosable.
🔧 Suggested tweak
- const connected = await client.tryConnectAndAuthenticate();
- if (connected) {
- await client.killAll({});
- console.log("[Tray] Killed all daemon sessions");
- }
+ const connected = await client.tryConnectAndAuthenticate();
+ if (!connected) {
+ console.warn("[Tray/killAllSessions] Terminal host unavailable; no sessions killed");
+ } else {
+ await client.killAll({});
+ console.log("[Tray] Killed all daemon sessions");
+ }📝 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.
| async function killAllSessions(): Promise<void> { | |
| try { | |
| const manager = getActiveTerminalManager(); | |
| if (manager instanceof DaemonTerminalManager) { | |
| await manager.forceKillAll(); | |
| const client = getTerminalHostClient(); | |
| const connected = await client.tryConnectAndAuthenticate(); | |
| if (connected) { | |
| await client.killAll({}); | |
| console.log("[Tray] Killed all daemon sessions"); | |
| async function killAllSessions(): Promise<void> { | |
| try { | |
| const client = getTerminalHostClient(); | |
| const connected = await client.tryConnectAndAuthenticate(); | |
| if (!connected) { | |
| console.warn("[Tray/killAllSessions] Terminal host unavailable; no sessions killed"); | |
| } else { | |
| await client.killAll({}); | |
| console.log("[Tray] Killed all daemon sessions"); | |
| } | |
| } catch (error) { | |
| console.error("[Tray] Failed to kill all sessions:", error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/tray/index.ts` around lines 115 - 121, In
killAllSessions (and similarly in killSession) add a diagnostic log when
tryConnectAndAuthenticate() returns false so the function doesn't silently
no-op: after calling const client = getTerminalHostClient() and awaiting
client.tryConnectAndAuthenticate(), if connected is false call
processLogger.warn or console.warn with a clear message (e.g., "[Tray] Terminal
host unavailable, cannot kill all sessions" and include any identifying context)
and then return; keep the existing success log path unchanged and reuse the same
pattern in killSession to mirror behavior.
Summary
Test plan
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.