feat(settings): per-product Coverage and Enabled controls on SP cards (closes #136) - #580
Conversation
…closes #136) Add Coverage % and Enabled toggle to each of the four Savings Plans cards (Compute, EC2 Instance, SageMaker, Database), matching the per-product settings already available on RI cards. - SERVICE_FIELDS SP entries extended with coverageId/enabledId - TRACKED_FIELDS includes the new IDs for dirty-state detection - loadGlobalSettings populates per-card coverage/enabled from service config, falling back to global default_coverage when absent - saveGlobalSettings reads per-card coverage/enabled from DOM when the card exposes the controls; non-SP cards continue to inherit global settings - index.html: Coverage % number input and Enabled checkbox added to each SP card in the settings form - Tests: DOM fixture updated; 3 new tests cover save-coverage, save-enabled, and load-from-service-config paths; HTML structural assertions extended
|
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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements per-plan-type AWS Savings Plans configuration controls. New HTML inputs for coverage percentage and enabled state are added to each of four plan cards. The service field registry and dirty-tracking infrastructure are extended, and load/save logic is implemented to persist these settings bidirectionally with appropriate fallback behavior. ChangesPer-Plan Savings Plans Controls
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/settings.ts (1)
2674-2688:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback is skipped when an SP service row is absent.
Line 2676 only populates SP
coverage/enabledfor services that exist indata.services. If a card has no row, coverage stays at the HTML default (80) instead ofglobal.default_coverage, which can persist incorrect values on next save.💡 Suggested fix
- if (data.services) { - loadedServiceConfigs = data.services; - for (const svc of data.services) { + const services = data.services ?? []; + loadedServiceConfigs = services; + for (const svc of services) { const key = `${svc.provider}-${svc.service}`; const termEl = document.getElementById(`${key}-term`) as HTMLSelectElement | null; if (termEl) termEl.value = String(svc.term); const paymentEl = document.getElementById(`${key}-payment`) as HTMLSelectElement | null; if (paymentEl) paymentEl.value = svc.payment; - // Issue `#136`: populate per-product SP coverage and enabled fields when - // the card exposes them. Other service cards fall through (IDs absent). - const coverageEl = document.getElementById(`${key}-coverage`) as HTMLInputElement | null; - if (coverageEl) coverageEl.value = String(svc.coverage ?? data.global?.default_coverage ?? 80); - const enabledEl = document.getElementById(`${key}-enabled`) as HTMLInputElement | null; - if (enabledEl) enabledEl.checked = svc.enabled !== false; - } - } + } + + // Ensure SP cards are populated even when a service row is absent. + const byKey = new Map(services.map(s => [`${s.provider}-${s.service}`, s] as const)); + for (const field of SERVICE_FIELDS) { + const svc = byKey.get(`${field.provider}-${field.service}`); + if ('coverageId' in field && field.coverageId) { + const el = byId<HTMLInputElement>(field.coverageId); + if (el) el.value = String(svc?.coverage ?? data.global?.default_coverage ?? 80); + } + if ('enabledId' in field && field.enabledId) { + const el = byId<HTMLInputElement>(field.enabledId); + if (el) el.checked = svc?.enabled !== false; + } + }🤖 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/settings.ts` around lines 2674 - 2688, The current loop over data.services only sets coverage/enabled for services present in data.services, so coverage falls back to the HTML default (80) when a service card exists but has no row; to fix, initialize coverage/enabled for all service DOM rows first (or iterate service DOM cards) and then overlay values from data.services: build a map from data.services (e.g., loadedServiceConfigs) keyed by `${svc.provider}-${svc.service}`, then iterate the DOM elements (or querySelectorAll for elements with ids ending in `-coverage`/`-enabled`) compute the same `key`, and set coverageEl.value = String(map[key]?.coverage ?? data.global?.default_coverage ?? 80) and enabledEl.checked = map[key]?.enabled !== false so missing services get the global default rather than the HTML default.
🧹 Nitpick comments (1)
frontend/src/__tests__/settings.test.ts (1)
691-710: ⚡ Quick winStrengthen fallback assertions to avoid false positives.
This test currently passes even if fallback assignment is skipped, because both expected values (
80/true) equal fixture defaults. Seed a non-default initial DOM state and a non-80 global default beforeloadGlobalSettings().✅ Suggested test hardening
test('loadGlobalSettings populates SP coverage and enabled from service config (issue `#136`)', async () => { + // Make fallback observable (different from fixture defaults). + (document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement).value = '12'; + (document.getElementById('aws-savings-plans-ec2instance-enabled') as HTMLInputElement).checked = false; + (api.getConfig as jest.Mock).mockResolvedValue({ - global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 80 }, + global: { enabled_providers: ['aws'], default_term: 3, default_payment: 'all-upfront', default_coverage: 67 }, services: [ { provider: 'aws', service: 'savings-plans-compute', term: 1, payment: 'no-upfront', coverage: 65, enabled: false }, ], }); setupSettingsHandlers(); await loadGlobalSettings(); @@ const ec2Coverage = document.getElementById('aws-savings-plans-ec2instance-coverage') as HTMLInputElement; const ec2Enabled = document.getElementById('aws-savings-plans-ec2instance-enabled') as HTMLInputElement; - expect(ec2Coverage.value).toBe('80'); + expect(ec2Coverage.value).toBe('67'); expect(ec2Enabled.checked).toBe(true); });🤖 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/__tests__/settings.test.ts` around lines 691 - 710, The test should be hardened so fallback behavior is actually verified: before calling loadGlobalSettings() set the initial DOM inputs for the fallback cards (e.g., elements with ids 'aws-savings-plans-ec2instance-coverage' and 'aws-savings-plans-ec2instance-enabled') to values that differ from the fixture defaults (e.g., coverage != '80' and enabled != true), and change the mocked global default in api.getConfig to a non-80 value; then call setupSettingsHandlers() and loadGlobalSettings() and assert the values were updated to the new global defaults or service-specific values to ensure fallback assignment ran (refer to loadGlobalSettings, setupSettingsHandlers and the element ids used in the test).
🤖 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.
Outside diff comments:
In `@frontend/src/settings.ts`:
- Around line 2674-2688: The current loop over data.services only sets
coverage/enabled for services present in data.services, so coverage falls back
to the HTML default (80) when a service card exists but has no row; to fix,
initialize coverage/enabled for all service DOM rows first (or iterate service
DOM cards) and then overlay values from data.services: build a map from
data.services (e.g., loadedServiceConfigs) keyed by
`${svc.provider}-${svc.service}`, then iterate the DOM elements (or
querySelectorAll for elements with ids ending in `-coverage`/`-enabled`) compute
the same `key`, and set coverageEl.value = String(map[key]?.coverage ??
data.global?.default_coverage ?? 80) and enabledEl.checked = map[key]?.enabled
!== false so missing services get the global default rather than the HTML
default.
---
Nitpick comments:
In `@frontend/src/__tests__/settings.test.ts`:
- Around line 691-710: The test should be hardened so fallback behavior is
actually verified: before calling loadGlobalSettings() set the initial DOM
inputs for the fallback cards (e.g., elements with ids
'aws-savings-plans-ec2instance-coverage' and
'aws-savings-plans-ec2instance-enabled') to values that differ from the fixture
defaults (e.g., coverage != '80' and enabled != true), and change the mocked
global default in api.getConfig to a non-80 value; then call
setupSettingsHandlers() and loadGlobalSettings() and assert the values were
updated to the new global defaults or service-specific values to ensure fallback
assignment ran (refer to loadGlobalSettings, setupSettingsHandlers and the
element ids used in the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a042b4d-fa09-4880-b52b-13453b4e3596
📒 Files selected for processing (3)
frontend/src/__tests__/settings.test.tsfrontend/src/index.htmlfrontend/src/settings.ts
…sent loadGlobalSettings only populated per-product Savings Plans coverage and enabled controls for services present in the config response. A SP card whose service row was missing kept the HTML default (80) instead of falling back to global.default_coverage, which then persisted an incorrect value on the next save. Iterate SERVICE_FIELDS (the source of truth for SP card IDs) and overlay any matching service-config values, so every SP card gets the global default when its row is absent. Harden the regression test with a non-default seeded DOM state and a non-80 global default so the fallback assignment is actually exercised. Addresses CodeRabbit review on PR #580.
|
Addressed the CodeRabbit review (commit Major (outside-diff) Confirmed real: Fix: split the term/payment loop from the SP-control population, then iterate Nitpick Seeded a non-default initial DOM state on the fallback card ( Verification: No out-of-scope follow-ups discovered. @coderabbitai review |
|
Triggering a fresh review of the updated commits now. ✅ Actions performedReview triggered.
|
Summary
SERVICE_FIELDSSP entries extended withcoverageId/enabledId;TRACKED_FIELDSpicks them up for dirty detectionloadGlobalSettingsreads per-card coverage/enabled from service config, falling back todefault_coveragewhen absentsaveGlobalSettingsreads per-card coverage/enabled from DOM controls when present; non-SP cards continue to inherit global settings unchangedTest plan
npx tsc --noEmitpasses (no type errors)npx jestpasses (1900/1900, +3 new tests covering save-coverage, save-enabled, load-from-service-config)Summary by CodeRabbit
New Features
Tests