[Claude] add subscription tracking #32
Conversation
- Backend: Added UpdateRecurringAsync and DeleteRecurringAsync to DbService
- Backend: Added AddRecurring, UpdateRecurring, DeleteRecurring to UserDataService
- Backend: Added POST/PUT/DELETE /api/Budget/{id}/recurring endpoints to BudgetController
- Frontend: Implemented updateRecurring and deleteRecurring in RemoteDataService
- Frontend: Created SubscriptionModal component for add/edit subscriptions
- Frontend: Created SubscriptionsSection component showing recurring expenses list
- Frontend: Integrated SubscriptionsSection into the Overview page
Users can now track subscriptions (rent, Netflix, internet, etc.) with frequency
options (weekly, bi-weekly, monthly, yearly) and see an estimated monthly total.
https://claude.ai/code/session_015haqKe5cYMpxCRX8XeDGKv
ParadoxZero
left a comment
There was a problem hiding this comment.
Overall: Request Changes
This PR adds a well-structured subscription tracking feature end-to-end (backend + frontend). The architecture is clean, the demo/local mode is properly implemented, and the UX is thoughtful. However, there are several issues that should be addressed before merging:
Major Issues (blockers):
[FromBody]attribute missing onAddRecurring(line 111) andUpdateRecurring(line 117) inbudget_controller.cs— may cause null binding in some ASP.NET Core configurations.-
throw new Exception("Recurring item not found")indb_service.csline 236 returns a 500 instead of a 404 for a valid "not found" case.
-
RemoveAllinDeleteRecurringAsyncsilently succeeds even for non-existent IDs, always returning 200 OK — this masks frontend bugs.
Minor suggestions (non-blocking):
AddRecurring/UpdateRecurring/DeleteRecurringinuser_data_service.csshould use theAsyncsuffix for naming consistency.-
frequencey_unittypo — good time to fix this across the datamodel since the feature is new.
-
defaultbranch intotalMonthlyincorrectly adds full amount for quarterly/halfYearly frequencies.
-
formmissing fromuseEffectdependency array insubscription_modal.tsx.
Positive notes:
- Clean three-layer separation maintained throughout.
-
LastUpdatedtimestamps correctly set in both add and update paths.
-
-
- Local (demo mode) implementation included for all new operations.
-
-
-
-
- Loading state properly threaded through the modal.
-
-
| { | ||
| Budget budget = await GetBudgetAsync(budget_id); | ||
| int index = budget.recurringList.FindIndex(r => r.Id == recurring.Id); | ||
| if (index == -1) throw new Exception("Recurring item not found"); |
There was a problem hiding this comment.
[Major Issue] throw new Exception returns 500 instead of 404
Throwing a generic Exception when a recurring item isn't found will bubble up as a 500 Internal Server Error to the client. This should be a 404. Consider one of two approaches:
- Return
nullhere and check for it in the controller, returningNotFound(): -
- if (index == -1) return null;
-
- Or throw a more specific exception type (e.g.,
KeyNotFoundException) and add global exception-handling middleware that maps it to a 404 response.
The current behavior gives callers a confusing 500 error for a perfectly valid "not found" scenario.
| public async Task<Budget> DeleteRecurringAsync(string budget_id, int recurring_id) | ||
| { | ||
| Budget budget = await GetBudgetAsync(budget_id); | ||
| budget.recurringList.RemoveAll(r => r.Id == recurring_id); |
There was a problem hiding this comment.
[Major Issue] Silent no-op on delete — always returns 200 OK
RemoveAll returns the number of items removed but the return value is discarded. If the caller sends a non-existent recurring_id, the method silently succeeds, removes nothing, and still returns 200 OK with the unchanged budget. This masks bugs in the frontend.
Suggest checking the removed count and returning a 404 if nothing was removed:
int removed = budget.recurringList.RemoveAll(r => r.Id == recurring_id);
if (removed == 0) return null; // caller maps to NotFound()| return await _dbService.DeleteExpenseAsync(budget_id, category_id, expense_id); | ||
| } | ||
|
|
||
| public async Task<Budget> AddRecurring(string budget_id, Recurring recurring) |
There was a problem hiding this comment.
[Minor] Missing Async suffix — inconsistent naming convention
The new methods AddRecurring, UpdateRecurring, and DeleteRecurring are async but omit the Async suffix. Every other async method in this file (and db_service.cs) uses the Async suffix convention (e.g., DeleteExpenseAsync, RolloverBudget is the one exception).
For consistency with the rest of the codebase, these should be renamed to AddRecurringAsync, UpdateRecurringAsync, and DeleteRecurringAsync.
| } | ||
|
|
||
| [HttpPost("{budget_id}/recurring")] | ||
| public async Task<IActionResult> AddRecurring(string budget_id, Recurring recurring) |
There was a problem hiding this comment.
[Major Issue] Missing [FromBody] attribute on POST/PUT endpoints
The recurring parameter is a complex object sent as JSON in the request body, but there is no [FromBody] attribute. Without it, ASP.NET Core may fail to bind the parameter from the request body in some configurations, resulting in a null recurring object silently passed to the service.
Same issue applies to line 117 (UpdateRecurring).
Fix:
public async Task<IActionResult> AddRecurring(string budget_id, [FromBody] Recurring recurring)Also consider adding [FromRoute] on budget_id for explicitness, matching the intent.
| description: initialValues.description, | ||
| amount: initialValues.amount, | ||
| frequency: initialValues.frequency, | ||
| frequencyUnit: initialValues.frequencey_unit, |
There was a problem hiding this comment.
[Minor] frequencey_unit — pre-existing typo, good time to fix
The field name frequencey_unit (misspelling of "frequency") is a pre-existing issue in the datamodel, but this PR is the first time it's used extensively in new UI code. Since the feature is brand new and no existing data depends on this field name yet, this is a low-cost opportunity to fix the spelling to frequency_unit across the datamodel, backend model, and all frontend references.
If left as-is, the typo will propagate further with each PR that touches this feature.
| setFrequency(RecurringType.monthly); | ||
| } | ||
| } | ||
| }, [isOpen, initialValues]); |
There was a problem hiding this comment.
[Minor] form missing from useEffect dependency array
form (from Form.useForm()) is used inside this effect but is not listed as a dependency. While form is a stable ref in Ant Design so this won't cause a runtime bug, it will trigger the react-hooks/exhaustive-deps ESLint rule if that's enabled.
Consider adding form to the dependency array:
}, [isOpen, initialValues, form]);| case RecurringType.yearly: | ||
| return sum + r.amount / 12; | ||
| default: | ||
| return sum + r.amount; |
There was a problem hiding this comment.
[Minor] default branch incorrectly counts quarterly/halfYearly as full monthly amounts
The default case here catches RecurringType.quarterly and RecurringType.halfYearly and adds the full amount as if it were a monthly expense. The correct multipliers are:
- Quarterly →
r.amount / 3 -
- Half-yearly →
r.amount / 6
While these frequency types are currently unsupported in the UI (not shown infrequencyOptions), they exist in the data model and can appear in existing data. If nothing else, add a comment explaining whydefaultis intentionally incorrect:
- Half-yearly →
// quarterly and halfYearly are unsupported; excluded from monthly estimate
default: return sum;
No description provided.