Summary
Surfaced by CodeRabbit during review of #743 (nitpick — out of scope for that
PR). In frontend/src/plans.ts around the plan save flow (currently L644-661),
after api.createPlan succeeds the frontend also calls api.setPlanAccounts
unconditionally, even though the backend createPlan handler now inserts
plan_accounts rows atomically as part of the same request (per the
universal-plan fix landed in #743).
This results in two writes to plan_accounts for the create path:
- Backend
POST /api/plans -> SetPlanAccounts inside createPlan.
- Frontend follow-up
PUT /api/plans/:id/accounts -> SetPlanAccounts again.
The second call uses DELETE+INSERT (per SetPlanAccounts contract), so the
end state is the same and the user-visible behaviour is correct. The current
inline comment in plans.ts rationalises the redundant call as a defensive
"belt-and-suspenders" against the backend ever diverging from the atomic
contract — which is genuine, but with #743 now hard-enforcing the atomic
insert, that defense is over-engineered.
What to change
Track wasExistingPlan (or capture a preSavePlanId snapshot) and only invoke
api.setPlanAccounts on the update path. CR's suggested diff:
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 inserted plan_accounts atomically from
+ // target_accounts in the POST body (see internal/api/handler_plans.go
+ // createPlan). No follow-up account-write needed.
}
-
- if (savedPlanId) {
- await api.setPlanAccounts(savedPlanId, accountIds);
- }
Why not in #743
#743's scope was eliminating universal plans (back-end + DB + back-door close +
modal validation). Trimming the frontend's redundant call is a separate
optimisation that doesn't affect correctness. The current code is documented
("belt-and-suspenders") so it's intentional, not an oversight.
References
Summary
Surfaced by CodeRabbit during review of #743 (nitpick — out of scope for that
PR). In
frontend/src/plans.tsaround the plan save flow (currently L644-661),after
api.createPlansucceeds the frontend also callsapi.setPlanAccountsunconditionally, even though the backend
createPlanhandler now insertsplan_accountsrows atomically as part of the same request (per theuniversal-plan fix landed in #743).
This results in two writes to
plan_accountsfor the create path:POST /api/plans->SetPlanAccountsinsidecreatePlan.PUT /api/plans/:id/accounts->SetPlanAccountsagain.The second call uses DELETE+INSERT (per
SetPlanAccountscontract), so theend state is the same and the user-visible behaviour is correct. The current
inline comment in
plans.tsrationalises the redundant call as a defensive"belt-and-suspenders" against the backend ever diverging from the atomic
contract — which is genuine, but with #743 now hard-enforcing the atomic
insert, that defense is over-engineered.
What to change
Track
wasExistingPlan(or capture apreSavePlanIdsnapshot) and only invokeapi.setPlanAccountson the update path. CR's suggested diff: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 inserted plan_accounts atomically from + // target_accounts in the POST body (see internal/api/handler_plans.go + // createPlan). No follow-up account-write needed. } - - if (savedPlanId) { - await api.setPlanAccounts(savedPlanId, accountIds); - }Why not in #743
#743's scope was eliminating universal plans (back-end + DB + back-door close +
modal validation). Trimming the frontend's redundant call is a separate
optimisation that doesn't affect correctness. The current code is documented
("belt-and-suspenders") so it's intentional, not an oversight.
References
frontend/src/plans.ts:653-661).internal/api/handler_plans.gocreatePlan).