fix(desktop): gate v2 workspace children on collection readiness#3464
fix(desktop): gate v2 workspace children on collection readiness#3464saddlepaddle merged 1 commit intomainfrom
Conversation
The v2-workspace layout rendered child routes without WorkspaceTrpcProvider during the tick between workspaceId changing and the useLiveQuery join resolving. If the outgoing page hadn't finished unmounting, its hooks (TerminalPane, useGitChangeEvents, etc.) crashed with "useWorkspaceClient must be used within WorkspaceClientProvider". Hold the render until useLiveQuery reports isReady so the new workspace mounts into a valid provider on the same tick, and show an explicit "Workspace not found" state when the collection has hydrated but the id doesn't resolve.
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 the Changes:
Confidence Score: 4/5Safe to merge — the crash is eliminated and the core logic is correct; remaining issues are P2 UX polish. The fix correctly uses No files require special attention beyond the single changed layout file. Important Files Changed
Sequence DiagramsequenceDiagram
participant Router as TanStack Router
participant Layout as V2WorkspaceLayout
participant LiveQuery as useLiveQuery
participant Provider as WorkspaceTrpcProvider
participant Child as Child Routes
Router->>Layout: workspaceId changes (navigation)
Layout->>LiveQuery: query(collections, workspaceId)
Note over LiveQuery: isReady = false
LiveQuery-->>Layout: data=[], isReady=false
Layout-->>Router: return null (no render, no crash)
LiveQuery-->>Layout: data=[workspace], isReady=true
alt workspace and hostUrl resolved
Layout->>Provider: mount WorkspaceTrpcProvider
Provider->>Child: render Outlet safely
else workspace missing or hostUrl null
Layout-->>Router: show Workspace not found
end
Reviews (1): Last reviewed commit: "fix(desktop): gate v2 workspace children..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!workspace || !hostUrl) { | ||
| return ( | ||
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | ||
| Workspace host service not available | ||
| Workspace not found | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
The two sub-cases covered by !workspace || !hostUrl have meaningfully different root causes:
!workspace— the id doesn't resolve in the collection → "Workspace not found" is correct.!hostUrlwhileworkspace != null— this only happens for a local workspace whenactiveHostUrlisnull(i.e., the host service hasn't started or has lost its connection). In that scenario the workspace does exist, but showing "Workspace not found" is misleading.
Before this PR the two cases had distinct messages ("Workspace host service not available" vs. silent <Outlet />). Splitting them again makes the error actionable:
| if (!workspace || !hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| Workspace not found | |
| </div> | |
| ); | |
| if (!workspace) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace not found | |
| </div> | |
| ); | |
| } | |
| if (!hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| </div> | |
| ); | |
| } |
| if (!workspaceId || !isReady) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Returning null while !isReady eliminates the crash, but it also produces a blank screen during the initial load and every workspace transition. Depending on how long isReady takes to flip, users may see a momentary flash of nothing. A lightweight loading state (spinner, skeleton, or even a static placeholder) would avoid that:
| if (!workspaceId || !isReady) { | |
| return null; | |
| } | |
| if (!workspaceId || !isReady) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Loading workspace… | |
| </div> | |
| ); | |
| } |
This is a non-blocking suggestion; the crash fix is the important part.
![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 the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx`:
- Around line 71-75: The current conditional lumps both missing workspace and
missing host into the same message; update the JSX in the layout component so
that the check for workspace and hostUrl are separate: when !workspace return
the existing "Workspace not found" fallback, but when workspace exists and
!hostUrl return a distinct fallback (e.g., "Host unavailable" or "Waiting for
host") so a valid workspace isn't mistaken for a bad ID; adjust the return logic
in the component that references the workspace and hostUrl variables
accordingly.
🪄 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: 49dbf33b-ec89-4d0c-8728-901e7dbc2d4a
📒 Files selected for processing (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx
| if (!workspace || !hostUrl) { | ||
| return ( | ||
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | ||
| Workspace host service not available | ||
| Workspace not found | ||
| </div> |
There was a problem hiding this comment.
Keep the missing-workspace and missing-host states separate.
Line 71 now maps both !workspace and !hostUrl to "Workspace not found". That makes a valid workspace with an unavailable or still-bootstrapping local host look like a bad ID. Please keep the new not-found state for !workspace, but preserve a distinct fallback for !hostUrl.
🔧 Suggested split
-if (!workspace || !hostUrl) {
+if (!workspace) {
return (
<div className="flex h-full w-full items-center justify-center text-muted-foreground">
Workspace not found
</div>
);
}
+
+if (!hostUrl) {
+ return (
+ <div className="flex h-full w-full items-center justify-center text-muted-foreground">
+ Workspace host service not available
+ </div>
+ );
+}📝 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.
| if (!workspace || !hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| Workspace not found | |
| </div> | |
| if (!workspace) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace not found | |
| </div> | |
| ); | |
| } | |
| if (!hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| </div> | |
| ); | |
| } |
🤖 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/v2-workspace/layout.tsx`
around lines 71 - 75, The current conditional lumps both missing workspace and
missing host into the same message; update the JSX in the layout component so
that the check for workspace and hostUrl are separate: when !workspace return
the existing "Workspace not found" fallback, but when workspace exists and
!hostUrl return a distinct fallback (e.g., "Host unavailable" or "Waiting for
host") so a valid workspace isn't mistaken for a bad ID; adjust the return logic
in the component that references the workspace and hostUrl variables
accordingly.
![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/v2-workspace/layout.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx:71">
P2: This condition conflates two distinct error states: `!workspace` (ID doesn't resolve) and `!hostUrl` (host service unavailable while workspace exists). Showing "Workspace not found" when the host service is down is misleading — the workspace does exist. Split into separate checks to preserve the actionable "Workspace host service not available" message that existed before this change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (!workspace || !hostUrl) { | ||
| return ( | ||
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | ||
| Workspace host service not available | ||
| Workspace not found | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
P2: This condition conflates two distinct error states: !workspace (ID doesn't resolve) and !hostUrl (host service unavailable while workspace exists). Showing "Workspace not found" when the host service is down is misleading — the workspace does exist. Split into separate checks to preserve the actionable "Workspace host service not available" message that existed before this change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx, line 71:
<comment>This condition conflates two distinct error states: `!workspace` (ID doesn't resolve) and `!hostUrl` (host service unavailable while workspace exists). Showing "Workspace not found" when the host service is down is misleading — the workspace does exist. Split into separate checks to preserve the actionable "Workspace host service not available" message that existed before this change.</comment>
<file context>
@@ -64,22 +64,14 @@ function V2WorkspaceLayout() {
}
- if (!hostUrl) {
+ if (!workspace || !hostUrl) {
return (
<div className="flex h-full w-full items-center justify-center text-muted-foreground">
</file context>
| if (!workspace || !hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| Workspace not found | |
| </div> | |
| ); | |
| if (!workspace) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace not found | |
| </div> | |
| ); | |
| } | |
| if (!hostUrl) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center text-muted-foreground"> | |
| Workspace host service not available | |
| </div> | |
| ); | |
| } |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
…erset-sh#3464) The v2-workspace layout rendered child routes without WorkspaceTrpcProvider during the tick between workspaceId changing and the useLiveQuery join resolving. If the outgoing page hadn't finished unmounting, its hooks (TerminalPane, useGitChangeEvents, etc.) crashed with "useWorkspaceClient must be used within WorkspaceClientProvider". Hold the render until useLiveQuery reports isReady so the new workspace mounts into a valid provider on the same tick, and show an explicit "Workspace not found" state when the collection has hydrated but the id doesn't resolve.
…ndle chore(upstream): PR1 低リスク修正5件バンドル (superset-sh#3457 superset-sh#3464 superset-sh#3462 superset-sh#3466 superset-sh#3461)
-s ours merge to record that upstream commits a3e34bf through de70163 (13 commits) are semantically already present on origin/main via the PR1-6 cherry-pick series (PRs #176, #177, #178, #179, #180, #182), plus fork-adaptation fixes layered on top. This merge target is de70163 specifically (not upstream/main) so newer upstream commits (9fff075 and later) remain visible in future behind counts. Upstream commits covered by this audit merge: - a3e34bf fix(desktop): restore cmd+click requirement for v1 terminal file links (superset-sh#3457) [PR1/#176] - 57557f8 fix(desktop): gate v2 workspace children on collection readiness (superset-sh#3464) [PR1/#176] - 4ee2e61 fix(desktop): use native clipboard for copy path in v2 sidebar (superset-sh#3462) [PR1/#176] - 87d6e93 feat(desktop): close settings with Escape key (superset-sh#3466) [PR1/#176] - 9c7f5f4 chore(desktop): auto-restart host-service on bundle change in dev (superset-sh#3461) [PR1/#176] - 93140d9 fix(mcp): accept MCP resource URL as valid OAuth audience (superset-sh#3459) [PR2/#177] - be9e000 fix(desktop): drive tray menu off events, fetch real org name (superset-sh#3458) [PR2/#177] - c5f791e feat(v2): unify workspace delete through host-service (superset-sh#3443) [PR3/#178] - 2c24d93 feat(desktop): paginated branch picker with checkout + open actions (superset-sh#3397) [PR4/#179] - 2bf1049 feat(desktop/hotkeys): v1 directional pane focus + best-effort v1 override migrator (superset-sh#3460) [PR5/#180] - 1294a7d feat(desktop/hotkeys): restore Cmd+Alt+Arrow for tab/workspace nav (superset-sh#3472) [PR5/#180] - de70163 feat(desktop): v2 review tab first pass — PR info, checks, comments (superset-sh#3463) [PR6/#182] Intentionally skipped (version bump, fork has independent versioning): - 1e23353 chore(desktop): bump version to 1.5.5 (superset-sh#3473) Fork-adaptation fixes layered on top of the cherry-picks: - PR1: host-service-coordinator alias import fix, settings Escape selector narrowing (role-based + popper wrapper), Escape close uses replace navigation - PR2: dual quit mode preservation (requestQuit "release"/"stop"), trayUpdateToken guard for stale async fetchHostInfo results - PR4: ChangesHeader.normalizeBranchName regex rewrite (lint false positive), worktree add uses fullRef for remote-tracking refs, syncTimedOut reset on pendingId change, GIT_REFS.md barrel example fix - PR5: migrate.ts re-sanitize of existing localStorage overrides (v2 marker bump intent), FOCUS_PANE_* enabled:isActive for KeepAliveWorkspaces, CATEGORY_ORDER merges Navigation (upstream) and Browser (fork) - PR6: normalizeThreadsToComments flattens all thread.comments (not just first), CommentPane overrides <a> (openUrl) and <img> (SafeImage), zero-badge suppression, pr-null comments gate Fork features verified intact (Explore agent audit of combined 36d4de4..35d95f3 range): - BROWSER_RELOAD / BROWSER_HARD_RELOAD hotkeys - dual quit mode menu in tray - v1 terminal cold-restore + retry reconnect (out of range but unaffected) - KeepAliveWorkspaces (FOCUS_PANE_* gated on isActive) - useCommandPalette + addMemoTab in v2 workspace - host-service-coordinator rename alias pattern
1 participant
Summary
useWorkspaceClient must be used within WorkspaceClientProvider. Thev2-workspacelayout was rendering<Outlet />withoutWorkspaceTrpcProviderduring the tick betweenworkspaceIdchanging and theuseLiveQueryjoin resolving — the outgoing page's hooks (TerminalPane,useGitChangeEvents, etc.) would calluseWorkspaceClient()against an empty context.useLiveQueryreportsisReady, so the new workspace always mounts into a valid provider on the same tick.<Outlet />.Test plan
/v2-workspace/<bogus-id>and confirm the "Workspace not found" message showsSummary by cubic
Fixes a race that crashed v2 workspace pages by delaying child rendering until the workspace collection is ready. Child routes now mount only when data is ready and a workspace exists, preventing provider-less renders.
useLiveQuery.isReadyand a validworkspaceIdto avoid the "useWorkspaceClient must be used within WorkspaceClientProvider" crash.nullduring loading to prevent<Outlet />withoutWorkspaceTrpcProvider.Written for commit 37e6eff. Summary will update on new commits.
Summary by CodeRabbit