Feat/student discharge #6
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a comprehensive "Student Discharge" feature, including backend API integration, repository, Bloc state management, UI widgets, routing, and localization in both English and Arabic. Additional changes standardize error logging across the codebase by replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant AppRouter
participant DischargePage
participant StudentDischargeBloc
participant StudentDischargeRepositoryImpl
participant DischargeApiClient
User->>Dashboard: Tap "My Discharge" card
Dashboard->>AppRouter: Navigate to discharge route
AppRouter->>DischargePage: Build DischargePage
DischargePage->>StudentDischargeBloc: Dispatch LoadStudentDischarge
StudentDischargeBloc->>StudentDischargeRepositoryImpl: getStudentDischarge()
StudentDischargeRepositoryImpl->>DischargeApiClient: getUuid(), get()
DischargeApiClient-->>StudentDischargeRepositoryImpl: Discharge data or exception
StudentDischargeRepositoryImpl-->>StudentDischargeBloc: StudentDischarge or exception
StudentDischargeBloc-->>DischargePage: Emit state (Loaded, NotRequired, or Error)
DischargePage-->>User: Render UI based on state
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
pubspec.yaml (1)
9-9: Consider widening the Flutter SDK constraint instead of hard-pinning to a single patch versionPinning to the exact version
"3.22.3"prevents consumers (including CI/CD and other contributors) from using later hot-fixes (e.g.,3.22.4) or security releases without editingpubspec.yaml. A more flexible constraint keeps the project on the latest compatible patch while still protecting against breaking major releases.Suggested update:
- flutter: "3.22.3" + flutter: ">=3.22.3 <4.0.0"This mirrors the semver pattern already used for the Dart SDK (
^3.7.2) and follows the recommendation in the official Flutter docs.lib/features/discharge/data/models/dischage.dart (1)
19-20: Document the API field name discrepancy.The comment explaining the API field name discrepancy (
sitBrsvssitBr) is helpful, but this mapping could be fragile if the API changes.Consider adding validation or creating a constants file to manage API field mappings:
// Consider adding constants for API field mappings class ApiFieldConstants { static const String sitBrApi = 'sitBrs'; static const String sitBrLocal = 'sitBr'; }lib/features/discharge/data/services/discharge_api_client.dart (1)
113-115: Potential confusion between sendTimeout and receiveTimeout options.The
sendTimeoutoption is set on the request options, but this typically controls how long to wait when sending data to the server, not receiving. For a GET request, you likely want to control thereceiveTimeout.Consider using only
receiveTimeoutfor GET requests:options: Options( - sendTimeout: _shortTimeout, receiveTimeout: _shortTimeout, ),lib/features/discharge/presentation/widgets/discharge.dart (2)
12-14: Consider using more robust responsive design approach.The current implementation uses a simple width check of 360 pixels, which may not work well across all device types and orientations.
Consider using
LayoutBuilderor predefined breakpoints:- final isSmallScreen = screenSize.width < 360; + final isSmallScreen = screenSize.width < 400 || screenSize.height < 600;Or use a more sophisticated approach with
LayoutBuilderto get available space rather than screen size.
88-136: Consider extracting discharge cards to a separate method or widget.The
_buildDischargeCardsmethod is quite long and repetitive. EachDischargeCardfollows the same pattern with only different properties.Consider using a data-driven approach:
Widget _buildDischargeCards(BuildContext context, bool isSmallScreen) { + final dischargeItems = [ + DischargeItem( + titleKey: 'departmentLevel', + descriptionKey: 'departmentDescription', + isCleared: discharge.sitDep, + icon: Icons.domain_outlined, + ), + // ... other items + ]; + return Column( children: [ + ...dischargeItems.map((item) => Padding( + padding: EdgeInsets.only(bottom: isSmallScreen ? 10 : 12), + child: DischargeCard( + title: GalleryLocalizations.of(context)![item.titleKey], + description: GalleryLocalizations.of(context)![item.descriptionKey], + isCleared: item.isCleared, + icon: item.icon, + isSmallScreen: isSmallScreen, + ), + )).toList(),lib/features/discharge/presentation/bloc/discharge_bloc.dart (1)
87-92: Consider adding loading state for cache clearing.The cache clearing operation doesn't emit any state changes, so users won't get feedback that the operation is in progress or completed.
Consider emitting states for cache clearing:
Future<void> _onClearCache( ClearDischargeCache event, Emitter<StudentDischargeState> emit, ) async { + emit(StudentDischargeLoading()); await cacheService.clearCache(); + emit(StudentDischargeInitial()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/app.dart(2 hunks)lib/config/routes/app_router.dart(4 hunks)lib/core/di/injector.dart(3 hunks)lib/features/dashboard/presentation/widgets/dashboard.dart(1 hunks)lib/features/discharge/data/models/dischage.dart(1 hunks)lib/features/discharge/data/repository/discharge_repository_impl.dart(1 hunks)lib/features/discharge/data/services/discharge_api_client.dart(1 hunks)lib/features/discharge/data/services/discharge_cache_service.dart(1 hunks)lib/features/discharge/presentation/bloc/discharge_bloc.dart(1 hunks)lib/features/discharge/presentation/pages/discharge_page.dart(1 hunks)lib/features/discharge/presentation/widgets/discharge.dart(1 hunks)lib/features/discharge/presentation/widgets/error.dart(1 hunks)lib/l10n/app_ar.arb(1 hunks)lib/l10n/app_en.arb(1 hunks)pubspec.yaml(1 hunks)
🔇 Additional comments (26)
lib/features/dashboard/presentation/widgets/dashboard.dart (1)
139-145: LGTM! Well-integrated discharge feature card.The new discharge grid card follows the established pattern perfectly, using proper localization, appropriate iconography, and correct navigation routing.
lib/app.dart (2)
17-17: LGTM! Proper import addition.The discharge bloc import is correctly placed and follows the existing import organization.
36-36: LGTM! Consistent bloc registration.The StudentDischargeBloc registration follows the established pattern and uses proper dependency injection.
lib/core/di/injector.dart (4)
27-29: LGTM! Proper import organization.The discharge feature imports are correctly placed and follow the existing import structure.
65-65: LGTM! Consistent cache service registration.The DischargeCacheService registration follows the established pattern for cache services.
113-118: LGTM! Proper bloc factory registration.The StudentDischargeBloc factory registration follows the established pattern with proper dependency injection for repository and cache service.
59-59: To confirm the constructor signature, let’s locate the class and inspect its parameters:#!/bin/bash # 1. Find the class definition to see its constructor signature rg "class StudentDischargeRepositoryImpl" -A 5 # 2. Search for any constructors or factory methods to catch alternate formats rg "StudentDischargeRepositoryImpl\s*\(" -A 2lib/config/routes/app_router.dart (4)
6-6: LGTM! Proper page import.The DischargePage import is correctly placed with other page imports.
31-31: LGTM! Consistent route naming.The discharge route name constant follows the established naming convention.
46-46: LGTM! Consistent path naming.The discharge route path constant follows the established naming pattern.
114-118: LGTM! Proper route registration.The discharge GoRoute registration follows the established pattern and is correctly nested under the dashboard routes.
lib/features/discharge/presentation/widgets/error.dart (1)
1-41: LGTM! Well-implemented error handling widget.The ErrorState widget demonstrates good practices:
- Responsive design with screen size adaptation
- Proper error recovery with retry functionality
- Correct bloc event dispatch for reloading data
- Appropriate use of localization
- Clean, centered UI layout
The implementation follows established patterns and provides a good user experience during error scenarios.
lib/features/discharge/data/repository/discharge_repository_impl.dart (2)
4-28: Well-structured repository implementation.The repository follows good architectural patterns with proper dependency injection, clear error handling, and appropriate use of custom exceptions. The logic flow is clean and easy to follow.
30-36: Good custom exception implementation.The
DischargeNotRequiredExceptionis well-designed with a clear message and propertoString()override. This provides good error context for the calling code.lib/features/discharge/presentation/pages/discharge_page.dart (4)
15-24: Good lifecycle and event management.The
initState()method properly triggers the initial data load, and the_loadDischarge()method is clean and focused. Good separation of concerns.
26-32: Well-implemented refresh functionality.The refresh mechanism properly clears cache before reloading, and the simulated delay enhances UX. Good use of async/await pattern.
47-68: Comprehensive state handling.The BlocBuilder properly handles all discharge states with appropriate UI for each case. The RefreshIndicator integration is well-implemented.
71-91: Good responsive design consideration.The responsive text sizing based on screen width shows attention to UX details. The initial state UI is clean and informative.
lib/l10n/app_en.arb (1)
537-621: Comprehensive discharge localization additions.The discharge-related localization entries are well-structured with clear descriptions and proper ARB formatting. All UI elements from the discharge feature appear to be covered with appropriate keys and descriptions.
lib/features/discharge/data/services/discharge_api_client.dart (1)
89-105: Potential null reference exception in offline mode.If
_cacheManagerhasn't been initialized yet (due to the race condition mentioned above), calling_cacheManager.getCache(key)will throw a null reference exception.Let me verify if the cache manager is properly handled throughout the codebase:
#!/bin/bash # Search for CacheManager usage patterns in the codebase rg -A 5 "CacheManager" --type dartlib/features/discharge/presentation/widgets/discharge.dart (2)
38-39: Good use of modern Flutter color API.The use of
withValues(alpha: 0.1)follows the latest Flutter recommendations for color manipulation, replacing the deprecatedwithOpacity.
157-158: Good color coding for status indication.The use of green for cleared and orange for pending provides clear visual feedback to users. This follows good UX practices for status indication.
lib/features/discharge/presentation/bloc/discharge_bloc.dart (2)
80-81: Good specific exception handling.The specific handling of
DischargeNotRequiredExceptionshows good error handling design, providing users with appropriate feedback for different scenarios.
46-57: Well-structured BLoC implementation.The BLoC follows proper patterns with clear separation of events and states. The use of Equatable ensures proper state comparison for UI rebuilds.
lib/l10n/app_ar.arb (2)
556-640: Comprehensive Arabic localization for discharge feature.The added Arabic translations are well-structured and follow the existing pattern with proper metadata descriptions. The translations cover all necessary UI elements including titles, descriptions, status messages, and error states.
The localization keys are consistent with the naming convention and provide complete coverage for the discharge feature UI.
556-559: ```shell
#!/bin/bash
echo "Checking for exact 'myDischarge' entries in English localization..."
grep -n '"myDischarge"' lib/l10n/app_en.arb
grep -n '"@myDischarge"' lib/l10n/app_en.arb</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
lib/features/discharge/data/services/discharge_cache_service.dart
Outdated
Show resolved
Hide resolved
lib/features/discharge/data/services/discharge_cache_service.dart
Outdated
Show resolved
Hide resolved
lib/features/discharge/data/services/discharge_cache_service.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
lib/features/discharge/data/models/discharge.dart (1)
26-35: Consider adding consistency in toJson implementation.The
toJsonmethod returns boolean values directly, while thefromJsonexpects integer values. This asymmetry might cause issues if the JSON data needs to be round-tripped through the API.Consider modifying
toJsonto match the expected API format:Map<String, dynamic> toJson() { return { - 'sitDep': sitDep, - 'sitBf': sitBf, - 'sitBc': sitBc, - 'sitRu': sitRu, - 'sitBrs': sitBrs, + 'sitDep': sitDep ? 1 : 0, + 'sitBf': sitBf ? 1 : 0, + 'sitBc': sitBc ? 1 : 0, + 'sitRu': sitRu ? 1 : 0, + 'sitBrs': sitBrs ? 1 : 0, }; }README.md (1)
46-47: Fix list indentation to maintain consistent formatting.The static analysis tool correctly identified an indentation issue. The list item should align with the other items in the module list.
Apply this fix to correct the indentation:
- - **Student Discharge** + - **Student Discharge**
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
screenshot/discharge.jpgis excluded by!**/*.jpg
📒 Files selected for processing (20)
README.md(2 hunks)lib/core/di/injector.dart(3 hunks)lib/core/network/api_client.dart(1 hunks)lib/features/academics/data/services/academics_cache_service.dart(5 hunks)lib/features/auth/presentation/bloc/auth_bloc.dart(1 hunks)lib/features/discharge/data/models/discharge.dart(1 hunks)lib/features/discharge/data/repository/discharge_repository_impl.dart(1 hunks)lib/features/discharge/presentation/bloc/discharge_bloc.dart(1 hunks)lib/features/discharge/presentation/pages/discharge_page.dart(1 hunks)lib/features/discharge/presentation/widgets/discharge.dart(1 hunks)lib/features/enrollment/data/services/enrollment_cache_service.dart(5 hunks)lib/features/enrollment/presentation/bloc/enrollment_bloc.dart(2 hunks)lib/features/groups/data/services/groups_cache_service.dart(5 hunks)lib/features/profile/data/services/profile_cache_service.dart(5 hunks)lib/features/subject/data/services/subject_cache_service.dart(6 hunks)lib/features/subject/presentation/bloc/subject_bloc.dart(2 hunks)lib/features/timeline/data/models/course_session.dart(1 hunks)lib/features/timeline/data/services/timeline_cache_service.dart(5 hunks)lib/features/transcript/data/services/transcript_cache_service.dart(7 hunks)lib/features/transcript/presentation/bloc/transcript_bloc.dart(6 hunks)
✅ Files skipped from review due to trivial changes (11)
- lib/features/timeline/data/models/course_session.dart
- lib/features/enrollment/presentation/bloc/enrollment_bloc.dart
- lib/features/profile/data/services/profile_cache_service.dart
- lib/features/enrollment/data/services/enrollment_cache_service.dart
- lib/features/timeline/data/services/timeline_cache_service.dart
- lib/features/transcript/data/services/transcript_cache_service.dart
- lib/features/subject/data/services/subject_cache_service.dart
- lib/features/groups/data/services/groups_cache_service.dart
- lib/features/academics/data/services/academics_cache_service.dart
- lib/features/auth/presentation/bloc/auth_bloc.dart
- lib/features/transcript/presentation/bloc/transcript_bloc.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/core/di/injector.dart
- lib/features/discharge/data/repository/discharge_repository_impl.dart
- lib/features/discharge/presentation/pages/discharge_page.dart
- lib/features/discharge/presentation/bloc/discharge_bloc.dart
- lib/features/discharge/presentation/widgets/discharge.dart
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
46-46: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (5)
lib/features/subject/presentation/bloc/subject_bloc.dart (2)
1-1: LGTM! Proper import for debugPrint.Adding the Flutter foundation import to access
debugPrintis the correct approach for improved logging in Flutter applications.
125-125: Excellent improvement to logging practices.Replacing
debugPrintis a Flutter best practice that provides better control over debug output, respects debug mode settings, and handles output throttling appropriately. This aligns well with the broader logging standardization across the codebase mentioned in the summary.lib/features/discharge/data/models/discharge.dart (2)
1-14: Well-structured model class with appropriate defaults.The class structure is clean and follows Dart conventions. The boolean fields with descriptive names and sensible default values provide a clear representation of discharge status.
16-24: Robust JSON deserialization with proper null safety.The
fromJsonfactory constructor correctly handles the conversion from integer values to booleans (1 = true, otherwise false) and includes proper null safety with theint?type annotation.README.md (1)
35-35: Good documentation update with new feature screenshot.The addition of the discharge screen screenshot maintains visual consistency with the existing documentation and helps users understand the new feature.
| void _initializeCacheManager() { | ||
| CacheManager.getInstance().then((value) => _cacheManager = value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for cache manager initialization
The cache manager initialization lacks error handling, which could leave _cacheManager uninitialized if CacheManager.getInstance() fails.
- void _initializeCacheManager() {
- CacheManager.getInstance().then((value) => _cacheManager = value);
+ Future<void> _initializeCacheManager() async {
+ try {
+ _cacheManager = await CacheManager.getInstance();
+ } catch (e) {
+ debugPrint('Failed to initialize cache manager: $e');
+ // Consider providing a fallback or rethrowing
+ rethrow;
+ }
}📝 Committable suggestion
‼️ 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.
| void _initializeCacheManager() { | |
| CacheManager.getInstance().then((value) => _cacheManager = value); | |
| } | |
| Future<void> _initializeCacheManager() async { | |
| try { | |
| _cacheManager = await CacheManager.getInstance(); | |
| } catch (e) { | |
| debugPrint('Failed to initialize cache manager: $e'); | |
| // Consider providing a fallback or rethrowing | |
| rethrow; | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/core/network/api_client.dart around lines 43 to 45, the
_initializeCacheManager method calls CacheManager.getInstance() without handling
potential errors. Modify the method to include error handling for the
asynchronous call, such as using a try-catch block or handling errors in the
then callback, to ensure that any failure in obtaining the cache manager is
caught and handled appropriately, preventing _cacheManager from remaining
uninitialized silently.
| _initializeCacheManager(); | ||
| } |
There was a problem hiding this comment.
Critical: Race condition in cache manager initialization
The asynchronous cache manager initialization in the constructor creates a race condition. Since _initializeCacheManager() is not awaited, the constructor completes before _cacheManager is initialized, potentially causing runtime exceptions when methods like get() are called immediately after object construction.
Consider these solutions:
Option 1: Factory constructor pattern
- ApiClient({FlutterSecureStorage? secureStorage})
- : _secureStorage = secureStorage ?? const FlutterSecureStorage() {
+ ApiClient._({FlutterSecureStorage? secureStorage})
+ : _secureStorage = secureStorage ?? const FlutterSecureStorage() {
_dio = Dio(/* ... */);
// ... other initialization
- _initializeCacheManager();
+ }
+
+ static Future<ApiClient> create({FlutterSecureStorage? secureStorage}) async {
+ final client = ApiClient._(secureStorage: secureStorage);
+ await client._initializeCacheManager();
+ return client;
}Option 2: Make initialization method async and handle properly
- void _initializeCacheManager() {
+ Future<void> _initializeCacheManager() async {
- CacheManager.getInstance().then((value) => _cacheManager = value);
+ _cacheManager = await CacheManager.getInstance();
}🤖 Prompt for AI Agents
In lib/core/network/api_client.dart around lines 40 to 41, the call to
_initializeCacheManager() is asynchronous but not awaited, causing a race
condition where _cacheManager may be uninitialized when used. To fix this,
refactor the class to use a factory constructor that performs async
initialization and returns a fully initialized instance, or make the
initialization method async and ensure callers await the completion before using
the instance. This guarantees _cacheManager is ready before any method calls.
Description
allow student to review his discharge status
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
printstatements withdebugPrintfor better debugging and consistency.Documentation