feat: initial windows support#2322
Open
ruangustavo wants to merge 9 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Windows support across the desktop app: platform-aware IPC (named pipes vs Unix sockets), Windows-aware shell/command handling (PowerShell/cmd), Win-specific environment/locale shortcuts, cross-platform agent file commands, and related renderer/platform flags and small misc fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (UI)
participant Client as Desktop Client
participant Daemon as Terminal Host Daemon
participant OS as OS (Filesystem / Named Pipes)
Renderer->>Client: request terminal start
Client->>Daemon: connect to socket (getSocketPath)
alt Unix (file-based)
Daemon->>OS: ensure socket file exists
OS-->>Daemon: socket file available
Client->>Daemon: establish Unix socket connection
else Windows (named pipe)
Daemon->>OS: create named pipe path
OS-->>Daemon: named pipe available
Client->>Daemon: attempt connection (no filesystem probe)
end
Note right of Daemon: platform-aware existence checks via socketMayExist/isFileBasedSocket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
310-315: Windows shells skip managed binary prelude—is this intentional?PowerShell and CMD receive the raw
commandrather thancommandWithManagedPrelude. Other shells get wrapper functions that prioritizeBIN_DIRexecutables (claude, codex, etc.) over system-installed ones.This is likely intentional since
buildManagedCommandPreludegenerates POSIX shell syntax incompatible with PowerShell/CMD. However, this means agent-managed binaries won't be preferentially resolved on Windows.If managed binary support is needed on Windows, a PowerShell-specific prelude would be required. Otherwise, consider adding a comment explaining this limitation.
Optional: Add clarifying comment
if (["powershell", "pwsh", "powershell.exe", "pwsh.exe"].includes(shellName)) { + // Note: Managed binary prelude is POSIX-specific; Windows relies on PATH ordering return ["-NoLogo", "-Command", command]; } if (["cmd", "cmd.exe"].includes(shellName)) { + // Note: Managed binary prelude is POSIX-specific; Windows relies on PATH ordering return ["/C", command]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` around lines 310 - 315, The Windows branch in the function that builds shell wrappers (checks on shellName) returns the raw command instead of commandWithManagedPrelude, so agent-managed binaries (constructed by buildManagedCommandPrelude / commandWithManagedPrelude) are not preferred on PowerShell/CMD; either implement a PowerShell/CMD-specific prelude (translate or recreate the BIN_DIR prioritization in PowerShell syntax and use it for shells matching "powershell", "pwsh", "powershell.exe", "pwsh.exe" and for CMD) or, if Windows support is intentionally omitted, add a clear comment next to the shellName checks explaining that buildManagedCommandPrelude is POSIX-only and Windows shells intentionally skip the managed binary prelude so managed binaries won’t be preferred on Windows.apps/desktop/src/lib/trpc/routers/external/helpers.ts (1)
110-126: Please add an explicitwin32case for this branch.In
apps/desktop/src/lib/trpc/routers/external/helpers.test.ts, Lines 6-15 forceprocess.platformto"darwin"inbeforeEach, and Lines 141-148 are the only explicit non-default platform case today (linux). Right now this code only gets exercised on real Windows machines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts` around lines 110 - 126, The branch that handles platform === "win32" (using the platform variable and returning commands built from CLI_CANDIDATES and CLI_COMMANDS for a given app) is not covered by tests because tests force process.platform to "darwin"; add an explicit test case in helpers.test.ts that sets process.platform to "win32" (or add a beforeEach/describe block that temporarily stubs process.platform to "win32" for Windows-specific tests), exercise the code paths for app === "finder", an app with CLI_CANDIDATES, and one resolved via CLI_COMMANDS, and ensure you restore the original process.platform after the test to avoid leaking state.
🤖 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/src/lib/trpc/routers/external/helpers.ts`:
- Around line 167-171: The check str.includes("\\") in looksLikePath() is too
permissive; update looksLikePath to only treat Windows-style paths as true —
specifically detect drive-letter paths (like "C:\\"), UNC paths (starting with
"\\\\"), or backslash-based paths that show a directory separator followed by a
filename component (not just any backslash from e.g. a regex). Modify the logic
in looksLikePath() (and any callers such as extractEmbeddedPath()) to replace
the broad includes("\\") with stricter patterns for drive-letter, UNC, or a
backslash + name segment, and add/adjust unit tests to cover cases like
"foo(\\d+)bar", "C:\\path\\file.txt", "\\\\server\\share\\file", and
"dir\\file".
In `@apps/desktop/src/main/lib/auto-updater.ts`:
- Around line 24-25: The unsupported-platform user-facing string still says
"auto-updates are only available on macOS and Linux" even though
IS_AUTO_UPDATE_PLATFORM (based on PLATFORM.IS_MAC || PLATFORM.IS_LINUX ||
PLATFORM.IS_WINDOWS) now includes Windows; update the copy in
apps/desktop/src/main/lib/auto-updater.ts to mention Windows as supported (or
reword to "macOS, Linux, and Windows") wherever the unsupported-platform message
is defined so it matches the IS_AUTO_UPDATE_PLATFORM logic (search for the
string or the function that returns the unsupported-platform message and update
it).
In `@apps/desktop/src/main/lib/terminal/env.ts`:
- Around line 42-54: The fallback shell on Windows is currently identical to the
primary shell (both resolve to "powershell.exe"), so retrying with
FALLBACK_SHELL in useFallbackShell becomes a no-op; change FALLBACK_SHELL to a
different Windows shell (for example "cmd.exe" or the COMSPEC path) so that
fallback attempts in session.ts (where useFallbackShell compares FALLBACK_SHELL
and getDefaultShell()) actually launch a different binary; update the
FALLBACK_SHELL declaration (and any comments) to return a distinct Windows
fallback while leaving getDefaultShell() returning "powershell.exe".
In `@apps/desktop/src/renderer/lib/terminal/launch-command.ts`:
- Around line 27-29: normalizeTerminalCommand currently appends a carriage
return ("\r") which breaks tests and terminal behavior; change it to ensure
commands end with a single line feed ("\n") instead: in
normalizeTerminalCommand, check whether command already ends with "\n" and if
not append "\n" (avoid adding duplicated line endings if the input already ends
with "\n" or "\r\n"); update the logic in normalizeTerminalCommand to produce a
single LF terminator so tests expecting "...\n" (e.g., launch-command.test.ts
and bootstrap-open-worktree.test.ts) pass and terminals receive the proper LF
ending.
In `@packages/mcp/src/tools/devices/start-agent-session/start-agent-session.ts`:
- Around line 107-117: The filename uses task.slug directly (taskPromptFileName
= `task-${task.slug}.md`) which can include Windows-reserved/path-escaping
chars; update start-agent-session.ts to build the filename with a sanitized slug
by calling sanitizeSegment(task.slug) from
apps/desktop/src/shared/utils/branch.ts (import sanitizeSegment at top), e.g.
taskPromptFileName = `task-${sanitizeSegment(task.slug)}.md`, and ensure you
keep the .md extension and handle a potential empty sanitized result (fallback
to a safe constant like "task") if needed.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts`:
- Around line 110-126: The branch that handles platform === "win32" (using the
platform variable and returning commands built from CLI_CANDIDATES and
CLI_COMMANDS for a given app) is not covered by tests because tests force
process.platform to "darwin"; add an explicit test case in helpers.test.ts that
sets process.platform to "win32" (or add a beforeEach/describe block that
temporarily stubs process.platform to "win32" for Windows-specific tests),
exercise the code paths for app === "finder", an app with CLI_CANDIDATES, and
one resolved via CLI_COMMANDS, and ensure you restore the original
process.platform after the test to avoid leaking state.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 310-315: The Windows branch in the function that builds shell
wrappers (checks on shellName) returns the raw command instead of
commandWithManagedPrelude, so agent-managed binaries (constructed by
buildManagedCommandPrelude / commandWithManagedPrelude) are not preferred on
PowerShell/CMD; either implement a PowerShell/CMD-specific prelude (translate or
recreate the BIN_DIR prioritization in PowerShell syntax and use it for shells
matching "powershell", "pwsh", "powershell.exe", "pwsh.exe" and for CMD) or, if
Windows support is intentionally omitted, add a clear comment next to the
shellName checks explaining that buildManagedCommandPrelude is POSIX-only and
Windows shells intentionally skip the managed binary prelude so managed binaries
won’t be preferred on Windows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14abc80a-9777-4bd5-8634-c6d7719d7687
📒 Files selected for processing (20)
apps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/lib/auto-updater.tsapps/desktop/src/main/lib/terminal-host/client.tsapps/desktop/src/main/lib/terminal-host/paths.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/terminal-host/index.tsapps/desktop/src/main/terminal-host/signal-handlers.tsapps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/PromptGroup.tsxapps/desktop/src/renderer/lib/agent-session-orchestrator/adapters/terminal-adapter.tsapps/desktop/src/renderer/lib/platform.tsapps/desktop/src/renderer/lib/terminal/launch-command.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/OpenInWorkspace/OpenInWorkspace.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/RunInWorkspacePopover/RunInWorkspacePopover.tsxpackages/mcp/src/tools/devices/start-agent-session/start-agent-session.tspackages/shared/src/agent-command.test.tspackages/shared/src/agent-command.tspackages/shared/src/agent-launch.ts
Comment on lines
+167
to
+171
| str.includes("\\") || | ||
| str.startsWith(".") || | ||
| str.startsWith("~") || | ||
| str.startsWith("/") | ||
| str.startsWith("/") || | ||
| /^[A-Za-z]:/.test(str) |
Contributor
There was a problem hiding this comment.
str.includes("\\") is too broad for looksLikePath().
extractEmbeddedPath() trusts this helper, so inputs like foo(\\d+)bar now get extracted and resolved as file paths just because they contain a backslash. Please tighten this to actual Windows path shapes (drive-letter, UNC, or a more filename-like backslash pattern).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts` around lines 167 -
171, The check str.includes("\\") in looksLikePath() is too permissive; update
looksLikePath to only treat Windows-style paths as true — specifically detect
drive-letter paths (like "C:\\"), UNC paths (starting with "\\\\"), or
backslash-based paths that show a directory separator followed by a filename
component (not just any backslash from e.g. a regex). Modify the logic in
looksLikePath() (and any callers such as extractEmbeddedPath()) to replace the
broad includes("\\") with stricter patterns for drive-letter, UNC, or a
backslash + name segment, and add/adjust unit tests to cover cases like
"foo(\\d+)bar", "C:\\path\\file.txt", "\\\\server\\share\\file", and
"dir\\file".
Comment on lines
+24
to
+25
| const IS_AUTO_UPDATE_PLATFORM = | ||
| PLATFORM.IS_MAC || PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS; |
Contributor
There was a problem hiding this comment.
Update the unsupported-platform copy to include Windows.
Line 24 now enables Windows here, but Line 135 still tells users that auto-updates are only available on macOS and Linux.
📝 Suggested fix
- message: "Auto-updates are only available on macOS and Linux.",
+ message: "Auto-updates are only available on macOS, Linux, and Windows.",📝 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
| const IS_AUTO_UPDATE_PLATFORM = | |
| PLATFORM.IS_MAC || PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS; | |
| message: "Auto-updates are only available on macOS, Linux, and Windows.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/auto-updater.ts` around lines 24 - 25, The
unsupported-platform user-facing string still says "auto-updates are only
available on macOS and Linux" even though IS_AUTO_UPDATE_PLATFORM (based on
PLATFORM.IS_MAC || PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS) now includes
Windows; update the copy in apps/desktop/src/main/lib/auto-updater.ts to mention
Windows as supported (or reword to "macOS, Linux, and Windows") wherever the
unsupported-platform message is defined so it matches the
IS_AUTO_UPDATE_PLATFORM logic (search for the string or the function that
returns the unsupported-platform message and update it).
Comment on lines
+42
to
+54
| export const FALLBACK_SHELL = | ||
| os.platform() === "win32" ? "powershell.exe" : "/bin/sh"; | ||
| export const SHELL_CRASH_THRESHOLD_MS = 1000; | ||
|
|
||
| export function getDefaultShell(): string { | ||
| if (defaultShell) { | ||
| return defaultShell; | ||
| } | ||
|
|
||
| const platform = os.platform(); | ||
|
|
||
| // On Windows, always use PowerShell. The `default-shell` package returns | ||
| // COMSPEC (cmd.exe), which can't handle the subexpressions and file-read | ||
| // syntax used by agent launch commands. | ||
| if (platform === "win32") { | ||
| return process.env.COMSPEC || "powershell.exe"; | ||
| return "powershell.exe"; | ||
| } |
Contributor
There was a problem hiding this comment.
Keep the Windows fallback shell distinct from the primary shell.
In apps/desktop/src/main/lib/terminal/session.ts:98, useFallbackShell chooses between FALLBACK_SHELL and getDefaultShell(). With both now resolving to powershell.exe on Windows, the recovery path becomes a no-op: if PowerShell itself or its profile is what breaks startup, retrying with the fallback shell will just relaunch the same failing shell.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/terminal/env.ts` around lines 42 - 54, The fallback
shell on Windows is currently identical to the primary shell (both resolve to
"powershell.exe"), so retrying with FALLBACK_SHELL in useFallbackShell becomes a
no-op; change FALLBACK_SHELL to a different Windows shell (for example "cmd.exe"
or the COMSPEC path) so that fallback attempts in session.ts (where
useFallbackShell compares FALLBACK_SHELL and getDefaultShell()) actually launch
a different binary; update the FALLBACK_SHELL declaration (and any comments) to
return a distinct Windows fallback while leaving getDefaultShell() returning
"powershell.exe".
Comment on lines
27
to
29
| function normalizeTerminalCommand(command: string): string { | ||
| return command.endsWith("\n") ? command : `${command}\n`; | ||
| return command.endsWith("\r") ? command : `${command}\r`; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test assertions that expect \n line endings
rg -n '\\n' apps/desktop/src/renderer/lib/terminal/launch-command.test.ts
rg -n '\\n' apps/desktop/src/renderer/react-query/workspaces/bootstrap-open-worktree.test.tsRepository: superset-sh/superset
Length of output: 216
🏁 Script executed:
# Extract the test assertions around the lines that use \n
sed -n '25,45p' apps/desktop/src/renderer/lib/terminal/launch-command.test.ts
sed -n '40,55p' apps/desktop/src/renderer/lib/terminal/launch-command.test.ts
sed -n '75,85p' apps/desktop/src/renderer/lib/terminal/launch-command.test.ts
sed -n '65,75p' apps/desktop/src/renderer/react-query/workspaces/bootstrap-open-worktree.test.tsRepository: superset-sh/superset
Length of output: 1279
Change from \n to \r breaks all test assertions expecting LF line endings.
The normalization function now appends \r (carriage return) instead of \n (line feed), causing test failures. Test assertions throughout the codebase explicitly expect commands to end with \n:
launch-command.test.ts(lines 29, 42, 49, 81): All expectdata: "...\\n"bootstrap-open-worktree.test.ts(line 69): Expectsdata: "echo setup\\n"
With this change:
- Input
"echo hello"produces"echo hello\r"instead of expected"echo hello\n" - Input
"echo hello\n"produces"echo hello\n\r"instead of expected"echo hello\n"
Additionally, a lone \r is not the standard line terminator for terminals. Unix shells expect \n, and Windows typically expects \r\n. A carriage return alone is unlikely to execute commands correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/lib/terminal/launch-command.ts` around lines 27 -
29, normalizeTerminalCommand currently appends a carriage return ("\r") which
breaks tests and terminal behavior; change it to ensure commands end with a
single line feed ("\n") instead: in normalizeTerminalCommand, check whether
command already ends with "\n" and if not append "\n" (avoid adding duplicated
line endings if the input already ends with "\n" or "\r\n"); update the logic in
normalizeTerminalCommand to produce a single LF terminator so tests expecting
"...\n" (e.g., launch-command.test.ts and bootstrap-open-worktree.test.ts) pass
and terminals receive the proper LF ending.
Comment on lines
+107
to
+117
| const taskPromptFileName = `task-${task.slug}.md`; | ||
|
|
||
| return { | ||
| kind: "terminal", | ||
| workspaceId, | ||
| agentType: agent, | ||
| agentType: agent as (typeof AGENT_TYPES)[number], | ||
| source: "mcp", | ||
| terminal: { | ||
| command: buildAgentCommand({ | ||
| task, | ||
| randomId: crypto.randomUUID(), | ||
| agent: agent as (typeof AGENT_TYPES)[number], | ||
| }), | ||
| name: task.slug, | ||
| taskPromptContent: buildAgentTaskPrompt(task), | ||
| taskPromptFileName, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how task slugs are defined/validated.
rg -n -C3 '\b(tasks\.slug|slug\b)' packages apps -g '*.ts' -g '*.tsx'
printf '\n--- taskPromptFileName consumers ---\n\n'
# Inspect where prompt filenames are converted into paths/commands.
rg -n -C3 'taskPromptFileName|\.superset/' packages apps -g '*.ts' -g '*.tsx'Repository: superset-sh/superset
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for .superset/ path construction in terminal-adapter
rg -n -B2 -A2 '\.superset/' apps/desktop --type ts --type tsx
# Also search for any sanitization functions that might be applied to slugs
rg -n 'sanitize|escape|normalize.*path|path.*sanitize' packages apps --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for .superset/ path construction in terminal-adapter
rg -n -B2 -A2 '\.superset/' apps/desktop -g '*.ts' -g '*.tsx'
# Also search for any sanitization functions that might be applied to slugs
rg -n 'sanitize|escape|normalize.*path|path.*sanitize' packages apps -g '*.ts' -g '*.tsx'Repository: superset-sh/superset
Length of output: 43281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the exact signature of sanitizeSegment to understand what it does
head -50 apps/desktop/src/shared/utils/branch.ts
# Verify that task slug has no pattern validation
cat packages/trpc/src/router/task/schema.ts | head -20Repository: superset-sh/superset
Length of output: 1961
Use sanitizeSegment() to sanitize task.slug in the filename.
The task slug can contain Windows-reserved characters (:, *, ?, ", <, >, |, \, /) that cause file I/O failures and path-escape vulnerabilities. Apply the existing sanitizeSegment() utility from apps/desktop/src/shared/utils/branch.ts when constructing taskPromptFileName:
Example fix
+ import { sanitizeSegment } from "../../utils/branch";
- const taskPromptFileName = `task-${task.slug}.md`;
+ const taskPromptFileName = `task-${sanitizeSegment(task.slug)}.md`;Alternatively, add a filename-safe regex pattern to the task slug schema in packages/trpc/src/router/task/schema.ts to enforce constraints at creation time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mcp/src/tools/devices/start-agent-session/start-agent-session.ts`
around lines 107 - 117, The filename uses task.slug directly (taskPromptFileName
= `task-${task.slug}.md`) which can include Windows-reserved/path-escaping
chars; update start-agent-session.ts to build the filename with a sanitized slug
by calling sanitizeSegment(task.slug) from
apps/desktop/src/shared/utils/branch.ts (import sanitizeSegment at top), e.g.
taskPromptFileName = `task-${sanitizeSegment(task.slug)}.md`, and ensure you
keep the .md extension and handle a potential empty sanitized result (fallback
to a safe constant like "task") if needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
9 issues found across 20 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/main/lib/auto-updater.ts">
<violation number="1" location="apps/desktop/src/main/lib/auto-updater.ts:25">
P3: Update the unsupported-platform message to include Windows now that `IS_AUTO_UPDATE_PLATFORM` allows it.</violation>
</file>
<file name="apps/desktop/scripts/copy-native-modules.ts">
<violation number="1" location="apps/desktop/scripts/copy-native-modules.ts:110">
P2: Gate the Windows symlink/junction removal flags behind a runtime platform check instead of changing the shared path for all OSes.
(Based on your team's feedback about gating platform-specific changes with runtime checks.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/lib/terminal/launch-command.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/terminal/launch-command.ts:28">
P2: This normalization only checks for `\r`, so commands that already end with `\n` get an extra terminator appended (`\n\r`). Handle both trailing newline forms to keep writes idempotent.</violation>
</file>
<file name="apps/desktop/src/renderer/lib/agent-session-orchestrator/adapters/terminal-adapter.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/agent-session-orchestrator/adapters/terminal-adapter.ts:19">
P1: Validate `taskPromptFileName` as a basename before building the command path. Without this, crafted values can escape `.superset/` and read unintended files when `taskPromptContent` is absent.</violation>
</file>
<file name="apps/desktop/src/main/lib/terminal-host/paths.ts">
<violation number="1" location="apps/desktop/src/main/lib/terminal-host/paths.ts:15">
P2: Windows named pipe name is not user-scoped, so different local users can collide on the same terminal-host pipe.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/external/helpers.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/external/helpers.ts:115">
P1: Windows app commands are returned as raw CLI names, but they are executed with `spawn` without a shell; common Windows CLI launchers (`.cmd`/`.bat`) can fail to start in this path.</violation>
</file>
<file name="packages/shared/src/agent-command.ts">
<violation number="1" location="packages/shared/src/agent-command.ts:91">
P2: Use `-LiteralPath` in the Windows `Get-Content` call so file paths are treated literally instead of wildcard patterns.</violation>
</file>
<file name="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts">
<violation number="1" location="apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts:313">
P2: Normalize cmd shell detection to handle Windows-style absolute paths. As written, `C:\\...\\cmd.exe` won’t match this branch and can fall through to the `-lc` fallback.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/shared/src/agent-launch.ts">
<violation number="1" location="packages/shared/src/agent-launch.ts:43">
P2: `command` is now optional, but there’s no schema check that `taskPromptFileName` is provided when `command` is missing. That allows terminal launch requests to pass validation and then throw in `resolveCommand`. Add a refinement (or keep `command` required) so at least one of those fields is enforced at parse time.</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.
Comment on lines
+115
to
+125
| const winCandidates = CLI_CANDIDATES[app]; | ||
| if (winCandidates) { | ||
| return winCandidates.map((cmd) => ({ | ||
| command: cmd, | ||
| args: [targetPath], | ||
| })); | ||
| } | ||
|
|
||
| const winCommand = CLI_COMMANDS[app]; | ||
| if (!winCommand) return null; | ||
| return [{ command: winCommand, args: [targetPath] }]; |
Contributor
There was a problem hiding this comment.
P1: Windows app commands are returned as raw CLI names, but they are executed with spawn without a shell; common Windows CLI launchers (.cmd/.bat) can fail to start in this path.
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/external/helpers.ts, line 115:
<comment>Windows app commands are returned as raw CLI names, but they are executed with `spawn` without a shell; common Windows CLI launchers (`.cmd`/`.bat`) can fail to start in this path.</comment>
<file context>
@@ -107,16 +107,33 @@ export function getAppCommand(
+ return [{ command: "explorer.exe", args: [targetPath] }];
+ }
+
+ const winCandidates = CLI_CANDIDATES[app];
+ if (winCandidates) {
+ return winCandidates.map((cmd) => ({
</file context>
Suggested change
| const winCandidates = CLI_CANDIDATES[app]; | |
| if (winCandidates) { | |
| return winCandidates.map((cmd) => ({ | |
| command: cmd, | |
| args: [targetPath], | |
| })); | |
| } | |
| const winCommand = CLI_COMMANDS[app]; | |
| if (!winCommand) return null; | |
| return [{ command: winCommand, args: [targetPath] }]; | |
| const winCandidates = CLI_CANDIDATES[app]; | |
| if (winCandidates) { | |
| return winCandidates.map((cmd) => ({ | |
| command: "cmd.exe", | |
| args: ["/d", "/s", "/c", cmd, targetPath], | |
| })); | |
| } | |
| const winCommand = CLI_COMMANDS[app]; | |
| if (!winCommand) return null; | |
| return [ | |
| { | |
| command: "cmd.exe", | |
| args: ["/d", "/s", "/c", winCommand, targetPath], | |
| }, | |
| ]; |
| // Remove the symlink | ||
| rmSync(modulePath); | ||
| // Remove the symlink (recursive needed on Windows for junction/symlink dirs) | ||
| rmSync(modulePath, { recursive: true, force: true }); |
Contributor
There was a problem hiding this comment.
P2: Gate the Windows symlink/junction removal flags behind a runtime platform check instead of changing the shared path for all OSes.
(Based on your team's feedback about gating platform-specific changes with runtime checks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/scripts/copy-native-modules.ts, line 110:
<comment>Gate the Windows symlink/junction removal flags behind a runtime platform check instead of changing the shared path for all OSes.
(Based on your team's feedback about gating platform-specific changes with runtime checks.) </comment>
<file context>
@@ -106,8 +106,8 @@ function copyModuleIfSymlink(
- // Remove the symlink
- rmSync(modulePath);
+ // Remove the symlink (recursive needed on Windows for junction/symlink dirs)
+ rmSync(modulePath, { recursive: true, force: true });
// Copy the actual files
</file context>
Suggested change
| rmSync(modulePath, { recursive: true, force: true }); | |
| if (process.platform === "win32") { | |
| rmSync(modulePath, { recursive: true, force: true }); | |
| } else { | |
| rmSync(modulePath); | |
| } |
|
|
||
| function normalizeTerminalCommand(command: string): string { | ||
| return command.endsWith("\n") ? command : `${command}\n`; | ||
| return command.endsWith("\r") ? command : `${command}\r`; |
Contributor
There was a problem hiding this comment.
P2: This normalization only checks for \r, so commands that already end with \n get an extra terminator appended (\n\r). Handle both trailing newline forms to keep writes idempotent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/lib/terminal/launch-command.ts, line 28:
<comment>This normalization only checks for `\r`, so commands that already end with `\n` get an extra terminator appended (`\n\r`). Handle both trailing newline forms to keep writes idempotent.</comment>
<file context>
@@ -25,7 +25,7 @@ interface LaunchCommandInPaneOptions {
function normalizeTerminalCommand(command: string): string {
- return command.endsWith("\n") ? command : `${command}\n`;
+ return command.endsWith("\r") ? command : `${command}\r`;
}
</file context>
Suggested change
| return command.endsWith("\r") ? command : `${command}\r`; | |
| return /[\r\n]$/.test(command) ? command : `${command}\r`; |
| supersetHomeDir: string, | ||
| ): string { | ||
| if (PLATFORM.IS_WINDOWS) { | ||
| return `\\\\?\\pipe\\superset-terminal-host-${supersetDirName}`; |
Contributor
There was a problem hiding this comment.
P2: Windows named pipe name is not user-scoped, so different local users can collide on the same terminal-host pipe.
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/terminal-host/paths.ts, line 15:
<comment>Windows named pipe name is not user-scoped, so different local users can collide on the same terminal-host pipe.</comment>
<file context>
@@ -0,0 +1,36 @@
+ supersetHomeDir: string,
+): string {
+ if (PLATFORM.IS_WINDOWS) {
+ return `\\\\?\\pipe\\superset-terminal-host-${supersetDirName}`;
+ }
+ return join(supersetHomeDir, "terminal-host.sock");
</file context>
| ): string { | ||
| if (platform === "win32") { | ||
| const escaped = filePath.replaceAll("'", "''"); | ||
| return `(Get-Content '${escaped}' -Raw)`; |
Contributor
There was a problem hiding this comment.
P2: Use -LiteralPath in the Windows Get-Content call so file paths are treated literally instead of wildcard patterns.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/src/agent-command.ts, line 91:
<comment>Use `-LiteralPath` in the Windows `Get-Content` call so file paths are treated literally instead of wildcard patterns.</comment>
<file context>
@@ -80,6 +80,51 @@ You are running fully autonomously. Do not ask questions or wait for user feedba
+): string {
+ if (platform === "win32") {
+ const escaped = filePath.replaceAll("'", "''");
+ return `(Get-Content '${escaped}' -Raw)`;
+ }
+ const escaped = filePath.replaceAll("'", "'\\''");
</file context>
Suggested change
| return `(Get-Content '${escaped}' -Raw)`; | |
| return `(Get-Content -LiteralPath '${escaped}' -Raw)`; |
| if (["powershell", "pwsh", "powershell.exe", "pwsh.exe"].includes(shellName)) { | ||
| return ["-NoLogo", "-Command", command]; | ||
| } | ||
| if (["cmd", "cmd.exe"].includes(shellName)) { |
Contributor
There was a problem hiding this comment.
P2: Normalize cmd shell detection to handle Windows-style absolute paths. As written, C:\\...\\cmd.exe won’t match this branch and can fall through to the -lc fallback.
(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/main/lib/agent-setup/shell-wrappers.ts, line 313:
<comment>Normalize cmd shell detection to handle Windows-style absolute paths. As written, `C:\\...\\cmd.exe` won’t match this branch and can fall through to the `-lc` fallback.
(Based on your team's feedback about using cross-platform path utilities instead of split.) </comment>
<file context>
@@ -304,5 +307,11 @@ export function getCommandShellArgs(
+ if (["powershell", "pwsh", "powershell.exe", "pwsh.exe"].includes(shellName)) {
+ return ["-NoLogo", "-Command", command];
+ }
+ if (["cmd", "cmd.exe"].includes(shellName)) {
+ return ["/C", command];
+ }
</file context>
Suggested change
| if (["cmd", "cmd.exe"].includes(shellName)) { | |
| if (/(^|[\\/])cmd(\.exe)?$/i.test(shell)) { |
Comment on lines
+43
to
46
| command: z.string().min(1).optional(), | ||
| name: z.string().min(1).optional(), | ||
| paneId: z.string().min(1).optional(), | ||
| taskPromptContent: z.string().min(1).optional(), |
Contributor
There was a problem hiding this comment.
P2: command is now optional, but there’s no schema check that taskPromptFileName is provided when command is missing. That allows terminal launch requests to pass validation and then throw in resolveCommand. Add a refinement (or keep command required) so at least one of those fields is enforced at parse time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/shared/src/agent-launch.ts, line 43:
<comment>`command` is now optional, but there’s no schema check that `taskPromptFileName` is provided when `command` is missing. That allows terminal launch requests to pass validation and then throw in `resolveCommand`. Add a refinement (or keep `command` required) so at least one of those fields is enforced at parse time.</comment>
<file context>
@@ -40,7 +40,7 @@ const baseAgentLaunchSchema = z.object({
export const terminalLaunchConfigSchema = z.object({
- command: z.string().min(1),
+ command: z.string().min(1).optional(),
name: z.string().min(1).optional(),
paneId: z.string().min(1).optional(),
</file context>
| const IS_PRERELEASE = isPrereleaseBuild(); | ||
| const IS_AUTO_UPDATE_PLATFORM = PLATFORM.IS_MAC || PLATFORM.IS_LINUX; | ||
| const IS_AUTO_UPDATE_PLATFORM = | ||
| PLATFORM.IS_MAC || PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS; |
Contributor
There was a problem hiding this comment.
P3: Update the unsupported-platform message to include Windows now that IS_AUTO_UPDATE_PLATFORM allows it.
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/auto-updater.ts, line 25:
<comment>Update the unsupported-platform message to include Windows now that `IS_AUTO_UPDATE_PLATFORM` allows it.</comment>
<file context>
@@ -21,7 +21,8 @@ function isPrereleaseBuild(): boolean {
const IS_PRERELEASE = isPrereleaseBuild();
-const IS_AUTO_UPDATE_PLATFORM = PLATFORM.IS_MAC || PLATFORM.IS_LINUX;
+const IS_AUTO_UPDATE_PLATFORM =
+ PLATFORM.IS_MAC || PLATFORM.IS_LINUX || PLATFORM.IS_WINDOWS;
// Use explicit feed URLs to ensure we always fetch platform-specific manifests
</file context>
This was referenced Mar 13, 2026
# Conflicts: # apps/desktop/src/main/lib/terminal/env.ts
…port # Conflicts: # apps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.ts # apps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/PromptGroup.tsx # apps/desktop/src/renderer/lib/agent-session-orchestrator/adapters/terminal-adapter.ts # apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/PropertiesSidebar/components/OpenInWorkspace/OpenInWorkspace.tsx # apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/RunInWorkspacePopover/RunInWorkspacePopover.tsx # packages/shared/src/agent-command.test.ts # packages/shared/src/agent-command.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/main/lib/terminal/env.ts (1)
42-43:⚠️ Potential issue | 🟠 MajorUse a distinct Windows fallback shell.
On Windows,
FALLBACK_SHELLandgetDefaultShell()now both resolve topowershell.exe, so fallback retry paths become a no-op for PowerShell-specific startup failures.Suggested fix
-export const FALLBACK_SHELL = - os.platform() === "win32" ? "powershell.exe" : "/bin/sh"; +export const FALLBACK_SHELL = + os.platform() === "win32" ? "cmd.exe" : "/bin/sh";Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/env.ts` around lines 42 - 43, FALLBACK_SHELL currently resolves to "powershell.exe" on Windows, which duplicates getDefaultShell() and makes fallback retries a no-op; change the Windows fallback to a distinct shell such as "cmd.exe" so retries can differ from the default PowerShell startup path, and apply the same change to the other occurrence around the getDefaultShell/fallback logic (references: FALLBACK_SHELL and getDefaultShell()).
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/external/helpers.ts (2)
45-46: Update comment to reflect cross-platform usage.The comment still says "Linux CLI commands" but
CLI_COMMANDSis now used for both Linux and Windows. Consider updating to "CLI commands for Linux and Windows" or similar.-/** Map of app IDs to their Linux CLI commands */ +/** Map of app IDs to their CLI commands (Linux/Windows) */ const CLI_COMMANDS: Record<ExternalApp, string | null> = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts` around lines 45 - 46, Update the top comment for the CLI_COMMANDS constant to reflect cross-platform usage instead of "Linux CLI commands": change the comment to something like "CLI commands for Linux and Windows" or "Platform CLI commands" near the declaration of CLI_COMMANDS (Record<ExternalApp, string | null>) so it accurately documents that entries may be used on both Linux and Windows.
75-80: Update comment to reflect cross-platform usage.The comment references "Linux" specifically, but
CLI_CANDIDATESis now also used for Windows. Consider generalizing the documentation./** - * CLI command candidates for JetBrains IDEs with multiple editions on Linux. + * CLI command candidates for JetBrains IDEs with multiple editions (Linux/Windows). * JetBrains Toolbox typically creates `idea`/`pycharm` launchers, * while package managers may use edition-specific names. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts` around lines 75 - 80, The comment above the CLI_CANDIDATES constant incorrectly limits the scope to Linux; update the documentation to reflect cross-platform usage (e.g., mention Linux and Windows or say "cross-platform") and note that CLI_CANDIDATES contains common launcher/CLI names for multiple editions across platforms so its entries apply to Windows as well as Linux (reference: CLI_CANDIDATES, ExternalApp).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/main/lib/terminal/env.ts`:
- Around line 42-43: FALLBACK_SHELL currently resolves to "powershell.exe" on
Windows, which duplicates getDefaultShell() and makes fallback retries a no-op;
change the Windows fallback to a distinct shell such as "cmd.exe" so retries can
differ from the default PowerShell startup path, and apply the same change to
the other occurrence around the getDefaultShell/fallback logic (references:
FALLBACK_SHELL and getDefaultShell()).
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/external/helpers.ts`:
- Around line 45-46: Update the top comment for the CLI_COMMANDS constant to
reflect cross-platform usage instead of "Linux CLI commands": change the comment
to something like "CLI commands for Linux and Windows" or "Platform CLI
commands" near the declaration of CLI_COMMANDS (Record<ExternalApp, string |
null>) so it accurately documents that entries may be used on both Linux and
Windows.
- Around line 75-80: The comment above the CLI_CANDIDATES constant incorrectly
limits the scope to Linux; update the documentation to reflect cross-platform
usage (e.g., mention Linux and Windows or say "cross-platform") and note that
CLI_CANDIDATES contains common launcher/CLI names for multiple editions across
platforms so its entries apply to Windows as well as Linux (reference:
CLI_CANDIDATES, ExternalApp).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53a58c2f-4172-44bf-a3bb-9165b5892d03
📒 Files selected for processing (6)
apps/desktop/scripts/copy-native-modules.tsapps/desktop/src/lib/trpc/routers/external/helpers.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/shell-env.tsapps/desktop/src/main/lib/agent-setup/shell-wrappers.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/renderer/lib/terminal/launch-command.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/scripts/copy-native-modules.ts
- apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts
- apps/desktop/src/renderer/lib/terminal/launch-command.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/terminal-host/client.ts (1)
271-291:⚠️ Potential issue | 🟠 MajorFinish moving the probe-only paths onto
socketMayExist().
tryConnectAndAuthenticate()andwaitForExistingDaemonProbe()still decide “daemon definitely exists” via raw file-existence checks. That makes these helpers inconsistent with the new abstraction and can incorrectly returnfalseon Windows, which then makes callers likelistSessionsIfRunning()andshutdownIfRunning()behave as if no daemon is running.🛠️ Proposed fix
- const socketPathExisted = existsSync(SOCKET_PATH); + const socketPathExisted = socketMayExist(SOCKET_PATH); const connected = await this.tryConnectControl(); if (!connected) { this.resetConnectionState({ emitDisconnected: false }); - if (!socketPathExisted && !existsSync(SOCKET_PATH)) { + if (!socketPathExisted && !socketMayExist(SOCKET_PATH)) { return false; } throw new Error( "Existing terminal daemon probe failed while a socket path was present", ); @@ - if (!existsSync(SOCKET_PATH)) { + if (!socketMayExist(SOCKET_PATH)) { return false; }Also applies to: 320-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal-host/client.ts` around lines 271 - 291, Replace direct filesystem existence checks for SOCKET_PATH in tryConnectAndAuthenticate() (and the similar block around lines 320-348) and in waitForExistingDaemonProbe() with the socketMayExist() abstraction so probe-only logic uses the new platform-aware check; specifically, where the code currently calls existsSync(SOCKET_PATH) to decide “daemon definitely exists” (including the socketPathExisted variable and subsequent conditional branches), call socketMayExist() instead and use its result to decide whether to return false or throw, ensuring callers like listSessionsIfRunning() and shutdownIfRunning() observe the same socket-existence semantics across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/lib/terminal-host/client.ts`:
- Around line 271-291: Replace direct filesystem existence checks for
SOCKET_PATH in tryConnectAndAuthenticate() (and the similar block around lines
320-348) and in waitForExistingDaemonProbe() with the socketMayExist()
abstraction so probe-only logic uses the new platform-aware check; specifically,
where the code currently calls existsSync(SOCKET_PATH) to decide “daemon
definitely exists” (including the socketPathExisted variable and subsequent
conditional branches), call socketMayExist() instead and use its result to
decide whether to return false or throw, ensuring callers like
listSessionsIfRunning() and shutdownIfRunning() observe the same
socket-existence semantics across platforms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04d03348-a6f5-4a20-aeff-ee0c4e33dae0
📒 Files selected for processing (4)
apps/desktop/src/main/lib/terminal-host/client.tsapps/desktop/src/main/lib/terminal/env.tsapps/desktop/src/main/terminal-host/index.tsapps/desktop/src/renderer/lib/terminal/launch-command.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/lib/terminal/launch-command.ts
- apps/desktop/src/main/lib/terminal/env.ts
- Default to DOM renderer on Windows to avoid D3D11/ANGLE context
creation cost (~200-500ms) on every workspace switch. Users can
opt into WebGL via localStorage('terminal-renderer', 'webgl').
- Defer LigaturesAddon init to a macrotask so disk I/O for font
file parsing doesn't block the first terminal render.
- Reduce MAX_SCROLLBACK_BYTES from 500KB to 150KB to cut snapshot
transfer and xterm replay time on reattach.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts:261">
P2: Deferred LigaturesAddon initialization uses an empty catch block, swallowing failures and reducing observability for runtime errors.</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.
Comment on lines
+261
to
263
| } catch { | ||
| // Ligatures not supported by current font | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Deferred LigaturesAddon initialization uses an empty catch block, swallowing failures and reducing observability for runtime errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts, line 261:
<comment>Deferred LigaturesAddon initialization uses an empty catch block, swallowing failures and reducing observability for runtime errors.</comment>
<file context>
@@ -233,13 +241,27 @@ export function createTerminalInstance(
+ if (isDisposed) return;
+ try {
xterm.loadAddon(new LigaturesAddon());
+ } catch {
+ // Ligatures not supported by current font
}
</file context>
Suggested change
| } catch { | |
| // Ligatures not supported by current font | |
| } | |
| } catch (error) { | |
| console.warn("[Terminal] Failed to load LigaturesAddon; continuing without ligatures", error); | |
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts (2)
106-111: Add a small co-located regression test for these new branches.This helper now has two easy-to-regress paths: Windows should still honor an explicit renderer preference, and
cleanup()should prevent the deferred ligature load from firing after disposal.As per coding guidelines, "
**/*.{ts,tsx}: Co-locate tests with their implementation files (e.g.,Component.test.tsxnext toComponent.tsx).`"Also applies to: 244-327
🤖 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/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts` around lines 106 - 111, Add a co-located test file (helpers.test.ts) next to helpers.ts that covers the two regression paths: verify getCurrentPlatform() === "win32" still returns "dom" unless an explicit renderer preference is supplied (mock/override the preference and assert the alternative renderer is used), and test that calling cleanup() prevents the deferred ligature load from firing after disposal (simulate the deferred timer/promise, call cleanup(), advance timers/mocks and assert the ligature loader is not invoked). Use the project's jest/testing utilities and co-locate the test so it follows the "*.{ts,tsx}" convention and also add similar tests for the other branch range (lines 244-327) referenced in the comment.
211-211: UseReturnType<typeof setTimeout>for consistency and platform portability.The desktop renderer's TypeScript config explicitly omits Node types (only includes
"lib": ["esnext", "dom", "dom.iterable"]and"types": ["bun"]), makingNodeJS.Timeoutan unsupported type reference. The codebase already uses the platform-agnosticReturnType<typeof setTimeout>pattern elsewhere (e.g., inuseSectionDropZone.ts,WorkspaceSection.tsx), so this change improves consistency.♻️ Proposed change
- let ligaturesTimeoutId: NodeJS.Timeout | null = null; + let ligaturesTimeoutId: ReturnType<typeof setTimeout> | null = null;🤖 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/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts` at line 211, The variable ligaturesTimeoutId is typed as NodeJS.Timeout which is incompatible with this renderer TS config; change its type to the platform-agnostic ReturnType<typeof setTimeout> to match the project's pattern (used in useSectionDropZone.ts and WorkspaceSection.tsx). Update the declaration of ligaturesTimeoutId to use ReturnType<typeof setTimeout> | null and ensure any assignments from setTimeout remain type-compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts`:
- Around line 106-111: Add a co-located test file (helpers.test.ts) next to
helpers.ts that covers the two regression paths: verify getCurrentPlatform() ===
"win32" still returns "dom" unless an explicit renderer preference is supplied
(mock/override the preference and assert the alternative renderer is used), and
test that calling cleanup() prevents the deferred ligature load from firing
after disposal (simulate the deferred timer/promise, call cleanup(), advance
timers/mocks and assert the ligature loader is not invoked). Use the project's
jest/testing utilities and co-locate the test so it follows the "*.{ts,tsx}"
convention and also add similar tests for the other branch range (lines 244-327)
referenced in the comment.
- Line 211: The variable ligaturesTimeoutId is typed as NodeJS.Timeout which is
incompatible with this renderer TS config; change its type to the
platform-agnostic ReturnType<typeof setTimeout> to match the project's pattern
(used in useSectionDropZone.ts and WorkspaceSection.tsx). Update the declaration
of ligaturesTimeoutId to use ReturnType<typeof setTimeout> | null and ensure any
assignments from setTimeout remain type-compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4175632a-d799-40f1-8965-03e0a8303242
📒 Files selected for processing (2)
apps/desktop/src/main/lib/terminal/daemon/constants.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/lib/terminal/daemon/constants.ts
… Windows
Add windowsHide: true to all child_process.spawn/exec calls so that
background processes (terminal daemon, PTY subprocess, host service,
port scanner) don't create visible CMD windows on Windows. This fixes
the CMD flash and app freeze ("not responding") when switching workspaces.
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.
Guys, I know this is important and needs to be discussed strategically. I initially implemented it FOR MYSELF because I wanted to try out the superset and don't have MacOS available. In any case, this can be merged or even serve as reference material for your future plans.
In practical terms, the main changes here are about making our command and terminal stack behave correctly on Windows: pipes and file-reading now use PowerShell‑friendly syntax instead of assuming a Unix shell, the logic for building agent commands has been centralized so we don’t have to duplicate shell quirks everywhere, and the agent-command helpers now generate cross‑platform invocations (for example using Get-Content on Windows instead of cat).
2026-03-10.08-58-24.mp4
Summary by cubic
Initial Windows support for the desktop app, plus terminal and UX improvements on Windows. Adds a cross‑platform terminal host (Windows named pipes), PowerShell‑first commands, Windows‑safe “open in app,” auto‑updates, and hides background consoles to stop CMD flashes.
New Features
getSocketPath,socketMayExist,isFileBasedSocket) for safe connect/cleanup and spawn waits that don’t rely on on‑disk sockets.-NoLogo,-Command);cmd.exehandled; terminal writes use CR; shell env returns process env on Windows; locale probe skipped.buildAgentFileCommandin@superset/shared/agent-command(usesGet-Content -Rawon Windows,caton Unix); consistent per‑agent flags/suffixes; tests cover Windows/Unix quoting and agents.explorer.exe) with improved path detection (backslashes, drive letters) and CLI candidates.windowsHide: true) to prevent CMD flashes and app freezes.Migration
terminalLaunchConfigSchema.commandis now optional; prefertaskPromptContent+taskPromptFileName.buildAgentFileCommand({ platform: 'win32' | 'unix' })from@superset/shared/agent-command.Written for commit 5464f76. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores