fix(web): a failed create says nothing about the current workspace (BUG-2978, codex round 3)

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
This commit is contained in:
xarmian
2026-09-09 19:31:42 +00:00
parent 7fb49e098d
commit d7ccaeaf19
3 changed files with 54 additions and 20 deletions
+17 -14
View File
@@ -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;
@@ -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);
});
});
@@ -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.