Release v0.51.342 — Release LF (transcript + sidebar reliability: #3875 + #3887 + #3831)#3889
Conversation
|
| Filename | Overview |
|---|---|
| static/ui.js | Adds a 57-line defensive fail-safe block at the end of renderMessages() that ensures no settled assistant turn renders with zero visible content; logic is well-scoped (excludes liveAssistantTurn), idempotent across re-renders, and runs before the session HTML cache is populated. |
| tests/test_issue3875_blank_transcript_failsafe.py | Three structural regression tests verify the fail-safe exists and that key string anchors are present in renderMessages(); intentionally documented as static source-structure assertions rather than live DOM tests. |
| CHANGELOG.md | Packages the v0.51.342 release header and adds the #3875 changelog entry; the other two entries (#3887, #3831) were already in [Unreleased] from prior PRs and are being promoted into this release tag. |
Reviews (3): Last reviewed commit: "docs(ui): clarify revealed-flag intent i..." | Re-trigger Greptile
| assert "Fail-safe invariant (#3875)" in body, ( | ||
| "the #3875 no-blank-turn fail-safe is missing from renderMessages" | ||
| ) | ||
|
|
||
|
|
||
| def test_failsafe_reveals_folded_worklog_for_blank_turns(): | ||
| """The fail-safe must expand the folded Worklog group when a turn has no visible content.""" | ||
| body = _function_body(UI_JS, "renderMessages") | ||
| # It must scan turns and skip any turn that already has visible content. | ||
| assert "_turnHasVisibleContent" in body | ||
| # A turn is only acted on when it lacks visible content (the skip-guard). | ||
| assert "if(_turnHasVisibleContent(turn)) continue;" in body | ||
| # The reveal action removes the collapsed class on the Worklog group. | ||
| assert "tool-call-group-collapsed" in body | ||
| assert "removeAttribute('aria-hidden')" in body | ||
|
|
||
|
|
||
| def test_failsafe_preserves_collapsed_worklog_when_visible_answer_exists(): | ||
| """The fail-safe must NOT touch turns that already render visible content. | ||
|
|
||
| The skip-guard (`if(_turnHasVisibleContent(turn)) continue;`) is what preserves the | ||
| intended collapsed-Worklog UX: a turn with any visible answer is left untouched, so | ||
| this fix only ever ADDS visibility to otherwise-blank turns and can never re-expand a | ||
| Worklog the user expects collapsed. | ||
| """ | ||
| body = _function_body(UI_JS, "renderMessages") | ||
| # The visible-content check skips worklog-source (folded) + anchor-only placeholder | ||
| # segments, and treats any other non-empty segment as "visible". | ||
| failsafe = body[body.find("Fail-safe invariant (#3875)") :] | ||
| assert "assistant-segment-worklog-source" in failsafe | ||
| assert "assistant-segment-anchor" in failsafe | ||
| # The live turn drives its own state and must be excluded from the sweep. | ||
| assert "liveAssistantTurn" in failsafe |
There was a problem hiding this comment.
Tests are anchored to string literals, not behaviour
All three tests pass as long as the key identifier strings (_turnHasVisibleContent, tool-call-group-collapsed, Fail-safe invariant (#3875), etc.) appear somewhere inside the renderMessages body — they cannot catch a logic inversion, a wrong selector, or the revealed=true being moved inside the if(collapsed) branch. The docstring acknowledges this trade-off ("static source-structure assertions"), which is reasonable for a quick regression anchor, but it is worth noting that any refactor that renames the inner helper or rewrites the comment tag would silently break the gate without a functional regression, while a logic bug that preserves the string content would silently pass.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
nesquena
left a comment
There was a problem hiding this comment.
Review — end-to-end ✅ (clean approval, no fixes needed)
Independent review of the P0 blank-transcript brick fix for #3875 — sessions rendered as a column of date separators with no message bodies because the Worklog redesign (#3401) hid intermediate assistant segments via assistant-segment-worklog-source when a turn's only content was folded into a collapsed Worklog group.
Traced against upstream hermes-agent
Pulled a fresh nousresearch/hermes-agent tarball at review time. The blank-transcript rendering bug is purely WebUI-side DOM logic — the agent's message stream is unchanged; the WebUI just decides what to show. No cross-tool change needed. ✓
Root cause and fix
The Worklog redesign (#3401) marks assistant segments folded into a collapsed Worklog group with class assistant-segment-worklog-source → CSS display:none. This is correct WHEN a visible final answer also exists on the same turn. But when a turn's ONLY content is folded (autonomous/interrupted run with empty final assistant message, OR reload where S.toolCalls didn't hydrate so the Worklog card has no expandable tool steps), every segment is hidden → turn paints as nothing → transcript stack of bare date separators.
Fail-safe at ui.js:8729-8785: at the end of renderMessages(), iterate every settled .assistant-turn; for any turn with zero visible .assistant-segment content, escalate through two recovery levels:
- Expand collapsed Worklog groups — find
.tool-worklog-group, .tool-call-groupin the turn; if any has non-empty text content, removetool-call-group-collapsed, addopen, setaria-expanded="true"on the summary. - Last resort — un-hide
assistant-segment-worklog-sourcesegments — if no usable Worklog group was found, the hidden source segments still carry the real text. Remove the worklog-source class and thearia-hiddenattribute. Nothing is lost.
Critical correctness invariants — verified by reading the gate logic
The fail-safe is carefully scoped so it can NEVER touch a turn whose intended display is "collapsed Worklog + visible answer":
const _turnHasVisibleContent=(turn)=>{
const segs=turn.querySelectorAll('.assistant-segment');
for(const seg of segs){
if(seg.classList.contains('assistant-segment-worklog-source')) continue;
if(seg.classList.contains('assistant-segment-anchor')) continue;
if((seg.textContent||'').trim()) return true;
}
return false;
};
for(const turn of inner.querySelectorAll('.assistant-turn')){
if(turn.id==='liveAssistantTurn') continue; // live turn drives its own state
if(_turnHasVisibleContent(turn)) continue;
// ... reveal
}Three guards:
- Live turn excluded by
id==='liveAssistantTurn'— the live turn drives its own state during a stream; the fail-safe never fights it. - Any visible segment skips the turn entirely — the worklog-source and anchor placeholders are explicitly excluded from the visible-content check, so a turn with at least one non-folded, non-anchor segment with painted text is left alone. The intended collapsed-Worklog UX is fully preserved.
- Empty groups can't help —
if(!(group.textContent||'').trim()) continue;skips groups with no usable text, escalating to the un-hide path only when needed.
Idempotence verified: re-running the failsafe on a DOM where it has already revealed turns is a no-op (revealed turns now have visible content; the gate skips them).
Tests
tests/test_issue3875_blank_transcript_failsafe.py — 3 structural tests, all pass:
test_render_messages_has_blank_turn_failsafe— the fail-safe code block exists in renderMessagestest_failsafe_reveals_folded_worklog_for_blank_turns— the Worklog-expansion + un-hide patterns are presenttest_failsafe_preserves_collapsed_worklog_when_visible_answer_exists— the visible-content gate is present (regression-lock against future "always expand" simplification)
Structural / source-grep tests are the right shape here — the actual DOM behavior would require a real browser harness, but the gate's correctness reduces to "we only act when no visible content exists" which the structural tests pin. The negative-form test (don't strip class when answer exists) is the load-bearing one.
Full suite: 8291 passed / 125 skipped / 0 PR-introduced failures in 154s (excludes pre-existing test_workspace_git.py::test_git_status_ignores_crlf_only_worktree_noise darwin CRLF failure on master).
CI: all green — browser-smoke + lint + 9 test shards.
Edge-case matrix
| Scenario | Behavior |
|---|---|
| Normal turn with visible answer + collapsed Worklog | Failsafe sees visible segment → skips turn → Worklog stays collapsed ✅ |
| Bug case: turn's only content folded into collapsed Worklog | Failsafe sees no visible content → expands Worklog group → content visible ✅ |
| Worse case: Worklog group is empty + only content is in worklog-source | Failsafe escalates to un-hide path → source class removed → content visible ✅ |
| Live streaming turn | Excluded by id==='liveAssistantTurn' → failsafe doesn't fight active stream ✅ |
| Empty turn (no content anywhere) | No groups to expand, no worklog-source segments either → turn stays blank (correct: no content to reveal) ✅ |
| Re-render after failsafe already ran | Revealed turns now have visible content → gate skips → idempotent ✅ |
| Mid-stream re-render (S.busy=true) | The Worklog rebuild guard at ui.js:8313 already preserves prior turn worklogs (#3401 deep-review fix); this failsafe sweeps after that rebuild → consistent ✅ |
Other audit — things that are correct already
- Security: zero new surface. Pure DOM class manipulation on already-rendered content.
- Performance: O(turns × segments-per-turn); only fires for blank turns (rare); per-render bounded.
- Comment quality at ui.js:8729-8745 is exemplary: explains both the bug class AND why the live-turn exclusion is required (Opus advisor note about historical blank turns re-painting during follow-up streams).
- The two-level escalation (Worklog group expand → worklog-source un-hide) covers both the common path (groups exist) and the edge path (groups didn't build due to hydration race), so the failsafe handles both reported failure modes.
- CHANGELOG entry correctly cites #3875 and explains both the symptom and the fix shape.
Minor observations (non-blocking)
- The failsafe runs after every render. A future optimization could short-circuit when the turn count hasn't changed since the last failsafe sweep, but the bounded work per render makes this premature.
- The Worklog redesign is recent enough that more class-state edge cases may surface; this PR is the right place to land a defensive sweep rather than relying on every Worklog code path to handle the edge case independently.
Recommendation
✅ Approved clean. Parked at approval — ready for the release agent's merge/tag pipeline.
P0 brick fix with the right shape: a fail-safe at the end of the render pipeline that only ever ADDS visibility to invisible turns, never strips visibility from intentional collapsed-Worklog displays. The live-turn exclusion + the "any visible segment" gate make it impossible to fight the intended UX. The structural tests pin the gate logic (positive AND negative forms) so a future refactor can't silently strip the safety net. Ship.
Address greptile review on PR #3889: the 'revealed' flag means 'turn has a visible non-empty Worklog group' not 'we just expanded one'. An already-open non-empty group is itself visible, so the last-resort un-hide is correctly skipped. Comment-only; no behavior change.
|
Addressed — added a clarifying comment at this site. You're right that |
Fixes #3875: chat transcript rendering as only a stack of date separators with no message bodies. The live-to-final/Worklog redesign (#3401) folds intermediate assistant segments into a collapsed Worklog and hides the source segment; when a turn's ONLY content is folded into a collapsed Worklog (empty final assistant message from an interrupted/autonomous run, or a reload where S.toolCalls did not hydrate so the Worklog has no expandable steps), every segment is hidden and the turn paints blank — leaving a bare column of date dividers. Adds a defensive fail-safe invariant at the end of renderMessages(): a settled assistant turn never renders with zero visible content. Blank turns get their folded Worklog expanded (or hidden segments un-hidden as a last resort). Turns with any visible answer are untouched, preserving the intended collapsed-Worklog UX. Reproduced + verified fixed in an isolated browser (clean Chrome profile to defeat the ?v= asset-cache); RED on master (blank 'Worklog' chip), GREEN with the fix (Worklog expanded, content visible). Includes #3875 structural regression coverage.
Address greptile review on PR #3889: the 'revealed' flag means 'turn has a visible non-empty Worklog group' not 'we just expanded one'. An already-open non-empty group is itself visible, so the last-resort un-hide is correctly skipped. Comment-only; no behavior change.
b07004c to
373476b
Compare
Release v0.51.342 — Release LF (transcript + sidebar reliability)
Consolidated release of three render/sidebar reliability fixes, all nesquena APPROVED. The other two (#3887, #3831) already merged to master; this release PR carries the #3875 brick fix and folds all three into one tagged v0.51.342 CHANGELOG section.
Fixed
S.toolCallsdidn't hydrate), every segment was hidden and the turn painted blank.renderMessages()now enforces a fail-safe invariant: a settled assistant turn never renders zero visible content — its folded Worklog is expanded (or hidden segments un-hidden) when otherwise blank. Turns with a visible answer are untouched. Reproduced RED→GREEN in a live browser. (this PR)idx_messages_sessionindex; WebUI now primes it withCREATE INDEX IF NOT EXISTS(13.3s → 0.009s self-heal). (fix(sidebar): prime idx_messages_session before CLI-session scan (#3887) #3888, merged)session.messages. (fix(session): retire stale truncation watermark on new committed turn (#3831) #3890, merged)This PR's diff
static/ui.js— +63 lines, the BUG: Messages stopped displaying correctly after update to .337. Update to .340 does not fix it #3875 fail-safe block at the end ofrenderMessages()(incl. greptile-requested clarifying comment on therevealedflag)tests/test_issue3875_blank_transcript_failsafe.py— NEW structural regression test (3 cases)CHANGELOG.md— consolidated v0.51.342 release entry covering all three fixesGate (this PR)
!S.busygate refinement)revealedflag → addressed with a clarifying commentCloses #3875.