- 
                Notifications
    You must be signed in to change notification settings 
- Fork 52
feat: autocapture unhandled exceptions #214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/capture-exception
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import 'dart:ui'; | ||
| import 'package:flutter/foundation.dart'; | ||
|  | ||
| import '../posthog_config.dart'; | ||
| import '../posthog_flutter_platform_interface.dart'; | ||
|  | ||
| /// Handles automatic capture of Flutter and Dart exceptions | ||
| class PostHogErrorTrackingAutoCaptureIntegration { | ||
| final PostHogErrorTrackingConfig _config; | ||
| final PosthogFlutterPlatformInterface _posthog; | ||
|  | ||
| // Store original handlers (we'll chain with them from our handler) | ||
| FlutterExceptionHandler? _originalFlutterErrorHandler; | ||
| ErrorCallback? _originalPlatformErrorHandler; | ||
|  | ||
| bool _isEnabled = false; | ||
|  | ||
| static PostHogErrorTrackingAutoCaptureIntegration? _instance; | ||
|  | ||
| PostHogErrorTrackingAutoCaptureIntegration._({ | ||
| required PostHogErrorTrackingConfig config, | ||
| required PosthogFlutterPlatformInterface posthog, | ||
| }) : _config = config, | ||
| _posthog = posthog; | ||
|  | ||
| /// Install the autocapture integration (can only be installed once) | ||
| static PostHogErrorTrackingAutoCaptureIntegration? install({ | ||
| required PostHogErrorTrackingConfig config, | ||
| required PosthogFlutterPlatformInterface posthog, | ||
| }) { | ||
| if (_instance != null) { | ||
| debugPrint( | ||
| 'PostHog: Error tracking autocapture integration is already installed. Call PostHogErrorTrackingAutoCaptureIntegration.uninstall() first.'); | ||
| return null; | ||
| } | ||
|  | ||
| _instance = PostHogErrorTrackingAutoCaptureIntegration._( | ||
| config: config, | ||
| posthog: posthog, | ||
| ); | ||
|  | ||
| if (config.captureUnhandledExceptions) { | ||
| _instance!.start(); | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. avoid the use of  | ||
| } | ||
|  | ||
| return _instance; | ||
| } | ||
|  | ||
| /// Uninstall the autocapture integration | ||
| static void uninstall() { | ||
| if (_instance != null) { | ||
| _instance!.stop(); | ||
| _instance = null; | ||
| } | ||
| } | ||
|  | ||
| /// Start automatic exception capture | ||
| void start() { | ||
| if (_isEnabled) return; | ||
|  | ||
| _isEnabled = true; | ||
|  | ||
| // Set up Flutter error handler if enabled | ||
| if (_config.captureFlutterErrors) { | ||
| _setupFlutterErrorHandler(); | ||
| } | ||
|  | ||
| // Set up platform error handler if enabled | ||
| if (_config.captureDartErrors) { | ||
| _setupPlatformErrorHandler(); | ||
| } | ||
| } | ||
|  | ||
| /// Stop automatic exception capture (restores original handlers) | ||
| void stop() { | ||
| if (!_isEnabled) return; | ||
|  | ||
| _isEnabled = false; | ||
|  | ||
| // Restore original handlers | ||
| if (_originalFlutterErrorHandler != null) { | ||
| FlutterError.onError = _originalFlutterErrorHandler; | ||
| _originalFlutterErrorHandler = null; | ||
| } | ||
|  | ||
| if (_originalPlatformErrorHandler != null) { | ||
| PlatformDispatcher.instance.onError = _originalPlatformErrorHandler; | ||
| _originalPlatformErrorHandler = null; | ||
| } | ||
| 
      Comment on lines
    
      +80
     to 
      +89
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should restore to the default value regardless if they are null or not | ||
| } | ||
|  | ||
| /// Flutter framework error handler | ||
| void _setupFlutterErrorHandler() { | ||
| // prevent circular calls | ||
| if (FlutterError.onError == _posthogFlutterErrorHandler) { | ||
| return; | ||
| } | ||
|  | ||
| _originalFlutterErrorHandler = FlutterError.onError; | ||
|  | ||
| FlutterError.onError = _posthogFlutterErrorHandler; | ||
| } | ||
|  | ||
| void _posthogFlutterErrorHandler(FlutterErrorDetails details) { | ||
| // don't capture silent errors (could maybe be a config?) | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes | ||
| if (!details.silent) { | ||
| _captureException( | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | ||
| error: details.exception, | ||
| stackTrace: details.stack, | ||
| handled: false, | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can create a posthogexception wrapper similar to android to pass those values There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that was the plan from our discussion in #212. Will do | ||
| context: details.context?.toString(), | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see https://github.com/PostHog/posthog-flutter/pull/214/files#r2459538837 | ||
| ); | ||
| } | ||
|  | ||
| // Call the original handler | ||
| if (_originalFlutterErrorHandler != null) { | ||
| try { | ||
| _originalFlutterErrorHandler!(details); | ||
| } catch (e) { | ||
| // Pretty sure we should be doing this to avoid infinite loops | ||
| debugPrint( | ||
| 'PostHog: Error in original FlutterError.onError handler: $e'); | ||
| } | ||
| } else { | ||
| // If no original handler, use the default behavior (default is to dump to console) | ||
| FlutterError.presentError(details); | ||
| } | ||
| 
      Comment on lines
    
      +115
     to 
      +127
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will swallow the error and modify the apps behaviour, we should just call the  | ||
| } | ||
|  | ||
| /// Platform error handler for Dart runtime errors | ||
| void _setupPlatformErrorHandler() { | ||
| // prevent circular calls | ||
| if (PlatformDispatcher.instance.onError == _posthogPlatformErrorHandler) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was added not a long ago so i am not sure this is available from our min version which is now 3.22 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iirc PlatformDispatcher.instance.onError does not work on flutter web, so it should be a no op There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the alternative for web and older versions before having  | ||
| return; | ||
| } | ||
|  | ||
| _originalPlatformErrorHandler = PlatformDispatcher.instance.onError; | ||
| PlatformDispatcher.instance.onError = _posthogPlatformErrorHandler; | ||
| } | ||
|  | ||
| bool _posthogPlatformErrorHandler(Object error, StackTrace stackTrace) { | ||
| _captureException( | ||
| error: error, | ||
| stackTrace: stackTrace, | ||
| handled: false, | ||
| context: 'Platform error'); | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be the  | ||
|  | ||
| // Call the original handler | ||
| if (_originalPlatformErrorHandler != null) { | ||
| try { | ||
| return _originalPlatformErrorHandler!(error, stackTrace); | ||
| } catch (e) { | ||
| debugPrint( | ||
| 'PostHog: Error in original PlatformDispatcher.onError handler: $e'); | ||
| return true; // Consider the error handled | ||
| } | ||
| } | ||
| 
      Comment on lines
    
      +148
     to 
      +157
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. | ||
|  | ||
| return false; // No original handler, don't modify behavior | ||
| } | ||
|  | ||
| Future<void> _captureException({ | ||
| required dynamic error, | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. always use Object or Object? if nullable, check the whole PR about this | ||
| required StackTrace? stackTrace, | ||
| required bool handled, | ||
| String? context, | ||
| }) { | ||
| return _posthog.captureException( | ||
| error: error, | ||
| stackTrace: stackTrace ?? StackTrace.current, | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not use  | ||
| properties: context != null ? {'context': context} : null, | ||
| handled: handled, | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import 'package:meta/meta.dart'; | ||
|  | ||
| import 'exceptions/posthog_error_tracking_autocapture_integration.dart'; | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets make the folder called  | ||
| import 'posthog_config.dart'; | ||
| import 'posthog_flutter_platform_interface.dart'; | ||
| import 'posthog_observer.dart'; | ||
|  | @@ -24,9 +25,27 @@ class Posthog { | |
| /// com.posthog.posthog.AUTO_INIT: false | ||
| Future<void> setup(PostHogConfig config) { | ||
| _config = config; // Store the config | ||
|  | ||
| installFlutterIntegrations(config); | ||
|  | ||
| return _posthog.setup(config); | ||
| } | ||
|  | ||
| void installFlutterIntegrations(PostHogConfig config) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be private | ||
| // Install exception autocapture if enabled | ||
| if (config.errorTrackingConfig.captureUnhandledExceptions) { | ||
| PostHogErrorTrackingAutoCaptureIntegration.install( | ||
| config: config.errorTrackingConfig, | ||
| posthog: _posthog, | ||
| ); | ||
| } | ||
| } | ||
|  | ||
| void uninstallFlutterIntegrations() { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same | ||
| // Uninstall exception autocapture integration | ||
| PostHogErrorTrackingAutoCaptureIntegration.uninstall(); | ||
| } | ||
|  | ||
| @internal | ||
| PostHogConfig? get config => _config; | ||
|  | ||
|  | @@ -85,7 +104,12 @@ class Posthog { | |
|  | ||
| Future<void> reset() => _posthog.reset(); | ||
|  | ||
| Future<void> disable() => _posthog.disable(); | ||
| Future<void> disable() { | ||
| // Uninstall Flutter-specific integrations when disabling | ||
| uninstallFlutterIntegrations(); | ||
|  | ||
| return _posthog.disable(); | ||
| } | ||
|  | ||
| Future<void> enable() => _posthog.enable(); | ||
|  | ||
|  | @@ -147,6 +171,10 @@ class Posthog { | |
| _config = null; | ||
| _currentScreen = null; | ||
| PosthogObserver.clearCurrentContext(); | ||
|  | ||
| // Uninstall Flutter integrations | ||
| uninstallFlutterIntegrations(); | ||
|  | ||
| return _posthog.close(); | ||
| } | ||
|  | ||
|  | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could create the Integration interface here and do it similar to android and ios, with the install, uninstall and setup methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped since this is now just 1 integration, but I agree that the next integration to be added should follow that pattern?