HubSpot sources - fixing ts comparison on initial run #18438
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughShared HubSpot pagination now short-circuits relevance checks when no Changes
Sequence Diagram(s)sequenceDiagram
participant Source as HubSpot Source
participant Common as common.mjs
participant API as HubSpot API
participant Emitter as Event Emitter
Source->>Common: paginate/processEvents(after)
Common->>API: fetch page/items
loop each item
alt after is unset
Note over Common,Emitter: Short-circuit emit (no isRelevant)
Common->>Emitter: emit(item)
else after is set
Common->>Common: isRelevant(item, after)?
alt relevant
Common->>Emitter: emit(item)
else not relevant
Common-->>Source: skip
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GTFalcao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
components/hubspot/sources/new-or-updated-company/new-or-updated-company.mjs (1)
43-47: Bug: usingthis.isNewinstead ofthis.newOnlybreaks timestamp selection.
this.isNewis undefined; this will always pickupdatedAtand can mis-emit. Usethis.newOnlyfor consistency with other sources.Apply:
getTs(company) { - return this.isNew + return this.newOnly ? Date.parse(company.createdAt) : Date.parse(company.updatedAt); }components/hubspot/sources/delete-blog-article/delete-blog-article.mjs (2)
21-26: Incorrect meta timestamp and id formatting for deletions.
tsshould reflect deletion time, and id concatenation lacks a delimiter (risking collisions).- const ts = Date.parse(blogpost.created); + const ts = this.getTs(blogpost); return { - id: `${id}${this.getTs(blogpost)}`, + id: `${id}-${ts}`, summary, ts, };
28-36: Do not send an undefined deletedAt filter; keep sort as "-updatedAt" (deletedAt is not a valid sort field).Only add deletedAt__gte when after != null. HubSpot Blog Posts API does not support sorting by deletedAt — valid sort fields: name, createdAt, updatedAt, createdBy, updatedBy.
- getParams(after) { - return { - params: { - limit: 100, - deletedAt__gte: after, - sort: "-updatedAt", - }, - }; - }, + getParams(after) { + const params = { limit: 100, sort: "-updatedAt" }; + if (after != null) params.deletedAt__gte = after; + return { params }; + },components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs (1)
27-35: Dedupe id risks collisions; add a separator.
id: \${id}${ts}`can collide (e.g.,1|234vs12|34). Other sources useid-ts`. Align for safety.- id: `${id}${ts}`, + id: `${id}-${ts}`,components/hubspot/sources/new-or-updated-custom-object/new-or-updated-custom-object.mjs (1)
27-35: Dedupe id risks collisions; add a separator.
Align with other sources to prevent accidental id/ts concatenation collisions.- id: `${id}${ts}`, + id: `${id}-${ts}`,components/hubspot/sources/common/common.mjs (3)
57-63: Bug: using !after treats 0 as “first run”, causing perpetual re‑emits when ts is missing. Use nullish checks.If ts is undefined/NaN on first run, maxTs stays 0, _setAfter(0) persists a falsy value, and subsequent runs bypass relevance again. Switch to nullish semantics.
- if (!after || await this.isRelevant(result, after)) { + if (after == null || await this.isRelevant(result, after)) {Follow‑ups outside this hunk (recommended):
- Initialize with nullish coalescing: let maxTs = after ?? 0;
- First‑run guards: if (after == null) return;
- In getPaginatedItems: while (params.after && after != null && page < maxPages)
83-91: Same nullish bug here; fix the gating condition.- if (!after || ts > after) { - if (!after || await this.isRelevant(result, after, ts)) { + if (after == null || ts > after) { + if (after == null || await this.isRelevant(result, after, ts)) {
127-134: Same nullish bug in hasMore pagination; fix gating.- if (!after || await this.isRelevant(item, after)) { + if (after == null || await this.isRelevant(item, after)) {Optional hardening: if (after == null && (ts == null || Number.isNaN(ts))) set after to Date.now() once to prevent infinite “first run” behavior when upstream never supplies timestamps.
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
101-114: Use nullish check for after to avoid treating 0 as “first run”.Align with fixes in common.
- if (!after || this.isRelevant(ts, after)) { + if (after == null || this.isRelevant(ts, after)) {Optional: ensure ts is numeric
const tsNum = typeof ts === "number" ? ts : Number(ts);components/hubspot/sources/new-form-submission/new-form-submission.mjs (1)
38-55: Same nullish check for after.Prevents epoch(0) from being misread as falsy and re-triggering first-run behavior.
- if (!after || await this.isRelevant(result, after)) { + if (after == null || await this.isRelevant(result, after)) {
🧹 Nitpick comments (26)
components/hubspot/sources/new-engagement/new-engagement.mjs (1)
19-19: Typo in user-facing description."engagment" → "engagement".
- description: "Filter results by the type of engagment", + description: "Filter results by the type of engagement",components/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs (3)
26-28: Make isRelevant resilient to nullish createdAfter.Central logic should gate calls, but adding a local nullish guard prevents accidental future regressions.
- isRelevant(timeline, createdAfter) { - return this.getTs(timeline) > createdAfter; - }, + isRelevant(timeline, createdAfter) { + if (createdAfter == null) return true; + return this.getTs(timeline) > createdAfter; + },
29-35: Avoid sending undefined query params.If
afteris undefined on first run, this will serializestartTimestamp: undefined. Prefer omitting the key.- getParams(after) { - return { - params: { - startTimestamp: after, - }, - }; - }, + getParams(after) { + const params = {}; + if (after != null) params.startTimestamp = after; + return { params }; + },
17-24: Dedupe id: add a delimiter to reduce collision risk.
"${id}${ts}"can collide for some concatenations. Add a delimiter.- return { - id: `${id}${ts}`, + return { + id: `${id}:${ts}`, summary: `New subscription event for recipient ${id}`, ts, };components/hubspot/sources/new-or-updated-line-item/new-or-updated-line-item.mjs (1)
43-47: Guard against invalid timestamps.If
createdAt/updatedAtis missing or unparsable,Date.parsereturns NaN and relevance comparisons fail. Consider a safe fallback.Suggested change:
getTs(lineItem) { - return this.newOnly - ? Date.parse(lineItem.createdAt) - : Date.parse(lineItem.updatedAt); + const raw = this.newOnly ? lineItem.createdAt : lineItem.updatedAt; + const ts = Date.parse(raw); + return Number.isFinite(ts) ? ts : 0; }components/hubspot/sources/new-task/new-task.mjs (1)
17-19: Minor: handle NaN from Date.parse.Add a fallback to avoid silent drops when
createdAtis missing.getTs(task) { - return Date.parse(task.createdAt); + const ts = Date.parse(task.createdAt); + return Number.isFinite(ts) ? ts : 0; }components/hubspot/sources/new-ticket/new-ticket.mjs (1)
36-38: Minor: protect against invalid dates.Guard
Date.parse(ticket.createdAt)returning NaN.getTs(ticket) { - return Date.parse(ticket.createdAt); + const ts = Date.parse(ticket.createdAt); + return Number.isFinite(ts) ? ts : 0; }components/hubspot/sources/new-or-updated-deal/new-or-updated-deal.mjs (1)
64-68: Minor: invalid timestamp fallback.Add a fallback when
createdAt/updatedAtis missing to avoid NaN comparisons.getTs(deal) { - return this.newOnly - ? Date.parse(deal.createdAt) - : Date.parse(deal.updatedAt); + const raw = this.newOnly ? deal.createdAt : deal.updatedAt; + const ts = Date.parse(raw); + return Number.isFinite(ts) ? ts : 0; }components/hubspot/sources/new-note/new-note.mjs (1)
16-18: Minor: handle NaN timestamps.Guard
Date.parse(note.createdAt)result.getTs(note) { - return Date.parse(note.createdAt); + const ts = Date.parse(note.createdAt); + return Number.isFinite(ts) ? ts : 0; }components/hubspot/sources/new-or-updated-contact/new-or-updated-contact.mjs (1)
79-98: Boundary check may re‑emit objects sitting exactly at the cursor.
if (this.getTs(contact) < updatedAfter) return false;treatsts === updatedAfteras relevant, unlike other sources that use strict>checks. Recommend consistent, cursor‑safe comparison.- async isRelevant(contact, updatedAfter) { - if (this.getTs(contact) < updatedAfter) { + async isRelevant(contact, updatedAfter) { + if (updatedAfter && this.getTs(contact) <= updatedAfter) { return false; }components/hubspot/sources/new-social-media-message/new-social-media-message.mjs (2)
24-26: Normalize timestamp to a number to avoid NaN comparisons.
getTsreturnsmessage.createdAtdirectly; if it’s an ISO string,>withcreatedAfterwill fail. Parse or coerce to number.- getTs(message) { - return message.createdAt; - }, + getTs(message) { + return typeof message.createdAt === "number" + ? message.createdAt + : Date.parse(message.createdAt); + },Also applies to: 38-40
41-47: Avoid sending undefined query params.
since: afterwhenafteris falsy may leakundefinedinto the request depending on the HTTP client. Guard it.- getParams(after) { - return { - params: { - withChannelKeys: this.channel, - since: after, - }, - }; - }, + getParams(after) { + const params = { withChannelKeys: this.channel }; + if (after) params.since = after; + return { params }; + },components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs (2)
43-47: Strict equality and naming nit.
Use===and avoid shadowing generic names; minor readability win.- const propertyName = - object == "contacts" + const propertyName = + object === "contacts" ? "lastmodifieddate" : "hs_lastmodifieddate";
60-66: Minor: avoid generic name “object”.
ConsiderobjectNameto improve readability.- async processResults(after) { - const object = + async processResults(after) { + const objectName = this.objectType == "company" ? "companies" : `${this.objectType}s`; - const params = this.getObjectParams(object); + const params = this.getObjectParams(objectName); await this.searchCRM(params, after); },components/hubspot/sources/new-event/new-event.mjs (2)
49-52: Prefer event occurrence time over “now” forts.
UsingDate.now()skews ordering/analytics. If available, use the API’s occurrence timestamp.- getTs() { - return Date.now(); - }, + getTs(result) { + // Prefer explicit occurrence field if present; fall back to now. + return result?.occurredAt + ? (typeof result.occurredAt === "number" ? result.occurredAt : Date.parse(result.occurredAt)) + : Date.now(); + }, @@ - generateMeta(result) { + generateMeta(result) { @@ - ts: this.getTs(), + ts: this.getTs(result),Also applies to: 53-61
76-85: GuardoccurredAfterparam to avoid undefined in requests.
Safer to include only when present.- getEventParams(objectId, occurredAfter) { - return { - params: { - limit: DEFAULT_LIMIT, - objectType: this.objectType, - objectId, - occurredAfter, - }, - }; - }, + getEventParams(objectId, occurredAfter) { + const params = { + limit: DEFAULT_LIMIT, + objectType: this.objectType, + objectId, + }; + if (occurredAfter) params.occurredAfter = occurredAfter; + return { params }; + },components/hubspot/sources/new-or-updated-blog-article/new-or-updated-blog-article.mjs (1)
46-53: Avoid passingupdated__gte: undefined.
Guard the filter param to prevent accidental API behavior.- getParams(after) { - return { - params: { - limit: DEFAULT_LIMIT, - updated__gte: after, - sort: "-updatedAt", - }, - }; - }, + getParams(after) { + const params = { limit: DEFAULT_LIMIT, sort: "-updatedAt" }; + if (after) params.updated__gte = after; + return { params }; + },components/hubspot/sources/new-email-event/new-email-event.mjs (1)
40-51: GuardstartTimestampto avoid undefined in requests.
Match the centralized “emit when no ts” behavior by omitting the filter entirely whenafteris falsy.- getParams(after) { - const params = { - limit: DEFAULT_LIMIT, - startTimestamp: after, - }; + getParams(after) { + const params = { limit: DEFAULT_LIMIT }; + if (after) params.startTimestamp = after; if (this.type) { params.eventType = this.type; } return { params, }; },components/hubspot/sources/new-custom-object-property-change/new-custom-object-property-change.mjs (1)
51-53: LGTM on tightened isRelevant; consider optional ts passthrough to avoid recomputing.Matches the centralized gating in common. To skip re-reading history when a precomputed ts is available, accept an optional third param:
- isRelevant(object, updatedAfter) { - return this.getTs(object) > updatedAfter; - }, + isRelevant(object, updatedAfter, ts) { + const t = ts ?? this.getTs(object); + return t > updatedAfter; + },components/hubspot/sources/new-contact-property-change/new-contact-property-change.mjs (1)
45-47: LGTM; same optional ts passthrough applies.Consistent with common’s first-run behavior. Optional micro-optimization below.
- isRelevant(contact, updatedAfter) { - return this.getTs(contact) > updatedAfter; - }, + isRelevant(contact, updatedAfter, ts) { + const t = ts ?? this.getTs(contact); + return t > updatedAfter; + },components/hubspot/sources/new-deal-property-change/new-deal-property-change.mjs (1)
43-45: LGTM; consider optional ts passthrough for parity/perf.- isRelevant(deal, updatedAfter) { - return this.getTs(deal) > updatedAfter; - }, + isRelevant(deal, updatedAfter, ts) { + const t = ts ?? this.getTs(deal); + return t > updatedAfter; + },components/hubspot/sources/new-company-property-change/new-company-property-change.mjs (1)
45-47: LGTM; mirrors other sources. Optional: accept precomputed ts.- isRelevant(company, updatedAfter) { - return this.getTs(company) > updatedAfter; - }, + isRelevant(company, updatedAfter, ts) { + const t = ts ?? this.getTs(company); + return t > updatedAfter; + },components/hubspot/sources/new-ticket-property-change/new-ticket-property-change.mjs (1)
46-48: LGTM; consistent with centralized gating. Optional ts passthrough for parity.- isRelevant(ticket, updatedAfter) { - return this.getTs(ticket) > updatedAfter; - }, + isRelevant(ticket, updatedAfter, ts) { + const t = ts ?? this.getTs(ticket); + return t > updatedAfter; + },components/hubspot/sources/common/common.mjs (1)
55-66: Use nullish semantics forafterand add a first‑run fallbackReplace
after || 0withafter ?? 0and change!afterchecks to explicit nullish checks (e.g.after == null). Ifafter == nulland no emitted item yields a finite timestamp, setaftertoDate.now()at the end of the run to avoid perpetual first‑run behavior. Locations to update:
- components/hubspot/sources/common/common.mjs — lines 55 (let maxTs), 57 (if !after ...), 69 (let maxTs), 113 (let maxTs)
- components/hubspot/sources/new-form-submission/new-form-submission.mjs — line 26 (let maxTs)
- components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs — line 91 (let maxTs)
components/hubspot/sources/new-contact-added-to-list/new-contact-added-to-list.mjs (2)
98-103: Avoid duplicate properties in batched contact fetchMerging DEFAULT_CONTACT_PROPERTIES with user-specified properties can introduce duplicates. Deduplicate to prevent larger payloads and potential API errors.
- const allProperties = [ - ...DEFAULT_CONTACT_PROPERTIES, - ...properties, - ]; + const allProperties = Array.from(new Set([ + ...DEFAULT_CONTACT_PROPERTIES, + ...properties, + ]));
183-187: Safer timestamp comparison (optional)Comparing ISO strings works if all values are normalized, but numeric comparison is more defensive.
- const { membershipTimestamp } = membership; + const { membershipTimestamp } = membership; + const lastTS = lastMembershipTimestamp ? Date.parse(lastMembershipTimestamp) : null; + const mTS = Date.parse(membershipTimestamp); @@ - if (!lastMembershipTimestamp || membershipTimestamp > lastMembershipTimestamp) { + if (lastTS == null || mTS > lastTS) { @@ - if ( - !latestMembershipTimestamp || - membershipTimestamp > latestMembershipTimestamp - ) { - latestMembershipTimestamp = membershipTimestamp; - } + if (!latestMembershipTimestamp || mTS > Date.parse(latestMembershipTimestamp)) { + latestMembershipTimestamp = membershipTimestamp; + }Run-time behavior should be identical with normalized inputs; this change guards against any format variance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
components/hubspot/package.json(1 hunks)components/hubspot/sources/common/common.mjs(3 hunks)components/hubspot/sources/delete-blog-article/delete-blog-article.mjs(1 hunks)components/hubspot/sources/new-company-property-change/new-company-property-change.mjs(2 hunks)components/hubspot/sources/new-contact-added-to-list/new-contact-added-to-list.mjs(1 hunks)components/hubspot/sources/new-contact-property-change/new-contact-property-change.mjs(2 hunks)components/hubspot/sources/new-custom-object-property-change/new-custom-object-property-change.mjs(2 hunks)components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs(2 hunks)components/hubspot/sources/new-deal-property-change/new-deal-property-change.mjs(2 hunks)components/hubspot/sources/new-email-event/new-email-event.mjs(1 hunks)components/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs(1 hunks)components/hubspot/sources/new-engagement/new-engagement.mjs(1 hunks)components/hubspot/sources/new-event/new-event.mjs(1 hunks)components/hubspot/sources/new-form-submission/new-form-submission.mjs(2 hunks)components/hubspot/sources/new-note/new-note.mjs(1 hunks)components/hubspot/sources/new-or-updated-blog-article/new-or-updated-blog-article.mjs(1 hunks)components/hubspot/sources/new-or-updated-company/new-or-updated-company.mjs(1 hunks)components/hubspot/sources/new-or-updated-contact/new-or-updated-contact.mjs(1 hunks)components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs(1 hunks)components/hubspot/sources/new-or-updated-custom-object/new-or-updated-custom-object.mjs(1 hunks)components/hubspot/sources/new-or-updated-deal/new-or-updated-deal.mjs(1 hunks)components/hubspot/sources/new-or-updated-line-item/new-or-updated-line-item.mjs(1 hunks)components/hubspot/sources/new-or-updated-product/new-or-updated-product.mjs(1 hunks)components/hubspot/sources/new-social-media-message/new-social-media-message.mjs(1 hunks)components/hubspot/sources/new-task/new-task.mjs(1 hunks)components/hubspot/sources/new-ticket-property-change/new-ticket-property-change.mjs(2 hunks)components/hubspot/sources/new-ticket/new-ticket.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-10T19:18:27.998Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#14265
File: components/the_magic_drip/sources/common.mjs:35-43
Timestamp: 2024-10-10T19:18:27.998Z
Learning: In `components/the_magic_drip/sources/common.mjs`, when processing items in `getAndProcessData`, `savedIds` is intentionally updated with IDs of both emitted and non-emitted items to avoid emitting retroactive events upon first deployment and ensure only new events are emitted as they occur.
Applied to files:
components/hubspot/sources/common/common.mjs
🧬 Code graph analysis (4)
components/hubspot/sources/new-form-submission/new-form-submission.mjs (1)
components/hubspot/sources/common/common.mjs (1)
after(187-187)
components/hubspot/sources/common/common.mjs (15)
components/hubspot/sources/new-form-submission/new-form-submission.mjs (3)
result(62-62)ts(47-47)ts(63-63)components/hubspot/sources/new-company-property-change/new-company-property-change.mjs (1)
ts(38-38)components/hubspot/sources/new-contact-property-change/new-contact-property-change.mjs (1)
ts(38-38)components/hubspot/sources/new-custom-object-property-change/new-custom-object-property-change.mjs (1)
ts(44-44)components/hubspot/sources/new-deal-property-change/new-deal-property-change.mjs (1)
ts(36-36)components/hubspot/sources/new-ticket-property-change/new-ticket-property-change.mjs (1)
ts(39-39)components/hubspot/sources/delete-blog-article/delete-blog-article.mjs (1)
ts(21-21)components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
ts(102-102)components/hubspot/sources/new-email-event/new-email-event.mjs (1)
ts(33-33)components/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs (1)
ts(19-19)components/hubspot/sources/new-engagement/new-engagement.mjs (1)
ts(33-33)components/hubspot/sources/new-or-updated-blog-article/new-or-updated-blog-article.mjs (1)
ts(34-34)components/hubspot/sources/new-or-updated-company/new-or-updated-company.mjs (1)
ts(52-52)components/hubspot/sources/new-or-updated-contact/new-or-updated-contact.mjs (1)
ts(58-58)components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs (1)
ts(29-29)
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
components/hubspot/sources/common/common.mjs (4)
after(187-187)ts(59-59)ts(82-82)ts(129-129)
components/hubspot/sources/new-custom-object-property-change/new-custom-object-property-change.mjs (1)
components/hubspot/hubspot.app.mjs (1)
object(768-771)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (18)
components/hubspot/sources/new-engagement/new-engagement.mjs (1)
11-11: Version bump to 0.0.39 — LGTM; verify publish flow.No functional changes here. Please confirm this version bump will be included in the release/publish for the HubSpot app and aligns with the centralized isRelevant behavior change elsewhere.
components/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs (2)
9-9: Approve version bump — confirm HubSpot dependency bumpcomponents/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs shows version "0.0.34" (line 9); grep returned no occurrences of "@pipedream/hubspot" or "hubspot" in package.json, pnpm-lock.yaml, or package-lock.json. If a dependency bump was intended, update the dependency and lockfile(s); otherwise confirm no change needed.
36-43: Pagination gating verified — no action requiredpaginateUsingHasMore (components/hubspot/sources/common/common.mjs) short‑circuits isRelevant when after is falsy and only calls isRelevant when after is truthy; new-email-subscriptions-timeline passes the stored after into paginateUsingHasMore, so central gating covers this source.
components/hubspot/package.json (1)
3-3: Version bump looks good; confirm publish artifacts.Please ensure the 1.7.7 tag/changelog and any release notes reflect the centralized isRelevant behavior so downstream users understand the behavior change on first runs.
components/hubspot/sources/new-ticket/new-ticket.mjs (1)
50-52: No action required — first-run emits are already handled.
processEvents in components/hubspot/sources/common/common.mjs usesif (!after || await this.isRelevant(...))before emitting (see ~lines 54–60, 81–86, 124–130) and the defaultisRelevant()returnstrue(lines 165–167).components/hubspot/sources/new-or-updated-line-item/new-or-updated-line-item.mjs (1)
59-61: No change required — common short-circuits isRelevant whenafteris falsy.processEvents/searchCRM guard with
if (!after || ...), so the override in components/hubspot/sources/new-or-updated-line-item/new-or-updated-line-item.mjs (isRelevant => this.getTs(lineItem) > updatedAfter) is not called whenafteris falsy. See components/hubspot/sources/common/common.mjs (~lines 54–58, 82–85, 165–167).components/hubspot/sources/new-or-updated-deal/new-or-updated-deal.mjs (1)
82-84: No action required — common emits on first run whenafteris falsy.common.processEvents and common.paginate short‑circuit on
!after(they useif (!after || ...)), soisRelevantis bypassed and items are emitted on first run (components/hubspot/sources/common/common.mjs — processEvents / paginate).components/hubspot/sources/new-note/new-note.mjs (1)
26-28: Common first-run emit logic is correct
Whenafteris falsy,processEventsshort-circuits theisRelevantcheck and emits all items on initial run.components/hubspot/sources/new-task/new-task.mjs (1)
27-29: First-run bypass confirmed — do not change isRelevant.
common.processEvents usesif (!after || await this.isRelevant(result, after))(components/hubspot/sources/common/common.mjs:54-63), so whenafteris falsy the relevance check is bypassed and items are emitted.components/hubspot/sources/new-or-updated-company/new-or-updated-company.mjs (1)
61-63: Confirmed — common bypasses isRelevant whenafteris falsy. common.mjs short‑circuits in processEvents and paginate withif (!after || await this.isRelevant(...)), so isRelevant is not invoked on the first run.components/hubspot/sources/new-or-updated-contact/new-or-updated-contact.mjs (1)
13-13: Version bump only — OK.
Metadata bump to 0.0.22 looks fine.components/hubspot/sources/new-or-updated-product/new-or-updated-product.mjs (1)
13-13: Version bump only — OK.
No behavior change here.components/hubspot/sources/new-social-media-message/new-social-media-message.mjs (1)
10-10: Version bump only — OK.
No logic changes in this file.components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs (1)
10-10: Version bump only — OK.
No behavioral diffs here.components/hubspot/sources/new-or-updated-custom-object/new-or-updated-custom-object.mjs (1)
10-10: Version bump only — OK.
Looks good.components/hubspot/sources/new-event/new-event.mjs (1)
11-11: Version bump only — OK.
No logic changes here.components/hubspot/sources/new-or-updated-blog-article/new-or-updated-blog-article.mjs (1)
10-10: Version bump only — OK.
No functional changes.components/hubspot/sources/new-email-event/new-email-event.mjs (1)
11-11: Version bump only — OK.
All good.
| description: | ||
| "Emit new event when a contact is added to a HubSpot list. [See the documentation](https://developers.hubspot.com/docs/reference/api/crm/lists#get-%2Fcrm%2Fv3%2Flists%2F%7Blistid%7D%2Fmemberships%2Fjoin-order)", | ||
| version: "0.0.5", | ||
| version: "0.0.6", |
There was a problem hiding this comment.
🧩 Analysis chain
Version bump only, but source still skips first run; align with PR goal to emit when ts is falsy
This source seeds and returns early when no last timestamp (Lines 154–158), which prevents emitting on first run. The PR objective is to always emit when ts is falsy. Recommend removing the early return and making the comparison include the falsy-ts case.
Apply:
@@
- if (!lastMembershipTimestamp) {
- const baselineTimestamp = new Date().toISOString();
- this._setLastMembershipTimestamp(listId, baselineTimestamp);
- return newMemberships;
- }
+ // Emit on first run when no prior timestamp exists
+ // (do not seed-and-skip; we'll set the latest seen at the end)
@@
- if (membershipTimestamp > lastMembershipTimestamp) {
+ if (!lastMembershipTimestamp || membershipTimestamp > lastMembershipTimestamp) {
newMemberships.push({
membership,
listInfo,
});
if (
- !latestMembershipTimestamp ||
- membershipTimestamp > latestMembershipTimestamp
+ !latestMembershipTimestamp ||
+ membershipTimestamp > latestMembershipTimestamp
) {
latestMembershipTimestamp = membershipTimestamp;
}
}Optionally, for robustness, compare numeric timestamps instead of ISO strings to avoid format pitfalls; I can provide that diff if desired. Please confirm this behavior matches your centralized isRelevant semantics.
Also applies to: 154-158, 183-195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(rg --hidden --files | rg "components/hubspot/sources/new-contact-added-to-list/new-contact-added-to-list.mjs" || true)
if [[ -z "$file" ]]; then
file=$(rg --hidden --files | rg "new-contact-added-to-list.mjs" || true)
fi
if [[ -z "$file" ]]; then
echo "FILE_NOT_FOUND"
exit 0
fi
echo "FILE: $file"
nl -ba "$file"
echo
echo "--- relevant identifier matches ---"
rg -n --hidden "lastMembershipTimestamp|_setLastMembershipTimestamp|membershipTimestamp|baselineTimestamp|latestMembershipTimestamp" "$file" -n -C3 || trueLength of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=$(rg --hidden --files | rg "components/hubspot/sources/new-contact-added-to-list/new-contact-added-to-list.mjs" || true)
if [[ -z "$file" ]]; then
file=$(rg --hidden --files | rg "new-contact-added-to-list.mjs" || true)
fi
if [[ -z "$file" ]]; then
echo "FILE_NOT_FOUND"
exit 0
fi
echo "FILE: $file"
echo
echo "----- lines 1-30 -----"
awk 'NR>=1 && NR<=30 {printf "%4d: %s\n", NR, $0}' "$file" || true
echo
echo "----- lines 140-210 -----"
awk 'NR>=140 && NR<=210 {printf "%4d: %s\n", NR, $0}' "$file" || true
echo
echo "----- identifier matches (with context) -----"
rg -n --hidden "lastMembershipTimestamp|_setLastMembershipTimestamp|membershipTimestamp|baselineTimestamp|latestMembershipTimestamp" "$file" -n -C3 || trueLength of output: 6064
Emit on first run: remove seed-and-skip and include falsy ts in comparison
The code currently seeds-and-returns when lastMembershipTimestamp is falsy (lines 154–158), preventing any emits on first run — remove the early return and treat a falsy lastMembershipTimestamp as "emit everything seen" by changing the comparison. Applies to lines 154–158 and 183–195.
Apply:
@@
- if (!lastMembershipTimestamp) {
- const baselineTimestamp = new Date().toISOString();
- this._setLastMembershipTimestamp(listId, baselineTimestamp);
- return newMemberships;
- }
+ // Emit on first run when no prior timestamp exists
+ // (do not seed-and-skip; we'll set the latest seen at the end)
@@
- if (membershipTimestamp > lastMembershipTimestamp) {
+ if (!lastMembershipTimestamp || membershipTimestamp > lastMembershipTimestamp) {
newMemberships.push({
membership,
listInfo,
});
if (
- !latestMembershipTimestamp ||
- membershipTimestamp > latestMembershipTimestamp
+ !latestMembershipTimestamp ||
+ membershipTimestamp > latestMembershipTimestamp
) {
latestMembershipTimestamp = membershipTimestamp;
}
}Optional: compare numeric timestamps (Date.getTime()) instead of ISO strings for robustness. Confirm this matches your centralized isRelevant semantics.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
components/hubspot/sources/new-contact-added-to-list/new-contact-added-to-list.mjs
around lines 154–158 and 183–195, the code currently seeds-and-skips when
lastMembershipTimestamp is falsy which prevents any emits on first run; remove
the early return and change the membership timestamp comparison so that a falsy
lastMembershipTimestamp is treated as "emit everything seen" (i.e., when
lastMembershipTimestamp is falsy, consider all events as newer), and use numeric
timestamps (Date.getTime() or Number(new Date(...))) for comparisons instead of
ISO string comparison to match centralized isRelevant semantics.
lcaresia
moved this from Ready for PR Review
to Ready for QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from Ready for QA
to In QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from In QA
to Changes Required
in Component (Source and Action) Backlog
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
components/hubspot/sources/common/common.mjs (3)
55-66: Avoid persisting after=0 when no valid ts; also await getTsOn first run, if all items lack a valid ts, this writes
afteras 0, causing repeated “first run” behavior. Also,getTsmay be async in some sources; not awaiting it risks comparing a Promise to a number.Apply:
- async processEvents(resources, after) { - let maxTs = after || 0; + async processEvents(resources, after) { + let maxTs = Number.isFinite(after) ? after : -Infinity; for (const result of resources) { - if (!after || await this.isRelevant(result, after)) { - this.emitEvent(result); - const ts = this.getTs(result); - if (ts > maxTs) { - maxTs = ts; - } - } + if (!after || await this.isRelevant(result, after)) { + this.emitEvent(result); + const ts = await this.getTs(result); + if (Number.isFinite(ts) && ts > maxTs) { + maxTs = ts; + } + } } - this._setAfter(maxTs); + if (Number.isFinite(maxTs) && (after == null || maxTs > after)) { + this._setAfter(maxTs); + } },
128-136: Align hasMore pagination with async getTs + safe ts checksAwait
getTsand guard withNumber.isFiniteto avoid persisting invalidafter.- if (!after || await this.isRelevant(item, after)) { - this.emitEvent(item); - const ts = this.getTs(item); - if (ts > maxTs) { - maxTs = ts; - this._setAfter(ts); - } - } + if (!after || await this.isRelevant(item, after)) { + this.emitEvent(item); + const ts = await this.getTs(item); + if (Number.isFinite(ts) && ts > maxTs) { + maxTs = ts; + this._setAfter(ts); + } + }
94-95: Don't early-return on ts check — HubSpot paging isn't guaranteed ordered by timestampHubSpot Search/List do not guarantee a global, monotonic sort by created/updated timestamp across pages; the opaque "after" cursor and "sorts" are not reliable for full incremental syncs. The early return at components/hubspot/sources/common/common.mjs:94-95 (
return;) can drop newer items on later pages. Remove the early return and instead continue paging + filter/dedupe (use an overlap window) or switch to Lists/changes/webhooks for reliable incremental syncs.
🧹 Nitpick comments (2)
components/hubspot/sources/common/common.mjs (2)
83-87: Comment doesn’t match behavior; proposed logic handles null ts explicitlyThe comment claims this handles null ts, but current condition forces a function‑level return when
tsis null on first run. The patch above makes nulltsa non‑blocking case on first run and a skip on subsequent runs.
57-57: Optional: pass ts to isRelevant in processEvents for parityFor parity and to avoid recomputing in overrides, consider computing
tsbeforeisRelevanthere and passing it (like in paginate).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/hubspot/sources/common/common.mjs(3 hunks)components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-10T19:18:27.998Z
Learnt from: GTFalcao
PR: PipedreamHQ/pipedream#14265
File: components/the_magic_drip/sources/common.mjs:35-43
Timestamp: 2024-10-10T19:18:27.998Z
Learning: In `components/the_magic_drip/sources/common.mjs`, when processing items in `getAndProcessData`, `savedIds` is intentionally updated with IDs of both emitted and non-emitted items to avoid emitting retroactive events upon first deployment and ensure only new events are emitted as they occur.
Applied to files:
components/hubspot/sources/common/common.mjs
🧬 Code graph analysis (1)
components/hubspot/sources/common/common.mjs (16)
components/hubspot/sources/new-event/new-event.mjs (1)
result(54-56)components/hubspot/sources/new-form-submission/new-form-submission.mjs (3)
result(62-62)ts(47-47)ts(63-63)components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
ts(102-102)components/hubspot/sources/new-contact-property-change/new-contact-property-change.mjs (1)
ts(38-38)components/hubspot/sources/new-custom-object-property-change/new-custom-object-property-change.mjs (1)
ts(44-44)components/hubspot/sources/new-ticket-property-change/new-ticket-property-change.mjs (1)
ts(39-39)components/hubspot/sources/new-company-property-change/new-company-property-change.mjs (1)
ts(38-38)components/hubspot/sources/delete-blog-article/delete-blog-article.mjs (1)
ts(21-21)components/hubspot/sources/new-deal-property-change/new-deal-property-change.mjs (1)
ts(36-36)components/hubspot/sources/new-email-subscriptions-timeline/new-email-subscriptions-timeline.mjs (1)
ts(19-19)components/hubspot/sources/new-email-event/new-email-event.mjs (1)
ts(33-33)components/hubspot/sources/new-engagement/new-engagement.mjs (1)
ts(33-33)components/hubspot/sources/new-or-updated-blog-article/new-or-updated-blog-article.mjs (1)
ts(34-34)components/hubspot/sources/new-or-updated-company/new-or-updated-company.mjs (1)
ts(52-52)components/hubspot/sources/new-or-updated-crm-object/new-or-updated-crm-object.mjs (1)
ts(29-29)components/hubspot/sources/new-or-updated-contact/new-or-updated-contact.mjs (1)
ts(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (1)
components/hubspot/sources/common/common.mjs (1)
81-96: Don't return early when ts is null — await getTs and guard comparisonsThe current
if ((ts && !after) || ts > after) { … } else { return; }short‑circuits whentsis null and blocks emission on first run. AwaitgetTs, emit all items when!after, updateafteronly for finite numericts, useNumber.isFinite(ts), and prefercontinuefor invalid/nullts. Keepreturnonly if the API guarantees results are sorted descending by timestamp.File: components/hubspot/sources/common/common.mjs (≈lines 81–96)
Apply:
for (const result of results) { - const ts = this.getTs(result); - // Adding ts && !after to handle the case where ts is null - // (e.g. when using deletedAt as the ts field for deleted items) - if ((ts && !after) || ts > after) { - if (!after || await this.isRelevant(result, after, ts)) { - this.emitEvent(result); - } - if (ts > maxTs) { - maxTs = ts; - this._setAfter(ts); - } - } else { - return; - } + const ts = await this.getTs(result); + // When no 'after', emit everything; update 'after' only with valid timestamps + if (!after) { + this.emitEvent(result); + if (Number.isFinite(ts) && ts > maxTs) { + maxTs = ts; + this._setAfter(ts); + } + continue; + } + + // With 'after', emit only newer items. If list is sorted by ts desc, + // we can return when we reach <= after; otherwise just continue. + if (Number.isFinite(ts) && ts > after) { + if (await this.isRelevant(result, after, ts)) { + this.emitEvent(result); + } + if (ts > maxTs) { + maxTs = ts; + this._setAfter(ts); + } + } else if (Number.isFinite(ts)) { + return; // safe only if API sorts by descending timestamp + } else { + continue; // skip items without a valid ts on subsequent runs + } }My automated search found no matches for
async getTsor usages ofthis.paginate(UsingHasMore)in the repo — manual verification required: confirmgetTsimplementations are async/await-safe and whether the HubSpot endpoints guarantee descending timestamps; if not, replace thereturnwithcontinue.
|
/approve |
github-project-automation
bot
moved this from Changes Required
to Done
in Component (Source and Action) Backlog
|
I'm merging this earlier due to the majority of the sources having issues with event emissions currently. The 2 remaining issues reported should've been fixed also, but we can work it out on a subsequent PR if needed. |
GTFalcao
moved this from Ready for QA
to Changes Required
in Component (Source and Action) Backlog
GTFalcao
moved this from Changes Required
to Done
in Component (Source and Action) Backlog
Some HubSpot sources currently ignore the "last timestamp" value if it is undefined (e.g. the first time the source runs).
This PR removes the
tscheck from the individual methods it was on, and instead expands that behavior to all sources, i.e. to all the instances of calling theisRelevantmethod. This ensures all sources properly emit the items fetched in their first run.Summary by CodeRabbit
Bug Fixes
Chores