fix(web): settings permissions are sticky, so the owner-only tab survives the /me window (BUG-2978) (#1307)

* fix(web): settings permissions are sticky, so the owner-only tab survives the /me window (BUG-2978)

Deep-linking `/{user}/{ws}/settings#danger` landed on General for a workspace
OWNER — 0/10 loads, at both 390px and 1280px, while `#storage` and `#members`
were 10/10.

The hash-restoration effect was not the fault, which is where I looked first.
Instrumenting it showed the effect applying `danger` correctly at 219ms and
losing it at 244ms. `workspaceStore.setCurrent` clears `currentMembership` to
null before `/me` resolves and the permission helpers treat unknown as
no-access by design; this route calls `setCurrent` TWICE per load, once from
the workspace layout and once from the page's own `load()`. So
`canEditWorkspace` reads true -> false -> true, the owner-only tab drops out of
the tab set during the false window, the effect's snap-back branch moves
`activeTab` off the now-invalid `danger`, and `pendingHash` was already
consumed — nothing restores it when the permission returns. Only the owner-only
tab could hit this, which is exactly why `#storage` never did.

The page now reads its permissions through sticky state that updates only when
membership is definitively known, reset on a real workspace switch — the same
two-effect shape the dashboard already uses for its owner-gated CTA, and for
the same reason (CONVE-606). `isOwner` and `canExport` get the same treatment,
because they flicker identically and gate ~15 controls on this page: read
straight from the store, an ordinary owner load makes the Save buttons, the
invite form and the delete controls go readonly and then come back.

One reset effect owns all three. Two effects testing the same
`wsSlug !== lastPermSlug` could never both fire, since whichever ran first
would have already updated the marker — an error in my first draft of this fix.

Default stays false, so owner-only chrome still never flashes before `/me`
confirms; the server-side owner check remains the enforcement boundary.

Measured after the fix: 40/40 deep links land across `#danger`, `#storage`,
`#members` and `#collections` at both widths, against 0/10 for `#danger` on
main.

The regression spec settles and then asserts ONCE, rather than using an
auto-retrying assertion: the pre-fix failure is "correct, then reverted", so a
retrying matcher can observe the correct intermediate state and pass on a
broken build. The owner leg asserts again after a further wait so a later
revert is still caught, and carries a non-vacuity check that the fixture user
really is an owner.

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm

* test(web): make the BUG-2978 guard the unit test, because the e2e leg does not discriminate

The e2e spec I wrote for this fix PASSES ON THE UNFIXED BUILD. I checked, which
is the only reason this is a commit and not a false green: on the e2e fixture
the layout's `setCurrent` and the page's own land inside a single unresolved
`/me` window, so membership never goes known -> unknown -> known and the flicker
the bug needs never occurs. The ordering is a property of a small fast fixture
workspace, not of the product — the real workspace produces it readily.

So the e2e spec is relabelled a smoke leg and says plainly, in its own docblock,
that it does not guard this bug and which test does.

The guard is a jsdom test that drives the two `/me` resolutions by hand, which
makes the sequence deterministic rather than dependent on fixture speed. Run
against the unfixed page it FAILS; against the fixed page it passes.

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm

* fix(web): tell "membership not fetched yet" apart from "no access" (BUG-2978, codex round 1)

The sticky permission cache from e6ed6fdd updated only when
`currentMembership !== null`, and null means BOTH "not fetched yet" and "no
access". So the cache held the last good answer forever once the answer became
a denial: an owner removed from the workspace, or a `/me` that 403s, kept the
Danger Zone tab, the Save buttons and the delete controls on screen
indefinitely. Server enforcement meant it was stale UI rather than an escalation
— which is why it is a behaviour regression I introduced and not a hole that was
already there.

The store now says which of the two a null is. `membershipKnown` is false while
a membership fetch is in flight and true once one has settled either way,
including the 403 path and the workspace-did-not-resolve path. The settings page
gates its cache on that instead, so the cache still covers exactly the in-flight
window it was written for and nothing else.

Second test leg, and it is the complement rather than a repeat: it drives `/me`
to REJECT and asserts the owner-only tab leaves. Each leg fails against the
code it guards and passes against the other — leg one fails on the pre-fix page,
leg two fails on e6ed6fdd's non-null gate. Verified by running all three
versions, not by reading them.

Dropped `vi.resetModules()` from the test's afterEach: it gave the second test a
fresh module graph including a second copy of the Svelte runtime, whose
`$effect` did not recognise the first copy's component context, and the remount
died with `effect_orphan`.

Found by Codex review round 1 (P2 + nit).

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm

* 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

* 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

* fix(web): observe the sequence token at entry, claim it on success (BUG-2978, codex round 4)

Moving the sequence claim after the create call in d7ccaeaf fixed the failure
path and opened an ordering hole: a create that STARTED EARLIER but resolved
later would override a `setCurrent` the user began in between, switching the
store out from under a navigation that was the newer intent. The comment I wrote
in that commit claimed the opposite, which is the part worth flagging — the
prose asserted the property the code had just stopped having.

Both obvious spellings get one ordering wrong. Claiming at entry (the original)
makes a FAILED create invalidate an in-flight `setCurrent`, whose writes are then
discarded on the seq check with nothing left to resolve membership. Claiming
only after the call is the hole above.

Reading the token at entry and comparing before claiming gives all three
orderings: a navigation started after this create wins, a navigation still in
flight from before loses to it, and a failed create claims nothing so it
invalidates nothing.

New leg for the ordering itself, mutation-tested: restoring the claim-after-call
spelling fails exactly that leg and leaves the other five green.

Two nits from the same round: the top-level comment still said every create exit
settles the flag "including a create that threw", which stopped being true when
failed creates went back to changing no state; and the failed-create leg's title
claimed the current workspace was untouched while asserting only the permission,
so it now asserts the workspace identity too.

Found by Codex review round 4 (P2 + two nits).

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm

* fix(web): a workspace that was created belongs in the list, whoever wins the selection race (BUG-2978, codex round 5)

Two concurrent creates both succeed on the server, so both workspaces exist —
but only one can be selected, and every spelling of this token protocol so far
dropped the other from `workspaces` entirely, differing only in which one. The
entry-claim original dropped the earlier-STARTED create's workspace; round 4's
version dropped the later-COMPLETING one. Neither loss was chosen by anyone, and
the visible symptom is the same either way: a workspace the user just created is
invisible until the next `loadAll`.

The list is additive, so the append moves ahead of the token check and no longer
depends on the race at all. Only the SELECTION is raced, and that stays
first-to-complete and is now documented as deliberately unspecified beyond
that — with two creates in flight there is no intent to honour, and the part a
user would notice missing no longer hangs on the answer.

Mutation-tested: moving the append back behind the token check fails exactly the
new leg and leaves the other six green.

Nit from the same round: the `membershipSeq` comment said every setCurrent and
create call increments it, which stopped being true when create moved to
observing the token at entry. It now says which call claims and when.

The remaining round-5 nit is 4a1bfd2a's commit message, which still carries the
"false only while a membership fetch is in flight" overstatement. A pushed
commit message cannot be edited; d7ccaeaf records the correction and the PR body
carries it where a reader will actually meet it.

Found by Codex review round 5 (P2 + nit).

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm

* docs(web): correct two store comments, and record what round 6 found out of scope (BUG-2978)

Two comment corrections, both places where the prose was wider than the code:

- `membershipKnown` is settled by membership RESOLUTION, not by a membership
  fetch. A workspace that does not resolve at all settles it without any `/me`
  request being made, which the old wording excluded.
- `recoverIfMissing` credited `loading` with preventing a duplicate list call.
  The actual guard is `inFlightFor`, which JOINS the run already in flight;
  `loading` is a rendering signal and is not consulted for that.

Round 6's P2 is filed as BUG-2981 rather than fixed here: a `loadAll` that
STARTS before a `create` completes commits a server list that legitimately
predates the new workspace, replacing the array and erasing the append. It is
PRE-EXISTING — verified against origin/main, where `create` does the same
wholesale append into the same array — and this branch only changed when that
append happens relative to the membership token. It is a different operation
pair, its fix contains a real design choice about what a list snapshot older
than a create means, and this is a settings-tab fix already five rounds deep.
Widening it there is how a small fix acquires a large blast radius.

Found by Codex review round 6 (two nits; its P2 filed).

Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm
This commit is contained in:
xarmian
2026-09-09 16:23:59 -04:00
committed by GitHub
parent f262449b18
commit 06ccabddb0
5 changed files with 546 additions and 22 deletions
@@ -0,0 +1,98 @@
import { expect, type Page } from '@playwright/test';
import { test } from './fixtures';
/**
* BUG-2978 — deep-linking `/{user}/{ws}/settings#danger` landed on General for
* a workspace OWNER: 0/10 loads before the fix, while `#storage` and
* `#members` were 10/10.
*
* Mechanism, measured rather than guessed (the trail carries the probe). The
* hash-restoration effect works: it applied `danger` correctly ~219ms in. What
* broke it is that `workspaceStore.setCurrent` clears `currentMembership` to
* null before `/me` resolves, and the permission helpers treat unknown as
* no-access by design — and this route calls `setCurrent` TWICE per load, once
* from the workspace layout and once from the page's own `load()`. So
* `canEditWorkspace` reads true -> false -> true, the owner-only tab leaves the
* tab set during the false window, the effect's snap-back branch moves
* `activeTab` off the now-invalid `danger`, and `pendingHash` has already been
* consumed — so nothing restores it when the permission comes back.
*
* Only the owner-only tab could hit this, which is why `#storage` never did:
* an always-valid tab is never snapped away from.
*
* WHAT THIS SPEC IS AND IS NOT. It is a smoke leg: it proves that deep-linking
* the two tabs it covers — the owner-only one and an always-visible control —
* works in a real browser. It is NOT the regression guard for
* BUG-2978, because it does not discriminate — RUN AGAINST THE UNFIXED BUILD IT
* PASSES. On the e2e fixture the layout's `setCurrent` and the page's own land
* inside a single unresolved `/me` window, so membership never goes
* known -> unknown -> known and the flicker the bug needs never happens. That
* ordering is a property of a small, fast fixture workspace; the real workspace
* produces it readily (0/10 deep links before the fix, 40/40 after, measured on
* the trail).
*
* The discriminating test is
* `src/routes/[username]/[workspace]/settings/settingsPermissionFlicker.svelte.test.ts`,
* which drives the two `/me` resolutions by hand and therefore fails without
* the fix.
*
* TIMING STILL MATTERS HERE. The pre-fix failure is "correct, then reverted",
* so an auto-retrying assertion (`toPass`, or a bare `toHaveAttribute` with its
* default timeout) can observe the CORRECT intermediate state and pass even
* where the ordering does occur. Both legs therefore settle first, then assert
* once, and the owner leg asserts again after a further wait.
*/
const SETTLE_MS = 2500;
async function openSettings(page: Page, username: string, workspace: string, hash: string) {
await page.goto(`/${username}/${workspace}/settings${hash}`);
// The owner-only tab arrives with `/me`; waiting for the full set is what
// makes this a measurement of the settled tab bar rather than of the
// pre-permission one.
await expect(page.locator('.tab-bar .tab')).toHaveCount(5);
await page.waitForTimeout(SETTLE_MS);
}
function activeTabLabel(page: Page) {
return page.evaluate(() => {
const active = [...document.querySelectorAll('.tab')].find((t) =>
t.classList.contains('active'),
);
return (active?.textContent ?? '').trim();
});
}
test('BUG-2978 smoke: deep-linking the owner-only settings tab lands on it and stays', async ({
page,
fixture,
}) => {
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug, '#danger');
// Non-vacuous: the fixture user must actually be an owner, or "Danger Zone
// is not selected" would be the correct answer and the leg would pass for
// the wrong reason.
await expect(page.locator('.tab', { hasText: 'Danger Zone' })).toHaveCount(1);
expect(await activeTabLabel(page), 'deep-linked owner-only tab not selected').toContain(
'Danger Zone',
);
// The pre-fix build SELECTED it and then reverted, so hold and look again.
await page.waitForTimeout(1500);
expect(await activeTabLabel(page), 'owner-only tab was selected and then lost').toContain(
'Danger Zone',
);
});
test('BUG-2978 control: a tab valid without /me deep-links on both builds', async ({
page,
fixture,
}) => {
// Storage is visible to every member, so it is never removed from the tab
// set by the permission window. This leg passed before the fix too — it is
// here to show the failure was specific to the owner-only tab rather than
// to hash restoration in general.
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug, '#storage');
expect(await activeTabLabel(page)).toContain('Storage');
});
+102 -13
View File
@@ -6,13 +6,30 @@ import { createKeyedSingleFlight } from './singleFlight';
let workspaces = $state<Workspace[]>([]); let workspaces = $state<Workspace[]>([]);
let current = $state<Workspace | null>(null); let current = $state<Workspace | null>(null);
let currentMembership = $state<WorkspaceMembership | 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 from the moment a call that will replace membership begins — which
// includes resolving the workspace itself, and the part of a create that
// follows a successful API call — until that call settles: a fetched
// membership, a 403, or a workspace that did not resolve.
//
// A create that THROWS is outside all of that. It changes no state at all,
// because a failed create says nothing about the workspace you are still
// looking at; whatever this flag was before such a create, it still is.
let membershipKnown = $state(false);
let loading = $state(false); let loading = $state(false);
// Monotonic sequence guarding async /me responses against navigation races. // Monotonic sequence guarding async /me responses against navigation races.
// Each setCurrent / create call increments the counter; a /me response is // A /me response is only applied if its captured token still matches at
// only applied if its captured token still matches at resolution time. This // resolution time, which prevents a slow /me for workspace A from clobbering a
// prevents a slow /me for workspace A from clobbering a freshly-set // freshly-set membership for workspace B.
// membership for workspace B. //
// Every `setCurrent` claims the token on entry. `create` OBSERVES it on entry
// and claims only once the workspace exists, so a create that fails or loses a
// selection race increments nothing — see the comment in `create` for the three
// orderings that shapes.
let membershipSeq = 0; let membershipSeq = 0;
// The keyed single-flight loader fencing `loadAll` (TASK-2947) — the same // The keyed single-flight loader fencing `loadAll` (TASK-2947) — the same
@@ -46,6 +63,9 @@ const loadAllFlight = createKeyedSingleFlight<string>({
* item grant is less permissive. `currentMembership` is null when not loaded * item grant is less permissive. `currentMembership` is null when not loaded
* yet or the fetch failed; in that case all helpers return false (treat * yet or the fetch failed; in that case all helpers return false (treat
* unknown as no access). * unknown as no access).
*
* That conflation is safe for gating an affordance and NOT safe for caching one:
* see `membershipKnown` below, which is what tells the two apart.
*/ */
export const workspaceStore = { export const workspaceStore = {
@@ -55,6 +75,24 @@ export const workspaceStore = {
get currentMembership() { return currentMembership; }, get currentMembership() { return currentMembership; },
/**
* True once membership RESOLUTION has settled for the current workspace, so
* a null `currentMembership` means "no access" rather than "not yet loaded".
* Resolution, not fetch: a workspace that does not resolve at all settles
* this without any `/me` request being made.
*
* 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
* boundary either way, but the UI should not lie.
*/
get membershipKnown() { return membershipKnown; },
get currentRole() { get currentRole() {
return currentMembership?.role ?? null; return currentMembership?.role ?? null;
}, },
@@ -126,8 +164,9 @@ export const workspaceStore = {
* gate, and it is cheap: two reads, both false on a healthy session. * gate, and it is cheap: two reads, both false on a healthy session.
* *
* IDEMPOTENT AND SELF-LIMITING. When identity is intact this does nothing * IDEMPOTENT AND SELF-LIMITING. When identity is intact this does nothing
* and issues no request. `loading` keeps it from stacking a second list call * and issues no request. A concurrent list call is not duplicated either:
* on an in-flight one. * `inFlightFor` JOINS the one already running — `loading` is a rendering
* signal here and is not consulted for that.
*/ */
async recoverIfMissing(ws: string): Promise<void> { async recoverIfMissing(ws: string): Promise<void> {
if (workspaces.length === 0) { if (workspaces.length === 0) {
@@ -174,6 +213,7 @@ export const workspaceStore = {
// Clear stale membership immediately so helpers don't briefly answer // Clear stale membership immediately so helpers don't briefly answer
// "yes" using the previous workspace's grants while /me is in flight. // "yes" using the previous workspace's grants while /me is in flight.
currentMembership = null; currentMembership = null;
membershipKnown = false;
// Resolve the workspace itself. Membership is fetched once we know // Resolve the workspace itself. Membership is fetched once we know
// the slug. // the slug.
@@ -206,26 +246,75 @@ export const workspaceStore = {
if (resolved) { if (resolved) {
try { try {
const m = await api.workspaces.me(slug); const m = await api.workspaces.me(slug);
if (seq === membershipSeq) currentMembership = m; if (seq === membershipSeq) { currentMembership = m; membershipKnown = true; }
} catch { } catch {
if (seq === membershipSeq) currentMembership = null; // A 403 or a removed member is an ANSWER, not a pending state.
if (seq === membershipSeq) { currentMembership = null; membershipKnown = true; }
} }
} else if (seq === membershipSeq) {
// The workspace itself did not resolve (404, or no access): also an
// answer, and the same one.
membershipKnown = true;
} }
}, },
async create(data: { name: string; description?: string; template?: string }) { 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.
//
// OBSERVE the sequence token at entry, CLAIM it only on success (codex
// round 4). Three orderings have to come out right and the obvious two
// spellings each get one wrong:
//
// - claiming at entry (the original) makes a FAILED create invalidate a
// `setCurrent` that is still in flight — its writes are discarded on
// the seq check and membership is left unresolved with nothing coming
// to fix it;
// - claiming only after the call lets a create that STARTED EARLIER but
// resolved later override a navigation the user began in between.
//
// Reading the token at entry and comparing before claiming gives all
// three: a navigation started after this create wins (it bumped the
// token), a navigation still in flight from before loses (this create is
// the newer intent), and a failed create claims nothing and therefore
// invalidates nothing.
const entrySeq = membershipSeq;
const ws = await api.workspaces.create(data);
// THE LIST IS ADDITIVE; ONLY THE SELECTION IS RACED (codex round 5).
// Two concurrent creates both succeed on the server, so both workspaces
// exist and both belong in `workspaces` — but only one can be the
// selected one. Appending before the token check means the loser of the
// selection race is still listed rather than invisible until the next
// `loadAll`. That loss predates this change: the entry-claim spelling
// dropped the EARLIER-started create's workspace, this one would have
// dropped the later-COMPLETING one, and neither is a loss anyone chose.
//
// Which create ends up SELECTED is first-to-complete, and is left
// deliberately unspecified beyond that: with two creates in flight there
// is no intent to honour, and the list — the part a user would notice
// missing — no longer depends on the answer.
workspaces = [...workspaces, ws];
if (membershipSeq !== entrySeq) return ws;
const seq = ++membershipSeq; const seq = ++membershipSeq;
currentMembership = null; currentMembership = null;
const ws = await api.workspaces.create(data); membershipKnown = false;
if (seq !== membershipSeq) return ws;
workspaces = [...workspaces, ws];
current = ws; current = ws;
// New workspace — refresh membership for the just-created context. // New workspace — refresh membership for the just-created context.
try { try {
const m = await api.workspaces.me(ws.slug); const m = await api.workspaces.me(ws.slug);
if (seq === membershipSeq) currentMembership = m; if (seq === membershipSeq) { currentMembership = m; membershipKnown = true; }
} catch { } catch {
if (seq === membershipSeq) currentMembership = null; if (seq === membershipSeq) { currentMembership = null; membershipKnown = true; }
} }
return ws; return ws;
} }
@@ -0,0 +1,156 @@
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 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');
let releaseGet: (v: unknown) => void = () => {};
api.workspaces.get.mockReturnValue(new Promise((r) => { releaseGet = r; }));
api.workspaces.me.mockResolvedValue(OWNER);
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);
// "Untouched" means the workspace identity too, not just the permission
// (codex round 4 — the title claimed more than the assertions did).
expect(workspaceStore.current?.slug).toBe('ws');
});
it('does not let a create override a navigation the user started after it', async () => {
// Ordering, not just settling (codex round 4). The create's API call is
// slow; a `setCurrent` begins while it is pending and resolves first.
// The navigation is the newer intent and must win — an earlier draft
// claimed the sequence token only after the create returned, which let
// the create switch the store out from under it.
const { workspaceStore } = await import('./workspace.svelte');
let releaseCreate: (v: unknown) => void = () => {};
api.workspaces.create.mockReturnValue(new Promise((r) => { releaseCreate = r; }));
api.workspaces.get.mockResolvedValue({ id: 'w2', slug: 'later', name: 'Later' });
api.workspaces.me.mockResolvedValue(OWNER);
const creating = workspaceStore.create({ name: 'New' });
await workspaceStore.setCurrent('later');
expect(workspaceStore.current?.slug).toBe('later');
releaseCreate({ id: 'w3', slug: 'created', name: 'Created' });
await creating;
expect(workspaceStore.current?.slug).toBe('later');
expect(workspaceStore.membershipKnown).toBe(true);
});
it('keeps BOTH workspaces when two creates race', async () => {
// Codex round 5. Both creates succeed on the server, so both workspaces
// exist; only one can be selected. The loser of the selection race must
// still be in the list — otherwise a workspace the user just created is
// invisible until the next loadAll. Every earlier spelling of this
// protocol dropped one of them, differing only in which.
const { workspaceStore } = await import('./workspace.svelte');
api.workspaces.me.mockResolvedValue(OWNER);
let releaseA: (v: unknown) => void = () => {};
let releaseB: (v: unknown) => void = () => {};
api.workspaces.create
.mockReturnValueOnce(new Promise((r) => { releaseA = r; }))
.mockReturnValueOnce(new Promise((r) => { releaseB = r; }));
const a = workspaceStore.create({ name: 'A' });
const b = workspaceStore.create({ name: 'B' });
releaseA({ id: 'wa', slug: 'a', name: 'A' });
await a;
releaseB({ id: 'wb', slug: 'b', name: 'B' });
await b;
const slugs = workspaceStore.workspaces.map((w) => w.slug);
expect(slugs).toContain('a');
expect(slugs).toContain('b');
});
});
@@ -84,11 +84,69 @@
{ id: 'storage', label: 'Storage', icon: '\uD83D\uDCBE', ownerOnly: false }, { id: 'storage', label: 'Storage', icon: '\uD83D\uDCBE', ownerOnly: false },
{ id: 'danger', label: 'Danger Zone', icon: '\u26A0\uFE0F', ownerOnly: true }, { id: 'danger', label: 'Danger Zone', icon: '\u26A0\uFE0F', ownerOnly: true },
]; ];
let tabs = $derived(allTabs.filter(t => !t.ownerOnly || workspaceStore.canEditWorkspace)); // BUG-2978: gate the owner-only tab on a STICKY read of the permission, not
// on `workspaceStore.canEditWorkspace` directly.
//
// `setCurrent` clears `currentMembership` to null before `/me` resolves, and
// the permission helpers treat unknown as no-access by design. This route
// calls it twice per load — once from the workspace layout, once from this
// page's own `load()` — so `canEditWorkspace` reads true -> false -> true on
// an ordinary owner page load. Measured on the trail: the effect below
// applied `#danger` correctly at 219ms and the false window at 244ms snapped
// it back to General, with `pendingHash` already consumed, so the tab was
// lost for good and 0/10 deep links landed.
//
// Same two-effect shape the dashboard uses for its owner-gated CTA, and for
// the same reason (CONVE-606: reset on a real workspace switch, update only
// when membership is definitively known). Default false so owner-only chrome
// never flashes before `/me` confirms; the server-side owner check remains
// the enforcement boundary, this is a stability fix.
let canEditWs = $state(false);
// Same treatment, same reason: read straight from the store these flip false
// during that window too, so on an ordinary owner load the Save buttons, the
// invite form and the delete controls go readonly and then come back.
let isOwner = $state(false);
let canExport = $state(false);
let lastPermSlug: string | null = null;
$effect(() => {
if (wsSlug !== lastPermSlug) {
lastPermSlug = wsSlug;
// ONE reset for every sticky permission on this page. A second
// effect testing the same `wsSlug !== lastPermSlug` could never
// fire — whichever ran first would have already updated the marker.
canEditWs = false;
isOwner = false;
canExport = false;
}
});
$effect(() => {
// Read through the store's getters rather than re-deriving the cascade —
// they mirror the server's ResolveUserPermission and must not be forked.
//
// Gated on `membershipKnown`, NOT on `currentMembership !== null` (codex
// round 1). Null means both "not fetched yet" and "no access", and gating
// 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 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;
// Editor-or-owner predicate for affordances outside the strict
// owner-only line (Export bundle is a read-side action gated to
// editor+ per project policy).
const role = workspaceStore.currentRole;
canExport = role === 'owner' || role === 'editor';
}
});
let tabs = $derived(allTabs.filter(t => !t.ownerOnly || canEditWs));
let validTabIds = $derived(tabs.map(t => t.id)); let validTabIds = $derived(tabs.map(t => t.id));
// Hash-driven tab restoration. The hash is captured once on mount, but // 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 // which arrives async from /me). So we re-evaluate when validTabIds
// expands \u2014 otherwise an owner deep-linking to #danger lands on // expands \u2014 otherwise an owner deep-linking to #danger lands on
// General because /me hadn't loaded yet at mount time. // General because /me hadn't loaded yet at mount time.
@@ -421,13 +479,6 @@
} }
} }
let isOwner = $derived(workspaceStore.isOwner);
// Editor-or-owner predicate for affordances that fall outside the
// strict canEditWorkspace owner-only line (e.g. Export bundle is a
// read-side action that we still gate to editor+ per project policy).
let canExport = $derived(
workspaceStore.currentRole === 'owner' || workspaceStore.currentRole === 'editor'
);
let confirmDelete = $state(false); let confirmDelete = $state(false);
let deleting = $state(false); let deleting = $state(false);
@@ -0,0 +1,130 @@
import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest';
import { render, screen, waitFor } from '@testing-library/svelte';
import { page } from '$app/state';
import SettingsPage from './+page.svelte';
import { workspaceStore } from '$lib/stores/workspace.svelte';
/**
* BUG-2978 the owner-only settings tab was lost whenever the workspace
* permission went known -> unknown -> known.
*
* `workspaceStore.setCurrent` clears `currentMembership` to null before `/me`
* resolves, and the permission helpers treat unknown as no-access by design.
* The settings route calls `setCurrent` twice per load (workspace layout, then
* the page's own `load()`), so an owner sees `canEditWorkspace` read
* true -> false -> true. During the false window the Danger Zone tab left the
* tab set, the hash-restoration effect's snap-back moved `activeTab` off it,
* and `pendingHash` had already been consumed so nothing restored it.
*
* This test drives that sequence DETERMINISTICALLY by controlling when each
* `/me` resolves, which is the reason it lives here rather than in e2e: on the
* e2e fixture both `setCurrent` calls land inside a single unresolved window,
* so the flicker never occurs and the spec there passes on a broken build. The
* browser measurement against a real workspace (0/10 deep links before, 40/40
* after) is on the trail; this is the part that fails in CI without the fix.
*/
const meCalls: Array<(value: unknown) => void> = [];
const meRejects: Array<(reason: unknown) => void> = [];
vi.mock('$lib/api/client', () => ({
api: {
workspaces: {
get: vi.fn(async () => ({ id: 'ws1', slug: 'ws', name: 'WS', context: {} })),
me: vi.fn(() => new Promise((resolve, reject) => {
meCalls.push(resolve);
meRejects.push(reject);
})),
list: vi.fn(async () => []),
},
collections: { list: vi.fn(async () => []) },
members: { list: vi.fn(async () => ({ members: [], invitations: [] })) },
},
isPlanLimitError: () => false,
planLimitMessage: () => '',
PadApiError: class extends Error {},
}));
vi.mock('$lib/services/sse.svelte', () => ({
sseService: { onItemEvent: () => () => {} },
}));
const OWNER = { role: 'owner', collection_grants: [], item_grants: [] };
/** Resolve the Nth outstanding `/me`, waiting for it to have been issued. */
async function resolveMe(index: number, value: unknown) {
await waitFor(() => expect(meCalls.length).toBeGreaterThan(index));
meCalls[index](value);
}
describe('BUG-2978: settings permissions survive the /me window', () => {
beforeEach(() => {
meCalls.length = 0;
meRejects.length = 0;
page.params = { username: 'dave', workspace: 'ws' };
window.location.hash = '#danger';
});
afterEach(() => {
window.location.hash = '';
// Deliberately NO `vi.resetModules()`: it hands the second test a fresh
// module graph including a second copy of the Svelte runtime, whose
// `$effect` does not recognise the first copy's component context, and
// the remount dies with `effect_orphan`. The store is a module-scoped
// singleton, and each test re-establishes its state through `setCurrent`.
});
it('keeps the deep-linked owner-only tab selected when membership goes known -> unknown -> known', async () => {
render(SettingsPage);
// First /me resolves as owner: the Danger Zone tab appears and the
// pending hash is applied.
await resolveMe(0, OWNER);
await waitFor(() => {
expect(screen.getByRole('tab', { name: /Danger Zone/ })).toHaveAttribute(
'aria-selected',
'true',
);
});
// A SECOND setCurrent — what the page's own load() does after the
// layout's — clears membership to null before its /me resolves. This is
// the window the bug lived in.
const second = workspaceStore.setCurrent('ws');
await waitFor(() => expect(workspaceStore.currentMembership).toBeNull());
await resolveMe(1, OWNER);
await second;
// Non-vacuity: the tab must still exist, or "not selected" would be the
// correct answer and this assertion would prove nothing.
const danger = await screen.findByRole('tab', { name: /Danger Zone/ });
expect(danger).toHaveAttribute('aria-selected', 'true');
});
it('drops owner-only chrome when membership becomes a definitive denial', async () => {
// The complement of the test above, and the reason the cache is gated on
// `membershipKnown` rather than on `currentMembership !== null` (codex
// round 1): null means BOTH "not fetched yet" and "no access". A cache
// that ignores every null holds the last good answer forever, so an owner
// removed from the workspace — or a `/me` that 403s — would keep the
// owner-only tab and the delete controls on screen indefinitely.
render(SettingsPage);
await resolveMe(0, OWNER);
await waitFor(() => {
expect(screen.getByRole('tab', { name: /Danger Zone/ })).toBeInTheDocument();
});
// Now the answer changes to "no access", delivered the way the store
// delivers it: a rejected `/me`, which leaves membership null.
const second = workspaceStore.setCurrent('ws');
await waitFor(() => expect(meCalls.length).toBeGreaterThan(1));
meRejects[1]?.(new Error('403'));
await second;
await waitFor(() => {
expect(screen.queryByRole('tab', { name: /Danger Zone/ })).not.toBeInTheDocument();
});
});
});