fix: address PR review feedback (#1387)
- gate Quick Chat's session-activation auto-flush to the pre-session queue only, so a restored queue cannot be sent before the restore effect's authoritative fetchChatSession check resolves (real flaw — the original test passed only because the mocked fetch resolved in a microtask and beat the effect) - add slow-fetch regression tests in both hooks proving the restored queue stays un-flushed while server validation is pending - assert attachChatStream ran before triggering its onDone in the quick-chat regression test - drop redundant Promise.resolve() wrapper around fetchChatSession in useQuickChat's restore effect Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2152,6 +2152,36 @@ describe("useChat", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a restored queued message un-flushed while the server validation fetch is pending", async () => {
|
||||
// Production latency case: the authoritative fetch takes one network
|
||||
// RTT. Nothing may flush (or delete) the restored queue in the interim.
|
||||
const sessionA = makeSession({ id: "session-001", agentId: "agent-001" });
|
||||
mockFetchChatSessions.mockResolvedValue({ sessions: [sessionA] });
|
||||
mockFetchChatMessages.mockResolvedValue({ messages: [] });
|
||||
// Server check never resolves within the test — simulates in-flight RTT.
|
||||
mockFetchChatSession.mockReturnValue(new Promise(() => {}) as never);
|
||||
|
||||
localStorage.setItem(getChatPendingMessageKey("session-001")!, "Queued follow-up");
|
||||
|
||||
const { result } = renderHook(() => useChat("proj-123"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(result.current.sessions).toHaveLength(1);
|
||||
});
|
||||
|
||||
act(() => {
|
||||
result.current.selectSession("session-001");
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
await new Promise((resolve) => setTimeout(resolve, 25));
|
||||
});
|
||||
|
||||
expect(result.current.pendingMessage).toBe("Queued follow-up");
|
||||
expect(mockStreamChatResponse).not.toHaveBeenCalled();
|
||||
expect(localStorage.getItem(getChatPendingMessageKey("session-001"))).toBe("Queued follow-up");
|
||||
});
|
||||
|
||||
it("preserves queued messages across session switches and rehydrates them when returning", async () => {
|
||||
const sessionA = {
|
||||
...makeSession({ id: "session-001", agentId: "agent-001" }),
|
||||
|
||||
@@ -1013,6 +1013,10 @@ describe("useQuickChat", () => {
|
||||
expect(mockStreamChatResponse).not.toHaveBeenCalled();
|
||||
expect(localStorage.getItem(getChatPendingMessageKey("session-a"))).toBe("Queued follow-up");
|
||||
|
||||
// The hook attached to the in-flight generation rather than flushing.
|
||||
expect(mockAttachChatStream).toHaveBeenCalledTimes(1);
|
||||
expect(attachHandlers.length).toBeGreaterThan(0);
|
||||
|
||||
// Once the attached generation completes, the queued message flushes.
|
||||
act(() => {
|
||||
attachHandlers[0]?.onDone?.({ messageId: "msg-001" });
|
||||
@@ -1027,6 +1031,35 @@ describe("useQuickChat", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("does not let the session-activation auto-flush send a restored queue while server validation is pending", async () => {
|
||||
// The restore effect's fetchChatSession check takes one network RTT in
|
||||
// production. The session-activation auto-flush effect runs in the same
|
||||
// commit that restores pendingMessageRef, so without the pre-session
|
||||
// gate it would send the restored queue before the check resolves and
|
||||
// re-open the stale-isGenerating loss path (FN-5852).
|
||||
const staleSessionA = makeSession({ id: "session-a", agentId: "agent-001" });
|
||||
mockFetchChatMessages.mockResolvedValue({ messages: [] });
|
||||
// Server check never resolves within the test — simulates in-flight RTT.
|
||||
mockFetchChatSession.mockReturnValue(new Promise(() => {}) as never);
|
||||
|
||||
localStorage.setItem(getChatPendingMessageKey("session-a")!, "Queued follow-up");
|
||||
|
||||
const { result } = renderHook(() => useQuickChat("proj-123"));
|
||||
|
||||
await act(async () => {
|
||||
await result.current.selectSession(staleSessionA);
|
||||
});
|
||||
|
||||
await act(async () => {
|
||||
await new Promise((resolve) => setTimeout(resolve, 25));
|
||||
});
|
||||
|
||||
// The restored queue is intact and nothing was sent.
|
||||
expect(result.current.pendingMessage).toBe("Queued follow-up");
|
||||
expect(mockStreamChatResponse).not.toHaveBeenCalled();
|
||||
expect(localStorage.getItem(getChatPendingMessageKey("session-a"))).toBe("Queued follow-up");
|
||||
});
|
||||
|
||||
it("pre-session queueing does not write a null localStorage key", async () => {
|
||||
const session = makeSession({ id: "session-pre", agentId: "agent-001" });
|
||||
mockFetchResumeChatSession.mockResolvedValueOnce({ session });
|
||||
|
||||
@@ -662,7 +662,7 @@ export function useQuickChat(
|
||||
// lacks), so flushing from local state alone fires a send that aborts a
|
||||
// live generation server-side and can lose the queued message (FN-5852).
|
||||
let cancelled = false;
|
||||
void Promise.resolve(fetchChatSession(sessionId, projectId))
|
||||
void fetchChatSession(sessionId, projectId)
|
||||
.then(({ session: refreshedSession }) => {
|
||||
if (
|
||||
cancelled ||
|
||||
@@ -1054,6 +1054,15 @@ export function useQuickChat(
|
||||
return;
|
||||
}
|
||||
|
||||
// Only the pre-session queue (a send issued before session init
|
||||
// completed) may auto-flush on session activation. Restored queued
|
||||
// messages must wait for the restore effect's authoritative
|
||||
// fetchChatSession check — flushing them here would race ahead of it
|
||||
// and re-open the stale-isGenerating loss path (FN-5852).
|
||||
if (!queuedPreSessionCompletionRef.current) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (pendingMessageRef.current.trim().length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user