From c66e58ef6737e68ee463aa38dd5dc42d84608aff Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Sun, 20 Sep 2026 18:43:13 -0400 Subject: [PATCH 1/6] test: add e2e coverage for same-name restore collisions --- test/e2e/tests/helper/driveRouteMocks.ts | 109 +++++++++++++- test/e2e/tests/helper/staticData.ts | 2 + test/e2e/tests/pages/drivePage.ts | 76 ++++++++++ ...nternxt-name-collision-resolutions.spec.ts | 140 +++++++++++++++++- 4 files changed, 320 insertions(+), 7 deletions(-) diff --git a/test/e2e/tests/helper/driveRouteMocks.ts b/test/e2e/tests/helper/driveRouteMocks.ts index b4e784fe2..352d44c7f 100644 --- a/test/e2e/tests/helper/driveRouteMocks.ts +++ b/test/e2e/tests/helper/driveRouteMocks.ts @@ -10,6 +10,7 @@ const SHARDS_URL = 'https://e2e-shards.internxt.test'; const FOLDER_CONTENT_UUID_PATTERN = /\/folders\/content\/([^/?]+)/; const FILE_META_UUID_PATTERN = /\/files\/([^/]+)\/meta/; const BRIDGE_FILE_INFO_ID_PATTERN = /\/files\/([^/]+)\/info/; +const FOLDER_NAVIGATION_UUID_PATTERN = /\/folders\/([^/?]+)\/(meta|ancestors)/; const FILES_LISTING_PATH = '/files/'; const START_UPLOAD_PATH = '/files/start'; const FINISH_UPLOAD_PATH = '/files/finish'; @@ -18,6 +19,7 @@ const FOLDERS_EXISTENCE_PATH = '/folders/existence'; const HTTP_METHOD = { get: 'GET', post: 'POST', put: 'PUT', patch: 'PATCH' }; const HTTP_NOT_FOUND = 404; +const HTTP_INTERNAL_SERVER_ERROR = 500; const SHARD_CONTENT_TYPE = 'application/octet-stream'; const BRIDGE_FILE_VERSION = 2; @@ -28,6 +30,8 @@ const FIRST_CREATED_FOLDER_ID = 1000; const EXISTING_FILE_SIZE = 1024; const ITEM_TIMESTAMP = '2026-08-01T10:00:00.000Z'; const ITEM_STATUS = 'EXISTS'; +const TRASHED_ITEM_STATUS = 'TRASHED'; +const TRASH_PAGE_DEFAULT_LIMIT = 50; const GIGABYTE = 1024 ** 3; const MAX_UPLOAD_FILE_SIZE = 20 * GIGABYTE; @@ -100,6 +104,7 @@ type RecordedRequests = { export type MockedDriveOptions = { files?: ExistingFile[]; folders?: ExistingFolder[]; + trashedFiles?: TrashedFile[]; declaredSizes?: Record; isVersioningEnabled?: boolean; }; @@ -158,6 +163,16 @@ export const buildExistingFile = (id: number, plainName: string, type: string, f ); export type ExistingFile = StoredFile; +const toTrashedFile = (file: ExistingFile, originalFolderUuid: string) => ({ + ...file, + status: TRASHED_ITEM_STATUS, + parent: { uuid: originalFolderUuid, status: ITEM_STATUS }, +}); + +export const buildTrashedFile = (id: number, plainName: string, type: string, originalFolderUuid = ROOT_FOLDER_UUID) => + toTrashedFile(buildExistingFile(id, plainName, type, originalFolderUuid), originalFolderUuid); +export type TrashedFile = ReturnType; + export const buildExistingFolder = (id: number, plainName: string, parentUuid = ROOT_FOLDER_UUID) => ({ id, uuid: `existing-folder-uuid-${id}`, @@ -174,24 +189,50 @@ export const buildExistingFolder = (id: number, plainName: string, parentUuid = }); export type ExistingFolder = ReturnType; +const ROOT_FOLDER: ExistingFolder = { ...buildExistingFolder(0, 'Drive', ''), uuid: ROOT_FOLDER_UUID }; + /** - * Trashing removes items and creating a folder adds it, so follow-up listings and - * existence checks see the new state. + * Trashing, creating and moving update it, so follow-up listings and existence checks + * see the new state. */ class InMemoryDrive { private files: StoredFile[]; private folders: ExistingFolder[]; + private trashedFiles: TrashedFile[]; private readonly declaredSizes: Record; private uploadedFilesCount = 0; private thumbnailsCount = 0; private createdFoldersCount = 0; - constructor({ files = [], folders = [], declaredSizes = {} }: MockedDriveOptions) { + constructor({ files = [], folders = [], trashedFiles = [], declaredSizes = {} }: MockedDriveOptions) { this.files = [...files]; this.folders = [...folders]; + this.trashedFiles = [...trashedFiles]; this.declaredSizes = declaredSizes; } + trashedFilesPage(offset: number, limit: number) { + return this.trashedFiles.slice(offset, offset + limit).map((file) => ({ + ...file, + parent: { ...file.parent, plainName: this.findFolder(file.parent.uuid)?.plainName }, + })); + } + + moveFile({ uuid, destinationFolder, name }: MoveRequest) { + const file = [...this.files, ...this.trashedFiles].find((candidate) => candidate.uuid === uuid); + if (!file) return; + + const movedFile: ExistingFile = { + ...buildExistingFile(file.id, name ?? file.plainName, file.type, destinationFolder), + uuid: file.uuid, + fileId: file.fileId, + }; + + this.files = this.files.filter((candidate) => candidate.uuid !== uuid); + this.trashedFiles = this.trashedFiles.filter((candidate) => candidate.uuid !== uuid); + this.files.push(movedFile); + } + filesIn(folderUuid: string) { return this.files.filter((file) => file.folderUuid === folderUuid); } @@ -210,6 +251,22 @@ class InMemoryDrive { return this.foldersIn(folderUuid).filter((existing) => plainNames.includes(existing.plainName)); } + findFolder(folderUuid: string) { + return this.folders.find((folder) => folder.uuid === folderUuid); + } + + ancestorsOf(folderUuid: string) { + const ancestors: ExistingFolder[] = []; + let folder = this.findFolder(folderUuid); + + while (folder) { + ancestors.push(folder); + folder = this.findFolder(folder.parentUuid); + } + + return [...ancestors, ROOT_FOLDER]; + } + createFolder({ plainName, parentFolderUuid }: CreateFolderRequest) { const folder = buildExistingFolder(FIRST_CREATED_FOLDER_ID + this.createdFoldersCount, plainName, parentFolderUuid); this.createdFoldersCount += 1; @@ -219,6 +276,11 @@ class InMemoryDrive { trash(uuids: string[]) { const trashed = new Set(uuids); + const newlyTrashedFiles = this.files + .filter((file) => trashed.has(file.uuid)) + .map((file) => toTrashedFile(file, file.folderUuid)); + + this.trashedFiles.push(...newlyTrashedFiles); this.files = this.files.filter((file) => !trashed.has(file.uuid)); this.folders = this.folders.filter((folder) => !trashed.has(folder.uuid)); } @@ -343,6 +405,15 @@ const mockFolderContentRoutes = async (page: Page, drive: InMemoryDrive) => { ); }; +const mockFolderNavigationRoutes = async (page: Page, drive: InMemoryDrive) => { + await mockRouteForMethod(page, `${BASE_API_URL}/folders/*/meta`, HTTP_METHOD.get, (route, request) => + fulfillJsonIfFound(route, drive.findFolder(firstCapture(FOLDER_NAVIGATION_UUID_PATTERN, request.url()))), + ); + await mockRouteForMethod(page, `${BASE_API_URL}/folders/*/ancestors`, HTTP_METHOD.get, (route, request) => + route.fulfill({ json: drive.ancestorsOf(firstCapture(FOLDER_NAVIGATION_UUID_PATTERN, request.url())) }), + ); +}; + const mockFileEntryRoutes = async (page: Page, drive: InMemoryDrive, requests: RecordedRequests) => { await mockRouteForMethod(page, `${BASE_API_URL}/files`, HTTP_METHOD.post, (route, request) => { const fileEntry = request.postDataJSON() as FileEntryRequest; @@ -368,14 +439,28 @@ const mockCreateFolderRoute = (page: Page, drive: InMemoryDrive, requests: Recor return route.fulfill({ json: drive.createFolder(createFolderRequest) }); }); -const mockMoveFileRoute = (page: Page, requests: RecordedRequests) => +const mockMoveFileRoute = (page: Page, drive: InMemoryDrive, requests: RecordedRequests) => mockRouteForMethod(page, `${BASE_API_URL}/files/*`, HTTP_METHOD.patch, (route, request) => { const uuid = request.url().split('/').pop() ?? ''; const { destinationFolder, name } = request.postDataJSON() as Omit; - requests.moves.push(name ? { uuid, destinationFolder, name } : { uuid, destinationFolder }); + const moveRequest: MoveRequest = name ? { uuid, destinationFolder, name } : { uuid, destinationFolder }; + drive.moveFile(moveRequest); + requests.moves.push(moveRequest); return route.fulfill({ json: {} }); }); +export const failMovesOf = async (page: Page, file: ExistingFile | TrashedFile): Promise => { + const rejectedMoves: MoveRequest[] = []; + + await mockRouteForMethod(page, `${BASE_API_URL}/files/${file.uuid}`, HTTP_METHOD.patch, (route, request) => { + const { destinationFolder, name } = request.postDataJSON() as Omit; + rejectedMoves.push({ uuid: file.uuid, destinationFolder, name }); + return route.fulfill({ status: HTTP_INTERNAL_SERVER_ERROR, json: { message: 'Internal Server Error' } }); + }); + + return rejectedMoves; +}; + const mockReplaceFileRoute = (page: Page, requests: RecordedRequests) => mockRouteForMethod(page, `${BASE_API_URL}/files/*`, HTTP_METHOD.put, (route, request) => { requests.replacedFileUuids.push(request.url().split('/').pop() ?? ''); @@ -390,6 +475,16 @@ const mockTrashRoute = (page: Page, drive: InMemoryDrive, requests: RecordedRequ return route.fulfill({ json: {} }); }); +const mockTrashListingRoute = (page: Page, drive: InMemoryDrive) => + page.route(`${BASE_API_URL}/storage/trash/paginated**`, (route, request) => { + const query = new URL(request.url()).searchParams; + if (query.get('type') !== 'files') return route.fulfill({ json: { result: [] } }); + + const offset = Number(query.get('offset') ?? 0); + const limit = Number(query.get('limit') ?? TRASH_PAGE_DEFAULT_LIMIT); + return route.fulfill({ json: { result: drive.trashedFilesPage(offset, limit) } }); + }); + const fulfillBridgeCall = (route: Route, request: Request, bridge: InMemoryBridge, requests: RecordedRequests) => { const url = request.url(); if (url.includes(START_UPLOAD_PATH)) return route.fulfill({ json: { uploads: [bridge.startUpload()] } }); @@ -438,11 +533,13 @@ export const mockDriveRoutes = async (page: Page, options: MockedDriveOptions = await mockAppBootstrapCalls(page, options.isVersioningEnabled ?? false); await mockAuthRoutes(page); await mockFolderContentRoutes(page, drive); + await mockFolderNavigationRoutes(page, drive); await mockFileEntryRoutes(page, drive, requests); await mockCreateFolderRoute(page, drive, requests); - await mockMoveFileRoute(page, requests); + await mockMoveFileRoute(page, drive, requests); await mockReplaceFileRoute(page, requests); await mockTrashRoute(page, drive, requests); + await mockTrashListingRoute(page, drive); await mockBridgeStorageRoutes(page, requests); return requests; diff --git a/test/e2e/tests/helper/staticData.ts b/test/e2e/tests/helper/staticData.ts index c481c8c24..35cdfdc63 100644 --- a/test/e2e/tests/helper/staticData.ts +++ b/test/e2e/tests/helper/staticData.ts @@ -54,4 +54,6 @@ export const staticData = { collisionKeepBothOption: 'Keep both', collisionSkipOption: 'Skip this item', collisionApplyToAll: 'Apply this action to all duplicates', + + trashDisposalAnnouncementTitle: 'Automatic Trash disposal', }; diff --git a/test/e2e/tests/pages/drivePage.ts b/test/e2e/tests/pages/drivePage.ts index f8e12fee7..049d62ebb 100644 --- a/test/e2e/tests/pages/drivePage.ts +++ b/test/e2e/tests/pages/drivePage.ts @@ -33,6 +33,12 @@ export class DrivePage { private fileInput: Locator; private movingToTrashAndMovedSign: Locator; private folderInput: Locator; + private driveSidenavButton: Locator; + private trashSidenavButton: Locator; + private selectAllItemsCheckbox: Locator; + private restoreHeaderButton: Locator; + private trashDisposalAnnouncementTitle: Locator; + private trashDisposalAnnouncementCloseButton: Locator; constructor(page: Page) { this.page = page; @@ -77,6 +83,19 @@ export class DrivePage { this.uploadWidgetBorder = this.page.locator('[class$="border-b border-gray-10 bg-gray-5 px-3 py-2.5"]'); this.fileInput = this.page.locator('[data-test="input-file"]'); this.folderInput = this.page.locator('[data-test="input-folder"]'); + //SIDENAV + this.driveSidenavButton = this.page.locator('[data-cy="sideNavDriveIcon"]'); + //TRASH + this.trashSidenavButton = this.page.locator('[data-cy="sideNavTrashIcon"]'); + this.selectAllItemsCheckbox = this.page.locator('[data-cy="driveListHeaderCheckbox"]'); + this.restoreHeaderButton = this.page.locator('[data-tooltip-content="Restore"]').getByRole('button'); + this.trashDisposalAnnouncementTitle = this.page.getByRole('heading', { + name: staticData.trashDisposalAnnouncementTitle, + }); + this.trashDisposalAnnouncementCloseButton = this.page + .getByRole('dialog') + .filter({ has: this.trashDisposalAnnouncementTitle }) + .getByRole('button', { name: 'Close' }); } async checkFolder(folderName: string) { const folderLocator = this.allFolderNamesInDrive.filter({ hasText: folderName }); @@ -224,6 +243,63 @@ export class DrivePage { await source.dispatchEvent('dragend', { dataTransfer }); } + itemRows() { + return this.page.locator('[data-test$="-parent"]'); + } + + async expectListedItems(itemNames: string[]) { + for (const itemName of new Set(itemNames)) { + const expectedRows = itemNames.filter((name) => name === itemName).length; + await expect(this.itemRow(itemName)).toHaveCount(expectedRows, { timeout: 10000 }); + } + await expect(this.itemRows()).toHaveCount(itemNames.length); + } + + async openDrive() { + await this.driveSidenavButton.click(); + await this.page.waitForURL((url) => url.pathname === '/'); + } + + async openFolder(folderName: string) { + const folderRow = this.itemRow(folderName); + const folderNameButton = folderRow.getByRole('button', { name: folderName, exact: true }); + + await folderRow.hover(); + await folderNameButton.click(); + await this.page.waitForURL('**/folder/**'); + } + + async openFolderFromDrive(folderName: string) { + await this.openDrive(); + await this.openFolder(folderName); + } + + async openTrash() { + await this.trashSidenavButton.click(); + await this.page.waitForURL('**/trash'); + await this.closeTrashDisposalAnnouncement(); + } + + async reopenTrash() { + await this.trashSidenavButton.click(); + await this.page.waitForURL('**/trash'); + } + + async closeTrashDisposalAnnouncement() { + await expect(this.trashDisposalAnnouncementTitle).toBeVisible({ timeout: 10000 }); + await this.trashDisposalAnnouncementCloseButton.click(); + await expect(this.trashDisposalAnnouncementTitle).toBeHidden(); + } + + async selectAllItems() { + await this.selectAllItemsCheckbox.click(); + } + + async restoreSelectedItems() { + await expect(this.restoreHeaderButton).toBeVisible(); + await this.restoreHeaderButton.click(); + } + fileRow(fileName: string) { return this.page.locator(`[title="${fileName}"]`); } diff --git a/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts b/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts index 2599f82df..80468b08f 100644 --- a/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts +++ b/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts @@ -2,7 +2,14 @@ import { expect, test } from '@playwright/test'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; import { tmpdir } from 'os'; import { join } from 'path'; -import { buildExistingFile, buildExistingFolder, ROOT_FOLDER_UUID } from '../helper/driveRouteMocks'; +import { + buildExistingFile, + buildExistingFolder, + buildTrashedFile, + failMovesOf, + ROOT_FOLDER_UUID, + TrashRequest, +} from '../helper/driveRouteMocks'; import { buildUploadFile, openMockedDrive } from '../helper/mockedDrive'; import { staticData } from '../helper/staticData'; @@ -16,6 +23,25 @@ const existingDrive = { folders: [archive, photos, buildExistingFolder(3, 'Sub', photos.uuid)], }; +const archivedOther = buildExistingFile(5, 'other', 'txt', archive.uuid); +const trashedReport = buildTrashedFile(10, 'report', 'txt', archive.uuid); +const trashedReportCopy = buildTrashedFile(11, 'report', 'txt', archive.uuid); +const trashedOther = buildTrashedFile(12, 'other', 'txt', archive.uuid); + +/** + * Archive holds report.txt and other.txt; the trash holds two report.txt and one other.txt deleted from it. + * Versioning is on only to keep the Playwright UI snapshots clean (see runbook §9, "Locked feature"). + */ +const driveWithTrash = { + files: [...existingDrive.files, archivedOther], + folders: existingDrive.folders, + trashedFiles: [trashedReport, trashedReportCopy, trashedOther], + isVersioningEnabled: true, +}; + +const getTrashedUuids = (trashRequests: TrashRequest[]) => + trashRequests.flatMap((trashRequest) => trashRequest.items.map((item) => item.uuid)); + /** * A local "Photos" folder that partially overlaps with the existing one: * Photos/a.txt (exists), Photos/b.txt (new), Photos/Sub/c.txt (Sub exists, c.txt is new), @@ -168,4 +194,116 @@ test.describe('Internxt name collision resolutions', () => { await expect.poll(() => requests.replacedFileUuids, { timeout: 15000 }).toEqual([invoice.uuid]); expect(requests.trash).toHaveLength(0); }); + + test.describe('Restore collisions', () => { + test('TC9: Validate that keeping both on every restored file gives each same-name file its own numbered name', async ({ + page, + }) => { + const { drivePage, collisionDialog, requests } = await openMockedDrive(page, driveWithTrash); + + await drivePage.openTrash(); + await expect(drivePage.fileRow('report.txt')).toHaveCount(2); + await drivePage.selectAllItems(); + await drivePage.restoreSelectedItems(); + + await collisionDialog.expectOpenFor('report'); + await collisionDialog.checkApplyToAllByClickingLabel(); + await collisionDialog.selectOption(staticData.collisionKeepBothOption); + await collisionDialog.submit(); + + await collisionDialog.expectClosed(); + await expect.poll(() => requests.moves, { timeout: 10000 }).toHaveLength(3); + expect(requests.moves).toEqual( + expect.arrayContaining([ + { uuid: trashedReport.uuid, destinationFolder: archive.uuid, name: 'report (1)' }, + { uuid: trashedReportCopy.uuid, destinationFolder: archive.uuid, name: 'report (2)' }, + { uuid: trashedOther.uuid, destinationFolder: archive.uuid, name: 'other (1)' }, + ]), + ); + expect(requests.trash).toHaveLength(0); + await drivePage.expectListedItems([]); + + await drivePage.openFolderFromDrive(archive.plainName); + await drivePage.expectListedItems([ + 'report.txt', + 'report (1).txt', + 'report (2).txt', + 'other.txt', + 'other (1).txt', + ]); + }); + + test('TC10: Validate that replacing with every restored file trashes each existing file once and keeps the second same-name file under a numbered name', async ({ + page, + }) => { + const { drivePage, collisionDialog, requests } = await openMockedDrive(page, driveWithTrash); + + await drivePage.openTrash(); + await expect(drivePage.fileRow('report.txt')).toHaveCount(2); + await drivePage.selectAllItems(); + await drivePage.restoreSelectedItems(); + + await collisionDialog.expectOpenFor('report'); + await collisionDialog.checkApplyToAllByClickingLabel(); + await collisionDialog.selectOption(staticData.collisionReplaceOption); + await collisionDialog.submit(); + + await collisionDialog.expectClosed(); + await expect.poll(() => requests.moves, { timeout: 10000 }).toHaveLength(3); + + const trashedUuids = getTrashedUuids(requests.trash); + expect(trashedUuids).toHaveLength(2); + expect(trashedUuids).toEqual(expect.arrayContaining([archivedReport.uuid, archivedOther.uuid])); + + const [firstMove, secondMove, lastMove] = requests.moves; + expect([firstMove, secondMove]).toEqual( + expect.arrayContaining([ + { uuid: trashedReport.uuid, destinationFolder: archive.uuid }, + { uuid: trashedOther.uuid, destinationFolder: archive.uuid }, + ]), + ); + expect(lastMove).toEqual({ uuid: trashedReportCopy.uuid, destinationFolder: archive.uuid, name: 'report (1)' }); + await drivePage.expectListedItems([]); + + await drivePage.openFolderFromDrive(archive.plainName); + await drivePage.expectListedItems(['report.txt', 'report (1).txt', 'other.txt']); + + await drivePage.reopenTrash(); + await drivePage.expectListedItems(['report.txt', 'other.txt']); + }); + + test('TC11: Validate that a restored file whose move fails stays in the trash while the moved one leaves it', async ({ + page, + }) => { + const { drivePage, collisionDialog, requests } = await openMockedDrive(page, { + ...driveWithTrash, + trashedFiles: [trashedReport, trashedOther], + }); + const rejectedMoves = await failMovesOf(page, trashedOther); + + await drivePage.openTrash(); + await expect(drivePage.fileRow('report.txt')).toHaveCount(1); + await expect(drivePage.fileRow('other.txt')).toHaveCount(1); + await drivePage.selectAllItems(); + await drivePage.restoreSelectedItems(); + + await collisionDialog.expectOpenFor('report'); + await collisionDialog.checkApplyToAllByClickingLabel(); + await collisionDialog.selectOption(staticData.collisionKeepBothOption); + await collisionDialog.submit(); + + await collisionDialog.expectClosed(); + await expect + .poll(() => rejectedMoves, { timeout: 10000 }) + .toEqual([{ uuid: trashedOther.uuid, destinationFolder: archive.uuid, name: 'other (1)' }]); + await drivePage.expectListedItems(['other.txt']); + expect(requests.moves).toEqual([ + { uuid: trashedReport.uuid, destinationFolder: archive.uuid, name: 'report (1)' }, + ]); + + await drivePage.openFolderFromDrive(archive.plainName); + await drivePage.expectListedItems(['report.txt', 'report (1).txt', 'other.txt']); + await expect(drivePage.itemRow('other (1).txt')).toHaveCount(0); + }); + }); }); From 8322c322b110f7093efa73140fb895b6c760215f Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Sun, 20 Sep 2026 18:42:17 -0400 Subject: [PATCH 2/6] fix: resolve same-name items within a move collision batch --- .../nameCollision.actions.test.ts | 196 ++++++++++++++++-- .../nameCollision.actions.ts | 156 +++++++++++--- .../nameCollision.utils.test.ts | 72 +++++++ .../nameCollision.utils.ts | 49 +++++ 4 files changed, 429 insertions(+), 44 deletions(-) diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts index a35724a5e..427c1da20 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, test, vi } from 'vitest'; import { getDriveItemData } from 'testUtils/fixtures/drive.fixtures'; +import { DriveItemData } from 'app/drive/types'; import { IRoot } from 'app/store/slices/storage/types'; import { NameCollisionContext, ResolveCollisionParams, resolveCollision } from './nameCollision.actions'; @@ -85,6 +86,63 @@ const getContext = (overrides: Partial = {}): NameCollisio ...overrides, }); +const asRenamed = (item: DriveItemData, name: string) => { + const renamedItem = { ...item, name, plain_name: name, newItemName: name }; + return item.isFolder ? renamedItem : { ...renamedItem, plainName: name }; +}; + +const expectMovesThenPop = (movedPayloads: unknown[], poppedItems: DriveItemData[]) => + expect(mocks.dispatch.mock.calls).toEqual([ + ...movedPayloads.map((payload) => [asMoveAction({ items: [payload], destinationFolderId: DESTINATION })]), + [asPopAction(poppedItems)], + ]); + +const getNextFreeName = (name: string, takenItems: unknown[]) => `${name} (${takenItems.length})`; +const getNextFreeFolderName = async (name: string, takenItems: unknown[]) => getNextFreeName(name, takenItems); +const getNextFreeFilename = async (name: string, _type: string, takenItems: unknown[]) => + getNextFreeName(name, takenItems); + +const findSameUniqueFilenameTwice = () => + mocks.getUniqueFilename + .mockResolvedValueOnce('report (2)') + .mockResolvedValueOnce('report (2)') + .mockResolvedValueOnce('report (3)'); + +/** + * Keeps the lookup of `heldName` pending until the lookup of another name starts, and returns the + * order in which the lookups start and end. + */ +const holdFilenameLookupUntilOthersStart = (heldName: string): string[] => { + const lookupOrder: string[] = []; + let releaseHeldLookup = () => {}; + const untilOtherLookupStarts = new Promise((release) => { + releaseHeldLookup = release; + }); + + mocks.getUniqueFilename.mockImplementation(async (name: string) => { + lookupOrder.push(`start ${name}`); + if (name === heldName) { + await untilOtherLookupStarts; + } else { + releaseHeldLookup(); + } + lookupOrder.push(`end ${name}`); + return `${name} (1)`; + }); + + return lookupOrder; +}; + +const failMovesOf = (failingItems: DriveItemData[]) => + mocks.dispatch.mockImplementation((action?: { payload?: { items?: DriveItemData[] } }) => ({ + unwrap: async () => { + const hasFailingItem = action?.payload?.items?.some((item) => failingItems.includes(item)); + if (hasFailingItem) throw new Error('move failed'); + }, + })); + +const succeedAllMoves = () => failMovesOf([]); + const resolve = (params: Omit, context = getContext()) => resolveCollision({ ...params, destinationUuid: DESTINATION }, context); @@ -93,6 +151,7 @@ const resolve = (params: Omit, contex */ beforeEach(() => { vi.resetAllMocks(); + succeedAllMoves(); mocks.checkDuplicatedFiles.mockResolvedValue({ duplicatedFilesResponse: ['file-dup'] }); mocks.getUniqueFilename.mockResolvedValue('report (1)'); mocks.checkFolderDuplicated.mockResolvedValue({ duplicatedFoldersResponse: ['folder-dup'] }); @@ -130,23 +189,121 @@ describe('resolveCollision', () => { test('when moving with keep, then items get a unique name and leave the pending deletion list', async () => { const file = getDriveItemData({ plainName: 'report', name: 'report', type: 'pdf', isFolder: false }); const folder = getDriveItemData({ plainName: 'Photos', name: 'Photos', isFolder: true }); - const renamedMove = { - items: [ - { ...file, name: 'report (1)', plainName: 'report (1)', plain_name: 'report (1)', newItemName: 'report (1)' }, - { ...folder, name: 'Photos (1)', plain_name: 'Photos (1)', newItemName: 'Photos (1)' }, - ], - destinationFolderId: DESTINATION, - }; await resolve({ operationType: 'move', operation: 'keep', items: [file, folder], existingItems: [] }); expect(mocks.getUniqueFilename).toHaveBeenCalledWith('report', 'pdf', ['file-dup'], DESTINATION); expect(mocks.getUniqueFolderName).toHaveBeenCalledWith('Photos', ['folder-dup'], DESTINATION); expect(mocks.moveItemsToTrash).not.toHaveBeenCalled(); - expect(mocks.dispatch.mock.calls).toEqual([[asMoveAction(renamedMove)], [asPopAction([file, folder])]]); + expectMovesThenPop([asRenamed(file, 'report (1)'), asRenamed(folder, 'Photos (1)')], [file, folder]); + }); + + test('when two moved files share name and extension and the user keeps both, then they are moved under different unique names', async () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'report', name: 'report', type: 'pdf' }); + const second = getDriveItemData({ uuid: 'second', plainName: 'report', name: 'report', type: 'pdf' }); + mocks.getUniqueFilename.mockImplementation(getNextFreeFilename); + + await resolve({ operationType: 'move', operation: 'keep', items: [first, second], existingItems: [] }); + + expectMovesThenPop([asRenamed(first, 'report (1)'), asRenamed(second, 'report (2)')], [first, second]); + }); + + test('when two moved folders share a name and the user keeps both, then they are moved under different unique names', async () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'Photos', name: 'Photos', isFolder: true }); + const second = getDriveItemData({ uuid: 'second', plainName: 'Photos', name: 'Photos', isFolder: true }); + const file = getDriveItemData({ uuid: 'file', plainName: 'Photos', name: 'Photos', type: 'pdf' }); + mocks.getUniqueFilename.mockImplementation(getNextFreeFilename); + mocks.getUniqueFolderName.mockImplementation(getNextFreeFolderName); + + await resolve({ operationType: 'move', operation: 'keep', items: [first, file, second], existingItems: [] }); + + expectMovesThenPop( + [asRenamed(first, 'Photos (1)'), asRenamed(second, 'Photos (2)'), asRenamed(file, 'Photos (1)')], + [first, second, file], + ); + }); + + test('when the unique name found for a kept file was already given to another kept file, then a new name is looked for among the names already given', async () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'report', name: 'report', type: 'pdf' }); + const second = getDriveItemData({ uuid: 'second', plainName: 'report', name: 'report', type: 'pdf' }); + const firstRenamed = expect.objectContaining({ uuid: 'first', plainName: 'report (2)' }); + findSameUniqueFilenameTwice(); + + await resolve({ operationType: 'move', operation: 'keep', items: [first, second], existingItems: [] }); + + expect(mocks.getUniqueFilename).toHaveBeenLastCalledWith( + 'report (2)', + 'pdf', + ['file-dup', firstRenamed], + DESTINATION, + ); + expectMovesThenPop([asRenamed(first, 'report (2)'), asRenamed(second, 'report (3)')], [first, second]); + }); + + test('when two kept files whose names only differ in the number at the end are given the same unique name, then the second one gets a new name', async () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'report', name: 'report', type: 'pdf' }); + const second = getDriveItemData({ uuid: 'second', plainName: 'report (1)', name: 'report (1)', type: 'pdf' }); + findSameUniqueFilenameTwice(); + + await resolve({ operationType: 'move', operation: 'keep', items: [first, second], existingItems: [] }); + + expectMovesThenPop([asRenamed(first, 'report (2)'), asRenamed(second, 'report (3)')], [first, second]); }); - test('when moving with replace, then matched existing items are trashed before moving and every moved item leaves the pending deletion list', async () => { + test('when kept items have different names, then their unique names are looked up in parallel', async () => { + const report = getDriveItemData({ uuid: 'report', plainName: 'report', name: 'report', type: 'pdf' }); + const other = getDriveItemData({ uuid: 'other', plainName: 'other', name: 'other', type: 'pdf' }); + const lookupOrder = holdFilenameLookupUntilOthersStart('report'); + + await resolve({ operationType: 'move', operation: 'keep', items: [report, other], existingItems: [] }); + + expect(lookupOrder).toEqual(['start report', 'start other', 'end other', 'end report']); + }); + + test('when one move fails, then only the successfully moved items are removed from the items to delete', async () => { + const moved = getDriveItemData({ uuid: 'moved', plainName: 'report', type: 'pdf' }); + const failed = getDriveItemData({ uuid: 'failed', plainName: 'other', type: 'pdf' }); + const existingItems = [ + getDriveItemData({ uuid: 'existing-report', plainName: 'report', type: 'pdf' }), + getDriveItemData({ uuid: 'existing-other', plainName: 'other', type: 'pdf' }), + ]; + failMovesOf([failed]); + + await resolve({ operationType: 'move', operation: 'replace', items: [moved, failed], existingItems }); + + expectMovesThenPop([moved, failed], [moved]); + }); + + test('when two moved files collide with the same existing file and the user replaces, then the existing file is trashed once, the first item is moved with its own name and the second is moved with a unique name', async () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'report', name: 'report', type: 'pdf' }); + const second = getDriveItemData({ uuid: 'second', plainName: 'report', name: 'report', type: 'pdf' }); + const other = getDriveItemData({ uuid: 'other', plainName: 'other', name: 'other', type: 'pdf' }); + const existing = getDriveItemData({ uuid: 'existing', plainName: 'report', type: 'pdf' }); + const existingOther = getDriveItemData({ uuid: 'existing-other', plainName: 'other', type: 'pdf' }); + const callOrder: string[] = []; + mocks.moveItemsThunk.mockImplementation((payload: { items: DriveItemData[] }) => { + callOrder.push(`move ${payload.items[0].uuid}`); + return asMoveAction(payload); + }); + mocks.getUniqueFilename.mockImplementation(async () => { + callOrder.push('rename'); + return 'report (1)'; + }); + + await resolve({ + operationType: 'move', + operation: 'replace', + items: [first, second, other], + existingItems: [existing, existingOther], + }); + + expect(mocks.moveItemsToTrash).toHaveBeenCalledTimes(1); + expect(mocks.moveItemsToTrash).toHaveBeenCalledWith([existing, existingOther]); + expect(callOrder).toEqual(['move first', 'move other', 'rename', 'move second']); + expectMovesThenPop([first, other, asRenamed(second, 'report (1)')], [first, other, second]); + }); + + test('when moving with replace, then matched existing items are trashed before moving and only the moved items leave the pending deletion list', async () => { const matched = getDriveItemData({ uuid: 'matched', plainName: 'report', type: 'pdf' }); const unmatched = getDriveItemData({ uuid: 'unmatched', plainName: 'other', type: 'pdf' }); const existing = getDriveItemData({ uuid: 'existing', plainName: 'report', type: 'pdf' }); @@ -166,10 +323,7 @@ describe('resolveCollision', () => { expect(mocks.moveItemsToTrash).toHaveBeenCalledWith([existing]); expect(callOrder).toEqual(['trash', 'move']); - expect(mocks.dispatch.mock.calls).toEqual([ - [asMoveAction({ items: [matched], destinationFolderId: DESTINATION })], - [asPopAction([matched, unmatched])], - ]); + expectMovesThenPop([matched], [matched]); }); test('when uploading with keep, then folders upload with tracking, files upload with the duplicates check, and the folder is refreshed', async () => { @@ -297,8 +451,20 @@ describe('resolveCollision', () => { expect.objectContaining({ payload: [{ root: { ...newSubfolder }, currentFolderId: 'photos-uuid' }] }), ); expect(mocks.dispatch.mock.calls).toEqual([ - [asUploadAction({ files: [newFile], parentFolderId: 'photos-uuid', options: { disableDuplicatedNamesCheck: true } })], - [asUploadAction({ files: [nestedFile], parentFolderId: 'sub-uuid', options: { disableDuplicatedNamesCheck: true } })], + [ + asUploadAction({ + files: [newFile], + parentFolderId: 'photos-uuid', + options: { disableDuplicatedNamesCheck: true }, + }), + ], + [ + asUploadAction({ + files: [nestedFile], + parentFolderId: 'sub-uuid', + options: { disableDuplicatedNamesCheck: true }, + }), + ], [asRefreshAction(DESTINATION)], ]); }); diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts index 1a9594233..c6cb2fdf0 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts @@ -20,7 +20,15 @@ import { IRoot } from 'app/store/slices/storage/types'; import { isVersioningExtensionAllowed } from 'views/Drive/components/VersionHistory/utils'; import replaceFileService from 'views/Drive/services/replaceFile.service'; import { moveItemsToTrash } from 'views/Trash/services'; -import { CollisionItem, CollisionPair, getCollisionPairs, isFolderUpload } from './nameCollision.utils'; +import { + CollisionItem, + CollisionPair, + getCollisionPairs, + groupByNameSeries, + isFolderUpload, + isNameTakenBy, + splitReplacingPairs, +} from './nameCollision.utils'; export type CollisionOperationType = 'move' | 'upload'; export type CollisionOperation = 'keep' | 'replace' | 'skip'; @@ -45,20 +53,107 @@ type ResolveUploadCollisionParams = Omit => { +interface ItemMove { + item: DriveItemData; + payload: MoveItemPayload; +} + +const getDestinationDuplicates = async (item: DriveItemData, destinationUuid: string): Promise => { if (item.isFolder) { const { duplicatedFoldersResponse } = await checkFolderDuplicated([item], destinationUuid); - const finalName = await getUniqueFolderName( - item.plainName ?? item.name, - duplicatedFoldersResponse as DriveItemData[], - destinationUuid, - ); - return { ...item, name: finalName, plain_name: finalName, newItemName: finalName }; + return duplicatedFoldersResponse as DriveItemData[]; } const { duplicatedFilesResponse } = await checkDuplicatedFiles([item], destinationUuid); - const finalName = await getUniqueFilename(item.name, item.type, duplicatedFilesResponse, destinationUuid); - return { ...item, name: finalName, plainName: finalName, plain_name: finalName, newItemName: finalName }; + return duplicatedFilesResponse as DriveItemData[]; +}; + +const getLookupName = (item: DriveItemData): string => (item.isFolder ? (item.plainName ?? item.name) : item.name); + +const getNextUniqueName = ( + item: DriveItemData, + name: string, + takenItems: DriveItemData[], + destinationUuid: string, +): Promise => { + if (item.isFolder) { + return getUniqueFolderName(name, takenItems, destinationUuid); + } + + return getUniqueFilename(name, item.type, takenItems, destinationUuid); +}; + +/** + * Re-checks the names already assigned in this batch after every attempt, because the server knows + * nothing about names that are assigned but not moved yet. + */ +const getUniqueNameInBatch = async ( + item: DriveItemData, + destinationUuid: string, + renamedItems: DriveItemData[], +): Promise => { + const destinationDuplicates = await getDestinationDuplicates(item, destinationUuid); + const takenItems = [...destinationDuplicates, ...renamedItems]; + let uniqueName = getLookupName(item); + + do { + uniqueName = await getNextUniqueName(item, uniqueName, takenItems, destinationUuid); + } while (isNameTakenBy(uniqueName, item, renamedItems)); + + return uniqueName; +}; + +const getRenamedMovePayload = (item: DriveItemData, newName: string): MoveItemPayload => { + const renamedItem = { ...item, name: newName, plain_name: newName, newItemName: newName }; + return item.isFolder ? renamedItem : { ...renamedItem, plainName: newName }; +}; + +const getSeriesMoves = async (seriesItems: DriveItemData[], destinationUuid: string): Promise => { + const renamedItems: DriveItemData[] = []; + const moves: ItemMove[] = []; + + for (const item of seriesItems) { + const uniqueName = await getUniqueNameInBatch(item, destinationUuid, renamedItems); + renamedItems.push({ ...item, name: uniqueName, plainName: uniqueName }); + moves.push({ item, payload: getRenamedMovePayload(item, uniqueName) }); + } + + return moves; +}; + +const getUniqueNameMoves = async (items: DriveItemData[], destinationUuid: string): Promise => { + const series = groupByNameSeries(items, getLookupName); + const movesBySeries = await Promise.all(series.map((seriesItems) => getSeriesMoves(seriesItems, destinationUuid))); + + return movesBySeries.flat(); +}; + +const moveItem = async ( + payload: MoveItemPayload, + destinationUuid: string, + { dispatch }: NameCollisionContext, +): Promise => { + try { + await dispatch(storageThunks.moveItemsThunk({ items: [payload], destinationFolderId: destinationUuid })).unwrap(); + return true; + } catch { + return false; + } +}; + +/** + * Moves every item on its own, because the move thunk settles once for the whole batch and would + * hide which items made it to the destination when one of them fails. + */ +const moveItems = async ( + moves: ItemMove[], + destinationUuid: string, + context: NameCollisionContext, +): Promise => { + const isMovedByIndex = await Promise.all(moves.map(({ payload }) => moveItem(payload, destinationUuid, context))); + const succeededMoves = moves.filter((_, index) => isMovedByIndex[index]); + + return succeededMoves.map(({ item }) => item); }; /** @@ -67,12 +162,12 @@ const getUniqueNameMovePayload = async (item: DriveItemData, destinationUuid: st const keepAndMoveItems = async ( items: DriveItemData[], destinationUuid: string, - { dispatch }: NameCollisionContext, -): Promise => { - if (items.length === 0) return; + context: NameCollisionContext, +): Promise => { + if (items.length === 0) return []; - const itemsParsed = await Promise.all(items.map((item) => getUniqueNameMovePayload(item, destinationUuid))); - await dispatch(storageThunks.moveItemsThunk({ items: itemsParsed, destinationFolderId: destinationUuid })); + const moves = await getUniqueNameMoves(items, destinationUuid); + return moveItems(moves, destinationUuid, context); }; /** @@ -81,22 +176,23 @@ const keepAndMoveItems = async ( const replaceAndMoveItems = async ( pairs: CollisionPair[], destinationUuid: string, - { dispatch }: NameCollisionContext, -): Promise => { - if (pairs.length === 0) return; + context: NameCollisionContext, +): Promise => { + if (pairs.length === 0) return []; - await moveItemsToTrash(pairs.map((pair) => pair.existing)); - await dispatch( - storageThunks.moveItemsThunk({ - items: pairs.map((pair) => pair.item), - destinationFolderId: destinationUuid, - }), - ); + const { replacingPairs, leftoverItems } = splitReplacingPairs(pairs); + const replacingMoves: ItemMove[] = replacingPairs.map(({ item }) => ({ item, payload: item })); + + await moveItemsToTrash(replacingPairs.map((pair) => pair.existing)); + const replacingItems = await moveItems(replacingMoves, destinationUuid, context); + const keptItems = await keepAndMoveItems(leftoverItems, destinationUuid, context); + + return [...replacingItems, ...keptItems]; }; /** * Applies the chosen resolution to items that collide while being moved, then removes them - * from the pending-deletion list. Skipped items are left untouched. + * from the pending-deletion list once moved. Skipped items are left untouched. */ const resolveMoveCollision = async ( { operation, items, existingItems, destinationUuid }: ResolveMoveCollisionParams, @@ -104,12 +200,14 @@ const resolveMoveCollision = async ( ) => { if (operation === 'skip') return; + let movedItems: DriveItemData[]; + if (operation === 'keep') { - await keepAndMoveItems(items, destinationUuid, context); + movedItems = await keepAndMoveItems(items, destinationUuid, context); } else { - await replaceAndMoveItems(getCollisionPairs(items, existingItems), destinationUuid, context); + movedItems = await replaceAndMoveItems(getCollisionPairs(items, existingItems), destinationUuid, context); } - context.dispatch(storageActions.popItemsToDelete(items)); + context.dispatch(storageActions.popItemsToDelete(movedItems)); }; const uploadFileAndGetFileId = async ( diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts index 2fcdc8a54..b82163698 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts @@ -7,8 +7,12 @@ import { findExistingItemFor, findPendingGroupIndex, getCollisionPairs, + getNameSeriesKey, getRemainingGroups, + groupByNameSeries, isFolderUpload, + isNameTakenBy, + splitReplacingPairs, } from './nameCollision.utils'; const getRoot = (name = 'Photos'): IRoot => ({ @@ -121,3 +125,71 @@ describe('getRemainingGroups', () => { expect(getRemainingGroups(groups, 0, undefined)).toEqual([otherGroup]); }); }); + +describe('splitReplacingPairs', () => { + test('when several items collide with the same existing item, then only the first one replaces it and the rest are left over', () => { + const first = getDriveItemData({ uuid: 'first', plainName: 'report', type: 'pdf' }); + const second = getDriveItemData({ uuid: 'second', plainName: 'report', type: 'pdf' }); + const folder = getDriveItemData({ uuid: 'moved-folder', plainName: 'report', isFolder: true }); + + expect(splitReplacingPairs(getCollisionPairs([first, folder, second], existingItems))).toEqual({ + replacingPairs: [ + { item: first, existing: existingFile }, + { item: folder, existing: existingFolder }, + ], + leftoverItems: [second], + }); + }); +}); + +describe('isNameTakenBy', () => { + const movedFile = getDriveItemData({ plainName: 'draft', type: 'pdf', isFolder: false }); + const movedFolder = getDriveItemData({ plainName: 'draft', type: null as never, isFolder: true }); + + test.each<[string, string, ReturnType, boolean]>([ + ['a file of the same extension holds the name', 'report', movedFile, true], + ['only a file of another extension holds the name', 'report', { ...movedFile, type: 'docx' }, false], + ['a folder holds the name wanted by a folder', 'report', movedFolder, true], + ['only a file holds the name wanted by a folder', 'README', movedFolder, false], + ['nothing holds the name', 'draft', movedFile, false], + ])('when %s, then it is reported accordingly', (_, name, item, expected) => { + expect(isNameTakenBy(name, item, existingItems)).toBe(expected); + }); +}); + +describe('getNameSeriesKey', () => { + const file = getDriveItemData({ type: 'pdf', isFolder: false }); + const folder = getDriveItemData({ type: null as never, isFolder: true }); + + test.each<[string, ReturnType, string, boolean]>([ + ['the same name and extension', file, 'report', true], + ['the same name with an increment', file, 'report (1)', true], + ['the same name in another casing', { ...file, type: 'PDF' }, 'Report (12)', true], + ['another name', file, 'other', false], + ['the same name with another extension', { ...file, type: 'docx' }, 'report', false], + ['the same name as a folder', folder, 'report', false], + ['an increment that is not at the end of the name', file, 'report (1) final', false], + ])('when a file is compared with %s, then the series match is reported', (_, item, name, expected) => { + const isSameSeries = getNameSeriesKey(item, name) === getNameSeriesKey(file, 'report'); + + expect(isSameSeries).toBe(expected); + }); + + test('when two folders share a name and only differ in the increment and the type, then they belong to the same series', () => { + const typedFolder = { ...folder, type: 'pdf' }; + + expect(getNameSeriesKey(typedFolder, 'report (2)')).toBe(getNameSeriesKey(folder, 'report')); + }); +}); + +describe('groupByNameSeries', () => { + test('when some items belong to the same name series, then they are grouped together in their given order', () => { + const first = getDriveItemData({ uuid: 'first', name: 'report', type: 'pdf', isFolder: false }); + const other = getDriveItemData({ uuid: 'other', name: 'other', type: 'pdf', isFolder: false }); + const second = getDriveItemData({ uuid: 'second', name: 'report (1)', type: 'pdf', isFolder: false }); + + const series = groupByNameSeries([first, other, second], (item) => item.name); + + expect(series).toEqual([[first, second], [other]]); + }); +}); diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts index 60079cd23..755a4e8cc 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts @@ -74,3 +74,52 @@ export const getRemainingGroups = ( : group, ) .filter(hasDuplicatedItems); + +export interface ReplacingPairsSplit { + replacingPairs: CollisionPair[]; + leftoverItems: T[]; +} + +export const splitReplacingPairs = (pairs: CollisionPair[]): ReplacingPairsSplit => { + const replacedUuids = new Set(); + + return pairs.reduce>( + (split, pair) => { + if (replacedUuids.has(pair.existing.uuid)) { + split.leftoverItems.push(pair.item); + } else { + replacedUuids.add(pair.existing.uuid); + split.replacingPairs.push(pair); + } + return split; + }, + { replacingPairs: [], leftoverItems: [] }, + ); +}; + +export const isNameTakenBy = (name: string, item: DriveItemData, takenItems: DriveItemData[]): boolean => + !!findExistingItemFor({ ...item, name, plainName: name }, takenItems); + +const INCREMENT_SUFFIX_REGEX = / \(\d+\)$/; + +export const getNameSeriesKey = (item: DriveItemData, name: string): string => { + const baseName = name.replace(INCREMENT_SUFFIX_REGEX, '').toLowerCase(); + const extension = (item.type ?? '').toLowerCase(); + + return JSON.stringify(item.isFolder ? ['folder', baseName] : ['file', baseName, extension]); +}; + +export const groupByNameSeries = ( + items: DriveItemData[], + getName: (item: DriveItemData) => string, +): DriveItemData[][] => { + const itemsBySeriesKey = new Map(); + + for (const item of items) { + const seriesKey = getNameSeriesKey(item, getName(item)); + const seriesItems = itemsBySeriesKey.get(seriesKey) ?? []; + itemsBySeriesKey.set(seriesKey, [...seriesItems, item]); + } + + return [...itemsBySeriesKey.values()]; +}; From f9e798bb628fc0f125c52cdf96bd8d286c458da4 Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Sun, 20 Sep 2026 21:54:56 -0400 Subject: [PATCH 3/6] test: add e2e coverage for same-name restore collisions resolved one at a time --- test/e2e/tests/pages/drivePage.ts | 7 ++ .../tests/pages/nameCollisionDialogPage.ts | 13 ++++ ...nternxt-name-collision-resolutions.spec.ts | 78 +++++++++++++++++++ 3 files changed, 98 insertions(+) diff --git a/test/e2e/tests/pages/drivePage.ts b/test/e2e/tests/pages/drivePage.ts index 049d62ebb..00268706b 100644 --- a/test/e2e/tests/pages/drivePage.ts +++ b/test/e2e/tests/pages/drivePage.ts @@ -300,6 +300,13 @@ export class DrivePage { await this.restoreHeaderButton.click(); } + async restoreAllFromTrash(trashedItemNames: string[]) { + await this.openTrash(); + await this.expectListedItems(trashedItemNames); + await this.selectAllItems(); + await this.restoreSelectedItems(); + } + fileRow(fileName: string) { return this.page.locator(`[title="${fileName}"]`); } diff --git a/test/e2e/tests/pages/nameCollisionDialogPage.ts b/test/e2e/tests/pages/nameCollisionDialogPage.ts index 884cc4cba..eb6949456 100644 --- a/test/e2e/tests/pages/nameCollisionDialogPage.ts +++ b/test/e2e/tests/pages/nameCollisionDialogPage.ts @@ -37,6 +37,13 @@ export class NameCollisionDialogPage { await expect(this.applyToAllLabel).toBeVisible({ visible: isVisible }); } + /** Between two same-name items the dialog text does not change: only "apply to all" disappears. */ + async expectReadyForLastItem() { + await expect(this.applyToAllLabel).toBeHidden({ timeout: 10000 }); + await expect(this.title).toBeVisible(); + await expect(this.submitButton).toBeEnabled(); + } + async selectOption(optionName: string) { await this.dialog.getByRole('radio', { name: optionName }).click(); } @@ -56,4 +63,10 @@ export class NameCollisionDialogPage { await this.selectOption(optionName); await this.submit(); } + + async resolveLastItem(optionName: string) { + await this.expectReadyForLastItem(); + await this.selectOption(optionName); + await this.submit(); + } } diff --git a/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts b/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts index 80468b08f..bbb82cf8c 100644 --- a/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts +++ b/test/e2e/tests/specs/DRIVE-internxt-name-collision-resolutions.spec.ts @@ -7,11 +7,14 @@ import { buildExistingFolder, buildTrashedFile, failMovesOf, + MoveRequest, ROOT_FOLDER_UUID, + TrashedFile, TrashRequest, } from '../helper/driveRouteMocks'; import { buildUploadFile, openMockedDrive } from '../helper/mockedDrive'; import { staticData } from '../helper/staticData'; +import { DrivePage } from '../pages/drivePage'; const archive = buildExistingFolder(1, 'Archive'); const photos = buildExistingFolder(2, 'Photos'); @@ -39,9 +42,35 @@ const driveWithTrash = { isVersioningEnabled: true, }; +/** Archive holds one report.txt; the trash holds two report.txt deleted from it and nothing else collides. */ +const driveWithSameNameTrash = { + ...driveWithTrash, + files: existingDrive.files, + trashedFiles: [trashedReport, trashedReportCopy], +}; + const getTrashedUuids = (trashRequests: TrashRequest[]) => trashRequests.flatMap((trashRequest) => trashRequest.items.map((item) => item.uuid)); +const archivedReportTrashedOnce: TrashRequest[] = [{ items: [{ uuid: archivedReport.uuid, type: 'file' }] }]; + +const buildMoveToArchive = (trashedFile: TrashedFile, newName?: string): MoveRequest => { + const move = { uuid: trashedFile.uuid, destinationFolder: archive.uuid }; + return newName ? { ...move, name: newName } : move; +}; + +/** Reloads both lists from the server the way a user would: Drive → Archive, then back to the trash. */ +const expectArchiveAndTrash = async ( + drivePage: DrivePage, + { archiveItems, trashItems }: { archiveItems: string[]; trashItems: string[] }, +) => { + await drivePage.openFolderFromDrive(archive.plainName); + await drivePage.expectListedItems(archiveItems); + + await drivePage.reopenTrash(); + await drivePage.expectListedItems(trashItems); +}; + /** * A local "Photos" folder that partially overlaps with the existing one: * Photos/a.txt (exists), Photos/b.txt (new), Photos/Sub/c.txt (Sub exists, c.txt is new), @@ -305,5 +334,54 @@ test.describe('Internxt name collision resolutions', () => { await drivePage.expectListedItems(['report.txt', 'report (1).txt', 'other.txt']); await expect(drivePage.itemRow('other (1).txt')).toHaveCount(0); }); + + test('TC12: Validate that replacing with two same-name restored files one at a time trashes the existing file only once', async ({ + page, + }) => { + const { drivePage, collisionDialog, requests } = await openMockedDrive(page, driveWithSameNameTrash); + const replacingMove = buildMoveToArchive(trashedReport); + const numberedMove = buildMoveToArchive(trashedReportCopy, 'report (1)'); + + await drivePage.restoreAllFromTrash(['report.txt', 'report.txt']); + await collisionDialog.expectApplyToAllVisible(true); + await collisionDialog.resolve('report', staticData.collisionReplaceOption); + await expect.poll(() => requests.moves, { timeout: 10000 }).toEqual([replacingMove]); + expect(requests.trash).toEqual(archivedReportTrashedOnce); + + await collisionDialog.resolveLastItem(staticData.collisionReplaceOption); + await collisionDialog.expectClosed(); + await expect.poll(() => requests.moves, { timeout: 10000 }).toEqual([replacingMove, numberedMove]); + expect(requests.trash).toEqual(archivedReportTrashedOnce); + + await drivePage.expectListedItems([]); + await expectArchiveAndTrash(drivePage, { + archiveItems: ['report.txt', 'report (1).txt'], + trashItems: ['report.txt'], + }); + }); + + test('TC13: Validate that replacing after keeping both on a same-name restored file still trashes the existing file', async ({ + page, + }) => { + const { drivePage, collisionDialog, requests } = await openMockedDrive(page, driveWithSameNameTrash); + const numberedMove = buildMoveToArchive(trashedReport, 'report (1)'); + const replacingMove = buildMoveToArchive(trashedReportCopy); + + await drivePage.restoreAllFromTrash(['report.txt', 'report.txt']); + await collisionDialog.resolve('report', staticData.collisionKeepBothOption); + await expect.poll(() => requests.moves, { timeout: 10000 }).toEqual([numberedMove]); + expect(requests.trash).toHaveLength(0); + + await collisionDialog.resolveLastItem(staticData.collisionReplaceOption); + await collisionDialog.expectClosed(); + await expect.poll(() => requests.moves, { timeout: 10000 }).toEqual([numberedMove, replacingMove]); + expect(requests.trash).toEqual(archivedReportTrashedOnce); + + await drivePage.expectListedItems([]); + await expectArchiveAndTrash(drivePage, { + archiveItems: ['report.txt', 'report (1).txt'], + trashItems: ['report.txt'], + }); + }); }); }); From 846a9a6aa933187a4366f5a9b115ca39027a2291 Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Sun, 20 Sep 2026 21:55:26 -0400 Subject: [PATCH 4/6] fix: restore same-name items resolved one at a time with replace --- .../NameCollisionContainer.tsx | 8 ++++-- .../nameCollision.actions.test.ts | 16 +++++++++-- .../nameCollision.actions.ts | 28 ++++++++++++++----- .../nameCollision.utils.test.ts | 20 ++++++++++++- .../nameCollision.utils.ts | 11 +++++--- 5 files changed, 65 insertions(+), 18 deletions(-) diff --git a/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx b/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx index 70aea771d..8c0959067 100644 --- a/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx +++ b/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx @@ -18,7 +18,7 @@ const NameCollisionContainer: FC = () => { const operationType = collisionDialogInfo?.operation; const newItems = useMemo(() => collisionGroups.flatMap((g) => g.duplicatedItems), [collisionGroups]); const existingItems = useMemo(() => collisionGroups.flatMap((g) => g.existingItems), [collisionGroups]); - const remainingItemsCount = existingItems.length; + const remainingItemsCount = newItems.length; const selectedWorkspace = useAppSelector(workspacesSelectors.getSelectedWorkspace); const limits = useAppSelector(fileVersionsSelectors.getLimits); @@ -60,7 +60,9 @@ const NameCollisionContainer: FC = () => { const group = collisionGroups[groupIndex]; const itemToUpload = group.duplicatedItems[0]; - const itemToReplace = findExistingItemFor(itemToUpload, group.existingItems); + const collidingExistingItem = findExistingItemFor(itemToUpload, group.existingItems); + const isReplacing = operation === 'replace'; + const replacedExistingItem = isReplacing ? collidingExistingItem : undefined; await resolveCollision( { @@ -73,7 +75,7 @@ const NameCollisionContainer: FC = () => { context, ); - const remainingGroups = getRemainingGroups(collisionGroups, groupIndex, itemToReplace); + const remainingGroups = getRemainingGroups(collisionGroups, groupIndex, replacedExistingItem); const hasRemainingGroups = remainingGroups.length > 0; if (hasRemainingGroups) { dispatch( diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts index 427c1da20..6b9132855 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts @@ -303,7 +303,7 @@ describe('resolveCollision', () => { expectMovesThenPop([first, other, asRenamed(second, 'report (1)')], [first, other, second]); }); - test('when moving with replace, then matched existing items are trashed before moving and only the moved items leave the pending deletion list', async () => { + test('when moving with replace, then matched existing items are trashed first and unmatched items are moved under a unique name', async () => { const matched = getDriveItemData({ uuid: 'matched', plainName: 'report', type: 'pdf' }); const unmatched = getDriveItemData({ uuid: 'unmatched', plainName: 'other', type: 'pdf' }); const existing = getDriveItemData({ uuid: 'existing', plainName: 'report', type: 'pdf' }); @@ -313,6 +313,7 @@ describe('resolveCollision', () => { callOrder.push('move'); return asMoveAction(payload); }); + mocks.getUniqueFilename.mockResolvedValue('other (1)'); await resolve({ operationType: 'move', @@ -322,8 +323,17 @@ describe('resolveCollision', () => { }); expect(mocks.moveItemsToTrash).toHaveBeenCalledWith([existing]); - expect(callOrder).toEqual(['trash', 'move']); - expectMovesThenPop([matched], [matched]); + expect(callOrder).toEqual(['trash', 'move', 'move']); + expectMovesThenPop([matched, asRenamed(unmatched, 'other (1)')], [matched, unmatched]); + }); + + test('when a moved file has no existing file left to replace and the user replaces, then nothing is trashed and the file is moved under a unique name', async () => { + const file = getDriveItemData({ uuid: 'file', plainName: 'report', name: 'report', type: 'pdf' }); + + await resolve({ operationType: 'move', operation: 'replace', items: [file], existingItems: [] }); + + expect(mocks.moveItemsToTrash).not.toHaveBeenCalled(); + expectMovesThenPop([asRenamed(file, 'report (1)')], [file]); }); test('when uploading with keep, then folders upload with tracking, files upload with the duplicates check, and the folder is refreshed', async () => { diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts index c6cb2fdf0..d6aa80b8b 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts @@ -24,6 +24,7 @@ import { CollisionItem, CollisionPair, getCollisionPairs, + getUnpairedItems, groupByNameSeries, isFolderUpload, isNameTakenBy, @@ -173,19 +174,32 @@ const keepAndMoveItems = async ( /** * Trashes the colliding drive items and then moves the incoming ones into their place. */ -const replaceAndMoveItems = async ( - pairs: CollisionPair[], +const trashAndMoveItems = async ( + replacingPairs: CollisionPair[], destinationUuid: string, context: NameCollisionContext, ): Promise => { - if (pairs.length === 0) return []; + if (replacingPairs.length === 0) return []; - const { replacingPairs, leftoverItems } = splitReplacingPairs(pairs); const replacingMoves: ItemMove[] = replacingPairs.map(({ item }) => ({ item, payload: item })); await moveItemsToTrash(replacingPairs.map((pair) => pair.existing)); - const replacingItems = await moveItems(replacingMoves, destinationUuid, context); - const keptItems = await keepAndMoveItems(leftoverItems, destinationUuid, context); + return moveItems(replacingMoves, destinationUuid, context); +}; + +const replaceAndMoveItems = async ( + items: DriveItemData[], + existingItems: DriveItemData[], + destinationUuid: string, + context: NameCollisionContext, +): Promise => { + const pairs = getCollisionPairs(items, existingItems); + const unpairedItems = getUnpairedItems(items, existingItems); + const { replacingPairs, leftoverItems } = splitReplacingPairs(pairs); + const itemsToKeep = [...leftoverItems, ...unpairedItems]; + + const replacingItems = await trashAndMoveItems(replacingPairs, destinationUuid, context); + const keptItems = await keepAndMoveItems(itemsToKeep, destinationUuid, context); return [...replacingItems, ...keptItems]; }; @@ -205,7 +219,7 @@ const resolveMoveCollision = async ( if (operation === 'keep') { movedItems = await keepAndMoveItems(items, destinationUuid, context); } else { - movedItems = await replaceAndMoveItems(getCollisionPairs(items, existingItems), destinationUuid, context); + movedItems = await replaceAndMoveItems(items, existingItems, destinationUuid, context); } context.dispatch(storageActions.popItemsToDelete(movedItems)); }; diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts index b82163698..1629db9f8 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.test.ts @@ -9,6 +9,7 @@ import { getCollisionPairs, getNameSeriesKey, getRemainingGroups, + getUnpairedItems, groupByNameSeries, isFolderUpload, isNameTakenBy, @@ -91,6 +92,14 @@ describe('getCollisionPairs', () => { }); }); +describe('getUnpairedItems', () => { + test('when only some items collide, then the items without an existing item are returned', () => { + const unmatched = new File([''], 'unknown.txt'); + + expect(getUnpairedItems([new File([''], 'report.pdf'), unmatched], existingItems)).toEqual([unmatched]); + }); +}); + describe('findPendingGroupIndex', () => { test('when several groups exist, then the first one with duplicated items is returned', () => { const pending = getGroup({ duplicatedItems: [new File([''], 'a.txt')] }); @@ -103,7 +112,7 @@ describe('findPendingGroupIndex', () => { describe('getRemainingGroups', () => { const otherGroup = getGroup({ destinationUuid: 'other', duplicatedItems: [new File([''], 'c.txt')] }); - test('when the handled group still has items, then it keeps the rest and drops the resolved existing item', () => { + test('when the handled group still has items, then it keeps the rest and drops the replaced existing item', () => { const [first, second] = [new File([''], 'a.txt'), new File([''], 'b.txt')]; const groups = [ getGroup({ duplicatedItems: [first, second], existingItems: [existingFile, existingFolder] }), @@ -116,6 +125,15 @@ describe('getRemainingGroups', () => { ]); }); + test('when the handled item did not replace any existing item, then every existing item stays in the group', () => { + const [first, second] = [new File([''], 'a.txt'), new File([''], 'b.txt')]; + const groups = [getGroup({ duplicatedItems: [first, second], existingItems: [existingFile, existingFolder] })]; + + expect(getRemainingGroups(groups, 0, undefined)).toEqual([ + { ...groups[0], duplicatedItems: [second], existingItems: [existingFile, existingFolder] }, + ]); + }); + test('when the handled group had a single item, then it is dropped from the result', () => { const groups = [ getGroup({ duplicatedItems: [new File([''], 'a.txt')], existingItems: [existingFile] }), diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts index 755a4e8cc..9fcd967aa 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.utils.ts @@ -50,18 +50,21 @@ export const getCollisionPairs = ( return existing ? [{ item, existing }] : []; }); +export const getUnpairedItems = (items: T[], existingItems: DriveItemData[]): T[] => + items.filter((item) => !findExistingItemFor(item, existingItems)); + export const hasDuplicatedItems = (group: CollisionGroup): boolean => group.duplicatedItems.length > 0; export const findPendingGroupIndex = (groups: CollisionGroup[]): number => groups.findIndex(hasDuplicatedItems); /** - * Returns the groups still waiting for a decision after the first duplicated item of the group - * at `groupIndex` has been handled. Groups left without duplicated items are dropped. + * Returns the groups still waiting for a decision once the first duplicated item of the group at + * `groupIndex` has been handled, without the existing item it replaced. */ export const getRemainingGroups = ( groups: CollisionGroup[], groupIndex: number, - resolvedExistingItem?: DriveItemData, + replacedExistingItem?: DriveItemData, ): CollisionGroup[] => groups .map((group, index) => @@ -69,7 +72,7 @@ export const getRemainingGroups = ( ? { ...group, duplicatedItems: group.duplicatedItems.slice(1), - existingItems: group.existingItems.filter((existing) => existing !== resolvedExistingItem), + existingItems: group.existingItems.filter((existing) => existing !== replacedExistingItem), } : group, ) From b733417e4ef2dcd0c862599c986eda653a86c08c Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Wed, 7 Oct 2026 10:04:35 -0400 Subject: [PATCH 5/6] feat: add versioning max file size to name collision context and update related logic --- .../NameCollisionContainer.tsx | 9 ++- .../nameCollision.actions.test.ts | 64 +++++++++++++++++++ .../nameCollision.actions.ts | 14 +++- 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx b/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx index 8c0959067..d91946152 100644 --- a/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx +++ b/src/app/drive/components/NameCollisionDialog/NameCollisionContainer.tsx @@ -24,8 +24,15 @@ const NameCollisionContainer: FC = () => { const limits = useAppSelector(fileVersionsSelectors.getLimits); const maxUploadFileSize = useAppSelector(fileVersionsSelectors.getMaxFileSizeLimit); const isVersioningEnabled = limits?.versioning?.enabled ?? false; + const versioningMaxFileSize = limits?.versioning?.maxFileSize ?? 0; - const context: NameCollisionContext = { dispatch, selectedWorkspace, maxUploadFileSize, isVersioningEnabled }; + const context: NameCollisionContext = { + dispatch, + selectedWorkspace, + maxUploadFileSize, + isVersioningEnabled, + versioningMaxFileSize, + }; const closeDialog = () => { dispatch(uiActions.setIsNameCollisionDialogOpen({ open: false, info: undefined })); diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts index 6b9132855..87b9ae65c 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts @@ -62,6 +62,7 @@ vi.mock('views/Drive/components/VersionHistory/utils', () => ({ })); const DESTINATION = 'destination-uuid'; +const VERSIONING_MAX_FILE_SIZE = 10; const asMoveAction = (payload: unknown) => ({ type: 'move', payload }); const asPopAction = (payload: unknown) => ({ type: 'pop', payload }); @@ -83,6 +84,7 @@ const getContext = (overrides: Partial = {}): NameCollisio selectedWorkspace: null, maxUploadFileSize: 5000, isVersioningEnabled: false, + versioningMaxFileSize: VERSIONING_MAX_FILE_SIZE, ...overrides, }); @@ -143,9 +145,25 @@ const failMovesOf = (failingItems: DriveItemData[]) => const succeedAllMoves = () => failMovesOf([]); +const getPdfCollision = (name: string, size: number) => ({ + file: new File(['x'.repeat(size)], `${name}.pdf`), + existing: getDriveItemData({ uuid: `existing-${name}`, plainName: name, type: 'pdf' }), +}); + const resolve = (params: Omit, context = getContext()) => resolveCollision({ ...params, destinationUuid: DESTINATION }, context); +const replaceWithVersioning = (collisions: ReturnType[]) => + resolve( + { + operationType: 'upload', + operation: 'replace', + items: collisions.map(({ file }) => file), + existingItems: collisions.map(({ existing }) => existing), + }, + getContext({ isVersioningEnabled: true }), + ); + /** * Mocks are reset by hand because the browser test project does not do it between tests. */ @@ -511,4 +529,50 @@ describe('resolveCollision', () => { [asRefreshAction(DESTINATION)], ]); }); + + test.each([ + ['under', VERSIONING_MAX_FILE_SIZE - 1], + ['exactly at', VERSIONING_MAX_FILE_SIZE], + ])( + 'when replacing a versionable file %s the versioning size limit, then it becomes a new version and nothing is trashed', + async (_, size) => { + const { file, existing } = getPdfCollision('report', size); + + await replaceWithVersioning([{ file, existing }]); + + expect(mocks.replaceFile).toHaveBeenCalledWith(existing.uuid, { fileId: 'new-file-id', size }); + expect(mocks.moveItemsToTrash).not.toHaveBeenCalled(); + expect(mocks.uploadItemsThunk).not.toHaveBeenCalled(); + }, + ); + + test('when replacing a versionable file over the versioning size limit, then the existing file is trashed and the new one is uploaded', async () => { + const { file, existing } = getPdfCollision('report', VERSIONING_MAX_FILE_SIZE + 1); + + await replaceWithVersioning([{ file, existing }]); + + expect(mocks.replaceFile).not.toHaveBeenCalled(); + expect(mocks.networkUploadFile).not.toHaveBeenCalled(); + expect(mocks.moveItemsToTrash).toHaveBeenCalledWith([existing]); + expect(mocks.uploadItemsThunk).toHaveBeenCalledWith({ + files: [file], + parentFolderId: DESTINATION, + options: { disableDuplicatedNamesCheck: true }, + }); + }); + + test('when replacing versionable files of mixed sizes at once, then only the ones within the limit become new versions', async () => { + const small = getPdfCollision('small', VERSIONING_MAX_FILE_SIZE); + const big = getPdfCollision('big', VERSIONING_MAX_FILE_SIZE + 1); + + await replaceWithVersioning([small, big]); + + expect(mocks.replaceFile).toHaveBeenCalledTimes(1); + expect(mocks.replaceFile).toHaveBeenCalledWith(small.existing.uuid, { + fileId: 'new-file-id', + size: small.file.size, + }); + expect(mocks.moveItemsToTrash).toHaveBeenCalledWith([big.existing]); + expect(mocks.uploadItemsThunk).toHaveBeenCalledWith(expect.objectContaining({ files: [big.file] })); + }); }); diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts index d6aa80b8b..5b5a487de 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts @@ -39,6 +39,7 @@ export interface NameCollisionContext { selectedWorkspace: WorkspaceData | null; maxUploadFileSize: number; isVersioningEnabled: boolean; + versioningMaxFileSize: number; } export interface ResolveCollisionParams { @@ -291,8 +292,14 @@ const uploadItems = async ( await uploadFiles(files, destinationUuid, context, shouldSkipDuplicatesCheck); }; -const isVersionedFilePair = (pair: CollisionPair, { isVersioningEnabled }: NameCollisionContext) => - !isFolderUpload(pair.item) && isVersioningEnabled && isVersioningExtensionAllowed(pair.existing); +const isVersionedFilePair = ( + pair: CollisionPair, + { isVersioningEnabled, versioningMaxFileSize }: NameCollisionContext, +) => + !isFolderUpload(pair.item) && + isVersioningEnabled && + isVersioningExtensionAllowed(pair.existing) && + pair.item.size <= versioningMaxFileSize; /** * Versioned files are replaced one at a time because that upload bypasses the upload queue. @@ -321,7 +328,8 @@ const trashAndUploadItems = async ( /** * Replaces the colliding drive items with the uploaded ones. Files whose extension supports - * versioning become a new version of the existing file; everything else is trashed and re-uploaded. + * versioning and whose size is within the versioning limit become a new version of the existing + * file; everything else is trashed and re-uploaded. */ const replaceAndUploadItems = async ( pairs: CollisionPair[], From f549f9233189444a45567e6078cd742d2b85ce04 Mon Sep 17 00:00:00 2001 From: Francis Terrero Date: Wed, 7 Oct 2026 10:34:11 -0400 Subject: [PATCH 6/6] feat: enhance unique name handling for file uploads to avoid collisions --- .../nameCollision.actions.test.ts | 36 ++++++++++- .../nameCollision.actions.ts | 59 +++++++++++++++---- 2 files changed, 82 insertions(+), 13 deletions(-) diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts index 87b9ae65c..8de192cd6 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.test.ts @@ -354,7 +354,7 @@ describe('resolveCollision', () => { expectMovesThenPop([asRenamed(file, 'report (1)')], [file]); }); - test('when uploading with keep, then folders upload with tracking, files upload with the duplicates check, and the folder is refreshed', async () => { + test('when uploading with keep, then folders upload with tracking, files upload under a unique name without the duplicates check, and the folder is refreshed', async () => { const context = getContext({ maxUploadFileSize: 123, selectedWorkspace: { id: 'ws' } as never }); const file = new File(['content'], 'report.pdf'); const root = getRoot(); @@ -368,12 +368,44 @@ describe('resolveCollision', () => { maxUploadFileSize: 123, }); expect(mocks.dispatch.mock.calls).toEqual([ - [asUploadAction({ files: [file], parentFolderId: DESTINATION, options: { disableDuplicatedNamesCheck: false } })], + [ + asUploadAction({ + files: [expect.objectContaining({ name: 'report (1).pdf' })], + parentFolderId: DESTINATION, + options: { disableDuplicatedNamesCheck: true }, + }), + ], [asRefreshAction(DESTINATION)], ]); expect(mocks.popItemsToDelete).not.toHaveBeenCalled(); }); + test('when three colliding files of the same series are uploaded with keep, then they upload together under the next free names', async () => { + const fileNames = ['file.txt', 'file (1).txt', 'file (2).txt']; + mocks.getUniqueFilename + .mockResolvedValueOnce('file (3)') + .mockResolvedValueOnce('file (4)') + .mockResolvedValueOnce('file (5)'); + + await resolve({ + operationType: 'upload', + operation: 'keep', + items: fileNames.map((name) => new File(['content'], name)), + existingItems: [], + }); + + expect(mocks.dispatch.mock.calls).toEqual([ + [ + asUploadAction({ + files: ['file (3).txt', 'file (4).txt', 'file (5).txt'].map((name) => expect.objectContaining({ name })), + parentFolderId: DESTINATION, + options: { disableDuplicatedNamesCheck: true }, + }), + ], + [asRefreshAction(DESTINATION)], + ]); + }); + test('when uploading with replace and versioning is off, then only matched existing items are trashed and re-uploaded without the duplicates check', async () => { const matched = new File(['content'], 'report.pdf'); const unmatched = new File(['content'], 'other.txt'); diff --git a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts index 5b5a487de..0a37d8757 100644 --- a/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts +++ b/src/app/drive/components/NameCollisionDialog/nameCollision.actions.ts @@ -1,4 +1,6 @@ +import { items as itemUtils } from '@internxt/lib'; import { WorkspaceData } from '@internxt/sdk/dist/workspaces'; +import { renameFile } from 'app/crypto/services/utils'; import { uploadFoldersWithTracking } from 'app/drive/services/folder.service/uploadFoldersWithTracking'; import { Network, getEnvironmentConfig } from 'app/drive/services/network.service'; import { DriveItemData } from 'app/drive/types'; @@ -60,6 +62,11 @@ interface ItemMove { payload: MoveItemPayload; } +interface UniquelyNamedItem { + item: DriveItemData; + uniqueName: string; +} + const getDestinationDuplicates = async (item: DriveItemData, destinationUuid: string): Promise => { if (item.isFolder) { const { duplicatedFoldersResponse } = await checkFolderDuplicated([item], destinationUuid); @@ -110,24 +117,34 @@ const getRenamedMovePayload = (item: DriveItemData, newName: string): MoveItemPa return item.isFolder ? renamedItem : { ...renamedItem, plainName: newName }; }; -const getSeriesMoves = async (seriesItems: DriveItemData[], destinationUuid: string): Promise => { +const getSeriesUniqueNames = async ( + seriesItems: DriveItemData[], + destinationUuid: string, +): Promise => { const renamedItems: DriveItemData[] = []; - const moves: ItemMove[] = []; + const namedItems: UniquelyNamedItem[] = []; for (const item of seriesItems) { const uniqueName = await getUniqueNameInBatch(item, destinationUuid, renamedItems); renamedItems.push({ ...item, name: uniqueName, plainName: uniqueName }); - moves.push({ item, payload: getRenamedMovePayload(item, uniqueName) }); + namedItems.push({ item, uniqueName }); } - return moves; + return namedItems; }; -const getUniqueNameMoves = async (items: DriveItemData[], destinationUuid: string): Promise => { +const getUniqueNames = async (items: DriveItemData[], destinationUuid: string): Promise => { const series = groupByNameSeries(items, getLookupName); - const movesBySeries = await Promise.all(series.map((seriesItems) => getSeriesMoves(seriesItems, destinationUuid))); + const namesBySeries = await Promise.all( + series.map((seriesItems) => getSeriesUniqueNames(seriesItems, destinationUuid)), + ); + + return namesBySeries.flat(); +}; - return movesBySeries.flat(); +const getUniqueNameMoves = async (items: DriveItemData[], destinationUuid: string): Promise => { + const namedItems = await getUniqueNames(items, destinationUuid); + return namedItems.map(({ item, uniqueName }) => ({ item, payload: getRenamedMovePayload(item, uniqueName) })); }; const moveItem = async ( @@ -279,14 +296,18 @@ const uploadFolders = async ( }); }; +const splitFoldersAndFiles = (items: (IRoot | File)[]) => ({ + folders: items.filter(isFolderUpload), + files: items.filter((item): item is File => !isFolderUpload(item)), +}); + const uploadItems = async ( items: (IRoot | File)[], destinationUuid: string, context: NameCollisionContext, shouldSkipDuplicatesCheck = false, ) => { - const folders = items.filter(isFolderUpload); - const files = items.filter((item): item is File => !isFolderUpload(item)); + const { folders, files } = splitFoldersAndFiles(items); await uploadFolders(folders, destinationUuid, context); await uploadFiles(files, destinationUuid, context, shouldSkipDuplicatesCheck); @@ -397,8 +418,22 @@ const skipAndUploadItems = async ( context.dispatch(fetchSortedFolderContentThunk(destinationUuid)); }; +const getFileLookupItem = (file: File): DriveItemData => { + const { filename, extension } = itemUtils.getFilenameAndExt(file.name); + return { name: filename, plainName: filename, type: extension, isFolder: false } as DriveItemData; +}; + +const getUniquelyNamedFiles = async (files: File[], destinationUuid: string): Promise => { + const lookupItems = files.map(getFileLookupItem); + const namedItems = await getUniqueNames(lookupItems, destinationUuid); + + return namedItems.map(({ item, uniqueName }) => + renameFile(files[lookupItems.indexOf(item)], itemUtils.getItemDisplayName({ name: uniqueName, type: item.type })), + ); +}; + /** - * Uploads the items next to the existing ones, letting the upload flow pick a unique name. + * Uploads the items next to the existing ones under a name that does not collide with anything. */ const keepAndUploadItems = async ( items: (IRoot | File)[], @@ -407,7 +442,9 @@ const keepAndUploadItems = async ( ): Promise => { if (items.length === 0) return; - await uploadItems(items, destinationUuid, context); + const { folders, files } = splitFoldersAndFiles(items); + await uploadFolders(folders, destinationUuid, context); + await uploadFiles(await getUniquelyNamedFiles(files, destinationUuid), destinationUuid, context, true); context.dispatch(fetchSortedFolderContentThunk(destinationUuid)); };