fix: ObserveCountChanged misses count if observer has side effect

[awesomestmason]

The issue is that OnNext can have any number of side effects. If a side effect modifies the collection.Count, then the countPrev will be inaccurate, because the collection.Count has changed. It was not the same value that was actually emitted.

        protected override void Handler(in NotifyCollectionChangedEventArgs<T> eventArgs)
        {
            switch (eventArgs.Action)
            {
                case NotifyCollectionChangedAction.Add:
                case NotifyCollectionChangedAction.Remove:
                case NotifyCollectionChangedAction.Reset when countPrev != collection.Count:
                    observer.OnNext(collection.Count);
                    break;
            }
            countPrev = collection.Count; // Issue: collection.Count be a different value from the emitted value above, if OnNext has side effects.
        }

To fix this, I moved the countPrev assignment to before the emission. That way it is guaranteed that countPrev matches the value that we actually emitted.

[awesomestmason]

closes #120

[aetos382]

@awesomestmason
Thanks for tracking this down and writing it up so clearly.

Your analysis matches what I see in the code: assigning countPrev before OnNext is what makes the stored value match the value that was actually emitted, so a nested notification raised by the observer's side effect no longer compares against a stale count.

One thing I noticed while reading around it: ObserveCountChanged has a second overload for ISynchronizedView<T, TView>, and it looks like it has the same bug. See _SynchronizedViewCountChanged.Handler in src/ObservableCollections.R3/ObservableCollectionR3Extensions.View.cs (around line 412) — it is the same shape of code, with countPrev assigned after the switch:

                 case NotifyCollectionChangedAction.Add:
                 case NotifyCollectionChangedAction.Remove:
                 case NotifyCollectionChangedAction.Reset when countPrev != source.Count:
+                    countPrev = source.Count;
                     observer.OnNext(source.Count);
                     break;
             }
-            countPrev = source.Count;

Moving the assignment inside the switch should be safe here for the same reason it is in the collection overload: the only actions that fall through without emitting are Replace, Move, and a Reset where the count did not change, and none of those alter Count, so leaving countPrev untouched keeps it accurate.

Here is a test mirroring ObserveCountChanged_WithSideEffect. I confirmed that it fails without the change above — the 0 from Clear() never arrives:

Expected events to be a collection with 2 item(s), but {1}
contains 1 item(s) less than
{1, 0}.

and that it passes with the change applied:

[Fact]
public void ObserveViewCountChanged_WithSideEffect()
{
    var events = new List<int>();
    var collection = new ObservableList<int>([]);
    using var view = collection.CreateView(x => x);

    using var _ = view.ObserveCountChanged().Subscribe(count =>
    {
        events.Add(count);
        // Side effect - when count is 1, clear the list
        if (count == 1) collection.Clear();
    });

    events.Should().BeEmpty();

    collection.Add(12);

    view.Count.Should().Be(0);
    events.Should().BeEquivalentTo([1, 0]);
}

Please feel free to take these into this PR if you like, or leave them and I will send a follow-up PR for the view overload — whichever you prefer. It would just be nice to have both overloads covered together so #120 does not have to be reopened for the view case.

I have only recently started looking at this codebase, so I may well be missing some history behind the current shape of the code; someone more familiar with it should confirm.

[Copilot]

🟡 Changes recommended

There are a couple of correctness gaps (double-reading Count in the handler and an order-insensitive assertion in the new regression test) that can reduce the reliability of the fix and its test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes ObserveCountChanged so that its internal countPrev tracking stays consistent with the value emitted to observers, even when OnNext has side effects that mutate the collection. It also adds a regression test that exercises re-entrant mutation (clearing the list from inside the observer).

Changes:

• Update ObserveCountChanged handler to assign countPrev before emitting the count.
• Add a new test covering observer side effects that change the collection count.

File summaries

File	Description
src/ObservableCollections.R3/ObservableCollectionR3Extensions.cs	Adjusts ObserveCountChanged handler to avoid countPrev becoming inaccurate when OnNext causes side effects.
tests/ObservableCollections.R3.Tests/ObservableCollectionExtensionsTest.cs	Adds a regression test for re-entrant side effects during ObserveCountChanged notifications.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[awesomestmason]

@aetos382 thank you for reviewing the PR and helping maintain this project.

Good catch on the synchronized view case, I agree that it would make sense to fix both in the PR, so I have updated the PR to include it. I also added your test case to the tests file, and implemented the Copilot suggestions which I agreed with.