Repository navigation
feat: add an onFeatureFlags listener you can unsubscribe - #670
turnipdabeets wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 34d79f4: 2 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
posthog-flutter Compliance ReportDate: 2026-10-26 18:43:53 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
5a85f1b to
2f7955e
Compare
de3a190 to
0a7ab2a
Compare
…FeatureFlags, update API snapshot
2f7955e to
a194366
Compare
dustinbyrne
left a comment
There was a problem hiding this comment.
The subscription API and ordinary delivery/replay paths look sound. Two dispatch-boundary cases remain: a config callback can restore an invalidated snapshot after reset/close, and a synchronous config-callback exception prevents runtime subscribers from receiving the load. Both reproduce through the public Dart API and IO channel harness.
AI-assisted review.
| ?.call(featureFlagsLoadedFromMap(call.arguments)); | ||
| final loaded = featureFlagsLoadedFromMap(call.arguments); | ||
| _onFeatureFlagsCallback?.call(loaded); | ||
| FeatureFlagsListeners.instance.deliver(loaded); |
There was a problem hiding this comment.
[P1] Preserve reset/close invalidation across the config callback
When a config callback starts reset() or close(), a subsequent runtime subscription can receive the invalidated load after that transition has completed. Expected: no replay until replacement flags arrive. Observed: replay of {'old-user-only': true}.
A supported caller can trigger this without private-state mutation:
Future<void>? transition;
var started = false;
await Posthog().setup(
PostHogConfig('test-token')
..onFeatureFlags = (_) {
if (!started) {
started = true;
transition = Posthog().reset(); // Also reproduces with close().
}
},
);
// After the callback has run:
await transition!;
final subscription = Posthog().onFeatureFlags(updateUi);The sequence is:
- A load arrives for the current user.
- The config callback starts reset/close.
- The public method synchronously clears the replay cache.
- The callback returns.
- The newly added
deliver(loaded)writes that invalidated load back into_last. - A late subscription replays it.
The cause is the changed dispatch ordering:
_onFeatureFlagsCallback?.call(loaded);
FeatureFlagsListeners.instance.deliver(loaded);Equivalent ordering exists in desktop (posthog_flutter_desktop.dart:182–183) and web (posthog_flutter_web.dart:144–145).
This contradicts the new registry's explicit purpose—preventing previous-user replay after reset/close—and the reset/cache invalidation contracts in sdk-specs at b865140e489c191e96e402d12b557d6a586afd0e (openspec/specs/reset/spec.md and openspec/specs/feature-flag-cache/spec.md). It also differs from iOS's invalidation-aware listener handling in PostHogSDK.swift:2395–2426 at f16284d53bb43c2277c1f861a14bb3536d8087cf.
Executed regression: I ran a public-entry IO harness that injects an ordinary onFeatureFlagsCallback message containing {'variants': {'old-user-only': true}, 'errorsLoading': false}, initiates reset/close from the config callback, awaits completion, subscribes, drains microtasks, and asserts an empty result. Both assertions fail on this head with the old-user snapshot. The ordinary delivery/replay control passes.
flutter test --no-pub test/review_callback_boundary_test.dart -r expanded
The local regression file was run in isolated copies using Flutter 3.35.4 / Dart 3.9.2, with identical validation-only removal of workspace resolution and the dev-only dart_apitool dependency. Shipping implementation sources were unchanged. Base 0a7ab2a01000dd97bb0fbb68310592edbf3c3f38 has neither this runtime API nor its replay cache; its controls confirm lifecycle completion, not a passing equivalent replay assertion. Source causality establishes that this stale replay state is introduced here.
The trigger is unusual reentrant application code; no deployed-consumer frequency was established. Impact is stale flag-driven UI following logout or teardown until another load—not demonstrated authorization bypass or backend disclosure. Ordinary reset/close calls between deliveries are not the failing case.
Consider preserving invalidation across this callback boundary, so a reset/close prevents this load from being cached or subsequently dispatched. Applying that consistently to IO, desktop, and web, with both regression cases, would cover the affected seams. No fix was applied or tested.
| _onFeatureFlagsCallback | ||
| ?.call(featureFlagsLoadedFromMap(call.arguments)); | ||
| final loaded = featureFlagsLoadedFromMap(call.arguments); | ||
| _onFeatureFlagsCallback?.call(loaded); |
There was a problem hiding this comment.
[P1] Isolate config-callback exceptions from runtime subscriptions
If the setup-owned config callback throws synchronously, every runtime subscriber misses that load. Expected: independent subscribers still receive it. Observed: the runtime subscriber receives nothing.
Minimal caller:
await Posthog().setup(
PostHogConfig('test-token')
..onFeatureFlags =
(_) => throw StateError('application callback'),
);
final subscription = Posthog().onFeatureFlags(updateUi);The config callback runs outside an exception boundary immediately before FeatureFlagsListeners.instance.deliver(loaded). Its exception prevents delivery from starting, so the registry's per-listener _notify protection cannot help. Equivalent ordering exists in desktop (posthog_flutter_desktop.dart:182–183) and web (posthog_flutter_web.dart:144–145).
The original config-callback exception behavior is pre-existing: the base control confirms the method channel returns an error envelope. The new impact is that this failure now suppresses an independently registered notification channel. A persistently failing handler can starve it on every refresh.
This conflicts with the target's deliberate per-listener isolation and existing desktop-emitter convention. Independent browser implementation/tests also isolate throwing handlers so later callbacks run (packages/browser-common/src/feature-flags.ts:1875–1898; tests/feature-flags-behavior.spec.ts:1479–1500, at 7ef9141d74cf01f25f96ec82f5b46ad95517d657). The sdk-specs on-feature-flags contract at b865140e489c191e96e402d12b557d6a586afd0e permits SDK-specific exception handling rather than mandating isolation universally.
Executed regression: I ran an IO harness that sets the throwing config callback, subscribes through public Posthog().onFeatureFlags, and injects an ordinary native load containing {'variants': {'old-user-only': true}, 'errorsLoading': false}. Head fails:
Expected: [{'old-user-only': true}]
Actual: []
flutter test --no-pub test/review_callback_boundary_test.dart -r expanded
The ordinary callback control passes. Base 0a7ab2a01000dd97bb0fbb68310592edbf3c3f38 explicitly detects the absent runtime API and confirms the pre-existing error envelope. Later replay/subsequent-load assertions follow the first failing assertion and were not reached. The local regression file was run in isolated copies using Flutter 3.35.4 / Dart 3.9.2, with identical validation-only removal of workspace resolution and the dev-only dart_apitool dependency. Shipping implementation sources were unchanged.
Consider catching/logging the config callback independently of runtime delivery on all three platforms. Coordinating with the invalidation case above matters: unconditional delivery in finally would still restore an invalidated snapshot. This finding concerns synchronous exceptions, not later errors from async callback work. No fix was applied or tested.
💡 Motivation and Context
Brings Flutter to parity with posthog-ios 4.0 (
PostHogSDK.shared.onFeatureFlags(_:)) and posthog-android 4.0 (PostHog.onFeatureFlags(callback), PostHog/posthog-android#878). Until now Flutter only hadPostHogConfig.onFeatureFlags, which is fixed atsetup()and can't be removed.Same semantics as iOS and Android:
reloadFeatureFlags(),identify()/group()/reset()), witherrorsLoadingand the last known flags on failure.setup()and survivesclose()/setup().reset()andclose()drop the cached load, so a new listener never gets the previous user's flags.Implementation: a Dart-side registry (
FeatureFlagsListeners,@internal). Each platform delivers to it next to the existing config callback: method channel (iOS/Android/macOS), desktop client (Windows/Linux), and posthog-js on web. No native changes, since native already forwards every load.Web now registers its posthog-js
onFeatureFlagslistener even withoutconfig.onFeatureFlags, and unsubscribes it onclose()and before re-registering on repeatedsetup(). Before, everysetup()added another JS listener, so the config callback ran once persetup()call.A throwing listener is logged and doesn't stop the others. Web skips registration when the page's
posthogobject has noonFeatureFlags(test stubs), instead of throwing duringsetup().Stacked on #664 (needs
PostHogFeatureFlagsLoaded).💚 How did you test it?
test/feature_flags_listeners_test.dart: delivery until unsubscribe, one replay, replay skipped when a newer load wins, no replay after clear, unsubscribe during delivery, a throwing listener.posthog_flutter_io_test), desktop (posthog_flutter_desktop_test), web (posthog_flutter_web_setup_test, also covers repeated setup delivering once).posthog_test: no replay of flags loaded beforereset()/close().flutter analyzeclean,flutter testandflutter test --platform chrome test/posthog_flutter_web_setup_test.dartpass.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code wrote this while aligning the flags API across iOS 4, Android 4 and Flutter 6. Kept the listener in Dart instead of adding a native subscribe/unsubscribe channel method: native already pushes every load to Dart, so a Dart registry covers all five platforms with one implementation.
🤖 Generated with Claude Code