-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[Components] demandbase - new components #15652
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis changeset introduces several new modules and enhancements. New action modules for "Company Lookup" and "Contact Search" have been added with rich metadata and configurable properties. Common utility modules for agents, constants, states, and URL encoding functions are now available. The main demandbase application module is enhanced with new property definitions and methods for building API requests. Additionally, webhook handling and a new company change event source have been implemented to manage subscriptions and validate incoming requests. Changes
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
components/demandbase/actions/contact-search/contact-search.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/demandbase/actions/company-lookup/company-lookup.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/demandbase/sources/common/webhook.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
87b1e63
to
1be25e0
Compare
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Nitpick comments (11)
components/demandbase/sources/common/webhook.mjs (2)
93-105
: Consider migrating to a more secure hashing algorithm
The code uses SHA-1, which is widely considered vulnerable. If the API supports stronger hashing (e.g., SHA-256), upgrading can improve security.validateSignature(headers, rawBody) { const signature = headers["X-InsideViewAPI-AlertDataSignature"]; const signingSecret = this.getSecret(); - const computedSignature = createHmac("sha1", signingSecret) + const computedSignature = createHmac("sha256", signingSecret) .update(rawBody) .digest("hex"); if (signature !== computedSignature) { throw new Error("Invalid signature"); } }
106-119
: Return a descriptive HTTP response on signature failure
Throwing an error is acceptable in many frameworks, but providing an explicit status code (e.g., 401) and message can improve debugging and client experience.async run({ method, body, headers, rawBody, }) { if (method === "HEAD") { return this.http.respond({ status: 200, headers: { "X-InsideViewAPI-ValidationCode": headers["X-InsideViewAPI-ValidationCode"], }, }); } try { this.validateSignature(headers, rawBody); } catch (err) { return this.http.respond({ status: 401, body: "Signature validation failed", }); } this.processResource(body); }components/demandbase/demandbase.app.mjs (1)
103-121
: Add error handling for_makeRequest
If the remote API call fails,_makeRequest
may throw an unhandled error. Consider wrapping the call toaxios
in a try/catch to surface clearer error messages or user-facing feedback._makeRequest({ $ = this, path, headers, ...args } = {}) { try { return axios($, { ...args, url: this.getUrl(path), headers: this.getHeaders(headers), }); } catch (err) { this.$emit(`Request to ${path} failed: ${err.message}`); throw err; } }components/demandbase/common/utils.mjs (2)
3-5
: Add input validation and handle edge cases.The function could be more robust by handling edge cases and validating input.
Consider this improved implementation:
-function arrayToCommaSeparatedList(array) { - return array?.join(","); +function arrayToCommaSeparatedList(array) { + if (!array) return ""; + if (!Array.isArray(array)) return String(array); + return array.join(",");
7-21
: Add error handling and input validation.While the function correctly handles the happy path, it could benefit from error handling.
Consider this enhanced implementation:
function getUrlEncodedData(data) { + if (!data || typeof data !== 'object') { + throw new Error('Input must be a non-null object'); + } const params = new URLSearchParams(); Object.entries(data) .forEach(([ key, value, ]) => { if (Array.isArray(value) && value.length) { params.append(key, arrayToCommaSeparatedList(value)); } else if (value) { params.append(key, value); } }); return params.toString(); }components/demandbase/sources/new-company-change-instant/new-company-change-instant.mjs (1)
36-42
: Add validation in generateMeta method.The metadata generation could be more robust with input validation.
Consider this enhanced implementation:
generateMeta(resource) { + if (!resource?.alertId || !resource?.date) { + throw new Error('Invalid resource: missing required fields'); + } return { id: resource.alertId, summary: `New Company Change: ${resource.alertId}`, ts: Date.parse(resource.date), }; }components/demandbase/common/agents.mjs (1)
1-74
: Consider making the agents object immutable.To prevent accidental modifications to these constants during runtime, consider freezing the object.
Apply this change:
-export default { +export default Object.freeze({ LEADERSHIP_CHANGES: { value: "LEADERSHIP_CHANGES", label: "Leadership Changes", }, // ... rest of the categories ... -}; +});components/demandbase/actions/company-lookup/company-lookup.mjs (2)
9-9
: Fix double period in description.The description ends with two periods.
- description: "Build a list of company names along with company ID, city, state and country using rich filters.. [See the documentation](https://kb.demandbase.com/hc/en-us/articles/7273850579227--POST-Company-Lookup).", + description: "Build a list of company names along with company ID, city, state and country using rich filters. [See the documentation](https://kb.demandbase.com/hc/en-us/articles/7273850579227--POST-Company-Lookup).",
228-254
: Consider simplifying the large destructuring.The large destructuring block could be simplified by destructuring only what's needed for validation and passing
this
directly togetUrlEncodedData
.async run({ $ }) { - const { - companyLookup, - agents, - areaCodes, - // ... many more properties - } = this; + const { companyLookup } = this; const response = await companyLookup({ $, headers: { "Content-Type": "application/x-www-form-urlencoded", }, - data: utils.getUrlEncodedData({ - agents, - areaCodes, - // ... many more properties - }), + data: utils.getUrlEncodedData(this), });components/demandbase/actions/contact-search/contact-search.mjs (1)
196-257
: Consider making resultsPerPage configurable.The
resultsPerPage
is hardcoded to 50, which might not suit all use cases.Add it as a configurable prop:
+resultsPerPage: { + type: "integer", + label: "Results Per Page", + description: "Number of results to return per page (max 100).", + optional: true, + default: 50, + min: 1, + max: 100, +}, async run({ $ }) { const { contactSearch, email, // ... other props ... + resultsPerPage = 50, } = this; const response = await contactSearch({ $, params: { // ... other params ... - resultsPerPage: 50, + resultsPerPage, }, });Also, consider enhancing the success message with the actual number of contacts found:
-$.export("$summary", "Successfully fetched contacts."); +$.export("$summary", `Successfully fetched ${response.length} contacts.`);components/demandbase/common/states.mjs (1)
1-1622
: Consider organizing states by country.The current flat array structure makes it difficult to maintain and use country-specific states.
Consider restructuring the data:
-export default [ +export default { + US: [ { value: "AK", label: "Alaska", }, // ... other US states + ], + UK: [ { value: "East Midlands", label: "East Midlands", }, // ... other UK regions + ], // ... other countries +};
🛑 Comments failed to post (8)
components/demandbase/sources/common/webhook.mjs (1)
16-39: 🛠️ Refactor suggestion
Add robust error handling in the
activate
hook
Currently, ifcreateSubscription
fails, the code logs no error and still sets DB values. Wrapping the subscription creation in try/catch would ensure that subscription IDs and secrets are only stored upon successful creation.async activate() { try { const { createSubscription, getData, http: { endpoint: webhook }, setSubscriptionId, setSecret, } = this; const signingSecret = uuidv4(); const response = await createSubscription({ data: { webhook, signingSecret, ...getData(), }, }); setSubscriptionId(response.subscriptionId); setSecret(signingSecret); } catch (err) { console.error("Failed to create subscription:", err); throw err; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.hooks: { async activate() { try { const { createSubscription, getData, http: { endpoint: webhook }, setSubscriptionId, setSecret, } = this; const signingSecret = uuidv4(); const response = await createSubscription({ data: { webhook, signingSecret, ...getData(), }, }); setSubscriptionId(response.subscriptionId); setSecret(signingSecret); } catch (err) { console.error("Failed to create subscription:", err); throw err; } },
components/demandbase/demandbase.app.mjs (1)
7-101: 🛠️ Refactor suggestion
Ensure paginated fetching for all multi-value props
The prop definitions (industries
,countryId
,categoryIds
,companyId
) retrieve lists from various endpoints, but onlycompanyId
explicitly handles page increments. Consider expanding pagination to other props (e.g., large industry lists) to prevent truncation of available options.components/demandbase/sources/new-company-change-instant/new-company-change-instant.mjs (1)
28-35:
⚠️ Potential issueFix typo in frequency parameter.
There's a typo in the
frecuency
parameter which could cause API issues.Apply this fix:
getData() { return { subscriptionType: constants.SUBSCRIPTION_TYPE.COMPANY, - frecuency: "1day", + frequency: "1day", name: "New Company Change", companyIds: utils.arrayToCommaSeparatedList(this.companyIds), }; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.getData() { return { subscriptionType: constants.SUBSCRIPTION_TYPE.COMPANY, frequency: "1day", name: "New Company Change", companyIds: utils.arrayToCommaSeparatedList(this.companyIds), }; }
components/demandbase/actions/company-lookup/company-lookup.mjs (1)
124-135: 🛠️ Refactor suggestion
Add validation for revenue parameters.
The revenue parameters should be validated to ensure maxRevenue is greater than minRevenue when both are provided.
Add validation in the run method:
async run({ $ }) { const { // ... other destructured properties minRevenue, maxRevenue, } = this; + + if (minRevenue && maxRevenue && minRevenue > maxRevenue) { + throw new Error('Maximum revenue must be greater than minimum revenue'); + } const response = await companyLookup({Committable suggestion skipped: line range outside the PR's diff.
components/demandbase/actions/contact-search/contact-search.mjs (3)
189-194: 🛠️ Refactor suggestion
Add error handling for the API request.
The
contactSearch
method should handle potential API errors gracefully.Consider adding error handling:
contactSearch(args = {}) { + try { return this.app._makeRequest({ path: "/contacts", ...args, }); + } catch (error) { + throw new Error(`Failed to search contacts: ${error.message}`); + } },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.contactSearch(args = {}) { try { return this.app._makeRequest({ path: "/contacts", ...args, }); } catch (error) { throw new Error(`Failed to search contacts: ${error.message}`); } },
13-18: 🛠️ Refactor suggestion
Consider adding validation for required fields.
The info alert indicates that one of email, lastName, or fullName should be present, but there's no validation enforcing this requirement.
Consider adding a custom validation function:
info: { type: "alert", alertType: "info", content: "**Email**, **Last Name** or **Full Name** should be present in the search query.", }, +validate: { + type: "function", + async: true, + test: ({ email, lastName, fullName }) => { + if (!email && !lastName && !fullName) { + return "At least one of Email, Last Name, or Full Name must be provided"; + } + return true; + }, +},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// eslint-disable-next-line pipedream/props-label, pipedream/props-description info: { type: "alert", alertType: "info", content: "**Email**, **Last Name** or **Full Name** should be present in the search query.", }, validate: { type: "function", async: true, test: ({ email, lastName, fullName }) => { if (!email && !lastName && !fullName) { return "At least one of Email, Last Name, or Full Name must be provided"; } return true; }, },
153-168: 💡 Verification agent
🧩 Analysis chain
Verify confidence score validation.
The min/max validation for confidence scores is properly set, but we should verify if these limits align with the latest Demandbase API specifications.
🌐 Web query:
What are the valid confidence score ranges in the Demandbase API for contact search?
💡 Result:
Demandbase's Contact Quality Score (CQS) uses letter grades (A+, A, B, C) to represent confidence in contact data accuracy, with corresponding percentage ranges:
Confidence Range Grade 95%+ A+ 85%-94% A 75%-84% B Below 75% C This grading system reflects the reliability of enriched contact data based on factors like recency of verification, source credibility, job title relevance, and data completeness[4]. For API implementations involving contact searches or exports, these grades serve as confidence indicators for prioritizing outreach efforts[4][9].
Citations:
- 1: https://support.demandbase.com/hc/en-us/articles/360053937452-Understanding-Qualification-Scores
- 2: https://support.demandbase.com/hc/en-us/articles/360057289392-Set-Up-Qualification-Scores
- 3: https://www.demandbase.com/products/data/api-integration/
- 4: https://support.demandbase.com/hc/en-us/articles/28486009160219-Understanding-Contact-Quality-Score-CQS
- 5: https://support.demandbase.com/hc/en-us/articles/4448853119131-Best-Practices-for-Setting-Up-and-Using-Predictive-Scores
- 6: https://support.demandbase.com/hc/en-us/articles/23789223879323-Reference-Demandbase-IP-API-v3-for-Demandbase-One-Updated-Version
- 7: https://developer.demandbase.com
- 8: https://nubela.co/blog/clearbit-competitors-alternatives/
- 9: https://www.demandbase.com/blog/lets-talk-about-match-rates/
- 10: https://support.demandbase.com/hc/en-us/articles/360054236292-FAQs-Predictive-Scores
Action Required: Update Confidence Score Validation
The current validation for
minContactConfidenceScore
andmaxContactConfidenceScore
uses a numeric range (0–100), but the latest Demandbase API specifies contact quality ratings using letter grades defined by percentage thresholds (e.g., A+ for 95%+; A for 85%–94%; etc.). Please verify whether the numeric range is still appropriate—perhaps with an internal conversion mechanism—or if the validation should be revised to accept and handle letter grades in accordance with the API.
- File to review:
components/demandbase/actions/contact-search/contact-search.mjs
(lines 153–168)- Action items:
- Confirm whether the current integer-based approach is part of an internal conversion strategy.
- If not, update the validation logic and corresponding documentation to align with Demandbase’s API specification.
components/demandbase/common/states.mjs (1)
1-1622:
⚠️ Potential issuePotential value collision in state codes.
There are duplicate values that could cause ambiguity:
- "NT" is used for both Northern Territory (Australia) and Northwest Territories (Canada)
- "AL" is used for both Alabama (US) and Alagoas (Brazil)
Consider prefixing the values with country codes to ensure uniqueness:
- value: "NT", + value: "AU-NT", label: "Northern Territory", }, // ... - value: "NT", + value: "CA-NT", label: "Northwest Territories", },
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: 1
🧹 Nitpick comments (8)
components/demandbase/demandbase.app.mjs (5)
103-105
: Consider moving the base URL to constants.The base URL should be defined in a constants file for better maintainability and reusability.
+import { BASE_URL, VERSION_PATH } from "../common/constants.mjs"; getUrl(path) { - return `https://api.insideview.com/api/v1${path}`; + return `${BASE_URL}${VERSION_PATH}${path}`; }
113-121
: Enhance error handling in _makeRequest method.Consider adding try-catch block and specific error handling for different HTTP status codes.
_makeRequest({ $ = this, path, headers, ...args } = {}) { + try { return axios($, { ...args, url: this.getUrl(path), headers: this.getHeaders(headers), }); + } catch (error) { + if (error.response) { + const { status, data } = error.response; + throw new Error(`Request failed with status ${status}: ${data.message}`); + } + throw error; + } }
18-31
: Simplify the industries options logic.The nested logic for handling sub-industries could be simplified for better readability and maintainability.
Consider extracting the sub-industries mapping logic:
- return industryIds.flatMap((industryId) => { - const industry = - industries.find(({ industry: { industryId: id } }) => id === industryId); - return industry?.subIndustries - ?.map(({ - industryId: value, - industryName: label, - }) => ({ - label, - value, - })) || []; - }); + const getSubIndustries = (industryId) => { + const industry = industries.find(({ industry: { industryId: id } }) => id === industryId); + return industry?.subIndustries?.map(({ industryId: value, industryName: label }) => ({ + label, + value, + })) || []; + }; + return industryIds.flatMap(getSubIndustries);
82-99
: Consider making pagination parameters configurable.The hardcoded
resultsPerPage
value of 100 could be made configurable to allow for different use cases.Consider adding a parameter for page size:
- async options({ page }) { + async options({ page, pageSize = 100 }) { const { companies } = await this.listCompanies({ headers: { "Content-Type": "application/x-www-form-urlencoded", }, params: { page: page + 1, - resultsPerPage: 100, + resultsPerPage: pageSize, }, });
50-57
: Consider extracting the mapper function.The default mapper function could be moved to a separate utility to improve code organization.
Consider creating a separate utility:
+const defaultCountryMapper = ({ + countryId: value, + countryName: label, +}) => ({ + label, + value, +}); countryId: { // ... async options({ - mapper = ({ - countryId: value, - countryName: label, - }) => ({ - label, - value, - }), + mapper = defaultCountryMapper, }) {components/demandbase/actions/company-lookup/company-lookup.mjs (1)
227-287
: Add input validation in the run method.Consider validating required parameters before making the API call to provide better error messages.
const response = await companyLookup({ + // Validate that at least one search parameter is provided + if (!industries?.length && !companyName && !keywords) { + throw new Error("At least one search parameter (industries, companyName, or keywords) must be provided"); + } $, headers: { "Content-Type": "application/x-www-form-urlencoded", }, data: utils.getUrlEncodedData({ agents, // ... rest of the parameters }), });components/demandbase/actions/contact-search/contact-search.mjs (1)
252-252
: Make resultsPerPage configurable.Consider making the
resultsPerPage
parameter configurable through props to allow users to control the number of results.+ resultsPerPage: { + type: "integer", + label: "Results Per Page", + description: "Number of results to return per page (max: 100).", + optional: true, + default: 50, + min: 1, + max: 100, + }, - resultsPerPage: 50, + resultsPerPage: this.resultsPerPage || 50,components/demandbase/common/states.mjs (1)
1-1622
: Consider organizing states by country and adding type definitions.The current flat array structure could be improved for better maintainability and type safety.
+/** + * @typedef {Object} State + * @property {string} value - The state code + * @property {string} label - The state name + */ + +/** + * @typedef {Object} CountryStates + * @property {State[]} states - Array of states + */ + -export default [ +export default { + US: { + name: "United States", + states: [ { value: "AK", label: "Alaska", }, // ... other US states + ], + }, + UK: { + name: "United Kingdom", + states: [ // ... UK states + ], + }, // ... other countries -]; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
components/demandbase/actions/company-lookup/company-lookup.mjs
(1 hunks)components/demandbase/actions/contact-search/contact-search.mjs
(1 hunks)components/demandbase/common/agents.mjs
(1 hunks)components/demandbase/common/constants.mjs
(1 hunks)components/demandbase/common/states.mjs
(1 hunks)components/demandbase/common/utils.mjs
(1 hunks)components/demandbase/demandbase.app.mjs
(1 hunks)components/demandbase/package.json
(2 hunks)components/demandbase/sources/common/webhook.mjs
(1 hunks)components/demandbase/sources/new-company-change-instant/new-company-change-instant.mjs
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- components/demandbase/sources/common/webhook.mjs
- components/demandbase/sources/new-company-change-instant/new-company-change-instant.mjs
- components/demandbase/common/constants.mjs
- components/demandbase/common/utils.mjs
- components/demandbase/common/agents.mjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
🔇 Additional comments (8)
components/demandbase/demandbase.app.mjs (3)
82-99
: LGTM! Well-structured prop definition with dynamic options.The implementation of the
companyId
prop with pagination support is clean and follows best practices.
1-5
: LGTM! Module structure follows best practices.The import and module definition are well-structured and follow Pipedream's conventions.
1-160
: Overall implementation looks solid!The Demandbase app module is well-structured with clear separation of concerns, proper type definitions, and comprehensive API integration. The suggested improvements are optimizations that can be implemented in future iterations.
components/demandbase/actions/company-lookup/company-lookup.mjs (1)
12-218
: LGTM! Comprehensive prop definitions with clear documentation.The props are well-documented with appropriate types, labels, and descriptions. The use of
propDefinition
for reusable props is a good practice.components/demandbase/actions/contact-search/contact-search.mjs (1)
13-18
: LGTM! Clear user guidance through info alert.Good use of the info alert to guide users about required search parameters.
components/demandbase/package.json (3)
3-3
: Version Field Update
The version has been updated to"0.1.0"
from the previous"0.0.1"
, which signals a new release with added functionality. Ensure that semantic versioning is correctly followed (e.g., major.minor.patch) for all components.
12-14
: Publish Configuration Update
The"publishConfig"
section now explicitly sets"access": "public"
. This is important for ensuring that the package is published with the correct access rights on the npm registry. Confirm that this aligns with your organization's publication policies.
15-17
: New Dependency Addition
A new dependency on"@pipedream/platform"
with version constraint"^3.0.3"
has been added. Please verify that this version is compatible with the rest of the codebase and that it meets any security or compatibility requirements.
WHY
Resolves #15164
Summary by CodeRabbit