mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 14:30:13 +00:00
fix(web): settle membershipKnown when workspace creation fails (BUG-2978, codex round 2)
`create()` clears membership at entry for the same reason `setCurrent` does, and a rejected `api.workspaces.create()` returned before anything settled the flag — so a failed create left `membershipKnown` false permanently. That is the worst value for it to be stuck at: consumers cache THROUGH the false window, so a page still mounted would hold its permissions and never update again. Membership is already null at that point, so settling it true says "no access", which is what the null already meant before the flag existed. Also narrows a claim I made in the round-1 comments and commit message: `membershipKnown` is not "false only while a membership fetch is in flight". It is false from the start of any call that will replace membership — workspace resolution and creation included — and true once that call settles on every exit. The prose now says that, because the flag is a contract other consumers will gate on and the narrower sentence was the one that made this path look impossible. Four store-level tests cover the arms: in-flight, 403, workspace-not-resolved, and failed create. The last one was mutation-tested — removing the line that settles the flag makes exactly that leg fail and leaves the other three green. Found by Codex review round 2 (P2 + nit). Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm
This commit is contained in:
@@ -8,8 +8,12 @@ let current = $state<Workspace | null>(null);
|
||||
let currentMembership = $state<WorkspaceMembership | null>(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;
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user