feat: undo/restore for destructive actions — habits, goals, tags (#11) - #330
Conversation
Deleting a habit/goal/tag shows an Undo snackbar; Undo restores via the orbit-api #263 restore endpoints (deploy #263 first). Fires from each delete hook's onSuccess (one seam, no call-site changes); reuses the existing showQueued toast. Web adds Ctrl/Cmd+Z (listener bound while visible, cleaned up, double-fire guarded). Mobile restore rides the offline queue. Shared: restore endpoint builders + additive mutationTypeSchema values. i18n both locales. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: PR #330
Scope: PR #330 — feat: undo/restore for destructive actions — habits, goals, tags
Recommendation: APPROVE
Summary
This PR adds undo/restore functionality for delete operations across habits, goals, and tags on both web and mobile. The implementation is clean — it reuses the existing showQueued toast primitive, the offline queue on mobile, and Server Actions on web. The web-specific Ctrl/Cmd+Z binding is properly scoped (bound on show, removed on settle/dismiss) with a double-fire guard. Both platforms are correctly covered. No Critical or High findings.
Findings
Critical
None.
High
None.
Medium
M1 — inferScope not exhaustive over new mutation types
File: apps/mobile/lib/offline-mutations.ts line 335
inferScope has a switch over MutationType with default: return 'habits'. The three new values 'restoreGoal', 'restoreHabit', 'restoreTag' aren't handled. In practice this is latent dead-code debt today — all three new hooks explicitly pass scope, so inferScope is never called for them. But a future refactor that omits scope would silently route goal/tag restore mutations to the habits scope, invalidating wrong query keys.
Fix: Add three cases to inferScope — 'restoreHabit' → 'habits', 'restoreGoal' → 'goals', 'restoreTag' → 'tags'.
M2 — useRestoreGoal (web) only invalidates goalKeys.lists(), but useRestoreHabit invalidates habits too
File: apps/web/hooks/use-goals.ts line 72
useDeleteGoal.onSettled (and the equivalent invalidateHabitDeleteQueries helper) invalidates habitKeys.lists() as well, since goals link habits. The restore path is narrower — only goalKeys.lists(). This leaves habit-list views stale after a goal restore (bounded by next focus-refetch; no wrong state visible immediately). The mobile useRestoreGoal routes through invalidateGoalQueries which includes habitKeys.lists() via the scope key map, so this is a parity gap.
Fix: Add queryClient.invalidateQueries({ queryKey: habitKeys.lists() }) inside useRestoreGoal.onSuccess in apps/web/hooks/use-goals.ts.
Low / Info
None above the signal gate.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED (Ctrl+Z web-only is a documented allowed platform difference; invalidation paths are functionally equivalent; showQueued API differs but both achieve the goal) |
| i18n-syncer | IN SYNC (all 6 undo.* keys present and translated in both locales) |
| contract-aligner | MATCH (endpoint paths correct; mutationTypeSchema additions are additive/backward-compatible; orbit-api #263 not verifiable in CI — must be reviewed separately) |
| security-reviewer | N/A (no orbit-api changes in this PR) |
Validation
| Check | Result |
|---|---|
| Lint | N/A (toolchain not installed in CI runner; author reports 0 errors) |
| Type check | N/A (toolchain not installed in CI runner; author reports clean 3/3) |
| Tests | N/A (toolchain not installed in CI runner; author reports 1031+1673+645 passed) |
| Build (api) | N/A (no backend changes; backend is orbit-api #263) |
Deferred — N/A dimensions
- Security (#12): No auth surfaces or trust boundaries changed. N/A.
- DESIGN.md / AI-slop (#8): No UI components changed. N/A.
- Backend hard rules (#13): No orbit-api code in this diff. N/A.
- Backward-compat guard: No fields removed or renamed from any Zod schema.
mutationTypeSchemaadditions are additive — old queued mutations parse and replay correctly. No old-client break risk.
What's good
- Single-seam design (
onSuccesson each delete hook) — zero call-site changes needed across all surfaces. - Web Ctrl/Cmd+Z implementation is correct:
state.settledprevents double-fire,cleanupis idempotent, listener removed on all exit paths. - Mobile restore correctly routes through the offline queue so delete→restore replays in insertion order on reconnect.
mutationTypeSchemaadditions are backward-compatible.- Test coverage is thorough — happy path, undo callback, error surface, offline path — for all six new hooks on both platforms.
invalidateHabitDeleteQuerieshelper on web is a clean DRY extraction.
Recommendation
APPROVE. Two Medium findings worth fixing in a follow-up (neither is a correctness bug today). Core feature is well-designed and production-ready pending orbit-api #263.
🤖 Generated with Claude Code
There was a problem hiding this comment.
PR #330 review — APPROVE. No Critical/High findings. Two Medium items (non-blocking): (M1) inferScope in apps/mobile/lib/offline-mutations.ts ~line 335 is not exhaustive over the three new restore mutation types — they fall through to default: return 'habits'; since all hooks pass scope explicitly this is latent dead-code debt, not a bug. (M2) useRestoreGoal on web (apps/web/hooks/use-goals.ts ~line 72) only invalidates goalKeys.lists() on success, but useDeleteGoal.onSettled also invalidates habitKeys.lists() since goals link habits — mobile restore correctly handles this; web should add the same invalidation. Parity: PAIRED. i18n: IN SYNC. Contract: client-side MATCH; orbit-api PR not verifiable in CI. Security: N/A.
# Conflicts: # apps/mobile/hooks/use-habits.ts
|
There was a problem hiding this comment.
Code Review: PR #330
Scope: PR #330 — feat: undo/restore for destructive actions — habits, goals, tags (#11)
Recommendation: APPROVE
Summary
PR #330 implements an undo/restore snackbar for delete actions on habits, goals, and tags across both web and mobile. The feature is architecturally clean: a single useUndoToast hook wires the undo toast from each delete hook's onSuccess, giving all call sites undo for free with no call-site changes. The web Ctrl/Cmd+Z shortcut is correctly guarded with a settled-flag double-fire defense. Mobile queues restore after delete in FIFO order, which is safe. Parity is complete, i18n is fully in sync, and the shared contract changes are additive.
Findings
Critical
None
High
None
Medium
[MEDIUM] useRestoreGoal on web invalidates queries in onSuccess, not onSettled
· dimension: Correctness (#1)
· location: apps/web/hooks/use-goals.ts:71-74
· issue: queryClient.invalidateQueries({ queryKey: goalKeys.lists() }) is called in onSuccess only. The equivalent hooks (useRestoreHabit, useRestoreTag) place invalidation in onSettled (fires regardless of success/error). If the restore call errors, the goal list cache is not refreshed, leaving the UI showing the deleted item as still gone with no correction.
· risk: A network error or 404 (e.g. if orbit-api #263 is not yet deployed) leaves the goals list stale — no background refetch to confirm the item's state.
· fix: Move queryClient.invalidateQueries({ queryKey: goalKeys.lists() }) into onSettled, mirroring useRestoreHabit and useRestoreTag.
· reference: CLAUDE.md rule 8 (error handling at boundaries; surface errors, don't silently suppress recovery)
[MEDIUM] useDeleteHabit on web has no optimistic remove — mobile removes immediately, web waits for onSettled invalidation
· dimension: Parity (#9)
· location: apps/web/hooks/use-habits.ts:302-319
· issue: Web useDeleteHabit has no onMutate / onError rollback. Mobile's equivalent (lines 422–436) immediately removes the habit from list cache in onMutate and rolls back on error. After delete, mobile removes the item instantly; web flashes the item until onSettled invalidation completes.
· risk: Observable behavioral parity break — same action produces different UX timing on the two platforms. Not a data-integrity issue; can be a follow-up.
· fix: Add onMutate (snapshot + optimistic remove via snapshotHabitLists / updateHabitLists(items => optimisticRemoveHabits(items, [habitId]))) and onError rollback to web useDeleteHabit, mirroring the mobile implementation.
· reference: CLAUDE.md "Cross-platform parity (MANDATORY)" — same logic, data flow, error handling
Low / Info
None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED |
| i18n-syncer | IN SYNC |
| contract-aligner | N/A — orbit-api not changed in this PR |
| security-reviewer | N/A — no orbit-api code changed |
Parity detail: Platform differences are correctly intentional — web adds dismissToast and Ctrl/Cmd+Z listener (no keyboard on mobile), mobile skips both. Server Actions (web) vs direct apiClient (mobile) for restore calls is the standard adapter split. The Medium #2 above is the only behavioral parity gap.
i18n detail: All 6 keys in the undo namespace (action, habitDeleted, goalDeleted, tagDeleted, restored, restoreFailed) are present in both en.json and pt-BR.json with correct structure and translations.
Validation
| Check | Result |
|---|---|
| Lint | PASS (per PR body: 0 errors) |
| Type check | PASS (per PR body: 3/3 workspaces clean) |
| Tests | PASS (per PR body: shared 1031, web 1673, mobile 645) |
| Build (api) | N/A — no orbit-api changes |
Backward-compat guard
packages/shared/src/types/sync.ts: The 3 additions / 3 deletions are a reorganization that adds 'restoreHabit', 'restoreGoal', 'restoreTag' as new enum values co-located with their delete counterparts. Pure addition to a client-local enum — old mobile clients never emit these values, they are not in any request/response shape. ⚠️ breaks old mobile clients: Not applicable.
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A — no
apps/*UI component files changed. - Backend hard rules (#13): N/A — no
orbit-apicode in this PR. - Contract drift (#11): N/A for automated comparison — orbit-api #263 is the paired PR. Endpoint paths follow the established
/{resource}/{id}/restorePOST pattern; confirm routes match before merging. - No-workaround (#5): N/A — immediate delete + undo-as-restore is the correct design.
- Type safety (#6): No
anyin new code.state.toastId: string | number | undefinedcorrectly typed. - No console.log (#7): None in new production code.
- Comment policy (#4): JSDoc on exported
useUndoToast(both platforms) is permitted. No banned narration comments in new files.
What's good
- Single seam design: wiring undo in
onSuccessof each delete hook gives every existing call site undo for free with zero call-site changes. - Double-fire guard in web
useUndoToast(state.settledflag beforeperformRestore()) correctly handles any race betweenonAutoClose, action-button click, and Ctrl+Z. - Offline ordering is correct: delete + restore enqueue in FIFO order;
flushQueuedMutationsprocesses sequentially, so restore always executes after delete on reconnect. mutationTypeSchemaadditions are purely additive; old clients don't emit them, no validation rejection risk.- Test coverage is thorough:
use-undo-toast.test.tscovers Ctrl+Z, Cmd+Z, redo exclusion, button action, and auto-close. Each delete/restore pair has integration tests on both platforms. - i18n fully parallel: both locales updated in the same diff, all 6 keys present.
Recommendation
APPROVE — two Mediums, zero Critical/High.
Follow-ups worth tracking:
- Move
useRestoreGoal'sinvalidateQueriestoonSettled(1-line fix). - Add optimistic remove + rollback to web
useDeleteHabitto match mobile behavior.
Pre-merge gate: orbit-api #263 must be merged and deployed before production — restore endpoints return 404 until then. Already noted in the PR body; just surfacing it as the explicit gate.
🤖 Generated with Claude Code
There was a problem hiding this comment.
PR Review - feat: undo/restore for destructive actions (#11). Decision: APPROVE - 0 Critical, 0 High findings. Medium M1: useRestoreGoal on web invalidates in onSuccess only (apps/web/hooks/use-goals.ts:71-74) - move invalidateQueries to onSettled. Medium M2: useDeleteHabit on web has no optimistic remove (apps/web/hooks/use-habits.ts:302-319) - add onMutate + onError rollback mirroring mobile. Everything else green: parity paired, i18n in sync, backward-compat clean, double-fire guard correct, offline queue ordering correct.



Undo / restore for destructive actions
Closes #11. Deleting a habit, goal, or tag now shows an Undo snackbar; Undo restores it. Backend is orbit-api #263 (restore endpoints) — merge/deploy #263 first; the restore calls 404 until then.
Model
Delete is immediate (server soft-deletes, the row vanishes now). Undo = restore, not a deferred delete. The snackbar fires from each delete hook's
onSuccess— one seam, so every call site (habit menu/detail, goal drawer, tag manager) gets undo for free with zero call-site changes.Both platforms
showQueuedtoast primitive (the same one the notification bell uses for undo). Web extended it minimally to return the toast id + an onClose hook (for the Ctrl+Z cleanup); mobile uses it unchanged.Shared
restoreendpoint builders for habits/goals/tags.mutationTypeSchemavalues (restoreHabit/Goal/Tag) for the offline queue — backward-compatible (client-local enum; old queued mutations still validate).i18n
undonamespace in bothen.jsonandpt-BR.json(parity test green).Validation
type-check clean (3/3); lint 0 errors; tests — shared 1031, web 1673, mobile 645 passed.
🤖 Generated with Claude Code