Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
mockDailySportDataReturn,
emptyDailySportData,
} from "../../test/fixtures/sportConfig";
import type { MultiSportData } from "../../hooks/useDailySportData";

// Mock useDailySportData hook
vi.mock("../../hooks/useDailySportData", () => ({
Expand Down Expand Up @@ -697,4 +698,72 @@ describe("ActivityCalendarHeatmap", () => {
});
});
});

describe("malformed wire data", () => {
// protojson omits proto3 defaults unless EmitUnpopulated is set, so a
// DailyActivity whose `activities` is 0 can arrive with the field absent.
// Before the `?? 0` guard, that `undefined` poisoned the day's count to NaN,
// which propagated through totalActivities into a literal "NaN activities"
// header. SportBucketSchema doesn't descend into DailyActivity, so the dev
// contract-drift warning doesn't fire either.
it("treats a missing `activities` field as zero instead of rendering NaN", () => {
const poisoned = {
cycling: {
"2026-01-02": {
distanceMeters: 45000,
timeMinutes: 90,
elevationMeters: 500,
// `activities` deliberately absent — the wire shape under test.
activityIds: [1],
},
"2026-01-03": {
distanceMeters: 30000,
timeMinutes: 60,
elevationMeters: 300,
activities: 2,
activityIds: [2],
},
},
running: {},
yoga: {},
} as unknown as MultiSportData;

mockUseDailySportData.mockReturnValue(mockDailySportDataReturn({ data: poisoned }));

const { container } = render(<ActivityCalendarHeatmap />);

expect(container.textContent).not.toContain("NaN");
// The one well-formed day still counts; the absent field contributes 0.
expect(screen.getByText(/2 activities in/)).toBeInTheDocument();
});

// Characterisation, not regression: this passes with or without the `?? 0`
// guard, because the cell already floors its input with `|| 0` and
// `NaN || 0` is 0. It pins that invariant so the audit's "paints the
// brightest colour" claim can't become true if someone drops the `|| 0`.
it("does not paint a day with a missing count as the busiest colour", () => {
const poisoned = {
cycling: {
"2026-01-02": {
distanceMeters: 45000,
timeMinutes: 90,
elevationMeters: 500,
activityIds: [1],
},
},
running: {},
yoga: {},
} as unknown as MultiSportData;

mockUseDailySportData.mockReturnValue(mockDailySportDataReturn({ data: poisoned }));

render(<ActivityCalendarHeatmap />);

// Cell labels are the accessible surface for the count; a poisoned day
// must read as 0, not NaN, and so must take the dim bucket.
const cell = screen.getByRole("img", { name: /^2026-01-02:/ });
expect(cell).toHaveAttribute("aria-label", "2026-01-02: 0 activities");
expect(cell).toHaveStyle({ background: "var(--color-slate-light)" });
});
});
});
17 changes: 14 additions & 3 deletions packages/web/src/components/dashboard/ActivityCalendarHeatmap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,16 @@ const INTENSITY_COLORS = [
"var(--color-intensity-4)", // 6+ activities - neon magenta
] as const;

/** Get color for activity count */
/**
* Get color for activity count.
*
* Defence in depth: the sole caller already floors its input with `|| 0`, so a
* non-finite count cannot reach here today. The guard exists because the
* fallthrough returns the *brightest* bucket — if that `|| 0` were ever removed,
* a NaN would silently paint a day as the busiest of the year rather than fail.
*/
function getIntensityColor(count: number): string {
if (count === 0) return INTENSITY_COLORS[0];
if (!Number.isFinite(count) || count <= 0) return INTENSITY_COLORS[0];
if (count === 1) return INTENSITY_COLORS[1];
if (count <= 3) return INTENSITY_COLORS[2];
if (count <= 5) return INTENSITY_COLORS[3];
Expand Down Expand Up @@ -252,7 +259,11 @@ export default function ActivityCalendarHeatmap({
const sportData = data[sport];
if (sportData) {
Object.entries(sportData).forEach(([date, activity]) => {
counts[date] = (counts[date] || 0) + activity.activities;
// `?? 0`: protojson omits proto3 defaults unless EmitUnpopulated is set, so a
// DailyActivity with activities: 0 arrives with the field absent. Unguarded,
// `undefined` poisons this date to NaN, which then propagates through
// totalActivities into a "NaN activities" header.
counts[date] = (counts[date] || 0) + (activity.activities ?? 0);
});
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
import { render, screen, fireEvent } from "@testing-library/react";
import RecentActivitiesList from "./RecentActivitiesList";
import type { UseActivitiesResult } from "../../hooks/useActivities";
import type { ActivitySummary } from "../../api/activities";

vi.mock("@tanstack/react-router", () => ({ useNavigate: () => vi.fn() }));
vi.mock("../../hooks/useAuth", () => ({ useAuth: () => ({ user: { uid: "u1" } }) }));
vi.mock("../../hooks/useDashboardGoalData", () => ({
useDashboardGoalData: () => ({ sportData: [], distanceUnit: "kilometers" }),
}));
vi.mock("../../contexts/ThemeContext", () => ({ useTheme: () => ({ resolvedTheme: "dark" }) }));
vi.mock("../../hooks/useActivities", () => ({ useActivities: vi.fn() }));

import { useActivities } from "../../hooks/useActivities";
const mockUseActivities = vi.mocked(useActivities);

function activity(id: number): ActivitySummary {
return {
id: String(id),
name: `Activity ${id}`,
type: "Ride",
sport: "cycling",
startDateLocal: "2026-01-0" + ((id % 9) + 1) + "T08:00:00",
distanceMeters: 1000 * id,
movingTimeSeconds: 600,
elevationMeters: 10,
hasRoute: false,
};
}

describe("RecentActivitiesList pagination", () => {
beforeEach(() => vi.clearAllMocks());

// Regression: at the last *local* page with hasMore, handleNextPage calls the
// async loadMore() and optimistically does setPage(p + 1). On the re-render that
// setPage triggers, the fetch has not resolved, so activities.length — and thus
// totalPages — is unchanged, and the clamp
// clampedPage = Math.min(page, totalPages - 1)
// snapped page straight back. The user's first "Older" click at each server-page
// boundary was a no-op and a second click was required. It only reproduced when
// the next page wasn't already cached, i.e. the common case, which is why it
// survived. The clamp is now skipped while isLoadingMore is true.
it("advances on the first Older click at a server-page boundary", () => {
const firstPage = [activity(1), activity(2)];
let isLoadingMore = false;

mockUseActivities.mockImplementation((): UseActivitiesResult => ({
activities: firstPage,
isLoading: false,
error: null,
hasMore: true,
isLoadingMore,
// Mirrors fetchNextPage: the flag flips now, the data lands later.
loadMore: () => {
isLoadingMore = true;
},
retry: vi.fn(),
}));

render(<RecentActivitiesList timeRange="4weeks" pageSize={2} />);

// pageSize 2 with 2 activities => one local page, so page 0 is the boundary.
expect(screen.getByText("1/+")).toBeInTheDocument();

fireEvent.click(screen.getByLabelText("Older activities"));

// Before the fix this read "1/+" again — the click was swallowed.
expect(screen.getByText("2/+")).toBeInTheDocument();
});

it("still clamps an out-of-range page when nothing is being fetched", () => {
// The clamp's original purpose: a resize shrinks pageSize-derived totalPages
// and the current page falls out of range. With no fetch in flight it must
// still pull the page back rather than render an empty slice.
mockUseActivities.mockImplementation((): UseActivitiesResult => ({
activities: [activity(1)],
isLoading: false,
error: null,
hasMore: true,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
}));

const { rerender } = render(<RecentActivitiesList timeRange="4weeks" pageSize={1} />);
fireEvent.click(screen.getByLabelText("Older activities"));

// hasMore is true but loadMore never resolves and isLoadingMore stays false,
// so the advance is not protected and the clamp reels it back in.
rerender(<RecentActivitiesList timeRange="4weeks" pageSize={1} />);
expect(screen.getByText("1/+")).toBeInTheDocument();
});
});
12 changes: 9 additions & 3 deletions packages/web/src/components/dashboard/RecentActivitiesList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,18 @@ export default function RecentActivitiesList({
// Memoized: useActivities' demo path filters on the filter object's identity,
// so an inline literal would re-filter (and re-render) every render signed out.
const filter = useMemo(() => ({ from, to, sports: [], limit: 20 }), [from, to]);
const { activities, isLoading, error, hasMore, loadMore } = useActivities(filter);
const { activities, isLoading, error, hasMore, isLoadingMore, loadMore } = useActivities(filter);

const totalPages = Math.ceil(activities.length / pageSize);
// Clamp page if pageSize changed (e.g. container resized) and current page is now out of range
// Clamp page if pageSize changed (e.g. container resized) and current page is now out of range.
//
// Skip the clamp while a next-page fetch is in flight. handleNextPage advances
// past the last *local* page and lets loadMore() fill it in, but activities.length
// has not grown yet on that render — so clamping here would snap the page back
// before the data lands and silently eat the user's click. It only reproduces
// when the next server page isn't already cached, i.e. the common case.
const clampedPage = Math.min(page, Math.max(0, totalPages - 1));
if (clampedPage !== page) setPage(clampedPage);
if (clampedPage !== page && !isLoadingMore) setPage(clampedPage);
const startIdx = clampedPage * pageSize;
const visibleActivities = activities.slice(startIdx, startIdx + pageSize);

Expand Down
7 changes: 7 additions & 0 deletions packages/web/src/hooks/useActivities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export interface UseActivitiesResult {
isLoading: boolean;
error: Error | null;
hasMore: boolean;
/**
* A next-page fetch is in flight. Distinct from `isLoading`, which covers only
* the initial load — consumers that optimistically advance a page on loadMore()
* need to know the difference so they don't undo the advance mid-fetch.
*/
isLoadingMore: boolean;
loadMore: () => void;
retry: () => void;
}
Expand Down Expand Up @@ -123,6 +129,7 @@ export function useActivities(filter: Omit<ActivityListFilter, "cursor">): UseAc
isLoading: authLoading || (!!user && isFetching && !isFetchingNextPage && !data),
error: error,
hasMore: !user ? false : !!hasNextPage,
isLoadingMore: isFetchingNextPage,
loadMore: () => {
void fetchNextPage();
},
Expand Down
20 changes: 14 additions & 6 deletions packages/web/src/hooks/useTrainingMomentum.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@ import { describe, it, expect } from "vitest";
import { renderHook } from "@testing-library/react";
import { useTrainingMomentum } from "./useTrainingMomentum";
import type { DistanceEntry } from "../types/activity";
// toLocalDateString, not toISOString(): these fixtures mean "N *local* calendar
// days ago", but toISOString() formats the UTC date. In a UTC-negative timezone
// an evening test run pushed the string to the next UTC day, so a fixture built
// as "8 days ago" was really 7 local days back. That made the staleness
// assertions below pass only because isActivityDataStale had a matching
// UTC-vs-local skew; with the clock anchored correctly the mismatch surfaced as
// a time-of-day-dependent failure. Keep both sides on the local convention.
import { toLocalDateString } from "../utils/dateUtils";

describe("useTrainingMomentum", () => {
const createDistanceData = (entries: Array<{ x: string; y: number }>): DistanceEntry[] => {
Expand All @@ -15,7 +23,7 @@ describe("useTrainingMomentum", () => {
for (let i = 9; i >= 0; i--) {
const date = new Date(today);
date.setDate(date.getDate() - i);
data.push({ x: date.toISOString().split("T")[0]!, y: 100 + (9 - i) ** 2 });
data.push({ x: toLocalDateString(date), y: 100 + (9 - i) ** 2 });
}

const { result } = renderHook(() => useTrainingMomentum(data, 10));
Expand All @@ -35,7 +43,7 @@ describe("useTrainingMomentum", () => {
for (let i = 9; i >= 0; i--) {
const date = new Date(today);
date.setDate(date.getDate() - i);
data.push({ x: date.toISOString().split("T")[0]!, y: distance });
data.push({ x: toLocalDateString(date), y: distance });
distance += increments[9 - i]!;
}

Expand All @@ -51,14 +59,14 @@ describe("useTrainingMomentum", () => {
for (let i = 15; i >= 8; i--) {
const date = new Date();
date.setDate(date.getDate() - i);
data.push({ x: date.toISOString().split("T")[0]!, y: 100 + (15 - i) * 10 });
data.push({ x: toLocalDateString(date), y: 100 + (15 - i) * 10 });
}
// Add extended data (flat-line) for the last 8 days
const lastDistance = data.at(-1)!.y;
for (let i = 7; i >= 0; i--) {
const date = new Date();
date.setDate(date.getDate() - i);
data.push({ x: date.toISOString().split("T")[0]!, y: lastDistance });
data.push({ x: toLocalDateString(date), y: lastDistance });
}

const { result } = renderHook(() => useTrainingMomentum(data, 10));
Expand All @@ -74,7 +82,7 @@ describe("useTrainingMomentum", () => {
for (let i = 7; i >= 0; i--) {
const date = new Date(today);
date.setDate(date.getDate() - i);
data.push({ x: date.toISOString().split("T")[0]!, y: 100 + (7 - i) * 10 });
data.push({ x: toLocalDateString(date), y: 100 + (7 - i) * 10 });
}

const { result } = renderHook(() => useTrainingMomentum(data, 10));
Expand All @@ -92,7 +100,7 @@ describe("useTrainingMomentum", () => {
dates.forEach((daysAgo, idx) => {
const date = new Date(today);
date.setDate(date.getDate() - daysAgo);
data.push({ x: date.toISOString().split("T")[0]!, y: values[idx]! });
data.push({ x: toLocalDateString(date), y: values[idx]! });
});

const { result } = renderHook(() => useTrainingMomentum(data, 10));
Expand Down
5 changes: 5 additions & 0 deletions packages/web/src/pages/ActivitiesPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ describe("ActivitiesPage", () => {
isLoading: false,
error: null,
hasMore: false,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
});
Expand All @@ -98,6 +99,7 @@ describe("ActivitiesPage", () => {
isLoading: false,
error: null,
hasMore: false,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
});
Expand All @@ -116,6 +118,7 @@ describe("ActivitiesPage", () => {
isLoading: false,
error: null,
hasMore: false,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
});
Expand Down Expand Up @@ -238,6 +241,7 @@ describe("ActivitiesPage", () => {
isLoading: true,
error: null,
hasMore: false,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
});
Expand All @@ -256,6 +260,7 @@ describe("ActivitiesPage", () => {
isLoading: false,
error: null,
hasMore: false,
isLoadingMore: false,
loadMore: vi.fn(),
retry: vi.fn(),
});
Expand Down
Loading
Loading