From 3b4331dbe6f056c0843fb0d2616aeffd50948cac Mon Sep 17 00:00:00 2001
From: xarmian <xarmian@gmail.com>
Date: Mon, 3 Aug 2026 12:31:03 +0000
Subject: [PATCH] fix(attachments): reload storage tab on workspace change
 (TASK-2418)

Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
---
 .../lib/components/settings/StorageTab.svelte |  90 +++++-
 .../settings/StorageTab.svelte.test.ts        | 276 ++++++++++++++++++
 web/src/test/mocks/app-state.ts               |  25 ++
 web/vitest.config.ts                          |   8 +-
 4 files changed, 389 insertions(+), 10 deletions(-)
 create mode 100644 web/src/lib/components/settings/StorageTab.svelte.test.ts
 create mode 100644 web/src/test/mocks/app-state.ts

diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte
index 9bd2192e..741d8cbe 100644
--- a/web/src/lib/components/settings/StorageTab.svelte
+++ b/web/src/lib/components/settings/StorageTab.svelte
@@ -1,5 +1,5 @@
 <script lang="ts">
-	import { onMount, untrack } from 'svelte';
+	import { untrack } from 'svelte';
 	import { page } from '$app/state';
 	import { api, PadApiError } from '$lib/api/client';
 	import { announceAttachmentDeleted } from '$lib/attachments/events';
@@ -159,13 +159,56 @@
 		attachments.find((a) => a.item_id === filterItemId)?.item_title ?? ''
 	);
 
-	// Re-seed when the deep-link changes under a MOUNTED tab (same route, new
-	// `?attachment_item=`). Without it, the second "View all" link a user
-	// follows from the same settings page silently keeps the first item's
-	// scope (Codex round 3).
+	// The workspace whose view is currently loaded. A plain `let` (not `$state`)
+	// so writing it from the effect below can't re-trigger it.
+	let loadedWsSlug: string | null = null;
+
+	/**
+	 * The tab's only load trigger. It owns BOTH transitions, in one effect and
+	 * one dependency set, because they can arrive in the same flush and the
+	 * order matters:
+	 *
+	 *  - **Workspace change.** `/{user}/{ws}/settings` is one SvelteKit route,
+	 *    so switching workspaces changes `wsSlug` under a MOUNTED tab. Without
+	 *    a reactive reload the previous workspace's rows and usage figure stay
+	 *    on screen, and a list request that was in flight across the switch is
+	 *    dropped by `loadList`'s workspace fence — which, with no successor
+	 *    request, would strand `listLoading` at true forever (final review P1).
+	 *    Since this fires on mount too, it REPLACES the old `onMount` load
+	 *    rather than sitting alongside it — an effect plus `onMount` would
+	 *    fetch the first page twice.
+	 *  - **Deep-link change.** A new `?attachment_item=` on the same route
+	 *    (a second "View all" followed from an already-open settings page)
+	 *    retargets the scope instead of silently keeping the first item's
+	 *    (Codex round 3).
+	 *
+	 * A workspace change subsumes the deep-link branch: it re-seeds the scope
+	 * from the incoming link itself, so a single load covers both and the
+	 * deep-link branch can't fire a second one behind it.
+	 */
 	$effect(() => {
+		const ws = wsSlug;
 		const incoming = initialItemId;
 		untrack(() => {
+			if (ws !== loadedWsSlug) {
+				loadedWsSlug = ws;
+				// Everything workspace-scoped is meaningless in the new
+				// workspace and must not survive the switch: the rows and
+				// usage figure obviously, but also the two workspace-scoped
+				// filter values — an item uuid and a collection uuid from the
+				// old workspace would silently filter the new list down to
+				// nothing. Category / attached / sort / page size are plain
+				// preferences with no workspace identity, so they carry over.
+				seededItemId = incoming;
+				filterItemId = incoming;
+				filterCollection = '';
+				offset = 0;
+				attachments = [];
+				total = 0;
+				usage = null;
+				void loadWorkspaceView();
+				return;
+			}
 			if (incoming === seededItemId) return;
 			seededItemId = incoming;
 			filterItemId = incoming;
@@ -196,10 +239,24 @@
 		retargetScope();
 	}
 
+	let usageGen = 0;
+
 	async function loadUsage() {
+		// Both halves of the list load's fence, for the same reasons. The
+		// workspace check: this tab stays mounted across a workspace change, so a
+		// figure fetched for the workspace the user just left must not paint over
+		// the one they switched to (nor raise its error toast). The generation
+		// check: on an A→B→A round trip the workspace matches again, so only the
+		// generation can tell the first A request from the current one
+		// (Codex round 1).
+		const gen = ++usageGen;
+		const reqWsSlug = wsSlug;
 		try {
-			usage = await api.attachments.storageUsage(wsSlug);
+			const next = await api.attachments.storageUsage(reqWsSlug);
+			if (gen !== usageGen || reqWsSlug !== wsSlug) return;
+			usage = next;
 		} catch (err) {
+			if (gen !== usageGen || reqWsSlug !== wsSlug) return;
 			const msg = err instanceof Error ? err.message : 'Failed to load storage usage';
 			toastStore.show(msg, 'error');
 		}
@@ -267,13 +324,28 @@
 		await Promise.all([loadList(), loadUsage()]);
 	}
 
-	onMount(async () => {
+	/**
+	 * Load (or reload) everything the tab shows for the current workspace, behind
+	 * the whole-tab `loading` gate. Used on mount and on every workspace change.
+	 */
+	let viewGen = 0;
+
+	async function loadWorkspaceView() {
+		const gen = ++viewGen;
+		const reqWsSlug = wsSlug;
+		loading = true;
 		try {
 			await Promise.all([loadList(), loadUsage()]);
 		} finally {
-			loading = false;
+			// Only the NEWEST load lowers the gate; a superseded one doing it
+			// would reveal the next workspace's half-loaded view. The workspace
+			// check alone isn't enough — on an A→B→A round trip the first A load
+			// finishes with `wsSlug` back at A while A's current load is still in
+			// flight (Codex round 1). It can't strand: whatever supersedes a load
+			// is itself a load, and the newest one's `finally` always runs.
+			if (gen === viewGen && reqWsSlug === wsSlug) loading = false;
 		}
-	});
+	}
 
 	// Filter / sort changes reset to the first page and refetch. Using an
 	// explicit handler instead of an $effect keeps the side-effect tied to
diff --git a/web/src/lib/components/settings/StorageTab.svelte.test.ts b/web/src/lib/components/settings/StorageTab.svelte.test.ts
new file mode 100644
index 00000000..ce63dcfb
--- /dev/null
+++ b/web/src/lib/components/settings/StorageTab.svelte.test.ts
@@ -0,0 +1,276 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import { flushSync, mount, unmount } from 'svelte';
+import type {
+	AttachmentListItem,
+	AttachmentListResponse,
+	WorkspaceStorageInfo,
+} from '$lib/types';
+
+// TASK-2418. The Storage tab lives at `/{user}/{ws}/settings` — ONE SvelteKit
+// route — so switching workspaces changes `wsSlug` under a MOUNTED component.
+// These tests drive that switch and pin the reactive reload: fresh rows, fresh
+// usage, no stale scope, and a loading state that can't strand when the switch
+// happens mid-flight.
+
+const listMock =
+	vi.fn<(ws: string, filters: Record<string, unknown>) => Promise<AttachmentListResponse>>();
+const usageMock = vi.fn<(ws: string) => Promise<WorkspaceStorageInfo>>();
+const toastMock = vi.fn<(message: string, kind?: string) => void>();
+
+class FakeApiError extends Error {
+	code: string;
+	constructor(code: string) {
+		super(code);
+		this.code = code;
+	}
+}
+
+vi.mock('$lib/api/client', () => ({
+	PadApiError: FakeApiError,
+	api: {
+		attachments: {
+			list: (ws: string, filters: Record<string, unknown>) => listMock(ws, filters),
+			storageUsage: (ws: string) => usageMock(ws),
+			downloadUrl: (ws: string, id: string, variant?: string) =>
+				`/api/v1/workspaces/${ws}/attachments/${id}${variant ? `?variant=${variant}` : ''}`,
+			delete: vi.fn(),
+		},
+	},
+}));
+
+vi.mock('$app/state', () => ({
+	page: { params: { username: 'dave', workspace: 'ws-a' }, url: new URL('http://x/') },
+}));
+
+vi.mock('$lib/attachments/events', () => ({
+	announceAttachmentDeleted: vi.fn(),
+}));
+
+vi.mock('$lib/components/editor/attachment-metadata', () => ({
+	invalidateAttachmentMetadata: vi.fn(),
+}));
+
+vi.mock('$lib/stores/toast.svelte', () => ({
+	toastStore: { show: (message: string, kind?: string) => toastMock(message, kind) },
+}));
+
+const { default: StorageTab } = await import('./StorageTab.svelte');
+
+function att(overrides: Partial<AttachmentListItem> & { id: string }): AttachmentListItem {
+	return {
+		workspace_id: 'ws-1',
+		uploaded_by: 'u-1',
+		storage_key: `key/${overrides.id}`,
+		content_hash: `hash-${overrides.id}`,
+		mime_type: 'application/pdf',
+		size_bytes: 2048,
+		filename: `${overrides.id}.pdf`,
+		created_at: '2026-08-01T00:00:00Z',
+		...overrides,
+	};
+}
+
+function response(attachments: AttachmentListItem[]): AttachmentListResponse {
+	return { attachments, total: attachments.length, limit: 50, offset: 0 };
+}
+
+function usage(used: number): WorkspaceStorageInfo {
+	return { used_bytes: used, limit_bytes: 1000, override_active: false } as WorkspaceStorageInfo;
+}
+
+/** A promise plus its resolver, so a test can control when a fetch lands. */
+function deferred<T>() {
+	let resolve!: (value: T) => void;
+	let reject!: (err: unknown) => void;
+	const promise = new Promise<T>((res, rej) => {
+		resolve = res;
+		reject = rej;
+	});
+	return { promise, resolve, reject };
+}
+
+// Reactive props object so a test can flip `wsSlug` the way the settings route
+// does when the user switches workspaces while this tab is open.
+const props = $state<{ wsSlug: string; collections: []; initialItemId: string }>({
+	wsSlug: 'ws-a',
+	collections: [],
+	initialItemId: '',
+});
+
+describe('StorageTab workspace switching', () => {
+	let target: HTMLElement;
+	let instance: ReturnType<typeof mount> | undefined;
+
+	beforeEach(() => {
+		listMock.mockReset();
+		listMock.mockResolvedValue(response([]));
+		usageMock.mockReset();
+		usageMock.mockResolvedValue(usage(1));
+		toastMock.mockReset();
+		props.wsSlug = 'ws-a';
+		props.initialItemId = '';
+		target = document.body.appendChild(document.createElement('div'));
+	});
+
+	afterEach(() => {
+		if (instance) unmount(instance);
+		instance = undefined;
+		target.remove();
+	});
+
+	function mountTab() {
+		instance = mount(StorageTab, { target, props });
+		flushSync();
+	}
+
+	async function settle() {
+		for (let i = 0; i < 6; i++) await Promise.resolve();
+		flushSync();
+	}
+
+	function rows(): HTMLElement[] {
+		return Array.from(target.querySelectorAll<HTMLElement>('.att-row'));
+	}
+
+	function text(): string {
+		return target.textContent ?? '';
+	}
+
+	it('loads exactly once on mount', async () => {
+		mountTab();
+		await settle();
+
+		expect(listMock).toHaveBeenCalledTimes(1);
+		expect(listMock).toHaveBeenCalledWith('ws-a', expect.anything());
+		expect(usageMock).toHaveBeenCalledTimes(1);
+		expect(usageMock).toHaveBeenCalledWith('ws-a');
+	});
+
+	it('reloads the list and the usage figure when the workspace changes', async () => {
+		listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'from-a.pdf' })]));
+		usageMock.mockResolvedValueOnce(usage(111));
+		mountTab();
+		await settle();
+		expect(rows()).toHaveLength(1);
+		expect(text()).toContain('from-a.pdf');
+		expect(text()).toContain('111 B');
+
+		listMock.mockResolvedValueOnce(response([att({ id: 'b1', filename: 'from-b.pdf' })]));
+		usageMock.mockResolvedValueOnce(usage(222));
+		props.wsSlug = 'ws-b';
+		flushSync();
+		await settle();
+
+		expect(listMock).toHaveBeenCalledTimes(2);
+		expect(listMock).toHaveBeenLastCalledWith('ws-b', expect.anything());
+		expect(usageMock).toHaveBeenLastCalledWith('ws-b');
+		expect(rows()).toHaveLength(1);
+		expect(text()).toContain('from-b.pdf');
+		expect(text()).not.toContain('from-a.pdf');
+		expect(text()).toContain('222 B');
+	});
+
+	it('drops the old workspace item scope when the workspace changes', async () => {
+		props.initialItemId = 'item-in-a';
+		mountTab();
+		await settle();
+		expect(listMock).toHaveBeenLastCalledWith(
+			'ws-a',
+			expect.objectContaining({ item_id: 'item-in-a' })
+		);
+		expect(target.querySelector('.item-scope')).not.toBeNull();
+
+		// A workspace switch without a deep link: the scope named an item in the
+		// workspace the user just left, so it must not narrow the new list.
+		props.wsSlug = 'ws-b';
+		props.initialItemId = '';
+		flushSync();
+		await settle();
+
+		expect(listMock).toHaveBeenCalledTimes(2);
+		expect(listMock).toHaveBeenLastCalledWith('ws-b', expect.not.objectContaining({ item_id: 'item-in-a' }));
+		expect(target.querySelector('.item-scope')).toBeNull();
+	});
+
+	it('does not strand the loading state when the switch happens mid-flight', async () => {
+		const slowA = deferred<AttachmentListResponse>();
+		listMock.mockReturnValueOnce(slowA.promise);
+		mountTab();
+		await settle();
+		expect(text()).toContain('Loading');
+
+		listMock.mockResolvedValueOnce(response([att({ id: 'b1', filename: 'from-b.pdf' })]));
+		props.wsSlug = 'ws-b';
+		flushSync();
+		await settle();
+
+		// B landed; the tab is showing B even though A is still outstanding.
+		expect(text()).toContain('from-b.pdf');
+		expect(text()).not.toContain('Loading attachments');
+
+		// A's superseded response resolves late and must change nothing — in
+		// particular it must not re-raise the loading gate or paint its rows.
+		slowA.resolve(response([att({ id: 'a1', filename: 'from-a.pdf' })]));
+		await settle();
+		expect(text()).toContain('from-b.pdf');
+		expect(text()).not.toContain('from-a.pdf');
+		expect(text()).not.toContain('Loading attachments');
+	});
+
+	it('ignores the first load of an A→B→A round trip, where the workspace matches again', async () => {
+		// The workspace fence can't see this one: by the time A's FIRST response
+		// lands, `wsSlug` is back at 'ws-a'. Only the generation distinguishes it
+		// from the load that is actually current (Codex round 1).
+		const staleA = deferred<AttachmentListResponse>();
+		const staleUsageA = deferred<WorkspaceStorageInfo>();
+		listMock.mockReturnValueOnce(staleA.promise);
+		usageMock.mockReturnValueOnce(staleUsageA.promise);
+		mountTab();
+		await settle();
+
+		props.wsSlug = 'ws-b';
+		flushSync();
+		await settle();
+
+		const freshA = deferred<AttachmentListResponse>();
+		listMock.mockReturnValueOnce(freshA.promise);
+		usageMock.mockResolvedValueOnce(usage(333));
+		props.wsSlug = 'ws-a';
+		flushSync();
+		await settle();
+
+		// The first A load resolves late. It must not lower the whole-tab gate
+		// (the current A load is still pending) nor paint its rows or usage.
+		staleA.resolve(response([att({ id: 'stale', filename: 'stale-a.pdf' })]));
+		staleUsageA.resolve(usage(999));
+		await settle();
+		expect(text()).toContain('Loading storage…');
+		expect(text()).not.toContain('stale-a.pdf');
+		expect(text()).not.toContain('999 B');
+
+		// The current one does.
+		freshA.resolve(response([att({ id: 'fresh', filename: 'fresh-a.pdf' })]));
+		await settle();
+		expect(text()).not.toContain('Loading storage…');
+		expect(text()).toContain('fresh-a.pdf');
+		expect(text()).not.toContain('stale-a.pdf');
+	});
+
+	it('ignores a superseded usage response and its error toast', async () => {
+		const slowA = deferred<WorkspaceStorageInfo>();
+		usageMock.mockReturnValueOnce(slowA.promise);
+		mountTab();
+		await settle();
+
+		usageMock.mockResolvedValueOnce(usage(222));
+		props.wsSlug = 'ws-b';
+		flushSync();
+		await settle();
+		expect(text()).toContain('222 B');
+
+		slowA.reject(new Error('workspace a is gone'));
+		await settle();
+		expect(text()).toContain('222 B');
+		expect(toastMock).not.toHaveBeenCalled();
+	});
+});
diff --git a/web/src/test/mocks/app-state.ts b/web/src/test/mocks/app-state.ts
new file mode 100644
index 00000000..97464bc2
--- /dev/null
+++ b/web/src/test/mocks/app-state.ts
@@ -0,0 +1,25 @@
+// Test-only stand-in for SvelteKit's `$app/state`.
+//
+// Same reason as `app-environment.ts`: the jsdom vitest project runs without
+// the SvelteKit vite plugin, so `$app/state` has no provider and any component
+// importing it fails to RESOLVE — before `vi.mock` ever gets a chance to
+// substitute it. vitest.config.ts aliases the import here so such components
+// are mountable at all; a test that cares about the values either mutates the
+// exported objects or `vi.mock`s the specifier itself.
+//
+// Plain mutable objects, not runes: components read `page.params.x` in
+// `$derived`, which works fine against a static object for tests that don't
+// need the read to be reactive.
+export const page = {
+	params: {} as Record<string, string>,
+	url: new URL('http://localhost/'),
+	route: { id: null as string | null },
+	status: 200,
+	error: null as unknown,
+	data: {} as Record<string, unknown>,
+	form: null as unknown,
+	state: {} as Record<string, unknown>,
+};
+
+export const navigating = { from: null, to: null, type: null, complete: null };
+export const updated = { current: false, check: async () => false };
diff --git a/web/vitest.config.ts b/web/vitest.config.ts
index 7c68f8bc..65dacbb5 100644
--- a/web/vitest.config.ts
+++ b/web/vitest.config.ts
@@ -35,6 +35,7 @@ function canResolve(id: string): boolean {
 const projectRoot = fileURLToPath(new URL('.', import.meta.url));
 const $lib = fileURLToPath(new URL('./src/lib', import.meta.url));
 const appEnvironmentMock = fileURLToPath(new URL('./src/test/mocks/app-environment.ts', import.meta.url));
+const appStateMock = fileURLToPath(new URL('./src/test/mocks/app-state.ts', import.meta.url));
 
 // Agent worktrees symlink `web/node_modules` to the main checkout's
 // node_modules rather than `npm install`-ing a copy (installing clobbers a
@@ -90,8 +91,13 @@ export default defineConfig(async () => {
 			resolve: {
 				alias: {
 					$lib,
-					// No SvelteKit plugin in this project, so provide `$app/environment`.
+					// No SvelteKit plugin in this project, so provide `$app/environment`
+					// and `$app/state` — without a provider these don't just come back
+					// undefined, they fail to RESOLVE, which is a load-time error for
+					// any component that imports them (and one `vi.mock` can't rescue,
+					// since resolution happens first).
 					'$app/environment': appEnvironmentMock,
+					'$app/state': appStateMock,
 				},
 			},
 			server: nodeModulesRealPath