feat(api): undo backend — restore endpoints + sub-habit cascade (closes a soft-delete leak) (#11)#263
Conversation
…delete (#11) Soft-delete already exists (sync tombstones); this adds the missing Restore. POST /api/{habits,goals,tags}/{id}/restore (IgnoreQueryFilters + ownership + sub-habit cascade). DeleteHabitCommand now cascade-soft-deletes the sub-habit subtree (closes a leak where a deleted parent's children stayed visible in aggregates). HabitHierarchy helper. 8 EF-filter leak tests covering every log-reading query shape + cascade + preservation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
PR 263 Review: REQUEST CHANGES - 2 High findings, 1 Medium. HIGH-1: RestoreHabitCommand and RestoreTagCommand missing IConcurrencyRetryable. These commands do not implement IConcurrencyRetryable, so ConcurrencyRetryBehavior does not retry them on DbUpdateConcurrencyException - the exception surfaces as HTTP 500. RestoreGoalCommand, DeleteHabitCommand, and SkipHabitCommand all carry IConcurrencyRetryable. Fix: add IConcurrencyRetryable to RestoreHabitCommand (line 18) and RestoreTagCommand (line 14). HIGH-2: RestoreHabit and RestoreTag controllers hand-write if (result.IsSuccess) instead of ToPayGateAwareResult. Per CLAUDE.md, use result.ToPayGateAwareResult - never hand-write the 403/PAY_GATE response block. RestoreGoal at GoalsController:200 does it correctly. If a PayGate check is later added to either handler, the controller silently returns 400 instead of 403. Both actions also missing ProducesResponseType 403. Fix: replace both action bodies with result.ToPayGateAwareResult and add the 403 attribute, matching RestoreGoal. MEDIUM: RestoreHabitCommand.cs:23 calls FindTrackedIgnoringFiltersAsync with only UserId predicate, hydrating all habits including deleted for the user. Scope to also filter by HabitId. What is Good: timestamp-matching restore logic is correct and well-tested; HabitHierarchy.SelfAndDescendants is clean; auth correct on all endpoints; 8 SoftDeleteLeakTests are high quality; full test coverage. Cross-repo: not verifiable in CI.
…store - Add IConcurrencyRetryable to RestoreHabitCommand + RestoreTagCommand (match RestoreGoalCommand): a DbUpdateConcurrencyException during the restore save is now retried by ConcurrencyRetryBehavior instead of surfacing as HTTP 500. - RestoreHabit + RestoreTag controllers use result.ToPayGateAwareResult(...) + ProducesResponseType(403), matching RestoreGoal — no hand-written pay-gate block. - Scope RestoreHabitCommand hydration to deleted habits (&& h.IsDeleted): excludes the live set. Kept the deleted subtree rather than HabitId-only, since cascade restore needs the descendants. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Re: the MEDIUM (scope |
|
There was a problem hiding this comment.
Code Review: PR #263 - feat(api): undo backend - restore endpoints + sub-habit cascade soft-delete
Critical: 0 | High: 0 | Medium: 1 | Low/Info: 2
DECISION: APPROVE
SUMMARY
PR adds POST /api/{habits,goals,tags}/{id}/restore backed by CQRS triples, fixes a real soft-delete leak in DeleteHabitCommand (children left IsDeleted=false), and adds 8 EF-filter leak tests. Design is clean, authorization is correct at every layer.
MEDIUM FINDING
RestoreHabitCommand bypasses the free-plan active-habit quota.
File: src/Orbit.Application/Habits/Commands/RestoreHabitCommand.cs:22-37
The handler restores a soft-deleted habit subtree without calling payGate.CanCreateHabits(). A free-plan user can cycle delete -> restore to exceed the habit limit. RestoreGoalCommand correctly calls payGate.CanAccessGoals() first - that is the pattern to mirror.
Fix (3-line addition):
var gateCheck = await payGate.CanCreateHabits(request.UserId, 1, cancellationToken);
if (gateCheck.IsFailure)
return gateCheck;
LOW: Double SaveChangesAsync in RestoreHabitCommand (lines 39-43) - handler saves, then ConcurrencyRetry.SaveWithRetryAsync saves again. No correctness impact, matches DeleteHabitCommand pattern, likely intentional.
INFO: Cross-repo consumer wiring (endpoints.ts, apiClient calls) not verifiable in CI - sibling repo not checked out. No new DTO fields so no Zod changes needed.
SECURITY PASS: [Authorize] at class level on all three controllers. Ownership enforced via UserId in every DB predicate. No injection paths. 204 No Content responses expose nothing.
WHAT IS GOOD
- Cascade-restore timestamp-matching is elegant: one deletedAtUtc instant shared across the subtree on delete, filtered on restore to distinguish this action from prior independent deletes. Handle_DoesNotRestoreChildDeletedInSeparateAction pins the invariant.
- Soft-delete leak bugfix is solid: HabitHierarchy.SelfAndDescendants shared between delete and restore.
- Test coverage: 8 EF-filter leak tests against real EF filters, not mocks.
- RestoreGoalCommand sets the right pay-gate pattern; RestoreHabitCommand just needs to follow it.
- AgentCatalogService.Capabilities.cs correctly updated for all three restore actions.



Undo backend — restore + cascade soft-delete
Backend half of the undo feature (the client Undo snackbar follows in a paired ui-mobile PR).
Finding: the soft-delete foundation already exists on
main(offline-sync tombstones —IsDeleted/DeletedAtUtc, EF query filters, soft-deleting delete handlers, a 31-day purge inSyncCleanupService). So this does NOT rebuild it (would cause EF drift). It adds the genuinely-missing pieces:Restore
POST /api/{habits,goals,tags}/{id}/restore— command + validator + handler each. Finds viaIgnoreQueryFilters, enforces ownership (cross-user → NotFound), clearsDeletedAtUtc. Habit restore walks the sub-habit subtree and restores descendants whoseDeletedAtUtcmatches the parent's (precise undo — a separately-deleted sub-habit stays deleted).Cascade soft-delete (a bugfix)
DeleteHabitCommandnow cascade-soft-deletes the sub-habit subtree with one shared timestamp — closing a real leak where deleting a parent left its childrenIsDeleted=false, so their logs stayed visible in aggregates.Leak-hardening + tests
Audited every
HabitLog/GoalProgressLogread — all source ids from the filteredHabitsset (orIncludethe filtered nav), so soft-deleted habits' logs never enter. 8 real EF-filter leak tests (SoftDeleteLeakTests.cs) reproduce each query shape against an in-memory context and assert exclusion, plus cascade + log-preservation.Known edge (flagged, not a blocker)
Restoring a calendar-synced habit re-enters the
(UserId, GoogleEventId) WHERE NOT IsDeletedpartial-unique set; if that exact Google event was re-synced to another live habit meanwhile, restore surfaces a loud DB uniqueness error (not silent corruption). Pre-existing constraint.Validation
Build 0 errors; full suite 3834 pass; no model drift.
🤖 Generated with Claude Code