[Components] Drift. Actions + Sources #16553
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
@SokolovskyiK is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughNew Drift integration components are introduced, including actions for creating, updating, deleting, and retrieving contacts, as well as polling sources for new conversations and messages. Utility functions and a comprehensive Drift API client are implemented. The package version is incremented to 0.7.0. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Action
participant DriftApp
participant DriftAPI
User->>Action: Trigger Create Contact
Action->>DriftApp: createContact (with attributes)
DriftApp->>DriftAPI: POST /contacts
DriftAPI-->>DriftApp: Response (contact created)
DriftApp-->>Action: Contact details
Action-->>User: Confirmation and contact info
sequenceDiagram
participant Source
participant DriftApp
participant DriftAPI
participant DB
Source->>DB: Get last conversation/message ID
Source->>DriftApp: getNewestConversations or getMessagesByConvId
DriftApp->>DriftAPI: GET /conversations or /messages
DriftAPI-->>DriftApp: Response (data, pagination)
DriftApp-->>Source: Data
Source->>DB: Update last conversation/message ID
Source-->>User: Emit new events
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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/drift/actions/create-contact/create-contact.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (31)
components/drift/actions/create-contact/create-contact.mjs (3)
53-61: Consider performance implications of object spreading with large customAttributesThe use of
...customAttributescould lead to performance issues if the customAttributes object is very large. Also, be careful when spreading user-provided objects as they might contain properties that could override your predefined attributes.const attributes = removeNullEntries({ email, name, phone, source, - ...customAttributes, + ...(customAttributes || {}), });
63-72: Fix extra space in error messageThere's an extra space between
Errorand the opening backtick in the error message.if (existingContact && existingContact.data.length > 0) { - throw new Error (`Contact ${email} already exists`); + throw new Error(`Contact ${email} already exists`); };
74-85: Improve formatting of warning messages in summaryThe warning message concatenation could be formatted more clearly. If there are no warnings, the current implementation will still add a newline character.
- $.export("$summary", `Contact ${email} created.` + warnings.join("\n- ")); + $.export("$summary", `Contact ${email} created.${warnings.length ? '\n- ' + warnings.join("\n- ") : ''}`);components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs (2)
30-30: Fix spacing and improve comment clarityThere's an extra space before the assignment operator, and the comment could be more descriptive.
- body.data.attributes = result.data.attributes; //inject more data + body.data.attributes = result.data.attributes; // Enrich webhook data with full contact attributes
32-36: Consider adding validation for required fieldsThe code assumes that
body.data.endUserIdandbody.timeStampwill always be present. Consider adding validation to handle cases where these fields might be missing.+ const id = body.data?.endUserId || `lead-${Date.now()}`; + const ts = body.timeStamp || Date.now(); + this.$emit(body, { summary: `Contact "${email}" ID "${contactId}" updated`, - id: body.data.endUserId, - ts: body.timeStamp, + id, + ts, });components/drift/sources/new-message-instant/new-message-instant.mjs (1)
12-17: Add validation for the conversationId prop.The conversationId is defined as an integer type, but there's no validation to ensure it's a positive number.
Add validation for the conversationId in the run method:
async run(event) { const { body } = event; const { drift } = this; const warnings = []; + if (this.conversationId && (isNaN(this.conversationId) || this.conversationId <= 0)) { + warnings.push("Conversation ID must be a positive number"); + } if (this.emailOrId) warnings.push(...drift.checkEmailOrId(this.emailOrId)); if (warnings.length > 0) console.log(warnings.join("\n- "));components/drift/actions/get-contact/get-contact.mjs (1)
34-35: Improve the warning message formatting.The warning messages are joined without a separator, which could make them harder to read.
- $.export("$summary", `Contact ${contact.attributes.email} ID "${contact.id}"` - + " fetched successfully." + warnings.join("\n- ")); + $.export("$summary", `Contact ${contact.attributes.email} ID "${contact.id}" fetched successfully.` + + (warnings.length ? "\n- " + warnings.join("\n- ") : ""));components/drift/sources/new-contact-update/new-contact-update.mjs (2)
24-26: Use consistent object reference pattern.You're using
this.driftwhile other files usedriftdirectly. Stay consistent with accessing the drift object.- const result = await this.drift.getContactById({ + const result = await drift.getContactById({ contactId, });
32-35: Remove redundant console.log statement.There are two consecutive console.log statements with similar content.
- console.log(body); - - console.log("Contact updated webhook payload:", body); + console.log("Contact updated webhook payload:", body);components/drift/actions/update-contact/update-contact.mjs (2)
8-8: Use consistent version numbering across components.This component uses version "0.0.9" while other new components use "0.0.1". Maintain consistent versioning for new components.
- version: "0.0.9", + version: "0.0.1",
86-86: Improve warning message formatting.The warning messages are joined without proper formatting, which could make them harder to read.
- $.export("$summary", `Contact ID ${contactId} updated successfully.` + warnings.join("\n- ")); + $.export("$summary", `Contact ID ${contactId} updated successfully.` + + (warnings.length ? "\n- " + warnings.join("\n- ") : ""));components/drift/actions/delete-contact/delete-contact.mjs (2)
28-32: Handle array vs. object response explicitly for clarity
contact = contact.data[0] || contact.data;works, but the implicit truthy check obscures intent and could mask edge–cases (e.g. whendatais an empty array). Prefer an explicit test so future readers immediately see why the fallback exists and to avoid accidentally operating onundefined.-let contact = await drift.getContactByEmailOrId($, emailOrId); -contact = contact.data[0] || contact.data; +const res = await drift.getContactByEmailOrId($, emailOrId); +const contact = Array.isArray(res.data) ? res.data[0] : res.data; + +if (!contact) { + throw new Error(`No contact returned for "${emailOrId}"`); +}
39-41: Avoid dangling bullet when no warnings are present
warnings.join("\n- ")always appends a newline + dash, so the summary becomes
…deleted successfully.-whenwarningsis empty. Build the string conditionally:-$.export("$summary", `Contact ${contactEmail} ID "${contactId}" deleted successfully.` + - warnings.join("\n- ")); +const summary = `Contact ${contactEmail} ID "${contactId}" deleted successfully.` + + (warnings.length ? `\n- ${warnings.join("\n- ")}` : ""); +$.export("$summary", summary);components/drift/drift.app.mjs (2)
108-114: Remove unreachableelsesemicolon and streamline error pathBiome flags line 113 as unreachable because both branches
throw. The trailing semicolon after the}is unnecessary and may confuse linters.- } catch (error) { - if (error.status === 404) { - throw new Error(`No contact found with ID: ${contactId}`); - } else { - throw error; - }; - } + } catch (error) { + if (error.status === 404) { + throw new Error(`No contact found with ID: ${contactId}`); + } + throw error; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: This code is unreachable
... because either this statement ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
132-136: Typo in variable name & potential off-by-one
const firtsNew→firstNew. Small typo hinders readability.Also, throwing when the id is not found may abort an otherwise recoverable poll. Consider logging and returning an empty list instead.
-const firtsNew = arr.indexOf(lastKnown); -if (firtsNew === -1) throw new Error("Id not found"); -const newest = arr.slice(0, firtsNew); +const firstNew = arr.indexOf(lastKnown); +if (firstNew === -1) return []; +const newest = arr.slice(0, firstNew);components/drift/sources/new-conversation-instant/new-conversation-instant.mjs (2)
41-43: Debug statement left in source
console.log("here");looks like leftover debugging noise and will spam logs.- console.log("here");
100-101: Event summary references contactId, not the conversation IDFor clarity to users looking at the event feed, consider using the conversation id (matches
idfield) or include both.-summary: `New conversation with ID ${conversations[i].contactId}`, +summary: `New conversation ${conversations[i].id} with contact ${conversations[i].contactId}`,components/drift/common/methods.mjs (14)
12-15: Fix semicolon placement in conditional block.There's an unnecessary semicolon after the conditional block which doesn't follow JavaScript best practices.
isEmptyString(input) { if (this.isString(input)) { if (input.trim() === "") return true; - }; + } return false; },
35-38: Fix semicolon placement in for loop.There's an unnecessary semicolon after the for loop which doesn't follow JavaScript best practices.
for (let i = 0; i < input.length; i++) { if (!this.isString(input[i])) return false; - }; + }
62-65: Fix JSDoc comment formatting.The comment has inconsistent slashes. The closing delimiter should match the opening style.
/* ============================================================================================= Function tries to parse the input as JSON, If it is not return the value as it was passed - //==============================================================================================*/ + ==============================================================================================*/
92-95: Fix semicolon placement after conditional block.There's an unnecessary semicolon after the conditional block which doesn't follow JavaScript best practices.
if (!this.isString(input)) { warnings.push("URL is not a string"); - - }; + }
97-100: Fix semicolon placement after conditional block.There's an unnecessary semicolon after the conditional block which doesn't follow JavaScript best practices.
if (this.isEmptyString(input)) { warnings.push("URL is empty string"); return warnings; - }; + }
104-109: Fix semicolon placement and improve regex check.There's an unnecessary semicolon after the conditional block, and the regex test could be improved.
// Reject if spaces, tabs, or newlines are present - if ((/[ \t\n]/.test(trimmedInput))) { + if (/[ \t\n]/.test(trimmedInput)) { warnings.push( "Url contains invalid characters like space, backslash etc., please check." + this._reasonMsg(input)); return warnings; - }; + }
119-121: Fix semicolon placement after conditional block.There's an unnecessary semicolon after the conditional block which doesn't follow JavaScript best practices.
if (dubiousMatches) { warnings.push(" URL contains dubious or non-standard characters " + this._reasonMsg(input) ); - }; + }
130-135: Fix semicolon placement and improve warning message.There's an unnecessary semicolon after the conditional block and the warning message could be better formatted.
// Warn if user typed only one slash (e.g., https:/) if (/^(https?):\/(?!\/)/.test(input)) { - warnings.push(` It looks like you're missing one slash after "${urlObject.protocol}".` + `Did you mean "${urlObject.protocol}//..."? ${this._reasonMsg(input)} `); - - }; + }
146-150: Fix semicolon placement after catch block.There's an unnecessary semicolon after the catch block which doesn't follow JavaScript best practices.
} catch (err) { warnings.push(" URL contains potentionally unacceptable characters\"" + this._reasonMsg(input)); - - }; + }
147-148: Fix typo in warning message.The warning message contains a typo in the word "potentially".
- warnings.push(" URL contains potentionally unacceptable characters\"" + + warnings.push(" URL contains potentially unacceptable characters\"" + this._reasonMsg(input));
198-204: Consider using a more robust email validation regex.The current regex is fairly minimal. While it checks basic structure, it might be worth considering a more comprehensive pattern that better matches email RFC standards.
For a more robust but still reasonable email validation, consider:
- const basicPattern = /^[^@\s]+@[^@\s]+\.[^@\s]+$/; + // More comprehensive pattern checking for valid characters and structure + const basicPattern = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/;This pattern better enforces valid characters in the local and domain parts, and requires a minimum domain extension length.
255-274: Optimize thecheckEmailOrIdimplementation.Currently, we convert the input to a number twice. We can optimize this by storing the result of the first conversion.
checkEmailOrId(input) { const warnings = []; const trimmed = this.trimIfString(input); + // Convert to number once + const numericValue = Number(trimmed); // If it's an ID like number or string (e.g 12345 or "12345"); // it's Valid. Return empty warnings array. - if (this.isIdNumber(Number(trimmed))) return warnings; + if (this.isIdNumber(numericValue)) return warnings; // If it's not a string. if (!this.isString(trimmed)) { warnings.push("Provided value is not an email or ID-like value (e.g., 1234 or '1234')."); return warnings; } warnings.push(...this.checkIfEmailValid(trimmed)); return warnings; },
283-285: Improve error source determination.The current method of determining the error source is somewhat simplified. Errors can come from other sources beyond API responses and internal code.
Consider a more detailed error classification:
- const thrower = error?.response?.status - ? "API response" - : "Internal Code"; + let thrower = "Internal Code"; + + if (error?.response?.status) { + thrower = `API response (${error.response.status})`; + } else if (error?.code === 'ECONNREFUSED' || error?.code === 'ENOTFOUND') { + thrower = "Network connectivity"; + } else if (error?.code === 'ETIMEDOUT') { + thrower = "Request timeout"; + }
1-302: Overall good structure but consider adding common validation interfaces.The utility module provides a comprehensive set of validation and helper functions. However, it would be beneficial to introduce some higher-level validation interfaces that combine multiple checks for common scenarios.
Consider adding a high-level validation method that combines related checks:
validateContactInput(input) { const warnings = []; // Check required fields if (!input.email && !input.id) { warnings.push("Either email or id must be provided for contact operations"); } // Validate email if provided if (input.email) { warnings.push(...this.checkIfEmailValid(input.email)); } // Validate id if provided if (input.id && !this.isIdNumber(Number(input.id))) { warnings.push("Contact ID must be a positive integer"); } return warnings; },This would help standardize validation across different actions that operate on contacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
components/drift/actions/create-contact/create-contact.mjs(1 hunks)components/drift/actions/delete-contact/delete-contact.mjs(1 hunks)components/drift/actions/get-contact/get-contact.mjs(1 hunks)components/drift/actions/update-contact/update-contact.mjs(1 hunks)components/drift/common/methods.mjs(1 hunks)components/drift/common/utils.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/package.json(1 hunks)components/drift/sources/new-contact-update/new-contact-update.mjs(1 hunks)components/drift/sources/new-conversation-instant/new-conversation-instant.mjs(1 hunks)components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs(1 hunks)components/drift/sources/new-message-instant/new-message-instant.mjs(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/drift/common/utils.mjs
[error] 19-19: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
components/drift/drift.app.mjs
[error] 113-113: This code is unreachable
... because either this statement ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
🔇 Additional comments (4)
components/drift/package.json (1)
3-3: Version increment looks appropriateThe package version has been properly incremented from 0.6.0 to 0.7.0, which aligns with the significant feature additions in this PR (new actions and sources for the Drift integration).
components/drift/actions/create-contact/create-contact.mjs (1)
45-52: LGTM: Validation setup is well implementedThe email validation setup and warnings collection are well implemented.
components/drift/sources/new-message-instant/new-message-instant.mjs (1)
65-65:❓ Verification inconclusive
Verify the ID field used for event deduplication.
You're using
body.data.endUserIdas the event ID, but this field may not exist in new_message events. Consider using the message ID if available.
🏁 Script executed:
#!/bin/bash # Check Drift webhook documentation to verify fields in new_message events curl -s https://devdocs.drift.com/docs/webhook-events-1 | grep -A 20 "new_message"Length of output: 102475
🏁 Script executed:
#!/bin/bash # Fetch and extract the JSON schema for the Message model from Drift docs curl -s https://devdocs.drift.com/docs/message-model \ | sed -n 's/"/"/g' \ | sed -n -n '/<code class="rdmd-code lang-json/,/<\/code>/p' \ | sed 's/<[^>]*>//g'Length of output: 160
Confirm Message payload ID field for deduplication
Please verify whether the
new_messagewebhook’sdataobject actually includes anendUserIdfield. The docs show thatdatafollows the Message model but don’t listendUserId—you may need to fall back to the message’s unique ID instead.• Location: components/drift/sources/new-message-instant/new-message-instant.mjs:65
• Suggested change (after confirming available fields):- id: body.data.endUserId, + id: body.data.id || body.data.messageId,components/drift/sources/new-conversation-instant/new-conversation-instant.mjs (1)
84-85: Ensure numeric sort when IDs are strings
a.id - b.idgivesNaNifidis a string. Cast explicitly:-conversations.sort((a, b) => a.id - b.id); +conversations.sort((a, b) => Number(a.id) - Number(b.id));
components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs
Outdated
Show resolved
Hide resolved
components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs
Outdated
Show resolved
Hide resolved
components/drift/sources/new-message-instant/new-message-instant.mjs
Outdated
Show resolved
Hide resolved
components/drift/sources/new-message-instant/new-message-instant.mjs
Outdated
Show resolved
Hide resolved
components/drift/sources/new-conversation-instant/new-conversation-instant.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/drift/drift.app.mjs (2)
45-46: Add fallback error handling for missing OAuth tokenSimilar to the issue in the
_makeRequestmethod, the OAuth token should have appropriate error handling here.- "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, + "Authorization": `Bearer ${this.$auth?.oauth_access_token ?? (() => { + throw new Error("Drift OAuth access token is required"); + })()}`,
32-34: 🛠️ Refactor suggestionAdd fallback error handling for missing OAuth token
The current implementation doesn't handle the case where
this.$auth?.oauth_access_tokenis undefined. This could lead to silent failures with unclear error messages.- "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, + "Authorization": `Bearer ${this.$auth?.oauth_access_token ?? (() => { + throw new Error("Drift OAuth access token is required"); + })()}`,
🧹 Nitpick comments (4)
components/drift/drift.app.mjs (4)
40-48: Reduce duplication by leveraging existing_makeRequestmethodThe
getNextPagemethod duplicates the authorization header logic that already exists in_makeRequest. This could lead to inconsistencies if authentication requirements change.- getNextPage($, url) { - return axios($, { - method: "GET", - url, - headers: { - "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, - }, - }); - }, + getNextPage($, url) { + return axios($, { + method: "GET", + url, + headers: { + "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, + }, + }); + },Alternatively, you could modify
_makeRequestto accept a full URL instead of constructing it from base URL and path, then use it for both cases.
124-127: Remove unnecessary semicolonFor consistency with the rest of the codebase, remove the unnecessary semicolon after the closing brace.
if (!response?.data?.length) { throw new Error(`No contact found with email: ${email}`); - }; + }
133-134: Fix typo in variable nameThe variable name "firtsNew" appears to be a typo for "firstNew".
- const firtsNew = arr.indexOf(lastKnown); - if (firtsNew === -1) throw new Error("Id not found"); + const firstNew = arr.indexOf(lastKnown); + if (firstNew === -1) throw new Error("Id not found");
8-138: Add JSDoc documentation for improved code readabilityThe methods in this file lack documentation. Adding JSDoc comments would improve code clarity and maintainability, especially for other developers who might need to work with this code in the future.
For example, add documentation to the
getContactByEmailOrIdmethod:/** * Retrieves a contact by either email or ID * @param {Object} $ - Pipedream event object * @param {string|number} emailOrId - Either an email address or a contact ID * @returns {Promise<Object>} - The contact data * @throws {Error} If no contact is found or if there's an API error */ async getContactByEmailOrId($, emailOrId) { // Existing implementation }Consider adding similar documentation to all methods in this file.
🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: This code is unreachable
... because either this statement ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
components/drift/actions/get-contact/get-contact.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-contact-update/new-contact-update.mjs(1 hunks)components/drift/sources/new-conversation-instant/new-conversation-instant.mjs(1 hunks)components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs(1 hunks)components/drift/sources/new-message-instant/new-message-instant.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- components/drift/sources/new-lead-instance.mjs/new-lead-instance.mjs
- components/drift/sources/new-message-instant/new-message-instant.mjs
- components/drift/actions/get-contact/get-contact.mjs
- components/drift/sources/new-conversation-instant/new-conversation-instant.mjs
- components/drift/sources/new-contact-update/new-contact-update.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
components/drift/drift.app.mjs
[error] 113-113: This code is unreachable
... because either this statement ...
... or this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/drift/sources/new-lead-instance/new-lead-instance.mjs (2)
3-12: Component setup looks good but consider adding webhook setup instructionsThe component is well-structured with appropriate key, name, and description. The document link is helpful for reference.
However, since this is a webhook-based source, consider adding setup instructions either in the description or as a separate
hookssection to guide users on how to configure the webhook in Drift.
32-36: Update the summary message for consistencyThe summary message indicates an update ("Contact X updated"), but this component is designed to trigger when a new lead adds their email in chat (as stated in the description).
this.$emit(body, { - summary: `Contact "${email}" ID "${contactId}" updated`, + summary: `New lead identified: "${email}" (ID: "${contactId}")`, id: body.data.endUserId, ts: body.timeStamp, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/drift/sources/new-lead-instance/new-lead-instance.mjs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (1)
components/drift/sources/new-lead-instance/new-lead-instance.mjs (1)
17-20: Good event filtering logicThe event filtering logic is clear and appropriately logs and ignores non-relevant events.
components/drift/sources/new-lead-instance/new-lead-instance.mjs
Outdated
Show resolved
Hide resolved
components/drift/sources/new-lead-instance/new-lead-instance.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/drift/drift.app.mjs (2)
107-118: Remove unnecessary semicolonsThere are unnecessary semicolons at the end of blocks that should be removed for consistency.
if (!response?.data?.length) { throw new Error(`No contact found with email: ${email}`); - }; - }; + } + }
123-128: Fix typo in variable nameThere's a typo in the variable name at line 124 -
firtsNewshould befirstNew.- const firtsNew = arr.indexOf(lastKnown); + const firstNew = arr.indexOf(lastKnown); if (firtsNew === -1) throw new Error("Id not found"); - const newest = arr.slice(0, firtsNew); + const newest = arr.slice(0, firstNew); return newest.reverse();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/drift/actions/create-contact/create-contact.mjs(1 hunks)components/drift/actions/delete-contact/delete-contact.mjs(1 hunks)components/drift/actions/get-contact/get-contact.mjs(1 hunks)components/drift/actions/update-contact/update-contact.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-contact-update/new-contact-update.mjs(1 hunks)components/drift/sources/new-conversation-instant/new-conversation-instant.mjs(1 hunks)components/drift/sources/new-lead-instance/new-lead-instance.mjs(1 hunks)components/drift/sources/new-message-instant/new-message-instant.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- components/drift/sources/new-message-instant/new-message-instant.mjs
- components/drift/actions/update-contact/update-contact.mjs
- components/drift/actions/create-contact/create-contact.mjs
- components/drift/actions/get-contact/get-contact.mjs
- components/drift/sources/new-contact-update/new-contact-update.mjs
- components/drift/actions/delete-contact/delete-contact.mjs
- components/drift/sources/new-lead-instance/new-lead-instance.mjs
- components/drift/sources/new-conversation-instant/new-conversation-instant.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (6)
components/drift/drift.app.mjs (6)
8-29: HTTP request method implementation looks goodThe
_baseUrl()and_makeRequest()methods follow best practices for Pipedream apps with proper authorization header handling and flexible parameter passing.
31-39: Well-structured pagination supportThe
getNextPage()method properly implements pagination support for the Drift API, maintaining authorization headers consistency.
41-84: CRUD operations for contacts are well-implementedAll contact management methods (getContactByEmail, createContact, updateContact, getContactById, deleteContactById) follow a consistent pattern with appropriate HTTP methods and paths.
90-105: Good error handling for contact lookup by IDThe error handling for contact lookup by ID is well-implemented with specific error messages for 404 responses.
129-141: Well-implemented JSON parsing utilityThe
parseIfJSONStringmethod safely handles JSON parsing with proper error handling, returning the original input if parsing fails.
142-144: Simple and effective ID validation methodThe
isIdNumbermethod provides a clean way to validate if an input is a positive integer ID.
luancazarine
left a comment
luancazarine
left a comment
There was a problem hiding this comment.
Hello @SokolovskyiK, I just added some suggestions for changes. Also, I have some considerations to make about sources.
In order for a source to be considered (Instant), the API (Drift) must provide an endpoint so that, within the activate() method, a webhook is created where we pass our URL (this.http.endpoint) to receive the events in real time.
As well as an endpoint so that we can, within the deactivate() method, delete this webhook.
Then the run() method will only receive these events and list them through $emit.
You can use this file as an example:
https://github.com/PipedreamHQ/pipedream/blob/master/components/everhour/sources/common/base.mjs
If the API does not provide these endpoints, you must create a polling source that's the same as the others you've created.
Please ask if you have any questions.
| export default { | ||
| key: "drift-create-contact", | ||
| name: "Create Contact", | ||
| description: "Creates a contact in Drift. [See the docs](https://devdocs.drift.com/docs/creating-a-contact).", |
There was a problem hiding this comment.
| description: "Creates a contact in Drift. [See the docs](https://devdocs.drift.com/docs/creating-a-contact).", | |
| description: "Creates a contact in Drift. [See the documentation](https://devdocs.drift.com/docs/creating-a-contact).", |
| export default { | ||
| key: "drift-delete-contact", | ||
| name: "Delete Contact", | ||
| description: "Deletes a contact in Drift by ID or email. [See the docs](https://devdocs.drift.com/docs/removing-a-contact).", |
There was a problem hiding this comment.
| description: "Deletes a contact in Drift by ID or email. [See the docs](https://devdocs.drift.com/docs/removing-a-contact).", | |
| description: "Deletes a contact in Drift by ID or email. [See the documentation](https://devdocs.drift.com/docs/removing-a-contact).", |
| export default { | ||
| key: "drift-get-contact", | ||
| name: "Get Contact", | ||
| description: "Retrieves a contact in Drift by ID or email. [See the docs](https://devdocs.drift.com/docs/retrieving-contact)", |
There was a problem hiding this comment.
| description: "Retrieves a contact in Drift by ID or email. [See the docs](https://devdocs.drift.com/docs/retrieving-contact)", | |
| description: "Retrieves a contact in Drift by ID or email. [See the documentation](https://devdocs.drift.com/docs/retrieving-contact)", |
| export default { | ||
| key: "drift-update-contact", | ||
| name: "Update Contact", | ||
| description: "Updates a contact in Drift using ID or email. Only changed attributes will be updated. [See Drift API docs](https://devdocs.drift.com/docs/updating-a-contact)", |
There was a problem hiding this comment.
| description: "Updates a contact in Drift using ID or email. Only changed attributes will be updated. [See Drift API docs](https://devdocs.drift.com/docs/updating-a-contact)", | |
| description: "Updates a contact in Drift using ID or email. Only changed attributes will be updated. [See the documentation](https://devdocs.drift.com/docs/updating-a-contact)", |
| _baseUrl() { | ||
| return "https://driftapi.com"; | ||
| }, | ||
|
|
| export default { | ||
| key: "drift-new-contact-update", | ||
| name: "New Contact Update", | ||
| description: "Emit new event when a contact is updated in Drift. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", |
There was a problem hiding this comment.
| description: "Emit new event when a contact is updated in Drift. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", | |
| description: "Emit new event when a contact is updated in Drift. [See the documentation](https://devdocs.drift.com/docs/webhook-events-1).", |
| export default { | ||
| key: "drift-new-conversation-instant", | ||
| name: "New Conversation", | ||
| description: "Emit new when a new conversation is started in Drift. [See the docs](https://devdocs.drift.com/docs/retrieve-a-conversation)", |
There was a problem hiding this comment.
| description: "Emit new when a new conversation is started in Drift. [See the docs](https://devdocs.drift.com/docs/retrieve-a-conversation)", | |
| description: "Emit new when a new conversation is started in Drift. [See the documentation](https://devdocs.drift.com/docs/retrieve-a-conversation)", |
| export default { | ||
| key: "drift-new-lead-instance", | ||
| name: "New Lead", | ||
| description: "Emit new event when a contact adds their email in chat. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", |
There was a problem hiding this comment.
| description: "Emit new event when a contact adds their email in chat. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", | |
| description: "Emit new event when a contact adds their email in chat. [See the documentation](https://devdocs.drift.com/docs/webhook-events-1).", |
| export default { | ||
| key: "drift-new-message-instant", | ||
| name: "New Message", | ||
| description: "Emit new event when a new message is received in Drift. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", |
There was a problem hiding this comment.
| description: "Emit new event when a new message is received in Drift. [See the docs](https://devdocs.drift.com/docs/webhook-events-1).", | |
| description: "Emit new event when a new message is received in Drift. [See the documentation](https://devdocs.drift.com/docs/webhook-events-1).", |
|
Hi @luancazarine, The task #13240 involves creating polling sources, but it’s over a year old. I have this question because I was a bit confused by this line from your comment:
All my other sources are webhook-based, not polling. I assume that was just a typo, and you meant: Please correct me if I misunderstood. Also I checked the documentation here: https://devdocs.drift.com/docs/webhook-events-1 And no mention of suitable endpoints. So just to confirm — should I go ahead and rewrite this polling source as a webhook-based source and remove "instant" from name, like the others? Thank you for your time! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/drift/sources/new-lead/new-lead.mjs (1)
23-23: Fix typo in summary message.There's a spelling error in the summary text.
- summary: `New lead. Contact ID "${body.id}" proveded their email`, + summary: `New lead. Contact ID "${body.id}" provided their email`,components/drift/sources/new-conversation/new-conversation.mjs (1)
6-6: Fix typo in component description.There's a spelling error in the description text.
- description: "Emit new event when a new convesation is started in Drift. [See the documentation](https://devdocs.drift.com/docs/webhook-events-1).", + description: "Emit new event when a new conversation is started in Drift. [See the documentation](https://devdocs.drift.com/docs/webhook-events-1).",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/drift/actions/create-contact/create-contact.mjs(1 hunks)components/drift/actions/delete-contact/delete-contact.mjs(1 hunks)components/drift/actions/get-contact/get-contact.mjs(1 hunks)components/drift/actions/update-contact/update-contact.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-contact-update/new-contact-update.mjs(1 hunks)components/drift/sources/new-conversation/new-conversation.mjs(1 hunks)components/drift/sources/new-lead/new-lead.mjs(1 hunks)components/drift/sources/new-message/new-message.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- components/drift/actions/create-contact/create-contact.mjs
- components/drift/actions/update-contact/update-contact.mjs
- components/drift/sources/new-contact-update/new-contact-update.mjs
- components/drift/actions/delete-contact/delete-contact.mjs
- components/drift/actions/get-contact/get-contact.mjs
- components/drift/drift.app.mjs
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
components/drift/sources/new-message/new-message.mjs (1)
25-63: LGTM with suggestions above!The overall structure and logic flow are solid. The component correctly filters events, enriches data, and emits meaningful summaries. The webhook implementation follows Pipedream conventions well.
components/drift/sources/new-lead/new-lead.mjs (1)
13-27: LGTM! Clean and focused implementation.The logic is straightforward and correctly filters for the target event type. The webhook implementation follows best practices for simple event processing.
components/drift/sources/new-conversation/new-conversation.mjs (1)
13-27: LGTM! Consistent and well-structured implementation.The component follows the same clean pattern as the other sources, correctly filtering events and emitting meaningful data. The webhook implementation is solid.
|
Hi @luancazarine |
Hi @SokolovskyiK, I apologize if I didn't make myself clear. Let me know if you have any questions |
|
Hi @luancazarine |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
components/drift/common/utils.mjs (1)
1-23: Typo and performance issues already identifiedThe previous review correctly identified the typo in
isNotEmpyStringand the O(n²) performance issue with spread operator in the reducer.🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
components/drift/sources/new-message/new-message.mjs (1)
44-48: Error handling for API calls already identifiedThe previous review correctly identified the need for error handling around the drift API calls.
components/drift/drift.app.mjs (1)
117-117: Remove unnecessary semicolonThe previous review already identified this issue.
🧹 Nitpick comments (9)
components/drift/common/utils.mjs (1)
28-28: Remove extra space before parenthesis for consistency- throw new Error ("Message context is not an object"); + throw new Error("Message context is not an object");components/drift/sources/new-message/new-message.mjs (1)
46-46: Remove debug console.log statementThis appears to be debug code that should be removed before production.
- console.log(messageContext);components/drift/sources/new-conversation/new-conversation.mjs (3)
7-7: Fix grammatical error in description- description: "Emit new when a new conversation is started in Drift. [See the documentations](https://devdocs.drift.com/docs/retrieve-a-conversation)", + description: "Emit new event when a new conversation is started in Drift. [See the documentation](https://devdocs.drift.com/docs/retrieve-a-conversation)",
67-67: Remove extra space before parenthesis for consistency- throw new Error ("lastConversation not found in fetched data. Skipping emit."); + throw new Error("lastConversation not found in fetched data. Skipping emit.");
78-78: Clarify the summary messageThe summary should be clearer about what ID is being referenced.
- summary: `New conversation with ID ${conversations[i].contactId}`, + summary: `New conversation with contact ID ${conversations[i].contactId}`,components/drift/drift.app.mjs (4)
24-24: Clean up comment artifactsRemove the trailing comment artifacts.
- "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, //"Bearer iHlC8LmFQiTH0DcWds7ETMRMmo3BvUyP", // , + "Authorization": `Bearer ${this.$auth?.oauth_access_token}`,
124-124: Fix typo in variable name- const firtsNew = arr.indexOf(lastKnown); + const firstNew = arr.indexOf(lastKnown);
125-125: Use consistent capitalization for "ID"- if (firtsNew === -1) throw new Error("Id not found"); + if (firstNew === -1) throw new Error("ID not found");
126-126: Update variable reference after fixing typo- const newest = arr.slice(0, firtsNew); + const newest = arr.slice(0, firstNew);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/drift/actions/create-contact/create-contact.mjs(1 hunks)components/drift/actions/delete-contact/delete-contact.mjs(1 hunks)components/drift/actions/get-contact/get-contact.mjs(1 hunks)components/drift/actions/update-contact/update-contact.mjs(1 hunks)components/drift/common/utils.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-conversation/new-conversation.mjs(1 hunks)components/drift/sources/new-message/new-message.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/drift/actions/create-contact/create-contact.mjs
- components/drift/actions/update-contact/update-contact.mjs
- components/drift/actions/get-contact/get-contact.mjs
- components/drift/actions/delete-contact/delete-contact.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
components/drift/common/utils.mjs
[error] 19-19: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
SokolovskyiK
force-pushed
the
drift
branch
2 times, most recently
from
51bc713 to
d4723d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
components/drift/common/utils.mjs (3)
8-8: Fix typo in variable name.The variable name has a typo that affects readability and consistency.
- const isNotEmpyString = typeof value === "string" && value.trim() !== ""; + const isNotEmptyString = typeof value === "string" && value.trim() !== "";
17-17: Fix typo in variable reference.This references the same misspelled variable from line 8.
- (isNotEmpyString || isNotEmptyArray || isNotEmptyObject || isBoolean || isNumber)) + (isNotEmptyString || isNotEmptyArray || isNotEmptyObject || isBoolean || isNumber))
19-21: Optimize performance by avoiding spread operator in reducer.Using the spread operator in a reducer causes O(n²) time complexity, which is inefficient for large objects. This was flagged by static analysis tools.
- ? { - ...acc, - [key]: value, - } + ? Object.assign(acc || {}, { [key]: value }) : acc;🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
components/drift/drift.app.mjs (1)
117-118: Remove unnecessary semicolons after closing braces.These semicolons create unreachable code and violate JavaScript syntax conventions.
if (!response?.data?.length) { throw new Error(`No contact found with email: ${email}`); - }; - }; + } + }
🧹 Nitpick comments (2)
components/drift/common/utils.mjs (1)
28-29: Fix formatting and unnecessary semicolon.Remove the space before the semicolon and the unnecessary semicolon after the if statement closing brace.
- throw new Error ("Message context is not an object"); - }; + throw new Error("Message context is not an object"); + }components/drift/drift.app.mjs (1)
124-124: Fix typo in variable name.The variable name has a typo that affects code readability.
- const firtsNew = arr.indexOf(lastKnown); + const firstNew = arr.indexOf(lastKnown);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/drift/common/utils.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-conversation/new-conversation.mjs(1 hunks)components/drift/sources/new-message/new-message.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/drift/sources/new-message/new-message.mjs
- components/drift/sources/new-conversation/new-conversation.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
components/drift/common/utils.mjs
[error] 19-19: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ 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 (5)
components/drift/common/utils.mjs (1)
25-42: Function logic is correct and well-implemented.The
doesContextMatchfunction properly validates input context against fetched context with appropriate error handling and logging.components/drift/drift.app.mjs (4)
24-24: OAuth access token security is properly implemented.The authorization headers now correctly use only the OAuth access token without any hard-coded fallback credentials, addressing the previous security concern.
Also applies to: 36-36
129-148: Pagination logic is well-implemented.The
getMessagesByConvIdmethod properly handles pagination with a do-while loop and accumulates all messages correctly.
150-165: Utility methods are robust and well-designed.Both
parseIfJSONStringandisIdNumbermethods have appropriate error handling and validation logic.
123-128:❓ Verification inconclusive
Verify array bounds and error handling in
getNewestConversations.The function correctly handles the case where
lastKnownID is not found, but consider if the reversed slice logic matches the intended behavior for conversation ordering.
🏁 Script executed:
#!/bin/bash # Description: Check how this function is used to understand the expected behavior ast-grep --pattern $'getNewestConversations($_, $_)'Length of output: 53
To locate all call sites of
getNewestConversationsand confirm its real‐world usage, let’s do a repository-wide grep:
🏁 Script executed:
#!/bin/bash # Search for any invocation or reference of getNewestConversations rg "getNewestConversations" -n .Length of output: 114
Verify getNewestConversations Usage and Ordering
- We couldn’t locate any call sites for
getNewestConversationsin the repo – please confirm it’s actually used (or remove it if dead code).- Double-check that taking
arr.slice(0, indexOf(lastKnown))then reversing yields the intended “newest-first” ordering.- Add unit tests for key cases:
•lastKnownat index 0 (should return[])
•lastKnownin the middle
• duplicate IDs
• nonexistent ID (error thrown)- Fix the typo: rename
firtsNew→firstNewfor clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/drift/common/utils.mjs (2)
8-8: Fix typo in variable nameThe variable name has a typo:
isNotEmpyStringshould beisNotEmptyString.Also applies to: 17-17
19-19: Avoid spread operator in reduce accumulator for performanceUsing the spread operator in a reduce accumulator causes O(n²) time complexity. Consider using
Object.assignor direct property assignment instead.🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🧹 Nitpick comments (3)
components/drift/common/utils.mjs (1)
28-29: Consider more descriptive error messageThe error message could be more specific about what constitutes a valid message context.
- throw new Error ("Message context is not an object"); + throw new Error("Message context must be a non-null object (not an array)");components/drift/drift.app.mjs (2)
31-39: Consider consolidating pagination logicThe
getNextPagemethod duplicates header logic from_makeRequest. Consider using_makeRequestfor consistency.getNextPage($, url) { - return axios($, { - method: "GET", - url, - headers: { - "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, - }, - }); + return axios($, { + method: "GET", + url, + headers: { + "Authorization": `Bearer ${this.$auth?.oauth_access_token}`, + }, + }); },
117-118: Remove unnecessary semicolonsUnnecessary semicolons after closing braces should be removed for cleaner code.
if (!response?.data?.length) { throw new Error(`No contact found with email: ${email}`); - }; - }; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/drift/common/utils.mjs(1 hunks)components/drift/drift.app.mjs(1 hunks)components/drift/sources/new-conversation/new-conversation.mjs(1 hunks)components/drift/sources/new-message/new-message.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/drift/sources/new-message/new-message.mjs
- components/drift/sources/new-conversation/new-conversation.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
components/drift/common/utils.mjs
[error] 19-19: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
🔇 Additional comments (7)
components/drift/common/utils.mjs (1)
25-42: LGTM! Well-structured context validation functionThe
doesContextMatchfunction provides clear validation logic with appropriate error handling and logging for debugging purposes.components/drift/drift.app.mjs (6)
8-10: LGTM! Clean base URL methodThe base URL method is properly implemented with a consistent return value.
12-29: LGTM! Well-structured request method with proper authenticationThe
_makeRequestmethod properly handles authentication using OAuth tokens without hardcoded fallbacks, which addresses the security concern from previous reviews.
41-84: LGTM! Comprehensive CRUD operations for contactsThe contact methods are well-structured with proper HTTP method usage and parameter handling.
129-148: LGTM! Efficient pagination implementation for messagesThe message retrieval method properly handles pagination and collects all messages efficiently.
150-162: LGTM! Robust JSON parsing utilityThe
parseIfJSONStringmethod handles JSON parsing gracefully with proper error handling.
163-165: LGTM! Clear ID validation utilityThe
isIdNumbermethod provides clear validation for positive integers.
|
Hi @luancazarine Please note: There is no longer a create-or-update endpoint — I created "create" and "update" as separate actions. Regarding new-lead-instant: Drift does not support listing contacts — only fetching them by known ID or email. |
luancazarine
left a comment
luancazarine
left a comment
There was a problem hiding this comment.
Hi @SokolovskyiK, LGTM! Ready for QA!
4 participants
Fixes #13240
Actions:
create-contact
delete-contact
get-contact
update-contact
Sources:
new-contact-update
new-conversation-instant
new-lead-instance
new-message-instant
Notes:
This task is over a year old. Since then, the Drift API has changed.
There is no longer a create-or-update endpoint — now create and update are separate actions.
Regarding new-lead-instant:
Drift does not support listing contacts — only fetching them by known ID or email.
However, their API now supports webhooks. So I implemented this source using webhooks instead of polling. Unfortunately, there's no "new contact" webhook event available.
To work around this, I created two sources:
new-lead-instance: Emits an event when a contact adds their email in chat.
new-contact-update: Emits an event when a contact is updated.
Polling vs Webhooks:
new-conversation-instant is currently implemented as a polling source.
If needed, I can rewrite it to use webhooks instead.
Summary by CodeRabbit
New Features
Improvements
Chores