fix(flags): show flag key instead of ID in flag dependencies#66218
Draft
phillram wants to merge 5 commits into
Draft
fix(flags): show flag key instead of ID in flag dependencies#66218phillram wants to merge 5 commits into
phillram wants to merge 5 commits into
Conversation
Flag-dependency release conditions store the numeric flag ID in `property.key` and only carry the human-readable key in `property.label`, which is populated at selection time but absent after a reload. Several display surfaces fell back to rendering the raw ID. Add a shared `withResolvedFlagLabels` helper that backfills `label` from the existing `getFlagKey` cache, and route the readonly view, the collapsible summary/editor, and the editable property filters through it so the flag key shows everywhere, with the ID only as a transient fallback until resolution completes. Generated-By: PostHog Code Task-Id: 994fe32e-f8c2-489e-baf9-034d23e6c554
Contributor
|
Reviews (1): Last reviewed commit: "fix(feature-flags): show flag key instea..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -128,7 +130,7 @@ function summarizeProperties( | |||
| if (property.type === PropertyFilterType.Cohort) { | |||
Contributor
There was a problem hiding this comment.
summarizeProperties still shows raw ID during loading window
getFlagKey falls back to the raw flag ID when the cache hasn't resolved yet (flagKeyCache[flagId] || flagId), so getFlagKey(property.key) here will return the numeric string while loading. This means the condition-group header summary can still display the bare ID transiently. By contrast, withResolvedFlagLabels guards against this with resolved !== property.key, so the pill views don't have this issue. The PR description acknowledges transient ID display, but if you want the same "wait until resolved, then show name" behaviour in the summary, this line could apply the same guard and only use the resolved value when it differs from property.key.
Contributor
|
Size Change: +375 kB (+0.59%) Total Size: 64.4 MB 📦 View Changed
ℹ️ View Unchanged
|
Address Greptile review: - withResolvedFlagLabels no longer short-circuits on a present label, so an injected label that round-trips through PropertyFilters onChange into state can still be re-resolved (e.g. after a flag rename). It now re-checks the getFlagKey cache every call and only overrides when the resolved key differs from both the raw ID and the current label. - summarizeProperties applies the same resolved-differs-from-ID guard so the condition summary no longer flashes the numeric ID while the cache is loading. Generated-By: PostHog Code Task-Id: 994fe32e-f8c2-489e-baf9-034d23e6c554
Add focused unit tests for the withResolvedFlagLabels helper, mirroring the precedent set by the distinct-ID name resolution work. Each case guards a concrete regression: backfilling the resolved key while keeping the ID in `key`, not injecting the raw ID as a label before the cache resolves, and re-resolving over a stale label rather than trusting one that round-tripped through onChange (the two behaviors raised in review). Generated-By: PostHog Code Task-Id: 994fe32e-f8c2-489e-baf9-034d23e6c554
Frontend typecheck failed because `label` is not accessible on the wide `AnyPropertyFilter` union without narrowing to the flag member. Type the test's flag filter factory and resolved results as `FlagPropertyFilter` so `.label` is accessible, matching how production code narrows before reading it. Generated-By: PostHog Code Task-Id: 994fe32e-f8c2-489e-baf9-034d23e6c554
phillram
changed the title
Generated-By: PostHog Code Task-Id: 994fe32e-f8c2-489e-baf9-034d23e6c554
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.
Problem
In a feature flag's release conditions, you can target on another feature flag ("flag dependency"). After selecting a flag, and especially after a reload, the UI showed the flag's numeric ID instead of its human-readable key. Reported via a Slack thread.
Root cause: a
FlagPropertyFilterintentionally stores the numeric flag ID inkey(that's what the backend evaluates against). The human-readable key only lives in the optionallabelfield, which is set at selection time but isn't persisted — so on reload it's gone and the display surfaces fell back toproperty.key(the ID). One surface already resolved the ID via an asyncgetFlagKeycache, but the readonly view, the collapsible summary/editor, and the editable property-filter pill did not.Changes
Follows the same pattern as a similar issue here: #63057
withResolvedFlagLabels(properties, getFlagKey)helper infeatureFlagReleaseConditionsLogicthat backfillslabelfrom the already-loadedgetFlagKeycache (leaving filters untouched until the key resolves).FeatureFlagReleaseConditionsReadonly(the flag overview display)FeatureFlagReleaseConditionsCollapsible(condition summary + the editable property filters)FeatureFlagReleaseConditions(editable property filters)No backend or data-model change — the filter
keystill holds the flag ID; only the display label is resolved.How did you test this code?
I'm an agent (PostHog Slack app). I have not run the app or automated tests for this change —
node_moduleswas not available in this environment, so I could not run the frontend typecheck. The change is type-checked manually against the existing types (label?: stringonBasePropertyFilter,getFlagKeyalready exposed by the logic). Please runpnpm --filter=@posthog/frontend typescript:checkin CI / locally to confirm.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored by the PostHog Slack app from a Slack thread. The reporter described flag-dependency conditions rendering the flag ID instead of its name. I traced the four render surfaces, confirmed the ID-vs-
labelroot cause, and chose to centralize resolution in a small reusable helper rather than duplicategetFlagKeylookups at each site —getFlagKey/flagKeyCachealready existed for the readonly-detail view, so this reuses that machinery and threads it (mirroring the existinggetDistinctIdNameprop) through the collapsible's condition components.Created with PostHog from a Slack thread