feat(terminal): drag-drop file attach with @-mention / raw modes#256
Conversation
Five fix-before-merge findings from the review pass: 1. Stale-closure dedupe in attachWorkspacePaths. The drop listener captured `selectedContextFiles` at first render, so repeated drops of the same path landed duplicate context pills. Move dedupe inside the setSelectedContextFiles functional updater so it reads the freshest state. 2. Inconsistent error surfacing on chat drop. The terminal path uses Notification.warning toasts; the chat path was emitting a chat-message-notice that scrolled out of view. Switch to Notification.warning to match. 3. Missing keyboard focus after chat drop. Users had to click into the prompt textarea before typing. Call promptInputRef.current.focus() after a successful attach. 4. Backend image branch only matched `is_image AND is_upload`. File- browser image drags set `isImage: true` but not `source: 'upload'`, so OpenAI-vision providers received a text breadcrumb without image bytes. Drop the is_upload constraint so the workspace image path reaches the vision payload too. 5. Path-traversal sandbox on workspace-relative context paths. Backend was joining `root_dir + filePath` without normalization; ``..`` and absolute paths leaked through. Resolve via realpath + commonpath and reject anything that escapes root_dir. Regression coverage: test_workspace_image_drag_reaches_vision_provider pins the non-upload image branch; test_path_traversal_outside_workspace and test_absolute_path_outside_workspace pin the sandbox.
|
Small additional change just pushed on this branch: terminal Shift+Enter now sends the ESC+CR "meta-enter" byte sequence to the PTY, so Claude Code's prompt input treats it as a newline instead of submitting. xterm.js sends bare CR for both Enter and Shift+Enter by default, so Claude Code couldn't distinguish them. The new keydown handler in Adjacent to the drag-drop work (lives in the same per-terminal Commit: 3f30a46. |
|
Ripped the Shift+Enter handler back out (commit c105267). JupyterLab already merged the upstream fix in jupyterlab/jupyterlab#18523, shipping in
Keeping ours alongside would have caused dual-fire (our capture-phase listener runs before xterm.js sees the event, so JL's fix would never trigger when this extension is active) and would have diverged on the injected byte. Cleaner to defer. Filed jupyterlab/jupyterlab#18888 before noticing the merged PR; will close after testing |
c105267 to
3b3b3da
Compare
Five fix-before-merge findings from the review pass: 1. Stale-closure dedupe in attachWorkspacePaths. The drop listener captured `selectedContextFiles` at first render, so repeated drops of the same path landed duplicate context pills. Move dedupe inside the setSelectedContextFiles functional updater so it reads the freshest state. 2. Inconsistent error surfacing on chat drop. The terminal path uses Notification.warning toasts; the chat path was emitting a chat-message-notice that scrolled out of view. Switch to Notification.warning to match. 3. Missing keyboard focus after chat drop. Users had to click into the prompt textarea before typing. Call promptInputRef.current.focus() after a successful attach. 4. Backend image branch only matched `is_image AND is_upload`. File- browser image drags set `isImage: true` but not `source: 'upload'`, so OpenAI-vision providers received a text breadcrumb without image bytes. Drop the is_upload constraint so the workspace image path reaches the vision payload too. 5. Path-traversal sandbox on workspace-relative context paths. Backend was joining `root_dir + filePath` without normalization; ``..`` and absolute paths leaked through. Resolve via realpath + commonpath and reject anything that escapes root_dir. Regression coverage: test_workspace_image_drag_reaches_vision_provider pins the non-upload image branch; test_path_traversal_outside_workspace and test_absolute_path_outside_workspace pin the sandbox.
|
hi @pjdoland this is a great feature. some comments:
|
Closes plmbr#248. Drag files from the OS desktop (or the JupyterLab file browser) onto a terminal pane to insert `@<path>` (Claude Code @-mention syntax) or a shell-escaped raw absolute path at the cursor. Per-terminal toolbar toggle switches the default mode; Shift-drop inverts for one drop. Reuses the existing `/notebook-intelligence/upload-file` endpoint so chat-sidebar uploads share the new lifecycle work: - `NBI_UPLOAD_MAX_MB` (default 50): per-file size cap, 413 on exceed. - `NBI_UPLOAD_RETENTION_HOURS` (default 24): lazy sweep of stale subdirs rate-limited to once per 60s so burst uploads don't pay the listdir cost on every request. - `NBI_TERMINAL_DRAG_DROP_POLICY` (force-on / force-off / user-choice): admin lock to suppress the listener for hardened deployments. The frontend handler reads the policy off the raw capabilities object (not the IFeaturePolicies getter) because dragover fires at ~60Hz and the getter rebuilds 11 child objects per call. Upload failures surface via JL Notification toast so screen-reader users see the same signal as sighted users; the chat sidebar's silent `console.error` path was the prior baseline. Pure helpers (formatForMode, invertMode) live in a separate format module so jest can exercise them without pulling Lumino/DOM imports. `shellSingleQuote` moves from utils.ts (which loads tiktoken at import time) to a tiny shell-utils.ts so the same applies to it; utils.ts re-exports for backward compat.
The file browser starts its Lumino Drag with `supportedActions: 'move'`. Our lm-dragover handlers set `dropAction = 'copy'`, which Lumino's validateAction reduces to 'none' since 'copy' isn't in the supported set. With _dropAction === 'none', lm-drop is never dispatched on pointerup — the drag silently does nothing. Mirror the source's proposedAction back as dropAction so we stay inside whatever the source supports. Also extend the chat sidebar with a document-level lm-* listener pair (matching the terminal-drag pattern) so dragging a workspace file from the file browser onto the chat panel attaches it as context via the same code path the @-mention picker uses. Verified end-to-end via Playwright simulation: terminal injects @<path>, chat sidebar grows a context pill.
When dragging a workspace image from the JL file browser onto the chat sidebar, fetch the model's base64 content via ContentsManager and attach it as image context (with the data URL set as the thumbnail source). Workspace path doubles as the backend serverPath since the chat backend already resolves workspace-relative paths. Mirrors the OS-desktop drag-and-drop behavior so users don't see a "Binary files cannot be attached" rejection when dragging a screenshot from the file browser.
Five fix-before-merge findings from the review pass: 1. Stale-closure dedupe in attachWorkspacePaths. The drop listener captured `selectedContextFiles` at first render, so repeated drops of the same path landed duplicate context pills. Move dedupe inside the setSelectedContextFiles functional updater so it reads the freshest state. 2. Inconsistent error surfacing on chat drop. The terminal path uses Notification.warning toasts; the chat path was emitting a chat-message-notice that scrolled out of view. Switch to Notification.warning to match. 3. Missing keyboard focus after chat drop. Users had to click into the prompt textarea before typing. Call promptInputRef.current.focus() after a successful attach. 4. Backend image branch only matched `is_image AND is_upload`. File- browser image drags set `isImage: true` but not `source: 'upload'`, so OpenAI-vision providers received a text breadcrumb without image bytes. Drop the is_upload constraint so the workspace image path reaches the vision payload too. 5. Path-traversal sandbox on workspace-relative context paths. Backend was joining `root_dir + filePath` without normalization; ``..`` and absolute paths leaked through. Resolve via realpath + commonpath and reject anything that escapes root_dir. Regression coverage: test_workspace_image_drag_reaches_vision_provider pins the non-upload image branch; test_path_traversal_outside_workspace and test_absolute_path_outside_workspace pin the sandbox.
xterm.js sends a bare CR for both Enter and Shift+Enter, so Claude Code's prompt input parses both as "submit." Intercept Shift+Enter at the widget level (capture-phase keydown), prevent xterm's default, and write the ESC+CR meta-enter byte sequence to the PTY via session.send. macOS Terminal emits the same bytes for Option+Enter and Claude Code's prompt parses them as a newline. Cross-platform; respects IME composition and ignores Ctrl/Cmd/Alt modifier combos so it doesn't shadow other terminal shortcuts.
…newlines" This reverts commit 3f30a46.
Mehmet reported that pressing Enter immediately after dropping a file from the JupyterLab file-browser into a terminal opened the file in a new tab instead of submitting the terminal command. Two Enter presses were needed. Same class of bug applied to chat-sidebar drops via HTML5 drag, the file dialog, and image paste. Root cause: after a successful drop, focus stays on the file-browser (which still has the file selected). The file-browser's keyboard handler treats Enter on a selected row as "open the selected item," so the first Enter is intercepted before the drop target sees it. Fix: - terminal-drag: call widget.activate() after paste so the MainAreaWidget takes focus and (if it's a background tab in a split) is raised. Skip via isDisposed when the async upload path returns to a closed terminal. - chat-sidebar: focus the prompt textarea after processAndAttachFiles succeeds, matching the existing focus call in the lm-drop path. Tests: - New jest regression test asserts activate() runs after paste() and covers a descendant-of-host drop target (not just the host node). - afterEach disposes test widgets so document-level lm-* listeners registered by setupTerminal don't leak across tests in the file. - Added @jupyterlab/apputils stub and a DragEvent → MouseEvent subclass shim so the lm-drop test can import terminal-drag.ts under jest/jsdom. Six-agent review converged on the focus fix as correct; followups for target-size etc. were deferred as out of scope.
3b3b3da to
306ccfb
Compare
|
Pushed two updates in
The fix calls Same bug class applied to Tests: new jest regression covers the activate-after-paste ordering and a descendant-of-host drop target; Verified locally: drag a file onto a terminal, press Enter, the |
…ball The TestArchiveSizeCap cases were patching notebook_intelligence.skill_github_import._get_github_token, which doesn't exist (the module imports resolve_github_token from notebook_intelligence.util). mock.patch raised AttributeError at context-manager enter, breaking the suite on every fresh clone. Patch the imported reference in the skill_github_import namespace (resolve_github_token) so the mock takes effect inside _fetch_tarball's call site.

Summary
Drag a file from your desktop onto a JupyterLab terminal, and the terminal injects
@<path>(Claude Code's@-mention syntax) at the cursor. The browser's security model doesn't expose OS paths to web pages, so the browser uploads the dropped file to a server-side staging directory first; the server returns the absolute path, and the terminal pastes that. Drags from the JupyterLab file browser work too (no upload, the path is already known).Solution
The chat sidebar already implements file-drop attachment with the exact server plumbing this feature needs:
NBIAPI.uploadFile(file)POSTsmultipart/form-datatoFileUploadHandler, which writes the file to a per-upload tempdir and returns{ serverPath, filename }. This PR adds a smallsrc/terminal-drag.tsmodule that subscribes toITerminalTracker.widgetAdded, attaches capture-phasedragover/drop(and Luminolm-drop) handlers on each terminal, and injects viaterminal.content.paste(...)so the PTY treats the text the same as a real paste (preserving bracketed-paste framing that Claude Code relies on for multi-token@<path>parses).Per-terminal toolbar toggle switches the default mode (
@-mention vs shell-escaped raw path). Holding Shift while dropping inverts mode for one drop.While the server side is shared with chat uploads, this PR also adds three knobs that benefit both surfaces:
NBI_TERMINAL_DRAG_DROP_POLICY(force-on / force-off / user-choice): standard admin policy triad.force-offsuppresses the terminal listener so files dragged onto a terminal fall through to the browser's default behavior. Useful for hardened deployments that don't want files staged through the upload endpoint.NBI_UPLOAD_MAX_MB(default 50): per-file size cap. Requests over the cap return HTTP 413; the frontend surfaces the rejection via a Notification toast so screen-reader users see the same signal as sighted users.NBI_UPLOAD_RETENTION_HOURS(default 24): lazy retention sweep on the staging directory. Rate-limited to once per 60s so a burst of parallel uploads doesn't pay thelistdir + statcost on every request.Two micro-architecture notes:
formatForMode,invertMode) live insrc/terminal-drag-format.tsso jest can exercise them without pulling Lumino/DOM imports. The same logic applies toshellSingleQuote, which moves fromsrc/utils.ts(loads tiktoken at module import) to a tinysrc/shell-utils.ts;utils.tsre-exports for backward compat.isEnabled()reads the policy off the rawNBIAPI.config.capabilities.feature_policies.terminal_drag_dropobject rather than thefeaturePoliciesgetter, becausedragoverfires at ~60 Hz and the getter rebuilds an 11-key object per call.Testing
Automated:
tests/test_file_upload.py: size cap 413 path + accepts under cap + 0 disables cap; retention sweep removes stale subdirs while preserving fresh ones, runs after a successful upload, throttles within the 60s window, handles missing root and non-directory entries.tests/ts/terminal-drag.test.ts:formatForMode(mention vs raw, multi-file space-joined, empty input, shell-escape edge cases),invertMode(Shift-inversion semantics).jlpm tsc --noEmit,jlpm lint:check,jlpm jest(111 passing) all green.Manual verification:
@</tmp/...>is injected.NBI_TERMINAL_DRAG_DROP_POLICY=force-off: confirm drops fall through to the browser default.Risks and follow-ups
ITerminalTrackertoken is cast toToken<unknown>because@jupyterlab/terminal@4.5.7ships its own nested@lumino/coreutils, making the nominal Token class distinct from our top-level copy. Runtime is unaffected (Lumino looks up tokens by reference identity against the host JupyterLab's own registry), but the type-system contortion is real. Cleanest follow-up is ayarn resolutionsblock; out of scope for this PR.src/terminal-drag.ts(IMainAreaWidgetLike,ITerminalTrackerLike, etc.) capture only what the module touches. Same nested-version-skew workaround. Will simplify once the resolutions follow-up lands.max_body_sizeis still the worst-case ceiling. Streamed uploads withset_max_body_sizewould tighten that further; out of scope here.terminal_drag_droppolicy currently maps "user-choice" and "force-on" to the same enabled state because there's no user-facing toggle. If a future Settings panel adds one, swap the literalTruein_build_feature_policies_responsefor the user value and add alocked_keysentry inConfigHandler.Closes #248.