Repository navigation
Conversation
cristim
added
triaged
Contributor
📝 WalkthroughWalkthroughThe PR fixes state inconsistency ( ChangesPlan State Consistency When Disabling Scheduled Purchases
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/__tests__/plans.test.ts (1)
804-821:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the refresh assertion to avoid a false positive.
This test already has prior
getPlanscalls from setup, sotoHaveBeenCalled()does not prove disable triggered a reload.🔧 Proposed fix
test('disable action deletes planned purchase with confirmation', async () => { (api.deletePlannedPurchase as jest.Mock).mockResolvedValue({}); (api.getPlans as jest.Mock).mockResolvedValue({ plans: [] }); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); window.confirm = jest.fn().mockReturnValue(true); const disableBtn = document.querySelector('[data-action="disable"]') as HTMLButtonElement; + (api.getPlans as jest.Mock).mockClear(); disableBtn?.click(); await new Promise(resolve => setTimeout(resolve, 50)); expect(window.confirm).toHaveBeenCalled(); expect(api.deletePlannedPurchase).toHaveBeenCalledWith('purchase-1'); // Issue `#774`: after disable the Plans page must refresh so the toggle // reflects the backend's new enabled=false. getPlans is the API call // that loadPlans() fires to repopulate the Plans list. - expect(api.getPlans).toHaveBeenCalled(); + expect(api.getPlans).toHaveBeenCalledTimes(1); });🤖 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/__tests__/plans.test.ts` around lines 804 - 821, The test's assert that api.getPlans was called is flaky because earlier setup already invoked getPlans; before simulating the disable action (the click on the element found via document.querySelector('[data-action="disable"]') and window.confirm), either clear the getPlans mock (api.getPlans.mockClear()) or record its current call count and then assert that api.getPlans was invoked one additional time (expect(api.getPlans).toHaveBeenCalledTimes(prevCount + 1)); use api.deletePlannedPurchase and api.getPlans mocks and the disableBtn click to verify the reload deterministically.
🤖 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_purchases.go`:
- Around line 274-280: The error handling after calling h.config.GetPurchasePlan
should preserve "not found" semantics: instead of wrapping every error as a
server error, detect the not-found case (e.g., using errors.Is(err, ErrNotFound)
or comparing to the specific sentinel your config layer returns, or to
sql.ErrNoRows) and return a client 404 via NewClientError(404,
fmt.Sprintf("disable plan: plan %s not found", cancelled.PlanID)); otherwise
keep the existing server-error wrap (fmt.Errorf("disable plan: failed to fetch
plan %s: %w", cancelled.PlanID, err)). Also keep the existing nil-plan check
that returns NewClientError(404, ...). This change should be applied around the
GetPurchasePlan call in the handler where plan, err :=
h.config.GetPurchasePlan(ctx, cancelled.PlanID) is invoked.
- Around line 264-285: The cancel flow can leave plan.enabled=true if
TransitionExecutionStatus succeeds but the subsequent
GetPurchasePlan/UpdatePurchasePlan steps fail and a retry hits
TransitionExecutionStatus again returning 409; to fix, make the disable path
idempotent by continuing to the plan-disable steps when
TransitionExecutionStatus reports a conflict: call h.config.GetExecution(ctx,
executionID) (or otherwise fetch the execution) when TransitionExecutionStatus
returns a 409/conflict to obtain the execution's PlanID, treat that as the
cancelled result (handle nil safely), then run the existing
GetPurchasePlan(planID) and UpdatePurchasePlan(plan) logic (only flip
plan.Enabled to false if true) so retries will still ensure the plan is disabled
even if the transition itself reports already-done; keep returning original
errors for non-conflict failures.
---
Outside diff comments:
In `@frontend/src/__tests__/plans.test.ts`:
- Around line 804-821: The test's assert that api.getPlans was called is flaky
because earlier setup already invoked getPlans; before simulating the disable
action (the click on the element found via
document.querySelector('[data-action="disable"]') and window.confirm), either
clear the getPlans mock (api.getPlans.mockClear()) or record its current call
count and then assert that api.getPlans was invoked one additional time
(expect(api.getPlans).toHaveBeenCalledTimes(prevCount + 1)); use
api.deletePlannedPurchase and api.getPlans mocks and the disableBtn click to
verify the reload deterministically.
🪄 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: 81de6670-4374-4007-8018-3b4869fceefb
📒 Files selected for processing (3)
frontend/src/__tests__/plans.test.tsinternal/api/handler_purchases.gointernal/api/handler_purchases_test.go
Comment on lines
+264
to
+285
| cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled") | ||
| if err != nil { | ||
| return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err)) | ||
| } | ||
|
|
||
| // Set the parent plan's enabled flag to false so the Plans page toggle | ||
| // reflects the disable action immediately. Issue #774: previously the | ||
| // execution was cancelled but plan.enabled was left true, causing | ||
| // inconsistent state between the Scheduled Purchases and Plans views. | ||
| if cancelled.PlanID != "" { | ||
| plan, err := h.config.GetPurchasePlan(ctx, cancelled.PlanID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("disable plan: failed to fetch plan %s: %w", cancelled.PlanID, err) | ||
| } | ||
| if plan == nil { | ||
| return nil, NewClientError(404, fmt.Sprintf("disable plan: plan %s not found", cancelled.PlanID)) | ||
| } | ||
| if plan.Enabled { | ||
| plan.Enabled = false | ||
| if err := h.config.UpdatePurchasePlan(ctx, plan); err != nil { | ||
| return nil, fmt.Errorf("disable plan: failed to update plan %s: %w", cancelled.PlanID, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Make disable flow retry-safe after partial success.
If execution cancellation succeeds but plan fetch/update fails, a retry will return 409 at the transition step and never re-run the disable path, leaving plan.enabled=true indefinitely.
🔧 Proposed fix (idempotent retry path)
cancelled, err := h.config.TransitionExecutionStatus(ctx, executionID, []string{"pending", "paused"}, "cancelled")
if err != nil {
- return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, err))
+ // Retry-safe: if a prior attempt already cancelled the execution,
+ // continue with the plan-disable step instead of hard-failing.
+ existing, getErr := h.config.GetExecutionByID(ctx, executionID)
+ if getErr == nil && existing != nil && existing.Status == "cancelled" {
+ cancelled = existing
+ } else {
+ return nil, NewClientError(409, fmt.Sprintf("execution %s cannot be cancelled: %v", executionID, 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_purchases.go` around lines 264 - 285, The cancel flow
can leave plan.enabled=true if TransitionExecutionStatus succeeds but the
subsequent GetPurchasePlan/UpdatePurchasePlan steps fail and a retry hits
TransitionExecutionStatus again returning 409; to fix, make the disable path
idempotent by continuing to the plan-disable steps when
TransitionExecutionStatus reports a conflict: call h.config.GetExecution(ctx,
executionID) (or otherwise fetch the execution) when TransitionExecutionStatus
returns a 409/conflict to obtain the execution's PlanID, treat that as the
cancelled result (handle nil safely), then run the existing
GetPurchasePlan(planID) and UpdatePurchasePlan(plan) logic (only flip
plan.Enabled to false if true) so retries will still ensure the plan is disabled
even if the transition itself reports already-done; keep returning original
errors for non-conflict failures.
Comment on lines
+274
to
+280
| plan, err := h.config.GetPurchasePlan(ctx, cancelled.PlanID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("disable plan: failed to fetch plan %s: %w", cancelled.PlanID, err) | ||
| } | ||
| if plan == nil { | ||
| return nil, NewClientError(404, fmt.Sprintf("disable plan: plan %s not found", cancelled.PlanID)) | ||
| } |
Contributor
There was a problem hiding this comment.
Return 404 when parent plan lookup is not found.
GetPurchasePlan not-found errors are currently wrapped and surfaced as generic server errors. This path should map to a client 404 consistently.
🔧 Proposed fix (preserve not-found semantics)
plan, err := h.config.GetPurchasePlan(ctx, cancelled.PlanID)
if err != nil {
- return nil, fmt.Errorf("disable plan: failed to fetch plan %s: %w", cancelled.PlanID, err)
+ if IsNotFoundError(err) {
+ return nil, NewClientError(404, fmt.Sprintf("disable plan: plan %s not found", cancelled.PlanID))
+ }
+ return nil, fmt.Errorf("disable plan: failed to fetch plan %s: %w", cancelled.PlanID, err)
}
if plan == nil {
return nil, NewClientError(404, fmt.Sprintf("disable plan: plan %s not found", cancelled.PlanID))
}🤖 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_purchases.go` around lines 274 - 280, The error handling
after calling h.config.GetPurchasePlan should preserve "not found" semantics:
instead of wrapping every error as a server error, detect the not-found case
(e.g., using errors.Is(err, ErrNotFound) or comparing to the specific sentinel
your config layer returns, or to sql.ErrNoRows) and return a client 404 via
NewClientError(404, fmt.Sprintf("disable plan: plan %s not found",
cancelled.PlanID)); otherwise keep the existing server-error wrap
(fmt.Errorf("disable plan: failed to fetch plan %s: %w", cancelled.PlanID,
err)). Also keep the existing nil-plan check that returns NewClientError(404,
...). This change should be applied around the GetPurchasePlan call in the
handler where plan, err := h.config.GetPurchasePlan(ctx, cancelled.PlanID) is
invoked.
cristim
added a commit
that referenced
this pull request
May 28, 2026
…an + deterministic test On a retry where TransitionExecutionStatus returns ErrExecutionNotInExpectedStatus (the cancel already landed in a prior attempt), the handler now calls GetExecutionByID to recover the PlanID and proceeds to disable the plan. This makes the disable path idempotent: plan.enabled is always set to false regardless of how many times the endpoint is called. Extract the plan-disable logic into disablePlan so it can be called from both the normal path and the conflict-retry path. Replace the dead plan==nil guard with errors.Is(err, config.ErrNotFound) so not-found maps to 404 rather than 500. Add two backend regression tests covering the conflict-retry path (enabled plan gets disabled, already-disabled plan is a no-op). Fix the frontend assertion to call (api.getPlans as jest.Mock).mockClear() before the disable click so the subsequent toHaveBeenCalledTimes(1) assertion only counts the reload triggered by the disable action itself, not prior setup calls. Addresses CR findings on PR #781 (1 major + 2 potential issues).
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
force-pushed
the
fix/774-disable-plan-atomic
branch
from
May 28, 2026 12:54
a3f96b5 to
3f56912
Compare
This branch was previously deployed
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
QA Planned 6.4: clicking "Disable plan" next to a scheduled purchase removed the scheduled execution but left the plan's toggle ON on the Plans page. State was inconsistent.
Root cause
deletePlannedPurchase(internal/api/handler_purchases.go) calledTransitionExecutionStatusto cancel the scheduled execution but never read the returned execution'sPlanIDand never setplan.Enabled = false. The frontend already calledloadPlans()after disable, so the stale toggle was purely a backend omission.Fix
TransitionExecutionStatus, reads itsPlanID, fetches the parent plan viaGetPurchasePlan, and callsUpdatePurchasePlanwithEnabled = false.loadPlans()fires after disable (toggle reflects new state).Files changed
internal/api/handler_purchases.gointernal/api/handler_purchases_test.gofrontend/src/__tests__/plans.test.tsTest plan
TestHandler_deletePlannedPurchase_DisablesPlanassertsUpdatePurchasePlancalled withEnabled=falseand post-call plan readsEnabled=false.TestHandler_deletePlannedPurchase_AlreadyDisabledPlanassertsUpdatePurchasePlanis NOT called when plan is already disabled (idempotent path).t.Cleanup(mockStore.AssertExpectations)perfeedback_mock_assert_expectationsmemory.Design notes (commented on #774 separately)
enabled=false-- label inconsistency to consider.Closes #774.
Summary by CodeRabbit
Bug Fixes
Tests