Skip to content

Conversation

@ChrisPulman
Copy link
Member

@ChrisPulman ChrisPulman commented Jan 4, 2026

What are the main goals of this change?

  • Better readability (style, grammar, spelling...)
  • Content correction (accuracy, wrong information...)
  • New content

The guidelines document now includes comprehensive best practices for modern ReactiveUI development, covering application initialization, source generators, event handling, lifecycle management, architecture, dependency injection, performance, testing, error handling, platform-specific advice, anti-patterns, and migration paths.

This update aims to help developers build maintainable, testable, and performant reactive applications.

closes #288

The guidelines document now includes comprehensive best practices for modern ReactiveUI development, covering application initialization, source generators, event handling, lifecycle management, architecture, dependency injection, performance, testing, error handling, platform-specific advice, anti-patterns, and migration paths. This update aims to help developers build maintainable, testable, and performant reactive applications.
@glennawatson glennawatson requested a review from Copilot January 6, 2026 08:08
Copy link

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

This PR significantly expands the ReactiveUI guidelines document from a simple header to a comprehensive 614-line guide covering modern development best practices. The update provides detailed recommendations for application initialization, source generators, event handling, architecture patterns, dependency injection, performance optimization, testing, error handling, and platform-specific guidance.

Key Changes:

  • Introduces modern patterns using RxAppBuilder and source generators
  • Establishes lifecycle management guidelines with WhenActivated
  • Provides comprehensive architecture and performance best practices

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +336 to +342
_itemsCache.Connect()
.Transform(item => new ItemViewModel(item))
.Filter(vm => vm.IsVisible)
.Sort(SortExpressionComparer<ItemViewModel>.Ascending(x => x.Name))
.ObserveOn(RxSchedulers.MainThreadScheduler)
.Bind(out _items)
.Subscribe();
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The DynamicData example shows a Subscribe() without proper disposal via DisposeWith. According to the guidelines established in this document, all subscriptions should be properly disposed. The subscription on line 342 should either be wrapped in WhenActivated or stored in a CompositeDisposable for later disposal.

Copilot uses AI. Check for mistakes.
var sharedObservable = this.WhenAnyValue(x => x.Property1)
.Publish()
.RefCount();

Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The "Optimize Observable Chains" example shows Subscribe calls without proper disposal. While this is demonstrating the concept of shared observables, it still violates the disposal guidelines. Consider adding DisposeWith or a comment noting that disposal code is omitted for brevity.

Suggested change
// In real code, ensure these subscriptions are disposed (e.g., with DisposeWith). Omitted for brevity.

Copilot uses AI. Check for mistakes.
Comment on lines +419 to +429

// Handle errors gracefully
SaveCommand.ThrownExceptions
.Subscribe(ex =>
{
// Log the error
this.Log().Error(ex, "Failed to save");

// Show user-friendly message
ErrorMessage = "Unable to save. Please try again.";
});
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The ThrownExceptions subscription on line 422 is not wrapped in WhenActivated and lacks proper disposal. This contradicts the disposal guidelines established earlier in the document. Subscriptions in ViewModels should also follow proper disposal patterns, either through WhenActivated or by storing in a CompositeDisposable.

Suggested change
// Handle errors gracefully
SaveCommand.ThrownExceptions
.Subscribe(ex =>
{
// Log the error
this.Log().Error(ex, "Failed to save");
// Show user-friendly message
ErrorMessage = "Unable to save. Please try again.";
});
this.WhenActivated(disposables =>
{
// Handle errors gracefully
SaveCommand.ThrownExceptions
.Subscribe(ex =>
{
// Log the error
this.Log().Error(ex, "Failed to save");
// Show user-friendly message
ErrorMessage = "Unable to save. Please try again.";
})
.DisposeWith(disposables);
});

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +87
public MyViewModel()
{
_fullNameHelper = this.WhenAnyValue(x => x.FirstName, x => x.LastName,
(f, l) => $"{f} {l}")
.ToProperty(this, nameof(FullName));
}
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

In the "New way (source generators)" example, the variable _fullNameHelper is referenced but was never declared. The [ObservableAsProperty] attribute on line 80 generates the helper, but the code on line 84 incorrectly assigns to _fullNameHelper. This line should be removed or the assignment should be to the auto-generated helper field which typically follows the naming convention of the attributed field with "Helper" suffix.

Suggested change
public MyViewModel()
{
_fullNameHelper = this.WhenAnyValue(x => x.FirstName, x => x.LastName,
(f, l) => $"{f} {l}")
.ToProperty(this, nameof(FullName));
}
// With source generators, the FullName observable and its helper are generated
// automatically based on the attributes above, so no manual wiring is required.

Copilot uses AI. Check for mistakes.
Comment on lines +456 to +460

// Command only executes when valid
LoginCommand = ReactiveCommand.CreateFromTask(
LoginAsync,
this.IsValid());
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The example code declares both LoginCommand as a property on line 458 and a Login method with [ReactiveCommand] attribute on line 463. These are conflicting - the [ReactiveCommand] attribute generates a LoginCommand property automatically from the Login method. Remove the explicit LoginCommand property declaration on line 458 since it will be auto-generated.

Suggested change
// Command only executes when valid
LoginCommand = ReactiveCommand.CreateFromTask(
LoginAsync,
this.IsValid());

Copilot uses AI. Check for mistakes.
Comment on lines +311 to +317
this.WhenAnyValue(x => x.SearchText)
.Throttle(TimeSpan.FromMilliseconds(500))
.DistinctUntilChanged()
.Where(text => !string.IsNullOrWhiteSpace(text))
.SelectMany(async text => await SearchAsync(text))
.ObserveOn(RxSchedulers.MainThreadScheduler)
.Subscribe(results => Results = results);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

In the performance example, the code directly assigns to Results in a Subscribe handler without using WhenActivated for lifecycle management. This violates the guideline established earlier in the document that subscriptions should be wrapped in WhenActivated and properly disposed. Consider adding a note that this is simplified example code or showing the proper WhenActivated pattern.

Suggested change
this.WhenAnyValue(x => x.SearchText)
.Throttle(TimeSpan.FromMilliseconds(500))
.DistinctUntilChanged()
.Where(text => !string.IsNullOrWhiteSpace(text))
.SelectMany(async text => await SearchAsync(text))
.ObserveOn(RxSchedulers.MainThreadScheduler)
.Subscribe(results => Results = results);
this.WhenActivated(disposables =>
{
this.WhenAnyValue(x => x.SearchText)
.Throttle(TimeSpan.FromMilliseconds(500))
.DistinctUntilChanged()
.Where(text => !string.IsNullOrWhiteSpace(text))
.SelectMany(async text => await SearchAsync(text))
.ObserveOn(RxSchedulers.MainThreadScheduler)
.Subscribe(results => Results = results)
.DisposeWith(disposables);
});

Copilot uses AI. Check for mistakes.
- [Testing Guide](~/docs/handbook/testing.md)
- [Sample Applications](~/docs/resources/samples.md)

## Platform-Specific Guidelines
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The "Platform-Specific Guidelines" section header appears twice in this document. The first occurrence is at line 468 with WPF, MAUI, and Blazor subsections. The second occurrence is at line 607 which just references other documents. Consider renaming the second section to "Platform-Specific Documentation" or "Platform-Specific Reference" to avoid confusion.

Suggested change
## Platform-Specific Guidelines
## Platform-Specific Documentation

Copilot uses AI. Check for mistakes.
Comment on lines +246 to +249
.ToProperty(this, x => x.IsSearching);

SearchCommand
.ToProperty(this, x => x.Results);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

The example shows a private field _isSearching with [ObservableAsProperty] attribute, but the code tries to use SearchCommand.IsExecuting.ToProperty(this, x => x.IsSearching). With source generators, when using [ObservableAsProperty], you typically need to assign to the generated helper field or use a different pattern. The assignment to a helper is missing here, similar to the pattern shown in the OAPH example above.

Suggested change
.ToProperty(this, x => x.IsSearching);
SearchCommand
.ToProperty(this, x => x.Results);
.ToPropertyEx(this, x => x.IsSearching);
SearchCommand
.ToPropertyEx(this, x => x.Results);

Copilot uses AI. Check for mistakes.
Comment on lines +246 to +249
.ToProperty(this, x => x.IsSearching);

SearchCommand
.ToProperty(this, x => x.Results);
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

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

Similar issue with the _results property - the code shows [ObservableAsProperty] on line 236, but then on line 248-249 tries to assign SearchCommand.ToProperty(this, x => x.Results) without storing the helper. When using source generators with [ObservableAsProperty], you need to assign the ToProperty result to the generated helper field.

Suggested change
.ToProperty(this, x => x.IsSearching);
SearchCommand
.ToProperty(this, x => x.Results);
.ToPropertyEx(this, x => x.IsSearching);
SearchCommand
.ToPropertyEx(this, x => x.Results);

Copilot uses AI. Check for mistakes.
@glennawatson glennawatson merged commit dbb986f into main Jan 8, 2026
10 checks passed
@glennawatson glennawatson deleted the UpdateGuidelinesBestPractices branch January 8, 2026 00:24
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.

Cleanup Guidelines section

4 participants