refactor(frontend/plans): drop redundant setPlanAccounts call (closes #745)

[cristim]

Summary

• Moves api.setPlanAccounts inside the if (planId) (update) branch, removing it from the create path
• On create, the backend already inserts plan_accounts atomically inside createPlan (landed in fix(plans): eliminate universal plans — require target_accounts on creation #743), so the follow-up PUT was a redundant double-write
• Drops the now-stale "belt-and-suspenders" comment and the unused savedPlanId variable

Test plan

• npx tsc --noEmit -- no type errors
• jest --no-coverage -- 65 suites, 2142 tests pass (0 failures)
• Existing test rejects submit and never calls createPlan when Target Accounts is empty still asserts setPlanAccounts not called
• Existing test forwards selected target_accounts on createPlan verifies create body still includes target_accounts

Summary by CodeRabbit

• Refactor
  • Optimized plan creation and update workflows to improve efficiency in how associated accounts are managed during these operations.

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 39742d71-8ce4-40bb-a7df-420ab8f356b2

📥 Commits

Reviewing files that changed from the base of the PR and between 88f573b and ba0267a.

📒 Files selected for processing (2)

• frontend/src/__tests__/plans.test.ts
• frontend/src/plans.ts

📝 Walkthrough

Walkthrough

The savePlan function in frontend/src/plans.ts now branches its plan persistence behavior: plan updates call api.updatePlan() followed by api.setPlanAccounts(), while plan creates call only api.createPlan() and skip the post-operation account-setting call, relying instead on account insertion during creation.

Changes

Plan Persistence Update/Create Branching

Layer / File(s)	Summary
Conditional plan persistence by operation type frontend/src/plans.ts	The savePlan function branches based on planId presence: updates execute updatePlan then explicitly setPlanAccounts, while creates execute only createPlan and remove the follow-up account re-write, trusting account insertion during creation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• LeanerCloud/CUDly#946: The frontend change to stop calling api.setPlanAccounts after api.createPlan (and instead rely on createPlan to insert accounts) is tightly coupled to the backend createPlan logic and error handling around account assignment.
• LeanerCloud/CUDly#711: Both PRs touch the plan accounts flow in frontend/src/plans.ts: #711 adjusts how target accounts are selected and computed, while this PR changes how those computed accountIds are persisted conditionally on operation type.

Suggested labels

impact/few

Poem

🐰 A plan takes shape, now split two ways,
Create flows fast through shorter days,
Updates pause to reset accounts with care,
Each path precise, persistence fair! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: removing a redundant setPlanAccounts call from the create path after the backend was enhanced to handle it atomically.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/745-wave17

---

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

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Adversarial verification: this PR is NOT safe to merge as-is. It removes the frontend api.setPlanAccounts(savedPlanId, accountIds) call on the plan-create path, on the premise that the backend inserts plan_accounts atomically (per #743). That backend behavior does NOT exist on the current base: CreatePurchasePlan (internal/config/store_postgres.go) only inserts into purchase_plans and never touches plan_accounts; PlanRequest (internal/api/types.go) has no target_accounts field, so the create body cannot carry accounts. The only plan_accounts INSERT in the codebase is SetPlanAccounts (PUT /plans/:id/accounts). Net effect: removing the FE call means a newly-created plan persists ZERO plan_accounts rows -> it becomes invisible under the account-filtered Plans query (the exact bug #973 addresses). The tests pass only because they assert mocked call counts, not end-to-end persistence (false-green). Recommend either (a) closing this PR, or (b) first landing the backend support (add target_accounts to PlanRequest + insert plan_accounts in the same tx as CreatePurchasePlan), verifying SELECT * FROM plan_accounts WHERE plan_id=<new> returns the selected accounts, then dropping the FE call. Leaving for a human decision.

[cristim]

@coderabbitai full review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff
gh pr view 861 --repo LeanerCloud/CUDly --json title,body,files,commits,state

echo "=== PR DIFF ==="
gh pr diff 861 --repo LeanerCloud/CUDly

Length of output: 3363

---

🏁 Script executed:

#!/bin/bash
# Check backend store for CreatePurchasePlan and plan_accounts handling
echo "=== CreatePurchasePlan in store_postgres.go ==="
rg -n "CreatePurchasePlan\|plan_accounts\|SetPlanAccounts\|target_accounts" --type go -A 5 -B 2

echo ""
echo "=== PlanRequest type ==="
rg -n "PlanRequest\|target_accounts\|TargetAccounts" --type go -A 20 -B 2

echo ""
echo "=== plans handler / createPlan endpoint ==="
rg -n "createPlan\|CreatePlan\|setPlanAccounts\|SetPlanAccounts" --type go -C 5

Length of output: 851

---

🏁 Script executed:

#!/bin/bash
# Check frontend plans.ts for the diff context
echo "=== Frontend plans.ts setPlanAccounts references ==="
rg -n "setPlanAccounts\|savedPlanId\|createPlan\|target_accounts" --type ts -C 5

echo ""
echo "=== Issue `#743` and `#745` references in code or comments ==="
rg -rn "`#743`\|`#745`\|`#973`" -C 2

Length of output: 661

---

🏁 Script executed:

#!/bin/bash
# Find all Go and TS files
echo "=== Go files ==="
fd -e go | head -50

echo ""
echo "=== TS files ==="
fd -e ts | head -30

Length of output: 3077

---

🏁 Script executed:

#!/bin/bash
# Try finding the handler and store files
echo "=== handler_plans.go ==="
fd "handler_plans" | xargs cat 2>/dev/null || echo "not found"

echo ""
echo "=== store_postgres.go ==="
fd "store_postgres" | xargs cat 2>/dev/null || echo "not found"

echo ""
echo "=== api/types.go ==="
fd "types.go" --full-path | head -5 | xargs cat 2>/dev/null || echo "not found"

Length of output: 270284

---

---

@cristim — I've done a full review of the PR diff and inspected the relevant backend code directly.

Full Review: PR #861 — Drop Redundant setPlanAccounts on Create

Adversarial claim reassessment

The adversarial analysis was incorrect. It correctly observes that store_postgres.go::CreatePurchasePlan only inserts into purchase_plans — but it missed that handler_plans.go::createPlan (the HTTP handler) does call h.config.SetPlanAccounts after CreatePurchasePlan, with a rollbackPlan closure:

if err := h.config.CreatePurchasePlan(ctx, plan); err != nil {
    return nil, err
}
// ...
rollbackPlan := func() {
    if delErr := h.config.DeletePurchasePlan(ctx, plan.ID); delErr != nil {
        logging.Warnf("createPlan rollback: ...")
    }
}
if err := h.validatePlanAccountProviders(ctx, plan.ID, req.TargetAccounts); err != nil {
    rollbackPlan()
    return nil, err
}
if err := h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts); err != nil {
    rollbackPlan()
    return nil, fmt.Errorf("accounts: %w", err)
}

PlanRequest also does have TargetAccounts []string (used as req.TargetAccounts), and validateTargetAccounts enforces a non-empty list. The PR's stated premise is correct.

---

Diff analysis

The change is correct. Key observations:

1. Create path (else branch): api.createPlan sends target_accounts in the POST body → backend inserts plan_accounts via SetPlanAccounts inside the handler (with rollback). The frontend follow-up call was a redundant double-write.

2. Update path (if (planId) branch): updatePlan handler does not call SetPlanAccounts, so the frontend api.setPlanAccounts call is still correctly required here. ✅

3. savedPlanId variable drop: This was only used to pass the newly-created plan's ID to setPlanAccounts. Since we no longer call setPlanAccounts on create, the variable has no purpose. ✅

4. Comment update: Old "belt-and-suspenders" comment is replaced with accurate per-branch explanations. ✅

---

One pre-existing caveat (not introduced by this PR)

CreatePurchasePlan and SetPlanAccounts are two separate Postgres transactions in the handler — not a single atomic tx. A hard crash between the two would leave an orphaned purchase_plans row. The rollback closure mitigates application-level errors, but not a Lambda cold-kill. This is a pre-existing backend design issue, not introduced here, and is out of scope for this frontend-only PR.

---

Verdict

✅ Safe to merge. The adversarial analysis was a stale-read artifact — the reviewer inspected only the store layer, not the handler layer. The PR correctly removes a redundant double-write on the create path.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/861

Timestamp: 2026-06-08T08:41:47.426Z
Learning: In LeanerCloud/CUDly, `store_postgres.go::CreatePurchasePlan` only inserts into the `purchase_plans` table. The `plan_accounts` insert happens in `handler_plans.go::createPlan` (the HTTP handler layer), which calls `h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts)` after `CreatePurchasePlan`, guarded by a `rollbackPlan` closure that calls `DeletePurchasePlan` on failure. `PlanRequest` includes a `TargetAccounts []string` field and `validateTargetAccounts` enforces a non-empty UUID list. The `updatePlan` handler does NOT call `SetPlanAccounts`, so the frontend must call `api.setPlanAccounts` explicitly on the update path but not on the create path (as of PR `#861`).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T04:21:35.802Z
Learning: In LeanerCloud/CUDly `frontend/src/recommendations.ts` (PR `#838`, commit d7c5764), `perRecPayments` in `FanOutBucket` must be seeded **sparsely** (override-only: only recs whose resolved payment differs from the bucket-level default). The execute path in `frontend/src/app.ts` resolves each rec's payment as `perRecPayments.get(rec.id) ?? b.payment`; if the map is fully populated the fallback is dead and changing the bucket-level dropdown has no effect on unedited rows. The per-rec change handler deletes the map entry when the user reverts to the bucket default (resuming tracking via the fallback); the bucket-level change handler re-syncs visible per-rec selects that have no override, skipping rows whose service does not support the new payment (mixed-SP buckets). Tests (i) and (j) cover the override-only contract and bucket-level propagation respectively; 322 tests pass, `tsc --noEmit` clean.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

Re-review verdict: VERIFIED-CORRECT (supersedes the earlier NOT-FIXED note)

The earlier NOT-FIXED verdict is stale. It predated the #742/#743 create-path account-persistence work that is now on feat/multicloud-web-frontend. Re-verified against the current base, this PR is safe and correct.

Deciding evidence: the FE create body DOES send target_accounts

frontend/src/plans.ts (savePlan) stamps the selected account IDs onto the request body before the create/update branch:

• plan.target_accounts = accountIds; — set for both paths.
• The create branch then calls api.createPlan(plan), and frontend/src/api/plans.ts createPlan does JSON.stringify(plan), so target_accounts is in the POST body.
• An empty account list is rejected up front with a toast (FE guard), so a created plan can never have zero accounts.

Typing is consistent: SavePlanData.target_accounts?: string[] (frontend/src/types.ts:159) and CreatePlanRequest.target_accounts: string[] (frontend/src/api/types.ts:175).

Backend now persists it atomically (current base)

• internal/api/types.go:713 — PlanRequest.TargetAccounts []string \json:"target_accounts,omitempty"``.
• internal/api/handler_plans.go createPlan: validateTargetAccounts(req.TargetAccounts) (rejects empty with 400) -> validatePlanAccountProviders(...) -> h.config.SetPlanAccounts(ctx, plan.ID, req.TargetAccounts), with rollbackPlan() deleting the partial plan if either validation or persistence fails.

So the create path inserts plan_accounts atomically from the POST body. The separate api.setPlanAccounts(savedPlanId, accountIds) call on the create path is genuinely redundant, and dropping it does not produce zero-account/invisible plans. The update path correctly retains the dedicated setPlanAccounts call (PUT /plans doesn't run the create-only insert).

Regression coverage

frontend/src/__tests__/plans.test.ts:1235 ("forwards selected target_accounts on createPlan (universal-plans fix)") asserts api.createPlan is called with a body objectContaining({ target_accounts: [...] }) — the exact wire contract this refactor relies on.

Mergeability

mergeStateStatus: CLEAN, state: OPEN. No code change required; this is a comment-only verdict correction.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.