refactor: introduce reusable SettingsGroup widget to reduce duplication in settings page - #642
Open
chetanr25 wants to merge 1 commit into
Open
refactor: introduce reusable SettingsGroup widget to reduce duplication in settings page#642chetanr25 wants to merge 1 commit into
chetanr25 wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
…on in settings page
Copy link
Copy Markdown
📝 Walkthrough
WalkthroughThis refactoring consolidates duplicated settings UI logic by introducing a reusable SettingsGroup widget with a sealed SettingsItemConfig hierarchy (SettingsToggleItem, SettingsDropdownItem<T>, SettingsCustomItem). Five single-purpose trailing widget files are removed, their logic absorbed into the new abstraction. The settings page body is restructured to use SettingsGroup, and a nativeName getter is added to SupportedLanguage for centralized language display names. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
Copy link
Copy Markdown
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)lib/app/modules/settings/views/settings_page_body.dart (1)🤖 Prompt for all review comments with AI agentslib/app/modules/settings/views/settings_page_delete_tasks_tile.dart (1)140-147: Redundant SentenceManager instantiation.
The sentences variable is already defined at line 55-57, but this code creates new SentenceManager instances. Use the existing sentences variable for consistency and to avoid unnecessary object creation.
♻️ Proposed fix🤖 Prompt for AI AgentsSettingsCustomItem( child: SettingsPageListTile( - title: SentenceManager( - currentLanguage: AppSettings.selectedLanguage) - .sentences - .logs, - subTitle: SentenceManager( - currentLanguage: AppSettings.selectedLanguage) - .sentences - .checkAllDebugLogsHere, + title: sentences.logs, + subTitle: sentences.checkAllDebugLogsHere, trailing: IconButton(Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/settings/views/settings_page_body.dart` around lines 140 - 147, The title and subTitle are creating new SentenceManager instances unnecessarily; instead reuse the existing sentences variable declared earlier (sentences) when setting title to sentences.logs and subTitle to sentences.checkAllDebugLogsHere so you avoid redundant SentenceManager(...) construction and ensure consistent language selection tied to the already-initialized sentences object.lib/app/modules/settings/views/settings_group.dart (2)16-19: FutureBuilder future is recreated on every rebuild.
Calling _getFlag() directly in the future: parameter means a new Future is created on each widget rebuild. This triggers redundant SharedPreferences reads and can cause unnecessary UI flickering.
Consider caching the future or moving the preference check to a higher level (e.g., the controller).
♻️ Proposed fix: cache the future in a late final or use a stateful approach🤖 Prompt for AI Agents-class SettingsPageDeleteTasksTile extends StatelessWidget { +class SettingsPageDeleteTasksTile extends StatefulWidget { final SettingsController controller; - const SettingsPageDeleteTasksTile({required this.controller, super.key}); + const SettingsPageDeleteTasksTile({required this.controller, super.key}); + + `@override` + State<SettingsPageDeleteTasksTile> createState() => + _SettingsPageDeleteTasksTileState(); +} - Future<bool> _getFlag() async { - final prefs = await SharedPreferences.getInstance(); - return prefs.getBool("settings_taskc") ?? false; - } +class _SettingsPageDeleteTasksTileState + extends State<SettingsPageDeleteTasksTile> { + late final Future<bool> _flagFuture = _getFlag(); + + Future<bool> _getFlag() async { + final prefs = await SharedPreferences.getInstance(); + return prefs.getBool("settings_taskc") ?? false; + } `@override` Widget build(BuildContext context) { ... return FutureBuilder<bool>( - future: _getFlag(), + future: _flagFuture,Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/settings/views/settings_page_delete_tasks_tile.dart` around lines 16 - 19, The Future returned by _getFlag() is being recreated on every rebuild because it's called directly in the FutureBuilder's future parameter; cache the Future (or move the preference load to the controller) so the SharedPreferences read happens once—e.g., create a late final Future<bool> _flagFuture = _getFlag() or invoke _getFlag() in initState and store the resulting Future/boolean in a state field, then pass that cached Future/field to the FutureBuilder instead of calling _getFlag() inline.78-80: Dropdown color uses static theme check instead of theme extension.
The dropdownColor is determined by AppSettings.isDarkMode statically, while the rest of the widget uses tColors from Theme.of(context). This creates inconsistency and the dropdown won't update if the theme changes while the settings page is open.
♻️ Proposed fix: use theme extension consistently🤖 Prompt for AI Agents- dropdownColor: AppSettings.isDarkMode - ? TaskWarriorColors.kprimaryBackgroundColor - : TaskWarriorColors.kLightPrimaryBackgroundColor, + dropdownColor: tColors.primaryBackgroundColor,Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/settings/views/settings_group.dart` around lines 78 - 80, The dropdownColor is using the static AppSettings.isDarkMode check causing inconsistency with the rest of the widget which uses tColors from Theme.of(context); replace the ternary that references AppSettings.isDarkMode with the theme extension value(s) from tColors (e.g., use tColors.kprimaryBackgroundColor or tColors.kLightPrimaryBackgroundColor as appropriate) so dropdownColor is derived from the Theme.of(context) extension (tColors) and will update when the theme changes; update the dropdownColor expression where it currently references AppSettings.isDarkMode to use tColors instead.
33-40: Async toggle method errors may be silently swallowed.
The toggle method is async but is passed directly to Switch.onChanged (line 185), which expects a synchronous callback. While Dart allows this, any exceptions from the SharedPreferences operation will be silently swallowed since the returned Future is not awaited or error-handled by the caller.
Consider wrapping the async operations with error handling or logging failures.
🛡️ Proposed fix: add error handling🤖 Prompt for AI AgentsFuture<void> toggle(bool v) async { value.value = v; if (prefsKey != null) { + try { final prefs = await SharedPreferences.getInstance(); await prefs.setBool(prefsKey!, v); + } catch (e) { + debugPrint('Failed to persist $prefsKey: $e'); + } } onChanged?.call(v); }Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/settings/views/settings_group.dart` around lines 33 - 40, The toggle(bool v) method can throw from the async SharedPreferences calls but is passed directly to Switch.onChanged (which expects a sync callback), so errors may be silently swallowed; update toggle to catch and handle/log any exceptions around the SharedPreferences work (wrap the await SharedPreferences.getInstance() / prefs.setBool(...) in try/catch and call a logger or debugPrint on error) or alternatively keep toggle async but change the Switch.onChanged usage to a synchronous closure that calls toggle(v).catchError(...) and handles/logs failures; reference the toggle method and the Switch.onChanged usage to locate where to add the try/catch or the synchronous wrapper.
Verify each finding against the current code and only fix it if needed. Inline comments: In `@lib/app/modules/settings/views/settings_page_delete_tasks_tile.dart`: - Around line 64-68: The onPressed handler calls the async method controller.deleteAllTasksInDB() but does not await it, so the confirmation dialog is closed immediately; change the handler to await controller.deleteAllTasksInDB(), show progress/disable the button while awaiting (or show a loading indicator via state/Controller), handle errors from deleteAllTasksInDB (log or show a SnackBar/error dialog) and only call Navigator.of(context).pop() after the awaited operation completes successfully (or pop with an error result if it failed) so users get feedback and the operation isn't cancelled by the UI closing. In `@lib/app/utils/language/supported_language.dart`: - Around line 29-30: The case for SupportedLanguage.german returns the English name "German" instead of the native name; update the return value in the SupportedLanguage.german branch to "Deutsch" so it follows the same native-name convention used by the other cases (locate the switch/case handling SupportedLanguage in supported_language.dart and change the return for SupportedLanguage.german). --- Nitpick comments: In `@lib/app/modules/settings/views/settings_group.dart`: - Around line 78-80: The dropdownColor is using the static AppSettings.isDarkMode check causing inconsistency with the rest of the widget which uses tColors from Theme.of(context); replace the ternary that references AppSettings.isDarkMode with the theme extension value(s) from tColors (e.g., use tColors.kprimaryBackgroundColor or tColors.kLightPrimaryBackgroundColor as appropriate) so dropdownColor is derived from the Theme.of(context) extension (tColors) and will update when the theme changes; update the dropdownColor expression where it currently references AppSettings.isDarkMode to use tColors instead. - Around line 33-40: The toggle(bool v) method can throw from the async SharedPreferences calls but is passed directly to Switch.onChanged (which expects a sync callback), so errors may be silently swallowed; update toggle to catch and handle/log any exceptions around the SharedPreferences work (wrap the await SharedPreferences.getInstance() / prefs.setBool(...) in try/catch and call a logger or debugPrint on error) or alternatively keep toggle async but change the Switch.onChanged usage to a synchronous closure that calls toggle(v).catchError(...) and handles/logs failures; reference the toggle method and the Switch.onChanged usage to locate where to add the try/catch or the synchronous wrapper. In `@lib/app/modules/settings/views/settings_page_body.dart`: - Around line 140-147: The title and subTitle are creating new SentenceManager instances unnecessarily; instead reuse the existing sentences variable declared earlier (sentences) when setting title to sentences.logs and subTitle to sentences.checkAllDebugLogsHere so you avoid redundant SentenceManager(...) construction and ensure consistent language selection tied to the already-initialized sentences object. In `@lib/app/modules/settings/views/settings_page_delete_tasks_tile.dart`: - Around line 16-19: The Future returned by _getFlag() is being recreated on every rebuild because it's called directly in the FutureBuilder's future parameter; cache the Future (or move the preference load to the controller) so the SharedPreferences read happens once—e.g., create a late final Future<bool> _flagFuture = _getFlag() or invoke _getFlag() in initState and store the resulting Future/boolean in a state field, then pass that cached Future/field to the FutureBuilder instead of calling _getFlag() inline.
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info ⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b216a4a2-be34-46ba-a510-b39b65dfa973
📥 CommitsReviewing files that changed from the base of the PR and between f058b4a and 158a28a.
📒 Files selected for processing (9)- lib/app/modules/settings/views/settings_group.dart
- lib/app/modules/settings/views/settings_page_body.dart
- lib/app/modules/settings/views/settings_page_delete_tasks_tile.dart
- lib/app/modules/settings/views/settings_page_enable_24hr_format_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_highlist_task_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_on_task_create_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_on_task_start_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_select_the_language_trailing.dart
- lib/app/utils/language/supported_language.dart
- lib/app/modules/settings/views/settings_page_on_task_create_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_enable_24hr_format_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_highlist_task_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_on_task_start_list_tile_trailing.dart
- lib/app/modules/settings/views/settings_page_select_the_language_trailing.dart
Sorry, something went wrong.
Comment on lines
+64
to
+68
| TextButton( | ||
| onPressed: () { | ||
| controller.deleteAllTasksInDB(); | ||
| Navigator.of(context).pop(); | ||
| }, |
Copy link
Copy Markdown
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Async delete operation is not awaited.
controller.deleteAllTasksInDB() is an async method, but the dialog dismisses immediately without waiting for completion. Users receive no feedback if the operation fails or takes time.
🛠️ Proposed fix: await the deletion and consider feedback TextButton(
- onPressed: () {
- controller.deleteAllTasksInDB();
+ onPressed: () async {
+ await controller.deleteAllTasksInDB();
Navigator.of(context).pop();
},‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| TextButton( | |
| onPressed: () { | |
| controller.deleteAllTasksInDB(); | |
| Navigator.of(context).pop(); | |
| }, | |
| TextButton( | |
| onPressed: () async { | |
| await controller.deleteAllTasksInDB(); | |
| Navigator.of(context).pop(); | |
| }, |
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/settings/views/settings_page_delete_tasks_tile.dart` around lines 64 - 68, The onPressed handler calls the async method controller.deleteAllTasksInDB() but does not await it, so the confirmation dialog is closed immediately; change the handler to await controller.deleteAllTasksInDB(), show progress/disable the button while awaiting (or show a loading indicator via state/Controller), handle errors from deleteAllTasksInDB (log or show a SnackBar/error dialog) and only call Navigator.of(context).pop() after the awaited operation completes successfully (or pop with an error result if it failed) so users get feedback and the operation isn't cancelled by the UI closing.
Sorry, something went wrong.
Comment thread
lib/app/utils/language/supported_language.dart
Comment on lines
+29
to
+30
| case SupportedLanguage.german: | ||
| return 'German'; |
Copy link
Copy Markdown
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Inconsistent native name for German.
All other languages use their native script/name (e.g., हिन्दी, Français, Español), but German returns 'German' instead of 'Deutsch'.
✏️ Proposed fix case SupportedLanguage.german:
- return 'German';
+ return 'Deutsch';‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case SupportedLanguage.german: | |
| return 'German'; | |
| case SupportedLanguage.german: | |
| return 'Deutsch'; |
Verify each finding against the current code and only fix it if needed. In `@lib/app/utils/language/supported_language.dart` around lines 29 - 30, The case for SupportedLanguage.german returns the English name "German" instead of the native name; update the return value in the SupportedLanguage.german branch to "Deutsch" so it follows the same native-name convention used by the other cases (locate the switch/case handling SupportedLanguage in supported_language.dart and change the return for SupportedLanguage.german).
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
At least 1 approving review is required to merge this pull request.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactors the settings page to use a declarative SettingsGroup widget backed by a sealed SettingsItemConfig class with three variants: SettingsToggleItem, SettingsDropdownItem<T>, and SettingsCustomItem. This replaces the repeated _buildSectionHeader/_buildSettingsCard pattern and eliminates 5 near-identical trailing widget files that each only wrapped a Switch or DropdownButton.
The grouping model means each settings section is now expressed as data (title, icon, list of items) rather than manually assembled widget trees making it immediately readable what settings exist and what type they are. Toggles accept an optional prefsKey that handles SharedPreferences persistence internally, so the common case needs zero callback boilerplate. An optional onChanged is still available for toggles with extra side effects.
Language display names were centralized into a nativeName getter on the SupportedLanguage enum, so Dart's exhaustive switch enforces updates when a new language is added no more hardcoded mappings scattered across files. The delete-tasks confirmation dialog was extracted into its own SettingsPageDeleteTasksTile widget to keep the body file focused on structure. End result: settings_page_body.dart drops from 381 to 169 lines, and adding a new setting is now a matter of appending one item to a list.
Fixes
Fixes: #641
Summary by CodeRabbit
New Features
Refactor