feat: add Firebase telemetry tracking for android client#1067
Open
DELTA-45-G wants to merge 11 commits into
Open
feat: add Firebase telemetry tracking for android client#1067DELTA-45-G wants to merge 11 commits into
DELTA-45-G wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Firebase-backed telemetry (core types, service, manager), collects device info, tracks screens/navigation, instruments login and network flows, updates app startup to initialize Firebase and telemetry, modernizes Android build to API 34, updates dependencies/gitignore, and adjusts widget tests. ChangesTelemetry System Implementation
Sequence Diagram(s)sequenceDiagram
participant AppStart as App Startup
participant Firebase as Firebase
participant TelemetryService as TelemetryService
participant TelemetryManager as TelemetryManager
participant DeviceInfo as Device Info
participant FirebaseAnalytics as Firebase Analytics
AppStart->>Firebase: Firebase.initializeApp()
AppStart->>TelemetryService: TelemetryService.init()
TelemetryService->>DeviceInfo: collect device metadata
TelemetryService->>TelemetryManager: logEvent(app_start_time)
TelemetryManager->>FirebaseAnalytics: logEvent(app_start_time)
AppStart->>TelemetryManager: TelemetryManager.initialize()
TelemetryManager->>FirebaseAnalytics: setAnalyticsCollectionEnabled(telemetry_enabled)
AppStart->>AppStart: Register TelemetryNavigationObserver
sequenceDiagram
participant User as User
participant LoginPage as LoginPage
participant AuthProvider as AuthProvider
participant TelemetryService as TelemetryService
participant FirebaseAnalytics as Firebase Analytics
User->>LoginPage: Clicks login
LoginPage->>TelemetryService: trackEvent(login_clicked)
LoginPage->>TelemetryService: trackEvent(login_attempt, {has_username})
LoginPage->>AuthProvider: Authenticate
alt Authentication succeeds
AuthProvider-->>LoginPage: Success
LoginPage->>TelemetryService: trackEvent(login_success)
LoginPage->>FirebaseAnalytics: Navigate
else Authentication fails
AuthProvider-->>LoginPage: Error
LoginPage->>TelemetryService: trackEvent(login_failed, {error})
TelemetryService->>FirebaseAnalytics: logEvent(login_failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 (2)
lib/platform_android/auth_service_impl.dart (1)
115-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix incorrect failure log message in
getAutoLogoutPopupTimeout().Line 120 logs
"getIdleTime call failed!", which does not match the method and makes debugging noisy.Suggested fix
- } on PlatformException { - debugPrint('getIdleTime call failed!'); + } on PlatformException { + debugPrint('getAutoLogoutPopupTimeout call failed!');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/platform_android/auth_service_impl.dart` around lines 115 - 121, Update the failure log in getAutoLogoutPopupTimeout(): when catching PlatformException (and/or general errors) replace the incorrect message "getIdleTime call failed!" with a message that accurately references getAutoLogoutPopupTimeout (e.g., "getAutoLogoutPopupTimeout call failed!") so logs match the method; locate the method Future<String> getAutoLogoutPopupTimeout() and the PlatformException catch block where debugPrint is called and change the string accordingly to reflect the correct function name (and do the same for other catch branches if they use the same wrong message).lib/ui/login_page.dart (1)
353-362:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmit
login_successfor all successful login flows.
login_successis only tracked in the non-auto-sync branch, so successful logins that trigger_autoSyncHandler()are missed in telemetry.Suggested fix
- if (authProvider.isLoggedIn && - (syncProvider.lastSuccessfulSyncTime == "LastSyncTimeIsNull")) { + TelemetryService.trackEvent("login_success"); + if (authProvider.isLoggedIn && + (syncProvider.lastSuccessfulSyncTime == "LastSyncTimeIsNull")) { log("sync time: ${syncProvider.lastSuccessfulSyncTime}"); syncProvider.setIsGlobalSyncInProgress(true); await _autoSyncHandler(); } else { authProvider.setIsSyncing(false); - TelemetryService.trackEvent("login_success"); await _navigateToHomePage(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ui/login_page.dart` around lines 353 - 362, The telemetry event "login_success" is only emitted in the else branch and is missed when the code enters the auto-sync branch; update the logic around authProvider.isLoggedIn / syncProvider.lastSuccessfulSyncTime so that TelemetryService.trackEvent("login_success") is called for all successful login flows (either move the call before the if/else or add the call inside the auto-sync branch), and ensure existing behavior (calling _autoSyncHandler(), authProvider.setIsSyncing(false), and _navigateToHomePage()) remains unchanged; reference authProvider.isLoggedIn, syncProvider.lastSuccessfulSyncTime, _autoSyncHandler(), and _navigateToHomePage() when making the change.
🧹 Nitpick comments (3)
lib/telemetry/telemetry_navigation.dart (1)
8-17: ⚡ Quick winTrack only
PageRouteinstances for screen telemetry.Line 8 and Line 13 currently track every route type; that includes dialog/popup routes and can pollute
screen_viewmetrics. Gate tracking toPageRoute.Proposed change
`@override` void didPush(Route route, Route? previousRoute) { - _trackScreen(route); + if (route is PageRoute) { + _trackScreen(route); + } } `@override` void didPop(Route route, Route? previousRoute) { - if (previousRoute != null) { + if (previousRoute is PageRoute) { _trackScreen(previousRoute); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/telemetry/telemetry_navigation.dart` around lines 8 - 17, The telemetry currently tracks every Route in didPush and didPop which can include dialogs/popups; update didPush(Route route, ...) and didPop(Route route, ...) to only call _trackScreen when the route (or previousRoute) is an instance of PageRoute (e.g., use an is PageRoute check or cast before calling _trackScreen). Specifically, guard the calls to _trackScreen in both the didPush and didPop handlers so only PageRoute instances are passed to _trackScreen.android/app/build.gradle (1)
140-324: ⚡ Quick winRemove duplicate dependency declarations.
The
dependenciesblock contains numerous duplicate entries (e.g., Dagger, Room, Retrofit, OkHttp appear 2-3 times). This increases maintenance burden and risks version conflicts.♻️ Consolidate dependencies
Review and deduplicate all dependency declarations. Keep only one declaration per library. For example:
dependencies { def fragment_version = "1.4.1" def room_version = "2.4.0" implementation 'javax.inject:javax.inject:1' implementation 'com.google.dagger:dagger:2.41' implementation 'com.google.dagger:dagger-android-support:2.41' annotationProcessor 'com.google.dagger:dagger-compiler:2.41' annotationProcessor 'com.google.dagger:dagger-android-processor:2.41' - // ... remove duplicate dagger entries below ...Run this script to identify all duplicates:
#!/bin/bash # Find duplicate dependencies in build.gradle echo "Duplicate dependency check:" rg -oP "^\s*(implementation|api|testImplementation|androidTestImplementation|annotationProcessor)\s+['\"]([^'\"]+)" android/app/build.gradle | \ awk '{print $2}' | sort | uniq -d🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/build.gradle` around lines 140 - 324, The dependencies block contains many duplicated entries (e.g., com.google.dagger:dagger:2.41, androidx.room:room-runtime, com.squareup.retrofit2:retrofit:2.9.0, com.auth0.android:jwtdecode:2.0.1, okhttp/okio, junit/mocking libs) which should be consolidated; open the dependencies block and remove repeated declarations so each artifact (identify by its Maven coordinates like com.google.dagger:dagger:2.41, androidx.room:room-runtime:2.4.2, com.squareup.retrofit2:retrofit:2.9.0, com.squareup.okhttp3:okhttp:4.9.2, com.auth0.android:jwtdecode:2.0.1, etc.) appears only once, keep the desired version you want to standardize on, merge any test vs androidTest duplicates appropriately (preserve test-specific scopes), and ensure annotationProcessor entries for dagger/room/lombok remain singular and match the runtime entries.lib/telemetry/telemetry_manager.dart (1)
55-85: ⚡ Quick winRemove the commented legacy implementation block.
The old queue-based implementation in comments is dead code and makes the class harder to maintain.
Proposed cleanup
-// import 'telemetry_event.dart'; -// import 'telemetry_logger.dart'; -... -// }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/telemetry/telemetry_manager.dart` around lines 55 - 85, Remove the entire commented-out legacy queue implementation block for TelemetryManager (the commented imports and the commented class containing TelemetryEvent/_queue/logEvent/addEvent/_flush) — keep only the active implementation files and references; ensure any usages still rely on the real TelemetryManager API (e.g., TelemetryManager.logEvent, TelemetryManager.addEvent) and that TelemetryEvent and TelemetryLogger imports used elsewhere are intact, but delete the dead commented code to clean up the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/app/build.gradle`:
- Line 51: Replace hard-coded SDK version literals by using the Flutter-provided
fallbacks: update the compileSdkVersion and targetSdkVersion occurrences to use
flutter.compileSdkVersion ?: 34 and flutter.targetSdkVersion ?: 34 respectively
(so they defer to Flutter values when present), and ensure the Gradle script
still has the Flutter plugin/config exposing the flutter object; if you
intentionally must pin versions, add a brief comment explaining why the override
is required.
- Around line 161-162: Add README instructions for Firebase setup: explain how
to obtain google-services.json from the Firebase console, instruct developers to
place that file in android/app/ so the build.gradle dependencies
(platform('com.google.firebase:firebase-bom:34.12.0') and
'com.google.firebase:firebase-analytics') can pick it up, and document CI/CD
handling (store the file as a secret in your CI provider and inject it into
android/app/ at build time or use environment-variable based Gradle replacement)
along with a brief note that google-services.json is gitignored and must be
provisioned per-developer or via pipeline.
In `@lib/app_router.dart`:
- Around line 22-55: Routes currently wrap pages with TelemetryScreenWrapper
which duplicates screen events already emitted by TelemetryNavigationObserver;
remove the wrapper and return the page widgets directly in the route map (e.g.,
replace occurrences of TelemetryScreenWrapper(screenName: "...", child: Xyz())
with just Xyz() or const Xyz()) so navigation observer is the single source of
truth for screen tracking; update all routes referencing TelemetryScreenWrapper
(examples: LoginPage.route, OnboardLandingPage.route, HomePage.route and
GenericProcess routes) to return their child widgets unwrapped.
In `@lib/telemetry/telemetry_manager.dart`:
- Around line 9-17: The telemetry flag _isEnabled in telemetry_manager.dart
currently defaults to true, allowing telemetry to be active before
SharedPreferences is loaded; change the default to false (or make _isEnabled
nullable and treat null as disabled) and ensure initialize() reads prefs and
then calls _analytics.setAnalyticsCollectionEnabled(_isEnabled) before any code
can log events; update any telemetry-entry points to check _isEnabled (or await
initialize) so no events are sent prior to consent, referencing the static field
_isEnabled and the initialize() method and the
_analytics.setAnalyticsCollectionEnabled call.
In `@lib/telemetry/telemetry_service.dart`:
- Around line 54-56: The logError method in telemetry_service.dart currently
sends raw exception text by calling error.toString() to trackEvent("error"),
which can leak sensitive data; change logError (and any callers) to send only
non-sensitive identifiers such as error.runtimeType.toString(), a short
non-reversible fingerprint (e.g. a SHA-256 hash) of the full message if you need
deduplication, and optional sanitized/whitelisted fields rather than the raw
message; update the call site to pass a data map like {"type": errorType,
"fingerprint": fingerprint} to trackEvent and remove/error-redact any direct use
of error.toString() while keeping trackEvent usage intact.
In `@lib/ui/machine_keys.dart`:
- Around line 44-47: Replace the hardcoded fallback "Not initialized" with the
localized string from appLocalizations.not_initialized where machineDetails is
set (e.g., inside the map.isEmpty branch and the similar block at lines 54-58);
locate the assignments to machineDetails in machine_keys.dart (the map.isEmpty
check and the other fallback) and use appLocalizations.not_initialized instead
of the literal so the UI uses localization once dependencies are available.
---
Outside diff comments:
In `@lib/platform_android/auth_service_impl.dart`:
- Around line 115-121: Update the failure log in getAutoLogoutPopupTimeout():
when catching PlatformException (and/or general errors) replace the incorrect
message "getIdleTime call failed!" with a message that accurately references
getAutoLogoutPopupTimeout (e.g., "getAutoLogoutPopupTimeout call failed!") so
logs match the method; locate the method Future<String>
getAutoLogoutPopupTimeout() and the PlatformException catch block where
debugPrint is called and change the string accordingly to reflect the correct
function name (and do the same for other catch branches if they use the same
wrong message).
In `@lib/ui/login_page.dart`:
- Around line 353-362: The telemetry event "login_success" is only emitted in
the else branch and is missed when the code enters the auto-sync branch; update
the logic around authProvider.isLoggedIn / syncProvider.lastSuccessfulSyncTime
so that TelemetryService.trackEvent("login_success") is called for all
successful login flows (either move the call before the if/else or add the call
inside the auto-sync branch), and ensure existing behavior (calling
_autoSyncHandler(), authProvider.setIsSyncing(false), and _navigateToHomePage())
remains unchanged; reference authProvider.isLoggedIn,
syncProvider.lastSuccessfulSyncTime, _autoSyncHandler(), and
_navigateToHomePage() when making the change.
---
Nitpick comments:
In `@android/app/build.gradle`:
- Around line 140-324: The dependencies block contains many duplicated entries
(e.g., com.google.dagger:dagger:2.41, androidx.room:room-runtime,
com.squareup.retrofit2:retrofit:2.9.0, com.auth0.android:jwtdecode:2.0.1,
okhttp/okio, junit/mocking libs) which should be consolidated; open the
dependencies block and remove repeated declarations so each artifact (identify
by its Maven coordinates like com.google.dagger:dagger:2.41,
androidx.room:room-runtime:2.4.2, com.squareup.retrofit2:retrofit:2.9.0,
com.squareup.okhttp3:okhttp:4.9.2, com.auth0.android:jwtdecode:2.0.1, etc.)
appears only once, keep the desired version you want to standardize on, merge
any test vs androidTest duplicates appropriately (preserve test-specific
scopes), and ensure annotationProcessor entries for dagger/room/lombok remain
singular and match the runtime entries.
In `@lib/telemetry/telemetry_manager.dart`:
- Around line 55-85: Remove the entire commented-out legacy queue implementation
block for TelemetryManager (the commented imports and the commented class
containing TelemetryEvent/_queue/logEvent/addEvent/_flush) — keep only the
active implementation files and references; ensure any usages still rely on the
real TelemetryManager API (e.g., TelemetryManager.logEvent,
TelemetryManager.addEvent) and that TelemetryEvent and TelemetryLogger imports
used elsewhere are intact, but delete the dead commented code to clean up the
file.
In `@lib/telemetry/telemetry_navigation.dart`:
- Around line 8-17: The telemetry currently tracks every Route in didPush and
didPop which can include dialogs/popups; update didPush(Route route, ...) and
didPop(Route route, ...) to only call _trackScreen when the route (or
previousRoute) is an instance of PageRoute (e.g., use an is PageRoute check or
cast before calling _trackScreen). Specifically, guard the calls to _trackScreen
in both the didPush and didPop handlers so only PageRoute instances are passed
to _trackScreen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 587a9170-b86b-4b76-9b5b-9455f80bbb7e
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.gitignoreandroid/.gitignoreandroid/app/build.gradleandroid/build.gradleandroid/clientmanager/build.gradleandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/jacoco.gradleandroid/keymanager/build.gradleandroid/packetmanager/build.gradleandroid/transliterationmanager/build.gradlelib/app_router.dartlib/main.dartlib/platform_android/auth_service_impl.dartlib/platform_android/network_service_impl.dartlib/provider/auth_provider.dartlib/telemetry/telemetry_config.dartlib/telemetry/telemetry_event.dartlib/telemetry/telemetry_logger.dartlib/telemetry/telemetry_manager.dartlib/telemetry/telemetry_navigation.dartlib/telemetry/telemetry_screen_wrapper.dartlib/telemetry/telemetry_service.dartlib/ui/login_page.dartlib/ui/machine_keys.dartpubspec.yamltest/widget_test.dart
DELTA-45-G
force-pushed
the
feat/719-telemetry-android-client
branch
from
b5719fe to
1931385
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
lib/app_router.dart (1)
22-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a single source of truth for screen tracking.
These route wrappers emit screen telemetry in addition to the navigation observer, causing duplicate
screen_viewevents.Proposed fix (keep observer-based tracking)
- LoginPage.route: (context) => const TelemetryScreenWrapper( - screenName: "LoginPage", - child: LoginPage(), - ), + LoginPage.route: (context) => const LoginPage(), - '/new_process': (context) => const TelemetryScreenWrapper( - screenName: "NewProcess", - child: GenericProcess(processType: ProcessType.newProcess), - ), + '/new_process': (context) => const GenericProcess(processType: ProcessType.newProcess), - '/update_process': (context) => const TelemetryScreenWrapper( - screenName: "UpdateProcess", - child: GenericProcess(processType: ProcessType.updateProcess), - ), + '/update_process': (context) => const GenericProcess(processType: ProcessType.updateProcess), - '/lost_process': (context) => const TelemetryScreenWrapper( - screenName: "LostProcess", - child: GenericProcess(processType: ProcessType.lostProcess), - ), + '/lost_process': (context) => const GenericProcess(processType: ProcessType.lostProcess), - '/correction_process': (context) => const TelemetryScreenWrapper( - screenName: "CorrectionProcess", - child: GenericProcess(processType: ProcessType.correctionProcess), - ), + '/correction_process': (context) => const GenericProcess(processType: ProcessType.correctionProcess), - OnboardLandingPage.route: (context) => const TelemetryScreenWrapper( - screenName: "OnboardLandingPage", - child: OnboardLandingPage(), - ), + OnboardLandingPage.route: (context) => const OnboardLandingPage(), - HomePage.route: (context) => const TelemetryScreenWrapper( - screenName: "HomePage", - child: HomePage(), - ), + HomePage.route: (context) => const HomePage(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/app_router.dart` around lines 22 - 55, The route map currently wraps pages with TelemetryScreenWrapper (e.g., LoginPage.route, OnboardLandingPage.route, HomePage.route and the '/new_process', '/update_process', '/lost_process', '/correction_process' entries) which duplicates screen_view events when you also have a navigation observer; remove the TelemetryScreenWrapper wrappers from these route entries and return the pages directly (e.g., child: LoginPage(), GenericProcess(...), OnboardLandingPage(), HomePage()) so the observer is the single source of truth for screen tracking.lib/telemetry/telemetry_manager.dart (1)
9-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail closed before consent is loaded.
Line 9 defaults telemetry to enabled. Any event emitted before
initialize()completes can bypass persisted consent.Proposed fix
- static bool _isEnabled = true;// This should be loaded from user consent (e.g., SharedPreferences) in initialize() + static bool _isEnabled = false; // fail-closed until persisted consent is loaded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/telemetry/telemetry_manager.dart` around lines 9 - 17, The telemetry flag currently defaults to true causing events to leak before persisted consent is read; change the default of _isEnabled to false (fail-closed) and ensure initialize() calls _analytics.setAnalyticsCollectionEnabled(_isEnabled) immediately after reading prefs so analytics are disabled until consent is confirmed; also add guards in any event-emitting methods (e.g., any trackEvent/record* functions) to check _isEnabled before sending events.lib/telemetry/telemetry_service.dart (1)
54-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid sending raw exception messages in telemetry.
Line 55 forwards
error.toString()directly, which can leak sensitive operational or user data.Proposed mitigation
static void logError(Object error) { - trackEvent("error", data: {"message": error.toString()}); + trackEvent("error", data: { + "type": error.runtimeType.toString(), + "message": "redacted", + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/telemetry/telemetry_service.dart` around lines 54 - 56, The logError method in telemetry_service.dart currently forwards error.toString() to trackEvent("error", data: {"message": ...}), which can leak sensitive data; change logError to avoid sending raw exception messages by sending a generic message (e.g., "Unhandled error") and a non-sensitive fingerprint instead — for example include only the error's runtimeType or an error code plus a hashed fingerprint (SHA256) of error.toString() or stack trace for correlation; update references in logError and the payload passed to trackEvent("error", ...) accordingly so telemetry contains no raw exception text but still allows grouping via the fingerprint.
🧹 Nitpick comments (2)
lib/app_router.dart (1)
57-63: ⚡ Quick winDrop commented-out route code.
Please remove the stale commented mappings to keep the route table maintainable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/app_router.dart` around lines 57 - 63, Remove the stale commented-out route mappings in lib/app_router.dart: delete the lines referencing LoginPage.route, the GenericProcess entries (with ProcessType.newProcess, ProcessType.updateProcess, ProcessType.lostProcess, ProcessType.correctionProcess), OnboardLandingPage.route, and HomePage.route so the route table contains only active mappings and no commented legacy routes.lib/telemetry/telemetry_manager.dart (1)
55-85: ⚡ Quick winRemove the commented-out legacy TelemetryManager block.
Keeping the old implementation commented in production code adds noise and makes future edits riskier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/telemetry/telemetry_manager.dart` around lines 55 - 85, Remove the entire commented-out legacy TelemetryManager block to eliminate dead code and noise; delete the commented imports ('telemetry_event.dart', 'telemetry_logger.dart') and the commented class TelemetryManager including its methods logEvent, addEvent, and _flush along with related TelemetryEvent/TelemetryLogger references so only the active implementation remains in lib/telemetry/telemetry_manager.dart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/main.dart`:
- Line 244: The app is double-counting screen_view events because
TelemetryNavigationObserver() is registered while screens/routes are also
wrapped with telemetry tracking; either choose a single strategy or add
deduplication: remove TelemetryNavigationObserver from navigatorObservers if you
prefer the route wrapper approach, or if keeping TelemetryNavigationObserver
implement dedupe logic (in the TelemetryNavigationObserver class) that ignores
events for the same route/screen within a short timeframe (e.g., track
lastSeenScreen+timestamp and drop duplicates), or add a guard in the route
wrapper to skip emitting if TelemetryNavigationObserver already emitted for that
route; update TelemetryNavigationObserver and/or the route wrapper to use a
shared dedupe key (route name/screen id + timestamp) so screen_view events are
emitted only once.
- Around line 47-48: TelemetryService.init() is being called before
TelemetryManager.initialize(), which risks emitting startup telemetry before
consent/config is applied; swap the calls so TelemetryManager.initialize() runs
first (load/restore consent and configure Firebase), then call
TelemetryService.init(), or alternatively make TelemetryService.init() wait for
TelemetryManager.isInitialized/isConsentLoaded (use the
TelemetryManager.initialize() future) before bootstrapping any startup events.
In `@lib/platform_android/auth_service_impl.dart`:
- Line 116: The variable declaration for refreshLoginTime has an extra
semicolon; locate the declaration "String refreshLoginTime = \"0\";;" in
auth_service_impl.dart (look for the refreshLoginTime symbol) and remove the
duplicate semicolon so it becomes a single-terminated declaration "String
refreshLoginTime = \"0\";".
In `@lib/platform_android/network_service_impl.dart`:
- Around line 31-37: The telemetry calls in the catch block of
checkInternetConnection (TelemetryService.trackEvent and
TelemetryService.logError) must be guarded so failures in telemetry do not
rethrow and break the method's fallback behavior; wrap those telemetry
invocations in their own try/catch (or check TelemetryService availability) and
swallow/log telemetry errors locally so the original network error handling
continues and checkInternetConnection still returns the intended fallback
result.
---
Duplicate comments:
In `@lib/app_router.dart`:
- Around line 22-55: The route map currently wraps pages with
TelemetryScreenWrapper (e.g., LoginPage.route, OnboardLandingPage.route,
HomePage.route and the '/new_process', '/update_process', '/lost_process',
'/correction_process' entries) which duplicates screen_view events when you also
have a navigation observer; remove the TelemetryScreenWrapper wrappers from
these route entries and return the pages directly (e.g., child: LoginPage(),
GenericProcess(...), OnboardLandingPage(), HomePage()) so the observer is the
single source of truth for screen tracking.
In `@lib/telemetry/telemetry_manager.dart`:
- Around line 9-17: The telemetry flag currently defaults to true causing events
to leak before persisted consent is read; change the default of _isEnabled to
false (fail-closed) and ensure initialize() calls
_analytics.setAnalyticsCollectionEnabled(_isEnabled) immediately after reading
prefs so analytics are disabled until consent is confirmed; also add guards in
any event-emitting methods (e.g., any trackEvent/record* functions) to check
_isEnabled before sending events.
In `@lib/telemetry/telemetry_service.dart`:
- Around line 54-56: The logError method in telemetry_service.dart currently
forwards error.toString() to trackEvent("error", data: {"message": ...}), which
can leak sensitive data; change logError to avoid sending raw exception messages
by sending a generic message (e.g., "Unhandled error") and a non-sensitive
fingerprint instead — for example include only the error's runtimeType or an
error code plus a hashed fingerprint (SHA256) of error.toString() or stack trace
for correlation; update references in logError and the payload passed to
trackEvent("error", ...) accordingly so telemetry contains no raw exception text
but still allows grouping via the fingerprint.
---
Nitpick comments:
In `@lib/app_router.dart`:
- Around line 57-63: Remove the stale commented-out route mappings in
lib/app_router.dart: delete the lines referencing LoginPage.route, the
GenericProcess entries (with ProcessType.newProcess, ProcessType.updateProcess,
ProcessType.lostProcess, ProcessType.correctionProcess),
OnboardLandingPage.route, and HomePage.route so the route table contains only
active mappings and no commented legacy routes.
In `@lib/telemetry/telemetry_manager.dart`:
- Around line 55-85: Remove the entire commented-out legacy TelemetryManager
block to eliminate dead code and noise; delete the commented imports
('telemetry_event.dart', 'telemetry_logger.dart') and the commented class
TelemetryManager including its methods logEvent, addEvent, and _flush along with
related TelemetryEvent/TelemetryLogger references so only the active
implementation remains in lib/telemetry/telemetry_manager.dart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 51b0a597-1836-401b-8a4f-fa7a07271e5c
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
.gitignoreandroid/.gitignoreandroid/app/build.gradleandroid/build.gradleandroid/clientmanager/build.gradleandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/jacoco.gradleandroid/keymanager/build.gradleandroid/packetmanager/build.gradleandroid/transliterationmanager/build.gradlelib/app_router.dartlib/main.dartlib/platform_android/auth_service_impl.dartlib/platform_android/network_service_impl.dartlib/provider/auth_provider.dartlib/telemetry/telemetry_config.dartlib/telemetry/telemetry_event.dartlib/telemetry/telemetry_logger.dartlib/telemetry/telemetry_manager.dartlib/telemetry/telemetry_navigation.dartlib/telemetry/telemetry_screen_wrapper.dartlib/telemetry/telemetry_service.dartlib/ui/login_page.dartlib/ui/machine_keys.dartpubspec.yamltest/widget_test.dart
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- android/.gitignore
- android/transliterationmanager/build.gradle
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/build.gradle (1)
140-324:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConsolidate duplicate dependencies and resolve version conflicts.
The dependencies block contains extensive duplication, with many libraries declared 2-3 times identically. More critically, commons-io has a version conflict:
2.14.0at line 199 versus2.11.0at line 304.Examples of duplication:
- Dagger dependencies appear 3 times (lines 144-148, 214-217, 264-267)
- Retrofit appears 3 times (lines 180-181, 242-243, 297-298)
- Test dependencies repeated 3 times each
- Many AndroidX libraries duplicated 2-3 times
This creates maintenance burden and risks inconsistencies. Gradle will use the last declaration, but developers may unknowingly update only one instance.
♻️ Recommended refactoring approach
- Remove all duplicate declarations
- Keep only one instance of each dependency
- Resolve the commons-io version conflict by choosing one version (prefer
2.14.0as it's newer)- Consider grouping related dependencies with comments for clarity
dependencies { def fragment_version = "1.4.1" def room_version = "2.4.0" implementation 'javax.inject:javax.inject:1' implementation 'com.google.dagger:dagger:2.41' implementation 'com.google.dagger:dagger-android-support:2.41' annotationProcessor 'com.google.dagger:dagger-compiler:2.41' annotationProcessor 'com.google.dagger:dagger-android-processor:2.41' // AndroidX libraries implementation 'androidx.appcompat:appcompat:1.4.1' // ... (keep first instance, remove duplicates) - implementation group: 'commons-io', name: 'commons-io', version: '2.11.0' + // Use 2.14.0 consistently (already declared at line 199) - // Remove all duplicate Dagger blocks (lines 214-217, 264-267) - // Remove all duplicate Retrofit blocks (lines 242-243, 297-298) - // Remove all duplicate test dependency blocks - // Remove duplicate AndroidX declarations🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/build.gradle` around lines 140 - 324, The dependencies block contains many duplicate entries (e.g., 'com.google.dagger:dagger', 'com.squareup.retrofit2:retrofit', various androidTest/test libs) and a commons-io version conflict (entries with version '2.14.0' and '2.11.0'); fix by deduplicating so each artifact appears exactly once, consolidate repeated Dagger entries ('com.google.dagger:dagger', 'dagger-android-support', 'dagger-compiler', 'dagger-android-processor'), Retrofit ('com.squareup.retrofit2:retrofit', 'converter-gson'), test libs, AndroidX libs, and choose a single commons-io version (prefer '2.14.0'), then group related dependencies with comments for clarity and remove the redundant repeated blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@android/app/build.gradle`:
- Around line 140-324: The dependencies block contains many duplicate entries
(e.g., 'com.google.dagger:dagger', 'com.squareup.retrofit2:retrofit', various
androidTest/test libs) and a commons-io version conflict (entries with version
'2.14.0' and '2.11.0'); fix by deduplicating so each artifact appears exactly
once, consolidate repeated Dagger entries ('com.google.dagger:dagger',
'dagger-android-support', 'dagger-compiler', 'dagger-android-processor'),
Retrofit ('com.squareup.retrofit2:retrofit', 'converter-gson'), test libs,
AndroidX libs, and choose a single commons-io version (prefer '2.14.0'), then
group related dependencies with comments for clarity and remove the redundant
repeated blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 60eacf46-4dc7-4229-aff3-ade59aa32399
📒 Files selected for processing (1)
android/app/build.gradle
DELTA-45-G
force-pushed
the
feat/719-telemetry-android-client
branch
from
3ed2c84 to
1051228
Compare
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/app_router.dart (1)
15-15: ⚡ Quick winRemove the commented-out code.
The commented-out import and route definitions (lines 15, 22-55) are dead code left over from resolving the duplicate screen tracking issue. Since the fix has been confirmed and the active routes (lines 57-63) are now the single source of truth, this commented code serves no purpose and should be removed. Version control preserves the history if needed.
🧹 Proposed cleanup
-// import 'package:registration_client/telemetry/telemetry_screen_wrapper.dart'; - class AppRouter { AppRouter._(); static Map<String, Widget Function(BuildContext)> routes = { - //telementary screen - // LoginPage.route: (context) => const TelemetryScreenWrapper( - // screenName: "LoginPage", - // child: LoginPage(), - // ), - - // '/new_process': (context) => const TelemetryScreenWrapper( - // screenName: "NewProcess", - // child: GenericProcess(processType: ProcessType.newProcess), - // ), - - // '/update_process': (context) => const TelemetryScreenWrapper( - // screenName: "UpdateProcess", - // child: GenericProcess(processType: ProcessType.updateProcess), - // ), - - // '/lost_process': (context) => const TelemetryScreenWrapper( - // screenName: "LostProcess", - // child: GenericProcess(processType: ProcessType.lostProcess), - // ), - - // '/correction_process': (context) => const TelemetryScreenWrapper( - // screenName: "CorrectionProcess", - // child: GenericProcess(processType: ProcessType.correctionProcess), - // ), - - // OnboardLandingPage.route: (context) => const TelemetryScreenWrapper( - // screenName: "OnboardLandingPage", - // child: OnboardLandingPage(), - // ), - - // HomePage.route: (context) => const TelemetryScreenWrapper( - // screenName: "HomePage", - // child: HomePage(), - // ), - // LoginPage.route: (context) => const LoginPage(),Also applies to: 22-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/app_router.dart` at line 15, Remove the dead commented-out code: delete the commented import "// import 'package:registration_client/telemetry/telemetry_screen_wrapper.dart';" and the entire commented route block (the stale commented route definitions between the earlier duplicate tracking fix and the active routes) so only the active routes remain (the single-source-of-truth block around lines with the current routes). Ensure no other comments referencing telemetry_screen_wrapper or the old route names remain in lib/app_router.dart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/app_router.dart`:
- Line 15: Remove the dead commented-out code: delete the commented import "//
import 'package:registration_client/telemetry/telemetry_screen_wrapper.dart';"
and the entire commented route block (the stale commented route definitions
between the earlier duplicate tracking fix and the active routes) so only the
active routes remain (the single-source-of-truth block around lines with the
current routes). Ensure no other comments referencing telemetry_screen_wrapper
or the old route names remain in lib/app_router.dart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 90b7b871-1a09-48d1-b9cf-52ba980f3908
📒 Files selected for processing (3)
README.mdlib/app_router.dartlib/telemetry/telemetry_manager.dart
✅ Files skipped from review due to trivial changes (1)
- README.md
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
Signed-off-by: DELTA-45-G <sagarbirwa80@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added Firebase telemetry integration for the Android Registration Client.
Changes
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests