fix: WIP Chart and tooltips are not informative#3915
fix: WIP Chart and tooltips are not informative#3915
Conversation
- Fix am/pm line wrapping by removing spaces (e.g., "2:30 pm" → "2:30pm") - Use international-friendly date formats (MMM d) instead of US-specific M/d - Increase tooltip width from 256px to 360px to accommodate longer timestamps - Move timezone information from bottom descriptions to inline tooltips - Streamline tooltip layout with single-line timezone display - Apply improvements universally across all chart components
Ratelimits fixed
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds granularity-aware timeseries charting across logs dashboards: new parsing/formatting utilities, tooltip interval rendering, typed chart mouse events, refined selection handling, and updated granularity mapping with tests. App pages now pass and synchronize granularity with charts. Minor UI tooltip accessibility and width tweaks included. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Chart as Chart (Area/Bar)
participant Tooltip as Tooltip Renderer
participant Utils as Formatting Utils
participant Parent as Parent Page
User->>Chart: Hover data point
Chart->>Tooltip: labelFormatter(payloadTimestamp, data, granularity)
Tooltip->>Utils: formatTooltipInterval(payloadTimestamp, data, granularity, tsIndexMap)
Utils-->>Tooltip: "start – end (TZ)" label
Tooltip-->>User: Render tooltip
User->>Chart: MouseDown / MouseMove / MouseUp
Chart->>Chart: Track selection {start,end} via activeLabel
Chart->>Parent: onSelectionChange({startTs,endTs, granularity})
Parent->>Parent: Update filters/state
Parent->>Chart: Rerender with granularity & data
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)
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 |
…g-at-different-date-times
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
apps/dashboard/components/ui/chart.tsx (1)
234-238: Zero values are hidden in tooltip
{item.value && ...}suppresses 0. Show 0 explicitly.Apply:
- {item.value && ( - <span className="font-mono tabular-nums text-accent-12"> - {formatNumber(item.value as number)} - </span> - )} + {item.value !== undefined && item.value !== null && ( + <span className="font-mono tabular-nums text-accent-12"> + {formatNumber(Number(item.value))} + </span> + )}apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
13-18: Handle microsecond timestamps for chart ticks too
formatTimestampTooltiphandles µs;formatTimestampForChartdoes not. Normalize here as well to avoid off-by-1000 errors when ticks receive µs.Apply:
- const localDate = new Date(value); + const localDate = isUnixMicro(value) ? unixMicroToDate(value) : new Date(value);apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
92-99: Minor: duplicate conditions map to the same resultBoth “≥ 2 weeks” and “≥ 1 week” return
perDay. Collapse to a single check for clarity.Apply:
- } else if (timeRange >= WEEK_IN_MS * 2) { - granularity = "perDay"; - } else if (timeRange >= WEEK_IN_MS) { - granularity = "perDay"; + } else if (timeRange >= WEEK_IN_MS) { + granularity = "perDay";
109-132: Update RegularTimeseriesGranularity to include new values
TheforRegularbranch now returns"perDay","per3Days","perWeek", and"perMonth", which aren’t defined inRegularTimeseriesGranularity. In apps/dashboard/lib/trpc/routers/utils/granularity.ts, expand the type accordingly:export type RegularTimeseriesGranularity = | "perMinute" | "per5Minutes" | "per15Minutes" | "per30Minutes" | "perHour" | "per2Hours" | "per4Hours" - | "per6Hours"; + | "per6Hours" + | "perDay" + | "per3Days" + | "perWeek" + | "perMonth";apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
97-100: Fix mismatched test names vs expectations (Regular context).Test names don’t match the asserted granularities. Either update names to reflect the current mapping or adjust expectations. Suggest renaming only (keeps the mapping under test intact):
- { - name: "should use per15Minutes for timeRange >= 4 hours & < 6 hours", + { + name: "should use per5Minutes for timeRange >= 4 hours & < 6 hours", startTime: getTime(HOUR_IN_MS * 5), expectedGranularity: "per5Minutes", }, - { - name: "should use per30Minutes for timeRange >= 6 hours & < 8 hours", + { + name: "should use per5Minutes for timeRange >= 6 hours & < 8 hours", startTime: getTime(HOUR_IN_MS * 7), expectedGranularity: "per5Minutes", }, - { - name: "should use perHour for timeRange >= 12 hours & < 16 hours", + { + name: "should use per30Minutes for timeRange >= 12 hours & < 16 hours", startTime: getTime(HOUR_IN_MS * 14), expectedGranularity: "per30Minutes", },Also applies to: 102-105, 112-115
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
57-66: Tighten granularity typing & extract zero-range buffer helper
- In
overview-bar-chart.tsxandoverview-area-chart.tsx, replacegranularity?: stringwith the concreteCompoundTimeseriesGranularityunion for stronger type safety.- Extract the duplicated “if start === end then buffer by granularity” block into a shared
applyZeroRangeBuffer(start, end, granularity, getTimeBufferForGranularity)helper (e.g. inutils/selection.ts) and replace all instances in chart components underapps/dashboard/app/**/charts/index.tsx.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
44-46: Bug: selection buffer uses verification granularity for both charts.When selecting on the Keys chart, end-time buffering should use
activeKeysGranularity, notverificationGranularity. Create per-chart handlers.Apply:
- const handleSelectionChange = ({ - start, - end, - }: { - start: number; - end: number; - }) => { - const activeFilters = filters.filter( - (f) => !["startTime", "endTime", "since"].includes(f.field), - ); - - let adjustedEnd = end; - if (start === end && verificationGranularity) { - adjustedEnd = end + getTimeBufferForGranularity(verificationGranularity); - } - updateFilters([ - ...activeFilters, - { field: "startTime", value: start, id: crypto.randomUUID(), operator: "is" }, - { field: "endTime", value: adjustedEnd, id: crypto.randomUUID(), operator: "is" }, - ]); - }; + const makeSelectionHandler = + (chartGranularity?: string) => + ({ start, end }: { start: number; end: number }) => { + const activeFilters = filters.filter( + (f) => !["startTime", "endTime", "since"].includes(f.field), + ); + const adjustedEnd = + start === end && chartGranularity + ? end + getTimeBufferForGranularity(chartGranularity) + : end; + updateFilters([ + ...activeFilters, + { field: "startTime", value: start, id: crypto.randomUUID(), operator: "is" }, + { field: "endTime", value: adjustedEnd, id: crypto.randomUUID(), operator: "is" }, + ]); + }; + + const handleVerificationSelectionChange = makeSelectionHandler(verificationGranularity); + const handleKeysSelectionChange = makeSelectionHandler(activeKeysGranularity); @@ - onSelectionChange={handleSelectionChange} + onSelectionChange={handleVerificationSelectionChange} @@ - onSelectionChange={handleSelectionChange} + onSelectionChange={handleKeysSelectionChange}Also applies to: 96-105, 108-117
apps/dashboard/components/logs/overview-charts/utils.tsx (2)
120-128: Missing timezone in the “no next point” branch.All other branches include the timezone; this one omits it, causing inconsistent UX.
- 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} ({getTimezoneAbbreviation(new Date(currentTimestamp))}) + </span> + </div> + );
106-108: Index lookup can fail due to type mismatch (string vs number vs Date).Strict equality on mixed timestamp types will often return
-1, breaking interval tooltips.-const currentIndex = data.findIndex((item) => item?.originalTimestamp === currentTimestamp); +const toEpoch = (ts: number | string | Date | undefined) => + ts instanceof Date ? ts.getTime() : Number(ts); +const currentIndex = data.findIndex( + (item) => toEpoch(item?.originalTimestamp) === toEpoch(currentTimestamp), +);apps/dashboard/components/logs/chart/index.tsx (3)
78-89: Guard against undefined chart event data on mousedown.Avoids setting an invalid selection when the cursor isn’t over a bar.
-const handleMouseDown = (e: ChartMouseEvent) => { - if (!enableSelection) { - return; - } - const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; - setSelection({ - start: e.activeLabel, - end: e.activeLabel, - startTimestamp: timestamp, - endTimestamp: timestamp, - }); -}; +const handleMouseDown = (e: ChartMouseEvent) => { + if (!enableSelection) return; + const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; + if (timestamp === undefined || e.activeLabel === undefined) return; + setSelection({ + start: e.activeLabel, + end: e.activeLabel, + startTimestamp: timestamp, + endTimestamp: timestamp, + }); +};
91-103: Selection bug: endTimestamp is never updated during drag.This breaks the final interval and can invert or collapse the range. Also, truthy check on
selection.startfails for valid0.-const handleMouseMove = (e: ChartMouseEvent) => { - if (!enableSelection) { - return; - } - if (selection.start) { - const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; - setSelection((prev) => ({ - ...prev, - end: e.activeLabel, - startTimestamp: timestamp, - })); - } -}; +const handleMouseMove = (e: ChartMouseEvent) => { + if (!enableSelection) return; + if (selection.start !== undefined) { + const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; + if (timestamp === undefined || e.activeLabel === undefined) return; + setSelection((prev) => ({ + ...prev, + end: e.activeLabel, + endTimestamp: timestamp, + })); + } +};
105-123: Use nullish checks to preserve epoch (0) and explicit start/end checks.Prevents false negatives when timestamps could be
0and aligns with the newundefinedsentinel.- if (selection.start && selection.end && onSelectionChange) { - if (!selection.startTimestamp || !selection.endTimestamp) { - return; - } + if (selection.start !== undefined && selection.end !== undefined && onSelectionChange) { + const { startTimestamp, endTimestamp } = selection; + if (startTimestamp == null || endTimestamp == null) { + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/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(6 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(4 hunks)apps/dashboard/components/logs/overview-charts/utils.tsx(5 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)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T13:46:08.303Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:1-1
Timestamp: 2025-08-25T13:46:08.303Z
Learning: The NamespaceListDateTime component in apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx is intentionally designed to use the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than the namespace list hook, as clarified by ogzhanolguncu. This coupling is by design, not an architectural issue.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsxapps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx
📚 Learning: 2025-08-25T13:46:34.441Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx:4-4
Timestamp: 2025-08-25T13:46:34.441Z
Learning: The namespace list refresh component (apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx) intentionally uses the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than a namespace-specific hook. This cross-coupling between namespace list components and overview hooks is an architectural design decision.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsxapps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx
🔇 Additional comments (17)
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (3)
164-167: Verifications 7–<30 days → perDay looks correct.Expectation aligns with the revised mapping.
175-176: Why shift the 30–60 day probe from 45d → 40d?If this is to increase distance from the 30d edge, add a short comment for future readers. Also consider adding an “exactly 60 days” edge test for symmetry with the 30-day test.
197-204: Edge at exactly 7 days → perDay is consistent.Boundary assertion matches the new rules.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
67-67: Pass-through granularity to OverviewBarChart — good wiring.Confirms tooltips/formatting can be granularity-aware.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (2)
70-70: Granularity forwarded to LogsTimeseriesBarChart — good.Enables correct tooltip/time formatting.
28-31: Verify granularity prop type and zero-range buffering
apps/dashboard/app/.../logs/components/charts/index.tsx:28-31
- Define
granularityas a specific union type (e.g.'hour' | 'day' | …) instead ofstring.- Extract the zero-range buffering logic into a shared helper (using
getTimeBufferForGranularity) across all chart components.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
105-105: Granularity passed to both charts — good.With the per-chart handlers above, formatting and buffering will be consistent per series.
Also applies to: 116-117
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
246-248: Guard hides tooltips when total is 0 — confirm intended UX.If a bucket has zero counts, you might still want to show the time interval. Verify product intent.
apps/dashboard/components/logs/chart/index.tsx (5)
11-11: Type-only import is correct.Using a type-only import avoids bundling/runtime impact in this client component.
22-23: Good shift to explicit undefined for “no selection”.This clarifies intent and avoids empty-string edge cases downstream.
50-50: Granularity plumbed through the public API: LGTM.Clean addition; no runtime cost and maintains backward compatibility.
Also applies to: 62-62
65-68: Initialization of selection to undefined: LGTM.Consistent with the updated
Selectionshape and simplifies checks.
141-142: Stable keys for header ticks: LGTM.Switching to
time.getTime()prevents key collisions on rerenders.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (4)
22-22: Good rename to avoid shadowing and improve clarity.Explicit
verificationGranularityreads better and prevents confusion.
29-29: Consistent, descriptive naming for keys granularity.Matches the verification counterpart and improves readability.
104-105: Passing per-fetch granularity to the bar chart: LGTM.This enables granularity-aware tooltips/formatting for requests.
116-117: Passing per-fetch granularity to the area chart: LGTM.Aligns keys chart with the same granularity-aware behavior.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx
Outdated
Show resolved
Hide resolved
perkinsjr
changed the title
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
97-115: Fix mismatched expectations vs. test names.Several cases assert the wrong granularity relative to their titles; these will fail or mask regressions.
Apply:
{ name: "should use per15Minutes for timeRange >= 4 hours & < 6 hours", startTime: getTime(HOUR_IN_MS * 5), - expectedGranularity: "per5Minutes", + expectedGranularity: "per15Minutes", }, { name: "should use per30Minutes for timeRange >= 6 hours & < 8 hours", startTime: getTime(HOUR_IN_MS * 7), - expectedGranularity: "per5Minutes", + expectedGranularity: "per30Minutes", }, { name: "should use perHour for timeRange >= 12 hours & < 16 hours", startTime: getTime(HOUR_IN_MS * 14), - expectedGranularity: "per30Minutes", + expectedGranularity: "perHour", },apps/dashboard/components/logs/overview-charts/utils.tsx (1)
124-127: Inconsistent styling and missing timezone in “no nextPoint” path.Match other branches and include TZ.
Apply:
- 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} ({currentTz}) + </span> + </div> + );apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (3)
1-5: Add "use client" — this component uses hooks and event handlers.Without the directive, Next.js will treat it as a Server Component and reject passing functions to Client charts.
Apply:
+ "use client"; import { OverviewBarChart } from "@/components/logs/overview-charts/overview-bar-chart"; import { getTimeBufferForGranularity } from "@/lib/trpc/routers/utils/granularity"; import { useFilters } from "../../hooks/use-filters"; import { useFetchRatelimitOverviewTimeseries } from "./bar-chart/hooks/use-fetch-timeseries";
38-41: Buffer selection even when granularity is temporarily undefined.If the fetch hasn’t resolved granularity, a click-selection yields a zero-width range. Derive a safe fallback from data spacing.
- let adjustedEnd = end; - if (start === end && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + let adjustedEnd = end; + if (start === end) { + const buffer = + granularity + ? getTimeBufferForGranularity(granularity) + : (timeseries?.length ?? 0) > 1 + ? Math.abs( + Number(timeseries![1].originalTimestamp) - + Number(timeseries![0].originalTimestamp), + ) + : 60_000; // 1m fallback + adjustedEnd = end + buffer; + }Optionally extract a shared helper (see below) to avoid duplication across charts.
27-58: Minor: memoize the handler.Prevents re-renders in children relying on referential equality.
-import { useFilters } from "../../hooks/use-filters"; +import { useFilters } from "../../hooks/use-filters"; +import { useCallback } from "react"; ... - const handleSelectionChange = ({ + const handleSelectionChange = useCallback(({ start, end, }: { start: number; end: number; }) => { const activeFilters = filters.filter( (f) => !["startTime", "endTime", "since"].includes(f.field), ); @@ - ]); - }; + ]); + }, [filters, granularity, timeseries, updateFilters]);apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (2)
28-31: Same zero-width selection edge case; add data-derived fallback.- let adjustedEnd = end; - if (start === end && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + let adjustedEnd = end; + if (start === end) { + const buffer = + granularity + ? getTimeBufferForGranularity(granularity) + : (timeseries?.length ?? 0) > 1 + ? Math.abs( + Number(timeseries![1].originalTimestamp) - + Number(timeseries![0].originalTimestamp), + ) + : 60_000; + adjustedEnd = end + buffer; + }
17-48: Extract a shared computeAdjustedEnd helper to dedupe logic across pages.You can place this in a shared logs utils module and reuse in all charts:
export const computeAdjustedEnd = ( start: number, end: number, granularity?: import("@/lib/trpc/routers/utils/granularity").CompoundTimeseriesGranularity, data?: Array<{ originalTimestamp: number | string }>, ) => { if (start !== end) return end; const buffer = granularity ? getTimeBufferForGranularity(granularity) : data && data.length > 1 ? Math.abs(Number(data[1].originalTimestamp) - Number(data[0].originalTimestamp)) : 60_000; return end + buffer; };Then call it in your handler.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
32-64: Optional: create makeSelectionHandler(granularity) and pass it, avoiding inline closures.- const handleSelectionChange = ({ start, end, granularity }: { start: number; end: number; granularity?: CompoundTimeseriesGranularity }) => { /*...*/ } + const makeSelectionHandler = + (g?: import("@/lib/trpc/routers/utils/granularity").CompoundTimeseriesGranularity) => + ({ start, end }: { start: number; end: number }) => + handleSelectionChange({ start, end, granularity: g }); @@ - onSelectionChange={(selection) => - handleSelectionChange({ - ...selection, - granularity: verificationGranularity, - }) - } + onSelectionChange={makeSelectionHandler(verificationGranularity)} @@ - onSelectionChange={(selection) => - handleSelectionChange({ - ...selection, - granularity: activeKeysGranularity, - }) - } + onSelectionChange={makeSelectionHandler(activeKeysGranularity)}apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
248-266: Nit: cursor/tooltip constants could be hoisted to avoid re-creation.Extract static objects (cursor style, wrapperStyle) outside the component to reduce allocations per render.
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
436-440: Use stable keys for time ticks; drop the linter suppression.Using the timestamp as the key prevents unnecessary re-renders and removes the need for the ignore comment.
- ).map((time, i) => ( - // biome-ignore lint/suspicious/noArrayIndexKey: <explanation> - <div key={i} className="z-10 text-center"> + ).map((time) => ( + <div key={time.getTime()} className="z-10 text-center"> {formatTimestampLabel(time)} </div> ))apps/dashboard/components/logs/chart/index.tsx (1)
91-102: Fix selection bug: updating startTimestamp instead of endTimestamp on drag.Dragging currently freezes the selection to a zero-width range. Update the end timestamp during mouse move.
const handleMouseMove = (e: ChartMouseEvent) => { if (!enableSelection) { return; } if (selection.start) { const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection((prev) => ({ ...prev, end: e.activeLabel, - startTimestamp: timestamp, + endTimestamp: timestamp, })); } };
♻️ Duplicate comments (8)
apps/dashboard/components/ui/chart.tsx (1)
170-172: Cap width to avoid overflow; restore fixed md width for stability.Current md:w-fit can cause jitter and overflow in narrow containers. Add a base max-width guard and fix md width to 360px.
- "grid sm:w-fit md:w-fit md:max-w-[360px] items-start gap-2 rounded-lg border border-gray-6 bg-gray-1 pt-4 pb-2 text-xs shadow-2xl select-none", + "grid max-w-[calc(100vw-2rem)] sm:w-fit md:w-[360px] md:max-w-[360px] items-start gap-2 rounded-lg border border-gray-6 bg-gray-1 pt-4 pb-2 text-xs shadow-2xl select-none",Optional: allow passing aria-* and id to the wrapper (currently swallowed) by spreading remaining div props:
// In the props destructuring add: ...divProps ({ /* existing props */, bottomExplainer, ...divProps }, ref) => { // ... return ( <div ref={ref} role="tooltip" className={cn(/* ... */)} {...divProps}>apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
6-6: Drop toUpperCase() to preserve casing and i18n.Uppercasing harms readability and localization. Let presentation handle casing if needed.
- return format(date, "MMM dd, h:mm a").toUpperCase(); + return format(date, "MMM dd, h:mm a");apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
164-167: Consolidated 7–<30d to perDay looks correct and removes prior duplication.Matches the updated mapping and resolves the earlier duplicate tests note.
apps/dashboard/components/logs/overview-charts/utils.tsx (2)
17-46: Standardize format tokens (spacing and 4‑digit years).Use "h:mm a" (with space) and "MMM yyyy" for clarity and consistency.
Apply:
- if (!granularity) { return format(date, "h:mma"); } + if (!granularity) { return format(date, "h:mm a"); } switch (granularity) { case "perMinute": - return format(date, "h:mm:ssa"); + return format(date, "h:mm:ss a"); case "per5Minutes": case "per15Minutes": case "per30Minutes": - return format(date, "h:mma"); + return format(date, "h:mm a"); case "perHour": case "per2Hours": case "per4Hours": case "per6Hours": case "per12Hours": - return format(date, "MMM d h:mma"); + return format(date, "MMM d h:mm a"); case "perDay": return format(date, "MMM d"); case "per3Days": return format(date, "MMM d"); case "perWeek": return format(date, "MMM d"); case "perMonth": - return format(date, "MMM yy"); + return format(date, "MMM yyyy"); default: - return format(date, "h:mma"); + return format(date, "h:mm a"); }
51-59: Timezone abbreviation must be derived from the hovered timestamp (DST‑aware).Pass the date being formatted; don’t use “now”.
Apply:
-function getTimezoneAbbreviation(): string { - const timezone = - new Intl.DateTimeFormat("en-US", { - timeZoneName: "short", - }) - .formatToParts(new Date()) - .find((part) => part.type === "timeZoneName")?.value || ""; - return timezone; -} +const TZ_FMT = new Intl.DateTimeFormat("en-US", { timeZoneName: "short" }); +function getTimezoneAbbreviation(date: Date): string { + return TZ_FMT + .formatToParts(date) + .find((p) => p.type === "timeZoneName")?.value || ""; +}apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
96-101: Fix correctly routes per-chart granularity into selection handler.This resolves the earlier cross-chart buffer bug. Nice.
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
277-406: Tooltip: DST-correct timezone and robust index matching — solid improvements.Normalization to numeric epochs and deriving tz from the point date address prior bugs.
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
272-401: Tooltip timezone: include TZ for single timestamps and compute it once per hover.Right now, the TZ abbreviation is only shown for intervals; single-point tooltips omit it. Also, TZ is computed later in the function. Compute TZ once from the hovered timestamp and use it for all return paths to keep UI consistent and avoid DST drift across paths.
Apply:
labelFormatter={(_, tooltipPayload) => { if (!tooltipPayload?.[0]?.payload?.originalTimestamp) { return ""; } const currentPayload = tooltipPayload[0].payload; const currentTimestamp = currentPayload.originalTimestamp; + // Normalize and derive timezone once per hover point + const currentTimestampNumeric = + typeof currentTimestamp === "number" + ? currentTimestamp + : +new Date(currentTimestamp); + const pointDate = new Date(currentTimestampNumeric); + const tz = + new Intl.DateTimeFormat("en-US", { timeZoneName: "short" }) + .formatToParts(pointDate) + .find((part) => part.type === "timeZoneName")?.value || ""; // Handle missing data - if (!data?.length) { + if (!data?.length) { return ( <div className="px-4"> <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formatTooltipTimestamp(currentTimestamp, granularity, data)} + {formatTooltipTimestamp(currentTimestampNumeric, granularity, data)} ({tz}) </span> </div> ); } // Handle single timestamp case - if (data.length === 1) { + if (data.length === 1) { return ( <div className="px-4"> <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formatTooltipTimestamp(currentTimestamp, granularity, data)} + {formatTooltipTimestamp(currentTimestampNumeric, granularity, data)} ({tz}) </span> </div> ); } - // Normalize timestamps to numeric for robust comparison - const currentTimestampNumeric = - typeof currentTimestamp === "number" - ? currentTimestamp - : +new Date(currentTimestamp); + // currentTimestampNumeric already computed above // If not found, fallback to single timestamp display if (currentIndex === -1) { return ( <div className="px-4"> <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formatTooltipTimestamp(currentTimestampNumeric, granularity, data)} + {formatTooltipTimestamp(currentTimestampNumeric, granularity, data)} ({tz}) </span> </div> ); } // Fallback when next point invalid if (!nextPoint?.originalTimestamp) { return ( <div className="px-4"> <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formatTooltipTimestamp( + {formatTooltipTimestamp( currentTimestampNumeric, granularity, data, - )} + )} ({tz}) </span> </div> ); } - // Get timezone abbreviation from the actual point date for correct DST handling - const pointDate = new Date(currentTimestampNumeric); - const timezone = - new Intl.DateTimeFormat("en-US", { - timeZoneName: "short", - }) - .formatToParts(pointDate) - .find((part) => part.type === "timeZoneName")?.value || ""; - // Return formatted interval with timezone info return ( <div className="px-4"> <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formattedCurrentTimestamp} - {formattedNextTimestamp} ({timezone}) + {formattedCurrentTimestamp} - {formattedNextTimestamp} ({tz}) </span> </div> ); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/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(6 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(5 hunks)apps/dashboard/components/logs/overview-charts/utils.tsx(5 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)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-25T13:46:08.303Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:1-1
Timestamp: 2025-08-25T13:46:08.303Z
Learning: The NamespaceListDateTime component in apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx is intentionally designed to use the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than the namespace list hook, as clarified by ogzhanolguncu. This coupling is by design, not an architectural issue.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsxapps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx
📚 Learning: 2025-08-25T13:46:34.441Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx:4-4
Timestamp: 2025-08-25T13:46:34.441Z
Learning: The namespace list refresh component (apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx) intentionally uses the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than a namespace-specific hook. This cross-coupling between namespace list components and overview hooks is an architectural design decision.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx
🔇 Additional comments (12)
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
55-56: LGTM: tooltip punctuation and 12-hour format are consistent.apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
14-22: per2Hours/per4Hours/per6Hours are still required – these values are emitted by getTimeseriesGranularity (cases at lines 164–169), covered by tests, used in front-end formatting utilities, and wired through ClickHouse handlers; do not prune them.apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
198-199: 7d exact boundary -> perDay is aligned with the new policy.Looks good.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (2)
36-39: Good: selection buffer keyed off per‑fetch granularity.This prevents zero‑width selections; matches the granularity model.
67-68: Confirm granularity is applied in OverviewBarChart’s tooltip formatter
Manually inspect OverviewBarChart to ensure itslabelFormatter(or tooltip creation) invokescreateTimeIntervalFormatter(data, ..., granularity)rather than omitting thegranularityargument.apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
105-105: Passing per-fetch granularity to the chart — good.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
70-71: Granularity propagation to chart — good.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
120-129: Area chart uses its own granularity for selection — good.apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
80-92: Prop type now matches canonical granularity via alias — good once alias is applied.After applying the alias above, props stay precise and aligned across components.
Please ensure OverviewBarChart and other consumers also use the same type to avoid mismatches.
apps/dashboard/components/logs/chart/index.tsx (3)
34-39: Good: event payload typed as read-only.Immutability here prevents accidental mutation in handlers.
140-142: Good: stable keys for header ticks.Using
time.getTime()removes the need for index keys.
200-205: Good: safe payload mapping and formatter defers display format.This removes the unsafe cast and lets the shared formatter apply global time rules.
apps/dashboard/app/(app)/[workspaceSlug]/apis/[apiId]/_overview/components/charts/index.tsx
Show resolved
Hide resolved
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx
Outdated
Show resolved
Hide resolved
…import and alias from the canonical granularity module ✅ **Single Source of Truth**: No more type drift between local and canonical definitions ✅ **Type Safety**: Better TypeScript integration with the canonical granularity system ✅ **Maintainability**: Changes to granularity types only need to be made in one place
Replace O(n) findIndex call with precomputed Map<number, number> for timestamp-to-index mapping in overview bar chart tooltip handler. - Add useMemo hook to create timestamp lookup map when data changes - Normalize timestamps to numbers during map construction - Replace data.findIndex() with map.get() for constant-time lookup - Eliminates expensive array traversal on every mouse move event Performance impact scales with data size - particularly beneficial for charts with large datasets where findIndex was causing noticeable lag during tooltip interactions.
Added DEFAULT_TIME_BUFFER_MS to the charts to make it easier to maintain for the future.
- Update getTimezoneAbbreviation() to accept optional timestamp parameter - Precompute timezone abbreviation using current timestamp for consistency - Fix fallback cases to use timestamp-aware timezone computation with safe fallbacks - Handle DST boundary crossings in time intervals by computing timezone for both timestamps - Display single timezone abbreviation when consistent, or "TZ1 → TZ2" format for transitions - Ensure displayed timezone always matches the corresponding formatted timestamp Previously, timezone abbreviations were computed using current time rather than the displayed timestamp, causing inconsistencies. Time intervals also showed incorrect timezone info when spanning DST boundaries.
- Import CompoundTimeseriesGranularity type to replace raw string types - Add microsecond timestamp detection and conversion to milliseconds - Fix Date construction for 16-digit microsecond Unix timestamps - Enhance type safety to prevent granularity typos and drift
…o module scope Replace per-call Intl.DateTimeFormat construction with singleton TZ_FORMATTER in logs utils and overview-charts utils to reduce object creation overhead
…ent-date-times' of github.com:unkeyed/unkey into eng-2053-tooltip-times-are-wrong-when-looking-at-different-date-times
|
@codereabbitai resolve |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (18)
apps/dashboard/components/ui/chart.tsx (3)
181-183: Use stable, unique React keys for payload rows.item.dataKey can be undefined/duplicated across series; fall back to name/index to avoid key collisions.
Apply:
- key={item.dataKey} + key={`${item.dataKey ?? item.name ?? index}`}
235-239: Bug: zero values are hidden due to truthy check.0 is a valid value and should render; current condition suppresses it.
Apply:
- {item.value && ( + {item.value !== undefined && item.value !== null && (
253-253: Display name mismatch.Set displayName to the actual component name for clearer React DevTools traces.
Apply:
-ChartTooltipContent.displayName = "ChartTooltip"; +ChartTooltipContent.displayName = "ChartTooltipContent";apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
67-76: Don’t treat 0 timestamps as “missing”. Use nullish checks.
if (!startTime && !endTime)wrongly treats 0 as absent. Align with??usage and check for null/undefined explicitly.Apply:
- if (!startTime && !endTime) { + if (startTime == null && endTime == null) {
97-107: Three redundant branches all return perHour — collapse to one.These consecutive conditions set the same granularity; keep only the smallest threshold (16h).
Apply:
} else if (timeRange >= WEEK_IN_MS) { granularity = "perDay"; - } else if (timeRange >= DAY_IN_MS * 3) { - granularity = "perHour"; - } else if (timeRange >= DAY_IN_MS) { - granularity = "perHour"; } else if (timeRange >= HOUR_IN_MS * 16) { granularity = "perHour";apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (3)
84-154: Regular-context tests are out of sync with new mapping (failures likely).Names and expectations don’t match the updated logic (e.g., 3h → perMinute; 4–<8h → per5Minutes; 12–<16h → per30Minutes; ≥16h → perHour; 24h/3–<7d now → perHour).
Apply:
{ - name: "should use per5Minutes for timeRange >= 2 hours & < 4 hours", + name: "should use perMinute for timeRange >= 2 hours & < 4 hours", startTime: getTime(HOUR_IN_MS * 3), - expectedGranularity: "per5Minutes", + expectedGranularity: "perMinute", }, { - name: "should use per15Minutes for timeRange >= 4 hours & < 6 hours", + name: "should use per5Minutes for timeRange >= 4 hours & < 6 hours", startTime: getTime(HOUR_IN_MS * 5), expectedGranularity: "per5Minutes", }, { - name: "should use per30Minutes for timeRange >= 6 hours & < 8 hours", + name: "should use per5Minutes for timeRange >= 6 hours & < 8 hours", startTime: getTime(HOUR_IN_MS * 7), expectedGranularity: "per5Minutes", }, { - name: "should use perHour for timeRange >= 12 hours & < 16 hours", + name: "should use per30Minutes for timeRange >= 12 hours & < 16 hours", startTime: getTime(HOUR_IN_MS * 14), expectedGranularity: "per30Minutes", }, { - name: "should use per2Hours for timeRange >= 16 hours & < 24 hours", + name: "should use perHour for timeRange >= 16 hours & < 24 hours", startTime: getTime(HOUR_IN_MS * 20), - expectedGranularity: "per2Hours", + expectedGranularity: "perHour", }, { - name: "should use per4Hours for timeRange >= 24 hours & < 3 days", + name: "should use perHour for timeRange >= 24 hours & < 3 days", startTime: getTime(DAY_IN_MS * 2), - expectedGranularity: "per4Hours", + expectedGranularity: "perHour", }, { - name: "should use per6Hours for timeRange >= 3 days & < 7 days", + name: "should use perHour for timeRange >= 3 days & < 7 days", startTime: getTime(DAY_IN_MS * 5), - expectedGranularity: "per6Hours", + expectedGranularity: "perHour", }, @@ it("should handle edge case at exactly 2 hours boundary", () => { const result = getTimeseriesGranularity("forRegular", FIXED_NOW - HOUR_IN_MS * 2, FIXED_NOW); - expect(result.granularity).toBe("per5Minutes"); + expect(result.granularity).toBe("perMinute"); });Also update the test names to reflect new expectations as in the diff.
265-272: Use case drift: 24h “forRegular” now buckets to perHour.Align expectation with new mapping.
Apply:
- expect(result.granularity).toBe("per4Hours"); + expect(result.granularity).toBe("perHour");
211-233: Update “type compatibility” allowlist if you expand Regular union.If you adopt the type fix, include the new Regular values here to keep this check meaningful.
Proposed list:
const validGranularities = [ - "perMinute","per5Minutes","per15Minutes","per30Minutes", - "perHour","per2Hours","per4Hours","per6Hours", + "perMinute","per5Minutes","per15Minutes","per30Minutes", + "perHour","per2Hours","per4Hours","per6Hours", + "perDay","per3Days","perWeek","perMonth", ];apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
1-1: Add "use client" directive to API charts components.These components call client hooks (
useFilters, context) and must be Client Components. Apply at the top of both files:
- apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx
- apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx
+ "use client";apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (2)
1-1: Mark as Client Component.This file uses client hooks; add the directive.
+ "use client";
25-55: Deduplicate time-selection filter logic.The selection-to-filter code is identical to the ratelimits wrapper; extract a helper to keep behavior consistent.
Example:
// shared/utils/applyTimeSelection.ts export function applyTimeSelection(filters, updateFilters, start: number, end: number, granularity?: Granularity) { const active = filters.filter((f) => !["startTime","endTime","since"].includes(f.field)); const buf = start === end && granularity ? getTimeBufferForGranularity(granularity) : 0; updateFilters([ ...active, { field: "startTime", value: start, id: crypto.randomUUID(), operator: "is" }, { field: "endTime", value: end + buf, id: crypto.randomUUID(), operator: "is" }, ]); }apps/dashboard/components/logs/chart/index.tsx (2)
91-102: Fix drag-selection: update endTimestamp (not startTimestamp) while moving.Current code mutates startTimestamp on mouse move; this inverts semantics and is error-prone.
- const handleMouseMove = (e: ChartMouseEvent) => { - if (!enableSelection || e.activeLabel === undefined) { + const handleMouseMove = (e: ChartMouseEvent) => { + if (!enableSelection || e.activeLabel === undefined) { return; } - if (selection.start) { + if (selection.start !== undefined) { const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; setSelection((prev) => ({ ...prev, end: e.activeLabel, - startTimestamp: timestamp, + endTimestamp: timestamp, })); } };
109-122: Guard against 0-valued labels; check for undefined instead of truthiness.Selections at x=0 currently fail the commit path.
- if (selection.start && selection.end && onSelectionChange) { + if (selection.start !== undefined && selection.end !== undefined && onSelectionChange) { if (!selection.startTimestamp || !selection.endTimestamp) { return; }apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (2)
96-107: Normalize selection timestamps to ms on mouse downPassing raw originalTimestamp can be µs, causing 1000x selection ranges upstream.
- const handleMouseDown = (e: ChartMouseEvent) => { + const handleMouseDown = (e: ChartMouseEvent) => { if (!enableSelection || e.activeLabel === undefined) { return; } const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; + const ts = parseTimestamp(timestamp as any); + if (!Number.isFinite(ts)) return; setSelection({ start: e.activeLabel, end: e.activeLabel, - startTimestamp: timestamp, - endTimestamp: timestamp, + startTimestamp: ts, + endTimestamp: ts, }); };
345-350: Normalize axis tick range inputscalculateTimePoints should receive ms, not raw possibly-µs values.
- ? calculateTimePoints( - data[0]?.originalTimestamp ?? Date.now(), - data.at(-1)?.originalTimestamp ?? Date.now() - ).map((time, i) => ( + ? calculateTimePoints( + parseTimestamp(data[0]?.originalTimestamp ?? Date.now()), + parseTimestamp(data.at(-1)?.originalTimestamp ?? Date.now()) + ).map((time, i) => (apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
45-48: Optional: guard getTimeBufferForGranularity at runtimeIf an unexpected value slips through, avoid throwing.
- if (start === end && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + if (start === end && granularity) { + try { + adjustedEnd = end + getTimeBufferForGranularity(granularity); + } catch { + // no-op: keep end unchanged on invalid granularity + } + }apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (2)
150-160: Compute min/max/avg in one pass to cut allocations.Avoid creating a values array and multiple traversals.
- labelsWithDefaults.metrics.forEach((metric) => { - const values = data.map((d) => d[metric.key] as number); - const min = data.length > 0 ? Math.min(...values) : 0; - const max = data.length > 0 ? Math.max(...values) : 0; - const avg = - data.length > 0 - ? values.reduce((sum, val) => sum + val, 0) / data.length - : 0; - - ranges[metric.key] = { min, max, avg }; - }); + labelsWithDefaults.metrics.forEach((metric) => { + let min = Infinity; + let max = -Infinity; + let sum = 0; + for (const d of data) { + const v = (d[metric.key] as number) ?? 0; + if (v < min) min = v; + if (v > max) max = v; + sum += v; + } + if (data.length === 0) { + min = 0; + max = 0; + } + const avg = data.length > 0 ? sum / data.length : 0; + ranges[metric.key] = { min, max, avg }; + });
92-104: Memoize event handlers with useCallback to reduce re-renders.These are passed to Recharts and recreated on each render.
-import { useMemo, useState } from "react"; +import { useMemo, useState, useCallback } from "react"; @@ - const handleMouseDown = (e: ChartMouseEvent) => { + const handleMouseDown = useCallback((e: ChartMouseEvent) => { if (!enableSelection || e.activeLabel === undefined) { return; } const timestamp = e?.activePayload?.[0]?.payload?.originalTimestamp; setSelection({ start: e.activeLabel, end: e.activeLabel, startTimestamp: timestamp, endTimestamp: timestamp, }); - }; + }, [enableSelection]); @@ - const handleMouseMove = (e: ChartMouseEvent) => { + const handleMouseMove = useCallback((e: ChartMouseEvent) => { if (!enableSelection || !selection.start || e.activeLabel === undefined) { return; } const timestamp = e?.activePayload?.[0]?.payload?.originalTimestamp; const activeLabel = e.activeLabel; setSelection((prev) => ({ ...prev, end: activeLabel, endTimestamp: timestamp, })); - }; + }, [enableSelection, selection.start]); @@ - const handleMouseUp = () => { + const handleMouseUp = useCallback(() => { if (!enableSelection) { return; } if (selection.start && selection.end && onSelectionChange) { if (!selection.startTimestamp || !selection.endTimestamp) { return; } const [start, end] = [ selection.startTimestamp, selection.endTimestamp, ].sort((a, b) => a - b); onSelectionChange({ start, end }); } setSelection({ start: "", end: "", startTimestamp: undefined, endTimestamp: undefined, }); - }; + }, [ + enableSelection, + onSelectionChange, + selection.start, + selection.end, + selection.startTimestamp, + selection.endTimestamp, + ]);Also applies to: 105-117, 118-139, 16-16
♻️ Duplicate comments (10)
apps/dashboard/components/ui/chart.tsx (2)
167-167: A11Y: Good fix—tooltip now uses only role="tooltip" (aria-live removed).This matches ARIA guidance for tooltips. No further changes needed here.
169-169: Prevent overflow on small viewports—add a base max-width cap.At <md breakpoints the tooltip can still overflow. Add a viewport-based max-width.
Apply:
- "grid sm:w-fit md:w-fit md:max-w-[360px] items-start gap-2 rounded-lg border border-gray-6 bg-gray-1 pt-4 pb-2 text-xs shadow-2xl select-none", + "grid max-w-[calc(100vw-2rem)] sm:w-fit md:w-fit md:max-w-[360px] items-start gap-2 rounded-lg border border-gray-6 bg-gray-1 pt-4 pb-2 text-xs shadow-2xl select-none",apps/dashboard/components/logs/parseTimestamp.ts (2)
32-35: Make JSDoc example timezone-stable.Local Date constructor yields locale-dependent millis. Prefer UTC in examples.
- * // Date object - * parseTimestamp(new Date(2022, 0, 1)) // returns 1641013200000 + * // Date object (UTC) + * parseTimestamp(new Date(Date.UTC(2022, 0, 1))) // returns 1640995200000
44-50: Reject mixed numeric strings; prefer Number over parseFloat.parseFloat("123abc") yields 123. Enforce numeric-only strings and parse with Number to avoid silent truncation.
- timestampNum = Number.parseFloat(trimmed); + if (!/^[+-]?\d+(\.\d+)?([eE][+-]?\d+)?$/.test(trimmed)) { + return 0; + } + timestampNum = Number(trimmed);apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
33-33: Optional: avoid uppercasing month names
toUpperCase()can look shouty and harms i18n. Consider dropping it unless product-specified.- return memoizedFormat(date, "MMM dd, h:mm a").toUpperCase(); + return memoizedFormat(date, "MMM dd, h:mm a");apps/dashboard/components/logs/chart/index.tsx (2)
34-39: Resolved: exported immutable ChartMouseEvent.Readonly payload and exported type look good; reuse across charts as intended.
200-205: Resolved: safe tooltip payload mapping and formatter-driven time format.Avoids unsafe casts and centralizes 12/24h formatting.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
32-40: Type granularity explicitly with the shared unionAvoids coupling to a variable’s type and prevents invalid strings.
+import type { CompoundTimeseriesGranularity } from "@/lib/trpc/routers/utils/granularity"; ... const handleSelectionChange = ({ start, end, granularity, }: { start: number; end: number; - granularity?: typeof verificationGranularity; + granularity?: CompoundTimeseriesGranularity; }) => {apps/dashboard/components/logs/overview-charts/utils.tsx (2)
30-57: Standardize date-fns tokens (spacing before “a”, 4‑digit years).Use “h:mm a”/“h:mm:ss a” and “MMM yyyy” for readability and consistency across branches.
- return format(date, "h:mma"); + return format(date, "h:mm a"); @@ - return format(date, "h:mm:ssa"); + return format(date, "h:mm:ss a"); @@ - return format(date, "h:mma"); + return format(date, "h:mm a"); @@ - return format(date, "MMM d h:mma"); + return format(date, "MMM d h:mm a"); @@ - return format(date, "MMM yy"); + return format(date, "MMM yyyy"); @@ - return format(date, "h:mma"); + return format(date, "h:mm a");
150-156: Include timezone in the single‑point fallback (next point missing).Match the other single‑point branches for consistency.
- 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> + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx(4 hunks)apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx(1 hunks)apps/dashboard/app/(app)/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(2 hunks)apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx(14 hunks)apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx(14 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(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-25T13:46:08.303Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx:1-1
Timestamp: 2025-08-25T13:46:08.303Z
Learning: The NamespaceListDateTime component in apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-datetime/index.tsx is intentionally designed to use the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than the namespace list hook, as clarified by ogzhanolguncu. This coupling is by design, not an architectural issue.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsxapps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx
📚 Learning: 2025-08-25T13:46:34.441Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#3834
File: apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx:4-4
Timestamp: 2025-08-25T13:46:34.441Z
Learning: The namespace list refresh component (apps/dashboard/app/(app)/ratelimits/_components/controls/components/namespace-list-refresh.tsx) intentionally uses the overview hook (useFilters from @/app/(app)/ratelimits/[namespaceId]/_overview/hooks/use-filters) rather than a namespace-specific hook. This cross-coupling between namespace list components and overview hooks is an architectural design decision.
Applied to files:
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx
📚 Learning: 2024-12-03T14:07:45.173Z
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2143
File: apps/dashboard/components/ui/group-button.tsx:21-31
Timestamp: 2024-12-03T14:07:45.173Z
Learning: In the `ButtonGroup` component (`apps/dashboard/components/ui/group-button.tsx`), avoid suggesting the use of `role="group"` in ARIA attributes.
Applied to files:
apps/dashboard/components/ui/chart.tsx
🔇 Additional comments (28)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
35-41: Defaults look sensible and consistent with buckets.Default granularity/durations align with the selection logic.
145-183: Time buffer mapping covers all emitted granularities.Good coverage; defaulting to 5 minutes is reasonable. No action.
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (3)
164-167: Consolidated 7–<30d verifications bucket to perDay — good.This matches the simplified server mapping.
170-172: 59d probe for 30–<60d bucket — good upper-edge check.Nice guard near the boundary.
198-199: Edge at exactly 7d → perDay — matches code.Consistent with the WEEK_IN_MS branch.
apps/dashboard/components/logs/parseTimestamp.ts (1)
67-79: LGTM! Magnitude-based timestamp unit detection is robust.The current implementation correctly uses numeric magnitude thresholds (>=1e18 for nanoseconds, >=1e15 for microseconds, >=1e12 for milliseconds, else seconds) to detect timestamp units. This approach is robust against exponential formatting issues that would affect digit-counting approaches.
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (3)
4-29: LGTM! Efficient memoization implementation with proper cache eviction.The memoization cache implementation with bounded size (1000 entries) and FIFO eviction strategy is well-designed. This will significantly improve performance when formatting many timestamps in chart rendering scenarios.
44-48: LGTM! Consistent 12-hour format adoption across granularities.The conversion from 24-hour to 12-hour format with AM/PM indicators improves readability and user experience. The granularity-specific formatting patterns are well-chosen for each time scale.
80-95: LGTM! Robust timestamp parsing with proper coercion.The implementation correctly handles numeric strings by coercing them to numbers, preventing Invalid Date errors. The fallback chain (Unix micro → numeric → string) ensures robust handling of various input formats.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
105-105: LGTM! Granularity prop correctly passed to OverviewBarChart.The addition of the
granularityprop toOverviewBarChartaligns with the broader PR objective of making tooltips granularity-aware. The granularity value is properly sourced from theuseFetchRatelimitOverviewTimeserieshook and will enable proper timestamp formatting in tooltips.apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
70-71: LGTM: granularity is correctly forwarded to the chart.This aligns tooltip/selection with bucket width.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
67-67: LGTM: granularity plumbed to OverviewBarChart.apps/dashboard/components/logs/utils.tsx (6)
42-55: Good normalization and guardsparseTimestamp + finite/zero checks prevent Invalid Date throws and microsecond bugs.
58-72: Correct: day-span check uses normalized endpointsUsing parseTimestamp for first/last avoids µs→ms drift.
136-146: O(1) lookup fallback is solidMap lookup with finite guard and linear fallback is appropriate.
200-215: Clear, compact interval renderingConcise JSX and reuse of formatTooltipTimestamp reads well.
29-35: Type precision is correctUsing CompoundTimeseriesGranularity here tightens the API.
95-103: Inconsistent with the PR summarySummary claims “pair-of-timezones when different,” but the current code showed only a single TZ; adopt the paired display as proposed above.
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (3)
72-86: Good: precomputed timestamp→index Map with normalizationAvoids per-hover O(n) scans and handles type/precision issues.
132-136: Ensure numeric sort of timestampsThe sort will be correct once both are normalized numbers.
301-310: Nice: centralized label formattingUsing formatTooltipInterval with the prebuilt map keeps things consistent and fast.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (2)
96-101: Correct: per-chart selection buffering uses the chart’s granularityThis fixes cross-chart buffer mismatches.
120-125: Also correct for the keys chartConsistent with the bar chart change above.
apps/dashboard/components/logs/overview-charts/utils.tsx (1)
164-173: DST-aware TZ display looks good.Showing “TZ → TZ” when abbreviations differ is correct and user-friendly.
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (4)
71-83: Precomputed timestamp→index map is a solid perf win.Eliminates per-hover scans; guards against NaN. Nice.
257-260: YAxis domain headroom change looks good.Adds 10% top padding; hidden axis keeps UI clean.
329-339: Good: normalize axis endpoints with parseTimestamp.Prevents type ambiguity downstream.
289-298: No changes needed—formatTooltipInterval and labelFormatter types align.formatTooltipInterval returns either a string or JSX element (both valid ReactNode) and ChartTooltipContent props extend Recharts’ Tooltip props (which include the labelFormatter callback).
eng-2053-tooltip-times-are-wrong-when-looking-at-different-date-times
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/dashboard/components/logs/parseTimestamp.ts (1)
5-7: Doc mismatch: “digit count” vs implementation.Update the JSDoc to reflect magnitude-threshold detection (ns/µs/ms/s).
- * This helper detects timestamp precision based on digit count and converts - * to milliseconds: nanoseconds (>=19 digits) ÷ 1e6, microseconds (>=16 digits) ÷ 1e3, - * milliseconds (>=13 digits) unchanged, seconds (<13 digits) × 1e3. + * This helper detects precision by numeric magnitude and converts to milliseconds: + * nanoseconds (>= 1e18) ÷ 1e6, microseconds (>= 1e15) ÷ 1e3, + * milliseconds (>= 1e12) unchanged, seconds (< 1e12) × 1e3.apps/dashboard/components/logs/chart/index.tsx (1)
187-193: Consider showing tooltip even when total is 0.Hiding tooltips on zero loses the time context; UX may be better if shown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/components/logs/chart/index.tsx(10 hunks)apps/dashboard/components/logs/parseTimestamp.ts(1 hunks)
🔇 Additional comments (7)
apps/dashboard/components/logs/parseTimestamp.ts (4)
32-35: Make the Date example timezone-stable (UTC).Use UTC so the example result isn’t locale-dependent.
- * // Date object - * parseTimestamp(new Date(2022, 0, 1)) // returns 1641013200000 + * // Date object (UTC) + * parseTimestamp(new Date(Date.UTC(2022, 0, 1))) // returns 1640995200000
50-74: BigInt path to preserve precision for long integer strings looks solid.
93-107: Magnitude-threshold normalization (ns/µs/ms/s) LGTM.
76-77: Reject mixed numeric strings; replace parseFloat with validated Number.Avoid silently accepting values like "123abc".
- // Fall back to parseFloat for non-integer or shorter strings - timestampNum = Number.parseFloat(trimmed); + // Fall back to strict numeric parsing for non-integer or shorter strings + if (!/^[+-]?\d+(\.\d+)?$/.test(trimmed)) { + return 0; + } + timestampNum = Number(trimmed);apps/dashboard/components/logs/chart/index.tsx (3)
27-33: Narrowing Selection to numbers is a good cleanup.
40-45: Exported, readonly ChartMouseEvent is appropriate.
223-233: Granularity-aware labelFormatter and payload mapping look correct.
| const handleMouseDown = (e: ChartMouseEvent) => { | ||
| if (!enableSelection || e.activeLabel === undefined) { | ||
| return; | ||
| } | ||
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | ||
| const numericLabel = Number(e.activeLabel); | ||
| setSelection({ | ||
| start: e.activeLabel, | ||
| end: e.activeLabel, | ||
| start: numericLabel, | ||
| end: numericLabel, | ||
| startTimestamp: timestamp, | ||
| endTimestamp: timestamp, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Guard against non-numeric activeLabel to avoid NaN selection.
Add a finite-number check before setting state.
const handleMouseDown = (e: ChartMouseEvent) => {
if (!enableSelection || e.activeLabel === undefined) {
return;
}
- const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp;
- const numericLabel = Number(e.activeLabel);
+ const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp;
+ const numericLabel = Number(e.activeLabel);
+ if (!Number.isFinite(numericLabel)) {
+ return;
+ }
+ if (timestamp === undefined) {
+ return;
+ }
setSelection({
start: numericLabel,
end: numericLabel,
startTimestamp: timestamp,
endTimestamp: timestamp,
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleMouseDown = (e: ChartMouseEvent) => { | |
| if (!enableSelection || e.activeLabel === undefined) { | |
| return; | |
| } | |
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | |
| const numericLabel = Number(e.activeLabel); | |
| setSelection({ | |
| start: e.activeLabel, | |
| end: e.activeLabel, | |
| start: numericLabel, | |
| end: numericLabel, | |
| startTimestamp: timestamp, | |
| endTimestamp: timestamp, | |
| }); | |
| }; | |
| const handleMouseDown = (e: ChartMouseEvent) => { | |
| if (!enableSelection || e.activeLabel === undefined) { | |
| return; | |
| } | |
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | |
| const numericLabel = Number(e.activeLabel); | |
| if (!Number.isFinite(numericLabel)) { | |
| return; | |
| } | |
| if (timestamp === undefined) { | |
| return; | |
| } | |
| setSelection({ | |
| start: numericLabel, | |
| end: numericLabel, | |
| startTimestamp: timestamp, | |
| endTimestamp: timestamp, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In apps/dashboard/components/logs/chart/index.tsx around lines 84 to 96, guard
against non-numeric e.activeLabel before calling Number(...) and setting
selection: compute numericLabel = Number(e.activeLabel) and if
!Number.isFinite(numericLabel) return (do not set selection), otherwise use
numericLabel and the timestamp as before; this prevents NaN values from being
stored in selection.
| const handleMouseMove = (e: ChartMouseEvent) => { | ||
| if (!enableSelection || e.activeLabel === undefined) { | ||
| return; | ||
| } | ||
| if (selection.start) { | ||
| if (selection.start !== undefined) { | ||
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | ||
| setSelection((prev) => ({ | ||
| ...prev, | ||
| end: e.activeLabel, | ||
| startTimestamp: timestamp, | ||
| end: Number(e.activeLabel), | ||
| endTimestamp: timestamp, | ||
| })); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Same NaN guard needed during drag.
const handleMouseMove = (e: ChartMouseEvent) => {
if (!enableSelection || e.activeLabel === undefined) {
return;
}
if (selection.start !== undefined) {
- const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp;
+ const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp;
+ const numericLabel = Number(e.activeLabel);
+ if (!Number.isFinite(numericLabel)) {
+ return;
+ }
+ if (timestamp === undefined) {
+ return;
+ }
setSelection((prev) => ({
...prev,
- end: Number(e.activeLabel),
+ end: numericLabel,
endTimestamp: timestamp,
}));
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleMouseMove = (e: ChartMouseEvent) => { | |
| if (!enableSelection || e.activeLabel === undefined) { | |
| return; | |
| } | |
| if (selection.start) { | |
| if (selection.start !== undefined) { | |
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | |
| setSelection((prev) => ({ | |
| ...prev, | |
| end: e.activeLabel, | |
| startTimestamp: timestamp, | |
| end: Number(e.activeLabel), | |
| endTimestamp: timestamp, | |
| })); | |
| } | |
| }; | |
| const handleMouseMove = (e: ChartMouseEvent) => { | |
| if (!enableSelection || e.activeLabel === undefined) { | |
| return; | |
| } | |
| if (selection.start !== undefined) { | |
| const timestamp = e.activePayload?.[0]?.payload?.originalTimestamp; | |
| const numericLabel = Number(e.activeLabel); | |
| if (!Number.isFinite(numericLabel)) { | |
| return; | |
| } | |
| if (timestamp === undefined) { | |
| return; | |
| } | |
| setSelection((prev) => ({ | |
| ...prev, | |
| end: numericLabel, | |
| endTimestamp: timestamp, | |
| })); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/dashboard/components/logs/chart/index.tsx around lines 98 to 110, the
mouse-move handler sets selection.end by coercing e.activeLabel to Number
without guarding against NaN which can corrupt the selection during drag; modify
the handler to coerce e.activeLabel once, check Number.isFinite (or
!Number.isNaN) and only update selection.end and endTimestamp when the coerced
label is a valid number (otherwise ignore the update or keep previous
selection), and keep extracting the originalTimestamp as the endTimestamp
fallback only when payload exists.
…-tooltip-times-are-wrong-when-looking-at-different-date-times
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
eng-2053-tooltip-times-are-wrong-when-looking-at-different-date-times
2 participants
What does this PR do?
Fixes # (issue)
#3189
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated