revert(desktop): optimistic v2 workspace.create (#4120)#4135
Merged
Conversation
This reverts commit 3b6615d.
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR refactors workspace creation from a failure-tracking model to an in-flight-tracking model, introduces a new ChangesWorkspace Creation: In-Flight Tracking and Hook Refactor
Sequence DiagramsequenceDiagram
participant User
participant DashboardUI as Dashboard Sidebar UI
participant useWorkspaceCreates as useWorkspaceCreates Hook
participant Store as In-Flight Store<br/>(useWorkspaceCreatesStore)
participant HostService as Host Service
participant Collections as Collections & DB
participant Manager as WorkspaceCreatesManager
User->>DashboardUI: Trigger workspace create
DashboardUI->>useWorkspaceCreates: submit(workspaceSpec)
useWorkspaceCreates->>Store: add(entry with "creating" state)
useWorkspaceCreates->>HostService: Create workspace via tRPC
HostService->>Collections: Mutate/insert workspace row
HostService-->>useWorkspaceCreates: Return workspace result
useWorkspaceCreates->>Collections: Update local workspace state<br/>& pane layout
par Parallel Monitoring
Manager->>Collections: Query live workspace IDs
Manager->>Store: Check in-flight entries
alt Workspace ID matches in-flight
Manager->>Store: remove(workspaceId)
end
end
useWorkspaceCreates->>Store: mark "creating"/"error" state
DashboardUI->>Store: Read in-flight entries
DashboardUI->>User: Render pending workspace item
User->>DashboardUI: Click retry or dismiss
DashboardUI->>useWorkspaceCreates: retry(workspaceId) or dismiss(workspaceId)
useWorkspaceCreates->>Store: Update/remove entry
DashboardUI->>User: Reflect updated state
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
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 reverts the optimistic v2 workspace create pattern from #4120, replacing it with a direct host-service TRPC call inside
Confidence Score: 4/5The crash fix is correct and well-targeted; the revert cleanly removes $synced as a create-loading signal. The new flow is simpler and the guard in useDashboardSidebarData prevents double-display during the Electric-sync window. Two observations hold back a clean merge: newly created workspace local state is always inserted with tabOrder: 0 instead of computing the prepend value, so workspace ordering within a project lane becomes non-deterministic when more than one workspace exists. Additionally, WorkspaceCreatesManager removes entries in any state (including error) as soon as their ID appears in the workspace collection, which could silently swallow an error banner in the rare case where the DB row lands before the TRPC error propagates back to the renderer. apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts — the tabOrder: 0 insertion; apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx — unconditional removal of error-state entries.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts | Core logic rewritten: optimistic Electric insert replaced with direct host-service TRPC call + in-flight store tracking. Local sidebar state insert uses hardcoded tabOrder: 0 instead of computing the correct prepend value. |
| apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx | New component that watches v2Workspaces via live query and removes in-flight entries once the workspace appears in Electric — clean design, but removes entries regardless of their state (including error). |
| apps/desktop/src/renderer/stores/workspace-creates/store.ts | Replaces useWorkspaceCreateFailuresStore (failures-only map) with useWorkspaceCreatesStore (array of in-flight entries with state creating |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Fixes the crash: creating/error states now driven by the dedicated in-flight store instead of $synced, so rename mutations no longer trigger the creating-spinner path. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts | Sidebar data now surfaces both creating and failed in-flight entries from the new store; pending worktrees no longer piggyback on $synced; localWorkspaceCandidates simplified to main-type-only query. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Removes the onInsert handler (and all its optimistic row / host-service call / txid machinery) from v2Workspaces. Logic moved to useWorkspaceCreates hook. |
| packages/host-service/src/trpc/router/workspaces/workspaces.ts | Drops txid from the workspace create response — additive field no longer needed since optimistic inserts are gone. |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | Removes getCurrentTxid calls and txid from the returned workspace row; simplifies create return paths. |
Sequence Diagram
sequenceDiagram
participant UI as Renderer UI
participant Store as useWorkspaceCreatesStore
participant Hook as useWorkspaceCreates
participant Host as Host Service (TRPC)
participant LocalState as v2WorkspaceLocalState
participant Electric as Electric Sync (v2Workspaces)
participant Manager as WorkspaceCreatesManager
UI->>Hook: submit(args)
Hook->>Store: add({ state: creating, ... })
Hook->>Host: workspaces.create.mutate(snapshot)
alt Success
Host-->>Hook: { workspace, terminals, agents, alreadyExists }
Hook->>LocalState: insert / update sidebarState + paneLayout
Hook-->>UI: { ok: true, workspaceId }
Note over UI,Store: UI shows WorkspaceCreatingState until Electric arrives
Electric-->>Manager: workspace row arrives in v2Workspaces
Manager->>Store: remove(id)
UI->>UI: render workspace normally
else alreadyExists at different id
Host-->>Hook: { workspace, alreadyExists: true }
Hook->>Store: remove(snapshotId)
Hook-->>UI: { ok: true, workspaceId: canonicalId, alreadyExists: true }
else Error
Host-->>Hook: throws
Hook->>Store: markError(workspaceId, message)
Hook-->>UI: { ok: false, error }
UI->>UI: render WorkspaceCreateErrorState
UI->>Hook: retry(workspaceId)
Hook->>Store: markCreating(workspaceId)
Hook->>Host: workspaces.create.mutate(snapshot)
end
Comments Outside Diff (2)
-
apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts, line 84-100 (link)New workspace always inserted with
tabOrder: 0The old
onInserthandler calledgetPrependTabOrder(projectTabOrders)so the new workspace landed at the top of its project lane. The new code uses the hardcoded value0. When an existing workspace already holdstabOrder: 0(or a negative value from a prior prepend), the new workspace will sort to the same position, and the sidebar order becomes non-deterministic for any project that already has workspaces.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts Line: 84-100 Comment: **New workspace always inserted with `tabOrder: 0`** The old `onInsert` handler called `getPrependTabOrder(projectTabOrders)` so the new workspace landed at the top of its project lane. The new code uses the hardcoded value `0`. When an existing workspace already holds `tabOrder: 0` (or a negative value from a prior prepend), the new workspace will sort to the same position, and the sidebar order becomes non-deterministic for any project that already has workspaces. How can I resolve this? If you propose a fix, please make it concise.
-
apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx, line 17-27 (link)WorkspaceCreatesManagerremoves every in-flight entry whose ID is present inv2Workspaces, regardless of the entry'sstate. If a create call both inserts the DB row and throws an error (e.g., the TRPC call succeeds server-side but the response is corrupted),markErrorsets the entry to"error", but the Manager will then silently remove it once Electric delivers the row — the user never sees the error banner and has no indication of why the workspace appeared unexpectedly.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx Line: 17-27 Comment: **Error-state entries silently cleared when workspace appears in collections** `WorkspaceCreatesManager` removes every in-flight entry whose ID is present in `v2Workspaces`, regardless of the entry's `state`. If a create call both inserts the DB row and throws an error (e.g., the TRPC call succeeds server-side but the response is corrupted), `markError` sets the entry to `"error"`, but the Manager will then silently remove it once Electric delivers the row — the user never sees the error banner and has no indication of why the workspace appeared unexpectedly. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts:84-100
**New workspace always inserted with `tabOrder: 0`**
The old `onInsert` handler called `getPrependTabOrder(projectTabOrders)` so the new workspace landed at the top of its project lane. The new code uses the hardcoded value `0`. When an existing workspace already holds `tabOrder: 0` (or a negative value from a prior prepend), the new workspace will sort to the same position, and the sidebar order becomes non-deterministic for any project that already has workspaces.
### Issue 2 of 2
apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx:17-27
**Error-state entries silently cleared when workspace appears in collections**
`WorkspaceCreatesManager` removes every in-flight entry whose ID is present in `v2Workspaces`, regardless of the entry's `state`. If a create call both inserts the DB row and throws an error (e.g., the TRPC call succeeds server-side but the response is corrupted), `markError` sets the entry to `"error"`, but the Manager will then silently remove it once Electric delivers the row — the user never sees the error banner and has no indication of why the workspace appeared unexpectedly.
Reviews (1): Last reviewed commit: "Revert "feat(desktop): optimistic v2 wor..." | Re-trigger Greptile
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
4 tasks
saddlepaddle
added a commit
that referenced
this pull request
May 6, 2026
The revert of #4120 (#4135) restored a hardcoded `tabOrder: 0` in the v2 workspace.create path. Existing rows default to `tabOrder: 0` too, so new workspaces tied with them and ordering was non-deterministic. Restores the `getPrependTabOrder` helper from #4120 as a shared util in `dashboardSidebarLocal/`, and uses it from `useWorkspaceCreates` so new rows land strictly above every existing top-level item — matching what the pending-row injection in `useDashboardSidebarData` already assumes.
6 tasks
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 8, 2026
) The revert of superset-sh#4120 (superset-sh#4135) restored a hardcoded `tabOrder: 0` in the v2 workspace.create path. Existing rows default to `tabOrder: 0` too, so new workspaces tied with them and ordering was non-deterministic. Restores the `getPrependTabOrder` helper from superset-sh#4120 as a shared util in `dashboardSidebarLocal/`, and uses it from `useWorkspaceCreates` so new rows land strictly above every existing top-level item — matching what the pending-row injection in `useDashboardSidebarData` already assumes.
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
v2-workspace/layout.tsxif (workspace && !isSynced)branch catches any in-flight optimistic mutation, not just creates — so renaming a workspace flips$synced=false, the layout enters the "Creating workspace…" path, andworkspace.createdAt.getTime()crashes the renderer.$synced-as-create-loading-signal pattern is too brittle; reverting until we have a separate "is this an in-flight create?" signal.Notes
v2Workspace.createdrops the additivetxidfield. Existing 1.8.5 clients gracefully fall back to shape-stream sync (slightly longer create spinner, no crash).Test plan
Summary by cubic
Reverts the optimistic v2 workspace.create flow and adds an explicit in‑flight creates store to fix crashes and unreliable loading states. Creation now calls the host service from the renderer and settles via shape sync (no txid).
Bug Fixes
$synced=falsein v2 workspace layout.Refactors
useWorkspaceCreatesStoreandWorkspaceCreatesManager; removeduseWorkspaceCreateFailuresStore.collections.v2Workspaces.onInsertintouseWorkspaceCreates(direct host call, pane layout setup, local state insert).txidfrom host-service and@trpccreate APIs.$synced; simplified local main workspace auto-include and in-flight row injection; localized tabOrder helpers.Written for commit 6aefdc9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes