fix: work out where trashed items came from once per listing instead of once per row

This commit is contained in:
Linus Rath
2026-10-29 03:13:31 +01:00
parent d908f95fd7
commit 4a56e825e4
4 changed files with 72 additions and 22 deletions
+4 -3
View File
@@ -7,7 +7,7 @@ import {
import { JMAPSetError } from './jmap-result';
import { getUniqueName } from '../lib/filenode-name';
import {
TRASH_FOLDER_NAME, findTrashFolder, restoreTarget, trashBatchName, trashSubtreeIds, trashedItems,
TRASH_FOLDER_NAME, findTrashFolder, restoreTarget, trashBatchName, trashPlaces, trashSubtreeIds, trashedItems,
type TrashedItem,
} from '../lib/files-trash';
@@ -185,7 +185,8 @@ export async function restoreFromTrash(items: TrashedItem[], drive?: TrashDrive)
const nodes = await getAllFileNodes();
assertDrive(drive);
const trash = findTrashFolder(nodes);
const hidden = trashSubtreeIds(nodes, trash?.id);
const places = trashPlaces(nodes);
const { hidden } = places;
// 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]));
@@ -212,7 +213,7 @@ export async function restoreFromTrash(items: TrashedItem[], drive?: TrashDrive)
const updates: Record<string, Partial<Pick<FileNode, 'name' | 'parentId'>>> = {};
const targets = new Map<string, string | null>();
for (const item of pending) {
const target = restoreTarget(item, nodes);
const target = restoreTarget(item, places);
const used = namesIn(target);
const name = getUniqueName(item.node.name, used);
used.add(name);
+20 -1
View File
@@ -2,7 +2,7 @@ import { describe, it, expect } from 'vitest';
import type { FileNode } from '../../api/types';
import {
findTrashFolder, parseTrashBatchName, restoreTarget, takesTrashName, trashBatchName, trashOriginPath,
trashSubtreeIds, trashedItems, withoutTrash,
trashPlaces, trashSubtreeIds, trashedItems, withoutTrash,
} from '../files-trash';
// The webmail's trash convention (its lib/files-trash.ts), read the same way here.
@@ -107,4 +107,23 @@ describe('trashOriginPath', () => {
expect(trashOriginPath({ fromParentId: null }, drive)).toBe('/');
expect(trashOriginPath({ fromParentId: 'old' }, drive)).toBeNull();
});
it('reads the listing once for all the rows of the trash, not once per row', () => {
// A listing that counts how often it is gone through.
let reads = 0;
const counted = new Proxy([...drive], {
get(target, key, receiver) {
if (key === Symbol.iterator || key === 'find' || key === 'filter' || key === 'map' || key === 'some') reads++;
return Reflect.get(target, key, receiver);
},
});
const places = trashPlaces(counted);
const before = reads;
const rows = trashedItems(drive);
const paths = rows.map((item) => trashOriginPath(item, places));
expect(paths).toEqual(['/Documents/Taxes', '/Documents/Taxes', '/', '/']);
expect(reads).toBe(before);
expect(rows.map((item) => restoreTarget(item, places))).toEqual(['tax', 'tax', null, null]);
expect(reads).toBe(before);
});
});
+44 -16
View File
@@ -55,14 +55,22 @@ export function parseTrashBatchName(name: string): { deletedAt: number; parentId
export function trashSubtreeIds(nodes: FileNode[], trashId: string | undefined): Set<string> {
const ids = new Set<string>();
if (!trashId) return ids;
// One pass over the drive, then down the tree: a pass per level was slow
// for a deep trash in a large drive.
const children = new Map<string, string[]>();
for (const n of nodes) {
if (!n.parentId) continue;
const list = children.get(n.parentId);
if (list) list.push(n.id);
else children.set(n.parentId, [n.id]);
}
const queue = [trashId];
ids.add(trashId);
let grew = true;
while (grew) {
grew = false;
for (const n of nodes) {
if (n.parentId && ids.has(n.parentId) && !ids.has(n.id)) {
ids.add(n.id);
grew = true;
for (let i = 0; i < queue.length; i++) {
for (const child of children.get(queue[i]) ?? []) {
if (!ids.has(child)) {
ids.add(child);
queue.push(child);
}
}
}
@@ -102,26 +110,46 @@ export function trashedItems(nodes: FileNode[], trash: FileNode | undefined = fi
return items;
}
/**
* What restoreTarget and trashOriginPath look up in a listing, worked out
* once for it: every row of the trash asks, and each time anew it cost a
* walk of the whole drive per row.
*/
export interface TrashPlaces {
byId: Map<string, FileNode>;
/** The trash and everything in it. */
hidden: Set<string>;
}
export function trashPlaces(nodes: FileNode[]): TrashPlaces {
return { byId: new Map(nodes.map((n) => [n.id, n])), hidden: trashSubtreeIds(nodes, findTrashFolder(nodes)?.id) };
}
function placesOf(from: FileNode[] | TrashPlaces): TrashPlaces {
return Array.isArray(from) ? trashPlaces(from) : from;
}
/**
* Where a restored item goes: the folder it came from while that is still a
* folder of the user's own drive outside the trash, the root otherwise.
*/
export function restoreTarget(item: Pick<TrashedItem, 'fromParentId'>, nodes: FileNode[]): string | null {
const from = item.fromParentId;
if (from === null) return null;
const hidden = trashSubtreeIds(nodes, findTrashFolder(nodes)?.id);
const folder = nodes.find((n) => n.id === from);
return folder && isFolder(folder) && !folder.isShared && !hidden.has(folder.id) ? from : null;
export function restoreTarget(item: Pick<TrashedItem, 'fromParentId'>, from: FileNode[] | TrashPlaces): string | null {
const parentId = item.fromParentId;
if (parentId === null) return null;
const { byId, hidden } = placesOf(from);
const folder = byId.get(parentId);
return folder && isFolder(folder) && !folder.isShared && !hidden.has(folder.id) ? parentId : null;
}
/**
* The path of the folder a deleted item came from ("/Documents/Taxes", "/"
* for the root); null when that folder is gone.
*/
export function trashOriginPath(item: Pick<TrashedItem, 'fromParentId'>, nodes: FileNode[]): string | null {
export function trashOriginPath(item: Pick<TrashedItem, 'fromParentId'>, from: FileNode[] | TrashPlaces): string | null {
if (item.fromParentId === null) return '/';
if (restoreTarget(item, nodes) === null) return null;
const byId = new Map(nodes.map((n) => [n.id, n]));
const places = placesOf(from);
if (restoreTarget(item, places) === null) return null;
const { byId } = places;
const names: string[] = [];
const seen = new Set<string>();
for (let cur = byId.get(item.fromParentId); cur && !seen.has(cur.id); cur = cur.parentId ? byId.get(cur.parentId) : undefined) {
+4 -2
View File
@@ -56,7 +56,7 @@ import {
type TrashMoveResult,
} from '../api/files-trash';
import {
TRASH_FOLDER_NAME, takesTrashName, trashOriginPath, trashedItems, withoutTrash, type TrashedItem,
TRASH_FOLDER_NAME, takesTrashName, trashOriginPath, trashPlaces, trashedItems, withoutTrash, type TrashedItem,
} from '../lib/files-trash';
import { toast } from '../stores/toast-store';
import { jmapClient } from '../api/jmap-client';
@@ -281,6 +281,8 @@ export default function FilesScreen() {
// but the trash's own, hidden files shown or not.
const driveNodes = useMemo(() => withoutTrash(allNodes), [allNodes]);
const trashItems = useMemo(() => trashedItems(allNodes), [allNodes]);
// Looked up by every row of the trash; worked out once per listing.
const trashPlacesNow = useMemo(() => trashPlaces(allNodes), [allNodes]);
// Inside a folder another account shared with us, new folders and uploads
// go to that account, and only where its rights allow them. Nothing is
// added to the trash other than by deleting.
@@ -1076,7 +1078,7 @@ export default function FilesScreen() {
// Where a deleted item came from, on its row in the trash.
const trashOrigin = (item: TrashedItem): string | null => {
const from = trashOriginPath(item, allNodes);
const from = trashOriginPath(item, trashPlacesNow);
if (from === null) return null;
return t('files.deleted_from', 'Deleted from {folder}', {
folder: from === '/' ? t('files.my_files', 'My files') : from,