feat(fb_custom_audience): handle invalid fields and events#5023
Merged
shekhar-rudder merged 5 commits intodevelopfrom Mar 11, 2026
Merged
feat(fb_custom_audience): handle invalid fields and events#5023shekhar-rudder merged 5 commits intodevelopfrom
shekhar-rudder merged 5 commits intodevelopfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Configuration & Feature Toggle src/v0/destinations/fb_custom_audience/config.ts |
Adds isRejectInvalidFieldsEnabled() that reads FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS and exports it. |
Field Validation & Formatting src/v0/destinations/fb_custom_audience/util.ts |
Updates ensureApplicableFormat(...) signature to include workspaceId and destinationId; implements EMAIL and COUNTRY validation with conditional rejection (controlled by config), preserves PHONE normalization, threads workspace/destination IDs into validation, and replaces per-user all-null metrics path with throwing an InstrumentationError listing invalid/null fields. |
Record Processing Pipeline src/v0/destinations/fb_custom_audience/recordTransform.ts |
Updates processRecord signature to accept workspaceId and destinationId, passes those through to formatting/validation, removes a stats-only all-null-field increment and instead surfaces an InstrumentationError, and ensures input metadata/destination ID are propagated. |
Sequence Diagram(s)
sequenceDiagram
participant Event as Input Event
participant Proc as processRecord
participant Prep as prepareDataField
participant Val as ensureApplicableFormat
participant Conf as config (isRejectInvalidFieldsEnabled)
participant Out as Destination Output
Event->>Proc: deliver record + metadata (workspaceId, destinationId)
Proc->>Prep: map fields -> prepareDataField(field, workspaceId, destinationId)
Prep->>Val: ensureApplicableFormat(property, value, workspaceId, destinationId)
Val->>Conf: check isRejectInvalidFieldsEnabled()
alt field valid or allowed
Val-->>Prep: formatted/hashed value
else field invalid and reject enabled
Val-->>Prep: empty/removed value (or InstrumentationError for all-invalid user)
end
Prep->>Proc: assembled dataElement
alt all fields invalid
Proc-->>Event: throw InstrumentationError(invalid_fields...)
else
Proc->>Out: emit transformed record
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Reviewer suggestions for improvement
- Ensure all call sites supplying
processRecordare updated and validated to always pass non-emptyworkspaceIdanddestinationId. - Add unit tests for: EMAIL/COUNTRY valid vs invalid behavior with both states of
FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS, PHONE normalization edge cases, and the InstrumentationError path when all user fields are invalid. - Confirm logging/monitoring captures the new InstrumentationError details (workspaceId, destinationId, invalid fields) for alerting and debugging.
- Consider centralizing validation error keys/messages to keep metrics and logs consistent across destinations.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The description is largely incomplete; it provides a clear summary of changes but lacks crucial required sections from the template such as Linear task reference, explanations of behavior changes, dependencies, and utility modifications. | Add the missing template sections: specify the related Linear task (INT-XXX), document behavior changes and reasons, note any new dependencies, and explain modifications to generic utilities. Complete the developer and reviewer checklists where applicable. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main changes: adding validation for invalid email and country fields, and improving error handling for events with all null user properties in the fb_custom_audience destination. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✏️ 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
int-5922-fb-audience-invalid-events
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/v0/destinations/fb_custom_audience/recordTransform.ts (1)
104-116:⚠️ Potential issue | 🟠 MajorDon’t let one bad record fail the whole chunk.
processRecord()now throws for an all-null/invalid record, and that exception will rejectmapInBatches()for the entirerecordArray. The result is batch-level data loss: one invalid input prevents every other valid record in the same chunk from being transformed. This needs per-record isolation here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 104 - 116, processRecord() can throw for a bad record which will reject mapInBatches and cause the whole recordArray to fail; wrap the per-record transformation in a try/catch inside the mapInBatches callback (the block using processRecord within the forEachInBatches over recordChunksArray) so a single thrown error is caught, convert that failure into a per-record error metadata entry pushed into metadata, and return a harmless sentinel (e.g., null) for that record; after mapInBatches completes, filter out null/failed entries from data before continuing so valid records in the chunk are preserved. Ensure you reference the same identifiers: processRecord, mapInBatches, forEachInBatches, recordChunksArray, and metadata when making the change.
🤖 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/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 64-66: The current guard uses isHashRequired so
ensureApplicableFormat(...) is skipped when hashing is off, allowing invalid
EMAIL/COUNTRY values through; remove the isHashRequired check so the format
validation runs for raw/unhashed events as well (i.e., call
ensureApplicableFormat when disableFormat is false regardless of
isHashRequired). Update the conditional in recordTransform.ts (around
updatedProperty assignment using eachProperty, userProperty, workspaceId,
destinationId) and make the equivalent change in processRecord() to keep the
audiencelist and record paths in sync.
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 240-246: The validation call to ensureApplicableFormat is
currently only executed when isHashRequired is true, so unhashed uploads bypass
email/country validation and metrics; change the gating logic to run
ensureApplicableFormat whenever disableFormat is false (i.e., remove dependency
on isHashRequired) so updatedProperty = ensureApplicableFormat(eachProperty,
userProperty, workspaceId, destinationId) executes for both hashed and unhashed
paths and the new stats/validation are emitted; keep any downstream hashing
logic tied to isHashRequired unchanged.
---
Outside diff comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 104-116: processRecord() can throw for a bad record which will
reject mapInBatches and cause the whole recordArray to fail; wrap the per-record
transformation in a try/catch inside the mapInBatches callback (the block using
processRecord within the forEachInBatches over recordChunksArray) so a single
thrown error is caught, convert that failure into a per-record error metadata
entry pushed into metadata, and return a harmless sentinel (e.g., null) for that
record; after mapInBatches completes, filter out null/failed entries from data
before continuing so valid records in the chunk are preserved. Ensure you
reference the same identifiers: processRecord, mapInBatches, forEachInBatches,
recordChunksArray, and metadata when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12b99750-96cd-4b59-86a5-40ba689ae40f
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/transform.tssrc/v0/destinations/fb_custom_audience/util.ts
achettyiitr
reviewed
Mar 10, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/v0/destinations/fb_custom_audience/config.ts (1)
127-136: Well-documented feature flag implementation.The function is clean, follows existing patterns in this file (like
getMaxPayloadSize), and the documentation clearly explains the behavior and default.One minor consideration: the comparison
=== 'true'is case-sensitive, so'TRUE'or'True'won't enable the feature. This is a common convention, but if you want to be more lenient:💡 Optional: Case-insensitive check
function isRejectInvalidFieldsEnabled(): boolean { - return process.env.FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS === 'true'; + return process.env.FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS?.toLowerCase() === 'true'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/config.ts` around lines 127 - 136, The feature-flag check in isRejectInvalidFieldsEnabled is case-sensitive and will miss values like "TRUE" or "True"; update isRejectInvalidFieldsEnabled to perform a case-insensitive check of process.env.FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS (e.g., normalize with toLowerCase() or a case-insensitive comparison after verifying the env var is defined) so that any variant of "true" enables the flag while preserving current behavior for other values.
🤖 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/v0/destinations/fb_custom_audience/config.ts`:
- Around line 127-136: The feature-flag check in isRejectInvalidFieldsEnabled is
case-sensitive and will miss values like "TRUE" or "True"; update
isRejectInvalidFieldsEnabled to perform a case-insensitive check of
process.env.FB_CUSTOM_AUDIENCE_REJECT_INVALID_FIELDS (e.g., normalize with
toLowerCase() or a case-insensitive comparison after verifying the env var is
defined) so that any variant of "true" enables the flag while preserving current
behavior for other values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01f0ebb9-2ebc-4a9d-997e-375ca9f2a33d
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/v0/destinations/fb_custom_audience/util.ts
- src/v0/destinations/fb_custom_audience/recordTransform.ts
shekhar-rudder
force-pushed
the
int-5922-fb-audience-invalid-events
branch
from
689e084 to
3d692b6
Compare
shekhar-rudder
force-pushed
the
int-5922-fb-audience-invalid-events
branch
from
3d692b6 to
718a4d7
Compare
achettyiitr
previously approved these changes
Mar 10, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/v0/destinations/fb_custom_audience/recordTransform.ts (1)
64-70:⚠️ Potential issue | 🟠 MajorKeep record-field validation independent from hashing.
processRecord()still skipsensureApplicableFormat()wheneverisHashRequired === false, so raw/unhashed record events continue to forward invalid EMAIL/COUNTRY values untouched and never emit the new stats. This guard should key only offdisableFormat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 64 - 70, processRecord() currently only calls ensureApplicableFormat(...) when isHashRequired is true, so validation (and associated stats) is skipped for unhashed records; change the guard to run ensureApplicableFormat whenever disableFormat is false (i.e., if (!disableFormat) { updatedProperty = ensureApplicableFormat(eachProperty, userProperty, workspaceId, destinationId); }) so formatting/validation and stat emission are independent of isHashRequired, leaving hashing logic separate.src/v0/destinations/fb_custom_audience/util.ts (1)
254-260:⚠️ Potential issue | 🟠 MajorRun field validation even when hashing is disabled.
This still keeps
ensureApplicableFormat()behindisHashRequired, so raw/unhashed audience uploads bypass the new EMAIL/COUNTRY validation and metrics. Formatting should depend only ondisableFormat; keep the hashing decision ingetUpdatedDataElement().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 254 - 260, The code currently calls ensureApplicableFormat() only when isHashRequired is true, which skips EMAIL/COUNTRY validation and metrics for unhashed uploads; move the formatting decision to depend solely on disableFormat by invoking ensureApplicableFormat(eachProperty, userProperty, workspaceId, destinationId) whenever disableFormat is false (regardless of isHashRequired) and keep the hashing decision logic inside getUpdatedDataElement(); update the block that sets updatedProperty so that isHashRequired controls only hashing in getUpdatedDataElement(), while ensureApplicableFormat is executed whenever !disableFormat to enforce validation/metrics for raw uploads.
🧹 Nitpick comments (1)
src/v0/destinations/fb_custom_audience/util.ts (1)
279-290: The aggregate null-event metric is now effectively dead code.With the new
throwhere, a non-emptyuserUpdateListeither flipsnullEventtofalseor exits before the counter below runs, sofb_custom_audience_event_having_all_null_field_values_for_all_usersnow only fires on empty input. If that metric still matters, emit it before throwing; otherwise it should be removed to avoid stale telemetry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 279 - 290, The stats.increment call for the metric fb_custom_audience_event_having_all_null_field_values_for_all_users is now unreachable when InstrumentationError is thrown; decide whether you want the metric to fire for cases where every user's fields are null or to treat it as dead telemetry. If you want the metric preserved, move the stats.increment(...) invocation so it executes before the throw inside the userUpdateList handling (ensure it still receives destinationId), otherwise remove the stats.increment(...) line and any references to that metric to avoid sending stale telemetry; update any tests or callers that rely on nullEvent/InstrumentationError behavior accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 64-70: processRecord() currently only calls
ensureApplicableFormat(...) when isHashRequired is true, so validation (and
associated stats) is skipped for unhashed records; change the guard to run
ensureApplicableFormat whenever disableFormat is false (i.e., if
(!disableFormat) { updatedProperty = ensureApplicableFormat(eachProperty,
userProperty, workspaceId, destinationId); }) so formatting/validation and stat
emission are independent of isHashRequired, leaving hashing logic separate.
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 254-260: The code currently calls ensureApplicableFormat() only
when isHashRequired is true, which skips EMAIL/COUNTRY validation and metrics
for unhashed uploads; move the formatting decision to depend solely on
disableFormat by invoking ensureApplicableFormat(eachProperty, userProperty,
workspaceId, destinationId) whenever disableFormat is false (regardless of
isHashRequired) and keep the hashing decision logic inside
getUpdatedDataElement(); update the block that sets updatedProperty so that
isHashRequired controls only hashing in getUpdatedDataElement(), while
ensureApplicableFormat is executed whenever !disableFormat to enforce
validation/metrics for raw uploads.
---
Nitpick comments:
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 279-290: The stats.increment call for the metric
fb_custom_audience_event_having_all_null_field_values_for_all_users is now
unreachable when InstrumentationError is thrown; decide whether you want the
metric to fire for cases where every user's fields are null or to treat it as
dead telemetry. If you want the metric preserved, move the stats.increment(...)
invocation so it executes before the throw inside the userUpdateList handling
(ensure it still receives destinationId), otherwise remove the
stats.increment(...) line and any references to that metric to avoid sending
stale telemetry; update any tests or callers that rely on
nullEvent/InstrumentationError behavior accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82c63e4b-efad-4285-98d2-71c420615059
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/v0/destinations/fb_custom_audience/config.ts
shekhar-rudder
force-pushed
the
int-5922-fb-audience-invalid-events
branch
from
718a4d7 to
539d151
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/v0/destinations/fb_custom_audience/util.ts (2)
278-291:⚠️ Potential issue | 🟡 MinorDead code:
nullEventstat increment is unreachable.The stats increment at lines 287-291 will never execute. Here's why:
nullEventstarts astrue(line 244)- For each user, if any property produces a truthy value,
nullEventis set tofalse(line 274)- If all properties for a user are null/invalid,
nullUserDataremainstrue, and we throw at line 279Since throwing exits the function immediately, the loop can only complete successfully if at least one user had at least one valid property—meaning
nullEventwas set tofalse. Therefore, the conditionif (nullEvent)at line 287 can never betrueafter the loop completes.🔧 Suggested fix: remove unreachable code
data.push(dataElement); }); - if (nullEvent) { - stats.increment('fb_custom_audience_event_having_all_null_field_values_for_all_users', { - destinationId, - }); - } - return data; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 278 - 291, The stats.increment call guarded by nullEvent is unreachable because nullUserData throws before the function can complete when all user fields are null; remove the dead code by deleting the nullEvent variable usage and the block calling stats.increment('fb_custom_audience_event_having_all_null_field_values_for_all_users', { destinationId }) (and remove nullEvent if no longer used elsewhere) so only reachable, meaningful stats remain in util.ts.
1-1:⚠️ Potential issue | 🟡 MinorAddress Prettier formatting failure.
The CI pipeline indicates that this file fails the Prettier formatting check. Run
prettier --write src/v0/destinations/fb_custom_audience/util.tsto resolve the formatting issues before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` at line 1, Prettier is failing on src/v0/destinations/fb_custom_audience/util.ts; run the formatter and commit the changes by executing `prettier --write src/v0/destinations/fb_custom_audience/util.ts` (or run your project's lint/format script) and then add/commit the resulting file so the import line ("import lodash from 'lodash';") and the rest of util.ts conform to the project's Prettier rules.
🧹 Nitpick comments (2)
src/v0/destinations/fb_custom_audience/recordTransform.ts (2)
64-80: Inconsistent parameter usage: function params vs direct extraction.There's an inconsistency in how
workspaceIdanddestinationIdare sourced:
- Lines 68-69:
ensureApplicableFormatuses function parametersworkspaceId,destinationId- Lines 78-79:
getUpdatedDataElementextracts directly fromrecord.metadata.workspaceId,record.destination.IDWhile these values should be identical, the inconsistent access pattern reduces clarity and could cause subtle bugs if assumptions change. Consider using the function parameters consistently.
♻️ Suggested refactor for consistency
dataElement = getUpdatedDataElement( dataElement, isHashRequired, eachProperty, updatedProperty, - record.metadata.workspaceId, - record.destination.ID, + workspaceId, + destinationId, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 64 - 80, The call site mixes sources for workspaceId/destinationId: ensureApplicableFormat uses the function parameters workspaceId and destinationId while getUpdatedDataElement pulls record.metadata.workspaceId and record.destination.ID; make them consistent by passing the same function parameters (workspaceId, destinationId) into getUpdatedDataElement instead of reading from record, so both calls use the same canonical values and avoid subtle divergence between ensureApplicableFormat and getUpdatedDataElement.
133-134: Optional chaining suggestsworkspaceIdmay be undefined.The use of
recordChunksArray[0]?.[0]?.metadata?.workspaceIdindicates awareness that this value could beundefined. ThisworkspaceIdis then passed tobatchingWithPayloadSize. If the function doesn't handle undefined gracefully, this could cause issues with the config retrieval atconfig.getMaxPayloadSize(workspaceId).💡 Consider adding a fallback
- const workspaceId = recordChunksArray[0]?.[0]?.metadata?.workspaceId; + const workspaceId = recordChunksArray[0]?.[0]?.metadata?.workspaceId ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 133 - 134, recordChunksArray[0]?.[0]?.metadata?.workspaceId may be undefined but we pass it straight into batchingWithPayloadSize which likely calls config.getMaxPayloadSize(workspaceId); ensure batchingWithPayloadSize receives a safe workspaceId by providing a fallback or validating before call: derive workspaceId from recordChunksArray (symbol: recordChunksArray) into a local const workspaceId, if undefined replace with a sensible default (e.g., an explicit default workspace id or undefined-safe placeholder) or early-return/error, then call batchingWithPayloadSize(prepareFinalPayload, workspaceId); alternatively update batchingWithPayloadSize to handle undefined workspaceId and call config.getMaxPayloadSize only when workspaceId is present (symbols: batchingWithPayloadSize, config.getMaxPayloadSize).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 278-291: The stats.increment call guarded by nullEvent is
unreachable because nullUserData throws before the function can complete when
all user fields are null; remove the dead code by deleting the nullEvent
variable usage and the block calling
stats.increment('fb_custom_audience_event_having_all_null_field_values_for_all_users',
{ destinationId }) (and remove nullEvent if no longer used elsewhere) so only
reachable, meaningful stats remain in util.ts.
- Line 1: Prettier is failing on src/v0/destinations/fb_custom_audience/util.ts;
run the formatter and commit the changes by executing `prettier --write
src/v0/destinations/fb_custom_audience/util.ts` (or run your project's
lint/format script) and then add/commit the resulting file so the import line
("import lodash from 'lodash';") and the rest of util.ts conform to the
project's Prettier rules.
---
Nitpick comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 64-80: The call site mixes sources for workspaceId/destinationId:
ensureApplicableFormat uses the function parameters workspaceId and
destinationId while getUpdatedDataElement pulls record.metadata.workspaceId and
record.destination.ID; make them consistent by passing the same function
parameters (workspaceId, destinationId) into getUpdatedDataElement instead of
reading from record, so both calls use the same canonical values and avoid
subtle divergence between ensureApplicableFormat and getUpdatedDataElement.
- Around line 133-134: recordChunksArray[0]?.[0]?.metadata?.workspaceId may be
undefined but we pass it straight into batchingWithPayloadSize which likely
calls config.getMaxPayloadSize(workspaceId); ensure batchingWithPayloadSize
receives a safe workspaceId by providing a fallback or validating before call:
derive workspaceId from recordChunksArray (symbol: recordChunksArray) into a
local const workspaceId, if undefined replace with a sensible default (e.g., an
explicit default workspace id or undefined-safe placeholder) or
early-return/error, then call batchingWithPayloadSize(prepareFinalPayload,
workspaceId); alternatively update batchingWithPayloadSize to handle undefined
workspaceId and call config.getMaxPayloadSize only when workspaceId is present
(symbols: batchingWithPayloadSize, config.getMaxPayloadSize).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2919ea9f-7ba0-40ff-9eea-af8966b62237
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/v0/destinations/fb_custom_audience/config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/v0/destinations/fb_custom_audience/util.ts (1)
260-266:⚠️ Potential issue | 🟠 MajorRun field validation even when hashing is disabled.
This still skips
ensureApplicableFormat()for raw/unhashed uploads, so invalidCOUNTRYvalues pass through untouched and the new stats never fire on that path. The format-validation gate should be!disableFormat, independent ofisHashRequired.Suggested change
- if (isHashRequired && !disableFormat) { + if (!disableFormat) { updatedProperty = ensureApplicableFormat( eachProperty, userProperty, workspaceId, destinationId, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/util.ts` around lines 260 - 266, The current guard skips calling ensureApplicableFormat when hashing is disabled; change the condition so format validation runs whenever disableFormat is false regardless of isHashRequired. Update the branch around isHashRequired/disableFormat to call ensureApplicableFormat(eachProperty, userProperty, workspaceId, destinationId) whenever !disableFormat (and keep any hashing logic separate), so updatedProperty is validated for raw/unhashed uploads as well.src/v0/destinations/fb_custom_audience/recordTransform.ts (1)
64-70:⚠️ Potential issue | 🟠 MajorApply the new EMAIL/COUNTRY validation on raw record uploads too.
processRecord()still tiesensureApplicableFormat()toisHashRequired, so the raw/unhashed record path never validates those fields and never emits the new invalid-field stats. This should use the same!disableFormatgate regardless of hashing.Suggested change
- if (isHashRequired && !disableFormat) { + if (!disableFormat) { updatedProperty = ensureApplicableFormat( eachProperty, userProperty, workspaceId, destinationId, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/v0/destinations/fb_custom_audience/recordTransform.ts` around lines 64 - 70, processRecord() currently only calls ensureApplicableFormat(...) when isHashRequired is true, so raw/unhashed uploads skip EMAIL/COUNTRY validation and never emit invalid-field stats; change the conditional so ensureApplicableFormat is invoked whenever !disableFormat is true (independent of isHashRequired) — locate the processRecord function and the block assigning updatedProperty (where isHashRequired && !disableFormat is checked) and split or replace that condition to call ensureApplicableFormat(updatedProperty assignment) whenever !disableFormat, leaving any hashing logic (isHashRequired) that follows intact.
🤖 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/v0/destinations/fb_custom_audience/util.ts`:
- Around line 285-287: prepareDataField currently throws an InstrumentationError
inside the per-user loop when a single user has all null/invalid fields, which
aborts the entire batch; change the logic in prepareDataField to skip/ignore
individual invalid user rows (log or collect their indices if needed) rather
than throwing immediately, continue processing the rest of the batch, and only
throw an InstrumentationError after the loop if the final output array is empty
(i.e., all rows were invalid) — reference prepareDataField, InstrumentationError
and userSchema to locate and implement this behavior.
---
Duplicate comments:
In `@src/v0/destinations/fb_custom_audience/recordTransform.ts`:
- Around line 64-70: processRecord() currently only calls
ensureApplicableFormat(...) when isHashRequired is true, so raw/unhashed uploads
skip EMAIL/COUNTRY validation and never emit invalid-field stats; change the
conditional so ensureApplicableFormat is invoked whenever !disableFormat is true
(independent of isHashRequired) — locate the processRecord function and the
block assigning updatedProperty (where isHashRequired && !disableFormat is
checked) and split or replace that condition to call
ensureApplicableFormat(updatedProperty assignment) whenever !disableFormat,
leaving any hashing logic (isHashRequired) that follows intact.
In `@src/v0/destinations/fb_custom_audience/util.ts`:
- Around line 260-266: The current guard skips calling ensureApplicableFormat
when hashing is disabled; change the condition so format validation runs
whenever disableFormat is false regardless of isHashRequired. Update the branch
around isHashRequired/disableFormat to call ensureApplicableFormat(eachProperty,
userProperty, workspaceId, destinationId) whenever !disableFormat (and keep any
hashing logic separate), so updatedProperty is validated for raw/unhashed
uploads as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a41dd31d-f374-42b1-8df8-32169e8d9d94
⛔ Files ignored due to path filters (2)
test/integrations/destinations/fb_custom_audience/processor/data.tsis excluded by!**/test/**test/integrations/destinations/fb_custom_audience/router/data.tsis excluded by!**/test/**
📒 Files selected for processing (3)
src/v0/destinations/fb_custom_audience/config.tssrc/v0/destinations/fb_custom_audience/recordTransform.tssrc/v0/destinations/fb_custom_audience/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/v0/destinations/fb_custom_audience/config.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5023 +/- ##
===========================================
- Coverage 92.27% 92.27% -0.01%
===========================================
Files 657 657
Lines 35927 35941 +14
Branches 8455 8479 +24
===========================================
+ Hits 33153 33163 +10
+ Misses 2557 2540 -17
- Partials 217 238 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
achettyiitr
approved these changes
Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validator.isEmail; emitfb_custom_audience_invalid_emailstats metric for invalid emails instead of silently passing them throughfb_custom_audience_invalid_country_codestats metric for invalid codesInstrumentationErrorwhen all user properties for a record are null/invalid, replacing the previous silent stats counter (fb_custom_audience_event_having_all_null_field_values_for_a_user)workspaceIdanddestinationIdthrough the call chain to enable proper metric attributionTest plan
not-an-email) produce thefb_custom_audience_invalid_emailstat and send an empty string in the payloadfb_custom_audience_invalid_country_codestatInstrumentationErrorwith a descriptive messagefb_custom_audience🤖 Generated with Claude Code