Skip to content

refactor: sandbox details header + live indicator determination #288

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ben-fornefeld wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: sandbox details header + live indicator determination #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/features/dashboard/layouts/details-row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,15 @@ export function DetailsItem({ label, children, ...props }: DetailItemProps) {

interface DetailsRowProps {
children: ReactNode
className?: string
}

export function DetailsRow({ children }: DetailsRowProps) {
export function DetailsRow({ children, className }: DetailsRowProps) {
return (
<div className="flex flex-wrap items-center gap-5 md:gap-7">{children}</div>
<div
className={cn('flex flex-wrap items-center gap-5 md:gap-7', className)}
>
{children}
</div>
)
}
59 changes: 30 additions & 29 deletions src/features/dashboard/sandbox/header/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use client'

import type { ReactNode } from 'react'
import { Skeleton } from '@/ui/primitives/skeleton'
import { DetailsItem, DetailsRow } from '../../layouts/details-row'
import { useSandboxContext } from '../context'
Expand All @@ -12,6 +13,12 @@ import StartedAt from './started-at'
import Status from './status'
import TemplateId from './template-id'

interface HeaderDetailItem {
label: string
content: ReactNode
skeletonWidth: string
}

export default function SandboxDetailsHeader() {
const { isRunning, sandboxInfo, isSandboxInfoLoading } = useSandboxContext()

Expand Down Expand Up @@ -39,36 +46,30 @@ export default function SandboxDetailsHeader() {
const renderContent = (content: React.ReactNode, skeletonWidth: string) =>
isInitialLoading ? <Skeleton className={`h-5 ${skeletonWidth}`} /> : content

const items: HeaderDetailItem[] = [
{ label: 'status', content: statusContent, skeletonWidth: 'w-20' },
{ label: 'template', content: templateContent, skeletonWidth: 'w-28' },
{ label: 'metadata', content: metadataContent, skeletonWidth: 'w-20' },
{ label: 'created at', content: createdAtContent, skeletonWidth: 'w-32' },
{ label: timeoutLabel, content: timeoutContent, skeletonWidth: 'w-22' },
{
label: runningLabel,
content: runningDurationContent,
skeletonWidth: 'w-22',
},
{ label: 'CPU', content: cpuSpecContent, skeletonWidth: 'w-20' },
{ label: 'Memory', content: memorySpecContent, skeletonWidth: 'w-20' },
{ label: 'Disk', content: diskSpecContent, skeletonWidth: 'w-20' },
]

return (
<header className="bg-bg relative z-30 w-full p-3 md:p-6">
<DetailsRow>
<DetailsItem label="status">
{renderContent(statusContent, 'w-20')}
</DetailsItem>
<DetailsItem label="template">
{renderContent(templateContent, 'w-28')}
</DetailsItem>
<DetailsItem label="metadata">
{renderContent(metadataContent, 'w-20')}
</DetailsItem>
<DetailsItem label="created at">
{renderContent(createdAtContent, 'w-32')}
</DetailsItem>
<DetailsItem label={timeoutLabel}>
{renderContent(timeoutContent, 'w-22')}
</DetailsItem>
<DetailsItem label={runningLabel}>
{renderContent(runningDurationContent, 'w-22')}
</DetailsItem>
<DetailsItem label="CPU">
{renderContent(cpuSpecContent, 'w-20')}
</DetailsItem>
<DetailsItem label="Memory">
{renderContent(memorySpecContent, 'w-20')}
</DetailsItem>
<DetailsItem label="Disk">
{renderContent(diskSpecContent, 'w-20')}
</DetailsItem>
<header className="bg-bg relative z-30 w-full border-b px-3 py-4 md:px-6 md:py-5">
<DetailsRow className="gap-x-5 gap-y-4 md:gap-x-7 md:gap-y-4">
{items.map((item) => (
<DetailsItem key={item.label} label={item.label}>
{renderContent(item.content, item.skeletonWidth)}
</DetailsItem>
))}
Comment on lines +67 to +72
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

Using item.label as the React key can be unstable because some labels are dynamic (runningLabel, timeoutLabel) and will change as the sandbox state changes. That will cause items to remount and can reset internal state/effects in child components. Prefer a separate stable id field for the key (and keep label purely for display).

Copilot uses AI. Check for mistakes.
</DetailsRow>
</header>
)
Expand Down
16 changes: 5 additions & 11 deletions src/features/dashboard/sandbox/monitoring/components/monitoring-sandbox-metrics-chart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import {
SANDBOX_MONITORING_CHART_FG_VAR,
SANDBOX_MONITORING_CHART_LINE_WIDTH,
SANDBOX_MONITORING_CHART_STROKE_VAR,
SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS,
} from '../utils/constants'
import { ChartOverlayLayer } from './chart-overlays'
import { useChartOverlays } from './use-chart-overlays'
Expand All @@ -60,7 +59,6 @@ const CHART_CONNECTOR_LINE_OPACITY = 0.8
const CHART_OUT_OF_BRUSH_ALPHA = 0.25
const CHART_AXIS_LABEL_FONT_SIZE = 12
const CHART_Y_AXIS_SCALE_FACTOR = 1.5
const CHART_LIVE_WINDOW_STEPS = 2
const CHART_LIVE_OUTER_DOT_SIZE = 16
const CHART_LIVE_MIDDLE_DOT_SIZE = 10
const CHART_LIVE_INNER_DOT_SIZE = 6
Expand Down Expand Up @@ -306,10 +304,10 @@ function SandboxMetricsChart({
[isMobile]
)

const liveWindowMs = useMemo(() => {
const liveMetricThresholdMs = useMemo(() => {
const duration =
xAxisMax !== undefined && xAxisMin !== undefined ? xAxisMax - xAxisMin : 0
return CHART_LIVE_WINDOW_STEPS * calculateStepForDuration(duration)
return calculateStepForDuration(duration)
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

liveMetricThresholdMs is now just calculateStepForDuration(duration) (1× step). For short ranges this can be 5s, which makes findLivePoint return null unless the latest point is extremely recent; previously the logic allowed a wider window (multiple steps and a minimum threshold). This change can cause the live dot/indicator to disappear intermittently under normal conditions. Consider using a wider window (e.g., 2× step and/or a minimum) when deciding live points.

Suggested change
return calculateStepForDuration(duration)
const stepMs = calculateStepForDuration(duration)
// Allow a slightly wider live window than a single step so the live
// indicator remains stable despite normal polling, ingestion, or render lag.
return Math.max(stepMs * 2, 15_000)

Copilot uses AI. Check for mistakes.
}, [xAxisMax, xAxisMin])

const option = useMemo<EChartsOption>(() => {
Expand Down Expand Up @@ -370,12 +368,8 @@ function SandboxMetricsChart({
const livePoint =
isPolling &&
latestPointTimestampMs !== null &&
Date.now() - latestPointTimestampMs <=
Math.max(liveWindowMs, SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS)
? findLivePoint(
line.data,
Math.max(liveWindowMs, SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS)
)
Date.now() - latestPointTimestampMs <= liveMetricThresholdMs
? findLivePoint(line.data, liveMetricThresholdMs)
: null

const regularSeriesItems = renderableSegments.map(
Expand Down Expand Up @@ -546,7 +540,7 @@ function SandboxMetricsChart({
grid,
isMobile,
isPolling,
liveWindowMs,
liveMetricThresholdMs,
series,
showXAxisLabels,
stroke,
Expand Down
10 changes: 7 additions & 3 deletions src/features/dashboard/sandbox/monitoring/state/use-sandbox-monitoring-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
import { useCallback, useEffect, useMemo, useRef } from 'react'
import { useDashboard } from '@/features/dashboard/context'
import { useSandboxContext } from '@/features/dashboard/sandbox/context'
import { calculateStepForDuration } from '@/features/dashboard/sandboxes/monitoring/utils'
import { getMsUntilNextAlignedInterval } from '@/lib/hooks/use-aligned-refetch-interval'
import type { SandboxMetric } from '@/server/api/models/sandboxes.models'
import { useTRPCClient } from '@/trpc/client'
import {
SANDBOX_LIFECYCLE_EVENT_KILLED,
SANDBOX_MONITORING_DEFAULT_PRESET_ID,
SANDBOX_MONITORING_DEFAULT_RANGE_MS,
SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS,
SANDBOX_MONITORING_LIVE_POLLING_MS,
SANDBOX_MONITORING_OVERFETCH_MIN_MS,
SANDBOX_MONITORING_OVERFETCH_RATIO,
Expand Down Expand Up @@ -256,12 +256,16 @@
return latest
}, [metricsQuery.data])

const liveMetricThresholdMs = useMemo(
() => calculateStepForDuration(fetchTimeframe.end - fetchTimeframe.start),
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 6, 2026

Choose a reason for hiding this comment

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

P2 Badge Compute live threshold from visible timeframe

useSandboxMonitoringController now derives liveMetricThresholdMs from fetchTimeframe, which includes the overfetch buffer rather than the user-visible range. That changes threshold buckets at duration boundaries (for example, a ~59m view can be treated like >1h after buffering), so isPolling may remain true while chart-side live-point detection (computed from the x-axis range) no longer considers data live. This causes inconsistent live state in the UI and makes the live badge/behavior drift from the actual displayed window.

Useful? React with 👍 / 👎.

[fetchTimeframe.end, fetchTimeframe.start]
)
Copy link
Copy Markdown

@cursor cursor bot Apr 6, 2026

Choose a reason for hiding this comment

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

Controller and chart use different durations for threshold

Medium Severity

The controller computes liveMetricThresholdMs using fetchTimeframe duration (which includes the overfetch buffer), while the chart computes the same threshold using xAxisMax - xAxisMin (the display timeframe, without buffer). Since calculateStepForDuration is a step function with discrete boundaries (1h, 6h, 12h, 24h, 7d), these two different durations can fall into different buckets near boundaries, producing different thresholds. This can cause the controller to set isPolling = true while the chart's stricter threshold suppresses the live dot — resulting in data polling without any visual live indicator.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3c7d849. Configure here.


const isLive =
shouldPoll &&
latestMetricTimestampMs !== null &&
Date.now() - latestMetricTimestampMs <=
SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS
Date.now() - latestMetricTimestampMs <= liveMetricThresholdMs
Comment on lines +259 to +267
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

liveMetricThresholdMs is derived directly from calculateStepForDuration(...), which can be as low as 5s for short ranges. Since isPolling/isLive gates the UI's “live” state, this makes the live indicator likely to flicker/off due to normal polling jitter, backend alignment, or slight clock skew. Consider using a more tolerant threshold (e.g., multiple steps and/or a minimum like 60s), or reusing a dedicated helper for “freshness” instead of raw step size.

Copilot uses AI. Check for mistakes.

Check failure on line 268 in src/features/dashboard/sandbox/monitoring/state/use-sandbox-monitoring-controller.ts

View check run for this annotation

Claude / Claude Code Review

Inconsistent liveMetricThresholdMs between controller and chart 

The controller computes `liveMetricThresholdMs` from the overfetched fetch window (`fetchTimeframe.end - fetchTimeframe.start`), while the chart computes it from the visible display window (`xAxisMax - xAxisMin`); near `calculateStepForDuration` bucket boundaries these resolve to different steps. For a ~59-minute view the display window (3,540,000ms) falls in the <1hr bucket → 5s step, while the fetch window (3,681,600ms with buffers) falls in the <6hr bucket → 30s step — so the header shows liv
Comment on lines +260 to 268
Copy link
Copy Markdown

@claude claude bot Apr 6, 2026

Choose a reason for hiding this comment

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

🔴 The controller computes liveMetricThresholdMs from the overfetched fetch window (fetchTimeframe.end - fetchTimeframe.start), while the chart computes it from the visible display window (xAxisMax - xAxisMin); near calculateStepForDuration bucket boundaries these resolve to different steps. For a ~59-minute view the display window (3,540,000ms) falls in the <1hr bucket → 5s step, while the fetch window (3,681,600ms with buffers) falls in the <6hr bucket → 30s step — so the header shows live (controller threshold: 30s) but the chart live dot disappears for data 6–30s old (chart threshold: 5s). Fix by computing both thresholds from the same duration, e.g. pass the controller's liveMetricThresholdMs as a prop to the chart.

Extended reasoning...

What the bug is and how it manifests

This PR replaces the old shared SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS constant (a 2-minute floor) with per-component calculateStepForDuration-based calculations. However, the two components use different input durations: the controller uses fetchTimeframe.end - fetchTimeframe.start (the overfetched window) while the chart uses xAxisMax - xAxisMin (the visible display window). Because calculateStepForDuration has discrete step buckets at 1h, 6h, 12h, 24h, and 7d boundaries, a display duration sitting just below one of those thresholds can map to a very different step than the wider fetch window that crosses it.

The specific code path

In use-sandbox-monitoring-controller.ts lines 260–268, liveMetricThresholdMs is computed as calculateStepForDuration(fetchTimeframe.end - fetchTimeframe.start). In monitoring-sandbox-metrics-chart.tsx around line 307, liveMetricThresholdMs is computed as calculateStepForDuration(xAxisMax - xAxisMin). The chart receives xAxisMin={renderedSnapshot.timeframe.start} and xAxisMax={renderedSnapshot.timeframe.end} (the display timeframe), while fetchTimeframe adds max(duration * 0.02, 30000ms) of overfetch buffer on each side.

Why existing code doesn't prevent it

The old SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS = 2 * millisecondsInMinute (120,000ms) constant was applied as a Math.max floor on both sides — Math.max(liveWindowMs, SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS) — so both always agreed on at least 120s. This PR removes that floor and switches to pure calculateStepForDuration outputs, but doesn't ensure both components use the same input duration.

Impact

For any visible timeframe sitting just below a bucket boundary (~57.7–60 minutes, ~5h45m–6h, ~11h30m–12h, etc.), the controller declares the sandbox live (isLive=true, isPolling=true) while the chart's threshold is 6× smaller. With a 5s polling interval plus typical network/server latency, data will routinely arrive 6–30s old. In this range the header live indicator says "live" but the chart live dot flickers off — contradictory UI state visible to users in the common 1-hour monitoring window.

Step-by-step proof

  1. User opens a sandbox monitoring page with a ~59-minute preset window. Display timeframe duration = 3,540,000ms.
  2. Controller computes fetch buffer: max(3,540,000 × 0.02, 30,000) = max(70,800, 30,000) = 70,800ms per side → fetchTimeframe duration = 3,540,000 + 2 × 70,800 = 3,681,600ms.
  3. Controller: calculateStepForDuration(3,681,600) → falls in <6h bucket → 30s step.
  4. Chart: calculateStepForDuration(3,540,000) → falls in <1h bucket → 5s step.
  5. A polling response arrives with latest metric timestamp 10 seconds ago. Date.now() - latestMetricTimestampMs = 10,000ms.
  6. Controller: 10,000 <= 30,000 → isLive=true → isPolling=true passed to chart.
  7. Chart: 10,000 <= 5,000 is false → live dot NOT shown.
  8. Header shows live indicator; chart shows no live dot. Inconsistent state.

How to fix

The cleanest fix is to compute liveMetricThresholdMs once in the controller (using the display timeframe duration, not the fetch timeframe duration) and pass it as an explicit prop to the chart, so both components always agree. Alternatively, the chart can derive its threshold from the same overfetched window by accepting fetchTimeframe bounds, but passing a pre-computed value is simpler and avoids prop drilling of implementation details.

return {
lifecycleBounds,
lifecycleEvents: sandboxLifecycle?.events ?? [],
Expand Down
3 changes: 0 additions & 3 deletions src/features/dashboard/sandbox/monitoring/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
millisecondsInDay,
millisecondsInHour,
millisecondsInMinute,
millisecondsInSecond,
} from 'date-fns/constants'

Expand Down Expand Up @@ -55,8 +54,6 @@ export const SANDBOX_MONITORING_LIVE_POLLING_MS = 5_000
export const SANDBOX_MONITORING_CUSTOM_END_FUTURE_MS = millisecondsInHour
export const SANDBOX_MONITORING_OVERFETCH_RATIO = 0.02
export const SANDBOX_MONITORING_OVERFETCH_MIN_MS = 30 * millisecondsInSecond
export const SANDBOX_MONITORING_LIVE_DATA_THRESHOLD_MS =
2 * millisecondsInMinute

export const SANDBOX_MONITORING_CHART_STROKE_VAR = '--stroke'
export const SANDBOX_MONITORING_CHART_FALLBACK_STROKE = '#000'
Expand Down
Loading