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']}" + )