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
38 changes: 37 additions & 1 deletion packages/web/src/api/activities.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
import { fetchActivities, fetchActivitySummary } from "./activities";
import {
fetchActivities,
fetchActivitySummary,
fetchMultiSportDailySummary,
fetchMultiSportMetrics,
} from "./activities";

// Stub the axios client so the tests see exactly the URL fetchActivities builds.
const { get } = vi.hoisted(() => ({ get: vi.fn() }));
Expand Down Expand Up @@ -69,3 +74,34 @@ describe("fetchActivitySummary query serialization", () => {
expect(await fetchActivitySummary({ sports: [] })).toEqual([]);
});
});

describe("multi-sport fetchers with an empty sports selection", () => {
beforeEach(() => {
get.mockReset();
get.mockResolvedValue({ data: {} });
});

// Regression: both fetchers built params as
// new URLSearchParams({ sports: options.sports.join(",") })
// and `[].join(",")` is "", which URLSearchParams still emits as `?sports=`.
// The handler rejects a blank value with 400, so hiding every sport in
// Settings and then clicking the dashboard heatmap's "Visible" chip produced
// a generic "Failed to load calendar data" error where an empty state belonged.
it.each([
["fetchMultiSportDailySummary", fetchMultiSportDailySummary],
["fetchMultiSportMetrics", fetchMultiSportMetrics],
])("%s makes no request and resolves empty", async (_name, fn) => {
const result = await fn({ year: 2026, sports: [] });

expect(get).not.toHaveBeenCalled();
expect(result).toEqual({});
});

it("still sends sports when the selection is non-empty", async () => {
get.mockResolvedValue({ data: { bySport: {} } });
await fetchMultiSportDailySummary({ year: 2026, sports: ["cycling"] });

const params = new URLSearchParams(requestedUrl().split("?")[1]);
expect(params.get("sports")).toBe("cycling");
});
});
45 changes: 31 additions & 14 deletions packages/web/src/api/activities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,20 +217,42 @@ export interface FetchMultiSportOptions {
}

/**
* Fetch daily summaries for multiple sports in a single request.
* Returns a map of sport → daily data (Record<string, DailyActivity>).
* Build the shared query string for the multi-sport endpoints.
*
* Extracted so both callers inherit the same empty-array contract: `[].join(",")`
* is `""`, and URLSearchParams still emits the key, so `?sports=` reaches a
* backend that rejects a blank value with 400. The single-sport fetchers already
* follow the omit-if-empty discipline; this brings the batch ones in line.
*
* Callers must short-circuit on an empty `sports` array rather than rely on this —
* an empty selection has no meaningful request to make.
*/
export const fetchMultiSportDailySummary = async (
options: FetchMultiSportOptions
): Promise<Record<string, Record<string, DailyActivity>>> => {
const params = new URLSearchParams({ sports: options.sports.join(",") });
function buildMultiSportParams(options: FetchMultiSportOptions): URLSearchParams {
const params = new URLSearchParams();
if (options.sports.length) params.set("sports", options.sports.join(","));
if (options.from && options.to) {
params.set("from", options.from);
params.set("to", options.to);
}
if (options.tz) {
params.set("tz", options.tz);
}
return params;
}

/**
* Fetch daily summaries for multiple sports in a single request.
* Returns a map of sport → daily data (Record<string, DailyActivity>).
*/
export const fetchMultiSportDailySummary = async (
options: FetchMultiSportOptions
): Promise<Record<string, Record<string, DailyActivity>>> => {
// No sports selected is a legitimate UI state (every sport hidden in Settings,
// then the "Visible" chip clicked). It is not a request: the backend 400s on a
// blank `sports` value, which surfaced as a generic "Failed to load calendar
// data" error where an empty state belonged.
if (options.sports.length === 0) return {};
const params = buildMultiSportParams(options);
const url = `activities/${options.year}/source?${params.toString()}`;

try {
Expand Down Expand Up @@ -258,14 +280,9 @@ export const fetchMultiSportDailySummary = async (
export const fetchMultiSportMetrics = async (
options: FetchMultiSportOptions
): Promise<Record<string, SportMetrics>> => {
const params = new URLSearchParams({ sports: options.sports.join(",") });
if (options.from && options.to) {
params.set("from", options.from);
params.set("to", options.to);
}
if (options.tz) {
params.set("tz", options.tz);
}
// See fetchMultiSportDailySummary: an empty selection has no request to make.
if (options.sports.length === 0) return {};
const params = buildMultiSportParams(options);
const url = `activities/${options.year}/metrics?${params.toString()}`;

try {
Expand Down
69 changes: 69 additions & 0 deletions packages/web/src/api/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,3 +268,72 @@ function createAxiosError(status: number, config: InternalAxiosRequestConfig): A
toJSON: () => ({}),
}) as AxiosError;
}

describe("auth-init re-arming (request interceptor)", () => {
beforeEach(() => {
resetClient();
client = getClient();
configureClientAuth(mockAuthService);
vi.mocked(mockAuthService.waitForAuthReady).mockReset();
vi.mocked(mockAuthService.getIdToken).mockReset().mockResolvedValue("original-token");
mockLogger.error.mockClear();
});

function captureAdapter() {
const seen: InternalAxiosRequestConfig[] = [];
client.defaults.adapter = vi.fn().mockImplementation((config: InternalAxiosRequestConfig) => {
seen.push(config);
return Promise.resolve({ status: 200, statusText: "OK", headers: {}, config, data: {} });
});
return seen;
}

const authHeaderOf = (config: InternalAxiosRequestConfig | undefined) =>
(config?.headers as unknown as Record<string, unknown> | undefined)?.Authorization as
string | undefined;

// Regression: authInitPromise used to cache the *result* of the readiness race,
// not just the in-flight wait. One throw (or one lost race against the 5s timer)
// left a resolved-false promise in the closure for the lifetime of the tab, so
// every later request skipped token injection, took a 401, and paid a
// refresh+retry round trip. The 401 interceptor kept it correct, which is why it
// stayed invisible. Found independently by two audits a week apart
// (2026-08-17-web:M1 and 2026-08-24-web:M1).
it("recovers on a later request after the first auth-init throws", async () => {
vi.mocked(mockAuthService.waitForAuthReady)
.mockRejectedValueOnce(new Error("firebase hiccup"))
.mockResolvedValue(undefined);

const seen = captureAdapter();

await client.get("activities");
expect(authHeaderOf(seen[0])).toBeUndefined(); // first request is the casualty

await client.get("activities");
expect(authHeaderOf(seen[1])).toBe("Bearer original-token"); // recovered
});

it("caches a successful init and does not re-wait on later requests", async () => {
vi.mocked(mockAuthService.waitForAuthReady).mockResolvedValue(undefined);
const seen = captureAdapter();

await client.get("activities");
await client.get("activities/1");

expect(authHeaderOf(seen[0])).toBe("Bearer original-token");
expect(authHeaderOf(seen[1])).toBe("Bearer original-token");
// The success path is still coalesced — one wait, not one per request.
expect(mockAuthService.waitForAuthReady).toHaveBeenCalledTimes(1);
});

it("logs the error cause rather than reporting a timeout that did not happen", async () => {
vi.mocked(mockAuthService.waitForAuthReady).mockRejectedValueOnce(new Error("boom"));
captureAdapter();

await client.get("activities");

const messages = mockLogger.error.mock.calls.map((c) => String(c[0]));
expect(messages.some((m) => m.includes("Auth initialization errored"))).toBe(true);
expect(messages.some((m) => m.includes("timed out"))).toBe(false);
});
});
58 changes: 48 additions & 10 deletions packages/web/src/api/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ function getClient() {
* - **Test isolation**: prevents interceptor state from leaking between tests
* - **HMR**: allows re-configuration when modules are hot-replaced in development
*
* Note on `authInitPromise`: it lives inside `configureClientAuth`'s closure and is
* deliberately not reset here. Clearing `configured` means the next
* `configureClientAuth()` call builds a fresh closure with its own null handle, and
* the interceptor no longer caches a failed outcome — so there is no stale negative
* for this function to clear.
*
* @internal — intended for tests and HMR; not for production application code.
*/
export function resetClient(): void {
Expand All @@ -109,6 +115,15 @@ export function resetClient(): void {

const AUTH_READY_TIMEOUT_MS = 5000;

/**
* Outcome of the one-time auth-readiness race.
*
* A discriminated result rather than a boolean so the request interceptor can
* report *which* failure happened — the two paths (timer won the race vs.
* waitForAuthReady threw) previously shared one "timed out" log line.
*/
type AuthInitOutcome = "ready" | "timeout" | "error";

/**
* Configure the API client with an auth service.
* Registers a request interceptor that waits for auth readiness and injects tokens.
Expand All @@ -122,7 +137,7 @@ export function configureClientAuth(authService: AuthService): void {
configured = true;

const instance = getClient();
let authInitPromise: Promise<boolean> | null = null;
let authInitPromise: Promise<AuthInitOutcome> | null = null;

instance.interceptors.request.use(async (config) => {
// Only our own API gateway gets auth tokens and trace propagation;
Expand All @@ -139,29 +154,52 @@ export function configureClientAuth(authService: AuthService): void {

// Wait for initial auth state with timeout (only on first request).
// Uses a shared promise so concurrent requests coalesce into one wait.
//
// Only a *successful* outcome is cached. Caching a failed race would poison
// the whole tab: every later request would await the same resolved-false
// promise, skip token injection via the early return below, take a 401, and
// pay a refresh+retry round trip — for the rest of the session, with no
// recovery short of a reload. The 401 response interceptor keeps that
// correct, which is exactly why it went unnoticed; the cost is silent
// latency plus one logger.error per request in production.
//
// The `authInitPromise === attempt` check means concurrent requests that are
// all awaiting this same attempt share one retry rather than stampeding N.
if (!authInitPromise) {
authInitPromise = (async () => {
const attempt = (async (): Promise<AuthInitOutcome> => {
try {
const timeoutPromise = new Promise<false>((resolve) => {
setTimeout(() => resolve(false), AUTH_READY_TIMEOUT_MS);
const timeoutPromise = new Promise<AuthInitOutcome>((resolve) => {
setTimeout(() => resolve("timeout"), AUTH_READY_TIMEOUT_MS);
});
const authPromise = authService.waitForAuthReady().then(() => true as const);
const authPromise = authService.waitForAuthReady().then(() => "ready" as const);
return await Promise.race([authPromise, timeoutPromise]);
} catch (e) {
logger.error(
"Auth initialization failed:",
e instanceof Error ? e.message : "unknown error"
);
return false;
return "error";
}
})();
authInitPromise = attempt;
void attempt.then((outcome) => {
if (outcome !== "ready" && authInitPromise === attempt) {
authInitPromise = null;
}
});
}

const ready = await authInitPromise;
if (!ready) {
const outcome = await authInitPromise;
if (outcome !== "ready") {
// Report the cause that actually occurred. Reporting "timed out" for the
// catch path sent readers looking for a slow network when the real signal
// was a thrown error.
logger.error(
`Auth initialization timed out after ${AUTH_READY_TIMEOUT_MS}ms. ` +
"Request will proceed without auth token and likely receive 401."
outcome === "timeout"
? `Auth initialization timed out after ${AUTH_READY_TIMEOUT_MS}ms. ` +
"Proceeding without an auth token; the 401 interceptor will refresh and retry."
: "Auth initialization errored. " +
"Proceeding without an auth token; the 401 interceptor will refresh and retry."
);
return config;
}
Expand Down
20 changes: 20 additions & 0 deletions packages/web/src/api/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,26 @@ describe("API Error Utilities", () => {
consoleErrorSpy.mockRestore();
});

// Regression: a native AbortError is a DOMException, which is NOT
// `instanceof Error` in browsers, so it fell through to the
// `new Error(String(err))` wrap and came back as a plain Error named
// "Error" — failing every branch of isCancellationError() and silently
// reclassifying a cancellation as a real failure.
it("re-throws a native AbortError unwrapped so it stays a cancellation", () => {
const abort = new DOMException("The operation was aborted.", "AbortError");
expect(abort instanceof Error).toBe(false); // the premise of the bug

let thrown: unknown;
try {
throwApiError(abort, "test");
} catch (e) {
thrown = e;
}

expect(thrown).toBe(abort);
expect(isCancellationError(thrown)).toBe(true);
});

it("should throw auth error for 401", () => {
const error = createAxiosError(401);
expect(() => throwApiError(error, "testFunc")).toThrow(
Expand Down
10 changes: 10 additions & 0 deletions packages/web/src/api/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,16 @@ export function throwApiError(err: unknown, context: string): never {
if (err instanceof Error) {
throw err;
}
// Re-throw cancellations unwrapped. A native AbortError is a DOMException,
// which is NOT `instanceof Error` in browsers, so it would otherwise fall to
// the wrap below and come back as a plain Error named "Error" — failing every
// branch of isCancellationError() above. That silently reclassifies a
// cancellation as a real failure for any caller following the documented
// isCancellationError pattern, contradicting the contract stated in the
// activities.ts header.
if (isCancellationError(err)) {
throw err;
}
// Wrap non-Error values, preserving original as cause for debugging
const wrapped = new Error(String(err));
(wrapped as Error & { cause?: unknown }).cause = err;
Expand Down
Loading