Skip to content

Commit 39e7cca

Browse files
committed
fix(ui-kit): stop useStreamingText cancel() from overwriting a settled status
cancel() only guarded against the per-effect cancelled flag, so once a stream reached idle/done/error it would still accept a later cancel() and overwrite the terminal status with "cancelled" - clobbering a completed stream, hiding a real error behind an inconsistent cancelled+error pair, or flipping an idle hook that never started a stream. Track settling with its own per-effect flag and gate cancelRef on it, so cancel() is a no-op once the stream has settled.
1 parent e199bb2 commit 39e7cca

2 files changed

Lines changed: 81 additions & 3 deletions

File tree

packages/loopover-ui-kit/src/hooks/use-streaming-text.test.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,4 +122,70 @@ describe("useStreamingText (#6516)", () => {
122122
expect(result.current.error?.message).toBe("stream boom");
123123
expect(result.current.text).toBe("partial");
124124
});
125+
126+
describe("cancel() after settle is a no-op (#10050)", () => {
127+
it("cancel() on a null source leaves status idle", () => {
128+
const { result } = renderHook(() => useStreamingText(null));
129+
expect(result.current.status).toBe("idle");
130+
131+
act(() => result.current.cancel());
132+
133+
expect(result.current.status).toBe("idle");
134+
expect(result.current.text).toBe("");
135+
expect(result.current.error).toBe(null);
136+
});
137+
138+
it("cancel() on a completed stream leaves status done and text unchanged", async () => {
139+
const src = deferredSource();
140+
const { result } = renderHook(() => useStreamingText(src.source));
141+
await src.push("Hello");
142+
await waitFor(() => expect(result.current.text).toBe("Hello"));
143+
await src.finish();
144+
await waitFor(() => expect(result.current.status).toBe("done"));
145+
146+
act(() => result.current.cancel());
147+
148+
expect(result.current.status).toBe("done");
149+
expect(result.current.text).toBe("Hello");
150+
});
151+
152+
it("cancel() on a failed stream leaves status error and the error intact", async () => {
153+
const src = deferredSource();
154+
const { result } = renderHook(() => useStreamingText(src.source));
155+
await src.push("partial");
156+
await waitFor(() => expect(result.current.text).toBe("partial"));
157+
await src.fail(new Error("stream boom"));
158+
await waitFor(() => expect(result.current.status).toBe("error"));
159+
160+
act(() => result.current.cancel());
161+
162+
expect(result.current.status).toBe("error");
163+
expect(result.current.error?.message).toBe("stream boom");
164+
});
165+
166+
it("swapping in a new source after a settled cancel() still starts and can be cancelled", async () => {
167+
const first = deferredSource();
168+
const { result, rerender } = renderHook(
169+
({ s }: { s: ChunkSource | null }) => useStreamingText(s),
170+
{ initialProps: { s: first.source as ChunkSource | null } },
171+
);
172+
await first.push("old");
173+
await waitFor(() => expect(result.current.text).toBe("old"));
174+
await first.finish();
175+
await waitFor(() => expect(result.current.status).toBe("done"));
176+
177+
act(() => result.current.cancel()); // settled cancel — no-op
178+
expect(result.current.status).toBe("done");
179+
180+
const second = deferredSource();
181+
rerender({ s: second.source });
182+
await waitFor(() => expect(result.current.status).toBe("streaming"));
183+
184+
await second.push("new");
185+
await waitFor(() => expect(result.current.text).toBe("new"));
186+
187+
act(() => result.current.cancel());
188+
await waitFor(() => expect(result.current.status).toBe("cancelled"));
189+
});
190+
});
125191
});

packages/loopover-ui-kit/src/hooks/use-streaming-text.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,11 @@ export interface StreamingTextState {
1616
text: string;
1717
status: StreamingStatus;
1818
error: Error | null;
19-
/** Stop consuming the current source; no later chunk from it reaches state. Idempotent, safe post-unmount. */
19+
/**
20+
* Stop consuming the current source; no later chunk from it reaches state. Idempotent, safe
21+
* post-unmount. A no-op once the stream has already settled (reached `idle`, `done` or `error`
22+
* for the current source) — it never overwrites a settled status with `"cancelled"`.
23+
*/
2024
cancel: () => void;
2125
}
2226

@@ -39,8 +43,11 @@ export function useStreamingText(
3943
// Per-effect flag (a fresh closure each run): the cleanup below flips it on a new source or unmount, so the
4044
// previous run's worker stops and writes no more state. cancel() flips this same flag for an explicit stop.
4145
let cancelled = false;
46+
// Per-effect flag mirroring `cancelled`, but set on each terminal transition (idle/done/error) instead of
47+
// by cancel()/cleanup. Once the stream has settled, cancel() must leave status/text/error untouched.
48+
let settled = false;
4249
cancelRef.current = () => {
43-
if (!cancelled) {
50+
if (!cancelled && !settled) {
4451
cancelled = true;
4552
setStatus("cancelled");
4653
}
@@ -55,6 +62,7 @@ export function useStreamingText(
5562
setText("");
5663
setError(null);
5764
if (!source) {
65+
settled = true;
5866
setStatus("idle");
5967
return;
6068
}
@@ -64,9 +72,13 @@ export function useStreamingText(
6472
if (cancelled) return;
6573
setText((prev) => prev + chunk);
6674
}
67-
if (!cancelled) setStatus("done");
75+
if (!cancelled) {
76+
settled = true;
77+
setStatus("done");
78+
}
6879
} catch (err) {
6980
if (!cancelled) {
81+
settled = true;
7082
setError(err instanceof Error ? err : new Error(String(err)));
7183
setStatus("error");
7284
}

0 commit comments

Comments
 (0)