From fef6bd68225f03a183b2577bfd2b83575424bc01 Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Mon, 13 Jul 2026 02:32:39 +0200 Subject: [PATCH] fix(dav): Prevent race condition in useDAVFiles Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: David Dreschner --- lib/composables/dav.spec.ts | 49 ++++++++++++++++++++ lib/composables/dav.ts | 92 +++++++++++++++++++++++++++++++------ 2 files changed, 127 insertions(+), 14 deletions(-) diff --git a/lib/composables/dav.spec.ts b/lib/composables/dav.spec.ts index 80921295f..b2b44c0b8 100644 --- a/lib/composables/dav.spec.ts +++ b/lib/composables/dav.spec.ts @@ -240,4 +240,53 @@ describe('dav composable', () => { await waitRefLoaded(isLoading) expect(abort).toBeCalledTimes(1) }) + + it('a superseded (aborted) load does not clear the loading state of the newer load', async () => { + // Each getDirectoryContents call stays pending until resolved manually, + // and rejects with an AbortError when its signal is aborted — mirroring a + // real DAV request that is cancelled on fast navigation. + const pending: Array<{ resolve: () => void }> = [] + const client = { + stat: vi.fn((v) => ({ data: { path: v } })), + getDirectoryContents: vi.fn((_path, opts) => new Promise((resolve, reject) => { + opts.signal?.addEventListener('abort', () => { + const error = new Error('aborted') + error.name = 'AbortError' + reject(error) + }) + pending.push({ resolve: () => resolve({ data: [] }) }) + })), + } + nextcloudFiles.getClient.mockImplementationOnce(() => client) + nextcloudFiles.resultToNode.mockImplementation((v) => v) + + const view = ref<'files' | 'recent' | 'favorites'>('files') + const path = ref('/') + const { loadFiles, isLoading } = useDAVFiles(view, path) + + // Load A is in flight + loadFiles() + await nextTick() + expect(pending).toHaveLength(1) + expect(isLoading.value).toBe(true) + + // Load B supersedes A: navigating aborts A and starts B loading + path.value = '/subdir/' + await nextTick() + expect(pending).toHaveLength(2) + + // Let A's aborted request reject and run its catch/finally + await nextTick() + await nextTick() + + // Regression: A's finally must not flip isLoading to false while B is + // still loading — otherwise the FilePicker sees isLoading=false with a + // null folder and confirms an empty selection ("No nodes selected"). + expect(isLoading.value).toBe(true) + + // Completing B settles the loading state + pending[1].resolve() + await waitRefLoaded(isLoading) + expect(isLoading.value).toBe(false) + }) }) diff --git a/lib/composables/dav.ts b/lib/composables/dav.ts index 99e1c25d1..851cf1e69 100644 --- a/lib/composables/dav.ts +++ b/lib/composables/dav.ts @@ -11,6 +11,64 @@ import { join } from '@nextcloud/paths' import { onMounted, ref, shallowRef, watch } from 'vue' import { getFile, getNodes, getRecentNodes } from '../utils/dav.ts' +/** + * Track which load of a repeatedly started async operation is the current one. + * + * The returned closure owns the ONLY state shared between overlapping loads + * (the abort controller of the most recently started one). A running load + * never touches that state directly — it only holds its own `AbortSignal`, + * so a stale invocation cannot accidentally reset shared state that already + * belongs to a newer load. + */ +function createCurrentLoad() { + let controller: AbortController | undefined + + /** + * Whether the load owning `signal` has been superseded by a newer `start()`. + * + * Pure query, no side effects — supersession is the only thing that ever + * aborts a load's signal, so `signal.aborted` is exactly this fact. + * + * @param signal The signal this load received from `start()` + * @return `true` when a newer `start()` has superseded this load + */ + const isSuperseded = (signal: AbortSignal): boolean => signal.aborted + + return { + /** + * Start a new load, aborting a still running previous one. + * + * @return This load's own abort signal. It is aborted exactly when a + * later `start()` supersedes this load. + */ + start(): AbortSignal { + controller?.abort() + controller = new AbortController() + return controller.signal + }, + + isSuperseded, + + /** + * Settle the load that owns `signal`. + * + * While not superseded, `controller` is guaranteed to still be this + * load's own (no later `start()` has run), so it can safely be released. + * + * @param signal The signal this load received from `start()` + * @return `true` if this load is still the current one — only then may + * the caller reset shared state. `false` if it was superseded. + */ + complete(signal: AbortSignal): boolean { + if (isSuperseded(signal)) { + return false + } + controller = undefined + return true + }, + } +} + /** * Handle file loading using WebDAV * @@ -42,9 +100,11 @@ export function useDAVFiles( const isLoading = ref(true) /** - * The cancelable promise used internally to cancel on fast navigation + * Coordination of overlapping loads on fast navigation. + * All shared state lives inside this closure — `loadDAVFiles` itself only + * ever holds the per-invocation signal returned by `currentLoad.start()`. */ - let abortController: AbortController | undefined + const currentLoad = createCurrentLoad() /** * Create a new directory in the current path @@ -66,34 +126,38 @@ export function useDAVFiles( * Force reload files using the DAV client */ async function loadDAVFiles() { - if (abortController) { - abortController.abort() - abortController = undefined - } + // `signal` is this invocation's own — the only way a newer load can + // interact with this one is by aborting it through currentLoad.start(). + const signal = currentLoad.start() - abortController = new AbortController() isLoading.value = true try { if (currentView.value === 'favorites') { - files.value = await getFavoriteNodes({ client, path: currentPath.value, signal: abortController.signal }) + files.value = await getFavoriteNodes({ client, path: currentPath.value, signal }) folder.value = null } else if (currentView.value === 'recent') { - files.value = await getRecentNodes({ client, signal: abortController.signal }) + files.value = await getRecentNodes({ client, signal }) folder.value = null } else { - const content = await getNodes({ client, path: currentPath.value, signal: abortController.signal }) + const content = await getNodes({ client, path: currentPath.value, signal }) folder.value = content.folder files.value = content.contents } } catch (error) { - if (error instanceof Error && error.name === 'AbortError') { - // ignore abort errors + // A superseded load's outcome is irrelevant: + // only the live load may report errors. + if (currentLoad.isSuperseded(signal)) { return } throw error } finally { - abortController = undefined - isLoading.value = false + // Only the still-current load may settle the shared loading state. + // A superseded load must never do this — a newer load is still in + // flight and `folder` may still be null, which would let the + // FilePicker confirm an empty selection ("No nodes selected"). + if (currentLoad.complete(signal)) { + isLoading.value = false + } } }