344 lines
12 KiB
Markdown
344 lines
12 KiB
Markdown
# iOS Rollover Implementation — Open Questions & Answers
|
|
|
|
**Date**: 2025-01-27
|
|
**Status**: Pre-Implementation Decisions
|
|
|
|
---
|
|
|
|
## Question 1: Fetcher Integration
|
|
|
|
**Question**: How should we integrate DailyNotificationFetcher for prefetch scheduling? (Phase 2)
|
|
|
|
### Current State Analysis
|
|
|
|
- **Android**: Uses `DailyNotificationFetcher.scheduleFetch(fetchTime)` and `scheduleImmediateFetch()`
|
|
- **iOS**: Has `DailyNotificationBackgroundTaskManager` with `scheduleBackgroundTask()` method
|
|
- **iOS Pattern**: Uses `BGTaskScheduler` with `BGAppRefreshTaskRequest`
|
|
|
|
### Recommendation: **Defer to Phase 2, Use Placeholder Pattern**
|
|
|
|
**Rationale**:
|
|
1. **Phase 1 Focus**: Core rollover functionality (scheduling next notification)
|
|
2. **Prefetch is Separate**: Prefetch scheduling is independent of rollover
|
|
3. **Existing Infrastructure**: iOS already has background task infrastructure
|
|
4. **Android Pattern**: Android also separates rollover from prefetch (optional parameter)
|
|
|
|
### Implementation Approach
|
|
|
|
**Phase 1 (Current)**:
|
|
- Make `fetcher` parameter optional in `scheduleNextNotification()`
|
|
- Add TODO comments for Phase 2 integration
|
|
- Log prefetch scheduling intent (even if not executed)
|
|
|
|
**Phase 2 (Future)**:
|
|
- Create `DailyNotificationFetcher` class (iOS equivalent)
|
|
- Integrate with `DailyNotificationBackgroundTaskManager`
|
|
- Use `BGTaskScheduler` for prefetch scheduling
|
|
- Calculate fetch time: `nextScheduledTime - (5 * 60 * 1000)` (5 minutes before)
|
|
|
|
### Code Pattern
|
|
|
|
```swift
|
|
// Phase 1: Optional fetcher, log intent
|
|
if let fetcher = fetcher {
|
|
let fetchTime = nextScheduledTime - (5 * 60 * 1000)
|
|
// TODO: Phase 2 - Implement fetcher.scheduleFetch(fetchTime)
|
|
print("\(Self.TAG): RESCHEDULE_PREFETCH_SCHEDULED id=\(content.id) next_fetch=\(fetchTime)")
|
|
} else {
|
|
print("\(Self.TAG): RESCHEDULE_PREFETCH_SKIP id=\(content.id) fetcher_not_available")
|
|
}
|
|
```
|
|
|
|
**Decision**: ✅ **Defer to Phase 2, use optional parameter pattern**
|
|
|
|
---
|
|
|
|
## Question 2: AppDelegate Access
|
|
|
|
**Question**: Is there a better way to access the plugin from AppDelegate without using Capacitor bridge?
|
|
|
|
### Current State Analysis
|
|
|
|
- **Capacitor Pattern**: Uses `CAPBridgeViewController` to access plugins
|
|
- **Test App**: Already uses this pattern for other operations
|
|
- **Production Apps**: May have different AppDelegate structures
|
|
|
|
### Recommendation: **Use Notification Center Pattern**
|
|
|
|
**Rationale**:
|
|
1. **Decoupling**: AppDelegate doesn't need direct plugin reference
|
|
2. **Flexibility**: Works across different app architectures
|
|
3. **Reliability**: Notification center is always available
|
|
4. **Testability**: Easier to test without Capacitor dependency
|
|
|
|
### Implementation Approach
|
|
|
|
**Option A: Notification Center (Recommended)**
|
|
- Plugin registers for notification delivery events
|
|
- AppDelegate posts notification when delivery detected
|
|
- Plugin handles rollover in response to notification
|
|
|
|
**Option B: Capacitor Bridge (Fallback)**
|
|
- Use existing bridge pattern
|
|
- Works but creates tight coupling
|
|
- Use as fallback if notification center doesn't work
|
|
|
|
### Code Pattern
|
|
|
|
```swift
|
|
// In DailyNotificationPlugin.load():
|
|
NotificationCenter.default.addObserver(
|
|
self,
|
|
selector: #selector(handleNotificationDelivery(_:)),
|
|
name: NSNotification.Name("DailyNotificationDelivered"),
|
|
object: nil
|
|
)
|
|
|
|
// In AppDelegate.willPresent:
|
|
NotificationCenter.default.post(
|
|
name: NSNotification.Name("DailyNotificationDelivered"),
|
|
object: nil,
|
|
userInfo: [
|
|
"notification_id": notificationId,
|
|
"scheduled_time": scheduledTime
|
|
]
|
|
)
|
|
```
|
|
|
|
**Decision**: ✅ **Use Notification Center pattern, with Capacitor bridge as fallback**
|
|
|
|
---
|
|
|
|
## Question 3: Background Task
|
|
|
|
**Question**: Should we add a dedicated background task for rollover detection, or rely on existing recovery mechanisms?
|
|
|
|
### Current State Analysis
|
|
|
|
- **Existing Recovery**: `DailyNotificationReactivationManager` already runs on app launch
|
|
- **Background Tasks**: iOS has strict limits on background execution
|
|
- **Reliability**: Multiple detection mechanisms increase reliability
|
|
|
|
### Recommendation: **Rely on Existing Recovery + AppDelegate**
|
|
|
|
**Rationale**:
|
|
1. **iOS Limitations**: Background tasks are unreliable (system-controlled)
|
|
2. **Existing Infrastructure**: Recovery manager already handles app launch scenarios
|
|
3. **Coverage**: AppDelegate (foreground) + Recovery (background) covers all cases
|
|
4. **Simplicity**: Fewer moving parts = fewer failure points
|
|
|
|
### Implementation Approach
|
|
|
|
**Two Detection Mechanisms**:
|
|
1. **Foreground**: AppDelegate `willPresent` → immediate rollover
|
|
2. **Background**: Recovery Manager → rollover on app launch
|
|
|
|
**No Dedicated Background Task**:
|
|
- Background tasks are unreliable (system decides when to run)
|
|
- Recovery manager already covers app launch scenarios
|
|
- Adding another mechanism adds complexity without significant benefit
|
|
|
|
### Code Pattern
|
|
|
|
```swift
|
|
// Detection Mechanism 1: Foreground (AppDelegate)
|
|
func userNotificationCenter(_ center: UNUserNotificationCenter, willPresent notification: UNNotification, ...) {
|
|
// Trigger rollover immediately
|
|
await handleNotificationRollover(...)
|
|
}
|
|
|
|
// Detection Mechanism 2: Background (Recovery Manager)
|
|
func performColdStartRecovery() async {
|
|
// Check for delivered notifications
|
|
await checkAndProcessDeliveredNotifications()
|
|
}
|
|
```
|
|
|
|
**Decision**: ✅ **Rely on existing recovery + AppDelegate, no dedicated background task**
|
|
|
|
---
|
|
|
|
## Question 4: Error Handling
|
|
|
|
**Question**: How should we handle rollover failures? Retry? Log? User notification?
|
|
|
|
### Current State Analysis
|
|
|
|
- **Android Pattern**: Logs errors, continues execution (non-fatal)
|
|
- **iOS Recovery Manager**: Catches all errors, logs, continues (non-fatal)
|
|
- **User Experience**: Failures should be silent (background operation)
|
|
|
|
### Recommendation: **Log + Continue (Non-Fatal)**
|
|
|
|
**Rationale**:
|
|
1. **Background Operation**: Rollover is background, shouldn't interrupt user
|
|
2. **Recovery Available**: Recovery manager will catch missed rollovers on next app launch
|
|
3. **Consistency**: Matches Android and existing iOS recovery patterns
|
|
4. **User Experience**: Silent failures, recovery on next launch
|
|
|
|
### Implementation Approach
|
|
|
|
**Error Handling Strategy**:
|
|
1. **Log Errors**: Comprehensive logging for debugging
|
|
2. **Continue Execution**: Don't crash or interrupt app
|
|
3. **No Retry**: Let recovery manager handle on next launch
|
|
4. **No User Notification**: Background operation, silent failure
|
|
5. **History Recording**: Record failures in history (if history implemented)
|
|
|
|
### Code Pattern
|
|
|
|
```swift
|
|
func scheduleNextNotification(...) async -> Bool {
|
|
do {
|
|
// Rollover logic
|
|
return true
|
|
} catch {
|
|
print("\(Self.TAG): RESCHEDULE_ERR id=\(content.id) err=\(error.localizedDescription)")
|
|
// Log error but don't throw - let recovery handle on next launch
|
|
return false
|
|
}
|
|
}
|
|
|
|
// In recovery manager:
|
|
if !scheduled {
|
|
print("\(Self.TAG): Failed to roll over delivered notification id=\(notificationId)")
|
|
// Recovery will retry on next app launch
|
|
}
|
|
```
|
|
|
|
**Decision**: ✅ **Log + Continue (non-fatal), no retry, no user notification**
|
|
|
|
---
|
|
|
|
## Question 5: Performance
|
|
|
|
**Question**: Should we batch rollover operations or process individually?
|
|
|
|
### Current State Analysis
|
|
|
|
- **Android Pattern**: Processes individually (one notification at a time)
|
|
- **iOS Recovery**: Processes notifications individually
|
|
- **Volume**: Typically 1-2 notifications per day (low volume)
|
|
|
|
### Recommendation: **Process Individually**
|
|
|
|
**Rationale**:
|
|
1. **Low Volume**: Typically 1 notification per day, batching unnecessary
|
|
2. **Simplicity**: Individual processing is simpler and easier to debug
|
|
3. **Error Isolation**: Individual processing isolates failures
|
|
4. **Consistency**: Matches Android and existing iOS patterns
|
|
|
|
### Implementation Approach
|
|
|
|
**Individual Processing**:
|
|
- Process each notification rollover separately
|
|
- Each rollover is independent operation
|
|
- Failures in one don't affect others
|
|
- Easier to log and debug
|
|
|
|
**Future Optimization** (if needed):
|
|
- If volume increases, consider batching
|
|
- Current volume doesn't justify batching complexity
|
|
|
|
### Code Pattern
|
|
|
|
```swift
|
|
// Process individually (current approach)
|
|
for notification in deliveredNotifications {
|
|
await scheduler.scheduleNextNotification(notification, ...)
|
|
}
|
|
|
|
// Batching would look like:
|
|
// await scheduler.scheduleNextNotificationsBatch(notifications, ...)
|
|
// But not needed for current volume
|
|
```
|
|
|
|
**Decision**: ✅ **Process individually (current volume doesn't justify batching)**
|
|
|
|
---
|
|
|
|
## Question 6: Testing
|
|
|
|
**Question**: Do we need automated tests for rollover, or is manual testing sufficient for Phase 1?
|
|
|
|
### Current State Analysis
|
|
|
|
- **Existing Tests**: iOS has unit tests for recovery manager
|
|
- **Test Coverage**: Some components have tests, others don't
|
|
- **Phase 1 Scope**: Core rollover functionality
|
|
|
|
### Recommendation: **Manual Testing for Phase 1, Automated Tests for Phase 2**
|
|
|
|
**Rationale**:
|
|
1. **Phase 1 Focus**: Core functionality, manual testing sufficient
|
|
2. **Complexity**: Rollover involves system notifications (hard to test automatically)
|
|
3. **Time Investment**: Automated tests take time, manual testing faster for Phase 1
|
|
4. **Phase 2**: Add automated tests when edge cases are implemented
|
|
|
|
### Implementation Approach
|
|
|
|
**Phase 1 Testing**:
|
|
- Manual testing checklist
|
|
- Test scenarios: foreground delivery, background delivery, duplicates
|
|
- Real device testing (simulator may not handle notifications correctly)
|
|
|
|
**Phase 2 Testing**:
|
|
- Unit tests for time calculations (DST, timezone)
|
|
- Integration tests for rollover flow
|
|
- Edge case tests (time changes, timezone changes)
|
|
|
|
### Test Checklist (Phase 1)
|
|
|
|
1. ✅ **Foreground Delivery**: App running, notification fires → rollover triggers
|
|
2. ✅ **Background Delivery**: App not running, notification fires → rollover on launch
|
|
3. ✅ **Duplicate Prevention**: Multiple rollover attempts → only one scheduled
|
|
4. ✅ **DST Transition**: Schedule on DST day → correct time calculation
|
|
5. ✅ **Error Handling**: Simulate failure → graceful degradation
|
|
|
|
**Decision**: ✅ **Manual testing for Phase 1, automated tests for Phase 2**
|
|
|
|
---
|
|
|
|
## Summary of Decisions
|
|
|
|
| Question | Decision | Rationale |
|
|
|----------|----------|-----------|
|
|
| **Fetcher Integration** | Defer to Phase 2, optional parameter | Prefetch is separate concern, Phase 1 focuses on core rollover |
|
|
| **AppDelegate Access** | Notification Center pattern | Decoupling, flexibility, reliability |
|
|
| **Background Task** | Rely on existing recovery | iOS limitations, existing infrastructure sufficient |
|
|
| **Error Handling** | Log + Continue (non-fatal) | Background operation, recovery handles failures |
|
|
| **Performance** | Process individually | Low volume, simplicity, consistency |
|
|
| **Testing** | Manual for Phase 1, automated for Phase 2 | Phase 1 scope, complexity, time investment |
|
|
|
|
---
|
|
|
|
## Implementation Impact
|
|
|
|
### Changes to Review Document
|
|
|
|
Based on these decisions, the review document should be updated:
|
|
|
|
1. **Fetcher Parameter**: Make optional, add Phase 2 TODOs
|
|
2. **AppDelegate Pattern**: Use Notification Center instead of Capacitor bridge
|
|
3. **Background Task**: Remove dedicated background task, rely on recovery
|
|
4. **Error Handling**: Add comprehensive logging, non-fatal errors
|
|
5. **Performance**: Individual processing (no batching)
|
|
6. **Testing**: Manual testing checklist for Phase 1
|
|
|
|
### Next Steps
|
|
|
|
1. ✅ **Decisions Made** (This document)
|
|
2. **Update Review Document** with decisions
|
|
3. **Update Implementation Plan** with specific patterns
|
|
4. **Begin Phase 1 Implementation**
|
|
|
|
---
|
|
|
|
## References
|
|
|
|
- Review Document: `docs/ios-rollover-implementation-review.md`
|
|
- Edge Case Plan: `docs/ios-rollover-edge-case-plan.md`
|
|
- Android Implementation: `android/src/main/java/com/timesafari/dailynotification/DailyNotificationWorker.java`
|
|
- iOS Recovery Manager: `ios/Plugin/DailyNotificationReactivationManager.swift`
|
|
- iOS Background Tasks: `ios/Plugin/DailyNotificationBackgroundTaskManager.swift`
|