Conversation
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The Provider-backed E2E check is red for a reason unrelated to this change: the Ollama account no longer subscribes, and the workflow's fallback model Tracked in #240, which has the detail and the options. The same failure is on #236 and will appear on #239. Every other check on this PR passes: Coverage, both platform builds, the clippy and structural lint gate, formatting, tests, and CodeRabbit. |
A stream error wrote last_error into the conversation's streaming state and nothing ever cleared it. The chat view projection checks last_error before it checks whether a stream is active, so once a turn died the conversation stayed on StreamingState::Error: the streaming bubble and the thinking rendered inside it were suppressed for every later turn, and the answer appeared only when some later successful finalize removed the whole state entry. Fix the state transition at its source. show_thinking_for_target is the stream-start seam (the presenter sends ShowThinking both when a stream starts and before sending a message), so it now clears last_error for the target. An error recorded by a finished turn must not describe the turn now running. changed also accounts for a cleared error so the projection refreshes and the target rejoins the active set. The check order in streaming_state_from_snapshot stays error-first. After the reset, every legitimately active stream has already cleared last_error, so the order cannot misfire on real turn sequences. The only remaining overlap is a stale delta arriving after its own turn's error with no new stream start, and for that case Error is the accurate projection; swapping the checks would instead resurrect dead partial content and hide the failure. The new regression test drives error, stream start, and a delta through the store and asserts the projection returns to Streaming with the new turn's thinking buffer intact.
1 participant
Fixes #218
After any stream error in a conversation, every later turn rendered nothing while streaming: no thinking, no token-by-token text, with the answer appearing all at once at the end.
Cause
last_errorwas written byreduce_stream_errorand cleared nowhere. A grep forlast_error = None,last_error: Noneandlast_error.take()acrosssrc/ui_gpui/returned nothing, which is the whole bug in one line.streaming_state_from_snapshotcheckslast_errorbefore it checks whether a stream is active, so once a turn died the conversation stayed onStreamingState::Error. The render gate only draws the streaming bubble forStreamingState::Streaming, and thinking renders inside that bubble, so both were suppressed. The store kept doing its job the entire time, fillingthinking_bufferand copying it intothinking_content; nothing ever drew it.It looked intermittent rather than permanent because a later successful finalize removes the whole streaming-state entry, taking
last_errorwith it.Fix
Clear
last_errorat the stream-start seam rather than special-casing the projection that reads it.show_thinking_for_targetis that seam (the presenter sendsShowThinkingwhen a stream starts and before sending a message), so it now clears the field: an error recorded by a finished turn must not describe the turn now running.changedalso accounts for a cleared error, so the projection refreshes and the target rejoins the active set. Without that, a turn starting with thinking already visible and the same model would clear the error without marking the state changed, and the view would not re-render.On the check order, which the issue also raised
Left as error-first, deliberately.
After this reset, every legitimately active stream has already cleared
last_error, so the order cannot misfire on a real turn sequence. The only remaining case where both could be set is a stale delta arriving after its own turn's error with no new stream start, and thereErroris the accurate projection. Swapping the checks would resurrect dead partial content and hide the failure, so the reset is the fix and the reorder is not needed.Tests
a_new_turn_after_a_stream_error_projects_streaming_againdrives error, then stream start, then a delta through the store, and asserts the projection returns toStreamingwith the new turn's thinking buffer intact.It was written first and failed against unmodified code with the reported symptom, a provider 429:
The test lives in a new
streaming_error_tests.rssibling module. Adding it tomod_tests.rspushed that file to 1067 lines, over the repo's 1000-line gate, so it moved rather than anything being deleted to fit;mod_tests.rsis unchanged in this diff.Verification
cargo fmt --all -- --check; clippy with the full CI lint set;cargo test --lib --tests(1976 passed, 0 failed, which is main's 1975 plus this regression test);cargo xtask guard;lizard -C 50 -L 100 -w src/; the 1000-line file gate. No lint suppressions, no new dependencies, no existing test weakened.Independent of the steering work in #236 and #237: this touches only
app_store_streaming.rsand the chat view test modules.