From d7b602ff342139e410f2e0331a6ce1a3a6ca019f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sat, 15 Aug 2026 19:42:49 +0000 Subject: [PATCH] a real dialog for New file, New folder and Clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `New file` shipped this morning behind `window.prompt()`, and prompt is the wrong tool for a reason that is not cosmetic: it cannot validate. It takes whatever you type, so the first thing you learned about a bad name was a red toast after the round trip — and for New file specifically, the server refuses to clobber, so the way you discovered a name was taken was by having the creation rejected. One dialog now serves all three. It checks on every keystroke, against the names already on screen: - empty is not an error, just nothing to submit — no red "required" before you have typed anything - `.` and `..` refused; a LEADING dot is fine, because a dotfile is a real thing to want - `/` refused rather than quietly doing mkdir -p, which is a surprise from a box labelled "Folder name" and is why the rename endpoint refuses slashes too - a collision is caught before the request, and case-SENSITIVELY: this is Linux, so Notes.txt and notes.txt genuinely coexist and refusing one would refuse a legal name The server keeps its own refusal. This is the courtesy; that is the lock. Clone joined them because it was the last prompt() I had added, and the URL check is deliberately loose: git takes ssh, git://, a local path and a host alias from ~/.ssh/config, so validating for https would break working setups to prevent a mistake git already reports well. Only whitespace is refused. The open state moved into `useFileBrowserApp` because two surfaces open the same dialog — the background menu and the toolbar — and each having its own would be two dialogs that could both be open. One mount, five call sites. The toolbar's New folder was on prompt() before today and is now on the dialog too, so there is no prompt() left anywhere in the file browser. 16 tests over the two validators. tsgo clean, frontend builds, 817 pass / 7 fail — the 7 unchanged. Co-Authored-By: Claude Opus 5 --- .../components/FileViewContainer.tsx | 44 +++-- .../components/NewEntryDialog.test.ts | 68 ++++++++ .../components/NewEntryDialog.tsx | 155 ++++++++++++++++++ .../components/Toolbar/DefaultActions.tsx | 7 +- .../FileBrowserApp/useFileBrowserApp.ts | 5 + 5 files changed, 250 insertions(+), 29 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.test.ts create mode 100644 src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.tsx diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx index dfcd287b..b1520c82 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx @@ -26,6 +26,7 @@ import { ContextMenuTrigger, } from '@/components/ui/context-menu'; import type { UseFileBrowserAppType } from '../useFileBrowserApp'; +import { NewEntryDialog } from './NewEntryDialog'; import { FileGrid } from './FileGrid'; type FileViewContainerProps = { @@ -53,6 +54,7 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps handleChatHere, handleCreateDir, handleCreateFile, + visibleEntries, handleCreateDashboardHere, setShowVideoDownload, setShowDictate, @@ -61,6 +63,8 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps showHidden, setShowHidden, clipboard, + newEntry, + setNewEntry, } = fileBrowserManager; const fileInputRef = useRef(null); @@ -94,26 +98,14 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps Paste )} - {/* Name taken whole, extension included — the extension is what the editor uses to pick a - language, so `notes.txt`, `deploy.sh` and `.env` all do the right thing. Opens straight - into the editor: creating an empty file you then have to find is half a feature. */} - { - const name = prompt('File name (with extension)'); - if (name?.trim()) handleCreateFile(name.trim()); - }} - className="cursor-pointer" - > + {/* Both open the same dialog. It validates as you type — against the names already on screen — + which `prompt()` structurally cannot: there the first thing you learn about a taken name is a + rejected request. The server still refuses to clobber; this is the courtesy, that is the lock. */} + setNewEntry('file')} className="cursor-pointer"> New file - { - const name = prompt('Folder name'); - if (name?.trim()) handleCreateDir(name.trim()); - }} - className="cursor-pointer" - > + setNewEntry('folder')} className="cursor-pointer"> New folder @@ -127,13 +119,7 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps {/* Was toolbar-only, and the toolbar hides it under `md:` — so on a phone there was no way to clone at all. */} - { - const url = prompt('Repository URL'); - if (url?.trim()) handleGitCloneUrl(url.trim()); - }} - className="cursor-pointer" - > + setNewEntry('clone')} className="cursor-pointer"> Clone repository @@ -262,6 +248,16 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps {folderMenu} )} + setNewEntry(null)} + onSubmit={(value) => { + if (newEntry === 'file') handleCreateFile(value); + else if (newEntry === 'folder') handleCreateDir(value); + else handleGitCloneUrl(value); + }} + existingNames={visibleEntries.map((e) => e.name)} + /> { diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.test.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.test.ts new file mode 100644 index 00000000..5c6fe39d --- /dev/null +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from 'bun:test'; +import { nameProblem, urlProblem } from './NewEntryDialog'; + +// The validation that replaced `window.prompt()`. Pure, and the whole reason the dialog exists — prompt +// could not do any of this, so a bad name only failed after a round trip. + +describe('nameProblem', () => { + test('an empty box is not an error, just nothing to submit', () => { + // Distinct from a bad name: showing "required" in red before the user has typed is nagging. + expect(nameProblem('', [])).toBeNull(); + expect(nameProblem(' ', [])).toBeNull(); + }); + + test('accepts the names people actually want', () => { + for (const name of ['notes.txt', 'deploy.sh', '.env', 'Makefile', 'my.photos', 'a b c.md']) { + expect(nameProblem(name, [])).toBeNull(); + } + }); + + test('a dotfile is fine — a leading dot is not a reserved name', () => { + expect(nameProblem('.gitignore', [])).toBeNull(); + expect(nameProblem('.', [])).not.toBeNull(); + expect(nameProblem('..', [])).not.toBeNull(); + }); + + test('refuses a slash rather than quietly creating intermediate folders', () => { + // `mkdir -p` semantics from a box labelled "Folder name" is a surprise, and the server's rename + // endpoint refuses slashes for the same reason. + expect(nameProblem('a/b', [])).toBe('Name cannot contain "/"'); + expect(nameProblem('/abs', [])).toBe('Name cannot contain "/"'); + }); + + test('catches a collision before the request, which is the point', () => { + expect(nameProblem('notes.txt', ['notes.txt'])).toBe('"notes.txt" already exists here'); + // Trimmed first, so trailing whitespace cannot smuggle a duplicate past the check and into a + // server-side clobber refusal. + expect(nameProblem('notes.txt ', ['notes.txt'])).toBe('"notes.txt" already exists here'); + }); + + test('collision is case-SENSITIVE, matching the filesystem underneath', () => { + // Linux, so `Notes.txt` and `notes.txt` genuinely coexist. Rejecting one would refuse a legal name. + expect(nameProblem('Notes.txt', ['notes.txt'])).toBeNull(); + }); +}); + +describe('urlProblem', () => { + test('accepts every transport git accepts, not just https', () => { + // Rejecting these would break working setups to prevent a mistake git reports perfectly well itself. + for (const url of [ + 'https://github.com/owner/repo.git', + 'git@github.com:owner/repo.git', + 'ssh://git@host:2222/repo.git', + 'git://host/repo.git', + '/srv/git/repo.git', + 'myhost:repo.git', + ]) { + expect(urlProblem(url)).toBeNull(); + } + }); + + test('refuses whitespace, the one thing that is always a typo', () => { + expect(urlProblem('https://host/a b.git')).toBe('A URL cannot contain spaces'); + }); + + test('empty is not an error', () => { + expect(urlProblem('')).toBeNull(); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.tsx new file mode 100644 index 00000000..b38dc25b --- /dev/null +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/NewEntryDialog.tsx @@ -0,0 +1,155 @@ +import { useEffect, useState } from 'react'; +import { FilePlus, FolderPlus, GitBranch } from 'lucide-react'; +import { Button } from '@/components/ui/button'; +import { + Dialog, + DialogContent, + DialogDescription, + DialogFooter, + DialogHeader, + DialogTitle, +} from '@/components/ui/dialog'; +import { Input } from '@/components/ui/input'; +import { Label } from '@/components/ui/label'; + +// Naming a new file or folder. +// +// This replaced two `window.prompt()` calls, and the reason is not that prompt is ugly. Prompt cannot +// validate: it takes whatever you type, and the first thing you learn about a bad name is a red toast +// after the round trip. Worse for `New file`, where the server refuses to clobber — so the way you found +// out a name was taken was by having the creation rejected. +// +// Here the check is in the form and runs on every keystroke, against the names already on screen. The +// server keeps its own refusal: this is the courtesy, that is the guarantee. + +type NewEntryDialogProps = { + /** `null` when closed. The kind is carried by the open state so one dialog serves both items. */ + kind: 'file' | 'folder' | 'clone' | null; + onClose: () => void; + onSubmit: (name: string) => void; + /** Names in the folder being viewed, for the collision check. Irrelevant to `clone`. */ + existingNames: string[]; +}; + +const COPY = { + file: { + title: 'New file', + description: 'Include the extension — it is what the editor uses to pick a language. It opens once created.', + label: 'File name', + placeholder: 'notes.txt', + submit: 'Create and open', + Icon: FilePlus, + }, + folder: { + title: 'New folder', + description: 'Created in the folder you are viewing.', + label: 'Folder name', + placeholder: 'documents', + submit: 'Create folder', + Icon: FolderPlus, + }, + clone: { + title: 'Clone repository', + description: 'Cloned into the folder you are viewing, under its own name.', + label: 'Repository URL', + placeholder: 'https://github.com/owner/repo.git', + submit: 'Clone', + Icon: GitBranch, + }, +} as const; + +/** + * Why a name is not usable, or null. + * + * `/` is rejected rather than silently creating intermediate folders: `mkdir -p` semantics from a box + * labelled "Folder name" is a surprise, and the server's rename endpoint refuses slashes for the same + * reason. Leading dots are fine — a dotfile is a real thing to want. + */ +export function nameProblem(raw: string, existing: string[]): string | null { + const name = raw.trim(); + if (!name) return null; // not an error yet, just nothing to submit + if (name === '.' || name === '..') return 'That name is reserved'; + if (name.includes('/')) return 'Name cannot contain "/"'; + if (existing.includes(name)) return `"${name}" already exists here`; + return null; +} + +/** + * Why a clone URL is not usable, or null. + * + * Deliberately loose. The server runs `git clone` and git accepts far more than https — ssh, git://, a + * local path, a host alias from ~/.ssh/config. Rejecting those here would break working setups to prevent + * a mistake git reports perfectly well itself. Only whitespace is refused, which is the one thing that + * is always a typo. + */ +export function urlProblem(raw: string): string | null { + const url = raw.trim(); + if (!url) return null; + if (/\s/.test(url)) return 'A URL cannot contain spaces'; + return null; +} + +export const NewEntryDialog = ({ kind, onClose, onSubmit, existingNames }: NewEntryDialogProps) => { + const [name, setName] = useState(''); + + // Clear on open rather than on close, so the field is never briefly showing the previous name as the + // dialog animates out. + useEffect(() => { + if (kind) setName(''); + }, [kind]); + + if (!kind) return null; + + const { title, description, label, placeholder, submit, Icon } = COPY[kind]; + const problem = kind === 'clone' ? urlProblem(name) : nameProblem(name, existingNames); + const canSubmit = name.trim().length > 0 && problem === null; + + const commit = () => { + if (!canSubmit) return; + onSubmit(name.trim()); + onClose(); + }; + + return ( + !open && onClose()}> + + + + + {title} + + {description} + + +
+ + setName(ev.target.value)} + onKeyDown={(ev) => { + if (ev.key === 'Enter') { + ev.preventDefault(); + commit(); + } + }} + /> + {/* Reserves its own line whether or not there is a problem, so the dialog does not jump as you + type past a colliding name. */} +

{problem ?? ''}

+
+ + + + + + + + ); +}; diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/Toolbar/DefaultActions.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/Toolbar/DefaultActions.tsx index e6dd0e6f..299a4621 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/Toolbar/DefaultActions.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/Toolbar/DefaultActions.tsx @@ -7,7 +7,7 @@ type DefaultActionsProps = { }; export const DefaultActions = ({ fileBrowserManager }: DefaultActionsProps) => { - const { handleCreateDir, handleUpload, clipboard, handlePaste } = fileBrowserManager; + const { setNewEntry, handleUpload, clipboard, handlePaste } = fileBrowserManager; const fileInputRef = useRef(null); const folderInputRef = useRef(null); @@ -21,10 +21,7 @@ export const DefaultActions = ({ fileBrowserManager }: DefaultActionsProps) => { return ( <>