From 10691c2c3374603d5901962560f1ad39adaf24ef Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Fri, 20 Mar 2026 23:46:21 -0700 Subject: [PATCH] address greptile review feedback (greploop iteration 4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Skip user-facing notification when guard throws for missing policyName — avoids exposing technical error message to users - Remove redundant fetchPolicyVersions wrapper, call listPolicyVersions directly in queryFn - Extract shared createTestEnv() factory to deduplicate test setup across all three describe blocks - Update null-policyName tests to verify no notification is shown Co-Authored-By: Claude Opus 4.6 --- .../hooks/policies/usePolicyVersions.test.ts | 132 +++++++----------- .../hooks/policies/usePolicyVersions.ts | 14 +- 2 files changed, 55 insertions(+), 91 deletions(-) diff --git a/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.test.ts index ba5792644c0..b5cce8c5e88 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.test.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.test.ts @@ -36,35 +36,41 @@ vi.mock("../useAuthorized", () => ({ default: () => mockUseAuthorized(), })); -// ── Setup ─────────────────────────────────────────────────────────────────── +// ── Shared test helpers ────────────────────────────────────────────────────── -describe("usePolicyVersions", () => { - let queryClient: QueryClient; +function createTestEnv() { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { retry: false }, + mutations: { retry: false }, + }, + }); - beforeEach(() => { - queryClient = new QueryClient({ - defaultOptions: { - queries: { retry: false }, - mutations: { retry: false }, - }, - }); + vi.clearAllMocks(); - vi.clearAllMocks(); - - mockUseAuthorized.mockReturnValue({ - accessToken: "test-access-token", - userRole: "Admin", - userId: "test-user-id", - token: "test-token", - userEmail: "test@example.com", - premiumUser: false, - }); + mockUseAuthorized.mockReturnValue({ + accessToken: "test-access-token", + userRole: "Admin", + userId: "test-user-id", + token: "test-token", + userEmail: "test@example.com", + premiumUser: false, }); const wrapper = ({ children }: { children: ReactNode }) => React.createElement(QueryClientProvider, { client: queryClient }, children); - // ── Query tests ───────────────────────────────────────────────────────── + return { queryClient, wrapper }; +} + +// ── Tests ──────────────────────────────────────────────────────────────────── + +describe("usePolicyVersions", () => { + let env: ReturnType; + + beforeEach(() => { + env = createTestEnv(); + }); it("fetches versions when policyName is provided", async () => { const mockResponse = { @@ -79,7 +85,7 @@ describe("usePolicyVersions", () => { const { result } = renderHook( () => usePolicyVersions({ policyName: "my-policy" }), - { wrapper } + { wrapper: env.wrapper } ); await waitFor(() => expect(result.current.isSuccess).toBe(true)); @@ -92,7 +98,7 @@ describe("usePolicyVersions", () => { it("does not fetch when policyName is null", () => { const { result } = renderHook( () => usePolicyVersions({ policyName: null }), - { wrapper } + { wrapper: env.wrapper } ); expect(result.current.fetchStatus).toBe("idle"); @@ -104,7 +110,7 @@ describe("usePolicyVersions", () => { it("does not fetch when enabled is false", () => { const { result } = renderHook( () => usePolicyVersions({ policyName: "my-policy", enabled: false }), - { wrapper } + { wrapper: env.wrapper } ); expect(result.current.fetchStatus).toBe("idle"); @@ -122,7 +128,7 @@ describe("usePolicyVersions", () => { const { result } = renderHook( () => usePolicyVersions({ policyName: "my-policy" }), - { wrapper } + { wrapper: env.wrapper } ); await waitFor(() => expect(result.current.isSuccess).toBe(true)); @@ -131,38 +137,19 @@ describe("usePolicyVersions", () => { }); describe("useCreatePolicyVersion", () => { - let queryClient: QueryClient; + let env: ReturnType; beforeEach(() => { - queryClient = new QueryClient({ - defaultOptions: { - queries: { retry: false }, - mutations: { retry: false }, - }, - }); - - vi.clearAllMocks(); - - mockUseAuthorized.mockReturnValue({ - accessToken: "test-access-token", - userRole: "Admin", - userId: "test-user-id", - token: "test-token", - userEmail: "test@example.com", - premiumUser: false, - }); + env = createTestEnv(); }); - const wrapper = ({ children }: { children: ReactNode }) => - React.createElement(QueryClientProvider, { client: queryClient }, children); - it("calls createPolicyVersion and shows success notification", async () => { const newPolicy = { policy_id: "v3", policy_name: "my-policy", version_number: 3 }; mockCreatePolicyVersion.mockResolvedValue(newPolicy); const { result } = renderHook( () => useCreatePolicyVersion("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); const returned = await result.current.mutateAsync(); @@ -174,11 +161,11 @@ describe("useCreatePolicyVersion", () => { it("invalidates the versions cache on success", async () => { mockCreatePolicyVersion.mockResolvedValue({ policy_id: "v3" }); - const invalidateSpy = vi.spyOn(queryClient, "invalidateQueries"); + const invalidateSpy = vi.spyOn(env.queryClient, "invalidateQueries"); const { result } = renderHook( () => useCreatePolicyVersion("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await result.current.mutateAsync(); @@ -193,7 +180,7 @@ describe("useCreatePolicyVersion", () => { const { result } = renderHook( () => useCreatePolicyVersion("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await expect(result.current.mutateAsync()).rejects.toThrow("Server error"); @@ -202,51 +189,33 @@ describe("useCreatePolicyVersion", () => { ); }); - it("throws when policyName is null", async () => { + it("does not show user-facing notification when policyName is null", async () => { const { result } = renderHook( () => useCreatePolicyVersion(null), - { wrapper } + { wrapper: env.wrapper } ); await expect(result.current.mutateAsync()).rejects.toThrow( "Missing access token or policy name" ); + expect(NotificationsManager.fromBackend).not.toHaveBeenCalled(); }); }); describe("useUpdatePolicyVersionStatus", () => { - let queryClient: QueryClient; + let env: ReturnType; beforeEach(() => { - queryClient = new QueryClient({ - defaultOptions: { - queries: { retry: false }, - mutations: { retry: false }, - }, - }); - - vi.clearAllMocks(); - - mockUseAuthorized.mockReturnValue({ - accessToken: "test-access-token", - userRole: "Admin", - userId: "test-user-id", - token: "test-token", - userEmail: "test@example.com", - premiumUser: false, - }); + env = createTestEnv(); }); - const wrapper = ({ children }: { children: ReactNode }) => - React.createElement(QueryClientProvider, { client: queryClient }, children); - it("publishes a version and shows success notification", async () => { const updatedPolicy = { policy_id: "v2", version_status: "published" }; mockUpdatePolicyVersionStatus.mockResolvedValue(updatedPolicy); const { result } = renderHook( () => useUpdatePolicyVersionStatus("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); const returned = await result.current.mutateAsync({ @@ -267,11 +236,11 @@ describe("useUpdatePolicyVersionStatus", () => { it("invalidates the versions cache on success", async () => { mockUpdatePolicyVersionStatus.mockResolvedValue({ policy_id: "v2" }); - const invalidateSpy = vi.spyOn(queryClient, "invalidateQueries"); + const invalidateSpy = vi.spyOn(env.queryClient, "invalidateQueries"); const { result } = renderHook( () => useUpdatePolicyVersionStatus("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await result.current.mutateAsync({ policyId: "v2", status: "published" }); @@ -287,7 +256,7 @@ describe("useUpdatePolicyVersionStatus", () => { const { result } = renderHook( () => useUpdatePolicyVersionStatus("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await result.current.mutateAsync({ @@ -310,7 +279,7 @@ describe("useUpdatePolicyVersionStatus", () => { const { result } = renderHook( () => useUpdatePolicyVersionStatus("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await expect( @@ -326,7 +295,7 @@ describe("useUpdatePolicyVersionStatus", () => { const { result } = renderHook( () => useUpdatePolicyVersionStatus("my-policy"), - { wrapper } + { wrapper: env.wrapper } ); await expect( @@ -337,14 +306,15 @@ describe("useUpdatePolicyVersionStatus", () => { ); }); - it("throws when policyName is null", async () => { + it("does not show user-facing notification when policyName is null", async () => { const { result } = renderHook( () => useUpdatePolicyVersionStatus(null), - { wrapper } + { wrapper: env.wrapper } ); await expect( result.current.mutateAsync({ policyId: "v2", status: "published" }) ).rejects.toThrow("Missing access token or policy name"); + expect(NotificationsManager.fromBackend).not.toHaveBeenCalled(); }); }); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.ts b/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.ts index 1bcf3a1e856..7a49876ea40 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/hooks/policies/usePolicyVersions.ts @@ -28,15 +28,6 @@ export interface PolicyVersionsData { total_count: number; } -// ── Fetch function ────────────────────────────────────────────────────────── - -const fetchPolicyVersions = async ( - accessToken: string, - policyName: string -): Promise => { - return await listPolicyVersions(accessToken, policyName); -}; - // ── Hook ──────────────────────────────────────────────────────────────────── export interface UsePolicyVersionsOptions { @@ -56,7 +47,8 @@ export const usePolicyVersions = ({ return useQuery({ queryKey: policyVersionKeys.detail(policyName ?? DISABLED_POLICY_KEY), - queryFn: async () => await fetchPolicyVersions(accessToken!, policyName!), + queryFn: async () => + await listPolicyVersions(accessToken!, policyName!) as PolicyVersionsResponse, enabled: isEnabled, select: (data) => ({ ...data, @@ -87,6 +79,7 @@ export const useCreatePolicyVersion = (policyName: string | null | undefined) => } }, onError: (error) => { + if (error.message === "Missing access token or policy name") return; NotificationsManager.fromBackend( "Failed to create version: " + error.message ); @@ -124,6 +117,7 @@ export const useUpdatePolicyVersionStatus = ( } }, onError: (error, variables) => { + if (error.message === "Missing access token or policy name") return; const action = variables.status === "published" ? "publish" : "promote to production"; NotificationsManager.fromBackend(