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 ( <>