Add custom time-range selection for graphs and the event timeline - #388
Open
dpage wants to merge 9 commits into
Open
Add custom time-range selection for graphs and the event timeline#388dpage wants to merge 9 commits into
dpage wants to merge 9 commits into
Conversation
The dashboard time selector offers only the five rolling presets, and `/api/v1/metrics/query` enforced that list inline, so an arbitrary incident window could not be requested even though the query layer has always bucketed over an absolute start and end pair. This adds the server half of custom time-range support. `metrics.TimeWindow` and `metrics.ResolveTimeWindow` become the single source of truth for what constitutes a valid window. A `time_range` other than `custom` still delegates to the unchanged `ParseTimeRange`, whilst `custom` requires `time_start` and `time_end` as RFC 3339 timestamps and rejects a missing or unparsable timestamp, an end that is not after the start, a start at or after the present moment, and a span longer than 366 days; the span cap matters because the bucket width derives from the span, so an unbounded window would be a resource-exhaustion surface rather than merely a slow query. An end in the future is clamped to now instead of rejected, since a picker set to the current day routinely overshoots by a few minutes. `QueryTimeSeries` now takes the resolved `TimeWindow` rather than a time-range string, so resolution happens once at the HTTP boundary; the `timeSeriesQueryFunc` seam and its test fakes follow. The handler maps any resolution error to a 400 carrying the resolver's own message, and keeps the historical `1h` default when `time_range` is absent. The performance-summary and database-summary handlers keep their own preset checks, which are out of scope here. The OpenAPI specification documents `time_start` and `time_end` on `/metrics/query` and advertises `custom` on `time_range`, and the static `openapi.json` is regenerated to match.
Issue #345 asked for an arbitrary start and end time on the dashboard, so that a user can line the graphs up with a known incident window rather than a rolling window ending now. This is the client half of that work: the picker, the context plumbing, and the selector control. The TimeRange union gains a 'custom' member, and setCustomTimeRange now switches the range to 'custom' rather than preserving the previous preset; the old behaviour left the query layer with no way to tell that an arbitrary window was in force. The existing context test that asserted the preserved range has been updated to match, because this is a deliberate change to tested behaviour. Auto-refresh is suspended whilst a custom range is active, since re-fetching a fixed historical window returns identical data on every poll. The suspension is derived rather than stored, exposed as autoRefreshSuspended, so the user's own autoRefresh.enabled preference survives untouched and resumes as soon as they return to a preset; the selector surfaces the pause with a tooltipped indicator, as there is no other UI control for the dashboard's auto-refresh today. CustomTimeRangePopover reads no context and takes the current window plus an onApply callback, so the event timeline can reuse it in a follow-on task despite keeping its range state elsewhere. Apply stays disabled until both bounds parse and the end is after the start. @mui/x-date-pickers and dayjs are new runtime dependencies, with LocalizationProvider and AdapterDayjs installed at the application root.
This is the second half of the client work for issue #345, wiring the window that the picker now produces through to the data layers so that the dashboard graphs and the event timeline actually honour it. useMetrics reads customStart and customEnd from DashboardContext itself and hands them to buildMetricsUrl, which emits time_start and time_end only for the 'custom' range; the bounds deliberately stay out of MetricQueryParams, so none of the twenty-odd consumer components change and they carry on passing timeRange.range as before. Both values join the fetch effect's dependencies and the initialLoadDoneRef reset list, because without the first a newly applied window would never refetch and without the second the loading state would not show whilst it did. A custom range whose bounds have somehow gone missing is a transient state the server answers with a 400, so the hook skips the request altogether and leaves whatever data and error state is already present. The timeline keeps its own range state, and its selector gains a Custom toggle that opens the very same popover; an applied window is described in the toggle's title attribute rather than in its label, because the timeline toolbar is considerably tighter on space than the dashboard selector. Custom windows are not written to localStorage, so the stored preset survives untouched and a reload returns to it rather than to a stale historical window. The bounds calculation that EventTimeline/utils.ts and useTimelineEvents each carried a diverging copy of now lives once, in utils/timelineRange, which both call; the range types move there with it and are re-exported from the hook for existing callers. Rather than teaching two switch statements about arbitrary windows, there is now one function that knows about them, and the bounds the canvas draws can no longer drift from the bounds the API is queried with.
The three preceding commits added custom time-range support to the server and the web client, so this documents it across the API reference, the dashboards user guide, the changelog, and the in-app help panel. The API reference gains a Metric Time Windows section covering time_range=custom together with the RFC 3339 time_start and time_end parameters, the five validation rules the server enforces, the reasoning behind the future-end clamp and the 366-day span cap, and a curl example. The section is explicit that only /metrics/query supports a custom window, because performance-summary and database-summaries still accept the presets alone and top-queries has no time dimension at all. The dashboards guide describes the picker, the auto-refresh pause and why it exists, the fact that the window is neither persisted nor reflected in the URL, and the event timeline's independent range control. It also states plainly which views honour the selector: the charts and the timeline do, including the two charts on the query detail overlay, whilst the query leaderboards and the summary tiles do not. That distinction is the one thing a reader is most likely to get wrong, and issue #387 tracks closing the gap. The help panel's Time Range Selector, Auto-Refresh, and Event Timeline entries were all stale in the same way, listing the five presets and promising unconditional refreshes, so each is corrected and a Custom Time Range entry is added alongside them. docs/admin-guide/api/openapi.json needed no change; regenerating it with `make openapi` produced no diff, confirming the artefact committed in 2e2d420 is current.
WalkthroughThe pull request adds custom absolute time-window selection to dashboard charts and the event timeline, propagates bounds through metrics requests, resolves and validates windows at the server boundary, suspends dashboard auto-refresh for custom ranges, and updates API and user documentation. ChangesCustom time-window selection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 273 complexity · 33 duplication
Metric Results Complexity 273 Duplication 33
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
client/src/components/EventTimeline/__tests__/config.test.ts (1)
21-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
vi.clearAllMocks()does not drop the throwinggetItemimplementation.
clearAllMocksonly clears calls/results, so themockImplementationthat throws at Line 49 survives into any test added after it. Prefer resetting implementations in the hook.♻️ Suggested change
beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); });Note
resetAllMocksalso clears return values, so each test must set its ownmockReturnValue(they already do, except the options-only test which never reads storage).Also applies to: 48-54
🤖 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 `@client/src/components/EventTimeline/__tests__/config.test.ts` around lines 21 - 23, Update the test setup around beforeEach and the throwing getItem mock to reset mock implementations between tests instead of only clearing calls. Use resetAllMocks, ensuring each test re-establishes any required mockReturnValue while leaving the options-only test independent of storage.client/src/components/Dashboard/CustomTimeRangePopover.tsx (1)
55-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider mirroring the server's max-span check here for immediate feedback.
isWindowValidonly checks presence andend > start. Per the PR objectives, the server resolver rejects spans over 366 days; surfacing that same limit here would let users see the problem before submitting, instead of relying on a later API failure.♻️ Suggested addition
const isWindowValid = (start: Dayjs | null, end: Dayjs | null): boolean => { if (start === null || end === null) { return false; } if (!start.isValid() || !end.isValid()) { return false; } - return end.isAfter(start); + if (!end.isAfter(start)) { + return false; + } + return end.diff(start, 'day') <= 366; };🤖 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 `@client/src/components/Dashboard/CustomTimeRangePopover.tsx` around lines 55 - 63, Update isWindowValid to enforce the same maximum 366-day span as the server after validating both dates and their ordering. Return false when the interval exceeds that limit while preserving the existing null, invalid-date, and valid-range behavior.
🤖 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.
Inline comments:
In `@client/src/components/Dashboard/TimeRangeSelector.tsx`:
- Around line 149-155: Make the auto-refresh pause indicator keyboard-focusable
in the autoRefreshSuspended rendering block by assigning the
PauseCircleOutlineIcon a suitable tabIndex so its Tooltip can be triggered
without pointer input. Preserve the existing tooltip text and styling.
In `@client/src/components/EventTimeline/TimelineHeader.tsx`:
- Around line 185-192: Cap custom timeline ranges before they reach the event
query flow, using the same maximum-span constant or validation behavior as the
metrics endpoint. Update the custom range handling around TimelineHeader and its
apply/query path, preserving valid ranges while rejecting or constraining spans
exceeding the configured limit.
In `@docs/admin-guide/api/reference.md`:
- Line 215: Shorten the descriptions in the Markdown table rows for GET
/api/v1/metrics/query and the related rows around the custom-window section so
every changed row is no more than 79 characters. Preserve the endpoint meaning
and keep each table row on a single line without splitting Markdown syntax.
In `@docs/user-guide/dashboards/index.md`:
- Around line 80-85: The dashboard documentation must distinguish the range
selectors: update the “Views That Honour the Selector” text in
docs/user-guide/dashboards/index.md lines 80-85 to state that time-series charts
use the dashboard range while the event timeline uses its own independent range;
also update client/src/components/HelpPanel/pages/MonitoringPage.tsx lines
184-185 to describe the timeline’s “own selected time range.”
---
Nitpick comments:
In `@client/src/components/Dashboard/CustomTimeRangePopover.tsx`:
- Around line 55-63: Update isWindowValid to enforce the same maximum 366-day
span as the server after validating both dates and their ordering. Return false
when the interval exceeds that limit while preserving the existing null,
invalid-date, and valid-range behavior.
In `@client/src/components/EventTimeline/__tests__/config.test.ts`:
- Around line 21-23: Update the test setup around beforeEach and the throwing
getItem mock to reset mock implementations between tests instead of only
clearing calls. Use resetAllMocks, ensuring each test re-establishes any
required mockReturnValue while leaving the options-only test independent of
storage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67ec2b5f-0b5c-4784-be2c-d662de33d51f
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
.claude/golang-expert/metrics-queries.mdclient/package.jsonclient/src/App.tsxclient/src/__tests__/App.test.tsxclient/src/components/Dashboard/CustomTimeRangePopover.tsxclient/src/components/Dashboard/TimeRangeSelector.tsxclient/src/components/Dashboard/__tests__/CustomTimeRangePopover.test.tsxclient/src/components/Dashboard/__tests__/TimeRangeSelector.test.tsxclient/src/components/Dashboard/index.tsclient/src/components/Dashboard/styles.tsclient/src/components/Dashboard/types.tsclient/src/components/EventTimeline/TimelineHeader.tsxclient/src/components/EventTimeline/__tests__/TimelineCustomRange.test.tsxclient/src/components/EventTimeline/__tests__/config.test.tsclient/src/components/EventTimeline/index.tsxclient/src/components/EventTimeline/utils.tsclient/src/components/HelpPanel/pages/MonitoringPage.tsxclient/src/components/__tests__/EventTimeline.test.tsxclient/src/contexts/DashboardContext.tsxclient/src/contexts/__tests__/DashboardContext.test.tsxclient/src/hooks/__tests__/useMetrics.test.tsclient/src/hooks/useMetrics.tsclient/src/hooks/useTimelineEvents.tsclient/src/utils/__tests__/timelineRange.test.tsclient/src/utils/timelineRange.tsdocs/admin-guide/api/openapi.jsondocs/admin-guide/api/reference.mddocs/changelog.mddocs/user-guide/dashboards/index.mdserver/src/internal/api/metrics_handlers.goserver/src/internal/api/metrics_handlers_test.goserver/src/internal/api/openapi.goserver/src/internal/api/openapi_test.goserver/src/internal/metrics/query.goserver/src/internal/metrics/query_timeseries_db_test.goserver/src/internal/metrics/timewindow_test.go
CodeRabbit raised two valid points against e4fb378 on PR #388, and this addresses both. First, the Markdown rows I added or changed in the API reference ran past the project's 79-character limit. Table rows cannot be wrapped without breaking the syntax, so the wording is shortened instead: the endpoint summary now reads "Query metrics for preset or custom windows", and the parameter table drops to short cells. The list of preset values that the `time_range` cell used to carry has moved up into the prose above the table, which wraps freely, so nothing is lost. The neighbouring rows I did not touch keep their existing lengths. Second, both the dashboards guide and the help panel could be read as saying the event timeline follows the dashboard time range selector. The timeline holds its own range state, so applying a custom window on the dashboard leaves the timeline untouched and vice versa; both descriptions now say so explicitly. The same misreading was available in two further places, so those are corrected as well: the Time Range Selector introduction claimed the selector "controls the time window for all charts", and the changelog entry lumped the charts and the timeline together as honouring one window.
The pause indicator added for the custom time-range selector was unreachable by keyboard and, because MUI's SvgIcon marks itself aria-hidden unless given titleAccess, its accessible name existed in the markup but was never exposed to assistive technology. The Tooltip title was therefore the only explanation available, and only to users with a pointer. The indicator now hangs off a focusable span carrying role="img" and an aria-label with the same wording as the tooltip, so screen readers announce the paused state without the tooltip being involved at all, whilst the span's tabIndex lets a sighted keyboard user focus it and read the tooltip too. Anchoring on a span rather than the icon avoids the inconsistent browser handling of tabindex on SVG elements, and inline-flex keeps the rendered layout byte-for-byte identical. Addresses review feedback on #388; refs #345.
`/api/v1/timeline/events` only ever checked that `end_time` was after `start_time`, which was harmless whilst the event timeline could send nothing but the five rolling presets, the largest of which was 30 days. The custom-range picker added earlier in this pull request now sends arbitrary absolute timestamps, so the endpoint can be handed a decade-long window and will happily union-scan every history table for it; that is a resource-exhaustion surface this pull request itself opens. The handler therefore rejects a span longer than `metrics.MaxCustomTimeSpan` with a 400 carrying the same message the metrics resolver uses, `invalid time range: span must not exceed 366 days`. The constant is imported rather than duplicated, which costs nothing because the `api` package already depends on `metrics` for the query layer, so there is no new dependency edge and no cycle. Nothing else in the handler moves, and the metrics endpoint is untouched. The new table-driven test covers the boundary in both directions: a span exactly at the cap is accepted whilst one second beyond it is rejected. Bringing the file to the 90% floor also needed the handler's post-validation paths, which no test reached before, so a Postgres-backed integration suite now drives the visibility filter (superuser bypass, an unfiltered request restricted to the visible set, a visible and an invisible `connection_id`, an intersected `connection_ids` list, an empty visible set), both 500 paths, and the configured branch of `RegisterRoutes`. `timeline_handlers.go` moves from 55.7% to 100% line coverage. The OpenAPI parameter descriptions record the limit and the static specification is regenerated.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server/src/internal/api/timeline_handlers_integration_test.go`:
- Around line 104-161: Register a test cleanup immediately after pgxpool.New
succeeds in newTimelineTestEnv, using t.Cleanup to close the pool on every
subsequent failure path, including insertTimelineConnection and
insertTimelineClearedAlert. Keep the existing explicit cleanup behavior
consistent and avoid double-closing the pool during normal teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8fa59bff-6c81-452c-858e-1cd3460b6184
📒 Files selected for processing (12)
.claude/golang-expert/metrics-queries.mdclient/src/components/Dashboard/TimeRangeSelector.tsxclient/src/components/Dashboard/__tests__/TimeRangeSelector.test.tsxclient/src/components/HelpPanel/pages/MonitoringPage.tsxdocs/admin-guide/api/openapi.jsondocs/admin-guide/api/reference.mddocs/changelog.mddocs/user-guide/dashboards/index.mdserver/src/internal/api/openapi.goserver/src/internal/api/timeline_handlers.goserver/src/internal/api/timeline_handlers_integration_test.goserver/src/internal/api/timeline_handlers_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/admin-guide/api/openapi.json
- client/src/components/HelpPanel/pages/MonitoringPage.tsx
- client/src/components/Dashboard/TimeRangeSelector.tsx
- client/src/components/Dashboard/tests/TimeRangeSelector.test.tsx
- .claude/golang-expert/metrics-queries.md
- docs/user-guide/dashboards/index.md
- docs/admin-guide/api/reference.md
- docs/changelog.md
The Go linter enforces US spelling, so the subtest name 'visible connection_id is honoured' failed the misspell check on CI whilst passing locally. Reworded to 'respected', which sidesteps the en-GB/en-US split rather than writing a spelling that sits oddly against the rest of the project's prose.
CodeRabbit spotted on PR #388 that newTimelineTestEnv leaked its pgx pool whenever one of the seeding helpers failed, because those helpers call t.Fatalf, which runs runtime.Goexit and so prevents the helper from ever returning the cleanup closure that the caller was expected to defer. The teardown, and with it pool.Close, therefore never ran. I have moved the whole teardown onto t.Cleanup, registering the pool close as soon as the pool exists so that every later exit path is covered, and dropping the returned closure along with the now-redundant explicit pool.Close calls on the Ping and schema paths; each resource is thus released exactly once. The callers lose their defer lines but the assertions are untouched. Refs #345
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Users can now select an arbitrary start and end time for dashboard
graphs and the event timeline, alongside the existing
1h,6h,24h,7dand30dpresets, which continue to behave exactly asbefore.
The issue described this as primarily front-end wiring, on the grounds
that the metrics query layer already buckets over an arbitrary
timeStart/timeEndpair. That was true of the query layer but not ofthe HTTP boundary, so there is server work here as well:
/api/v1/metrics/querynow acceptstime_range=customtogether withRFC 3339
time_startandtime_end, resolved by a single newmetrics.ResolveTimeWindow, andQueryTimeSeriestakes analready-validated
TimeWindowrather than resolving a preset stringitself.
The central design decision on the client is that the window's bounds
do not travel through
MetricQueryParams.useMetricsalreadyconsumed
DashboardContextforrefreshTrigger, so it readscustomStart/customEndfrom there directly; consequently none of theroughly twenty consumer components changed, and they keep passing the
range they already passed.
Auto-refresh is suspended whilst a custom window is active, since
re-polling a fixed historical window returns identical data. The
suspension is derived rather than stored, so the user's own
auto-refresh preference is untouched and resumes on returning to a
preset. The window is held in memory only: it is neither persisted nor
reflected in the URL, matching how the preset selection already
behaves.
Validation rejects a start at or after the present, a non-positive
span, and spans beyond 366 days (the bucket-width heuristic derives
from the span, so an unbounded window is a resource-exhaustion
surface); an end in the future is clamped to now rather than rejected,
because a picker set to the current day routinely overshoots by
minutes.
The event timeline keeps its own independent range state but gains the
same picker. Its display-side bounds logic and the calculation in
useTimelineEventswere two diverged copies of the same thing, onlyone of which understood custom ranges; both now call a single shared
helper in
client/src/utils/timelineRange.ts.@mui/x-date-pickersis pinned to^7because v8 requires MUI 6+ andthis project is on
@mui/material5.18.Scope
Deliberately out of scope, tracked in #387:
The query leaderboards and
QueryDetail's headline statistics. Thetop-queriesendpoint has no time dimension at all; its SQL pinscollected_atto the latest snapshot, and giving it a window needsdelta aggregation over cumulative
pg_stat_statementscounters,which changes preset behaviour too.
QueryDetailis therefore splittoday: its two charts follow the selected range, its headline
statistics report the latest sample. The documentation says so
explicitly rather than glossing over it.
The performance-summary and database-summary tiles, which hardcode
time_range=24hand ignore the selector entirely.Test plan
gofmt -landgo vet ./...clean across the server.make test-all: everything passes bar two pre-existing failuresin
internal/tools(
TestStoreMemoryGeneratesEmbeddingIntegrationandTestRecallMemoriesGeneratesQueryEmbeddingIntegration), whichreproduce identically on
mainwithexpected 3 dimensions, not 4000from a stalevector(3)fixture and touch no code in thischange.
lcov.info: elevenof twelve at 100% line coverage,
EventTimeline/utils.tsat97.26% (the two uncovered lines are pre-existing
formatEventTimebranches). Server side,ResolveTimeWindowisat 100% and
handleMetricsQueryrose from 73.4% to 96.9%.go test ./internal/api/ -run OpenAPI -vpasses, including a newtest asserting the parameters landed on
/metrics/queryandnot on the performance-summary path.
openapi.jsonregenerated viamake openapi;mkdocs build --strictclean.playwright-cliis still outstanding: itneeds the dev server and web client running, which are started
manually, and they serve the main checkout rather than this
worktree.
Closes #345
Summary by CodeRabbit
time_range=customusing RFC3339time_start/time_end.Added during review
One substantive change came out of the review round and is worth
calling out, because it is a fix to something this PR itself exposed.
/api/v1/timeline/eventsonly ever enforcedend_time > start_time;before this change the timeline UI could send nothing but presets, the
largest being 30 days, so an unbounded span was unreachable in
practice. Giving the timeline a custom-range picker put a decade-long
window one click away, so the endpoint now rejects spans beyond
metrics.MaxCustomTimeSpan, importing the same constant rather thanredeclaring the number.
The other review fixes were an accessibility correction to the paused
indicator (MUI v5's
SvgIconrendersaria-hidden="true"unless giventitleAccess, so although the tooltip did inject anaria-label, theelement was pruned from the accessibility tree and never announced),
markdown line lengths on the rows this PR changed, and wording in the
docs and help panel that implied the event timeline follows the
dashboard selector when the two are genuinely independent.
Two pre-existing issues found along the way are recorded on #387
rather than fixed here:
ValidateTimeRangeacceptsend_timeequal tostart_timedespite its error text, and the undocumentedindex_nameparameter.