STRATCONN-6892 - [Reddit Conversions API] - add v3 support - #3958
Open
joe-ayoub-segment wants to merge 23 commits into
Open
STRATCONN-6892 - [Reddit Conversions API] - add v3 support#3958joe-ayoub-segment wants to merge 23 commits into
joe-ayoub-segment wants to merge 23 commits into
Conversation
Adds Reddit Conversions API v3 support alongside the existing v2.0
implementation, gated behind the reddit-conversions-api-canary-version
Flagon flag plus a new per-action api_version field (default Latest V3,
falls back to Legacy V2 for existing configured customers who never set
it).
- New per-action fields: api_version, action_source (conditionally
required only when api_version is v3, no default), event_source_url
(defaults to $.context.page.url), test_id
- products gains quantity/item_price sub-fields
- event_metadata.value_decimal is sent as event_metadata.value on v3;
event_type renames to type with UPPER_SNAKE_CASE tracking_type values
- v3 request: POST /api/v3/pixels/{ad_account_id}/conversion_events,
body wrapped in { data: { events, partner, test_id } }, event_at sent
as epoch milliseconds
- sendV2/sendV3 kept as fully separate, duplicated implementations
(types-v2.ts/types-v3.ts, utils-v2.ts/utils-v3.ts) so v2 can be
deleted outright once the migration is complete
- Settings-level test_mode field removed (unused by any customer)
Not included in this PR: replacing the 8 tracking_type presets with
dedicated actions (Page Visit, Purchase, etc.) - deferred for now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ls.ts/types.ts Deletes types-v2.ts and utils-v2.ts, and restores utils.ts/types.ts to their original main-branch content verbatim. v2 stays completely untouched; v3 is purely additive in types-v3.ts/utils-v3.ts, which now also owns the FLAGON_NAME/isCanary/resolveVersion routing helpers. standardEvent/customEvent call the original send() for the v2 path and sendV3() for v3, per resolveVersion(payload.api_version, features). Known follow-up: utils.ts still references settings.test_mode, which no longer exists on Settings (removed earlier). Left as-is per instruction not to worry about lint/type checks for this push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
New required fields detectedWarning Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production. The following required fields were added in this PR:
Add these new fields as optional instead and assume default values in |
- Move types-v3.ts and utils-v3.ts into a new v3/ subfolder, fixing their relative imports and all call sites - Re-add the test_mode Settings field (removed earlier, but still referenced by v2 utils.ts) and regenerate generated-types.ts + metadata.json - Update test_mode's description to clarify it's V2-only (deprecated) and point to the per-action Test ID field for V3 test events Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
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.
Adds gated Reddit Conversions API v3 support alongside the existing v2 integration, including new per-action fields and v3-specific payload shaping/routing behind a Flagon canary.
Changes:
- Introduces v3 types + request builder/sender and routes events to v2 vs v3 based on feature flag + per-action
api_version. - Adds new action fields for v3 (
api_version,action_source,event_source_url,test_id) and extendsproductsitem schema. - Removes unused destination-level
test_modeand updates/extends Jest coverage for v3 behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts | Adds explicit constant for canary v3 API version. |
| packages/destination-actions/src/destinations/reddit-conversions-api/utils-v3.ts | Implements v3 URL, payload shaping, hashing/cleaning, version resolution, and sender. |
| packages/destination-actions/src/destinations/reddit-conversions-api/types-v3.ts | Defines wire-level v3 payload interfaces. |
| packages/destination-actions/src/destinations/reddit-conversions-api/standardEvent/index.ts | Routes standard events to v2 vs v3; adds new v3 fields to action. |
| packages/destination-actions/src/destinations/reddit-conversions-api/customEvent/index.ts | Routes custom events to v2 vs v3; adds new v3 fields to action. |
| packages/destination-actions/src/destinations/reddit-conversions-api/*/generated-types.ts | Updates generated payload typings for new fields and product sub-fields. |
| packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts | Adds new v3 mapping fields and extends products schema. |
| packages/destination-actions/src/destinations/reddit-conversions-api/index.ts | Removes destination-level test_mode and removes multiple presets. |
| packages/destination-actions/src/destinations/reddit-conversions-api/tests/index.test.ts | Updates v2 assertions and adds v3 (canary) routing tests. |
| packages/destination-actions/src/destinations/reddit-conversions-api/**/snapshots/*.snap | Updates snapshots for test_mode removal and adds additional snapshots. |
Suppressed comments (2)
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:8
- The PR description says replacing the 8 tracking_type presets with dedicated actions is deferred, but this diff removes multiple presets from the destination definition. If this is intentional (e.g., presets moved/renamed elsewhere in this PR), the description should be updated to match; otherwise, the removed presets should be restored to avoid an unintended behavior change for users relying on them.
const destination: DestinationDefinition<Settings> = {
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:82
- The PR description says replacing the 8 tracking_type presets with dedicated actions is deferred, but this diff removes multiple presets from the destination definition. If this is intentional (e.g., presets moved/renamed elsewhere in this PR), the description should be updated to match; otherwise, the removed presets should be restored to avoid an unintended behavior change for users relying on them.
presets: [
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Replace raw 'v2'/'v3' string literals with named constants (LEGACY_API_VERSION, LATEST_API_VERSION) exported from versioning-info.ts, used consistently across fields.ts, resolveVersion(), and both action perform/performBatch handlers - LEGACY_API_VERSION reuses the existing v2.0 URL-segment constant rather than introducing a separate 'v2' identifier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:35
- The PR description says the settings-level
test_modefield was removed, but it still exists in the destination settings schema (and in metadata/generated types). Either remove it end-to-end (schema + metadata + any v2 payload usage) or update the PR description to reflect that it's only deprecated (not removed).
label: '[Deprecated] Test Mode',
description:
'Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.',
type: 'boolean',
required: false,
default: false
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:229
canonicalizeEmailassumes the input contains exactly one '@'. If an email value is present but malformed (no '@' or trailing '@'),localPartAndDomain[1]will beundefinedand this will produce an invalid canonical email (or throw when calling.toLowerCase()). Add a guard (e.g., require exactly 2 parts and a non-empty domain) and fall back to a safer normalization (trim + lowercase) or skip hashing for invalid emails.
function canonicalizeEmail(value: string): string {
value = value.trim()
const localPartAndDomain = value.split('@')
const localPart = localPartAndDomain[0].replace(/\./g, '').split('+')[0]
return `${localPart.toLowerCase()}@${localPartAndDomain[1].toLowerCase()}`
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/types-v3.ts:50
action_sourceis treated as required by the v3 sender (it throws if missing) and is conditionally required by the field schema for v3. Keeping it optional inV3EventItemweakens type-safety and can let incorrect callers compile. Makeaction_sourcerequired inV3EventItem(and optionally narrow it to the allowed enum values) to align the types with actual runtime requirements.
export interface V3EventItem {
event_at: number
action_source?: string
event_source_url?: string
click_id?: string
type: {
tracking_type: string
custom_event_name?: string
}
event_metadata?: V3Metadata
user?: V3User
}
packages/destination-actions/src/destinations/reddit-conversions-api/standardEvent/index.ts:53
- The batch router filters the same
payloadarray twice and re-runsresolveVersionfor each event two times. This can be simplified into a single pass partition (buildv2Payloadsandv3Payloadsin one loop) to reduce repeated work and keep the routing logic in one place. The same pattern appears incustomEvent/index.ts.
performBatch: async (request, { settings, payload, features }) => {
const v2Payloads = payload.filter((p) => resolveVersion(p.api_version, features) === LEGACY_API_VERSION)
const v3Payloads = payload.filter((p) => resolveVersion(p.api_version, features) === LATEST_API_VERSION)
const requests = []
if (v2Payloads.length) requests.push(send(request, settings, v2Payloads))
if (v3Payloads.length) requests.push(sendV3(request, settings, v3Payloads))
return Promise.all(requests)
}
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The earlier doc comment with the Reddit API reference link was removed. Since these constants define externally-versioned behavior, re-adding a brief comment/link (e.g., v2 changelog and v3 docs) would help future maintainers validate version semantics and deprecation timelines.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:134
- The new
toEpochMsvalidation has multiple important branches (epoch-ms number, epoch-ms numeric string, ISO string, and rejection of epoch seconds viaEPOCH_MS_MIN). Current integration tests cover the ISO-string path, but not the numeric-string/number paths or the rejection behavior. Add focused unit tests (or extend existing tests) to cover: (1) numeric epoch-ms string input, (2) integer epoch-ms number input, and (3) a 10-digit epoch-seconds value being rejected with the expected error.
const EPOCH_MS_MIN = 1e12
export function toEpochMs(value: string | number | undefined): number {
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
// Already epoch milliseconds (number or numeric string), only if plausibly ms.
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string' && /^\d+$/.test(value.trim()) && Number(value.trim()) >= EPOCH_MS_MIN) {
return Number(value.trim())
}
// ISO 8601 / RFC3339 string.
if (typeof value === 'string' && !/^\d+$/.test(value.trim())) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
throw new PayloadValidationError(
`event_at must be an ISO 8601 timestamp or epoch milliseconds, received: ${String(value)}`
)
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Suppressed comments (5)
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:134
- The new
toEpochMsbehavior has multiple important branches (epoch-ms numeric string, epoch-seconds rejection viaEPOCH_MS_MIN, invalid timestamps) but the added tests only appear to cover the ISO-string path. Please add unit tests covering: (1) numeric epoch-ms string, (2) 10-digit epoch-seconds string/number being rejected, and (3) invalid timestamp strings producing the expected validation error.
const EPOCH_MS_MIN = 1e12
export function toEpochMs(value: string | number | undefined): number {
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
// Already epoch milliseconds (number or numeric string), only if plausibly ms.
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string' && /^\d+$/.test(value.trim()) && Number(value.trim()) >= EPOCH_MS_MIN) {
return Number(value.trim())
}
// ISO 8601 / RFC3339 string.
if (typeof value === 'string' && !/^\d+$/.test(value.trim())) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
throw new PayloadValidationError(
`event_at must be an ISO 8601 timestamp or epoch milliseconds, received: ${String(value)}`
)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:194
modescan become['']when the input is an empty string or contains trailing commas (e.g.'LDU,'), which may be an invalid value to send downstream. Consider trimming and filtering empty tokens (e.g.split -> trim -> filter(Boolean)), and returningundefined(or an empty array, depending on Reddit's schema) when no valid modes remain.
return {
country: clean(dataProcessingOptions.country),
modes: dataProcessingOptions.modes?.split(',').map((mode) => mode.trim()),
region: clean(dataProcessingOptions.region)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:64
- Serializing and parsing the payload (
JSON.parse(JSON.stringify(data))) adds overhead and can mask issues by coercing values (and would drop non-JSON types if introduced later). Prefer passing the plain object directly asjson: data. If the intent is to omitundefinedkeys, consider using a dedicated, explicit sanitizer that preserves types while removing onlyundefined.
return request(V3_URL(settings.ad_account_id), {
method: 'POST',
headers: { Authorization: `Bearer ${settings.conversion_token}` },
json: JSON.parse(JSON.stringify(data))
})
packages/destination-actions/src/destinations/reddit-conversions-api/v3/types-v3.ts:41
action_sourceis treated as required increateRedditPayloadV3(it throws if missing), but the type marks it optional. Making this field required inV3EventItemwill align compile-time guarantees with runtime behavior and reduce the chance of accidentally constructing invalid v3 payloads in future refactors.
export interface V3EventItem {
event_at: number
action_source?: string
event_source_url?: string
click_id?: string
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:35
- The PR description states: "Settings-level test_mode field removed". In the changes shown,
test_modestill exists as a destination setting (now labeled deprecated). Please either (a) update the PR description to reflect thattest_moderemains but is deprecated, or (b) remove the setting from the destination definition and generated types if the intent is to fully remove it.
label: '[Deprecated] Test Mode',
description:
'Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.',
type: 'boolean',
required: false,
default: false
- Add action.ts exporting standardEventAction(trackingType, title, description, defaultSubscription?) - builds the shared fields/ perform/performBatch logic for the standardEvent action family. When trackingType is set, tracking_type is hardcoded and hidden from the UI (for future preset-replacement actions); when undefined, tracking_type stays user-selectable (current standardEvent behavior) - standardEvent/index.ts is now a one-line call into the factory - Move isCanary/resolveVersion from v3/utils-v3.ts into action.ts (FLAGON_NAME stays in v3/utils-v3.ts); customEvent/index.ts now imports resolveVersion from ../action instead Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- api_version field: default flips to LEGACY_API_VERSION (V2); choices/descriptions across fields.ts and test_mode reworded to frame V2 as the current standard and V3 as beta, not "legacy vs latest" - Restore the 8 tracking_type presets (Page Visit, View Content, Search, Add to Cart, Add to Wishlist, Purchase, Lead, Sign Up) removed earlier in this branch, matching main's mapping shape but with api_version explicitly pinned to LEGACY_API_VERSION so they always resolve to V2 regardless of the Flagon flag state - Regenerate generated-types.ts for root/standardEvent/customEvent to match the description wording changes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (7)
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:50
- The PR description states the per-action
api_versiondefault should be “Latest v3” (with existing customers staying on v2 because the stored mapping has no value), but the field default is currentlyLEGACY_API_VERSION(v2.0). Either update the description, or change the default (and any templates/default mappings that intentionally pin v2) so new mappings default to v3 while “missing field” continues to resolve to v2.
export const api_version: InputField = {
label: 'API Version',
description:
'The version of the Reddit Conversions API to send this event to. "V3 (Beta)" requires Action Source to be set.',
type: 'string',
required: false,
default: LEGACY_API_VERSION,
choices: [
{ label: 'V3 (Beta)', value: LATEST_API_VERSION },
{ label: 'V2', value: LEGACY_API_VERSION }
]
packages/destination-actions/src/destinations/reddit-conversions-api/v3/types-v3.ts:41
action_sourceis treated as required for v3 at runtime (payload creation throws when missing), but the v3 wire type marks it optional. Making this property required inV3EventItemwill better reflect the API contract and prevent accidental creation of invalid v3 event objects in future changes.
export interface V3EventItem {
event_at: number
action_source?: string
event_source_url?: string
click_id?: string
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:209
canonicalizeEmailassumes the email contains exactly one@and will produce a malformed canonical form (or throw when calling.toLowerCase()onundefined) if the input is missing a domain part. Consider validating the split result and either (a) return a conservative normalized value (e.g.,value.toLowerCase()) or (b) skip hashing/canonicalization for invalid emails to avoid sending incorrect hashed identifiers to Reddit.
function canonicalizeEmail(value: string): string {
value = value.trim()
const localPartAndDomain = value.split('@')
const localPart = localPartAndDomain[0].replace(/\./g, '').split('+')[0]
return `${localPart.toLowerCase()}@${localPartAndDomain[1].toLowerCase()}`
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:112
- The
EPOCH_MS_MIN = 1e12guard will reject legitimate historical timestamps before ~2001-09-09, and also rejects common “epoch seconds” inputs rather than converting them. If backfills or user-provided timestamps are realistic here, consider accommodating 10-digit seconds (convert to ms) or lowering the minimum bound so you prevent 1970-misreads without dropping otherwise valid events.
// v3 requires event_at as an integer Unix epoch in milliseconds. We own the timestamp source
// (defaults to $.timestamp, an ISO string), so we accept ISO strings and epoch-ms; anything else
// is rejected rather than sent wrong. EPOCH_MS_MIN guards against epoch *seconds* being misread
// as ms (a 10-digit seconds value is < 1e12, so it's rejected instead of landing in 1970).
const EPOCH_MS_MIN = 1e12
export function toEpochMs(value: string | number | undefined): number {
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
// Already epoch milliseconds (number or numeric string), only if plausibly ms.
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string' && /^\d+$/.test(value.trim()) && Number(value.trim()) >= EPOCH_MS_MIN) {
return Number(value.trim())
}
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:83
- The v2/v3 split logic is duplicated here and in
customEvent/index.ts. Consider extracting a small helper (e.g., “partitionByResolvedVersion” or “buildVersionedRequests”) to keep routing behavior consistent as fields/versions evolve and reduce the chance of drift between Standard and Custom event actions.
performBatch: async (request, { settings, payload, features }) => {
const resolvedPayloads = trackingType ? payload.map((p) => ({ ...p, tracking_type: trackingType })) : payload
const v2Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version, features) === LEGACY_API_VERSION)
const v3Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version, features) === LATEST_API_VERSION)
const requests = []
if (v2Payloads.length) requests.push(send(request, settings, v2Payloads))
if (v3Payloads.length) requests.push(sendV3(request, settings, v3Payloads))
return Promise.all(requests)
packages/destination-actions/src/destinations/reddit-conversions-api/metadata.json:31
metadata.jsonmarkstest_modeas deprecated, but the runtime destination settings definition inindex.tsstill labels it simply “Test Mode” (andgenerated-types.tsstill includes it). To avoid confusing customers, align the label/description between the catalog metadata and the actual settings UI definition (or remove the setting entirely if that’s the intended outcome).
"label": "[Deprecated] Test Mode",
"description": "Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.",
"type": "boolean",
"required": false,
"multiple": false,
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:114
- The new
toEpochMsbehavior has several branches (ISO parsing, numeric string parsing, integer ms validation, and error cases). The current v3 tests cover the ISO-to-epoch-ms path, but adding targeted unit tests for numeric string ms input and rejection/error cases would help prevent regressions in timestamp handling.
export function toEpochMs(value: string | number | undefined): number {
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
// Already epoch milliseconds (number or numeric string), only if plausibly ms.
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string' && /^\d+$/.test(value.trim()) && Number(value.trim()) >= EPOCH_MS_MIN) {
return Number(value.trim())
}
// ISO 8601 / RFC3339 string.
if (typeof value === 'string' && !/^\d+$/.test(value.trim())) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
throw new PayloadValidationError(
`event_at must be an ISO 8601 timestamp or epoch milliseconds, received: ${String(value)}`
)
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:1
- The PR description states that the settings-level
test_modefield is removed, but it remains exposed in the destination settings (and is also still present in generated settings types and referenced by v2 payload building). If the intention is deprecation (asmetadata.jsonsuggests), update the PR description accordingly; if the intention is true removal, the settings schema and any remaining usage in v2 payload construction should be removed in this PR.
import { defaultValues, DestinationDefinition } from '@segment/actions-core'
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:174
- When
dataProcessingOptions.modesis an empty string (or contains extra commas), this produces[''](or includes empty entries), which can be invalid/noisy on the wire. Consider filtering out falsy/empty trimmed modes after splitting so the API sees only meaningful values.
return {
country: clean(dataProcessingOptions.country),
modes: dataProcessingOptions.modes?.split(',').map((mode) => mode.trim()),
region: clean(dataProcessingOptions.region)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/types-v3.ts:41
action_sourceis enforced as required increateRedditPayloadV3(it throws when missing), but the type marks it optional. This mismatch weakens type-safety and makes it easier for future refactors to accidentally construct invalid payloads. Consider makingaction_source: stringrequired onV3EventItem(and/or adjusting upstream payload typing) to match actual requirements.
export interface V3EventItem {
event_at: number
action_source?: string
event_source_url?: string
- customEvent/index.ts is now a one-line call into a new
customEventAction(title, description, defaultSubscription?)
factory in action.ts, sharing buildEventAction's fields/perform/
performBatch logic with standardEventAction
- buildEventAction takes two named optional fields (trackingTypeField,
customEventNameField) instead of a generic fields bag - exactly one
is passed per action variant, the other stays undefined and is
omitted from fields
- resolveVersion no longer checks the Flagon canary flag - version
selection is based purely on the payload's api_version value now.
Removed isCanary/FLAGON_NAME (dead code) and the now-meaningless
features:{...} from test payloads
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
packages/destination-actions/src/destinations/reddit-conversions-api/tests/index.test.ts:1
- Tests set
api_version: 'v2', but your defined legacy version constant/choice value is'v2.0'. Using'v2'may not reflect real configured mappings produced by the UI/choices and can hide issues if strict validation is later added. Prefer using the actual legacy value ('v2.0') or importLEGACY_API_VERSIONin tests.
import nock from 'nock'
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:44
test_idis lifted only from the first payload in a batch; if later payloads have a differenttest_id(or unset), the request will silently use the first value for all events. Since v3 placestest_idat the request level, consider validating that all payloads have the sametest_id(or all unset) and throw aPayloadValidationErrorif they differ.
function createRedditPayloadV3(payloads: StandardEvent[] | CustomEvent[]): V3Payload {
const test_id = clean((payloads[0] as StandardEvent | CustomEvent)?.test_id)
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:168
- Splitting
modesand trimming can produce empty strings (e.g.,"LDU,"=>["LDU", ""]), which may be rejected downstream. Filter out empty entries after trimming (and consider omittingmodesentirely if it becomes an empty array).
return {
country: clean(dataProcessingOptions.country),
modes: dataProcessingOptions.modes?.split(',').map((mode) => mode.trim()),
region: clean(dataProcessingOptions.region)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:203
canonicalizeEmailassumes the input contains exactly one@. For invalid inputs (missing@, multiple@),localPartAndDomain[1]can beundefined, producing a corrupted canonical form and potentially hashing the wrong value. Add a guard (e.g., if the split doesn’t yield exactly 2 parts) to fall back to a safe normalization (trim/lowercase) or to throw a validation error.
function canonicalizeEmail(value: string): string {
value = value.trim()
const localPartAndDomain = value.split('@')
const localPart = localPartAndDomain[0].replace(/\./g, '').split('+')[0]
return `${localPart.toLowerCase()}@${localPartAndDomain[1].toLowerCase()}`
}
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:73
- These casts to
StandardEvent/StandardEvent[]are incorrect for the custom event action path and reduce type safety (e.g.,custom_event_namebecomes invisible to the compiler). Keep the values typed as the genericPayload(or as(StandardEvent|CustomEvent)) so TypeScript can validate both variants and you don’t accidentally mask future changes.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload])
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
const v2Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version) === LEGACY_API_VERSION)
const v3Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version) === LATEST_API_VERSION)
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:80
- The new batching behavior can fan out into two HTTP requests (one for v2 payloads and one for v3 payloads). Add a unit test that sends a mixed-version batch and asserts that two requests are made with the correct endpoints/bodies (and that the returned
Promise.allshape is what callers expect).
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
const v2Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version) === LEGACY_API_VERSION)
const v3Payloads = resolvedPayloads.filter((p) => resolveVersion(p.api_version) === LATEST_API_VERSION)
const requests = []
if (v2Payloads.length) requests.push(send(request, settings, v2Payloads))
if (v3Payloads.length) requests.push(sendV3(request, settings, v3Payloads))
return Promise.all(requests)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/types-v3.ts:41
- In the v3 sender,
action_sourceis treated as required (it throws if missing). Marking it optional inV3EventItemmakes the type inconsistent with actual runtime requirements and reduces compile-time guarantees. Consider makingaction_sourcerequired inV3EventItem.
export interface V3EventItem {
event_at: number
action_source?: string
event_source_url?: string
packages/destination-actions/src/destinations/reddit-conversions-api/metadata.json:31
- The PR description says the settings-level
test_modefield was removed, but the destination metadata still exposes it (now marked deprecated). Either update the PR description to reflect that it’s deprecated (not removed) or remove it from metadata/settings definitions if the intent is full removal.
"label": "[Deprecated] Test Mode",
"description": "Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.",
…wn api_version and test_id - sendV3 now takes raw payloads + an isBatch flag: for a single event it throws on invalid input as before; for a batch it validates each payload independently (via the merged createRedditPayloadV3), sends only the valid ones in one request, and reports per-item validation errors through a MultiStatusResponse instead of failing the whole batch. V2 batches are unaffected - homogeneous per-mapping api_version means a batch is never mixed V2/V3 - Add ProductV3.id required-id validation (throws if missing/blank) now that the type requires it; V2's getProducts gains matching quantity/item_price mapping - New v3/constants.ts: ACTION_SOURCE_V3 (+ labels) and EVENT_TYPE_V3, used to type-narrow action_source/tracking_type in types-v3.ts and to generate the action_source field's choices in fields.ts - api_version field: disabledInputMethods locks it to the choices dropdown only - customers can no longer map it from event payload data - test_id moved from an action-level field to a destination-level Settings field (settings are never payload-mapped, so this was a cleaner way to guarantee literal-only entry than restricting the action field directly) - Consolidated duplicate helpers (clean, cleanNum, getAdId, getDataProcessingOptions, getScreen, getUser, canonicalizeEmail, smartHash, cleanPhoneNumber) into utils.ts, exported and reused from v3/utils-v3.ts instead of being duplicated Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
createRedditPayloadV3 now takes the whole payloads array (plus settings, multiStatusResponse, isBatch) and builds the full PayloadV3 envelope internally - looping, validating, and populating the MultiStatusResponse per item - the same shape as v2's createRedditPayload(payloads, settings). sendV3 is simplified to: build the payload, POST if there's anything to send, and return appropriately. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EVENT_TYPE_V3 was only ever used to type TRACKING_TYPE_V3's values and derive EventTypeV3 - never as an array in its own right, and its values duplicated TRACKING_TYPE_V3's values exactly. Made TRACKING_TYPE_V3 `as const` and derive EventTypeV3 from its values directly instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (8)
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:53
- The PR description says the per-action
api_versiondefaults to Latest (v3) for new mappings, falling back to legacy v2 only for existing customers who never set it. This field currently defaults toLEGACY_API_VERSION, which will keep new mappings on v2 by default; either update the default toLATEST_API_VERSION(and handle the “existing mapping missing the field” case separately) or update the PR description to match the implemented behavior.
export const api_version: InputField = {
label: 'API Version',
description:
'The version of the Reddit Conversions API to send this event to. "V3 (Beta)" requires Action Source to be set.',
type: 'string',
required: false,
default: LEGACY_API_VERSION,
choices: [
{ label: 'V3 (Beta)', value: LATEST_API_VERSION },
{ label: 'V2', value: LEGACY_API_VERSION }
],
disabledInputMethods: ['literal', 'variable', 'function', 'freeform', 'enrichment']
}
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:43
- The PR description states “Settings-level test_mode field removed” and also calls out a new per-action
test_idfield, but this change set keepstest_modein destination settings and introducestest_idas a settings-level field. Please align the implementation with the PR description (removetest_modefrom settings and/or addtest_idat the action level), or update the PR description to reflect the actual design.
test_mode: {
label: 'Test Mode',
description:
'Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2. V3 (Beta) is the latest API version. To send test events on V3, set the Test ID setting instead.',
type: 'boolean',
required: false,
default: false
},
test_id: {
label: 'Test ID',
description:
'A test ID from Reddit Event Testing. When set, events are routed to Event Testing for verification instead of production. Remove before sending production traffic. Only applies to Reddit Conversions API V3 (Beta).',
type: 'string',
required: false
}
packages/destination-actions/src/destinations/reddit-conversions-api/utils.ts:94
quantityanditem_priceare now included in the v2 payload shaping as well. The PR description indicates these additions are for v3, so including them in v2 requests risks breaking v2 if the endpoint rejects unknown fields. Recommendation: only add these fields in the v3 transformer (or conditionally omit them when sending v2).
return {
category: clean(product.category),
id: clean(product.id),
name: clean(product.name),
quantity: cleanNum(product.quantity),
item_price: cleanNum(product.item_price)
}
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
buildEventActionis generic overPayload extends StandardEvent | CustomEvent, but it force-casts both single and batch payloads toStandardEvent. This weakens type-safety for thecustomEventaction (and can mask shape issues in future edits). Suggestion: preserve the genericPayloadtype (or cast toStandardEvent | CustomEvent) and pass that through tosend/sendV3, which already accept both payload shapes.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:63
- The
indicesarray is unnecessary here, and usingindices.indexOf(index)adds an avoidable linear lookup for every payload (O(n²) overall). You can removeindicesentirely and setsentdirectly toevent(orevents[events.length - 1]) since you already have the event object in scope.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
const {
event_at,
click_id,
products,
user,
data_processing_options,
screen_dimensions,
event_metadata,
conversion_id,
action_source,
event_source_url
} = payload
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- The
indicesarray is unnecessary here, and usingindices.indexOf(index)adds an avoidable linear lookup for every payload (O(n²) overall). You can removeindicesentirely and setsentdirectly toevent(orevents[events.length - 1]) since you already have the event object in scope.
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/metadata.json:31
- This metadata marks
test_modeas “[Deprecated]”, butindex.tsstill presents the settings field label as “Test Mode” (non-deprecated). Please make the labels/descriptions consistent acrossmetadata.jsonand the destination definition so the UI and generated docs don’t disagree about deprecation status.
"label": "[Deprecated] Test Mode",
"description": "Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.",
"type": "boolean",
"required": false,
"multiple": false,
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:118
toEpochMscontains several important parsing branches and validation rules (epoch ms number, epoch ms digit string, ISO 8601 string, invalid inputs). Current v3 tests cover the ISO string path but not the other branches. Please add focused unit tests for: digit-string epoch ms, numeric epoch ms, rejection of epoch seconds, and an invalid string to ensure error messaging is stable.
export function toEpochMs(value: string | number | undefined): number {
const EPOCH_MS_MIN = 1e12
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string') {
const trimmed = value.trim()
const isDigitsOnly = /^\d+$/.test(trimmed)
if (isDigitsOnly && Number(trimmed) >= EPOCH_MS_MIN) return Number(trimmed)
if (!isDigitsOnly) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
}
throw new PayloadValidationError(
`event_at must be an ISO 8601 timestamp or epoch milliseconds, received: ${String(value)}`
)
}
Same pattern as the earlier EVENT_TYPE_V3/TRACKING_TYPE_V3 dedupe: ACTION_SOURCE_V3_LABELS was already keyed by the same values ACTION_SOURCE_V3 held, so the array was pure duplication. Derive ActionSourceV3 from the labels object's keys instead, build fields.ts's choices via Object.entries(), and validate via the `in` operator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Suppressed comments (7)
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:53
- The PR description says
api_versiondefaults to “Latest V3” with fallback to legacy v2 for existing customers who never set it, but the field default here isLEGACY_API_VERSION(v2.0) andresolveVersion()routes missing/unknown values to v2.0. Either update the PR description to match the shipped behavior, or adjust the implementation to default new mappings to v3 while still routing truly “missing field” legacy mappings to v2 (e.g., by distinguishing “undefined because field didn’t exist” from “defaulted” and/or by only applying a v3 default in UI/templates rather than at runtime).
export const api_version: InputField = {
label: 'API Version',
description:
'The version of the Reddit Conversions API to send this event to. "V3 (Beta)" requires Action Source to be set.',
type: 'string',
required: false,
default: LEGACY_API_VERSION,
choices: [
{ label: 'V3 (Beta)', value: LATEST_API_VERSION },
{ label: 'V2', value: LEGACY_API_VERSION }
],
disabledInputMethods: ['literal', 'variable', 'function', 'freeform', 'enrichment']
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:118
EPOCH_MS_MIN = 1e12rejects valid epoch-millisecond timestamps prior to ~2001-09-09 (e.g.,946684800000for 2000-01-01). If the contract is “epoch milliseconds”, the validator should accept any reasonable ms value and only reject values that look like epoch seconds (commonly 10 digits) or otherwise invalid. A more robust approach is to infer seconds vs ms by digit length / magnitude (e.g., treat 10-digit integers as seconds and either convert to ms or reject with a clearer message) rather than hard-rejecting all ms < 1e12.
export function toEpochMs(value: string | number | undefined): number {
const EPOCH_MS_MIN = 1e12
if (value === undefined || value === null || value === '') {
throw new PayloadValidationError('event_at is required')
}
if (typeof value === 'number' && Number.isInteger(value) && value >= EPOCH_MS_MIN) return value
if (typeof value === 'string') {
const trimmed = value.trim()
const isDigitsOnly = /^\d+$/.test(trimmed)
if (isDigitsOnly && Number(trimmed) >= EPOCH_MS_MIN) return Number(trimmed)
if (!isDigitsOnly) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
}
throw new PayloadValidationError(
`event_at must be an ISO 8601 timestamp or epoch milliseconds, received: ${String(value)}`
)
}
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
- These casts force
resolvedPayload/resolvedPayloadstoStandardEventeven when the action is aCustomEvent. It works at runtime but undermines type-safety (e.g., could hide missingcustom_event_namehandling or allow incompatible payload shaping without compiler feedback). Suggested fix: keepresolvedPayloadtyped asPayload(orStandardEvent | CustomEvent) and pass it directly tosend/sendV3, which already accept unions; avoid theas StandardEventcasts.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:48
events[indices.indexOf(index)]is O(n) inside a loop, making this O(n²) across payloads, and it’s redundant becauseeventis already in scope and corresponds to the item being recorded. Usesent: event(cast as needed) and dropindices/indexOfto reduce complexity and improve readability.
const indices: number[] = []
const events: EventItemV3[] = []
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
events[indices.indexOf(index)]is O(n) inside a loop, making this O(n²) across payloads, and it’s redundant becauseeventis already in scope and corresponds to the item being recorded. Usesent: event(cast as needed) and dropindices/indexOfto reduce complexity and improve readability.
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The previous file-level JSDoc (including the API reference/changelog link) was removed. Since this file defines the public version constants that drive endpoint selection, it would be helpful to restore a brief doc comment explaining what “legacy” vs “latest” mean and linking to the official Reddit API docs/changelog.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
packages/destination-actions/src/destinations/reddit-conversions-api/metadata.json:31
- The description says “set the Test ID field on an action mapping”, but in this PR
test_idis implemented as a destination-level setting (used assettings.test_idin v3 payload creation). Update this description to match the actual configuration surface (either “Test ID setting” or movetest_idto an action-mapping field if that’s the intended design).
"label": "[Deprecated] Test Mode",
"description": "Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2, which is deprecated - V3 is the latest API version. To send test events on V3, set the Test ID field on an action mapping instead.",
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
packages/destination-actions/src/destinations/reddit-conversions-api/index.ts:36
- The PR description states 'Settings-level test_mode field removed', but
test_modeis still present in the destination settings (and referenced in metadata/generated types). Either removetest_modefrom code + metadata as described, or update the PR description to reflect that it remains supported for v2.
test_mode: {
label: 'Test Mode',
description:
'Indicates if events should be treated as test events by Reddit. Only applies to Reddit Conversions API V2. V3 (Beta) is the latest API version. To send test events on V3, set the Test ID setting instead.',
type: 'boolean',
required: false,
default: false
},
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:62
- These casts force all payloads to
StandardEventeven when the action iscustomEvent. While it likely works at runtime due to structural typing, it weakens compile-time guarantees and can mask future regressions (e.g., if v2/v3 senders diverge further). Consider keeping the payload type as the genericPayload(or(StandardEvent | CustomEvent)) all the way through, and passing that through tosend/sendV3without narrowing viaas StandardEvent.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:51
- The
indicesarray plusindices.indexOf(index)makes success recording O(n²) over the batch and is unnecessary becauseeventis already available at the success site. You can setsentdirectly toevent(orevents[events.length - 1]) and dropindicesentirely.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- The
indicesarray plusindices.indexOf(index)makes success recording O(n²) over the batch and is unnecessary becauseeventis already available at the success site. You can setsentdirectly toevent(orevents[events.length - 1]) and dropindicesentirely.
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The previous file-level doc comment (including the Reddit API changelog reference) was removed. Since these constants define wire-level compatibility, consider reintroducing a brief comment and link (updated for v3) so future changes to version strings/endpoints are easier to validate.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:50
- Building
indicesand then doingindices.indexOf(index)makes success recording O(n²) and is unnecessary becauseeventis already in scope. Useeventdirectly forsent(or useevents[events.length - 1]immediately after pushing) and remove theindicesarray to simplify the logic and reduce overhead for large batches.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- Building
indicesand then doingindices.indexOf(index)makes success recording O(n²) and is unnecessary becauseeventis already in scope. Useeventdirectly forsent(or useevents[events.length - 1]immediately after pushing) and remove theindicesarray to simplify the logic and reduce overhead for large batches.
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:62
- These casts force
resolvedPayload/resolvedPayloadstoStandardEventeven when the action iscustomEvent, which defeats type safety and can hide real schema/type issues during future changes. Keep the resolved payload typed as the genericPayload(or(StandardEvent | CustomEvent)) and pass that through tosend/sendV3without narrowing it toStandardEvent.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:111
- You compute
trimmedbut parse the untrimmed originalvaluewithDate.parse(value). This can cause avoidable validation failures for ISO timestamps with leading/trailing whitespace. Parsetrimmedinstead to match the intent of the earlier normalization.
if (typeof value === 'string') {
const trimmed = value.trim()
const isDigitsOnly = /^\d+$/.test(trimmed)
if (isDigitsOnly && Number(trimmed) >= EPOCH_MS_MIN) return Number(trimmed)
if (!isDigitsOnly) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
}
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:53
- The PR title says 'add v3 support behind feature flag', but the implementation gates via a per-mapping
API Versiondropdown (no feature flag). To avoid confusion in release notes and reviews, update the PR title (or the referenced ticket scope) to reflect the actual rollout mechanism.
export const api_version: InputField = {
label: 'API Version',
description:
'The version of the Reddit Conversions API to send this event to. "V3 (Beta)" requires Action Source to be set.',
type: 'string',
required: false,
default: LEGACY_API_VERSION,
choices: [
{ label: 'V3 (Beta)', value: LATEST_API_VERSION },
{ label: 'V2', value: LEGACY_API_VERSION }
],
disabledInputMethods: ['literal', 'variable', 'function', 'freeform', 'enrichment']
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (7)
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:48
- In
createRedditPayloadV3,indices.indexOf(index)inside the loop makes payload construction O(n²) for batched requests. Sinceeventis already available (and events are appended in the same iteration), you can removeindicesentirely and setsentdirectly fromevent(or fromevents[events.length - 1]) to keep this O(n) and simplify control flow.
const indices: number[] = []
const events: EventItemV3[] = []
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:85
- In
createRedditPayloadV3,indices.indexOf(index)inside the loop makes payload construction O(n²) for batched requests. Sinceeventis already available (and events are appended in the same iteration), you can removeindicesentirely and setsentdirectly fromevent(or fromevents[events.length - 1]) to keep this O(n) and simplify control flow.
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:62
buildEventActioncastsresolvedPayload/resolvedPayloadstoStandardEventeven when building thecustomEventaction. This defeats type-safety and makes it easier to accidentally rely on StandardEvent-only fields later. Consider keepingresolvedPayloadtyped asPayload(the generic) and adjustingsendto acceptPayload[](or overloadingsend/ adding a generic) so both standard/custom actions remain strongly typed without assertions.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
buildEventActioncastsresolvedPayload/resolvedPayloadstoStandardEventeven when building thecustomEventaction. This defeats type-safety and makes it easier to accidentally rely on StandardEvent-only fields later. Consider keepingresolvedPayloadtyped asPayload(the generic) and adjustingsendto acceptPayload[](or overloadingsend/ adding a generic) so both standard/custom actions remain strongly typed without assertions.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:109
toEpochMsrejects epoch-millisecond timestamps earlier than1e12(Sep 2001). Epoch-ms values for legitimate historical events (e.g., 1990s backfills) are < 1e12 and will incorrectly throw. If the intent is only to reject epoch seconds, consider validating by digit-length/magnitude (e.g., treat ~1e9–1e10 as seconds) while allowing any non-negative epoch-ms value.
export function toEpochMs(value: string | number | undefined): number {
const EPOCH_MS_MIN = 1e12
packages/destination-actions/src/destinations/reddit-conversions-api/standardEvent/generated-types.ts:37
- These docstrings say
quantity/item_priceonly apply to v3, but the PR description and the v2 implementation update inutils.tsindicate they are included in the v2 wire payload as well. To avoid misleading API consumers, update the generated-type comments (and corresponding field descriptions infields.ts/metadata.jsonif needed) to reflect that these fields are sent for both v2 and v3 (or, alternatively, stop sending them in v2 if that’s not intended).
/**
* The number of this product in the event. Only applies to Reddit Conversions API V3 (Beta).
*/
quantity?: number
/**
* The unit price of the product. Only applies to Reddit Conversions API V3 (Beta).
*/
item_price?: number
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:26
- PR title references 'behind feature flag', but the implementation (including
resolveVersion) uses only the per-mappingapi_versionfield for rollout and no flag gating. Consider updating the PR title (or adding a brief note in code comments/docs) so the reviewed change matches the stated rollout mechanism.
export function resolveVersion(apiVersion: string | undefined): ApiVersion {
return apiVersion === LATEST_API_VERSION ? LATEST_API_VERSION : LEGACY_API_VERSION
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (7)
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The PR title says “add v3 support behind feature flag”, but the implementation uses only the new per-mapping
api_versionfield (no feature flag). Please update the PR title (or add the flag gating) so the PR description/title and rollout mechanism are consistent.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:345
- The field descriptions say
quantity/item_price“Only applies” to v3, but this PR also adds them to the v2 wire payload (utils.tsnow sends them for v2 as well). Please update these descriptions (and the corresponding generated-types docs) to reflect that they’re sent for both API versions (or clarify the exact behavior).
quantity: {
label: 'Quantity',
description: 'The number of this product in the event. Only applies to Reddit Conversions API V3 (Beta).',
type: 'integer',
required: false
},
item_price: {
label: 'Item Price',
description: 'The unit price of the product. Only applies to Reddit Conversions API V3 (Beta).',
type: 'number',
required: false
}
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
- The
as StandardEventcasts make this factory type-unsafe forcustomEvent(those payloads areCustomEvent, notStandardEvent). This can hide real typing mistakes and makes future refactors riskier. Prefer keeping the resolved payload(s) typed as the genericPayload(or as(StandardEvent | CustomEvent)) and adjustsend/sendV3typing accordingly so the compiler enforces correctness without casts.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:51
- The
indices+indices.indexOf(index)lookup is unnecessary and adds avoidable O(n) work per item. Sinceeventis already available in-scope, you can setsent: eventdirectly (and removeindicesentirely), which is simpler and avoids quadratic behavior for large batches.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- The
indices+indices.indexOf(index)lookup is unnecessary and adds avoidable O(n) work per item. Sinceeventis already available in-scope, you can setsent: eventdirectly (and removeindicesentirely), which is simpler and avoids quadratic behavior for large batches.
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:38
- For batch sends,
createRedditPayloadV3pre-populates per-item successes withstatus: 200before the HTTP call, andsendV3always returns that MultiStatusResponse regardless of the actual HTTP status unless the request client throws. If the request client returns a non-2xx response without throwing, this will incorrectly report successes. Consider explicitly checking the HTTP response status (and throwing/failing the batch) to ensure the MultiStatusResponse can’t mask an upstream failure.
if (data.data.events.length) {
const response = await request(
`https://ads-api.reddit.com/api/${LATEST_API_VERSION}/pixels/${settings.ad_account_id}/conversion_events`,
{
method: 'POST',
headers: { Authorization: `Bearer ${settings.conversion_token}` },
json: JSON.parse(JSON.stringify(data))
}
)
if (!isBatch) {
return response
}
}
return multiStatusResponse
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The previous JSDoc explaining the constant and linking the API reference was removed. Since these version constants drive routing and wire endpoints, consider re-adding brief JSDoc (including the API reference link) so future maintainers have context for what these versions represent and where to validate changes.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
joe-ayoub-segment
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
- The
as StandardEvent/as StandardEvent[]casts defeat the generic typing ofbuildEventActionand can mask type errors forcustomEvent(where the payload is not aStandardEvent). Prefer keepingresolvedPayload/resolvedPayloadstyped as the genericPayload(orStandardEvent | CustomEvent) and pass those through, so TypeScript continues enforcing the correct shape per action.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
indices.indexOf(index)inside the loop adds unnecessary work (O(n) per event), and theindicesarray is redundant sinceeventis already available. Use the localeventvalue (orevents[events.length - 1]) when settingsentand removeindicesentirely to keep payload building O(n) and simpler to maintain.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
const {
event_at,
click_id,
products,
user,
data_processing_options,
screen_dimensions,
event_metadata,
conversion_id,
action_source,
event_source_url
} = payload
const custom_event_name = clean((payload as CustomEvent).custom_event_name)
const tracking_type = custom_event_name ? 'Custom' : (payload as StandardEvent).tracking_type
const event: EventItemV3 = {
event_at: toEpochMs(event_at),
action_source: toActionSourceV3(action_source),
event_source_url: clean(event_source_url),
click_id: clean(click_id),
type: {
tracking_type: toV3TrackingType(tracking_type),
custom_event_name
},
metadata: getMetadata(event_metadata, products, conversion_id),
user: getUser(user, data_processing_options, screen_dimensions)
}
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:111
Date.parse(value)should use the already-trimmed string (trimmed) for consistency with the digits-only branch and to avoid surprising failures on inputs with leading/trailing whitespace.
if (typeof value === 'string') {
const trimmed = value.trim()
const isDigitsOnly = /^\d+$/.test(trimmed)
if (isDigitsOnly && Number(trimmed) >= EPOCH_MS_MIN) return Number(trimmed)
if (!isDigitsOnly) {
const ms = Date.parse(value)
if (!Number.isNaN(ms)) return ms
}
}
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:345
- These descriptions say
quantity/item_priceare v3-only, but this PR also sends them on the wire for v2 (see v2getProductsinutils.ts). Update the field descriptions (and the corresponding generated-types/metadata descriptions) to reflect that these fields apply to both v2 and v3 to avoid misleading customers.
quantity: {
label: 'Quantity',
description: 'The number of this product in the event. Only applies to Reddit Conversions API V3 (Beta).',
type: 'integer',
required: false
},
item_price: {
label: 'Item Price',
description: 'The unit price of the product. Only applies to Reddit Conversions API V3 (Beta).',
type: 'number',
required: false
}
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:53
- The PR title indicates “behind feature flag”, but the implementation (and PR description) uses the per-mapping
API Versionfield as the rollout mechanism with no flag. Consider updating the PR title (or other visible release notes) to match the actual rollout approach so reviewers/operators aren’t expecting a flag-gated deploy.
export const api_version: InputField = {
label: 'API Version',
description:
'The version of the Reddit Conversions API to send this event to. "V3 (Beta)" requires Action Source to be set.',
type: 'string',
required: false,
default: LEGACY_API_VERSION,
choices: [
{ label: 'V3 (Beta)', value: LATEST_API_VERSION },
{ label: 'V2', value: LEGACY_API_VERSION }
],
disabledInputMethods: ['literal', 'variable', 'function', 'freeform', 'enrichment']
}
…I docs
PageVisit/ViewContent/Search do not support currency/value/item_count at
all; Lead/SignUp support currency/value but not item_count. Reddit staging
diagnostics flagged both cases ("unsupported metadata" for Search/View
Content). Adds supportsValueMetadata/supportsItemCount helpers shared by
both the V2 and V3 wire-payload builders, plus unit tests for each.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
- The
as StandardEvent/as StandardEvent[]casts erase type-safety for the customEvent action path (where payloads areCustomEvent). This can mask real shape mismatches at compile time. Prefer keepingresolvedPayloadtyped asPayload(orStandardEvent | CustomEvent) and adjustingsend()/sendV3()signatures (e.g., overloads or a shared union-array type) so no unsafe cast is needed.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- The
indicesarray +indices.indexOf(index)lookup is unnecessary and makes this loop O(n²) in the success path. Sinceeventis already in scope (and just pushed), you can setsenttoevent(orevents[events.length - 1]) and dropindicesentirely.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
const {
event_at,
click_id,
products,
user,
data_processing_options,
screen_dimensions,
event_metadata,
conversion_id,
action_source,
event_source_url
} = payload
const custom_event_name = clean((payload as CustomEvent).custom_event_name)
const tracking_type = custom_event_name ? 'Custom' : (payload as StandardEvent).tracking_type
const event: EventItemV3 = {
event_at: toEpochMs(event_at),
action_source: toActionSourceV3(action_source),
event_source_url: clean(event_source_url),
click_id: clean(click_id),
type: {
tracking_type: toV3TrackingType(tracking_type),
custom_event_name
},
metadata: getMetadata(event_metadata, products, conversion_id, tracking_type),
user: getUser(user, data_processing_options, screen_dimensions)
}
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:345
- These field descriptions state
quantity/item_priceonly apply to v3, but the PR description says they are sent on the wire for both v2 and v3, and v2 payload-building now includes them. Please update these descriptions (and regeneratemetadata.json/ generated-types comments if needed) so customers aren’t misled about when the fields take effect.
quantity: {
label: 'Quantity',
description: 'The number of this product in the event. Only applies to Reddit Conversions API V3 (Beta).',
type: 'integer',
required: false
},
item_price: {
label: 'Item Price',
description: 'The unit price of the product. Only applies to Reddit Conversions API V3 (Beta).',
type: 'number',
required: false
}
packages/destination-actions/src/destinations/reddit-conversions-api/utils.ts:96
- This introduces new v2 behavior: v2 payloads will now drop
currency/value_decimal/item_countfor certaintracking_typevalues via the newsupportsValueMetadata/supportsItemCountgating ingetMetadata(). The PR description says v2 is 'completely untouched' except for adding product fields; either update the PR description to include this v2 behavior change, or confine this metadata filtering to v3 only (keeping v2 output unchanged).
// Per https://business.reddithelp.com/s/article/about-event-metadata: PageVisit/ViewContent/Search
// don't support currency/value/item_count at all (conversion_id/products are still fine), and
// Lead/SignUp support currency/value but not item_count.
const TRACKING_TYPES_WITHOUT_VALUE_METADATA = new Set(['PageVisit', 'ViewContent', 'Search'])
const TRACKING_TYPES_WITHOUT_ITEM_COUNT = new Set(['Lead', 'SignUp'])
export function supportsValueMetadata(trackingType: string | undefined): boolean {
return !TRACKING_TYPES_WITHOUT_VALUE_METADATA.has(trackingType ?? '')
}
export function supportsItemCount(trackingType: string | undefined): boolean {
return supportsValueMetadata(trackingType) && !TRACKING_TYPES_WITHOUT_ITEM_COUNT.has(trackingType ?? '')
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/destination-actions/src/destinations/reddit-conversions-api/action.ts:69
- The
as StandardEvent/as StandardEvent[]casts are incorrect for the customEvent action path and weaken type safety (they can mask mismatches between v2/v3 payload expectations). Prefer keepingresolvedPayload/resolvedPayloadstyped asPayload(orStandardEvent | CustomEvent) and adjustsend()typing to accept(StandardEvent | CustomEvent)[](or add overloads) so no unsafe casts are needed.
perform: async (request, { settings, payload }) => {
const resolvedPayload = resolvePayload(payload) as StandardEvent
return resolveVersion(payload.api_version) === LATEST_API_VERSION
? sendV3(request, settings, [resolvedPayload], false)
: send(request, settings, [resolvedPayload])
},
performBatch: async (request, { settings, payload }) => {
const resolvedPayloads = payload.map(resolvePayload) as StandardEvent[]
// api_version is a static per-mapping setting, not derived from event data, so a batch is
// always homogeneously all-V2 or all-V3 - checking the first payload is enough.
return resolveVersion(resolvedPayloads[0]?.api_version) === LATEST_API_VERSION
? sendV3(request, settings, resolvedPayloads, true)
: send(request, settings, resolvedPayloads)
}
packages/destination-actions/src/destinations/reddit-conversions-api/v3/utils-v3.ts:87
- The
indicesarray +indices.indexOf(index)lookup is redundant and adds an O(n) search per event. You can setsentdirectly from the localevent(or useevents[events.length - 1]) and removeindicesentirely, which simplifies the logic and avoids extra work in large batches.
const indices: number[] = []
const events: EventItemV3[] = []
payloads.forEach((payload, index) => {
try {
const {
event_at,
click_id,
products,
user,
data_processing_options,
screen_dimensions,
event_metadata,
conversion_id,
action_source,
event_source_url
} = payload
const custom_event_name = clean((payload as CustomEvent).custom_event_name)
const tracking_type = custom_event_name ? 'Custom' : (payload as StandardEvent).tracking_type
const event: EventItemV3 = {
event_at: toEpochMs(event_at),
action_source: toActionSourceV3(action_source),
event_source_url: clean(event_source_url),
click_id: clean(click_id),
type: {
tracking_type: toV3TrackingType(tracking_type),
custom_event_name
},
metadata: getMetadata(event_metadata, products, conversion_id, tracking_type),
user: getUser(user, data_processing_options, screen_dimensions)
}
indices.push(index)
events.push(event)
multiStatusResponse.setSuccessResponseAtIndex(index, {
status: 200,
sent: events[indices.indexOf(index)] as unknown as JSONLikeObject,
body: { success: true }
})
packages/destination-actions/src/destinations/reddit-conversions-api/fields.ts:345
- These descriptions say quantity/item_price "only apply" to v3, but the PR description states these fields are intentionally sent on the wire for both v2 and v3. Please update the field descriptions (and the generated-types JSDoc / metadata.json text that mirrors them) to reflect actual behavior (e.g., "Sent for both v2 and v3"), or alternatively gate the v2 payload so the statement remains accurate.
quantity: {
label: 'Quantity',
description: 'The number of this product in the event. Only applies to Reddit Conversions API V3 (Beta).',
type: 'integer',
required: false
},
item_price: {
label: 'Item Price',
description: 'The unit price of the product. Only applies to Reddit Conversions API V3 (Beta).',
type: 'number',
required: false
packages/destination-actions/src/destinations/reddit-conversions-api/versioning-info.ts:3
- The previous exported version constant included an API reference comment; with the rename to LEGACY/LATEST, that context is lost. Consider reintroducing a brief doc comment explaining what these constants correspond to (and linking the relevant Reddit API docs/changelog), especially since 'LATEST' currently refers to a beta choice in the UI.
export const LEGACY_API_VERSION = 'v2.0'
export const LATEST_API_VERSION = 'v3'
export type ApiVersion = typeof LEGACY_API_VERSION | typeof LATEST_API_VERSION
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
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.
What this does
Adds support for Reddit Conversions API v3 alongside the existing v2.0 implementation, as an opt-in, per-mapping choice — v2 stays the default and is completely untouched by this PR.
There is no feature flag. The original plan (per the JIRA ticket) was to gate v3 behind the
reddit-conversions-api-canary-versionFlagon flag, but that flag has been removed from this implementation — the rollout mechanism is entirely the newApi Versionfield described below. Flagging this explicitly since it's a scope change from what's on the ticket.How it works
Send Standard EventandSend Custom Eventare the only two actions affected. Both now share a factory (action.ts) so the v2/v3 branching logic (resolveVersion,perform/performBatch) lives in one place instead of being duplicated.Api Versionfield on both actions lets a customer chooseV2(default) orV3 (Beta)per mapping. It's a fixed dropdown (no variables/functions/literal text), so a single mapping — and therefore a single batch — is always homogeneously V2 or V3.V2at runtime exactly like today.Send Standard Event/Send Custom Eventmapping and setsApi VersiontoV3 (Beta). Doing so surfaces the newAction Sourcefield (required only in that case — see below).utils.ts/types.ts) is otherwise unchanged frommain, with one deliberate exception:products.quantity/products.item_pricewere added to v2's wire payload too (see New Fields below). All other v3 logic is new and isolated inv3/utils-v3.ts,v3/types-v3.ts,v3/constants.ts— nothing else is shared between the two versions' payload-building code, so v2 can still be deleted outright later without untangling much.POST /api/v3/pixels/{ad_account_id}/conversion_events, body wrapped in{ data: { events, partner: 'SEGMENT', test_id } }, withevent_atsent as epoch milliseconds andtracking_typevalues inUPPER_SNAKE_CASE(vs v2'sPascalCase).MultiStatusResponseso one bad event doesn't fail the whole batch — but only for validation failures on Segment's side (e.g. a missing product id). If the actual HTTP call to Reddit fails, the whole batch still fails/throws as it always has; Reddit itself has no per-item batch response to parse.event_metadata is now filtered by tracking_type (both V2 and V3)
Per Reddit's Event Metadata docs, support for
currency/value/item_countvaries by event type:PageVisit/ViewContent/Searchsupport none ofcurrency/value/item_count(onlyconversion_id/productsare supported for these).Lead/SignUpsupportcurrency/valuebut notitem_count.AddToCart,AddToWishlist,Purchase,Custom) support all three.Found this via staging diagnostics on real Reddit traffic (Search/View Content events flagged as "unsupported metadata" for carrying
currency). Added sharedsupportsValueMetadata/supportsItemCounthelpers inutils.ts, applied in bothgetMetadataimplementations (utils.tsfor V2,v3/utils-v3.tsfor V3) so a customer can't send unsupported metadata regardless of which version they're on. Unit tests added in__tests__/utils.test.ts(new, V2) and__tests__/v3-utils.test.ts(V3).New fields
Per-action (Standard Event / Custom Event):
Api VersionV2default /V3 (Beta))Action SourceWEBSITE/APP/OTHER/PHYSICAL_STORE)Api Version = V3 (Beta); no default — Reddit (Manik Mehta) explicitly asked us not to guess this per customerEvent Source URL$.context.page.url; only applies to V3products.quantity/products.item_priceDestination Settings:
Test ID(new)Test Modeboolean for V3 only.Test Mode(existing, unchanged)Testing
returns a plain response (no MultiStatusResponse) for a pure V2 batch)New/changed test files
__tests__/v3-single-event.test.ts—performbehavior for a single event: sending a Standard/Custom event to V3, staying on V2 whenapi_versionis unset, rejecting the mapping when V3 is selected withoutaction_source, and throwing when a product is missing anid.__tests__/v3-batch-events.test.ts—performBatchbehavior: a mixed batch of 10 (schema failures, Reddit-side validation failures, and successes) resolved viaMultiStatusResponse, plus confirming a pure-V2 batch still returns a plain response with noMultiStatusResponse.__tests__/v3-utils.test.ts— unit coverage for the v3 helper functions:toEpochMs,toV3TrackingType,toActionSourceV3,toProductIdV3,getProducts,getMetadata,createRedditPayloadV3, andsendV3. Now also covers per-tracking-type metadata filtering.__tests__/utils.test.ts(new) — unit coverage for V2'sgetMetadata, covering per-tracking-type metadata filtering.__tests__/__snapshots__/snapshot.test.ts.snap— regenerated required/all-fields snapshots to include the newapi_version/action_source/event_source_url/ product fields (snapshot.test.tsitself is unchanged).Security Review
Api Version,Action Source,Event Source URL, orTest IDcarry secrets; no new fields needtype: 'password'.