Skip to content

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.

Sign up for GitHub

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

Open
0x5bfa wants to merge 4 commits into
base: main
Choose a base branch
from
+61 −119
Open

Code Quality: Removed ISidebarViewModel #17972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c7408a
Update the UI automation classes
0x5bfa Dec 18, 2025
314d8a3
Removed ISidebarViewModel
0x5bfa Dec 18, 2025
8cbdfc4
Revert the DP changes
0x5bfa Dec 19, 2025
c32a679
Req
0x5bfa Dec 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 1 addition & 36 deletions src/Files.App.Controls/Sidebar/ISidebarViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,8 @@

namespace Files.App.Controls
{
public record ItemInvokedEventArgs(PointerUpdateKind PointerUpdateKind) { }
public record ItemDroppedEventArgs(object DropTarget, DataPackageView DroppedItem, SidebarItemDropPosition dropPosition, DragEventArgs RawEvent) { }
public record ItemDragOverEventArgs(object DropTarget, DataPackageView DroppedItem, SidebarItemDropPosition dropPosition, DragEventArgs RawEvent) { }
public record ItemContextInvokedArgs(object? Item, Point Position) { }

public interface ISidebarViewModel
{
/// <summary>
/// The source/list of items that will be rendered in the sidebar
/// </summary>
object SidebarItems { get; }

/// <summary>
/// Gets invoked when the context was requested for an item in the sidebar.
/// Also applies when context was requested for the pane itsself.
/// </summary>
/// <param name="sender">The sender of this event</param>
/// <param name="args">The <see cref="ItemContextInvokedArgs"/> for this event.</param>
void HandleItemContextInvokedAsync(object sender, ItemContextInvokedArgs args);

/// <summary>
/// Gets invoked when an item drags over any item of the sidebar.
/// </summary>
/// <param name="args">The <see cref="ItemDragOverEventArgs"/> for this event.</param>
/// <returns>A <see cref="Task"/> that represents the asynchronous operation.</returns>
Task HandleItemDragOverAsync(ItemDragOverEventArgs args);

/// <summary>
/// Gets invoked when an item is dropped on any item of the sidebar.
/// </summary>
/// <param name="args">The <see cref="ItemDroppedEventArgs"/> for this event.</param>
/// <returns>A <see cref="Task"/> that represents the asynchronous operation.</returns>
Task HandleItemDroppedAsync(ItemDroppedEventArgs args);

/// <summary>
/// Gets invoked when an item is invoked (double clicked) on any item of the sidebar.
/// </summary>
/// <param name="item">The item that was invoked.</param>
void HandleItemInvokedAsync(object item, PointerUpdateKind pointerUpdateKind);
}
}
12 changes: 3 additions & 9 deletions src/Files.App.Controls/Sidebar/SidebarItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,12 +404,7 @@ private async void ItemBorder_DragOver(object sender, DragEventArgs e)
VisualStateManager.GoToState(this, "DragInsertBelow", true);
}

if (Owner is not null)
{
var deferral = e.GetDeferral();
await Owner.RaiseItemDragOver(this, insertsAbove, e);
deferral.Complete();
}
Owner?.RaiseItemDragOver(this, insertsAbove, e);
}

private void ItemBorder_ContextRequested(UIElement sender, Microsoft.UI.Xaml.Input.ContextRequestedEventArgs args)
Expand All @@ -423,11 +418,10 @@ private void ItemBorder_DragLeave(object sender, DragEventArgs e)
UpdatePointerState();
}

private async void ItemBorder_Drop(object sender, DragEventArgs e)
private void ItemBorder_Drop(object sender, DragEventArgs e)
{
UpdatePointerState();
if (Owner is not null)
await Owner.RaiseItemDropped(this, DetermineDropTargetPosition(e), e);
Owner?.RaiseItemDropped(this, DetermineDropTargetPosition(e), e);
}

private SidebarItemDropPosition DetermineDropTargetPosition(DragEventArgs args)
Expand Down
22 changes: 9 additions & 13 deletions src/Files.App.Controls/Sidebar/SidebarItemAutomationPeer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

namespace Files.App.Controls
{
/// <summary>
/// Exposes <see cref="SidebarItem"/> types to Microsoft UI Automation.
/// </summary>
public sealed partial class SidebarItemAutomationPeer : FrameworkElementAutomationPeer, IInvokeProvider, IExpandCollapseProvider, ISelectionItemProvider
{
public ExpandCollapseState ExpandCollapseState
Expand All @@ -17,6 +20,7 @@ public ExpandCollapseState ExpandCollapseState
{
if (Owner.HasChildren)
return Owner.IsExpanded ? ExpandCollapseState.Expanded : ExpandCollapseState.Collapsed;

return ExpandCollapseState.LeafNode;
}
}
Expand All @@ -27,7 +31,7 @@ public ExpandCollapseState ExpandCollapseState

public SidebarItemAutomationPeer(SidebarItem owner) : base(owner)
{
this.Owner = owner;
Owner = owner;
}

protected override AutomationControlType GetAutomationControlTypeCore()
Expand All @@ -49,28 +53,22 @@ protected override object GetPatternCore(PatternInterface patternInterface)
else if (patternInterface == PatternInterface.ExpandCollapse)
{
if (Owner.CollapseEnabled)
{
return this;
}
}

return base.GetPatternCore(patternInterface);
}

public void Collapse()
{
if (Owner.CollapseEnabled)
{
Owner.IsExpanded = false;
}
}

public void Expand()
{

if (Owner.CollapseEnabled)
{
Owner.IsExpanded = true;
}
}

public void Invoke()
Expand Down Expand Up @@ -106,13 +104,11 @@ protected override int GetPositionInSetCore()
private IList GetOwnerCollection()
{
if (Owner.FindAscendant<SidebarItem>() is SidebarItem parent && parent.Item?.Children is IList list)
{
return list;
}
if (Owner?.Owner is not null && Owner.Owner.ViewModel.SidebarItems is IList items)
{

if (Owner?.Owner is not null && Owner.Owner?.MenuItemsSource is IList items)
return items;
}

return new List<object>();
}
}
Expand Down
8 changes: 0 additions & 8 deletions src/Files.App.Controls/Sidebar/SidebarView.Properties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,6 @@ public double NegativeOpenPaneLength
public static readonly DependencyProperty NegativeOpenPaneLengthProperty =
DependencyProperty.Register(nameof(NegativeOpenPaneLength), typeof(double), typeof(SidebarView), new PropertyMetadata(null));

public ISidebarViewModel ViewModel
{
get => (ISidebarViewModel)GetValue(ViewModelProperty);
set => SetValue(ViewModelProperty, value);
}
public static readonly DependencyProperty ViewModelProperty =
DependencyProperty.Register(nameof(ViewModel), typeof(ISidebarViewModel), typeof(SidebarView), new PropertyMetadata(null));

public ISidebarItemModel SelectedItem
{
get => (ISidebarItemModel)GetValue(SelectedItemProperty);
Expand Down
21 changes: 10 additions & 11 deletions src/Files.App.Controls/Sidebar/SidebarView.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ public sealed partial class SidebarView : UserControl, INotifyPropertyChanged
{
private const double COMPACT_MAX_WIDTH = 200;

public event EventHandler<object>? ItemInvoked;
public event EventHandler<ItemInvokedEventArgs>? ItemInvoked;
public event EventHandler<ItemContextInvokedArgs>? ItemContextInvoked;
public event EventHandler<ItemDragOverEventArgs>? ItemDragOver;
public event EventHandler<ItemDroppedEventArgs>? ItemDropped;
public event PropertyChangedEventHandler? PropertyChanged;

internal SidebarItem? SelectedItemContainer = null;
Expand All @@ -40,26 +42,24 @@ internal void RaiseItemInvoked(SidebarItem item, PointerUpdateKind pointerUpdate
if (item.Item is null || item.IsGroupHeader) return;

SelectedItem = item.Item;
ItemInvoked?.Invoke(item, item.Item);
ViewModel.HandleItemInvokedAsync(item.Item, pointerUpdateKind);
ItemInvoked?.Invoke(item, new(pointerUpdateKind));
}

internal void RaiseContextRequested(SidebarItem item, Point e)
{
ItemContextInvoked?.Invoke(item, new ItemContextInvokedArgs(item.Item, e));
ViewModel.HandleItemContextInvokedAsync(item, new ItemContextInvokedArgs(item.Item, e));
ItemContextInvoked?.Invoke(item, new(item.Item, e));
}

internal async Task RaiseItemDropped(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
internal void RaiseItemDropped(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
{
if (sideBarItem.Item is null) return;
await ViewModel.HandleItemDroppedAsync(new ItemDroppedEventArgs(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
ItemDropped?.Invoke(this, new(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
}

internal async Task RaiseItemDragOver(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
internal void RaiseItemDragOver(SidebarItem sideBarItem, SidebarItemDropPosition dropPosition, DragEventArgs rawEvent)
{
if (sideBarItem.Item is null) return;
await ViewModel.HandleItemDragOverAsync(new ItemDragOverEventArgs(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
ItemDragOver?.Invoke(this, new(sideBarItem.Item, rawEvent.DataView, dropPosition, rawEvent));
}

private void UpdateMinimalMode()
Expand Down Expand Up @@ -230,8 +230,7 @@ private void SidebarResizer_ManipulationCompleted(object sender, ManipulationCom

private void MenuItemHostScrollViewer_ContextRequested(UIElement sender, ContextRequestedEventArgs e)
{
var newArgs = new ItemContextInvokedArgs(null, e.TryGetPosition(this, out var point) ? point : default);
ViewModel.HandleItemContextInvokedAsync(this, newArgs);
ItemContextInvoked?.Invoke(this, new(null, e.TryGetPosition(this, out var point) ? point : default));
e.Handled = true;
}

Expand Down
14 changes: 7 additions & 7 deletions src/Files.App.Controls/Sidebar/SidebarViewAutomationPeer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@

namespace Files.App.Controls
{
/// <summary>
/// Exposes <see cref="SidebarView"/> types to Microsoft UI Automation.
/// </summary>
public sealed partial class SidebarViewAutomationPeer : FrameworkElementAutomationPeer, ISelectionProvider
{
public bool CanSelectMultiple => false;
Expand All @@ -20,20 +23,17 @@ public SidebarViewAutomationPeer(SidebarView owner) : base(owner)

protected override object GetPatternCore(PatternInterface patternInterface)
{
if (patternInterface == PatternInterface.Selection)
{
if (patternInterface is PatternInterface.Selection)
return this;
}

return base.GetPatternCore(patternInterface);
}

public IRawElementProviderSimple[] GetSelection()
{
if (Owner.SelectedItemContainer != null)
return
[
ProviderFromPeer(CreatePeerForElement(Owner.SelectedItemContainer))
];
return [ProviderFromPeer(CreatePeerForElement(Owner.SelectedItemContainer))];

return [];
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Files.App/ViewModels/UserControls/SidebarViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

namespace Files.App.ViewModels.UserControls
{
public sealed partial class SidebarViewModel : ObservableObject, IDisposable, ISidebarViewModel
public sealed partial class SidebarViewModel : ObservableObject, IDisposable
{
private INetworkService NetworkService { get; } = Ioc.Default.GetRequiredService<INetworkService>();
private IUserSettingsService UserSettingsService { get; } = Ioc.Default.GetRequiredService<IUserSettingsService>();
Expand Down
7 changes: 5 additions & 2 deletions src/Files.App/Views/MainPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,14 @@
HorizontalContentAlignment="Stretch"
DisplayMode="{x:Bind SidebarAdaptiveViewModel.SidebarDisplayMode, Mode=TwoWay}"
IsPaneOpen="{x:Bind ViewModel.IsSidebarPaneOpen, Mode=TwoWay}"
ItemContextInvoked="SidebarControl_ItemContextInvoked"
ItemDragOver="SidebarControl_ItemDragOver"
ItemDropped="SidebarControl_ItemDropped"
ItemInvoked="SidebarControl_ItemInvoked"
Loaded="SidebarControl_Loaded"
MenuItemsSource="{x:Bind SidebarAdaptiveViewModel.SidebarItems, Mode=OneWay}"
OpenPaneLength="{x:Bind UserSettingsService.AppearanceSettingsService.SidebarWidth, Mode=TwoWay}"
SelectedItem="{x:Bind SidebarAdaptiveViewModel.SidebarSelectedItem, Mode=TwoWay}"
ViewModel="{x:Bind SidebarAdaptiveViewModel}">
SelectedItem="{x:Bind SidebarAdaptiveViewModel.SidebarSelectedItem, Mode=TwoWay}">

<!-- Inner Content -->
<controls:SidebarView.InnerContent>
Expand Down
25 changes: 25 additions & 0 deletions src/Files.App/Views/MainPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.UI.Xaml.Navigation;
using Windows.Foundation.Metadata;
using Windows.Graphics;
using Windows.UI.Input;
using WinUIEx;
using GridSplitter = Files.App.Controls.GridSplitter;
using VirtualKey = Windows.System.VirtualKey;
Expand Down Expand Up @@ -465,5 +466,29 @@ private void Page_PointerReleased(object sender, PointerRoutedEventArgs e)
// shortcuts from working properly, see https://github.com/microsoft/microsoft-ui-xaml/issues/6467
DispatcherQueue.TryEnqueue(() => ContentPageContext.ShellPage?.PaneHolder.FocusActivePane());
}

private void SidebarControl_ItemContextInvoked(object sender, ItemContextInvokedArgs e)
{
SidebarAdaptiveViewModel.HandleItemContextInvokedAsync(sender, e);
}
Comment on lines +470 to +473
Copy link

@sentry sentry bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The SidebarControl_ItemContextInvoked and SidebarControl_ItemInvoked handlers call async void methods without await, which can cause unhandled exceptions and application crashes.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The event handlers SidebarControl_ItemContextInvoked and SidebarControl_ItemInvoked call the async void methods HandleItemContextInvokedAsync and HandleItemInvokedAsync respectively. Because these calls are not awaited, any exception thrown within the async methods, such as during file system access or navigation operations, will be unhandled. An unhandled exception from an async void method will crash the application. This issue arises when a user clicks or right-clicks an item in the sidebar, and an underlying operation fails.

💡 Suggested Fix

Change the SidebarControl_ItemContextInvoked and SidebarControl_ItemInvoked event handlers to be async void. Then, await the calls to HandleItemContextInvokedAsync and HandleItemInvokedAsync within them. This ensures that any exceptions are propagated to the dispatcher and can be handled, preventing application crashes. Alternatively, add comprehensive try-catch blocks inside the called methods.

🤖 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#L470-L473

Potential issue: The event handlers `SidebarControl_ItemContextInvoked` and
`SidebarControl_ItemInvoked` call the `async void` methods
`HandleItemContextInvokedAsync` and `HandleItemInvokedAsync` respectively. Because these
calls are not awaited, any exception thrown within the `async` methods, such as during
file system access or navigation operations, will be unhandled. An unhandled exception
from an `async void` method will crash the application. This issue arises when a user
clicks or right-clicks an item in the sidebar, and an underlying operation fails.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7808336


private async void SidebarControl_ItemDragOver(object sender, ItemDragOverEventArgs e)
{
var deferral = e.RawEvent.GetDeferral();
await SidebarAdaptiveViewModel.HandleItemDragOverAsync(e);
deferral.Complete();
}

private async void SidebarControl_ItemDropped(object sender, ItemDroppedEventArgs e)
{
var deferral = e.RawEvent.GetDeferral();
await SidebarAdaptiveViewModel.HandleItemDroppedAsync(e);
deferral.Complete();
}

private void SidebarControl_ItemInvoked(object sender, ItemInvokedEventArgs e)
{
SidebarAdaptiveViewModel.HandleItemInvokedAsync(((SidebarItem)sender).Item, e.PointerUpdateKind);
}
}
}
30 changes: 0 additions & 30 deletions tests/Files.App.UITests/TestData/TestSidebarViewModel.cs
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions tests/Files.App.UITests/Views/SidebarViewPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ public SidebarViewPage()
sidebarModels.Add(new TestSidebarModel { Text = "Test 1" });
sidebarModels.Add(new TestSidebarModel { Text = "Test 2" });
sidebarModels.Add(new TestSidebarModel { Text = "Test 3" });

Sidebar.ViewModel = new TestSidebarViewModel();
}
}
}
Loading