refactor: use environment names instead of IDs for rule matching#58
refactor: use environment names instead of IDs for rule matching#58calthejuggler merged 4 commits intomainfrom
Conversation
WalkthroughThe PR migrates environment scoping from numeric IDs to environment names across core logic and tests. Context now tracks Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
calthejuggler
requested review from
marcio-absmartly and
mario-silva
as code owners
![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
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)
487-525:⚠️ Potential issue | 🟠 MajorInclude injected system attributes in the assignment cache key.
audienceMatches()now depends onapplication/environment/app_versionvia_getAttributesMap(), but this branch only re-runs whenthis._attrsSeqorruleKeychanges.this._attrsSeqonly trackscontext.attribute()calls, andruleKeyonly capturesassignmentRules + _environmentName, so a changedgetApplication()/app_versionleaves a stale assignment cached afterrefresh(). Please fold a stable snapshot of the injected system attributes into this invalidation check before reusingassignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 487 - 525, The assignment cache is not invalidated when injected system attributes change; update audienceMatches to incorporate a stable snapshot key of injected system attributes (e.g., application, environment/_environmentName, app_version) when deciding to reuse an Assignment: compute a deterministic systemAttrsKey from this._getAttributesMap() or from getApplication()/this._environmentName/app_version, compare it to a stored assignment.systemAttrsKey (or add that field to Assignment), and treat a mismatch like ruleKeyChanged/attrsSeq change so the method recomputes ruleVariant/audience and updates assignment.systemAttrsKey along with assignment.ruleKey and assignment.attrsSeq before returning the cached assignment.
🤖 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 461-469: The environment attribute is left undefined here while
_init() coerces client.getEnvironment() to null; update the injection to
normalize the value the same way by assigning attrs["environment"] =
client.getEnvironment() ?? null when this._opts.includeSystemAttributes is true
(locate the block using this._sdk.getClient(), app = client.getApplication(),
and attrs["application"]), and make the same null-coercion change in
_buildAttributes() so rule vars and serialized system attributes receive null
instead of undefined.
---
Outside diff comments:
In `@src/context.ts`:
- Around line 487-525: The assignment cache is not invalidated when injected
system attributes change; update audienceMatches to incorporate a stable
snapshot key of injected system attributes (e.g., application,
environment/_environmentName, app_version) when deciding to reuse an Assignment:
compute a deterministic systemAttrsKey from this._getAttributesMap() or from
getApplication()/this._environmentName/app_version, compare it to a stored
assignment.systemAttrsKey (or add that field to Assignment), and treat a
mismatch like ruleKeyChanged/attrsSeq change so the method recomputes
ruleVariant/audience and updates assignment.systemAttrsKey along with
assignment.ruleKey and assignment.attrsSeq before returning the cached
assignment.
🪄 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: 24af86e7-87fc-42db-b4ec-18fc0c40aff1
📒 Files selected for processing (4)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/context.tssrc/matcher.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/context.ts (1)
461-469: Consider extracting the sharedapp_versioncondition.The condition for including
app_version(lines 466–468) is duplicated in_buildAttributes()(lines 871–873). Extracting this to a helper method would reduce duplication and ensure consistent behaviour if the logic changes.♻️ Optional refactor to extract shared logic
private _shouldIncludeAppVersion(version: string | number): boolean { return (typeof version === "string" && version.length > 0) || (typeof version === "number" && version > 0); }Then use it in both methods:
-if ((typeof app.version === "string" && app.version.length > 0) || (typeof app.version === "number" && app.version > 0)) { +if (this._shouldIncludeAppVersion(app.version)) { attrs["app_version"] = app.version; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context.ts` around lines 461 - 469, Extract the duplicated app_version check into a single helper on the Context class (e.g. _shouldIncludeAppVersion(version: string | number): boolean) and replace the inline condition in both the includeSystemAttributes block (where attrs["app_version"] is set) and the _buildAttributes() method to call this helper; ensure the helper returns true for non-empty strings or numbers > 0 so behavior stays identical and update both sites to use the new method.
🤖 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/context.ts`:
- Around line 461-469: Extract the duplicated app_version check into a single
helper on the Context class (e.g. _shouldIncludeAppVersion(version: string |
number): boolean) and replace the inline condition in both the
includeSystemAttributes block (where attrs["app_version"] is set) and the
_buildAttributes() method to call this helper; ensure the helper returns true
for non-empty strings or numbers > 0 so behavior stays identical and update both
sites to use the new method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2bf163fc-caae-45e1-905e-03fc0617d467
📒 Files selected for processing (4)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/context.tssrc/matcher.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/context.test.js (1)
2575-2637: Add a regression test forenvironmentnormalisation tonull.Lines 2575–2637 cover system-attribute matching well for
"production", but they do not protect the recent undefined→null normalisation. Please add one case withclient.getEnvironmentreturningundefinedand assert the emitted/matchedenvironmentattribute isnull.🧪 Suggested test addition
+ it("should normalise environment system attribute to null when getEnvironment returns undefined", () => { + client.getEnvironment.mockReturnValueOnce(undefined); + const envNullRuleResponse = buildRulesResponse({ + assignmentRules: JSON.stringify({ + rules: [ + { + name: "Null Environment", + type: "assign", + conditions: { and: [{ eq: [{ var: "environment" }, { value: null }] }] }, + environments: [], + variant: 1, + }, + ], + }), + }); + const optionsWithSystemAttrs = { + publishDelay: -1, + refreshPeriod: 0, + includeSystemAttributes: true, + }; + const context = new Context(sdk, optionsWithSystemAttrs, contextParams, envNullRuleResponse); + expect(context.treatment("exp_test_abc")).toEqual(1); + });🤖 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 2575 - 2637, Add a regression test that ensures environment undefined is normalised to null before rule evaluation: in the test suite create a rule that matches environment === null (use buildRulesResponse with a rule whose condition compares { var: "environment" } to { value: null }), stub the SDK client getEnvironment to return undefined (or set contextParams/environment to undefined) and construct Context with includeSystemAttributes: true (as in optionsWithSystemAttrs), then call Context.treatment("exp_test_abc") and assert it matches the rule variant (1). Place this alongside the existing environment tests and reference Context and treatment to locate where to add the new case.
🤖 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 2575-2637: Add a regression test that ensures environment
undefined is normalised to null before rule evaluation: in the test suite create
a rule that matches environment === null (use buildRulesResponse with a rule
whose condition compares { var: "environment" } to { value: null }), stub the
SDK client getEnvironment to return undefined (or set contextParams/environment
to undefined) and construct Context with includeSystemAttributes: true (as in
optionsWithSystemAttrs), then call Context.treatment("exp_test_abc") and assert
it matches the rule variant (1). Place this alongside the existing environment
tests and reference Context and treatment to locate where to add the new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ace070e-d8c3-48c8-bd39-51897c74833f
📒 Files selected for processing (3)
src/__tests__/context.test.jssrc/__tests__/matcher.test.jssrc/context.ts
✅ Files skipped from review due to trivial changes (1)
- src/context.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/matcher.test.js
2 participants
Description
Switch assignment rule environment matching from numeric
environment_id(from API response) to string environment names (from client). This aligns the SDK with how the collector handles environment-scoped rules and simplifies the matching logic.Solution
evaluateRulesinmatcher.tsnow acceptsenvironmentName: string | nullinstead ofenvironmentId: number | null, matching rule environments by name string rather than numeric ID.audienceMismatchis only set when no rule override applies, preventing incorrect flags when a rule match should take precedence.application,environment, andapp_versionas automatically-injected attributes whenincludeSystemAttributesoption is enabled, allowing rules to match on these without manual attribute setting.environment_idfromContextData: The environment is now resolved from the client viagetEnvironment()rather than from the API response payload.Other Changes
mockReturnValueOncecalls tomockReturnValuefor consistency in tests.How to Test
npm testType of change
Checklist
Summary by CodeRabbit
Refactor
Bug Fixes
Tests