fix(cli_tools): Prevent analytics send failures from crashing the host process#125
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 55 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAnalytics.track error handling is refactored to use a new ChangesAnalytics Error Handling and Async Control Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 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.
🧹 Nitpick comments (1)
packages/cli_tools/test/analytics/analytics_test.dart (1)
51-69: ⚡ Quick winMake the test name more behavior-focused.
The test name describes implementation details ("whose sendEvent returns a future with a non-void reified type") rather than observable behavior. As per coding guidelines, test names should describe behavior, not implementation.
Consider rewording to focus on what the test verifies from a behavioral perspective:
- "Given analytics with a failing event send, when flushing, then flush completes successfully despite underlying future type issues."
Or more concisely:
- "Given analytics with a failing send event, when flushing, then flush ignores the failure and completes."
This would align better with the behavioral descriptions used in the other tests (e.g., "pending event", "send fails").
🤖 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 `@packages/cli_tools/test/analytics/analytics_test.dart` around lines 51 - 69, Rename the test that currently starts with "Given an analytics implementation whose sendEvent returns a future with a non-void reified type..." to a behavior-focused description; locate the test(...) invocation and replace the long implementation-detail name with a concise behavioral name such as "Given analytics with a failing send event, when flushing, then flush ignores the failure and completes" (or the alternative "Given analytics with a failing event send, when flushing, then flush completes successfully despite underlying future type issues") so the test describes observable behavior rather than implementation details.Source: Coding guidelines
🤖 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 `@packages/cli_tools/test/analytics/analytics_test.dart`:
- Around line 51-69: Rename the test that currently starts with "Given an
analytics implementation whose sendEvent returns a future with a non-void
reified type..." to a behavior-focused description; locate the test(...)
invocation and replace the long implementation-detail name with a concise
behavioral name such as "Given analytics with a failing send event, when
flushing, then flush ignores the failure and completes" (or the alternative
"Given analytics with a failing event send, when flushing, then flush completes
successfully despite underlying future type issues") so the test describes
observable behavior rather than implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b162de1-f811-48ff-82cd-8f1457e05f75
📒 Files selected for processing (4)
packages/cli_tools/lib/src/analytics/analytics.dartpackages/cli_tools/lib/src/analytics/mixpanel.dartpackages/cli_tools/lib/src/analytics/posthog.dartpackages/cli_tools/test/analytics/analytics_test.dart
marcelomendoncasoares
left a comment
marcelomendoncasoares
left a comment
There was a problem hiding this comment.
Analyzer if failing, but the solution LGTM!
…t process Make both providers' `sendEvent` properly async so they return reified `Future<void>`s, and harden `Analytics.track` to swallow errors with try/catch instead of `catchError`, so implementations returning foreign futures cannot crash the process.
nielsenko
force-pushed
the
fix-missing-async
branch
from
8955c12 to
e3a0cd9
Compare
2 participants
Make both providers'
sendEventproperly async so they return reifiedFuture<void>s, and hardenAnalytics.trackto swallow errors with try/catch instead ofcatchError, so implementations returning foreign futures cannot crash the process.Summary by CodeRabbit
Refactor
Tests