Add period navigation arrows to Reports view#1707
Add period navigation arrows to Reports view#1707joaocbatista wants to merge 0 commit intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
contributor:verified
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617a46471e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <%= link_to reports_path(period_type: @period_type, start_date: next_start, end_date: next_end), | ||
| aria: { label: "Next period" }, | ||
| class: "#{ at_latest ? 'text-disabled pointer-events-none' : 'text-secondary hover:text-primary transition-colors' }" do %> |
There was a problem hiding this comment.
When the report is already at the latest period, this still renders an anchor with an href to the next future period and only adds pointer-events-none. That blocks mouse clicks, but the link remains focusable and can still be activated from the keyboard or by assistive tech, so users on the current month/quarter/YTD can navigate into a future period despite the arrow appearing disabled. Render a non-link/omit the href or otherwise remove it from the tab order when at_latest is true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This sounds worth addressing, @joaocbatista ... also tests?
There was a problem hiding this comment.
sorry, still understanding the structure of the code and tests.
I've now added and fixed some tests based on coderabbitai feedback, made sure tests are passing as expected.
|
Can you include screenshots in light and dark mode, please? 🙏 |
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe reports index now computes and exposes period navigation ( ChangesPeriod Navigation UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/views/reports/index.html.erb (1)
93-157: 🏗️ Heavy liftExtract period-navigation date math out of the ERB.
The 30+ line
case@period_type`` block computes prev/next ranges andat_latestinline in the template. This is exactly the kind of "heavy logic" the project guidelines push out of views, and it would also let you unit-test the boundary cases (last_6_months transition, quarter end, YTD year rollover) without going through the view.A few options, in increasing scope:
- A
ReportsHelper#period_nav(period_type, start_date, end_date)returning a small struct/hash withprev_start,prev_end,next_start,next_end,at_latest.- A PORO like
Reports::PeriodNavigator.new(period_type, start_date, end_date)exposing#prev,#next,#at_latest?.- A
DS::PeriodNavigatorViewComponent that owns the markup as well, since the chevron+label+chevron pattern is reusable, has conditional disabled state, and a clear API — a good fit per the "Use ViewComponents when … reused / has variants / accessibility" guideline.As per coding guidelines: "Avoid heavy logic in ERB views; prefer helpers and components" and "Keep domain logic out of views".
🤖 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 `@app/views/reports/index.html.erb` around lines 93 - 157, The view contains heavy date-calculation logic (the case on `@period_type` computing prev_start/prev_end/next_start/next_end/at_latest) that should be extracted; implement a helper or PORO (suggested: ReportsHelper#period_nav(period_type, start_date, end_date) returning a hash/struct with keys prev_start, prev_end, next_start, next_end, at_latest OR a class Reports::PeriodNavigator.new(period_type, start_date, end_date) exposing methods prev, next, at_latest?) that encapsulates the month/quarter/ytd/last_6_months logic, update the ERB to call that API (passing `@period_type`, `@start_date`, `@end_date`) and use the returned values for link_to and the disabled state so the view only renders markup and the navigator (prev/next/at_latest) is fully unit-testable.
🤖 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.
Inline comments:
In `@app/views/reports/index.html.erb`:
- Line 131: Replace the hard-coded aria label strings used for the pagination
controls ("Previous period" and "Next period") with translations using the t()
helper (e.g., change aria: { label: "Previous period" } and aria: { label: "Next
period" } to use t("reports.index.previous_period") and
t("reports.index.next_period") respectively) and add corresponding
previous_period and next_period keys under reports.index in
config/locales/en.yml.
- Around line 117-126: The :last_6_months branch computes
prev_start/prev_end/next_start/next_end incorrectly when next_end is clamped to
the current month: if you clamp next_end to Date.current.end_of_month you must
re-anchor next_start to be exactly 6 months before that clamped next_end (so the
next window remains 6 months) and keep at_latest computed from the clamped end;
move the entire date-range calculation (the block that defines prev_start,
prev_end, next_start, next_end, at_latest for all cases) into a helper (e.g.,
ReportsHelper#period_bounds or similar) and call it from the view, and replace
hard-coded aria-label strings "Previous period"/"Next period" with translation
lookups using t(...) (e.g., t('reports.previous_period'),
t('reports.next_period')) so labels are localized.
---
Nitpick comments:
In `@app/views/reports/index.html.erb`:
- Around line 93-157: The view contains heavy date-calculation logic (the case
on `@period_type` computing prev_start/prev_end/next_start/next_end/at_latest)
that should be extracted; implement a helper or PORO (suggested:
ReportsHelper#period_nav(period_type, start_date, end_date) returning a
hash/struct with keys prev_start, prev_end, next_start, next_end, at_latest OR a
class Reports::PeriodNavigator.new(period_type, start_date, end_date) exposing
methods prev, next, at_latest?) that encapsulates the
month/quarter/ytd/last_6_months logic, update the ERB to call that API (passing
`@period_type`, `@start_date`, `@end_date`) and use the returned values for link_to
and the disabled state so the view only renders markup and the navigator
(prev/next/at_latest) is fully unit-testable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01780560-94c4-43dc-9fa6-682f2e764c35
📒 Files selected for processing (1)
app/views/reports/index.html.erb
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@test/controllers/reports_controller_test.rb`:
- Around line 110-112: The current assertions use start_date.strftime("%b") and
end_date.strftime("%b") which are too permissive; update the assertions in
reports_controller_test.rb (the lines using assert_includes `@response.body` with
start_date and end_date) to check for a more specific representation of the
swapped dates—e.g., use full formatted dates including year (start_date.strftime
with "%b %d, %Y" or ISO "%Y-%m-%d") or assert the exact date-range string
rendered by the view, or target the specific HTML element/text node that
contains the range (assert_includes `@response.body`, exact_range_string or
assert_match /#{Regexp.escape(formatted_date)}/) so the test verifies the swap
precisely rather than any three-letter month substring.
- Around line 272-284: In the test method "last 6 months next window extends to
current month end when crossing boundary" remove the debug puts line and replace
the placeholder assert with a concrete assertion: compute the expected_end as
Date.current.end_of_month and assert that at least one href in the captured
hrefs (from `@response.body.scan`(...)) includes an end_date query parameter equal
to that expected_end (or otherwise verify the generated next-window link extends
to the current month end); keep the existing get call and hrefs extraction but
assert presence of the expected end_date in those hrefs instead of using assert
true.
- Around line 286-293: The test "quarterly period navigation shows previous and
next quarter links" only asserts the previous-quarter link; add an assertion for
the next-quarter link by computing next_start = (Date.current.end_of_quarter +
1.day).beginning_of_quarter and next_end = next_start.end_of_quarter and then
call assert_select "a[href=?]", reports_path(period_type: :quarterly,
start_date: next_start, end_date: next_end) alongside the existing previous-link
assertion so both navigation directions are verified in the test.
- Around line 254-259: The test "monthly period navigation disables next arrow
on current month" uses a too-broad selector; tighten it to target the actual
next-period control (e.g. the next-arrow button/link) instead of any span.
Update the assertion in that test to scope into the period navigation component
(using its container selector or id) and assert on the specific control's
attributes (for example the element with aria-label like "Next period" or the
next-arrow button/link) having aria-disabled="true" so the test only passes when
the next-arrow control is disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9daa6f4-332e-4fb5-bcaf-732f35b584d4
📒 Files selected for processing (3)
app/views/reports/index.html.erbconfig/locales/views/reports/en.ymltest/controllers/reports_controller_test.rb
✅ Files skipped from review due to trivial changes (1)
- config/locales/views/reports/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/reports/index.html.erb
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@app/controllers/reports_controller.rb`:
- Around line 1099-1109: The forward-navigation snap for the :last_6_months
branch uses the wrong threshold: replace the current condition that compares
candidate_start + 6.months against Date.current.beginning_of_month with a check
against the current month's end using the candidate window's end; compute
candidate_end = candidate_start + 6.months - 1.day (same formula used for
prev_end/next_end) and trigger the snap only when candidate_end >
Date.current.end_of_month so the transition does not skip the immediate next
6-month block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 707c9983-f90a-4f89-a728-0c98d059a98b
📒 Files selected for processing (2)
app/controllers/reports_controller.rbapp/views/reports/index.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/reports/index.html.erb
| when :last_6_months | ||
| prev_start = @start_date.beginning_of_month - 6.months | ||
| prev_end = prev_start + 6.months - 1.day | ||
| candidate_start = @start_date.beginning_of_month + 6.months | ||
| if candidate_start + 6.months >= Date.current.beginning_of_month | ||
| next_end = Date.current.end_of_month | ||
| next_start = (next_end + 1.day - 6.months).beginning_of_month | ||
| else | ||
| next_start = candidate_start | ||
| next_end = next_start + 6.months - 1.day | ||
| end |
There was a problem hiding this comment.
last_6_months forward navigation skips one period due to an overly broad snap condition.
The condition on line 1103 is candidate_start + 6.months >= Date.current.beginning_of_month, which fires one full step too early. Concrete example (today = May 8 2026):
- Default view: Nov 2025 – May 2026 (
at_latest = true, next disabled ✓). - Click prev → May 2025 – Oct 2025.
- Click next from May–Oct 2025:
candidate_start= Nov 1 2025Nov 1 + 6m = May 1 2026 >= May 1 2026→ snap fires → result: Dec 2025 – May 2026- Nov 2025 is skipped entirely.
The snap should only activate when the candidate window's end date would exceed the current month's end. Correcting the condition:
- candidate_start = `@start_date.beginning_of_month` + 6.months
- if candidate_start + 6.months >= Date.current.beginning_of_month
+ candidate_start = `@start_date.beginning_of_month` + 6.months
+ candidate_end = candidate_start + 6.months - 1.day
+ if candidate_end > Date.current.end_of_month
next_end = Date.current.end_of_month
next_start = (next_end + 1.day - 6.months).beginning_of_month
else
next_start = candidate_start
- next_end = next_start + 6.months - 1.day
+ next_end = candidate_end
endWith the fix:
- From May–Oct 2025:
candidate_end= Apr 30 2026 > May 31 2026 → false → Nov 2025 – Apr 2026 ✓ - From Nov 2025–Apr 2026:
candidate_end= Oct 31 2026 > May 31 2026 → true → snap to Dec 2025 – May 2026 ✓
🤖 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 `@app/controllers/reports_controller.rb` around lines 1099 - 1109, The
forward-navigation snap for the :last_6_months branch uses the wrong threshold:
replace the current condition that compares candidate_start + 6.months against
Date.current.beginning_of_month with a check against the current month's end
using the candidate window's end; compute candidate_end = candidate_start +
6.months - 1.day (same formula used for prev_end/next_end) and trigger the snap
only when candidate_end > Date.current.end_of_month so the transition does not
skip the immediate next 6-month block.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/controllers/reports_controller_test.rb`:
- Around line 317-318: The test for custom-period display currently asserts that
the previous period control is hidden but misses checking the next period
control. Add an assertion similar to the existing one but for "a[aria-label=?]"
with I18n.t("reports.index.next_period") and count: 0 to the same test method to
verify the next period control is also hidden.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a86964d2-6e02-4481-9be6-775421a18267
📒 Files selected for processing (2)
app/views/reports/index.html.erbtest/controllers/reports_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/reports/index.html.erb
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Could we try for consitency with budget period navigation? Is there design system components for there?
|
Good feature overall — clean i18n usage and sensible period math. A few things worth addressing before merge: 1. Nil-crash risk in the view
Safer guard: <% if @nav %>
<div class="flex items-center gap-3 text-sm text-subdued">
...
</div>
<% end %>This is also more expressive: "show navigation when it's available," rather than "show navigation unless custom." 2. YTD case has no test The test suite covers monthly, quarterly, last-6-months, and custom, but not 3. Inconsistent indentation in the quarterly test test "quarterly period navigation shows previous and next quarter links" do
get reports_path(period_type: :quarterly) # 6-space indent instead of 4The body uses 6 spaces instead of the 4-space style used everywhere else in the file. Generated by Claude Code Generated by Claude Code |
|
adjusted the style of the arrows to match the one found in the budget screen. missed that we already had this pattern in the app. it's looking better now. 
|
|
Thoughts, @gariasf? This should probably get |
|
Hey @joaocbatista, thanks for taking this on! Coming at this fresh — turns out the budget header already does this exact job, so most of what's below is just "mirror that". (as @jjmata already pointed out). I'd suggest not get inspiration but rather just go with that, to be consistent. I understand that the expected behaviour is the same, right? A few things to lift over:
Two smaller things while we're here:
Not blocking, but worth flagging: now both budget and reports hand-roll the disabled icon-link. A I also ran this through Claude Design and here is an interesting suggestion that requires a bit more work, if we're feeling that it goes in the right direction: 
If we end up introducing a new component, we might as well add it as a Design System component or just a separate template that could be reused. Another option, is to re-use the UI we have on Budgets, but skip the period selector (the arrow and popup). This would better align with the domain behaviour on reports. 
|
github-project-automation
Bot
moved this from Todo
to Done
in Latest release train (v0.7.1) - May 2026
implemented your main suggestions, let me know your thoughts. 
|
3 participants
Closes #1681
Adds previous/next navigation arrows to the period display in the Reports view, allowing users to move between periods without manually changing dates.
Changes:
Monthly: navigate one month at a time
Quarterly: navigate one quarter at a time
YTD: navigate one year at a time
Last 6 months: navigate 6 months at a time, correctly extending to current month end when the window reaches the present
Custom: period display hidden (date picker is sufficient)
Next arrow is disabled when already at the current period
Summary by CodeRabbit
New Features
Localization
Tests