From 603375d71c3f1dcb9222a0b62d981e6d1841b862 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 3 Jul 2026 15:57:33 -0700 Subject: [PATCH 1/3] fix(#5472): preserve composer draft when a send fails (#5484) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#5472): preserve composer draft when a send fails When a provider/background error aborts a send, send() has already cleared the composer and the persisted draft before /api/chat/start durably accepts the turn. On a start-time throw the turn is never persisted, so the user's typed message is lost and must be retyped. Add _restoreComposerDraftAfterFailedSend(text, sid): restores the typed text into the composer, keeps staged files in S.pendingFiles, and re-persists the draft (via _saveComposerDraftNow) so it also survives a reload. Guarded to no-op when there's nothing to restore and to never clobber a new message the user began typing during the async send window. Wired into the /api/chat/start throw handler's general-error branch. Mirrors the existing _trySteer / _stashClarifyDraft draft-restore idioms. Tests: tests/test_issue5472_preserve_draft_on_failed_send.py — 3 static wiring assertions + 4 node-vm behavioral tests of the helper's branching logic. * fix(#5472): address gate findings — original-payload snapshot, re-stage files, session-guard Codex + Opus gate on the first commit converged on three real edges: 1. /moa and bundle sends rewrite `text` before /api/chat/start, so restoring `text` replayed the transformed payload, not the user's typed command. Fix: capture an immutable _failedSendDraftText snapshot right after the post-flush trim, before any slash rewrite, and restore that. 2. uploadPendingFiles() clears S.pendingFiles=[] before the start call, so the original 'keep staged files' assumption was false — a resend silently dropped attachments. Fix: snapshot S.pendingFiles before the upload drain (_failedSendFilesSnapshot) and re-stage from it (+ renderTray()) on failure. 3. A start failure after a session switch could write the failed old session's draft into the currently-visible composer. Fix: the helper now only mutates the visible composer/tray when sid === the visible session; the sid-keyed _saveComposerDraftNow persist stays unconditional so the draft survives a switch-back/reload. Helper signature is now (draftText, filesSnapshot, sid). Tests expanded to 11 (5 static + 6 node-vm behavioral) covering all three edges: original-vs-mutated payload, re-stage drained attachments, and cross-session no-pollution. * test(#5472): bound send() body slice to LIVE_STREAMS instead of fixed char window The #5472 snapshot-capture lines added ~400 chars near the top of send(), which pushed 'cancelStream' (in the busy-interrupt branch) past the arbitrary [send_idx:send_idx+5000] window that test_1062's structural assertions used — a real CI failure (shard 1, all 3 Pythons) caused by a brittle test, not by the fix logic. Bound both send()-body slices in test_1062 to the actual function extent (up to the 'const LIVE_STREAMS=' decl that immediately follows send()) so future additions near the top of send() don't false-fail these assertions. --------- Co-authored-by: nesquena-hermes --- static/messages.js | 59 ++++++ tests/test_1062_busy_input_modes.py | 14 +- ...issue5472_preserve_draft_on_failed_send.py | 200 ++++++++++++++++++ 3 files changed, 270 insertions(+), 3 deletions(-) create mode 100644 tests/test_issue5472_preserve_draft_on_failed_send.py diff --git a/static/messages.js b/static/messages.js index 8319ff74..191e4b3d 100644 --- a/static/messages.js +++ b/static/messages.js @@ -1216,6 +1216,51 @@ function applySessionTitleUpdate(sid, titleText, options={}){ return true; } +// #5472: when a provider/background error aborts a send, send() has already +// cleared the composer (`$('msg').value=''`), the persisted draft +// (`_clearComposerDraft`), and the staged files (uploadPendingFiles() sets +// `S.pendingFiles=[]`) before the turn was durably accepted server-side. On a +// start-time throw the turn is never persisted, so the user loses the entire +// typed message + attachments and must retype. Restore the ORIGINAL captured +// draft text + staged files so the user can re-send with one key press. +// Mirrors the draft-restore idiom already used by _trySteer (commands.js) and +// _stashClarifyDraft. +// +// `draftText` and `filesSnapshot` are immutable snapshots captured in send() +// BEFORE slash rewrites (/moa, bundles) mutate the payload and BEFORE +// uploadPendingFiles() drains S.pendingFiles — so we restore what the user +// actually typed, not the transformed send payload. +function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid){ + const restore=String(draftText||''); + const files=Array.isArray(filesSnapshot)?filesSnapshot.filter(Boolean):[]; + if(!restore&&!files.length) return false; + // Persist the draft for the failed send's own session so it survives a reload + // regardless of which session is currently visible. Files staged in the + // composer are File objects (not serializable), so only text is persisted; + // the in-memory S.pendingFiles restore below covers the immediate re-send. + if(sid&&typeof _saveComposerDraftNow==='function'){ + try{ _saveComposerDraftNow(sid, restore, []); }catch(_){ } + } + // Only mutate the VISIBLE composer / staged tray when the failed send belongs + // to the session the user is currently looking at — otherwise a background + // send failure would pollute another session's composer. (Codex #5484 catch.) + const visibleSid=(S.session&&S.session.session_id)||null; + if(sid&&visibleSid&&sid!==visibleSid) return false; + const inp=$('msg'); + if(!inp) return false; + // Do not clobber a new message the user began typing during the async window. + if(String(inp.value||'').trim()) return false; + inp.value=restore; + if(typeof autoResize==='function') autoResize(); + if(typeof updateSendBtn==='function') updateSendBtn(); + // Re-stage the originally attached files so a one-key resend keeps them. + if(files.length){ + S.pendingFiles=files; + if(typeof renderTray==='function') renderTray(); + } + return true; +} + async function send(){ // Static guards expect _defaultMessageMode to stay near send() while the actual // read remains in the S.busy branch below. @@ -1248,6 +1293,15 @@ async function send(){ text=$('msg').value.trim(); if(!text&&!S.pendingFiles.length){_sendInProgress=false;_sendInProgressSid=null;return;} + // #5472: snapshot the ORIGINAL user-typed composer state now — before slash + // rewrites (/moa, bundles) mutate `text` and before uploadPendingFiles() + // clears S.pendingFiles. If /api/chat/start throws (turn never durably + // started), _restoreComposerDraftAfterFailedSend() puts this exact text + + // staged files back so the user can re-send without retyping. Captured as an + // immutable snapshot so later reassignments to `text` don't leak into it. + const _failedSendDraftText=text; + const _failedSendFilesSnapshot=Array.isArray(S.pendingFiles)?[...S.pendingFiles]:[]; + // Dismiss handoff hint when user sends a message (resets seen_at). if(S.session&&S.session.session_id&&typeof _dismissHandoffHint==='function'){ _dismissHandoffHint(S.session.session_id); @@ -1677,6 +1731,11 @@ async function send(){ if(!_clarifySessionId || _clarifySessionId===activeSid) hideClarifyCard(true, 'terminal'); S.messages.push({role:'assistant',content:`**Error:** ${errMsg}`}); _queueDrainSid=activeSid;renderMessages();setBusy(false);setComposerStatus(`Error: ${errMsg}`); + // #5472: the send was rejected before the turn was durably started, so the + // composer text + attachments (cleared at send time) would otherwise be + // lost. Put back the ORIGINAL captured draft (not the mutated /moa/bundle + // payload) and re-stage files so the user can re-send without retyping. + _restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid); if(typeof clearOptimisticSessionStreaming==='function') clearOptimisticSessionStreaming(activeSid); // Reconcile with server truth after immediately clearing the optimistic spinner. if(typeof renderSessionList==='function') void renderSessionList(); diff --git a/tests/test_1062_busy_input_modes.py b/tests/test_1062_busy_input_modes.py index b9ae4433..1872dfd0 100644 --- a/tests/test_1062_busy_input_modes.py +++ b/tests/test_1062_busy_input_modes.py @@ -269,15 +269,23 @@ class TestSendBusyBranchDispatch: # The send() function should read window._defaultMessageMode in the busy block send_idx = MESSAGES_JS.find("async function send(") assert send_idx >= 0 - # Look in the first ~3000 chars of send() for the busy mode read - send_body = MESSAGES_JS[send_idx:send_idx + 3000] + # Bound the window to the actual send() body (up to the LIVE_STREAMS decl + # that immediately follows it) rather than an arbitrary char count, so + # unrelated additions near the top of send() don't push the assertion + # target out of a fixed window (#5472 caught this). + send_end = MESSAGES_JS.find("const LIVE_STREAMS=", send_idx) + assert send_end > send_idx, "could not bound send() body" + send_body = MESSAGES_JS[send_idx:send_end] assert "_defaultMessageMode" in send_body, ( "send() must read window._defaultMessageMode in the S.busy branch" ) def test_send_calls_cancel_stream_on_interrupt(self): send_idx = MESSAGES_JS.find("async function send(") - send_body = MESSAGES_JS[send_idx:send_idx + 5000] + assert send_idx >= 0 + send_end = MESSAGES_JS.find("const LIVE_STREAMS=", send_idx) + assert send_end > send_idx, "could not bound send() body" + send_body = MESSAGES_JS[send_idx:send_end] # The interrupt branch must call cancelStream assert "cancelStream" in send_body # And queue before cancel (otherwise the drain has nothing to pick up) diff --git a/tests/test_issue5472_preserve_draft_on_failed_send.py b/tests/test_issue5472_preserve_draft_on_failed_send.py new file mode 100644 index 00000000..d6a903c3 --- /dev/null +++ b/tests/test_issue5472_preserve_draft_on_failed_send.py @@ -0,0 +1,200 @@ +"""Regression coverage for #5472 — preserve the composer draft when a send fails. + +Bug: when a provider/background error aborts a send, ``send()`` in +``static/messages.js`` has already cleared the composer (``$('msg').value=''``), +the persisted draft (``_clearComposerDraft``), AND the staged files +(``uploadPendingFiles()`` sets ``S.pendingFiles=[]``) at send time — before the +turn is durably accepted by ``/api/chat/start``. On a start-time throw the turn +is never persisted, so the user loses the entire typed message + attachments and +must retype. + +Fix: ``send()`` snapshots the ORIGINAL typed text + staged files BEFORE slash +rewrites (/moa, bundles) mutate the payload and BEFORE the upload drains +``S.pendingFiles``. On a start-time throw, +``_restoreComposerDraftAfterFailedSend(text, files, sid)`` restores that exact +snapshot, re-stages the files, and re-persists the draft. It is session-aware +(never pollutes a different session's visible composer) and never clobbers a new +message the user began typing during the async window. + +This module verifies BOTH: + 1. (static) the snapshot capture + wiring into the send-error path, and + 2. (behavioral, via node's ``vm``) the helper's branching logic, including the + three Codex-caught edges: original-vs-mutated payload, dropped attachments, + and cross-session composer pollution. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).parents[1] +MESSAGES_JS = ROOT.joinpath("static", "messages.js").read_text(encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Static wiring assertions +# --------------------------------------------------------------------------- + +def _helper_body() -> str: + start = MESSAGES_JS.find("function _restoreComposerDraftAfterFailedSend(") + assert start != -1, "the _restoreComposerDraftAfterFailedSend helper must exist" + end = MESSAGES_JS.find("\nasync function send(", start) + assert end != -1, "helper must be defined immediately before send()" + return MESSAGES_JS[start:end] + + +def test_helper_has_new_three_arg_signature_and_guards(): + body = _helper_body() + assert "function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid)" in body + # No-op when there is nothing to restore (no text AND no staged files). + assert "if(!restore&&!files.length) return false;" in body + # Session-aware: never mutate a different session's visible composer. + assert "const visibleSid=(S.session&&S.session.session_id)||null;" in body + assert "if(sid&&visibleSid&&sid!==visibleSid) return false;" in body + # Never clobber a message the user began typing during the async window. + assert "if(String(inp.value||'').trim()) return false;" in body + # Restores text and re-stages files. + assert "inp.value=restore;" in body + assert "S.pendingFiles=files;" in body + + +def test_send_captures_immutable_snapshot_before_rewrites_and_upload(): + # The snapshot must be captured right after the post-flush trim, BEFORE the + # busy branch / slash-command rewrites and BEFORE uploadPendingFiles(). + snap_idx = MESSAGES_JS.find("const _failedSendDraftText=text;") + files_idx = MESSAGES_JS.find( + "const _failedSendFilesSnapshot=Array.isArray(S.pendingFiles)?[...S.pendingFiles]:[];" + ) + moa_idx = MESSAGES_JS.find("text=_moaArgs;") + upload_idx = MESSAGES_JS.find("uploaded=await uploadPendingFiles();") + assert snap_idx != -1 and files_idx != -1, "send() must snapshot text + files for #5472" + assert moa_idx != -1 and upload_idx != -1 + # Snapshot happens before both the /moa rewrite and the upload drain. + assert snap_idx < moa_idx, "text snapshot must precede the /moa rewrite of `text`" + assert files_idx < upload_idx, "files snapshot must precede uploadPendingFiles() drain" + + +def test_error_branch_restores_original_snapshot_not_mutated_payload(): + start = MESSAGES_JS.find("S.messages.push({role:'assistant',content:`**Error:** ${errMsg}`});") + assert start != -1, "the /api/chat/start error branch must still push an Error turn" + window = MESSAGES_JS[start:start + 900] + assert ( + "_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid);" + in window + ), "the send-error path must restore the ORIGINAL captured snapshot (not `text`)" + + +def test_send_still_clears_composer_on_the_happy_path(): + assert "$('msg').value='';autoResize();" in MESSAGES_JS + assert "if (activeSid && typeof _clearComposerDraft === 'function') _clearComposerDraft(activeSid);" in MESSAGES_JS + + +# --------------------------------------------------------------------------- +# Behavioral test — actually execute the helper in a JS sandbox +# --------------------------------------------------------------------------- + +def _run_helper_in_node(draft_text, files_snapshot, initial_input, visible_sid, sid="sid-1"): + """Execute _restoreComposerDraftAfterFailedSend in a node vm sandbox.""" + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + + body = _helper_body() + harness = textwrap.dedent( + """ + const state = { + input: {value: %(initial_input)s, resized: false}, + pendingFiles: [], + trayRendered: false, + saved: null, + sendBtnUpdated: false, + }; + const $ = (id) => (id === 'msg' ? state.input : null); + const S = {pendingFiles: state.pendingFiles, session: %(session)s}; + function autoResize(){ state.input.resized = true; } + function updateSendBtn(){ state.sendBtnUpdated = true; } + function renderTray(){ state.trayRendered = true; } + function _saveComposerDraftNow(sid, text, files){ state.saved = {sid, text, files}; } + + %(helper)s + + const ret = _restoreComposerDraftAfterFailedSend(%(draft_text)s, %(files)s, %(sid)s); + console.log(JSON.stringify({ + ret, + inputValue: state.input.value, + resized: state.input.resized, + sendBtnUpdated: state.sendBtnUpdated, + trayRendered: state.trayRendered, + pendingFiles: S.pendingFiles, + saved: state.saved, + })); + """ + ) % { + "initial_input": json.dumps(initial_input), + "session": json.dumps({"session_id": visible_sid} if visible_sid else None), + "helper": body, + "draft_text": json.dumps(draft_text), + "files": json.dumps(files_snapshot), + "sid": json.dumps(sid), + } + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def test_restores_typed_text_into_empty_composer(): + out = _run_helper_in_node("my long message", [], "", visible_sid="sid-1") + assert out["ret"] is True + assert out["inputValue"] == "my long message" + assert out["resized"] is True and out["sendBtnUpdated"] is True + # Draft persisted for reload (text only — File objects aren't serializable). + assert out["saved"] == {"sid": "sid-1", "text": "my long message", "files": []} + + +def test_restores_original_text_not_mutated_moa_payload(): + # The snapshot passed in is the user's ORIGINAL "/moa summarize this", even + # though send() would have rewritten `text` to just "summarize this". + out = _run_helper_in_node("/moa summarize this", [], "", visible_sid="sid-1") + assert out["ret"] is True + assert out["inputValue"] == "/moa summarize this" + + +def test_restages_attachments_that_upload_already_drained(): + files = [{"name": "a.pdf"}, {"name": "b.png"}] + out = _run_helper_in_node("look at these", files, "", visible_sid="sid-1") + assert out["ret"] is True + assert out["pendingFiles"] == files + assert out["trayRendered"] is True + + +def test_restores_when_only_staged_files_remain(): + files = [{"name": "a.pdf"}] + out = _run_helper_in_node("", files, "", visible_sid="sid-1") + assert out["ret"] is True + assert out["pendingFiles"] == files + + +def test_does_not_clobber_a_new_in_progress_draft(): + out = _run_helper_in_node("original failed", [], "something new", visible_sid="sid-1") + assert out["ret"] is False + assert out["inputValue"] == "something new" + + +def test_does_not_pollute_a_different_visible_session(): + # The failed send belongs to sid-1, but the user has switched to sid-2. The + # visible composer must NOT be touched — but the draft is still persisted for + # sid-1 so it survives a switch-back / reload. + out = _run_helper_in_node("failed on old session", [], "", visible_sid="sid-2") + assert out["ret"] is False + assert out["inputValue"] == "" + assert out["pendingFiles"] == [] + assert out["saved"] == {"sid": "sid-1", "text": "failed on old session", "files": []} + + +def test_noop_when_nothing_to_restore(): + out = _run_helper_in_node("", [], "", visible_sid="sid-1") + assert out["ret"] is False + assert out["saved"] is None From 88bc4feffed902ad6d22a1396afefdd1e7c6e158 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 3 Jul 2026 22:58:49 +0000 Subject: [PATCH 2/3] docs(changelog): #5472 preserve composer draft on failed send --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac74888..515ea28f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ ### Fixed +- **A failed send no longer wipes the message you just typed.** When a message couldn't be started because of a provider or background error (auth failure, rate limit, connection refused, etc.), the composer had already been cleared, so the error message appeared with an empty box and you had to retype the whole thing from scratch. Now, when the send is rejected before the turn actually starts, your original text is put back in the composer (and any attached files are re-staged) so you can fix the issue and re-send with one key press. It restores exactly what you typed — not the expanded form of a slash command like `/moa` — never overwrites a new message you started typing while the send was in flight, and won't touch the composer if you've since switched to a different conversation (your draft is still saved to the original one). (#5472) - **Desktop: the transcript no longer jumps back up while the assistant is streaming a reply.** On desktop (which doesn't use the browser's overflow-anchoring), the scroll-position restore keyed off a content-derived anchor that a streaming chunk kept invalidating, so it silently fell back to an absolute scroll position that didn't account for the content growing above the viewport — yanking you upward mid-read. Restore now recovers via the stable per-message index when the content key goes stale, so your reading position holds steady as the answer grows. A genuinely removed message still concedes cleanly (no forced jump to the wrong row), and the separate mobile overflow-anchor fix is untouched. Thanks @allenliang2022. (#5469) - **The live streaming view stays smooth on long answers instead of getting progressively heavier.** With Transparent Stream / compact worklog, the live response area re-rendered the entire growing answer on every streamed frame (and re-serialized the whole turn for its restore snapshot, and re-parsed the full text on every token), so a long reply got quadratically more expensive as it grew — visible as rising CPU and jank late in a response. The live prose row now renders incrementally (the same streaming markdown parser the main chat body already uses, fed only the new text), the restore snapshot is throttled, and the redundant per-token parse is skipped once the answer row exists. Settled messages still render through the canonical full-markdown path (unchanged final output), and every fallback path (parser unavailable, mid-stream edit, session switch) reverts to it cleanly. Part of the response to "WebUI feels laggy." Thanks @ai-ag2026. (#5466, #5455) - **The sidebar no longer keeps polling and rebuilding itself in a hidden background tab.** The 30-second session-list poll and the 60-second relative-time refresh ran unconditionally, so a backgrounded window kept fetching `/api/sessions` and rebuilding a sidebar it couldn't show. Both now pause while the tab is hidden and refresh immediately when you return to it (each poll has its own `visibilitychange` handler), so nothing goes stale — you just stop paying for updates you can't see. (One small timing change: while a tab is hidden *and* a session is streaming, the in-app attention chime / unread badge now fires when you return to the tab rather than mid-hidden; OS-level notifications are unaffected, and browsers already throttled the old hidden-tab poll anyway.) Part of the response to "WebUI feels laggy." Thanks @ai-ag2026. (#5465, #5455) From 5c54fd598c0f8b9bfb76295826f63298cb49cb94 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Fri, 3 Jul 2026 16:36:21 -0700 Subject: [PATCH 3/3] fix(#5472): order draft re-persist after clear + honest happy-path test (#5488) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#5472): harden failed-send draft restore — order persist after clear, honest test Two NIT-level follow-ups from the Opus review of #5484 (both on already-shipped v0.51.842 code; neither is a functional break): 1. Draft clear/save ordering. send() fires an unawaited _clearComposerDraft POST (text:'') at send time, and the failed-send restore fired a separate _saveComposerDraftNow POST (text:). Under HTTP/2 multiplexing those two same-origin writes can be reordered, leaving the SERVER draft empty after a reload (the in-memory composer restore was always fine). send() now captures the clear POST's promise and passes it to the restore helper, which chains the re-persist after the clear resolves (falls back to immediate persist if the promise is absent/rejects). 2. Honest happy-path test. test_send_still_clears_composer_on_the_happy_path asserted a bare $('msg').value='' string that matches ~13 sites, so it would pass even if the MAIN-path clear were removed. It now anchors to the unique main-path _clearComposerDraft line and window-checks the composer wipe immediately precedes it. Tests: +2 (promise-ordering behavioral via node-vm microtask ordering; absent-promise fallback), signature/call assertions updated. 14 in the #5472 file, 93 across the related composer/send/draft/upload surface — all green. * fix(#5472): make deferred draft persist stale-aware (Codex #5488 catch) The promise-ordering fix introduced a regression: the deferred _persist saved the CAPTURED snapshot and its _saveComposerDraftNow clearTimeout cancelled any newer debounced save. So if the user edited the restored composer before the clear POST settled, the delayed persist overwrote their edit with the stale original text (lost on reload). Restructure the helper so the deferred persist is stale-aware at FIRE time: - If the failed session is still visible, re-read the LIVE composer (.value + current S.pendingFiles) instead of the captured snapshot — a post-restore edit is captured, not clobbered. - If we restored the visible session but the user has since switched away, SKIP the stale write entirely (the session-switch save path already saved that session). - Background failure (sid never visible): persist the captured snapshot, since there's no live composer to read — it's the only copy. The visible-composer restore + session guard are unchanged in behavior (moved above the persist). Helper now returns whether it restored the VISIBLE composer. Tests: +3 regression cases (post-restore edit captured; switch-away skip; background snapshot persist), signature/guard assertions updated. 17 in the #5472 file, 87 across the related surface — all green. --- static/messages.js | 82 +++++-- ...issue5472_preserve_draft_on_failed_send.py | 217 +++++++++++++++++- 2 files changed, 267 insertions(+), 32 deletions(-) diff --git a/static/messages.js b/static/messages.js index 191e4b3d..219c6fca 100644 --- a/static/messages.js +++ b/static/messages.js @@ -1230,35 +1230,64 @@ function applySessionTitleUpdate(sid, titleText, options={}){ // BEFORE slash rewrites (/moa, bundles) mutate the payload and BEFORE // uploadPendingFiles() drains S.pendingFiles — so we restore what the user // actually typed, not the transformed send payload. -function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid){ +function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid, clearPromise){ const restore=String(draftText||''); const files=Array.isArray(filesSnapshot)?filesSnapshot.filter(Boolean):[]; if(!restore&&!files.length) return false; - // Persist the draft for the failed send's own session so it survives a reload - // regardless of which session is currently visible. Files staged in the - // composer are File objects (not serializable), so only text is persisted; - // the in-memory S.pendingFiles restore below covers the immediate re-send. - if(sid&&typeof _saveComposerDraftNow==='function'){ - try{ _saveComposerDraftNow(sid, restore, []); }catch(_){ } - } + // Only mutate the VISIBLE composer / staged tray when the failed send belongs // to the session the user is currently looking at — otherwise a background // send failure would pollute another session's composer. (Codex #5484 catch.) const visibleSid=(S.session&&S.session.session_id)||null; - if(sid&&visibleSid&&sid!==visibleSid) return false; - const inp=$('msg'); - if(!inp) return false; - // Do not clobber a new message the user began typing during the async window. - if(String(inp.value||'').trim()) return false; - inp.value=restore; - if(typeof autoResize==='function') autoResize(); - if(typeof updateSendBtn==='function') updateSendBtn(); - // Re-stage the originally attached files so a one-key resend keeps them. - if(files.length){ - S.pendingFiles=files; - if(typeof renderTray==='function') renderTray(); + const belongsToVisible=!(sid&&visibleSid&&sid!==visibleSid); + let restoredVisible=false; + if(belongsToVisible){ + const inp=$('msg'); + // Do not clobber a new message the user began typing during the async window. + if(inp && !String(inp.value||'').trim()){ + inp.value=restore; + if(typeof autoResize==='function') autoResize(); + if(typeof updateSendBtn==='function') updateSendBtn(); + // Re-stage the originally attached files so a one-key resend keeps them. + if(files.length){ + S.pendingFiles=files; + if(typeof renderTray==='function') renderTray(); + } + restoredVisible=true; + } } - return true; + + // Persist the failed session's draft so it survives a reload, ordered AFTER the + // send-time _clearComposerDraft POST (text:'') resolves — otherwise the two + // same-origin writes can be reordered under HTTP/2 multiplexing and leave the + // server draft empty. (Opus #5484 NIT.) Because the persist is deferred, it + // must be STALE-AWARE at fire time (Codex #5488 catch): if the failed session + // is still visible, re-read the LIVE composer so a post-restore edit is + // captured rather than clobbered by the original snapshot; if we restored the + // visible session but the user has since switched away, skip entirely (the + // session-switch save path already persisted this session's composer). + if(sid&&typeof _saveComposerDraftNow==='function'){ + const _persist=()=>{ + try{ + const stillVisible=(S.session&&S.session.session_id)===sid; + if(stillVisible){ + const inp=$('msg'); + const liveText=inp?String(inp.value||''):restore; + _saveComposerDraftNow(sid, liveText, S.pendingFiles?[...S.pendingFiles]:[]); + } else if(!restoredVisible){ + // Background failure (sid was never the visible session): no live + // composer to read, so persist the captured snapshot — it's the only copy. + _saveComposerDraftNow(sid, restore, []); + } + // else: restored the visible composer, then the user switched away — the + // session-switch save path already saved sid's composer; skip stale write. + }catch(_){ } + }; + if(clearPromise&&typeof clearPromise.then==='function') clearPromise.then(_persist,_persist); + else _persist(); + } + + return restoredVisible; } async function send(){ @@ -1550,8 +1579,13 @@ async function send(){ if(!msgText){setComposerStatus('Nothing to send');return;} $('msg').value='';autoResize(); - // Clear persisted composer draft since message was sent. - if (activeSid && typeof _clearComposerDraft === 'function') _clearComposerDraft(activeSid); + // Clear persisted composer draft since message was sent. Capture the promise + // so the #5472 failed-send restore can chain its re-persist AFTER this clear + // resolves — otherwise the two same-origin POSTs (clear text:'' then restore + // text:) can be reordered under HTTP/2 multiplexing and leave the + // server draft empty after a reload. (Opus #5484 NIT.) + let _composerDraftClearPromise=null; + if (activeSid && typeof _clearComposerDraft === 'function') _composerDraftClearPromise=_clearComposerDraft(activeSid); const displayText=_slashDisplayTextOverride||text||(uploaded.length?`Uploaded: ${uploadedNames.join(', ')}`:'(file upload)'); const userMsg={role:'user',content:displayText,attachments:uploaded.length?uploadedNames:undefined,_ts:Date.now()/1000}; S.toolCalls=[]; // clear tool calls from previous turn @@ -1735,7 +1769,7 @@ async function send(){ // composer text + attachments (cleared at send time) would otherwise be // lost. Put back the ORIGINAL captured draft (not the mutated /moa/bundle // payload) and re-stage files so the user can re-send without retyping. - _restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid); + _restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid, _composerDraftClearPromise); if(typeof clearOptimisticSessionStreaming==='function') clearOptimisticSessionStreaming(activeSid); // Reconcile with server truth after immediately clearing the optimistic spinner. if(typeof renderSessionList==='function') void renderSessionList(); diff --git a/tests/test_issue5472_preserve_draft_on_failed_send.py b/tests/test_issue5472_preserve_draft_on_failed_send.py index d6a903c3..0610cfcb 100644 --- a/tests/test_issue5472_preserve_draft_on_failed_send.py +++ b/tests/test_issue5472_preserve_draft_on_failed_send.py @@ -48,17 +48,22 @@ def _helper_body() -> str: def test_helper_has_new_three_arg_signature_and_guards(): body = _helper_body() - assert "function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid)" in body + assert "function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid, clearPromise)" in body # No-op when there is nothing to restore (no text AND no staged files). assert "if(!restore&&!files.length) return false;" in body # Session-aware: never mutate a different session's visible composer. assert "const visibleSid=(S.session&&S.session.session_id)||null;" in body - assert "if(sid&&visibleSid&&sid!==visibleSid) return false;" in body + assert "const belongsToVisible=!(sid&&visibleSid&&sid!==visibleSid);" in body # Never clobber a message the user began typing during the async window. - assert "if(String(inp.value||'').trim()) return false;" in body + assert "if(inp && !String(inp.value||'').trim()){" in body # Restores text and re-stages files. assert "inp.value=restore;" in body assert "S.pendingFiles=files;" in body + # The deferred persist is stale-aware: re-reads the LIVE composer when the + # failed session is still visible (Codex #5488 catch), rather than the + # captured snapshot. + assert "const stillVisible=(S.session&&S.session.session_id)===sid;" in body + assert "const liveText=inp?String(inp.value||''):restore;" in body def test_send_captures_immutable_snapshot_before_rewrites_and_upload(): @@ -80,16 +85,46 @@ def test_send_captures_immutable_snapshot_before_rewrites_and_upload(): def test_error_branch_restores_original_snapshot_not_mutated_payload(): start = MESSAGES_JS.find("S.messages.push({role:'assistant',content:`**Error:** ${errMsg}`});") assert start != -1, "the /api/chat/start error branch must still push an Error turn" - window = MESSAGES_JS[start:start + 900] + window = MESSAGES_JS[start:start + 1100] assert ( - "_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid);" + "_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid, _composerDraftClearPromise);" in window ), "the send-error path must restore the ORIGINAL captured snapshot (not `text`)" def test_send_still_clears_composer_on_the_happy_path(): - assert "$('msg').value='';autoResize();" in MESSAGES_JS - assert "if (activeSid && typeof _clearComposerDraft === 'function') _clearComposerDraft(activeSid);" in MESSAGES_JS + # Anchor to the MAIN-path clear specifically (the send-time composer wipe + + # persisted-draft clear), not a bare `$('msg').value=''` string that also + # appears at ~13 other sites (slash returns, bundle-error paths). This + # assertion must actually guard the main send path's clear. (Opus #5484 NIT.) + main_clear = ( + "if (activeSid && typeof _clearComposerDraft === 'function') " + "_composerDraftClearPromise=_clearComposerDraft(activeSid);" + ) + assert main_clear in MESSAGES_JS, "main send path must clear the persisted draft at send time" + # The composer textarea wipe must sit immediately above that clear. + clear_idx = MESSAGES_JS.find(main_clear) + window_before = MESSAGES_JS[clear_idx - 700:clear_idx] + assert "$('msg').value='';autoResize();" in window_before, ( + "the main-path composer wipe must precede the persisted-draft clear" + ) + + +def test_restore_persist_chains_after_the_clear_promise(): + # NIT 1: the restore's re-persist must be ordered AFTER the send-time clear + # POST resolves (avoids an HTTP/2 reorder leaving the server draft empty). + body = _helper_body() + assert "clearPromise" in body, "helper must accept the clear promise for ordering" + assert "clearPromise.then(_persist,_persist)" in body, ( + "re-persist must chain off the clear promise (both fulfill and reject → persist)" + ) + # And send() must pass the captured clear promise into the restore call. + assert "let _composerDraftClearPromise=null;" in MESSAGES_JS + call = ( + "_restoreComposerDraftAfterFailedSend(_failedSendDraftText, " + "_failedSendFilesSnapshot, activeSid, _composerDraftClearPromise);" + ) + assert call in MESSAGES_JS, "send() must pass the clear promise into the restore helper" # --------------------------------------------------------------------------- @@ -197,4 +232,170 @@ def test_does_not_pollute_a_different_visible_session(): def test_noop_when_nothing_to_restore(): out = _run_helper_in_node("", [], "", visible_sid="sid-1") assert out["ret"] is False - assert out["saved"] is None + + +def test_persist_is_ordered_after_the_clear_promise(): + """Behavioral: the re-persist must run AFTER the clear promise resolves. + + Simulates the send-time clear POST as a promise that records the persist + order. The re-persist must observe that the clear has already resolved. + """ + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + body = _helper_body() + harness = textwrap.dedent( + """ + const order = []; + const state = {input: {value: ""}, pendingFiles: []}; + const $ = (id) => (id === 'msg' ? state.input : null); + const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}}; + function autoResize(){} + function updateSendBtn(){} + function renderTray(){} + function _saveComposerDraftNow(sid, text, files){ order.push('persist:' + text); } + + %(helper)s + + // Clear POST resolves on a microtask; record its completion first. + const clearPromise = Promise.resolve().then(() => { order.push('clear'); }); + _restoreComposerDraftAfterFailedSend('hello', [], 'sid-1', clearPromise); + // Flush microtasks, then report ordering. + Promise.resolve().then(() => Promise.resolve()).then(() => { + console.log(JSON.stringify({order})); + }); + """ + ) % {"helper": body} + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + out = json.loads(proc.stdout.strip()) + assert out["order"] == ["clear", "persist:hello"], ( + f"persist must run after the clear resolves, got {out['order']}" + ) + + +def test_persist_still_fires_when_clear_promise_absent(): + """Fallback: with no clear promise, the persist happens immediately (sync).""" + out = _run_helper_in_node("no clear promise", [], "", visible_sid="sid-1") + assert out["ret"] is True + assert out["saved"] == {"sid": "sid-1", "text": "no clear promise", "files": []} + + +def test_deferred_persist_captures_a_post_restore_edit_not_the_stale_snapshot(): + """Codex #5488 regression: if the user edits the restored composer before the + deferred persist fires, the persist must save the EDITED text — not clobber it + with the original failed-send snapshot.""" + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + body = _helper_body() + harness = textwrap.dedent( + """ + let saved = null; + const state = {input: {value: ""}, pendingFiles: []}; + const $ = (id) => (id === 'msg' ? state.input : null); + const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}}; + function autoResize(){} + function updateSendBtn(){} + function renderTray(){} + function _saveComposerDraftNow(sid, text, files){ saved = {sid, text}; } + + %(helper)s + + // Clear POST settles on a microtask; the deferred persist runs after it. + const clearPromise = Promise.resolve(); + _restoreComposerDraftAfterFailedSend('original', [], 'sid-1', clearPromise); + // Synchronously the composer shows the restored text... + const afterRestore = state.input.value; + // ...then the user edits it BEFORE the deferred persist fires. + state.input.value = 'edited after restore'; + Promise.resolve().then(() => Promise.resolve()).then(() => { + console.log(JSON.stringify({afterRestore, saved})); + }); + """ + ) % {"helper": body} + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + out = json.loads(proc.stdout.strip()) + assert out["afterRestore"] == "original", "composer should show the restored text synchronously" + assert out["saved"] == {"sid": "sid-1", "text": "edited after restore"}, ( + f"deferred persist must save the LIVE edited text, not the stale snapshot; got {out['saved']}" + ) + + +def test_deferred_persist_skips_when_user_switched_away_after_restore(): + """Codex #5488 regression: if we restored the visible session then the user + switched to a different session before the deferred persist fires, the stale + persist must be SKIPPED (the session-switch save path already saved it).""" + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + body = _helper_body() + harness = textwrap.dedent( + """ + let saveCalls = []; + const state = {input: {value: ""}, pendingFiles: []}; + const $ = (id) => (id === 'msg' ? state.input : null); + const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}}; + function autoResize(){} + function updateSendBtn(){} + function renderTray(){} + function _saveComposerDraftNow(sid, text, files){ saveCalls.push({sid, text}); } + + %(helper)s + + const clearPromise = Promise.resolve(); + _restoreComposerDraftAfterFailedSend('original', [], 'sid-1', clearPromise); + // User switches to a different session before the deferred persist fires. + S.session = {session_id: 'sid-2'}; + state.input.value = 'draft for sid-2'; + Promise.resolve().then(() => Promise.resolve()).then(() => { + console.log(JSON.stringify({saveCalls})); + }); + """ + ) % {"helper": body} + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + out = json.loads(proc.stdout.strip()) + # No persist for sid-1 with the stale 'original' text, and crucially no write + # of sid-2's live composer under sid-1 (that would corrupt sid-1's draft). + assert out["saveCalls"] == [], ( + f"deferred persist must skip entirely after a switch-away; got {out['saveCalls']}" + ) + + +def test_background_failure_persists_snapshot_since_no_live_composer(): + """A failed send for a NON-visible session (background) has no live composer + to read, so the deferred persist saves the captured snapshot for that sid.""" + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + body = _helper_body() + harness = textwrap.dedent( + """ + let saveCalls = []; + const state = {input: {value: "visible session draft"}, pendingFiles: []}; + const $ = (id) => (id === 'msg' ? state.input : null); + // The visible session is sid-2; the failed send was for sid-1 (background). + const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-2'}}; + function autoResize(){} + function updateSendBtn(){} + function renderTray(){} + function _saveComposerDraftNow(sid, text, files){ saveCalls.push({sid, text}); } + + %(helper)s + + const ret = _restoreComposerDraftAfterFailedSend('bg failed msg', [], 'sid-1', null); + Promise.resolve().then(() => { + console.log(JSON.stringify({ret, saveCalls, visibleUntouched: state.input.value})); + }); + """ + ) % {"helper": body} + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + out = json.loads(proc.stdout.strip()) + assert out["ret"] is False, "a background (non-visible) failure must not report a visible restore" + assert out["visibleUntouched"] == "visible session draft", "the visible composer must be untouched" + assert out["saveCalls"] == [{"sid": "sid-1", "text": "bg failed msg"}], ( + f"background failure must persist the snapshot for its own sid; got {out['saveCalls']}" + )