feat(logging): add structured logger + Crashlytics integration - #35
feat(logging): add structured logger + Crashlytics integration#35Melsaeed276 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a centralized structured logging facility (AppLogger) and wires global error capture + Firebase Crashlytics reporting into app bootstrap, with additional auth-flow instrumentation and supporting test/dependency updates.
Changes:
- Introduce
AppLogger(structured levels, context, Crashlytics breadcrumbs/error reporting) and a unit test for pre-initialization fallback behavior - Wire global error capture in
bootstrapApp()(FlutterError.onError,PlatformDispatcher.instance.onError,runZonedGuarded) - Update auth pages to emit success/failure logs; update timezone service contract + fakes to support coordinate-based timezone resolution
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| project_code/lib/app/logging/app_logger.dart | New centralized logger + Crashlytics integration and fallback behavior |
| project_code/lib/app/bootstrap.dart | Initializes logger and installs global error handlers / zone guard |
| project_code/lib/features/auth/presentation/login_page.dart | Adds logging around phone/Google login flows |
| project_code/lib/features/auth/presentation/register_page.dart | Adds logging around phone/Google registration flows |
| project_code/lib/features/prayer_times/location/domain/location_services.dart | Extends TimezoneService API with coordinate-based resolver |
| project_code/lib/features/prayer_times/location/data/flutter_timezone_service.dart | Implements new TimezoneService method (currently a stub) |
| project_code/test/app/logging/app_logger_test.dart | New unit test covering logger safety before initialization |
| project_code/test/prayer_times/location/location_setup_page_test.dart | Updates fake timezone service to satisfy new interface |
| project_code/test/prayer_times/location/location_setup_controller_test.dart | Updates fake timezone service to satisfy new interface |
| project_code/test/design/design_golden_test.dart | Removes unused app import (keeps design preview golden setup) |
| project_code/test/design/design_accessibility_test.dart | Removes unused app import (keeps semantics/RTL checks) |
| project_code/pubspec.yaml | Adds logger and firebase_crashlytics dependencies |
| project_code/pubspec.lock | Locks new dependency versions and transitives |
| project_code/macos/Flutter/GeneratedPluginRegistrant.swift | Registers Crashlytics plugin for macOS |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (crashlyticsEnabled) { | ||
| try { | ||
| await FirebaseCrashlytics.instance.setCrashlyticsCollectionEnabled( | ||
| true, | ||
| ); | ||
| enabled = true; | ||
| } catch (_) { | ||
| enabled = false; | ||
| } |
There was a problem hiding this comment.
initialize() only calls setCrashlyticsCollectionEnabled(true) when crashlyticsEnabled is true, but never explicitly disables collection when it’s false. Since Crashlytics collection can persist between runs, debug/test sessions may still upload reports. Consider always calling setCrashlyticsCollectionEnabled(crashlyticsEnabled) (or explicitly setting false in the else branch) to guarantee the intended behavior.
| if (crashlyticsEnabled) { | |
| try { | |
| await FirebaseCrashlytics.instance.setCrashlyticsCollectionEnabled( | |
| true, | |
| ); | |
| enabled = true; | |
| } catch (_) { | |
| enabled = false; | |
| } | |
| try { | |
| await FirebaseCrashlytics.instance.setCrashlyticsCollectionEnabled( | |
| crashlyticsEnabled, | |
| ); | |
| enabled = crashlyticsEnabled; | |
| } catch (_) { | |
| enabled = false; |
| Future<String> resolveTimezoneForCoordinates({ | ||
| required double latitude, | ||
| required double longitude, | ||
| }) { | ||
| return resolveLocalTimezone(); |
There was a problem hiding this comment.
resolveTimezoneForCoordinates ignores latitude/longitude and just returns the device’s local timezone. This is misleading given the API name and will produce incorrect timezones when the user manually selects a location in a different timezone. Either implement a coordinate-based lookup (or rename/remove this API if not supported) so manual locations get the correct timezone.
| setState(() { | ||
| _error = 'Login failed unexpectedly.'; | ||
| }); |
There was a problem hiding this comment.
This setState runs in the generic catch path without checking mounted. If the page is disposed while the async call is running, this can throw at runtime. Mirror the mounted guard used elsewhere before calling setState in this error path.
| setState(() { | ||
| _error = error.message; | ||
| }); |
There was a problem hiding this comment.
This setState runs in the AuthFailure catch path without checking mounted. If the widget is disposed while the Google login is in-flight, this can throw setState() called after dispose(). Add a mounted guard before calling setState here.
| setState(() { | ||
| _error = 'Google login failed unexpectedly.'; | ||
| }); |
There was a problem hiding this comment.
This setState runs in the generic catch path without checking mounted. If the widget is disposed before the Future completes, this can throw at runtime. Add a mounted guard before calling setState here.
| fatal: true, | ||
| ), | ||
| ); | ||
| return true; |
There was a problem hiding this comment.
PlatformDispatcher.instance.onError currently returns true unconditionally, which signals the error was handled and can suppress default error reporting/printing. Consider returning false in debug (or delegating to any previous handler) so developers still see uncaught errors, while returning true only when you explicitly want to swallow them.
| setState(() { | ||
| _error = error.message; | ||
| }); |
There was a problem hiding this comment.
This setState runs in the AuthFailure catch path without checking mounted. If the widget is disposed while the login request is in-flight, this can throw setState() called after dispose(). Add a if (!mounted) return; guard (or wrap the setState in a if (mounted) check) before mutating state in error paths too.
| AppLogger.instance.warning( | ||
| 'Register with phone failed', | ||
| context: <String, Object?>{'code': error.code.name}, | ||
| ); |
There was a problem hiding this comment.
This setState runs in the AuthFailure catch path without checking mounted. If the widget is disposed while the registration request is in-flight, this can throw setState() called after dispose(). Add a mounted guard before calling setState in this error path.
| ); | |
| ); | |
| if (!mounted) { | |
| return; | |
| } |
| setState(() { | ||
| _error = error.message; | ||
| }); |
There was a problem hiding this comment.
This setState runs in the AuthFailure catch path without checking mounted. If the widget is disposed while the Google registration is in-flight, this can throw setState() called after dispose(). Add a mounted guard before calling setState here.
| 'Unexpected Google registration failure', | ||
| error: error, | ||
| stackTrace: stackTrace, | ||
| ); |
There was a problem hiding this comment.
This setState runs in the generic catch path without checking mounted. If the widget is disposed before the Future completes, this can throw at runtime. Add a mounted guard before calling setState here.
| ); | |
| ); | |
| if (!mounted) { | |
| return; | |
| } |
Summary
AppLogger) with structured levels and context supportFlutterError.onErrorPlatformDispatcher.instance.onErrorrunZonedGuarded)Testing
flutter analyzeflutter testflutter test --tags goldendart run tool/design_guard.dart --changed-only --base-ref origin/renewDesign Compliance
Notes