Fix nested property binding to avoid redundant setter calls #4240
+378
−28
Conversation
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
Adds tests and refactors PropertyBinderImplementation to ensure that BindTo only sets nested properties once per value, even when the host or nested object is replaced. This prevents redundant setter invocations and ensures correct property update semantics for nested bindings across platforms.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #4214 by refactoring PropertyBinderImplementation to prevent redundant setter calls during nested property binding operations. The implementation adds equality checks and state tracking to ensure nested properties are set only once per value, even when host objects are replaced.
Key Changes
- Refactored
SetThenGetto return a tuple indicating whether the value changed and should be emitted - Implemented state tracking in nested property bindings using
Observable.CreatewithlatestHost,currentHost, andlastObservedValuevariables - Added equality comparisons before invoking setters to avoid redundant calls when values haven't changed
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/ReactiveUI/Bindings/Property/PropertyBinderImplementation.cs | Core fix implementing deduplication logic for nested property bindings, adds state tracking for host and value changes, and modifies EvalBindingHooks to require IViewFor constraint |
| src/ReactiveUI.Tests/Bindings/BindToNestedPropertyTests.cs | New test file with three test cases verifying single setter invocation behavior for nested properties in various scenarios including host replacement |
| src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs | Adds two test cases for nested property binding behavior and corrects test method name from ViewToViewModel to ViewModelToView |
| src/ReactiveUI.Tests/Platforms/winforms/DefaultPropertyBindingTests.cs | Minor style improvements converting multi-line methods to expression-bodied members |
src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs
Outdated
Show resolved
Hide resolved
src/ReactiveUI/Bindings/Property/PropertyBinderImplementation.cs
Outdated
Show resolved
Hide resolved
Renamed a test method for clarity, improved property setter logic in TrackingHostViewModel to only raise notifications and increment SetCallCount on value changes, and removed an unnecessary assignment in PropertyBinderImplementation.
src/ReactiveUI/Bindings/Property/PropertyBinderImplementation.cs
Outdated
Show resolved
Hide resolved
src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs
Outdated
Show resolved
Hide resolved
src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs
Outdated
Show resolved
Hide resolved
src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs
Outdated
Show resolved
Hide resolved
src/ReactiveUI.Tests/Platforms/windows-xaml/PropertyBindingTest.cs
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4240 +/- ##
==========================================
+ Coverage 40.33% 40.77% +0.44%
==========================================
Files 146 146
Lines 6655 6744 +89
Branches 1025 1038 +13
==========================================
+ Hits 2684 2750 +66
- Misses 3779 3794 +15
- Partials 192 200 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Contributor
|
@ChrisPulman I've opened a new pull request, #4241, to work on those changes. Once the pull request is ready, I'll request review from you. |
Copilot AI
added a commit
that referenced
this pull request
Dec 21, 2025
- Fix type mismatch in EvalBindingHooks call (pass object, IViewFor type args) - Add synchronization to hostChanges to prevent race conditions - Convert static SetCallCount to instance-level in BindToNestedPropertyTests - Update ViewModel property to use RaiseAndSetIfChanged pattern - Fix test expectations after converting to instance counters - Remove redundant using directives from PropertyBindingTest.cs - Fix test expectation for deduplicated setter calls (9 -> 3) - Remove readonly modifier from _nested field to allow RaiseAndSetIfChanged Co-authored-by: ChrisPulman <[email protected]>
Addresses code review comments from the nested property binding fix, focusing on type safety, thread safety, and test maintainability. ## Changes **Type Safety** - Fixed `EvalBindingHooks` call to provide both required type arguments `<object, IViewFor>` (was missing second type parameter causing compilation error) **Thread Safety** - Added `.Synchronize()` to `hostChanges` observable to prevent race conditions between `hostDisposable` and `changeDisposable` subscriptions accessing shared state variables **Test Infrastructure** - Converted `SetCallCount` from static to instance-level properties in test classes to prevent test isolation issues during parallel execution - Updated test expectations to match instance-level counting behavior (9→1 for per-instance tests, 9→3 for deduplication tests) **Code Cleanup** - Removed redundant using directives already declared as global usings - Removed `readonly` modifier from `_nested` field to allow `RaiseAndSetIfChanged` - Fixed property initializer structure to use explicit backing field pattern All tests passing (294/294) across .NET 8.0, 9.0, and 10.0. <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: ChrisPulman <[email protected]>
glennawatson
approved these changes
Dec 21, 2025
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.
What kind of change does this PR introduce?
Fixes #4214
What is the current behavior?
#4214
What is the new behavior?
Adds tests and refactors PropertyBinderImplementation to ensure that BindTo only sets nested properties once per value, even when the host or nested object is replaced.
This prevents redundant setter invocations and ensures correct property update semantics for nested bindings across platforms.
What might this PR break?
One less value should be emitted from BindTo when used in the two-way binding manner as shown in #4214
Please check if the PR fulfills these requirements
Other information: