Comfy-Org / Comfy-Org/ComfyUI_frontend

Agent send: a localStorage failure after the ack hides a turn the server already started

Open
#17,358 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Problem / Goal

In `sendMessage()` ([`useAgentSession.ts`](https://github.com/Comfy-Org/ComfyUI_frontend/blob/main/src/workbench/extensions/agent/composables/agent/useAgentSession.ts)) the post-ack bookkeeping shares one `try` with the request itself:

```ts
const ack = await postTurn(conversationStore.threadId ?? 'new')
accepted = true
conversationStore.setThreadId(ack.thread_id)
localStorage.setItem(THREAD_STORAGE_KEY, ack.thread_id) // can throw
...
conversationStore.recordUser(turnId, text, ...) // never runs
conversationStore.startTurn(turnId) // never runs
```

`localStorage.setItem` throws on quota exhaustion and in Safari private mode. When it does, the server has already started the turn but neither `recordUser()` nor `startTurn()` runs. The user sees "Send failed" and their own message never appears in the transcript, while the agent streams a response for a turn the conversation store has never heard of — every frame for it is dropped or, for `agent_message_done`, surfaced as a malformed-event overlay.

The same applies to any other throw between `accepted = true` and `startTurn()` (for example a throwing `adopted` callback).

Surfaced during review of #16628, which added an `accepted` flag that documents this window rather than closing it. Out of scope there: #16628 is a telemetry change.

## Proposed Solution

Separate the request from the bookkeeping so a local failure cannot make an accepted turn invisible:

- Narrow the `try` to `postTurn()` alone; run the post-ack bookkeeping outside it.
- Make thread-id persistence non-fatal — `localStorage` is a convenience for surviving a reload, not a precondition for showing the turn that is already running. Wrap the `setItem` and report the failure rather than aborting the send.
- Order the bookkeeping so store state (`recordUser` / `startTurn`) lands before anything that can fail.

Alternative considered: keep the single `try` and, in the catch, roll the turn forward when `accepted` is true. That reproduces the whole happy path in the error branch and drifts the moment either side changes.

## Acceptance Criteria

- [ ] A send whose `localStorage.setItem` throws still records the user message and starts the turn; the transcript shows the user's text and the streaming assistant reply.
- [ ] That send does not show "Send failed".
- [ ] The persistence failure is reported via `reportError()` with a stable `errorType`.
- [ ] Subsequent stream frames for that turn are ingested normally rather than dropped or surfaced as a malformed event.
- [ ] Unit test in `useAgentSession.test.ts` covering a throwing `localStorage.setItem`, proven to fail against current `main`.
- [ ] A throw from the `adopted` callback after the ack likewise leaves the turn visible.

Contributor guide

Open the contributing guide

Research direction

Start in src/workbench/extensions/agent/composables/agent/useAgentSession.ts at sendMessage(), focusing on the boundary after postTurn() acknowledges the request. Run the unit tests in useAgentSession.test.ts, including a throwing localStorage.setItem and adopted callback. Done means the user message and streaming reply remain visible, no "Send failed" appears, persistence errors are reported, and later stream frames are ingested normally.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
frontend, testing
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.