V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button#3197
Merged
Conversation
…tton - Replace v1/v2 open-in button branching with unified WorkspaceOpenInButton - Move OrganizationDropdown from top bar to sidebar header (v2 cloud) - Add folder+ "Add Repository" button next to org dropdown in sidebar - Org dropdown expands to fill available width with truncation
Contributor
📝 WalkthroughWalkthroughThe changes restructure dashboard UI by replacing workspace navigation with an "Add Repository" action, updating OrganizationDropdown to be variant-driven, adding a new RightSidebarToggle component, conditionally rendering based on feature flags, and refactoring V2OpenInMenuButton to use local state instead of react-query queries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR restructures the v2 top bar by moving Key changes:
Confidence Score: 4/5Safe to merge with one targeted fix — the non-functional Add Repository button should be addressed before release. The refactoring is clean and well-structured: the OrganizationDropdown variant system is correct, the WorkspaceOpenInButton unification avoids React rules-of-hooks violations, and the RightSidebarToggle correctly wires to the same state as the TOGGLE_SIDEBAR hotkey. The only blocking gap is the missing onClick on the Add Repository button — it's likely an intentional placeholder, but ships as a visible interactive element that does nothing. DashboardSidebarHeader.tsx — Add Repository button has no onClick handler in either the collapsed or expanded variant. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WorkspaceOpenInButton] --> B{v2WorkspaceId?}
B -- Yes --> C[V2Inner]
B -- No --> D{v1WorkspaceId?}
D -- Yes --> E[V1Inner]
D -- No --> F[null]
C --> G[useLiveQuery: v2Workspaces]
C --> H[useLiveQuery: v2Devices]
C --> I[electronTrpc.auth.getDeviceInfo]
C --> J{workspace && hostUrl && isLocalWorkspace?}
J -- No --> F
J -- Yes --> K[useQuery: host workspace.get]
K --> L{worktreePath?}
L -- No --> F
L -- Yes --> M[OpenInMenuButton]
E --> N[electronTrpc.workspaces.get]
N --> O{worktreePath?}
O -- No --> F
O -- Yes --> M
Reviews (1): Last reviewed commit: "Unify open-in button, move org dropdown ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
19
to
27
| <button | ||
| type="button" | ||
| onClick={() => openModal()} | ||
| className="flex size-8 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground" | ||
| > | ||
| <LuPlus className="size-4" strokeWidth={STROKE_WIDTH_THICK} /> | ||
| <LuFolderPlus className="size-4" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="right"> | ||
| New Workspace ({shortcutText}) | ||
| </TooltipContent> | ||
| <TooltipContent side="right">Add Repository</TooltipContent> | ||
| </Tooltip> |
There was a problem hiding this comment.
onClick handler
The LuFolderPlus button in both the collapsed (lines 19-27) and expanded (lines 38-47) states is rendered as an interactive button but has no onClick handler attached. Clicking it does nothing. If this is intended as a placeholder, it should at minimum be disabled or noted with a TODO comment. If it's meant to open a modal or navigate somewhere, the handler is missing.
Collapsed state:
Suggested change
| <button | |
| type="button" | |
| onClick={() => openModal()} | |
| className="flex size-8 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground" | |
| > | |
| <LuPlus className="size-4" strokeWidth={STROKE_WIDTH_THICK} /> | |
| <LuFolderPlus className="size-4" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="right"> | |
| New Workspace ({shortcutText}) | |
| </TooltipContent> | |
| <TooltipContent side="right">Add Repository</TooltipContent> | |
| </Tooltip> | |
| <button | |
| type="button" | |
| onClick={() => { /* TODO: open add repository flow */ }} | |
| className="flex size-8 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground" | |
| > |
Comment on lines
+25
to
+38
| const getToggleIcon = (isHovering: boolean) => { | ||
| if (!isOpen) { | ||
| return isHovering ? ( | ||
| <LuPanelRightOpen className="size-4" strokeWidth={1.5} /> | ||
| ) : ( | ||
| <LuPanelRight className="size-4" strokeWidth={1.5} /> | ||
| ); | ||
| } | ||
| return isHovering ? ( | ||
| <LuPanelRightClose className="size-4" strokeWidth={1.5} /> | ||
| ) : ( | ||
| <LuPanelRight className="size-4" strokeWidth={1.5} /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The getToggleIcon function returns LuPanelRight (a neutral icon) for both isOpen=false and isOpen=true when not hovering. This means the button looks identical whether the sidebar is open or closed at rest, giving the user no visual feedback about the current state.
Consider returning LuPanelRightOpen when the sidebar is open and not hovering, to mirror typical toggle button conventions:
Suggested change
| const getToggleIcon = (isHovering: boolean) => { | |
| if (!isOpen) { | |
| return isHovering ? ( | |
| <LuPanelRightOpen className="size-4" strokeWidth={1.5} /> | |
| ) : ( | |
| <LuPanelRight className="size-4" strokeWidth={1.5} /> | |
| ); | |
| } | |
| return isHovering ? ( | |
| <LuPanelRightClose className="size-4" strokeWidth={1.5} /> | |
| ) : ( | |
| <LuPanelRight className="size-4" strokeWidth={1.5} /> | |
| ); | |
| }; | |
| const getToggleIcon = (isHovering: boolean) => { | |
| if (!isOpen) { | |
| return isHovering ? ( | |
| <LuPanelRightOpen className="size-4" strokeWidth={1.5} /> | |
| ) : ( | |
| <LuPanelRight className="size-4" strokeWidth={1.5} /> | |
| ); | |
| } | |
| return isHovering ? ( | |
| <LuPanelRightClose className="size-4" strokeWidth={1.5} /> | |
| ) : ( | |
| <LuPanelRightOpen className="size-4" strokeWidth={1.5} /> | |
| ); | |
| }; |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 7 files
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/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx:20">
P2: Guard `v2WorkspaceLocalState.update` with an existence check before toggling right sidebar state.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx:40">
P2: The "Add Repository" button is rendered as an interactive `<button>` element but has no `onClick` handler attached, so clicking it does nothing. The previous "New Workspace" button this replaced had `onClick={() => openModal()}`. Either wire up the intended action or disable the button and add a TODO comment if this is a placeholder.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const isOpen = localState?.rightSidebarOpen ?? false; | ||
|
|
||
| const toggle = () => { | ||
| collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { |
Contributor
There was a problem hiding this comment.
P2: Guard v2WorkspaceLocalState.update with an existence check before toggling right sidebar state.
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/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx, line 20:
<comment>Guard `v2WorkspaceLocalState.update` with an existence check before toggling right sidebar state.</comment>
<file context>
@@ -0,0 +1,59 @@
+ const isOpen = localState?.rightSidebarOpen ?? false;
+
+ const toggle = () => {
+ collections.v2WorkspaceLocalState.update(workspaceId, (draft) => {
+ draft.rightSidebarOpen = !draft.rightSidebarOpen;
+ });
</file context>
| @@ -1,13 +1,6 @@ | |||
| import { Tooltip, TooltipContent, TooltipTrigger } from "@superset/ui/tooltip"; | |||
Contributor
There was a problem hiding this comment.
P2: The "Add Repository" button is rendered as an interactive <button> element but has no onClick handler attached, so clicking it does nothing. The previous "New Workspace" button this replaced had onClick={() => openModal()}. Either wire up the intended action or disable the button and add a TODO comment if this is a placeholder.
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/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx, line 40:
<comment>The "Add Repository" button is rendered as an interactive `<button>` element but has no `onClick` handler attached, so clicking it does nothing. The previous "New Workspace" button this replaced had `onClick={() => openModal()}`. Either wire up the intended action or disable the button and add a TODO comment if this is a placeholder.</comment>
<file context>
@@ -16,89 +9,42 @@ interface DashboardSidebarHeaderProps {
+ <Tooltip delayDuration={300}>
+ <TooltipTrigger asChild>
+ <button
+ type="button"
+ className="flex size-8 shrink-0 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground"
+ >
</file context>
![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: 3
🤖 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/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`:
- Around line 17-27: The "Add Repository" button in DashboardSidebarHeader is
currently a focusable no-op (TooltipTrigger/button with LuFolderPlus) and must
either be wired or disabled: either hook its onClick to the add-repository flow
(e.g., call the existing openAddRepositoryModal or router navigation function
from the surrounding DashboardSidebarHeader component) and add an explicit
aria-label like "Add repository", or render the button as non-interactive until
implemented by adding disabled and aria-disabled attributes and ensuring
TooltipContent reflects that state; apply the same fix to the second identical
instance (the other Tooltip/button block).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx`:
- Around line 87-100: The span showing the organization label in
OrganizationDropdown doesn't shrink because it lacks the min-w-0/flex rules;
update the span that renders displayName inside the button (in
OrganizationDropdown) to include "min-w-0 flex-1" (keeping the existing
"truncate") so long names ellipsize instead of pushing the chevron; no other
structural changes needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx`:
- Around line 43-55: The right-sidebar icon button is currently unnamed and
ambiguous; update the RightSidebarToggle component to provide an explicit
accessible name and state by adding an aria-label like "Toggle right sidebar"
and an aria-pressed attribute bound to the current open/closed state (the same
boolean passed into getToggleIcon/toggle). Also update the HotkeyLabel
invocation to use "Toggle right sidebar" and a unique id (e.g.,
"TOGGLE_RIGHT_SIDEBAR") so the tooltip and keyboard hint match the control;
ensure you reference the button element, the toggle handler, and the
getToggleIcon(boolean) usage when making these changes.
🪄 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: 16086092-3d39-4f74-9c93-262c7b27e0c9
📒 Files selected for processing (7)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceOpenInButton/WorkspaceOpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/WorkspaceOpenInButton/index.ts
Comment on lines
17
to
27
| <Tooltip delayDuration={300}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={() => openModal()} | ||
| className="flex size-8 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground" | ||
| > | ||
| <LuPlus className="size-4" strokeWidth={STROKE_WIDTH_THICK} /> | ||
| <LuFolderPlus className="size-4" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="right"> | ||
| New Workspace ({shortcutText}) | ||
| </TooltipContent> | ||
| <TooltipContent side="right">Add Repository</TooltipContent> | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
Don't expose Add Repository as a no-op.
Both variants render a focusable, actionable-looking button, but there is no click handler, navigation target, or disabled state, so the new action does nothing. Please either wire it to the add-repository flow or render it disabled/hidden until that behavior lands; once it is interactive, it should also get an explicit aria-label.
Also applies to: 37-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`
around lines 17 - 27, The "Add Repository" button in DashboardSidebarHeader is
currently a focusable no-op (TooltipTrigger/button with LuFolderPlus) and must
either be wired or disabled: either hook its onClick to the add-repository flow
(e.g., call the existing openAddRepositoryModal or router navigation function
from the surrounding DashboardSidebarHeader component) and add an explicit
aria-label like "Add repository", or render the button as non-interactive until
implemented by adding disabled and aria-disabled attributes and ensuring
TooltipContent reflects that state; apply the same fix to the second identical
instance (the other Tooltip/button block).
Comment on lines
+87
to
+100
| <button | ||
| type="button" | ||
| className="flex w-full items-center gap-2 rounded-md px-2 py-1.5 text-sm font-medium text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground min-w-0" | ||
| aria-label="Organization menu" | ||
| > | ||
| <Avatar | ||
| size="xs" | ||
| fullName={activeOrganization?.name} | ||
| image={activeOrganization?.logo} | ||
| className="rounded size-4 shrink-0" | ||
| /> | ||
| <span className="truncate">{displayName}</span> | ||
| <HiChevronUpDown className="h-3.5 w-3.5 text-muted-foreground shrink-0" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read it
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx" | head -110 | tail -40Repository: superset-sh/superset
Length of output: 1767
Make the expanded label shrinkable.
In a flex row, the span needs min-w-0 to shrink below its content width and enable the truncate class to work. Without it, long organization names will push the chevron instead of ellipsizing. Adding flex-1 allows the label to grow and fill available space.
Suggested fix
- <span className="truncate">{displayName}</span>
+ <span className="min-w-0 flex-1 truncate">{displayName}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx`
around lines 87 - 100, The span showing the organization label in
OrganizationDropdown doesn't shrink because it lacks the min-w-0/flex rules;
update the span that renders displayName inside the button (in
OrganizationDropdown) to include "min-w-0 flex-1" (keeping the existing
"truncate") so long names ellipsize instead of pushing the chevron; no other
structural changes needed.
Comment on lines
+43
to
+55
| <button | ||
| type="button" | ||
| onClick={toggle} | ||
| className="no-drag group flex items-center justify-center size-8 rounded-md text-muted-foreground hover:text-foreground hover:bg-accent/50 transition-colors" | ||
| > | ||
| <span className="group-hover:hidden">{getToggleIcon(false)}</span> | ||
| <span className="hidden group-hover:block"> | ||
| {getToggleIcon(true)} | ||
| </span> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="left"> | ||
| <HotkeyLabel label="Toggle sidebar" id="TOGGLE_SIDEBAR" /> |
Contributor
There was a problem hiding this comment.
Give the new icon button a specific accessible name and state.
Right now this is an unnamed icon-only control, and "Toggle sidebar" is ambiguous because the top bar already has a left SidebarToggle. Please expose it as the right-sidebar toggle and report its pressed state.
Suggested fix
<button
type="button"
onClick={toggle}
+ aria-label="Toggle right sidebar"
+ aria-pressed={isOpen}
className="no-drag group flex items-center justify-center size-8 rounded-md text-muted-foreground hover:text-foreground hover:bg-accent/50 transition-colors"
>
@@
<TooltipContent side="left">
- <HotkeyLabel label="Toggle sidebar" id="TOGGLE_SIDEBAR" />
+ <HotkeyLabel label="Toggle right sidebar" id="TOGGLE_SIDEBAR" />
</TooltipContent>📝 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={toggle} | |
| className="no-drag group flex items-center justify-center size-8 rounded-md text-muted-foreground hover:text-foreground hover:bg-accent/50 transition-colors" | |
| > | |
| <span className="group-hover:hidden">{getToggleIcon(false)}</span> | |
| <span className="hidden group-hover:block"> | |
| {getToggleIcon(true)} | |
| </span> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="left"> | |
| <HotkeyLabel label="Toggle sidebar" id="TOGGLE_SIDEBAR" /> | |
| <button | |
| type="button" | |
| onClick={toggle} | |
| aria-label="Toggle right sidebar" | |
| aria-pressed={isOpen} | |
| className="no-drag group flex items-center justify-center size-8 rounded-md text-muted-foreground hover:text-foreground hover:bg-accent/50 transition-colors" | |
| > | |
| <span className="group-hover:hidden">{getToggleIcon(false)}</span> | |
| <span className="hidden group-hover:block"> | |
| {getToggleIcon(true)} | |
| </span> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="left"> | |
| <HotkeyLabel label="Toggle right sidebar" id="TOGGLE_SIDEBAR" /> | |
| </TooltipContent> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx`
around lines 43 - 55, The right-sidebar icon button is currently unnamed and
ambiguous; update the RightSidebarToggle component to provide an explicit
accessible name and state by adding an aria-label like "Toggle right sidebar"
and an aria-pressed attribute bound to the current open/closed state (the same
boolean passed into getToggleIcon/toggle). Also update the HotkeyLabel
invocation to use "Toggle right sidebar" and a unique id (e.g.,
"TOGGLE_RIGHT_SIDEBAR") so the tooltip and keyboard hint match the control;
ensure you reference the button element, the toggle handler, and the
getToggleIcon(boolean) usage when making these changes.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx (1)
26-33:⚠️ Potential issue | 🟠 Major
Add Repositorybuttons are still focusable no-ops.Both new icon buttons look interactive but have no action (
onClick/navigation) and no disabled state. Please either wire them to the add-repository flow or render them as disabled/hidden until implemented. Also add an explicit accessible name (aria-label="Add repository").Also applies to: 62-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx` around lines 26 - 33, The Add Repository icon buttons in DashboardSidebarHeader (the <button> elements wrapped by TooltipTrigger showing LuFolderPlus and the similar button at the other occurrence) are focusable no-ops; either wire them into the add-repository flow or make them non-interactive and accessible: add a proper onClick handler that calls the existing add-repository function/route (or dispatch) used by the app, or set disabled and update styling to indicate disabled, and in both cases add an explicit accessible name via aria-label="Add repository" (and remove tabindex/focusability if hidden). Locate the button elements in DashboardSidebarHeader.tsx (the ones rendering LuFolderPlus and wrapped by TooltipTrigger/TooltipContent) and apply one of these fixes consistently for both occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`:
- Around line 26-33: The Add Repository icon buttons in DashboardSidebarHeader
(the <button> elements wrapped by TooltipTrigger showing LuFolderPlus and the
similar button at the other occurrence) are focusable no-ops; either wire them
into the add-repository flow or make them non-interactive and accessible: add a
proper onClick handler that calls the existing add-repository function/route (or
dispatch) used by the app, or set disabled and update styling to indicate
disabled, and in both cases add an explicit accessible name via aria-label="Add
repository" (and remove tabindex/focusability if hidden). Locate the button
elements in DashboardSidebarHeader.tsx (the ones rendering LuFolderPlus and
wrapped by TooltipTrigger/TooltipContent) and apply one of these fixes
consistently for both occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bafa9005-9d35-4212-b3b2-189cdd45e285
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`:
- Around line 51-53: The interactive row in DashboardSidebarProjectRow currently
gets role="button" but doesn't expose its state to assistive tech; when
rendering the clickable row (i.e., when isRenaming is false and onToggleCollapse
is used), add an aria-expanded attribute that reflects the collapse state (use
the same state variable used by the collapse logic—e.g., isCollapsed or
isExpanded—so aria-expanded={isRenaming ? undefined : !isCollapsed} or
aria-expanded={isRenaming ? undefined : isExpanded}) so screen readers can know
whether the section is expanded or collapsed.
- Around line 53-54: Double-click currently triggers onClick (onToggleCollapse)
before onDoubleClick (onStartRename), causing duplicate toggles; update
DashboardSidebarProjectRow so onClick ignores clicks that are part of a
double-click by adding a short-lived suppression flag: create a ref (e.g.
suppressClickRef) checked at the top of onToggleCollapse wrapper to return early
if set, set suppressClickRef.current = true at the start of onStartRename (or
its wrapper) and clear it after a short timeout (e.g. 200ms) or once rename mode
is entered; keep using isRenaming to disable handlers otherwise and reference
the existing onToggleCollapse, onStartRename and isRenaming symbols when
applying the change.
🪄 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: 2d3885dc-9563-4793-8e1f-2c126640e27f
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/RightSidebarToggle/RightSidebarToggle.tsx
Comment on lines
51
to
53
| role={isRenaming ? undefined : "button"} | ||
| tabIndex={isRenaming ? undefined : 0} | ||
| onClick={isRenaming ? undefined : onToggleCollapse} |
Contributor
There was a problem hiding this comment.
Expose expanded/collapsed state on the interactive row.
Since the row is now the collapse control (role="button"), it should report state with aria-expanded for screen reader users.
Suggested fix
role={isRenaming ? undefined : "button"}
tabIndex={isRenaming ? undefined : 0}
+ aria-expanded={isRenaming ? undefined : !isCollapsed}📝 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
| role={isRenaming ? undefined : "button"} | |
| tabIndex={isRenaming ? undefined : 0} | |
| onClick={isRenaming ? undefined : onToggleCollapse} | |
| role={isRenaming ? undefined : "button"} | |
| tabIndex={isRenaming ? undefined : 0} | |
| aria-expanded={isRenaming ? undefined : !isCollapsed} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`
around lines 51 - 53, The interactive row in DashboardSidebarProjectRow
currently gets role="button" but doesn't expose its state to assistive tech;
when rendering the clickable row (i.e., when isRenaming is false and
onToggleCollapse is used), add an aria-expanded attribute that reflects the
collapse state (use the same state variable used by the collapse logic—e.g.,
isCollapsed or isExpanded—so aria-expanded={isRenaming ? undefined :
!isCollapsed} or aria-expanded={isRenaming ? undefined : isExpanded}) so screen
readers can know whether the section is expanded or collapsed.
Comment on lines
53
to
+54
| onClick={isRenaming ? undefined : onToggleCollapse} | ||
| onDoubleClick={isRenaming ? undefined : onStartRename} |
Contributor
There was a problem hiding this comment.
Prevent duplicate collapse toggles on double-click rename.
Double-click emits click events first, so this path can fire onToggleCollapse multiple times before onStartRename, causing extra state churn/flicker.
Suggested fix
- onClick={isRenaming ? undefined : onToggleCollapse}
+ onClick={
+ isRenaming
+ ? undefined
+ : (event) => {
+ // Ignore multi-clicks; double-click is reserved for rename.
+ if (event.detail > 1) return;
+ onToggleCollapse();
+ }
+ }
onDoubleClick={isRenaming ? undefined : onStartRename}📝 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
| onClick={isRenaming ? undefined : onToggleCollapse} | |
| onDoubleClick={isRenaming ? undefined : onStartRename} | |
| onClick={ | |
| isRenaming | |
| ? undefined | |
| : (event) => { | |
| // Ignore multi-clicks; double-click is reserved for rename. | |
| if (event.detail > 1) return; | |
| onToggleCollapse(); | |
| } | |
| } | |
| onDoubleClick={isRenaming ? undefined : onStartRename} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`
around lines 53 - 54, Double-click currently triggers onClick (onToggleCollapse)
before onDoubleClick (onStartRename), causing duplicate toggles; update
DashboardSidebarProjectRow so onClick ignores clicks that are part of a
double-click by adding a short-lived suppression flag: create a ref (e.g.
suppressClickRef) checked at the top of onToggleCollapse wrapper to return early
if set, set suppressClickRef.current = true at the start of onStartRename (or
its wrapper) and clear it after a short timeout (e.g. 200ms) or once rename mode
is entered; keep using isRenaming to disable handlers otherwise and reference
the existing onToggleCollapse, onStartRename and isRenaming symbols when
applying the change.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsx (1)
47-62: Consider handling query error state.The component returns
nullfor both loading and error states. While this is acceptable for loading (the button appears once data is ready), silently swallowing errors could make debugging difficult if the workspace query fails.Consider adding minimal error handling or logging:
♻️ Optional error handling
const workspaceQuery = useQuery({ queryKey: ["v2-open-in-workspace", hostUrl, workspaceId], queryFn: () => getHostServiceClientByUrl(hostUrl as string).workspace.get.query({ id: workspaceId, }), enabled: !!workspace && !!hostUrl && isLocalWorkspace, }); if (!workspace || !hostUrl || !isLocalWorkspace) { return null; } +if (workspaceQuery.isError) { + console.warn("[V2WorkspaceOpenInButton] Failed to fetch workspace:", workspaceQuery.error); + return null; +} + if (!workspaceQuery.data?.worktreePath) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsx` around lines 47 - 62, The component currently swallows query failures; add minimal error handling by checking workspaceQuery.isError (and optionally workspaceQuery.isLoading) before the early returns and log the error with context (e.g., include hostUrl and workspaceId) using console.error or the app logger so failures from getHostServiceClientByUrl(...).workspace.get.query are visible; keep the existing null return for loading but ensure workspaceQuery.isError triggers a logged message referencing workspaceQuery.error, workspaceQuery, workspaceId, hostUrl and isLocalWorkspace to aid debugging.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx (1)
73-89: Inconsistent dependency arrays in useCallback hooks.The dependency arrays mix mutation objects with their
.isPendingproperties inconsistently:
- Line 76:
[..., openInApp, copyPath.isPending]— includes fullopenInAppbut onlycopyPath.isPending- Line 83:
[..., openInApp, copyPath.isPending]— same pattern- Line 89:
[..., copyPath, openInApp.isPending]— inverted patternFor consistency and to avoid subtle stale closure issues, prefer using the full mutation object references:
♻️ Suggested fix for consistency
const handleOpenInEditor = useCallback(() => { if (openInApp.isPending || copyPath.isPending) return; openInApp.mutate({ path: worktreePath, app: defaultApp }); -}, [worktreePath, defaultApp, openInApp, copyPath.isPending]); +}, [worktreePath, defaultApp, openInApp, copyPath]); const handleOpenInOtherApp = useCallback( (appId: ExternalApp) => { if (openInApp.isPending || copyPath.isPending) return; openInApp.mutate({ path: worktreePath, app: appId }); }, - [worktreePath, openInApp, copyPath.isPending], + [worktreePath, openInApp, copyPath], ); const handleCopyPath = useCallback(() => { if (openInApp.isPending || copyPath.isPending) return; copyPath.mutate(worktreePath); -}, [worktreePath, copyPath, openInApp.isPending]); +}, [worktreePath, copyPath, openInApp]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx` around lines 73 - 89, The three useCallback hooks (handleOpenInEditor, handleOpenInOtherApp, handleCopyPath) have inconsistent dependency arrays mixing full mutation objects and their .isPending properties; update each hook to depend on the full mutation objects (openInApp and copyPath) along with the other used values (worktreePath, defaultApp for handleOpenInEditor, and appId usage context for handleOpenInOtherApp) so all three callbacks list consistent dependencies: worktreePath, defaultApp where used, openInApp, and copyPath.
🤖 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/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx`:
- Around line 73-89: The three useCallback hooks (handleOpenInEditor,
handleOpenInOtherApp, handleCopyPath) have inconsistent dependency arrays mixing
full mutation objects and their .isPending properties; update each hook to
depend on the full mutation objects (openInApp and copyPath) along with the
other used values (worktreePath, defaultApp for handleOpenInEditor, and appId
usage context for handleOpenInOtherApp) so all three callbacks list consistent
dependencies: worktreePath, defaultApp where used, openInApp, and copyPath.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsx`:
- Around line 47-62: The component currently swallows query failures; add
minimal error handling by checking workspaceQuery.isError (and optionally
workspaceQuery.isLoading) before the early returns and log the error with
context (e.g., include hostUrl and workspaceId) using console.error or the app
logger so failures from getHostServiceClientByUrl(...).workspace.get.query are
visible; keep the existing null return for loading but ensure
workspaceQuery.isError triggers a logged message referencing
workspaceQuery.error, workspaceQuery, workspaceId, hostUrl and isLocalWorkspace
to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8d1d855-3498-4df4-902c-4963e047e144
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Contributor
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
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/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx:37">
P1: `defaultApp` is initialized once from `workspaceId`-scoped data and never resynced when `workspaceId` changes, so the button can open the new workspace in the previous workspace’s default editor.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx:51">
P2: This adds a second near-identical implementation of the open-in split-button flow instead of reusing a shared component/helper, which increases maintenance risk and drift.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const activeTheme = useThemeStore((state) => state.activeTheme); | ||
|
|
||
| const localState = collections.v2WorkspaceLocalState.get(workspaceId); | ||
| const [defaultApp, setDefaultApp] = useState<ExternalApp>( |
Contributor
There was a problem hiding this comment.
P1: defaultApp is initialized once from workspaceId-scoped data and never resynced when workspaceId changes, so the button can open the new workspace in the previous workspace’s default editor.
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/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx, line 37:
<comment>`defaultApp` is initialized once from `workspaceId`-scoped data and never resynced when `workspaceId` changes, so the button can open the new workspace in the previous workspace’s default editor.</comment>
<file context>
@@ -1,37 +1,201 @@
+ const activeTheme = useThemeStore((state) => state.activeTheme);
+
+ const localState = collections.v2WorkspaceLocalState.get(workspaceId);
+ const [defaultApp, setDefaultApp] = useState<ExternalApp>(
+ (localState?.defaultOpenInApp as ExternalApp) ?? "finder",
+ );
</file context>
| [collections, workspaceId], | ||
| ); | ||
|
|
||
| const openInApp = electronTrpc.external.openInApp.useMutation({ |
Contributor
There was a problem hiding this comment.
P2: This adds a second near-identical implementation of the open-in split-button flow instead of reusing a shared component/helper, which increases maintenance risk and drift.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.)
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/_dashboard/components/TopBar/components/V2OpenInMenuButton/V2OpenInMenuButton.tsx, line 51:
<comment>This adds a second near-identical implementation of the open-in split-button flow instead of reusing a shared component/helper, which increases maintenance risk and drift.
(Based on your team's feedback about preferring shared helpers over repeated multi-step flows.) </comment>
<file context>
@@ -1,37 +1,201 @@
+ [collections, workspaceId],
+ );
+
+ const openInApp = electronTrpc.external.openInApp.useMutation({
+ onSuccess: (_data, variables) => {
+ handleDefaultAppChange(variables.app);
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
…en-in button (superset-sh#3197) * Add toggle button * Remove workspaces view * Move org * Unify open-in button, move org dropdown to sidebar, add repository button - Replace v1/v2 open-in button branching with unified WorkspaceOpenInButton - Move OrganizationDropdown from top bar to sidebar header (v2 cloud) - Add folder+ "Add Repository" button next to org dropdown in sidebar - Org dropdown expands to fill available width with truncation * new ws button * Clean up ui * Rename and icon * Lint * Open in * 2 buttons * Clean up
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
Apr 7, 2026
cherry-pick済み: - e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190) - 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177) - 11ed4f8 V2 terminal env (superset-sh#3184) - 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196) - 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199) - 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198) - d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197) - 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205) - 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194) - 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218) フォーク独自対応: - cleanupMainWindowResources()をexit pathに移動維持 (#3205対応) - BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線 - BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正（リマップ対応） - ansi_up依存維持
4 tasks
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
WorkspaceOpenInButtoncomponentTest plan
Summary by cubic
Streamlines the v2 workspace UI by moving org controls into the sidebar, adding a right-panel toggle, and unifying the Open in control with a per-workspace default. Also polishes project rows and icons for quicker navigation.
expanded/collapsedvariants; top-bar dropdown shows only whenFEATURE_FLAGS.V2_CLOUDis disabled.WorkspaceOpenInButtonfor v1 worktrees and v2 local workspaces; split button opens the default editor, dropdown lets you choose another app or copy path, shows branch, remembers the default per workspace, and supports hotkeys.Written for commit b44be71. Summary will update on new commits.
Summary by CodeRabbit
New Features
UI Improvements
Enhancements