feat(recommendations): usage_history + per-row sparkline (closes #239) - #801
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a "Coverage (7d)" sparkline column: backend fetches 7-day daily RI-coverage from AWS Cost Explorer and persists it; frontend types, table model, and rendering gain a ChangesUsage history sparkline visualization
Sequence Diagram(s)sequenceDiagram
participant GetRecommendationsForService
participant AttachDailyUsageHistory
participant groupRecsByTuple
participant GetDailyUsagePcts
participant CostExplorer
GetRecommendationsForService->>AttachDailyUsageHistory: recs list
AttachDailyUsageHistory->>groupRecsByTuple: build tuple index
loop per tuple
AttachDailyUsageHistory->>GetDailyUsagePcts: service, region, resourceType
GetDailyUsagePcts->>CostExplorer: GetReservationCoverage
CostExplorer-->>GetDailyUsagePcts: coverage periods
GetDailyUsagePcts-->>AttachDailyUsageHistory: daily percentages or nil
end
AttachDailyUsageHistory->>AttachDailyUsageHistory: assign UsageHistory to matching recs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Backend (AWS): add GetDailyUsagePcts / AttachDailyUsageHistory in providers/aws/recommendations/usage_history.go. Calls CE GetReservationCoverage with DAILY granularity over the last 7 days, batches by unique (service, region, resourceType) tuple so N recs for the same SKU fire only one CE call. Wired into GetRecommendationsForService after the existing rec fetch. Propagated through scheduler.convertRecommendations and the RecommendationRecord + common.Recommendation types. Frontend: add usage_history column (Coverage 7d) to the rec table with a 56x20px inline SVG polyline (renderUsageSparkline). Visual- only column (sortable:false) renders a plain non-interactive header. Degrades to "—" for null/absent data so pre-#239 cached rows and non-AWS providers are unaffected. Tests: 5 Go tests (daily points, nil-on-no-data, empty-input guard, end-to-end attach, empty-region skip) + 8 Jest tests (null/undefined/ empty degrade, single-point circle, 7-point polyline, 100%/0% y-axis mapping, full-width x-axis span). All 327 frontend tests pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/recommendations.ts (1)
2413-2432: ⚡ Quick winExpose sparkline values to assistive tech.
The SVG is marked
aria-hidden, so non-empty cells have no accessible text alternative. Add an accessible label on the SVG (or equivalent text in the cell).♿ Suggested patch
export function renderUsageSparkline(pcts: number[] | null | undefined): string { if (!pcts || pcts.length === 0) return '—'; + const clamped = pcts.map((v) => { + if (!Number.isFinite(v)) return 0; + return Math.max(0, Math.min(100, v)); + }); const w = 56; const h = 20; const pad = 1; const innerH = h - 2 * pad; - const n = pcts.length; + const n = clamped.length; + const aria = `RI coverage last ${n} days: ${clamped.map((v) => `${v.toFixed(1)}%`).join(', ')}`; if (n === 1) { - const cy = pad + innerH * (1 - pcts[0]! / 100); - return `<svg width="${w}" height="${h}" viewBox="0 0 ${w} ${h}" aria-hidden="true" class="usage-sparkline"><circle cx="${(w / 2).toFixed(1)}" cy="${cy.toFixed(1)}" r="2.5" fill="currentColor"/></svg>`; + const cy = pad + innerH * (1 - clamped[0]! / 100); + return `<svg width="${w}" height="${h}" viewBox="0 0 ${w} ${h}" role="img" aria-label="${aria}" class="usage-sparkline"><circle cx="${(w / 2).toFixed(1)}" cy="${cy.toFixed(1)}" r="2.5" fill="currentColor"/></svg>`; } - const points = pcts + const points = clamped .map((p, i) => { const x = (i / (n - 1)) * w; const y = pad + innerH * (1 - p / 100); return `${x.toFixed(1)},${y.toFixed(1)}`; }) .join(' '); - return `<svg width="${w}" height="${h}" viewBox="0 0 ${w} ${h}" aria-hidden="true" class="usage-sparkline"><polyline points="${points}" fill="none" stroke="currentColor" stroke-width="1.5" stroke-linejoin="round" stroke-linecap="round"/></svg>`; + return `<svg width="${w}" height="${h}" viewBox="0 0 ${w} ${h}" role="img" aria-label="${aria}" class="usage-sparkline"><polyline points="${points}" fill="none" stroke="currentColor" stroke-width="1.5" stroke-linejoin="round" stroke-linecap="round"/></svg>`; }🤖 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 `@frontend/src/recommendations.ts` around lines 2413 - 2432, The SVGs produced by renderUsageSparkline are marked aria-hidden so screen readers get no information; update renderUsageSparkline to provide an accessible label for non-empty pcts by removing aria-hidden and adding either a <title> element or an aria-label/role="img" that conveys the sparkline values (e.g., "Usage: 10%, 20%, 30%") for both the single-point branch (circle) and the multi-point branch (polyline), while keeping the empty-case return '—' unchanged.
🤖 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 `@frontend/src/recommendations.ts`:
- Around line 2413-2432: The SVGs produced by renderUsageSparkline are marked
aria-hidden so screen readers get no information; update renderUsageSparkline to
provide an accessible label for non-empty pcts by removing aria-hidden and
adding either a <title> element or an aria-label/role="img" that conveys the
sparkline values (e.g., "Usage: 10%, 20%, 30%") for both the single-point branch
(circle) and the multi-point branch (polyline), while keeping the empty-case
return '—' unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6d2426f-cd7e-4e57-b6a9-2c0ccd10c6bb
📒 Files selected for processing (11)
frontend/src/__tests__/recommendations.test.tsfrontend/src/api/types.tsfrontend/src/recommendations.tsfrontend/src/state.tsfrontend/src/types.tsinternal/config/types.gointernal/scheduler/scheduler.gopkg/common/types.goproviders/aws/recommendations/client.goproviders/aws/recommendations/usage_history.goproviders/aws/recommendations/usage_history_test.go
…nderUsageSparkline Replace aria-hidden="true" with role="img" and an aria-label conveying the coverage values (e.g. "RI coverage last 7 days: 80.0%, ...") on both the single-point and polyline SVG branches, so screen readers receive meaningful information. Also clamp non-finite inputs (NaN, Infinity, -Infinity) to 0 before computing geometry and the label, preventing invalid y-coordinates. Addresses CodeRabbit nitpick on PR #801.
Member
Author
|
Addressed the CodeRabbit nitpick from the 2026-06-01T18:34Z review. Finding fixed: The SVG elements were marked
Tests updated: added Commit: d614fd0 on @coderabbitai review |
Contributor
|
Triggering a review of the latest commit. ✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
providers/aws/recommendations/usage_history.goaddsGetDailyUsagePcts(CEGetReservationCoveragewithDAILYgranularity, last 7 days) andAttachDailyUsageHistory(batches by unique(service, region, resourceType)tuple so N recs for the same SKU fire one CE call). Wired intoGetRecommendationsForServiceafter the existing rec fetch; propagated throughscheduler.convertRecommendationsand theRecommendationRecord+common.Recommendationtypes.usage_historycolumn ("Coverage 7d") to the rec table rendering a 56x20px inline SVG polyline (renderUsageSparkline). Visual-only column (sortable: false) gets a plain non-interactive header; degrades to—for null/absent data so pre-feat(recommendations): populate usage_history + surface per-row sparkline in rec list #239 cached rows and non-AWS providers are unaffected.Test plan
go test ./...cd frontend && npx tsc --noEmitcd frontend && npx jest(327 pass, 0 fail)usage_historyfor EC2/RDS/ElastiCache/OpenSearchusage_historyfield) render "—" without errorsSummary by CodeRabbit
New Features
Tests