Release v0.51.341 — Release LE (stale thinking-dot placeholder fix #3869/#3876)#3886
Conversation
/#3876) Fixes #3869: empty legacy three-dot thinking spinners piled up as stale rows after the agent finished thinking. The live-to-final redesign (#3401) made the thinking-card-row wrapper class unconditional, which broke finalizeThinkingCard()'s dots-only detection — it treated the wrapper class itself as a "has content" signal, so the dots-only removal branch went dead. Narrow hasContent to the actual .thinking-card element so dots-only spinners are removed on finalize while real Worklog Thinking Cards are preserved. Includes #3869 regression coverage (brace-walks finalizeThinkingCard, asserts the narrowed check + that real thinking cards are not removed). Co-authored-by: franksong2702 <franksong2702@users.noreply.github.com>
|
| Filename | Overview |
|---|---|
| static/ui.js | Single-line fix in finalizeThinkingCard(): removes the always-truthy ` |
| tests/test_issue3869_thinking_dots.py | New regression test using string-matching against the extracted function body; validates fix is present and that the Worklog branch does not call active.remove(). Functionally correct but tightly coupled to exact source formatting. |
| CHANGELOG.md | Adds v0.51.341 release entry and silently removes the ⛔ HELD pending independent review annotation from v0.51.340 without explanation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[finalizeThinkingCard called] --> B{Session guard OK?}
B -- No --> Z[return - wrong session]
B -- Yes --> C{isSimplifiedToolCalling?}
C -- No - legacy path --> D[row = thinkingRow element]
D --> E{row exists?}
E -- No --> Z2[return]
E -- Yes --> F["hasContent = row.querySelector('.thinking-card') != null\nFIX: removed always-truthy OR clause"]
F --> G{hasContent false AND data-thinking-active eq 1?}
G -- Yes dots-only spinner --> H[row.remove - stale placeholder gone]
G -- No real thinking card --> I[reset scrollTop, strip id and active attr]
C -- Yes Worklog path --> J[get liveAssistantTurn and tool-call group]
J --> K{group exists?}
K -- No --> Z3[return]
K -- Yes --> L[strip data-worklog-reason-active]
L --> M["forEach .agent-activity-thinking active\nstrip data-thinking-active + data-live-thinking\nactive.remove NOT called - cards preserved"]
M --> N[syncToolCallGroupSummary]
Reviews (1): Last reviewed commit: "Release v0.51.341 — Release LE (stale th..." | Re-trigger Greptile
| """#3869: an empty legacy thinking row is only a spinner, not durable content.""" | ||
| body = _function_body(UI_JS, "finalizeThinkingCard") | ||
|
|
||
| assert "const hasContent=!!row.querySelector('.thinking-card');" in body | ||
| assert "row.classList.contains('thinking-card-row')" not in body | ||
| assert "if(!hasContent && row.getAttribute('data-thinking-active')==='1'){" in body | ||
| assert "row.remove();" in body | ||
|
|
||
|
|
There was a problem hiding this comment.
Test is tightly coupled to exact source formatting
The assertions match against verbatim substrings like "const hasContent=!!row.querySelector('.thinking-card');" (no spaces around =). Any cosmetic reformat of ui.js — a prettier run, a minifier pass, or even an editor auto-format — will cause these assertions to fail even when the logic is identical. Consider relaxing the check to a regex or normalizing whitespace before asserting, so the test survives formatting-only changes.
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!
| ### Fixed | ||
|
|
||
| - **Legacy thinking placeholders no longer pile up as stale three-dot rows.** Empty pre-stream thinking spinners are now removed when the thinking phase finalizes instead of being mistaken for durable Thinking content. The live-to-final redesign (#3401) made the `thinking-card-row` wrapper class unconditional, which broke `finalizeThinkingCard()`'s dots-only detection (it treated the wrapper class itself as a "has content" signal); the check is now narrowed to the actual `.thinking-card` element so dots-only spinners are removed on finalize while real Worklog Thinking Cards are preserved. (#3869, #3876) | ||
|
|
There was a problem hiding this comment.
Silent removal of the v0.51.340 HELD annotation
The previous entry carried — ⛔ HELD pending independent review, which is dropped here without a note explaining why the hold was cleared (or whether it still applies). If v0.51.340 was genuinely unblocked, a brief inline remark like (independent review complete) preserves the audit trail for anyone reading git history later.
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.341 — Release LE (stale thinking-dot placeholder fix)
Brick-prepass release. Absorbs contributor PR #3876 (@franksong2702) which fixes the confirmed #3401 regression #3869.
Fixed
Root cause
The live-to-final redesign (#3401, v0.51.294) made the
thinking-card-rowwrapper class unconditional on every thinking row, including the dots-only spinner. ButfinalizeThinkingCard()used that same class as its "has real content" signal:Since the wrapper now always carries
thinking-card-row,hasContentwas always truthy, so the dots-only-removal branch went dead and empty spinners stacked across turns. Narrowed to the actual content element:Real Worklog Thinking Cards (which contain a
.thinking-card) are preserved; dots-only spinners are removed on finalize.Files
static/ui.js— 1-line change infinalizeThinkingCard()tests/test_issue3869_thinking_dots.py— NEW regression test (asserts the narrowed check + that real thinking cards are not removed)CHANGELOG.md— release entryGate
pytest -p no:xdist)thinking-card-rowas a content signal (the remaining use at ui.js:5189 is a positional insertion anchor)Closes #3869.
Co-authored-by: franksong2702 franksong2702@users.noreply.github.com