[cristim]

Closes #539. This is the last open part. Earlier parts landed in #570 (global keys sent only when changed), #1487 (service rows and keys) and #1495 (GCP payment repair).

Problem: the backend replaces grace_period_days and enabled_providers as a unit. Save sent each of them whole whenever any one of its inputs changed. So a tab opened before another admin's save wrote that admin's grace value or provider checkbox back. Separately, an emptied grace field was saved as 7 without telling the user.

Change (frontend/src/settings.ts, frontend only):

• When either key changed, Save first re-reads GET /api/config. It calls api.getConfig() directly, not the page cache. Then it overlays only the inputs this tab changed:
  • grace: the server's map, plus this tab's edited providers. A provider the server lacks and this tab didn't change stays absent.
  • providers: the server's list, with each edited checkbox's membership set to its state. This is idempotent and keeps the server's order.
• If that read fails or has no global, the whole save aborts with the error toast. No config or service PUT is sent and nothing falls back to the stale value.
• The filter that picks changed keys and the overlay share one predicate, decided from the values captured when Save is clicked (sentFields against the snapshot). An input edited while the re-read is pending is therefore not sent, and not misjudged; it stays changed for the next Save.
• Unchecking a provider removes every copy of it from the server list.
• After the save, the grace inputs show the persisted values. An input the user edited while the save ran is left alone, and the inline validators re-run.
• The provider checkboxes are not written back. Setting .checked from code fires no change event, so the provider sections and account loads wouldn't follow. A checkbox that differs only because another admin changed it is not marked changed, so it isn't re-sent.
• An emptied grace field is now refused, both inline (required: true) and on Save, instead of becoming 7.

Not done here:

• Optimistic concurrency (a 409 on a stale write, via an ETag or version) would close the race at the root, but it changes the API contract and needs 409 handling in the UI. There is no version check, so the GET-to-PUT window remains, at milliseconds against the hours a tab can sit stale.
• When two tabs change the same field, the later Save wins, as for every other setting.
• The other follow-ups from the fix(settings): repair GCP rows holding an AWS payment and refuse an empty cache window #1495 review (a GCP payment data migration, the stale-hours required test, a stronger propagation test) are not part of this PR.

Tests:

• Jest (settings-purchasing-grace.test.ts) covers:
  • a stale-tab grace value that is kept, shown, and not re-sent on the next save;
  • an edit typed during the PUT, which is kept;
  • the providers rebase, with no duplicate;
  • the abort when the read rejects or has no global, with a working retry afterwards;
  • no extra read when grace is unchanged;
  • a missing server key staying absent;
  • emptied grace refused, both inline and on Save. This replaces the old "empty defaults to 7" test.
• Failing-first: with main's settings.ts, 7 of the new or changed tests fail.
• Mutation probes all fail a test: overlaying every grace field, overlaying every provider, using the stale form map, dropping either abort, restoring empty -> 7, and dropping required.
• Playwright: a new case in settings-save-changed-fields.spec.ts drives the real Settings page. A later config read returns another admin's AWS change, and the user edits only Azure. The PUT carries {aws: 3, azure: 5, gcp: 7}, and the AWS input then shows 3. An emptied grace field shows the toast and sends no PUT.

Local (macOS, built dist):

• tsc --noEmit, npm run build and lint are clean.
• Full jest: 174 suites, 4557 tests passed, 0 failed.
• Playwright: 7/7 pass for settings-save-changed-fields, settings-range-error-reset and purchasing-sections, with CI=1 under the shared port lock.
• Pre-commit ran with SKIP=hadolint,actionlint,zizmor,cfn-lint, because those hooks' Python and Docker environments can't be built locally. They don't apply to these files, and CI runs them.
• Nothing was applied to any cloud.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 19 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available. Your 51 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/settings-rebase-multi-field-keys
• File: .coderabbit.yaml
• Commit: a9c47de
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: 61481cf7-ec9c-4efb-b318-7b90b0cfd7d6

Commits

Reviewing files that changed from the base of the PR and between 7e94bb2 and a9c47de.

Files selected for processing: 3

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

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

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

[cristim]

VERDICT: REQUEST_CHANGES 3ad2c8d
PR #1530 OPEN, head contains origin/main. Frontend only.

Findings:

• low frontend/src/settings.ts:3485,3492 - rebaseMultiFieldKeys runs isFieldChanged() (live DOM vs snapshot) after await api.getConfig(), but the overlaid values come from changedGlobal, which was read before the await. If an input changes during the GET, the stale value read at click time overwrites the fresh server value, which is the fix(config): Settings Save from a stale tab reverts the global default term, payment and coverage #539 revert. Reproduced in a scratch jest probe:
  (A) loaded aws=7, server aws=3, user edits azure=9 and clicks Save, then types aws=12 while the GET is pending. The PUT is {aws:7, azure:9, gcp:7}, which reverts the other admin's 3 to 7, and the 12 is not sent.
  (B) loaded ['aws'], server ['aws','gcp'], user checks azure and Saves, then checks gcp during the GET. The PUT is ['aws','azure'], which removes the gcp another admin enabled.
  Both self-heal on the next Save because the field stays dirty, but the intermediate write is wrong. The window is one GET round trip, and the user has to be typing during it. The existing "typed during the PUT" test covers only the PUT await. Fix: decide "changed" from the values captured at click time, e.g. sentFields[id] !== savedSnapshot[id] inside the rebase (sentFields is already built), and add a GET-pending test like probe A.
• nit frontend/src/settings.ts:3496 - indexOf + a single splice removes only the first copy of a duplicate. With server ['aws','aws'] (probe D), unchecking aws sends ['aws'] and the uncheck does nothing. The backend does not dedupe, so this is reachable only via API writes. Optional: filter all copies.
• nit (pre-existing semantics, not a regression) - with an empty enabled_providers meaning "all" in the backend (handler_recommendations.go:199), the rebase can produce [] while this tab still shows one box checked. Example: loaded ['aws','gcp'], server ['aws'], user unchecks aws. Checkboxes are deliberately not written back, so the tab shows gcp-only while the server is permissive-all. This is documented in the PR body. Worth a follow-up issue rather than a change here.

Attacked:

• Stale/partial snapshot: the stale-tab grace map is overlaid only for edited providers. A missing server key stays absent and renders as 7 with snapshot '7', so it is not re-sent (existing test plus reading the code). Server null/missing fields (probe C): grace_period_days:null gives {azure:9}, and enabled_providers:null gives ['gcp']. Both match the "overlay only edits" contract, and the backend accepts a partial map (handler_config.go nils the map and then unmarshals).
• Provider ordering/duplicates: server order is kept and the add is idempotent (test plus probe D above).
• Abort path: a rejected read or no global throws before updateConfig, so no config PUT and no service PUT (the service saves run after the throw point), and the snapshot is untouched. Retry works (test). Removing the explicit !fresh?.global guard would still abort, through a TypeError, so the guard only buys a clearer toast.
• Snapshot after success: the grace ids advance to the displayed persisted value, and sentFields is mutated in step so the final TRACKED_FIELDS loop agrees. Provider ids advance to the local checkbox state, so a differing server checkbox is not re-sent. After an updateConfig failure the snapshot is not advanced, the inputs are not overwritten, and the next Save re-reads.
• defaultCoverageChanged: decided before any await. A rebase abort skips the coverage mirror and propagation (all inside the try). No interaction with the service base rows.
• Empty -> 7: no other caller of readGraceInput. populateGraceInput(…,7) on reset/missing is unchanged. All 10 settings* jest suites (318 tests, including settings-permissions and settings.test) pass, so no other test relied on it. required: true on the three grace inputs matches stale-hours/coverage.
• Other senders of these keys: only saveGlobalSettings sends grace_period_days/enabled_providers. ladder.ts:203 and recommendations.ts:832 send single scalar keys. dashboard.ts and recommendations.ts only read them.
• Security/error bodies: no new error path reaches a lower-trust principal. The abort toast shows getConfig's error message, which is the same apiRequest error class the page already shows.
• Tests run: npm ci, then jest settings-purchasing-grace.test.ts plus settings.test.ts (148/148), then all settings* suites (318/318). npm run build OK. Playwright CI=1 under the shared lock: settings-save-changed-fields.spec.ts 2/2 passed, and the served bundle app.6df16b76.js matched the scratch dist. PR CI: all checks pass except Build Docker Image (amd64), which was pending at review time.
• Mutation probes (scratch copy, restored byte-identical by cmp):
  M1 grace overlay guard forced true (isFieldChanged(id) || id): 2 tests fail. KILLED. A plain guard removal fails to compile on the unused id.
  M2 showSavedGrace "edited during save" guard removed: 1 test fails. KILLED.
  M3 providers checked && at < 0 changed to checked (duplicate push): 1 test fails. KILLED.
  Probes A-D were temporary scratch tests and were removed afterwards (cmp identical).

[cristim]

VERDICT: APPROVE a9c47de
PR #1530, second pass. Delta reviewed: 3ad2c8d..a9c47de (settings.ts +10/-6, settings-purchasing-grace.test.ts +44).

Findings:

• nit frontend/src/settings.ts:3470-3471 - removing the old module-level isFieldChanged left two blank lines in a row. eslint and tsc are clean, so this is cosmetic only.
• nit (pre-existing, out of scope) settings.ts service path - changedServiceFields/readServiceFilterControls still read the live DOM after the global awaits. A service field edited during the global GET/PUT therefore goes out at its newest value, not its click-time value. This sends the user's own latest edit rather than reverting another admin's change, and the final TRACKED_FIELDS loop keeps that field dirty because live != sentFields. It is not the fix(config): Settings Save from a stale tab reverts the global default term, payment and coverage #539 revert class.

Prior finding (low, :3485/:3492, edits during the GET reverting server values): FIXED.

• isFieldChanged is now sentFields[id] !== savedSnapshot[id]. It is defined in saveGlobalSettings and used both for the changedGlobal filter and inside the rebase, so there is one predicate.
• sentFields is captured in the same synchronous tick as enabledProviders, gracePeriodDays and defaultCoverageChanged (no await in between), so the "changed" decision and the overlaid values come from the same moment.
• savedSnapshot can only move during the GET through another save, and saveInFlight blocks that.
• Every GLOBAL_CONFIG_FIELDS id is in TRACKED_FIELDS (script check: none missing), so no global key is silently treated as unchanged because sentFields[id] is undefined.
  Prior nit (duplicate splice): FIXED. filter removes every copy.

Attacked / re-run:

• Probes from pass 1, re-run as temporary scratch tests and removed afterwards (cmp identical):
  A) loaded aws=7, server aws=3, edit azure=9, type aws=12 during the held GET. PUT {aws:3, azure:9, gcp:7}. The aws input keeps 12 and the next Save sends aws:12. Closed.
  B) loaded ['aws'], server ['aws','gcp'], check azure, check gcp during the GET. PUT ['aws','gcp','azure']. Closed.
  C, new) edit azure=9, then revert azure to 7 during the GET. PUT carries the click-time 9, the input stays 7, and the next Save sends azure:7. Consistent, nothing lost.
  D) server ['aws','aws','gcp'], uncheck aws. PUT ['gcp']. Closed.
• Other live-DOM reads after the await: showSavedGrace (live vs sentFields, which correctly detects in-save edits) and the final snapshot loop (same, correct). defaultCoverageChanged is read before any await. The service path is described in the nit above.
• Jest: npm ci, all settings* suites 10/10, 321/321 tests pass. eslint src/settings.ts clean, tsc --noEmit clean.
• Playwright: npm run build OK, then CI=1 under the shared lock, settings-save-changed-fields.spec.ts 2/2 pass. The served bundle app.b3ddc149.js is the scratch dist.
• Mutation probes (scratch, restored byte-identical by cmp):
  M1 predicate changed back to the live DOM (getFieldValue): 2 tests fail. KILLED. The new held-GET tests catch it.
  M2 filter changed back to the first-copy splice: 1 test fails. KILLED.
  M3 grace overlay guard forced true: 3 tests fail. KILLED.
• PR CI at a9c47de: 32 pass, 7 pending, 1 skipping (CodeRabbit) at review time. No failures.

[cristim]

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

Findings:

• Fixed since 3ad2c8d (was [blocking]): changed fields were judged from the live form after await api.getConfig(), so an edit made during the read overlaid a pre-await value and reverted the other admin's (grace: aws edited mid-read sent stale 7 over B's 3; providers: ticking gcp mid-read spliced out B's gcp). Now frontend/src/settings.ts:3649 defines isFieldChanged over sentFields (captured at 3618, before any await) vs savedSnapshot, used by both the key filter (3651) and the overlay (3488, 3495) via the new parameter. Re-traced both interleavings: aws is unchanged in sentFields, so the server's 3 is kept; gcp is unchanged in sentFields, so the server's gcp is kept. Held-GET tests prove it (below).
• frontend/src/settings.ts:3506-3513 - [non-blocking] write-back still covers the grace map only. After a rebase keeps another admin's newly enabled provider, its checkbox stays unchecked (reviewer test: gcp checkbox false after a save that sent [aws, gcp, azure]). The next Save is still correct (sends nothing; an untouched box keeps the server value), but the page misstates what is enabled until reload. The updated PR body documents this as intended: setting .checked from code fires no change event, so the dependent sections would not follow.
• Residual, [non-blocking]: re-read-to-PUT race remains; the backend has no ETag/version/If-Match (internal/api/handler_config.go:102-127 locks the read-modify-write but replaces grace_period_days and enabled_providers whole). A field both tabs changed resolves last-writer-wins for that field only (reviewer test: aws 5 from this tab wins, B's azure 2 survives and is shown). The updated PR body states both: no ETag or version check, so the GET-to-PUT race remains, and last-writer-wins for a field both tabs changed.
• frontend/src/settings.ts:3649 - [non-blocking, low] isFieldChanged reads savedSnapshot live, so a loadGlobalSettings re-snapshot during the pending read would shift the baseline. Needs a settings reload while Save is in flight; not reachable from the normal UI flow that I found.
• frontend/src/settings.ts:3469-3470 - [non-blocking, cosmetic] double blank line left where the old isFieldChanged was. eslint passes.

Attacked:

• Duplicates: they can only come from the server list (fresh.global.enabled_providers, validateProviders does not dedupe); the local list is built once per provider. Uncheck: filter removes every copy and keeps order. Check: at < 0 guard adds nothing when present (server [aws, aws, azure] stays as is). New providers append at the end.
• Provider removed server-side between load and save: tab ticks gcp, sends [aws, gcp] (azure stays removed); tab also unticks azure, sends [aws]. Both pass.
• Second Save after a successful save sends nothing (grace: existing test; providers: reviewer test). Whitespace grace refused with no GET and no PUT; 0 accepted and sent as 0.
• Abort ordering: rebase (3656) precedes the global PUT and every service PUT; failure lands in catch, finally resets saveInFlight and the button.
• Mutation probes (head copy, each restored and checked with cmp):
  1. live-form change detection (revert fix): 2 tests fail (both held-GET tests).
  2. overlay all grace fields: 3 fail.
  3. skip the re-read: 7 fail.
  4. drop the grace write-back: 1 fails.
  5. accept empty as 0: 1 fails.
  6. remove only the first duplicate (splice): 1 fails (duplicate test).
  7. PUT before the re-read outcome is known: 9 fail (incl. both abort tests).
  8. drop required on the inline aws validator: 1 fails.
• Fail-before: the new test file on 3ad2c8d's settings.ts fails exactly the 3 new tests (two held-GET, duplicate). On base 7e94bb2 it fails 9 of 28 (both rewritten empty-field tests and 7 of the 11 fix(config): Settings Save from a stale tab reverts the global default term, payment and coverage #539 tests); the 4 fix(config): Settings Save from a stale tab reverts the global default term, payment and coverage #539 tests that pass on base are non-regression guards (PUT-in-flight edit kept, no duplicate, no read when grace unchanged, duplicate removal which base satisfies trivially).

Verified locally:

• node_modules cloned from the identical lockfile; npx jest src/__tests__/settings src/__tests__/riexchange-automation-settings.test.ts plus my 7 extra tests: 365/365 pass (358 without my extras).
• npx tsc --noEmit -p .: exit 0. npx eslint src/settings.ts src/__tests__/settings-purchasing-grace.test.ts: exit 0.
• npx webpack --mode production then CI=1 npx playwright test tests-e2e/settings-save-changed-fields.spec.ts (Chromium, hermetic page.route mocks): 2/2 pass.
• git fetch; origin/main = 7e94bb2 (equals the merge-base); git merge-tree --write-tree origin/main a9c47dec: clean, exit 0.
• No migrations, no Go changes.

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

[cristim]

Local verification at a9c47deca44470d131f93a0101a86ff17dfb9b22 merged onto current main (b4994d2f; merge-tree aa1a9d8f; 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.