Skip to content

Commit

Permalink
fix: Handle deletion of unsaved copied file in NotebookPanel (#1557)
Browse files Browse the repository at this point in the history
* 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 <georgecwan@users.noreply.github.com>
  • Loading branch information
georgecwan and georgecwan authored Oct 13, 2023
1 parent 327bcb6 commit 4021aac
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 20 deletions.
18 changes: 4 additions & 14 deletions packages/dashboard-core-plugins/src/panels/FileExplorerPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
32 changes: 26 additions & 6 deletions packages/dashboard-core-plugins/src/panels/NotebookPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand Down
31 changes: 31 additions & 0 deletions packages/file-explorer/src/FileUtils.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 4021aac

Please sign in to comment.