feat: snap extra parametrs pass #1921
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications across three files related to user trait handling in analytics integrations. In the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js (2)
16-16: Consider using relative paths for internal imports.The import statement uses a package name path which could be fragile if the package structure changes. Consider using a relative path instead.
-import { getDefinedTraits } from '@rudderstack/analytics-js-integrations/utils/utils'; +import { getDefinedTraits } from '../../utils/utils';
115-125: Consider adding validation for the new user traits.While the implementation is correct, consider adding validation for the new fields to ensure data quality:
- Age should be a positive number
- Postal code format varies by country
- State/region codes might need standardization
Example validation implementation:
const validateUserTraits = (traits) => { const { age, postalCode, state } = traits; if (age !== undefined && (typeof age !== 'number' || age <= 0)) { logger.warn('Invalid age value provided'); delete traits.age; } // Add more validation as needed return traits; }; // Use in identify method payload = validateUserTraits({ ...payload, ip_address: ipAddress, firstname: firstName, // ... other fields });packages/analytics-js-integrations/src/utils/utils.js (1)
274-280: Consider additional postal code field variationsThe implementation follows the established pattern for address fields and provides good coverage of common field names. Consider adding these additional variations that are commonly used:
- zip_code
- zipCode
- zipcode
- address.zip
- address.zip_code
postalCode: get(message, 'context.traits.postalCode') || get(message, 'context.traits.postalcode') || get(message, 'context.traits.postal_code') || get(message, 'context.traits.address.postalCode') || get(message, 'context.traits.address.postal_code') || - get(message, 'context.traits.address.postalcode'), + get(message, 'context.traits.address.postalcode') || + get(message, 'context.traits.zipCode') || + get(message, 'context.traits.zipcode') || + get(message, 'context.traits.zip_code') || + get(message, 'context.traits.zip') || + get(message, 'context.traits.address.zip') || + get(message, 'context.traits.address.zip_code'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js(3 hunks)packages/analytics-js-integrations/src/integrations/SpotifyPixel/browser.js(1 hunks)packages/analytics-js-integrations/src/utils/utils.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/analytics-js-integrations/src/integrations/SpotifyPixel/browser.js
🔇 Additional comments (3)
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js (1)
103-104: LGTM! Clean implementation of additional user traits.
The extraction of additional user traits is well-implemented and aligns with Snap's supported user properties.
packages/analytics-js-integrations/src/utils/utils.js (2)
151-151: LGTM: Minor string quote change
The change from double to single quotes for the circular reference string is consistent with the codebase's string formatting.
274-280: Verify impact on existing integrations
Let's verify if any existing integrations rely on specific postal code field names and ensure this change doesn't affect them.
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about postal code usage across integrations.
Changes to postal code field mapping are safe and aligned with existing integrations
The current implementation in utils.js that checks multiple variations of postal code fields (postalCode, postalcode, postal_code) is consistent with how integrations are using these fields. Specifically:
- SnapPixel uses
postalCodeformat - Klaviyo uses both
zipandaddress.postalCode - GoogleAds uses
postalCodeandpostal_code - Criteo uses
zipCodeandzip - AdobeAnalytics uses
zip
The current implementation in utils.js properly handles all these variations, ensuring backward compatibility and proper field mapping across integrations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for postal code usage in integration files
# Look for postal code field references in integration files
echo "Searching for postal code usage in integrations..."
rg -i "postal.*code|zip.*code|zipcode|zip" packages/analytics-js-integrations/src/integrations/
# Look for address field mapping patterns
echo "Searching for address field mapping patterns..."
ast-grep --pattern 'getDefinedTraits($message)' packages/analytics-js-integrations/src/integrations/
Length of output: 5352
size-limit report 📦
|
aashishmalik
force-pushed
the
feat.snap-userdata
branch
from
f2c6374
to
338cbb7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1921 +/- ##
===========================================
- Coverage 57.52% 57.51% -0.01%
===========================================
Files 485 485
Lines 16505 16508 +3
Branches 3315 3298 -17
===========================================
+ Hits 9494 9495 +1
- Misses 5732 5794 +62
+ Partials 1279 1219 -60 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js (1)
Line range hint
103-125: Consider adding validation for new user traitsWhile the code adds several new user traits, it doesn't validate their format or handle potential edge cases. Consider:
- Adding type checks for numeric fields like
age- Format validation for
postalCode- Length limits for
firstNameandlastName- Case normalization for geographic fields
Would you like me to provide an example implementation of these validations?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-115: packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js#L115
Added line #L115 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/analytics-js-integrations/__tests__/utils/utils.test.js(2 hunks)packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js(3 hunks)packages/analytics-js-integrations/src/integrations/SpotifyPixel/browser.js(1 hunks)packages/analytics-js-integrations/src/utils/utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-integrations/src/integrations/SpotifyPixel/browser.js
- packages/analytics-js-integrations/src/utils/utils.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js
[warning] 103-104: packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js#L103-L104
Added lines #L103 - L104 were not covered by tests
[warning] 115-115: packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js#L115
Added line #L115 was not covered by tests
🔇 Additional comments (5)
packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js (3)
16-16: LGTM: Import statement follows consistent pattern
The import of getDefinedTraits is properly structured and follows the established pattern in the codebase.
103-104: 🛠️ Refactor suggestion
Standardize trait extraction approach
The code uses two different approaches for trait extraction:
- Direct extraction using
get()forage - Using
getDefinedTraits()for other traits
This inconsistency could lead to maintenance issues.
Consider using getDefinedTraits() for all traits:
-const age = get(message, 'context.traits.age');
-const { firstName, lastName, postalCode, country, city, state } = getDefinedTraits(message);
+const { age, firstName, lastName, postalCode, country, city, state } = getDefinedTraits(message);Additionally, these new changes lack test coverage.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 103-104: packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js#L103-L104
Added lines #L103 - L104 were not covered by tests
115-125: Verify field names against Snap's API documentation
The payload includes several new user traits. While the implementation looks clean, we should verify that the field names exactly match Snap's API expectations:
firstnamelastnamegeo_citygeo_regiongeo_postal_codegeo_country
Also, consider adding JSDoc comments to document these fields and their expected formats.
✅ Verification successful
Field names match Snap's API requirements
Based on the codebase analysis, the field names used in the payload are correct and consistent with Snap's API requirements:
- The implementation follows a well-established pattern across the codebase
- Field names like
firstname,lastname,geo_city,geo_region,geo_postal_code, andgeo_countryare used consistently in the SnapPixel integration - The code includes proper payload sanitization through
removeUndefinedAndNullValues - The implementation handles both hashed and unhashed user data appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing usage of these field names in tests or documentation
rg -l "firstname|lastname|geo_city|geo_region|geo_postal_code|geo_country" .
Length of output: 1176
Script:
#!/bin/bash
# Let's check the specific usage of these fields in SnapPixel integration and its tests
rg -A 5 -B 5 "firstname|lastname|geo_city|geo_region|geo_postal_code|geo_country" ./packages/analytics-js-integrations/src/integrations/SnapPixel/ ./packages/analytics-js-integrations/__tests__/integrations/SnapPixel/
# Also check for any Snap-specific documentation or constants
rg -A 5 -B 5 "Snap" ./packages/analytics-js-common/src/constants/integrations/
Length of output: 24391
Script:
#!/bin/bash
# Let's check for any Snap Pixel specific documentation or test files
rg -A 10 "payload" ./packages/analytics-js-integrations/src/integrations/SnapPixel/
# Also check for any test files that might contain payload examples
fd -e test.js -e spec.js . -x rg -l "SnapPixel.*payload"
Length of output: 16736
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-115: packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js#L115
Added line #L115 was not covered by tests
packages/analytics-js-integrations/__tests__/utils/utils.test.js (2)
331-331: LGTM! Minor formatting improvement
The formatting change in the circular reference test case maintains consistent test behavior.
130-130: LGTM! Verify related changes in utils.js
The addition of postalCode assertion aligns with the PR objective of passing extra parameters in snap functionality.
Let's verify the corresponding implementation in utils.js:
✅ Verification successful
LGTM! The postalCode field is properly implemented
The getDefinedTraits function in utils.js correctly handles the postalCode field by checking multiple possible paths and variations:
postalCode:
get(message, 'context.traits.postalCode') ||
get(message, 'context.traits.postalcode') ||
get(message, 'context.traits.postal_code') ||
get(message, 'context.traits.address.postalCode') ||
get(message, 'context.traits.address.postal_code') ||
get(message, 'context.traits.address.postalcode'),The implementation:
- Follows the same pattern as other address fields (city, state, country)
- Handles multiple casing variations (postalCode, postalcode, postal_code)
- Checks both root traits and nested address object
- Returns undefined if no value is found (matching test assertion)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify postalCode handling in getDefinedTraits implementation
# Expected: Should find postalCode being extracted from message object
ast-grep --pattern 'function getDefinedTraits($_) {
$$$
postalCode: $_
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Let's try a different approach to find getDefinedTraits implementation
# First, find the utils.js file that corresponds to the test file
fd "utils.js" --type f --exclude "__tests__" packages/analytics-js-integrations
# Then let's examine its content to find getDefinedTraits
rg "getDefinedTraits" packages/analytics-js-integrations -A 10 -B 2
Length of output: 44332
Script:
#!/bin/bash
# Let's examine the implementation of getDefinedTraits in utils.js
cat packages/analytics-js-integrations/src/utils/utils.js
Length of output: 15688
PR Description
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests
These updates improve the accuracy and comprehensiveness of user data sent to analytics services.