fix(ux/settings): override surfaces reuse commitmentOptions validity rules (closes #107)#176
Conversation
|
@coderabbitai review |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 wires commitmentOptions validation into override UIs: inline override selectors and the create-override modal now limit payment options by valid (provider, service, term) combos, dynamically sync options on changes, and perform submit-side validation; tests added and two functions exported for test use. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Override UI (Inline/Modal)
participant Validation as commitmentOptions
participant API as Backend
User->>UI: Open modal / edit inline row
UI->>Validation: getValidPaymentOptions(provider, service, term)
Validation-->>UI: valid payment list
UI->>UI: Render payment dropdown (omit invalid options)
User->>UI: Change service or term
UI->>Validation: getValidPaymentOptions(provider, newService, newTerm)
Validation-->>UI: updated valid payment list
UI->>UI: Repopulate payment dropdown (preserve valid selection or reset)
User->>UI: Submit override (service, term, payment)
UI->>Validation: isValidCombination(provider, service, term, payment)
alt valid
Validation-->>UI: true
UI->>API: POST/PUT override
API-->>UI: Success
else invalid
Validation-->>UI: false
UI->>User: Show override-form-error, block submit
end
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 unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 5 minutes and 15 seconds. Comment |
✅ Actions performedReview triggered.
|
cristim
added
triaged
|
P2 bug fix: override UI surfaces previously ignored |
|
Re-requesting review now that the earlier CodeRabbit rate-limit window has passed.\n\n@coderabbitai review |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/settings.ts`:
- Around line 910-915: openOverrideModal currently creates a new onChange
function each time so removeEventListener is ineffective and handlers
accumulate; fix by defining and reusing a stable handler reference (e.g., move
the onChange function for syncOverridePaymentOptions out of openOverrideModal
into module scope or store it on the element as a property) or by using an
AbortController/Signal to attach listeners that can be aborted when the modal
closes; ensure you do not overwrite existing onchange logic like
handlePaymentOverrideChange — attach the reused handler to select and
override-term (or register with AbortController) so removeEventListener (or
abort) actually removes the prior listener instead of stacking handlers.
🪄 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: e4c82a72-ea03-4eda-b816-87053101f98a
📒 Files selected for processing (3)
frontend/src/__tests__/settings-accounts.test.tsfrontend/src/recommendations.tsfrontend/src/settings.ts
|
Addressed the latest listener cleanup comment: override payment-option listeners are now scoped to a per-open |
|
✅ Actions performedReview triggered.
|
…rules Closes #107. The two override UIs (inline payment selector from #72 and create-modal from #106) accepted any (service, term, payment) combination — including ones the global Settings → Purchasing cards already block as invalid. Concrete example: RDS 3yr no-upfront is hidden in the global RDS card via commitmentOptions.invalidCombinations, but the override modal saved it cleanly and the inline selector let the user pick it. Wired the override surfaces through the same commitmentOptions helpers the global cards use: 1. **Inline payment selector (`buildPaymentOverrideSelect`)**: now calls `getValidPaymentOptions(provider, service, term)` and only renders options that pass. For an RDS row with term=3, no-upfront is gone. When term is unset (override row created without a term), falls back to the full AWS list — can't pre-validate without it. If the row's CURRENT payment somehow isn't in the valid set (e.g. saved via direct API curl before this filter shipped, or AWS tightened the rules), still renders it as a dedicated "(invalid for term Ny — Delete the override and recreate)" option so the row isn't silently mutated. 2. **Override-create modal (`openOverrideModal`)**: new `syncOverridePaymentOptions` helper repopulates the payment dropdown on every service or term change, dropping invalid combinations automatically and snapping the selection back to "Inherit" if the previously-chosen payment is no longer valid for the new (service, term) pair. 3. **Submit-side validation (`submitOverrideForm`)**: when both term and payment are explicitly set, refuses to save invalid combos even if a stale select option leaked through (e.g. browser back-button restoring a stale dropdown state). Mirror of the global Settings → Purchasing card's checkCommitmentOptionCombo. Side change: exported `loadOverridesPanel` and `openOverrideModal` so the new tests can drive the panel and modal directly. Both are coherent units; just needed the export. Drive-by: removed unused `formatRelativeTime` import in `frontend/src/recommendations.ts` (pre-existing TS6133 blocking the pre-commit Run frontend tests hook on this branch — same fix as PR Tests: - `inline payment selector hides invalid options for RDS term=3 row` pins the RDS-no-3yr-no-upfront case. - `inline payment selector shows full list when term is unset` pins the fallback case. - `override-create modal hides invalid payment options on service+term change` pins the dependency loop and the reset- to-Inherit-when-stale snap. - All 40 existing settings-accounts tests still pass; 43 total. Backend save-side guard (the issue's third bullet) is deferred — the frontend gates close the practical bug; a backend mirror via the existing `commitmentopts.Service.Validate` machinery is its own PR-sized change touching `internal/api/handler_accounts.go` and would need probe-data integration tests. Worth filing as a follow-up.
Scope override payment-option listeners to an AbortController per modal open so reopening the modal cannot accumulate stale service/term handlers.
cristim
force-pushed
the
fix/issue-107-override-commitment-validation
branch
from
f050458 to
ddcc9a9
Compare
|
Rebased onto current |
|
✅ Actions performedReview triggered.
|
1 participant
Summary
Closes #107.
The two override UIs (inline payment selector from #72 and create-modal from #106) accepted any (service, term, payment) combination — including ones the global Settings → Purchasing cards already block. Concrete example: RDS 3yr no-upfront is hidden in the global RDS card via
commitmentOptions.invalidCombinations, but the override modal saved it cleanly and the inline selector let the user pick it.What changed
buildPaymentOverrideSelect)getValidPaymentOptions(provider, service, term). RDS+term=3 row shows onlyall-upfront+partial-upfront. Falls back to full list whentermis unset. Stale current-payment values are kept (so the row isn't silently mutated) but flagged with "(invalid for term Ny — Delete the override and recreate)".openOverrideModal)syncOverridePaymentOptionshelper repopulates the payment dropdown on every service or term change, snapping back to "Inherit" when the previously-chosen payment becomes invalid.submitOverrideForm)checkCommitmentOptionCombo.Tests
inline payment selector hides invalid options for RDS term=3 row— RDS-no-3yr-no-upfront case pinned.inline payment selector shows full list when term is unset— fallback case pinned.override-create modal hides invalid payment options on service+term change— dependency loop + snap-to-Inherit pinned.Out of scope
commitmentopts.Service.Validatemachinery is its own PR-sized change touchinginternal/api/handler_accounts.goand would need probe-data integration tests. Worth filing as a follow-up.Drive-by
Removed unused
formatRelativeTimeimport infrontend/src/recommendations.ts(pre-existing TS6133 blocking the pre-commitRun frontend testshook on every commit on this branch — same fix as PR #174).🤖 Generated with claude-flow
Summary by CodeRabbit
Tests
Chores
Bug Fixes