-
Notifications
You must be signed in to change notification settings - Fork 990
fix(desktop): keep sidebar from sticking on "creating" when Electric is slow #4390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -134,12 +134,17 @@ export function useDashboardSidebarData() { | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // In-flight workspace.create operations. These don't have a backing DB row | ||||||||||||||||||||||||||||||||||||||
| // — they're kept in renderer memory until the real v2Workspaces row arrives | ||||||||||||||||||||||||||||||||||||||
| // via Electric sync (or until error/dismiss). | ||||||||||||||||||||||||||||||||||||||
| // via Electric sync (or until error/dismiss). Entries that have already | ||||||||||||||||||||||||||||||||||||||
| // resolved on the host service carry `cloudRow`; those are surfaced as | ||||||||||||||||||||||||||||||||||||||
| // real synced rows below so the sidebar doesn't stick on "creating" when | ||||||||||||||||||||||||||||||||||||||
| // Electric is slow. | ||||||||||||||||||||||||||||||||||||||
| const inFlightEntries = useWorkspaceCreatesStore((store) => store.entries); | ||||||||||||||||||||||||||||||||||||||
| const inFlightSidebarRows = useMemo( | ||||||||||||||||||||||||||||||||||||||
| () => | ||||||||||||||||||||||||||||||||||||||
| inFlightEntries | ||||||||||||||||||||||||||||||||||||||
| .filter((entry) => entry.snapshot.id !== undefined) | ||||||||||||||||||||||||||||||||||||||
| // Entries with a cloudRow are rendered via the synced fallback below. | ||||||||||||||||||||||||||||||||||||||
| .filter((entry) => !(entry.state === "creating" && entry.cloudRow)) | ||||||||||||||||||||||||||||||||||||||
| .map((entry) => ({ | ||||||||||||||||||||||||||||||||||||||
| id: entry.snapshot.id as string, | ||||||||||||||||||||||||||||||||||||||
| projectId: entry.snapshot.projectId, | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -287,6 +292,44 @@ export function useDashboardSidebarData() { | |||||||||||||||||||||||||||||||||||||
| [collections], | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // Cloud-row fallback: when workspaces.create has resolved on the host | ||||||||||||||||||||||||||||||||||||||
| // service but Electric hasn't yet delivered the v2Workspaces row, surface | ||||||||||||||||||||||||||||||||||||||
| // the cloud row cached on the in-flight entry so the sidebar renders the | ||||||||||||||||||||||||||||||||||||||
| // workspace as fully synced. Manager.tsx removes the entry once Electric | ||||||||||||||||||||||||||||||||||||||
| // catches up, at which point the live query takes over seamlessly. | ||||||||||||||||||||||||||||||||||||||
| const cloudRowFallbackWorkspaces = useMemo(() => { | ||||||||||||||||||||||||||||||||||||||
| if (inFlightEntries.length === 0) return []; | ||||||||||||||||||||||||||||||||||||||
| const hostByMachineId = new Map( | ||||||||||||||||||||||||||||||||||||||
| hosts.map((host) => [host.machineId, host]), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
| const rows = inFlightEntries.flatMap((entry) => { | ||||||||||||||||||||||||||||||||||||||
| const cloudRow = entry.cloudRow; | ||||||||||||||||||||||||||||||||||||||
| if (!cloudRow) return []; | ||||||||||||||||||||||||||||||||||||||
| // Electric already delivered; let the live query own this row. | ||||||||||||||||||||||||||||||||||||||
| if (localStateWorkspaceIds.has(cloudRow.id)) return []; | ||||||||||||||||||||||||||||||||||||||
| const localState = collections.v2WorkspaceLocalState.get(cloudRow.id); | ||||||||||||||||||||||||||||||||||||||
| const host = hostByMachineId.get(cloudRow.hostId); | ||||||||||||||||||||||||||||||||||||||
| return [ | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+305
to
+312
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 305-312
Comment:
The fallback includes a workspace with `hostIsOnline: false` even when `cloudRow.hostId` doesn't match any entry in the `hosts` live-query result. The real `rawSidebarWorkspaces` live query uses an inner join with `v2Hosts`, so it would silently exclude the workspace in that same situation. In practice this window should be zero (the host row existed before the workspace was created), but adding an early-return guard makes the fallback's intent explicit and consistent with the live-query contract.
```suggestion
const rows = inFlightEntries.flatMap((entry) => {
const cloudRow = entry.cloudRow;
if (!cloudRow) return [];
// Electric already delivered; let the live query own this row.
if (localStateWorkspaceIds.has(cloudRow.id)) return [];
const localState = collections.v2WorkspaceLocalState.get(cloudRow.id);
const host = hostByMachineId.get(cloudRow.hostId);
// No host row yet — mirror the inner-join behaviour of rawSidebarWorkspaces.
if (!host) return [];
return [
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||
| id: cloudRow.id, | ||||||||||||||||||||||||||||||||||||||
| projectId: localState?.sidebarState.projectId ?? cloudRow.projectId, | ||||||||||||||||||||||||||||||||||||||
| hostId: cloudRow.hostId, | ||||||||||||||||||||||||||||||||||||||
| type: cloudRow.type, | ||||||||||||||||||||||||||||||||||||||
| hostIsOnline: host?.isOnline ?? false, | ||||||||||||||||||||||||||||||||||||||
| name: cloudRow.name, | ||||||||||||||||||||||||||||||||||||||
| branch: cloudRow.branch, | ||||||||||||||||||||||||||||||||||||||
| createdAt: cloudRow.createdAt, | ||||||||||||||||||||||||||||||||||||||
| updatedAt: cloudRow.updatedAt, | ||||||||||||||||||||||||||||||||||||||
| tabOrder: | ||||||||||||||||||||||||||||||||||||||
| localState?.sidebarState.tabOrder ?? PENDING_WORKSPACE_TAB_ORDER, | ||||||||||||||||||||||||||||||||||||||
| sectionId: localState?.sidebarState.sectionId ?? null, | ||||||||||||||||||||||||||||||||||||||
| isHidden: localState?.sidebarState.isHidden ?? false, | ||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||
| return getVisibleSidebarWorkspaces(rows); | ||||||||||||||||||||||||||||||||||||||
| }, [collections, hosts, inFlightEntries, localStateWorkspaceIds]); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| const visibleSidebarWorkspaces = useMemo(() => { | ||||||||||||||||||||||||||||||||||||||
| const sidebarProjectIds = new Set( | ||||||||||||||||||||||||||||||||||||||
| sidebarProjects.map((project) => project.id), | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -298,8 +341,13 @@ export function useDashboardSidebarData() { | |||||||||||||||||||||||||||||||||||||
| sidebarProjectIds.has(workspace.projectId), | ||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| return [...autoLocalMainWorkspaces, ...sidebarWorkspaces]; | ||||||||||||||||||||||||||||||||||||||
| return [ | ||||||||||||||||||||||||||||||||||||||
| ...autoLocalMainWorkspaces, | ||||||||||||||||||||||||||||||||||||||
| ...sidebarWorkspaces, | ||||||||||||||||||||||||||||||||||||||
| ...cloudRowFallbackWorkspaces, | ||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||
| }, [ | ||||||||||||||||||||||||||||||||||||||
| cloudRowFallbackWorkspaces, | ||||||||||||||||||||||||||||||||||||||
| localMainWorkspaces, | ||||||||||||||||||||||||||||||||||||||
| localStateWorkspaceIds, | ||||||||||||||||||||||||||||||||||||||
| machineId, | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collections.v2WorkspaceLocalState.get(cloudRow.id)reads the TanStack DB collection directly inside auseMemo. Ifcollectionsis a stable context-object reference (i.e. its identity doesn't change when a row is upserted), this memo will not re-execute whenv2WorkspaceLocalStateis updated aftercloudRowis set — soprojectId,tabOrder,sectionId, andisHiddencould be stale. In practice the local-state row is written byuseWorkspaceCreates.dispatchbefore the API call and isn't mutated during the fallback window, so the impact is minimal today. But if that assumption changes (e.g. user reorders while Electric is slow), the fallback would silently render with outdated positioning.Prompt To Fix With AI