fix(recombee): Fix API inconsistencies#3767
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes timestamp handling for the Recombee destination, ensuring delete interactions send timestamps in seconds (as required by Recombee) and aligning action field schemas to use the datetime field type.
Changes:
- Update
Delete Bookmark/Delete Cart AdditionURL construction to sendtimestampas epoch seconds (converting from ms/ISO when needed). - Change
timestampinput fields across Recombee actions fromstringtodatetime(and update generated types accordingly). - Update unit tests/snapshots to reflect the new timestamp expectations and payload shapes.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/recombeeApiClient.ts | Convert delete timestamp to epoch seconds and build query via URLSearchParams. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/tests/index.test.ts | Update tests to expect 10-digit (seconds) timestamps and seconds-based URL assertions. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/tests/index.test.ts | Update tests to expect 10-digit (seconds) timestamps and seconds-based URL assertions. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/addRating/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/addRating/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/addRating/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/addPurchase/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/addPurchase/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/addPurchase/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/addDetailView/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/addDetailView/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/addDetailView/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/addBookmark/index.ts | Switch timestamp field to datetime. |
| packages/destination-actions/src/destinations/recombee/addBookmark/generated-types.ts | Update timestamp payload type to string | number. |
| packages/destination-actions/src/destinations/recombee/addBookmark/tests/snapshots/snapshot.test.ts.snap | Snapshot update for timestamp value shape. |
| packages/destination-actions/src/destinations/recombee/tests/snapshots/snapshot.test.ts.snap | Update destination-level snapshots for timestamp value shape across actions. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Recombee delete-interaction timestamp handling (seconds vs. milliseconds) and updates Recombee action field schemas so timestamp is modeled as a datetime (and thus typed as string | number) across the destination.
Changes:
- Fix
Delete Cart AdditionandDelete BookmarkURL timestamp to send Unix seconds (and add more robust timestamp parsing/normalization for deletes). - Change
timestampinput field type fromstring→datetimeacross Recombee actions and update generated payload types accordingly. - Update unit tests and snapshots to reflect the
datetimefield behavior and the corrected delete timestamp semantics.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/recombee/recombeeApiClient.ts | Normalize delete URL query params via URLSearchParams; convert/delete timestamp to epoch seconds and validate invalid timestamps. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/tests/index.test.ts | Update delete URL expectations to 10-digit seconds timestamps. |
| packages/destination-actions/src/destinations/recombee/deleteCartAddition/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/tests/index.test.ts | Update delete URL expectations to 10-digit seconds timestamps. |
| packages/destination-actions/src/destinations/recombee/deleteBookmark/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/addBookmark/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/addBookmark/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/addBookmark/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/addCartAddition/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/addDetailView/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/addDetailView/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/addDetailView/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/addPurchase/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/addPurchase/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/addPurchase/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/addRating/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/addRating/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/addRating/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/setViewPortion/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/index.ts | Change timestamp field type to datetime. |
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/generated-types.ts | Update payload timestamp type to string | number. |
| packages/destination-actions/src/destinations/recombee/setViewPortionFromWatchTime/tests/snapshots/snapshot.test.ts.snap | Snapshot update for datetime timestamp example. |
| packages/destination-actions/src/destinations/recombee/tests/snapshots/snapshot.test.ts.snap | Aggregate snapshot updates for datetime timestamp examples across actions. |
|
Hi @mstieranka please ping me when this PR is ready for review. |
| // 10,000,000,000 is the year 2286 in seconds, anything larger is likely in milliseconds | ||
| if (numValue >= 10000000000) { | ||
| return numValue / 1000 | ||
| } else { | ||
| return numValue |
| function getDeleteUrl(interactionType: string, params: DeleteParams): string { | ||
| const query = new URLSearchParams({ | ||
| userId: params.userId, | ||
| itemId: params.itemId | ||
| }) | ||
|
|
||
| if (params.timestamp !== undefined && params.timestamp !== null) { | ||
| query.append('timestamp', String(datetimeToEpochSeconds(params.timestamp))) | ||
| } | ||
| return url | ||
|
|
||
| return `/${interactionType}/?${query.toString()}` | ||
| } |
mstieranka
changed the title
mstieranka
force-pushed
the
recombee-fix-timestamp-type
branch
from
61b9364 to
9b20518
Compare
|
Hi @joe-ayoub-segment , this PR has had some changes added to it and is now ready for review. |
| // 10,000,000,000 is the year 2286 in seconds, anything larger is likely in milliseconds | ||
| if (numValue >= 10000000000) { | ||
| return numValue / 1000 | ||
| } else { | ||
| return numValue |
joe-ayoub-segment
left a comment
joe-ayoub-segment
left a comment
There was a problem hiding this comment.
Hi @mstieranka thanks for the PR and apologies for the delay reviewing it.
It's generally good - however some important points I left comments on.
If you can respond to those, and potentially fix, that would be great.
Thanks,
Joe
| }) | ||
|
|
||
| if (params.timestamp !== undefined && params.timestamp !== null) { | ||
| query.append('timestamp', String(datetimeToEpochSeconds(params.timestamp))) |
There was a problem hiding this comment.
consider calling datetimeToEpochSeconds into a variable first, then check if it is a valid timestamp before doing query.append.
| } | ||
| } | ||
|
|
||
| function datetimeToEpochSeconds(datetime: string | number): number { |
There was a problem hiding this comment.
The millisecond detection heuristic is fragile. The threshold 10000000000 (Nov 2286 in seconds) means any epoch-seconds value after that date would be incorrectly halved. Conversely, millisecond timestamps before Sept 9, 2001 (10000000000 ms = ~115 days from
epoch) would be treated as seconds. Both are unlikely in practice but the magic number deserves at least a comment explaining the tradeoff.
The division produces fractional seconds. numValue / 1000 for an odd millisecond value yields something like 1716048000.123. If the consumer expects an integer epoch (as the name "EpochSeconds" suggests), this should be Math.floor(numValue / 1000).
String(datetime).trim() !== '' is dead code for the number input type. Number('') is 0 which would pass !isNaN, so the check guards against empty strings — fine. But String(someNumber).trim() !== '' is always true for any number input. Not wrong, just
unnecessary for that branch.
new Date(datetime) when datetime is a number that somehow failed the first check — this can't actually happen. If datetime is a number, Number(datetime) will never be NaN. So the new Date(datetime) path only runs for non-numeric strings, which is correct but not
obvious from reading the flow.
No timezone consideration for string parsing. new Date("2024-01-15") vs new Date("2024-01-15T00:00:00") behave differently (UTC vs local time) depending on the format. If callers pass date-only strings, the result will vary by environment.
The biggest practical issue is the missing Math.floor — everything else is edge-case or readability.
| default: { | ||
| '@if': { | ||
| exists: { '@path': '$.properties.timestamp' }, | ||
| then: { '@path': '$.properties.timestamp' }, | ||
| else: { '@path': '$.timestamp' } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Unless you have a good reason, I would leave the detault as $.timestamp as that's the location where Segment event timestamps are always found.
| } | ||
|
|
||
| function datetimeToEpochSeconds(datetime: string | number): number { | ||
| const numValue = Number(datetime) |
There was a problem hiding this comment.
If user passes "" this will evaluate to 0. It might enter a code path you didn't anticipate below. Please check.
| ) => ({ | ||
| label: 'Timestamp', | ||
| description: `The UTC timestamp of the ${interactionName} to delete, in Unix seconds, Unix milliseconds, or ISO-8601 format. Must match the timestamp used in the ${interactionName} to be deleted. If omitted, all ${interactionName}s for the given \`userId\` and \`itemId\` are deleted.`, | ||
| type: 'datetime', |
There was a problem hiding this comment.
Note the original field you are now generating via a function originally was not of type: 'datetime'.
Adding type: 'datetime' might break data collection for customers who are already sending in non datetime string to this field.
If you confirm that this is OK and deliberate that's OK.
| ) => ({ | ||
| label: 'Timestamp', | ||
| description: `The UTC timestamp of when the ${interactionName} occurred, in Unix seconds, Unix milliseconds, or ISO-8601 format. When recording interactions you plan to later delete by exact timestamp — whether via this destination or the Recombee API directly — avoid mapping the root \`timestamp\` here, as it may be corrected for clock skew. Use \`properties.timestamp\` instead.`, | ||
| type: 'datetime', |
There was a problem hiding this comment.
Note the original field you are now generating via a function originally was not of type: 'datetime'.
Adding type: 'datetime' might break data collection for customers who are already sending in non datetime string to this field.
If you confirm that this is OK and deliberate that's OK.
Hi, we have identified several issues with out Recombee destination, and this pull request fixes all of them.
Delete Cart AdditionandDelete Bookmark, the request to our API was incorrectly setting the timestamp to milliseconds since epoch where seconds were expected. The following changes were made:timestamphas been modified to use thedatetimetype instead ofstring, which more accurately reflects the intended values.HTTP 400(the API rejects this parameter), whereas this version returnsHTTP 500(the validation already fails in Segment). I'm not sure if this is a breaking change, since both requests would have failed and the field itself is optional in all mappings. This is why the "Tested for backwards compatibility" task below is not yet marked as complete.timestamphandling has generally been made more robust to accept any form of date string parseable bynew Date(str)or a number representing either seconds or milliseconds since epoch. The field description has been changed to reflect this fact.$.timestamp) is corrected for clock skew, however, that means that the Recombee API receives a different timestamp than the user sent. Since theDelete Bookmarkaction expects the exact same timestamp as when sending the request, this meant that such a use case wouldn't work. The following changes were made:timestampmapping inAdd*events has been changed to use$.properties.timestampby default, falling back to$.timestampif not present.timestampmapping inDelete*events now also uses$.properties.timestamp.pricefield is stated per-unit, however, the Recombee API expects thepriceandprofitfields to be stated per the given quantity. Therefore, the destination now multipliespricebyamountto match the expectation of the Recombee API.Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example