feat(desktop): split auto-apply preset into workspace creation and new tab settings#1320
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 terminal preset auto-apply flags and TRPC endpoints, extends preset schema, refactors TerminalSettings into modular sections, and updates hooks to use new preset selection for workspace creation and new tabs. New UI components manage auto-apply, link behavior, presets CRUD/reorder, and daemon sessions. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Settings UI
participant TRPC as TRPC Server
participant DB as Settings Store
User->>UI: Toggle auto-apply for workspace creation
UI->>TRPC: setPresetAutoApply({ id, field: "applyOnWorkspaceCreated" })
TRPC->>DB: Upsert terminalPresets with updated flag
DB-->>TRPC: Success
TRPC-->>UI: { success: true }
UI->>TRPC: getWorkspaceCreationPreset()
TRPC->>DB: Query presets (applyOnWorkspaceCreated first)
DB-->>TRPC: Matching TerminalPreset | null
TRPC-->>UI: TerminalPreset | null
UI->>User: Reflect updated auto-apply state
sequenceDiagram
participant System as Workspace Creator
participant Init as Workspace Init
participant TRPC as TRPC Server
participant DB as Settings Store
participant Tab as Tab Creator
System->>Init: Create new workspace
Init->>TRPC: getDefaultPreset()
TRPC->>DB: Query presets (applyOnWorkspaceCreated → isDefault)
DB-->>TRPC: Matching TerminalPreset | null
TRPC-->>Init: TerminalPreset | null
Init->>Tab: Create tab with preset (commands, cwd, exec mode)
Tab->>User: Terminal created with applied preset
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Kitenite
force-pushed
the
kitenite/allow-preset-to-run-on-worktree-created-vs-new-tab
branch
from
6968836 to
15fb047
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx (1)
692-722:⚠️ Potential issue | 🟡 MinorUpdate settings-search entry title to match UI label capitalization.
The settings-search entry for
TERMINAL_AUTO_APPLY_PRESEThas title "Apply Preset on Workspace Creation" (Title Case) at line 409 ofapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts, but the UI label at line 705 uses "Apply preset on workspace creation" (sentence case). Update the settings-search title to use sentence case for consistency so search results match the visible UI.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx (1)
128-131: Consider adding cleanup to disconnect DnD connectors.When the connector functions (
preview,drop,drag) change, the effect re-runs and re-attaches, but the previous connectors aren't explicitly disconnected. Passingnullon cleanup is the defensive pattern recommended byreact-dndto avoid stale connector bindings.🛡️ Suggested cleanup
useEffect(() => { preview(drop(rowRef)); drag(dragHandleRef); + return () => { + preview(null); + drop(null); + drag(null); + }; }, [preview, drop, drag]);
…w tab settings Give users independent control over when the default preset is applied: one toggle for workspace/worktree creation and another for new tabs, panes, and splits. Explicit preset launches via hotkey/menu are never gated. Refactors TerminalSettings from a monolithic component into isolated sub-components (PresetsSection, SessionsSection, individual setting components), each owning their own queries. This prevents query resolutions in one section from re-rendering others, fixing the "Maximum update depth exceeded" error caused by Radix UI ref composition cascading through react-dnd connectors on re-renders.
Kitenite
force-pushed
the
kitenite/allow-preset-to-run-on-worktree-created-vs-new-tab
branch
from
15fb047 to
4b28a47
Compare
… global toggle Replace the global "apply preset on new tab" setting with per-preset flags (applyOnWorkspaceCreated, applyOnNewTab) that give users granular control over when each preset auto-applies. Each preset now shows two toggle icons in the row: folder+ for workspace creation and document+ for new tab.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx`:
- Around line 135-140: The toggle UI fails to turn off legacy presets because
setPresetAutoApply doesn't clear isDefault when explicitly setting
applyOnWorkspaceCreated/applyOnNewTab; update the mutation logic in
setPresetAutoApply to map over presets and for the preset being changed set
[input.field] = input.id === p.id ? true : undefined AND clear isDefault for
that preset (isDefault = input.id === p.id ? undefined : p.isDefault), so legacy
presets with only isDefault can be transitioned off; verify related UI booleans
isWorkspaceCreation and isNewTab still derive from
preset.applyOnWorkspaceCreated/applyOnNewTab || (!preset.applyOnNewTab &&
preset.isDefault) etc.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/stores/tabs/useTabsWithPresets.ts (1)
167-175: Aliased return name may confuse consumers.
defaultPreset: newTabPresetat line 174 aliases the new tab preset asdefaultPresetfor backward compatibility. This works, but the semantics have changed — consumers may assumedefaultPresetis theisDefaultpreset. Consider renaming tonewTabPresetin consumers or adding a brief inline comment explaining the alias.apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsx (1)
21-35:SectionListkey extraction relies on unsafe cast.Line 27 casts every child to
React.ReactElementto access.key, but afterfilter(Boolean), children could still include non-elementReactNodetypes (strings, numbers). In practice, callers only pass JSX elements so this works, but the cast is fragile.A safer approach would use
React.isValidElement:♻️ Suggested improvement
- <div - key={(child as React.ReactElement).key ?? i} - className={i > 0 ? "pt-6 border-t mt-6" : ""} - > + <div + key={React.isValidElement(child) ? child.key ?? i : i} + className={i > 0 ? "pt-6 border-t mt-6" : ""} + >apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection.tsx (3)
247-258: Deleted row remains visible until server round-trip completes.
handleDeleteRowfiresdeletePreset.mutatebut returnscurrentLocalunchanged, so the row stays rendered untilserverPresetsupdates via invalidation and the syncuseEffectruns. This creates a brief flash where the user could click delete again (double-delete).Consider optimistically removing the row from local state immediately:
♻️ Suggested fix
const handleDeleteRow = useCallback( (rowIndex: number) => { setLocalPresets((currentLocal) => { const preset = currentLocal[rowIndex]; if (preset) { deletePreset.mutate({ id: preset.id }); } - return currentLocal; + return currentLocal.filter((_, i) => i !== rowIndex); }); }, [deletePreset], );
143-162: State setter used as a read-only accessor is unconventional.
handleCellBlur,handleCommandsBlur, andhandleDeleteRowall callsetLocalPresets(currentLocal => { ...; return currentLocal; })purely to readcurrentLocalwithout modifying state. While React does bail out on same-reference returns, this pattern is non-obvious and may confuse future maintainers.A cleaner alternative would be to use a ref (e.g.,
localPresetsRef) to access the latest local state in these callbacks, or restructure so the blur handler receives the preset ID/data directly.
260-268: No optimistic update for auto-apply toggle.
handleToggleAutoApplyfires the mutation but doesn't optimistically updatelocalPresets. The UI will appear unresponsive until the server round-trip completes and theuseEffectsyncs server state. Given that the PresetRow readspreset.applyOnWorkspaceCreated/applyOnNewTabdirectly, the toggle button won't reflect the change until the next render cycle after invalidation.Consider optimistically toggling the field in
localPresetsalongside the mutation for a snappier UX, consistent with howhandleExecutionModeChange(line 210) already does local + server updates together.
| const isWorkspaceCreation = | ||
| preset.applyOnWorkspaceCreated || | ||
| (!preset.applyOnNewTab && preset.isDefault); | ||
| const isNewTab = | ||
| preset.applyOnNewTab || | ||
| (!preset.applyOnWorkspaceCreated && preset.isDefault); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setPresetAutoApply function definition
rg -n 'setPresetAutoApply|setPreset.*AutoApply' --type=ts --type=tsx -B2 -A30 | head -100Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for setPresetAutoApply function definition without file type
rg -n 'setPresetAutoApply|setPreset.*AutoApply' -B2 -A30 | head -150Repository: superset-sh/superset
Length of output: 17150
🏁 Script executed:
#!/bin/bash
# Get the complete setPresetAutoApply mutation implementation
rg -n 'setPresetAutoApply: publicProcedure' -A50 'apps/desktop/src/lib/trpc/routers/settings/index.ts'Repository: superset-sh/superset
Length of output: 1591
🏁 Script executed:
#!/bin/bash
# Check PresetRow toggle logic around lines 186-243
sed -n '186,243p' 'apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx'Repository: superset-sh/superset
Length of output: 1897
🏁 Script executed:
#!/bin/bash
# Check if there are any places where isDefault is cleared or modified
rg -n 'isDefault' 'apps/desktop/src/lib/trpc/routers/settings/index.ts' -B3 -A3Repository: superset-sh/superset
Length of output: 902
🏁 Script executed:
#!/bin/bash
# Verify the complete flow: check what PresetRow sends when toggling each scenario
sed -n '135,243p' 'apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx' | grep -n 'onToggleAutoApply\|isWorkspaceCreation\|isNewTab'Repository: superset-sh/superset
Length of output: 709
Server mutation doesn't clear isDefault when explicitly setting applyOn* fields, causing toggle-off to silently fail for legacy presets.
When a preset has only isDefault: true (both applyOnWorkspaceCreated and applyOnNewTab undefined), both buttons render active. Clicking an active button sends id: null to toggle off, but the server mutation treats null like any other non-matching ID and sets the field to undefined—which is already its current state. This leaves the preset unchanged, so both buttons remain active and cannot be toggled off.
To align with the migration strategy, setPresetAutoApply should clear isDefault when explicitly enabling one of the applyOn* fields (similar to how setDefaultPreset mutually clears other presets' isDefault):
const updatedPresets = presets.map((p) => ({
...p,
[input.field]: input.id === p.id ? true : undefined,
isDefault: input.id === p.id ? undefined : p.isDefault, // Clear isDefault when explicitly setting
}));This ensures users can transition legacy presets out of the isDefault-only state via the toggle UI.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsx`
around lines 135 - 140, The toggle UI fails to turn off legacy presets because
setPresetAutoApply doesn't clear isDefault when explicitly setting
applyOnWorkspaceCreated/applyOnNewTab; update the mutation logic in
setPresetAutoApply to map over presets and for the preset being changed set
[input.field] = input.id === p.id ? true : undefined AND clear isDefault for
that preset (isDefault = input.id === p.id ? undefined : p.isDefault), so legacy
presets with only isDefault can be transitioned off; verify related UI booleans
isWorkspaceCreation and isNewTab still derive from
preset.applyOnWorkspaceCreated/applyOnNewTab || (!preset.applyOnNewTab &&
preset.isDefault) etc.
…un-on-worktree-created-vs-new-tab
Replace icon buttons with clickable WS/Tab badges that toggle independently. Multiple presets can now be tagged for the same trigger - all tagged presets fire when a workspace is created or a new tab opens. - setPresetAutoApply toggles per-preset (no longer mutually exclusive) - getWorkspaceCreationPresets/getNewTabPresets return arrays - WorkspaceInitEffects applies all workspace-tagged presets - useTabsWithPresets opens a tab for each new-tab-tagged preset
Kitenite
deleted the
kitenite/allow-preset-to-run-on-worktree-created-vs-new-tab
branch
1 participant
Summary
autoApplyDefaultPresettoggle into two independent settings: one for workspace/worktree creation and one for new tabs/panes/splitsChanges
Database (
packages/local-db)apply_preset_on_new_tabboolean column to thesettingstableShared constants (
apps/desktop/src/shared/constants.ts)DEFAULT_APPLY_PRESET_ON_NEW_TAB = truedefaulttRPC settings router
getApplyPresetOnNewTabquery andsetApplyPresetOnNewTabmutationTab store (
useTabsWithPresets.ts)defaultPresetOptions,shouldUseParallelMode, and tab rename behind itopenPreset()(explicit user action) remains ungatedSettings UI (
TerminalSettings.tsx)Settings search (
settings-search.ts)TERMINAL_APPLY_PRESET_ON_NEW_TABsearch entryTest Plan
openPreset()(explicit preset launch via hotkey/menu) always works regardless of settingsbun run typecheckpassesSummary by CodeRabbit
New Features
Improvements
API