fix: stream live reasoning and tool progress in the active thread - #367
fix: stream live reasoning and tool progress in the active thread#367Jordan-SkyLF wants to merge 6 commits into
Conversation
(cherry picked from commit 401e3b643d25e8dad8c06883b478b3c3073f07a5)
(cherry picked from commit 7ee093ba19978af23b79148df2f2347e2f1e5bde)
PR Review: fix: stream live reasoning and tool progress in the active threadThanks for this fix! Here's a summary of the review: What this does
Code quality notesThe approach of making the active thread the source of truth for live progress is sound — it avoids the race condition where The test plan is thorough and the 7 passing tests cover the key edge cases:
Suggestions
Summary✅ Looks good to merge. The live streaming improvements directly address user-visible latency in the thinking/tool progress display. |
Review: close to ready, but ships with 5 failing testsHi @Jordan-SkyLF — this is a substantial and thoughtful improvement to the live streaming experience. The However, the PR as-is cannot be merged because its own test suite fails on 5 tests: These tests describe features that are not yet implemented in this PR:
What to doOption A (recommended): Remove the 5 failing tests from this PR and submit them as a separate follow-up PR that fully implements the reload-recovery and session-scoped queue features they describe. That way the improvements that are done (live reasoning, tool completion events, session-switch stability) ship now, and the more ambitious reload-recovery feature follows separately. Option B: Implement the missing features (session-scoped message queue, What's already good and would merge cleanly
If you go with Option A, this is very close to mergeable. Let me know if you'd like help splitting the PR. |
|
If you have questions or are not willing or able to update the PR to be merged, lmk! |
|
Implemented the follow-up fixes from the review in Addressed items:
Local verification I ran before updating the PR:
One GitHub-side note: the PR branch currently shows |
Follow-up Review: commit 8ad112e (reload-recovery + session-scoped queue)Thanks for implementing Option B and keeping the tests in the PR. The new commit addresses all 5 previously failing tests. Here's what was reviewed: Changes in
|
| Test | Implementation | Status |
|---|---|---|
test_newSession_resets_busy_state_for_fresh_chat |
S.busy=false, S.activeStreamId=null, updateQueueBadge(S.session.session_id) in newSession() |
✅ |
test_session_scoped_message_queue_frontend_wiring |
SESSION_QUEUES, queueSessionMessage, shiftQueuedSessionMessage in messages.js |
✅ |
test_chat_start_persists_pending_turn_metadata_for_reload_recovery |
s.active_stream_id, s.pending_user_message, s.pending_attachments set in _handle_chat_start |
✅ |
test_reload_path_restores_pending_message_and_reattaches_live_stream |
getPendingSessionMessage, attachLiveStream(sid, activeStreamId, guard in sessions.js |
✅ |
test_loadSession_inflight_restores_live_tool_cards |
appendLiveToolCard called in INFLIGHT branch |
✅ |
One question on loadInflightState
The loadSession() code calls typeof loadInflightState === 'function' as a guard before invoking it. This is good defensive style. However, loadInflightState is not defined in this PR's diff — it appears to be expected from the existing ui.js (or another file). If it's not present in the current codebase, the reload-recovery path silently does nothing. Is loadInflightState already defined in ui.js on master, or is it expected to be added? Worth verifying before merge.
Security
pending_user_messageis stored on the session file server-side and returned via/api/session— it's the user's own message, no new attack surface ✅getPendingSessionMessagesanitizes by comparing against existing messages before appending — dedup guard is correct ✅attachLiveStreamreconnect path is guarded byINFLIGHT[sid].reattach=falseimmediately before calling, preventing duplicate attachment ✅
Summary
| Area | Status |
|---|---|
| 5 previously failing tests | ✅ All addressed |
| Session-scoped message queue | ✅ Clean replacement of global MSG_QUEUE |
| Reload recovery (backend) | ✅ Pending metadata persisted on session |
| Reload recovery (frontend) | ✅ loadInflightState + attachLiveStream reconnect |
newSession() idle reset |
✅ busy/activeStreamId cleared |
Nearly ready to merge. The one outstanding question is whether loadInflightState exists in the current codebase — if yes, this is good to go.
* fix: preserve live session output across chat switches (cherry picked from commit 401e3b643d25e8dad8c06883b478b3c3073f07a5) * fix: preserve todo state after session reload (cherry picked from commit 7ee093ba19978af23b79148df2f2347e2f1e5bde) * fix: preserve live assistant anchor across rerenders * fix: stream live reasoning and tool progress * fix: recover inflight session state after reload * fix: add loadInflightState stub + CHANGELOG v0.50.21 - static/ui.js: add loadInflightState() function (currently returns null — the typeof guard in sessions.js means reload recovery works via the else-path attachLiveStream call; this stub satisfies the guard cleanly and documents the extension point for future localStorage-backed state) - CHANGELOG.md: v0.50.21 entry; 960 tests (up from 949) --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
* fix: persist durable inflight reload snapshots * fix: remove duplicate loadInflightState stub, update CHANGELOG test count The stub added in the previous review branch is superseded by the author's real localStorage-backed implementation in the cherry-picked commit 36051c0. Remove the duplicate. Update CHANGELOG to 961 tests and document the durable inflight state feature. --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
|
Closed — all commits fully merged. The initial work landed in v0.50.20 via #383. This final commit ( Great work @Jordan-SkyLF! The final implementation is solid: per-session snapshots, stream ID validation, 10-minute TTL, clean clear-on-completion. The reasoning card upgrade and session-scoped queue are also excellent additions. 🎉 |
…ena#367) * fix: preserve live session output across chat switches (cherry picked from commit 401e3b643d25e8dad8c06883b478b3c3073f07a5) * fix: preserve todo state after session reload (cherry picked from commit 7ee093ba19978af23b79148df2f2347e2f1e5bde) * fix: preserve live assistant anchor across rerenders * fix: stream live reasoning and tool progress * fix: recover inflight session state after reload * fix: add loadInflightState stub + CHANGELOG v0.50.21 - static/ui.js: add loadInflightState() function (currently returns null — the typeof guard in sessions.js means reload recovery works via the else-path attachLiveStream call; this stub satisfies the guard cleanly and documents the extension point for future localStorage-backed state) - CHANGELOG.md: v0.50.21 entry; 960 tests (up from 949) --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
* fix: persist durable inflight reload snapshots * fix: remove duplicate loadInflightState stub, update CHANGELOG test count The stub added in the previous review branch is superseded by the author's real localStorage-backed implementation in the cherry-picked commit 36051c0. Remove the duplicate. Update CHANGELOG to 961 tests and document the durable inflight state feature. --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
…ena#367) * fix: preserve live session output across chat switches (cherry picked from commit 401e3b643d25e8dad8c06883b478b3c3073f07a5) * fix: preserve todo state after session reload (cherry picked from commit 7ee093ba19978af23b79148df2f2347e2f1e5bde) * fix: preserve live assistant anchor across rerenders * fix: stream live reasoning and tool progress * fix: recover inflight session state after reload * fix: add loadInflightState stub + CHANGELOG v0.50.21 - static/ui.js: add loadInflightState() function (currently returns null — the typeof guard in sessions.js means reload recovery works via the else-path attachLiveStream call; this stub satisfies the guard cleanly and documents the extension point for future localStorage-backed state) - CHANGELOG.md: v0.50.21 entry; 960 tests (up from 949) --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
* fix: persist durable inflight reload snapshots * fix: remove duplicate loadInflightState stub, update CHANGELOG test count The stub added in the previous review branch is superseded by the author's real localStorage-backed implementation in the cherry-picked commit 36051c0. Remove the duplicate. Update CHANGELOG to 961 tests and document the durable inflight state feature. --------- Co-authored-by: Jordan SkyLF <jordan@skylinkfiber.net> Co-authored-by: Nathan Esquenazi <nesquena@gmail.com>
Summary
Stream live reasoning and tool progress into the active conversation so the operator can see in-progress work as it happens instead of waiting for the final turn snapshot.
This PR now also closes the reload/session-recovery gaps that the first review caught, so an in-flight turn can survive refreshes and session switches without dropping the pending user turn, live reasoning, or live tool cards.
What this changes
active_stream_id, pending user message, attachments) so reload recovery can reattach to the active streamWhy
Before this change, much of the useful progress UI only became visible after the final
donepayload, and some in-flight session switches or reloads could duplicate or lose live state. This makes the active thread the source of truth for live progress while preserving correct behavior when returning to an in-flight session.Test plan
python -m pytest tests/test_regressions.py -q36 passed, 1 skippedpython -m pytest tests/test_regressions.py -q -k 'loadSession_inflight_sets_busy_before_renderMessages or streaming_bridge_accepts_current_tool_progress_callback_signature or messages_js_supports_live_reasoning_and_tool_completion or ui_js_can_upgrade_thinking_spinner_into_live_reasoning_card or live_stream_tokens_persist_partial_assistant_for_session_switch or inflight_session_state_tracks_live_tool_cards_per_session or refresh_handler_does_not_drop_tool_messages_needed_by_todos or loadSession_inflight_restores_live_tool_cards or newSession_resets_busy_state_for_fresh_chat or session_scoped_message_queue_frontend_wiring or chat_start_persists_pending_turn_metadata_for_reload_recovery or reload_path_restores_pending_message_and_reattaches_live_stream'12 passed, 25 deselected