Refactor user and sheet providers for improved login and data filtering.#165
Refactor user and sheet providers for improved login and data filtering.#165kumarpalsinh25 merged 1 commit intomainfrom
Conversation
- Set a default user ('user_1') on first launch to ensure a logged-in state.
- Refactor `loggedInUser` and `currentUser` providers to be more robust and rely on other providers.
- Reinstate sheet filtering logic to only display sheets the current user is a member of.
- Convert `HomeScreen` from a `StatefulWidget` to a `ConsumerWidget` and remove unnecessary state management.
WalkthroughHomeScreen is refactored from stateful to stateless ConsumerWidget, removing PreferencesService dependency. The sheetList provider now filters sheets by current user membership. User authentication providers are updated to source data from loggedInUserProvider with fallback defaults instead of direct PreferencesService queries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/features/users/providers/user_providers.dart (1)
47-55: Use provider pattern for consistency with other parts of the codebase.The hardcoded default user
'user_1'exists in the initialuserListdata and is properly defined. However, the code directly instantiatesPreferencesService()instead of using the established provider pattern. While this is functionally safe (PreferencesService is a singleton), other providers in the codebase use the provider pattern (e.g.,task_providers.dart,theme_provider.dart). For consistency and testability, inject viaref.read(preferencesServiceProvider):@riverpod Future<String?> loggedInUser(Ref ref) async { final defaultUser = 'user_1'; final prefsService = ref.read(preferencesServiceProvider); final userId = await prefsService.getLoginUserId(); if (userId == null || userId.isEmpty) { await prefsService.setLoginUserId(defaultUser); } return userId ?? defaultUser; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/home/screens/home_screen.dart(6 hunks)lib/features/sheet/providers/sheet_providers.dart(1 hunks)lib/features/users/providers/user_providers.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: format
- GitHub Check: Test zoe_native plugin
- GitHub Check: test
- GitHub Check: windows
- GitHub Check: macos
- GitHub Check: linux
- GitHub Check: ios
- GitHub Check: android
🔇 Additional comments (3)
lib/features/users/providers/user_providers.dart (1)
59-63: LGTM! Clean async provider pattern.The
currentUserprovider correctly usesloggedInUserProvider.futureto await the user ID and resolves the user throughgetUserByIdProvider. This pattern properly handles the async nature of the login state.lib/features/home/screens/home_screen.dart (1)
25-39: LGTM! Clean refactoring to stateless widget.The refactoring from
ConsumerStatefulWidgettoConsumerWidgetis well-executed:
- Class signature correctly updated
- Build method properly accepts
WidgetRef ref- Helper methods updated to receive
contextandrefas parameters- Removed state management and
PreferencesServicedependency in favor of providersThis aligns well with the provider-based architecture changes in the PR.
lib/features/sheet/providers/sheet_providers.dart (1)
82-90: Review comment is substantively valid but overstates localization—this is a systemic pattern, not an isolated issue.The
.valueaccess pattern you identified exists across the entire codebase in all synchronous providers (tasksList,linksList,pollsList,eventsList,documentsList, etc.), not justsheetsList. When any of these providers load, list contents will indeed be empty.However, the referenced
user_providers.dartpatterns at lines 41 and 60 use.futurebecause they are async providers—sheetsListcannot adopt that approach without becoming async itself. SincesheetsListis synchronous, it must choose between.value(current approach, with loading gaps) or handling fullAsyncValue<String?>states directly.The design consistency suggests this loading behavior may be intentional. Verify whether empty lists during user ID resolution are acceptable in your UX, or if sheets should conditionally display previous state during loading. If refactoring is desired, it would need to address all affected list providers comprehensively.
Summary by CodeRabbit
Bug Fixes
Improvements