Skip to content

fix(context): forward all BucketingAttributes to ExperienceManager - #382

Merged
JosephSamirL merged 1 commit into
main-convertfrom
fix/context-forward-bucketing-attributes
May 21, 2026
Merged

fix(context): forward all BucketingAttributes to ExperienceManager#382
JosephSamirL merged 1 commit into
main-convertfrom
fix/context-forward-bucketing-attributes

Conversation

@abbaseya

Copy link
Copy Markdown
Collaborator

Summary

Context.runExperience and Context.runExperiences rebuilt a fresh attributes object with only four hardcoded keys (visitorProperties, locationProperties, updateVisitorProperties, environment) before forwarding to ExperienceManager.selectVariation / selectVariations. Every other BucketingAttributes field — enableTracking, forceVariationId, ignoreLocationProperties, typeCasting, experienceKeys — was silently dropped.

The most user-visible consequence: a caller invoking context.runExperience(key, { enableTracking: false }) still fires the bucketing track event, because by the time the call reaches DataManager._getBucketingByField (which destructures enableTracking = true as a default), the field has been discarded one layer up. Same for forceVariationId (the forced variation was never honored) and ignoreLocationProperties.

Root cause

A whitelist-then-rebuild pattern in Context instead of spread-then-override. Two transforms must happen at the Context layer:

  1. visitorProperties must be merged with stored visitor props via getVisitorProperties().
  2. environment must default to this._environment when the caller omits it.

Everything else can pass through unmodified; downstream destructures by name and ignores unknown keys.

Fix

// before
{
  visitorProperties,
  locationProperties: attributes?.locationProperties,
  updateVisitorProperties: attributes?.updateVisitorProperties,
  environment: attributes?.environment || this._environment
}

// after
{
  ...attributes,
  visitorProperties,
  environment: attributes?.environment || this._environment
}

Applied identically to runExperience (line 135) and runExperiences (line 190).

{...undefined} evaluates to {} in JS, so the optional-attributes case is safe.

Tests

Added two cases in packages/js-sdk/tests/context.tests.ts:

  • Should forward BucketingAttributes (enableTracking, forceVariationId, ignoreLocationProperties) through runExperience
  • Should forward BucketingAttributes (enableTracking, forceVariationId, ignoreLocationProperties) through runExperiences

Both monkey-patch experienceManager.selectVariation / selectVariations to capture the attributes argument, then assert the previously-dropped fields are forwarded. Use enableTracking: false to suppress the tracking request so the tests stay deterministic.

Test plan

  • yarn test:mocha from packages/js-sdk/ — 143 passing, 0 failing
  • yarn lint in packages/js-sdk/ — clean
  • Existing Shoud successfully get variation from specific experience and Shoud successfully get variations across all experiences still pass (no regression in the standard path)

Notes

  • Fix is API-surface-neutral. Callers that were never passing the dropped fields see no change. Callers that were passing them and silently getting the default behavior will now get the documented behavior — this is what they expected from the public BucketingAttributes type all along.
  • No changes to BucketingAttributes typing, DataManager, ExperienceManager, or any downstream layer.

🤖 Generated with Claude Code

Context.runExperience and Context.runExperiences rebuilt a fresh
attributes object with only four hardcoded keys (visitorProperties,
locationProperties, updateVisitorProperties, environment) before
forwarding to ExperienceManager.selectVariation/selectVariations.

This silently dropped every other BucketingAttributes field —
enableTracking, forceVariationId, ignoreLocationProperties,
typeCasting, experienceKeys — so callers opting out of tracking,
forcing a variation, or ignoring location targeting were silently
ignored. DataManager._getBucketingByField destructures these by name
with enableTracking defaulting to `true`, which is how the opt-out
was being lost.

Switch to spread-then-override so all caller-supplied attributes pass
through while still applying the two transforms Context must do:
merging visitorProperties via getVisitorProperties() and defaulting
environment to the Context's own environment.

Add tests covering forwarding of enableTracking, forceVariationId,
ignoreLocationProperties, and updateVisitorProperties through both
runExperience and runExperiences.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@abbaseya abbaseya self-assigned this May 21, 2026
@abbaseya
abbaseya requested a review from JosephSamirL May 21, 2026 00:47
@sonarqubecloud

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the Context class to forward all attributes to the ExperienceManager using the spread operator, enabling support for properties like enableTracking and forceVariationId. Corresponding tests were added to verify this behavior. However, feedback indicates that the Context layer still emits SystemEvents.BUCKETING events even when enableTracking is set to false. It is recommended to conditionally suppress these events based on the attribute flag and to update the test suite to verify this suppression.

Comment thread packages/js-sdk/src/context.ts
Comment thread packages/js-sdk/src/context.ts
Comment thread packages/js-sdk/tests/context.tests.ts
@JosephSamirL
JosephSamirL merged commit b215167 into main-convert May 21, 2026
6 checks passed
@JosephSamirL
JosephSamirL deleted the fix/context-forward-bucketing-attributes branch May 21, 2026 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants