fix(desktop): stable Adopt button + Adopt-all on v1 workspaces#4214
Merged
fix(desktop): stable Adopt button + Adopt-all on v1 workspaces#4214
Conversation
…kspaces - Render the adopting state as a disabled button instead of swapping to a div so focus stays put and Radix doesn't scroll the dialog to top mid-list. - Show "Adopting…" with a spinner on the row while the mutation is in flight. - Add an "Adopt all (N)" button to the workspaces page header that sequentially adopts pending workspaces and shows live progress.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImportWorkspacesPage centralizes per-workspace adoption into a page-level AdoptStatus map, precomputes worktreePath/baseBranch per entry, runs sequential "Adopt all" with progress, and passes status/disabled/onAdopt into a presentational WorkspaceRow. ImportPageShell adds headerAction; RowAction supports an optional running label. ChangesCentralized Workspace Adoption State & UI
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportWorkspacesPage
participant adoptWorkspace
participant SidebarState
participant QueryClient
User->>ImportWorkspacesPage: clicks "Adopt all" or per-row "Adopt"
ImportWorkspacesPage->>adoptWorkspace: invoke (entry)
adoptWorkspace->>SidebarState: ensureWorkspaceInSidebar(v2ProjectId)
adoptWorkspace->>QueryClient: invalidate cloud list query
adoptWorkspace-->>ImportWorkspacesPage: success / error
ImportWorkspacesPage->>User: update row status / adoptAllProgress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Greptile SummaryThis PR addresses two bugs in the v1 import modal: (1) the workspace list scrolled to the top when clicking Adopt because the row swapped its
Confidence Score: 3/5The core focus-scroll fix and Adopt-all implementation are sound, but the Retry button on error-state rows remains active during an ongoing Adopt-all run, which can trigger concurrent API mutations for the same workspace. The main logic is well-structured: state is correctly lifted, the ref pattern avoids stale closures in adoptAll, and the sequential loop is straightforward. The gap is that disabled is only threaded into the ready action; error-state rows keep an active Retry button even while adoptAll processes the same workspace, potentially firing two simultaneous adopt.mutate calls. ImportWorkspacesPage.tsx — the interaction between the adoptAll queue snapshot and error-state Retry buttons deserves a closer look before this ships to users with large v1 workspace lists.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx | Core change: lifts per-row adopt state into parent, adds sequential Adopt-all with live progress. Has a concurrency gap where error-row Retry buttons remain active during adoptAll, potentially triggering double API calls for the same workspace. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportPageShell/ImportPageShell.tsx | Minor layout change: wraps the existing refresh button in a flex container and adds an optional headerAction slot before it. Straightforward and safe. |
| apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsx | The "running" case now renders a disabled Button (with optional label) instead of a plain div, fixing the Radix Dialog focus-scroll issue. Clean and correct. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:412-413
**Retry button active during Adopt-all**
When `isAdoptingAll` is true, the `disabled` flag is only threaded into the `"ready"` action branch; the `"error"` branch emits `{ kind: "error", onRetry: onAdopt }` without it. The `RowActionView` error case renders an always-enabled Retry button, so a user can click Retry on an error-state row while an `adoptAll` run is already processing that same workspace (it was included in the queue snapshot). This results in two concurrent `adoptWorkspace` calls for the same workspace — two parallel `adopt.mutate` API requests — which may create duplicate entries or produce a misleading double-error.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:302-307
**`adoptAll` does not skip workspaces already transitioned to `"running"` mid-queue**
`adoptAll` builds the queue once from the ref snapshot and then iterates without re-checking state. If a workspace was `"error"` when the queue was built (and thus included), but the user clicked Retry on it before `adoptAll` reaches it, `adoptWorkspace` will be called a second time even though the workspace is already mid-flight. A guard like `if (adoptStatesRef.current.get(entry.workspace.id)?.kind === "running") continue;` at the top of each loop iteration would prevent the duplicate call.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:328-330
**Redundant `isAdoptingAll &&` guard on `adoptAllProgress`**
`isAdoptingAll` is defined as `adoptAllProgress !== null`, so `isAdoptingAll && adoptAllProgress` is always the same as just `adoptAllProgress`. The extra condition implies `adoptAllProgress` could be null while `isAdoptingAll` is true, which is impossible. Replacing the ternary condition with just `adoptAllProgress` makes the intent clearer.
Reviews (1): Last reviewed commit: "fix(desktop): keep adopting button stabl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+412
to
+413
| if (status.kind === "error") { | ||
| return { kind: "error", message: status.message, onRetry: onAdopt }; |
There was a problem hiding this comment.
When isAdoptingAll is true, the disabled flag is only threaded into the "ready" action branch; the "error" branch emits { kind: "error", onRetry: onAdopt } without it. The RowActionView error case renders an always-enabled Retry button, so a user can click Retry on an error-state row while an adoptAll run is already processing that same workspace (it was included in the queue snapshot). This results in two concurrent adoptWorkspace calls for the same workspace — two parallel adopt.mutate API requests — which may create duplicate entries or produce a misleading double-error.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Line: 412-413
Comment:
**Retry button active during Adopt-all**
When `isAdoptingAll` is true, the `disabled` flag is only threaded into the `"ready"` action branch; the `"error"` branch emits `{ kind: "error", onRetry: onAdopt }` without it. The `RowActionView` error case renders an always-enabled Retry button, so a user can click Retry on an error-state row while an `adoptAll` run is already processing that same workspace (it was included in the queue snapshot). This results in two concurrent `adoptWorkspace` calls for the same workspace — two parallel `adopt.mutate` API requests — which may create duplicate entries or produce a misleading double-error.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+302
to
+307
| for (let i = 0; i < queue.length; i++) { | ||
| const entry = queue[i]; | ||
| if (!entry) continue; | ||
| setAdoptAllProgress({ current: i, total: queue.length }); | ||
| await adoptWorkspace(entry); | ||
| } |
There was a problem hiding this comment.
adoptAll does not skip workspaces already transitioned to "running" mid-queue
adoptAll builds the queue once from the ref snapshot and then iterates without re-checking state. If a workspace was "error" when the queue was built (and thus included), but the user clicked Retry on it before adoptAll reaches it, adoptWorkspace will be called a second time even though the workspace is already mid-flight. A guard like if (adoptStatesRef.current.get(entry.workspace.id)?.kind === "running") continue; at the top of each loop iteration would prevent the duplicate call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Line: 302-307
Comment:
**`adoptAll` does not skip workspaces already transitioned to `"running"` mid-queue**
`adoptAll` builds the queue once from the ref snapshot and then iterates without re-checking state. If a workspace was `"error"` when the queue was built (and thus included), but the user clicked Retry on it before `adoptAll` reaches it, `adoptWorkspace` will be called a second time even though the workspace is already mid-flight. A guard like `if (adoptStatesRef.current.get(entry.workspace.id)?.kind === "running") continue;` at the top of each loop iteration would prevent the duplicate call.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+328
to
+330
| {isAdoptingAll && adoptAllProgress | ||
| ? `Adopting ${adoptAllProgress.current + 1}/${adoptAllProgress.total}` | ||
| : `Adopt all (${pendingEntries.length})`} |
There was a problem hiding this comment.
isAdoptingAll && guard on adoptAllProgress
isAdoptingAll is defined as adoptAllProgress !== null, so isAdoptingAll && adoptAllProgress is always the same as just adoptAllProgress. The extra condition implies adoptAllProgress could be null while isAdoptingAll is true, which is impossible. Replacing the ternary condition with just adoptAllProgress makes the intent clearer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx
Line: 328-330
Comment:
**Redundant `isAdoptingAll &&` guard on `adoptAllProgress`**
`isAdoptingAll` is defined as `adoptAllProgress !== null`, so `isAdoptingAll && adoptAllProgress` is always the same as just `adoptAllProgress`. The extra condition implies `adoptAllProgress` could be null while `isAdoptingAll` is true, which is impossible. Replacing the ternary condition with just `adoptAllProgress` makes the intent clearer.
How can I resolve this? If you propose a fix, please make it concise.- 13px row text, flat icons (no chip background), subtle row hover bg. - Smaller header (14px medium tracking-tight, 12px description), border/60. - Section labels become 10px semibold uppercase tracking-wider muted/70. - Buttons standardized to h-7 with 12px font; modal footer to h-8 13px. - Adopt-all label uses tabular-nums and "·" separator.
![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 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:306">
P2: Add a guard at the top of the loop iteration to skip workspaces that are already in `"running"` state. Since the queue is built once from a snapshot but execution is sequential and async, a workspace's state could change mid-queue (e.g., via a concurrent Retry click). Without re-checking, `adoptWorkspace` may be called redundantly for an already-in-flight workspace.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx:412">
P2: The Retry button in the error state is not disabled during an adopt-all run. The `disabled` prop is only threaded into the `"ready"` action branch but not the `"error"` branch. This allows a user to click Retry on an errored row while adopt-all is actively processing the queue (which includes that same errored workspace), resulting in two concurrent `adoptWorkspace` calls for the same workspace — potentially creating duplicates or confusing error states.</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: 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/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsx`:
- Around line 375-385: The Retry button can be clicked during an Adopt-all run
causing duplicate adopt calls; fix by honoring the disabled flag in the error
branch or skipping running entries: update the error-branch RowAction generation
(the code path where status.kind === "error" in ImportRow/WorkspaceRow) to
respect the passed disabled prop (isAdoptingAll) so Retry is rendered disabled
when isAdoptingAll is true, and/or modify adoptWorkspace or the adopt-all loop
(the function that iterates over queue and calls adoptWorkspace) to skip entries
that already have an in-progress state in adoptStates (or a running flag) to
prevent calling client.workspaceCreation.adopt.mutate twice for the same entry.
🪄 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: 588c1d94-94af-4f88-a86b-01c886f9d26a
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/ImportWorkspacesPage/ImportWorkspacesPage.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportPageShell/ImportPageShell.tsxapps/desktop/src/renderer/routes/_authenticated/components/V1ImportModal/components/ImportRow/ImportRow.tsx
- adoptWorkspace no longer returns a never-read boolean. - AdoptStatus.imported drops the unread v2WorkspaceId field. - Skip the redundant initial setAdoptAllProgress; the loop's first iteration writes it.
Adopt-all snapshots its queue, so a manual Retry click on an errored row could fire adopt.mutate twice for the same workspace. Two fixes: - During adopt-all, the row's "error" action becomes a disabled "Queued" button instead of an active Retry. - The adopt-all loop re-checks each entry's live status and skips entries already running or imported. Also drop a redundant isAdoptingAll guard (adoptAllProgress alone is the truth).
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
4 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
Recorded as integrated via -s ours after batch PRs #455-#464. Taken via individual PRs: - PR 1 (#455): v2 polish 前半 safe set (9 commits) - PR 2 (#456): v2/host-service polish 中盤 (12 commits) - PR 3 (#457): sidebar polish + jwt API (5 commits) - PR 4 (#458): host-service tRPC retry/cache/timeout (3 commits) - PR 5 (#459): v2 diff pane / file pane polish (2 commits) - PR 7 (#462): host-service v2 canonical workspace.create + attachment store (PR1 superset-sh#3893 + PR2 superset-sh#3916) - PR 11 (#463): agents API + onboarding (7 commits + 1 cleanup) - PR 12 (#464): v1→v2 import flow rewrite (11 commits + 2 follow-ups) - PR 13 (#460): host-service shell env probe + typo (2 commits) - PR 16 (#461): marketplace 19 themes (1 commit) Skipped / deferred (recorded as integrated for behind=0): - PR 6: CLI v1 launch (superset-sh#3898 + 30+ CLI/SDK followups) — defer to dedicated migration - PR 9: v2 PR3 (superset-sh#3940) + revert (superset-sh#4017) — net-zero pair - PR 10: pty-daemon (superset-sh#3896, superset-sh#3971, superset-sh#4054) — fork keeps its terminal-host - PR 14: Slack MCP-v2 (superset-sh#4197, superset-sh#4208) — depends on mcp-v2/sdk divergence - PR 15: onboarding remaining (superset-sh#4115, superset-sh#4125, superset-sh#4214, superset-sh#4213, superset-sh#4222, superset-sh#4225) — depends on fork's deleted setup pages Behind: 0 after this merge.
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
<Button>for a<div>mid-flight; Radix Dialog then re-focused the dialog root. The adopting state is now a disabled button (with spinner + "Adopting…") so focus stays on the row.ImportWorkspacesPageso Adopt-all and per-row clicks share one state machine.Test plan
Summary by cubic
Stabilizes the Adopt button and adds Adopt‑all with progress in the v1→v2 import modal, with a cleaner Linear‑style UI. Prevents duplicate adopts during bulk runs and ensures adopted workspaces appear in the sidebar.
Bug Fixes
New Features
ImportWorkspacesPage; sidebar updates and the cloud list is invalidated after adoption.Written for commit e89a621. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements