Ak/bd 7586#38
Conversation
… these events are displayed even if user has left GeoTriggering page.
…utter-Minimal-Integration into ak/BD-7586 # Conflicts: # pubspec.yaml
akrupczynskirezolve
requested review from
denniskoluris,
nehabluedot and
nklymenko
akrupczynskirezolve
left a comment
akrupczynskirezolve
left a comment
There was a problem hiding this comment.
Code Review — BD-7586: Push Notifications - Flutter plugin
ℹ️
.github/copilot-instructions.mdanddocs/ai/resources/were not found in the repo; review is performed against general Flutter/Dart best practices.
What this PR does
Adds bluedot_point_sdk_push as an optional dependency, gates the push-notifications feature behind a PUSH_ENABLED flag (set in gradle.properties / Info.plist and bridged to Flutter via a MethodChannel), wires up PushNotificationManager and a new PushNotificationsPage, and substantially refactors GeoTriggeringPage to use a singleton event store + live list view instead of modal alerts.
Overview
The direction is solid — the singleton handler pattern for the geo-triggering channel is a meaningful improvement over per-page setMethodCallHandler calls. The push isolation behind a flag is sensible for a demo app that not-always has Firebase credentials available.
However, there are 4 blocking issues and 5 important issues that need attention before merge (see inline comments). The most critical are:
- 🔴 Uncancelled stream subscription in
PushNotificationsPage(memory/behaviour leak) - 🔴
setStatewithoutmountedguard inHomePage.initState - 🔴 Inconsistent
PUSH_ENABLEDdefaults (trueinmain.dart,falseinconstants.dart) - 🔴 Split-brain push-enabled detection where the listener registration (compile-time) and the UI button (runtime) are driven by different mechanisms that can disagree
Top-level observations
- Testing: No tests are included. New logic (singleton store, event parsing,
AppConfig,PushNotificationManager) is well-suited for unit tests with a mockMethodChannel. Not blocking for a demo app, but worth flagging. GeoTriggeringPagerefactor: The new layout (controls pinned top, scrollable event log below) is a UX improvement. The_GeoTriggeringChannelHandlersingleton elegantly solves the previous problem of losing events on page re-creation.- Documentation: PR body was empty; Jira context has been added. Consider updating the
README.mdwith how to enable push (settingPUSH_ENABLED=true+ providinggoogle-services.json).
🎭 A Flutter dev said: "Push shall be found!"
So flags were born — both true and false, profound.
The singleton stood tall while streams ran free,
Uncancelled, unguarded — a bug in the tree. 🎭
| }); | ||
| } | ||
|
|
||
| @override |
There was a problem hiding this comment.
🔴 Stream subscription leak — the StreamSubscription returned by .listen(...) is never stored and cancelled in dispose(). If the user navigates away and back repeatedly, each mount creates a new subscription that runs indefinitely.
late StreamSubscription<PushNotificationEvent> _sub;
@override
void initState() {
super.initState();
_events = List.from(PushNotificationManager.instance.events);
_sub = PushNotificationManager.instance.stream.listen((event) {
if (mounted) setState(() => _events.add(event));
});
}
@override
void dispose() {
_sub.cancel();
super.dispose();
}| }); | ||
| _retrieveProjectId(); | ||
| AppConfig.isPushEnabled().then((value) { | ||
| setState(() { |
There was a problem hiding this comment.
🔴 setState without mounted guard — if the widget is disposed before the async MethodChannel call resolves (e.g. the user navigates away quickly), this throws a setState() called after dispose() error.
AppConfig.isPushEnabled().then((value) {
if (mounted) {
setState(() => _pushEnabled = value);
}
});|
|
||
| /// Whether push-notifications is enabled in this build. | ||
| /// Set via --dart-define=PUSH_ENABLED=true|false (driven by gradle.properties). | ||
| const bool _pushEnabled = bool.fromEnvironment('PUSH_ENABLED', defaultValue: true); |
There was a problem hiding this comment.
🔴 Inconsistent PUSH_ENABLED defaults — defaultValue: true here means push is on by default when no --dart-define is passed at build time, but constants.dart declares bool.fromEnvironment('PUSH_ENABLED') (which defaults to false). A developer who runs the app without explicitly setting --dart-define=PUSH_ENABLED=true will get _pushEnabled = true in main.dart (push listener registered) but pushEnabled = false in constants.dart. The two sources of truth drift.
Align both to the same default, or remove the compile-time constant in constants.dart and rely solely on the runtime AppConfig.isPushEnabled() path.
| PushNotificationEvent.fromMap('clicked', data), | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🔴 Split-brain push-enabled detection — _pushEnabled in main.dart (compile-time bool.fromEnvironment) drives whether the push listener is wired up, while AppConfig.isPushEnabled() (runtime MethodChannel → native) drives whether the button appears in HomePage. These can disagree:
- Build without
--dart-define=PUSH_ENABLED=true→ listener not registered, butAppConfigreturns the native value (Android:truefromgradle.properties) → button is shown → tapping it may crash or silently do nothing.
Consider making the push listener setup reactive to the same runtime value, or gate everything on a single source of truth.
| _controller.add(event); | ||
| } | ||
|
|
||
| void dispose() { |
There was a problem hiding this comment.
🟡 dispose() on a singleton is dangerous — PushNotificationManager is a singleton, but exposes a dispose() method that closes the StreamController. Any caller who invokes it will permanently break push delivery for the entire app lifetime (the controller cannot be re-opened). Either:
- Remove the
dispose()method if the singleton is meant to live for the app's lifetime, or - Add an
isClosedguard and re-create the controller on next use.
| @@ -1,4 +1,5 @@ | |||
| org.gradle.jvmargs=-Xmx1536M | |||
| org.gradle.jvmargs=-Xmx4096M -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError | |||
| PUSH_ENABLED=true | |||
There was a problem hiding this comment.
🟡 PUSH_ENABLED=true committed to source — gradle.properties sets PUSH_ENABLED=true, but android/app/google-services.json is now gitignored. Any contributor who clones the repo and runs flutter build apk (or flutter run) will get a build failure because com.google.gms.google-services is applied but no google-services.json is present.
Consider defaulting to PUSH_ENABLED=false here and documenting in the README how to opt-in, or provide a no-op placeholder google-services.json for CI / open-source builds.
| # The example app is bundled with the plugin so we use a path dependency on | ||
| # the parent directory to use the current plugin's version. | ||
| git: | ||
| url: https://github.com/Bluedot-Innovation/PointSDK-Flutter-Plugin.git |
There was a problem hiding this comment.
🟡 Feature-branch SDK refs must not merge to a stable branch — pubspec.yaml pins both bluedot_point_sdk and bluedot_point_sdk_push to ref: ak/BD-7586. Likewise pubspec.lock resolves to those branch SHAs. If the SDK branch is rebased or force-pushed, anyone running flutter pub get will get a different (or missing) commit. These refs must be replaced with a stable tag/release SHA before merge.
|
|
||
| Future<void> _handleCall(MethodCall call) async { | ||
| final args = call.arguments; | ||
| final store = GeoTriggerEventStore.instance; |
There was a problem hiding this comment.
🟡 Possible regression: didUpdateZoneInfo no longer fetches zone details — the previous implementation called _getZonesAndFences() on didUpdateZoneInfo, which fetched the full zone list and displayed destination custom data in an alert. The new handler just logs the raw args to the event list. This is a silent behavioural change: the zone detail display is completely removed. If this is intentional (the old approach was overly noisy), please note it in the PR description. For a minimal integration demo, showing the raw event args is probably fine, but the old getZonesAndFences() call was the only place demonstrating that API — its removal should be deliberate.
| return result; | ||
| } on PlatformException { | ||
| return true; // default to enabled on error | ||
| } |
There was a problem hiding this comment.
🔵 AppConfig.isPushEnabled() catches only PlatformException — if the MethodChannel is not set up yet (e.g. MissingPluginException in a test harness or a fresh install state), the exception won't be caught and the call will throw unhandled. Consider broadening the catch to Exception or at minimum also catching MissingPluginException.
} catch (_) {
return true; // default to enabled on error
}| @@ -2,6 +2,8 @@ | |||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | |||
There was a problem hiding this comment.
🔵 iOS defaults PUSH_ENABLED to false, AppConfig defaults to true — Info.plist sets <key>PUSH_ENABLED</key><false/>, but AppConfig.isPushEnabled() returns true on any PlatformException. If the Info.plist key is read successfully it returns false (push hidden), but if the channel call fails for any reason it returns true (push shown). The two fallback behaviours are opposite. Make them consistent.
There was a problem hiding this comment.
Pull request overview
Adds push-notification support to the Flutter minimal integration app (including a UI event log) and refactors geo-triggering event handling to persist logs across navigations. It also introduces build-time/runtime “push enabled” gating via platform config and Android Gradle flags.
Changes:
- Add
bluedot_point_sdk_pushdependency plus a push notifications page + in-app event logging. - Wire native (iOS/Android) config into Flutter via a MethodChannel (
isPushEnabled) and add Android Gradle toggles for Google Services. - Refactor geo-triggering UI to include an on-screen event log backed by a singleton store/channel handler.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pubspec.yaml | Adds push module dependency and overrides to align git-sourced SDK dependencies. |
| pubspec.lock | Locks new git dependency refs for SDK + push module. |
| lib/main.dart | Registers push notification listener (gated by Dart compile-time flag) and adds route to push notifications page. |
| lib/home_page.dart | Adds conditional navigation button for push notifications based on native config. |
| lib/push_notifications_page.dart | New UI to display received/clicked push notification events. |
| lib/helpers/push_notification_manager.dart | New singleton + stream model for push event collection. |
| lib/helpers/constants.dart | Adds PUSH_ENABLED compile-time constant (documented as build-time flag). |
| lib/helpers/app_config.dart | New MethodChannel wrapper for reading native PUSH_ENABLED flag. |
| lib/geo_triggering_page.dart | Refactors geo-triggering event handling to persist and display event logs. |
| ios/Runner/Info.plist | Adds PUSH_ENABLED flag (default false). |
| ios/Runner/AppDelegate.swift | Exposes isPushEnabled via FlutterMethodChannel. |
| android/gradle.properties | Adds PUSH_ENABLED default and increases Gradle JVM args. |
| android/build.gradle | Adds Google Services Gradle plugin classpath. |
| android/app/build.gradle | Conditionally applies Google Services plugin and exposes PUSH_ENABLED to BuildConfig. |
| android/app/src/main/kotlin/io/bluedot/flutter_minimal_app/MainActivity.kt | Exposes isPushEnabled via MethodChannel backed by BuildConfig.PUSH_ENABLED. |
| android/.gitignore | Ignores google-services.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Snapshot the current list plus subscribe for live updates. | ||
| _events = List.from(PushNotificationManager.instance.events); | ||
| PushNotificationManager.instance.stream.listen((event) { | ||
| if (mounted) { | ||
| setState(() => _events.add(event)); | ||
| } | ||
| }); |
| @@ -23,6 +31,28 @@ class _MyAppState extends State<MyApp> { | |||
| super.initState(); | |||
| // Request permissions for location and notification | |||
| _requestPermission(); | |||
|
|
|||
| // Register push notification listener when Firebase is enabled. | |||
| // In default mode the plugin's DefaultMessagingService automatically | |||
| // handles FCM token updates and message delivery — no extra wiring needed. | |||
| if (_pushEnabled) { | |||
| _setupPushListener(); | |||
| } | |||
| AppConfig.isPushEnabled().then((value) { | ||
| setState(() { | ||
| _pushEnabled = value; | ||
| }); | ||
| }); |
| final bool result = await _channel.invokeMethod('isPushEnabled'); | ||
| return result; | ||
| } on PlatformException { | ||
| return true; // default to enabled on error |
| let controller = window?.rootViewController as! FlutterViewController | ||
| let configChannel = FlutterMethodChannel( | ||
| name: "io.bluedot.flutter_minimal_app/config", | ||
| binaryMessenger: controller.binaryMessenger | ||
| ) |
| @@ -1,4 +1,5 @@ | |||
| org.gradle.jvmargs=-Xmx1536M | |||
| org.gradle.jvmargs=-Xmx4096M -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError | |||
| PUSH_ENABLED=true | |||
|
|
||
| if (pushEnabled) { |
| versionCode flutterVersionCode.toInteger() | ||
| versionName flutterVersionName | ||
|
|
||
| // Expose flag to Android (BuildConfig) and Flutter (dart-define) |
| /// Set in gradle.properties (or overridden by the PUSH_ENABLED env var). | ||
| /// Passed to Dart via --dart-define at build time. |
AI Development Checklist
Jira Context
Ticket: BD-7586 – Push Notifications - Flutter plugin
Status: In Merge Request | Priority: P3 - Major | Type: Story
Summary
Enable Flutter developers to use push notifications (including location-triggered notifications) without requiring native iOS/Android implementation, ensuring parity with our native SDKs.
Acceptance Criteria