From b42b17fbbfdfc5058d20f9d505d60ef839d8dee2 Mon Sep 17 00:00:00 2001 From: Mohamed Habib Date: Fri, 12 Jun 2026 11:10:43 +0300 Subject: [PATCH] fix: address transparent stream review findings --- static/panels.js | 7 +- static/ui.js | 115 ++++++++++++------ ...st_issue3820_chat_activity_display_mode.py | 110 ++++++++++++++++- 3 files changed, 193 insertions(+), 39 deletions(-) diff --git a/static/panels.js b/static/panels.js index 9ec3c586..57c3db79 100644 --- a/static/panels.js +++ b/static/panels.js @@ -6287,9 +6287,12 @@ async function _autosaveAppearanceSettings(payload){ if(saved){ window._sessionJumpButtonsEnabled=!!saved.session_jump_buttons; if(Object.prototype.hasOwnProperty.call(saved,'chat_activity_display_mode')){ + const beforeMode=window._chatActivityDisplayMode; _syncChatActivityDisplayModeControl(saved.chat_activity_display_mode); - if(typeof clearMessageRenderCache==='function') clearMessageRenderCache(); - if(typeof renderMessages==='function') renderMessages({preserveScroll:true}); + if(window._chatActivityDisplayMode!==beforeMode){ + if(typeof clearMessageRenderCache==='function') clearMessageRenderCache(); + if(typeof renderMessages==='function') renderMessages({preserveScroll:true}); + } } if(typeof _applySessionNavigationPrefs==='function') _applySessionNavigationPrefs(); } diff --git a/static/ui.js b/static/ui.js index 38ecd73b..a526e677 100644 --- a/static/ui.js +++ b/static/ui.js @@ -6619,10 +6619,20 @@ function _copyEventToClipboard(row){ let label='event'; if(type==='tool'){ const tc=row._tcData||{}; - label=`tool ${tc.name||row.getAttribute('data-event-name')||'tool'}`; - const parts=[`tool: ${tc.name||''}`]; + const fallbackName=row.getAttribute('data-event-name')||row.getAttribute('data-tool-name')||'tool'; + label=`tool ${tc.name||fallbackName}`; + const parts=[`tool: ${tc.name||fallbackName}`]; if(tc.args&&Object.keys(tc.args).length) parts.push('args: '+JSON.stringify(tc.args,null,2)); if(tc.snippet) parts.push('output:\n'+String(tc.snippet)); + if(parts.length===1){ + const argsText=Array.from(row.querySelectorAll('.tool-card-args .tool-arg-pair')) + .map(pair=>String(pair.textContent||'').trim()) + .filter(Boolean) + .join('\n'); + const outputText=String((row.querySelector('.tool-card-result pre')||{}).textContent||'').trim(); + if(argsText) parts.push('args:\n'+argsText); + if(outputText) parts.push('output:\n'+outputText); + } text=parts.join('\n'); }else if(type==='thinking'){ const pre=row.querySelector('.thinking-card-body pre'); @@ -6657,6 +6667,25 @@ function _copyEventToClipboard(row){ } function _attachCopyButton(header){ if(!header) return null; + const bindCopyButton=(btn)=>{ + if(!btn) return null; + btn.classList.add('transparent-event-copy'); + btn.setAttribute('role','button'); + btn.setAttribute('tabindex','0'); + btn.setAttribute('aria-label',t('copy')||'Copy'); + btn.setAttribute('data-transparent-copy','1'); + btn.title=t('copy')||'Copy'; + const handler=function(ev){ + ev.stopPropagation(); + ev.preventDefault(); + _copyEventToClipboard(header.closest('.transparent-event-row')); + }; + btn.onclick=handler; + btn.onkeydown=function(ev){ + if(ev.key==='Enter'||ev.key===' '){ev.preventDefault();handler(ev);} + }; + return btn; + }; // Reuse ANY existing copy button (handles both .transparent-event-copy // added by this function AND the legacy .thinking-copy-btn baked into the // thinking-card template). Returning the existing one prevents the @@ -6664,28 +6693,12 @@ function _attachCopyButton(header){ const existing=header.querySelector('.transparent-event-copy,.thinking-copy-btn'); if(existing){ // Normalise the class so CSS treats them identically. - if(!existing.classList.contains('transparent-event-copy')){ - existing.classList.add('transparent-event-copy'); - } - return existing; + return bindCopyButton(existing); } - btn=document.createElement('span'); + const btn=document.createElement('span'); btn.className='transparent-event-copy'; - btn.setAttribute('role','button'); - btn.setAttribute('tabindex','0'); - btn.setAttribute('aria-label',t('copy')||'Copy'); - btn.setAttribute('data-transparent-copy','1'); - btn.title=t('copy')||'Copy'; btn.innerHTML=``; - const handler=function(ev){ - ev.stopPropagation(); - ev.preventDefault(); - _copyEventToClipboard(header.closest('.transparent-event-row')); - }; - btn.onclick=handler; - btn.onkeydown=function(ev){ - if(ev.key==='Enter'||ev.key===' '){ev.preventDefault();handler(ev);} - }; + bindCopyButton(btn); // Place the copy button at a FIXED flexbox position regardless of // whether a toggle or status badge is present: always right before // the toggle. The CSS uses flexbox order to keep it visually stable @@ -6718,7 +6731,7 @@ function _setTransparentCardOpen(card, open){ if(header) header.setAttribute('aria-expanded',expanded?'true':'false'); } function _wireTransparentHeaderToggle(header){ - if(!header||header.getAttribute('data-transparent-toggle-bound')==='1') return; + if(!header) return; header.setAttribute('data-transparent-toggle-bound','1'); header.onclick=function(ev){ const target=ev&&ev.target; @@ -6788,6 +6801,16 @@ function _syncTransparentEventControls(turn){ if(blocks.firstChild&&blocks.firstChild.parentNode===blocks) blocks.insertBefore(bar, blocks.firstChild); else blocks.appendChild(bar); } + const expand=bar.querySelector('[data-transparent-expand-all]'); + if(expand){ + expand.onclick=function(ev){ev.stopPropagation();_setTransparentRowsExpanded(this.closest('.assistant-turn'),true);}; + expand.onkeydown=function(ev){if(ev.key==='Enter'||ev.key===' '){ev.preventDefault();ev.stopPropagation();_setTransparentRowsExpanded(this.closest('.assistant-turn'),true);}}; + } + const collapse=bar.querySelector('[data-transparent-collapse-all]'); + if(collapse){ + collapse.onclick=function(ev){ev.stopPropagation();_setTransparentRowsExpanded(this.closest('.assistant-turn'),false);}; + collapse.onkeydown=function(ev){if(ev.key==='Enter'||ev.key===' '){ev.preventDefault();ev.stopPropagation();_setTransparentRowsExpanded(this.closest('.assistant-turn'),false);}}; + } const label=bar.querySelector('.transparent-event-controls-label'); if(label){ label.textContent=_transparentEventCountLabel(toolCount); @@ -6797,6 +6820,22 @@ function _syncTransparentEventControls(turn){ // Wire the Hermes chat name tag toggle for the live turn. _wireTransparentTurnToggle(turn); } +function _rehydrateTransparentStreamDom(root){ + if(!root||!isTransparentStream()) return; + root.querySelectorAll('.assistant-turn').forEach(turn=>{ + _wireTransparentTurnToggle(turn); + _syncTransparentEventControls(turn); + }); + root.querySelectorAll('.transparent-event-row').forEach(row=>{ + const card=row.querySelector('.tool-card,.thinking-card'); + const header=row.querySelector('.tool-card-header,.thinking-card-header'); + if(header){ + _wireTransparentHeaderToggle(header); + _attachCopyButton(header); + } + if(card) _setTransparentCardOpen(card,card.classList.contains('open')); + }); +} function _decorateTransparentEventRow(row, opts){ if(!row) return row; opts=opts||{}; @@ -6969,7 +7008,7 @@ function _setTransparentRowsExpanded(root, expanded){ // data-attribute only — the turn's render path reads it on rebuild. const _transparentTurnCollapsedStates={}; // key: `${sid}:${turnMsgIdx}` → boolean function _wireTransparentTurnToggle(turn){ - if(!turn||turn.getAttribute('data-transparent-turn-toggle-bound')==='1') return; + if(!turn) return; if(!isTransparentStream()) return; const role=turn.querySelector('.msg-role.assistant'); if(!role) return; @@ -8518,6 +8557,7 @@ function renderMessages(options){ if(cached&&cached.msgCount===msgCount&&cached.renderWindowSize===renderWindowSize&&cached.signature===renderSignature){ inner.innerHTML=cached.html; _sessionHtmlCacheSid=sid; + _rehydrateTransparentStreamDom(inner); _wireMessageWindowLoadEarlierButton(); if(typeof _applySessionNavigationPrefs==='function') _applySessionNavigationPrefs(); _scrollAfterMessageRender(preserveScroll, scrollSnapshot); @@ -9343,6 +9383,7 @@ function renderMessages(options){ }); }else{ // ── transparent_stream path: individual expandable event rows ── + const transparentInsertCursors=new Map(); for(const entry of activityOrder){ const event={ ...entry, @@ -9356,7 +9397,18 @@ function renderMessages(options){ const turn=anchorTurn; const blocks=_assistantTurnBlocks(anchorTurn); if(!anchorTurn||!blocks) continue; - const insertBefore=anchorRow&&anchorRow.parentElement===blocks?anchorRow.nextElementSibling:null; + const anchorIsWorklogSource=anchorRow.classList&&anchorRow.classList.contains('assistant-segment-worklog-source'); + const insertAfterCursor=(row)=>{ + const cursor=transparentInsertCursors.get(anchorRow)||anchorRow; + const ref=cursor&&cursor.parentElement===blocks?cursor.nextElementSibling:null; + if(ref&&ref.parentElement===blocks) blocks.insertBefore(row,ref); + else blocks.appendChild(row); + transparentInsertCursors.set(anchorRow,row); + }; + const insertBeforeAnchor=(row)=>{ + if(anchorRow&&anchorRow.parentElement===blocks) blocks.insertBefore(row,anchorRow); + else blocks.appendChild(row); + }; if(event.thinkingText){ const thinkingRow=_decorateTransparentEventRow(_thinkingActivityNode(event.thinkingText,false),{ type:'thinking', @@ -9365,8 +9417,8 @@ function renderMessages(options){ segmentSeq, burstId, }); - if(insertBefore&&insertBefore.parentElement===blocks) blocks.insertBefore(thinkingRow,insertBefore); - else blocks.appendChild(thinkingRow); + if(!anchorIsWorklogSource) insertBeforeAnchor(thinkingRow); + else insertAfterCursor(thinkingRow); } for(const toolCall of cards){ event.toolCall=toolCall; @@ -9378,8 +9430,7 @@ function renderMessages(options){ segmentSeq, burstId, }); - if(insertBefore&&insertBefore.parentElement===blocks) blocks.insertBefore(toolRow,insertBefore); - else blocks.appendChild(toolRow); + insertAfterCursor(toolRow); } _syncTransparentEventControls(turn); } @@ -10278,13 +10329,7 @@ function appendLiveToolCard(tc){ if(isTransparentStream()){ const insertTransparentRow=(row)=>{ const liveFooter=inner.querySelector('#liveRunStatus'); - const previousRows=Array.from(inner.querySelectorAll('.transparent-event-row')); - const previous=previousRows[previousRows.length-1]||null; - if(previous&&previous.parentElement===inner){ - previous.insertAdjacentElement('afterend',row); - }else if(anchor&&anchor.parentElement===inner){ - anchor.insertAdjacentElement('afterend',row); - }else if(liveFooter&&liveFooter.parentElement===inner){ + if(liveFooter&&liveFooter.parentElement===inner){ inner.insertBefore(row,liveFooter); }else{ inner.appendChild(row); diff --git a/tests/test_issue3820_chat_activity_display_mode.py b/tests/test_issue3820_chat_activity_display_mode.py index 117a5555..a0d8f1d0 100644 --- a/tests/test_issue3820_chat_activity_display_mode.py +++ b/tests/test_issue3820_chat_activity_display_mode.py @@ -132,6 +132,110 @@ def test_settings_ui_exposes_chat_activity_display_mode_selector(): assert "settings_desc_chat_activity_display_mode" in I18N_JS +def test_appearance_autosave_rerenders_only_when_activity_mode_changes(): + """Appearance autosave receives the full settings object back from the + server, so the presence of chat_activity_display_mode alone is not a + reason to rebuild the message list. Only an effective mode change should + clear the render cache and re-render messages.""" + start = PANELS_JS.index("async function _autosaveAppearanceSettings(payload)") + end = PANELS_JS.index("window._sessionEndlessScrollEnabled=", start) + autosave_block = PANELS_JS[start:end] + + assert "const beforeMode=window._chatActivityDisplayMode;" in autosave_block + assert "_syncChatActivityDisplayModeControl(saved.chat_activity_display_mode);" in autosave_block + changed_guard = "if(window._chatActivityDisplayMode!==beforeMode){" + assert changed_guard in autosave_block + guarded = autosave_block[autosave_block.index(changed_guard):] + assert "clearMessageRenderCache()" in guarded + assert "renderMessages({preserveScroll:true})" in guarded + + +def test_attach_copy_button_declares_local_button(): + """_attachCopyButton must not leak an implicit window.btn global.""" + start = UI_JS.index("function _attachCopyButton(header)") + end = UI_JS.index("\nfunction _transparentToolDetailHtml", start) + copy_block = UI_JS[start:end] + + assert "const btn=document.createElement('span');" in copy_block + assert "\n btn=document.createElement('span');" not in copy_block + + +def test_transparent_settled_rows_preserve_same_anchor_order(): + """When multiple transparent event groups resolve to the same anchor, each + inserted row must advance a per-anchor cursor instead of recomputing + anchor.nextElementSibling and reversing chronological order.""" + transparent_branch = UI_JS[ + UI_JS.index("// ── transparent_stream path: individual expandable event rows ──"): + UI_JS.index("// Render per-turn duration", UI_JS.index("// ── transparent_stream path: individual expandable event rows ──")) + ] + + assert "const transparentInsertCursors=new Map();" in transparent_branch + assert "const cursor=transparentInsertCursors.get(anchorRow)||anchorRow;" in transparent_branch + assert "transparentInsertCursors.set(anchorRow,row);" in transparent_branch + assert "insertAfterCursor(toolRow);" in transparent_branch + assert "anchorRow.nextElementSibling" not in transparent_branch + + +def test_transparent_settled_reasoning_thinking_stays_before_final_answer(): + """Reasoning-only final assistant messages should keep their Thinking row + above the final answer, matching the live order and Compact Worklog's + beforeAnchor behavior.""" + transparent_branch = UI_JS[ + UI_JS.index("// ── transparent_stream path: individual expandable event rows ──"): + UI_JS.index("// Render per-turn duration", UI_JS.index("// ── transparent_stream path: individual expandable event rows ──")) + ] + + assert "const anchorIsWorklogSource=anchorRow.classList&&anchorRow.classList.contains('assistant-segment-worklog-source');" in transparent_branch + assert "const insertBeforeAnchor=(row)=>{" in transparent_branch + assert "if(!anchorIsWorklogSource) insertBeforeAnchor(thinkingRow);" in transparent_branch + assert "else insertAfterCursor(thinkingRow);" in transparent_branch + + +def test_transparent_live_tool_rows_append_at_turn_end_before_status(): + """Live Transparent Stream rows arrive chronologically, so new rows should + append at the end of the live turn before #liveRunStatus instead of jumping + after the previous transparent row.""" + start = UI_JS.index("function appendLiveToolCard(tc){") + end = UI_JS.index("function clearLiveToolCards()", start) + live_block = UI_JS[start:end] + transparent_start = live_block.index("if(isTransparentStream()){") + transparent_end = live_block.index("if(anchor) _removeEmptyLiveWorklogShells(inner);", transparent_start) + transparent_live = live_block[transparent_start:transparent_end] + + assert "const liveFooter=inner.querySelector('#liveRunStatus');" in transparent_live + assert "inner.insertBefore(row,liveFooter);" in transparent_live + assert "previousRows" not in transparent_live + assert "previous.insertAdjacentElement('afterend',row)" not in transparent_live + + +def test_cached_transparent_html_is_rehydrated_after_restore(): + """Cached HTML restores DOM shape but not property-assigned handlers. The + fast path must rehydrate Transparent Stream controls before returning.""" + cache_start = UI_JS.index("if(cached&&cached.msgCount===msgCount") + cache_end = UI_JS.index("return;", cache_start) + cache_block = UI_JS[cache_start:cache_end] + + assert "_rehydrateTransparentStreamDom(inner);" in cache_block + assert "function _rehydrateTransparentStreamDom(root)" in UI_JS + assert "_wireTransparentTurnToggle(turn);" in UI_JS + assert "_syncTransparentEventControls(turn);" in UI_JS + assert "_wireTransparentHeaderToggle(header);" in UI_JS + assert "_attachCopyButton(header);" in UI_JS + + +def test_transparent_existing_copy_buttons_are_rebound_after_cache_restore(): + """The serialized cached DOM keeps copy button elements but loses onclick + properties, so _attachCopyButton must bind existing buttons too.""" + start = UI_JS.index("function _attachCopyButton(header)") + end = UI_JS.index("\nfunction _transparentToolDetailHtml", start) + copy_block = UI_JS[start:end] + + assert "const bindCopyButton=(btn)=>{" in copy_block + assert "return bindCopyButton(existing);" in copy_block + assert "bindCopyButton(btn);" in copy_block + assert "const fallbackName=row.getAttribute('data-event-name')||row.getAttribute('data-tool-name')||'tool';" in UI_JS + + def test_transparent_event_row_pre_3820_visual_rhythm(): """Restore the pre-#3820 visual rhythm: collapsed rows are flat list-items (no border) so the stack reads as a clean list, and tool @@ -428,8 +532,10 @@ def test_copy_button_position_is_stable_and_dedup_handles_legacy_template(): # Dedup: _attachCopyButton checks for both .transparent-event-copy # and .thinking-copy-btn. assert "'.transparent-event-copy,.thinking-copy-btn'" in UI_JS - # The function normalises the class so CSS treats them identically. - assert "existing.classList.add('transparent-event-copy')" in UI_JS + # The function normalises and rebinds existing buttons so cached HTML + # restores keep copy behavior. + assert "btn.classList.add('transparent-event-copy')" in UI_JS + assert "return bindCopyButton(existing);" in UI_JS def test_increased_transparency_and_softened_edges():