fix(analytics): bootstrap landing-variant server-side & stop early identify (#1188)#1189
Open
kilbot wants to merge 3 commits into
Open
fix(analytics): bootstrap landing-variant server-side & stop early identify (#1188)#1189kilbot wants to merge 3 commits into
kilbot wants to merge 3 commits into
Conversation
…ntify The landing-variant A/B test was stuck on the free-plus fallback for every visitor. Two root causes: 1. The self-hosted PostHog /flags endpoint rejects the public project token (401) while /capture works, so posthog-js never resolves the flag over the network (zero render_source:flag events). The infra needs a separate fix. 2. The plugin's early posthog.init + identify() on the welcome page wrote the bundle's shared `ph_<token>_posthog` localStorage identity first, flipping the bundle's hasPersistedIdentity() gate and breaking the deliberate flag-before-identify exposure ordering. Fix (plugin side): - Add Services\Feature_Flags: resolve landing-variant via PostHog's standard consistent-hash locally (no network, no secret), so assignment matches the server SDK's get_all_feature_flags() and works regardless of the broken /flags endpoint. - Landing_Profile: inject `bootstrap_flags` into the always-present functional data (anon, consent-independent) so the bundle can seed posthog.init bootstrap.featureFlags at first paint. - Menu: stop initialising PostHog / identifying on the welcome page and remove the redundant DOM CTA tracker (it double-fired upgrade_cta_clicked against the bundle's own React tracking). The bundle now solely owns PostHog init + identify ordering. Companion bundle change required: wcpos/wp-admin-landing#39 (read bootstrap_flags into bootstrap.featureFlags). This PR is necessary but the experiment only goes green once both ship. Refs #1188
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: wcpos/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new ChangesLanding Variant Bootstrap & PostHog Init Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E UI Test Results24 tests 24 ✅ 1m 7s ⏱️ Results for commit fb8a325. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/includes/Admin/Test_Menu_Landing_Bootstrap.php`:
- Around line 26-27: The setUp() method modifies the general settings
(tracking_consent = allowed), but tearDown() doesn't restore the original
values, causing test isolation issues for subsequent test suites. Store the
original state of the general settings before modification in setUp(), then
restore those original values in tearDown() to ensure proper cleanup and
maintain test isolation across the test suite.
In `@tests/includes/Services/Test_Feature_Flags.php`:
- Around line 129-138: The filter cleanup using remove_filter for
woocommerce_pos_landing_bootstrap_flags in the
test_bootstrap_flags_filter_can_override_variant method is not exception-safe.
If get_landing_bootstrap_flags throws an exception, the remove_filter call will
not execute, causing the filter to leak into subsequent tests. Wrap the call to
get_landing_bootstrap_flags and the assertions in a try-finally block, moving
the remove_filter call into the finally block to ensure the filter is always
cleaned up regardless of whether an exception occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: wcpos/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 49f79754-e3a5-490f-ace2-3830b80990dc
📒 Files selected for processing (6)
includes/Admin/Menu.phpincludes/Services/Feature_Flags.phpincludes/Services/Landing_Profile.phptests/includes/Admin/Test_Menu_Landing_Bootstrap.phptests/includes/Services/Test_Feature_Flags.phptests/includes/Services/Test_Landing_Profile.php
Contributor
Contributor
|
Fix round triage before changes:
CI scope: current PR checks are passing; no failing CI item to fix in this round. |
Open
5 tasks
Contributor
|
Review-fix triage after push:
Excluded threads: none.
Validation:
Post-push unresolved inline review threads: 0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the
landing-variantA/B test on the wp-admin welcome page, which was stuck on thefree-plusfallback for every visitor (issue #1188).Services\Feature_Flags— resolveslanding-variantserver-side, locally, using PostHog's standard consistent-hash algorithm (the same one the server SDKs'get_all_feature_flags()uses). No network call, no secret.Landing_Profile::get_functional_data()now injectsbootstrap_flagsinto the always-present, consent-independent functional data, keyed by the per-siteanon_id.Menuwelcome page — removed the earlyposthog.init()+identify()inline script and the redundant DOM CTA tracker. Thewp-admin-landingbundle now solely owns PostHog init and the flag-before-identify ordering.Root cause (two compounding bugs)
ph.wcpos.com:POST /capture/→ 200 (token valid), butPOST /flags/?v=2&config=true→ 401 "API key invalid" andPOST /decide/?v=3→ 403 CSRF. Soposthog-jsnever resolves the flag over the network — hence zerorender_source:flagevents across all 337 visitors (a 500 ms timeout would have let some through; zero means systematic endpoint failure). This is an infra bug that still needs a separate fix — see "Follow-ups". The team'sget_all_feature_flags()"works" only because it's the Python SDK doing local evaluation with a personal API key, never touching/flags.posthog.init+identify(user_uuid)in an inline script that runs beforewelcome.js. Both PostHog instances share theph_<token>_posthoglocalStorage key, so this flipped the bundle'shasPersistedIdentity()gate and poisoned the anon-bucket exposure (spec §5.1 flag-before-identify).The issue's suggested fix (bootstrap inside the plugin's
posthog.init, server-side/decide) is not viable: that init is consent-gated so it never runs for the anonymous majority, and/decideis the broken endpoint. This PR instead delivers the variant through the always-present landing data and removes the plugin's PostHog involvement on that page entirely.Design decisions (preserve through rebases)
/decidenetwork call — deliberate, because the self-hosted/flags//decideendpoints reject the public token (see root cause). Local hashing replicates PostHog's exact algorithm (verified against a reference implementation — all 6 test vectors match), so assignment agrees withget_all_feature_flags()and is independent of the broken endpoint.posthog.init/identifyandlanding_tracking_inline_script()— not dead-code cleanup. The bundle owns PostHog; the plugin's early init poisoned shared localStorage and the DOM CTA tracker double-firedupgrade_cta_clickedagainst the bundle's own React tracking.Menu::get_posthog_inline_script()is kept — still used by the Settings and wc-Analytics pages.bootstrap_flagsis consent-independent — it's anonymous (random per-site UUID, no PII) and the experiment runs pre-consent by design, so it ships in the functional tier, not the consented tier.@since 1.9.7on the new filter assumes the next patch release off1.9.6— adjust if the release is numbered differently.Companion PRs / cross-repo
data.bootstrap_flagsintoposthog.init'sbootstrap.featureFlags. This PR is necessary but not sufficient on its own — the experiment only goes green once both ship. (This PR alone still stops the localStorage poisoning and the double-counted CTA event.)Test plan
Test_Feature_Flags,Test_Menu_Landing_Bootstrap, andbootstrap_flagscases inTest_Landing_Profile.window.wcpos.landingcontainsbootstrap_flags: { "landing-variant": "indie" | "free-plus" }, and there is noposthog.init/posthog.identifyinline script.get_posthog_inline_script()unchanged).landing_variant_renderedevents showrender_source: flagwith a ~50/50 split and a non-empty$feature_flag_response.Local validation note
composer run lint(PHPCS),composer run phpstan(level 5), andphp -lall pass on every changed file./review(correctness) and/critic(architecture) gates: both "ship it", no blocking findings.docker pullof evenalpinestalls). Per project policy this is not worked around with host PHPUnit — CI is the sanctioned runner. The new tests are written to the repo's conventions and the hash logic was independently verified against a reference implementation.Follow-ups (out of scope here)
ph.wcpos.com/flags+/decideendpoints so the public token authenticates — required for client-side flag reloads, experience-continuity, and any future experiment flag./flagsis down). Consider a detectable server-side exposure property so a future PostHog-side config change can't silently skew the split.Refs #1188
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests