feat(desktop): optimistic v2 workspace.create#4120
Merged
Conversation
Wires `collections.v2Workspaces.insert` as the entry point for new v2 workspaces. The optimistic row appears immediately in every live query (sidebar, list page, detail page) and resolves once Electric syncs the host-service insert. On failure, Electric rolls back the row and the renderer persists a failure record keyed by workspace id so the user can retry from the detail page. API: `v2Workspace.create` returns Postgres `txid` on writes (omitted on idempotent reuse) so Electric can match the synced shape. Host-service threads the txid back through `workspaces.create` and the `adoptExistingWorktree` paths. Renderer: `onInsert` on the v2_workspaces collection invokes `workspaces.create` on the host service via a metadata sidecar (host URL plus the create args that don't live on the row). Throws a typed `WorkspaceAlreadyExistsAtDifferentIdError` when the host returns a canonical id different from the optimistic one — Electric rolls back the row and the submit hook redirects to the canonical id. Sidebar reads `$synced` directly off the v2_workspaces row to flag optimistic worktrees and renders them with the existing `creationStatus: "creating"` indicator. Detail page transitions between `WorkspaceCreatingState` and the actual workspace via the same flag. Failed creates render via the failures store. New workspaces land at the top of the sidebar by inserting their local state row at `getPrependTabOrder(...)` instead of a hardcoded `0`.
Contributor
📝 WalkthroughWalkthroughReplaces renderer in-flight workspace-create tracking with a failure-only store, adds optimistic local inserts for workspace creation, propagates cloud transaction IDs (txid) through creation APIs, and updates sidebar/dashboard UI to surface creating/failed states and retry/dismiss flows. ChangesWorkspace Creation Refactor: Failure Store + Optimistic Inserts
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Dashboard UI
participant Collections as Collections DB
participant HostService as Host Service
participant FailureStore as Failure Store
rect rgba(100, 150, 255, 0.5)
Note over User,HostService: Optimistic creation + sync flow
User->>UI: Submit workspace create
UI->>Collections: Insert optimistic v2_workspace with metadata
Collections->>Collections: onInsert handler runs
Collections->>HostService: Request create/adopt worktree
HostService-->>Collections: Success (includes txid)
Collections->>Collections: Mirror pane layout, init local state
UI->>Collections: Observe $synced flag
UI->>UI: Render creating state until synced
end
rect rgba(255, 100, 100, 0.5)
Note over UI,FailureStore: Failure and recovery
Collections->>FailureStore: Record failed create (snapshot + error)
FailureStore-->>UI: Failure visible in sidebar
UI->>UI: Render error state (retry/dismiss)
alt Retry
UI->>Collections: Re-insert using failure snapshot via submit
Collections->>HostService: Retry create
else Dismiss
UI->>FailureStore: Clear failure
FailureStore-->>UI: Remove failed row
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR replaces a renderer-side in-flight Zustand store with Electric's native
Confidence Score: 3/5The core optimistic-insert architecture is sound, but two gaps in sidebar-to-detail-page consistency and the unguarded metadata access in After
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Adds onInsert handler to v2Workspaces that calls the host service, manages local state, and returns txid — but accesses transaction.mutations[0] and casts metadata without defensive guards. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts | Replaces in-flight store with live $synced query for creating status, but sidebarWithSyncMeta loses the creating indicator before Electric confirms sync after local state is inserted. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Switches creating/error state detection from in-flight store to workspace.$synced and failures store; isSynced defaults to false for null workspace which is a semantic mismatch but harmless today. |
| apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts | Refactored to build an optimistic row and use collections.v2Workspaces.insert with metadata; failures are now recorded only on rollback/error. Retry and dismiss wire up correctly to the new store. |
| apps/desktop/src/renderer/stores/workspace-creates/store.ts | Clean redesign — replaces the in-flight multi-state entries array with a narrow failures-only Record<id, FailedWorkspaceCreate> store; WorkspaceAlreadyExistsAtDifferentIdError is a well-scoped custom error class. |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | Returns { ...row, txid } from all v2Workspace.create code paths — fresh inserts get a real txid from getCurrentTxid, idempotent/reuse paths return txid: undefined. |
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/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts:307-311
**Sidebar spinner dropped before Electric confirms `$synced`**
After `onInsert` completes its host-service call and inserts the `v2WorkspaceLocalState` row, the workspace moves from `autoIncluded` (where `creationStatus: "creating"` is set) into `sidebarWithSyncMeta`. Because `rawSidebarWorkspaces` does not select `$synced`, `sidebarWithSyncMeta` unconditionally sets `synced: true` / `creationStatus: undefined`. From that point until Electric's shape stream confirms the txid and flips `$synced = true`, the sidebar shows no creating indicator while the detail page still renders `WorkspaceCreatingState` (which reads `workspace?.$synced` directly). The window is bounded by Electric's shape-stream round-trip — typically short, but observable during slow or congested sync.
### Issue 2 of 3
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts:408-409
**No guard against missing `metadata` or empty `mutations` array**
`transaction.mutations[0]` is accessed without checking that the array has at least one element, and `metadata` is immediately cast to `WorkspaceCreateMeta` without validation. If anything calls `v2Workspaces.insert` without a `metadata` object (e.g., a future code path or a test), `onInsert` will throw `TypeError: Cannot read properties of undefined` when destructuring `modified` and `metadata`, which Electric would surface as an unhandled rejection and roll back the row without a user-visible error message.
### Issue 3 of 3
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx:39
**`?? false` default for `isSynced` is semantically inverted**
When `workspace` is `null` (not found in the collection), `isSynced` resolves to `false`. The `?? false` default is harmless today because `isSynced` is only meaningful when `workspace` is truthy (the `if (workspace && !isSynced)` guard prevents misuse), but it risks confusion if a future branch reads `isSynced` without first asserting `workspace` is non-null. `?? true` would be the safer semantic default.
```suggestion
const isSynced = workspace?.$synced ?? true;
```
Reviews (1): Last reviewed commit: "feat(desktop): optimistic v2 workspace.c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+307
to
+311
| const sidebarWithSyncMeta = sidebarWorkspaces.map((workspace) => ({ | ||
| ...workspace, | ||
| synced: true, | ||
| creationStatus: undefined, | ||
| })); |
There was a problem hiding this comment.
$synced
After onInsert completes its host-service call and inserts the v2WorkspaceLocalState row, the workspace moves from autoIncluded (where creationStatus: "creating" is set) into sidebarWithSyncMeta. Because rawSidebarWorkspaces does not select $synced, sidebarWithSyncMeta unconditionally sets synced: true / creationStatus: undefined. From that point until Electric's shape stream confirms the txid and flips $synced = true, the sidebar shows no creating indicator while the detail page still renders WorkspaceCreatingState (which reads workspace?.$synced directly). The window is bounded by Electric's shape-stream round-trip — typically short, but observable during slow or congested sync.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 307-311
Comment:
**Sidebar spinner dropped before Electric confirms `$synced`**
After `onInsert` completes its host-service call and inserts the `v2WorkspaceLocalState` row, the workspace moves from `autoIncluded` (where `creationStatus: "creating"` is set) into `sidebarWithSyncMeta`. Because `rawSidebarWorkspaces` does not select `$synced`, `sidebarWithSyncMeta` unconditionally sets `synced: true` / `creationStatus: undefined`. From that point until Electric's shape stream confirms the txid and flips `$synced = true`, the sidebar shows no creating indicator while the detail page still renders `WorkspaceCreatingState` (which reads `workspace?.$synced` directly). The window is bounded by Electric's shape-stream round-trip — typically short, but observable during slow or congested sync.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+408
to
+409
| const { modified, metadata } = transaction.mutations[0]; | ||
| const meta = metadata as WorkspaceCreateMeta; |
There was a problem hiding this comment.
metadata or empty mutations array
transaction.mutations[0] is accessed without checking that the array has at least one element, and metadata is immediately cast to WorkspaceCreateMeta without validation. If anything calls v2Workspaces.insert without a metadata object (e.g., a future code path or a test), onInsert will throw TypeError: Cannot read properties of undefined when destructuring modified and metadata, which Electric would surface as an unhandled rejection and roll back the row without a user-visible error message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
Line: 408-409
Comment:
**No guard against missing `metadata` or empty `mutations` array**
`transaction.mutations[0]` is accessed without checking that the array has at least one element, and `metadata` is immediately cast to `WorkspaceCreateMeta` without validation. If anything calls `v2Workspaces.insert` without a `metadata` object (e.g., a future code path or a test), `onInsert` will throw `TypeError: Cannot read properties of undefined` when destructuring `modified` and `metadata`, which Electric would surface as an unhandled rejection and roll back the row without a user-visible error message.
How can I resolve this? If you propose a fix, please make it concise.| : undefined, | ||
| // Read `$synced` straight off the row — useLiveQuery returns rows enriched | ||
| // with virtual props, and changes to optimistic state retrigger the query. | ||
| const isSynced = workspace?.$synced ?? false; |
There was a problem hiding this comment.
?? false default for isSynced is semantically inverted
When workspace is null (not found in the collection), isSynced resolves to false. The ?? false default is harmless today because isSynced is only meaningful when workspace is truthy (the if (workspace && !isSynced) guard prevents misuse), but it risks confusion if a future branch reads isSynced without first asserting workspace is non-null. ?? true would be the safer semantic default.
Suggested change
| const isSynced = workspace?.$synced ?? false; | |
| const isSynced = workspace?.$synced ?? true; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
Line: 39
Comment:
**`?? false` default for `isSynced` is semantically inverted**
When `workspace` is `null` (not found in the collection), `isSynced` resolves to `false`. The `?? false` default is harmless today because `isSynced` is only meaningful when `workspace` is truthy (the `if (workspace && !isSynced)` guard prevents misuse), but it risks confusion if a future branch reads `isSynced` without first asserting `workspace` is non-null. `?? true` would be the safer semantic default.
```suggestion
const isSynced = workspace?.$synced ?? true;
```
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: 3
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/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx (1)
144-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a failure-specific accessible label here.
creationStatus === "failed"still announcesCreating workspace: …in collapsed mode, so screen readers get the wrong state.Suggested fix
creationStatus={creationStatus} aria-label={ - creationStatus ? `Creating workspace: ${name}` : undefined + creationStatus === "creating" + ? `Creating workspace: ${name}` + : creationStatus === "failed" + ? `Workspace creation failed: ${name}` + : undefined } />🤖 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/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx` around lines 144 - 147, The aria-label currently always says "Creating workspace: {name}" when creationStatus is truthy, which miscommunicates the state when creationStatus === "failed"; update the aria-label logic inside DashboardSidebarWorkspaceItem to return a failure-specific label (e.g., `Failed to create workspace: ${name}`) when creationStatus === "failed", `Creating workspace: ${name}` when creationStatus === "creating" (or other in-progress value), and undefined otherwise so screen readers receive the correct state for the name prop.
🤖 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/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 307-310: The code currently forces every sidebar row to synced:
true in the sidebarWithSyncMeta mapping which hides optimistic "creating" state;
instead, stop overriding the real sync flag — in useDashboardSidebarData replace
the hardcoded synced: true with the actual sync state from the workspace (or the
rawSidebarWorkspaces / v2Workspaces.$synced lookup) so rows backed by
v2WorkspaceLocalState keep their true $synced value; only clear creationStatus
(creationStatus: undefined) when that resolved/synced value is true so
optimistic rows remain in "creating" until onInsert / v2Workspaces.$synced
confirms the insert.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Around line 411-479: The remote create (client.workspaces.create.mutate) must
not be coupled to subsequent local-state work (appendLaunchesToPaneLayout,
v2WorkspaceLocalState.update/insert) so that failures in mirroring don't roll
back an already-created workspace; after calling client.workspaces.create.mutate
and handling the WorkspaceAlreadyExistsAtDifferentIdError case as now, perform
the paneLayout computation and v2WorkspaceLocalState.update/insert inside a
try/catch that does not throw back to the caller on failure—log or surface a
repairable local-state error separately (e.g., emit to a local error reporter)
and still return the successful result.txid; ensure only the canonical-id
conflict still throws by keeping the existing throw of
WorkspaceAlreadyExistsAtDifferentIdError for result.alreadyExists with
mismatched id.
In `@apps/desktop/src/renderer/stores/workspace-creates/store.ts`:
- Around line 58-71: The current useWorkspaceCreateFailuresStore
(WorkspaceCreateFailuresState) stores failures in-memory so records made by
record(workspaceId, entry) and cleared by clear(workspaceId) are lost on reload;
wrap the create(...) with Zustand's persist middleware (or the app's existing
persisted store helper) to persist the failures map under a stable key (e.g.,
"workspace-create-failures") to localStorage / electron store, ensure
rehydration on startup, and keep the same API (record/clear) while updating
types if necessary and avoiding persisting non-serializable fields.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx`:
- Around line 144-147: The aria-label currently always says "Creating workspace:
{name}" when creationStatus is truthy, which miscommunicates the state when
creationStatus === "failed"; update the aria-label logic inside
DashboardSidebarWorkspaceItem to return a failure-specific label (e.g., `Failed
to create workspace: ${name}`) when creationStatus === "failed", `Creating
workspace: ${name}` when creationStatus === "creating" (or other in-progress
value), and undefined otherwise so screen readers receive the correct state for
the name prop.
🪄 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: a1edd7cf-689d-4d0a-b1db-78297473c837
📒 Files selected for processing (15)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/index.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/tabOrder.tsapps/desktop/src/renderer/stores/workspace-creates/Manager.tsxapps/desktop/src/renderer/stores/workspace-creates/index.tsapps/desktop/src/renderer/stores/workspace-creates/store.tsapps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.tspackages/host-service/src/trpc/router/workspace-creation/shared/adopt-existing-worktree.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx
- apps/desktop/src/renderer/stores/workspace-creates/Manager.tsx
Comment on lines
+58
to
+71
| export const useWorkspaceCreateFailuresStore = | ||
| create<WorkspaceCreateFailuresState>((set) => ({ | ||
| failures: {}, | ||
| record: (workspaceId, entry) => | ||
| set((state) => ({ | ||
| entries: state.entries.filter( | ||
| (entry) => entry.snapshot.id !== workspaceId, | ||
| ), | ||
| failures: { ...state.failures, [workspaceId]: entry }, | ||
| })), | ||
| }), | ||
| ); | ||
| clear: (workspaceId) => | ||
| set((state) => { | ||
| if (!(workspaceId in state.failures)) return state; | ||
| const { [workspaceId]: _, ...rest } = state.failures; | ||
| return { failures: rest }; | ||
| }), | ||
| })); |
Contributor
There was a problem hiding this comment.
Persist rollback entries instead of keeping them renderer-memory only.
This plain Zustand create() store is wiped on refresh/relaunch, so a failed create loses its retry/dismiss snapshot as soon as the renderer reloads. That makes the failure-only flow unreliable once the optimistic row has already rolled back.
Suggested direction
import { create } from "zustand";
+import { createJSONStorage, persist } from "zustand/middleware";
...
-export const useWorkspaceCreateFailuresStore =
- create<WorkspaceCreateFailuresState>((set) => ({
- failures: {},
- record: (workspaceId, entry) =>
- set((state) => ({
- failures: { ...state.failures, [workspaceId]: entry },
- })),
- clear: (workspaceId) =>
- set((state) => {
- if (!(workspaceId in state.failures)) return state;
- const { [workspaceId]: _, ...rest } = state.failures;
- return { failures: rest };
- }),
- }));
+export const useWorkspaceCreateFailuresStore =
+ create<WorkspaceCreateFailuresState>()(
+ persist(
+ (set) => ({
+ failures: {},
+ record: (workspaceId, entry) =>
+ set((state) => ({
+ failures: { ...state.failures, [workspaceId]: entry },
+ })),
+ clear: (workspaceId) =>
+ set((state) => {
+ if (!(workspaceId in state.failures)) return state;
+ const { [workspaceId]: _, ...rest } = state.failures;
+ return { failures: rest };
+ }),
+ }),
+ {
+ name: "workspace-create-failures",
+ storage: createJSONStorage(() => localStorage),
+ },
+ ),
+ );📝 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
| export const useWorkspaceCreateFailuresStore = | |
| create<WorkspaceCreateFailuresState>((set) => ({ | |
| failures: {}, | |
| record: (workspaceId, entry) => | |
| set((state) => ({ | |
| entries: state.entries.filter( | |
| (entry) => entry.snapshot.id !== workspaceId, | |
| ), | |
| failures: { ...state.failures, [workspaceId]: entry }, | |
| })), | |
| }), | |
| ); | |
| clear: (workspaceId) => | |
| set((state) => { | |
| if (!(workspaceId in state.failures)) return state; | |
| const { [workspaceId]: _, ...rest } = state.failures; | |
| return { failures: rest }; | |
| }), | |
| })); | |
| export const useWorkspaceCreateFailuresStore = | |
| create<WorkspaceCreateFailuresState>()( | |
| persist( | |
| (set) => ({ | |
| failures: {}, | |
| record: (workspaceId, entry) => | |
| set((state) => ({ | |
| failures: { ...state.failures, [workspaceId]: entry }, | |
| })), | |
| clear: (workspaceId) => | |
| set((state) => { | |
| if (!(workspaceId in state.failures)) return state; | |
| const { [workspaceId]: _, ...rest } = state.failures; | |
| return { failures: rest }; | |
| }), | |
| }), | |
| { | |
| name: "workspace-create-failures", | |
| storage: createJSONStorage(() => localStorage), | |
| }, | |
| ), | |
| ); |
🤖 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/stores/workspace-creates/store.ts` around lines 58
- 71, The current useWorkspaceCreateFailuresStore (WorkspaceCreateFailuresState)
stores failures in-memory so records made by record(workspaceId, entry) and
cleared by clear(workspaceId) are lost on reload; wrap the create(...) with
Zustand's persist middleware (or the app's existing persisted store helper) to
persist the failures map under a stable key (e.g., "workspace-create-failures")
to localStorage / electron store, ensure rehydration on startup, and keep the
same API (record/clear) while updating types if necessary and avoiding
persisting non-serializable fields.
Drop `retry` and `dismiss` from the hook's API — they were thin wrappers around `useWorkspaceCreateFailuresStore`. `WorkspaceCreateErrorState` now reads/writes the store directly for retry/dismiss, matching the precedent already set by `DashboardSidebarWorkspaceItem`. Collapse the two early-return validation branches (no org, no host URL) into throws inside the same try/catch that handles host-service failures, so all failure paths flow through one record-to-store path. Dismiss now navigates via `useNavigateAwayFromWorkspace` (same hook the destroy flow uses) — lands on the top sidebar workspace, falling back to "/" if the sidebar is empty, instead of always sending the user back to the v2 workspaces list.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts (1)
99-99: ⚡ Quick winDouble cast bypasses metadata type-checking.
meta as unknown as Record<string, unknown>discards theWorkspaceCreateMetatype contract at the boundary, so a future drift between this sidecar andcollections.ts:onInsert's reader will not be caught by the compiler. If the cast exists only because optional props don't widen toRecord<string, unknown>cleanly, prefer typing theinsertmetadata generic (or acceptingWorkspaceCreateMetadirectly in the collection definition) so both ends share one type.🤖 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/stores/workspace-creates/useWorkspaceCreates.ts` at line 99, The code is double-casting meta with "meta as unknown as Record<string, unknown>" which defeats compile-time checks; remove the double-cast and make the insert metadata strongly typed by sharing WorkspaceCreateMeta across boundary—either change the collection definition (the insert metadata generic in collections.ts/onInsert) to accept WorkspaceCreateMeta or call insert with the properly typed generic so useWorkspaceCreates (the meta variable) is passed as WorkspaceCreateMeta instead of casting to Record<string, unknown>.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts`:
- Line 99: The code is double-casting meta with "meta as unknown as
Record<string, unknown>" which defeats compile-time checks; remove the
double-cast and make the insert metadata strongly typed by sharing
WorkspaceCreateMeta across boundary—either change the collection definition (the
insert metadata generic in collections.ts/onInsert) to accept
WorkspaceCreateMeta or call insert with the properly typed generic so
useWorkspaceCreates (the meta variable) is passed as WorkspaceCreateMeta instead
of casting to Record<string, unknown>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 676372ec-4b64-4156-b541-ebc3406f94f1
📒 Files selected for processing (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/components/WorkspaceCreateErrorState/WorkspaceCreateErrorState.tsxapps/desktop/src/renderer/stores/workspace-creates/index.tsapps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/stores/workspace-creates/index.ts
Two PR-review fixes for the optimistic v2 workspace.create path. 1. Sidebar spinner gap. `rawSidebarWorkspaces` now selects `synced: workspaces.\$synced`, and `sidebarWithSyncMeta` derives `creationStatus` from the actual flag instead of hardcoding `synced: true`. Without this, the moment `onInsert` wrote `v2WorkspaceLocalState`, the workspace flipped from the optimistic-only injection into the local-state-backed query and lost its spinner — while the detail page (which reads `\$synced` directly off the row) kept rendering `WorkspaceCreatingState` until Electric confirmed the txid. Now both surfaces stay in lockstep until sync. 2. Local-state write coupling. The post-create paneLayout setup (`appendLaunchesToPaneLayout` + `v2WorkspaceLocalState` insert/update) was inside `onInsert` but unguarded — a localStorage failure after a successful remote create would throw back to Electric, roll the optimistic row back, and the user would see a false "create failed" state even though the host already wrote the workspace. Wrap that section in try/catch + console.warn so local-state failures stay local; the detail page's `ensureWorkspaceInSidebar` mount effect re-syncs sidebar state on next view.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts (1)
487-527: 💤 Low valueStabilize
createdAt/updatedAtfor failed-item rows to preserve project fingerprint stability.
new Date()is invoked insidecomputedGroups, which re-runs whenever any of its many dependencies change (machineId,sidebarProjects,sidebarSections,visibleSidebarWorkspaces,pullRequestsByWorkspaceId,localStateWorkspaceIds, etc.) — not only when failures change. Each rerun produces freshDateinstances for every failed item, which propagate into the project's JSON fingerprint computed bygetDashboardSidebarProjectFingerprint(line 60). The result:useStableDashboardSidebarProjectswill always treat any project containing a failed row as "changed", partially defeating the stability layer.Hoist the timestamps into
failureSidebarRows(which only recomputes whenfailuresMapactually changes), or pull them fromentry.snapshot.createdAt/updatedAtif the snapshot already carries them.♻️ Suggested fix
const failureSidebarRows = useMemo( () => Object.entries(failuresMap).map(([id, entry]) => ({ id, projectId: entry.snapshot.projectId, hostId: entry.hostId, name: entry.snapshot.name ?? "New workspace", branchName: entry.snapshot.branch ?? entry.snapshot.name ?? "New workspace", + createdAt: entry.snapshot.createdAt ?? new Date(), + updatedAt: entry.snapshot.updatedAt ?? new Date(), })), [failuresMap], ); @@ - createdAt: new Date(), - updatedAt: new Date(), + createdAt: failure.createdAt, + updatedAt: failure.updatedAt, creationStatus: "failed",🤖 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/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts` around lines 487 - 527, The computedGroups loop creates new Date() for createdAt/updatedAt on every recompute which destabilizes project fingerprints; instead derive stable timestamps from the failure snapshot or compute them once when failureSidebarRows is built so they only change when failuresMap changes—update useDashboardSidebarData so the failedItem createdAt/updatedAt come from entry.snapshot.createdAt/updatedAt or from a timestamp stored on failureSidebarRows (constructed where failureSidebarRows is produced) rather than calling new Date() inside computedGroups, ensuring getDashboardSidebarProjectFingerprint and useStableDashboardSidebarProjects see stable values.apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts (1)
408-410: ⚡ Quick winGuard the
WorkspaceCreateMetacontract before dereferencing it.This callback now hard-depends on
metadata, but the boundary is still just an unchecked cast. If a future caller inserts intov2Workspaceswithout the expected metadata, or the payload shape drifts, this will fail with a generic property-access error instead of a diagnosable create failure.🛡️ Minimal guard
onInsert: async ({ transaction }) => { const { modified, metadata } = transaction.mutations[0]; + if ( + !metadata || + typeof metadata !== "object" || + !("hostUrl" in metadata) + ) { + throw new Error( + "v2Workspaces.insert requires WorkspaceCreateMeta metadata", + ); + } const meta = metadata as WorkspaceCreateMeta; const client = getHostServiceClientByUrl(meta.hostUrl);🤖 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/providers/CollectionsProvider/collections.ts` around lines 408 - 410, The code assumes transaction.mutations[0].metadata conforms to WorkspaceCreateMeta and directly casts it; guard that contract first by checking the metadata exists and has the expected shape (e.g., metadata && typeof metadata.hostUrl === 'string' and any other required fields) before assigning meta and calling getHostServiceClientByUrl(meta.hostUrl); if the guard fails, log or throw a clear, diagnosable error (including the offending metadata) and return early instead of dereferencing invalid properties inside the WorkspaceCreateMeta handling path.
🤖 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.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 487-527: The computedGroups loop creates new Date() for
createdAt/updatedAt on every recompute which destabilizes project fingerprints;
instead derive stable timestamps from the failure snapshot or compute them once
when failureSidebarRows is built so they only change when failuresMap
changes—update useDashboardSidebarData so the failedItem createdAt/updatedAt
come from entry.snapshot.createdAt/updatedAt or from a timestamp stored on
failureSidebarRows (constructed where failureSidebarRows is produced) rather
than calling new Date() inside computedGroups, ensuring
getDashboardSidebarProjectFingerprint and useStableDashboardSidebarProjects see
stable values.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts`:
- Around line 408-410: The code assumes transaction.mutations[0].metadata
conforms to WorkspaceCreateMeta and directly casts it; guard that contract first
by checking the metadata exists and has the expected shape (e.g., metadata &&
typeof metadata.hostUrl === 'string' and any other required fields) before
assigning meta and calling getHostServiceClientByUrl(meta.hostUrl); if the guard
fails, log or throw a clear, diagnosable error (including the offending
metadata) and return early instead of dereferencing invalid properties inside
the WorkspaceCreateMeta handling path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e15b3be-96c4-459b-877e-02d0ce748524
📒 Files selected for processing (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
3 tasks
saddlepaddle
added a commit
that referenced
this pull request
May 6, 2026
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.
5 tasks
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
collections.v2Workspaces.insertso the row appears optimistically across every live query (sidebar, list page, detail page) and resolves via$syncedonce Electric confirms the host-service insert.txidfrom API → host-service → rendereronInsertso Electric matches the optimistic upsert against the synced shape.tabOrderviagetPrependTabOrder(...)instead of hardcoding0.Behavior
Submit.
useWorkspaceCreates.submitbuilds an optimisticSelectV2Workspace(placeholder name/branch when the user didn't supply them) plus aWorkspaceCreateMetasidecar (host URL, PR number, base branch, agent launches), then callscollections.v2Workspaces.insert(row, { metadata }). The row appears immediately with$synced=false.onInsert handler (
collections.ts) reads the metadata, callsworkspaces.createon the host service via the resolved client, and:{ txid }on a fresh write so Electric awaits the synced shape stream.WorkspaceAlreadyExistsAtDifferentIdError(canonicalId)when the host returns an existing workspace at a different id — Electric rolls back the optimistic row and the submit hook surfaces the canonical id to the caller.Sidebar (
useDashboardSidebarData) selectssynced: workspaces.$syncedin the local-machine candidates query and auto-includestype=worktreerows wheresynced === false, alongside the existingtype=mainauto-include.creationStatus: "creating"is attached on the row, read directly in the iteration loop. Failed creates inject from the failures store withcreationStatus: "failed".Detail page triggers
WorkspaceCreatingStatewhenworkspace && !workspace.$synced,WorkspaceCreateErrorStatewhen!workspace && failureEntry.ensureWorkspaceInSidebaronly fires after sync confirmation, so a rolled-back create doesn't leave a phantom local-state row.Failures store is a
Record<workspaceId, FailedWorkspaceCreate>zustand store.submitwrites on rollback,retryre-runsdispatchwith the saved snapshot,dismissclears.API
v2Workspace.createnow returns{ ...row, txid?: number }.txidis set on fresh inserts and main-workspace patches viagetCurrentTxid(tx). Idempotent return paths (existing row, no patch) omittxid— those rows are already synced and don't need an optimistic match.workspaces.createon the host service threads the txid throughregisterCloudAndLocaland theadoptExistingWorktreepaths and surfaces it in the procedure response.Test plan
OpenInWorkspaceV2) — same flowRunInWorkspacePopoverV2) — same flowuseBranchPickerController) — same flowWorkspaceCreateErrorStatev2Workspace.updateand other consumers of the create response still work (tests pass)Summary by cubic
Adds optimistic v2 workspace creates via
collections.v2Workspaces.insertso new workspaces show up immediately and settle via$synced. Replaces the in-flight store with a failures-only store and keeps the create hook to justsubmit.New Features
onInsertcalls hostworkspaces.createwith a metadata sidecar and returns{ txid }on fresh writes; throwsWorkspaceAlreadyExistsAtDifferentIdErrorto roll back and surface the canonical id.type=worktreerows with$synced=falseas "creating"; failed creates inject from the failures store. Detail page and sidebar read$syncedto show creating until confirmed;ensureWorkspaceInSidebarruns only after sync.v2Workspace.createreturns{ ...row, txid? }; host-service threadstxidthroughregisterCloudAndLocalandadoptExistingWorktreeso optimistic inserts match the synced row.getPrependTabOrder(...).Bug Fixes
$syncedis true, ensuring sidebar and detail page stay in sync.onInsertwith try/catch so a storage failure doesn’t roll back a successful remote create; the next view re-syncs local state.Written for commit 089f71d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes