Skip to content

Commit 8b7eef8

Browse files
authored
Show a distinct no-permission state for approvals (#3318)
1 parent a1ef500 commit 8b7eef8

9 files changed

Lines changed: 320 additions & 22 deletions

File tree

packages/api-client/src/fetcher.test.ts

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
import { beforeEach, describe, expect, it, vi } from "vitest";
2-
import { buildApiFetcher } from "./fetcher";
2+
import {
3+
ApiRequestError,
4+
buildApiFetcher,
5+
requestErrorStatus,
6+
} from "./fetcher";
37

48
describe("buildApiFetcher", () => {
59
const mockFetch = vi.fn();
@@ -170,4 +174,48 @@ describe("buildApiFetcher", () => {
170174
"Network request failed",
171175
);
172176
});
177+
178+
it("throws an ApiRequestError with a typed status and the legacy message format", async () => {
179+
mockFetch.mockResolvedValueOnce(err(404, { detail: "Not found" }));
180+
const fetcher = buildApiFetcher({
181+
getAccessToken: vi.fn().mockResolvedValue("token"),
182+
refreshAccessToken: vi.fn().mockResolvedValue("new-token"),
183+
appVersion: "test",
184+
});
185+
186+
const error = await fetcher.fetch(mockInput).catch((e: unknown) => e);
187+
expect(error).toBeInstanceOf(ApiRequestError);
188+
expect((error as ApiRequestError).status).toBe(404);
189+
// Catch sites across the codebase string-match on this exact format.
190+
expect((error as ApiRequestError).message).toBe(
191+
'Failed request: [404] {"detail":"Not found"}',
192+
);
193+
});
194+
195+
it("throws an ApiRequestError when refetching a token fails during retry", async () => {
196+
mockFetch.mockResolvedValueOnce(err(401));
197+
const fetcher = buildApiFetcher({
198+
getAccessToken: vi.fn().mockResolvedValue("token"),
199+
refreshAccessToken: vi.fn().mockRejectedValueOnce(new Error("failed")),
200+
appVersion: "test",
201+
});
202+
203+
const error = await fetcher.fetch(mockInput).catch((e: unknown) => e);
204+
expect(error).toBeInstanceOf(ApiRequestError);
205+
expect((error as ApiRequestError).status).toBe(401);
206+
});
207+
});
208+
209+
describe("requestErrorStatus", () => {
210+
it("returns the status of an ApiRequestError", () => {
211+
expect(requestErrorStatus(new ApiRequestError(404, "{}"))).toBe(404);
212+
});
213+
214+
it("returns undefined for plain errors and non-errors", () => {
215+
expect(requestErrorStatus(new Error("Failed request: [404] x"))).toBe(
216+
undefined,
217+
);
218+
expect(requestErrorStatus("Failed request: [404] x")).toBe(undefined);
219+
expect(requestErrorStatus(null)).toBe(undefined);
220+
});
173221
});

packages/api-client/src/fetcher.ts

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,26 @@ export type ApiFetcherConfig = {
66
appVersion: string;
77
};
88

9+
/**
10+
* Non-2xx HTTP response from the PostHog API. Keeps the legacy
11+
* `Failed request: [<status>] <body>` message format — many catch sites
12+
* string-match on it — while exposing the status as a typed field.
13+
*/
14+
export class ApiRequestError extends Error {
15+
readonly status: number;
16+
17+
constructor(status: number, serializedBody: string) {
18+
super(`Failed request: [${status}] ${serializedBody}`);
19+
this.name = "ApiRequestError";
20+
this.status = status;
21+
}
22+
}
23+
24+
/** HTTP status of an ApiRequestError, or undefined for any other error. */
25+
export function requestErrorStatus(error: unknown): number | undefined {
26+
return error instanceof ApiRequestError ? error.status : undefined;
27+
}
28+
929
export const buildApiFetcher: (
1030
config: ApiFetcherConfig,
1131
) => Parameters<typeof createApiClient>[0] = (config) => {
@@ -91,8 +111,9 @@ export const buildApiFetcher: (
91111
.catch(() =>
92112
cloned.text().then((t) => ({ error: t || `${response.status}` })),
93113
);
94-
throw new Error(
95-
`Failed request: [${response.status}] ${JSON.stringify(errorResponse)}`,
114+
throw new ApiRequestError(
115+
response.status,
116+
JSON.stringify(errorResponse),
96117
);
97118
}
98119
}
@@ -104,8 +125,9 @@ export const buildApiFetcher: (
104125
.catch(() =>
105126
cloned.text().then((t) => ({ error: t || `${response.status}` })),
106127
);
107-
throw new Error(
108-
`Failed request: [${response.status}] ${JSON.stringify(errorResponse)}`,
128+
throw new ApiRequestError(
129+
response.status,
130+
JSON.stringify(errorResponse),
109131
);
110132
}
111133

packages/ui/src/features/agent-applications/components/AgentApprovalsPane.tsx

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,18 @@ export function AgentApprovalsPane({
3232
filter: ApprovalFilter;
3333
onFilterChange: (f: ApprovalFilter) => void;
3434
}) {
35-
const { data, isLoading, isError, isFetching, dataUpdatedAt, refetch } =
36-
useAgentApplicationApprovals(
37-
idOrSlug,
38-
filter === "all" ? undefined : { state: filter },
39-
);
35+
const {
36+
data,
37+
isLoading,
38+
isError,
39+
isPermissionError,
40+
isFetching,
41+
dataUpdatedAt,
42+
refetch,
43+
} = useAgentApplicationApprovals(
44+
idOrSlug,
45+
filter === "all" ? undefined : { state: filter },
46+
);
4047
const approvals = useMemo(() => data ?? [], [data]);
4148
const selected = selectedId
4249
? (approvals.find((a) => a.id === selectedId) ?? null)
@@ -78,10 +85,17 @@ export function AgentApprovalsPane({
7885
/>
7986
))}
8087
</Flex>
88+
) : isPermissionError ? (
89+
// A 404 here means the admin gate: AgentDetailLayout only renders this
90+
// pane's content once the application itself has loaded successfully.
91+
<AgentDetailEmptyState
92+
title="You need organization admin access"
93+
description="Tool approvals can only be viewed and decided by organization admins. Ask an admin to review pending requests."
94+
/>
8195
) : isError ? (
8296
<AgentDetailEmptyState
8397
title="Couldn't load approvals"
84-
description="The agent platform API returned an error. Approvals are team-admin only — you may not have access."
98+
description="The agent platform API returned an error while loading approvals. Try again shortly."
8599
/>
86100
) : approvals.length === 0 ? (
87101
<AgentDetailEmptyState

packages/ui/src/features/agent-applications/components/AgentFleetApprovalsPane.tsx

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,15 @@ export function AgentFleetApprovalsPane({
3737
filter: ApprovalFilter;
3838
onFilterChange: (f: ApprovalFilter) => void;
3939
}) {
40-
const { data, isLoading, isError, isFetching, dataUpdatedAt, refetch } =
41-
useAgentFleetApprovals(filter === "all" ? undefined : { state: filter });
40+
const {
41+
data,
42+
isLoading,
43+
isError,
44+
isPermissionError,
45+
isFetching,
46+
dataUpdatedAt,
47+
refetch,
48+
} = useAgentFleetApprovals(filter === "all" ? undefined : { state: filter });
4249
const { data: applications } = useAgentApplications();
4350

4451
const approvals = useMemo(() => data ?? [], [data]);
@@ -109,10 +116,15 @@ export function AgentFleetApprovalsPane({
109116
/>
110117
))}
111118
</Flex>
119+
) : isPermissionError ? (
120+
<AgentDetailEmptyState
121+
title="You need organization admin access"
122+
description="Tool approvals can only be viewed and decided by organization admins. Ask an admin to review pending requests."
123+
/>
112124
) : isError ? (
113125
<AgentDetailEmptyState
114126
title="Couldn't load approvals"
115-
description="The agent platform API returned an error. Fleet approvals are team-admin only — you may not have access."
127+
description="The agent platform API returned an error while loading fleet approvals. Try again shortly."
116128
/>
117129
) : approvals.length === 0 ? (
118130
<AgentDetailEmptyState
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { requestErrorStatus } from "@posthog/api-client/fetcher";
2+
3+
/**
4+
* The backend gates the approvals endpoints behind an org-membership ADMIN
5+
* check and answers 404 (not 403) for non-admins, so a 404 from these
6+
* endpoints means "no permission". For the per-agent endpoint this only
7+
* holds when the application is known to exist — AgentApprovalsPane renders
8+
* inside AgentDetailLayout, which gates on the application having loaded.
9+
*/
10+
export function isApprovalsPermissionError(error: unknown): boolean {
11+
return requestErrorStatus(error) === 404;
12+
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import { ApiRequestError } from "@posthog/api-client/fetcher";
2+
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
3+
import { renderHook, waitFor } from "@testing-library/react";
4+
import type { ReactNode } from "react";
5+
import { beforeEach, describe, expect, it, vi } from "vitest";
6+
7+
const mockListAgentApplicationApprovals = vi.hoisted(() => vi.fn());
8+
const mockClient = vi.hoisted(() => ({
9+
listAgentApplicationApprovals: mockListAgentApplicationApprovals,
10+
}));
11+
12+
vi.mock("@posthog/ui/features/auth/authClient", () => ({
13+
useOptionalAuthenticatedClient: () => mockClient,
14+
}));
15+
16+
vi.mock("../../auth/store", () => ({
17+
useAuthStateValue: <T,>(
18+
selector: (state: { currentProjectId: number }) => T,
19+
) => selector({ currentProjectId: 2 }),
20+
}));
21+
22+
import { useAgentApplicationApprovals } from "./useAgentApplicationApprovals";
23+
24+
function renderApprovalsHook(idOrSlug: string) {
25+
const queryClient = new QueryClient({
26+
// Zero out the backoff so the hook's own `retry` option (under test)
27+
// drives the call count without slowing the suite.
28+
defaultOptions: { queries: { retryDelay: 0 } },
29+
});
30+
const wrapper = ({ children }: { children: ReactNode }) => (
31+
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
32+
);
33+
return renderHook(() => useAgentApplicationApprovals(idOrSlug), { wrapper });
34+
}
35+
36+
describe("useAgentApplicationApprovals", () => {
37+
beforeEach(() => {
38+
vi.clearAllMocks();
39+
});
40+
41+
it("flags a 404 as a permission error without retrying", async () => {
42+
mockListAgentApplicationApprovals.mockRejectedValue(
43+
new ApiRequestError(404, '{"detail":"Not found"}'),
44+
);
45+
const { result } = renderApprovalsHook("my-agent");
46+
47+
await waitFor(() => expect(result.current.isError).toBe(true));
48+
expect(result.current.isPermissionError).toBe(true);
49+
// The admin gate never clears on retry, so the hook must not retry.
50+
expect(mockListAgentApplicationApprovals).toHaveBeenCalledTimes(1);
51+
});
52+
53+
it("treats non-404 failures as genuine errors and retries them", async () => {
54+
mockListAgentApplicationApprovals.mockRejectedValue(
55+
new ApiRequestError(500, '{"error":"boom"}'),
56+
);
57+
const { result } = renderApprovalsHook("my-agent");
58+
59+
await waitFor(() => expect(result.current.isError).toBe(true));
60+
expect(result.current.isPermissionError).toBe(false);
61+
expect(mockListAgentApplicationApprovals).toHaveBeenCalledTimes(4);
62+
});
63+
64+
it("returns approvals on success", async () => {
65+
mockListAgentApplicationApprovals.mockResolvedValue([{ id: "approval-1" }]);
66+
const { result } = renderApprovalsHook("my-agent");
67+
68+
await waitFor(() => expect(result.current.isSuccess).toBe(true));
69+
expect(result.current.data).toEqual([{ id: "approval-1" }]);
70+
expect(result.current.isPermissionError).toBe(false);
71+
});
72+
73+
it("stays disabled and never fetches without an idOrSlug", async () => {
74+
mockListAgentApplicationApprovals.mockResolvedValue([]);
75+
const { result } = renderApprovalsHook("");
76+
77+
// enabled gates on !!idOrSlug — the query must stay pending with no fetch.
78+
await new Promise((resolve) => setTimeout(resolve, 50));
79+
expect(mockListAgentApplicationApprovals).not.toHaveBeenCalled();
80+
expect(result.current.isPending).toBe(true);
81+
expect(result.current.isPermissionError).toBe(false);
82+
});
83+
});

packages/ui/src/features/agent-applications/hooks/useAgentApplicationApprovals.ts

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,36 @@ import type {
55
import { useAuthenticatedQuery } from "@posthog/ui/hooks/useAuthenticatedQuery";
66
import { useAuthStateValue } from "../../auth/store";
77
import { agentApplicationsKeys } from "./agentApplicationsKeys";
8+
import { isApprovalsPermissionError } from "./approvalsPermission";
89

910
/**
10-
* Lists tool-approval requests for one agent. Optionally filtered to a single
11-
* state (the backend accepts one `state` value); omit for all states.
11+
* Lists tool-approval requests for one agent (organization-admin only).
12+
* Optionally filtered to a single state (the backend accepts one `state`
13+
* value); omit for all states. `isPermissionError` is true when the viewer
14+
* lacks admin access.
1215
*/
1316
export function useAgentApplicationApprovals(
1417
idOrSlug: string,
1518
params?: AgentApprovalsListParams,
1619
) {
1720
const projectId = useAuthStateValue((state) => state.currentProjectId);
18-
return useAuthenticatedQuery<AgentApprovalRequest[]>(
21+
const query = useAuthenticatedQuery<AgentApprovalRequest[]>(
1922
agentApplicationsKeys.approvals(projectId, idOrSlug, params?.state),
2023
(client) => client.listAgentApplicationApprovals(idOrSlug, params),
2124
{
2225
enabled: !!projectId && !!idOrSlug,
2326
staleTime: 10_000,
24-
// Queued approvals change as agents run; poll while the tab is focused.
25-
refetchInterval: 10_000,
27+
// Queued approvals change as agents run; poll while the tab is focused —
28+
// but stop once we know the viewer lacks org-admin access.
29+
refetchInterval: (q) =>
30+
isApprovalsPermissionError(q.state.error) ? false : 10_000,
31+
// A 404 here is the admin gate, not a transient failure — don't retry.
32+
retry: (failureCount, error) =>
33+
!isApprovalsPermissionError(error) && failureCount < 3,
2634
},
2735
);
36+
return {
37+
...query,
38+
isPermissionError: isApprovalsPermissionError(query.error),
39+
};
2840
}

0 commit comments

Comments
 (0)