client/src/components/ui/card.tsx (1)

28-28: LGTM - Good loader improvements.

The additions of overflow-hidden and pointer-events-none are appropriate:

• overflow-hidden prevents the large Zoomies animation (size 1400) from creating visual artifacts
• pointer-events-none ensures the loader doesn't intercept user interactions with the underlying content

The negative margin mt-[-15px] appears to be for visual alignment, though it's a magic number that might benefit from a comment explaining the offset purpose.

client/src/api/analytics/useGetOverviewBucketed.ts (1)

17-18: Client response type correctly extended for new/returning users

Adding new_users and returning_users here matches the server-side shape and enables typed access in charts; no issues from a typing or usage standpoint. You may later want to centralize this response type in a shared module to avoid divergence between client and server definitions, but that’s optional.

client/src/lib/store.ts (1)

20-21: Store flag wiring for showUsersSplit is consistent; minor duplication only

The new showUsersSplit state and setShowUsersSplit action are wired consistently across:

• Store type (Lines 20-21)
• setSite defaults (Line 59)
• Initial state (Lines 149-150)
• resetStore (Lines 156-161)

Semantics look good: toggling is per-site, and reset paths correctly clear the flag. The only nit is that setSite and the initial state both hardcode showUsersSplit: false, and resetStore also calls setShowUsersSplit(false), which is slightly redundant but harmless. If you want to reduce duplication, you could rely on either setSite or setShowUsersSplit(false) in resetStore, not both.

Also applies to: 59-60, 149-151, 156-161

server/src/api/analytics/getOverviewBucketed.ts (1)

177-187: Server-side response type matches client expectations; consider sharing via a common type

The getOverviewBucketed type alias (Lines 177-187) now includes new_users and returning_users, mirroring the SQL select list and the client’s GetOverviewBucketedResponse. The call to processResults<getOverviewBucketed[number]> (Lines 225-226) remains consistent with that alias.

To avoid drift between server and client, it may be worth lifting this shape into a shared @rybbit/shared type and importing it on both sides, but the current implementation is functionally correct.

Also applies to: 225-226

client/src/app/[site]/main/components/MainSection/MainSection.tsx (1)

84-111: User breakdown toggle + legend UX is solid; minor nit on duplicate control IDs

The toggle and legend integration read cleanly:

• Desktop: Switch + label shown only when selectedStat === "users" (Lines 101-109).
• Mobile: a separate Switch + label version for small screens (Lines 113-121).
• Both bind checked={showUsersSplit} and onCheckedChange={setShowUsersSplit}, ensuring state is shared.
• When showUserBreakdown is active, the legend below the header clearly labels “New users” vs “Returning users” with appropriate colors (Lines 122-133), aligning with the chart series.

One small nit: both desktop and mobile switches use the same id="toggle-user-breakdown". While functionally this still works, it means:

• There are duplicate IDs in the DOM.
• Each <label htmlFor="toggle-user-breakdown"> may associate with whichever control the browser picks first.

If you want to tighten this up, consider giving each switch a unique id and matching label, or rendering a single switch whose layout is controlled purely by responsive classes.

Also applies to: 113-121, 122-133

client/src/app/[site]/main/components/MainSection/Chart.tsx (4)

122-139: Series labels and config abstraction are clear; consider tightening SeriesConfig typing

The SERIES_LABELS + SeriesConfig pattern nicely centralizes series metadata and keeps the “new vs returning” additions localized. To make this even safer long‑term, you could narrow SeriesConfig["id"] (and possibly dataKey) to a union of known keys ("pageviews" | "sessions" | "users" | "new_users" | "returning_users" | …) instead of plain string. That would surface mismatches at compile time if future metrics are added or renamed.

---

196-255: Multi‑series data shaping and custom layers are sound; small opportunities to simplify and optimize

The pipeline from seriesConfig → seriesData → chartPropsData/defs/fill → StackedLines/DashedOverlay reads well and generalizes the previous single‑series logic to multiple series without obvious correctness issues. A couple of small tweaks you might consider:

• Cache DateTime.now() once per render instead of inside the .map when filtering out future timestamps to avoid tiny time skews and redundant calls:

-  const seriesData = seriesConfig.map(config => {
-    const points =
-      data?.data
-        ?.map((e, i) => {
-          // Parse timestamp properly
-          const timestamp = DateTime.fromSQL(e.time).toUTC();
-
-          // filter out dates from the future
-          if (timestamp > DateTime.now()) {
+  const now = DateTime.now();
+  const seriesData = seriesConfig.map(config => {
+    const points =
+      data?.data
+        ?.map((e, i) => {
+          // Parse timestamp properly
+          const timestamp = DateTime.fromSQL(e.time).toUTC();
+
+          // filter out dates from the future
+          if (timestamp > now) {
             return null;
           }

• If you ever see perf pressure here, seriesData and the derived chartPropsData/defs/fill arrays would be good candidates for useMemo keyed on (data?.data, previousData?.data, selectedStat, showUsersSplit) to avoid recomputing on every render.

Overall, the use of yScale.stacked = showUserBreakdown and the yStacked ?? y fallback in both custom layers matches the intended “stack only when showing new vs returning” behaviour.

Also applies to: 257-300, 318-318

---

366-427: Single‑series tooltip logic is consistent; minor leftover normalization can be simplified

The single‑series tooltip correctly pulls currentTime, previousTime, and Y values from the underlying point data and formats them using the existing helpers, so behaviour should match the old implementation for non‑split modes.

The only small nit is the normalization step:

const normalizedPoints = slice.points.map((point: any) => ({
  ...point,
  originalSerieId: String(point.serieId),
  serieId: String(point.serieId).replace(/-dashed$/, "-base"),
}));

Given that the actual series ids you feed into Nivo are already of the form ${series.id}-base and the dashed overlay is drawn as a separate SVG path (not as its own series), this -dashed → -base replacement no longer seems necessary. You could drop it and just use slice.points directly for a tiny simplification, unless you plan to reintroduce separate dashed series ids later.

---

429-511: Two‑series tooltip for new vs returning is well‑structured; optional extra guard for empty rows

The two‑series tooltip correctly uses the slice’s timestamp to look up matching points in seriesData for both new_users and returning_users, and presents current/previous values plus percentage deltas with appropriate colours per series. This looks aligned with the new toggle’s intent.

You might add a small defensive guard before rendering, in case rows ends up empty due to any future data inconsistencies, to avoid an empty tooltip shell:

-        const rows = seriesConfig
+        const rows = seriesConfig
           .map(series => {
             // ...
           })
           .filter(Boolean);
-
-        return (
+        if (!rows.length) return null;
+
+        return (
           <ChartTooltip>
             {rows.map((row: any, idx: number) => (
               {/* ... */}

Not required for current behaviour (your data pipeline should always yield at least one row), but it makes the tooltip more robust to edge cases.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro