chore(release): pull main into develop post release v1.126.2#5048
chore(release): pull main into develop post release v1.126.2#5048devops-github-rudderstack wants to merge 4 commits intodevelopfrom
Conversation
👑 *An automated PR*
👑 *An automated PR* --------- Co-authored-by: Sudip Paul <67197965+ItsSudip@users.noreply.github.com> Co-authored-by: GitHub Actions <noreply@github.com>
👑 *An automated PR* --------- Co-authored-by: Sudip Paul <67197965+ItsSudip@users.noreply.github.com> Co-authored-by: GitHub Actions <noreply@github.com>
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Facebook Lead Ads Hydration src/sources/facebook_lead_ads_native/hydrate.ts, src/sources/facebook_lead_ads_native/hydrate.test.ts |
Replaces direct OK status check with generic HTTP success check using isHttpStatusSuccess(). Adds structured logging via JsonSchemaGenerator for non-OK responses. Introduces 4 new test cases verifying error handling for various error object shapes (missing property, empty object, string, null) with assertion of rejection message and logger calls. |
Shopify Web Pixel Email Handling src/sources/shopify/webpixelTransformations/pixelUtils.js, src/sources/shopify/webpixelTransformations/pixelTransform.test.js |
Introduces setCheckoutEmail() helper function to extract and set email from inputEvent.data.checkout.email into context.traits.email. Integrates this function into both checkoutEventBuilder and checkoutStepEventBuilder. Adds 3 new tests validating email extraction for checkout_completed events and proper absence of email for non-checkout scenarios. |
Iterable Email Validation src/sources/iterable/transform.js |
Adds required-field check for event.dataFields.email in checkForRequiredFields(). Updates userId generation logic to derive from message.context.traits.email instead of event.email when userId is missing. |
Mixpanel Optional Chaining src/v0/destinations/mp/transform.js |
Applies optional chaining operator to safely access message.context?.traits when computing segregatedTraits. Adds conditional guard ensuring segregatedTraits.contextTraits assignment only occurs if message.context is present. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
- achettyiitr
- saikumarrs
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description is minimal and generic (":crown: An automated PR"), missing all required template sections including what changes were introduced, related Linear task, objectives, and developer/reviewer checklists. | Populate the description template with specific details: summarize the integrated changes, reference the Linear task, explain release integration objectives, and complete the developer and reviewer checklists. | |
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly describes the main change: a release-related PR that integrates the main branch back into develop following a release version bump. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
main
📝 Coding Plan
- Generate coding plan for human review comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
Tip
CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.
Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/sources/facebook_lead_ads_native/hydrate.test.ts (1)
399-516: Excellent test coverage for theerrorResponseHandleredge cases.The four test scenarios comprehensively cover the conditions where
errorResponseHandlerreturns without throwing:
- Response with no
errorproperty- Empty object body
- String body (non-JSON structure)
- Explicit
nullerrorEach test correctly validates both the rejection behavior and the structured logging with schema output. This ensures the defensive error handling in
hydrate.tsis properly exercised.One minor suggestion: the suite name could be more specific to clarify the conditions under which the handler doesn't throw.
💡 Optional: More descriptive suite name
- describe('errorResponseHandler does not throw', () => { + describe('non-OK responses without valid error structure', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sources/facebook_lead_ads_native/hydrate.test.ts` around lines 399 - 516, The describe block name "errorResponseHandler does not throw" is vague—rename it to a more specific suite title that reflects the conditions being tested (e.g., "errorResponseHandler: non-OK responses without valid error field should log schema and throw") and update any individual it() names if needed for clarity; locate the describe wrapping the tests referencing hydrate and errorResponseHandler in hydrate.test.ts and change the string literal only (no behavior changes) so the suite clearly documents that these tests cover non-OK Facebook responses with missing/invalid error payloads.src/sources/facebook_lead_ads_native/hydrate.ts (1)
113-118: Solid defensive error handling for the edge case whereerrorResponseHandlerreturns without throwing.The logging and throw correctly handle scenarios where Facebook returns non-OK responses without a proper
errorstructure (e.g., missingerrorproperty, empty body, null error). Per theerrorResponseHandlerimplementation innetworkHandler.js, it returns early when!response.erroris truthy.One consideration: using
JsonSchemaGenerator.generate(processedResponse)logs the structural schema rather than actual values—good for avoiding PII exposure, but may limit debugging visibility. A brief inline comment explaining this privacy-conscious choice would be helpful for future maintainers.💡 Optional: Add clarifying comment
+ // Log schema (not actual values) to avoid exposing potentially sensitive data logger.error(`[facebook_lead_ads_native] Non-OK response from Facebook API`, { status: processedResponse.status, response: JSON.stringify(JsonSchemaGenerator.generate(processedResponse)), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sources/facebook_lead_ads_native/hydrate.ts` around lines 113 - 118, Add a short inline comment above the logger.error call in hydrate.ts explaining that JsonSchemaGenerator.generate(processedResponse) is used intentionally to log the response schema (not raw values) to avoid logging PII while still providing structure for debugging; reference the involved symbols logger.error, JsonSchemaGenerator.generate, processedResponse and note that this is defensive handling for the edge-case where errorResponseHandler returns without throwing.src/sources/iterable/transform.js (1)
57-61: Correct alignment with mapping contract.Using
message.context.traits.emailinstead ofevent.emailproperly reflects the mapping consolidation where bothdataFields.emailresolve tocontext.traits.email. This ensures consistent userId generation regardless of the source field.One edge case to note:
checkForRequiredFieldsallows events with onlyuserId(no email fields). In this scenario,message.context.traits.emailwould be undefined, causingmd5(undefined)to produce a deterministic but likely unintended hash. While this edge case is theoretically possible based on the validation logic and exists in the prior implementation as well, current test coverage does not demonstrate userId-only scenarios. Consider adding a defensive check if such events are expected in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sources/iterable/transform.js` around lines 57 - 61, The current code sets message.userId = md5(message.context.traits.email) unconditionally, which can produce an unintended hash when context.traits.email is undefined; update the logic in src/sources/iterable/transform.js so you only call md5 when message.context?.traits?.email is defined (i.e., check that message.context and message.context.traits.email exist) and otherwise leave message.userId alone (or explicitly do not generate a userId), referencing the symbols message.userId, md5, message.context.traits.email and the validation path checkForRequiredFields to locate the fix.
🤖 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/sources/facebook_lead_ads_native/hydrate.test.ts`:
- Around line 399-516: The describe block name "errorResponseHandler does not
throw" is vague—rename it to a more specific suite title that reflects the
conditions being tested (e.g., "errorResponseHandler: non-OK responses without
valid error field should log schema and throw") and update any individual it()
names if needed for clarity; locate the describe wrapping the tests referencing
hydrate and errorResponseHandler in hydrate.test.ts and change the string
literal only (no behavior changes) so the suite clearly documents that these
tests cover non-OK Facebook responses with missing/invalid error payloads.
In `@src/sources/facebook_lead_ads_native/hydrate.ts`:
- Around line 113-118: Add a short inline comment above the logger.error call in
hydrate.ts explaining that JsonSchemaGenerator.generate(processedResponse) is
used intentionally to log the response schema (not raw values) to avoid logging
PII while still providing structure for debugging; reference the involved
symbols logger.error, JsonSchemaGenerator.generate, processedResponse and note
that this is defensive handling for the edge-case where errorResponseHandler
returns without throwing.
In `@src/sources/iterable/transform.js`:
- Around line 57-61: The current code sets message.userId =
md5(message.context.traits.email) unconditionally, which can produce an
unintended hash when context.traits.email is undefined; update the logic in
src/sources/iterable/transform.js so you only call md5 when
message.context?.traits?.email is defined (i.e., check that message.context and
message.context.traits.email exist) and otherwise leave message.userId alone (or
explicitly do not generate a userId), referencing the symbols message.userId,
md5, message.context.traits.email and the validation path checkForRequiredFields
to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7ad3a58-b939-4afb-8789-69e087fe0092
⛔ Files ignored due to path filters (10)
CHANGELOG.mdis excluded by!**/*.mdgo/webhook/testcases/testdata/testcases/iterable/test_1_0.jsonis excluded by!**/*.jsongo/webhook/testcases/testdata/testcases/iterable/test_30_0.jsonis excluded by!**/*.jsonpackage-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.jsonsrc/sources/iterable/mapping.jsonis excluded by!**/*.jsontest/integrations/destinations/mp/processor/data.tsis excluded by!**/test/**test/integrations/sources/iterable/data.tsis excluded by!**/test/**test/integrations/sources/shopify/pixelMappingTests/CheckoutStepsTests.tsis excluded by!**/test/**test/integrations/sources/shopify/pixelTestScenarios/CheckoutStepsTests.tsis excluded by!**/test/**
📒 Files selected for processing (6)
src/sources/facebook_lead_ads_native/hydrate.test.tssrc/sources/facebook_lead_ads_native/hydrate.tssrc/sources/iterable/transform.jssrc/sources/shopify/webpixelTransformations/pixelTransform.test.jssrc/sources/shopify/webpixelTransformations/pixelUtils.jssrc/v0/destinations/mp/transform.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5048 +/- ##
===========================================
- Coverage 92.30% 92.27% -0.03%
===========================================
Files 663 656 -7
Lines 36490 35915 -575
Branches 8622 8469 -153
===========================================
- Hits 33681 33142 -539
+ Misses 2592 2535 -57
- Partials 217 238 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
👑 An automated PR