docs: expand proper-fix section in New Activity lay-of-the-land
Add section 6 with Option A (skip single reminder when dialog is for New Activity), Option B (cancel single reminder on disable, with caveats), and optional cleanup notes. For team discussion and implementation.
This commit is contained in:
Jose Olarte III
145
doc/notification-new-activity-lay-of-the-land.md
Normal file
145
doc/notification-new-activity-lay-of-the-land.md
Normal file
@@ -0,0 +1,145 @@
|
|||||||
|
# Lay of the Land: API-Driven Daily Message (New Activity) and Web-Push Confusion
|
||||||
|
|
||||||
|
**Purpose:** Shareable analysis of the New Activity (API-driven daily message) implementation and the root cause of “always fires / can’t be turned off.” For discussion with teammates.
|
||||||
|
|
||||||
|
**Related:** `doc/notification-from-api-call.md` (plan and progress), teammate note about web-push confusion and possibly removing that logic.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Two Separate Notification Features
|
||||||
|
|
||||||
|
There are **two** distinct native notification flows that both go through the same UI component:
|
||||||
|
|
||||||
|
| Feature | Plugin API | Purpose |
|
||||||
|
|--------|------------|--------|
|
||||||
|
| **Daily Reminder** | `scheduleDailyNotification` / `cancelDailyReminder` | Single daily alarm, static title/body (user’s message). |
|
||||||
|
| **New Activity** (API-driven) | `scheduleDualNotification` / `cancelDualSchedule` | Prefetch from API 5 min before, then notify at chosen time with API or fallback content. |
|
||||||
|
|
||||||
|
- **Daily Reminder** is driven from AccountViewView’s “Daily Reminder” toggle; on native it uses `NotificationService.getInstance().scheduleDailyNotification()` / `cancelDailyNotification()` (backed by `NativeNotificationService` and a single `reminderId`: `"daily_timesafari_reminder"`).
|
||||||
|
- **New Activity** is intended to be driven only by `scheduleNewActivityDualNotification()` / `cancelDualSchedule()` in AccountViewView (dual schedule only).
|
||||||
|
|
||||||
|
So: one feature = single schedule (reminder), the other = dual schedule (prefetch + notify). They are different plugin APIs and different lifecycle (enable/disable) handling.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. Where the Bug Comes From: One Dialog, Two Behaviors
|
||||||
|
|
||||||
|
**New Activity** reuses the same dialog as Daily Reminder: **`PushNotificationPermission.vue`**.
|
||||||
|
|
||||||
|
- When the user turns **New Activity** on from AccountViewView:
|
||||||
|
- AccountViewView opens this dialog with `DAILY_CHECK_TITLE` and a callback that, on success, calls `scheduleNewActivityDualNotification(timeText)` on native.
|
||||||
|
- The dialog does **not** receive `skipSchedule: true` for this flow (only the “edit reminder” flow does).
|
||||||
|
|
||||||
|
So when the user clicks “Turn on Daily Reminder” in the dialog for **New Activity**:
|
||||||
|
|
||||||
|
1. **PushNotificationPermission** (native path) runs `turnOnNativeNotifications()` and always calls:
|
||||||
|
- `service.scheduleDailyNotification({ time, title: "Daily Check-In", body: "Time to check your TimeSafari activity", ... })`
|
||||||
|
- i.e. it schedules the **single** daily reminder (plugin’s `scheduleDailyNotification`), using the same `reminderId` as Daily Reminder (`"daily_timesafari_reminder"`).
|
||||||
|
2. Then the callback runs and AccountViewView calls **`scheduleNewActivityDualNotification(timeText)`**, which calls the plugin’s **`scheduleDualNotification`**.
|
||||||
|
|
||||||
|
Result:
|
||||||
|
|
||||||
|
- **Two schedules** are created when enabling New Activity:
|
||||||
|
- One **single** reminder (wrong for New Activity): static “Daily Check-In” message, same ID as Daily Reminder.
|
||||||
|
- One **dual** schedule (correct): prefetch + notify with API/fallback content.
|
||||||
|
- When the user turns **New Activity** off, AccountViewView only calls **`cancelDualSchedule()`**. It never calls `cancelDailyNotification()` (or equivalent) for the single reminder.
|
||||||
|
- So the **single** reminder stays scheduled and keeps firing at the chosen time. That’s the notification that “always fires” and “can’t be turned off.”
|
||||||
|
|
||||||
|
So the “huge problem with confusion with the web-push” is really: **the same dialog and the same “Turn on” path are used for both Daily Reminder and New Activity, but the dialog always schedules the single daily reminder on native**, while New Activity is supposed to use only the dual schedule. That mixing is what makes the wrong schedule stick and not be cancellable from the New Activity toggle.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Key Files and Flows
|
||||||
|
|
||||||
|
- **`src/components/PushNotificationPermission.vue`**
|
||||||
|
- Shared dialog for both “Daily Reminder” and “New Activity” (via `pushType` = `DIRECT_PUSH_TITLE` vs `DAILY_CHECK_TITLE`).
|
||||||
|
- On native it always uses `NotificationService.getInstance().scheduleDailyNotification(...)` (single reminder) and does not branch on “New Activity” to skip scheduling or to call the dual API.
|
||||||
|
- Saves `notifyingNewActivityTime` when `pushType === DAILY_CHECK_TITLE` (lines 834–836). So the dialog both schedules the wrong thing and persists settings for New Activity.
|
||||||
|
|
||||||
|
- **`src/views/AccountViewView.vue`**
|
||||||
|
- **Daily Reminder:** toggle opens same dialog with `DIRECT_PUSH_TITLE`; on native, disable path calls `service.cancelDailyNotification()`.
|
||||||
|
- **New Activity:** toggle opens same dialog with `DAILY_CHECK_TITLE`; on success callback calls `scheduleNewActivityDualNotification(timeText)`; on disable only calls `DailyNotification.cancelDualSchedule()`.
|
||||||
|
- `initializeState()`: on native with `activeDid`, calls `configureNativeFetcherIfReady(activeDid)` and, if New Activity is on, `updateStarredPlans(...)`. It does **not** re-call `scheduleNewActivityDualNotification` on load (so no double dual-schedule from here).
|
||||||
|
|
||||||
|
- **`src/services/notifications/NativeNotificationService.ts`**
|
||||||
|
- Single reminder only: `scheduleDailyNotification` → plugin `scheduleDailyNotification` with `id: this.reminderId` (`"daily_timesafari_reminder"`); `cancelDailyNotification` → `cancelDailyReminder({ reminderId })`. No dual API here.
|
||||||
|
|
||||||
|
- **`src/services/notifications/nativeFetcherConfig.ts`**
|
||||||
|
- Only configures the plugin for API calls (JWT, apiBaseUrl, activeDid). No scheduling.
|
||||||
|
|
||||||
|
- **`src/services/notifications/dualScheduleConfig.ts`**
|
||||||
|
- Builds config for `scheduleDualNotification` (contentFetch 5 min before, userNotification at notify time). Used only from AccountViewView’s `scheduleNewActivityDualNotification`.
|
||||||
|
|
||||||
|
- **`src/main.capacitor.ts`**
|
||||||
|
- Imports the daily-notification plugin; after a 2s delay calls `configureNativeFetcherIfReady()`. No scheduling; only fetcher config.
|
||||||
|
|
||||||
|
So: the “always fires / can’t turn off” behavior is from the **single** reminder created in `PushNotificationPermission` for New Activity and never cancelled when New Activity is turned off. The “confusion with web-push” is the reuse of the same dialog and the same native “schedule single reminder” path for both features.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Plugin Usage Summary
|
||||||
|
|
||||||
|
- **Single daily reminder (Daily Reminder):**
|
||||||
|
- Scheduled/cancelled via `NativeNotificationService.scheduleDailyNotification` / `cancelDailyNotification` → plugin `scheduleDailyNotification` / `cancelDailyReminder` with one `reminderId`.
|
||||||
|
- **Dual schedule (New Activity):**
|
||||||
|
- Scheduled/cancelled only in AccountViewView via `DailyNotification.scheduleDualNotification` / `cancelDualSchedule` (and `configureNativeFetcherIfReady` + `updateStarredPlans` as per doc).
|
||||||
|
- **Fetcher config (New Activity):**
|
||||||
|
- `configureNativeFetcherIfReady()` from main.capacitor and from AccountViewView `initializeState` / `scheduleNewActivityDualNotification`; no scheduling by itself.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Root Cause (Concise)
|
||||||
|
|
||||||
|
- **Single code path in PushNotificationPermission** for native: it always schedules the **single** daily reminder, regardless of `pushType` (Daily Reminder vs New Activity).
|
||||||
|
- For **New Activity**, that creates an extra, wrong schedule (single reminder) in addition to the correct dual schedule.
|
||||||
|
- **Disable path for New Activity** only calls `cancelDualSchedule()` and never cancels the single reminder, so that reminder keeps firing and appears as “always fires” and “can’t be turned off.”
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 6. Proper Fix: Options and Detail
|
||||||
|
|
||||||
|
A fix should ensure that (1) enabling New Activity creates only the dual schedule, and (2) disabling New Activity removes every schedule that was created for it. Below are concrete options and implementation notes.
|
||||||
|
|
||||||
|
### 6.1 Option A: Don’t schedule the single reminder when the dialog is for New Activity (recommended)
|
||||||
|
|
||||||
|
**Idea:** On native, when the dialog is opened for **New Activity** (`pushType === DAILY_CHECK_TITLE`), the dialog should **not** call `scheduleDailyNotification`. Only the callback in AccountViewView should run, and it already calls `scheduleNewActivityDualNotification(timeText)`, which uses the dual API only.
|
||||||
|
|
||||||
|
**Where:** `PushNotificationPermission.vue`, inside `turnOnNativeNotifications()`.
|
||||||
|
|
||||||
|
**Implementation sketch:**
|
||||||
|
|
||||||
|
- After requesting permissions and before calling `service.scheduleDailyNotification(...)`, branch on `pushType` and platform:
|
||||||
|
- If native **and** `pushType === this.DAILY_CHECK_TITLE`: skip the `scheduleDailyNotification` call entirely. Still run the rest of the flow (e.g. build `timeText`, save settings if desired, call `callback(true, timeText, ...)`). AccountViewView’s callback will then call `scheduleNewActivityDualNotification(timeText)` and that is the only schedule created for New Activity.
|
||||||
|
- Otherwise (web, or Daily Reminder on native): keep current behavior and call `scheduleDailyNotification` as today.
|
||||||
|
|
||||||
|
**Pros:** Single source of truth for “what is scheduled for New Activity” (dual only). No leftover single reminder to cancel later. Clear separation: dialog collects time + permission; AccountViewView owns native scheduling for New Activity.
|
||||||
|
|
||||||
|
**Cons:** Dialog’s native path now has two behaviors (schedule vs no schedule) depending on `pushType`; needs a quick comment so future changes don’t regress.
|
||||||
|
|
||||||
|
**Note:** The “edit reminder” flow already uses `skipSchedule: true` so the dialog doesn’t schedule; only the parent does. For New Activity enable, we’re doing the same idea: dialog doesn’t schedule on native, parent does.
|
||||||
|
|
||||||
|
### 6.2 Option B: When turning New Activity off, also cancel the single reminder
|
||||||
|
|
||||||
|
**Idea:** Assume the wrong single reminder might already exist (e.g. from before the fix, or from a different code path). When the user turns **New Activity** off, in addition to `cancelDualSchedule()`, call the service’s `cancelDailyNotification()` so the single reminder (same `reminderId` as Daily Reminder) is cancelled too.
|
||||||
|
|
||||||
|
**Where:** `AccountViewView.vue`, inside the disable branch of `showNewActivityNotificationChoice()` (where we currently only call `DailyNotification.cancelDualSchedule()`).
|
||||||
|
|
||||||
|
**Implementation sketch:**
|
||||||
|
|
||||||
|
- On native, when user confirms “turn off New Activity”:
|
||||||
|
1. Call `DailyNotification.cancelDualSchedule()` (existing).
|
||||||
|
2. Call `NotificationService.getInstance().cancelDailyNotification()` (new) so any single reminder that was mistakenly scheduled for this flow is removed.
|
||||||
|
|
||||||
|
**Pros:** Defensive: cleans up the bad schedule even if it was created in the past or by another path. Complements Option A (e.g. A prevents new wrong schedules; B cleans up existing ones).
|
||||||
|
|
||||||
|
**Cons:** That single `reminderId` is shared with **Daily Reminder**. If the user has **Daily Reminder** on and **New Activity** on, then turns only **New Activity** off, we must not cancel the reminder they still want for Daily Reminder. So either:
|
||||||
|
- Only call `cancelDailyNotification()` when we’re sure the single reminder was created for New Activity (e.g. we don’t have a separate “New Activity reminder ID”), which is hard without more state, or
|
||||||
|
- Don’t use Option B alone as the primary fix: use Option A so we never create the single reminder for New Activity, and only add B if we decide we need a one-time cleanup or a safety net (with care not to cancel Daily Reminder’s schedule).
|
||||||
|
|
||||||
|
**Recommendation:** Use Option A as the main fix. Add Option B only if the team agrees we need to cancel the single reminder on “New Activity off” and can do so without affecting Daily Reminder (e.g. by introducing a distinct reminder ID for a “New Activity legacy” reminder and only cancelling that, or by documenting that B is a one-time migration and not long-term behavior).
|
||||||
|
|
||||||
|
### 6.3 Optional cleanup: Separate reminder IDs or dialog responsibilities
|
||||||
|
|
||||||
|
- **Separate reminder IDs:** Today both Daily Reminder and the mistaken New Activity single reminder use `"daily_timesafari_reminder"`. If we ever want to support “both features on” and cancel only one, we’d need a second ID (e.g. one for Daily Reminder, one for New Activity). With Option A in place, New Activity no longer creates a single reminder, so we might not need a second ID unless we add a dedicated “New Activity fallback” single alarm later.
|
||||||
|
- **Dialog responsibilities:** We could narrow the dialog’s role when used for New Activity on native to “collect time + request permission and report success,” and leave all scheduling to AccountViewView. That’s what Option A does without necessarily refactoring the rest of the dialog (e.g. web push, Daily Reminder) in the same change.
|
||||||
|
- **Removing web-push logic for New Activity:** If the team decides to “totally remove” web-push logic that was added for New Activity, that would be a separate change (e.g. ensure New Activity on web either uses a different mechanism or is explicitly unsupported). The lay-of-the-land and this fix section focus on native; web can be scoped in a follow-up.
|
||||||
Reference in New Issue
Block a user