-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Code Quality: Removed ISidebarViewModel #17972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ce37c2d to
314d8a3
Compare
| private async void SidebarControl_ItemDropped(object sender, ItemDroppedEventArgs e) | ||
| { | ||
| await SidebarAdaptiveViewModel.HandleItemDroppedAsync(e); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The async event handler SidebarControl_ItemDropped is missing a required deferral, which can cause the drag operation's data to become invalid before async work finishes.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The SidebarControl_ItemDropped event handler is an async method that performs significant asynchronous work, such as file operations and database updates, via SidebarAdaptiveViewModel.HandleItemDroppedAsync. However, it fails to acquire a deferral from the ItemDroppedEventArgs. In the WinUI drag-and-drop API, a deferral is necessary to prevent the operating system from cleaning up the drag operation's data context before the asynchronous work completes. Without it, the DataPackageView could be invalidated mid-operation, leading to failed drop actions.
💡 Suggested Fix
In the SidebarControl_ItemDropped method, acquire a deferral from the event arguments before the await call and complete it after the operation is finished. Example: var deferral = e.RawEvent.GetDeferral(); await SidebarAdaptiveViewModel.HandleItemDroppedAsync(e); deferral.Complete();
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/Files.App/Views/MainPage.xaml.cs#L482-L485
Potential issue: The `SidebarControl_ItemDropped` event handler is an `async` method
that performs significant asynchronous work, such as file operations and database
updates, via `SidebarAdaptiveViewModel.HandleItemDroppedAsync`. However, it fails to
acquire a deferral from the `ItemDroppedEventArgs`. In the WinUI drag-and-drop API, a
deferral is necessary to prevent the operating system from cleaning up the drag
operation's data context before the asynchronous work completes. Without it, the
`DataPackageView` could be invalidated mid-operation, leading to failed drop actions.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7750491
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

Resolved / Related Issues
This is part of the phase 1 mentioned in #17970 (comment)
Steps used to test these changes
Check if the click functionalities are the same as those in the main branch.