Skip to content

Commit 1709591

Browse files
dlechdlech
committed
fileStorage: untangle file id/name
We were using file UUID and path interchangeable. This adds a new UUID type for static checking and fixes a bunch of issues found.
1 parent daa6c07 commit 1709591

File tree

16 files changed

+417
-275
lines changed

16 files changed

+417
-275
lines changed

‎src/editor/Editor.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import MonacoEditor, {
1616
import { useDispatch } from 'react-redux';
1717
import { useTernaryDarkMode } from 'usehooks-ts';
1818
import { IDisposable } from 'xterm';
19-
import { fileStorageWriteFile } from '../fileStorage/actions';
19+
import { UUID, fileStorageWriteFile } from '../fileStorage/actions';
2020
import { compile } from '../mpy/actions';
2121
import { useSettingIsShowDocsEnabled } from '../settings/hooks';
2222
import { isMacOS } from '../utils/os';
@@ -268,7 +268,7 @@ const Editor: React.VFC = () => {
268268

269269
const handleChange = useCallback<ChangeHandler>(
270270
// REVISIT: need to ensure we have exclusive access to file
271-
(v) => dispatch(fileStorageWriteFile('main.py', v)),
271+
(v) => dispatch(fileStorageWriteFile('main.py' as UUID, v)),
272272
[dispatch],
273273
);
274274

‎src/explorer/Explorer.test.tsx‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,9 @@ import { getByLabelText, waitFor } from '@testing-library/dom';
55
import { cleanup } from '@testing-library/react';
66
import userEvent from '@testing-library/user-event';
77
import React from 'react';
8-
import { testRender } from '../../test';
8+
import { testRender, uuid } from '../../test';
99
import {
10+
FileMetadata,
1011
fileStorageArchiveAllFiles,
1112
fileStorageExportFile,
1213
} from '../fileStorage/actions';
@@ -19,10 +20,16 @@ afterEach(async () => {
1920
localStorage.clear();
2021
});
2122

23+
const testFile: FileMetadata = {
24+
uuid: uuid(0),
25+
path: 'test.file',
26+
sha256: '',
27+
};
28+
2229
describe('archive button', () => {
2330
it('should be enabled if there are files', () => {
2431
const [explorer, dispatch] = testRender(<Explorer />, {
25-
fileStorage: { fileNames: ['test.file'] },
32+
fileStorage: { files: [testFile] },
2633
});
2734

2835
const button = explorer.getByTitle('Backup all files');
@@ -34,7 +41,7 @@ describe('archive button', () => {
3441

3542
it('should be disabled if there are no files', () => {
3643
const [explorer, dispatch] = testRender(<Explorer />, {
37-
fileStorage: { fileNames: [] },
44+
fileStorage: { files: [] },
3845
});
3946

4047
const button = explorer.getByTitle('Backup all files');
@@ -75,7 +82,7 @@ describe('new file button', () => {
7582
describe('tree item', () => {
7683
it('should dispatch action when button is clicked', async () => {
7784
const [explorer, dispatch] = testRender(<Explorer />, {
78-
fileStorage: { fileNames: ['test.file'] },
85+
fileStorage: { files: [testFile] },
7986
});
8087

8188
expect(
@@ -93,7 +100,7 @@ describe('tree item', () => {
93100

94101
it('should dispatch action when key is pressed', async () => {
95102
const [explorer, dispatch] = testRender(<Explorer />, {
96-
fileStorage: { fileNames: ['test.file'] },
103+
fileStorage: { files: [testFile] },
97104
});
98105

99106
expect(
@@ -110,7 +117,7 @@ describe('tree item', () => {
110117

111118
it('should dispatch delete action when button is clicked', async () => {
112119
const [explorer, dispatch] = testRender(<Explorer />, {
113-
fileStorage: { fileNames: ['test.file'] },
120+
fileStorage: { files: [testFile] },
114121
});
115122

116123
// NB: this button is intentionally not accessible (by role) since
@@ -124,7 +131,7 @@ describe('tree item', () => {
124131

125132
it('should dispatch delete action when key is pressed', async () => {
126133
const [explorer, dispatch] = testRender(<Explorer />, {
127-
fileStorage: { fileNames: ['test.file'] },
134+
fileStorage: { files: [testFile] },
128135
});
129136

130137
const treeItem = explorer.getByRole('treeitem', { name: 'test.file' });
@@ -137,7 +144,7 @@ describe('tree item', () => {
137144

138145
it('should dispatch export action when button is clicked', async () => {
139146
const [explorer, dispatch] = testRender(<Explorer />, {
140-
fileStorage: { fileNames: ['test.file'] },
147+
fileStorage: { files: [testFile] },
141148
});
142149

143150
// NB: this button is intentionally not accessible (by role) since
@@ -151,7 +158,7 @@ describe('tree item', () => {
151158

152159
it('should dispatch export action when key is pressed', async () => {
153160
const [explorer, dispatch] = testRender(<Explorer />, {
154-
fileStorage: { fileNames: ['test.file'] },
161+
fileStorage: { files: [testFile] },
155162
});
156163

157164
const treeItem = explorer.getByRole('treeitem', { name: 'test.file' });

‎src/explorer/Explorer.tsx‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import {
2323
useTreeEnvironment,
2424
} from 'react-complex-tree';
2525
import { useDispatch } from 'react-redux';
26-
import { useDebounce } from 'usehooks-ts';
2726
import {
2827
fileStorageArchiveAllFiles,
2928
fileStorageExportFile,
@@ -131,15 +130,15 @@ type HeaderProps = {
131130
const Header: React.VoidFunctionComponent<HeaderProps> = ({ i18n }) => {
132131
const [isNewFileWizardOpen, setIsNewFileWizardOpen] = useState(false);
133132
const dispatch = useDispatch();
134-
const fileNames = useSelector((s) => s.fileStorage.fileNames);
133+
const files = useSelector((s) => s.fileStorage.files);
135134

136135
return (
137136
<div style={{ display: 'flex', justifyContent: 'flex-end' }}>
138137
<ButtonGroup minimal={true}>
139138
<ActionButton
140139
icon="archive"
141140
tooltip={i18n.translate(I18nId.HeaderExportAllTooltip)}
142-
disabled={fileNames.length === 0}
141+
disabled={files.length === 0}
143142
onClick={() => dispatch(fileStorageArchiveAllFiles())}
144143
/>
145144
<ActionButton
@@ -280,22 +279,21 @@ type FileTreeProps = {
280279

281280
const FileTree: React.VoidFunctionComponent<FileTreeProps> = ({ i18n }) => {
282281
const [focusedItem, setFocusedItem] = useState<TreeItemIndex>();
283-
const fileNames = useSelector((s) => s.fileStorage.fileNames);
284-
const debouncedFileNames = useDebounce(fileNames);
282+
const files = useSelector((s) => s.fileStorage.files);
285283
const liveDescriptors = useLiveDescriptors(i18n);
286284

287-
const rootItemIndex = '/';
285+
const rootItemIndex = 'root';
288286

289287
const treeItems = useMemo(
290288
() =>
291-
debouncedFileNames.reduce(
292-
(obj, fileName) => {
293-
const index = `/${fileName}`;
289+
files.reduce(
290+
(obj, file) => {
291+
const index = file.uuid;
294292

295293
obj[index] = {
296294
index,
297295
data: {
298-
fileName,
296+
fileName: file.path,
299297
icon: 'document',
300298
secondaryLabel: (
301299
<TreeItemContext.Consumer>
@@ -317,11 +315,14 @@ const FileTree: React.VoidFunctionComponent<FileTreeProps> = ({ i18n }) => {
317315
index: rootItemIndex,
318316
data: { fileName: '/' },
319317
hasChildren: true,
320-
children: debouncedFileNames.map((n) => `/${n}`),
318+
children: [...files]
319+
// REVISIT: consider using Intl.Collator() for i18n.locale
320+
.sort((a, b) => a.path.localeCompare(b.path))
321+
.map((n) => n.uuid),
321322
},
322323
} as Record<TreeItemIndex, FileTreeItem>,
323324
),
324-
[debouncedFileNames],
325+
[files],
325326
);
326327

327328
const getItemTitle = useCallback((item: FileTreeItem) => item.data.fileName, []);

‎src/explorer/actions.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,22 @@ export const explorerCreateNewFile = createAction(
6161
}),
6262
);
6363

64+
/**
65+
* Action that indicates that {@link explorerCreateNewFile} succeeded.
66+
*/
67+
export const explorerDidCreateNewFile = createAction(() => ({
68+
type: 'explorer.action.didCreateNewFile',
69+
}));
70+
71+
/**
72+
* Action that indicates that {@link explorerCreateNewFile} failed.
73+
* @param error The error.
74+
*/
75+
export const explorerDidFailToCreateNewFile = createAction((error: Error) => ({
76+
type: 'explorer.action.didFailToCreateNewFile',
77+
error,
78+
}));
79+
6480
/**
6581
* Action that requests to rename a file.
6682
* @param fileName The file name.

‎src/explorer/newFileWizard/NewFileWizard.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,11 @@ const NewFileWizard: React.VoidFunctionComponent<NewFileWizardProps> = ({
4141
const dispatch = useDispatch();
4242

4343
const [fileName, setFileName] = useState('');
44-
const fileNames = useSelector((s) => s.fileStorage.fileNames);
44+
const files = useSelector((s) => s.fileStorage.files);
4545
const fileNameValidation = validateFileName(
4646
fileName,
4747
pythonFileExtension,
48-
fileNames,
48+
files.map((f) => f.path),
4949
);
5050
const [hubType, setHubType] = useState(defaultHub);
5151

‎src/explorer/renameFileDialog/RenameFileDialog.tsx‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,12 @@ const RenameFileDialog: React.VFC = () => {
2424
const [baseName, extension] = oldName.split(/(\.\w+)$/);
2525

2626
const [newName, setNewName] = useState(baseName);
27-
const fileNames = useSelector((s) => s.fileStorage.fileNames);
28-
const result = validateFileName(newName, extension, fileNames);
27+
const files = useSelector((s) => s.fileStorage.files);
28+
const result = validateFileName(
29+
newName,
30+
extension,
31+
files.map((f) => f.path),
32+
);
2933

3034
const inputRef = useRef<HTMLInputElement>(null);
3135

‎src/explorer/sagas.test.ts‎

Lines changed: 32 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ import { FileWithHandle } from 'browser-fs-access';
66
import { mock } from 'jest-mock-extended';
77
import { AsyncSaga, uuid } from '../../test';
88
import {
9-
fileStorageDidFailToOpenFile,
109
fileStorageDidFailToRenameFile,
1110
fileStorageDidOpenFile,
1211
fileStorageDidRenameFile,
12+
fileStorageDidWriteFile,
1313
fileStorageOpenFile,
1414
fileStorageRenameFile,
1515
fileStorageWriteFile,
@@ -18,6 +18,7 @@ import { pythonFileExtension } from '../pybricksMicropython/lib';
1818
import {
1919
Hub,
2020
explorerCreateNewFile,
21+
explorerDidCreateNewFile,
2122
explorerDidFailToImportFiles,
2223
explorerDidFailToRenameFile,
2324
explorerDidImportFiles,
@@ -48,11 +49,17 @@ describe('handleExplorerImportFiles', () => {
4849

4950
saga.put(explorerImportFiles());
5051

51-
const action = await saga.take();
52-
expect(action).toEqual(fileStorageWriteFile(testFileName, testFileContents));
52+
await expect(saga.take()).resolves.toEqual(fileStorageOpenFile(testFileName));
53+
54+
saga.put(fileStorageDidOpenFile(testFileName, uuid(0)));
55+
56+
await expect(saga.take()).resolves.toEqual(
57+
fileStorageWriteFile(uuid(0), testFileContents),
58+
);
59+
60+
saga.put(fileStorageDidWriteFile(uuid(0)));
5361

54-
const action2 = await saga.take();
55-
expect(action2).toEqual(explorerDidImportFiles());
62+
await expect(saga.take()).resolves.toEqual(explorerDidImportFiles());
5663

5764
await saga.end();
5865
});
@@ -79,8 +86,11 @@ describe('handleExplorerCreateNewFile', () => {
7986

8087
saga.put(explorerCreateNewFile('test', pythonFileExtension, Hub.Technic));
8188

82-
const action = await saga.take();
83-
expect(action).toMatchInlineSnapshot(`
89+
await expect(saga.take()).resolves.toEqual(fileStorageOpenFile('test.py'));
90+
91+
saga.put(fileStorageDidOpenFile('test.py', uuid(0)));
92+
93+
await expect(saga.take()).resolves.toMatchInlineSnapshot(`
8494
Object {
8595
"contents": "from pybricks.hubs import TechnicHub
8696
from pybricks.pupdevices import Motor
@@ -91,11 +101,15 @@ describe('handleExplorerCreateNewFile', () => {
91101
hub = TechnicHub()
92102
93103
",
94-
"id": "test.py",
104+
"id": "00000000-0000-0000-0000-000000000000",
95105
"type": "fileStorage.action.writeFile",
96106
}
97107
`);
98108

109+
saga.put(fileStorageDidWriteFile(uuid(0)));
110+
111+
await expect(saga.take()).resolves.toEqual(explorerDidCreateNewFile());
112+
99113
await saga.end();
100114
});
101115
});
@@ -121,39 +135,23 @@ describe('handleExplorerRenameFile', () => {
121135
beforeEach(async () => {
122136
saga.put(renameFileDialogDidAccept('old.file', 'new.file'));
123137

124-
await expect(saga.take()).resolves.toEqual(fileStorageOpenFile('old.file'));
138+
await expect(saga.take()).resolves.toEqual(
139+
fileStorageRenameFile('old.file', 'new.file'),
140+
);
125141
});
126142

127-
it('should dispatch action on fileStorageOpenFile failure', async () => {
128-
saga.put(fileStorageDidFailToOpenFile('old.file', new Error('test error')));
143+
it('should dispatch action on fileStorageRenameFile failure', async () => {
144+
saga.put(
145+
fileStorageDidFailToRenameFile('old.file', new Error('test error')),
146+
);
129147

130148
await expect(saga.take()).resolves.toEqual(explorerDidFailToRenameFile());
131149
});
132150

133-
describe('should dispatch fileStorageRenameFile action on fileStorageOpenFile success', () => {
134-
beforeEach(async () => {
135-
saga.put(fileStorageDidOpenFile('old.file', uuid(0)));
136-
137-
await expect(saga.take()).resolves.toEqual(
138-
fileStorageRenameFile(uuid(0), 'new.file'),
139-
);
140-
});
141-
142-
it('should dispatch action on fileStorageRenameFile failure', async () => {
143-
saga.put(
144-
fileStorageDidFailToRenameFile(uuid(0), new Error('test error')),
145-
);
146-
147-
await expect(saga.take()).resolves.toEqual(
148-
explorerDidFailToRenameFile(),
149-
);
150-
});
151-
152-
it('should dispatch action on fileStorageRenameFile success', async () => {
153-
saga.put(fileStorageDidRenameFile(uuid(0)));
151+
it('should dispatch action on fileStorageRenameFile success', async () => {
152+
saga.put(fileStorageDidRenameFile('old.file'));
154153

155-
await expect(saga.take()).resolves.toEqual(explorerDidRenameFile());
156-
});
154+
await expect(saga.take()).resolves.toEqual(explorerDidRenameFile());
157155
});
158156
});
159157

0 commit comments

Comments
 (0)