fix(chat): switch completion TPS calculation to OpenCode-style generation samples + live still - #26
Conversation
…tion samples + live still Track assistant text generation as discrete samples from first text token to completion/tool-call boundary, then compute aggregate throughput via `(tokens - 1) / duration` with proper exclusion of tool-call segments and pause intervals; fall back when no valid rate exists. Persist `tokens_per_sec` support in session message conversion and message type defaults.
PR Review: |
| Area | What changed |
|---|---|
src/ui/components/chat.rs | GenerationSample + active generation tracking; pause/resume around tool calls; still t/s API; metadata uses (n-1)/s; reuse turn counter for samples; gate layout invalidation on adaptive interval |
src/app.rs | Call sites for pause/resume/end-for-tool-calls; still rate into input; idempotent retry-status setter; test warm-up for tiktoken |
src/ui/components/input.rs | Live still NNt/s in footer during stream |
src/session/types.rs | tokens_per_sec: Option<f64> on Message |
src/persistence/conversions.rs | Load path sets tokens_per_sec: None |
Regressions / risks
Previously failing (fixed on this tree)
completed_assistant_metadata_includes_latency_metrics— expected50t/s; formula is(40-1)/0.8s→ 49t/s. Assertion updated.assistant_reasoning_can_be_collapsed— asserted non-existent"Thinking collapsed"label. Now accepts realThought for/Thinkinglabels.tool_heavy_streaming_message_defers_full_layout_refresh— markdown refresh was bumpingrender_revisioneven when layout interval still open. Production now only invalidates layout when interval elapses; test freezes the clock to avoid wall-clock flakes.streaming_text_persistence_is_throttled_*/streaming_text_chunks_do_not_dirty_*— first text chunk after stream start paid ~280ms tiktoken load, blowing the 250ms snapshot/probe windows. Generation samples now clone/reset the turn counter; tests warm the counter before measuring.
Remaining (low)
- Completed metadata t/s is systematically lower than old wall-clock math (
n-1+ tool-gap exclusion). Intentional OpenCode parity — note in release notes. tokens_per_secis not persisted — reload falls back to duration/output_tokensmath.- Single-token / zero-duration samples → no t/s (
None).
Migrations?
No DB / schema migrations required.
Runtime-only field; SQLite message columns unchanged. No backfill.
Pre-merge checklist
- Fix
completed_assistant_metadata_includes_latency_metricsfor(n-1)formula - Fix collapsed-thinking label assertion
- Fix tool-heavy deferred layout (production + test)
- Fix streaming text dirty/persist tests (tiktoken warm path)
cargo test --bin crabcode chat::tests— 119 passedcargo test --bin crabcode app::tests— 115 passedcargo fmt- Manual smoke (still recommended):
- Plain stream → still t/s + final metadata
- Tool multi-round → pause during tools; final t/s excludes gap
- Abort / error mid-stream
- Reload session → fallback metadata OK
- Single-token reply
- Open PR if not opened; document OpenCode-style formula so lower t/s isn’t filed as a bug
- Commit the uncommitted checklist fixes (
src/app.rs,src/ui/components/chat.rs) before merge
Checks run (this pass)
| Check | Result |
|---|---|
| Latency metrics unit test | PASS (49t/s) |
| Thinking collapse unit test | PASS |
| Tool-heavy defer layout unit test | PASS |
| Streaming dirty / persist unit tests | PASS |
Full chat::tests | 119 passed |
Full app::tests | 115 passed |
cargo fmt | applied |
| Schema / migrations scan | no new message-column migration |
Verdict
Ready to merge after commit + optional manual smoke. Design direction is sound; checklist items that could be fixed without mutating user data are done. Confidence 4.5/5 (half-point reserved for manual stream/tool smoke only).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b1de123f06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(counter) = self.generation_token_counter.as_mut() { | ||
| let tokens = counter.add_text(chunk); | ||
| if let Some(sample) = self.active_generation.as_mut() { | ||
| sample.tokens = tokens; | ||
| } |
There was a problem hiding this comment.
When a provider retry emits StreamRollback after partial text, rollback_streamed_output resets only the turn-level streaming_token_counter; this newly added generation_token_counter and the active sample's start time remain unchanged. Retried text is therefore counted on top of discarded text, while retry/backoff time remains in the sample duration, corrupting both live and finalized TPS. Reset or discard the active generation sample when a rollback succeeds.
Useful? React with 👍 / 👎.
| output_tokens: msg | ||
| .output_tokens | ||
| .and_then(|v| if v > 0 { Some(v as usize) } else { None }), | ||
| tokens_per_sec: None, |
There was a problem hiding this comment.
Every saved and reloaded assistant message receives None here because the persistence message/schema does not carry tokens_per_sec. For multi-step turns, the fallback cannot reconstruct the same value: output_tokens includes reasoning and tool-call-ending steps, while the aggregate TPS excludes those contributions. Consequently reopening a session changes the displayed throughput, so this field needs to be round-tripped through persistence.
Useful? React with 👍 / 👎.
| msg.output_tokens = Some(token_count); | ||
| msg.token_count = Some(token_count); | ||
| msg.duration_ms = Some(decode_duration_ms); | ||
| msg.tokens_per_sec = final_tps; |
There was a problem hiding this comment.
In the remote-host context, this computed value stops at the session message: RemoteMessage in src/remote/mod.rs has no corresponding field, and assistantMetrics in remote-client/src/pages/index/thread-model.ts still calculates tokens / (tn - t1). Browser clients therefore continue to include tool execution and reasoning in the old formula instead of showing the new sample aggregate; serialize this value and prefer it in the remote client.
Useful? React with 👍 / 👎.
- reuse the previous turn’s streaming token counter when available to avoid repeated tiktoken warm-up after tool calls - gate markdown stream cache invalidation behind layout interval cadence and keep pending dirty state for later flush - update streaming and token-rate tests for revised collapsed-reasoning label and layout-timing behavior
Deploying crabcode with |
| Latest commit: | 0813fa6 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b27c08f4.crabcode.pages.dev |
| Branch Preview URL: | https://feat-better-tps-tally.crabcode.pages.dev |
Uh oh!
There was an error while loading. Please reload this page.
Track assistant text generation as discrete samples from first text token to completion/tool-call boundary, then compute aggregate throughput via
(tokens - 1) / durationwith proper exclusion of tool-call segments and pause intervals; fall back when no valid rate exists. Persisttokens_per_secsupport in session message conversion and message type defaults.