diff --git a/CLAUDE.md b/CLAUDE.md index 270f78b9af..6cbeba5b6f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,7 +36,9 @@ Keep server state and client state separate. - React Context is for platform plumbing only, such as `WorkspaceIdProvider` and `NavigationProvider`. - Only auth/workspace stores may call `api.*` directly. Other server interaction belongs in queries/mutations. - Workspace-scoped query keys must include `wsId`. -- Mutations should be optimistic by default: patch locally, send request, roll back on failure, invalidate on settle. +- Optimistic updates only when ALL hold: outcome locally predictable, user stays on the same screen (no navigation), failure is rare, rollback is trivial. Canonical: status/assignee/toggle field patches — patch determinate caches, roll back on failure, invalidate uncertain projections on settle. +- Flows that navigate or confirm (create, delete, leave) must await the server before navigating or cleaning up; never optimistically remove an entity from cache. +- Chat/message send uses the pending-message pattern: render immediately with a visible pending state and retry on failure, not silent optimism. - WebSocket events invalidate or patch Query cache; they never write directly to Zustand stores. - Persist durable preferences/drafts/layout. Do not persist server data or ephemeral UI state. - Zustand selectors must return stable references. Do not return freshly allocated objects/arrays from selectors without shallow comparison. diff --git a/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx b/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx index b40d0d8f9b..6084a0f3c4 100644 --- a/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx +++ b/packages/core/realtime/use-realtime-sync-ws-instance.test.tsx @@ -4,8 +4,14 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { renderHook } from "@testing-library/react"; import type { ReactNode } from "react"; -import { describe, expect, it, vi, beforeEach } from "vitest"; +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; import type { WSClient } from "../api/ws-client"; +import { defaultStorage } from "../platform/storage"; +import { workspaceKeys } from "../workspace/queries"; +import { + markWorkspaceDeletePending, + unmarkWorkspaceDeletePending, +} from "../workspace/pending-delete"; import { useRealtimeSync, type RealtimeSyncStores } from "./use-realtime-sync"; vi.mock("../platform/workspace-storage", () => ({ @@ -187,3 +193,59 @@ describe("useRealtimeSync — ws instance change", () => { expect(calls).toContainEqual(["task-messages"]); }); }); + +describe("useRealtimeSync — workspace:deleted self-initiated suppression", () => { + let qc: QueryClient; + let stores: RealtimeSyncStores; + + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + stores = createStores(); + }); + + afterEach(() => { + unmarkWorkspaceDeletePending("ws-2"); + localStorage.clear(); + }); + + // getCurrentWsId is mocked to "ws-1" at module level, so deleting "ws-2" + // never enters the relocate branch — these tests only exercise the + // storage-cleanup path, which is the observable difference between a + // handled and a suppressed event. + const dispatchWorkspaceDeleted = (ws: WSClient, workspaceId: string) => { + const call = vi + .mocked(ws.on) + .mock.calls.find(([event]) => event === "workspace:deleted"); + expect(call).toBeDefined(); + (call![1] as (p: unknown) => void)({ workspace_id: workspaceId }); + }; + + it("ignores the event for a delete this client initiated", () => { + const ws = createMockWs(); + renderHook(() => useRealtimeSync(ws, stores), { + wrapper: createWrapper(qc), + }); + qc.setQueryData(workspaceKeys.list(), [{ id: "ws-2", slug: "delete-me" }]); + defaultStorage.setItem("multica_issue_draft:delete-me", "draft"); + + markWorkspaceDeletePending("ws-2"); + dispatchWorkspaceDeleted(ws, "ws-2"); + + // useDeleteWorkspace.onSuccess owns cleanup for self-initiated deletes; + // the handler must not have touched storage. + expect(defaultStorage.getItem("multica_issue_draft:delete-me")).toBe("draft"); + }); + + it("still cleans up for a delete initiated elsewhere", () => { + const ws = createMockWs(); + renderHook(() => useRealtimeSync(ws, stores), { + wrapper: createWrapper(qc), + }); + qc.setQueryData(workspaceKeys.list(), [{ id: "ws-2", slug: "delete-me" }]); + defaultStorage.setItem("multica_issue_draft:delete-me", "draft"); + + dispatchWorkspaceDeleted(ws, "ws-2"); + + expect(defaultStorage.getItem("multica_issue_draft:delete-me")).toBeNull(); + }); +}); diff --git a/packages/core/realtime/use-realtime-sync.ts b/packages/core/realtime/use-realtime-sync.ts index f2db8b8a82..3f6a3733f1 100644 --- a/packages/core/realtime/use-realtime-sync.ts +++ b/packages/core/realtime/use-realtime-sync.ts @@ -38,6 +38,7 @@ import { notificationPreferenceKeys, } from "../notification-preferences/queries"; import { workspaceKeys, workspaceListOptions } from "../workspace/queries"; +import { isWorkspaceDeletePending } from "../workspace/pending-delete"; import { showWebNotification, type SystemNotificationPayload, @@ -794,6 +795,12 @@ export function useRealtimeSync( const unsubWsDeleted = ws.on("workspace:deleted", (p) => { const { workspace_id } = p as WorkspaceDeletedPayload; + // Self-initiated delete: useDeleteWorkspace owns storage cleanup and + // navigation (both run after the DELETE resolves). Reacting here too + // would race that flow's navigation with a full-page relocate — the + // CancelledError + reload combo this guard exists to prevent. This + // handler only serves deletes initiated elsewhere (other user/device). + if (isWorkspaceDeletePending(workspace_id)) return; // Event payload has UUID; look up slug from cached workspace list // since clearWorkspaceStorage keys are namespaced by slug. const wsList = qc.getQueryData<{ id: string; slug: string }[]>(workspaceKeys.list()) ?? []; diff --git a/packages/core/workspace/mutations.test.tsx b/packages/core/workspace/mutations.test.tsx new file mode 100644 index 0000000000..371f598c48 --- /dev/null +++ b/packages/core/workspace/mutations.test.tsx @@ -0,0 +1,177 @@ +/** + * @vitest-environment jsdom + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactNode } from "react"; +import { setApiInstance } from "../api"; +import type { ApiClient } from "../api/client"; +import { defaultStorage } from "../platform/storage"; +import type { Workspace } from "../types"; +import { useDeleteWorkspace } from "./mutations"; +import { workspaceKeys } from "./queries"; +import { + isWorkspaceDeletePending, + unmarkWorkspaceDeletePending, +} from "./pending-delete"; + +function createWrapper(qc: QueryClient) { + return function Wrapper({ children }: { children: ReactNode }) { + return {children}; + }; +} + +const makeWorkspace = (id: string, slug: string): Workspace => ({ + id, + name: slug, + slug, + description: null, + context: null, + settings: {}, + repos: [], + issue_prefix: "MUL", + avatar_url: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", +}); + +describe("useDeleteWorkspace", () => { + let qc: QueryClient; + let deleteWorkspace: ReturnType Promise>>; + let listWorkspaces: ReturnType Promise>>; + + const serverList = () => [ + makeWorkspace("ws-1", "keep-me"), + makeWorkspace("ws-2", "delete-me"), + ]; + + const seedList = () => { + qc.setQueryData(workspaceKeys.list(), serverList()); + }; + + const cachedList = () => + qc.getQueryData(workspaceKeys.list()) ?? []; + + beforeEach(() => { + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + deleteWorkspace = vi.fn().mockResolvedValue(undefined); + listWorkspaces = vi.fn().mockResolvedValue(serverList()); + setApiInstance({ deleteWorkspace, listWorkspaces } as unknown as ApiClient); + }); + + afterEach(() => { + qc.clear(); + // The self-initiated marker is module state and is intentionally KEPT + // after a successful delete (it suppresses the WS echo); reset it so + // tests stay independent. + unmarkWorkspaceDeletePending("ws-2"); + localStorage.clear(); + vi.restoreAllMocks(); + }); + + it("leaves the list cache untouched while the DELETE is pending (no optimistic removal)", async () => { + seedList(); + // Hold the DELETE open to observe the pending window. The flow awaits + // the mutation with the dialog in a loading state, so the cache must + // keep reflecting server truth: the workspace still exists. + let resolveDelete!: () => void; + deleteWorkspace.mockReturnValue( + new Promise((resolve) => { + resolveDelete = resolve; + }), + ); + + const { result } = renderHook(() => useDeleteWorkspace(), { + wrapper: createWrapper(qc), + }); + + let mutationDone: Promise; + await act(async () => { + mutationDone = result.current.mutateAsync("ws-2"); + await Promise.resolve(); + }); + + expect(deleteWorkspace).toHaveBeenCalledWith("ws-2"); + expect(cachedList().map((w) => w.id)).toEqual(["ws-1", "ws-2"]); + + await act(async () => { + resolveDelete(); + await mutationDone; + }); + }); + + it("invalidates the workspace list after a successful delete", async () => { + seedList(); + const { result } = renderHook(() => useDeleteWorkspace(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current.mutateAsync("ws-2"); + }); + + expect(qc.getQueryState(workspaceKeys.list())?.isInvalidated).toBe(true); + }); + + it("clears the deleted slug's workspace-scoped storage on success", async () => { + seedList(); + // The realtime `workspace:deleted` handler skips self-initiated deletes, + // so the mutation owns this cleanup; the slug is captured from the list + // cache before the mutation fires. + defaultStorage.setItem("multica_issue_draft:delete-me", "draft"); + defaultStorage.setItem("multica_issue_draft:keep-me", "draft"); + + const { result } = renderHook(() => useDeleteWorkspace(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await result.current.mutateAsync("ws-2"); + }); + + expect(defaultStorage.getItem("multica_issue_draft:delete-me")).toBeNull(); + expect(defaultStorage.getItem("multica_issue_draft:keep-me")).toBe("draft"); + }); + + it("leaves storage and cache untouched when the DELETE fails", async () => { + seedList(); + deleteWorkspace.mockRejectedValue(new Error("boom")); + defaultStorage.setItem("multica_issue_draft:delete-me", "draft"); + + const { result } = renderHook(() => useDeleteWorkspace(), { + wrapper: createWrapper(qc), + }); + + await act(async () => { + await expect(result.current.mutateAsync("ws-2")).rejects.toThrow("boom"); + }); + + // No optimistic write happened, so there is nothing to roll back. + expect(defaultStorage.getItem("multica_issue_draft:delete-me")).toBe("draft"); + expect(cachedList().map((w) => w.id)).toEqual(["ws-1", "ws-2"]); + }); + + it("keeps the self-initiated marker after success and lifts it after failure", async () => { + seedList(); + const { result } = renderHook(() => useDeleteWorkspace(), { + wrapper: createWrapper(qc), + }); + + // Success: the id is gone for good; the kept marker suppresses the WS + // echo of our own delete whenever it arrives. + await act(async () => { + await result.current.mutateAsync("ws-2"); + }); + expect(isWorkspaceDeletePending("ws-2")).toBe(true); + + // Failure: the workspace still exists, so a later external delete of + // the same id must be handled by the realtime handler again. + unmarkWorkspaceDeletePending("ws-2"); + deleteWorkspace.mockRejectedValue(new Error("boom")); + await act(async () => { + await expect(result.current.mutateAsync("ws-2")).rejects.toThrow("boom"); + }); + expect(isWorkspaceDeletePending("ws-2")).toBe(false); + }); +}); diff --git a/packages/core/workspace/mutations.ts b/packages/core/workspace/mutations.ts index f23bb7f964..84487d4572 100644 --- a/packages/core/workspace/mutations.ts +++ b/packages/core/workspace/mutations.ts @@ -1,7 +1,13 @@ import { useMutation, useQueryClient } from "@tanstack/react-query"; import type { Workspace } from "../types"; import { api } from "../api"; +import { defaultStorage } from "../platform/storage"; +import { clearWorkspaceStorage } from "../platform/storage-cleanup"; import { workspaceKeys } from "./queries"; +import { + markWorkspaceDeletePending, + unmarkWorkspaceDeletePending, +} from "./pending-delete"; export function useCreateWorkspace() { const qc = useQueryClient(); @@ -39,6 +45,34 @@ export function useDeleteWorkspace() { const qc = useQueryClient(); return useMutation({ mutationFn: (workspaceId: string) => api.deleteWorkspace(workspaceId), + // No optimistic removal: the delete flow awaits this mutation with the + // confirm dialog in a loading state and only navigates on success, so + // the cache staying truthful (server still has the row until commit) + // is correct, and a failed DELETE needs no rollback. + onMutate: (workspaceId) => { + // Mark the delete as self-initiated so the realtime `workspace:deleted` + // handler no-ops instead of racing this flow's navigation with its own + // full-page relocate. See pending-delete.ts for lifetime rules. + markWorkspaceDeletePending(workspaceId); + // Capture the slug for onSuccess's storage cleanup — cheap here, and + // the row is guaranteed to still be in the list pre-mutation. + const slug = qc + .getQueryData(workspaceKeys.list()) + ?.find((w) => w.id === workspaceId)?.slug; + return { slug }; + }, + // Success is the only path that clears the deleted workspace's persisted + // `${key}:${slug}` namespace — a failed DELETE means the workspace still + // exists and its drafts/view state must survive. The realtime handler + // skips self-initiated deletes, so cleanup has to happen here. + onSuccess: (_data, _workspaceId, ctx) => { + if (ctx?.slug) clearWorkspaceStorage(defaultStorage, ctx.slug); + }, + // The workspace still exists after a failed DELETE, so a later external + // delete of the same ID must be handled by the realtime handler again. + onError: (_err, workspaceId) => { + unmarkWorkspaceDeletePending(workspaceId); + }, onSettled: () => { qc.invalidateQueries({ queryKey: workspaceKeys.list() }); }, diff --git a/packages/core/workspace/pending-delete.ts b/packages/core/workspace/pending-delete.ts new file mode 100644 index 0000000000..e580c5de57 --- /dev/null +++ b/packages/core/workspace/pending-delete.ts @@ -0,0 +1,32 @@ +/** + * Registry of workspace IDs whose DELETE was initiated by THIS client. + * + * Marked in useDeleteWorkspace.onMutate. The realtime `workspace:deleted` + * handler checks it and no-ops for self-initiated deletes: the mutation flow + * owns storage cleanup and navigation (both run after the DELETE resolves), + * and letting the handler react too would race that flow's navigation with + * its own full-page relocate. + * + * Lifted only when the DELETE fails — the workspace still exists, so a later + * external delete of the same ID must be handled normally. On success the ID + * is gone for good, and keeping the mark suppresses the WS echo of our own + * delete no matter when it arrives. + * + * Module scope rather than React state because the realtime handler runs + * outside the component tree. Per-tab by construction: other tabs/devices + * have an empty registry, so their handlers process the event normally. + */ +const pendingDeletes = new Set(); + +export function markWorkspaceDeletePending(workspaceId: string) { + pendingDeletes.add(workspaceId); +} + +export function unmarkWorkspaceDeletePending(workspaceId: string) { + pendingDeletes.delete(workspaceId); +} + +/** True if this client initiated a DELETE for the workspace. */ +export function isWorkspaceDeletePending(workspaceId: string): boolean { + return pendingDeletes.has(workspaceId); +} diff --git a/packages/views/settings/components/workspace-tab.tsx b/packages/views/settings/components/workspace-tab.tsx index 35232b6755..d26e244824 100644 --- a/packages/views/settings/components/workspace-tab.tsx +++ b/packages/views/settings/components/workspace-tab.tsx @@ -21,7 +21,6 @@ import { toast } from "sonner"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { useAuthStore } from "@multica/core/auth"; import { useLeaveWorkspace, useDeleteWorkspace } from "@multica/core/workspace/mutations"; -import { useWorkspaceId } from "@multica/core/hooks"; import { memberListOptions, workspaceKeys, @@ -46,8 +45,17 @@ export function WorkspaceTab() { const { t } = useT("settings"); const user = useAuthStore((s) => s.user); const workspace = useCurrentWorkspace(); - const wsId = useWorkspaceId(); - const { data: members = [], isFetched: membersFetched } = useQuery(memberListOptions(wsId)); + // Derive the id from useCurrentWorkspace instead of the throwing + // useWorkspaceId: this component can legitimately render while the + // workspace is gone from the list cache but the URL slug hasn't changed + // yet (post-delete invalidation before navigation completes, or an + // external delete of the workspace we're on). The `!workspace` guard + // below renders null for that window; a throwing hook would crash first. + const wsId = workspace?.id; + const { data: members = [], isFetched: membersFetched } = useQuery({ + ...memberListOptions(wsId ?? ""), + enabled: !!wsId, + }); const qc = useQueryClient(); const leaveWorkspace = useLeaveWorkspace(); const deleteWorkspace = useDeleteWorkspace(); @@ -55,36 +63,34 @@ export function WorkspaceTab() { const hasOnboarded = useHasOnboarded(); /** - * Send the user to a safe URL BEFORE the leave/delete mutation fires. - * The destination is computed from the current cached workspace list, - * minus the workspace that's about to go away. + * Send the user to a safe URL, computed from the current cached workspace + * list minus the workspace that's going away. * - * Why navigate first, not after: - * 1. The backend broadcasts `workspace:deleted` / `member:removed` the - * moment the mutation lands. If the user is still on the soon-to- - * be-deleted workspace's URL when that event arrives, the realtime - * handler in `use-realtime-sync.ts` also triggers a relocation — - * and both code paths race with the mutation's own - * `invalidateQueries` refetch. The loser's in-flight fetch gets - * cancelled, surfacing as an unhandled `CancelledError`. - * 2. Navigating first means by the time the WS event fires, the - * active workspace is already something else; the realtime - * handler's "current === deleted" check fails and its relocate - * branch no-ops. - * 3. UX: the destructive flow feels instant (dialog closes → new - * workspace appears) even though the API hasn't responded yet. + * Call ordering differs per flow: + * - Delete calls this AFTER the mutation succeeds. The realtime + * `workspace:deleted` handler skips self-initiated deletes (see + * pending-delete.ts), so nothing races this navigation. + * - Leave still calls this BEFORE the mutation fires: `member:removed` + * has no self-initiated marker yet, so if the user were still on the + * workspace's URL when that event arrives, the realtime handler in + * `use-realtime-sync.ts` would trigger a parallel full-page relocate + * that races the mutation's `invalidateQueries` refetch — the loser's + * in-flight fetch gets cancelled, surfacing as an unhandled + * `CancelledError`. Navigating first makes the handler's + * "current === lost workspace" check fail and its relocate no-op. + * Known debt: give leave the same await-then-navigate shape as delete. */ const navigateAwayFromCurrentWorkspace = () => { const cachedList = qc.getQueryData(workspaceListOptions().queryKey) ?? []; const remaining = cachedList.filter((w) => w.id !== workspace?.id); - // Clear the workspace-context singleton BEFORE navigating and BEFORE - // the mutation fires. Three downstream consumers read it: - // 1. Realtime `workspace:deleted` handler's "current === deleted" - // check — if the singleton still points at the deleting workspace - // when the WS event arrives, it fires a parallel relocate that - // races the mutation's invalidate and the settings page's own - // navigate, surfacing a CancelledError and a full-page reload. + // Clear the workspace-context singleton BEFORE navigating. Three + // downstream consumers read it: + // 1. Realtime relocate handlers' "current === lost workspace" check + // (`member:removed` for leave; also a second line of defense for + // delete) — if the singleton still points at the lost workspace + // when the WS event arrives, they fire a parallel full-page + // relocate that races this navigation. // 2. Chrome gating (`{slug && }` on desktop) — if the // singleton lingers, the sidebar stays mounted while the deleted // workspace is no longer in the list, and `useWorkspaceId` throws. @@ -237,14 +243,17 @@ export function WorkspaceTab() { const handleConfirmDelete = async () => { if (!workspace) return; setActionId("delete-workspace"); - // Close the dialog and navigate away FIRST. See navigateAwayFromCurrentWorkspace - // comment for why: keeps the realtime `workspace:deleted` handler out - // of the race so we don't end up with concurrent refetches cancelling - // each other and surfacing CancelledError. - setDeleteDialogOpen(false); - navigateAwayFromCurrentWorkspace(); + // Await the DELETE with the dialog in its loading state, and only + // navigate on success (CLAUDE.md: flows that navigate must await the + // server; no optimistic removal). The realtime `workspace:deleted` + // handler skips self-initiated deletes via the pending-delete registry, + // so it can't race this navigation with its own full-page relocate. + // On failure the dialog stays open, the cache was never touched, and + // the user is exactly where they started. try { await deleteWorkspace.mutateAsync(workspace.id); + setDeleteDialogOpen(false); + navigateAwayFromCurrentWorkspace(); } catch (e) { toast.error(e instanceof Error ? e.message : t(($) => $.workspace.toast_delete_failed)); } finally {