diff --git a/web/src/lib/stores/workspace.svelte.ts b/web/src/lib/stores/workspace.svelte.ts index c00727275..8bd7996cc 100644 --- a/web/src/lib/stores/workspace.svelte.ts +++ b/web/src/lib/stores/workspace.svelte.ts @@ -248,23 +248,26 @@ export const workspaceStore = { }, async create(data: { name: string; description?: string; template?: string }) { + // CLEAR NOTHING UNTIL THE CREATE HAS SUCCEEDED (codex round 3). + // + // This used to clear membership at entry, mirroring `setCurrent` — but + // `setCurrent` is switching to a workspace it already names, while a + // create that FAILS leaves the current workspace exactly as it was. So + // the clear was making an assertion about the wrong workspace, and my + // round-2 fix made that assertion louder rather than removing it: + // settling the flag turned "we don't know" into "no access to the + // workspace you are still looking at", which hid a mounted settings + // page's owner controls until the next `setCurrent`. A failed create + // says nothing about the current membership, so it now changes nothing. + // + // Claiming the sequence token after the call, rather than before, keeps + // the guard it was written for: this create still supersedes anything + // started before this point, and a `setCurrent` started after it still + // supersedes this. + const ws = await api.workspaces.create(data); const seq = ++membershipSeq; currentMembership = null; membershipKnown = false; - 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 index f3b2f334f..438155ff2 100644 --- a/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts +++ b/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts @@ -64,11 +64,40 @@ describe('workspaceStore.membershipKnown', () => { expect(workspaceStore.membershipKnown).toBe(true); }); - it('is true after a FAILED create, which used to leave it false forever', async () => { + it('is false while the WORKSPACE is still resolving, not just the /me', async () => { + // The contract is the whole replacing call, so the window has to open + // before `/me` is even reached (codex round 3). const { workspaceStore } = await import('./workspace.svelte'); - api.workspaces.create.mockRejectedValue(new Error('plan limit')); + let releaseGet: (v: unknown) => void = () => {}; + api.workspaces.get.mockReturnValue(new Promise((r) => { releaseGet = r; })); + api.workspaces.me.mockResolvedValue(OWNER); - await expect(workspaceStore.create({ name: 'nope' })).rejects.toThrow('plan limit'); + const pending = workspaceStore.setCurrent('ws'); + expect(workspaceStore.membershipKnown).toBe(false); + + releaseGet(WS); + await pending; expect(workspaceStore.membershipKnown).toBe(true); }); + + it('leaves the CURRENT workspace untouched when a create fails', async () => { + // A failed create says nothing about the workspace you are still looking + // at. The first version of this fix cleared membership at entry and then + // settled the flag on the failure path, which told every consumer the + // current workspace was now a definitive "no access" — hiding a mounted + // settings page's owner controls until the next setCurrent (codex round + // 3). The flag alone cannot catch that, so this asserts the membership. + const { workspaceStore } = await import('./workspace.svelte'); + api.workspaces.get.mockResolvedValue(WS); + api.workspaces.me.mockResolvedValue(OWNER); + await workspaceStore.setCurrent('ws'); + expect(workspaceStore.isOwner).toBe(true); + + api.workspaces.create.mockRejectedValue(new Error('plan limit')); + await expect(workspaceStore.create({ name: 'nope' })).rejects.toThrow('plan limit'); + + expect(workspaceStore.membershipKnown).toBe(true); + expect(workspaceStore.currentMembership).not.toBeNull(); + expect(workspaceStore.isOwner).toBe(true); + }); }); diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index 2d7c038a0..3370172f3 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -128,8 +128,10 @@ // on non-null would hold the last good answer forever once the answer // became a denial: an owner removed from the workspace, or a `/me` that // 403s, would keep the Save buttons, the invite form and the Danger Zone - // tab on screen indefinitely. `membershipKnown` is false only while a - // fetch is in flight, which is exactly the window this cache exists for. + // tab on screen indefinitely. `membershipKnown` is false for the span of + // any call that will replace membership — workspace resolution and + // creation included, not only the `/me` request — which is exactly the + // window this cache exists to ride out. if (workspaceStore.membershipKnown) { canEditWs = workspaceStore.canEditWorkspace; isOwner = workspaceStore.isOwner; @@ -144,7 +146,7 @@ let validTabIds = $derived(tabs.map(t => t.id)); // Hash-driven tab restoration. The hash is captured once on mount, but - // validTabIds is reactive (depends on workspaceStore.canEditWorkspace, + // validTabIds is reactive (depends on the sticky `canEditWs` above, // which arrives async from /me). So we re-evaluate when validTabIds // expands \u2014 otherwise an owner deep-linking to #danger lands on // General because /me hadn't loaded yet at mount time.