fix(ui): don't show loading/error budget as "No limit"; add wiring test

Addresses review findings on the Max Budget tile fix:

- P1: while the selected user's info is loading or errored, the tile
  fed `null` and falsely rendered "No limit". Thread the query's
  loading/error state through a `budgetLoading` prop so the tile shows
  a neutral placeholder until the budget is resolved.
- Add a UsagePageView wiring regression test proving the tile receives
  the selected user's budget/duration (not the admin's) and is marked
  loading while unresolved, plus a ViewUserSpend loading-state test.
- Remove explanatory source comments per the repo comment policy.
This commit is contained in:
Trevor Mack 2026-09-16 16:02:49 -04:00
parent 3c7d3e0c57
commit c19001ce2e
No known key found for this signature in database
5 changed files with 93 additions and 31 deletions

View file

@ -5,14 +5,7 @@ import { createQueryKeys } from "../common/queryKeysFactory";
const userKeys = createQueryKeys("users"); const userKeys = createQueryKeys("users");
/** /** Fetch a specific user's info from `/v2/user/info?user_id=`; disabled when userId is null. */
* Fetch a SPECIFIC user's info from /v2/user/info?user_id=<userId>.
*
* Companion to useCurrentUser (which self-looks-up the caller): pass the user
* you actually want to display. Disabled when userId is null so the caller can
* fall back to a global/unfiltered view without firing a request. Admins may
* query any user; the backend authorizes the lookup.
*/
export const useUserInfo = (userId: string | null): UseQueryResult<UserInfoV2Response> => { export const useUserInfo = (userId: string | null): UseQueryResult<UserInfoV2Response> => {
const { accessToken } = useAuthorized(); const { accessToken } = useAuthorized();
return useQuery<UserInfoV2Response>({ return useQuery<UserInfoV2Response>({

View file

@ -4,6 +4,7 @@ import useAuthorized from "@/app/(dashboard)/hooks/useAuthorized";
import useIsOrgAdmin from "@/app/(dashboard)/hooks/useIsOrgAdmin"; import useIsOrgAdmin from "@/app/(dashboard)/hooks/useIsOrgAdmin";
import { useCurrentUser } from "@/app/(dashboard)/hooks/users/useCurrentUser"; import { useCurrentUser } from "@/app/(dashboard)/hooks/users/useCurrentUser";
import { useInfiniteUsers } from "@/app/(dashboard)/hooks/users/useUsers"; import { useInfiniteUsers } from "@/app/(dashboard)/hooks/users/useUsers";
import { useUserInfo } from "@/app/(dashboard)/hooks/users/useUserInfo";
import { act, fireEvent, screen, waitFor } from "@testing-library/react"; import { act, fireEvent, screen, waitFor } from "@testing-library/react";
import userEvent from "@testing-library/user-event"; import userEvent from "@testing-library/user-event";
import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
@ -40,7 +41,20 @@ vi.mock("@/components/activity_metrics", () => ({
})); }));
vi.mock("@/components/view_user_spend", () => ({ vi.mock("@/components/view_user_spend", () => ({
default: () => <div>View User Spend</div>, default: ({ userMaxBudget, budgetDuration, budgetLoading }: any) => (
<div
data-testid="view-user-spend"
data-max-budget={userMaxBudget === null || userMaxBudget === undefined ? "null" : String(userMaxBudget)}
data-budget-duration={budgetDuration ?? "null"}
data-budget-loading={budgetLoading ? "true" : "false"}
>
View User Spend
</div>
),
}));
vi.mock("@/app/(dashboard)/hooks/users/useUserInfo", () => ({
useUserInfo: vi.fn(),
})); }));
vi.mock("@/components/UsagePage/components/EntityUsage/TopKeyView", () => ({ vi.mock("@/components/UsagePage/components/EntityUsage/TopKeyView", () => ({
@ -166,6 +180,7 @@ describe("UsagePage", () => {
const mockUseAuthorized = vi.mocked(useAuthorized); const mockUseAuthorized = vi.mocked(useAuthorized);
const mockUseCurrentUser = vi.mocked(useCurrentUser); const mockUseCurrentUser = vi.mocked(useCurrentUser);
const mockUseInfiniteUsers = vi.mocked(useInfiniteUsers); const mockUseInfiniteUsers = vi.mocked(useInfiniteUsers);
const mockUseUserInfo = vi.mocked(useUserInfo);
const mockSpendData = { const mockSpendData = {
results: [ results: [
@ -411,6 +426,11 @@ describe("UsagePage", () => {
isLoading: false, isLoading: false,
error: null, error: null,
} as any); } as any);
mockUseUserInfo.mockReturnValue({
data: undefined,
isLoading: false,
isError: false,
} as any);
}); });
it("should render and fetch usage data on mount", async () => { it("should render and fetch usage data on mount", async () => {
@ -1350,19 +1370,57 @@ describe("UsagePage", () => {
}); });
}); });
describe("tab navigation in global view", () => { describe("selected-user budget wiring", () => {
it("should render all expected tabs", async () => { const userSelectCombobox = (): HTMLElement => {
let node: HTMLElement | null = screen.getByText("Filter by user");
while (node && !node.querySelector('[role="combobox"]')) {
node = node.parentElement;
}
return node!.querySelector('[role="combobox"]') as HTMLElement;
};
it("feeds the tile the selected user's budget and duration, not the admin's", async () => {
mockUseCurrentUser.mockReturnValue({
data: { user_id: "user-123", max_budget: 3000 },
isLoading: false,
error: null,
} as any);
mockUseUserInfo.mockReturnValue({
data: { user_id: "user-001", max_budget: 600, budget_duration: "30d" },
isLoading: false,
isError: false,
} as any);
renderWithProviders(<UsagePage {...defaultProps} />); renderWithProviders(<UsagePage {...defaultProps} />);
await waitFor(() => expect(mockUserDailyActivityAggregatedCall).toHaveBeenCalled());
await userEvent.setup().click(userSelectCombobox());
await userEvent.setup().click(screen.getByText("Alice (user-001)"));
await waitFor(() => { await waitFor(() => {
expect(mockUserDailyActivityAggregatedCall).toHaveBeenCalled(); expect(screen.getByTestId("view-user-spend")).toHaveAttribute("data-max-budget", "600");
}); });
expect(screen.getByTestId("view-user-spend")).toHaveAttribute("data-budget-duration", "30d");
expect(mockUseUserInfo).toHaveBeenCalledWith("user-001");
});
expect(screen.getByText("Cost")).toBeInTheDocument(); it("marks the tile loading (not unlimited) while the selected user's info is unresolved", async () => {
expect(screen.getByText("Model Activity")).toBeInTheDocument(); mockUseCurrentUser.mockReturnValue({
expect(screen.getByText("Key Activity")).toBeInTheDocument(); data: { user_id: "user-123", max_budget: 3000 },
expect(screen.getByText("MCP Server Activity")).toBeInTheDocument(); isLoading: false,
expect(screen.getByText("Endpoint Activity")).toBeInTheDocument(); error: null,
} as any);
mockUseUserInfo.mockReturnValue({ data: undefined, isLoading: true, isError: false } as any);
renderWithProviders(<UsagePage {...defaultProps} />);
await waitFor(() => expect(mockUserDailyActivityAggregatedCall).toHaveBeenCalled());
await userEvent.setup().click(userSelectCombobox());
await userEvent.setup().click(screen.getByText("Alice (user-001)"));
await waitFor(() => {
expect(screen.getByTestId("view-user-spend")).toHaveAttribute("data-budget-loading", "true");
});
}); });
}); });
}); });

View file

@ -141,12 +141,12 @@ const UsagePage: React.FC<UsagePageProps> = ({ teams, organizations }) => {
// For non-admins or "my-usage" view, always pass their own user_id // For non-admins or "my-usage" view, always pass their own user_id
const effectiveUserId = usageView === "my-usage" || !isAdmin ? userID || null : selectedUserId; const effectiveUserId = usageView === "my-usage" || !isAdmin ? userID || null : selectedUserId;
// Budget shown in the header tile must reflect the SELECTED user, not the const {
// logged-in admin (currentUser). Fetch the effective user's own record so the data: selectedUserInfo,
// Max Budget tile and its reset period track "Filter by user". Falls back to isLoading: isSelectedUserLoading,
// currentUser only while the per-user fetch is unresolved or when no user is isError: isSelectedUserError,
// selected (global view has no single budget to show). } = useUserInfo(effectiveUserId);
const { data: selectedUserInfo } = useUserInfo(effectiveUserId); const isSelectedUserResolved = effectiveUserId === null || (!isSelectedUserLoading && !isSelectedUserError);
const effectiveMaxBudget = const effectiveMaxBudget =
effectiveUserId !== null ? selectedUserInfo?.max_budget ?? null : currentUser?.max_budget ?? null; effectiveUserId !== null ? selectedUserInfo?.max_budget ?? null : currentUser?.max_budget ?? null;
const effectiveBudgetDuration = effectiveUserId !== null ? selectedUserInfo?.budget_duration ?? null : null; const effectiveBudgetDuration = effectiveUserId !== null ? selectedUserInfo?.budget_duration ?? null : null;
@ -591,6 +591,7 @@ const UsagePage: React.FC<UsagePageProps> = ({ teams, organizations }) => {
selectedTeam={null} selectedTeam={null}
userMaxBudget={effectiveMaxBudget} userMaxBudget={effectiveMaxBudget}
budgetDuration={effectiveBudgetDuration} budgetDuration={effectiveBudgetDuration}
budgetLoading={!isSelectedUserResolved}
/> />
</div> </div>

View file

@ -3,7 +3,6 @@ import { render, screen } from "@testing-library/react";
import React from "react"; import React from "react";
import ViewUserSpend from "./view_user_spend"; import ViewUserSpend from "./view_user_spend";
// ViewUserSpend fetches available models in an effect; stub the network call.
vi.mock("./networking", () => ({ vi.mock("./networking", () => ({
modelAvailableCall: vi.fn().mockResolvedValue({ data: [] }), modelAvailableCall: vi.fn().mockResolvedValue({ data: [] }),
})); }));
@ -18,7 +17,6 @@ describe("ViewUserSpend — Max Budget tile", () => {
it("shows a finite cap with its reset period when budgetDuration is set", () => { it("shows a finite cap with its reset period when budgetDuration is set", () => {
render(<ViewUserSpend userSpend={10} userMaxBudget={600} selectedTeam={null} budgetDuration="30d" />); render(<ViewUserSpend userSpend={10} userMaxBudget={600} selectedTeam={null} budgetDuration="30d" />);
expect(screen.getByText(/\$600\.0000 limit/)).toBeInTheDocument(); expect(screen.getByText(/\$600\.0000 limit/)).toBeInTheDocument();
// 30d maps to "monthly" via getBudgetDurationLabel
expect(screen.getByText(/over monthly/)).toBeInTheDocument(); expect(screen.getByText(/over monthly/)).toBeInTheDocument();
}); });
@ -33,4 +31,10 @@ describe("ViewUserSpend — Max Budget tile", () => {
expect(screen.getByText(/\$300\.0000 limit/)).toBeInTheDocument(); expect(screen.getByText(/\$300\.0000 limit/)).toBeInTheDocument();
expect(screen.queryByText(/over/)).not.toBeInTheDocument(); expect(screen.queryByText(/over/)).not.toBeInTheDocument();
}); });
it('does not render "No limit" while the budget is loading', () => {
render(<ViewUserSpend userSpend={10} userMaxBudget={null} selectedTeam={null} budgetLoading={true} />);
expect(screen.queryByText("No limit")).not.toBeInTheDocument();
expect(screen.getByText("—")).toBeInTheDocument();
});
}); });

View file

@ -10,15 +10,15 @@ interface ViewUserSpendProps {
userSpend: number | null; userSpend: number | null;
userMaxBudget: number | null; userMaxBudget: number | null;
selectedTeam: any | null; selectedTeam: any | null;
// Optional reset period paired with userMaxBudget (e.g. "24h", "30d"), rendered
// as a human-readable window next to the cap. Omitted/null → no period shown.
budgetDuration?: string | null; budgetDuration?: string | null;
budgetLoading?: boolean;
} }
const ViewUserSpend: React.FC<ViewUserSpendProps> = ({ const ViewUserSpend: React.FC<ViewUserSpendProps> = ({
userSpend, userSpend,
userMaxBudget, userMaxBudget,
selectedTeam, selectedTeam,
budgetDuration = null, budgetDuration = null,
budgetLoading = false,
}) => { }) => {
const { accessToken, userRole, userId: userID } = useAuthorized(); const { accessToken, userRole, userId: userID } = useAuthorized();
let [spend, setSpend] = useState(userSpend !== null ? userSpend : 0.0); let [spend, setSpend] = useState(userSpend !== null ? userSpend : 0.0);
@ -116,12 +116,18 @@ const ViewUserSpend: React.FC<ViewUserSpendProps> = ({
modelsToDisplay = userModels; modelsToDisplay = userModels;
} }
const displayMaxBudget = maxBudget !== null ? `$${formatNumberWithCommas(Number(maxBudget), 4)} limit` : "No limit"; let displayMaxBudget: string;
if (budgetLoading) {
displayMaxBudget = "—";
} else if (maxBudget !== null) {
displayMaxBudget = `$${formatNumberWithCommas(Number(maxBudget), 4)} limit`;
} else {
displayMaxBudget = "No limit";
}
// Show the reset window (e.g. "over monthly") only for a finite cap that has a
// paired duration; an unlimited budget or a cap with no duration shows nothing.
const durationLabel = maxBudget !== null && budgetDuration ? getBudgetDurationLabel(budgetDuration) : null; const durationLabel = maxBudget !== null && budgetDuration ? getBudgetDurationLabel(budgetDuration) : null;
const budgetPeriodSuffix = durationLabel && durationLabel !== "Not set" ? ` over ${durationLabel}` : ""; const budgetPeriodSuffix =
!budgetLoading && durationLabel && durationLabel !== "Not set" ? ` over ${durationLabel}` : "";
const roundedSpend = spend !== undefined ? formatNumberWithCommas(spend, 4) : null; const roundedSpend = spend !== undefined ? formatNumberWithCommas(spend, 4) : null;