diff --git a/hub/src/web/routes/guards.ts b/hub/src/web/routes/guards.ts index 0e17ec077..9056111d6 100644 --- a/hub/src/web/routes/guards.ts +++ b/hub/src/web/routes/guards.ts @@ -27,7 +27,11 @@ export function requireSession( return c.json({ error }, status) } if (options?.requireActive && !access.session.active) { - return c.json({ error: 'Session is inactive' }, 409) + // `code` lets the web client discriminate the inactive-session 409 from + // other 4xx without string-matching the human message (which is i18n'd + // by the consumer and may change). See web onError handler in + // router.tsx which surfaces a Reopen affordance on this code. + return c.json({ error: 'Session is inactive', code: 'session_inactive' }, 409) } return { sessionId: access.sessionId, session: access.session } } diff --git a/hub/src/web/routes/messages.test.ts b/hub/src/web/routes/messages.test.ts index fbf12f1e6..64248810f 100644 --- a/hub/src/web/routes/messages.test.ts +++ b/hub/src/web/routes/messages.test.ts @@ -217,3 +217,27 @@ describe('POST /api/sessions/:id/messages — scheduledAt + attachments rejected expect(sentMessages).toHaveLength(1) }) }) + +// --------------------------------------------------------------------------- +// #918: inactive session 409 carries a machine-readable code +// --------------------------------------------------------------------------- + +describe('POST /api/sessions/:id/messages — inactive session response shape', () => { + it('returns 409 with code "session_inactive" when sending to an inactive session', async () => { + const { app, sentMessages } = createApp({ active: false }) + + const response = await app.request('/api/sessions/session-1/messages', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ text: 'hello', localId: 'local-inactive' }) + }) + + expect(response.status).toBe(409) + const body = await response.json() as { error: string; code: string } + expect(body.error).toBe('Session is inactive') + // Web client discriminates this branch via `code` without string-matching + // the human message; see useSendMessage onError consumer in router.tsx. + expect(body.code).toBe('session_inactive') + expect(sentMessages).toHaveLength(0) + }) +}) diff --git a/web/src/components/AssistantChat/HappyComposer.tsx b/web/src/components/AssistantChat/HappyComposer.tsx index d0bf46345..1d2f78386 100644 --- a/web/src/components/AssistantChat/HappyComposer.tsx +++ b/web/src/components/AssistantChat/HappyComposer.tsx @@ -51,6 +51,10 @@ export interface TextInputState { * or null for an immediate send. When non-null, the composer also * restores the schedule via `onSchedule` so the operator can edit and * retry without silently downgrading a scheduled send to immediate. + * - `action` is an optional recovery affordance rendered as a button next + * to the message. Used by the inactive-session branch (#918) to expose + * a one-click Reopen. Other failure modes (5xx, network, generic 4xx) + * leave this null and only render the message. * * Owned by the route component (`router.tsx`); the composer is a pure * consumer that: @@ -63,6 +67,11 @@ export type ComposerSendError = { text: string message: string scheduledAt: number | null + action?: { + label: string + onClick: () => void + pending?: boolean + } | null } const defaultSuggestionHandler = async (): Promise => [] @@ -983,9 +992,20 @@ export function HappyComposer(props: {
- {sendError.message} + {sendError.message} + {sendError.action ? ( + + ) : null}
) : null} diff --git a/web/src/hooks/mutations/useSendMessage.test.tsx b/web/src/hooks/mutations/useSendMessage.test.tsx index 9187bce44..084f06a28 100644 --- a/web/src/hooks/mutations/useSendMessage.test.tsx +++ b/web/src/hooks/mutations/useSendMessage.test.tsx @@ -3,7 +3,7 @@ import { renderHook, act, waitFor } from '@testing-library/react' import { QueryClient, QueryClientProvider } from '@tanstack/react-query' import type { ReactNode } from 'react' import { useSendMessage } from './useSendMessage' -import type { ApiClient } from '@/api/client' +import { ApiError, type ApiClient } from '@/api/client' vi.mock('@/lib/message-window-store', () => ({ appendOptimisticMessage: vi.fn(), @@ -523,6 +523,114 @@ describe('useSendMessage', () => { await expect(acceptedPromise!).resolves.toBe(true) }) + // #918: the inactive-session 409 path + describe('inactive-session 409 (issue #918)', () => { + it('fires onError with the ApiError so the consumer can render a session_inactive affordance', async () => { + // Hub returns 409 with code: 'session_inactive' (guards.ts). + // The api client throws ApiError(status=409, code='session_inactive'). + const onError = vi.fn() + const api = createMockApi(async () => { + throw new ApiError( + 'HTTP 409 Conflict: {"error":"Session is inactive","code":"session_inactive"}', + 409, + 'session_inactive', + '{"error":"Session is inactive","code":"session_inactive"}' + ) + }) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('hello inactive') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown; sessionId: string } + expect(info.text).toBe('hello inactive') + expect(info.sessionId).toBe('session-A') + expect(info.error).toBeInstanceOf(ApiError) + const apiErr = info.error as ApiError + expect(apiErr.status).toBe(409) + expect(apiErr.code).toBe('session_inactive') + }) + + it('fires onError when resolveSessionId rejects (pre-mutation inactive-session failure)', async () => { + // Pre-mutation: the route's resolveSessionId throws when + // inactiveSessionCanResume returns false OR api.resumeSession + // fails. Prior to #918 this dropped the typed text into the + // void with only a console.error; the operator saw nothing. + // The hook must surface this through onError too. + const onError = vi.fn() + const api = createMockApi() + const resumeError = new ApiError('Session is inactive', 409, 'session_inactive') + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { + onError, + resolveSessionId: async () => { throw resumeError }, + onSessionResolved: vi.fn(), + }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('hello pre-mutation') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + const info = onError.mock.calls[0][0] as { text: string; error: unknown; sessionId: string } + expect(info.text).toBe('hello pre-mutation') + // Keyed by the ORIGINAL sessionId: pre-mutation never navigated. + expect(info.sessionId).toBe('session-A') + expect(info.error).toBe(resumeError) + }) + + it('5xx still uses the legacy text-restore path (#918 must not regress transient-failure UX)', async () => { + // Acceptance criterion: a real transient 500/network failure + // must keep the original behavior (remove optimistic row, + // onError fires with the plain message), not adopt the + // session_inactive affordance. + const onError = vi.fn() + const api = createMockApi(async () => { + throw new ApiError( + 'HTTP 500 Internal Server Error', + 500, + undefined, + undefined + ) + }) + + const { removeOptimisticMessage } = await import('@/lib/message-window-store') + const removeMock = vi.mocked(removeOptimisticMessage) + + const { result } = renderHook( + () => useSendMessage(api, 'session-A', { onError }), + { wrapper: createWrapper() }, + ) + + act(() => { + result.current.sendMessage('hello transient') + }) + + await waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1) + }) + expect(removeMock).toHaveBeenCalledWith('session-A', 'local-id-1') + const info = onError.mock.calls[0][0] as { error: unknown } + // No session_inactive code -> consumer renders fallback + // message, no Reopen action attached. + expect((info.error as ApiError).code).toBeUndefined() + expect((info.error as ApiError).status).toBe(500) + }) + }) + it('preserves scheduledAt when retrying a failed scheduled message', async () => { const sendMock = vi.fn(async () => {}) const api = createMockApi(sendMock) diff --git a/web/src/hooks/mutations/useSendMessage.ts b/web/src/hooks/mutations/useSendMessage.ts index 2f6c77ff4..c37d38742 100644 --- a/web/src/hooks/mutations/useSendMessage.ts +++ b/web/src/hooks/mutations/useSendMessage.ts @@ -235,6 +235,22 @@ export function useSendMessage( } catch (error) { haptic.notification('error') console.error('Failed to resolve session before send:', error) + // #918: surface the failure via onError so the route can render + // an inline affordance instead of silently swallowing the + // typed text. This covers the "no resume target" branch + // (inactiveSessionCanResume === false) and also any failure + // from api.resumeSession itself. The mutation never started + // (no optimistic row to clean up); onError is the only + // visibility hook the consumer has for this pre-mutation + // path. Key by the ORIGINAL sessionId because navigation + // hasn't happened yet -- the operator is still on the + // archived session's route. + options?.onError?.({ + sessionId, + text, + error, + scheduledAt: scheduledAt ?? null + }) return false } finally { resolveGuardRef.current = false diff --git a/web/src/lib/locales/en.ts b/web/src/lib/locales/en.ts index 5a0aba4e8..390303b80 100644 --- a/web/src/lib/locales/en.ts +++ b/web/src/lib/locales/en.ts @@ -240,6 +240,8 @@ export default { 'chat.terminal': 'Terminal', 'chat.switchRemote': 'Switch to remote mode', 'chat.sendError.fallback': "Couldn't send your message. Edit and try again.", + 'chat.sendError.sessionInactive': 'This session is archived. Reopen it to send your message.', + 'chat.sendError.sessionInactive.action': 'Reopen', // Codex review 'codexReview.title': 'Codex review', diff --git a/web/src/lib/locales/zh-CN.ts b/web/src/lib/locales/zh-CN.ts index d3cecec50..4f4353827 100644 --- a/web/src/lib/locales/zh-CN.ts +++ b/web/src/lib/locales/zh-CN.ts @@ -244,6 +244,8 @@ export default { 'chat.terminal': '终端', 'chat.switchRemote': '切换到远程模式', 'chat.sendError.fallback': '消息未能发送。请修改后重试。', + 'chat.sendError.sessionInactive': '此会话已归档。请先重新打开再发送消息。', + 'chat.sendError.sessionInactive.action': '重新打开', // Codex review 'codexReview.title': 'Codex review', diff --git a/web/src/router.tsx b/web/src/router.tsx index 84e030a53..01682fa3d 100644 --- a/web/src/router.tsx +++ b/web/src/router.tsx @@ -32,6 +32,7 @@ import { useSlashCommands } from '@/hooks/queries/useSlashCommands' import { useSkills } from '@/hooks/queries/useSkills' import { useSendMessage, type SendErrorInfo } from '@/hooks/mutations/useSendMessage' import type { ComposerSendError } from '@/components/AssistantChat/HappyComposer' +import { ApiError } from '@/api/client' import { queryKeys } from '@/lib/query-keys' import { useToast } from '@/lib/toast-context' import { useTranslation } from '@/lib/use-translation' @@ -574,20 +575,27 @@ function SessionsIndexPage() { } /** - * Extract a user-facing message from a thrown send error. - * `request` in the api client throws plain `Error` for !res.ok, with the - * format `"HTTP : "` -- we surface the message as - * a single line and fall back to a localized default when nothing usable is - * present (e.g. an aborted fetch that resolved with no message). + * Classify a thrown send error into a {message, code} pair the composer can + * render. `code` lets the consumer attach a recovery affordance (Reopen on + * `session_inactive`) without re-inspecting the raw error. + * + * `request` in the api client throws `ApiError` for !res.ok with `status` + * and `code` parsed from the JSON body. Older / non-JSON failures arrive as + * plain `Error`; we surface those by their message verbatim, falling back to + * a localized default when nothing usable is present (e.g. an aborted fetch + * that resolved with no message). */ -function deriveSendErrorMessage( +function classifySendError( error: unknown, t: (key: string) => string, -): string { +): { message: string; code: string | null } { + if (error instanceof ApiError && error.status === 409 && error.code === 'session_inactive') { + return { message: t('chat.sendError.sessionInactive'), code: 'session_inactive' } + } if (error instanceof Error && error.message) { - return error.message + return { message: error.message, code: null } } - return t('chat.sendError.fallback') + return { message: t('chat.sendError.fallback'), code: null } } function SessionPage() { @@ -629,9 +637,22 @@ function SessionPage() { // text into the OLD session's composer and the next render would clear // it. The bumped `id` still lets the composer dedupe restorations of // identical text. - const [sendErrors, setSendErrors] = useState>({}) + // + // We persist the classifier `code` (not the bound action) so the + // composer-visible action stays reactive to `reopeningSessionId` state + // changes -- the action is built fresh on each render from {raw error + // record} x {current reopen state}. See classifySendError + the + // Reopen affordance below. + type RawSendError = { + id: number + text: string + message: string + code: string | null + scheduledAt: number | null + } + const [sendErrors, setSendErrors] = useState>({}) + const [reopeningSessionId, setReopeningSessionId] = useState(null) const sendErrorIdRef = useRef(0) - const sendError = sendErrors[sessionId] ?? null const clearSendError = useCallback(() => { setSendErrors((prev) => { if (!(sessionId in prev)) return prev @@ -641,6 +662,67 @@ function SessionPage() { }) }, [sessionId]) + // Reopen recovery (#918): one-click affordance attached to the inline + // composer error when the rejected send was inactive-session. Mirrors + // SessionList's Reopen UX -- POST /sessions/:id/reopen via + // api.reopenSession -- so the operator's mental model is consistent + // across surfaces. We do NOT auto-replay the send: per #917 the reopen + // path has known fragility, so the operator re-clicks Send on the + // restored composer text once Reopen lands. + const reopenFromErrorAffordance = useCallback((errorSessionId: string) => { + if (!api) return + setReopeningSessionId((prev) => prev ?? errorSessionId) + void (async () => { + try { + const result = await api.reopenSession(errorSessionId) + // Clear the inline error -- the operator now has a live + // session to retry against. + setSendErrors((prev) => { + if (!(errorSessionId in prev)) return prev + const next = { ...prev } + delete next[errorSessionId] + return next + }) + await queryClient.invalidateQueries({ queryKey: queryKeys.session(result.sessionId) }) + await queryClient.invalidateQueries({ queryKey: queryKeys.sessions }) + if (result.sessionId && result.sessionId !== errorSessionId) { + navigate({ + to: '/sessions/$sessionId', + params: { sessionId: result.sessionId }, + replace: true + }) + } + } catch (err) { + const message = err instanceof Error ? err.message : t('dialog.error.default') + addToast({ + title: t('resume.failed.title'), + body: message, + sessionId: errorSessionId, + url: '' + }) + } finally { + setReopeningSessionId(null) + } + })() + }, [api, queryClient, navigate, addToast, t]) + + const rawSendError = sendErrors[sessionId] ?? null + const sendError: ComposerSendError | null = rawSendError + ? { + id: rawSendError.id, + text: rawSendError.text, + message: rawSendError.message, + scheduledAt: rawSendError.scheduledAt, + action: rawSendError.code === 'session_inactive' + ? { + label: t('chat.sendError.sessionInactive.action'), + onClick: () => reopenFromErrorAffordance(sessionId), + pending: reopeningSessionId === sessionId + } + : null + } + : null + const { sendMessage, retryMessage, @@ -662,12 +744,14 @@ function SessionPage() { }, onError: (info: SendErrorInfo) => { sendErrorIdRef.current += 1 + const { message, code } = classifySendError(info.error, t) setSendErrors((prev) => ({ ...prev, [info.sessionId]: { id: sendErrorIdRef.current, text: info.text, - message: deriveSendErrorMessage(info.error, t), + message, + code, scheduledAt: info.scheduledAt } })) @@ -677,7 +761,15 @@ function SessionPage() { return currentSessionId } if (!inactiveSessionCanResume(session, messages.length)) { - throw new Error(t('resume.unavailable.noTarget')) + // #918: surface as a session_inactive ApiError so the + // onError consumer's classifier renders the Reopen + // affordance. `status: 409` mirrors the hub guard for + // structural parity; no HTTP call was made. + throw new ApiError( + t('chat.sendError.sessionInactive'), + 409, + 'session_inactive', + ) } try { return await api.resumeSession(currentSessionId, { permissionMode: session.permissionMode ?? undefined }) @@ -689,7 +781,14 @@ function SessionPage() { sessionId: currentSessionId, url: '' }) - throw error + // Rebrand as a session_inactive ApiError so the inline + // affordance offers Reopen (a separate code path from the + // failed Resume) and the operator has a recovery click. + throw new ApiError( + t('chat.sendError.sessionInactive'), + 409, + 'session_inactive', + ) } }, onSessionResolved: (resolvedSessionId) => {