Opt-In Anonymous Telemetry#67
Conversation
|
Warning Rate limit exceeded@theSoberSobber has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds PostHog analytics: new dependency and macOS plugin registration, a consent-aware singleton AnalyticsService with distinct_id persistence, CI/Android workflow dart-define propagation for PostHog keys, app startup integration with PosthogObserver, and widespread instrumentation across UI screens and widgets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Flutter App (main)
participant AS as AnalyticsService (singleton)
participant SP as SharedPreferences
participant PH as PostHog SDK
participant Screen as Instrumented Screen
App->>AS: initializeIfConsented()
AS->>SP: read _promptedKey, _optInKey
alt user opted in
AS->>PH: configure(apiKey, host, options)
PH-->>AS: configured
AS->>SP: getOrCreate distinct_id
AS->>PH: identify(distinct_id)
AS->>PH: track("app_started")
AS-->>App: initialized
else not opted in
AS-->>App: no-op (consent required)
end
Screen->>AS: trackEvent / trackScreen / trackButton / trackException
AS->>PH: track(...) (if enabled)
PH-->>AS: ack / error
AS->>SP: persist consent changes (on setUserConsent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 4
🧹 Nitpick comments (12)
lib/presentation/widgets/settings_icon_button.dart (1)
4-4: Minor: avoid recreating AnalyticsService inbuildThis is totally fine as‑is, but if
AnalyticsService()ever does more work than just returning a singleton, consider hoisting it to a static field so it isn’t re‑instantiated on every rebuild:class SettingsIconButton extends StatelessWidget { - const SettingsIconButton({super.key}); + const SettingsIconButton({super.key}); + + static final AnalyticsService _analytics = AnalyticsService(); @override Widget build(BuildContext context) { - final analytics = AnalyticsService(); return Container( ... - onPressed: () { - analytics.trackButton('open_settings', location: 'search_bar'); + onPressed: () { + _analytics.trackButton('open_settings', location: 'search_bar');Route name
SettingsforRouteSettingsalso looks consistent with how navigation analytics are usually keyed.Also applies to: 11-12, 27-32
lib/presentation/screens/images/build_image_screen.dart (2)
4-5: Be consistent about awaiting analytics futures (or intentionally fire-and-forget)Here you call:
_analytics.trackEvent('images.build.started', properties: {...});without
await, while latertrackEvent/trackExceptioncalls are awaited. IftrackEventreturns aFuture, consider either:
- Always awaiting it for consistency, or
- Intentionally treating it as fire‑and‑forget and wrapping with
unawaited(...)(fromdart:async) so it’s explicit and avoids “unawaited future” lint warnings.Also, double‑check that sending
imageandtagas plain strings fits your “anonymous telemetry” bar (those values can include internal image names/registries); if you want stricter anonymity, you could instead send only coarse metadata (e.g., “hasCustomRegistryHost”, “tagIsLatest”, etc.).Also applies to: 23-24, 60-65
116-120: Exception tracking is useful; consider how much detail you really needThe success/failure
images.build.completedevents and theimages.build.failedexception tracking are well placed and should give you a good view of build reliability.For anonymity:
images.build.completedwithstatus: success|failureis safe and high‑value.- For
trackException('images.build.failed', e, ...), ensure thatAnalyticsServiceeither:
- Sanitizes
e(or its string form) before sending to PostHog, or- Only records high‑level fields (e.g., exception type, “timeout/auth/docker_error” buckets) rather than raw messages that might contain hostnames, registry URLs, or user-specific paths.
Centralizing that sanitization inside
AnalyticsService.trackExceptionkeeps call sites like this simple while still honoring “anonymous telemetry.”Also applies to: 140-145, 170-176
lib/presentation/screens/containers/create_container_screen.dart (1)
230-237: Analytics tracking occurs afterNavigator.pop()- potential race condition.The
await _analytics.trackEvent(...)on lines 231-237 executes afterNavigator.pop(true)on line 230. At this point, the widget may already be disposed, and since AnalyticsService is a singleton, the tracking should still work, but this is an awkward code flow.Consider moving the tracking before the navigation:
+ await _analytics.trackEvent('containers.created', properties: { + 'name': _nameController.text, + 'image': _selectedImage != null ? '${_selectedImage!.repository}:${_selectedImage!.tag}' : '', + 'restartPolicy': _restartPolicy, + 'networkMode': _networkMode, + 'privileged': _privileged, + }); Navigator.of(context).pop(true); // Return true to indicate success - await _analytics.trackEvent('containers.created', properties: { - 'name': _nameController.text, - 'image': _selectedImage != null ? '${_selectedImage!.repository}:${_selectedImage!.tag}' : '', - 'restartPolicy': _restartPolicy, - 'networkMode': _networkMode, - 'privileged': _privileged, - });lib/presentation/screens/home_screen.dart (1)
124-128: Consider privacy implications of tracking server names.
serverNamemight contain user-identifying or sensitive information (e.g., "John's Home Server", "Production-DB-Primary"). While the telemetry is opt-in, you may want to hash or omit server names to ensure anonymity.Consider whether server names should be tracked, or if only the
serverIdis sufficient for analytics purposes.Also applies to: 157-160
lib/presentation/screens/volumes_screen.dart (1)
143-146: Volume names may contain sensitive path information.Docker volume names, especially custom ones, might reveal directory structures or project names. Consider tracking only a boolean like
isAnonymousVolumeor hashing the name.await _analytics.trackEvent('volumes.action_selected', properties: { 'action': action.command, - 'volume': volume.volumeName, + 'volumeNameLength': volume.volumeName.length, });Also applies to: 204-208, 214-221
lib/presentation/screens/server_list_screen.dart (1)
67-70: Same privacy consideration for server names.Server names are tracked in multiple events. Consider the same approach as suggested for HomeScreen - use only
serverIdor hash the name.Also applies to: 89-92, 131-134
lib/presentation/screens/containers_screen.dart (1)
291-296: Container names may expose application/project names.Similar to previous screens,
containerNamemight reveal project structure. ThecontainerId(Docker's SHA) is safe to track.lib/presentation/screens/images/pull_image_screen.dart (1)
53-56: Registry URL might reveal private infrastructure.The
registryfield could expose private registry URLs (e.g.,private-registry.company.com). Consider tracking only whether it's the default registry or a custom one._analytics.trackEvent('images.pull.search', properties: { 'queryLength': _searchController.text.trim().length, - 'registry': _registryController.text.trim(), + 'isDefaultRegistry': _registryController.text.trim() == 'hub.docker.com', });lib/presentation/screens/settings_screen.dart (1)
348-357: Telemetry section strings are not localized.The "Telemetry" section header and switch tile text are hardcoded in English while the rest of the settings use
easy_localization. Consider adding translation keys for consistency.- _buildSectionHeader('Telemetry'), + _buildSectionHeader('settings.telemetry'.tr()), SwitchListTile( - title: const Text('Share anonymous telemetry'), - subtitle: const Text( - 'Helps us improve the app. No server credentials or commands are sent.', - ), + title: Text('settings.telemetry_title'.tr()), + subtitle: Text('settings.telemetry_description'.tr()), value: _telemetryOptIn, onChanged: _setTelemetryOptIn, ),lib/presentation/screens/images_screen.dart (1)
141-151: Avoid awaiting analytics calls in_handleImageActionto keep the UI snappy.Right now, action execution, success snackbars, refresh, and error handling all wait on PostHog network calls. You can safely fire‑and‑forget these since
AnalyticsServicealready catches its own errors.For example:
- await _analytics.trackEvent('images.action_selected', properties: { + _analytics.trackEvent('images.action_selected', properties: { 'action': action.command, 'repository': image.repository, 'tag': image.tag, });- await _analytics.trackEvent('images.action_completed', properties: { + _analytics.trackEvent('images.action_completed', properties: { 'action': action.command, 'repository': image.repository, 'tag': image.tag, 'status': 'success', });- await _analytics.trackException( + _analytics.trackException( 'images.action_failed', e, properties: { 'action': action.command, 'repository': image.repository, 'tag': image.tag, }, );If you have the
unawaited_futureslint enabled, you can instead wrap these inunawaited(...)and addimport 'dart:async';at the top of the file.Also applies to: 197-213, 217-226
lib/presentation/screens/networks_screen.dart (1)
140-149: Make network action analytics fire‑and‑forget to avoid blocking user feedback.Similar to images,
_handleNetworkActioncurrently awaits analytics before showing success/error snackbars and reloading, which can add latency on slow networks even though telemetry is non-critical.You can avoid awaiting the futures, e.g.:
- await _analytics.trackEvent('networks.action_selected', properties: { + _analytics.trackEvent('networks.action_selected', properties: { 'action': action.command, 'network': network.name, 'scope': network.scope, });- await _analytics.trackEvent('networks.action_completed', properties: { + _analytics.trackEvent('networks.action_completed', properties: { 'action': action.command, 'network': network.name, 'scope': network.scope, 'status': 'success', });- await _analytics.trackException( + _analytics.trackException( 'networks.action_failed', e, properties: { 'action': action.command, 'network': network.name, 'scope': network.scope, }, );Again, if you enforce
unawaited_futures, wrap these inunawaited(...)instead of just droppingawait.Also applies to: 210-225, 231-239
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/CD.yml(1 hunks).github/workflows/android-release.yml(3 hunks)lib/data/services/analytics_service.dart(1 hunks)lib/main.dart(2 hunks)lib/presentation/screens/add_server_screen.dart(5 hunks)lib/presentation/screens/containers/create_container_screen.dart(5 hunks)lib/presentation/screens/containers_screen.dart(12 hunks)lib/presentation/screens/home_screen.dart(16 hunks)lib/presentation/screens/images/build_image_screen.dart(6 hunks)lib/presentation/screens/images/pull_image_screen.dart(8 hunks)lib/presentation/screens/images_screen.dart(6 hunks)lib/presentation/screens/networks_screen.dart(8 hunks)lib/presentation/screens/server_list_screen.dart(8 hunks)lib/presentation/screens/settings_screen.dart(12 hunks)lib/presentation/screens/volumes_screen.dart(7 hunks)lib/presentation/widgets/settings_icon_button.dart(2 hunks)macos/Flutter/GeneratedPluginRegistrant.swift(1 hunks)pubspec.yaml(1 hunks)
🔇 Additional comments (35)
pubspec.yaml (1)
42-42: Confirmposthog_flutterversion compatibility and keep it updatedAdding
posthog_flutter: ^5.9.0looks fine; just make sure this version is compatible with your current Flutter/Dart toolchain and platform targets and that Dependabot/renovate (orflutter pub outdated) keeps it current, especially for security fixes..github/workflows/android-release.yml (1)
56-68: Telemetry wiring in Android workflow looks sound; confirm secret handlingThe PostHog integration here looks reasonable:
POSTHOG_API_KEYis treated as optional (-zcheck) and only written intoandroid/local.propertieswhen present.POSTHOG_HOSTfalls back tohttps://us.i.posthog.comboth when writinglocal.propertiesand when passing--dart-define.flutter buildis invoked with--dart-define=POSTHOG_API_KEY=…andPOSTHOG_HOST=…, which should be enough for the Dart side to detect “no key” via an empty string.Two things to verify:
- That no other workflow step prints
android/local.propertiesor the expanded--dart-definevalues into logs (to avoid leaking the API key).- That your
AnalyticsServicetreats emptyPOSTHOG_API_KEYas “telemetry disabled”, even ifPOSTHOG_HOSTis set.Also applies to: 90-93, 100-102
macos/Flutter/GeneratedPluginRegistrant.swift (1)
8-13: Ensure PosthogFlutterPlugin registration is generated, not hand-editedRegistering
PosthogFlutterPluginhere is correct for enabling macOS telemetry, but this file is marked as generated. Make sure this change comes from running the Flutter tooling (e.g.,flutter pub get/flutter build macos) rather than manual edits, otherwise it may be overwritten on the next regenerate.It’s also worth confirming in PostHog’s docs that no additional macOS-specific configuration is required beyond this registration and your existing initialization.
lib/presentation/screens/containers/create_container_screen.dart (3)
6-6: LGTM - Analytics service integration.The import and singleton instantiation follow the established pattern across other screens.
Also applies to: 22-22
206-215: LGTM - Submission tracking with appropriate properties.The tracked properties capture useful metadata without logging sensitive data. The
imageSelectedcheck on line 214 is technically redundant since the method returns early if_selectedImageis null, but it doesn't cause harm.
248-250: LGTM - Exception tracking.Properly tracks failure with minimal context (image repository only) without exposing sensitive error details.
lib/presentation/screens/home_screen.dart (5)
10-10: LGTM - Analytics service import and instantiation.Also applies to: 31-31
39-41: Good practice using post-frame callback for consent prompt.This ensures the widget tree is fully built before showing the dialog, avoiding potential build-phase issues.
83-119: Well-implemented consent flow.The telemetry consent dialog:
- Uses
barrierDismissible: falseto ensure a deliberate choice- Persists the choice via
setUserConsent()- Only tracks the opt-in event after consent is given
- Properly checks
mountedbefore popping
202-202: LGTM - Button and navigation tracking with route names.Good pattern of:
- Using
trackButtonwith location context for FAB actions- Adding
RouteSettings(name: ...)to MaterialPageRoute for navigation analyticsAlso applies to: 215-215, 238-238, 251-251, 265-265, 278-278
355-358: LGTM - UI state tracking.Theme toggle and tab navigation tracking capture useful UX metrics without sensitive data.
Also applies to: 394-396
lib/presentation/screens/volumes_screen.dart (4)
10-10: LGTM - Analytics service integration.Also applies to: 26-26
114-117: Clean pattern for refresh tracking.Good separation - the helper method tracks and delegates to the actual load function.
134-136: LGTM - Search tracking.Tracking only query length preserves privacy while providing useful metrics.
153-153: LGTM - Route settings and refresh wiring.Also applies to: 239-239, 293-293
lib/presentation/screens/server_list_screen.dart (3)
6-6: LGTM - Analytics service integration.Also applies to: 23-23
111-113: LGTM - Delete tracking with ID only.Good that only
serverIdis tracked here, not the name.
141-141: LGTM - Button tracking and route settings.Consistent pattern with location context for button tracking.
Also applies to: 144-144, 155-155, 158-158
lib/presentation/screens/containers_screen.dart (5)
10-10: LGTM - Analytics service integration.Also applies to: 29-29
225-225: LGTM - Refresh, search, and filter tracking.Good tracking of user interaction patterns without exposing sensitive data. Using
queryLengthinstead of the actual query is privacy-conscious.Also applies to: 271-274, 282-284
386-391: LGTM - Action completion and exception tracking.Good differentiation between
success_with_outputandsuccess_no_outputstatuses. Exception tracking includes appropriate context.Also applies to: 416-421, 432-440
547-551: Executable path may reveal technology stack.Tracking the
executable(e.g.,redis-cli,mysql,psql) reveals what software the user runs. This is probably acceptable for anonymous telemetry to understand popular use cases, but worth noting.Confirm this is intentional for product analytics.
315-315: LGTM - Route settings and button tracking.Consistent naming conventions for routes and location context for buttons.
Also applies to: 329-329, 554-554, 669-669, 673-673, 723-723, 727-727
lib/presentation/screens/images/pull_image_screen.dart (3)
5-5: LGTM - Analytics service integration.Also applies to: 19-19
79-81: Public image names are generally safe to track.Docker Hub image names and tags are public information. This is reasonable for understanding popular image usage patterns.
Also applies to: 137-141, 173-176
216-219: LGTM - Pull result tracking with appropriate status differentiation.Good pattern of tracking completion with status and using
trackExceptionfor actual errors.Also applies to: 228-232, 237-244
lib/presentation/screens/settings_screen.dart (6)
7-7: LGTM - Analytics service integration and state.Also applies to: 22-22, 24-24
40-40: LGTM - Loading telemetry opt-in status on settings load.Also applies to: 45-45
56-58: Good privacy practice tracking boolean instead of actual path.Using
usesDefaultinstead of the actual Docker CLI path protects user filesystem information.Also applies to: 77-79
91-108: LGTM - Telemetry toggle implementation.Properly updates local state, persists via AnalyticsService, and provides user feedback.
185-187: LGTM - Prune completion and failure tracking.Also applies to: 216-216
317-319: LGTM - Language and theme change tracking.Also applies to: 531-533
lib/presentation/screens/images_screen.dart (1)
10-10: Telemetry wiring for images screen looks consistent and safe.Using a shared
AnalyticsServiceinstance, logging only non-sensitive properties (query length, image repo/tag) and relying on the internal_isEnabledgate gives you opt‑in, anonymous-style telemetry without changing feature behavior; this integration looks good.Also applies to: 26-26, 114-117, 131-139, 145-150, 156-156, 207-212, 218-226, 244-244
lib/presentation/screens/networks_screen.dart (1)
10-10: Networks telemetry is aligned with the images screen and respects the opt‑in model.Tracking refresh, search, inspect/delete selections, blocked deletes, completions, and failures with only network name/scope and query length keeps events low-risk while giving good observability; reusing
AnalyticsServiceand wiring_refreshNetworksinto pull‑to‑refresh and retry is a solid pattern.Also applies to: 26-26, 114-117, 130-138, 144-148, 155-155, 167-169, 220-225, 231-239, 257-257, 311-314
lib/data/services/analytics_service.dart (1)
7-188: AnalyticsService design correctly implements opt‑in anonymous telemetry; verify PostHog CI wiring.This singleton is well-designed: initialization is gated on stored consent,
Posthog().setupis skipped when the API key is missing, all publictrack*methods no‑op when_isEnabledis false, the distinct ID is a random local token, and_cleanPropertiesfilters to primitives/collections with .toString() fallback for unsupported types—delivering resilient, consent-based, non-identifying telemetry. The PostHog configuration flags (captureApplicationLifecycleEvents,errorTrackingConfig.captureFlutterErrors,capturePlatformDispatcherErrors) are current and correct for posthog_flutter v5.9, andPosthog().disable()is the documented method for runtime opt-out. The main follow-up is operational: ensure all build pipelines passPOSTHOG_API_KEYandPOSTHOG_HOSTvia--dart-define.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/data/services/analytics_service.dart (3)
45-50: VerifyflushAt = 1is intentional for production.Setting
flushAt = 1means every event is sent immediately, which can impact battery life and network usage on mobile. Consider a higher batch size (e.g., 10-20) for production use unless real-time analytics is critical.
146-159: Missing_isEnabledguard intrackButton.Unlike
trackScreen,trackEvent, andtrackException, thetrackButtonmethod doesn't check_isEnabledbefore proceeding. While it delegates totrackEvent(which has the guard), this creates unnecessary work building the properties map when analytics is disabled.Future<void> trackButton( String label, { String? location, Map<String, Object?>? properties, }) { + if (!_isEnabled) return Future.value(); return trackEvent( 'ui.button_tap', properties: { 'label': label, if (location != null) 'location': location, ..._cleanProperties(properties), }, ); }
187-204:_cleanPropertiesmay miss nested Map validation.The check
value is Map<String, Object>on line 197 won't matchMap<String, dynamic>which is common in Dart. This could cause nested maps to be converted viatoString()unexpectedly.Consider a broader check:
if (value is bool || value is num || value is String || value is List || - value is Map<String, Object>) { + value is Map) { cleaned[key] = value; } else {Or recursively clean nested maps if stricter validation is needed.
lib/presentation/screens/home_screen.dart (1)
83-121: Consent dialog implementation looks good.The dialog properly:
- Uses
barrierDismissible: falseto require explicit choice- Checks
mountedbeforeNavigator.pop()- Persists consent before closing
- Clearly communicates what data is collected
Minor suggestion: Consider making the dialog text localizable using
.tr()for consistency with the rest of the app.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/data/services/analytics_service.dart(1 hunks)lib/presentation/screens/home_screen.dart(16 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
lib/data/services/analytics_service.dart (3)
8-22: Singleton pattern and initialization look correct.The singleton implementation using private constructor and factory is idiomatic Dart. The reentrancy guard (
_isInitializing) properly prevents duplicate setup calls.
35-70: Potential race condition in_setupPosthogbetween check and set.If
_setupPosthogis called concurrently (e.g., from multiple async contexts), the check on line 40 and subsequent set on line 42 aren't atomic. While unlikely in practice given the single-threaded Dart event loop for synchronous code, theawaitbetween check and flag set could allow interleaving.Consider setting the flag before the first await:
Future<void> _setupPosthog() async { if (_apiKey.isEmpty) { debugPrint('[Analytics] PostHog disabled (POSTHOG_API_KEY missing)'); return; } if (_isEnabled || _isInitializing) return; _isInitializing = true; + // Flag is now set before any await, preventing interleaved calls try {The current placement is correct. This is a minor note—the code is acceptable as-is since the flag is set before the first
awaiton line 52.
100-112: Distinct ID generation is cryptographically sound.Using
Random.secure()for generating the anonymous identifier is appropriate. The 16-byte hex ID provides sufficient entropy for anonymization.lib/presentation/screens/home_screen.dart (3)
63-65: Tracking boolean is appropriate here.The
hasLastServerboolean property is a good example of privacy-preserving analytics—it provides useful product insights without exposing any identifying information.
204-204: Fire-and-forget analytics calls are intentional but consider awaiting for consistency.
trackButtonis called withoutawaithere and in similar locations (lines 240, 267, 307, 329, 368). While this is fine since tracking shouldn't block UI, it's inconsistent with awaited calls elsewhere. This is acceptable as-is given the non-blocking nature is desirable.
396-398: Tab navigation tracking is appropriate.Tracking tab index provides useful navigation analytics without any PII concerns.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/presentation/screens/settings_screen.dart (1)
220-241: Movesettings.prune_startedbefore the command and reconsider what you send inprune_failed.
- The
settings.prune_startedevent is still fired afterexecuteCommand, which misrepresents the lifecycle and was already called out earlier. It should be recorded before the prune actually runs:- // Get the configured Docker CLI path (server-specific if set) - final dockerCmd = await _dockerCliPathService.getDockerCliPath(); - - final result = await sshService.executeCommand('$dockerCmd system prune -af --volumes'); - await _analytics.trackEvent('settings.prune_started'); + // Get the configured Docker CLI path (server-specific if set) + final dockerCmd = await _dockerCliPathService.getDockerCliPath(); + + await _analytics.trackEvent('settings.prune_started'); + final result = await sshService.executeCommand('$dockerCmd system prune -af --volumes');
For
settings.prune_completed, sending onlyresultLengthis a good anonymity safeguard.For
settings.prune_failed, be careful whatecontains. Depending on howAnalyticsService.trackExceptionis implemented,e.toString()or a stack trace might include the full SSH command, host, or username, which would conflict with the UI promise that no commands/credentials are sent. It would be safer iftrackExceptionrecorded only a high‑level error code or type (e.g., “ssh_prune_failed”, timeout vs auth vs generic) rather than raw exception text.Also applies to: 269-269
🧹 Nitpick comments (1)
lib/presentation/screens/settings_screen.dart (1)
7-7: Telemetry state & analytics service wiring look sound; consider guarding againstisOptedInfailure.Using a dedicated
AnalyticsServiceinstance and loading_telemetryOptInviaawait _analytics.isOptedIn()is consistent with the new opt‑in model, and the state update in_loadSettingsis correct. As a minor hardening step, you may want to wrap theisOptedIncall in atry/catch(defaulting tofalse) so a telemetry initialization failure doesn’t block the settings screen from leaving_isLoading = true.Also applies to: 25-25, 31-31, 48-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
lib/presentation/screens/settings_screen.dart(12 hunks)macos/Flutter/GeneratedPluginRegistrant.swift(1 hunks)pubspec.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
macos/Flutter/GeneratedPluginRegistrant.swift (1)
8-8: LGTM!The PostHog Flutter plugin import and registration follow the standard pattern for macOS plugin registration. This appears to be correctly auto-generated after adding
posthog_flutterto the dependencies.Also applies to: 14-14
lib/presentation/screens/settings_screen.dart (5)
103-112: Log-lines change event is well-scoped and privacy-safe.
settings.logs_default_changedwith a simple'value'property is a good, non-sensitive signal; it doesn’t expose any server- or user-specific information and fits the anonymous telemetry promise.
123-133: Docker CLI path event avoids leaking the actual command, which is good.Emitting
settings.docker_path_savedwith only a booleanusesDefaultflag is a nice privacy-preserving compromise; you capture behavior without sending the actual CLI path or host/credentials.
214-217: Prune confirmation event placement is correct.Tracking
settings.prune_confirmedonly whenconfirmed == true && mountedensures you don’t log canceled dialogs, and you also avoid calling analytics on an unmounted widget.
368-395: Language change telemetry is reasonable and low-risk.
settings.language_changedwith alanguageproperty using thelanguageCodeis simple, anonymous, and aligned with the rest of your instrumentation; the snackbar UX around it also looks fine.
647-649: Theme change analytics are minimal and appropriate.
settings.theme_changedwith a singlemodeproperty (mode.name) is a clean signal and doesn’t introduce privacy or UX concerns.
| Future<void> _setTelemetryOptIn(bool enabled) async { | ||
| setState(() { | ||
| _telemetryOptIn = enabled; | ||
| }); | ||
| await _analytics.setUserConsent(enabled); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text( | ||
| enabled | ||
| ? 'Anonymous telemetry enabled. Thank you for helping improve the app.' | ||
| : 'Telemetry disabled. You can re-enable it anytime.', | ||
| ), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Dart Flutter async function callback type void Future<void> Function compatibility
💡 Result:
Short answer: they are not directly compatible. A synchronous callback type (void Function() or Flutter's VoidCallback) is a different static type than an async callback (Future Function()). An async function declared async returns Future, so you must either change the parameter type to accept Future Function(), use Flutter's AsyncCallback, or wrap/pass a synchronous wrapper that launches the async work. [1][2][3]
Options:
- Change the signature to AsyncCallback / Future Function(). Example: typedef AsyncCallback = Future Function();. Use that for onPressed/onRefresh when you need awaiting. [1]
- Wrap the async call in a sync lambda: onPressed: () { unawaited(myAsync()); } or onRefresh: () async { await myAsync(); } — or pass the function itself (without "()") when the parameter already expects Future Function(). Be careful not to pass myAsync() (that calls it and gives a Future, not a function). [2][3]
- If the caller expects a sync VoidCallback and you cannot change it, make the body non-async (start the async work but don’t return/await it) — but prefer returning Future so callers can await and handle errors. [2]
Sources: Flutter AsyncCallback typedef, Dart async docs, and common examples/discussion. [1][2][3]
🏁 Script executed:
# Find and read the settings screen file
find . -name "settings_screen.dart" -type fRepository: theSoberSobber/Docker-Manager
Length of output: 120
🏁 Script executed:
# Read the settings_screen.dart file around lines 144-161 and nearby context
wc -l ./lib/presentation/screens/settings_screen.dartRepository: theSoberSobber/Docker-Manager
Length of output: 124
🏁 Script executed:
# Read the section with the _setTelemetryOptIn method
sed -n '140,170p' ./lib/presentation/screens/settings_screen.dartRepository: theSoberSobber/Docker-Manager
Length of output: 942
🏁 Script executed:
# Also check lines 404-411 mentioned as "Also applies to"
sed -n '400,415p' ./lib/presentation/screens/settings_screen.dartRepository: theSoberSobber/Docker-Manager
Length of output: 706
Fix type mismatch between _setTelemetryOptIn and SwitchListTile.onChanged.
SwitchListTile.onChanged expects void Function(bool)?, but _setTelemetryOptIn is declared as Future<void> _setTelemetryOptIn(bool enabled) async. Passing the method tear-off directly will cause a compile-time error because Future<void> Function(bool) is not assignable to void Function(bool).
Wrap the call in a closure:
- onChanged: _setTelemetryOptIn,
+ onChanged: (value) async {
+ await _setTelemetryOptIn(value);
+ },Alternatively, change _setTelemetryOptIn's signature to void _setTelemetryOptIn(bool enabled) and fire-and-forget the async work (not recommended if error handling is needed).
Also applies to: 404-411
🤖 Prompt for AI Agents
In lib/presentation/screens/settings_screen.dart around lines 144 to 161, the
code passes the async method tear-off _setTelemetryOptIn directly to
SwitchListTile.onChanged which expects a synchronous void Function(bool),
causing a type mismatch; fix by passing a synchronous closure that calls the
async method (e.g. onChanged: (bool enabled) { _setTelemetryOptIn(enabled); })
so the handler signature matches and the async work runs fire-and-forget (or,
alternatively, change _setTelemetryOptIn to return void and handle errors
internally, though the closure approach is preferred); apply the same fix at the
other occurrence around lines 404 to 411.
| // Telemetry Section | ||
| _buildSectionHeader('Telemetry'), | ||
| SwitchListTile( | ||
| title: const Text('Share anonymous telemetry'), | ||
| subtitle: const Text( | ||
| 'Helps us improve the app. No server credentials or commands are sent.', | ||
| ), | ||
| value: _telemetryOptIn, | ||
| onChanged: _setTelemetryOptIn, | ||
| ), | ||
|
|
||
| const Divider(height: 32), |
There was a problem hiding this comment.
Localize telemetry strings and align the promise with what is actually sent.
Two points here:
-
All other settings strings are localized, but this section hardcodes English text. Consider adding
settings.telemetry_title,settings.telemetry_label, andsettings.telemetry_descriptionkeys and using.tr()so it behaves consistently across locales. -
The subtitle promises that “No server credentials or commands are sent.” That’s true for most events (they avoid paths/commands), but may not hold if
trackException('settings.prune_failed', e)ultimately logs the full SSH command or similar. Once you sanitize exceptions (or restrict them to a generic code), this statement will be accurate; otherwise you may want to soften or clarify it.
🤖 Prompt for AI Agents
In lib/presentation/screens/settings_screen.dart around lines 402 to 413, the
telemetry section currently uses hardcoded English strings and an absolute
promise about what telemetry contains; replace the hardcoded title, label and
subtitle with localized keys (e.g. settings.telemetry_title,
settings.telemetry_label, settings.telemetry_description) and call .tr() on each
so the UI follows the app localization convention, and update the subtitle text
to either (a) soften/clarify the promise (e.g. say “We avoid sending server
credentials or commands” or “We do not intentionally send server credentials or
commands”) or (b) sanitize telemetry before sending (ensure exceptions are
stripped of sensitive data or mapped to generic error codes like
settings.prune_failed) so the subtitle’s claim is accurate; implement one of
these two fixes and keep the UI strings localized.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.