From 5ea19a85814e117a04b28f401307b90c1978ecde Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 03:17:48 +0100 Subject: [PATCH] fix: restore a trashed item into its folder when that folder is restored with it --- src/api/__tests__/files-trash.test.ts | 32 ++++++++++++ src/api/files-trash.ts | 73 ++++++++++++++++++++------- 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/src/api/__tests__/files-trash.test.ts b/src/api/__tests__/files-trash.test.ts index 5dbaa249..dcb29ea6 100644 --- a/src/api/__tests__/files-trash.test.ts +++ b/src/api/__tests__/files-trash.test.ts @@ -277,6 +277,38 @@ describe('restoreFromTrash', () => { expect(byId('a2')).toMatchObject({ parentId: 'docs', name: 'a.txt' }); }); + it('puts an item back into its folder when that folder is restored with it', async () => { + await moveToTrash(listed(), listed().filter((n) => n.id === 'a'), 1730112000007); + await moveToTrash(listed(), listed().filter((n) => n.id === 'docs'), 1730112000008); + const nodes = listed(); + const { done } = await restoreFromTrash(trashedItems(nodes)); + expect(done.map((i) => [i.node.id, i.node.parentId]).sort()).toEqual([['a', 'docs'], ['docs', null]]); + expect(byId('docs')!.parentId).toBeNull(); + expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a.txt' }); + expect(store.some((n) => /^\d+~/.test(n.name))).toBe(false); + }); + + it('puts an item back into a folder inside one restored with it, under a free name', async () => { + store.push({ id: 'sub', parentId: 'docs', name: 'Sub', blobId: null }, { id: 'c', parentId: 'sub', name: 'c.txt', blobId: 'bc' }); + await moveToTrash(listed(), listed().filter((n) => n.id === 'c'), 1730112000007); + store.push({ id: 'c2', parentId: 'sub', name: 'c.txt', blobId: 'bc2' }); + await moveToTrash(listed(), listed().filter((n) => n.id === 'docs'), 1730112000008); + await restoreFromTrash(trashedItems(listed())); + expect(byId('docs')!.parentId).toBeNull(); + expect(byId('c')).toMatchObject({ parentId: 'sub', name: 'c (1).txt' }); + expect(byId('c2')).toMatchObject({ parentId: 'sub', name: 'c.txt' }); + }); + + it('puts the item at the root when its folder could not be restored with it', async () => { + await moveToTrash(listed(), listed().filter((n) => n.id === 'a'), 1730112000007); + await moveToTrash(listed(), listed().filter((n) => n.id === 'docs'), 1730112000008); + refuseUpdate.add('docs'); + const { done, refused } = await restoreFromTrash(trashedItems(listed())); + expect(done.map((i) => i.node.id)).toEqual(['a']); + expect(refused).toHaveLength(1); + expect(byId('a')!.parentId).toBeNull(); + }); + it('gives a folder named .Trash that goes back to the root another name', async () => { store.push({ id: 'x', parentId: 'docs', name: '.Trash', blobId: null }, { id: 'xin', parentId: 'x', name: 'in.txt', blobId: 'bi' }); let nodes = listed(); diff --git a/src/api/files-trash.ts b/src/api/files-trash.ts index 9a4028ae..3bb15c15 100644 --- a/src/api/files-trash.ts +++ b/src/api/files-trash.ts @@ -2,7 +2,7 @@ import type { FileNode } from './types'; import { jmapClient } from './jmap-client'; import { createFolder, deleteFileNodes, destroyEmptyFolders, filesAccountId, getAllFileNodes, isCrossAccountId, - updateFileNodes, + isFolder, updateFileNodes, } from './files'; import { JMAPSetError } from './jmap-result'; import { getUniqueName } from '../lib/filenode-name'; @@ -168,8 +168,9 @@ export async function moveToTrash( /** * Put items of the trash back into the folder each came from, or the root - * when that folder is gone or in the trash itself. A name taken there by now - * gets " (1)" and so on. Their trash folders go once they are empty. + * when that folder is gone or in the trash itself (unless it comes back in + * the same call). A name taken there by now gets " (1)" and so on. Their + * trash folders go once they are empty. * * The drive is read again first, and that is what counts: the listing on * screen (for an undo, the one from before the delete) misses a folder @@ -195,13 +196,14 @@ export async function restoreFromTrash(items: TrashedItem[], drive?: TrashDrive) return item ? [item] : []; }); if (pending.length === 0) return result; - // Names taken in each folder, so a restored item never clashes. + // Names taken in each folder, so a restored item never clashes. Folders of + // the trash count too: one restored below may be the target. const taken = new Map>(); const namesIn = (parentId: string | null): Set => { let names = taken.get(parentId); if (!names) { names = new Set(nodes - .filter((n) => (n.parentId ?? null) === parentId && !n.isShared && !hidden.has(n.id)) + .filter((n) => (n.parentId ?? null) === parentId && !n.isShared) .map((n) => n.name)); // Something named ".Trash" doesn't go back to the root under that name. if (parentId === null) names.add(TRASH_FOLDER_NAME); @@ -210,22 +212,57 @@ export async function restoreFromTrash(items: TrashedItem[], drive?: TrashDrive) return names; }; + // An item may come from a folder that is among the items too, or inside + // one of them (deleted from it before the folder itself went). Once that + // item is back, so is the folder: the item waits for it instead of going + // to the root. + const restoring = new Set(pending.map((item) => item.node.id)); + const holder = (folderId: string | null): string | null => { + const seen = new Set(); + for (let cur = folderId ? places.byId.get(folderId) : undefined; cur && hidden.has(cur.id) && !seen.has(cur.id); + cur = cur.parentId ? places.byId.get(cur.parentId) : undefined) { + seen.add(cur.id); + if (restoring.has(cur.id)) return cur.id; + } + return null; + }; + const restored = new Set(); + const tried = new Set(); const updates: Record>> = {}; const targets = new Map(); - for (const item of pending) { - const target = restoreTarget(item, places); - const used = namesIn(target); - const name = getUniqueName(item.node.name, used); - used.add(name); - // The name only goes along when it has to change: the listing shows a - // WebDAV name decoded, and sending that back would rename the file. - updates[item.node.id] = name === item.node.name ? { parentId: target } : { parentId: target, name }; - targets.set(item.node.id, target); + for (let rest = pending; rest.length > 0;) { + const waits = (item: TrashedItem) => { + const h = holder(item.fromParentId); + return h !== null && h !== item.node.id && !tried.has(h); + }; + let round = rest.filter((item) => !waits(item)); + // Only a loop of folders (impossible in a tree) leaves none ready. + if (round.length === 0) round = rest; + rest = rest.filter((item) => !round.includes(item)); + + const roundUpdates: typeof updates = {}; + for (const item of round) { + const from = item.fromParentId; + const h = holder(from); + const origin = from ? places.byId.get(from) : undefined; + const target = h !== null && restored.has(h) && origin && isFolder(origin) && !origin.isShared + ? from + : restoreTarget(item, places); + const used = namesIn(target); + const name = getUniqueName(item.node.name, used); + used.add(name); + // The name only goes along when it has to change: the listing shows a + // WebDAV name decoded, and sending that back would rename the file. + roundUpdates[item.node.id] = name === item.node.name ? { parentId: target } : { parentId: target, name }; + targets.set(item.node.id, target); + } + Object.assign(updates, roundUpdates); + assertDrive(drive); + const update = await updateFileNodes(roundUpdates); + for (const id of Object.keys(roundUpdates)) tried.add(id); + for (const id of update.updated) restored.add(id); + result.refused.push(...Object.values(update.notUpdated)); } - assertDrive(drive); - const update = await updateFileNodes(updates); - result.refused.push(...Object.values(update.notUpdated)); - const restored = new Set(update.updated); if (restored.size === 0) { throw result.refused[0] ?? new JMAPSetError('setError', 'Failed to restore'); }