Fix capped large token totals in TUI stats#130
Conversation
📝 WalkthroughWalkthroughThis PR expands numeric fields in the TuiStats struct from u32 to u64 to prevent value capping at u32::MAX (4,294,967,295). Supporting type conversions and arithmetic operations across contribution caching, MCP server, TUI rendering, and utility modules are updated to eliminate now-redundant casts and work with the expanded types. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tui/tests.rs (1)
328-355: Please cover the widenedcost_centspath in this regression too.The test proves the token counters can cross
u32::MAX, but this PR widens cost totals for the same reason. One extra assertion above the old cents ceiling would keepadd_costfrom regressing independently.Possible extension
#[test] fn test_tui_stats_accumulation_exceeds_u32_max() { - let mut dst = TuiStats::default(); + let mut dst = TuiStats { + cost_cents: u32::MAX as u64, + ..TuiStats::default() + }; let src = Stats { input_tokens: u32::MAX as u64, output_tokens: 1, reasoning_tokens: 2, cached_tokens: 3, + cost: 0.01, ..Stats::default() }; accumulate_tui_stats(&mut dst, &src); accumulate_tui_stats( @@ assert_eq!(dst.input_tokens, u32::MAX as u64 + 1); assert_eq!(dst.output_tokens, u32::MAX as u64 + 1); assert_eq!(dst.reasoning_tokens, u32::MAX as u64 + 2); assert_eq!(dst.cached_tokens, u32::MAX as u64 + 3); + assert_eq!(dst.cost_cents, u32::MAX as u64 + 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/tests.rs` around lines 328 - 355, Extend the test_tui_stats_accumulation_exceeds_u32_max test to also exercise the widened cost totals by populating Stats.cost_cents with values at and past u32::MAX and then asserting dst.cost_cents accumulates beyond the old u32 ceiling; specifically, call accumulate_tui_stats (or the underlying add_cost path) with a Stats instance whose cost_cents is u32::MAX as u64 and another with 1 (or vice versa) and add assertions that dst.cost_cents == u32::MAX as u64 + 1 to ensure add_cost and TuiStats.cost_cents no longer overflow at the previous u32 limit.src/tui/logic.rs (1)
10-16: Consider moving this accumulation helper out of the TUI module.
src/utils.rs:221-242now carries the same 6-field update logic. Extracting one shared helper in a neutral module would make it harder for the daily and session paths to drift again the next time a counter changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/logic.rs` around lines 10 - 16, The TUI-specific function accumulate_tui_stats duplicates logic present in src/utils.rs; extract the six-field accumulation into a shared helper (e.g., pub fn accumulate_stats(dst: &mut TuiStats, src: &Stats) or a more generic signature using the common Stats-like fields) placed in a neutral module (utils or stats) and make it public so both the TUI code and the code at utils.rs:221-242 call it; update the accumulate_tui_stats call sites to use the new shared helper (or replace accumulate_tui_stats with a thin wrapper that delegates to the shared function) and keep the dst.add_cost(src.cost) logic and saturating_add usages intact, ensuring the function visibility and imports are adjusted for the new module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tui/logic.rs`:
- Around line 10-16: The TUI-specific function accumulate_tui_stats duplicates
logic present in src/utils.rs; extract the six-field accumulation into a shared
helper (e.g., pub fn accumulate_stats(dst: &mut TuiStats, src: &Stats) or a more
generic signature using the common Stats-like fields) placed in a neutral module
(utils or stats) and make it public so both the TUI code and the code at
utils.rs:221-242 call it; update the accumulate_tui_stats call sites to use the
new shared helper (or replace accumulate_tui_stats with a thin wrapper that
delegates to the shared function) and keep the dst.add_cost(src.cost) logic and
saturating_add usages intact, ensuring the function visibility and imports are
adjusted for the new module.
In `@src/tui/tests.rs`:
- Around line 328-355: Extend the test_tui_stats_accumulation_exceeds_u32_max
test to also exercise the widened cost totals by populating Stats.cost_cents
with values at and past u32::MAX and then asserting dst.cost_cents accumulates
beyond the old u32 ceiling; specifically, call accumulate_tui_stats (or the
underlying add_cost path) with a Stats instance whose cost_cents is u32::MAX as
u64 and another with 1 (or vice versa) and add assertions that dst.cost_cents ==
u32::MAX as u64 + 1 to ensure add_cost and TuiStats.cost_cents no longer
overflow at the previous u32 limit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee8764d7-bd27-4f37-9d90-84b3d42e69ed
📒 Files selected for processing (8)
src/contribution_cache/single_message.rssrc/mcp/server.rssrc/mcp/types.rssrc/tui.rssrc/tui/logic.rssrc/tui/tests.rssrc/types.rssrc/utils.rs
Summary
Closes #129.
Verification
cargo build --quietcargo test --quietcargo clippy --quiet -- -D warningscargo doc --quietcargo fmt --all --quietSummary by CodeRabbit
Bug Fixes
Tests