Fix Electric create sync txids#4632
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemove renderer-side in-flight cloud-row caching/fallback and creation-status UI; backend create mutations now run in transactions, compute numeric txid, and return txid with created records. Collections typing updated; client creation flow writes local state and awaits txid when provided. ChangesWorkspace creation refactor: cloud-row to txid
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (useWorkspaceCreates / UI)
participant Collections as CollectionsProvider (v2Workspaces.onInsert)
participant Host as HostService (workspaces.create)
participant TRPC as trpc.v2Workspace.create
participant DB as dbWs.transaction
participant TxUtil as getCurrentTxid
Renderer->>Collections: insert v2Workspace with WorkspaceCreateInsertMetadata
Collections->>Host: call host-service workspaces.create with input
Host-->>Collections: returns canonical workspace (id[, txid])
Collections-->>Collections: write host result into insert metadata
Collections->>Renderer: electric insert persists (metadata may include txid)
Renderer->>TRPC: call create mutation
TRPC->>DB: perform insert/update inside transaction
DB->>TxUtil: compute current txid
TxUtil-->>TRPC: txid
TRPC-->>Renderer: return { ...workspace, txid }
Renderer->>Collections: await collection utils for txid (when needed)
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 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 12 individual chapters for you: 
Chapters generated by Stage for commit 8ff46e7 on May 17, 2026 12:15am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/stores/workspace-creates/useWorkspaceCreates.ts`:
- Around line 131-133: The code is prematurely removing the snapshot-keyed
optimistic entry in the alreadyExists && workspace.id !== workspaceId branch
which causes V2WorkspaceLayout to briefly render WorkspaceNotFoundState;
instead, change the handling inside the result.alreadyExists && workspace.id !==
workspaceId branch to either defer removal until after the caller performs the
redirect or retarget the optimistic entry to the canonical id before removing
the snapshot key (i.e., update the store entry for the snapshot key to point to
workspace.id or call a retarget/rename helper on useWorkspaceCreatesStore rather
than immediately calling
useWorkspaceCreatesStore.getState().remove(workspaceId)), ensuring the entry
remains available during the redirect so the inFlight/syncedWorkspace state
doesn’t vanish.
🪄 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: 952fdc87-ffb9-4499-bdcf-bbac0968633a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceTitle/V2WorkspaceTitle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/stores/workspace-creates/store.tsapps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.tsapps/electric-proxy/package.jsonapps/electric-proxy/src/electric.tspackages/host-service/src/trpc/router/workspace-creation/shared/adopt-existing-worktree.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/trpc/src/router/chat/chat.tspackages/trpc/src/router/v2-project/v2-project.test.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/stores/workspace-creates/store.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 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="packages/trpc/src/router/v2-project/v2-project.test.ts">
<violation number="1" location="packages/trpc/src/router/v2-project/v2-project.test.ts:74">
P2: Transaction insert is mocked through the same return queue as the normal insert, so the create tests cannot detect if the implementation uses the wrong insert path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Greptile SummaryThis PR plumbs same-transaction
Confidence Score: 3/5The workspace creation flow now blocks until Electric confirms the row, removing the cloudRow safety net — if Electric drops after the server write, the user is stuck with no escape path. The
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts | Replaces cloudRow fallback with awaitTxId — blocks dispatch indefinitely if Electric is unreachable after a successful server mutation. |
| packages/trpc/src/router/v2-project/v2-project.test.ts | Adds tx-level insert mocks; getCurrentTxidMock returns a string but the real implementation returns a number, making txid type assertions misleading. |
| packages/trpc/src/router/v2-workspace/v2-workspace.ts | Wraps workspace create/update in a transaction and returns same-transaction txid; covers all branches correctly. |
| packages/trpc/src/router/v2-project/v2-project.ts | Moves project insert into a transaction to capture txid; returns { ...project, txid } — additive, non-breaking. |
| packages/trpc/src/router/chat/chat.ts | Wraps chat session insert in a transaction; correctly returns txid: null on conflict. |
| apps/electric-proxy/src/electric.ts | Replaces hand-maintained PROTOCOL_PARAMS set with library-provided ELECTRIC_PROTOCOL_QUERY_PARAMS constant. |
Comments Outside Diff (1)
-
packages/trpc/src/router/v2-project/v2-project.test.ts, line 280-281 (link)getCurrentTxidMockreturns a string"txid-123", butgetCurrentTxidhas return typePromise<number>(it callsNumber.parseIntinternally). The test assertions that checktxid: "txid-123"therefore verify a string value that never appears in production — in production the txid will be an integer. The client guardtypeof workspace.txid === "number"inuseWorkspaceCreates.tsevaluates tofalseagainst the mock value, so tests never exercise the actualawaitTxIdbranch. Using a numeric mock value keeps the mock type-accurate and makes the guard testable.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: 280-281 Comment: `getCurrentTxidMock` returns a string `"txid-123"`, but `getCurrentTxid` has return type `Promise<number>` (it calls `Number.parseInt` internally). The test assertions that check `txid: "txid-123"` therefore verify a string value that never appears in production — in production the txid will be an integer. The client guard `typeof workspace.txid === "number"` in `useWorkspaceCreates.ts` evaluates to `false` against the mock value, so tests never exercise the actual `awaitTxId` branch. Using a numeric mock value keeps the mock type-accurate and makes the guard testable. 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:79-81
**`awaitTxId` has no timeout or cancellation path**
`awaitTxId` blocks the entire `dispatch` call until Electric delivers the workspace row. If the Electric connection is lost or degraded after the server mutation succeeds, this promise can hang indefinitely, leaving the user stuck on the "creating" spinner with no escape. The `cloudRow` fallback that was removed specifically protected against this scenario — the server had confirmed the workspace, so the UI could proceed regardless of Electric's liveness. Consider wrapping `awaitTxId` with a `Promise.race` against a reasonable timeout so a slow-Electric situation degrades gracefully rather than silently stalling.
### Issue 2 of 2
packages/trpc/src/router/v2-project/v2-project.test.ts:280-281
`getCurrentTxidMock` returns a string `"txid-123"`, but `getCurrentTxid` has return type `Promise<number>` (it calls `Number.parseInt` internally). The test assertions that check `txid: "txid-123"` therefore verify a string value that never appears in production — in production the txid will be an integer. The client guard `typeof workspace.txid === "number"` in `useWorkspaceCreates.ts` evaluates to `false` against the mock value, so tests never exercise the actual `awaitTxId` branch. Using a numeric mock value keeps the mock type-accurate and makes the guard testable.
```suggestion
getCurrentTxidMock.mockReset();
getCurrentTxidMock.mockImplementation(async () => 123);
```
Reviews (1): Last reviewed commit: "fix electric create sync txids" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (typeof workspace.txid === "number") { | ||
| await collections.v2Workspaces.utils.awaitTxId(workspace.txid); | ||
| } |
There was a problem hiding this comment.
awaitTxId has no timeout or cancellation path
awaitTxId blocks the entire dispatch call until Electric delivers the workspace row. If the Electric connection is lost or degraded after the server mutation succeeds, this promise can hang indefinitely, leaving the user stuck on the "creating" spinner with no escape. The cloudRow fallback that was removed specifically protected against this scenario — the server had confirmed the workspace, so the UI could proceed regardless of Electric's liveness. Consider wrapping awaitTxId with a Promise.race against a reasonable timeout so a slow-Electric situation degrades gracefully rather than silently stalling.
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: 79-81
Comment:
**`awaitTxId` has no timeout or cancellation path**
`awaitTxId` blocks the entire `dispatch` call until Electric delivers the workspace row. If the Electric connection is lost or degraded after the server mutation succeeds, this promise can hang indefinitely, leaving the user stuck on the "creating" spinner with no escape. The `cloudRow` fallback that was removed specifically protected against this scenario — the server had confirmed the workspace, so the UI could proceed regardless of Electric's liveness. Consider wrapping `awaitTxId` with a `Promise.race` against a reasonable timeout so a slow-Electric situation degrades gracefully rather than silently stalling.
How can I resolve this? If you propose a fix, please make it concise.
KyleAMathews
force-pushed
the
fix/electric-create-sync-txids
branch
from
c7d9339 to
ea7e08e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/trpc/src/router/v2-project/v2-project.test.ts (1)
749-749: 💤 Low valueConsider also asserting
transactionMock.not.toHaveBeenCalled().Since create mutations now run inside transactions, checking
dbInsert.not.toHaveBeenCalled()confirms direct inserts didn't occur but doesn't verify that no transaction was started before the validation failure. Addingexpect(transactionMock).not.toHaveBeenCalled()would strengthen the assertion that validation happens before any DB work begins.Same applies to the similar assertion at line 762.
🤖 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 `@packages/trpc/src/router/v2-project/v2-project.test.ts` at line 749, Add assertions to the failing-create tests to ensure no transaction was started by asserting transactionMock.not.toHaveBeenCalled() in addition to the existing expect(dbInsert).not.toHaveBeenCalled(); specifically, update the test cases that currently assert dbInsert wasn't called (referenced as dbInsert) to also assert transactionMock wasn't called (referenced as transactionMock) in both places where this pattern appears so the tests confirm validation fails before any transaction or DB work begins.
🤖 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 `@packages/trpc/src/router/v2-project/v2-project.test.ts`:
- Line 749: Add assertions to the failing-create tests to ensure no transaction
was started by asserting transactionMock.not.toHaveBeenCalled() in addition to
the existing expect(dbInsert).not.toHaveBeenCalled(); specifically, update the
test cases that currently assert dbInsert wasn't called (referenced as dbInsert)
to also assert transactionMock wasn't called (referenced as transactionMock) in
both places where this pattern appears so the tests confirm validation fails
before any transaction or DB work begins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a23750f-0eeb-4dc6-926c-8a64f752c2f1
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceTitle/V2WorkspaceTitle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/stores/workspace-creates/store.tsapps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.tsapps/electric-proxy/package.jsonapps/electric-proxy/src/electric.tspackages/host-service/src/trpc/router/workspace-creation/shared/adopt-existing-worktree.tspackages/host-service/src/trpc/router/workspaces/workspaces.tspackages/trpc/src/router/chat/chat.tspackages/trpc/src/router/v2-project/v2-project.test.tspackages/trpc/src/router/v2-project/v2-project.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/stores/workspace-creates/store.ts
✅ Files skipped from review due to trivial changes (2)
- packages/host-service/src/trpc/router/workspace-creation/shared/adopt-existing-worktree.ts
- apps/electric-proxy/package.json
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/electric-proxy/src/electric.ts
- packages/host-service/src/trpc/router/workspaces/workspaces.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
- packages/trpc/src/router/chat/chat.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceTitle/V2WorkspaceTitle.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
- apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts
- packages/trpc/src/router/v2-workspace/v2-workspace.ts
- packages/trpc/src/router/v2-project/v2-project.ts
KyleAMathews
force-pushed
the
fix/electric-create-sync-txids
branch
from
ea7e08e to
4ee5c9c
Compare
The create tests now assert txid: 123 in the return value, matching the update/icon tests. Also updates OPTIMISTIC_ELECTRIC_UPDATES.md to reflect that v2Workspaces now has an onInsert handler for workspace creation.
1 participant
Summary
Hardens workspace/project/chat writes against Electric sync races by returning correct txids from backend mutations, awaiting them in renderer collection handlers, and removing the parallel in-memory workspace-create sidecar that modeled in-flight cloud rows outside TanStack DB.
User-visible impact: Workspace create, rename, delete, import/adopt, and secondary navigation surfaces no longer race ahead of Electric confirmation, and optimistic UI is gated on sync/delete state.
Approach
Electric txid correctness
getCurrentTxid()now returnspg_current_xact_id()::xid::text, parsed and validated as a safe integer.ELECTRIC_WRITE_SYNC_TIMEOUT_MS = 30_000constant for all write confirmation waits.v2Workspaces.onUpdatenow uses the same txid matching helper (previously returned{ txid: result.txid }directly)..returning()or equivalent affected-row checks before callinggetCurrentTxid(), so a raced no-op DML cannot fabricate a txid Electric will never emit.Note on xid casting: We discovered
getCurrentTxid()was usingpg_current_xact_id()::textwithout the::xid::cast. Electric receives 32-bitxidvalues through logical replication, so the correct form ispg_current_xact_id()::xid::text. Without the cast, the values happen to match on fresh databases (epoch = 0), but would diverge after ~4.29B txids when the epoch increments. Whether this contributed to reported timeouts is unconfirmed (depends on transaction volume), but it's a correctness fix regardless.Workspace create/import flow
v2Workspace.create,v2Project.create, andchat.createSessionwrap writes in transactions and return txids.useWorkspaceCreates.submit()flow:v2Workspaces.insert()tx.isPersisted.promise$synced === trueNavigation and UI guards
OpenInWorkspaceV2no longer navigate optimistically; they awaitsubmit()and open the confirmed id.V2WorkspaceLayoutrefuses to mount host-backed workspace UI for unsynced optimistic rows ($syncedcheck)./v2-workspaceslist filter out unsynced rows and rows being deleted, preventing stale actions and hotkey targets.Delete/update coverage
v2Workspace.deleteanddeleteMainForHostnow return txids for real deletes and return no txid for already-gone raced deletes.setTask,updateNameFromHost) return txids for real writes.Removed workspace-create sidecar
Removed the renderer-memory workspace-create model and related fallback UI:
WorkspaceCreatingState/,WorkspaceCreateErrorState/,workspace-creates/Manager.tsx,workspace-creates/store.tscreationStatusplumbing, pending/failed in-flight row injectionV2WorkspaceTitlein-flight fallbackThe remaining create path uses TanStack DB's optimistic entry plus Electric confirmation only.
Key Invariants
getCurrentTxid()must be called inside the same SQL transaction as a real replicated write.xidform:pg_current_xact_id()::xid::text.$synced === true./v2-workspaces, command palette, resource monitor, and port actions only see synced rows not currently being deleted.{ txid: null }or{ txid: undefined }to TanStack's Electric adapter.Non-goals
packages/trpc.workspace.deletepath is not converted into a renderer-observed Electric wait. The desktop delete dialog path now usesworkspaceCleanup.destroywith Electric confirmation.Verification
Latest reviewer-driven follow-up validation also passed:
Full
bun --cwd packages/trpc typecheckhas unrelated pre-existing crypto/Buffer type errors.