[devanathan-vaithiyanathan]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Issue Details

Issue Details
When a VisualStateGroup is replaced through the VisualStateGroupList indexer, the existing implementation only updates the internal list:

visualStateGroups[index] = replacementGroup;

The replacement bypasses the lifecycle behavior used by Add, Insert, and Remove:

• The old group remains subscribed to StatesChanged.
• The replacement group is not subscribed to StatesChanged.
• Group and state names are not revalidated.
• State triggers are not updated for the replacement.
• The associated VisualElement is not notified to reevaluate its visual state.

Description of Change

• Updated the VisualStateGroupList indexer setter to correctly manage group replacement:
• Reject null replacement values with ArgumentNullException.
• Unsubscribe ValidateAndNotify from the replaced group.
• Replace the group in the internal collection.
• Subscribe ValidateAndNotify to the new group.
• Explicitly call ValidateAndNotify(_internalList) after replacement.

Issues Fixed

Fixes #37171

Tested the behavior in the following platforms.

• Android
• Windows
• iOS
• Mac

Before	After
Android	Android

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37340

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37340"

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

Fixes a VisualStateManager lifecycle bug where replacing a VisualStateGroup via VisualStateGroupList’s indexer bypassed the normal bookkeeping (validation, event subscriptions, trigger updates, and visual-state re-evaluation), leading to stale StatesChanged subscriptions and potential memory leaks.

Changes:

• Updated VisualStateGroupList.this[int].set to reject null, unsubscribe from the replaced group, subscribe the new group, and run ValidateAndNotify to revalidate and trigger ChangeVisualState.
• Added a unit test covering visual-state re-evaluation after index replacement and verifying the replacement group participates in validation via StatesChanged.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Controls/src/Core/VisualStateManager.cs	Implements correct unsubscribe/subscribe + validation/notification path for indexer replacements in VisualStateGroupList.
src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs	Adds a regression test asserting index replacement re-applies visual state and that the new group is wired for validation notifications.

[MauiBot]

Expert Review — 5 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness — The replaced group's currently-applied setters are never un-applied. GoToState only calls setter.UnApply(...) for group.CurrentState of groups still present in the list (see GoToState, this file), and the subsequent ValidateAndNotify -> OnStatesChanged -> VisualElement.ChangeVisualState() iterates the new list, so oldGroup (now detached) never gets its active state un-applied. Concrete scenario: group 0's Normal state sets BackgroundColor=Red and Margin=1; groups[0] = replacement where the replacement's Normal sets only Margin=2. After the assignment Margin becomes 2 but BackgroundColor stays Red permanently at VSM specificity, because nothing ever un-applies the removed group's setters. Un-apply oldGroup.CurrentState's setters (using the same specificity GoToState applies) and clear oldGroup.CurrentState/oldGroup.VisualElement before installing the replacement.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — The -=/+= pair is unconditional and does not account for the same VisualStateGroup instance appearing at more than one index. Two concrete cases: (1) groups[1] = groups[0] subscribes ValidateAndNotify to that instance a second time, so every later States mutation on it runs Validate + OnStatesChanged (and therefore ChangeVisualState) twice; (2) if oldGroup is also stored at another index, oldGroup.StatesChanged -= ValidateAndNotify removes the only handler, so the surviving copy's later States mutations are no longer validated or propagated to the element. Guard with if (ReferenceEquals(oldGroup, value)) return; and only unsubscribe/subscribe when the instance is not otherwise present in _internalList.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — ValidateAndNotify runs after the list and both subscriptions have already been mutated, and it can throw. Concrete scenario: groups[0] contains a state named Normal; assigning groups[1] = groupWithNormalState reaches Validate, which throws InvalidOperationException("VisualState Names must be unique"). The exception escapes the setter with the invalid group already installed at index 1, oldGroup already unsubscribed, and the new group already subscribed — a caller that catches the exception is left with a corrupted list rather than the pre-assignment state. Validate the candidate list before committing the swap (or restore oldGroup and its subscription in a catch/finally) so the setter is atomic. Note this is a new failure mode for this indexer: before this change the assignment was never validated.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[major] Regression Prevention and Test Coverage — This test does not prove the stale-subscription half of the fix. Assert.Throws<InvalidOperationException> on replacementGroup.States.Add(...) only exercises the new subscription added on line 426; deleting line 424 (oldGroup.StatesChanged -= ValidateAndNotify) leaves this test fully green, so the regression it is supposed to lock in is untested. Add the negative assertion the guard exists for: keep a reference to the original group, and after groups[0] = replacementGroup assert that mutating the detached group (e.g. originalGroup.States.Add(new VisualState { Name = "Foo" })) neither throws the uniqueness InvalidOperationException nor changes label.Margin — that is the only assertion that fails if the -= is removed. The new ArgumentNullException path on lines 420-421 is likewise uncovered.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — The fixture conflates two signals: the original and replacement states both set the same property (View.MarginProperty), so Assert.Equal(replacementMargin, label.Margin) cannot distinguish "replacement setters were applied" from "the removed group's setters were correctly un-applied" — the replacement's Apply at the same specificity simply overwrites the stale value. This is exactly why the leaked-setter behavior flagged on VisualStateManager.cs:425 is invisible here. Give the original group a setter for a property the replacement does not set (e.g. original sets BackgroundColor + Margin, replacement sets only Margin) and assert that property returns to its default after the swap.

[MauiBot]

AI Review Summary

@devanathan-vaithiyanathan — new AI review results are available based on commit 2d0cb8e.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: IOS · Base: main · Merge base: f377ff1c

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 VisualStateManagerTests VisualStateManagerTests	✅ FAIL — 33s	✅ PASS — 17s

🔴 Without fix — 🧪 VisualStateManagerTests: FAIL ✅ · 33s

Error-relevant lines (filtered from the build log):

     at Microsoft.Maui.Controls.Core.UnitTests.VisualStateManagerTests.ReplacingGroupUpdatesVisualStateAndValidationSubscription() in /_/src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs:line 200
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

🟢 With fix — 🧪 VisualStateManagerTests: PASS ✅ · 17s

(no coded error found; showing last 1200 chars)

placesExisting [< 1 ms]
  Skipped ValidatePerformance [1 ms]
  Passed InitialStateIsNullIfNormalNotAvailable [< 1 ms]
  Passed StateNamesMustBeUniqueWithinGroup [< 1 ms]
  Passed IsEnabledToggleWhileSelectedPreservesState [1 ms]
  Passed SelectHoverDeselectRestoresPointerOverState [< 1 ms]
  Passed StateNamesMustBeUniqueWithinGroupList [< 1 ms]
  Passed VisualStateGroupsFromSettersAreDistinct [< 1 ms]
  Passed ImplicitStyleDisabledVSMOverridesLocalValue [< 1 ms]
  Passed VisualElementGoesToCorrectStateWhenAvailable [< 1 ms]
  Passed SelectingWhileDisabledStaysDisabledUntilReEnabled [< 1 ms]
  Passed StateNamesInGroupMayNotBeEmpty [< 1 ms]
  Passed CanRemoveAStateAndAddANewStateWithTheSameName [< 1 ms]
  Passed VSMFromStyleAreUnApplied [< 1 ms]
  Passed UnapplyingVSMShouldUnapplySetters [< 1 ms]
  Passed StateNamesInGroupMayNotBeNull [< 1 ms]
[xUnit.net 00:00:00.60]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed VerifyVisualStateChanges [< 1 ms]
  Passed AssigningSameIsItemSelectedValueIsNoOp [< 1 ms]
  Passed VisualElementGoesToCorrectStateWhenAvailableFromSetter [< 1 ms]
Test Run Successful.
Total tests: 30
     Passed: 29
    Skipped: 1
 Total time: 0.8447 Seconds

📁 Fix files reverted (1 files)

• src/Controls/src/Core/VisualStateManager.cs

---

📋 Pre-Flight — Context & Validation

PR #37340 Pre-Flight

Context

• PR: Fix stale StatesChanged subscription when replacing VisualStateGroupList item
• Issue: #37171, a managed memory leak caused by indexer replacement retaining the old VisualStateGroupList through the old group's StatesChanged delegate.
• Platform requested: iOS. The affected path is platform-independent managed code and is covered by a net10.0 unit test.
• Base / review commit: origin/main at f377ff1c5e; local squashed PR commit 1e0843ba61.

Existing PR Approach

The PR changes VisualStateGroupList.this[int] to reject null, unsubscribe the replaced group's StatesChanged handler, assign the replacement, subscribe the replacement, and call ValidateAndNotify(_internalList). It adds ReplacingGroupUpdatesVisualStateAndValidationSubscription to verify visual-state reevaluation and validation subscription.

Changed files:

• src/Controls/src/Core/VisualStateManager.cs
• src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs

Validation Scope

The prior gate already proved the detected tests fail without the fix and pass with it; it must not be rerun or overwritten.

Use only this class-scoped test command for each candidate:

dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "VisualStateManagerTests"

This runs the detected primary regression and the existing VisualStateManagerTests regressions (30 tests in the gate, 29 passed and 1 skipped). Do not run a broader suite or device tests.

Try-Fix Constraints

• Each candidate must differ from the PR's direct unsubscribe/assign/subscribe/notify implementation and from prior candidates.
• One implementation/test pass is allowed, plus at most one focused correction/retest.
• Preserve all unrelated pre-existing working-tree changes.
• Use EstablishBrokenBaseline.ps1 for baseline/restore as required by the try-fix skill.
• Perform the skill's expert self-review inline; do not launch another reviewer agent.

---

🔬 Code Review — Deep Analysis

Expert PR Evaluation — PR #37340

Verdict: NEEDS_CHANGES

Confidence: high

The submitted setter correctly adds the missing null guard and transfers the StatesChanged subscription, but it does not complete the lifecycle of the group being replaced. Once the old group is removed from the list, the normal GoToState path can no longer unapply its active setters; any old-state property not also set by the replacement remains applied.

Findings

1. Major — stale active setters: Replacing an active group does not unapply the old group's current-state setters or clear its visual-element/current-state association.
2. Major — regression gap: The new test proves notification and subscription of the replacement group, but it remains green if the old group's stale subscription is never removed.
3. Moderate — conflated fixture: Both groups set Margin, allowing the replacement to overwrite that value while hiding unrelated setters left behind by the old group.
4. Moderate — failed validation is non-atomic: Duplicate-name validation occurs after the list and subscriptions have changed, so a thrown exception leaves the rejected replacement installed.
5. Moderate — repeated group references: Per-index subscription bookkeeping can produce duplicate callbacks for the same group instance. Subscription changes should account for whether that instance remains elsewhere in the list.

Failure-Mode Probing

• Replacing a group whose active state sets BackgroundColor and Margin with one that sets only Margin leaves the old BackgroundColor applied.
• Mutating the detached original group is not covered, so the test does not prove that the memory-retaining delegate was removed.
• Assigning a group whose state name duplicates another group's state throws only after mutating the collection.
• Null replacement is rejected, but that new contract has no direct test.

Evidence

The trusted gate passed its fail-without/pass-with-fix check. Required-check and prior-review GitHub surfaces could not be queried because gh is unauthenticated; this does not alter the blocking code findings above. regression-check/risks.json is CLEAN.

---

🛠️ Try-Fix — Analysis & Comparison

PR #37340 Alternative Fix Candidates

Candidate 1 — Subscription Reconciliation + Notify-on-Replace

Model: claude-opus-5
Result: Pass

The candidate treated the leak as a structural mutation-funnel problem. It changed WatchAddList<T>.this[int] to invoke its mutation callback and added VisualStateGroupList.SyncSubscriptions() to reconcile tracked StatesChanged subscriptions against current list contents. Existing per-mutator subscription bookkeeping was removed, and the VisualStateGroupList indexer became a null guard plus assignment rather than the PR's direct unsubscribe/assign/subscribe/notify sequence.

Diff scope: src/Controls/src/Core/VisualStateManager.cs (+55/-13); no test changes.

Test: dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "VisualStateManagerTests"

Test result: 30 total; 29 passed, 1 skipped, 0 failed. One implementation pass; no correction/retest.

Inline self-review: 2 findings, neither critical nor major. The moderate finding notes the broader behavior change from making WatchAddList<T> index replacement notify for other users; the minor finding notes the reconciliation's O(n*m) cost.

Analysis: The primary regression passes and subscription ownership is centralized, but this alternative is substantially broader than the PR fix and changes shared WatchAddList<T> indexer behavior. Complete narrative and diff: ../try-fix-1/content.md. Mandatory skill artifacts: attempt-1/.

Candidate 2 — Compose Existing Remove/Insert Mutators

Model: gpt-5.6-sol
Result: Pass

The candidate replaced the indexer assignment with a null check followed by RemoveAt(index) and Insert(index, value). This delegates unsubscription, subscription, validation, and notification to the collection's existing mutation paths. It differs from the PR's duplicated inline event bookkeeping and from candidate 1's shared WatchAddList<T> behavior change and subscription-reconciliation layer.

Diff scope: src/Controls/src/Core/VisualStateManager.cs (+7/-1); no test changes.

Test: dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "VisualStateManagerTests"

Test result: 30 total; 29 passed, 1 skipped, 0 failed. One implementation pass; no correction/retest.

Inline self-review: Clean, 0 findings.

Analysis: This is the smallest alternative and reuses established mutator behavior rather than duplicating subscription mechanics. Complete narrative and diff: ../try-fix-2/content.md. Mandatory skill artifacts: attempt-2/.

---

🏁 Report — Final Recommendation

⚠️ Final Recommendation: REQUEST CHANGES

Winner: pr

The submitted PR is the strongest candidate that completed its required regression run: it is narrowly scoped, its trusted gate proves the detected test fails without the fix and passes with it, and both alternatives retain the expert review's unresolved lifecycle/atomicity gaps. It is not ready for approval, however, because the expert review found blocking behavior and coverage errors that none of the validation-passing candidates resolve.

Rank	Candidate	Targeted validation	Assessment
1	pr	Pass: 29 passed, 1 skipped	Smallest direct fix with gate evidence. Still leaves active setters and attached triggers from the removed group, validates only after mutation, and does not test detachment of the old StatesChanged handler.
2	try-fix-2	Pass: 29 passed, 1 skipped	Reuses RemoveAt/Insert, but those mutators do not clean up active setters/triggers and Insert still validates after mutation. It therefore preserves the raw PR's blocking adjacent behavior without a material safety gain.
3	try-fix-1	Pass: 29 passed, 1 skipped	Reconciles subscriptions robustly, but broadens WatchAddList<T> replacement behavior for every consumer, adds reconciliation complexity, and still does not unapply the removed group's active state or make validation atomic.
4	pr-plus-reviewer	Fail: 30 passed, 1 skipped, 1 failed	The implementation addresses the expert lifecycle and atomicity findings and adds discriminating coverage, but its new atomicity test uses two unnamed groups and fails during setup under the existing uniqueness contract. Per the hard ranking rule, it is below every passing candidate and was not repaired or rerun.

Required changes

1. Complete replacement lifecycle: detach old state triggers, unapply active old-state setters at the original specificity, clear stale state association, and attach replacement triggers for an already-attached element.
2. Validate the proposed replacement before committing list and subscription changes.
3. Add a regression assertion that fails when the old group's handler remains subscribed; use a distinct old-only setter to expose stale applied values.
4. Cover the new null-replacement contract.

The trusted Gate passed for the submitted regression. GitHub required-check and prior-review surfaces were unavailable because gh is unauthenticated, so no approval can be inferred from remote CI.

---

📱 UI Tests — VisualStateManager

Detected UI test categories: VisualStateManager

✅ Deep UI tests — 112 passed, 0 failed, 4 skipped across 1 category on platform-pool agent (replaces in-process counts above).

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
VisualStateManager	112/116 (4 skipped) ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

---

🧭 Next Steps — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.