Skip to content

Commit b2da69b

Browse files
committed
fix(shortcuts): New Chat fires from composer + never reopens a pending send
Two review fixes: - chat.new (⌘N/Ctrl+N) gains allowInInput: true so it fires while the chat composer textarea is focused (the normal state when starting fresh) and preventDefault stops the OS "new window" accelerator swallowing it. - Lift the optimistic-send signal into Redux: chatRuntimeSlice gains pendingSendThreadIds with mark/clear reducers, set synchronously the instant a user sends (mirrored from Conversations' existing pending-send helpers, before addMessageLocal is awaited). useNewChat now also excludes pending-send threads, closing the earliest window where messageCount, messagesByThreadId and streamingAssistantByThread are all still empty.
1 parent b102af5 commit b2da69b

5 files changed

Lines changed: 85 additions & 25 deletions

File tree

app/src/components/layout/shell/useNewChat.test.tsx

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,16 @@ const mockDispatch = vi.fn();
3232
let mockThreads: MockThread[] = [];
3333
let mockMessagesByThreadId: Record<string, unknown[]> = {};
3434
let mockStreamingByThread: Record<string, unknown> = {};
35+
let mockPendingSendThreadIds: Record<string, true> = {};
3536
vi.mock('../../../store/hooks', () => ({
3637
useAppDispatch: () => mockDispatch,
3738
useAppSelector: (sel: (s: unknown) => unknown) =>
3839
sel({
3940
thread: { threads: mockThreads, messagesByThreadId: mockMessagesByThreadId },
40-
chatRuntime: { streamingAssistantByThread: mockStreamingByThread },
41+
chatRuntime: {
42+
streamingAssistantByThread: mockStreamingByThread,
43+
pendingSendThreadIds: mockPendingSendThreadIds,
44+
},
4145
}),
4246
}));
4347

@@ -64,6 +68,7 @@ describe('useNewChat', () => {
6468
mockThreads = [];
6569
mockMessagesByThreadId = {};
6670
mockStreamingByThread = {};
71+
mockPendingSendThreadIds = {};
6772
mockDispatch.mockImplementation((action: MockAction) => {
6873
if (action?.type === 'thread/createNewThread') {
6974
return { unwrap: () => Promise.resolve({ id: 'fresh-thread' }) };
@@ -129,6 +134,23 @@ describe('useNewChat', () => {
129134
});
130135
});
131136

137+
it('does not reuse a thread with an optimistic send pending (pre-fulfillment window)', async () => {
138+
// Earliest window: send recorded in pendingSendThreadIds before
139+
// addMessageLocal resolves and before any streaming state exists.
140+
mockThreads = [{ id: 'optimistic', messageCount: 0 }];
141+
mockMessagesByThreadId = {};
142+
mockStreamingByThread = {};
143+
mockPendingSendThreadIds = { optimistic: true };
144+
const { result } = renderHook(() => useNewChat(), { wrapper });
145+
result.current();
146+
147+
expect(mockNavigate).not.toHaveBeenCalledWith('/chat/optimistic');
148+
expect(dispatchedTypes()).toContain('thread/createNewThread');
149+
await waitFor(() => {
150+
expect(mockNavigate).toHaveBeenCalledWith('/chat/fresh-thread');
151+
});
152+
});
153+
132154
it('reuses a genuinely-blank thread (count-empty, no cache, no in-flight turn)', () => {
133155
// Round-trip of the earlier review: a blank current chat must be reused
134156
// rather than spawning yet another empty thread.

app/src/components/layout/shell/useNewChat.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ import { chatThreadPath } from '../../../utils/chatRoutes';
3030
* count, no locally-cached messages, and no in-flight assistant turn. The first
3131
* two guard the post-send window where `addMessageLocal` has populated
3232
* `messagesByThreadId` (or the thread-list `messageCount`) but the other lags;
33-
* the streaming check (`chatRuntime.streamingAssistantByThread`, whose
34-
* `started` lifecycle is set the moment the user sends) covers the
35-
* pre-fulfillment window. We intentionally key off "is this thread actually
36-
* occupied?" rather than "is it selected?" so a genuinely-blank current chat is
37-
* still reused (no piling up of empties) while a chat with a send in flight is
38-
* never reopened.
33+
* the `pendingSendThreadIds` marker (set synchronously the instant the user
34+
* sends, before `addMessageLocal` is even awaited) closes the earliest
35+
* optimistic-send window, and the streaming buffer covers later ones. We key
36+
* off "is this thread actually occupied?" rather than "is it selected?" so a
37+
* genuinely-blank current chat is still reused (no piling up of empties) while
38+
* a chat with a send in flight is never reopened.
3939
*/
4040
export function useNewChat(): () => void {
4141
const navigate = useNavigate();
@@ -45,6 +45,7 @@ export function useNewChat(): () => void {
4545
// Optional-chained so minimal test stores without the chatRuntime reducer
4646
// still resolve (mirrors `selectPanelLayout`'s defensive access).
4747
const streamingByThread = useAppSelector(state => state.chatRuntime?.streamingAssistantByThread);
48+
const pendingSendThreadIds = useAppSelector(state => state.chatRuntime?.pendingSendThreadIds);
4849

4950
return useCallback(() => {
5051
dispatch(setActiveAccount(AGENT_ACCOUNT_ID));
@@ -53,7 +54,8 @@ export function useNewChat(): () => void {
5354
thr =>
5455
(thr.messageCount ?? 0) === 0 &&
5556
(messagesByThreadId[thr.id]?.length ?? 0) === 0 &&
56-
!streamingByThread?.[thr.id]
57+
!streamingByThread?.[thr.id] &&
58+
!pendingSendThreadIds?.[thr.id]
5759
);
5860
if (empty) {
5961
dispatch(setSelectedThread(empty.id));
@@ -74,5 +76,5 @@ export function useNewChat(): () => void {
7476
// diagnosable. The user stays where they are (no broken navigation).
7577
console.error('[new-chat] createNewThread failed', err);
7678
});
77-
}, [navigate, dispatch, threads, messagesByThreadId, streamingByThread]);
79+
}, [navigate, dispatch, threads, messagesByThreadId, streamingByThread, pendingSendThreadIds]);
7880
}

app/src/lib/commands/globalActions.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,10 @@ function buildGlobalActions(h: GlobalActionHandlers): GlobalActionDef[] {
180180
labelKey: 'shortcuts.action.newChat',
181181
group: 'Chat',
182182
shortcut: 'mod+n',
183+
// Must fire from the composer too — that's the normal focus state when a
184+
// user decides to start fresh — and preventDefault keeps the OS "new
185+
// window" accelerator from swallowing it.
186+
allowInInput: true,
183187
handler: h.newChat,
184188
keywords: ['new', 'thread', 'compose', 'conversation', 'session'],
185189
},

app/src/pages/Conversations.tsx

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,11 @@ import {
4848
beginInferenceTurn,
4949
clearFollowupsForThread,
5050
clearRuntimeForThread,
51+
clearThreadSendPending,
5152
enqueueFollowup,
5253
fetchAndHydrateTurnState,
5354
markSubagentCancelled,
55+
markThreadSendPending,
5456
type QueuedFollowup,
5557
registerParallelRequest,
5658
setTaskBoardForThread,
@@ -287,22 +289,32 @@ const Conversations = ({
287289
const [pendingSendingThreadIds, setPendingSendingThreadIds] = useState<ReadonlySet<string>>(
288290
() => new Set()
289291
);
290-
const addPendingSendingThread = useCallback((threadId: string) => {
291-
setPendingSendingThreadIds(prev => {
292-
if (prev.has(threadId)) return prev;
293-
const next = new Set(prev);
294-
next.add(threadId);
295-
return next;
296-
});
297-
}, []);
298-
const removePendingSendingThread = useCallback((threadId: string) => {
299-
setPendingSendingThreadIds(prev => {
300-
if (!prev.has(threadId)) return prev;
301-
const next = new Set(prev);
302-
next.delete(threadId);
303-
return next;
304-
});
305-
}, []);
292+
const addPendingSendingThread = useCallback(
293+
(threadId: string) => {
294+
// Mirror to Redux so global surfaces (e.g. the New Chat shortcut) can see
295+
// an in-flight send before any message/streaming state exists.
296+
dispatch(markThreadSendPending({ threadId }));
297+
setPendingSendingThreadIds(prev => {
298+
if (prev.has(threadId)) return prev;
299+
const next = new Set(prev);
300+
next.add(threadId);
301+
return next;
302+
});
303+
},
304+
[dispatch]
305+
);
306+
const removePendingSendingThread = useCallback(
307+
(threadId: string) => {
308+
dispatch(clearThreadSendPending({ threadId }));
309+
setPendingSendingThreadIds(prev => {
310+
if (!prev.has(threadId)) return prev;
311+
const next = new Set(prev);
312+
next.delete(threadId);
313+
return next;
314+
});
315+
},
316+
[dispatch]
317+
);
306318
const socketStatus = useAppSelector(selectSocketStatus);
307319
const agentProfiles = useAppSelector(selectAgentProfiles);
308320
const selectedAgentProfileId = useAppSelector(selectActiveAgentProfileId);

app/src/store/chatRuntimeSlice.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,13 @@ export type QueueMode = 'interrupt' | 'steer' | 'followup' | 'collect' | 'parall
313313
interface ChatRuntimeState {
314314
inferenceStatusByThread: Record<string, InferenceStatus>;
315315
streamingAssistantByThread: Record<string, StreamingAssistantState>;
316+
/**
317+
* Threads with an optimistic user send in flight, set the instant the user
318+
* sends (before `addMessageLocal` resolves and before any streaming state
319+
* exists). Lets global surfaces — e.g. the New Chat shortcut — tell a
320+
* mid-send conversation apart from a genuinely-blank one.
321+
*/
322+
pendingSendThreadIds: Record<string, true>;
316323
/**
317324
* Live streams for concurrent PARALLEL (forked) turns on a thread, nested
318325
* `threadId -> requestId -> stream`. A separate lane from
@@ -390,6 +397,7 @@ export interface QueuedFollowup {
390397
const initialState: ChatRuntimeState = {
391398
inferenceStatusByThread: {},
392399
streamingAssistantByThread: {},
400+
pendingSendThreadIds: {},
393401
parallelStreamsByThread: {},
394402
parallelRequestThreads: {},
395403
toolTimelineByThread: {},
@@ -657,6 +665,14 @@ const chatRuntimeSlice = createSlice({
657665
clearStreamingAssistantForThread: (state, action: PayloadAction<{ threadId: string }>) => {
658666
delete state.streamingAssistantByThread[action.payload.threadId];
659667
},
668+
/** Mark a thread as having an optimistic user send in flight. */
669+
markThreadSendPending: (state, action: PayloadAction<{ threadId: string }>) => {
670+
state.pendingSendThreadIds[action.payload.threadId] = true;
671+
},
672+
/** Clear the in-flight-send marker once the send settles (or fails). */
673+
clearThreadSendPending: (state, action: PayloadAction<{ threadId: string }>) => {
674+
delete state.pendingSendThreadIds[action.payload.threadId];
675+
},
660676
/**
661677
* Register a parallel (forked) turn so its socket events route to the
662678
* parallel lane. Called when a `queueMode: 'parallel'` send is accepted.
@@ -1068,6 +1084,7 @@ const chatRuntimeSlice = createSlice({
10681084
delete state.pendingPlanReviewByThread[action.payload.threadId];
10691085
delete state.queueStatusByThread[action.payload.threadId];
10701086
delete state.queuedFollowupsByThread[action.payload.threadId];
1087+
delete state.pendingSendThreadIds[action.payload.threadId];
10711088
// Note: artifactsByThread intentionally NOT cleared here. The
10721089
// ArtifactCard renders inline in the message timeline, so the
10731090
// snapshot needs to survive turn boundaries — historic artifacts
@@ -1088,6 +1105,7 @@ const chatRuntimeSlice = createSlice({
10881105
state.artifactsByThread = {};
10891106
state.queueStatusByThread = {};
10901107
state.queuedFollowupsByThread = {};
1108+
state.pendingSendThreadIds = {};
10911109
},
10921110
recordChatTurnUsage: (
10931111
state,
@@ -1224,6 +1242,8 @@ export const {
12241242
clearInferenceStatusForThread,
12251243
setStreamingAssistantForThread,
12261244
clearStreamingAssistantForThread,
1245+
markThreadSendPending,
1246+
clearThreadSendPending,
12271247
registerParallelRequest,
12281248
setParallelStream,
12291249
clearParallelRequest,

0 commit comments

Comments
 (0)