From 1035f2b46de358f9700443469e6f25986d8064fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ari=20Perkki=C3=B6?= Date: Thu, 19 Sep 2024 16:36:04 +0300 Subject: [PATCH 1/3] fix: prevent overwriting template files via `` --- .../react-examples/ExampleFileTree.tsx | 2 +- .../content/docs/reference/configuration.mdx | 18 +++++- .../docs/reference/react-components.mdx | 4 +- e2e/src/templates/default/file-on-template.js | 1 + .../default/folder-on-template/.gitkeep | 0 e2e/src/templates/file-server/index.mjs | 2 +- e2e/test/file-tree.test.ts | 26 ++++++++ packages/react/src/Panels/WorkspacePanel.tsx | 2 +- packages/react/src/core/ContextMenu.tsx | 59 +++++++++++++------ packages/runtime/src/store/index.ts | 14 ++++- packages/runtime/src/store/tutorial-runner.ts | 56 ++++++++++++++++++ packages/types/src/default-localization.ts | 1 + packages/types/src/entities/index.ts | 2 + packages/types/src/schemas/i18n.ts | 12 ++++ 14 files changed, 172 insertions(+), 27 deletions(-) create mode 100644 e2e/src/templates/default/file-on-template.js create mode 100644 e2e/src/templates/default/folder-on-template/.gitkeep diff --git a/docs/tutorialkit.dev/src/components/react-examples/ExampleFileTree.tsx b/docs/tutorialkit.dev/src/components/react-examples/ExampleFileTree.tsx index f95b8cecb..df1f51a9c 100644 --- a/docs/tutorialkit.dev/src/components/react-examples/ExampleFileTree.tsx +++ b/docs/tutorialkit.dev/src/components/react-examples/ExampleFileTree.tsx @@ -13,7 +13,7 @@ export default function ExampleFileTree() { hiddenFiles={['package-lock.json']} selectedFile={selectedFile} onFileSelect={setSelectedFile} - onFileChange={(event) => { + onFileChange={async (event) => { if (event.method === 'add') { setFiles([...files, { path: event.value, type: event.type }]); } diff --git a/docs/tutorialkit.dev/src/content/docs/reference/configuration.mdx b/docs/tutorialkit.dev/src/content/docs/reference/configuration.mdx index e1f3ef41a..a1c6419a5 100644 --- a/docs/tutorialkit.dev/src/content/docs/reference/configuration.mdx +++ b/docs/tutorialkit.dev/src/content/docs/reference/configuration.mdx @@ -103,15 +103,29 @@ type I18nText = { * * @default 'This action is not allowed' */ - fileTreeActionNotAllowed?: string, + fileTreeActionNotAllowedText?: string, /** - * Text shown on dialog describing allowed patterns when file or folder createion failed. + * Text shown on dialog when user attempts create file or folder that already exists on filesystem but is not visible on file tree, e.g. template files. + * + * @default 'File exists on filesystem already' + */ + fileTreeFileExistsAlreadyText?: string, + + /** + * Text shown on dialog describing allowed patterns when file or folder creation failed. * * @default 'Created files and folders must match following patterns:' */ fileTreeAllowedPatternsText?: string, + /** + * Text shown on confirmation buttons on dialogs. + * + * @default 'OK' + */ + confirmationText?: string, + /** * Text shown on top of the steps section. * diff --git a/docs/tutorialkit.dev/src/content/docs/reference/react-components.mdx b/docs/tutorialkit.dev/src/content/docs/reference/react-components.mdx index a659578c4..cae59f462 100644 --- a/docs/tutorialkit.dev/src/content/docs/reference/react-components.mdx +++ b/docs/tutorialkit.dev/src/content/docs/reference/react-components.mdx @@ -107,13 +107,15 @@ A component to list files in a tree view. * `onFileSelect: (file: string) => void` - A callback that will be called when a file is clicked. The path of the file that was clicked will be passed as an argument. -* `onFileChange: (event: FileChangeEvent) => void` - An optional callback that will be called when a new file or folder is created from the file tree's context menu. When callback is not passed, file tree does not allow adding new files. +* `onFileChange: (event: FileChangeEvent) => Promise` - An optional callback that will be called when a new file or folder is created from the file tree's context menu. This callback should throw errors with `FilesystemError` messages. ```ts interface FileChangeEvent { type: 'file' | 'folder'; method: 'add' | 'remove' | 'rename'; value: string; } + + type FilesystemError = 'FILE_EXISTS' | 'FOLDER_EXISTS'; ``` * `allowEditPatterns?: string[]` - Glob patterns for paths that allow editing files and folders. Disabled by default. diff --git a/e2e/src/templates/default/file-on-template.js b/e2e/src/templates/default/file-on-template.js new file mode 100644 index 000000000..5f9ed1eac --- /dev/null +++ b/e2e/src/templates/default/file-on-template.js @@ -0,0 +1 @@ +export default 'This file is present on template'; diff --git a/e2e/src/templates/default/folder-on-template/.gitkeep b/e2e/src/templates/default/folder-on-template/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/e2e/src/templates/file-server/index.mjs b/e2e/src/templates/file-server/index.mjs index 406e08585..7ffeb0f39 100644 --- a/e2e/src/templates/file-server/index.mjs +++ b/e2e/src/templates/file-server/index.mjs @@ -1,5 +1,5 @@ -import http from 'node:http'; import fs from 'node:fs'; +import http from 'node:http'; const server = http.createServer((req, res) => { if (req.url === '/' || req.url === '/index.html') { diff --git a/e2e/test/file-tree.test.ts b/e2e/test/file-tree.test.ts index 72c3bc510..bd13ed08c 100644 --- a/e2e/test/file-tree.test.ts +++ b/e2e/test/file-tree.test.ts @@ -222,3 +222,29 @@ test('user cannot create files or folders in disallowed directories', async ({ p await expect(dialog).not.toBeVisible(); } }); + +test('user cannot create files or folders that exist on template', async ({ page }) => { + await page.goto(`${BASE_URL}/allow-edits-enabled`); + await expect(page.getByRole('heading', { level: 1, name: 'File Tree test - Allow Edits enabled' })).toBeVisible(); + + // wait for terminal to start + const terminalOutput = page.getByRole('tabpanel', { name: 'Terminal' }); + await expect(terminalOutput).toContainText('~/tutorial', { useInnerText: true }); + + for (const [name, type] of [ + ['file-on-template.js', 'file'], + ['folder-on-template', 'folder'], + ] as const) { + await page.getByTestId('file-tree-root-context-menu').click({ button: 'right' }); + await page.getByRole('menuitem', { name: `Create ${type}` }).click(); + + await page.locator('*:focus').fill(name); + await page.locator('*:focus').press('Enter'); + + const dialog = page.getByRole('dialog', { name: 'This action is not allowed' }); + await expect(dialog.getByText('File exists on filesystem already')).toBeVisible(); + + await dialog.getByRole('button', { name: 'OK' }).click(); + await expect(dialog).not.toBeVisible(); + } +}); diff --git a/packages/react/src/Panels/WorkspacePanel.tsx b/packages/react/src/Panels/WorkspacePanel.tsx index 9546fcfaa..7665b0ef8 100644 --- a/packages/react/src/Panels/WorkspacePanel.tsx +++ b/packages/react/src/Panels/WorkspacePanel.tsx @@ -122,7 +122,7 @@ function EditorSection({ theme, tutorialStore, hasEditor }: PanelProps) { } } - function onFileTreeChange({ method, type, value }: FileTreeChangeEvent) { + async function onFileTreeChange({ method, type, value }: FileTreeChangeEvent) { if (method === 'add' && type === 'file') { return tutorialStore.addFile(value); } diff --git a/packages/react/src/core/ContextMenu.tsx b/packages/react/src/core/ContextMenu.tsx index 0b70004ab..343dc0df3 100644 --- a/packages/react/src/core/ContextMenu.tsx +++ b/packages/react/src/core/ContextMenu.tsx @@ -1,6 +1,6 @@ import { Root, Portal, Content, Item, Trigger } from '@radix-ui/react-context-menu'; import * as RadixDialog from '@radix-ui/react-dialog'; -import { DEFAULT_LOCALIZATION, type FileDescriptor, type I18n } from '@tutorialkit/types'; +import { DEFAULT_LOCALIZATION, type FileDescriptor, type I18n, type FilesystemError } from '@tutorialkit/types'; import picomatch from 'picomatch/posix'; import { useRef, useState, type ComponentProps, type ReactNode } from 'react'; import { Button } from '../Button.js'; @@ -18,8 +18,8 @@ interface FileRenameEvent extends FileChangeEvent { } interface Props extends ComponentProps<'div'> { - /** Callback invoked when file is changed. */ - onFileChange?: (event: FileChangeEvent | FileRenameEvent) => void; + /** Callback invoked when file is changed. This callback should throw errors with {@link FilesystemError} messages. */ + onFileChange?: (event: FileChangeEvent | FileRenameEvent) => Promise; /** Glob patterns for paths that allow editing files and folders. Disabled by default. */ allowEditPatterns?: string[]; @@ -37,6 +37,7 @@ interface Props extends ComponentProps<'div'> { | 'fileTreeCreateFolderText' | 'fileTreeActionNotAllowedText' | 'fileTreeAllowedPatternsText' + | 'fileTreeFileExistsAlreadyText' | 'confirmationText' >; @@ -54,14 +55,16 @@ export function ContextMenu({ triggerProps, ...props }: Props) { - const [state, setState] = useState<'idle' | 'add_file' | 'add_folder' | 'add_failed'>('idle'); + const [state, setState] = useState< + 'idle' | 'add_file' | 'add_folder' | 'add_failed_not_allowed' | 'add_failed_exists' + >('idle'); const inputRef = useRef(null); if (!allowEditPatterns?.length) { return children; } - function onFileNameEnd(event: React.KeyboardEvent | React.FocusEvent) { + async function onFileNameEnd(event: React.KeyboardEvent | React.FocusEvent) { if (state !== 'add_file' && state !== 'add_folder') { return; } @@ -72,14 +75,22 @@ export function ContextMenu({ const value = `${directory}/${name}`; const isAllowed = picomatch.isMatch(value, allowEditPatterns!); - if (isAllowed) { - onFileChange?.({ + if (!isAllowed) { + return setState('add_failed_not_allowed'); + } + + try { + await onFileChange?.({ value, type: state === 'add_file' ? 'file' : 'folder', method: 'add', }); - } else { - return setState('add_failed'); + } catch (error: any) { + const message: FilesystemError | (string & {}) | undefined = error?.message; + + if (message === 'FILE_EXISTS' || message === 'FOLDER_EXISTS') { + return setState('add_failed_exists'); + } } } @@ -140,20 +151,20 @@ export function ContextMenu({ - {state === 'add_failed' && ( + {(state === 'add_failed_not_allowed' || state === 'add_failed_exists') && ( setState('idle')} > - {i18n?.fileTreeAllowedPatternsText || DEFAULT_LOCALIZATION.fileTreeAllowedPatternsText} -
    1 && 'list-disc ml-4')}> - {allowEditPatterns.map((pattern) => ( -
  • - {pattern} -
    • - ))} -
+ {state === 'add_failed_not_allowed' ? ( + <> + {i18n?.fileTreeAllowedPatternsText || DEFAULT_LOCALIZATION.fileTreeAllowedPatternsText} + + + ) : ( + <>{i18n?.fileTreeFileExistsAlreadyText || DEFAULT_LOCALIZATION.fileTreeFileExistsAlreadyText} + )} )} @@ -203,3 +214,15 @@ function Dialog({ ); } + +function AllowPatternsList({ allowEditPatterns }: Required>) { + return ( +
    1 && 'list-disc ml-4')}> + {allowEditPatterns.map((pattern) => ( +
  • + {pattern} +
    • + ))} +
+ ); +} diff --git a/packages/runtime/src/store/index.ts b/packages/runtime/src/store/index.ts index 91efbe405..5c500ac98 100644 --- a/packages/runtime/src/store/index.ts +++ b/packages/runtime/src/store/index.ts @@ -1,4 +1,4 @@ -import type { FileDescriptor, Files, Lesson } from '@tutorialkit/types'; +import type { FileDescriptor, Files, FilesystemError, Lesson } from '@tutorialkit/types'; import type { WebContainer } from '@webcontainer/api'; import { atom, type ReadableAtom } from 'nanostores'; import { LessonFilesFetcher } from '../lesson-files.js'; @@ -312,7 +312,7 @@ export class TutorialStore { this._editorStore.setSelectedFile(filePath); } - addFile(filePath: string) { + async addFile(filePath: string): Promise { // always select the existing or newly created file this.setSelectedFile(filePath); @@ -321,16 +321,24 @@ export class TutorialStore { return; } + if (await this._runner.fileExists(filePath)) { + throw new Error('FILE_EXISTS' satisfies FilesystemError); + } + this._editorStore.addFileOrFolder({ path: filePath, type: 'file' }); this._runner.updateFile(filePath, ''); } - addFolder(folderPath: string) { + async addFolder(folderPath: string) { // prevent creating duplicates if (this._editorStore.files.get().some((file) => file.path.startsWith(folderPath))) { return; } + if (await this._runner.folderExists(folderPath)) { + throw new Error('FOLDER_EXISTS' satisfies FilesystemError); + } + this._editorStore.addFileOrFolder({ path: folderPath, type: 'folder' }); this._runner.createFolder(folderPath); } diff --git a/packages/runtime/src/store/tutorial-runner.ts b/packages/runtime/src/store/tutorial-runner.ts index 5f565e632..615eff49f 100644 --- a/packages/runtime/src/store/tutorial-runner.ts +++ b/packages/runtime/src/store/tutorial-runner.ts @@ -189,6 +189,62 @@ export class TutorialRunner { ); } + async fileExists(filepath: string) { + const previousLoadPromise = this._currentLoadTask?.promise; + + return new Promise((resolve, reject) => { + this._currentLoadTask = newTask( + async (signal) => { + await previousLoadPromise; + + const webcontainer = await this._webcontainer; + + if (signal.aborted) { + reject(new Error('Task was aborted')); + } + + signal.throwIfAborted(); + + try { + await webcontainer.fs.readFile(filepath); + resolve(true); + } catch { + resolve(false); + } + }, + { ignoreCancel: true }, + ); + }); + } + + async folderExists(folderPath: string) { + const previousLoadPromise = this._currentLoadTask?.promise; + + return new Promise((resolve, reject) => { + this._currentLoadTask = newTask( + async (signal) => { + await previousLoadPromise; + + const webcontainer = await this._webcontainer; + + if (signal.aborted) { + reject(new Error('Task was aborted')); + } + + signal.throwIfAborted(); + + try { + await webcontainer.fs.readdir(folderPath); + resolve(true); + } catch { + resolve(false); + } + }, + { ignoreCancel: true }, + ); + }); + } + /** * Load the provided files into WebContainer and remove any other files that had been loaded previously. * diff --git a/packages/types/src/default-localization.ts b/packages/types/src/default-localization.ts index 6d3c95dd0..f3f158b7a 100644 --- a/packages/types/src/default-localization.ts +++ b/packages/types/src/default-localization.ts @@ -10,6 +10,7 @@ export const DEFAULT_LOCALIZATION = { fileTreeCreateFileText: 'Create file', fileTreeCreateFolderText: 'Create folder', fileTreeActionNotAllowedText: 'This action is not allowed', + fileTreeFileExistsAlreadyText: 'File exists on filesystem already', fileTreeAllowedPatternsText: 'Created files and folders must match following patterns:', confirmationText: 'OK', prepareEnvironmentTitleText: 'Preparing Environment', diff --git a/packages/types/src/entities/index.ts b/packages/types/src/entities/index.ts index c5ec81c6d..dd224b2b2 100644 --- a/packages/types/src/entities/index.ts +++ b/packages/types/src/entities/index.ts @@ -6,6 +6,8 @@ export type * from './nav.js'; export type FileDescriptor = { path: string; type: 'file' | 'folder' }; export type Files = Record; +export type FilesystemError = 'FILE_EXISTS' | 'FOLDER_EXISTS'; + /** * This tuple contains a "ref" which points to a file to fetch with the `LessonFilesFetcher` and * the list of file paths included by that ref. diff --git a/packages/types/src/schemas/i18n.ts b/packages/types/src/schemas/i18n.ts index 5c596dea0..a234d7ee2 100644 --- a/packages/types/src/schemas/i18n.ts +++ b/packages/types/src/schemas/i18n.ts @@ -82,6 +82,18 @@ export const i18nSchema = z.object({ .optional() .describe("Text shown on dialog when user attempts to edit files that don't match allowed patterns."), + /** + * Text shown on dialog when user attempts create file or folder that already exists on filesystem but is not visible on file tree, e.g. template files. + * + * @default 'File exists on filesystem already' + */ + fileTreeFileExistsAlreadyText: z + .string() + .optional() + .describe( + 'Text shown on dialog when user attempts create file or folder that already exists on filesystem but is not visible on file tree, e.g. template files.', + ), + /** * Text shown on dialog describing allowed patterns when file or folder creation failed. * From 188f9b16651d753d69f6b8a2712d084921566743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ari=20Perkki=C3=B6?= Date: Mon, 23 Sep 2024 13:49:56 +0300 Subject: [PATCH 2/3] fix: code review --- packages/runtime/src/store/tutorial-runner.ts | 51 +++++++------------ 1 file changed, 17 insertions(+), 34 deletions(-) diff --git a/packages/runtime/src/store/tutorial-runner.ts b/packages/runtime/src/store/tutorial-runner.ts index 615eff49f..e1df25d7c 100644 --- a/packages/runtime/src/store/tutorial-runner.ts +++ b/packages/runtime/src/store/tutorial-runner.ts @@ -190,51 +190,34 @@ export class TutorialRunner { } async fileExists(filepath: string) { - const previousLoadPromise = this._currentLoadTask?.promise; - - return new Promise((resolve, reject) => { - this._currentLoadTask = newTask( - async (signal) => { - await previousLoadPromise; - - const webcontainer = await this._webcontainer; - - if (signal.aborted) { - reject(new Error('Task was aborted')); - } - - signal.throwIfAborted(); - - try { - await webcontainer.fs.readFile(filepath); - resolve(true); - } catch { - resolve(false); - } - }, - { ignoreCancel: true }, - ); - }); + return this._fsExists(filepath, 'file'); } async folderExists(folderPath: string) { + return this._fsExists(folderPath, 'folder'); + } + + private async _fsExists(filepath: string, type: 'file' | 'folder') { + if (this._currentFiles?.[filepath] || this._currentTemplate?.[filepath]) { + return true; + } + const previousLoadPromise = this._currentLoadTask?.promise; - return new Promise((resolve, reject) => { + return new Promise((resolve) => { this._currentLoadTask = newTask( - async (signal) => { + async () => { await previousLoadPromise; const webcontainer = await this._webcontainer; - if (signal.aborted) { - reject(new Error('Task was aborted')); - } - - signal.throwIfAborted(); - try { - await webcontainer.fs.readdir(folderPath); + if (type === 'file') { + await webcontainer.fs.readFile(filepath); + } else { + await webcontainer.fs.readdir(filepath); + } + resolve(true); } catch { resolve(false); From 30314b3e99f6a667863f684e535770d6fc8ff519 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ari=20Perkki=C3=B6?= Date: Mon, 23 Sep 2024 15:00:52 +0300 Subject: [PATCH 3/3] fix: code review --- packages/react/src/core/ContextMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/src/core/ContextMenu.tsx b/packages/react/src/core/ContextMenu.tsx index 343dc0df3..53751c54b 100644 --- a/packages/react/src/core/ContextMenu.tsx +++ b/packages/react/src/core/ContextMenu.tsx @@ -200,7 +200,7 @@ function Dialog({ -
+
{title}
{children}