Skip to content

refactor(frontend/plans): drop redundant setPlanAccounts call on create path (#743 follow-up) #745

Description

@cristim

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:

  1. Backend POST /api/plans -> SetPlanAccounts inside createPlan.
  2. 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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions