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:
WalkthroughAdds a DragDropFileExtractor to materialize dropped or virtual files (FileDrop or FileGroupDescriptorW) into a target directory; integrates it into MainWindow drop handling with per-file overwrite prompts and exception reporting; updates ShellContextMenu COM interop signatures and cleans minor formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant DragDropFileExtractor
participant FileSystem
participant ViewModel
User->>MainWindow: Drop data
MainWindow->>MainWindow: HandleDrop()
alt DataFormats.FileDrop present
MainWindow->>ViewModel: HandleDroppedFiles(paths)
else Non-FileDrop payload
MainWindow->>DragDropFileExtractor: TryExtractFiles(data, targetDir, overwriteCheck, progress)
DragDropFileExtractor->>DragDropFileExtractor: Parse FILEDESCRIPTOR / enumerate entries
loop per file
DragDropFileExtractor->>DragDropFileExtractor: GetData(FORMATETC with lindex)
alt returned TYMED_ISTREAM
DragDropFileExtractor->>FileSystem: Copy stream to file
else returned TYMED_HGLOBAL
DragDropFileExtractor->>FileSystem: Lock HGLOBAL and write bytes
end
DragDropFileExtractor->>MainWindow: Call overwriteCheck(path) if exists
MainWindow->>MainWindow: Show MessageBox (Yes/No/Cancel) when prompted
DragDropFileExtractor->>FileSystem: Write file, set last-write time
DragDropFileExtractor->>DragDropFileExtractor: ReleaseStgMedium
DragDropFileExtractor->>MainWindow: Update progress
end
DragDropFileExtractor-->>MainWindow: Return extracted paths
MainWindow->>ViewModel: SelectFileAsync(firstPath)
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 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: 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
PhotoLocator/MainWindow.xaml.cs (1)
348-351:⚠️ Potential issue | 🟡 MinorDon't display user cancel as an exception.
OperationCanceledException(thrown when user clicks Cancel in the overwrite dialog) propagates here and is shown viaExceptionHandler.ShowException. User cancellation is expected flow, not an error.💡 Proposed fix
- catch (Exception ex) + catch (OperationCanceledException) + { + // User cancelled - nothing to do + } + catch (Exception ex) { ExceptionHandler.ShowException(ex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/MainWindow.xaml.cs` around lines 348 - 351, The generic catch block currently passes every exception to ExceptionHandler.ShowException including OperationCanceledException from the overwrite dialog; update the handler so OperationCanceledException is filtered out and not shown to the user — e.g. add a specific check or separate catch for OperationCanceledException and silently ignore or handle it as a user-cancel (no call to ExceptionHandler.ShowException), while preserving the existing call to ExceptionHandler.ShowException for all other exceptions thrown in this block.PhotoLocator/Helpers/DragDropFileExtractor.cs (2)
127-130:⚠️ Potential issue | 🔴 CriticalSanitize virtual filenames before combining paths.
Unlike the FileDrop branch (line 41),
fileInfo.Nameis used directly withoutPath.GetFileName. A malicious drag source can supply rooted paths or..segments, writing files outsidetargetDirectory.🔒 Proposed fix
var fileInfo = fileInfos[i]; - var targetPath = Path.Combine(targetDirectory, fileInfo.Name); + var fileName = Path.GetFileName(fileInfo.Name); + 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 127 - 130, The code uses fileInfo.Name directly when building targetPath which allows rooted paths or “..” segments to escape targetDirectory; replace usage with a sanitized filename (e.g., var safeName = Path.GetFileName(fileInfo.Name)) when computing targetPath and then validate the resolved path (e.g., var resolved = Path.GetFullPath(Path.Combine(targetDirectory, safeName)) and ensure resolved.StartsWith(Path.GetFullPath(targetDirectory), StringComparison.OrdinalIgnoreCase)) before calling overwriteCheck or writing the file to prevent path traversal; update references to fileInfo.Name/targetPath accordingly in the loop where fileInfos is iterated.
156-161:⚠️ Potential issue | 🟠 MajorCheck
GlobalLockreturn value before copying.
GlobalLockreturnsIntPtr.Zeroon failure. CallingMarshal.Copywith a null pointer causes an access violation.🛡️ Proposed fix
var hglobal = medium.unionmember; var ptrData = GlobalLock(hglobal); + if (ptrData == IntPtr.Zero) + continue; try { var buffer = new byte[fileInfo.SizeLow];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 156 - 161, In DragDropFileExtractor.cs, check the return value of GlobalLock (the ptrData local) before calling Marshal.Copy to avoid dereferencing IntPtr.Zero; if GlobalLock returns IntPtr.Zero handle the failure (e.g., log/throw or skip this medium) and ensure you still call GlobalUnlock on medium.unionmember in the finally block as appropriate; specifically update the block around medium.unionmember, ptrData and Marshal.Copy (which uses fileInfo.SizeLow) to validate ptrData != IntPtr.Zero and handle the error path safely.
🧹 Nitpick comments (1)
PhotoLocator/Helpers/DragDropFileExtractor.cs (1)
73-76: Validate thatcountis positive before iterating.
Marshal.ReadInt32on untrusted data could yield a negative value, which would bypass the size check (count * descSize + 4would underflow) and skip the loop harmlessly—but explicitly guarding makes intent clear and catches truly malformed descriptors early.🛡️ Proposed fix
var ptr = handle.AddrOfPinnedObject(); int count = Marshal.ReadInt32(ptr); + if (count <= 0) + return null; var descSize = Marshal.SizeOf<FILEDESCRIPTOR>(); if (count * descSize + 4 > bytes.Length) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 73 - 76, Validate that the descriptor count read by Marshal.ReadInt32 is positive before using it: in DragDropFileExtractor (where you read int count = Marshal.ReadInt32(ptr)), add an explicit guard that returns null if count <= 0 (and optionally if count is unreasonably large), and perform the size overflow-safe check for count * descSize + 4 (e.g., check count <= Int32.MaxValue / descSize) before using descSize/FILEDESCRIPTOR and iterating; this prevents negative or overflowing counts from bypassing the size check and protects the subsequent loop that processes FILEDESCRIPTOR entries.
🤖 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/MainWindow.xaml.cs`:
- Around line 332-346: The overwrite confirmation MessageBox is being shown from
the background task inside the lambda passed to
DragDropFileExtractor.TryExtractFiles (see MainWindow.xaml.cs around the
RunProcessWithProgressBarAsync call and the overwriteCheck callback), which will
throw because it accesses the Window from a non-UI thread; modify the overwrite
callback so it marshals the MessageBox.Show call to the UI thread (use
this.Dispatcher.Invoke/BeginInvoke or Application.Current.Dispatcher) to display
the dialog and return the result to the background work, keeping the rest of the
Task.Run body intact and ensuring OperationCanceledException is still thrown for
cancel responses.
---
Duplicate comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 127-130: The code uses fileInfo.Name directly when building
targetPath which allows rooted paths or “..” segments to escape targetDirectory;
replace usage with a sanitized filename (e.g., var safeName =
Path.GetFileName(fileInfo.Name)) when computing targetPath and then validate the
resolved path (e.g., var resolved =
Path.GetFullPath(Path.Combine(targetDirectory, safeName)) and ensure
resolved.StartsWith(Path.GetFullPath(targetDirectory),
StringComparison.OrdinalIgnoreCase)) before calling overwriteCheck or writing
the file to prevent path traversal; update references to
fileInfo.Name/targetPath accordingly in the loop where fileInfos is iterated.
- Around line 156-161: In DragDropFileExtractor.cs, check the return value of
GlobalLock (the ptrData local) before calling Marshal.Copy to avoid
dereferencing IntPtr.Zero; if GlobalLock returns IntPtr.Zero handle the failure
(e.g., log/throw or skip this medium) and ensure you still call GlobalUnlock on
medium.unionmember in the finally block as appropriate; specifically update the
block around medium.unionmember, ptrData and Marshal.Copy (which uses
fileInfo.SizeLow) to validate ptrData != IntPtr.Zero and handle the error path
safely.
In `@PhotoLocator/MainWindow.xaml.cs`:
- Around line 348-351: The generic catch block currently passes every exception
to ExceptionHandler.ShowException including OperationCanceledException from the
overwrite dialog; update the handler so OperationCanceledException is filtered
out and not shown to the user — e.g. add a specific check or separate catch for
OperationCanceledException and silently ignore or handle it as a user-cancel (no
call to ExceptionHandler.ShowException), while preserving the existing call to
ExceptionHandler.ShowException for all other exceptions thrown in this block.
---
Nitpick comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 73-76: Validate that the descriptor count read by
Marshal.ReadInt32 is positive before using it: in DragDropFileExtractor (where
you read int count = Marshal.ReadInt32(ptr)), add an explicit guard that returns
null if count <= 0 (and optionally if count is unreasonably large), and perform
the size overflow-safe check for count * descSize + 4 (e.g., check count <=
Int32.MaxValue / descSize) before using descSize/FILEDESCRIPTOR and iterating;
this prevents negative or overflowing counts from bypassing the size check and
protects the subsequent loop that processes FILEDESCRIPTOR entries.
🪄 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: f7327848-17cd-4f7a-a094-3940b4a41853
📒 Files selected for processing (2)
PhotoLocator/Helpers/DragDropFileExtractor.csPhotoLocator/MainWindow.xaml.cs
meesoft
commented
Mar 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
PhotoLocator/MainWindow.xaml.cs (2)
322-325:⚠️ Potential issue | 🟡 MinorCompare dropped paths by content, not by array instance.
string[].Equalsonly succeeds for the same array object, so a self-drop that round-trips through a newstring[]still re-enters_viewModel.HandleDroppedFiles.💡 Suggested fix
- if (!dropped.Equals(_draggedFiles)) + if (_draggedFiles is null || !dropped.SequenceEqual(_draggedFiles, StringComparer.OrdinalIgnoreCase)) await Dispatcher.BeginInvoke(() => _viewModel.HandleDroppedFiles(dropped));🤖 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 equality check is using reference equality on string[] (dropped.Equals(_draggedFiles)), so identical path lists from different array instances will be treated as different; change the comparison to compare sequences by content (e.g., use Enumerable.SequenceEqual or compare lengths and elements) when deciding whether to call _viewModel.HandleDroppedFiles(dropped), and update _draggedFiles to the new array only after confirming they differ; reference the local variable dropped, the field _draggedFiles, and the method HandleDroppedFiles to locate the change.
336-348:⚠️ Potential issue | 🟡 MinorDon't surface overwrite-cancel as an error.
MessageBoxResult.Cancelis intentionally mapped toOperationCanceledException, but the outercatch (Exception)still shows it viaExceptionHandler.ShowException.💡 Suggested fix
} + catch (OperationCanceledException) + { + return; + } catch (Exception ex) { ExceptionHandler.ShowException(ex); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/MainWindow.xaml.cs` around lines 336 - 348, The code currently maps MessageBoxResult.Cancel to an OperationCanceledException but the outer catch (Exception ex) always calls ExceptionHandler.ShowException, which surfaces user-cancel as an error; update the error handling around the try/catch that wraps the async copy task so that OperationCanceledException (from the MessageBoxResult.Cancel mapping) is handled separately and not passed to ExceptionHandler.ShowException — either add a specific catch (OperationCanceledException) { /* no-op or user-cancel flow */ } before the generic catch, or change the generic catch to if (!(ex is OperationCanceledException)) ExceptionHandler.ShowException(ex); to ignore cancels.PhotoLocator/Helpers/DragDropFileExtractor.cs (2)
69-80:⚠️ Potential issue | 🟠 MajorValidate the descriptor buffer before reading
cItems.
bytescomes from the drag source.Marshal.ReadInt32(ptr)runs even when the payload is shorter than 4 bytes, andcount * descSize + 4usesintarithmetic, so large or malformed counts can bypass the guard beforePtrToStructurewalks past the pinned buffer.🔒 Suggested fix
- var ptr = handle.AddrOfPinnedObject(); - int count = Marshal.ReadInt32(ptr); var descSize = Marshal.SizeOf<FILEDESCRIPTOR>(); - if (count * descSize + 4 > bytes.Length) + if (bytes.Length < sizeof(int)) + return null; + + var ptr = handle.AddrOfPinnedObject(); + var count = Marshal.ReadInt32(ptr); + if (count <= 0 || (long)count > (bytes.Length - sizeof(int)) / descSize) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 69 - 80, The code reads cItems with Marshal.ReadInt32(ptr) and then uses count * descSize + 4 without validating the input size or preventing integer overflow; first ensure bytes.Length >= 4 before calling Marshal.ReadInt32, then read count and validate it is non-negative and within a sane limit, perform the size check using checked 64-bit arithmetic (e.g. compute required = 4 + (long)count * descSize) and ensure required <= bytes.Length before entering the loop and before calling Marshal.PtrToStructure for FILEDESCRIPTOR; also bail out on suspiciously large counts to avoid OOM/overrun after GCHandle.Alloc and before PtrToStructure.
131-132:⚠️ Potential issue | 🟠 MajorBound
HGLOBALcopies to the actual allocation.
nFileSizeLowis descriptor metadata, not proof of theHGLOBALsize. Without aGlobalLocknull check and aGlobalSizebound,Marshal.Copycan read past the locked block on malformed drag data.🔒 Suggested fix
- if (fileInfo.SizeHigh > 0) + if (fileInfo.SizeHigh > 0 || fileInfo.SizeLow > int.MaxValue) throw new UserMessageException("Huge file import not supported"); ... var hglobal = medium.unionmember; var ptrData = GlobalLock(hglobal); + var globalSize = GlobalSize(hglobal).ToUInt64(); try { - var buffer = new byte[fileInfo.SizeLow]; + if (ptrData == IntPtr.Zero) + throw new UserMessageException("Could not read dragged file contents."); + if ((ulong)fileInfo.SizeLow > globalSize) + throw new UserMessageException("Dragged file contents are truncated."); + + var buffer = new byte[checked((int)fileInfo.SizeLow)]; Marshal.Copy(ptrData, buffer, 0, buffer.Length); File.WriteAllBytes(targetPath, buffer); File.SetLastWriteTime(targetPath, fileInfo.LastWriteTime); saved.Add(targetPath); } finally { - GlobalUnlock(hglobal); + if (ptrData != IntPtr.Zero) + GlobalUnlock(hglobal); } ... + [DllImport("kernel32.dll", SetLastError = true), DefaultDllImportSearchPaths(DllImportSearchPath.System32)] + static extern UIntPtr GlobalSize(IntPtr hMem); + [DllImport("kernel32.dll", SetLastError = true), DefaultDllImportSearchPaths(DllImportSearchPath.System32)] static extern IntPtr GlobalLock(IntPtr hMem);Also applies to: 153-169, 213-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 131 - 132, The code assumes nFileSizeLow proves the HGLOBAL buffer size and directly calls Marshal.Copy; instead, in the DragDropFileExtractor logic (the block checking fileInfo.SizeHigh and reading from an HGLOBAL), add a GlobalLock null-check and use GlobalSize(hGlobal) to get the actual allocation size, then bound the copy length to Math.Min((int)fileInfo.nFileSizeLow, (int)globalSize) and only call Marshal.Copy when GlobalLock succeeded; also preserve the existing UserMessageException for huge files but do not trust nFileSizeLow alone. Apply the same pattern for the other HGLOBAL-read sites referenced (the blocks around 153-169 and 213-221) so every GlobalLock is checked and copies are limited by GlobalSize.
🤖 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 81-82: The FILETIME reconstruction is sign-extending because
dwLowDateTime/dwHighDateTime are treated as signed ints; in the fileInfos.Add
call inside DragDropFileExtractor (where Path.GetFileName(fd.cFileName),
fd.nFileSizeLow, fd.nFileSizeHigh, DateTime.FromFileTime(...)) convert both
ftLastWriteTime.dwHighDateTime and .dwLowDateTime to unsigned (uint) before
combining: cast each half to uint, shift the high half left by 32 bits as a
ulong, OR with the low half (as ulong), then cast the resulting 64-bit value to
long for DateTime.FromFileTime so the timestamp is reconstructed losslessly
without sign-extension.
---
Duplicate comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 69-80: The code reads cItems with Marshal.ReadInt32(ptr) and then
uses count * descSize + 4 without validating the input size or preventing
integer overflow; first ensure bytes.Length >= 4 before calling
Marshal.ReadInt32, then read count and validate it is non-negative and within a
sane limit, perform the size check using checked 64-bit arithmetic (e.g. compute
required = 4 + (long)count * descSize) and ensure required <= bytes.Length
before entering the loop and before calling Marshal.PtrToStructure for
FILEDESCRIPTOR; also bail out on suspiciously large counts to avoid OOM/overrun
after GCHandle.Alloc and before PtrToStructure.
- Around line 131-132: The code assumes nFileSizeLow proves the HGLOBAL buffer
size and directly calls Marshal.Copy; instead, in the DragDropFileExtractor
logic (the block checking fileInfo.SizeHigh and reading from an HGLOBAL), add a
GlobalLock null-check and use GlobalSize(hGlobal) to get the actual allocation
size, then bound the copy length to Math.Min((int)fileInfo.nFileSizeLow,
(int)globalSize) and only call Marshal.Copy when GlobalLock succeeded; also
preserve the existing UserMessageException for huge files but do not trust
nFileSizeLow alone. Apply the same pattern for the other HGLOBAL-read sites
referenced (the blocks around 153-169 and 213-221) so every GlobalLock is
checked and copies are limited by GlobalSize.
In `@PhotoLocator/MainWindow.xaml.cs`:
- Around line 322-325: The equality check is using reference equality on
string[] (dropped.Equals(_draggedFiles)), so identical path lists from different
array instances will be treated as different; change the comparison to compare
sequences by content (e.g., use Enumerable.SequenceEqual or compare lengths and
elements) when deciding whether to call _viewModel.HandleDroppedFiles(dropped),
and update _draggedFiles to the new array only after confirming they differ;
reference the local variable dropped, the field _draggedFiles, and the method
HandleDroppedFiles to locate the change.
- Around line 336-348: The code currently maps MessageBoxResult.Cancel to an
OperationCanceledException but the outer catch (Exception ex) always calls
ExceptionHandler.ShowException, which surfaces user-cancel as an error; update
the error handling around the try/catch that wraps the async copy task so that
OperationCanceledException (from the MessageBoxResult.Cancel mapping) is handled
separately and not passed to ExceptionHandler.ShowException — either add a
specific catch (OperationCanceledException) { /* no-op or user-cancel flow */ }
before the generic catch, or change the generic catch to if (!(ex is
OperationCanceledException)) ExceptionHandler.ShowException(ex); to ignore
cancels.
🪄 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: a6e351ce-db47-4036-b81b-5c9837c7e5b2
📒 Files selected for processing (3)
PhotoLocator/Helpers/DragDropFileExtractor.csPhotoLocator/MainViewModel.csPhotoLocator/MainWindow.xaml.cs
✅ Files skipped from review due to trivial changes (1)
- PhotoLocator/MainViewModel.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
PhotoLocator/Helpers/DragDropFileExtractor.cs (2)
127-129:⚠️ Potential issue | 🟠 MajorSkip blank basenames before building
targetPath.A malformed virtual descriptor can still leave
fileInfo.Nameempty here;Path.GetFileNamereturnsEmptyfor some inputs, so the write path below ends up targeting the directory itself and the firstFile.Create/File.WriteAllBytesfailure aborts the rest of the drop. Guardstring.IsNullOrWhiteSpace(fileInfo.Name)and skip the bad entry. (learn.microsoft.com)💡 Suggested guard
var fileInfo = fileInfos[i]; + if (string.IsNullOrWhiteSpace(fileInfo.Name)) + continue; var targetPath = Path.Combine(targetDirectory, fileInfo.Name); 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 127 - 129, Protect against empty basenames before constructing targetPath by checking fileInfo.Name for whitespace/empty and skipping that entry; in the loop that builds targetPath (uses variable fileInfo and method Path.Combine and the overwriteCheck callback), add a guard like string.IsNullOrWhiteSpace(fileInfo.Name) and continue to the next item if true so you never attempt to write to the directory itself.
52-76:⚠️ Potential issue | 🟠 MajorHandle both descriptor formats before reading the header.
Windows exposes separate ANSI and Unicode
FILEGROUPDESCRIPTORformats. Because this branch only queries"FileGroupDescriptorW", any provider that exposes only"FileGroupDescriptor"still returnsnullhere. It also callsMarshal.ReadInt32(ptr)before proving the blob even contains the 4-bytecItemsheader, so a truncated payload can reach the raw read before the later bounds check runs. Please fall back to"FileGroupDescriptor"with an ANSI parser, and rejectbytes.Length < sizeof(uint)before touching the pinned buffer. (learn.microsoft.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 52 - 76, Before pinning and calling Marshal.ReadInt32, validate the blob length (ensure bytes.Length >= sizeof(uint)) to avoid reading a truncated payload; change the GetData logic to try "FileGroupDescriptorW" first and if null fall back to "FileGroupDescriptor" and parse the ANSI descriptor layout accordingly (use the appropriate FILEGROUPDESCRIPTOR/FILEDESCRIPTOR ANSI sizes and field parsing). Update the code paths that compute descSize and read cItems (Marshal.ReadInt32(ptr)) so they only run after the length check, and ensure you use the correct Marshal.SizeOf for the chosen FILEDESCRIPTOR type when validating (count * descSize + 4 <= bytes.Length) before any pointer reads from the pinned GCHandle.Alloc buffer.
🤖 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 68-82: The code currently ignores FILEDESCRIPTOR.dwFlags; change
the tuple collected in fileInfos to also store fd.dwFlags and preserve that flag
value when adding entries (use FILEDESCRIPTOR.dwFlags alongside Name,
nFileSizeLow, nFileSizeHigh, ftLastWriteTime), then when consuming fileInfos
(where you currently use fd.nFileSizeLow/nFileSizeHigh and call
File.SetLastWriteTime), first check the flags: only construct/use the 64-bit
file size when (dwFlags & FD_FILESIZE) != 0 and only call
File.SetLastWriteTime/DateTime.FromFileTime when (dwFlags & FD_WRITESTIME) != 0.
Ensure you reference FILEDESCRIPTOR, fd.dwFlags, FD_FILESIZE (0x40) and
FD_WRITESTIME (0x20) in the checks so consumers skip absent fields safely.
---
Duplicate comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 127-129: Protect against empty basenames before constructing
targetPath by checking fileInfo.Name for whitespace/empty and skipping that
entry; in the loop that builds targetPath (uses variable fileInfo and method
Path.Combine and the overwriteCheck callback), add a guard like
string.IsNullOrWhiteSpace(fileInfo.Name) and continue to the next item if true
so you never attempt to write to the directory itself.
- Around line 52-76: Before pinning and calling Marshal.ReadInt32, validate the
blob length (ensure bytes.Length >= sizeof(uint)) to avoid reading a truncated
payload; change the GetData logic to try "FileGroupDescriptorW" first and if
null fall back to "FileGroupDescriptor" and parse the ANSI descriptor layout
accordingly (use the appropriate FILEGROUPDESCRIPTOR/FILEDESCRIPTOR ANSI sizes
and field parsing). Update the code paths that compute descSize and read cItems
(Marshal.ReadInt32(ptr)) so they only run after the length check, and ensure you
use the correct Marshal.SizeOf for the chosen FILEDESCRIPTOR type when
validating (count * descSize + 4 <= bytes.Length) before any pointer reads from
the pinned GCHandle.Alloc buffer.
🪄 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: c8926b5d-6c18-44d2-a7f5-0b0aa2ad1fd2
📒 Files selected for processing (1)
PhotoLocator/Helpers/DragDropFileExtractor.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
PhotoLocator/Helpers/DragDropFileExtractor.cs (1)
127-134: Consider logging or reporting failures whenGetDatathrows.The bare
catchswallows all exceptions silently. While this allows extraction to continue for remaining files, diagnostic information is lost. Consider at minimum logging the exception or incrementing a failure count to report to the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Helpers/DragDropFileExtractor.cs` around lines 127 - 134, The bare catch around comData.GetData(ref fmt, out medium) in DragDropFileExtractor.cs should be changed to capture and report the exception instead of swallowing it; replace the empty catch with catch(Exception ex) and either log the exception (including fmt and any identifying info) via the project's logger or increment a local failure counter (e.g., failureCount) so the caller can be informed, ensuring you continue the loop after recording the error; update any method return or out parameters to surface the failure count or attach diagnostics as appropriate.
🤖 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 237-238: The P/Invoke for GlobalSize is declared with return type
int which can truncate SIZE_T on 64-bit; change the extern declaration of
GlobalSize to return nint (or IntPtr) and adjust any usage that assumes an int
(e.g., casts or buffer length calculations) to use nint and validate/cast to int
only after checking the size is within Int32 range; update references to
GlobalSize and any code that reads the returned size to use the new nint type
safely.
---
Nitpick comments:
In `@PhotoLocator/Helpers/DragDropFileExtractor.cs`:
- Around line 127-134: The bare catch around comData.GetData(ref fmt, out
medium) in DragDropFileExtractor.cs should be changed to capture and report the
exception instead of swallowing it; replace the empty catch with catch(Exception
ex) and either log the exception (including fmt and any identifying info) via
the project's logger or increment a local failure counter (e.g., failureCount)
so the caller can be informed, ensuring you continue the loop after recording
the error; update any method return or out parameters to surface the failure
count or attach diagnostics as appropriate.
🪄 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: aafe0fc1-0078-4904-b995-41a0d677855c
📒 Files selected for processing (1)
PhotoLocator/Helpers/DragDropFileExtractor.cs
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