Skip to content

fix: codebase review — Android getApplicationId staleness, clean push-payload errors, 32-bit guard (v0.30.3)#495

Open
DennisAlund wants to merge 1 commit into
mainfrom
claude/codebase-review-97j9c8
Open

fix: codebase review — Android getApplicationId staleness, clean push-payload errors, 32-bit guard (v0.30.3)#495
DennisAlund wants to merge 1 commit into
mainfrom
claude/codebase-review-97j9c8

Conversation

@DennisAlund

@DennisAlund DennisAlund commented Jul 3, 2026

Copy link
Copy Markdown
Member

Full-codebase review of the Dart API, Android handler, iOS handler, build configs, tests, and docs. The three layers are in very good shape overall; this PR fixes the handful of genuine parity/robustness issues found, bundled as 0.30.3 (rebased on top of the 0.30.2 toolchain bump from #494).

Fixes

Android: getApplicationId returned a stale app id

The handler read appEventsLogger.applicationId, which is captured when the logger is created in onAttachedToEngine — so app-id changes made programmatically after startup (or an access-token app id) were never reflected, contradicting the dartdoc and diverging from iOS (Settings.shared.appID, which is read live). It now reads the live FacebookSdk.getApplicationId().

Android: opaque error for Bundle-incompatible push payloads

logPushNotificationOpen payloads are (deliberately) not validated in the Dart layer, but createBundleFromMap throws IllegalArgumentException for values a Bundle can't hold (e.g. lists), which surfaced as a generic PlatformException(error, ...) with a stack trace — while iOS accepts the same payload. The handler now catches this and returns a clean INVALID_ARGUMENT, and the exception message names the offending key and suggests JSON-encoding structured values.

Dart: setDataProcessingOptions 32-bit validation

The standard method codec delivers Dart ints larger than 32 bits as Long, so an out-of-range country/state caused a ClassCastException in the Android handler (call.argument<Int>). The iOS handler already guards this natively (Int32(exactly:)). Per this repo's convention of validating in the Dart layer, setDataProcessingOptions now throws a RangeError for values that don't fit in a signed 32-bit integer.

Consistency cleanups

  • setDataProcessingOptions and logPushNotificationOpen no longer send explicit nulls over the channel for omitted arguments — every other multi-argument method already filters them via _filterOutNulls.
  • Example app: facebook_app_events moved from dev_dependencies to dependencies (it's imported by lib/main.dart — this was an analyzer depend_on_referenced_packages info), and MyApp got a const constructor with a key parameter. flutter analyze is now clean with zero infos.

Release prep (per CONTRIBUTING.md)

Verification

  • flutter analyze: no issues.
  • flutter test (root): 66/66 passing (re-run after the rebase), including new tests for the RangeError guard and the omitted-null wire shapes of setDataProcessingOptions / logPushNotificationOpen.
  • flutter test (example): passing.
  • Kotlin changes are exercised by the existing Android build matrix in CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_01CqZG34ExQKtZX5QHkD2UFL

Copilot AI review requested due to automatic review settings July 3, 2026 06:38
…guard for data processing options

Codebase review fixes (v0.30.2):

- Android: getApplicationId now reads the live FacebookSdk.getApplicationId()
  instead of the app id captured at engine attach, matching iOS
  (Settings.shared.appID) and the documented behavior.
- Android: logPushNotificationOpen returns a clean INVALID_ARGUMENT error when
  the payload contains a value a Bundle cannot represent (e.g. a list) instead
  of an opaque platform exception; the message now names the offending key.
- Dart: setDataProcessingOptions validates that country/state fit in a signed
  32-bit integer (RangeError otherwise) — out-of-range values previously hit a
  ClassCastException on Android; iOS already guarded natively.
- Dart: stop sending explicit nulls over the channel for omitted arguments of
  setDataProcessingOptions and logPushNotificationOpen, consistent with the
  rest of the API.
- Example: move facebook_app_events from dev_dependencies to dependencies
  (main.dart imports it) and fix widget lints; analyzer is now clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CqZG34ExQKtZX5QHkD2UFL
@DennisAlund DennisAlund force-pushed the claude/codebase-review-97j9c8 branch from 4332723 to 1337d9e Compare July 3, 2026 06:39
@DennisAlund DennisAlund changed the title fix: codebase review — Android getApplicationId staleness, clean push-payload errors, 32-bit guard (v0.30.2) fix: codebase review — Android getApplicationId staleness, clean push-payload errors, 32-bit guard (v0.30.3) Jul 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR prepares a 0.30.2 release by tightening cross-platform parity and robustness in the Flutter Facebook App Events plugin, with targeted fixes in the Dart API and Android handler plus accompanying tests and version bumps.

Changes:

  • Android: getApplicationId now returns the live SDK app id (FacebookSdk.getApplicationId()), avoiding staleness from the logger-captured id.
  • Dart API: stop sending explicit nulls for optional arguments on logPushNotificationOpen / setDataProcessingOptions, and add 32-bit range validation for country/state (throwing RangeError).
  • Android: convert Bundle-incompatible logPushNotificationOpen payload values into a clean INVALID_ARGUMENT error with an actionable message; add tests and bump versions/changelog for 0.30.2.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/facebook_app_events_test.dart Adds coverage for omitted-null wire shapes and 32-bit RangeError validation.
pubspec.yaml Bumps package version to 0.30.2.
lib/facebook_app_events.dart Updates dartdoc, filters nulls on two method calls, and adds 32-bit guards for setDataProcessingOptions.
ios/facebook_app_events.podspec Bumps iOS podspec version to 0.30.2 to match the Dart package version.
example/pubspec.yaml Moves facebook_app_events to dependencies (used by example code).
example/pubspec.lock Updates the example lockfile to reflect the dependency move and new version.
example/lib/main.dart Makes MyApp constructor const and adds a key parameter.
CHANGELOG.md Adds a 0.30.2 entry documenting the fixes and cleanups.
android/src/main/kotlin/id/oddbit/flutter/facebook_app_events/FacebookAppEventsPlugin.kt Fixes live app-id retrieval and maps Bundle incompatibilities for push payloads to a clean INVALID_ARGUMENT error.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

3 participants