Skip to content

fix: resolve CodeQL complex-condition alert and PerformanceView MVVM violation#435

Merged
laurentiu021 merged 1 commit into
mainfrom
fix/code-quality-mvvm
May 19, 2026
Merged

fix: resolve CodeQL complex-condition alert and PerformanceView MVVM violation#435
laurentiu021 merged 1 commit into
mainfrom
fix/code-quality-mvvm

Conversation

@laurentiu021

@laurentiu021 laurentiu021 commented May 19, 2026

Copy link
Copy Markdown
Owner

Summary

Two code quality improvements:

1. CodeQL alert #302 — complex condition (ProcessManagerViewModel)

The MatchesDescription method had 3 chained || with null-conditional operators, triggering CodeQL's cs/complex-condition rule. Replaced with a ReadOnlySpan<string?> loop that checks each field individually — same behavior, lower cyclomatic complexity.

2. PerformanceView MVVM violation

The code-behind had:

  • A PropertyChanged subscription to sync radio buttons from ViewModel
  • A Checked event handler to push radio button selection back to ViewModel

Both are now replaced by a reusable EqualityConverter with two-way binding. Code-behind reduced from 55 lines to 14 (just InitializeComponent).

Files changed

  • ProcessManagerViewModel.cs — simplified MatchesDescription
  • PerformanceView.xaml — radio buttons use converter binding
  • PerformanceView.xaml.cs — removed all code-behind logic
  • Helpers/EqualityConverter.cs — new reusable converter
  • App.xaml — registered IsEqual converter
  • EqualityConverterTests.cs — 10 unit tests
  • CHANGELOG.md — updated

Testing

  • Build: 0 errors (main + tests)
  • 10 new unit tests for EqualityConverter (Convert, ConvertBack, null, case sensitivity)

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved code quality and fixed performance-related issues.
    • Enhanced radio button behavior in the Performance settings for more reliable selection.
  • Tests

    • Added comprehensive unit test coverage for converter functionality.
  • Refactor

    • Simplified internal logic to improve maintainability and performance.

Review Change Stack

…violation

- ProcessManagerViewModel: replace chained || with ReadOnlySpan loop in MatchesDescription (CodeQL #302)
- PerformanceView: remove PropertyChanged handler and Checked event from code-behind; use EqualityConverter for two-way radio button binding
- Add EqualityConverter (reusable IValueConverter for radio button groups)
- Add EqualityConverterTests (10 tests: Convert, ConvertBack, null, case sensitivity)
@coderabbitai

coderabbitai Bot commented May 19, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Introduces a reusable WPF EqualityConverter for two-way binding equality checks, applies it to refactor PerformanceView's radio-button selection from event-driven code-behind to pure XAML binding, refactors ProcessManagerViewModel.MatchesDescription to use ReadOnlySpan for code-quality compliance, and documents all changes in the changelog.

Changes

EqualityConverter & PerformanceView Binding

Layer / File(s) Summary
EqualityConverter Implementation & Registration
SysManager/SysManager/Helpers/EqualityConverter.cs, SysManager/SysManager/App.xaml
EqualityConverter is a sealed WPF IValueConverter implementing two-way equality comparison: Convert returns false when value or parameter is null, otherwise compares via ordinal ToString() equality; ConvertBack returns parameter on true, Binding.DoNothing otherwise. Registered as IsEqual resource in application XAML.
EqualityConverter Unit Tests
SysManager/SysManager.Tests/EqualityConverterTests.cs
EqualityConverterTests with 8 unit tests covering Convert and ConvertBack paths: matching/non-matching strings, empty string, null value, null parameter, both null, case-sensitivity validation, and Binding.DoNothing returns.
PerformanceView Binding Integration
SysManager/SysManager/Views/PerformanceView.xaml, SysManager/SysManager/Views/PerformanceView.xaml.cs
Power-plan radio buttons switched from Tag + Checked event handler to IsChecked bindings to SelectedPlan with IsEqual converter and per-button ConverterParameter values. Code-behind simplified to only InitializeComponent(); removed PropertyChanged subscription, handler, SyncRadioButtons helper, and PowerPlan_Checked event handler.
Documentation: New EqualityConverter
CHANGELOG.md
Documented introduction of EqualityConverter and EqualityConverterTests in the [Unreleased] → Added section.

ProcessManagerViewModel Code Quality Refactor

Layer / File(s) Summary
MatchesDescription ReadOnlySpan Refactor
SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs, CHANGELOG.md
MatchesDescription refactored from expression-bodied null-coalescing OR chain to block implementation iterating over Description, PlainDescription, and Category via ReadOnlySpan<string?>, returning true when any field contains the filter (case-insensitive). Resolves CodeQL cs/complex-condition warning. Documented in [Unreleased] → Fixed section.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • laurentiu021/SystemManager#294: Both PRs modify SysManager/SysManager/Views/PerformanceView.xaml.cs's PropertyChanged/data-context change handling—this PR removes the code-behind subscription logic in favor of XAML EqualityConverter binding, while the related PR addresses those subscription/unsubscription handlers.
  • laurentiu021/SystemManager#413: Both PRs modify ProcessManagerViewModel's text-matching/filter logic with CodeQL-driven complex-condition refactoring around MatchesDescription, with overlapping changes at the same function/class level.
  • laurentiu021/SystemManager#428: Both PRs modify ProcessManagerViewModel.cs by refactoring description-matching logic; this PR tightens MatchesDescription with ReadOnlySpan iteration while the related PR extracts matching into helper methods.

Poem

🐰 A converter born of binding's way,
Makes radio buttons dance in XAML's play.
No more event handlers cluttering the view,
Just equality checks through and through.
ReadOnlySpan trims complex condition's weight—
Cleaner, simpler code, and it's looking great! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: resolving a CodeQL complex-condition alert in ProcessManagerViewModel and fixing an MVVM violation in PerformanceView by introducing EqualityConverter.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/code-quality-mvvm

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
SysManager/SysManager/Helpers/EqualityConverter.cs (1)

25-29: ⚡ Quick win

Preserve ConverterParameter type in ConvertBack return path.

At Line 28, returning parameter?.ToString() forces string conversion and weakens reusability. Return the raw parameter and short-circuit null to Binding.DoNothing.

♻️ Proposed fix
 public object? ConvertBack(object? value, Type targetType, object? parameter, CultureInfo culture)
 {
     if (value is true)
-        return parameter?.ToString();
+        return parameter ?? Binding.DoNothing;
     return Binding.DoNothing;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@SysManager/SysManager/Helpers/EqualityConverter.cs` around lines 25 - 29, The
ConvertBack method currently returns parameter?.ToString(), which forces the
ConverterParameter to a string; change ConvertBack so when value is true it
returns the raw parameter object (not ToString()), and if parameter is null
return Binding.DoNothing; keep the existing behavior of returning
Binding.DoNothing when value is not true. Ensure you update the ConvertBack
method (symbol: ConvertBack) to return parameter directly and use
Binding.DoNothing as the null short-circuit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@SysManager/SysManager/Helpers/EqualityConverter.cs`:
- Around line 25-29: The ConvertBack method currently returns
parameter?.ToString(), which forces the ConverterParameter to a string; change
ConvertBack so when value is true it returns the raw parameter object (not
ToString()), and if parameter is null return Binding.DoNothing; keep the
existing behavior of returning Binding.DoNothing when value is not true. Ensure
you update the ConvertBack method (symbol: ConvertBack) to return parameter
directly and use Binding.DoNothing as the null short-circuit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c423aa3-3f70-42b7-92ec-953baf56d7b2

📥 Commits

Reviewing files that changed from the base of the PR and between 1128f28 and 7720d81.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • SysManager/SysManager.Tests/EqualityConverterTests.cs
  • SysManager/SysManager/App.xaml
  • SysManager/SysManager/Helpers/EqualityConverter.cs
  • SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs
  • SysManager/SysManager/Views/PerformanceView.xaml
  • SysManager/SysManager/Views/PerformanceView.xaml.cs
💤 Files with no reviewable changes (1)
  • SysManager/SysManager/Views/PerformanceView.xaml.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build & unit tests
  • GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
SysManager/SysManager/ViewModels/ProcessManagerViewModel.cs (1)

168-177: LGTM!

SysManager/SysManager/Views/PerformanceView.xaml (1)

64-64: LGTM!

Also applies to: 68-68, 72-72

@laurentiu021
laurentiu021 merged commit a57056d into main May 19, 2026
5 checks passed
@laurentiu021
laurentiu021 deleted the fix/code-quality-mvvm branch May 19, 2026 10:01
laurentiu021 added a commit that referenced this pull request May 22, 2026
Adds CHANGELOG entries for all 9 releases from the QA bug fix session:

- **v0.28.16** — Dispose lifecycle (#395, #410)
- **v0.28.17** — CTS disposal + bare catch (#396, #413)
- **v0.28.18** — Input validation + null checks (#397, #398)
- **v0.28.19** — JSON error handling (#400)
- **v0.28.20** — Drive scanning + cache eviction + ConfigureAwait (#401,
#402, #403)
- **v0.28.21** — Audit logging + error messages (#405, #407)
- **v0.28.22** — SHA256 verification (#408, #409)
- **v0.28.23** — Service timeout + snapshot persist + traceroute DNS
(#414, #415, #416)
- **v0.28.24** — Accessibility (#411)

18 bugs fixed in total.

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021 added a commit that referenced this pull request May 22, 2026
…violation (#435)

- ProcessManagerViewModel: replace chained || with ReadOnlySpan loop in MatchesDescription (CodeQL #302)
- PerformanceView: remove PropertyChanged handler and Checked event from code-behind; use EqualityConverter for two-way radio button binding
- Add EqualityConverter (reusable IValueConverter for radio button groups)
- Add EqualityConverterTests (10 tests: Convert, ConvertBack, null, case sensitivity)

Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
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