Skip to content

[Facebook Pixel] - Private Beta bug fixes - #3920

Open
joe-ayoub-segment wants to merge 14 commits into
mainfrom
fix/fb-pixel-web-content-ids-default
Open

[Facebook Pixel] - Private Beta bug fixes#3920
joe-ayoub-segment wants to merge 14 commits into
mainfrom
fix/fb-pixel-web-content-ids-default

Conversation

@joe-ayoub-segment

@joe-ayoub-segment joe-ayoub-segment commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Destination not in use by any customers yet.

Bug fixes and hardening for the Facebook Conversions API Web (device-mode Facebook Pixel) destination, found
during private-beta QA.

Fixes

  • Removed the broken content_ids default (disabled Liquid map filter → always dropped).
  • Coerce single-product content_ids/contents scalars to arrays so they aren't dropped for single-item events.
  • Emit search_string (Search) and status (CompleteRegistration) — both were defined/mapped but never sent.
  • Added LDU support for Maryland (1012) and Rhode Island (1013).
  • Split event_config into top-level event_name / custom_event_name / show_fields so depends_on conditions
    resolve in the app mapping editor (dotted fieldKeys never matched). Breaking mapping change — existing mappings
    need re-saving.
  • Field normalization: trim string fields + contents items, uppercase/validate currency (ISO 4217), normalize
    gender (m/male/f/female → m/f), minimum: 0 on numeric fields.
  • Aligned field↔event mappings with Meta docs (num_items on Purchase; content_category/content_name on all
    events; predicted_ltv per the pLTV guide).
  • Hardened validate(): reject whitespace-only content_ids; require custom_event_name when event is CustomEvent.

Testing

  • Unit tests — full destination suite passing (129 tests); regression guards for each fix (no content_ids
    default, scalar→array coercion, search_string/status emitted, depends_on conditions, normalization,
    validation).
  • Local end-to-end via Actions Tester (./bin/run serve --browser) — ran real events against local code and
    inspected the resulting fbq / facebook.com/tr requests.
  • All 9 presets + Custom Event fired and verified on the wire.
  • Standard events without presets (Lead, Contact, Subscribe, StartTrial, Donate) verified.
  • User data / Advanced Matching — all 11 identifiers; normalization checked (California→ca, United States→us,
    birthday→YYYYMMDD, phone digits-only); hashing verified byte-for-byte with Parameter Builder off.
  • All 7 settings — LDU, Disable Auto Config, Disable First Party Cookies, Disable Push State, Format User Data
    with Parameter Builder, Agent, Pixel ID (both on/off states where applicable).
  • _fbc from fbclid, eventID dedup, and multi-product contents confirmed on the wire.

The content_ids field defaulted to a Liquid template
`{{ properties.products | map: 'product_id' }}`. This never worked:
the `map` filter is on the mapping-kit disabledFilters list (it throws
`filter "map" is disabled`), and @liquid can only return a string, never
an array — so a multiple:true field always dropped it via the
Array.isArray check in formatFBEvent. Presets relying on the default
(Purchase, AddPaymentInfo, InitiateCheckout) sent no content_ids.

An @arrayPath alternative was rejected: the app mapping editor has no
@arrayPath render branch for scalar multiple:true fields, so it would
display blank in the UI.

Removing the default entirely — customers configure content_ids if they
need it, matching the server-side facebook-conversions-api destination.
Added a test covering the field definition (no default) and formatFBEvent
handling.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 4, 2026 13:39
@joe-ayoub-segment
joe-ayoub-segment requested review from a team as code owners August 4, 2026 13:39
@joe-ayoub-segment joe-ayoub-segment self-assigned this Aug 4, 2026
@joe-ayoub-segment joe-ayoub-segment changed the title Fix broken content_ids default in Facebook Conversions API Web [Facebook Pixel] - Private Beta bug fixes Aug 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Removes the non-functional default mapping for content_ids in the Facebook Conversions API Web destination to prevent silently dropping content_ids on multi-product events and aligns behavior with server-side destination defaults.

Changes:

  • Removed the broken @liquid default mapping from the content_ids field definition.
  • Updated destination metadata.json to remove content_ids from preset mappings that previously relied on the broken default.
  • Added unit tests to guard against reintroducing a content_ids default and to verify formatFBEvent omission/passthrough behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/fields.ts Removes content_ids default mapping (and reformats) so the field no longer evaluates to an unusable value for multiple: true.
packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/tests/content-ids.test.ts Adds regression tests for “no default” and formatFBEvent behavior with provided/empty/absent content_ids.
packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json Removes the broken default/preset mappings for content_ids and sets the field default to null in metadata.

content_ids and contents are multiple:true fields, but the browser
device-mode runtime does not arrify single values the way the server
runtime does. A single-product event (properties.product_id) maps these
to a scalar/object, which formatFBEvent's Array.isArray gate silently
dropped — so ViewContent/AddToCart could send no product identifiers.

formatFBEvent now coerces both fields with a toArray helper before the
empty-check. Added send-path tests covering a single content_ids string
and a single contents object.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 15:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:56

  • validate() destructures event_config.event_name without a default, which will throw at runtime if payload.event_config is ever missing/undefined (unlike send() / formatFBEvent() which defensively default event_config to {}). Make validate() consistent by defaulting event_config in the destructure (or safely reading payload.event_config?.event_name) so validation fails gracefully instead of crashing.
  const {
    event_config: { event_name },
    content_ids,
    contents
  } = payload

packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json:398

  • The PR description says the broken content_ids default was removed, but metadata.json still includes a default key (set to null). If null is semantically equivalent to “no default” in this metadata schema then this is fine, but if consumers interpret presence of the key as an explicit default it could be confusing or behavior-changing. Consider omitting the default property entirely (if supported), or add a brief note in the PR description explaining that null is the canonical “no default” representation in generated metadata.
          "default": null,

The search_string field was defined and mapped in the Search preset but
formatFBEvent never read it and it was absent from the FBEvent type, so
it was silently dropped — Search events reached Facebook with no query
string. Added search_string to the FBEvent type and emit it in
formatFBEvent (top-level event property, per Meta Pixel reference).
Added tests covering formatFBEvent and the send path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 16:30
The status field was defined and mapped but formatFBEvent never read it
and it was absent from the FBEvent type, so it was silently dropped.
Added status to the FBEvent type and emit it in formatFBEvent using a
typeof boolean check so status: false (registration not completed) is
preserved. Added tests covering true, the false edge case, and the send
path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:73

  • toArray will treat an empty string ('') as a present value and convert it to [''], which will then be sent as content_ids: [''] (previously an empty string would be omitted because it was falsy). Consider special-casing strings to trim() and return [] when the trimmed value is empty, so content_ids doesn’t get populated with invalid IDs.
function toArray<T>(value: T | T[] | undefined | null): T[] {
  if (value === undefined || value === null) return []
  return Array.isArray(value) ? value : [value]
}

packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json:398

  • Setting "default": null is a behavioral change from “no default” to “explicitly default to null” and may be interpreted differently by tooling (e.g., mapping editor / codegen) than an absent default key. If the intent is “no default,” prefer removing the default property entirely (or using the metadata convention for “unset”) to avoid consumers treating null as a meaningful configured default.
          "default": null,

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:29

  • This ternary is redundant and makes the intent slightly harder to scan. Use the boolean expression directly (e.g., assign event_name === 'CustomEvent') to reduce noise.
  const isCustom = event_name === 'CustomEvent' ? true : false

Copilot AI review requested due to automatic review settings August 4, 2026 16:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:56

  • validate() destructures event_config without a default, so it will throw if payload.event_config is missing/undefined. Since send() already defensively destructures event_config with a default, validate() should do the same (e.g., default event_config to {} in the destructuring, or read payload.event_config?.event_name) to avoid an unhandled runtime exception.
  const {
    event_config: { event_name },
    content_ids,
    contents
  } = payload

packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json:398

  • The field declares "allowNull": false but now has "default": null. If the metadata schema/runtime treats default as an actual value (rather than “no default”), this is internally inconsistent and can cause validation/UI/runtime issues. Prefer removing the default key entirely for “no default” (or, if the schema requires default, ensure it uses the repository’s established sentinel for “unset” rather than null, or make the nullability contract consistent).
          "multiple": true,
          "allowNull": false,
          "dynamic": false,
          "default": null,

packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json:2563

  • The PR description says “Removed the default; customers configure it if needed,” but metadata.json adds (and similarly elsewhere) preset/default mappings for content_ids via @path (e.g., $.properties.product_id). If the intention is truly “no default mapping,” these preset mappings should be removed; if the intention is “remove the broken Liquid default but keep working preset defaults,” the PR description should be updated to reflect that behavioral change.
        },
        "content_ids": {
          "@path": "$.properties.product_id"
        }

Facebook's Limited Data Use supports state codes 1012 (Maryland,
effective 2025-09-09) and 1013 (Rhode Island, effective 2025-11-17), but
the destination's LDU options stopped at 1011 (Minnesota). Customers in
those states could not enable LDU. Added both to the LDU map and the
ldu setting choices.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 18:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Suppressed comments (4)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:73

  • toArray will wrap and preserve empty scalar values (ex: content_ids: '' becomes ['']), and formatFBEvent will then send content_ids even though it's effectively empty. This can result in invalid payloads being sent (especially when contents is present so validation doesn’t block). Recommendation (mandatory): add content-id specific normalization before spreading (trim strings and filter out empty/whitespace-only entries; if content_ids can include non-strings defensively filter to strings). Consider a dedicated helper (e.g., toNonEmptyStringArray) instead of the generic toArray for content_ids.
function toArray<T>(value: T | T[] | undefined | null): T[] {
  if (value === undefined || value === null) return []
  return Array.isArray(value) ? value : [value]
}

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:94

  • toArray will wrap and preserve empty scalar values (ex: content_ids: '' becomes ['']), and formatFBEvent will then send content_ids even though it's effectively empty. This can result in invalid payloads being sent (especially when contents is present so validation doesn’t block). Recommendation (mandatory): add content-id specific normalization before spreading (trim strings and filter out empty/whitespace-only entries; if content_ids can include non-strings defensively filter to strings). Consider a dedicated helper (e.g., toNonEmptyStringArray) instead of the generic toArray for content_ids.
  const contentIdsArr = toArray(content_ids)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:100

  • toArray will wrap and preserve empty scalar values (ex: content_ids: '' becomes ['']), and formatFBEvent will then send content_ids even though it's effectively empty. This can result in invalid payloads being sent (especially when contents is present so validation doesn’t block). Recommendation (mandatory): add content-id specific normalization before spreading (trim strings and filter out empty/whitespace-only entries; if content_ids can include non-strings defensively filter to strings). Consider a dedicated helper (e.g., toNonEmptyStringArray) instead of the generic toArray for content_ids.
    ...(contentIdsArr.length > 0 ? { content_ids: contentIdsArr } : {}),

packages/browser-destinations/destinations/facebook-conversions-api-web/metadata.json:406

  • The PR description says the broken content_ids default was removed. In metadata.json, default is still present but set to null. If any consumers interpret the presence of a default key as meaningful (even when null), this may still surface as a default in tooling or generate a null mapping. Recommendation: remove the default property entirely for content_ids in metadata.json (or confirm/document that null is the canonical representation of “no default” in this metadata format).
          "multiple": true,
          "allowNull": false,
          "dynamic": false,
          "default": null,

- Trim string fields (content_category/name/type, delivery_category,
  search_string) and each content_ids entry, dropping whitespace-only
  values so they aren't sent.
- Normalize currency: trim + uppercase + validate against
  CURRENCY_ISO_CODES; drop invalid codes.
- Add minimum: 0 to num_items, value, predicted_ltv, net_revenue.
- Add a test exercising the normalizers over a full payload.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 5, 2026 09:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:97

  • When currency is invalid, it is silently dropped. For events where currency is effectively required (e.g., Purchase), this can lead to hard-to-debug missing fields and potential downstream rejection/unsupported payloads. Consider warning when a non-empty currency fails validation (include the original value and event name), or alternatively keep the original value and let Facebook handle validation errors.
function normalizeCurrency(value: unknown): string | undefined {
  const t = trimmed(value)
  if (!t) return undefined
  const upper = t.toUpperCase()
  return CURRENCY_ISO_CODES.has(upper) ? upper : undefined
}

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/fields.ts:222

  • Adding minimum: 0 is a behavioral change that can reject previously valid configurations/payloads (e.g., negative values for refunds/credits or adjustment events if customers use them). The PR description states this is non-breaking; please either (a) justify and document why negative values must be disallowed per Meta spec, (b) remove the minimum constraint, or (c) update the PR description to reflect the compatibility impact.
export const value: InputField = {
  label: 'Value',
  description:
    'A numeric value associated with this event. This could be a monetary value or a value in some other metric.',
  type: 'number',
  minimum: 0,
  default: { '@path': '$.properties.value' },
  depends_on: getDependenciesFor('value'),

The event_config object field held event_name/custom_event_name/show_fields
as sub-properties, and depends_on referenced them via dotted fieldKeys
(event_config.show_fields). The app mapping editor only resolves flat
top-level fieldKeys, so those conditions never matched and dependent
fields (predicted_ltv, net_revenue, etc.) never displayed. The
show_fields condition also compared a boolean field against the string
'true'.

Changes:
- Split event_config into top-level event_name, custom_event_name,
  show_fields fields.
- depends_on now uses flat fieldKeys and a boolean `true` for show_fields.
- Fixed the getDependenciesFor guard (>1 -> >=1) so single-event fields
  (num_items, search_string, status, net_revenue, custom_event_name)
  gate on their event, not only show_fields.
- custom_event_name is required when event_name is CustomEvent.
- Rewired functions.ts, all 9 presets, generated-types, and tests;
  expanded depends-on tests.

Note: this changes the mapping shape (event_config removed) — existing
mappings referencing event_config will need re-saving.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 5, 2026 11:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Suppressed comments (4)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:68

  • The validation logic can incorrectly pass when content_ids is a scalar string containing only whitespace (or an array of whitespace-only strings). Since formatFBEvent trims and drops whitespace-only entries, you can end up sending an event missing both content_ids and contents without warning. Consider validating against normalized values (e.g., trim/arrify in validate, or validate the already-formatted fbEvent result) so the warning behavior matches what is actually sent to fbq.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`
    }
  }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:36

  • When event_name is CustomEvent, custom_event_name is cast to string without being validated. If a payload is misconfigured (or comes from an older mapping shape), this can call trackSingleCustom with undefined, which is likely to produce an invalid event. Add a validation branch for CustomEvent to require a non-empty custom_event_name (after trimming) before calling fbq.
  const { custom_event_name, event_name } = payload

  const isCustom = event_name === 'CustomEvent' ? true : false

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:55

  • When event_name is CustomEvent, custom_event_name is cast to string without being validated. If a payload is misconfigured (or comes from an older mapping shape), this can call trackSingleCustom with undefined, which is likely to produce an invalid event. Add a validation branch for CustomEvent to require a non-empty custom_event_name (after trimming) before calling fbq.
  if (isCustom) {
    client('trackSingleCustom', pixelId, custom_event_name as string, { ...fbEvent }, options)
  } else {
    client('trackSingle', pixelId, event_name as FBStandardEventType, { ...fbEvent }, options)
  }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/index.ts:98

  • Loading runtime code from unpkg.com without a pinned version (and without integrity verification) introduces supply-chain and operational risk: the contents at that URL can change over time, potentially breaking the destination or introducing malicious code. Prefer pinning an exact package version in the URL and, if supported by the loader, enforcing Subresource Integrity (SRI) or hosting a vetted copy from a controlled domain.
    if (formatUserDataWithParamBuilder) {
      const script = `https://unpkg.com/meta-capi-param-builder-clientjs/dist/clientParamBuilder.bundle.js`
      await deps.loadScript(script)

Copilot AI review requested due to automatic review settings August 5, 2026 11:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:68

  • Validation currently runs against the raw content_ids value, but formatFBEvent later trims and drops whitespace-only IDs. This can allow content_ids: ' ' (or an array of whitespace strings) to pass validation but still be omitted when sending, resulting in an event missing required fields. Consider validating against normalized values (e.g., trimmedArray(toArray(content_ids)) and toArray(contents)) so the validation reflects what will actually be sent.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`
    }
  }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:93

  • When a currency is provided but not recognized, this silently drops the currency field. For events like Purchase where currency is expected/required, silently omitting it can make debugging difficult and can degrade downstream attribution. Consider logging a warning when an invalid currency is provided (or alternatively passing through the uppercased value and relying on Meta to validate) so misconfigurations are visible.
function normalizeCurrency(value: unknown): string | undefined {
  const t = trimmed(value)
  if (!t) return undefined
  const upper = t.toUpperCase()
  return CURRENCY_ISO_CODES.has(upper) ? upper : undefined
}

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/depends-on.ts:31

  • CustomEvent is excluded from the dependency lists for fields like currency and value. Because formatFBEvent deletes fields that are not visible when show_fields === false, this effectively prevents users from sending common parameters (e.g., value/currency) on custom events unless they also toggle show_fields on. Since Meta custom events support these parameters, consider adding CustomEvent to the relevant dependency lists (and/or adjusting the deletion logic so explicitly mapped fields are not stripped for custom events).
  currency: [
    'AddPaymentInfo',
    'AddToCart',
    'AddToWishlist',
    'CompleteRegistration',
    'InitiateCheckout',
    'Lead',
    'Purchase',
    'Search',
    'StartTrial',
    'Subscribe',
    'ViewContent'
  ],

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/depends-on.ts:50

  • CustomEvent is excluded from the dependency lists for fields like currency and value. Because formatFBEvent deletes fields that are not visible when show_fields === false, this effectively prevents users from sending common parameters (e.g., value/currency) on custom events unless they also toggle show_fields on. Since Meta custom events support these parameters, consider adding CustomEvent to the relevant dependency lists (and/or adjusting the deletion logic so explicitly mapped fields are not stripped for custom events).
  value: [
    'AddPaymentInfo',
    'AddToCart',
    'AddToWishlist',
    'CompleteRegistration',
    'InitiateCheckout',
    'Lead',
    'Purchase',
    'Search',
    'StartTrial',
    'Subscribe',
    'ViewContent'
  ]

gender only accepted exact 'm'/'f', so a common trait like 'male' or
'female' was dropped on the local (non-param-builder) hashing path.
Normalize male/female (any case) to m/f before hashing, and widen the
gender field choices to match. Added a test covering word-form
normalization and drop of unrecognized values.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 5, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:67

  • Validation can incorrectly pass when content_ids contains only whitespace (e.g., ' '), because it only checks presence/array length and doesn’t apply the same trimming/normalization used later in formatFBEvent. This can result in sending events that effectively have neither content_ids nor contents after normalization. Consider normalizing in validate (e.g., arrify + trim/filter) and validating against the normalized arrays.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`
    }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:92

  • When an invalid currency is provided, it is silently dropped. For events where currency is effectively required (e.g., common Purchase mappings), silently omitting it can lead to hard-to-debug downstream data issues. Consider surfacing a warning (or returning a validation error) when a non-empty currency is present but invalid, especially for Purchase.
function normalizeCurrency(value: unknown): string | undefined {
  const t = trimmed(value)
  if (!t) return undefined
  const upper = t.toUpperCase()
  return CURRENCY_ISO_CODES.has(upper) ? upper : undefined

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/fields.ts:172

  • num_items is now shown for Purchase as well (per fieldDependencies), but the field description still reads as specific to checkout initiation. Either update the description to be event-agnostic (e.g., number of items associated with the event) or remove Purchase from the dependency list to match the description.
export const num_items: InputField = {
  label: 'Number of Items',
  description: 'The number of items when checkout was initiated.',
  type: 'integer',
  minimum: 0,
  default: { '@path': '$.properties.num_items' },

Copilot AI review requested due to automatic review settings August 5, 2026 11:54
- num_items: add Purchase (Pixel reference lists num_items for Purchase).
- content_category / content_name: allow on all events (removed from the
  event-dependency map and dropped their depends_on) — Meta documents them
  as general object properties without a per-event list.
- predicted_ltv: unchanged; confirmed correct (Purchase, Subscribe,
  StartTrial, CompleteRegistration, AddPaymentInfo, CustomEvent) against
  the pLTV value-optimization guide.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:67

  • Validation can pass even when content_ids is a whitespace-only string (truthy, not an array), but formatFBEvent() will trim and drop it—resulting in an event missing required identifiers. Use the same normalization approach in validate() as formatFBEvent() (e.g., trimmedArray(toArray(content_ids)) and toArray(contents)) and validate on the normalized array lengths so validation matches what is actually sent.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`
    }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/depends-on.ts:18

  • getNotVisibleForEvent() relies on fieldDependencies to decide which fields to delete when show_fields === false. By removing certain keys (e.g., content_category, content_name) from fieldDependencies, those fields can no longer be classified as 'not visible' and therefore won’t be removed from the outgoing event when show_fields is false. Either re-add the missing fields to fieldDependencies, or decouple the 'visibility/deletion' list from the 'depends_on gating' list so that UI gating changes don’t unintentionally change what gets sent.
export const fieldDependencies: Record<string, (FBStandardEventType | FBNonStandardEventType)[]> = {
  custom_event_name: ['CustomEvent'],
  content_ids: [
    'AddPaymentInfo',
    'AddToCart',
    'AddToWishlist',
    'InitiateCheckout',
    'Purchase',
    'Search',
    'ViewContent'
  ],
  content_type: ['AddToCart', 'Purchase', 'Search', 'ViewContent'],
  contents: ['AddPaymentInfo', 'AddToCart', 'AddToWishlist', 'InitiateCheckout', 'Purchase', 'Search', 'ViewContent'],
  currency: [
    'AddPaymentInfo',

Copilot AI review requested due to automatic review settings August 5, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/fields.ts:1

  • These fields still have default mappings, but their depends_on gating was removed. As a result, they can be mapped/sent for events where they’re not relevant, and show_fields === false can no longer rely on dependency-driven pruning (since the dependency list no longer includes them). Re-add depends_on: getDependenciesFor(...) for content_category and content_name, and ensure both fields are included in fieldDependencies so getNotVisibleForEvent(...) can remove them when show_fields is false.
import type { InputField } from '@segment/actions-core'

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:66

  • Validation doesn’t align with the new normalization behavior: content_ids can be a scalar string at runtime (and can be whitespace-only). A whitespace-only string currently passes validation but is later trimmed/dropped, potentially resulting in sending an event without content_ids or contents. Update validation to use the same normalization logic as formatFBEvent (e.g., trimmedArray(toArray(content_ids)) and toArray(contents)) before checking emptiness.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/generated-types.ts:7

  • The PR description focuses on removing the broken content_ids default, but this change set also significantly alters the payload shape (flattening event_config into top-level fields) and adds/changes other behaviors (new fields, dependency gating changes, Param Builder script source, new LDU options). Please update the PR description to reflect these broader changes so reviewers and release notes accurately capture the scope.
  event_name: string

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/generated-types.ts:11

  • The PR description focuses on removing the broken content_ids default, but this change set also significantly alters the payload shape (flattening event_config into top-level fields) and adds/changes other behaviors (new fields, dependency gating changes, Param Builder script source, new LDU options). Please update the PR description to reflect these broader changes so reviewers and release notes accurately capture the scope.
  custom_event_name?: string

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/generated-types.ts:15

  • The PR description focuses on removing the broken content_ids default, but this change set also significantly alters the payload shape (flattening event_config into top-level fields) and adds/changes other behaviors (new fields, dependency gating changes, Param Builder script source, new LDU options). Please update the PR description to reflect these broader changes so reviewers and release notes accurately capture the scope.
  show_fields?: boolean

content_ids entries were trimmed but the id (and any other string field)
inside contents objects was not, so a single-product event could send an
untrimmed id in contents while content_ids was trimmed. trimContents now
trims every string value in each contents item. Added a test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 5, 2026 12:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Suppressed comments (3)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:68

  • Validation can incorrectly pass when content_ids is a whitespace-only string (truthy), because this check doesn’t trim/normalize. Later, formatFBEvent() will trim and drop it, causing events to be sent without either content_ids or contents even though validation succeeded. Consider normalizing content_ids/contents inside validate() using the same trimming/array-wrapping logic used by formatFBEvent() (or by calling a shared helper) so the validation reflects what will actually be sent.
  const { event_name, content_ids, contents } = payload

  if (['AddToCart', 'Purchase', 'ViewContent'].includes(event_name)) {
    if (
      (!content_ids || (Array.isArray(content_ids) && content_ids.length === 0)) &&
      (!contents || (Array.isArray(contents) && contents.length === 0))
    ) {
      return `At least one of content_ids or contents is required for the ${event_name} event.`
    }
  }

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/depends-on.ts:14

  • content_category and content_name were removed from fieldDependencies. Since getNotVisibleForEvent() derives “fields to delete” from fieldDependencies, these fields will no longer be removed when show_fields === false, which changes runtime behavior (hidden fields can still be sent if present in the payload). If show_fields is meant to control both UI visibility and runtime field stripping, add content_category/content_name back into fieldDependencies (and, if needed, restore depends_on in fields.ts) so getNotVisibleForEvent() can correctly consider them.
export const fieldDependencies: Record<string, (FBStandardEventType | FBNonStandardEventType)[]> = {
  custom_event_name: ['CustomEvent'],
  content_ids: [
    'AddPaymentInfo',
    'AddToCart',
    'AddToWishlist',
    'InitiateCheckout',
    'Purchase',
    'Search',
    'ViewContent'
  ],

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:32

  • The PR description focuses on removing the broken content_ids default, but this PR also introduces a breaking-ish schema reshaping (event_config → top-level event_name / custom_event_name / show_fields), adds normalization behavior, updates LDU options, and changes the Parameter Builder script source. Please update the PR description (and/or title) to reflect these additional changes so reviewers and downstream consumers understand the full scope.
export async function send(
  client: FBClient,
  clientParamBuilder: FBClientParamBuilder | undefined,
  payload: Payload,
  settings: Settings,
  analytics: Analytics
) {

- content_ids/contents are now trimmed and coerced to arrays in validate()
  (mirroring formatFBEvent), so a whitespace-only content_ids no longer
  passes validation and then gets dropped, sending an event with neither
  identifier.
- Require a non-empty custom_event_name when event_name is CustomEvent,
  preventing fbq('trackSingleCustom', ..., undefined).
Added tests for both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 5, 2026 13:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/fields.ts:65

  • content_category and content_name no longer have depends_on, which makes them appear for all events even when show_fields is false. This is a behavioral regression from the prior gating logic and is inconsistent with the rest of the field visibility approach in this action. Re-introduce depends_on: getDependenciesFor(...) for these fields (and ensure fieldDependencies includes the correct event list) so the UI hides them unless relevant or show_fields is enabled.
export const content_category: InputField = {
  label: 'Content Category',
  description: 'The category of the content associated with the event.',
  type: 'string',
  default: { '@path': '$.properties.category' }
}

export const content_name: InputField = {
  label: 'Content Name',
  description: 'The name of the page or product associated with the event.',
  type: 'string',
  default: { '@path': '$.properties.name' }
}

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/functions.ts:96

  • Currency is silently dropped when invalid. For Purchase events (where the UI marks currency as required), this can result in sending a Purchase without currency even though an input value was provided, which is likely to be rejected or misinterpreted downstream. Consider adding validation in validate(payload) to warn/abort when event_name === 'Purchase' and currency is present-but-invalid (or missing), using the same normalization logic so runtime behavior matches the UI requirement.
function normalizeCurrency(value: unknown): string | undefined {
  const t = trimmed(value)
  if (!t) return undefined
  const upper = t.toUpperCase()
  return CURRENCY_ISO_CODES.has(upper) ? upper : undefined
}

packages/browser-destinations/destinations/facebook-conversions-api-web/src/send/generated-types.ts:15

  • The PR description focuses on removing the broken content_ids default, but the diff also includes a larger, user-visible schema change: moving from event_config.* to top-level event_name/custom_event_name/show_fields (plus added normalization/validation behavior). Please update the PR description to explicitly call out this broader change (and why it’s necessary/non-breaking) so reviewers and release notes accurately reflect the impact.
  event_name: string
  /**
   * Custom event name to send to Facebook
   */
  custom_event_name?: string
  /**
   * Show all fields, even those which are not relevant to the selected Event Name.
   */
  show_fields?: boolean

Comment on lines +96 to 99
if (formatUserDataWithParamBuilder) {
const script = `https://unpkg.com/meta-capi-param-builder-clientjs/dist/clientParamBuilder.bundle.js`
await deps.loadScript(script)
await deps.resolveWhen(() => typeof window.clientParamBuilder === 'object', 100)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants