Conversation
…d date formats - Fix timestamp parsing for microsecond precision timestamps - Improve timezone handling in chart tooltips - Optimize tooltip performance with memoization and precomputed timestamp maps - Standardize time formatting across all chart components - Add proper type safety for chart mouse events - Extract shared utilities for timestamp parsing and tooltip formatting - Resolve issues with tooltip display when viewing different date ranges
…-tooltip-times-are-wrong-when-looking-at-different-date-times
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughPropagates TimeseriesGranularity through chart components and selection handlers; normalizes timestamps via parseTimestamp and timestamp→index maps; refactors tooltip/interval formatting (timezone-aware, memoized); expands granularity registry/constants and ClickHouse timeseries exports; updates mouse/selection event types and tooltip accessibility. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Chart as LogsTimeseriesBarChart / Area
participant Parse as parseTimestamp / timestampMap
participant Tooltip as formatTooltipInterval
participant Gran as Granularity hook
User->>Chart: hover / move (ChartMouseEvent)
Chart->>Parse: normalize activeLabel, lookup timestampToIndexMap
alt index found
Parse-->>Chart: index
else
Chart->>Chart: linear search for index
end
Chart->>Tooltip: formatTooltipInterval(timestamp, data, granularity, map)
Tooltip-->>Chart: formatted label (single or range + TZ)
Chart-->>User: render tooltip (role="tooltip")
sequenceDiagram
autonumber
actor User
participant Chart as LogsTimeseriesBarChart
participant Gran as Granularity hook
participant Buffer as getTimeBufferForGranularity
participant Parent as onSelectionChange
User->>Chart: MouseDown (selection enabled)
User->>Chart: MouseMove (update end)
User->>Chart: MouseUp
Chart->>Gran: read chart granularity
Chart->>Buffer: getTimeBufferForGranularity(granularity)
Chart->>Parent: onSelectionChange({start, end, granularity})
Chart->>Chart: reset selection to undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MichaelUnkey
requested review from
Flo4604,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/dashboard/components/ui/chart.tsx (1)
235-239: 0 values are suppressed in tooltip.The conditional {item.value && ...} hides valid zeros. Render when value is not null/undefined instead.
- {item.value && ( + {(item.value !== undefined && item.value !== null) && ( <span className="font-mono tabular-nums text-accent-12"> - {formatNumber(item.value as number)} + {formatNumber(item.value as number)} </span> )}apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
36-67: Implement safe parsing in formatTimestampForChart to handle numeric strings and microseconds
Replace the unguardednew Date(value)with the same logic used elsewhere in this file:const parsed = typeof value === "string" && /^\d+$/.test(value) ? Number(value) : value; const localDate = parsed instanceof Date ? parsed : typeof parsed === "number" && isUnixMicro(parsed) ? unixMicroToDate(parsed) : new Date(parsed as any);This ensures numeric strings aren’t mis-parsed and microsecond timestamps are correctly converted.
apps/dashboard/components/logs/overview-charts/utils.tsx (1)
91-96: Don’t treat timestamp 0 as missing.
if (!currentTimestamp)will incorrectly hide valid epoch timestamps. Use a nullish check.- if (!currentTimestamp) { + if (currentTimestamp == null) { return ""; }apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (2)
84-154: Update Regular-context expectations to match new thresholds.The implementation now maps longer ranges to coarser buckets: 8–16h → per30Minutes, 16h–<7d → perHour, ≥7d → perDay. Several assertions still expect the old per15Minutes/per2Hours values.
Apply these diffs:
- { - name: "should use per15Minutes for timeRange >= 8 hours & < 12 hours", - startTime: getTime(HOUR_IN_MS * 10), - expectedGranularity: "per15Minutes", - }, + { + name: "should use per30Minutes for timeRange >= 8 hours & < 12 hours", + startTime: getTime(HOUR_IN_MS * 10), + expectedGranularity: "per30Minutes", + }, - { - name: "should use per15Minutes for timeRange >= 12 hours & < 16 hours", - startTime: getTime(HOUR_IN_MS * 14), - expectedGranularity: "per15Minutes", - }, + { + name: "should use per30Minutes for timeRange >= 12 hours & < 16 hours", + startTime: getTime(HOUR_IN_MS * 14), + expectedGranularity: "per30Minutes", + }, - { - name: "should use per15Minutes for timeRange >= 16 hours & < 24 hours", - startTime: getTime(HOUR_IN_MS * 20), - expectedGranularity: "per15Minutes", - }, + { + name: "should use perHour for timeRange >= 16 hours & < 24 hours", + startTime: getTime(HOUR_IN_MS * 20), + expectedGranularity: "perHour", + }, - { - name: "should use per15Minutes for timeRange >= 24 hours & < 3 days", - startTime: getTime(DAY_IN_MS * 2), - expectedGranularity: "per15Minutes", - }, + { + name: "should use perHour for timeRange >= 24 hours & < 3 days", + startTime: getTime(DAY_IN_MS * 2), + expectedGranularity: "perHour", + }, - { - name: "should use per30Minutes for timeRange >= 3 days & < 7 days", - startTime: getTime(DAY_IN_MS * 5), - expectedGranularity: "per30Minutes", - }, + { + name: "should use perHour for timeRange >= 3 days & < 7 days", + startTime: getTime(DAY_IN_MS * 5), + expectedGranularity: "perHour", + }, - { - name: "should use per2Hours for timeRange >= 7 days", - startTime: getTime(DAY_IN_MS * 10), - expectedGranularity: "per2Hours", - }, + { + name: "should use perDay for timeRange >= 7 days", + startTime: getTime(DAY_IN_MS * 10), + expectedGranularity: "perDay", + }, @@ - it("should handle edge case at exactly 7 days boundary", () => { + it("should handle edge case at exactly 7 days boundary", () => { const result = getTimeseriesGranularity("forRegular", FIXED_NOW - DAY_IN_MS * 7, FIXED_NOW); - expect(result.granularity).toBe("per2Hours"); + expect(result.granularity).toBe("perDay"); });
265-281: Update Regular use-case expectations to match code.24h view now maps to perHour; 1-week view maps to perDay.
- expect(result.granularity).toBe("per15Minutes"); + expect(result.granularity).toBe("perHour"); @@ - expect(result.granularity).toBe("per2Hours"); + expect(result.granularity).toBe("perDay");apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
107-124: Type mismatch in forRegular mapping: the else-branch forforRegularassigns"perDay","per3Days","perWeek", and"perMonth", none of which are in theRegularTimeseriesGranularityunion (which caps at"per6Hours"), and tests still expect"per15Minutes"/"per2Hours"for longer ranges. This unsafe cast will break existing tests.Two fixes:
- Option A (preferred): extend
RegularTimeseriesGranularityand update tests thresholds.- Option B: revert mapping to only return the original Regular buckets.
Option A minimal diff:
export type RegularTimeseriesGranularity = | "perMinute" | "per5Minutes" | "per15Minutes" | "per30Minutes" | "perHour" | "per2Hours" | "per4Hours" - | "per6Hours"; + | "per6Hours" + | "perDay" + | "per3Days" + | "perWeek" + | "perMonth";
🧹 Nitpick comments (18)
apps/dashboard/components/ui/chart.tsx (1)
165-171: Tooltip a11y: wire role="tooltip" to a trigger or consider a live region.Adding role="tooltip" is good, but it won’t be announced unless a trigger element references it via aria-describedby and a stable id. Either:
- Provide a stable id here and ensure the chart’s focusable trigger sets aria-describedby to it; or
- If you intend announcements on change without focus, use aria-live="polite" (and optionally aria-atomic="true") instead of role="tooltip".
Example (local-only change):
- <div - ref={ref} - role="tooltip" + const tooltipId = React.useId(); + <div + ref={ref} + role="tooltip" + id={tooltipId} + aria-live="polite" + aria-atomic="true"apps/dashboard/components/logs/chart/utils/format-timestamp.ts (2)
4-29: Memoization cache: OK, but confirm client-only usage to avoid SSR-global growth.A module-level Map persists for the app lifetime. With MAX_CACHE_SIZE=1000 it’s small, but on SSR this can accumulate across requests. Verify this module is only imported in client components; otherwise consider scoping the cache per request or using a tiny LRU.
No change required if client-only.
74-78: Microsecond heuristic is brittle.length === 16 works today but breaks as the epoch grows or for left-padded values. Prefer a numeric range check, e.g., 1e15 <= n < 1e17.
-const isUnixMicro = (unix: string | number): boolean => { - const digitLength = String(unix).length === 16; - const isNum = !Number.isNaN(Number(unix)); - return isNum && digitLength; -}; +const isUnixMicro = (unix: string | number): boolean => { + const n = Number(unix); + return Number.isFinite(n) && n >= 1e15 && n < 1e17; +};apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
27-58: Duplicate selection-range logic across charts—extract a helper.The selection filtering and single-point expansion appears in multiple files. Consider a small utility (e.g., computeAdjustedRange + applyTimeFilters) to DRY this.
apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
24-48: Selection-change logic duplication.Same duplication as other charts; consider extracting a shared helper.
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
25-55: DRY up selection filter update.Same selection-change logic—consider centralizing to avoid drift.
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/charts/index.tsx (2)
35-40: Broaden granularity param type (avoidtypeofnarrowing).
granularity?: typeof verificationGranularityties the param to only the verification union. Since you passactiveKeysGranularitytoo, prefer the shared union type to avoid assignability issues.Apply:
-import { getTimeBufferForGranularity } from "@/lib/trpc/routers/utils/granularity"; +import { getTimeBufferForGranularity } from "@/lib/trpc/routers/utils/granularity"; +import type { CompoundTimeseriesGranularity } from "@/lib/trpc/routers/utils/granularity"; @@ - granularity?: typeof verificationGranularity; + granularity?: CompoundTimeseriesGranularity;
32-64: Optional: stabilize callback and default zero-width range behavior.
- Wrap
handleSelectionChangeinuseCallbackwithfilters/updateFiltersdeps to reduce renders on large pages.- If a caller forgets to pass
granularity, consider a default (e.g., 60_000 ms) to avoid zero-width ranges.Happy to wire this with a tiny diff if you want it in this PR.
apps/dashboard/components/logs/parseTimestamp.ts (2)
38-41: Return NaN for invalid inputs to avoid “1970” artifacts.Returning
0for invalid/empty inputs can render as the Unix epoch in downstream tooltips. PreferNumber.NaNso callers can explicitly guard.- if (timestamp == null) { - return 0; - } + if (timestamp == null) { + return Number.NaN; + } @@ - if (!Number.isFinite(timestampNum)) { - return 0; - } + if (!Number.isFinite(timestampNum)) { + return Number.NaN; + }Follow-up: update consumers (e.g.,
createTimeIntervalFormatter) to treatNumber.isNaN(parseTimestamp(...))as “no timestamp”.Also applies to: 84-88
95-101: Use truncation for symmetric rounding of sub-ms conversions.Current approach floors the absolute then reapplies sign, effectively truncating toward zero—good. Make this intent explicit and simpler with
Math.trunc.- if (absoluteTimestamp >= 1e18) { - timestampMs = Math.floor(absoluteTimestamp / 1e6); - } else if (absoluteTimestamp >= 1e15) { - timestampMs = Math.floor(absoluteTimestamp / 1e3); + if (absoluteTimestamp >= 1e18) { + timestampMs = Math.trunc(absoluteTimestamp / 1e6); + } else if (absoluteTimestamp >= 1e15) { + timestampMs = Math.trunc(absoluteTimestamp / 1e3);Also applies to: 109-115
apps/dashboard/components/logs/overview-charts/utils.tsx (3)
145-150: Consistency: include timezone in this fallback too.Other branches show
{formattedCurrentTimestamp} ({timezoneAbbr}). Keep this path consistent.- return ( - <div> - <span className="font-mono text-accent-9 text-xs px-4">{formattedCurrentTimestamp}</span> - </div> - ); + return ( + <div className="px-4"> + <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> + {formattedCurrentTimestamp} ({timezoneAbbr}) + </span> + </div> + );
116-130: Index lookup can be O(1) with a precomputed map.For large series,
findIndexper tooltip hover is O(n). If you already build atimestampToIndexMapin chart components, accept it here (optional param) or inject via closure.I can sketch a small overload that accepts an optional
Map<number, number>and uses it when provided.
23-55: UX consistency and i18n nits.
- Mixed patterns:
"h:mma"vs default"h:mm a". Pick one for consistent spacing.- Consider locale-aware formatting (user locale or a project-wide setting) instead of hardcoding
"en-US"if that’s a goal later.No blocking changes; just flagging for polish.
Also applies to: 76-80, 97-100, 154-157, 163-167, 169-175
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
46-46: Consider moving type export to a shared types file.The
Granularitytype alias is exported here but may be useful in other chart components. If multiple components need this alias, consider moving it totypes.tsfor better reusability.apps/dashboard/lib/trpc/routers/utils/granularity.ts (3)
65-74: Use nullish checks for defaults to avoid misclassification of epoch 0.Replace falsy checks with explicit null/undefined checks to prevent 0 being treated as “missing”.
- // If both are missing, fallback to an appropriate default for the context - if (!startTime && !endTime) { + // If both are missing, fallback to an appropriate default for the context + if (startTime == null && endTime == null) {
84-106: Verifications mapping LGTM; simplify redundant perHour branches.Behavior change to perDay for 7–30 days matches intended UX, but the two consecutive perHour checks for ≥3 days and ≥1 day are redundant.
- } else if (timeRange >= DAY_IN_MS * 3) { - granularity = "perHour"; - } else if (timeRange >= DAY_IN_MS) { + } else if (timeRange >= DAY_IN_MS) { granularity = "perHour";
59-63: Consider centralizing QUARTER/MONTH/WEEK in constants to avoid drift.These are derived locally; exporting them from ./constants keeps thresholds single‑sourced.
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
220-231: If adopting the new Regular mapping, extend valid granularity list.Keep type-compat test aligned with Option A (Regular can be day/week/month).
- const validGranularities = [ + const validGranularities = [ "perMinute", "per5Minutes", "per15Minutes", "per30Minutes", "perHour", "per2Hours", "per4Hours", - "per6Hours", + "per6Hours", + "perDay", + "per3Days", + "perWeek", + "perMonth", ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/charts/index.tsx(4 hunks)apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/_overview/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/logs/components/charts/index.tsx(1 hunks)apps/dashboard/components/logs/chart/index.tsx(10 hunks)apps/dashboard/components/logs/chart/utils/format-timestamp.ts(3 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(8 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(6 hunks)apps/dashboard/components/logs/overview-charts/utils.tsx(4 hunks)apps/dashboard/components/logs/parseTimestamp.ts(1 hunks)apps/dashboard/components/logs/utils.tsx(1 hunks)apps/dashboard/components/ui/chart.tsx(1 hunks)apps/dashboard/lib/trpc/routers/utils/granularity.test.ts(2 hunks)apps/dashboard/lib/trpc/routers/utils/granularity.ts(2 hunks)internal/ui/src/components/logs/control-cloud/control-pill.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/dashboard/components/logs/utils.tsx (4)
apps/dashboard/components/logs/overview-charts/utils.tsx (1)
DEFAULT_TIME_BUFFER_MS(7-7)apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
CompoundTimeseriesGranularity(31-33)getTimeBufferForGranularity(140-176)apps/dashboard/components/logs/overview-charts/types.ts (1)
TimeseriesData(16-19)apps/dashboard/components/logs/parseTimestamp.ts (1)
parseTimestamp(36-115)
apps/dashboard/components/logs/chart/index.tsx (3)
apps/dashboard/components/logs/overview-charts/types.ts (2)
TimeseriesData(16-19)Selection(9-14)apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
CompoundTimeseriesGranularity(31-33)apps/dashboard/components/logs/overview-charts/utils.tsx (1)
createTimeIntervalFormatter(76-177)
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (6)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
CompoundTimeseriesGranularity(31-33)apps/dashboard/components/logs/overview-charts/types.ts (1)
Selection(9-14)apps/dashboard/components/logs/parseTimestamp.ts (1)
parseTimestamp(36-115)apps/dashboard/components/logs/chart/index.tsx (1)
ChartMouseEvent(34-39)apps/dashboard/components/logs/utils.tsx (1)
formatTooltipInterval(100-204)apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
formatTimestampLabel(31-34)
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (5)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
CompoundTimeseriesGranularity(31-33)apps/dashboard/components/logs/overview-charts/types.ts (1)
Selection(9-14)apps/dashboard/components/logs/parseTimestamp.ts (1)
parseTimestamp(36-115)apps/dashboard/components/logs/chart/index.tsx (1)
ChartMouseEvent(34-39)apps/dashboard/components/logs/utils.tsx (1)
formatTooltipInterval(100-204)
apps/dashboard/components/logs/overview-charts/utils.tsx (3)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
CompoundTimeseriesGranularity(31-33)apps/dashboard/components/logs/overview-charts/types.ts (1)
TimeseriesData(16-19)apps/dashboard/components/logs/parseTimestamp.ts (1)
parseTimestamp(36-115)
🔇 Additional comments (31)
internal/ui/src/components/logs/control-cloud/control-pill.tsx (1)
69-69: Nice alignment with numeric timestamp flow.Passing the raw filter value keeps
TimestampInfoconsistent with the rest of the numeric timestamp rollout and avoids double-formatting. Looks good.apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (2)
99-106: Granularity propagation: LGTM.Forwarding granularity keeps tooltips/labels consistent with interval selection. No issues spotted.
38-41: Verify time units for adjusted end.Ensure getTimeBufferForGranularity returns milliseconds to match start/end units. If API expects seconds, convert accordingly.
apps/dashboard/app/(app)/[workspaceSlug]/ratelimits/[namespaceId]/logs/components/charts/index.tsx (2)
70-71: Granularity forwarding: LGTM.Keeps LogsTimeseriesBarChart aligned with the data interval. Good.
28-31: Confirm buffer units match start/end.Same note: verify getTimeBufferForGranularity granularity units match start/end (ms vs s).
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (2)
66-68: Granularity forwarding: LGTM.Consistent with other charts; no concerns.
36-39: Unit check for buffer.Confirm getTimeBufferForGranularity aligns with start/end units before adding.
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/charts/index.tsx (1)
45-49: Validate time units for filters and buffer.Confirm
getTimeBufferForGranularityreturns milliseconds and thatstartTime/endTimefilters expect ms. A units mismatch would skew ranges.If the filter API expects seconds or µs, convert here before
updateFilters. I can provide a small adapter once confirmed.Also applies to: 52-63
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (6)
3-18: LGTM! Clean import organization.The imports are well-organized and bring in the necessary types and utilities for timestamp handling and tooltip formatting.
47-47: LGTM! Proper granularity propagation.The
granularityprop is correctly typed and threaded through the component for tooltip formatting.Also applies to: 60-60
65-79: Excellent O(1) lookup optimization.Precomputing the timestamp-to-index map is a smart performance optimization that avoids linear searches during tooltip rendering. The implementation correctly:
- Normalizes timestamps using
parseTimestamp- Validates finite numeric results before storing
- Uses
useMemowith proper dependencies
102-115: Guard logic is correct but could be clearer.The mouse move handler correctly checks for
selection.startbefore updating, preventing updates when no selection has been initiated. However, the guard at line 103 checkse.activeLabel === undefined, while line 106 checksselection.start(which is a string that could be empty).The logic is correct but mixing undefined/truthy checks could be confusing. Consider whether
selection.start === ""is the intended initial state (it is, per line 63), and document this behavior.
275-283: LGTM! Tooltip formatter correctly uses granularity.The
labelFormatterproperly:
- Extracts the payload timestamp
- Passes all required parameters to
formatTooltipInterval- Uses the precomputed
timestampToIndexMapfor efficient lookups
89-100: No changes needed: originalTimestamp is always defined and numeric
TheTimeseriesDatatype declaresoriginalTimestamp: numberand every data construction (hooks, loaders, utils) assigns a numeric value, sotimestampwill never beundefined;Selection’s optional fields simply allow for payload-absent fallbacks but aren’t needed here.apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (5)
56-56: LGTM! Consistent granularity threading.The
granularityprop is properly added to both the props interface and the component parameters, maintaining consistency with the bar chart implementation.Also applies to: 67-67
71-83: LGTM! Identical optimization pattern.The
timestampToIndexMapprecomputation follows the same pattern as in the bar chart, ensuring consistent performance across chart types.
104-115: Stronger guard condition than bar chart.This implementation checks
!selection.startat line 105, which is stricter than the bar chart's approach. This ensures the selection has been initiated before allowing mouse moves to update it. This is the correct behavior.
266-274: LGTM! Consistent tooltip formatting.The tooltip formatter implementation matches the bar chart pattern, correctly passing granularity and the timestamp map.
307-320: No changes needed forcalculateTimePointsinvocation. TheparseTimestamphelper returns a numeric millisecond value, which matches the(startTime: number, endTime: number)signature ofcalculateTimePoints.apps/dashboard/components/logs/utils.tsx (10)
1-8: LGTM! Clean imports and constants.The imports are well-organized, and
DEFAULT_TIME_BUFFER_MSprovides a sensible 1-minute fallback for granularity calculations.
10-13: Excellent singleton pattern for performance.Using a singleton
Intl.DateTimeFormatavoids repeated construction during tooltip rendering, which is a good performance optimization.
16-25: Robust error handling in granularity buffer.The try-catch block gracefully handles unexpected granularity values, falling back to a safe default. This prevents tooltip rendering failures from propagating.
30-51: Comprehensive null and validation handling.The function correctly handles:
- Null/undefined timestamps
- Invalid parsed results (checking both 0 and non-finite values)
- Edge case where
timestamp === 0is legitimate
56-68: Smart multi-day span detection.The logic checks if data spans multiple calendar days by comparing date strings. This correctly handles scenarios where the data crosses midnight boundaries.
70-89: Granularity-based formatting with good defaults.The function smartly chooses time-only format for fine-grained data on the same day, and date+time for coarser granularities or multi-day spans. The threshold at 12 hours is reasonable.
100-130: LGTM! Defensive early returns.The function handles missing data and single-point data gracefully by falling back to single timestamp display.
132-152: Efficient lookup with linear search fallback.The implementation correctly:
- Prefers O(1) map lookup when available
- Falls back to linear search with
findIndex- Uses
parseTimestampfor robust comparison- Handles "not found" case gracefully
183-204: LGTM! Complete interval formatting with timezone.The final formatting:
- Uses consistent
formatTooltipTimestampcalls- Extracts timezone from the actual point date (correct for DST)
- Returns well-structured JSX with proper styling
154-181: Verify last interval end in utils.tsx (apps/dashboard/components/logs/utils.tsx:154–181)
The code infers the final bucket’s end by addinginferredGranularityMsto the last timestamp—this presumes a full-width interval. If your backend timeseries query can produce partial or end-of-range buckets, confirm this matches actual data or adjust the tooltip to indicate an approximate/open-ended interval.apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (2)
164-172: Verifications 7–30 days → perDay looks good.
192-199: Edge at 7 days → perDay aligns with the new mapping.
|
Thank you for following the naming conventions for pull request titles! 🙏 |
eng-2053-tooltip-times-are-wrong-when-looking-at-different-date-times
…g-at-different-date-times
|
Regression in /logs chart Density of old /logs chart: I think this is not a regression, but I recently increased the granularity for metrics logs so it looks better by default. We might wanna stick to that. Ratelimits logs chart: This is using the same structure as /log so we probably regressed something in the shared component. |
ogzhanolguncu
left a comment
ogzhanolguncu
left a comment
There was a problem hiding this comment.
It works great. There are some regressed stuff and I believe things we can improve a bit. Great work 💪🏻
…-tooltip-times-are-wrong-when-looking-at-different-date-times
…-tooltip-times-are-wrong-when-looking-at-different-date-times
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
40-55: Consider consolidating time buffer logic.The
TIMESERIES_GRANULARITIESregistry duplicates the millisecond values fromgetTimeBufferForGranularity(lines 123-160). This creates a maintenance burden where both must be kept in sync.Consider refactoring
getTimeBufferForGranularityto use the registry:export const getTimeBufferForGranularity = (granularity: TimeseriesGranularity): number => { - // Constants for commonly used durations - - // Return appropriate buffer based on granularity - switch (granularity) { - case "perMinute": - return MINUTE_IN_MS; - case "per5Minutes": - return 5 * MINUTE_IN_MS; - // ... (all other cases) - default: - // Default to 5 minutes if granularity is unknown - return 5 * MINUTE_IN_MS; - } + return TIMESERIES_GRANULARITIES[granularity]?.ms ?? 5 * MINUTE_IN_MS; };
76-76: Remove unnecessary type casts.The
as TimeseriesGranularitycasts at lines 76 and 110 suggest TypeScript cannot infer the type correctly. Sincegranularityis already typed asTimeseriesGranularity(line 89) and assigned from the union literal values, these casts should be unnecessary.Remove the type assertions:
return { - granularity: defaultGranularity as TimeseriesGranularity, + granularity: defaultGranularity, startTime: now - defaultDuration, endTime: now, context, };return { - granularity: granularity as TimeseriesGranularity, + granularity, startTime: effectiveStartTime, endTime: effectiveEndTime, context, };Also applies to: 110-110
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
180-183: Document 3-day threshold rationale The implementation uses>= 3 days + 1 hourfor switching toperDay, so at exactly 3 days it correctly returnsper2Hours. Add a clarifying comment beside the+ HOUR_IN_MSoffset to explain this choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts(6 hunks)apps/dashboard/lib/trpc/routers/utils/granularity.ts(5 hunks)internal/clickhouse/src/logs.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/clickhouse/src/logs.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
apps/dashboard/lib/trpc/routers/utils/constants.ts (6)
MINUTE_IN_MS(1-1)HOUR_IN_MS(2-2)DAY_IN_MS(3-3)WEEK_IN_MS(4-4)MONTH_IN_MS(5-5)QUARTER_IN_MS(6-6)apps/dashboard/components/logs/overview-charts/utils.tsx (1)
getTimeBufferForGranularity(38-40)
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (2)
apps/dashboard/lib/trpc/routers/utils/constants.ts (5)
HOUR_IN_MS(2-2)DAY_IN_MS(3-3)WEEK_IN_MS(4-4)MONTH_IN_MS(5-5)QUARTER_IN_MS(6-6)apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
getTimeseriesGranularity(64-115)TimeseriesGranularity(10-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (2)
147-149: LGTM! Unified granularity logic across contexts.The test correctly verifies that both
forRegularandforVerificationscontexts now use the same granularity selection logic, which aligns with the team's decision in the past review comments to unify granularity across all charts.Based on past review comments.
324-354: Good coverage of edge cases.The new edge case tests (zero time range, very small ranges, negative ranges) provide valuable protection against unexpected inputs and ensure the function degrades gracefully.
apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
91-107: Confirm +1 hour offset for perDay threshold
Implementation and tests treatperDayat ≥ 3 days + 1 hour, so exactly 3 days yieldsper2Hours. If this offset is intentional, add a clarifying comment; otherwise update the threshold:- } else if (timeRange >= DAY_IN_MS * 3 + HOUR_IN_MS) { + } else if (timeRange >= DAY_IN_MS * 3) { granularity = "perDay";
…-tooltip-times-are-wrong-when-looking-at-different-date-times
|
|
…g-at-different-date-times
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/09/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…g-at-different-date-times
) * fix(dashboard): correct tooltip timestamps for different timezones and date formats - Fix timestamp parsing for microsecond precision timestamps - Improve timezone handling in chart tooltips - Optimize tooltip performance with memoization and precomputed timestamp maps - Standardize time formatting across all chart components - Add proper type safety for chart mouse events - Extract shared utilities for timestamp parsing and tooltip formatting - Resolve issues with tooltip display when viewing different date ranges * format * fix tests for granularity * Rabbit suggestions in format-timestamp * PR changes requested * remove log * [autofix.ci] apply automated fixes * ganularity changes * [autofix.ci] apply automated fixes * fix type error after merge with main and FMT * tweakes to granularity --------- Co-authored-by: James Perkins <james@unkey.dev> Co-authored-by: Oğuzhan Olguncu <21091016+ogzhanolguncu@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> Co-authored-by: Flo <53355483+Flo4604@users.noreply.github.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
) * fix(dashboard): correct tooltip timestamps for different timezones and date formats - Fix timestamp parsing for microsecond precision timestamps - Improve timezone handling in chart tooltips - Optimize tooltip performance with memoization and precomputed timestamp maps - Standardize time formatting across all chart components - Add proper type safety for chart mouse events - Extract shared utilities for timestamp parsing and tooltip formatting - Resolve issues with tooltip display when viewing different date ranges * format * fix tests for granularity * Rabbit suggestions in format-timestamp * PR changes requested * remove log * [autofix.ci] apply automated fixes * ganularity changes * [autofix.ci] apply automated fixes * fix type error after merge with main and FMT * tweakes to granularity --------- Co-authored-by: James Perkins <james@unkey.dev> Co-authored-by: Oğuzhan Olguncu <21091016+ogzhanolguncu@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Andreas Thomas <dev@chronark.com> Co-authored-by: Flo <53355483+Flo4604@users.noreply.github.com>
perkinsjr
deleted the
eng-2053-tooltip-times-are-wrong-when-looking-at-different-date-times
branch
6 participants
What does this PR do?
Fixes # (issue)
Eng 2053
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated