Merge pull request #965 from pybricks/dlech

fixes

CHANGELOG.md

@@ -4,6 +4,14 @@
 
 ## [Unreleased]
 
+### Added
+- Added better error message when no files to backup ([support#681]).
+
+### Fixed
+- Fixed deleting files that are not open in the editor.
+
+[support#681]: https://github.com/pybricks/support/issues/681
+
 ## [2.0.0-beta.3] - 2022-07-06
 
 ### Changed

src/alerts/actions.ts

@@ -15,7 +15,9 @@ export const alertsShowAlert = createAction(
     <D extends AlertDomain, S extends AlertSpecific<D>>(
         domain: D,
         specific: S,
-        props: AlertProps<D, S>,
+        ...props: AlertProps<D, S> extends never
+            ? [props?: never]
+            : [props: AlertProps<D, S>]
     ) => ({
         type: 'alerts.action.showAlert',
         domain,

src/app/App.tsx

@@ -5,15 +5,7 @@ import 'react-splitter-layout/lib/index.css';
 import './app.scss';
 import { Classes } from '@blueprintjs/core';
 import docsPackage from '@pybricks/ide-docs/package.json';
-import { getFocusableTreeWalker } from '@react-aria/focus';
-import React, {
-    FocusEventHandler,
-    MouseEventHandler,
-    useCallback,
-    useEffect,
-    useRef,
-    useState,
-} from 'react';
+import React, { useEffect, useState } from 'react';
 import SplitterLayout from 'react-splitter-layout';
 import { useLocalStorage, useTernaryDarkMode } from 'usehooks-ts';
 import Activities from '../activities/Activities';
@@ -164,54 +156,10 @@ const App: React.VFC = () => {
         return () => removeEventListener('keydown', listener);
     }, []);
 
-    // keep track of last focused element in the activities area and restore
-    // focus to that element if any non-interactive area is clicked
-
-    const lastActivitiesFocusChildRef = useRef<HTMLElement>();
-
-    const handleFocus = useCallback<FocusEventHandler>(
-        (e) => {
-            if (e.target instanceof HTMLElement) {
-                lastActivitiesFocusChildRef.current = e.target;
-            }
-        },
-        [lastActivitiesFocusChildRef],
-    );
-
-    const handleActivitiesMouseDown = useCallback<MouseEventHandler<HTMLDivElement>>(
-        (e) => {
-            if (
-                lastActivitiesFocusChildRef.current &&
-                e.currentTarget.contains(lastActivitiesFocusChildRef.current)
-            ) {
-                // if the last focused child exists and it is still inside of
-                // the activities area, focus it
-                lastActivitiesFocusChildRef.current.focus();
-            } else {
-                // otherwise, focus the first focusable element
-                const walker = getFocusableTreeWalker(e.currentTarget);
-                const first = walker.nextNode();
-
-                if (first instanceof HTMLElement) {
-                    first.focus();
-                }
-            }
-
-            // prevent document body from getting focus
-            e.stopPropagation();
-            e.preventDefault();
-        },
-        [lastActivitiesFocusChildRef],
-    );
-
     return (
         <div className="pb-app" onContextMenu={(e) => e.preventDefault()}>
             <div className="pb-app-body">
-                <div
-                    className="pb-app-activities"
-                    onFocus={handleFocus}
-                    onMouseDown={handleActivitiesMouseDown}
-                >
+                <div className="pb-app-activities">
                     <Activities />
                 </div>
                 {/* need a container with position: relative; for SplitterLayout since it uses position: absolute; */}

src/explorer/alerts/NoFilesToBackup.tsx

@@ -0,0 +1,27 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { Icon, Intent } from '@blueprintjs/core';
+import React from 'react';
+import { CreateToast } from '../../i18nToaster';
+import { I18nId, useI18n } from './i18n';
+
+const NoFilesToBackup: React.VoidFunctionComponent = () => {
+    const i18n = useI18n();
+    return (
+        <>
+            {i18n.translate(I18nId.NoFilesToBackupMessage, {
+                icon: <Icon icon="plus" />,
+            })}
+        </>
+    );
+};
+
+export const noFilesToBackup: CreateToast = (onAction) => {
+    return {
+        message: <NoFilesToBackup />,
+        icon: 'info-sign',
+        intent: Intent.PRIMARY,
+        onDismiss: () => onAction('dismiss'),
+    };
+};

src/explorer/alerts/i18n.ts

@@ -11,4 +11,5 @@ export function useI18n(): I18n {
 
 export enum I18nId {
     FileInUseMessage = 'fileInUse.message',
+    NoFilesToBackupMessage = 'noFilesToBackup.message',
 }

src/explorer/alerts/index.ts

@@ -2,6 +2,7 @@
 // Copyright (c) 2022 The Pybricks Authors
 
 import { fileInUse } from './FileInUseAlert';
+import { noFilesToBackup } from './NoFilesToBackup';
 
 // gathers all of the alert creation functions for passing up to the top level
-export default { fileInUse };
+export default { fileInUse, noFilesToBackup };

src/explorer/alerts/translations/en.json

@@ -1,5 +1,8 @@
 {
     "fileInUse": {
         "message": "The file '{fileName}' could not be opened. It is already open in another window."
+    },
+    "noFilesToBackup": {
+        "message": "There are no files to backup. Create a new file first by clicking the {icon} icon."
     }
 }

src/explorer/error.ts

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: MIT
+// Copyright (c) 2022 The Pybricks Authors
+
+import { CustomError } from '../utils/customError';
+
+/** Specific errors for editor subsystem. */
+export type ExplorerErrorName = 'NoFiles';
+
+/** Error class for editor subsystem. */
+export class ExplorerError extends CustomError<ExplorerErrorName> {}

src/explorer/sagas.test.ts

@@ -14,6 +14,7 @@ import {
     editorDidFailToActivateFile,
 } from '../editor/actions';
 import { EditorError } from '../editor/error';
+import { UUID } from '../fileStorage';
 import {
     fileStorageCopyFile,
     fileStorageDeleteFile,
@@ -69,6 +70,7 @@ import {
     duplicateFileDialogDidCancel,
     duplicateFileDialogShow,
 } from './duplicateFileDialog/actions';
+import { ExplorerError, ExplorerErrorName } from './error';
 import {
     Hub,
     newFileWizardDidAccept,
@@ -84,6 +86,23 @@ import explorer from './sagas';
 
 jest.mock('browser-fs-access');
 
+/**
+ * Asymmetric matcher for matching errors by name while ignoring the message.
+ * @param name The name to match.
+ * @returns An asymmetric matcher cast to an ExplorerError so that it can be
+ * passed to action functions.
+ */
+function expectExplorerError(name: ExplorerErrorName): ExplorerError {
+    const matcher: jest.AsymmetricMatcher & Record<string, unknown> = {
+        $$typeof: Symbol.for('jest.asymmetricMatcher'),
+        asymmetricMatch: (other) =>
+            other instanceof ExplorerError && other.name === name,
+        toAsymmetricMatcher: () => `[ExplorerError: ${name}]`,
+    };
+
+    return matcher as unknown as ExplorerError;
+}
+
 describe('handleExplorerArchiveAllFiles', () => {
     let saga: AsyncSaga;
 
@@ -103,6 +122,10 @@ describe('handleExplorerArchiveAllFiles', () => {
 
             saga.put(fileStorageDidFailToDumpAllFiles(testError));
 
+            await expect(saga.take()).resolves.toEqual(
+                alertsShowAlert('alerts', 'unexpectedError', { error: testError }),
+            );
+
             await expect(saga.take()).resolves.toEqual(
                 explorerDidFailToArchiveAllFiles(testError),
             );
@@ -112,7 +135,11 @@ describe('handleExplorerArchiveAllFiles', () => {
             saga.put(fileStorageDidDumpAllFiles([]));
 
             await expect(saga.take()).resolves.toEqual(
-                explorerDidFailToArchiveAllFiles(new Error('no files')),
+                alertsShowAlert('explorer', 'noFilesToBackup'),
+            );
+
+            await expect(saga.take()).resolves.toEqual(
+                explorerDidFailToArchiveAllFiles(expectExplorerError('NoFiles')),
             );
         });
 
@@ -132,6 +159,10 @@ describe('handleExplorerArchiveAllFiles', () => {
                     throw testError;
                 });
 
+                await expect(saga.take()).resolves.toEqual(
+                    alertsShowAlert('alerts', 'unexpectedError', { error: testError }),
+                );
+
                 await expect(saga.take()).resolves.toEqual(
                     explorerDidFailToArchiveAllFiles(testError),
                 );
@@ -471,31 +502,47 @@ describe('handleExplorerDeleteFile', () => {
         );
     });
 
-    describe('accepted', () => {
-        beforeEach(async () => {
-            saga.put(deleteFileAlertDidAccept());
+    describe.each([false, true])(
+        'accepted, file is open: %o',
+        (fileIsOpenInEditor: boolean) => {
+            beforeEach(async () => {
+                if (fileIsOpenInEditor) {
+                    const openFileUuids: readonly UUID[] = [uuid(0)];
+                    saga.updateState({ editor: { openFileUuids } });
+                }
 
-            // should close the editor first
-            await expect(saga.take()).resolves.toEqual(editorCloseFile(uuid(0)));
-            saga.put(editorDidCloseFile(uuid(0)));
+                saga.put(deleteFileAlertDidAccept());
 
-            // then delete the file
-            await expect(saga.take()).resolves.toEqual(fileStorageDeleteFile(testFile));
-        });
+                if (fileIsOpenInEditor) {
+                    // should close the editor first
+                    await expect(saga.take()).resolves.toEqual(
+                        editorCloseFile(uuid(0)),
+                    );
+                    saga.put(editorDidCloseFile(uuid(0)));
+                }
 
-        it('should propagate error', async () => {
-            const testError = new Error('test error');
-            saga.put(fileStorageDidFailToDeleteFile(testFile, testError));
-            await expect(saga.take()).resolves.toEqual(
-                explorerDidFailToDeleteFile(testFile, testError),
-            );
-        });
+                // then delete the file
+                await expect(saga.take()).resolves.toEqual(
+                    fileStorageDeleteFile(testFile),
+                );
+            });
 
-        it('should succeed', async () => {
-            saga.put(fileStorageDidDeleteFile(testFile));
-            await expect(saga.take()).resolves.toEqual(explorerDidDeleteFile(testFile));
-        });
-    });
+            it('should propagate error', async () => {
+                const testError = new Error('test error');
+                saga.put(fileStorageDidFailToDeleteFile(testFile, testError));
+                await expect(saga.take()).resolves.toEqual(
+                    explorerDidFailToDeleteFile(testFile, testError),
+                );
+            });
+
+            it('should succeed', async () => {
+                saga.put(fileStorageDidDeleteFile(testFile));
+                await expect(saga.take()).resolves.toEqual(
+                    explorerDidDeleteFile(testFile),
+                );
+            });
+        },
+    );
 
     afterEach(async () => {
         await saga.end();

src/explorer/sagas.ts

@@ -3,7 +3,7 @@
 
 import { fileOpen, fileSave } from 'browser-fs-access';
 import JSZip from 'jszip';
-import { call, put, race, take, takeEvery } from 'typed-redux-saga/macro';
+import { call, put, race, select, take, takeEvery } from 'typed-redux-saga/macro';
 import { alertsShowAlert } from '../alerts/actions';
 import {
     editorActivateFile,
@@ -41,6 +41,7 @@ import {
     pythonFileMimeType,
     validateFileName,
 } from '../pybricksMicropython/lib';
+import { RootState } from '../reducers';
 import { defined, ensureError, timestamp } from '../utils';
 import {
     explorerArchiveAllFiles,
@@ -77,6 +78,7 @@ import {
     duplicateFileDialogDidCancel,
     duplicateFileDialogShow,
 } from './duplicateFileDialog/actions';
+import { ExplorerError } from './error';
 import {
     newFileWizardDidAccept,
     newFileWizardDidCancel,
@@ -104,7 +106,7 @@ function* handleExplorerArchiveAllFiles(): Generator {
         defined(didDump);
 
         if (didDump.files.length === 0) {
-            throw new Error('no files');
+            throw new ExplorerError('NoFiles', 'no files in storage');
         }
 
         const zip = new JSZip();
@@ -130,7 +132,18 @@ function* handleExplorerArchiveAllFiles(): Generator {
 
         yield* put(explorerDidArchiveAllFiles());
     } catch (err) {
-        yield* put(explorerDidFailToArchiveAllFiles(ensureError(err)));
+        const error = ensureError(err);
+
+        if (error instanceof ExplorerError && error.name === 'NoFiles') {
+            yield* put(alertsShowAlert('explorer', 'noFilesToBackup'));
+        } else {
+            yield* put(
+                alertsShowAlert('alerts', 'unexpectedError', {
+                    error,
+                }),
+            );
+        }
+        yield* put(explorerDidFailToArchiveAllFiles(error));
     }
 }
 
@@ -405,9 +418,13 @@ function* handleExplorerDeleteFile(action: ReturnType<typeof explorerDeleteFile>
 
         // at this point we know the user accepted
 
+        const openUuids = yield* select((s: RootState) => s.editor.openFileUuids);
+
         // have to close editor before deleting, otherwise we get "in use" error
-        yield* put(editorCloseFile(action.uuid));
-        yield* take(editorDidCloseFile.when((a) => a.uuid === action.uuid));
+        if (openUuids.includes(action.uuid)) {
+            yield* put(editorCloseFile(action.uuid));
+            yield* take(editorDidCloseFile.when((a) => a.uuid === action.uuid));
+        }
 
         yield* put(fileStorageDeleteFile(action.fileName));
 

src/notifications/sagas.test.ts

@@ -19,7 +19,6 @@ import {
 import { editorDidFailToOpenFile } from '../editor/actions';
 import { EditorError } from '../editor/error';
 import {
-    explorerDidFailToArchiveAllFiles,
     explorerDidFailToCreateNewFile,
     explorerDidFailToDeleteFile,
     explorerDidFailToDuplicateFile,
@@ -111,7 +110,6 @@ test.each([
     appDidCheckForUpdate(false),
     bleDIServiceDidReceiveFirmwareRevision('3.0.0'),
     fileStorageDidFailToInitialize(new Error('test error')),
-    explorerDidFailToArchiveAllFiles(new Error('test error')),
     explorerDidFailToImportFiles(new Error('test error')),
     explorerDidFailToCreateNewFile(new Error('test error')),
     explorerDidFailToDuplicateFile('test.file', new Error('test error')),
@@ -137,7 +135,6 @@ test.each([
     serviceWorkerDidSucceed(),
     appDidCheckForUpdate(true),
     bleDIServiceDidReceiveFirmwareRevision(firmwareVersion),
-    explorerDidFailToArchiveAllFiles(new DOMException('test message', 'AbortError')),
     explorerDidFailToImportFiles(new DOMException('test message', 'AbortError')),
     explorerDidFailToCreateNewFile(new DOMException('test message', 'AbortError')),
     explorerDidFailToDuplicateFile(

src/notifications/sagas.ts

@@ -21,7 +21,6 @@ import {
 import { editorDidFailToOpenFile } from '../editor/actions';
 import { EditorError } from '../editor/error';
 import {
-    explorerDidFailToArchiveAllFiles,
     explorerDidFailToCreateNewFile,
     explorerDidFailToDeleteFile,
     explorerDidFailToDuplicateFile,
@@ -387,17 +386,6 @@ function* showFileStorageFailToInitialize(
     yield* showUnexpectedError(I18nId.FileStorageFailedToInitialize, action.error);
 }
 
-function* showFileStorageFailToArchive(
-    action: ReturnType<typeof explorerDidFailToArchiveAllFiles>,
-): Generator {
-    if (action.error.name === 'AbortError') {
-        // user clicked cancel button - not an error
-        return;
-    }
-
-    yield* showUnexpectedError(I18nId.ExplorerFailedToArchive, action.error);
-}
-
 function* showExplorerFailToImportFiles(
     action: ReturnType<typeof explorerDidFailToImportFiles>,
 ): Generator {
@@ -476,7 +464,6 @@ export default function* (): Generator {
     yield* takeEvery(appDidCheckForUpdate, showNoUpdateInfo);
     yield* takeEvery(bleDIServiceDidReceiveFirmwareRevision, checkVersion);
     yield* takeEvery(fileStorageDidFailToInitialize, showFileStorageFailToInitialize);
-    yield* takeEvery(explorerDidFailToArchiveAllFiles, showFileStorageFailToArchive);
     yield* takeEvery(explorerDidFailToImportFiles, showExplorerFailToImportFiles);
     yield* takeEvery(explorerDidFailToCreateNewFile, showExplorerFailToCreateFile);
     yield* takeEvery(explorerDidFailToDuplicateFile, showExplorerFailToDuplicate);

test/index.tsx

@@ -9,12 +9,7 @@ import userEvent from '@testing-library/user-event';
 import type { UserEvent } from '@testing-library/user-event/dist/types/setup';
 import React, { ReactElement } from 'react';
 import { Provider } from 'react-redux';
-import {
-    AnyAction,
-    DeepPartial,
-    PreloadedState,
-    legacy_createStore as createStore,
-} from 'redux';
+import { AnyAction, PreloadedState, legacy_createStore as createStore } from 'redux';
 import { END, MulticastChannel, Saga, Task, runSaga, stdChannel } from 'redux-saga';
 import { UUID } from '../src/fileStorage';
 import { RootState, rootReducer } from '../src/reducers';
@@ -81,7 +76,7 @@ export class AsyncSaga {
         return Promise.resolve(next);
     }
 
-    public updateState(state: DeepPartial<RootState>): void {
+    public updateState(state: PreloadedState<RootState>): void {
         for (const key of Object.keys(state) as Array<keyof RootState>) {
             // @ts-expect-error: writing to readonly for testing
             this.state[key] = { ...this.state[key], ...state[key] };