-
Notifications
You must be signed in to change notification settings - Fork 69
feat: extend sandboxes monitoring charts to support up to 90 days #350
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
7c34e66
8de383e
543fbf8
8bcb217
9428621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,10 +67,16 @@ export const TIME_OPTIONS: TimeOption[] = [ | |
| shortcut: '30D', | ||
| rangeMs: 30 * 24 * 60 * 60 * 1000, | ||
| }, | ||
| { | ||
| label: `Last 90 days`, | ||
| value: '90d', | ||
| shortcut: '90D', | ||
| rangeMs: 90 * 24 * 60 * 60 * 1000, | ||
| }, | ||
| ] | ||
|
|
||
| // constraints | ||
| export const MAX_DAYS_AGO = 31 * 24 * 60 * 60 * 1000 // 31 days in ms | ||
| export const MAX_DAYS_AGO = 91 * 24 * 60 * 60 * 1000 // 91 days in ms | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this 91 days and not 90?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a 1-day buffer so the "Last 90 days" preset never hits the validation boundary. The preset calculates The original code used the same pattern: Happy to change it to exactly 90 days if you'd prefer — the existing |
||
| export const MIN_RANGE_MS = 1.5 * 60 * 1000 // 1.5 minutes minimum | ||
| export const CLOCK_SKEW_TOLERANCE = 60 * 1000 // 60 seconds | ||
| export const DEFAULT_RANGE_MS = 60 * 60 * 1000 // 1 hour default | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,17 +36,17 @@ | |
| return | ||
| } | ||
|
|
||
| const now = Date.now() | ||
| const startTimestamp = startDateTime.getTime() | ||
|
|
||
| // validate start date is not more than 31 days ago | ||
| // validate start date is not more than 91 days ago | ||
| if (startTimestamp < now - MAX_DAYS_AGO) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'Start date cannot be more than 31 days ago', | ||
| message: 'Start date cannot be more than 91 days ago', | ||
| path: ['startDate'], | ||
| }) | ||
| return | ||
|
Check warning on line 49 in src/features/dashboard/sandboxes/monitoring/time-picker/validation.ts
|
||
|
Comment on lines
44
to
54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The validation error messages now reference "91 days" ( Extended reasoning...What the bug isThe PR introduces a "Last 90 days" preset and updates the header card to read "(90-day max)" — the user-visible product surface promises 90 days. Internally, however, // src/features/dashboard/sandboxes/monitoring/time-picker/validation.ts:46
message: 'Start date cannot be more than 91 days ago',
// :111
message: 'Date range cannot exceed 91 days',The same
All of those now interpolate How it manifests — step-by-step proof
Why existing code doesn't prevent itThe PR deliberately left ImpactUX polish, not a functional bug — the picker still works and the data still loads. But a user who reads "(90-day max)" and then sees "91 days" in a validation toast is left wondering which is correct, and the inconsistency surfaces an implementation detail (the clock-skew buffer) as if it were a product limit. It's more visually jarring at 90/91 than at the previous 30/31 because the larger numbers are read more carefully. How to fixTwo clean options:
Option 1 is the smaller change and keeps the existing buffer semantics. The pre-existing 30/31 split shows the pattern was already in the codebase, but the larger number makes it more noticeable now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8de383e — all user-facing validation error messages now hardcode "90 days" instead of interpolating |
||
| } | ||
|
|
||
| // validate start date is not in the future (with tolerance for clock skew) | ||
|
|
@@ -108,7 +108,7 @@ | |
| if (endTimestamp - startTimestamp > MAX_DAYS_AGO) { | ||
| ctx.addIssue({ | ||
| code: z.ZodIssueCode.custom, | ||
| message: 'Date range cannot exceed 31 days', | ||
| message: 'Date range cannot exceed 91 days', | ||
| path: ['endDate'], | ||
| }) | ||
| return | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Mock data generator at
src/configs/mock-data.ts:1087-1102still hardcodes a 30-day clamp (const thirtyDaysAgo = now - 30 * 24 * 60 * 60 * 1000; if (startMs < thirtyDaysAgo) startMs = thirtyDaysAgo). This PR's new 90-day query inMaxConcurrentSandboxes(header.tsx:162) and the "Last 90 days" preset now silently truncate to 30 days whenUSE_MOCK_DATAis on, so the "(90-day max)" card and the 90d chart on the Vercel preview deployments listed in this PR are computed over only the last 30 days. Bump the clamp to matchMAX_DAYS_AGO(≥91 days) and update the stale "past 30 days from now" docstring above the function.Extended reasoning...
What the bug is.
generateMockTeamMetricsinsrc/configs/mock-data.ts:1087-1102clamps any requested start time forward to no more than 30 days ago:It is exported as both
MOCK_TEAM_METRICS_DATA(line 1277) and viaMOCK_TEAM_METRICS_MAX_DATA(lines 1282-1311, which callsgenerateMockTeamMetricsthen computes the max). Bothget-team-metrics.tsandget-team-metrics-max.tsdispatch to these mock paths whenUSE_MOCK_DATAis set (src/configs/flags.ts:8-10:VERCEL_ENV !== 'production' && NEXT_PUBLIC_MOCK_DATA === '1').Code path this PR triggers.
header.tsx:162now computesstart = end - (90 * 24 * 60 * 60 * 1000 - 60_000)and callsgetTeamMetricsMax({ ..., metric: 'concurrent_sandboxes' }). In mock mode that routes toMOCK_TEAM_METRICS_MAX_DATA(start, end, 'concurrent_sandboxes')→generateMockTeamMetrics(start, end), which silently rewritesstartMstonow - 30d. The "max" returned is therefore the max over the last 30 days, butheader.tsx:88labels the card(90-day max). The same issue affects the new90dpreset inTIME_OPTIONS(constants.ts) /TIME_RANGES(timeframe.ts): the 90-day chart paints only the most-recent 30 days.Why existing code doesn't prevent it. The clamp is silent — no warning is returned and no callers inspect the actual range covered. Pre-PR this was a 1-day gap (
MAX_DAYS_AGOwas 31d vs the clamp at 30d), so it was effectively invisible. This PR widensMAX_DAYS_AGOto 91 days and adds a 90-day preset, opening a 60-day gap between what the UI promises and what the mock returns. The function docstring atmock-data.ts:1082-1086("Can generate data for the past 30 days from now") is now stale and will mislead the next reader.Impact. Production is unaffected —
USE_MOCK_DATAis gated to non-prod byflags.ts:8-10. But the PR explicitly links Vercel preview deployments (web-git-devin-1780334031-90d-timepicker.e2b-preview.dev) for QA, and those are exactly where mock data is on. A reviewer toggling mock mode to verify this PR's headline feature will see a "90-day max" card and a "Last 90 days" chart that quietly only reflect 30 days of data, defeating the purpose of preview verification.Step-by-step proof. Assume
now = Date.now() = TandUSE_MOCK_DATA = true.MaxConcurrentSandboxescomputesend = T,start = T - (90d - 60s)≈T - 90d.getTeamMetricsMax({ startDate: T - 90d, endDate: T, metric: 'concurrent_sandboxes' }).MOCK_TEAM_METRICS_MAX_DATA(T - 90d, T, 'concurrent_sandboxes')→generateMockTeamMetrics(T - 90d, T).thirtyDaysAgo = T - 30d. SincestartMs (T - 90d) < thirtyDaysAgo (T - 30d),startMsis reassigned toT - 30d.T - 30dandT. The max returned is the max over those 30 days, not 90.MOCK_TEAM_METRICS_DATAwhen a user picks "Last 90 days" in the time picker.How to fix. Change the clamp to
MAX_DAYS_AGO(or at least91 * 24 * 60 * 60 * 1000) and update the docstring atmock-data.ts:1082-1086to reflect the new horizon. One-line change, bundles cleanly with this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 8de383e — the mock data generator's clamp was updated from 30 days to 90 days, and the "past 30 days from now" docstring was updated to "past 90 days from now".