Release v0.51.244 — Release HL (stage-q16): workspace OS-import drop + composer drop-zone polish#3509
Conversation
Workspace OS-import drop-to-subfolder (#3424) + composer drop-zone polish. #3424 (@pamnard): drop OS files/folders onto a workspace folder row or breadcrumb to upload into THAT dir (not just currentDir); OS folder drops traversed via webkitGetAsEntry/readEntries preserving nested structure. Composer @path drag (#1097), tree-move (#3402, the hardened version on master), and OS-drop isolation (#3411) all preserved. NOTE: #3424's branch carried the OLD unhardened _handle_file_move — applied FRONTEND ONLY (ui.js/workspace.js/i18n/css/test); master's hardened move backend is untouched. Drop-zone polish (Nathan-reported jank): dragging a workspace file over the composer footer rendered a translucent overlay that let the textarea/chips/icons bleed through and collide with the hint text. Fixed: opaque overlay (stacked input-bg over --bg) + context-aware label — "Drop to insert workspace reference" for a ws-path drag vs "Drop files to attach" for an OS file. + regression test. Verified ALL drag-drop flows live end-to-end in-browser: - OS file -> composer footer = attaches (hint "Drop files to attach") - workspace file -> composer footer = inserts @path (hint "...insert reference") - workspace file -> workspace folder = moves on disk (report.md -> docs/) - OS file -> workspace folder = upload binding + traversal fns present Co-authored-by: pamnard <pamnard@users.noreply.github.com> Codex CORE MUST-FIX absorbed: _bindWorkspaceOsUploadDropTarget assigned el.ondrop (+ ondragenter/over/leave), OVERWRITING _bindWorkspaceMoveDropTarget's handlers on the same folder-row/breadcrumb element → workspace-file drag-to-move silently broke (fell through to composer @path insert). Fixed: OS-upload binder now uses addEventListener so both handlers COMPOSE (each gates on its drag type). Verified live: report.md drag→drop onto docs/ moves on disk AND does not insert into the composer. + regression test test_os_upload_binder_composes_not_overwrites_move_binder.
|
| Filename | Overview |
|---|---|
| static/workspace.js | Adds OS file/folder drop-upload infrastructure: entry traversal helpers, uploadOsDropToWorkspace, and per-element _bindWorkspaceOsUploadDropTarget. Missing try/catch around async directory traversal; _clearWorkspaceOsUploadDragOver is dead code. |
| static/panels.js | Expands the document dragenter handler to set context-aware hint text (#dropHintText) based on drag type; logic is correct and composes cleanly with workspace drop handlers. |
| static/ui.js | Binds _bindWorkspaceOsUploadDropTarget on breadcrumb root/segments and directory folder rows; adds el.dataset.wsType for CSS/closest selector targeting. |
| static/style.css | Makes .drop-hint overlay fully opaque via stacked linear-gradient background and raises z-index to 30; adds .drag-over-upload variant with blue highlight for folder-row OS-drop targets. |
| static/index.html | Adds #dropHintText span inside the drop-hint div so the hint label can be dynamically swapped per drag type. |
| tests/test_issue3402_workspace_os_import.py | Source-contract assertions for OS-import feature: checks presence of functions, data-attributes, CSS classes, and stopPropagation calls. |
| tests/test_issue3424_composer_drop_hint_polish.py | Source-contract regression tests for the opaque overlay and context-aware hint text; validates the binder uses addEventListener. |
Sequence Diagram
sequenceDiagram
participant U as User (drag)
participant Row as Folder Row / Breadcrumb
participant Tree as #fileTree
participant Doc as document
participant WS as workspace.js
participant API as /api/workspace/upload
Note over U,API: OS file drag to folder row
U->>Row: dragenter (Files type)
Row->>Row: _isOsFilesDrag adds drag-over-upload
Row-->>Tree: stopPropagation
U->>Row: drop (Files type)
Row->>WS: uploadOsDropToWorkspace(dataTransfer, destDir)
WS->>WS: _collectOsDropUploads via webkitGetAsEntry
WS->>API: uploadToWorkspace(file, targetDir) x N
API-->>WS: ok or error toast
WS->>WS: loadDir(currentDir)
Row-->>Doc: stopPropagation skips composer
Note over U,API: Workspace file drag to folder row
U->>Row: dragenter (ws-path type)
Row->>Row: _isOsFilesDrag false, skip OS-upload handler
Row->>Row: _bindWorkspaceMoveDropTarget handles move
Note over U,API: OS file drag to composer footer
U->>Doc: dragenter sets hint to Drop files to attach
U->>Doc: drop calls addFiles(files)
Reviews (1): Last reviewed commit: "Release v0.51.244 — Release HL (stage-q1..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async function _readAllDirectoryEntries(reader) { | ||
| const entries = []; | ||
| while (true) { | ||
| const batch = await new Promise((resolve, reject) => { | ||
| reader.readEntries(resolve, reject); | ||
| }); | ||
| if (!batch.length) break; | ||
| entries.push(...batch); | ||
| } | ||
| return entries; | ||
| } | ||
|
|
||
| async function _collectFilesFromEntry(entry, relPrefix) { | ||
| if (entry.isFile) { | ||
| const file = await new Promise((resolve, reject) => { | ||
| entry.file(resolve, reject); | ||
| }); | ||
| return [{ file, relDir: relPrefix || '' }]; | ||
| } | ||
| if (!entry.isDirectory) return []; | ||
| const reader = entry.createReader(); | ||
| const children = await _readAllDirectoryEntries(reader); | ||
| const dirPrefix = `${relPrefix || ''}${entry.name}/`; | ||
| let out = []; | ||
| for (const child of children) { | ||
| out = out.concat(await _collectFilesFromEntry(child, dirPrefix)); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
_readAllDirectoryEntries and _collectFilesFromEntry both create Promises that can be rejected (the reject callbacks of entry.file() and reader.readEntries()). Neither function has error handling, and neither _collectOsDropUploads nor uploadOsDropToWorkspace wraps these in a try/catch. When a directory entry is unreadable (e.g., access-denied, network filesystem, or a stale FileEntry), the rejection propagates to the drop handler in _bindWorkspaceOsUploadDropTarget which is also unguarded — resulting in a silent unhandled Promise rejection, no user toast, and loadDir never called so the file tree appears unchanged after a partial upload attempt.
| function _clearWorkspaceOsUploadDragOver() { | ||
| document.querySelectorAll('.file-item.drag-over-upload,.breadcrumb-seg.drag-over-upload').forEach((el) => { | ||
| el.classList.remove('drag-over-upload'); | ||
| }); | ||
| } |
There was a problem hiding this comment.
_clearWorkspaceOsUploadDragOver is defined but never called
This cleanup function is declared but has no call site anywhere in the diff or the surrounding code. If it was intended as a safety net (e.g., called from ondragend on draggable file items or the document-level drop handler), the absence of the call means drag-over-upload highlights on folder rows could get stuck in edge cases where element-level dragleave/drop handlers don't fire. Either add the missing call site or remove the function.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Release v0.51.244 — Release HL (stage-q16)
UX-approved direction via Telegram (workspace drag-drop polish you requested). All 4 drag-drop flows verified live in-browser.
Added
webkitGetAsEntry/readEntries) preserving nested structure. Uploads via the existing/api/workspace/upload(no new backend).Fixed
@pathinsert) vs "Drop files to attach" (OS file → message attach).el.ondropon folder rows, which overwrote the drag-to-move handler from feat(workspace): drag-and-drop move within file tree (#3402) #3422 (alsoel.ondrop) — silently breaking move-to-folder (the ws-path drop fell through to the composer as an@pathinsert). Fixed by binding the OS-upload handlers viaaddEventListenerso they compose; each handler gates on its own drag type.Drag-drop matrix — all verified LIVE in-browser (real drag→drop, asserted on disk)
@pathScope note
#3424's PR branch carried the OLD pre-hardening
_handle_file_move. Applied frontend-only — master's hardened move backend (v0.51.243, TOCTOU/symlink fixes) is untouched (Codex confirmed noapi/routes.pydiff).Gate
Co-authored-by: pamnard pamnard@users.noreply.github.com