diff --git a/web/e2e/bug-2978-settings-hash-deeplink.spec.ts b/web/e2e/bug-2978-settings-hash-deeplink.spec.ts new file mode 100644 index 00000000..c783e83c --- /dev/null +++ b/web/e2e/bug-2978-settings-hash-deeplink.spec.ts @@ -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'); +}); diff --git a/web/src/lib/stores/workspace.svelte.ts b/web/src/lib/stores/workspace.svelte.ts index 38343ea0..384effe5 100644 --- a/web/src/lib/stores/workspace.svelte.ts +++ b/web/src/lib/stores/workspace.svelte.ts @@ -6,13 +6,30 @@ import { createKeyedSingleFlight } from './singleFlight'; let workspaces = $state([]); let current = $state(null); let currentMembership = $state(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); // Monotonic sequence guarding async /me responses against navigation races. -// Each setCurrent / create call increments the counter; a /me response is -// only applied if its captured token still matches at resolution time. This -// prevents a slow /me for workspace A from clobbering a freshly-set -// membership for workspace B. +// A /me response is only applied if its captured token still matches at +// resolution time, which prevents a slow /me for workspace A from clobbering a +// freshly-set 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; // The keyed single-flight loader fencing `loadAll` (TASK-2947) — the same @@ -46,6 +63,9 @@ const loadAllFlight = createKeyedSingleFlight({ * item grant is less permissive. `currentMembership` is null when not loaded * yet or the fetch failed; in that case all helpers return false (treat * 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 = { @@ -55,6 +75,24 @@ export const workspaceStore = { 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() { return currentMembership?.role ?? null; }, @@ -126,8 +164,9 @@ export const workspaceStore = { * gate, and it is cheap: two reads, both false on a healthy session. * * IDEMPOTENT AND SELF-LIMITING. When identity is intact this does nothing - * and issues no request. `loading` keeps it from stacking a second list call - * on an in-flight one. + * and issues no request. A concurrent list call is not duplicated either: + * `inFlightFor` JOINS the one already running — `loading` is a rendering + * signal here and is not consulted for that. */ async recoverIfMissing(ws: string): Promise { if (workspaces.length === 0) { @@ -174,6 +213,7 @@ export const workspaceStore = { // Clear stale membership immediately so helpers don't briefly answer // "yes" using the previous workspace's grants while /me is in flight. currentMembership = null; + membershipKnown = false; // Resolve the workspace itself. Membership is fetched once we know // the slug. @@ -206,26 +246,75 @@ export const workspaceStore = { if (resolved) { try { const m = await api.workspaces.me(slug); - if (seq === membershipSeq) currentMembership = m; + if (seq === membershipSeq) { currentMembership = m; membershipKnown = true; } } 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 }) { + // 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; currentMembership = null; - const ws = await api.workspaces.create(data); - if (seq !== membershipSeq) return ws; - workspaces = [...workspaces, ws]; + membershipKnown = false; current = ws; // New workspace — refresh membership for the just-created context. try { const m = await api.workspaces.me(ws.slug); - if (seq === membershipSeq) currentMembership = m; + if (seq === membershipSeq) { currentMembership = m; membershipKnown = true; } } catch { - if (seq === membershipSeq) currentMembership = null; + if (seq === membershipSeq) { currentMembership = null; membershipKnown = true; } } return ws; } diff --git a/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts b/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts new file mode 100644 index 00000000..4cd3ddb1 --- /dev/null +++ b/web/src/lib/stores/workspaceMembershipKnown.svelte.test.ts @@ -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'); + }); +}); diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index efbe7ace..3370172f 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -84,11 +84,69 @@ { id: 'storage', label: 'Storage', icon: '\uD83D\uDCBE', ownerOnly: false }, { 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)); // 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. @@ -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 deleting = $state(false); diff --git a/web/src/routes/[username]/[workspace]/settings/settingsPermissionFlicker.svelte.test.ts b/web/src/routes/[username]/[workspace]/settings/settingsPermissionFlicker.svelte.test.ts new file mode 100644 index 00000000..23b96b2c --- /dev/null +++ b/web/src/routes/[username]/[workspace]/settings/settingsPermissionFlicker.svelte.test.ts @@ -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(); + }); + }); +});