feat(cli_tools): Make the analytics behavior customizable #87
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCentralizes analytics in BetterCommandRunner: makes Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI process
participant Runner as BetterCommandRunner
participant Analytics as OnAnalyticsEvent (nullable)
rect rgb(230,245,255)
CLI->>Runner: instantiate(onAnalyticsEvent?)
Runner-->>Analytics: store final _onAnalyticsEvent
Runner->>Runner: set _analyticsEnabled from constructor
end
rect rgb(245,255,230)
CLI->>Runner: run(args)
Runner->>Runner: await determineAnalyticsSettings()
alt analyticsEnabled == true
Runner->>Analytics: sendAnalyticsEvent("run_started")
else analyticsEnabled == false
Note right of Runner: analytics suppressed
end
Runner->>Runner: dispatch & execute command
alt help / invalid / not-found / error
Runner->>Analytics: sendAnalyticsEvent("<event>") %% emitted only if enabled
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (1)
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (1)
210-218: Consider adding error handling for analytics callback failures.If
onAnalyticsEventthrows an exception, it will propagate fromsendAnalyticsEvent(). This is particularly problematic at line 322 and 337 where it's called from anunawaitedfuture—unhandled exceptions there can cause issues in Dart. Since analytics failures shouldn't disrupt the main flow, consider wrapping the callback invocation in a try-catch.Apply this diff to add defensive error handling:
void sendAnalyticsEvent(final String event) { if (analyticsEnabled()) { - onAnalyticsEvent?.call(event); + try { + onAnalyticsEvent?.call(event); + } catch (_) { + // Silently ignore analytics errors to prevent disrupting the main flow + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart(10 hunks)
🔇 Additional comments (7)
packages/cli_tools/lib/src/better_command_runner/better_command_runner.dart (7)
69-70: LGTM: Clean initialization of analytics state.The addition of
_analyticsEnabledand making_onAnalyticsEventfinal provides clear separation between the callback and the enabled state, allowing for proper customization.Also applies to: 161-161
220-242: LGTM: Well-designed extension point.This method provides a clean customization point for subclasses to implement custom analytics logic (e.g., user consent prompts). The default implementation correctly checks both the callback presence and the configuration flag.
254-276: LGTM: Proper sequencing of analytics initialization.The placement of
determineAnalyticsSettings()after configuration resolution but before command execution ensures analytics state is correctly established. The centralized error handling withsendAnalyticsEvent()is clean.
278-288: LGTM: Consistent analytics flow.The migration to
sendAnalyticsEvent()ensures all analytics go through the centralized, customizable path.
308-351: LGTM: Clean centralization of analytics emission.The refactoring successfully centralizes all analytics through
sendAnalyticsEvent(), making the behavior consistently customizable. The asynchronous analytics sending pattern is appropriate for avoiding impact on the main execution path.
172-172: Good use of the public API.Using the public
onAnalyticsEventgetter instead of the private field maintains consistency with the new public interface design.
202-205: LGTM: Clear documentation of state change behavior.The comment appropriately warns about the mutable nature of the analytics state, which is important for users of this API to understand.
This is a backwards-compatible change that opens up the analytics configuration and sending behavior to be overridden by subclasses.
Summary by CodeRabbit
New Features
Refactor