Skip to content

fix(tui): stop one bad event from dropping the rest of a flush batch - #13125

Open
johnnyeric wants to merge 1 commit into
mainfrom
johnnyeric/tui-turn-end-batch-drop
Open

fix(tui): stop one bad event from dropping the rest of a flush batch#13125
johnnyeric wants to merge 1 commit into
mainfrom
johnnyeric/tui-turn-end-batch-drop

Conversation

@johnnyeric

@johnnyeric johnnyeric commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

What

Fixes the TUI stall where a turn visibly finishes (footer shows the completed duration) but the prompt keeps spinning and the transcript stops updating.

Why

The SSE client batches events and flush() clears the queue before iterating, emitting each event unguarded inside a single Solid batch(). The legacy message.removed / message.part.removed handlers in sync.tsx called search(...) on store.message[...] / store.part[...] without checking they exist — for a session this TUI never hydrated (background-session cleanup, revert, another window), that is undefined and search throws reading items.length. One such event mid-batch permanently dropped every later event in that flush, including the idle status the spinner depends on.

Changes:

  • sync.tsx: guard the legacy message.removed / message.part.removed handlers with the same missing-collection checks the .1 handlers already have, and handle session.idle by resetting session_status to idle.
  • sdk.tsx: isolate event dispatch per handler and per event so one throwing handler logs (tui event handler failed) instead of dropping the rest of the batch or killing the SSE loop.
  • Tests reproduce both symptoms on the existing sync fixture; both fail without the fix.

@johnnyeric
johnnyeric marked this pull request as ready for review August 17, 2026 12:20
Comment thread packages/tui/src/context/sdk.tsx Outdated
@kilo-code-bot

kilo-code-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The PR was rewritten to a single commit (d0b8152d0b). Per-handler isolation in emit still prevents one throwing subscriber from aborting the rest of a Solid batch(), and the missing-collection guards on message.removed / message.part.removed match the existing .1 handlers. session.idle correctly maps to { type: "idle" } on SessionStatus. Tests live under packages/tui/test/kilocode/ and exercise the real sync fixture. Changeset is user-facing. No memory leaks introduced.

Files Reviewed (4 files)
  • packages/tui/src/context/sdk.tsx
  • packages/tui/src/context/sync.tsx
  • packages/tui/test/kilocode/sync-batch-drop.test.tsx
  • .changeset/tui-turn-end-batch-drop.md
Previous Review Summaries (3 snapshots, latest commit 5975641)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 5975641)

Status: No Issues Found | Recommendation: Merge

The new commit 5975641ae3 moves sync-batch-drop.test.tsx from packages/tui/test/cli/cmd/tui/ to packages/tui/test/kilocode/ and updates the fixture imports. The relative paths (../fixture/fixture, ../cli/cmd/tui/sync-fixture) resolve correctly from the new location. Dropping the kilocode_change - new file marker is appropriate because the file now lives under a kilocode path. No other files changed in this increment; the previous suggestion (redundant per-event try/catch in flush) remains resolved. No memory leaks introduced.

Files Reviewed (1 file)
  • packages/tui/test/kilocode/sync-batch-drop.test.tsx - 0 issues (moved; import paths verified)

Previous review (commit 71907a6)

Status: No Issues Found | Recommendation: Merge

The new commit 71907a6b removes the redundant per-event try/catch in flush, resolving the previous review's only suggestion. The removal is safe: emit still isolates each handler with its own try/catch (logging tui event handler failed), so a throwing handler cannot escape and abort the Solid batch(). Dropping the kilocode_change markers here also shrinks the diff against upstream. The existing tests exercise the real event path through emit and do not depend on the removed flush-level guard. No memory leaks introduced — no new subscriptions, timers, or retained references.

Files Reviewed (4 files)
  • packages/tui/src/context/sdk.tsx - 0 issues (incremental change verified; previous suggestion resolved)
  • packages/tui/src/context/sync.tsx - 0 issues
  • packages/tui/test/cli/cmd/tui/sync-batch-drop.test.tsx - 0 issues
  • .changeset/tui-turn-end-batch-drop.md - 0 issues

Previous review (commit 71de80b)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
packages/tui/src/context/sdk.tsx 72 Per-event try/catch in flush is redundant now that emit isolates per-handler errors; consider dropping the extra layer.
Files Reviewed (4 files)
  • packages/tui/src/context/sdk.tsx - 1 issue
  • packages/tui/src/context/sync.tsx - 0 issues (missing-collection guards match the .1 handlers; session.idle case matches the SessionStatus type)
  • packages/tui/test/cli/cmd/tui/sync-batch-drop.test.tsx - 0 issues (tests exercise the real sync fixture, no mocks)
  • .changeset/tui-turn-end-batch-drop.md - 0 issues (user-facing, correct patch scope)

The root-cause fix is sound: per-handler isolation in emit plus the missing-collection guards prevent one bad event from aborting the Solid batch() and dropping the rest of a flush. No memory leaks introduced — no new subscriptions, timers, or retained references.

Fix these issues in Kilo Cloud


Reviewed by grok-4.6 · Input: 65.3K · Output: 9.9K · Cached: 475.3K

Review guidance: REVIEW.md from base branch main

@johnnyeric
johnnyeric force-pushed the johnnyeric/tui-turn-end-batch-drop branch from 5975641 to d0b8152 Compare August 20, 2026 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant