fix(ui): gate the usage export on range coverage, not on a fetch being in flight

A loading flag only flips once the fetch effect runs, so the render right after a
date or filter change still reported the previous range as loaded and let an export
read its rows. Stamp the completed range on the hook and compare it during render
instead, the way the tiles already do.

Also stop the failure banner claiming a page loaded when the first request is what
failed, which left it reading 1/1.
This commit is contained in:
ryan-crabbe-berri 2026-09-15 16:21:37 -07:00
parent 9b35954347
commit 69c3212220
9 changed files with 144 additions and 35 deletions

View file

@ -146,11 +146,11 @@ const EntityUsage: React.FC<EntityUsageProps> = ({
const {
data: spendDataRaw,
loading,
isFetchingMore,
progress,
cancelled,
failed,
coversRange,
cancel,
} = usePaginatedDailyActivity({
fetchFn,
@ -664,7 +664,7 @@ const EntityUsage: React.FC<EntityUsageProps> = ({
{ key: "endpoints", label: "Endpoint Activity", content: <EndpointUsage userSpendData={spendData} /> },
];
const spendFetchState = { loading, isFetchingMore, cancelled, failed };
const spendFetchState = { coversRange, cancelled, failed };
return (
<div style={{ width: "100%" }} className="relative">

View file

@ -250,9 +250,10 @@ const UsagePage: React.FC<UsagePageProps> = ({ teams, organizations }) => {
const loading = aggregatedLoading || paginatedResult.loading;
// Read through the same range stamp as the tiles, so the export is blocked from the first
// render of a new range rather than from whenever the fetch effect gets around to running.
const spendFetchState = {
loading,
isFetchingMore: paginatedResult.isFetchingMore,
coversRange: activeAggregated !== null || paginatedResult.coversRange,
cancelled: paginatedResult.cancelled,
failed: paginatedResult.failed,
};

View file

@ -180,6 +180,20 @@ describe("usePaginatedDailyActivity failure reporting", () => {
consoleError.mockRestore();
});
it("reports no pages loaded when the very first request is what failed", async () => {
const consoleError = vi.spyOn(console, "error").mockImplementation(() => {});
const fetchFn = vi.fn(() => Promise.reject(new Error("page 1 never came back")));
const { result } = renderHook(() =>
usePaginatedDailyActivity({ fetchFn, args: ["tok", start, end, null], enabled: true }),
);
await waitFor(() => expect(result.current.failed).toBe(true), { timeout: 5000 });
expect(result.current.progress).toEqual({ currentPage: 0, totalPages: 0 });
consoleError.mockRestore();
});
it("stays unfailed when every page arrives", async () => {
const pages = [
firstPage,
@ -222,3 +236,84 @@ describe("usePaginatedDailyActivity failure reporting", () => {
consoleError.mockRestore();
});
});
describe("usePaginatedDailyActivity range coverage", () => {
const start = new Date("2026-08-10");
const end = new Date("2026-08-17");
const singlePage = { results: [dayOf("2026-08-16", 2)], metadata: { total_pages: 1, page: 1, total_spend: 2 } };
it("does not cover the range while the hook is disabled", () => {
const fetchFn = vi.fn(() => Promise.resolve(singlePage));
const { result } = renderHook(() =>
usePaginatedDailyActivity({ fetchFn, args: ["tok", start, end, null], enabled: false }),
);
expect(result.current.coversRange).toBe(false);
expect(fetchFn).not.toHaveBeenCalled();
});
it("covers the range only once every page of it has landed", async () => {
const pages = [
{ results: [dayOf("2026-08-16", 2)], metadata: { total_pages: 2, page: 1, total_spend: 2 } },
{ results: [dayOf("2026-08-15", 1)], metadata: { total_pages: 2, page: 2, total_spend: 1 } },
];
const fetchFn = vi.fn((_token: string, _start: Date, _end: Date, page: number) => Promise.resolve(pages[page - 1]));
const { result } = renderHook(() =>
usePaginatedDailyActivity({ fetchFn, args: ["tok", start, end, null], enabled: true }),
);
expect(result.current.coversRange).toBe(false);
await waitFor(() => expect(result.current.coversRange).toBe(true), { timeout: 5000 });
});
it("never reports a range as covered while the data on screen is empty", async () => {
// Disabling the hook empties the data. Re-enabling it asks for the same args the last
// completed fetch used, so coverage that survives the disable would vouch for nothing.
const seen: Array<{ coversRange: boolean; rows: number }> = [];
const fetchFn = vi.fn(() => Promise.resolve(singlePage));
const { result, rerender } = renderHook(
({ enabled }: { enabled: boolean }) => {
const activity = usePaginatedDailyActivity({ fetchFn, args: ["tok", start, end, null], enabled });
seen.push({ coversRange: activity.coversRange, rows: activity.data.results.length });
return activity;
},
{ initialProps: { enabled: true } },
);
await waitFor(() => expect(result.current.coversRange).toBe(true), { timeout: 5000 });
rerender({ enabled: false });
rerender({ enabled: true });
await waitFor(() => expect(result.current.coversRange).toBe(true), { timeout: 5000 });
expect(seen.filter((render) => render.coversRange && render.rows === 0)).toEqual([]);
});
it("stops covering the range on the very render the args change, not once an effect catches up", async () => {
// The render after a filter change still holds the previous filter's rows, so resetting
// coverage inside the fetch effect would leave a paint where the export reads them as the
// new range. That paint is the whole thing the gate exists to stop.
const seen: Array<{ filter: string; coversRange: boolean }> = [];
const fetchFn = vi.fn(() => Promise.resolve(singlePage));
const { result, rerender } = renderHook(
({ filter }: { filter: string }) => {
const activity = usePaginatedDailyActivity({ fetchFn, args: ["tok", start, end, filter], enabled: true });
seen.push({ filter, coversRange: activity.coversRange });
return activity;
},
{ initialProps: { filter: "team-a" } },
);
await waitFor(() => expect(result.current.coversRange).toBe(true), { timeout: 5000 });
rerender({ filter: "team-b" });
const rendersForNewFilter = seen.filter((render) => render.filter === "team-b");
expect(rendersForNewFilter.length).toBeGreaterThan(0);
expect(rendersForNewFilter.map((render) => render.coversRange)).not.toContain(true);
});
});

View file

@ -61,8 +61,8 @@ interface UsePaginatedDailyActivityReturn {
isFetchingMore: boolean;
progress: PaginationProgress;
cancelled: boolean;
/** A page request threw, so `data` covers only part of the requested range. */
failed: boolean;
coversRange: boolean;
cancel: () => void;
}
@ -203,6 +203,7 @@ export function usePaginatedDailyActivity({
});
const [cancelled, setCancelled] = useState(false);
const [failed, setFailed] = useState(false);
const [completedKey, setCompletedKey] = useState<string | null>(null);
const fetchIdRef = useRef(0);
const cancelledRef = useRef(false);
@ -216,6 +217,11 @@ export function usePaginatedDailyActivity({
// Stable serialised key so the effect only re-runs when the arg *values* change.
const argsKey = JSON.stringify(args);
// Stamped like the data itself and compared during render, so the render that follows an arg
// change already reports the new range as uncovered. Clearing it inside the fetch effect would
// be one render too late, leaving a paint where an export reads the previous range's rows.
const coversRange = enabled && completedKey === argsKey;
const cancel = useCallback(() => {
cancelledRef.current = true;
setCancelled(true);
@ -234,6 +240,7 @@ export function usePaginatedDailyActivity({
setProgress({ currentPage: 0, totalPages: 0 });
setCancelled(false);
setFailed(false);
setCompletedKey(null);
return;
}
@ -257,7 +264,7 @@ export function usePaginatedDailyActivity({
const currentArgs = argsRef.current;
setLoading(true);
setIsFetchingMore(false);
setProgress({ currentPage: 1, totalPages: 1 });
setProgress({ currentPage: 0, totalPages: 0 });
if (aggregatedFetchFn) {
try {
@ -266,6 +273,7 @@ export function usePaginatedDailyActivity({
setData(aggregated);
setProgress({ currentPage: 1, totalPages: 1 });
setLoading(false);
setCompletedKey(argsKey);
return;
} catch (error) {
if (isStale()) return;
@ -288,6 +296,7 @@ export function usePaginatedDailyActivity({
if (totalPages <= 1) {
setLoading(false);
setCompletedKey(argsKey);
return;
}
@ -333,6 +342,7 @@ export function usePaginatedDailyActivity({
}
setIsFetchingMore(false);
setCompletedKey(argsKey);
} catch (error) {
if (!isStale()) {
console.error("Error fetching daily activity:", error);
@ -356,5 +366,5 @@ export function usePaginatedDailyActivity({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [enabled, fetchFn, aggregatedFetchFn, argsKey]);
return { data, loading, isFetchingMore, progress, cancelled, failed, cancel };
return { data, loading, isFetchingMore, progress, cancelled, failed, coversRange, cancel };
}

View file

@ -34,7 +34,6 @@ interface UsageExportHeaderProps {
customTitle?: string;
compactLayout?: boolean;
teams?: Team[];
/** Set to block the export and explain why; see getExportBlockedReason. */
exportBlockedReason?: string;
}

View file

@ -3,35 +3,30 @@ import { describe, expect, it } from "vitest";
import { getExportBlockedReason, type UsageFetchState } from "./exportBlockedReason";
const state = (overrides: Partial<UsageFetchState> = {}): UsageFetchState => ({
loading: false,
isFetchingMore: false,
coversRange: true,
cancelled: false,
failed: false,
...overrides,
});
describe("getExportBlockedReason", () => {
it("lets the export through once the range has fully loaded", () => {
it("lets the export through once the data on screen covers the range", () => {
expect(getExportBlockedReason(state())).toBeUndefined();
});
it("blocks the first load, before any page has arrived", () => {
expect(getExportBlockedReason(state({ loading: true }))).toMatch(/still loading/i);
});
it("blocks while later pages are still arriving, which is when a CSV silently under-reports", () => {
expect(getExportBlockedReason(state({ isFetchingMore: true }))).toMatch(/still loading/i);
it("blocks whenever the data on screen does not cover the range, which is when a CSV silently under-reports", () => {
expect(getExportBlockedReason(state({ coversRange: false }))).toMatch(/still loading/i);
});
it("blocks after a stopped fetch and says a reload is what fixes it", () => {
const reason = getExportBlockedReason(state({ cancelled: true }));
const reason = getExportBlockedReason(state({ coversRange: false, cancelled: true }));
expect(reason).toMatch(/stopped/i);
expect(reason).toMatch(/reload/i);
});
it("blocks after a failed page and names the failure rather than the stop", () => {
const reason = getExportBlockedReason(state({ failed: true, cancelled: true }));
const reason = getExportBlockedReason(state({ coversRange: false, failed: true, cancelled: true }));
expect(reason).toMatch(/failed to load/i);
expect(reason).not.toMatch(/stopped/i);

View file

@ -1,21 +1,13 @@
export interface UsageFetchState {
loading: boolean;
isFetchingMore: boolean;
coversRange: boolean;
cancelled: boolean;
failed: boolean;
}
/** Why exporting what is on screen would under-report, or undefined once it covers the whole range. */
export const getExportBlockedReason = ({
loading,
isFetchingMore,
cancelled,
failed,
}: UsageFetchState): string | undefined => {
export const getExportBlockedReason = ({ coversRange, cancelled, failed }: UsageFetchState): string | undefined => {
if (failed) return "Some spend data failed to load, so an export would under-report. Reload the page to try again.";
if (cancelled)
return "Loading was stopped before the whole range arrived, so an export would under-report. Reload the page to load it all.";
if (loading || isFetchingMore)
return "Spend data is still loading, so an export would under-report. Wait for it to finish.";
if (!coversRange) return "Spend data is still loading, so an export would under-report. Wait for it to finish.";
return undefined;
};

View file

@ -45,10 +45,25 @@ describe("PaginationStatusAlerts", () => {
);
expect(
screen.getByText(/Fetching spend data failed, so the totals below cover only part of the range \(7\/42 pages/),
screen.getByText(/Fetching spend data failed, so the totals below cover only 7 of 42 pages of the range/),
).toBeInTheDocument();
});
it("does not claim a page loaded when the very first request is what failed", () => {
render(
<PaginationStatusAlerts
isFetchingMore={false}
cancelled={false}
failed={true}
progress={{ currentPage: 0, totalPages: 0 }}
cancel={vi.fn()}
/>,
);
expect(screen.getByText(/failed before any of it arrived/)).toBeInTheDocument();
expect(screen.queryByText(/pages of the range/)).not.toBeInTheDocument();
});
it("shows only the failure when a stopped fetch also failed", () => {
render(
<PaginationStatusAlerts

View file

@ -12,6 +12,11 @@ interface PaginationStatusAlertsProps {
failed?: boolean;
}
const failureMessage = (subject: string, progress: { currentPage: number; totalPages: number }) =>
progress.currentPage === 0
? `Fetching ${subject} failed before any of it arrived, so the totals below are empty rather than final. Reload the page to try again.`
: `Fetching ${subject} failed, so the totals below cover only ${progress.currentPage} of ${progress.totalPages} pages of the range. Reload the page to try again.`;
const PaginationStatusAlerts = ({
isFetchingMore,
cancelled,
@ -42,10 +47,7 @@ const PaginationStatusAlerts = ({
)}
{failed && (
<Alert variant="error" className="mb-2">
<AlertDescription className="text-inherit">
Fetching {subject} failed, so the totals below cover only part of the range ({progress.currentPage}/
{progress.totalPages} pages loaded). Reload the page to try again.
</AlertDescription>
<AlertDescription className="text-inherit">{failureMessage(subject, progress)}</AlertDescription>
</Alert>
)}
{cancelled && !failed && (