Skip to content

Commit 80ee657

Browse files
committed
fix(mothership): keep any queued send the server may already hold from being edited
#8717 blocked edits only for a first message the unmount queued itself. A send handed to another surface, or re-queued after its dispatch got no answer, also carries an earlier attempt's id and was still editable; an edit dropped that id and could send a second message. The queue store now marks every message resuming an earlier attempt as admissionUnknown unless the writer knows the server refused it or never got it (a busy refusal, a superseded 409, a Stop that never settled), which also makes a held message editable again once the server refuses it.
1 parent 988b079 commit 80ee657

5 files changed

Lines changed: 203 additions & 13 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx‎

Lines changed: 153 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2151,11 +2151,6 @@ describe('useChat remount send recovery', () => {
21512151
expect(allQueuedMessages()).toHaveLength(0)
21522152
})
21532153

2154-
/**
2155-
* After a Stop of the first message, only the follow-up was the user's
2156-
* intent: the Stop's POST is left to the server, nothing withdraws it, and the
2157-
* next mount sends just the follow-up, once.
2158-
*/
21592154
/**
21602155
* A first message held at the queue head after a remount may already be a
21612156
* turn on the server. Editing it would send different text under a new id,
@@ -2204,6 +2199,159 @@ describe('useChat remount send recovery', () => {
22042199
})
22052200
})
22062201

2202+
/** A chat with a turn running, so anything sent to it waits in its queue. */
2203+
function renderBusyChat(id: string) {
2204+
const history: MothershipChatHistory = {
2205+
id,
2206+
mode: 'agent',
2207+
title: 'Busy',
2208+
messages: [],
2209+
activeStreamId: 'turn-still-running',
2210+
resources: [],
2211+
}
2212+
mockRequestJson.mockImplementation(() => Promise.resolve({ chat: history }))
2213+
vi.stubGlobal('fetch', async (input: RequestInfo | URL, init?: RequestInit) => {
2214+
if (String(input).includes('/api/mothership/chat/stream')) {
2215+
if (String(input).includes('batch=true')) {
2216+
return Response.json({ success: true, events: [], status: 'streaming' })
2217+
}
2218+
return new Response(new ReadableStream<Uint8Array>(), {
2219+
headers: { 'Content-Type': 'text/event-stream' },
2220+
})
2221+
}
2222+
return fetchStub(input, init)
2223+
})
2224+
return { history, ...renderUseChatInChat(id, history) }
2225+
}
2226+
2227+
/**
2228+
* A send another surface withdrew arrives here under its original id, through
2229+
* the send event or the stored handoff. Queued behind a running turn, it may
2230+
* already be a turn on the server, so it can't be edited either.
2231+
*/
2232+
it('does not let a withdrawn send handed to a busy chat be edited', async () => {
2233+
const { history, getResult } = renderBusyChat('chat-busy-on-handoff')
2234+
await waitFor(() => getResult().isSending)
2235+
await act(async () => {
2236+
await getResult().sendMessage('handed over from another surface', undefined, undefined, {
2237+
resumeUserMessageId: 'withdrawn-attempt',
2238+
})
2239+
})
2240+
const queued = useMothershipQueueStore.getState().queues[history.id]?.[0]
2241+
expect(queued?.resumeUserMessageId).toBe('withdrawn-attempt')
2242+
2243+
let edited: ReturnType<ReturnType<typeof useChat>['editQueuedMessage']>
2244+
await act(async () => {
2245+
edited = getResult().editQueuedMessage(queued?.id ?? '')
2246+
})
2247+
2248+
expect(edited).toBeUndefined()
2249+
expect(getResult().editingQueuedId).toBeNull()
2250+
})
2251+
2252+
/** A follow-up whose dispatch got no answer may have reached the server too. */
2253+
it('does not let a queued follow-up be edited after its send got no answer', async () => {
2254+
const history: MothershipChatHistory = {
2255+
id: 'chat-follow-up-unanswered',
2256+
mode: 'agent',
2257+
title: 'Unanswered',
2258+
messages: [],
2259+
activeStreamId: null,
2260+
resources: [],
2261+
}
2262+
mockRequestJson.mockImplementation(() => Promise.resolve({ chat: history }))
2263+
vi.stubGlobal('fetch', async (input: RequestInfo | URL, init?: RequestInit) => {
2264+
if (String(input) === '/api/mothership/chat' && init?.method === 'POST') {
2265+
state.postBodies.push(JSON.parse(String(init.body)))
2266+
throw new TypeError('Failed to fetch')
2267+
}
2268+
return fetchStub(input, init)
2269+
})
2270+
useMothershipQueueStore
2271+
.getState()
2272+
.enqueue(history.id, { id: 'follow-up', content: 'and the second invoice' })
2273+
const { getResult } = renderUseChatInChat(history.id, history)
2274+
await waitFor(
2275+
() =>
2276+
useMothershipQueueStore.getState().queues[history.id]?.[0]?.resumeUserMessageId !==
2277+
undefined
2278+
)
2279+
2280+
let edited: ReturnType<ReturnType<typeof useChat>['editQueuedMessage']>
2281+
await act(async () => {
2282+
edited = getResult().editQueuedMessage('follow-up')
2283+
})
2284+
2285+
expect(edited).toBeUndefined()
2286+
expect(useMothershipQueueStore.getState().queues[history.id]?.[0]).toMatchObject({
2287+
content: 'and the second invoice',
2288+
resumeUserMessageId: state.postBodies[0].userMessageId,
2289+
})
2290+
})
2291+
2292+
/**
2293+
* A held message the server then refuses as busy is known not to be a turn
2294+
* there: the server answers a retry of an admitted id as a duplicate, never
2295+
* as busy. The user can edit it again.
2296+
*/
2297+
it('lets a held message be edited again once the server refuses it as busy', async () => {
2298+
const history: MothershipChatHistory = {
2299+
id: 'chat-held-then-refused',
2300+
mode: 'agent',
2301+
title: 'Refused',
2302+
messages: [],
2303+
activeStreamId: null,
2304+
resources: [],
2305+
}
2306+
mockRequestJson.mockImplementation(() => Promise.resolve({ chat: history }))
2307+
vi.stubGlobal('fetch', async (input: RequestInfo | URL, init?: RequestInit) => {
2308+
const url = String(input)
2309+
if (url === '/api/mothership/chat' && init?.method === 'POST') {
2310+
state.postBodies.push(JSON.parse(String(init.body)))
2311+
return Response.json(
2312+
{
2313+
error: 'A response is already in progress for this chat.',
2314+
activeStreamId: 'turn-from-another-tab',
2315+
},
2316+
{ status: 409 }
2317+
)
2318+
}
2319+
if (url.includes('/api/mothership/chat/stream')) {
2320+
if (url.includes('batch=true')) {
2321+
return Response.json({ success: true, events: [], status: 'streaming' })
2322+
}
2323+
return new Response(new ReadableStream<Uint8Array>(), {
2324+
headers: { 'Content-Type': 'text/event-stream' },
2325+
})
2326+
}
2327+
return fetchStub(input, init)
2328+
})
2329+
useMothershipQueueStore.getState().enqueue(history.id, {
2330+
id: 'held-first',
2331+
content: 'inspect the workspace',
2332+
resumeUserMessageId: 'first-attempt',
2333+
admissionUnknown: true,
2334+
})
2335+
const { getResult } = renderUseChatInChat(history.id, history)
2336+
await waitFor(() => state.postBodies.length === 1)
2337+
await waitFor(
2338+
() => useMothershipQueueStore.getState().queues[history.id]?.[0]?.id === 'held-first'
2339+
)
2340+
2341+
let edited: ReturnType<ReturnType<typeof useChat>['editQueuedMessage']>
2342+
await act(async () => {
2343+
edited = getResult().editQueuedMessage('held-first')
2344+
})
2345+
2346+
expect(edited?.content).toBe('inspect the workspace')
2347+
expect(getResult().editingQueuedId).toBe('held-first')
2348+
})
2349+
2350+
/**
2351+
* After a Stop of the first message, only the follow-up was the user's
2352+
* intent: the Stop's POST is left to the server, nothing withdraws it, and the
2353+
* next mount sends just the follow-up, once.
2354+
*/
22072355
it('sends only the follow-up after a failed Stop when the new-chat surface remounts', async () => {
22082356
stubFirstPostPendingThenAdmitted()
22092357
const first = renderHomeLikeSurface()

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,11 @@ interface WithdrawnSendResult {
234234
busy?: boolean
235235
/** Not sent at all (its Stop handoff failed); kept queued for the user to send. */
236236
held?: boolean
237+
/**
238+
* The server is known not to have it: it was never sent, or the server
239+
* refused it outright. Its queue entry can be edited.
240+
*/
241+
notAdmitted?: boolean
237242
}
238243

239244
/**
@@ -3887,7 +3892,7 @@ export function useChat(
38873892
setError(getErrorMessage(err, 'Failed to stop the previous response'))
38883893
/* Nothing was sent. Hand the message back so it stays in its chat's queue
38893894
even if the user has switched chats since the Stop began. */
3890-
return { userMessageId, held: true }
3895+
return { userMessageId, held: true, notAdmitted: true }
38913896
}
38923897
}
38933898

@@ -4004,7 +4009,7 @@ export function useChat(
40044009
}
40054010
if (viewOnSend)
40064011
setError('Previous response is still shutting down; queued message was restored.')
4007-
return { userMessageId, held: true }
4012+
return { userMessageId, held: true, notAdmitted: true }
40084013
}
40094014
/** Withdraws this refused send so the queue retries it, under the same id, later. */
40104015
const releaseRefusedSend = () => {
@@ -4040,7 +4045,7 @@ export function useChat(
40404045
exact: true,
40414046
refetchType: 'none',
40424047
})
4043-
return { userMessageId, busy: true }
4048+
return { userMessageId, busy: true, notAdmitted: true }
40444049
}
40454050
/* "Already sent" with no stream for it means the earlier attempt is still
40464051
in flight on the server (or died before starting a turn), not that a turn
@@ -4414,6 +4419,7 @@ export function useChat(
44144419
: {}),
44154420
...(result.held ? { retryRequired: true } : {}),
44164421
...(result.busy ? busyRetry(1) : {}),
4422+
...(result.notAdmitted ? { admissionUnknown: false } : {}),
44174423
...((result.unreachable || result.busy) && activeChatKey.startsWith(PENDING_CHAT_KEY_PREFIX)
44184424
? { heldSurface: heldSendSurface }
44194425
: {}),
@@ -5043,6 +5049,8 @@ export function useChat(
50435049
? { heldSurface: heldSendSurface }
50445050
: {}),
50455051
...(withdrawnUserMessageId ? { resumeUserMessageId: withdrawnUserMessageId } : {}),
5052+
/** This attempt's outcome decides; an earlier refusal says nothing about it. */
5053+
...(withdrawn ? { admissionUnknown: !withdrawn.notAdmitted } : {}),
50465054
})
50475055
}
50485056

‎apps/sim/app/workspace/[workspaceId]/home/types.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,10 @@ export interface QueuedMessage {
3535
assistantSearch?: WorkspaceSearchFilters
3636
assistantSearchLevel?: AssistantSearchLevel
3737
/**
38-
* A first message withdrawn before the server answered. The server may
39-
* already hold it as sent, so it goes out exactly as written, under its
40-
* original id, and cannot be edited into a different message.
38+
* An earlier attempt at this message got no answer, so the server may
39+
* already hold it as sent. It goes out exactly as written, under that
40+
* attempt's id, and cannot be edited into a different message. False once
41+
* the server is known not to have it (it refused it, or it was never sent).
4142
*/
4243
admissionUnknown?: boolean
4344
}

‎apps/sim/stores/mothership-queue/store.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,24 @@ describe('useMothershipQueueStore', () => {
3030
})
3131

3232
describe('replaceAt', () => {
33+
it('treats any message resuming an earlier attempt as possibly sent, unless told otherwise', () => {
34+
useMothershipQueueStore
35+
.getState()
36+
.enqueue('chat-A', { id: 'resumed', content: 'original', resumeUserMessageId: 'attempt-1' })
37+
useMothershipQueueStore.getState().insertAt('chat-A', 0, {
38+
id: 'refused',
39+
content: 'original',
40+
resumeUserMessageId: 'attempt-2',
41+
admissionUnknown: false,
42+
})
43+
useMothershipQueueStore.getState().replaceAt('chat-A', 'resumed', { content: 'edited' })
44+
useMothershipQueueStore.getState().replaceAt('chat-A', 'refused', { content: 'edited' })
45+
46+
const [refused, resumed] = useMothershipQueueStore.getState().queues['chat-A'] ?? []
47+
expect(resumed).toMatchObject({ content: 'original', resumeUserMessageId: 'attempt-1' })
48+
expect(refused?.content).toBe('edited')
49+
})
50+
3351
it('leaves a first message the server may already hold unchanged, with its id', () => {
3452
useMothershipQueueStore.getState().enqueue('chat-A', {
3553
id: 'm1',
@@ -50,6 +68,8 @@ describe('useMothershipQueueStore', () => {
5068
content: 'original',
5169
retryRequired: true,
5270
resumeUserMessageId: 'prior-request',
71+
/** Its Stop never settled, so it was never sent: the server cannot hold it. */
72+
admissionUnknown: false,
5373
queuedSendHandoff: {
5474
id: 'm1',
5575
chatId: 'chat-A',

‎apps/sim/stores/mothership-queue/store.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,19 @@ const initialState = {
5050
cleared: {} as Record<string, number>,
5151
}
5252

53+
/**
54+
* A message resuming an earlier attempt (`resumeUserMessageId`) may already be
55+
* a turn on the server, unless the writer knows it is not
56+
* (`admissionUnknown: false`). Every queue write goes through this, so no path
57+
* can queue such a message as editable by leaving the flag out.
58+
*/
59+
function withAdmissionGuard(message: QueuedMothershipMessage): QueuedMothershipMessage {
60+
if (message.resumeUserMessageId === undefined || message.admissionUnknown !== undefined) {
61+
return message
62+
}
63+
return { ...message, admissionUnknown: true }
64+
}
65+
5366
const omitKey = <V>(record: Record<string, V>, key: string): Record<string, V> => {
5467
if (!(key in record)) return record
5568
const { [key]: _removed, ...rest } = record
@@ -75,7 +88,7 @@ export const useMothershipQueueStore = create<MothershipQueueState>()(
7588
return {
7689
queues: setQueueForChat(state.queues, chatKey, [
7790
...(state.queues[chatKey] ?? []),
78-
message,
91+
withAdmissionGuard(message),
7992
]),
8093
}
8194
}),
@@ -87,7 +100,7 @@ export const useMothershipQueueStore = create<MothershipQueueState>()(
87100
const current = state.queues[chatKey] ?? []
88101
if (current.some((m) => m.id === message.id)) return state
89102
const next = [...current]
90-
next.splice(Math.max(0, Math.min(index, next.length)), 0, message)
103+
next.splice(Math.max(0, Math.min(index, next.length)), 0, withAdmissionGuard(message))
91104
return { queues: setQueueForChat(state.queues, chatKey, next) }
92105
}),
93106

0 commit comments

Comments
 (0)