style(desktop): unified settings vocabulary across every page#4035
Merged
Conversation
Replace shadcn Card with bordered row-stack pattern (matching 1Code). Bump avatar to 48px with explicit Change button instead of hover-pencil. Fix right-column width inconsistency (avatar w-8 vs inputs flex-1) by giving every row a fixed w-80 control. Wrap Sign out in its own box so the page reads as two parallel containers instead of card + bare button.
- Sentence case all section headings (Markdown style, Editor font, Terminal font, Import theme, Download base file) - Standardize header rhythm: h3 mb-1 + xs description mb-3 - Declutter Theme header: fold Marketplace and Docs links into a natural-language description line - Markdown: merge the trailing Tufte note into the section description so the section ends with the control, not prose - Font Reset button: ghost text-xs muted -> outline sm so it's discoverable next to the controls - Add aria-label to font size Input and Markdown Select for a11y
Wrap the Outlet in a centered max-width container so every settings page sits in the middle of the content pane instead of pinned to the left edge of the sidebar. The pages' own max-w-4xl becomes redundant but harmless; new pages get centered for free.
- Sentence case headings and button labels (Notification sound, Add / Replace custom audio) - Broaden subtitle so it covers toggle + volume + ringtone, not just the picker - Drop the dangling tip block at the bottom; fold the format hint into the section description under Notification sound - Swap HiPlus -> HiArrowPath when the button means Replace, so the icon matches the action - Drop the decorative speaker icon from the Volume label so it matches every other setting label on the page - Tighten section gap from space-y-8 to space-y-6 to match Behavior
- Drop the ring around selected cards: border-2 + ring-2 + check was three signals for one state. Now border-primary + check + a subtle footer tint, single-thickness border so cards don't shift on select. - Promote the play button: bigger (h-9), centered in the preview area with a shadow, so it reads as the primary affordance instead of a corner hint. Emoji moves below the button as visual identity. - Move duration inline with the name in the footer (it's metadata, not decoration) and drop the floating top-right badge. - Add aria-label and aria-pressed for keyboard / SR users.
Centered play overlapping the emoji read as a single broken element, not as two distinct affordances. Restore the corner placement and the 4xl emoji. Keep the other RingtoneCard wins from the previous commit (no ring on selected, inline duration, aria labels).
Check moves to top-right of the preview area as a small badge so the emoji area stays uncluttered. Play moves into the footer alongside the name + duration, where it sits next to the metadata it acts on. Both affordances now have a single, predictable home each.
Drops the always-hidden ({false && ...}) telemetry toggle and its
useQuery/useMutation/handler plumbing from BehaviorSettings, plus the
matching SETTING_ITEM_ID, variant gate, and search index entry. The
underlying getTelemetryEnabled tRPC procedure stays because
TelemetrySync still consumes it at boot to set the PostHog opt state.
- Match other settings pages: text-xl page title, sentence case
("Keyboard shortcuts"), p-6 max-w-4xl w-full wrapper order.
- Drop the inner max-h-[calc(100vh-320px)] scroll: it doubled up with
the parent settings pane scroll and made the page feel cramped.
Parent now scrolls naturally.
- Drop the loud uppercase Command/Shortcut column header inside each
category card -- the rows are clearly label + chip, no header needed.
- Promote "Reset all" from ghost to outline so it reads as a page-level
action, distinct from the per-row Reset buttons.
- Recording state now visually highlights the active row (subtle
destructive-tinted bg + ring on the trigger) and changes the trigger
text from "Recording..." to "Press a key...".
Sentence case headings ("Git & worktrees", "Branch prefix") and tighten
the Branch prefix description so it explains the setting in addition
to showing the preview. WorktreeLocationPicker is left untouched
because v1 ProjectSettings still consumes it.
- Page wrapper max-w-5xl -> max-w-4xl: the layout already caps at 4xl so the wider value was silently dead. Match the rest of settings. - Reset to defaults: ghost -> outline so it reads as a page-level action distinct from row-level controls (matches Keyboard's Reset all). - Friendlier zero-state: dashed-border card with a Bot icon, "No agents yet" heading, and a hint pointing at the Add menu, instead of a single bare line of muted text. - Loading: skeleton cards that mirror the V2AgentCard layout instead of "Loading agent settings...". - Drop the empty grey square placeholder when a preset has no icon (in both the card and the Add-agent dropdown). Empty rectangles read as broken; absence reads as intentional.
Card was reading as a "toy" -- big rounded shadow on every row, app icon at 32px, semibold title, big primary-blue argv/stdin pills. - Card: rounded-xl + shadow-sm + py-6/gap-6 -> rounded-lg, no shadow, no internal gap. Flatter, less floating widget energy. - Header: text-base semibold title -> text-sm medium title; description text-sm -> text-xs muted; preset icon size-8 -> size-7. Header padding p-4 -> p-3 to fit the smaller type. - Drag handle: bare icon with no affordance -> icon in a hover-bg pill with a slightly muted default color so it reads as draggable. - Trash: hover -> destructive tint to make the destructive action read as destructive only on intent. - Padding alignment: expanded CardContent inherited shadcn px-6 (24px) while the header was p-4 (16px), so fields were inset further than the title above them. Drop CardContent and use plain px-4 pb-4. - Field grouping: insert a border-t between Label (identity) and the three launch fields so the form has a readable two-section structure. - Field labels shrunk to text-xs and inputs use text-xs font-mono so the dense launch settings fit without feeling oversized. - Argv/stdin: replace two primary-blue pill buttons with a true segmented control (single bordered group, accent-tinted selected half). Reads as one decision, not two competing buttons. - Tighten copy: drop "(empty)" placeholder for "--prompt"; compress the argv/stdin explanation into one line.
1Code's settings list pattern is one bordered container with divide-y rows -- no individual cards per item. Agents had 8 separate Cards floating with space-y-3 between them, which made each row look like its own widget instead of a list entry. - V2AgentsSettings: wrap configs in `rounded-lg border bg-card divide-y` instead of `space-y-3`. - V2AgentCard: drop the per-item <Card> wrapper. The row sits flat inside the parent container; hover bg lifts via hover:bg-muted/50 to match 1Code AccountRow. - Drag state: when isDragging, lift the row via bg-accent/40 + z-10 so it visually sits above the rest of the list during reorder. Less floating-widget energy, denser, reads as a settings list.
…elds The expanded panel previously stacked Label/Input/description vertically for each field. 1Code's settings forms (model config, account config, etc.) put label + hint on the left and the control on the right at a fixed w-80, separated by border-t. Same row vocabulary as the rest of settings, so the expanded content reads continuous with the page instead of like a popup form. - New local AgentField helper renders one row: label + hint left, control (w-80, shrink-0) right, separated by border-t. - All four fields (Label, Command, Prompt-only args, Prompt transport) use the new pattern. - Drop the previous nested grouping divider; row dividers carry the separation now. - Promote field labels back to text-sm so they line up visually with the agent's title in the header above.
Same pattern as the v2 agents list: one bordered container with divide-y rows, no per-item bg, no per-item chrome. Each ringtone is one row with [emoji] [name + duration on top, description below] [play button] [check]. Sound selection is a list-of-options task, not visual comparison, so cards were wasting vertical space and making each option feel like a widget instead of a list entry.
- Page wrapper max-w-7xl -> max-w-4xl: layout caps at 4xl already, so the wider value was dead. Match the rest of settings. - Sentence case "Terminal presets" / "Add preset" header. - Drop the orphan "Click a preset row to edit details." hint that sat below the table -- click affordance is discoverable from the row hover state and editor sheet open.
…down Replaces the 3-column ThemeCard grid (and split-preview SystemThemeCard) with a single bordered list: one Theme row with a Select dropdown, and Light/Dark rows that slide in only when System is selected. Each option shows a small Aa swatch in the picker. ThemeCard / SystemThemeCard components are no longer needed.
Drop the uppercase column header + striped rows. PresetsTable becomes a bordered list with divide-y matching Agents/Ringtones. PresetRow shows name + workspace/tab badges on top, command snippet (truncated, with +N overflow indicator) below, and a right-side meta column with applies-to + mode. Visibility eye and chevron sit at the right edge. QuickAddPresets becomes a tighter chip strip with rounded-full pills.
V2PresetsSection now wraps header, quick-add chips and the preset list into one bordered card with divide-y separators, matching the new theme section. PresetsTable gains a `bordered` prop so v1 keeps its own border. PresetRow trims the right side: chevron is gone (the whole row already clicks), applies-to + mode collapse to a single muted line, and the visibility eye fades in on row hover when the preset is visible.
Renames PresetEditorSheet -> PresetEditorDialog and switches the side sheet to a max-w-2xl modal Dialog matching 1Code's MCP/agent edit patterns. The body now uses a 2-column grid for short fields (Name + Description) and full-width sections for Commands, Applies-to, Directory, Launch mode, and a bordered Auto-run group. Drops the custom field/control className overrides, the uppercase "Advanced" subheading, the question-mark tooltips, and the LabelWithTooltip helper. Fields use a small Field helper with a hint underneath instead.
The dialog now uses both halves of its width: left column groups the recipe (Name, Description, Commands), right column groups the launch behavior (Applies to, Directory, Launch mode, Auto-run). Drops the DialogDescription (the title carries enough context), drops the bordered Auto-run group, and shortens the multi-command launch-mode labels.
Drops the cluttered two-column form. Width returns to max-w-2xl
(matching 1Code's MCP edit dialog footprint) and the body is single
column:
- Top: Name, Description, Commands as labeled fields
- Bottom: bordered settings list with rows for Applies to, Directory,
Launch mode, plus two Switch rows for the auto-run options
Launch mode now uses a single Select dropdown for both single- and
multi-command cases (replacing the radio group). Auto-run uses Switches
in their own rows instead of two checkboxes packed under one label.
The violet/amber gradient + sparkles read as cheap. Replaced with a small high-contrast monochrome pill (Linear/Notion-style) — bg-foreground text-background, uppercase, tight tracking. Single restrained signal that the tier is paid; the Upgrade button goes back to the default primary style.
RepositorySection: drop the Edit/Save/Cancel state machine for save-on-blur on a single Input — type, blur to save, Esc to revert. ProjectLocationSection: drop the redundant 'This device' label column (the section header already covers it); show path or 'Not set up' across the full row with the action button to its right. DeleteProjectSection: lift to label + hint + button so it reads consistently with other settings rows.
…tings Drawing from 1Code's project tab: add a Name section at the top with save-on-blur rename (was previously read-only via the page header). Repository row now shows a small 'Open' button with the GitHub icon when the URL parses to a github.com remote — opens the repo in the browser. Soften the destructive Delete button to outline + destructive hover so it's clearly dangerous without dominating the page; the AlertDialog still gates the actual delete.
1Code uses text-sm muted descriptions in settings; ours used text-xs which read as cramped. Bump the shared SettingsSection helper to text-sm (touches v1 too — acceptable per v1-sunset). Shorten the v2 project descriptions to label-style phrases ('Display name for this project') instead of conversational ones.
Drop the redundant 'Danger zone' heading — the Delete row already self-describes; replace with a thin border-t separator. Drop the descriptive subtitles on Name and Repository (the labels + placeholders cover it; only Project location keeps its description since state matters). Tighten section spacing space-y-8 → space-y-6.
Drop redundant section descriptions on Label and Launch (the labels + placeholders cover them). Tighten section spacing space-y-10 → space-y-6. Replace the 'Delete this agent' Section heading with a thin border-t separator + label+hint+button row, matching the v2 project Delete pattern.
Members heading text-base → text-sm to match every other settings section. Drop the redundant 'People in your organization who can use this host' description (the heading + table covers it). Surface the 'Only owners can change membership' hint up under the heading when the viewer isn't an owner, instead of as a footer below the table. HostHeader: drop the inline <code> tag for the machine ID, use a font-mono span; trim 'machine ID' to just the ID after the status separator.
Replace the section heading <Label> with an <h3 className='text-sm font-medium'> so it matches every other settings section. Bump the description to text-sm and trim the verbose copy ('Owns every PTY session and survives app restarts.' instead of two sentences explaining what a PTY is). Status row, action buttons, monochrome 'Update available' pill, and AlertDialogs all already match the vocabulary — left alone.
…ilable state Drop the manual Refresh button — refetchOnWindowFocus + the 5s poll while the list is open already cover this. Replace the 'Show sessions / Hide sessions' action button with a chevron disclosure on the status row itself: chevron + dot + count becomes one clickable element when there are sessions to show. Color-code the daemon-unavailable case with destructive (red dot + destructive text) so it reads as a problem rather than a benign 'no sessions' state.
Update daemon and Force restart move from a row below the status into the right side of the section header — same pattern as Hosts > Members + Add member button. Frees up vertical space and reads top-down: heading + actions, then status + disclosure, then expanded session table.
items-center centers each child's box vertically — but the chevron + dot inside the button have different box metrics than the plain font-mono version span next to it, so the text baselines end up offset. items-baseline aligns the actual text baselines so '1 session running', 'v0.1.4', and the 'Update available' pill all sit on the same line.
…le border Version moves from the status row up next to 'Manage daemon' as inline metadata (small font-mono, normal weight, muted) — version is a property of the daemon itself, not part of session state. Drop the rounded-md border wrapping the expanded sessions table; the table sits directly in the section without a contained box.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds migration run-time tracking and event hooks; introduces a reusable SettingsListSidebar and SettingRow; refactors many settings UIs (agents, themes, terminal presets, account, billing, models, projects), replaces preset sheets with dialogs, removes legacy theme components, and adjusts layout/styling across settings. ChangesMigration Tracking
Settings UI & Layout Refactors
Sequence DiagramssequenceDiagram
participant User
participant V2AgentsSettings
participant AgentsSettingsSidebar
participant AgentDetail
participant HostService
User->>V2AgentsSettings: Load agents page
V2AgentsSettings->>V2AgentsSettings: Fetch configs via query
V2AgentsSettings->>V2AgentsSettings: Auto-select first agent
V2AgentsSettings->>AgentsSettingsSidebar: Render sidebar with configs
User->>AgentsSettingsSidebar: Drag agent to reorder
AgentsSettingsSidebar->>V2AgentsSettings: Call onReorder(newIds)
V2AgentsSettings->>HostService: Reorder configs mutation
User->>AgentsSettingsSidebar: Click add agent
AgentsSettingsSidebar->>V2AgentsSettings: Call onAddAgent(presetId)
V2AgentsSettings->>HostService: Create new config
V2AgentsSettings->>V2AgentsSettings: Select newly added agent
User->>AgentsSettingsSidebar: Click agent to select
AgentsSettingsSidebar->>V2AgentsSettings: Call onSelectAgent(id)
V2AgentsSettings->>AgentDetail: Render detail for selected agent
User->>AgentDetail: Edit label and blur
AgentDetail->>HostService: Patch agent config
AgentDetail->>User: Toast success/error
sequenceDiagram
participant User
participant PresetEditorDialog
participant ElectronTRPC
participant ProjectTargetingField
User->>PresetEditorDialog: Open edit dialog
PresetEditorDialog->>ElectronTRPC: Fetch origin workspace
ElectronTRPC-->>PresetEditorDialog: Workspace metadata
User->>PresetEditorDialog: Change directory
PresetEditorDialog->>ElectronTRPC: selectDirectory()
ElectronTRPC-->>PresetEditorDialog: Selected path
PresetEditorDialog->>ElectronTRPC: getDirectoryStatus()
ElectronTRPC-->>PresetEditorDialog: Is valid directory
User->>ProjectTargetingField: Switch to specific projects
ProjectTargetingField->>ProjectTargetingField: Show project popover
User->>ProjectTargetingField: Select/deselect projects
ProjectTargetingField->>PresetEditorDialog: onChange(projectIds)
User->>PresetEditorDialog: Save preset
PresetEditorDialog->>ElectronTRPC: Update preset config
ElectronTRPC-->>PresetEditorDialog: Success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Greptile SummaryThis PR is a broad visual-vocabulary sweep across all settings pages — standardising padding, typography, layout, and copy with no intended behavioral changes. The main additions are: a new
Confidence Score: 3/5Mostly safe style refactor, but the nested-button HTML violation in the new agent sidebar will cause broken drag + double-fire click behaviour and should be fixed before merging. One clear P1 (nested button HTML violation producing broken UI), plus two P2 issues. The P1 is isolated to the new V2 agent sidebar but directly breaks the intended drag-to-reorder UX for agents. AgentsSettingsSidebar.tsx (P1 nested button), RepositorySection.tsx (P2 useEffect guard), V2SessionsSection.tsx (P2 missing row dividers)
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx | New file: sidebar + drag-reorder for V2 agents. Contains a P1 HTML violation — <button> nested inside <button> for the drag handle, which browsers reparent, breaking both drag and click-to-select. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx | Refactored to save-on-blur; removed the isEditing guard in the useEffect, meaning a background prop update can silently wipe mid-flight user input. |
| apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx | Daemon section redesigned: Update/Force-restart buttons hoisted to header, chevron-based session disclosure replaces Show/Hide button. Missing divide-y on new bare <tbody> leaves session rows without visual separators. |
| apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx | New file: detail panel for a selected V2 agent with save-on-blur for label, command, and prompt-args fields, inline transport toggle, and delete action. Logic looks correct. |
| apps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsListSidebar/SettingsListSidebar.tsx | New generic sidebar component with filter input, multi-group support, and a toolbar slot. Clean generic design with hideFilterWhenEmpty guard. |
| apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx | Flattened Table to divide-y list; removed amber warning box in favour of a concise copy sentence; key-visibility warning still communicated via dialog description. Clean refactor. |
| apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/BillingOverview.tsx | Hero cards replaced with borderless divide-y rows under a Plan section heading; layout and spacing standardised. Straightforward style refactor. |
| apps/desktop/src/renderer/routes/_authenticated/settings/integrations/components/IntegrationsSettings/IntegrationsSettings.tsx | Card-based layout replaced with flat IntegrationRow components. comingSoon prop and its guard removed cleanly. No behavioral changes. |
| apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx | New component: save-on-blur project rename input with Escape-to-revert. Logic is correct; Enter triggers blur to commit, Escape restores the previous name. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[V2AgentsSettings] --> B[AgentsSettingsSidebar]
A --> C[AgentDetail]
B --> D[SettingsListSidebar generic]
B --> E[AgentSidebarRow - DnD sortable]
E -->|"⚠️ nested button"| F[outer button - onSelect]
F --> G[inner button - drag handle]
C --> H[save-on-blur: label / command / promptArgs]
C --> I[inline transport toggle: argv / stdin]
C --> J[Delete agent - destructive row]
K[V2ProjectSettings] --> L[NameSection - save-on-blur rename NEW]
K --> M[RepositorySection - save-on-blur]
M -->|"⚠️ no isEditing guard"| N[useEffect resets on any prop change]
M --> O[parseGitHubRemote - Open button]
P[V2SessionsSection] --> Q[chevron disclosure replaces Show/Hide]
P --> R[Update/ForceRestart hoisted to section header]
P -->|"⚠️ missing divide-y"| S[bare tbody - no row separators]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx:198-222
**Nested `<button>` inside `<button>` — invalid HTML**
The drag-handle trigger is rendered as a `<button>` child of the outer `<button>` (select row). The HTML spec forbids interactive content (including buttons) inside `<button>` elements. Browsers auto-correct this by hoisting the inner button outside the outer one, so the grip icon ends up mis-positioned in the DOM and `e.stopPropagation()` no longer intercepts the parent click, causing every drag-handle click to also fire `onSelect`.
Replace the inner `<button>` with a `<span>` (styled to look interactive) and apply the drag listeners to it — `@dnd-kit` supports any element as the activator node, not just buttons.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx:22-24
**`useEffect` resets in-progress input on any prop change**
The old code guarded `setValue` with `if (!isEditing)`. The new `useEffect` fires unconditionally on every `currentRepoCloneUrl` change (including optimistic updates triggered by other settings panels or a background sync). If `currentRepoCloneUrl` updates while the user is actively typing a URL, their partially-entered text will be silently wiped.
Or alternatively, keep an `isEditing` ref set on focus/blur and gate the reset behind it, matching the prior behaviour.
```suggestion
const isFocusedRef = useRef(false);
useEffect(() => {
if (!isFocusedRef.current) {
setValue(currentRepoCloneUrl ?? "");
}
}, [currentRepoCloneUrl]);
```
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx:268
**Session rows lose all visual separation**
The old `<tbody>` had `className="divide-y divide-border/60"`, giving each session row a bottom border. The border container card was also removed. The new plain `<tbody>` has neither, so all session rows will run together — the `hover:bg-muted/30` row highlight is the only visual cue, but that only appears on hover, leaving the table unreadable at rest.
```suggestion
<tbody className="divide-y divide-border/40">
```
Reviews (1): Last reviewed commit: "other styles" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+198
to
+222
| type="button" | ||
| onClick={onSelect} | ||
| className={settingsListItemClass(isActive, "gap-2 w-full text-left")} | ||
| > | ||
| {icon ? ( | ||
| <img | ||
| src={icon} | ||
| alt="" | ||
| className="size-4 object-contain shrink-0" | ||
| /> | ||
| ) : null} | ||
| <span className="truncate flex-1">{row.label}</span> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| ref={setActivatorNodeRef} | ||
| {...attributes} | ||
| {...listeners} | ||
| className={cn( | ||
| "absolute right-1 top-1/2 -translate-y-1/2 p-0.5 rounded text-muted-foreground/60 hover:text-foreground hover:bg-accent cursor-grab active:cursor-grabbing", | ||
| "opacity-0 group-hover/row:opacity-100 focus-visible:opacity-100 transition-opacity", | ||
| isDragging && "opacity-100", | ||
| )} | ||
| aria-label="Drag to reorder" | ||
| > |
Contributor
There was a problem hiding this comment.
<button> inside <button> — invalid HTML
The drag-handle trigger is rendered as a <button> child of the outer <button> (select row). The HTML spec forbids interactive content (including buttons) inside <button> elements. Browsers auto-correct this by hoisting the inner button outside the outer one, so the grip icon ends up mis-positioned in the DOM and e.stopPropagation() no longer intercepts the parent click, causing every drag-handle click to also fire onSelect.
Replace the inner <button> with a <span> (styled to look interactive) and apply the drag listeners to it — @dnd-kit supports any element as the activator node, not just buttons.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx
Line: 198-222
Comment:
**Nested `<button>` inside `<button>` — invalid HTML**
The drag-handle trigger is rendered as a `<button>` child of the outer `<button>` (select row). The HTML spec forbids interactive content (including buttons) inside `<button>` elements. Browsers auto-correct this by hoisting the inner button outside the outer one, so the grip icon ends up mis-positioned in the DOM and `e.stopPropagation()` no longer intercepts the parent click, causing every drag-handle click to also fire `onSelect`.
Replace the inner `<button>` with a `<span>` (styled to look interactive) and apply the drag listeners to it — `@dnd-kit` supports any element as the activator node, not just buttons.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
22
to
+24
| useEffect(() => { | ||
| if (!isEditing) setValue(currentRepoCloneUrl ?? ""); | ||
| }, [currentRepoCloneUrl, isEditing]); | ||
|
|
||
| const startEdit = () => { | ||
| setIsEditing(true); | ||
| setTimeout(() => inputRef.current?.focus(), 0); | ||
| }; | ||
|
|
||
| const cancelEdit = () => { | ||
| setValue(currentRepoCloneUrl ?? ""); | ||
| setIsEditing(false); | ||
| }; | ||
| }, [currentRepoCloneUrl]); |
Contributor
There was a problem hiding this comment.
useEffect resets in-progress input on any prop change
The old code guarded setValue with if (!isEditing). The new useEffect fires unconditionally on every currentRepoCloneUrl change (including optimistic updates triggered by other settings panels or a background sync). If currentRepoCloneUrl updates while the user is actively typing a URL, their partially-entered text will be silently wiped.
Or alternatively, keep an isEditing ref set on focus/blur and gate the reset behind it, matching the prior behaviour.
Suggested change
| useEffect(() => { | |
| if (!isEditing) setValue(currentRepoCloneUrl ?? ""); | |
| }, [currentRepoCloneUrl, isEditing]); | |
| const startEdit = () => { | |
| setIsEditing(true); | |
| setTimeout(() => inputRef.current?.focus(), 0); | |
| }; | |
| const cancelEdit = () => { | |
| setValue(currentRepoCloneUrl ?? ""); | |
| setIsEditing(false); | |
| }; | |
| }, [currentRepoCloneUrl]); | |
| const isFocusedRef = useRef(false); | |
| useEffect(() => { | |
| if (!isFocusedRef.current) { | |
| setValue(currentRepoCloneUrl ?? ""); | |
| } | |
| }, [currentRepoCloneUrl]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx
Line: 22-24
Comment:
**`useEffect` resets in-progress input on any prop change**
The old code guarded `setValue` with `if (!isEditing)`. The new `useEffect` fires unconditionally on every `currentRepoCloneUrl` change (including optimistic updates triggered by other settings panels or a background sync). If `currentRepoCloneUrl` updates while the user is actively typing a URL, their partially-entered text will be silently wiped.
Or alternatively, keep an `isEditing` ref set on focus/blur and gate the reset behind it, matching the prior behaviour.
```suggestion
const isFocusedRef = useRef(false);
useEffect(() => {
if (!isFocusedRef.current) {
setValue(currentRepoCloneUrl ?? "");
}
}, [currentRepoCloneUrl]);
```
How can I resolve this? If you propose a fix, please make it concise.| <th className="px-2 py-2 text-left font-medium">Status</th> | ||
| </tr> | ||
| </thead> | ||
| <tbody> |
Contributor
There was a problem hiding this comment.
The old <tbody> had className="divide-y divide-border/60", giving each session row a bottom border. The border container card was also removed. The new plain <tbody> has neither, so all session rows will run together — the hover:bg-muted/30 row highlight is the only visual cue, but that only appears on hover, leaving the table unreadable at rest.
Suggested change
| <tbody> | |
| <tbody className="divide-y divide-border/40"> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx
Line: 268
Comment:
**Session rows lose all visual separation**
The old `<tbody>` had `className="divide-y divide-border/60"`, giving each session row a bottom border. The border container card was also removed. The new plain `<tbody>` has neither, so all session rows will run together — the `hover:bg-muted/30` row highlight is the only visual cue, but that only appears on hover, leaving the table unreadable at rest.
```suggestion
<tbody className="divide-y divide-border/40">
```
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
12 issues found across 68 files
You’re at about 97% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx:198">
P2: Avoid nesting a drag-handle `<button>` inside the row `<button>`; nested interactive elements are invalid HTML and can break focus/click behavior.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx:229">
P2: The session status dot is always green in the expandable state, even when there are zero alive sessions.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx:24">
P3: When trimmed input equals the current name, reset the field value so the UI stays in sync with persisted data.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx:38">
P2: Escape can still trigger a rename because blur commits before the state reset is guaranteed to be applied.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx:192">
P2: The revoke button is hover-only (`opacity-0 group-hover:opacity-100`), making it effectively invisible for keyboard/no-hover users. Add focus-based visibility so the action is discoverable and usable without mouse hover.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx:50">
P2: Pressing Escape can still save the edited repository URL because blur is triggered before the state reset is applied.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx:50">
P2: Guard this state sync while the repository field is focused; otherwise external updates to `currentRepoCloneUrl` can overwrite in-progress user edits.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx:54">
P1: The new v2-flag conditional hides all v1 projects in the sidebar, causing an empty/incorrect project list for users who still have v1 projects.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx:130">
P2: The auto-select effect can overwrite the newly added agent selection with the first existing agent before refetch completes.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx:180">
P3: The detail panel shows the empty-state message while data is still loading, which misrepresents loading as “no agents”.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx:335">
P2: Clicking Reset from a focused advanced field can trigger two competing saves (blur-save + reset-save), which can persist stale Anthropic override values due to request race timing.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/PresetEditorDialog.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/PresetEditorDialog.tsx:204">
P2: Wrap the async directory-picker mutation in `try/catch` so failures don’t become unhandled promise rejections.
(Based on your team's feedback about handling async rejections explicitly.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| /> | ||
| <span | ||
| aria-hidden | ||
| className="size-1.5 rounded-full bg-emerald-500" |
Contributor
There was a problem hiding this comment.
P2: The session status dot is always green in the expandable state, even when there are zero alive sessions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx, line 229:
<comment>The session status dot is always green in the expandable state, even when there are zero alive sessions.</comment>
<file context>
@@ -166,99 +167,105 @@ function V2SessionsSectionInner() {
+ />
+ <span
+ aria-hidden
+ className="size-1.5 rounded-full bg-emerald-500"
+ />
+ {sessionCountLabel}
</file context>
Suggested change
| className="size-1.5 rounded-full bg-emerald-500" | |
| className={cn( | |
| "size-1.5 rounded-full", | |
| aliveCount && aliveCount > 0 ? "bg-emerald-500" : "bg-muted-foreground/60", | |
| )} |
| e.preventDefault(); | ||
| (e.target as HTMLInputElement).blur(); | ||
| } | ||
| if (e.key === "Escape") { |
Contributor
There was a problem hiding this comment.
P2: Escape can still trigger a rename because blur commits before the state reset is guaranteed to be applied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx, line 38:
<comment>Escape can still trigger a rename because blur commits before the state reset is guaranteed to be applied.</comment>
<file context>
@@ -0,0 +1,47 @@
+ e.preventDefault();
+ (e.target as HTMLInputElement).blur();
+ }
+ if (e.key === "Escape") {
+ e.preventDefault();
+ setValue(currentName);
</file context>
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-8 w-8 text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100 transition-opacity" |
Contributor
There was a problem hiding this comment.
P2: The revoke button is hover-only (opacity-0 group-hover:opacity-100), making it effectively invisible for keyboard/no-hover users. Add focus-based visibility so the action is discoverable and usable without mouse hover.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx, line 192:
<comment>The revoke button is hover-only (`opacity-0 group-hover:opacity-100`), making it effectively invisible for keyboard/no-hover users. Add focus-based visibility so the action is discoverable and usable without mouse hover.</comment>
<file context>
@@ -138,131 +130,100 @@ export function ApiKeysSettings({ visibleItems }: ApiKeysSettingsProps) {
+ <Button
+ variant="ghost"
+ size="icon"
+ className="h-8 w-8 text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100 transition-opacity"
+ onClick={() => handleRevokeKey(key.id, key.name)}
+ aria-label="Revoke key"
</file context>
Suggested change
| className="h-8 w-8 text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100 transition-opacity" | |
| className="h-8 w-8 text-muted-foreground hover:text-destructive opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus-visible:opacity-100 transition-opacity" |
| {advancedOpen && hasAdvancedContent ? ( | ||
| <button | ||
| type="button" | ||
| onClick={resetAnthropicAdvanced} |
Contributor
There was a problem hiding this comment.
P2: Clicking Reset from a focused advanced field can trigger two competing saves (blur-save + reset-save), which can persist stale Anthropic override values due to request race timing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx, line 335:
<comment>Clicking Reset from a focused advanced field can trigger two competing saves (blur-save + reset-save), which can persist stale Anthropic override values due to request race timing.</comment>
<file context>
@@ -247,51 +267,154 @@ export function ModelsSettings({ visibleItems }: ModelsSettingsProps) {
+ {advancedOpen && hasAdvancedContent ? (
+ <button
+ type="button"
+ onClick={resetAnthropicAdvanced}
+ disabled={isSavingAnthropicConfig}
+ className="text-xs text-muted-foreground hover:text-destructive transition-colors disabled:opacity-50"
</file context>
Suggested change
| onClick={resetAnthropicAdvanced} | |
| onMouseDown={(event) => { | |
| event.preventDefault(); | |
| }} | |
| onClick={resetAnthropicAdvanced} |
Comment on lines
+204
to
+215
| const result = await selectDirectory.mutateAsync({ | ||
| title: "Select preset directory", | ||
| defaultPath: browseDefaultPath, | ||
| }); | ||
| if (!result.canceled && result.path) { | ||
| if (originWorkspace?.worktreePath) { | ||
| onDirectorySelect( | ||
| toPresetDirectoryValue(originWorkspace.worktreePath, result.path), | ||
| ); | ||
| return; | ||
| } | ||
| onDirectorySelect(result.path); |
Contributor
There was a problem hiding this comment.
P2: Wrap the async directory-picker mutation in try/catch so failures don’t become unhandled promise rejections.
(Based on your team's feedback about handling async rejections explicitly.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/PresetEditorDialog.tsx, line 204:
<comment>Wrap the async directory-picker mutation in `try/catch` so failures don’t become unhandled promise rejections.
(Based on your team's feedback about handling async rejections explicitly.) </comment>
<file context>
@@ -0,0 +1,444 @@
+ );
+
+ const handleBrowseDirectory = async () => {
+ const result = await selectDirectory.mutateAsync({
+ title: "Select preset directory",
+ defaultPath: browseDefaultPath,
</file context>
Suggested change
| const result = await selectDirectory.mutateAsync({ | |
| title: "Select preset directory", | |
| defaultPath: browseDefaultPath, | |
| }); | |
| if (!result.canceled && result.path) { | |
| if (originWorkspace?.worktreePath) { | |
| onDirectorySelect( | |
| toPresetDirectoryValue(originWorkspace.worktreePath, result.path), | |
| ); | |
| return; | |
| } | |
| onDirectorySelect(result.path); | |
| try { | |
| const result = await selectDirectory.mutateAsync({ | |
| title: "Select preset directory", | |
| defaultPath: browseDefaultPath, | |
| }); | |
| if (!result.canceled && result.path) { | |
| if (originWorkspace?.worktreePath) { | |
| onDirectorySelect( | |
| toPresetDirectoryValue(originWorkspace.worktreePath, result.path), | |
| ); | |
| return; | |
| } | |
| onDirectorySelect(result.path); | |
| } | |
| } catch (error) { | |
| console.warn("Failed to select preset directory", error); | |
| } |
| setValue(currentName); | ||
| return; | ||
| } | ||
| if (trimmed === currentName) return; |
Contributor
There was a problem hiding this comment.
P3: When trimmed input equals the current name, reset the field value so the UI stays in sync with persisted data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx, line 24:
<comment>When trimmed input equals the current name, reset the field value so the UI stays in sync with persisted data.</comment>
<file context>
@@ -0,0 +1,47 @@
+ setValue(currentName);
+ return;
+ }
+ if (trimmed === currentName) return;
+ projectActions.renameProject(projectId, trimmed);
+ };
</file context>
| }} | ||
| /> | ||
| ) : ( | ||
| <EmptyState /> |
Contributor
There was a problem hiding this comment.
P3: The detail panel shows the empty-state message while data is still loading, which misrepresents loading as “no agents”.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx, line 180:
<comment>The detail panel shows the empty-state message while data is still loading, which misrepresents loading as “no agents”.</comment>
<file context>
@@ -124,140 +101,113 @@ export function V2AgentsSettings() {
+ }}
+ />
+ ) : (
+ <EmptyState />
+ )}
+ </div>
</file context>
Suggested change
| <EmptyState /> | |
| configsQuery.isLoading ? <div className="p-6 text-sm text-muted-foreground">Loading agent settings...</div> : <EmptyState /> |
Resolve conflict in LinkTierMapper.tsx by adopting origin/main's clickPolicy types (Surface prop, modifierLabel/actionLabel helpers, shift tier) while keeping the v1 borderless layout and size="sm" select trigger. Address Greptile review: - AgentsSettingsSidebar: replace nested <button> drag handle with a <span role="button"> so click events stop on the inner element - RepositorySection: gate the prop-sync useEffect on a focus ref so background updates don't wipe in-progress user input - V2SessionsSection: restore divide-y on the daemon sessions tbody
Did not work in practice. Restore the simple Account redirect.
![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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx (1)
221-236:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
returnafter the catch block leaves a dangling timer on preview failure.When
ringtone.preview.mutaterejects,setPlayingId(null)is called correctly, but execution falls through to thesetTimeoutsetup on line 233. This plants a phantom timer (up to ~5.5 s) that writes topreviewTimerRef.current, which can mask a subsequent play attempt's timer or confuse the cleanup path — even though the timer callback itself ends up being a no-op.🐛 Proposed fix
} catch (error) { console.error("Failed to play ringtone:", error); setPlayingId(null); + return; } // Auto-reset after the ringtone's actual duration (with 500ms buffer)🤖 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/ringtones/components/RingtonesSettings/RingtonesSettings.tsx` around lines 221 - 236, The catch in the ringtone preview flow currently logs the error and calls setPlayingId(null) but does not return, so execution continues and schedules a preview timer; modify the preview logic (around electronTrpcClient.ringtone.preview.mutate, the catch block, and the subsequent durationMs/setTimeout) to return early from the function after handling the error (i.e., after console.error and setPlayingId(null)) so that previewTimerRef.current is not set on preview failure; ensure previewTimerRef is only assigned when the preview mutate succeeds and the timer cleanup behavior remains unchanged.apps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/GitSettings.tsx (1)
184-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProgrammatically label the branch-prefix controls.
This row still only has a visual label. A screen reader will announce the select by its current value, and the custom text field falls back to its placeholder. Please wire an accessible name onto both controls.
♿ Suggested fix
- <Label className="text-sm font-medium">Branch prefix</Label> + <Label htmlFor="branch-prefix-mode" className="text-sm font-medium"> + Branch prefix + </Label> ... - <SelectTrigger className="w-[180px]"> + <SelectTrigger + id="branch-prefix-mode" + className="w-[180px]" + aria-label="Branch prefix mode" + > ... <Input placeholder="Prefix" + aria-label="Custom branch prefix" value={customPrefixInput}🤖 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/git/components/GitSettings/GitSettings.tsx` around lines 184 - 228, The branch-prefix controls lack accessible names; add one by giving the existing Label element a stable id (e.g., branchPrefixLabel) and wire that id into the form controls so screen readers announce the visual label: set aria-labelledby={labelId} on the Select (or its trigger) and on the Input used for customPrefixInput; ensure the same id is used for both when visible, and if you prefer a standalone name instead, set descriptive aria-label values on Select and Input (e.g., "Branch prefix mode" and "Custom branch prefix") and keep controls' disabled/pending logic unchanged (targets: Select, SelectTrigger/SelectValue, and Input and handlers handleBranchPrefixModeChange and handleCustomPrefixBlur).apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx (3)
229-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancelling the "Generate API key" dialog leaves
newKeyNamestale.
onOpenChangeonly callssetShowGenerateDialog— it does not resetnewKeyName. If a user types a name, cancels, then reopens the dialog, the previous name is pre-filled.🛠️ Proposed fix
- <Button - variant="outline" - onClick={() => setShowGenerateDialog(false)} - > + <Button + variant="outline" + onClick={() => { + setShowGenerateDialog(false); + setNewKeyName(""); + }} + >Or alternatively, reset inside the
onOpenChangehandler on theDialog:-<Dialog open={showGenerateDialog} onOpenChange={setShowGenerateDialog}> +<Dialog open={showGenerateDialog} onOpenChange={(open) => { + setShowGenerateDialog(open); + if (!open) setNewKeyName(""); +}}>🤖 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/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx` around lines 229 - 233, The cancel flow for the "Generate API key" dialog doesn't clear the newKeyName state, so previously typed values persist; update the dialog close logic to reset newKeyName whenever the dialog is closed: call setNewKeyName("") alongside setShowGenerateDialog(false) (e.g., in the Button onClick handler that currently calls setShowGenerateDialog(false) and/or in the Dialog onOpenChange handler) so that newKeyName is cleared when the dialog is cancelled or closed.
60-79:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
handleGenerateKeyswallows errors silently — add user-facing feedback.The
catchblock only logs to the console. WhenapiTrpcClient.apiKey.create.mutatefails, the dialog remains open with no indication to the user about what went wrong.🛠️ Proposed fix
} catch (error) { console.error("[api-keys] Failed to generate API key:", error); + toast.error("Failed to generate API key. Please try again."); } finally {🤖 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/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx` around lines 60 - 79, handleGenerateKey currently only logs failures to console so the user sees no feedback and the dialog stays open; update it to surface errors to the UI by catching the error and setting a user-visible error state (e.g., setGenerateError / setApiKeyGenerateError) or invoking the app's toast/notification utility inside the catch block, ensure setIsGenerating(false) still runs in finally, and keep or update dialog visibility accordingly (keep showGenerateDialog open so the user can correct input and display the error text in the ApiKeysSettings generate dialog). Reference: function handleGenerateKey and state setters setIsGenerating, setShowGenerateDialog, setShowNewKeyDialog, setNewKeyValue, setNewKeyName when making the change.
81-97:⚠️ Potential issue | 🟠 MajorAdd error handling to the revoke handler to prevent silent failures.
The
onClickhandler at lines 90–92 lacks a try-catch block. IfauthClient.apiKey.deleterejects, the error is silently swallowed andtoast.successis never called, leaving the user unaware that revocation failed. This contradicts the pattern used inhandleGenerateKey(lines 74–76), which properly catches errors and notifies the user.🛠️ Proposed fix
onClick: async () => { - await authClient.apiKey.delete({ keyId: id }); - toast.success("API key revoked"); + try { + await authClient.apiKey.delete({ keyId: id }); + toast.success("API key revoked"); + } catch (error) { + console.error("[api-keys] Failed to revoke API key:", error); + toast.error("Failed to revoke API key. Please try again."); + } },🤖 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/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx` around lines 81 - 97, The revoke handler handleRevokeKey currently calls authClient.apiKey.delete inside the alert action without error handling; wrap the async onClick body in a try/catch so that if authClient.apiKey.delete({ keyId: id }) rejects you show toast.error with the error message (and avoid showing toast.success), and only call toast.success when the delete completes successfully; update the onClick in the alert actions to use this try/catch pattern (referencing handleRevokeKey and authClient.apiKey.delete) so failures are surfaced to the user.
🧹 Nitpick comments (6)
apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/components/SettingsSection/SettingsSection.tsx (1)
27-29: ⚡ Quick win
text-xshere diverges from thetext-smdescription size used in the sharedSettingsSection(ProjectSettings.tsx line 74).The PR's goal is a unified visual vocabulary, and the shared helper's description was bumped to
text-sm. This component went in the opposite direction (text-sm→text-xs), which will produce visibly smaller description text on the Models page compared to all other settings pages. If the discrepancy is intentional for a denser layout, a brief comment would help; otherwise this should align with the rest.🎨 Proposed fix to align with the shared vocabulary
- <p className="text-xs text-muted-foreground mt-0.5"> + <p className="text-sm text-muted-foreground mt-0.5">🤖 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/models/components/ModelsSettings/components/SettingsSection/SettingsSection.tsx` around lines 27 - 29, In SettingsSection (component SettingsSection.tsx) the description paragraph uses the class "text-xs" which diverges from the shared SettingsSection/ProjectSettings description size ("text-sm"); update the class on the <p> that renders {description} from "text-xs" to "text-sm" to match the shared visual vocabulary (or, if the smaller size was intentional, add a brief inline comment above the <p> explaining why this component deviates). Ensure you modify the class string on the paragraph that contains {description} in SettingsSection so it matches the other settings pages.apps/desktop/src/renderer/routes/_authenticated/settings/integrations/components/IntegrationsSettings/IntegrationsSettings.tsx (1)
129-163: 💤 Low valueConsider
divide-yoverspace-y-1for visual row separationThe PR description explicitly calls out "flat divide-y lists" for the integrations page, but the container uses
space-y-1(a 4px gap) with no border separator between rows. Thepy-3padding on eachIntegrationRowmeans rows sit very close together visually without a divider.♻️ Proposed change
- <div className="space-y-1"> + <div className="divide-y">🤖 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/integrations/components/IntegrationsSettings/IntegrationsSettings.tsx` around lines 129 - 163, Replace the vertical spacing utility on the integrations container to use a flat divider instead of gaps: in the parent div that currently has className="space-y-1", remove "space-y-1" and add "divide-y divide-neutral-200" (or your design-system's divider color) so IntegrationRow entries are visually separated by a horizontal rule while preserving each row's existing padding (e.g., py-3) and props (IntegrationRow, isConnected, connectedOrgName, isLoading, onManage).apps/desktop/src/renderer/routes/_authenticated/settings/account/components/AccountSettings/AccountSettings.tsx (1)
177-195: ⚡ Quick win
SettingRowis nearly identical toSettingsRowinOrganizationSettings.tsx— extract to a shared component.Both files define a local row helper with the same flex layout,
flex-1 min-w-0label column, optional hint, and shrink-0 children slot. The only meaningful differences arepy-2.5on the wrapper and the use of<Label htmlFor>(inOrganizationSettings). Given that this PR's goal is to standardise the settings vocabulary across every page, shipping two private copies with different names (SettingRow/SettingsRow) and slightly diverging internals works against that goal.A single shared component (e.g.,
SettingsSection/SettingRow) would give all settings pages a consistent row primitive and prevent further drift.🤖 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/account/components/AccountSettings/AccountSettings.tsx` around lines 177 - 195, SettingRow in AccountSettings (function SettingRow) is a duplicate of SettingsRow used in OrganizationSettings.tsx; extract a single shared SettingsRow/SettingRow component in a common UI folder and replace both local definitions with an import. Move the shared JSX (wrapper div with flex layout, inner label block using "flex-1 min-w-0", optional hint rendering, and children slot in "flex-shrink-0") into the new component (preserve props: label, hint?, children) and update callers to use the shared component (adjust any wrapper spacing like py-2.5 via a prop or wrapper class at call sites if needed) so both AccountSettings and OrganizationSettings import the same component instead of defining SettingRow/SettingsRow locally.apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/BillingDetails/BillingDetails.tsx (1)
150-168: ⚡ Quick win
DetailRowis reimplemented inline inCurrentPlanCardandUpgradeCard— extract to a shared component.All three billing components implement the same row skeleton:
flex items-center justify-between gap-8 py-3 └─ min-w-0 flex-1 (label + hint) └─ shrink-0 (action)Because
DetailRowis unexported,CurrentPlanCard.tsxandUpgradeCard.tsxduplicate the layout inline. ExportingDetailRow(or promoting it to a sharedBillingRow/SettingRowcomponent) would let all three billing rows share the same layout contract and be updated in one place.🤖 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/billing/components/BillingOverview/components/BillingDetails/BillingDetails.tsx` around lines 150 - 168, The DetailRow layout is duplicated in CurrentPlanCard and UpgradeCard; extract and export the existing DetailRow (and its DetailRowProps) as a shared component (e.g., BillingRow or keep the name DetailRow) and replace the inline implementations in CurrentPlanCard and UpgradeCard with imports of that shared component so all three billing components (BillingDetails/DetailRow, CurrentPlanCard, UpgradeCard) use the same exported component and prop contract.apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx (1)
199-229: ⚡ Quick winPrompt transport toggle is missing ARIA group semantics.
The segmented
argv / stdincontrol has exclusive-selection semantics (a radio group), but screen readers see two plain buttons with no stated relationship or selection state.♿ Proposed fix
-<div className="inline-flex rounded-md border border-border overflow-hidden"> +<div + role="radiogroup" + aria-label="Prompt transport" + className="inline-flex rounded-md border border-border overflow-hidden" +> <button type="button" onClick={() => handleTransportChange("argv")} + role="radio" + aria-checked={promptTransport === "argv"} className={cn( "px-3 py-1 text-xs font-medium transition-colors", promptTransport === "argv" ? "bg-accent text-accent-foreground" : "bg-transparent text-muted-foreground hover:bg-accent/50", )} > argv </button> <button type="button" onClick={() => handleTransportChange("stdin")} + role="radio" + aria-checked={promptTransport === "stdin"} className={cn( "px-3 py-1 text-xs font-medium transition-colors border-l border-border", promptTransport === "stdin" ? "bg-accent text-accent-foreground" : "bg-transparent text-muted-foreground hover:bg-accent/50", )} > stdin </button> </div>🤖 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/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx` around lines 199 - 229, Wrap the two buttons in a radiogroup and give each button radio semantics so assistive tech sees them as an exclusive-selection control: add role="radiogroup" to the div and for each option set role="radio", aria-checked={promptTransport === "argv" || "stdin"}, and tabIndex={promptTransport === value ? 0 : -1}; keep the existing onClick pointing to handleTransportChange and add an onKeyDown handler on each radio to handle ArrowLeft/ArrowRight (calling handleTransportChange and moving focus) so keyboard arrow navigation works; preserve the current cn classes and visual styling while adding these ARIA attributes on the StackedField children.apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx (1)
11-15: ⚡ Quick winUse the
rendereralias for this shared sidebar import.This new relative path is brittle and goes against the desktop import convention already used in the same file.
♻️ Suggested change
import { type SettingsListGroup, settingsListItemClass, SettingsListSidebar, -} from "../../../components/SettingsListSidebar"; +} from "renderer/routes/_authenticated/settings/components/SettingsListSidebar";As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible.🤖 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/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx` around lines 11 - 15, The import for SettingsListSidebar, settingsListItemClass, and SettingsListGroup uses a brittle relative path; update the import to use the renderer alias defined in tsconfig (e.g. import { type SettingsListGroup, settingsListItemClass, SettingsListSidebar } from "renderer/…") so the file ProjectsSettingsSidebar.tsx follows the apps/desktop alias convention and avoids fragile relative paths.
🤖 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/routes/_authenticated/settings/account/components/AccountSettings/AccountSettings.tsx`:
- Around line 143-149: The Email input lacks a programmatic label association;
update SettingRow to render a proper <Label htmlFor={htmlFor}> (mirroring
OrganizationSettings' SettingsRow) and ensure each Input in AccountSettings
(e.g., the Email and Name inputs inside AccountSettings.tsx) receives a matching
id prop (unique per field) and the SettingRow call sites pass that same htmlFor
value; also remove reliance on placeholder for labeling and prefer the explicit
label/id pairing so assistive tech can announce the controls.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/account/components/AccountSettings/components/ProfileSkeleton/ProfileSkeleton.tsx`:
- Around line 11-14: ProfileSkeleton renders a phantom rectangular placeholder
that doesn't exist in the real Avatar SettingRow; in the ProfileSkeleton
component remove the extra Skeleton with className "h-8 w-20" (the rectangular
placeholder) so only the circular Skeleton remains and the markup matches
AccountSettings' Avatar row; reference the ProfileSkeleton component and ensure
the remaining Skeleton keeps its "size-12 rounded-full" classes and alignment
consistent with SettingRow in AccountSettings.tsx.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx`:
- Around line 97-101: handleLabelBlur currently checks trimmed length but calls
updateMutation.mutate with the raw label, which can persist leading/trailing
whitespace; change handleLabelBlur to pass the trimmed string to
updateMutation.mutate (e.g., compute const trimmed = label.trim() and call
updateMutation.mutate({ label: trimmed })) and keep the existing comparison
using config.label so you only update when the effective label changed.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx`:
- Around line 216-220: The drag-handle is hidden by default (opacity-0) which
breaks touch users; update AgentsSettingsSidebar to detect touch devices (e.g.,
add an isTouchDevice state set in a useEffect using window.matchMedia('(hover:
none)') or 'ontouchstart' probe) and include that flag in the handle className
so the handle gets "opacity-100" when isTouchDevice is true (in addition to the
existing isDragging condition), e.g., add isTouchDevice && "opacity-100"
alongside isDragging to ensure the handle is visible on touch devices.
- Around line 68-74: The KeyboardSensor used in useSensors is not wired to
sortableKeyboardCoordinates, causing keyboard navigation to use default 25px
increments instead of moving between sortable items; update the
useSensor(KeyboardSensor) call inside useSensors to pass
sortableKeyboardCoordinates as the coordinate getter (i.e.,
useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates })) so
KeyboardSensor uses the sortable list coordinates; ensure you import
sortableKeyboardCoordinates where AgentsSettingsSidebar defines useSensors and
keep the existing MouseSensor and TouchSensor options intact.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsx`:
- Around line 124-131: The effect in V2AgentsSettings is rerunning when
selectedAgentId changes and thus overwrites the new “select added agent” and
“clear selection on reset” flows; restrict the effect to run only when configs
change (remove selectedAgentId from the dependency array) and keep the internal
checks as-is so selection is only updated on config list changes (useEffect(...,
[configs]) with the same body that sets selectedAgentId to null if
configs.length === 0 or to configs[0].id only when the current selectedAgentId
is not present).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx`:
- Around line 189-197: The revoke Button in ApiKeysSettings is hidden on
keyboard focus because it uses "opacity-0 group-hover:opacity-100" only; update
the Button's className to also include "focus-visible:opacity-100" (and
optionally "focus:opacity-100") so keyboard-focused users see the control, and
ensure the existing aria-label remains; locate the Button element in
ApiKeysSettings (the one calling handleRevokeKey) and add those focus classes to
its className.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsx`:
- Around line 87-121: The SelectTrigger in ThemeRow lacks an accessible name for
screen readers; update the SelectTrigger (used inside ThemeRow) to expose the
prop label as its accessible name—e.g., add an aria-label or aria-labelledby
that references the label text so the trigger announces whether it controls
"Theme", "Light theme", or "Dark theme" (use the existing label variable passed
into ThemeRow). Ensure the accessible name is applied to the SelectTrigger
element that wraps SelectValue so both system and explicit theme displays are
properly announced.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/UpgradeCard/UpgradeCard.tsx`:
- Around line 11-23: The displayed monthly price uses a raw division (monthly =
plan.price?.monthly ? plan.price.monthly / 100 : 0) which can produce truncated
decimals; update UpgradeCard to format the amount with Intl.NumberFormat like
the existing formatAmount helper (or call formatAmount if exported) instead of
interpolating ${monthly} directly so prices render as proper currency (e.g., use
plan.price.monthly through formatAmount or an equivalent Intl.NumberFormat call
where monthly is currently used in the JSX).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsx`:
- Around line 152-167: The blur-save/reset race happens because
handleAnthropicFormBlur saves the old form when an input blur fires before
reset; fix by adding a short-lived guard flag (a mutable ref like
isProgrammaticChangeRef) that resetAnthropicAdvanced sets to true before calling
setAnthropicForm and saveAnthropicForm and clears after the reset save
completes, and update handleAnthropicFormBlur to return early if that flag is
set; do the same pattern for the analogous handlers at lines 332-340 (the other
model provider reset/blur handlers) so programmatic resets don't trigger the
blur save.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/organization/components/OrganizationSettings/OrganizationSettings.tsx`:
- Around line 259-270: The logo button in OrganizationSettings is missing the
upload-in-progress guard, so update the button that calls handleLogoUpload
(rendering OrganizationLogo) to also disable while selectImageMutation.isPending
— e.g., change the disabled prop to combine the ownership check and the mutation
pending flag (disabled={!isOwner || selectImageMutation.isPending}) so it
matches the avatar/upload pattern in AccountSettings.tsx and prevents
re-entry/double toasts during the async upload flow.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/PresetEditorDialog.tsx`:
- Around line 203-217: The handleBrowseDirectory function calls
selectDirectory.mutateAsync directly and can throw, so wrap the await call in a
try/catch inside handleBrowseDirectory (the function defined near
PresetEditorDialog) to prevent unhandled promise rejections; on error,
log/report the error (use existing logger or set an error state/notification)
and ensure any UI state that disables the browse button is reset so the button
is re-enabled, while keeping the successful-path behavior that calls
onDirectorySelect (including using toPresetDirectoryValue when
originWorkspace?.worktreePath exists).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsx`:
- Around line 216-232: The expand/collapse button in V2SessionsSection lacks an
aria-expanded attribute; update the button (the element that calls
setShowSessionList and uses showSessionList) to include
aria-expanded={showSessionList} so assistive tech can read the current state,
and optionally add aria-controls pointing to the session list's id (ensure the
session list element has a matching id) to link the button to the collapsible
region.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx:
- Around line 18-42: The Escape handler currently calls setValue(currentName)
(which is batched) then blur(), causing onBlur → commit() to read the stale
draft value and accidentally rename; fix by introducing a synchronous ref flag
(e.g., isDiscardRef) used by onKeyDown Escape to mark that the change was
discarded, and update commit() to check that ref and skip rename if set,
clearing the ref afterwards; reference the commit function, value/currentName
state, the onBlur handler, and the onKeyDown Escape branch to implement this
behavior.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/`$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx:
- Around line 48-52: The Escape handler currently does
setValue(currentRepoCloneUrl ?? "") then immediately triggers blur which invokes
onBlur → commit while the state update is still pending, causing commit to see
the old draft; modify the handler and related onBlur/commit logic to use an
"escapeRef" boolean flag (or similar) on keydown: when Escape is pressed, set
escapeRef.current = true, reset the input visually via
setValue(currentRepoCloneUrl ?? ""), call blur, and in onBlur/commit check
escapeRef.current—if true, clear the flag and skip calling
updateRepository/commit; ensure the flag is reset for normal blurs so other
paths still commit changes.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsx`:
- Around line 229-233: The cancel flow for the "Generate API key" dialog doesn't
clear the newKeyName state, so previously typed values persist; update the
dialog close logic to reset newKeyName whenever the dialog is closed: call
setNewKeyName("") alongside setShowGenerateDialog(false) (e.g., in the Button
onClick handler that currently calls setShowGenerateDialog(false) and/or in the
Dialog onOpenChange handler) so that newKeyName is cleared when the dialog is
cancelled or closed.
- Around line 60-79: handleGenerateKey currently only logs failures to console
so the user sees no feedback and the dialog stays open; update it to surface
errors to the UI by catching the error and setting a user-visible error state
(e.g., setGenerateError / setApiKeyGenerateError) or invoking the app's
toast/notification utility inside the catch block, ensure setIsGenerating(false)
still runs in finally, and keep or update dialog visibility accordingly (keep
showGenerateDialog open so the user can correct input and display the error text
in the ApiKeysSettings generate dialog). Reference: function handleGenerateKey
and state setters setIsGenerating, setShowGenerateDialog, setShowNewKeyDialog,
setNewKeyValue, setNewKeyName when making the change.
- Around line 81-97: The revoke handler handleRevokeKey currently calls
authClient.apiKey.delete inside the alert action without error handling; wrap
the async onClick body in a try/catch so that if authClient.apiKey.delete({
keyId: id }) rejects you show toast.error with the error message (and avoid
showing toast.success), and only call toast.success when the delete completes
successfully; update the onClick in the alert actions to use this try/catch
pattern (referencing handleRevokeKey and authClient.apiKey.delete) so failures
are surfaced to the user.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/GitSettings.tsx`:
- Around line 184-228: The branch-prefix controls lack accessible names; add one
by giving the existing Label element a stable id (e.g., branchPrefixLabel) and
wire that id into the form controls so screen readers announce the visual label:
set aria-labelledby={labelId} on the Select (or its trigger) and on the Input
used for customPrefixInput; ensure the same id is used for both when visible,
and if you prefer a standalone name instead, set descriptive aria-label values
on Select and Input (e.g., "Branch prefix mode" and "Custom branch prefix") and
keep controls' disabled/pending logic unchanged (targets: Select,
SelectTrigger/SelectValue, and Input and handlers handleBranchPrefixModeChange
and handleCustomPrefixBlur).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsx`:
- Around line 221-236: The catch in the ringtone preview flow currently logs the
error and calls setPlayingId(null) but does not return, so execution continues
and schedules a preview timer; modify the preview logic (around
electronTrpcClient.ringtone.preview.mutate, the catch block, and the subsequent
durationMs/setTimeout) to return early from the function after handling the
error (i.e., after console.error and setPlayingId(null)) so that
previewTimerRef.current is not set on preview failure; ensure previewTimerRef is
only assigned when the preview mutate succeeds and the timer cleanup behavior
remains unchanged.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/account/components/AccountSettings/AccountSettings.tsx`:
- Around line 177-195: SettingRow in AccountSettings (function SettingRow) is a
duplicate of SettingsRow used in OrganizationSettings.tsx; extract a single
shared SettingsRow/SettingRow component in a common UI folder and replace both
local definitions with an import. Move the shared JSX (wrapper div with flex
layout, inner label block using "flex-1 min-w-0", optional hint rendering, and
children slot in "flex-shrink-0") into the new component (preserve props: label,
hint?, children) and update callers to use the shared component (adjust any
wrapper spacing like py-2.5 via a prop or wrapper class at call sites if needed)
so both AccountSettings and OrganizationSettings import the same component
instead of defining SettingRow/SettingsRow locally.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx`:
- Around line 199-229: Wrap the two buttons in a radiogroup and give each button
radio semantics so assistive tech sees them as an exclusive-selection control:
add role="radiogroup" to the div and for each option set role="radio",
aria-checked={promptTransport === "argv" || "stdin"}, and
tabIndex={promptTransport === value ? 0 : -1}; keep the existing onClick
pointing to handleTransportChange and add an onKeyDown handler on each radio to
handle ArrowLeft/ArrowRight (calling handleTransportChange and moving focus) so
keyboard arrow navigation works; preserve the current cn classes and visual
styling while adding these ARIA attributes on the StackedField children.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/BillingDetails/BillingDetails.tsx`:
- Around line 150-168: The DetailRow layout is duplicated in CurrentPlanCard and
UpgradeCard; extract and export the existing DetailRow (and its DetailRowProps)
as a shared component (e.g., BillingRow or keep the name DetailRow) and replace
the inline implementations in CurrentPlanCard and UpgradeCard with imports of
that shared component so all three billing components (BillingDetails/DetailRow,
CurrentPlanCard, UpgradeCard) use the same exported component and prop contract.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/integrations/components/IntegrationsSettings/IntegrationsSettings.tsx`:
- Around line 129-163: Replace the vertical spacing utility on the integrations
container to use a flat divider instead of gaps: in the parent div that
currently has className="space-y-1", remove "space-y-1" and add "divide-y
divide-neutral-200" (or your design-system's divider color) so IntegrationRow
entries are visually separated by a horizontal rule while preserving each row's
existing padding (e.g., py-3) and props (IntegrationRow, isConnected,
connectedOrgName, isLoading, onManage).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/components/SettingsSection/SettingsSection.tsx`:
- Around line 27-29: In SettingsSection (component SettingsSection.tsx) the
description paragraph uses the class "text-xs" which diverges from the shared
SettingsSection/ProjectSettings description size ("text-sm"); update the class
on the <p> that renders {description} from "text-xs" to "text-sm" to match the
shared visual vocabulary (or, if the smaller size was intentional, add a brief
inline comment above the <p> explaining why this component deviates). Ensure you
modify the class string on the paragraph that contains {description} in
SettingsSection so it matches the other settings pages.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx`:
- Around line 11-15: The import for SettingsListSidebar, settingsListItemClass,
and SettingsListGroup uses a brittle relative path; update the import to use the
renderer alias defined in tsconfig (e.g. import { type SettingsListGroup,
settingsListItemClass, SettingsListSidebar } from "renderer/…") so the file
ProjectsSettingsSidebar.tsx follows the apps/desktop alias convention and avoids
fragile relative paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60d69476-28d1-4f5d-9c04-5acaf7f7d8f2
📒 Files selected for processing (72)
apps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/index.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useMigrateV1DataToV2/useMigrateV1DataToV2.tsapps/desktop/src/renderer/routes/_authenticated/settings/account/components/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/account/components/AccountSettings/components/ProfileSkeleton/ProfileSkeleton.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/AgentsSettings/AgentsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/V2AgentsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentDetail/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/api-keys/components/ApiKeysSettings/ApiKeysSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/AppearanceSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/FontSettingSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/MarkdownStyleSection/MarkdownStyleSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeCard/ThemeCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeCard/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeSection/ThemeSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/BillingOverview.tsxapps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/BillingDetails/BillingDetails.tsxapps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/CurrentPlanCard/CurrentPlanCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/RecentInvoices/RecentInvoices.tsxapps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/UpgradeCard/UpgradeCard.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsListSidebar/SettingsListSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/components/SettingsListSidebar/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/experimental/components/ExperimentalSettings/ExperimentalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/git/components/GitSettings/GitSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/HostSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/components/HostHeader/HostHeader.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/components/HostsSettingsSidebar/HostsSettingsSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/integrations/components/IntegrationsSettings/IntegrationsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/keyboard/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/layout.tsxapps/desktop/src/renderer/routes/_authenticated/settings/links/components/LinkTierMapper/LinkTierMapper.tsxapps/desktop/src/renderer/routes/_authenticated/settings/members/components/MembersSettings/components/InviteMemberButton/InviteMemberButton.tsxapps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/ModelsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/components/ConfigRow/ConfigRow.tsxapps/desktop/src/renderer/routes/_authenticated/settings/models/components/ModelsSettings/components/SettingsSection/SettingsSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/organization/components/OrganizationSettings/OrganizationSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/permissions/components/PermissionsSettings/PermissionsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/cloud/secrets/components/SecretsSettings/SecretsSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/project/$projectId/components/ProjectSettings/ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/ringtones/components/RingtonesSettings/components/VolumeDropdown/VolumeDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/TerminalSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetRow/PresetRow.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/PresetsSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/LabelWithTooltip/LabelWithTooltip.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/LabelWithTooltip/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/PresetEditorDialog.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/components/ProjectTargetingField/ProjectTargetingField.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/components/ProjectTargetingField/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorDialog/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/PresetEditorSheet.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/components/ProjectTargetingField/ProjectTargetingField.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetsTable/PresetsTable.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/QuickAddPresets/QuickAddPresets.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2PresetsSection/V2PresetsSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/V2SessionsSection/V2SessionsSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/DeleteProjectSection/DeleteProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/NameSection/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/ProjectLocationSection/ProjectLocationSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx
💤 Files with no reviewable changes (13)
- apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeCard/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/LabelWithTooltip/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/LabelWithTooltip/LabelWithTooltip.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/index.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/SystemThemeCard/SystemThemeCard.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/PresetEditorSheet.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/terminal/components/TerminalSettings/components/PresetsSection/components/PresetEditorSheet/components/ProjectTargetingField/ProjectTargetingField.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/ThemeCard/ThemeCard.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx
- apps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.ts
- apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/V2AgentCard/V2AgentCard.tsx
Comment on lines
+143
to
+149
| <SettingRow label="Email"> | ||
| <Input | ||
| value={user.email} | ||
| readOnly | ||
| className="w-80 opacity-60" | ||
| /> | ||
| </SettingRow> |
Contributor
There was a problem hiding this comment.
Email input has no accessible name — WCAG 1.3.1 Level A gap.
The SettingRow helper uses a plain <div> for the label with no htmlFor prop, and the Input elements have no id to form a programmatic association. The Email field is the most critical — it has neither a placeholder nor an aria-label, so assistive technologies have nothing to announce as the control's label. The Name input is partially covered by its placeholder, but a real <label> association is still required per WCAG.
OrganizationSettings.tsx's SettingsRow already solves this correctly with <Label htmlFor={htmlFor}> + matching id on each Input. Aligning SettingRow here to the same pattern fixes both the accessibility gap and the inconsistency.
🛡️ Proposed fix
-interface SettingRowProps {
+interface SettingRowProps {
label: string;
hint?: string;
+ htmlFor?: string;
children: React.ReactNode;
}
-function SettingRow({ label, hint, children }: SettingRowProps) {
+function SettingRow({ label, hint, htmlFor, children }: SettingRowProps) {
return (
<div className="flex items-center justify-between gap-8">
<div className="flex-1 min-w-0">
- <div className="text-sm font-medium">{label}</div>
+ <Label htmlFor={htmlFor} className="text-sm font-medium">{label}</Label>
{hint && (
<div className="text-xs text-muted-foreground mt-0.5">{hint}</div>
)}
</div>
<div className="flex-shrink-0">{children}</div>
</div>
);
}Then add id/htmlFor at each call-site:
-<SettingRow label="Name">
+<SettingRow label="Name" htmlFor="account-name">
<Input
+ id="account-name"
value={nameValue}
...
/>
</SettingRow>
-<SettingRow label="Email">
+<SettingRow label="Email" htmlFor="account-email">
<Input
+ id="account-email"
value={user.email}
readOnly
...
/>
</SettingRow>🤖 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/account/components/AccountSettings/AccountSettings.tsx`
around lines 143 - 149, The Email input lacks a programmatic label association;
update SettingRow to render a proper <Label htmlFor={htmlFor}> (mirroring
OrganizationSettings' SettingsRow) and ensure each Input in AccountSettings
(e.g., the Email and Name inputs inside AccountSettings.tsx) receives a matching
id prop (unique per field) and the SettingRow call sites pass that same htmlFor
value; also remove reliance on placeholder for labeling and prefer the explicit
label/id pairing so assistive tech can announce the controls.
Comment on lines
+11
to
+14
| <div className="flex items-center gap-3"> | ||
| <Skeleton className="size-12 rounded-full" /> | ||
| <Skeleton className="h-8 w-20" /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Skeleton has a phantom rectangular placeholder that doesn't correspond to the rendered Avatar row.
The actual Avatar SettingRow in AccountSettings.tsx renders only a circular avatar button on the right side; there is no adjacent rectangular element. The h-8 w-20 skeleton (Line 13) appears to be a leftover from the previous design that had a separate "Change" button next to the avatar.
🔧 Proposed fix
<div className="flex items-center gap-3">
<Skeleton className="size-12 rounded-full" />
- <Skeleton className="h-8 w-20" />
</div>📝 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.
Suggested change
| <div className="flex items-center gap-3"> | |
| <Skeleton className="size-12 rounded-full" /> | |
| <Skeleton className="h-8 w-20" /> | |
| </div> | |
| <div className="flex items-center gap-3"> | |
| <Skeleton className="size-12 rounded-full" /> | |
| </div> |
🤖 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/account/components/AccountSettings/components/ProfileSkeleton/ProfileSkeleton.tsx`
around lines 11 - 14, ProfileSkeleton renders a phantom rectangular placeholder
that doesn't exist in the real Avatar SettingRow; in the ProfileSkeleton
component remove the extra Skeleton with className "h-8 w-20" (the rectangular
placeholder) so only the circular Skeleton remains and the markup matches
AccountSettings' Avatar row; reference the ProfileSkeleton component and ensure
the remaining Skeleton keeps its "size-12 rounded-full" classes and alignment
consistent with SettingRow in AccountSettings.tsx.
Comment on lines
+97
to
+101
| const handleLabelBlur = () => { | ||
| if (label !== config.label && label.trim().length > 0) { | ||
| updateMutation.mutate({ label }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
handleLabelBlur saves the untrimmed label string.
The guard uses label.trim().length > 0 but the mutation still sends the raw (possibly leading/trailing-whitespace) value.
🐛 Proposed fix
const handleLabelBlur = () => {
- if (label !== config.label && label.trim().length > 0) {
- updateMutation.mutate({ label });
+ const trimmedLabel = label.trim();
+ if (trimmedLabel !== config.label && trimmedLabel.length > 0) {
+ updateMutation.mutate({ label: trimmedLabel });
}
};🤖 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/agents/components/V2AgentsSettings/components/AgentDetail/AgentDetail.tsx`
around lines 97 - 101, handleLabelBlur currently checks trimmed length but calls
updateMutation.mutate with the raw label, which can persist leading/trailing
whitespace; change handleLabelBlur to pass the trimmed string to
updateMutation.mutate (e.g., compute const trimmed = label.trim() and call
updateMutation.mutate({ label: trimmed })) and keep the existing comparison
using config.label so you only update when the effective label changed.
Comment on lines
+68
to
+74
| const sensors = useSensors( | ||
| useSensor(MouseSensor, { activationConstraint: { distance: 4 } }), | ||
| useSensor(TouchSensor, { | ||
| activationConstraint: { delay: 150, tolerance: 5 }, | ||
| }), | ||
| useSensor(KeyboardSensor), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get its content
find . -name "AgentsSettingsSidebar.tsx" -type fRepository: superset-sh/superset
Length of output: 217
🏁 Script executed:
# Read the relevant section of the file
head -100 apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx | tail -50Repository: superset-sh/superset
Length of output: 1472
🏁 Script executed:
# Check the imports at the top of the file
head -30 apps/desktop/src/renderer/routes/_authenticated/settings/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsxRepository: superset-sh/superset
Length of output: 682
🌐 Web query:
@dnd-kit/sortable KeyboardSensor sortableKeyboardCoordinates required
💡 Result:
In @dnd-kit/sortable, the KeyboardSensor requires the sortableKeyboardCoordinates coordinateGetter option to enable proper keyboard navigation for sortable items. Without it, the default KeyboardSensor moves items by a fixed pixel distance (25px), which doesn't align with sortable behavior. With it, arrow keys move the active item to the closest sortable element in the pressed direction within the same DndContext. Usage example from official docs: const sensors = useSensors( useSensor(PointerSensor), useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates, }) ); This is shown consistently in the sortable preset documentation and source code. It is required for sortable keyboard dragging to work intuitively, preventing issues like page scrolling or incorrect movement.
Citations:
- 1: https://docs.dndkit.com/presets/sortable/
- 2: https://docs.dndkit.com/api-documentation/sensors/keyboard
- 3: https://docs.dndkit.com/legacy/presets/sortable/overview
- 4: https://docs.dndkit.com/presets/sortable
- 5: https://github.com/clauderic/dnd-kit/blob/master/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts
🌐 Web query:
@dnd-kit/sortable keyboard navigation coordinateGetter best practices
💡 Result:
For keyboard navigation in @dnd-kit/sortable, the best practice is to configure the KeyboardSensor with the sortableKeyboardCoordinates coordinateGetter. This custom getter moves the active sortable item to the closest sortable element in the direction of the arrow key pressed (Up, Down, Left, Right), rather than the default 25-pixel increments of the standard KeyboardSensor. Import from '@dnd-kit/sortable': import { sortableKeyboardCoordinates, verticalListSortingStrategy, // other imports } from '@dnd-kit/sortable'; Setup sensors: const sensors = useSensors( useSensor(PointerSensor), useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates, }) ); Use in DndContext within a SortableContext: {/* sortable items */} Key benefits: - Handles variable item sizes and multiple containers. - Filters droppables by direction (e.g., only those below for Down arrow). - Uses closestCorners collision detection. - Adjusts coordinates based on scroll ancestors, same container checks, and item order. - Prevents default browser scrolling with event.preventDefault. Accessibility requirements: - Draggable activators must be focusable (e.g., button, role="button" tabindex=0). - Use Space/Enter to start/end drag (ARIA rule 3). - Customize screenReaderInstructions if changing default keys. Potential issues: - In scrollable containers or dropdowns, downward movement may stick or cause page scroll; mitigations include react-remove-scroll-bar or custom coordinateGetters. - For lists with variable heights, update state in onDragOver for optimal UX. This approach ensures ARIA-compliant keyboard accessibility for sortable lists.
Citations:
- 1: https://github.com/dnd-kit/docs/blob/master/api-documentation/sensors/keyboard.md
- 2: https://docs.dndkit.com/presets/sortable
- 3: https://dndkit.com/legacy/api-documentation/sensors/keyboard
- 4: https://docs.dndkit.com/presets/sortable/
- 5: https://github.com/clauderic/dnd-kit/blob/master/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts
- 6: https://docs.dndkit.com/api-documentation/sensors/keyboard
Wire the keyboard sensor to sortable coordinates.
KeyboardSensor without sortableKeyboardCoordinates breaks keyboard navigation in this sortable list. Without it, the sensor defaults to 25px movement increments instead of moving to the adjacent sortable item.
Suggested change
import {
arrayMove,
SortableContext,
+ sortableKeyboardCoordinates,
useSortable,
verticalListSortingStrategy,
} from "@dnd-kit/sortable";
@@
const sensors = useSensors(
useSensor(MouseSensor, { activationConstraint: { distance: 4 } }),
useSensor(TouchSensor, {
activationConstraint: { delay: 150, tolerance: 5 },
}),
- useSensor(KeyboardSensor),
+ useSensor(KeyboardSensor, {
+ coordinateGetter: sortableKeyboardCoordinates,
+ }),
);📝 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.
Suggested change
| const sensors = useSensors( | |
| useSensor(MouseSensor, { activationConstraint: { distance: 4 } }), | |
| useSensor(TouchSensor, { | |
| activationConstraint: { delay: 150, tolerance: 5 }, | |
| }), | |
| useSensor(KeyboardSensor), | |
| ); | |
| const sensors = useSensors( | |
| useSensor(MouseSensor, { activationConstraint: { distance: 4 } }), | |
| useSensor(TouchSensor, { | |
| activationConstraint: { delay: 150, tolerance: 5 }, | |
| }), | |
| useSensor(KeyboardSensor, { | |
| coordinateGetter: sortableKeyboardCoordinates, | |
| }), | |
| ); |
🤖 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/agents/components/V2AgentsSettings/components/AgentsSettingsSidebar/AgentsSettingsSidebar.tsx`
around lines 68 - 74, The KeyboardSensor used in useSensors is not wired to
sortableKeyboardCoordinates, causing keyboard navigation to use default 25px
increments instead of moving between sortable items; update the
useSensor(KeyboardSensor) call inside useSensors to pass
sortableKeyboardCoordinates as the coordinate getter (i.e.,
useSensor(KeyboardSensor, { coordinateGetter: sortableKeyboardCoordinates })) so
KeyboardSensor uses the sortable list coordinates; ensure you import
sortableKeyboardCoordinates where AgentsSettingsSidebar defines useSensors and
keep the existing MouseSensor and TouchSensor options intact.
Comment on lines
+259
to
+270
| <button | ||
| type="button" | ||
| onClick={handleLogoUpload} | ||
| disabled={!isOwner} | ||
| className="rounded-md transition-opacity hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-100" | ||
| aria-label="Change organization logo" | ||
| > | ||
| <OrganizationLogo | ||
| logo={logoPreview} | ||
| name={organization.name} | ||
| /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Logo button missing selectImageMutation.isPending guard — inconsistent with the avatar button pattern.
In AccountSettings.tsx the avatar button uses disabled={selectImageMutation.isPending} to block re-entry during the async upload flow. The logo button here only guards with disabled={!isOwner}, so a second click during the HTTP upload phase (after the Electron file dialog closes) will race with the in-flight request — producing duplicate toast.success calls and a non-deterministic final logoPreview value.
🐛 Proposed fix
<button
type="button"
onClick={handleLogoUpload}
- disabled={!isOwner}
+ disabled={!isOwner || selectImageMutation.isPending}
className="rounded-md transition-opacity hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-100"
aria-label="Change organization logo"
>📝 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.
Suggested change
| <button | |
| type="button" | |
| onClick={handleLogoUpload} | |
| disabled={!isOwner} | |
| className="rounded-md transition-opacity hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-100" | |
| aria-label="Change organization logo" | |
| > | |
| <OrganizationLogo | |
| logo={logoPreview} | |
| name={organization.name} | |
| /> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={handleLogoUpload} | |
| disabled={!isOwner || selectImageMutation.isPending} | |
| className="rounded-md transition-opacity hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:cursor-not-allowed disabled:opacity-100" | |
| aria-label="Change organization logo" | |
| > | |
| <OrganizationLogo | |
| logo={logoPreview} | |
| name={organization.name} | |
| /> | |
| </button> |
🤖 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/organization/components/OrganizationSettings/OrganizationSettings.tsx`
around lines 259 - 270, The logo button in OrganizationSettings is missing the
upload-in-progress guard, so update the button that calls handleLogoUpload
(rendering OrganizationLogo) to also disable while selectImageMutation.isPending
— e.g., change the disabled prop to combine the ownership check and the mutation
pending flag (disabled={!isOwner || selectImageMutation.isPending}) so it
matches the avatar/upload pattern in AccountSettings.tsx and prevents
re-entry/double toasts during the async upload flow.
Comment on lines
+203
to
+217
| const handleBrowseDirectory = async () => { | ||
| const result = await selectDirectory.mutateAsync({ | ||
| title: "Select preset directory", | ||
| defaultPath: browseDefaultPath, | ||
| }); | ||
| if (!result.canceled && result.path) { | ||
| if (originWorkspace?.worktreePath) { | ||
| onDirectorySelect( | ||
| toPresetDirectoryValue(originWorkspace.worktreePath, result.path), | ||
| ); | ||
| return; | ||
| } | ||
| onDirectorySelect(result.path); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Add error handling around selectDirectory.mutateAsync.
If the IPC call throws (e.g., unavailable channel or unexpected Electron error), the exception bubbles up from the onClick handler as an unhandled promise rejection — the UI shows no feedback and the button is not automatically re-enabled. A try/catch keeps the interaction safe.
🛡️ Proposed fix
- const handleBrowseDirectory = async () => {
- const result = await selectDirectory.mutateAsync({
- title: "Select preset directory",
- defaultPath: browseDefaultPath,
- });
- if (!result.canceled && result.path) {
- if (originWorkspace?.worktreePath) {
- onDirectorySelect(
- toPresetDirectoryValue(originWorkspace.worktreePath, result.path),
- );
- return;
- }
- onDirectorySelect(result.path);
- }
- };
+ const handleBrowseDirectory = async () => {
+ try {
+ const result = await selectDirectory.mutateAsync({
+ title: "Select preset directory",
+ defaultPath: browseDefaultPath,
+ });
+ if (!result.canceled && result.path) {
+ if (originWorkspace?.worktreePath) {
+ onDirectorySelect(
+ toPresetDirectoryValue(originWorkspace.worktreePath, result.path),
+ );
+ return;
+ }
+ onDirectorySelect(result.path);
+ }
+ } catch {
+ // IPC failure – dialog was not shown or channel is unavailable.
+ // selectDirectory.error will be set; nothing further to do.
+ }
+ };🤖 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/components/PresetEditorDialog/PresetEditorDialog.tsx`
around lines 203 - 217, The handleBrowseDirectory function calls
selectDirectory.mutateAsync directly and can throw, so wrap the await call in a
try/catch inside handleBrowseDirectory (the function defined near
PresetEditorDialog) to prevent unhandled promise rejections; on error,
log/report the error (use existing logger or set an error state/notification)
and ensure any UI state that disables the browse button is reset so the button
is re-enabled, while keeping the successful-path behavior that calls
onDirectorySelect (including using toPresetDirectoryValue when
originWorkspace?.worktreePath exists).
Comment on lines
+216
to
+232
| <button | ||
| type="button" | ||
| onClick={() => setShowSessionList((v) => !v)} | ||
| className="inline-flex items-center gap-1.5 text-muted-foreground hover:text-foreground transition-colors cursor-pointer" | ||
| > | ||
| <HiChevronRight | ||
| className={cn( | ||
| "size-3 transition-transform", | ||
| showSessionList && "rotate-90", | ||
| )} | ||
| /> | ||
| <span | ||
| aria-hidden | ||
| className="size-1.5 rounded-full bg-emerald-500" | ||
| /> | ||
| {sessionCountLabel} | ||
| </button> |
Contributor
There was a problem hiding this comment.
Expand/collapse button is missing aria-expanded.
Screen readers can't determine the expansion state without it.
♿ Proposed fix
<button
type="button"
onClick={() => setShowSessionList((v) => !v)}
+ aria-expanded={showSessionList}
className="inline-flex items-center gap-1.5 text-muted-foreground hover:text-foreground transition-colors cursor-pointer"
>🤖 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/V2SessionsSection/V2SessionsSection.tsx`
around lines 216 - 232, The expand/collapse button in V2SessionsSection lacks an
aria-expanded attribute; update the button (the element that calls
setShowSessionList and uses showSessionList) to include
aria-expanded={showSessionList} so assistive tech can read the current state,
and optionally add aria-controls pointing to the session list's id (ensure the
session list element has a matching id) to link the button to the collapsible
region.
Comment on lines
+18
to
+42
| const commit = () => { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed) { | ||
| setValue(currentName); | ||
| return; | ||
| } | ||
| if (trimmed === currentName) return; | ||
| projectActions.renameProject(projectId, trimmed); | ||
| }; | ||
|
|
||
| return ( | ||
| <Input | ||
| value={value} | ||
| onChange={(e) => setValue(e.target.value)} | ||
| onBlur={commit} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter") { | ||
| e.preventDefault(); | ||
| (e.target as HTMLInputElement).blur(); | ||
| } | ||
| if (e.key === "Escape") { | ||
| e.preventDefault(); | ||
| setValue(currentName); | ||
| (e.target as HTMLInputElement).blur(); | ||
| } |
Contributor
There was a problem hiding this comment.
Escape resets display but onBlur → commit() still commits the draft
When the user presses Escape:
setValue(currentName)schedules a batched state update —valuein the closure is still the draft..blur()fires synchronously, triggeringonBlur→commit().commit()reads the stale draftvalue, findstrimmed !== currentName, and callsrenameProject(projectId, draft)— the opposite of the intended discard.
The fix is a synchronous ref flag, since refs are not batched:
🐛 Proposed fix
-import { useEffect, useState } from "react";
+import { useEffect, useRef, useState } from "react";
export function NameSection({ projectId, currentName }: NameSectionProps) {
const { v2Projects: projectActions } = useOptimisticCollectionActions();
const [value, setValue] = useState(currentName);
+ const escapingRef = useRef(false);
useEffect(() => {
setValue(currentName);
}, [currentName]);
const commit = () => {
+ if (escapingRef.current) {
+ escapingRef.current = false;
+ return;
+ }
const trimmed = value.trim();
if (!trimmed) {
setValue(currentName);
return;
}
if (trimmed === currentName) return;
projectActions.renameProject(projectId, trimmed);
};
return (
<Input
value={value}
onChange={(e) => setValue(e.target.value)}
onBlur={commit}
onKeyDown={(e) => {
if (e.key === "Enter") {
e.preventDefault();
(e.target as HTMLInputElement).blur();
}
if (e.key === "Escape") {
e.preventDefault();
+ escapingRef.current = true;
setValue(currentName);
(e.target as HTMLInputElement).blur();
}
}}🤖 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/v2-project/`$projectId/components/V2ProjectSettings/components/NameSection/NameSection.tsx
around lines 18 - 42, The Escape handler currently calls setValue(currentName)
(which is batched) then blur(), causing onBlur → commit() to read the stale
draft value and accidentally rename; fix by introducing a synchronous ref flag
(e.g., isDiscardRef) used by onKeyDown Escape to mark that the change was
discarded, and update commit() to check that ref and skip rename if set,
clearing the ref afterwards; reference the commit function, value/currentName
state, the onBlur handler, and the onKeyDown Escape branch to implement this
behavior.
Comment on lines
+48
to
+52
| if (e.key === "Escape") { | ||
| e.preventDefault(); | ||
| setValue(currentRepoCloneUrl ?? ""); | ||
| (e.target as HTMLInputElement).blur(); | ||
| } |
Contributor
There was a problem hiding this comment.
Escape discards visual state but still commits the draft via onBlur
Same race condition as NameSection (see that file): setValue(currentRepoCloneUrl ?? "") schedules a batched state update, so when the synchronous .blur() fires onBlur → commit(), the value closure still holds the draft. If the draft differs from currentRepoCloneUrl, updateRepository is called with the wrong value.
🐛 Proposed fix using an escape ref
+import { useEffect, useRef, useState } from "react";
-import { useEffect, useState } from "react"; export function RepositorySection({ ... }) {
const { v2Projects: projectActions } = useOptimisticCollectionActions();
const [value, setValue] = useState(currentRepoCloneUrl ?? "");
+ const escapingRef = useRef(false);
const openUrl = electronTrpc.external.openUrl.useMutation();
...- onBlur={commit}
+ onBlur={() => {
+ if (escapingRef.current) {
+ escapingRef.current = false;
+ return;
+ }
+ commit();
+ }}
onKeyDown={(e) => {
if (e.key === "Enter") {
e.preventDefault();
(e.target as HTMLInputElement).blur();
}
if (e.key === "Escape") {
e.preventDefault();
+ escapingRef.current = true;
setValue(currentRepoCloneUrl ?? "");
(e.target as HTMLInputElement).blur();
}
}}🤖 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/v2-project/`$projectId/components/V2ProjectSettings/components/RepositorySection/RepositorySection.tsx
around lines 48 - 52, The Escape handler currently does
setValue(currentRepoCloneUrl ?? "") then immediately triggers blur which invokes
onBlur → commit while the state update is still pending, causing commit to see
the old draft; modify the handler and related onBlur/commit logic to use an
"escapeRef" boolean flag (or similar) on keydown: when Escape is pressed, set
escapeRef.current = true, reset the input visually via
setValue(currentRepoCloneUrl ?? ""), call blur, and in onBlur/commit check
escapeRef.current—if true, clear the flag and skip calling
updateRepository/commit; ensure the flag is reset for normal blurs so other
paths still commit changes.
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx # apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx
Click-the-thumbnail upload (drops the redundant Upload/Replace button) and quieter ghost Remove, mirroring the AccountSettings avatar pattern. Switch to FaGithub for consistency with RepositorySection.
Biome useSemanticElements: span[role=button] should just be button. Safe here because the drag handle is a sibling of the row button, not nested inside it — sibling DOM nodes don't propagate clicks.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx (1)
11-15: ⚡ Quick winUse alias import for
SettingsListSidebarinstead of a relative path.Line 11 currently uses a long relative import; switch this to the repo alias form for consistency with the rest of this file and desktop codebase.
♻️ Suggested diff
import { type SettingsListGroup, settingsListItemClass, SettingsListSidebar, -} from "../../../components/SettingsListSidebar"; +} from "renderer/routes/_authenticated/settings/components/SettingsListSidebar";As per coding guidelines, "
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible".🤖 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/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx` around lines 11 - 15, Replace the long relative import for SettingsListSidebar/SettingsListGroup/settingsListItemClass with the repo tsconfig alias import used elsewhere in this file; locate the import statement that currently imports SettingsListSidebar, SettingsListGroup, and settingsListItemClass and change its module specifier to the project alias form (the same alias pattern used by other imports in this file) so the three symbols are imported via the configured path alias instead of the deep relative 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/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsx`:
- Around line 11-15: Replace the long relative import for
SettingsListSidebar/SettingsListGroup/settingsListItemClass with the repo
tsconfig alias import used elsewhere in this file; locate the import statement
that currently imports SettingsListSidebar, SettingsListGroup, and
settingsListItemClass and change its module specifier to the project alias form
(the same alias pattern used by other imports in this file) so the three symbols
are imported via the configured path alias instead of the deep relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60b2e2dc-1d79-4d90-8354-a57e5692f47e
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/settings/projects/components/ProjectsSettingsSidebar/ProjectsSettingsSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/components/IconUploadField/IconUploadField.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/v2-project/$projectId/components/V2ProjectSettings/V2ProjectSettings.tsx
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
2 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…et-sh#4035) * refactor(desktop): restyle Account settings page Replace shadcn Card with bordered row-stack pattern (matching 1Code). Bump avatar to 48px with explicit Change button instead of hover-pencil. Fix right-column width inconsistency (avatar w-8 vs inputs flex-1) by giving every row a fixed w-80 control. Wrap Sign out in its own box so the page reads as two parallel containers instead of card + bare button. * refactor(desktop): polish Appearance settings page - Sentence case all section headings (Markdown style, Editor font, Terminal font, Import theme, Download base file) - Standardize header rhythm: h3 mb-1 + xs description mb-3 - Declutter Theme header: fold Marketplace and Docs links into a natural-language description line - Markdown: merge the trailing Tufte note into the section description so the section ends with the control, not prose - Font Reset button: ghost text-xs muted -> outline sm so it's discoverable next to the controls - Add aria-label to font size Input and Markdown Select for a11y * refactor(desktop): horizontally center settings content Wrap the Outlet in a centered max-width container so every settings page sits in the middle of the content pane instead of pinned to the left edge of the sidebar. The pages' own max-w-4xl becomes redundant but harmless; new pages get centered for free. * refactor(desktop): polish Notifications settings page - Sentence case headings and button labels (Notification sound, Add / Replace custom audio) - Broaden subtitle so it covers toggle + volume + ringtone, not just the picker - Drop the dangling tip block at the bottom; fold the format hint into the section description under Notification sound - Swap HiPlus -> HiArrowPath when the button means Replace, so the icon matches the action - Drop the decorative speaker icon from the Volume label so it matches every other setting label on the page - Tighten section gap from space-y-8 to space-y-6 to match Behavior * refactor(desktop): polish RingtoneCard selection state - Drop the ring around selected cards: border-2 + ring-2 + check was three signals for one state. Now border-primary + check + a subtle footer tint, single-thickness border so cards don't shift on select. - Promote the play button: bigger (h-9), centered in the preview area with a shadow, so it reads as the primary affordance instead of a corner hint. Emoji moves below the button as visual identity. - Move duration inline with the name in the footer (it's metadata, not decoration) and drop the floating top-right badge. - Add aria-label and aria-pressed for keyboard / SR users. * fix(desktop): move ringtone play button back to corner Centered play overlapping the emoji read as a single broken element, not as two distinct affordances. Restore the corner placement and the 4xl emoji. Keep the other RingtoneCard wins from the previous commit (no ring on selected, inline duration, aria labels). * refactor(desktop): swap RingtoneCard play/check positions Check moves to top-right of the preview area as a small badge so the emoji area stays uncluttered. Play moves into the footer alongside the name + duration, where it sits next to the metadata it acts on. Both affordances now have a single, predictable home each. * style(desktop): soften selected ringtone border to primary/40 * refactor(desktop): remove dead telemetry UI from General settings Drops the always-hidden ({false && ...}) telemetry toggle and its useQuery/useMutation/handler plumbing from BehaviorSettings, plus the matching SETTING_ITEM_ID, variant gate, and search index entry. The underlying getTelemetryEnabled tRPC procedure stays because TelemetrySync still consumes it at boot to set the PostHog opt state. * refactor(desktop): polish Keyboard shortcuts settings page - Match other settings pages: text-xl page title, sentence case ("Keyboard shortcuts"), p-6 max-w-4xl w-full wrapper order. - Drop the inner max-h-[calc(100vh-320px)] scroll: it doubled up with the parent settings pane scroll and made the page feel cramped. Parent now scrolls naturally. - Drop the loud uppercase Command/Shortcut column header inside each category card -- the rows are clearly label + chip, no header needed. - Promote "Reset all" from ghost to outline so it reads as a page-level action, distinct from the per-row Reset buttons. - Recording state now visually highlights the active row (subtle destructive-tinted bg + ring on the trigger) and changes the trigger text from "Recording..." to "Press a key...". * refactor(desktop): polish Git & worktrees settings page Sentence case headings ("Git & worktrees", "Branch prefix") and tighten the Branch prefix description so it explains the setting in addition to showing the preview. WorktreeLocationPicker is left untouched because v1 ProjectSettings still consumes it. * refactor(desktop): polish v2 Agents settings page - Page wrapper max-w-5xl -> max-w-4xl: the layout already caps at 4xl so the wider value was silently dead. Match the rest of settings. - Reset to defaults: ghost -> outline so it reads as a page-level action distinct from row-level controls (matches Keyboard's Reset all). - Friendlier zero-state: dashed-border card with a Bot icon, "No agents yet" heading, and a hint pointing at the Add menu, instead of a single bare line of muted text. - Loading: skeleton cards that mirror the V2AgentCard layout instead of "Loading agent settings...". - Drop the empty grey square placeholder when a preset has no icon (in both the card and the Add-agent dropdown). Empty rectangles read as broken; absence reads as intentional. * refactor(desktop): tone down V2AgentCard styling Card was reading as a "toy" -- big rounded shadow on every row, app icon at 32px, semibold title, big primary-blue argv/stdin pills. - Card: rounded-xl + shadow-sm + py-6/gap-6 -> rounded-lg, no shadow, no internal gap. Flatter, less floating widget energy. - Header: text-base semibold title -> text-sm medium title; description text-sm -> text-xs muted; preset icon size-8 -> size-7. Header padding p-4 -> p-3 to fit the smaller type. - Drag handle: bare icon with no affordance -> icon in a hover-bg pill with a slightly muted default color so it reads as draggable. - Trash: hover -> destructive tint to make the destructive action read as destructive only on intent. - Padding alignment: expanded CardContent inherited shadcn px-6 (24px) while the header was p-4 (16px), so fields were inset further than the title above them. Drop CardContent and use plain px-4 pb-4. - Field grouping: insert a border-t between Label (identity) and the three launch fields so the form has a readable two-section structure. - Field labels shrunk to text-xs and inputs use text-xs font-mono so the dense launch settings fit without feeling oversized. - Argv/stdin: replace two primary-blue pill buttons with a true segmented control (single bordered group, accent-tinted selected half). Reads as one decision, not two competing buttons. - Tighten copy: drop "(empty)" placeholder for "--prompt"; compress the argv/stdin explanation into one line. * refactor(desktop): collapse V2 agent cards into a single bordered list 1Code's settings list pattern is one bordered container with divide-y rows -- no individual cards per item. Agents had 8 separate Cards floating with space-y-3 between them, which made each row look like its own widget instead of a list entry. - V2AgentsSettings: wrap configs in `rounded-lg border bg-card divide-y` instead of `space-y-3`. - V2AgentCard: drop the per-item <Card> wrapper. The row sits flat inside the parent container; hover bg lifts via hover:bg-muted/50 to match 1Code AccountRow. - Drag state: when isDragging, lift the row via bg-accent/40 + z-10 so it visually sits above the rest of the list during reorder. Less floating-widget energy, denser, reads as a settings list. * style(desktop): drop bg-card from V2 agents list container * style(desktop): pad expanded V2 agent content from header * refactor(desktop): adopt 1Code horizontal-row pattern for V2 agent fields The expanded panel previously stacked Label/Input/description vertically for each field. 1Code's settings forms (model config, account config, etc.) put label + hint on the left and the control on the right at a fixed w-80, separated by border-t. Same row vocabulary as the rest of settings, so the expanded content reads continuous with the page instead of like a popup form. - New local AgentField helper renders one row: label + hint left, control (w-80, shrink-0) right, separated by border-t. - All four fields (Label, Command, Prompt-only args, Prompt transport) use the new pattern. - Drop the previous nested grouping divider; row dividers carry the separation now. - Promote field labels back to text-sm so they line up visually with the agent's title in the header above. * refactor(desktop): convert ringtone picker to bordered list rows Same pattern as the v2 agents list: one bordered container with divide-y rows, no per-item bg, no per-item chrome. Each ringtone is one row with [emoji] [name + duration on top, description below] [play button] [check]. Sound selection is a list-of-options task, not visual comparison, so cards were wasting vertical space and making each option feel like a widget instead of a list entry. * refactor(desktop): polish v2 Terminal settings page - Page wrapper max-w-7xl -> max-w-4xl: layout caps at 4xl already, so the wider value was dead. Match the rest of settings. - Sentence case "Terminal presets" / "Add preset" header. - Drop the orphan "Click a preset row to edit details." hint that sat below the table -- click affordance is discoverable from the row hover state and editor sheet open. * fix(desktop): put play button to the right of the check in ringtone rows * refactor(desktop): collapse theme section into 1Code-style row + dropdown Replaces the 3-column ThemeCard grid (and split-preview SystemThemeCard) with a single bordered list: one Theme row with a Select dropdown, and Light/Dark rows that slide in only when System is selected. Each option shows a small Aa swatch in the picker. ThemeCard / SystemThemeCard components are no longer needed. * refactor(desktop): unify theme section header into the bordered list * style(desktop): drop divider lines between appearance sections * refactor(desktop): redesign terminal presets list Drop the uppercase column header + striped rows. PresetsTable becomes a bordered list with divide-y matching Agents/Ringtones. PresetRow shows name + workspace/tab badges on top, command snippet (truncated, with +N overflow indicator) below, and a right-side meta column with applies-to + mode. Visibility eye and chevron sit at the right edge. QuickAddPresets becomes a tighter chip strip with rounded-full pills. * refactor(desktop): unify v2 presets into one card and quiet the rows V2PresetsSection now wraps header, quick-add chips and the preset list into one bordered card with divide-y separators, matching the new theme section. PresetsTable gains a `bordered` prop so v1 keeps its own border. PresetRow trims the right side: chevron is gone (the whole row already clicks), applies-to + mode collapse to a single muted line, and the visibility eye fades in on row hover when the preset is visible. * refactor(desktop): convert PresetEditorSheet to a centered Dialog Renames PresetEditorSheet -> PresetEditorDialog and switches the side sheet to a max-w-2xl modal Dialog matching 1Code's MCP/agent edit patterns. The body now uses a 2-column grid for short fields (Name + Description) and full-width sections for Commands, Applies-to, Directory, Launch mode, and a bordered Auto-run group. Drops the custom field/control className overrides, the uppercase "Advanced" subheading, the question-mark tooltips, and the LabelWithTooltip helper. Fields use a small Field helper with a hint underneath instead. * style(desktop): widen PresetEditorDialog to max-w-4xl * refactor(desktop): two-column layout for PresetEditorDialog body The dialog now uses both halves of its width: left column groups the recipe (Name, Description, Commands), right column groups the launch behavior (Applies to, Directory, Launch mode, Auto-run). Drops the DialogDescription (the title carries enough context), drops the bordered Auto-run group, and shortens the multi-command launch-mode labels. * chore(desktop): drop unused DialogDescription import in PresetEditorDialog * refactor(desktop): single column PresetEditorDialog with settings rows Drops the cluttered two-column form. Width returns to max-w-2xl (matching 1Code's MCP edit dialog footprint) and the body is single column: - Top: Name, Description, Commands as labeled fields - Bottom: bordered settings list with rows for Applies to, Directory, Launch mode, plus two Switch rows for the auto-run options Launch mode now uses a single Select dropdown for both single- and multi-command cases (replacing the radio group). Auto-run uses Switches in their own rows instead of two checkboxes packed under one label. * refactor(desktop): unify PresetEditorDialog rows on the V2AgentCard pattern The whole body becomes one bordered card with divide-y rows. Each row follows V2AgentCard's AgentField shape: label + hint on the left (min-w-0 flex-1), control on the right (w-80 shrink-0). Multi-line controls (Commands editor, ProjectTargetingField) use a stacked variant of the same row so the label sits on top and the editor spans the full width. Also collapses Directory's text "Browse" button to an icon-only button so the input gets the row's full control width. * style(desktop): drop the inner card border around PresetEditorDialog rows * style(desktop): drop row dividers in PresetEditorDialog * refactor(desktop): tighter row controls in PresetEditorDialog - Applies to: stacked radio + popover -> segmented (All / Specific) with the project picker only appearing under Specific. Inline row. - Launch mode: single-command case becomes a Segmented control (Current tab / New tab); multi-command keeps the Select for the longer labels. - Auto-run: collapses two Switch rows into one row with two checkbox options (On workspace creation / On new tab). - Tighten DialogRow padding from p-4 to py-2.5 (no horizontal padding) and the right-side control width from w-80 to w-72. * revert(desktop): bring back two Switch rows for Auto-run * revert(desktop): bring back stacked radios for Applies to * style(desktop): restore segmented Applies to in an inline row * style(desktop): more breathing room between PresetEditorDialog rows * refactor(desktop): drop bordered cards in Links settings Each LinkTierMapper section becomes a flat title + description + 3 inline rows (label + smaller Select). Matches the borderless settings section style we converged on elsewhere. * refactor(desktop): redesign Models settings around the lighter section style Each provider is one borderless SettingsSection: small icon + name + description on the left, status badge + Sign in / Sign out on the right. Below sits the API key ConfigRow (label on top, input + Save/Clear inline). The old standalone "Override Provider" collapsible folds into Anthropic as an inline Advanced collapsible holding Auth token, Base URL, and Additional env vars. Sentence case throughout (API key, Auth token, Additional env vars), swaps Connect/Logout to Sign in/Sign out, drops the rounded-xl border + bg-card chrome, and softens row title weight from font-semibold to font-medium to match Appearance/Links. * refactor(desktop): horizontal ConfigRow + Active badge + ghost Sign out ConfigRow lifts the AgentField horizontal pattern: label+description on the left, the input + Save/Clear buttons on the right at w-80. When the API key is the active credential, an inline 'Active' badge sits next to the row's label. Sign out drops to the ghost variant so it visually steps back next to a primary Sign in. * Revert "refactor(desktop): horizontal ConfigRow + Active badge + ghost Sign out" This reverts commit d227690. * refactor(desktop): simpler Anthropic Advanced section in Models The three override fields all save into the same env config, so per-row Save/Clear buttons were redundant. Switch to save-on-blur for Auth token, Base URL, and Additional env vars; replace the per-row buttons with a single 'Reset' link that appears in the Advanced trigger row when any field has content. Adds a small 'Saved on blur' hint at the bottom. * chore(desktop): drop unused hasAnthropicConfig in ModelsSettings * refactor(desktop): align Organization settings with the rest of settings Standardize page wrapper to p-6 max-w-4xl and the title to text-xl. Drop the Card/CardContent wrapper and convert the org info rows to the flat SettingsRow pattern used elsewhere. Replace the hover-pencil overlay on Logo and Slug with visible "Change" buttons. Members table keeps its border but loses the heavy text-lg heading in favor of a small section header with hint. * style(desktop): drop dividers between Organization info rows * style(desktop): drop explicit Change buttons in Organization rows The logo image itself is now the trigger for the file picker (cursor + hover-fade + ring-on-focus). The slug input is read-only and clicking or pressing Enter/Space opens the SlugDialog, which still owns the format check + availability check + URL-changes-confirmation flow. * chore(desktop): drop unused Button import in OrganizationSettings * style(desktop): drop Change button on Account avatar; click avatar to upload * style(desktop): align project settings section heading with the rest Soften the shared SettingsSection heading from text-base font-semibold to text-sm font-medium so V2 project pages match the section heading style used in Appearance, Models, etc. Sentence-case the V2 project section titles ("Host Service Location" -> "Project location", "Danger Zone" -> "Danger zone") and tighten the descriptions. * style(desktop): flatten integrations settings rows Drop the heavyweight Card layout for borderless rows that match the rest of settings: smaller icon, description below name, status dot + 'Connected to X' / 'Not connected' inline next to the Manage/Connect button. Sentence case throughout. * style(desktop): standardize billing settings page Apply the shared settings vocabulary: text-xl page title with sentence-cased description, drop the manual border-border/60 + shadow-none on the hero plan/upgrade cards (use defaults), refactor BillingDetails into 3 borderless DetailRows with Edit on right, slim RecentInvoices into a divide-y list with always-visible View link. * style(desktop): standardize API keys settings page Match the rest of settings: p-6 max-w-4xl wrapper, text-xl page title, sentence case throughout. Move Generate key into the header row, drop the second redundant subtitle, swap the Table for a flat divide-y list with hover-revealed Revoke. Drop the bespoke amber warning box in the new-key dialog for a one-line muted hint. * style(desktop): flatten billing plan + upgrade into borderless rows Drop the two heavy bordered hero cards (CurrentPlan + Upgrade) for a single 'Plan' section with two divide-y rows that match BillingDetails and RecentInvoices below. Drop the Upgrade card's feature grid and View-all-plans button (the All plans link in the page header already covers this); keep a single Upgrade button on the right. * style(desktop): give Pro and Enterprise plans a premium accent Pro gets a violet→blue gradient on the plan name + violet sparkles icon. Enterprise gets an amber→rose gradient + amber sparkles. Free stays plain. The Upgrade button on the free-plan upsell row also takes the violet→blue gradient so the destination tier reads visually before you even click it. * style(desktop): revert gradient billing accent to a monochrome pill The violet/amber gradient + sparkles read as cheap. Replaced with a small high-contrast monochrome pill (Linear/Notion-style) — bg-foreground text-background, uppercase, tight tracking. Single restrained signal that the tier is paid; the Upgrade button goes back to the default primary style. * refactor(desktop): tighten v2 project settings sections RepositorySection: drop the Edit/Save/Cancel state machine for save-on-blur on a single Input — type, blur to save, Esc to revert. ProjectLocationSection: drop the redundant 'This device' label column (the section header already covers it); show path or 'Not set up' across the full row with the action button to its right. DeleteProjectSection: lift to label + hint + button so it reads consistently with other settings rows. * feat(desktop): rename / GitHub link / softer delete on v2 project settings Drawing from 1Code's project tab: add a Name section at the top with save-on-blur rename (was previously read-only via the page header). Repository row now shows a small 'Open' button with the GitHub icon when the URL parses to a github.com remote — opens the repo in the browser. Soften the destructive Delete button to outline + destructive hover so it's clearly dangerous without dominating the page; the AlertDialog still gates the actual delete. * style(desktop): bump section descriptions to text-sm + tighter copy 1Code uses text-sm muted descriptions in settings; ours used text-xs which read as cramped. Bump the shared SettingsSection helper to text-sm (touches v1 too — acceptable per v1-sunset). Shorten the v2 project descriptions to label-style phrases ('Display name for this project') instead of conversational ones. * style(desktop): tighter v2 project settings layout Drop the redundant 'Danger zone' heading — the Delete row already self-describes; replace with a thin border-t separator. Drop the descriptive subtitles on Name and Repository (the labels + placeholders cover it; only Project location keeps its description since state matters). Tighten section spacing space-y-8 → space-y-6. * style(desktop): tighter agent detail layout to match v2 project Drop redundant section descriptions on Label and Launch (the labels + placeholders cover them). Tighten section spacing space-y-10 → space-y-6. Replace the 'Delete this agent' Section heading with a thin border-t separator + label+hint+button row, matching the v2 project Delete pattern. * style(desktop): align host settings with v2 project vocabulary Members heading text-base → text-sm to match every other settings section. Drop the redundant 'People in your organization who can use this host' description (the heading + table covers it). Surface the 'Only owners can change membership' hint up under the heading when the viewer isn't an owner, instead of as a footer below the table. HostHeader: drop the inline <code> tag for the machine ID, use a font-mono span; trim 'machine ID' to just the ID after the status separator. * style(desktop): align daemon section heading with the rest of settings Replace the section heading <Label> with an <h3 className='text-sm font-medium'> so it matches every other settings section. Bump the description to text-sm and trim the verbose copy ('Owns every PTY session and survives app restarts.' instead of two sentences explaining what a PTY is). Status row, action buttons, monochrome 'Update available' pill, and AlertDialogs all already match the vocabulary — left alone. * style(desktop): chevron-disclosure for daemon sessions; surface unavailable state Drop the manual Refresh button — refetchOnWindowFocus + the 5s poll while the list is open already cover this. Replace the 'Show sessions / Hide sessions' action button with a chevron disclosure on the status row itself: chevron + dot + count becomes one clickable element when there are sessions to show. Color-code the daemon-unavailable case with destructive (red dot + destructive text) so it reads as a problem rather than a benign 'no sessions' state. * style(desktop): hoist daemon action buttons into the section header Update daemon and Force restart move from a row below the status into the right side of the section header — same pattern as Hosts > Members + Add member button. Frees up vertical space and reads top-down: heading + actions, then status + disclosure, then expanded session table. * fix(desktop): align daemon status row to text baseline items-center centers each child's box vertically — but the chevron + dot inside the button have different box metrics than the plain font-mono version span next to it, so the text baselines end up offset. items-baseline aligns the actual text baselines so '1 session running', 'v0.1.4', and the 'Update available' pill all sit on the same line. * style(desktop): move daemon version next to heading; drop session-table border Version moves from the status row up next to 'Manage daemon' as inline metadata (small font-mono, normal weight, muted) — version is a property of the daemon itself, not part of session state. Drop the rounded-md border wrapping the expanded sessions table; the table sits directly in the section without a contained box. * other styles * revert(desktop): drop last-visited settings page persistence Did not work in practice. Restore the simple Account redirect. * style(desktop): align v2 project icon field with settings vocabulary Click-the-thumbnail upload (drops the redundant Upload/Replace button) and quieter ghost Remove, mirroring the AccountSettings avatar pattern. Switch to FaGithub for consistency with RepositorySection. * fix(desktop): use real <button> for agent drag handle Biome useSemanticElements: span[role=button] should just be button. Safe here because the drag handle is a sibling of the row button, not nested inside it — sibling DOM nodes don't propagate clicks.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…ngs vocabulary superset-sh#4035 introduced settings vocabulary unification (superset-sh#4035 [HUGE 73f]) and a new clickPolicy-driven LinkTierMapper API. Several fork-specific settings were not in upstream's SETTING_ITEM_ID enum, the v1->v2 migration UI relies on trpc procedures the fork does not implement, and superset-sh#3823's iconUrl field is not yet adopted. settings-search.ts: add 21 fork-specific SETTING_ITEM_ID entries (APPEARANCE_VIBRANCY, BEHAVIOR_LANGUAGE_DIAGNOSTICS, BEHAVIOR_RIGHT_SIDEBAR_OPEN_VIEW_WIDTH, GIT_SMART_COMMIT, GIT_AUTO_STASH, GIT_BRANCH_SORT_ORDER, GIT_POST_COMMIT_COMMAND, METRICS_GITHUB_OVERVIEW, METRICS_GITHUB_TRAFFIC, METRICS_GITHUB_COPY, PROJECT_AUTO_IMPORT_WORKTREES, PROJECT_AUTO_REMOVE_WORKTREES, RINGTONES_AIVIS, RINGTONES_AIVIS_DICTIONARY, RINGTONES_AIVIS_USAGE, SERVICE_STATUS_PROVIDERS, TERMINAL_SUGGESTIONS, VSCODE_EXTENSIONS_MANAGE, VSCODE_EXTENSIONS_INDENT_RAINBOW, VSCODE_EXTENSIONS_TRAILING_SPACES, VSCODE_EXTENSIONS_REFERENCE_GRAPH) with matching SETTING_ITEM_VARIANT entries. LinkTierMapper.tsx + index.ts: revert to fork's pre-superset-sh#3988 actionLabels API (no renderer/lib/clickPolicy yet, which arrives with Phase 5 superset-sh#3988). ExperimentalSettings.tsx: drop v1->v2 migration UI block. The underlying useMigrateV1DataToV2 hook calls trpc procedures (readV1WorkspaceSections / listState / upsertState) the fork removed. Keep EXPERIMENTAL_SUPERSET_V2 toggle. EXPERIMENTAL_V1_MIGRATION constant stays in settings-search so existing search keywords remain stable. Re-introduce the migration UI and re-wire the missing trpc procedures when v1->v2 migration is intentionally added back. useMigrateV1DataToV2/: re-delete entire hook directory (upstream attempted to restore it via superset-sh#4035 vocabulary cleanup paths). V2ProjectSettings.tsx: drop the Appearance / IconUploadField section (superset-sh#3823 iconUrl arrives in cycle 41). ProjectsSettingsSidebar.tsx: drop iconUrl from ProjectRow + select, pass githubOwner={null} to ProjectThumbnail (will be restored in cycle 41).
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
A sweep across every settings page in the desktop app to converge on a single visual vocabulary. No behavioral changes — just style, layout, and copy.
The shared vocabulary:
p-6 max-w-4xl w-fulltext-xl font-semibold(sentence case)<h3 class="text-sm font-medium">text-sm text-muted-foreground(wastext-xsin places)divide-ylists where appropriate; reserved bordered cards for hero panels (e.g. expanded session table)Pages touched
SettingsSectionhelper bumped totext-smdescriptions (affects v1 and v2 — acceptable per v1-sunset)Highlights worth reviewing
FaGithubfor "Use GitHub icon" so it visually matches the Repository "Open" buttonisV2CloudEnabledand dropping v1)Notes
origin/mainmid-branch to absorb feat(desktop): v2 project icon settings #3823 (v2 project icon settings) and fix(desktop): make NewProjectModal error text selectable #4036 (modal text selection); resolved two conflicts (ProjectsSettingsSidebar,V2ProjectSettings) to keep both main's new behaviors and our visual vocabulary.Test plan
Summary by CodeRabbit
Release Notes
New Features
UI/Style Improvements
Refactor