Facebook custom audiences - Refactor#3598
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (72.24%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3598 +/- ##
==========================================
- Coverage 80.35% 80.35% -0.01%
==========================================
Files 1309 1314 +5
Lines 23946 24007 +61
Branches 4829 4868 +39
==========================================
+ Hits 19243 19291 +48
+ Misses 3807 3801 -6
- Partials 896 915 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
varadarajan-tw
left a comment
varadarajan-tw
left a comment
There was a problem hiding this comment.
Gave a first pass and made few suggestions. I'd like to review this again after stage testing.
| label: 'Sync Mode', | ||
| description: 'The sync mode to use when syncing data to Facebook.', | ||
| default: 'upsert', | ||
| description: 'Define how the records will be synced to Facebook Custom Audiences. When syncing Engage Audiences this should be set to "Mirror". When syncing from Reverse ETL, choose Upsert or Delete.', |
There was a problem hiding this comment.
"Mirror" could work with RETL too, right?
| const addAccountId = (engageAdAccountId ?? retlAdAccountId) as string | ||
| const { data: { externalId: id } = {}, error } = await createAudience(request, audienceName, addAccountId, audienceDescription) | ||
| if (error) { | ||
| throw new IntegrationError(error.message || 'Failed to create audience', error.code || 'CREATE_AUDIENCE_FAILED', 400) |
There was a problem hiding this comment.
Could we add this CREATE_AUDIENCE_FIELD to error codes?
| const { externalId } = getAudienceInput | ||
| const { data: { externalId: id } = {}, error } = await getAudience(request, externalId) | ||
| if (error) { | ||
| throw new IntegrationError(error.message || 'Failed to get audience', error.code || 'GET_AUDIENCE_FAILED', 400) |
There was a problem hiding this comment.
Could we add GET_AUDIENCE_FAILED to error codes?
|
|
||
| try { | ||
| const response = await request<GetAudienceResponse>(url, { method: 'GET' }) | ||
| const r = await response.json() |
There was a problem hiding this comment.
nit: response.data already would have parsed value. I would suggest changing it to response.data
| json | ||
| }) | ||
|
|
||
| const r = await response.json() |
There was a problem hiding this comment.
nit: response.data already would have parsed value. I would suggest changing it to response.data
| export const API_VERSION = 'v24.0' | ||
| export const CANARY_API_VERSION = 'v24.0' |
There was a problem hiding this comment.
We should retain this setup to switch between CANARY to STABLE API Version. Otherwise, the engineer has to reintroduce code and write test cases every 6 months when facebook deprecates its APIs.
| requests.push(sendRequest(request, audienceId, addMap, msResponse, 'POST', isBatch)) | ||
| } | ||
|
|
||
| if (deleteMap.size > 0) { | ||
| requests.push(sendRequest(request, audienceId, deleteMap, msResponse, 'DELETE', isBatch)) | ||
| } |
There was a problem hiding this comment.
I don't have any specific opinion but wondering if we should use batch keys and ensure all Deletes and Adds are batched together? This way we don't have to retry two API calls
There was a problem hiding this comment.
I don't think we can do this as the name of the audience is variable - so what would the batck key value be?
| mobileAdId | ||
| } = payload | ||
|
|
||
| const row: FacebookDataRow = [ |
There was a problem hiding this comment.
Claude review pointed out the properties are out of order
There was a problem hiding this comment.
- SCHEMA_PROPERTIES Order Changed — Data Corruption Risk
sync/constants.ts:23-38
The order of SCHEMA_PROPERTIES has been changed compared to the original in fbca-properties.ts. Facebook's API expects data columns in the exact order specified by the schema array. The reordering moves:
- GEN (gender) from index 3 → index 9
- DOBY/DOBM/DOBD shift up to fill the gap
- COUNTRY and MADID swap positions
This means gender data will be sent as year-of-birth, and vice versa, causing silent data corruption for all existing customers. This is the highest priority fix needed.
There was a problem hiding this comment.
Claude got it wrong. I reorded the values to be in line with facebook's doc (so they would be easier to audit in future.
The order is controlled using this:
export const SCHEMA_PROPERTIES = [
'EXTERN_ID',
'EMAIL',
'PHONE',
'DOBY',
'DOBM',
'DOBD',
'LN',
'FN',
'FI',
'GEN',
'CT',
'ST',
'ZIP',
'COUNTRY',
'MADID'
] as const
I just changed the order accordingly.
I'm OK to set this back to the original ordering if it's safer...
|
Sharing comments from deep review using Claude. It pointed out 1 critical issue. PR #3598 Review: Facebook Custom Audiences — RefactorAuthor: Joe Ayoub | Branch: Critical1.
|
| Field | Old Index | New Index |
|---|---|---|
GEN |
3 | 9 |
DOBY |
4 | 3 |
DOBM |
5 | 4 |
DOBD |
6 | 5 |
LN |
7 | 6 |
FN |
8 | 7 |
FI |
9 | 8 |
COUNTRY |
14 | 13 |
MADID |
13 | 14 |
Gender data will be sent as year-of-birth and vice versa, causing silent data corruption for all existing customers. This is a showstopper.
Fix: Revert SCHEMA_PROPERTIES to the original order.
2. Feature Flag & Canary API Version Removed
constants.ts
The original code had FACEBOOK_CUSTOM_AUDIENCE_FLAGON, CANARY_API_VERSION, and a getApiVersion() function that selected the API version based on feature flags and tracked stats via statsClient.incr(). All of this is removed — the destination now hardcodes v24.0.
Per CLAUDE.md: "For high-volume destinations, use feature flags for safe rollout."
Fix: Preserve feature flag support or confirm no customers are on the canary path before removing.
3. error_user_message Fallback Lost
functions.ts:11-30
The original error parser checked for both error_user_msg and error_user_message from Facebook's API response. The new parseFacebookError() only checks error_user_msg, losing the fallback.
- userMsg = parsed.error.error_user_msg || parsed.error.error_user_message
+ error_user_msg // no error_user_message fallbackFix: Add error_user_message as a fallback in parseFacebookError().
High
4. EmptyValueError Handling Removed
sync/functions.ts:207-219
The original fbca-operations.ts wrapped processHashing() calls in try-catch to convert EmptyValueError into a user-friendly PayloadValidationError:
// Original
throw new PayloadValidationError(
`Invalid value for ${key}. After normalization, the value is empty.`
)The new getData() calls processHashing() without this handling. Errors will bubble up with unhelpful messages.
Fix: Wrap processHashing() calls in try-catch or create a shared helper that converts EmptyValueError to PayloadValidationError.
5. Missing Integration Test Files
The PR deletes 809 lines of tests from sync/__tests__/index.test.ts, __tests__/index.test.ts, and __tests__/fb-operations.test.ts. The following integration test scenarios from deleted files have no replacement:
| Deleted Test Scenario | Covered in New Tests? |
|---|---|
| Mirror mode (add + delete split based on audience membership) | No |
| Upsert end-to-end flow | No |
| RETL single user sync with canary API version | No |
Flagged audience error subcode 1713231 handling |
No |
| Pre-hashed email preservation (no double-hashing) | No |
Engage audience sync with external_id |
No |
| Facebook API error for flagged Engage audiences | No |
6. No Tests for retlOnMappingSave Hook
sync/hook-functions.ts
The performHook() function orchestrates audience creation/lookup and is the customer-facing entry point for mapping configuration. It has zero test coverage. Missing scenarios:
operation: 'create'with validaudienceNameoperation: 'existing'with validexistingAudienceId- Invalid operation handling
- Error propagation from
createAudience()/getAudience()
7. No Tests for sendRequest()
sync/functions.ts
This function handles all Facebook API communication — error parsing, batch vs non-batch mode, multi-status responses. No unit tests exist. Missing scenarios:
- Success response handling
- Facebook error response parsing
- Batch mode error accumulation vs non-batch throwing
- Partial batch failures
Medium
8. Duplicated Ad Account ID Formatting
functions.ts:43-45, hook-functions.ts:89-91
The adAccountId.startsWith('act_') ? adAccountId.slice(4) : adAccountId logic is duplicated. The original code had a single formatAdAccount() method.
Fix: Extract to a shared utility.
9. No Batch Size Edge Case Tests
sync/__tests__/delete.test.ts
Only tests 3 users per batch. Missing edge cases:
- Maximum batch size (10,000)
- Empty batch
- Single item batch
- Batching disabled
10. API Version Jump Undocumented
constants.ts
The API version jumps from v19.0/v21.0 to v24.0 with no migration notes in the PR description. This is a significant version bump that could introduce breaking changes from Facebook's side.
11. Instagram Account IDs — New Field Needs Verification
sync/fields.ts:123-127
A new igAccountIds field is added. While it appears optional/additive, verify that:
- Existing mappings without this field won't break
- Facebook's API accepts
ig_account_idsalongside existing fields - Generated types include this as optional
Nit
12. Use Labeled Tuple Elements for FacebookDataRow
sync/types.ts
Since this project uses TypeScript 5.5, consider labeled tuple elements for self-documenting positional types:
export type FacebookDataRow = [
externId: string,
email: string,
phone: string,
year: string,
month: string,
day: string,
lastName: string,
firstName: string,
firstInitial: string,
gender: string,
city: string,
state: string,
zip: string,
country: string,
mobileAdId: string,
]Labels don't affect runtime behavior but make it easier to catch ordering mistakes during code review and improve IDE hover tooltips.
Positive Aspects
- Code organization is significantly improved — functions, types, constants, and fields cleanly separated into focused modules
- 62 unit tests for normalization functions in
functions.test.tsare thorough - Audience creation tests are well-structured with good error case coverage
- Net test line count is slightly positive (+15 lines)
- The decomposition of the monolithic
sync/index.tsis a good architectural decision
2 participants
Refactoring Facebook Custom Audiences to tidy up code.
The refactor is limited because there are customers using this integration (which is in Public Beta).
This PR is designed to be non-breaking.
Testing
Unit tests updated.
Tested in Staging. Needs further staging testing.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example