Skip to content

fix(commitmentOptions): align savingsplans key (CodeRabbit nitpick sweep)#63

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/coderabbit-service-key
Apr 25, 2026
Merged

fix(commitmentOptions): align savingsplans key (CodeRabbit nitpick sweep)#63
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/coderabbit-service-key

Conversation

@cristim
Copy link
Copy Markdown
Member

@cristim cristim commented Apr 24, 2026

Summary

Addresses the only live CodeRabbit finding across all open/closed PRs (24–39, 52–54).

  • CodeRabbit flagged a key mismatch on PR #30: SERVICE_FIELDS in settings.ts:32 uses service: 'savingsplans' while commitmentConfigs in commitmentOptions.ts:58 registered the AWS entry under 'savings-plans'. Any lookup through isValidCombination/getCommitmentConfig with the non-hyphenated identifier silently fell through to _default.
  • CodeRabbit suggested renaming SERVICE_FIELDS to 'savings-plans', but that value also flows into api.updateServiceConfig(provider, service, cfg) at settings.ts:1443 — the service column is the DB primary key in the service_configs table, existing rows are ('aws', 'savingsplans'), and renaming without a migration would silently create duplicate rows. The dropdown at index.html:112,703 (<option value="savingsplans">) and the backend CLI flag (cmd/main.go:92) also use the non-hyphenated form.
  • Safer fix: rename the commitmentConfigs.aws key to savingsplans so every frontend call site — the SERVICE_FIELDS loop, the plan-form dropdown, and any direct lookup — resolves to the Savings-Plans-specific config. Update the two test cases.

Scope of the PR

PR Finding Status
#52 Actionable: guard catch block against non-Error throws ✅ already fixed in db429fe
#52 Nitpick: btn.disabled vs setAttribute, drop dead ?? 'Refresh' ✅ already fixed in db429fe
#30 Outside-diff: savingsplans vs savings-plans key mismatch fixed here
#30 Nitpick: double Promise.resolve() in settings.test.ts:854 ⤴ applies only to code on PR #30's branch
#30 Advisory: memorydb absent from SERVICE_FIELDS ⤴ forward-looking (no MemoryDB card exists yet)
All other PRs (24–29, 31–39, 53, 54, 3, 4) "No actionable comments" from CodeRabbit

Test plan

  • jest — 1244/1244 pass (all 35 suites)
  • tsc --noEmit — clean
  • grep confirms no remaining 'savings-plans' references in frontend code outside Go backend constants (pkg/common/types.go:49 is the backend canonical — unchanged, and execution.go:384 already accepts both forms)
  • Smoke test on the deployed AWS Lambda URL once CI finishes

🤖 Generated with claude-flow

Summary by CodeRabbit

  • Chores
    • Updated AWS savings plans configuration handling to improve consistency and reliability.

…opdown/DB

CodeRabbit flagged a key mismatch on PR #30: `SERVICE_FIELDS` in
`settings.ts` uses `service: 'savingsplans'`, while `commitmentConfigs`
in `commitmentOptions.ts` registered the AWS entry under `'savings-plans'`
(hyphenated). Any lookup via `isValidCombination`/`getCommitmentConfig`
with the non-hyphenated identifier silently fell through to `_default`
instead of hitting the Savings-Plans-specific config. Today both configs
are identical so behaviour is unchanged, but any future restriction
added to `'savings-plans'` would have been silently skipped.

CodeRabbit's suggestion was to rewrite `SERVICE_FIELDS` to the hyphenated
form, but `field.service` in `SERVICE_FIELDS` flows directly to
`api.updateServiceConfig(provider, service, cfg)` (settings.ts:1443)
which persists `service` as the primary key in the `service_configs`
table — existing production rows are `('aws', 'savingsplans')`, and
renaming SERVICE_FIELDS without a DB migration would have created
duplicate rows and orphaned the old config. The dropdown at
`index.html:112,703` (`<option value="savingsplans">`) and the backend
CLI flag (`cmd/main.go:92`) also use the non-hyphenated form.

Fix by renaming the `commitmentConfigs.aws` key from `'savings-plans'`
to `savingsplans` so every frontend call site — the SERVICE_FIELDS
path, the plan-form dropdown path, and any direct lookups — resolves
to the Savings-Plans-specific config. Update the two test cases in
`commitmentOptions.test.ts` to match.

This addresses the only live CodeRabbit finding across all open/closed
PRs (24-39, 52-54); PR #52's nitpicks were already resolved in db429fe,
and the remaining PR #30 nitpicks (double `Promise.resolve()` in
settings.test.ts:854, memorydb absent from SERVICE_FIELDS) apply only
to code that lives on PR #30's branch and not the base.
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 24, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 110d5d90-d7b6-4310-841c-df9591a4324e

📥 Commits

Reviewing files that changed from the base of the PR and between db429fe and e0260d2.

📒 Files selected for processing (2)
  • frontend/src/__tests__/commitmentOptions.test.ts
  • frontend/src/commitmentOptions.ts

📝 Walkthrough

Walkthrough

The AWS Savings Plans service identifier is updated from 'savings-plans' to savingsplans in the configuration implementation and corresponding test files to align with AWS naming conventions.

Changes

Cohort / File(s) Summary
Savings Plans Service Identifier Update
frontend/src/commitmentOptions.ts, frontend/src/__tests__/commitmentOptions.test.ts
Renamed the service configuration key for AWS Savings Plans from 'savings-plans' to savingsplans across implementation and test files, including updated test descriptions.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A simple rename, so clean and bright,
From hyphens lost to letters tight,
savingsplans now stands its ground,
Where AWS conventions are found,
Configuration unified, tests aligned—perfection found! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main change: aligning the savingsplans key in commitmentOptions from 'savings-plans' to 'savingsplans' to fix a key mismatch across the codebase.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/coderabbit-service-key

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim cristim merged commit 148c4b1 into feat/multicloud-web-frontend Apr 25, 2026
3 checks passed
@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-sprint Within the current sprint impact/few Limited audience effort/xs Trivial / one-liner type/chore Maintenance / non-user-visible labels Apr 28, 2026
@cristim cristim deleted the fix/coderabbit-service-key branch April 29, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant