Repository navigation
chore!: pass the loaded flags to onFeatureFlags and reloadFeatureFlags - #664
turnipdabeets wants to merge 9 commits into
Conversation
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 93f0034: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
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-27 18:31:52 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
|
2aa9ae4 to
da3dedd
Compare
onFeatureFlags and reloadFeatureFlags now carry a PostHogFeatureFlagsLoaded with the enabled flags, their values and whether loading failed, matching posthog-ios 4.0 and posthog-android 4.0. On web, reloadFeatureFlags completes right away with the flags loaded so far, because posthog-js doesn't report when a reload finishes.
Wait for the next posthog-js onFeatureFlags call instead of returning the flags already loaded, so a failed reload reports errorsLoading. Complete right away when flags are disabled, and after twice the flags request timeout when posthog-js skips the request without calling back.
The next onFeatureFlags callback can carry a response from a request already in flight, so reloadFeatureFlags() could complete with flags from before the reload. posthog-js 1.438.2+ settles reloadFeatureFlagsAsync() after this reload's own request; use it when present and keep the callback path for older posthog-js.
de3a190 to
0a7ab2a
Compare
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
The native/desktop result plumbing and major migration look consistent, but the legacy web fallback can report a successful retry as failed with default settings. Consider accounting for the full retry lifecycle before completing the fallback. Two optional documentation qualifications are inline. Native 4.0 dependency/build prerequisites remain separate.
AI-assisted review.
| try { | ||
| return await reloaded.future.timeout( | ||
| Duration(milliseconds: 2 * (requestTimeoutMs ?? 3000)), | ||
| onTimeout: () => currentFlags(errorsLoading: true), |
There was a problem hiding this comment.
1. [P1] Let supported legacy retries finish before reporting reload failure
IMPORTANT · REQUIRED · Introduced result-reporting defect
Location: posthog_flutter/lib/src/posthog_flutter_web_handler.dart:273–274, right side.
The legacy deadline can return cached flags with errorsLoading: true before posthog-js finishes a successful retry—even with its default 3000 ms timeout and one retry. It should await that supported reload cycle and return the successful snapshot.
A normal slow-network sequence is:
- The first request reaches its 3000 ms timeout.
- JS waits its built-in 500 ms retry backoff.
- The retry succeeds after 2800 ms, within its own timeout.
- Flutter has already returned at approximately 6000 ms with cached flags and failure; JS subsequently publishes fresh flags and success.
The supported caller is simply:
await Posthog().setup(PostHogConfig('<project_token>'));
final loaded = await Posthog().reloadFeatureFlags();No overlapping reload, identity change or custom timeout is necessary. Applications branching on loaded.errorsLoading can incorrectly enter failure handling and receive stale values. This affects the retained older-JS branch; the modern async branch avoids this deadline.
Official JS 1.438.1 defaults to one retry and keeps retries/backoff inside the logical reload. The canonical completion contract explicitly forbids reporting failure when that reload succeeds.
Executed evidence: Loaded the actual published npm JS 1.438.1 distribution and exercised public Flutter setup/reload. Only fetch was intercepted: attempt one respected the real AbortSignal timeout; attempt two supplied a successful Response after 2800 ms. Both timeout/retry configuration keys were omitted. Actual JS retry and callback machinery remained intact.
Head: cached flags, errorsLoading=true, elapsedMs=6003
Later: 2 JS fetch attempts; fresh flags; successful notification
Assertion expected errorsLoading=false: FAIL
Ordinary successful-request control: PASS, fresh flags, ~6 ms
The equivalent base run observes the same retry and eventual fresh flags, but returns void/null immediately. Its controls pass; this is not a passing base error-result assertion, because that API did not exist.
Exact executed commands:
bash /Users/db/dev/review-evidence/flutter-664/run-validation.sh \
head test --no-pub --reporter expanded --platform chrome \
test/real_default_js_reload_test.dart
# exit 1: retry assertion fails; ordinary control passes
bash /Users/db/dev/review-evidence/flutter-664/run-validation.sh \
base test --no-pub --reporter expanded --platform chrome \
--dart-define=REVIEW_BASE=true test/real_default_js_reload_test.dart
# exit 0: both base controls passThe complete fixture, readable template and logs remain under the evidence directory. The published tarball SHA1 was verified as d06c1d7727bd872ed34bce04af794d205023d138. This was deterministic browser execution, not a live backend test.
Consider deriving the fallback from the full supported retry budget, including configured retries, backoff and scheduling, and adding this regression beside the no-notification test. Requiring newer JS instead would need an explicit support-policy decision. No fix was applied or claimed tested.
There was a problem hiding this comment.
Thanks for the repro! Fixed in a406020: the older-JS deadline now covers every attempt posthog-js may make, (feature_flag_request_max_retries + 2) × timeout + retries × 500 ms, and there's a regression test next to the no-notification one that fails the same way your run did without the fix.
| /// It receives the enabled flags and whether loading failed. It runs after | ||
| /// the flags load at startup, after [Posthog.reloadFeatureFlags], and after | ||
| /// calls that reload flags such as `identify()` or `group()`. When a load | ||
| /// fails, [PostHogFeatureFlagsLoaded.errorsLoading] is `true` and the flags |
There was a problem hiding this comment.
2. [P2] Qualify the callback lifecycle documentation for Android
MINOR · OPTIONAL · New documentation overstates unchanged runtime behavior
Location: posthog_flutter/lib/src/posthog_config.dart:268–270, right side.
The new text promises that config.onFeatureFlags runs after manual Posthog.reloadFeatureFlags(), but Android delivers only the supplied per-call completion on that path.
config.onFeatureFlags = (loaded) {
// Rebuild feature-dependent UI.
};
// Later:
final loaded = await Posthog().reloadFeatureFlags();The returned result works, but this configuration listener is not separately notified on Android. The Flutter plugin supplies its reload completion at PosthogFlutterPlugin.kt:1933–1937; Android’s reload implementation passes it through, and remote-config callback delivery does not additionally invoke the configuration callback. Setup/identity/group flows explicitly supply that callback instead.
Consider qualifying the Android documentation and directing manual-refresh callers to the returned result, rather than changing native callback policy here.
Verification: Deterministic source call-graph comparison. The manual-reload structure exists on base; only the unconditional documentation guarantee is new. Native runtime execution was not performed.
There was a problem hiding this comment.
Good point, confirmed that manual reload on Android only passes the per-call callback. Qualified in 0e8a97b and pointed callers at the returned result.
| /// Whether the latest attempt to load feature flags failed. | ||
| /// | ||
| /// When it did, [flags] and [variants] hold the last known values. | ||
| final bool errorsLoading; |
There was a problem hiding this comment.
3. [P2] Document that browser replay lacks historical load-error context
MINOR · OPTIONAL · New documentation overstates an existing upstream representation
Location: posthog_flutter/lib/src/feature_flags_loaded.dart:30–33, right side.
On web, errorsLoading: false on an immediate callback replay does not necessarily mean the latest load succeeded.
The host initializes JS before Flutter setup. If its flags request fails before Flutter registers config.onFeatureFlags, JS can replay cached values without error context. Both current browser replay and legacy posthog-featureflags.ts:1677–1685 call the listener with only flags and variants. The legacy failed-response path still marks flags loaded. Flutter then defaults absent context to false in channel_serialization.dart:187.
An ordinary startup callback can therefore receive cached flags and errorsLoading: false after a failed JS load, contrary to the unqualified “whether the latest attempt … failed” wording. Live failed-load notifications still carry their error context.
Consider documenting the browser replay limitation. Preserving historical error state would require separate upstream/wrapper work and need not expand this PR.
Verification: Deterministic source proof; no dedicated replay regression was executed. This is a documentation qualification, not an introduced upstream runtime defect or a claim based on the newer listener scenario absent from the authoritative default branch.
There was a problem hiding this comment.
Agreed, documented the web replay limitation in 0e8a97b. Keeping historical error context would need upstream work, so it stays out of this PR.
… failure posthog-js retries a timed-out /flags request once by default, 500 ms later, inside the same reload. The older-JS deadline of twice the request timeout reported cached flags with errorsLoading set before a successful retry landed.
💡 Motivation and Context
PostHogConfig.onFeatureFlagswas avoid Function()andreloadFeatureFlags()returnedFuture<void>, so Flutter apps couldn't tell what loaded or whether loading failed. posthog-ios 4.0 passes this to its listener and reload callback (PostHog/posthog-ios#897, #913), posthog-android 4.0 gets the same (PostHog/posthog-android#877), and posthog-js already passes it toonFeatureFlags.Closes #231
Both now carry a new
PostHogFeatureFlagsLoaded:flagsvariantstrueor the variant string; disabled flags left outerrorsLoadingtruewhen the latest load failed; the values are then the last known onesPer platform:
onFeatureFlagsreloadFeatureFlags()onFeatureFlagsreloadFeatureFlags(_:)PostHogFeatureFlagsCallbackreloadFeatureFlagsonFeatureFlagsfeatureFlags.reloadFeatureFlagsAsync(), which settles after this reload's own request (not one already in flight);errorsLoadingunless it loaded. Older posthog-js: the nextonFeatureFlagscall, with a timeout of twicefeature_flag_request_timeout_msfromMapstays internal (channel_serialization.dart), in line with #646.Breaking:
onFeatureFlags = () { … }becomes(loaded) { … }or(_) { … }. Callers ofreloadFeatureFlags()are unaffected; custom platform interface implementations must return the new type.Depends on #663 (posthog-android 4.0). The Android code is written against the
PostHogFeatureFlagsLoadedposthog-android 4.0 adds, which is still in review, so this stays a draft until that API is final.💚 How did you test it?
flutter analyze,flutter test(1258 pass),dart format,scripts/check-api-dart.sh(snapshot updated).reloadFeatureFlags()returns the loaded flags, and after a failed request returns the last known flags witherrorsLoading: true, as doesonFeatureFlags. The desktop test fails whenerrorsLoadingis hardcoded tofalse.flutter build ios --simulatoragainst a local posthog-ios 4.0 checkout (temporary.package(path:), reverted), andflutter build web.reloadFeatureFlagsawaitsreloadFeatureFlagsAsync()when present and reportserrorsLoadingwhen posthog-js skips the request; the first test fails without that path.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Claude Code (Claude Opus 5.5) made this as part of the feature flag parity work across the iOS, Android and Flutter majors. The field names mirror posthog-ios's
PostHogFeatureFlagsLoaded. For webreloadFeatureFlags(), waiting only for the next posthog-jsonFeatureFlagscall could return a response from a request already in flight, and never fires when posthog-js skips the request, so newer posthog-js usesreloadFeatureFlagsAsync()and older versions keep that path behind a timeout.🤖 Generated with Claude Code