- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.5k
HubSpot 'new deal in stage' - fixing initial event emission #18455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
 | 
| WalkthroughBumps HubSpot component version to 1.7.10 and updates the new-deal-in-stage source to version 0.0.41, adjusting relevance checks to handle undefined initial timestamps and adding an early return to limit processing to the first page on initial runs. Changes
 Sequence Diagram(s)sequenceDiagram
    autonumber
    participant U as Source (new-deal-in-stage v0.0.41)
    participant HS as HubSpot API
    Note over U: Initial run: after = undefined
    U->>HS: List deals in stage (page 1)
    HS-->>U: Deals page 1
    rect rgba(200,230,255,0.3)
    note right of U: Relevance check updated
    U->>U: For each deal: if (!after || isRelevant(ts, after)) emit
    end
    alt Initial run
      Note over U: Early return after page 1 (no pagination)
      U-->>U: Stop pagination
    else Subsequent runs
      loop Additional pages
        U->>HS: Fetch next page
        HS-->>U: Deals page N
        U->>U: Apply same relevance check
      end
    end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
 Pre-merge checks and finishing touches❌ Failed checks (1 warning)
 ✅ Passed checks (4 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  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (2)
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (2)
38-45: Potential crash: missing optional chaining onversions.
properties.dealstage?.versions[0]can throw ifversionsis undefined or empty. Use optional chaining onversionsand the element access.Apply this diff:
- return properties.dealstage?.versions[0].timestamp; + return properties.dealstage?.versions?.[0]?.timestamp;
101-121: components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs: defer_setAfteruntil after loop and guard nullishts
- Changetoif (!after || this.isRelevant(ts, after))if (!after || (ts != null && this.isRelevant(ts, after)))
- Remove the inline
this._setAfter(ts)in the loop and instead, after thefor(but beforeif (!after) return;), add:const prev = this._getAfter() ?? 0; if (maxTs > prev) this._setAfter(maxTs);This ensures
afteronly ever increases and skips nullish timestamps.
🧹 Nitpick comments (1)
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
103-115: Optional: prevent emitting events with undefined/NaN timestamps.On first run,
!aftermakes the condition true even iftsis undefined, which can produce events with an undefinedts. If that’s undesirable, add a guard/coercion.- if (!after || this.isRelevant(ts, after)) { + const tsNum = (ts == null ? undefined : Number(ts)); + if (!after || (Number.isFinite(tsNum) && this.isRelevant(tsNum, after))) { ... - this.emitEvent(deal, ts); + this.emitEvent(deal, tsNum);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
- components/hubspot/package.json(1 hunks)
- components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
components/hubspot/sources/common/common.mjs (1)
after(189-189)
⏰ 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: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
🔇 Additional comments (2)
components/hubspot/package.json (1)
3-3: Version bump looks good.No functional changes here. Please ensure a corresponding CHANGELOG entry and publish step.
components/hubspot/sources/new-deal-in-stage/new-deal-in-stage.mjs (1)
14-14: Source version bump OK.Matches package bump; ready for release.
Closes #18416
Follow-up to #18438
Summary by CodeRabbit
Bug Fixes
Chores