Refine dashboard sidebar interactions#2518
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the V2 workspace/sidebar and new-workspace modal surface with a Dashboard-oriented implementation: removes many V2 components/hooks/tests and adds DashboardSidebar components, hooks, localStorage collections/schemas, modal form refactor, and wiring to keep workspaces in the sidebar. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DashboardSidebar as DashboardSidebar
participant DataHook as useDashboardSidebarData
participant Collections as LocalStorageCollections
participant TRPCApi as TRPC API
participant SidebarState as useDashboardSidebarState
User->>DashboardSidebar: mount
DashboardSidebar->>DataHook: request groups
DataHook->>Collections: read v2SidebarProjects/workspaces/sections
DataHook->>TRPCApi: fetch v2Projects/v2Workspaces/v2Devices/githubRepos
TRPCApi-->>DataHook: return data
DataHook-->>DashboardSidebar: return groups (projects/sections/workspaces)
User->>DashboardSidebar: toggle project collapse
DashboardSidebar->>SidebarState: toggleProjectCollapsed(projectId)
SidebarState->>Collections: persist updated isCollapsed/tabOrder
Collections-->>SidebarState: acknowledge
SidebarState-->>DashboardSidebar: updated groups/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 128 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
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/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.ts:34">
P2: The mock status selection is mathematically pinned to the first status, so `working` and `review` are never generated.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsx:56">
P2: Reset `renameValue` when starting rename from the context menu; otherwise the input can open with stale text from a previous rename attempt.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx:81">
P2: The collapsed-sidebar context menu exposes a Rename action that does not present any rename UI, so the action is effectively non-functional in that mode.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx:50">
P1: The parent keydown handler toggles collapse even when Enter/Space is pressed on nested buttons, causing unintended section toggles for keyboard users.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx:59">
P1: Guard the row-level key handler so it only runs when the row itself is focused; otherwise keyboard use on nested buttons can also toggle collapse.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx:73">
P1: Ignore descendant key events in the row key handler; Enter/Space on the nested delete button currently triggers the row action.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx:79">
P2: Disable row double-click while renaming to avoid resetting the rename draft from bubbled dblclick events.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx:36">
P2: Remove the internal placeholder message from the hover card UI; it exposes development copy to end users.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getWorkspaceShortcutLabels.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/getWorkspaceShortcutLabels.ts:11">
P2: Shortcut labels are hardcoded to `⌘1-9`, so the sidebar can show incorrect shortcuts on non-mac platforms or when users remap workspace hotkeys.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx:80">
P2: Collapsed items pass a `<Tooltip>` component as the context-menu/hover-card trigger child, so trigger bindings may not attach to the actual button.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts:35">
P2: Selecting Rename in collapsed mode can leave the item stuck in `isRenaming`, which blocks navigation clicks with no visible way to finish/cancel rename.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| isRenaming | ||
| ? undefined | ||
| : (event) => { | ||
| if (event.key === "Enter" || event.key === " ") { |
Contributor
There was a problem hiding this comment.
P1: The parent keydown handler toggles collapse even when Enter/Space is pressed on nested buttons, causing unintended section toggles for keyboard users.
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/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx, line 50:
<comment>The parent keydown handler toggles collapse even when Enter/Space is pressed on nested buttons, causing unintended section toggles for keyboard users.</comment>
<file context>
@@ -0,0 +1,116 @@
+ isRenaming
+ ? undefined
+ : (event) => {
+ if (event.key === "Enter" || event.key === " ") {
+ event.preventDefault();
+ onToggleCollapse();
</file context>
| isRenaming | ||
| ? undefined | ||
| : (event) => { | ||
| if (event.key === "Enter" || event.key === " ") { |
Contributor
There was a problem hiding this comment.
P1: Guard the row-level key handler so it only runs when the row itself is focused; otherwise keyboard use on nested buttons can also toggle collapse.
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/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx, line 59:
<comment>Guard the row-level key handler so it only runs when the row itself is focused; otherwise keyboard use on nested buttons can also toggle collapse.</comment>
<file context>
@@ -0,0 +1,149 @@
+ isRenaming
+ ? undefined
+ : (event) => {
+ if (event.key === "Enter" || event.key === " ") {
+ event.preventDefault();
+ onToggleCollapse();
</file context>
Comment on lines
+73
to
+78
| onKeyDown={(event) => { | ||
| if (event.key === "Enter" || event.key === " ") { | ||
| event.preventDefault(); | ||
| onClick(); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
P1: Ignore descendant key events in the row key handler; Enter/Space on the nested delete button currently triggers the row action.
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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx, line 73:
<comment>Ignore descendant key events in the row key handler; Enter/Space on the nested delete button currently triggers the row action.</comment>
<file context>
@@ -0,0 +1,255 @@
+ tabIndex={0}
+ ref={ref}
+ onClick={onClick}
+ onKeyDown={(event) => {
+ if (event.key === "Enter" || event.key === " ") {
+ event.preventDefault();
</file context>
Suggested change
| onKeyDown={(event) => { | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onClick(); | |
| } | |
| }} | |
| onKeyDown={(event) => { | |
| if (event.target !== event.currentTarget) return; | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onClick(); | |
| } | |
| }} |
| const paneStatuses: ActivePaneStatus[] = ["permission", "working", "review"]; | ||
| const hasPr = seed % 5 !== 0; | ||
| const status = | ||
| seed % 6 === 0 ? paneStatuses[seed % paneStatuses.length] : null; |
Contributor
There was a problem hiding this comment.
P2: The mock status selection is mathematically pinned to the first status, so working and review are never generated.
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/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.ts, line 34:
<comment>The mock status selection is mathematically pinned to the first status, so `working` and `review` are never generated.</comment>
<file context>
@@ -0,0 +1,51 @@
+ const paneStatuses: ActivePaneStatus[] = ["permission", "working", "review"];
+ const hasPr = seed % 5 !== 0;
+ const status =
+ seed % 6 === 0 ? paneStatuses[seed % paneStatuses.length] : null;
+
+ return {
</file context>
| <div style={sectionBorderStyle}> | ||
| <DashboardSidebarSectionContextMenu | ||
| color={section.color} | ||
| onRename={() => setIsRenaming(true)} |
Contributor
There was a problem hiding this comment.
P2: Reset renameValue when starting rename from the context menu; otherwise the input can open with stale text from a previous rename attempt.
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/DashboardSidebarSection/DashboardSidebarSection.tsx, line 56:
<comment>Reset `renameValue` when starting rename from the context menu; otherwise the input can open with stale text from a previous rename attempt.</comment>
<file context>
@@ -0,0 +1,83 @@
+ <div style={sectionBorderStyle}>
+ <DashboardSidebarSectionContextMenu
+ color={section.color}
+ onRename={() => setIsRenaming(true)}
+ onSetColor={(color) => setSectionColor(section.id, color)}
+ onDelete={() => onDelete(section.id)}
</file context>
| onClick(); | ||
| } | ||
| }} | ||
| onDoubleClick={onDoubleClick} |
Contributor
There was a problem hiding this comment.
P2: Disable row double-click while renaming to avoid resetting the rename draft from bubbled dblclick events.
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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx, line 79:
<comment>Disable row double-click while renaming to avoid resetting the rename draft from bubbled dblclick events.</comment>
<file context>
@@ -0,0 +1,255 @@
+ onClick();
+ }
+ }}
+ onDoubleClick={onDoubleClick}
+ className={cn(
+ "relative flex w-full items-center pl-3 pr-2 text-left text-sm cursor-pointer",
</file context>
Suggested change
| onDoubleClick={onDoubleClick} | |
| onDoubleClick={isRenaming ? undefined : onDoubleClick} |
| </div> | ||
|
|
||
| <div className="rounded-md border border-border bg-muted/30 px-2 py-1.5 text-xs text-muted-foreground"> | ||
| Mocked preview of the legacy workspace hover card. |
Contributor
There was a problem hiding this comment.
P2: Remove the internal placeholder message from the hover card UI; it exposes development copy to end users.
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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx, line 36:
<comment>Remove the internal placeholder message from the hover card UI; it exposes development copy to end users.</comment>
<file context>
@@ -0,0 +1,80 @@
+ </div>
+
+ <div className="rounded-md border border-border bg-muted/30 px-2 py-1.5 text-xs text-muted-foreground">
+ Mocked preview of the legacy workspace hover card.
+ </div>
+
</file context>
| }); | ||
|
|
||
| const handleClick = () => { | ||
| if (isRenaming) return; |
Contributor
There was a problem hiding this comment.
P2: Selecting Rename in collapsed mode can leave the item stuck in isRenaming, which blocks navigation clicks with no visible way to finish/cancel rename.
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/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts, line 35:
<comment>Selecting Rename in collapsed mode can leave the item stuck in `isRenaming`, which blocks navigation clicks with no visible way to finish/cancel rename.</comment>
<file context>
@@ -0,0 +1,109 @@
+ });
+
+ const handleClick = () => {
+ if (isRenaming) return;
+ navigate({
+ to: "/v2-workspace/$workspaceId",
</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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx (1)
69-74:⚠️ Potential issue | 🟡 MinorAvoid mutating query data via in-place sort.
Array.prototype.sortmutates the source array. Ifdata.branchesis reused elsewhere, this can cause side effects.Proposed fix
- const branches = (data?.branches ?? []).sort((a, b) => { + const branches = [...(data?.branches ?? [])].sort((a, b) => { if (a.name === defaultBranch) return -1; if (b.name === defaultBranch) return 1; if (a.isLocal !== b.isLocal) return a.isLocal ? -1 : 1; return a.name.localeCompare(b.name); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx` around lines 69 - 74, The current computation of branches uses Array.prototype.sort which mutates data.branches; change it to sort a shallow copy instead so you don't mutate query data—e.g. create a copy from data?.branches (via spread or slice) before calling sort in the expression that assigns branches; keep the same sort comparator logic (referencing defaultBranch, a.isLocal, b.isLocal and name.localeCompare) so behavior is unchanged while avoiding in-place mutation.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx (1)
28-31:⚠️ Potential issue | 🟡 MinorAwait the clipboard write before showing success.
navigator.clipboard.writeText()is async. Right now a rejected write still emits "Project ID copied", which gives the user a false success state.💡 Suggested fix
- const handleCopyId = () => { - navigator.clipboard.writeText(id); - toast.success("Project ID copied"); - }; + const handleCopyId = async () => { + try { + await navigator.clipboard.writeText(id); + toast.success("Project ID copied"); + } catch { + toast.error("Couldn't copy project ID"); + } + };🤖 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/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx` around lines 28 - 31, The handleCopyId function calls navigator.clipboard.writeText(id) but doesn't await it, so failures still trigger toast.success; update DashboardSidebarProjectContextMenu's handleCopyId to await the async write (e.g., use async/await) and wrap it in try/catch, calling toast.success("Project ID copied") only on successful await and calling toast.error(...) (or similar) in the catch with a helpful message; keep references to id, navigator.clipboard.writeText, toast.success and toast.error so the changes are easy to locate.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/PromptGroup.tsx (1)
133-153:⚠️ Potential issue | 🟠 MajorGuard the keyboard submit path against concurrent submissions.
The
⌘/Ctrl+Enterkeyboard handler callshandleCreate()without checkingisPending, allowing repeated presses to initiate concurrentcreateWorkspace()calls while a request is already in flight—despite the button being disabled.🔧 Suggested fix
const handleCreate = () => { + if (isPending) { + return; + } if (!projectId) { toast.error("Select a project first"); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/PromptGroup.tsx` around lines 133 - 153, The keyboard submit path can start concurrent requests because handleCreate() is invoked regardless of isPending; update handleCreate to early-return when isPending is true (check the isPending state at the top of handleCreate) so repeated ⌘/Ctrl+Enter presses do nothing while a request is in flight, and also update the keyboard handler that calls handleCreate to short-circuit when isPending to avoid invoking the function from the shortcut; reference symbols: handleCreate, isPending, runAsyncAction, and createWorkspace.
🧹 Nitpick comments (14)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/IssuesGroup/IssuesGroup.tsx (1)
24-25: Prefer alias imports in this component as well.Please switch these deep relative imports to
renderer/...aliases for consistency with the desktop codebase.Suggested import update
-import { useDashboardNewWorkspaceDraft } from "../../../../DashboardNewWorkspaceDraftContext"; -import { useCreateDashboardWorkspace } from "../../../../hooks/useCreateDashboardWorkspace"; +import { useDashboardNewWorkspaceDraft } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceDraftContext"; +import { useCreateDashboardWorkspace } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace";As per coding guidelines:
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when 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/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/IssuesGroup/IssuesGroup.tsx` around lines 24 - 25, Replace the deep relative imports in IssuesGroup.tsx with the configured tsconfig alias by importing useDashboardNewWorkspaceDraft and useCreateDashboardWorkspace from the 'renderer/...' paths instead of "../../../../DashboardNewWorkspaceDraftContext" and "../../../../hooks/useCreateDashboardWorkspace"; update the import statements at the top of IssuesGroup.tsx to use the alias so the component uses the project's canonical renderer module paths.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace/useCreateDashboardWorkspace.ts (1)
11-11: Use alias import for local module consistency.This import can use the
renderer/...alias instead of a deep relative path.Suggested import update
-import { useWorkspaceHostOptions } from "../../components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions"; +import { useWorkspaceHostOptions } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions";As per coding guidelines:
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when 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/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace/useCreateDashboardWorkspace.ts` at line 11, Update the import of useWorkspaceHostOptions to use the project TS alias instead of the relative path; locate the import statement that currently references "../../components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions" and replace it with the alias-based import (e.g., import { useWorkspaceHostOptions } from "renderer/..." matching your tsconfig paths) so the useCreateDashboardWorkspace hook uses the aliased module.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx (1)
14-15: Prefer alias imports over deep relative paths.Use the configured
renderer/...alias here for consistency with the rest of the desktop code.Suggested import cleanup
-import { useDashboardNewWorkspaceDraft } from "../../../../DashboardNewWorkspaceDraftContext"; -import { useCreateDashboardWorkspace } from "../../../../hooks/useCreateDashboardWorkspace"; +import { useDashboardNewWorkspaceDraft } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceDraftContext"; +import { useCreateDashboardWorkspace } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace";As per coding guidelines:
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when 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/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx` around lines 14 - 15, Replace the deep relative imports in BranchesGroup.tsx for useDashboardNewWorkspaceDraft and useCreateDashboardWorkspace with the project alias import rooted at "renderer" to match the tsconfig alias; locate the two import lines currently using "../../../../" and change them to import from the equivalent "renderer/..." paths that mirror the same nested component locations (e.g., renderer/routes/_authenticated/.../DashboardNewWorkspaceDraftContext and renderer/routes/_authenticated/.../hooks/useCreateDashboardWorkspace) so the module resolution uses the configured alias instead of deep relative traversal.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DashboardNewWorkspaceListTabContent/DashboardNewWorkspaceListTabContent.tsx (1)
3-3: Prefer therenderer/...alias for this import.The
../../../../hop on Line 3 is brittle, and this file already uses the alias style on Line 2.💡 Suggested fix
-import type { DashboardNewWorkspaceTab } from "../../../../DashboardNewWorkspaceDraftContext"; +import type { DashboardNewWorkspaceTab } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceDraftContext";As per coding guidelines: "apps/desktop/**/*.{ts,tsx}: Use alias as defined in
tsconfig.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/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DashboardNewWorkspaceListTabContent/DashboardNewWorkspaceListTabContent.tsx` at line 3, Replace the brittle relative import of DashboardNewWorkspaceTab (currently imported via "../../../../DashboardNewWorkspaceDraftContext") with the project alias used elsewhere in this file (e.g., the renderer/... alias) so it matches the import style on Line 2; update the import statement that references DashboardNewWorkspaceTab to use the renderer alias path to DashboardNewWorkspaceDraftContext.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx (1)
11-17: Tighten thechildrencontract forasChild.
ContextMenuTrigger asChildneeds exactly one element to clone.React.ReactNodealso admits strings, fragments, and arrays, so callers can type-check usages that this component can't actually render safely.💡 Suggested fix
+import type { ReactElement } from "react"; + interface DashboardSidebarProjectContextMenuProps { id: string; onCreateSection: () => void; onRemoveFromSidebar: () => void; onRename: () => void; onDelete: () => void; - children: React.ReactNode; + children: ReactElement; }Also applies to: 35-35
🤖 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/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx` around lines 11 - 17, The prop type for children in DashboardSidebarProjectContextMenuProps is too broad (React.ReactNode) for the use of ContextMenuTrigger with asChild; change the children type to React.ReactElement (or React.ReactElement<any>) so the component requires exactly one element to clone, and update the same change where this prop type is defined/used (the DashboardSidebarProjectContextMenu component and the other occurrence noted) to ensure callers pass a single React element rather than strings, fragments, or arrays.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PullRequestsGroup/PullRequestsGroup.tsx (1)
18-19: Use path aliases instead of deep relative imports.Please switch these imports to the
renderer/...alias for consistency and safer refactors.♻️ Suggested change
-import { useDashboardNewWorkspaceDraft } from "../../../../DashboardNewWorkspaceDraftContext"; -import { useCreateDashboardWorkspace } from "../../../../hooks/useCreateDashboardWorkspace"; +import { useDashboardNewWorkspaceDraft } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceDraftContext"; +import { useCreateDashboardWorkspace } from "renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace";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/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PullRequestsGroup/PullRequestsGroup.tsx` around lines 18 - 19, Replace the deep relative imports for the DashboardNewWorkspaceDraft context and create-workspace hook with the project alias form; specifically update the import statements that currently import useDashboardNewWorkspaceDraft and useCreateDashboardWorkspace from "../../../../DashboardNewWorkspaceDraftContext" and "../../../../hooks/useCreateDashboardWorkspace" to use the 'renderer/...' path alias that maps to the same modules (e.g., import useDashboardNewWorkspaceDraft from "renderer/…/DashboardNewWorkspaceDraftContext" and import useCreateDashboardWorkspace from "renderer/…/hooks/useCreateDashboardWorkspace") so the component PullRequestsGroup uses aliased paths instead of deep relative paths.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsx (1)
15-52: AdddisplayNamefor the forwardRef component.Components wrapped with
forwardRefshould have adisplayNameset for better debugging experience in React DevTools.♻️ Proposed fix
export const DashboardSidebarCollapsedWorkspaceButton = forwardRef< HTMLButtonElement, DashboardSidebarCollapsedWorkspaceButtonProps >( ( { hostType, isActive, isUnread = false, workspaceStatus = null, className, ...props }, ref, ) => { return ( <button type="button" ref={ref} className={cn( "relative flex items-center justify-center size-8 rounded-md", "hover:bg-muted/50 transition-colors cursor-pointer", isActive && "bg-muted", className, )} {...props} > <DashboardSidebarWorkspaceIcon hostType={hostType} isActive={isActive} isUnread={isUnread} variant="collapsed" workspaceStatus={workspaceStatus} /> </button> ); }, ); + +DashboardSidebarCollapsedWorkspaceButton.displayName = + "DashboardSidebarCollapsedWorkspaceButton";🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsx` around lines 15 - 52, The forwardRef-wrapped component DashboardSidebarCollapsedWorkspaceButton lacks a displayName which makes React DevTools debugging harder; after the forwardRef declaration, set DashboardSidebarCollapsedWorkspaceButton.displayName = "DashboardSidebarCollapsedWorkspaceButton" so the component shows a readable name in DevTools and stack traces (locate the forwardRef export for DashboardSidebarCollapsedWorkspaceButton and add the displayName assignment immediately after it).apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceStatusBadge/DashboardSidebarWorkspaceStatusBadge.tsx (1)
4-4: Consider renamingMockPrStatewhen real data integration is complete.The
Mockprefix suggests this is placeholder typing. Consider renaming toPrStateor importing a shared type when integrating with real PR data.🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceStatusBadge/DashboardSidebarWorkspaceStatusBadge.tsx` at line 4, The type name MockPrState indicates a placeholder; rename MockPrState to a production-ready name (e.g., PrState) or replace it by importing the canonical PR state type from the shared types/module used by the app, then update all references (e.g., in DashboardSidebarWorkspaceStatusBadge and any files importing MockPrState) to the new name or import, and update any exports, type-only imports, and tests accordingly so the code compiles with real PR data.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx (1)
2-7: Consider using path aliases for imports.The relative imports with multiple parent traversals (
../../../../types) could be simplified using path aliases as defined intsconfig.json. This improves readability and maintainability.As per coding guidelines: "Use alias as defined in
tsconfig.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/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx` around lines 2 - 7, Replace the long relative import paths in DashboardSidebarExpandedProjectContent.tsx with the configured tsconfig path aliases: update the import for DashboardSidebarSection and DashboardSidebarWorkspace (currently from "../../../../types") to use the project alias defined in tsconfig.json, and likewise import DashboardSidebarSectionComponent and DashboardSidebarWorkspaceItem via their aliased module roots rather than "../../../DashboardSidebarSection" and "../../../DashboardSidebarWorkspaceItem"; adjust the import statements so the same exported symbols (DashboardSidebarSection, DashboardSidebarWorkspace, DashboardSidebarSection as DashboardSidebarSectionComponent, DashboardSidebarWorkspaceItem) are imported via aliases to improve readability and maintainability.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx (1)
30-37: Hardcoded placeholder content noted.The timestamp "Updated a few minutes ago" and the mocked preview section are placeholders. Ensure these are tracked for replacement with real data when available.
Would you like me to open an issue to track replacing the placeholder content with real data?
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx` around lines 30 - 37, The component DashboardSidebarWorkspaceHoverCardContent contains hardcoded placeholder strings ("Updated a few minutes ago" and "Mocked preview of the legacy workspace hover card"); update the component to accept real data props (e.g., updatedAt/lastUpdated and previewContent or previewData) and render the timestamp using a formatter (timeago/date-fns) or a prop-driven formatted string instead of the fixed text, and render the preview area conditionally (show real previewContent when available, otherwise show a lightweight skeleton or a TODO comment/aria-hidden placeholder). Also add a clear TODO comment or create an issue reference in the component indicating where to wire real data, and ensure the element(s) with the hardcoded text are replaced with prop-driven values in DashboardSidebarWorkspaceHoverCardContent.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx (2)
147-178: Extract duplicated trailing-actions block.The diff-stats + shortcut + close-button cluster is duplicated in both layout branches. Pulling it into a small local component/helper will reduce drift risk.
Also applies to: 217-248
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx` around lines 147 - 178, The trailing "diff-stats + shortcut + close-button" JSX is duplicated inside DashboardSidebarExpandedWorkspaceRow; extract it into a small local component (e.g., DashboardSidebarTrailingActions) that accepts props used by the block — additions, deletions (pass through to DashboardSidebarWorkspaceDiffStats), shortcutLabel, isActive, and onDeleteClick — and reuse that component in both layout branches where the block currently appears (replace the inline cluster that references DashboardSidebarWorkspaceDiffStats, shortcutLabel, Tooltip/TooltipTrigger/TooltipContent and the HiMiniXMark button). Ensure the new component preserves event.stopPropagation(), aria-label, classNames, and Tooltip behavior so the UI and accessibility remain identical.
73-77: Add defensive check to keyboard handler to prevent nested control interference.The onKeyDown handler at lines 73–77 processes keyboard events without validating event origin. Although RenameInput currently prevents propagation via
stopPropagation()at the start of its handleKeyDown, adding a target check follows best practices for accessible interactive divs and protects against potential issues if nested component implementations change.onKeyDown={(event) => { + if (event.target !== event.currentTarget) return; if (event.key === "Enter" || event.key === " ") { event.preventDefault(); onClick(); } }}🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx` around lines 73 - 77, Add a defensive target check in the DashboardSidebarExpandedWorkspaceRow onKeyDown handler so it only reacts to key events originating from the interactive row itself (not nested controls); specifically, before handling Enter/Space and calling onClick, verify that event.target === event.currentTarget (or otherwise ensure the event did not originate from a nested control like the RenameInput) and return early if it did — this complements RenameInput.handleKeyDown's stopPropagation and prevents nested control interference.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx (1)
36-58: Remove redundanttransition-colorsconditionals.Line 38, Line 47, and Line 56 repeat a class already included in the base class string, which adds noise without behavior change.
♻️ Minimal cleanup
className={cn( "size-4 transition-colors", - variant === "expanded" && "transition-colors", isActive ? "text-foreground" : "text-muted-foreground", )}🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx` around lines 36 - 58, The className for the icons in DashboardSidebarWorkspaceIcon contains a redundant conditional "variant === 'expanded' && 'transition-colors'" because "transition-colors" is already present in the base class string; remove the conditional from each icon JSX (the Lu* elements where className is built) so className becomes cn("size-4 transition-colors", isActive ? "text-foreground" : "text-muted-foreground") to eliminate noise while preserving behavior.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/components/PromptGroupAdvancedOptions/PromptGroupAdvancedOptions.tsx (1)
92-93: Inconsistent use of native<label>vs imported<Label>component.Line 92 uses the native
<label>element, but theLabelcomponent is imported from@superset/ui/label(line 15) and used correctly on line 199. Use the imported component for consistent styling.♻️ Suggested fix
- <label htmlFor="branch" className="text-xs text-muted-foreground"> + <Label htmlFor="branch" className="text-xs text-muted-foreground"> Branch name - </label> + </Label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/components/PromptGroupAdvancedOptions/PromptGroupAdvancedOptions.tsx` around lines 92 - 93, Replace the native <label> element with the imported Label component to keep styling consistent: in the PromptGroupAdvancedOptions component swap the plain HTML <label htmlFor="branch" ...> usage with the imported Label component (from `@superset/ui/label`) so that the "Branch name" label uses the same styled Label as other labels in this file (e.g., the Label used at line ~199).
🤖 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 38-69: The two icon-only buttons in DashboardSidebarHeader (the
button that calls handleWorkspacesClick and the button that calls openModal)
lack accessible names; add explicit accessible labels by supplying either
aria-label attributes (e.g., aria-label="Open workspaces" and aria-label="New
workspace") or include visually-hidden text (screen-reader-only) inside the
buttons so assistive tech can announce them; ensure these labels match the
TooltipContent ("Workspaces" and "New Workspace") and keep TooltipTrigger/Tookit
behavior unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx`:
- Around line 53-65: The project thumbnail button does not expose the disclosure
state to assistive tech; wire the button to the component's collapse state by
adding an aria-expanded attribute bound to the disclosure boolean used in
DashboardSidebarCollapsedProjectContent (e.g., isOpen/isCollapsed) and include
aria-controls pointing to the id of the workspace list element so screen readers
know what it toggles; ensure the toggle handler onToggleCollapse remains and
that ProjectThumbnail/button composition passes through ARIA attributes (or
update ProjectThumbnail to accept and forward them) so the state is announced.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`:
- Around line 110-145: The two icon-only buttons (the plus button that calls
onNewWorkspace and the chevron button that calls onToggleCollapse and reads
isCollapsed) lack accessible names; add explicit accessible labels (e.g.,
aria-label or aria-labelledby) to each button instead of relying on Tooltip
text—give the plus button a clear label like "Create new workspace" and the
chevron button a dynamic label that reflects state (e.g., "Expand projects" when
isCollapsed is true and "Collapse projects" when isCollapsed is false); keep
existing aria-expanded on the chevron and ensure event handlers (onNewWorkspace,
onToggleCollapse) remain unchanged.
- Around line 88-105: The rename button in DashboardSidebarProjectRow is
focusable while fully transparent, making it invisible to keyboard users; update
the button (the element that calls onStartRename and is rendered when isRenaming
is false and contains the LuPencil icon) to reveal itself on keyboard focus by
adding focus/focus-visible styles matching the hover reveal (e.g., include
focus-visible:opacity-100 and focus-visible:scale-100 or focus:opacity-100 and
focus:scale-100) so it becomes visible when focused, or alternatively remove it
from the tab order when intentionally hidden using tabIndex={-1}; ensure the
aria-label remains and that the change is applied to the same button element in
DashboardSidebarProjectRow.
- Around line 55-64: The row-level onKeyDown in DashboardSidebarProjectRow is
responding to keyboard events from nested controls (pencil/plus/chevron) and
wrongly calling onToggleCollapse; update the handler to ignore bubbled events by
verifying the event target is the row itself (e.g., check event.target ===
event.currentTarget or similar) before calling preventDefault() and
onToggleCollapse(), preserving the existing isRenaming gating; this ensures
buttons keep their native activation while the row keyboard shortcuts remain
functional.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx`:
- Around line 96-105: The chevron toggle button is icon-only and lacks an
accessible name; add an explicit accessible label (e.g., aria-label or
aria-labelledby) to the button that conveys its purpose and target (use the
section title/header text) while keeping the existing aria-expanded and event
handlers; update the element where onToggleCollapse is used (the button with
aria-expanded={!section.isCollapsed}) to include this accessible name so screen
readers can announce the control.
- Around line 77-91: The pencil rename button in DashboardSidebarSectionHeader
is invisible to keyboard users because it relies only on group-hover classes;
update the button's className to also reveal it on keyboard focus (e.g., add
focus-visible:opacity-100 focus:scale-100 or focus:scale-100
focus-visible:text-foreground), so that when the button receives focus via
keyboard it becomes fully visible and operable without hover, keeping the
existing onClick/onStartRename behavior intact.
- Around line 46-55: The onKeyDown handler in DashboardSidebarSectionHeader is
collapsing the section due to bubbled Enter/Space events from nested buttons;
update the handler (the onKeyDown prop where isRenaming is checked and
onToggleCollapse is called) to ignore events that originated from a child
interactive element by early-returning when event.target !== event.currentTarget
(i.e., only handle the key when the event target is the header itself) so
Enter/Space on the pencil or chevron buttons won't trigger onToggleCollapse.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx`:
- Around line 59-75: The two interactive Button instances in
DashboardSidebarWorkspaceHoverCardContent (the "View branch" and "Open preview"
Buttons) lack onClick handlers; either add appropriate handler props (e.g.,
onViewBranch/onOpenPreview passed into DashboardSidebarWorkspaceHoverCardContent
and wired to the respective Button onClick) or make them clearly non-interactive
by adding disabled={true} and aria-disabled attributes until the feature is
implemented; update the Button usages to reference the handler names
(onViewBranch, onOpenPreview) or disabled flags so the UI is accessible and not
misleading.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 63-79: The context menu's onRename handler is wired to startRename
but the collapsed rendering branch never shows the rename input, so clicking
Rename does nothing; update the collapsed branch of
DashboardSidebarWorkspaceItem to either (a) render the same rename input/control
used in the expanded branch when rename mode is active, or (b) make startRename
trigger the existing rename UI flow (for example by expanding the item or
opening the rename modal) so the user can actually type a new name; locate
DashboardSidebarWorkspaceContextMenu, the startRename function, and the
collapsed rendering branch in DashboardSidebarWorkspaceItem and ensure the
rename UI is presented or the item is expanded when startRename is invoked.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx`:
- Around line 62-69: The dedupe ref lastEnsuredWorkspaceIdRef is never cleared,
so when workspace becomes null the previous id remains and prevents
ensureWorkspaceInSidebar from running for the same workspace later; update the
useEffect that references lastEnsuredWorkspaceIdRef, workspace, and
ensureWorkspaceInSidebar to explicitly reset lastEnsuredWorkspaceIdRef.current =
null when workspace is falsy (before returning) and only perform the equality
check/ensure call when workspace exists, so the ref is cleared on route changes
with no active workspace.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts`:
- Around line 173-185: The tabOrder computation uses only top-level workspaces
and misses other top-level row types, causing ordering collisions; update the
siblingRows filtering (the
Array.from(collections.v2SidebarWorkspaces.state.values()).filter(...) used
before calling getNextTabOrder) to include the full set of top-level rows used
elsewhere (e.g., include items with sectionId === null AND any section rows that
are considered top-level in your app, while still matching projectId and
excluding the moved workspaceId), then pass that complete set into
getNextTabOrder to compute draft.tabOrder; apply the same change to the
analogous code path used when deleting a section (the block around lines 195-213
that also calls getNextTabOrder) so both move-to-top-level and delete-section
use the identical top-level row set for ordering.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Around line 562-587: The v2 sidebar collections (v2SidebarProjects,
v2SidebarWorkspaces, v2SidebarSections) are currently keyed only by
organizationId so UI prefs leak between users; update each
localStorageCollectionOptions call to include the signed-in userId in both the
id and storageKey (e.g., `...-${organizationId}-${userId}`) so the per-user
cache key and localStorage key are unique, ensuring caches created by
createCollection are also scoped by user; use the same userId variable available
in this module when constructing the id and storageKey for each collection.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`:
- Around line 3-5: The persistedDateSchema currently transforms strings to Date
objects but doesn't re-validate the transformed value, allowing Invalid Date
instances through; update the schema for persistedDateSchema (the
z.union([...]).transform(...) chain) to pipe the transformed result into
z.date() (e.g., add .pipe(z.date())) so malformed strings that produce Invalid
Date are rejected by the schema validation.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx`:
- Around line 28-31: The handleCopyId function calls
navigator.clipboard.writeText(id) but doesn't await it, so failures still
trigger toast.success; update DashboardSidebarProjectContextMenu's handleCopyId
to await the async write (e.g., use async/await) and wrap it in try/catch,
calling toast.success("Project ID copied") only on successful await and calling
toast.error(...) (or similar) in the catch with a helpful message; keep
references to id, navigator.clipboard.writeText, toast.success and toast.error
so the changes are easy to locate.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx`:
- Around line 69-74: The current computation of branches uses
Array.prototype.sort which mutates data.branches; change it to sort a shallow
copy instead so you don't mutate query data—e.g. create a copy from
data?.branches (via spread or slice) before calling sort in the expression that
assigns branches; keep the same sort comparator logic (referencing
defaultBranch, a.isLocal, b.isLocal and name.localeCompare) so behavior is
unchanged while avoiding in-place mutation.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/PromptGroup.tsx`:
- Around line 133-153: The keyboard submit path can start concurrent requests
because handleCreate() is invoked regardless of isPending; update handleCreate
to early-return when isPending is true (check the isPending state at the top of
handleCreate) so repeated ⌘/Ctrl+Enter presses do nothing while a request is in
flight, and also update the keyboard handler that calls handleCreate to
short-circuit when isPending to avoid invoking the function from the shortcut;
reference symbols: handleCreate, isPending, runAsyncAction, and createWorkspace.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx`:
- Around line 2-7: Replace the long relative import paths in
DashboardSidebarExpandedProjectContent.tsx with the configured tsconfig path
aliases: update the import for DashboardSidebarSection and
DashboardSidebarWorkspace (currently from "../../../../types") to use the
project alias defined in tsconfig.json, and likewise import
DashboardSidebarSectionComponent and DashboardSidebarWorkspaceItem via their
aliased module roots rather than "../../../DashboardSidebarSection" and
"../../../DashboardSidebarWorkspaceItem"; adjust the import statements so the
same exported symbols (DashboardSidebarSection, DashboardSidebarWorkspace,
DashboardSidebarSection as DashboardSidebarSectionComponent,
DashboardSidebarWorkspaceItem) are imported via aliases to improve readability
and maintainability.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsx`:
- Around line 11-17: The prop type for children in
DashboardSidebarProjectContextMenuProps is too broad (React.ReactNode) for the
use of ContextMenuTrigger with asChild; change the children type to
React.ReactElement (or React.ReactElement<any>) so the component requires
exactly one element to clone, and update the same change where this prop type is
defined/used (the DashboardSidebarProjectContextMenu component and the other
occurrence noted) to ensure callers pass a single React element rather than
strings, fragments, or arrays.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsx`:
- Around line 15-52: The forwardRef-wrapped component
DashboardSidebarCollapsedWorkspaceButton lacks a displayName which makes React
DevTools debugging harder; after the forwardRef declaration, set
DashboardSidebarCollapsedWorkspaceButton.displayName =
"DashboardSidebarCollapsedWorkspaceButton" so the component shows a readable
name in DevTools and stack traces (locate the forwardRef export for
DashboardSidebarCollapsedWorkspaceButton and add the displayName assignment
immediately after it).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`:
- Around line 147-178: The trailing "diff-stats + shortcut + close-button" JSX
is duplicated inside DashboardSidebarExpandedWorkspaceRow; extract it into a
small local component (e.g., DashboardSidebarTrailingActions) that accepts props
used by the block — additions, deletions (pass through to
DashboardSidebarWorkspaceDiffStats), shortcutLabel, isActive, and onDeleteClick
— and reuse that component in both layout branches where the block currently
appears (replace the inline cluster that references
DashboardSidebarWorkspaceDiffStats, shortcutLabel,
Tooltip/TooltipTrigger/TooltipContent and the HiMiniXMark button). Ensure the
new component preserves event.stopPropagation(), aria-label, classNames, and
Tooltip behavior so the UI and accessibility remain identical.
- Around line 73-77: Add a defensive target check in the
DashboardSidebarExpandedWorkspaceRow onKeyDown handler so it only reacts to key
events originating from the interactive row itself (not nested controls);
specifically, before handling Enter/Space and calling onClick, verify that
event.target === event.currentTarget (or otherwise ensure the event did not
originate from a nested control like the RenameInput) and return early if it did
— this complements RenameInput.handleKeyDown's stopPropagation and prevents
nested control interference.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsx`:
- Around line 30-37: The component DashboardSidebarWorkspaceHoverCardContent
contains hardcoded placeholder strings ("Updated a few minutes ago" and "Mocked
preview of the legacy workspace hover card"); update the component to accept
real data props (e.g., updatedAt/lastUpdated and previewContent or previewData)
and render the timestamp using a formatter (timeago/date-fns) or a prop-driven
formatted string instead of the fixed text, and render the preview area
conditionally (show real previewContent when available, otherwise show a
lightweight skeleton or a TODO comment/aria-hidden placeholder). Also add a
clear TODO comment or create an issue reference in the component indicating
where to wire real data, and ensure the element(s) with the hardcoded text are
replaced with prop-driven values in DashboardSidebarWorkspaceHoverCardContent.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx`:
- Around line 36-58: The className for the icons in
DashboardSidebarWorkspaceIcon contains a redundant conditional "variant ===
'expanded' && 'transition-colors'" because "transition-colors" is already
present in the base class string; remove the conditional from each icon JSX (the
Lu* elements where className is built) so className becomes cn("size-4
transition-colors", isActive ? "text-foreground" : "text-muted-foreground") to
eliminate noise while preserving behavior.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceStatusBadge/DashboardSidebarWorkspaceStatusBadge.tsx`:
- Line 4: The type name MockPrState indicates a placeholder; rename MockPrState
to a production-ready name (e.g., PrState) or replace it by importing the
canonical PR state type from the shared types/module used by the app, then
update all references (e.g., in DashboardSidebarWorkspaceStatusBadge and any
files importing MockPrState) to the new name or import, and update any exports,
type-only imports, and tests accordingly so the code compiles with real PR data.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/BranchesGroup/BranchesGroup.tsx`:
- Around line 14-15: Replace the deep relative imports in BranchesGroup.tsx for
useDashboardNewWorkspaceDraft and useCreateDashboardWorkspace with the project
alias import rooted at "renderer" to match the tsconfig alias; locate the two
import lines currently using "../../../../" and change them to import from the
equivalent "renderer/..." paths that mirror the same nested component locations
(e.g., renderer/routes/_authenticated/.../DashboardNewWorkspaceDraftContext and
renderer/routes/_authenticated/.../hooks/useCreateDashboardWorkspace) so the
module resolution uses the configured alias instead of deep relative traversal.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DashboardNewWorkspaceListTabContent/DashboardNewWorkspaceListTabContent.tsx`:
- Line 3: Replace the brittle relative import of DashboardNewWorkspaceTab
(currently imported via "../../../../DashboardNewWorkspaceDraftContext") with
the project alias used elsewhere in this file (e.g., the renderer/... alias) so
it matches the import style on Line 2; update the import statement that
references DashboardNewWorkspaceTab to use the renderer alias path to
DashboardNewWorkspaceDraftContext.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/IssuesGroup/IssuesGroup.tsx`:
- Around line 24-25: Replace the deep relative imports in IssuesGroup.tsx with
the configured tsconfig alias by importing useDashboardNewWorkspaceDraft and
useCreateDashboardWorkspace from the 'renderer/...' paths instead of
"../../../../DashboardNewWorkspaceDraftContext" and
"../../../../hooks/useCreateDashboardWorkspace"; update the import statements at
the top of IssuesGroup.tsx to use the alias so the component uses the project's
canonical renderer module paths.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PromptGroup/components/PromptGroupAdvancedOptions/PromptGroupAdvancedOptions.tsx`:
- Around line 92-93: Replace the native <label> element with the imported Label
component to keep styling consistent: in the PromptGroupAdvancedOptions
component swap the plain HTML <label htmlFor="branch" ...> usage with the
imported Label component (from `@superset/ui/label`) so that the "Branch name"
label uses the same styled Label as other labels in this file (e.g., the Label
used at line ~199).
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/PullRequestsGroup/PullRequestsGroup.tsx`:
- Around line 18-19: Replace the deep relative imports for the
DashboardNewWorkspaceDraft context and create-workspace hook with the project
alias form; specifically update the import statements that currently import
useDashboardNewWorkspaceDraft and useCreateDashboardWorkspace from
"../../../../DashboardNewWorkspaceDraftContext" and
"../../../../hooks/useCreateDashboardWorkspace" to use the 'renderer/...' path
alias that maps to the same modules (e.g., import useDashboardNewWorkspaceDraft
from "renderer/…/DashboardNewWorkspaceDraftContext" and import
useCreateDashboardWorkspace from "renderer/…/hooks/useCreateDashboardWorkspace")
so the component PullRequestsGroup uses aliased paths instead of deep relative
paths.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace/useCreateDashboardWorkspace.ts`:
- Line 11: Update the import of useWorkspaceHostOptions to use the project TS
alias instead of the relative path; locate the import statement that currently
references
"../../components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions"
and replace it with the alias-based import (e.g., import {
useWorkspaceHostOptions } from "renderer/..." matching your tsconfig paths) so
the useCreateDashboardWorkspace hook uses the aliased module.
Comment on lines
+38
to
+69
| <Tooltip delayDuration={300}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={handleWorkspacesClick} | ||
| className={cn( | ||
| "flex size-8 items-center justify-center rounded-md transition-colors", | ||
| isWorkspacesPageOpen | ||
| ? "bg-accent text-foreground" | ||
| : "text-muted-foreground hover:bg-accent/50 hover:text-foreground", | ||
| )} | ||
| > | ||
| <LuLayers className="size-4" strokeWidth={STROKE_WIDTH} /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="right">Workspaces</TooltipContent> | ||
| </Tooltip> | ||
|
|
||
| <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} /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="right"> | ||
| New Workspace ({shortcutText}) | ||
| </TooltipContent> | ||
| </Tooltip> |
Contributor
There was a problem hiding this comment.
Label the collapsed-header buttons.
In the collapsed state both actions are icon-only, so assistive tech will get unnamed buttons here.
🔧 Suggested fix
<button
type="button"
+ aria-label="Go to workspaces"
onClick={handleWorkspacesClick}
className={cn(
"flex size-8 items-center justify-center rounded-md transition-colors",
@@
<button
type="button"
+ aria-label="New workspace"
onClick={() => openModal()}
className="flex size-8 items-center justify-center rounded-md text-muted-foreground transition-colors hover:bg-accent/50 hover:text-foreground"
>📝 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
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={handleWorkspacesClick} | |
| className={cn( | |
| "flex size-8 items-center justify-center rounded-md transition-colors", | |
| isWorkspacesPageOpen | |
| ? "bg-accent text-foreground" | |
| : "text-muted-foreground hover:bg-accent/50 hover:text-foreground", | |
| )} | |
| > | |
| <LuLayers className="size-4" strokeWidth={STROKE_WIDTH} /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="right">Workspaces</TooltipContent> | |
| </Tooltip> | |
| <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} /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="right"> | |
| New Workspace ({shortcutText}) | |
| </TooltipContent> | |
| </Tooltip> | |
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| aria-label="Go to workspaces" | |
| onClick={handleWorkspacesClick} | |
| className={cn( | |
| "flex size-8 items-center justify-center rounded-md transition-colors", | |
| isWorkspacesPageOpen | |
| ? "bg-accent text-foreground" | |
| : "text-muted-foreground hover:bg-accent/50 hover:text-foreground", | |
| )} | |
| > | |
| <LuLayers className="size-4" strokeWidth={STROKE_WIDTH} /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="right">Workspaces</TooltipContent> | |
| </Tooltip> | |
| <Tooltip delayDuration={300}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| aria-label="New workspace" | |
| 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} /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="right"> | |
| New Workspace ({shortcutText}) | |
| </TooltipContent> | |
| </Tooltip> |
🤖 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 38 - 69, The two icon-only buttons in DashboardSidebarHeader (the
button that calls handleWorkspacesClick and the button that calls openModal)
lack accessible names; add explicit accessible labels by supplying either
aria-label attributes (e.g., aria-label="Open workspaces" and aria-label="New
workspace") or include visually-hidden text (screen-reader-only) inside the
buttons so assistive tech can announce them; ensure these labels match the
TooltipContent ("Workspaces" and "New Workspace") and keep TooltipTrigger/Tookit
behavior unchanged.
Comment on lines
+53
to
+65
| <button | ||
| type="button" | ||
| onClick={onToggleCollapse} | ||
| className={cn( | ||
| "flex items-center justify-center size-8 rounded-md", | ||
| "hover:bg-muted/50 transition-colors", | ||
| )} | ||
| > | ||
| <ProjectThumbnail | ||
| projectName={projectName} | ||
| githubOwner={githubOwner} | ||
| /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Expose the disclosure state on the project thumbnail button.
This button expands and collapses the workspace list, but it never announces the current state.
🔧 Suggested fix
<button
type="button"
+ aria-expanded={!isCollapsed}
onClick={onToggleCollapse}
className={cn(
"flex items-center justify-center size-8 rounded-md",
"hover:bg-muted/50 transition-colors",🤖 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/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx`
around lines 53 - 65, The project thumbnail button does not expose the
disclosure state to assistive tech; wire the button to the component's collapse
state by adding an aria-expanded attribute bound to the disclosure boolean used
in DashboardSidebarCollapsedProjectContent (e.g., isOpen/isCollapsed) and
include aria-controls pointing to the id of the workspace list element so screen
readers know what it toggles; ensure the toggle handler onToggleCollapse remains
and that ProjectThumbnail/button composition passes through ARIA attributes (or
update ProjectThumbnail to accept and forward them) so the state is announced.
Comment on lines
+55
to
+64
| onKeyDown={ | ||
| isRenaming | ||
| ? undefined | ||
| : (event) => { | ||
| if (event.key === "Enter" || event.key === " ") { | ||
| event.preventDefault(); | ||
| onToggleCollapse(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsxRepository: superset-sh/superset
Length of output: 5589
Scope the row keyboard handler to the row itself.
The onKeyDown handler at lines 55-64 processes bubbled events from nested buttons. Although these buttons use event.stopPropagation() on onClick, this does not prevent keydown event bubbling. When the pencil, plus, or chevron button has focus and Space or Enter is pressed, the parent's keyboard handler fires and calls preventDefault() and onToggleCollapse(), blocking the button's activation.
Suggested fix:
onKeyDown={
isRenaming
? undefined
: (event) => {
+ if (event.target !== event.currentTarget) {
+ return;
+ }
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
onToggleCollapse();
}
}
}📝 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
| onKeyDown={ | |
| isRenaming | |
| ? undefined | |
| : (event) => { | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onToggleCollapse(); | |
| } | |
| } | |
| } | |
| onKeyDown={ | |
| isRenaming | |
| ? undefined | |
| : (event) => { | |
| if (event.target !== event.currentTarget) { | |
| return; | |
| } | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onToggleCollapse(); | |
| } | |
| } | |
| } |
🤖 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 55 - 64, The row-level onKeyDown in DashboardSidebarProjectRow is
responding to keyboard events from nested controls (pencil/plus/chevron) and
wrongly calling onToggleCollapse; update the handler to ignore bubbled events by
verifying the event target is the row itself (e.g., check event.target ===
event.currentTarget or similar) before calling preventDefault() and
onToggleCollapse(), preserving the existing isRenaming gating; this ensures
buttons keep their native activation while the row keyboard shortcuts remain
functional.
Comment on lines
+88
to
+105
| <div className="grid shrink-0 items-center [&>*]:col-start-1 [&>*]:row-start-1"> | ||
| {!isRenaming && ( | ||
| <span className="text-xs font-normal tabular-nums text-muted-foreground transition-all duration-150 group-hover:scale-95 group-hover:opacity-0"> | ||
| ({totalWorkspaceCount}) | ||
| </span> | ||
| )} | ||
| {!isRenaming && ( | ||
| <button | ||
| type="button" | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onStartRename(); | ||
| }} | ||
| className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 hover:text-foreground" | ||
| aria-label="Rename project" | ||
| > | ||
| <LuPencil className="size-3.5 transition-transform duration-150 group-hover:rotate-[-8deg]" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Reveal the rename action on keyboard focus.
This control stays in the tab order while fully transparent, so keyboard users can land on an invisible button.
🔧 Suggested fix
<span className="text-xs font-normal tabular-nums text-muted-foreground transition-all duration-150 group-hover:scale-95 group-hover:opacity-0">
+ <span className="text-xs font-normal tabular-nums text-muted-foreground transition-all duration-150 group-hover:scale-95 group-hover:opacity-0 group-focus-within:scale-95 group-focus-within:opacity-0">
({totalWorkspaceCount})
</span>
)}
{!isRenaming && (
<button
@@
- className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 hover:text-foreground"
+ className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 group-focus-within:scale-100 group-focus-within:opacity-100 hover:text-foreground focus-visible:text-foreground"
aria-label="Rename project"
>📝 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="grid shrink-0 items-center [&>*]:col-start-1 [&>*]:row-start-1"> | |
| {!isRenaming && ( | |
| <span className="text-xs font-normal tabular-nums text-muted-foreground transition-all duration-150 group-hover:scale-95 group-hover:opacity-0"> | |
| ({totalWorkspaceCount}) | |
| </span> | |
| )} | |
| {!isRenaming && ( | |
| <button | |
| type="button" | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onStartRename(); | |
| }} | |
| className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 hover:text-foreground" | |
| aria-label="Rename project" | |
| > | |
| <LuPencil className="size-3.5 transition-transform duration-150 group-hover:rotate-[-8deg]" /> | |
| </button> | |
| <div className="grid shrink-0 items-center [&>*]:col-start-1 [&>*]:row-start-1"> | |
| {!isRenaming && ( | |
| <span className="text-xs font-normal tabular-nums text-muted-foreground transition-all duration-150 group-hover:scale-95 group-hover:opacity-0 group-focus-within:scale-95 group-focus-within:opacity-0"> | |
| ({totalWorkspaceCount}) | |
| </span> | |
| )} | |
| {!isRenaming && ( | |
| <button | |
| type="button" | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onStartRename(); | |
| }} | |
| className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 group-focus-within:scale-100 group-focus-within:opacity-100 hover:text-foreground focus-visible:text-foreground" | |
| aria-label="Rename project" | |
| > | |
| <LuPencil className="size-3.5 transition-transform duration-150 group-hover:rotate-[-8deg]" /> | |
| </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/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`
around lines 88 - 105, The rename button in DashboardSidebarProjectRow is
focusable while fully transparent, making it invisible to keyboard users; update
the button (the element that calls onStartRename and is rendered when isRenaming
is false and contains the LuPencil icon) to reveal itself on keyboard focus by
adding focus/focus-visible styles matching the hover reveal (e.g., include
focus-visible:opacity-100 and focus-visible:scale-100 or focus:opacity-100 and
focus:scale-100) so it becomes visible when focused, or alternatively remove it
from the tab order when intentionally hidden using tabIndex={-1}; ensure the
aria-label remains and that the change is applied to the same button element in
DashboardSidebarProjectRow.
Comment on lines
+110
to
+145
| <Tooltip delayDuration={500}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onNewWorkspace(); | ||
| }} | ||
| onContextMenu={(event) => event.stopPropagation()} | ||
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | ||
| > | ||
| <HiMiniPlus className="size-4 text-muted-foreground" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="bottom" sideOffset={4}> | ||
| New workspace | ||
| </TooltipContent> | ||
| </Tooltip> | ||
|
|
||
| <button | ||
| type="button" | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onToggleCollapse(); | ||
| }} | ||
| onContextMenu={(event) => event.stopPropagation()} | ||
| aria-expanded={!isCollapsed} | ||
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | ||
| > | ||
| <HiChevronRight | ||
| className={cn( | ||
| "size-3.5 text-muted-foreground transition-transform duration-150", | ||
| !isCollapsed && "rotate-90", | ||
| )} | ||
| /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Give the icon-only actions accessible names.
The plus and chevron buttons do not expose an accessible label. Tooltip text is not a reliable replacement here.
🔧 Suggested fix
<button
type="button"
+ aria-label="New workspace"
onClick={(event) => {
event.stopPropagation();
onNewWorkspace();
}}
@@
<button
type="button"
+ aria-label={isCollapsed ? "Expand project" : "Collapse project"}
onClick={(event) => {
event.stopPropagation();
onToggleCollapse();
}}📝 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
| <Tooltip delayDuration={500}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onNewWorkspace(); | |
| }} | |
| onContextMenu={(event) => event.stopPropagation()} | |
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | |
| > | |
| <HiMiniPlus className="size-4 text-muted-foreground" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={4}> | |
| New workspace | |
| </TooltipContent> | |
| </Tooltip> | |
| <button | |
| type="button" | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onToggleCollapse(); | |
| }} | |
| onContextMenu={(event) => event.stopPropagation()} | |
| aria-expanded={!isCollapsed} | |
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | |
| > | |
| <HiChevronRight | |
| className={cn( | |
| "size-3.5 text-muted-foreground transition-transform duration-150", | |
| !isCollapsed && "rotate-90", | |
| )} | |
| /> | |
| </button> | |
| <Tooltip delayDuration={500}> | |
| <TooltipTrigger asChild> | |
| <button | |
| type="button" | |
| aria-label="New workspace" | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onNewWorkspace(); | |
| }} | |
| onContextMenu={(event) => event.stopPropagation()} | |
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | |
| > | |
| <HiMiniPlus className="size-4 text-muted-foreground" /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent side="bottom" sideOffset={4}> | |
| New workspace | |
| </TooltipContent> | |
| </Tooltip> | |
| <button | |
| type="button" | |
| aria-label={isCollapsed ? "Expand project" : "Collapse project"} | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| onToggleCollapse(); | |
| }} | |
| onContextMenu={(event) => event.stopPropagation()} | |
| aria-expanded={!isCollapsed} | |
| className="p-1 rounded hover:bg-muted transition-colors shrink-0 ml-1" | |
| > | |
| <HiChevronRight | |
| className={cn( | |
| "size-3.5 text-muted-foreground transition-transform duration-150", | |
| !isCollapsed && "rotate-90", | |
| )} | |
| /> | |
| </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/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectRow/DashboardSidebarProjectRow.tsx`
around lines 110 - 145, The two icon-only buttons (the plus button that calls
onNewWorkspace and the chevron button that calls onToggleCollapse and reads
isCollapsed) lack accessible names; add explicit accessible labels (e.g.,
aria-label or aria-labelledby) to each button instead of relying on Tooltip
text—give the plus button a clear label like "Create new workspace" and the
chevron button a dynamic label that reflects state (e.g., "Expand projects" when
isCollapsed is true and "Collapse projects" when isCollapsed is false); keep
existing aria-expanded on the chevron and ensure event handlers (onNewWorkspace,
onToggleCollapse) remain unchanged.
Comment on lines
+62
to
+69
| const lastEnsuredWorkspaceIdRef = useRef<string | null>(null); | ||
|
|
||
| useEffect(() => { | ||
| if (!workspace || lastEnsuredWorkspaceIdRef.current === workspace.id) | ||
| return; | ||
| lastEnsuredWorkspaceIdRef.current = workspace.id; | ||
| ensureWorkspaceInSidebar(workspace.id, workspace.projectId); | ||
| }, [ensureWorkspaceInSidebar, workspace]); |
Contributor
There was a problem hiding this comment.
Reset the dedupe ref when the route has no active workspace.
Line 65 only compares the previous ensured id, and that ref is never cleared. Navigating workspace A -> /v2-workspace -> workspace A will skip ensureWorkspaceInSidebar() on the second visit, so the sidebar can stay out of sync when reopening the same workspace.
Suggested fix
- const lastEnsuredWorkspaceIdRef = useRef<string | null>(null);
+ const lastEnsuredWorkspaceKeyRef = useRef<string | null>(null);
useEffect(() => {
- if (!workspace || lastEnsuredWorkspaceIdRef.current === workspace.id)
- return;
- lastEnsuredWorkspaceIdRef.current = workspace.id;
+ if (!workspace) {
+ lastEnsuredWorkspaceKeyRef.current = null;
+ return;
+ }
+ const workspaceKey = `${workspace.id}:${workspace.projectId}`;
+ if (lastEnsuredWorkspaceKeyRef.current === workspaceKey) return;
+ lastEnsuredWorkspaceKeyRef.current = workspaceKey;
ensureWorkspaceInSidebar(workspace.id, workspace.projectId);
- }, [ensureWorkspaceInSidebar, workspace]);
+ }, [ensureWorkspaceInSidebar, workspace?.id, workspace?.projectId]);📝 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 lastEnsuredWorkspaceIdRef = useRef<string | null>(null); | |
| useEffect(() => { | |
| if (!workspace || lastEnsuredWorkspaceIdRef.current === workspace.id) | |
| return; | |
| lastEnsuredWorkspaceIdRef.current = workspace.id; | |
| ensureWorkspaceInSidebar(workspace.id, workspace.projectId); | |
| }, [ensureWorkspaceInSidebar, workspace]); | |
| const lastEnsuredWorkspaceKeyRef = useRef<string | null>(null); | |
| useEffect(() => { | |
| if (!workspace) { | |
| lastEnsuredWorkspaceKeyRef.current = null; | |
| return; | |
| } | |
| const workspaceKey = `${workspace.id}:${workspace.projectId}`; | |
| if (lastEnsuredWorkspaceKeyRef.current === workspaceKey) return; | |
| lastEnsuredWorkspaceKeyRef.current = workspaceKey; | |
| ensureWorkspaceInSidebar(workspace.id, workspace.projectId); | |
| }, [ensureWorkspaceInSidebar, workspace?.id, workspace?.projectId]); |
🤖 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/v2-workspace/layout.tsx`
around lines 62 - 69, The dedupe ref lastEnsuredWorkspaceIdRef is never cleared,
so when workspace becomes null the previous id remains and prevents
ensureWorkspaceInSidebar from running for the same workspace later; update the
useEffect that references lastEnsuredWorkspaceIdRef, workspace, and
ensureWorkspaceInSidebar to explicitly reset lastEnsuredWorkspaceIdRef.current =
null when workspace is falsy (before returning) and only perform the equality
check/ensure call when workspace exists, so the ref is cleared on route changes
with no active workspace.
Comment on lines
+173
to
+185
| const siblingRows = Array.from( | ||
| collections.v2SidebarWorkspaces.state.values(), | ||
| ).filter( | ||
| (item) => | ||
| item.projectId === projectId && | ||
| item.workspaceId !== workspaceId && | ||
| item.sectionId === sectionId, | ||
| ); | ||
|
|
||
| collections.v2SidebarWorkspaces.update(workspaceId, (draft) => { | ||
| draft.sectionId = sectionId; | ||
| draft.tabOrder = getNextTabOrder(siblingRows); | ||
| }); |
Contributor
There was a problem hiding this comment.
Top-level tabOrder calculation is inconsistent and can reorder rows incorrectly.
When moving workspaces to top-level (Line 173+) or deleting a section (Line 195+), nextOrder is computed from top-level workspaces only. But top-level ordering elsewhere includes both sections and top-level workspaces, so this can create collisions/unstable ordering between those row types.
Suggested fix
@@
- const siblingRows = Array.from(
- collections.v2SidebarWorkspaces.state.values(),
- ).filter(
- (item) =>
- item.projectId === projectId &&
- item.workspaceId !== workspaceId &&
- item.sectionId === sectionId,
- );
+ const siblingRows =
+ sectionId === null
+ ? [
+ ...Array.from(collections.v2SidebarWorkspaces.state.values()).filter(
+ (item) =>
+ item.projectId === projectId &&
+ item.workspaceId !== workspaceId &&
+ item.sectionId === null,
+ ),
+ ...Array.from(collections.v2SidebarSections.state.values()).filter(
+ (item) => item.projectId === projectId,
+ ),
+ ]
+ : Array.from(collections.v2SidebarWorkspaces.state.values()).filter(
+ (item) =>
+ item.projectId === projectId &&
+ item.workspaceId !== workspaceId &&
+ item.sectionId === sectionId,
+ );
@@
- const siblingTopLevelRows = Array.from(
- collections.v2SidebarWorkspaces.state.values(),
- ).filter(
- (item) =>
- item.projectId === section.projectId && item.sectionId === null,
- );
+ const siblingTopLevelRows = [
+ ...Array.from(collections.v2SidebarWorkspaces.state.values()).filter(
+ (item) =>
+ item.projectId === section.projectId && item.sectionId === null,
+ ),
+ ...Array.from(collections.v2SidebarSections.state.values()).filter(
+ (item) =>
+ item.projectId === section.projectId &&
+ item.sectionId !== sectionId,
+ ),
+ ];Also applies to: 195-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts`
around lines 173 - 185, The tabOrder computation uses only top-level workspaces
and misses other top-level row types, causing ordering collisions; update the
siblingRows filtering (the
Array.from(collections.v2SidebarWorkspaces.state.values()).filter(...) used
before calling getNextTabOrder) to include the full set of top-level rows used
elsewhere (e.g., include items with sectionId === null AND any section rows that
are considered top-level in your app, while still matching projectId and
excluding the moved workspaceId), then pass that complete set into
getNextTabOrder to compute draft.tabOrder; apply the same change to the
analogous code path used when deleting a section (the block around lines 195-213
that also calls getNextTabOrder) so both move-to-top-level and delete-section
use the identical top-level row set for ordering.
Comment on lines
+562
to
+587
| const v2SidebarProjects = createCollection( | ||
| localStorageCollectionOptions({ | ||
| id: `v2_sidebar_projects-${organizationId}`, | ||
| storageKey: `v2-sidebar-projects-${organizationId}`, | ||
| schema: dashboardSidebarProjectSchema, | ||
| getKey: (item) => item.projectId, | ||
| }), | ||
| ); | ||
|
|
||
| const v2SidebarWorkspaces = createCollection( | ||
| localStorageCollectionOptions({ | ||
| id: `v2_sidebar_workspaces-${organizationId}`, | ||
| storageKey: `v2-sidebar-workspaces-${organizationId}`, | ||
| schema: dashboardSidebarWorkspaceSchema, | ||
| getKey: (item) => item.workspaceId, | ||
| }), | ||
| ); | ||
|
|
||
| const v2SidebarSections = createCollection( | ||
| localStorageCollectionOptions({ | ||
| id: `v2_sidebar_sections-${organizationId}`, | ||
| storageKey: `v2-sidebar-sections-${organizationId}`, | ||
| schema: dashboardSidebarSectionSchema, | ||
| getKey: (item) => item.sectionId, | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | head -100Repository: superset-sh/superset
Length of output: 3836
🏁 Script executed:
wc -l apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsRepository: superset-sh/superset
Length of output: 160
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | sed -n '550,600p'Repository: superset-sh/superset
Length of output: 1606
🏁 Script executed:
# Check for logout/sign-out logic in the file
rg -n "logout|sign.out|clearStorage|removeItem" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for where these collections are created and how user context is accessed
ast-grep --pattern 'createCollection'Repository: superset-sh/superset
Length of output: 4350
🏁 Script executed:
# Check function signature and parameters
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | head -150Repository: superset-sh/superset
Length of output: 5384
🏁 Script executed:
# Look for the function that contains lines 562-587
sed -n '110,180p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | cat -nRepository: superset-sh/superset
Length of output: 2172
🏁 Script executed:
# Search for where this module is used and how user context is passed
rg -n "createOrgCollections\|import.*collections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/ -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check for logout or sign-out clearing logic in the CollectionsProvider
rg -n "logout|sign.out|clear\|remove\|destroy" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/ -iRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for user context in the provider file
rg -n "userId\|user\|session" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -i | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for the CollectionsProvider component
find apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider -type f -name "*.tsx" -o -name "*.ts"Repository: superset-sh/superset
Length of output: 564
🏁 Script executed:
# Check the parent file that uses the collections
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/index.tsx 2>/dev/null | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Find the main export and see how createOrgCollections is used
sed -n '150,250p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | cat -nRepository: superset-sh/superset
Length of output: 3163
🏁 Script executed:
# Look for where collections cache is cleared or how multiple users are handled
sed -n '600,674p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | cat -nRepository: superset-sh/superset
Length of output: 2716
🏁 Script executed:
# Search for sign-out/logout in the entire renderer directory
rg -l "signOut|logout|clearAuth" apps/desktop/src/renderer --type ts --type tsx | head -10Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for sign-out/logout in the renderer directory
rg -l "signOut|logout|clearAuth" apps/desktop/src/renderer --type tsRepository: superset-sh/superset
Length of output: 535
🏁 Script executed:
# Check the CollectionsProvider component for how it handles user changes
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxRepository: superset-sh/superset
Length of output: 3347
🏁 Script executed:
# Look for how the auth state is managed and what happens on logout
rg "signOut\|logout\|clearAuth" apps/desktop/src/renderer -A 3 -B 3 | head -50Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's user context or session management that might clear collections
rg "userId\|user\|session" apps/desktop/src/renderer/routes/_authenticated -i -l | head -10Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check AuthProvider for logout handling
cat -n apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx | head -150Repository: superset-sh/superset
Length of output: 3800
🏁 Script executed:
# Look for where collections might be cleared on logout
rg -n "clear\|destroy\|remove" apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx -iRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the dashboardSidebarLocal schema to see what data it stores
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tsRepository: superset-sh/superset
Length of output: 1567
🏁 Script executed:
# Search for where these collections are used to understand the impact
rg "v2SidebarProjects\|v2SidebarWorkspaces\|v2SidebarSections" apps/desktop/src/renderer -A 2 -B 2 | head -100Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's a user ID available in the session
rg -n "session\|user" apps/desktop/src/renderer/lib/auth-client.ts -A 3 | head -80Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the session type definition
fd -e ts -e tsx auth-client | xargs cat | grep -A 20 "useSession\|session\?"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the collections are actually not cleared on logout
rg -n "collectionsCache" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/Repository: superset-sh/superset
Length of output: 631
🏁 Script executed:
# Check if there's any cleanup on logout in the entire authentication flow
rg -n "signOut" apps/desktop/src/renderer -A 10 -B 2 | grep -A 10 -B 2 "cache\|clear\|destroy"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the full createOrgCollections function signature and whether userId is passed
sed -n '150,200p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | cat -nRepository: superset-sh/superset
Length of output: 1678
🏁 Script executed:
# Trace how getCollections and createOrgCollections are called
rg -n "getCollections\|createOrgCollections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's userId available in the session
cat -n apps/desktop/src/renderer/lib/auth-client.ts | head -150Repository: superset-sh/superset
Length of output: 1918
🏁 Script executed:
# Check if logout clears the collections cache
rg -n "signOut\|logout" apps/desktop/src/renderer/providers/AuthProvider/AuthProvider.tsx -A 5 -B 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the actual behavior: when switching users in same org, are collections cleared?
sed -n '38,85p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx | cat -nRepository: superset-sh/superset
Length of output: 1748
🏁 Script executed:
# Check if localStorage is cleared anywhere on logout
rg -n "localStorage\|sessionStorage\|clear" apps/desktop/src/renderer/providers/AuthProvider/ -iRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if there's any cleanup when activeOrganizationId or session changes
sed -n '27,46p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxRepository: superset-sh/superset
Length of output: 705
🏁 Script executed:
# Verify: can two different users be in the same org simultaneously in the app?
rg -n "activeOrganizationId\|session\.user" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Final check: verify the collections are truly tied only to org, not user
sed -n '114,125p' apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsRepository: superset-sh/superset
Length of output: 369
🏁 Script executed:
# Check if there's a logout handler anywhere that might clear data
rg -rn "signOut" apps/desktop/src/renderer/providers/ --type tsRepository: superset-sh/superset
Length of output: 189
🏁 Script executed:
# Verify: when logout happens, does the app completely unmount CollectionsProvider?
rg -n "CollectionsProvider" apps/desktop/src/renderer -B 3 -A 3 --type tsx | head -50Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Check the complete flow: when logout happens, does activeOrganizationId get cleared?
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsx | grep -A 20 "useEffect"Repository: superset-sh/superset
Length of output: 1556
🏁 Script executed:
# Final verification: collections are stored in a global Map that persists across user sessions
grep -n "collectionsCache\|Map" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | head -5Repository: superset-sh/superset
Length of output: 273
🏁 Script executed:
# Check if the user ID is even available anywhere in the collections system
rg -n "userId\|user\.id" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/ -iRepository: superset-sh/superset
Length of output: 46
Add user ID to sidebar collection storage keys and cache keys.
The storage keys for v2SidebarProjects, v2SidebarWorkspaces, and v2SidebarSections (lines 565, 574, 583) include only organizationId but not the signed-in user. These collections store per-user UI preferences (isCollapsed, tabOrder, name, color). When two users access the same organization on the same app profile, the second user will load the first user's sidebar state from localStorage. Additionally, the global collections cache is keyed only by organization, never cleared on logout, and does not include a user dimension.
Include userId in both the storage key and the collections cache key to properly scope these preferences per user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`
around lines 562 - 587, The v2 sidebar collections (v2SidebarProjects,
v2SidebarWorkspaces, v2SidebarSections) are currently keyed only by
organizationId so UI prefs leak between users; update each
localStorageCollectionOptions call to include the signed-in userId in both the
id and storageKey (e.g., `...-${organizationId}-${userId}`) so the per-user
cache key and localStorage key are unique, ensuring caches created by
createCollection are also scoped by user; use the same userId variable available
in this module when constructing the id and storageKey for each collection.
Comment on lines
+3
to
+5
| const persistedDateSchema = z | ||
| .union([z.string(), z.date()]) | ||
| .transform((value) => (typeof value === "string" ? new Date(value) : value)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "schema.ts" | grep -i dashboardsidebarlocalRepository: superset-sh/superset
Length of output: 175
🏁 Script executed:
git ls-files | grep -E "apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal"Repository: superset-sh/superset
Length of output: 282
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts | head -50Repository: superset-sh/superset
Length of output: 1567
🏁 Script executed:
node <<'EOF'
const { z } = require("zod");
// Test the current schema
const persistedDateSchema = z
.union([z.string(), z.date()])
.transform((value) => (typeof value === "string" ? new Date(value) : value));
// Test with invalid string
console.log("=== Testing current schema ===");
try {
const result = persistedDateSchema.parse("invalid-date-string");
console.log("Result:", result);
console.log("Result type:", result.constructor.name);
console.log("Result.getTime():", result.getTime());
console.log("Is Invalid Date?", isNaN(result.getTime()));
console.log("PASSED validation (but is Invalid Date)");
} catch (e) {
console.log("FAILED validation:", e.message);
}
// Test with valid date string
console.log("\n=== Testing with valid ISO date ===");
try {
const result = persistedDateSchema.parse("2024-01-15T10:00:00Z");
console.log("Result:", result);
console.log("Result.getTime():", result.getTime());
console.log("Is Invalid Date?", isNaN(result.getTime()));
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation:", e.message);
}
// Test the proposed fix
const proposedSchema = z
.union([z.string(), z.date()])
.transform((value) => (typeof value === "string" ? new Date(value) : value))
.pipe(z.date());
console.log("\n=== Testing proposed fix with invalid string ===");
try {
const result = proposedSchema.parse("invalid-date-string");
console.log("Result:", result);
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation (as expected):", e.message);
}
console.log("\n=== Testing proposed fix with valid date string ===");
try {
const result = proposedSchema.parse("2024-01-15T10:00:00Z");
console.log("Result:", result);
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation:", e.message);
}
EOFRepository: superset-sh/superset
Length of output: 897
🏁 Script executed:
cd apps/desktop && npm list zod 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 116
🏁 Script executed:
node <<'EOF'
const { z } = require("zod");
// Test the current schema
const persistedDateSchema = z
.union([z.string(), z.date()])
.transform((value) => (typeof value === "string" ? new Date(value) : value));
// Test with invalid string
console.log("=== Testing current schema ===");
try {
const result = persistedDateSchema.parse("invalid-date-string");
console.log("Result:", result);
console.log("Result type:", result.constructor.name);
console.log("Result.getTime():", result.getTime());
console.log("Is Invalid Date?", isNaN(result.getTime()));
console.log("PASSED validation (but is Invalid Date)");
} catch (e) {
console.log("FAILED validation:", e.message);
}
// Test with valid date string
console.log("\n=== Testing with valid ISO date ===");
try {
const result = persistedDateSchema.parse("2024-01-15T10:00:00Z");
console.log("Result:", result);
console.log("Result.getTime():", result.getTime());
console.log("Is Invalid Date?", isNaN(result.getTime()));
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation:", e.message);
}
// Test the proposed fix
const proposedSchema = z
.union([z.string(), z.date()])
.transform((value) => (typeof value === "string" ? new Date(value) : value))
.pipe(z.date());
console.log("\n=== Testing proposed fix with invalid string ===");
try {
const result = proposedSchema.parse("invalid-date-string");
console.log("Result:", result);
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation (as expected):", e.message);
}
console.log("\n=== Testing proposed fix with valid date string ===");
try {
const result = proposedSchema.parse("2024-01-15T10:00:00Z");
console.log("Result:", result);
console.log("PASSED validation");
} catch (e) {
console.log("FAILED validation:", e.message);
}
EOFRepository: superset-sh/superset
Length of output: 897
🏁 Script executed:
web_search
Zod z.transform() validation behavior Invalid DateRepository: superset-sh/superset
Length of output: 250
🏁 Script executed:
web_search
Zod pipe z.date() validation reject Invalid DateRepository: superset-sh/superset
Length of output: 248
🌐 Web query:
Zod z.transform() validation behavior with Invalid Date objects
💡 Result:
What happens with Invalid Date and .transform()
z.date()rejects invalidDateinstances (i.e.,new Date("nope"), whose time value isNaN) and will fail parsing rather than returning theDate. This is the intent of the “Dates” primitive. [1].transform()does not automatically “re-validate” what you return. So if you doz.string().transform(s => new Date(s)), Zod can succeed even when the transform returns anInvalid Date—unless you explicitly add an issue in the transform. Zod’s docs explicitly note that transforms “accept anything”, and that validation inside transforms should be done viactx.issues(and that thrown errors are not caught). [2]- If you’re on Zod v4, note a related gotcha: a chained
.transform()can still execute even when a prior.refine()fails, which can surface runtime errors unless you guard inside the transform. [3]
How to ensure “no Invalid Date escapes”
Option A (recommended): validate inside the transform
const stringToValidDate = z.string().transform((s, ctx) => {
const d = new Date(s);
if (!z.date().safeParse(d).success) {
ctx.addIssue({ code: z.ZodIssueCode.invalid_date });
return z.NEVER;
}
return d;
});This pattern is commonly suggested for date parsing so that invalid strings don’t turn into “validated” Invalid Date objects. [4]
Option B: use coercion/piping (when it matches your input shape)
If your input is already something z.coerce.date() can handle, prefer that over a manual new Date() transform, because it bakes the “date validity” check into parsing. (See the discussion’s z.coerce.date() / piping examples.) [4]
Sources:
[1] Zod docs — Dates (z.date()) https://zod.dev/api?id=dates (zod.dev)
[2] Zod docs — Transforms (ctx.issues, “accept anything”, “thrown errors are not caught”) https://zod.dev/api?id=preprocess (zod.dev)
[3] GitHub issue: “Zod v4: transform() runs even when .refine() fails” colinhacks/zod#4926 (github.com)
[4] GitHub discussion: “Parsing a date” (validate inside transform using z.date().safeParse + invalid_date, plus z.coerce.date() examples) colinhacks/zod#879 (github.com)
Citations:
- 1: https://zod.dev/?id=dates-1
- 2: https://zod.dev/api?id=preprocess&utm_source=openai
- 3: Zod v4: transform() runs even when .refine() fails, causing unexpected runtime errors colinhacks/zod#4926
- 4: Parsing a date colinhacks/zod#879
🌐 Web query:
Zod pipe z.date() validation Invalid Date rejection
💡 Result:
To have Zod reject JavaScript Invalid Date values when using .pipe(), make sure the value you pass into z.date() is a Date and that Zod is the one validating it (not just instanceof Date).
Recommended pattern (string → Date, then validate)
import { z } from "zod";
const schema = z.string().pipe(z.coerce.date()); // rejects "invalid-date" with code: "invalid_date"This is a common/simple way to accept a date string and ensure invalid strings don’t sneak through. [1]
If you need a custom “invalid date” message
z.coerce.date() (and z.date()) produce the invalid_date issue; customize it with errorMap:
const schema = z.coerce.date({
errorMap: ({ code }, { defaultError }) => ({
message: code === "invalid_date" ? "Please enter a valid date." : defaultError,
}),
});This is the standard workaround for customizing the “Invalid date” error. [2][3]
Notes
z.date()already rejectsInvalid Date(it’s not enough for a value to merely be aDateinstance). [4]- If you’re doing your own transform/preprocess that returns
new Date(x), piping intoz.date()should be what enforces the rejection—if you instead validate withz.instanceof(Date),Invalid Datewill incorrectly pass.
Sources:
[1] notes.zachmanson.com – Zod Datestrings
[2] GitHub Discussions – customize error for z.coerce.date() via errorMap
[3] Stack Overflow – override errorMap for invalid_date
[4] w3tutorials – z.date() validates Date instance and not Invalid Date
🏁 Script executed:
rg -A 5 "dashboardSidebarProjectSchema|dashboardSidebarWorkspaceSchema|dashboardSidebarSectionSchema" apps/desktop/src/renderer --type ts --type tsx | head -100Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg "persistedDateSchema" apps/desktop/src --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg "dashboardSidebarProjectSchema|dashboardSidebarWorkspaceSchema|dashboardSidebarSectionSchema" apps/desktop/src --type ts | head -100Repository: superset-sh/superset
Length of output: 2619
🏁 Script executed:
rg "persistedDateSchema" apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 632
🏁 Script executed:
rg "dashboardSidebarLocal" apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 190
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | head -150Repository: superset-sh/superset
Length of output: 5384
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | sed -n '150,300p'Repository: superset-sh/superset
Length of output: 4770
🏁 Script executed:
cat -n apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | sed -n '350,450p'Repository: superset-sh/superset
Length of output: 2918
🏁 Script executed:
rg "v2SidebarProjects|v2SidebarWorkspaces|v2SidebarSections" apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts -A 15Repository: superset-sh/superset
Length of output: 2235
Add validation to reject Invalid Date objects in persisted rows.
On line 5, the transform can produce an Invalid Date object (from malformed strings like "invalid"), which passes schema validation because .transform() does not re-validate its output. This allows corrupted localStorage entries to pass the boundary and enter the app. Pipe the result through z.date() to reject Invalid Date values.
Proposed fix
const persistedDateSchema = z
.union([z.string(), z.date()])
- .transform((value) => (typeof value === "string" ? new Date(value) : value));
+ .transform((value) => (typeof value === "string" ? new Date(value) : value))
+ .pipe(z.date());📝 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 persistedDateSchema = z | |
| .union([z.string(), z.date()]) | |
| .transform((value) => (typeof value === "string" ? new Date(value) : value)); | |
| const persistedDateSchema = z | |
| .union([z.string(), z.date()]) | |
| .transform((value) => (typeof value === "string" ? new Date(value) : value)) | |
| .pipe(z.date()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`
around lines 3 - 5, The persistedDateSchema currently transforms strings to Date
objects but doesn't re-validate the transformed value, allowing Invalid Date
instances through; update the schema for persistedDateSchema (the
z.union([...]).transform(...) chain) to pipe the transformed result into
z.date() (e.g., add .pipe(z.date())) so malformed strings that produce Invalid
Date are rejected by the schema validation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx (3)
96-105:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the chevron toggle button.
aria-expandedalone does not provide a label for this icon-only control.💡 Suggested fix
<button type="button" + aria-label={ + section.isCollapsed + ? `Expand section ${section.name}` + : `Collapse section ${section.name}` + } onClick={(event) => { event.stopPropagation(); onToggleCollapse(); }}🤖 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/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx` around lines 96 - 105, The chevron toggle button in the DashboardSidebarSectionHeader component lacks an accessible name; update the button (in DashboardSidebarSectionHeader) to include a clear accessible label such as aria-label or aria-labelledby that reflects the section title and action (e.g., "Expand {section.title}" / "Collapse {section.title}" based on section.isCollapsed) so screen readers can describe the control, and keep existing aria-expanded behavior unchanged.
77-88:⚠️ Potential issue | 🟠 MajorExpose rename affordance on focus, not only hover.
The control is effectively hidden for keyboard users until pointer hover; add focus/focus-within visibility states.
💡 Suggested fix
- <span className="text-[10px] font-normal tabular-nums transition-all duration-150 group-hover:scale-95 group-hover:opacity-0"> + <span className="text-[10px] font-normal tabular-nums transition-all duration-150 group-hover:scale-95 group-hover:opacity-0 group-focus-within:scale-95 group-focus-within:opacity-0"> ({section.workspaces.length}) </span> <button type="button" @@ - className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 hover:text-foreground" + className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 group-focus-within:scale-100 group-focus-within:opacity-100 focus-visible:scale-100 focus-visible:opacity-100 hover:text-foreground" aria-label="Rename section" >🤖 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/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx` around lines 77 - 88, The rename button in DashboardSidebarSectionHeader is only revealed on pointer hover, which hides the affordance from keyboard users; update the relevant JSX (the span showing count and the rename button with className in DashboardSidebarSectionHeader) to include focus and focus-within variants (e.g., add group-focus, focus, and focus-within equivalents alongside group-hover) so the button becomes visible and the count fades/animates when the section or button receives keyboard focus, and ensure the onStartRename handler is still triggered via keyboard activation.
46-55:⚠️ Potential issue | 🟠 MajorPrevent parent toggle from handling bubbled key events from nested controls.
Line 50 currently toggles collapse even when Enter/Space originates from a child button, so focusing the rename/chevron controls can still collapse the section.
💡 Suggested fix
onKeyDown={ isRenaming ? undefined : (event) => { + if (event.target !== event.currentTarget) return; if (event.key === "Enter" || event.key === " ") { event.preventDefault(); onToggleCollapse(); } } }🤖 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/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx` around lines 46 - 55, The onKeyDown handler in DashboardSidebarSectionHeader currently toggles collapse for Enter/Space even when the key event bubbles up from child controls (like rename or chevron buttons); update the handler in DashboardSidebarSectionHeader so that before calling onToggleCollapse it checks that the event originated on the element itself (e.g., if (event.target !== event.currentTarget) return) so bubbled events from nested controls are ignored, preserving isRenaming behavior and only toggling when the section header itself is focused.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx (1)
51-63:⚠️ Potential issue | 🟡 MinorExpose disclosure state/target on the project toggle button.
The toggle control should announce current expanded/collapsed state and what region it controls.
💡 Suggested fix
<button type="button" + aria-expanded={!isCollapsed} + aria-controls={`project-${projectId}-workspaces`} onClick={onToggleCollapse} className={cn( "flex items-center justify-center size-8 rounded-md", "hover:bg-muted/50 transition-colors", )} > @@ <motion.div + id={`project-${projectId}-workspaces`} initial={{ height: 0, opacity: 0 }} animate={{ height: "auto", opacity: 1 }} exit={{ height: 0, opacity: 0 }}Also applies to: 74-83
🤖 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/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx` around lines 51 - 63, The project toggle button currently lacks accessible disclosure attributes; update the button rendered with onToggleCollapse (and the analogous button at the other block) to include aria-expanded (set from the project's collapsed/expanded state prop) and aria-controls pointing to the id of the collapsible region it toggles (assign a unique id to the collapsible content section rendered by DashboardSidebarCollapsedProjectContent or its parent). Ensure the aria-controls value matches the region id and that the button also has an accessible name (e.g., via aria-label or visible text) so screen readers announce the control and its expanded/collapsed state.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx (1)
59-75:⚠️ Potential issue | 🟠 MajorCollapsed-mode Rename still has no visible rename flow.
At Line 73,
onRename={startRename}is still wired in the collapsed branch, but there is no rename UI rendered in that branch, so the action appears broken.🤖 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx` around lines 59 - 75, The collapsed-mode branch wires the rename handler but provides no visible rename UI, so either stop passing onRename into DashboardSidebarWorkspaceContextMenu when in collapsed mode or add the rename UI there; locate the collapsed branch where DashboardSidebarWorkspaceContextMenu is rendered with hoverCardContent (and the props like projectId, onCreateSection, onMoveToSection, onRemoveFromSidebar, onRename={startRename}, onDelete) and update it to conditionally omit the onRename={startRename} prop when collapsed, or implement the appropriate rename UI activation inside DashboardSidebarWorkspaceContextMenu for collapsed state so startRename is actionable.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsx (1)
9-13: Trim unusedallSectionsfrom this component API.Line 12 defines
allSections, but this component does not use it. Removing it keeps the new sidebar API tighter and easier to evolve.Also applies to: 19-26
🤖 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/DashboardSidebarSection/DashboardSidebarSection.tsx` around lines 9 - 13, Remove the unused allSections prop from the DashboardSidebarSectionProps interface and from the DashboardSidebarSection component signature/props destructuring (symbols: DashboardSidebarSectionProps, DashboardSidebarSection, allSections); update any places that pass allSections into DashboardSidebarSection to stop providing that prop (or remove it from spread props), and run TS checks to ensure no remaining references to allSections remain in this component or its callers.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx (1)
97-104: Deduplicate the delete dialog render path.The delete dialog block is repeated in both branches with identical props. Hoisting a single dialog render below the branch will reduce maintenance churn.
Also applies to: 147-154
🤖 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/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx` around lines 97 - 104, The DashboardSidebarDeleteDialog is rendered twice with identical props; remove the duplicate instances and render a single DashboardSidebarDeleteDialog once (hoisted outside the conditional branches) using the same props: open={isDeleteDialogOpen}, onOpenChange={setIsDeleteDialogOpen}, onConfirm={handleDelete}, title={`Delete "${name || branch}"?`}, description="This will permanently delete the workspace.", and isPending={isDeleting}; ensure you remove both duplicated render paths (the other occurrence around the second branch) and keep the shared state/handlers (isDeleteDialogOpen, setIsDeleteDialogOpen, handleDelete, name, branch, isDeleting) in scope for the hoisted dialog.
🤖 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/DashboardSidebarProjectSection.tsx`:
- Around line 64-73: The context menu is calling startRename for collapsed
projects but the collapsed UI (DashboardSidebarCollapsedProjectContent) has no
rename controls, so wire the rename action to a visible UX or remove it in
collapsed mode: update DashboardSidebarProjectContextMenu usage to conditionally
pass onRename={isCollapsed ? openRenameDialogForProject : startRename} (or
remove the prop when collapsed), implement/open a rename modal/helper (e.g.,
openRenameDialogForProject(project.id)) that shows the rename input when
collapsed, and ensure DashboardSidebarCollapsedProjectContent and
DashboardSidebarProjectContextMenu cooperate so startRename only targets the
expanded row UI or triggers the global rename dialog.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsx`:
- Around line 55-56: The context-menu rename opens with a stale input because
onRename only calls setIsRenaming(true) and doesn't reset renameValue to the
current section.name; update the onRename handler used in the context menu to
first setRenameValue(section.name) (or the equivalent state setter used for the
rename input) and then call setIsRenaming(true) so the rename dialog/input
always shows the latest section.name (mirror the header flow that sets
renameValue before enabling renaming).
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 155-160: The code in useDashboardSidebarData is skipping
workspaces when a section record is missing because it unconditionally continues
after attempting to push to
project.sectionMap.get(workspace.sectionId)?.workspaces.push(sidebarWorkspace);
change the logic so you only continue when the section lookup succeeds; if
project.sectionMap.get(workspace.sectionId) is undefined, fall back to adding
sidebarWorkspace to the project's top-level collection (e.g., project.workspaces
or project.children) instead of continuing, ensuring workspace.sectionId,
project.sectionMap, and workspaces.push(sidebarWorkspace) are used to detect
success and perform the fallback.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx`:
- Around line 51-63: The project toggle button currently lacks accessible
disclosure attributes; update the button rendered with onToggleCollapse (and the
analogous button at the other block) to include aria-expanded (set from the
project's collapsed/expanded state prop) and aria-controls pointing to the id of
the collapsible region it toggles (assign a unique id to the collapsible content
section rendered by DashboardSidebarCollapsedProjectContent or its parent).
Ensure the aria-controls value matches the region id and that the button also
has an accessible name (e.g., via aria-label or visible text) so screen readers
announce the control and its expanded/collapsed state.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx`:
- Around line 96-105: The chevron toggle button in the
DashboardSidebarSectionHeader component lacks an accessible name; update the
button (in DashboardSidebarSectionHeader) to include a clear accessible label
such as aria-label or aria-labelledby that reflects the section title and action
(e.g., "Expand {section.title}" / "Collapse {section.title}" based on
section.isCollapsed) so screen readers can describe the control, and keep
existing aria-expanded behavior unchanged.
- Around line 77-88: The rename button in DashboardSidebarSectionHeader is only
revealed on pointer hover, which hides the affordance from keyboard users;
update the relevant JSX (the span showing count and the rename button with
className in DashboardSidebarSectionHeader) to include focus and focus-within
variants (e.g., add group-focus, focus, and focus-within equivalents alongside
group-hover) so the button becomes visible and the count fades/animates when the
section or button receives keyboard focus, and ensure the onStartRename handler
is still triggered via keyboard activation.
- Around line 46-55: The onKeyDown handler in DashboardSidebarSectionHeader
currently toggles collapse for Enter/Space even when the key event bubbles up
from child controls (like rename or chevron buttons); update the handler in
DashboardSidebarSectionHeader so that before calling onToggleCollapse it checks
that the event originated on the element itself (e.g., if (event.target !==
event.currentTarget) return) so bubbled events from nested controls are ignored,
preserving isRenaming behavior and only toggling when the section header itself
is focused.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 59-75: The collapsed-mode branch wires the rename handler but
provides no visible rename UI, so either stop passing onRename into
DashboardSidebarWorkspaceContextMenu when in collapsed mode or add the rename UI
there; locate the collapsed branch where DashboardSidebarWorkspaceContextMenu is
rendered with hoverCardContent (and the props like projectId, onCreateSection,
onMoveToSection, onRemoveFromSidebar, onRename={startRename}, onDelete) and
update it to conditionally omit the onRename={startRename} prop when collapsed,
or implement the appropriate rename UI activation inside
DashboardSidebarWorkspaceContextMenu for collapsed state so startRename is
actionable.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsx`:
- Around line 9-13: Remove the unused allSections prop from the
DashboardSidebarSectionProps interface and from the DashboardSidebarSection
component signature/props destructuring (symbols: DashboardSidebarSectionProps,
DashboardSidebarSection, allSections); update any places that pass allSections
into DashboardSidebarSection to stop providing that prop (or remove it from
spread props), and run TS checks to ensure no remaining references to
allSections remain in this component or its callers.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 97-104: The DashboardSidebarDeleteDialog is rendered twice with
identical props; remove the duplicate instances and render a single
DashboardSidebarDeleteDialog once (hoisted outside the conditional branches)
using the same props: open={isDeleteDialogOpen},
onOpenChange={setIsDeleteDialogOpen}, onConfirm={handleDelete}, title={`Delete
"${name || branch}"?`}, description="This will permanently delete the
workspace.", and isPending={isDeleting}; ensure you remove both duplicated
render paths (the other occurrence around the second branch) and keep the shared
state/handlers (isDeleteDialogOpen, setIsDeleteDialogOpen, handleDelete, name,
branch, isDeleting) in scope for the hoisted dialog.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2551799-f1a1-45db-8a16-30475a5fd6ba
📒 Files selected for processing (15)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/DashboardSidebarSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/projectChildren/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/utils/projectChildren/projectChildren.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.ts
Comment on lines
+64
to
+73
| <DashboardSidebarProjectContextMenu | ||
| id={project.id} | ||
| onCreateSection={handleNewSection} | ||
| onRemoveFromSidebar={() => removeProjectFromSidebar(project.id)} | ||
| onRename={startRename} | ||
| onDelete={() => setIsDeleteDialogOpen(true)} | ||
| > | ||
| <div className={cn("border-b border-border last:border-b-0")}> | ||
| <DashboardSidebarCollapsedProjectContent | ||
| projectId={project.id} |
Contributor
There was a problem hiding this comment.
Collapsed-mode project Rename appears disconnected from any visible editor.
Line 68 triggers startRename, but the collapsed branch does not render rename controls (those are only in the expanded row). Please route this action to an actual rename UX in collapsed mode (or hide/replace the action there).
🤖 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/DashboardSidebarProjectSection.tsx`
around lines 64 - 73, The context menu is calling startRename for collapsed
projects but the collapsed UI (DashboardSidebarCollapsedProjectContent) has no
rename controls, so wire the rename action to a visible UX or remove it in
collapsed mode: update DashboardSidebarProjectContextMenu usage to conditionally
pass onRename={isCollapsed ? openRenameDialogForProject : startRename} (or
remove the prop when collapsed), implement/open a rename modal/helper (e.g.,
openRenameDialogForProject(project.id)) that shows the rename input when
collapsed, and ensure DashboardSidebarCollapsedProjectContent and
DashboardSidebarProjectContextMenu cooperate so startRename only targets the
expanded row UI or triggers the global rename dialog.
Comment on lines
+55
to
+56
| onRename={() => setIsRenaming(true)} | ||
| onSetColor={(color) => setSectionColor(section.id, color)} |
Contributor
There was a problem hiding this comment.
Context-menu rename can open with stale input value.
At Line 55, rename starts without resetting renameValue to the latest section.name, unlike the header flow at Lines 66-69. This can show outdated text in the rename input.
💡 Proposed fix
- onRename={() => setIsRenaming(true)}
+ onRename={() => {
+ setRenameValue(section.name);
+ setIsRenaming(true);
+ }}📝 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
| onRename={() => setIsRenaming(true)} | |
| onSetColor={(color) => setSectionColor(section.id, color)} | |
| onRename={() => { | |
| setRenameValue(section.name); | |
| setIsRenaming(true); | |
| }} | |
| onSetColor={(color) => setSectionColor(section.id, color)} |
🤖 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/DashboardSidebarSection/DashboardSidebarSection.tsx`
around lines 55 - 56, The context-menu rename opens with a stale input because
onRename only calls setIsRenaming(true) and doesn't reset renameValue to the
current section.name; update the onRename handler used in the context menu to
first setRenameValue(section.name) (or the equivalent state setter used for the
rename input) and then call setIsRenaming(true) so the rename dialog/input
always shows the latest section.name (mirror the header flow that sets
renameValue before enabling renaming).
Comment on lines
+155
to
+160
| if (workspace.sectionId) { | ||
| project.sectionMap | ||
| .get(workspace.sectionId) | ||
| ?.workspaces.push(sidebarWorkspace); | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t drop section-assigned workspaces when the section record is missing.
If Line 156 lookup fails, Line 159 still continues, so that workspace disappears from UI entirely instead of falling back to top-level children.
💡 Suggested fix
if (workspace.sectionId) {
- project.sectionMap
- .get(workspace.sectionId)
- ?.workspaces.push(sidebarWorkspace);
- continue;
+ const section = project.sectionMap.get(workspace.sectionId);
+ if (section) {
+ section.workspaces.push(sidebarWorkspace);
+ continue;
+ }
}🤖 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/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`
around lines 155 - 160, The code in useDashboardSidebarData is skipping
workspaces when a section record is missing because it unconditionally continues
after attempting to push to
project.sectionMap.get(workspace.sectionId)?.workspaces.push(sidebarWorkspace);
change the logic so you only continue when the section lookup succeeds; if
project.sectionMap.get(workspace.sectionId) is undefined, fall back to adding
sidebarWorkspace to the project's top-level collection (e.g., project.workspaces
or project.children) instead of continuing, ensuring workspace.sectionId,
project.sectionMap, and workspaces.push(sidebarWorkspace) are used to detect
success and perform the fallback.
Contributor
There was a problem hiding this comment.
2 issues found across 20 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/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx:63">
P2: Narrow the live-query dependency to the specific collection instead of the whole `collections` object to prevent unnecessary re-execution.
(Based on your team's feedback about narrowing React effect dependencies to required fields.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts:155">
P2: Workspaces with a missing or invalid `sectionId` will be silently dropped from the sidebar instead of falling back to the project root.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| id: sidebarSections.sectionId, | ||
| name: sidebarSections.name, | ||
| })), | ||
| [collections, projectId], |
Contributor
There was a problem hiding this comment.
P2: Narrow the live-query dependency to the specific collection instead of the whole collections object to prevent unnecessary re-execution.
(Based on your team's feedback about narrowing React effect dependencies to required fields.)
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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx, line 63:
<comment>Narrow the live-query dependency to the specific collection instead of the whole `collections` object to prevent unnecessary re-execution.
(Based on your team's feedback about narrowing React effect dependencies to required fields.) </comment>
<file context>
@@ -43,7 +46,22 @@ export function DashboardSidebarWorkspaceContextMenu({
+ id: sidebarSections.sectionId,
+ name: sidebarSections.name,
+ })),
+ [collections, projectId],
+ );
</file context>
Suggested change
| [collections, projectId], | |
| [collections.v2SidebarSections, projectId], |
Contributor
There was a problem hiding this comment.
2 issues found across 17 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/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx:87">
P2: The hidden rename button is still clickable (opacity alone doesn’t disable pointer events), so clicks on the count area can trigger rename instead of collapsing/expanding the section.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts:93">
P2: `Open in Finder` and `Copy Path` are wired as no-op placeholder toasts, so the new dashboard context menu exposes actions that do not actually execute.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| event.stopPropagation(); | ||
| onStartRename(); | ||
| }} | ||
| className="z-10 flex items-center justify-center opacity-0 text-muted-foreground transition-[opacity,color] duration-150 group-hover:opacity-100 hover:text-foreground" |
Contributor
There was a problem hiding this comment.
P2: The hidden rename button is still clickable (opacity alone doesn’t disable pointer events), so clicks on the count area can trigger rename instead of collapsing/expanding the section.
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/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx, line 87:
<comment>The hidden rename button is still clickable (opacity alone doesn’t disable pointer events), so clicks on the count area can trigger rename instead of collapsing/expanding the section.</comment>
<file context>
@@ -84,10 +84,10 @@ export const DashboardSidebarSectionHeader = forwardRef<
onStartRename();
}}
- className="flex items-center justify-center opacity-0 scale-90 text-muted-foreground transition-all duration-150 group-hover:scale-100 group-hover:opacity-100 hover:text-foreground"
+ className="z-10 flex items-center justify-center opacity-0 text-muted-foreground transition-[opacity,color] duration-150 group-hover:opacity-100 hover:text-foreground"
aria-label="Rename section"
>
</file context>
Suggested change
| className="z-10 flex items-center justify-center opacity-0 text-muted-foreground transition-[opacity,color] duration-150 group-hover:opacity-100 hover:text-foreground" | |
| className="z-10 flex items-center justify-center opacity-0 pointer-events-none text-muted-foreground transition-[opacity,color] duration-150 group-hover:pointer-events-auto group-hover:opacity-100 hover:text-foreground" |
| }; | ||
|
|
||
| const handleOpenInFinder = () => { | ||
| toast.info("Open in Finder is coming soon"); |
Contributor
There was a problem hiding this comment.
P2: Open in Finder and Copy Path are wired as no-op placeholder toasts, so the new dashboard context menu exposes actions that do not actually execute.
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/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts, line 93:
<comment>`Open in Finder` and `Copy Path` are wired as no-op placeholder toasts, so the new dashboard context menu exposes actions that do not actually execute.</comment>
<file context>
@@ -89,11 +89,21 @@ export function useDashboardSidebarWorkspaceItemActions({
};
+ const handleOpenInFinder = () => {
+ toast.info("Open in Finder is coming soon");
+ };
+
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx (1)
60-77:⚠️ Potential issue | 🟠 MajorCollapsed-mode Rename is still wired to a non-visible editor path.
Line 65 triggers
startRename, but the collapsed branch does not render inline rename controls, so the action appears broken to users.🤖 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/DashboardSidebarProjectSection.tsx` around lines 60 - 77, The collapsed project view is invoking startRename via the DashboardSidebarProjectContextMenu even though DashboardSidebarCollapsedProjectContent does not render inline rename controls; update the code so the context menu's onRename is only passed when the project is not collapsed (use project.isCollapsed to conditionally omit or replace onRename), or alternatively wire onRename to a modal-based rename handler for collapsed projects; locate DashboardSidebarProjectContextMenu and the props block where onRename={startRename} is passed and conditionally remove or swap that prop when project.isCollapsed is true.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx (1)
51-81:⚠️ Potential issue | 🟡 MinorExpose disclosure semantics on the project toggle button.
Line 51 toggles expandable content, but the control still doesn’t expose state/target to assistive tech. Add
aria-expandedand link it witharia-controlsto the animated content container.Suggested fix
<button type="button" + aria-expanded={!isCollapsed} + aria-controls={`project-${projectId}-workspace-list`} onClick={onToggleCollapse} className={cn( "flex items-center justify-center size-8 rounded-md", "hover:bg-muted/50 transition-colors", )} > @@ <motion.div + id={`project-${projectId}-workspace-list`} initial={{ height: 0, opacity: 0 }} animate={{ height: "auto", opacity: 1 }} exit={{ height: 0, opacity: 0 }}🤖 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/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx` around lines 51 - 81, The project toggle button (onToggleCollapse) currently doesn't expose its expanded state or target; update the button that calls onToggleCollapse to include aria-expanded={isCollapsed === false} (or !isCollapsed) and aria-controls pointing to a stable id for the animated content (e.g., `project-panel-${projectId}` or a generated unique id); then add that same id to the animated content container (the motion.div rendering the collapsible panel in DashboardSidebarCollapsedProjectContent) and optionally set aria-hidden={isCollapsed} on the panel so assistive tech can determine visibility. Ensure the id you choose references a unique symbol (like projectId or projectName) so aria-controls correctly links the button to the motion.div.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts (1)
156-165:⚠️ Potential issue | 🟠 MajorDon’t drop section-assigned workspaces when section lookup fails.
Line 164 currently runs even when
project.sectionMap.get(workspace.sectionId)isundefined, so those workspaces disappear from the sidebar instead of falling back to top-level children.Suggested fix
if (workspace.sectionId) { const section = project.sectionMap.get(workspace.sectionId); if (section) { section.workspaces.push({ ...sidebarWorkspace, accentColor: section.color, }); + continue; } - continue; }🤖 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/hooks/useDashboardSidebarData/useDashboardSidebarData.ts` around lines 156 - 165, The current code always continues when workspace.sectionId is truthy even if project.sectionMap.get(workspace.sectionId) returns undefined, causing those workspaces to be dropped; change the logic so you only continue when a valid section is found (i.e., after verifying section is truthy), and if section is undefined push sidebarWorkspace into the project's top-level children array (the same array used for non-section workspaces in this function) instead of dropping it; keep referencing workspace.sectionId, project.sectionMap.get(...), section.workspaces.push(...), and sidebarWorkspace to locate and update the branch.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx (1)
35-37: Remove duplicatedtransition-colorsutility class.
"transition-colors"is already in the base class string; the conditionalvariant === "expanded" && "transition-colors"is redundant in all three icon branches.Also applies to: 44-46, 53-55
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx` around lines 35 - 37, In DashboardSidebarWorkspaceIcon, remove the redundant conditional additions of the "transition-colors" utility (e.g., the expressions variant === "expanded" && "transition-colors") in each icon branch because "transition-colors" is already included in the base class string; update the className arrays for the icon render paths inside the DashboardSidebarWorkspaceIcon component to only conditionally set variant-specific classes and active/inactive text classes, leaving out any duplicate "transition-colors".apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx (1)
138-169: Extract the repeated right-action cluster into a shared subcomponent.The diff-stats + shortcut + close-action cluster is duplicated in both layout branches. Extracting it reduces drift risk and keeps interaction fixes in one place.
Also applies to: 208-239
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx` around lines 138 - 169, The block that renders DashboardSidebarWorkspaceDiffStats plus the shortcut label and close button (using Tooltip, TooltipTrigger, TooltipContent, HiMiniXMark) is duplicated; extract it from DashboardSidebarExpandedWorkspaceRow into a new shared subcomponent (e.g., WorkspaceRightActions or DashboardSidebarRightActions) that accepts props: additions, deletions, isActive, shortcutLabel, and onDeleteClick, preserves event.stopPropagation, aria-label="Close workspace", classes/transition behavior, and the Tooltip content; then replace both duplicated clusters (the current instance and the other branch at lines noted in the review) with this new component and add the necessary import/prop forwarding in DashboardSidebarExpandedWorkspaceRow.
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`:
- Around line 144-168: The close-action wrapper is hidden via "invisible ...
opacity-0" and only shown with "group-hover:visible group-hover:opacity-100",
making the delete button (the button that calls onDeleteClick inside
DashboardSidebarExpandedWorkspaceRow) unreachable by keyboard; update the
wrapper's class list to also include focus/focus-within visibility rules (e.g.
add "group-focus-within:visible group-focus-within:opacity-100" and optionally
"focus-within:visible focus-within:opacity-100" or "group-focus-visible:visible
group-focus-visible:opacity-100") so keyboard focus reveals the control, and
apply the same change to the other action wrapper block mentioned in the
comment.
- Around line 68-73: The row's onKeyDown handler (the inline handler that checks
event.key === "Enter" || " ") needs a guard so it only activates when the event
originates on the row itself (use event.target === event.currentTarget or
similar) to prevent activation from nested controls; update the handler that
calls onClick to check event.currentTarget === event.target before handling. For
nested close buttons (the elements wrapped with "invisible opacity-0 ...
group-hover:visible"), make them keyboard-discoverable and stop keyboard events
from bubbling: remove/avoid using fully invisible classes (or add
focus-visible/focus-within variants like focus-visible:visible or
focus-within:visible) so the buttons can receive focus, and add onKeyDown
handlers on the close buttons that call event.stopPropagation() (and
event.preventDefault() when handling Enter/Space) just like their click handlers
do, ensuring keyboard activation of the close buttons does not bubble to the
row.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx`:
- Around line 51-81: The project toggle button (onToggleCollapse) currently
doesn't expose its expanded state or target; update the button that calls
onToggleCollapse to include aria-expanded={isCollapsed === false} (or
!isCollapsed) and aria-controls pointing to a stable id for the animated content
(e.g., `project-panel-${projectId}` or a generated unique id); then add that
same id to the animated content container (the motion.div rendering the
collapsible panel in DashboardSidebarCollapsedProjectContent) and optionally set
aria-hidden={isCollapsed} on the panel so assistive tech can determine
visibility. Ensure the id you choose references a unique symbol (like projectId
or projectName) so aria-controls correctly links the button to the motion.div.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx`:
- Around line 60-77: The collapsed project view is invoking startRename via the
DashboardSidebarProjectContextMenu even though
DashboardSidebarCollapsedProjectContent does not render inline rename controls;
update the code so the context menu's onRename is only passed when the project
is not collapsed (use project.isCollapsed to conditionally omit or replace
onRename), or alternatively wire onRename to a modal-based rename handler for
collapsed projects; locate DashboardSidebarProjectContextMenu and the props
block where onRename={startRename} is passed and conditionally remove or swap
that prop when project.isCollapsed is true.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 156-165: The current code always continues when
workspace.sectionId is truthy even if
project.sectionMap.get(workspace.sectionId) returns undefined, causing those
workspaces to be dropped; change the logic so you only continue when a valid
section is found (i.e., after verifying section is truthy), and if section is
undefined push sidebarWorkspace into the project's top-level children array (the
same array used for non-section workspaces in this function) instead of dropping
it; keep referencing workspace.sectionId, project.sectionMap.get(...),
section.workspaces.push(...), and sidebarWorkspace to locate and update the
branch.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`:
- Around line 138-169: The block that renders DashboardSidebarWorkspaceDiffStats
plus the shortcut label and close button (using Tooltip, TooltipTrigger,
TooltipContent, HiMiniXMark) is duplicated; extract it from
DashboardSidebarExpandedWorkspaceRow into a new shared subcomponent (e.g.,
WorkspaceRightActions or DashboardSidebarRightActions) that accepts props:
additions, deletions, isActive, shortcutLabel, and onDeleteClick, preserves
event.stopPropagation, aria-label="Close workspace", classes/transition
behavior, and the Tooltip content; then replace both duplicated clusters (the
current instance and the other branch at lines noted in the review) with this
new component and add the necessary import/prop forwarding in
DashboardSidebarExpandedWorkspaceRow.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsx`:
- Around line 35-37: In DashboardSidebarWorkspaceIcon, remove the redundant
conditional additions of the "transition-colors" utility (e.g., the expressions
variant === "expanded" && "transition-colors") in each icon branch because
"transition-colors" is already included in the base class string; update the
className arrays for the icon render paths inside the
DashboardSidebarWorkspaceIcon component to only conditionally set
variant-specific classes and active/inactive text classes, leaving out any
duplicate "transition-colors".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23b4d591-16ba-4b3f-9456-ee83b654c3c3
📒 Files selected for processing (17)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarProjectContextMenu/DashboardSidebarProjectContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/hooks/useDashboardSidebarProjectSectionActions/useDashboardSidebarProjectSectionActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContextMenu/DashboardSidebarSectionContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceIcon/DashboardSidebarWorkspaceIcon.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarCollapsedWorkspaceButton/DashboardSidebarCollapsedWorkspaceButton.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/hooks/useDashboardSidebarWorkspaceItemActions/useDashboardSidebarWorkspaceItemActions.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/types.ts
Comment on lines
+68
to
+73
| onKeyDown={(event) => { | ||
| if (event.key === "Enter" || event.key === " ") { | ||
| event.preventDefault(); | ||
| onClick(); | ||
| } | ||
| }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=$(fd 'DashboardSidebarExpandedWorkspaceRow.tsx' | head -n 1)
echo "Reviewing: $FILE"
echo "--- Parent keyboard handler ---"
sed -n '64,80p' "$FILE"
echo "--- Nested close button handlers ---"
rg -n -C2 'onClick=\{\(event\) => \{|event\.stopPropagation\(\)|onDeleteClick\(\)' "$FILE"
echo "--- Guard presence check (should include currentTarget/target guard) ---"
rg -n 'event\.currentTarget\s*!==\s*event\.target' "$FILE" || trueRepository: superset-sh/superset
Length of output: 1505
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=$(fd 'DashboardSidebarExpandedWorkspaceRow.tsx' | head -n 1)
echo "--- Full file length and structure ---"
wc -l "$FILE"
echo "--- Action controls visibility styling (checking for group-hover) ---"
rg -n 'group-hover|invisible|opacity-0' "$FILE" | head -20
echo "--- Nested button structure and parent chain ---"
sed -n '145,165p' "$FILE"
echo "---"
sed -n '215,235p' "$FILE"
echo "--- Check if onKeyDown uses event delegation properly ---"
sed -n '68,76p' "$FILE"Repository: superset-sh/superset
Length of output: 2614
Guard row keyboard activation from bubbling events and make nested controls keyboard-discoverable.
The parent row's onKeyDown handler (line 68) lacks a guard to check if the event originates from the row itself or a nested element. Additionally, the nested close buttons are wrapped in invisible opacity-0 ... group-hover:visible containers (lines 144, 214), making them non-discoverable for keyboard users and creating conflicting behavior if they somehow receive focus.
Two issues need fixing:
-
Keyboard event bubbling: Nested close buttons only stop click propagation (lines 154–157, 224–227), not keyboard events. Pressing Enter/Space on a focused nested button will bubble to the parent's
onKeyDownhandler and trigger the row click unexpectedly. -
Keyboard discoverability: The close button containers use
invisible opacity-0 ... group-hover:visible, which hides them from keyboard navigation. Users cannot discover or reliably access these controls via keyboard.
🔧 Proposed fix for keyboard bubbling
onKeyDown={(event) => {
+ if (event.currentTarget !== event.target) {
+ return;
+ }
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
onClick();
}
}}For the visibility issue, consider making the close button permanently visible or at least focus-visible (e.g., group-hover:visible focus-within:visible).
📝 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
| onKeyDown={(event) => { | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onClick(); | |
| } | |
| }} | |
| onKeyDown={(event) => { | |
| if (event.currentTarget !== event.target) { | |
| return; | |
| } | |
| if (event.key === "Enter" || event.key === " ") { | |
| event.preventDefault(); | |
| onClick(); | |
| } | |
| }} |
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`
around lines 68 - 73, The row's onKeyDown handler (the inline handler that
checks event.key === "Enter" || " ") needs a guard so it only activates when the
event originates on the row itself (use event.target === event.currentTarget or
similar) to prevent activation from nested controls; update the handler that
calls onClick to check event.currentTarget === event.target before handling. For
nested close buttons (the elements wrapped with "invisible opacity-0 ...
group-hover:visible"), make them keyboard-discoverable and stop keyboard events
from bubbling: remove/avoid using fully invisible classes (or add
focus-visible/focus-within variants like focus-visible:visible or
focus-within:visible) so the buttons can receive focus, and add onKeyDown
handlers on the close buttons that call event.stopPropagation() (and
event.preventDefault() when handling Enter/Space) just like their click handlers
do, ensuring keyboard activation of the close buttons does not bubble to the
row.
Comment on lines
+144
to
+168
| <div className="invisible flex items-center justify-end gap-1.5 opacity-0 transition-[opacity,visibility] group-hover:visible group-hover:opacity-100"> | ||
| {shortcutLabel && ( | ||
| <span className="shrink-0 font-mono text-[10px] tabular-nums text-muted-foreground"> | ||
| {shortcutLabel} | ||
| </span> | ||
| )} | ||
| <Tooltip delayDuration={300}> | ||
| <TooltipTrigger asChild> | ||
| <button | ||
| type="button" | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| onDeleteClick(); | ||
| }} | ||
| className="flex items-center justify-center text-muted-foreground hover:text-foreground" | ||
| aria-label="Close workspace" | ||
| > | ||
| <HiMiniXMark className="size-3.5" /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="top" sideOffset={4}> | ||
| Close workspace | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Close action is mouse-hover gated and not keyboard discoverable.
The close-action wrapper stays invisible unless group-hover is active, so keyboard users cannot reliably reach/discover it. Add focus-within visibility states for parity.
🔧 Proposed fix
-<div className="invisible flex items-center justify-end gap-1.5 opacity-0 transition-[opacity,visibility] group-hover:visible group-hover:opacity-100">
+<div className="invisible flex items-center justify-end gap-1.5 opacity-0 transition-[opacity,visibility] group-hover:visible group-hover:opacity-100 group-focus-within:visible group-focus-within:opacity-100">Apply in both action wrappers (Line 144 and Line 214 blocks).
Also applies to: 214-238
🤖 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/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`
around lines 144 - 168, The close-action wrapper is hidden via "invisible ...
opacity-0" and only shown with "group-hover:visible group-hover:opacity-100",
making the delete button (the button that calls onDeleteClick inside
DashboardSidebarExpandedWorkspaceRow) unreachable by keyboard; update the
wrapper's class list to also include focus/focus-within visibility rules (e.g.
add "group-focus-within:visible group-focus-within:opacity-100" and optionally
"focus-within:visible focus-within:opacity-100" or "group-focus-visible:visible
group-focus-visible:opacity-100") so keyboard focus reveals the control, and
apply the same change to the other action wrapper block mentioned in the
comment.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This was referenced Apr 3, 2026
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
Validation
Summary by cubic
Refined the dashboard sidebar and rebuilt the new workspace modal for faster, clearer navigation and creation. Replaced legacy V2 components with dashboard-scoped versions and removed flaky terminal host tests.
New Features
useDashboardSidebarData; organized children with project-children utils.DashboardNewWorkspaceModal): project selector, device picker, prompt/list tabs (branches, PRs, issues), and advanced options (branch prefix/edit, setup script); added a WIPv2-workspacesbrowse route.Bug Fixes
Written for commit 5ab43c7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Chores