fix: address Codex review comments (preview flags, fresh kill-switch)#30
Conversation
#29 — restore fresh kill-switch on cache hits. The synchronous exposure read only sees the persisted kill-switch value; a switch flipped after the last load wouldn't clear the cache. Add watchKillSwitch(ph): a background onFeatureFlags watch that re-reads the kill-switch with send_event:false (non-exposure, so no post-identify bucket concern — the ops flag is bucket-agnostic) and clears the assignment cache so the kill takes effect next load (spec §3.1.4). The experiment-flag exposure read stays synchronous + pre-identify. #23 — preview must not resolve production flags. Add advanced_disable_flags in preview so the Pages harness never fetches landing-variant (a live flag could otherwise win over the seeded assignment) and preview iframe loads don't hit the production flag endpoint. Verified both seeded variants render deterministically. Extends tests/exposure-ordering.test.mjs.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: wcpos/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a ChangesKill-switch watcher and preview flag isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35a9e24eb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (killValue === true || killValue === 'on') clearCache(); | ||
| unsub?.(); | ||
| }); | ||
| if (fired) unsub(); |
There was a problem hiding this comment.
On cache-served repeat visits, persisted PostHog flags exist, and the nearby resolveFlag comment already notes that onFeatureFlags can fire inline in that state. This guard immediately unsubscribes the new watcher after that stale cached callback, so it observes only the same value recordCachedExposure already read and is gone before the post-identify flag reload that should contain a freshly flipped kill-switch. In that scenario, a kill-switch enabled after the prior visit will not clear wcpos_landing_assignment for the next load.
Useful? React with 👍 / 👎.
) watchKillSwitch copied resolveFlag's unsubscribe-after-first-fire guard, but on a cached repeat visit posthog-js fires onFeatureFlags inline with the STALE persisted flags — so it read the same value recordCachedExposure already saw and unsubscribed before the fresh network reload, meaning a kill-switch flipped since the last visit never cleared the assignment cache (the exact case it was meant to handle). Removed the guard: stay subscribed, clear the cache on any observed ON value (fresh reload / post-identify reload), bounded by an 8s timeout so the listener never lingers. Pins the regression in tests/exposure-ordering.test.mjs.
…Codex on #30) (#32) * fix: kill-switch watch must survive the stale inline callback (Codex #30) watchKillSwitch copied resolveFlag's unsubscribe-after-first-fire guard, but on a cached repeat visit posthog-js fires onFeatureFlags inline with the STALE persisted flags — so it read the same value recordCachedExposure already saw and unsubscribed before the fresh network reload, meaning a kill-switch flipped since the last visit never cleared the assignment cache (the exact case it was meant to handle). Removed the guard: stay subscribed, clear the cache on any observed ON value (fresh reload / post-identify reload), bounded by an 8s timeout so the listener never lingers. Pins the regression in tests/exposure-ordering.test.mjs. * test: assert the documented 8s kill-switch watch timeout (CodeRabbit #32) * docs: align watchKillSwitch docblock with persistent-subscription design (CodeRabbit #32) The docblock at lines 95-96 still described the old synchronous-fire guard that unsubscribes after the first reload. The implementation deliberately dropped that guard so the watch survives the stale inline callback and picks up the fresh network reload. Update the wording to match.
1 participant
Addresses the two open Codex P2 comments from the rapid landing-page PR series (the P1 cached-exposure one was already fixed in #29; the plugin #1146 P2s were fixed in 3c6bde58).
wp-admin-landing#27 thread (P2) — fresh kill-switch on cache hits. My #29 fix made the kill-switch read synchronous-only, so a switch flipped after the visitor's last load wouldn't clear the cache. Adds
watchKillSwitch(ph): a backgroundonFeatureFlagswatch that re-reads the kill-switch withsend_event:false(non-exposure → safe post-identify, the ops flag is bucket-agnostic) and clears the assignment cache so the kill takes effect next load. The experiment-flag exposure read stays synchronous + pre-identify (#29 invariant intact).wp-admin-landing#23 thread (P2) — preview must not touch production flags. Adds
advanced_disable_flags: previewso the Pages harness never resolveslanding-variant(a live flag could otherwise override the seeded preview assignment) and preview iframe loads don't hit the production flag endpoint. Verified both seeded variants render deterministically.Tests extended in
tests/exposure-ordering.test.mjs.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests