Repository navigation
fix(planned-purchases): Edit button uses plan_id, not purchase id - #780
Conversation
The Edit button on a scheduled-purchase row surfaced "Failed to load plan details" because handlePlannedPurchaseAction passed the purchase's own id to editPlan, which then called GET /plans/<purchase-id> and got a 404. Fixed by adding a data-plan-id attribute to the Edit button and passing it as a third argument through the handler to editPlan, so the correct GET /plans/<plan-id> call is made. Closes #773.
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 13 minutes and 12 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR fixes a bug where editing a planned purchase failed with "Failed to load plan details." The fix updates the Edit Plan button to capture and pass the parent plan's ID through the action handler instead of using the purchase row's ID, then calls ChangesPlanned Purchase Edit Action
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/plans.ts (1)
225-225: ⚡ Quick winConsider validating
planIdis non-empty in the edit action.The default value
planId = ''could lead to a malformed API call if thedata-plan-idattribute is missing or empty. Although the current markup at line 201 always sets it, adding a guard in the edit case (lines 240-244) would make the code more defensive.🛡️ Proposed validation guard
case 'edit': + if (!planId) { + showToast({ message: 'Failed to load plan: missing plan ID', kind: 'error' }); + return; + } // Open edit modal for the parent plan using plan_id, not the purchase id. // The purchase row's data-plan-id attribute carries the plan FK (`#773`). await editPlan(planId); return;🤖 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` at line 225, The edit branch in handlePlannedPurchaseAction currently assumes planId (parameter planId) is present which can produce malformed API calls if data-plan-id is missing; add a defensive guard in the edit branch (where action === 'edit') that checks if planId is truthy and, if not, aborts early and surfaces an error (e.g., show a user toast/console.error and return) before performing any API call using purchaseId/planId so callers cannot send empty plan IDs.
🤖 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.
Nitpick comments:
In `@frontend/src/plans.ts`:
- Line 225: The edit branch in handlePlannedPurchaseAction currently assumes
planId (parameter planId) is present which can produce malformed API calls if
data-plan-id is missing; add a defensive guard in the edit branch (where action
=== 'edit') that checks if planId is truthy and, if not, aborts early and
surfaces an error (e.g., show a user toast/console.error and return) before
performing any API call using purchaseId/planId so callers cannot send empty
plan IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 947c95ba-0631-4dab-bf21-2f0e84732278
📒 Files selected for processing (2)
frontend/src/__tests__/plans.test.tsfrontend/src/plans.ts
Add an early-return guard in the edit case of handlePlannedPurchaseAction so that a missing or empty data-plan-id attribute produces a console.warn and a no-op instead of forwarding an empty string to editPlan / the API. Covers the defensive path with a focused unit test.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
QA Planned 6.2: clicking "Edit" next to a scheduled purchase immediately showed "Failed to load plan details".
Root cause
In
renderPlannedPurchaseRow, the Edit button was rendered withdata-id="${purchase.id}"-- the purchase's own primary key. The listener calledhandlePlannedPurchaseAction(action, btn.dataset['id']), which passed it toeditPlan(purchaseId).editPlanthen calledGET /plans/<id>with a purchase ID instead of a plan ID, getting a 404 every time.Fix
data-plan-id="${purchase.plan_id}"to the Edit button.btn.dataset['planId']as a third argument.handlePlannedPurchaseAction(action, purchaseId, planId)--editcase callseditPlan(planId).Files changed
frontend/src/plans.tsfrontend/src/__tests__/plans.test.tsTest plan
'edit action calls getPlan with plan_id, not the purchase id (#773)'.Note on design ambiguity
Edit currently scopes to the whole plan (not just this execution). The original QA note flagged this as worth deciding -- documented but not changed here. Will follow up separately.
Closes #773.
Summary by CodeRabbit
Bug Fixes
Tests