From 84d6d7e7ce4c80fc4d1c4b4ebe334affee60acbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Sat, 15 Aug 2026 19:31:05 +0000 Subject: [PATCH] file browser: the eight defects from the audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Kebab Cut/Copy silently did nothing, and sometimes cut the wrong rows. `EllipsisMenu` stops click propagation so opening it does not toggle selection — but Cut, Copy, Download, Delete and Compress all act on the SELECTION, which meant the kebab operated on whatever was selected elsewhere, or returned early with no toast at all. Right-click never had this because it selects first. Both now call one `ensureSelected`, on open. 2. Every menu vanished during a search. That branch had no ContextMenu and its rows are not FileItems, so right-click fell through to the browser. The folder menu is now declared once and mounted in both branches — it acts on `currentPath`, which a search does not change, so it stays meaningful. Search ROWS get their own three-item menu rather than the row menu: a hit carries an absolute path from anywhere in the tree, while every row-menu handler builds its path from the folder being browsed. Reusing it would have acted on a different file with the same name. `DirEntry.path` is optional — only search results carry one — so it is guarded, not asserted. 3. Download and Copy path now fan out over a multi-selection, the rule Delete and Compress already used. The menu downloaded one of six selected files while the toolbar zipped all six. Chat is deliberately NOT included: `chatContext` is a single path that the provider string-splits into a working directory, so a list would need Chat's contract changed. Left alone rather than half-done. 4. Read Aloud stopped offering TTS on binaries — same guard the editor got, since `text` is `getFileType`'s fallback and caught .exe and .sqlite. 5. `POST /file-browser/download` is filed as a read. It zips a selection and writes nothing; it is a POST only because a list of paths does not fit a query string. A read-level member could download a whole folder but not two files. Proved by hand that `readOnlyWrites: ['/download']` permits GET and POST /download, and still denies /download-video, /rm and /write — `isPrefixOf` is segment-aware, so the sibling route does not leak. 6. `disabled` had no use anywhere in the feature — the shared item type did not even expose it. It does now, and the first real case: `/file-browser/read` refuses over 5 MB, so Open in editor on a 40 MB log opened an editor that then failed. Hiding it would have suggested the file is not text; disabled says the true thing. 7. The grid kebab could fade out while its own dropdown was open — list view had `data-[state=open]:opacity-100` and grid did not. 8. Dead exports `getMatchingTasks` / `getMatchingAgents` removed. Both were the ungrouped input to the grouped form the menu actually renders; exporting them invited a second code path that never arrived. 9 is untouched on purpose — a plugin cannot register a menu item, and the music plugin is where that should be designed. tsgo clean, frontend builds, 808 pass / 7 fail unchanged. Co-Authored-By: Claude Opus 5 --- src/servers/permissions/registry.ts | 7 + .../FileBrowserApp/components/FileItem.tsx | 63 +++- .../components/FileViewContainer.tsx | 273 ++++++++++-------- .../FileBrowserApp/useFileBrowserApp.ts | 38 ++- .../src/apps/FileBrowser/useAgents.ts | 2 +- .../src/apps/FileBrowser/useTasks.ts | 4 +- 6 files changed, 254 insertions(+), 133 deletions(-) diff --git a/src/servers/permissions/registry.ts b/src/servers/permissions/registry.ts index fb26eeb2..312d30f3 100644 --- a/src/servers/permissions/registry.ts +++ b/src/servers/permissions/registry.ts @@ -311,6 +311,13 @@ const CORE_REGISTRY: Permission[] = [ kind: 'confined', api: ['/file-browser', '/upload'], routes: ['/files', '/code-editor'], + // `POST /file-browser/download` zips a multi-selection and streams it back. It writes nothing — it is + // a POST only because a list of paths does not fit a query string. Without this a read-level member + // could download a whole folder (GET /download) but not two files, which is not a rule anyone chose. + // + // `isPrefixOf` is segment-aware, so this does NOT also permit `/download-video` — which really is a + // write, fetching a video onto the disk. + readOnlyWrites: ['/download'], }, { key: 'tasks', diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileItem.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileItem.tsx index b2d9d0dc..3611c39d 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileItem.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileItem.tsx @@ -152,10 +152,19 @@ const BINARY_EXTS = new Set([ 'ods', ]); +/** `/file-browser/read` refuses past this, so the editor cannot open it either. Keep the two in step. */ +const MAX_EDITABLE_BYTES = 5 * 1024 * 1024; + const isBinaryName = (name: string): boolean => BINARY_EXTS.has(name.split('.').pop()?.toLowerCase() ?? ''); type MenuPrimitives = { - Item: React.ComponentType<{ onClick?: () => void; className?: string; children: React.ReactNode }>; + Item: React.ComponentType<{ + onClick?: () => void; + /** Radix supports it on both families; the shared type just never surfaced it, so nothing used it. */ + disabled?: boolean; + className?: string; + children: React.ReactNode; + }>; Separator: React.ComponentType>; Sub: React.ComponentType<{ children: React.ReactNode }>; SubTrigger: React.ComponentType<{ className?: string; children: React.ReactNode }>; @@ -225,7 +234,10 @@ const MenuItems = ({ }: MenuItemsProps & { ui: MenuPrimitives }) => { const isDir = entry.type === 'directory'; const fileType = entry.type === 'file' ? getFileType(entry.name) : null; - const showReadAloud = fileType === 'markdown' || fileType === 'code' || fileType === 'text'; + // Same binary guard as the editor. `text` is the FALLBACK type, so without this Read Aloud offered to + // speak .exe and .sqlite — it read the bytes as UTF-8 and sent the mojibake to a TTS engine. + const showReadAloud = + (fileType === 'markdown' || fileType === 'code' || fileType === 'text') && !isBinaryName(entry.name); const showExtract = fileType === 'archive'; // Editable in the code editor. // @@ -238,6 +250,7 @@ const MenuItems = ({ // are named rather than guessed at, because a denylist that is too short only ever costs a bad render, // while a `text` test that is too strict costs the feature. const showEdit = (fileType === 'markdown' || fileType === 'code' || fileType === 'text') && !isBinaryName(entry.name); + const tooBigToEdit = entry.type === 'file' && entry.size > MAX_EDITABLE_BYTES; const hasTasks = taskGroups.length > 0; const hasAgents = agentGroups.length > 0; @@ -258,9 +271,17 @@ const MenuItems = ({ Open {showEdit && ( - onOpenInEditor(entry)} className="cursor-pointer"> + // The first place `disabled` is used rather than hiding. `/file-browser/read` refuses anything + // over 5 MB, so a 40 MB log would open an editor that then fails — and hiding the item would + // just as wrongly suggest the file is not text. Disabled says the true thing: this is editable, + // but not here. + onOpenInEditor(entry)} + disabled={tooBigToEdit} + className={tooBigToEdit ? '' : 'cursor-pointer'} + > - Open in editor + {tooBigToEdit ? 'Open in editor (too large)' : 'Open in editor'} )} {/* Paste INTO a folder. The background menu could only paste into the folder already open, so a @@ -419,9 +440,17 @@ const CONTEXT_UI = { const DropdownMenuItems = (props: MenuItemsProps) => ; const ContextMenuItems = (props: MenuItemsProps) => ; -const EllipsisMenu = (props: MenuItemsProps) => ( +/** + * The overflow menu. + * + * `stopPropagation` keeps opening the menu from toggling the row's selection — but it also meant the row + * was never selected, and `Cut`/`Copy` act on the SELECTION. So the kebab's Cut did nothing on a fresh + * listing, and worse, cut a different row's selection when one existed. Right-click never had this + * because `handleContextMenu` selects first; this now does the same on open. + */ +const EllipsisMenu = ({ onEnsureSelected, ...props }: MenuItemsProps & { onEnsureSelected: () => void }) => (
ev.stopPropagation()}> - + open && onEnsureSelected()}>
@@ -713,8 +752,8 @@ export const FileItem = ({ onSelect(entry, ev)} /> -
- +
+
{iconLarge} 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 1f472433..dfcd287b 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/FileViewContainer.tsx @@ -12,6 +12,7 @@ import { MessageSquare, Download, Mic, + ExternalLink, GitBranch, Eye, EyeOff, @@ -47,6 +48,8 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps loading, handlePaste, handleCopyCurrentPath, + handleCopyAbsPath, + handleDownloadPath, handleChatHere, handleCreateDir, handleCreateFile, @@ -70,6 +73,91 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps } }; + // The folder menu, shared by the browse view and the search view. It acts on `currentPath`, which does + // not change while a search is open — so "New file", "Paste" and the rest stay meaningful there. + const folderMenu = ( + + + + Copy path + + + + Chat about this + + {/* Gated on the clipboard, as both toolbars already were. Ungated it fell through to the + OS clipboard path, which on an insecure origin toasts an error and otherwise silently + did nothing — an item that looks available and is not. */} + {clipboard !== null && ( + + + 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" + > + + New file + + { + const name = prompt('Folder name'); + if (name?.trim()) handleCreateDir(name.trim()); + }} + className="cursor-pointer" + > + + New folder + + fileInputRef.current?.click()} className="cursor-pointer"> + + Upload files + + folderInputRef.current?.click()} className="cursor-pointer"> + + Upload folder + + {/* 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" + > + + Clone repository + + + setShowHidden(!showHidden)} className="cursor-pointer"> + {showHidden ? : } + {showHidden ? 'Hide hidden files' : 'Show hidden files'} + + + + + Create Dashboard here + + setShowVideoDownload(true)} className="cursor-pointer"> + + Download video + + setShowDictate(true)} className="cursor-pointer"> + + Dictate + + + ); + return (
)} {searchQuery.trim() ? ( -
- {searching ? ( -
- + // Until now every menu vanished the moment a search was open: this branch had no ContextMenu and + // its rows are not FileItems, so right-click fell through to the browser's own menu. + + +
+ {searching ? ( +
+ +
+ ) : searchResults && searchResults.length > 0 ? ( +
+ {searchResults.map((entry) => { + const isDir = entry.type === 'directory'; + // `DirEntry.path` is optional because a plain listing entry has none — only search + // results carry one. Guarded rather than asserted, so the two shapes stay honest. + const absPath = entry.path; + return ( + + +
handleSearchResultClick(entry)} + onContextMenu={(ev) => ev.stopPropagation()} + > + {isDir ? ( + + ) : ( + + )} +
+ {entry.name} + {entry.path} +
+
+ + {/* Deliberately three items, not the full row menu. A search hit carries an absolute + path from anywhere in the tree, while every handler behind the row menu builds its + path from the folder being browsed — reusing it would act on the wrong file. These + three take the path as given. */} + + handleSearchResultClick(entry)} className="cursor-pointer"> + + Open + + {absPath && ( + handleCopyAbsPath(absPath)} className="cursor-pointer"> + + Copy path + + )} + {absPath && !isDir && ( + handleDownloadPath(absPath)} className="cursor-pointer"> + + Download + + )} + + + ); + })} +
+ ) : searchResults ? ( +
No results found
+ ) : null}
- ) : searchResults && searchResults.length > 0 ? ( -
- {searchResults.map((entry) => { - const isDir = entry.type === 'directory'; - return ( -
handleSearchResultClick(entry)} - > - {isDir ? ( - - ) : ( - - )} -
- {entry.name} - {entry.path} -
-
- ); - })} -
- ) : searchResults ? ( -
No results found
- ) : null} -
+ + {folderMenu} + ) : ( @@ -135,86 +259,7 @@ export const FileViewContainer = ({ fileBrowserManager }: FileViewContainerProps )}
- - - - Copy path - - - - Chat about this - - {/* Gated on the clipboard, as both toolbars already were. Ungated it fell through to the - OS clipboard path, which on an insecure origin toasts an error and otherwise silently - did nothing — an item that looks available and is not. */} - {clipboard !== null && ( - - - 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" - > - - New file - - { - const name = prompt('Folder name'); - if (name?.trim()) handleCreateDir(name.trim()); - }} - className="cursor-pointer" - > - - New folder - - fileInputRef.current?.click()} className="cursor-pointer"> - - Upload files - - folderInputRef.current?.click()} className="cursor-pointer"> - - Upload folder - - {/* 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" - > - - Clone repository - - - setShowHidden(!showHidden)} className="cursor-pointer"> - {showHidden ? : } - {showHidden ? 'Hide hidden files' : 'Show hidden files'} - - - - - Create Dashboard here - - setShowVideoDownload(true)} className="cursor-pointer"> - - Download video - - setShowDictate(true)} className="cursor-pointer"> - - Dictate - - + {folderMenu} )} diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts index 7a6c7596..aacf30ba 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/useFileBrowserApp.ts @@ -90,7 +90,7 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa const [showVideoDownload, setShowVideoDownload] = useState(false); const [showDictate, setShowDictate] = useState(false); const dragCounter = useRef(0); - const { getMatchingTasks, getMatchingTaskGroups } = useTasks(); + const { getMatchingTaskGroups } = useTasks(); const { getMatchingAgentGroups } = useAgents(); const searchTimerRef = useRef | null>(null); const searchInputRef = useRef(null); @@ -430,10 +430,17 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa }); }; + /** + * Download the clicked entry — or the whole selection, when the click landed inside one. + * + * Same rule `handleDelete` and `handleCompress` already use. It was single-entry only, so the menu + * quietly downloaded one of six selected files while the toolbar's button zipped all six: two controls + * for one intent that disagreed. + */ const handleDownload = async (entry: DirEntry) => { - const path = entryPath(entry.name); + const paths = selected.size > 1 && selected.has(entry.name) ? selectedPaths() : [entryPath(entry.name)]; try { - await files.download([path]); + await files.download(paths); } catch { toast.error('Failed to download'); } @@ -468,10 +475,30 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa }; const handleCopyPath = (entry: DirEntry) => { - copyToClipboard(`~${entryPath(entry.name)}`); + const paths = selected.size > 1 && selected.has(entry.name) ? selectedPaths() : [entryPath(entry.name)]; + // Newline-separated, which is what a shell, an editor and a chat box all accept as a list of paths. + copyToClipboard(paths.map((p) => `~${p}`).join('\n')); + toast.success(paths.length > 1 ? `${paths.length} paths copied` : 'Path copied'); + }; + + /** + * The same two, for a SEARCH result — which carries an absolute path rather than a name in the folder + * being browsed. `entryPath` would build the wrong path for a result from another directory, which is + * why search rows could not simply reuse the row menu. + */ + const handleCopyAbsPath = (path: string) => { + copyToClipboard(`~${path}`); toast.success('Path copied'); }; + const handleDownloadPath = async (path: string) => { + try { + await files.download([path]); + } catch { + toast.error('Failed to download'); + } + }; + const handleCopyCurrentPath = () => { copyToClipboard(`~${currentPath}`); toast.success('Path copied'); @@ -813,7 +840,6 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa // Task runner runningTask, setRunningTask, - getMatchingTasks, getMatchingTaskGroups, // Agent runner runningAgent, @@ -841,6 +867,8 @@ export const useFileBrowserApp = (basePath: string, rootOverride?: string, urlPa handleDeleteSelected, handleChat, handleCopyPath, + handleCopyAbsPath, + handleDownloadPath, handleCopyCurrentPath, handleChatHere, handleDownload, diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/useAgents.ts b/src/workspaces/officerdev/src/apps/FileBrowser/useAgents.ts index 4999c0e1..7bf8cb76 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/useAgents.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/useAgents.ts @@ -46,5 +46,5 @@ export const useAgents = () => { agents: items, })); - return { agents, categoryOrder, getMatchingAgents, getMatchingAgentGroups }; + return { agents, categoryOrder, getMatchingAgentGroups }; }; diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/useTasks.ts b/src/workspaces/officerdev/src/apps/FileBrowser/useTasks.ts index 1ef2dae5..785009bf 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/useTasks.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/useTasks.ts @@ -98,5 +98,7 @@ export const useTasks = () => { const getMatchingTaskGroups = (fileName: string, entryType: 'file' | 'directory'): TaskGroup[] => groupTasksByCategory(getMatchingTasks(fileName, entryType), categoryOrder); - return { tasks, categoryOrder, getMatchingTasks, getMatchingTaskGroups }; + // `getMatchingTasks` stays internal: it is the input to the grouped form, and the menu only ever + // renders groups. Exporting it invited a second, ungrouped code path that never arrived. + return { tasks, categoryOrder, getMatchingTaskGroups }; };