Merged
Conversation
Contributor
|
Size Change: +35.2 kB (+0.03%) Total Size: 112 MB
ℹ️ View Unchanged
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
1520
to
1542
|
|
||
| actions.setTemplateExpanded(false) | ||
| actions.applyUrlTemplate(templateId) | ||
|
|
||
| // Apply intent after template so intent presets are not overwritten | ||
| if (!values.urlIntentApplied) { | ||
| const rawIntent = router.values.searchParams.intent | ||
| const intent = VALID_INTENTS.includes(rawIntent) ? (rawIntent as FlagIntent) : undefined | ||
| if (intent) { | ||
| actions.setFlagIntent(intent) | ||
| actions.applyUrlIntent() | ||
| if (intent === 'local-eval') { | ||
| actions.setFeatureFlag({ | ||
| ...values.featureFlag, | ||
| evaluation_runtime: FeatureFlagEvaluationRuntime.SERVER, | ||
| ensure_experience_continuity: false, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| copyFlagSuccess: ({ featureFlagCopy }) => { | ||
| if (featureFlagCopy?.success.length) { |
Contributor
There was a problem hiding this comment.
Duplicated intent-application block (OnceAndOnlyOnce)
The same "read intent from URL, validate, call setFlagIntent / applyUrlIntent / setFeatureFlag" logic appears twice: once inside the loadFeatureFlag success handler (when no template is in the URL) and again at the end of the applyTemplate handler (when a template is present). Both blocks are character-for-character identical apart from the surrounding guard condition.
Extract a shared helper action (or shared-listener) to keep this logic in one place:
// Suggested extracted listener (illustrative)
applyUrlIntentIfNeeded: () => {
if (values.urlIntentApplied) {
return
}
const rawIntent = router.values.searchParams.intent
const intent = VALID_INTENTS.includes(rawIntent) ? (rawIntent as FlagIntent) : undefined
if (intent) {
actions.setFlagIntent(intent)
actions.applyUrlIntent()
if (intent === 'local-eval') {
actions.setFeatureFlag({
...values.featureFlag,
evaluation_runtime: FeatureFlagEvaluationRuntime.SERVER,
ensure_experience_continuity: false,
})
}
}
},Then both call sites become a single actions.applyUrlIntentIfNeeded(), eliminating the duplication. The same pattern already exists for applyUrlTemplate / urlTemplateApplied.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Additional Comments (2)
The cohort objects passed to Consider importing Context Used: Rule from Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The union
import type { FlagIntent } from 'scenes/feature-flags/featureFlagIntentWarningLogic'Then the param type becomes simply |
Contributor
Visual regression: Storybook UI snapshots updatedChanges: 2 snapshots (2 modified, 0 added, 0 deleted) What this means:
Next steps:
|
dmarticus
reviewed
Mar 4, 2026
Contributor
dmarticus
left a comment
dmarticus
left a comment
There was a problem hiding this comment.
Nice work — the intent system is well-designed for extensibility and the test coverage is solid. A few comments inline, nothing blocking.
| } | ||
|
|
||
| // Apply intent from URL param (when no template, or template already applied) | ||
| if (!templateId && featureFlag && !values.urlIntentApplied) { |
Contributor
There was a problem hiding this comment.
This 13-line block is duplicated verbatim in the applyTemplate listener below. If a new intent is added (e.g. 'edge-eval'), both sites need updating.
Consider extracting a helper:
function maybeApplyUrlIntent(values, actions) {
if (values.urlIntentApplied) return
const rawIntent = router.values.searchParams.intent
const intent = VALID_INTENTS.includes(rawIntent) ? (rawIntent as FlagIntent) : undefined
if (intent) {
actions.setFlagIntent(intent)
actions.applyUrlIntent()
if (intent === 'local-eval') {
actions.setFeatureFlag({
...values.featureFlag,
evaluation_runtime: FeatureFlagEvaluationRuntime.SERVER,
ensure_experience_continuity: false,
})
}
}
}|
|
||
| const intentsEnabled = !!featureFlags[FEATURE_FLAGS.FEATURE_FLAG_CREATION_INTENTS] | ||
| const [selectedTemplate, setSelectedTemplate] = useState<FeatureFlagTemplate | null>(null) | ||
|
|
Contributor
There was a problem hiding this comment.
Convention note: PostHog prefers kea over useState for scene-level components. featureFlagTemplatesSceneLogic already has selectedTemplate / setSelectedTemplate state that could be reused here, or a small dedicated logic could own this.
| import { INTENT_KEYS, INTENT_METADATA } from 'products/feature_flags/frontend/featureFlagTemplateConstants' | ||
|
|
||
| type FeatureFlagTemplate = 'simple' | 'targeted' | 'multivariate' | 'targeted-multivariate' | ||
|
|
Contributor
There was a problem hiding this comment.
This type and the TEMPLATE_METADATA below duplicate TemplateKey and TEMPLATE_NAMES from products/feature_flags/frontend/featureFlagTemplateConstants.ts, with slightly different labels ("Simple flag" here vs "Percentage rollout" there). The divergent names could confuse users navigating between the dropdown and the templates page. Consider importing TemplateKey and consolidating the metadata.
| } | ||
|
|
||
| if (slowPropertyKeys.length === 1) { | ||
| issues.add( |
Contributor
There was a problem hiding this comment.
When Group 1 has one slow property email (producing 'The property "email" won't be...') and Group 2 has two slow properties (producing '2 properties won't be...'), both messages appear because they are different strings. Users see both but can't tell which properties Group 2 refers to.
Consider collecting all slow properties across all groups first, then producing a single unified message — or always naming the properties regardless of count.
| if (property.type === PropertyFilterType.Cohort) { | ||
| const cohortId = property.value | ||
| const cohort = cohortsById[cohortId] ?? cohortsById[String(cohortId)] | ||
| if (cohort?.is_static) { |
Contributor
There was a problem hiding this comment.
Nit: The double lookup suggests cohortId can be either a number or a string. A brief comment explaining this known quirk would save future readers from wondering if it's a bug.
// cohortId may be numeric or stringified depending on filter source
const cohort = cohortsById[cohortId] ?? cohortsById[String(cohortId)]| } | ||
|
|
||
| const INTENT_CONSEQUENCE: Record<FlagIntent, string> = { | ||
| 'local-eval': |
Contributor
There was a problem hiding this comment.
Intent metadata is now scattered across three files:
INTENT_METADATA(name, description, icon) infeatureFlagTemplateConstants.tsINTENT_CONSEQUENCEhereintentDocUrlinfeatureFlagIntentWarningLogic.ts
Co-locating all of these in featureFlagTemplateConstants.ts would make adding a new intent a single-file change.
| className: 'new-feature-flag-overlay', | ||
| actionable: true, | ||
| closeOnClickInside: false, | ||
| overlay: <OverlayForNewFeatureFlagMenu />, |
Contributor
There was a problem hiding this comment.
Nit: A brief comment explaining WHY this is needed (the intent submenu adds a second step inside the overlay) would help future maintainers who might otherwise remove it.
Contributor
|
Overall this is well-structured and the intent system is nicely designed for extensibility. I'd suggest addressing the duplicated intent-application block in |
ordehi
force-pushed
the
feat/feature-flag-creation-intents
branch
from
7bf481f to
1449849
Compare
…warnings Add evaluation intent declaration to V2 flag creation flow. Users pick an intent (local evaluation or first page load) and the form surfaces contextual warnings scoped to that intent. Also adds unreachable conditions detection as an always-on warning. - New featureFlagIntentWarningLogic with structured per-group warnings - Intent cards on the template scene (gated behind FEATURE_FLAG_CREATION_INTENTS) - flagIntent action/reducer in featureFlagLogic with ?intent URL param - Warning banners in FeatureFlagReleaseConditionsCollapsible
…aming Integrate intent selection as a second step after template selection instead of a separate section. Rename "First page load" to "Prevent flicker" and rewrite warning copy to describe symptoms users experience.
For prevent-flicker intent, cohort and non-instant property warnings now produce a single flicker_risk warning listing all issues instead of separate banners per property. Local-eval warnings are also deduplicated to one per category.
…ry property Count slow properties and cohorts instead of enumerating each one, so the warning stays compact regardless of how many filters are added.
…only ones Property-only dynamic cohorts work fine for local evaluation. Now checks cohort criteria types and only warns when behavioral filters (events, sequences) are present. Static cohorts still always warn.
When intents are enabled, clicking a template in the dropdown drills into an intent submenu (local eval, prevent flicker, skip). Uses closeOnClickInside: false so the popover stays open during drill-down. Remote config and experiment items still navigate directly. Also adds intent param to featureFlagNew URL helper and regenerates products.tsx.
…ocal eval Add flag-level warnings via a new flagWarnings selector. When the local-eval intent is set and ensure_experience_continuity is enabled, show a warning that this setting requires server-side state.
Replace per-warning banners with a single collapsible "N issues detected" banner. Expanding shows a consequence message explaining the impact (e.g., forced server request for local eval) followed by a bullet list of detected issues and a single shared doc link. - Simplify data model: logic produces flat string[] instead of ConditionWarning objects for intent issues - Remove regex check (already handled by existing global warning) - Improve "is not set" message to explain why it breaks local eval - Unreachable conditions remain as inline banners per group
- Fix race condition: apply intent after template in applyTemplate listener - Add idempotency guard (urlIntentApplied) to prevent re-application on reload - Validate intent URL param against VALID_INTENTS before use - Simplify unreachableGroups from O(n²) to O(n) with foundBroad flag - Remove redundant feature flag gate from intentDocUrl selector - Replace inline style with Tailwind min-h-[80vh] - Consolidate intent metadata into single source of truth in featureFlagTemplateConstants.ts (removes duplicates from FeatureFlagTemplatesScene.tsx and NewFeatureFlagMenu.tsx) - Add test coverage for static cohort, behavioral cohort, and singular/plural flicker property messages
…elper maybeApplyUrlIntent() replaces two identical 13-line blocks in loadFeatureFlagSuccess and applyTemplate listeners. Adding a new intent now only requires updating VALID_INTENTS and the helper.
Move consequence strings and doc URLs into INTENT_METADATA in featureFlagTemplateConstants.ts. Remove intentDocUrl selector and INTENT_CONSEQUENCE constant. Adding a new intent is now a single-file change.
…gMenu Import TemplateKey from featureFlagTemplateConstants instead of duplicating the type locally. Keep dropdown-specific display metadata (shorter labels for compact menu).
Extract IntentWarningsBanner and UnreachableConditionBanner into separate components that only mount featureFlagIntentWarningLogic when flagId is provided. Fixes unintended side effects when DefaultReleaseConditions renders FeatureFlagReleaseConditionsCollapsible without a flag context. Also fixes merge conflict artifact in FeatureFlagOverviewV2 (broken render prop pattern), sentence casing in template names, and adds explicit ComponentType import.
Add tests for unreachable groups + intent issues firing simultaneously, invalid intent string producing no side effects, and a full test suite for featureFlagTemplatesSceneLogic (selectedTemplate reducer, intentsEnabled selector, afterMount redirect behavior).
Move IntentIssuesSummary expand/collapse state into featureFlagIntentWarningLogic as issuesExpanded reducer and toggleIssuesExpanded action, replacing local useState. Also fixes setFeatureFlag parameter type in maybeApplyUrlIntent helper.
Move handleTemplateSelect business logic (analytics capture, conditional navigation vs template selection) from FeatureFlagTemplatesScene component body into featureFlagTemplatesSceneLogic as a selectTemplate listener. Also co-locates navigateToNewFlag helper in the logic file.
The templates scene test added in a9ef2b4 imports kea-test-utils but the product package.json was missing the dependency, breaking the TypeScript check in CI.
ordehi
force-pushed
the
feat/feature-flag-creation-intents
branch
from
0804804 to
d30c024
Compare
dmarticus
approved these changes
Mar 9, 2026
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
2 participants
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.
What this does
Adds evaluation intent declaration to the V2 feature flag creation flow. When creating a flag, users now declare how they intend to use it "Local evaluation" or "Prevent flicker" and the form surfaces contextual warnings scoped to that intent. Also adds unreachable condition detection as an always-on structural warning.
All work is behind FEATURE_FLAG_CREATION_INTENTS (requires FEATURE_FLAGS_V2).
Why this matters
Feature flags are deceptively easy to misconfigure. A user who wants local evaluation might add a static cohort (not sent to SDKs), use is_not_set (unknowable without the full property list), or enable experience continuity (requires server state). Today, the form shows nothing, the flag silently doesn't work as expected.
Warnings are only useful when they have context about what the user is trying to do. Generic "this cohort is static" messages get ignored. "This static cohort will force a server request, removing the speed and cost benefits of local evaluation" gets acted on.
This is the first piece of an intent-driven correctness system for flags. The pattern:
How this maps to the wider product topology
Feature flags sit at the intersection of multiple evaluation paths (client SDK, server SDK with local eval, bootstrapped first-page-load). Each path has different constraints on what's evaluable. Today, users discover these constraints through debugging in production. Intent declaration moves that discovery to creation time.
The intent system is designed to be extensible: new intents (e.g., "edge evaluation", "mobile offline") slot in by adding entries to INTENT_METADATA and check functions to the warning logic. The
unreachableGroupsdetector is intent-agnostic and catches structural issues regardless. This will allow us to provide user-defined intents to further map PostHog to the user's product.How did you test this code?
CleanShot.2026-03-02.at.18.01.10-converted.mp4
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
No