diff --git a/web/e2e/permission-walk-workspace.spec.ts b/web/e2e/permission-walk-workspace.spec.ts index f3c733178..777a63967 100644 --- a/web/e2e/permission-walk-workspace.spec.ts +++ b/web/e2e/permission-walk-workspace.spec.ts @@ -109,9 +109,7 @@ for (const key of MEMBERS) { }); test(`${key}: General name field holds the workspace name`, async ({ page }) => { - // BUG-3260: on a direct load the field is empty for every member. - // Remove once the fix lands. - test.fail(true, 'BUG-3260'); + // BUG-3260: a direct load used to leave this empty for every member. await openSettingsTab(page, key, 'general'); await expect(page.locator('#ws-name')).toHaveValue(/^Walk /); }); diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index 49f446fef..9ce987735 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -290,6 +290,8 @@ untrack(() => { wsName = ''; contextEditor = ''; + nameEdited = false; + contextEdited = false; collections = []; members = []; invitations = []; @@ -297,6 +299,31 @@ }); }); + // The name and context fields are SEEDED FROM THE STORE, reactively, not + // snapshotted inside load() (BUG-3260). load() used to read + // `workspaceStore.current` right after `await setCurrent(slug)`. But a + // setCurrent that a concurrent call supersedes (the layout makes one for + // the same slug on every cold load) returns BEFORE it writes `current`. So + // the snapshot read an unset store: an empty name, and a `{}` context that + // saveContext would then have written back as "no context". + // + // Seeded only once the store has SETTLED for this user + // (`membershipKnown`) and holds this workspace, so a previous identity's + // workspace is never shown. Re-seeded whenever `current` changes, unless + // the user has typed in that field since the page's (user, workspace) key + // last changed. + let nameEdited = false; + let contextEdited = false; + $effect(() => { + void sessionUserId; + const cur = workspaceStore.current; + if (!workspaceStore.membershipKnown || !cur || cur.slug !== wsSlug) return; + untrack(() => { + if (!nameEdited) wsName = cur.name; + if (!contextEdited) contextEditor = formatContextEditor(cur.context); + }); + }); + // BUG-2265: keep the collections list fresh when another client changes a // collection, so opening the edit modal seeds a CURRENT // expected_updated_at instead of a stale one. UNIFIED generation (Codex): @@ -395,8 +422,8 @@ try { await workspaceStore.setCurrent(slug); if (myLoad !== loadGen) return; - wsName = workspaceStore.current?.name ?? ''; - contextEditor = JSON.stringify(workspaceStore.current?.context ?? {}, null, 2); + // The name and context are seeded by the effect after the load key + // (BUG-3260), not read here: `current` may not be set yet. const fresh = await api.collections.list(slug); if (myLoad !== loadGen) return; if (myColl === collectionsGen) collections = fresh; @@ -447,12 +474,14 @@ } function resetContextEditor() { + contextEdited = false; contextEditor = formatContextEditor(workspaceStore.current?.context); contextError = ''; contextStatus = 'idle'; } function clearContextEditor() { + contextEdited = true; contextEditor = '{}'; contextError = ''; contextStatus = 'idle'; @@ -853,6 +882,7 @@ id="ws-name" type="text" bind:value={wsName} + oninput={() => (nameEdited = true)} readonly={!isOwner} onkeydown={(e) => isOwner && e.key === 'Enter' && saveName()} /> @@ -924,6 +954,7 @@ id="workspace-context" class="context-editor mono" bind:value={contextEditor} + oninput={() => (contextEdited = true)} spellcheck="false" readonly={!isOwner} rows="18" diff --git a/web/src/routes/[username]/[workspace]/settings/bug3260NameOnColdLoad.svelte.test.ts b/web/src/routes/[username]/[workspace]/settings/bug3260NameOnColdLoad.svelte.test.ts new file mode 100644 index 000000000..ccff564b3 --- /dev/null +++ b/web/src/routes/[username]/[workspace]/settings/bug3260NameOnColdLoad.svelte.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { cleanup, render, screen, waitFor } from '@testing-library/svelte'; +import { flushSync } from 'svelte'; +import { page } from '$app/state'; +import SettingsPage from './+page.svelte'; +import { api } from '$lib/api/client'; +import { workspaceStore } from '$lib/stores/workspace.svelte'; +import { + bindReactiveEpoch, + bindReactiveUserId, + isEpochReactive, + isUserIdReactive, +} from '../../../../test/identityEpochMock.svelte'; + +/** + * BUG-3260: Settings -> General showed an EMPTY workspace name on a direct load + * or reload (measured in e2e, owner included), while in-app navigation filled + * it. This suite reproduces the load orders in isolation. + * + * MECHANISM (traced in e2e): on a cold load the page's load() and the layout + * both call workspaceStore.setCurrent for the same slug at once. The page's + * call is superseded: it returns before `current` is written, because + * setCurrent drops a stale call's writes. load() then read `current` while it + * was still unset, and seeded the name as "" and the context as {}. + * Nothing wrote either again. + * + * CONTROL: an uncontended load, as after in-app navigation, when `current` is + * already this workspace. + * DEFECT LEG: the page's setCurrent is superseded by a concurrent one, and + * the superseded call's fetch answers first. + * A leg where the user id arrives late is kept: it was the first hypothesis, + * and it does not reproduce. + */ + +type Deferred = { promise: Promise; resolve: (v: T) => void }; +function deferred(): Deferred { + let resolve!: (v: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +/** Outstanding members.list calls, in call order, with the slug each asked for. */ +const memberLists: Array<{ slug: string; d: Deferred }> = []; +const copies = vi.hoisted(() => [] as string[]); + +function membersFor(who: string) { + return { + members: [{ user_id: `m-${who}`, user_name: `Member ${who}`, user_email: `${who}@example.com`, role: 'editor' }], + invitations: [ + { id: `inv-${who}`, email: `invitee-of-${who}@example.com`, role: 'editor', code: `code-${who}`, join_url: `https://pad.test/join/${who}` }, + ], + }; +} + +vi.mock('$lib/utils/clipboard', () => ({ + copyToClipboard: async (s: string) => { + copies.push(s); + return true; + }, +})); + +vi.mock('$lib/stores/toast.svelte', () => ({ + toastStore: { show: () => 'toast-id', dismiss: () => {}, get toasts() { return []; } }, + quietExternalToasts: () => false, +})); + +vi.mock('$lib/api/client', () => ({ + api: { + workspaces: { + get: vi.fn(async (slug: string) => ({ id: `id-${slug}`, slug, name: slug.toUpperCase(), context: {} })), + me: vi.fn(async () => ({ role: 'owner', collection_grants: [], item_grants: [] })), + list: vi.fn(async () => []), + update: vi.fn(async () => ({})), + delete: vi.fn(async () => ({})), + restore: vi.fn(async () => ({})), + }, + collections: { list: vi.fn(async () => []) }, + members: { + list: vi.fn(async () => ({ members: [], invitations: [] })), + getMemberCollectionAccess: vi.fn(async () => ({ collection_access: 'all', collection_ids: [] })), + setMemberCollectionAccess: vi.fn(async () => ({})), + remove: vi.fn(async () => ({})), + }, + }, + isPlanLimitError: () => false, + planLimitMessage: () => '', + PadApiError: class extends Error {}, +})); + +vi.mock('$app/state', async () => ({ page: (await import('../../../../test/mocks/reactivePage.svelte')).page })); +vi.mock('$lib/services/sse.svelte', () => ({ sseService: { onItemEvent: () => () => {} } })); +vi.mock('$app/navigation', () => ({ goto: () => Promise.resolve() })); + +const auth = vi.hoisted(() => { + const hook = { + read: null as null | (() => number), + write: null as null | ((n: number) => void), + readUser: null as null | (() => string), + writeUser: null as null | ((id: string) => void), + }; + let fallbackEpoch = 0; + let fallbackUser = 'u1'; + const getEpoch = () => (hook.read ? hook.read() : fallbackEpoch); + const setEpoch = (n: number) => (hook.write ? hook.write(n) : (fallbackEpoch = n)); + const getUser = () => (hook.readUser ? hook.readUser() : fallbackUser); + const setUser = (id: string) => (hook.writeUser ? hook.writeUser(id) : (fallbackUser = id)); + return { + __hook: hook, + get identityEpoch() { return getEpoch(); }, + get userId() { return getUser(); }, + setUser, + bumpEpoch() { setEpoch(getEpoch() + 1); }, + /** A real identity change: user id and epoch move in the same step. */ + swapTo(id: string) { + setUser(id); + setEpoch(getEpoch() + 1); + }, + reset() { + setUser('u1'); + setEpoch(0); + }, + identityFence() { + const captured = getEpoch(); + return () => getEpoch() === captured; + }, + onIdentityChange() { return () => {}; }, + clear() {}, + }; +}); + +vi.mock('$lib/stores/auth.svelte', () => ({ authStore: auth })); + +bindReactiveEpoch(auth.__hook); +bindReactiveUserId(auth.__hook); + + +function nameField(): HTMLInputElement | null { + return document.querySelector('#ws-name'); +} + +describe('BUG-3260: the General name field holds the workspace name however the page was reached', () => { + beforeEach(() => { + memberLists.length = 0; + copies.length = 0; + auth.reset(); + page.params = { username: 'dave', workspace: 'ws' }; + window.location.hash = ''; + }); + + afterEach(() => { + cleanup(); + window.location.hash = ''; + }); + + it('CONTROL: user known before mount (in-app navigation)', async () => { + render(SettingsPage); + await waitFor(() => expect(nameField()?.value).toBe('WS')); + }); + + it('cold load: the page mounts before the session user is known, and the user arrives after', async () => { + auth.setUser(''); + render(SettingsPage); + await waitFor(() => expect(nameField()).not.toBeNull()); + auth.swapTo('u1'); + flushSync(); + await waitFor(() => expect(nameField()?.value).toBe('WS')); + }); + + it('DEFECT: the page setCurrent is superseded by a concurrent one (the layout call), and its fetch answers first', async () => { + const get = vi.mocked(api.workspaces.get); + const first = deferred(); + const second = deferred(); + // Any call beyond the two this leg controls answers with a name that + // cannot be mistaken for the fix working. + get.mockImplementation(async () => ({ id: 'id-ws', slug: 'ws', name: 'UNPLANNED', context: {} }) as never); + get.mockImplementationOnce(() => first.promise as never); + get.mockImplementationOnce(() => second.promise as never); + + render(SettingsPage); // load() -> setCurrent('ws') -> get #1, pending + await waitFor(() => expect(get).toHaveBeenCalledTimes(1)); + void workspaceStore.setCurrent('ws'); // the layout's call -> get #2; supersedes #1 + await waitFor(() => expect(get).toHaveBeenCalledTimes(2)); + + first.resolve({ id: 'id-ws', slug: 'ws', name: 'Real Name', context: { repo: 'x' } }); + // The superseded call's continuation, and load() after it, run to + // completion before the winning call's fetch lands, as traced in e2e. + await new Promise((r) => setTimeout(r, 0)); + await waitFor(() => expect(vi.mocked(api.members.list)).toHaveBeenCalled()); + second.resolve({ id: 'id-ws', slug: 'ws', name: 'Real Name', context: { repo: 'x' } }); + + // 'Real Name' comes only from these two answers. The store is a module + // singleton, so an earlier leg's workspace ('WS') can still be current + // here; a load that read the store at the wrong moment shows that + // instead, or '' on a real cold load. + await waitFor(() => expect(nameField()?.value).toBe('Real Name')); + const ctx = document.querySelector('#workspace-context'); + await waitFor(() => expect(ctx?.value ?? '').toContain('"repo"')); + }); +}); diff --git a/web/src/routes/[username]/[workspace]/settings/settingsIdentityGate.test.ts b/web/src/routes/[username]/[workspace]/settings/settingsIdentityGate.test.ts index 2e4f80b1a..56c5e0495 100644 --- a/web/src/routes/[username]/[workspace]/settings/settingsIdentityGate.test.ts +++ b/web/src/routes/[username]/[workspace]/settings/settingsIdentityGate.test.ts @@ -16,8 +16,8 @@ identityGateSuite({ asyncFunctions: { refreshCollections: { reviewed: '99eebed18312', why: 'collectionsGen and the workspace, then identityHeld(epochAtEntry), before the commit; a failure commits nothing' }, load: { - reviewed: '1070cdd4b0df', - why: 'loadGen after each await (collectionsGen too for the collections write). It has no identity check, and it re-stamps identityEpochAtLoad BEFORE its awaits, where roles and library re-stamp after the data lands. The advisory list on TASK-3097 carries that', + reviewed: 'fedd925ecc86', + why: 'loadGen after each await (collectionsGen too for the collections write). It has no identity check, and it re-stamps identityEpochAtLoad BEFORE its awaits, where roles and library re-stamp after the data lands. The advisory list on TASK-3097 carries that. Re-reviewed for BUG-3260: it no longer writes the name or context (an effect seeds them from the settled store), so it commits strictly less than before', }, saveName: { reviewed: '9d02b62e22dd', why: `${ENTRY}; the finally clears the button's own busy flag unfenced, on purpose` }, saveContext: { reviewed: '74dda10addda', why: `${ENTRY}, including between the update and the shared-store setCurrent; the finally clears its own busy flag` },