feat(flags): support mixed targeting in local evaluation#188
Merged
patricio-posthog merged 3 commits intomainfrom Apr 29, 2026
Merged
feat(flags): support mixed targeting in local evaluation#188patricio-posthog merged 3 commits intomainfrom
patricio-posthog merged 3 commits intomainfrom
Conversation
Contributor
posthog-dotnet Compliance ReportDate: 2026-04-29 17:21:08 UTC
|
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 41ms |
Failures
request_payload.request_with_person_properties_device_id
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: tests/UnitTests/Features/LocalEvaluatorTests.cs
Line: 880-938
Comment:
**Non-parameterized tests where parameterization applies**
The first three facts (`PersonConditionMatchesWhenNoGroupsPassed`, `GroupConditionMatchesWhenGroupPropsMatch`, `NoMatchWhenBothPersonAndGroupFail`) all call `EvaluateFeatureFlag` on the same `"mixed-flag"` fixture with different person properties, groups, and expected outcomes. The project convention — and the simplicity rules — favour a single `[Theory]` + `[MemberData]` over multiple `[Fact]` methods to avoid repeating the arrange/act/assert skeleton and make it easy to add new cases later.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/PostHog/Features/LocalEvaluator.cs
Line: 284-298
Comment:
**Superfluous `HasValue` guard is unreachable**
Inside `if (conditionAggregation != flagAggregation)`, `conditionAggregation` is computed as `condition.AggregationGroupTypeIndex ?? flagAggregation`. The only way `conditionAggregation` can be `null` is when both `condition.AggregationGroupTypeIndex` and `flagAggregation` are `null` — but then `conditionAggregation == flagAggregation == null`, so the outer condition is false and we never enter this block. Therefore `conditionAggregation.HasValue` is always `true` here and the inner `if` is dead code (simplicity rule 4: no superfluous parts).
```csharp
// Mixed-override path: condition-level aggregation differs from flag-level.
if (conditionAggregation != flagAggregation)
{
if (!_groupTypeMapping.TryGetValue(conditionAggregation!.Value, out var groupType)
|| groups is null
|| !groups.TryGetGroup(groupType, out var group))
{
continue;
}
effectiveProperties = group.Properties;
effectiveBucketingId = group.GroupKey;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(flags): support mixed targeting in ..." | Re-trigger Greptile |
haacked
approved these changes
Apr 29, 2026
Contributor
haacked
left a comment
There was a problem hiding this comment.
Heh! Perhaps another C# developer convert! 😆
Two suggestions inline: the new rollout test does not actually exercise group-key bucketing (rollout 100 short-circuits before hashing), and one comment about the override path is narrower than the code. Greptile already covered the unreachable HasValue guard.
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
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.
💡 Motivation and Context
Feature flags with "mixed targeting" (the
feature-flag-mixed-targetingbeta) allow different condition sets within a single flag to target different aggregation levels — some targeting users/persons, others targeting groups.The SDK only reads
aggregation_group_type_indexat the flag level. For mixed flags this isnull, so the SDK treats all conditions as person-targeted. Group conditions fail property matching and the SDK falls back to a server-side HTTP call.This breaks customers using flags in environments that cannot make HTTP calls (e.g., background workers).
Ports posthog-python#523 to posthog-dotnet.
💚 How did you test it?
Updated
MatchFeatureFlagPropertiesinLocalEvaluatorto resolve aggregation per condition when a condition sets its ownAggregationGroupTypeIndexthat differs from the flag level. Each condition uses the correct properties and bucketing value for its aggregation type. Backwards compatible with existing pure person and pure group flags.Added a new
AggregationGroupTypeIndexproperty onFeatureFlagGroup(internal record — not part of the public API surface, so non-breaking).Added 5 new unit tests in
TheMixedTargetingEvaluation: person match, group match, no match, only-group-no-groups, and rollout bucketing. All 767 existing UnitTests continue to pass on net8.0; xUnit CI run reports 805 passed, 0 failed, 2 skipped.Note on the SDK compliance report
The auto-posted
posthog-dotnet Compliance Reportcomment showsrequest_payload.request_with_person_properties_device_idfailing with a 404 fromhttp://sdk-adapter:8080/get_feature_flag. This failure pre-exists onmainand is not related to this PR — it's reproducible on the three most recent merged PRs (#182, #183, #184). The cause is that the .NET compliance adapter (sdk_compliance_adapter/) doesn't implement the/get_feature_flagendpoint yet (mirrors the same gap that posthog-go had). Worth tracking as a follow-up to bring the adapter to parity with the harness contract, but out of scope here.📝 Checklist
If releasing new changes
releaselabel to the PRbump-patch,bump-minor, orbump-major