feat(desktop): roomier v2 workspace pickers + clearer branch actions#4226
Merged
Conversation
Widen the PR/issue/task pickers from 320px to 440px and switch each row to a Linear-style two-line layout (title on top, monospace id + state below) so long titles aren't clipped. Branch picker matches the new width and gets a "Select base branch…" placeholder when no base is set. For each branch row, surface exactly one action that names what it does: "Open workspace" when a tracked workspace exists, "Create workspace" otherwise. Drops the misleading "Create"/"Check out" pairing and the disabled-tooltip state — every row is now actionable.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFour picker/command components standardize their UI: IssueLinkCommand, GitHubIssueLinkCommand, PRLinkCommand, and CompareBaseBranchPicker all increase popover width to ChangesPicker UI Standardization and Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Branch picker now mirrors the PR/Issue/Task two-line item layout: top row is the branch name in mono, bottom row is the meta strip (relative commit time · default · remote · worktree). Action button and the selected-base check moved to the right column.
Contributor
Greptile SummaryThis PR widens four command-palette pickers (PR, GitHub issue, Linear task, and compare-base branch) from
Confidence Score: 4/5Safe to merge; the layout and width changes are low-risk UI updates, and the branch-action simplification is intentional and well-commented. The only behavioral change beyond styling is in CompareBaseBranchPicker: removing the disabled Check out state means users clicking Create workspace on an already-checked-out branch get an error toast instead of a pre-click indicator. The author explicitly acknowledges this, and the handler is expected to recover gracefully. CompareBaseBranchPicker.tsx deserves a second look around the isCheckedOut && !isWorktree case; the other three files are purely cosmetic.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx | Widened popover to 440px, replaced three-state branch action (Open/Create/disabled Check out) with a simpler two-state (Open workspace / Create workspace) using hasWorkspaceForBranch; dropped TooltipProvider. Already-checked-out branches now show an active "Create workspace" that will fail at the handler level. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx | Widened popover to 440px and reformatted PR rows to two-line Linear style (title + mono id·state). No logic changes. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsx | Widened popover to 440px and reformatted GitHub issue rows to two-line style with extracted state variable. No logic changes. |
| apps/desktop/src/renderer/components/Chat/ChatInterface/components/IssueLinkCommand/IssueLinkCommand.tsx | Widened popover to 440px and reformatted Linear task rows to two-line style; status icon wrapped in a fixed-size container, slug and status.type shown on the second line. No logic changes. |
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx:198-230
**"Create workspace" enabled for already-checked-out branches**
The new two-case logic shows an active "Create workspace" button for every branch where `hasWorkspaceForBranch` returns false, including branches where `branch.isCheckedOut && !isWorktree`. Clicking the button calls `onCheckoutBranch`, which will fail at the handler level because git won't allow two worktrees for the same branch. The old code displayed a disabled tooltip for this state to signal the branch was already in use. The PR description notes the handler is expected to surface a toast on failure, but users clicking "Create workspace" have no pre-click indication that the action will fail — the button looks identical to the normal case. Consider keeping a distinct visual cue (e.g. a muted style or small badge) for branches that are already checked out elsewhere.
Reviews (1): Last reviewed commit: "feat(desktop): two-line branch rows to m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+198
to
+230
| </span> | ||
| </div> | ||
| <span className="ml-2 flex shrink-0 items-center gap-1.5 self-center"> | ||
| {(() => { | ||
| // Authoritative check against the cloud-synced | ||
| // collection — a server `hasWorkspace:true` row | ||
| // may be stale after a delete. | ||
| const canOpen = hasWorkspaceForBranch(branch.name); | ||
| return ( | ||
| <span className="hidden items-center gap-1.5 group-hover:inline-flex group-focus-within:inline-flex"> | ||
| {canOpen ? ( | ||
| <button | ||
| type="button" | ||
| className="hidden group-hover:inline-flex group-focus-within:inline-flex items-center rounded-sm bg-primary/10 hover:bg-primary/20 px-2 py-0.5 text-[11px] text-primary font-medium" | ||
| className="inline-flex items-center rounded-sm bg-primary/10 px-2 py-0.5 text-[11px] font-medium text-primary hover:bg-primary/20" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| if (hasWorkspace) { | ||
| onOpenExisting(branch.name); | ||
| } else { | ||
| onCheckoutBranch(branch.name); | ||
| } | ||
| onOpenExisting(branch.name); | ||
| }} | ||
| > | ||
| {hasWorkspace ? "Open" : "Create"} | ||
| Open workspace | ||
| </button> | ||
| ); | ||
| })() | ||
| ) : branch.isCheckedOut ? ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| {/* | ||
| Use aria-disabled, NOT the native `disabled` attribute. | ||
| Native disabled buttons don't fire pointer events, so the | ||
| Tooltip never sees hover/focus and never opens — defeating | ||
| the purpose of explaining why the button is unavailable. | ||
| */} | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| aria-disabled="true" | ||
| className="hidden group-hover:inline-flex group-focus-within:inline-flex items-center rounded-sm bg-muted px-2 py-0.5 text-[11px] text-muted-foreground/70 cursor-not-allowed" | ||
| onClick={(e) => e.stopPropagation()} | ||
| className="inline-flex items-center rounded-sm bg-primary/10 px-2 py-0.5 text-[11px] font-medium text-primary hover:bg-primary/20" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onCheckoutBranch(branch.name); | ||
| }} | ||
| > | ||
| Check out | ||
| Create workspace | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="left"> | ||
| Already checked out in another worktree | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ) : ( | ||
| <button | ||
| type="button" | ||
| className="hidden group-hover:inline-flex group-focus-within:inline-flex items-center rounded-sm bg-primary/10 hover:bg-primary/20 px-2 py-0.5 text-[11px] text-primary font-medium" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onCheckoutBranch(branch.name); | ||
| }} | ||
| > | ||
| Check out | ||
| </button> | ||
| )} | ||
| {effectiveCompareBaseBranch === branch.name && ( | ||
| <HiCheck className="size-4 text-primary" /> | ||
| )} | ||
| </span> | ||
| </CommandItem> | ||
| ); | ||
| })} | ||
| {hasNextPage && ( | ||
| <div | ||
| ref={sentinelRef} | ||
| className="py-2 text-center text-[11px] text-muted-foreground/60" | ||
| > | ||
| {isFetchingNextPage ? "Loading more..." : ""} | ||
| </div> | ||
| )} | ||
| </CommandList> | ||
| </TooltipProvider> | ||
| )} |
Contributor
There was a problem hiding this comment.
The new two-case logic shows an active "Create workspace" button for every branch where hasWorkspaceForBranch returns false, including branches where branch.isCheckedOut && !isWorktree. Clicking the button calls onCheckoutBranch, which will fail at the handler level because git won't allow two worktrees for the same branch. The old code displayed a disabled tooltip for this state to signal the branch was already in use. The PR description notes the handler is expected to surface a toast on failure, but users clicking "Create workspace" have no pre-click indication that the action will fail — the button looks identical to the normal case. Consider keeping a distinct visual cue (e.g. a muted style or small badge) for branches that are already checked out elsewhere.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx
Line: 198-230
Comment:
**"Create workspace" enabled for already-checked-out branches**
The new two-case logic shows an active "Create workspace" button for every branch where `hasWorkspaceForBranch` returns false, including branches where `branch.isCheckedOut && !isWorktree`. Clicking the button calls `onCheckoutBranch`, which will fail at the handler level because git won't allow two worktrees for the same branch. The old code displayed a disabled tooltip for this state to signal the branch was already in use. The PR description notes the handler is expected to surface a toast on failure, but users clicking "Create workspace" have no pre-click indication that the action will fail — the button looks identical to the normal case. Consider keeping a distinct visual cue (e.g. a muted style or small badge) for branches that are already checked out elsewhere.
How can I resolve this? If you propose a fix, please make it concise.![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: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx (1)
38-42: ⚡ Quick winTighten
normalizeState's return type to eliminate the unchecked type assertion.
normalizeStatereturnsstring, so theas PRStatecast at line 197 bypasses TypeScript's safety checks — if the upstream API returns an unmapped state (e.g."merged"not handled), the cast silently succeeds andPRIconcould receive an unexpected value. ThenormalizeIssueStatepattern inGitHubIssueLinkCommand.tsxshows the better approach: return the typed value directly with explicit narrowing, requiring no cast at the call site.Change the return type to
PRStateand add an explicit guard covering all valid state values with a safe fallback:♻️ Proposed fix
-function normalizeState(state: string, isDraft: boolean): string { - if (isDraft) return "draft"; - if (state === "OPEN" || state === "open") return "open"; - return state.toLowerCase(); -} +function normalizeState(state: string, isDraft: boolean): PRState { + if (isDraft) return "draft"; + const lowered = state.toLowerCase(); + if (lowered === "open" || lowered === "closed" || lowered === "merged") { + return lowered; + } + return "open"; +} @@ - const state = normalizeState(pr.state, pr.isDraft) as PRState; + const state = normalizeState(pr.state, pr.isDraft);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx` around lines 38 - 42, The normalizeState function currently returns string and forces a cast to PRState at the call site; change normalizeState to return PRState directly by updating its signature to normalizeState(state: string, isDraft: boolean): PRState and implement explicit narrowing for all allowed PRState values (handle isDraft => "draft", "OPEN"|"open" => "open", "CLOSED"|"closed" => "closed", "MERGED"|"merged" => "merged" or whatever PRState enum contains) and provide a safe fallback (e.g., "open" or a defined default PRState) for any unknown input so callers (like where the result is passed to PRIcon) no longer need an unchecked as PRState cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx`:
- Around line 198-230: The action buttons in CompareBaseBranchPicker are hidden
for keyboard users because visibility relies on group-hover/group-focus-within;
change the visibility classes on the span wrapping the buttons to also show when
the cmdk row is active by adding data-[selected=true]:inline-flex (i.e. replace
"hidden group-hover:inline-flex group-focus-within:inline-flex" with something
that includes "data-[selected=true]:inline-flex") so the buttons (and their
onClick handlers onOpenExisting/onCheckoutBranch used with
hasWorkspaceForBranch) are visible during keyboard navigation; additionally,
update the CommandItem selection handler (CommandItem.onSelect or the component
that closes the picker) to branch: if the row is selected and Enter is pressed
with no modifiers, call onOpenExisting(branch.name) or
onCheckoutBranch(branch.name) based on hasWorkspaceForBranch(branch.name)
instead of only closing the picker.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx`:
- Around line 38-42: The normalizeState function currently returns string and
forces a cast to PRState at the call site; change normalizeState to return
PRState directly by updating its signature to normalizeState(state: string,
isDraft: boolean): PRState and implement explicit narrowing for all allowed
PRState values (handle isDraft => "draft", "OPEN"|"open" => "open",
"CLOSED"|"closed" => "closed", "MERGED"|"merged" => "merged" or whatever PRState
enum contains) and provide a safe fallback (e.g., "open" or a defined default
PRState) for any unknown input so callers (like where the result is passed to
PRIcon) no longer need an unchecked as PRState cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06338b37-17b5-4c15-8de1-43111298b156
📒 Files selected for processing (4)
apps/desktop/src/renderer/components/Chat/ChatInterface/components/IssueLinkCommand/IssueLinkCommand.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsx
Branch rows now use a distinct glyph + tint per state so users can spot worktree and remote-only branches without parsing the meta strip: - worktree → folder-open in primary tint - remote-only → globe in muted/60 - local → git-branch in muted (unchanged)
Branches whose worktree git knows about but Superset hasn't tracked as a v2 workspace (e.g. created outside Superset) now light up "Open workspace" instead of "Create workspace". The server's workspaces.create already adopts in this case via adoptExistingWorktree; the client just needs to await the resolved canonical workspace id before navigating — the optimistic snapshot id wouldn't match the existing row, leaving the user on a 404. New onAdoptForeignWorktree handler does that, and the picker routes to it whenever branch.worktreePath is set without a matching tracked workspace.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts`:
- Around line 175-190: The onAdoptForeignWorktree flow in
useBranchPickerController closes the modal then awaits submit without error
handling, so add a try/catch around the await submit({...}) call in the
onAdoptForeignWorktree handler (and keep closeModal() where it is); on catch
call the existing toast/error helper to show a failure message and return; also
handle the case where submit returns result.ok === false by showing an error
toast (with a helpful message from result if present) instead of silently doing
nothing—only navigate when result.ok is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd190929-31de-4309-9c79-a3f3f7dfce91
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx
Comment on lines
+175
to
+190
| closeModal(); | ||
| const result = await submit({ | ||
| hostId: resolvedHostId, | ||
| snapshot: { | ||
| id: snapshotId, | ||
| projectId, | ||
| name: workspaceName, | ||
| branch: branchName, | ||
| }, | ||
| }); | ||
| if (result.ok) { | ||
| void navigate({ | ||
| to: "/v2-workspace/$workspaceId", | ||
| params: { workspaceId: result.workspaceId }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
onAdoptForeignWorktree silently swallows failures — add try-catch and an else toast.
Two reachable failure paths leave the user with a closed modal and zero feedback, directly contradicting the PR objective ("handler should surface an error toast on failure"):
submitthrows (network error, server crash, etc.) — there is notry/catch, so the async callback produces an unhandled promise rejection.result.ok === false— theif (result.ok)branch is simply skipped; no navigation, no toast, no recovery.
In both cases closeModal() already ran (Line 175), so the only feedback path left is a toast.
🛡️ Proposed fix
- closeModal();
- const result = await submit({
- hostId: resolvedHostId,
- snapshot: {
- id: snapshotId,
- projectId,
- name: workspaceName,
- branch: branchName,
- },
- });
- if (result.ok) {
- void navigate({
- to: "/v2-workspace/$workspaceId",
- params: { workspaceId: result.workspaceId },
- });
- }
+ closeModal();
+ try {
+ const result = await submit({
+ hostId: resolvedHostId,
+ snapshot: {
+ id: snapshotId,
+ projectId,
+ name: workspaceName,
+ branch: branchName,
+ },
+ });
+ if (result.ok) {
+ void navigate({
+ to: "/v2-workspace/$workspaceId",
+ params: { workspaceId: result.workspaceId },
+ });
+ } else {
+ toast.error("Failed to adopt worktree — workspace could not be created");
+ }
+ } catch (err) {
+ toast.error("Failed to adopt worktree — an unexpected error occurred");
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts`
around lines 175 - 190, The onAdoptForeignWorktree flow in
useBranchPickerController closes the modal then awaits submit without error
handling, so add a try/catch around the await submit({...}) call in the
onAdoptForeignWorktree handler (and keep closeModal() where it is); on catch
call the existing toast/error helper to show a failure message and return; also
handle the case where submit returns result.ok === false by showing an error
toast (with a helpful message from result if present) instead of silently doing
nothing—only navigate when result.ok is true.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx:249">
P2: Handle failures from the foreign-worktree action explicitly; this new click path currently allows rejected async work to go unhandled and can fail silently.
(Based on your team's feedback about handling async calls and surfacing errors.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| } else { | ||
| onCheckoutBranch(branch.name); | ||
| } | ||
| onAdoptForeignWorktree(branch.name); |
Contributor
There was a problem hiding this comment.
P2: Handle failures from the foreign-worktree action explicitly; this new click path currently allows rejected async work to go unhandled and can fail silently.
(Based on your team's feedback about handling async calls and surfacing errors.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx, line 249:
<comment>Handle failures from the foreign-worktree action explicitly; this new click path currently allows rejected async work to go unhandled and can fail silently.
(Based on your team's feedback about handling async calls and surfacing errors.) </comment>
<file context>
@@ -223,6 +240,17 @@ export function CompareBaseBranchPicker({
+ className="inline-flex items-center rounded-sm bg-primary/10 px-2 py-0.5 text-[11px] font-medium text-primary hover:bg-primary/20"
+ onClick={(e) => {
+ e.stopPropagation();
+ onAdoptForeignWorktree(branch.name);
+ }}
+ >
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
Drop the v2Workspaces gate from the branch picker. The server's workspaces.create already covers all three cases (open tracked, adopt foreign worktree, create fresh) and is the source of truth — having the client second-guess via cloud-collection state was an optimization that introduced bugs (stale lookups after deletes / cross-device sync, snapshot-id vs canonical-id mismatch on adoption, edge cases around checked-out-elsewhere branches). Picker now exposes one CTA: "Open workspace". The handler awaits submit and navigates to the canonical workspaceId returned by the server. Costs ~one round-trip on the common open-tracked case but keeps the flow correct in every state.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts (1)
95-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
submitfailures explicitly after closing the modal.
onOpenWorkspacestill has silent failure paths: thrownsubmit(...)errors andresult.ok === falseboth leave the user with no feedback aftercloseModal(). Please addtry/catchand an explicit non-ok toast before returning.💡 Minimal patch
const workspaceName = resolveActionWorkspaceName(branchName); closeModal(); - const result = await submit({ - hostId: resolvedHostId, - snapshot: { - id: snapshotId, - projectId, - name: workspaceName, - branch: branchName, - }, - }); - if (result.ok) { - void navigate({ - to: "/v2-workspace/$workspaceId", - params: { workspaceId: result.workspaceId }, - }); - } + try { + const result = await submit({ + hostId: resolvedHostId, + snapshot: { + id: snapshotId, + projectId, + name: workspaceName, + branch: branchName, + }, + }); + if (result.ok) { + void navigate({ + to: "/v2-workspace/$workspaceId", + params: { workspaceId: result.workspaceId }, + }); + return; + } + toast.error("Failed to open workspace"); + } catch { + toast.error("Failed to open workspace"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts` around lines 95 - 110, The code currently calls closeModal() then awaits submit(...) and only handles the success path; update the onOpenWorkspace flow (the function that calls closeModal, submit, and navigate) to wrap the submit call in try/catch and handle non-ok results: await submit(...) inside a try, on catch call a user-visible error toast (and return) and also, if submit returns result.ok === false, show an explicit error toast before returning; keep the existing navigate(...) on success and ensure closeModal() remains called before attempting submit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts`:
- Around line 95-110: The code currently calls closeModal() then awaits
submit(...) and only handles the success path; update the onOpenWorkspace flow
(the function that calls closeModal, submit, and navigate) to wrap the submit
call in try/catch and handle non-ok results: await submit(...) inside a try, on
catch call a user-visible error toast (and return) and also, if submit returns
result.ok === false, show an explicit error toast before returning; keep the
existing navigate(...) on success and ensure closeModal() remains called before
attempting submit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4721104-36e4-4ec3-9e09-c216a7c00f05
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx:212">
P2: The CTA label is now always "Open workspace", so branches that require creation no longer show a "Create workspace" action.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| onOpenWorkspace(branch.name); | ||
| }} | ||
| > | ||
| Open workspace |
Contributor
There was a problem hiding this comment.
P2: The CTA label is now always "Open workspace", so branches that require creation no longer show a "Create workspace" action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx, line 212:
<comment>The CTA label is now always "Open workspace", so branches that require creation no longer show a "Create workspace" action.</comment>
<file context>
@@ -212,60 +200,18 @@ export function CompareBaseBranchPicker({
+ onOpenWorkspace(branch.name);
+ }}
+ >
+ Open workspace
+ </button>
+ </span>
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
Two CodeRabbit findings: 1. The "Open workspace" button was unreachable via keyboard. cmdk leaves DOM focus on the search input during arrow-key navigation, so the per-row button gated on group-focus-within never showed and could not be clicked. Make the button visible on the cmdk-active row (`group-data-[selected=true]:inline-flex`) and wire Mod+Enter to fire onOpenWorkspace for the selected branch. Plain Enter still sets the compare base. 2. onOpenWorkspace closed the modal silently on submit failure. The in-flight tracker stores the error but does not toast — surface it here so a failed open isn't invisible. Adds a `MOD↵` keyboard hint to the button so the shortcut is discoverable.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts (1)
95-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
submit()intry/catchtoo.
result.ok === falsenow toasts, but a thrownsubmit(...)still escapes aftercloseModal()has already run. That leaves the modal closed with no user feedback on transport/server failures, which is still a reachable failure path here.Suggested minimal fix
const workspaceName = resolveActionWorkspaceName(branchName); closeModal(); - const result = await submit({ - hostId: resolvedHostId, - snapshot: { - id: snapshotId, - projectId, - name: workspaceName, - branch: branchName, - }, - }); - if (result.ok) { - void navigate({ - to: "/v2-workspace/$workspaceId", - params: { workspaceId: result.workspaceId }, - }); - } else { - // `submit` tracks the failure via `markError`, but the in-flight - // manager doesn't toast — without this, a rejected open closes - // the modal silently and the user has no feedback that anything - // failed. - toast.error(result.error || "Failed to open workspace"); - } + try { + const result = await submit({ + hostId: resolvedHostId, + snapshot: { + id: snapshotId, + projectId, + name: workspaceName, + branch: branchName, + }, + }); + if (result.ok) { + void navigate({ + to: "/v2-workspace/$workspaceId", + params: { workspaceId: result.workspaceId }, + }); + } else { + toast.error(result.error || "Failed to open workspace"); + } + } catch { + toast.error("Failed to open workspace"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts` around lines 95 - 116, closeModal() is called before awaiting submit(), but submit(...) can throw and escape leaving the modal closed with no feedback; wrap the await submit(...) call in a try/catch inside useBranchPickerController, keep the existing success path that calls navigate({ to: "/v2-workspace/$workspaceId", params: { workspaceId: result.workspaceId } }) when result.ok, and in the catch block call toast.error(error.message || "Failed to open workspace") (or use result.error if available) so transport/server exceptions are surfaced to the user; ensure you do not re-open the modal — just catch the exception, toast the error, and retain the existing markError logic inside submit().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts`:
- Around line 95-116: closeModal() is called before awaiting submit(), but
submit(...) can throw and escape leaving the modal closed with no feedback; wrap
the await submit(...) call in a try/catch inside useBranchPickerController, keep
the existing success path that calls navigate({ to:
"/v2-workspace/$workspaceId", params: { workspaceId: result.workspaceId } })
when result.ok, and in the catch block call toast.error(error.message || "Failed
to open workspace") (or use result.error if available) so transport/server
exceptions are surfaced to the user; ensure you do not re-open the modal — just
catch the exception, toast the error, and retain the existing markError logic
inside submit().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d52dc1ea-9bf0-4def-8d9a-2153094c7436
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.ts
Deslop pass on the branch picker: - Trim verbose "why" comments down to the load-bearing lines. - Collapse the redundant span around the row's Open-workspace button (visibility classes move onto the button itself). - Align the compare-base check icon's hide-trigger with the button's show-trigger so they don't briefly overlap on cmdk-active rows.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx:64">
P2: Reset or validate `selectedValue` before handling Mod+Enter; otherwise the shortcut can open a stale branch selection.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| // Mirror cmdk's selected-row value so a Mod+Enter keydown can resolve it | ||
| // to the branch name without traversing DOM. Without this, keyboard users | ||
| // have no path to `onOpenWorkspace`. | ||
| const [selectedValue, setSelectedValue] = useState(""); |
Contributor
There was a problem hiding this comment.
P2: Reset or validate selectedValue before handling Mod+Enter; otherwise the shortcut can open a stale branch selection.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/CompareBaseBranchPicker/CompareBaseBranchPicker.tsx, line 64:
<comment>Reset or validate `selectedValue` before handling Mod+Enter; otherwise the shortcut can open a stale branch selection.</comment>
<file context>
@@ -55,6 +58,10 @@ export function CompareBaseBranchPicker({
+ // Mirror cmdk's selected-row value so a Mod+Enter keydown can resolve it
+ // to the branch name without traversing DOM. Without this, keyboard users
+ // have no path to `onOpenWorkspace`.
+ const [selectedValue, setSelectedValue] = useState("");
const sentinelRef = useRef<HTMLDivElement | null>(null);
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…uperset-sh#4226) * feat(desktop): roomier v2 workspace pickers + clearer branch actions Widen the PR/issue/task pickers from 320px to 440px and switch each row to a Linear-style two-line layout (title on top, monospace id + state below) so long titles aren't clipped. Branch picker matches the new width and gets a "Select base branch…" placeholder when no base is set. For each branch row, surface exactly one action that names what it does: "Open workspace" when a tracked workspace exists, "Create workspace" otherwise. Drops the misleading "Create"/"Check out" pairing and the disabled-tooltip state — every row is now actionable. * feat(desktop): two-line branch rows to match v2 picker layout Branch picker now mirrors the PR/Issue/Task two-line item layout: top row is the branch name in mono, bottom row is the meta strip (relative commit time · default · remote · worktree). Action button and the selected-base check moved to the right column. * style(desktop): drop font-mono from branch row title for picker parity * feat(desktop): per-state branch row icons (worktree/remote/local) Branch rows now use a distinct glyph + tint per state so users can spot worktree and remote-only branches without parsing the meta strip: - worktree → folder-open in primary tint - remote-only → globe in muted/60 - local → git-branch in muted (unchanged) * fix(desktop): adopt foreign worktrees when opening from branch picker Branches whose worktree git knows about but Superset hasn't tracked as a v2 workspace (e.g. created outside Superset) now light up "Open workspace" instead of "Create workspace". The server's workspaces.create already adopts in this case via adoptExistingWorktree; the client just needs to await the resolved canonical workspace id before navigating — the optimistic snapshot id wouldn't match the existing row, leaving the user on a 404. New onAdoptForeignWorktree handler does that, and the picker routes to it whenever branch.worktreePath is set without a matching tracked workspace. * refactor(desktop): unify branch picker on a single Open workspace path Drop the v2Workspaces gate from the branch picker. The server's workspaces.create already covers all three cases (open tracked, adopt foreign worktree, create fresh) and is the source of truth — having the client second-guess via cloud-collection state was an optimization that introduced bugs (stale lookups after deletes / cross-device sync, snapshot-id vs canonical-id mismatch on adoption, edge cases around checked-out-elsewhere branches). Picker now exposes one CTA: "Open workspace". The handler awaits submit and navigates to the canonical workspaceId returned by the server. Costs ~one round-trip on the common open-tracked case but keeps the flow correct in every state. * fix(desktop): keyboard activation + error toast for branch picker Two CodeRabbit findings: 1. The "Open workspace" button was unreachable via keyboard. cmdk leaves DOM focus on the search input during arrow-key navigation, so the per-row button gated on group-focus-within never showed and could not be clicked. Make the button visible on the cmdk-active row (`group-data-[selected=true]:inline-flex`) and wire Mod+Enter to fire onOpenWorkspace for the selected branch. Plain Enter still sets the compare base. 2. onOpenWorkspace closed the modal silently on submit failure. The in-flight tracker stores the error but does not toast — surface it here so a failed open isn't invisible. Adds a `MOD↵` keyboard hint to the button so the shortcut is discoverable. * refactor(desktop): tighten branch-picker comments + collapse spans Deslop pass on the branch picker: - Trim verbose "why" comments down to the load-bearing lines. - Collapse the redundant span around the row's Open-workspace button (visibility classes move onto the button itself). - Align the compare-base check icon's hide-trigger with the button's show-trigger so they don't briefly overlap on cmdk-active rows.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
superset-sh#4037 NewProjectModal: drop fork-only OPTIONS multi-mode selector and adopt upstream v2-only simplification (toast errors + Label + spinner). The 'empty'/'template' modes were 'coming soon' disabled entries with no implementation behind them; no user-facing fork feature lost. superset-sh#4191 DashboardSidebarDeleteDialog: keep fork's 'Checking…' confirm-button label UX while incorporating upstream's fixed-height warning banner slot. Restore isCheckingStatus destructure + prop pass; add to DestroyConfirmPane interface so the deleting-button disabled state still gates on it. superset-sh#4045 schema.ts: backfill sidebarFileLinks field into v2UserPreferencesSchema and DEFAULT_V2_USER_PREFERENCES (upstream introduced these in superset-sh#3988 which fork will adopt in Phase 5). Also add 'shift' tier to linkTierMapSchema (matches superset-sh#3988 LinkTier expansion) so superset-sh#4045's added schema tests pass. superset-sh#4212 tray Quit Completely: replace upstream quitAppCompletely() with fork's existing requestQuit('stop') which already performs full host- service + pty-daemon teardown via prepareQuit('stop') + getTodoScheduler stop + cleanupMainWindowResources. Tray menu entry retained. superset-sh#4225 automations/page.tsx: drop iconUrl prop from ProjectThumbnail call (fork ProjectThumbnail uses githubOwner, iconUrl arrives with superset-sh#3823 v2 project icon settings in cycle 41). Pass githubOwner={null} as transient fallback. superset-sh#4226 useBranchPickerController: fork's useWorkspaceCreates.submit returns Promise<void> (in-flight tracker carries error state); upstream changed it to Promise<SubmitResult>. Wrap submit in try/catch and navigate using snapshot.id so the picker compiles. Note: foreign-worktree adopt path with canonical workspaceId waiting is deferred to cycle 44b when WorkspaceCreatesManager / submit API are upgraded.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…low-up superset-sh#4074 exhaustive worktree search in v2 branch picker: - adopt.ts / search-branches.ts / branch-search.ts: whole-file upstream overwrite. Fork's <repoPath>/.worktrees/<branch>-scoped variant (FORK NOTE in branch-search.ts) is dropped in favour of upstream's exhaustive 'all git worktrees including foreign ones' strategy. This matches superset-sh#4226's adopt-foreign-worktree UX direction. superset-sh#4160 listBranches single git spawn perf (skipped, HEAD-preferred): - packages/host-service/src/trpc/router/git/git.ts: keep fork's rich listBranches (sortOrder/pinDefault/buildBranch per-branch upstream/ahead-behind/last-commit). Upstream's 4xN -> 1 spawn simplification drops those fields the fork's BaseBranchSelector consumes; perf win not worth the rich UI regression. Revisit if BaseBranchSelector is unified with upstream's BaseBranchSelector in a future cycle. superset-sh#4218 respawn host-service on Electron auto-update: - packages/host-service/package.json: merge fork's ./settings export and upstream's ./package.json export (both needed). - packages/host-service/src/trpc/router/host/host.ts: whole-file upstream overwrite. HOST_SERVICE_VERSION switches from fork's hard-coded '0.7.0' to dynamic hostServicePackageJson.version (currently 0.8.1). Bumps via apps/desktop/electron-builder.ts respawn flow are now automatic. superset-sh#4231 restore terminal modes on reattach via headless xterm: - packages/host-service/src/terminal/terminal.ts: createModeTracker call sites referenced un-declared 'cols, rows' inside session-create closure; fix to hard-code (120, 32) which matches the daemon.open() call above. Real cols/rows arrive via subsequent resize() calls; modeTracker.resize handles the update path. Files clean cherry-picked: superset-sh#4046 host-service bump 0.7.0, plus adopt-foreign-worktree wiring.
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
w-80tow-[440px]and reformat each row as a Linear-style two-line item (title on top, mono id · state below) so long titles aren't clipped.Select base branch…placeholder, drop the disabledCheck outtooltip +TooltipProvider.Open/Create/Check outmix with exactly one action that names what it does:hasWorkspaceForBranch(branch)→ Open workspace (onOpenExisting)onCheckoutBranch)Notes
apps/desktop/src/renderer/components/NewWorkspaceModal/...) is untouched per the v1-sunset rule.↵,⌘↵) were removed — they weren't actually wired to a keydown handler, so showing them was misleading.onCheckoutBranchwill be invoked for branches that have a worktree elsewhere (branch.isCheckedOut). The handler is expected to surface a toast on failure rather than silently no-op.Test plan
Select base branch…when no base is selected.Open workspaceif a workspace exists for that branch, elseCreate workspace.Open workspace→ jumps to the existing workspace.Create workspaceon a branch without a worktree → workspace is created and opened.Create workspaceon a branch already checked out elsewhere → handler surfaces an error toast (no silent failure).Summary by cubic
Widened the v2 pickers with two-line rows and unified branch rows to one “Open workspace” action resolved on the server (open/adopt/create). Adds keyboard activation, better error handling, and a small polish to selected-row icon behavior.
New Features
Bug Fixes
Written for commit 7028914. Summary will update on new commits.
Summary by CodeRabbit
Style
Refactor
New Features