fix(reconciler): reconcile Flyout for DropDownButton + SplitButton + ToggleSplitButton#385
Open
arcadiogarcia wants to merge 1 commit into
Conversation
…ToggleSplitButton Mount wrote .Flyout for all three controls, but Update never touched it, so data-driven flyout content (e.g. a MenuFlyout whose Items list is state-derived, or a ContentFlyoutElement whose subtree depends on state) stayed frozen at mount-time across re-renders. A button whose flyout transitioned to null across renders also kept the stale flyout attached. UpdateDropDownButton, UpdateSplitButton, and UpdateToggleSplitButton now call ApplyFlyoutAttachment for Flyout, mirroring the Mount-side wiring and the universal AttachedFlyout / ContextFlyout path already taken by ApplyModifiers. ApplyFlyoutAttachment reuses the realized FlyoutBase (so an already-open flyout stays open), clear+repopulates MenuFlyout.Items, or reconciles content inside a Flyout.Content child via UpdateFlyoutInPlace. UpdateDropDownButton also gains an `o` parameter (the dispatch tuple was previously discarding it) and all three now take requestRerender. The X-to-null case is handled with an explicit `.Flyout = null` branch so detached flyouts no longer linger. Adds FlyoutReconcileFixtures with six selftests that mutate flyout content / item count / subtree across re-renders and assert the rendered flyout reflects the new value (and the realized FlyoutBase instance is preserved), plus a clear-on-null selftest. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a reconciliation gap in Reactor’s WinUI reconciler so .Flyout on DropDownButton, SplitButton, and ToggleSplitButton is updated across re-renders (instead of being “mount-only”), and adds selftest coverage to prevent regressions.
Changes:
- Update
UpdateDropDownButton,UpdateSplitButton, andUpdateToggleSplitButtonto threadold+requestRerenderand reconcile.FlyoutviaApplyFlyoutAttachment, including X→null clearing. - Add
FlyoutReconcileFixturesselftests to validate flyout content/items update across re-renders and flyout instances are preserved. - Register the new fixtures in
SelfTestFixtureRegistryand document the fix inCHANGELOG.md.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs | Registers the new flyout reconciliation selftests in the selftest registry. |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/FlyoutReconcileFixtures.cs | Adds new fixtures covering flyout text/content/items updates and X→null flyout clearing. |
| src/Reactor/Core/Reconciler.Update.cs | Updates button reconciler paths to reconcile .Flyout during Update (not just Mount). |
| CHANGELOG.md | Documents the reconciler fix and the added regression tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
Comment on lines
+743
to
+746
| if (n.Flyout is not null) | ||
| ApplyFlyoutAttachment(ddb, o.Flyout, n.Flyout, requestRerender); | ||
| else if (o.Flyout is not null) | ||
| ddb.Flyout = null; |
Comment on lines
+756
to
+759
| if (n.Flyout is not null) | ||
| ApplyFlyoutAttachment(sb, o.Flyout, n.Flyout, requestRerender); | ||
| else if (o.Flyout is not null) | ||
| sb.Flyout = null; |
Comment on lines
+780
to
+783
| if (n.Flyout is not null) | ||
| ApplyFlyoutAttachment(tsb, o.Flyout, n.Flyout, requestRerender); | ||
| else if (o.Flyout is not null) | ||
| tsb.Flyout = null; |
Comment on lines
+1
to
+8
| using Microsoft.UI.Reactor.AppTests.Host.SelfTest; | ||
| using Microsoft.UI.Reactor.Core; | ||
| using Microsoft.UI.Xaml; | ||
| using Microsoft.UI.Xaml.Controls; | ||
| using static Microsoft.UI.Reactor.Factories; | ||
| using WinPrim = Microsoft.UI.Xaml.Controls.Primitives; | ||
| using WinUI = Microsoft.UI.Xaml.Controls; | ||
|
|
codemonkeychris
requested changes
May 23, 2026
Collaborator
codemonkeychris
left a comment
codemonkeychris
left a comment
There was a problem hiding this comment.
please address the CR feedback from AIs, i ran 3 different robot code reviews against this, and did a human spot check, the combined feedback is all inline. just have your robot action this, and then you are clear to merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mount wrote .Flyout for all three controls, but Update never touched it,
so data-driven flyout content (e.g. a MenuFlyout whose Items list is
state-derived, or a ContentFlyoutElement whose subtree depends on state)
stayed frozen at mount-time across re-renders. A button whose flyout
transitioned to null across renders also kept the stale flyout attached.
UpdateDropDownButton, UpdateSplitButton, and UpdateToggleSplitButton now
call ApplyFlyoutAttachment for Flyout, mirroring the Mount-side wiring
and the universal AttachedFlyout / ContextFlyout path already taken by
ApplyModifiers. ApplyFlyoutAttachment reuses the realized FlyoutBase
(so an already-open flyout stays open), clear+repopulates
MenuFlyout.Items, or reconciles content inside a Flyout.Content child
via UpdateFlyoutInPlace. UpdateDropDownButton also gains an
oparameter(the dispatch tuple was previously discarding it) and all three now take
requestRerender. The X-to-null case is handled with an explicit
.Flyout = nullbranch so detached flyouts no longer linger.Adds FlyoutReconcileFixtures with six selftests that mutate flyout
content / item count / subtree across re-renders and assert the
rendered flyout reflects the new value (and the realized FlyoutBase
instance is preserved), plus a clear-on-null selftest.
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com