fix(insights): correct the budget card's figure, badge noise and toast a11y#2799
fix(insights): correct the budget card's figure, badge noise and toast a11y#2799gariasf wants to merge 3 commits into
Conversation
…t a11y
Four defects found while reviewing the insights surfaces for hierarchy.
**The budget_at_risk card's focal figure argued against its own headline.**
`insight_key_figure` returned `budget_spent_pct` for both budget cards, so
"2 categories need attention in your budget" displayed "14% / of budget" —
a reassuring number as the visual focus of a warning. It now leads with the
flagged count ("2 / need attention"); budget_on_track keeps the percentage,
where overall consumption genuinely is the subject.
**The "New" pill carried no information.** Visiting /insights marks every
insight read in one `update_all`, so at first paint the pill was on every
row. On the page it becomes a dot — same signal, without an uppercase
tracked chip stealing weight from the title beside it. In the dashboard
widget it goes entirely: the well's header already counts unread ("New · 3")
and, with three rows, the pill was usually on all of them.
**The undo toast was silent to screen readers.** A card leaves the page via
a Turbo `remove`, which announces nothing, and the toast that explains it
had no live region — unlike its neighbour `_sync_toast`, which sets
`role="status" aria-live="polite"`.
**The undo toast could only be closed with a mouse.** Its close affordance
was a bare `icon "x"` with a click action: not focusable, not named. Now a
real `DS::Button`, matching `_sync_toast`.
The controller test asserting a per-row badge is updated to assert the
header count that replaces it, and to lock in the pill's removal.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughInsights now use acknowledgement and unacknowledgement actions across status handling, routes, controllers, Turbo Streams, cards, and dashboard feeds. Budget figures distinguish attention counts from budget percentages, and unread indicators and toast accessibility are updated. ChangesInsights acknowledgement flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InsightCard
participant InsightsController
participant Insight
participant DashboardFeed
participant NotificationTray
InsightCard->>InsightsController: PATCH acknowledge
InsightsController->>Insight: acknowledge!
InsightsController->>DashboardFeed: reload visible feed
InsightsController->>InsightCard: remove acknowledged card
InsightsController->>NotificationTray: append undo toast
NotificationTray->>InsightsController: PATCH unacknowledge
InsightsController->>Insight: unacknowledge!
InsightsController->>DashboardFeed: refresh feed and insight list
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 (1)
test/controllers/insights_controller_test.rb (1)
33-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the unread count, not only the header label.
The current selector would pass if the header rendered “New” but omitted the numeric count. Match the separator and a number, or assert the fixture’s expected count, so this test verifies the new contract.
Proposed test adjustment
- assert_select "`#insights-feed` p", text: /#{Regexp.escape(I18n.t("insights.feed.header_new"))}/ + assert_select "`#insights-feed` p", text: /#{Regexp.escape(I18n.t("insights.feed.header_new"))}.*\d+/🤖 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 `@test/controllers/insights_controller_test.rb` around lines 33 - 34, Update the insights feed assertion in the relevant controller test to verify the unread count as part of the header text, not just the “New” label. Match the expected separator and numeric count, or use the fixture’s known count, while preserving the existing header selector and zero-card assertion.
🤖 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 `@test/controllers/insights_controller_test.rb`:
- Around line 33-34: Update the insights feed assertion in the relevant
controller test to verify the unread count as part of the header text, not just
the “New” label. Match the expected separator and numeric count, or use the
fixture’s known count, while preserving the existing header selector and
zero-card assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df923547-b080-4553-9253-7701efb64444
📒 Files selected for processing (7)
app/helpers/insights_helper.rbapp/views/insights/_insight_card.html.erbapp/views/insights/_undo_toast.html.erbapp/views/pages/dashboard/_insights_feed.html.erbconfig/locales/views/insights/en.ymltest/controllers/insights_controller_test.rbtest/helpers/insights_helper_test.rb
Two complaints about the insight feed: the close (×) control felt wrong,
and clearing an insight was only possible on /insights — not on the
dashboard widget, which is the surface people actually look at.
**The × was lying.** Dismissal has never been permanent. GenerateInsightsJob
resurfaces a row whose bucketed metadata changes materially "even if the user
had read or dismissed the stale version" (its own comment), and 6 of 8
generators scope dedup_key to a month token, so dismissing July's budget card
says nothing about August's. A destructive-looking control was performing a
non-destructive act. It is now "Got it", and the contract is statable:
acknowledgement covers the numbers you saw; new numbers are a new insight.
No migration. The DB value stays "dismissed" and dismissed_at keeps its name;
only the enum key and the vocabulary the code speaks change, so existing rows
stay hidden and become undoable under an honest label.
**The action pyramid was inverted.** The escape hatch was a chromed icon
button in the card's top-right — the strongest secondary scan position — while
the card's actual purpose ("View budget") was a borderless ghost link under
the body text. Both now sit in a footer strip: the subject action gets the
chrome, acknowledging is quiet labelled text beside it, and the key figure
gets the corner to itself instead of competing with a control.
**The widget can clear its own rows.** Each row gains an acknowledge control,
revealed on pointer hover, on keyboard focus, and shown unconditionally on
touch where there is no hover. No gesture, so the section's drag-to-reorder
handlers are untouched. The row becomes a stretched link plus a sibling
button, because button_to renders a <form> and a form cannot nest in an <a>.
The group is named (group/insight). The dashboard <section> is itself a
`.group` for its header controls, and a bare group-hover: matches any ancestor
group — hovering one row, or the section header, revealed every row's control.
Acknowledging re-renders the well rather than removing a row, so the next
insight is promoted into the freed slot; Insight::FEED_LIMIT is now shared
between the two controllers that render it so they cannot drift. Undo restores
the row on both surfaces, and carries autofocus so it is one keystroke away
after the acknowledged card leaves the DOM.
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/models/insight.rb`:
- Around line 61-64: Update Insight#unacknowledge! to perform the state change
only when the insight is currently acknowledged; leave expired or otherwise
non-acknowledged insights unchanged, while preserving the existing read status
and timestamp behavior for valid reversals.
🪄 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 Plus
Run ID: e677dd4b-217a-4be3-ad02-cb15ce4bc407
📒 Files selected for processing (15)
app/controllers/insights_controller.rbapp/controllers/pages_controller.rbapp/models/insight.rbapp/views/insights/_insight_card.html.erbapp/views/insights/_undo_toast.html.erbapp/views/insights/acknowledge.turbo_stream.erbapp/views/insights/dismiss.turbo_stream.erbapp/views/insights/unacknowledge.turbo_stream.erbapp/views/insights/undismiss.turbo_stream.erbapp/views/pages/dashboard/_insights_feed.html.erbconfig/locales/views/insights/en.ymlconfig/routes.rbtest/controllers/insights_controller_test.rbtest/jobs/generate_insights_job_test.rbtest/models/insight_test.rb
💤 Files with no reviewable changes (2)
- app/views/insights/undismiss.turbo_stream.erb
- app/views/insights/dismiss.turbo_stream.erb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/pages/dashboard/_insights_feed.html.erb
|
Reviewed the diff, files, and prior CodeRabbit passes. Notes on the three bundled fixes: 1. Budget card figure ( 2. Badge noise reduction 3. Toast a11y ( i18n: only Pre-existing, still open: CodeRabbit's review-thread comment on Overall: the rename ( Generated by Claude Code |
CodeRabbit, Major: an arbitrary/stale PATCH /unacknowledge (e.g. an old undo-toast link clicked after GenerateInsightsJob has since expired or resurrected the insight) could force it back to :read regardless of its actual current state — including pulling an :expired insight back into visible view. Guards the transition to only reverse an actual acknowledgement, per CodeRabbit's suggested fix.
3 participants
Summary
Four defects found while reviewing the Insights surfaces for hierarchy and affordance. All independent of each other and of the larger affordance work, so they're batched here to land on their own.
This is the first of a short series. The follow-up replaces the corner dismiss
×with an acknowledgement in the card's action row and adds the same control to the dashboard widget, which today has no way to clear an insight at all.1. The
budget_at_riskcard's focal figure argued against its own headlineinsight_key_figurereturnedbudget_spent_pctfor both budget cards. So the card headlined "2 categories need attention in your budget" displayed "14% / of budget" as its focal number — a reassuring figure as the visual anchor of a warning.It now leads with the flagged count.
budget_on_trackkeeps the percentage, where overall consumption genuinely is the subject.2. The "New" pill carried no information
InsightsController#indexmarks every insight read in oneupdate_all, so at first paint the pill was on every row — the only moment it renders is the moment it differentiates nothing.DS::Pill(dot_only: true), already in the component's API). Same signal, without an uppercase tracked chip stealing weight from the title beside it.3. The undo toast was silent to screen readers
Dismissing a card removes it via a Turbo
remove, which announces nothing. The toast that explains what happened had no live region — unlike its neighbourshared/notifications/_sync_toast, which setsrole="status" aria-live="polite". Now it matches.4. The undo toast could only be closed with a mouse
Its close affordance was a bare
icon "x"with aclick->element-removal#removeaction — an SVG, so neither focusable nor named. A keyboard user could reach Undo but never dismiss the toast. Now a realDS::Button(variant: "icon")with anaria-label, again matching_sync_toast.Notes
dot_onlyandDS::Buttonalready exist.insights.figures.need_attention.test/controllers/insights_controller_test.rbasserted the per-row badge exists. Updated to assert the header count that replaces it, pluscount: 0on the row pill so the removal is locked in rather than silently reversible._insights_feed.html.erband_insight_card.html.erb, which feat(insights): gate the insights feed behind preview features #2788 also edits (adding Preview pills). Different lines, so a textual conflict is unlikely, but worth knowing if feat(insights): gate the insights feed behind preview features #2788 lands first.Test plan
bin/rails test test/helpers/insights_helper_test.rb test/controllers/insights_controller_test.rb test/models/insight_test.rb— 31 runs, 0 failuresbin/rubocopclean on the three Ruby filesbundle exec erb_lintclean on the three templatesbudget_spent_pctstill correct on an on-track card, unread dots render on the page, widget header shows "New · 3" with no per-row pills, undo toast reachable and closable by keyboardSummary by CodeRabbit