Fix GeometryGroup.Children.Clear() leaking shared child geometries

[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

GeometryGroup.Children.Clear() triggers a Reset event, but Reset does not provide OldItems, so removed geometries were not unsubscribed from PropertyChanged. This could leave stale event handlers and cause object retention.

Description of Change

The fix in GeometryGroup.cs tracks subscribed children, handles Reset explicitly by unsubscribing all previously tracked children, and then re-subscribes only the current collection items and also add a logic improvements.

Issues Fixed

Fixes #35795

Tested the behavior in the following platforms.

• Android
• Windows
• iOS
• Mac

Before	After
iOS Before.mov	iOS After.mov

[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 -- 36232

Or

• Run remotely in PowerShell:

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

[Copilot]

Pull request overview

Fixes a retention/leak scenario in GeometryGroup where Children.Clear() raises a collection Reset without OldItems, leaving removed child geometries still subscribed to PropertyChanged and potentially retaining visual trees.

Changes:

• Refactors GeometryGroup child subscription management by tracking subscribed children and explicitly handling NotifyCollectionChangedAction.Reset.
• Adds a unit test ensuring Clear() unsubscribes previous children so their later mutations don’t invalidate the group.

Reviewed changes

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

File	Description
src/Controls/src/Core/Shapes/GeometryGroup.cs	Adds subscription tracking and Reset handling to prevent stale PropertyChanged subscriptions after Children.Clear().
src/Controls/tests/Core.UnitTests/GeometryGroupTests.cs	Adds coverage validating that cleared children no longer trigger invalidation.

[kubaflo]

Could you please try the ai's suggestions?

[MauiBot]

Expert Review — 1 findings

See inline comments for details.

[MauiBot]

🔍 AI-Generated Review (multi-model)

[moderate] Regression Prevention and Test Coverage — A prior review comment on this PR explicitly asked for "a unit test covering duplicate add/remove" after the fix moved from a HashSet to a ref-counted Dictionary<Geometry,int> specifically to handle the same Geometry instance being added to Children more than once. That code change was made in GeometryGroup.cs, but no test for it was added here — the only test covers a single non-duplicate child through a Clear()/Reset cycle. Concrete failing scenario the current suite would miss if the ref-counting regressed: add the same Geometry instance twice, RemoveAt one occurrence, then mutate the geometry — it should still raise InvalidateGeometryRequested (subscription is only removed once the ref count reaches 0), and a Replace that substitutes one duplicate for another already-present instance should also keep the surviving subscription intact. Please add these cases so the ref-counting behavior — and the exact scenario in the linked issue title, "leaking shared child geometries" — is regression-tested, not just the simple Clear() path.

[MauiBot]

AI Review Summary

@devanathan-vaithiyanathan — new AI review results are available based on this last commit: f038788. To request a fresh review after new comments or commits, comment /review rerun.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ✅ PASSED

Platform: ANDROID · Base: main · Merge base: 39af9158

Test	Without Fix (expect FAIL)	With Fix (expect PASS)
🧪 GeometryGroupTests GeometryGroupTests	✅ FAIL — 106s	✅ PASS — 65s

🔴 Without fix — 🧪 GeometryGroupTests: FAIL ✅ · 106s

  Determining projects to restore...
  Restored /home/vsts/work/1/s/src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj (in 6.47 sec).
  Restored /home/vsts/work/1/s/src/Controls/Maps/src/Controls.Maps.csproj (in 10.01 sec).
  Restored /home/vsts/work/1/s/src/Controls/src/Xaml/Controls.Xaml.csproj (in 1.85 sec).
  Restored /home/vsts/work/1/s/src/TestUtils/src/TestUtils/TestUtils.csproj (in 5 ms).
  Restored /home/vsts/work/1/s/src/Controls/src/Core/Controls.Core.csproj (in 52 ms).
  Restored /home/vsts/work/1/s/src/Graphics/src/Graphics/Graphics.csproj (in 10 ms).
  Restored /home/vsts/work/1/s/src/Essentials/src/Essentials.csproj (in 23 ms).
  Restored /home/vsts/work/1/s/src/Core/maps/src/Maps.csproj (in 26 ms).
  Restored /home/vsts/work/1/s/src/Core/src/Core.csproj (in 65 ms).
  1 of 10 projects are up-to-date for restore.
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
  Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
  Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
  TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
  Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.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.
[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.15]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.30]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.31]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:01.37]     ClearUnsubscribesPreviousChildrenFromPropertyChanged [FAIL]
[xUnit.net 00:00:01.37]       Assert.Equal() Failure: Values differ
[xUnit.net 00:00:01.37]       Expected: 0
[xUnit.net 00:00:01.37]       Actual:   1
[xUnit.net 00:00:01.37]       Stack Trace:
[xUnit.net 00:00:01.37]         /_/src/Controls/tests/Core.UnitTests/GeometryGroupTests.cs(27,0): at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupTests.ClearUnsubscribesPreviousChildrenFromPropertyChanged()
[xUnit.net 00:00:01.37]            at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
[xUnit.net 00:00:01.37]            at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:01.38]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Failed ClearUnsubscribesPreviousChildrenFromPropertyChanged [10 ms]
  Error Message:
   Assert.Equal() Failure: Values differ
Expected: 0
Actual:   1
  Stack Trace:
     at Microsoft.Maui.Controls.Core.UnitTests.GeometryGroupTests.ClearUnsubscribesPreviousChildrenFromPropertyChanged() in /_/src/Controls/tests/Core.UnitTests/GeometryGroupTests.cs:line 27
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Test Run Failed.
Total tests: 1
     Failed: 1
 Total time: 1.9230 Seconds

🟢 With fix — 🧪 GeometryGroupTests: PASS ✅ · 65s

  Determining projects to restore...
  All projects are up-to-date for restore.
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Graphics -> /home/vsts/work/1/s/artifacts/bin/Graphics/Debug/net10.0/Microsoft.Maui.Graphics.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Essentials -> /home/vsts/work/1/s/artifacts/bin/Essentials/Debug/net10.0/Microsoft.Maui.Essentials.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Core -> /home/vsts/work/1/s/artifacts/bin/Core/Debug/net10.0/Microsoft.Maui.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.BindingSourceGen -> /home/vsts/work/1/s/artifacts/bin/Controls.BindingSourceGen/Debug/netstandard2.0/Microsoft.Maui.Controls.BindingSourceGen.dll
  Maps -> /home/vsts/work/1/s/artifacts/bin/Maps/Debug/net10.0/Microsoft.Maui.Maps.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.Core -> /home/vsts/work/1/s/artifacts/bin/Controls.Core/Debug/net10.0/Microsoft.Maui.Controls.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.Xaml -> /home/vsts/work/1/s/artifacts/bin/Controls.Xaml/Debug/net10.0/Microsoft.Maui.Controls.Xaml.dll
  ##vso[build.updatebuildnumber]10.0.90-ci+azdo.14552921
  Controls.Maps -> /home/vsts/work/1/s/artifacts/bin/Controls.Maps/Debug/net10.0/Microsoft.Maui.Controls.Maps.dll
  TestUtils -> /home/vsts/work/1/s/artifacts/bin/TestUtils/Debug/netstandard2.0/Microsoft.Maui.TestUtils.dll
  Controls.Core.UnitTests -> /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.dll
Test run for /home/vsts/work/1/s/artifacts/bin/Controls.Core.UnitTests/Debug/net10.0/Microsoft.Maui.Controls.Core.UnitTests.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.
[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.45]   Discovering: Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.83]   Discovered:  Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.84]   Starting:    Microsoft.Maui.Controls.Core.UnitTests
[xUnit.net 00:00:02.98]   Finished:    Microsoft.Maui.Controls.Core.UnitTests
  Passed ClearUnsubscribesPreviousChildrenFromPropertyChanged [25 ms]

Test Run Successful.
Total tests: 1
     Passed: 1
 Total time: 3.8429 Seconds

📁 Fix files reverted (1 files)

• src/Controls/src/Core/Shapes/GeometryGroup.cs

---

📱 UI Tests — Shape

Detected UI test categories: Shape

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

🧪 UI Test Execution Results (deep, platform pool)

Category	Tests	Snapshot diffs
Shape	35/35 ✓	—
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs)

---

📋 Pre-Flight — Context & Validation

Issue: #35795 - GeometryGroup.Children.Clear() leaks Path visual trees when removed child geometries are shared
PR: #36232 - Fix GeometryGroup.Children.Clear() leaking shared child geometries
Platforms Affected: Shared Controls code; issue observed on Android and iOS; requested try-fix testing platform Android
Files Changed: 1 implementation, 1 test

Key Findings

• GeometryGroup.Children.Clear() raises NotifyCollectionChangedAction.Reset without OldItems, so the old implementation did not detach removed child Geometry.PropertyChanged handlers.
• The PR fix keeps per-Geometry subscription reference counts in GeometryGroup, handles Reset by detaching all tracked children, and resubscribes current children.
• Prior inline review found a duplicate-child flaw in an earlier HashSet implementation; the current PR's Dictionary<Geometry,int> ref counts address that concern.
• gh CLI was unauthenticated in this environment, so PR metadata and issue context were fetched from public GitHub API and CI status could not be fully authenticated.

Code Review Summary

Verdict: NEEDS_DISCUSSION
Confidence: low
Errors: 0 | Warnings: 0 | Suggestions: 1

Key code review findings:

• 💡 Add duplicate-child regression coverage to lock in the behavior that motivated the dictionary ref-count change.
• Prior ❌ duplicate-child finding is fixed by current _subscriptionRefCounts logic.
• CI/required-check status remained undetermined because gh pr checks --required could not authenticate.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #36232	Track child subscriptions with Dictionary<Geometry,int> ref counts; on Reset unsubscribe all tracked children and resubscribe current collection	✅ PASSED (Gate)	src/Controls/src/Core/Shapes/GeometryGroup.cs, src/Controls/tests/Core.UnitTests/GeometryGroupTests.cs	Original PR; localized and handles duplicate child instances

---

🔬 Code Review — Deep Analysis

Code Review — PR #36232

Independent Assessment

What this changes: GeometryGroup now centralizes child-geometry PropertyChanged subscription management, handles NotifyCollectionChangedAction.Reset, and tracks duplicate child instances with a Dictionary<Geometry,int> reference count so removing one occurrence does not detach the last remaining occurrence.
Inferred motivation: GeometryGroup.Children.Clear() raises Reset without OldItems, so removed geometries could keep stale PropertyChanged handlers and retain the old GeometryGroup/Path graph.

Reconciliation with PR Narrative

Author claims: The PR fixes GeometryGroup.Children.Clear() leaking shared child geometries by tracking subscribed children, explicitly handling reset, and adding a unit test.
Agreement/disagreement: This matches the current code. The current Dictionary<Geometry,int> implementation also addresses the prior duplicate-child concern raised against the earlier HashSet version.

Prior Review Reconciliation

Prior ❌ Error Finding	Source	Status	Evidence
Earlier HashSet<Geometry> tracking unsubscribed a duplicated child after one removal, so remaining duplicate entries would no longer invalidate.	Copilot inline comment; MauiBot NEEDS_CHANGES review on commit b8e3054...; kubaflo requested trying the AI suggestion.	✅ Fixed	Current head f038788... uses _subscriptionRefCounts (GeometryGroup.cs:15) and only detaches when the count reaches zero (GeometryGroup.cs:165-190).

Blast Radius Assessment

• Runs for all instances: Yes — every GeometryGroup now uses the new subscription bookkeeping.
• Startup impact: No — no static initialization or startup path changes; constructor still initializes Children normally.
• Static/shared state: No — the dictionary is per-instance.

CI Status

• Required-check result: gh pr checks --required could not run because gh is unauthenticated. Supplemental GitHub Checks API for head f038788... shows maui-pr (Build .NET MAUI Build macOS (Release)) failed and many maui-pr legs still in_progress.
• Classification: The failed macOS Release log reports missing .buildtasks/Microsoft.Maui.Core.Before.targets while building Controls.TestCases.HostApp.csproj, which does not appear caused by this Shapes-only PR. Pending checks remain undetermined.
• Action taken: Invoked azdo-build-investigator; ci-analysis skill was unavailable. Read MAUI CI facts and inspected AzDO build 1491545 timeline/log 241. Confidence capped low and verdict cannot be LGTM while CI is red/pending/undetermined.

Findings

💡 Suggestion — Add duplicate-child regression coverage

The current code fixes the prior duplicate-child bug with reference counting, but the added test only covers Clear() reset behavior (GeometryGroupTests.cs:10). A small test for adding the same Geometry twice, removing once, then mutating it would lock in the behavior that motivated the dictionary ref-count change.

Failure-Mode Probing

• Children.Clear() on a group with previously subscribed children: Reset calls UnsubscribeFromAllChildren(), detaching stale child handlers before clearing the dictionary.
• Duplicate child geometry added twice, then removed once: ref count decrements from 2 to 1 and the handler remains attached, so later mutations still invalidate the group.
• Replacing/removing normal children: old items decrement/detach, new items subscribe, and the group invalidates once after collection changes.
• Null child entries or null Children: helper methods guard null geometries/collections; AppendPath still assumes non-null Children, which is pre-existing public behavior.

Verdict: NEEDS_DISCUSSION

Confidence: low (code change is localized and reasoned/tested, but CI is currently failing/pending and required-check status could not be obtained through authenticated gh.)
Summary: I found no blocking code issue in the current implementation; the prior ❌ duplicate-child finding appears fixed. The only code suggestion is additional regression coverage. Because CI is not clean/complete, this cannot be LGTM yet under the skill rules.

---

🛠️ Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	try-fix	Add internal GeometryCollection.Clearing pre-clear snapshot event; GeometryGroup unsubscribes from snapshot and uses live membership for other changes	✅ PASS	3 files	Passed, but broader/invasive because it changes GeometryCollection internals
2	try-fix	Maintain a GeometryGroup shadow List<Geometry> of subscribed collection entries so Reset can detach pre-clear entries	✅ PASS	2 files	Passed, but more index bookkeeping and edge-case risk than ref counts
3	try-fix	Override GeometryCollection.ClearItems() to emit per-item Remove notifications instead of Reset	✅ PASS	2 files	Passed, but changes collection notification semantics globally
4	try-fix	Use WeakNotifyPropertyChangedProxy plus Children.Contains(sender) guard	✅ PASS	1 file	Passed narrow gate, but rejected by self-review: leaves weak proxy subscribers behind and can over-deliver for duplicates
PR	PR #36232	GeometryGroup Dictionary<Geometry,int> subscription ref counts; Reset unsubscribes all tracked children and resubscribes current collection	✅ PASSED (Gate)	2 files	Original PR; best balance of locality and deterministic cleanup

Cross-Pollination

Model	Round	New Ideas?	Details
gpt-5.5	1	No	NO NEW IDEAS after attempts 1-3
claude-opus-4.6	2	No	NO NEW IDEAS
claude-opus-4.7	2	No	NO NEW IDEAS
gpt-5.3-codex	2	Yes	Proposed weak subscription plus membership guard; evaluated as try-fix-4 and rejected after self-review
gpt-5.5	2	No	NO NEW IDEAS

Exhausted: Yes
Selected Fix: PR's fix — It is the only passing approach that is localized to GeometryGroup, preserves GeometryCollection notification semantics, and deterministically detaches child event handlers while handling duplicate child instances.

---

📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current title is close but not in the component-focused format, and the description has an awkward/inaccurate phrase while missing the ref-count and regression-test details from the winning fix.

Recommended title

Shapes: Fix GeometryGroup.Children.Clear() child subscription leak

Recommended description

### Issue Details

`GeometryGroup.Children.Clear()` triggers a `Reset` collection event, but `Reset` does not provide `OldItems`. The old `GeometryGroup` subscription logic only unsubscribed removed child geometries from `PropertyChanged` when `OldItems` was present, so cleared/shared geometries could retain stale handlers and keep the old `GeometryGroup`, `Path`, page, bindings, handlers, and view model alive.

### Description of Change

Updated `GeometryGroup.cs` to centralize child `PropertyChanged` subscription management with per-`Geometry` reference counts.

- Detaches the old `Children` collection and its tracked child handlers when the collection is replaced.
- Handles `NotifyCollectionChangedAction.Reset` by unsubscribing all previously tracked children, then re-subscribing only the current collection items.
- Handles add/remove/replace/move collection changes without prematurely detaching duplicate child geometry instances.
- Keeps shared child geometries working when the same `Geometry` is used more than once or across multiple `GeometryGroup` instances.

Added `GeometryGroupTests` coverage for the original `Clear()` leak path and the ref-count edge cases: duplicate child removal, final duplicate removal, shared children across groups, and collection replacement.

### Issues Fixed

Fixes #35795

**Tested the behavior in the following platforms.**
- [x] Android
- [x] Windows
- [x] iOS
- [x] Mac

| Before  | After  |
|---------|--------|
| **iOS**<br> <video src="https://github.com/user-attachments/assets/778c7ca3-c29d-45e4-9183-5cd9e5e057c0" width="600" height="300"> | **iOS**<br> <video src="https://github.com/user-attachments/assets/43fadd22-21b0-4543-8777-0e084b197981" width="600" height="300"> |

---

🏁 Report — Final Recommendation

Comparative Report — PR #36232

Winner

Selected candidate: pr-plus-reviewer

pr-plus-reviewer keeps the PR's localized, deterministic GeometryGroup ref-count fix and adds the missing regression coverage identified by the expert reviewer. It passes the targeted GeometryGroupTests suite with 5 tests and avoids the broader behavioral risks introduced by the alternative try-fix candidates.

Candidate ranking

Rank	Candidate	Regression result	Assessment
1	pr-plus-reviewer	✅ PASS	Best candidate. Same sound implementation as the PR, plus duplicate/shared-child and collection replacement regression tests covering the ref-count behavior.
2	pr	✅ PASSED gate	Sound localized implementation. Handles Reset, duplicate child instances, and collection replacement via per-Geometry ref counts, but lacks regression coverage for the duplicate/shared-child behavior that motivated the dictionary.
3	try-fix-2	✅ PASS	Maintains a GeometryGroup shadow list of subscribed entries. Localized, but more complex and index-sensitive than ref counts across add/remove/replace/move/reset paths.
4	try-fix-1	✅ PASS	Adds an internal GeometryCollection.Clearing pre-clear snapshot event. Works, but is broader than needed and changes GeometryCollection internals to fix one consumer.
5	try-fix-3	✅ PASS	Changes GeometryCollection.Clear() to emit per-item Remove notifications instead of Reset. This fixes the source notification issue, but globally changes observable collection semantics for every consumer.
6	try-fix-4	✅ PASS narrow gate	Ranked last despite passing the gate. The weak proxy approach leaves source event lists with proxy subscribers until later cleanup and can accumulate duplicate proxies; it does not deterministically clean up the subscription graph.

Comparison details

pr

The raw PR fix tracks child subscriptions with Dictionary<Geometry,int>, detaches all tracked children on Reset, and re-subscribes current collection items. This is the best implementation shape because it is localized to GeometryGroup, preserves GeometryCollection notification semantics, and handles duplicate child geometries without prematurely detaching the last remaining occurrence.

The main weakness is test coverage: the PR test proves the original Clear() leak is fixed, but it does not lock in the duplicate/shared-child behavior that the ref-counted dictionary is designed to protect.

pr-plus-reviewer

This candidate applies the expert reviewer feedback without changing production code. It adds regression tests for:

• duplicate child removal keeping the surviving occurrence subscribed;
• final duplicate removal detaching the child;
• one GeometryGroup.Children.Clear() not breaking another group that shares the same child geometry;
• collection replacement detaching old children and subscribing new children.

Targeted sandbox result: ✅ GeometryGroupTests passed, 5 total tests.

try-fix-1

This candidate adds an internal pre-clear snapshot event to GeometryCollection and uses live membership checks in GeometryGroup. It passes the targeted tests but has a larger blast radius because it modifies collection internals and adds a new event path solely to work around reset missing OldItems.

try-fix-2

This candidate keeps a shadow list of subscribed GeometryGroup.Children entries. It passes targeted tests and is reasonably localized, but the positional bookkeeping is more fragile than the PR's ref-count dictionary, especially across replace and move operations.

try-fix-3

This candidate overrides GeometryCollection.ClearItems() so Clear() emits repeated Remove notifications. It passes targeted tests, but it changes observable collection semantics globally from one Reset event to multiple Remove events, which is the highest compatibility risk among the passing deterministic-cleanup alternatives.

try-fix-4

This candidate uses weak PropertyChanged proxies with a live Children.Contains(sender) guard. It passes the narrow gate, but self-review found it does not fully clean the source event list and can accumulate duplicate proxies. Because it leaves stale proxy subscribers behind, it is materially weaker than deterministic unsubscribe approaches.

Final recommendation

Use pr-plus-reviewer: keep the PR production fix and add the expert-reviewer regression tests. If the PR author does not want additional tests, the raw PR fix remains the best implementation among production-code alternatives, but pr-plus-reviewer is the stronger merge candidate because it preserves the same fix and improves coverage.

---

🧭 Next Steps — review latest findings

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