Open
Conversation
Allow the stats table to switch between daily and monthly periods with a hotkey so broader usage trends are easier to inspect without leaving the dashboard.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tui.rs (1)
110-120: Consider caching monthly aggregates per refresh.
get_aggregate_stats()rebuilds the rolled-upBTreeMapevery call. In monthly mode this helper is hit by navigation, date jump, and draw paths, so one keypress can re-aggregate the samedaily_statsseveral times.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui.rs` around lines 110 - 120, get_aggregate_stats currently recomputes the monthly rollup every call; to fix, add a cache that stores the result of aggregate_daily_stats_by_month(&view.daily_stats) and only rebuild it when daily_stats changes (e.g., on a refresh/update). Concretely: add a cached_monthly field (Option<...>) to the owner of AnalyzerStatsView or AnalyzerStatsView itself, set/invalidate that cache whenever daily_stats is updated/refreshed, update get_aggregate_stats to return AggregateStatsData::Owned from the cached_monthly when AggregateViewMode::Monthly (computing and storing it on first use if absent), and ensure cache invalidation happens on the same code paths that mutate or reload view.daily_stats so navigation/draw calls reuse the cached monthly BTreeMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui.rs`:
- Around line 1205-1207: The clamp and metadata must be computed from the
actually rendered subset rather than the full map: change the code so
get_aggregate_stats/aggregate_stats.as_map() is invoked on the same
filtered/reordered collection used to render the table (the rendered_rows /
rendered_aggregate_map), then call clamp_table_selection(table_state,
rendered_aggregate_map.len() + 2); also update the routines that derive best-row
indices, totals labels and model metadata (the code paths that currently read
from the unfiltered order) to read from that same rendered_aggregate_map so
reversed/filtered views and search jumps show correct max markers, selected row
and Total (...m) label.
In `@src/tui/logic.rs`:
- Around line 84-96: The single-number branch currently treats any parsed number
>31 as year and 1..=12 as month but lets 13..=31 fall-through to the substring
match, causing "/20" or "/25" to match year substrings; update the logic in the
parts[0] parse block so that: if number > 31 return day_year == number; else if
(13..=31).contains(&number) return a comparison against the day-of-month field
(use the existing day-of-month variable or parse it from day); else if
(1..=12).contains(&number) return day_month == number; otherwise fall back to
return day.contains(trimmed); ensure parts[0], number, day_year, day_month and
the day substring match are the referenced symbols to locate where to change.
---
Nitpick comments:
In `@src/tui.rs`:
- Around line 110-120: get_aggregate_stats currently recomputes the monthly
rollup every call; to fix, add a cache that stores the result of
aggregate_daily_stats_by_month(&view.daily_stats) and only rebuild it when
daily_stats changes (e.g., on a refresh/update). Concretely: add a
cached_monthly field (Option<...>) to the owner of AnalyzerStatsView or
AnalyzerStatsView itself, set/invalidate that cache whenever daily_stats is
updated/refreshed, update get_aggregate_stats to return
AggregateStatsData::Owned from the cached_monthly when
AggregateViewMode::Monthly (computing and storing it on first use if absent),
and ensure cache invalidation happens on the same code paths that mutate or
reload view.daily_stats so navigation/draw calls reuse the cached monthly
BTreeMap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61154f04-2114-48a6-87a9-bc659339419e
📒 Files selected for processing (3)
src/tui.rssrc/tui/logic.rssrc/tui/tests.rs
Comment on lines
+84
to
96
| // Single number - prefer month matching, but allow year-only lookups too. | ||
| if let Ok(number) = parts[0].parse::<u32>() { | ||
| if number > 31 { | ||
| return day_year == number; | ||
| } | ||
|
|
||
| if (1..=12).contains(&number) { | ||
| return day_month == number; | ||
| } | ||
| } | ||
|
|
||
| // Otherwise match if the date contains this string | ||
| return day.contains(trimmed); |
There was a problem hiding this comment.
Handle single-number day jumps explicitly.
Line 84 treats a lone number as month-or-year, but values 13..=31 fall through to the substring match on Line 96. On 20xx data, /20 or /25 will match the year portion of many rows instead of the day-of-month, so jump-to-date lands on the wrong entry.
💡 Proposed fix
if parts.len() == 1 {
// Single number - prefer month matching, but allow year-only lookups too.
if let Ok(number) = parts[0].parse::<u32>() {
if number > 31 {
return day_year == number;
}
if (1..=12).contains(&number) {
return day_month == number;
}
+
+ return day_number
+ .map(|actual_day| actual_day == number)
+ .unwrap_or(false);
}
// Otherwise match if the date contains this string
return day.contains(trimmed);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui/logic.rs` around lines 84 - 96, The single-number branch currently
treats any parsed number >31 as year and 1..=12 as month but lets 13..=31
fall-through to the substring match, causing "/20" or "/25" to match year
substrings; update the logic in the parts[0] parse block so that: if number > 31
return day_year == number; else if (13..=31).contains(&number) return a
comparison against the day-of-month field (use the existing day-of-month
variable or parse it from day); else if (1..=12).contains(&number) return
day_month == number; otherwise fall back to return day.contains(trimmed); ensure
parts[0], number, day_year, day_month and the day substring match are the
referenced symbols to locate where to change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Press
min the TUI to toggle between daily and monthly stats. The monthly view rolls up all your daily usage into per-month totals, making it easier to see longer-term trends.Notes
This works, but it's not perfect.
The changes could be a bit cleaner; but that'd require a larger refactor.
I wanted a quick view personally; and felt like throwing away the code would be a waste, so here it is.
Use it, or do anything else with it :p
Refactor needed: Notably
src/tui.rshas grown too large (~500 lines of changes in this PR alone). It should be split into smaller modules (e.g., separate files for rendering, state management, input handling).Summary by CodeRabbit
New Features
Tests