Skip to content

fix(auth): Surface real apple sign In error and harden crashlytics de…#202

Merged
nfebe merged 1 commit into
devfrom
fix/ios-logging
Jun 9, 2026
Merged

fix(auth): Surface real apple sign In error and harden crashlytics de…#202
nfebe merged 1 commit into
devfrom
fix/ios-logging

Conversation

@austin047

Copy link
Copy Markdown
Collaborator

Description

Resurface Apple signing exception

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@austin047 austin047 marked this pull request as ready for review June 9, 2026 16:48
@sourceant

sourceant Bot commented Jun 9, 2026

Copy link
Copy Markdown

Code Review Summary

The PR improves error reporting for Apple Sign-In by capturing and surface specific error messages. It also ensures Firebase Crashlytics is actively collecting and sending reports.

🚀 Key Improvements

  • Added message reporting to Crashlytics for Apple Sign-In failures.
  • Updated Apple Sign-In error handling to return more descriptive failures.
  • Explicitly enabled Crashlytics collection in the service initialization.

💡 Minor Suggestions

  • Avoid hardcoding setCrashlyticsCollectionEnabled(true) if you need to disable it in local development.
  • Review if Failure.badRequest is the appropriate type compared to unauthorizedError for Auth failures.

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. See the overview comment for a summary.

Future<void> initialize() async {
try {
setupFlutterErrorHandling();
await _crashlyticsInstance.setCrashlyticsCollectionEnabled(true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While enabling collection is good for production, hardcoding 'true' here might override environment-specific configurations (e.g., disabling crashlytics in dev). Consider using a configuration flag.

Suggested change
await _crashlyticsInstance.setCrashlyticsCollectionEnabled(true);
if (!kDebugMode) {
await _crashlyticsInstance.setCrashlyticsCollectionEnabled(true);
}
await _crashlyticsInstance.sendUnsentReports();

},
);
return const Left(Failure.unauthorizedError());
return Left(Failure.badRequest(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing from 'unauthorizedError' to 'badRequest' with a string message is a functional change. Ensure the UI layer is prepared to display this raw error message to users, as 'e.message' from Apple might contain technical jargon.

Suggested change
return Left(Failure.badRequest(
return Left(Failure.unauthorizedError(message: 'Apple Sign-In failed: ${e.message}'));

@austin047 austin047 requested a review from nfebe June 9, 2026 16:50
@nfebe nfebe merged commit 78c3387 into dev Jun 9, 2026
3 checks passed
@nfebe nfebe deleted the fix/ios-logging branch June 9, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants