Shell: cross-app drag-drop primitive + Files → Messages wiring#239
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 36 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (69)
📝 WalkthroughWalkthroughThis PR introduces a cross-app drag-and-drop primitive layer with a global drag-state bus, two React hooks for drag sources and drop targets, and integrations in FilesApp (file drag source) and MessagesApp (file drop target). Includes comprehensive test coverage and design documentation. Changes
Sequence DiagramsequenceDiagram
participant FileRow as FilesApp<br/>(FileRow)
participant DragBus as DnD Bus
participant MsgApp as MessagesApp
participant API as API
FileRow->>FileRow: onDragStart triggered
FileRow->>DragBus: useDragSource calls startDrag(file_payload)
DragBus->>DragBus: Sets current payload, clears stale timer
DragBus->>DragBus: Emits change event (30s timeout active)
MsgApp->>DragBus: useDropTarget subscribes via useSyncExternalStore
DragBus->>MsgApp: getCurrent() returns file_payload
MsgApp->>MsgApp: Computes isValidTarget=true (file in accept list)
MsgApp->>MsgApp: Sets isOver=true on dragEnter
MsgApp->>MsgApp: onDrop triggered
MsgApp->>DragBus: Reads current payload again
MsgApp->>API: attachmentFromPath(path) → creates AttachmentRecord
API->>MsgApp: Returns attachment with source metadata
MsgApp->>DragBus: Calls endDrag() to clear bus
DragBus->>DragBus: Clears current, cancels stale timer, emits change
MsgApp->>MsgApp: Updates UI, sets isOver=false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| try { | ||
| const r = onDrop(payload); | ||
| if (r && typeof (r as Promise<void>).then === "function") { | ||
| (r as Promise<void>).catch((err) => console.warn("drop handler failed", err)); |
There was a problem hiding this comment.
WARNING: endDrag() is not called after successful drop. This leaves the payload active in the bus until the 30s stale timer triggers, which may cause unexpected drop targets to become valid after a completed drag operation.
Call endDrag() after the onDrop handler completes successfully.
| const { payload, disabled = false, htmlMirror } = opts; | ||
| return { | ||
| dragHandlers: { | ||
| draggable: !disabled, |
There was a problem hiding this comment.
SUGGESTION: Missing check for existing active drag before starting new drag. If multiple drag sources receive dragStart events concurrently, the last one will overwrite the current payload without clearing the previous stale timer.
Add endDrag() at the beginning of onDragStart to safely reset existing state.
| const Icon = getFileIcon(f.name, f.is_dir); | ||
| const relPath = f.path || (currentPath ? `${currentPath}/${f.name}` : f.name); | ||
|
|
||
| let vfsPath: string | null = null; |
There was a problem hiding this comment.
WARNING: When vfsPath is null, this passes an empty string as path in the drag payload (due to ?? ""). Empty paths will fail validation in drop targets.
The payload should never have an empty path - ensure vfsPath is always valid when dragEnabled is true.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All Changes Verified✅ FilesApp.tsx: FileRow component correctly implements drag source Files Reviewed (12 files)
Reviewed by seed-2-0-pro-260328 · 257,929 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
static/desktop/assets/ImportApp-CB2pqwOR.js (1)
1-1: Drag hover state can flicker with nested children.On Line 1 (dropzone JSX),
onDragLeave={() => x(false)}is used without an enter-counter. With nested elements, moving within the dropzone can trigger false “leave” events and clear hover state prematurely.Consider switching this dropzone to the new shell DnD primitive (
useDropTarget) or adding an enter-counter locally to stabilizeisOver.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/desktop/assets/ImportApp-CB2pqwOR.js` at line 1, The dropzone's hover state uses onDragLeave={() => x(false)} which can flicker because nested children fire leave events; replace this with an enter-count approach (or swap to the shell DnD primitive useDropTarget). Add a ref like dragCounterRef in the ImportApp component, implement onDragEnter to increment the counter and call x(true) when counter>0, implement onDragLeave to decrement and only call x(false) when the counter reaches 0, and keep the existing onDrop handler to reset the counter and call x(false); update the JSX props on the dropzone element (the component rendered as C with props onDragOver/onDragLeave/onDrop) to use the new onDragEnter/onDragLeave logic and ensure the counter is reset in z (onDrop) and any early exits.desktop/src/apps/FilesApp.tsx (1)
279-285: Redundantkeyprop on inner<tr>element.The
key={f.path || f.name}on the<tr>is unnecessary since React only needs the key on the direct child of the.map()call (already provided on<FileRow>at line 1361). Remove to reduce noise.♻️ Suggested fix
return ( <tr - key={f.path || f.name} data-file-row className="border-b border-white/5 hover:bg-shell-surface/50 transition-colors group" {...dragHandlers} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/FilesApp.tsx` around lines 279 - 285, The inner <tr> in FilesApp.tsx has a redundant key prop (key={f.path || f.name}); remove that key from the <tr> element so React uses the key already provided on the parent FileRow component, leaving the dragHandlers, data-file-row attribute and className intact—update the JSX in the render where <tr ...> is created (inside FilesApp / FileRow render) to drop the key prop.docs/superpowers/plans/2026-04-20-shell-cross-app-dnd.md (1)
95-110: Stale timer reset test has subtle timing edge case.At Line 106-109, the test advances 15s after restart, then 20s more (35s total after restart). However, the stale timeout is 30s, so
getCurrent()should be null at 30s, not 35s. The test assertion at Line 108 (expect(getCurrent()).not.toBeNull()) passes because 15s < 30s, and Line 109 (expect(getCurrent()).toBeNull()) passes because 35s > 30s.The logic is correct, but the comment "35s after restart total" could mislead readers into thinking the timeout is 35s. Consider clarifying:
- vi.advanceTimersByTime(20_000); // 35s after restart total + vi.advanceTimersByTime(20_000); // 35s after restart — well past 30s stale timeout🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-shell-cross-app-dnd.md` around lines 95 - 110, Update the misleading inline comment in the "starting a new drag resets the stale timer" test: in the block using startDrag, getCurrent, and vi.advanceTimersByTime, replace the comment "35s after restart total" with a clarifying note like "35s after restart total (exceeds 30s stale timeout)" or otherwise state that the second advance exceeds the 30s timeout so getCurrent() should be null; keep the test logic unchanged.static/desktop/assets/FilesApp-C4Wn3SGu.js (1)
1-1: Hardcoded MIME type may reduce drop target usefulness.The drag payload uses
mime_type:"application/octet-stream"regardless of actual file type. While the sourceFilesApp.tsxsnippet showsmime_type: entry.mime_type ?? guessFromExt(entry.name)in the plan, the bundled code appears to hardcode it. This could limit drop targets that want to filter by specific MIME types (e.g., accepting only images).If the intent is to support MIME-based filtering in the future, consider deriving the MIME type from the file extension using the existing
isextension-to-icon mapping or a dedicated lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/desktop/assets/FilesApp-C4Wn3SGu.js` at line 1, The drag payload currently hardcodes mime_type:"application/octet-stream" (see the us(...) call to ls(...) and ls(...)’s payload), which prevents MIME-based drop filtering; update the payload to derive mime_type from the file entry (use entry.mime_type if present, otherwise compute from the file extension), e.g. add a small helper that maps extensions using the existing is mapping (or implement guessFromExt) and use that value in the payload passed to ls (and/or inside ls when building the payload) instead of the hardcoded "application/octet-stream".static/desktop/assets/MessagesApp-BLfj1ISY.js (1)
1-1: Implement actual retry logic or hide the retry button for attachment upload failures.The retry button on failed attachments misleads users—clicking it only updates the error message to "retry not yet supported — remove and re-add" rather than retrying the upload. Either implement actual retry functionality (re-running
Me()orattachmentFromPath), or remove the "retry" button and show the limitation upfront in the error message instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/desktop/assets/MessagesApp-BLfj1ISY.js` at line 1, The retry button for failed attachments currently only sets an error text instead of re-uploading; implement real retry in the onRetry handler passed to Ts by locating the failed attachment in the G state, set uploading=true and clear error, then re-run the appropriate upload function (Me for disk-sourced attachments, yt for workspace/agent-workspace sources) and update the G entry with the returned record and uploading:false or set error on failure; reference Ts (renders retry button), Me (uploads File), yt (uploads from workspace path), the G state/ R updater where Ts is invoked, and ensure channel id x is forwarded when calling Me.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 242-264: The shell drop handler (useDropTarget → onDrop /
shellFileDropTarget) currently processes dropped files even when the channel is
archived; add an early guard that checks isCurrentArchived (similar to other
attachment entry points) and returns immediately if true or if no
selectedChannel, so no pending attachment is added; ensure the same guard is
added to the native dataTransfer.files handling path so both drop flows skip
resolving paths and calling attachmentFromPath and setPendingAttachments when
isCurrentArchived is true.
In `@desktop/src/shell/dnd/use-drop-target.ts`:
- Around line 30-60: Update the drag handlers to only treat a drag as active
when there's a current payload and its kind is accepted: in onDragEnter and
onDragOver call getCurrent() and verify payload && accept.includes(payload.kind)
(and skip early if disabled) before calling e.preventDefault() or incrementing
enterCounter / setting setIsOver(true); keep onDragLeave and onDrop behavior for
resetting enterCounter/setIsOver but in onDrop also gate calling onDrop(payload)
behind the same payload && accept.includes check (reuse the existing
getCurrent() usage) so unrelated drags no longer trigger hover visuals or
preventDefault().
In `@static/desktop/assets/ImportApp-CB2pqwOR.js`:
- Line 1: The upload loop in async function T() always increments the success
counter and reports "Uploaded ..." even on fetch failures or non-2xx responses;
change T to only increment the success counter when a POST returns ok, track
failures (e.g., failedCount or failedNames) when fetch throws or response.ok is
false, and update the final status message (the o state setter) to report
successes vs failures (e.g., "Uploaded X of Y files" and list failed files or
count). Locate and update async function T, the loop over i, the FormData/
fetch("/api/import/upload") call, the v state updates (progress), and the final
o(...) call to reflect real success/failure counts and accurate progress math.
Ensure catch blocks and non-ok responses both mark failures and still advance
progress UI correctly.
---
Nitpick comments:
In `@desktop/src/apps/FilesApp.tsx`:
- Around line 279-285: The inner <tr> in FilesApp.tsx has a redundant key prop
(key={f.path || f.name}); remove that key from the <tr> element so React uses
the key already provided on the parent FileRow component, leaving the
dragHandlers, data-file-row attribute and className intact—update the JSX in the
render where <tr ...> is created (inside FilesApp / FileRow render) to drop the
key prop.
In `@docs/superpowers/plans/2026-04-20-shell-cross-app-dnd.md`:
- Around line 95-110: Update the misleading inline comment in the "starting a
new drag resets the stale timer" test: in the block using startDrag, getCurrent,
and vi.advanceTimersByTime, replace the comment "35s after restart total" with a
clarifying note like "35s after restart total (exceeds 30s stale timeout)" or
otherwise state that the second advance exceeds the 30s timeout so getCurrent()
should be null; keep the test logic unchanged.
In `@static/desktop/assets/FilesApp-C4Wn3SGu.js`:
- Line 1: The drag payload currently hardcodes
mime_type:"application/octet-stream" (see the us(...) call to ls(...) and
ls(...)’s payload), which prevents MIME-based drop filtering; update the payload
to derive mime_type from the file entry (use entry.mime_type if present,
otherwise compute from the file extension), e.g. add a small helper that maps
extensions using the existing is mapping (or implement guessFromExt) and use
that value in the payload passed to ls (and/or inside ls when building the
payload) instead of the hardcoded "application/octet-stream".
In `@static/desktop/assets/ImportApp-CB2pqwOR.js`:
- Line 1: The dropzone's hover state uses onDragLeave={() => x(false)} which can
flicker because nested children fire leave events; replace this with an
enter-count approach (or swap to the shell DnD primitive useDropTarget). Add a
ref like dragCounterRef in the ImportApp component, implement onDragEnter to
increment the counter and call x(true) when counter>0, implement onDragLeave to
decrement and only call x(false) when the counter reaches 0, and keep the
existing onDrop handler to reset the counter and call x(false); update the JSX
props on the dropzone element (the component rendered as C with props
onDragOver/onDragLeave/onDrop) to use the new onDragEnter/onDragLeave logic and
ensure the counter is reset in z (onDrop) and any early exits.
In `@static/desktop/assets/MessagesApp-BLfj1ISY.js`:
- Line 1: The retry button for failed attachments currently only sets an error
text instead of re-uploading; implement real retry in the onRetry handler passed
to Ts by locating the failed attachment in the G state, set uploading=true and
clear error, then re-run the appropriate upload function (Me for disk-sourced
attachments, yt for workspace/agent-workspace sources) and update the G entry
with the returned record and uploading:false or set error on failure; reference
Ts (renders retry button), Me (uploads File), yt (uploads from workspace path),
the G state/ R updater where Ts is invoked, and ensure channel id x is forwarded
when calling Me.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4b7c74c-ad33-4138-8c3f-67a6c365967b
📒 Files selected for processing (73)
desktop/src/apps/FilesApp.tsxdesktop/src/apps/MessagesApp.tsxdesktop/src/shell/dnd/__tests__/dnd-bus.test.tsdesktop/src/shell/dnd/__tests__/use-drag-source.test.tsxdesktop/src/shell/dnd/__tests__/use-drop-target.test.tsxdesktop/src/shell/dnd/dnd-bus.tsdesktop/src/shell/dnd/types.tsdesktop/src/shell/dnd/use-drag-source.tsdesktop/src/shell/dnd/use-drop-target.tsdesktop/tsconfig.tsbuildinfodocs/superpowers/plans/2026-04-20-shell-cross-app-dnd.mddocs/superpowers/specs/2026-04-20-shell-cross-app-dnd-design.mdstatic/desktop/assets/ActivityApp-B9wIvJAK.jsstatic/desktop/assets/AgentBrowsersApp-Ch7Kx-O4.jsstatic/desktop/assets/AgentsApp-BZw6W6Vy.jsstatic/desktop/assets/BrowserApp-KbremVMB.jsstatic/desktop/assets/CalendarApp-DZD5-sDA.jsstatic/desktop/assets/ChannelsApp-BveF_Oog.jsstatic/desktop/assets/ClusterApp-oYzyi0Dp.jsstatic/desktop/assets/ContactsApp-DDczp9Hc.jsstatic/desktop/assets/FilesApp-Bm-rxwrE.jsstatic/desktop/assets/FilesApp-C4Wn3SGu.jsstatic/desktop/assets/GitHubApp-B8-CS8Px.jsstatic/desktop/assets/ImageViewerApp-IGHKuKLt.jsstatic/desktop/assets/ImagesApp-DrJsImHG.jsstatic/desktop/assets/ImportApp-CB2pqwOR.jsstatic/desktop/assets/ImportApp-DBAV17Xb.jsstatic/desktop/assets/LibraryApp-BD1Wtjm7.jsstatic/desktop/assets/MCPApp-COa1XWmj.jsstatic/desktop/assets/MemoryApp-Dw5harOM.jsstatic/desktop/assets/MessagesApp-BLfj1ISY.jsstatic/desktop/assets/MessagesApp-BkrxHSyC.jsstatic/desktop/assets/MobileSplitView-B-4MQO2C.jsstatic/desktop/assets/ModelsApp-BGQXnEI3.jsstatic/desktop/assets/ProvidersApp-CLIvGvDZ.jsstatic/desktop/assets/RedditApp-CVpnnpBP.jsstatic/desktop/assets/SecretsApp-Dq-GKn40.jsstatic/desktop/assets/SettingsApp-BSwNHIqf.jsstatic/desktop/assets/StoreApp-B9C6ujYf.jsstatic/desktop/assets/TasksApp-CcqxzxyH.jsstatic/desktop/assets/TextEditorApp-DX1ldFmW.jsstatic/desktop/assets/XApp-CKMiMFvk.jsstatic/desktop/assets/YouTubeApp-CAVU-1ZB.jsstatic/desktop/assets/chat-D-JXvZwU.jsstatic/desktop/assets/chat-D-WzE2QU.jsstatic/desktop/assets/dnd-bus-CdEoCWzR.jsstatic/desktop/assets/index-BEv-dcGk.jsstatic/desktop/assets/index-BlduOVkv.jsstatic/desktop/assets/index-BqnE0gGN.jsstatic/desktop/assets/index-Ce1hN6V-.jsstatic/desktop/assets/index-ClPK3Mg_.jsstatic/desktop/assets/index-CniRZ7Tm.jsstatic/desktop/assets/index-Crp4FTaY.jsstatic/desktop/assets/index-DGMKK4xB.jsstatic/desktop/assets/index-DMMUxG_R.jsstatic/desktop/assets/index-DTZBLne8.jsstatic/desktop/assets/index-DU8P0UT_.jsstatic/desktop/assets/index-DUWvMC11.jsstatic/desktop/assets/index-DuDKTnwy.jsstatic/desktop/assets/index-DwTu_mTz.jsstatic/desktop/assets/index-ESMkhiX0.jsstatic/desktop/assets/index-HCqzTdtf.jsstatic/desktop/assets/index-OlIDYpbO.jsstatic/desktop/assets/main-DJzY8MYL.jsstatic/desktop/assets/main-DncG9bdV.jsstatic/desktop/assets/tokens-BrwHLUZg.cssstatic/desktop/assets/tokens-C1AR9-o7.cssstatic/desktop/assets/tokens-C91S_gMx.jsstatic/desktop/assets/vendor-codemirror-aNcHammg.jsstatic/desktop/assets/vendor-icons--yQaiqiL.jsstatic/desktop/chat.htmlstatic/desktop/index.htmltests/e2e/test_cross_app_dnd.py
💤 Files with no reviewable changes (3)
- static/desktop/assets/ImportApp-DBAV17Xb.js
- static/desktop/assets/FilesApp-Bm-rxwrE.js
- static/desktop/assets/MessagesApp-BkrxHSyC.js
| @@ -0,0 +1 @@ | |||
| import{r as l,j as t}from"./vendor-react-l6srOxy7.js";import{L as U,C,c as k,B as p}from"./toolbar-UW6q5pkx.js";import{ab as f,aj as B,y as M,an as O}from"./vendor-icons--yQaiqiL.js";import"./vendor-radix-BhM7AEEG.js";import"./vendor-layout-B-pp9n1f.js";const g=[".txt",".md",".pdf",".html",".json",".csv"],L=["text/plain","text/markdown","application/pdf","text/html","application/json","text/csv"];function R(c){return c<1024?`${c} B`:c<1024*1024?`${(c/1024).toFixed(1)} KB`:`${(c/(1024*1024)).toFixed(1)} MB`}function Y({windowId:c}){const[S,D]=l.useState([]),[r,E]=l.useState(""),[i,j]=l.useState([]),[A,x]=l.useState(!1),[h,b]=l.useState(!1),[u,v]=l.useState(0),[y,w]=l.useState(!1),[d,o]=l.useState(null),m=l.useRef(null);l.useEffect(()=>{(async()=>{try{const e=await fetch("/api/agents",{headers:{Accept:"application/json"}});if(e.ok&&(e.headers.get("content-type")??"").includes("application/json")){const a=await e.json();Array.isArray(a)&&a.length>0&&D(a.map(n=>String(n.name??"unknown")))}}catch{}})()},[]);const $=l.useCallback(e=>{var a;const s="."+((a=e.name.split(".").pop())==null?void 0:a.toLowerCase());return g.includes(s)||L.includes(e.type)},[]);function N(e){const a=e.filter($).map(n=>({id:`${n.name}-${Date.now()}-${Math.random().toString(36).slice(2,6)}`,file:n,name:n.name,size:n.size}));j(n=>[...n,...a]),o(null)}function z(e){e.preventDefault(),x(!1);const s=Array.from(e.dataTransfer.files);N(s)}function F(e){e.target.files&&N(Array.from(e.target.files)),e.target.value=""}function I(e){j(s=>s.filter(a=>a.id!==e))}async function T(){if(!r||i.length===0)return;b(!0),v(0),o(null);const e=i.length;let s=0;for(const a of i){const n=new FormData;n.append("file",a.file),n.append("agent",r);try{await fetch("/api/import/upload",{method:"POST",body:n})}catch{}s++,v(Math.round(s/e*100))}b(!1),o(`Uploaded ${e} file${e!==1?"s":""} for ${r}`)}async function P(){if(r){w(!0),o(null);try{(await fetch("/api/import/embed",{method:"POST",headers:{"Content-Type":"application/json"},body:JSON.stringify({agent:r})})).ok?o("Embedding complete. Memory updated."):o("Embedding request sent. Check agent memory.")}catch{o("Could not reach embed endpoint. API may not be available.")}w(!1)}}return t.jsxs("div",{className:"flex flex-col h-full bg-shell-bg text-shell-text select-none",children:[t.jsxs("div",{className:"flex items-center gap-2 px-4 py-3 border-b border-white/5",children:[t.jsx(f,{size:18,className:"text-accent"}),t.jsx("h1",{className:"text-sm font-semibold",children:"Import"})]}),t.jsxs("div",{className:"flex-1 overflow-auto p-4 space-y-4",children:[t.jsxs("div",{className:"space-y-1.5",children:[t.jsx(U,{htmlFor:"import-agent",children:"Target Agent"}),t.jsxs("select",{id:"import-agent",value:r,onChange:e=>E(e.target.value),className:"flex h-9 w-full max-w-sm rounded-lg border border-white/10 bg-shell-bg-deep px-3 py-1 text-sm text-shell-text focus-visible:outline-none focus-visible:border-accent/40 focus-visible:ring-2 focus-visible:ring-accent/20",children:[t.jsx("option",{value:"",children:"Select an agent..."}),S.map(e=>t.jsx("option",{value:e,children:e},e))]})]}),t.jsx(C,{onDragOver:e=>{e.preventDefault(),x(!0)},onDragLeave:()=>x(!1),onDrop:z,className:`border-2 border-dashed transition-colors cursor-pointer ${A?"border-accent bg-accent/5":"border-white/10 hover:border-white/20"}`,onClick:()=>{var e;return(e=m.current)==null?void 0:e.click()},role:"button","aria-label":"Drop files here or click to browse",tabIndex:0,onKeyDown:e=>{var s;(e.key==="Enter"||e.key===" ")&&(e.preventDefault(),(s=m.current)==null||s.click())},children:t.jsxs(k,{className:"flex flex-col items-center justify-center gap-3 p-8",children:[t.jsx(f,{size:32,className:"text-shell-text-tertiary"}),t.jsxs("div",{className:"text-center",children:[t.jsx("p",{className:"text-sm text-shell-text-secondary",children:"Drag and drop files here"}),t.jsx("p",{className:"text-xs text-shell-text-tertiary mt-1",children:g.join(", ")})]}),t.jsx(p,{variant:"secondary",size:"sm",onClick:e=>{var s;e.stopPropagation(),(s=m.current)==null||s.click()},children:"Browse"}),t.jsx("input",{ref:m,type:"file",multiple:!0,accept:g.join(","),onChange:F,className:"hidden","aria-label":"Select files to import"})]})}),i.length>0&&t.jsxs("div",{className:"space-y-1.5",children:[t.jsxs("h2",{className:"text-xs text-shell-text-secondary font-medium",children:["Queued Files (",i.length,")"]}),i.map(e=>t.jsx(C,{children:t.jsxs(k,{className:"flex items-center gap-3 px-3.5 py-2.5",children:[t.jsx(B,{size:14,className:"text-shell-text-tertiary shrink-0"}),t.jsx("span",{className:"text-sm flex-1 truncate",children:e.name}),t.jsx("span",{className:"text-xs text-shell-text-tertiary tabular-nums shrink-0",children:R(e.size)}),t.jsx(p,{variant:"ghost",size:"icon",onClick:()=>I(e.id),className:"h-7 w-7 hover:text-red-400 hover:bg-red-500/15","aria-label":`Remove ${e.name}`,children:t.jsx(M,{size:14})})]})},e.id))]}),h&&t.jsxs("div",{className:"space-y-1.5",children:[t.jsxs("div",{className:"flex items-center justify-between text-xs text-shell-text-secondary",children:[t.jsx("span",{children:"Uploading..."}),t.jsxs("span",{className:"tabular-nums",children:[u,"%"]})]}),t.jsx("div",{className:"h-2 w-full rounded-full bg-white/5",role:"progressbar","aria-valuenow":u,"aria-valuemin":0,"aria-valuemax":100,children:t.jsx("div",{className:"h-full rounded-full bg-accent transition-all",style:{width:`${u}%`}})})]}),d&&t.jsx("p",{className:`text-xs ${d.includes("complete")||d.includes("Uploaded")?"text-emerald-400":"text-amber-400"}`,children:d}),t.jsxs("div",{className:"flex gap-2",children:[t.jsxs(p,{onClick:T,disabled:!r||i.length===0||h,children:[t.jsx(f,{size:14}),h?"Uploading...":"Upload"]}),t.jsxs(p,{variant:"secondary",onClick:P,disabled:!r||y,className:"bg-violet-600 text-white hover:bg-violet-500",children:[t.jsx(O,{size:14}),y?"Embedding...":"Embed"]})]})]})]})}export{Y as ImportApp}; | |||
There was a problem hiding this comment.
Upload result reporting is incorrect on partial/total failures.
On Line 1 (async function T()), each iteration increments progress and the final status always says Uploaded ... even when fetch fails or returns non-2xx. This misreports outcome to users.
Proposed fix (apply in source desktop/src/apps/ImportApp.tsx, then rebuild asset)
async function uploadAll() {
if (!selectedAgent || queuedFiles.length === 0) return;
setUploading(true);
setProgress(0);
setStatus(null);
const total = queuedFiles.length;
- let done = 0;
+ let done = 0;
+ let success = 0;
+ const failed: string[] = [];
for (const item of queuedFiles) {
const form = new FormData();
form.append("file", item.file);
form.append("agent", selectedAgent);
try {
- await fetch("/api/import/upload", { method: "POST", body: form });
+ const res = await fetch("/api/import/upload", { method: "POST", body: form });
+ if (res.ok) success++;
+ else failed.push(item.name);
} catch {
+ failed.push(item.name);
}
done++;
setProgress(Math.round((done / total) * 100));
}
setUploading(false);
- setStatus(`Uploaded ${total} file${total !== 1 ? "s" : ""} for ${selectedAgent}`);
+ if (failed.length === 0) {
+ setStatus(`Uploaded ${success} file${success !== 1 ? "s" : ""} for ${selectedAgent}`);
+ } else {
+ setStatus(
+ `Uploaded ${success}/${total} file${total !== 1 ? "s" : ""}. Failed: ${failed.join(", ")}`
+ );
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@static/desktop/assets/ImportApp-CB2pqwOR.js` at line 1, The upload loop in
async function T() always increments the success counter and reports "Uploaded
..." even on fetch failures or non-2xx responses; change T to only increment the
success counter when a POST returns ok, track failures (e.g., failedCount or
failedNames) when fetch throws or response.ok is false, and update the final
status message (the o state setter) to report successes vs failures (e.g.,
"Uploaded X of Y files" and list failed files or count). Locate and update async
function T, the loop over i, the FormData/ fetch("/api/import/upload") call, the
v state updates (progress), and the final o(...) call to reflect real
success/failure counts and accurate progress math. Ensure catch blocks and
non-ok responses both mark failures and still advance progress UI correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
desktop/src/apps/MessagesApp.tsx (2)
242-269:⚠️ Potential issue | 🟠 MajorDisable the shell drop target when the chat is archived.
onDropbails out for archived channels, but the target itself is still active, so archived chats still light up as valid drop zones and intercept the drag. Compute the archived state before this hook and pass it throughdisabledso the hover state and handler gating stay consistent with the rest of the composer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/MessagesApp.tsx` around lines 242 - 269, Compute whether the selected channel is archived before calling useDropTarget (e.g. derive a boolean like isSelectedChannelArchived by finding selectedChannel in allChannels and checking c.settings?.archived) and pass that boolean into useDropTarget via its disabled prop so shellFileDropTarget is disabled for archived chats; keep the existing onDrop guard but ensure the hook receives disabled={isSelectedChannelArchived} to prevent hover/highlight and interception for archived channels.
1340-1351:⚠️ Potential issue | 🟠 MajorNative file drops still bypass the archived-channel guard.
This branch uploads
dataTransfer.fileseven when the selected chat is archived, so users can still end up with unsendable pending attachment chips. Mirror the same early return here before enqueueing uploads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/MessagesApp.tsx` around lines 1340 - 1351, The onDrop handler branch that processes e.dataTransfer.files currently enqueues uploads and pending attachments without checking if the selectedChannel is archived; add the same archived-channel guard used elsewhere (check selectedChannel?.archived or call the existing isArchived utility) at the start of this block and return early before calling setPendingAttachments or uploadDiskFile when the channel is archived, so setPendingAttachments, uploadDiskFile, and the Math.random id generation are skipped for archived channels (refer to the onDrop handler, selectedChannel, setPendingAttachments, and uploadDiskFile symbols to locate the code).
🧹 Nitpick comments (1)
static/desktop/assets/MCPApp-Dp9wLdka.js (1)
1-2: Handle clipboard write failures in the “Copy all logs” action.The current handler assumes
navigator.clipboard.writeText(...)always succeeds; on rejection, users get a silent failure. Wrap this path intry/catchand only show the copied state on success.Proposed patch
-async function j(){await navigator.clipboard.writeText(n.join(` - -`)),u(!0),setTimeout(()=>u(!1),1500)} +async function j(){ + try{ + await navigator.clipboard.writeText(n.join("\n")); + u(!0); + setTimeout(()=>u(!1),1500); + }catch{ + u(!1); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/desktop/assets/MCPApp-Dp9wLdka.js` around lines 1 - 2, The "Copy all logs" handler in Se currently assumes navigator.clipboard.writeText(n.join(`\n`)) always succeeds and sets the copied flag (u) unconditionally; wrap the clipboard write in a try/catch inside the j function in Se, only set the copied state (u(!0)) and start the timeout when writeText resolves, and handle rejection by leaving u false and optionally setting an error state or logging (do not change other UI flags). Locate Se -> function j and update its implementation to await navigator.clipboard.writeText(...) inside try/catch so failures no longer silently show the "copied" state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/FilesApp.tsx`:
- Around line 246-280: The grid view doesn't get the drag source because
useDragSource and its dragHandlers are only used inside FileRow (list view);
update FilesApp so the same drag setup is used for grid cards: extract the
vfsPath/dragEnabled logic and the useDragSource call (payload, disabled,
htmlMirror) from FileRow into a reusable spot (or compute them in the parent
where grid/list rendering happens), then attach the resulting dragHandlers to
the grid card wrapper element (the component rendering file cards in grid view)
the same way FileRow attaches them. Ensure you reuse the same symbols: compute
vfsPath (workspace or agent via agentSlug/isAgentLocation), set dragEnabled =
!!vfsPath && !f.is_dir, call useDragSource with the same payload fields (kind,
path, mime_type, size, name) and pass dragHandlers to the grid card so grid view
supports the identical drag behavior.
---
Duplicate comments:
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 242-269: Compute whether the selected channel is archived before
calling useDropTarget (e.g. derive a boolean like isSelectedChannelArchived by
finding selectedChannel in allChannels and checking c.settings?.archived) and
pass that boolean into useDropTarget via its disabled prop so
shellFileDropTarget is disabled for archived chats; keep the existing onDrop
guard but ensure the hook receives disabled={isSelectedChannelArchived} to
prevent hover/highlight and interception for archived channels.
- Around line 1340-1351: The onDrop handler branch that processes
e.dataTransfer.files currently enqueues uploads and pending attachments without
checking if the selectedChannel is archived; add the same archived-channel guard
used elsewhere (check selectedChannel?.archived or call the existing isArchived
utility) at the start of this block and return early before calling
setPendingAttachments or uploadDiskFile when the channel is archived, so
setPendingAttachments, uploadDiskFile, and the Math.random id generation are
skipped for archived channels (refer to the onDrop handler, selectedChannel,
setPendingAttachments, and uploadDiskFile symbols to locate the code).
---
Nitpick comments:
In `@static/desktop/assets/MCPApp-Dp9wLdka.js`:
- Around line 1-2: The "Copy all logs" handler in Se currently assumes
navigator.clipboard.writeText(n.join(`\n`)) always succeeds and sets the copied
flag (u) unconditionally; wrap the clipboard write in a try/catch inside the j
function in Se, only set the copied state (u(!0)) and start the timeout when
writeText resolves, and handle rejection by leaving u false and optionally
setting an error state or logging (do not change other UI flags). Locate Se ->
function j and update its implementation to await
navigator.clipboard.writeText(...) inside try/catch so failures no longer
silently show the "copied" state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6052285d-6028-43d8-ac3e-c57b2cf6d28b
📒 Files selected for processing (14)
desktop/src/apps/FilesApp.tsxdesktop/src/apps/MessagesApp.tsxdesktop/src/shell/dnd/__tests__/use-drop-target.test.tsxdesktop/src/shell/dnd/use-drag-source.tsdesktop/src/shell/dnd/use-drop-target.tsstatic/desktop/assets/FilesApp-DpnvIlme.jsstatic/desktop/assets/MCPApp-Dp9wLdka.jsstatic/desktop/assets/MessagesApp-C-Qp-BQP.jsstatic/desktop/assets/ProvidersApp-f_IA1pBY.jsstatic/desktop/assets/SettingsApp-53U4rH_g.jsstatic/desktop/assets/chat-6DvfRGaD.jsstatic/desktop/assets/main-tvByLW-C.jsstatic/desktop/chat.htmlstatic/desktop/index.html
✅ Files skipped from review due to trivial changes (2)
- static/desktop/assets/ProvidersApp-f_IA1pBY.js
- desktop/src/shell/dnd/tests/use-drop-target.test.tsx
Comment on lines
+246
to
+280
| function FileRow({ | ||
| f, | ||
| location, | ||
| currentPath, | ||
| navigateTo, | ||
| isWritable, | ||
| deleteConfirm, | ||
| handleDelete, | ||
| setDeleteConfirm, | ||
| }: FileRowProps) { | ||
| const Icon = getFileIcon(f.name, f.is_dir); | ||
| const relPath = f.path || (currentPath ? `${currentPath}/${f.name}` : f.name); | ||
|
|
||
| let vfsPath: string | null = null; | ||
| if (location === "workspace") { | ||
| vfsPath = `/workspaces/user/${relPath}`; | ||
| } else if (isAgentLocation(location)) { | ||
| vfsPath = `/workspaces/agent/${agentSlug(location)}/${relPath}`; | ||
| } | ||
|
|
||
| const dragEnabled = !!vfsPath && !f.is_dir; | ||
| const { dragHandlers } = useDragSource({ | ||
| // When dragEnabled is false, `disabled: true` short-circuits the payload | ||
| // before it ever lands on the bus — the empty-string placeholder below | ||
| // is never read. | ||
| payload: { | ||
| kind: "file", | ||
| path: vfsPath ?? "", | ||
| mime_type: "application/octet-stream", | ||
| size: f.size ?? 0, | ||
| name: f.name, | ||
| }, | ||
| disabled: !dragEnabled, | ||
| htmlMirror: dragEnabled && vfsPath ? { "text/plain": vfsPath } : undefined, | ||
| }); |
There was a problem hiding this comment.
Wire the drag source into grid view too.
useDragSource() only gets applied through FileRow, and FileRow is only rendered in list view. Since FilesApp still defaults to grid view, the new Files → Messages flow is unavailable in the default layout until users manually switch views. Reuse the same drag config on the grid cards as well.
Also applies to: 1362-1374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@desktop/src/apps/FilesApp.tsx` around lines 246 - 280, The grid view doesn't
get the drag source because useDragSource and its dragHandlers are only used
inside FileRow (list view); update FilesApp so the same drag setup is used for
grid cards: extract the vfsPath/dragEnabled logic and the useDragSource call
(payload, disabled, htmlMirror) from FileRow into a reusable spot (or compute
them in the parent where grid/list rendering happens), then attach the resulting
dragHandlers to the grid card wrapper element (the component rendering file
cards in grid view) the same way FileRow attaches them. Ensure you reuse the
same symbols: compute vfsPath (workspace or agent via
agentSlug/isAgentLocation), set dragEnabled = !!vfsPath && !f.is_dir, call
useDragSource with the same payload fields (kind, path, mime_type, size, name)
and pass dragHandlers to the grid card so grid view supports the identical drag
behavior.
jaylfc
force-pushed
the
feat/shell-cross-app-dnd
branch
from
40adaac to
0489c00
Compare
- shell/dnd/ — types, bus, useDragSource, useDropTarget (15 tests) - FilesApp: file rows become drag sources with VFS path payload - MessagesApp: message list is a drop target that converts VFS paths into chat attachments via existing attachmentFromPath; blocked in archived channels; gated on isValidTarget so unrelated drags aren't intercepted; OS-level file drops retain existing native path - endDrag() called after successful drop so next target doesn't stay valid - E2E stub: drag file from Files into Messages composer
jaylfc
force-pushed
the
feat/shell-cross-app-dnd
branch
from
0489c00 to
7873947
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands the shell-wide drag-drop primitive (#60): a typed payload bus +
useDragSource/useDropTargethooks. First integration wires FilesApp file rows as drag sources and the MessagesApp message list as a drop target that turns workspace files into chat attachments.Primitive
desktop/src/shell/dnd/types.ts— discriminated union `DragPayload = file | message | knowledge | canvas-block`.desktop/src/shell/dnd/dnd-bus.ts— singleton event-emitter bus with 30s stale-drag safety timeout.desktop/src/shell/dnd/use-drag-source.ts— spreads `draggable` + drag handlers; writes HTML5 `dataTransfer.setData` mirror for OS-level drags.desktop/src/shell/dnd/use-drop-target.ts— subscribes to the bus, exposes `isOver` / `isValidTarget` with accept-filter; enter-counter handles nested children correctly.Wiring
Test plan
Follow-ups (not in this PR)
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests