feature (desktop): presets bar + group strip UX polish#1727
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds optional pinnedToBar to TerminalPreset schema and TRPC inputs; converts presets UI to a dropdown-driven manager with pin toggles and optimistic TRPC updates; refactors tab strip for horizontal overflow and plus-control; adds HotkeyMenuShortcut and NEW_CHAT hotkey; tweaks top/side bar translucency and enables the presets bar by default. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Renderer UI
participant TRPC as Electron TRPC
participant DB as LocalDB
User->>UI: Open Presets dropdown & toggle pin
UI->>TRPC: mutate updateTerminalPreset(presetId, { pinnedToBar: true/false })
TRPC->>DB: write terminal_preset { id, pinnedToBar }
DB-->>TRPC: success
TRPC-->>UI: mutation result
UI->>UI: optimistic update reconciled (pinnedPresets recalculated)
UI-->>User: updated pinned state shown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
AviPeltz
changed the title
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx (1)
128-257:⚠️ Potential issue | 🟠 MajorConfirm parent component conditionally mounts entire
PresetsBarbased onshowPresetsBar.In
ContentView/index.tsxline 21, the entirePresetsBarcomponent is conditionally mounted ({showPresetsBar && <PresetsBar />}). WhenshowPresetsBarisfalse, the cog button used to re-enable it becomes inaccessible. While users can toggle this setting fromGroupStrip, the pinned presets section should either:
- Always render the cog button (via the parent, before conditional mount), or
- Never conditionally unmount the component and instead hide only the pinned presets area.
🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 128 - 257, The PresetsBar cog button becomes inaccessible when ContentView conditionally unmounts the entire <PresetsBar /> based on showPresetsBar; either move the cog trigger out of that conditional into the parent so it always renders, or change ContentView to always mount PresetsBar and let PresetsBar itself hide only the pinnedPresets area when showPresetsBar is false. Concretely: in ContentView/index.tsx stop using "{showPresetsBar && <PresetsBar />}" and instead always render "<PresetsBar showPresetsBar={showPresetsBar} />" (or render the DropdownMenu/cog button JSX before the conditional), then inside PresetsBar use the showPresetsBar prop to conditionally render the pinnedPresets mapping while keeping the cog DropdownMenu (DropdownMenuTrigger/DropdownMenuContent) always mounted so users can re-enable the bar.
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx (3)
94-100: Implicit pin-by-default for legacy presets is a deliberate choice — worth a brief comment.
pinnedToBar === falseis the only "unpinned" state;undefined(all legacy/existing presets) is treated as pinned. This ensures backward compatibility but may surprise future readers. A one-line comment here would help.📝 Suggested clarification comment
const pinnedPresets = useMemo( () => - presets.flatMap((preset, index) => - preset.pinnedToBar === false ? [] : [{ preset, index }], + // Legacy presets without pinnedToBar are treated as pinned (backward compat) + presets.flatMap((preset, index) => + preset.pinnedToBar === false ? [] : [{ preset, index }], ), [presets], );🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 94 - 100, The current useMemo that builds pinnedPresets treats any preset where pinnedToBar is undefined as pinned (only pinnedToBar === false is unpinned), which is intentional for backward compatibility but not documented; add a one-line clarifying comment above the useMemo (referencing pinnedPresets, presets, useMemo and the pinnedToBar property) stating that legacy presets default to pinned unless explicitly set to false to avoid surprising future readers.
55-61:PresetShortcutis a separate component in the same file.Per coding guidelines, each component should live in its own file. Since this is a tiny, tightly-coupled helper, extracting it is optional — but worth noting for guideline compliance.
As per coding guidelines, "No multi-component files - use one component per file."
🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 55 - 61, The file contains a secondary component PresetShortcut (using useHotkeyText and rendering DropdownMenuShortcut) inside PresetsBar; extract PresetShortcut into its own file (e.g., PresetShortcut.tsx) as a named/default exported React component, move its dependencies (useHotkeyText import and any related types like HotkeyId), update the original PresetsBar to import PresetShortcut where it was used, and run typechecks to ensure HotkeyId and DropdownMenuShortcut imports remain correct; keep the component behavior identical (return null when useHotkeyText === "Unassigned").
33-53: Consider co-locatingPresetTemplateinterface andQUICK_ADD_PRESET_TEMPLATESconstant.These are only consumed by
PresetsBar. If they grow or get reused, extracting them to a sibling file (e.g.,PresetsBar/preset-templates.ts) would keep the component file focused. Fine for now. As per coding guidelines, "Co-locate hooks, utils, constants, config, stories, and dependencies next to the files using them."🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 33 - 53, The PresetTemplate interface and QUICK_ADD_PRESET_TEMPLATES constant are declared inside PresetsBar but are only consumed by that component; move them to a new sibling module (e.g., PresetTemplates or preset-templates) to co-locate constants and keep the component focused, export both PresetTemplate and QUICK_ADD_PRESET_TEMPLATES from the new file, then replace the inline definitions in PresetsBar with an import of those symbols and update any references to PresetTemplate and QUICK_ADD_PRESET_TEMPLATES accordingly.apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
56-88: Consider settingpinnedToBar: trueexplicitly on default presets.Default presets omit
pinnedToBar, relying on the UI treatingundefinedas pinned. While this works, explicitly marking them as pinned would make the intent clear and prevent surprises if the default-pin logic ever changes.📝 Example for one entry
const DEFAULT_PRESETS: Omit<TerminalPreset, "id">[] = [ { name: "claude", description: "Danger mode: All permissions auto-approved", cwd: "", commands: ["claude --dangerously-skip-permissions"], + pinnedToBar: true, }, // ... same for others ];🤖 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 56 - 88, DEFAULT_PRESETS relies on implicit UI behavior for pinning; explicitly add pinnedToBar: true to each preset object in the DEFAULT_PRESETS array (the constant named DEFAULT_PRESETS of type Omit<TerminalPreset, "id">) so the intent is clear and resistant to future UI changes—update every entry (e.g., "claude", "codex", "copilot", "opencode", "gemini") to include pinnedToBar: true and ensure the TerminalPreset shape accepts this field.
🤖 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/components/PresetsBar/PresetsBar.tsx`:
- Around line 90-93: presetByName currently creates a Map keyed by preset.name
which lets later presets with duplicate names silently shadow earlier ones;
update the memo to key by a unique identifier (e.g., preset.id) or build a
multi-value map (Map<string, Preset[]>), and then update any consumers such as
managedPresets to look up by id or handle multiple matches accordingly (refer to
useMemo => presetByName, the presets array, and the managedPresets logic) so
duplicate names no longer hide earlier presets.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 65-68: The overflow flag (hasHorizontalOverflow) is oscillating
because track.scrollWidth includes the plusControl when it conditionally moves
in/out; fix by measuring tabs-only width and subtracting a fixed reserved width
for the plus-control before comparing to container width. Update the logic that
reads scrollContainerRef and tabsTrackRef (where you currently compute
scrollWidth/containerWidth and call setHasHorizontalOverflow) to instead compute
tabsOnlyWidth = tabsTrackRef.querySelector(...) or measure children excluding
the plusControl, then compare tabsOnlyWidth + RESERVED_PLUS_WIDTH to
scrollContainerRef.clientWidth; keep RESERVED_PLUS_WIDTH consistent (e.g., match
the plusControl CSS width) and use that in the hasHorizontalOverflow calculation
so setHasHorizontalOverflow stops flipping when the plusControl moves.
- Around line 38-46: The MenuItemShortcut component should be moved into its own
file: create a new folder MenuItemShortcut with MenuItemShortcut.tsx exporting
the MenuItemShortcut component (using the existing implementation that
references HotkeyId, useHotkeyText and DropdownMenuShortcut) and add a barrel
index.ts that re-exports it; then replace the inline definition in
GroupStrip.tsx with an import of MenuItemShortcut from that barrel. Ensure prop
typing (HotkeyId) and the conditional return (null when hotkeyText ===
"Unassigned") are preserved exactly.
- Around line 203-209: The icon-only Button in GroupStrip.tsx (the <Button>
wrapping the <LuPlus /> icon) lacks an accessible label; update the Button
component to provide an explicit accessible name (for example via an aria-label
or title prop such as aria-label="Add group" or similar) or include visually
hidden text alongside the LuPlus icon so screen readers get context; locate the
Button that contains LuPlus and add the label to that element.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx`:
- Around line 128-257: The PresetsBar cog button becomes inaccessible when
ContentView conditionally unmounts the entire <PresetsBar /> based on
showPresetsBar; either move the cog trigger out of that conditional into the
parent so it always renders, or change ContentView to always mount PresetsBar
and let PresetsBar itself hide only the pinnedPresets area when showPresetsBar
is false. Concretely: in ContentView/index.tsx stop using "{showPresetsBar &&
<PresetsBar />}" and instead always render "<PresetsBar
showPresetsBar={showPresetsBar} />" (or render the DropdownMenu/cog button JSX
before the conditional), then inside PresetsBar use the showPresetsBar prop to
conditionally render the pinnedPresets mapping while keeping the cog
DropdownMenu (DropdownMenuTrigger/DropdownMenuContent) always mounted so users
can re-enable the bar.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 56-88: DEFAULT_PRESETS relies on implicit UI behavior for pinning;
explicitly add pinnedToBar: true to each preset object in the DEFAULT_PRESETS
array (the constant named DEFAULT_PRESETS of type Omit<TerminalPreset, "id">) so
the intent is clear and resistant to future UI changes—update every entry (e.g.,
"claude", "codex", "copilot", "opencode", "gemini") to include pinnedToBar: true
and ensure the TerminalPreset shape accepts this field.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx`:
- Around line 94-100: The current useMemo that builds pinnedPresets treats any
preset where pinnedToBar is undefined as pinned (only pinnedToBar === false is
unpinned), which is intentional for backward compatibility but not documented;
add a one-line clarifying comment above the useMemo (referencing pinnedPresets,
presets, useMemo and the pinnedToBar property) stating that legacy presets
default to pinned unless explicitly set to false to avoid surprising future
readers.
- Around line 55-61: The file contains a secondary component PresetShortcut
(using useHotkeyText and rendering DropdownMenuShortcut) inside PresetsBar;
extract PresetShortcut into its own file (e.g., PresetShortcut.tsx) as a
named/default exported React component, move its dependencies (useHotkeyText
import and any related types like HotkeyId), update the original PresetsBar to
import PresetShortcut where it was used, and run typechecks to ensure HotkeyId
and DropdownMenuShortcut imports remain correct; keep the component behavior
identical (return null when useHotkeyText === "Unassigned").
- Around line 33-53: The PresetTemplate interface and QUICK_ADD_PRESET_TEMPLATES
constant are declared inside PresetsBar but are only consumed by that component;
move them to a new sibling module (e.g., PresetTemplates or preset-templates) to
co-locate constants and keep the component focused, export both PresetTemplate
and QUICK_ADD_PRESET_TEMPLATES from the new file, then replace the inline
definitions in PresetsBar with an import of those symbols and update any
references to PresetTemplate and QUICK_ADD_PRESET_TEMPLATES 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/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsxapps/desktop/src/shared/constants.tspackages/local-db/src/schema/zod.ts
Comment on lines
65
to
+68
| const hasAiChat = useFeatureFlagEnabled(FEATURE_FLAGS.AI_CHAT); | ||
| const { presets } = usePresets(); | ||
| const isDark = useIsDarkTheme(); | ||
| const scrollContainerRef = useRef<HTMLDivElement>(null); | ||
| const tabsTrackRef = useRef<HTMLDivElement>(null); | ||
| const [hasHorizontalOverflow, setHasHorizontalOverflow] = useState(false); |
Contributor
There was a problem hiding this comment.
Prevent overflow detection from oscillating near the threshold.
Because plusControl moves in/out of the scroll track (and the spacer width differs), track.scrollWidth can change across renders, making hasHorizontalOverflow flip repeatedly when tabs are near the boundary. That can cause flicker and extra layout work. Stabilize overflow detection by measuring tabs-only width and reserving a consistent plus-control width.
🔧 Suggested fix (stable overflow calculation)
const scrollContainerRef = useRef<HTMLDivElement>(null);
const tabsTrackRef = useRef<HTMLDivElement>(null);
+const tabsListRef = useRef<HTMLDivElement>(null);
+const plusControlRef = useRef<HTMLDivElement>(null);
const [hasHorizontalOverflow, setHasHorizontalOverflow] = useState(false);
const updateOverflow = useCallback(() => {
const container = scrollContainerRef.current;
- const track = tabsTrackRef.current;
- if (!container || !track) return;
- setHasHorizontalOverflow(track.scrollWidth > container.clientWidth + 1);
+ const tabsList = tabsListRef.current;
+ const plusControlEl = plusControlRef.current;
+ if (!container) return;
+ const tabsWidth = tabsList?.scrollWidth ?? 0;
+ const reserved = plusControlEl?.offsetWidth ?? 0;
+ setHasHorizontalOverflow(tabsWidth + reserved > container.clientWidth + 1);
}, []);
...
- <div ref={tabsTrackRef} className="flex items-stretch">
+ <div ref={tabsTrackRef} className="flex items-stretch">
{tabs.length > 0 && (
- <div className="flex items-stretch h-full shrink-0">
+ <div ref={tabsListRef} className="flex items-stretch h-full shrink-0">
{tabs.map(...)}
</div>
)}
{hasHorizontalOverflow ? (
<div className="h-full w-10 shrink-0" />
) : (
- <div className="shrink-0">{plusControl}</div>
+ <div ref={plusControlRef} className="shrink-0">{plusControl}</div>
)}
</div>
...
- {hasHorizontalOverflow && (
- <div className="shrink-0 bg-background/95 pr-1">{plusControl}</div>
- )}
+ {hasHorizontalOverflow && (
+ <div ref={plusControlRef} className="shrink-0 bg-background/95 pr-1">
+ {plusControl}
+ </div>
+ )}Also applies to: 168-173, 252-285
🤖 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 65 - 68, The overflow flag (hasHorizontalOverflow) is oscillating
because track.scrollWidth includes the plusControl when it conditionally moves
in/out; fix by measuring tabs-only width and subtracting a fixed reserved width
for the plus-control before comparing to container width. Update the logic that
reads scrollContainerRef and tabsTrackRef (where you currently compute
scrollWidth/containerWidth and call setHasHorizontalOverflow) to instead compute
tabsOnlyWidth = tabsTrackRef.querySelector(...) or measure children excluding
the plusControl, then compare tabsOnlyWidth + RESERVED_PLUS_WIDTH to
scrollContainerRef.clientWidth; keep RESERVED_PLUS_WIDTH consistent (e.g., match
the plusControl CSS width) and use that in the hasHorizontalOverflow calculation
so setHasHorizontalOverflow stops flipping when the plusControl moves.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (2)
191-197:⚠️ Potential issue | 🟡 MinorAdd an accessible label to the icon-only button.
Screen readers need a name for the “+” control.
🛠️ Suggested fix
<Button variant="ghost" size="icon" + aria-label="Add tab" className="size-7 px-1 shrink-0 rounded-md border border-border/60 bg-muted/30 text-muted-foreground hover:bg-accent/60 hover:text-foreground" >🤖 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 191 - 197, The icon-only Button in GroupStrip.tsx (the Button containing <LuPlus />) lacks an accessible name; update that Button (the one with variant="ghost" and size="icon") to provide a clear accessible label by adding an aria-label (e.g., aria-label="Add group") or by including visually hidden text associated with the button so screen readers can announce its purpose; ensure the label is concise and descriptive and avoid changing the visual appearance.
156-173:⚠️ Potential issue | 🟠 MajorStabilize overflow detection to avoid flip-flopping.
track.scrollWidthchanges when the plus control moves in/out of the track, which can causehasHorizontalOverflowto oscillate near the threshold. Consider measuring tabs-only width and reserving a fixed plus-control width so the calculation is stable.Also applies to: 265-274
🤖 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 156 - 173, The overflow toggle is unstable because track.scrollWidth includes the plus-control when it moves; update the updateOverflow logic to measure only the tabs area and subtract/ reserve a fixed width for the plus-control instead of using raw track.scrollWidth: use scrollContainerRef and tabsTrackRef to compute tabsOnlyWidth (e.g., sum child widths or query a tabs-only element) and compare tabsOnlyWidth against container.clientWidth minus a reservedPlusControlWidth constant, then setHasHorizontalOverflow based on that stable threshold; apply the same change to the other overflow detection instance that uses updateOverflow-like logic so both places use the reserved plus-control width calculation.
🧹 Nitpick comments (1)
packages/local-db/src/schema/zod.ts (1)
59-70: Confirm the implicit "pinned by default" semantic for existing presets.
z.boolean().optional()makespinnedToBarabsent for all existing stored presets. Per the PR description the UI renders a preset wheneverpinnedToBar !== false, so every legacy preset will appear in the bar immediately upon upgrade — no migration needed, but this is a behavioral side-effect worth an explicit acknowledgment in the schema comment (or a Zod.default(true)to make the intent self-documenting).💡 Optional: make the default explicit
- pinnedToBar: z.boolean().optional(), + /** When absent or true the preset is shown in the quick-launch bar; set to false to hide it. */ + pinnedToBar: z.boolean().optional(),Or, if a runtime default is desirable:
- pinnedToBar: z.boolean().optional(), + pinnedToBar: z.boolean().default(true),Note: switching to
.default(true)changes the inferred type fromboolean | undefinedtobooleanand meansparse({…})will always materialise the field — verify that the tRPC update mutation'spinnedToBar !== undefinedguard still behaves correctly before applying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/local-db/src/schema/zod.ts` around lines 59 - 70, The schema terminalPresetSchema currently declares pinnedToBar as z.boolean().optional(), which leaves legacy presets without the key and thus treated as "pinned by default" by the UI; either add an explicit comment above terminalPresetSchema noting this implicit semantic for existing records, or change pinnedToBar to z.boolean().default(true) to make the default explicit at runtime—if you choose .default(true) verify consumers (notably any tRPC update mutation that checks pinnedToBar !== undefined) still behave correctly since the inferred type will become boolean rather than boolean | undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 191-197: The icon-only Button in GroupStrip.tsx (the Button
containing <LuPlus />) lacks an accessible name; update that Button (the one
with variant="ghost" and size="icon") to provide a clear accessible label by
adding an aria-label (e.g., aria-label="Add group") or by including visually
hidden text associated with the button so screen readers can announce its
purpose; ensure the label is concise and descriptive and avoid changing the
visual appearance.
- Around line 156-173: The overflow toggle is unstable because track.scrollWidth
includes the plus-control when it moves; update the updateOverflow logic to
measure only the tabs area and subtract/ reserve a fixed width for the
plus-control instead of using raw track.scrollWidth: use scrollContainerRef and
tabsTrackRef to compute tabsOnlyWidth (e.g., sum child widths or query a
tabs-only element) and compare tabsOnlyWidth against container.clientWidth minus
a reservedPlusControlWidth constant, then setHasHorizontalOverflow based on that
stable threshold; apply the same change to the other overflow detection instance
that uses updateOverflow-like logic so both places use the reserved plus-control
width calculation.
---
Nitpick comments:
In `@packages/local-db/src/schema/zod.ts`:
- Around line 59-70: The schema terminalPresetSchema currently declares
pinnedToBar as z.boolean().optional(), which leaves legacy presets without the
key and thus treated as "pinned by default" by the UI; either add an explicit
comment above terminalPresetSchema noting this implicit semantic for existing
records, or change pinnedToBar to z.boolean().default(true) to make the default
explicit at runtime—if you choose .default(true) verify consumers (notably any
tRPC update mutation that checks pinnedToBar !== undefined) still behave
correctly since the inferred type will become boolean rather than boolean |
undefined.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/renderer/components/HotkeyMenuShortcut/HotkeyMenuShortcut.tsxapps/desktop/src/renderer/components/HotkeyMenuShortcut/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/PresetMenuItemShortcut/PresetMenuItemShortcut.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/PresetMenuItemShortcut/index.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsxapps/desktop/src/shared/hotkeys.tspackages/local-db/src/schema/zod.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/PresetMenuItemShortcut/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/components/PresetMenuItemShortcut/PresetMenuItemShortcut.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx
- apps/desktop/src/lib/trpc/routers/settings/index.ts
# Conflicts: # apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (2)
229-235: [Unresolved from previous review] Icon-only button still missing an accessible label.The
<Button>wrapping<LuPlus />has noaria-label, so assistive technology announces it as a generic "button" with no context.🤖 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 229 - 235, The icon-only Button in GroupStrip (the <Button> wrapping <LuPlus /> inside GroupStrip.tsx) lacks an accessible label; update that Button to include an explicit aria-label (or aria-labelledby) with a clear, concise description such as "Add group" (or similar context-appropriate text) so assistive tech can announce its purpose; ensure you add the prop on the same Button element that renders <LuPlus /> (or add visually hidden text associated via aria-labelledby) rather than relying on title-only tooltips.
194-199: [Unresolved from previous review] Overflow detection oscillates whenplusControlstraddles the boundary.
track.scrollWidthat line 198 includes theplusControlwhenhasHorizontalOverflowisfalse. Once overflow is detected andplusControlis evicted from the track,scrollWidthdecreases, potentially making the condition false again — causing a flip-flop on every measurement cycle.The previous review already proposed a stable fix (separate
tabsListRef/plusControlRefand measuring tabs-only width). That fix hasn't been applied yet.Also applies to: 303-311
🤖 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 194 - 199, The overflow detection in updateOverflow currently uses tabsTrackRef.scrollWidth which can include the plusControl and cause oscillation when the plusControl straddles the boundary; update updateOverflow to measure only the tabs area (not the plusControl) by adding/using a tabsListRef (or a plusControlRef) and compute tabsWidth = tabsListRef.current.scrollWidth (or track.scrollWidth - plusControlRef.current.offsetWidth if you prefer) then setHasHorizontalOverflow(tabsWidth > scrollContainerRef.current.clientWidth + 1); update any other mirror checks (lines ~303-311) to the same tabs-only measurement to stabilize behavior.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx (1)
210-220: Minor redundancy: window resize listener and deferredrequestAnimationFramecall can be removed.
- Line 210:
window.addEventListener("resize", updateOverflow)is redundant —ResizeObserveralready observescontainer, which resizes with the window, so the window event never fires first.- Lines 218-220:
updateOverflowis stable (emptyuseCallbackdeps), so thisuseEffectruns once on mount, scheduling a deferred duplicate of the synchronousupdateOverflow()call already made insideuseLayoutEffecton line 206. It's only useful to catch post-paint layout changes (e.g. font loading), which aren't a concern in Electron.♻️ Suggested cleanup
useLayoutEffect(() => { const container = scrollContainerRef.current; const track = tabsTrackRef.current; if (!container || !track) return; updateOverflow(); const resizeObserver = new ResizeObserver(updateOverflow); resizeObserver.observe(container); resizeObserver.observe(track); - window.addEventListener("resize", updateOverflow); return () => { resizeObserver.disconnect(); - window.removeEventListener("resize", updateOverflow); }; }, [updateOverflow]); - useEffect(() => { - requestAnimationFrame(updateOverflow); - }, [updateOverflow]);🤖 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 210 - 220, Remove the redundant window resize listener and the extra useEffect that calls requestAnimationFrame(updateOverflow): rely solely on the existing ResizeObserver attached in the useLayoutEffect and call updateOverflow there; delete the window.addEventListener("resize", updateOverflow) registration and its matching removeEventListener in the cleanup, and remove the separate useEffect that schedules requestAnimationFrame(updateOverflow). Ensure the cleanup in the component still disconnects the resizeObserver (resizeObserver.disconnect()) so only the observer is cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 229-235: The icon-only Button in GroupStrip (the <Button> wrapping
<LuPlus /> inside GroupStrip.tsx) lacks an accessible label; update that Button
to include an explicit aria-label (or aria-labelledby) with a clear, concise
description such as "Add group" (or similar context-appropriate text) so
assistive tech can announce its purpose; ensure you add the prop on the same
Button element that renders <LuPlus /> (or add visually hidden text associated
via aria-labelledby) rather than relying on title-only tooltips.
- Around line 194-199: The overflow detection in updateOverflow currently uses
tabsTrackRef.scrollWidth which can include the plusControl and cause oscillation
when the plusControl straddles the boundary; update updateOverflow to measure
only the tabs area (not the plusControl) by adding/using a tabsListRef (or a
plusControlRef) and compute tabsWidth = tabsListRef.current.scrollWidth (or
track.scrollWidth - plusControlRef.current.offsetWidth if you prefer) then
setHasHorizontalOverflow(tabsWidth > scrollContainerRef.current.clientWidth +
1); update any other mirror checks (lines ~303-311) to the same tabs-only
measurement to stabilize behavior.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx`:
- Around line 210-220: Remove the redundant window resize listener and the extra
useEffect that calls requestAnimationFrame(updateOverflow): rely solely on the
existing ResizeObserver attached in the useLayoutEffect and call updateOverflow
there; delete the window.addEventListener("resize", updateOverflow) registration
and its matching removeEventListener in the cleanup, and remove the separate
useEffect that schedules requestAnimationFrame(updateOverflow). Ensure the
cleanup in the component still disconnects the resizeObserver
(resizeObserver.disconnect()) so only the observer is cleaned up.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupStrip/GroupStrip.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx (1)
88-99:presetsByNameduplicate-name fix confirmed. ✓The map now correctly accumulates all presets per name into
Preset[]arrays, andmanagedPresetsconsumes only[0]for the template primary match. Duplicate-named presets surface as separatecustomExistingrows as intended.🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 88 - 99, The Map value type is incorrect: change the declaration in PresetsBar for presetsByName from Map<string, typeof presets> to Map<string, Preset[]> (or Array<Preset>) and ensure the loop builds arrays of Preset objects from the presets iterable; update any related type imports/aliases so presets is typed as Preset[] and usages like managedPresets that access presetsByName.get(name)?.[0] remain valid.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx (1)
177-218: Two optional UX improvements for managed preset rows.
Dropdown stays open after checkbox toggle but closes after pin/unpin.
DropdownMenuCheckboxItem(line 225) correctly usesonSelect={(e) => e.preventDefault()}so the menu stays open for successive checkbox changes. The same pattern would benefit preset rows — users pinning/unpinning multiple presets currently have to reopen the dropdown each time.
disabledis broader than necessary.createPreset.isPendingdisables every row, including rows for existing presets that only callupdatePreset.mutate. Consider tightening this per row type.♻️ Proposed refactor
<DropdownMenuItem key={item.key} className="gap-2" - disabled={createPreset.isPending} + disabled={hasPreset ? updatePreset.isPending : createPreset.isPending} + onSelect={(e) => { + if (hasPreset) e.preventDefault(); // keep open for multi-pin + }} onClick={() => {Note:
hasPresetmust be defined before the JSX. Move theconst hasPreset = !!item.presetdeclaration above thereturn.🤖 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/components/PresetsBar/PresetsBar.tsx` around lines 177 - 218, The preset menu rows close when clicked and are overly disabled; before returning JSX move const hasPreset = !!item.preset above the return, then change the DropdownMenuItem onClick to call e.preventDefault() (like DropdownMenuCheckboxItem) so the menu stays open when pinning/unpinning multiple items, and tighten the disabled logic so rows are disabled per-action (e.g., disable creation rows when createPreset.isPending and disable existing-preset rows when updatePreset.isPending) while keeping the existing updatePreset.mutate and createPreset.mutate behavior that toggles pinnedToBar for item.preset / item.template.preset.
🤖 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/components/PresetsBar/PresetsBar.tsx`:
- Around line 153-155: The icon-only Button in PresetsBar (the Button containing
HiMiniCog6Tooth) is missing an accessible name; update the Button component in
PresetsBar (where HiMiniCog6Tooth is rendered) to include an accessible label
(e.g., aria-label="Settings" or aria-labelledby referencing a visible label) so
screen readers announce its purpose; ensure the label matches the Tooltip text
and use aria-hidden on the icon if your Button implementation needs it.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx`:
- Around line 88-99: The Map value type is incorrect: change the declaration in
PresetsBar for presetsByName from Map<string, typeof presets> to Map<string,
Preset[]> (or Array<Preset>) and ensure the loop builds arrays of Preset objects
from the presets iterable; update any related type imports/aliases so presets is
typed as Preset[] and usages like managedPresets that access
presetsByName.get(name)?.[0] remain valid.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx`:
- Around line 177-218: The preset menu rows close when clicked and are overly
disabled; before returning JSX move const hasPreset = !!item.preset above the
return, then change the DropdownMenuItem onClick to call e.preventDefault()
(like DropdownMenuCheckboxItem) so the menu stays open when pinning/unpinning
multiple items, and tighten the disabled logic so rows are disabled per-action
(e.g., disable creation rows when createPreset.isPending and disable
existing-preset rows when updatePreset.isPending) while keeping the existing
updatePreset.mutate and createPreset.mutate behavior that toggles pinnedToBar
for item.preset / item.template.preset.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/components/PresetsBar/PresetsBar.tsx
Comment on lines
+153
to
+155
| <Button variant="ghost" size="icon" className="size-6 shrink-0"> | ||
| <HiMiniCog6Tooth className="size-3.5" /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Icon-only button is missing an accessible name.
The Tooltip provides a visual label but not an aria-label for screen readers. Without it, assistive technologies will announce this as an unlabeled button.
♿ Proposed fix
-<Button variant="ghost" size="icon" className="size-6 shrink-0">
+<Button variant="ghost" size="icon" className="size-6 shrink-0" aria-label="Manage Presets">🤖 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/components/PresetsBar/PresetsBar.tsx`
around lines 153 - 155, The icon-only Button in PresetsBar (the Button
containing HiMiniCog6Tooth) is missing an accessible name; update the Button
component in PresetsBar (where HiMiniCog6Tooth is rendered) to include an
accessible label (e.g., aria-label="Settings" or aria-labelledby referencing a
visible label) so screen readers announce its purpose; ensure the label matches
the Tooltip text and use aria-hidden on the icon if your Button implementation
needs it.
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
Overhauls the desktop presets bar and group strip UX. The presets bar now defaults to visible and introduces pin/unpin semantics so users control which presets appear. The group strip's "+" button is simplified to core tab types and stays accessible even when tabs overflow.
What changed
Presets bar (
PresetsBar.tsx)AGENT_TYPES). Each row shows a pin/unpin toggle or a "+" icon for templates not yet created. Keyboard shortcuts are displayed inline viaDropdownMenuShortcut.pinnedToBar !== falserender as quick-launch buttons. Clicking a preset in the manage dropdown toggles its pin state; clicking a template that doesn't exist yet creates it pinned.Group strip (
GroupStrip.tsx)ResizeObserver+useLayoutEffectto detect when tabs exceed the container width. When they do, the "+" button pins to the right edge outside the scroll area so it's always reachable.useNavigate,usePresets,HiStar,HiMiniChevronDown,PresetMenuItemShortcut, tooltip wrappers). Replaced inline button group with a single compact icon button.Data model
pinnedToBar: booleanto the terminal preset schema (packages/local-db/src/schema/zod.ts) and plumbed it through the settings tRPCcreateandupdatemutations.Defaults & styling
DEFAULT_SHOW_PRESETS_BARflipped fromfalse→trueso the bar is visible out of the box.bg-backgroundtobg-muted/45 dark:bg-muted/35for a subtle tinted look.Test plan
Summary by CodeRabbit
New Features
Hotkeys
Style