Skip to content

Conversation

glennawatson
Copy link
Contributor

What’s fixed

When a view’s ViewModel is set to null, any previously bound Interaction<TInput,TOutput> handlers are now unregistered immediately. This restores the expected behavior where calling Interaction.Handle(...) after clearing the ViewModel throws an UnhandledInteractionException<,>.

Symptoms

  • After view.ViewModel = null;, invoking vm.Interaction1.Handle("123") still hits the old handler instead of throwing.

  • NUnit tests like:

    Assert.That(async () => await vm.Interaction1.Handle("123").ToTask(),
                Throws.TypeOf<UnhandledInteractionException<string,bool>>());

    fail with “But was: no exception thrown”.

Before vs After

Before

  • InteractionBinderImplementation only reacted to non-null ViewModel transitions.
  • If ViewModel became null, the binding stream didn’t emit a value to trigger disposal, so the previously registered handler could remain attached.

After

  • We explicitly merge a “VM became null” stream into the binding sequence:

    • When ViewModel is null, we emit a default(IInteraction<,>) value.
    • The binder disposes the current handler (SerialDisposable) on that emission.
  • Result: as soon as the ViewModel is cleared, handlers are unregistered and subsequent Handle(...) calls correctly surface UnhandledInteractionException<,>.

Technical summary

  • Added:

    var vmNulls = view.WhenAnyValue(x => x.ViewModel)
                      .Where(x => x is null)
                      .Select(_ => default(IInteraction<TInput, TOutput>));
    var source = Reflection.ViewModelWhenAnyValue(viewModel, view, vmExpression)
        .Cast<IInteraction<TInput, TOutput>?>()
        .Merge(vmNulls);
  • On each emission we set interactionDisposable.Disposable to either the new handler registration or Disposable.Empty when null is seen, which disposes the prior registration.

  • Implemented in both overloads (Task and IObservable<TDontCare> handlers).

Impact

  • Restores the contract many apps rely on: no handler after ViewModel = null.
  • Aligns runtime behavior with test expectations in NUnit (no timing hacks/sleeps needed).
  • No API changes; behavior is more predictable and correct.

How to verify

  1. Bind an interaction handler.

  2. Set ViewModel to null.

  3. Call Interaction.Handle(...).

    • Expected: UnhandledInteractionException<,> is thrown.
  4. Run the test suite. Previously failing tests like:

    • UnregisterTaskHandlerWhenViewModelIsSetToNull
    • UnregisterObservableHandlerWhenViewModelIsSetToNull
      should pass.

Compatibility & risk

  • Low risk: only affects the unbinding path when ViewModel to null.
  • If any app previously depended on handlers remaining active after clearing the ViewModel (unlikely and contrary to docs/intent), behavior will now be corrected.

…Root cause: Reflection.ViewModelWhenAnyValue filters out null ViewModel values, so the binder never saw a null and didn’t dispose the previous registration. This left the handler attached after ViewModel was set to null, causing Interaction.Handle to succeed instead of throwing UnhandledInteractionException.\n\nFix: Merge a null-emitting stream based on view.WhenAnyValue(x => x.ViewModel) into the interaction property stream, and set the SerialDisposable to Disposable.Empty when null is observed. Applied to both Task and Observable handler overloads.\n\nTests: ReactiveUI.Tests.InteractionBinding.InteractionBinderImplementationTests now pass (20/20). Ran entire ReactiveUI.Tests namespace: 294/294 passed.
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Fixes immediate unregistration of Interaction<TInput,TOutput> handlers when a view’s ViewModel becomes null so subsequent Handle(...) calls correctly throw UnhandledInteractionException<,>.

  • Emit a null interaction when ViewModel transitions to null and merge it into the binding stream.
  • Replace WhereNotNull() with logic that disposes the current registration on null emissions via SerialDisposable.
  • Adjust test parallelization attributes and csproj condition for WinUI.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/ReactiveUI/Bindings/Interaction/InteractionBinderImplementation.cs Merge a VM-null stream into binding source and dispose handler on null to unregister immediately.
src/ReactiveUI.WinUI/ReactiveUI.WinUI.csproj Fix ItemGroup condition to only include CsWinRT on Windows targets.
src/ReactiveUI.Builder.Tests/ReactiveUIBuilderCoreTests.cs Mark test fixture NonParallelizable.
src/ReactiveUI.Builder.Tests/ReactiveUIBuilderBlockingTests.cs Mark test fixture NonParallelizable.
src/ReactiveUI.Builder.Tests/AssemblyInfo.Parallel.cs Reduce assembly test parallelism to 1.
src/ReactiveUI.Builder.Maui.Tests/AssemblyInfo.Parallel.cs Reduce assembly test parallelism to 1.
Comments suppressed due to low confidence (1)

@reactiveui reactiveui deleted a comment from Copilot AI Sep 21, 2025
Copy link

codecov bot commented Sep 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.16%. Comparing base (d75f999) to head (6f5398f).
⚠️ Report is 273 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4140       +/-   ##
===========================================
- Coverage   59.03%   37.16%   -21.88%     
===========================================
  Files         160      138       -22     
  Lines        5847     5686      -161     
  Branches     1031      852      -179     
===========================================
- Hits         3452     2113     -1339     
- Misses       2007     3357     +1350     
+ Partials      388      216      -172     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@glennawatson glennawatson merged commit 89d9200 into main Sep 21, 2025
5 of 6 checks passed
@glennawatson glennawatson deleted the fix/nunit-interaction-unregister branch September 21, 2025 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant