feat: add all-tools period aggregation views#158
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR introduces weekly and yearly aggregation support to the TUI by generalizing day-based filtering into a ChangesWeekly/Yearly Period Aggregation
Sequence DiagramsequenceDiagram
participant User as User Input
participant NavLogic as Navigation Logic
participant DisplayStats as Display Stats
participant PeriodFilter as Period Filters
participant Sessions as Session Table
participant Render as Render Engine
User ->> NavLogic: Enter (drill aggregate→session)
NavLogic ->> DisplayStats: Get selected aggregate period
DisplayStats ->> PeriodFilter: Compute PeriodFilter (week/month/year)
PeriodFilter ->> Sessions: Filter sessions by period
Sessions ->> Render: Render filtered session table
Render ->> User: Display period-filtered sessions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/tui.rs (2)
2426-2435: 💤 Low valueSimplify
update_period_filters— thetruncateis redundant afterresize.
Vec::resize(new_len, value)already truncates whennew_len < current_len, so the explicittruncate(and theold_lencapture) are dead code that misleads readers about whatresizedoes.♻️ Proposed simplification
fn update_period_filters(filters: &mut Vec<Option<PeriodFilter>>, filtered_count: &usize) { - let old_len = filters.len(); filters.resize(*filtered_count, None); - if *filtered_count < old_len { - filters.truncate(*filtered_count); - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui.rs` around lines 2426 - 2435, The function update_period_filters contains redundant code: it captures old_len and calls truncate after calling filters.resize(*filtered_count, None), but Vec::resize already truncates when new_len < current_len; remove the dead old_len binding and the explicit filters.truncate(*filtered_count) call, leaving only filters.resize(*filtered_count, None) in update_period_filters to simplify and clarify intent.
348-380: 💤 Low valueOptional: surface the source analyzer in the "All Tools" session view.
combined_sessionsmixesSessionAggregates from every analyzer, but the session table (draw_session_stats_table) doesn't renderanalyzer_name. On the "All Tools" tab, two sessions from different tools with similar names/IDs are indistinguishable to the user. Consider adding an analyzer column (or prefixing the session label) when the tab is "All Tools".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui.rs` around lines 348 - 380, The combined "All Tools" session list loses source analyzer context because combined_sessions aggregates SessionAggregate items without recording which analyzer they came from; update the loop that builds combined_sessions to attach the source analyzer to each SessionAggregate (e.g., set a new or existing field like analyzer_name or prefix the session label) when pushing into combined_sessions in the block that iterates filtered_stats and calls view.session_aggregates.iter().cloned(); then update draw_session_stats_table to render the analyzer column (or parse the prefixed label) so the "All Tools" AnalyzerStatsView shows which analyzer produced each session.src/tui/tests.rs (1)
105-124: 💤 Low valueTest name is stale — it now exercises period filters, not day filters.
The body uses
PeriodFilterandupdate_period_filters, but the function name still says "day_filters", which makes it harder to find/grep when revisiting this area.♻️ Proposed rename
-fn test_update_window_offsets_and_day_filters_resize() { +fn test_update_window_offsets_and_period_filters_resize() {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/tests.rs` around lines 105 - 124, The test function name is misleading — it references "day_filters" but the body exercises PeriodFilter and update_period_filters; rename the test fn to reflect that (e.g., change test_update_window_offsets_and_day_filters_resize to test_update_window_offsets_and_period_filters_resize) so it matches the tested symbols (PeriodFilter, update_period_filters, update_window_offsets) and improves discoverability.src/tui/logic.rs (1)
256-284: ⚡ Quick winDoc comment for
aggregate_daily_stats_by_periodis stale.The doc still describes the previous monthly-only behavior ("Roll up daily statistics into monthly totals", "Groups daily stats by year-month..."). Since this is now the generic helper, the description is misleading. Move the monthly-specific docs to
aggregate_daily_stats_by_monthand add a generic description here.📝 Proposed doc update
-/// Roll up daily statistics into monthly totals. -/// -/// Groups daily stats by year-month (YYYY-MM) and sums all metrics. Each month -/// entry uses day 1 of that month as its representative date. Returns a new -/// map with monthly aggregated data. +/// Roll up daily statistics into periods derived by `period_key_fn`. +/// +/// `period_key_fn` returns `(period_key, representative_date)` for each `DailyStats`, +/// and all entries sharing the same key are summed via `DailyStats += &DailyStats`. fn aggregate_daily_stats_by_period<F>( daily_stats: &BTreeMap<String, DailyStats>, mut period_key_fn: F, ) -> BTreeMap<String, DailyStats> where F: FnMut(&DailyStats) -> (String, CompactDate), { ... } + +/// Roll up daily statistics into monthly totals. +/// +/// Groups daily stats by year-month (`YYYY-MM`) and sums all metrics. Each month +/// entry uses day 1 of that month as its representative date. pub fn aggregate_daily_stats_by_month(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui/logic.rs` around lines 256 - 284, The doc comment for aggregate_daily_stats_by_period incorrectly describes monthly-only behavior; replace it with a generic description that explains this function groups DailyStats using the provided period_key_fn (which returns a (period_key, representative_date)) and returns a new BTreeMap of aggregated DailyStats by period, and move the existing month-specific wording ("Roll up daily statistics into monthly totals", "Groups daily stats by year-month...") into the doc comment for aggregate_daily_stats_by_month so that aggregate_daily_stats_by_period's docs accurately reflect its generic helper role.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tui.rs`:
- Around line 2426-2435: The function update_period_filters contains redundant
code: it captures old_len and calls truncate after calling
filters.resize(*filtered_count, None), but Vec::resize already truncates when
new_len < current_len; remove the dead old_len binding and the explicit
filters.truncate(*filtered_count) call, leaving only
filters.resize(*filtered_count, None) in update_period_filters to simplify and
clarify intent.
- Around line 348-380: The combined "All Tools" session list loses source
analyzer context because combined_sessions aggregates SessionAggregate items
without recording which analyzer they came from; update the loop that builds
combined_sessions to attach the source analyzer to each SessionAggregate (e.g.,
set a new or existing field like analyzer_name or prefix the session label) when
pushing into combined_sessions in the block that iterates filtered_stats and
calls view.session_aggregates.iter().cloned(); then update
draw_session_stats_table to render the analyzer column (or parse the prefixed
label) so the "All Tools" AnalyzerStatsView shows which analyzer produced each
session.
In `@src/tui/logic.rs`:
- Around line 256-284: The doc comment for aggregate_daily_stats_by_period
incorrectly describes monthly-only behavior; replace it with a generic
description that explains this function groups DailyStats using the provided
period_key_fn (which returns a (period_key, representative_date)) and returns a
new BTreeMap of aggregated DailyStats by period, and move the existing
month-specific wording ("Roll up daily statistics into monthly totals", "Groups
daily stats by year-month...") into the doc comment for
aggregate_daily_stats_by_month so that aggregate_daily_stats_by_period's docs
accurately reflect its generic helper role.
In `@src/tui/tests.rs`:
- Around line 105-124: The test function name is misleading — it references
"day_filters" but the body exercises PeriodFilter and update_period_filters;
rename the test fn to reflect that (e.g., change
test_update_window_offsets_and_day_filters_resize to
test_update_window_offsets_and_period_filters_resize) so it matches the tested
symbols (PeriodFilter, update_period_filters, update_window_offsets) and
improves discoverability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b61e13a-223f-450f-bb3f-547d85733292
📒 Files selected for processing (3)
src/tui.rssrc/tui/logic.rssrc/tui/tests.rs
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Summary
This PR adds broader period-based aggregation support to the TUI and introduces a combined All Tools view so reviewers can inspect total usage across all tracked tools in one place.
Key Changes
Testing
Summary by CodeRabbit
New Features
Improvements