Files

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`