feat(academic year): add academic year selection feature #9
Conversation
WalkthroughAdds an academic year selection feature: a YearSelectionPage, YearSelectionService persisting chosen year, year-scoped profile caching, modifications to profile loading and student repository to respect selected year, settings UI to pick year, minor routing import, and logout clearing of the saved year. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router as App Router
participant Auth as AuthBloc
participant YearSvc as YearSelectionService
participant YearPage as YearSelectionPage
participant EnrollRepo as EnrollmentRepo
participant ProfileCache as ProfileCacheService
participant ProfileRepo as StudentRepositoryImpl
User->>Router: navigate app
Router->>Auth: query auth state
Auth-->>Router: authenticated
Router->>YearSvc: hasSelectedYear()?
alt no selected year
Router->>YearPage: navigate to YearSelectionPage
YearPage->>EnrollRepo: fetch user enrollments
EnrollRepo-->>YearPage: enrollments
User->>YearPage: select year + confirm
YearPage->>YearSvc: saveSelectedYear(id, code)
YearSvc-->>YearPage: saved
YearPage->>ProfileCache: clearCache(yearId) / clearCache()
YearPage->>Router: restart/redirect to app
else selected year exists
Router->>ProfileRepo: getCurrentAcademicYear()
ProfileRepo->>YearSvc: getSelectedYearId() (may be used)
ProfileRepo-->>Router: academic year
Router->>ProfileCache: getCachedProfileData(yearId)
alt cache hit
ProfileCache-->>Router: cached profile
else cache miss
Router->>ProfileRepo: fetch profile/enrollments live
ProfileRepo-->>Router: profile data
Router->>ProfileCache: cacheProfileData(data, yearId)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/profile/presentation/bloc/profile_bloc.dart (1)
119-148: State emission order causes UI flicker.
ProfileLoadingis emitted at line 148 afterProfileLoadedis emitted from cache (lines 136-145). This causes the UI to:
- Show cached data (ProfileLoaded)
- Immediately show loading spinner (ProfileLoading)
- Show fresh data (ProfileLoaded again)
This results in a visual flicker. Consider restructuring to emit
ProfileLoadingfirst, or skip emitting it when cache is available since you're refreshing in the background anyway.🔎 Proposed fix - Option A: Emit loading first, then cache
+ emit(ProfileLoading()); + // Try to load cached profile for this specific year final cachedProfileData = await cacheService.getCachedProfileData( academicYear.id, ); if (cachedProfileData != null) { // ... emit ProfileLoaded from cache } - - emit(ProfileLoading());🔎 Proposed fix - Option B: Skip loading state when cache exists (background refresh)
if (cachedProfileData != null) { // ... existing cache logic emit( ProfileLoaded( // ... ), ); - } - - emit(ProfileLoading()); + } else { + emit(ProfileLoading()); + }
🧹 Nitpick comments (5)
lib/features/settings/presentation/pages/settings_page.dart (1)
20-35: Consider dependency injection for YearSelectionService.The service initialization and state management logic is correct. However, consider whether
YearSelectionServiceshould be a singleton or injected via dependency injection for better testability and consistency with other services in the codebase.lib/core/services/year_selection_service.dart (1)
3-33: Consider singleton pattern for shared state consistency.The service implementation is correct, but since it manages global user preference state via
SharedPreferences, consider implementing it as a singleton to prevent potential inconsistencies from multiple instances and reduce unnecessarySharedPreferences.getInstance()calls.🔎 Example singleton implementation
class YearSelectionService { + static final YearSelectionService _instance = YearSelectionService._internal(); + factory YearSelectionService() => _instance; + YearSelectionService._internal(); + static const String _selectedYearIdKey = 'selected_academic_year_id'; static const String _selectedYearCodeKey = 'selected_academic_year_code';lib/features/settings/presentation/pages/year_selection_page.dart (1)
83-85: Consider alternatives to full app restart.Calling
Restart.restartApp()is a heavy operation. Consider whether you can achieve the same effect by:
- Clearing relevant caches and resetting navigation state
- Using a state management solution to propagate the year change
- Only reloading affected screens
This would provide a smoother user experience.
lib/config/routes/app_router.dart (1)
64-93: RepeatedYearSelectionServiceinstantiation on every redirect.A new
YearSelectionService()instance is created on lines 81 and 88 for every navigation event. SinceYearSelectionServicelikely interacts withSharedPreferences, this adds unnecessary overhead.Consider caching the service instance or making it a singleton:
🔎 Proposed fix - Cache the service
class AppRouter { // Route names as static constants static const String splash = 'splash'; // ... + + // Cached year selection service + static final YearSelectionService _yearSelectionService = YearSelectionService(); // ... - redirect: (context, state) async { + redirect: (context, state) async { // ... // Authenticated but on login page - check year selection if (authState is AuthSuccess && isLoginRoute) { - final yearService = YearSelectionService(); - final hasSelectedYear = await yearService.hasSelectedYear(); + final hasSelectedYear = await _yearSelectionService.hasSelectedYear(); return hasSelectedYear ? dashboardPath : yearSelectionPath; } // Authenticated - check if year is selected for protected routes if (authState is AuthSuccess && !isYearSelectionRoute) { - final yearService = YearSelectionService(); - final hasSelectedYear = await yearService.hasSelectedYear(); + final hasSelectedYear = await _yearSelectionService.hasSelectedYear(); if (!hasSelectedYear) { return yearSelectionPath; } }lib/features/profile/data/services/profile_cache_service.dart (1)
65-89: Consider handling_currentYearKeywhen clearing a specific year.The
clearCachemethod has two modes: clearing a specific year (lines 69-73) or clearing all profile caches (lines 74-82). When clearing a specific year, if that year happens to be the current year tracked in_currentYearKey, the key becomes stale—it points to data that no longer exists in cache.While this is a minor edge case (the stale reference will self-correct on the next cache operation), consider either:
- Checking if
yearId == _currentYearKeyand clearing_currentYearKeywhen clearing that specific year, or- Documenting this behavior if it's intentional
🔎 Optional fix to maintain consistency
if (yearId != null) { // Clear specific year cache await prefs.remove(_getProfileKey(yearId)); await prefs.remove(_getLastUpdatedKey(yearId)); + // Clear current year reference if we're clearing the current year + final currentYear = prefs.getInt(_currentYearKey); + if (currentYear == yearId) { + await prefs.remove(_currentYearKey); + } } else {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
lib/config/routes/app_router.dart(5 hunks)lib/core/services/year_selection_service.dart(1 hunks)lib/features/auth/presentation/bloc/auth_bloc.dart(2 hunks)lib/features/profile/data/models/academic_year.dart(1 hunks)lib/features/profile/data/models/student_detailed_info.dart(1 hunks)lib/features/profile/data/repositories/student_repository_impl.dart(2 hunks)lib/features/profile/data/services/profile_cache_service.dart(3 hunks)lib/features/profile/presentation/bloc/profile_bloc.dart(4 hunks)lib/features/profile/presentation/pages/profile_page.dart(3 hunks)lib/features/settings/presentation/pages/settings_page.dart(2 hunks)lib/features/settings/presentation/pages/year_selection_page.dart(1 hunks)lib/l10n/app_ar.arb(1 hunks)lib/l10n/app_en.arb(1 hunks)lib/l10n/app_localizations.dart(1 hunks)lib/l10n/app_localizations_ar.dart(1 hunks)lib/l10n/app_localizations_en.dart(1 hunks)
🔇 Additional comments (16)
lib/features/auth/presentation/bloc/auth_bloc.dart (1)
84-90: LGTM!Year selection clearing is properly integrated into the logout flow with consistent error handling that matches the existing cache clearing patterns.
lib/l10n/app_ar.arb (1)
738-757: LGTM!The new localization keys are properly formatted with descriptions and consistent with the existing pattern in the file.
lib/features/profile/presentation/pages/profile_page.dart (2)
52-68: LGTM!The enrollment selection logic properly filters by the current academic year and handles the case where no matching enrollment exists with a graceful catch block.
163-233: Good fallback handling.The conditional rendering properly displays enrollment data when available and falls back to existing profile data when no matching enrollment is found, ensuring the UI remains functional in all scenarios.
lib/features/settings/presentation/pages/settings_page.dart (1)
118-149: LGTM!The year selection UI integration properly displays the current selection and reloads the state after navigation returns, ensuring the display stays synchronized.
lib/l10n/app_localizations_ar.dart (1)
551-565: LGTM!The new localization getters are properly implemented and consistent with the existing pattern in the generated file.
lib/features/profile/data/repositories/student_repository_impl.dart (2)
13-17: LGTM - Constructor with optional dependency injection.The constructor pattern allows for proper dependency injection while providing sensible defaults.
35-42: LGTM - Manual year selection takes priority.The logic correctly prioritizes a manually selected year over automatic detection, enabling the year selection feature.
lib/l10n/app_localizations.dart (1)
1101-1130: LGTM - New localization getters for year selection.The five new getters (
selectYearDescription,pleaseSelectYear,noEnrollmentsFound,errorLoadingData,notSelected) are properly defined with documentation and align with the year selection feature requirements.lib/features/profile/presentation/bloc/profile_bloc.dart (2)
186-197: LGTM - Year-scoped cache storage.The cache now correctly stores profile data keyed by the academic year ID, enabling proper cache separation for different years.
209-250: LGTM - Robust error handling with cache fallback.The nested try-catch properly handles the case where fetching the academic year might also fail during error recovery. The fallback to cached data provides good offline resilience.
lib/config/routes/app_router.dart (1)
108-112: LGTM - Year selection route definition.The new route is correctly defined outside the
ShellRoute, ensuring users must complete year selection before accessing the main navigation shell.lib/l10n/app_en.arb (1)
719-739: LGTM - New localization entries for year selection.The five new ARB keys are properly structured with descriptions and provide appropriate English translations for the year selection feature.
lib/l10n/app_localizations_en.dart (1)
556-570: LGTM! Clean localization additions.The new localization strings are well-formed, grammatically correct, and follow the existing pattern in the file. They appropriately support the year selection feature.
lib/features/profile/data/services/profile_cache_service.dart (2)
7-62: Well-structured year-scoped caching implementation.The refactoring to year-scoped cache keys is clean and well-implemented. The approach of using key prefixes with yearId suffixes provides good namespace isolation, and storing
_currentYearKey(line 27) enables tracking the most recently cached year. Error handling and null safety are properly maintained throughout.Note: This is a breaking change to the cache structure—existing cached profile data (from pre-year-scoped versions) will become inaccessible after upgrade. Since this is cache data (ephemeral by nature), this is acceptable, though users will experience a brief reload after upgrading.
3-3: Version 2.5.3 is current and has no known security advisories.The shared_preferences package version 2.5.3 is the latest release. No known CVEs or active security advisories were found for this package version.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/config/routes/app_router.dart (1)
63-81: Add redirect logic to enforce academic year selection after login.The PR includes a
YearSelectionServiceand route, but the redirect function inapp_router.dart(lines 63-81) does not enforce year selection after successful authentication. AfterAuthSuccess, users should be redirected to/year-selectionif they haven't selected an academic year (checked viaYearSelectionService.getSelectedYearId()), ensuring they complete this step before accessing protected routes like the dashboard.
♻️ Duplicate comments (2)
lib/features/settings/presentation/pages/year_selection_page.dart (2)
18-22: Direct instantiation still breaks dependency injection.The issue flagged in previous reviews remains: creating new instances of
YearSelectionService,ProfileCacheService, andEnrollmentRepositoryImplwithApiClient()bypasses dependency injection, which can cause problems with shared state, authentication context, and testability.
66-86: App restart still lacks user confirmation.The issue flagged in previous reviews remains: the app restarts immediately after saving the year selection without asking for user confirmation, which can surprise users and cause them to lose unsaved context.
🧹 Nitpick comments (2)
lib/features/profile/data/models/academic_year.dart (1)
5-5: Consider making the constructorconstfor immutable instances.Since all fields are
finaland there are no side effects, the constructor can be marked asconstto enable compile-time constant instantiation and improve performance.🔎 Suggested enhancement
-AcademicYear({required this.id, required this.code}); +const AcademicYear({required this.id, required this.code});Additionally, consider overriding
==,hashCode, andtoStringfor better debugging and object comparison (useful when storing in Sets or using as Map keys):@override bool operator ==(Object other) => identical(this, other) || other is AcademicYear && runtimeType == other.runtimeType && id == other.id && code == other.code; @override int get hashCode => id.hashCode ^ code.hashCode; @override String toString() => 'AcademicYear(id: $id, code: $code)';lib/features/profile/data/repositories/student_repository_impl.dart (1)
33-97: Consider extracting helper methods for improved readability.The
getCurrentAcademicYearmethod now handles two distinct flows (manual selection and automatic determination) across ~60 lines. While functionally correct, extracting helper methods could improve maintainability.Potential extractions:
_getManuallySelectedYear()for lines 35-43_computeMaxEnrollmentYear(enrollments)for lines 64-77_determineAndPersistYear(currentYear, enrollments)for the fallback logicThis would make the main method read more like a high-level workflow.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/config/routes/app_router.dart(3 hunks)lib/features/profile/data/models/academic_year.dart(1 hunks)lib/features/profile/data/models/student_detailed_info.dart(1 hunks)lib/features/profile/data/repositories/student_repository_impl.dart(2 hunks)lib/features/settings/presentation/pages/settings_page.dart(2 hunks)lib/features/settings/presentation/pages/year_selection_page.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/profile/data/models/student_detailed_info.dart
- lib/features/settings/presentation/pages/settings_page.dart
🔇 Additional comments (10)
lib/features/profile/data/models/academic_year.dart (1)
11-13: LGTM! The copyWith bug has been fixed.The method now correctly preserves existing values when parameters are not provided. The nullable parameters and null-coalescing operators ensure that calling
copyWith(id: newId)will keep the existingcodevalue intact.lib/features/settings/presentation/pages/year_selection_page.dart (2)
35-64: Well-implemented enrollment loading.The method properly handles loading states, sorts enrollments by most recent first, pre-selects the current year if available, and includes comprehensive error handling with retry capability.
88-274: Solid UI implementation with good UX.The build method comprehensively handles all states (loading, error, empty, success) with appropriate UI feedback. The responsive design with small-screen adaptations, proper localization, accessible radio buttons, and disabled confirm button when no selection is made all contribute to a polished user experience.
lib/config/routes/app_router.dart (3)
18-18: LGTM!Import correctly added for the new YearSelectionPage.
25-25: LGTM!Route name and path constants follow the existing naming conventions.
Also applies to: 43-43
93-97: Clarify route protection and layout intent.The
yearSelectionroute is outside theShellRoute, which is by design:
- It won't have the
MainShellwrapper (navigation bars, persistent UI)- However, it IS protected by authentication via the global
redirectfunction in GoRouter (lines 63-81), which redirects unauthenticated users to login for all routes except splash and loginQuestions to verify:
- Is the full-screen layout (without MainShell) intentional for a better UX during year selection?
- If year selection is meant for post-login configuration, the current protection is correct. If it's for pre-login enrollment/discovery, you may need to exclude it from the auth redirect.
lib/features/profile/data/repositories/student_repository_impl.dart (4)
2-3: LGTM! Clean dependency injection.The addition of
YearSelectionServiceas a dependency is well-structured. The constructor follows the existing pattern with nullable parameters and sensible defaults, maintaining backward compatibility.Also applies to: 11-11, 13-17
45-62: LGTM! Automatic flow is well-structured.The logic to fetch UUID, enrollments, and current academic year is clear and includes appropriate null checks with informative error messages.
64-77: Nice fix! Empty enrollments edge case is now handled.The guard at lines 68-70 properly addresses the concern raised in the previous review. By returning
currentAcademicYearwhen enrollments are empty, the code avoids creating an invalid year with id=0.
79-93: LGTM! Fallback and persistence logic is sound.The logic correctly handles graduated students by falling back to the maximum enrollment year when the current academic year exceeds it. Persisting the determined year via
YearSelectionServiceensures consistent behavior across subsequent calls.
Description
Types of changes
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.