fix(desktop): v2 preset commands run reliably in new tabs#4107
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR centralizes terminal session creation behind a new TerminalLauncher abstraction and hook, replaces ad-hoc crypto UUID generation with launcher.create() across workspace wiring (hotkeys, presets, pane openers, context menus, pane actions), removes TerminalPaneData.initialCommand, and refactors TerminalPane to mount-and-connect using an existing session URL instead of creating sessions locally. ChangesTerminal Launcher Integration (single cohort)
🎯 4 (Complex) | ⏱️ ~50 minutes sequenceDiagram
participant User as "User/UI"
participant Page as "Page / Hooks"
participant Launcher as "TerminalLauncher (TRPC)"
participant Pane as "TerminalPane (Runtime)"
participant WS as "Workspace WebSocket"
rect rgba(100,150,240,0.5)
User->>Page: trigger new tab / split / preset
end
rect rgba(120,200,160,0.5)
Page->>Launcher: call create({ command? }) -> Promise<terminalId>
Launcher->>Launcher: trpcClient.terminal.createSession.mutate(...)
Launcher-->>Page: terminalId
end
rect rgba(240,180,120,0.5)
Page->>Pane: create/add pane with terminalId
Pane->>Pane: mount runtime
Pane->>WS: connect to websocketUrl for terminalId
WS-->>Pane: stream terminal IO
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR fixes the
Confidence Score: 3/5The core fix works correctly, but the launcher hook returns a new object literal on every render, causing the rehydration effect to re-fire The design of apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts — the missing
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts | New central launcher hook — returns an unstable object literal each render, causing useEnsurePersistedTerminals to fire repeated tRPC mutations; also omits terminal dimensions that the old mount path forwarded to createSession. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useEnsurePersistedTerminals/useEnsurePersistedTerminals.ts | New rehydration hook — logic is correct but the useEffect dep on launcher (unstable object reference) causes it to re-run on every render instead of once per workspace mount. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx | Correctly removes the async createSession call from mount, delegating PTY creation to the launcher; loses user-visible error feedback path as a side effect. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts | Cleanly migrated all preset modes to use launcher.create({ command }) before writing panes to the store; logic is correct and simpler than before. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsx | Wires launcher into all consumer hooks correctly; downstream instability flows from the unstable launcher object returned by useV2TerminalLauncher. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsx | Correctly replaced crypto.randomUUID() calls with launcher.create() for split actions; signature change to accept launcher is clean. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts | All four terminal-creation hotkey paths correctly updated to use launcher.create() instead of bare crypto.randomUUID(). |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts | Removes initialCommand from TerminalPaneData — correct, since commands are now passed at createSession time via the launcher, not stored in pane data. |
Sequence Diagram
sequenceDiagram
participant Caller as Preset / Hotkey / Split
participant Launcher as useV2TerminalLauncher
participant HostSvc as Host Service (tRPC)
participant Store as WorkspaceStore
participant Pane as TerminalPane (mount)
participant WS as WebSocket
Caller->>Launcher: launcher.create({ command? })
Launcher->>HostSvc: createSession(terminalId, command)
Note over HostSvc: Spawns PTY, buffers output
Launcher-->>Caller: terminalId (sync)
Caller->>Store: addTab / addPane (pane with terminalId)
Note over Store: Active tab may flip here (new-tab-per-command)
Note over Pane: Mounts whenever tab becomes active
Pane->>Pane: mount() - attach DOM container
Pane->>WS: connect(terminalId)
WS-->>HostSvc: WebSocket attaches
HostSvc-->>WS: Flush buffered PTY output
Note over Caller,Store: Rehydration path (workspace open)
Caller->>Launcher: launcher.ensure(terminalId)
Launcher->>HostSvc: createSession (idempotent - adopt or spawn)
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx, line 130-147 (link)createSessionfailures are now silent to the userThe removed async block wrote the error into the terminal display via
terminalRuntimeRegistry.getTerminal(...).writeln(...), giving users a visible message when the host-service was unreachable or the session couldn't start. With this code gone, acreateSessionfailure is logged to the console only (inuseV2TerminalLauncher), and the pane mounts, callsconnect(), and then sits with an unresponsive blank terminal. There's no way for the user to distinguish "still connecting" from "failed."Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx Line: 130-147 Comment: **`createSession` failures are now silent to the user** The removed async block wrote the error into the terminal display via `terminalRuntimeRegistry.getTerminal(...).writeln(...)`, giving users a visible message when the host-service was unreachable or the session couldn't start. With this code gone, a `createSession` failure is logged to the console only (in `useV2TerminalLauncher`), and the pane mounts, calls `connect()`, and then sits with an unresponsive blank terminal. There's no way for the user to distinguish "still connecting" from "failed." How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:1-2
`useV2TerminalLauncher` returns a plain object literal (`{ create, ensure }`) on every render. Even though `create` and `ensure` are individually stabilised by `useCallback`, the containing object is a new reference on every call, so `launcher` itself is always a fresh value. `useEnsurePersistedTerminals` has `[store, launcher]` in its effect deps — because `launcher` is never the same reference, the effect fires on every render of `V2WorkspacePage` and calls `launcher.ensure()` (a live `createSession` tRPC mutation) for every terminal in the persisted layout each time. Wrapping the return value in `useMemo` makes the object stable so the effect runs only when `create`/`ensure` actually change.
```suggestion
import { useWorkspaceClient } from "@superset/workspace-client";
import { useCallback, useMemo } from "react";
```
### Issue 2 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:78-79
The returned object also needs to be wrapped in `useMemo` so its reference is stable across re-renders. Without this, every downstream `useEffect` / `useCallback` / `useMemo` that lists `launcher` in its dependency array will treat each render as a change and re-execute — most importantly, `useEnsurePersistedTerminals`'s effect will fire `createSession` mutations for all persisted terminals on every render cycle (e.g. every tab switch).
```suggestion
return useMemo(() => ({ create, ensure }), [create, ensure]);
}
```
### Issue 3 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:39-58
**Terminal created without measured dimensions**
The old `TerminalPane` mount path called `terminalRuntimeRegistry.getDimensions()` and forwarded `cols`/`rows` to `createSession`. `launcher.create()` omits those fields entirely, so the host-service spawns the PTY at whatever default size it uses. The terminal will resize to fit its container on the first `resize` event, but until then the shell's line-wrap width is wrong — this can corrupt output for commands that query `$COLUMNS` or use readline immediately on startup (e.g. an `initialCommand` that runs a curses UI).
### Issue 4 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:130-147
**`createSession` failures are now silent to the user**
The removed async block wrote the error into the terminal display via `terminalRuntimeRegistry.getTerminal(...).writeln(...)`, giving users a visible message when the host-service was unreachable or the session couldn't start. With this code gone, a `createSession` failure is logged to the console only (in `useV2TerminalLauncher`), and the pane mounts, calls `connect()`, and then sits with an unresponsive blank terminal. There's no way for the user to distinguish "still connecting" from "failed."
Reviews (1): Last reviewed commit: "fix(desktop): v2 preset commands run rel..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1
to
+2
| import { useWorkspaceClient } from "@superset/workspace-client"; | ||
| import { useCallback } from "react"; |
Contributor
There was a problem hiding this comment.
useV2TerminalLauncher returns a plain object literal ({ create, ensure }) on every render. Even though create and ensure are individually stabilised by useCallback, the containing object is a new reference on every call, so launcher itself is always a fresh value. useEnsurePersistedTerminals has [store, launcher] in its effect deps — because launcher is never the same reference, the effect fires on every render of V2WorkspacePage and calls launcher.ensure() (a live createSession tRPC mutation) for every terminal in the persisted layout each time. Wrapping the return value in useMemo makes the object stable so the effect runs only when create/ensure actually change.
Suggested change
| import { useWorkspaceClient } from "@superset/workspace-client"; | |
| import { useCallback } from "react"; | |
| import { useWorkspaceClient } from "@superset/workspace-client"; | |
| import { useCallback, useMemo } from "react"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts
Line: 1-2
Comment:
`useV2TerminalLauncher` returns a plain object literal (`{ create, ensure }`) on every render. Even though `create` and `ensure` are individually stabilised by `useCallback`, the containing object is a new reference on every call, so `launcher` itself is always a fresh value. `useEnsurePersistedTerminals` has `[store, launcher]` in its effect deps — because `launcher` is never the same reference, the effect fires on every render of `V2WorkspacePage` and calls `launcher.ensure()` (a live `createSession` tRPC mutation) for every terminal in the persisted layout each time. Wrapping the return value in `useMemo` makes the object stable so the effect runs only when `create`/`ensure` actually change.
```suggestion
import { useWorkspaceClient } from "@superset/workspace-client";
import { useCallback, useMemo } from "react";
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+78
to
+79
| return { create, ensure }; | ||
| } |
Contributor
There was a problem hiding this comment.
useMemo so its reference is stable across re-renders. Without this, every downstream useEffect / useCallback / useMemo that lists launcher in its dependency array will treat each render as a change and re-execute — most importantly, useEnsurePersistedTerminals's effect will fire createSession mutations for all persisted terminals on every render cycle (e.g. every tab switch).
Suggested change
| return { create, ensure }; | |
| } | |
| return useMemo(() => ({ create, ensure }), [create, ensure]); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts
Line: 78-79
Comment:
The returned object also needs to be wrapped in `useMemo` so its reference is stable across re-renders. Without this, every downstream `useEffect` / `useCallback` / `useMemo` that lists `launcher` in its dependency array will treat each render as a change and re-execute — most importantly, `useEnsurePersistedTerminals`'s effect will fire `createSession` mutations for all persisted terminals on every render cycle (e.g. every tab switch).
```suggestion
return useMemo(() => ({ create, ensure }), [create, ensure]);
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+39
to
+58
|
|
||
| const create = useCallback( | ||
| (options?: CreateOptions): string => { | ||
| const terminalId = crypto.randomUUID(); | ||
| const mutation = trpcClient.terminal.createSession.mutate({ | ||
| terminalId, | ||
| workspaceId, | ||
| themeType, | ||
| initialCommand: options?.command, | ||
| }); | ||
| terminalSessionPending.track(terminalId, mutation); | ||
| mutation.catch((error) => { | ||
| console.error( | ||
| "[useV2TerminalLauncher] Failed to create terminal session:", | ||
| { terminalId, error }, | ||
| ); | ||
| }); | ||
| return terminalId; | ||
| }, | ||
| [trpcClient, workspaceId, themeType], |
Contributor
There was a problem hiding this comment.
The old TerminalPane mount path called terminalRuntimeRegistry.getDimensions() and forwarded cols/rows to createSession. launcher.create() omits those fields entirely, so the host-service spawns the PTY at whatever default size it uses. The terminal will resize to fit its container on the first resize event, but until then the shell's line-wrap width is wrong — this can corrupt output for commands that query $COLUMNS or use readline immediately on startup (e.g. an initialCommand that runs a curses UI).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts
Line: 39-58
Comment:
**Terminal created without measured dimensions**
The old `TerminalPane` mount path called `terminalRuntimeRegistry.getDimensions()` and forwarded `cols`/`rows` to `createSession`. `launcher.create()` omits those fields entirely, so the host-service spawns the PTY at whatever default size it uses. The terminal will resize to fit its container on the first `resize` event, but until then the shell's line-wrap width is wrong — this can corrupt output for commands that query `$COLUMNS` or use readline immediately on startup (e.g. an `initialCommand` that runs a curses UI).
How can I resolve this? If you propose a fix, please make it concise.![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: 2
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts (2)
21-21: 💤 Low valueDrop the
as TerminalPaneDatacast if the type now only requiresterminalId.Per the PR summary,
initialCommandwas removed fromTerminalPaneData. If the type is now structurally just{ terminalId: string }(with optional fields), the cast is redundant and hides any future shape mismatch. Prefer letting inference do the work so a future required field onTerminalPaneDatasurfaces here as a compile error.♻️ Suggested change
- data: { terminalId } as TerminalPaneData, + data: { terminalId } satisfies TerminalPaneData,(or simply
data: { terminalId }ifCreatePaneInput<PaneViewerData>already discriminates the kind).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts at line 21, Remove the unnecessary type assertion on the pane data: in useV2PresetExecution (the line currently using "data: { terminalId } as TerminalPaneData") drop the "as TerminalPaneData" so the object is passed as "data: { terminalId }" and let TypeScript infer the shape; this surfaces any future required fields on TerminalPaneData or mismatches from CreatePaneInput<PaneViewerData> instead of hiding them behind an explicit cast.
77-132: ⚡ Quick winDe-duplicate the multi-pane builder between
new-tab-multi-paneandactive-tab-multi-pane.Both branches build
panesfrompreset.commandswith the same fallback semantics and the same non-empty tuple cast. Extracting a small local helper keeps the two cases in lockstep and isolates the cast to one place.♻️ Suggested refactor
+ const buildMultiPanes = (): [ + CreatePaneInput<PaneViewerData>, + ...CreatePaneInput<PaneViewerData>[], + ] => { + const panes = + preset.commands.length > 0 + ? preset.commands.map((command) => + makeTerminalPane(launcher.create({ command }), title), + ) + : [makeTerminalPane(launcher.create(), title)]; + return panes as [ + CreatePaneInput<PaneViewerData>, + ...CreatePaneInput<PaneViewerData>[], + ]; + }; + try { switch (plan) { ... case "new-tab-multi-pane": { - const panes = - preset.commands.length > 0 - ? preset.commands.map((command) => - makeTerminalPane(launcher.create({ command }), title), - ) - : [makeTerminalPane(launcher.create(), title)]; - state.addTab({ - panes: panes as [ - CreatePaneInput<PaneViewerData>, - ...CreatePaneInput<PaneViewerData>[], - ], - }); + state.addTab({ panes: buildMultiPanes() }); break; } ... case "active-tab-multi-pane": { - const panes = - preset.commands.length > 0 - ? preset.commands.map((command) => - makeTerminalPane(launcher.create({ command }), title), - ) - : [makeTerminalPane(launcher.create(), title)]; + const panes = buildMultiPanes(); if (!activeTabId) { - state.addTab({ - panes: panes as [ - CreatePaneInput<PaneViewerData>, - ...CreatePaneInput<PaneViewerData>[], - ], - }); + state.addTab({ panes }); break; } for (const pane of panes) { state.addPane({ tabId: activeTabId, pane }); } break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts around lines 77 - 132, The two switch cases "new-tab-multi-pane" and "active-tab-multi-pane" duplicate the logic that builds the panes array from preset.commands (including the fallback to a single launcher.create() and the non-empty tuple cast); extract that logic into a small helper (e.g., buildPanesForPreset or createPanesFromPreset) that returns the correctly typed panes array (doing the CreatePaneInput<PaneViewerData> non-empty tuple cast in one place) and replace the duplicated code in both cases to call this helper before calling state.addTab or iterating to call state.addPane; keep references to makeTerminalPane and launcher.create inside the helper so the cast and creation logic are centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:
- Line 78: The returned launcher object from useV2TerminalLauncher is a new
object each render causing downstream hooks (that depend on launcher) to re-run;
fix this by memoizing the launcher reference—wrap the { create, ensure } return
in useMemo (or store in a ref) inside useV2TerminalLauncher so that the object
identity remains stable while still exposing the useCallback-stable create and
ensure functions; update the return to return the memoized launcher (reference
the create and ensure symbols and the useV2TerminalLauncher hook) so effects
depending on launcher no longer retrigger every render.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx:
- Around line 90-95: The returned launcher object from useV2TerminalLauncher is
recreated each render which invalidates downstream dependency arrays (causing
hooks like useV2PresetExecution and useWorkspaceHotkeys to recompute); update
useV2TerminalLauncher to wrap the returned object in useMemo so it returns a
stable reference (e.g., memoize the object containing the create and ensure
callbacks with useMemo and depend on [create, ensure]) so calls to
useEnsurePersistedTerminals, useV2PresetExecution, and other consumers stop
rebinding on every render.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts:
- Line 21: Remove the unnecessary type assertion on the pane data: in
useV2PresetExecution (the line currently using "data: { terminalId } as
TerminalPaneData") drop the "as TerminalPaneData" so the object is passed as
"data: { terminalId }" and let TypeScript infer the shape; this surfaces any
future required fields on TerminalPaneData or mismatches from
CreatePaneInput<PaneViewerData> instead of hiding them behind an explicit cast.
- Around line 77-132: The two switch cases "new-tab-multi-pane" and
"active-tab-multi-pane" duplicate the logic that builds the panes array from
preset.commands (including the fallback to a single launcher.create() and the
non-empty tuple cast); extract that logic into a small helper (e.g.,
buildPanesForPreset or createPanesFromPreset) that returns the correctly typed
panes array (doing the CreatePaneInput<PaneViewerData> non-empty tuple cast in
one place) and replace the duplicated code in both cases to call this helper
before calling state.addTab or iterating to call state.addPane; keep references
to makeTerminalPane and launcher.create inside the helper so the cast and
creation logic are centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f91e28df-2a95-453c-8e56-03ebc66fd78b
📒 Files selected for processing (12)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultPaneActions/useDefaultPaneActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useEnsurePersistedTerminals/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useEnsurePersistedTerminals/useEnsurePersistedTerminals.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
Comment on lines
+90
to
+95
| const launcher = useV2TerminalLauncher(); | ||
| useEnsurePersistedTerminals({ store, launcher }); | ||
| const { matchedPresets, executePreset } = useV2PresetExecution({ | ||
| store, | ||
| launcher, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the launcher hook to confirm the returned object/methods are memoized.
fd -t f 'useV2TerminalLauncher.ts' --exec cat {}Repository: superset-sh/superset
Length of output: 2648
Wrap the returned launcher object in useMemo to avoid invalidating downstream dependency arrays.
The create and ensure methods are memoized with useCallback, but useV2TerminalLauncher returns a new object { create, ensure } on every render. Each fresh reference will cause useV2PresetExecution and other hooks to recompute unnecessarily, rippling through useWorkspaceHotkeys and triggering hotkey rebinding.
Memoize the return value:
const memoizedLauncher = useMemo(() => ({ create, ensure }), [create, ensure]);
return memoizedLauncher;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/page.tsx
around lines 90 - 95, The returned launcher object from useV2TerminalLauncher is
recreated each render which invalidates downstream dependency arrays (causing
hooks like useV2PresetExecution and useWorkspaceHotkeys to recompute); update
useV2TerminalLauncher to wrap the returned object in useMemo so it returns a
stable reference (e.g., memoize the object containing the create and ensure
callbacks with useMemo and depend on [create, ensure]) so calls to
useEnsurePersistedTerminals, useV2PresetExecution, and other consumers stop
rebinding on every render.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts (1)
79-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMemoize the returned
launcherobject.
createandensureare callback-stable, but{ create, ensure }is a new object every render. AnyuseEffect/useMemodepending onlauncherwill retrigger every render, which is especially problematic for the persisted-terminal ensure flow added in this PR.Suggested fix
-import { useCallback } from "react"; +import { useCallback, useMemo } from "react"; ... - return { create, ensure }; + return useMemo(() => ({ create, ensure }), [create, ensure]);You can confirm the downstream dependency churn with:
#!/bin/bash set -euo pipefail echo "=== useV2TerminalLauncher ===" fd 'useV2TerminalLauncher\.ts$' apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace \ | while read -r f; do echo "--- $f ---" sed -n '1,140p' "$f" done echo echo "=== launcher-dependent hooks/components ===" rg -n -C2 '\blauncher\b' apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace --iglob '*.ts' --iglob '*.tsx'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts at line 79, The hook useV2TerminalLauncher returns a fresh object { create, ensure } each render which breaks referential equality for consumers; wrap the returned launcher in React.useMemo (e.g. create a const launcher = useMemo(() => ({ create, ensure }), [create, ensure]) and return launcher) so the object is stable across renders while keeping create and ensure callback-stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/terminal-session-pending.ts`:
- Around line 39-40: awaitReady currently does a one-shot lookup on the pending
Map and returns immediately if no entry exists, which races with effect-driven
launcher.ensure calls; change awaitReady (in terminal-session-pending) to create
and register a deferred promise when no pending entry exists (store a resolve in
the pending Map) or implement a short retry/poll loop so callers (e.g.,
TerminalPane's useEffect) will wait until launcher.ensure (from
useEnsurePersistedTerminals) registers the real pending promise; reference
awaitReady, pending, launcher.ensure, useEnsurePersistedTerminals and
TerminalPane when making the change so the deferred promise is created/returned
by awaitReady before the rehydration effect runs.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:
- Line 79: The hook useV2TerminalLauncher returns a fresh object { create,
ensure } each render which breaks referential equality for consumers; wrap the
returned launcher in React.useMemo (e.g. create a const launcher = useMemo(() =>
({ create, ensure }), [create, ensure]) and return launcher) so the object is
stable across renders while keeping create and ensure callback-stable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a1afedd-785a-4760-afe7-8db525da1348
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/terminal-session-pending.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts
Presets that opened multiple new tabs (`new-tab-per-command` mode) only
ran their command in the LAST tab — earlier tabs sat empty until the
user clicked them. The v2 workspace renders only the active tab, so
each `addTab` in the loop flipped `activeTabId` and unmounted the prior
tab before its `TerminalPane` could call `terminal.createSession`.
Background tabs never spawned a PTY.
Move session creation off the pane and onto the call site:
- `useV2TerminalLauncher` is the single seam in v2 renderer code that
calls `terminal.createSession.mutate(...)`. One method, async:
`create({ command?, terminalId? }) => Promise<string>` mints a fresh
id when terminalId is omitted, adopts the given id otherwise (relying
on host-service createSession idempotency).
- Every v2 site that creates a terminal pane now awaits
`launcher.create(...)` before writing the pane into the store: preset
hook, hotkeys (NEW_GROUP / SPLIT_*), pane action button, context menu
splits, `useWorkspacePaneOpeners.addTerminalTab`. Multi-pane / per-
command presets fan out via `Promise.all` so they don't pay serial
latency.
- `TerminalPane.mount` is back to plain `mount() + connect()`. It never
calls createSession; if a terminalId arrives without a live session,
it's a launching-call-site bug, not the pane's problem.
- `TerminalPaneData.initialCommand` is gone — pane data is just the
terminalId now.
Kitenite
force-pushed
the
debug-presets-new-tabs
branch
from
c298364 to
b267fe6
Compare
Contributor
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/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx (1)
413-416:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the disconnected banner text selectable.
This error/status text is rendered without
select-text cursor-text, so users still cannot copy it in the renderer.💡 Proposed fix
{connectionState === "closed" && ( - <div className="flex items-center gap-2 border-t border-border px-3 py-1.5 text-xs text-muted-foreground"> + <div className="flex items-center gap-2 border-t border-border px-3 py-1.5 text-xs text-muted-foreground select-text cursor-text"> <span>Disconnected</span> </div> )}As per coding guidelines, Error text must be selectable by users with explicit
select-text cursor-textclasses; renderer setsuser-select: noneonbody.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx around lines 413 - 416, The disconnected banner rendered in TerminalPane (check the connectionState === "closed" block inside the TerminalPane component) is not selectable due to global renderer styles; update the banner div to include the explicit classes "select-text" and "cursor-text" so the "Disconnected" status can be copied—i.e., add those classes to the existing className on the div that renders the disconnected message.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts (1)
52-52: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMemoize the returned launcher object so consumers' dependency arrays stay stable.
{ create }is a fresh object literal on every render even thoughcreateitself isuseCallback-stabilized. Every consumer that listslauncherin a dependency array (useV2PresetExecution[store, launcher, resolvePresetCommands],useDefaultPaneActions[launcher],useDefaultContextMenuActions[…, launcher],useWorkspacePaneOpeners[store, launcher],useWorkspaceHotkeysvia threaded deps) will re-create their callbacks/memoized values on every parent render, defeating the memoization in this file.♻️ Proposed fix
-import { useCallback } from "react"; +import { useCallback, useMemo } from "react"; @@ - return { create }; + return useMemo(() => ({ create }), [create]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts at line 52, The returned launcher object literal in useV2TerminalLauncher is recreated each render which breaks consumers' dependency stability; instead memoize the launcher object (containing the create callback) so its reference is stable — e.g. use useMemo or useRef inside useV2TerminalLauncher to return a stable object like launcher = useMemo(() => ({ create }), [create]) and then return that launcher from the hook (keep the create useCallback as-is).
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts (1)
47-57: 💤 Low valueConsider surfacing
launcher.create()failures to the user, the wayuseV2PresetExecutiondoes.In
NEW_GROUP,SPLIT_AUTO,SPLIT_RIGHT, andSPLIT_DOWNtheawait launcher.create()call is unguarded. If the host-service mutation rejects (e.g., daemon down, schema validation failure on the protected procedure — seepackages/host-service/src/trpc/router/terminal/terminal.ts:92-95), the rejection becomes an unhandled promise rejection and the user sees nothing — no tab, no error toast.useV2PresetExecutionalready wraps the same call intry/catchwith atoast.error; mirroring that pattern here keeps hotkey-driven creation paths consistent with preset-driven ones.♻️ Example shape (apply to each `SPLIT_*` and `NEW_GROUP` callback)
- useHotkey("NEW_GROUP", async () => { - const terminalId = await launcher.create(); - store.getState().addTab({ - panes: [ - { - kind: "terminal", - data: { terminalId } as TerminalPaneData, - }, - ], - }); - }); + useHotkey("NEW_GROUP", async () => { + try { + const terminalId = await launcher.create(); + store.getState().addTab({ + panes: [ + { kind: "terminal", data: { terminalId } as TerminalPaneData }, + ], + }); + } catch (err) { + console.error("[useWorkspaceHotkeys] NEW_GROUP failed:", err); + toast.error("Failed to open new terminal", { + description: err instanceof Error ? err.message : "Terminal session creation failed.", + }); + } + });Also applies to: 204-255
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts around lines 47 - 57, The NEW_GROUP, SPLIT_AUTO, SPLIT_RIGHT and SPLIT_DOWN hotkey handlers call launcher.create() without handling rejections; wrap each async callback (the function passed to useHotkey for those keys) in a try/catch, call const terminalId = await launcher.create() inside the try, then on success call store.getState().addTab(...) as existing, and on failure call toast.error(...) (mirror the pattern used in useV2PresetExecution) so launcher.create() failures surface to the user instead of causing unhandled promise rejections.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts (1)
85-183: LGTM — preset execution now mints sessions before tabs/panes are written, fixing the new-tab-per-command race.Each plan branch awaits
launcher.create({ command })(parallelized withPromise.allwhere multiple sessions are needed) before callingstate.addTab/state.addPane, so background tabs that flipactiveTabIdand unmount before mounting still get their PTY queued — exactly the behavior the PR aims to fix. The wrappingtry/catchwith toast covers user-visible failures.One thing worth keeping in mind (not a blocker for this PR): if
Promise.allrejects partway, sessions that already created on host-service become orphaned until the next workspace open. IfuseEnsurePersistedTerminals/launcher.ensurelands as the PR description suggests, that path covers re-adoption; otherwise a follow-up to clean up dangling sessions on partial failure may be worthwhile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts around lines 85 - 183, The code can leave orphaned terminal sessions if Promise.all rejects partway; update executePreset to track any terminal IDs created before a failure in the parallel branches ("new-tab-multi-pane", "new-tab-per-command", "active-tab-multi-pane") and, in the catch, iterate those IDs to clean them up (e.g. call the launcher cleanup API for each created id) or switch to using the planned resilience helper (useEnsurePersistedTerminals / launcher.ensure) once available; ensure tracking/cleanup logic is added around the launcher.create calls so partial successes are removed on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:
- Around line 19-27: The PR description advertises missing APIs
(launcher.ensure, useEnsurePersistedTerminals, terminalSessionPending) but the
implementation only defines TerminalLauncher.create; either update the PR text
or implement the missing features—preferably implement: add an
ensure(terminalId: string): Promise<void> method to the TerminalLauncher
interface and its concrete launcher implementation to idempotently
confirm/rehydrate a session, add a useEnsurePersistedTerminals hook that reads
persisted terminal IDs and calls launcher.ensure for each on mount, and
introduce a shared terminalSessionPending map (or similar store) used by
create() and ensure() to track in-flight session creation so callers can await
the same Promise; update any types and tests referencing TerminalLauncher to
include ensure and wire the hook to the launcher.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsx:
- Around line 413-416: The disconnected banner rendered in TerminalPane (check
the connectionState === "closed" block inside the TerminalPane component) is not
selectable due to global renderer styles; update the banner div to include the
explicit classes "select-text" and "cursor-text" so the "Disconnected" status
can be copied—i.e., add those classes to the existing className on the div that
renders the disconnected message.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts:
- Line 52: The returned launcher object literal in useV2TerminalLauncher is
recreated each render which breaks consumers' dependency stability; instead
memoize the launcher object (containing the create callback) so its reference is
stable — e.g. use useMemo or useRef inside useV2TerminalLauncher to return a
stable object like launcher = useMemo(() => ({ create }), [create]) and then
return that launcher from the hook (keep the create useCallback as-is).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.ts:
- Around line 85-183: The code can leave orphaned terminal sessions if
Promise.all rejects partway; update executePreset to track any terminal IDs
created before a failure in the parallel branches ("new-tab-multi-pane",
"new-tab-per-command", "active-tab-multi-pane") and, in the catch, iterate those
IDs to clean them up (e.g. call the launcher cleanup API for each created id) or
switch to using the planned resilience helper (useEnsurePersistedTerminals /
launcher.ensure) once available; ensure tracking/cleanup logic is added around
the launcher.create calls so partial successes are removed on error.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.ts:
- Around line 47-57: The NEW_GROUP, SPLIT_AUTO, SPLIT_RIGHT and SPLIT_DOWN
hotkey handlers call launcher.create() without handling rejections; wrap each
async callback (the function passed to useHotkey for those keys) in a try/catch,
call const terminalId = await launcher.create() inside the try, then on success
call store.getState().addTab(...) as existing, and on failure call
toast.error(...) (mirror the pattern used in useV2PresetExecution) so
launcher.create() failures surface to the user instead of causing unhandled
promise rejections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fc02001-25b5-4aaf-80d1-eade92b522cd
📒 Files selected for processing (11)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/V2PresetsBar/V2PresetsBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultContextMenuActions/useDefaultContextMenuActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useDefaultPaneActions/useDefaultPaneActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/TerminalPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2PresetExecution/useV2PresetExecution.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspaceHotkeys/useWorkspaceHotkeys.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useV2TerminalLauncher/index.ts
Comment on lines
+19
to
+27
| export interface TerminalLauncher { | ||
| /** | ||
| * Awaits `terminal.createSession` and returns the terminalId. Callers | ||
| * should await this before writing the pane into the store, so the pane's | ||
| * WebSocket connect doesn't race ahead of the session existing on | ||
| * host-service. | ||
| */ | ||
| create(options?: CreateOptions): Promise<string>; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether ensure / useEnsurePersistedTerminals / terminalSessionPending exist anywhere in the repo.
rg -nP --type=ts -C2 '\b(useEnsurePersistedTerminals|terminalSessionPending)\b'
rg -nP --type=ts -C2 '\blauncher\.ensure\s*\('
rg -nP --type=ts -C2 'ensure\s*\(\s*[a-zA-Z_]*terminalId'Repository: superset-sh/superset
Length of output: 46
PR description claims features that do not exist in the implementation.
The PR objectives reference a launcher.ensure(terminalId) method, a useEnsurePersistedTerminals rehydration hook, and a terminalSessionPending shared map. None of these appear anywhere in the codebase. The current TerminalLauncher interface only exposes create().
Before merge, either update the PR description to reflect what actually ships, or complete the implementation to match the stated objectives. This ensures reviewers and the test plan align with the actual code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/useV2TerminalLauncher/useV2TerminalLauncher.ts
around lines 19 - 27, The PR description advertises missing APIs
(launcher.ensure, useEnsurePersistedTerminals, terminalSessionPending) but the
implementation only defines TerminalLauncher.create; either update the PR text
or implement the missing features—preferably implement: add an
ensure(terminalId: string): Promise<void> method to the TerminalLauncher
interface and its concrete launcher implementation to idempotently
confirm/rehydrate a session, add a useEnsurePersistedTerminals hook that reads
persisted terminal IDs and calls launcher.ensure for each on mount, and
introduce a shared terminalSessionPending map (or similar store) used by
create() and ensure() to track in-flight session creation so callers can await
the same Promise; update any types and tests referencing TerminalLauncher to
include ensure and wire the hook to the launcher.
…re stable
The hook returned a fresh `{ create }` object every render, invalidating
the [launcher] dep array in every consumer (preset hook, hotkeys, pane
actions, context menu, openers) on each parent render and pointlessly
recomputing their useCallback / useMemo bodies. Wrap in useMemo so
launcher's reference is stable across renders.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
pushed a commit
that referenced
this pull request
May 6, 2026
* fix(desktop): v2 preset commands run reliably in new tabs
Presets that opened multiple new tabs (`new-tab-per-command` mode) only
ran their command in the LAST tab — earlier tabs sat empty until the
user clicked them. The v2 workspace renders only the active tab, so
each `addTab` in the loop flipped `activeTabId` and unmounted the prior
tab before its `TerminalPane` could call `terminal.createSession`.
Background tabs never spawned a PTY.
Move session creation off the pane and onto the call site:
- `useV2TerminalLauncher` is the single seam in v2 renderer code that
calls `terminal.createSession.mutate(...)`. One method, async:
`create({ command?, terminalId? }) => Promise<string>` mints a fresh
id when terminalId is omitted, adopts the given id otherwise (relying
on host-service createSession idempotency).
- Every v2 site that creates a terminal pane now awaits
`launcher.create(...)` before writing the pane into the store: preset
hook, hotkeys (NEW_GROUP / SPLIT_*), pane action button, context menu
splits, `useWorkspacePaneOpeners.addTerminalTab`. Multi-pane / per-
command presets fan out via `Promise.all` so they don't pay serial
latency.
- `TerminalPane.mount` is back to plain `mount() + connect()`. It never
calls createSession; if a terminalId arrives without a live session,
it's a launching-call-site bug, not the pane's problem.
- `TerminalPaneData.initialCommand` is gone — pane data is just the
terminalId now.
* fix(desktop): memoize useV2TerminalLauncher return so consumer deps are stable
The hook returned a fresh `{ create }` object every render, invalidating
the [launcher] dep array in every consumer (preset hook, hotkeys, pane
actions, context menu, openers) on each parent render and pointlessly
recomputing their useCallback / useMemo bodies. Wrap in useMemo so
launcher's reference is stable across renders.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
superset-sh#4101 import agents as v2 terminal presets with live link: - useV2PresetExecution.ts: drop upstream useV2AgentConfigs + useWorkspace import. useWorkspace arrives with superset-sh#4067 (cycle 44a WorkspaceProvider); fork keeps its workspaceId/projectId props until then. - V2AgentsSettings.tsx: adopt upstream initialAgentPresetId prop and useV2AgentConfigs re-import (fork retains the hook elsewhere). - PresetEditorDialog.tsx / V2PresetsSection.tsx: adopt upstream Switch / Link / ExternalLink additions for live-link UX. - useMigrateV1PresetsToV2/: re-delete fork-removed hook; drop import from V2PresetsSection.tsx (call sites were stub-only). - PresetEditorDialog.tsx: dedupe Switch import (one survived from each side of the merge). superset-sh#4107 v2 preset commands in new tabs (skipped): introduces useV2TerminalLauncher hook that takes a launcher prop through useV2PresetExecution / useDefaultContextMenuActions / useDefaultPaneActions / useWorkspacePaneOpeners. This pattern is tightly coupled to superset-sh#4067's WorkspaceProvider (workspaceId/projectId stop being passed down by page.tsx and instead come from useWorkspace()). Deferred to cycle 44a so the launcher and provider land together. superset-sh#4112 include agent args (skipped): depends on superset-sh#4107. superset-sh#4057 / superset-sh#4083 fix-compat: - RunInWorkspacePopoverV2.tsx / OpenInWorkspaceV2.tsx: drop iconUrl from project map and ProjectThumbnail props; pass githubOwner={null} until cycle 41 (superset-sh#3823 v2 project icon settings) lands. Phase 4 cycle 39 net: 3 cherry-picks landed (superset-sh#4057, superset-sh#4083, superset-sh#4101); 2 deferred to cycle 44a (superset-sh#4107, superset-sh#4112). useV2PresetExecution preserves fork's workspaceId/projectId calling convention.
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.
Summary
new-tab-per-commandmode) only ran the command in the LAST tab — earlier tabs sat empty until clicked. The v2 workspace only renders the active tab, so eachaddTabin the loop flippedactiveTabIdand unmounted the prior tab before itsTerminalPanecould callterminal.createSession. Background tabs never spawned a PTY.useV2TerminalLauncheris the single seam in v2 renderer code that callsterminal.createSession.mutate(...). One method:create({ command?, terminalId? }) => Promise<string>(mints whenterminalIdomitted, adopts when given — relying on host-servicecreateSessionidempotency).awaitslauncher.create(...)before writing the pane into the store: preset hook, hotkeys (NEW_GROUP/SPLIT_*), pane action button, context menu splits,useWorkspacePaneOpeners.addTerminalTab. Multi-pane / per-command presets fan out withPromise.allso they don't pay serial latency.TerminalPane.mountis back to plainmount() → connect(). It never callscreateSession; if a terminalId arrives without a live session, it's a launching-call-site bug, not the pane's problem.TerminalPaneData.initialCommandis gone — pane data is just the terminalId now.Test plan
new-tab-per-commandpreset with 3 commands — all 3 tabs run their commands without needing to be clickednew-tab-single,new-tab-multi-pane,active-tab-single,active-tab-multi-panepreset modes still workCmd+T/ NEW_GROUP hotkey opens a working empty terminalSummary by CodeRabbit