5 logic gamification competitive leaderboard - #18
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a history page feature for tracking user scan activities, along with several code quality improvements including updating deprecated opacity APIs throughout the codebase. The PR adds new backend infrastructure (repository and controller) for real-time scan history streaming, updates the navigation to include a History tab, and improves weekly points tracking logic with reset functionality.
Changes:
- Added History screen with real-time scan tracking and today's scan count display
- Implemented backend support with HistoryRepository, HistoryController, and updates to ScanRepository for weekly reset logic
- Replaced deprecated
withOpacity()API calls withwithValues(alpha:)across multiple UI files - Fixed division-by-zero bug in analytics screen and improved type safety in analytics controller
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pubspec.yaml | Added intl package dependency for date formatting |
| pubspec.lock | Updated lock file with intl package details |
| lib/features/home/ui/text.txt | Contains sensitive test data that should be removed |
| lib/features/home/ui/history_screen.dart | New screen displaying user's scan history with real-time updates |
| lib/features/home/repositories/history_repository.dart | New repository for streaming scan history from Firestore |
| lib/features/home/controllers/history_controller.dart | New controller managing history state and today's scan count logic |
| lib/features/home/repositories/scan_repository.dart | Added weekly reset logic for leaderboard points tracking |
| lib/features/home/models/user_model.dart | Added lastScanWeekId field for tracking weekly resets |
| lib/features/home/ui/home_screen.dart | Added History navigation destination, contains dead logout code |
| lib/features/home/ui/scoreboard_screen.dart | Updated opacity API calls to use withValues |
| lib/features/home/ui/template_screen.dart | Updated opacity API calls to use withValues |
| lib/features/home/ui/camera_screen.dart | Updated opacity API calls to use withValues |
| lib/features/home/ui/analytics_screen.dart | Fixed division-by-zero bug, updated opacity API calls |
| lib/features/home/controllers/analytics_controller.dart | Improved type safety with explicit type conversions |
| lib/features/auth/ui/welcome_screen.dart | Updated opacity API calls to use withValues |
| lib/features/auth/ui/register_screen.dart | Updated opacity API calls to use withValues |
| lib/core/widgets/smart_result_modal.dart | Updated opacity API calls to use withValues |
| lib/core/widgets/impact_row.dart | Updated opacity API calls to use withValues |
Comments suppressed due to low confidence (3)
lib/features/home/ui/text.txt:289
- This file contains what appears to be debug/test data including a real email address and personal information (nabil.0413@gmail.com, "Syed Nabil"). This sensitive data should not be committed to the repository. Additionally, this appears to be sample Firestore document data that should either be removed or placed in a proper documentation or test fixtures location.
lib/features/home/ui/text.txt:280 - The dates in this test/debug data (February 19, 2026 and February 19, 2026) are in the future. While this may be intentional for testing, it's worth noting that future dates could cause issues with date-based logic or comparisons if this data is used anywhere in the application.
lib/features/home/ui/home_screen.dart:56 - Dead code: The comment says "Logout index = 5" but the check is for index == 6. Since there are only 6 navigation destinations (indices 0-5), this condition will never be true. The logout functionality has been moved to the Profile screen, so this entire block (lines 27-56) can be removed.
// Logout index = 5
if (index == 6) {
final confirm = await showDialog<bool>(
context: context,
builder: (context) => AlertDialog(
title: const Text('Logout'),
content: const Text('Are you sure you want to logout?'),
actions: [
TextButton(
onPressed: () => Navigator.pop(context, false),
child: const Text('Cancel'),
),
ElevatedButton(
onPressed: () => Navigator.pop(context, true),
child: const Text('Logout'),
),
],
),
);
if (confirm == true) {
await ref.read(authProvider.notifier).logout();
Navigator.of(context).pushNamedAndRemoveUntil(
'/login',
(route) => false,
);
}
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,270 @@ | |||
| import 'package:flutter/material.dart'; | |||
There was a problem hiding this comment.
The PR title "5 logic gamification competitive leaderboard" doesn't match the actual changes which primarily implement a history page backend and frontend. The title should be updated to better reflect the actual changes, such as "Add history page with scan tracking" or similar.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| try { | ||
| // Fetch the current user data to check the last recorded week | ||
| final userDoc = await userRef.get(); | ||
|
|
||
| WriteBatch batch = _firestore.batch(); | ||
|
|
||
| // 1. Save scan details | ||
| batch.set(scanRef, scan.toMap()); | ||
| // 1. Save individual scan details | ||
| batch.set(scanRef, scan.toMap()); | ||
|
|
||
| // 2. Update user's aggregate stats and check week reset logic | ||
| // Inside ScanRepository.saveScan() | ||
| batch.update(userRef, { | ||
| 'categoryCounts.${scan.category}': FieldValue.increment(1), // Updates the specific category | ||
| 'totalScans': FieldValue.increment(1), | ||
| 'ecoPoints': FieldValue.increment(scan.pointsEarned), | ||
| 'co2Offset': FieldValue.increment(scan.co2Saved), | ||
| }); | ||
| // 2. Weekly Reset Logic | ||
| // We check if the 'lastScanWeekId' in Firestore matches the scan's current 'weekId' | ||
| bool isNewWeek = true; | ||
| if (userDoc.exists) { | ||
| final data = userDoc.data() as Map<String, dynamic>; | ||
| final String lastWeekId = data['lastScanWeekId'] ?? ""; | ||
|
|
||
| // If the stored week ID matches the current scan's week ID, it's NOT a new week | ||
| if (lastWeekId == scan.weekId) { | ||
| isNewWeek = false; | ||
| } | ||
| } | ||
|
|
||
| await batch.commit(); | ||
| // 3. Update user's aggregate stats | ||
| if (isNewWeek) { | ||
| // RESET: Start weekly points fresh for the new week | ||
| batch.update(userRef, { | ||
| 'categoryCounts.${scan.category}': FieldValue.increment(1), | ||
| 'totalScans': FieldValue.increment(1), | ||
| 'ecoPoints': FieldValue.increment(scan.pointsEarned), | ||
| 'weeklyPoints': scan.pointsEarned, // Set to current scan points (Overwrite old week) | ||
| 'co2Offset': FieldValue.increment(scan.co2Saved), | ||
| 'lastScanWeekId': scan.weekId, // Update the tracking week ID | ||
| }); | ||
| } else { | ||
| // INCREMENT: Add to existing weekly points | ||
| batch.update(userRef, { | ||
| 'categoryCounts.${scan.category}': FieldValue.increment(1), | ||
| 'totalScans': FieldValue.increment(1), | ||
| 'ecoPoints': FieldValue.increment(scan.pointsEarned), | ||
| 'weeklyPoints': FieldValue.increment(scan.pointsEarned), // Correctly updating the scoreboard | ||
| 'co2Offset': FieldValue.increment(scan.co2Saved), | ||
| 'lastScanWeekId': scan.weekId, | ||
| }); | ||
| } | ||
|
|
||
| // Commit all changes at once | ||
| await batch.commit(); |
There was a problem hiding this comment.
Potential race condition: The code performs a read operation (userRef.get()) followed by a batch update. If two scans are submitted nearly simultaneously in a new week, both could read the old lastScanWeekId, both determine it's a new week, and both could reset weeklyPoints instead of one resetting and one incrementing. Consider using a Firestore transaction instead of a batch operation to ensure atomic read-modify-write.
| final Map<String, dynamic> rawData = Map<String, dynamic>.from(data); | ||
| return Map<String, double>.from(rawData.map((key, value) => MapEntry(key, (value as num).toDouble()))); |
There was a problem hiding this comment.
Unnecessary type conversion: The code converts data to Map<String, dynamic> and then converts back. The intermediate rawData variable adds complexity without clear benefit. Consider simplifying to: return Map<String, double>.from(data.map((key, value) => MapEntry(key, (value as num).toDouble())));
| final Map<String, dynamic> rawData = Map<String, dynamic>.from(data); | |
| return Map<String, double>.from(rawData.map((key, value) => MapEntry(key, (value as num).toDouble()))); | |
| return Map<String, double>.from( | |
| data.map((key, value) => MapEntry(key, (value as num).toDouble())), | |
| ); |
Clarify PR title mismatch feedback is a metadata change, not a code fix
…//github.com/SNFASA/EcoScan into 5-logic-gamification-competitive-leaderboard
SNFASA
left a comment
SNFASA
left a comment
There was a problem hiding this comment.
tinggal full testing and masukkan API key Google Maps (android/ios)
| android:name="flutterEmbedding" | ||
| android:value="2" /> | ||
| <meta-data android:name="com.google.android.geo.API_KEY" | ||
| android:value="YOUR_ANDROID_API_KEY_HERE"/> |
There was a problem hiding this comment.
@Shaam143 nanti letak API Google maps ( Android)
| _ application: UIApplication, | ||
| didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? | ||
| ) -> Bool { | ||
| GMSServices.provideAPIKey("YOUR_iOS_API_KEY_HERE") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 37 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (1)
lib/features/home/ui/text.txt:216
- This file contains what looks like a Firestore export/debug dump including personal data (email, name) and timestamps inside the
lib/tree. This is sensitive data and should not be committed; remove it from the repo (or move sanitized example data into documentation/tests if needed).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data: (centers) => centers.map((c) => Marker( | ||
| markerId: MarkerId(c.name), | ||
| position: LatLng(c.location!.latitude, c.location!.longitude), | ||
| infoWindow: InfoWindow(title: c.name, snippet: c.type), | ||
| )).toSet(), |
There was a problem hiding this comment.
location is nullable in RecyclingCenterModel, but the marker builder force-unwraps it (c.location!). If any center doc is missing location, this will throw during map render. Filter out centers with null locations or provide a fallback before creating LatLng.
| data: (centers) => centers.map((c) => Marker( | |
| markerId: MarkerId(c.name), | |
| position: LatLng(c.location!.latitude, c.location!.longitude), | |
| infoWindow: InfoWindow(title: c.name, snippet: c.type), | |
| )).toSet(), | |
| data: (centers) => centers | |
| .where((c) => c.location != null) | |
| .map((c) => Marker( | |
| markerId: MarkerId(c.name), | |
| position: LatLng(c.location!.latitude, c.location!.longitude), | |
| infoWindow: InfoWindow(title: c.name, snippet: c.type), | |
| )) | |
| .toSet(), |
| if (!serviceEnabled) return Future.error('Location services are disabled.'); | ||
|
|
||
| LocationPermission permission = await Geolocator.checkPermission(); | ||
| if (permission == LocationPermission.denied) { | ||
| permission = await Geolocator.requestPermission(); | ||
| } |
There was a problem hiding this comment.
getCurrentLocation() requests permission only when denied but doesn't handle deniedForever or the case where the user still denies after requesting. As written, it will call getCurrentPosition() even without permission, which can throw. Add explicit handling for deniedForever and re-check permission after the request.
| if (!serviceEnabled) return Future.error('Location services are disabled.'); | |
| LocationPermission permission = await Geolocator.checkPermission(); | |
| if (permission == LocationPermission.denied) { | |
| permission = await Geolocator.requestPermission(); | |
| } | |
| if (!serviceEnabled) { | |
| return Future.error('Location services are disabled.'); | |
| } | |
| LocationPermission permission = await Geolocator.checkPermission(); | |
| if (permission == LocationPermission.denied) { | |
| permission = await Geolocator.requestPermission(); | |
| } | |
| if (permission == LocationPermission.denied) { | |
| return Future.error('Location permissions are denied'); | |
| } | |
| if (permission == LocationPermission.deniedForever) { | |
| return Future.error( | |
| 'Location permissions are permanently denied, we cannot request permissions.', | |
| ); | |
| } |
| android:name="flutterEmbedding" | ||
| android:value="2" /> | ||
| <meta-data android:name="com.google.android.geo.API_KEY" | ||
| android:value="YOUR_ANDROID_API_KEY_HERE"/> |
There was a problem hiding this comment.
This commits a Google Maps API key placeholder into the manifest. API keys should be provided via build config/CI secrets (e.g., manifest placeholder, gradle resValue, or injected string resource) and not stored directly in source-controlled XML.
| android:value="YOUR_ANDROID_API_KEY_HERE"/> | |
| android:value="${MAPS_API_KEY}"/> |
| Future<void> changePassword({required String currentPassword, required String newPassword}) async { | ||
| final user = FirebaseAuth.instance.currentUser; | ||
| if (user == null || user.email == null) throw "User not found"; | ||
|
|
||
| // 1. Re-authenticate the user (Required by Firebase for security) | ||
| AuthCredential credential = EmailAuthProvider.credential( | ||
| email: user.email!, | ||
| password: currentPassword, | ||
| ); | ||
|
|
||
| await user.reauthenticateWithCredential(credential); | ||
|
|
||
| // 2. Update to new password | ||
| await user.updatePassword(newPassword); | ||
| } | ||
| } |
There was a problem hiding this comment.
This password-change method bypasses the injected auth instance by using FirebaseAuth.instance, which makes the class harder to test and inconsistent with the rest of the repository. It also throws a raw string and has inconsistent formatting. Prefer using the injected auth, throwing an Exception type, and aligning formatting with the rest of the file.
| Future<void> changePassword({required String currentPassword, required String newPassword}) async { | |
| final user = FirebaseAuth.instance.currentUser; | |
| if (user == null || user.email == null) throw "User not found"; | |
| // 1. Re-authenticate the user (Required by Firebase for security) | |
| AuthCredential credential = EmailAuthProvider.credential( | |
| email: user.email!, | |
| password: currentPassword, | |
| ); | |
| await user.reauthenticateWithCredential(credential); | |
| // 2. Update to new password | |
| await user.updatePassword(newPassword); | |
| } | |
| } | |
| Future<void> changePassword({ | |
| required String currentPassword, | |
| required String newPassword, | |
| }) async { | |
| final user = auth.currentUser; | |
| if (user == null || user.email == null) { | |
| throw Exception('User not found'); | |
| } | |
| // 1. Re-authenticate the user (Required by Firebase for security) | |
| final AuthCredential credential = EmailAuthProvider.credential( | |
| email: user.email!, | |
| password: currentPassword, | |
| ); | |
| await user.reauthenticateWithCredential(credential); | |
| // 2. Update to new password | |
| await user.updatePassword(newPassword); | |
| } | |
| } |
| import 'package:google_maps_flutter/google_maps_flutter.dart'; | ||
| import 'package:url_launcher/url_launcher.dart'; // Add this for Directions | ||
| import '../controllers/centers_controller.dart'; | ||
| import '../models/recycleingcenter_model.dart'; |
There was a problem hiding this comment.
The model file/import name recycleingcenter_model.dart appears misspelled ("recycleing"). Consider renaming to recycling_center_model.dart (and updating imports) to avoid propagating the typo across the codebase and to improve discoverability.
| import '../models/recycleingcenter_model.dart'; | |
| import '../models/recycling_center_model.dart'; |
| onPressed: () => _openDirections(center.location!.latitude, center.location!.longitude), | ||
| ), | ||
| onTap: () => controller.animateToCenter(center.location!.latitude, center.location!.longitude), |
There was a problem hiding this comment.
These callbacks force-unwrap center.location! (directions and onTap). Since location is nullable, this can crash when a center lacks coordinates. Consider disabling the directions button / tap handler (or showing an error) when location == null.
| onPressed: () => _openDirections(center.location!.latitude, center.location!.longitude), | |
| ), | |
| onTap: () => controller.animateToCenter(center.location!.latitude, center.location!.longitude), | |
| onPressed: () { | |
| final location = center.location; | |
| if (location == null) { | |
| debugPrint('Center "${center.name}" has no location; cannot open directions.'); | |
| return; | |
| } | |
| _openDirections(location.latitude, location.longitude); | |
| }, | |
| ), | |
| onTap: () { | |
| final location = center.location; | |
| if (location == null) { | |
| debugPrint('Center "${center.name}" has no location; cannot animate map to center.'); | |
| return; | |
| } | |
| controller.animateToCenter(location.latitude, location.longitude); | |
| }, |
| void setFilter(String category) { | ||
| selectedCategory = category; | ||
| ref.invalidateSelf(); // Refresh the stream and UI | ||
| } |
There was a problem hiding this comment.
Calling ref.invalidateSelf() in setFilter restarts the Firestore stream subscription every time the user changes the chip filter, which is unnecessary work and can increase reads/flicker. Prefer keeping the centers stream stable and storing selectedCategory in separate state (e.g., a StateProvider) or in the provider state, then filter the already-streamed list.
| _ application: UIApplication, | ||
| didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? | ||
| ) -> Bool { | ||
| GMSServices.provideAPIKey("YOUR_iOS_API_KEY_HERE") |
There was a problem hiding this comment.
Hardcoding an API key placeholder in source code is risky and will also break maps unless manually edited per developer. Load the Google Maps API key from build configuration (e.g., Info.plist / xcconfig) and keep it out of version control; avoid committing placeholders in the app delegate.
| GMSServices.provideAPIKey("YOUR_iOS_API_KEY_HERE") | |
| if let apiKey = Bundle.main.object(forInfoDictionaryKey: "GOOGLE_MAPS_API_KEY") as? String, | |
| !apiKey.isEmpty { | |
| GMSServices.provideAPIKey(apiKey) | |
| } |
| try { | ||
| // Fetch the current user data to check the last recorded week | ||
| final userDoc = await userRef.get(); | ||
|
|
||
| WriteBatch batch = _firestore.batch(); | ||
|
|
There was a problem hiding this comment.
Weekly reset logic reads userRef.get() and then applies a batch update based on that snapshot. If multiple scans are saved concurrently (or from multiple devices), both calls can see the same lastScanWeekId and overwrite weeklyPoints, losing increments. Use a Firestore transaction (or other atomic server-side mechanism) so the week check + update happens atomically.
| import 'package:flutter/material.dart'; | ||
| import 'package:flutter_riverpod/flutter_riverpod.dart'; | ||
| import 'package:intl/intl.dart'; | ||
| import 'package:firebase_auth/firebase_auth.dart'; | ||
| import '../controllers/history_controller.dart'; |
There was a problem hiding this comment.
PR title ("5 logic gamification competitive leaderboard") doesn’t match the actual changes, which add a History screen, Recycling Centers map, and profile editing/password change flows. Consider renaming the PR to reflect the history/centers features so it’s easier to track in release notes and future git history.
History page backend and frontend