Skip to content

Fix #3705: Code window is empty when select a .baml and refresh#3726

Open
christophwille wants to merge 1 commit intomasterfrom
christophwille/3705
Open

Fix #3705: Code window is empty when select a .baml and refresh#3726
christophwille wants to merge 1 commit intomasterfrom
christophwille/3705

Conversation

@christophwille
Copy link
Copy Markdown
Member

@christophwille christophwille commented Apr 26, 2026

image image image image image

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes ILSpy issue #3705 where the code window can become empty after selecting a .baml resource and refreshing, by ensuring the relevant assembly is loaded before attempting to re-select the previously selected tree path.

Changes:

  • Refactors the refresh path into an async flow so the UI can await assembly metadata loading.
  • Preloads the selected assembly’s metadata before calling FindNodeByPath, improving lookup of lazy-loaded resource nodes.
  • Keeps the existing refresh throttle entry point (Refresh()refreshThrottle.Tick()).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 955 to +973
using (Keyboard.FocusedElement.PreserveFocus())
{
var path = GetPathForNode(SelectedItem);

ShowAssemblyList(settingsService.AssemblyListManager.LoadList(AssemblyList.ListName));

// Ensure assembly loaded before FindNodeByPath to allow lazy-loaded resource nodes to be found
if (path?.Length > 0)
{
foreach (var asm in AssemblyList.GetAssemblies())
{
if (asm.FileName == path[0])
{
await asm.GetMetadataFileAsync().Catch<Exception>(_ => { });
break;
}
}
}

Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

Keyboard.FocusedElement.PreserveFocus() now spans an await. Since PreserveFocus restores focus unconditionally on Dispose (see ExtensionMethods.cs), a slow GetMetadataFileAsync() can cause focus to be restored later and steal focus from whatever the user clicked during the await. Consider only restoring focus if it hasn't changed since the refresh started (or move the PreserveFocus scope so it doesn't cover the awaited portion).

Suggested change
using (Keyboard.FocusedElement.PreserveFocus())
{
var path = GetPathForNode(SelectedItem);
ShowAssemblyList(settingsService.AssemblyListManager.LoadList(AssemblyList.ListName));
// Ensure assembly loaded before FindNodeByPath to allow lazy-loaded resource nodes to be found
if (path?.Length > 0)
{
foreach (var asm in AssemblyList.GetAssemblies())
{
if (asm.FileName == path[0])
{
await asm.GetMetadataFileAsync().Catch<Exception>(_ => { });
break;
}
}
}
var path = GetPathForNode(SelectedItem);
using (Keyboard.FocusedElement.PreserveFocus())
{
ShowAssemblyList(settingsService.AssemblyListManager.LoadList(AssemblyList.ListName));
}
// Ensure assembly loaded before FindNodeByPath to allow lazy-loaded resource nodes to be found
if (path?.Length > 0)
{
foreach (var asm in AssemblyList.GetAssemblies())
{
if (asm.FileName == path[0])
{
await asm.GetMetadataFileAsync().Catch<Exception>(_ => { });
break;
}
}
}
using (Keyboard.FocusedElement.PreserveFocus())
{

Copilot uses AI. Check for mistakes.

private void RefreshInternal()
{
_ = RefreshInternalAsync();
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

RefreshInternal() fires RefreshInternalAsync() without observing the returned task. If RefreshInternalAsync throws (e.g., from ShowAssemblyList, SelectNode, or RefreshDecompiledView), the exception becomes unobserved and can be lost/crash later. Consider attaching the existing task error handling used elsewhere in this file (e.g., call .HandleExceptions() on the returned task, or explicitly .Catch<Exception>(...) + .IgnoreExceptions()).

Suggested change
_ = RefreshInternalAsync();
_ = RefreshInternalAsync().HandleExceptions();

Copilot uses AI. Check for mistakes.
Comment on lines +949 to 954
{
_ = RefreshInternalAsync();
}

private async Task RefreshInternalAsync()
{
Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

Because RefreshInternal() can be invoked multiple times (via refreshThrottle.Tick()), the fire-and-forget async refresh allows multiple RefreshInternalAsync executions to overlap/re-enter while awaiting GetMetadataFileAsync(). That can result in out-of-order ShowAssemblyList/SelectNode/RefreshDecompiledView updates. Consider serializing refreshes (e.g., track the currently running refresh task and skip/await it, or use a generation id/cancellation token so only the latest refresh applies).

Copilot uses AI. Check for mistakes.
}
}
}

Copy link

Copilot AI Apr 26, 2026

Choose a reason for hiding this comment

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

RefreshInternalAsync captures a path and later awaits GetMetadataFileAsync(). During that await, the user can change the selection; when execution resumes, SelectNode(FindNodeByPath(path, ...)) may unexpectedly revert the selection to the stale path. Consider capturing the initial selection/path and, before calling SelectNode/RefreshDecompiledView, verifying the selection is still the same (or using a refresh generation id so stale refreshes don't apply).

Suggested change
var currentPath = GetPathForNode(SelectedItem);
var selectionChanged = SelectedItem is not null
&& !((path == null && currentPath == null)
|| (path != null && currentPath != null && path.SequenceEqual(currentPath)));
if (selectionChanged)
return;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants