diff --git a/CHANGELOG.md b/CHANGELOG.md index 7af21350..149020ba 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/alerts/actions.ts b/src/alerts/actions.ts index 70c630fb..e50238e6 100644 --- a/src/alerts/actions.ts +++ b/src/alerts/actions.ts @@ -15,7 +15,9 @@ export const alertsShowAlert = createAction( >( domain: D, specific: S, - props: AlertProps, + ...props: AlertProps extends never + ? [props?: never] + : [props: AlertProps] ) => ({ type: 'alerts.action.showAlert', domain, diff --git a/src/app/App.tsx b/src/app/App.tsx index 91340716..c3ec8cb4 100644 --- a/src/app/App.tsx +++ b/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(); - - const handleFocus = useCallback( - (e) => { - if (e.target instanceof HTMLElement) { - lastActivitiesFocusChildRef.current = e.target; - } - }, - [lastActivitiesFocusChildRef], - ); - - const handleActivitiesMouseDown = useCallback>( - (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 (
e.preventDefault()}>
-
+
{/* need a container with position: relative; for SplitterLayout since it uses position: absolute; */} diff --git a/src/explorer/alerts/NoFilesToBackup.tsx b/src/explorer/alerts/NoFilesToBackup.tsx new file mode 100644 index 00000000..66293783 --- /dev/null +++ b/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: , + })} + + ); +}; + +export const noFilesToBackup: CreateToast = (onAction) => { + return { + message: , + icon: 'info-sign', + intent: Intent.PRIMARY, + onDismiss: () => onAction('dismiss'), + }; +}; diff --git a/src/explorer/alerts/i18n.ts b/src/explorer/alerts/i18n.ts index 40d331a8..ef87d766 100644 --- a/src/explorer/alerts/i18n.ts +++ b/src/explorer/alerts/i18n.ts @@ -11,4 +11,5 @@ export function useI18n(): I18n { export enum I18nId { FileInUseMessage = 'fileInUse.message', + NoFilesToBackupMessage = 'noFilesToBackup.message', } diff --git a/src/explorer/alerts/index.ts b/src/explorer/alerts/index.ts index f5545974..535c81e6 100644 --- a/src/explorer/alerts/index.ts +++ b/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 }; diff --git a/src/explorer/alerts/translations/en.json b/src/explorer/alerts/translations/en.json index 73fb7d65..61607e20 100644 --- a/src/explorer/alerts/translations/en.json +++ b/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." } } diff --git a/src/explorer/error.ts b/src/explorer/error.ts new file mode 100644 index 00000000..e449101b --- /dev/null +++ b/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 {} diff --git a/src/explorer/sagas.test.ts b/src/explorer/sagas.test.ts index 66de7c1f..2cfdf7f7 100644 --- a/src/explorer/sagas.test.ts +++ b/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 = { + $$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(); diff --git a/src/explorer/sagas.ts b/src/explorer/sagas.ts index 3cacef3a..a6460b6d 100644 --- a/src/explorer/sagas.ts +++ b/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 // 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)); diff --git a/src/notifications/sagas.test.ts b/src/notifications/sagas.test.ts index 9b84ff99..c3d08c18 100644 --- a/src/notifications/sagas.test.ts +++ b/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( diff --git a/src/notifications/sagas.ts b/src/notifications/sagas.ts index 92f26458..5b4470f1 100644 --- a/src/notifications/sagas.ts +++ b/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, -): Generator { - if (action.error.name === 'AbortError') { - // user clicked cancel button - not an error - return; - } - - yield* showUnexpectedError(I18nId.ExplorerFailedToArchive, action.error); -} - function* showExplorerFailToImportFiles( action: ReturnType, ): 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); diff --git a/test/index.tsx b/test/index.tsx index a4f6d9ba..a8fe20e9 100644 --- a/test/index.tsx +++ b/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): void { + public updateState(state: PreloadedState): void { for (const key of Object.keys(state) as Array) { // @ts-expect-error: writing to readonly for testing this.state[key] = { ...this.state[key], ...state[key] };