diff --git a/.changeset/11736-inbox-error-stops-poll.md b/.changeset/11736-inbox-error-stops-poll.md new file mode 100644 index 0000000000..3ccfdc196f --- /dev/null +++ b/.changeset/11736-inbox-error-stops-poll.md @@ -0,0 +1,17 @@ +--- +'@object-ui/plugin-chatbot': patch +--- + +The AI Approvals inbox shows the error, not an empty queue, when its read fails, and its poll stops on an answer that will not change (objectui#11736). + +On a deployment with no AI service, the open edition answers `501` on `/api/v1/ai/pending-actions`. `AiPendingActionsInbox` rendered the error alert and, beneath it, "No actions waiting". That reads as a live approval queue that happens to be empty, on a deployment that has none. `usePendingActions` also re-armed its five-second interval whatever the read answered, so the page asked the dead endpoint every five seconds for as long as it stayed open. + +- The inbox shows the empty state only when the read answered. When the read failed, the error alert stands alone. +- `usePendingActions` now arms each poll after the previous read settles, according to its answer: + - A refused read stops the poll. That is `501`, or any `4xx` except `408` and `429`, such as `401`, `403` or `404`. A manual `refresh()` that succeeds, the re-fetch after `approve` or `reject`, or a change to the hook's options starts it again. + - A transient failure backs the poll off. That is no answer at all, `408`, `429`, or a `5xx` other than `501`. The delay doubles with each consecutive failure, up to 120 seconds, or up to `pollInterval` if that is longer. + - A read that succeeds polls again after `pollInterval`, as before, and resets the backoff. + +The policy is in the hook, so every caller of `usePendingActions` gets it, not only the inbox. With a working endpoint nothing changes: an empty queue still shows "No actions waiting", and the poll keeps its interval. + +**Clause-②: no.** No export, prop, option, return member or language-pack key is added or removed, and no accepted input widens. diff --git a/packages/plugin-chatbot/src/AiPendingActionsInbox.tsx b/packages/plugin-chatbot/src/AiPendingActionsInbox.tsx index 8b406d3206..0ff7c08a74 100644 --- a/packages/plugin-chatbot/src/AiPendingActionsInbox.tsx +++ b/packages/plugin-chatbot/src/AiPendingActionsInbox.tsx @@ -364,13 +364,21 @@ export function AiPendingActionsInbox({ ) : null} + {/* + The empty state is an ANSWER ("nothing is waiting"), so it is shown + only when the read answered (objectui#11736). If the read failed, the + error alert above is the whole story. On a deployment with no AI + service, an empty queue under the 501 alert read as a live approval + queue that happened to be empty, which is a queue the deployment does + not have. + */} {isLoading && rows.length === 0 ? (
- ) : rows.length === 0 ? ( + ) : rows.length === 0 && error ? null : rows.length === 0 ? ( diff --git a/packages/plugin-chatbot/src/__tests__/AiPendingActionsInbox.errorReplacesEmpty-11736.test.tsx b/packages/plugin-chatbot/src/__tests__/AiPendingActionsInbox.errorReplacesEmpty-11736.test.tsx new file mode 100644 index 0000000000..f610680e3e --- /dev/null +++ b/packages/plugin-chatbot/src/__tests__/AiPendingActionsInbox.errorReplacesEmpty-11736.test.tsx @@ -0,0 +1,117 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `AiPendingActionsInbox` on a deployment with no AI service (objectui#11736). + * + * The open edition answers `501` on `/api/v1/ai/pending-actions`. Before the + * fix the inbox rendered the error alert and, beneath it, the "No actions + * waiting" empty state. That reads as a live approval queue that happens to be + * empty, on a deployment that has no queue at all. The page also re-polled the + * dead endpoint every five seconds while it stayed open. + * + * These pins hold both halves, and the live control the card asks for: with a + * working endpoint, an empty queue still shows "No actions waiting" and the + * poll continues. The empty-state title is asserted by its English copy + * because the card names that copy as the thing a reader sees. No + * `I18nProvider` is mounted, so it comes from the component's defaults map + * (see `AiPendingActionsInbox.noProviderFallback.test.tsx` for why that stays + * in its own file). + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { render, screen, cleanup, act } from '@testing-library/react'; +import { AiPendingActionsInbox } from '../AiPendingActionsInbox'; + +const POLL = 5_000; +const EMPTY_TITLE = 'No actions waiting'; +const REMEDY = 'The AI service is not available on this deployment.'; + +function answer(status: number, body: unknown): Response { + return { + ok: status >= 200 && status < 300, + status, + statusText: '', + json: async () => body, + } as unknown as Response; +} + +function stubFetch(respond: (n: number) => Response) { + let n = 0; + const fetchMock = vi.fn(async () => respond(++n)); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +const settle = () => act(async () => { await vi.advanceTimersByTimeAsync(0); }); +const advance = (ms: number) => act(async () => { await vi.advanceTimersByTimeAsync(ms); }); + +beforeEach(() => { + vi.useFakeTimers(); +}); +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('AiPendingActionsInbox: an errored read shows the error, not an empty queue (objectui#11736)', () => { + it('on a 501 it shows the server message, no empty state, and stops polling', async () => { + const fetchMock = stubFetch(() => + answer(501, { success: false, error: { code: 'NOT_IMPLEMENTED', message: REMEDY } }), + ); + render(); + await settle(); + + expect(screen.getByText(REMEDY)).toBeTruthy(); + expect(screen.queryByText(EMPTY_TITLE)).toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await advance(10 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(screen.getByText(REMEDY)).toBeTruthy(); + expect(screen.queryByText(EMPTY_TITLE)).toBeNull(); + }); + + it('on a transient failure it shows the error, no empty state, and the empty state returns once the read answers', async () => { + const fetchMock = stubFetch((n) => + n === 1 ? answer(503, { error: { message: 'Service Unavailable' } }) : answer(200, { items: [], total: 0 }), + ); + render(); + await settle(); + + expect(screen.getByText('Service Unavailable')).toBeTruthy(); + expect(screen.queryByText(EMPTY_TITLE)).toBeNull(); + + // Backed off: the retry comes after twice the interval, not after one. + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(2); + + expect(screen.queryByText('Service Unavailable')).toBeNull(); + expect(screen.getByText(EMPTY_TITLE)).toBeTruthy(); + }); +}); + +describe('AiPendingActionsInbox: live control, a working endpoint (objectui#11736)', () => { + it('an empty queue shows "No actions waiting" and the poll continues', async () => { + const fetchMock = stubFetch(() => answer(200, { items: [], total: 0 })); + render(); + await settle(); + + expect(screen.getByText(EMPTY_TITLE)).toBeTruthy(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await advance(3 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(4); + expect(screen.getByText(EMPTY_TITLE)).toBeTruthy(); + }); +}); diff --git a/packages/plugin-chatbot/src/__tests__/usePendingActions.pollFaultPolicy-11736.test.tsx b/packages/plugin-chatbot/src/__tests__/usePendingActions.pollFaultPolicy-11736.test.tsx new file mode 100644 index 0000000000..96189868ba --- /dev/null +++ b/packages/plugin-chatbot/src/__tests__/usePendingActions.pollFaultPolicy-11736.test.tsx @@ -0,0 +1,216 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `usePendingActions` polls according to what it was last answered + * (objectui#11736). + * + * The defect: the hook re-armed a fixed `setInterval` whatever the list read + * answered. On a deployment with no AI service, the open edition answers `501`, + * so the page asked the dead endpoint every five seconds for as long as it was + * open. The fault policy is documented on `ListReadOutcome` in + * `usePendingActions.ts`. These pins hold its three arms, and the live control + * the card asks for, by counting reads under fake timers: + * + * - refused (`501`, `404`, `403`, `401`): one read, then nothing, however long + * the page stays open; + * - transient (`5xx` other than `501`, `408`, `429`, no answer at all): the + * delay doubles per consecutive failure, up to a ceiling; + * - ok: a read every `pollInterval`, and a success after failures resets the + * cadence. This is the live control: a working endpoint keeps being polled. + * + * Counting reads, not timers: `vi.getTimerCount()` would also count whatever + * React or the DOM shim schedules, while the number of `fetch` calls is exactly + * the traffic the card is about. + */ + +import { describe, it, expect, afterEach, beforeEach, vi } from 'vitest'; +import { renderHook, act, cleanup } from '@testing-library/react'; +import { usePendingActions } from '../usePendingActions'; + +const POLL = 5_000; + +/** A `Response`-shaped answer: the hook reads `ok`, `status`, `statusText` and `json()`. */ +function answer(status: number, body: unknown): Response { + return { + ok: status >= 200 && status < 300, + status, + statusText: '', + json: async () => body, + } as unknown as Response; +} + +const EMPTY = { items: [], total: 0 }; +const NOT_IMPLEMENTED = { + success: false, + error: { code: 'NOT_IMPLEMENTED', message: 'The AI service is not available on this deployment.' }, +}; + +/** Stub `fetch` with a per-call responder; `n` is the 1-based call number. */ +function stubFetch(respond: (n: number) => Response | Promise) { + let n = 0; + const fetchMock = vi.fn(async () => respond(++n)); + vi.stubGlobal('fetch', fetchMock); + return fetchMock; +} + +const settle = () => act(async () => { await vi.advanceTimersByTimeAsync(0); }); +const advance = (ms: number) => act(async () => { await vi.advanceTimersByTimeAsync(ms); }); + +beforeEach(() => { + vi.useFakeTimers(); +}); +afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe('usePendingActions: a refused read stops the poll (objectui#11736)', () => { + it.each([501, 404, 403, 401])('a %i is read once, and not again across ten poll intervals', async (status) => { + const fetchMock = stubFetch(() => answer(status, NOT_IMPLEMENTED)); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result.current.error?.message).toBe(NOT_IMPLEMENTED.error.message); + + await advance(10 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result.current.error?.message).toBe(NOT_IMPLEMENTED.error.message); + }); + + it('a manual refresh that succeeds restarts the poll at its normal cadence', async () => { + const fetchMock = stubFetch((n) => (n === 1 ? answer(501, NOT_IMPLEMENTED) : answer(200, EMPTY))); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + await advance(3 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + + await act(async () => { await result.current.refresh(); }); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(result.current.error).toBeUndefined(); + + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(3); + }); + + it('a manual refresh that is refused again leaves the poll stopped', async () => { + const fetchMock = stubFetch(() => answer(501, NOT_IMPLEMENTED)); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + + await act(async () => { await result.current.refresh(); }); + expect(fetchMock).toHaveBeenCalledTimes(2); + + await advance(10 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('a change to the inputs starts the poll over, and is judged on its own answer', async () => { + const fetchMock = stubFetch(() => answer(501, NOT_IMPLEMENTED)); + const { rerender } = renderHook( + ({ status }: { status: 'pending' | 'all' }) => usePendingActions({ pollInterval: POLL, status }), + { initialProps: { status: 'pending' } }, + ); + await settle(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + rerender({ status: 'all' }); + await settle(); + expect(fetchMock).toHaveBeenCalledTimes(2); + + await advance(10 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); + +describe('usePendingActions: a transient failure backs the poll off, within a ceiling (objectui#11736)', () => { + it('doubles the delay per consecutive 503 and stops growing at the ceiling', async () => { + const fetchMock = stubFetch(() => answer(503, { error: { message: 'Service Unavailable' } })); + renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + // 5 s doubled per failure: 10 s, 20 s, 40 s, 80 s, then the 120 s ceiling. + const delays = [10_000, 20_000, 40_000, 80_000, 120_000, 120_000]; + let reads = 1; + for (const delay of delays) { + await advance(delay - 1); + expect(fetchMock).toHaveBeenCalledTimes(reads); + await advance(1); + reads += 1; + expect(fetchMock).toHaveBeenCalledTimes(reads); + } + }); + + it.each([500, 502, 504, 408, 429])('a %i backs off rather than stopping', async (status) => { + const fetchMock = stubFetch(() => answer(status, { error: { message: `status ${status}` } })); + renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('a read with no answer at all (fetch rejects) backs off rather than stopping', async () => { + const fetchMock = vi.fn(async () => { throw new TypeError('Failed to fetch'); }); + vi.stubGlobal('fetch', fetchMock); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + expect(result.current.error?.message).toBe('Failed to fetch'); + + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('a success after failures resets the cadence to pollInterval', async () => { + const fetchMock = stubFetch((n) => (n <= 2 ? answer(503, {}) : answer(200, EMPTY))); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + await advance(10_000); + expect(fetchMock).toHaveBeenCalledTimes(2); + await advance(20_000); + expect(fetchMock).toHaveBeenCalledTimes(3); + expect(result.current.error).toBeUndefined(); + + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(4); + }); +}); + +describe('usePendingActions: live control, a working endpoint keeps being polled (objectui#11736)', () => { + it('reads every pollInterval while the endpoint answers', async () => { + const fetchMock = stubFetch(() => answer(200, EMPTY)); + const { result } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + expect(fetchMock).toHaveBeenCalledTimes(1); + + for (let tick = 2; tick <= 6; tick += 1) { + await advance(POLL); + expect(fetchMock).toHaveBeenCalledTimes(tick); + } + expect(result.current.error).toBeUndefined(); + expect(result.current.items).toEqual([]); + }); + + it('an unmounted hook reads nothing more', async () => { + const fetchMock = stubFetch(() => answer(200, EMPTY)); + const { unmount } = renderHook(() => usePendingActions({ pollInterval: POLL })); + await settle(); + unmount(); + + await advance(10 * POLL); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/plugin-chatbot/src/usePendingActions.ts b/packages/plugin-chatbot/src/usePendingActions.ts index 8a03a0c02f..b1dec0e6e0 100644 --- a/packages/plugin-chatbot/src/usePendingActions.ts +++ b/packages/plugin-chatbot/src/usePendingActions.ts @@ -19,6 +19,12 @@ * `approve`/`reject` mutators that re-fetch on completion so consumers * don't need to micromanage state. * + * The poll follows the answer it last got (objectui#11736). It does not re-arm + * at a fixed interval no matter what: a refused read stops it, a transient + * failure backs it off, and a success resets it. The policy lives here, in the + * hook, so every caller gets the same behaviour. It is documented on + * `ListReadOutcome` below. + * * @module */ @@ -201,9 +207,100 @@ async function call( return body as T; } +/** + * The poll's one fault policy: what a single list read's answer does to the + * NEXT read (objectui#11736). + * + * - `ok`: the read answered. The next poll comes after `pollInterval`, and + * the backoff resets. + * - `transient`: there was no answer, or the answer means "try again later". + * That covers a transport failure (no HTTP status at all, such as a network + * error or an unreadable body), `408`, `429`, and every `5xx` except `501`. + * The poll backs off: each consecutive failure doubles the delay, up to + * `MAX_BACKOFF_MS`. + * - `refused`: an answer that repeating the same request cannot change. That + * covers `501 Not Implemented`, which the open edition answers on a + * deployment with no AI service, and every other HTTP status (`401`, `403`, + * `404`, ...). The poll STOPS. Two things restart it: a read that succeeds + * (a manual `refresh()`, or the re-fetch after `approve` / `reject`), or a + * change to the hook's inputs, which starts the poll over. + * + * Before objectui#11736 the poll was a `setInterval` that re-armed whatever + * the read answered. On a deployment without the AI service it asked a dead + * endpoint every five seconds for as long as the page stayed open. + */ +type ListReadOutcome = 'ok' | 'transient' | 'refused'; + +/** + * Ceiling for the transient-failure backoff, in ms. A caller whose own + * `pollInterval` is longer keeps that interval as the ceiling, so a backoff + * never polls faster than success does. The value was copied from + * `@object-ui/app-shell`'s shared user feeds (`MAX_BACKOFF_MS` in + * `sharedUserFeeds.ts`) when this was written. Nothing keeps the two equal. + */ +const MAX_BACKOFF_MS = 120_000; + +/** Classify a failed list read. `call()` puts the HTTP status on the error as `status`. */ +function classifyListFailure(err: Error): Exclude { + const status = (err as Error & { status?: unknown }).status; + if (typeof status !== 'number') return 'transient'; + if (status === 408 || status === 429) return 'transient'; + if (status >= 500 && status !== 501) return 'transient'; + return 'refused'; +} + +/** + * The poll's schedule. It is held in a ref, and the timer is a `setTimeout` + * chain armed after each read settles, not a `setInterval`. + */ +interface PollSchedule { + /** + * Bumped by every start and every teardown of the poll effect. A read + * schedules the next one only if the generation it started in is still + * current, so a read that outlives an unmount or an input change re-arms + * nothing. + */ + generation: number; + /** The success cadence in ms. `0` means the hook is not polling. */ + interval: number; + /** Consecutive transient failures: the backoff exponent. */ + failures: number; + timer: ReturnType | undefined; +} + +/** + * Re-arm (or stop) the poll from a read that has just settled. Every list + * read comes through here: the poll's own tick, the initial read, a manual + * `refresh()`, and the re-fetch after a decision. So there is one timer at a + * time, and the policy cannot be skipped by reading through a different door. + */ +function settlePoll( + poll: PollSchedule, + generation: number, + outcome: ListReadOutcome, + read: () => Promise, +): void { + if (generation !== poll.generation || poll.interval <= 0) return; + clearTimeout(poll.timer); + poll.timer = undefined; + if (outcome === 'refused') return; + poll.failures = outcome === 'ok' ? 0 : poll.failures + 1; + const ceiling = Math.max(poll.interval, MAX_BACKOFF_MS); + const delay = Math.min(poll.interval * 2 ** poll.failures, ceiling); + poll.timer = setTimeout(() => { + poll.timer = undefined; + void read(); + }, delay); +} + /** * Hook that drives the HITL pending-actions inbox. * + * Polls every `pollInterval` while the endpoint answers. A refused read + * (`501`, `403`, `404`, any `4xx` other than `408` / `429`) stops the poll. A + * transient failure backs it off, doubling up to a ceiling. A read that + * succeeds resumes the normal cadence (objectui#11736; see `ListReadOutcome`). + * * @example * ```tsx * const { items, isLoading, error, approve, reject, refresh } = @@ -237,10 +334,22 @@ export function usePendingActions( const cfgRef = React.useRef({ apiBase, status, conversationId, limit, headers }); cfgRef.current = { apiBase, status, conversationId, limit, headers }; - const refresh = React.useCallback(async () => { + const pollRef = React.useRef({ + generation: 0, + interval: 0, + failures: 0, + timer: undefined, + }); + + // A named function expression: the poll's timer calls `load` itself, so the + // schedule never has to reach the fetch through a memoised identity. + const refresh = React.useCallback(async function load(): Promise { + const poll = pollRef.current; + const generation = poll.generation; const { apiBase, status, conversationId, limit, headers } = cfgRef.current; setIsLoading(true); setError(undefined); + let outcome: ListReadOutcome = 'ok'; try { const url = buildUrl(apiBase, '/pending-actions', { status: status && status !== 'all' ? status : undefined, @@ -255,12 +364,22 @@ export function usePendingActions( setItems(Array.isArray(out.items) ? out.items : []); setTotal(typeof out.total === 'number' ? out.total : (out.items?.length ?? 0)); } catch (err) { - setError(err instanceof Error ? err : new Error(String(err))); + const failure = err instanceof Error ? err : new Error(String(err)); + setError(failure); + outcome = classifyListFailure(failure); } finally { setIsLoading(false); } + settlePoll(poll, generation, outcome, load); }, []); + // The effect below reaches `refresh` through this ref and keys only on the + // primitives that define the poll. It must not key on `refresh`'s identity: + // `useCallback` is a performance hint that React may discard (AGENTS.md + // commandment #10, objectui#8640). + const refreshRef = React.useRef(refresh); + refreshRef.current = refresh; + const approve = React.useCallback(async (id: string): Promise => { const { apiBase, headers } = cfgRef.current; const url = buildUrl(apiBase, `/pending-actions/${encodeURIComponent(id)}/approve`); @@ -287,14 +406,22 @@ export function usePendingActions( } }, [refresh]); - // Initial fetch + polling. + // Initial fetch + polling. Each read re-arms the next one through + // `settlePoll`, according to what it was answered. React.useEffect(() => { if (!enabled) return; - void refresh(); - if (!pollInterval || pollInterval <= 0) return; - const id = setInterval(() => { void refresh(); }, pollInterval); - return () => clearInterval(id); - }, [enabled, pollInterval, refresh, apiBase, status, conversationId, limit]); + const poll = pollRef.current; + poll.generation += 1; + poll.interval = pollInterval > 0 ? pollInterval : 0; + poll.failures = 0; + void refreshRef.current(); + return () => { + poll.generation += 1; + poll.interval = 0; + clearTimeout(poll.timer); + poll.timer = undefined; + }; + }, [enabled, pollInterval, apiBase, status, conversationId, limit]); return { items, total, isLoading, error, refresh, approve, reject }; }