diff --git a/packages/web/src/api/activities.test.ts b/packages/web/src/api/activities.test.ts index 161aaca3..ddc2e04d 100644 --- a/packages/web/src/api/activities.test.ts +++ b/packages/web/src/api/activities.test.ts @@ -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() })); @@ -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"); + }); +}); diff --git a/packages/web/src/api/activities.ts b/packages/web/src/api/activities.ts index 8c2508d3..f9271506 100644 --- a/packages/web/src/api/activities.ts +++ b/packages/web/src/api/activities.ts @@ -217,13 +217,19 @@ export interface FetchMultiSportOptions { } /** - * Fetch daily summaries for multiple sports in a single request. - * Returns a map of sport → daily data (Record). + * 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>> => { - 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); @@ -231,6 +237,22 @@ export const fetchMultiSportDailySummary = async ( 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). + */ +export const fetchMultiSportDailySummary = async ( + options: FetchMultiSportOptions +): Promise>> => { + // 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 { @@ -258,14 +280,9 @@ export const fetchMultiSportDailySummary = async ( export const fetchMultiSportMetrics = async ( options: FetchMultiSportOptions ): Promise> => { - 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 { diff --git a/packages/web/src/api/client.test.ts b/packages/web/src/api/client.test.ts index 8bc5c5da..9b12bc4f 100644 --- a/packages/web/src/api/client.test.ts +++ b/packages/web/src/api/client.test.ts @@ -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 | 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); + }); +}); diff --git a/packages/web/src/api/client.ts b/packages/web/src/api/client.ts index 7906d717..141373ba 100644 --- a/packages/web/src/api/client.ts +++ b/packages/web/src/api/client.ts @@ -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 { @@ -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. @@ -122,7 +137,7 @@ export function configureClientAuth(authService: AuthService): void { configured = true; const instance = getClient(); - let authInitPromise: Promise | null = null; + let authInitPromise: Promise | null = null; instance.interceptors.request.use(async (config) => { // Only our own API gateway gets auth tokens and trace propagation; @@ -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 => { try { - const timeoutPromise = new Promise((resolve) => { - setTimeout(() => resolve(false), AUTH_READY_TIMEOUT_MS); + const timeoutPromise = new Promise((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; } diff --git a/packages/web/src/api/errors.test.ts b/packages/web/src/api/errors.test.ts index 7ceb42d4..3c5b7436 100644 --- a/packages/web/src/api/errors.test.ts +++ b/packages/web/src/api/errors.test.ts @@ -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( diff --git a/packages/web/src/api/errors.ts b/packages/web/src/api/errors.ts index f9439737..9bda5e6f 100644 --- a/packages/web/src/api/errors.ts +++ b/packages/web/src/api/errors.ts @@ -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;