From 4021aac3bc130f8eec84385c9aadcb4ecf0b995c Mon Sep 17 00:00:00 2001 From: George Wan Date: Fri, 13 Oct 2023 07:08:04 -0700 Subject: [PATCH] fix: Handle deletion of unsaved copied file in NotebookPanel (#1557) * Previously, duplicating a notebook file and then trying to delete it without saving it first will cause an error to occur * Added a `fileExists` function to `FileUtils` to handle this check * Added a `getUniqueCopyFileName` function to `FileUtils` * Behaviour of copying a file from `NotebookPanel` changed from opening an unsaved copy of the file to duplicating the file in `FileStorage` first * Related to #1359 #### Testing Instructions: * Open a file in the notebook panel * Copy the opened file by using the 'Copy File' option in the overflow menu * Delete the file by using the overflow menu (No error should occur) * Repeat the same steps but copy the file by right clicking the file in the file explorer * Delete the copied file again, this time it should also disappear from the file explorer --------- Co-authored-by: georgecwan --- .../src/panels/FileExplorerPanel.tsx | 18 +++-------- .../src/panels/NotebookPanel.tsx | 32 +++++++++++++++---- packages/file-explorer/src/FileUtils.ts | 31 ++++++++++++++++++ 3 files changed, 61 insertions(+), 20 deletions(-) diff --git a/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx b/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx index b3f8f53395..1b07cd85be 100644 --- a/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx @@ -165,20 +165,10 @@ export class FileExplorerPanel extends React.Component< log.error('Invalid item in handleCopyItem', file); return; } - let newName = FileUtils.getCopyFileName(file.filename); - const checkNewName = async (): Promise => { - try { - await fileStorage.info(newName); - return true; - } catch (error) { - return false; - } - }; - // await in loop is fine here, this isn't a parallel task - // eslint-disable-next-line no-await-in-loop - while (await checkNewName()) { - newName = FileUtils.getCopyFileName(newName); - } + const newName = await FileUtils.getUniqueCopyFileName( + fileStorage, + file.filename + ); await fileStorage.copyFile(file.filename, newName); } diff --git a/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx b/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx index a77867d221..5ec6ad73b7 100644 --- a/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx @@ -681,15 +681,31 @@ class NotebookPanel extends Component { this.focus(); } - handleCopy(): void { + async handleCopy(): Promise { + const { fileStorage, glEventHub, session } = this.props; const { fileMetadata, settings } = this.state; assertNotNull(fileMetadata); - const content = this.getNotebookValue(); const { language } = settings; const { itemName } = fileMetadata; - const copyName = FileUtils.getCopyFileName(itemName); + const copyName = await FileUtils.getUniqueCopyFileName( + fileStorage, + itemName + ); log.debug('handleCopy', fileMetadata, itemName, copyName); - this.createNotebook(copyName, language, content); + await fileStorage.copyFile(itemName, copyName); + const newFileMetadata = { id: copyName, itemName: copyName }; + const notebookSettings = { + value: null, + language, + }; + glEventHub.emit( + NotebookEvent.SELECT_NOTEBOOK, + session, + language, + notebookSettings, + newFileMetadata, + true + ); } handleDelete(): void { @@ -697,7 +713,7 @@ class NotebookPanel extends Component { this.setState({ showDeleteModal: true }); } - handleDeleteConfirm(): void { + async handleDeleteConfirm(): Promise { const { fileStorage, glContainer, glEventHub } = this.props; const { fileMetadata } = this.state; @@ -708,7 +724,11 @@ class NotebookPanel extends Component { return; } - if (FileUtils.hasPath(fileMetadata.itemName)) { + if ( + FileUtils.hasPath(fileMetadata.itemName) && + // eslint-disable-next-line @typescript-eslint/strict-boolean-expressions + (await FileUtils.fileExists(fileStorage, fileMetadata.itemName)) + ) { glEventHub.emit(NotebookEvent.CLOSE_FILE, fileMetadata, { force: true }); fileStorage.deleteFile(fileMetadata.itemName); } else { diff --git a/packages/file-explorer/src/FileUtils.ts b/packages/file-explorer/src/FileUtils.ts index 6bae38bbb9..6acaae7f58 100644 --- a/packages/file-explorer/src/FileUtils.ts +++ b/packages/file-explorer/src/FileUtils.ts @@ -1,4 +1,6 @@ import { ValidationError } from '@deephaven/utils'; +import FileNotFoundError from './FileNotFoundError'; +import { FileStorage } from './FileStorage'; /** * A basic list of some common MIME types. @@ -16,6 +18,22 @@ export enum MIME_TYPE { * Collection of utils for operating on file names */ export class FileUtils { + static async fileExists( + fileStorage: FileStorage, + name: string + ): Promise { + try { + await fileStorage.info(name); + return true; + } catch (e) { + // Should probably make sure it's a `FileNotFoundError`, and re-throw the error if it is not + if (!(e instanceof FileNotFoundError)) { + throw e; + } + return false; + } + } + /** * Format file extension * @param extension File extension to format, defaults to empty string @@ -80,6 +98,19 @@ export class FileUtils { return extension !== null ? `${copyName}.${extension}` : copyName; } + static async getUniqueCopyFileName( + fileStorage: FileStorage, + originalName: string + ): Promise { + let copyName = FileUtils.getCopyFileName(originalName); + // await in loop is fine here, this isn't a parallel task + // eslint-disable-next-line no-await-in-loop + while (await FileUtils.fileExists(fileStorage, copyName)) { + copyName = FileUtils.getCopyFileName(copyName); + } + return copyName; + } + /** * Return a MIME type for the provided file * @param name The file name to get the type for