From 6e55be056c23883748fb3b1b6027cdb63d0043e6 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Tue, 5 Apr 2022 10:52:11 -0500 Subject: [PATCH] explorer/reducers: don't use FileMetadata This separates the explorer files state from the fileStorage FileMetadata. The latter contains extra info that the explorer doesn't need. This will prevent rerendering each time the file contents change. --- src/explorer/Explorer.test.tsx | 9 +- src/explorer/Explorer.tsx | 8 +- src/explorer/newFileWizard/NewFileWizard.tsx | 2 +- src/explorer/reducers.test.ts | 100 ++++++++++++------ src/explorer/reducers.ts | 29 ++++- .../renameFileDialog/RenameFileDialog.tsx | 2 +- src/explorer/sagas.ts | 2 +- 7 files changed, 102 insertions(+), 50 deletions(-) diff --git a/src/explorer/Explorer.test.tsx b/src/explorer/Explorer.test.tsx index c94c6b4d..533f86b1 100644 --- a/src/explorer/Explorer.test.tsx +++ b/src/explorer/Explorer.test.tsx @@ -6,7 +6,6 @@ import { cleanup } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { testRender, uuid } from '../../test'; -import { FileMetadata } from '../fileStorage/actions'; import Explorer from './Explorer'; import { explorerArchiveAllFiles, @@ -15,6 +14,7 @@ import { explorerImportFiles, explorerRenameFile, } from './actions'; +import { ExplorerFileInfo } from './reducers'; afterEach(async () => { jest.restoreAllMocks(); @@ -22,10 +22,9 @@ afterEach(async () => { localStorage.clear(); }); -const testFile: FileMetadata = { - uuid: uuid(0), - path: 'test.file', - sha256: '', +const testFile: ExplorerFileInfo = { + id: uuid(0), + name: 'test.file', }; describe('archive button', () => { diff --git a/src/explorer/Explorer.tsx b/src/explorer/Explorer.tsx index 59a15701..38cda4d5 100644 --- a/src/explorer/Explorer.tsx +++ b/src/explorer/Explorer.tsx @@ -290,12 +290,12 @@ const FileTree: React.VoidFunctionComponent = ({ i18n }) => { () => files.reduce( (obj, file) => { - const index = file.uuid; + const index = file.id; obj[index] = { index, data: { - fileName: file.path, + fileName: file.name, icon: 'document', secondaryLabel: ( @@ -319,8 +319,8 @@ const FileTree: React.VoidFunctionComponent = ({ i18n }) => { hasChildren: true, children: [...files] // REVISIT: consider using Intl.Collator() for i18n.locale - .sort((a, b) => a.path.localeCompare(b.path)) - .map((n) => n.uuid), + .sort((a, b) => a.name.localeCompare(b.name)) + .map((n) => n.id), }, } as Record, ), diff --git a/src/explorer/newFileWizard/NewFileWizard.tsx b/src/explorer/newFileWizard/NewFileWizard.tsx index d09b5bd0..3b47487e 100644 --- a/src/explorer/newFileWizard/NewFileWizard.tsx +++ b/src/explorer/newFileWizard/NewFileWizard.tsx @@ -45,7 +45,7 @@ const NewFileWizard: React.VoidFunctionComponent = ({ const fileNameValidation = validateFileName( fileName, pythonFileExtension, - files.map((f) => f.path), + files.map((f) => f.name), ); const [hubType, setHubType] = useState(defaultHub); diff --git a/src/explorer/reducers.test.ts b/src/explorer/reducers.test.ts index f65e2c8c..ebc1ed7a 100644 --- a/src/explorer/reducers.test.ts +++ b/src/explorer/reducers.test.ts @@ -9,50 +9,84 @@ import { fileStorageDidInitialize, fileStorageDidRemoveItem, } from '../fileStorage/actions'; -import reducers from './reducers'; +import reducers, { ExplorerFileInfo } from './reducers'; type State = ReturnType; -test('files', () => { - const testFile: FileMetadata = { +describe('files', () => { + const testFile: ExplorerFileInfo = { + id: uuid(0), + name: 'test.file', + }; + + const testFileMetadata: FileMetadata = { uuid: uuid(0), path: 'test.file', sha256: '', }; - const modifiedFile: FileMetadata = { ...testFile, path: 'modified.file' }; + const modifiedFile: ExplorerFileInfo = { + ...testFile, + name: 'modified.file', + }; - expect(testFile).not.toEqual(modifiedFile); + const modifiedFileMetadata: FileMetadata = { + ...testFileMetadata, + path: 'modified.file', + }; - // initialization populates file list - expect( - reducers( - { files: [] as readonly FileMetadata[] } as State, - fileStorageDidInitialize([testFile]), - ).files, - ).toEqual([testFile]); + beforeAll(() => { + // check validity of test data before starting tests + expect(testFile).not.toEqual(modifiedFile); + expect(testFileMetadata).not.toEqual(modifiedFileMetadata); + }); - // adding appends an item - expect( - reducers( - { files: [] as readonly FileMetadata[] } as State, - fileStorageDidAddItem(testFile), - ).files, - ).toEqual([testFile]); + it('should get a list when file storage is initialized', () => { + expect( + reducers( + { files: [] as readonly ExplorerFileInfo[] } as State, + fileStorageDidInitialize([testFileMetadata]), + ).files, + ).toEqual([testFile]); + }); - // changing replaces an item - expect( - reducers( - { files: [testFile] as readonly FileMetadata[] } as State, - fileStorageDidChangeItem(testFile, modifiedFile), - ).files, - ).toEqual([modifiedFile]); + it('should modify the list when a file is added to storage', () => { + expect( + reducers( + { files: [] as readonly ExplorerFileInfo[] } as State, + fileStorageDidAddItem(testFileMetadata), + ).files, + ).toEqual([testFile]); + }); - // removing deletes an item - expect( - reducers( - { files: [testFile] as readonly FileMetadata[] } as State, - fileStorageDidRemoveItem(testFile), - ).files, - ).not.toContain(testFile); + it('should modify the list when an item is renamed in storage', () => { + expect( + reducers( + { files: [testFile] as readonly ExplorerFileInfo[] } as State, + fileStorageDidChangeItem(testFileMetadata, modifiedFileMetadata), + ).files, + ).toEqual([modifiedFile]); + }); + + it('should not modify the list if a change is made other than renaming', () => { + const originalList: readonly ExplorerFileInfo[] = [testFile]; + expect( + reducers( + { files: originalList } as State, + fileStorageDidChangeItem(testFileMetadata, { + ...testFileMetadata, + sha256: 'changed', + }), + ).files, + ).toBe(originalList); + }); + + it('should modify the list when a file is removed from storage', () => { + expect( + reducers( + { files: [testFile] as readonly ExplorerFileInfo[] } as State, + fileStorageDidRemoveItem(testFileMetadata), + ).files, + ).not.toContain(testFile); + }); }); diff --git a/src/explorer/reducers.ts b/src/explorer/reducers.ts index d820af8c..cda4c2e7 100644 --- a/src/explorer/reducers.ts +++ b/src/explorer/reducers.ts @@ -12,21 +12,40 @@ import { import renameFileDialog from './renameFileDialog/reducers'; -const files: Reducer = (state = [], action) => { +export type ExplorerFileInfo = Readonly<{ + /** A unique identifier for this file (not the path, which can change). */ + id: string; + /** The file name (including extension - without directory). */ + name: string; +}>; + +function metadataToInfo(file: FileMetadata): ExplorerFileInfo { + return { id: file.uuid, name: file.path }; +} + +const files: Reducer = (state = [], action) => { if (fileStorageDidInitialize.matches(action)) { - return [...action.files]; + return action.files.map(metadataToInfo); } if (fileStorageDidAddItem.matches(action)) { - return [...state, action.file]; + return [...state, metadataToInfo(action.file)]; } if (fileStorageDidChangeItem.matches(action)) { - return [...state].map((f) => (f.uuid === action.file.uuid ? action.file : f)); + // We only care about UUID and the path. UUID can't change so if the + // path didn't change, there is nothing to do. + if (action.oldFile.path === action.file.path) { + return state; + } + + return state.map((f) => + f.id === action.file.uuid ? metadataToInfo(action.file) : f, + ); } if (fileStorageDidRemoveItem.matches(action)) { - return [...state].filter((value) => value.uuid !== action.file.uuid); + return state.filter((value) => value.id !== action.file.uuid); } return state; diff --git a/src/explorer/renameFileDialog/RenameFileDialog.tsx b/src/explorer/renameFileDialog/RenameFileDialog.tsx index 34ba7672..bfabcf0a 100644 --- a/src/explorer/renameFileDialog/RenameFileDialog.tsx +++ b/src/explorer/renameFileDialog/RenameFileDialog.tsx @@ -28,7 +28,7 @@ const RenameFileDialog: React.VFC = () => { const result = validateFileName( newName, extension, - files.map((f) => f.path), + files.map((f) => f.name), ); const inputRef = useRef(null); diff --git a/src/explorer/sagas.ts b/src/explorer/sagas.ts index bcfc7e47..bff37894 100644 --- a/src/explorer/sagas.ts +++ b/src/explorer/sagas.ts @@ -125,7 +125,7 @@ function* handleExplorerImportFiles(): Generator { const result = validateFileName( baseName, pythonFileExtension, - existingFiles.map((f) => f.path), + existingFiles.map((f) => f.name), ); if (result != FileNameValidationResult.IsOk) {