fix explorer import silently ignored when invalid file name

This finishes a TODO to handle allowing the user to rename a file
with an invalid file name when import files from the file system.

Fixes: https://github.com/pybricks/support/issues/717

src/explorer/Explorer.tsx

@@ -45,6 +45,7 @@ import DuplicateFileDialog from './duplicateFileDialog/DuplicateFileDialog';
 import { useI18n } from './i18n';
 import NewFileWizard from './newFileWizard/NewFileWizard';
 import RenameFileDialog from './renameFileDialog/RenameFileDialog';
+import RenameImportDialog from './renameImportDialog/RenameImportDialog';
 
 type ActionButtonProps = {
     /** The DOM id for this instance. */
@@ -417,6 +418,7 @@ const Explorer: React.VFC = () => {
             <FileTree />
             <NewFileWizard />
             <RenameFileDialog />
+            <RenameImportDialog />
             <DuplicateFileDialog />
             <DeleteFileAlert />
         </div>

src/explorer/reducers.ts

@@ -7,10 +7,12 @@ import deleteFileAlert from './deleteFileAlert/reducers';
 import duplicateFileDialog from './duplicateFileDialog/reducers';
 import newFileWizard from './newFileWizard/reducers';
 import renameFileDialog from './renameFileDialog/reducers';
+import renameImportDialog from './renameImportDialog/reducers';
 
 export default combineReducers({
     duplicateFileDialog,
     deleteFileAlert,
     newFileWizard,
     renameFileDialog,
+    renameImportDialog,
 });

src/explorer/renameImportDialog/RenameImportDialog.test.tsx

@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { waitFor } from '@testing-library/dom';
+import React from 'react';
+import { testRender } from '../../../test';
+import RenameImportDialog from './RenameImportDialog';
+import { renameImportDialogDidAccept, renameImportDialogDidCancel } from './actions';
+
+describe('rename button', () => {
+    it('should accept the dialog Rename is clicked', async () => {
+        const [user, dialog, dispatch] = testRender(<RenameImportDialog />, {
+            explorer: { renameImportDialog: { isOpen: true, fileName: 'old.file' } },
+        });
+
+        const button = dialog.getByRole('button', { name: 'Rename' });
+
+        // have to type a new file name before Rename button is enabled
+        const input = dialog.getByLabelText('File name');
+        await waitFor(() => expect(input).toHaveFocus());
+        await user.type(input, 'new', { skipClick: true });
+        await waitFor(() => expect(button).not.toBeDisabled());
+
+        await user.click(button);
+        expect(dispatch).toHaveBeenCalledWith(
+            renameImportDialogDidAccept('old.file', 'new.file'),
+        );
+    });
+
+    it('should accept the dialog when enter is pressed in the text input', async () => {
+        const [user, dialog, dispatch] = testRender(<RenameImportDialog />, {
+            explorer: { renameImportDialog: { isOpen: true, fileName: 'old.file' } },
+        });
+
+        // have to type a new file name before Rename button is enabled
+        const input = dialog.getByLabelText('File name');
+        await waitFor(() => expect(input).toHaveFocus());
+        await user.type(input, 'new{Enter}', { skipClick: true });
+
+        expect(dispatch).toHaveBeenCalledWith(
+            renameImportDialogDidAccept('old.file', 'new.file'),
+        );
+    });
+
+    it('should cancel when close button is clicked', async () => {
+        const [user, dialog, dispatch] = testRender(<RenameImportDialog />, {
+            explorer: { renameImportDialog: { isOpen: true } },
+        });
+
+        const button = dialog.getByRole('button', { name: 'Close' });
+
+        await waitFor(() => expect(button).toBeVisible());
+
+        await user.click(button);
+        expect(dispatch).toHaveBeenCalledWith(renameImportDialogDidCancel());
+    });
+
+    it('should cancel when skip button is clicked', async () => {
+        const [user, dialog, dispatch] = testRender(<RenameImportDialog />, {
+            explorer: { renameImportDialog: { isOpen: true } },
+        });
+
+        const button = dialog.getByRole('button', { name: 'Skip importing this file' });
+
+        await waitFor(() => expect(button).toBeVisible());
+
+        await user.click(button);
+        expect(dispatch).toHaveBeenCalledWith(renameImportDialogDidCancel());
+    });
+});

src/explorer/renameImportDialog/RenameImportDialog.tsx

@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { Button, Classes, Dialog } from '@blueprintjs/core';
+import React, { useCallback, useRef, useState } from 'react';
+import { useDispatch } from 'react-redux';
+import { appName } from '../../app/constants';
+import { useFileStorageMetadata } from '../../fileStorage/hooks';
+import {
+    FileNameValidationResult,
+    validateFileName,
+} from '../../pybricksMicropython/lib';
+import { useSelector } from '../../reducers';
+import FileNameFormGroup from '../fileNameFormGroup/FileNameFormGroup';
+import { renameImportDialogDidAccept, renameImportDialogDidCancel } from './actions';
+import { useI18n } from './i18n';
+
+const RenameImportDialog: React.VFC = () => {
+    const i18n = useI18n();
+    const dispatch = useDispatch();
+    const isOpen = useSelector((s) => s.explorer.renameImportDialog.isOpen);
+    const oldName = useSelector((s) => s.explorer.renameImportDialog.fileName);
+
+    const [baseName, extension] = oldName.split(/(\.\w+)$/);
+
+    const [newName, setNewName] = useState(baseName);
+    const files = useFileStorageMetadata() ?? [];
+    const result = validateFileName(
+        newName,
+        extension,
+        files.map((f) => f.path),
+    );
+
+    const inputRef = useRef<HTMLInputElement>(null);
+
+    const handleSubmit = useCallback<React.FormEventHandler>(
+        (e) => {
+            e.preventDefault();
+            dispatch(renameImportDialogDidAccept(oldName, `${newName}${extension}`));
+        },
+        [dispatch, oldName, newName, extension],
+    );
+
+    const handleClose = useCallback(() => {
+        dispatch(renameImportDialogDidCancel());
+    }, [dispatch]);
+
+    return (
+        <Dialog
+            title={i18n.translate('title')}
+            isOpen={isOpen}
+            onOpening={() => setNewName(baseName)}
+            onOpened={() => {
+                inputRef.current?.select();
+                inputRef.current?.focus();
+            }}
+            onClose={handleClose}
+        >
+            <form onSubmit={handleSubmit}>
+                <div className={Classes.DIALOG_BODY}>
+                    <p>{i18n.translate('message', { fileName: oldName, appName })}</p>
+                    <FileNameFormGroup
+                        fileName={newName}
+                        fileExtension={extension}
+                        validationResult={result}
+                        inputRef={inputRef}
+                        onChange={setNewName}
+                    />
+                </div>
+                <div className={Classes.DIALOG_FOOTER}>
+                    <div className={Classes.DIALOG_FOOTER_ACTIONS}>
+                        <Button intent="none" onClick={handleClose}>
+                            {i18n.translate('action.skip')}
+                        </Button>
+                        <Button
+                            intent="primary"
+                            disabled={result !== FileNameValidationResult.IsOk}
+                            type="submit"
+                        >
+                            {i18n.translate('action.rename')}
+                        </Button>
+                    </div>
+                </div>
+            </form>
+        </Dialog>
+    );
+};
+
+export default RenameImportDialog;

src/explorer/renameImportDialog/actions.ts

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { createAction } from '../../actions';
+
+/**
+ * Action that requests to show the rename file dialog.
+ * @param oldName The old file name.
+ */
+export const renameImportDialogShow = createAction((oldName: string) => ({
+    type: 'explorer.renameImportDialog.action.show',
+    oldName,
+}));
+
+/**
+ * Action that indicates the rename file dialog was accepted.
+ * @param oldName The old file name.
+ * @param newName The new file name.
+ */
+export const renameImportDialogDidAccept = createAction(
+    (oldName: string, newName: string) => ({
+        type: 'explorer.renameImportDialog.action.didAccept',
+        oldName,
+        newName,
+    }),
+);
+
+/**
+ * Action that indicates the rename file dialog was canceled.
+ */
+export const renameImportDialogDidCancel = createAction(() => ({
+    type: 'explorer.renameImportDialog.action.didCancel',
+}));

src/explorer/renameImportDialog/i18n.ts

@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { useI18n as useShopifyI18n } from '@shopify/react-i18n';
+import type { TypedI18n } from '../../i18n';
+import type translations from './translations/en.json';
+
+export function useI18n(): TypedI18n<typeof translations> {
+    // istanbul ignore next: babel-loader rewrites this line
+    const [i18n] = useShopifyI18n();
+    return i18n;
+}

src/explorer/renameImportDialog/reducers.ts

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { Reducer, combineReducers } from 'redux';
+import {
+    renameImportDialogDidAccept,
+    renameImportDialogDidCancel,
+    renameImportDialogShow,
+} from './actions';
+
+/** Controls the rename file dialog isOpen state. */
+const isOpen: Reducer<boolean> = (state = false, action) => {
+    if (renameImportDialogShow.matches(action)) {
+        return true;
+    }
+
+    if (
+        renameImportDialogDidAccept.matches(action) ||
+        renameImportDialogDidCancel.matches(action)
+    ) {
+        return false;
+    }
+
+    return state;
+};
+
+/** Controls the rename file dialog file name input box text. */
+const fileName: Reducer<string> = (state = '', action) => {
+    if (renameImportDialogShow.matches(action)) {
+        return action.oldName;
+    }
+
+    return state;
+};
+
+export default combineReducers({ isOpen, fileName });

src/explorer/renameImportDialog/translations/en.json

@@ -0,0 +1,8 @@
+{
+    "title": "Rename imported file",
+    "message": "The name of the imported file '{fileName}' is not allowed in {appName}. Please change the name below.",
+    "action": {
+        "skip": "Skip importing this file",
+        "rename": "Rename"
+    }
+}

src/explorer/sagas.test.ts

@@ -82,6 +82,10 @@ import {
     renameFileDialogDidCancel,
     renameFileDialogShow,
 } from './renameFileDialog/actions';
+import {
+    renameImportDialogDidAccept,
+    renameImportDialogShow,
+} from './renameImportDialog/actions';
 import explorer from './sagas';
 
 jest.mock('browser-fs-access');
@@ -226,6 +230,40 @@ describe('handleExplorerImportFiles', () => {
 
         await saga.end();
     });
+
+    it('should handle invalid file name', async () => {
+        const testFileName = 'bad#name.py';
+        const testFileContents = '# test';
+
+        const saga = new AsyncSaga(explorer);
+
+        jest.spyOn(browserFsAccess, 'fileOpen').mockResolvedValueOnce([
+            mock<FileWithHandle>({
+                name: testFileName,
+                text: () => Promise.resolve(testFileContents),
+            }),
+        ]);
+
+        saga.put(explorerImportFiles());
+
+        await expect(saga.take()).resolves.toEqual(
+            renameImportDialogShow(testFileName),
+        );
+
+        const renamedFileName = 'good_name.py';
+
+        saga.put(renameImportDialogDidAccept(testFileName, renamedFileName));
+
+        await expect(saga.take()).resolves.toEqual(
+            fileStorageWriteFile(renamedFileName, testFileContents),
+        );
+
+        saga.put(fileStorageDidWriteFile(renamedFileName, uuid(0)));
+
+        await expect(saga.take()).resolves.toEqual(explorerDidImportFiles());
+
+        await saga.end();
+    });
 });
 
 describe('handleExplorerCreateNewFile', () => {

src/explorer/sagas.ts

@@ -89,6 +89,11 @@ import {
     renameFileDialogDidCancel,
     renameFileDialogShow,
 } from './renameFileDialog/actions';
+import {
+    renameImportDialogDidAccept,
+    renameImportDialogDidCancel,
+    renameImportDialogShow,
+} from './renameImportDialog/actions';
 
 function* handleExplorerArchiveAllFiles(): Generator {
     try {
@@ -166,20 +171,26 @@ function* handleExplorerImportFiles(): Generator {
             const text = yield* call(() => file.text());
 
             const [baseName] = file.name.split(pythonFileExtensionRegex);
+            let fileName = `${baseName}${pythonFileExtension}`;
 
             const result = validateFileName(baseName, pythonFileExtension, []);
 
             if (result != FileNameValidationResult.IsOk) {
-                // TODO: validate file name and allow user to rename or skip
-                console.error(
-                    'skipping file',
-                    file.name,
-                    FileNameValidationResult[result],
-                );
-                continue;
-            }
+                yield* put(renameImportDialogShow(file.name));
 
-            const fileName = `${baseName}${pythonFileExtension}`;
+                const { accepted, cancelled } = yield* race({
+                    accepted: take(renameImportDialogDidAccept),
+                    cancelled: take(renameImportDialogDidCancel),
+                });
+
+                if (cancelled) {
+                    continue;
+                }
+
+                defined(accepted);
+
+                fileName = accepted.newName;
+            }
 
             yield* put(fileStorageWriteFile(fileName, text));