[Windows] Fix RefreshView IsRefreshing property not working while binding#34845
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34845Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34845" |
dotnet-policy-service
Bot
added
the
partner/syncfusion
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-specific RefreshView regression where IsRefreshing=true (via binding) wouldn’t show the indicator after navigating back to a page, by ensuring the Windows handler re-applies refresh state whenever the control is loaded.
Changes:
- Keep the Windows
RefreshContainer.Loadedhandler subscribed soUpdateIsRefreshing()can run on subsequent loads. - Add a new HostApp repro page for issue #30535.
- Add a new Appium/NUnit UI test for issue #30535.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Core/src/Handlers/RefreshView/RefreshViewHandler.Windows.cs | Stops self-unsubscribing from Loaded so refresh state can be re-triggered after navigation/back. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue30535.cs | Adds repro scenario page using RefreshView bound to a ViewModel and navigation to an options page. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30535.cs | Adds an automated UI test that navigates, sets IsRefreshing=true, returns, and screenshots the result. |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
MauiBot
added
s/agent-changes-requested
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Could you please review the ai's summary?
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 5 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 3 findings
See inline comments for details.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 4 findings
See inline comments for details.
🤖 AI Summary
📊 Review Session —
|
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🖥️ Issue30535 Issue30535 |
✅ FAIL — 568s | ✅ PASS — 464s |
🔴 Without fix — 🖥️ Issue30535: FAIL ✅ · 568s
Determining projects to restore...
Restored D:\a\1\s\src\Controls\src\Core\Controls.Core.csproj (in 40.46 sec).
Restored D:\a\1\s\src\Controls\Maps\src\Controls.Maps.csproj (in 40.3 sec).
Restored D:\a\1\s\src\Controls\Foldable\src\Controls.Foldable.csproj (in 382 ms).
Restored D:\a\1\s\src\BlazorWebView\src\Maui\Microsoft.AspNetCore.Components.WebView.Maui.csproj (in 4.04 sec).
Restored D:\a\1\s\src\Graphics\src\Graphics.Win2D\Graphics.Win2D.csproj (in 23 ms).
Restored D:\a\1\s\src\Essentials\src\Essentials.csproj (in 22 ms).
Restored D:\a\1\s\src\Graphics\src\Graphics\Graphics.csproj (in 3.75 sec).
Restored D:\a\1\s\src\Core\maps\src\Maps.csproj (in 32 ms).
Restored D:\a\1\s\src\Core\src\Core.csproj (in 56 ms).
Restored D:\a\1\s\src\Controls\src\Xaml\Controls.Xaml.csproj (in 24 ms).
Restored D:\a\1\s\src\Controls\tests\TestCases.HostApp\Controls.TestCases.HostApp.csproj (in 1.12 sec).
3 of 14 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics.Win2D -> D:\a\1\s\artifacts\bin\Graphics.Win2D\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.Win2D.WinUI.Desktop.dll
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Maps -> D:\a\1\s\artifacts\bin\Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Foldable -> D:\a\1\s\artifacts\bin\Controls.Foldable\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Foldable.dll
Controls.Xaml -> D:\a\1\s\artifacts\bin\Controls.Xaml\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Xaml.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Maps -> D:\a\1\s\artifacts\bin\Controls.Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Maps.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Microsoft.AspNetCore.Components.WebView.Maui -> D:\a\1\s\artifacts\bin\Microsoft.AspNetCore.Components.WebView.Maui\Debug\net10.0-windows10.0.19041.0\Microsoft.AspNetCore.Components.WebView.Maui.dll
Controls.TestCases.HostApp -> D:\a\1\s\artifacts\bin\Controls.TestCases.HostApp\Debug\net10.0-windows10.0.19041.0\win-x64\Controls.TestCases.HostApp.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:06:02.56
Determining projects to restore...
Restored D:\a\1\s\src\TestUtils\src\VisualTestUtils\VisualTestUtils.csproj (in 916 ms).
Restored D:\a\1\s\src\TestUtils\src\UITest.NUnit\UITest.NUnit.csproj (in 1.36 sec).
Restored D:\a\1\s\src\TestUtils\src\UITest.Core\UITest.Core.csproj (in 3 ms).
Restored D:\a\1\s\src\TestUtils\src\UITest.Appium\UITest.Appium.csproj (in 1.86 sec).
Restored D:\a\1\s\src\TestUtils\src\VisualTestUtils.MagickNet\VisualTestUtils.MagickNet.csproj (in 5.44 sec).
Restored D:\a\1\s\src\Controls\tests\TestCases.WinUI.Tests\Controls.TestCases.WinUI.Tests.csproj (in 3.79 sec).
Restored D:\a\1\s\src\Controls\tests\CustomAttributes\Controls.CustomAttributes.csproj (in 25 ms).
Restored D:\a\1\s\src\TestUtils\src\UITest.Analyzers\UITest.Analyzers.csproj (in 9.73 sec).
7 of 15 projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0\Microsoft.Maui.Essentials.dll
Controls.CustomAttributes -> D:\a\1\s\artifacts\bin\Controls.CustomAttributes\Debug\net10.0\Controls.CustomAttributes.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0\Microsoft.Maui.dll
Controls.Core.Design -> D:\a\1\s\artifacts\bin\Controls.Core.Design\Debug\net472\Microsoft.Maui.Controls.DesignTools.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.dll
UITest.Core -> D:\a\1\s\artifacts\bin\UITest.Core\Debug\net10.0\UITest.Core.dll
UITest.Appium -> D:\a\1\s\artifacts\bin\UITest.Appium\Debug\net10.0\UITest.Appium.dll
UITest.NUnit -> D:\a\1\s\artifacts\bin\UITest.NUnit\Debug\net10.0\UITest.NUnit.dll
VisualTestUtils -> D:\a\1\s\artifacts\bin\VisualTestUtils\Debug\netstandard2.0\VisualTestUtils.dll
VisualTestUtils.MagickNet -> D:\a\1\s\artifacts\bin\VisualTestUtils.MagickNet\Debug\netstandard2.0\VisualTestUtils.MagickNet.dll
UITest.Analyzers -> D:\a\1\s\artifacts\bin\UITest.Analyzers\Debug\netstandard2.0\UITest.Analyzers.dll
Controls.TestCases.WinUI.Tests -> D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
Test run for D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 5/6/2026 9:18:44 PM FixtureSetup for Issue30535(Windows)
>>>>> 5/6/2026 9:18:53 PM RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus Start
>>>>> 5/6/2026 9:18:58 PM RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus Stop
>>>>> 5/6/2026 9:18:58 PM Log types:
Failed RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus [5 s]
Error Message:
VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus.png (12.04% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.
More info: https://aka.ms/visual-test-workflow
Stack Trace:
at VisualTestUtils.VisualRegressionTester.Fail(String message) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 162
at VisualTestUtils.VisualRegressionTester.VerifyMatchesSnapshot(String name, ImageSnapshot actualImage, String environmentName, ITestContext testContext) in /_/src/TestUtils/src/VisualTestUtils/VisualRegressionTester.cs:line 123
at Microsoft.Maui.TestCases.Tests.UITest.<VerifyScreenshot>g__Verify|13_0(String name, <>c__DisplayClass13_0&) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 477
at Microsoft.Maui.TestCases.Tests.UITest.VerifyScreenshot(String name, Nullable`1 retryDelay, Nullable`1 retryTimeout, Int32 cropLeft, Int32 cropRight, Int32 cropTop, Int32 cropBottom, Double tolerance, Boolean includeTitleBar) in /_/src/Controls/tests/TestCases.Shared.Tests/UITest.cs:line 309
at Microsoft.Maui.TestCases.Tests.Issues.Issue30535.RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus() in /_/src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30535.cs:line 25
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
NUnit Adapter 4.5.0.0: Test execution complete
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.10] Discovering: Controls.TestCases.WinUI.Tests
[xUnit.net 00:00:00.31] Discovered: Controls.TestCases.WinUI.Tests
Total tests: 1
Failed: 1
Test Run Failed.
Total time: 33.6436 Seconds
🟢 With fix — 🖥️ Issue30535: PASS ✅ · 464s
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics.Win2D -> D:\a\1\s\artifacts\bin\Graphics.Win2D\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Graphics.Win2D.WinUI.Desktop.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Maps -> D:\a\1\s\artifacts\bin\Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Maps.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Xaml -> D:\a\1\s\artifacts\bin\Controls.Xaml\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Xaml.dll
Microsoft.AspNetCore.Components.WebView.Maui -> D:\a\1\s\artifacts\bin\Microsoft.AspNetCore.Components.WebView.Maui\Debug\net10.0-windows10.0.19041.0\Microsoft.AspNetCore.Components.WebView.Maui.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Foldable -> D:\a\1\s\artifacts\bin\Controls.Foldable\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Foldable.dll
Controls.Maps -> D:\a\1\s\artifacts\bin\Controls.Maps\Debug\net10.0-windows10.0.19041.0\Microsoft.Maui.Controls.Maps.dll
Controls.TestCases.HostApp -> D:\a\1\s\artifacts\bin\Controls.TestCases.HostApp\Debug\net10.0-windows10.0.19041.0\win-x64\Controls.TestCases.HostApp.dll
Build succeeded.
0 Warning(s)
0 Error(s)
Time Elapsed 00:05:47.31
Determining projects to restore...
All projects are up-to-date for restore.
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Graphics -> D:\a\1\s\artifacts\bin\Graphics\Debug\net10.0\Microsoft.Maui.Graphics.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Essentials -> D:\a\1\s\artifacts\bin\Essentials\Debug\net10.0\Microsoft.Maui.Essentials.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Core -> D:\a\1\s\artifacts\bin\Core\Debug\net10.0\Microsoft.Maui.dll
Controls.CustomAttributes -> D:\a\1\s\artifacts\bin\Controls.CustomAttributes\Debug\net10.0\Controls.CustomAttributes.dll
Controls.Core.Design -> D:\a\1\s\artifacts\bin\Controls.Core.Design\Debug\net472\Microsoft.Maui.Controls.DesignTools.dll
Controls.BindingSourceGen -> D:\a\1\s\artifacts\bin\Controls.BindingSourceGen\Debug\netstandard2.0\Microsoft.Maui.Controls.BindingSourceGen.dll
##vso[build.updatebuildnumber]10.0.70-ci+azdo.14031544
Controls.Core -> D:\a\1\s\artifacts\bin\Controls.Core\Debug\net10.0\Microsoft.Maui.Controls.dll
UITest.Core -> D:\a\1\s\artifacts\bin\UITest.Core\Debug\net10.0\UITest.Core.dll
UITest.Appium -> D:\a\1\s\artifacts\bin\UITest.Appium\Debug\net10.0\UITest.Appium.dll
UITest.NUnit -> D:\a\1\s\artifacts\bin\UITest.NUnit\Debug\net10.0\UITest.NUnit.dll
VisualTestUtils -> D:\a\1\s\artifacts\bin\VisualTestUtils\Debug\netstandard2.0\VisualTestUtils.dll
VisualTestUtils.MagickNet -> D:\a\1\s\artifacts\bin\VisualTestUtils.MagickNet\Debug\netstandard2.0\VisualTestUtils.MagickNet.dll
UITest.Analyzers -> D:\a\1\s\artifacts\bin\UITest.Analyzers\Debug\netstandard2.0\UITest.Analyzers.dll
Controls.TestCases.WinUI.Tests -> D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
Test run for D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll (.NETCoreApp,Version=v10.0)
VSTest version 18.0.1 (x64)
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit Adapter 4.5.0.0: Test execution started
Running selected tests in D:\a\1\s\artifacts\bin\Controls.TestCases.WinUI.Tests\Debug\net10.0\Controls.TestCases.WinUI.Tests.dll
NUnit3TestExecutor discovered 1 of 1 NUnit test cases using Current Discovery mode, Non-Explicit run
>>>>> 5/6/2026 9:26:30 PM FixtureSetup for Issue30535(Windows)
>>>>> 5/6/2026 9:26:39 PM RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus Start
>>>>> 5/6/2026 9:26:43 PM RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus Stop
Passed RefreshViewSetIsRefreshingValueinNewPageAndVerifyStatus [4 s]
NUnit Adapter 4.5.0.0: Test execution complete
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.8.2+699d445a1a (64-bit .NET 10.0.0)
[xUnit.net 00:00:00.13] Discovering: Controls.TestCases.WinUI.Tests
[xUnit.net 00:00:00.33] Discovered: Controls.TestCases.WinUI.Tests
Test Run Successful.
Total tests: 1
Passed: 1
Total time: 27.9737 Seconds
📁 Fix files reverted (2 files)
eng/pipelines/ci-copilot.ymlsrc/Core/src/Handlers/RefreshView/RefreshViewHandler.Windows.cs
🧪 UI Tests — Category Detection
Detected UI test categories: RefreshView,ViewBaseTests
🔍 Pre-Flight — Context & Validation
Issue: #30535 - [Windows] RefreshView IsRefreshing property not working while binding
PR: #34845 - [Windows] Fix RefreshView IsRefreshing property not working while binding
Platforms Affected: Windows (primary), test snapshots added for Android, iOS, macOS
Files Changed: 1 implementation, 2 test files, 5 snapshot images
Key Findings
- The
OnLoadedhandler inRefreshViewHandler.Windows.cswas unsubscribing itself after the first fire (refreshControl.Loaded -= OnLoaded). When the user navigates to another page and back, theRefreshContainerfiresLoadedagain but had no handler —UpdateIsRefreshing()was never called. - Fix removes the self-unsubscribe line (2 lines deleted) so the handler persists across navigations.
DisconnectHandlerstill correctly unsubscribes (nativeView.Loaded -= OnLoaded), so no memory leak is introduced.UpdateIsRefreshing()is safe to call multiple times (guards onIsLoaded(),_refreshCompletionDeferral, andVirtualView.IsRefreshing).- Prior reviewer comment about test method name typo ("Valuein" vs "ValueIn") is unresolved.
- Prior reviewer comment about screenshot race condition (no wait for main page after "Apply" tap) is unresolved.
- Prior outdated comments (dead code: clickButton not added to layout, Button_Clicked as dead handler) appear to have been fixed in subsequent commits — those threads are marked outdated.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: high
Errors: 0 | Warnings: 2 | Suggestions: 2
Key code review findings:
⚠️ RefreshViewHandler.Windows.cs:71—UpdateIsRefreshinglacks explicitVirtualViewnull guard; inconsistent withOnRefresh(which guardsVirtualView != null). WithOnLoadednow firing every navigation cycle, the dispatched lambda has a narrow window afterDisconnectHandlerzeroesVirtualView.⚠️ TestCases.Shared.Tests/Tests/Issues/Issue30535.cs:25— Test callsVerifyScreenshot()immediately afterApp.Tap("Apply")without waiting for navigation back + dispatch to complete. The fix relies onRefreshContainer.Loaded→TryEnqueue→UpdateIsRefreshing. Screenshot taken before dispatch completes may pass on unfixed code too.- 💡
TestCases.HostApp/Issues/Issue30535.cs:131—setFalseButtonhas noAutomationId, preventing automation of the reset scenario. - 💡
TestCases.HostApp/Issues/Issue30535.cs:6—PlatformAffected.Alloverstates scope; only Windows handler changed, should bePlatformAffected.UWP.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #34845 | Remove self-unsubscribe from OnLoaded handler |
✅ PASSED (Gate) | RefreshViewHandler.Windows.cs |
Original PR |
🔬 Code Review — Deep Analysis
Code Review — PR #34845
Independent Assessment
What this changes: Removes two lines from OnLoaded in RefreshViewHandler.Windows.cs — the self-unsubscription refreshControl.Loaded -= OnLoaded. This means the handler now stays subscribed to the Loaded event across the entire lifetime of the connected handler (properly cleaned up in DisconnectHandler). It also adds a new UI test (Issue30535) and associated host app, plus snapshot images for all platforms.
Inferred motivation: The Loaded event on WinUI RefreshContainer fires every time the element is loaded into the visual tree — once on initial display and again on each navigation return. The old code unsubscribed after the first firing, so if IsRefreshing was set to true from a secondary page and the user navigated back, UpdateIsRefreshing() was never called for the re-load, leaving no refresh indicator visible.
Reconciliation with PR Narrative
Author claims: "OnLoaded unsubscribed itself after firing once. So when the user navigated back to the page, the Loaded event fired again but had no handler — UpdateIsRefreshing() was never called, and the refresh indicator never appeared."
Agreement: Assessment matches precisely. The root-cause analysis is accurate and the minimal one-line removal is the correct fix. No disagreement.
Findings
⚠️ Warning — UpdateIsRefreshing missing explicit VirtualView null guard
File: src/Core/src/Handlers/RefreshView/RefreshViewHandler.Windows.cs, line 71
With the self-unsubscription removed, OnLoaded now fires on every navigation cycle and dispatches UpdateIsRefreshing via DispatcherQueue.TryEnqueue. There is a window between the enqueue and execution where DisconnectHandler could run, nulling VirtualView on the base class. The method only guards PlatformView:
void UpdateIsRefreshing()
{
if (PlatformView is not { } platform || !platform.IsLoaded())
return;
if (!VirtualView.IsRefreshing) // VirtualView could theoretically be null hereThis is inconsistent with the OnRefresh handler in the same file which explicitly checks:
if (VirtualView != null && !VirtualView.IsRefreshing)Recommended fix:
if (PlatformView is not { } platform || !platform.IsLoaded() || VirtualView is null)
return;This is a defensive hardening concern — it won't reproduce reliably in practice because the base class zeros both fields together — but the inconsistency with OnRefresh is worth closing, especially now that OnLoaded fires more frequently.
⚠️ Warning — Test has no stable wait between App.Tap("Apply") and VerifyScreenshot()
File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue30535.cs, line 25
App.Tap("Apply") triggers Navigation.PopAsync() — an async operation. The entire fix mechanism requires:
- Navigation back completes
RefreshContainer.LoadedfiresDispatcherQueue.TryEnqueue(UpdateIsRefreshing)executes
Without a WaitForElement call after App.Tap("Apply"), the screenshot can land during the slide-back animation, before the dispatched callback has run. This makes the test pass trivially on the unfixed code too, since the refresh spinner hasn't had a chance to appear.
Suggested fix:
App.Tap("Apply");
App.WaitForElement("BoxContent"); // wait for main page to be fully visible
VerifyScreenshot();Alternatively use the retry overload seen in other tests in this suite:
VerifyScreenshot(retryDelay: TimeSpan.FromSeconds(2));💡 Suggestion — setFalseButton has no AutomationId
File: src/Controls/tests/TestCases.HostApp/Issues/Issue30535.cs, line 131
The "Set IsRefreshing = False" button is central to the two-way binding scenario (resetting the refresh state), but it cannot be referenced in automated tests. Adding AutomationId = "SetIsRefreshingFalse" now avoids a follow-up PR if a test for the reset direction is ever needed.
💡 Suggestion — PlatformAffected.All overstates the fix scope
File: src/Controls/tests/TestCases.HostApp/Issues/Issue30535.cs, line 6
The only modified production file is RefreshViewHandler.Windows.cs. The [Issue] attribute metadata would be more accurate as PlatformAffected.UWP (the closest Windows-targeted value in the enum). This is metadata-only and does not affect test execution; it's a minor triage aid.
Devil's Advocate
On removing the self-unsubscription: Could Loaded fire in rapid succession in a single navigation cycle? In WinUI, Loaded fires once per visual-tree insertion. The DispatcherQueue.TryEnqueue dispatch means UpdateIsRefreshing runs asynchronously, but the _refreshCompletionDeferral is null guard makes it idempotent: only the first call to UpdateIsRefreshing when IsRefreshing == true will call RequestRefresh(); subsequent calls are no-ops. No duplicate refresh triggers.
On the null-guard concern: PlatformView and VirtualView are both nulled inside base.DisconnectHandler, and ConnectHandler subscribes to Loaded. In practice the null check on PlatformView effectively covers VirtualView being null as well, since both are managed by the same lifecycle. However, OnRefresh guards both explicitly — the inconsistency is real, even if the risk is low in practice.
On the test race: The risk is real on slow CI machines. Similar tests in this suite (e.g. Issue18751) do call VerifyScreenshot() after taps without intermediate waits, suggesting this is a common but not universal pattern. Given the PR fix specifically relies on a deferred dispatch, the missing wait here is more than a style concern.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The core production fix is correct — removing the self-unsubscription is the right solution, event subscriptions are balanced, and UpdateIsRefreshing is idempotent so multiple Loaded firings are safe. All CI checks pass. However, the new UI test lacks a WaitForElement synchronization point after navigation, which means it can pass on both the old and new code depending on timing. That test robustness issue and the missing defensive VirtualView null guard should be addressed before merge.
🔧 Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix-1 | Unloaded/Loaded cycling — OnLoaded one-shot per cycle, OnUnloaded re-subscribes for next navigation | ✅ PASS | 1 file (+15/-1) | 0 reviewer findings; preserves one-shot semantics |
| 2 | try-fix-2 | Mapper-driven one-shot — UpdateIsRefreshing subscribes OnDeferredIsRefreshing when IsRefreshing=true + not loaded |
✅ PASS | 1 file (+26/-4) | 0 reviewer findings; fully deferred from mapper |
| 3 | try-fix-3 | Unload-cycle re-arm + connect-time dispatch — like try-fix-1 but also dispatches UpdateIsRefreshing from ConnectHandler if already loaded | ✅ PASS | 1 file | 0 reviewer findings; handles already-loaded case at connect |
| 4 | try-fix-4 | Deferred UpdateIsRefreshing from not-loaded path via one-shot replay | ❌ FAIL | 1 file | App hung on iteration 1; locked build output on iteration 3 |
| PR | PR #34845 | Remove refreshControl.Loaded -= OnLoaded (handler stays permanently subscribed) |
✅ PASSED (Gate) | 1 file (-2 lines) | Simplest fix; missing VirtualView null guard + test wait |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | UpdateLayout() synchronous — remove IsLoaded guard, force layout before RequestRefresh. Not pursued: risky (undocumented side effects on template application). |
| claude-sonnet-4.6 | 2 | Yes | _pendingRefresh flag — store pending state in UpdateIsRefreshing, check in OnLoaded. Not pursued: still requires persistent OnLoaded (same as PR fix); no net advantage. |
| gpt-5.3-codex | 2 | Yes | SizeChanged/LayoutUpdated event as readiness gate. Not pursued: overkill — ActualWidth == 0 is not the root cause; Loaded event timing is the real issue. |
| gpt-5.4 | 2 | Yes | SizeChanged gate (similar to gpt-5.3-codex idea). Not pursued: same reasoning. |
| All | 3 | NO NEW IDEAS | Second cross-pollination: models converge — no substantially new approaches identified. |
Exhausted: Yes
Selected Fix: PR's fix is the frontrunner for simplicity (2-line deletion). All try-fix alternatives pass but add more code complexity. pr-plus-reviewer improves the PR fix with VirtualView null guard + test synchronization.
📋 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Phase Status
| Phase | Status | Notes |
|---|---|---|
| Pre-Flight | ✅ COMPLETE | Issue #30535, 1 production file + 2 test files |
| Code Review | NEEDS_CHANGES (high) | 0 errors, 2 warnings, 2 suggestions |
| Gate | ✅ PASSED | windows |
| Try-Fix | ✅ COMPLETE | 4 attempts (3 passing, 1 failing) + cross-pollination |
| Report | ✅ COMPLETE |
Code Review Impact on Try-Fix
The code review's VirtualView null guard (line 71) was incorporated as a hint into all 4 try-fix attempts — every passing candidate added this guard. The screenshot-race warning informed the pr-plus-reviewer candidate's App.WaitForElement("BoxContent") addition. The NEEDS_CHANGES verdict confirms pr-plus-reviewer is the correct winning candidate over the raw PR.
Summary
The core production fix (removing refreshControl.Loaded -= OnLoaded) is correct and minimal. The code review identified two VirtualView null guard in UpdateIsRefreshing (inconsistent with OnRefresh) and a screenshot race in the new test. The pr-plus-reviewer candidate addresses both without changing the fix's fundamental approach. Three independent try-fix alternatives also passed but all added 4–9× more code complexity than the PR's 2-line deletion. The PR's fix plus the two reviewer improvements is the winning candidate.
Root Cause
RefreshViewHandler.Windows.cs subscribed OnLoaded in ConnectHandler but self-unsubscribed inside the handler. On first navigation, Loaded fired and called UpdateIsRefreshing. On navigation back, Loaded fired again but no handler was registered — UpdateIsRefreshing was never called, so the refresh indicator never appeared.
Fix Quality
Winner: pr-plus-reviewer — PR's fix (remove self-unsubscription: -2 lines) + reviewer improvements (+2 lines):
|| VirtualView is nullguard inUpdateIsRefreshing— closes null-deref window betweenTryEnqueuedispatch and execution afterDisconnectHandlerApp.WaitForElement("BoxContent")afterApp.Tap("Apply")— ensures navigation back + dispatch completes before screenshot
Candidate comparison:
| Candidate | Test | Complexity | Notes |
|---|---|---|---|
| pr | ✅ Gate PASS | -2 lines | Missing null guard + test race |
| pr-plus-reviewer | ✅ Gate PASS (inferred) | -2 +2 lines | Fixes both warnings; WINNER |
| try-fix-1 | ✅ PASS | +15/-1 lines | Correct but 8× more code |
| try-fix-2 | ✅ PASS | +26/-4 lines | Correct but 15× more code |
| try-fix-3 | ✅ PASS | +18 lines | Correct but 9× more code |
| try-fix-4 | ❌ FAIL | N/A | App hung; approach unstable |
The recommendation is REQUEST CHANGES per the code review hard gate (NEEDS_CHANGES verdict), asking the PR author to apply the two reviewer improvements. No production logic change is needed beyond what the PR already does.
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 4 findings
See inline comments for details.
| App.Tap("SetIsRefreshingTrue"); | ||
| App.WaitForElement("Apply"); | ||
| App.Tap("Apply"); | ||
| VerifyScreenshot(); |
There was a problem hiding this comment.
VerifyScreenshot() called immediately after async navigation
App.Tap("Apply") triggers Navigation.PopAsync() — an async operation. The fix mechanism requires: (1) navigation back completes, (2) RefreshContainer.Loaded fires, (3) DispatcherQueue.TryEnqueue(UpdateIsRefreshing) executes. Without a stable wait, the screenshot can land during the slide-back animation before the dispatched callback has run — causing the test to pass on the unfixed code too (no spinner visible yet).
Add a wait for a stable main-page element:
App.Tap("Apply");
App.WaitForElement("BoxContent"); // wait for main page fully loaded
VerifyScreenshot();Or use the retry overload:
VerifyScreenshot(retryDelay: TimeSpan.FromSeconds(2));| }; | ||
| setTrueButton.Clicked += OnSetTrueClicked; | ||
|
|
||
| var setFalseButton = new Button |
There was a problem hiding this comment.
💡 Suggestion — setFalseButton missing AutomationId
The "Set IsRefreshing = False" button is key to the two-way binding scenario (resetting the refresh state from a bound property), but without an AutomationId it cannot be tapped from UI tests. Adding one now avoids a follow-up change if a reset-direction test is ever added:
var setFalseButton = new Button
{
Text = "Set IsRefreshing = False",
AutomationId = "SetIsRefreshingFalse",
BackgroundColor = Colors.Red,
TextColor = Colors.White
};|
|
||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 30535, "[Windows] RefreshView IsRefreshing property not working while binding", PlatformAffected.All)] |
There was a problem hiding this comment.
💡 Suggestion — PlatformAffected.All overstates the fix scope
The only modified production file is RefreshViewHandler.Windows.cs. The [Issue] attribute would be more accurate as PlatformAffected.UWP (the closest Windows-targeted value in the enum). This is metadata-only and does not affect test execution, but helps future triage correctly identify the platform scope:
[Issue(IssueTracker.Github, 30535,
"[Windows] RefreshView IsRefreshing property not working while binding",
PlatformAffected.UWP)]
kubaflo
added
the
s/agent-fix-implemented
Issue Details
When set IsRefreshing = true in navigated page, refreshview is not visible
Root Cause
OnLoaded unsubscribed itself after firing once (refreshControl.Loaded -= OnLoaded). So when the user navigated
back to the page, the Loaded event fired again but had no handler — UpdateIsRefreshing() was never called, and
the refresh indicator never appeared.
Description of Change
Removed Self-unsubscribe line refreshControl.Loaded -= OnLoaded so the handler stays active across.
Issues Fixed
Fixes #30535
Tested the behavior in the following platforms.
Before-demo.mp4
After-Demo.mp4