Re-evaluate audience expression on attribute change#48
Conversation
WalkthroughThis pull request adds per-attribute-sequence tracking to Context by introducing a private Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)src/__tests__/context.test.js (1)
🔇 Additional comments (10)
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.40.0)src/__tests__/context.test.jsComment |
0cb6e25 to
aca8aef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/context.ts (1)
462-476: Consider adding a clarifying comment for the side effect.The
audienceMatchesfunction mutatesassignment.attrsSeq(line 472) even when returningtrue(cache valid). While this is intentional to prevent repeated re-evaluations when the audience result is unchanged, a brief inline comment would improve readability.if (newAudienceMismatch !== assignment.audienceMismatch) { return false; } + // Update attrsSeq to avoid re-evaluating on subsequent calls when result unchanged assignment.attrsSeq = this._attrsSeq; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/context.test.js(2 hunks)src/context.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/context.test.js (1)
src/context.ts (1)
publisher(243-245)
🔇 Additional comments (10)
src/__tests__/context.test.js (2)
1312-1365: Well-structured tests forpeek()audience re-evaluation.These tests effectively cover the key scenarios for audience re-evaluation via
peek():
- Strict mode re-evaluation when attributes change
- Non-strict mode re-evaluation when attributes change
- Caching behaviour when no new attributes are set
The assertions correctly verify that
pending()remains 0 sincepeek()should not queue exposures.
1799-2025: Comprehensive test coverage fortreatment()audience re-evaluation.The tests thoroughly cover:
- Re-evaluation in strict and non-strict modes (lines 1800-1838)
- Caching when no new attributes set (lines 1840-1857)
- Experiments without audience filter (lines 1859-1876)
- Transition from mismatch to match with exposure verification (lines 1878-1933)
- Attribute timing scenarios (before vs after assignment) (lines 1935-1971)
- Cache preservation when audience result unchanged (lines 1973-1993)
attrsSequpdate to prevent repeated evaluations (lines 1995-2025)The test at lines 1973-1993 is particularly valuable as it verifies that changing an attribute that doesn't affect the audience result (e.g., age 15 → 18, both still < 20) doesn't queue a new exposure.
src/context.ts (8)
65-66: LGTM - OptionalattrsSeqfield added to Assignment type.The optional property correctly supports tracking the attribute sequence at the time of assignment.
146-146: LGTM - Private counter for tracking attribute mutations.Correctly placed with other private state fields.
169-169: LGTM - Proper initialisation of_attrsSeqcounter.Initialising to 0 in the constructor is correct.
325-330: LGTM - Attribute sequence increment on attribute set.Incrementing
_attrsSeqafter adding the attribute ensures the counter reflects when attributes have changed since the last evaluation.
443-449: LGTM - Helper method to build attributes map.The implementation is consistent with the existing
getAttribute()logic where the last value wins for duplicate attribute names.
494-498: LGTM - Proper integration of audience matching with experiment matching.The short-circuit evaluation correctly checks
experimentMatchesfirst (cheaper metadata comparison) beforeaudienceMatches(which may re-evaluate the audience expression).
531-537: LGTM - Consistent use of_getAttributesMap()helper.Using the same helper for both initial audience evaluation and re-evaluation ensures consistent behaviour.
590-590: LGTM - Correct placement ofattrsSeqassignment.Setting
attrsSeqat the end of the experiment assignment block ensures it captures the current attribute sequence when the assignment is created, enabling proper re-evaluation detection on subsequent calls.
This PR add cache re-validation when attributes change after an assignment was cached.
Summary by CodeRabbit
Tests
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.