feat(desktop): add SSH remote workspace support #788
Conversation
Add the ability to connect to remote servers via SSH and open remote workspaces, similar to VS Code Remote SSH functionality. ## Features - **SSH Config Import**: Parse and import hosts from ~/.ssh/config file - **SSH Connection Management**: Connect to remote servers using key-based or SSH agent authentication - **Remote Projects**: Track remote project paths and associate them with SSH connections - **Remote Workspaces**: Create workspaces for remote projects with SSH terminal sessions - **Terminal Routing**: Route terminal operations (create, write, resize, stream) to the appropriate backend (local or SSH) based on workspace type ## Implementation Details ### New Files - `apps/desktop/src/lib/trpc/routers/ssh/` - SSH tRPC router for connection management, config parsing, and remote project operations - `apps/desktop/src/main/lib/ssh/` - SSH client, terminal manager, and config parser using ssh2 library - `apps/desktop/src/main/lib/workspace-runtime/ssh.ts` - SSH workspace runtime adapter - `apps/desktop/src/renderer/routes/_authenticated/settings/ssh/` - SSH settings page for managing connections - `apps/desktop/src/renderer/screens/main/components/StartView/OpenRemoteDialog.tsx` - Dialog for connecting to SSH hosts ### Modified Files - Terminal router now routes operations to SSH or local backend based on workspace type using a paneId->workspaceId mapping - Workspace query now checks both local and remote workspace tables - StartView displays remote projects and allows opening them - Stream subscription accepts workspaceId for correct terminal routing ### Database - Added ssh_connections, remote_projects, and remote_workspaces tables Co-Authored-By: Claude Opus 4.5 <[email protected]>
📝 WalkthroughWalkthroughAdds end-to-end SSH support: dependency, DB migration and schema, SSH client/parser/terminal manager, workspace-runtime SSH integration, tRPC SSH router and terminal routing changes, and renderer UI for SSH settings and remote-project flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings / OpenRemoteDialog
participant tRPC as tRPC SSH Router
participant DB as Local DB
participant Registry as WorkspaceRuntimeRegistry
participant Runtime as SSHWorkspaceRuntime / SSHTerminalManager
participant Client as SSHClient
User->>UI: select connection + remote path (or import)
UI->>tRPC: ssh.connect(connectionId)
tRPC->>DB: fetch ssh connection record
DB-->>tRPC: connection config
tRPC->>Registry: getSSHRuntime(config)
Registry->>Runtime: create or return runtime for config
Runtime->>Client: SSHClient.connect()
Client-->>Runtime: connected / events
UI->>tRPC: ssh.remoteProjects.create(...)
tRPC->>DB: insert remote_project
DB-->>tRPC: created row
UI->>tRPC: ssh.remoteWorkspaces.create(...)
tRPC->>DB: insert remote_workspace
DB-->>tRPC: created row
tRPC->>Registry: registerSSHWorkspace(workspaceId, sshConnectionId)
Registry-->>UI: success (navigate to workspace)
sequenceDiagram
participant Renderer as Terminal Renderer
participant tRPC as tRPC Terminal Router
participant Registry as WorkspaceRuntimeRegistry
participant Runtime as SSHWorkspaceRuntime / LocalRuntime
participant Manager as SSHTerminalManager / LocalTerminalManager
participant Client as SSHClient
Renderer->>tRPC: subscribe stream.useSubscription({ paneId, workspaceId })
tRPC->>Registry: getForWorkspaceId(workspaceId)
alt workspace is SSH
Registry-->>tRPC: SSHWorkspaceRuntime
tRPC->>Runtime: delegate stream for paneId
Runtime->>Manager: route to SSHTerminalManager
Manager->>Client: channel events (data/exit/error)
Client-->>Manager: data
Manager-->>tRPC: forward data event
else local workspace
Registry-->>tRPC: LocalRuntime
LocalRuntime->>Manager: forward data event
Manager-->>tRPC: forward data
end
tRPC-->>Renderer: data event
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}
There was a problem hiding this comment.
Actionable comments posted: 8
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)
124-190: PassisRemoteflag toresolveCwdand usepath.posixfor remote workspace paths.When
workspacePathis a POSIX remote path (e.g.,/home/user/...), the current implementation will fail on Windows becausenode:pathmethods use platform-specific separators. On Windows,join("/home/user/project", "apps")produces a mangled path with backslashes, causingexistsSync()to fail and the function to fall back to the baseworkspacePath, losing relative path resolution.Pass the
isRemoteflag toresolveCwdand conditionally usepath.posixfor remote workspaces:const cwd = resolveCwd(cwdOverride, workspacePath, isRemote);Then in
resolve-cwd.ts, import and usepath.posixwhenisRemoteis true to preserve POSIX path semantics.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/ssh/ssh-client.ts`:
- Around line 292-295: The code currently interpolates cwd directly into a shell
command via channel.write(`cd ${JSON.stringify(cwd)} && clear\n`), which risks
shell injection; update the logic in ssh-client.ts to avoid injecting raw cwd
into the shell by either validating/whitelisting the cwd (e.g., ensure it's an
absolute path with no shell metacharacters) or by using a proper escaping
function (e.g., shell-escape or similar) before interpolation, or alternately
change the approach to execute a safe built-in cd operation on the remote
channel (send a separate, validated exec rather than building a combined
command); modify the code around the cwd check and the channel.write call so
only a safely-escaped or validated path is sent to the remote shell.
In `@apps/desktop/src/main/lib/ssh/ssh-config-parser.ts`:
- Around line 77-78: The port parsing in the ssh-config-parser switch (case
"port") uses parseInt(value, 10) and may assign NaN to currentHost.port for
invalid input; update the logic in ssh-config-parser.ts where currentHost.port
is set so you parse the value, verify it's a finite integer within the valid
port range (1–65535) using Number.isInteger or Number.isFinite checks, and only
assign it when valid (otherwise leave unset or apply a safe default/emit a
warning) to avoid storing NaN in currentHost.port.
In `@apps/desktop/src/main/lib/ssh/ssh-terminal-manager.ts`:
- Around line 163-184: The per-pane sshClient.on handlers (data:${paneId},
exit:${paneId}, error:${paneId}) are added for each session but never removed,
causing listener leaks and duplicate events; modify the session creation in
ssh-terminal-manager to store the actual listener functions (e.g., dataHandler,
exitHandler, errorHandler) on the Session object (or a handlers map keyed by
paneId) and call sshClient.off/removeListener for those handlers when a session
ends (in the existing exit/kill/cleanup logic and the exit:${paneId} handler),
ensuring you detach data:${paneId}, exit:${paneId}, and error:${paneId} to
prevent MaxListenersExceededWarning and duplicate emissions.
- Around line 302-310: The catch block in the loop that calls
this.sshClient.killChannel(paneId) is swallowing errors; update the catch to
capture the exception (e) and log a meaningful error including paneId and
workspaceId and the error details so failures are diagnosable (e.g., use
this.logger.error or the module logger in this file to log "Failed to kill SSH
channel" with paneId, workspaceId and the error/stack), then increment failed as
before.
In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts`:
- Around line 177-192: The cleanupAll implementation can abort cleaning
remaining SSH runtimes if any runtime.terminal.cleanup() throws; update
cleanupAll to iterate over this.sshRuntimes and for each [id, runtime] call
runtime.terminal.cleanup() inside a try/catch so errors are logged but do not
break the loop, and ensure runtime.disconnect() is invoked in a finally block
(or after catching) so connections are always torn down even when cleanup fails;
preserve the final clearing of this.sshRuntimes, this.sshConfigs, and
this.workspaceToSSH after the loop.
- Around line 156-172: disconnectSSHRuntime can fail if
runtime.terminal.cleanup() or runtime.disconnect() throws, leaving sshRuntimes,
sshConfigs and workspaceToSSH in an inconsistent state; wrap the
cleanup/disconnect calls for the found runtime in a try/catch/finally: perform
runtime.terminal.cleanup() and runtime.disconnect() inside try, catch and log
the error with its details (do not swallow silently), and in finally always
delete this.sshRuntimes.delete(sshConnectionId),
this.sshConfigs.delete(sshConnectionId) and remove entries from
this.workspaceToSSH so cleanup always runs; after logging, rethrow the error so
callers can handle it. Use the existing function and symbols:
disconnectSSHRuntime, runtime.terminal.cleanup, runtime.disconnect, sshRuntimes,
sshConfigs, workspaceToSSH when making the change.
In
`@apps/desktop/src/renderer/screens/main/components/StartView/OpenRemoteDialog.tsx`:
- Around line 25-33: The onSuccess handler for createRemoteProject is hardcoding
the branch to "main"; change it to use the returned project's defaultBranch by
passing branch: project.defaultBranch || "main" when calling
createRemoteWorkspace.mutate so the workspace uses the actual default branch.
Locate the createRemoteProject mutation's onSuccess callback and update the
branch field in the createRemoteWorkspace.mutate call (reference:
createRemoteProject, createRemoteWorkspace, project.defaultBranch).
In `@packages/local-db/drizzle/meta/0011_snapshot.json`:
- Around line 4-5: The snapshot 0011 has a duplicated "id" value (currently
matching the previous snapshot) which must be unique; update the "id" field in
this snapshot's JSON to a new UUID while keeping "prevId" set to the existing
previous snapshot id (the current value of "prevId" should remain unchanged) so
the migration chain remains intact—regenerate a UUID for the "id" key in the
0011 snapshot JSON and save.
🧹 Nitpick comments (16)
apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx (4)
158-162: MoveexistingNamesSet computation outside the map callback.The
existingNamesSet is recreated for every host in the list. This should be computed once before mapping.♻️ Suggested refactor
<div className="max-h-64 overflow-y-auto space-y-2"> - {configHosts.map((host) => { - const existingNames = new Set( - (connections ?? []).map((c) => c.name.toLowerCase()), - ); - const alreadyExists = existingNames.has(host.name.toLowerCase()); + {(() => { + const existingNames = new Set( + (connections ?? []).map((c) => c.name.toLowerCase()), + ); + return configHosts.map((host) => { + const alreadyExists = existingNames.has(host.name.toLowerCase());Or better yet, use
useMemoat the component level:const existingConnectionNames = useMemo( () => new Set((connections ?? []).map((c) => c.name.toLowerCase())), [connections], );Then use
existingConnectionNames.has(host.name.toLowerCase())inside the map.
313-320: Consider adding a confirmation dialog before deleting connections.Deleting an SSH connection is a destructive action that could disrupt workflows if done accidentally. A confirmation step would improve the user experience and prevent accidental deletions.
Additionally,
deleteConnection.isPendingdisables all delete buttons when any deletion is in progress. If you want per-row disabled state, you'd need to track which connection is being deleted (similar totestingId).
364-375: Add validation for the port number.
parseInt(port, 10)can returnNaNif the input is empty or invalid. While therequiredattribute andtype="number"help, edge cases could slip through (e.g., submitting before the browser validates). Consider adding explicit validation.♻️ Suggested validation
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); + const portNum = parseInt(port, 10); + if (Number.isNaN(portNum) || portNum < 1 || portNum > 65535) { + // Handle invalid port - could set error state or show toast + return; + } onSubmit({ name, host, - port: parseInt(port, 10), + port: portNum, username, authMethod, privateKeyPath: authMethod === "key" ? privateKeyPath : undefined, remoteWorkDir: remoteWorkDir || undefined, }); };
339-482: Consider extractingAddConnectionFormto a separate file.Per coding guidelines, each file should contain one component. While
AddConnectionFormis a private helper tightly coupled to this page, extracting it to a sibling file (e.g.,add-connection-form.tsx) would improve testability and align with the project convention.This is a nitpick given the component is not exported and only used here.
packages/local-db/src/schema/zod.ts (2)
122-133: Constrain SSH numeric fields to valid ranges.
port,keepAliveInterval, andconnectionTimeoutcurrently accept negative/non-int values. Tightening these avoids invalid configs propagating to the runtime. Also, extracting the default port improves clarity. As per coding guidelines, extract hardcoded values to named constants.♻️ Proposed refinement
+const DEFAULT_SSH_PORT = 22; + export const sshConnectionConfigSchema = z.object({ id: z.string(), name: z.string(), host: z.string(), - port: z.number().default(22), + port: z.number().int().min(1).max(65535).default(DEFAULT_SSH_PORT), username: z.string(), authMethod: z.enum(SSH_AUTH_METHODS), privateKeyPath: z.string().optional(), agentForward: z.boolean().optional(), remoteWorkDir: z.string().optional(), - keepAliveInterval: z.number().optional(), - connectionTimeout: z.number().optional(), + keepAliveInterval: z.number().int().positive().optional(), + connectionTimeout: z.number().int().positive().optional(), });
115-118: Avoid duplicating SSHAuthMethod/SSHConnectionConfig definitions.There’s a parallel
SSHAuthMethodandSSHConnectionConfiginapps/desktop/src/main/lib/ssh/types.ts(Line 10–38). Consider sharing or re-exporting to prevent drift.apps/desktop/src/renderer/screens/main/components/StartView/OpenRemoteDialog.tsx (1)
113-120: Reset dialog state on all close paths.The “Configure SSH Servers” and “Manage Servers” links call
onClose()without resetting form state, so stale inputs can reappear when reopening. Consider a sharedhandleClose()that also callsresetForm().♻️ Suggested tweak
+ const handleClose = () => { + onClose(); + resetForm(); + }; ... - <Link + <Link to="/settings/ssh" className="inline-flex items-center gap-2 px-4 py-2 rounded-md bg-foreground text-background hover:bg-foreground/90 transition-colors text-sm font-medium" - onClick={onClose} + onClick={handleClose} > ... - <Link + <Link to="/settings/ssh" className="px-4 py-2 rounded-md text-muted-foreground hover:text-foreground transition-colors text-sm" - onClick={onClose} + onClick={handleClose} > ... - <button + <button type="button" onClick={() => { - onClose(); - resetForm(); + handleClose(); }}Also applies to: 191-204
apps/desktop/src/main/lib/ssh/ssh-config-parser.ts (1)
87-89:proxyJumpis parsed but never used.The
proxyJumpfield is captured from the SSH config butconvertToConnectionConfigsdoesn't include it in the output. If proxy jump support is intended for a future iteration, consider adding a TODO comment; otherwise, remove the dead code.apps/desktop/src/lib/trpc/routers/ssh/index.ts (3)
151-155: Manual deletion may be redundant due to cascade.The
remoteProjectstable hasonDelete: "cascade"on thesshConnectionIdforeign key (see schema.ts lines 192-194), so deleting the connection should automatically cascade to projects and workspaces. This manual deletion is defensive but redundant.Optional: rely on cascade
// Disconnect the runtime if active await registry.disconnectSSHRuntime(input.id); -// Delete associated remote projects (cascades to remote workspaces) -localDb - .delete(remoteProjects) - .where(eq(remoteProjects.sshConnectionId, input.id)) - .run(); - // Delete the connection localDb .delete(sshConnections) .where(eq(sshConnections.id, input.id)) .run();
597-610: Default branch "main" is hardcoded.When creating a default workspace, the branch is hardcoded to "main". Consider using
project.defaultBranchif available, falling back to "main".Proposed fix
// Create a default workspace +const defaultBranch = project.defaultBranch || "main"; workspace = localDb .insert(remoteWorkspaces) .values({ remoteProjectId: project.id, - branch: "main", - name: "main", + branch: defaultBranch, + name: defaultBranch, tabOrder: 0, }) .returning() .get();
483-489: Tab order calculation loads all workspaces into memory.For projects with many workspaces, this could be inefficient. Consider using a SQL aggregate instead.
Proposed optimization
-// Get max tab order -const maxOrder = localDb - .select() - .from(remoteWorkspaces) - .where(eq(remoteWorkspaces.remoteProjectId, input.remoteProjectId)) - .all() - .reduce((max, ws) => Math.max(max, ws.tabOrder), -1); +// Get max tab order using SQL aggregate +import { max } from "drizzle-orm"; + +const result = localDb + .select({ maxOrder: max(remoteWorkspaces.tabOrder) }) + .from(remoteWorkspaces) + .where(eq(remoteWorkspaces.remoteProjectId, input.remoteProjectId)) + .get(); +const maxOrder = result?.maxOrder ?? -1;apps/desktop/src/main/lib/ssh/ssh-client.ts (1)
150-170: Consider removing once handlers on timeout to prevent memory leaks.If the connection times out, the
oncehandlers forreadyanderrorremain attached. They should be removed on timeout to avoid potential issues.Proposed fix
return new Promise((resolve, reject) => { + const onReady = () => { + clearTimeout(timeout); + this.client.off("error", onError); + console.log(`[ssh/client] Connection ready`); + resolve(); + }; + + const onError = (err: Error) => { + clearTimeout(timeout); + this.client.off("ready", onReady); + console.error(`[ssh/client] Connection error: ${err.message}`); + reject(err); + }; + const timeout = setTimeout(() => { console.error(`[ssh/client] Connection timeout after ${this.config.connectionTimeout ?? DEFAULT_CONNECTION_TIMEOUT}ms`); + this.client.off("ready", onReady); + this.client.off("error", onError); this.client.end(); reject(new Error("Connection timeout")); }, this.config.connectionTimeout ?? DEFAULT_CONNECTION_TIMEOUT); - this.client.once("ready", () => { - clearTimeout(timeout); - console.log(`[ssh/client] Connection ready`); - resolve(); - }); - - this.client.once("error", (err) => { - clearTimeout(timeout); - console.error(`[ssh/client] Connection error: ${err.message}`); - reject(err); - }); + this.client.once("ready", onReady); + this.client.once("error", onError); this.client.connect(connectConfig); });apps/desktop/src/main/lib/workspace-runtime/ssh.ts (1)
109-197: Consider simplifying EventEmitter delegation.The EventEmitter interface delegation is verbose. If
TerminalRuntimeextendsEventEmitter, you could potentially use composition differently or extend the backend directly. However, this explicit delegation is safe and works correctly.apps/desktop/src/main/lib/workspace-runtime/registry.ts (3)
79-79:sshConfigsmap is stored but never read.The
sshConfigsmap is populated ingetSSHRuntimeand cleared indisconnectSSHRuntime/cleanupAll, but the stored configs are never retrieved or used elsewhere. Consider removing this dead state unless there's a planned use case.♻️ Suggested removal
class DefaultWorkspaceRuntimeRegistry implements ExtendedWorkspaceRuntimeRegistry { private localRuntime: LocalWorkspaceRuntime | null = null; private sshRuntimes: Map<string, SSHWorkspaceRuntime> = new Map(); private workspaceToSSH: Map<string, string> = new Map(); // workspaceId -> sshConnectionId - private sshConfigs: Map<string, SSHConnectionConfig> = new Map(); // sshConnectionId -> configThen remove corresponding
.set()and.delete()calls ingetSSHRuntime,disconnectSSHRuntime, andcleanupAll.
86-95: Silent fallback when SSH runtime is missing could mask bugs.If a workspace is registered as SSH (via
registerSSHWorkspace) but the SSH runtime hasn't been created yet (viagetSSHRuntime), this silently falls back to the local runtime. This could mask configuration issues where callers forget to initialize the SSH runtime before using the workspace.Consider adding a warning log to aid debugging:
🔧 Suggested improvement
getForWorkspaceId(workspaceId: string): WorkspaceRuntime { const sshConnectionId = this.workspaceToSSH.get(workspaceId); if (sshConnectionId) { const sshRuntime = this.sshRuntimes.get(sshConnectionId); if (sshRuntime) { return sshRuntime; } + console.warn(`[registry] Workspace ${workspaceId} is registered for SSH ${sshConnectionId} but runtime not found, falling back to local`); } return this.getDefault(); }
135-137: Missing log statement for consistency.
registerSSHWorkspacelogs the action, butunregisterSSHWorkspacedoes not. For debugging parity, consider adding a log here.🔧 Suggested improvement
unregisterSSHWorkspace(workspaceId: string): void { + console.log(`[registry] Unregistering workspace ${workspaceId} from SSH`); this.workspaceToSSH.delete(workspaceId); }
apps/desktop/src/renderer/screens/main/components/StartView/OpenRemoteDialog.tsx
Show resolved
Hide resolved
- Add Windows SSH agent support using OpenSSH named pipe (\\.\pipe\openssh-ssh-agent) instead of SSH_AUTH_SOCK - Auto-detect default SSH keys (id_ed25519, id_rsa, id_ecdsa) instead of hardcoding id_rsa - Fix tilde expansion to handle both forward and back slashes for cross-platform path compatibility Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/ssh/ssh-client.ts`:
- Around line 150-171: The promise in connect() currently registers a one-time
"error" handler while setupClientHandlers() registers a persistent "error"
handler, causing both the Promise rejection and attemptReconnect() to fire on
initial connection failure; modify the logic so initial connection errors do not
trigger the automatic reconnect: add a connection state flag (e.g., isConnected
or isInitialConnect) managed by connect() and setupClientHandlers(), have the
persistent this.client.on("error") handler call attemptReconnect() only when the
flag indicates an established connection (not during initial connect), and have
the once("error") in connect() still reject the promise for initial failures;
update connect(), setupClientHandlers(), and attemptReconnect() usages to
check/update that flag accordingly.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/ssh/ssh-client.ts (1)
265-326: Shell injection viacwd(lines 319-321) was flagged in a previous review.The concern about shell metacharacter injection when interpolating
cwdinto the shell command remains valid. Please address the earlier feedback.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/ssh/ssh-client.ts (2)
100-126: Consider exponential backoff for reconnection attempts.The fixed 2000ms delay between reconnection attempts could lead to aggressive reconnection behavior, especially if the server is temporarily overloaded. Exponential backoff is a common resilience pattern for this scenario.
♻️ Suggested exponential backoff implementation
+const BASE_RECONNECT_DELAY_MS = 1000; +const MAX_RECONNECT_DELAY_MS = 30000; -const RECONNECT_DELAY_MS = 2000; private attemptReconnect(): void { if (this.reconnectTimer) { return; } if (this.reconnectAttempts >= MAX_RECONNECT_ATTEMPTS) { console.log(`[ssh/client] Max reconnect attempts reached`); this.state = "error"; this.emitStatus("Max reconnect attempts reached"); return; } this.state = "reconnecting"; this.reconnectAttempts++; this.emitStatus(); + const delay = Math.min( + BASE_RECONNECT_DELAY_MS * Math.pow(2, this.reconnectAttempts - 1), + MAX_RECONNECT_DELAY_MS + ); console.log( - `[ssh/client] Reconnecting in ${RECONNECT_DELAY_MS}ms (attempt ${this.reconnectAttempts})`, + `[ssh/client] Reconnecting in ${delay}ms (attempt ${this.reconnectAttempts})`, ); this.reconnectTimer = setTimeout(() => { this.reconnectTimer = null; this.connect().catch((err) => { console.error(`[ssh/client] Reconnect failed:`, err.message); }); - }, RECONNECT_DELAY_MS); + }, delay); }
331-356: Use object parameters for functions with 2+ arguments.Per coding guidelines, functions with 2+ parameters should accept a single params object for self-documentation and extensibility. The
write,resize, andsignalmethods use positional arguments.♻️ Suggested refactor to object parameters
- write(paneId: string, data: string): void { + write({ paneId, data }: { paneId: string; data: string }): void { const channel = this.channels.get(paneId); if (channel) { channel.write(data); } } - resize(paneId: string, cols: number, rows: number): void { + resize({ paneId, cols, rows }: { paneId: string; cols: number; rows: number }): void { const channel = this.channels.get(paneId); if (channel) { channel.setWindow(rows, cols, 0, 0); } } - signal(paneId: string, signalName: string): void { + signal({ paneId, signalName }: { paneId: string; signalName: string }): void { const channel = this.channels.get(paneId); if (channel) { channel.signal(signalName); } }Based on coding guidelines, functions with 2+ parameters should use object parameters.
- Add path.normalize() after tilde expansion to ensure consistent path separators on Windows - Verify Windows OpenSSH agent pipe exists before using it, with helpful error message if the ssh-agent service is not running Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/ssh/ssh-client.ts`:
- Around line 342-367: The write, resize, and signal methods silently ignore
when this.channels.get(paneId) returns undefined; update each method (write,
resize, signal) to log a warning when the channel is missing instead of
swallowing the condition—include paneId and the attempted action/parameters
(e.g., cols/rows or signalName) in the message; use the class's existing logger
(e.g., this.logger or processLogger) to emit a warning so missing-channel cases
are visible during debugging.
♻️ Duplicate comments (3)
apps/desktop/src/main/lib/ssh/ssh-client.ts (3)
40-88: Solid event handler setup with one caveat.The persistent handlers are well-structured. The keyboard-interactive handler correctly finishes with an empty array since password prompts aren't supported.
Note: The interaction between the persistent
errorhandler (line 48) callingattemptReconnect()and the one-timeerrorhandler inconnect()was already flagged in a previous review.
150-171: Double error handling concern already flagged.The potential conflict between the one-time
errorhandler here and the persistenterrorhandler insetupClientHandlers()was addressed in a previous review comment.
329-332: Shell injection concern already flagged.The
cwdinterpolation into a shell command was addressed in a previous review comment.
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/ssh/ssh-client.ts (3)
100-126: Consider exponential backoff for reconnection.The fixed 2-second delay works but exponential backoff would be more resilient against transient network issues, reducing load during extended outages.
♻️ Optional: Exponential backoff
+const BASE_RECONNECT_DELAY_MS = 1000; +const MAX_RECONNECT_DELAY_MS = 30000; -const RECONNECT_DELAY_MS = 2000; private attemptReconnect(): void { // ... existing guards ... + const delay = Math.min( + BASE_RECONNECT_DELAY_MS * Math.pow(2, this.reconnectAttempts), + MAX_RECONNECT_DELAY_MS + ); console.log( - `[ssh/client] Reconnecting in ${RECONNECT_DELAY_MS}ms (attempt ${this.reconnectAttempts})`, + `[ssh/client] Reconnecting in ${delay}ms (attempt ${this.reconnectAttempts})`, ); this.reconnectTimer = setTimeout(() => { // ... - }, RECONNECT_DELAY_MS); + }, delay); }
259-266: Consider a dedicated flag to disable reconnection.Setting
reconnectAttempts = MAX_RECONNECT_ATTEMPTSworks but obscures intent. AshouldReconnectflag would be more explicit.♻️ Clearer reconnection control
+ private shouldReconnect = true; disconnect(): void { if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; } - this.reconnectAttempts = MAX_RECONNECT_ATTEMPTS; // Prevent reconnect + this.shouldReconnect = false; this.client.end(); } private attemptReconnect(): void { + if (!this.shouldReconnect) { + return; + } if (this.reconnectTimer) { return; } // ... }
311-317: Consider explicit encoding for data conversion.
toString()defaults to UTF-8, which is correct for most terminal output. However, explicit encoding prevents surprises with binary data or locale-specific output.♻️ Explicit encoding
channel.on("data", (data: Buffer) => { - this.emit(`data:${paneId}`, data.toString()); + this.emit(`data:${paneId}`, data.toString("utf-8")); }); channel.stderr.on("data", (data: Buffer) => { - this.emit(`data:${paneId}`, data.toString()); + this.emit(`data:${paneId}`, data.toString("utf-8")); });
- Use path.posix for remote workspace paths in resolveCwd to prevent mangled paths on Windows when working with POSIX remote paths - Fix shell injection vulnerability by adding shellEscape() and isValidRemotePath() validation for SSH cwd handling - Fix double error handling in SSH connect by tracking isInitialConnect flag to prevent reconnect during initial connection attempts - Add exponential backoff for reconnection (1s, 2s, 4s, 8s, 16s) capped at 30 seconds - Add warning logs when write/resize/signal is called on missing channels Co-Authored-By: Claude Opus 4.5 <[email protected]>
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 (2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
193-248: Clear pane mapping when createOrAttach fails.
paneToWorkspaceis set beforeterminal.createOrAttach. If the call throws, the mapping remains and later operations can be routed to a workspace that never attached. Clean it up on failure.🛠️ Proposed fix
try { const result = await terminal.createOrAttach({ paneId, tabId, workspaceId, workspaceName, workspacePath, rootPath, cwd, cols, rows, initialCommands, skipColdRestore, }); @@ return { paneId, isNew: result.isNew, scrollback: result.scrollback, wasRecovered: result.wasRecovered, viewportY: result.viewportY, isColdRestore: result.isColdRestore, previousCwd: result.previousCwd, snapshot: result.snapshot, }; } catch (error) { + paneToWorkspace.delete(paneId); if (DEBUG_TERMINAL) { console.warn("[Terminal Router] createOrAttach failed:", { callId, paneId, durationMs: Date.now() - startedAt, error: error instanceof Error ? error.message : String(error), }); } console.error("[Terminal Router] createOrAttach ERROR:", error); throw error; }apps/desktop/src/lib/trpc/routers/terminal/utils/resolve-cwd.ts (1)
3-33: RefactorresolveCwdto use a params object to avoid boolean blindness.The function signature with three positional parameters violates the coding guideline requiring object parameters for functions with 2+ arguments. The
isRemoteboolean flag is not self-documenting at call sites likeresolveCwd(cwdOverride, workspacePath, isRemote).♻️ Proposed refactor
-export function resolveCwd( - cwdOverride: string | undefined, - worktreePath: string | undefined, - isRemote = false, -): string | undefined { +export function resolveCwd({ + cwdOverride, + worktreePath, + isRemote = false, +}: { + cwdOverride?: string; + worktreePath?: string; + isRemote?: boolean; +}): string | undefined {Update the single call site in
apps/desktop/src/lib/trpc/routers/terminal/terminal.tsand test cases to use named properties.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/terminal/terminal.ts`:
- Around line 44-52: The getTerminalForPane function currently falls back to
registry.getDefault().terminal when paneToWorkspace mapping is missing; instead
fail fast so callers can reattach. Modify getTerminalForPane (and any callers if
needed) to throw a clear error (e.g., including the paneId) when
paneToWorkspace.get(paneId) is undefined rather than returning the default
terminal from getWorkspaceRuntimeRegistry().getDefault().terminal; keep the
existing successful path that calls getTerminalForWorkspace(workspaceId).
- Around line 563-575: Tests are calling caller.stream with a string but the
router's subscription input uses a Zod object schema (z.object({ paneId:
z.string(), workspaceId: z.string().optional() })) so update each test
invocation in terminal.stream.test.ts (the three caller.stream calls) to pass
the object shape, e.g. caller.stream({ paneId: "pane-1" }) (add workspaceId when
a workspace is needed), so the input matches the subscription handler in
terminal.ts.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/terminal/utils/resolve-cwd.ts (1)
39-75: Guard against Windows-style remote paths.In remote mode, inputs like
C:\fooor strings with backslashes will be treated as relative POSIX segments and joined to the worktree, which can be surprising on Windows. Consider rejecting/normalizing non‑POSIX inputs here, or ensure callers validate with a remote-path validator before callingresolveCwd.
- Clean up paneToWorkspace mapping on createOrAttach failure to prevent stale routing to workspaces that never attached - Refactor resolveCwd to use params object instead of positional args to follow coding guidelines and avoid boolean blindness Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Validate port in SSH config parser (1-65535 range) - Fix listener leaks in SSH terminal manager by storing and cleaning up handlers - Add error logging in killByWorkspaceId catch block - Add try/catch in cleanupAll to prevent aborting on individual failures - Add try/catch/finally in disconnectSSHRuntime for consistent state cleanup - Use project.defaultBranch instead of hardcoded "main" in OpenRemoteDialog - Fix duplicate ID in 0011_snapshot.json migration Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts`:
- Around line 156-181: In disconnectSSHRuntime, runtime.disconnect() is
currently inside the try block and is skipped if runtime.terminal.cleanup()
throws; move the disconnect call into the finally block so it always runs (but
only if runtime is truthy), capturing any error from disconnect separately so it
doesn't overwrite cleanupError; ensure state cleanup (this.sshRuntimes.delete,
this.sshConfigs.delete, and iterating this.workspaceToSSH) still runs and that
any thrown errors from cleanup() and disconnect() are propagated appropriately
(use separate error variables or aggregate errors) while referencing the
existing runtime variable and this.sshRuntimes/this.sshConfigs/workspaceToSSH
maps.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/ssh/ssh-terminal-manager.ts (1)
135-149: Clean up stale session handlers before recreating.At Line 135, if a session exists but is dead or the channel is missing, we proceed to recreate without detaching old SSHClient listeners, which can reintroduce duplicate events and listener leaks for the same pane.
🛠️ Proposed fix
// Check for existing session const existing = this.sessions.get(paneId); if (existing && existing.isAlive && this.sshClient.hasChannel(paneId)) { existing.lastActive = Date.now(); return { isNew: false, scrollback: "", wasRecovered: false, viewportY: existing.viewportY, }; } + + // Stale session: remove old handlers before recreating + if (existing) { + this.cleanupSessionHandlers(paneId); + this.sessions.delete(paneId); + }
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/workspace-runtime/registry.ts (2)
33-37: Prefer params object for registerSSHWorkspace.
Two positional params are easy to swap; a named params object is clearer and extensible. Please update the implementation and callers to match.♻️ Proposed refactor
- registerSSHWorkspace(workspaceId: string, sshConnectionId: string): void; + registerSSHWorkspace(params: { workspaceId: string; sshConnectionId: string }): void;As per coding guidelines.
113-129: Adopt [domain/operation] log prefixes for new registry logs.
This makes it easier to trace per-operation actions in mixed logs.♻️ Example log prefix updates
- console.log(`[registry] Creating new SSH runtime for ${config.name} (${config.host})`); + console.log(`[registry/getSSHRuntime] Creating new SSH runtime for ${config.name} (${config.host})`); ... - console.log(`[registry] Registering workspace ${workspaceId} with SSH connection ${sshConnectionId}`); + console.log(`[registry/registerSSHWorkspace] Registering workspace ${workspaceId} with SSH connection ${sshConnectionId}`);As per coding guidelines.
- Update resolve-cwd.test.ts to use object params - Update terminal.stream.test.ts to pass object to stream subscription - Make getTerminalForPane throw instead of falling back to default - Move runtime.disconnect() to finally block in disconnectSSHRuntime Co-Authored-By: Claude Opus 4.5 <[email protected]>
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/lib/trpc/routers/terminal/terminal.stream.test.ts (1)
74-81: Tests will fail—missing required pane-to-workspace mapping setupWhen
stream({ paneId })is called withoutworkspaceId, the code routes togetTerminalForPane(paneId)(line 603 in terminal.ts), which throws aTRPCErrorif the paneId isn't in thepaneToWorkspacemap. These tests never populate that map (onlycreateOrAttachdoes, at line 207), so the subscription will fail immediately with "No workspace mapping found for pane pane-N".Fix by passing
workspaceIdtostream()so it usesgetTerminalForWorkspace()directly (which is mocked):- const stream$ = await caller.stream({ paneId: "pane-1" }); + const stream$ = await caller.stream({ paneId: "pane-1", workspaceId: "test-workspace" });Apply the same change to all three test cases (lines 81, 120, 148).
🧹 Nitpick comments (4)
apps/desktop/src/lib/trpc/routers/terminal/utils/resolve-cwd.test.ts (1)
24-109: Missing test coverage forisRemoteparameter.The implementation accepts an
isRemoteparameter that significantly changes behavior (uses POSIX paths, skips local existence checks, returnsundefinedinstead ofhomedirin some fallback cases). Given this PR adds SSH remote workspace support, consider adding test coverage forisRemote = truescenarios.Example test cases for remote path handling
describe("when isRemote is true", () => { test("uses POSIX path resolution", () => { expect( resolveCwd({ cwdOverride: "apps/desktop", worktreePath: "/home/user/project", isRemote: true, }), ).toBe("/home/user/project/apps/desktop"); }); test("returns absolute path without existence check", () => { expect( resolveCwd({ cwdOverride: "/remote/path", isRemote: true, }), ).toBe("/remote/path"); }); test("returns undefined when relative path has no worktreePath", () => { expect( resolveCwd({ cwdOverride: "apps/desktop", isRemote: true, }), ).toBeUndefined(); }); });apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
138-158: Consider adding a warning when remote project is not found.If
remoteWsexists butremoteProjectis not found (e.g., deleted or orphaned),workspacePathandrootPathremainundefined. The terminal will still be created but may behave unexpectedly without a working directory. Consider logging a warning for observability.💡 Add warning log
if (remoteProject) { workspacePath = remoteProject.remotePath; rootPath = remoteProject.remotePath; + } else { + console.warn( + `[Terminal Router] Remote project not found for workspace ${workspaceId}, remoteProjectId: ${remoteWs.remoteProjectId}`, + ); }apps/desktop/src/main/lib/workspace-runtime/registry.ts (2)
89-98: Consider logging when mapped SSH runtime is missing.If
workspaceToSSHhas a mapping butsshRuntimesdoesn't have the runtime, this silently falls back to local. WhiledisconnectSSHRuntimecleans both maps, edge cases (race conditions, partial cleanup) could leave inconsistent state. A warning would aid debugging.💡 Add warning for missing SSH runtime
getForWorkspaceId(workspaceId: string): WorkspaceRuntime { const sshConnectionId = this.workspaceToSSH.get(workspaceId); if (sshConnectionId) { const sshRuntime = this.sshRuntimes.get(sshConnectionId); if (sshRuntime) { return sshRuntime; } + console.warn( + `[registry] Workspace ${workspaceId} mapped to SSH ${sshConnectionId} but runtime not found, falling back to local`, + ); } return this.getDefault(); }
284-290: Consider cleanup before reset to prevent orphaned connections.
resetWorkspaceRuntimeRegistrysets the instance to null without callingcleanupAll(). In tests that create SSH runtimes, this could leave orphaned connections. Consider:💡 Optional: Add async reset with cleanup
/** * Reset the registry (for testing only). * This should not be called in production code. */ -export function resetWorkspaceRuntimeRegistry(): void { +export async function resetWorkspaceRuntimeRegistry(): Promise<void> { + if (registryInstance) { + await registryInstance.cleanupAll(); + } registryInstance = null; }
- Pass workspaceId in stream tests to avoid getTerminalForPane throwing - Add warning when remote project not found for workspace - Add warning when SSH runtime not found for mapped workspace Co-Authored-By: Claude Opus 4.5 <[email protected]>
Extract hostNames and skipExisting from optional input to preserve
type information instead of using `input ?? {}` which loses types.
Co-Authored-By: Claude Opus 4.5 <[email protected]>
…er leak - Extend WorkspaceRuntimeRegistry to support cloud workspaces alongside SSH - Add CloudWorkspaceRuntime interface for cloud providers to implement - Add unified workspaceToRemote mapping for both SSH and cloud types - Add cloud-specific methods: registerCloudRuntime, getCloudRuntime, registerCloudWorkspace, isCloudWorkspace, disconnectCloudRuntime - Add generic methods: getWorkspaceType, unregisterRemoteWorkspace - Fix memory leak: connectionStatus listener now properly removed on disconnect - Update cleanupAll to handle local, SSH, and cloud runtimes Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts`:
- Around line 350-355: The loop deletes entries from the Map while iterating
(this.workspaceToRemote) which can skip items; change both places (the SSH
cleanup block using sshConnectionId and the similar cloud cleanup in
disconnectCloudRuntime) to first collect keys to remove (e.g., const toDelete:
string[] = []; for (const [workspaceId, mapping] of this.workspaceToRemote) { if
(mapping.type === "ssh" && mapping.runtimeId === sshConnectionId)
toDelete.push(workspaceId); }) and then iterate toDelete to call
this.workspaceToRemote.delete(id), ensuring you reference the same Map
(workspaceToRemote) and the same matching logic used currently.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx`:
- Around line 42-114: The UI uses a single testingId which is overwritten by
rapid clicks causing racey pending state; replace testingId (and setTestingId)
with a per-connection map (e.g., testingIds: Record<string, boolean>) and update
all usage sites: in handleTest toggle testingIds[id]=true and remove it on
completion, and in testConnection.onSuccess and onError clear testingIds[id]
while still updating testResults; also update any UI checks that read testingId
to instead consult testingIds[id] (including the other occurrence range
referencing the same logic).
- Around line 376-386: The handleSubmit handler currently uses parseInt(port,
10) without validation; update handleSubmit to validate that port is a numeric
integer between 1 and 65535 before calling onSubmit, and block submission if
invalid. Add or reuse a local state like portError (e.g., via useState) and set
it when validation fails so the UI can render a small inline error near the port
input; only call onSubmit({ ..., port: parsedPort, ... }) when parsedPort is a
valid integer in the 1–65535 range. Ensure when authMethod is "key" the
privateKeyPath logic remains unchanged and clear portError on successful
validation.
- Around line 54-65: Add onError handlers to the createConnection and
deleteConnection mutations so failures are logged instead of swallowed: in the
electronTrpc.ssh.createConnection.useMutation call for createConnection and in
electronTrpc.ssh.deleteConnection.useMutation for deleteConnection, add an
onError callback that calls the logger (or processLogger) with a clear prefixed
message (e.g., "createConnection error:" / "deleteConnection error:") and the
error object; keep existing onSuccess behavior
(utils.ssh.listConnections.invalidate and setIsAddDialogOpen(false)) unchanged.
In `@apps/web/src/app/auth/desktop/success/page.tsx`:
- Around line 11-13: The protocolParam from searchParams is user-controlled and
must be validated before passing to DesktopRedirect; add an allowlist check
(e.g., const allowedProtocols = ['lance', 'lance-desktop', 'myapp'] or whatever
app-specific schemes you support) and only use protocolParam if it exactly
matches an entry in that list, otherwise fall back to a safe default (or
reject/redirect to a safe page). Update the code paths that read protocolParam
(the destructuring of searchParams and the place where DesktopRedirect is
constructed/used) to perform this validation and ensure inputs containing ":" or
"//" or full URLs are rejected.
🧹 Nitpick comments (5)
apps/api/src/app/api/auth/desktop/connect/route.ts (1)
10-10: Consider validating the protocol parameter.The protocol parameter is forwarded to the success page and used to construct a deep link URL. While the value originates from the desktop app's build-time constant, consider validating against an allowlist (
["superset", "superset-dev"]) for defense-in-depth. This prevents potential open-redirect-like behavior if the parameter were ever manipulated.🔒 Optional: Add protocol validation
const protocol = url.searchParams.get("protocol") || "superset"; +const ALLOWED_PROTOCOLS = ["superset", "superset-dev"]; +if (!ALLOWED_PROTOCOLS.includes(protocol)) { + return new Response("Invalid protocol", { status: 400 }); +}Also applies to: 20-22
apps/desktop/src/main/lib/ssh/ssh-terminal-manager.ts (2)
413-422: Handler cleanup missing before killing channels incleanup().The
cleanup()method kills all channels but doesn't remove the session handlers first. Whiledisconnect()is called afterward which does remove handlers, the handlers could still fire during the channel kill loop. Consider callingcleanupSessionHandlers(paneId)beforekillChannel(paneId)for consistency with thekill()method.♻️ Suggested improvement
async cleanup(): Promise<void> { // Kill all sessions for (const paneId of this.sessions.keys()) { + this.cleanupSessionHandlers(paneId); this.sshClient.killChannel(paneId); } this.sessions.clear(); // Disconnect SSH this.disconnect(); }
376-384: Async method performs only synchronous operations.
getSessionCountByWorkspaceIdis markedasyncbut only performs synchronous iteration over an in-memory Map. This is likely to match theTerminalRuntimeinterface signature, which is acceptable for consistency.apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx (2)
163-169: Avoid recreatingexistingNamesfor every host.It’s recomputed inside the map, yielding O(n²) work. Precompute once before mapping.
♻️ Suggested refactor
-<div className="max-h-64 overflow-y-auto space-y-2"> - {configHosts.map((host) => { - const existingNames = new Set( - (connections ?? []).map((c) => c.name.toLowerCase()), - ); +const existingNames = new Set( + (connections ?? []).map((c) => c.name.toLowerCase()), +); + +<div className="max-h-64 overflow-y-auto space-y-2"> + {configHosts.map((host) => { const alreadyExists = existingNames.has(host.name.toLowerCase()); return (
351-496: SplitAddConnectionForminto its own file.This file defines two components, which conflicts with the “one component per file” rule.
As per coding guidelines, keep one component per
.tsxfile.
| // Remove all workspace mappings for this SSH connection | ||
| for (const [workspaceId, mapping] of this.workspaceToRemote) { | ||
| if (mapping.type === "ssh" && mapping.runtimeId === sshConnectionId) { | ||
| this.workspaceToRemote.delete(workspaceId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Deleting from Map while iterating may skip entries.
Deleting entries from workspaceToRemote while iterating over it with for...of can cause entries to be skipped. Consider collecting the keys to delete first, then deleting them after the iteration.
🐛 Proposed fix
// Remove all workspace mappings for this SSH connection
+ const workspacesToRemove: string[] = [];
for (const [workspaceId, mapping] of this.workspaceToRemote) {
if (mapping.type === "ssh" && mapping.runtimeId === sshConnectionId) {
- this.workspaceToRemote.delete(workspaceId);
+ workspacesToRemove.push(workspaceId);
}
}
+ for (const workspaceId of workspacesToRemove) {
+ this.workspaceToRemote.delete(workspaceId);
+ }The same issue exists in disconnectCloudRuntime at lines 452-457.
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts` around lines 350 -
355, The loop deletes entries from the Map while iterating
(this.workspaceToRemote) which can skip items; change both places (the SSH
cleanup block using sshConnectionId and the similar cloud cleanup in
disconnectCloudRuntime) to first collect keys to remove (e.g., const toDelete:
string[] = []; for (const [workspaceId, mapping] of this.workspaceToRemote) { if
(mapping.type === "ssh" && mapping.runtimeId === sshConnectionId)
toDelete.push(workspaceId); }) and then iterate toDelete to call
this.workspaceToRemote.delete(id), ensuring you reference the same Map
(workspaceToRemote) and the same matching logic used currently.
| const [testingId, setTestingId] = useState<string | null>(null); | ||
| const [testResults, setTestResults] = useState< | ||
| Record<string, { success: boolean; message: string }> | ||
| >({}); | ||
|
|
||
| // Queries | ||
| const { data: connections, isLoading } = | ||
| electronTrpc.ssh.listConnections.useQuery(); | ||
| const { data: hasConfig } = electronTrpc.ssh.hasSSHConfig.useQuery(); | ||
| const { data: configHosts } = electronTrpc.ssh.getSSHConfigHosts.useQuery(); | ||
|
|
||
| // Mutations | ||
| const createConnection = electronTrpc.ssh.createConnection.useMutation({ | ||
| onSuccess: () => { | ||
| utils.ssh.listConnections.invalidate(); | ||
| setIsAddDialogOpen(false); | ||
| }, | ||
| }); | ||
|
|
||
| const deleteConnection = electronTrpc.ssh.deleteConnection.useMutation({ | ||
| onSuccess: () => { | ||
| utils.ssh.listConnections.invalidate(); | ||
| }, | ||
| }); | ||
|
|
||
| const testConnection = electronTrpc.ssh.testConnection.useMutation({ | ||
| onSuccess: (result, variables) => { | ||
| setTestResults((prev) => ({ | ||
| ...prev, | ||
| [variables.id]: result, | ||
| })); | ||
| setTestingId(null); | ||
| }, | ||
| onError: (error, variables) => { | ||
| setTestResults((prev) => ({ | ||
| ...prev, | ||
| [variables.id]: { success: false, message: error.message }, | ||
| })); | ||
| setTestingId(null); | ||
| }, | ||
| }); | ||
|
|
||
| const [importResult, setImportResult] = useState<{ | ||
| imported: string[]; | ||
| skipped: string[]; | ||
| total: number; | ||
| } | null>(null); | ||
|
|
||
| const importFromConfig = electronTrpc.ssh.importFromSSHConfig.useMutation({ | ||
| onSuccess: (result) => { | ||
| utils.ssh.listConnections.invalidate(); | ||
| setImportResult(result); | ||
| // Only close if something was imported | ||
| if (result.imported.length > 0) { | ||
| setIsImportDialogOpen(false); | ||
| setImportResult(null); | ||
| } | ||
| }, | ||
| onError: (err) => { | ||
| console.error("[ssh/import] Failed to import:", err); | ||
| setImportResult(null); | ||
| }, | ||
| }); | ||
|
|
||
| const handleTest = (id: string) => { | ||
| setTestingId(id); | ||
| setTestResults((prev) => { | ||
| const next = { ...prev }; | ||
| delete next[id]; | ||
| return next; | ||
| }); | ||
| testConnection.mutate({ id }); | ||
| }; |
There was a problem hiding this comment.
Track pending tests per connection instead of a single testingId.
With multiple quick clicks, testingId gets overwritten and pending state clears prematurely. A per‑connection map avoids racey UI states.
✅ Suggested fix
-const [testingId, setTestingId] = useState<string | null>(null);
+const [testingIds, setTestingIds] = useState<Record<string, boolean>>({});
const testConnection = electronTrpc.ssh.testConnection.useMutation({
onSuccess: (result, variables) => {
setTestResults((prev) => ({
...prev,
[variables.id]: result,
}));
- setTestingId(null);
+ setTestingIds((prev) => {
+ const next = { ...prev };
+ delete next[variables.id];
+ return next;
+ });
},
onError: (error, variables) => {
setTestResults((prev) => ({
...prev,
[variables.id]: { success: false, message: error.message },
}));
- setTestingId(null);
+ setTestingIds((prev) => {
+ const next = { ...prev };
+ delete next[variables.id];
+ return next;
+ });
},
});
const handleTest = (id: string) => {
- setTestingId(id);
+ setTestingIds((prev) => ({ ...prev, [id]: true }));
setTestResults((prev) => {
const next = { ...prev };
delete next[id];
return next;
});
testConnection.mutate({ id });
};
-const isTesting = testingId === conn.id;
+const isTesting = !!testingIds[conn.id];Also applies to: 262-322
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx` around
lines 42 - 114, The UI uses a single testingId which is overwritten by rapid
clicks causing racey pending state; replace testingId (and setTestingId) with a
per-connection map (e.g., testingIds: Record<string, boolean>) and update all
usage sites: in handleTest toggle testingIds[id]=true and remove it on
completion, and in testConnection.onSuccess and onError clear testingIds[id]
while still updating testResults; also update any UI checks that read testingId
to instead consult testingIds[id] (including the other occurrence range
referencing the same logic).
| const createConnection = electronTrpc.ssh.createConnection.useMutation({ | ||
| onSuccess: () => { | ||
| utils.ssh.listConnections.invalidate(); | ||
| setIsAddDialogOpen(false); | ||
| }, | ||
| }); | ||
|
|
||
| const deleteConnection = electronTrpc.ssh.deleteConnection.useMutation({ | ||
| onSuccess: () => { | ||
| utils.ssh.listConnections.invalidate(); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add onError logging for create/delete mutations.
Failures currently surface no signal. Please add prefixed error logging for both mutations so users (and logs) capture failures.
✅ Suggested fix
const createConnection = electronTrpc.ssh.createConnection.useMutation({
onSuccess: () => {
utils.ssh.listConnections.invalidate();
setIsAddDialogOpen(false);
},
+ onError: (err) => {
+ console.error("[ssh/create] Failed to create connection:", err);
+ },
});
const deleteConnection = electronTrpc.ssh.deleteConnection.useMutation({
onSuccess: () => {
utils.ssh.listConnections.invalidate();
},
+ onError: (err, variables) => {
+ console.error("[ssh/delete] Failed to delete connection:", err, {
+ id: variables.id,
+ });
+ },
});As per coding guidelines, never swallow errors silently.
📝 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.
| const createConnection = electronTrpc.ssh.createConnection.useMutation({ | |
| onSuccess: () => { | |
| utils.ssh.listConnections.invalidate(); | |
| setIsAddDialogOpen(false); | |
| }, | |
| }); | |
| const deleteConnection = electronTrpc.ssh.deleteConnection.useMutation({ | |
| onSuccess: () => { | |
| utils.ssh.listConnections.invalidate(); | |
| }, | |
| }); | |
| const createConnection = electronTrpc.ssh.createConnection.useMutation({ | |
| onSuccess: () => { | |
| utils.ssh.listConnections.invalidate(); | |
| setIsAddDialogOpen(false); | |
| }, | |
| onError: (err) => { | |
| console.error("[ssh/create] Failed to create connection:", err); | |
| }, | |
| }); | |
| const deleteConnection = electronTrpc.ssh.deleteConnection.useMutation({ | |
| onSuccess: () => { | |
| utils.ssh.listConnections.invalidate(); | |
| }, | |
| onError: (err, variables) => { | |
| console.error("[ssh/delete] Failed to delete connection:", err, { | |
| id: variables.id, | |
| }); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx` around
lines 54 - 65, Add onError handlers to the createConnection and deleteConnection
mutations so failures are logged instead of swallowed: in the
electronTrpc.ssh.createConnection.useMutation call for createConnection and in
electronTrpc.ssh.deleteConnection.useMutation for deleteConnection, add an
onError callback that calls the logger (or processLogger) with a clear prefixed
message (e.g., "createConnection error:" / "deleteConnection error:") and the
error object; keep existing onSuccess behavior
(utils.ssh.listConnections.invalidate and setIsAddDialogOpen(false)) unchanged.
| const handleSubmit = (e: React.FormEvent) => { | ||
| e.preventDefault(); | ||
| onSubmit({ | ||
| name, | ||
| host, | ||
| port: parseInt(port, 10), | ||
| username, | ||
| authMethod, | ||
| privateKeyPath: authMethod === "key" ? privateKeyPath : undefined, | ||
| remoteWorkDir: remoteWorkDir || undefined, | ||
| }); |
There was a problem hiding this comment.
Validate the port before submitting.
parseInt can yield NaN (or out‑of‑range values), which will pass through to the backend. Consider blocking submit and showing a small inline error when the port isn’t 1–65535.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh/page.tsx` around
lines 376 - 386, The handleSubmit handler currently uses parseInt(port, 10)
without validation; update handleSubmit to validate that port is a numeric
integer between 1 and 65535 before calling onSubmit, and block submission if
invalid. Add or reuse a local state like portError (e.g., via useState) and set
it when validation fails so the UI can render a small inline error near the port
input; only call onSubmit({ ..., port: parsedPort, ... }) when parsedPort is a
valid integer in the 1–65535 range. Ensure when authMethod is "key" the
privateKeyPath logic remains unchanged and clear portError on successful
validation.
| searchParams: Promise<{ desktop_state?: string; protocol?: string }>; | ||
| }) { | ||
| const { desktop_state: state } = await searchParams; | ||
| const { desktop_state: state, protocol: protocolParam } = await searchParams; |
There was a problem hiding this comment.
Validate the protocol parameter to prevent arbitrary URL schemes.
The protocolParam comes from URL search params (user-controllable) and is used directly to construct the desktop callback URL. An attacker could craft a malicious link like ?protocol=javascript&desktop_state=... or ?protocol=https://evil.com/fake which would be passed to DesktopRedirect.
You should validate against the known allowed protocols:
🔒 Proposed fix: Validate protocol against allowlist
+const ALLOWED_PROTOCOLS = ["superset", "superset-dev"];
+
// Use protocol from URL param (passed from desktop app), fallback to production
-const protocol = protocolParam || "superset";
+const protocol = protocolParam && ALLOWED_PROTOCOLS.includes(protocolParam)
+ ? protocolParam
+ : "superset";
const desktopUrl = `${protocol}://auth/callback?token=${encodeURIComponent(token)}&expiresAt=${encodeURIComponent(expiresAt.toISOString())}&state=${encodeURIComponent(state)}`;Also applies to: 80-82
🤖 Prompt for AI Agents
In `@apps/web/src/app/auth/desktop/success/page.tsx` around lines 11 - 13, The
protocolParam from searchParams is user-controlled and must be validated before
passing to DesktopRedirect; add an allowlist check (e.g., const allowedProtocols
= ['lance', 'lance-desktop', 'myapp'] or whatever app-specific schemes you
support) and only use protocolParam if it exactly matches an entry in that list,
otherwise fall back to a safe default (or reject/redirect to a safe page).
Update the code paths that read protocolParam (the destructuring of searchParams
and the place where DesktopRedirect is constructed/used) to perform this
validation and ensure inputs containing ":" or "//" or full URLs are rejected.
1 participant
This PR adds SSH remote workspace support to the Superset desktop app, similar to VS Code Remote SSH functionality. Users can now:
~/.ssh/configfileFeatures
~/.ssh/configImplementation
New Components
ssh2libraryKey Changes
workspaceIdfor correct terminal routingssh_connections,remote_projects, andremote_workspacesdatabase tablesWindows Support
\\.\pipe\openssh-ssh-agent)id_ed25519,id_rsa,id_ecdsa)Test Plan
~/.ssh/config🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.