ux(settings): bulk-create service override modal applies to multiple services (closes #119) - #844
Conversation
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds bulk-mode to the "Add service override" modal: compute availableServices excluding already-overridden services, toggle between single-service select and bulk checkbox list, re-evaluate checkbox compatibility on term/payment changes, route submission to a parallel fan-out with aggregated toasts, and add tests for UI/state/outcome permutations. ChangesBulk Override Modal
Sequence DiagramsequenceDiagram
participant User
participant OverrideModal
participant submitOverrideForm
participant submitBulkOverrideForm
participant OverrideAPI
participant UI
User->>OverrideModal: open modal (bulk toggle on)
User->>OverrideModal: click submit
OverrideModal->>submitOverrideForm: submit event
submitOverrideForm->>submitBulkOverrideForm: delegate (bulk on)
submitBulkOverrideForm->>OverrideAPI: parallel save for each checked service
OverrideAPI-->>submitBulkOverrideForm: per-service results
submitBulkOverrideForm->>UI: show aggregated toast or inline error
submitBulkOverrideForm->>OverrideModal: close modal on success or partial success
submitBulkOverrideForm->>OverrideModal: keep modal open on full failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
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)
1675-1778:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the bulk selection before fan-out.
Lines 1679-1681 jump into bulk mode before the
isValidCombination(...)guard on Lines 1693-1701 runs. AWS validity is service-specific, so a term/payment pair that is valid for the hidden#override-serviceselect can still be invalid for some checked services, producing deterministic partial failures that the frontend already knows how to prevent.Suggested fix
const bulkToggle = document.getElementById('override-bulk-toggle') as HTMLInputElement | null; const isBulk = bulkToggle?.checked ?? false; if (isBulk) { + const checkedServices = Array.from( + document.querySelectorAll<HTMLInputElement>('`#override-bulk-services-list` input[type="checkbox"]:checked'), + ).map(cb => cb.value); + + if (req.term != null && req.payment) { + const invalid = checkedServices.filter(service => + !isValidCombination(ctx.provider, service, req.term!, req.payment!), + ); + if (invalid.length > 0) { + const errEl = document.getElementById('override-form-error'); + if (errEl) { + errEl.textContent = `Unsupported term/payment selection for: ${invalid.join(', ')}.`; + } + return; + } + } + await submitBulkOverrideForm(ctx, req); return; }🤖 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 1675 - 1778, submitBulkOverrideForm currently skips the per-service validity check and can attempt saves for services that don't support the requested term/payment; before creating the Promise.allSettled fan-out, iterate the checked services (variable checked) and call isValidCombination(ctx.provider, service, req.term, req.payment) for each, collect any invalid services, and if any exist set override-form-error to a clear message listing the offending services and return early; otherwise proceed with the existing save logic. Ensure you reference submitBulkOverrideForm, checked, ctx.provider, req.term, req.payment, isValidCombination and errEl when implementing the validation.
🤖 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 `@frontend/src/__tests__/settings-accounts.test.ts`:
- Around line 1921-1935: The test "already-overridden services are excluded from
the checkbox list" calls loadAccountsForProvider('aws') without explicitly
mocking api.listAccounts, making it flaky; add an explicit mockResolvedValue for
api.listAccounts at the start of this test (similar to other tests) so
loadAccountsForProvider receives a deterministic accounts list, while keeping
the existing api.listAccountServiceOverrides mock; update the test to mock
api.listAccounts (and api.listAccounts should return the account(s) expected by
loadAccountsForProvider) before calling loadAccountsForProvider('aws').
In `@frontend/src/index.html`:
- Around line 1102-1108: The markup nests a <label> inside another for the bulk
override toggle which is invalid and causes odd click/focus behavior; update the
template so each toggle (for the element using id="override-bulk-toggle" and
likewise for the other toggles account-enabled and account-aws-is-org-root) uses
a single wrapping label (or a single input outside with a separate text element)
and remove the inner <label>; follow the existing pattern used elsewhere by
giving the descriptive text an id and adding aria-labelledby="<that-id>" on the
<input> (keep the <span class="slider"> for styling) so the input remains
accessible and the nested label is eliminated.
In `@frontend/src/settings.ts`:
- Around line 1534-1580: In applyOverrideBulkMode: when bulk is false the
function currently returns before restoring the single-service submit button
state, leaving `#override-submit-btn` disabled after toggling back from bulk; fix
by, before returning in the (!bulk) branch, recomputing submitBtn.disabled using
the single-service control inside singleRow (e.g., find the single select or
input via singleRow.querySelector('select') or appropriate input) and set
submitBtn.disabled = <true if nothing selected/empty, false otherwise>, handling
nulls for singleRow and submitBtn.
---
Outside diff comments:
In `@frontend/src/settings.ts`:
- Around line 1675-1778: submitBulkOverrideForm currently skips the per-service
validity check and can attempt saves for services that don't support the
requested term/payment; before creating the Promise.allSettled fan-out, iterate
the checked services (variable checked) and call
isValidCombination(ctx.provider, service, req.term, req.payment) for each,
collect any invalid services, and if any exist set override-form-error to a
clear message listing the offending services and return early; otherwise proceed
with the existing save logic. Ensure you reference submitBulkOverrideForm,
checked, ctx.provider, req.term, req.payment, isValidCombination and errEl when
implementing the validation.
🪄 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: c313cd42-78a0-4b8e-89ce-4beb34feb41b
📒 Files selected for processing (3)
frontend/src/__tests__/settings-accounts.test.tsfrontend/src/index.htmlfrontend/src/settings.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
… rows Eliminate nested <label> inside <label> (invalid HTML) for the bulk-override toggle and the two account-form toggles (account-enabled, account-aws-is-org-root). Replace the outer <label class="toggle-row"> with <div class="toggle-row">, add a <span id="..."> for the descriptive text, and wire aria-labelledby on each <input> -- the same pattern already used for the global settings toggles (issue #464). Addresses CR finding on PR #844.
|
Addressed remaining CR finding (nested label elements) in commit c5cc243. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
All CR findings addressed: listAccounts mock in 7cda593, nested labels in c5cc243, submit state restore in a2259f6. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
All CR findings from the initial review (2026-05-28) are addressed in HEAD (c5cc243):
No new commits needed; all four findings are satisfied by the three commits already on the branch. @coderabbitai review |
|
Triggering a review of the latest commits now. ✅ Actions performedReview triggered.
|
Add "Apply to multiple services" toggle to the existing per-account override modal. When enabled, the single-service <select> is replaced by a checkbox group listing unconfigured services. On save, all checked services receive the same term/payment/coverage via Promise.allSettled (parallel PUTs, no new backend endpoint). A single aggregated toast reports success count on full success, names the failed service(s) on partial failure, and surfaces an inline error on total failure while keeping the modal open for retry. Existing overrides are excluded from the checkbox list. The toggle resets to off on every close. Tests: 8 new cases in settings-accounts.test.ts covering toggle UI, submit-disabled-until-checked, 3/3 success, 2/3 partial failure, 0/3 total failure, no-selection guard, exclusion of existing overrides, and modal-reset-on-close.
… state on bulk toggle-off Per-service isValidCombination check in submitBulkOverrideForm blocks invalid (provider, service, term, payment) combos before Promise.allSettled fan-out, mirroring the existing single-service path guard. applyOverrideBulkMode now recomputes submitBtn.disabled from the single-service <select> value when toggling back to single mode, so the button is not left stuck disabled.
…n test Removes order-dependent flakiness by providing an explicit deterministic accounts list, so the test no longer relies on state from a previous test that happened to call loadAccountsForProvider.
… rows Eliminate nested <label> inside <label> (invalid HTML) for the bulk-override toggle and the two account-form toggles (account-enabled, account-aws-is-org-root). Replace the outer <label class="toggle-row"> with <div class="toggle-row">, add a <span id="..."> for the descriptive text, and wire aria-labelledby on each <input> -- the same pattern already used for the global settings toggles (issue #464). Addresses CR finding on PR #844.
|
Rebased on Conflicts resolved (additive merges on both sides):
Post-rebase verification: All prior CR pass-1 findings remain addressed (confirmed by CR in commits @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
|
@coderabbitai review |
Rate Limit Exceeded
|
…fore save (refs #119) In bulk mode the override modal now proactively disables (and greys out via title annotation) service checkboxes whose service does not support the currently chosen term/payment combination, mirroring the existing single-select logic (isValidCombination from commitmentOptions.ts). Previously the UI blocked the entire bulk save at submit time with an inline error if any checked service conflicted, forcing the user to manually find and deselect it. The submit-time guard is kept as a belt-and-suspenders defence, but is now unreachable in normal use. Changes: - Add syncBulkServiceConflicts(provider) that iterates the bulk checkbox list, calls isValidCombination per service, and disables/re-enables each - Call it immediately after the checkbox list is built in applyOverrideBulkMode - Extend the onChange listener in openOverrideModal to call it whenever term or payment changes (also wires the payment select to onChange, previously untouched by that handler) - Add two tests in settings-accounts.test.ts: one asserts rds is disabled when 3yr/no-upfront is chosen and the API is never called; the other asserts rds is re-enabled when the combo changes back to a valid one
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
<select>is swapped for a checkbox group listing unconfigured services; already-overridden services are excludedPromise.allSettled(parallel PUTs -- no new backend endpoint needed)Failure handling
Batch strategy
Frontend fan-out via
Promise.allSettled-- all NPUT /api/accounts/:id/service-overrides/:provider/:servicecalls fire in parallel. No new batch endpoint needed; the backend already handles idempotent individual PUTs.Test plan
npx tsc --noEmitcleannpx jest-- 2150 pass / 0 failsettings-accounts.test.ts:Summary by CodeRabbit
New Features
Bug Fixes / UX
Accessibility
Tests