[Proposal?] Update the PM UI when a successful restore finishes#7209
[Proposal?] Update the PM UI when a successful restore finishes#7209
Conversation
| _service.ProjectRemoved += OnProjectRemoved; | ||
| _service.ProjectRenamed += OnProjectRenamed; | ||
| _service.ProjectUpdated += OnProjectUpdated; | ||
| _service.SolutionRestoreCompleted += OnSolutionRestoreCompleted; |
There was a problem hiding this comment.
So AfterNuGetCacheUpdated is tied to nomination.
Is AfterNuGetCacheUpdated not firing?
There was a problem hiding this comment.
Yep is not firing, not sure why, I'll investigate more since doesn't make a lot of sense to me
There was a problem hiding this comment.
@NuGet/nuget-client Okay second investigation help me understand better the problem:
There are two main problems in the PM UI:
- We refresh when nomination fires, not when restore completes, so it could happen that PM UI is not up to date with the project state.
- We only refresh the PM UI if it's visible, we have a comment saying that it will refresh when we focus on it again, but that code is not working, it's a simple fix
Most likely what is happening is that Copilot Chat switches focus to the csproj file and PM UI is not being restored, I was able to reproduce this behavior, but also Copilot doesn't always focus to the file.
I was also able to reproduce the "late" restore problem, most likely because I was debugging.
I think adding a way to restore the PM UI when restore finishes is needed but could be more intrusive change.
There was a problem hiding this comment.
I think adding a way to restore the PM UI when restore finishes is needed but could be more intrusive change
Yeah, let's delay that, but there's already some APIs that are intelligent about when a project has been updated, so if we do that, we should be able to only do restore updates when needed.
If we add a simple restore refresh, I think it'll refresh twice when package installation happens.
There was a problem hiding this comment.
I changed the implementation, I added two new event handlers in the PM UI, ProjectUpdateFinished and SolutionRestoreFinished, this is the flow of each event:
Per-project: RestoreRunner.CommitAsync calls EndProjectUpdate → fires ProjectUpdateFinished once per project, immediately after that project's assets files are written.
Guarded by !isNoOp — only fires when something actually changed.
Solution-level: SolutionRestoreJob calls EndSolutionRestore → fires SolutionRestoreFinished once at the very end, after ALL projects have been committed (including
no-ops).
I had to add the SolutionRestoreFinished to avoid N project refreshes in the PM UI, but by combining both we can handle no-op restores in the Solution level
jebriede
left a comment
There was a problem hiding this comment.
@martinrrm it looks like you're going to explore refreshing when a project has been updated instead of when the restore completes. Please request rereviews and update the PR description when that's ready. Thanks!
Use _projectUpdateOccurredDuringRestore flag to track whether any project had a non-no-op restore. Solution-level PM UI only refreshes in OnSolutionRestoreFinished if the flag is set, avoiding unnecessary refreshes when the entire restore was a no-op. Also reorder OnProjectUpdateFinished to check Model.IsSolution first, setting the flag for solution-level PM UI before checking visibility. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ad08b79 to
2b6ba07
Compare
Bug
Fixes: NuGet/Home#14761
Description
This is a proposal fix for NuGet/Home#14761, currently we don't directly watch for files saves, instead we do something like this:
If the file save causes CPS/project system nomination, the path is:
VsSolutionRestoreService4.
AddProjectRestoreInfo(...)updatesProjectSystemCacheCacheUpdatedfiresVSSolutionManager.AfterNuGetCacheUpdatedfiresOnNuGetCacheUpdatedRefresh PM UI on restore completion instead of cache nomination
The Package Manager UI previously refreshed via
AfterNuGetCacheUpdated, which fires during nomination (pre-restore) — meaning the UI would refresh with stale databefore the restore had actually written updated assets files.
This PR replaces that mechanism with
IVsNuGetProjectUpdateEvents, which fires after restore completes and assets files are written:ProjectUpdateFinished— per-project, post-restore signal. Only fires for non-no-op restores (guarded by!isNoOpinRestoreRunner.CommitAsync). Also fires forpackages.config install/uninstall via
PackageProjectEvents.BatchEnd.SolutionRestoreFinished— solution-level, post-restore signal. Used for solution-level PM UI, but only triggers a refresh if at least oneProjectUpdateFinishedfired during that restore cycle (skips full no-op restores).
Key changes
PackageManagerControl.xaml.csAfterNuGetCacheUpdatedsubscription withIVsNuGetProjectUpdateEvents.ProjectUpdateFinishedandSolutionRestoreFinishedPackageManagerControl.xaml.cs_projectUpdateOccurredDuringRestoreflag so solution-level PM UI skips refresh on full no-op restoresPackageManagerControl.xaml.cs_isRefreshRequired = trueand refresh on nextLoadedeventPackageManagerUIRefreshEvent.csRestoreCompletedtoRefreshOperationSourcetelemetry enumVsRestoreProgressEventsTests.csEndProjectUpdateandEndSolutionRestoreNuGetTelemetryServiceTests.csRestoreCompleted(Success, NoOp, NotApplicable)Behavior summary
CacheUpdated)ProjectUpdateFinished)BatchEnd_projectUpdateOccurredDuringRestore_isRefreshRequiredtriggers refresh onLoadedHow it works
OnProjectUpdateFinishedfilters by matchingprojectUniqueNameagainst the viewed project. Refreshes immediately if visible, defers ifhidden.
OnProjectUpdateFinishedsets_projectUpdateOccurredDuringRestore = true.OnSolutionRestoreFinishedchecks this flag — only refreshes ifat least one project had a real restore. This avoids both N-per-project refreshes and unnecessary refreshes on full no-op restores.
_isExecutingActiongates all refresh signals; single refresh happens inExecuteActionfinally block (no double refresh).PR Checklist