fix(plans): eliminate universal plans — require target_accounts on creation - #743
Merged
cristim merged 6 commits intoMay 27, 2026
Merged
Conversation
Eliminates the "universal plans" class — purchase_plans rows with no matching plan_accounts row — that the historical "leave Target Account blank to mean all-accounts-of-this-provider" UX created. Plans now must be tied to at least one cloud_account. Changes: - PlanRequest gains target_accounts ([]string), validated non-empty + per- entry UUID-formed at POST /api/plans. Rejected requests return 400 with a clear message; the store is never touched. - createPlan inserts the plan_accounts rows immediately after CreatePurchasePlan inside the same handler, rolling the plan row back if provider-match validation or the SetPlanAccounts write fails. This keeps the invariant "every purchase_plans row has at least one plan_accounts row" end-to-end so a fresh universal plan can no longer appear via a partial failure. - PUT /api/plans/:id/accounts now rejects an empty account_ids body. Closes the back-door for re-creating a universal plan by clearing the list on an existing plan. - OpenAPI schema marks target_accounts as required with minItems: 1. Tests: - TestHandler_createPlan_RejectsEmptyTargetAccounts (missing field + empty array): both return 400; CreatePurchasePlan never reached. - TestHandler_createPlan_RejectsInvalidTargetAccountUUID: malformed UUID rejected before any DB write. - TestSetPlanAccounts_RejectsEmpty: PUT-with-empty-list returns 400, SetPlanAccounts never reached. - Existing TestHandler_createPlan + TestHandler_HandleRequest_CreatePlan extended to include target_accounts + GetCloudAccount stub for the new provider-match path. - Two router dispatch tests that used empty account_ids switched to a single valid UUID — they were never asserting on body content. Existing universal plans in the DB are NOT touched by this PR — those require operator review (delete / fan-out / manual reassignment). See scripts/list_universal_plans.sql in a follow-up commit for the diagnostic.
Matches the new backend contract (POST /api/plans rejects empty target_accounts). Without this change the user could fill in every other plan field and only see the failure as a toast after the POST round-trip landed. Changes: - Modal help text + section heading mark Target Accounts as required (was "Leave empty to target all enabled accounts" — that semantics no longer exists). - savePlan rejects an empty plan-account-ids field with a clear toast before the createPlan/updatePlan call ever fires. - Selected account IDs are stamped onto the request as target_accounts so the backend can validate + persist atomically on POST. The follow-up PUT /api/plans/:id/accounts call is preserved as defence-in- depth for the update flow (which still uses the 2-step write). - New refreshPlanSaveButtonState() disables the Save Plan submit button whenever the chip list is empty and re-enables it as soon as the user adds one. The button title surfaces the reason to mouseover. - CreatePlanRequest.target_accounts added to the API types so the request shape is checked at compile time. Tests: - "rejects submit and never calls createPlan when Target Accounts is empty (universal-plans fix)": empty hidden field → no createPlan call, no setPlanAccounts call, toast surfaces the requirement. - "forwards selected target_accounts on createPlan (universal-plans fix)": multi-account chip list flows through to the request body. - DOM setup gained #plan-accounts-selected, #plan-account-ids, and a submit button so the new code paths have something to drive. - Existing tests in the savePlan describe block now stamp a single UUID in beforeEach; tests that re-open the modal (form.reset() clears the field) call a stampAccountIds() helper to re-stamp before submit.
Read-only diagnostic for existing universal plans (purchase_plans rows
with no matching plan_accounts row). The companion API change rejects
new ones, but pre-existing rows from the legacy "leave Target Account
blank" UX remain until an operator decides per-plan.
The file documents three cleanup options inline so the operator can pick
without leaving psql:
(a) DELETE the plan if genuinely orphaned (no consumer, no executions)
(b) Fan out: insert plan_accounts rows for every enabled cloud_account
whose provider matches the plan's services map
(c) Manual review per plan via the (now-required-Target-Account) UI
No destructive operations live in the script — the operator runs the
SELECT, decides per plan, and executes one of the three documented
follow-ups by hand. A separate ops-tracking issue captures the broader
cleanup plan + scheduling.
Contributor
|
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughEnforces non-empty Target Accounts: frontend modal marks and requires selection (Save disabled until chosen); frontend submits selected account IDs as ChangesUniversal Plans Target Accounts Requirement
Sequence DiagramsequenceDiagram
participant Browser as Browser
participant Frontend as Frontend.savePlan
participant API as API.createPlan
participant Store as Store.CreatePurchasePlan
participant Accounts as Store.SetPlanAccounts
Browser->>Frontend: submit plan (includes target_accounts)
Frontend->>API: POST /api/plans {plan + target_accounts}
API->>Store: CreatePurchasePlan(...)
Store-->>API: purchase_plan_id
API->>Accounts: SetPlanAccounts(plan_id, account_ids)
Accounts--xAPI: error (accounts insertion failure)
API->>Store: DeletePurchasePlan(plan_id) -- rollback
Store-->>API: rollback result
API-->>Frontend: error (accounts: <original error>)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
frontend/src/plans.ts (1)
653-661: ⚡ Quick winRedundant setPlanAccounts call on the create flow.
The backend
createPlanhandler already insertsplan_accountsatomically (per the stack context), so callingsetPlanAccountsagain from the frontend after create results in two writes to theplan_accountstable for the same plan. While safe (the comment notes setPlanAccounts does DELETE+INSERT), it's inefficient.Consider calling
setPlanAccountsonly on the update path:try { let savedPlanId = planId; if (planId) { await api.updatePlan(planId, plan as unknown as api.CreatePlanRequest); + // Update flow: push the selected account list via the dedicated endpoint. + await api.setPlanAccounts(planId, accountIds); } else { const created = await api.createPlan(plan as unknown as api.CreatePlanRequest) as unknown as { id: string }; savedPlanId = created.id; + // Create flow: backend already inserted plan_accounts atomically from target_accounts in POST body. } - // On update, the create path's atomic plan_accounts insert doesn't fire - // (we only POST /plans on create). For updates we still need to push the - // selected account list via the dedicated endpoint. On create, we also - // re-push here so that subsequent reselection is reflected even if the - // backend later opens an "atomic-create-only" path that diverges from - // PUT semantics — same call already handles dedupe via DELETE+INSERT. - if (savedPlanId) { - await api.setPlanAccounts(savedPlanId, accountIds); - } closePlanModal();🤖 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/plans.ts` around lines 653 - 661, The frontend is redundantly calling api.setPlanAccounts after creating a plan even though the backend createPlan already writes plan_accounts atomically; change the call site so api.setPlanAccounts(savedPlanId, accountIds) only runs on the update path, not immediately after a successful create. Concretely, track whether the save was an update vs create (e.g., capture a pre-save flag like wasExistingPlan or preSavePlanId) and invoke api.setPlanAccounts only when the plan existed before the save (update case); do not call api.setPlanAccounts in the branch that handles create responses from createPlan.
🤖 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 `@internal/api/handler_plans.go`:
- Around line 103-109: When rollback DeletePurchasePlan errors are ignored after
validatePlanAccountProviders or SetPlanAccounts fail, surface those failures
instead of discarding them: call h.config.DeletePurchasePlan(ctx, plan.ID) and
if it returns an error, wrap and return a combined error that includes both the
original operation error and the rollback error (e.g., "rollback delete failed:
%w") so callers see the orphaning risk; update the error handling around
validatePlanAccountProviders and SetPlanAccounts to capture the original err,
attempt DeletePurchasePlan, and return a wrapped error containing both the
original err and any DeletePurchasePlan error, referencing the functions
validatePlanAccountProviders, SetPlanAccounts, and DeletePurchasePlan.
In `@internal/api/openapi.yaml`:
- Line 1980: The OpenAPI schema made PlanRequest universally require
target_accounts (required: [name, target_accounts]), which incorrectly forces
target_accounts on update endpoints; modify the schema so that the base
PlanRequest only requires name, and add a distinct request schema used by the
create-plan operation (e.g., PlanCreateRequest or an inline request for POST
/api/plans) that includes required: [name, target_accounts]; update the POST
/api/plans operation to reference PlanCreateRequest (or its inline schema) while
leaving PUT/PATCH /api/plans/{id} to accept the base PlanRequest without
target_accounts as required.
In `@scripts/list_universal_plans.sql`:
- Line 3: Update the top-line comment in scripts/list_universal_plans.sql that
currently reads "PR `#739` follow-up" to reference the correct PR/issue "`#742`"
instead; locate the comment string "-- Read-only diagnostic for the \"universal
plans\" cleanup (PR `#739` follow-up):" and replace the PR number so the file
documents the correct tracking issue.
- Around line 24-34: The fan-out example must filter out disabled accounts and
ensure the provider extraction uses the same JSONB element names from
purchase_plans.services; update the INSERT ... SELECT that uses
jsonb_each(pp.services) AS svc(k, v) to include "AND ca.enabled = true" in the
JOIN/WHERE and make the provider comparison use the same symbol produced by
jsonb_each (e.g., join ON ca.provider = svc.v->>'provider' or replace svc.v with
whatever alias you used) so the join references purchase_plans.services
consistently; keep the target table plan_accounts and the rest of the INSERT ...
ON CONFLICT DO NOTHING unchanged.
---
Nitpick comments:
In `@frontend/src/plans.ts`:
- Around line 653-661: The frontend is redundantly calling api.setPlanAccounts
after creating a plan even though the backend createPlan already writes
plan_accounts atomically; change the call site so
api.setPlanAccounts(savedPlanId, accountIds) only runs on the update path, not
immediately after a successful create. Concretely, track whether the save was an
update vs create (e.g., capture a pre-save flag like wasExistingPlan or
preSavePlanId) and invoke api.setPlanAccounts only when the plan existed before
the save (update case); do not call api.setPlanAccounts in the branch that
handles create responses from createPlan.
🪄 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: ede37eb9-ffdf-4cf8-8687-7ccc88118e17
📒 Files selected for processing (16)
frontend/src/__tests__/api.test.tsfrontend/src/__tests__/plans.test.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/plans.tsfrontend/src/types.tsinternal/api/handler_accounts.gointernal/api/handler_accounts_router_test.gointernal/api/handler_accounts_test.gointernal/api/handler_plans.gointernal/api/handler_plans_test.gointernal/api/handler_test.gointernal/api/openapi.yamlinternal/api/router_handlers_test.gointernal/api/types.goscripts/list_universal_plans.sql
Comment on lines
+103
to
+109
| if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil { | ||
| _ = h.config.DeletePurchasePlan(ctx, plan.ID) | ||
| return nil, err | ||
| } | ||
| if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { | ||
| _ = h.config.DeletePurchasePlan(ctx, plan.ID) | ||
| return nil, fmt.Errorf("accounts: %w", err) |
Contributor
There was a problem hiding this comment.
Handle rollback delete failures explicitly.
Line 104 and Line 108 discard DeletePurchasePlan errors. If delete fails after CreatePurchasePlan succeeds, the handler can still leave an orphaned plan row and silently weaken the “no universal plans” invariant.
🔧 Suggested fix
+ rollbackPlan := func(cause error) error {
+ if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil {
+ return fmt.Errorf("%w; rollback failed deleting plan %s: %v", cause, plan.ID, delErr)
+ }
+ return cause
+ }
+
if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil {
- _ = h.config.DeletePurchasePlan(ctx, plan.ID)
- return nil, err
+ return nil, rollbackPlan(err)
}
if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil {
- _ = h.config.DeletePurchasePlan(ctx, plan.ID)
- return nil, fmt.Errorf("accounts: %w", err)
+ return nil, rollbackPlan(fmt.Errorf("accounts: %w", err))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil { | |
| _ = h.config.DeletePurchasePlan(ctx, plan.ID) | |
| return nil, err | |
| } | |
| if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { | |
| _ = h.config.DeletePurchasePlan(ctx, plan.ID) | |
| return nil, fmt.Errorf("accounts: %w", err) | |
| rollbackPlan := func(cause error) error { | |
| if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil { | |
| return fmt.Errorf("%w; rollback failed deleting plan %s: %v", cause, plan.ID, delErr) | |
| } | |
| return cause | |
| } | |
| if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil { | |
| return nil, rollbackPlan(err) | |
| } | |
| if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil { | |
| return nil, rollbackPlan(fmt.Errorf("accounts: %w", err)) | |
| } |
🤖 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 `@internal/api/handler_plans.go` around lines 103 - 109, When rollback
DeletePurchasePlan errors are ignored after validatePlanAccountProviders or
SetPlanAccounts fail, surface those failures instead of discarding them: call
h.config.DeletePurchasePlan(ctx, plan.ID) and if it returns an error, wrap and
return a combined error that includes both the original operation error and the
rollback error (e.g., "rollback delete failed: %w") so callers see the orphaning
risk; update the error handling around validatePlanAccountProviders and
SetPlanAccounts to capture the original err, attempt DeletePurchasePlan, and
return a wrapped error containing both the original err and any
DeletePurchasePlan error, referencing the functions
validatePlanAccountProviders, SetPlanAccounts, and DeletePurchasePlan.
…action When validatePlanAccountProviders or SetPlanAccounts fails after a successful CreatePurchasePlan, the handler rolls the plan row back to preserve the "every purchase_plans row has at least one plan_accounts row" invariant. The rollback DeletePurchasePlan error was previously discarded, so a transient DB failure could leave a partial plan row behind unnoticed — the exact universal-plan state this PR is trying to eliminate. Wrap the rollback delete in a closure that logs at WARN with the plan ID when it fails, so an operator can clean up manually. The caller still receives the original cause unchanged (no API contract change). Adds a regression test asserting that: - DeletePurchasePlan is called on the rollback path - The user-facing error is the original SetPlanAccounts error wrapped as "accounts: ..." (not the rollback error) - The rollback error message does not leak into the returned error CR #743 finding F1.
PlanRequest is reused across POST /api/plans, PUT /api/plans/{id}, and
PATCH /api/plans/{id}. Adding target_accounts to the shared schema's
required array forced the field on every update payload too, which
contradicts updatePlan's design (omitting target_accounts on PUT/PATCH
means "leave the existing account list unchanged"; the back-door is
closed at setPlanAccounts itself by rejecting empty arrays).
Move the requirement onto the POST operation only via an inline allOf
that composes PlanRequest with `required: [target_accounts]`. The
shared schema goes back to `required: [name]`, leaving update payloads
unaffected while still advertising the create contract correctly to
client codegen and OpenAPI validators.
CR #743 finding F2.
…lans.sql Two minor fixes per CR #743 review: - F3: header comment referenced PR #739 (the now-closed account-filter fix) but the operator-driven cleanup is tracked by issue #742, not #739. Update the reference so future readers find the right issue. - F4: the fan-out cleanup example (option b) attached every cloud account matching the plan's provider, including disabled accounts. Add `ca.enabled = true` to the JOIN so disabled accounts aren't swept into a re-attach pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 28, 2026
6 tasks
cristim
added a commit
that referenced
this pull request
Jun 5, 2026
Add migration 000057 that deletes purchase_plans rows with no plan_accounts entry (universal plans created before #743's API guard). Linked executions and history rows have their plan_id NULLed via the existing ON DELETE SET NULL FK constraints; the .down.sql is a no-op because deleted rows cannot be reconstructed from SQL alone. Includes an integration test covering 2 universal plans deleted, 1 scoped plan preserved, FK SET NULL on child rows, and idempotency.
This was referenced Jun 5, 2026
cristim
added a commit
that referenced
this pull request
Jun 6, 2026
…) (#994) * feat(plans): surface legacy no-account plans as Unassigned (closes #973) Plans created before target_accounts was required (#743) have zero rows in plan_accounts and are invisible in account-filtered views because the JOIN on plan_accounts excludes them. Backend: buildListPlansQuery now uses LEFT JOIN + OR NOT EXISTS so that zero-account plans are included alongside matched-account plans when an account filter is active. A computed boolean column "unassigned" (true for zero-account plans, false otherwise) is selected so callers can bucket the two groups without a second query. The no-filter case continues to return all plans and sets unassigned=false. PurchasePlan gains an Unassigned field that is omitted from JSON when false. Frontend: renderPlans splits plans into assigned and unassigned buckets. Assigned plans render as before. Unassigned plans are appended under a clearly labeled "Unassigned" section header (class unassigned-plans-header). Account-scoped actions (Add Purchases, Edit, enable toggle) are suppressed for unassigned plans; History and Delete remain available. Account-name resolution is skipped for unassigned plans because they have no plan_accounts rows. Tests: backend adds TestPGXMock_ListPurchasePlans_UnassignedIncluded (zero-account plan flagged true, assigned plan flagged false) and TestHandler_HandleRequest_ListPlans_UnassignedFlagged (API-level regression guard). Frontend adds two loadPlans tests: one asserting the Unassigned section appears with the correct order, another asserting it is absent when all plans are assigned. * test(plans): real DB test for Unassigned bucket query (refs #973) Add a testcontainers-backed integration test (TestPostgresStoreDB_ListPurchasePlans_UnassignedBucket) that exercises the LEFT JOIN + OR NOT EXISTS query introduced in #973 against a real Postgres instance. Seed layout: - planA assigned to accountX: must appear with Unassigned=false - planB with zero plan_accounts rows: must appear with Unassigned=true - planC assigned to accountY only: must NOT appear in accountX filter The discriminating assertion (planB present in accountX-filtered result) fails on the pre-fix INNER JOIN code and passes with the LEFT JOIN fix, so a regression back to INNER JOIN will be caught by CI. Also fix two pre-existing compile errors in store_postgres_test.go (package config_test): add the config. qualifier to PurchasePlanFilter and inline the unexported pf() helper that was inaccessible from the external test package.
cristim
added a commit
that referenced
this pull request
Jun 19, 2026
On the create path, backend already inserts plan_accounts atomically inside createPlan (landed in #743). Move setPlanAccounts inside the update branch so it only fires on PUT, eliminating the double write.
cristim
added a commit
that referenced
this pull request
Jul 10, 2026
On the create path, backend already inserts plan_accounts atomically inside createPlan (landed in #743). Move setPlanAccounts inside the update branch so it only fires on PUT, eliminating the double write.
cristim
added a commit
that referenced
this pull request
Jul 17, 2026
On the create path, backend already inserts plan_accounts atomically inside createPlan (landed in #743). Move setPlanAccounts inside the update branch so it only fires on PUT, eliminating the double write.
cristim
added a commit
that referenced
this pull request
Jul 19, 2026
…745) (#861) * refactor(frontend/plans): drop redundant setPlanAccounts call (#745) On the create path, backend already inserts plan_accounts atomically inside createPlan (landed in #743). Move setPlanAccounts inside the update branch so it only fires on PUT, eliminating the double write. * test(frontend/plans): assert setPlanAccounts not called on create path Add regression guards that will fail if PR #861 is reverted: the create path must not call setPlanAccounts (backend persists plan_accounts atomically from target_accounts in the POST body). Also assert the update path still calls setPlanAccounts, completing the behavioral contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eliminate "universal plans" — rows in
purchase_planswith no matching rowin
plan_accounts. Plans must now be tied to at least one cloud account atcreation time, enforced across all four layers:
POST /api/plansvalidatestarget_accountsisnon-empty + every entry is a valid UUID; rejected with HTTP 400 before
any DB write.
createPlaninserts theplan_accountsrowsimmediately after
CreatePurchasePlanand rolls the plan row back ifprovider-match validation or the
SetPlanAccountswrite fails. Keepsthe invariant "every
purchase_plansrow has at least oneplan_accountsrow" end-to-end.PUT /api/plans/:id/accountsnow also rejectsan empty
account_idsbody, so the universal-plan state can't bere-created by clearing the list on an existing plan.
text updated, Save button disabled when no account selected, and the
request body now stamps the selected IDs as
target_accountson thePOST so the backend can validate + persist atomically.
What's NOT in this PR
Existing universal plans in the DB are not touched. Those rows predate
the enforcement and require operator-driven cleanup per plan. Filed
follow-up #742 with three cleanup options (delete / fan-out to all-
provider-accounts / manual reassignment via the UI) and the SQL for each.
The diagnostic
scripts/list_universal_plans.sqlis in this PR as theread-only starting point.
PR #739 implications
PR #739 (still open) added a UNION branch to
buildListPlansQuerysouniversal plans appeared in the account-filtered list. Once this PR + the
#742 cleanup land:
Recommendation (defer to the user): close #739 as obsolete after this PR
diff to just the array-arg refactor (the
= ANY($1::uuid[])change isworth keeping standalone).
Verification
Test plan
scripts/list_universal_plans.sqlagainstthe live DB and decides per-plan via ops(plans): clean up existing universal plans (DB rows with no plan_accounts entry) #742.
environment — Save button stays disabled until a Target Account is
added; saving without one shows a clear error toast; the backend
rejects a curl with empty target_accounts.
Closes #742 follow-up scope (the cleanup itself).
Summary by CodeRabbit