fix: undo and restore against the drive as it is now, not the listing from before the delete

This commit is contained in:
Linus Rath
2026-10-29 02:57:10 +01:00
parent be04fcb179
commit 323fea7451
4 changed files with 88 additions and 30 deletions
+51 -6
View File
@@ -34,7 +34,14 @@ function serve(method: string, args: Record<string, any>) {
if (method === 'FileNode/get') return { list: store.map((n) => ({ ...n })) };
calls.push(args);
const out: Record<string, any> = {};
// Like Stalwart, two nodes of one folder can't share a name.
const clashes = (parentId: string | null, name: string, self?: string) =>
store.some((n) => n.parentId === parentId && n.name === name && n.id !== self);
for (const [key, props] of Object.entries((args.create ?? {}) as Record<string, any>)) {
if (clashes(props.parentId ?? null, props.name)) {
(out.notCreated ??= {})[key] = { type: 'alreadyExists', description: 'taken' };
continue;
}
const id = `n${nextId++}`;
store.push({ id, parentId: props.parentId ?? null, name: props.name, blobId: props.blobId ?? null });
(out.created ??= {})[key] = { id };
@@ -45,6 +52,11 @@ function serve(method: string, args: Record<string, any>) {
(out.notUpdated ??= {})[id] = { type: 'forbidden', description: 'no' };
continue;
}
const parentId = 'parentId' in patch ? patch.parentId : node.parentId;
if (clashes(parentId, patch.name ?? node.name, id)) {
(out.notUpdated ??= {})[id] = { type: 'alreadyExists', description: 'taken' };
continue;
}
Object.assign(node, patch);
(out.updated ??= {})[id] = null;
}
@@ -164,7 +176,7 @@ describe('restoreFromTrash', () => {
await moveToTrash(nodes, nodes.filter((n) => n.parentId === 'docs'), 1730112000007);
nodes = listed();
const batch = store.find((n) => n.name === '1730112000007~docs')!;
await restoreFromTrash(nodes, trashedItems(nodes));
await restoreFromTrash(trashedItems(nodes));
expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a.txt' });
expect(byId('b')).toMatchObject({ parentId: 'docs', name: 'b.txt' });
expect(byId(batch.id)).toBeUndefined();
@@ -178,7 +190,7 @@ describe('restoreFromTrash', () => {
await moveToTrash(nodes, nodes.filter((n) => n.parentId === 'docs'), 1730112000007);
nodes = listed();
const batch = store.find((n) => n.name === '1730112000007~docs')!;
await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a'));
await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a'));
expect(byId(batch.id)).toBeDefined();
expect(byId('b')!.parentId).toBe(batch.id);
});
@@ -191,26 +203,59 @@ describe('restoreFromTrash', () => {
await moveToTrash(nodes, nodes.filter((n) => n.id === 'docs'), 1730112000008);
nodes = listed();
// Docs is in the trash itself now: a.txt goes to the root.
await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a'));
await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a'));
expect(byId('a')).toMatchObject({ parentId: null, name: 'a.txt' });
nodes = listed();
await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'docs'));
await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'docs'));
await moveToTrash(listed(), listed().filter((n) => n.id === 'a2'), 1730112000009);
store.push({ id: 'a3', parentId: 'docs', name: 'a.txt', blobId: 'ba3' });
nodes = listed();
await restoreFromTrash(nodes, trashedItems(nodes).filter((i) => i.node.id === 'a2'));
await restoreFromTrash(trashedItems(nodes).filter((i) => i.node.id === 'a2'));
expect(byId('a2')).toMatchObject({ parentId: 'docs', name: 'a (1).txt' });
});
it('undoes a delete with the listing from before it, without renaming', async () => {
const before = listed();
const { done } = await moveToTrash(before, before.filter((n) => n.parentId === 'docs'), 1730112000007);
await restoreFromTrash(before, done);
await restoreFromTrash(done);
expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a.txt' });
expect(byId('b')).toMatchObject({ parentId: 'docs', name: 'b.txt' });
expect(store.some((n) => n.name === '1730112000007~docs')).toBe(false);
});
it('undoes into the root when the folder went to the trash after the delete', async () => {
const before = listed();
const { done } = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
await moveToTrash(listed(), listed().filter((n) => n.id === 'docs'), 1730112000008);
const { done: back } = await restoreFromTrash(done);
expect(back).toHaveLength(1);
expect(byId('a')).toMatchObject({ parentId: null, name: 'a.txt' });
});
it('undoes under a free name when the old one was taken after the delete', async () => {
const before = listed();
const { done } = await moveToTrash(before, before.filter((n) => n.id === 'a'), 1730112000007);
store.push({ id: 'a2', parentId: 'docs', name: 'a.txt', blobId: 'ba2' });
await restoreFromTrash(done);
expect(byId('a')).toMatchObject({ parentId: 'docs', name: 'a (1).txt' });
expect(byId('a2')).toMatchObject({ parentId: 'docs', name: 'a.txt' });
});
it('leaves an item alone that left the trash since the rows were listed', async () => {
let nodes = listed();
await moveToTrash(nodes, nodes.filter((n) => n.id === 'a'), 1730112000007);
nodes = listed();
const rows = trashedItems(nodes);
// Restored on another device and moved on from there.
byId('a')!.parentId = 'top-dir';
store.push({ id: 'top-dir', parentId: null, name: 'Elsewhere', blobId: null });
const sent = calls.length;
const { done } = await restoreFromTrash(rows);
expect(done).toEqual([]);
expect(calls.slice(sent).filter((c) => c.update)).toEqual([]);
expect(byId('a')!.parentId).toBe('top-dir');
});
});
describe('purgeFromTrash and emptyTrash', () => {
@@ -298,14 +298,14 @@ live('live Stalwart files trash', () => {
const item = all.find((i) => i.node.name === 'trash-me.txt' && i.fromParentId === scratch)!;
const sibling = all.find((i) => i.node.name === 'Trash me' && i.batchId === item.batchId)!;
let list = await own();
const { done } = await restoreFromTrash(list, [item]);
const { done } = await restoreFromTrash([item]);
expect(done).toHaveLength(1);
list = await own();
expect(list.find((n) => n.id === item.node.id)).toMatchObject({ name: 'trash-me.txt', parentId: scratch });
// The other item still holds the trash folder.
expect(list.some((n) => n.id === item.batchId)).toBe(true);
await restoreFromTrash(list, [sibling]);
await restoreFromTrash([sibling]);
list = await own();
expect(list.find((n) => n.id === sibling.node.id)?.parentId).toBe(scratch);
expect(list.some((n) => n.id === item.batchId)).toBe(false);
@@ -324,7 +324,7 @@ live('live Stalwart files trash', () => {
await deleteFileNodes([sub.id]);
list = await own();
await restoreFromTrash(list, [(await trashed(a.id))!, (await trashed(b.id))!]);
await restoreFromTrash([(await trashed(a.id))!, (await trashed(b.id))!]);
list = await own();
expect(list.find((n) => n.id === a.id)).toMatchObject({ name: 'clash (1).txt', parentId: scratch });
const orphan = list.find((n) => n.id === b.id)!;
@@ -342,7 +342,7 @@ live('live Stalwart files trash', () => {
const { done } = await moveToTrash(list, [list.find((n) => n.id === file.id)!]);
batches.add(done[0].batchId!);
// The rows on screen still show it where it was.
await restoreFromTrash(list, done);
await restoreFromTrash(done);
const after = await own();
expect(after.find((n) => n.id === file.id)).toMatchObject({ name: 'undo-me.txt', parentId: scratch });
expect(after.some((n) => n.id === done[0].batchId)).toBe(false);
+22 -11
View File
@@ -100,23 +100,34 @@ 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. Throws
* when nothing went.
* 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
* trashed since and a name taken since. Items no longer in the trash are left
* where they are. Throws when nothing of what is still there went.
*/
export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]): Promise<TrashWriteResult> {
export async function restoreFromTrash(items: TrashedItem[]): Promise<TrashWriteResult> {
const result: TrashWriteResult = { done: [], refused: [] };
if (items.length === 0) return result;
const hidden = trashSubtreeIds(nodes, findTrashFolder(nodes)?.id);
const restoring = new Set(items.map((item) => item.node.id));
// Names taken in each folder, so a restored item never clashes; the items
// themselves don't count (after an undo the listing may still show them
// where they were).
const nodes = await getAllFileNodes();
const trash = findTrashFolder(nodes);
const hidden = trashSubtreeIds(nodes, trash?.id);
// The items as the trash holds them now: their name and trash folder may
// have changed on another device.
const current = new Map(trashedItems(nodes, trash).map((item) => [item.node.id, item]));
const pending = [...new Set(items.map((item) => item.node.id))].flatMap((id) => {
const item = current.get(id);
return item ? [item] : [];
});
if (pending.length === 0) return result;
// Names taken in each folder, so a restored item never clashes.
const taken = new Map<string | null, Set<string>>();
const namesIn = (parentId: string | null): Set<string> => {
let names = taken.get(parentId);
if (!names) {
names = new Set(nodes
.filter((n) => (n.parentId ?? null) === parentId && !n.isShared && !hidden.has(n.id) && !restoring.has(n.id))
.filter((n) => (n.parentId ?? null) === parentId && !n.isShared && !hidden.has(n.id))
.map((n) => n.name));
taken.set(parentId, names);
}
@@ -125,7 +136,7 @@ export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]):
const updates: Record<string, Partial<Pick<FileNode, 'name' | 'parentId'>>> = {};
const targets = new Map<string, string | null>();
for (const item of items) {
for (const item of pending) {
const target = restoreTarget(item, nodes);
const used = namesIn(target);
const name = getUniqueName(item.node.name, used);
@@ -141,7 +152,7 @@ export async function restoreFromTrash(nodes: FileNode[], items: TrashedItem[]):
if (restored.size === 0) {
throw result.refused[0] ?? new JMAPSetError('setError', 'Failed to restore');
}
for (const item of items.filter((i) => restored.has(i.node.id))) {
for (const item of pending.filter((i) => restored.has(i.node.id))) {
const name = updates[item.node.id].name ?? item.node.name;
result.done.push({ ...item, node: { ...item.node, name, parentId: targets.get(item.node.id) ?? null } });
}
+11 -9
View File
@@ -744,12 +744,12 @@ export default function FilesScreen() {
}
}, [uploading, currentParentId, allNodes, loadFiles, t]);
// Undo of a delete: the items go back from the trash, the way Restore puts
// them back, judged against the drive as it was before the delete.
const undoTrash = useCallback(async (listing: FileNode[], items: TrashedItem[]) => {
// Undo of a delete: the items go back from the trash the way Restore puts
// them back, judged against the drive as it is by then.
const undoTrash = useCallback(async (items: TrashedItem[]) => {
try {
await restoreFromTrash(listing, items);
toast.success(t('files.undo_success', 'Action undone'));
const { done } = await restoreFromTrash(items);
if (done.length > 0) toast.success(t('files.undo_success', 'Action undone'));
} catch (e) {
toast.error(t('files.undo_error', 'Failed to undo'), fileErrorMessage(e, t));
}
@@ -770,7 +770,7 @@ export default function FilesScreen() {
}
toast.success(
t('files.moved_to_trash', '{count, plural, one {Moved to trash} other {# items moved to trash}}', { count: done.length }),
{ action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(listing, done) } },
{ action: { label: t('files.undo', 'Undo'), onPress: () => void undoTrash(done) } },
);
} catch (e) {
Alert.alert(t('files.delete_error', 'Failed to delete'), fileErrorMessage(e, t));
@@ -812,16 +812,18 @@ export default function FilesScreen() {
if (items.length === 0) return;
clearSelection();
try {
const { done, refused } = await restoreFromTrash(allNodes, items);
const { done, refused } = await restoreFromTrash(items);
if (refused.length > 0) {
Alert.alert(t('files.restore_error', "Couldn't restore"), fileErrorMessage(refused[0], t));
}
toast.success(t('files.restored', '{count, plural, one {Item restored} other {# items restored}}', { count: done.length }));
if (done.length > 0) {
toast.success(t('files.restored', '{count, plural, one {Item restored} other {# items restored}}', { count: done.length }));
}
} catch (e) {
Alert.alert(t('files.restore_error', "Couldn't restore"), fileErrorMessage(e, t));
}
await loadFiles('refresh');
}, [allNodes, clearSelection, loadFiles, t]);
}, [clearSelection, loadFiles, t]);
const performEmptyTrash = useCallback(async () => {
setConfirmEmptyTrash(false);