Fix/dnd - #46
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a conflict-resolution modal and batch decision flow, consolidates file move/copy behavior into an executeFileOperations pipeline, adds local/remote unique-path probing with 100-attempt bailout and SFTP timeout/reconnect hardening, and blocks external OS drops; also extends backend fs_rename to support auto-rename. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FM as FileManager (UI)
participant CModal as ConflictModal (UI)
participant Backend as Tauri Backend
participant FS as FileSystem / SFTP
User->>FM: Paste / Move / Drop files
FM->>Backend: Parallel fs_exists checks for targets
Backend->>FS: Local/remote existence queries
FS-->>Backend: Existence responses
alt Conflicts found
Backend-->>FM: return conflicts list
FM->>CModal: open modal (isBatch if >1)
CModal-->>User: show options (overwrite/rename/skip + applyToAll)
User->>CModal: choose action
CModal-->>FM: onResolve(action, applyToAll)
alt Rename chosen
FM->>Backend: request unique path (local or get_unique_path_remote)
Backend->>FS: probe candidate names (up to 100)
FS-->>Backend: candidate / error
else Overwrite chosen
FM->>Backend: instruct overwrite (rename-to-backup → move/replace → delete-backup)
Backend->>FS: perform rename/move/delete with timeout & retry
else Skip chosen
FM->>FM: mark skipped
end
FM->>Backend: execute resolved operations
else No conflicts
FM->>Backend: execute executionList directly
end
Backend->>FS: perform move/copy/transfer
FS-->>Backend: completion result
Backend-->>FM: success / error
FM->>FM: refresh UI, clear/advance conflicts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/components/FileManager.tsx (2)
348-365: Sequential existence checks could be slow for remote connections.The rename loop makes up to 100 sequential
fs_existsIPC calls. For remote SFTP connections with latency, this could be noticeably slow.This is acceptable for now since most conflicts resolve within a few iterations, but consider batching existence checks or using a server-side unique name generator if performance becomes an issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileManager.tsx` around lines 348 - 365, The current rename loop in FileManager.tsx that builds candidate names using base, counter, ext and calls window.ipcRenderer.invoke('fs_exists', ...) sequentially (the while loop that sets finalTarget) can be slow for high-latency remote connections; replace the sequential checks with a batched strategy: generate a batch of candidate names (e.g., next N counters) and call fs_exists for all candidates in parallel via Promise.all, pick the first non-existent candidate as finalTarget, and repeat batches if needed (or fallback to asking the backend to generate a unique name via a new IPC like 'fs_generate_unique_name' to avoid client-side polling). Update the loop logic around finalTarget, counter, base, and ext to use the batched checks and ensure the existing timeout/100-attempts failure handling remains intact.
113-126: Consider extracting the shared conflict type.The conflict shape is duplicated between
pendingConflictsandcurrentConflict. Extracting it improves maintainability.♻️ Suggested refactor
+type PendingConflict = { + source: string; + target: string; + name: string; + op: 'move' | 'copy'; + sourceConnectionId?: string; +}; + -const [pendingConflicts, setPendingConflicts] = useState<{ - source: string; - target: string; - name: string; - op: 'move' | 'copy'; - sourceConnectionId?: string; -}[]>([]); -const [currentConflict, setCurrentConflict] = useState<{ - source: string; - target: string; - name: string; - op: 'move' | 'copy'; - sourceConnectionId?: string; -} | null>(null); +const [pendingConflicts, setPendingConflicts] = useState<PendingConflict[]>([]); +const [currentConflict, setCurrentConflict] = useState<PendingConflict | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileManager.tsx` around lines 113 - 126, The conflict object shape is duplicated for pendingConflicts and currentConflict; extract a reusable type (e.g., FileConflict or Conflict) and replace the inline types on useState declarations (pendingConflicts, setPendingConflicts, currentConflict, setCurrentConflict) with that shared type to improve maintainability; update any references that used the inline shape to use the new type name and export it if needed for reuse elsewhere.
🤖 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-tauri/src/commands.rs`:
- Around line 963-965: The call to get_unique_path_remote(...) is performed
outside the rename_remote timeout/reconnect guard so auto-rename can hang on a
stale SFTP channel; move or wrap the get_unique_path_remote call into the same
timeout/reconnect flow used by rename_remote (i.e., invoke
state.file_system.get_unique_path_remote(&sftp, &new_path).await inside the same
reconnect/with_timeout block or reuse the same helper that performs the 10s
timeout and sftp reconnect) so the probing uses the identical retry/timeout
semantics as rename_remote.
- Around line 931-952: The auto-rename loop can exhaust the 100-candidate limit
and leave new_path unchanged, risking overwrite; modify the logic around the
loop that uses auto_rename, new_path, counter and the candidate generation so
that if the loop exits due to counter > 100 (no unique candidate found) you
explicitly fail the operation (return Err / propagate an error or Result)
instead of proceeding to fs::rename(), providing a clear error message like "too
many existing files, cannot auto-rename"; ensure the code path that performs
fs::rename() only runs when a unique candidate was found and new_path was
updated.
In `@src/components/file-manager/ConflictModal.tsx`:
- Around line 81-83: The "Keep Both" copy misleads users because it shows
"{fileName} (1)" but the actual auto-rename logic places the counter before the
extension (e.g., "stem (1).ext"); update the displayed example in ConflictModal
to reflect the real pattern by formatting the fileName into stem and extension
and rendering as `${stem} (1)${ext}` (use the same utility or logic used by the
rename codepath or replicate the split logic near the fileName usage) so the
text matches the generated name.
In `@src/components/file-manager/FileGrid.tsx`:
- Around line 458-461: FileListItem’s right-click handler must mirror
FileGridItem by selecting the file before opening the context menu so list
view’s selectedFiles state stays in sync; update the onContextMenu handling in
FileListItem to first call the same selection function (e.g., onSelectFile or
the prop handler that updates selectedFiles / setSelectedFiles for that file)
and only then call onContextMenu(e, file), ensuring you still stopPropagation as
FileGridItem does; this keeps selection logic consistent across FileGridItem and
FileListItem and prevents stale selectedFiles state on right-click.
In `@src/components/FileManager.tsx`:
- Around line 373-395: The current overwrite flow deletes the target via the
fs_delete IPC call before performing the move/copy (see action === 'overwrite',
fs_delete, fs_rename, fs_copy_batch in FileManager.tsx), risking data loss if
the subsequent fs_rename or fs_copy_batch fails; change the sequence to perform
a safe swap: first write/copy the source to a temporary path (e.g., append a
.tmp or unique suffix), verify the operation succeeded, then atomically rename
the temp into the final target (or rename the existing target to a backup and
remove backup only after success) using fs_rename, and only call fs_delete on
the old target after the new file is in place; if your backend supports atomic
replace, use that instead.
- Around line 232-236: The code in FileManager.tsx assumes all items in
executionList share the same op (line using executionList[0].op) which can break
with mixed 'move'/'copy' inputs; change the logic inside the sameConnection
branch to group executionList by item.op, then for each group build the sources
array and call pasteEntries(activeConnectionId, sources, op === 'move' ? 'cut' :
'copy') per group (instead of a single call), ensuring you iterate groups and
preserve existing behavior for homogeneous lists; reference the sameConnection
check, executionList, pasteEntries, and activeConnectionId when making the
change.
---
Nitpick comments:
In `@src/components/FileManager.tsx`:
- Around line 348-365: The current rename loop in FileManager.tsx that builds
candidate names using base, counter, ext and calls
window.ipcRenderer.invoke('fs_exists', ...) sequentially (the while loop that
sets finalTarget) can be slow for high-latency remote connections; replace the
sequential checks with a batched strategy: generate a batch of candidate names
(e.g., next N counters) and call fs_exists for all candidates in parallel via
Promise.all, pick the first non-existent candidate as finalTarget, and repeat
batches if needed (or fallback to asking the backend to generate a unique name
via a new IPC like 'fs_generate_unique_name' to avoid client-side polling).
Update the loop logic around finalTarget, counter, base, and ext to use the
batched checks and ensure the existing timeout/100-attempts failure handling
remains intact.
- Around line 113-126: The conflict object shape is duplicated for
pendingConflicts and currentConflict; extract a reusable type (e.g.,
FileConflict or Conflict) and replace the inline types on useState declarations
(pendingConflicts, setPendingConflicts, currentConflict, setCurrentConflict)
with that shared type to improve maintainability; update any references that
used the inline shape to use the new type name and export it if needed for reuse
elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b86f6d0a-44f4-4d52-9979-ef5b4dea4838
📒 Files selected for processing (8)
CHANGELOG.mdsrc-tauri/src/commands.rssrc-tauri/src/fs.rssrc-tauri/tauri.conf.jsonsrc/components/FileManager.tsxsrc/components/file-manager/ConflictModal.tsxsrc/components/file-manager/FileGrid.tsxsrc/components/layout/Sidebar.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/file-manager/ConflictModal.tsx (1)
6-6: Consider removing unused'cancel'from theConflictActiontype.The
'cancel'action is defined in the type but never passed toonResolve- the cancel button callshandleClose()which invokesonClose()instead. This isn't a bug since the behavior is correct, but the type could be simplified to match actual usage.♻️ Suggested cleanup
-export type ConflictAction = 'overwrite' | 'skip' | 'rename' | 'cancel'; +export type ConflictAction = 'overwrite' | 'skip' | 'rename';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-manager/ConflictModal.tsx` at line 6, The ConflictAction union type includes an unused 'cancel' member; remove 'cancel' from the export type declaration (export type ConflictAction = 'overwrite' | 'skip' | 'rename';) and then update any references if needed—inspect usages of ConflictAction, the onResolve prop, and the handleClose/onClose flow to ensure they still match (onResolve is only called with 'overwrite'|'skip'|'rename' and cancel remains handled via handleClose/onClose).src/components/FileManager.tsx (1)
503-505: Cross-connection transfer errors during conflict resolution are fire-and-forget.When resolving a conflict with
renameaction for cross-connection transfers (lines 503-505), the transfer is started but errors are only caught to callfailTransfer. Unlike the overwrite path which is synchronous (awaitat line 432), this path doesn't wait for transfer completion before counting assuccessCount++at line 508.This means the toast at line 512 may show "Renamed X item(s)" before the transfer actually completes, and the backup cleanup in overwrite cases won't apply here. This is likely intentional for UX (background transfers), but worth noting for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/FileManager.tsx` around lines 503 - 505, The rename branch starts a cross-connection transfer via window.ipcRenderer.invoke(command, args) and only catches errors to call failTransfer(transferId) which makes the code increment successCount (successCount++) and show the "Renamed X item(s)" toast before the transfer completes; change this to mirror the overwrite path by awaiting the ipcRenderer.invoke call (or collecting its Promise and awaiting before incrementing successCount) so failures trigger failTransfer and the backup/cleanup logic runs before counting success, i.e., replace the fire-and-forget invoke with an awaited call (or Promise handling) tied to transferId and only increment successCount and run any post-transfer cleanup after the transfer resolves successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/file-manager/ConflictModal.tsx`:
- Line 6: The ConflictAction union type includes an unused 'cancel' member;
remove 'cancel' from the export type declaration (export type ConflictAction =
'overwrite' | 'skip' | 'rename';) and then update any references if
needed—inspect usages of ConflictAction, the onResolve prop, and the
handleClose/onClose flow to ensure they still match (onResolve is only called
with 'overwrite'|'skip'|'rename' and cancel remains handled via
handleClose/onClose).
In `@src/components/FileManager.tsx`:
- Around line 503-505: The rename branch starts a cross-connection transfer via
window.ipcRenderer.invoke(command, args) and only catches errors to call
failTransfer(transferId) which makes the code increment successCount
(successCount++) and show the "Renamed X item(s)" toast before the transfer
completes; change this to mirror the overwrite path by awaiting the
ipcRenderer.invoke call (or collecting its Promise and awaiting before
incrementing successCount) so failures trigger failTransfer and the
backup/cleanup logic runs before counting success, i.e., replace the
fire-and-forget invoke with an awaited call (or Promise handling) tied to
transferId and only increment successCount and run any post-transfer cleanup
after the transfer resolves successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 446a7b33-dc39-4237-bed4-8054977e0b93
📒 Files selected for processing (5)
CHANGELOG.mdsrc-tauri/src/commands.rssrc/components/FileManager.tsxsrc/components/file-manager/ConflictModal.tsxsrc/components/file-manager/FileGrid.tsx
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- src/components/file-manager/FileGrid.tsx
…and path-aware state updates
…and path-aware state updates
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/file-manager/FileGrid.tsx (1)
221-229:⚠️ Potential issue | 🟠 MajorBlock external drops inside folder targets too.
These handlers call
stopPropagation()before inspecting the payload, so the container-level blocker at Lines 601-614 never runs when a desktop file is dropped on a folder. The current path then falls through toJSON.parse('')and only logs an error instead of surfacing the “external drop disabled” toast.🧩 Suggested guard
+ const types = Array.from(e.dataTransfer.types || []); + const isExternal = types.includes('Files') || types.includes('text/uri-list'); + if (isExternal) { + e.stopPropagation(); + useAppStore.getState().showToast('info', 'External drop here is currently disabled. We are working to bring this feature to Zync soon!'); + return; + } try { const data = JSON.parse(e.dataTransfer.getData('application/json'));Also applies to: 428-436
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-manager/FileGrid.tsx` around lines 221 - 229, The onDrop handlers currently call e.stopPropagation() and preventDefault() before checking whether the drop is an internal app payload, which prevents the container-level external-drop blocker from running; update the onDrop logic in FileGrid.tsx (the onDrop handler around where JSON.parse(e.dataTransfer.getData('application/json')) is used, and the similar onDrop at the 428-436 region) to first inspect e.dataTransfer.types or the presence of 'application/json' and only call e.preventDefault()/e.stopPropagation() after confirming the payload is internal and parseable; if the transfer does not contain the expected internal type, return early (do not stop propagation) and trigger the external-drop behavior (toast) at the container level.src/store/fileSystemSlice.ts (1)
325-333:⚠️ Potential issue | 🟠 MajorHandle dotfiles before applying the
stem + extrename pattern.
/^(.*?)(\.[^.]*)?$/makes.envand.gitignorelook likebase="",ext=".env", so “Keep Both” generates(1).envand the copy stops being hidden. Please split on the last.only when it is not the first character, and reuse the same helper insrc/components/file-manager/ConflictModal.tsxso the UI text stays aligned with the generated name.🧩 Suggested fix
- const match = originalName.match(/^(.*?)(\.[^.]*)?$/); - const base = match ? match[1] : originalName; - const ext = match && match[2] ? match[2] : ''; + const lastDot = originalName.lastIndexOf('.'); + const base = lastDot > 0 ? originalName.slice(0, lastDot) : originalName; + const ext = lastDot > 0 ? originalName.slice(lastDot) : '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/fileSystemSlice.ts` around lines 325 - 333, The current name-splitting logic treats dotfiles like ".env" as having an empty base and a visible ext, producing names like " (1).env"; change the split to treat a leading dot with no other dots as part of the base and only split on the last '.' when that '.' is not the first character. Concretely: replace the regex-based split around originalName (variables originalName, base, ext in fileSystemSlice.ts) with logic that if originalName startsWith('.') and originalName.indexOf('.', 1) === -1 then set base = originalName and ext = ''; otherwise find lastDot = originalName.lastIndexOf('.') and set base = originalName.slice(0, lastDot) and ext = originalName.slice(lastDot). Extract this into a shared helper (e.g. getBaseAndExt or splitName) and use that same helper from src/components/file-manager/ConflictModal.tsx so the UI label and filesystem naming stay consistent.
🤖 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/components/file-manager/FileGrid.tsx`:
- Around line 230-251: The move handler currently allows self-drops and dropping
a folder onto its descendant because it builds target paths using string
concatenation (creating `//name` at root) and only checks equality against
`${currentPath}/${file.name}`; update the logic in both the multi-source branch
(where moves is built) and the single-source branch (the onMove call) to (1)
normalize/clean paths (collapse duplicate slashes, remove trailing slashes
except for root) when constructing targetFolder and source paths, (2) compute
canonical source and target paths and skip any move where source === target
(self-drop) or where source is an ancestor of target (target startsWith
`${source}/`) to reject descendant-drops, and (3) ensure you use the same
normalized path checks before pushing into moves or calling onMove (apply this
fix to the code around the onMove, currentPath, file.name, data.path and
data.paths handling and also to the similar block at lines ~440-458).
- Around line 402-405: The current code sets dragPreview.innerHTML with
file.name (in FileGrid.tsx, symbol dragPreview and variable file.name), which
allows attacker-controlled names to inject DOM; replace the innerHTML usage with
a safe text insertion approach such as setting dragPreview.textContent or
appending a created TextNode for the file name and separately adding the emoji
prefix as plain text so no HTML is parsed. Ensure you remove any innerHTML
assignment and use only textContent/createTextNode (or setAttribute) to build
the preview string so the emoji prefix (📁/📄) and file.name are concatenated
safely without HTML parsing.
In `@src/store/fileSystemSlice.ts`:
- Around line 396-410: For the optimistic update, only remove the actually moved
sources and only add entries that belong to the currently visible directory:
when op === 'cut' build a movedSources set (e.g., movedSources =
sources.filter(s => normalizePath(s) !== normalizePath(destPath)) and use
normalizedMovedSources to filter currentFiles (replace normalizedSources with
normalizedMovedSources in the newFiles filter), and when computing
filteredNewEntries ensure you compare each entry's parent to normCurrentPath
(the visible directory stored with files[connectionId]) rather than to
destinationDirectory so you only append entries that belong in the currently
shown folder.
---
Outside diff comments:
In `@src/components/file-manager/FileGrid.tsx`:
- Around line 221-229: The onDrop handlers currently call e.stopPropagation()
and preventDefault() before checking whether the drop is an internal app
payload, which prevents the container-level external-drop blocker from running;
update the onDrop logic in FileGrid.tsx (the onDrop handler around where
JSON.parse(e.dataTransfer.getData('application/json')) is used, and the similar
onDrop at the 428-436 region) to first inspect e.dataTransfer.types or the
presence of 'application/json' and only call
e.preventDefault()/e.stopPropagation() after confirming the payload is internal
and parseable; if the transfer does not contain the expected internal type,
return early (do not stop propagation) and trigger the external-drop behavior
(toast) at the container level.
In `@src/store/fileSystemSlice.ts`:
- Around line 325-333: The current name-splitting logic treats dotfiles like
".env" as having an empty base and a visible ext, producing names like "
(1).env"; change the split to treat a leading dot with no other dots as part of
the base and only split on the last '.' when that '.' is not the first
character. Concretely: replace the regex-based split around originalName
(variables originalName, base, ext in fileSystemSlice.ts) with logic that if
originalName startsWith('.') and originalName.indexOf('.', 1) === -1 then set
base = originalName and ext = ''; otherwise find lastDot =
originalName.lastIndexOf('.') and set base = originalName.slice(0, lastDot) and
ext = originalName.slice(lastDot). Extract this into a shared helper (e.g.
getBaseAndExt or splitName) and use that same helper from
src/components/file-manager/ConflictModal.tsx so the UI label and filesystem
naming stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07595bc8-ba79-4952-9560-913657ef7654
📒 Files selected for processing (4)
src/components/FileManager.tsxsrc/components/file-manager/ConflictModal.tsxsrc/components/file-manager/FileGrid.tsxsrc/store/fileSystemSlice.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/FileManager.tsx
Comment on lines
+396
to
+410
| // 1. Remove sources if this is a move (cut) | ||
| let newFiles = currentFiles; | ||
| if (op === 'cut') { | ||
| // Normalize paths for reliable comparison | ||
| const normalizedSources = sources.map(s => normalizePath(s)); | ||
| newFiles = newFiles.filter(f => !normalizedSources.includes(normalizePath(f.path))); | ||
| } | ||
|
|
||
| // 2. Only add new entries if they belong in the CURRENT directory | ||
| const filteredNewEntries = newEntries.filter(entry => { | ||
| const entryParent = entry.path.substring(0, entry.path.lastIndexOf('/')) || '/'; | ||
| return normalizePath(entryParent) === normCurrentPath; | ||
| }); | ||
|
|
||
| newFiles = [...newFiles, ...filteredNewEntries]; |
Contributor
There was a problem hiding this comment.
Use the actual moved set and the visible directory for the optimistic update.
Line 400 removes every requested source, even the cut entries skipped earlier because source === destPath, so a same-folder move hides the item locally. Line 407 then appends entries when they belong to destinationDirectory, not the directory currently stored in files[connectionId], so dropping into /parent/child leaks /parent/child/file into the /parent listing.
🧩 Suggested fix
- const normalizedSources = sources.map(s => normalizePath(s));
+ const normalizedSources = pathsToRemoveFromSource.map(s => normalizePath(s));
newFiles = newFiles.filter(f => !normalizedSources.includes(normalizePath(f.path)));
+ const visibleDirectory = normalizePath(state.currentPath[connectionId] || '/');
const filteredNewEntries = newEntries.filter(entry => {
const entryParent = entry.path.substring(0, entry.path.lastIndexOf('/')) || '/';
- return normalizePath(entryParent) === normCurrentPath;
+ return normalizePath(entryParent) === visibleDirectory;
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/fileSystemSlice.ts` around lines 396 - 410, For the optimistic
update, only remove the actually moved sources and only add entries that belong
to the currently visible directory: when op === 'cut' build a movedSources set
(e.g., movedSources = sources.filter(s => normalizePath(s) !==
normalizePath(destPath)) and use normalizedMovedSources to filter currentFiles
(replace normalizedSources with normalizedMovedSources in the newFiles filter),
and when computing filteredNewEntries ensure you compare each entry's parent to
normCurrentPath (the visible directory stored with files[connectionId]) rather
than to destinationDirectory so you only append entries that belong in the
currently shown folder.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/file-manager/FileGrid.tsx (2)
202-210:⚠️ Potential issue | 🟡 MinorAlign the drag affordances with the new external-drop block.
Folder targets still highlight on any drag, and the empty state still says “Drag files here to upload”, but the container drop handler rejects external
Files/text/uri-listdrops. Unsupported OS drags will look accepted until the toast appears.🎯 Possible adjustment
- onDragOver={(e: any) => { - if (!isFolder || !onMove) return; + onDragOver={(e: any) => { + if (!isFolder || !onMove || !e.dataTransfer.types.includes('application/json')) return; e.preventDefault(); e.dataTransfer.dropEffect = 'move'; @@ - onDragOver={(e) => { - if (!isFolder || !onMove) return; + onDragOver={(e) => { + if (!isFolder || !onMove || !e.dataTransfer.types.includes('application/json')) return; e.preventDefault(); e.dataTransfer.dropEffect = 'move'; @@ - <p className="text-sm opacity-50">Drag files here to upload</p> + <p className="text-sm opacity-50">Use the upload action to add files here</p>Also applies to: 432-437, 623-632, 661-670
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-manager/FileGrid.tsx` around lines 202 - 210, The folder drag-over affordance currently activates for any drag; update the onDragOver handlers in FileGrid so they only show the highlight and call e.preventDefault()/set dropEffect when the drag is acceptable: either it's an internal move (isFolder && onMove && the DataTransfer contains your app's internal drag type) or an external file/URI drop (dataTransfer.types includes 'Files' or 'text/uri-list'). Also mirror this logic in the corresponding onDragEnter/onDragLeave and onDrop handlers and update the empty-state copy ("Drag files here to upload") only to show when external file/URI drops are accepted. Identify these handlers by the onDragOver/onDragEnter/onDragLeave/onDrop callbacks in FileGrid (and the empty-state render that emits "Drag files here to upload") and gate their behavior on dataTransfer.types checks and the onMove/internal-drag marker.
101-145:⚠️ Potential issue | 🟠 MajorWrap
FileGridItemandFileListItemwithforwardRefto enablepopLayoutmode.With
AnimatePresence mode="popLayout", immediate custom child components must forward a ref to their root DOM node for Framer Motion to reliably pop exiting items. Currently both components usememo()only, which prevents ref forwarding. Convert them toforwardRef(memo(...))or extract the motion elements to the parent level.Affects:
FileListItem(lines 145–129) andFileGridItem(lines 101–143), used inAnimatePresenceblocks at lines 308 and 342.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-manager/FileGrid.tsx` around lines 101 - 145, FileGridItem and FileListItem are wrapped with memo but do not forward refs, which breaks AnimatePresence mode="popLayout"; update each component to accept and forward a ref by wrapping the memoized component with React.forwardRef (e.g., export default forwardRef(memo(function FileGridItem(props, ref) { ... })) or define the inner functional component then export forwardRef(memo(InnerComponent))), pass that ref to the root motion/div element so Framer Motion can access the DOM node, and ensure prop types include ref if needed; alternatively move the motion root into the parent and keep the components memoized without refs.
🧹 Nitpick comments (1)
src/components/file-manager/FileGrid.tsx (1)
148-262: Extract the shared DnD pipeline before the two views drift again.
FileGridItemandFileListItemduplicate the drag payload, preview DOM, and drop-validation logic almost verbatim. They have already diverged (align-itemsvsitems-center, folder icon handling), so the next DnD fix is likely to land in only one view.Also applies to: 377-486
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-manager/FileGrid.tsx` around lines 148 - 262, FileGridItem and FileListItem duplicate drag-and-drop logic (onDragStart, onDrop, drag payload construction, drag preview DOM creation, and drop validation) which causes divergence; extract a shared DnD helper module and replace the inline code with calls into it: create reusable functions like buildDragData(event args) (returns the dragData object), createDragPreview(draggedFiles, isFolder, fileName) (creates and returns the preview element and handles cleanup), and validateAndBuildMoves(data, currentPath, file, normalizePath) (performs self/descendant checks and returns moves array), and have both components call setCurrentDragSource in a single utility; update FileGridItem and FileListItem to call these helpers from their onDragStart/onDrop handlers and remove duplicated DOM/style logic while keeping existing uses of setCurrentDragSource, onMove, normalizePath, and the drag data shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-28: Add missing footer reference definitions for the new
commit-labels so they render as links: create reference entries for [61dc6e8],
[82c1612], [f28e04a], and [3dc9898] at the bottom of the CHANGELOG.md mapping
each label to its corresponding commit URL (e.g., the repo commit URL for that
hash) so the inline "([61dc6e8]) / ([82c1612]) / ([f28e04a]) / ([3dc9898])"
markers resolve to actual links.
In `@src/components/file-manager/ConflictModal.tsx`:
- Around line 33-36: The handleResolve handler currently calls onResolve(action,
applyToAll) and returns immediately allowing quick repeated clicks to dispatch
duplicate async resolution; add a re-entrancy guard using an isResolving flag
(or wire to existing isResolving state) so handleResolve returns early if
isResolving is true, set isResolving = true before calling onResolve and clear
it after the async resolve finishes (or when currentConflict is cleared), and
ensure the action buttons, cancel button and the applyToAll checkbox are
disabled/readonly when isResolving is true so the user cannot interact while the
conflict resolver is running; reference handleResolve, onResolve, applyToAll,
isResolving and currentConflict in your changes.
---
Outside diff comments:
In `@src/components/file-manager/FileGrid.tsx`:
- Around line 202-210: The folder drag-over affordance currently activates for
any drag; update the onDragOver handlers in FileGrid so they only show the
highlight and call e.preventDefault()/set dropEffect when the drag is
acceptable: either it's an internal move (isFolder && onMove && the DataTransfer
contains your app's internal drag type) or an external file/URI drop
(dataTransfer.types includes 'Files' or 'text/uri-list'). Also mirror this logic
in the corresponding onDragEnter/onDragLeave and onDrop handlers and update the
empty-state copy ("Drag files here to upload") only to show when external
file/URI drops are accepted. Identify these handlers by the
onDragOver/onDragEnter/onDragLeave/onDrop callbacks in FileGrid (and the
empty-state render that emits "Drag files here to upload") and gate their
behavior on dataTransfer.types checks and the onMove/internal-drag marker.
- Around line 101-145: FileGridItem and FileListItem are wrapped with memo but
do not forward refs, which breaks AnimatePresence mode="popLayout"; update each
component to accept and forward a ref by wrapping the memoized component with
React.forwardRef (e.g., export default forwardRef(memo(function
FileGridItem(props, ref) { ... })) or define the inner functional component then
export forwardRef(memo(InnerComponent))), pass that ref to the root motion/div
element so Framer Motion can access the DOM node, and ensure prop types include
ref if needed; alternatively move the motion root into the parent and keep the
components memoized without refs.
---
Nitpick comments:
In `@src/components/file-manager/FileGrid.tsx`:
- Around line 148-262: FileGridItem and FileListItem duplicate drag-and-drop
logic (onDragStart, onDrop, drag payload construction, drag preview DOM
creation, and drop validation) which causes divergence; extract a shared DnD
helper module and replace the inline code with calls into it: create reusable
functions like buildDragData(event args) (returns the dragData object),
createDragPreview(draggedFiles, isFolder, fileName) (creates and returns the
preview element and handles cleanup), and validateAndBuildMoves(data,
currentPath, file, normalizePath) (performs self/descendant checks and returns
moves array), and have both components call setCurrentDragSource in a single
utility; update FileGridItem and FileListItem to call these helpers from their
onDragStart/onDrop handlers and remove duplicated DOM/style logic while keeping
existing uses of setCurrentDragSource, onMove, normalizePath, and the drag data
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd1b705-ad44-4ca3-99fa-acc216c95320
📒 Files selected for processing (4)
CHANGELOG.mdsrc/components/file-manager/ConflictModal.tsxsrc/components/file-manager/FileGrid.tsxsrc/store/fileSystemSlice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/fileSystemSlice.ts
- implemented re-entrancy guard for ConflictModal - refactored FileGrid items with forwardRef for AnimatePresence - extracted shared DnD logic to dragDropUtils.ts - refined drag-over highlight and empty state prompts - fixed missing commit links in CHANGELOG.md - bumped version to 2.7.0 across all platforms
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 by CodeRabbit
New Features
Bug Fixes
Security