Drag and drop improvements#903
Conversation
Tree rows in the Files panel are now draggable. Dropping anywhere in the AI chat appends the file's relative path as an @mention in the active AI input. Phase 1 scope: AI mode only (group chat excluded); external file drops continue to stage as images unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…at (phase 2) Dragging files or folders from Finder/Explorer into the chat now appends them as @<path> mentions. Paths inside the session's project root are inserted relative; outside paths are kept absolute. Multi-file drops are joined with spaces. Group chat drops update the active chat's draftMessage. Image drops continue to stage as images unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a secondary line "or drop a file or folder to insert as @reference" beneath the existing "Drop image to attach" message so users discover the new path-mention drop target shipped in phase 2. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…g and drop (phase 3)
📝 WalkthroughWalkthroughDrag-and-drop was generalized from image-only to file/folder handling: internal file-panel drags publish file-paths, external drops and internal drops insert Changes
Sequence DiagramsequenceDiagram
participant User
participant FileExplorerPanel
participant DragEvent
participant AppOverlay
participant useInputHandlers
participant Filesystem
participant ChatDraft
User->>FileExplorerPanel: Drag file (internal)
FileExplorerPanel->>DragEvent: setData('application/x-maestro-file-path', path)
DragEvent->>AppOverlay: dragenter / dragover
AppOverlay->>useInputHandlers: handleFileDragEnter
User->>DragEvent: Drop onto input area
DragEvent->>useInputHandlers: handleDrop(event)
alt Internal file-panel drop
useInputHandlers->>useInputHandlers: getData('application/x-maestro-file-path')
alt Image extension
useInputHandlers->>Filesystem: readFile(path)
Filesystem-->>useInputHandlers: dataURL or null
useInputHandlers->>ChatDraft: stage image if dataURL
else Non-image
useInputHandlers->>useInputHandlers: toMentionPath(path)
useInputHandlers->>ChatDraft: append `@mention` to draft/input
end
else External OS drop
useInputHandlers->>useInputHandlers: extract file.path from DataTransfer
useInputHandlers->>useInputHandlers: convert to project-relative or normalized absolute
useInputHandlers->>ChatDraft: append mention(s) or stage images
end
useInputHandlers-->>User: update inputValue or draftMessage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extends the drag-and-drop surface in three ways: file-tree rows in the Files panel are now draggable and carry a custom
Confidence Score: 3/5Mergeable after fixing the unhandled promise rejection in the image IPC path. One P1 (missing .catch() on readFile) creates a real unhandled rejection surface in SSH/remote sessions where IPC calls are more likely to fail. Two P2s (overlay not shown for internal drags, misleading overlay text) reduce UX quality but don't break functionality. The new logic is otherwise well-structured and well-tested. src/renderer/hooks/input/useInputHandlers.ts — the readFile IPC call needs a .catch() handler. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User drags item] --> B{Internal file panel drag?}
B -- Yes: application/x-maestro-file-path set --> C{Group chat active or not AI mode?}
C -- Yes --> D[Return early — no action]
C -- No --> E{Image extension?}
E -- Yes --> F[readFile via IPC]
F --> G{Returns data: URL?}
G -- Yes --> H[Stage as image attachment]
G -- No --> I[Silent no-op]
E -- No --> J[appendMentionsToAiInput]
B -- No: external filesystem drop --> K{Any files in dataTransfer.files?}
K -- Yes --> L{file.type starts with image/?}
L -- Yes --> M[FileReader → base64 data URL]
M --> N{Group chat?}
N -- Yes --> O[setGroupChatStagedImages]
N -- No --> P[setStagedImages]
L -- No --> Q{file.path present?}
Q -- Yes --> R[toMentionPath: relativize or keep absolute]
R --> S{Group chat active?}
S -- Yes --> T[appendMentionsToGroupChatDraft]
S -- No --> U[appendMentionsToAiInput]
Q -- No --> V[Skip — browser-only file, no fs path]
Reviews (1): Last reviewed commit: "feat: drag images from Files panel into ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| void window.maestro.fs.readFile(absolutePath, sshRemoteId).then((content) => { | ||
| if (typeof content !== 'string' || !content.startsWith('data:image/')) return; | ||
| setStagedImages((prev) => { | ||
| if (prev.includes(content)) { | ||
| setSuccessFlashNotification('Duplicate image ignored'); | ||
| setTimeout(() => setSuccessFlashNotification(null), 2000); | ||
| return prev; | ||
| } | ||
| return [...prev, content]; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
readFile failure
void window.maestro.fs.readFile(...) fires the IPC call with no .catch(). If the call rejects (file not found, permission denied, SSH connection dropped), the rejection is silently swallowed and produces an unhandled promise rejection — no user feedback and potentially noisy DevTools/Sentry logs. A minimal fix is to add an error handler:
void window.maestro.fs.readFile(absolutePath, sshRemoteId)
.then((content) => {
if (typeof content !== 'string' || !content.startsWith('data:image/')) return;
setStagedImages((prev) => {
if (prev.includes(content)) {
setSuccessFlashNotification('Duplicate image ignored');
setTimeout(() => setSuccessFlashNotification(null), 2000);
return prev;
}
return [...prev, content];
});
})
.catch(() => {
// Optionally surface a notification to the user
});
| {/* External File Drop Overlay */} | ||
| {isDraggingImage && ( | ||
| <div | ||
| className="fixed inset-0 z-[9999] pointer-events-none flex items-center justify-center" | ||
| style={{ backgroundColor: `${theme.colors.accent}20` }} | ||
| > | ||
| <div | ||
| className="pointer-events-none rounded-xl border-2 border-dashed p-8 flex flex-col items-center gap-4" | ||
| className="pointer-events-none rounded-xl border-2 border-dashed p-8 flex flex-col items-center gap-3" | ||
| style={{ | ||
| borderColor: theme.colors.accent, | ||
| backgroundColor: `${theme.colors.bgMain}ee`, | ||
| }} | ||
| > | ||
| <svg | ||
| className="w-16 h-16" | ||
| style={{ color: theme.colors.accent }} | ||
| fill="none" | ||
| stroke="currentColor" | ||
| viewBox="0 0 24 24" | ||
| > | ||
| <path | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| strokeWidth={2} | ||
| d="M4 16l4.586-4.586a2 2 0 012.828 0L16 16m-2-2l1.586-1.586a2 2 0 012.828 0L20 14m-6-6h.01M6 20h12a2 2 0 002-2V6a2 2 0 00-2-2H6a2 2 0 00-2 2v12a2 2 0 002 2z" | ||
| /> | ||
| </svg> | ||
| <span className="text-lg font-medium" style={{ color: theme.colors.textMain }}> | ||
| Drop image to attach | ||
| </span> | ||
| <span className="text-sm" style={{ color: theme.colors.textDim }}> | ||
| or drop a file or folder to insert as @reference | ||
| </span> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
handleImageDragEnter only sets isDraggingImage = true when e.dataTransfer.types.includes('Files'). When a user drags an item from the internal Files panel, dataTransfer only carries 'application/x-maestro-file-path' (set via setData in FileExplorerPanel), so 'Files' is absent from types and the overlay is never shown. The user gets no visual feedback that they can drop the item onto the chat area. The new hint text "or drop a file or folder to insert as @reference" is therefore invisible during internal drags.
| <span className="text-lg font-medium" style={{ color: theme.colors.textMain }}> | ||
| Drop image to attach | ||
| </span> | ||
| <span className="text-sm" style={{ color: theme.colors.textDim }}> | ||
| or drop a file or folder to insert as @reference | ||
| </span> |
There was a problem hiding this comment.
The overlay's primary heading says "Drop image to attach" even when the user is dragging a non-image file or folder from the filesystem. In that case the file will be inserted as an @reference instead of being staged as an attachment. The two messages are shown side by side with no conditional logic, so a user dropping a README.md will first see "Drop image to attach", which is incorrect. Consider making the heading context-aware or reordering/rephrasing the copy.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/App.tsx (1)
2569-2601:⚠️ Potential issue | 🟡 MinorThis overlay still excludes file-panel drags.
The new copy says users can drop a file or folder here, but
isDraggingImageis still only turned on for drag payloads whosedataTransfer.typescontainFilesinsrc/renderer/hooks/ui/useAppHandlers.ts:164-210. Internal drags fromFileExplorerPanelonly carryapplication/x-maestro-file-path, so this overlay never appears for the new in-app@referenceflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/App.tsx` around lines 2569 - 2601, The overlay is gated by isDraggingImage which is only set when dataTransfer.types contains native Files, so internal drags from FileExplorerPanel (which use application/x-maestro-file-path) never trigger it; update the drag-detection logic in useAppHandlers (the handler that sets isDraggingImage) to also treat dataTransfer.types containing "application/x-maestro-file-path" (or the internal drag MIME used by FileExplorerPanel) as a file/folder drag and set isDraggingImage accordingly, or rename/expand that state to isDraggingFile and include both native "Files" and the internal "application/x-maestro-file-path" type so the overlay appears for the `@reference` in-app flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/input/useInputHandlers.ts`:
- Around line 627-644: The code resolves the drag image path against
activeSession.projectRoot first, but internalPath is relative to the explorer
root (session.fullPath), causing wrong absolute paths; change the root selection
to use activeSession.fullPath before activeSession.projectRoot (i.e. set
treeRoot = activeSession?.fullPath ?? activeSession?.projectRoot) so the
absolutePath built for window.maestro.fs.readFile(absolutePath, sshRemoteId)
matches FileExplorerPanel's resolution for internalPath and the thumbnail attach
flow works correctly.
---
Outside diff comments:
In `@src/renderer/App.tsx`:
- Around line 2569-2601: The overlay is gated by isDraggingImage which is only
set when dataTransfer.types contains native Files, so internal drags from
FileExplorerPanel (which use application/x-maestro-file-path) never trigger it;
update the drag-detection logic in useAppHandlers (the handler that sets
isDraggingImage) to also treat dataTransfer.types containing
"application/x-maestro-file-path" (or the internal drag MIME used by
FileExplorerPanel) as a file/folder drag and set isDraggingImage accordingly, or
rename/expand that state to isDraggingFile and include both native "Files" and
the internal "application/x-maestro-file-path" type so the overlay appears for
the `@reference` in-app flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73ab7060-3623-4d0a-b828-de24c3a38a57
📒 Files selected for processing (4)
src/__tests__/renderer/hooks/useInputHandlers.test.tssrc/renderer/App.tsxsrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/hooks/input/useInputHandlers.ts
Folds in code-review feedback on the Files-panel drag-drop work and fixes a handful of stuck-overlay edge cases. Code-review feedback (P1/P2 + inline notes): * `toMentionPath` no longer duplicates the canonical IMAGE_EXTENSIONS; imports the existing Set from `utils/fileExplorerIcons/shared`. * Tree-root resolution for the image staging path uses `fullPath ?? projectRoot` to match `FileExplorerPanel.tsx`'s own absolute-path construction (the two diverge in worktree/custom-root sessions). * `fs.readFile` rejection now surfaces a flash notification instead of producing a silent unhandled promise rejection. * The drop overlay now activates for both OS-level `Files` drags and internal Files-panel drags carrying `application/x-maestro-file-path`. * Overlay copy rewritten to be accurate for non-image drops: "Drop file or folder / Images attach as thumbnails. Anything else becomes an @reference." * Renamed `isDraggingImage` -> `isDraggingFile` and `handleImageDrag{Enter,Leave,Over}` -> `handleFileDrag{Enter,Leave,Over}` across the app/hooks/tests so the names match the broader trigger. Overlay UX fixes: * Suppressed dragenter/leave bubbling for internal drags inside the Files panel so the overlay no longer flashes on row-to-row movement. * Stuck-overlay fallbacks: ESC keydown listener on document, document-level `dragleave` window-exit detection (relatedTarget=null or viewport-edge coords), and a click-to-dismiss escape hatch on the overlay itself. Tests: 71/71 in useInputHandlers, 145/145 in FileExplorerPanel. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/renderer/hooks/ui/useAppHandlers.ts (1)
221-260: Solid stuck-overlay fallbacks.The Escape gate on
dragCounterRef.current > 0correctly avoids interfering with non-drag Escape handling, and the cleanup symmetrically removes both new listeners.One small consideration on
handleDocumentDragLeave:relatedTarget === nullcan occasionally fire mid-drag when the cursor crosses shadow-DOM or iframe boundaries (e.g., embedded webviews used elsewhere in the app), which would prematurely reset the overlay. The viewport-edge coordinate check is the more reliable signal here. If you observe overlay flicker during normal drags over webview content, consider tightening this to require bothrelatedTarget === nulland a viewport-edge coordinate. Not a blocker — current logic should be fine for the common OS-drag-cancel case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/ui/useAppHandlers.ts` around lines 221 - 260, handleDocumentDragLeave can misfire when relatedTarget === null for shadow DOM/iframe crossings; tighten the left-window detection in handleDocumentDragLeave by requiring both relatedTarget === null AND coordinates at/past the viewport edge (use e.clientX/e.clientY vs window.innerWidth/innerHeight) before calling handleDragEnd, leaving the existing coordinate checks intact for normal edge-detection; update the leftWindow boolean logic inside the handleDocumentDragLeave function accordingly.src/__tests__/renderer/hooks/useInputHandlers.test.ts (1)
1618-2010: Comprehensive coverage — LGTM.The new
handleDroptest cases cover the important scenarios: internal Files-panel drags (image vs non-image), group-chat exclusion, external drops with relative/absolute paths, multi-file joins, group-chatdraftMessageupdates, missingpath, non-data-url IPC responses, and Windows backslash normalization.One optional suggestion: every test fixture sets
projectRoot === fullPath === '/test', so the path-resolution asymmetry in the implementation (internal drag usesfullPath ?? projectRoot, external drop usesprojectRoot ?? fullPath) isn't exercised. A worktree-style fixture (e.g.projectRoot: '/repo',fullPath: '/repo/.worktrees/feat') for both internal and external drops would lock in the intended behavior and surface any future divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useInputHandlers.test.ts` around lines 1618 - 2010, Tests never exercise the asymmetry between using fullPath vs projectRoot in handleDrop path-resolution; add at least one test fixture where createMockSession returns differing projectRoot and fullPath (e.g., projectRoot: '/repo', fullPath: '/repo/.worktrees/feat') and reuse it for both an internal Files-panel drag (application/x-maestro-file-path) and an external file drop so you assert the expected relative path behavior in both branches; update the relevant tests that call useSessionStore.setState and renderHook/useInputHandlers to use that worktree-style session so the implementation’s fullPath ?? projectRoot vs projectRoot ?? fullPath logic is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/input/useInputHandlers.ts`:
- Around line 638-654: The catch currently swallows errors from
window.maestro.fs.readFile; update the promise chain to bind the error parameter
in the .catch and report it to Sentry using the captureException utility from
src/utils/sentry.ts (include contextual extras like absolutePath and
sshRemoteId), and optionally also log to console.error for local dev, while
preserving the existing user-facing flash notifications in useInputHandlers.ts
where the readFile call happens.
---
Nitpick comments:
In `@src/__tests__/renderer/hooks/useInputHandlers.test.ts`:
- Around line 1618-2010: Tests never exercise the asymmetry between using
fullPath vs projectRoot in handleDrop path-resolution; add at least one test
fixture where createMockSession returns differing projectRoot and fullPath
(e.g., projectRoot: '/repo', fullPath: '/repo/.worktrees/feat') and reuse it for
both an internal Files-panel drag (application/x-maestro-file-path) and an
external file drop so you assert the expected relative path behavior in both
branches; update the relevant tests that call useSessionStore.setState and
renderHook/useInputHandlers to use that worktree-style session so the
implementation’s fullPath ?? projectRoot vs projectRoot ?? fullPath logic is
validated.
In `@src/renderer/hooks/ui/useAppHandlers.ts`:
- Around line 221-260: handleDocumentDragLeave can misfire when relatedTarget
=== null for shadow DOM/iframe crossings; tighten the left-window detection in
handleDocumentDragLeave by requiring both relatedTarget === null AND coordinates
at/past the viewport edge (use e.clientX/e.clientY vs
window.innerWidth/innerHeight) before calling handleDragEnd, leaving the
existing coordinate checks intact for normal edge-detection; update the
leftWindow boolean logic inside the handleDocumentDragLeave function
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a3900e7-2640-4717-b96d-fafb671f3429
📒 Files selected for processing (5)
src/__tests__/renderer/hooks/useInputHandlers.test.tssrc/renderer/App.tsxsrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/hooks/input/useInputHandlers.tssrc/renderer/hooks/ui/useAppHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/components/FileExplorerPanel.tsx
| void window.maestro.fs | ||
| .readFile(absolutePath, sshRemoteId) | ||
| .then((content) => { | ||
| if (typeof content !== 'string' || !content.startsWith('data:image/')) return; | ||
| setStagedImages((prev) => { | ||
| if (prev.includes(content)) { | ||
| setSuccessFlashNotification('Duplicate image ignored'); | ||
| setTimeout(() => setSuccessFlashNotification(null), 2000); | ||
| return prev; | ||
| } | ||
| return [...prev, content]; | ||
| }); | ||
| }) | ||
| .catch(() => { | ||
| setSuccessFlashNotification('Could not read image file'); | ||
| setTimeout(() => setSuccessFlashNotification(null), 2000); | ||
| }); |
There was a problem hiding this comment.
Report fs.readFile failures via captureException.
The catch block discards the error entirely — no captureException, no console.error, not even the err parameter is bound. As per coding guidelines: "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production." and "Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
The user-facing flash is fine, but observability needs the error too — especially for SSH remote read failures, which would otherwise be invisible to monitoring.
🛡️ Proposed fix
+import { captureException } from '../../utils/sentry';
@@
.then((content) => {
if (typeof content !== 'string' || !content.startsWith('data:image/')) return;
setStagedImages((prev) => {
if (prev.includes(content)) {
setSuccessFlashNotification('Duplicate image ignored');
setTimeout(() => setSuccessFlashNotification(null), 2000);
return prev;
}
return [...prev, content];
});
})
- .catch(() => {
+ .catch((err) => {
+ captureException(err instanceof Error ? err : new Error(String(err)), {
+ extra: { context: 'handleDrop.readImage', absolutePath, sshRemoteId },
+ });
setSuccessFlashNotification('Could not read image file');
setTimeout(() => setSuccessFlashNotification(null), 2000);
});As per coding guideline src/**/*.{ts,tsx}: "Do not silently swallow errors... Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void window.maestro.fs | |
| .readFile(absolutePath, sshRemoteId) | |
| .then((content) => { | |
| if (typeof content !== 'string' || !content.startsWith('data:image/')) return; | |
| setStagedImages((prev) => { | |
| if (prev.includes(content)) { | |
| setSuccessFlashNotification('Duplicate image ignored'); | |
| setTimeout(() => setSuccessFlashNotification(null), 2000); | |
| return prev; | |
| } | |
| return [...prev, content]; | |
| }); | |
| }) | |
| .catch(() => { | |
| setSuccessFlashNotification('Could not read image file'); | |
| setTimeout(() => setSuccessFlashNotification(null), 2000); | |
| }); | |
| void window.maestro.fs | |
| .readFile(absolutePath, sshRemoteId) | |
| .then((content) => { | |
| if (typeof content !== 'string' || !content.startsWith('data:image/')) return; | |
| setStagedImages((prev) => { | |
| if (prev.includes(content)) { | |
| setSuccessFlashNotification('Duplicate image ignored'); | |
| setTimeout(() => setSuccessFlashNotification(null), 2000); | |
| return prev; | |
| } | |
| return [...prev, content]; | |
| }); | |
| }) | |
| .catch((err) => { | |
| captureException(err instanceof Error ? err : new Error(String(err)), { | |
| extra: { context: 'handleDrop.readImage', absolutePath, sshRemoteId }, | |
| }); | |
| setSuccessFlashNotification('Could not read image file'); | |
| setTimeout(() => setSuccessFlashNotification(null), 2000); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/input/useInputHandlers.ts` around lines 638 - 654, The
catch currently swallows errors from window.maestro.fs.readFile; update the
promise chain to bind the error parameter in the .catch and report it to Sentry
using the captureException utility from src/utils/sentry.ts (include contextual
extras like absolutePath and sshRemoteId), and optionally also log to
console.error for local dev, while preserving the existing user-facing flash
notifications in useInputHandlers.ts where the readFile call happens.
2 participants
I am working on some more drag and drop functionality:
Summary by CodeRabbit
New Features
UI/UX Improvements
Tests