mirror of
https://github.com/wu736139669/hapi.git
synced 2026-08-05 06:24:37 +00:00
Sending text via the web composer to an archived/inactive session
silently dropped on the floor: the hub returned 409 but the web client
swallowed the failure with a console.error in the resolveSessionId catch
branch, leaving the operator with no signal and no recovery path.
Hub: add a machine-readable `code: 'session_inactive'` to the 409 body
so the web client can discriminate this branch without string-matching
the i18n-able human message.
Web (router.tsx, useSendMessage.ts, HappyComposer.tsx):
- useSendMessage now fires `onError` on resolveSessionId rejection,
not just on POST /messages failure -- closes the visibility hole
when the inactive session has no resume target or resume itself
fails.
- The route classifies the thrown error: a 409 + session_inactive
code or a synthetic ApiError thrown from resolveSessionId attaches
a Reopen action to the existing inline composer-error affordance.
Plain 4xx / 5xx / network keep the legacy text-restore UX
untouched.
- Reopen calls api.reopenSession (the same path as SessionList's
Reopen menu item), invalidates the session queries, and navigates
to the resumed sessionId. Per the orchestrator brief's friction
pass on #917 the affordance does NOT auto-replay the send; the
operator re-clicks Send on the restored composer text.
Tests:
- hub messages.test.ts: 409 carries `code: 'session_inactive'`.
- useSendMessage.test.tsx: ApiError(409, session_inactive) from POST
flows through onError; resolveSessionId rejection flows through
onError keyed by the original sessionId; 500 keeps the legacy
fallback path with no code attached.
AI disclosure: implemented by an AI agent (Claude Opus 4.7) acting on
operator instructions; tests pass locally (bun typecheck + bun run
test for hub and web).
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
HeavyGee
@@ -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 }
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -56,6 +56,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:
|
||||
@@ -68,6 +72,11 @@ export type ComposerSendError = {
|
||||
text: string
|
||||
message: string
|
||||
scheduledAt: number | null
|
||||
action?: {
|
||||
label: string
|
||||
onClick: () => void
|
||||
pending?: boolean
|
||||
} | null
|
||||
}
|
||||
|
||||
const defaultSuggestionHandler = async (): Promise<Suggestion[]> => []
|
||||
@@ -1190,9 +1199,20 @@ export function HappyComposer(props: {
|
||||
<div
|
||||
role="alert"
|
||||
data-testid="composer-send-error"
|
||||
className="mb-2 rounded-md bg-[var(--app-subtle-bg)] px-3 py-2 text-sm text-red-600"
|
||||
className="mb-2 flex items-center justify-between gap-3 rounded-md bg-[var(--app-subtle-bg)] px-3 py-2 text-sm text-red-600"
|
||||
>
|
||||
{sendError.message}
|
||||
<span className="flex-1">{sendError.message}</span>
|
||||
{sendError.action ? (
|
||||
<button
|
||||
type="button"
|
||||
data-testid="composer-send-error-action"
|
||||
onClick={sendError.action.onClick}
|
||||
disabled={sendError.action.pending}
|
||||
className="shrink-0 rounded-md border border-red-300 px-2 py-1 text-xs font-medium text-red-700 hover:bg-red-50 disabled:cursor-not-allowed disabled:opacity-60"
|
||||
>
|
||||
{sendError.action.label}
|
||||
</button>
|
||||
) : null}
|
||||
</div>
|
||||
) : null}
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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',
|
||||
|
||||
+114
-15
@@ -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<T>` in the api client throws plain `Error` for !res.ok, with the
|
||||
* format `"HTTP <status> <statusText>: <body>"` -- 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<T>` 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 {
|
||||
if (error instanceof Error && error.message) {
|
||||
return error.message
|
||||
): { 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' }
|
||||
}
|
||||
return t('chat.sendError.fallback')
|
||||
if (error instanceof Error && error.message) {
|
||||
return { message: error.message, code: null }
|
||||
}
|
||||
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<Record<string, ComposerSendError>>({})
|
||||
//
|
||||
// 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<Record<string, RawSendError>>({})
|
||||
const [reopeningSessionId, setReopeningSessionId] = useState<string | null>(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) => {
|
||||
|
||||
Reference in New Issue
Block a user