From 1e0d44bbe4429481948d23d6932dcfabcb2060fb Mon Sep 17 00:00:00 2001 From: David Lechner Date: Wed, 6 Jul 2022 15:56:22 -0500 Subject: [PATCH 1/3] Fixed deleting files that are not open in the editor. --- CHANGELOG.md | 3 ++ src/explorer/sagas.test.ts | 59 ++++++++++++++++++++++++-------------- src/explorer/sagas.ts | 11 +++++-- test/index.tsx | 9 ++---- 4 files changed, 51 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7af21350..3b07d63c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ ## [Unreleased] +### Fixed +- Fixed deleting files that are not open in the editor. + ## [2.0.0-beta.3] - 2022-07-06 ### Changed diff --git a/src/explorer/sagas.test.ts b/src/explorer/sagas.test.ts index 66de7c1f..19726eea 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, @@ -471,31 +472,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..84602c6c 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, @@ -405,9 +406,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/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] }; From 7232bf615326316085c031146e1511cacd29d52e Mon Sep 17 00:00:00 2001 From: David Lechner Date: Wed, 6 Jul 2022 16:53:07 -0500 Subject: [PATCH 2/3] add better error message when no files to backup Instead of showing unexpected error, show message explaining that there are no files to be backed up and how to fix it. Fixes: https://github.com/pybricks/support/issues/681 --- CHANGELOG.md | 5 ++++ src/alerts/actions.ts | 4 ++- src/explorer/alerts/NoFilesToBackup.tsx | 27 ++++++++++++++++++++ src/explorer/alerts/i18n.ts | 1 + src/explorer/alerts/index.ts | 3 ++- src/explorer/alerts/translations/en.json | 3 +++ src/explorer/error.ts | 10 ++++++++ src/explorer/sagas.test.ts | 32 +++++++++++++++++++++++- src/explorer/sagas.ts | 16 ++++++++++-- src/notifications/sagas.test.ts | 3 --- src/notifications/sagas.ts | 13 ---------- 11 files changed, 96 insertions(+), 21 deletions(-) create mode 100644 src/explorer/alerts/NoFilesToBackup.tsx create mode 100644 src/explorer/error.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b07d63c..149020ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +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/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 19726eea..2cfdf7f7 100644 --- a/src/explorer/sagas.test.ts +++ b/src/explorer/sagas.test.ts @@ -70,6 +70,7 @@ import { duplicateFileDialogDidCancel, duplicateFileDialogShow, } from './duplicateFileDialog/actions'; +import { ExplorerError, ExplorerErrorName } from './error'; import { Hub, newFileWizardDidAccept, @@ -85,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; @@ -104,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), ); @@ -113,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')), ); }); @@ -133,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), ); diff --git a/src/explorer/sagas.ts b/src/explorer/sagas.ts index 84602c6c..a6460b6d 100644 --- a/src/explorer/sagas.ts +++ b/src/explorer/sagas.ts @@ -78,6 +78,7 @@ import { duplicateFileDialogDidCancel, duplicateFileDialogShow, } from './duplicateFileDialog/actions'; +import { ExplorerError } from './error'; import { newFileWizardDidAccept, newFileWizardDidCancel, @@ -105,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(); @@ -131,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)); } } 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); From c3fec0b77996c775a6838ca602155fc6fec5ea03 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Wed, 6 Jul 2022 17:53:49 -0500 Subject: [PATCH 3/3] app: drop last focused feature This feature that keeps track of last focused item in the activities panel broke child dialogs like the about dialog. Fixes: https://github.com/pybricks/support/issues/682 --- src/app/App.tsx | 56 ++----------------------------------------------- 1 file changed, 2 insertions(+), 54 deletions(-) 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; */}