Update preset launch modes to split pane/new tab with parallel default#1715
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 a Launch Mode ("split-pane" | "new-tab") to terminal presets, normalizes legacy values, persists normalized presets, updates schema and settings router, renames UI label to "Launch Mode", and implements mode-aware preset opening via a new openPreset(workspaceId, preset, options) API. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Preset UI
participant Router as Settings Router
participant Store as useTabsWithPresets
participant Tabs as TabManager
User->>UI: create/select preset (may include executionMode)
UI->>Router: createPreset.mutate({ ..., executionMode? })
Router->>Router: getNormalizedTerminalPresets() → normalize & persist if needed
Router-->>UI: saved preset
User->>Store: openPreset(workspaceId, preset, {target?})
Store->>Store: preparePreset(preset) / normalizePresetMode
alt mode == "new-tab"
Store->>Tabs: create tab(s) per command (name tabs if specified)
Tabs-->>Store: tabId/paneId per created tab
else mode == "split-pane"
Store->>Tabs: create single tab with multiple panes (or update active tab)
Tabs-->>Store: tabId/paneId
end
Store-->>User: return {tabId, paneId}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts (2)
61-63: All tabs in "new-tab" mode get the same name.When a preset has multiple commands and runs in
"new-tab"mode, every created tab is renamed topreset.name. This could make it hard for users to distinguish between tabs. Consider appending a suffix (e.g., command index or a snippet of the command) to differentiate them.This is a UX design choice, so flagging for awareness rather than as a defect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts` around lines 61 - 63, When applying a preset that spawns multiple commands in "new-tab" mode, the current renameTab(result.tabId, preset.name) call gives every tab the same name; update the logic in useTabsWithPresets where renameTab is invoked (look for renameTab, result.tabId and preset.name) to append a unique suffix per command (for example use the command index or a short sanitized snippet of the command) so each created tab gets a distinct name like `${preset.name} #${i+1}` or `${preset.name} — ${shortCmd}`; ensure the suffixing happens only when preset.commands.length > 1 and keep renameTab usage consistent.
45-77: Dead fallback code on lines 66–74.The guard
if (!firstResult)on line 66 is unreachable. Entry to this block requireshasMultipleCommands(line 45), meaningcommands.length > 1, so theforloop on line 51 always executes at least once andfirstResultis always assigned.Suggested simplification
if (mode === "new-tab" && hasMultipleCommands) { - let firstResult: { - tabId: string; - paneId: string; - } | null = null; + let firstResult: { tabId: string; paneId: string } | undefined; for (const command of commands) { const result = storeAddTab(workspaceId, { initialCommands: [command], initialCwd: preset.cwd || undefined, }); if (!firstResult) { firstResult = result; } if (preset.name) { renameTab(result.tabId, preset.name); } } - if (!firstResult) { - const fallback = storeAddTab(workspaceId, { - initialCwd: preset.cwd || undefined, - }); - if (preset.name) { - renameTab(fallback.tabId, preset.name); - } - return fallback; - } - - return firstResult; + // hasMultipleCommands guarantees at least one iteration + return firstResult!; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts` around lines 45 - 77, The fallback branch checking if (!firstResult) is dead because hasMultipleCommands implies commands.length>1 so the for loop will always set firstResult; remove the unreachable block (the fallback that calls storeAddTab and renameTab) and simplify the logic in useTabsWithPresets.ts by ensuring the for loop sets firstResult and then returning firstResult after the loop (optionally assert it non-null), keeping calls to storeAddTab and renameTab as-is for each command; references: mode, hasMultipleCommands, commands, firstResult, storeAddTab, renameTab, preset.apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsx (1)
196-211: Template-based preset creation relies on server-side default forexecutionMode.
handleAddRowexplicitly passesexecutionMode: "split-pane"(line 201), buthandleAddTemplate(line 208) omits it, relying on the server's?? "split-pane"fallback. This works correctly but is a minor inconsistency — if the server default ever changes, template-created presets would silently pick up the new default while manually-added ones wouldn't.Consider passing
executionMode: "split-pane"in the template path too for explicitness:Suggested change
const handleAddTemplate = useCallback( (template: PresetTemplate) => { if (existingPresetNames.has(template.preset.name)) return; - createPreset.mutate(template.preset); + createPreset.mutate({ ...template.preset, executionMode: "split-pane" }); }, [createPreset, existingPresetNames], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsx` around lines 196 - 211, handleAddTemplate currently calls createPreset.mutate(template.preset) without setting executionMode, leading to inconsistency with handleAddRow which supplies executionMode: "split-pane"; update handleAddTemplate so that before calling createPreset.mutate it ensures the preset includes executionMode: "split-pane" (e.g., merge or override template.preset.executionMode) while keeping the existing existingPresetNames check and using the same createPreset.mutate call path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsx`:
- Around line 196-211: handleAddTemplate currently calls
createPreset.mutate(template.preset) without setting executionMode, leading to
inconsistency with handleAddRow which supplies executionMode: "split-pane";
update handleAddTemplate so that before calling createPreset.mutate it ensures
the preset includes executionMode: "split-pane" (e.g., merge or override
template.preset.executionMode) while keeping the existing existingPresetNames
check and using the same createPreset.mutate call path.
In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts`:
- Around line 61-63: When applying a preset that spawns multiple commands in
"new-tab" mode, the current renameTab(result.tabId, preset.name) call gives
every tab the same name; update the logic in useTabsWithPresets where renameTab
is invoked (look for renameTab, result.tabId and preset.name) to append a unique
suffix per command (for example use the command index or a short sanitized
snippet of the command) so each created tab gets a distinct name like
`${preset.name} #${i+1}` or `${preset.name} — ${shortCmd}`; ensure the suffixing
happens only when preset.commands.length > 1 and keep renameTab usage
consistent.
- Around line 45-77: The fallback branch checking if (!firstResult) is dead
because hasMultipleCommands implies commands.length>1 so the for loop will
always set firstResult; remove the unreachable block (the fallback that calls
storeAddTab and renameTab) and simplify the logic in useTabsWithPresets.ts by
ensuring the for loop sets firstResult and then returning firstResult after the
loop (optionally assert it non-null), keeping calls to storeAddTab and renameTab
as-is for each command; references: mode, hasMultipleCommands, commands,
firstResult, storeAddTab, renameTab, preset.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsxapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tspackages/local-db/src/schema/zod.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts (1)
87-108: Dead fallback — lines 104–108 are unreachableThe block is only entered when
hasMultipleCommands(commands.length > 1) is true, which guarantees the for-loop executes at least twice.firstResultis set on the very first iteration, soif (firstResult)is always true and the fallbackstoreAddTabat line 104 can never be reached.♻️ Proposed cleanup
- let firstResult: { tabId: string; paneId: string } | null = null; - + let firstResult = storeAddTab(workspaceId, { + initialCommands: [preset.commands[0]], + initialCwd: preset.initialCwd, + }); + applyTabName(firstResult.tabId, preset.name); + for (const command of preset.commands) { const result = storeAddTab(workspaceId, { initialCommands: [command], initialCwd: preset.initialCwd, }); - if (!firstResult) { - firstResult = result; - } applyTabName(result.tabId, preset.name); } - - if (firstResult) { - return firstResult; - } - - const fallback = storeAddTab(workspaceId, { - initialCwd: preset.initialCwd, - }); - applyTabName(fallback.tabId, preset.name); - return fallback; + return firstResult;Or if you prefer to keep the loop shape unchanged, simply remove the post-loop fallback and assert the result:
if (firstResult) { return firstResult; } - - const fallback = storeAddTab(workspaceId, { - initialCwd: preset.initialCwd, - }); - applyTabName(fallback.tabId, preset.name); - return fallback;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts` around lines 87 - 108, The fallback storeAddTab call is unreachable because firstResult is always set in the loop; remove the post-loop fallback (the block that calls storeAddTab and applyTabName for fallback) and instead return the firstResult found by the loop (or assert/throw if you want to be explicit). Update the code around firstResult, storeAddTab, and applyTabName so the function returns firstResult (non-null) after the loop and eliminate the dead fallback block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts`:
- Around line 87-108: The fallback storeAddTab call is unreachable because
firstResult is always set in the loop; remove the post-loop fallback (the block
that calls storeAddTab and applyTabName for fallback) and instead return the
firstResult found by the loop (or assert/throw if you want to be explicit).
Update the code around firstResult, storeAddTab, and applyTabName so the
function returns firstResult (non-null) after the loop and eliminate the dead
fallback block.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceInitEffects.tsxapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceInitEffects.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/desktop/src/renderer/stores/tabs/preset-launch.ts (1)
11-16:normalizePresetModeduplicatesnormalizePresetExecutionModefrompreset-execution-mode.ts.Both functions are identical in logic. Since the
shared/directory is already used by both the main and renderer processes (e.g.,shared/constants), this could be consolidated there to avoid drift.♻️ Suggested extraction to shared utilities
In
shared/preset-utils.ts(new file):export function normalizePresetMode(mode: unknown): "split-pane" | "new-tab" { if (mode === "new-tab") return "new-tab"; return "split-pane"; }Then both
preset-execution-mode.tsandpreset-launch.tscan import from there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/preset-launch.ts` around lines 11 - 16, normalizePresetMode duplicates normalizePresetExecutionMode; extract the shared logic into a single utility and import it where needed. Create a new exported function normalizePresetMode in a shared module (e.g., shared preset-utils) that returns "new-tab" or "split-pane" as currently implemented, replace the local implementation in preset-launch.ts (export function normalizePresetMode) and remove the duplicate in preset-execution-mode.ts so both modules import the new shared normalizePresetMode; ensure exported type remains PresetMode or the union "split-pane" | "new-tab" and update imports/usages accordingly.apps/desktop/src/renderer/stores/tabs/preset-launch.test.ts (1)
17-60: Missing test coverage for single-command launch plans.The
"new-tab-single"and"active-tab-single"branches ofgetPresetLaunchPlanare never exercised. Consider adding:➕ Suggested additional test cases
+ it("uses active-tab-single for active-tab target + split-pane + single command", () => { + expect( + getPresetLaunchPlan({ + mode: "split-pane", + target: "active-tab", + commandCount: 1, + hasActiveTab: true, + }), + ).toBe("active-tab-single"); + }); + + it("uses new-tab-single for a single command with no active tab", () => { + expect( + getPresetLaunchPlan({ + mode: "split-pane", + target: "active-tab", + commandCount: 1, + hasActiveTab: false, + }), + ).toBe("new-tab-single"); + }); + + it("uses new-tab-single for new-tab mode with single command", () => { + expect( + getPresetLaunchPlan({ + mode: "new-tab", + target: "new-tab", + commandCount: 1, + hasActiveTab: false, + }), + ).toBe("new-tab-single"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/preset-launch.test.ts` around lines 17 - 60, Add unit tests exercising the single-command branches of getPresetLaunchPlan: add a case where mode:"new-tab", target:"new-tab", commandCount:1 (hasActiveTab true/false as appropriate) expecting "new-tab-single", and a case where mode:"split-pane", target:"active-tab", commandCount:1 with hasActiveTab:true expecting "active-tab-single" and hasActiveTab:false expecting fallback "new-tab-single"; reference getPresetLaunchPlan to locate where to add these assertions.apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
46-66:getNormalizedTerminalPresetssilently writes to the DB — name doesn't reflect this.Every tRPC query that calls this function (e.g.
getTerminalPresets,getDefaultPreset) may trigger a DB write when legacyexecutionModevalues are found. This violates the expectation that queries are read-only and makes the side effect invisible to callers. A rename or a brief doc comment would significantly improve discoverability:♻️ Suggested rename + comment
-function getNormalizedTerminalPresets() { +/** + * Returns terminal presets with normalized executionMode values. + * Side effect: if any preset has a legacy/unknown mode, the normalized + * list is immediately persisted back to the DB (lazy one-time migration). + */ +function getAndMigrateTerminalPresets() {Update all call sites (
getNormalizedTerminalPresets→getAndMigrateTerminalPresets) accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/settings/index.ts` around lines 46 - 66, The function getNormalizedTerminalPresets does an implicit DB write when it detects legacy executionMode values; rename it to getAndMigrateTerminalPresets (or add a clear doc comment) to make the side effect explicit, update all call sites (e.g., getTerminalPresets, getDefaultPreset) to use the new name, and ensure any exports/imports are adjusted; leave the logic using shouldPersistNormalizedPresetModes, normalizeTerminalPresets and the localDb.insert(settings)...onConflictDoUpdate(...) block unchanged so the migration still runs but is discoverable from the function name or its new doc comment.
🤖 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/stores/tabs/useTabsWithPresets.ts`:
- Around line 83-106: The fallback block is unreachable because
hasMultipleCommands implies preset.commands.length > 1 so firstResult will
always be set; remove the dead code and simplify the "new-tab" branch by
returning the firstResult after the for-loop (use a non-null assertion or guard
if you prefer), i.e., keep the loop that calls storeAddTab and applyTabName,
then directly return firstResult (or throw/assert if null) instead of the
fallback storeAddTab/applyTabName block; reference symbols: preset.mode,
hasMultipleCommands, firstResult, storeAddTab, applyTabName.
In `@apps/docs/content/docs/terminal-presets.mdx`:
- Line 40: The phrase "Presets are parallel by default." is ambiguous; update
the copy to state that Split Pane is the default launch mode (e.g., replace the
sentence "Presets are parallel by default." with "Split Pane is the default
launch mode.") so it clearly communicates that Split Pane is the default rather
than implying anything about parallel vs sequential execution.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 46-66: The function getNormalizedTerminalPresets does an implicit
DB write when it detects legacy executionMode values; rename it to
getAndMigrateTerminalPresets (or add a clear doc comment) to make the side
effect explicit, update all call sites (e.g., getTerminalPresets,
getDefaultPreset) to use the new name, and ensure any exports/imports are
adjusted; leave the logic using shouldPersistNormalizedPresetModes,
normalizeTerminalPresets and the
localDb.insert(settings)...onConflictDoUpdate(...) block unchanged so the
migration still runs but is discoverable from the function name or its new doc
comment.
In `@apps/desktop/src/renderer/stores/tabs/preset-launch.test.ts`:
- Around line 17-60: Add unit tests exercising the single-command branches of
getPresetLaunchPlan: add a case where mode:"new-tab", target:"new-tab",
commandCount:1 (hasActiveTab true/false as appropriate) expecting
"new-tab-single", and a case where mode:"split-pane", target:"active-tab",
commandCount:1 with hasActiveTab:true expecting "active-tab-single" and
hasActiveTab:false expecting fallback "new-tab-single"; reference
getPresetLaunchPlan to locate where to add these assertions.
In `@apps/desktop/src/renderer/stores/tabs/preset-launch.ts`:
- Around line 11-16: normalizePresetMode duplicates
normalizePresetExecutionMode; extract the shared logic into a single utility and
import it where needed. Create a new exported function normalizePresetMode in a
shared module (e.g., shared preset-utils) that returns "new-tab" or "split-pane"
as currently implemented, replace the local implementation in preset-launch.ts
(export function normalizePresetMode) and remove the duplicate in
preset-execution-mode.ts so both modules import the new shared
normalizePresetMode; ensure exported type remains PresetMode or the union
"split-pane" | "new-tab" and update imports/usages accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/lib/trpc/routers/settings/preset-execution-mode.test.tsapps/desktop/src/lib/trpc/routers/settings/preset-execution-mode.tsapps/desktop/src/renderer/stores/tabs/preset-launch.test.tsapps/desktop/src/renderer/stores/tabs/preset-launch.tsapps/desktop/src/renderer/stores/tabs/useTabsWithPresets.tsapps/docs/content/docs/terminal-presets.mdx
| if (preset.mode === "new-tab" && hasMultipleCommands) { | ||
| let firstResult: { tabId: string; paneId: string } | null = null; | ||
|
|
||
| for (const command of preset.commands) { | ||
| const result = storeAddTab(workspaceId, { | ||
| initialCommands: [command], | ||
| initialCwd: preset.initialCwd, | ||
| }); | ||
| if (!firstResult) { | ||
| firstResult = result; | ||
| } | ||
| applyTabName(result.tabId, preset.name); | ||
| } | ||
|
|
||
| if (firstResult) { | ||
| return firstResult; | ||
| } | ||
|
|
||
| const fallback = storeAddTab(workspaceId, { | ||
| initialCwd: preset.initialCwd, | ||
| }); | ||
| applyTabName(fallback.tabId, preset.name); | ||
| return fallback; | ||
| } |
There was a problem hiding this comment.
Unreachable fallback code (lines 101–105).
hasMultipleCommands guarantees preset.commands.length > 1, so the for-loop always executes at least once and firstResult is always non-null after it. The if (firstResult) at line 97 always returns, making lines 101–105 dead code.
🧹 Proposed cleanup
if (preset.mode === "new-tab" && hasMultipleCommands) {
- let firstResult: { tabId: string; paneId: string } | null = null;
+ let firstResult!: { tabId: string; paneId: string };
for (const command of preset.commands) {
const result = storeAddTab(workspaceId, {
initialCommands: [command],
initialCwd: preset.initialCwd,
});
if (!firstResult) {
firstResult = result;
}
applyTabName(result.tabId, preset.name);
}
-
- if (firstResult) {
- return firstResult;
- }
-
- const fallback = storeAddTab(workspaceId, {
- initialCwd: preset.initialCwd,
- });
- applyTabName(fallback.tabId, preset.name);
- return fallback;
+ return firstResult;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts` around lines 83
- 106, The fallback block is unreachable because hasMultipleCommands implies
preset.commands.length > 1 so firstResult will always be set; remove the dead
code and simplify the "new-tab" branch by returning the firstResult after the
for-loop (use a non-null assertion or guard if you prefer), i.e., keep the loop
that calls storeAddTab and applyTabName, then directly return firstResult (or
throw/assert if null) instead of the fallback storeAddTab/applyTabName block;
reference symbols: preset.mode, hasMultipleCommands, firstResult, storeAddTab,
applyTabName.
| **New Tab** - Commands run in parallel in separate tabs, with one tab per command. | ||
|
|
||
| **Parallel** - Each command runs in its own split pane within a single tab. Useful for running a dev server and tests simultaneously. | ||
| Presets are parallel by default. |
There was a problem hiding this comment.
Ambiguous phrasing in "Presets are parallel by default."
Since both modes (Split Pane and New Tab) run commands in parallel, this line doesn't clearly distinguish itself. The intended meaning appears to be that Split Pane is the default launch mode, not that sequential execution is gone. Consider replacing it with:
-Presets are parallel by default.
+The default launch mode is Split Pane.📝 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.
| Presets are parallel by default. | |
| The default launch mode is Split Pane. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/docs/content/docs/terminal-presets.mdx` at line 40, The phrase "Presets
are parallel by default." is ambiguous; update the copy to state that Split Pane
is the default launch mode (e.g., replace the sentence "Presets are parallel by
default." with "Split Pane is the default launch mode.") so it clearly
communicates that Split Pane is the default rather than implying anything about
parallel vs sequential execution.
Summary
Sequential/ParalleltoSplit Pane/New TabSplit Pane: one tab with one pane per commandNew Tab: one tab per commandsplit-paneValidation
bun run typecheck --filter=@superset/desktop --filter=@superset/local-dbbunx biome check apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsx apps/desktop/src/lib/trpc/routers/settings/index.ts packages/local-db/src/schema/zod.tsSummary by CodeRabbit
Feature Updates
UI/UX
Stability