[codex] Revert optimistic workspace Electric transaction work#4744
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (38)
📝 WalkthroughWalkthroughThis PR refactors workspace creation tracking from Electric-based ChangesIn-flight workspace creation refactor with store, sidebar UI, and submission flow updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Ready to review this PR? Stage has broken it down into 9 individual chapters for you: 
Chapters generated by Stage for commit 74f82cb on May 20, 2026 12:11am UTC. |
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
saddlepaddle
added a commit
that referenced
this pull request
May 20, 2026
Contributor
Greptile SummaryThis PR reverts the optimistic workspace creation and Electric write-sync path from #4707, restoring a renderer-side in-flight tracking store (
Confidence Score: 4/5Safe to merge as a revert; no new functional regressions were introduced beyond a few small gaps in the restoration. The PR faithfully reverts the workspace Electric-sync work and the new in-flight store machinery is straightforward. The main concerns are: getCurrentTxid casts xid8 directly to text without the 32-bit truncation or safe-integer guard; the test mock returns a string where the real function returns a number; keyboard-shortcut filtering dropped the isDeleting guard; and PENDING_WORKSPACE_TAB_ORDER shares the same sentinel as MAIN_WORKSPACE_TAB_ORDER. None of these are blocking. packages/db/src/utils/sql.ts (txid cast precision), packages/trpc/src/router/v2-project/v2-project.test.ts (mock type mismatch), and useDashboardSidebarShortcuts.ts (missing isDeleting filter).
|
| Filename | Overview |
|---|---|
| packages/db/src/utils/sql.ts | getCurrentTxid now returns pg_current_xact_id()::text (64-bit xid8) instead of ::xid::text (32-bit); the isSafeInteger guard is also removed, leaving no protection against precision loss on very large txids. |
| packages/trpc/src/router/v2-project/v2-project.test.ts | getCurrentTxidMock changed to return string "txid-123" while the real getCurrentTxid returns number; uploadIcon/resetIconToGitHub/removeIcon assertions now check txid: "txid-123" making the mock type-inconsistent with production. |
| apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts | Replaces optimistic TanStack DB insert with direct host-service call; stores in-flight state in Zustand; submit/retry/dismiss API looks correct. |
| packages/trpc/src/router/v2-host/v2-host.ts | renameHost and updateHostMemberRole drop NOT_FOUND guards; operations now silently succeed even when target row doesn't exist. Part of intentional revert but changes error-reporting contract. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Removes failedWorkspaceCreates collection, Electric onError JWT-refresh handlers, and workspace insert mutation; electricTxidMatch replaced with direct { txid } return objects. |
| apps/electric-proxy/src/electric.ts | Protocol params inlined as a string set instead of imported from @electric-sql/client; will need manual update if Electric adds/renames protocol params in future releases. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts | PENDING_WORKSPACE_TAB_ORDER and MAIN_WORKSPACE_TAB_ORDER both set to Number.MIN_SAFE_INTEGER; in-flight workspaces injected with cloud-row fallback path added cleanly. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts | isDeleting() filter removed from workspace shortcut list; workspaces currently being deleted are no longer excluded from keyboard shortcuts. |
Sequence Diagram
sequenceDiagram
participant UI as Renderer UI
participant Store as WorkspaceCreatesStore
participant Manager as WorkspaceCreatesManager
participant Host as Host Service
participant Electric as Electric Sync
UI->>Store: "add({ state: "creating" })"
UI->>UI: navigate to /v2-workspace/:id (optimistic)
UI->>Host: workspaces.create.mutate(snapshot)
alt success
Host-->>UI: "{ workspace, terminals, agents, alreadyExists }"
UI->>Store: markCloudRow(workspace.id, workspace)
UI->>Collections: insert/update v2WorkspaceLocalState
Note over UI,Electric: workspace detail renders from cloudRow fallback
Electric-->>Collections: v2Workspaces row delivered
Manager->>Store: remove(workspace.id)
Collections-->>UI: live query updates sidebar
else error
Host-->>UI: throws
UI->>Store: markError(workspaceId, error)
UI->>UI: show WorkspaceCreateErrorState
UI->>Store: retry / dismiss
end
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
packages/trpc/src/router/v2-project/v2-project.test.ts:4
The mock now returns a string `"txid-123"` but the real `getCurrentTxid` returns `Number.parseInt(txid, 10)` — a `number`. The `uploadIcon`, `resetIconToGitHub`, and `removeIcon` test assertions that check `txid: "txid-123"` are therefore validating against the wrong type. If the txid type ever regresses from number to string the tests would still pass, defeating the purpose of the assertion.
```suggestion
const getCurrentTxidMock = mock(async () => 123);
```
### Issue 2 of 4
packages/db/src/utils/sql.ts:24-33
`pg_current_xact_id()` returns an `xid8` (64-bit unsigned integer). Casting it directly to `text` and then calling `Number.parseInt` is safe today, but `xid8` values can exceed `Number.MAX_SAFE_INTEGER` (2⁵³−1) on a sufficiently busy/long-running database, at which point `parseInt` silently returns an imprecise value. The previous code used `::xid::text` to truncate to 32 bits before parsing and guarded with `Number.isSafeInteger`. Restoring either protection prevents a hard-to-diagnose mismatch between the value stored server-side and the value Electric waits for.
```suggestion
const result = await tx.execute<{ txid: string }>(
sql`SELECT pg_current_xact_id()::xid::text as txid`,
);
const raw = result.rows[0]?.txid;
const txid = raw === undefined ? Number.NaN : Number.parseInt(raw, 10);
if (!Number.isSafeInteger(txid)) {
throw new Error(`Failed to get valid txid: ${raw}`);
}
return txid;
```
### Issue 3 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts:24-27
`PENDING_WORKSPACE_TAB_ORDER` and `MAIN_WORKSPACE_TAB_ORDER` are both `Number.MIN_SAFE_INTEGER`. When a project has both a main workspace and a pending in-flight workspace at the same time, they share the same sort key, which can cause non-deterministic ordering. A distinct sentinel (e.g., `Number.MIN_SAFE_INTEGER + 1`) makes the intent explicit.
```suggestion
// Sits above every real workspace so the pending row lines up with the real one,
// which is inserted via getPrependTabOrder.
const PENDING_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER + 1;
const MAIN_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER;
```
### Issue 4 of 4
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts:56-62
**Missing `isDeleting` guard on keyboard shortcuts**
The previous filter excluded both non-synced workspaces and workspaces actively being deleted (`isDeleting`). The new filter only excludes workspaces with a `creationStatus`, so a workspace whose delete API call has been sent but whose Electric deletion event hasn't arrived yet will still receive a `⌘N` shortcut and be navigatable. `DeletingWorkspacesProvider` still tracks this state; the `isDeleting` check and its import were dropped from this file as part of the revert but can be restored with two lines.
Reviews (1): Last reviewed commit: "Revert "feat: optimistic workspace creat..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { TRPCError, type TRPCRouterRecord } from "@trpc/server"; | ||
|
|
||
| const getCurrentTxidMock = mock(async () => 123); | ||
| const getCurrentTxidMock = mock(async () => "txid-123"); |
Contributor
There was a problem hiding this comment.
"txid-123" but the real getCurrentTxid returns Number.parseInt(txid, 10) — a number. The uploadIcon, resetIconToGitHub, and removeIcon test assertions that check txid: "txid-123" are therefore validating against the wrong type. If the txid type ever regresses from number to string the tests would still pass, defeating the purpose of the assertion.
Suggested change
| const getCurrentTxidMock = mock(async () => "txid-123"); | |
| const getCurrentTxidMock = mock(async () => 123); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/router/v2-project/v2-project.test.ts
Line: 4
Comment:
The mock now returns a string `"txid-123"` but the real `getCurrentTxid` returns `Number.parseInt(txid, 10)` — a `number`. The `uploadIcon`, `resetIconToGitHub`, and `removeIcon` test assertions that check `txid: "txid-123"` are therefore validating against the wrong type. If the txid type ever regresses from number to string the tests would still pass, defeating the purpose of the assertion.
```suggestion
const getCurrentTxidMock = mock(async () => 123);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
24
to
+33
| const result = await tx.execute<{ txid: string }>( | ||
| sql`SELECT pg_current_xact_id()::xid::text as txid`, | ||
| sql`SELECT pg_current_xact_id()::text as txid`, | ||
| ); | ||
|
|
||
| const raw = result.rows[0]?.txid; | ||
| const txid = raw === undefined ? Number.NaN : Number.parseInt(raw, 10); | ||
| if (!Number.isSafeInteger(txid)) { | ||
| throw new Error(`Failed to get valid Electric txid: ${raw}`); | ||
| const txid = result.rows[0]?.txid; | ||
| if (!txid) { | ||
| throw new Error("Failed to get current transaction ID"); | ||
| } | ||
|
|
||
| return txid; | ||
| return Number.parseInt(txid, 10); |
Contributor
There was a problem hiding this comment.
pg_current_xact_id() returns an xid8 (64-bit unsigned integer). Casting it directly to text and then calling Number.parseInt is safe today, but xid8 values can exceed Number.MAX_SAFE_INTEGER (2⁵³−1) on a sufficiently busy/long-running database, at which point parseInt silently returns an imprecise value. The previous code used ::xid::text to truncate to 32 bits before parsing and guarded with Number.isSafeInteger. Restoring either protection prevents a hard-to-diagnose mismatch between the value stored server-side and the value Electric waits for.
Suggested change
| const result = await tx.execute<{ txid: string }>( | |
| sql`SELECT pg_current_xact_id()::xid::text as txid`, | |
| sql`SELECT pg_current_xact_id()::text as txid`, | |
| ); | |
| const raw = result.rows[0]?.txid; | |
| const txid = raw === undefined ? Number.NaN : Number.parseInt(raw, 10); | |
| if (!Number.isSafeInteger(txid)) { | |
| throw new Error(`Failed to get valid Electric txid: ${raw}`); | |
| const txid = result.rows[0]?.txid; | |
| if (!txid) { | |
| throw new Error("Failed to get current transaction ID"); | |
| } | |
| return txid; | |
| return Number.parseInt(txid, 10); | |
| const result = await tx.execute<{ txid: string }>( | |
| sql`SELECT pg_current_xact_id()::xid::text as txid`, | |
| ); | |
| const raw = result.rows[0]?.txid; | |
| const txid = raw === undefined ? Number.NaN : Number.parseInt(raw, 10); | |
| if (!Number.isSafeInteger(txid)) { | |
| throw new Error(`Failed to get valid txid: ${raw}`); | |
| } | |
| return txid; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/db/src/utils/sql.ts
Line: 24-33
Comment:
`pg_current_xact_id()` returns an `xid8` (64-bit unsigned integer). Casting it directly to `text` and then calling `Number.parseInt` is safe today, but `xid8` values can exceed `Number.MAX_SAFE_INTEGER` (2⁵³−1) on a sufficiently busy/long-running database, at which point `parseInt` silently returns an imprecise value. The previous code used `::xid::text` to truncate to 32 bits before parsing and guarded with `Number.isSafeInteger`. Restoring either protection prevents a hard-to-diagnose mismatch between the value stored server-side and the value Electric waits for.
```suggestion
const result = await tx.execute<{ txid: string }>(
sql`SELECT pg_current_xact_id()::xid::text as txid`,
);
const raw = result.rows[0]?.txid;
const txid = raw === undefined ? Number.NaN : Number.parseInt(raw, 10);
if (!Number.isSafeInteger(txid)) {
throw new Error(`Failed to get valid txid: ${raw}`);
}
return txid;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+24
to
27
| // Sits above every real workspace so the pending row lines up with the real one, | ||
| // which is inserted via getPrependTabOrder. | ||
| const PENDING_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER; | ||
| const MAIN_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER; |
Contributor
There was a problem hiding this comment.
PENDING_WORKSPACE_TAB_ORDER and MAIN_WORKSPACE_TAB_ORDER are both Number.MIN_SAFE_INTEGER. When a project has both a main workspace and a pending in-flight workspace at the same time, they share the same sort key, which can cause non-deterministic ordering. A distinct sentinel (e.g., Number.MIN_SAFE_INTEGER + 1) makes the intent explicit.
Suggested change
| // Sits above every real workspace so the pending row lines up with the real one, | |
| // which is inserted via getPrependTabOrder. | |
| const PENDING_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER; | |
| const MAIN_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER; | |
| // Sits above every real workspace so the pending row lines up with the real one, | |
| // which is inserted via getPrependTabOrder. | |
| const PENDING_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER + 1; | |
| const MAIN_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER; |
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: 24-27
Comment:
`PENDING_WORKSPACE_TAB_ORDER` and `MAIN_WORKSPACE_TAB_ORDER` are both `Number.MIN_SAFE_INTEGER`. When a project has both a main workspace and a pending in-flight workspace at the same time, they share the same sort key, which can cause non-deterministic ordering. A distinct sentinel (e.g., `Number.MIN_SAFE_INTEGER + 1`) makes the intent explicit.
```suggestion
// Sits above every real workspace so the pending row lines up with the real one,
// which is inserted via getPrependTabOrder.
const PENDING_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER + 1;
const MAIN_WORKSPACE_TAB_ORDER = Number.MIN_SAFE_INTEGER;
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
56
to
62
| const flattenedWorkspaces = useMemo( | ||
| () => | ||
| groups | ||
| .flatMap((project) => getProjectChildrenWorkspaces(project.children)) | ||
| .filter((workspace) => workspace.isSynced && !isDeleting(workspace.id)), | ||
| [groups, isDeleting], | ||
| .filter((workspace) => !workspace.creationStatus), | ||
| [groups], | ||
| ); |
Contributor
There was a problem hiding this comment.
isDeleting guard on keyboard shortcuts
The previous filter excluded both non-synced workspaces and workspaces actively being deleted (isDeleting). The new filter only excludes workspaces with a creationStatus, so a workspace whose delete API call has been sent but whose Electric deletion event hasn't arrived yet will still receive a ⌘N shortcut and be navigatable. DeletingWorkspacesProvider still tracks this state; the isDeleting check and its import were dropped from this file as part of the revert but can be restored with two lines.
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/useDashboardSidebarShortcuts/useDashboardSidebarShortcuts.ts
Line: 56-62
Comment:
**Missing `isDeleting` guard on keyboard shortcuts**
The previous filter excluded both non-synced workspaces and workspaces actively being deleted (`isDeleting`). The new filter only excludes workspaces with a `creationStatus`, so a workspace whose delete API call has been sent but whose Electric deletion event hasn't arrived yet will still receive a `⌘N` shortcut and be navigatable. `DeletingWorkspacesProvider` still tracks this state; the `isDeleting` check and its import were dropped from this file as part of the revert but can be restored with two lines.
How can I resolve this? If you propose a fix, please make it concise.
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
Reverts #4707 (
feat: optimistic workspace creation + Electric write-sync correctness). This brings back the pre-#4707 workspace creation flow, including the renderer-side workspace-creates store/manager, and removes the TanStack DB optimistic workspace insert + Electric txid-confirmed sync rewrite.Why
The TanStack DB / Electric SQL transaction work needs to be backed out while we revisit the approach. This PR is a straight revert of the merged squash commit
0a40cf2e.Impact
Validation
bun run lint:fixbun run lintSummary by cubic
Reverts the optimistic workspace creation and Electric write-sync path from #4707. Restores the previous renderer-driven create flow with in-flight tracking, removes txid-based sync, and simplifies server/router responses.
Refactors
useWorkspaceCreatesStoreandWorkspaceCreatesManager; replacefailedWorkspaceCreateslocal collection.creationStatus(preparing/generating/creating/failed); failed rows can be dismissed; shortcuts ignore in-flight items.isSyncedgating and deletion sync waits; simplify delete dialog.New Workspace, task/issue run, adopt worktrees) awaitsubmit()and toast success/failure; redirect to canonical id when it differs from the snapshot id.v2-workspace,v2-project,v2-host, andchatrouters; mutations return rows/flags only. SimplifygetCurrentTxidusage.Dependencies
@electric-sql/clientfromapps/electric-proxy; inline protocol params and drop Electric write-sync/error handling in collections.Written for commit 74f82cb. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Improvements