feat: add dedicated Notifications settings page with frequency slider#4247
feat: add dedicated Notifications settings page with frequency slider#4247
Conversation
- Add NotificationsSettingsPage combining Daily Summary, Daily Reflection, and Notification Frequency settings - Move Notifications to main Settings drawer (after Profile) - Remove Daily Summary from Profile page (now consolidated in Notifications page) - Add 0-5 Notification Frequency slider to control proactive notification frequency - Add notifications section to desktop settings modal with same functionality - Comment out 'Wrapped 2025' option from settings drawer⚠️ NOT FULLY TESTED - Can be reverted if issues are found
There was a problem hiding this comment.
Code Review
This pull request introduces a new dedicated "Notifications" settings page, consolidating daily summary, daily reflection, and a new notification frequency control. The changes are well-structured, moving notification-related settings to a more logical place and enhancing user control over notifications. The implementation on both mobile and desktop looks consistent.
My review focuses on improving code robustness and maintainability, particularly around asynchronous operations in Flutter widgets. I've suggested a couple of refactorings to prevent potential issues with widget lifecycle and to improve code clarity.
| Future<void> _loadDailySummarySettings() async { | ||
| final settings = await getDailySummarySettings(); | ||
| final reflectionEnabled = SharedPreferencesUtil().dailyReflectionEnabled; | ||
| final frequency = SharedPreferencesUtil().notificationFrequency; | ||
| if (settings != null && mounted) { | ||
| setState(() { | ||
| _dailySummaryEnabled = settings.enabled; | ||
| _dailySummaryHour = settings.hour; | ||
| _dailyReflectionEnabled = reflectionEnabled; | ||
| _notificationFrequency = frequency; | ||
| _dailySummaryLoading = false; | ||
| }); | ||
| } else if (mounted) { | ||
| setState(() => _dailySummaryLoading = false); | ||
| setState(() { | ||
| _dailyReflectionEnabled = reflectionEnabled; | ||
| _notificationFrequency = frequency; | ||
| _dailySummaryLoading = false; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The _loadDailySummarySettings method contains duplicated code in the if and else if blocks for setting state. This can lead to maintenance issues where one block is updated but the other is forgotten. Additionally, the mounted check should ideally be performed immediately after an await call to avoid further processing if the widget has been disposed. Refactoring this method will improve its readability and robustness.
Future<void> _loadDailySummarySettings() async {
final settings = await getDailySummarySettings();
if (!mounted) return;
final reflectionEnabled = SharedPreferencesUtil().dailyReflectionEnabled;
final frequency = SharedPreferencesUtil().notificationFrequency;
setState(() {
if (settings != null) {
_dailySummaryEnabled = settings.enabled;
_dailySummaryHour = settings.hour;
}
_dailyReflectionEnabled = reflectionEnabled;
_notificationFrequency = frequency;
_dailySummaryLoading = false;
});
}
| Future<void> _loadSettings() async { | ||
| // Load Daily Summary settings from API | ||
| final settings = await getDailySummarySettings(); | ||
|
|
||
| // Load settings from local prefs | ||
| final reflectionEnabled = SharedPreferencesUtil().dailyReflectionEnabled; | ||
| final frequency = SharedPreferencesUtil().notificationFrequency; | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| if (settings != null) { | ||
| _dailySummaryEnabled = settings.enabled; | ||
| _dailySummaryHour = settings.hour; | ||
| } | ||
| _dailyReflectionEnabled = reflectionEnabled; | ||
| _notificationFrequency = frequency; | ||
| _isLoading = false; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
To prevent potential 'setState() called after dispose()' errors, it's a best practice to check if (mounted) immediately after an await call before executing any further logic that depends on the widget's state. In _loadSettings, the mounted check is performed after synchronous calls to SharedPreferencesUtil, which could be avoided if the widget is no longer in the tree.
| Future<void> _loadSettings() async { | |
| // Load Daily Summary settings from API | |
| final settings = await getDailySummarySettings(); | |
| // Load settings from local prefs | |
| final reflectionEnabled = SharedPreferencesUtil().dailyReflectionEnabled; | |
| final frequency = SharedPreferencesUtil().notificationFrequency; | |
| if (mounted) { | |
| setState(() { | |
| if (settings != null) { | |
| _dailySummaryEnabled = settings.enabled; | |
| _dailySummaryHour = settings.hour; | |
| } | |
| _dailyReflectionEnabled = reflectionEnabled; | |
| _notificationFrequency = frequency; | |
| _isLoading = false; | |
| }); | |
| } | |
| } | |
| Future<void> _loadSettings() async { | |
| // Load Daily Summary settings from API | |
| final settings = await getDailySummarySettings(); | |
| if (!mounted) return; | |
| // Load settings from local prefs | |
| final reflectionEnabled = SharedPreferencesUtil().dailyReflectionEnabled; | |
| final frequency = SharedPreferencesUtil().notificationFrequency; | |
| setState(() { | |
| if (settings != null) { | |
| _dailySummaryEnabled = settings.enabled; | |
| _dailySummaryHour = settings.hour; | |
| } | |
| _dailyReflectionEnabled = reflectionEnabled; | |
| _notificationFrequency = frequency; | |
| _isLoading = false; | |
| }); | |
| } | |
…BasedHardware#4247) ## Changes - Add `NotificationsSettingsPage` combining Daily Summary, Daily Reflection, and Notification Frequency settings - Move Notifications to main Settings drawer (after Profile) - Remove Daily Summary from Profile page (now consolidated in Notifications page) - Add 0-5 Notification Frequency slider to control proactive notification frequency - Add notifications section to desktop settings modal with same functionality - Comment out 'Wrapped 2025' option from settings drawer ##⚠️ WARNING **This PR is NOT fully tested and can be reverted if issues are found.** The notification frequency slider currently only saves the preference locally. Backend integration for using this frequency value in notification scheduling is pending.
Changes
NotificationsSettingsPagecombining Daily Summary, Daily Reflection, and Notification Frequency settingsThis PR is NOT fully tested and can be reverted if issues are found.
The notification frequency slider currently only saves the preference locally. Backend integration for using this frequency value in notification scheduling is pending.