fix(desktop): prevent "workspace not found" flash after v2 workspace create#3494
Conversation
…create The pending page previously navigated to /v2-workspace/$id after 3s even if the workspace row hadn't synced into the local Electric collection. The destination route's live query resolved empty and flashed WorkspaceNotFoundState. Hard-gate navigation on sync completion, bump the stall timeout to 10s, and on stall show a recoverable error with Keep waiting / Open anyway / Dismiss instead of silently navigating into a broken page.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughNavigation from the pending workspace page is gated by a 10s SYNC_TIMEOUT_MS; navigation now uses a guarded Changes
Sequence Diagram(s)sequenceDiagram
participant PendingPage as Pending Page
participant Timer as Sync Timer (SYNC_TIMEOUT_MS)
participant Collections as Local Collections
participant Sidebar as Dashboard Sidebar
participant Router as Router
participant Cleanup as Pending Row Cleanup
PendingPage->>Collections: check for workspace row
alt workspace present
PendingPage->>PendingPage: set workspaceSynced = true
PendingPage->>PendingPage: call doNavigate()
else workspace not present
Timer->>PendingPage: SYNC_TIMEOUT_MS elapsed -> set syncTimedOut
PendingPage-->>PendingPage: show recoverable warning (wait / open anyway / dismiss)
alt user chooses "Open anyway"
PendingPage->>Collections: ensure workspace row exists
PendingPage->>Sidebar: ensure workspace in sidebar
PendingPage->>Collections: optionally write paneLayout to v2WorkspaceLocalState
PendingPage->>Router: navigate to /v2-workspace/$workspaceId
PendingPage->>Cleanup: schedule delete pending row (1s)
else user chooses "Keep waiting"
PendingPage->>Timer: clear syncTimedOut / continue waiting
else user chooses "Dismiss"
PendingPage->>Cleanup: delete pending row + clear attachments
PendingPage->>Router: navigate to /
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 fixes a race condition in v2 workspace creation where the pending page would navigate to Key changes:
Confidence Score: 4/5Safe to merge with one targeted fix: the "Keep waiting" timer restart bug should be addressed to prevent users being stranded. The core sync-gate logic is correct and directly addresses the race condition. The three-button stall UI is a clean improvement. Two issues exist in the error-recovery path: (1) clicking "Keep waiting" clears syncTimedOut but doesn't restart the 10 s timer (missing dep in the useEffect), leaving the user on a spinner with no action buttons if sync still doesn't arrive; (2) syncTimedOut is not reset when pendingId changes, potentially showing the stall UI prematurely on a new pending page. Issue (1) is a concrete P1 bug in the error path; issue (2) is an edge-case P2. The happy path (sync arrives within 10 s) is unaffected. apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx — sync timeout useEffect dependency array and pendingId-change guard
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx | Core sync-gate fix is correct; two issues in the "Keep waiting" path: the sync timer is never restarted after the flag is cleared (leaving the user stranded with no UI actions), and syncTimedOut is not reset when pendingId changes between navigations. |
Sequence Diagram
sequenceDiagram
participant Modal
participant PendingPage
participant HostService
participant Electric
Modal->>PendingPage: navigate(/pending/$id) with intent row
PendingPage->>HostService: fireIntent() (fork/checkout/adopt)
HostService-->>PendingPage: result { workspace.id, terminals, warnings }
PendingPage->>PendingPage: pending.status = "succeeded", workspaceId set
PendingPage->>Electric: live query collections.v2Workspaces WHERE id=workspaceId
alt Sync arrives within 10s (happy path)
Electric-->>PendingPage: workspaceSynced = true
PendingPage->>PendingPage: doNavigate() → /v2-workspace/$id
else Sync stalls past 10s
PendingPage->>PendingPage: syncTimedOut = true → show amber error UI
alt User clicks Keep waiting
PendingPage->>PendingPage: syncTimedOut = false (timer NOT restarted)
Electric-->>PendingPage: workspaceSynced = true (eventually)
PendingPage->>PendingPage: doNavigate()
else User clicks Open anyway
PendingPage->>PendingPage: doNavigate() (force, no sync check)
else User clicks Dismiss
PendingPage->>PendingPage: delete pending row → navigate(/)
end
end
Comments Outside Diff (1)
-
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx, line 136-141 (link)syncTimedOutnot reset whenpendingIdchangesThe guard block that resets the per-pendingId bookkeeping when the route param changes resets
firedRefandnavigatedRef, but it does not resetsyncTimedOut.If a user navigates from pending page A (where sync timed out →
syncTimedOut=true) to pending page B (where the workspace has already succeeded but sync hasn't arrived yet),syncTimedOutwill still betrueon mount. The render conditionsyncTimedOut && !workspaceSyncedwill immediately be satisfied, showing the amber stall UI on page B — before the 10 s timeout has even elapsed.if (prevPendingIdRef.current !== pendingId) { prevPendingIdRef.current = pendingId; firedRef.current = false; navigatedRef.current = false; setSyncTimedOut(false); // add this }
Calling a state setter during render is a documented React pattern for derived-from-params state ("store information from previous renders"). React will immediately discard the current render and restart with the new value.
Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx Line: 136-141 Comment: **`syncTimedOut` not reset when `pendingId` changes** The guard block that resets the per-pendingId bookkeeping when the route param changes resets `firedRef` and `navigatedRef`, but it does **not** reset `syncTimedOut`. If a user navigates from pending page A (where sync timed out → `syncTimedOut=true`) to pending page B (where the workspace has already succeeded but sync hasn't arrived yet), `syncTimedOut` will still be `true` on mount. The render condition `syncTimedOut && !workspaceSynced` will immediately be satisfied, showing the amber stall UI on page B — before the 10 s timeout has even elapsed. ```tsx if (prevPendingIdRef.current !== pendingId) { prevPendingIdRef.current = pendingId; firedRef.current = false; navigatedRef.current = false; setSyncTimedOut(false); // add this } ``` Calling a state setter during render is a documented React pattern for derived-from-params state ("store information from previous renders"). React will immediately discard the current render and restart with the new value. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Line: 375-380
Comment:
**"Keep waiting" leaves user with no escape UI**
When the user clicks **Keep waiting**, `setSyncTimedOut(false)` is called. This dismisses the amber error and renders the `"Workspace ready — opening..."` success branch, which has **no action buttons** (no Dismiss, no Keep waiting).
The sync-timeout `useEffect` won't restart because its dependency array `[pending?.status, pending?.workspaceId, workspaceSynced]` doesn't include `syncTimedOut` — so no new 10 s timer is scheduled after the reset. If sync never arrives, the user is stranded on a spinner with no way to dismiss or escape.
Fix: add `syncTimedOut` as a guard *and* dependency so the effect rearms itself when the flag is cleared:
```tsx
useEffect(() => {
if (
pending?.status !== "succeeded" ||
!pending.workspaceId ||
workspaceSynced ||
syncTimedOut || // don't double-arm while already showing the error
navigatedRef.current
) {
return;
}
const timer = setTimeout(() => setSyncTimedOut(true), SYNC_TIMEOUT_MS);
return () => clearTimeout(timer);
}, [pending?.status, pending?.workspaceId, workspaceSynced, syncTimedOut]);
```
With this change, clicking **Keep waiting** (`setSyncTimedOut(false)`) re-triggers the effect, arms a fresh 10 s timer, and the amber error will reappear if sync still hasn't landed — giving the user another opportunity to act.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
Line: 136-141
Comment:
**`syncTimedOut` not reset when `pendingId` changes**
The guard block that resets the per-pendingId bookkeeping when the route param changes resets `firedRef` and `navigatedRef`, but it does **not** reset `syncTimedOut`.
If a user navigates from pending page A (where sync timed out → `syncTimedOut=true`) to pending page B (where the workspace has already succeeded but sync hasn't arrived yet), `syncTimedOut` will still be `true` on mount. The render condition `syncTimedOut && !workspaceSynced` will immediately be satisfied, showing the amber stall UI on page B — before the 10 s timeout has even elapsed.
```tsx
if (prevPendingIdRef.current !== pendingId) {
prevPendingIdRef.current = pendingId;
firedRef.current = false;
navigatedRef.current = false;
setSyncTimedOut(false); // add this
}
```
Calling a state setter during render is a documented React pattern for derived-from-params state ("store information from previous renders"). React will immediately discard the current render and restart with the new value.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): prevent "workspace not fou..." | Re-trigger Greptile
The layout had its own bare unstyled "Workspace not found" text that fired before the page-level component could render, so users saw a raw centered string instead of the styled empty state. Promote WorkspaceNotFoundState to the shared v2-workspace/components/ folder and use it in both the layout and the page.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/pending/$pendingId/page.tsx (1)
225-237:⚠️ Potential issue | 🟠 MajorRe-arm the sync timeout after "Keep waiting".
The timeout effect (line 225) does not include
syncTimedOutin its dependency array. When "Keep waiting" (line 377) setssyncTimedOutto false, the effect does not re-run because none of its listed dependencies (pending?.status,pending?.workspaceId,workspaceSynced) have changed. This means no new timeout is started, so "Keep waiting" only hides the error UI without actually resetting the timer. Users pressing it will not get a fresh 10-second window for sync to complete.Proposed fix
const [syncTimedOut, setSyncTimedOut] = useState(false); +useEffect(() => { + setSyncTimedOut(false); +}, [pendingId]); useEffect(() => { if ( pending?.status !== "succeeded" || !pending.workspaceId || workspaceSynced || - navigatedRef.current + navigatedRef.current || + syncTimedOut ) { return; } const timer = setTimeout(() => setSyncTimedOut(true), SYNC_TIMEOUT_MS); return () => clearTimeout(timer); -}, [pending?.status, pending?.workspaceId, workspaceSynced]); +}, [ + pending?.status, + pending?.workspaceId, + workspaceSynced, + syncTimedOut, + pendingId, +]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx around lines 225 - 237, The useEffect that starts the sync timeout (uses SYNC_TIMEOUT_MS and setSyncTimedOut) doesn't include the syncTimedOut state in its dependency array so clicking "Keep waiting" (which sets syncTimedOut = false) won't re-arm the timer; update the dependency array of that effect to include syncTimedOut (alongside pending?.status, pending?.workspaceId, workspaceSynced) so the effect re-runs when syncTimedOut is reset, keeping the same timer setup and cleanup (clearTimeout) logic in the effect body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 225-237: The useEffect that starts the sync timeout (uses
SYNC_TIMEOUT_MS and setSyncTimedOut) doesn't include the syncTimedOut state in
its dependency array so clicking "Keep waiting" (which sets syncTimedOut =
false) won't re-arm the timer; update the dependency array of that effect to
include syncTimedOut (alongside pending?.status, pending?.workspaceId,
workspaceSynced) so the effect re-runs when syncTimedOut is reset, keeping the
same timer setup and cleanup (clearTimeout) logic in the effect body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db62a3de-3c41-4739-b0d3-b57893e15c26
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx
![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 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx:377">
P2: `Keep waiting` clears `syncTimedOut` but does not restart the sync timeout, so the stall warning can be permanently dismissed while sync is still stalled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ingId change Clicking "Keep waiting" cleared syncTimedOut but the effect's dep array didn't include it, so no new timer was scheduled. User would land on the "Workspace ready — opening..." branch (no buttons) with no way out if sync still didn't arrive. Add syncTimedOut as a dep and an early-return guard so the effect re-arms cleanly. Also reset syncTimedOut when pendingId changes so a new pending page doesn't inherit the prior one's stall UI.
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
1 participant
Summary
/v2-workspace/$idafter 3s even when the workspace row hadn't synced into the local Electric collection, causingWorkspaceNotFoundStateto flash on the destination route.collections.v2Workspaces(removes the silent|| syncTimedOutescape that was jumping into a broken page).Test plan
Summary by cubic
Prevents the "Workspace not found" flash after creating a v2 workspace by waiting for local sync before navigating. Unifies the not‑found UI and adds a recoverable stall state with a 10s timeout.
collections.v2Workspaces(no more silent timeout escape).WorkspaceNotFoundStateto sharedv2-workspace/componentsand use it in both the layout and page to avoid the unstyled flash.Written for commit 3278690. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Bug Fixes