[cristim]

Closes #1531 using option 1 from the issue: refuse an empty selection. What an empty list should mean in the backend stays open for the owner; see the issue comment.

Problem: the backend disagrees with itself about an empty enabled_providers:

• config.FilterRecommendationsByEnabledProviders treats empty as "all" (internal/config/recommendation_enabled_providers.go:7-9). The Opportunities handler, the scan handler and the plan-step resolver all use it.
• The scheduler collects only the listed providers (internal/scheduler/scheduler.go:322), so empty means "collect nothing".

So an admin who unchecks the last enabled provider stops collection, while stored recommendations from every provider keep showing. #1530's rebase made it easier to send []: unchecking the last provider the server still has enabled now sends an empty list.

Change (frontend/src/settings.ts): in saveGlobalSettings, right after rebaseMultiFieldKeys merges this tab's checkbox changes onto the server's current list, an empty enabled_providers aborts the whole save. It shows "Enable at least one provider before saving." and sends no config PUT and no service PUT. The snapshot is untouched, and the existing finally re-enables Save. Nothing changes when the providers weren't edited, so a fresh install whose stored default is [] still saves its other settings. There are no API or backend changes.

Tests (written first):

• Jest (settings.test.ts):
  • (a) the server has ['aws'] and the user unchecks AWS: refused, with the toast, and no updateConfig or updateServiceConfig call;
  • (b) the server has ['aws','gcp'] and the user unchecks AWS: ['gcp'] is sent;
  • (c) the server has [] and the user edits an unrelated field: it saves, without an enabled_providers key;
  • (d) another admin's ['gcp'] keeps the merged list non-empty, so it saves.
  • On main only (a) fails: it gets "Settings saved successfully".
  • The existing "handles missing form elements gracefully" test empties the DOM, which yields []. It now asserts the save resolves without throwing and sends nothing.
• Playwright (settings-save-changed-fields.spec.ts, on /admin/general where the provider checkboxes live): uncheck AWS, click Save, then the toast shows and no PUT is sent. It fails on main's settings.ts (no toast, also on the retry).
• Mutation: disabling the check fails (a).

Local (macOS):

• tsc --noEmit and eslint on the changed files are clean.
• The settings jest suites pass (10 suites, 325 tests).
• settings-save-changed-fields.spec.ts passes 3/3, run with CI=1 under the shared port lock.
• Pre-commit printed Passed for every applicable hook, with SKIP=hadolint,actionlint,zizmor,cfn-lint, because those hooks' environments can't be built locally. CI runs them.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Prevented settings from being saved when no providers are enabled. An error message now prompts you to enable at least one provider, and no configuration changes are submitted.
  • Saving remains available after the validation message, so you can correct the selection and try again.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/settings-refuse-empty-providers
• File: .coderabbit.yaml
• Commit: 4db63f1
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: b3e69377-12e5-4e02-a2f4-eb87fb8aaa82

Commits

Reviewing files that changed from the base of the PR and between 7d049bc and 4db63f1.

Files selected for processing: 3

• frontend/src/__tests__/settings.test.ts
• frontend/src/settings.ts
• frontend/tests-e2e/settings-save-changed-fields.spec.ts

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Walkthrough

The settings save path now rejects a changed provider list when it is empty. Unit and end-to-end tests cover the rejection and related save cases.

Changes: Provider settings save

Layer / File(s)	Summary
Reject empty provider selections frontend/src/settings.ts, frontend/src/__tests__/settings.test.ts, frontend/tests-e2e/settings-save-changed-fields.spec.ts	The save path shows an error and skips the global settings update when the rebased provider list is empty. Tests cover this behavior, saving with another provider enabled, unrelated edits with an empty stored list, and a provider enabled in the latest server response.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

• LeanerCloud/cloud-commitments-platform#1487: This PR changes the same settings save path and provider-list write behavior.

Suggested reviewers: iamjsd

Merge Risk: Minimal · up to 4db63

No merge-blocking risk remains from the reviewed change.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: refusing a settings save when it would leave no provider enabled.
Linked Issues check		The PR implements option 1 for #1531. After rebasing provider changes, saveGlobalSettings refuses an empty enabled_providers list, shows Enable at least one provider before saving., and returns …
Out of Scope Changes check		The changes stay within #1531. They modify the settings save guard and add focused Jest and Playwright coverage for empty-provider behavior. No unrelated backend, API, or public declaration changes ar…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…

Full details: Linked Issues check

Explanation

The PR implements option 1 for #1531. After rebasing provider changes, saveGlobalSettings refuses an empty enabled_providers list, shows Enable at least one provider before saving., and returns before config or service updates. The existing finally path remains responsible for re-enabling Save. The snapshot is not advanced on this path. The Jest tests cover the single-provider refusal, multi-provider save, stored empty-list unrelated save, and concurrent-provider merge. The Playwright test verifies the toast and zero PUT requests.

Full details: Out of Scope Changes check

Explanation

The changes stay within #1531. They modify the settings save guard and add focused Jest and Playwright coverage for empty-provider behavior. No unrelated backend, API, or public declaration changes are included.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 3 files.

Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE 4db63f1

Scope: frontend only, frontend/src/settings.ts (+6), plus jest and Playwright tests. I read saveGlobalSettings (frontend/src/settings.ts:3518-3756) to its closing brace.

Findings: none blocking.

• Only 1 of the 4 new fix(settings): an empty enabled_providers means all providers on the backend but none on the page #1531 jest cases fails on base: the refusal case. The other three pass on base, so they are non-regression guards (for example, a non-empty merged list is still saved). [note]
• The pre-existing test that expected updateConfig to be called with an empty provider list when no checkbox exists now expects a refusal. That is the intended behaviour change. [note]
• The Playwright case was reviewed by reading, not run, to keep memory use light. [note]
• UX: the last provider checkbox can still be unticked with no inline warning; the refusal appears only on Save. That is acceptable, and an inline hint could follow. [non-blocking]

Attacked:

• Placement (:3654-3663): the check runs inside the grace_period_days or enabled_providers branch, after rebaseMultiFieldKeys has merged this tab's checkbox changes over a fresh GET. So it judges the list that would actually be written: changedGlobal.enabled_providers?.length === 0. It does not fire when this tab did not change providers, so an unrelated save is never blocked by an empty list already on the server.
• No partial save: in this function the global updateConfig PUT (:3664) comes before every service-config PUT (:3685 onward, collected and awaited at :3721). The refusal therefore returns before any request goes out. The user's service-field edits are not sent either, so the whole save is refused, which is the safe direction.
• State reset: the return sits inside the try that opens at :3636, so the finally (:3752-3754) resets saveInFlight and re-enables the Save button. The snapshot is not advanced, because that happens only after updateConfig succeeds, so a later Save re-detects the change.
• Root cause: the PR body's backend split (filters treat an empty list as all providers, while the scheduler collects nothing) is the reason never to write an empty list. The frontend guard closes the UI path. A direct API client can still PUT an empty enabled_providers. A backend validation that refuses an empty list would close that path at the root and could follow. [non-blocking]

Verified locally (node_modules cloned from an identical lockfile, under the heavy lock):

• jest settings.test.ts -t 1531: 4 passed. All settings suites: 325/325.
• Fail-before: the head test file on base settings.ts fails 1 of the 4 fix(settings): an empty enabled_providers means all providers on the backend but none on the page #1531 cases.
• tsc --noEmit: OK. eslint on settings.ts, the test file and the spec: OK.
• git merge-tree --write-tree origin/main 4db63f1: clean (b41374b7).

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE 4db63f1
PR #1542 is OPEN. One commit (4db63f1) on merge-base 7d049bc, which is current origin/main. Frontend only: settings.ts +6, settings.test.ts, settings-save-changed-fields.spec.ts.

Findings:

• nit frontend/src/tests/settings.test.ts:2237 - test (a) asserts that the refusal toast was shown, but not that it was the only toast. A mutant that throws after the toast instead of returning (M4) survives: the catch adds a second "Failed to save settings: ..." toast and no test notices. Asserting toHaveBeenCalledTimes(1) would kill M4. This does not block.
• nit frontend/src/tests/settings.test.ts:658 - the rewritten "handles missing form elements gracefully" test no longer checks that defaults are written when the DOM is empty. It now checks only that the save resolves and sends nothing. M1 shows the test does reach the new check: with the check disabled, the test fails. An empty DOM with a Save click is not a real path (the button is in the DOM), so the lost coverage does not matter. The new assertion is accurate.
• nit (pre-existing, not this PR) frontend/src/recommendations.ts:673-676 - this comment says the Settings load renders the same permissive default on the checkboxes. It does not: settings.ts:3097-3099 leaves every checkbox unchecked for a stored []. The PR relies on this behaviour, and it is safe here: a stored [] shows nothing checked, so a user cannot uncheck a provider that looks enabled and then be refused.
• note (scope, owner decision in fix(settings): an empty enabled_providers means all providers on the backend but none on the page #1531) - a direct PUT /api/config with enabled_providers: [] is still accepted. validateProviders (internal/config/validation.go:428-434) only checks membership. The PR is honest that it adds no API or backend change, so this is not a defect in the PR.

Backend claims, checked against the code:

• internal/config/recommendation_enabled_providers.go:7-9: empty means pass everything through. Confirmed. Its callers are internal/api/handler_recommendations.go:216 (Opportunities), internal/api/handler_recommendations_scan.go:57,67 (scan) and internal/planstep/resolve.go:116,260 (plan-step resolver). Confirmed.
• internal/scheduler/scheduler.go:322 and :354: both range over EnabledProviders, so an empty list collects nothing. Confirmed.
• Frontend filters treat empty the same way as the backend ones: lib/enabled-providers.ts:23 and dashboard.ts:278.
• Other readers: store_postgres.go:147,168,263-265 (defaults to []) and validation.go:429. None of them treats empty as meaningful.

Attacked:

• The early return inside the try (settings.ts:3659-3662). It sits before updateConfig, so the savedSnapshot loop (3667), showSavedGrace (3669), the defaultCoverageChanged side effects (3671-3677: snapshot, loadedServiceConfigs, showPropagatedCoverage), the filter validation and the service PUTs (3683+) and updateDirtyMarkers never run. defaultCoverageChanged is only computed, never acted on. The sentFields mutations all happen after this point. The finally block (3751-3754) resets saveInFlight and re-enables Save. Every dirty marker stays, so the user can still see the unsaved edits.
• The check runs after rebaseMultiFieldKeys (3479-3503), so it sees the merged list. Moving it before the rebase (M3) makes test (d) fail. ?.length === 0 ignores a grace-only edit where enabled_providers is undefined. M2 (!length) makes 5 tests fail. A fresh server value of null is turned into [] by ?? [].
• Fresh install with a stored []: the load renders every box unchecked (3092-3099), and when the providers are unchanged the key is deleted (3650-3652), so other edits still save. Test (c) covers this and passes.
• Other writers: the frontend calls updateConfig in only three places, ladder.ts:203 (laddering_enabled), recommendations.ts:848 (lookback_days) and settings.ts:3664. Only settings.ts can send enabled_providers. resetSettings (3795-3800) sets AWS checked, so it gives a non-empty list. No onboarding or setup flow writes providers, and nothing in cmd/ references enabled_providers. The API path is covered in the note above.
• Error bodies: no backend or error-path changes. The new toast is a fixed string.
• Jest, run under the heavy lock on a clean archive of 4db63f1: src/tests/settings* has 10 suites and 325 tests, all passing.
• Playwright: I read the spec at tests-e2e/settings-save-changed-fields.spec.ts:88-123 instead of running it, because memory is tight. It aborts non-local traffic, counts PUTs on /api/config and /api/config/service/**, unchecks #provider-aws on /admin/general, then asserts the toast and puts length 0. That matches the jest case.
• Mutation probes (settings.test.ts; file restored byte-identical to 4db63f1 and checked with cmp):
  M1, check disabled: killed, 2 tests fail ((a) and missing-elements).
  M2, !enabled_providers?.length: killed, 5 tests fail.
  M3, check moved before the rebase: killed, 1 test fails ((d)).
  M4, throw instead of return: SURVIVED (see the first nit).

[cristim]

Local verification at 4db63f1559e152e80510869fb3ec9c631ca52ea6 merged onto current main (7d049bcf; merge-tree b41374b7; macOS):

• Frontend: npm ci, tsc --noEmit and npm run build pass on the merged tree, and the full jest suite has no failures beyond those that fail identically on main in this local environment (CI runs the authoritative jest suite).
• Nothing was applied to any cloud.