feat(desktop): add SSH remote workspace support#2352
Closed
jihlee wants to merge 1 commit into
Closed
Conversation
Add the ability to connect to remote SSH hosts and create workspaces where coding agents run on the remote machine instead of locally, similar to VS Code's Remote-SSH functionality. - Add SSH connection manager with connection pooling, shell channels, SFTP caching, and reconnect logic - Add SSH config parser (~/.ssh/config) for importing existing hosts - Implement SshTerminalRuntime and SshWorkspaceRuntime abstractions - Add remote WorkspaceFsService over SFTP for file operations - Add tRPC router for SSH host CRUD, connect/disconnect, and connection state subscriptions - Add SSH Hosts settings page for host management - Add SSH Remote tab in New Workspace modal - Add connection badges and server icons in workspace sidebar - Add ssh_hosts table and sshHostId/remotePath columns to workspaces - Add DB migration (0036_add_ssh_remote_workspaces) - Register remote workspaces in runtime registry with lazy hydration
Contributor
📝 WalkthroughWalkthroughThis pull request introduces comprehensive SSH remote workspace support to the Superset desktop application. It adds SSH connection management infrastructure, TRPC endpoints for SSH host and remote workspace operations, database schema extensions to store SSH configurations, workspace runtime implementations for remote execution, a filesystem service abstraction for SSH-backed file operations, and UI components for managing SSH hosts and creating remote workspaces. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as UI Layer
participant TRPC as TRPC Router
participant SSH as SSH Manager
participant DB as Database
participant Runtime as Runtime Registry
User->>UI: Click "Create Remote Workspace"
UI->>TRPC: createRemote(projectId, sshHostId, remotePath)
TRPC->>DB: Validate project exists
TRPC->>DB: Get SSH host config
TRPC->>SSH: Connect to SSH host (if not connected)
SSH-->>TRPC: Connection established
TRPC->>SSH: Get SFTP client & verify remotePath
SSH-->>TRPC: Remote path accessible
TRPC->>DB: Insert remote workspace record
DB-->>TRPC: Workspace created
TRPC->>Runtime: registerRemoteWorkspace(workspaceId, hostId)
Runtime->>Runtime: Create SshWorkspaceRuntime
TRPC-->>UI: Return workspace + metadata
UI-->>User: Workspace ready for use
Note over Runtime: Runtime now routes FS ops to SSH for this workspace
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
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🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
140-143:⚠️ Potential issue | 🟠 MajorSkip local Git-status reads for remote workspaces.
getAllGroupednow feedsremotePaththroughworktreePath, so this hook will run against a path that only exists on the SSH host. Hovering a remote item can therefore produce bogus local status/errors and unnecessary filesystem work.🩹 Suggested fix
const { status: localChanges } = useGitChangesStatus({ worktreePath, - enabled: hasHovered && !!worktreePath, + enabled: hasHovered && !!worktreePath && !isRemoteWorkspace, staleTime: GITHUB_STATUS_STALE_TIME, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 140 - 143, The hook useGitChangesStatus in WorkspaceListItem runs against worktreePath even for remote workspaces (remotePath is set), causing bogus local status reads; update the enabled flag to only run when hovered, worktreePath exists, and the workspace is not remote by adding a check like && !remotePath (i.e. enabled: hasHovered && !!worktreePath && !remotePath) so local Git reads are skipped for remote workspaces.
🧹 Nitpick comments (9)
apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/page.tsx (1)
4-4: Prefer arenderer/...alias for the shared settings-search import.This import crosses out of the local folder, so using the tsconfig alias here would keep the import style consistent with the rest of
apps/desktop.Suggested change
-import { getMatchingItemsForSection } from "../utils/settings-search"; +import { getMatchingItemsForSection } from "renderer/routes/_authenticated/settings/utils/settings-search";As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/page.tsx` at line 4, The import of getMatchingItemsForSection currently uses a relative path ("../utils/settings-search"); update it to use the project's tsconfig alias for renderer imports (e.g., replace the relative import with the renderer/... alias) so it conforms to apps/desktop import conventions; locate the import of getMatchingItemsForSection in this file and switch to the renderer alias form while leaving the imported symbol name unchanged.apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts (1)
284-286: Keep remote runtime cleanup adjacent todeleteWorkspace()in both flows.
deleteunregisters immediately, butclosewaits until afterhideProjectIfNoWorkspaces()andupdateActiveWorkspaceIfRemoved(). If either helper throws, the workspace row is already gone while the remote runtime stays registered. I’d move theclosecleanup up to mirrordelete, or extract a shared post-delete helper.Suggested change
deleteWorkspace(input.id); + if (workspace.type === "remote") { + getWorkspaceRuntimeRegistry().unregisterRemoteWorkspace(input.id); + } hideProjectIfNoWorkspaces(workspace.projectId); updateActiveWorkspaceIfRemoved(input.id); - - if (workspace.type === "remote") { - getWorkspaceRuntimeRegistry().unregisterRemoteWorkspace(input.id); - }Also applies to: 325-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts` around lines 284 - 286, The remote runtime unregister call (getWorkspaceRuntimeRegistry().unregisterRemoteWorkspace(input.id)) must be executed immediately after deleteWorkspace() in both the "delete" and "close" flows to avoid leaving a registered runtime if subsequent helpers throw; move the unregisterRemoteWorkspace call up so it sits directly after deleteWorkspace() in the close flow (mirror the delete flow) or extract a shared helper (e.g., postDeleteCleanup or unregisterRemoteWorkspaceForId) and call it immediately after deleteWorkspace() in both code paths, keeping hideProjectIfNoWorkspaces() and updateActiveWorkspaceIfRemoved() after that.apps/desktop/src/main/lib/workspace-runtime/ssh.ts (1)
8-8: Consider using import alias for consistency.The import uses a relative path
"../ssh". Per coding guidelines, use aliases as defined intsconfig.jsonwhen possible (e.g.,"main/lib/ssh").♻️ Suggested import alias
-import { getSshConnectionManager } from "../ssh"; +import { getSshConnectionManager } from "main/lib/ssh";As per coding guidelines: "Use alias as defined in
tsconfig.jsonwhen possible"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/ssh.ts` at line 8, Replace the relative import of getSshConnectionManager from "../ssh" with the tsconfig alias import (e.g., "main/lib/ssh") to match project conventions; update the import statement that imports getSshConnectionManager so it uses the alias path instead of a relative path, ensuring other references to getSshConnectionManager remain unchanged.apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts (1)
118-120: Type cast bypasses type safety; consider a proper adapter.The
as nevercast suppresses type checking entirely. While the comment explains the structural compatibility issue, this approach could hide future type mismatches if eitherSFTPWrapperinterface changes.Consider creating a thin adapter function or type assertion function that validates the structural compatibility at runtime or at least narrows the cast scope.
♻️ Alternative: scoped assertion
return createSshWorkspaceFsService({ - // Cast: ssh2 SFTPWrapper satisfies SftpWrapper structurally; minor err type difference (undefined vs null) - getSftp: () => sshManager.getSftpClient(sshHostId) as never, + getSftp: async () => { + const client = await sshManager.getSftpClient(sshHostId); + // ssh2 SFTPWrapper is structurally compatible with SftpWrapper + // (minor difference: error callback param is Error | undefined vs Error | null) + return client as unknown as Parameters<typeof createSshWorkspaceFsService>[0] extends { getSftp: () => Promise<infer T> } ? T : never; + },Alternatively, if the type mismatch is truly minor (null vs undefined), consider updating the
SftpWrapperinterface inworkspace-fsto be more permissive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts` around lines 118 - 120, The current use of "as never" on getSftp (returning sshManager.getSftpClient(sshHostId) as never) bypasses TypeScript safety; replace this with a small adapter function that calls sshManager.getSftpClient(sshHostId), asserts/normalizes the minor difference (e.g. converts null to undefined or provides the missing error shape), and returns a properly typed SftpWrapper; implement the adapter near createSshWorkspaceFsService usage (e.g., a getSftpAdapter function) that performs a runtime check for required methods/props and then returns the value typed as SftpWrapper so you can remove the "as never" cast while preserving safety and clarity.apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx (2)
401-409: Add confirmation before deleting SSH host.Deleting an SSH host could leave associated remote workspaces orphaned. Consider adding a confirmation dialog to prevent accidental deletions.
🛡️ Suggested implementation
<Button variant="ghost" size="icon" className="h-8 w-8 text-destructive hover:text-destructive" - onClick={() => deleteHost.mutate({ id: host.id })} + onClick={() => { + if (window.confirm(`Delete SSH host "${host.label}"? Any remote workspaces using this host will no longer be accessible.`)) { + deleteHost.mutate({ id: host.id }); + } + }} disabled={deleteHost.isPending} >For a more polished UX, consider using an AlertDialog component instead of
window.confirm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx` around lines 401 - 409, Add a confirmation step before calling deleteHost.mutate to prevent accidental host deletions: wrap the delete action (the Button onClick that currently calls deleteHost.mutate({ id: host.id })) in a confirmation flow (either window.confirm or preferably the project's AlertDialog component) and only invoke deleteHost.mutate when the user confirms; update the Button handler in SshHostsSettings (the Button rendering the LuTrash2 icon and referencing deleteHost.isPending) to open the dialog and then call deleteHost.mutate({ id: host.id }) on confirm, keeping the disabled state tied to deleteHost.isPending.
369-392: Simplify onClick handlers.The ternary expressions returning
nullare redundant since the buttons are already disabled via thedisabledprop:♻️ Cleaner onClick handlers
<Button variant="outline" size="sm" - onClick={() => - connectHost.isPending - ? null - : connectHost.mutate({ id: host.id }) - } + onClick={() => connectHost.mutate({ id: host.id })} disabled={connectHost.isPending} > Connect </Button> <Button variant="ghost" size="sm" - onClick={() => - disconnectHost.isPending - ? null - : disconnectHost.mutate({ id: host.id }) - } + onClick={() => disconnectHost.mutate({ id: host.id })} disabled={disconnectHost.isPending} > Disconnect </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx` around lines 369 - 392, The onClick handlers for the Connect and Disconnect Buttons use redundant ternary expressions that return null when pending; remove the ternary and call the mutation directly (e.g. change the Connect button's onClick from the conditional to simply invoking connectHost.mutate({ id: host.id }) and do the same for disconnectHost.mutate({ id: host.id })). Keep the existing disabled={connectHost.isPending} / disabled={disconnectHost.isPending} props to prevent clicks while pending and ensure connectHost and disconnectHost are still referenced as before.apps/desktop/src/main/lib/workspace-runtime/registry.ts (1)
51-67: Consider filtering in SQL rather than client-side.The current implementation fetches all non-deleted workspaces and filters client-side. While acceptable for a local SQLite DB with few workspaces, the filter can be pushed to the WHERE clause for cleaner intent:
♻️ Suggested optimization
private hydrateRemoteWorkspaces(): void { if (this.hydrated) return; this.hydrated = true; const remoteWorkspaces = localDb .select() .from(workspaces) - .where(isNull(workspaces.deletingAt)) + .where( + and( + isNull(workspaces.deletingAt), + eq(workspaces.type, "remote"), + isNotNull(workspaces.sshHostId), + ), + ) - .all() - .filter((w) => w.type === "remote" && w.sshHostId); + .all(); for (const workspace of remoteWorkspaces) { - if (workspace.sshHostId) { - this.remoteWorkspaceMap.set(workspace.id, workspace.sshHostId); - } + this.remoteWorkspaceMap.set(workspace.id, workspace.sshHostId!); } }You'll need to add imports for
and,eq, andisNotNullfromdrizzle-orm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts` around lines 51 - 67, The hydrateRemoteWorkspaces method currently fetches all non-deleted workspaces then filters for type === "remote" and sshHostId client-side; update the query to push those predicates into the WHERE clause (use and(eq(workspaces.type, "remote"), isNotNull(workspaces.sshHostId)) alongside the existing isNull(workspaces.deletingAt)) so the DB returns only matching rows, and add/import and, eq, isNotNull from drizzle-orm; keep the rest of the logic that sets this.remoteWorkspaceMap with workspace.id -> workspace.sshHostId unchanged.apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts (1)
266-276: Type assertion forsetWindowis fragile but necessary.The
ssh2TypeScript definitions may not fully expose thesetWindowmethod onClientChannel. The type assertion works but could break if the ssh2 library changes its internal API.Consider adding a runtime check or documenting this dependency:
🛡️ Optional defensive check
resizeSession(paneId: string, cols: number, rows: number): void { const session = this.sessions.get(paneId); if (!session) { console.warn( `[SshConnectionManager] resizeSession: session ${paneId} not found`, ); return; } - // ClientChannel exposes setWindow for PTY resize - ( - session.channel as ClientChannel & { - setWindow( - rows: number, - cols: number, - height: number, - width: number, - ): void; - } - ).setWindow(rows, cols, 0, 0); + // ClientChannel exposes setWindow for PTY resize (ssh2 internal API) + const channel = session.channel as unknown as { + setWindow?: (rows: number, cols: number, height: number, width: number) => void; + }; + if (typeof channel.setWindow === "function") { + channel.setWindow(rows, cols, 0, 0); + } else { + console.warn(`[SshConnectionManager] setWindow not available for pane ${paneId}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts` around lines 266 - 276, The code currently force-casts session.channel to ClientChannel to call setWindow (the assertion around session.channel as ClientChannel & { setWindow(...) }) which is fragile if ssh2 changes; update the call site in ssh-connection-manager by adding a runtime guard that checks typeof (session.channel as any)?.setWindow === 'function' before invoking setWindow(rows, cols, 0, 0) and handle the missing-method case (e.g., log a warning via the module's logger or skip resizing) and keep the TypeScript assertion only as a last-resort fallback; also add a short comment documenting the dependency on ssh2 exposing setWindow so future readers know why the guard exists.packages/workspace-fs/src/ssh/service.ts (1)
330-345: Full file read before slicing is inefficient for large files.The current implementation reads the entire file into memory before checking if it exceeds
maxBytes. For very large files, this defeats the purpose of the limit.SFTP does support range reads via
open+readwith offset/length, but that would require additional complexity. Consider documenting this limitation or implementing streaming for large file support in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workspace-fs/src/ssh/service.ts` around lines 330 - 345, readFileBufferUpTo currently calls sftpReadFile which reads the whole file into memory; change it to avoid full-file reads by using the SFTP range API: call getSftp(), use sftp.stat or sftp.fstat to get file size, open the remote file (sftp.open), allocate a buffer of input.maxBytes, call sftp.read with offset 0 and length input.maxBytes into that buffer, close the handle, set exceededLimit = (size > input.maxBytes) and return the sliced buffer; update references to sftpReadFile usage in readFileBufferUpTo to use this open/read/close flow (or if you prefer to keep current behavior, add a clear comment on the limitation and a FIXME pointing to getSftp(), sftpReadFile, and readFileBufferUpTo so future work can implement streaming/ranged reads).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/electron-builder.ts`:
- Around line 162-167: The current copy rule object for ssh2 only copies
node_modules/ssh2 but misses top-level dependencies installed alongside it; add
four additional copy rule objects matching the existing ssh2 pattern for "asn1",
"bcrypt-pbkdf", "cpu-features", and "nan" so each has from:
"node_modules/<pkg>", to: "node_modules/<pkg>", and filter: ["**/*"] (mirroring
the ssh2 entry) to ensure these packages are included in packaged builds.
In `@apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts`:
- Around line 97-105: The testConnection input schema currently allows any
integer for port; update the zod schema used by the testConnection route to
enforce the same port bounds as the persisted-host schema by changing the port
validator to z.number().int().min(1).max(65535).default(22), keeping the rest of
the fields (hostname, username, authMethod, privateKeyPath, password,
passphrase) unchanged so testConnection, create, and update all validate ports
identically.
In `@apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts`:
- Around line 141-143: The current handler in the channel.on("close", ...)
callback (in workspace-fs-service.ts) coerces a null exitCode to 0 which can
hide failures; change the resolve payload to preserve null or use a sentinel
(e.g., exitCode: exitCode ?? -1) instead of 0 so callers can distinguish “no
exit code received” from successful execution — update the resolve in the
channel.on("close", (exitCode: number | null) => { ... }) block and ensure any
downstream code expecting a number handles the sentinel or nullable value.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts`:
- Around line 1020-1040: The SFTP check currently only verifies existence;
update the sftp.stat callback used in the create workspace flow
(sshManager.getSftpClient / sftp.stat) to accept the second stats parameter and
confirm the path is a directory before resolving. In the callback for
sftp.stat(input.remotePath, ...) inspect stats: prefer calling
stats.isDirectory() if available, otherwise check the mode bits (e.g. test
(stats.mode & 0o170000) === 0o040000) and reject with an error if it is not a
directory; keep the same error handling around the try/catch that wraps
sshManager.getSftpClient and the Promise.
In `@apps/desktop/src/main/lib/ssh/ssh-config-parser.ts`:
- Around line 33-37: The parser currently treats block.host as a single string
(in the loop over "for (const block of blocks)") which leaves multi-pattern Host
lines like "Host foo bar" as one value and causes patterns containing "*" (e.g.,
"Host foo *.corp") to be dropped; update the logic to split block.host on
whitespace into individual patterns, then iterate each pattern: for each pattern
skip if pattern === "*" or pattern.includes("*"), otherwise handle/import that
single pattern (same handling currently applied to block.host). Apply the same
split-and-iterate fix to the other handling site referenced around the code that
mirrors lines 86-90.
- Around line 75-84: The parser only checks for a literal space or '=' when
locating the key/value separator, so tab or other whitespace-separated
directives are ignored; update the separator detection by computing spaceIdx
using a regex search for any whitespace (e.g. line.search(/\s/)) instead of
indexOf(" "), keep eqIdx for '=' and then derive sepIdx the same way, then slice
key and value as before (symbols: spaceIdx, eqIdx, sepIdx, key, value,
line.slice) so lines like "Host\tprod" are correctly parsed.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/components/SshRemoteGroup/SshRemoteGroup.tsx`:
- Around line 27-29: The component currently uses shared booleans isConnecting
and isConnected which cause cross-host state leaks; change these to per-host
state (e.g., objects or Maps keyed by the host identifier or connectionKey used
in SshRemoteGroup) and update all setters and callbacks to mutate only the
current host's entry. Locate usages of isConnecting, setIsConnecting,
isConnected, setIsConnected, and onSuccess/onError handlers inside
SshRemoteGroup and replace them so connect attempts set state for the specific
host (setConnecting(hostId, true)), on success setConnected(hostId, true) and on
failure clear only that host's flags; also revise any UI checks (e.g., disabling
inputs) to reference the current host's state and optionally garbage-collect
entries when hosts are removed or changed.
- Around line 55-60: The handler handleConnect currently always sends the input
as password to connectMutation.mutateAsync, but for hosts with authMethod ===
"privateKey" the API expects the secret under passphrase; update handleConnect
so it looks up the target host's authMethod (e.g., from sshHost or selected SSH
host object) and, when authMethod === "privateKey", call
connectMutation.mutateAsync with { id: sshHostId, passphrase: password ||
undefined } instead of password, otherwise continue to send { id: sshHostId,
password: password || undefined }; make this change in the handleConnect
function where connectMutation.mutateAsync is invoked.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx`:
- Around line 28-29: The context menu in CollapsedWorkspaceItem currently uses
worktree-specific copy ("Close Worktree") for non-branch items, which results in
incorrect copy for type === "remote"; update the destructive-action label
generation in CollapsedWorkspaceItem (the branch/non-branch menu path) to choose
neutral wording for remote items (e.g., "Remove Workspace" or "Disconnect
Remote") instead of "Close Worktree" — implement a small conditional on the
workspace type (the union "worktree" | "branch" | "remote") when building the
menu so branch keeps branch-specific labels, worktree keeps "Close Worktree",
and remote receives neutral copy.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx`:
- Around line 23-27: DeleteWorkspaceDialog currently treats "remote" like local
worktrees; update DeleteWorkspaceDialogProps handling so workspaceType
explicitly branches for "remote" (separate from "worktree" and "branch") and
avoid rendering any worktree/local-branch controls when workspaceType ===
"remote". Specifically: in the DeleteWorkspaceDialog component, guard UI pieces
that reference deleteLocalBranch, worktree copy/tooltips, and preference-loading
logic so they only appear for "worktree" or "branch"; do not forward
deleteLocalBranch into the delete mutation when workspaceType === "remote"
(adjust the mutation call, e.g., in the onConfirm/handleDelete code path that
calls deleteWorkspaceMutation or similar). Also split/replace description and
tooltip strings into distinct remote-specific text and use those when
workspaceType === "remote" so the dialog copy no longer mentions worktrees/local
branches. Ensure all checks reference the workspaceType enum value strings
("worktree" | "branch" | "remote") and update any props/state names
(DeleteWorkspaceDialogProps, deleteLocalBranch) accordingly.
In `@packages/local-db/drizzle/0036_add_ssh_remote_workspaces.sql`:
- Line 15: The migration adds workspaces.ssh_host_id referencing ssh_hosts but
SQLite won't enforce ON DELETE SET NULL, so update the SSH host deletion flow to
perform application-layer cleanup: in the SSH hosts deletion handler (the
function that deletes from ssh_hosts), add a DB update to set
workspaces.ssh_host_id = NULL for rows referencing the deleted ssh_host id
(mirror the pattern used for sectionId cleanup in sections.ts around the section
deletion code), or alternatively document the chosen cascading strategy in the
SSH host deletion docs if you intentionally rely on application-level
guarantees.
In `@packages/workspace-fs/src/ssh/service.ts`:
- Around line 637-692: pollOnce uses a GNU-only `find ... -printf` and silences
errors, and it also hardcodes isDirectory: false for all events; update pollOnce
to use a portable remote listing (e.g., run a cross-platform command per host
like `find ... -type f -print0 -exec stat -f "%N %m %HT" "%@" \;` on BSD/macOS
or `find ... -printf` on GNU, or fallback to `ls -lR`/`stat` output) by
detecting the remote OS at connection time or trying a GNU-safe command and
falling back, parse the output into a Map of path→(mtime,type) instead of only
mtime, and when generating events via push (in pollOnce) set isDirectory based
on the parsed type (or by running `test -d <path>`/stat mode) so
create/update/delete events correctly reflect directories vs files; use
execCommand and the existing lastStats/current maps and revision/push logic to
replace the current parsing and hardcoded isDirectory behavior.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 140-143: The hook useGitChangesStatus in WorkspaceListItem runs
against worktreePath even for remote workspaces (remotePath is set), causing
bogus local status reads; update the enabled flag to only run when hovered,
worktreePath exists, and the workspace is not remote by adding a check like &&
!remotePath (i.e. enabled: hasHovered && !!worktreePath && !remotePath) so local
Git reads are skipped for remote workspaces.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts`:
- Around line 118-120: The current use of "as never" on getSftp (returning
sshManager.getSftpClient(sshHostId) as never) bypasses TypeScript safety;
replace this with a small adapter function that calls
sshManager.getSftpClient(sshHostId), asserts/normalizes the minor difference
(e.g. converts null to undefined or provides the missing error shape), and
returns a properly typed SftpWrapper; implement the adapter near
createSshWorkspaceFsService usage (e.g., a getSftpAdapter function) that
performs a runtime check for required methods/props and then returns the value
typed as SftpWrapper so you can remove the "as never" cast while preserving
safety and clarity.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts`:
- Around line 284-286: The remote runtime unregister call
(getWorkspaceRuntimeRegistry().unregisterRemoteWorkspace(input.id)) must be
executed immediately after deleteWorkspace() in both the "delete" and "close"
flows to avoid leaving a registered runtime if subsequent helpers throw; move
the unregisterRemoteWorkspace call up so it sits directly after
deleteWorkspace() in the close flow (mirror the delete flow) or extract a shared
helper (e.g., postDeleteCleanup or unregisterRemoteWorkspaceForId) and call it
immediately after deleteWorkspace() in both code paths, keeping
hideProjectIfNoWorkspaces() and updateActiveWorkspaceIfRemoved() after that.
In `@apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts`:
- Around line 266-276: The code currently force-casts session.channel to
ClientChannel to call setWindow (the assertion around session.channel as
ClientChannel & { setWindow(...) }) which is fragile if ssh2 changes; update the
call site in ssh-connection-manager by adding a runtime guard that checks typeof
(session.channel as any)?.setWindow === 'function' before invoking
setWindow(rows, cols, 0, 0) and handle the missing-method case (e.g., log a
warning via the module's logger or skip resizing) and keep the TypeScript
assertion only as a last-resort fallback; also add a short comment documenting
the dependency on ssh2 exposing setWindow so future readers know why the guard
exists.
In `@apps/desktop/src/main/lib/workspace-runtime/registry.ts`:
- Around line 51-67: The hydrateRemoteWorkspaces method currently fetches all
non-deleted workspaces then filters for type === "remote" and sshHostId
client-side; update the query to push those predicates into the WHERE clause
(use and(eq(workspaces.type, "remote"), isNotNull(workspaces.sshHostId))
alongside the existing isNull(workspaces.deletingAt)) so the DB returns only
matching rows, and add/import and, eq, isNotNull from drizzle-orm; keep the rest
of the logic that sets this.remoteWorkspaceMap with workspace.id ->
workspace.sshHostId unchanged.
In `@apps/desktop/src/main/lib/workspace-runtime/ssh.ts`:
- Line 8: Replace the relative import of getSshConnectionManager from "../ssh"
with the tsconfig alias import (e.g., "main/lib/ssh") to match project
conventions; update the import statement that imports getSshConnectionManager so
it uses the alias path instead of a relative path, ensuring other references to
getSshConnectionManager remain unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx`:
- Around line 401-409: Add a confirmation step before calling deleteHost.mutate
to prevent accidental host deletions: wrap the delete action (the Button onClick
that currently calls deleteHost.mutate({ id: host.id })) in a confirmation flow
(either window.confirm or preferably the project's AlertDialog component) and
only invoke deleteHost.mutate when the user confirms; update the Button handler
in SshHostsSettings (the Button rendering the LuTrash2 icon and referencing
deleteHost.isPending) to open the dialog and then call deleteHost.mutate({ id:
host.id }) on confirm, keeping the disabled state tied to deleteHost.isPending.
- Around line 369-392: The onClick handlers for the Connect and Disconnect
Buttons use redundant ternary expressions that return null when pending; remove
the ternary and call the mutation directly (e.g. change the Connect button's
onClick from the conditional to simply invoking connectHost.mutate({ id: host.id
}) and do the same for disconnectHost.mutate({ id: host.id })). Keep the
existing disabled={connectHost.isPending} / disabled={disconnectHost.isPending}
props to prevent clicks while pending and ensure connectHost and disconnectHost
are still referenced as before.
In `@apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/page.tsx`:
- Line 4: The import of getMatchingItemsForSection currently uses a relative
path ("../utils/settings-search"); update it to use the project's tsconfig alias
for renderer imports (e.g., replace the relative import with the renderer/...
alias) so it conforms to apps/desktop import conventions; locate the import of
getMatchingItemsForSection in this file and switch to the renderer alias form
while leaving the imported symbol name unchanged.
In `@packages/workspace-fs/src/ssh/service.ts`:
- Around line 330-345: readFileBufferUpTo currently calls sftpReadFile which
reads the whole file into memory; change it to avoid full-file reads by using
the SFTP range API: call getSftp(), use sftp.stat or sftp.fstat to get file
size, open the remote file (sftp.open), allocate a buffer of input.maxBytes,
call sftp.read with offset 0 and length input.maxBytes into that buffer, close
the handle, set exceededLimit = (size > input.maxBytes) and return the sliced
buffer; update references to sftpReadFile usage in readFileBufferUpTo to use
this open/read/close flow (or if you prefer to keep current behavior, add a
clear comment on the limitation and a FIXME pointing to getSftp(), sftpReadFile,
and readFileBufferUpTo so future work can implement streaming/ranged reads).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4f1d2ec-65e2-469f-97c3-303ae77b794f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
apps/desktop/electron-builder.tsapps/desktop/package.jsonapps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/ssh-hosts/index.tsapps/desktop/src/lib/trpc/routers/workspace-fs-service.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/worktree.tsapps/desktop/src/main/lib/ssh/index.tsapps/desktop/src/main/lib/ssh/ssh-config-parser.tsapps/desktop/src/main/lib/ssh/ssh-connection-manager.tsapps/desktop/src/main/lib/ssh/types.tsapps/desktop/src/main/lib/workspace-runtime/index.tsapps/desktop/src/main/lib/workspace-runtime/registry.tsapps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.tsapps/desktop/src/main/lib/workspace-runtime/ssh.tsapps/desktop/src/main/lib/workspace-runtime/types.tsapps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModalDraftContext.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/NewWorkspaceModalContent/NewWorkspaceModalContent.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/SshRemoteGroup/SshRemoteGroup.tsxapps/desktop/src/renderer/components/NewWorkspaceModal/components/SshRemoteGroup/index.tsapps/desktop/src/renderer/lib/getWorkspaceDisplayName/getWorkspaceDisplayName.tsapps/desktop/src/renderer/react-query/workspaces/index.tsapps/desktop/src/renderer/react-query/workspaces/useCreateRemoteWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/screens/main/components/CommandPalette/useCommandPalette.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceConnectionBadge.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceIcon.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/types.tsapps/desktop/src/renderer/screens/main/components/WorkspacesListView/types.tsapps/desktop/src/renderer/stores/settings-state.tspackages/local-db/drizzle/0036_add_ssh_remote_workspaces.sqlpackages/local-db/drizzle/meta/0036_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.tspackages/local-db/src/schema/zod.tspackages/workspace-fs/package.jsonpackages/workspace-fs/src/ssh/index.tspackages/workspace-fs/src/ssh/service.ts
Comment on lines
+162
to
+167
| // ssh2 for remote SSH workspace connections | ||
| { | ||
| from: "node_modules/ssh2", | ||
| to: "node_modules/ssh2", | ||
| filter: ["**/*"], | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SSH2_VERSION=$(python - <<'PY'
import json
with open("apps/desktop/package.json") as f:
pkg = json.load(f)
print(pkg["dependencies"]["ssh2"].lstrip("^~"))
PY
)
echo "ssh2 version: $SSH2_VERSION"
curl -fsSL "https://registry.npmjs.org/ssh2/${SSH2_VERSION}" | python - <<'PY'
import json, sys
data = json.load(sys.stdin)
print("dependencies:")
print(json.dumps(data.get("dependencies", {}), indent=2, sort_keys=True))
print("optionalDependencies:")
print(json.dumps(data.get("optionalDependencies", {}), indent=2, sort_keys=True))
PY
echo
echo "Current electron-builder coverage:"
rg -n -C2 'ssh2|cpu-features|nan|asn1|bcrypt-pbkdf' apps/desktop/electron-builder.tsRepository: superset-sh/superset
Length of output: 853
🏁 Script executed:
cat apps/desktop/package.json | grep -A 5 '"ssh2"'Repository: superset-sh/superset
Length of output: 215
🏁 Script executed:
if [ -f apps/desktop/package-lock.json ]; then
echo "=== package-lock.json exists ==="
jq '.dependencies."ssh2" | keys' apps/desktop/package-lock.json 2>/dev/null | head -20
elif [ -f bun.lock ]; then
echo "=== bun.lock exists ==="
grep -A 30 'ssh2' bun.lock | head -40
else
echo "No lock file found"
fiRepository: superset-sh/superset
Length of output: 1479
🏁 Script executed:
echo "=== Current electron-builder.ts packaging rules ==="
cat -n apps/desktop/electron-builder.ts | sed -n '150,200p'Repository: superset-sh/superset
Length of output: 2192
🏁 Script executed:
echo "=== Looking for all copy rules in electron-builder.ts ==="
rg -n 'from.*node_modules' apps/desktop/electron-builder.tsRepository: superset-sh/superset
Length of output: 750
🏁 Script executed:
echo "=== Searching bun.lock for ssh2 dependencies ==="
grep -A 50 '"ssh2"' bun.lock | head -60Repository: superset-sh/superset
Length of output: 2658
🏁 Script executed:
echo "=== Full ssh2 entry in bun.lock ==="
python3 << 'EOF'
import re
with open('bun.lock', 'r') as f:
content = f.read()
# Find ssh2@1.16.0 entry
pattern = r'"ssh2@\^1\.16\.0".*?(?="(?:[\w-]+)@|$)'
match = re.search(pattern, content, re.DOTALL)
if match:
print(match.group(0)[:1500])
else:
print("ssh2 entry not found with that pattern, trying simpler search")
# Find any ssh2 section
if 'ssh2@^1.16.0' in content:
idx = content.find('ssh2@^1.16.0')
print(content[idx:idx+1000])
EOFRepository: superset-sh/superset
Length of output: 161
🏁 Script executed:
echo "=== Check if ssh2 dependencies are nested or separate in bun.lock ==="
rg -A 3 'ssh2@.*1\.16\.0' bun.lockRepository: superset-sh/superset
Length of output: 133
🏁 Script executed:
echo "=== Packages ssh2 depends on (from bun.lock) ==="
echo "runtime dependencies: asn1, bcrypt-pbkdf"
echo "optional dependencies: cpu-features, nan"
echo
echo "=== Are these packaged in electron-builder.ts? ==="
for pkg in asn1 bcrypt-pbkdf cpu-features nan; do
if rg -q "from.*$pkg" apps/desktop/electron-builder.ts; then
echo "✓ $pkg - explicitly packaged"
else
echo "✗ $pkg - NOT packaged"
fi
doneRepository: superset-sh/superset
Length of output: 349
Add explicit copy rules for ssh2 dependencies.
The ssh2@1.17.0 package requires four dependencies that are not currently packaged: asn1, bcrypt-pbkdf (runtime), and cpu-features, nan (optional). These are installed at the top-level node_modules, not nested under node_modules/ssh2, so the current copy rule will not include them. SSH connections will work in dev but fail in packaged builds.
Add copy rules similar to the existing ssh2 rule for each of these four packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/electron-builder.ts` around lines 162 - 167, The current copy
rule object for ssh2 only copies node_modules/ssh2 but misses top-level
dependencies installed alongside it; add four additional copy rule objects
matching the existing ssh2 pattern for "asn1", "bcrypt-pbkdf", "cpu-features",
and "nan" so each has from: "node_modules/<pkg>", to: "node_modules/<pkg>", and
filter: ["**/*"] (mirroring the ssh2 entry) to ensure these packages are
included in packaged builds.
Comment on lines
+97
to
+105
| z.object({ | ||
| hostname: z.string().min(1), | ||
| port: z.number().int().default(22), | ||
| username: z.string().min(1), | ||
| authMethod: z.enum(["password", "privateKey", "agent"]), | ||
| privateKeyPath: z.string().optional(), | ||
| password: z.string().optional(), | ||
| passphrase: z.string().optional(), | ||
| }), |
Contributor
There was a problem hiding this comment.
Align testConnection port validation with the persisted-host schema.
create and update reject ports outside 1..65535, but testConnection currently accepts any integer. That lets obviously invalid values reach the SSH client only on the test path.
🩹 Suggested fix
- port: z.number().int().default(22),
+ port: z.number().int().min(1).max(65535).default(22),📝 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.
Suggested change
| z.object({ | |
| hostname: z.string().min(1), | |
| port: z.number().int().default(22), | |
| username: z.string().min(1), | |
| authMethod: z.enum(["password", "privateKey", "agent"]), | |
| privateKeyPath: z.string().optional(), | |
| password: z.string().optional(), | |
| passphrase: z.string().optional(), | |
| }), | |
| z.object({ | |
| hostname: z.string().min(1), | |
| port: z.number().int().min(1).max(65535).default(22), | |
| username: z.string().min(1), | |
| authMethod: z.enum(["password", "privateKey", "agent"]), | |
| privateKeyPath: z.string().optional(), | |
| password: z.string().optional(), | |
| passphrase: z.string().optional(), | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts` around lines 97 - 105,
The testConnection input schema currently allows any integer for port; update
the zod schema used by the testConnection route to enforce the same port bounds
as the persisted-host schema by changing the port validator to
z.number().int().min(1).max(65535).default(22), keeping the rest of the fields
(hostname, username, authMethod, privateKeyPath, password, passphrase) unchanged
so testConnection, create, and update all validate ports identically.
Comment on lines
+141
to
+143
| channel.on("close", (exitCode: number | null) => { | ||
| resolve({ stdout, stderr, exitCode: exitCode ?? 0 }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Exit code null coalesced to 0 may mask command failures.
When exitCode is null (e.g., if the channel closes unexpectedly without an exit code), treating it as 0 (success) could mask actual failures. Consider preserving null or using a sentinel value.
🐛 Suggested fix
channel.on("close", (exitCode: number | null) => {
- resolve({ stdout, stderr, exitCode: exitCode ?? 0 });
+ resolve({ stdout, stderr, exitCode: exitCode ?? -1 });
});Using -1 or another sentinel value makes it clearer that the exit code was not received, rather than implying success.
📝 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.
Suggested change
| channel.on("close", (exitCode: number | null) => { | |
| resolve({ stdout, stderr, exitCode: exitCode ?? 0 }); | |
| }); | |
| channel.on("close", (exitCode: number | null) => { | |
| resolve({ stdout, stderr, exitCode: exitCode ?? -1 }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts` around lines 141 -
143, The current handler in the channel.on("close", ...) callback (in
workspace-fs-service.ts) coerces a null exitCode to 0 which can hide failures;
change the resolve payload to preserve null or use a sentinel (e.g., exitCode:
exitCode ?? -1) instead of 0 so callers can distinguish “no exit code received”
from successful execution — update the resolve in the channel.on("close",
(exitCode: number | null) => { ... }) block and ensure any downstream code
expecting a number handles the sentinel or nullable value.
Comment on lines
+1020
to
+1040
| // Verify remote path exists via SFTP | ||
| try { | ||
| const sftp = await sshManager.getSftpClient(input.sshHostId); | ||
| await new Promise<void>((resolve, reject) => { | ||
| sftp.stat(input.remotePath, (err) => { | ||
| if (err) { | ||
| reject( | ||
| new Error( | ||
| `Remote path "${input.remotePath}" does not exist or is not accessible`, | ||
| ), | ||
| ); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| throw error instanceof Error | ||
| ? error | ||
| : new Error("Failed to verify remote path"); | ||
| } |
Contributor
There was a problem hiding this comment.
SFTP stat doesn't verify the path is a directory.
The current check only verifies the remote path exists, but doesn't confirm it's a directory. A workspace root path should be a directory, not a file.
🛡️ Suggested fix to verify directory
// Verify remote path exists via SFTP
try {
const sftp = await sshManager.getSftpClient(input.sshHostId);
- await new Promise<void>((resolve, reject) => {
+ await new Promise<void>((resolve, reject) => {
sftp.stat(input.remotePath, (err, stats) => {
if (err) {
reject(
new Error(
`Remote path "${input.remotePath}" does not exist or is not accessible`,
),
);
+ } else if ((stats.mode & 0o170000) !== 0o040000) {
+ reject(
+ new Error(
+ `Remote path "${input.remotePath}" is not a directory`,
+ ),
+ );
} else {
resolve();
}
});
});You'll need to add the stats parameter to the callback and check the file mode for directory type (S_IFDIR = 0o040000).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts` around
lines 1020 - 1040, The SFTP check currently only verifies existence; update the
sftp.stat callback used in the create workspace flow (sshManager.getSftpClient /
sftp.stat) to accept the second stats parameter and confirm the path is a
directory before resolving. In the callback for sftp.stat(input.remotePath, ...)
inspect stats: prefer calling stats.isDirectory() if available, otherwise check
the mode bits (e.g. test (stats.mode & 0o170000) === 0o040000) and reject with
an error if it is not a directory; keep the same error handling around the
try/catch that wraps sshManager.getSftpClient and the Promise.
Comment on lines
+33
to
+37
| for (const block of blocks) { | ||
| // Skip wildcard hosts | ||
| if (block.host === "*" || block.host.includes("*")) { | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Split multi-pattern Host lines before importing.
Host foo bar is valid SSH config, but this stores foo bar as one host value and later imports it as a single alias/hostname. It also causes Host foo *.corp to be dropped completely because the combined string contains *.
Also applies to: 86-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/ssh/ssh-config-parser.ts` around lines 33 - 37, The
parser currently treats block.host as a single string (in the loop over "for
(const block of blocks)") which leaves multi-pattern Host lines like "Host foo
bar" as one value and causes patterns containing "*" (e.g., "Host foo *.corp")
to be dropped; update the logic to split block.host on whitespace into
individual patterns, then iterate each pattern: for each pattern skip if pattern
=== "*" or pattern.includes("*"), otherwise handle/import that single pattern
(same handling currently applied to block.host). Apply the same
split-and-iterate fix to the other handling site referenced around the code that
mirrors lines 86-90.
Comment on lines
+55
to
+60
| const handleConnect = () => { | ||
| if (!sshHostId) return; | ||
| setIsConnecting(true); | ||
| void connectMutation | ||
| .mutateAsync({ id: sshHostId, password: password || undefined }) | ||
| .finally(() => { |
Contributor
There was a problem hiding this comment.
Route encrypted-key secrets through passphrase.
This handler always sends the input as password, but sshHosts.connect expects passphrase for authMethod: "privateKey". Encrypted private-key hosts will fail from this form even though the placeholder says passphrases are supported.
🩹 Suggested fix
const handleConnect = () => {
if (!sshHostId) return;
+ const secret = password || undefined;
setIsConnecting(true);
void connectMutation
- .mutateAsync({ id: sshHostId, password: password || undefined })
+ .mutateAsync({
+ id: sshHostId,
+ password: selectedHost?.authMethod === "password" ? secret : undefined,
+ passphrase:
+ selectedHost?.authMethod === "privateKey" ? secret : undefined,
+ })
.finally(() => {
setIsConnecting(false);
});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/NewWorkspaceModal/components/SshRemoteGroup/SshRemoteGroup.tsx`
around lines 55 - 60, The handler handleConnect currently always sends the input
as password to connectMutation.mutateAsync, but for hosts with authMethod ===
"privateKey" the API expects the secret under passphrase; update handleConnect
so it looks up the target host's authMethod (e.g., from sshHost or selected SSH
host object) and, when authMethod === "privateKey", call
connectMutation.mutateAsync with { id: sshHostId, passphrase: password ||
undefined } instead of password, otherwise continue to send { id: sshHostId,
password: password || undefined }; make this change in the handleConnect
function where connectMutation.mutateAsync is invoked.
Comment on lines
+28
to
29
| type: "worktree" | "branch" | "remote"; | ||
| isActive: boolean; |
Contributor
There was a problem hiding this comment.
Avoid worktree-specific menu copy for remote items.
With remote now falling through the shared non-branch context menu, the destructive action below still renders as Close Worktree. Remote workspaces need neutral copy there.
Also applies to: 57-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx`
around lines 28 - 29, The context menu in CollapsedWorkspaceItem currently uses
worktree-specific copy ("Close Worktree") for non-branch items, which results in
incorrect copy for type === "remote"; update the destructive-action label
generation in CollapsedWorkspaceItem (the branch/non-branch menu path) to choose
neutral wording for remote items (e.g., "Remove Workspace" or "Disconnect
Remote") instead of "Close Worktree" — implement a small conditional on the
workspace type (the union "worktree" | "branch" | "remote") when building the
menu so branch keeps branch-specific labels, worktree keeps "Close Worktree",
and remote receives neutral copy.
Comment on lines
23
to
27
| interface DeleteWorkspaceDialogProps { | ||
| workspaceId: string; | ||
| workspaceName: string; | ||
| workspaceType?: "worktree" | "branch"; | ||
| workspaceType?: "worktree" | "branch" | "remote"; | ||
| open: boolean; |
Contributor
There was a problem hiding this comment.
"remote" needs its own delete path.
Adding "remote" to workspaceType makes remote workspaces fall through the same branch/worktree logic as local worktrees. That path still loads the "delete local branch" preference, shows worktree-specific copy/tooltips, and forwards deleteLocalBranch into the mutation, so the SSH delete dialog is misleading at best and wrong at worst.
💡 Minimal guard to keep remote workspaces out of the worktree-only controls
const { data: deleteLocalBranchDefault } =
electronTrpc.settings.getDeleteLocalBranch.useQuery(undefined, {
- enabled: open && !isBranch,
+ enabled: open && workspaceType === "worktree",
}); const handleDelete = async () => {
onOpenChange(false);
- setDeleteLocalBranchSetting.mutate({
- enabled: deleteLocalBranchChecked,
- });
+ if (workspaceType === "worktree") {
+ setDeleteLocalBranchSetting.mutate({
+ enabled: deleteLocalBranchChecked,
+ });
+ }
await deleteWithToast({
name: workspaceName,
deleteFn: () =>
deleteWorkspace.mutateAsync({
id: workspaceId,
- deleteLocalBranch: deleteLocalBranchChecked,
+ deleteLocalBranch:
+ workspaceType === "worktree" ? deleteLocalBranchChecked : false,
}),
forceDeleteFn: () =>
deleteWorkspace.mutateAsync({
id: workspaceId,
- deleteLocalBranch: deleteLocalBranchChecked,
+ deleteLocalBranch:
+ workspaceType === "worktree" ? deleteLocalBranchChecked : false,
force: true,
}),
});
};- {!isLoading && canDelete && (
+ {!isLoading && canDelete && workspaceType === "worktree" && (
<div className="px-4 pb-2">
...
</div>
)}The description and tooltip strings should also be split so remote workspaces stop referring to git worktrees and local branches.
Also applies to: 44-47, 105-115, 191-235, 257-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/components/DeleteWorkspaceDialog/DeleteWorkspaceDialog.tsx`
around lines 23 - 27, DeleteWorkspaceDialog currently treats "remote" like local
worktrees; update DeleteWorkspaceDialogProps handling so workspaceType
explicitly branches for "remote" (separate from "worktree" and "branch") and
avoid rendering any worktree/local-branch controls when workspaceType ===
"remote". Specifically: in the DeleteWorkspaceDialog component, guard UI pieces
that reference deleteLocalBranch, worktree copy/tooltips, and preference-loading
logic so they only appear for "worktree" or "branch"; do not forward
deleteLocalBranch into the delete mutation when workspaceType === "remote"
(adjust the mutation call, e.g., in the onConfirm/handleDelete code path that
calls deleteWorkspaceMutation or similar). Also split/replace description and
tooltip strings into distinct remote-specific text and use those when
workspaceType === "remote" so the dialog copy no longer mentions worktrees/local
branches. Ensure all checks reference the workspaceType enum value strings
("worktree" | "branch" | "remote") and update any props/state names
(DeleteWorkspaceDialogProps, deleteLocalBranch) accordingly.
| ); | ||
| --> statement-breakpoint | ||
| CREATE INDEX `ssh_hosts_hostname_idx` ON `ssh_hosts` (`hostname`);--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `ssh_host_id` text REFERENCES ssh_hosts(id);--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's application-layer handling for SSH host deletion cascading to workspaces
rg -n "sshHostId.*null|ssh_host_id.*null|onDelete.*sshHosts" --type ts apps/desktop/src/lib/trpc/routers/ssh-hosts/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
cat -n packages/local-db/drizzle/0036_add_ssh_remote_workspaces.sqlRepository: superset-sh/superset
Length of output: 751
🏁 Script executed:
# Find the schema definition file
fd -e ts -e js schema packages/local-db/src/ | grep -E "(schema|index)" | head -10Repository: superset-sh/superset
Length of output: 102
🏁 Script executed:
# Search for ssh_hosts schema definition
rg -n "ssh_hosts|sshHosts" packages/local-db/src/schema/ -A 5 -B 2Repository: superset-sh/superset
Length of output: 2077
🏁 Script executed:
# Search for workspaces schema and its relationships
rg -n "workspaces" packages/local-db/src/schema/ -A 3 -B 1Repository: superset-sh/superset
Length of output: 4762
🏁 Script executed:
# Check if sshHosts is included in relations
cat -n packages/local-db/src/schema/relations.ts | grep -A 10 -B 2 "sshHosts\|ssh"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for SSH host deletion or removal handlers
rg -n "deleteSshHost|removeSshHost|sshHost.*delete|ssh.*delete" --type ts apps/desktop/src/ -iRepository: superset-sh/superset
Length of output: 246
🏁 Script executed:
# Search for any SSH host router/handlers
fd -type f -name "*ssh*" apps/desktop/src/lib/trpc/routers/Repository: superset-sh/superset
Length of output: 234
🏁 Script executed:
# Check the entire schema.ts file around the sshHostId definition to see if there are comments
sed -n '150,165p' packages/local-db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 731
🏁 Script executed:
# Check if there's a sshHosts relation definition
rg -n "sshHosts" packages/local-db/src/schema/relations.ts -A 5 -B 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the full delete handler for SSH hosts
rg -n "delete.*sshHost|sshHosts\.delete" apps/desktop/src/lib/trpc/routers/ -A 10 -B 2 --type tsRepository: superset-sh/superset
Length of output: 1083
🏁 Script executed:
# Search for the SSH hosts router file
fd "ssh" apps/desktop/src/lib/trpc/routers/Repository: superset-sh/superset
Length of output: 108
🏁 Script executed:
# Check if Drizzle migration generates ON DELETE for ALTER TABLE
# Look for patterns in other migrations that might show how Drizzle handles this
rg -n "ALTER TABLE.*ADD.*REFERENCES" packages/local-db/drizzle/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 1146
🏁 Script executed:
# Check the full delete handler for SSH hosts to see if it handles cascading
cat -n apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts | sed -n '75,105p'Repository: superset-sh/superset
Length of output: 1063
🏁 Script executed:
# Check if there's any logic to clean up workspaces when SSH host is deleted
rg -n "sshHostId.*null|ssh_host_id.*null" apps/desktop/src/ --type tsRepository: superset-sh/superset
Length of output: 742
🏁 Script executed:
# Look at the complete relations definition
cat -n packages/local-db/src/schema/relations.tsRepository: superset-sh/superset
Length of output: 1490
🏁 Script executed:
# Check if there's any application-layer cascading for workspace cleanup when SSH host deleted
rg -n "onDelete|cascade|sshHostId" apps/desktop/src/lib/trpc/routers/ssh-hosts/ -B 3 -A 3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Compare with section_id migration to see the pattern
cat -n packages/local-db/drizzle/0035_add_workspace_sections.sqlRepository: superset-sh/superset
Length of output: 813
🏁 Script executed:
# Check if Drizzle is supposed to generate ON DELETE in ALTER TABLE
# Look at the schema.ts comments around the sectionId field to understand the pattern
sed -n '145,165p' packages/local-db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 1054
🏁 Script executed:
# Verify if there's a way to add FK with ON DELETE after table creation in SQLite
# Check if there are any other migrations that show how to add FK constraints with ON DELETE behavior
rg -n "ON DELETE" packages/local-db/drizzle/ -B 3 -A 1Repository: superset-sh/superset
Length of output: 2975
🏁 Script executed:
# Check what happens when SSH host is deleted - does the app handle cleanup of workspaces?
rg -n "deleteHost\|sshHostId.*null" apps/desktop/src/lib/trpc/routers/ssh-hosts/ -B 5 -A 10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# See if there's a pattern in how the app handles section deletion (sectionId is similar)
rg -n "workspaceSections.*delete\|section.*delete" apps/desktop/src/lib/trpc/routers/workspace-sections/ -B 5 -A 10 --type tsRepository: superset-sh/superset
Length of output: 157
🏁 Script executed:
# Check if there's any workspace cleanup logic triggered on SSH host deletion
rg -n "sshHostId" apps/desktop/src/lib/trpc/routers/ -B 5 -A 5Repository: superset-sh/superset
Length of output: 12150
🏁 Script executed:
# Check if the sectionId has the same pattern and how it's handled
rg -n "sectionId.*null" apps/desktop/src/lib/trpc/routers/ -B 3 -A 3Repository: superset-sh/superset
Length of output: 11738
🏁 Script executed:
# Verify if FK enforcement is enabled in SQLite for this app
rg -n "PRAGMA foreign_keys" packages/local-db/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if foreign key enforcement is enabled in the database initialization
rg -n "PRAGMA foreign_keys\|foreign_keys" packages/local-db/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any cleanup logic for SSH host deletion that we may have missed
rg -n "sshHostId\|ssh_host_id" apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts -B 5 -A 10Repository: superset-sh/superset
Length of output: 46
Add application-layer cleanup for ssh_host_id or document the cascading strategy.
SQLite's ALTER TABLE ADD COLUMN doesn't support ON DELETE in the REFERENCES syntax, so the onDelete: "set null" in the schema definition won't be enforced at the database level. Unlike sectionId, which has explicit application-layer cleanup (lines in sections.ts:87 set workspaces' sectionId to null when a section is deleted), there's no equivalent cleanup for sshHostId in the SSH hosts deletion handler. Either add the cleanup logic or document why FK cascading is not necessary for this feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/local-db/drizzle/0036_add_ssh_remote_workspaces.sql` at line 15, The
migration adds workspaces.ssh_host_id referencing ssh_hosts but SQLite won't
enforce ON DELETE SET NULL, so update the SSH host deletion flow to perform
application-layer cleanup: in the SSH hosts deletion handler (the function that
deletes from ssh_hosts), add a DB update to set workspaces.ssh_host_id = NULL
for rows referencing the deleted ssh_host id (mirror the pattern used for
sectionId cleanup in sections.ts around the section deletion code), or
alternatively document the chosen cascading strategy in the SSH host deletion
docs if you intentionally rely on application-level guarantees.
Comment on lines
+637
to
+692
| async function pollOnce(): Promise<void> { | ||
| if (closed) return; | ||
| try { | ||
| const result = await execCommand( | ||
| `find ${shellEscape(rootPath)} -maxdepth 5 -printf '%p %T@\\n' 2>/dev/null | sort`, | ||
| ); | ||
| const current: Map<string, string> = new Map(); | ||
| for (const line of result.stdout.split("\n").filter(Boolean)) { | ||
| const spaceIdx = line.lastIndexOf(" "); | ||
| if (spaceIdx === -1) continue; | ||
| const p = line.slice(0, spaceIdx); | ||
| const t = line.slice(spaceIdx + 1); | ||
| current.set(p, t); | ||
| } | ||
|
|
||
| if (lastStats.size > 0) { | ||
| for (const [p, t] of current) { | ||
| if (!lastStats.has(p)) { | ||
| revision++; | ||
| push({ | ||
| type: "create", | ||
| workspaceId, | ||
| absolutePath: p, | ||
| isDirectory: false, | ||
| revision, | ||
| }); | ||
| } else if (lastStats.get(p) !== t) { | ||
| revision++; | ||
| push({ | ||
| type: "update", | ||
| workspaceId, | ||
| absolutePath: p, | ||
| isDirectory: false, | ||
| revision, | ||
| }); | ||
| } | ||
| } | ||
| for (const p of lastStats.keys()) { | ||
| if (!current.has(p)) { | ||
| revision++; | ||
| push({ | ||
| type: "delete", | ||
| workspaceId, | ||
| absolutePath: p, | ||
| isDirectory: false, | ||
| revision, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| lastStats = current; | ||
| } catch { | ||
| // Ignore transient poll errors | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
GNU-specific find -printf will fail silently on BSD/macOS remote hosts.
The -printf option is a GNU find extension not available in BSD or macOS find. When connecting to macOS or BSD remote hosts, this command will fail (silently, due to 2>/dev/null), resulting in no file change detection.
Additionally, isDirectory is hardcoded to false for all watch events (lines 660, 670, 680), which is inaccurate.
🛠️ Suggested portable alternative
Consider using stat format strings that work cross-platform, or fall back to ls -lR:
-const result = await execCommand(
- `find ${shellEscape(rootPath)} -maxdepth 5 -printf '%p %T@\\n' 2>/dev/null | sort`,
-);
+// Use stat -c on Linux, stat -f on BSD/macOS
+const result = await execCommand(
+ `find ${shellEscape(rootPath)} -maxdepth 5 -exec stat -c '%n %Y %F' {} \\; 2>/dev/null || ` +
+ `find ${shellEscape(rootPath)} -maxdepth 5 -exec stat -f '%N %m %HT' {} \\; 2>/dev/null`,
+);Alternatively, detect the remote OS on connection and select the appropriate command format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workspace-fs/src/ssh/service.ts` around lines 637 - 692, pollOnce
uses a GNU-only `find ... -printf` and silences errors, and it also hardcodes
isDirectory: false for all events; update pollOnce to use a portable remote
listing (e.g., run a cross-platform command per host like `find ... -type f
-print0 -exec stat -f "%N %m %HT" "%@" \;` on BSD/macOS or `find ... -printf` on
GNU, or fallback to `ls -lR`/`stat` output) by detecting the remote OS at
connection time or trying a GNU-safe command and falling back, parse the output
into a Map of path→(mtime,type) instead of only mtime, and when generating
events via push (in pollOnce) set isDirectory based on the parsed type (or by
running `test -d <path>`/stat mode) so create/update/delete events correctly
reflect directories vs files; use execCommand and the existing lastStats/current
maps and revision/push logic to replace the current parsing and hardcoded
isDirectory behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
19 issues found across 49 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:427">
P2: Remote workspace metadata is not propagated to `WorkspaceListItem`, so the new connection badge/host-path subtitle logic won't render for actual remote items.</violation>
</file>
<file name="apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts">
<violation number="1" location="apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts:93">
P1: The close handler retries even while already reconnecting, which can schedule duplicate reconnect timers and burn retry attempts for a single disconnect.</violation>
<violation number="2" location="apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts:480">
P1: Reconnect attempts drop password/passphrase credentials, so password-based (or passphrase-protected key) hosts fail after the first disconnect.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/CommandPalette/useCommandPalette.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/CommandPalette/useCommandPalette.ts:71">
P2: Including `remote` workspaces here causes global search to drop results when different SSH hosts use the same remote path, because multi-root search dedupes by `rootPath` only.</violation>
</file>
<file name="apps/desktop/src/main/lib/workspace-runtime/registry.ts">
<violation number="1" location="apps/desktop/src/main/lib/workspace-runtime/registry.ts:53">
P2: Set `hydrated` only after the remote workspace query succeeds; currently a failed first query can permanently disable remote runtime hydration.</violation>
</file>
<file name="apps/desktop/src/main/lib/ssh/ssh-config-parser.ts">
<violation number="1" location="apps/desktop/src/main/lib/ssh/ssh-config-parser.ts:25">
P2: Handle only missing-file reads silently and log other read failures instead of swallowing every error.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/main/lib/ssh/ssh-config-parser.ts:75">
P1: Parse directive separators as generic whitespace (not only a literal space), otherwise tab-indented SSH config entries are skipped.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx:200">
P2: Handle save failures in the click path (or inside save handlers) to avoid unhandled promise rejections when `mutateAsync` fails.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/ssh-hosts/components/SshHostsSettings/SshHostsSettings.tsx:397">
P2: Clear `testResult` when switching to edit a different host to prevent stale connection-test messages from leaking between forms.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/CollapsedWorkspaceItem.tsx:28">
P2: Adding `remote` here routes SSH workspaces through worktree-specific close/delete flows, so the UI exposes incorrect actions like “Close Worktree” and local-branch deletion options.</violation>
</file>
<file name="apps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.ts">
<violation number="1" location="apps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.ts:91">
P2: Register the pane only after `openShell` succeeds (or roll it back on failure) to avoid stale workspace mappings when attach fails.</violation>
<violation number="2" location="apps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.ts:94">
P1: `createOrAttach` is missing required in-flight deduplication, so concurrent attaches can open multiple SSH shells for the same pane.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts:142">
P1: Do not coerce a missing SSH exit code to `0`; it can falsely report failed remote commands as successful.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts:298">
P1: Use POSIX joining for remote workspace paths; `path.join` can generate Windows backslash paths that break remote SFTP operations.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
<violation number="3" location="apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts:311">
P1: Directory creation should also use POSIX path joining for remote workspaces; current joining can emit invalid remote paths on Windows.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:1024">
P2: Validate that `remotePath` is a directory (not just an existing path) before creating the remote workspace.</violation>
<violation number="2" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:1045">
P2: Deriving the remote workspace name with `split("/")` is path-separator fragile. Handle both `/` and `\\` so Windows-style SSH paths are named correctly.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
<violation number="3" location="apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts:1080">
P3: Avoid empty catch blocks for async operations. Log a warning with context so auto-rename failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts:85">
P2: Delete should disconnect unconditionally; gating on `isConnected()` misses `connecting`/`reconnecting` states and can leak active SSH state.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| client.on("close", () => { | ||
| const info = this.connections.get(config.id); | ||
| if (info?.state === "connected" || info?.state === "reconnecting") { |
Contributor
There was a problem hiding this comment.
P1: The close handler retries even while already reconnecting, which can schedule duplicate reconnect timers and burn retry attempts for a single disconnect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts, line 93:
<comment>The close handler retries even while already reconnecting, which can schedule duplicate reconnect timers and burn retry attempts for a single disconnect.</comment>
<file context>
@@ -0,0 +1,542 @@
+
+ client.on("close", () => {
+ const info = this.connections.get(config.id);
+ if (info?.state === "connected" || info?.state === "reconnecting") {
+ this.handleDisconnect(config);
+ }
</file context>
|
|
||
| const timer = setTimeout(() => { | ||
| this.reconnectTimers.delete(config.id); | ||
| this.connect(config).catch((err: Error) => { |
Contributor
There was a problem hiding this comment.
P1: Reconnect attempts drop password/passphrase credentials, so password-based (or passphrase-protected key) hosts fail after the first disconnect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/ssh/ssh-connection-manager.ts, line 480:
<comment>Reconnect attempts drop password/passphrase credentials, so password-based (or passphrase-protected key) hosts fail after the first disconnect.</comment>
<file context>
@@ -0,0 +1,542 @@
+
+ const timer = setTimeout(() => {
+ this.reconnectTimers.delete(config.id);
+ this.connect(config).catch((err: Error) => {
+ console.error(
+ `[SshConnectionManager] Reconnect failed for ${config.id}:`,
</file context>
| continue; | ||
| } | ||
|
|
||
| const spaceIdx = line.indexOf(" "); |
Contributor
There was a problem hiding this comment.
P1: Parse directive separators as generic whitespace (not only a literal space), otherwise tab-indented SSH config entries are skipped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/ssh/ssh-config-parser.ts, line 75:
<comment>Parse directive separators as generic whitespace (not only a literal space), otherwise tab-indented SSH config entries are skipped.</comment>
<file context>
@@ -0,0 +1,128 @@
+ continue;
+ }
+
+ const spaceIdx = line.indexOf(" ");
+ const eqIdx = line.indexOf("=");
+ let sepIdx: number;
</file context>
| this._registerPane(workspaceId, paneId); | ||
|
|
||
| // If already open (re-attach), just wire up listeners again | ||
| const existing = this.sshManager.getSession(paneId); |
Contributor
There was a problem hiding this comment.
P1: createOrAttach is missing required in-flight deduplication, so concurrent attaches can open multiple SSH shells for the same pane.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.ts, line 94:
<comment>`createOrAttach` is missing required in-flight deduplication, so concurrent attaches can open multiple SSH shells for the same pane.</comment>
<file context>
@@ -0,0 +1,393 @@
+ this._registerPane(workspaceId, paneId);
+
+ // If already open (re-attach), just wire up listeners again
+ const existing = this.sshManager.getSession(paneId);
+ if (!existing) {
+ await this.sshManager.openShell(this.hostId, paneId, {
</file context>
| workspaceId: input.workspaceId, | ||
| absolutePath: path.join(input.parentAbsolutePath, input.name), | ||
| }); | ||
| const result = await getWorkspaceFsService(input.workspaceId).createDirectory( |
Contributor
There was a problem hiding this comment.
P1: Directory creation should also use POSIX path joining for remote workspaces; current joining can emit invalid remote paths on Windows.
(Based on your team's feedback about using cross-platform path utilities instead of split.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspace-fs-service.ts, line 311:
<comment>Directory creation should also use POSIX path joining for remote workspaces; current joining can emit invalid remote paths on Windows.
(Based on your team's feedback about using cross-platform path utilities instead of split.) </comment>
<file context>
@@ -255,10 +308,12 @@ export async function createWorkspaceDirectory(input: {
- workspaceId: input.workspaceId,
- absolutePath: path.join(input.parentAbsolutePath, input.name),
- });
+ const result = await getWorkspaceFsService(input.workspaceId).createDirectory(
+ {
+ workspaceId: input.workspaceId,
</file context>
| const { paneId, workspaceId, cols = 80, rows = 24, cwd } = params; | ||
|
|
||
| // Register pane under workspace for workspace-scoped operations | ||
| this._registerPane(workspaceId, paneId); |
Contributor
There was a problem hiding this comment.
P2: Register the pane only after openShell succeeds (or roll it back on failure) to avoid stale workspace mappings when attach fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/workspace-runtime/ssh-terminal-runtime.ts, line 91:
<comment>Register the pane only after `openShell` succeeds (or roll it back on failure) to avoid stale workspace mappings when attach fails.</comment>
<file context>
@@ -0,0 +1,393 @@
+ const { paneId, workspaceId, cols = 80, rows = 24, cwd } = params;
+
+ // Register pane under workspace for workspace-scoped operations
+ this._registerPane(workspaceId, paneId);
+
+ // If already open (re-attach), just wire up listeners again
</file context>
| const maxTabOrder = getMaxProjectChildTabOrder(input.projectId); | ||
| const workspaceName = | ||
| input.name || | ||
| `${host.label}: ${input.remotePath.split("/").pop() || input.remotePath}`; |
Contributor
There was a problem hiding this comment.
P2: Deriving the remote workspace name with split("/") is path-separator fragile. Handle both / and \\ so Windows-style SSH paths are named correctly.
(Based on your team's feedback about using cross-platform path utilities instead of split.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts, line 1045:
<comment>Deriving the remote workspace name with `split("/")` is path-separator fragile. Handle both `/` and `\\` so Windows-style SSH paths are named correctly.
(Based on your team's feedback about using cross-platform path utilities instead of split.) </comment>
<file context>
@@ -971,6 +979,117 @@ export const createCreateProcedures = () => {
+ const maxTabOrder = getMaxProjectChildTabOrder(input.projectId);
+ const workspaceName =
+ input.name ||
+ `${host.label}: ${input.remotePath.split("/").pop() || input.remotePath}`;
+
+ const workspace = localDb
</file context>
| try { | ||
| const sftp = await sshManager.getSftpClient(input.sshHostId); | ||
| await new Promise<void>((resolve, reject) => { | ||
| sftp.stat(input.remotePath, (err) => { |
Contributor
There was a problem hiding this comment.
P2: Validate that remotePath is a directory (not just an existing path) before creating the remote workspace.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts, line 1024:
<comment>Validate that `remotePath` is a directory (not just an existing path) before creating the remote workspace.</comment>
<file context>
@@ -971,6 +979,117 @@ export const createCreateProcedures = () => {
+ try {
+ const sftp = await sshManager.getSftpClient(input.sshHostId);
+ await new Promise<void>((resolve, reject) => {
+ sftp.stat(input.remotePath, (err) => {
+ if (err) {
+ reject(
</file context>
| .input(z.object({ id: z.string() })) | ||
| .mutation(({ input }) => { | ||
| // Disconnect if connected | ||
| if (sshManager.isConnected(input.id)) { |
Contributor
There was a problem hiding this comment.
P2: Delete should disconnect unconditionally; gating on isConnected() misses connecting/reconnecting states and can leak active SSH state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/ssh-hosts/index.ts, line 85:
<comment>Delete should disconnect unconditionally; gating on `isConnected()` misses `connecting`/`reconnecting` states and can leak active SSH state.</comment>
<file context>
@@ -0,0 +1,253 @@
+ .input(z.object({ id: z.string() }))
+ .mutation(({ input }) => {
+ // Disconnect if connected
+ if (sshManager.isConnected(input.id)) {
+ sshManager.disconnect(input.id);
+ }
</file context>
| void attemptWorkspaceAutoRenameFromPrompt({ | ||
| workspaceId: workspace.id, | ||
| prompt: input.prompt, | ||
| }).catch(() => {}); |
Contributor
There was a problem hiding this comment.
P3: Avoid empty catch blocks for async operations. Log a warning with context so auto-rename failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts, line 1080:
<comment>Avoid empty catch blocks for async operations. Log a warning with context so auto-rename failures are observable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -971,6 +979,117 @@ export const createCreateProcedures = () => {
+ void attemptWorkspaceAutoRenameFromPrompt({
+ workspaceId: workspace.id,
+ prompt: input.prompt,
+ }).catch(() => {});
+ }
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add the ability to connect to remote SSH hosts and create workspaces where coding agents run on the remote machine instead of locally, similar to VS Code's Remote-SSH functionality.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Adds SSH Remote Workspaces so agents and terminals run on a remote machine over SSH. Includes host management, SFTP-backed file operations, and clear connection status in the UI.
New Features
~/.ssh/config; new DBssh_hostsplussshHostId/remotePathonworkspaces.WorkspaceFsServiceover SFTP via@superset/workspace-fs/ssh; SSH terminal/runtime through the registry.remoteworkspace type across lists, commands, and delete flows.Migration
0036_add_ssh_remote_workspaces(addsssh_hostsand workspace columns).ssh2and@types/ssh2(Electron build bundlesssh2).~/.ssh/configin Settings → SSH Hosts.remotePath.Written for commit 4e5ccc0. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features