From d7ccaeaf19951b08e386b3297438f428183b13fa Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 9 Sep 2026 19:31:42 +0000 Subject: [PATCH] fix(web): a failed create says nothing about the current workspace (BUG-2978, codex round 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2's fix was pointed at the wrong thing. `create()` cleared 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 asserting something about the wrong workspace, and settling `membershipKnown` on the failure path made that assertion louder rather than removing it: it 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`. I fixed the symptom my flag exposed instead of the clear that was wrong underneath it. Nothing is cleared now until the create has SUCCEEDED, so a failure changes no state at all. The sequence token is claimed after the call rather than before, which keeps the guard it was written for: this create still supersedes anything started before it, and a `setCurrent` started after it still supersedes this. Two test legs the previous round missed, both of which it should have had: - a failed create leaves the CURRENT membership and `isOwner` intact — the flag alone could not have caught this, which is why the old leg passed while the regression was live; - `membershipKnown` is false while the WORKSPACE is still resolving, not merely while `/me` is, which is the contract the prose now claims. Two stale comments corrected: the settings page still said `membershipKnown` is "false only while a fetch is in flight" (the claim 7fb49e09 narrowed in the store but not here), and the hash-restoration comment still named `workspaceStore.canEditWorkspace` as `validTabIds`' dependency when it has been the sticky `canEditWs` since e6ed6fdd. For the record, since a pushed commit message cannot be edited: 4a1bfd2a's "false only while a membership fetch is in flight" is wrong in the same way, and this is the correction. Found by Codex review round 3 (P2 + two nits). Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm --- web/src/lib/stores/workspace.svelte.ts | 31 ++++++++-------- .../workspaceMembershipKnown.svelte.test.ts | 35 +++++++++++++++++-- .../[workspace]/settings/+page.svelte | 8 +++-- 3 files changed, 54 insertions(+), 20 deletions(-) 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.