Skip to content

Fix 6 verified Lite bugs from code review#611

Merged
erikdarlingdata merged 1 commit intodevfrom
fix/lite-code-review-findings
Mar 17, 2026
Merged

Fix 6 verified Lite bugs from code review#611
erikdarlingdata merged 1 commit intodevfrom
fix/lite-code-review-findings

Conversation

@erikdarlingdata
Copy link
Owner

@erikdarlingdata erikdarlingdata commented Mar 17, 2026

Summary

  • Anomaly detector rate normalization: Wait/blocking/deadlock spike detection now compares per-hour rates instead of raw totals across different-length windows (24h baseline vs 4h analysis). Previously a "5x spike" threshold required ~30x the actual hourly rate to trigger — anomaly detection was effectively non-functional.
  • Poison wait UTC fix: Replaced the only NOW() call in the codebase with parameterized DateTime.UtcNow to match UTC-stored collection_time
  • Daily summary aggregation: top_wait_type now uses GROUP BY wait_type ORDER BY SUM() instead of picking the single highest individual delta sample
  • FinOps High Impact filters: Routes data through DataGridFilterManager instead of setting ItemsSource directly — column filters were broken on this grid
  • Plan viewer accuracy ratio: Divides ActualRows by ActualExecutions before comparing to EstimateRows — operators with many executions no longer show false "1000x off" warnings
  • Settings Test Email: Saves/restores App SMTP values around the test instead of mutating them permanently. Cancel no longer leaves test values in effect or persists password to credential store.

Test plan

  • Run Lite against a server with active workload, verify anomaly detection triggers on real spikes
  • Verify daily summary shows the correct top wait type (highest aggregate, not highest single sample)
  • Open FinOps > High Impact Queries, apply a column filter, verify it works
  • Open a plan with Nested Loop joins, verify accuracy % in node visuals matches edge tooltips
  • Open Settings > SMTP, click Test Email, then Cancel — verify SMTP settings revert

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved anomaly detection accuracy for wait times, blocking, and deadlock analyses by normalizing metrics across different reporting windows.
    • Corrected execution plan visualization to display actual rows per execution versus estimated rows.
    • Enhanced identification of top wait types in daily performance summaries.
    • Fixed SMTP settings to preserve original values correctly during test email operations.

- Anomaly detector: normalize wait/blocking/deadlock ratios to per-hour
  rates before comparing across different-length windows (24h baseline vs
  4h analysis). Previously required ~30x actual rate increase to trigger
  a "5x spike" threshold.
- Poison wait query: replace DuckDB NOW() with parameterized DateTime.UtcNow
  to match UTC-stored collection_time (only NOW() usage in entire codebase)
- Daily summary: top_wait_type now uses GROUP BY + SUM aggregate instead of
  picking the single highest individual delta sample
- FinOps High Impact grid: route data through filter manager instead of
  setting ItemsSource directly (column filters were broken on this grid)
- Plan viewer accuracy ratio: divide ActualRows by ActualExecutions before
  comparing to EstimateRows (per-execution comparison, not total vs estimate)
- Settings: Test Email now saves/restores App SMTP values instead of mutating
  them permanently. Cancel no longer leaves test values in effect.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 17, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Updates anomaly detection to normalize metrics by window lengths, adjusts UI data flows through filter managers, refines row-per-execution visualization calculations, modifies SQL queries for top-wait-type and timestamped filtering, and introduces state preservation for SMTP settings during email testing.

Changes

Cohort / File(s) Summary
Anomaly Detection Normalization
Lite/Analysis/AnomalyDetector.cs
Reworked wait, blocking, and deadlock anomaly comparisons to normalize metrics by window lengths (per-hour rates). Introduces baselineHours/currentHours calculations, derives per-hour ratios (baselineRate/currentRate), and replaces absolute ratio logic with normalized rate-based ratios. Threshold checks updated accordingly across all anomaly types.
UI Control Data Flows
Lite/Controls/FinOpsTab.xaml.cs, Lite/Controls/PlanViewerControl.xaml.cs
FinOpsTab replaces direct grid data-source assignment with filter manager update call. PlanViewerControl computes actual rows per execution (ActualRows/ActualExecutions) and uses this for accuracy ratio calculation and display instead of raw ActualRows.
Data Service Query Updates
Lite/Services/LocalDataService.DailySummary.cs, Lite/Services/LocalDataService.WaitStats.cs
DailySummary changes top\_wait\_type query to group by wait\_type and order by aggregated SUM(delta\_wait\_time\_ms). WaitStats converts relative timestamp filter (NOW() - INTERVAL) to parameterized DateTime.UtcNow-based filtering for deterministic window handling.
Settings State Preservation
Lite/Windows/SettingsWindow.xaml.cs
Introduces temporary backup (origServer/origPort/origSsl/origUsername/origFrom/origRecipients) of current SMTP settings before email test, then restores them in a finally block to ensure only explicit Save operations persist changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix 6 verified Lite bugs from code review' accurately summarizes the main objective of the changeset, which addresses six specific bug fixes across the Lite codebase identified from code review.

✏️ 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/lite-code-review-findings
📝 Coding Plan
  • Generate coding plan for human review comments

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

@erikdarlingdata erikdarlingdata merged commit b0bbb41 into dev Mar 17, 2026
4 of 5 checks passed
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