feat(widget): bouncing-dots loading that survives thread switches

[AmitAvital1]

Stack 3/3 · base: feat/widget-thread-list (#59)

Shows a bouncing-dots indicator while the assistant has no answer text yet, and keeps it visible when the user leaves a streaming thread and returns. Messages and usage live in per-conversation maps keyed by the id captured at submit time, so switching threads only swaps the view — the in-flight stream keeps writing to its own bucket. typing means "stream in flight" (set at submit, cleared at completion); reduceStreamEvent is reduced to content accumulation only, fixing dots that could otherwise linger on an empty final answer.

Every request now names its conversation (send(id, text), stream(id, text), loadHistory(id), loadUsage(id)); localStorage only remembers which thread was selected last and never decides where an in-flight request lands (review feedback, f529a92).

Verified: widget typecheck, test:widget, and Playwright e2e (incl. switch-away-and-back and a mid-request thread-switch regression test) all green.

🤖 Generated with Claude Code

[Asaf-prog]

The new per-conversation message state is a good improvement.

However, methods like stream(), send(), loadHistory(), and loadUsage() still read the active conversation ID from local storage.

This can cause a problem if the user switches conversations while a request is still running. The request may start in conversation A, but later use conversation B from local storage.

I suggest passing the conversation ID directly to every method:

stream(conversationId, text)
send(conversationId, text)
loadHistory(conversationId)
loadUsage(conversationId)

Local storage should only remember the last selected conversation. It should not decide which conversation an active request belongs to.

I would fix this before merging.

[AmitAvital1]

Agreed — fixed in f529a92, exactly the signature change you proposed:

send(conversationId, text)
stream(conversationId, text)
loadHistory(conversationId)
loadUsage(conversationId)

submit already captures the id for the per-conversation message map, so it passes that same id down. Storage is back to only remembering which thread was selected last.

Two follow-ons that fell out of it:

• Usage had the same bug, and it fires in finally — the most likely moment for the user to have switched away. loadUsage takes the id now, and usage moved into a per-conversation map next to the messages, so a late response can't paint another thread's number.
• Stale-conversation recovery (404 → create a new one) was silently changing which conversation a turn belonged to after its messages were already bucketed under the dead id — so the next send would jump to the fresh id and the visible messages would vanish. It now reports the swap through onReplaced, and the app re-keys that thread's messages onto the id the turn actually ran under.

Regression test: a turn whose stream fails after the user switched threads retries on its own conversation, and the usage refresh follows it. I checked it fails against the previous code (the retry went to the other thread) before landing the fix.

Rebased on top of the updated #58 and #59; whole stack now sits on main.

[Asaf-prog]

Thanks for addressing the original thread-switching issue. Passing the conversation ID explicitly into send, stream, loadHistory, and loadUsage is the right approach.

However, I think there is still an issue in the stale-conversation recovery flow.

When a stale conversation returns 404, replace() creates a new conversation and calls onReplaced(staleId, freshId). The UI then moves the entries to freshId and may switch activeId to it.

But the submit callback still holds the original cid:

const cid = await conversation.ensureId();

for await (const event of conversation.stream(cid, text)) {
  replaceEntry(cid, pending.id, entry);
}

void refreshUsage(cid);

So after the request is retried against freshId, subsequent stream events and the usage refresh are still applied using staleId.

This can leave the response or usage stored under the old conversation while the UI is displaying the new one.

I think the effective conversation ID used after recovery needs to be propagated back to the caller and then used consistently for:

• updating the pending entry
• storing streamed content
• the non-streaming fallback
• refreshing usage

Also, PR #60 currently depends on #59, so I would hold off on merging it until the authorization issue in #59 is resolved and the branch is rebased onto main.

One additional improvement: the GitHub CI currently does not appear to run the widget typecheck or Playwright tests, so it would be good to add those checks to the workflow rather than relying only on locally reported results.

[AmitAvital1]

Good catch — and it was worse than the trace shows. If the non-streaming fallback also 404s, replace() runs again against the already-moved key, so onReplaced carries an empty array onto a second fresh id and the entry is stranded a second time.

Fixed in 3b99734. send() and stream() now take a Retarget callback, and the turn keeps a target that starts as the id it began with and moves when recovery happens. Everything after that point follows it — the pending entry, streamed content, the fallback answer, and the usage refresh. sendWithoutStreaming() returns where it landed, so a fallback that recovers again is accounted for too.

useConversation() also loses its constructor-level onReplaced. Turn identity is per-request, not per-hook, and having two channels for the same fact is what made this easy to miss in the first place.

Worth saying why the existing stale-conversation test didn't catch this: its history request 404s at open, which clears storage before the turn, so that test creates a fresh conversation up front and never exercises mid-turn replacement. The new e2e test keeps the conversation alive through history loading and only 404s it once the turn is under way. I confirmed it fails on the previous code — the answer stays stranded in the dead conversation and the dots never stop.

On CI: you're right, and that was the most useful of the three points. make check was the only gate, so the widget typecheck and the Playwright suite were verified nowhere but on my machine. d48f166 adds a widget job — typecheck (widget + e2e), the node self-check, and Playwright on chromium. It also rebuilds widget.js and fails if the result differs from the committed bundle, since that file is what the browser actually loads and a stale one would let the browser tests pass against source that was never built. Both jobs are green on the current head.

Agreed on merge order — this stays behind #59. Once that lands I'll rebase this branch onto main.

[Asaf-prog]

Thanks for fixing the stale-conversation retargeting flow and for adding the widget CI job. The effective conversation ID now correctly follows the stream, fallback, and usage refresh, and the new browser coverage is valuable.

However, I think there is still a race condition involving the selected conversation in localStorage.

The current replacement flow removes the stored conversation ID and then creates a fresh conversation:

removeStoredConversationId(endpoint, userId);
const fresh = await startConversation();

startConversation() stores the fresh ID as the selected conversation.

Consider this sequence:

1. A request starts in conversation A.
2. The user switches to conversation B.
3. localStorage now points to B.
4. The in-flight request for A receives a 404.
5. Recovery removes the stored ID and stores the replacement conversation A2.

adoptConversation() correctly avoids changing activeId when the user is already viewing B:

setActiveId((current) => (current === staleId ? freshId : current));

But localStorage now points to A2 while the UI is still displaying B.

The next submission calls:

const cid = await conversation.ensureId();

Since ensureId() reads from localStorage, a message submitted while the user is viewing B may be sent to A2 instead.

Recovery for an inactive background conversation should not replace the user’s currently selected conversation.

I suggest only updating storage when the stale conversation is still the selected one:

const freshId = await client.createConversation();

if (peekId() === staleId) {
  setStoredConversationId(endpoint, userId, freshId);
}

onReplaced?.(staleId, freshId);
return freshId;

It would also be safer for submission to use the currently displayed activeId when available, rather than resolving the target again from storage:

const cid = activeId || await conversation.ensureId();

A regression test should cover this sequence:

• start a request in A
• switch to B
• make A return 404 and recover as A2
• verify that B remains selected in storage
• submit another message while viewing B
• verify that it is sent to B, not A2

The current tests verify that the original in-flight request follows its replacement, but they do not verify that background recovery cannot change the destination of the next user request.

I would hold off on merging until this selected-conversation race is addressed.

[AmitAvital1]

Confirmed and fixed in a10fb7b. Both new tests fail on the previous code with exactly the sequence you described — storage ends on conv-reborn instead of conv-other, and the next message follows it.

I went slightly further than your suggestion. Rather than guarding the storage write inside replace(), replace() no longer touches storage at all:

const replace = useCallback(
  async (staleId: string, onReplaced?: Retarget) => {
    // Deliberately does not touch storage. Recovery can run for a turn the
    // user has since navigated away from, and only the caller knows which
    // thread is on screen — it selects the replacement via `onReplaced`.
    const fresh = await client.createConversation();
    onReplaced?.(staleId, fresh);
    return fresh;
  },
  [client],
);

The component then decides, because it is the thing that knows what is displayed:

const adoptConversation = useCallback((staleId: string, freshId: string) => {
  setEntriesById(({ [staleId]: moved = [], ...rest }) => ({ ...rest, [freshId]: moved }));
  if (activeIdRef.current !== staleId) return;
  selectThread(freshId);
  conversation.switchTo(freshId);
}, [conversation, selectThread]);

Your peekId() === staleId guard would fix the reported sequence too. I preferred keying on the displayed thread because storage is a cache of the selection rather than the selection itself, so the two can disagree — and the hook has no business knowing which one the user is looking at.

On why a ref and not state: the old code read the current selection through setActiveId(current => ...), but selecting now also writes storage, and a state updater has to stay pure — React may call it twice. Every setActiveId goes through selectThread, so the ref cannot drift. submit() reads that same ref rather than the activeId state you suggested; they are equivalent at click time, but the ref closes a one-update gap if a background recovery lands between render and click, and it keeps one source of truth for "which thread is on screen".

Tests, following your sequence:

1. background recovery for a conversation the user left does not hijack the next submission — start in A, switch to B, A 404s and recovers as A2; asserts B stays selected in storage and that the next message goes to B.
2. background recovery does not capture a new chat the user started meanwhile — the same race against "New chat", where the selection is empty rather than another thread. Asserts the new chat gets its own conversation and that the recovered one never receives "fresh start".

I also added the inverse assertion to the existing foreground test: recovery of the thread on screen must still move the selection. That direction is now conditional, so without pinning it a future change could quietly stop selecting anything.

CI is green on both jobs. This branch is rebased onto the current #59 head; once #59 lands I will rebase it onto main so it stands alone.