Allow passing groups to feature flags #2010
Conversation
Signed-off-by: Alexis Rico <[email protected]>
|
@SferaDev is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implements group-based feature flag evaluation in PostHog JS SDK, allowing feature flags to be evaluated in the context of specific groups (e.g., organizations) rather than just user context.
- Added optional
groupsparameter touseFeatureFlagEnabled,useFeatureFlagPayload, anduseFeatureFlagVariantKeyhooks for per-evaluation group context - Critical bug:
groupsparameter not being passed togetFeatureFlagPayload()calls despite being in dependency array - Modified
PostHogFeaturecomponent to support group-based flag evaluation through newgroupsprop - Enhanced capture events to include group context when evaluating feature flags
- Added comprehensive test coverage for group-based feature flag functionality
11 files reviewed, 9 comments
Edit PR Review Bot Settings | Greptile
| }) | ||
| }, [client, flag]) | ||
| }, [client, flag, options]) |
There was a problem hiding this comment.
logic: including 'options' in deps array may cause infinite re-renders if object is recreated on each render
| return client.onFeatureFlags(() => { | ||
| setFeatureFlagPayload(client.getFeatureFlagPayload(flag)) | ||
| }) |
There was a problem hiding this comment.
logic: The groups option is not being passed to getFeatureFlagPayload() here, which means group-based evaluation won't work despite the parameter being added to the function signature
| return client.onFeatureFlags(() => { | |
| setFeatureFlagPayload(client.getFeatureFlagPayload(flag)) | |
| }) | |
| return client.onFeatureFlags(() => { | |
| setFeatureFlagPayload(client.getFeatureFlagPayload(flag, options?.groups)) | |
| }) |
| const [featureFlagVariantKey, setFeatureFlagVariantKey] = useState<string | boolean | undefined>(() => | ||
| client.getFeatureFlag(flag) | ||
| client.getFeatureFlag(flag, { send_event: false, ...options }) | ||
| ) |
There was a problem hiding this comment.
style: Consider memoizing options object with useMemo to prevent unnecessary re-renders when passing inline objects
| export function useFeatureFlagPayload( | ||
| flag: string, | ||
| options?: { groups?: Record<string, string> } | ||
| ): JsonType { | ||
| const client = usePostHog() | ||
|
|
||
| const [featureFlagPayload, setFeatureFlagPayload] = useState<JsonType>(() => client.getFeatureFlagPayload(flag)) |
There was a problem hiding this comment.
logic: Initial call to getFeatureFlagPayload() is missing the groups option, causing inconsistency with subsequent updates
| const [featureFlagPayload, setFeatureFlagPayload] = useState<JsonType>(() => client.getFeatureFlagPayload(flag)) | |
| const [featureFlagPayload, setFeatureFlagPayload] = useState<JsonType>(() => client.getFeatureFlagPayload(flag, options?.groups)) |
| }) | ||
| }, [client, flag]) | ||
| }, [client, flag, options]) |
There was a problem hiding this comment.
logic: Adding 'options' to deps array could cause infinite loops if options object is created inline - needs memoization
| const eventPayload = | ||
| jest.mocked(posthog._requestQueue!.enqueue).mock.calls[0]?.[0] || | ||
| jest.mocked(posthog._send_retriable_request as any).mock.calls[0][0] |
There was a problem hiding this comment.
style: Consider refactoring this payload extraction logic into a test helper function since it's used multiple times
| }): JSX.Element { | ||
| const clickTrackedRef = useRef(false) | ||
| const visibilityTrackedRef = useRef(false) | ||
| const posthog = usePostHog() | ||
| const variant = useFeatureFlagVariantKey(flag) | ||
| const variant = useFeatureFlagVariantKey(flag, { groups }) |
There was a problem hiding this comment.
style: Consider moving variant call up to parent component to avoid duplicate feature flag evaluations (already called in lines 28-29)
| // Return different values when groups are passed to test the functionality | ||
| if (flag === 'group_specific_flag' && options.groups.team === '123') { | ||
| return 'group_variant' | ||
| } |
There was a problem hiding this comment.
style: Magic string 'group_variant' should be defined as a constant at the top of the file
| * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_called event to PostHog. If {groups: {group_type: group_key}}, we will use the group key to evaluate the flag. | ||
| */ |
There was a problem hiding this comment.
style: JSDoc param type doesn't match updated TypeScript type (Object|String vs { send_event?: boolean; groups?: Record<string, string> })
| * @param {Object|String} options (optional) If {send_event: false}, we won't send an $feature_flag_called event to PostHog. If {groups: {group_type: group_key}}, we will use the group key to evaluate the flag. | |
| */ | |
| * @param {{ send_event?: boolean; groups?: Record<string, string> }} options (optional) If {send_event: false}, we won't send an $feature_flag_called event to PostHog. If {groups: {group_type: group_key}}, we will use the group key to evaluate the flag. | |
| */ |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Changes
Closes #2012
Checklist