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
📝 WalkthroughWalkthroughThreads per-chart granularity from timeseries hooks into multiple charts, makes selection and tooltip formatting granularity-aware (including timestamp parsing and 12‑hour formatting), updates granularity mapping logic and tests, and introduces robust mouse-based selection handlers emitting numeric timestamps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Chart as Chart (Bar/Area)
participant Container as Parent Container
participant Utils as Granularity/Time Utils
rect rgba(220,235,255,0.45)
User->>Chart: mouseDown(activeLabel)
Chart->>Chart: record startTimestamp, start label
User->>Chart: mouseMove(activeLabel)
Chart->>Chart: update endTimestamp, end label
User->>Chart: mouseUp
Chart->>Container: onSelectionChange(startTs, endTs, granularity)
end
alt single-point selection (start==end)
Container->>Utils: getTimeBufferForGranularity(granularity)
Utils-->>Container: bufferMs
Container-->>Container: endTs = startTs + bufferMs
else range selection
Container-->>Container: sort [startTs,endTs]
end
Container-->>Chart: re-render with granularity
Chart->>Chart: format tooltip via formatTooltipInterval(payloadTs, data, granularity, tsIndexMap)
note right of Chart: Tooltip uses parseTimestamp, timezone/DST-aware intervals\nand 12-hour am/pm formatting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs. ✨ Finishing Touches
🧪 Generate unit tests
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.
1 Skipped Deployment
|
|
@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)/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
✅ Actions performedComments resolved. |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (2)
13-22: Handle numeric string timestamps to avoid Invalid Date.
new Date("1694058387000")is invalid. Coerce numeric strings before constructing Date.- const localDate = new Date(value); + const v = + typeof value === "string" && !Number.isNaN(Number(value)) ? Number(value) : value; + const localDate = new Date(v);
43-51: Make microseconds detection numeric and future-proof.Length check is brittle; rely on magnitude. Also use 1e6 for clarity.
-const unixMicroToDate = (unix: string | number): Date => { - return fromUnixTime(Number(unix) / 1000 / 1000); -}; +const unixMicroToDate = (unix: string | number): Date => { + return fromUnixTime(Number(unix) / 1e6); +}; -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 < 1e16; +};apps/dashboard/lib/trpc/routers/utils/granularity.ts (4)
65-74: Use nullish checks; current truthiness check misclassifies 0 epoch.
!startTimetreats 0 as missing. Use== null.- if (!startTime && !endTime) { + if (startTime == null && endTime == null) {
76-82: Guard against reversed time range (start > end).Swap to avoid negative ranges; return normalized times.
- const effectiveEndTime = endTime ?? now; + let effectiveEndTime = endTime ?? now; // Set default start time if missing (defaults vary by context) - const defaultDuration = context === "forVerifications" ? DAY_IN_MS : HOUR_IN_MS; - const effectiveStartTime = startTime ?? effectiveEndTime - defaultDuration; - const timeRange = effectiveEndTime - effectiveStartTime; + const defaultDuration = context === "forVerifications" ? DAY_IN_MS : HOUR_IN_MS; + let effectiveStartTime = startTime ?? effectiveEndTime - defaultDuration; + if (effectiveStartTime > effectiveEndTime) { + const tmp = effectiveStartTime; + effectiveStartTime = effectiveEndTime; + effectiveEndTime = tmp; + } + const timeRange = effectiveEndTime - effectiveStartTime;
144-180: Sanity check: month buffer constant.If “perMonth” aligns to calendar months elsewhere, a fixed 30-day buffer may clip/over-expand. If acceptable, ignore; otherwise consider deriving from selected range or using 31d.
107-128: Align forRegular tests with simplified mapping
In granularity.test.ts, replace the expectedGranularity for the 20 h, 2 d, and 5 d regular-context cases (currently “per2Hours”, “per4Hours”, “per6Hours”) with “perHour”.apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (1)
84-156: Regular tests are out of sync with current mapping (names and expectations).Update expectations to match simplified mapping (perMinute → per5Minutes at ≥4h, per30Minutes at ≥8h, perHour at ≥16h, ≥1d, ≥3d, perDay at ≥7d). Also fix misleading test names.
{ - name: "should use per5Minutes for timeRange >= 2 hours & < 4 hours", - startTime: getTime(HOUR_IN_MS * 3), - expectedGranularity: "per5Minutes", + name: "should use perMinute for timeRange >= 2 hours & < 4 hours", + startTime: getTime(HOUR_IN_MS * 3), + 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 perHour for timeRange >= 12 hours & < 16 hours", startTime: getTime(HOUR_IN_MS * 14), - expectedGranularity: "per30Minutes", + expectedGranularity: "perHour", }, { - 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"); }); @@ it("should handle a 24-hour dashboard view correctly", () => { const oneDayAgo = FIXED_NOW - DAY_IN_MS; const result = getTimeseriesGranularity("forRegular", oneDayAgo, FIXED_NOW); - expect(result.granularity).toBe("per4Hours"); + expect(result.granularity).toBe("perHour"); expect(result.startTime).toBe(oneDayAgo); expect(result.endTime).toBe(FIXED_NOW); });apps/dashboard/components/logs/chart/index.tsx (2)
91-103: Drag selection bug: endTimestamp never updates; truthy check blocks epoch 0.Update endTimestamp on move and check start explicitly against undefined.
- 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-116: Use explicit undefined checks; avoid falsy pitfalls for 0.Ensure epoch 0 is handled and timestamps exist before emitting selection.
- if (selection.start && selection.end && onSelectionChange) { - if (!selection.startTimestamp || !selection.endTimestamp) { + if (selection.start !== undefined && selection.end !== undefined && onSelectionChange) { + if (selection.startTimestamp == null || selection.endTimestamp == null) { return; }apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
94-121: Optional: stabilize handlers with useCallback.Prevents unnecessary re-renders of Chart children when parent state changes.
-import { useEffect, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; ... -const handleMouseDown = (e: ChartMouseEvent) => { +const handleMouseDown = useCallback((e: ChartMouseEvent) => { ... -}; +}, [enableSelection]); ... -const handleMouseMove = (e: ChartMouseEvent) => { +const handleMouseMove = useCallback((e: ChartMouseEvent) => { ... -}; +}, [enableSelection, selection.start]);apps/dashboard/components/logs/overview-charts/utils.tsx (1)
139-148: Optional: when next point is missing, synthesize an interval using a buffer.Improves informativeness by showing an interval end derived from a safe default.
- const nextPoint = data[currentIndex + 1]; - if (!nextPoint) { - return ( - <div className="px-4"> - <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> - {formattedCurrentTimestamp} ({timezoneAbbr}) - </span> - </div> - ); - } + const nextPoint = data[currentIndex + 1]; + if (!nextPoint) { + const nextDate = new Date(currentDate.getTime() + DEFAULT_TIME_BUFFER_MS); + const formattedNextTimestamp = granularity + ? formatTimeForGranularity(nextDate, granularity) + : format(nextDate, timeFormat); + const endTz = getTimezoneAbbreviation(nextDate); + const tzLabel = timezoneAbbr === endTz ? timezoneAbbr : `${timezoneAbbr} → ${endTz}`; + return ( + <div className="px-4"> + <span className="font-mono text-accent-9 text-xs whitespace-nowrap"> + {formattedCurrentTimestamp} - {formattedNextTimestamp} ({tzLabel}) + </span> + </div> + ); + }apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (2)
90-101: Normalize selection timestamps before sorting/emitting.
originalTimestampmay be string/Date; store numeric via parseTimestamp to ensure correct sort and type for onSelectionChange.- const handleMouseDown = (e: ChartMouseEvent) => { + const handleMouseDown = (e: ChartMouseEvent) => { if (!enableSelection || e.activeLabel === undefined) { return; } - const timestamp = e?.activePayload?.[0]?.payload?.originalTimestamp; + const rawTs = e?.activePayload?.[0]?.payload?.originalTimestamp; + const timestamp = + rawTs !== undefined ? parseTimestamp(rawTs as any) : undefined; setSelection({ start: e.activeLabel, end: e.activeLabel, startTimestamp: timestamp, endTimestamp: timestamp, }); }; @@ - const handleMouseMove = (e: ChartMouseEvent) => { + const handleMouseMove = (e: ChartMouseEvent) => { if (!enableSelection || !selection.start || e.activeLabel === undefined) { return; } - const timestamp = e?.activePayload?.[0]?.payload?.originalTimestamp; + const rawTs = e?.activePayload?.[0]?.payload?.originalTimestamp; + const timestamp = + rawTs !== undefined ? parseTimestamp(rawTs as any) : undefined; const activeLabel = e.activeLabel; setSelection((prev) => ({ ...prev, end: activeLabel, endTimestamp: timestamp, })); }; @@ - const [start, end] = [ - selection.startTimestamp, - selection.endTimestamp, - ].sort((a, b) => a - b); + const [start, end] = [ + selection.startTimestamp as number, + selection.endTimestamp as number, + ].sort((a, b) => a - b); onSelectionChange({ start, end });Also applies to: 103-114, 124-129
148-156: Average uses data.length instead of count of valid values; may skew/NaN.Compute over finite numbers only and divide by values.length.
- labelsWithDefaults.metrics.forEach((metric) => { - const values = data.map((d) => d[metric.key] as number); + labelsWithDefaults.metrics.forEach((metric) => { + const values = data + .map((d) => Number(d[metric.key])) + .filter((v) => Number.isFinite(v)); 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; + const avg = + values.length > 0 + ? values.reduce((sum, val) => sum + val, 0) / values.length + : 0;
♻️ Duplicate comments (8)
apps/dashboard/components/ui/chart.tsx (1)
169-169: Prevent overflow on small viewports and stabilize width on md+.Without a base max-width, the tooltip can overflow narrow containers; md:w-fit may also jitter as content changes. Cap width on small screens and lock 360px on md+.
- "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] 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/chart/utils/format-timestamp.ts (1)
4-7: Drop forced uppercase; leave casing to presentation/i18n.Uppercasing harms i18n and is a view concern. Keep the formatted string as-is.
export const formatTimestampLabel = (timestamp: string | number | Date) => { const date = new Date(timestamp); - return format(date, "MMM dd, h:mm a").toUpperCase(); + return format(date, "MMM dd, h:mm a"); };apps/dashboard/components/logs/chart/index.tsx (2)
34-39: Good: exported ChartMouseEvent and readonly payload.This matches prior guidance and prevents accidental mutation.
199-205: Good: safer tooltip mapping and formatter-driven display.Avoids unsafe casts and centralizes time format logic.
apps/dashboard/components/logs/utils.tsx (2)
64-83: Make 12‑hour granularity explicit for readability.Handle
"per12Hours"explicitly instead of relying on default branch.// For granularities less than 12 hours on same day, show only time if ( granularity && [ "perMinute", "per5Minutes", "per15Minutes", "per30Minutes", "perHour", "per2Hours", "per4Hours", "per6Hours", ].includes(granularity) ) { return format(date, "h:mm a"); } + if (granularity === "per12Hours") { + return format(date, "MMM dd, h:mm a"); + } + // For granularities 12 hours or more, show date + time return format(date, "MMM dd, h:mm a");
126-136: Validate payload timestamp before map lookup.If
parseTimestampreturns NaN, the map lookup and laternew Date(...)paths become unsafe.// Normalize timestamps to numeric for robust comparison - const currentTimestampNumeric = parseTimestamp(payloadTimestamp); + const currentTimestampNumeric = parseTimestamp(payloadTimestamp); + if (!Number.isFinite(currentTimestampNumeric)) { + return ""; + }apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
32-40: Type the handler’s granularity param with the shared union.Avoids coupling to
typeof verificationGranularityand tightens the API.const handleSelectionChange = ({ start, end, granularity, }: { start: number; end: number; - granularity?: typeof verificationGranularity; + granularity?: import("@/lib/trpc/routers/utils/granularity").CompoundTimeseriesGranularity; }) => {apps/dashboard/components/logs/overview-charts/utils.tsx (1)
26-26: Standardize date-fns tokens (“h:mm a”, “MMM yyyy”).Use a space before “a” and 4‑digit years for readability/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");Also applies to: 31-31, 35-35, 41-41, 49-49, 51-51
📜 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(3 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(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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
📚 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 (19)
apps/dashboard/components/ui/chart.tsx (1)
167-167: ARIA: Using role="tooltip" here is correct.Good fix; removing aria-live (per earlier note) aligns with the tooltip pattern.
apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (3)
164-167: LGTM: Verifications 7–<30d maps to perDay.
169-172: LGTM: 59d probe stays in per3Days bucket.
192-199: LGTM: 7d boundary → perDay.apps/dashboard/components/logs/parseTimestamp.ts (1)
28-31: Confirm 0-as-sentinel behavior.Returning 0 maps to 1970-01-01 and can leak into charts if unchecked. Verify downstream callers treat 0 as “invalid” and never render it.
Also applies to: 47-50
apps/dashboard/components/logs/chart/index.tsx (3)
141-142: Good: stable keys with getTime().Prevents key thrash on renders.
21-26: LGTM: selection state uses undefined instead of empty strings.Removes string-number coercions later.
50-51: LGTM: optional granularity is threaded into the chart.Keeps component flexible without forcing callers.
Also applies to: 62-63
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (2)
28-31: Verify [start, end) semantics and units.Confirm TRPC expects epoch milliseconds and that endTime is exclusive; otherwise the added buffer may double-count or miss the last bin.
70-71: LGTM: passing granularity through to the chart.Aligns selection buffer and tooltip intervals with server granularity.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
105-105: Granularity correctly propagated to chart.Passing
granularityfixes tooltip interval and selection buffering alignment for ratelimit charts.apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
67-67: Granularity forwarded to OverviewBarChart — good.Keeps tooltip/selection consistent with the fetched timeseries granularity.
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (2)
72-85: O(1) timestamp→index map — nice perf win.Precomputing
timestampToIndexMapremoves per-hover O(n) scans.
299-307: Centralized tooltip label formatting — good reuse.Using
formatTooltipIntervalkeeps both area/bar charts in sync and fixes timezone/DST issues.apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
96-101: Per-chart selection buffering — correct.Each chart now buffers using its own granularity, fixing cross-chart mismatches.
Also applies to: 120-125
apps/dashboard/components/logs/overview-charts/utils.tsx (1)
55-66: TZ helper and DST-aware display look good.Timestamp-aware abbreviation and dual‑TZ interval label correctly handle DST crossings.
Also applies to: 96-105, 156-165, 166-174
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (3)
71-81: Good: O(1) tooltip index via precomputed Map with normalized timestamps.This removes per-hover scans and normalizes types using parseTimestamp.
287-296: LGTM: central tooltip formatter usage.Delegating to formatTooltipInterval with precomputed index map ensures consistency with bar chart and handles DST.
46-47: Type alias for Granularity matches canonical union.Reduces drift; good change.
apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx
Outdated
Show resolved
Hide resolved
- Coerce string inputs to numbers before checking isUnixMicro to prevent Invalid Date errors - Unify micro/millisecond timestamp handling with explicit branching logic - Add fallback path for non-numeric strings to maintain backward compatibility - Fixes tooltips displaying "Invalid Date" for numeric string timestamps
Coerce originalTimestamp values to numeric epoch milliseconds before passing to calculateTimePoints in overview-area-chart.tsx to prevent type ambiguity. Use existing parseTimestamp utility with proper undefined handling and Date.now() fallbacks. Fixes potential runtime errors when data contains string or Date timestamps instead of numbers.
…l matching The findIndex comparison in createTimeIntervalFormatter was using strict equality on raw timestamps which could be string/number/Date types, potentially missing matches. Now both currentTimestamp and item.originalTimestamp are normalized to numeric epoch values using parseTimestamp() before comparison, with NaN-safe checks to ensure reliable interval finding. This prevents issues where timestamps of different types (e.g., "1640995200000" vs 1640995200000) would fail to match despite representing the same time.
Removed the unreachable conditional logic**: The ternary operator `currentTimestamp && currentDate ? timezoneAbbr : getTimezoneAbbreviation()` was checking conditions that are guaranteed to be truthy at this point in the code flow. 2. **Simplified to direct assignment**: Now the code directly uses the precomputed `timezoneAbbr` value that was already calculated on line 94. 3. **Eliminated dead code**: The fallback call to `getTimezoneAbbreviation()` was unreachable and has been removed.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
…g-at-different-date-times
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (17)
apps/dashboard/components/ui/chart.tsx (2)
235-239: Zero values are hidden in tooltip.The truthy check suppresses 0; show zeros.
- <div className="ml-auto"> - {item.value && ( + <div className="ml-auto"> + {item.value !== undefined && item.value !== null && ( <span className="font-mono tabular-nums text-accent-12"> {formatNumber(item.value as number)} </span> )} </div>
112-127: Forwardid(and other div props) to tooltip and legend containers
ChartTooltipContent and ChartLegendContent declareReact.ComponentProps<"div">but only extract specific props, droppingid,data-testid,style, etc. Destructure...props(or at leastid) in each component’s signature and spread ({...props}) onto the root<div>so callers can attacharia-describedby/test hooks. Ensure upstream Recharts bindsaria-describedbyto thisid.apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
107-128: Type mismatch: Invalid granularities for Regular contextThe Regular context is returning
perMonth,perWeek, andper3Daysgranularities which are not part of theRegularTimeseriesGranularitytype (lines 14-22). This will cause runtime type errors.The
RegularTimeseriesGranularitytype only includes:
perMinute,per5Minutes,per15Minutes,per30MinutesperHour,per2Hours,per4Hours,per6HoursBut does not include
perMonth,perWeek,per3Days, orperDaythat are being returned here.Apply this diff to fix the type mismatch:
- if (timeRange >= QUARTER_IN_MS) { - granularity = "perMonth"; - } else if (timeRange >= MONTH_IN_MS * 2) { - granularity = "perWeek"; - } else if (timeRange >= MONTH_IN_MS) { - granularity = "per3Days"; - } else if (timeRange >= WEEK_IN_MS) { - granularity = "perDay"; + if (timeRange >= WEEK_IN_MS * 2) { + granularity = "per6Hours"; + } else if (timeRange >= WEEK_IN_MS) { + granularity = "per6Hours"; } else if (timeRange >= DAY_IN_MS * 3) { - granularity = "perHour"; + granularity = "per4Hours"; } else if (timeRange >= DAY_IN_MS) { - granularity = "perHour"; + granularity = "per2Hours"; } else if (timeRange >= HOUR_IN_MS * 16) { granularity = "perHour";Alternatively, if these granularities are intentional for the Regular context, update the
RegularTimeseriesGranularitytype to include them.
93-99: Redundant conditional branchesLines 93-99 have three consecutive conditions that all set
granularity = "perHour". This is redundant and can be simplified.Apply this diff to simplify the logic:
} 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) { + } else if (timeRange >= HOUR_IN_MS * 16) { granularity = "perHour";apps/dashboard/lib/trpc/routers/utils/granularity.test.ts (2)
117-136: Inconsistent test expectations for Regular contextThe test expectations don't match the actual implementation. For example:
- Line 119: expects
per2Hoursfor 16-24 hours, but the implementation would returnperHour- Line 124: expects
per4Hoursfor 24 hours-3 days, but implementation would returnperHour- Line 129: expects
per6Hoursfor 3-7 days, but implementation would returnperHour- Line 134: expects
perDayfor >= 7 days, but implementation would returnperDay(this one is correct but only because of the type issue mentioned earlier)These test failures indicate that either:
- The implementation in
granularity.tsis incorrect, OR- The test expectations need to be updated
Please verify the intended behavior and update either the implementation or the tests accordingly.
269-271: Test expectation doesn't match implementationThe test expects
per4Hoursfor a 24-hour view, but based on the implementation ingranularity.ts, it would returnperHourfor this time range.Update the test expectation to match the implementation:
- expect(result.granularity).toBe("per4Hours"); + expect(result.granularity).toBe("perHour");apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (2)
17-47: Normalize reversed selections; keep zero-length click behavior.If users drag right-to-left, start can be greater than end. Normalize before applying the granularity buffer and writing filters.
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 && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + const [s, e] = start <= end ? [start, end] : [end, start]; + let adjustedEnd = e; + if (s === e && granularity) { + adjustedEnd = e + getTimeBufferForGranularity(granularity); + } updateFilters([ ...activeFilters, { field: "startTime", - value: start, + value: s, id: crypto.randomUUID(), operator: "is", }, { field: "endTime", value: adjustedEnd, id: crypto.randomUUID(), operator: "is", }, ]); };
24-47: DRY the selection→filters logic.This block is duplicated across charts. Extract a small helper (e.g., applySelectionToFilters(filters, {start,end}, granularity)) to centralize behavior.
I can sketch the helper and call sites if helpful.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (3)
27-58: Normalize reversed selections; keep zero-length click behavior.Mirror the selection normalization used in other charts.
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 && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + const [s, e] = start <= end ? [start, end] : [end, start]; + let adjustedEnd = e; + if (s === e && granularity) { + adjustedEnd = e + getTimeBufferForGranularity(granularity); + } updateFilters([ ...activeFilters, { field: "startTime", - value: start, + value: s, id: crypto.randomUUID(), operator: "is", }, { field: "endTime", value: adjustedEnd, id: crypto.randomUUID(), operator: "is", }, ]); };
106-115: Use a consistent semantic token for “Blocked” color.Other charts use --warning-9; keep palette semantics consistent.
error: { label: "Blocked", - color: "hsl(var(--orange-9))", + color: "hsl(var(--warning-9))", },
60-95: Trim long commented blocks.Convert to a TODO with a ticket reference or remove to reduce noise.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
25-55: Normalize reversed selections; keep zero-length click behavior.Same selection normalization as other charts.
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 && granularity) { - adjustedEnd = end + getTimeBufferForGranularity(granularity); - } + const [s, e] = start <= end ? [start, end] : [end, start]; + let adjustedEnd = e; + if (s === e && granularity) { + adjustedEnd = e + getTimeBufferForGranularity(granularity); + } updateFilters([ ...activeFilters, { field: "startTime", - value: start, + value: s, id: crypto.randomUUID(), operator: "is", }, { field: "endTime", value: adjustedEnd, id: crypto.randomUUID(), operator: "is", }, ]); };apps/dashboard/components/logs/chart/index.tsx (1)
91-102: Bug: endTimestamp never updates during drag (selection collapses to a point)handleMouseMove updates startTimestamp instead of endTimestamp, so the final range is {startTimestamp===endTimestamp}. Fix to update endTimestamp.
- const handleMouseMove = (e: ChartMouseEvent) => { + 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, })); } };apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (2)
115-126: Normalize selection timestamps to numbers before emitting.
startTimestamp/endTimestampcan be string/Date. Coerce withparseTimestampto satisfy the numeric contract and avoid downstream bugs.- const [start, end] = [selection.startTimestamp, selection.endTimestamp].sort((a, b) => a - b); - - onSelectionChange({ start, end }); + const startNum = parseTimestamp(selection.startTimestamp as any); + const endNum = parseTimestamp(selection.endTimestamp as any); + if (Number.isNaN(startNum) || Number.isNaN(endNum)) return; + const [start, end] = [startNum, endNum].sort((a, b) => a - b); + onSelectionChange({ start, end });
310-319: Normalize axis endpoints passed to calculateTimePoints.Align with the area chart and avoid type ambiguity by parsing to epoch ms.
- ? calculateTimePoints( - data[0]?.originalTimestamp ?? Date.now(), - data.at(-1)?.originalTimestamp ?? Date.now(), - ).map((time, i) => ( + ? calculateTimePoints( + data[0]?.originalTimestamp ? parseTimestamp(data[0].originalTimestamp) : Date.now(), + data.at(-1)?.originalTimestamp ? parseTimestamp(data.at(-1)!.originalTimestamp) : Date.now(), + ).map((time, i) => (apps/dashboard/components/logs/overview-charts/utils.tsx (1)
176-176: ExportformatTooltipIntervalhelper
- overview-bar-chart.tsx (L274) and overview-area-chart.tsx (L266) call
formatTooltipInterval, but it isn’t exported from apps/dashboard/components/logs/overview-charts/utils.tsx; add the helper implementation and export in that file.apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
115-125: Normalize selection timestamps before emitting.Ensure numeric epochs are sent to
onSelectionChange.- const [start, end] = [selection.startTimestamp, selection.endTimestamp].sort((a, b) => a - b); - onSelectionChange({ start, end }); + const startNum = parseTimestamp(selection.startTimestamp as any); + const endNum = parseTimestamp(selection.endTimestamp as any); + if (Number.isNaN(startNum) || Number.isNaN(endNum)) return; + const [start, end] = [startNum, endNum].sort((a, b) => a - b); + onSelectionChange({ start, end });
♻️ Duplicate comments (11)
apps/dashboard/components/ui/chart.tsx (1)
167-171: Cap tooltip width for narrow containers (keep role=tooltip).Nice removal of aria-live earlier. Add a base max-width to prevent overflow in narrow/embedded layouts while preserving md cap.
- className={cn( - "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", - className, - )} + className={cn( + "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", + className, + )}apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
4-7: Keep consistent formatting - avoid uppercasing timestampsThe
toUpperCase()transformation can harm internationalization and accessibility. Consider removing it or applying uppercase styling via CSS (text-transform: uppercase) if the design requires it.Apply this diff to remove the uppercase transformation:
export const formatTimestampLabel = (timestamp: string | number | Date) => { const date = new Date(timestamp); - return format(date, "MMM dd, h:mm a").toUpperCase(); + return format(date, "MMM dd, h:mm a"); };apps/dashboard/components/logs/parseTimestamp.ts (2)
32-35: Make the Date example timezone-stable (UTC).Use UTC construction to avoid locale-dependent millis in docs.
- * // 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 and allow exponents.Avoid parseFloat’s permissiveness (e.g., "123abc" → 123). Validate first, then Number().
if (typeof timestamp === "string") { const trimmed = timestamp.trim(); if (!trimmed) { return 0; } - timestampNum = Number.parseFloat(trimmed); + if (!/^[+-]?\d+(\.\d+)?([eE][+-]?\d+)?$/.test(trimmed)) { + return 0; + } + timestampNum = Number(trimmed);apps/dashboard/components/logs/chart/index.tsx (2)
34-39: Good: exported, readonly event payload typeExporting ChartMouseEvent and using ReadonlyArray prevents accidental mutation and enables reuse across charts.
199-205: Good: safe payload mapping and formatter-driven displayReplacing the unsafe cast with explicit mapping and delegating format selection to the formatter is the right move.
apps/dashboard/components/logs/utils.tsx (1)
127-137: Guard NaN before lookups/formatting to avoid runtime errorsIf parseTimestamp returns NaN/Infinity, Map lookups and Intl/date-fns can throw. Early-return safely.
// Normalize timestamps to numeric for robust comparison - const currentTimestampNumeric = parseTimestamp(payloadTimestamp); + const currentTimestampNumeric = parseTimestamp(payloadTimestamp); + if (!Number.isFinite(currentTimestampNumeric)) { + return ""; + }apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
35-40: Type granularity explicitly; avoid typeof couplingUsing typeof verificationGranularity ties the type to a local variable. Use the shared union type.
+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)
143-147: Include timezone and consistent classes in the next‑point missing fallback.Current branch omits TZ and uses different padding.
- 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> + );
21-49: Standardize time format tokens (space before am/pm; 4‑digit year).Improves readability and consistency; matches the rest of the PR defaults.
- 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");apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (1)
46-47: Granularity alias looks good.Using
CompoundTimeseriesGranularity | undefinedkeeps a single source of truth.
📜 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(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)
🧰 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]/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
📚 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 (10)
apps/dashboard/components/logs/chart/utils/format-timestamp.ts (1)
53-68: Good robust parsing implementation!The numeric string coercion and fallback logic properly handles edge cases to prevent Invalid Date errors. The unified micro/milli timestamp handling is well-implemented.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/charts/index.tsx (1)
70-71: LGTM: granularity is correctly forwarded to the chart.This unblocks tooltip interval formatting and selection buffering.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/charts/index.tsx (1)
105-105: LGTM: granularity is correctly forwarded to OverviewBarChart.Aligns with the new chart API.
apps/dashboard/app/(app)/apis/[apiId]/keys/[keyAuthId]/[keyId]/components/charts/index.tsx (1)
67-67: LGTM: granularity is correctly forwarded.Matches OverviewBarChart’s new API.
apps/dashboard/app/(app)/apis/[apiId]/_overview/components/charts/index.tsx (1)
96-101: Good: per-chart granularity passed into selection handlerSelections from each chart now buffer using that chart’s granularity. This resolves the earlier cross-chart granularity mix-up.
Also applies to: 120-125
apps/dashboard/components/logs/overview-charts/overview-bar-chart.tsx (1)
6-6:formatTooltipIntervalis correctly exported. It’s exported fromapps/dashboard/components/logs/utils.tsx(line 95), so the import inoverview-bar-chart.tsxwill resolve without error.Likely an incorrect or invalid review comment.
apps/dashboard/components/logs/overview-charts/utils.tsx (2)
74-78: Default to 12‑hour format (matches product intent).The factory’s default
timeFormat = "h:mm a"looks good.
6-8: Shared fallback constant looks good.
DEFAULT_TIME_BUFFER_MS = 60_000is clear and centralized.apps/dashboard/components/logs/overview-charts/overview-area-chart.tsx (2)
303-319: Nice: endpoints normalized for calculateTimePoints.Passing parsed epochs avoids downstream ambiguity.
264-272: Approve labelFormatter integration
formatTooltipInterval is exported in apps/dashboard/components/logs/utils.tsx (line 95) and correctly imported in overview-area-chart.tsx, so no changes are needed.
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
| // Parse timestamp to number with robust validation | ||
| let timestampNum: number; | ||
| if (typeof timestamp === "string") { | ||
| const trimmed = timestamp.trim(); | ||
| if (!trimmed) { | ||
| return 0; | ||
| } | ||
| timestampNum = Number.parseFloat(trimmed); | ||
| } else if (timestamp instanceof Date) { | ||
| timestampNum = timestamp.getTime(); | ||
| } else { | ||
| timestampNum = timestamp; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Preserve precision for 16–19 digit integer strings with BigInt (optional but recommended).
For µs/ns provided as strings, Number loses precision. Use BigInt on integer-only strings, then downscale to ms.
Example snippet for the string branch (insert before Number(...) assignment):
// If it's an integer-only string and large, use BigInt for exact scaling.
const intOnly = /^[+-]?\d+$/.test(trimmed);
if (intOnly && trimmed.replace(/^[+-]?/, "").length >= 16) {
const negative = trimmed.startsWith("-");
const absStr = trimmed.replace(/^[+-]?/, "");
const abs = BigInt(absStr);
let msBig: bigint;
if (abs >= 1_000_000_000_000_000_000n) {
// ns → ms
msBig = abs / 1_000_000n;
} else if (abs >= 1_000_000_000_000_000n) {
// µs → ms
msBig = abs / 1_000n;
} else if (abs >= 1_000_000_000_000n) {
// ms
msBig = abs;
} else {
// s → ms
msBig = abs * 1_000n;
}
return (negative ? -1 : 1) * Number(msBig);
}🤖 Prompt for AI Agents
In apps/dashboard/components/logs/parseTimestamp.ts around lines 42 to 55, the
string branch currently uses Number.parseFloat which loses precision for 16–19
digit integer-only timestamps (µs/ns); detect integer-only trimmed strings and
when length >= 16 parse with BigInt, determine sign, convert absolute value to
milliseconds by dividing or multiplying according to magnitude (ns → divide by
1_000_000n, µs → divide by 1_000n, ms → leave, s → multiply by 1_000n), then
return the signed Number of that bigint result; otherwise fall back to
Number.parseFloat for non-integer or shorter strings and keep the existing
empty-trim handling.
| } else if (timeRange >= DAY_IN_MS * 3) { | ||
| granularity = "per6Hours"; | ||
| } else if (timeRange >= HOUR_IN_MS * 24) { | ||
| granularity = "per4Hours"; | ||
| granularity = "perHour"; | ||
| } else if (timeRange >= DAY_IN_MS) { | ||
| granularity = "perHour"; | ||
| } else if (timeRange >= HOUR_IN_MS * 16) { | ||
| granularity = "per2Hours"; | ||
| } else if (timeRange >= HOUR_IN_MS * 12) { | ||
| granularity = "per30Minutes"; | ||
| granularity = "perHour"; | ||
| } else if (timeRange >= HOUR_IN_MS * 8) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redundant conditional branches in Regular context
Same issue as above - three consecutive conditions that all set granularity = "perHour".
Apply this diff to simplify:
} 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) {
+ } else if (timeRange >= HOUR_IN_MS * 16) {
granularity = "perHour";📝 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.
| } else if (timeRange >= DAY_IN_MS * 3) { | |
| granularity = "per6Hours"; | |
| } else if (timeRange >= HOUR_IN_MS * 24) { | |
| granularity = "per4Hours"; | |
| granularity = "perHour"; | |
| } else if (timeRange >= DAY_IN_MS) { | |
| granularity = "perHour"; | |
| } else if (timeRange >= HOUR_IN_MS * 16) { | |
| granularity = "per2Hours"; | |
| } else if (timeRange >= HOUR_IN_MS * 12) { | |
| granularity = "per30Minutes"; | |
| granularity = "perHour"; | |
| } else if (timeRange >= HOUR_IN_MS * 8) { | |
| } else if (timeRange >= WEEK_IN_MS) { | |
| granularity = "perDay"; | |
| } else if (timeRange >= HOUR_IN_MS * 16) { | |
| granularity = "perHour"; | |
| } else if (timeRange >= HOUR_IN_MS * 8) { |
🤖 Prompt for AI Agents
In apps/dashboard/lib/trpc/routers/utils/granularity.ts around lines 115 to 121,
three consecutive else-if branches all set granularity = "perHour" (timeRange >=
DAY_IN_MS * 3, >= DAY_IN_MS, >= HOUR_IN_MS * 16); collapse these redundant
branches into a single condition using the smallest threshold that covers all
three (i.e., timeRange >= HOUR_IN_MS * 16) and remove the two redundant else-if
branches so only one branch assigns "perHour".
…mance - Add bounded Map cache (1000 entries) with LRU eviction to prevent memory bloat - Create memoizedFormat() helper that caches date-fns format() results - Replace all format() calls with memoized version across all formatting functions - Use timestamp + format string as cache key for reliable deduplication - Optimize chart re-renders by eliminating redundant date formatting operations This reduces expensive date-fns format() calls during frequent chart updates and re-renders, particularly beneficial for time-series data visualization.
Previously, the timestampToIndexMap would add entries even when parseTimestamp returned NaN or other invalid values. This could lead to incorrect lookups during hover/tooltip operations. Now validates that parseTimestamp returns a finite number before adding entries to the map, ensuring only valid numeric timestamps are stored for O(1) lookups.
Prevent Map pollution in overview bar chart by skipping NaN/non-finite timestamps from parseTimestamp. Only valid numeric timestamps are now added to timestampToIndexMap, ensuring reliable O(1) lookups and preventing potential runtime errors. - Add Number.isFinite check before map.set() in timestampToIndexMap - Skip invalid normalizedTimestamp values to prevent missed lookups - Maintains performance while improving data integrity
…resholds in parseTimestamp Replace string-based digit counting with direct numeric comparisons for timestamp unit detection. Use absoluteTimestamp >= 1e18 for nanoseconds, >= 1e15 for microseconds, >= 1e12 for milliseconds, otherwise seconds. Preserves existing scaling and Math.floor behavior while eliminating fragile toString() logic.
…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).
| type Selection = { | ||
| start: string | number; | ||
| end: string | number; | ||
| start: string | number | undefined; | ||
| end: string | number | undefined; | ||
| startTimestamp?: number; | ||
| endTimestamp?: number; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Narrow Selection types to numbers to avoid coercions.
Store numeric x values and drop string union; fewer Number() casts downstream.
type Selection = {
- start: string | number | undefined;
- end: string | number | undefined;
+ start: number | undefined;
+ end: number | undefined;
startTimestamp?: number;
endTimestamp?: number;
};And coerce on mouse down:
- setSelection({
- start: e.activeLabel,
- end: e.activeLabel,
+ const labelNum = Number(e.activeLabel);
+ setSelection({
+ start: labelNum,
+ end: labelNum,📝 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.
| type Selection = { | |
| start: string | number; | |
| end: string | number; | |
| start: string | number | undefined; | |
| end: string | number | undefined; | |
| startTimestamp?: number; | |
| endTimestamp?: number; | |
| }; | |
| // apps/dashboard/components/logs/chart/index.tsx | |
| // ─── Narrow Selection to numbers ─────────────────────────────────────────────── | |
| type Selection = { | |
| start: number | undefined; | |
| end: number | undefined; | |
| startTimestamp?: number; | |
| endTimestamp?: number; | |
| }; | |
| // ─── In your mouse-down (or similar) handler ──────────────────────────────────── | |
| const labelNum = Number(e.activeLabel); | |
| setSelection({ | |
| start: labelNum, | |
| end: labelNum, | |
| // …other selection fields unchanged | |
| }); |
🤖 Prompt for AI Agents
In apps/dashboard/components/logs/chart/index.tsx around lines 21 to 26, the
Selection type uses string | number | undefined which forces downstream
coercions; change start and end to number | undefined (keep
startTimestamp/endTimestamp as-is or remove if redundant), update any code paths
that set Selection (notably the mouse down handler) to coerce incoming x values
to numbers at the point of assignment (e.g., parse/Number on the event
coordinate) so all stored x values are numeric and downstream Number() casts can
be removed.
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
Summary by CodeRabbit
New Features
Style