fix(webchat): persist chat queue to localStorage with drain-on-reconnect#73154
fix(webchat): persist chat queue to localStorage with drain-on-reconnect#73154jjjojoj wants to merge 4 commits intoopenclaw:mainfrom
Conversation
- New file chat-queue-persistence.ts: persistChatQueue + restoreChatQueue with 24h TTL - app-chat.ts: call persistQueue after every chatQueue mutation - app-chat.ts: export flushChatQueue wrapper for lifecycle use - app-lifecycle.ts: restore queue from localStorage on reconnect + auto-drain - app-render.ts: restore queue on session switch Fixes openclaw#51549
Greptile SummaryThis PR persists
Confidence Score: 1/5Not safe to merge — the PR will fail to compile due to a missing function rename. A P0 build-breaking error (undefined
Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/app-chat.ts
Line: 396-398
Comment:
**`flushChatQueueInternal` is never defined — build will fail**
The exported wrapper calls `flushChatQueueInternal`, but that name does not exist anywhere in this file. The private async implementation is still declared as `async function flushChatQueue` on line 349, so TypeScript will emit two errors: "Duplicate function implementation" for the name collision and "Cannot find name 'flushChatQueueInternal'" for the undefined reference. Any call site that reaches `flushChatQueue` (including `sendChatMessageNow` line 224 and `flushChatQueueForEvent` on line 650) will fail.
The private function needs to be renamed to `flushChatQueueInternal` (including the recursive call on line 379).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/ui/app-lifecycle.ts
Line: 70-72
Comment:
**Drain guard `host.connected` is always false here — restored queue never sent**
`handleConnected` is the LitElement `connectedCallback`, called when the component mounts to the DOM. At this point the gateway WebSocket has not yet been established — `connectGateway` is only invoked later inside the `bootstrapReady.finally()` callback. `host.connected` is therefore `false` (or `undefined`) when this branch is evaluated, so `flushChatQueue` is never called and the restored items will sit in memory indefinitely without being drained.
The drain should be triggered wherever the gateway fires its "connected" event and sets `host.connected = true`, not here in the DOM lifecycle hook.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: ui/src/ui/chat-queue-persistence.ts
Line: 14-27
Comment:
**Attachment `dataUrl` fields can exhaust localStorage quota silently**
`ChatQueueItem` carries an `attachments` array whose `dataUrl` fields are base64-encoded blobs. For a single image these can easily be several megabytes, which exceeds localStorage's ~5 MB budget for the entire origin. `storage.setItem` will throw a `QuotaExceededError`, caught silently here, so the queue is never actually persisted — reproducing the original data-loss bug for users with attachments in their queue.
Consider stripping attachment data before serialising (persist only metadata such as `id`, `mimeType`, `fileName`) or checking the serialized size before writing and falling back gracefully.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(webchat): persist chat queue to loca..." | Re-trigger Greptile |
| export function flushChatQueue(host: ChatHost): void { | ||
| void flushChatQueueInternal(host); | ||
| } |
There was a problem hiding this comment.
flushChatQueueInternal is never defined — build will fail
The exported wrapper calls flushChatQueueInternal, but that name does not exist anywhere in this file. The private async implementation is still declared as async function flushChatQueue on line 349, so TypeScript will emit two errors: "Duplicate function implementation" for the name collision and "Cannot find name 'flushChatQueueInternal'" for the undefined reference. Any call site that reaches flushChatQueue (including sendChatMessageNow line 224 and flushChatQueueForEvent on line 650) will fail.
The private function needs to be renamed to flushChatQueueInternal (including the recursive call on line 379).
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-chat.ts
Line: 396-398
Comment:
**`flushChatQueueInternal` is never defined — build will fail**
The exported wrapper calls `flushChatQueueInternal`, but that name does not exist anywhere in this file. The private async implementation is still declared as `async function flushChatQueue` on line 349, so TypeScript will emit two errors: "Duplicate function implementation" for the name collision and "Cannot find name 'flushChatQueueInternal'" for the undefined reference. Any call site that reaches `flushChatQueue` (including `sendChatMessageNow` line 224 and `flushChatQueueForEvent` on line 650) will fail.
The private function needs to be renamed to `flushChatQueueInternal` (including the recursive call on line 379).
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed: renamed private flushChatQueue → flushChatQueueInternal (including recursive call). Thanks for catching this.
| if (restored.length > 0 && host.connected) { | ||
| flushChatQueue(host as unknown as ChatHost); | ||
| } |
There was a problem hiding this comment.
host.connected is always false here — restored queue never sent
handleConnected is the LitElement connectedCallback, called when the component mounts to the DOM. At this point the gateway WebSocket has not yet been established — connectGateway is only invoked later inside the bootstrapReady.finally() callback. host.connected is therefore false (or undefined) when this branch is evaluated, so flushChatQueue is never called and the restored items will sit in memory indefinitely without being drained.
The drain should be triggered wherever the gateway fires its "connected" event and sets host.connected = true, not here in the DOM lifecycle hook.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/app-lifecycle.ts
Line: 70-72
Comment:
**Drain guard `host.connected` is always false here — restored queue never sent**
`handleConnected` is the LitElement `connectedCallback`, called when the component mounts to the DOM. At this point the gateway WebSocket has not yet been established — `connectGateway` is only invoked later inside the `bootstrapReady.finally()` callback. `host.connected` is therefore `false` (or `undefined`) when this branch is evaluated, so `flushChatQueue` is never called and the restored items will sit in memory indefinitely without being drained.
The drain should be triggered wherever the gateway fires its "connected" event and sets `host.connected = true`, not here in the DOM lifecycle hook.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed: removed the drain trigger from handleConnected (which fires before connectGateway). Moved it to onHello in app-gateway.ts, called unconditionally after host.connected = true. This ensures the queue drains on: (1) initial page load, (2) normal reconnect, and (3) seq-gap reconnect. Thanks for the precise analysis.
| } | ||
| try { | ||
| const key = `${CHAT_QUEUE_STORAGE_KEY_PREFIX}${sessionKey}`; | ||
| if (queue.length === 0) { | ||
| // Drain complete — remove the stale entry so localStorage doesn't grow indefinitely. | ||
| storage.removeItem(key); | ||
| return; | ||
| } | ||
| const value = JSON.stringify({ items: queue, ts: Date.now() }); | ||
| storage.setItem(key, value); | ||
| } catch { | ||
| // Best-effort: localStorage may be unavailable (e.g. private browsing). | ||
| } | ||
| } |
There was a problem hiding this comment.
dataUrl fields can exhaust localStorage quota silently
ChatQueueItem carries an attachments array whose dataUrl fields are base64-encoded blobs. For a single image these can easily be several megabytes, which exceeds localStorage's ~5 MB budget for the entire origin. storage.setItem will throw a QuotaExceededError, caught silently here, so the queue is never actually persisted — reproducing the original data-loss bug for users with attachments in their queue.
Consider stripping attachment data before serialising (persist only metadata such as id, mimeType, fileName) or checking the serialized size before writing and falling back gracefully.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/chat-queue-persistence.ts
Line: 14-27
Comment:
**Attachment `dataUrl` fields can exhaust localStorage quota silently**
`ChatQueueItem` carries an `attachments` array whose `dataUrl` fields are base64-encoded blobs. For a single image these can easily be several megabytes, which exceeds localStorage's ~5 MB budget for the entire origin. `storage.setItem` will throw a `QuotaExceededError`, caught silently here, so the queue is never actually persisted — reproducing the original data-loss bug for users with attachments in their queue.
Consider stripping attachment data before serialising (persist only metadata such as `id`, `mimeType`, `fileName`) or checking the serialized size before writing and falling back gracefully.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed: on QuotaExceededError, strip attachment dataUrl fields (keep id/mimeType/fileName metadata) and retry. If even the slimmed data exceeds quota, give up silently. Thanks for catching this edge case.
1. flushChatQueueInternal: rename private function from flushChatQueue (the exported wrapper referenced undefined flushChatQueueInternal) 2. Dead drain in handleConnected: host.connected is always false there (handleConnected fires before connectGateway is called). Move drain trigger to onHello in app-gateway.ts, after host.connected = true. Always call flushChatQueueForEvent so queue drains on initial load AND reconnect. 3. QuotaExceededError: catch specifically and retry with attachment dataUrl fields stripped (keep id/mimeType/fileName only). Fall back silently if even slimmed data exceeds quota.
|
Codex review: found issues before merge. Summary Reproducibility: yes. A static reproduction is clear: refresh during an active response with queued follow-ups, restore the queue before gateway hello, then the PR drains after chatRunId is cleared while the old run can still be active; attachment loss follows by queuing a file, refreshing, and replaying metadata without payload bytes. Next step before merge Security Review findings
Review detailsBest possible solution: Implement durable WebChat queue persistence in the shared chat/session lifecycle, with active-run ACK or completion coordination, explicit attachment restore or rejection behavior, reviewed retention semantics, focused tests, and a user-facing changelog entry. Do we have a high-confidence way to reproduce the issue? Yes. A static reproduction is clear: refresh during an active response with queued follow-ups, restore the queue before gateway hello, then the PR drains after chatRunId is cleared while the old run can still be active; attachment loss follows by queuing a file, refreshing, and replaying metadata without payload bytes. Is this the best way to solve the issue? No. localStorage may be part of the fix, but this implementation is not the best path until it uses the shared session helper, coordinates restored drains with active-run state, and defines attachment and privacy behavior. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4655ee803d27. |
Summary
Fixes #51549 — WebChat loses queued messages on browser refresh or session switch.
Problem
chatQueue was stored as a pure in-memory array. On browser refresh or session switch, the array was recreated empty and all queued messages were permanently lost.
Solution
Persists chatQueue to localStorage on every mutation, with automatic restore on reconnect or session switch.
Changes
Notes
Fixes #51549