fix(desktop): derive v2 workspace locality from workspace.hostId, not joined v2Hosts#3837
Merged
Merged
Conversation
… joined v2Hosts Locality decisions previously leftJoined v2Workspaces -> v2Hosts and compared the joined hostMachineId to the local machineId. Electric was syncing v2Hosts spottily, so the joined value was often null and the code routed local workspaces through the cloud relay (most visibly: v2-workspace/layout.tsx sending the entire WorkspaceTrpcProvider at the relay URL while waiting on Electric). v2Workspaces.hostId is notNull and itself FKs to v2Hosts.machineId, so the workspace already carries the same value the join was producing. Switch locality to workspace.hostId === machineId everywhere; routing-only paths drop the v2Hosts join entirely. Metadata-display paths keep an innerJoin to v2Hosts (for host name + isOnline) — when v2Hosts lags, the row briefly omits from the listing but stays routable via direct URL. Also: synthesize a local target in DashboardSidebarPorts and DevicePicker so local ports / "this device" never disappear while Electric catches up.
Contributor
📝 WalkthroughWalkthroughThis PR systematically removes the derived Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Greptile SummaryThis PR fixes a race condition where v2 workspaces loaded while Electric was still syncing Confidence Score: 5/5Safe to merge — the fix is logically sound, consistently applied across all affected sites, and all remaining findings are P2 or lower. The root cause is well-understood and the fix correctly exploits the FK invariant (v2Workspaces.hostId IS v2Hosts.machineId). All 6 routing sites are updated consistently, the test fixtures are corrected to reflect the real schema invariant, and the synthesis fallbacks for ports and device picker are well-guarded. No P0/P1 issues were found. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.ts | Core routing fix: drops leftJoin with v2Hosts, compares workspace.hostId directly to machineId — eliminates race condition that sent local workspaces through relay. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsx | Primary layout fix: drops leftJoin, uses workspace.hostId === machineId for isLocal — WorkspaceTrpcProvider now points at local host immediately without waiting for Electric to sync v2Hosts. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.ts | Removes hostMachineId from DashboardSidebarWorkspaceRow interface; adds synthesis of local port-query target when v2Hosts row hasn't synced; eliminates spurious "cloud" hostType. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts | leftJoin → innerJoin for v2Hosts (intentional: rows briefly hidden during sync); hostMachineId removed; hostIsOnline now non-nullable from innerJoin (compatible with boolean |
| apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx | Removes leftJoin and hostMachineId; getHostUrlForWorkspace uses hostId === machineId instead of the old hostMachineId guard — local notifications route correctly before Electric syncs. |
| apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions/useWorkspaceHostOptions.ts | Synthesizes "This device" fallback when v2Hosts row hasn't synced, ensuring the picker always shows the local entry. |
| apps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.ts | Drops leftJoin; terminal URL mapping now uses workspace.hostId directly, same fix pattern as other routing sites. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts | Removes hostMachineId from AccessibleV2Workspace interface; hostId now sourced from workspaces.hostId (same FK value as hosts.machineId); "cloud" hostType eliminated. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.ts | Test fixtures updated to remove hostMachineId; machineId corrected to match hostId (reflecting the real FK invariant), fixing an artificial split in the old test setup. |
Sequence Diagram
sequenceDiagram
participant App
participant LiveQuery
participant v2Workspaces
participant v2Hosts
participant LocalHostService
Note over App,v2Hosts: Before fix (race condition)
App->>LiveQuery: leftJoin(v2Workspaces, v2Hosts)
v2Workspaces-->>LiveQuery: workspace { hostId: "abc" }
v2Hosts-->>LiveQuery: null (Electric not synced yet)
LiveQuery-->>App: hostMachineId = null
App->>App: null === machineId? → false → RELAY URL ❌
Note over App,v2Hosts: After fix
App->>LiveQuery: from(v2Workspaces) only
v2Workspaces-->>LiveQuery: workspace { hostId: "abc" }
LocalHostService-->>App: machineId = "abc"
App->>App: hostId === machineId? → true → LOCAL URL ✅
Note over App,v2Hosts: Sidebar / list (innerJoin — display only)
App->>LiveQuery: innerJoin(v2Workspaces, v2Hosts)
v2Hosts-->>LiveQuery: null (not synced) → row hidden briefly
Note right of App: Workspace still routable via direct URL
Reviews (1): Last reviewed commit: "fix(desktop): derive v2 workspace locali..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts (1)
152-156: Narrow this hook’s host-type contract.After this change,
hostTypeis binary here, butV2WorkspaceHostTypeandV2WorkspaceDeviceCountsstill expose a"cloud"state that this hook can no longer produce. That leaves an impossible branch and a permanently-zerocounts.cloud. Consider dropping"cloud"from this hook-local contract, or documenting why callers still need it.🤖 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-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts` around lines 152 - 156, The hook useAccessibleV2Workspaces now only produces two host values but still uses V2WorkspaceHostType and V2WorkspaceDeviceCounts which include a "cloud" branch; update the hook to use a narrowed local type and counts shape (or map/omit "cloud") so the impossible "cloud" branch and zero counts.cloud are removed. Specifically, replace/alias the hook-local hostType with a two-value union (e.g., "local-device" | "remote-device") or create a V2WorkspaceHostTypeLocalRemote used only by useAccessibleV2Workspaces, and adjust any counts construction (V2WorkspaceDeviceCounts usage) to exclude or map the "cloud" key so callers of useAccessibleV2Workspaces no longer see an unreachable "cloud" branch (ensure symbols to change include useAccessibleV2Workspaces, hostType, and V2WorkspaceDeviceCounts/V2WorkspaceHostType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts`:
- Around line 152-156: The hook useAccessibleV2Workspaces now only produces two
host values but still uses V2WorkspaceHostType and V2WorkspaceDeviceCounts which
include a "cloud" branch; update the hook to use a narrowed local type and
counts shape (or map/omit "cloud") so the impossible "cloud" branch and zero
counts.cloud are removed. Specifically, replace/alias the hook-local hostType
with a two-value union (e.g., "local-device" | "remote-device") or create a
V2WorkspaceHostTypeLocalRemote used only by useAccessibleV2Workspaces, and
adjust any counts construction (V2WorkspaceDeviceCounts usage) to exclude or map
the "cloud" key so callers of useAccessibleV2Workspaces no longer see an
unreachable "cloud" branch (ensure symbols to change include
useAccessibleV2Workspaces, hostType, and
V2WorkspaceDeviceCounts/V2WorkspaceHostType).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 095a53cb-ac73-46af-8942-063709bd9e83
📒 Files selected for processing (12)
apps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions/useWorkspaceHostOptions.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
6 tasks
5 tasks
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
v2Hostswere routed through the cloud relay instead of the local host service. The locality check inv2-workspace/layout.tsx(and 5 sibling sites) leftJoinedv2Workspaces → v2Hostsand comparedjoined.hostMachineId === machineId— when Electric hadn't synced the host row, the joined value wasnull, the equality failed, and the entireWorkspaceTrpcProvidergot pointed at${RELAY_URL}/hosts/....v2Workspaces.hostIdisnotNulland FKs tov2Hosts.machineId, so the workspace already carries the value the join was producing. Switched locality toworkspace.hostId === machineIdeverywhere. Routing-only paths drop thev2Hostsjoin entirely; metadata-display paths (sidebar / list) keep aninnerJoinfor host name +isOnline.useAccessibleV2Workspacesnow stays innerJoin (cleaner; rows briefly hide while Electric catches up but workspaces remain routable via direct URL);DashboardSidebarPortsListsynthesizes a local-host port-query target so local ports show without a syncedv2Hostsrow;DevicePickersynthesizes "this device" so the picker never loses the local entry.Test plan
bun run lint:fix— cleanbun run typecheck— cleanbun test(desktop) — 1870 passv2Hostsin tanstack-db inspector, navigate to/v2-workspace/$workspaceId, confirmWorkspaceTrpcProviderconnects tohttp://127.0.0.1:<port>(not the relay) and the workspace loadsv2Hostscleared, "this device" still appears as defaultv2Hostscleared, local ports still renderhostIdis a different machine; confirm it still routes via${RELAY_URL}/hosts/<key>Summary by cubic
Fixes v2 workspace routing by deriving locality from
v2Workspaces.hostId === machineId, avoiding relay misrouting whenv2Hostshasn’t synced. Also keeps local ports and “this device” visible even if the local host row is missing.Bug Fixes
hostId === machineId; otherwise use the relay URL.machineId+activeHostUrl.machineIdwhen the localv2Hostsrow is missing.Refactors
v2HostsleftJoin from routing-only queries; comparehostIddirectly.innerJointov2Hostsonly where host metadata (name,isOnline) is displayed.hostTypederivation tohostId === machineId ? "local-device" : "remote-device".Written for commit 5e6513a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit