feat(desktop): add project run commands#2511
Conversation
|
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 workspace "run" support: config schemas accept/merge Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Workspace UI
participant Hook as useWorkspaceRunCommand
participant TRPC as electronTrpc
participant Store as TabsStore
participant Lifecycle as useTerminalLifecycle
User->>UI: trigger RUN_WORKSPACE_COMMAND
UI->>Hook: toggleWorkspaceRun()
alt start run
Hook->>TRPC: workspaces.getResolvedRunCommands(workspaceId)
TRPC-->>Hook: { commands }
Hook->>Store: find or create pane
alt reuse pane
Hook->>Store: focus pane
Hook->>Lifecycle: restartTerminal({ command })
Lifecycle->>TRPC: terminal.createOrAttach({ command, skipColdRestore: true })
TRPC-->>Lifecycle: attached
Lifecycle->>Store: setPaneWorkspaceRun(paneId, { state: "running" })
else new pane
Hook->>Store: addTab/addPane(...)
Hook->>TRPC: terminal.createOrAttach({ command })
TRPC-->>Hook: session created
Hook->>Store: setPaneWorkspaceRun(paneId, { state: "running" })
end
else stop run
Hook->>Store: find running pane
Hook->>TRPC: terminal.kill({ paneId })
TRPC-->>Hook: killed
Hook->>Store: setPaneWorkspaceRun(paneId, { state: "stopped-by-user" })
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
5 issues found across 31 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx:334">
P2: Run-status indicator is incorrectly hidden for single-line worktree items whose name matches the branch. The indicator should only depend on `workspaceRunState`, not `showBranchSubtitle`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts:67">
P2: Async query loading state is misreported as 'no command configured'</violation>
</file>
<file name="apps/desktop/src/main/terminal-host/session.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:254">
P1: The new `getCommandShellArgs` helper is Unix-centric and will break on Windows. It only explicitly handles `zsh` and `bash`, falling back to POSIX-style `-lc` arguments for all other shells. On Windows, `cmd.exe` uses `/c` (not `-lc`) and PowerShell uses `-Command` (not `-lc`), so run commands will fail when the default Windows shell is used.</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts:52">
P2: The validation for 'run' arrays only checks that values are arrays, but doesn't validate that array elements are strings before shell execution. Error messages claim 'must be an array of strings' but the actual check only validates the container type, not element types.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts:348">
P2: Empty catch block swallows terminal.kill errors without logging. Replace `.catch(() => undefined)` with a handler that logs at minimum a warning (e.g., `.catch(err => { console.warn('[Terminal] Kill failed:', err); })`) to preserve observability while still allowing the attach to proceed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/config/config.ts (1)
447-487:⚠️ Potential issue | 🟡 MinorKeep the setup-card predicate in sync with the new
runfield.This mutation can now persist
run, butshouldShowSetupCard()still derives fromhasConfiguredScripts(), which only checkssetupandteardown. A project with only a run command still looks “unconfigured” to that flow.🩹 Suggested fix
function hasConfiguredScripts( project: Pick<SelectProject, "id" | "mainRepoPath">, ) { const config = loadSetupConfig({ mainRepoPath: project.mainRepoPath, projectId: project.id, }); const setup = Array.isArray(config?.setup) ? config.setup.filter( (s): s is string => typeof s === "string" && s.trim().length > 0, ) : []; const teardown = Array.isArray(config?.teardown) ? config.teardown.filter( (s): s is string => typeof s === "string" && s.trim().length > 0, ) : []; - return setup.length > 0 || teardown.length > 0; + const run = Array.isArray(config?.run) + ? config.run.filter( + (s): s is string => typeof s === "string" && s.trim().length > 0, + ) + : []; + return setup.length > 0 || teardown.length > 0 || run.length > 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/config/config.ts` around lines 447 - 487, The updateConfig mutation now persists a run field but the UI predicate shouldShowSetupCard()/hasConfiguredScripts() still only checks setup and teardown; update hasConfiguredScripts (or shouldShowSetupCard) to also consider a non-empty run array (e.g., return true if setup.length>0 || teardown.length>0 || (run && run.length>0)), and ensure any code that reads project config (where existingConfig is merged in updateConfig) correctly preserves run so the predicate reads the new value.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsx (1)
46-57: Expose run status to assistive tech.Lines 46-93 render icon-only status indicators without an accessible label. Add an
aria-label(and optionallytitle) so run state is announced non-visually.Suggested fix
export function WorkspaceRunIndicator({ className, state, variant = "circle", }: WorkspaceRunIndicatorProps) { + const stateLabel = + state === "running" + ? "Workspace run command running" + : state === "stopped-by-user" + ? "Workspace run command stopped" + : "Workspace run command crashed"; + const icon = state === "running" ? ( <HiMiniPlay className="size-[0.45rem] translate-x-[0.5px]" /> @@ <span className={cn( "flex size-3 items-center justify-center rounded-full text-white ring-1 ring-background shadow-sm", colorClasses, className, )} + aria-label={stateLabel} + title={stateLabel} > {icon} </span> @@ <span className={cn( "flex items-center justify-center", toolbarColorClasses, className, )} + aria-label={stateLabel} + title={stateLabel} > {toolbarIcon} </span> @@ <span className={cn( "flex h-2.5 w-5 items-center justify-center rounded-[2px]", inlineColorClasses, className, )} + aria-label={stateLabel} + title={stateLabel} > {icon} </span> ); }Also applies to: 60-79, 83-93
🤖 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/WorkspaceRunIndicator/WorkspaceRunIndicator.tsx` around lines 46 - 57, The icon-only status spans in the WorkspaceRunIndicator component (e.g., the branch handling variant === "circle" that returns a span containing {icon}) lack accessible labels; update those returned spans (including the other icon-only branches) to include an appropriate aria-label (e.g., aria-label={`Run status: ${status}`} or similar) and optionally a title attribute, using the component's run state prop (status/variant) to build the label; ensure you add the attributes to the same span elements that use className, colorClasses, and icon so screen readers announce the run state.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/mosaic-theme.css (1)
140-164: Tighten selector scope to avoid accidental style bleed.Using
[class*="workspace-run-pane-"]is broad. Restricting to.mosaic-window.workspace-run-pane-*keeps the styling targeted.🎯 Suggested selector tightening
-:is(.mosaic-theme-dark, .mosaic-theme-light) [class*="workspace-run-pane-"] { +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-running, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-user, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-exit { border-color: color-mix( in srgb, var(--ws-run-color) 25%, var(--color-border) ); } :is(.mosaic-theme-dark, .mosaic-theme-light) - [class*="workspace-run-pane-"] + .mosaic-window.workspace-run-pane-running + .mosaic-window-toolbar, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-user + .mosaic-window-toolbar, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-exit .mosaic-window-toolbar { background: color-mix( in srgb, var(--ws-run-color) 12%, var(--color-tertiary) ); } :is(.mosaic-theme-dark, .mosaic-theme-light) - [class*="workspace-run-pane-"].mosaic-window-focused + .mosaic-window.workspace-run-pane-running.mosaic-window-focused + .mosaic-window-toolbar, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-user.mosaic-window-focused + .mosaic-window-toolbar, +:is(.mosaic-theme-dark, .mosaic-theme-light) + .mosaic-window.workspace-run-pane-stopped-by-exit.mosaic-window-focused .mosaic-window-toolbar { background: color-mix( in srgb, var(--ws-run-color) 12%, var(--color-secondary) ); }🤖 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/TabView/mosaic-theme.css` around lines 140 - 164, The selectors using [class*="workspace-run-pane-"] are too broad and can leak styles; tighten them to target mosaic window elements only by restricting to the mosaic window class and the workspace-run-pane prefix (e.g. use the .mosaic-window element combined with a workspace-run-pane-* class) for the rules that reference .mosaic-window-toolbar and .mosaic-window-focused so only actual mosaic windows receive the border-color and background color-mix styles; update each selector that currently mentions [class*="workspace-run-pane-"] (the three rules affecting .mosaic-window-toolbar and .mosaic-window-focused) to the stricter .mosaic-window + workspace-run-pane-* form while keeping the same color-mix declarations.
🤖 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/terminal/terminal.ts`:
- Around line 68-69: The "command" input currently allows blank or
whitespace-only strings; update the terminal router's input schema (the command
property) to trim input and reject empty commands at the API boundary by adding
a transformation/validation (e.g., trim then require length > 0) so that
whitespace-only strings are considered invalid and return a validation error
rather than creating a no-op terminal session; ensure this change is applied to
the same schema where "command: z.string().optional()" is declared and keep
skipColdRestore handling unchanged.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts`:
- Around line 52-54: The current validation only checks that parsed.run is an
array but not that its elements are strings; update the validation in setup.ts
(the parsed.run checks) to ensure parsed.run.every(item => typeof item ===
'string') and throw a descriptive Error if any element is not a string; apply
the same strict element-type check to the other run-array validation block
around lines 80-98 so malformed non-string entries are rejected before
run-command execution.
In `@apps/desktop/src/main/terminal-host/session.test.ts`:
- Around line 123-126: The test currently only asserts the absence of "--rcfile"
but must explicitly verify the intended "-lc" flag; update the assertions that
examine spawnPayload?.args (used with getCommandShellArgs/getShellArgs) to
assert that "-lc" is present (e.g., expect(spawnPayload?.args).toContain("-lc")
or expect(spawnPayload?.args?.join(" ")).toMatch(/(^|\s)-lc(\s|$)/)) and still
assert the command string "echo hello && exit 1" is included, so the test
guarantees the exact "-lc" form is used rather than a weaker "-c" or other
variant.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/hooks/useWorkspaceRunCommand.ts:
- Around line 47-48: The toggleWorkspaceRun callback is racey because isPending
only guards terminal.kill while the start/restart branch reads runPane/isRunning
from the previous render, allowing two quick toggles to both start/restart the
run; fix by making the start/restart path non-reentrant: introduce a local
mutex/atomic flag (e.g. isStartingRef) checked and set at the top of
toggleWorkspaceRun so subsequent invocations early-return until the
start/restart flow completes, and ensure you atomically read current
isRunning/runPane state inside the locked section before calling runPane or
restart logic and clear the flag on completion or error; reference
functions/vars to update: toggleWorkspaceRun, isPending, terminal.kill, runPane,
isRunning.
- Around line 18-22: The missing-config toast is shown while
getResolvedRunCommands is still loading because runConfig is undefined; update
useWorkspaceRunCommand to read the query's loading state (isLoading or
isFetching) from electronTrpc.workspaces.getResolvedRunCommands.useQuery and
only treat a null/undefined result as "not configured" after the query has
finished; specifically, guard the call sites that use buildTerminalCommand(...)
(and the shortcut handler logic around lines 67-74) so they skip showing the
missing-config toast while isLoading/isFetching is true and only show it when
the query is settled and buildTerminalCommand still returns null.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/project/`$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx:
- Around line 176-184: The effect that rehydrates scripts only runs when
configData?.content is truthy, so when switching to a project without a config
the previous setup/teardown/run state is retained; update the useEffect that
currently watches configData?.content (the one that calls
parseContentFromConfig, setSetupContent, setTeardownContent, setRunContent,
setHasChanges) to handle the falsy case by explicitly clearing local script
state (e.g. setSetupContent(''), setTeardownContent(''), setRunContent('')) and
resetting setHasChanges(false) when configData or configData.content is absent
so stale scripts are not carried into a new project.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 334-336: The run badge is gated by showBranchSubtitle causing it
not to render for single-line items; change the conditional so
WorkspaceRunIndicator is rendered whenever workspaceRunState is truthy (not tied
to showBranchSubtitle). Locate the JSX that currently uses {workspaceRunState &&
showBranchSubtitle && (<WorkspaceRunIndicator state={workspaceRunState}
variant="inline" />)} and update it to render WorkspaceRunIndicator based solely
on workspaceRunState (keeping the same props/variant), so the indicator appears
even when showBranchSubtitle is false.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 323-341: The restart success path (onSuccess in the restart
mutation where pendingInitialStateRef.current is set and
maybeApplyInitialState() is called) must clear any stale connection error so
input handlers can resume; call setConnectionError(null) (and if used, also
clear connectionErrorRef.current) inside the onSuccess callback before resolve()
so the pane becomes interactive after a successful restart.
In `@apps/desktop/src/renderer/stores/tabs/store.ts`:
- Around line 2092-2100: Currently hydration unconditionally converts
pane.workspaceRun.state "running" to "stopped-by-exit", which hides still-live
runs; instead, during the hydration path that touches pane.workspaceRun, stop
coercing the state — call terminal.getSession (as useTerminalLifecycle does) to
check whether the session actually exists and only set pane.workspaceRun.state =
"stopped-by-exit" when terminal.getSession confirms the session is gone; keep
the original "running" state otherwise so components like the sidebar badge and
useWorkspaceRunCommand see the true live state until a liveness check proves it
stopped.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/config/config.ts`:
- Around line 447-487: The updateConfig mutation now persists a run field but
the UI predicate shouldShowSetupCard()/hasConfiguredScripts() still only checks
setup and teardown; update hasConfiguredScripts (or shouldShowSetupCard) to also
consider a non-empty run array (e.g., return true if setup.length>0 ||
teardown.length>0 || (run && run.length>0)), and ensure any code that reads
project config (where existingConfig is merged in updateConfig) correctly
preserves run so the predicate reads the new value.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsx`:
- Around line 46-57: The icon-only status spans in the WorkspaceRunIndicator
component (e.g., the branch handling variant === "circle" that returns a span
containing {icon}) lack accessible labels; update those returned spans
(including the other icon-only branches) to include an appropriate aria-label
(e.g., aria-label={`Run status: ${status}`} or similar) and optionally a title
attribute, using the component's run state prop (status/variant) to build the
label; ensure you add the attributes to the same span elements that use
className, colorClasses, and icon so screen readers announce the run state.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/mosaic-theme.css`:
- Around line 140-164: The selectors using [class*="workspace-run-pane-"] are
too broad and can leak styles; tighten them to target mosaic window elements
only by restricting to the mosaic window class and the workspace-run-pane prefix
(e.g. use the .mosaic-window element combined with a workspace-run-pane-* class)
for the rules that reference .mosaic-window-toolbar and .mosaic-window-focused
so only actual mosaic windows receive the border-color and background color-mix
styles; update each selector that currently mentions
[class*="workspace-run-pane-"] (the three rules affecting .mosaic-window-toolbar
and .mosaic-window-focused) to the stricter .mosaic-window +
workspace-run-pane-* form while keeping the same color-mix declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3009d0a-577d-4c69-aeff-12774a585c56
📒 Files selected for processing (31)
apps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/main/lib/terminal-host/types.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/TabPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/mosaic-theme.cssapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/terminal-callbacks.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/shared/tabs-types.tsapps/desktop/src/shared/types/config.ts
cd362b4 to
07ea67f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
334-336:⚠️ Potential issue | 🟠 MajorRender run indicator whenever run state exists (not only with subtitle).
On Line 334, tying the indicator to
showBranchSubtitlehides run status for expanded single-line items, so some active run states become invisible in the sidebar.🩹 Suggested fix
- {workspaceRunState && showBranchSubtitle && ( + {workspaceRunState && ( <WorkspaceRunIndicator state={workspaceRunState} variant="inline" /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 334 - 336, The run indicator is currently gated by both workspaceRunState and showBranchSubtitle, which hides run status for expanded single-line items; update the conditional in WorkspaceListItem so WorkspaceRunIndicator is rendered whenever workspaceRunState is truthy (remove the showBranchSubtitle check) — keep the existing props (e.g., state={workspaceRunState} variant="inline") and relocate the JSX so the indicator is not dependent on showBranchSubtitle.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
140-146: Prefer shared run-state type reuse to avoid drift.On Line 144, the inline
"running" | "stopped-by-user" | "stopped-by-exit"union duplicates shared domain typing. Reusing the shared alias keeps store API and pane model in lockstep.♻️ Suggested refactor
import type { BaseTab, BaseTabsState, BrowserLoadError, ChatMastraLaunchConfig, FileViewerMode, Pane, PaneStatus, PaneType, + WorkspaceRunState, ViewportPreset, } from "shared/tabs-types"; ... setPaneWorkspaceRun: ( paneId: string, workspaceRun: { workspaceId: string; - state: "running" | "stopped-by-user" | "stopped-by-exit"; + state: WorkspaceRunState; } | null, ) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/types.ts` around lines 140 - 146, Replace the inline union type in the setPaneWorkspaceRun signature with the project’s shared run-state alias: import the shared type (e.g., RunState or WorkspaceRunState) used elsewhere for pane/workspace models and use it for workspaceRun.state instead of the literal "running" | "stopped-by-user" | "stopped-by-exit"; update the function signature for setPaneWorkspaceRun and its workspaceRun parameter to reference that imported type so the store API stays in sync with the pane model.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx (1)
80-87: Consider avoiding full pane scans in each sidebar row selector.Lines 80–87 iterate all panes per
WorkspaceListItemrender subscription. With many workspaces/panes this can turn frequent pane updates into expensive sidebar recomputation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx` around lines 80 - 87, The current selector in WorkspaceListItem scans state.panes for every item on each pane update (useTabsStore -> state.panes loop), which is expensive; change the store or selector to avoid per-item full scans by exposing a direct lookup (e.g., add a workspaceRunByWorkspaceId map or a selector method like getWorkspaceRunState(workspaceId) on the tabs store) and then read that map/selector in WorkspaceListItem instead of iterating panes; update useTabsStore call in WorkspaceListItem to select state.workspaceRunByWorkspaceId[id] or state.getWorkspaceRunState(id) and maintain/keep the map updated within the tabs store when panes change (update add/remove pane logic to update the index).
🤖 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/workspaces/utils/setup.ts`:
- Around line 52-54: The current guard if (parsed.run &&
!Array.isArray(parsed.run)) allows falsy non-array values (false, 0, "", null)
to slip through; change the check to explicitly detect presence (not just
truthiness) and reject any non-array type, e.g. if (parsed.run != null &&
!Array.isArray(parsed.run)) throw new Error("'run' field must be an array of
strings"); this ensures parsed.run is either undefined/null or a real array
before passing into getResolvedRunCommands and mergeConfigs (look for parsed.run
usage and the functions getResolvedRunCommands and mergeConfigs in the diff).
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts`:
- Around line 64-73: The pane workspace-run state transition runs
unconditionally and can overwrite an intentional stopped-by-user state; before
calling useTabsStore.getState().setPaneWorkspaceRun(paneId, ...), check that
currentPaneForRun.workspaceRun.state === "running" (i.e., only transition if the
pane is still "running"); use the existing variables currentPaneForRun,
wasKilledByUser, paneId and preserve the computed nextState ("stopped-by-user"
vs "stopped-by-exit") but only apply it when the current workspaceRun.state
equals "running".
---
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 334-336: The run indicator is currently gated by both
workspaceRunState and showBranchSubtitle, which hides run status for expanded
single-line items; update the conditional in WorkspaceListItem so
WorkspaceRunIndicator is rendered whenever workspaceRunState is truthy (remove
the showBranchSubtitle check) — keep the existing props (e.g.,
state={workspaceRunState} variant="inline") and relocate the JSX so the
indicator is not dependent on showBranchSubtitle.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx`:
- Around line 80-87: The current selector in WorkspaceListItem scans state.panes
for every item on each pane update (useTabsStore -> state.panes loop), which is
expensive; change the store or selector to avoid per-item full scans by exposing
a direct lookup (e.g., add a workspaceRunByWorkspaceId map or a selector method
like getWorkspaceRunState(workspaceId) on the tabs store) and then read that
map/selector in WorkspaceListItem instead of iterating panes; update
useTabsStore call in WorkspaceListItem to select
state.workspaceRunByWorkspaceId[id] or state.getWorkspaceRunState(id) and
maintain/keep the map updated within the tabs store when panes change (update
add/remove pane logic to update the index).
In `@apps/desktop/src/renderer/stores/tabs/types.ts`:
- Around line 140-146: Replace the inline union type in the setPaneWorkspaceRun
signature with the project’s shared run-state alias: import the shared type
(e.g., RunState or WorkspaceRunState) used elsewhere for pane/workspace models
and use it for workspaceRun.state instead of the literal "running" |
"stopped-by-user" | "stopped-by-exit"; update the function signature for
setPaneWorkspaceRun and its workspaceRun parameter to reference that imported
type so the store API stays in sync with the pane model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3175413-b93c-4ace-96de-5d3183a14675
📒 Files selected for processing (31)
apps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/main/lib/terminal-host/types.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/TabPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/mosaic-theme.cssapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/terminal-callbacks.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/shared/tabs-types.tsapps/desktop/src/shared/types/config.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/desktop/src/main/lib/terminal-host/types.ts
- apps/desktop/src/lib/trpc/routers/config/config.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/TabPane.tsx
- apps/desktop/src/shared/types/config.ts
- apps/desktop/src/shared/tabs-types.ts
- apps/desktop/src/renderer/routes/_authenticated/layout.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/main/terminal-host/session.test.ts
| if (parsed.run && !Array.isArray(parsed.run)) { | ||
| throw new Error("'run' field must be an array of strings"); | ||
| } |
There was a problem hiding this comment.
Reject falsy non-array run values.
if (parsed.run && !Array.isArray(parsed.run)) still lets false, 0, "", and null through. That leaks an invalid run shape into getResolvedRunCommands, and with a local run.before/after overlay it can also make mergeConfigs throw when it spreads base[key].
🛠️ Proposed fix
- if (parsed.run && !Array.isArray(parsed.run)) {
+ if (parsed.run !== undefined && !Array.isArray(parsed.run)) {
throw new Error("'run' field must be an array of strings");
}📝 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.
| if (parsed.run && !Array.isArray(parsed.run)) { | |
| throw new Error("'run' field must be an array of strings"); | |
| } | |
| if (parsed.run !== undefined && !Array.isArray(parsed.run)) { | |
| throw new Error("'run' field must be an array of strings"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts` around lines 52
- 54, The current guard if (parsed.run && !Array.isArray(parsed.run)) allows
falsy non-array values (false, 0, "", null) to slip through; change the check to
explicitly detect presence (not just truthiness) and reject any non-array type,
e.g. if (parsed.run != null && !Array.isArray(parsed.run)) throw new
Error("'run' field must be an array of strings"); this ensures parsed.run is
either undefined/null or a real array before passing into getResolvedRunCommands
and mergeConfigs (look for parsed.run usage and the functions
getResolvedRunCommands and mergeConfigs in the diff).
| const currentPaneForRun = useTabsStore.getState().panes[paneId]; | ||
| if (currentPaneForRun?.workspaceRun) { | ||
| const nextState = wasKilledByUser | ||
| ? "stopped-by-user" | ||
| : "stopped-by-exit"; | ||
| useTabsStore.getState().setPaneWorkspaceRun(paneId, { | ||
| workspaceId: currentPaneForRun.workspaceRun.workspaceId, | ||
| state: nextState, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Only transition workspace-run panes out of "running" once.
layout.tsx already guards this same transition on pane.workspaceRun.state === "running". Without the same check here, a late/duplicate exit can overwrite an intentional stopped-by-user state with stopped-by-exit.
🩹 Proposed fix
const currentPaneForRun = useTabsStore.getState().panes[paneId];
- if (currentPaneForRun?.workspaceRun) {
+ const workspaceRun = currentPaneForRun?.workspaceRun;
+ if (workspaceRun?.state === "running") {
const nextState = wasKilledByUser
? "stopped-by-user"
: "stopped-by-exit";
useTabsStore.getState().setPaneWorkspaceRun(paneId, {
- workspaceId: currentPaneForRun.workspaceRun.workspaceId,
+ workspaceId: workspaceRun.workspaceId,
state: nextState,
});
}🤖 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/hooks/useTerminalStream.ts`
around lines 64 - 73, The pane workspace-run state transition runs
unconditionally and can overwrite an intentional stopped-by-user state; before
calling useTabsStore.getState().setPaneWorkspaceRun(paneId, ...), check that
currentPaneForRun.workspaceRun.state === "running" (i.e., only transition if the
pane is still "running"); use the existing variables currentPaneForRun,
wasKilledByUser, paneId and preserve the computed nextState ("stopped-by-user"
vs "stopped-by-exit") but only apply it when the current workspaceRun.state
equals "running".
07ea67f to
1009bcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/terminal-host/session.ts (1)
202-256:⚠️ Potential issue | 🟠 MajorPreserve buffered output before making command exit the happy path.
With
commandsessions, normal completion now goes throughhandleSubprocessExit(). That method on Lines 351-370 immediately callsresetProcessState(), andresetProcessState()on Lines 849-867 clearsemulatorWriteQueue. AnyDataframes that arrived just before exit but are still waiting for thesetImmediatedrain never make it into the emulator snapshot, so exited/crashed workspace-run panes can lose their final log lines.Possible fix
private handleSubprocessExit(exitCode: number): void { if (this.exitCode === null) { this.exitCode = exitCode; this.broadcastEvent("exit", { type: "exit", exitCode, } satisfies TerminalExitEvent); this.onSessionExit?.(this.sessionId, exitCode); } + if (!this.disposed) { + this.flushPendingEmulatorWritesSync(); + } + // Ensure waiters don't hang forever if the subprocess exits before sending Spawned. // Callers must still check isAlive before writing. if (this.ptyReadyResolve) { this.ptyReadyResolve(); this.ptyReadyResolve = null; @@ this.resetProcessState(); } + +private flushPendingEmulatorWritesSync(): void { + const MAX_CHUNK_CHARS = 8192; + + while (this.emulatorWriteQueue.length > 0) { + let chunk = this.emulatorWriteQueue[0]; + if (chunk.length > MAX_CHUNK_CHARS) { + this.emulatorWriteQueue[0] = chunk.slice(MAX_CHUNK_CHARS); + chunk = chunk.slice(0, MAX_CHUNK_CHARS); + } else { + this.emulatorWriteQueue.shift(); + this.emulatorWriteProcessedItems++; + } + + this.emulatorWriteQueuedBytes -= chunk.length; + this.emulator.write(chunk); + } + + this.resolveReachedSnapshotBoundaryWaiters(); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 202 - 256, handleSubprocessExit is currently making process exit the "happy path" immediately which calls resetProcessState and clears emulatorWriteQueue before any pending Data frames (queued via setImmediate) are drained; update handleSubprocessExit (or its caller) to delay clearing state until those pending tasks run — e.g., make handleSubprocessExit asynchronous or wrap the reset call in setImmediate (or await new Promise(r => setImmediate(r))) so any pending frames from subprocessDecoder and writes to emulatorWriteQueue are processed into the emulator snapshot before resetProcessState() clears the queue.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts (1)
324-329: Consider reusinggetWorkspacePathhelper for consistency.The inline worktree path resolution duplicates logic that may already exist in the
getWorkspacePathhelper (imported at line 16 and used at line 68). Using the helper would reduce code duplication and ensure consistent path resolution across procedures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts` around lines 324 - 329, The inline resolution for worktreePath duplicates existing logic; replace the ternary block that computes worktreePath with a call to the existing helper getWorkspacePath (which is already imported and used elsewhere) so the procedure uses getWorkspacePath(workspace, worktree, project) or the appropriate parameter set to return the path; ensure you pass the same values used in the current ternary (workspace.type, worktree?.path, project.mainRepoPath) so behavior remains identical and remove the duplicated logic.
🤖 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/workspaces/procedures/query.ts`:
- Around line 295-305: The get and getResolvedRunCommands procedures are
querying workspaces without excluding ones pending deletion; update both to
filter out deleted workspaces by using the existing helper
getWorkspaceNotDeleting() from db-helpers.ts (or add
isNull(workspaces.deletingAt) to their queries) so they behave consistently with
getAllGrouped; replace calls to getWorkspace or direct selects on workspaces in
get and getResolvedRunCommands with calls to getWorkspaceNotDeleting() (or add
the isNull(workspaces.deletingAt) clause) and ensure subsequent logic still uses
the returned workspace object.
---
Outside diff comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 202-256: handleSubprocessExit is currently making process exit the
"happy path" immediately which calls resetProcessState and clears
emulatorWriteQueue before any pending Data frames (queued via setImmediate) are
drained; update handleSubprocessExit (or its caller) to delay clearing state
until those pending tasks run — e.g., make handleSubprocessExit asynchronous or
wrap the reset call in setImmediate (or await new Promise(r => setImmediate(r)))
so any pending frames from subprocessDecoder and writes to emulatorWriteQueue
are processed into the emulator snapshot before resetProcessState() clears the
queue.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts`:
- Around line 324-329: The inline resolution for worktreePath duplicates
existing logic; replace the ternary block that computes worktreePath with a call
to the existing helper getWorkspacePath (which is already imported and used
elsewhere) so the procedure uses getWorkspacePath(workspace, worktree, project)
or the appropriate parameter set to return the path; ensure you pass the same
values used in the current ternary (workspace.type, worktree?.path,
project.mainRepoPath) so behavior remains identical and remove the duplicated
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6738b750-2ae2-4ee6-b40a-63d3822a511e
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/main/lib/terminal-host/types.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/main/terminal-host/session.test.ts
- apps/desktop/src/main/lib/terminal-host/types.ts
- apps/desktop/src/lib/trpc/routers/ui-state/index.ts
- apps/desktop/src/lib/trpc/routers/config/config.ts
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
| const workspace = localDb | ||
| .select() | ||
| .from(workspaces) | ||
| .where(eq(workspaces.id, input.workspaceId)) | ||
| .get(); | ||
| if (!workspace) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: `Workspace ${input.workspaceId} not found`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if getWorkspace helper filters by deletingAt
ast-grep --pattern $'function getWorkspace($_) {
$$$
}'
rg -n -A 10 'export.*function getWorkspace|export const getWorkspace' --type tsRepository: superset-sh/superset
Length of output: 17111
🏁 Script executed:
# Check the actual code at line 44 of query.ts and around the getResolvedRunCommands procedure
sed -n '40,50p' apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
# Also check the getResolvedRunCommands procedure around lines 291-340
sed -n '291,340p' apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsRepository: superset-sh/superset
Length of output: 1506
🏁 Script executed:
# Check the getAllGrouped procedure to see if it actually filters by deletingAt
sed -n '190,210p' apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsRepository: superset-sh/superset
Length of output: 684
Both get and getResolvedRunCommands lack workspace deletion filtering.
The get procedure uses getWorkspace (line 44), which does not filter by deletingAt. Similarly, getResolvedRunCommands (line 295) queries directly without a deletion filter. However, getAllGrouped (line 198) explicitly filters isNull(workspaces.deletingAt). This inconsistency means both procedures could operate on workspaces pending deletion, unlike getAllGrouped.
Consider using getWorkspaceNotDeleting() helper (available in db-helpers.ts) or adding isNull(workspaces.deletingAt) to both queries for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts` around
lines 295 - 305, The get and getResolvedRunCommands procedures are querying
workspaces without excluding ones pending deletion; update both to filter out
deleted workspaces by using the existing helper getWorkspaceNotDeleting() from
db-helpers.ts (or add isNull(workspaces.deletingAt) to their queries) so they
behave consistently with getAllGrouped; replace calls to getWorkspace or direct
selects on workspaces in get and getResolvedRunCommands with calls to
getWorkspaceNotDeleting() (or add the isNull(workspaces.deletingAt) clause) and
ensure subsequent logic still uses the returned workspace object.
Add project-level run command support with a keyboard shortcut (Ctrl+R) to start/stop a dev server from any workspace. - Run command configured in Project Settings > Scripts alongside setup/teardown - Single source of truth: pane.workspaceRun metadata drives all state - Pane reused across restarts and survives app reload - Visual indicators in sidebar (colored bar) and pane toolbar (border + header tint) - Terminal exits when command finishes (uses shell -c, not interactive write) - State persisted via ui-state pane schema for reload recovery
|
Rebased locally onto current main and added follow-up commit |
1009bcf to
bb729cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts (1)
525-541:⚠️ Potential issue | 🟠 MajorRollback
workspaceRun.statewhen the first attach fails.New run panes are marked
"running"before the initialcreateOrAttach. If that attach errors here, the code only setsconnectionError, so the pane stays green and the next toggle tries to stop a session that never started.💡 Minimal fix
onError: (error) => { if (!isAttachActive()) return; + const workspaceRun = + useTabsStore.getState().panes[paneId]?.workspaceRun; if (error.message?.includes("TERMINAL_SESSION_KILLED")) { + if (workspaceRun) { + useTabsStore.getState().setPaneWorkspaceRun(paneId, { + workspaceId: workspaceRun.workspaceId, + state: "stopped-by-user", + }); + } wasKilledByUserRef.current = true; isExitedRef.current = true; isStreamReadyRef.current = false; setExitStatus("killed"); setConnectionError(null); return; } console.error("[Terminal] Failed to create/attach:", error); + if (workspaceRun) { + useTabsStore.getState().setPaneWorkspaceRun(paneId, { + workspaceId: workspaceRun.workspaceId, + state: "stopped-by-exit", + }); + } setConnectionError( error.message || "Failed to connect to terminal", ); isStreamReadyRef.current = true; flushPendingEvents(); },🤖 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/hooks/useTerminalLifecycle.ts` around lines 525 - 541, When the initial create/attach fails in the onError handler, roll back the optimistic workspace run state so the pane doesn't remain "running": after setting connection error (the setConnectionError call) in the non-TERMINAL_SESSION_KILLED branch, reset the optimistic state on the run (e.g. set workspaceRun.state = "idle" or call the updater used elsewhere like setWorkspaceRunState(workspaceRun.id, "idle") if available) before calling isStreamReadyRef.current = true and flushPendingEvents(); keep the TERMINAL_SESSION_KILLED branch unchanged.
🤖 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/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/hooks/useWorkspaceRunCommand.ts:
- Around line 37-43: The selector for runPane can pick up panes that are already
closed; update the useTabsStore selector (the function that computes runPane) to
ignore closed/destroyed panes by excluding any pane where a closed flag is
truthy (e.g., pane.closed === true or pane.destroyed === true) before matching
type === "terminal" and workspaceRun?.workspaceId === workspaceId; this ensures
runPane only returns an active terminal pane for the workspace and not an
orphaned/closed one.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.ts`:
- Around line 525-541: When the initial create/attach fails in the onError
handler, roll back the optimistic workspace run state so the pane doesn't remain
"running": after setting connection error (the setConnectionError call) in the
non-TERMINAL_SESSION_KILLED branch, reset the optimistic state on the run (e.g.
set workspaceRun.state = "idle" or call the updater used elsewhere like
setWorkspaceRunState(workspaceRun.id, "idle") if available) before calling
isStreamReadyRef.current = true and flushPendingEvents(); keep the
TERMINAL_SESSION_KILLED branch unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 766a0808-4c86-4d31-ac74-288c775785c7
📒 Files selected for processing (32)
apps/desktop/src/lib/trpc/routers/config/config.tsapps/desktop/src/lib/trpc/routers/terminal/terminal.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/lib/trpc/routers/workspaces/procedures/query.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/setup.tsapps/desktop/src/main/lib/terminal-host/types.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/types.tsapps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/TabPane.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/mosaic-theme.cssapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/tabs/terminal-callbacks.tsapps/desktop/src/renderer/stores/tabs/types.tsapps/desktop/src/shared/constants.tsapps/desktop/src/shared/hotkeys.tsapps/desktop/src/shared/tabs-types.tsapps/desktop/src/shared/types/config.ts
✅ Files skipped from review due to trivial changes (10)
- apps/desktop/src/main/lib/terminal/types.ts
- apps/desktop/src/lib/trpc/routers/ui-state/index.ts
- apps/desktop/src/shared/constants.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/index.ts
- apps/desktop/src/shared/types/config.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/components/BasePaneWindow/BasePaneWindow.tsx
- apps/desktop/src/shared/tabs-types.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/setup.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- apps/desktop/src/main/lib/terminal-host/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/TabPane.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceListItem/WorkspaceListItem.tsx
- apps/desktop/src/main/lib/terminal/daemon/daemon-manager.ts
- apps/desktop/src/renderer/stores/tabs/types.ts
- apps/desktop/src/shared/hotkeys.ts
- apps/desktop/src/renderer/routes/_authenticated/layout.tsx
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
- apps/desktop/src/main/terminal-host/session.test.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalStream.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceRunIndicator/WorkspaceRunIndicator.tsx
- apps/desktop/src/renderer/stores/tabs/store.ts
- apps/desktop/src/lib/trpc/routers/workspaces/procedures/query.ts
- apps/desktop/src/renderer/stores/tabs/terminal-callbacks.ts
- apps/desktop/src/main/terminal-host/session.ts
| const runPane = useTabsStore((s) => { | ||
| const pane = Object.values(s.panes).find( | ||
| (p) => | ||
| p.type === "terminal" && p.workspaceRun?.workspaceId === workspaceId, | ||
| ); | ||
| return pane ?? null; | ||
| }); |
There was a problem hiding this comment.
Ignore closed panes when resolving runPane.
Closed panes can still be present in s.panes during destroy cleanup, so this selector can latch onto an orphaned workspace-run pane and send the next toggle down the reuse path instead of creating the replacement pane after close.
💡 Minimal fix
const runPane = useTabsStore((s) => {
+ const liveTabIds = new Set(s.tabs.map((t) => t.id));
const pane = Object.values(s.panes).find(
(p) =>
p.type === "terminal" && p.workspaceRun?.workspaceId === workspaceId,
+ liveTabIds.has(p.tabId),
);
return pane ?? null;
});📝 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 runPane = useTabsStore((s) => { | |
| const pane = Object.values(s.panes).find( | |
| (p) => | |
| p.type === "terminal" && p.workspaceRun?.workspaceId === workspaceId, | |
| ); | |
| return pane ?? null; | |
| }); | |
| const runPane = useTabsStore((s) => { | |
| const liveTabIds = new Set(s.tabs.map((t) => t.id)); | |
| const pane = Object.values(s.panes).find( | |
| (p) => | |
| p.type === "terminal" && p.workspaceRun?.workspaceId === workspaceId && liveTabIds.has(p.tabId), | |
| ); | |
| return pane ?? null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/hooks/useWorkspaceRunCommand.ts
around lines 37 - 43, The selector for runPane can pick up panes that are
already closed; update the useTabsStore selector (the function that computes
runPane) to ignore closed/destroyed panes by excluding any pane where a closed
flag is truthy (e.g., pane.closed === true or pane.destroyed === true) before
matching type === "terminal" and workspaceRun?.workspaceId === workspaceId; this
ensures runPane only returns an active terminal pane for the workspace and not
an orphaned/closed one.
There was a problem hiding this comment.
2 issues found across 9 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/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts:64">
P2: Unawaited promise rejection risk in START path - `getResolvedRunCommands.query` lacks error handling</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/workspaceRun.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/workspaceRun.ts:108">
P1: Empty catch block silently swallows all errors and incorrectly treats query failures as process exits. Should capture error context with logging and differentiate between transport failures vs actual process exits.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| xterm.writeln("[Press any key to restart]"); | ||
| done(); | ||
| return true; | ||
| } catch { |
There was a problem hiding this comment.
P1: Empty catch block silently swallows all errors and incorrectly treats query failures as process exits. Should capture error context with logging and differentiate between transport failures vs actual process exits.
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/hooks/workspaceRun.ts, line 108:
<comment>Empty catch block silently swallows all errors and incorrectly treats query failures as process exits. Should capture error context with logging and differentiate between transport failures vs actual process exits.</comment>
<file context>
@@ -0,0 +1,121 @@
+ xterm.writeln("[Press any key to restart]");
+ done();
+ return true;
+ } catch {
+ if (shouldAbort()) return true;
+
</file context>
|
|
||
| // START: always fetch the latest config so run-script detection never | ||
| // depends on stale cache state or on a query still loading in the view. | ||
| const runConfig = |
There was a problem hiding this comment.
P2: Unawaited promise rejection risk in START path - getResolvedRunCommands.query lacks error handling
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts, line 64:
<comment>Unawaited promise rejection risk in START path - `getResolvedRunCommands.query` lacks error handling</comment>
<file context>
@@ -60,13 +53,19 @@ export function useWorkspaceRunCommand({
- const command = buildTerminalCommand(runConfig?.commands);
+ // START: always fetch the latest config so run-script detection never
+ // depends on stale cache state or on a query still loading in the view.
+ const runConfig =
+ await electronTrpcClient.workspaces.getResolvedRunCommands.query({
+ workspaceId,
</file context>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.ts (1)
30-36:⚠️ Potential issue | 🟡 MinorIgnore closed panes when resolving
runPane.The selector can match a pane that's been closed but not yet fully cleaned up from
s.panes. This can cause the toggle to try reusing an orphaned pane instead of creating a new one after the user closes the run pane.🛠️ Suggested fix to filter by live tabs
const runPane = useTabsStore((s) => { + const liveTabIds = new Set(s.tabs.map((t) => t.id)); const pane = Object.values(s.panes).find( (p) => - p.type === "terminal" && p.workspaceRun?.workspaceId === workspaceId, + p.type === "terminal" && + p.workspaceRun?.workspaceId === workspaceId && + liveTabIds.has(p.tabId), ); return pane ?? null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/hooks/useWorkspaceRunCommand.ts around lines 30 - 36, The selector for runPane may pick up a pane that was closed but still present in s.panes; update the useTabsStore selector that computes runPane to ignore closed/orphaned panes by filtering out panes with a closed flag (or checking against the store's live tabs collection) before finding a terminal with matching workspaceRun.workspaceId; reference the runPane selector (useTabsStore, s.panes, pane.type, pane.workspaceRun, workspaceId) and ensure you only consider panes that are not closed (e.g., pane.closed !== true or present in s.liveTabs) so toggling creates a fresh pane when the previous one was closed.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/workspaceRun.ts (1)
50-63: Consider clarifying theisNewWorkspaceRunnaming.The condition
workspaceRun?.state === "running" && Boolean(defaultRestartCommand)identifies panes that were persisted as "running" but need a fresh start (since the daemon died on app restart). The name "isNewWorkspaceRun" is slightly misleading since it refers to "needs to start a new run" rather than "never ran before."A comment or rename to something like
needsFreshStartcould improve clarity, but the logic itself is correct.🤖 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/hooks/workspaceRun.ts` around lines 50 - 63, Rename or clarify the boolean currently returned as isNewWorkspaceRun in resolveWorkspaceRunAttachMode to reflect that it indicates a persisted run that needs starting again (e.g., needsFreshStart or needsRestart) or add an inline comment above the computation to explain the semantics; update the return property name and any callers that destructure or read isNewWorkspaceRun (references: resolveWorkspaceRunAttachMode, getPaneWorkspaceRun, PaneWorkspaceRun, paneId, defaultRestartCommand) so behavior is unchanged but the purpose is clear.apps/desktop/src/renderer/lib/project-scripts.ts (1)
12-12: Consider scopinggetResolvedRunCommandsinvalidation to affected workspaces.
getResolvedRunCommandsis keyed byworkspaceId(notprojectId), so calling.invalidate()without a filter busts the cache for all workspaces globally. This works correctly but may cause unnecessary re-fetches for unrelated workspaces when editing a single project's scripts.If this becomes a performance concern, you could query the workspaces belonging to
projectIdand invalidate each individually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/project-scripts.ts` at line 12, The call to utils.workspaces.getResolvedRunCommands.invalidate() is clearing the cache for all workspaces because getResolvedRunCommands is keyed by workspaceId; instead, restrict invalidation to only the workspaces affected by the edited project (use the workspace-specific key or pass a filter for workspaceId). Locate the code around utils.workspaces.getResolvedRunCommands.invalidate() in project-scripts.ts, fetch or compute the list of workspace ids for the given projectId (e.g., query the project->workspaces relation), and call invalidate for each workspaceId (or call invalidate with the workspaceId filter) so only those workspace caches are busted.
🤖 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/renderer/routes/_authenticated/_dashboard/workspace/`$workspaceId/hooks/useWorkspaceRunCommand.ts:
- Around line 30-36: The selector for runPane may pick up a pane that was closed
but still present in s.panes; update the useTabsStore selector that computes
runPane to ignore closed/orphaned panes by filtering out panes with a closed
flag (or checking against the store's live tabs collection) before finding a
terminal with matching workspaceRun.workspaceId; reference the runPane selector
(useTabsStore, s.panes, pane.type, pane.workspaceRun, workspaceId) and ensure
you only consider panes that are not closed (e.g., pane.closed !== true or
present in s.liveTabs) so toggling creates a fresh pane when the previous one
was closed.
---
Nitpick comments:
In `@apps/desktop/src/renderer/lib/project-scripts.ts`:
- Line 12: The call to utils.workspaces.getResolvedRunCommands.invalidate() is
clearing the cache for all workspaces because getResolvedRunCommands is keyed by
workspaceId; instead, restrict invalidation to only the workspaces affected by
the edited project (use the workspace-specific key or pass a filter for
workspaceId). Locate the code around
utils.workspaces.getResolvedRunCommands.invalidate() in project-scripts.ts,
fetch or compute the list of workspace ids for the given projectId (e.g., query
the project->workspaces relation), and call invalidate for each workspaceId (or
call invalidate with the workspaceId filter) so only those workspace caches are
busted.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/workspaceRun.ts`:
- Around line 50-63: Rename or clarify the boolean currently returned as
isNewWorkspaceRun in resolveWorkspaceRunAttachMode to reflect that it indicates
a persisted run that needs starting again (e.g., needsFreshStart or
needsRestart) or add an inline comment above the computation to explain the
semantics; update the return property name and any callers that destructure or
read isNewWorkspaceRun (references: resolveWorkspaceRunAttachMode,
getPaneWorkspaceRun, PaneWorkspaceRun, paneId, defaultRestartCommand) so
behavior is unchanged but the purpose is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3317f16-c1e3-43ef-8db2-2d70926f983b
📒 Files selected for processing (9)
apps/desktop/src/renderer/lib/project-scripts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/WorkspaceRunButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/project/$projectId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/hooks/useWorkspaceRunCommand.tsapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/components/ScriptsEditor/ScriptsEditor.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/useTerminalLifecycle.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/hooks/workspaceRun.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/index.ts
There was a problem hiding this comment.
1 issue found across 3 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/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/WorkspaceRunButton.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/WorkspaceRunButton.tsx:38">
P2: Run-command absence is inferred from unresolved query data, causing premature redirect to settings instead of run/stop behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ); | ||
|
|
||
| const handleClick = useCallback(() => { | ||
| if (!hasRunCommand && projectId) { |
There was a problem hiding this comment.
P2: Run-command absence is inferred from unresolved query data, causing premature redirect to settings instead of run/stop behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceRunButton/WorkspaceRunButton.tsx, line 38:
<comment>Run-command absence is inferred from unresolved query data, causing premature redirect to settings instead of run/stop behavior.</comment>
<file context>
@@ -1,29 +1,73 @@
+ );
const handleClick = useCallback(() => {
+ if (!hasRunCommand && projectId) {
+ setSettingsSearchQuery("scripts");
+ void navigate({
</file context>
Summary
bun run dev) configurable in Project Settings > Scripts alongside setup/teardownCmd+Shift+G) (configurable in settings) - toggles start/stop, reuses the same pane across restartsshell -lc "command"so the PTY exits when the command finishes (enables crash detection)Demo Video (walkthrough explanation)
https://screen.studio/share/fbKjnjzV
Architecture
Single source of truth:
pane.workspaceRunmetadata on the Pane object. Run state is derived from pane metadata everywhere (sidebar selector, toolbar indicator, terminal lifecycle).UI Changes
Key files
useWorkspaceRunCommand.ts— orchestration hook (toggle, pane reuse, start/stop)useTerminalLifecycle.ts— workspace-run recovery on mount, restart with command, cleanupuseTerminalStream.ts— exit handler updates pane stateWorkspaceRunIndicator/— shared visual indicator (sidebar + pane toolbar)setup.ts—runfield support in config loading/mergingsession.ts—getCommandShellArgsfor non-interactive command executionui-state/index.ts—workspaceRunadded to pane persistence schemaFollow-ups
Test plan
bun run typecheck)bun run lint)-lcvs--rcfile), run config loading/validation/merging (8 tests)Summary by CodeRabbit
New Features
Tests
Style