diff --git a/web/src/lib/stores/workspace.svelte.ts b/web/src/lib/stores/workspace.svelte.ts index ae6fd71ca..c00727275 100644 --- a/web/src/lib/stores/workspace.svelte.ts +++ b/web/src/lib/stores/workspace.svelte.ts @@ -8,8 +8,12 @@ let current = $state(null); let currentMembership = $state(null); // Whether `currentMembership` is an ANSWER or merely NOT YET FETCHED (BUG-2978). // It is null in both cases, which makes the two indistinguishable to consumers — -// and they are opposites: one is "wait", the other is "no access". False while a -// membership fetch is in flight, true once one has settled either way. +// and they are opposites: one is "wait", the other is "no access". +// +// False from the moment a call that will replace membership begins — which +// includes resolving the workspace itself, and creating one — until that call +// settles. True once it has, on every exit: a fetched membership, a 403, a +// workspace that did not resolve, or a create that threw. let membershipKnown = $state(false); let loading = $state(false); @@ -67,8 +71,11 @@ export const workspaceStore = { * True once a membership fetch has SETTLED for the current workspace, so a * null `currentMembership` means "no access" rather than "not yet loaded". * - * Consumers that cache a permission to avoid flickering during the fetch - * window (BUG-2978) must gate on this rather than on `currentMembership !== + * False spans the whole replacing call — workspace resolution and creation + * included, not just the `/me` request itself. + * + * Consumers that cache a permission to avoid flickering during that window + * (BUG-2978) must gate on this rather than on `currentMembership !== * null`: gating on non-null holds the last good answer forever when the * answer becomes a definitive denial — a removed member or a 403 keeps * owner-only affordances on screen. The server remains the enforcement @@ -244,7 +251,20 @@ export const workspaceStore = { const seq = ++membershipSeq; currentMembership = null; membershipKnown = false; - const ws = await api.workspaces.create(data); + let ws: Workspace; + try { + ws = await api.workspaces.create(data); + } catch (err) { + // SETTLE THE FLAG ON THE WAY OUT (codex round 2). This method clears + // membership at entry for the same reason `setCurrent` does, so a + // create that throws would otherwise leave `membershipKnown` false + // forever — permanently "still loading" for a fetch that will never + // happen, which is exactly the state consumers cache through. + // Membership is already null, so "known" here says no access, which + // is what the null already meant before this flag existed. + if (seq === membershipSeq) membershipKnown = true; + throw err; + } if (seq !== membershipSeq) return ws; workspaces = [...workspaces, ws]; current = ws; diff --git a/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts b/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts new file mode 100644 index 000000000..f3b2f334f --- /dev/null +++ b/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; + +/** + * BUG-2978 — `membershipKnown` says whether a null `currentMembership` is an + * ANSWER ("no access") or merely NOT YET FETCHED. Consumers cache permissions + * through the false window, so a path that leaves it false forever is a cache + * that never updates again. + * + * The create-failure leg is the one codex round 2 found: `create()` clears + * membership at entry, and a rejected create returned before anything settled + * the flag. + */ + +const api = vi.hoisted(() => ({ + workspaces: { + get: vi.fn(), + me: vi.fn(), + list: vi.fn(), + create: vi.fn(), + }, +})); + +vi.mock('$lib/api/client', () => ({ api })); + +const OWNER = { role: 'owner', collection_grants: [], item_grants: [] }; +const WS = { id: 'w1', slug: 'ws', name: 'WS' }; + +describe('workspaceStore.membershipKnown', () => { + beforeEach(() => { + vi.resetAllMocks(); + }); + + it('is false while a membership fetch is in flight and true once it settles', async () => { + const { workspaceStore } = await import('./workspace.svelte'); + let release: (v: unknown) => void = () => {}; + api.workspaces.get.mockResolvedValue(WS); + api.workspaces.me.mockReturnValue(new Promise((r) => { release = r; })); + + const pending = workspaceStore.setCurrent('ws'); + expect(workspaceStore.membershipKnown).toBe(false); + + release(OWNER); + await pending; + expect(workspaceStore.membershipKnown).toBe(true); + expect(workspaceStore.isOwner).toBe(true); + }); + + it('is true after a 403 — a denial is an answer, not a pending state', async () => { + const { workspaceStore } = await import('./workspace.svelte'); + api.workspaces.get.mockResolvedValue(WS); + api.workspaces.me.mockRejectedValue(new Error('403')); + + await workspaceStore.setCurrent('ws'); + expect(workspaceStore.membershipKnown).toBe(true); + expect(workspaceStore.currentMembership).toBeNull(); + expect(workspaceStore.isOwner).toBe(false); + }); + + it('is true when the workspace itself does not resolve', async () => { + const { workspaceStore } = await import('./workspace.svelte'); + api.workspaces.get.mockRejectedValue(new Error('404')); + + await workspaceStore.setCurrent('missing'); + expect(workspaceStore.membershipKnown).toBe(true); + }); + + it('is true after a FAILED create, which used to leave it false forever', async () => { + const { workspaceStore } = await import('./workspace.svelte'); + api.workspaces.create.mockRejectedValue(new Error('plan limit')); + + await expect(workspaceStore.create({ name: 'nope' })).rejects.toThrow('plan limit'); + expect(workspaceStore.membershipKnown).toBe(true); + }); +});