fix(habits): stop edit-save from wiping Astra-set reminders (#447 Bug 3) - #500
Conversation
Opening a due-timed habit's edit form and pressing Save silently blanked any scheduled reminders the habit held. Reminders live in two mutually- exclusive stores: reminderTimes (minute offsets, used with a due time) and scheduledReminders (absolute times, used without one). Astra's UpdateHabitTool historically wrote scheduledReminders onto due-timed habits, so a due-timed habit can legitimately carry that data. The edit form rendered only one reminder section (gated on dueTime), so the scheduledReminders were invisible, and buildUpdateHabitRequest always emitted scheduledReminders=[] in the due-time branch, wiping them on save. Two UI-only fixes (the Astra API write-fix is a separate orbit-api PR): - SAVE-guard (packages/shared habit-request-builders): the update contract is preserve-on-omit (an omitted field keeps its stored value; [] clears it). The builder now writes the scheduledReminders store only when the form actually surfaced its editor (hasScheduledReminders), and otherwise omits it so the API preserves it. It still clears the store when the user turns reminders off. reminderTimes is unchanged. - READ/render (web habit-form-fields + mobile advanced-section): the scheduled-reminder section is now also surfaced for a due-timed habit that already holds scheduled reminders, so the user can see and keep (or edit) them. Web and mobile stay visually and behaviorally identical. Regression tests (fail without the fix): a due-timed habit's scheduled reminders survive an edit-open + Save (shared builder), and the edit form renders the non-default store's section on both platforms. Refs #447 (Bug 3) 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
|
…ers card When a due-timed habit also holds scheduled reminders (legacy Astra mixed data), the edit form now shows the offset-reminder card and the scheduled- reminder card together. Both were rendering their own on/off switch bound to the single shared reminderEnabled flag, which read as duplicate reminder cards with duplicate toggles. The secondary (scheduled) card now renders without its own switch when it sits beside the offset card under a due time (new nested prop on ScheduledReminderSection, web + mobile): the offset card owns the single master toggle, and the scheduled card reads as the habit's existing scheduled reminders, still fully viewable, keepable, and removable via its chips. Behaviour and the save path are unchanged. The READ tests now assert a single toggle in the mixed view on both platforms. Also drops em-dashes from the new JSDoc to match the repo code standard. Refs #447 (Bug 3) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Follow-up commit |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #500 Review — fix(habits): stop edit-save from wiping Astra-set reminders (#447 Bug 3)
Reviewed per .claude/skills/pr-review/rubric.md, orchestrating parity-checker and design-reviewer against the diff, plus a direct read of the core logic change (packages/shared/src/utils/habit-request-builders.ts), its Zod contract (packages/shared/src/types/habit.ts), and both edit-modal call sites.
Summary
This is a UI-only fix for a real data-loss bug: applyUpdateReminderFields's due-time branch always emitted scheduledReminders: [], wiping Astra-set legacy reminder data on any edit+Save of a due-timed habit. The fix (a) only writes scheduledReminders on save when the form actually surfaced that editor (hasScheduledReminders), otherwise omitting the field so the (pre-existing, already-optional) preserve-on-omit backend contract keeps it, and (b) surfaces the scheduled-reminder section for a due-timed habit that already holds legacy scheduled reminders, so the data is visible and editable rather than invisible-then-wiped. A follow-up commit removed the resulting duplicate on/off switch by adding a nested prop that suppresses the switch on the secondary card.
Correctness: verified the builder logic by hand — hasScheduledReminders=false preserves prior behavior (backward-compatible default param, all other call sites unaffected); hasScheduledReminders=true writes the form's edited reminders when enabled and clears ([]) only when the user turns reminders off, matching the "deliberate clear must still persist" requirement. Confirmed updateHabitRequestSchema.scheduledReminders was already .optional() before this PR (packages/shared/src/types/habit.ts:296) — this change alters which optional fields get populated, not the wire contract itself, so there's no shared-type change and no backward-compat risk for old mobile clients.
Parity: parity-checker confirmed all changed web files have their mobile mirror changed with behaviorally identical logic (shouldShowScheduledReminders helper on web vs. an equivalent inlined condition on mobile — a stylistic, not behavioral, difference). Tests are paired on both platforms and in packages/shared.
Comment policy / root-cause: new JSDoc blocks all carry a WHY note with an issue URL (#447), consistent with the repo's comment-policy gate. This is a genuine root-cause fix, not a workaround.
No findings in: dead code, type safety (no any), console.log, i18n (no new user-facing strings — reuses existing habits.form.reminder / habits.form.scheduledReminder keys), contract drift (no shared-type change), security (no security-relevant surface touched), backend hard rules (orbit-api not touched by this PR — the API write-fix is explicitly a separate PR), FEATURES.md (bugfix restoring existing behavior, not a new/changed feature surface).
Not verifiable in CI: the orbit-api side of the backward-compat guard (sibling repo not checked out in this job) — moot here since this PR makes no shared-type change to verify against.
Findings
[Medium] Nested scheduled-reminder card has no visual cue when reminders are off
· dimension: 8 — DESIGN.md / AI-slop
· location: apps/web/components/habits/habit-form-fields/scheduled-reminder-section.tsx:66-85 (mirrored at apps/mobile/components/habits/habit-form-fields/scheduled-reminder-section.tsx:89-105)
· issue: When nested is true (a due-timed habit that also holds legacy scheduled reminders) and reminderEnabled is false, the card renders only a Bell icon + static label — no switch, no dimming, no caption tying it to the sibling offset-reminder card's toggle.
· risk: A user landing on this state sees two stacked, visually-identical inset-ring cards, one with a working switch and one that looks like a dead/broken toggle row, since every other list-row in the app pairs a label with its own control or an explicit disabled treatment.
· fix: When nested && !reminderEnabled, dim the row (e.g. reduced opacity) or add a short caption noting it's governed by the card above, on both platforms.
· reference: DESIGN.md (Switch/ListRow pattern — label always paired with its own control or explicit disabled state)
No Critical or High findings survive. This finding is Medium and concretely actionable but does not block merge per the rubric's deterministic outcome rule.
Outcome: APPROVE
Zero Critical/High findings. The core logic fix is correct, backward-compatible, well-tested (both new regression tests and the preserved-vs-cleared distinction), and paired across web/mobile. The one Medium UI-polish finding above is worth a fast follow-up but doesn't rise to blocking.
There was a problem hiding this comment.
PR #500 Review — fix(habits): stop edit-save from wiping Astra-set reminders (#447 Bug 3)
Reviewed per .claude/skills/pr-review/rubric.md, orchestrating parity-checker and design-reviewer against the diff, plus a direct read of the core logic change (packages/shared/src/utils/habit-request-builders.ts), its Zod contract (packages/shared/src/types/habit.ts), and both edit-modal call sites. This is a re-review after my prior APPROVE on this PR (2026-07-13T22:57:26Z) — the only commit since then (78e049de, a merge of main) touches unrelated pages (achievements, ai-settings, calendar-sync, profile, social, retrospective) and does not modify any file this PR changes, so there is nothing new to re-assess in this fix.
Summary
This is a UI-only fix for a real data-loss bug: applyUpdateReminderFields's due-time branch always emitted scheduledReminders: [], wiping Astra-set legacy reminder data on any edit+Save of a due-timed habit. The fix (a) only writes scheduledReminders on save when the form actually surfaced that editor (hasScheduledReminders), otherwise omitting the field so the (pre-existing, already-optional) preserve-on-omit backend contract keeps it, and (b) surfaces the scheduled-reminder section for a due-timed habit that already holds legacy scheduled reminders, so the data is visible and editable rather than invisible-then-wiped. A follow-up commit removed the resulting duplicate on/off switch by adding a nested prop that suppresses the switch on the secondary card.
Correctness: verified the builder logic by hand — hasScheduledReminders=false preserves prior behavior (backward-compatible default param, all other call sites unaffected); hasScheduledReminders=true writes the form's edited reminders when enabled and clears ([]) only when the user turns reminders off, matching the "deliberate clear must still persist" requirement. Confirmed updateHabitRequestSchema.scheduledReminders was already .optional() before this PR (packages/shared/src/types/habit.ts:296) — this change alters which optional fields get populated, not the wire contract itself, so there's no shared-type change and no backward-compat risk for old mobile clients.
Parity: parity-checker confirmed all changed web files have their mobile mirror changed with behaviorally identical logic (shouldShowScheduledReminders helper on web vs. an equivalent inlined condition on mobile — a stylistic, not behavioral, difference). Tests are paired on both platforms and in packages/shared.
Comment policy / root-cause: new JSDoc blocks all carry a WHY note with an issue URL (#447), consistent with the repo's comment-policy gate. This is a genuine root-cause fix, not a workaround.
No findings in: dead code, type safety (no any), console.log, i18n (no new user-facing strings — reuses existing habits.form.reminder / habits.form.scheduledReminder keys), contract drift (no shared-type change), security (no security-relevant surface touched), backend hard rules (orbit-api not touched by this PR — the API write-fix is explicitly a separate PR), FEATURES.md (bugfix restoring existing behavior, not a new/changed feature surface).
Not verifiable in CI: the orbit-api side of the backward-compat guard (sibling repo not checked out in this job) — moot here since this PR makes no shared-type change to verify against.
Findings
[Medium] Nested scheduled-reminder card has no visual cue when reminders are off
· dimension: 8 — DESIGN.md / AI-slop
· location: apps/web/components/habits/habit-form-fields/scheduled-reminder-section.tsx:66-85 (mirrored at apps/mobile/components/habits/habit-form-fields/scheduled-reminder-section.tsx:89-105)
· issue: When nested is true (a due-timed habit that also holds legacy scheduled reminders) and reminderEnabled is false, the card renders only a Bell icon + static label — no switch, no dimming, no caption tying it to the sibling offset-reminder card's toggle.
· risk: A user landing on this state sees two stacked, visually-identical inset-ring cards, one with a working switch and one that looks like a dead/broken toggle row, since every other list-row in the app pairs a label with its own control or an explicit disabled treatment.
· fix: When nested && !reminderEnabled, dim the row (e.g. reduced opacity) or add a short caption noting it's governed by the card above, on both platforms.
· reference: DESIGN.md (Switch/ListRow pattern — label always paired with its own control or explicit disabled state)
This finding was already surfaced in my prior review on this PR and is carried forward unchanged (not re-derived) since nothing relevant changed. It remains Medium and does not block merge per the rubric's deterministic outcome rule.
No Critical or High findings survive.
Outcome: APPROVE
Zero Critical/High findings. The core logic fix is correct, backward-compatible, well-tested (both new regression tests and the preserved-vs-cleared distinction), and paired across web/mobile. The one Medium UI-polish finding above is worth a fast follow-up but doesn't rise to blocking.
|
Bug 3) (#393) * fix(api): route Astra reminders into the correct store by due time (#447 Bug 3) Reminders live in two mutually exclusive stores keyed on whether a habit has a due time: due-timed habits use reminderTimes (minute offsets before the due time) while habits with no due time use scheduledReminders (absolute day_before/same_day times). The Astra create_habit and update_habit tools expose both shapes and passed whichever store the model populated straight through, so a model that emitted scheduled_reminders for a due-timed habit wrote the WRONG store. The mobile/web clients only read reminderTimes for due-timed habits, so those reminders were silently lost. Add ReminderStoreNormalizer, mirroring the web/mobile request builders' store selection: for a due-timed habit it converts any supplied absolute reminders into minute offsets, merges them with reminder_times, and empties the scheduled-reminder store; for a habit with no due time both stores pass through unchanged. Wire both tools through it. This also self-heals a due-timed habit that carried stale scheduledReminders whenever Astra next touches its reminders. The UI half (SAVE-guard + tolerant render) shipped in thomasluizon/orbit-ui-mobile#500. Append-only: no DTO field renamed or removed; the model is unchanged (no migration). Refs thomasluizon/orbit-ui-mobile#447 (Bug 3, API write-fix) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api): normalize reminders in create_sub_habit too; add day_before tests Address PR review: create_sub_habit exposed the same dual reminder-store shape (due_time + scheduled_reminders) and passed both straight through CreateSubHabitCommand, so a due-timed sub-habit created by Astra with scheduled_reminders reproduced Bug 3. Route CreateSubHabitTool through ReminderStoreNormalizer, closing the root cause across all three habit-creating Astra tools. Tests: sub-habit same_day + day_before conversion and no-due-time pass-through; plus day_before cross-day offset coverage for create_habit and update_habit (the +1440 branch the same_day-only tests missed). Refs thomasluizon/orbit-ui-mobile#447 (Bug 3, API write-fix) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(api): drop redundant System.Linq import in sub-habit tool tests ImplicitUsings already provides System.Linq; the explicit import flagged in review was dead. No behavior change. Refs thomasluizon/orbit-ui-mobile#447 (Bug 3, API write-fix) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(api): heal due-time transitions and stop dropping late same-day reminders Address PR review (round 3): [HIGH] A due-time-only update crossed the reminder-store boundary without re-routing existing reminders: adding a due time to a habit that held scheduledReminders, or clearing the due time on a habit that held reminderTimes offsets, left the reminders stranded in the store the client no longer reads for that habit shape — reminders silently stopped firing. UpdateHabitTool now calls ReminderStoreNormalizer.NormalizeForUpdate, which migrates the habit's existing reminders across the boundary when the caller did not supply reminder fields: scheduledReminders -> minute offsets before the new due time; reminderTimes offsets -> absolute reminders relative to the previous due time. When reminders are supplied, or no boundary is crossed, behavior is unchanged. [MEDIUM] A same-day scheduled reminder timed after the due time produced a negative offset that was silently discarded. It is now clamped to 0 (fire at the due time) so no reminder is lost. Tests: add-due-time conversion, clear-due-time conversion, and the after-due-time clamp, each failing without the fix. Model unchanged (has-pending-model-changes = false). Refs thomasluizon/orbit-ui-mobile#447 (Bug 3, API write-fix) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
The data-loss chain (#447 Bug 3)
Opening a due-timed habit's edit form and pressing Save silently blanked any Astra-set reminders the habit held.
Reminders live in two mutually-exclusive stores, keyed on whether the habit has a due time:
reminderTimes— minute offsets, used when a due time existsscheduledReminders— absoluteday_before/same_daytimes, used when there is noneAstra's
UpdateHabitTool(orbit-api) historically wrotescheduled_remindersonto a due-timed habit, so a due-timed habit can legitimately carryscheduledRemindersdata. The failure chain:packages/shared/.../habit-request-builders.tsapplyUpdateReminderFieldsalways emittedscheduledReminders: []in the due-time branch → the update wiped whatever the form wasn't editing → the domain overwrote it to empty.dueTime(webhabit-form-fields.tsx, mobileadvanced-section.tsx), so that data was invisible and then wiped on Save.The API update contract (verified against
orbit-apiHabit.Update/ApplyOptionalUpdates) is preserve-on-omit: a reminder field absent from the request keeps its stored value; an explicit[]clears it.The two UI fixes (this PR)
The Astra API write-fix is a separate orbit-api PR — this PR is UI-only.
1. SAVE-guard (
packages/shared/src/utils/habit-request-builders.ts)applyUpdateReminderFieldsnow writes thescheduledRemindersstore only when the form actually surfaced its editor (hasScheduledReminders), otherwise it omits the field so the API preserves whatever it holds. It no longer emitsscheduledReminders: []purely because of the due-time branch. It still clears the store ([]) when the user turns reminders off, and thereminderTimesstore behaviour is unchanged.buildUpdateHabitRequestgains an optionalhasScheduledRemindersargument, threaded from each edit modal (habit.scheduledReminders.length > 0).2. READ / render (web
habit-form-fields.tsx, mobileadvanced-section.tsx)The scheduled-reminder section is now also surfaced for a due-timed habit that already holds scheduled reminders (legacy mixed data), so the user can see and keep (or edit) them. The visibility rule (
!isGeneral && (!dueTime || hasScheduledReminders)) is stable across the edit session, so emptying the list still lets a deliberate clear persist. Web and mobile stay visually and behaviourally identical.Tests (fail without the fix)
packages/sharedbuilder): a due-timed habit that holdsscheduledReminderssurvives an edit-open + Save with its reminders intact; the store is only cleared when reminders are turned off; and it is omitted (not wiped) for a plain due-timed habit. Before the fix the due-time branch always producedscheduledReminders: [], so the preserve test fails.habit-form-fieldscomponent tests): the edit form renders both reminder sections for a due-timed habit that also holds scheduled reminders, and hides the scheduled section for a plain due-timed habit.Verification (all green, in the worktree)
packages/shared: lint, type-check, 1515 testsapps/web: lint (0 errors), type-check, 2265 testsapps/mobile: lint, type-check, 1072 testsRefs #447 (Bug 3)
🤖 Generated with Claude Code