feat: posthog feature flag #1825
Conversation
aashishmalik
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020 and
1abhishekpandey
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1825 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/posthog/ui-config.json (1)
105-105: Consider enhancing the documentation note.While the link to PostHog docs is helpful, consider adding a brief description of what bootstrapping feature flags means and when to use it.
- "note": "https://posthog.com/docs/feature-flags/bootstrapping", + "note": "Configure feature flags that will be immediately available during initialization. See: https://posthog.com/docs/feature-flags/bootstrapping",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/configurations/destinations/posthog/db-config.json(2 hunks)src/configurations/destinations/posthog/ui-config.json(1 hunks)
🔇 Additional comments (5)
src/configurations/destinations/posthog/db-config.json (2)
21-22: LGTM! Addition of feature flag configuration key.
The addition of "flags" to the includeKeys array is consistent with the PR objective of introducing PostHog feature flag support.
80-81: Verify if feature flags should be web-only.
The "flags" configuration is only added to the web platform configuration. Please confirm if this is intentional or if it should also be available for other platforms (android, ios, etc.).
✅ Verification successful
Let me analyze the results and generate a final response.
Looking at the search results:
- The PostHog documentation search didn't return any results, but this could be due to API limitations.
- The codebase search shows that feature flags are used extensively across different destinations in both
ui-config.jsonanddb-config.jsonfiles. - Specifically for PostHog, in
src/configurations/destinations/posthog/ui-config.json, there's a reference to feature flags with a note linking to PostHog's documentation about bootstrapping feature flags: "https://posthog.com/docs/feature-flags/bootstrapping"
This indicates that feature flags are a core functionality in PostHog that's specifically designed for bootstrapping, which is a web-specific concept where flags need to be available immediately when the page loads.
Feature flags configuration should indeed be web-only.
The "flags" configuration is correctly added only to the web platform as it relates to PostHog's bootstrapping feature flags functionality, which is specifically designed for web applications to have immediate access to feature flags when the page loads. This is intentional and aligns with PostHog's architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if PostHog feature flags are supported on other platforms
# Check PostHog documentation for feature flag support across platforms
curl -s "https://api.github.com/search/code?q=feature+flags+repo:PostHog/posthog-js+repo:PostHog/posthog-android+repo:PostHog/posthog-ios" | jq '.items[].path'
# Look for any existing feature flag implementations in other platform configs
rg -i "feature.?flag" --type json
Length of output: 66929
src/configurations/destinations/posthog/ui-config.json (3)
102-111: LGTM! The feature flag configuration is well-structured.
The implementation follows the UI configuration patterns and provides clear documentation through the PostHog link.
102-111: Verify configuration alignment with db-config.json.
Ensure that the feature flag configuration aligns with the changes in db-config.json where "flags" was added to includeKeys and web arrays.
✅ Verification successful
Based on the examination of the PostHog destination configuration files, I can now provide a definitive response about the configuration alignment.
Configuration is properly aligned
The feature flag configuration in ui-config.json is correctly supported by the destination configuration:
- The "flags" field is properly included in the
includeKeysarray in the main PostHog configuration - The "flags" field is correctly listed in the
destConfig.webarray for web-specific configuration - The UI configuration for feature flags bootstrapping matches the schema requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the db-config.json changes are consistent
rg --json -g 'db-config.json' '"flags".*includeKeys|"flags".*web'
# Look for any other files that might need updates for feature flags
rg --json -g '*.json' '"flags".*config|feature.*flag.*config'
Length of output: 643
Script:
#!/bin/bash
# Let's try a broader search for db-config.json first to locate it
fd "db-config.json"
# Then let's examine its content
fd "db-config.json" --exec cat {}
# Also search for any related configuration files in the destinations/posthog directory
fd . "src/configurations/destinations/posthog" --type f --exec cat {}
Length of output: 83435
Script:
#!/bin/bash
# Let's specifically look for the PostHog destination configuration files
fd . "src/configurations/destinations/posthog" -t f -x cat {}
# Also search for any files containing "flags" configuration
rg -g '*.json' '"flags".*includeKeys|"flags".*web|"flags".*value'
Length of output: 37556
102-111: Verify feature flag value types support.
Please ensure that the dynamic form supports all possible PostHog feature flag value types (boolean, string, JSON, etc.).
aashishmalik
force-pushed
the
feat.posthog-flag
branch
2 times, most recently
from
491f7b3 to
41dc411
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/posthog/schema.json (1)
5-5: Document the purpose of additionalPropertiesConsider adding a comment in the schema to explain why
additionalPropertiesis enabled and its relationship to feature flags. This will help future maintainers understand the design decision.Apply this diff:
"$schema": "http://json-schema.org/draft-07/schema#", "type": "object", + "description": "Schema for PostHog destination configuration. additionalProperties is enabled to support dynamic feature flags.", "additionalProperties": true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/configurations/destinations/posthog/db-config.json(2 hunks)src/configurations/destinations/posthog/schema.json(1 hunks)src/configurations/destinations/posthog/ui-config.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/configurations/destinations/posthog/db-config.json
- src/configurations/destinations/posthog/ui-config.json
🔇 Additional comments (1)
src/configurations/destinations/posthog/schema.json (1)
5-5: LGTM with security considerations
The addition of "additionalProperties": true aligns with the PR objective of supporting PostHog feature flags. However, be aware that this makes the schema more permissive and could potentially allow invalid configurations to pass validation.
Let's verify the impact of this change:
✅ Verification successful
LGTM - Feature flag support is properly integrated
The addition of "additionalProperties": true is safe and necessary for PostHog feature flags support. The codebase shows:
- Feature flags are properly configured in
db-config.jsonwith "flags" in includeKeys - UI configuration in
ui-config.jsonincludes a dedicated "Bootstrapping feature flags" field - Other destinations already use feature flags for functionality control
- The schema remains secure for core properties while allowing feature flag extensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any validation constraints or sanitization for feature flags in other files
# Look for feature flag related validation or sanitization
rg -l "feature.*flag.*valid|sanitize.*feature|validate.*flag"
# Check for any security-related comments or documentation about feature flags
rg -l "//.*security.*flag|#.*security.*flag|/\*.*security.*flag|\"security\".*\"flag\""
Length of output: 167
Script:
#!/bin/bash
# Let's check for any feature flag related code and configuration
rg -g '!package-lock.json' -g '!*.lock' "featureFlag|feature_flag" -A 3
# Check for any validation or sanitization patterns in PostHog related files
fd posthog -t f --exec rg -l "validate|sanitize|clean"
# Look at the PostHog configuration files to understand the context better
fd "posthog.*\.(json|js|ts)" --exec cat {}
Length of output: 69411
Script:
#!/bin/bash
# Let's look at the PostHog schema and related configuration files
cat src/configurations/destinations/posthog/schema.json
cat src/configurations/destinations/posthog/ui-config.json
cat src/configurations/destinations/posthog/db-config.json
# Check for any feature flag validation patterns in the codebase
rg -g '!package-lock.json' -g '!*.lock' "validateFeatureFlag|feature.*flag.*schema" -A 3
Length of output: 37729
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/configurations/destinations/posthog/schema.json(1 hunks)
🔇 Additional comments (1)
src/configurations/destinations/posthog/schema.json (1)
6-6: LGTM: Allowing additional properties
Adding "additionalProperties": true is a good practice that:
- Aligns with the feature flag functionality addition
- Provides flexibility for future PostHog API extensions
- Follows common practice for third-party API integrations
| @@ -2,6 +2,8 @@ | |||
| "configSchema": { | |||
| "$schema": "http://json-schema.org/draft-07/schema#", | |||
| "type": "object", | |||
| "required": ["teamApiKey"], | |||
There was a problem hiding this comment.
Adding teamApiKey as a required field is a breaking change that contradicts the PR objectives. This change will cause validation failures for existing configurations that don't explicitly specify this field.
Consider one of these approaches:
- Remove the required field to maintain backward compatibility
- Document this as a breaking change and create a migration plan for existing configurations
- "required": ["teamApiKey"],
aashishmalik
force-pushed
the
feat.posthog-flag
branch
from
77651e5 to
1f1ca07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/data/validation/destinations/posthog.json (2)
29-30: Consider using a more realistic test API key formatWhile "1234" works as a test value, consider using a format that matches PostHog's actual API key pattern (e.g., "phc_xxxxxxxxxxxxxxxxxxxx") to make the tests more realistic.
Line range hint
306-419: Consider documenting the validation patternThe test case thoroughly validates various constraints, but the pattern
(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$is complex. Consider adding a comment explaining that it validates:
- Template expressions:
{{...||...}}- Environment variables:
env.VARIABLE- Regular strings: max 100 characters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/configurations/destinations/posthog/db-config.json(2 hunks)src/configurations/destinations/posthog/schema.json(1 hunks)src/configurations/destinations/posthog/ui-config.json(1 hunks)test/data/validation/destinations/posthog.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/configurations/destinations/posthog/ui-config.json
- src/configurations/destinations/posthog/db-config.json
- src/configurations/destinations/posthog/schema.json
🔇 Additional comments (4)
test/data/validation/destinations/posthog.json (4)
37-37: LGTM! Consistent placement of teamApiKey
The teamApiKey is consistently placed at the root level of the config object across all test cases, which is good for maintainability.
Also applies to: 56-56, 74-74, 88-88, 103-103, 257-257, 306-306
Line range hint 103-256: LGTM! Comprehensive consent management test coverage
Excellent test coverage including:
- Multiple consent providers
- Platform-specific configurations
- Environment variable syntax
- Template expressions
- Empty values
Line range hint 257-305: LGTM! Clear validation of data structure
The test case properly validates that oneTrustCookieCategories and ketchConsentPurposes must be objects, not arrays, with clear error messages.
Line range hint 1-419: Verify alignment with PR objectives
The PR description mentions PostHog feature flag functionality, but the test changes focus on consent management and API key validation. Could you clarify how these changes relate to the feature flag implementation?
✅ Verification successful
Let me search for PostHog feature flag related code in the repository to understand the context better.
Let me check the PostHog UI configuration to understand the feature flag functionality better.
The test changes are correctly aligned with PostHog feature flag functionality
The test file changes in test/data/validation/destinations/posthog.json are validating the configuration schema for PostHog destination, which includes feature flag bootstrapping functionality. This is evident from:
- The PostHog UI configuration shows feature flags are a supported feature through the "Bootstrapping feature flags" field
- The
flagsproperty is included in theincludeKeysarray in PostHog's db-config.json - The test cases validate various configuration scenarios including consent management, which is a prerequisite for proper feature flag functionality
The changes are not disconnected from feature flags but rather are essential validation tests ensuring the PostHog destination configuration works correctly, including its feature flag capabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for feature flag related code
rg -i "feature.?flag|flag" --type json
Length of output: 67410
Script:
#!/bin/bash
# Search for PostHog feature flag related files and code
rg -l "posthog.*flag" --type json
cat src/configurations/destinations/posthog/db-config.json
Length of output: 3671
Script:
#!/bin/bash
# Check PostHog UI config for feature flag related settings
cat src/configurations/destinations/posthog/ui-config.json
Length of output: 9955
WalkthroughThe changes in this pull request involve modifications to the PostHog destination configuration files, specifically adding new keys and properties to enhance the configuration options and validation rules. Notably, Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/configurations/destinations/posthog/ui-config.json (1)
102-111: LGTM: Feature flag UI configuration added correctlyThe dynamicForm implementation for feature flags follows the established patterns and includes appropriate documentation link.
Consider adding a brief note about the expected value format (boolean, string, number, etc.) in the UI, similar to:
{ "type": "dynamicForm", "label": "Bootstrapping feature flags", "note": "https://posthog.com/docs/feature-flags/bootstrapping", + "note": "https://posthog.com/docs/feature-flags/bootstrapping\nValues can be boolean, string, or number.", "labelLeft": "Flag", "labelRight": "Value", "keyLeft": "flag", "keyRight": "value", "value": "flags" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/configurations/destinations/posthog/db-config.json(2 hunks)src/configurations/destinations/posthog/schema.json(1 hunks)src/configurations/destinations/posthog/ui-config.json(1 hunks)test/data/validation/destinations/posthog.json(7 hunks)
🔇 Additional comments (6)
src/configurations/destinations/posthog/db-config.json (2)
21-22: LGTM: Feature flag configuration key added correctly
The addition of the "flags" key to the includeKeys array is appropriate for enabling PostHog's feature flag bootstrapping functionality.
80-81: LGTM: Feature flag support added for web platform
The addition of "flags" to the web configuration is appropriate as feature flags are typically needed in web applications.
test/data/validation/destinations/posthog.json (2)
29-30: LGTM: Team API key added to test configurations
The addition of teamApiKey to all test configurations is consistent with the schema requirements.
Also applies to: 37-37, 56-56, 74-74, 88-88, 103-103, 257-257, 306-306
Line range hint 1-400: Add test cases for feature flags configuration
The test file lacks specific test cases for validating the new feature flags functionality. Consider adding test cases to verify:
- Valid feature flag configurations
- Invalid feature flag formats
- Empty feature flags
- Maximum length/size constraints
src/configurations/destinations/posthog/schema.json (2)
5-5:
Making teamApiKey a required field contradicts the PR objectives which state there are no breaking changes.
6-6:
Setting additionalProperties to false is a breaking change that will reject configurations containing any properties not explicitly defined in the schema. This could impact existing configurations that might have custom fields.
Consider:
- Removing this restriction to maintain backward compatibility
- Document this as a breaking change and create a migration plan
- If restriction is necessary, consider allowing additional properties with a deprecation notice
Let's verify if any existing configurations use additional properties:
✅ Verification successful
Let me gather more information about the schema and configurations to make a more informed decision.
Let me check the db-config.json file to see if there are any existing configurations that might be affected by this change.
Based on the analysis of the schema.json, ui-config.json, and db-config.json files, I can now provide a final response:
Setting additionalProperties: false is safe in this context
The schema is well-defined and complete, covering all configuration properties that are explicitly listed in db-config.json's includeKeys. The UI configuration (ui-config.json) also aligns perfectly with the schema's property definitions. There's no evidence of any properties being used that aren't defined in the schema, and the configuration system appears to be strictly controlled through the includeKeys list.
Key observations:
- All configurable properties are explicitly defined in both the schema and db-config
- The UI configuration only exposes fields that match the schema's properties
- The
destConfigsection in db-config.json shows a clear mapping of allowed properties per platform
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for PostHog configurations to check for custom properties
# that would be rejected by additionalProperties: false
# Look for PostHog configuration files
echo "Searching for PostHog configurations..."
rg -l --type json "posthog" | while read -r file; do
# Extract and analyze the configuration structure
echo "Analyzing $file for additional properties..."
jq -r 'keys[]' "$file" 2>/dev/null || true
done
Length of output: 424
Script:
#!/bin/bash
# Let's examine the schema file and UI config in detail
echo "=== Schema File Contents ==="
cat src/configurations/destinations/posthog/schema.json
echo -e "\n=== UI Config Contents ==="
cat src/configurations/destinations/posthog/ui-config.json
# Let's also check if there are any test configurations or examples
echo -e "\n=== Looking for test/example configurations ==="
fd -e json -e jsonc . src/configurations/destinations/posthog/
Length of output: 34564
Script:
#!/bin/bash
# Let's examine the db-config file for any existing configurations
echo "=== DB Config Contents ==="
cat src/configurations/destinations/posthog/db-config.json
# Let's also check if there are any test files that might contain example configurations
echo -e "\n=== Looking for test files with PostHog configurations ==="
fd -e test.js -e test.ts -e spec.js -e spec.ts . | grep -i posthog
Length of output: 3845
aashishmalik
force-pushed
the
feat.posthog-flag
branch
from
c145a06 to
9033c35
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/data/validation/destinations/posthog.json (1)
29-30: Consider adding more test casesWhile the test coverage for basic teamApiKey validation is good, consider adding test cases for:
- Invalid teamApiKey patterns
- Empty teamApiKey values
- Feature flag specific scenarios
Would you like me to help generate additional test cases?
Also applies to: 37-37, 56-56, 74-74, 103-103, 257-257, 306-306
src/configurations/destinations/posthog/schema.json (1)
800-948: Consider grouping related web configurationsThe schema has multiple web-specific boolean configurations. Consider grouping them under a single
webConfigobject for better organization:- "useNativeSDK": { - "type": "object", - "properties": { - "web": { - "type": "boolean" - } - } - }, + "webConfig": { + "type": "object", + "properties": { + "useNativeSDK": { + "type": "boolean" + }, + "enableLocalStoragePersistence": { + "type": "boolean" + }, + // ... other web-specific configs + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/configurations/destinations/posthog/db-config.json(2 hunks)src/configurations/destinations/posthog/schema.json(2 hunks)src/configurations/destinations/posthog/ui-config.json(1 hunks)test/data/validation/destinations/posthog.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/configurations/destinations/posthog/ui-config.json
🔇 Additional comments (5)
src/configurations/destinations/posthog/db-config.json (2)
21-23: LGTM: New configuration keys added correctly
The addition of flags and disableCookie to the includeKeys array is implemented correctly and aligns with the feature flag support objective.
81-83: LGTM: Web configuration updated consistently
The web configuration section is updated consistently with the same keys added to includeKeys.
src/configurations/destinations/posthog/schema.json (3)
879-899: LGTM: Feature flag schema implementation
The schema for feature flags is well-structured with:
- Proper type validation
- Pattern restrictions for flag names and values
- Length limits for security
5-5:
Making teamApiKey a required field contradicts the PR objectives of "no breaking changes". This will cause validation failures for existing configurations.
- "required": ["teamApiKey"],796-799: Verify the ngrok.io domain restriction
The pattern for yourInstance explicitly blocks .ngrok.io domains. Please confirm if this restriction is intentional and document the reasoning.
✅ Verification successful
Let me check the specific usage of ngrok.io in these files to understand the context better.
The ngrok.io domain restriction is intentionally enforced across multiple destination schemas
The restriction on ngrok.io domains appears to be a consistent security pattern across the codebase. It's implemented in both PostHog and Postgres destination schemas to prevent the use of temporary ngrok tunnels in production configurations. The test files contain ngrok.io URLs only in test data, which validates that these restrictions are working as intended.
- Found in production schemas:
src/configurations/destinations/posthog/schema.json: Blocks ngrok.io in yourInstancesrc/configurations/destinations/postgres/schema.json: Blocks ngrok.io in host
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing ngrok.io references in the codebase
rg -l "ngrok\.io"
Length of output: 206
Script:
#!/bin/bash
# Check the context of ngrok.io usage in the found files
rg "ngrok\.io" -C 3
Length of output: 2537
|
@aashishmalik mention ticket number in the PR description |
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
What are the changes introduced in this PR?
What is the related Linear task?
Resolves INT-XXX
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
teamApiKeymandatory and adding new configuration options for improved flexibility.