Skip to content

Commit adaa4fd

Browse files
authored
fix(frontend): resolve stale closure in chatHistory updateCurrentSession (#1294)
## Root cause `updateCurrentSession` in `useChatHistory.ts` read `historyState.currentSessionId` from its **stale closure** for the early-return guard: ```typescript // BEFORE — stale closure const updateCurrentSession = useCallback( (messages: ChatMessage[]) => { if (!historyState.currentSessionId) return; // captures outer historyState setHistoryState((prev) => { … }); }, [historyState.currentSessionId], ); ``` If `currentSessionId` changed between renders (e.g. the user opened a different session) before the callback was recreated by React, the guard could read the wrong value — silently skipping the update and causing the previous session's messages to be lost. ## Fix (`src/hooks/useChatHistory.ts`) Move the guard inside the `setHistoryState` functional updater, where `prev` is always the fresh committed state provided by React — no closure involved: ```typescript // AFTER — always reads fresh state via prev const updateCurrentSession = useCallback( (messages: ChatMessage[]) => { setHistoryState((prev) => { if (!prev.currentSessionId) return prev; // fresh, never stale … }); }, [], // no dependency on historyState needed ); ``` ## Regression tests (`src/hooks/useChatHistory.test.ts`) Two tests added: 1. Verifies the updater applies messages to the session identified by `prev.currentSessionId`, even when a stale outer value would point to a different session. 2. Verifies the updater returns `prev` unchanged when `prev.currentSessionId` is `null`. Closes #1223
1 parent 598a802 commit adaa4fd

2 files changed

Lines changed: 59 additions & 6 deletions

File tree

Dechat/dex_with_fiat_frontend/src/hooks/useChatHistory.test.ts

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
import { describe, expect, it } from 'vitest';
1+
import { describe, expect, it, vi, beforeEach } from 'vitest';
22
import { ChatSession } from '@/types';
33

4-
// Pure utility tests for pin ordering logic (mirrors useChatHistory internals)
4+
// Pure utility tests for pin ordering logic and stale-closure regression (#1223)
55

66
function sortSessions(sessions: ChatSession[]): ChatSession[] {
77
return [...sessions].sort((a, b) => {
@@ -106,3 +106,55 @@ describe('Thread pinning ordering', () => {
106106
expect(sorted[1].id).toBe(noPinField.id);
107107
});
108108
});
109+
110+
// ── updateCurrentSession stale-closure regression (#1223) ──────────────────
111+
//
112+
// Before the fix, updateCurrentSession closed over historyState.currentSessionId
113+
// for its early-return guard. If currentSessionId changed between renders the
114+
// stale closure value would cause the guard to use the wrong session ID.
115+
//
116+
// The fix moves the guard inside the setHistoryState functional updater so it
117+
// always reads `prev.currentSessionId` (fresh state), never the closure value.
118+
// We test the invariant in isolation so the fix is not coupled to the full hook.
119+
120+
describe('updateCurrentSession guard reads fresh state (regression #1223)', () => {
121+
beforeEach(() => {
122+
vi.clearAllMocks();
123+
});
124+
125+
// Simulate the functional updater pattern used by the fixed updateCurrentSession.
126+
function makeUpdater(messages: { id: string }[]) {
127+
return (prev: { currentSessionId: string | null; sessions: { id: string; messages: { id: string }[] }[] }) => {
128+
if (!prev.currentSessionId) return prev;
129+
const idx = prev.sessions.findIndex((s) => s.id === prev.currentSessionId);
130+
if (idx === -1) return prev;
131+
const updated = [...prev.sessions];
132+
updated[idx] = { ...updated[idx], messages };
133+
return { ...prev, sessions: updated };
134+
};
135+
}
136+
137+
it('updates the session identified by prev.currentSessionId, not a stale outer value', () => {
138+
const sessionA = { id: 'a', messages: [] as { id: string }[] };
139+
const sessionB = { id: 'b', messages: [] as { id: string }[] };
140+
const newMessages = [{ id: 'msg1' }];
141+
142+
// Stale outer value would be 'a', but we simulate the state having already
143+
// advanced to 'b' before the updater runs.
144+
const freshState = { currentSessionId: 'b', sessions: [sessionA, sessionB] };
145+
146+
const nextState = makeUpdater(newMessages)(freshState);
147+
148+
expect(nextState.sessions.find((s) => s.id === 'b')?.messages).toEqual(newMessages);
149+
expect(nextState.sessions.find((s) => s.id === 'a')?.messages).toEqual([]);
150+
});
151+
152+
it('returns prev unchanged when prev.currentSessionId is null', () => {
153+
const session = { id: 'a', messages: [] as { id: string }[] };
154+
const state = { currentSessionId: null, sessions: [session] };
155+
156+
const nextState = makeUpdater([{ id: 'msg1' }])(state);
157+
158+
expect(nextState).toBe(state);
159+
});
160+
});

Dechat/dex_with_fiat_frontend/src/hooks/useChatHistory.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,11 @@ export const useChatHistory = () => {
7676

7777
const updateCurrentSession = useCallback(
7878
(messages: ChatMessage[]) => {
79-
if (!historyState.currentSessionId) return;
80-
8179
setHistoryState((prev) => {
80+
// Guard inside the functional updater so it always reads fresh state,
81+
// not the stale closure value of historyState.currentSessionId (#1223).
82+
if (!prev.currentSessionId) return prev;
83+
8284
const sessionIndex = prev.sessions.findIndex(
8385
(s) => s.id === prev.currentSessionId,
8486
);
@@ -90,7 +92,6 @@ export const useChatHistory = () => {
9092
lastUpdated: new Date(),
9193
};
9294

93-
// Update title if this is the first user message
9495
const updatedSessionWithTitle =
9596
ChatHistoryManager.updateSessionTitle(updatedSession);
9697

@@ -103,7 +104,7 @@ export const useChatHistory = () => {
103104
};
104105
});
105106
},
106-
[historyState.currentSessionId],
107+
[],
107108
);
108109

109110
const loadSession = useCallback(

0 commit comments

Comments
 (0)