Open
Conversation
Add support for targeting rules in the audience payload. Rules are evaluated after audience filter but before audienceStrict/traffic split, allowing forced variant assignment based on context attributes and environment scoping. - Add getEnvironment() to Client - Add evaluateRules() to AudienceMatcher (parses rules[].or[] structure) - Integrate rules into Context._assign() with custom=true flag - Add comprehensive tests for matcher and context integration
joalves
requested review from
calthejuggler,
marcio-absmartly and
mario-silva
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds rule-based audience targeting with environment scoping. Client gains a public getEnvironment() method. AudienceMatcher adds evaluateRules(audienceString, environmentName, vars) to parse audience JSON, skip or evaluate rules based on rule.environments and rule.and, and return a numeric variant or null. Context captures the client environment and calls evaluateRules during experiment assignment; when a rule variant is returned the assignment is forced (assigned = true, eligible = true, variant set, overridden = true). Multiple new tests cover rule precedence, environment gating, audienceStrict interactions and exposure reporting. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/matcher.test.js (1)
214-218: Consider adding a test for rules with missingvariantproperty.The malformed rules tests cover invalid
rulesstructure but don't test the case where a rule object lacks thevariantproperty. This would help document the expected behaviour (and catch the issue flagged inmatcher.ts).📝 Proposed additional test
it("should return null when rule has no variant property", () => { const audience = JSON.stringify({ rules: [ { or: [ { name: "rule1", and: [{ value: true }], environments: [], // variant is missing }, ], }, ], }); expect(matcher.evaluateRules(audience, "production", {})).toBe(null); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/matcher.test.js` around lines 214 - 218, Add a test to src/__tests__/matcher.test.js that asserts matcher.evaluateRules returns null when a rule object is missing the required variant property: construct an audience JSON string with rules containing an object that has name/and/environments but no variant and call matcher.evaluateRules(audience, "production", {}) expecting null; this will document the expected behaviour and catch the case flagged in matcher.evaluateRules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/matcher.ts`:
- Around line 36-45: The matcher currently returns rule.variant directly which
can be undefined if the rule object lacks a variant, letting callers treat it as
non-null; update the logic in the matching method (the block referencing rule,
rule.variant, conditions, and this._jsonExpr.evaluateBooleanExpr) to explicitly
validate the presence of a variant before returning it: when conditions are
absent or when evaluateBooleanExpr returns true, check that the rule has a
defined variant (e.g., using Object.prototype.hasOwnProperty or typeof
rule.variant !== 'undefined') and return that value only if present, otherwise
return null so callers (like context.ts which tests ruleVariant !== null) won't
receive undefined.
---
Nitpick comments:
In `@src/__tests__/matcher.test.js`:
- Around line 214-218: Add a test to src/__tests__/matcher.test.js that asserts
matcher.evaluateRules returns null when a rule object is missing the required
variant property: construct an audience JSON string with rules containing an
object that has name/and/environments but no variant and call
matcher.evaluateRules(audience, "production", {}) expecting null; this will
document the expected behaviour and catch the case flagged in
matcher.evaluateRules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 517d7527-f95e-4572-89b4-24e573253ef8
📒 Files selected for processing (5)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/client.tssrc/context.tssrc/matcher.ts
- Guard against missing/non-number variant in evaluateRules returning undefined instead of null - Switch context tests from exp_test_ab (2 variants, normal=1) to exp_test_abc (3 variants, normal=2) so rule variant (1) differs from normal assignment, making tests actually meaningful - Add tests for missing variant and non-number variant properties
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/matcher.test.js (1)
255-277: Consider adding a test with multiple objects in therulesarray.All current tests use a single object in the
rulesarray with variation only in theorarray. If the implementation iterates through multiple top-level rule groups inrules[], a test verifying that behaviour would improve coverage.🧪 Example test case
it("should evaluate rules across multiple rule groups", () => { const audience = JSON.stringify({ rules: [ { or: [ { name: "rule1", and: [{ eq: [{ var: "country" }, { value: "GB" }] }], environments: [], variant: 1, }, ], }, { or: [ { name: "rule2", and: [{ eq: [{ var: "country" }, { value: "US" }] }], environments: [], variant: 2, }, ], }, ], }); expect(matcher.evaluateRules(audience, "production", { country: "US" })).toBe(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/matcher.test.js` around lines 255 - 277, Add a new unit test that verifies matcher.evaluateRules correctly iterates multiple top-level rule groups in the rules array: create an audience JSON with two objects in rules[] (first containing a GB rule with variant 1, second containing a US rule with variant 2), call matcher.evaluateRules(audience, "production", { country: "US" }) and assert it returns 2; name the test something like "should evaluate rules across multiple rule groups" so it complements the existing single-group tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/matcher.test.js`:
- Around line 255-277: Add a new unit test that verifies matcher.evaluateRules
correctly iterates multiple top-level rule groups in the rules array: create an
audience JSON with two objects in rules[] (first containing a GB rule with
variant 1, second containing a US rule with variant 2), call
matcher.evaluateRules(audience, "production", { country: "US" }) and assert it
returns 2; name the test something like "should evaluate rules across multiple
rule groups" so it complements the existing single-group tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 979d44b4-6279-4763-9d8a-833b504314e8
📒 Files selected for processing (3)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/matcher.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/context.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/matcher.ts
Rule-matched assignments should be excluded from statistical analysis, same as overrides. The custom flag is for developer-driven randomization which is still included in analysis.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/context.ts`:
- Around line 550-553: The code in _assign sets assignment.assigned,
assignment.variant and incorrectly sets assignment.overridden when ruleVariant
!== null; remove or avoid setting assignment.overridden in the rule-match branch
so rule-derived assignments remain distinct from manual overrides. Update the
logic in _assign (the branch that checks ruleVariant) to only mark
assigned/variant for rule matches and leave overridden false (or introduce a
separate ruleMatched flag if needed), and ensure the existing manual override
fast-path is the only place that sets assignment.overridden to true so
override("...") will always take the explicit-override semantics.
- Around line 541-548: The cached assignment reuse logic only checks
assignment.audienceMismatch but not the actual rule-driven variant, so updates
to attributes used by rules can return a stale variant; compute the current
ruleVariant via _audienceMatcher.evaluateRules(experiment.data.audience,
this._environmentName, this._getAttributesMap()) (as already done) and compare
it to the cached assignment's rule-derived identity (e.g., a stored ruleVariant
id or serialized ruleVariant) and treat a mismatch as invalidation (in addition
to assignment.audienceMismatch), or store the ruleVariant id onto the assignment
when first created so subsequent reuse logic compares that id against the newly
computed ruleVariant and rejects reuse if they differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44c23f01-bff6-485e-ba9a-bb01d4572414
📒 Files selected for processing (2)
src/__tests__/context.test.jssrc/context.ts
Rules bypass the traffic split, so eligible must be explicitly set to true (same as fullOn) rather than relying on the default value.
Add tests checking the complete flag combination for each scenario: - Rule match: assigned=true, eligible=true, overridden=true - No match (normal): assigned=true, eligible=true, overridden=false - Rule match with audienceMismatch: overridden=true, audienceMismatch=true - Override priority over rule: assigned=false, overridden=true
Re-evaluate rules in the audienceMatches cache check so that attribute changes affecting rule conditions properly invalidate the cached assignment. Store ruleVariant on the assignment for comparison.
When a rule caches an assignment with overridden=true and assigned=true, a subsequent override() with the same variant would incorrectly reuse the cached rule assignment (which has assigned=true). Add !assigned check to the override cache path so it always recomputes, producing the correct override flags (assigned=false, overridden=true).
- Log errors in evaluateRules catch block (matching evaluate() pattern) - Extract _getAttributesMap() to avoid double call in assignment path - Add test: multiple rule groups (second group matches when first doesn't) - Add test: variableValue returns correct config for rule-assigned variant - Add test: cache invalidation when rule switches between matching variants
Remove unnecessary optional chaining on sdk.getClient() to fail fast consistently with the rest of the codebase. Document how rule-matched assignments (assigned=true, overridden=true) are distinguished from SDK overrides (assigned=false, overridden=true) in analytics.
Change from return null to continue when a matched rule has a non-numeric variant, so subsequent valid rules are still evaluated. Also persist ruleVariant in audienceMatches cache validation to maintain cache consistency across attribute changes.
Author
Code reviewFound 2 issues (both now fixed in f32df8e):
Lines 33 to 41 in 3b9ab8c
Lines 476 to 481 in 3b9ab8c |
Contributor
calthejuggler
left a comment
calthejuggler
left a comment
There was a problem hiding this comment.
Looking really good so far 🙂
Good to put it in a separate PR, but let's not forget to add the SDK version to the payload before we publish this.
| fullOn: boolean; | ||
| custom: boolean; | ||
| audienceMismatch: boolean; | ||
| ruleVariant?: number | null; |
Contributor
There was a problem hiding this comment.
It's effectively just an override that's configured from the web console, right? 🤔 Why do we need this?
src/context.ts
Outdated
Comment on lines
+557
to
+559
| // Rule-matched: assigned=true + overridden=true | ||
| // SDK overrides: assigned=false + overridden=true | ||
| // This distinction lets analytics differentiate the two cases |
Contributor
There was a problem hiding this comment.
Does the engine definitely support this? 🤔 Feels like quite a lot of work, for something that could be simpler. I guess if we wanted analytics to be able to tell the difference, we could make it the same as an override, but the ruleVariant is set? Or we could change ruleVariant to be a ruleOverride: boolean - then we get the best of both worlds in the future, but we can keep the assigned/overridden logic the same.
Comment on lines
+20
to
+50
| evaluateRules(audienceString: string, environmentName: string | null, vars: Record<string, unknown>): number | null { | ||
| try { | ||
| const audience = JSON.parse(audienceString); | ||
| if (audience && Array.isArray(audience.rules)) { | ||
| for (const ruleGroup of audience.rules) { | ||
| if (!ruleGroup || !Array.isArray(ruleGroup.or)) continue; | ||
| for (const rule of ruleGroup.or) { | ||
| if (Array.isArray(rule.environments) && rule.environments.length > 0) { | ||
| if (environmentName == null || !rule.environments.includes(environmentName)) { | ||
| continue; | ||
| } | ||
| } | ||
| if (typeof rule.variant !== "number") continue; | ||
| const conditions = rule.and; | ||
| if (!conditions || (Array.isArray(conditions) && conditions.length === 0)) { | ||
| return rule.variant; | ||
| } | ||
| if (Array.isArray(conditions)) { | ||
| const result = this._jsonExpr.evaluateBooleanExpr({ and: conditions }, vars); | ||
| if (result === true) { | ||
| return rule.variant; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
We can reduce nesting here with some early returns 🙂 Something like:
Suggested change
| evaluateRules(audienceString: string, environmentName: string | null, vars: Record<string, unknown>): number | null { | |
| try { | |
| const audience = JSON.parse(audienceString); | |
| if (audience && Array.isArray(audience.rules)) { | |
| for (const ruleGroup of audience.rules) { | |
| if (!ruleGroup || !Array.isArray(ruleGroup.or)) continue; | |
| for (const rule of ruleGroup.or) { | |
| if (Array.isArray(rule.environments) && rule.environments.length > 0) { | |
| if (environmentName == null || !rule.environments.includes(environmentName)) { | |
| continue; | |
| } | |
| } | |
| if (typeof rule.variant !== "number") continue; | |
| const conditions = rule.and; | |
| if (!conditions || (Array.isArray(conditions) && conditions.length === 0)) { | |
| return rule.variant; | |
| } | |
| if (Array.isArray(conditions)) { | |
| const result = this._jsonExpr.evaluateBooleanExpr({ and: conditions }, vars); | |
| if (result === true) { | |
| return rule.variant; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| return null; | |
| } | |
| evaluateRules(audienceString: string, environmentName: string | null, vars: Record<string, unknown>): number | null { | |
| let audience | |
| try { | |
| audience = JSON.parse(audienceString); | |
| } catch (error) { | |
| console.error(error); | |
| return null; | |
| } | |
| if (!audience || !Array.isArray(audience.rules)) return null; | |
| for (const ruleGroup of audience.rules) { | |
| if (!ruleGroup || !Array.isArray(ruleGroup.or)) continue; | |
| for (const rule of ruleGroup.or) { | |
| if (Array.isArray(rule.environments) && rule.environments.length > 0) { | |
| if (environmentName == null || !rule.environments.includes(environmentName)) { | |
| continue; | |
| } | |
| } | |
| if (typeof rule.variant !== "number") continue; | |
| const conditions = rule.and; | |
| if (!conditions || (Array.isArray(conditions) && conditions.length === 0)) { | |
| return rule.variant; | |
| } | |
| if (!Array.isArray(conditions)) continue; | |
| const result = this._jsonExpr.evaluateBooleanExpr({ and: conditions }, vars); | |
| if (result === true) { | |
| return rule.variant; | |
| } | |
| } | |
| } | |
| return null; | |
| } |
Comment on lines
+2068
to
+2079
| rules: [ | ||
| { | ||
| or: [ | ||
| { | ||
| name: "Production Only", | ||
| and: [{ eq: [{ var: "country" }, { value: "US" }] }], | ||
| environments: ["production"], | ||
| variant: 1, | ||
| }, | ||
| ], | ||
| }, | ||
| ], |
Contributor
There was a problem hiding this comment.
We should add some tests for multiple ors and multiple ands (and maybe multiple environments?)
|
|
||
| describe("rules evaluation", () => { | ||
| // Uses exp_test_abc (3 variants, normal assignment = 2) with rules forcing variant 1 | ||
| // This ensures tests are meaningful: rule variant (1) differs from normal assignment (2) |
Contributor
There was a problem hiding this comment.
Suggested change
| // This ensures tests are meaningful: rule variant (1) differs from normal assignment (2) |
Comment on lines
+2352
to
+2365
| or: [ | ||
| { | ||
| name: "US Users", | ||
| and: [{ eq: [{ var: "country" }, { value: "US" }] }], | ||
| environments: [], | ||
| variant: 1, | ||
| }, | ||
| { | ||
| name: "GB Users", | ||
| and: [{ eq: [{ var: "country" }, { value: "GB" }] }], | ||
| environments: [], | ||
| variant: 2, | ||
| }, | ||
| ], |
Contributor
There was a problem hiding this comment.
Ah there's a double or here - maybe we can add some extra tests for these cases. Plus some multiple ands and multiple environments 🙂
Add a dedicated targetingRule boolean flag (bit 8, value 256) to exposure events so analytics can explicitly identify rule-forced assignments without relying on the assigned+overridden combination. - Rule match: targetingRule=true, overridden=true, assigned=true - SDK override: targetingRule=false, overridden=true, assigned=false - Normal assignment: targetingRule=false, overridden=false
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.
Summary
evaluateRules()toAudienceMatchersupporting the newrules[].or[]payload structure with environment scoping and JsonExpr conditionsContext._assign()— rules are checked after audience filter but beforeaudienceStrict/ traffic split, settingoverridden=trueon matchgetEnvironment()toClientso the Context can access the environment name for rule filteringrule.variantis a number to preventundefinedpropagationAssignment Flags Analysis
Rule-matched assignments set
assigned=true, eligible=true, overridden=true. This combination is uniquely identifiable and ensures rule-matched units are excluded from statistical analysis (same as overrides), while signaling they are real participants (unlike developer overrides which leaveassigned=false).All possible assignment states
assignedeligibleoverriddenfullOncustomaudienceMismatch*=audienceMismatchis set independently before rules are evaluated, so it can be either value.Unique identification by flags
assigned=false, overridden=false, audienceMismatch=falseassigned=false, overridden=trueassigned=false, audienceMismatch=trueassigned=true, overridden=trueassigned=true, eligible=true, custom=false, fullOn=false, overridden=falseassigned=true, eligible=falseassigned=true, custom=trueassigned=true, fullOn=trueEngine bitmask filter:
bitAnd(flags, 207) = 3This main statistical analysis filter requires
assigned=true, eligible=true, overridden=false, fullOn=false. Rule-matched exposures (overridden=true) are correctly excluded.Test Plan
evaluateRulesin matcher (no rules, empty rules, conditions match/mismatch, environment filtering, first-match-wins, empty conditions, malformed JSON, variant 0, missing variant, non-number variant)Summary by CodeRabbit
Release Notes
New Features
Tests