feat(desktop): SUPER-362 V2 workspace sidebar + modal + projects#2399
Merged
Conversation
Build the V2 workspace sidebar behind the V2_CLOUD feature flag: - localStorage Zustand stores for project/workspace/section local meta - Expanded useV2SidebarData hook joining Electric data with local meta - Component tree: header, collapsible project sections, workspace list items, empty state - Placeholder route for V2 workspace detail view - Feature flag conditional render in dashboard layout
Contributor
📝 WalkthroughWalkthroughAdds a feature-flagged V2 Cloud workspace experience: new V2 route, V2 workspace sidebar and modal, many V2-specific sidebar/modal components and hooks, and three persistent local meta stores for project/section/workspace UI state. Rendering switches to V2 components when the V2_CLOUD flag is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Layout as DashboardLayout
participant FeatureFlag as FeatureFlagService
participant Sidebar as V2WorkspaceSidebar
participant Hook as useV2SidebarData
participant ProjectStore as V2ProjectLocalMetaStore
participant Router as Router
participant Modal as V2NewWorkspaceModal
participant Draft as V2NewWorkspaceModalDraft
User->>Layout: Open dashboard
Layout->>FeatureFlag: check V2_CLOUD
FeatureFlag-->>Layout: true/false
alt V2 enabled
Layout->>Sidebar: render V2WorkspaceSidebar
Sidebar->>Hook: request groups
Hook->>ProjectStore: getProjectMeta(s)
ProjectStore-->>Hook: meta (isCollapsed, tabOrder)
Hook-->>Sidebar: groups + metadata
User->>Sidebar: toggle project collapse
Sidebar->>Hook: toggleProjectCollapsed(projectId)
Hook->>ProjectStore: toggleProjectCollapsed
User->>Sidebar: click workspace
Sidebar->>Router: navigate -> /v2-workspace/$id
User->>Sidebar: open "New Workspace"
Sidebar->>Modal: open V2NewWorkspaceModal
Modal->>Draft: initialize draft
User->>Modal: interact / create workspace
Modal->>Draft: runAsyncAction(createWorkspace)
else V2 disabled
Layout->>Layout: render legacy sidebar/modal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/stores/v2-section-local-meta.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/v2-section-local-meta.ts:13">
P3: This new store is never consumed anywhere in the renderer code, so it adds dead code and persisted state surface area without any runtime effect.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx:30">
P2: When V2 sidebar is enabled, workspace navigation uses `/v2-workspace/$workspaceId`, but this layout still derives `currentWorkspace` only from `/workspace/$workspaceId`. That breaks project preselection for `NEW_WORKSPACE` on V2 workspace pages.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx (1)
34-43:⚠️ Potential issue | 🟡 MinorUpdate route matcher to include V2 workspace pages.
The
matchRouteon line 34 only matches/workspace/$workspaceId, but the V2 sidebar navigates to/v2-workspace/$workspaceId. When users are on V2 pages,currentWorkspaceIdremainsnull, the workspace query is disabled, and theNEW_WORKSPACEhotkey opens without the active project preselected. Update the matcher to also resolve/v2-workspace/$workspaceId, or derive the project ID from V2 workspace data before callingopenNewWorkspaceModal.🤖 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/layout.tsx` around lines 34 - 43, The route matcher only checks "/workspace/$workspaceId" so currentWorkspaceMatch/currentWorkspaceId stays null on V2 pages and the electronTrpc.workspaces.get.useQuery is disabled; update the matchRoute call (currentWorkspaceMatch) to also match "/v2-workspace/$workspaceId" (or add a second match and fall back) so currentWorkspaceId is populated for V2 routes, ensuring the query is enabled and openNewWorkspaceModal receives the active project; adjust logic that derives currentWorkspaceId and the enabled flag for useQuery accordingly.
🤖 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/v2-workspace`.$workspaceId/page.tsx:
- Around line 26-29: The code is querying devices via useLiveQuery against
collections.v2Devices but v2Workspaces does not expose a device relation so
workspace?.deviceId is always undefined, making the devices query and Device row
dead; either add the device relation to the Electric collection shape or remove
the lookup/render. Fix by updating the collection shape in CollectionsProvider
so collections.v2Workspaces includes the device relation (expose deviceId or
devices relation used by v2Devices), or remove the dead code: eliminate the
useLiveQuery for v2Devices and the Device row render that depends on
workspace?.deviceId (and any code paths at useLiveQuery lines referencing
v2Devices and the component that reads workspace?.deviceId) so UI only renders
when a valid device relation exists.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx`:
- Around line 23-27: The collapse toggle button for the workspace list (the
element using onToggleCollapse(projectId)) needs accessible state: add
aria-expanded set to the current collapsed state and add aria-controls
referencing the panel's id; ensure the panel element (the collapsible container
rendering the workspace list) has a stable id that matches aria-controls (e.g.,
`workspace-list-${projectId}`) and update any other identical toggle/button
instance (the second toggle at the other block using the same pattern) to mirror
this change so assistive tech receives proper expanded/collapsed information.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx`:
- Around line 25-41: The button in V2WorkspaceListItem should expose the active
workspace semantically for screen readers: when isActive is true add
aria-current="page" to the <button> (e.g., set aria-current={isActive ? "page" :
undefined}) in the V2WorkspaceListItem component so assistive tech can identify
the current workspace; update the button JSX where navigate(...) is used and
keep the existing visual styles unchanged.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.ts`:
- Around line 58-60: The current mapping in useV2SidebarData sorts workspaces
purely by name (workspacesByProject) and ignores the persisted workspace
ordering in the local-meta store; update the mapping so it first looks up the
persisted order from the workspace local-meta (e.g., workspaceLocalMetaStore or
workspaceLocalMetaMap keyed by project.id), then sort the workspaces by that
order (use the index of each workspace.id in the persisted array, with items
missing from the array appended and sorted by name as a fallback). Locate the
workspaces mapping inside useV2SidebarData and replace the simple name
localeCompare sort with a comparator that consults the project's local-meta
ordering array, falling back to name-based ordering for unknown ids and an empty
persisted order.
- Around line 9-10: The memo that computes `groups` is using the stable function
reference `getProjectMeta` from `useV2ProjectLocalMetaStore`, so toggling via
`toggleProjectCollapsed(id)` doesn’t change the memo dependencies and
`isCollapsed` stays stale; change the hook to subscribe directly to the projects
metadata instead of the function by using a selector from
`useV2ProjectLocalMetaStore` (e.g., select the `projects` record or the specific
project's meta) and use that selected value in the `groups` memo dependencies so
updates to `projects` trigger recomputation; keep `toggleProjectCollapsed` as
the action but ensure `groups` reads from the selected `projects`/project meta
rather than `getProjectMeta`.
In `@apps/desktop/src/renderer/stores/v2-section-local-meta.ts`:
- Around line 9-25: The store V2SectionLocalMetaStore is incomplete:
V2SectionLocalMetaState currently only exposes sections and callers must use
setState directly; add typed actions on useV2SectionLocalMetaStore such as
getSectionMeta(sectionId: string): SectionMeta | undefined,
setSectionMeta(sectionId: string, meta: Partial<SectionMeta>),
removeSectionMeta(sectionId: string) and clearSections() that internally call
setState/update to read/merge/remove entries from the sections Record<string,
SectionMeta>, ensuring consumers can persist section-level preferences through
the store API instead of reaching for setState(...) directly.
---
Outside diff comments:
In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx`:
- Around line 34-43: The route matcher only checks "/workspace/$workspaceId" so
currentWorkspaceMatch/currentWorkspaceId stays null on V2 pages and the
electronTrpc.workspaces.get.useQuery is disabled; update the matchRoute call
(currentWorkspaceMatch) to also match "/v2-workspace/$workspaceId" (or add a
second match and fall back) so currentWorkspaceId is populated for V2 routes,
ensuring the query is enabled and openNewWorkspaceModal receives the active
project; adjust logic that derives currentWorkspaceId and the enabled flag for
useQuery accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7786e070-c3d8-4175-b175-c9fee98fe437
📒 Files selected for processing (16)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace.$workspaceId/page.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/V2WorkspaceSidebar.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarEmptyState/V2SidebarEmptyState.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarEmptyState/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/types.tsapps/desktop/src/renderer/stores/v2-project-local-meta.tsapps/desktop/src/renderer/stores/v2-section-local-meta.tsapps/desktop/src/renderer/stores/v2-workspace-local-meta.ts
Comment on lines
+23
to
+27
| <button | ||
| type="button" | ||
| onClick={() => onToggleCollapse(projectId)} | ||
| className="flex w-full items-center gap-1 rounded-md px-2 py-1 text-left text-[11px] font-medium tracking-wider text-muted-foreground uppercase hover:bg-muted/50" | ||
| > |
Contributor
There was a problem hiding this comment.
Announce the collapse state on the section toggle.
This button hides/shows the workspace list, but assistive tech never gets that state change. Please add at least aria-expanded here, and ideally aria-controls on the panel too.
Minimal fix
<button
type="button"
onClick={() => onToggleCollapse(projectId)}
+ aria-expanded={!isCollapsed}
className="flex w-full items-center gap-1 rounded-md px-2 py-1 text-left text-[11px] font-medium tracking-wider text-muted-foreground uppercase hover:bg-muted/50"
>Also applies to: 39-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx`
around lines 23 - 27, The collapse toggle button for the workspace list (the
element using onToggleCollapse(projectId)) needs accessible state: add
aria-expanded set to the current collapsed state and add aria-controls
referencing the panel's id; ensure the panel element (the collapsible container
rendering the workspace list) has a stable id that matches aria-controls (e.g.,
`workspace-list-${projectId}`) and update any other identical toggle/button
instance (the second toggle at the other block using the same pattern) to mirror
this change so assistive tech receives proper expanded/collapsed information.
Comment on lines
+25
to
+41
| <button | ||
| type="button" | ||
| onClick={() => | ||
| navigate({ | ||
| to: "/v2-workspace/$workspaceId", | ||
| params: { workspaceId: id }, | ||
| }) | ||
| } | ||
| className={`flex w-full flex-col rounded-md px-2 py-1.5 text-left text-sm transition-colors hover:bg-muted/50 ${ | ||
| isActive | ||
| ? "border-l-2 border-primary bg-muted/50" | ||
| : "border-l-2 border-transparent" | ||
| }`} | ||
| > | ||
| <span className="truncate">{name}</span> | ||
| <span className="truncate text-xs text-muted-foreground">{branch}</span> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Expose the active workspace semantically.
Right now selection is only visible via border/background. Add aria-current="page" when isActive so screen readers can identify the current workspace.
Minimal fix
<button
type="button"
+ aria-current={isActive ? "page" : undefined}
onClick={() =>
navigate({
to: "/v2-workspace/$workspaceId",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx`
around lines 25 - 41, The button in V2WorkspaceListItem should expose the active
workspace semantically for screen readers: when isActive is true add
aria-current="page" to the <button> (e.g., set aria-current={isActive ? "page" :
undefined}) in the V2WorkspaceListItem component so assistive tech can identify
the current workspace; update the button JSX where navigate(...) is used and
keep the existing visual styles unchanged.
Comment on lines
+9
to
+25
| interface V2SectionLocalMetaState { | ||
| sections: Record<string, SectionMeta>; | ||
| } | ||
|
|
||
| export const useV2SectionLocalMetaStore = create<V2SectionLocalMetaState>()( | ||
| devtools( | ||
| persist( | ||
| () => ({ | ||
| sections: {}, | ||
| }), | ||
| { | ||
| name: "v2-section-local-meta", | ||
| version: 1, | ||
| }, | ||
| ), | ||
| { name: "V2SectionLocalMetaStore" }, | ||
| ), |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
This store looks unfinished compared with the other local-meta stores.
Unlike the project/workspace stores, this one exposes no typed getter/setter actions. Callers would have to reach for useV2SectionLocalMetaStore.setState(...) to persist anything, so section-level preferences are not actually modeled through the store API yet.
♻️ Possible shape
interface V2SectionLocalMetaState {
sections: Record<string, SectionMeta>;
+ getSectionMeta: (id: string) => SectionMeta;
+ setSectionTabOrder: (id: string, order: number) => void;
}
+
+const DEFAULT_SECTION_META: SectionMeta = {
+ name: "",
+ tabOrder: 0,
+};
export const useV2SectionLocalMetaStore = create<V2SectionLocalMetaState>()(
devtools(
persist(
- () => ({
+ (set, get) => ({
sections: {},
+ getSectionMeta: (id) => get().sections[id] ?? DEFAULT_SECTION_META,
+ setSectionTabOrder: (id, order) => {
+ set((state) => ({
+ sections: {
+ ...state.sections,
+ [id]: {
+ ...(state.sections[id] ?? DEFAULT_SECTION_META),
+ tabOrder: order,
+ },
+ },
+ }));
+ },
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/stores/v2-section-local-meta.ts` around lines 9 -
25, The store V2SectionLocalMetaStore is incomplete: V2SectionLocalMetaState
currently only exposes sections and callers must use setState directly; add
typed actions on useV2SectionLocalMetaStore such as getSectionMeta(sectionId:
string): SectionMeta | undefined, setSectionMeta(sectionId: string, meta:
Partial<SectionMeta>), removeSectionMeta(sectionId: string) and clearSections()
that internally call setState/update to read/merge/remove entries from the
sections Record<string, SectionMeta>, ensuring consumers can persist
section-level preferences through the store API instead of reaching for
setState(...) directly.
Port the New Workspace modal to V2 schemas behind V2_CLOUD feature flag. Adds device picker, V2 project selector, and all four tabs (Prompt, Issues, Pull Requests, Branches) using Electric collections and cloud tRPC mutations.
- Hide setup script toggle in V2 prompt tab via hideSetupScript prop - Validate preSelectedProjectId against v2 projects before using it - Stabilize createWorkspace reference with useCallback - Improve local project matching with mainRepoPath basename fallback
Contributor
There was a problem hiding this comment.
6 issues found across 22 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/components/V2NewWorkspaceModal/V2NewWorkspaceModalDraftContext.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModalDraftContext.tsx:81">
P2: Extract shared draft-context behavior instead of duplicating this provider logic; keeping two near-identical implementations will make fixes/features drift between legacy and V2 modal flows.
(Based on your team's feedback about avoiding duplicated business logic across components.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx:138">
P2: The created branch ignores the computed prefix preview, so auto-prefixed branch names are not actually persisted.</violation>
<violation number="2" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx:143">
P2: Base branch selection is currently ignored during creation, so the advanced option has no effect.</violation>
</file>
<file name="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsx:105">
P2: Path parsing uses a Unix-only separator, so local project matching can fail on Windows. Use a cross-platform separator pattern (or a basename utility) when deriving `dirName`.
(Based on your team's feedback about using cross-platform path utilities instead of `split`.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsx:33">
P2: Narrow the `useLiveQuery` dependency from the whole `collections` object to `collections.v2Projects` to avoid unnecessary query re-initialization on unrelated provider re-renders.
(Based on your team's feedback about narrowing React dependencies to required fields.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx">
<violation number="1" location="apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx:68">
P2: Avoid mutating query result arrays in render; `.sort()` here mutates `data.branches` in place.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const V2NewWorkspaceModalDraftContext = | ||
| createContext<V2NewWorkspaceModalDraftContextValue | null>(null); | ||
|
|
||
| export function V2NewWorkspaceModalDraftProvider({ |
Contributor
There was a problem hiding this comment.
P2: Extract shared draft-context behavior instead of duplicating this provider logic; keeping two near-identical implementations will make fixes/features drift between legacy and V2 modal flows.
(Based on your team's feedback about avoiding duplicated business logic across components.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModalDraftContext.tsx, line 81:
<comment>Extract shared draft-context behavior instead of duplicating this provider logic; keeping two near-identical implementations will make fixes/features drift between legacy and V2 modal flows.
(Based on your team's feedback about avoiding duplicated business logic across components.) </comment>
<file context>
@@ -0,0 +1,190 @@
+const V2NewWorkspaceModalDraftContext =
+ createContext<V2NewWorkspaceModalDraftContextValue | null>(null);
+
+export function V2NewWorkspaceModalDraftProvider({
+ children,
+ onClose,
</file context>
| if (lp.githubOwner !== githubRepo.owner) return false; | ||
| if (lp.name === githubRepo.name) return true; | ||
| // Fallback: check directory basename in case user renamed the project | ||
| const dirName = lp.mainRepoPath?.split("/").pop(); |
Contributor
There was a problem hiding this comment.
P2: Path parsing uses a Unix-only separator, so local project matching can fail on Windows. Use a cross-platform separator pattern (or a basename utility) when deriving dirName.
(Based on your team's feedback about using cross-platform path utilities instead of split.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsx, line 105:
<comment>Path parsing uses a Unix-only separator, so local project matching can fail on Windows. Use a cross-platform separator pattern (or a basename utility) when deriving `dirName`.
(Based on your team's feedback about using cross-platform path utilities instead of `split`.) </comment>
<file context>
@@ -0,0 +1,215 @@
+ if (lp.githubOwner !== githubRepo.owner) return false;
+ if (lp.name === githubRepo.name) return true;
+ // Fallback: check directory basename in case user renamed the project
+ const dirName = lp.mainRepoPath?.split("/").pop();
+ return dirName === githubRepo.name;
+ });
</file context>
| q | ||
| .from({ projects: collections.v2Projects }) | ||
| .select(({ projects }) => ({ ...projects })), | ||
| [collections], |
Contributor
There was a problem hiding this comment.
P2: Narrow the useLiveQuery dependency from the whole collections object to collections.v2Projects to avoid unnecessary query re-initialization on unrelated provider re-renders.
(Based on your team's feedback about narrowing React 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/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsx, line 33:
<comment>Narrow the `useLiveQuery` dependency from the whole `collections` object to `collections.v2Projects` to avoid unnecessary query re-initialization on unrelated provider re-renders.
(Based on your team's feedback about narrowing React dependencies to required fields.) </comment>
<file context>
@@ -0,0 +1,77 @@
+ q
+ .from({ projects: collections.v2Projects })
+ .select(({ projects }) => ({ ...projects })),
+ [collections],
+ );
+
</file context>
|
|
||
| const defaultBranch = data?.defaultBranch ?? "main"; | ||
|
|
||
| const branches = (data?.branches ?? []).sort((a, b) => { |
Contributor
There was a problem hiding this comment.
P2: Avoid mutating query result arrays in render; .sort() here mutates data.branches in place.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx, line 68:
<comment>Avoid mutating query result arrays in render; `.sort()` here mutates `data.branches` in place.</comment>
<file context>
@@ -0,0 +1,205 @@
+
+ const defaultBranch = data?.defaultBranch ?? "main";
+
+ const branches = (data?.branches ?? []).sort((a, b) => {
+ if (a.name === defaultBranch) return -1;
+ if (b.name === defaultBranch) return 1;
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx (1)
13-19: Addaria-labelfor screen reader accessibility.The button contains only an icon with no visible text. Screen readers cannot determine the button's purpose. Adding an accessible label ensures the button is usable for all users.
♿ Proposed accessibility fix
<button type="button" onClick={() => openModal()} + aria-label="Create new workspace" className="rounded-md p-1 text-muted-foreground hover:text-foreground transition-colors" > <LuPlus className="size-4" /> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx` around lines 13 - 19, The icon-only button in V2SidebarHeader lacks an accessible name; update the button element (the one calling openModal and rendering <LuPlus />) to include an aria-label (for example aria-label="Create new workspace") so screen readers can convey its purpose; ensure the label is concise and descriptive and keep the existing onClick handler (openModal) unchanged.apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx (1)
14-14: ExtractPromptGroupAdvancedOptionsbehind a shared barrel.This reaches into
NewWorkspaceModal’s nested internals from the V2 flow, which makes the new modal depend on another feature’s private folder layout. Moving the shared advanced-options UI behind a stable barrel/alias would reduce that coupling.As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible, and**/*.tsx: Organize project files using the prescribed folder structure: one folder per component with barrel exports (index.ts), co-located tests and utilities, nested components by usage frequency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx` at line 14, The V2 flow file V2PromptGroup.tsx imports PromptGroupAdvancedOptions from another feature’s private path; extract PromptGroupAdvancedOptions into a shared barrel and update the import to use the project alias. Create or update an index.ts barrel that re-exports PromptGroupAdvancedOptions from its component folder (so the symbol PromptGroupAdvancedOptions is exported from a stable module), adjust any tsconfig path alias if needed, and then change the import in V2PromptGroup.tsx to use the shared barrel/alias (e.g., import { PromptGroupAdvancedOptions } from '<shared-alias>/PromptGroup') to remove coupling to NewWorkspaceModal internals.
🤖 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/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx`:
- Around line 66-73: The code mutates shared query data by calling sort()
directly on data?.branches when computing branches; change this to work on a
shallow copy so the original cached array isn't mutated (e.g., create a new
array from data?.branches before sorting). Update the logic that defines
branches (the variable named branches that uses defaultBranch) to copy the
source array first, then apply the existing comparator (respecting defaultBranch
and isLocal) so downstream consumers like branchRows/Fuse see a stable,
non-mutated query result.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsx`:
- Around line 77-92: Currently V2IssuesGroup builds workspaceByBranch as
Map<branch, workspaceId> which collapses workspaces across devices; change the
dedup key to include device scope and update lookups to check branch+device. In
the useMemo that builds workspaceByBranch (referenced as workspaceByBranch and
the useLiveQuery result v2WorkspacesData) create either a Map<branch,
Map<deviceId, workspaceId>> or use a composite key like `${branch}:${deviceId}`
when inserting w (use w.deviceId) and when later checking existence (the checks
near selectedDeviceId in the logic around the lines noted: 152-163 and 177-206)
look up using the branch+selectedDeviceId key so the UI only treats a workspace
as “existing” for the currently selected device.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsx`:
- Around line 101-106: The fallback that derives dirName from lp.mainRepoPath by
splitting on "/" fails on Windows; update the logic inside the
localProjects.find callback (the block referencing lp.mainRepoPath and
githubRepo) to compute the basename using a cross-platform method (e.g., use
Node's path.basename(lp.mainRepoPath || '') after importing path from "path" or
normalize by splitting on both "/" and "\\"), then compare that basename to
githubRepo.name so renamed repos on Windows are detected and
resolvedLocalProjectId is set correctly.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx`:
- Around line 113-116: The UI shows branchPreview (computed from branchSlug,
resolvedPrefix and applyPrefix via sanitizeBranchNameWithMaxLength) but the
creation/submission logic still uses the raw branchSlug, so the persisted branch
drops the prefix; update the submission path (the handler that constructs the
workspace/branch payload — e.g., the function that currently reads branchSlug at
submit time) to use branchPreview as the branch name (falling back to branchSlug
if branchPreview is falsy), ensuring you pass the sanitized, prefixed branch
value produced by sanitizeBranchNameWithMaxLength and governed by resolvedPrefix
and applyPrefix.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx`:
- Around line 57-63: The workspaceByBranch map currently keys only on branch
name (workspaceByBranch built from v2WorkspacesData) which allows collisions for
forked PRs sharing the same headBranch; change the key to include a PR-qualifier
(e.g., `${repositoryId}:${pr.headBranch}` or use `prNumber` if available) when
building workspaceByBranch and when looking up/creating workspaces (locations
referencing pr.headBranch and the createWorkspace call), and ensure
createWorkspace receives the same qualifier (repositoryId or prNumber) so
workspace identity is unambiguous across forks; apply the same change to the
parallel PullRequestsGroup component referenced in the review.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModal.tsx`:
- Around line 22-23: The modal is being closed via the Dialog onOpenChange
(backdrop/Esc) which only calls closeModal, so the
V2NewWorkspaceModalDraftProvider’s draft state isn't reset; update the
onOpenChange handler inside V2NewWorkspaceModal (the Dialog line) to call the
draft provider's reset API (e.g., closeAndResetDraft() or runAsyncAction()
exposed from V2NewWorkspaceModalDraftContext) when open becomes false, or call
both closeModal() and closeAndResetDraft() so manual dismisses clear the draft
stored by V2NewWorkspaceModalDraftProvider.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx`:
- Line 14: The V2 flow file V2PromptGroup.tsx imports PromptGroupAdvancedOptions
from another feature’s private path; extract PromptGroupAdvancedOptions into a
shared barrel and update the import to use the project alias. Create or update
an index.ts barrel that re-exports PromptGroupAdvancedOptions from its component
folder (so the symbol PromptGroupAdvancedOptions is exported from a stable
module), adjust any tsconfig path alias if needed, and then change the import in
V2PromptGroup.tsx to use the shared barrel/alias (e.g., import {
PromptGroupAdvancedOptions } from '<shared-alias>/PromptGroup') to remove
coupling to NewWorkspaceModal internals.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx`:
- Around line 13-19: The icon-only button in V2SidebarHeader lacks an accessible
name; update the button element (the one calling openModal and rendering <LuPlus
/>) to include an aria-label (for example aria-label="Create new workspace") so
screen readers can convey its purpose; ensure the label is concise and
descriptive and keep the existing onClick handler (openModal) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f77c72d-46fc-4c9f-81c3-5138029e9f2b
📒 Files selected for processing (22)
apps/desktop/src/renderer/components/NewWorkspaceModal/components/PromptGroup/components/PromptGroupAdvancedOptions/PromptGroupAdvancedOptions.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModal.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModalDraftContext.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/DevicePicker.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/hooks/useV2CreateWorkspace/index.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/hooks/useV2CreateWorkspace/useV2CreateWorkspace.tsapps/desktop/src/renderer/components/V2NewWorkspaceModal/index.tsapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/index.ts
Comment on lines
+66
to
+73
| const defaultBranch = data?.defaultBranch ?? "main"; | ||
|
|
||
| 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); | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't sort the query result in place.
data?.branches is shared query data, and sort() mutates it. That means this component can reorder cached branch data for other consumers, and it also forces the downstream branchRows/Fuse work to churn on every render.
Proposed fix
- 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);
- });
+ const branches = useMemo(
+ () =>
+ [...(data?.branches ?? [])].sort((a, b) => {
+ const aIsDefault = a.name === defaultBranch;
+ const bIsDefault = b.name === defaultBranch;
+ if (aIsDefault !== bIsDefault) return aIsDefault ? -1 : 1;
+ if (a.isLocal !== b.isLocal) return a.isLocal ? -1 : 1;
+ return a.name.localeCompare(b.name);
+ }),
+ [data?.branches, defaultBranch],
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx`
around lines 66 - 73, The code mutates shared query data by calling sort()
directly on data?.branches when computing branches; change this to work on a
shallow copy so the original cached array isn't mutated (e.g., create a new
array from data?.branches before sorting). Update the logic that defines
branches (the variable named branches that uses defaultBranch) to copy the
source array first, then apply the existing comparator (respecting defaultBranch
and isLocal) so downstream consumers like branchRows/Fuse see a stable,
non-mutated query result.
Comment on lines
+77
to
+92
| const { data: v2WorkspacesData } = useLiveQuery( | ||
| (q) => | ||
| q | ||
| .from({ ws: collections.v2Workspaces }) | ||
| .where(({ ws }) => eq(ws.projectId, projectId ?? "")) | ||
| .select(({ ws }) => ({ id: ws.id, branch: ws.branch })), | ||
| [collections, projectId], | ||
| ); | ||
|
|
||
| const workspaceByBranch = useMemo(() => { | ||
| const map = new Map<string, string>(); | ||
| for (const w of v2WorkspacesData ?? []) { | ||
| map.set(w.branch, w.id); | ||
| } | ||
| return map; | ||
| }, [v2WorkspacesData]); |
Contributor
There was a problem hiding this comment.
Deduplicate existing workspaces by branch and device.
Lines 81-90 collapse every workspace in a project into a single branch -> workspaceId entry. If the same branch already exists on another device, Line 152 still resolves it as existing and the row flips to Open, even though Line 163 would otherwise create a workspace for the currently selected selectedDeviceId.
🔧 Proposed fix
const { data: v2WorkspacesData } = useLiveQuery(
(q) =>
q
.from({ ws: collections.v2Workspaces })
.where(({ ws }) => eq(ws.projectId, projectId ?? ""))
- .select(({ ws }) => ({ id: ws.id, branch: ws.branch })),
+ .select(({ ws }) => ({
+ id: ws.id,
+ branch: ws.branch,
+ deviceId: ws.deviceId,
+ })),
[collections, projectId],
);
- const workspaceByBranch = useMemo(() => {
+ const workspaceByBranchAndDevice = useMemo(() => {
const map = new Map<string, string>();
for (const w of v2WorkspacesData ?? []) {
- map.set(w.branch, w.id);
+ map.set(`${w.branch}::${w.deviceId ?? ""}`, w.id);
}
return map;
}, [v2WorkspacesData]);
@@
- const existingId = workspaceByBranch.get(task.slug.toLowerCase());
+ const existingId = workspaceByBranchAndDevice.get(
+ `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`,
+ );
@@
- {workspaceByBranch.has(task.slug.toLowerCase()) ? (
+ {workspaceByBranchAndDevice.has(
+ `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`,
+ ) ? (
@@
- {workspaceByBranch.has(task.slug.toLowerCase()) ? "Open" : "Create"}{" "}
+ {workspaceByBranchAndDevice.has(
+ `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`,
+ )
+ ? "Open"
+ : "Create"}{" "}Also applies to: 152-163, 177-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsx`
around lines 77 - 92, Currently V2IssuesGroup builds workspaceByBranch as
Map<branch, workspaceId> which collapses workspaces across devices; change the
dedup key to include device scope and update lookups to check branch+device. In
the useMemo that builds workspaceByBranch (referenced as workspaceByBranch and
the useLiveQuery result v2WorkspacesData) create either a Map<branch,
Map<deviceId, workspaceId>> or use a composite key like `${branch}:${deviceId}`
when inserting w (use w.deviceId) and when later checking existence (the checks
near selectedDeviceId in the logic around the lines noted: 152-163 and 177-206)
look up using the branch+selectedDeviceId key so the UI only treats a workspace
as “existing” for the currently selected device.
Comment on lines
+101
to
+106
| const match = localProjects.find((lp) => { | ||
| if (lp.githubOwner !== githubRepo.owner) return false; | ||
| if (lp.name === githubRepo.name) return true; | ||
| // Fallback: check directory basename in case user renamed the project | ||
| const dirName = lp.mainRepoPath?.split("/").pop(); | ||
| return dirName === githubRepo.name; |
Contributor
There was a problem hiding this comment.
Make the basename fallback work on Windows paths.
Line 105 only splits mainRepoPath on /, so renamed repos on Windows never match and resolvedLocalProjectId stays null. That disables the local-project-backed prompt/branch flows for otherwise valid repos.
🔧 Proposed fix
- const dirName = lp.mainRepoPath?.split("/").pop();
+ const dirName = lp.mainRepoPath?.split(/[/\\]/).pop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsx`
around lines 101 - 106, The fallback that derives dirName from lp.mainRepoPath
by splitting on "/" fails on Windows; update the logic inside the
localProjects.find callback (the block referencing lp.mainRepoPath and
githubRepo) to compute the basename using a cross-platform method (e.g., use
Node's path.basename(lp.mainRepoPath || '') after importing path from "path" or
normalize by splitting on both "/" and "\\"), then compare that basename to
githubRepo.name so renamed repos on Windows are detected and
resolvedLocalProjectId is set correctly.
Comment on lines
+57
to
+63
| const workspaceByBranch = useMemo(() => { | ||
| const map = new Map<string, string>(); | ||
| for (const w of v2WorkspacesData ?? []) { | ||
| map.set(w.branch, w.id); | ||
| } | ||
| return map; | ||
| }, [v2WorkspacesData]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Pull request model fields =="
rg -n -C3 'githubPullRequests|headBranch|repositoryId|headRepo|headRepository|fork|authorLogin'
echo
echo "== V2 workspace creation inputs used by PR flows =="
rg -n -C3 'createWorkspace\(|v2Workspace\.create|branch:' apps/desktop/src/renderer/components/V2NewWorkspaceModal apps/desktop/src/renderer/components/V2NewWorkspaceModal/hooksRepository: superset-sh/superset
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== V2PullRequestsGroup PR object fields (useLiveQuery select) ==="
sed -n '38,45p' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx
echo
echo "=== SelectGithubPullRequest type definition ==="
rg -A 30 'export type SelectGithubPullRequest' packages/db/src/schema/
echo
echo "=== createWorkspace procedure signature ==="
rg -B 5 -A 20 'export.*createWorkspace\s*=' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts | head -60
echo
echo "=== createWorkspace input validation ==="
rg -B 5 -A 15 'createWorkspace.*\.input' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts | head -80Repository: superset-sh/superset
Length of output: 599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full githubPullRequests table schema ==="
sed -n '109,186p' packages/db/src/schema/github.ts
echo
echo "=== createWorkspace input/procedure ==="
rg -A 30 'export const createWorkspace' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts | head -80Repository: superset-sh/superset
Length of output: 2506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== createWorkspace full file ==="
wc -l apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts
echo
echo "=== createWorkspace procedure (first 100 lines) ==="
head -100 apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.ts
echo
echo "=== Check for branch or pr_number in create input ==="
rg -C 5 'branch:|pr_number:' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsRepository: superset-sh/superset
Length of output: 8597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for createWorkspace mutation in V2PullRequestsGroup call context ==="
rg -B 5 -A 10 'createWorkspace\(' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx
echo
echo "=== Search for the actual createWorkspace trpc mutation definition ==="
rg -B 2 -A 25 'createWorkspace:.*publicProcedure' apps/desktop/src/lib/trpc/routers/workspaces/procedures/create.tsRepository: superset-sh/superset
Length of output: 654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check what happens in V2 workspace lookup - how are workspaces matched ==="
rg -B 10 -A 5 'workspaceByBranch\s*=' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx
echo
echo "=== Check v2WorkspacesData structure and how workspaces are loaded ==="
rg -B 5 -A 10 'v2WorkspacesData' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx | head -50
echo
echo "=== Check if v2Workspaces schema has PR or fork info ==="
rg -A 50 'export const v2Workspaces' packages/db/src/schema/ | head -80Repository: superset-sh/superset
Length of output: 6739
🏁 Script executed:
#!/bin/bash
set -euo zipefail
echo "=== Check if githubPullRequests table stores fork/cross-repo info ==="
grep -r 'isCross\|fork\|headRepo' packages/db/src/schema/github.ts
echo
echo "=== Check full PR schema one more time (complete table definition) ==="
cat packages/db/src/schema/github.ts | sed -n '/^export const githubPullRequests/,/^);/p'
echo
echo "=== Check if PR data is extended at runtime with fork info ==="
rg -B 5 -A 10 'isCrossRepository|headRepository' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/Repository: superset-sh/superset
Length of output: 117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if githubPullRequests table stores fork/cross-repo info ==="
grep -E 'isCross|fork|headRepo' packages/db/src/schema/github.ts || echo "No fork-related fields found"
echo
echo "=== Check full PR schema one more time (complete table definition) ==="
cat packages/db/src/schema/github.ts | sed -n '/^export const githubPullRequests/,/^);/p'
echo
echo "=== Check if PR data is extended at runtime with fork info ==="
rg -B 5 -A 10 'isCrossRepository|headRepository' apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/ || echo "Not found in V2 component"Repository: superset-sh/superset
Length of output: 2681
Branch-name-only workspace identity risks collision from fork PRs.
The flow uses only pr.headBranch to look up and create workspaces (lines 131, 141), but githubPullRequests schema has no fork-source tracking. Multiple PRs from different forks can have the same headBranch under a single repositoryId. The workspaceByBranch map will only track one workspace ID per branch name, losing or overwriting the other. The createWorkspace call receives no PR-qualifying context to resolve ambiguity.
Either store fork source info in githubPullRequests, use prNumber as a workspace qualifier alongside branch, or limit the feature to same-repo PRs only.
Also applies to: 131–143, and the parallel issue in apps/desktop/src/renderer/components/NewWorkspaceModal/components/PullRequestsGroup/PullRequestsGroup.tsx (lines 228, 251, 269).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx`
around lines 57 - 63, The workspaceByBranch map currently keys only on branch
name (workspaceByBranch built from v2WorkspacesData) which allows collisions for
forked PRs sharing the same headBranch; change the key to include a PR-qualifier
(e.g., `${repositoryId}:${pr.headBranch}` or use `prNumber` if available) when
building workspaceByBranch and when looking up/creating workspaces (locations
referencing pr.headBranch and the createWorkspace call), and ensure
createWorkspace receives the same qualifier (repositoryId or prNumber) so
workspace identity is unambiguous across forks; apply the same change to the
parallel PullRequestsGroup component referenced in the review.
Comment on lines
+22
to
+23
| <V2NewWorkspaceModalDraftProvider onClose={closeModal}> | ||
| <Dialog open={isOpen} onOpenChange={(open) => !open && closeModal()}> |
Contributor
There was a problem hiding this comment.
Manual dismiss should reset the modal draft.
V2NewWorkspaceModalDraftProvider only clears state through closeAndResetDraft() or runAsyncAction() in V2NewWorkspaceModalDraftContext.tsx. Here backdrop clicks and Esc only call the store's closeModal, so reopening the modal reuses the previous project/query/prompt instead of starting fresh.
Proposed fix
import { V2NewWorkspaceModalContent } from "./components/V2NewWorkspaceModalContent";
-import { V2NewWorkspaceModalDraftProvider } from "./V2NewWorkspaceModalDraftContext";
+import {
+ useV2NewWorkspaceModalDraft,
+ V2NewWorkspaceModalDraftProvider,
+} from "./V2NewWorkspaceModalDraftContext";
export function V2NewWorkspaceModal() {
const isOpen = useNewWorkspaceModalOpen();
const closeModal = useCloseNewWorkspaceModal();
const preSelectedProjectId = usePreSelectedProjectId();
return (
<V2NewWorkspaceModalDraftProvider onClose={closeModal}>
- <Dialog open={isOpen} onOpenChange={(open) => !open && closeModal()}>
- <DialogHeader className="sr-only">
- <DialogTitle>New Workspace</DialogTitle>
- <DialogDescription>
- Create a new workspace from a PR, branch, issue, or prompt.
- </DialogDescription>
- </DialogHeader>
- <DialogContent
- showCloseButton={false}
- className="bg-popover text-popover-foreground sm:max-w-[560px] max-h-[min(70vh,600px)] !top-[calc(50%-min(35vh,300px))] !-translate-y-0 flex flex-col overflow-hidden p-0"
- >
- <V2NewWorkspaceModalContent
- isOpen={isOpen}
- preSelectedProjectId={preSelectedProjectId}
- />
- </DialogContent>
- </Dialog>
+ <V2NewWorkspaceModalDialog
+ isOpen={isOpen}
+ preSelectedProjectId={preSelectedProjectId}
+ />
</V2NewWorkspaceModalDraftProvider>
);
}
+
+function V2NewWorkspaceModalDialog({
+ isOpen,
+ preSelectedProjectId,
+}: {
+ isOpen: boolean;
+ preSelectedProjectId: string | null;
+}) {
+ const { closeAndResetDraft } = useV2NewWorkspaceModalDraft();
+
+ return (
+ <Dialog
+ open={isOpen}
+ onOpenChange={(open) => !open && closeAndResetDraft()}
+ >
+ <DialogHeader className="sr-only">
+ <DialogTitle>New Workspace</DialogTitle>
+ <DialogDescription>
+ Create a new workspace from a PR, branch, issue, or prompt.
+ </DialogDescription>
+ </DialogHeader>
+ <DialogContent
+ showCloseButton={false}
+ className="bg-popover text-popover-foreground sm:max-w-[560px] max-h-[min(70vh,600px)] !top-[calc(50%-min(35vh,300px))] !-translate-y-0 flex flex-col overflow-hidden p-0"
+ >
+ <V2NewWorkspaceModalContent
+ isOpen={isOpen}
+ preSelectedProjectId={preSelectedProjectId}
+ />
+ </DialogContent>
+ </Dialog>
+ );
+}📝 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
| <V2NewWorkspaceModalDraftProvider onClose={closeModal}> | |
| <Dialog open={isOpen} onOpenChange={(open) => !open && closeModal()}> | |
| import { V2NewWorkspaceModalContent } from "./components/V2NewWorkspaceModalContent"; | |
| import { | |
| useV2NewWorkspaceModalDraft, | |
| V2NewWorkspaceModalDraftProvider, | |
| } from "./V2NewWorkspaceModalDraftContext"; | |
| export function V2NewWorkspaceModal() { | |
| const isOpen = useNewWorkspaceModalOpen(); | |
| const closeModal = useCloseNewWorkspaceModal(); | |
| const preSelectedProjectId = usePreSelectedProjectId(); | |
| return ( | |
| <V2NewWorkspaceModalDraftProvider onClose={closeModal}> | |
| <V2NewWorkspaceModalDialog | |
| isOpen={isOpen} | |
| preSelectedProjectId={preSelectedProjectId} | |
| /> | |
| </V2NewWorkspaceModalDraftProvider> | |
| ); | |
| } | |
| function V2NewWorkspaceModalDialog({ | |
| isOpen, | |
| preSelectedProjectId, | |
| }: { | |
| isOpen: boolean; | |
| preSelectedProjectId: string | null; | |
| }) { | |
| const { closeAndResetDraft } = useV2NewWorkspaceModalDraft(); | |
| return ( | |
| <Dialog | |
| open={isOpen} | |
| onOpenChange={(open) => !open && closeAndResetDraft()} | |
| > | |
| <DialogHeader className="sr-only"> | |
| <DialogTitle>New Workspace</DialogTitle> | |
| <DialogDescription> | |
| Create a new workspace from a PR, branch, issue, or prompt. | |
| </DialogDescription> | |
| </DialogHeader> | |
| <DialogContent | |
| showCloseButton={false} | |
| className="bg-popover text-popover-foreground sm:max-w-[560px] max-h-[min(70vh,600px)] !top-[calc(50%-min(35vh,300px))] !-translate-y-0 flex flex-col overflow-hidden p-0" | |
| > | |
| <V2NewWorkspaceModalContent | |
| isOpen={isOpen} | |
| preSelectedProjectId={preSelectedProjectId} | |
| /> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/V2NewWorkspaceModal.tsx`
around lines 22 - 23, The modal is being closed via the Dialog onOpenChange
(backdrop/Esc) which only calls closeModal, so the
V2NewWorkspaceModalDraftProvider’s draft state isn't reset; update the
onOpenChange handler inside V2NewWorkspaceModal (the Dialog line) to call the
draft provider's reset API (e.g., closeAndResetDraft() or runAsyncAction()
exposed from V2NewWorkspaceModalDraftContext) when open becomes false, or call
both closeModal() and closeAndResetDraft() so manual dismisses clear the draft
stored by V2NewWorkspaceModalDraftProvider.
… prefix bug, and polish modal - Add V2SidebarFooter with GitHub repo picker for creating V2 projects - Fix stale useMemo in useV2SidebarData by using projectMetas directly instead of stable getProjectMeta function ref (collapse toggle now works) - Fix branch prefix not applied at workspace creation (use branchPreview instead of raw branchSlug in V2PromptGroup) - Restyle DevicePicker to match V2ProjectSelector (ghost button + popover) - Polish modal header layout with consistent padding and divider - Add missing useFeatureFlagEnabled import in authenticated layout
Adds a hover-visible + button on each project header in the V2 sidebar that opens the new workspace modal pre-selected to that project.
Always-visible + button with tooltip, chevron as separate toggle on the right, matching padding/spacing/icon sizes from ProjectHeader.
- Add V2ProjectThumbnail with GitHub avatar (resolved via githubRepositories collection) and first-letter fallback - Project sections use border separators instead of spacing - Workspace list items match legacy: active left-border indicator, text-[13px] with font-medium when active, mono branch subtitle - Remove extra padding wrapper from sidebar content area
AviPeltz
changed the title
- Add navigateToV2Workspace helper that routes to /v2-workspace/$id - Update V2BranchesGroup, V2IssuesGroup, V2PullRequestsGroup to use navigateToV2Workspace instead of navigateToWorkspace (V1 route) - Add V2_CLOUD feature flag guard to the v2-workspace page, redirecting to /workspace when the flag is disabled
…ice picker fix - Add collapsed sidebar mode matching legacy sidebar (header, project thumbnails, workspace icons, footer all render in icon-only mode) - Add keyboard shortcuts (⌘1-9) for V2 workspace switching - Show shortcut hints on workspace hover in expanded mode - Update header with New Workspace button and shortcut text - Update footer with DropdownMenu matching legacy pattern - Fix DevicePicker sending null instead of actual device ID for "This device", and auto-default to local device on mount - Fix empty state text to match legacy sidebar
…ename to V2 sidebar - Add right-click context menus on projects (rename, new workspace, delete, copy ID) and workspaces (rename, delete, copy ID) - Add drag-and-drop reorder for projects and workspaces within projects using react-dnd - Add inline rename via context menu or double-click on project names - Add delete confirmation dialogs using AlertDialog pattern - Add workspace tab order sorting to useV2SidebarData with sortVersion reactivity - Add bumpSortVersion and partialize to v2-workspace-local-meta store
Dialog no longer auto-closes on confirm click. Callers explicitly close after successful deletion. Cancel and overlay dismiss are disabled while the mutation is pending.
Contributor
There was a problem hiding this comment.
5 issues found across 24 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/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx:27">
P2: Handle `navigator.clipboard.writeText` asynchronously before showing success. Right now the success toast is shown even when copy fails, and rejections are unhandled.
(Based on your team's feedback about handling async calls with await/catch.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/useV2WorkspaceShortcuts.ts">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/useV2WorkspaceShortcuts.ts:11">
P2: This hook duplicates the existing workspace shortcut registration logic instead of reusing a shared helper, creating two maintenance points for the same 1-9 hotkey behavior.
(Based on your team's feedback about avoiding duplicated business logic across components.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx:244">
P3: Double-click rename is wired on a button that also toggles collapse on click, so starting rename also triggers collapse toggles.</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx:25">
P2: Handle `clipboard.writeText` as an async operation; the current code can trigger an unhandled promise rejection and incorrectly shows a success toast on failure.
(Based on your team's feedback about handling async calls with proper await/catch.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx:116">
P2: When the sidebar is collapsed, clicking 'Rename' updates state but fails to show the `<RenameInput>`, making the action silently fail visually.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| children, | ||
| }: V2ProjectContextMenuProps) { | ||
| const handleCopyId = () => { | ||
| navigator.clipboard.writeText(id); |
Contributor
There was a problem hiding this comment.
P2: Handle navigator.clipboard.writeText asynchronously before showing success. Right now the success toast is shown even when copy fails, and rejections are unhandled.
(Based on your team's feedback about handling async calls with await/catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx, line 27:
<comment>Handle `navigator.clipboard.writeText` asynchronously before showing success. Right now the success toast is shown even when copy fails, and rejections are unhandled.
(Based on your team's feedback about handling async calls with await/catch.) </comment>
<file context>
@@ -0,0 +1,58 @@
+ children,
+}: V2ProjectContextMenuProps) {
+ const handleCopyId = () => {
+ navigator.clipboard.writeText(id);
+ toast.success("Project ID copied");
+ };
</file context>
| * Keyboard shortcuts for V2 workspace switching (⌘1-9). | ||
| * Mirrors the legacy useWorkspaceShortcuts hook but for V2 workspaces. | ||
| */ | ||
| export function useV2WorkspaceShortcuts(groups: V2SidebarProject[]) { |
Contributor
There was a problem hiding this comment.
P2: This hook duplicates the existing workspace shortcut registration logic instead of reusing a shared helper, creating two maintenance points for the same 1-9 hotkey behavior.
(Based on your team's feedback about avoiding duplicated business logic across components.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/useV2WorkspaceShortcuts.ts, line 11:
<comment>This hook duplicates the existing workspace shortcut registration logic instead of reusing a shared helper, creating two maintenance points for the same 1-9 hotkey behavior.
(Based on your team's feedback about avoiding duplicated business logic across components.) </comment>
<file context>
@@ -0,0 +1,55 @@
+ * Keyboard shortcuts for V2 workspace switching (⌘1-9).
+ * Mirrors the legacy useWorkspaceShortcuts hook but for V2 workspaces.
+ */
+export function useV2WorkspaceShortcuts(groups: V2SidebarProject[]) {
+ const navigate = useNavigate();
+
</file context>
| children, | ||
| }: V2WorkspaceContextMenuProps) { | ||
| const handleCopyId = () => { | ||
| navigator.clipboard.writeText(id); |
Contributor
There was a problem hiding this comment.
P2: Handle clipboard.writeText as an async operation; the current code can trigger an unhandled promise rejection and incorrectly shows a success toast on failure.
(Based on your team's feedback about handling async calls with proper await/catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx, line 25:
<comment>Handle `clipboard.writeText` as an async operation; the current code can trigger an unhandled promise rejection and incorrectly shows a success toast on failure.
(Based on your team's feedback about handling async calls with proper await/catch.) </comment>
<file context>
@@ -0,0 +1,52 @@
+ children,
+}: V2WorkspaceContextMenuProps) {
+ const handleCopyId = () => {
+ navigator.clipboard.writeText(id);
+ toast.success("Workspace ID copied");
+ };
</file context>
| <> | ||
| <V2WorkspaceContextMenu | ||
| id={id} | ||
| onRename={startRename} |
Contributor
There was a problem hiding this comment.
P2: When the sidebar is collapsed, clicking 'Rename' updates state but fails to show the <RenameInput>, making the action silently fail visually.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx, line 116:
<comment>When the sidebar is collapsed, clicking 'Rename' updates state but fails to show the `<RenameInput>`, making the action silently fail visually.</comment>
<file context>
@@ -24,44 +56,188 @@ export function V2WorkspaceListItem({
+ <>
+ <V2WorkspaceContextMenu
+ id={id}
+ onRename={startRename}
+ onDelete={() => setIsDeleteDialogOpen(true)}
+ >
</file context>
| <button | ||
| type="button" | ||
| onClick={() => onToggleCollapse(projectId)} | ||
| onDoubleClick={startRename} |
Contributor
There was a problem hiding this comment.
P3: Double-click rename is wired on a button that also toggles collapse on click, so starting rename also triggers collapse toggles.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx, line 244:
<comment>Double-click rename is wired on a button that also toggles collapse on click, so starting rename also triggers collapse toggles.</comment>
<file context>
@@ -21,90 +33,300 @@ export function V2ProjectSection({
+ <button
+ type="button"
+ onClick={() => onToggleCollapse(projectId)}
+ onDoubleClick={startRename}
+ className="flex items-center gap-2 flex-1 min-w-0 py-0.5 text-left cursor-pointer"
+ >
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx (1)
68-73:⚠️ Potential issue | 🟠 MajorAvoid mutating query data with in-place
sort().
Line 68sorts the query-backed array directly, which can mutate shared cached data and produce side effects across consumers.🔧 Proposed fix
- 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); - }); + const branches = useMemo( + () => + [...(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); + }), + [data?.branches, defaultBranch], + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx` around lines 68 - 73, The current assignment to branches calls sort() directly on data?.branches which mutates the query-backed array; instead create a shallow copy before sorting to avoid mutating cached query data (e.g., copy the array from data?.branches then call .sort(...) so references like branches, defaultBranch and data?.branches remain unchanged). Ensure you replace the in-place sort with a non-mutating variant (spread or slice) when computing branches in V2BranchesGroup.apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsx (1)
77-92:⚠️ Potential issue | 🟠 MajorScope existing-workspace detection by branch + device.
Line 152,Line 177, andLine 206currently check existence by branch only, so a workspace on a different device is treated as existing for the selected device. This can show Open when Create is expected and navigate to the wrong workspace.🔧 Proposed fix
const { data: v2WorkspacesData } = useLiveQuery( (q) => q .from({ ws: collections.v2Workspaces }) .where(({ ws }) => eq(ws.projectId, projectId ?? "")) - .select(({ ws }) => ({ id: ws.id, branch: ws.branch })), + .select(({ ws }) => ({ + id: ws.id, + branch: ws.branch, + deviceId: ws.deviceId, + })), [collections, projectId], ); - const workspaceByBranch = useMemo(() => { + const workspaceByBranchAndDevice = useMemo(() => { const map = new Map<string, string>(); for (const w of v2WorkspacesData ?? []) { - map.set(w.branch, w.id); + map.set(`${w.branch.toLowerCase()}::${w.deviceId ?? ""}`, w.id); } return map; }, [v2WorkspacesData]); @@ - const existingId = workspaceByBranch.get(task.slug.toLowerCase()); + const existingId = workspaceByBranchAndDevice.get( + `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`, + ); @@ - {workspaceByBranch.has(task.slug.toLowerCase()) ? ( + {workspaceByBranchAndDevice.has( + `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`, + ) ? ( @@ - {workspaceByBranch.has(task.slug.toLowerCase()) ? "Open" : "Create"}{" "} + {workspaceByBranchAndDevice.has( + `${task.slug.toLowerCase()}::${selectedDeviceId ?? ""}`, + ) + ? "Open" + : "Create"}{" "}Also applies to: 152-163, 177-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsx` around lines 77 - 92, The workspace existence check currently only keys by branch; update the live query and lookup to scope by both branch and deviceId: modify the useLiveQuery in V2IssuesGroup to select deviceId (add deviceId to the select projection from collections.v2Workspaces), change workspaceByBranch to build a combined key (e.g. `${branch}:${deviceId}`) when calling map.set and when looking up workspaces, and update all places that currently consult workspaceByBranch (the checks that use branch-only lookup) to use the combined branch:deviceId key so detection, UI labels (Create/Open) and navigation use branch+device identity.apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx (1)
138-150:⚠️ Potential issue | 🟡 MinorAdd
aria-expandedto the collapsed sidebar toggle for consistency.The expanded sidebar's toggle button (line 281) correctly includes
aria-expanded={!isCollapsed}, but this toggle in the collapsed sidebar view lacks it. Both buttons control the same collapse behavior and should provide consistent accessibility information.🔧 Proposed fix
<button type="button" onClick={() => onToggleCollapse(projectId)} + aria-expanded={!isCollapsed} 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/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx` around lines 138 - 150, The collapsed-sidebar toggle button in V2ProjectSection (the button wrapping V2ProjectThumbnail) is missing the aria-expanded attribute; update that button to include aria-expanded={!isCollapsed} so it matches the expanded-sidebar toggle and consistently exposes the collapse state for screen readers (use the existing onToggleCollapse(projectId) and isCollapsed variable in the component to set the value).
🧹 Nitpick comments (5)
apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx (1)
126-130: RedundantprojectIdnull check.The
projectIdcheck on line 127 is unreachable because the early return at line 98 already handles the!projectIdcase. This check can be safely removed.♻️ Proposed fix
<CommandItem key={pr.id} onSelect={() => { - if (!projectId) { - toast.error("Select a project first"); - return; - } const existingId = workspaceByBranch.get(pr.headBranch);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx` around lines 126 - 130, Remove the redundant projectId null check inside the onSelect handler in V2PullRequestsGroup (the inline onSelect={() => { ... }} callback) because the component already returns early when !projectId; delete the if (!projectId) { toast.error("Select a project first"); return; } block and keep the remaining onSelect logic intact so the handler assumes projectId is defined.apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/DevicePicker.tsx (1)
108-122: Consider usingdevice.idas the CommandItem value.Using
device.nameas the value could cause selection ambiguity if two devices share the same name. The Command component usesvaluefor internal matching and filtering.♻️ Suggested change
{otherDevices.map((device) => ( <CommandItem key={device.id} - value={device.name} + value={device.id} onSelect={() => { onSelectDevice(device.id); setOpen(false); }} > {device.name} {device.id === selectedDeviceId && ( <HiCheck className="ml-auto size-4" /> )} </CommandItem> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/DevicePicker.tsx` around lines 108 - 122, The CommandItem values in DevicePicker are using device.name which can clash for duplicate names; change the value prop passed in the otherDevices.map render of CommandItem to use device.id instead of device.name, keep the displayed text as device.name, and ensure onSelect still calls onSelectDevice(device.id) and setOpen(false) so selection and internal matching use the unique device.id (refer to CommandItem in the otherDevices.map, selectedDeviceId, onSelectDevice, and setOpen).apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/V2WorkspaceSidebar.tsx (1)
22-31: Optional: avoid repeated array copies inprojectShortcutIndicescomputation.This works, but
[...]insidereducereallocates on each iteration.♻️ Suggested refactor
- const projectShortcutIndices = useMemo( - () => - groups.reduce<{ indices: number[]; cumulative: number }>( - (acc, group) => ({ - indices: [...acc.indices, acc.cumulative], - cumulative: acc.cumulative + group.workspaces.length, - }), - { indices: [], cumulative: 0 }, - ).indices, - [groups], - ); + const projectShortcutIndices = useMemo(() => { + const indices: number[] = []; + let cumulative = 0; + for (const group of groups) { + indices.push(cumulative); + cumulative += group.workspaces.length; + } + return indices; + }, [groups]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/V2WorkspaceSidebar.tsx` around lines 22 - 31, The current useMemo computing projectShortcutIndices recreates the indices array on every reduce iteration (spread operator inside the reducer), causing repeated allocations; modify the reducer to mutate the accumulator's indices in-place (e.g., push the current cumulative into acc.indices) or replace the reduce with an explicit for-loop that appends to a local array and returns it, ensuring you still return indices only and keep the same variable name projectShortcutIndices and dependency on groups. This preserves behavior while avoiding the repeated [...acc.indices] allocations.apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsx (1)
130-167: Extract the duplicated dialog/command block into a shared render path.Both branches duplicate the same
<Dialog>content, which makes future behavior changes easy to miss in one branch.Also applies to: 196-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsx` around lines 130 - 167, The Dialog/Command JSX block duplicated in V2SidebarFooter should be extracted into a single reusable render path or small component (e.g., renderAddProjectDialog or AddProjectDialog) and invoked from both branches to avoid drift; move the Dialog, DialogContent, DialogHeader, DialogTitle, DialogDescription, Command, CommandInput, CommandList, CommandEmpty, CommandGroup and CommandItem markup into that function/component and pass the necessary props: isOpen, setIsOpen (or onOpenChange handler), repos/availableRepos, isCreating, and handleSelectRepo so both original locations simply call the shared renderer/component instead of duplicating the JSX.apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.ts (1)
82-92: Consider precomputing workspace metas before sorting.
getWorkspaceMetais invoked inside the sort comparator, which runs O(n log n) times per project. While thesortVersiondependency ensures correctness, fetching metas in a loop is inefficient.♻️ Precompute metas to avoid repeated getState calls
.map((project) => { const meta = projectMetas[project.id] ?? DEFAULT_META; const repoId = project.githubRepositoryId ?? null; + const projectWorkspaces = workspacesByProject.get(project.id) ?? []; + const getMeta = + useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta; + const workspacesWithMeta = projectWorkspaces.map((w) => ({ + ...w, + tabOrder: getMeta(w.id).tabOrder, + })); return { id: project.id, name: project.name, slug: project.slug, githubRepositoryId: repoId, githubOwner: repoId ? (repoOwnerMap.get(repoId) ?? null) : null, createdAt: project.createdAt, updatedAt: project.updatedAt, isCollapsed: meta.isCollapsed, - workspaces: (workspacesByProject.get(project.id) ?? []).sort( - (a, b) => { - const getMeta = - useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta; - const orderA = getMeta(a.id).tabOrder; - const orderB = getMeta(b.id).tabOrder; - const orderDiff = orderA - orderB; - if (orderDiff !== 0) return orderDiff; - return a.name.localeCompare(b.name); - }, - ), + workspaces: workspacesWithMeta.sort((a, b) => { + const orderDiff = a.tabOrder - b.tabOrder; + if (orderDiff !== 0) return orderDiff; + 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/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.ts` around lines 82 - 92, The comparator currently calls useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta repeatedly during sorting (in the workspaces sort for workspacesByProject), which is inefficient; before calling .sort on (workspacesByProject.get(project.id) ?? []), capture a local reference to getWorkspaceMeta (e.g., const getMeta = useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta) and precompute a small map of workspaceId -> meta for that project's workspaces (or at least fetch each workspace's meta once into an array of {workspace, meta}) and then sort using those cached metas in the comparator (falling back to a.name.localeCompare(b.name) when tabOrder ties) so the comparator no longer calls getWorkspaceMeta repeatedly.
🤖 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/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx`:
- Around line 49-64: The workspace existence/map logic only keys by branch,
causing cross-device collisions; update the useLiveQuery and workspaceByBranch
to include the selected device identifier so keys are branch+device (e.g., use
the device id or selectedDevice variable in the query where clause and in the
Map key), and then update any lookups that use workspaceByBranch (the places
around V2BranchesGroup's workspaceByBranch creation and the other uses noted at
lines ~75-80 and ~135-143) to build/lookup using the same composite key (branch
+ deviceId) so Open/Create and navigation target the correct device-scoped
workspace.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx`:
- Around line 132-153: handleCreate can be invoked multiple times via keyboard
even though the button is disabled; add an isPending boolean state (e.g.,
useState(false)) and guard the start of handleCreate to return early if
isPending is true, set isPending = true just before calling
runAsyncAction(createWorkspace(...)) and clear isPending on both success and
error in the runAsyncAction callbacks (or finally) so duplicate workspace
creation is prevented; reference the existing handleCreate, createWorkspace and
runAsyncAction symbols and ensure the button’s disabled prop stays tied to
isPending as well.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx`:
- Around line 26-29: The handleCopyId function currently calls
navigator.clipboard.writeText(id) without awaiting or handling errors, causing
toast.success("Project ID copied") to show even on failure; update handleCopyId
to await navigator.clipboard.writeText(id) inside a try/catch, call
toast.success only on successful await, and call toast.error (or a failure
message) in the catch branch; optionally add a fallback copy method if
navigator.clipboard is unavailable and ensure you reference handleCopyId,
navigator.clipboard.writeText, and toast.success/toast.error when making the
change.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectThumbnail/V2ProjectThumbnail.tsx`:
- Around line 19-24: The component keeps imageError true after a failed avatar
load, causing permanent fallback; inside V2ProjectThumbnail add logic to reset
imageError when the avatar source changes by using an effect that calls
setImageError(false) whenever githubOwner (or the computed avatar URL) changes
so the component will retry rendering the image; reference the imageError state
and setImageError setter and the githubOwner prop/variable to locate where to
add the useEffect.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsx`:
- Around line 107-113: The icon-only Button wrapping LuFolderPlus in
V2SidebarFooter lacks an accessible name; add an aria-label (e.g.,
aria-label="Add project") to that Button element (the Button containing
<LuFolderPlus />) so screen readers can announce its purpose—if your project
uses i18n, use the localized string instead of a hardcoded label.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx`:
- Around line 25-33: The icon-only "New Workspace" button in V2SidebarHeader is
missing an accessible name; update the button element that calls openModal (the
icon-only trigger containing LuPlus) to include an accessible name by adding an
aria-label or aria-labelledby (e.g., aria-label="Create new workspace" or
referencing a visually-hidden label), ensuring screen readers can discover and
announce the button while preserving existing classes and behavior.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx`:
- Around line 15-31: The children prop on V2WorkspaceContextMenu is typed too
broadly as React.ReactNode but ContextMenuTrigger uses the asChild contract
which requires a single React element; update V2WorkspaceContextMenuProps to
type children as React.ReactElement (or ReactElement<Props> if you need specific
props) and adjust any callers if necessary so they pass a single element to
V2WorkspaceContextMenu; this change touches the V2WorkspaceContextMenu component
and its V2WorkspaceContextMenuProps type and prevents fragments/arrays/null from
being passed into ContextMenuTrigger asChild.
- Around line 24-27: The handleCopyId function calls
navigator.clipboard.writeText(id) without awaiting or handling failures, so the
success toast is shown even if the clipboard write fails; update handleCopyId in
V2WorkspaceContextMenu.tsx (and the analogous handler in
V2ProjectContextMenu.tsx) to await navigator.clipboard.writeText(id) inside a
try/catch (or attach .then/.catch), only call toast.success when the write
resolves, and call toast.error (with a helpful message) when it rejects or if
the Clipboard API is unavailable.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2ProjectDnD/useV2ProjectDnD.ts`:
- Around line 48-54: The end handler in useV2ProjectDnD currently commits
reorders whenever the index changed even if the drag was cancelled; update the
end callback to accept the second argument monitor and bail out early with if
(monitor.didDrop()) return; before computing ids/splice/commitOrder, mirroring
the guard used in WorkspaceSection.tsx and ProjectSection.tsx; apply the same
change to the end handler in useV2WorkspaceDnD (the corresponding end at the
mentioned location) so canceled drops do not persist unintended order changes.
---
Duplicate comments:
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx`:
- Around line 68-73: The current assignment to branches calls sort() directly on
data?.branches which mutates the query-backed array; instead create a shallow
copy before sorting to avoid mutating cached query data (e.g., copy the array
from data?.branches then call .sort(...) so references like branches,
defaultBranch and data?.branches remain unchanged). Ensure you replace the
in-place sort with a non-mutating variant (spread or slice) when computing
branches in V2BranchesGroup.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsx`:
- Around line 77-92: The workspace existence check currently only keys by
branch; update the live query and lookup to scope by both branch and deviceId:
modify the useLiveQuery in V2IssuesGroup to select deviceId (add deviceId to the
select projection from collections.v2Workspaces), change workspaceByBranch to
build a combined key (e.g. `${branch}:${deviceId}`) when calling map.set and
when looking up workspaces, and update all places that currently consult
workspaceByBranch (the checks that use branch-only lookup) to use the combined
branch:deviceId key so detection, UI labels (Create/Open) and navigation use
branch+device identity.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsx`:
- Around line 138-150: The collapsed-sidebar toggle button in V2ProjectSection
(the button wrapping V2ProjectThumbnail) is missing the aria-expanded attribute;
update that button to include aria-expanded={!isCollapsed} so it matches the
expanded-sidebar toggle and consistently exposes the collapse state for screen
readers (use the existing onToggleCollapse(projectId) and isCollapsed variable
in the component to set the value).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/DevicePicker.tsx`:
- Around line 108-122: The CommandItem values in DevicePicker are using
device.name which can clash for duplicate names; change the value prop passed in
the otherDevices.map render of CommandItem to use device.id instead of
device.name, keep the displayed text as device.name, and ensure onSelect still
calls onSelectDevice(device.id) and setOpen(false) so selection and internal
matching use the unique device.id (refer to CommandItem in the otherDevices.map,
selectedDeviceId, onSelectDevice, and setOpen).
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsx`:
- Around line 126-130: Remove the redundant projectId null check inside the
onSelect handler in V2PullRequestsGroup (the inline onSelect={() => { ... }}
callback) because the component already returns early when !projectId; delete
the if (!projectId) { toast.error("Select a project first"); return; } block and
keep the remaining onSelect logic intact so the handler assumes projectId is
defined.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsx`:
- Around line 130-167: The Dialog/Command JSX block duplicated in
V2SidebarFooter should be extracted into a single reusable render path or small
component (e.g., renderAddProjectDialog or AddProjectDialog) and invoked from
both branches to avoid drift; move the Dialog, DialogContent, DialogHeader,
DialogTitle, DialogDescription, Command, CommandInput, CommandList,
CommandEmpty, CommandGroup and CommandItem markup into that function/component
and pass the necessary props: isOpen, setIsOpen (or onOpenChange handler),
repos/availableRepos, isCreating, and handleSelectRepo so both original
locations simply call the shared renderer/component instead of duplicating the
JSX.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.ts`:
- Around line 82-92: The comparator currently calls
useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta repeatedly during
sorting (in the workspaces sort for workspacesByProject), which is inefficient;
before calling .sort on (workspacesByProject.get(project.id) ?? []), capture a
local reference to getWorkspaceMeta (e.g., const getMeta =
useV2WorkspaceLocalMetaStore.getState().getWorkspaceMeta) and precompute a small
map of workspaceId -> meta for that project's workspaces (or at least fetch each
workspace's meta once into an array of {workspace, meta}) and then sort using
those cached metas in the comparator (falling back to
a.name.localeCompare(b.name) when tabOrder ties) so the comparator no longer
calls getWorkspaceMeta repeatedly.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/V2WorkspaceSidebar.tsx`:
- Around line 22-31: The current useMemo computing projectShortcutIndices
recreates the indices array on every reduce iteration (spread operator inside
the reducer), causing repeated allocations; modify the reducer to mutate the
accumulator's indices in-place (e.g., push the current cumulative into
acc.indices) or replace the reduce with an explicit for-loop that appends to a
local array and returns it, ensuring you still return indices only and keep the
same variable name projectShortcutIndices and dependency on groups. This
preserves behavior while avoiding the repeated [...acc.indices] allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc47720b-07e2-483e-8a53-c1c3faa1c2be
📒 Files selected for processing (32)
apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/DevicePicker/DevicePicker.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2IssuesGroup/V2IssuesGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2NewWorkspaceModalContent/V2NewWorkspaceModalContent.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsxapps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PullRequestsGroup/V2PullRequestsGroup.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/utils/workspace-navigation.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace.$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/layout.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/V2WorkspaceSidebar.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2DeleteDialog/V2DeleteDialog.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2DeleteDialog/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectSection.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectThumbnail/V2ProjectThumbnail.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectThumbnail/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarEmptyState/V2SidebarEmptyState.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsxapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2ProjectDnD/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2ProjectDnD/useV2ProjectDnD.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2SidebarData/useV2SidebarData.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceDnD/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceDnD/useV2WorkspaceDnD.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/index.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/useV2WorkspaceShortcuts.tsapps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/types.tsapps/desktop/src/renderer/stores/v2-workspace-local-meta.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectThumbnail/index.ts
- apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2WorkspaceShortcuts/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2ProjectSelector/V2ProjectSelector.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace.$workspaceId/page.tsx
- apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceListItem.tsx
Comment on lines
+49
to
+64
| const { data: v2WorkspacesData } = useLiveQuery( | ||
| (q) => | ||
| q | ||
| .from({ ws: collections.v2Workspaces }) | ||
| .where(({ ws }) => eq(ws.projectId, projectId ?? "")) | ||
| .select(({ ws }) => ({ id: ws.id, branch: ws.branch })), | ||
| [collections, projectId], | ||
| ); | ||
|
|
||
| const workspaceByBranch = useMemo(() => { | ||
| const map = new Map<string, string>(); | ||
| for (const w of v2WorkspacesData ?? []) { | ||
| map.set(w.branch, w.id); | ||
| } | ||
| return map; | ||
| }, [v2WorkspacesData]); |
Contributor
There was a problem hiding this comment.
Branch existence should be keyed by branch + selected device.
Current lookups only use branch name, so workspaces from other devices can be treated as existing in this device context (wrong Open/Create behavior and wrong navigation target).
🔧 Proposed fix
const { data: v2WorkspacesData } = useLiveQuery(
(q) =>
q
.from({ ws: collections.v2Workspaces })
.where(({ ws }) => eq(ws.projectId, projectId ?? ""))
- .select(({ ws }) => ({ id: ws.id, branch: ws.branch })),
+ .select(({ ws }) => ({
+ id: ws.id,
+ branch: ws.branch,
+ deviceId: ws.deviceId,
+ })),
[collections, projectId],
);
- const workspaceByBranch = useMemo(() => {
+ const workspaceByBranchAndDevice = useMemo(() => {
const map = new Map<string, string>();
for (const w of v2WorkspacesData ?? []) {
- map.set(w.branch, w.id);
+ map.set(`${w.branch}::${w.deviceId ?? ""}`, w.id);
}
return map;
}, [v2WorkspacesData]);
@@
- existingWorkspaceId: workspaceByBranch.get(branch.name),
+ existingWorkspaceId: workspaceByBranchAndDevice.get(
+ `${branch.name}::${selectedDeviceId ?? ""}`,
+ ),
@@
- const existingId = workspaceByBranch.get(branchName);
+ const existingId = workspaceByBranchAndDevice.get(
+ `${branchName}::${selectedDeviceId ?? ""}`,
+ );
@@
- [handleCreate, handleOpen, workspaceByBranch],
+ [handleCreate, handleOpen, workspaceByBranchAndDevice, selectedDeviceId],Also applies to: 75-80, 135-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2BranchesGroup/V2BranchesGroup.tsx`
around lines 49 - 64, The workspace existence/map logic only keys by branch,
causing cross-device collisions; update the useLiveQuery and workspaceByBranch
to include the selected device identifier so keys are branch+device (e.g., use
the device id or selectedDevice variable in the query where clause and in the
Map key), and then update any lookups that use workspaceByBranch (the places
around V2BranchesGroup's workspaceByBranch creation and the other uses noted at
lines ~75-80 and ~135-143) to build/lookup using the same composite key (branch
+ deviceId) so Open/Create and navigation target the correct device-scoped
workspace.
Comment on lines
+132
to
+153
| const handleCreate = () => { | ||
| if (!projectId) { | ||
| toast.error("Select a project first"); | ||
| return; | ||
| } | ||
| const name = branchSlug || trimmedPrompt || "workspace"; | ||
| const branch = branchPreview || "workspace"; | ||
| void runAsyncAction( | ||
| createWorkspace({ | ||
| projectId, | ||
| name, | ||
| branch, | ||
| deviceId: selectedDeviceId ?? undefined, | ||
| }), | ||
| { | ||
| loading: "Creating workspace...", | ||
| success: "Workspace created", | ||
| error: (err) => | ||
| err instanceof Error ? err.message : "Failed to create workspace", | ||
| }, | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
Block keyboard submit while create is already pending.
Line 225 disables the button, but Line 188-191 can still trigger handleCreate() repeatedly. Add an isPending guard to prevent duplicate workspace creation requests.
🔧 Proposed fix
const handleCreate = () => {
+ if (isPending) return;
if (!projectId) {
toast.error("Select a project first");
return;
}
@@
- onKeyDown={(e) => {
- if (e.key === "Enter" && (e.metaKey || e.ctrlKey)) {
+ onKeyDown={(e) => {
+ if (!isPending && e.key === "Enter" && (e.metaKey || e.ctrlKey)) {
e.preventDefault();
handleCreate();
}
}}Also applies to: 187-191, 225-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/components/V2NewWorkspaceModal/components/V2PromptGroup/V2PromptGroup.tsx`
around lines 132 - 153, handleCreate can be invoked multiple times via keyboard
even though the button is disabled; add an isPending boolean state (e.g.,
useState(false)) and guard the start of handleCreate to return early if
isPending is true, set isPending = true just before calling
runAsyncAction(createWorkspace(...)) and clear isPending on both success and
error in the runAsyncAction callbacks (or finally) so duplicate workspace
creation is prevented; reference the existing handleCreate, createWorkspace and
runAsyncAction symbols and ensure the button’s disabled prop stays tied to
isPending as well.
Comment on lines
+26
to
+29
| const handleCopyId = () => { | ||
| navigator.clipboard.writeText(id); | ||
| toast.success("Project ID copied"); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx" | head -50Repository: superset-sh/superset
Length of output: 1679
Handle clipboard failures before showing success toast.
writeText is async; currently success is shown even if copy fails.
✅ 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("Failed to copy Project ID");
+ }
+ };📝 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 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("Failed to 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/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsx`
around lines 26 - 29, The handleCopyId function currently calls
navigator.clipboard.writeText(id) without awaiting or handling errors, causing
toast.success("Project ID copied") to show even on failure; update handleCopyId
to await navigator.clipboard.writeText(id) inside a try/catch, call
toast.success only on successful await, and call toast.error (or a failure
message) in the catch branch; optionally add a fallback copy method if
navigator.clipboard is unavailable and ensure you reference handleCopyId,
navigator.clipboard.writeText, and toast.success/toast.error when making the
change.
Comment on lines
+19
to
+24
| const [imageError, setImageError] = useState(false); | ||
|
|
||
| const firstLetter = projectName.charAt(0).toUpperCase(); | ||
|
|
||
| if (githubOwner && !imageError) { | ||
| return ( |
Contributor
There was a problem hiding this comment.
Reset imageError when avatar source changes.
Line 19/Line 23 can leave the component permanently in fallback mode after one failed image load. Resetting on githubOwner change prevents stale thumbnail state.
💡 Proposed fix
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
const [imageError, setImageError] = useState(false);
+
+ useEffect(() => {
+ setImageError(false);
+ }, [githubOwner]);📝 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 [imageError, setImageError] = useState(false); | |
| const firstLetter = projectName.charAt(0).toUpperCase(); | |
| if (githubOwner && !imageError) { | |
| return ( | |
| const [imageError, setImageError] = useState(false); | |
| useEffect(() => { | |
| setImageError(false); | |
| }, [githubOwner]); | |
| const firstLetter = projectName.charAt(0).toUpperCase(); | |
| if (githubOwner && !imageError) { | |
| return ( |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectThumbnail/V2ProjectThumbnail.tsx`
around lines 19 - 24, The component keeps imageError true after a failed avatar
load, causing permanent fallback; inside V2ProjectThumbnail add logic to reset
imageError when the avatar source changes by using an effect that calls
setImageError(false) whenever githubOwner (or the computed avatar URL) changes
so the component will retry rendering the image; reference the imageError state
and setImageError setter and the githubOwner prop/variable to locate where to
add the useEffect.
Comment on lines
+107
to
+113
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="size-8 text-muted-foreground hover:text-foreground" | ||
| > | ||
| <LuFolderPlus className="size-4" strokeWidth={1.5} /> | ||
| </Button> |
Contributor
There was a problem hiding this comment.
Add an aria-label for the collapsed “Add project” icon button.
This is an icon-only actionable control and currently lacks an accessible name.
✅ Suggested fix
<Button
variant="ghost"
size="icon"
+ aria-label="Add project"
className="size-8 text-muted-foreground 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
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="size-8 text-muted-foreground hover:text-foreground" | |
| > | |
| <LuFolderPlus className="size-4" strokeWidth={1.5} /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| aria-label="Add project" | |
| className="size-8 text-muted-foreground hover:text-foreground" | |
| > | |
| <LuFolderPlus className="size-4" strokeWidth={1.5} /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarFooter/V2SidebarFooter.tsx`
around lines 107 - 113, The icon-only Button wrapping LuFolderPlus in
V2SidebarFooter lacks an accessible name; add an aria-label (e.g.,
aria-label="Add project") to that Button element (the Button containing
<LuFolderPlus />) so screen readers can announce its purpose—if your project
uses i18n, use the localized string instead of a hardcoded label.
Comment on lines
+25
to
+33
| <button | ||
| type="button" | ||
| onClick={() => openModal()} | ||
| className="group flex items-center justify-center size-8 rounded-md bg-accent/40 hover:bg-accent/60 transition-colors" | ||
| > | ||
| <div className="flex items-center justify-center size-5 rounded bg-accent"> | ||
| <LuPlus className="size-3" strokeWidth={2} /> | ||
| </div> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible name to the collapsed icon button.
The icon-only “New Workspace” trigger has no aria-label, so it is not discoverable by screen readers.
✅ Suggested fix
<button
type="button"
onClick={() => openModal()}
+ aria-label={`New Workspace (${shortcutText})`}
className="group flex items-center justify-center size-8 rounded-md bg-accent/40 hover:bg-accent/60 transition-colors"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2SidebarHeader/V2SidebarHeader.tsx`
around lines 25 - 33, The icon-only "New Workspace" button in V2SidebarHeader is
missing an accessible name; update the button element that calls openModal (the
icon-only trigger containing LuPlus) to include an accessible name by adding an
aria-label or aria-labelledby (e.g., aria-label="Create new workspace" or
referencing a visually-hidden label), ensuring screen readers can discover and
announce the button while preserving existing classes and behavior.
Comment on lines
+15
to
+31
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export function V2WorkspaceContextMenu({ | ||
| id, | ||
| onRename, | ||
| onDelete, | ||
| children, | ||
| }: V2WorkspaceContextMenuProps) { | ||
| const handleCopyId = () => { | ||
| navigator.clipboard.writeText(id); | ||
| toast.success("Workspace ID copied"); | ||
| }; | ||
|
|
||
| return ( | ||
| <ContextMenu> | ||
| <ContextMenuTrigger asChild>{children}</ContextMenuTrigger> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current usages provide a single JSX child element.
rg -n --type=tsx -C3 '<V2WorkspaceContextMenu\b|</V2WorkspaceContextMenu>'Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for usages of V2WorkspaceContextMenu with correct file glob
rg -g "*.tsx" -g "*.ts" -C3 'V2WorkspaceContextMenu' --no-headingRepository: superset-sh/superset
Length of output: 8141
🏁 Script executed:
#!/bin/bash
# Read the full V2WorkspaceContextMenu.tsx file
cat apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsxRepository: superset-sh/superset
Length of output: 1301
Narrow children type to a single React element for asChild contract.
Line 31 uses ContextMenuTrigger asChild, which requires exactly one element child. The current type React.ReactNode at line 14 is too broad and allows fragments, arrays, and null—none of which work with asChild. Narrowing to ReactElement enforces the contract and prevents runtime errors.
Suggested fix
+import type { ReactElement } from "react";
+
interface V2WorkspaceContextMenuProps {
id: string;
onRename: () => void;
onDelete: () => void;
- children: React.ReactNode;
+ children: ReactElement;
}📝 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
| children: React.ReactNode; | |
| } | |
| export function V2WorkspaceContextMenu({ | |
| id, | |
| onRename, | |
| onDelete, | |
| children, | |
| }: V2WorkspaceContextMenuProps) { | |
| const handleCopyId = () => { | |
| navigator.clipboard.writeText(id); | |
| toast.success("Workspace ID copied"); | |
| }; | |
| return ( | |
| <ContextMenu> | |
| <ContextMenuTrigger asChild>{children}</ContextMenuTrigger> | |
| import type { ReactElement } from "react"; | |
| interface V2WorkspaceContextMenuProps { | |
| id: string; | |
| onRename: () => void; | |
| onDelete: () => void; | |
| children: ReactElement; | |
| } | |
| export function V2WorkspaceContextMenu({ | |
| id, | |
| onRename, | |
| onDelete, | |
| children, | |
| }: V2WorkspaceContextMenuProps) { | |
| const handleCopyId = () => { | |
| navigator.clipboard.writeText(id); | |
| toast.success("Workspace ID copied"); | |
| }; | |
| return ( | |
| <ContextMenu> | |
| <ContextMenuTrigger asChild>{children}</ContextMenuTrigger> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx`
around lines 15 - 31, The children prop on V2WorkspaceContextMenu is typed too
broadly as React.ReactNode but ContextMenuTrigger uses the asChild contract
which requires a single React element; update V2WorkspaceContextMenuProps to
type children as React.ReactElement (or ReactElement<Props> if you need specific
props) and adjust any callers if necessary so they pass a single element to
V2WorkspaceContextMenu; this change touches the V2WorkspaceContextMenu component
and its V2WorkspaceContextMenuProps type and prevents fragments/arrays/null from
being passed into ContextMenuTrigger asChild.
Comment on lines
+24
to
+27
| const handleCopyId = () => { | ||
| navigator.clipboard.writeText(id); | ||
| toast.success("Workspace ID copied"); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check clipboard write call sites and whether failures are handled consistently.
rg -n --type=ts --type=tsx -C2 'navigator\.clipboard\.writeText\('Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx | head -50Repository: superset-sh/superset
Length of output: 1645
🏁 Script executed:
# Search for clipboard usage with corrected syntax
rg -n 'navigator\.clipboard\.writeText' apps/desktop/src --type tsRepository: superset-sh/superset
Length of output: 5230
🏁 Script executed:
# Also search the entire apps/desktop for all clipboard usage
rg -n 'clipboard\.' apps/desktop/srcRepository: superset-sh/superset
Length of output: 6112
🏁 Script executed:
# Check ContextMenuItem type definition and what onSelect handler expects
fd -t ts -t tsx '@superset/ui' apps/desktop | head -5Repository: superset-sh/superset
Length of output: 233
🏁 Script executed:
# Look for ContextMenuItem definition or type
rg -n 'export.*ContextMenuItem' apps/desktop/src --type ts -A3Repository: superset-sh/superset
Length of output: 1407
🏁 Script executed:
# Check how other handlers like onRename and onDelete are defined/used
cat -n apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx | sed -n '11,50p'Repository: superset-sh/superset
Length of output: 1326
🏁 Script executed:
# Check ContextMenuItem from `@superset/ui` to see what type onSelect expects
rg -n 'onSelect' apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx -B2 -A2Repository: superset-sh/superset
Length of output: 551
🏁 Script executed:
# Search for ContextMenuItem in node_modules or type definitions
find apps/desktop/node_modules/@superset/ui -name "*.d.ts" -o -name "*.tsx" | head -10Repository: superset-sh/superset
Length of output: 137
🏁 Script executed:
# Check if other async handlers are used with onSelect
rg -n 'onSelect=.*async' apps/desktop/src --type ts -B2 -A2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check how similar clipboard operations handle async in ContextMenu items
rg -n 'ContextMenuItem.*onSelect' apps/desktop/src -A5 -B1 | grep -A5 -B1 'clipboard'Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Find where ContextMenu components are defined
rg -n 'from.*context-menu' apps/desktop/src --type ts | head -5Repository: superset-sh/superset
Length of output: 843
🏁 Script executed:
# Check if there are type stubs or source for the UI library
find apps/desktop -type f -name "*.d.ts" | xargs rg -l "ContextMenuItem" 2>/dev/null | head -5Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look at the complete V2ProjectContextMenu to see the second occurrence
cat -n apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2ProjectSection/V2ProjectContextMenu.tsxRepository: superset-sh/superset
Length of output: 1883
🏁 Script executed:
# Check examples of working async clipboard handlers in context menus
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/TabView/FileViewerPane/components/DiffViewerContextMenu/DiffViewerContextMenu.tsx | sed -n '95,110p'Repository: superset-sh/superset
Length of output: 544
🏁 Script executed:
# Check if async handlers with void wrapper are used elsewhere in the codebase
rg -n 'onSelect.*void' apps/desktop/src --type ts -A1 -B1 | head -30Repository: superset-sh/superset
Length of output: 3557
🏁 Script executed:
# Check the CommitItem pattern which uses void on line 40
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitItem/CommitItem.tsx | sed -n '35,45p'Repository: superset-sh/superset
Length of output: 322
🏁 Script executed:
# Look for any async onSelect handlers
rg -n 'onSelect=\{.*async' apps/desktop/src --type ts -A2 -B2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if async handlers wrapped with void are used elsewhere with onSelect
rg -n 'onSelect.*void.*async\|onSelect=.*\(\)\s*=>.*void.*async' apps/desktop/src --type ts -A2Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Verify the actual impact by checking if any clipboard operations fail silently in tests
rg -n 'clipboard.*writeText' apps/desktop/src --type ts -B3 | grep -E '(test|spec|mock|fail|error)' | head -10Repository: superset-sh/superset
Length of output: 1583
Handle clipboard failures before showing success toast.
The navigator.clipboard.writeText(id) call on line 25 returns an unhandled Promise. Line 26 always shows a success toast regardless of whether the clipboard operation succeeds, which provides false feedback to the user.
Suggested fix
- const handleCopyId = () => {
- navigator.clipboard.writeText(id);
- toast.success("Workspace ID copied");
- };
+ const handleCopyId = async () => {
+ try {
+ await navigator.clipboard.writeText(id);
+ toast.success("Workspace ID copied");
+ } catch {
+ toast.error("Failed to copy workspace ID");
+ }
+ };
...
- <ContextMenuItem onSelect={handleCopyId}>
+ <ContextMenuItem onSelect={() => void handleCopyId()}>This same pattern applies to V2ProjectContextMenu.tsx (lines 26-29).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/components/V2WorkspaceListItem/V2WorkspaceContextMenu.tsx`
around lines 24 - 27, The handleCopyId function calls
navigator.clipboard.writeText(id) without awaiting or handling failures, so the
success toast is shown even if the clipboard write fails; update handleCopyId in
V2WorkspaceContextMenu.tsx (and the analogous handler in
V2ProjectContextMenu.tsx) to await navigator.clipboard.writeText(id) inside a
try/catch (or attach .then/.catch), only call toast.success when the write
resolves, and call toast.error (with a helpful message) when it rejects or if
the Clipboard API is unavailable.
Comment on lines
+48
to
+54
| end: (item) => { | ||
| if (!item || item.originalIndex === item.index) return; | ||
| const ids = [...projectIds]; | ||
| const [moved] = ids.splice(item.originalIndex, 1); | ||
| ids.splice(item.index, 0, moved); | ||
| commitOrder(ids); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify local DnD end handlers and whether they guard with monitor.didDrop()
rg -n -C3 --type=ts --type=tsx 'end:\s*\(' apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks
rg -n -C3 --type=ts --type=tsx '\bdidDrop\s*\(' apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooksRepository: superset-sh/superset
Length of output: 119
🌐 Web query:
For react-dnd v16, what arguments are provided to the useDrag spec endcallback, and ismonitor.didDrop() the recommended way to avoid handling canceled drops?
💡 Result:
In react-dnd v16, the useDrag spec end callback is called with two arguments:
end(draggedItem, monitor)— wheredraggedItemis the drag item produced by youritem(oritemfactory), andmonitoris aDragSourceMonitor<DragObject, DropResult>. [1]
To avoid handling canceled/unhandled drops (dropped outside a compatible target), yes—the documented/standard pattern is to guard your logic with:
if (!monitor.didDrop()) return;[2][3]
Then you can read the target’s return value (if any) via monitor.getDropResult() (commonly null/undefined when no drop was handled). [3]
🏁 Script executed:
#!/bin/bash
# First, read the actual file to confirm current code state
cat -n apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2ProjectDnD/useV2ProjectDnD.ts | head -80
# Then search for other useDrag end handlers in the codebase
echo "---"
echo "Searching for other DnD end handlers:"
rg -n -C2 'end:\s*\(' apps/desktop/src/renderer/screens/main/components/ -t ts 2>/dev/null || rg -n -C2 'end:\s*\(' apps/desktop/src/renderer/screens/main/components/
# Check for didDrop usage patterns in the codebase
echo "---"
echo "Searching for didDrop usage:"
rg -n 'didDrop' apps/desktop/src/renderer/ -t ts 2>/dev/null || rg -n 'didDrop' apps/desktop/src/renderer/Repository: superset-sh/superset
Length of output: 10771
Guard reorder commits with monitor.didDrop() in end handler.
Line 48 currently persists reorder whenever item.index changed via hover, even if the drag is released outside a valid drop target. This commits unintended order changes on canceled drags. Other similar DnD handlers in the codebase (e.g., WorkspaceSection.tsx, ProjectSection.tsx) correctly use if (monitor.didDrop()) return; to avoid this.
The same issue exists in useV2WorkspaceDnD.ts at line 56.
Proposed fix
- end: (item) => {
- if (!item || item.originalIndex === item.index) return;
+ end: (item, monitor) => {
+ if (!monitor.didDrop() || !item || item.originalIndex === item.index)
+ return;
const ids = [...projectIds];
const [moved] = ids.splice(item.originalIndex, 1);
ids.splice(item.index, 0, moved);
commitOrder(ids);
},📝 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
| end: (item) => { | |
| if (!item || item.originalIndex === item.index) return; | |
| const ids = [...projectIds]; | |
| const [moved] = ids.splice(item.originalIndex, 1); | |
| ids.splice(item.index, 0, moved); | |
| commitOrder(ids); | |
| }, | |
| end: (item, monitor) => { | |
| if (!monitor.didDrop() || !item || item.originalIndex === item.index) | |
| return; | |
| const ids = [...projectIds]; | |
| const [moved] = ids.splice(item.originalIndex, 1); | |
| ids.splice(item.index, 0, moved); | |
| commitOrder(ids); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/screens/main/components/V2WorkspaceSidebar/hooks/useV2ProjectDnD/useV2ProjectDnD.ts`
around lines 48 - 54, The end handler in useV2ProjectDnD currently commits
reorders whenever the index changed even if the drag was cancelled; update the
end callback to accept the second argument monitor and bail out early with if
(monitor.didDrop()) return; before computing ids/splice/commitOrder, mirroring
the guard used in WorkspaceSection.tsx and ProjectSection.tsx; apply the same
change to the end handler in useV2WorkspaceDnD (the corresponding end at the
mentioned location) so canceled drops do not persist unintended order changes.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useV2SidebarData()hook to join Electric collections with local meta for collapse state and tab orderingV2SidebarHeader,V2ProjectSection(with framer-motion collapse animation),V2WorkspaceListItem(with active route highlight),V2SidebarEmptyState/v2-workspace/$workspaceIdshowing workspace details from ElectricV2_CLOUDfeature flag is enabled; legacy sidebar unchanged when flag is offTest plan
V2_CLOUDfeature flag → V2 sidebar renders with project sections and workspace itemsV2_CLOUDflag → legacy sidebar renders unchangedbun run --cwd apps/desktop typecheckpassesbun run lintpassesSummary by cubic
Adds the V2 workspace sidebar and V2 new workspace modal behind the
V2_CLOUDflag. Implements SUPER-362 and SUPER-363 with a polished sidebar, creation flow, and a V2 workspace route.New Features
zustand,useV2SidebarData()merges Electric + local meta, header, collapsible project sections, workspace items, empty state. Adds collapsed mode, ⌘1–9 shortcuts, project thumbnails, inline rename, context menus, drag-and-drop reorder, “+ workspace” on project headers, and a footer with GitHub repo picker and dropdown.V2WorkspaceSidebarand authenticated layout rendersV2NewWorkspaceModalwhen enabled. AddsnavigateToV2Workspaceand a guarded/v2-workspace/$workspaceIdroute.Bug Fixes
preSelectedProjectId; stabilizecreateWorkspace.mainRepoPathbasename.Written for commit 8a5093a. Summary will update on new commits.
Summary by CodeRabbit