Repository navigation
fix(flags): apply default release conditions on new flag creation - #65356
Conversation
…tion defaultReleaseConditionsLogic.loadDefaultReleaseConditions() fires in afterMount but is async. featureFlagLogic.loadFeatureFlag() also fires in afterMount and synchronously reads values.defaultReleaseConditions, which is still null when the API hasn't responded yet — so default conditions are never applied. Fix: if values.defaultReleaseConditions is null when loading a new flag, fetch it directly in the loader rather than relying on the parallel load from defaultReleaseConditionsLogic to win the race. Fixes PostHog#63662
|
Reviews (1): Last reviewed commit: "fix(feature-flags): apply default releas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
haacked
changed the title
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Fix looks correct, and the race-condition diagnosis is solid. A couple of non-blocking suggestions inline, the main one being test coverage for the new fallback path. One scope question: this closes #63662 for fresh flag creation, but applyTemplate (featureFlagLogic.ts:2008) reads values.defaultReleaseConditions the same synchronous way and isn't touched here, so the same bug can still recur when a user applies a template on a cold cache. Worth either fixing or filing a follow-up. I also agree with @greptile-apps's duplicate-fetch point; the inline shared-helper suggestion covers it.
…tion - Extract fetchDefaultReleaseConditions helper to defaultReleaseConditionsLogic so the API URL lives in one place and both call sites reuse it - Apply same fallback fetch pattern to applyTemplate, which has the identical race when called on a cold cache before the async loader resolves - Add test covering the 500 error path: flag creation must not be blocked when the default_release_conditions endpoint is unavailable
|
cleaned up — extracted |
|
The changes land exactly what was asked for. A few notes: What's implemented correctly:
One remaining open thread (the first review comment) — the duplicate concurrent request: The extracted helper resolves the duplicate URL maintenance issue, but the two concurrent requests on cold cache still happen: This is a behavioral issue, not just style. That said, the tradeoff is real — coordinating on The test coverage question you raised: The 500 test is the right one to have — it's the only test that actually exercises the new fallback block in isolation. A success-path test using a 200 mock would pass through Overall this is in good shape. The main open question is whether the duplicate cold-cache request is acceptable — happy to close the first thread if you've decided it's fine as-is. Tip: You can customize Greptile's behavior for this repo with |
haacked
added
feature/feature-flags
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Updates look good. The helper extraction, the applyTemplate fix, and the 500 test all close out the earlier threads. Two non-blocking suggestions inline, plus a question on the cold-cache duplicate request greptile raised.
| const templateValues = template.getValues(values.featureFlag) | ||
|
|
||
| const defaultConfig = values.defaultReleaseConditions | ||
| let defaultConfig: DefaultReleaseConditionsResponse | null = values.defaultReleaseConditions |
There was a problem hiding this comment.
suggestion: Nothing asserts the merge this block feeds: applyTemplate now prepends the org default groups ahead of the template's own groups ([...defaultGroups, ...templateGroups]). The new 500 test covers loadFeatureFlag, which sets defaults directly without merging, so this ordering runs nowhere in the suite. If it breaks, a flag created from a template ships without the org defaults and rolls out wider than configured.
This path isn't theoretical: on ?template=… new-flag creation, loadFeatureFlag fetches the config into a local and never writes it to the cache, so values.defaultReleaseConditions is still null when applyTemplate runs and it takes this fetch branch.
Mock default_release_conditions to return enabled: true with a non-empty default_groups, apply a template that contributes its own groups, and assert filters.groups equals [...defaultGroups, ...templateGroups]. The existing applyTemplate tests have the scaffold to mirror.
…ache extract resolveDefaultReleaseConditions helper that handles the cached-else-fetch-else-warn pattern; both loadFeatureFlag and applyTemplate now call it instead of duplicating the try/catch block. also write the fetched result back to defaultReleaseConditionsLogic's cache so a cold-cache new-flag creation followed by applyTemplate doesn't issue a third redundant request. add test covering applyTemplate group merge ordering when default conditions are enabled
d300a75 to
7225389
Compare
|
Please don't force push after I've approved your changes. |
There was a problem hiding this comment.
Pull request overview
Fixes a race in the feature flags scene where “Default release conditions” weren’t applied because defaultReleaseConditionsLogic loads asynchronously, while new-flag initialization (loadFeatureFlag) and template application (applyTemplate) previously read values.defaultReleaseConditions synchronously.
Changes:
- Add
fetchDefaultReleaseConditions+resolveDefaultReleaseConditionshelpers to allow on-demand retrieval when the cached value hasn’t loaded yet. - Use
resolveDefaultReleaseConditionsin new-flag creation and inapplyTemplateto avoid the mount-time race. - Add Jest coverage for template group merging and the “endpoint returns 500” fallback behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/scenes/feature-flags/featureFlagLogic.ts | Uses resolveDefaultReleaseConditions during new-flag initialization and template application to avoid async-load races. |
| frontend/src/scenes/feature-flags/featureFlagLogic.test.ts | Adds tests for prepending default groups and for non-blocking behavior when defaults endpoint errors. |
| frontend/src/scenes/feature-flags/defaultReleaseConditionsLogic.ts | Extracts shared fetch + “cached-or-fetch” resolution helpers used by other logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
The warm-cache commit added after my approval regressed a couple of things: the suite is red and loadFeatureFlag throws on the cold-cache path (the exact race this PR fixes). Three blocking inline, one test suggestion. Verified by running the two new tests.
| }).toDispatchActions(['applyTemplate', 'setFeatureFlag']) | ||
|
|
||
| const groups = logic.values.featureFlag.filters.groups | ||
| expect(groups[0]).toMatchObject(DEFAULT_GROUP) |
There was a problem hiding this comment.
blocking: This test fails on a clean checkout, groups[0] is the template's targeted group, not DEFAULT_GROUP, so the suite is red.
The global mock at frontend/src/mocks/jest.ts:20 returns {enabled: false, default_groups: []}. Mounting logic in the outer beforeEach warms the connected defaultReleaseConditionsLogic singleton to that disabled value before this test's useMocks(...200, enabled:true) runs, so applyTemplate's resolveDefaultReleaseConditions hits the if (cached) return cached short-circuit and never sees the enabled mock. Nothing gets prepended.
The feature is likely fine in prod; the test just isn't reaching the path. Defeat the pre-warm so the per-test mock drives the value, keep the singleton cold (or reload it) after the enabled useMocks and before dispatching applyTemplate, then re-assert the prepend.
|
@okxint I'm going to push some commits to this branch to try and get this to pass CI. Please don't force push over my commits. |
Remove the loadDefaultReleaseConditionsSuccess write-back in loadFeatureFlag. The action is not connected to featureFlagLogic, so the call threw on the cold-cache path and dispatched loadFeatureFlagFailure, breaking new flag creation in exactly the race this change fixes. The resolve-on-demand fix for the race stays. Rework the tests so they exercise the real paths. The global test mock pre-warms the shared defaultReleaseConditionsLogic singleton to a disabled value before per-test mocks apply, so the prepend test was failing and the 500 test passed without ever issuing the request. Seed the singleton cache directly for the integration tests, add a test that a new flag receives the org default groups, and unit-test resolveDefaultReleaseConditions directly.
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I'll take it from here.
Problem
defaultReleaseConditionsLogicloads its data async on mount.loadFeatureFlagalso runs on mount and readsvalues.defaultReleaseConditionssynchronously — which is always null at that point. So default release conditions never got applied to new flags.Closes #63662
Changes
When
values.defaultReleaseConditionsis null,loadFeatureFlagnow fetches it directly before building the initial flag config. If the cached value is already there (warmed cache), it uses that. If the fetch fails, it logs a warning and proceeds without defaults so flag creation isn't blocked.Also fixed the same race in
applyTemplatewhich readsvalues.defaultReleaseConditionsthe same way.Pulled the fetch into a
fetchDefaultReleaseConditionshelper indefaultReleaseConditionsLogic.tsso the url isn't duplicated.How did you test this code?
Added a test for the error path — mocks the endpoint to 500 and checks that
loadFeatureFlagSuccessstill fires with the default empty groups. The success path is harder to pin in Jest because the connected logic auto-loads on mount via the same url, so a 200 mock would pass even without the fallback.Automatic notifications
Docs update
N/A
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Used Claude Code to trace the kea afterMount lifecycle and identify both the original race in
loadFeatureFlagand the same issue inapplyTemplate. The fix pattern (check cached value, fetch directly if null, warn on error) was chosen to avoid cross-logic coordination which kea doesn't support cleanly.