diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index 628709130..959061579 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -231,6 +231,8 @@ untrack(() => { wsName = ''; contextEditor = ''; + nameEdited = false; + contextEdited = false; collections = []; members = []; invitations = []; @@ -238,6 +240,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): @@ -307,8 +334,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; @@ -345,12 +372,14 @@ } function resetContextEditor() { + contextEdited = false; contextEditor = formatContextEditor(workspaceStore.current?.context); contextError = ''; contextStatus = 'idle'; } function clearContextEditor() { + contextEdited = true; contextEditor = '{}'; contextError = ''; contextStatus = 'idle'; @@ -650,6 +679,7 @@ id="ws-name" type="text" bind:value={wsName} + oninput={() => (nameEdited = true)} readonly={!isOwner} onkeydown={(e) => isOwner && e.key === 'Enter' && saveName()} /> @@ -721,6 +751,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/test/identityEpochMock.svelte.ts b/web/src/test/identityEpochMock.svelte.ts new file mode 100644 index 000000000..220b5b340 --- /dev/null +++ b/web/src/test/identityEpochMock.svelte.ts @@ -0,0 +1,162 @@ +/** + * The family's fake `authStore` for identity-fence suites (BUG-3084). + * + * WHY THIS EXISTS, and it is not convenience. Each surface's suite grew its own + * double with `let epoch = 0` — a plain closure variable. That models the + * store's VALUES correctly and its REACTIVITY not at all, and the real + * `identityEpoch` is `$state`: reading it inside an `$effect` creates a + * DEPENDENCY, so an identity change re-runs that effect. + * + * A non-reactive double silently converts every "did this effect re-run?" + * question into "does this compile?". That is not a coverage gap; it is an + * instrument answering a different question than its name. It hid a real defect + * on the collection page through ten source assertions, ten behavioural legs, a + * 10/10 mutation matrix, four review rounds and two lead reads — the search + * dispatch effect re-armed its debounce with the previous user's typed text and + * captured the NEW epoch, so the fence inside the timer passed and the old + * query went to the server under the new identity. + * + * The general form, which is the part worth carrying to other families: + * + * A test double that models a dependency's VALUES but not its REACTIVITY + * turns every question about re-running into a question about compiling. + * Where the subject is a framework effect, the double's reactivity IS the + * thing under test. + * + * MECHANICS. `vi.hoisted` runs before the Svelte runtime is initialised, so + * `$state` cannot be declared inside a hoisted factory. This module declares it + * at module scope — legal in a `.svelte.ts` — and a suite binds its hoisted + * object to it with `bindReactiveEpoch`. Probed against a known answer before + * being relied on, rather than assumed to work. + */ + +import { flushSync } from 'svelte'; + +let epochSignal = $state(0); + +/** The reactive epoch. Reading this inside an `$effect` creates a dependency. */ +export function readEpoch(): number { + return epochSignal; +} + +export function writeEpoch(next: number): void { + epochSignal = next; +} + +export function bumpEpoch(): void { + epochSignal += 1; +} + +export function resetEpoch(): void { + epochSignal = 0; +} + +/** + * Bind a hoisted mock object's epoch hook to the reactive signal above. + * + * The hoisted object holds `{ read, write }` slots and delegates to them when + * they are set, falling back to a plain variable when they are not — so a suite + * that forgets to bind still runs, but its "did this re-run?" assertions are + * then measuring nothing. That is exactly the failure this module exists to + * remove, which is why `assertReactive` below is offered alongside it. + */ +export function bindReactiveEpoch(hook: { + read: null | (() => number); + write: null | ((n: number) => void); +}): void { + hook.read = readEpoch; + hook.write = writeEpoch; +} + +/** + * Proves the SUBJECT'S OWN epoch path is reactive. + * + * Takes the mock's getter and its bump, because the first version of this + * function did NOT: it read and wrote this module's signal directly, so it + * returned true even with both hooks disconnected and every PRECONDITION leg + * built on it passed against exactly the non-reactive double it existed to + * detect. Codex reproduced that. A guard that cannot fail for the case it is + * named after is the failure it is named after, wearing its own badge. + * + * Pass the values the PAGE will see: `() => authStore.identityEpoch` and + * `() => authStore.bumpEpoch()` from the suite's own mock. Nothing here + * references the module signal, so a suite that forgot `bindReactiveEpoch` + * reads its inert fallback and this returns false. + * + * Returns rather than throws, so the caller owns the assertion message. + */ +export function isEpochReactive(read: () => number, bump: () => void): boolean { + let runs = 0; + const stop = $effect.root(() => { + $effect(() => { + read(); + runs += 1; + }); + }); + flushSync(); + const baseline = runs; + const before = read(); + bump(); + flushSync(); + const moved = read() !== before; + const reran = runs > baseline; + stop(); + // Both halves, because either alone is satisfiable by a broken double: a + // bump that does not move the value proves nothing about tracking, and a + // re-run with an unmoved value would mean the effect depends on something + // else entirely. + return moved && reran; +} + +/** + * The reactive USER ID, for surfaces whose recovery is keyed on + * `authStore.userId` rather than driven by an `onIdentityChange` listener + * (settings, the dashboard: a `(sessionUserId, wsSlug)` effect drops the data + * and reloads). The real getter reads the `session` `$state`, so it is a + * dependency; a suite whose double returns a plain variable turns "did the + * keyed effect re-run?" into "does this compile?" — the same failure as the + * epoch, one field over (BUG-3084 surface 4 found it on its first recovery + * leg: the reload never came, and the mock was why). + */ +let userIdSignal = $state('u1'); + +export function readUserId(): string { + return userIdSignal; +} + +export function writeUserId(next: string): void { + userIdSignal = next; +} + +export function bindReactiveUserId(hook: { + readUser: null | (() => string); + writeUser: null | ((id: string) => void); +}): void { + hook.readUser = readUserId; + hook.writeUser = writeUserId; +} + +/** + * Proves the SUBJECT'S OWN userId path is reactive — same contract as + * `isEpochReactive`: pass the mock's getter and setter, never this module's. + */ +export function isUserIdReactive(read: () => string, set: (id: string) => void): boolean { + let runs = 0; + const stop = $effect.root(() => { + $effect(() => { + read(); + runs += 1; + }); + }); + flushSync(); + const baseline = runs; + const before = read(); + set(before + '-moved'); + flushSync(); + const moved = read() !== before; + const reran = runs > baseline; + set(before); + flushSync(); + stop(); + return moved && reran; +} diff --git a/web/src/test/mocks/reactivePage.svelte.ts b/web/src/test/mocks/reactivePage.svelte.ts new file mode 100644 index 000000000..7311ffeb8 --- /dev/null +++ b/web/src/test/mocks/reactivePage.svelte.ts @@ -0,0 +1,21 @@ +// A REACTIVE `$app/state` page double, for suites that navigate between routes +// while a component is mounted. The default alias (`app-state.ts`) is a plain +// object, so reassigning `page.params` there never re-runs a `$derived` that +// reads it, and a "switch workspace mid-load" leg silently measures nothing +// (BUG-3237: the first run of its workspace-switch leg issued no second load). +// +// Use from a suite with: +// vi.mock('$app/state', async () => ({ page: (await import('/reactivePage.svelte')).page })); +// `$state` must live at module scope in a `.svelte.ts` file; a hoisted mock +// factory runs before the Svelte runtime is ready to declare it. + +export const page = $state({ + params: {} as Record, + url: new URL('http://localhost/'), + route: { id: null as string | null }, + status: 200, + error: null as unknown, + data: {} as Record, + form: null as unknown, + state: {} as Record, +});