a queue can no longer be delivered to the wrong conversation

My fix was worse than the bug it replaced. Queue in one chat, switch to another,
and the messages were SENT there — into a conversation the person had not written
them in.

`ChatDetailPanel` mounts `<EmbeddableChat sessionId={undefined}>` and passed no
panel id, so `queueKey` fell through to `screen:new` for every conversation it
showed. One key, every chat. Switching restored the queue into the chat being
opened, and the mount-drain I added — which exists so a returning queue is not left
sitting there — then delivered it. Two of my changes, each defensible alone,
combining into the worst outcome available.

Three things, in order of what actually protects the user:

**A key that cannot be shared.** `queueKey` returns null when neither a panel nor a
session identifies the mount, and null persists nothing. Then an unidentifiable
chat behaves exactly as it did before any of this: queue local to the mount, lost on
a switch. That is the original complaint, and it is strictly better than delivering
to the wrong place. A shared key is worse than no key.

**The drain refuses a null key too.** Belt and braces: nothing was restored, so
nothing may be sent.

**`ChatDetailPanel` now says which conversation it is**, from `resumeSessionId`,
which it already had in scope. The identity was one line away the whole time.

Three tests pin it, including the collision itself: two resumed conversations must
not produce the same key, and a null key must never persist.

The lesson I would rather write down than repeat: I added the mount-drain reasoning
about a queue's promise, and never asked what it would do if the key were wrong. A
mechanism that sends messages needs its failure mode considered before its happy
path.

tsgo clean, frontend builds, 901 pass / 7 fail unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-16 14:19:31 +00:00
co-authored by Claude Opus 5
parent 19e75651e7
commit bc2bafe24a
4 changed files with 46 additions and 6 deletions
@@ -12,6 +12,22 @@ const KEYS = ['p1:s1', 'p2:s1', 'p1:new', 'p2:new', 'screen:s1'];
beforeEach(() => KEYS.forEach((k) => writeQueue(k, [])));
describe('queueKey', () => {
// The bug the first version shipped: `ChatDetailPanel` mounts with `sessionId={undefined}`, so every
// conversation fell back to one shared key. Queue in one chat, open another, and the queue was restored
// there — and the mount-drain SENT it. Somebody's message delivered to a chat they did not write it in.
test('an unidentifiable mount gets NO key, so it cannot share one', () => {
expect(queueKey(undefined, undefined)).toBeNull();
});
test('a null key never persists anything', () => {
writeQueue(null, [prompt('must not survive')]);
expect(readQueue(null)).toEqual([]);
});
test('two resumed conversations in the same surface do not collide', () => {
expect(queueKey('chat:AAA', undefined)).not.toBe(queueKey('chat:BBB', undefined));
});
test('a panel and a session together identify a queue', () => {
expect(queueKey('p1', 's1')).toBe('p1:s1');
});
@@ -28,14 +28,31 @@ import type { QueuedPrompt } from './useEmbeddableChat';
*/
const queues = new Map<string, QueuedPrompt[]>();
/** A queue belongs to one panel's view of one conversation. */
export const queueKey = (panelId: string | undefined, sessionId: string | undefined): string =>
`${panelId ?? 'screen'}:${sessionId ?? 'new'}`;
/**
* A queue belongs to one panel's view of one conversation — or to nothing at all.
*
* **Null when neither identifies it, and that is the important case.** The first version fell back to
* `screen:new`, so every conversation opened without a session id shared one queue: `ChatDetailPanel`
* mounts with `sessionId={undefined}`, so queueing in one chat and opening another restored the queue
* there — and the mount-drain then SENT it into the wrong conversation. Somebody's message, delivered to
* a chat they did not write it in.
*
* A shared key is worse than no key. With null the queue simply stays local to the mount and is lost on a
* switch, which is where this started — bad, but not wrong.
*
* A panel alone is enough: one panel holds one new chat, so `panel-3:new` is unambiguous. A session alone
* is enough: that IS the conversation. Neither is not.
*/
export function queueKey(panelId: string | undefined, sessionId: string | undefined): string | null {
if (!panelId && !sessionId) return null;
return `${panelId ?? 'screen'}:${sessionId ?? 'new'}`;
}
export const readQueue = (key: string): QueuedPrompt[] => queues.get(key) ?? [];
export const readQueue = (key: string | null): QueuedPrompt[] => (key ? (queues.get(key) ?? []) : []);
/** Empty is the same as absent — an entry per conversation ever opened would grow without bound. */
export function writeQueue(key: string, items: QueuedPrompt[]): void {
export function writeQueue(key: string | null, items: QueuedPrompt[]): void {
if (!key) return;
if (items.length === 0) queues.delete(key);
else queues.set(key, items);
}
@@ -102,7 +102,10 @@ export function useEmbeddableChat(params: UseEmbeddableChatParams, onMessageComp
useEffect(() => {
if (restoredRef.current) return;
restoredRef.current = true;
if (!isGenerating && queuedRef.current.length > 0) void drainQueueRef.current();
// `storeKey` being null means this mount cannot identify its conversation, so nothing was restored and
// nothing may be sent — see `queue-store`. Sending here on a shared key is what delivered a message
// into the chat somebody had switched TO.
if (storeKey && !isGenerating && queuedRef.current.length > 0) void drainQueueRef.current();
}, []);
// Detect when agent finishes a message (isGenerating: true → false)
@@ -200,6 +200,10 @@ function NewChat(props: NewChatProps) {
<EmbeddableChat
chat={chat}
sessionId={undefined}
// Which CONVERSATION this is, from the URL — the queue is keyed on it so switching chats cannot
// carry queued messages into the one you switched to. `sessionId` above is a different question
// (it seeds the socket handshake and must stay undefined here, since `chat` is already resumed).
panelId={resumeSessionId ? `chat:${resumeSessionId}` : undefined}
// The session's own model wins. `locationState.model` is declared on ChatLocationState and
// written by nothing, so it was the only source and always undefined on resume; it stays as a
// fallback for a future caller that navigates with one deliberately.