-
Notifications
You must be signed in to change notification settings - Fork 5
Re-evaluate audience expression on attribute change #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.