Release v0.51.333 — Release KW (collapse old interim progress notes, #3574)#3848
Conversation
…s live-turn restore The interim-collapse toggle attached its click listener via per-element addEventListener at creation time. snapshotLiveTurnHtmlForSession / restoreLiveTurnHtmlForSession rebuild the live turn via outerHTML/innerHTML on session switch, which strips JS listeners — so a restored toggle was visible but inert and the collapsed interim notes became permanently unreachable for the rest of the turn. Replace with a stateless document-level delegated click handler (_interimCollapseDelegatedClick) that resolves the toggle via closest(), reads state from the DOM (.interim-collapsed) + data-threshold, and works on both freshly-created and innerHTML-restored toggles. Add 4 regression tests pinning the delegated-handler contract. Co-authored-by: rodboev <rodboev@users.noreply.github.com>
…3574) Collapse old interim progress notes after 3 visible during a live turn (#3574, @rodboev). Maintainer fix during re-gate: replaced the per-element toggle listener with a stateless document-level delegated handler so the toggle survives the live-turn DOM restore (Codex caught: innerHTML rebuild dropped the listener → collapsed notes unreachable). Full suite 8303, ESLint/scope-undef CLEAN, Opus SHIP-safe, Codex SAFE-TO-SHIP after fix, collapse + manual-expand-guard + restore-path delegated handler all live-verified. Co-authored-by: rodboev <rodboev@users.noreply.github.com>
|
| Filename | Overview |
|---|---|
| static/messages.js | Adds delegated collapse handler and collapse logic in the interim_assistant SSE handler; the alreadyStreamed reconnect branch does not stamp data-interim="1" on the row it flushes, causing the collapse query to miss it. |
| static/style.css | Adds .interim-collapsed { display:none } and .interim-collapse-toggle styling; straightforward and correct. |
| tests/test_issue2403_interim_collapse.py | Adds 13 static-analysis tests pinning delegated-handler contract, CSS rules, and ordering invariants; well-structured but the alreadyStreamed path is not covered. |
| CHANGELOG.md | Standard release entry for v0.51.333; accurate and complete. |
Comments Outside Diff (1)
-
static/messages.js, line 2447-2453 (link)alreadyStreamedbranch omitsdata-interim="1"on the flushed rowWhen the SSE connection drops mid-event and reconnects,
_freshSegmentistrue(set by the previous_resetAssistantSegmentcall), soensureAssistantRow(true)creates a brand-new DOM row here — butassistantRow.setAttribute('data-interim','1')is never called on that row. The sealed element ends up inblockswithout the attribute, soquerySelectorAll('[data-interim="1"]')in the collapse logic misses it. The result is that the toggle's count is off and that row is never collapsed, even if it should be hidden. The non-alreadyStreamedpath sets the attribute immediately afterensureAssistantRow; the same line is needed here before the flush.
Reviews (1): Last reviewed commit: "Release v0.51.333 — Release KW (collapse..." | Re-trigger Greptile
| if(!toggle){ | ||
| toggle=document.createElement('span'); | ||
| toggle.className='interim-collapse-toggle'; |
There was a problem hiding this comment.
Toggle
<span> is not keyboard-accessible
The collapse toggle is a plain <span> with only a delegated click handler. Keyboard-only users cannot Tab to it (no tabindex="0") and cannot activate it with Enter/Space (no keydown handler or role="button"). Any user navigating without a mouse cannot expand collapsed interim notes at all. Adding role="button" and tabindex="0" to the created element, and extending _interimCollapseDelegatedClick to also listen for keydown with key === 'Enter' || key === ' ', would fix this without touching the delegation pattern.
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!
Release v0.51.333 — Release KW (collapse old interim progress notes)
Absorbs #3574 (@rodboev) — deep-reviewed, rebased, fixed, and shipped.
What ships
During a long live streaming turn the agent emits many interim progress notes that pushed the latest one out of view. Once more than 3 accumulate, the older ones now collapse behind a "Show N earlier updates" toggle (CSS
.interim-collapsed { display:none }) so the most recent notes stay visible. Expanding is sticky — new interim events won't re-hide what the user manually opened.Deep review (Opus + Codex + live browser drive vs master)
snapshotLiveTurnHtmlForSession/restoreLiveTurnHtmlForSessionrebuild the live turn viainnerHTMLon session switch — stripping the listener and leaving collapsed notes permanently unreachable for the rest of the turn._interimCollapseDelegatedClick) that resolves the toggle viaclosest(), reads state from the DOM (.interim-collapsed+data-threshold), and survives the innerHTML restore. Live-verified on the exact restore scenario (markup re-parsed via innerHTML → click still toggles). + 4 regression tests pinning the delegated-handler contract.Greptile flags (all adjudicated)
data-expandedguard works)_flushPendingSegmentRenderwritesassistantBody.innerHTMLonly, not the row attribute)sessions.js("Show N archived", "Show N from other profiles") which the codebase intentionally leaves un-i18n'd — not a gap.Gate
node -c✓ · ESLint runtime CLEAN · scope-undef CLEAN · ruff forward CLEANCo-authored-by: rodboev rodboev@users.noreply.github.com