Conversation
📝 WalkthroughWalkthroughAdds cloud-workspace design docs, DB enums/tables/migrations, a Freestyle cloud provider and factory, a cloudWorkspace tRPC router with provisioning/session/SSH flows, desktop collections/hooks/UI, and a desktop SSH terminal subsystem (sshManager, cloud-terminal router, and terminal UI). Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop as Desktop Client
participant TRPC as trpc Server
participant DB as Database
participant Provider as Freestyle Provider
Desktop->>TRPC: createCloudWorkspace(repoId,name,branch,settings)
TRPC->>DB: insert cloud_workspaces (status: provisioning)
TRPC->>Provider: createVM(params: repo, path, timeout, persistence)
Provider-->>TRPC: { vmId, status }
TRPC->>DB: update cloud_workspaces (provider_vm_id, status)
Desktop->>TRPC: getSSHCredentials(workspaceId)
TRPC->>DB: select cloud_workspaces (provider_vm_id)
TRPC->>Provider: getSSHCredentials(vmId)
Provider-->>TRPC: { host, port, username, token }
TRPC-->>Desktop: return SSH credentials
Desktop->>TRPC: cloudTerminal.createSession(paneId, credentials)
TRPC-->>Desktop: stream terminal data/events (observable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 4
🤖 Fix all issues with AI agents
In `@docs/CLOUD_WORKSPACE_IMPLEMENTATION.md`:
- Around line 15-19: Add the missing language identifier to the fenced code
block containing the environment variable example so markdownlint stops flagging
it; update the fenced block that shows FREESTYLE_API_KEY=your_api_key to use
```bash as the opening fence (i.e., change the code fence around the
FREESTYLE_API_KEY snippet to a bash code block).
- Around line 187-205: The “Implementation Tasks” section currently uses bold
text for subsection titles (which triggers MD036); update those bold labels to
proper Markdown headings (e.g., prepend with `#/`##/###) so they are treated as
headings instead of emphasized text—look for the “Implementation Tasks” block in
the same area as mapFreestyleStatus, CloudWorkspaceStatus and the Freestyle SDK
method list (freestyle.vms.create, freestyle.vms.suspend, freestyle.vms.start,
freestyle.vms.stop) and replace each bolded label with an appropriate heading
level, keeping the existing content/ordering intact.
In `@docs/CLOUD_WORKSPACE_PLAN.md`:
- Around line 156-159: Update the phrasing in the docs to use a spaced time unit
for readability: change the bullet text "Auto-pause after 30min idle" to
"Auto-pause after 30 min idle" (or "Auto-pause after 30 minutes idle") in the
CLOUD_WORKSPACE_PLAN.md content so it matches the document's style; locate and
edit the list item containing "Auto-pause after 30min idle" and replace with the
spaced form.
In `@docs/cloud-workspace-testing-plan.md`:
- Around line 219-227: The DELETE shown (DELETE FROM cloud_workspaces WHERE name
LIKE 'test-%') is too broad and may remove other orgs' data; update the cleanup
to scope by organization/repository (e.g., include org_id or repo_id filters) or
replace the example with a parameterized statement and an explicit warning;
reference the cloud_workspaces table and the DELETE statement in the docs and
show the safer pattern (add WHERE org_id = <ORG> AND name LIKE 'test-%' or use a
transaction/SELECT confirmation) so testers must scope cleanup to their org.
| ### Environment Variables Required | ||
| ``` | ||
| FREESTYLE_API_KEY=your_api_key | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Add a language identifier to the fenced block.
Markdownlint flags fenced code blocks without a language; add bash here.
📝 Suggested fix
-```
+```bash
FREESTYLE_API_KEY=your_api_key</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
### Environment Variables Required
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@docs/CLOUD_WORKSPACE_IMPLEMENTATION.md` around lines 15 - 19, Add the missing
language identifier to the fenced code block containing the environment variable
example so markdownlint stops flagging it; update the fenced block that shows
FREESTYLE_API_KEY=your_api_key to use ```bash as the opening fence (i.e., change
the code fence around the FREESTYLE_API_KEY snippet to a bash code block).
|
|
||
| private mapFreestyleStatus(status: string): CloudWorkspaceStatus { | ||
| const statusMap: Record<string, CloudWorkspaceStatus> = { | ||
| "running": "running", | ||
| "suspended": "paused", | ||
| "stopped": "stopped", | ||
| "starting": "provisioning", | ||
| "error": "error", | ||
| }; | ||
| return statusMap[status] ?? "error"; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Key Freestyle v2 SDK methods used:** | ||
| - `freestyle.vms.create()` - Create VM with git repo cloning | ||
| - `freestyle.vms.suspend()` - Pause VM (preserves state) | ||
| - `freestyle.vms.start()` - Resume suspended VM | ||
| - `freestyle.vms.stop()` - Graceful shutdown |
There was a problem hiding this comment.
Use headings instead of bold text for section titles.
The bold labels under “Implementation Tasks” read like headings; converting them avoids MD036 and improves structure.
📝 Suggested fix
-**Database**
+#### Database
...
-**Cloud Provider**
+#### Cloud Provider
...
-**tRPC Procedures**
+#### tRPC Procedures
...
-**Desktop App**
+#### Desktop App🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
189-189: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
194-194: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
200-200: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@docs/CLOUD_WORKSPACE_IMPLEMENTATION.md` around lines 187 - 205, The
“Implementation Tasks” section currently uses bold text for subsection titles
(which triggers MD036); update those bold labels to proper Markdown headings
(e.g., prepend with `#/`##/###) so they are treated as headings instead of
emphasized text—look for the “Implementation Tasks” block in the same area as
mapFreestyleStatus, CloudWorkspaceStatus and the Freestyle SDK method list
(freestyle.vms.create, freestyle.vms.suspend, freestyle.vms.start,
freestyle.vms.stop) and replace each bolded label with an appropriate heading
level, keeping the existing content/ordering intact.
| **Key behaviors:** | ||
| - Auto-resume on connect to paused workspace | ||
| - Auto-pause after 30min idle | ||
| - Stopped workspaces persist until deleted |
There was a problem hiding this comment.
Use a spaced unit for readability.
Consider “30 min” or “30 minutes” instead of “30min” for consistency.
🧰 Tools
🪛 LanguageTool
[grammar] ~158-~158: Ensure spelling is correct
Context: ... to paused workspace - Auto-pause after 30min idle - Stopped workspaces persist until...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In `@docs/CLOUD_WORKSPACE_PLAN.md` around lines 156 - 159, Update the phrasing in
the docs to use a spaced time unit for readability: change the bullet text
"Auto-pause after 30min idle" to "Auto-pause after 30 min idle" (or "Auto-pause
after 30 minutes idle") in the CLOUD_WORKSPACE_PLAN.md content so it matches the
document's style; locate and edit the list item containing "Auto-pause after
30min idle" and replace with the spaced form.
| ## Test Data Cleanup | ||
|
|
||
| After testing: | ||
| ```sql | ||
| -- Clean up test cloud workspaces | ||
| DELETE FROM cloud_workspaces WHERE name LIKE 'test-%'; | ||
|
|
||
| -- Or use Freestyle dashboard to delete test VMs | ||
| ``` |
There was a problem hiding this comment.
Scope the cleanup SQL to avoid cross‑org deletions.
DELETE FROM cloud_workspaces WHERE name LIKE 'test-%'; can wipe other teams’ test data in shared environments. Consider adding org/repo filters or a warning about scoped cleanup.
✅ Suggested tweak
--- a/docs/cloud-workspace-testing-plan.md
+++ b/docs/cloud-workspace-testing-plan.md
@@
-DELETE FROM cloud_workspaces WHERE name LIKE 'test-%';
+-- Prefer scoping by org/repo when in shared envs
+DELETE FROM cloud_workspaces
+WHERE name LIKE 'test-%'
+ AND organization_id = '<org-id>';📝 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.
| ## Test Data Cleanup | |
| After testing: | |
| ```sql | |
| -- Clean up test cloud workspaces | |
| DELETE FROM cloud_workspaces WHERE name LIKE 'test-%'; | |
| -- Or use Freestyle dashboard to delete test VMs | |
| ``` | |
| ## Test Data Cleanup | |
| After testing: |
🤖 Prompt for AI Agents
In `@docs/cloud-workspace-testing-plan.md` around lines 219 - 227, The DELETE
shown (DELETE FROM cloud_workspaces WHERE name LIKE 'test-%') is too broad and
may remove other orgs' data; update the cleanup to scope by
organization/repository (e.g., include org_id or repo_id filters) or replace the
example with a parameterized statement and an explicit warning; reference the
cloud_workspaces table and the DELETE statement in the docs and show the safer
pattern (add WHERE org_id = <ORG> AND name LIKE 'test-%' or use a
transaction/SELECT confirmation) so testers must scope cleanup to their org.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/trpc/src/lib/cloud-providers/freestyle-provider.ts`:
- Around line 80-111: stopVM currently calls vm.suspend() but returns
"stopped"—update stopVM to either call the proper stop API (if
freestyle.vms.stop or similar exists) and return "stopped", or if only suspend
exists make stopVM return "suspended" (or rename to pauseVM) and ensure
consistency with pauseVM/vm.suspend; for getVMStatus replace the blanket catch
in getVMStatus with targeted handling: call freestyle.vms.ref({ vmId
}).exec("echo ok") and on success return {status:"running"}, on errors inspect
the error (e.g. SDK-specific codes/messages or a NotFound/404) to return
"deleted" or "error", detect suspended-specific errors to return "suspended",
and rethrow or return an {status:"error"} for unexpected API failures so real
errors are not masked.
🧹 Nitpick comments (4)
packages/trpc/src/lib/cloud-providers/types.ts (1)
6-12: Consider clarifying authentication requirements.Both
privateKeyandtokenare optional, but SSH authentication typically requires at least one credential. Consider whether one should be required, or document the expected usage pattern where the provider determines which credential type to return.💡 Optional: Add discriminated union for credential types
export type SSHCredentials = { host: string; port: number; username: string; } & ( | { privateKey: string; token?: never } | { token: string; privateKey?: never } );packages/trpc/src/lib/cloud-providers/index.ts (1)
13-35: LGTM with minor suggestion.The factory function with caching is well-implemented. Consider adding prefixed console logging when throwing errors for better observability in production, per coding guidelines.
💡 Optional: Add error logging with context
case "fly": + console.error("[cloud-providers/getCloudProvider] Fly.io provider not yet implemented"); throw new Error("Fly.io provider not yet implemented"); default: + console.error(`[cloud-providers/getCloudProvider] Unknown provider type: ${type}`); throw new Error(`Unknown cloud provider: ${type}`);packages/db/src/schema/cloud-workspace.ts (1)
55-66: Hoist default literals to module-level constants. This keeps defaults centralized and avoids scattered magic values.♻️ Proposed refactor
+const DEFAULT_PROVIDER_TYPE = "freestyle" as const; +const DEFAULT_WORKSPACE_STATUS = "provisioning" as const; +const DEFAULT_AUTO_STOP_MINUTES = 30; +const DEFAULT_CLIENT_TYPE = "desktop" as const; ... - providerType: cloudProviderType("provider_type") - .notNull() - .default("freestyle"), + providerType: cloudProviderType("provider_type") + .notNull() + .default(DEFAULT_PROVIDER_TYPE), ... - status: cloudWorkspaceStatus().notNull().default("provisioning"), + status: cloudWorkspaceStatus().notNull().default(DEFAULT_WORKSPACE_STATUS), ... - autoStopMinutes: integer("auto_stop_minutes").notNull().default(30), + autoStopMinutes: integer("auto_stop_minutes") + .notNull() + .default(DEFAULT_AUTO_STOP_MINUTES), ... - clientType: cloudClientType("client_type").notNull().default("desktop"), + clientType: cloudClientType("client_type") + .notNull() + .default(DEFAULT_CLIENT_TYPE),As per coding guidelines, extract magic numbers and hardcoded defaults to named constants at the module top.
Also applies to: 104-105
packages/trpc/src/lib/cloud-providers/freestyle-provider.ts (1)
40-55: Extract Freestyle defaults/SSH endpoints into constants. Centralizing/workspace, idle timeout, and SSH host/port simplifies future changes.♻️ Proposed refactor
+const DEFAULT_WORKDIR = "/workspace"; +const DEFAULT_IDLE_TIMEOUT_SECONDS = 1800; +const SSH_HOST = "vm-ssh.freestyle.sh"; +const SSH_PORT = 22; ... - path: params.workdir ?? "/workspace", + path: params.workdir ?? DEFAULT_WORKDIR, ... - workdir: params.workdir ?? "/workspace", - idleTimeoutSeconds: params.idleTimeoutSeconds ?? 1800, // 30 min default + workdir: params.workdir ?? DEFAULT_WORKDIR, + idleTimeoutSeconds: + params.idleTimeoutSeconds ?? DEFAULT_IDLE_TIMEOUT_SECONDS, // 30 min default ... - host: "vm-ssh.freestyle.sh", - port: 22, + host: SSH_HOST, + port: SSH_PORT,As per coding guidelines, extract magic numbers and hardcoded values to named constants at the module top.
Also applies to: 134-136
| async stopVM(vmId: string): Promise<VMStatus> { | ||
| console.log("[cloud/freestyle] Stopping VM:", vmId); | ||
|
|
||
| // Freestyle doesn't have a separate "stop" - we use suspend for now | ||
| // which preserves state and allows quick resume | ||
| const vm = this.freestyle.vms.ref({ vmId }); | ||
| await vm.suspend(); | ||
|
|
||
| return { status: "stopped" }; | ||
| } | ||
|
|
||
| async deleteVM(vmId: string): Promise<void> { | ||
| console.log("[cloud/freestyle] Deleting VM:", vmId); | ||
|
|
||
| await this.freestyle.vms.delete({ vmId }); | ||
| } | ||
|
|
||
| async getVMStatus(vmId: string): Promise<VMStatus> { | ||
| console.log("[cloud/freestyle] Getting status for VM:", vmId); | ||
|
|
||
| // Get VM info - Freestyle SDK ref doesn't directly expose status | ||
| // We need to list and find, or use exec to check if it's running | ||
| try { | ||
| const vm = this.freestyle.vms.ref({ vmId }); | ||
| // Try a simple exec to check if VM is running | ||
| await vm.exec("echo ok"); | ||
| return { status: "running" }; | ||
| } catch (_error) { | ||
| // If exec fails, VM might be suspended or stopped | ||
| console.log("[cloud/freestyle] VM not running, may be paused:", vmId); | ||
| return { status: "paused" }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the target file
find . -name "freestyle-provider.ts" -type fRepository: superset-sh/superset
Length of output: 125
🏁 Script executed:
# Get an overview of the repository structure
git ls-files | grep -E "(freestyle|cloud-provider)" | head -20Repository: superset-sh/superset
Length of output: 217
🏁 Script executed:
# Check if file exists and examine the code around lines 80-111
if [ -f "packages/trpc/src/lib/cloud-providers/freestyle-provider.ts" ]; then
wc -l "packages/trpc/src/lib/cloud-providers/freestyle-provider.ts"
echo "---"
sed -n '75,115p' "packages/trpc/src/lib/cloud-providers/freestyle-provider.ts"
fiRepository: superset-sh/superset
Length of output: 1426
🏁 Script executed:
# Look for Freestyle SDK imports and usage patterns
rg -A 5 -B 5 "freestyle" packages/trpc/src/lib/cloud-providers/ --context=3Repository: superset-sh/superset
Length of output: 11225
🏁 Script executed:
# View the types file to understand VMStatus and CloudProviderInterface
cat -n packages/trpc/src/lib/cloud-providers/types.tsRepository: superset-sh/superset
Length of output: 1772
🏁 Script executed:
# Search for VMStatus usage across the codebase
rg "VMStatus" packages/trpc/src/lib/cloud-providers/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 3723
🏁 Script executed:
# Look for how stopVM vs pauseVM are used in the codebase
rg "stopVM|pauseVM" --include="*.ts" --include="*.tsx" -B 2 -A 2Repository: superset-sh/superset
Length of output: 505
🏁 Script executed:
# Find CloudWorkspaceStatus definition
rg "CloudWorkspaceStatus" -t ts -A 5 | head -50Repository: superset-sh/superset
Length of output: 3636
🏁 Script executed:
# Search for stopVM and pauseVM usage in the codebase
rg "stopVM\|pauseVM" -t ts -A 1 -B 1Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there are other cloud provider implementations to see patterns
find . -name "*-provider.ts" -type f | grep -E "cloud-providers"Repository: superset-sh/superset
Length of output: 125
🏁 Script executed:
# View the full freestyle-provider.ts to see the status mapping and how it's used
cat -n packages/trpc/src/lib/cloud-providers/freestyle-provider.tsRepository: superset-sh/superset
Length of output: 5001
🏁 Script executed:
# Search for "stopped" status in Freestyle SDK context
rg "stopped" packages/trpc/src/lib/cloud-providers/freestyle-provider.ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 456
Fix stopVM/getVMStatus semantic and error handling issues. The stopVM method calls vm.suspend() but returns "stopped" despite the status map showing Freestyle supports distinct "stopped" and "suspended" states—verify if a proper stop method exists. Additionally, getVMStatus conflates all exec failures (paused, stopped, deleted, API errors) into "paused", masking real errors; either distinguish failure types or return "error" status.
Suggested fix
For stopVM, if a dedicated stop method exists:
- await vm.suspend();
- return { status: "stopped" };
+ await vm.stop(); // if SDK supports this
+ return { status: "stopped" };Otherwise, align with pauseVM:
- return { status: "stopped" };
+ return { status: "paused" };For getVMStatus, improve error handling:
- } catch (_error) {
- console.log("[cloud/freestyle] VM not running, may be paused:", vmId);
- return { status: "paused" };
- }
+ } catch (error) {
+ console.log("[cloud/freestyle] VM status check failed:", vmId, error);
+ return { status: "error", message: "Unable to determine VM status" };
+ }🤖 Prompt for AI Agents
In `@packages/trpc/src/lib/cloud-providers/freestyle-provider.ts` around lines 80
- 111, stopVM currently calls vm.suspend() but returns "stopped"—update stopVM
to either call the proper stop API (if freestyle.vms.stop or similar exists) and
return "stopped", or if only suspend exists make stopVM return "suspended" (or
rename to pauseVM) and ensure consistency with pauseVM/vm.suspend; for
getVMStatus replace the blanket catch in getVMStatus with targeted handling:
call freestyle.vms.ref({ vmId }).exec("echo ok") and on success return
{status:"running"}, on errors inspect the error (e.g. SDK-specific
codes/messages or a NotFound/404) to return "deleted" or "error", detect
suspended-specific errors to return "suspended", and rethrow or return an
{status:"error"} for unexpected API failures so real errors are not masked.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Around line 183-226: The onInsert handler in the cloudWorkspaces
createCollection is omitting the client-generated id, causing Electric
optimistic rows to diverge from server-created rows; update the onInsert in
cloudWorkspaces (the function calling apiClient.cloudWorkspace.create.mutate) to
include the client item.id (or send the full item) in the create payload so the
backend returns/uses the same id (e.g., pass id: item.id along with
organizationId, repositoryId, name, branch, providerType, autoStopMinutes) and
return the result.txid as before.
In `@packages/trpc/src/router/cloud-workspace/cloud-workspace.ts`:
- Around line 29-109: The cloud-workspace procedures (list, get,
getSSHCredentials, getSessions and the other procedures noted) currently trust
organizationId/workspaceId inputs; add explicit authorization checks before any
DB access: implement or call an authorization helper (e.g.,
ensureUserInOrganization(userId, organizationId) and
ensureUserCanAccessWorkspace(userId, workspaceId) or similar) that verifies the
caller is a member of the organization or owner/has appropriate role for the
workspace, and use it at the start of list, get, getSSHCredentials, getSessions
(and the other procedures referenced) to throw a TRPCError("FORBIDDEN") if the
check fails; for workspace-level checks, load the workspace by id first, confirm
its organizationId matches a membership record for the caller (or that the
caller is the workspace owner), then proceed with the original logic.
- Around line 170-184: The mutation allows calling updateCloudWorkspaceSchema
with only workspaceId so data can be an empty object; before calling
dbWs.transaction in the update protectedProcedure mutation, add a guard that
checks Object.keys(data).length > 0 and throw a descriptive error (e.g., a
TRPCError with code 'BAD_REQUEST') or enforce via the schema (zod .refine) so
you never call .set({}) on cloudWorkspaces; place the check immediately after
const { workspaceId, ...data } = input to short-circuit and return a clear error
if no update fields were provided.
🧹 Nitpick comments (5)
packages/local-db/src/schema/relations.ts (1)
34-49: Consider wiring org/creator/user relations for completeness.
cloud_workspacesincludesorganization_id/creator_id, andcloud_workspace_sessionsincludesuser_id. If the local DB exposesorganizations/users, addingorganization,creator, anduserrelations will keep the relation graph consistent and enablewithqueries without manual joins.packages/trpc/src/router/cloud-workspace/schema.ts (1)
8-43: Extract validation bounds/defaults into named constants.This reduces scattered magic numbers and makes updates safer.
♻️ Suggested refactor
import { z } from "zod"; +const WORKSPACE_NAME_MIN = 1; +const WORKSPACE_NAME_MAX = 100; +const BRANCH_MIN = 1; +const AUTO_STOP_MINUTES_MIN = 5; +const AUTO_STOP_MINUTES_MAX = 480; +const AUTO_STOP_MINUTES_DEFAULT = 30; + // Create a new cloud workspace export const createCloudWorkspaceSchema = z.object({ organizationId: z.string().uuid(), repositoryId: z.string().uuid(), - name: z.string().min(1).max(100), - branch: z.string().min(1), + name: z.string().min(WORKSPACE_NAME_MIN).max(WORKSPACE_NAME_MAX), + branch: z.string().min(BRANCH_MIN), providerType: z.enum(cloudProviderTypeValues).default("freestyle"), - autoStopMinutes: z.number().int().min(5).max(480).default(30), // 5 min to 8 hours + autoStopMinutes: z + .number() + .int() + .min(AUTO_STOP_MINUTES_MIN) + .max(AUTO_STOP_MINUTES_MAX) + .default(AUTO_STOP_MINUTES_DEFAULT), // 5 min to 8 hours }); @@ export const updateCloudWorkspaceSchema = z.object({ workspaceId: z.string().uuid(), - name: z.string().min(1).max(100).optional(), - autoStopMinutes: z.number().int().min(5).max(480).optional(), + name: z.string().min(WORKSPACE_NAME_MIN).max(WORKSPACE_NAME_MAX).optional(), + autoStopMinutes: z + .number() + .int() + .min(AUTO_STOP_MINUTES_MIN) + .max(AUTO_STOP_MINUTES_MAX) + .optional(), });As per coding guidelines, ...
packages/trpc/src/router/cloud-workspace/cloud-workspace.ts (2)
492-505: Use the shared CloudProviderType instead of a literal union.This avoids drift when providers change and keeps the type aligned with DB enums.
♻️ Suggested refactor
import { cloudWorkspaceSessions, cloudWorkspaces, repositories, + type CloudProviderType, } from "@superset/db/schema"; @@ workspaceName, providerType, autoStopMinutes, }: { workspaceId: string; repoUrl: string; branch: string; workspaceName: string; - providerType: "freestyle" | "fly"; + providerType: CloudProviderType; autoStopMinutes: number; }) {
364-367: Align log prefixes with the[domain/operation]convention.This makes log streams easier to filter by operation.
♻️ Example adjustments
- console.error( - "[cloud-workspace] Failed to delete VM from provider:", - error, - ); + console.error( + "[cloud-workspace/delete] Failed to delete VM from provider:", + error, + ); @@ - console.log( - "[cloud-workspace] Starting async provisioning for:", - workspaceId, - ); + console.log( + "[cloud-workspace/provision] Starting async provisioning for:", + workspaceId, + ); @@ - console.log("[cloud-workspace] VM created:", vmId, "status:", status); + console.log("[cloud-workspace/provision] VM created:", vmId, "status:", status); @@ - console.log("[cloud-workspace] Provisioning complete for:", workspaceId); + console.log("[cloud-workspace/provision] Provisioning complete for:", workspaceId); @@ - console.error("[cloud-workspace] Provisioning failed:", error); + console.error("[cloud-workspace/provision] Provisioning failed:", error);As per coding guidelines, ...
Also applies to: 507-535
packages/local-db/src/schema/schema.ts (1)
285-362: Consider centralizing Cloud enum literals to prevent drift.*These unions are now defined locally; if backend enums evolve, the SQLite schema can silently diverge. Consider importing shared constants/enums from a common module to keep the values aligned long‑term.
| const cloudWorkspaces = createCollection( | ||
| electricCollectionOptions<SelectCloudWorkspace>({ | ||
| id: `cloud_workspaces-${organizationId}`, | ||
| shapeOptions: { | ||
| url: electricUrl, | ||
| params: { | ||
| table: "cloud_workspaces", | ||
| organizationId, | ||
| }, | ||
| headers, | ||
| columnMapper, | ||
| }, | ||
| getKey: (item) => item.id, | ||
| onInsert: async ({ transaction }) => { | ||
| const item = transaction.mutations[0].modified; | ||
| const result = await apiClient.cloudWorkspace.create.mutate({ | ||
| organizationId: item.organizationId, | ||
| repositoryId: item.repositoryId, | ||
| name: item.name, | ||
| branch: item.branch, | ||
| providerType: item.providerType, | ||
| autoStopMinutes: item.autoStopMinutes, | ||
| }); | ||
| return { txid: result.txid }; | ||
| }, | ||
| onUpdate: async ({ transaction }) => { | ||
| const { original, changes } = transaction.mutations[0]; | ||
| const result = await apiClient.cloudWorkspace.update.mutate({ | ||
| workspaceId: original.id, | ||
| ...changes, | ||
| }); | ||
| return { txid: result.txid }; | ||
| }, | ||
| onDelete: async ({ transaction }) => { | ||
| const item = transaction.mutations[0].original; | ||
| const result = await apiClient.cloudWorkspace.delete.mutate({ | ||
| workspaceId: item.id, | ||
| }); | ||
| return { txid: result.txid }; | ||
| }, | ||
| }), | ||
| ); | ||
|
|
||
| return { tasks, taskStatuses, repositories, members, users, cloudWorkspaces }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for cloudWorkspace router/mutation definitions
find . -type f -name "*.ts" -o -name "*.tsx" | xargs grep -l "cloudWorkspace" | grep -E "(routes|router)" | head -20Repository: superset-sh/superset
Length of output: 325
🏁 Script executed:
# Search for tRPC cloudWorkspace mutations
rg "cloudWorkspace.*create|\.create\s*:" packages -g '*.ts' -B 5 -A 10Repository: superset-sh/superset
Length of output: 1432
🏁 Script executed:
# Also search in apps directory
rg "cloudWorkspace" apps -g '*.ts' --type ts -l | grep -E "(route|router|mutation)" | head -10Repository: superset-sh/superset
Length of output: 156
🏁 Script executed:
# Look at the collections.ts file to see how tasks and repositories are structured
rg -n "createCollection" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -B 2 -A 30 | head -150Repository: superset-sh/superset
Length of output: 4605
🏁 Script executed:
# Get the full task create mutation definition
rg -n "create.*:" packages/trpc/src/router -A 15 | grep -A 15 "task"Repository: superset-sh/superset
Length of output: 2263
🏁 Script executed:
# Look at the cloud-workspace router definition
cat packages/trpc/src/router/cloud-workspace/cloud-workspace.tsRepository: superset-sh/superset
Length of output: 13439
🏁 Script executed:
# Check the schema file for cloudWorkspace input types
cat packages/trpc/src/router/cloud-workspace/schema.tsRepository: superset-sh/superset
Length of output: 1302
🏁 Script executed:
# Check the task schema for create input
rg -n "createTaskSchema" packages/trpc/src/router -A 20Repository: superset-sh/superset
Length of output: 4321
🏁 Script executed:
# Check the repository schema for create input
rg -n "repositoryRouter|create.*:" packages/trpc/src/router/repository -B 5 -A 15 | head -50Repository: superset-sh/superset
Length of output: 3491
🏁 Script executed:
# Check the cloudWorkspaces table schema
rg -n "cloudWorkspaces" packages/db/src/schema -A 30 | grep -A 30 "export const cloudWorkspaces"Repository: superset-sh/superset
Length of output: 4365
🏁 Script executed:
# Check how Electric generates ids on the client side for optimistic inserts
rg -n "electricCollectionOptions|transaction.mutations" apps/desktop/src/renderer -B 5 -A 15 | grep -A 20 "cloudWorkspaces"Repository: superset-sh/superset
Length of output: 3714
🏁 Script executed:
# Look for how the task item is constructed when passed to the create mutation
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | sed -n '75,108p'Repository: superset-sh/superset
Length of output: 1230
🏁 Script executed:
# Check if Electric's SelectTask type includes an id field
rg -n "SelectTask|SelectRepository|SelectCloudWorkspace" packages/db -A 5 | head -60Repository: superset-sh/superset
Length of output: 1914
Pass the client-generated id to avoid Electric sync duplication.
Line 198: Unlike tasks and repositories (which pass the entire item object), the cloudWorkspace create payload manually constructs a new object and omits the id. When the backend generates a new ID, Electric will sync a second row with that ID while the optimistic insert retains the original client ID, creating duplicate rows.
Pass the client ID to keep them synchronized:
Suggested fix
const result = await apiClient.cloudWorkspace.create.mutate({
+ id: item.id,
organizationId: item.organizationId,
repositoryId: item.repositoryId,
name: item.name,
branch: item.branch,
providerType: item.providerType,
autoStopMinutes: item.autoStopMinutes,
});🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`
around lines 183 - 226, The onInsert handler in the cloudWorkspaces
createCollection is omitting the client-generated id, causing Electric
optimistic rows to diverge from server-created rows; update the onInsert in
cloudWorkspaces (the function calling apiClient.cloudWorkspace.create.mutate) to
include the client item.id (or send the full item) in the create payload so the
backend returns/uses the same id (e.g., pass id: item.id along with
organizationId, repositoryId, name, branch, providerType, autoStopMinutes) and
return the result.txid as before.
| list: protectedProcedure | ||
| .input(listCloudWorkspacesSchema) | ||
| .query(async ({ input }) => { | ||
| return db | ||
| .select() | ||
| .from(cloudWorkspaces) | ||
| .where(eq(cloudWorkspaces.organizationId, input.organizationId)) | ||
| .orderBy(desc(cloudWorkspaces.createdAt)); | ||
| }), | ||
|
|
||
| /** | ||
| * Get a single cloud workspace by ID with relations | ||
| */ | ||
| get: protectedProcedure | ||
| .input(cloudWorkspaceIdSchema) | ||
| .query(async ({ input }) => { | ||
| const [workspace] = await db | ||
| .select() | ||
| .from(cloudWorkspaces) | ||
| .where(eq(cloudWorkspaces.id, input.workspaceId)) | ||
| .limit(1); | ||
|
|
||
| if (!workspace) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Cloud workspace not found", | ||
| }); | ||
| } | ||
|
|
||
| return workspace; | ||
| }), | ||
|
|
||
| /** | ||
| * Get SSH credentials for connecting to a cloud workspace | ||
| */ | ||
| getSSHCredentials: protectedProcedure | ||
| .input(cloudWorkspaceIdSchema) | ||
| .query(async ({ input }) => { | ||
| const [workspace] = await db | ||
| .select() | ||
| .from(cloudWorkspaces) | ||
| .where(eq(cloudWorkspaces.id, input.workspaceId)) | ||
| .limit(1); | ||
|
|
||
| if (!workspace) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: "Cloud workspace not found", | ||
| }); | ||
| } | ||
|
|
||
| if (workspace.status !== "running") { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: `Cannot get SSH credentials for workspace in "${workspace.status}" state. Workspace must be running.`, | ||
| }); | ||
| } | ||
|
|
||
| if (!workspace.providerVmId) { | ||
| throw new TRPCError({ | ||
| code: "BAD_REQUEST", | ||
| message: "Workspace has no VM ID. It may still be provisioning.", | ||
| }); | ||
| } | ||
|
|
||
| const provider = getCloudProvider(workspace.providerType); | ||
| return provider.getSSHCredentials(workspace.providerVmId); | ||
| }), | ||
|
|
||
| /** | ||
| * Get active sessions for a workspace | ||
| */ | ||
| getSessions: protectedProcedure | ||
| .input(cloudWorkspaceIdSchema) | ||
| .query(async ({ input }) => { | ||
| return db | ||
| .select() | ||
| .from(cloudWorkspaceSessions) | ||
| .where(eq(cloudWorkspaceSessions.workspaceId, input.workspaceId)); | ||
| }), | ||
|
|
There was a problem hiding this comment.
Enforce org/user authorization on all cloud-workspace procedures.
Every query/mutation trusts organizationId/workspaceId without verifying caller membership or ownership, so any authenticated user could read/modify others’ workspaces and fetch SSH credentials. Add access checks (e.g., membership/role validation or workspace ownership) before returning or mutating data.
Also applies to: 115-337, 389-483
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/cloud-workspace/cloud-workspace.ts` around lines 29
- 109, The cloud-workspace procedures (list, get, getSSHCredentials, getSessions
and the other procedures noted) currently trust organizationId/workspaceId
inputs; add explicit authorization checks before any DB access: implement or
call an authorization helper (e.g., ensureUserInOrganization(userId,
organizationId) and ensureUserCanAccessWorkspace(userId, workspaceId) or
similar) that verifies the caller is a member of the organization or owner/has
appropriate role for the workspace, and use it at the start of list, get,
getSSHCredentials, getSessions (and the other procedures referenced) to throw a
TRPCError("FORBIDDEN") if the check fails; for workspace-level checks, load the
workspace by id first, confirm its organizationId matches a membership record
for the caller (or that the caller is the workspace owner), then proceed with
the original logic.
| update: protectedProcedure | ||
| .input(updateCloudWorkspaceSchema) | ||
| .mutation(async ({ input }) => { | ||
| const { workspaceId, ...data } = input; | ||
|
|
||
| const result = await dbWs.transaction(async (tx) => { | ||
| const [workspace] = await tx | ||
| .update(cloudWorkspaces) | ||
| .set(data) | ||
| .where(eq(cloudWorkspaces.id, workspaceId)) | ||
| .returning(); | ||
|
|
||
| const txid = await getCurrentTxid(tx); | ||
| return { workspace, txid }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and check its basic structure
find . -path "*/packages/trpc/src/router/cloud-workspace/cloud-workspace.ts" -type fRepository: superset-sh/superset
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Look for the schema definition
find . -type f -name "*cloud-workspace*schema*" -o -name "*workspace*schema*" | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for updateCloudWorkspaceSchema definition
rg "updateCloudWorkspaceSchema" --type ts --type tsx -A 5 -B 2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for updateCloudWorkspaceSchema definition with corrected command
rg "updateCloudWorkspaceSchema" -A 5 -B 2Repository: superset-sh/superset
Length of output: 2026
🏁 Script executed:
#!/bin/bash
# Read the cloud-workspace.ts file to see the actual implementation
cat -n packages/trpc/src/router/cloud-workspace/cloud-workspace.ts | head -200Repository: superset-sh/superset
Length of output: 6407
🏁 Script executed:
#!/bin/bash
# Check other update patterns in the codebase to see how they handle similar situations
rg "\.set\(" -B 3 -A 1 | head -100Repository: superset-sh/superset
Length of output: 6529
🏁 Script executed:
#!/bin/bash
# Look for any existing guards against empty updates
rg "Object\.keys\(" -B 2 -A 2 | grep -A 5 -B 5 "\.set\|update"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for other Drizzle update patterns in the trpc directory
rg "\.update\(" packages/trpc -B 3 -A 3Repository: superset-sh/superset
Length of output: 9394
🏁 Script executed:
#!/bin/bash
# Search for Drizzle update patterns more broadly
rg "\.set\(" packages/ -l | head -10Repository: superset-sh/superset
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Check the actual mutation code to see if it returns anything when data is empty
rg "mutation.*input.*=>|\.update|\.set" packages/trpc/src/router -B 2 -A 5Repository: superset-sh/superset
Length of output: 29156
🏁 Script executed:
#!/bin/bash
# Look for tests related to update mutations with empty data
find packages/trpc -name "*.test.ts" -o -name "*.spec.ts" | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for any error handling or guards against empty updates
rg "Object\.keys.*length|entries.*length|Object\.values" packages/trpc -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the schema files for task, repository to see if they have same issue
cat packages/trpc/src/router/task/schema.ts | grep -A 20 "update"Repository: superset-sh/superset
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Check the repository update schema to confirm same issue
cat packages/trpc/src/router/repository/schema.ts | grep -A 15 "updateRepository"Repository: superset-sh/superset
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Check organization update schema
cat packages/trpc/src/router/organization/schema.ts | grep -A 15 "updateOrganization"Repository: superset-sh/superset
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Search for any existing guards or validation in mutation handlers
rg "Object\.entries|Object\.keys.*0|data.*length|Object\.assign" packages/trpc/src/router -A 2 -B 2 | head -50Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find all schema files in the trpc router
find packages/trpc/src/router -name "schema.ts" -type fRepository: superset-sh/superset
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Read the repository router file to find schema usage
head -50 packages/trpc/src/router/repository/repository.ts | grep -A 10 "updateRepository"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check organization router for update schema
head -50 packages/trpc/src/router/organization/organization.ts | grep -A 10 "updateOrganization"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for the update schema definitions inline in the router files
rg "z\.object.*name.*optional|z\.object.*id.*optional" packages/trpc/src/router -B 1 -A 10 | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look at the repository.ts router to see how update is defined
sed -n '1,100p' packages/trpc/src/router/repository/repository.tsRepository: superset-sh/superset
Length of output: 2462
🏁 Script executed:
#!/bin/bash
# Find the organization router and check its update schema
rg "update.*protectedProcedure" packages/trpc/src/router/organization/organization.ts -A 20 | head -50Repository: superset-sh/superset
Length of output: 1104
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about this pattern
rg "empty.*update|no.*fields|\.set\(\{\}\)" packages/trpc -B 3 -A 3Repository: superset-sh/superset
Length of output: 46
Guard against empty updates.
The schema allows all update fields to be optional, meaning clients can submit requests with only workspaceId, resulting in .set({}). This causes either an error or a misleading no-op. Require at least one update field.
🐛 Suggested guard
.mutation(async ({ input }) => {
const { workspaceId, ...data } = input;
+ if (Object.keys(data).length === 0) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "No update fields provided",
+ });
+ }
const result = await dbWs.transaction(async (tx) => {📝 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.
| update: protectedProcedure | |
| .input(updateCloudWorkspaceSchema) | |
| .mutation(async ({ input }) => { | |
| const { workspaceId, ...data } = input; | |
| const result = await dbWs.transaction(async (tx) => { | |
| const [workspace] = await tx | |
| .update(cloudWorkspaces) | |
| .set(data) | |
| .where(eq(cloudWorkspaces.id, workspaceId)) | |
| .returning(); | |
| const txid = await getCurrentTxid(tx); | |
| return { workspace, txid }; | |
| }); | |
| update: protectedProcedure | |
| .input(updateCloudWorkspaceSchema) | |
| .mutation(async ({ input }) => { | |
| const { workspaceId, ...data } = input; | |
| if (Object.keys(data).length === 0) { | |
| throw new TRPCError({ | |
| code: "BAD_REQUEST", | |
| message: "No update fields provided", | |
| }); | |
| } | |
| const result = await dbWs.transaction(async (tx) => { | |
| const [workspace] = await tx | |
| .update(cloudWorkspaces) | |
| .set(data) | |
| .where(eq(cloudWorkspaces.id, workspaceId)) | |
| .returning(); | |
| const txid = await getCurrentTxid(tx); | |
| return { workspace, txid }; | |
| }); |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/cloud-workspace/cloud-workspace.ts` around lines 170
- 184, The mutation allows calling updateCloudWorkspaceSchema with only
workspaceId so data can be an empty object; before calling dbWs.transaction in
the update protectedProcedure mutation, add a guard that checks
Object.keys(data).length > 0 and throw a descriptive error (e.g., a TRPCError
with code 'BAD_REQUEST') or enforce via the schema (zod .refine) so you never
call .set({}) on cloudWorkspaces; place the check immediately after const {
workspaceId, ...data } = input to short-circuit and return a clear error if no
update fields were provided.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/ssh-terminal/ssh-manager.ts`:
- Around line 81-92: The spawned SSH pty uses process.env.HOME for cwd which can
be undefined on Windows; update the pty.spawn call in ssh-manager.ts (the code
creating shell and ptyProcess via pty.spawn) to use a reliable home directory
fallback such as calling os.homedir() or falling back to process.env.USERPROFILE
before passing cwd, ensuring env also includes that TERM setting; locate the
block that defines shell and ptyProcess and replace the cwd reference with the
homedir fallback.
- Around line 146-170: The SSH args currently hardcode
UserKnownHostsFile=/dev/null which fails on Windows; in
buildSSHArgs(credentials: SSHCredentials) detect the platform (process.platform
=== 'win32') and use "NUL" on Windows and "/dev/null" otherwise (or omit the
UserKnownHostsFile option on Windows if preferred), so update the
UserKnownHostsFile argument generation to be platform-specific while keeping the
other SSH options and the existing handling of port and username@host.
In
`@apps/desktop/src/renderer/react-query/cloud-workspaces/useCloudWorkspaceMutations.ts`:
- Around line 40-109: The mutation catch blocks (pauseWorkspace.mutate,
resumeWorkspace.mutate, stopWorkspace.mutate, deleteWorkspace.mutate — and any
similar mutations) currently only show toasts; update each catch to also log the
failure with context (e.g., console.error or the app logger) including a
prefixed message, the action name, workspaceId, and the error object so the
error isn’t swallowed; keep the existing toast and onError behavior but add the
descriptive log call before/after the toast.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/CloudTerminal/CloudTerminal.tsx`:
- Around line 176-190: The cleanup races with async initTerminal: change the
effect to track and cancel in-progress initialization by creating an
AbortController or a mutable flag and pass it into initTerminal (or have
initTerminal set cleanupRef when ready); on unmount/cleanup, set the abort flag
and if a cleanup function is already stored in cleanupRef call it synchronously
before disposing xterm; also ensure any ResizeObserver is disconnected inside
that cleanup function (or before xterm.dispose()) and clear xtermRef,
fitAddonRef, isConnectedRef, and call setIsConnected(false) to avoid the
observer accessing a disposed terminal (update references: initTerminal,
cleanupPromise, cleanupRef/cleanupPromise, xterm, xtermRef, fitAddonRef,
ResizeObserver, isConnectedRef, setIsConnected).
- Around line 70-113: The connect callback currently depends on the entire
cloudWorkspace object which causes spurious changes; update connect (function
name connect) to remove cloudWorkspace from its dependency array and only
reference primitive values it needs (cloudWorkspaceId, paneId, createSession,
xtermRef), and in the effect that triggers connection (the useEffect that
currently depends on connect) stop depending on connect and instead depend on
cloudWorkspace?.status (or call connect() directly inside the effect when
cloudWorkspace?.status === "running"); ensure status checks use
cloudWorkspace?.status rather than the whole object so reconnection only happens
when status actually becomes "running".
In `@packages/trpc/src/router/cloud-workspace/cloud-workspace.ts`:
- Around line 530-545: The DB update currently forces status: "running" after
provider.createVM returns a status; instead use the returned status (or map it
into your internal states) when calling dbWs.update(cloudWorkspaces).set so you
don't prematurely mark a workspace running. Locate the createVM call
(provider.createVM) and the subsequent dbWs.update(cloudWorkspaces).set block,
replace the hardcoded "running" with either the provider `status` (or a mapped
value, e.g., map provider statuses to "provisioning" | "running" | "failed") and
ensure lastActiveAt remains correct; if you want to keep a conservative default,
set to "provisioning" until you observe a confirmed running state instead of
forcing "running".
♻️ Duplicate comments (6)
packages/trpc/src/router/cloud-workspace/cloud-workspace.ts (3)
46-112: Ensure org/user authorization on all workspaceId-based procedures.Access checks are still missing before DB/provider calls.
Also applies to: 205-398, 405-499
186-195: Reject empty update payloads before calling.set({}).Currently a request with only
workspaceIdresults in an empty update.🐛 Suggested guard
.mutation(async ({ input }) => { const { workspaceId, ...data } = input; + if (Object.keys(data).length === 0) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "No update fields provided", + }); + } const result = await dbWs.transaction(async (tx) => {
131-143: Validate repository ownership within the active organization.
repositoryIdis accepted without checking org/user ownership, which can allow cross-org workspace creation. Ensure the repo belongs to the active org (or the caller has access) before provisioning.🔒 Possible guard (adjust ownership field as needed)
- const [repository] = await db + const [repository] = await db .select() .from(repositories) - .where(eq(repositories.id, input.repositoryId)) + .where( + and( + eq(repositories.id, input.repositoryId), + eq(repositories.organizationId, organizationId), + ), + ) .limit(1);packages/trpc/src/lib/cloud-providers/freestyle-provider.ts (2)
81-90: Semantic mismatch:stopVMcallssuspend()but returns"stopped".The comment acknowledges Freestyle lacks a separate "stop" API, yet the method returns
"stopped"after callingvm.suspend(). This creates inconsistency withpauseVM(which also callssuspend()but returns"paused") and withFREESTYLE_STATUS_MAPwhich distinguishes betweensuspended → "paused"andstopped → "stopped".Either return
"paused"to align with the actual SDK operation, or document the intentional semantic difference.Suggested alignment
async stopVM(vmId: string): Promise<VMStatus> { console.log("[cloud/freestyle] Stopping VM:", vmId); // Freestyle doesn't have a separate "stop" - we use suspend for now // which preserves state and allows quick resume const vm = this.freestyle.vms.ref({ vmId }); await vm.suspend(); - return { status: "stopped" }; + return { status: "paused" }; // Align with actual suspend operation }
98-112: Error handling ingetVMStatusmasks failures as"paused".Catching all errors and returning
"paused"conflates distinct failure scenarios (VM deleted, API errors, authentication issues) with a suspended state. This can lead to incorrect UI state and confusing behavior.Suggested improvement
async getVMStatus(vmId: string): Promise<VMStatus> { console.log("[cloud/freestyle] Getting status for VM:", vmId); try { const vm = this.freestyle.vms.ref({ vmId }); await vm.exec("echo ok"); return { status: "running" }; - } catch (_error) { - // If exec fails, VM might be suspended or stopped - console.log("[cloud/freestyle] VM not running, may be paused:", vmId); - return { status: "paused" }; + } catch (error) { + console.log("[cloud/freestyle] VM status check failed:", vmId, error); + // TODO: Inspect error type to distinguish paused vs deleted vs error + return { status: "error" }; } }apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (1)
196-206: Pass the client-generatedidto avoid Electric sync duplication.The
onInserthandler omitsitem.idfrom the create payload, unlike thetasksandrepositoriescollections which pass the entireitem. This causes the server to generate a new ID, resulting in duplicate rows when Electric syncs.Suggested fix
onInsert: async ({ transaction }) => { const item = transaction.mutations[0].modified; const result = await apiClient.cloudWorkspace.create.mutate({ + id: item.id, repositoryId: item.repositoryId, name: item.name, branch: item.branch ?? undefined, providerType: item.providerType, autoStopMinutes: item.autoStopMinutes, }); return { txid: result.txid }; },
🧹 Nitpick comments (11)
apps/desktop/src/renderer/react-query/cloud-workspaces/useCloudWorkspaceMutations.ts (1)
14-26: Use a single params object formutateto avoid positional args.This keeps the API self-documenting and aligns with the repo guideline.
♻️ Proposed refactor
- mutate: async ( - params: { - name: string; - repositoryId: string; - branch?: string; - providerType?: "freestyle" | "fly"; - autoStopMinutes?: number; - }, - options?: { - onSuccess?: () => void; - onError?: (error: Error) => void; - }, - ) => { + mutate: async ({ + params, + onSuccess, + onError, + }: { + params: { + name: string; + repositoryId: string; + branch?: string; + providerType?: "freestyle" | "fly"; + autoStopMinutes?: number; + }; + onSuccess?: () => void; + onError?: (error: Error) => void; + }) => { setIsPending(true); try { await apiTrpcClient.cloudWorkspace.create.mutate({ repositoryId: params.repositoryId, name: params.name, branch: params.branch, providerType: params.providerType ?? "freestyle", autoStopMinutes: params.autoStopMinutes, }); toast.success("Cloud workspace created", { description: "Provisioning VM...", }); - options?.onSuccess?.(); + onSuccess?.(); } catch (error) { const err = error instanceof Error ? error : new Error("Failed to create"); toast.error("Failed to create cloud workspace", { description: err.message, }); - options?.onError?.(err); + onError?.(err); } finally { setIsPending(false); } },apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/CloudWorkspaceSection/CloudWorkspaceListItem.tsx (1)
167-173: Consider a confirmation step for Delete.Accidental deletes are easy from a menu; a confirm dialog would reduce risk.
packages/trpc/src/router/cloud-workspace/schema.ts (1)
8-14: Extract auto-stop bounds/default to named constants.Keeps constraints consistent and avoids magic numbers.
♻️ Suggested constants
+const AUTO_STOP_MINUTES_MIN = 5; +const AUTO_STOP_MINUTES_MAX = 480; +const AUTO_STOP_MINUTES_DEFAULT = 30; + export const createCloudWorkspaceSchema = z.object({ repositoryId: z.string().uuid(), name: z.string().min(1).max(100), branch: z.string().min(1).optional(), // Optional - uses repo default branch if not provided providerType: z.enum(cloudProviderTypeValues).default("freestyle"), - autoStopMinutes: z.number().int().min(5).max(480).default(30), // 5 min to 8 hours + autoStopMinutes: z + .number() + .int() + .min(AUTO_STOP_MINUTES_MIN) + .max(AUTO_STOP_MINUTES_MAX) + .default(AUTO_STOP_MINUTES_DEFAULT), // 5 min to 8 hours }); ... export const updateCloudWorkspaceSchema = z.object({ workspaceId: z.string().uuid(), name: z.string().min(1).max(100).optional(), - autoStopMinutes: z.number().int().min(5).max(480).optional(), + autoStopMinutes: z + .number() + .int() + .min(AUTO_STOP_MINUTES_MIN) + .max(AUTO_STOP_MINUTES_MAX) + .optional(), });Also applies to: 36-39
apps/desktop/src/main/lib/ssh-terminal/ssh-manager.ts (2)
1-5: Consider extracting magic numbers to named constants.There are several hardcoded values that would benefit from named constants for clarity and maintainability: cleanup delay (5000ms), SSH timeouts (30s), default terminal dimensions (80x24).
♻️ Suggested constants
import { EventEmitter } from "node:events"; import * as pty from "node-pty"; const DEBUG_SSH = process.env.SUPERSET_SSH_DEBUG === "1"; + +// Terminal defaults +const DEFAULT_COLS = 80; +const DEFAULT_ROWS = 24; + +// SSH connection settings +const SSH_CONNECT_TIMEOUT_SECONDS = 30; +const SSH_KEEPALIVE_INTERVAL_SECONDS = 30; +const SSH_KEEPALIVE_MAX_COUNT = 3; + +// Session cleanup delay after exit (ms) +const SESSION_CLEANUP_DELAY_MS = 5000;
58-66: Redundant condition:colsandrowsalways have values due to defaults.Since
colsdefaults to 80 androwsdefaults to 24 at destructuring (line 54-55), the conditioncols !== undefined && rows !== undefinedis always true. If the intent is to resize only when explicit dimensions are passed, check the originalparamsbefore destructuring.♻️ Suggested fix
async createSession( params: CreateSSHSessionParams, ): Promise<SSHSessionResult> { - const { - paneId, - cloudWorkspaceId, - credentials, - cols = 80, - rows = 24, - } = params; + const { paneId, cloudWorkspaceId, credentials } = params; + const cols = params.cols ?? 80; + const rows = params.rows ?? 24; // Return existing session if alive const existing = this.sessions.get(paneId); if (existing?.isAlive) { existing.lastActive = Date.now(); - if (cols !== undefined && rows !== undefined) { - this.resize({ paneId, cols, rows }); - } + // Resize to match requested dimensions + this.resize({ paneId, cols, rows }); return { paneId, isNew: false }; }apps/desktop/src/lib/trpc/routers/cloud-terminal/index.ts (2)
69-86: Silent error handling may hide issues from the caller.The
writeprocedure catches errors but doesn't throw or return an error status to the caller. The caller has no way to know the write failed except via the stream subscription. Consider returning a result or re-throwing after emitting.♻️ Suggested improvement
write: publicProcedure .input( z.object({ paneId: z.string(), data: z.string(), }), ) - .mutation(({ input }) => { + .mutation(({ input }): { success: boolean; error?: string } => { try { sshManager.write(input); + return { success: true }; } catch (error) { const message = error instanceof Error ? error.message : "Write failed"; if (message.includes("not found or not alive")) { sshManager.emit(`exit:${input.paneId}`, 0, 15); - return; + return { success: false, error: "Session not alive" }; } sshManager.emit(`error:${input.paneId}`, { error: message, code: "WRITE_FAILED", }); + return { success: false, error: message }; } }),
119-121: Consider using object input for consistency across procedures.
getSessionandstreamuse a bare string input while other procedures use object inputs. While single-param functions don't require object params per guidelines, using consistent patterns across the router improves API ergonomics.Also applies to: 135-137
packages/trpc/src/lib/cloud-providers/freestyle-provider.ts (2)
22-24: Unused helper function_mapFreestyleStatus.The
_mapFreestyleStatusfunction andFREESTYLE_STATUS_MAPconstant are defined but never used in this file. Consider either removing them or integrating them intogetVMStatusto provide accurate status mapping.
55-55: Extract magic number to a named constant.The idle timeout default of
1800seconds should be extracted to a named constant for clarity and maintainability.Suggested fix
+const DEFAULT_IDLE_TIMEOUT_SECONDS = 1800; // 30 minutes + // ... in createVM - idleTimeoutSeconds: params.idleTimeoutSeconds ?? 1800, // 30 min default + idleTimeoutSeconds: params.idleTimeoutSeconds ?? DEFAULT_IDLE_TIMEOUT_SECONDS,apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)
34-37: Placeholder handler with TODO - track for follow-up.The
handleConnectCloudWorkspacehandler is stubbed with a console log. The TODO indicates this needs implementation to open a cloud terminal tab.Would you like me to open an issue to track implementing the cloud workspace connection flow, or help generate the implementation that integrates with the SSH terminal subsystem mentioned in the PR summary?
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/CloudWorkspaceSection/CloudWorkspaceSection.tsx (1)
28-57: Collapsed mode cloud button creates "New Workspace" - verify UX intent.In collapsed mode, the main cloud button (lines 34-41) triggers
onNewWorkspace, while individualCloudWorkspaceListItementries handle workspace connections. This is reasonable, but the tooltip says "Cloud Workspaces" (line 44) which might suggest it would expand/list workspaces rather than create a new one.Consider clarifying the tooltip to "New Cloud Workspace" for consistency with the expanded mode's button tooltip.
Suggested tooltip clarity
- <TooltipContent side="right">Cloud Workspaces</TooltipContent> + <TooltipContent side="right">New Cloud Workspace</TooltipContent>
| // Spawn SSH process using node-pty | ||
| const shell = process.platform === "win32" ? "ssh.exe" : "ssh"; | ||
| const ptyProcess = pty.spawn(shell, sshArgs, { | ||
| name: "xterm-256color", | ||
| cols, | ||
| rows, | ||
| cwd: process.env.HOME, | ||
| env: { | ||
| ...process.env, | ||
| TERM: "xterm-256color", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
process.env.HOME may be undefined on Windows.
On Windows, the HOME environment variable is often not set. Consider using os.homedir() or providing a fallback.
🛠️ Suggested fix
+import * as os from "node:os";
+
// In createSession method:
const ptyProcess = pty.spawn(shell, sshArgs, {
name: "xterm-256color",
cols,
rows,
- cwd: process.env.HOME,
+ cwd: process.env.HOME ?? os.homedir(),
env: {
...process.env,
TERM: "xterm-256color",
},
});📝 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.
| // Spawn SSH process using node-pty | |
| const shell = process.platform === "win32" ? "ssh.exe" : "ssh"; | |
| const ptyProcess = pty.spawn(shell, sshArgs, { | |
| name: "xterm-256color", | |
| cols, | |
| rows, | |
| cwd: process.env.HOME, | |
| env: { | |
| ...process.env, | |
| TERM: "xterm-256color", | |
| }, | |
| }); | |
| import * as os from "node:os"; | |
| // Spawn SSH process using node-pty | |
| const shell = process.platform === "win32" ? "ssh.exe" : "ssh"; | |
| const ptyProcess = pty.spawn(shell, sshArgs, { | |
| name: "xterm-256color", | |
| cols, | |
| rows, | |
| cwd: process.env.HOME ?? os.homedir(), | |
| env: { | |
| ...process.env, | |
| TERM: "xterm-256color", | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/ssh-terminal/ssh-manager.ts` around lines 81 - 92,
The spawned SSH pty uses process.env.HOME for cwd which can be undefined on
Windows; update the pty.spawn call in ssh-manager.ts (the code creating shell
and ptyProcess via pty.spawn) to use a reliable home directory fallback such as
calling os.homedir() or falling back to process.env.USERPROFILE before passing
cwd, ensuring env also includes that TERM setting; locate the block that defines
shell and ptyProcess and replace the cwd reference with the homedir fallback.
| private buildSSHArgs(credentials: SSHCredentials): string[] { | ||
| const args: string[] = []; | ||
|
|
||
| // Disable strict host key checking for cloud VMs (they're ephemeral) | ||
| args.push("-o", "StrictHostKeyChecking=no"); | ||
| args.push("-o", "UserKnownHostsFile=/dev/null"); | ||
|
|
||
| // Set connection timeout | ||
| args.push("-o", "ConnectTimeout=30"); | ||
|
|
||
| // Keep connection alive | ||
| args.push("-o", "ServerAliveInterval=30"); | ||
| args.push("-o", "ServerAliveCountMax=3"); | ||
|
|
||
| // Port | ||
| if (credentials.port !== 22) { | ||
| args.push("-p", String(credentials.port)); | ||
| } | ||
|
|
||
| // User@host | ||
| // For Freestyle: username is "{vmId}:{token}" | ||
| args.push(`${credentials.username}@${credentials.host}`); | ||
|
|
||
| return args; | ||
| } |
There was a problem hiding this comment.
Cross-platform issue: /dev/null path is Unix-specific.
On Windows, /dev/null doesn't exist. This will cause SSH to fail or behave unexpectedly. Use NUL on Windows or handle this platform-specifically.
🛠️ Suggested fix for cross-platform compatibility
private buildSSHArgs(credentials: SSHCredentials): string[] {
const args: string[] = [];
+ const nullDevice = process.platform === "win32" ? "NUL" : "/dev/null";
+
// Disable strict host key checking for cloud VMs (they're ephemeral)
args.push("-o", "StrictHostKeyChecking=no");
- args.push("-o", "UserKnownHostsFile=/dev/null");
+ args.push("-o", `UserKnownHostsFile=${nullDevice}`);
// Set connection timeout
args.push("-o", "ConnectTimeout=30");📝 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.
| private buildSSHArgs(credentials: SSHCredentials): string[] { | |
| const args: string[] = []; | |
| // Disable strict host key checking for cloud VMs (they're ephemeral) | |
| args.push("-o", "StrictHostKeyChecking=no"); | |
| args.push("-o", "UserKnownHostsFile=/dev/null"); | |
| // Set connection timeout | |
| args.push("-o", "ConnectTimeout=30"); | |
| // Keep connection alive | |
| args.push("-o", "ServerAliveInterval=30"); | |
| args.push("-o", "ServerAliveCountMax=3"); | |
| // Port | |
| if (credentials.port !== 22) { | |
| args.push("-p", String(credentials.port)); | |
| } | |
| // User@host | |
| // For Freestyle: username is "{vmId}:{token}" | |
| args.push(`${credentials.username}@${credentials.host}`); | |
| return args; | |
| } | |
| private buildSSHArgs(credentials: SSHCredentials): string[] { | |
| const args: string[] = []; | |
| const nullDevice = process.platform === "win32" ? "NUL" : "/dev/null"; | |
| // Disable strict host key checking for cloud VMs (they're ephemeral) | |
| args.push("-o", "StrictHostKeyChecking=no"); | |
| args.push("-o", `UserKnownHostsFile=${nullDevice}`); | |
| // Set connection timeout | |
| args.push("-o", "ConnectTimeout=30"); | |
| // Keep connection alive | |
| args.push("-o", "ServerAliveInterval=30"); | |
| args.push("-o", "ServerAliveCountMax=3"); | |
| // Port | |
| if (credentials.port !== 22) { | |
| args.push("-p", String(credentials.port)); | |
| } | |
| // User@host | |
| // For Freestyle: username is "{vmId}:{token}" | |
| args.push(`${credentials.username}@${credentials.host}`); | |
| return args; | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/ssh-terminal/ssh-manager.ts` around lines 146 -
170, The SSH args currently hardcode UserKnownHostsFile=/dev/null which fails on
Windows; in buildSSHArgs(credentials: SSHCredentials) detect the platform
(process.platform === 'win32') and use "NUL" on Windows and "/dev/null"
otherwise (or omit the UserKnownHostsFile option on Windows if preferred), so
update the UserKnownHostsFile argument generation to be platform-specific while
keeping the other SSH options and the existing handling of port and
username@host.
| } catch (error) { | ||
| const err = | ||
| error instanceof Error ? error : new Error("Failed to create"); | ||
| toast.error("Failed to create cloud workspace", { | ||
| description: err.message, | ||
| }); | ||
| options?.onError?.(err); | ||
| } finally { | ||
| setIsPending(false); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const pauseWorkspace = { | ||
| mutate: async (workspaceId: string) => { | ||
| try { | ||
| await apiTrpcClient.cloudWorkspace.pause.mutate({ workspaceId }); | ||
| toast.success("Workspace paused"); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error ? error : new Error("Failed to pause"); | ||
| toast.error("Failed to pause workspace", { | ||
| description: err.message, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const resumeWorkspace = { | ||
| mutate: async (workspaceId: string) => { | ||
| try { | ||
| await apiTrpcClient.cloudWorkspace.resume.mutate({ workspaceId }); | ||
| toast.success("Workspace resumed"); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error ? error : new Error("Failed to resume"); | ||
| toast.error("Failed to resume workspace", { | ||
| description: err.message, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const stopWorkspace = { | ||
| mutate: async (workspaceId: string) => { | ||
| try { | ||
| await apiTrpcClient.cloudWorkspace.stop.mutate({ workspaceId }); | ||
| toast.success("Workspace stopped"); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error ? error : new Error("Failed to stop"); | ||
| toast.error("Failed to stop workspace", { | ||
| description: err.message, | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const deleteWorkspace = { | ||
| mutate: async (workspaceId: string) => { | ||
| try { | ||
| await apiTrpcClient.cloudWorkspace.delete.mutate({ workspaceId }); | ||
| toast.success("Workspace deleted"); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error ? error : new Error("Failed to delete"); | ||
| toast.error("Failed to delete workspace", { | ||
| description: err.message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Log mutation failures with context instead of only toasting.
Guideline says errors shouldn’t be swallowed; add a prefixed log in each catch.
✅ Suggested pattern (apply to each mutation)
} catch (error) {
const err =
error instanceof Error ? error : new Error("Failed to create");
+ console.error("[cloud-workspace/mutations] create failed:", err);
toast.error("Failed to create cloud workspace", {
description: err.message,
});📝 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.
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to create"); | |
| toast.error("Failed to create cloud workspace", { | |
| description: err.message, | |
| }); | |
| options?.onError?.(err); | |
| } finally { | |
| setIsPending(false); | |
| } | |
| }, | |
| }; | |
| const pauseWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.pause.mutate({ workspaceId }); | |
| toast.success("Workspace paused"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to pause"); | |
| toast.error("Failed to pause workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const resumeWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.resume.mutate({ workspaceId }); | |
| toast.success("Workspace resumed"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to resume"); | |
| toast.error("Failed to resume workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const stopWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.stop.mutate({ workspaceId }); | |
| toast.success("Workspace stopped"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to stop"); | |
| toast.error("Failed to stop workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const deleteWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.delete.mutate({ workspaceId }); | |
| toast.success("Workspace deleted"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to delete"); | |
| toast.error("Failed to delete workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to create"); | |
| console.error("[cloud-workspace/mutations] create failed:", err); | |
| toast.error("Failed to create cloud workspace", { | |
| description: err.message, | |
| }); | |
| options?.onError?.(err); | |
| } finally { | |
| setIsPending(false); | |
| } | |
| }, | |
| }; | |
| const pauseWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.pause.mutate({ workspaceId }); | |
| toast.success("Workspace paused"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to pause"); | |
| console.error("[cloud-workspace/mutations] pause failed:", err); | |
| toast.error("Failed to pause workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const resumeWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.resume.mutate({ workspaceId }); | |
| toast.success("Workspace resumed"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to resume"); | |
| console.error("[cloud-workspace/mutations] resume failed:", err); | |
| toast.error("Failed to resume workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const stopWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.stop.mutate({ workspaceId }); | |
| toast.success("Workspace stopped"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to stop"); | |
| console.error("[cloud-workspace/mutations] stop failed:", err); | |
| toast.error("Failed to stop workspace", { | |
| description: err.message, | |
| }); | |
| } | |
| }, | |
| }; | |
| const deleteWorkspace = { | |
| mutate: async (workspaceId: string) => { | |
| try { | |
| await apiTrpcClient.cloudWorkspace.delete.mutate({ workspaceId }); | |
| toast.success("Workspace deleted"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error ? error : new Error("Failed to delete"); | |
| console.error("[cloud-workspace/mutations] delete failed:", err); | |
| toast.error("Failed to delete workspace", { | |
| description: err.message, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/react-query/cloud-workspaces/useCloudWorkspaceMutations.ts`
around lines 40 - 109, The mutation catch blocks (pauseWorkspace.mutate,
resumeWorkspace.mutate, stopWorkspace.mutate, deleteWorkspace.mutate — and any
similar mutations) currently only show toasts; update each catch to also log the
failure with context (e.g., console.error or the app logger) including a
prefixed message, the action name, workspaceId, and the error object so the
error isn’t swallowed; keep the existing toast and onError behavior but add the
descriptive log call before/after the toast.
| const connect = useCallback(async () => { | ||
| if (!cloudWorkspace || cloudWorkspace.status !== "running") { | ||
| setConnectionError("Workspace is not running"); | ||
| return; | ||
| } | ||
|
|
||
| setIsConnecting(true); | ||
| setConnectionError(null); | ||
|
|
||
| try { | ||
| // Get SSH credentials from the API | ||
| const credentials = | ||
| await apiTrpcClient.cloudWorkspace.getSSHCredentials.query({ | ||
| workspaceId: cloudWorkspaceId, | ||
| }); | ||
|
|
||
| const xterm = xtermRef.current; | ||
| if (!xterm) throw new Error("Terminal not initialized"); | ||
|
|
||
| // Create SSH session | ||
| await createSession.mutateAsync({ | ||
| paneId, | ||
| cloudWorkspaceId, | ||
| credentials: { | ||
| host: credentials.host, | ||
| port: credentials.port, | ||
| username: credentials.username, | ||
| token: credentials.token, | ||
| }, | ||
| cols: xterm.cols, | ||
| rows: xterm.rows, | ||
| }); | ||
|
|
||
| isConnectedRef.current = true; | ||
| setIsConnected(true); | ||
| setIsConnecting(false); | ||
| } catch (error) { | ||
| console.error("[CloudTerminal] Connection failed:", error); | ||
| setConnectionError( | ||
| error instanceof Error ? error.message : "Connection failed", | ||
| ); | ||
| setIsConnecting(false); | ||
| } | ||
| }, [cloudWorkspace, cloudWorkspaceId, paneId, createSession]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the CloudTerminal.tsx file to see the full context
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/CloudTerminal/CloudTerminal.tsx | head -200Repository: superset-sh/superset
Length of output: 7326
🏁 Script executed:
# Search for useEffect that uses connect
rg -n "useEffect.*connect|connect.*useEffect" apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/CloudTerminal/CloudTerminal.tsx -A 3 -B 3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Find the useCloudWorkspace hook implementation
fd -t f "useCloudWorkspace" apps/desktop/src --type ts --type tsxRepository: superset-sh/superset
Length of output: 233
🏁 Script executed:
# Search for useCloudWorkspace definition
rg -n "export.*useCloudWorkspace|function useCloudWorkspace" apps/desktop/src -A 10 --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
find apps/desktop/src -name "*.ts" -o -name "*.tsx" | xargs grep -l "useCloudWorkspace" | head -5Repository: superset-sh/superset
Length of output: 492
🏁 Script executed:
rg -n "export.*useCloudWorkspace|function useCloudWorkspace" apps/desktop/src --type-listRepository: superset-sh/superset
Length of output: 453
🏁 Script executed:
rg -n "useCloudWorkspace" apps/desktop/src --max-count=10Repository: superset-sh/superset
Length of output: 2697
🏁 Script executed:
cat -n apps/desktop/src/renderer/react-query/cloud-workspaces/useCloudWorkspaces.tsRepository: superset-sh/superset
Length of output: 2743
The connect callback dependency causes unnecessary effect re-runs when workspace object reference changes.
The cloudWorkspace object from useCloudWorkspace is memoized but depends on the cloudWorkspaces array from useLiveQuery, which returns a new reference on data updates. This causes cloudWorkspace to change even when the workspace status hasn't. Since connect depends on cloudWorkspace (line 113) and is included in the useEffect dependencies (line 190), the effect re-runs when only the object reference changes, causing unnecessary terminal reinitialization and reconnection attempts.
Extract the status check outside the callback dependencies. Instead of including connect in the useEffect deps, call connect() directly from within the effect when status becomes "running", or use a separate dependency on cloudWorkspace?.status only.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/CloudTerminal/CloudTerminal.tsx`
around lines 70 - 113, The connect callback currently depends on the entire
cloudWorkspace object which causes spurious changes; update connect (function
name connect) to remove cloudWorkspace from its dependency array and only
reference primitive values it needs (cloudWorkspaceId, paneId, createSession,
xtermRef), and in the effect that triggers connection (the useEffect that
currently depends on connect) stop depending on connect and instead depend on
cloudWorkspace?.status (or call connect() directly inside the effect when
cloudWorkspace?.status === "running"); ensure status checks use
cloudWorkspace?.status rather than the whole object so reconnection only happens
when status actually becomes "running".
| const cleanupPromise = initTerminal(); | ||
|
|
||
| return () => { | ||
| cleanupPromise.then((cleanup) => cleanup?.()); | ||
| if (xterm) { | ||
| xterm.dispose(); | ||
| } | ||
| xtermRef.current = null; | ||
| fitAddonRef.current = null; | ||
| isConnectedRef.current = false; | ||
| setIsConnected(false); | ||
| }; | ||
| // Note: terminalTheme is intentionally NOT in deps - theme updates are handled separately below | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [cloudWorkspace?.status, connect, paneId]); |
There was a problem hiding this comment.
Potential race condition in cleanup logic.
The cleanup function runs synchronously but initTerminal is async. If the component unmounts before initTerminal completes:
xtermwill be null, soxterm.dispose()won't run- The
cleanupPromise.then(...)schedules cleanup for later, but the terminal may have been created after the check
Also, if cleanup runs after initTerminal completes, the ResizeObserver cleanup runs async while xterm.dispose() runs sync, potentially causing the observer to access a disposed terminal.
🛠️ Suggested fix with abort handling
useEffect(() => {
const container = terminalRef.current;
if (!container) return;
- let xterm: XTerm | null = null;
- let fitAddon: FitAddon | null = null;
+ let disposed = false;
+ let xtermInstance: XTerm | null = null;
+ let resizeObserver: ResizeObserver | null = null;
const initTerminal = async () => {
+ if (disposed) return;
+
const { Terminal } = await import("@xterm/xterm");
const { FitAddon } = await import("@xterm/addon-fit");
- xterm = new Terminal({
+ if (disposed) return;
+
+ const xterm = new Terminal({
cursorBlink: true,
cursorStyle: "block",
fontFamily: "var(--font-mono), monospace",
fontSize: 13,
lineHeight: 1.2,
theme: terminalTheme ?? undefined,
});
- fitAddon = new FitAddon();
+ const fitAddon = new FitAddon();
xterm.loadAddon(fitAddon);
xterm.open(container);
fitAddon.fit();
xtermRef.current = xterm;
fitAddonRef.current = fitAddon;
+ xtermInstance = xterm;
// Handle input
xterm.onData((data) => {
if (!isConnectedRef.current) return;
writeRef.current({ paneId, data });
});
// Handle resize
- const resizeObserver = new ResizeObserver(() => {
+ resizeObserver = new ResizeObserver(() => {
if (fitAddon) {
fitAddon.fit();
if (xterm && isConnectedRef.current) {
resizeRef.current({
paneId,
cols: xterm.cols,
rows: xterm.rows,
});
}
}
});
resizeObserver.observe(container);
// Auto-connect when workspace is ready
if (cloudWorkspace?.status === "running") {
connect();
}
-
- return () => {
- resizeObserver.disconnect();
- };
};
- const cleanupPromise = initTerminal();
+ initTerminal();
return () => {
- cleanupPromise.then((cleanup) => cleanup?.());
- if (xterm) {
- xterm.dispose();
- }
+ disposed = true;
+ resizeObserver?.disconnect();
+ xtermInstance?.dispose();
xtermRef.current = null;
fitAddonRef.current = null;
isConnectedRef.current = false;
setIsConnected(false);
};
}, [cloudWorkspace?.status, connect, paneId]);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/CloudTerminal/CloudTerminal.tsx`
around lines 176 - 190, The cleanup races with async initTerminal: change the
effect to track and cancel in-progress initialization by creating an
AbortController or a mutable flag and pass it into initTerminal (or have
initTerminal set cleanupRef when ready); on unmount/cleanup, set the abort flag
and if a cleanup function is already stored in cleanupRef call it synchronously
before disposing xterm; also ensure any ResizeObserver is disconnected inside
that cleanup function (or before xterm.dispose()) and clear xtermRef,
fitAddonRef, isConnectedRef, and call setIsConnected(false) to avoid the
observer accessing a disposed terminal (update references: initTerminal,
cleanupPromise, cleanupRef/cleanupPromise, xterm, xtermRef, fitAddonRef,
ResizeObserver, isConnectedRef, setIsConnected).
| const { vmId, status } = await provider.createVM({ | ||
| repoUrl, | ||
| branch, | ||
| workspaceName, | ||
| idleTimeoutSeconds: autoStopMinutes * 60, | ||
| }); | ||
|
|
||
| console.log("[cloud-workspace] VM created:", vmId, "status:", status); | ||
|
|
||
| // Update workspace with VM ID and running status | ||
| await dbWs | ||
| .update(cloudWorkspaces) | ||
| .set({ | ||
| providerVmId: vmId, | ||
| status: "running", | ||
| lastActiveAt: new Date(), |
There was a problem hiding this comment.
Don’t force running when the provider returns a different status.
createVM returns a status, but the DB is always set to running. This can surface a ready state before the VM is actually ready. Map the provider status or keep provisioning until confirmed running.
✅ Example mapping
const { vmId, status } = await provider.createVM({
repoUrl,
branch,
workspaceName,
idleTimeoutSeconds: autoStopMinutes * 60,
});
+ const nextStatus = status === "running" ? "running" : "provisioning";
+
// Update workspace with VM ID and running status
await dbWs
.update(cloudWorkspaces)
.set({
providerVmId: vmId,
- status: "running",
+ status: nextStatus,
lastActiveAt: new Date(),
})
.where(eq(cloudWorkspaces.id, workspaceId));📝 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 { vmId, status } = await provider.createVM({ | |
| repoUrl, | |
| branch, | |
| workspaceName, | |
| idleTimeoutSeconds: autoStopMinutes * 60, | |
| }); | |
| console.log("[cloud-workspace] VM created:", vmId, "status:", status); | |
| // Update workspace with VM ID and running status | |
| await dbWs | |
| .update(cloudWorkspaces) | |
| .set({ | |
| providerVmId: vmId, | |
| status: "running", | |
| lastActiveAt: new Date(), | |
| const { vmId, status } = await provider.createVM({ | |
| repoUrl, | |
| branch, | |
| workspaceName, | |
| idleTimeoutSeconds: autoStopMinutes * 60, | |
| }); | |
| console.log("[cloud-workspace] VM created:", vmId, "status:", status); | |
| const nextStatus = status === "running" ? "running" : "provisioning"; | |
| // Update workspace with VM ID and running status | |
| await dbWs | |
| .update(cloudWorkspaces) | |
| .set({ | |
| providerVmId: vmId, | |
| status: nextStatus, | |
| lastActiveAt: new Date(), |
🤖 Prompt for AI Agents
In `@packages/trpc/src/router/cloud-workspace/cloud-workspace.ts` around lines 530
- 545, The DB update currently forces status: "running" after provider.createVM
returns a status; instead use the returned status (or map it into your internal
states) when calling dbWs.update(cloudWorkspaces).set so you don't prematurely
mark a workspace running. Locate the createVM call (provider.createVM) and the
subsequent dbWs.update(cloudWorkspaces).set block, replace the hardcoded
"running" with either the provider `status` (or a mapped value, e.g., map
provider statuses to "provisioning" | "running" | "failed") and ensure
lastActiveAt remains correct; if you want to keep a conservative default, set to
"provisioning" until you observe a confirmed running state instead of forcing
"running".
# Conflicts: # packages/db/src/schema/index.ts # packages/db/src/schema/relations.ts # packages/trpc/src/env.ts
- Add migration 0012_add_cloud_workspaces.sql with tables and enums - Creates cloud_workspaces and cloud_workspace_sessions tables - Adds cloud_workspace_status, cloud_provider_type, cloud_client_type enums - Update testing plan to reference correct migration number (0012)
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
1 participant
Summary
Docs included
CLOUD_WORKSPACE_PLAN.md- High-level architecture and designCLOUD_WORKSPACE_IMPLEMENTATION.md- Implementation detailscloud-workspace-testing-plan.md- Testing strategySummary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.