Add a Range Navigator Slicer/Filter to the querystore screen#113
Add a Range Navigator Slicer/Filter to the querystore screen#113rferraton wants to merge 1 commit intoerikdarlingdata:devfrom
Conversation
…tric display in background of the range is base on the sorting metric in the drop down selector Auto fetch when opening the query store
📝 WalkthroughWalkthroughThis PR introduces a TimeRangeSlicerControl—a new interactive UI component for time-range selection—into QueryStoreGridControl. It includes the control implementation, integration logic, supporting data models, service methods to fetch hourly aggregated metrics, and dark theme styling resources. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs (1)
38-42: Consider unsubscribing from SizeChanged event.The
SizeChangedevent handler lambda is subscribed in the constructor but never unsubscribed. Consider using the control's lifecycle events to clean up, or use weak event patterns if the control could be disposed while the parent Border persists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs` around lines 38 - 42, The SizeChanged lambda subscribed in TimeRangeSlicerControl() via SlicerBorder.SizeChanged += (_, _) => Redraw() is never unsubscribed; change this to a named event handler (e.g., OnSlicerBorderSizeChanged) and unsubscribe it when the control is torn down (for Avalonia use Unloaded or override OnDetachedFromVisualTree/OnDetachedFromLogicalTree) or implement IDisposable to remove SlicerBorder.SizeChanged -= OnSlicerBorderSizeChanged so the handler is not kept alive after the control is disposed.src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs (1)
55-56: Consider unsubscribing from event handler on control disposal.
TimeRangeSlicer.RangeChanged += OnTimeRangeChangedcreates a subscription but there's no corresponding unsubscription when the control is disposed. If the control can be recreated during the application lifecycle, this could cause a memory leak or duplicate event handling.♻️ Add cleanup on unload or implement IDisposable
public QueryStoreGridControl(ServerConnection serverConnection, ICredentialService credentialService, string initialDatabase, List<string> databases) { ... TimeRangeSlicer.RangeChanged += OnTimeRangeChanged; + this.Unloaded += (_, _) => TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; TimeRangeSlicer.IsExpanded = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs` around lines 55 - 56, The control subscribes TimeRangeSlicer.RangeChanged += OnTimeRangeChanged but never unsubscribes, so add cleanup by either implementing IDisposable on the QueryStoreGridControl or handling the control's Unloaded event and calling TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; place the unsubscription in the Dispose/Unloaded handler to mirror the subscription and prevent memory leaks or duplicate calls; ensure the Unloaded/Dispose routine is wired up (e.g., register Unloaded in the constructor or implement Dispose and call it from parent) so OnTimeRangeChanged is reliably removed.src/PlanViewer.Core/Services/QueryStoreService.cs (2)
354-356: UnusedorderByMetricparameter.The
orderByMetricparameter is declared but never referenced in the SQL query or method body. The query always returns all metrics regardless of which one is passed. Either remove the parameter or use it to filter/sort the results.♻️ Option 1: Remove unused parameter
public static async Task<List<QueryStoreTimeSlice>> FetchTimeSliceDataAsync( - string connectionString, string orderByMetric = "cpu", + string connectionString, CancellationToken ct = default)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PlanViewer.Core/Services/QueryStoreService.cs` around lines 354 - 356, The method FetchTimeSliceDataAsync declares orderByMetric but never uses it; either remove the parameter from the signature and update all callers, or apply it to the SQL query: modify the query built in FetchTimeSliceDataAsync (the command text / sql variable passed to SqlCommand) to accept a parameter (e.g., `@orderByMetric`) and use it to filter or order results (e.g., add "WHERE MetricName = `@orderByMetric`" or "ORDER BY CASE WHEN `@orderByMetric` = 'cpu' THEN cpu WHEN `@orderByMetric` = 'memory' THEN memory END") and add the parameter to the command (e.g., command.Parameters.AddWithValue("orderByMetric", orderByMetric)) to avoid SQL injection; keep the chosen behavior consistent with callers and update any unit tests.
374-401: Consider ordering ascending in SQL to avoid in-memory reversal.The query orders
DESCthen reverses the list in memory. You could simplify by orderingASCin the SQL query directly.♻️ Proposed change
-ORDER BY bucket_hour DESC;"; +ORDER BY bucket_hour ASC;"; ... - // Return in chronological order - rows.Reverse(); return rows;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/PlanViewer.Core/Services/QueryStoreService.cs` around lines 374 - 401, The SQL currently orders bucket_hour DESC and the code reads rows into List<QueryStoreTimeSlice> then calls rows.Reverse() to return chronological order; change the SQL ORDER BY clause to ASC (order by bucket_hour ASC) so rows are produced chronologically and remove the in-memory reversal (the rows.Reverse() call) in the method in QueryStoreService (the reader loop that builds QueryStoreTimeSlice and the subsequent rows.Reverse()). Ensure any comments about chronological order are updated if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 481-491: span may be zero causing a division-by-zero when
computing pivotInRange; before computing pivotInRange (using _rangeStart,
_rangeEnd, span, and MinNormInterval) guard against span == 0 (or <=
double.Epsilon) by replacing span with a safe non-zero value (e.g.
Math.Max(span, MinNormInterval) or set pivotInRange to 0.5 when span is zero) so
pivotInRange and subsequent newStart/newEnd calculations cannot divide by zero;
apply the guard immediately after computing span and use the guarded value for
pivotInRange and newSpan calculations.
In `@src/PlanViewer.App/Themes/DarkTheme.axaml`:
- Around line 55-66: SlicerHandleHoverBrush is defined but unused; either remove
the <SolidColorBrush x:Key="SlicerHandleHoverBrush" .../> resource or
document/attach it to the slicer handle hover state: delete the
SlicerHandleHoverBrush resource line if not needed, or add a comment above it
clarifying it’s reserved for future hover styling and update the Slicer handle
Style/VisualState (the control that uses
SlicerHandleBrush/SlicerHandleHoverBrush) to reference StaticResource
SlicerHandleHoverBrush for the PointerOver/MouseOver state.
---
Nitpick comments:
In `@src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs`:
- Around line 55-56: The control subscribes TimeRangeSlicer.RangeChanged +=
OnTimeRangeChanged but never unsubscribes, so add cleanup by either implementing
IDisposable on the QueryStoreGridControl or handling the control's Unloaded
event and calling TimeRangeSlicer.RangeChanged -= OnTimeRangeChanged; place the
unsubscription in the Dispose/Unloaded handler to mirror the subscription and
prevent memory leaks or duplicate calls; ensure the Unloaded/Dispose routine is
wired up (e.g., register Unloaded in the constructor or implement Dispose and
call it from parent) so OnTimeRangeChanged is reliably removed.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs`:
- Around line 38-42: The SizeChanged lambda subscribed in
TimeRangeSlicerControl() via SlicerBorder.SizeChanged += (_, _) => Redraw() is
never unsubscribed; change this to a named event handler (e.g.,
OnSlicerBorderSizeChanged) and unsubscribe it when the control is torn down (for
Avalonia use Unloaded or override
OnDetachedFromVisualTree/OnDetachedFromLogicalTree) or implement IDisposable to
remove SlicerBorder.SizeChanged -= OnSlicerBorderSizeChanged so the handler is
not kept alive after the control is disposed.
In `@src/PlanViewer.Core/Services/QueryStoreService.cs`:
- Around line 354-356: The method FetchTimeSliceDataAsync declares orderByMetric
but never uses it; either remove the parameter from the signature and update all
callers, or apply it to the SQL query: modify the query built in
FetchTimeSliceDataAsync (the command text / sql variable passed to SqlCommand)
to accept a parameter (e.g., `@orderByMetric`) and use it to filter or order
results (e.g., add "WHERE MetricName = `@orderByMetric`" or "ORDER BY CASE WHEN
`@orderByMetric` = 'cpu' THEN cpu WHEN `@orderByMetric` = 'memory' THEN memory END")
and add the parameter to the command (e.g.,
command.Parameters.AddWithValue("orderByMetric", orderByMetric)) to avoid SQL
injection; keep the chosen behavior consistent with callers and update any unit
tests.
- Around line 374-401: The SQL currently orders bucket_hour DESC and the code
reads rows into List<QueryStoreTimeSlice> then calls rows.Reverse() to return
chronological order; change the SQL ORDER BY clause to ASC (order by bucket_hour
ASC) so rows are produced chronologically and remove the in-memory reversal (the
rows.Reverse() call) in the method in QueryStoreService (the reader loop that
builds QueryStoreTimeSlice and the subsequent rows.Reverse()). Ensure any
comments about chronological order are updated if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6a2dc07-e3d3-4511-8a94-766513e95487
📒 Files selected for processing (7)
src/PlanViewer.App/Controls/QueryStoreGridControl.axamlsrc/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cssrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axamlsrc/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cssrc/PlanViewer.App/Themes/DarkTheme.axamlsrc/PlanViewer.Core/Models/QueryStoreTimeSlice.cssrc/PlanViewer.Core/Services/QueryStoreService.cs
| var span = _rangeEnd - _rangeStart; | ||
| var minInterval = MinNormInterval; | ||
|
|
||
| // Zoom in (wheel up) → shrink span; zoom out (wheel down) → expand span | ||
| var zoomFactor = e.Delta.Y > 0 ? 0.85 : 1.0 / 0.85; | ||
| var newSpan = Math.Clamp(span * zoomFactor, minInterval, 1.0); | ||
|
|
||
| // Keep the pivot point stable in the range | ||
| var pivotInRange = (pivot - _rangeStart) / span; | ||
| var newStart = pivot - pivotInRange * newSpan; | ||
| var newEnd = newStart + newSpan; |
There was a problem hiding this comment.
Potential division by zero if span is zero.
At line 489, span is used as a divisor. If _rangeStart equals _rangeEnd, this would cause a division by zero. While MinNormInterval should prevent this during normal operation, consider adding a guard.
🛡️ Proposed fix
var span = _rangeEnd - _rangeStart;
var minInterval = MinNormInterval;
+ if (span <= 0) return; // Guard against zero span
// Zoom in (wheel up) → shrink span; zoom out (wheel down) → expand span🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Controls/TimeRangeSlicerControl.axaml.cs` around lines 481
- 491, span may be zero causing a division-by-zero when computing pivotInRange;
before computing pivotInRange (using _rangeStart, _rangeEnd, span, and
MinNormInterval) guard against span == 0 (or <= double.Epsilon) by replacing
span with a safe non-zero value (e.g. Math.Max(span, MinNormInterval) or set
pivotInRange to 0.5 when span is zero) so pivotInRange and subsequent
newStart/newEnd calculations cannot divide by zero; apply the guard immediately
after computing span and use the guarded value for pivotInRange and newSpan
calculations.
| <!-- Time Range Slicer theme resources --> | ||
| <SolidColorBrush x:Key="SlicerBackgroundBrush" Color="#15171C"/> | ||
| <SolidColorBrush x:Key="SlicerChartLineBrush" Color="#2EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerChartFillBrush" Color="#332EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerOverlayBrush" Color="#99000000"/> | ||
| <SolidColorBrush x:Key="SlicerSelectedBrush" Color="#22FFFFFF"/> | ||
| <SolidColorBrush x:Key="SlicerHandleBrush" Color="#E4E6EB"/> | ||
| <SolidColorBrush x:Key="SlicerHandleHoverBrush" Color="#2EAEF1"/> | ||
| <SolidColorBrush x:Key="SlicerBorderBrush" Color="#3A3D45"/> | ||
| <SolidColorBrush x:Key="SlicerLabelBrush" Color="#99E4E6EB"/> | ||
| <SolidColorBrush x:Key="SlicerToggleBrush" Color="#E4E6EB"/> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if SlicerHandleHoverBrush is used anywhere
rg -n "SlicerHandleHoverBrush" --type-add 'cs:*.cs' --type-add 'axaml:*.axaml' --type cs --type axamlRepository: erikdarlingdata/PerformanceStudio
Length of output: 191
Remove unused SlicerHandleHoverBrush resource or document its intended use.
The slicer theme resources are well-organized and consistently named. However, SlicerHandleHoverBrush (line 62) is defined but never used anywhere in the codebase. Either remove it or add a comment clarifying if it's reserved for a future hover state implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/PlanViewer.App/Themes/DarkTheme.axaml` around lines 55 - 66,
SlicerHandleHoverBrush is defined but unused; either remove the <SolidColorBrush
x:Key="SlicerHandleHoverBrush" .../> resource or document/attach it to the
slicer handle hover state: delete the SlicerHandleHoverBrush resource line if
not needed, or add a comment above it clarifying it’s reserved for future hover
styling and update the Slicer handle Style/VisualState (the control that uses
SlicerHandleBrush/SlicerHandleHoverBrush) to reference StaticResource
SlicerHandleHoverBrush for the PointerOver/MouseOver state.
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Thanks for the creative work on the slicer control — the interactive drag/zoom UX is well done. However, this PR has several issues that need to be addressed before it can be merged.
Must fix
-
Do not remove HoursBackBox or hardcode
hoursBack = 8760— The existing hours-back control limits the server-side query. Replacing it with a client-side slicer that fetches a full year of data will be extremely slow on production databases with large Query Store data sets. The slicer should be an additional client-side filter on top of the existing server-side time bound, not a replacement for it. Restore HoursBackBox. -
FetchTimeSliceDataAsynchas no time filter — The SQL scans ALL ofsys.query_store_runtime_statswith no WHERE clause on time. On databases with months of Query Store data, this is a heavy query. Add a time bound (e.g., match the hours-back value, or cap at 30 days). -
Do not auto-fetch on constructor —
Dispatcher.UIThread.Post(() => Fetch_Click(...))changes the existing behavior where users set up filters before clicking Fetch. This is a UX change that wasn't discussed. Remove the auto-fetch. -
Restore hoursBack parameter on ViewHistory_Click — The PR removes the hours-back pass-through to the history window, silently defaulting to 24. The user's chosen value should still be passed.
-
Slicer filters by
LastExecutedUtcbut chart shows interval start times — These are different times. A query's last execution time can be well after the interval start. This mismatch means the slicer's visual range won't accurately reflect what gets filtered in/out of the grid. Consider filtering by the same time dimension the chart displays.
Suggestions
-
orderByMetricparameter is accepted but unused inFetchTimeSliceDataAsync— either use it or remove it. -
Errors silently swallowed in
LoadTimeSlicerDataAsync— at minimum surface a status message if the slicer data fails to load. -
Slicer placement above the toolbar is unusual — most apps put the toolbar at the top. Consider placing the slicer between the toolbar and the grid, or making it collapsible (you already have the toggle, which is good).
The slicer control itself is solid work — just needs to be integrated without removing existing functionality.
2 participants
What does this PR do?
Add a time range filter in the query store with the selected sorting metric as background of the time range graphic selector
Which component(s) does this affect?
How was this tested?
Checklist
dotnet build -c Debug)dotnet test)Summary by CodeRabbit
Release Notes