feat(desktop): support big and compact terminal add buttons#1757
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a compact-vs-big terminal add-button feature: DB migration and schema field, new DEFAULT_USE_COMPACT_TERMINAL_ADD_BUTTON constant, TRPC getters/setters for the compact toggle, a new AddTabButton (with PresetsSubmenu) UI component, and GroupStrip wired to use the component and setting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as GroupStrip / AddTabButton
participant TRPC as Settings Router
participant DB as Local Settings DB
User->>UI: Mount workspace / view tabs
UI->>TRPC: getUseCompactTerminalAddButton()
TRPC->>DB: SELECT use_big_terminal_add_button FROM settings
DB-->>TRPC: value or null
TRPC-->>UI: computed compact flag (or DEFAULT_USE_COMPACT_TERMINAL_ADD_BUTTON)
UI->>UI: render AddTabButton (compact or big)
User->>UI: Toggle compact add button
UI->>TRPC: setUseCompactTerminalAddButton({ enabled })
TRPC->>DB: INSERT ... ON CONFLICT UPDATE use_big_terminal_add_button
DB-->>TRPC: success
TRPC-->>UI: { success: true }
UI->>TRPC: refetch getUseCompactTerminalAddButton()
TRPC->>DB: SELECT ...
DB-->>TRPC: updated value
TRPC-->>UI: new value
UI->>UI: re-render accordingly
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/AddTabButton.tsx`:
- Around line 1-137: The icon-only DropdownMenuTrigger buttons (the chevron
button inside the big add group and the compact LuPlus button) lack accessible
names; update the AddTabButton component so each icon-only Button (the ones
wrapping HiMiniChevronDown and LuPlus) receives a clear accessible label (e.g.
aria-label="Open add menu" or aria-label="Add tab") and/or a title prop to
provide a screen-reader/tooltip name; keep the existing asChild usage and ensure
you add the aria-label on the Button elements (the children of
DropdownMenuTrigger) so screen readers can announce their purpose.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/AddTabButton.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/index.tsapps/desktop/src/shared/constants.tspackages/local-db/drizzle/0034_add_use_big_terminal_add_button_setting.sqlpackages/local-db/drizzle/meta/0034_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
| import { Button } from "@superset/ui/button"; | ||
| import { | ||
| DropdownMenu, | ||
| DropdownMenuCheckboxItem, | ||
| DropdownMenuContent, | ||
| DropdownMenuItem, | ||
| DropdownMenuSeparator, | ||
| DropdownMenuTrigger, | ||
| } from "@superset/ui/dropdown-menu"; | ||
| import { BsTerminalPlus } from "react-icons/bs"; | ||
| import { HiMiniChevronDown } from "react-icons/hi2"; | ||
| import { LuPlus } from "react-icons/lu"; | ||
| import { TbMessageCirclePlus, TbWorld } from "react-icons/tb"; | ||
| import { HotkeyMenuShortcut } from "renderer/components/HotkeyMenuShortcut"; | ||
| import { NewTabDropZone } from "../../NewTabDropZone"; | ||
|
|
||
| interface AddTabButtonProps { | ||
| hasAiChat: boolean; | ||
| showBigAddButton: boolean; | ||
| showPresetsBar: boolean; | ||
| onDropToNewTab: (paneId: string) => void; | ||
| isLastPaneInTab: (paneId: string) => boolean; | ||
| onAddTerminal: () => void; | ||
| onAddChat: () => void; | ||
| onAddBrowser: () => void; | ||
| onToggleShowPresetsBar: (enabled: boolean) => void; | ||
| onToggleBigAddButton: (enabled: boolean) => void; | ||
| } | ||
|
|
||
| export function AddTabButton({ | ||
| hasAiChat, | ||
| showBigAddButton, | ||
| showPresetsBar, | ||
| onDropToNewTab, | ||
| isLastPaneInTab, | ||
| onAddTerminal, | ||
| onAddChat, | ||
| onAddBrowser, | ||
| onToggleShowPresetsBar, | ||
| onToggleBigAddButton, | ||
| }: AddTabButtonProps) { | ||
| return ( | ||
| <NewTabDropZone onDrop={onDropToNewTab} isLastPaneInTab={isLastPaneInTab}> | ||
| <DropdownMenu> | ||
| <div className="flex items-center shrink-0"> | ||
| {showBigAddButton ? ( | ||
| <> | ||
| <Button | ||
| variant="outline" | ||
| className="h-7 rounded-r-none pl-2 pr-1.5 gap-1 text-xs" | ||
| onClick={onAddTerminal} | ||
| > | ||
| <BsTerminalPlus className="size-3.5" /> | ||
| Terminal | ||
| </Button> | ||
| {hasAiChat && ( | ||
| <Button | ||
| variant="outline" | ||
| className="h-7 rounded-none border-l-0 px-1.5 gap-1 text-xs" | ||
| onClick={onAddChat} | ||
| > | ||
| <TbMessageCirclePlus className="size-3.5" /> | ||
| Chat | ||
| </Button> | ||
| )} | ||
| <Button | ||
| variant="outline" | ||
| className="h-7 rounded-none border-l-0 px-1.5 gap-1 text-xs" | ||
| onClick={onAddBrowser} | ||
| > | ||
| <TbWorld className="size-3.5" /> | ||
| Browser | ||
| </Button> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| variant="outline" | ||
| size="icon" | ||
| className="size-7 rounded-l-none border-l-0 px-1" | ||
| > | ||
| <HiMiniChevronDown className="size-3" /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| </> | ||
| ) : ( | ||
| <DropdownMenuTrigger asChild> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-7 px-1 rounded-md border border-border/60 bg-muted/30 text-muted-foreground hover:bg-accent/60 hover:text-foreground" | ||
| > | ||
| <LuPlus className="size-3.5" strokeWidth={1.8} /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| )} | ||
| </div> | ||
| <DropdownMenuContent align="end" className="w-56"> | ||
| {!showBigAddButton && ( | ||
| <> | ||
| <DropdownMenuItem onClick={onAddTerminal} className="gap-2"> | ||
| <BsTerminalPlus className="size-4" /> | ||
| <span>Terminal</span> | ||
| <HotkeyMenuShortcut hotkeyId="NEW_GROUP" /> | ||
| </DropdownMenuItem> | ||
| {hasAiChat && ( | ||
| <DropdownMenuItem onClick={onAddChat} className="gap-2"> | ||
| <TbMessageCirclePlus className="size-4" /> | ||
| <span>Chat</span> | ||
| <HotkeyMenuShortcut hotkeyId="NEW_CHAT" /> | ||
| </DropdownMenuItem> | ||
| )} | ||
| <DropdownMenuItem onClick={onAddBrowser} className="gap-2"> | ||
| <TbWorld className="size-4" /> | ||
| <span>Browser</span> | ||
| <HotkeyMenuShortcut hotkeyId="NEW_BROWSER" /> | ||
| </DropdownMenuItem> | ||
| <DropdownMenuSeparator /> | ||
| </> | ||
| )} | ||
| <DropdownMenuCheckboxItem | ||
| checked={showPresetsBar} | ||
| onCheckedChange={onToggleShowPresetsBar} | ||
| onSelect={(e) => e.preventDefault()} | ||
| > | ||
| Show Preset Bar | ||
| </DropdownMenuCheckboxItem> | ||
| <DropdownMenuCheckboxItem | ||
| checked={showBigAddButton} | ||
| onCheckedChange={onToggleBigAddButton} | ||
| onSelect={(e) => e.preventDefault()} | ||
| > | ||
| Use Big Add Button | ||
| </DropdownMenuCheckboxItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| </NewTabDropZone> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add accessible labels to icon-only triggers.
The chevron and compact-plus buttons have no accessible name, which makes them hard to use with screen readers.
♿️ Suggested fix
@@
<DropdownMenuTrigger asChild>
<Button
variant="outline"
size="icon"
className="size-7 rounded-l-none border-l-0 px-1"
+ aria-label="Add tab options"
>
@@
<DropdownMenuTrigger asChild>
<Button
variant="ghost"
size="icon"
className="size-7 px-1 rounded-md border border-border/60 bg-muted/30 text-muted-foreground hover:bg-accent/60 hover:text-foreground"
+ aria-label="Add tab"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/AddTabButton.tsx`
around lines 1 - 137, The icon-only DropdownMenuTrigger buttons (the chevron
button inside the big add group and the compact LuPlus button) lack accessible
names; update the AddTabButton component so each icon-only Button (the ones
wrapping HiMiniChevronDown and LuPlus) receives a clear accessible label (e.g.
aria-label="Open add menu" or aria-label="Add tab") and/or a title prop to
provide a screen-reader/tooltip name; keep the existing asChild usage and ensure
you add the aria-label on the Button elements (the children of
DropdownMenuTrigger) so screen readers can announce their purpose.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/electron.vite.config.ts`:
- Around line 105-112: The build packaging is missing mastracode and
`@ast-grep/napi` so they are externalized by rollup (see rollupOptions.external)
but not copied into the packaged app; update the electron-builder files
configuration in electron-builder.ts to add entries for
"node_modules/mastracode" and "node_modules/@ast-grep/napi" (same pattern used
for better-sqlite3, node-pty, friendly-words) so those folders are included in
the distributable, and if either contains native binaries add their paths to the
asarUnpack config so native modules are unpacked outside the ASAR.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
apps/desktop/electron.vite.config.tsapps/desktop/package.json
| external: [ | ||
| "electron", | ||
| "better-sqlite3", | ||
| "node-pty", | ||
| "pg-native", | ||
| "mastracode", | ||
| "@ast-grep/napi", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find packaging/copy configuration files ==="
fd "copy-native-modules|electron-builder" -t f
echo
echo "=== Check for copy-native-modules implementation ==="
fd "copy-native-modules" -t f -x head -100 {}
echo
echo "=== Inspect electron.vite.config.ts externals section ==="
rg -n -A 10 "external:\s*\[" apps/desktop/electron.vite.config.ts
echo
echo "=== Search for handling of `@ast-grep/napi` and mastracode ==="
rg -rn "@ast-grep/napi|mastracode" --type ts --type json --type yml --type yaml apps/
echo
echo "=== Check electron-builder config if exists ==="
fd "electron-builder" apps/desktop/ -t f -x cat {}
echo
echo "=== Look for package.json scripts and build config ==="
cat apps/desktop/package.json | head -50Repository: superset-sh/superset
Length of output: 3226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect electron-builder.ts configuration ==="
head -200 apps/desktop/electron-builder.ts
echo
echo "=== Check for extraResources, asarUnpack, or files configuration ==="
rg -n "extraResources|asarUnpack|files|nativeModules" apps/desktop/electron-builder.ts
echo
echo "=== Look for mastracode and `@ast-grep/napi` in package.json ==="
rg -A2 -B2 "mastracode|@ast-grep/napi" apps/desktop/package.json
echo
echo "=== Check if `@ast-grep/napi` or mastracode are symlinks in node_modules ==="
ls -la apps/desktop/node_modules/@ast-grep 2>/dev/null | head -20 || echo "Directory check skipped"
ls -la apps/desktop/node_modules/mastracode 2>/dev/null | head -20 || echo "Directory check skipped"
echo
echo "=== Verify copy-native-modules.ts is run as part of build ==="
rg -n "copy-native-modules" apps/desktop/package.jsonRepository: superset-sh/superset
Length of output: 6055
Add mastracode and @ast-grep/napi to electron-builder packaging rules.
These modules are externalized in rollupOptions.external but are not included in the files configuration in electron-builder.ts. They will not be copied to the packaged application, causing runtime failures when the app attempts to load them.
Add both modules to the files configuration in electron-builder.ts (alongside better-sqlite3, node-pty, and friendly-words):
Example addition to electron-builder.ts files config
{
from: "node_modules/mastracode",
to: "node_modules/mastracode",
filter: ["**/*"],
},
{
from: "node_modules/@ast-grep/napi",
to: "node_modules/@ast-grep/napi",
filter: ["**/*"],
},If either module contains native binaries, also add them to the asarUnpack configuration so electron-builder unpacks them outside the asar archive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/electron.vite.config.ts` around lines 105 - 112, The build
packaging is missing mastracode and `@ast-grep/napi` so they are externalized by
rollup (see rollupOptions.external) but not copied into the packaged app; update
the electron-builder files configuration in electron-builder.ts to add entries
for "node_modules/mastracode" and "node_modules/@ast-grep/napi" (same pattern
used for better-sqlite3, node-pty, friendly-words) so those folders are included
in the distributable, and if either contains native binaries add their paths to
the asarUnpack config so native modules are unpacked outside the ASAR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (2)
307-315: Overflow spacer width is brittle due to hard-coded values.At Line 309–Line 314, fixed widths (
220px,170px,w-10) can desync from the actual rendered add control after style/text changes, causing overlap/clipping in overflow mode. Prefer measuring the real control width (ref +ResizeObserver) and using that value for the spacer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx` around lines 307 - 315, The spacer div in GroupStrip using hard-coded widths should be replaced with a measured width derived from the actual add-control element: add a ref (e.g., addButtonRef) on the add control rendered by the GroupStrip component, create a ResizeObserver (or use useResizeObserver hook) to track its current offsetWidth, store that width in state (e.g., addButtonWidth), and apply that numeric width to the spacer div (replace the conditional className block with an inline style or tailwind arbitrary value using addButtonWidth) so the spacer always matches the real control size; update references to showBigAddButton and hasAiChat only to select which control to measure if there are multiple variants.
58-95: Extract a shared helper for optimistic boolean setting mutations.
setShowPresetsBarandsetUseBigTerminalAddButtonduplicate the same optimistic update/rollback/invalidate flow. A small helper would reduce drift and keep behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx` around lines 58 - 95, Extract a small helper that returns the optimistic mutation handlers and use it for both setShowPresetsBar and setUseBigTerminalAddButton to remove duplication: create a function (e.g., createOptimisticBooleanHandlers) that accepts the utils query accessor (like utils.settings.getShowPresetsBar or utils.settings.getUseBigTerminalAddButton) and returns an object with onMutate, onError, and onSettled implementing the same flow (cancel(), getData(), setData(undefined, enabled), return { previous }, rollback on error, and invalidate() on settled); then replace the inline handler objects in setShowPresetsBar and setUseBigTerminalAddButton with the returned handlers from createOptimisticBooleanHandlers passed their respective utils getter. Ensure you reference the exact utils accessors (utils.settings.getShowPresetsBar and utils.settings.getUseBigTerminalAddButton) and keep the mutation signatures intact.
🤖 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/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 177-183: handleOpenPreset currently opens a preset into the active
tab by calling openPreset(activeWorkspaceId, preset, { target: "active-tab" });
change this to create a new tab to match other "add" actions — call openPreset
with the appropriate new-tab target (e.g. { target: "new-tab" } or the project’s
equivalent) using the same activeWorkspaceId and preset; update the call inside
handleOpenPreset (function name: handleOpenPreset, called symbol: openPreset,
param type: TerminalPreset, guard: activeWorkspaceId) and keep the existing
useCallback dependency array unchanged.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 307-315: The spacer div in GroupStrip using hard-coded widths
should be replaced with a measured width derived from the actual add-control
element: add a ref (e.g., addButtonRef) on the add control rendered by the
GroupStrip component, create a ResizeObserver (or use useResizeObserver hook) to
track its current offsetWidth, store that width in state (e.g., addButtonWidth),
and apply that numeric width to the spacer div (replace the conditional
className block with an inline style or tailwind arbitrary value using
addButtonWidth) so the spacer always matches the real control size; update
references to showBigAddButton and hasAiChat only to select which control to
measure if there are multiple variants.
- Around line 58-95: Extract a small helper that returns the optimistic mutation
handlers and use it for both setShowPresetsBar and setUseBigTerminalAddButton to
remove duplication: create a function (e.g., createOptimisticBooleanHandlers)
that accepts the utils query accessor (like utils.settings.getShowPresetsBar or
utils.settings.getUseBigTerminalAddButton) and returns an object with onMutate,
onError, and onSettled implementing the same flow (cancel(), getData(),
setData(undefined, enabled), return { previous }, rollback on error, and
invalidate() on settled); then replace the inline handler objects in
setShowPresetsBar and setUseBigTerminalAddButton with the returned handlers from
createOptimisticBooleanHandlers passed their respective utils getter. Ensure you
reference the exact utils accessors (utils.settings.getShowPresetsBar and
utils.settings.getUseBigTerminalAddButton) and keep the mutation signatures
intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/AddTabButton.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/AddTabButton/AddTabButton.tsx
| const handleOpenPreset = useCallback( | ||
| (preset: TerminalPreset) => { | ||
| if (!activeWorkspaceId) return; | ||
| openPreset(activeWorkspaceId, preset, { target: "active-tab" }); | ||
| }, | ||
| [activeWorkspaceId, openPreset], | ||
| ); |
There was a problem hiding this comment.
Preset selection currently targets the active tab instead of creating a new tab.
At Line 180, openPreset(..., { target: "active-tab" }) makes preset selection behave differently from the other add actions in this menu (Terminal/Chat/Browser), which are tab-creation oriented.
Proposed fix
- openPreset(activeWorkspaceId, preset, { target: "active-tab" });
+ openPreset(activeWorkspaceId, preset, { target: "new-tab" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleOpenPreset = useCallback( | |
| (preset: TerminalPreset) => { | |
| if (!activeWorkspaceId) return; | |
| openPreset(activeWorkspaceId, preset, { target: "active-tab" }); | |
| }, | |
| [activeWorkspaceId, openPreset], | |
| ); | |
| const handleOpenPreset = useCallback( | |
| (preset: TerminalPreset) => { | |
| if (!activeWorkspaceId) return; | |
| openPreset(activeWorkspaceId, preset, { target: "new-tab" }); | |
| }, | |
| [activeWorkspaceId, openPreset], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`
around lines 177 - 183, handleOpenPreset currently opens a preset into the
active tab by calling openPreset(activeWorkspaceId, preset, { target:
"active-tab" }); change this to create a new tab to match other "add" actions —
call openPreset with the appropriate new-tab target (e.g. { target: "new-tab" }
or the project’s equivalent) using the same activeWorkspaceId and preset; update
the call inside handleOpenPreset (function name: handleOpenPreset, called
symbol: openPreset, param type: TerminalPreset, guard: activeWorkspaceId) and
keep the existing useCallback dependency array unchanged.
Summary
+and big segmented add controlsAddTabButtoncomponentChanges
use_big_terminal_add_buttonto local settings schema + migrationgetUseBigTerminalAddButtonsetUseBigTerminalAddButtonGroupStripto consume the setting and render either modeUse Big Add Buttoncheckbox to the add-menuGroupStrip/components/AddTabButton/AddTabButton.tsxValidation
bunx biome check apps/desktop/src/shared/constants.ts apps/desktop/src/lib/trpc/routers/settings/index.ts apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx packages/local-db/src/schema/schema.tsbun run typecheck --filter=@superset/local-db --filter=@superset/desktopbun run typecheck --filter=@superset/desktopSummary by CodeRabbit
New Features
Improvements
Chores