-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix hide goals toggle #4335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix hide goals toggle #4335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements the 'hide goals' toggle by adding a condition to check SharedPreferencesUtil().showGoalTrackerEnabled before rendering the goals-related widgets on both mobile and desktop conversation pages.
While the logic correctly gates the feature, the implementation has a significant drawback: it's not reactive. Accessing SharedPreferences directly in the build method means the UI won't update automatically when the user changes the setting. I've left comments on both modified files with a recommendation to use a ChangeNotifierProvider for managing this setting to ensure the UI responds instantly to user actions. This is a high-severity issue as it impacts user experience directly.
| // Daily Score + Today Tasks + Goals section | ||
| if (hasAnyConversationsInSystem && | ||
| !isSearchActive && | ||
| SharedPreferencesUtil().showGoalTrackerEnabled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling SharedPreferencesUtil() directly within the build method to control UI visibility can lead to a poor user experience. SharedPreferences is not reactive, meaning that when the user changes the showGoalTrackerEnabled setting (e.g., on a settings page), this widget will not automatically rebuild to reflect the change. The goals section will only hide or show after a rebuild is triggered by another state change.
To fix this, you should manage this setting using a reactive state management solution like Provider.
Recommendation:
- Create a
SettingsProviderthat usesChangeNotifier. - This provider should load the setting from
SharedPreferencesin its constructor. - Expose the setting value through a getter.
- Create a method in the provider to update the setting. This method should update the local value, save it to
SharedPreferences, and then callnotifyListeners(). - In this widget, use
context.watch<SettingsProvider>().showGoalTrackerEnabledto reactively listen to changes.
This will ensure the UI updates instantly when the user toggles the setting.
| padding: EdgeInsets.symmetric(horizontal: 16, vertical: 8), | ||
| child: DailyScoreWidget(), | ||
| // Daily Score, Today's Tasks, and Goals Widgets | ||
| if (SharedPreferencesUtil().showGoalTrackerEnabled) ...[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the desktop page, calling SharedPreferencesUtil() directly in the build method is not reactive. When the showGoalTrackerEnabled setting is changed by the user, the UI will not update to hide or show the goals-related widgets until something else causes a rebuild.
For a responsive user experience, this setting should be managed by a reactive state manager like a ChangeNotifierProvider.
Recommendation:
I've left a more detailed comment on desktop_conversations_page.dart with a recommendation to use a SettingsProvider. The same approach should be applied here to ensure the UI updates immediately when the setting is toggled. You would then use context.watch<SettingsProvider>().showGoalTrackerEnabled in this if condition.
close #4316