Open
Conversation
Contributor
WalkthroughA new drag-and-drop file extraction helper has been added to support extracting files from multiple data formats including standard FileDrop and COM-based FileGroupDescriptor payloads. The main window's drop handler is updated to utilize this helper for non-FileDrop cases with overwrite confirmation. COM interop calls are refactored to use readonly reference parameters for GUIDs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant DragDropFileExtractor
participant FileSystem
participant ViewModel
User->>MainWindow: Drag files to drop zone
MainWindow->>MainWindow: HandleDrop event triggered
alt FileDrop format detected
MainWindow->>ViewModel: HandleDroppedFiles(paths)
else Other format (FileGroupDescriptor, etc.)
MainWindow->>DragDropFileExtractor: TryExtractFiles(data, targetDir, overwriteCheck)
DragDropFileExtractor->>DragDropFileExtractor: Parse FileGroupDescriptor
DragDropFileExtractor->>DragDropFileExtractor: Query IStream/HGLOBAL content
loop For each file
DragDropFileExtractor->>FileSystem: Check if file exists
alt File exists
DragDropFileExtractor->>MainWindow: overwriteCheck callback
MainWindow->>MainWindow: MessageBox for confirmation
end
DragDropFileExtractor->>FileSystem: Write file to targetDirectory
end
DragDropFileExtractor-->>MainWindow: Return extracted file paths
MainWindow->>ViewModel: SelectFileAsync(firstFile)
ViewModel-->>MainWindow: File selected
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip You can generate walkthrough in a markdown collapsible section to save space.Enable the |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 151-161: The code currently uses GlobalSize(hglobal) and copies
the whole allocation into targetPath and calls Marshal.Copy without validating
GlobalLock; instead, after obtaining ptrData via GlobalLock(hglobal) check that
ptrData is non-zero/null before copying, retrieve the exact payload length from
the FILEDESCRIPTOR's nFileSizeHigh/nFileSizeLow (from the descriptor you already
parsed) and copy exactly that many bytes to the file (not the full GlobalSize);
only if the descriptor sizes are missing/zero fall back to GlobalSize but trim
the write to avoid padding; ensure you still call GlobalUnlock and release any
handles after the copy.
- Around line 136-139: Sanitize the incoming fileName from the drag source
before combining: replace fileName with its basename (use Path.GetFileName on
the value used in DragDropFileExtractor) to strip rooted paths and .. segments,
then compute targetPath with Path.Combine(targetDirectory, basename) and resolve
using Path.GetFullPath; after resolving ensure the resulting full targetPath is
still under the intended targetDirectory full path (compare FullPath prefixes
with a directory separator check) before proceeding to overwriteCheck and file
creation.
- Around line 56-89: The current parser treats both "FileGroupDescriptorW" and
"FileGroupDescriptor" as the same Unicode layout (FILEDESCRIPTOR), causing
misaligned reads and unsafe memory access; update the logic in
DragDropFileExtractor.cs to branch on fgFormat and use distinct structs/sizes
for the two formats (e.g., FILEDESCRIPTORA for "FileGroupDescriptor" and
FILEDESCRIPTORW for "FileGroupDescriptorW"), compute descSize =
Marshal.SizeOf<T>() using the chosen struct, then read count and validate it
(non-negative and bounded by a sane max, e.g., <= 1024) and verify the buffer
length satisfies 4 + count * descSize before iterating; use the correct struct
when calling Marshal.PtrToStructure and extract the file name with the
appropriate ANSI vs Unicode handling.
In `@PhotoLocator/MainWindow.xaml.cs`:
- Around line 322-325: The current guard uses array identity
(droppedEntries.Equals(_draggedFiles)) so self-drops with different array
instances still pass; change the check in the drop handler to compare contents
instead of identity (e.g., use Enumerable.SequenceEqual or a path-aware
comparison) to compare droppedEntries to _draggedFiles before calling
Dispatcher.BeginInvoke(() => _viewModel.HandleDroppedFiles(droppedEntries));
ensure the comparison uses the same comparison semantics as
HandleFileItemMouseMove (case-insensitive path comparison if file paths are
normalized) so a self-drop will be correctly skipped and SelectDropActionWindow
won't reopen.
- Around line 333-348: The callback passed into
DragDropFileExtractor.TryExtractFiles currently throws
OperationCanceledException for MessageBoxResult.Cancel which ends up being
caught by the outer catch (Exception ex) and logged; change the error handling
so user-cancel is not logged as an exception: update the outer try/catch around
TryExtractFiles (the block containing DragDropFileExtractor.TryExtractFiles, the
existingFile callback, and the catch (Exception ex) {
ExceptionHandler.LogException(ex); }) to handle OperationCanceledException
separately (e.g., catch OperationCanceledException and swallow/return) and only
call ExceptionHandler.LogException for other exceptions, so cancel is treated as
a normal user action rather than an error.
🪄 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: eed248e1-2090-4e0e-8258-38e8598fddc0
📒 Files selected for processing (3)
PhotoLocator/Helpers/DragDropFileExtractor.csPhotoLocator/Helpers/ShellContextMenu.csPhotoLocator/MainWindow.xaml.cs
Comment on lines
+136
to
+139
| var fileName = fileNames[index]; | ||
| var targetPath = Path.Combine(targetDirectory, fileName); | ||
| if (File.Exists(targetPath) && !overwriteCheck(targetPath)) | ||
| continue; |
Contributor
There was a problem hiding this comment.
Sanitize virtual filenames before combining paths.
fileName comes from the drag source. Path.Combine(targetDirectory, fileName) will honor rooted paths and .. segments, so a malicious provider can write outside the photo folder. Strip it to a basename and verify the resolved path still stays under targetDirectory.
🔒 Suggested fix
- var fileName = fileNames[index];
- var targetPath = Path.Combine(targetDirectory, fileName);
+ var fileName = Path.GetFileName(fileNames[index]);
+ if (string.IsNullOrWhiteSpace(fileName))
+ continue;
+
+ var targetRoot = Path.GetFullPath(targetDirectory)
+ .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar)
+ + Path.DirectorySeparatorChar;
+ var targetPath = Path.GetFullPath(Path.Combine(targetRoot, fileName));
+ if (!targetPath.StartsWith(targetRoot, StringComparison.OrdinalIgnoreCase))
+ continue;
if (File.Exists(targetPath) && !overwriteCheck(targetPath))
continue;📝 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.
Suggested change
| var fileName = fileNames[index]; | |
| var targetPath = Path.Combine(targetDirectory, fileName); | |
| if (File.Exists(targetPath) && !overwriteCheck(targetPath)) | |
| continue; | |
| var fileName = Path.GetFileName(fileNames[index]); | |
| if (string.IsNullOrWhiteSpace(fileName)) | |
| continue; | |
| var targetRoot = Path.GetFullPath(targetDirectory) | |
| .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) | |
| Path.DirectorySeparatorChar; | |
| var targetPath = Path.GetFullPath(Path.Combine(targetRoot, fileName)); | |
| if (!targetPath.StartsWith(targetRoot, StringComparison.OrdinalIgnoreCase)) | |
| continue; | |
| if (File.Exists(targetPath) && !overwriteCheck(targetPath)) | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 136 - 139,
Sanitize the incoming fileName from the drag source before combining: replace
fileName with its basename (use Path.GetFileName on the value used in
DragDropFileExtractor) to strip rooted paths and .. segments, then compute
targetPath with Path.Combine(targetDirectory, basename) and resolve using
Path.GetFullPath; after resolving ensure the resulting full targetPath is still
under the intended targetDirectory full path (compare FullPath prefixes with a
directory separator check) before proceeding to overwriteCheck and file
creation.
Comment on lines
+322
to
+325
| if (e.Data.GetDataPresent(DataFormats.FileDrop) && e.Data.GetData(DataFormats.FileDrop) is string[] droppedEntries && droppedEntries.Length > 0) | ||
| { | ||
| Dispatcher.BeginInvoke(() => _viewModel.HandleDroppedFiles(droppedEntries)); | ||
| if (!droppedEntries.Equals(_draggedFiles)) | ||
| Dispatcher.BeginInvoke(() => _viewModel.HandleDroppedFiles(droppedEntries)); |
Contributor
There was a problem hiding this comment.
Equals is only checking array identity here.
_draggedFiles is populated from the current selection in HandleFileItemMouseMove (PhotoLocator/MainWindow.xaml.cs:296-298), so this guard is the only thing preventing a self-drop from reopening SelectDropActionWindow (PhotoLocator/MainViewModel.cs:708-712). string[].Equals only succeeds for the same array instance, not the same paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@PhotoLocator/MainWindow.xaml.cs` around lines 322 - 325, The current guard
uses array identity (droppedEntries.Equals(_draggedFiles)) so self-drops with
different array instances still pass; change the check in the drop handler to
compare contents instead of identity (e.g., use Enumerable.SequenceEqual or a
path-aware comparison) to compare droppedEntries to _draggedFiles before calling
Dispatcher.BeginInvoke(() => _viewModel.HandleDroppedFiles(droppedEntries));
ensure the comparison uses the same comparison semantics as
HandleFileItemMouseMove (case-insensitive path comparison if file paths are
normalized) so a self-drop will be correctly skipped and SelectDropActionWindow
won't reopen.
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
Improvements