From 5c06ae8bc28b89f1aedaf835be583139309c75dd Mon Sep 17 00:00:00 2001 From: Stefan Hildebrandt <695494+hildebrandttk@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:04:44 +0200 Subject: [PATCH] fix(files): reset the account-scoped Files drive on every account switch The Files store was the only account-scoped store never wired into the account lifecycle: account-state-manager clears email/contacts/calendar/ filter/identity/vacation on a switch, but not files. Because the Files store is a global that outlives the FilesApp component, the previous account's client + resources lingered in memory and resurfaced whenever the drive was reopened after a switch - most visibly when the switch happened on the mail view (FilesApp unmounted), the common normal-shell flow that the FilesApp re-init effect alone does not cover. Clear the Files store centrally in clearAllStores() alongside the other account-scoped stores, so the reset happens on every switch path. Tests: - lib/__tests__/account-state-manager.test.ts: assert clearAllStores() resets the Files drive (client, resources, pathStack, selection, supportsFiles) with no cross-account leak. - integration/tests/13-files-account-switch.spec.ts (dockerized Stalwart, JMAP FileNode): seeds a distinct top-level folder in alice's and bob's drives and asserts the on-screen drive swaps on a switch with no stale folder - both switching off the Files view (mail -> switch -> open Files) and on it (rail account switcher). Verified: passes with the fix, fails (path 1) without it. New FileNode seeding helpers in helpers/jmap.ts; openFiles/openMailView/driveEntry nav helpers in helpers/app.ts. --- components/files/files-app.tsx | 22 ++++-- .../tests/13-files-account-switch.spec.ts | 69 +++++++++++++++++++ integration/tests/helpers/app.ts | 36 ++++++++++ integration/tests/helpers/jmap.ts | 44 ++++++++++++ lib/__tests__/account-state-manager.test.ts | 28 ++++++++ lib/account-state-manager.ts | 9 +++ 6 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 integration/tests/13-files-account-switch.spec.ts diff --git a/components/files/files-app.tsx b/components/files/files-app.tsx index ecbb962fb..0998523b8 100644 --- a/components/files/files-app.tsx +++ b/components/files/files-app.tsx @@ -106,7 +106,11 @@ export function FilesApp({ linkSegments }: FilesAppProps = {}) { const isMobile = useIsMobile(); const isEmbedded = useIsEmbedded(); const [folderLayout, setFolderLayout] = useState(() => loadFilesSettings().folderLayout); - const hasFetched = useRef(false); + // The account the files client was last initialised for (`undefined` = never). + // Tracked so a Pro-shell account switch re-initialises instead of showing the + // previous account's files until a manual Home click. + const initedAccountRef = useRef(undefined); + const filesBootstrapRef = useRef(false); // Sync folderLayout when settings change useEffect(() => { @@ -153,11 +157,22 @@ export function FilesApp({ linkSegments }: FilesAppProps = {}) { // are surfaced as top-level folders at the root, so we *don't* auto-attach // to the active account - the user picks one explicitly. useEffect(() => { - if (!isAuthenticated || !client || hasFetched.current) return; - hasFetched.current = true; + if (!isAuthenticated || !client) return; + // Re-run when the ACTIVE account changes (Pro multi-account switch), not + // just once - otherwise the previous account's files linger until a manual + // Home click. + if (initedAccountRef.current === activeAccountId) return; + const switched = initedAccountRef.current !== undefined; + initedAccountRef.current = activeAccountId; if (isEmbedded) { useFileStore.getState().clearClient(); } else { + if (switched) { + // Drop the previous account's attached view so its files don't linger, + // and let the bootstrap effect below re-list for the new account. + useFileStore.getState().clearClient(); + filesBootstrapRef.current = false; + } initClient(client, activeAccountId); } }, [isAuthenticated, client, initClient, activeAccountId, isEmbedded]); @@ -203,7 +218,6 @@ export function FilesApp({ linkSegments }: FilesAppProps = {}) { // race - the warm one following the deep link while the cold one, a tick // later, listed the root on top of it. const storeClient = useFileStore(s => s.client); - const filesBootstrapRef = useRef(false); useEffect(() => { if (filesBootstrapRef.current) return; if (!storeClient) return; diff --git a/integration/tests/13-files-account-switch.spec.ts b/integration/tests/13-files-account-switch.spec.ts new file mode 100644 index 000000000..0995b07b7 --- /dev/null +++ b/integration/tests/13-files-account-switch.spec.ts @@ -0,0 +1,69 @@ +/** + * The Files drive is account-scoped: switching the active account must swap the + * drive to the new account's FileNodes and never leave the previous account's + * folders on screen. + * + * Regression for the stale-drive bug: the Files store is a global that outlives + * the FilesApp component, and it was the one account-scoped store not reset on a + * switch (account-state-manager cleared email/contacts/calendar/… but not + * files). So after switching accounts the old account's folders lingered until a + * manual navigation - both when the switch happened on the Files view and, more + * commonly, when it happened on the mail view before the drive was reopened. + * + * Two paths are exercised: + * 1. switch while the Files view is NOT mounted (mail view → switch → open + * Files) - the reported normal-shell case, guarded by clearing the store + * centrally on every switch. + * 2. switch while the Files view IS mounted (rail account switcher) - guarded + * by FilesApp re-initialising on the active-account change. + */ +import { test, expect } from '@playwright/test'; +import { ACCOUNTS } from './helpers/config'; +import { JmapClient } from './helpers/jmap'; +import { login, addAccount, switchAccount, openFiles, openMailView, driveEntry } from './helpers/app'; + +// Distinct top-level folder in each account's drive; presence/absence of these +// names is how we tell which account's drive is on screen. +const ALICE_DIR = 'alice-only-folder'; +const BOB_DIR = 'bob-only-folder'; + +test.describe('Files drive is account-scoped across an account switch', () => { + test.beforeAll(async () => { + const alice = await JmapClient.connect(ACCOUNTS.alice.email, ACCOUNTS.alice.password); + const bob = await JmapClient.connect(ACCOUNTS.bob.email, ACCOUNTS.bob.password); + // Start each drive from a known-empty root so the only entries are ours. + await alice.resetFiles(); + await bob.resetFiles(); + await alice.createFileDirectory(ALICE_DIR); + await bob.createFileDirectory(BOB_DIR); + }); + + test('switching accounts swaps the drive with no stale folders', async ({ page }) => { + await login(page, ACCOUNTS.alice); + await addAccount(page, ACCOUNTS.bob); // active account becomes bob + + // Bob's drive loads into the (global) Files store. + await openFiles(page); + await expect(driveEntry(page, BOB_DIR)).toBeVisible(); + await expect(driveEntry(page, ALICE_DIR)).toHaveCount(0); + + // Path 1 - switch happens OFF the Files view: go to mail, switch to alice + // there (FilesApp unmounted), then reopen Files. Without the central reset + // bob's folder would still be sitting in the store here. + await openMailView(page); + await switchAccount(page, ACCOUNTS.alice.email); + await openFiles(page); + await expect(driveEntry(page, ALICE_DIR)).toBeVisible(); + await expect(driveEntry(page, BOB_DIR)).toHaveCount(0); + + // Path 2 - switch happens ON the Files view via the rail account switcher. + await switchAccount(page, ACCOUNTS.bob.email); + await expect(driveEntry(page, BOB_DIR)).toBeVisible(); + await expect(driveEntry(page, ALICE_DIR)).toHaveCount(0); + + // And back again, to be sure the swap is symmetric and repeatable. + await switchAccount(page, ACCOUNTS.alice.email); + await expect(driveEntry(page, ALICE_DIR)).toBeVisible(); + await expect(driveEntry(page, BOB_DIR)).toHaveCount(0); + }); +}); diff --git a/integration/tests/helpers/app.ts b/integration/tests/helpers/app.ts index f9859b960..143d9267e 100644 --- a/integration/tests/helpers/app.ts +++ b/integration/tests/helpers/app.ts @@ -137,6 +137,42 @@ export async function forceSync(page: Page): Promise { await page.evaluate(() => document.dispatchEvent(new Event('visibilitychange'))); } +// ─── Files (drive) ───────────────────────────────────────────────────────── + +/** The primary navigation rail (there are two - mobile + desktop chrome). */ +function navRail(page: Page): Locator { + return page.locator('nav[aria-label="Navigation"]').first(); +} + +/** + * Open the Files drive via the navigation rail. A deep `page.goto('/files')` + * bounces to login in this SPA, so navigate in-app through the rail link + * (rendered for every route once files support is detected). Waits until the + * file browser toolbar is present. + */ +export async function openFiles(page: Page): Promise { + await navRail(page).getByRole('link', { name: 'Files', exact: true }).click(); + await page.locator('[role="toolbar"]').first().waitFor({ state: 'visible', timeout: 30000 }); +} + +/** + * Return to the mail view via the navigation rail's Mail link and wait until it + * has settled. Settling matters before touching the account switcher: while the + * mailbox is still loading the sidebar reflows, which leaves the switcher's + * popover items shifting/detaching under a click. Waiting for the Inbox row + * (and the switcher) pins the layout first. + */ +export async function openMailView(page: Page): Promise { + await navRail(page).getByRole('link', { name: 'Mail', exact: true }).click(); + await accountSwitcher(page).waitFor({ state: 'visible', timeout: 30000 }); + await folderRow(page, { role: 'inbox' }).first().waitFor({ state: 'visible', timeout: 30000 }); +} + +/** A drive entry (folder/file) row, selected by its name via `data-resource`. */ +export function driveEntry(page: Page, name: string): Locator { + return page.locator(`[data-resource="${name}"]`); +} + // ─── Composer / drafts ──────────────────────────────────────────────────── /** Open the composer via the keyboard shortcut and wait for it to render. */ diff --git a/integration/tests/helpers/jmap.ts b/integration/tests/helpers/jmap.ts index 772821e11..2238ad22b 100644 --- a/integration/tests/helpers/jmap.ts +++ b/integration/tests/helpers/jmap.ts @@ -14,6 +14,7 @@ const CORE = 'urn:ietf:params:jmap:core'; const MAIL = 'urn:ietf:params:jmap:mail'; const PRINCIPALS = 'urn:ietf:params:jmap:principals'; const SUBMISSION = 'urn:ietf:params:jmap:submission'; +const FILENODE = 'urn:ietf:params:jmap:filenode'; /** Rights granted on a shared mailbox (JMAP ACL). */ export const FULL_MAILBOX_RIGHTS = { @@ -43,6 +44,10 @@ export class JmapClient { private authHeader: string; private apiUrl: string; accountId = ''; + /** Account that holds this user's Files drive (FileNode). On Stalwart's + * personal mailboxes this equals the mail account, but read it from the + * session so the helper stays correct if that ever diverges. */ + filesAccountId = ''; /** Every account visible in this user's session (own + shared/group), * keyed by accountId -> account name (its email address). */ accounts: Record = {}; @@ -64,6 +69,7 @@ export class JmapClient { const primary = session.primaryAccounts?.[MAIL]; if (!primary) throw new Error(`No mail account for ${email} in JMAP session`); c.accountId = primary; + c.filesAccountId = session.primaryAccounts?.[FILENODE] ?? primary; c.accounts = Object.fromEntries( Object.entries(session.accounts ?? {}).map(([id, a]) => [id, (a as { name: string }).name]), ); @@ -295,4 +301,42 @@ export class JmapClient { await new Promise((r) => setTimeout(r, 500)); } } + + // ─── Files (JMAP FileNode) ──────────────────────────────────────────────── + + /** Ids of every FileNode in this account's drive. */ + private async allFileNodeIds(): Promise { + const r = await this.request( + [['FileNode/query', { accountId: this.filesAccountId, filter: {}, limit: 5000 }, '0']], + [CORE, FILENODE], + ); + return r.methodResponses[0][1].ids as string[]; + } + + /** Wipe the account's drive so a test starts from a known-empty root. */ + async resetFiles(): Promise { + const ids = await this.allFileNodeIds(); + if (!ids.length) return; + await this.request( + [['FileNode/set', { accountId: this.filesAccountId, destroy: ids }, '0']], + [CORE, FILENODE], + ); + } + + /** + * Create a top-level directory FileNode and return its id. A directory is a + * FileNode with no blob/type/size (server stores it with `file == null`), + * mirroring how the app's createFileDirectory seeds one. + */ + async createFileDirectory(name: string, parentId: string | null = null): Promise { + const props: Record = { name }; + if (parentId !== null) props.parentId = parentId; + const r = await this.request( + [['FileNode/set', { accountId: this.filesAccountId, create: { dir0: props } }, '0']], + [CORE, FILENODE], + ); + const created = r.methodResponses[0][1].created?.dir0; + if (!created) throw new Error(`createFileDirectory failed: ${JSON.stringify(r.methodResponses[0][1])}`); + return created.id as string; + } } diff --git a/lib/__tests__/account-state-manager.test.ts b/lib/__tests__/account-state-manager.test.ts index 7f61265f5..3ad649036 100644 --- a/lib/__tests__/account-state-manager.test.ts +++ b/lib/__tests__/account-state-manager.test.ts @@ -12,6 +12,7 @@ import { useCalendarStore } from '@/stores/calendar-store'; import { useFilterStore } from '@/stores/filter-store'; import { useIdentityStore } from '@/stores/identity-store'; import { useVacationStore } from '@/stores/vacation-store'; +import { useFileStore } from '@/stores/file-store'; import { DEFAULT_SEARCH_FILTERS } from '@/lib/jmap/search-utils'; import { makeEmail, makeMailbox } from './helpers/factories'; @@ -109,6 +110,33 @@ describe('clearAllStores', () => { expect(s.tagCounts).toEqual({}); expect(s.searchFilters).toEqual(DEFAULT_SEARCH_FILTERS); }); + + it('resets the account-scoped Files drive (no stale resources across a switch)', () => { + // Simulate account A having opened the drive: client attached, a folder + // navigated into, and its listing loaded. + const fakeClient = {} as never; + useFileStore.getState().initClient(fakeClient, 'A'); + useFileStore.setState({ + resources: [{ id: 'r1', name: 'A-secret.txt' } as never], + pathStack: [{ id: null, name: '' }, { id: 'folderA', name: 'Folder A' }], + currentPath: '/Folder A', + currentParentId: 'folderA', + supportsFiles: true, + selectedResources: new Set(['r1']), + }); + + clearAllStores(); + + const f = useFileStore.getState(); + expect(f.client).toBeNull(); + expect(f.currentAccountId).toBeNull(); + expect(f.resources).toEqual([]); + expect(f.selectedResources.size).toBe(0); + expect(f.currentParentId).toBeNull(); + expect(f.currentPath).toBe('/'); + expect(f.pathStack).toEqual([{ id: null, name: '' }]); + expect(f.supportsFiles).toBeNull(); + }); }); describe('evictAccount / evictAll', () => { diff --git a/lib/account-state-manager.ts b/lib/account-state-manager.ts index 036cf6d58..92fdb092d 100644 --- a/lib/account-state-manager.ts +++ b/lib/account-state-manager.ts @@ -11,6 +11,7 @@ import { useFilterStore } from '@/stores/filter-store'; import { DEFAULT_SEARCH_FILTERS } from '@/lib/jmap/search-utils'; import { useIdentityStore } from '@/stores/identity-store'; import { useVacationStore } from '@/stores/vacation-store'; +import { useFileStore } from '@/stores/file-store'; // Minimal snapshot shapes - we only capture what we need // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -132,6 +133,14 @@ export function clearAllStores(): void { useVacationStore.getState().clearState(); useCalendarStore.getState().clearState(); useFilterStore.getState().clearState(); + // The Files drive is account-scoped like every store above, but unlike them + // it lives in a global store that outlives the FilesApp component. If we + // don't reset it here, the previous account's client + resources linger in + // memory and resurface (stale) whenever the drive is opened after a switch - + // even when FilesApp wasn't mounted at switch time (the common case: switch + // from the mail view, then navigate to Files). Clearing centrally makes the + // reset happen on every switch path, not just the one the component observes. + useFileStore.getState().clearClient(); } /** Evict cached state for one account */