feat: add custom assignment rules evaluation#52
Merged
Conversation
joalves
requested review from
calthejuggler,
marcio-absmartly and
mario-silva
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughAdds rule-based assignment evaluation and environment-aware rule gating. AudienceMatcher gains evaluateRules(assignmentRulesString: string, environmentId: number | null, vars: Record<string, unknown>) returning a numeric variant or null. Context accepts ContextData.environment_id, stores it on _environmentId, and supports ExperimentData.assignmentRules plus per-assignment fields ruleVariant, ruleKey and ruleOverride. Assignments are created and re-evaluated using rules (including cache invalidation when attributes or ruleKey change), ruleOverride is included in exposure events and queued publishes, variable gating considers ruleOverride, and client.getEnvironment() was relocated within the class. Tests expanded to cover rule evaluation, environment scoping and exposure flags. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.1)src/__tests__/context.test.jsComment |
![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
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
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
Contributor
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.
calthejuggler
force-pushed
the
feature/sdk-rules-implementation
branch
from
c6d8137 to
75540b6
Compare
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
- 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
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.
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.
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
…dback - Rename targetingRule flag to ruleOverride per Cal's suggestion - Reduce nesting in evaluateRules with early returns - Remove unnecessary comment in test fixtures - Add tests for multiple and conditions, multiple environments, and multiple or rules in context integration tests
Rule-forced users are not real experiment participants. Set assigned=false and overridden=false, relying on ruleOverride=true to identify them. The assigned=false already excludes them from the engine filter (bitAnd(flags, 207) = 3 requires assigned=true). Update variableValue/peekVariable to check ruleOverride alongside assigned and overridden for returning variable configs. Remove the !assignment.assigned override cache workaround since rules no longer set overridden=true.
…ent name for rule matching
calthejuggler
force-pushed
the
feature/sdk-rules-implementation
branch
from
0ec80ce to
ecc0e1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/context.ts (1)
24-35:⚠️ Potential issue | 🟠 MajorMake
assignment_rulesoptional on the exported payload type.The runtime already treats this field as optional (
if (experiment.data.assignment_rules && ...)), and multiple payloads in this PR omit it entirely. BecauseExperimentDatais exported, making a newly added wire field mandatory is a breaking type change for consumer fixtures and stubs.💡 Suggested fix
export type ExperimentData = { id: number; name: string; unitType: string | null; iteration: number; fullOnVariant: number; trafficSplit: number[]; trafficSeedHi: number; trafficSeedLo: number; audience: string; - assignment_rules: string; + assignment_rules?: string; audienceStrict: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 24 - 35, The exported ExperimentData type currently declares assignment_rules as required but runtime and fixtures treat it as optional; update the ExperimentData definition to make assignment_rules optional (e.g., change to assignment_rules?: string or string | undefined) so consumers/stubs aren’t broken, and scan usages of ExperimentData (references to ExperimentData and any code accessing experiment.data.assignment_rules) to ensure they handle the possibly undefined value.
🤖 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 563-571: The value returned from evaluateRules() (assigned to
ruleVariant) must be canonicalised to null unless it is an integer within [0,
experiment.data.variants.length) before being used as an index or stored; update
the branch that sets
assignment.ruleVariant/assignment.variant/assignment.ruleOverride to first
validate ruleVariant (e.g., check Number.isInteger(ruleVariant) and 0 <=
ruleVariant < experiment.data.variants.length) and set it to null otherwise, and
apply the same canonicalisation when comparing against cached values (where
audienceMatches or cache comparison uses ruleVariant) so only valid integer
indices are treated as matches and invalid values become null.
---
Outside diff comments:
In `@src/context.ts`:
- Around line 24-35: The exported ExperimentData type currently declares
assignment_rules as required but runtime and fixtures treat it as optional;
update the ExperimentData definition to make assignment_rules optional (e.g.,
change to assignment_rules?: string or string | undefined) so consumers/stubs
aren’t broken, and scan usages of ExperimentData (references to ExperimentData
and any code accessing experiment.data.assignment_rules) to ensure they handle
the possibly undefined value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ae62f71-e790-4aec-9196-4d4dc0f3af44
📒 Files selected for processing (5)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/client.tssrc/context.tssrc/matcher.ts
✅ Files skipped from review due to trivial changes (1)
- src/client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/matcher.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/context.ts (1)
24-35:⚠️ Potential issue | 🟠 MajorDo not make
assignmentRulesa required public input.
ExperimentDatais exported and used by customContextDataProviderimplementations. Requiring this field makes the upgrade source-incompatible for TypeScript users even though the runtime already treats a missing value as “no rules”. Please keep it optional, or default it during initialisation.♻️ Proposed fix
export type ExperimentData = { id: number; name: string; unitType: string | null; iteration: number; fullOnVariant: number; trafficSplit: number[]; trafficSeedHi: number; trafficSeedLo: number; audience: string; - assignmentRules: string; + assignmentRules?: string | null; audienceStrict: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 24 - 35, The exported ExperimentData type currently requires assignmentRules which breaks TypeScript compatibility for custom ContextDataProvider implementations; change the ExperimentData declaration so assignmentRules is optional (e.g., assignmentRules?: string) or ensure the initialization code for ExperimentData (where ContextDataProvider results are consumed) assigns a default (empty string or null) when assignmentRules is missing; update any references that assume it exists to handle undefined (e.g., checks in assignment/validation logic) so runtime behavior remains “no rules” when the field is absent.
♻️ Duplicate comments (1)
src/context.ts (1)
472-493:⚠️ Potential issue | 🟠 MajorRefresh-time rule changes can still reuse a stale assignment.
This cache check only re-evaluates when attributes change, and the rule comparison is skipped entirely once
experiment.assignmentRulesbecomes empty. Arefresh()that adds/removes rules or changesenvironment_idcan therefore keep serving an oldruleVariant/ruleOverrideindefinitely. Please treat rule config and environment as cache keys too, and explicitly invalidate when a cached rule result exists but the refreshed experiment no longer has rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 472 - 493, The current cache refresh only checks attribute sequence (_attrsSeq vs assignment.attrsSeq) and skips rule comparison when experiment.assignmentRules is empty, allowing stale assignment.ruleVariant/ruleOverride to persist; update the logic in the block that references _attrsSeq, experiment.assignmentRules, _audienceMatcher.evaluateRules, assignment.ruleVariant, assignment.ruleOverride and _environmentId so that rule configuration and environment are treated as cache keys: compute/compare a ruleKey (derived from experiment.assignmentRules content and _environmentId) against a stored assignment.ruleKey and if they differ or if the refreshed experiment has no rules while assignment.ruleVariant or assignment.ruleOverride exists, invalidate (return false) and clear assignment.ruleVariant/ruleOverride/ruleKey before proceeding; ensure assignment.ruleVariant is still set only when evaluateRules is called and matches the new ruleKey.
🤖 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/__tests__/matcher.test.js`:
- Around line 385-399: The test title claims it covers fractional environment
IDs but it doesn't pass a fractional ID to AudienceMatcher.evaluateRules; update
the test in matcher.test.js to either (a) assert the fractional case by adding
expect(matcher.evaluateRules(audience, 1.5, {})).toBe(null) so it actually
exercises the fractional-ID path, or (b) change the spec string to reflect only
integer checks (rename the test). Refer to matcher.evaluateRules /
AudienceMatcher.evaluateRules and the environments array in the rule when making
this change.
---
Outside diff comments:
In `@src/context.ts`:
- Around line 24-35: The exported ExperimentData type currently requires
assignmentRules which breaks TypeScript compatibility for custom
ContextDataProvider implementations; change the ExperimentData declaration so
assignmentRules is optional (e.g., assignmentRules?: string) or ensure the
initialization code for ExperimentData (where ContextDataProvider results are
consumed) assigns a default (empty string or null) when assignmentRules is
missing; update any references that assume it exists to handle undefined (e.g.,
checks in assignment/validation logic) so runtime behavior remains “no rules”
when the field is absent.
---
Duplicate comments:
In `@src/context.ts`:
- Around line 472-493: The current cache refresh only checks attribute sequence
(_attrsSeq vs assignment.attrsSeq) and skips rule comparison when
experiment.assignmentRules is empty, allowing stale
assignment.ruleVariant/ruleOverride to persist; update the logic in the block
that references _attrsSeq, experiment.assignmentRules,
_audienceMatcher.evaluateRules, assignment.ruleVariant, assignment.ruleOverride
and _environmentId so that rule configuration and environment are treated as
cache keys: compute/compare a ruleKey (derived from experiment.assignmentRules
content and _environmentId) against a stored assignment.ruleKey and if they
differ or if the refreshed experiment has no rules while assignment.ruleVariant
or assignment.ruleOverride exists, invalidate (return false) and clear
assignment.ruleVariant/ruleOverride/ruleKey before proceeding; ensure
assignment.ruleVariant is still set only when evaluateRules is called and
matches the new ruleKey.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca12f6af-ef09-4cbf-9c47-25db3e38ccb3
📒 Files selected for processing (5)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/client.tssrc/context.tssrc/matcher.ts
✅ Files skipped from review due to trivial changes (1)
- src/client.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/matcher.ts
- src/tests/context.test.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/context.ts (1)
499-506:⚠️ Potential issue | 🟠 MajorReintroduced: normalise out-of-range
ruleVariantvalues tonull.
src/matcher.ts:20-68only narrows rule results to “integer ornull”; it does not bound them tovariants.length. This path caches the raw integer inassignment.ruleVariant, but only treats it as a real rule match when it is in range. Two different invalid rule results therefore become two different cached states even though the effective assignment is the same fallback, which can invalidate the cache and duplicate exposures after attribute changes.Suggested fix
+ const normaliseRuleVariant = (value: number | null, variantCount: number) => + value !== null && value >= 0 && value < variantCount ? value : null; + const audienceMatches = (experiment: ExperimentData, assignment: Assignment) => { const ruleKey = experiment.assignmentRules ? `${experiment.assignmentRules}:${this._environmentId}` : ""; @@ if (experiment.assignmentRules && experiment.assignmentRules.length > 0) { - const ruleVariant = this._audienceMatcher.evaluateRules(experiment.assignmentRules, this._environmentId, attrs); + const ruleVariant = normaliseRuleVariant( + this._audienceMatcher.evaluateRules(experiment.assignmentRules, this._environmentId, attrs), + experiment.variants.length + ); if (ruleVariant !== (assignment.ruleVariant ?? null)) { return false; } assignment.ruleVariant = ruleVariant; @@ if (experiment.data.assignmentRules && experiment.data.assignmentRules.length > 0) { - ruleVariant = this._audienceMatcher.evaluateRules(experiment.data.assignmentRules, this._environmentId, attrs); + ruleVariant = normaliseRuleVariant( + this._audienceMatcher.evaluateRules(experiment.data.assignmentRules, this._environmentId, attrs), + experiment.data.variants.length + ); } assignment.ruleVariant = ruleVariant; @@ - if (ruleVariant !== null && ruleVariant >= 0 && ruleVariant < experiment.data.variants.length) { + if (ruleVariant !== null) { assignment.variant = ruleVariant; assignment.ruleOverride = true; } else if (experiment.data.audienceStrict && assignment.audienceMismatch) {Also applies to: 583-590
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 499 - 506, The code stores raw integer rule results into assignment.ruleVariant without bounding them to the experiment's variants, causing distinct out-of-range integers to be cached differently; update the logic in the block that calls this._audienceMatcher.evaluateRules (and similarly in the other occurrence) to normalise any ruleVariant that is not null and is outside the valid range (i.e., < 0 or >= experiment.variants.length) to null before comparing to assignment.ruleVariant and before assigning assignment.ruleVariant, so the cache only records either a valid variant index or null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/context.ts`:
- Around line 499-506: The code stores raw integer rule results into
assignment.ruleVariant without bounding them to the experiment's variants,
causing distinct out-of-range integers to be cached differently; update the
logic in the block that calls this._audienceMatcher.evaluateRules (and similarly
in the other occurrence) to normalise any ruleVariant that is not null and is
outside the valid range (i.e., < 0 or >= experiment.variants.length) to null
before comparing to assignment.ruleVariant and before assigning
assignment.ruleVariant, so the cache only records either a valid variant index
or null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f432351-27b2-44ef-9d52-754ee6b24afc
📒 Files selected for processing (2)
src/__tests__/matcher.test.jssrc/context.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/matcher.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/context.ts (1)
499-509: Extract the rule evaluation into one helper.
ruleKeyconstruction andrawRuleVariantnormalisation now live in both the cache-validation path and the fresh-assignment path. These two branches need to stay perfectly in sync for cache invalidation to remain correct, so a small shared helper would make future edits much safer.Also applies to: 582-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 499 - 509, Extract the duplicated rule-evaluation + normalization logic into a single helper (e.g., a private method like computeRuleVariant) that takes (experiment, environmentId, attrs) or the minimal inputs and internally calls this._audienceMatcher.evaluateRules and normalizes rawRuleVariant into either a valid variant index or null; replace both uses of the inline logic (the cache-validation branch that checks assignment.ruleVariant and the fresh-assignment branch that sets assignment.ruleVariant) to call this helper so both paths use the exact same behavior (ensure you reference experiment.assignmentRules, this._environmentId, attrs, evaluateRules result, and assignment.ruleVariant in the new helper).src/__tests__/context.test.js (1)
2057-2736: Consider a small response-builder for the rules suite.Most of these cases rebuild the same
exp_test_abcfixture with tiny tweaks toaudience,assignmentRules, orenvironment_id. A helper here would cut a lot of noise and make each scenario’s intent much easier to read.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/context.test.js` around lines 2057 - 2736, The tests repeatedly reconstruct the same exp_test_abc experiment with small changes to audience, assignmentRules, or environment_id; add a small response-builder helper (e.g. makeRulesResponse or buildExperimentResponse) used by the "rules evaluation" suite to produce variants of the base getContextResponse for exp_test_abc by accepting overrides for audience, assignmentRules, and environment_id; update each test to call that helper (referencing the existing symbols assignmentRules, audience, envScopedRulesContextResponse, rulesContextResponse, rulesStrictContextResponse, etc.) instead of manually mapping getContextResponse.experiments each time to reduce duplication and improve readability.
🤖 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__/context.test.js`:
- Around line 2057-2736: The tests repeatedly reconstruct the same exp_test_abc
experiment with small changes to audience, assignmentRules, or environment_id;
add a small response-builder helper (e.g. makeRulesResponse or
buildExperimentResponse) used by the "rules evaluation" suite to produce
variants of the base getContextResponse for exp_test_abc by accepting overrides
for audience, assignmentRules, and environment_id; update each test to call that
helper (referencing the existing symbols assignmentRules, audience,
envScopedRulesContextResponse, rulesContextResponse, rulesStrictContextResponse,
etc.) instead of manually mapping getContextResponse.experiments each time to
reduce duplication and improve readability.
In `@src/context.ts`:
- Around line 499-509: Extract the duplicated rule-evaluation + normalization
logic into a single helper (e.g., a private method like computeRuleVariant) that
takes (experiment, environmentId, attrs) or the minimal inputs and internally
calls this._audienceMatcher.evaluateRules and normalizes rawRuleVariant into
either a valid variant index or null; replace both uses of the inline logic (the
cache-validation branch that checks assignment.ruleVariant and the
fresh-assignment branch that sets assignment.ruleVariant) to call this helper so
both paths use the exact same behavior (ensure you reference
experiment.assignmentRules, this._environmentId, attrs, evaluateRules result,
and assignment.ruleVariant in the new helper).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afcef707-87fa-4707-969e-95cf47eaedbb
📒 Files selected for processing (2)
src/__tests__/context.test.jssrc/context.ts
calthejuggler
approved these changes
Apr 13, 2026
5 tasks
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 splitgetEnvironment()toClientso the Context can access the environment name for rule filteringtargetingRuleflag to exposure events for explicit identification in analyticsrule.variantis a number; skip invalid rules instead of aborting iterationAssignment Flags
Rule-matched assignments set
assigned=true, eligible=true, overridden=true, targetingRule=true.All possible assignment states
assignedeligibleoverriddenfullOncustomaudienceMismatchtargetingRule*=audienceMismatchis set independently before rules, can be either value.Engine bitmask
targetingRuleuses bit 8 (value 256). The existing filterbitAnd(flags, 207) = 3has bit 8 as0in the mask, so it's automatically ignored — no engine query changes needed. The engine'sExposure.javaandEventForwardingManager.javawill need the new field when implementing server-side support.Test Plan
evaluateRulesin matcherSummary by CodeRabbit
New Features
Behaviour
Tests