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 <[email protected]>
deephaven · Oct 13, 2023 · 4021aac · 4021aac
1 parent 327bcb6
commit 4021aac
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 20 deletions.
diff --git 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<boolean> => {
-      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
@@ -681,23 +681,39 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
     this.focus();
   }
 
-  handleCopy(): void {
+  async handleCopy(): Promise<void> {
+    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 {
     log.debug('handleDelete, pending confirmation');
     this.setState({ showDeleteModal: true });
   }
 
-  handleDeleteConfirm(): void {
+  async handleDeleteConfirm(): Promise<void> {
     const { fileStorage, glContainer, glEventHub } = this.props;
     const { fileMetadata } = this.state;
 
@@ -708,7 +724,11 @@ class NotebookPanel extends Component<NotebookPanelProps, NotebookPanelState> {
       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
@@ -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<boolean> {
+    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<string> {
+    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