Fix stale thinking dot placeholders

[franksong2702]

Thinking Path

• Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869 reports that empty three-dot thinking placeholders can remain visible after the agent has moved on to tool calls or final content.
• The live-to-final model treats real provider reasoning as a durable Thinking Card, but an empty pre-stream spinner is only a transient placeholder.
• The bug was in the legacy thinking-row finalizer: it treated the row container class itself as durable content, so empty spinners were finalized instead of removed.
• This PR narrows the content check to actual .thinking-card content and adds a regression test so dot-only placeholders cannot pile up again.

What Changed

• Updated finalizeThinkingCard() so dot-only legacy thinking rows are removed when finalized.
• Added Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869 regression coverage that distinguishes empty spinner placeholders from real Worklog Thinking Cards.
• Added an Unreleased changelog entry.

Why It Matters

• Users should only see the three-dot animation while the agent is actively waiting/thinking.
• Once the turn advances to tools or final content, empty spinner rows should disappear instead of stacking as stale visual noise.
• Real Thinking Cards with provider reasoning remain preserved; this does not change the Worklog / Thinking Card model.

UI Evidence

• Before: Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869 includes the reporter's screenshot showing multiple stale three-dot thinking rows stacked in the message area.
• After / invariant evidence: the new Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869 regression test pins the DOM-finalization behavior that creates the visible fix:
  • dot-only legacy thinking rows have no .thinking-card child and are removed when finalizeThinkingCard() runs;
  • real Worklog Thinking Cards keep their content and only lose live/active markers.
• CI also ran the repository browser smoke check successfully on this PR.
• I did not restart the user's local 8787/8788 runtime or record a provider-specific video for this narrow one-line DOM finalization fix.

Contract Routing

Task type: focused UI regression fix.
Touched areas: static/ui.js, thinking placeholder finalization, regression tests, changelog.
Relevant public docs:

• AGENTS.md
• CONTRIBUTING.md
• docs/CONTRACTS.md
• docs/rfcs/live-to-final-assistant-replies.md
• docs/UIUX-GUIDE.md
• DESIGN.md
  Scope boundaries: no backend schema changes, no Thinking Card redesign, no new animation style.
  Evidence needed before claiming done: syntax checks plus regression coverage for dot-only placeholder removal and real Thinking Card preservation.

Verification

• node --check static/ui.js
• node --check static/messages.js
• git diff --check
• uv run --python /opt/homebrew/bin/python3.12 --with pytest pytest tests/test_issue3869_thinking_dots.py tests/test_ui_tool_call_cleanup.py tests/test_issue1298_cancel_and_activity.py tests/test_regressions.py::test_ui_js_can_upgrade_thinking_spinner_into_live_reasoning_card tests/test_regressions.py::test_messages_js_finalizes_thinking_card_before_tool_card -q
  • Result: 41 passed
• python3 scripts/scope_undef_gate.py
  • Result: skipped locally because eslint is not installed on PATH.
• GitHub CI on this PR:
  • browser-smoke: passed
  • lint: passed
  • test matrix for Python 3.11 / 3.12 / 3.13: passed

Risks / Follow-ups

• Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869 also mentions richer thinking animation inspiration; this PR intentionally does not redesign the animation.
• A future visual polish PR can revisit the spinner animation itself if maintainers want a richer active-thinking indicator.

Fixes #3869

Model Used

OpenAI GPT-5 Codex via Codex CLI, with local shell, GitHub CLI, and CodeGraph context.

[greptile-apps]

Greptile Summary

This PR fixes a one-line logic bug in finalizeThinkingCard() where row.classList.contains('thinking-card-row') was always true (every row is created with that class), causing hasContent to be perpetually truthy and preventing empty spinner rows from ever being removed. The fix replaces the class check with !!row.querySelector('.thinking-card'), which correctly distinguishes a bare spinning placeholder from a row that holds real provider reasoning.

• static/ui.js: Replaces the always-truthy class-presence check with a DOM-content check so dot-only rows are removed on finalization instead of persisting as stale visual noise.
• tests/test_issue3869_thinking_dots.py: Adds a static source-assertion regression test that confirms the new check is present, the old class check is absent, and the real-Thinking-Card preservation path is intact.
• CHANGELOG.md: Adds an Unreleased entry with the issue reference (Thinking animation dots persist as multiple stale rows after agent finishes thinking #3869).

Confidence Score: 4/5

Safe to merge; the one-line change is narrow and correct, and the tests confirm both the empty-spinner removal and real-Thinking-Card preservation paths.

The fix closes a stale-spinner accumulation bug with minimal surface area. Real Thinking Card rows continue to be preserved because they always contain a .thinking-card child; only bare spinner rows (no child) are removed. The regression test is a static source assertion rather than a live DOM test, which works for this specific function but would not catch related rendering regressions elsewhere. The contribution guidelines call for before/after UI evidence on visual changes, which is absent here.

No files require special attention, though a quick browser confirmation that dot rows disappear and real Thinking Cards remain would close the gap left by the missing UI evidence.

Important Files Changed

Filename	Overview
static/ui.js	Single-line fix in finalizeThinkingCard(): replaces a class-presence check (always true) with a DOM-content check, correctly enabling removal of empty spinner rows.
tests/test_issue3869_thinking_dots.py	New regression test that performs static source-text assertions on finalizeThinkingCard. Brace-counting parser is fragile but safe for this function's current structure; covers both empty-spinner removal and real-card preservation.
CHANGELOG.md	Adds an Unreleased entry documenting the stale thinking-dot fix with the issue reference. No issues.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[finalizeThinkingCard called] --> B{isSimplifiedToolCalling?}
    B -- yes --> C[Find live worklog group in liveAssistantTurn]
    C --> D[removeAttribute data-thinking-active / data-live-thinking on all active nodes]
    D --> E[_syncToolCallGroupSummary]
    B -- no --> F[Find thinkingRow by ID]
    F --> G{row exists?}
    G -- no --> H[return]
    G -- yes --> I{row.querySelector '.thinking-card'?}
    I -- null: empty spinner --> J{data-thinking-active === '1'?}
    J -- yes --> K[row.remove / return]
    J -- no --> L[fall through to finalize]
    I -- found: real content --> L
    L --> M[Optionally reset scroll to top]
    M --> N[removeAttribute id + data-thinking-active / return]
    style K fill:#f66,color:#fff
    style I fill:#6af,color:#000

Loading

_{Reviews (1): Last reviewed commit: "Fix stale thinking dot placeholders" | Re-trigger Greptile}

[greptile-apps]

Missing before/after UI evidence per contribution guidelines

AGENTS.md requires "before/after evidence and test relevant desktop, narrow, and mobile states" for UI/UX changes. The PR description explicitly acknowledges that no browser screenshot was captured, citing DOM-only verification. Even for a finalization-only regression, a short screen recording or screenshot pair showing that dot-only rows disappear (and real Thinking Cards still appear) after the change would satisfy the guideline and make the fix independently verifiable.

Context Used: AGENTS.md (source)

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!

[franksong2702]

Thanks, fair point. I updated the PR body with a dedicated UI Evidence section.

For this narrow fix, the before evidence is the reporter screenshot in #3869. The after evidence is the DOM-finalization invariant now pinned by tests/test_issue3869_thinking_dots.py: dot-only legacy thinking rows are removed on finalizeThinkingCard(), while real Worklog Thinking Cards keep their content and only lose live/active markers. CI browser-smoke is also green.

I intentionally did not restart the user's local 8787/8788 runtime or record a provider-specific video for this one-line finalization fix.

[nesquena-hermes]

Pulled the branch into a read-only worktree and read the full finalizeThinkingCard() at static/ui.js:10180 on master plus appendThinking() right below it. The one-line change is correct and the diagnosis in the PR body matches the code.

Why the old check was dead code

The thinking placeholder row is always created with the class thinking-card-row:

// ui.js, appendThinking()
row=document.createElement('div');
row.id='thinkingRow';
row.className='thinking-card-row';

So in the master version:

const hasContent=row.querySelector('.thinking-card') || row.classList.contains('thinking-card-row');

the right-hand classList.contains('thinking-card-row') was unconditionally true for every thinking row — meaning hasContent could never be false, and the if(!hasContent && ... data-thinking-active==='1') row.remove() branch was unreachable. Empty dot-only spinners were never removed, which is exactly the stacking symptom in #3869. The fix narrows the test to the real content marker:

const hasContent=!!row.querySelector('.thinking-card');

.thinking-card is only present once _renderThinkingInto() gets non-empty sanitized text (_thinkingMarkup() emits .thinking-card for real reasoning, and the bare <div class="thinking"><div class="dot">… spinner otherwise — ui.js:10162). So the new check correctly distinguishes a finalized real Thinking Card from a transient spinner.

Scope looks right

This only touches the legacy (!isSimplifiedToolCalling()) branch. The simplified/Worklog branch below it is untouched and still only strips data-thinking-active/data-live-thinking from .agent-activity-thinking nodes without removing real cards, which the second test pins. The data-thinking-active==='1' guard ensures only rows that were actively spinning get removed, so a finalized card (which clears that attribute) is safe.

One note on the tests

tests/test_issue3869_thinking_dots.py is a source-text assertion test — it greps the finalizeThinkingCard body string for the exact new line and the absence of the old class check. That's a reasonable guard against regressing this specific line, but it's brittle to formatting (it asserts the exact const hasContent=!!row.querySelector('.thinking-card'); string with no whitespace tolerance). It does not exercise DOM behavior, so it won't catch a case where the spinner markup itself changes class names. Given the existing repo pattern of source-assertion regression tests this is consistent, just flagging that it's an invariant pin rather than a behavioral test.

Net: minimal, correct, well-scoped fix for #3869. No concerns on the diff itself.

[nesquena-hermes]

Absorbed and shipped in v0.51.341 (Release LE, deployed live) via the release #3886 — your fix and regression test were applied with attribution (your diff was byte-identical to the maintainer-recommended fix on #3869). Closes #3869. Thanks @franksong2702! 🙏