feat: snap extra parametrs pass #1921
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications across multiple files related to user trait handling in analytics integrations. The Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@aashishmalik has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1921 +/- ##
===========================================
+ Coverage 57.83% 58.01% +0.17%
===========================================
Files 485 485
Lines 16506 16516 +10
Branches 3315 3314 -1
===========================================
+ Hits 9547 9582 +35
- Misses 5677 5709 +32
+ Partials 1282 1225 -57 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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
aashishmalik
force-pushed
the
feat.snap-userdata
branch
from
338cbb7 to
28b2963
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js-integrations/__tests__/integrations/SnapPixel/browser.test.js (2)
1-2: Consider addressing ESLint warnings instead of disabling them.The ESLint rules
sonarjs/no-duplicate-stringandno-underscore-dangleare disabled globally for the file. Consider:
- Extracting duplicate strings into constants
- Using alternative patterns to avoid underscore dangles
15-21: Add error handling for script element manipulation.The script element insertion could fail if the document doesn't have a head element. Consider adding error handling:
const headElements = document.getElementsByTagName('head'); -headElements[0].insertBefore(scriptElement, headElements[0].firstChild); +if (headElements.length > 0) { + headElements[0].insertBefore(scriptElement, headElements[0].firstChild); +} else { + throw new Error('Document head not found'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/analytics-js-integrations/__tests__/integrations/SnapPixel/browser.test.js(1 hunks)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 (4)
- packages/analytics-js-integrations/tests/utils/utils.test.js
- packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js
- packages/analytics-js-integrations/src/integrations/SpotifyPixel/browser.js
- packages/analytics-js-integrations/src/utils/utils.js
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-integrations/__tests__/integrations/SnapPixel/browser.test.js (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js-integrations/__tests__/integrations/SpotifyPixel/browser.test.js:249-260
Timestamp: 2024-11-12T15:14:33.334Z
Learning: Inconsistencies in data structures such as `line_items` within test scripts of the `SpotifyPixel` and `Podsights` integrations can be ignored as per user guidance, as they do not necessarily reflect production code requirements.
🔇 Additional comments (1)
packages/analytics-js-integrations/__tests__/integrations/SnapPixel/browser.test.js (1)
72-83: Verify trait mapping consistency with Snap Pixel documentation.
The test assumes specific parameter names for the Snap Pixel initialization. Let's verify these match Snap's official documentation:
✅ Verification successful
Let me check the actual implementation and configuration files to verify the parameter mapping.
The parameter names in the test match Snap Pixel's initialization requirements
After reviewing the implementation, the parameter names used in the test (user_email, phone_number, ip_address, firstname, lastname, age, geo_city, geo_region, geo_postal_code, geo_country) are correctly mapped in the identify method of the SnapPixel integration. The implementation also supports hashed variants of email and phone number when hashMethod is set to 'sha256'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any configuration or constant files that might define the expected parameter names
rg -g '!node_modules' -g '!dist' -g '!*.test.js' -l 'snaptr|snap_pixel|snap.pixel' .
# Search for any mapping configurations
rg -g '!node_modules' -g '!dist' -l 'trait.*mapping|parameter.*mapping' .
Length of output: 722
Script:
#!/bin/bash
# Check the constants file for parameter definitions
cat ./packages/analytics-js-common/src/constants/integrations/SnapPixel/constants.ts
# Check the browser implementation for parameter mapping
cat ./packages/analytics-js-integrations/src/integrations/SnapPixel/browser.js
# Check utils for any parameter transformations
cat ./packages/analytics-js-integrations/src/integrations/SnapPixel/util.js
Length of output: 14348
packages/analytics-js-integrations/__tests__/integrations/SnapPixel/browser.test.js
Outdated
Show resolved
Hide resolved
aashishmalik
force-pushed
the
feat.snap-userdata
branch
from
28b2963 to
6c7a091
Compare
aashishmalik
force-pushed
the
feat.snap-userdata
branch
from
6c7a091 to
636226c
Compare
|
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.