Skip to content

refactor: uplift metrics#4499

Open
jog1t wants to merge 1 commit intomainfrom
03-24-refactor_uplift_metrics
Open

refactor: uplift metrics#4499
jog1t wants to merge 1 commit intomainfrom
03-24-refactor_uplift_metrics

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Mar 24, 2026

Description

This pull request implements a dual-chart system for metrics visualization with brush-based zooming functionality. The changes introduce an overview chart showing 7 days of data with a brush selector, and a detail chart that displays the selected time range with higher resolution data.

Key changes include:

  • Added chart synchronization context with default 24-hour brush range and 7-day maximum range
  • Implemented separate hooks for overview and detail metrics data fetching
  • Enhanced timestamp parsing to normalize date formats for consistent browser compatibility
  • Added brush range clamping to prevent excessive data requests
  • Removed time range selector in favor of interactive brush-based navigation
  • Improved chart layout with proper spacing and legend positioning
  • Added placeholder data support to prevent chart flickering during updates

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

The changes have been tested by:

  • Verifying brush selection updates the detail chart with appropriate resolution
  • Testing brush range clamping with selections exceeding the 7-day maximum
  • Confirming timestamp normalization works across different date formats
  • Validating smooth transitions between overview and detail data loading states

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link

railway-app bot commented Mar 24, 2026

🚅 Deployed to the rivet-pr-4499 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Mar 24, 2026 at 8:34 pm
frontend-inspector 😴 Sleeping (View Logs) Web Mar 24, 2026 at 8:25 pm
frontend-cloud 😴 Sleeping (View Logs) Web Mar 24, 2026 at 8:24 pm
kitchen-sink ❌ Build Failed (View Logs) Web Mar 24, 2026 at 8:16 pm
ladle ❌ Build Failed (View Logs) Web Mar 24, 2026 at 8:14 pm
mcp-hub ✅ Success (View Logs) Web Mar 24, 2026 at 8:13 pm

Copy link
Contributor Author

jog1t commented Mar 24, 2026

@claude
Copy link

claude bot commented Mar 24, 2026

PR Review: refactor: uplift metrics

Overall this is a solid refactor — the two-tier overview/detail architecture is a clear improvement, and several correctness bugs are fixed along the way. A few issues worth addressing before merge.


Bugs / Issues

1. N+1 detail query per chart card (high priority)

Each MetricsChart and NamespaceMetricsChart independently calls useNamespaceDetailMetrics / useSingleNamespaceDetailMetrics. With e.g. 4 metric cards on the page, every brush interaction fires 4 identical API requests instead of 1. The detail query should be hoisted to the page level (alongside the overview query) and passed down as a prop, the same way overviewData is passed.

// metrics-page.tsx — hoist this:
const { data: detailData, ... } = useNamespaceDetailMetrics({ namespaces: selectedNamespaces, brushDomain });

// then pass detailData/detailStartAt/detailEndAt/detailResolution as props to MetricsChart

2. Stale overview window after long sessions

startAt / endAt for the overview are computed once on mount with useMemo(…, []). After the page has been open for hours the overview window drifts: new data points generated after mount won't appear because the query range is anchored to the initial load time. The 30s refetchInterval refreshes values but cannot show data outside the fixed window.

Consider computing the window inside a callback that runs on each refetch, or at minimum include a dependency on the refetch timestamp.

3. Dead exports (minor)

DETAIL_RANGE_MS and DETAIL_RESOLUTION are exported from hooks.ts (lines 15–16) but never imported anywhere in the codebase. Either use them or remove them.


Minor Issues

4. Double clamping of MAX_BRUSH_RANGE_MS

onBrushEnd in visx-brush-chart.tsx (line 108–110) clamps the range to MAX_BRUSH_RANGE_MS before calling setBrushDomain, and then clampBrushDomain in chart-sync-context.tsx clamps again. The context-level clamp is the right authoritative place; the one in onBrushEnd can be removed.

5. Resolution target not met for large brush selections

detailRangeFromBrush aims for "at most ~200 points", but for brush ranges > ~1.85 days the fallback resolution of 800s yields significantly more points (e.g. 7-day brush → ~756 points). The comment is misleading. Not a functional bug but worth documenting accurately.

6. Extra blank lines

There are double blank lines after the buildSeries function in metrics-chart.tsx (line 78) and after parseApiSeriesZeroFilled in namespace-metrics-chart.tsx (line 101). Minor style nit.


Positive Changes

  • keepPreviousData on queries prevents flash-of-empty-content during brush transitions. Good call.
  • ISO 8601 timestamp normalization (replace(" ", "T") + "Z") is a real browser-compat fix — Date.parse("2024-01-01 00:00:00") is undefined behavior in Safari.
  • initialBrushPositionRef pattern correctly prevents the brush from snapping back to the default after data loads. The render-time initialization is the right approach here (same pattern React docs use for lazy refs).
  • Removing the "No data available" empty state in favor of showing the chart frame immediately is a better UX.
  • buildSeries / parseApiSeriesZeroFilled extraction into standalone functions makes the useMemo deps cleaner and the components easier to read.

@jog1t jog1t marked this pull request as ready for review March 24, 2026 20:17
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