[Components] engage #13410 #15656
[Components] engage #13410 #15656
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Context
participant A as Action Module
participant E as Engage App Module
U->>A: Trigger action (e.g., create user, add customer, add event)
A->>E: Call corresponding API method
E-->>A: Return response data
A-->>U: Return summary message and response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
components/engage/engage.app.mjs (1)
41-50: Avoid duplicate code in options functions.The
optionsfunctions inuidandcustomerIdproperties are identical. Consider extracting this into a shared method.+ methods: { + async _getUserOptions() { + const response = await this.getUsers(); + const usersIds = response.data; + return usersIds.map(({ + uid, first_name, last_name, + }) => ({ + label: `${first_name} ${last_name}`, + value: uid, + })); + }, }, propDefinitions: { uid: { // ... async options() { - const response = await this.getUsers(); - const usersIds = response.data; - return usersIds.map(({ - uid, first_name, last_name, - }) => ({ - label: `${first_name} ${last_name}`, - value: uid, - })); + return this._getUserOptions(); }, }, customerId: { // ... async options() { - const response = await this.getUsers(); - const usersIds = response.data; - return usersIds.map(({ - uid, first_name, last_name, - }) => ({ - label: `${first_name} ${last_name}`, - value: uid, - })); + return this._getUserOptions(); }, }, },Also applies to: 56-65
components/engage/actions/add-event/add-event.mjs (1)
36-47: Add timestamp validation and default handling.The timestamp field should be validated if provided, or default to current time if not provided.
async run({ $ }) { + const eventData = { + event: this.event, + properties: this.properties, + }; + + if (this.timestamp) { + // Validate timestamp format + if (!Date.parse(this.timestamp)) { + throw new Error("Invalid timestamp format"); + } + eventData.timestamp = this.timestamp; + } + const response = await this.app.addEvent({ $, uid: this.uid, - data: { - event: this.event, - timestamp: this.timestamp, - properties: this.properties, - }, + data: eventData, });
📜 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 (7)
components/engage/.gitignore(0 hunks)components/engage/actions/add-customer/add-customer.mjs(1 hunks)components/engage/actions/add-event/add-event.mjs(1 hunks)components/engage/actions/create-user/create-user.mjs(1 hunks)components/engage/app/engage.app.ts(0 hunks)components/engage/engage.app.mjs(1 hunks)components/engage/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- components/engage/app/engage.app.ts
- components/engage/.gitignore
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
🔇 Additional comments (2)
components/engage/package.json (2)
3-5: Version and Main Entry Update Validation
- The package version has been updated to
"0.1.0", which signifies a new release. Please ensure that any breaking changes or new features introduced are documented in the changelog or release notes.- The
"main"entry point has been updated to"engage.app.mjs". Confirm that this file exists in the project directory and that all module consumers are updated accordingly.
14-16: PublishConfig and Dependencies Block Review
- The
"publishConfig"block now correctly terminates, ensuring proper package publishing settings.- The new dependency
"@pipedream/platform": "^3.0.3"is added. Make sure that this dependency is intended for use in the project and is compatible with the rest of the codebase.
| async _makeRequest(opts = {}) { | ||
| const { | ||
| $ = this, | ||
| path, | ||
| auth, | ||
| ...otherOpts | ||
| } = opts; | ||
| return axios($, { | ||
| ...otherOpts, | ||
| url: this._baseUrl() + path, | ||
| auth: { | ||
| ...auth, | ||
| username: `${this.$auth.public_key}`, | ||
| password: `${this.$auth.secret_key}`, | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and request validation.
The _makeRequest method should include:
- Error handling for network failures
- Request validation
- Rate limiting considerations
async _makeRequest(opts = {}) {
const {
$ = this,
path,
auth,
...otherOpts
} = opts;
+ if (!path) {
+ throw new Error("Path is required");
+ }
+ try {
return axios($, {
...otherOpts,
url: this._baseUrl() + path,
auth: {
...auth,
username: `${this.$auth.public_key}`,
password: `${this.$auth.secret_key}`,
},
});
+ } catch (error) {
+ const errorMessage = error.response?.data?.message || error.message;
+ throw new Error(`Request failed: ${errorMessage}`);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
components/engage/engage.app.mjs
Outdated
| properties: { | ||
| type: "object", | ||
| label: "Properties", | ||
| description: "The properties of the event", | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding schema validation for properties object.
The properties field accepts any object structure. Consider adding schema validation to ensure the properties match expected format.
🌐 Web query:
What is the expected format for event properties in Engage.so?
💡 Result:
In Engage.so, event properties are structured as key-value pairs associated with user actions, allowing granular tracking and segmentation. Here's the breakdown based on available documentation:
Event Properties Format
-
Event Structure
Each event consists of:- Event Name: A descriptive label for the user action (e.g.,
button_clicked,order_placed). - Properties: Customizable key-value pairs providing context (e.g.,
plan_type: "premium",button_color: "blue").
- Event Name: A descriptive label for the user action (e.g.,
-
Data Types
Events support dynamic property values, including:- Strings (
"success") - Numbers (
42) - Booleans (
true/false) - Timestamps (automatically recorded for each event)[10].
- Strings (
-
Usage Guidelines
- Timestamps: Automatically attached to events for historical analysis[10].
- Overwriting Behavior: Unlike attributes (which store only the latest value), events retain all historical instances[10].
- Segmentation: Properties enable filtering users based on past actions (e.g., "users who clicked ‘Upgrade’ in the last 30 days")[10].
Example Implementation
// Tracking a plan upgrade event with properties
Engage.track('plan_upgraded', {
plan_name: "Pro",
previous_plan: "Basic",
payment_method: "credit_card"
});Key Differences from Attributes
| Feature | Events | Attributes |
|---|---|---|
| Storage | Historical records with timestamps[10] | Latest value only[10] |
| Use Case | Tracking actions over time | Storing static user traits |
| Example | plan_upgraded with details |
current_plan: "Pro" |
For optimal results, use events when you need to analyze temporal patterns or count occurrences (e.g., logins per week), and attributes for fixed user states (e.g., subscription tier)[10].
Citations:
- 1: https://support.higherlogic.com/hc/en-us/articles/360036208371-Define-Event-Properties
- 2: https://www.brightedge.com/blog/live-event-structure-data-markup
- 3: https://knowledgebase.webengage.com/docs/events-and-event-attributes
- 4: https://segment.com/docs/engage/using-engage-data/
- 5: https://developers.google.com/search/docs/appearance/structured-data/event
- 6: https://docs.precisely.com/docs/sftw/e1-compose-designer/6.6.11/en-us/pdf/engageone-designer-v6-6-11-users-guide.pdf
- 7: https://support.pendo.io/hc/en-us/articles/7710433678619-Event-properties
- 8: https://segment.com/docs/connections/sources/catalog/cloud-apps/engage-events/
- 9: https://developers.moengage.com/hc/en-us/articles/12513847534868-Tracking-Events
- 10: https://docs.engage.so/en-us/a/62d2bf84a168cece13b23cb3-understanding-the-data-typesattributes-and-events
Action Required: Enhance Schema Validation for Event Properties
The current definition for the properties field only specifies it as an object without enforcing the key-value structure demanded by Engage.so. According to the documentation, event properties should be dynamic key-value pairs with values limited to strings, numbers, booleans, or timestamps. Please add schema validation (for example, using JSON Schema with appropriate patternProperties or additionalProperties constraints) to ensure that properties strictly adhere to this format.
- Location:
components/engage/engage.app.mjs(lines 77-81) - Action: Enhance the validation logic for the
propertiesobject by enforcing allowed types and structure.
| async run({ $ }) { | ||
| const response = await this.app.createUser({ | ||
| $, | ||
| data: { | ||
| id: this.userId, | ||
| first_name: this.firstName, | ||
| last_name: this.lastName, | ||
| is_account: this.isAccount, | ||
| number: this.number, | ||
| email: this.email, | ||
| }, | ||
| }); | ||
| $.export("$summary", `Successfully created user with ID: ${response.id}`); | ||
| return response; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The run method should validate inputs and handle potential errors from the API call.
async run({ $ }) {
+ // Validate required fields
+ if (!this.userId || !this.email) {
+ throw new Error("User ID and email are required fields");
+ }
+
+ try {
const response = await this.app.createUser({
$,
data: {
id: this.userId,
first_name: this.firstName,
last_name: this.lastName,
is_account: this.isAccount,
number: this.number,
email: this.email,
},
});
$.export("$summary", `Successfully created user with ID: ${response.id}`);
return response;
+ } catch (error) {
+ throw new Error(`Failed to create user: ${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.
| async run({ $ }) { | |
| const response = await this.app.createUser({ | |
| $, | |
| data: { | |
| id: this.userId, | |
| first_name: this.firstName, | |
| last_name: this.lastName, | |
| is_account: this.isAccount, | |
| number: this.number, | |
| email: this.email, | |
| }, | |
| }); | |
| $.export("$summary", `Successfully created user with ID: ${response.id}`); | |
| return response; | |
| }, | |
| async run({ $ }) { | |
| // Validate required fields | |
| if (!this.userId || !this.email) { | |
| throw new Error("User ID and email are required fields"); | |
| } | |
| try { | |
| const response = await this.app.createUser({ | |
| $, | |
| data: { | |
| id: this.userId, | |
| first_name: this.firstName, | |
| last_name: this.lastName, | |
| is_account: this.isAccount, | |
| number: this.number, | |
| email: this.email, | |
| }, | |
| }); | |
| $.export("$summary", `Successfully created user with ID: ${response.id}`); | |
| return response; | |
| } catch (error) { | |
| throw new Error(`Failed to create user: ${error.message}`); | |
| } | |
| }, |
| async run({ $ }) { | ||
| const response = await this.app.addCustomer({ | ||
| $, | ||
| uid: this.uid, | ||
| data: { | ||
| accounts: [ | ||
| { | ||
| id: this.customerId, | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| $.export("$summary", "add-customer executed successfully"); | ||
| return response; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for customer ID and improve error handling.
The run method should validate inputs and handle potential errors from the API call.
async run({ $ }) {
+ if (!this.uid || !this.customerId) {
+ throw new Error("User ID and Customer ID are required");
+ }
+
+ try {
const response = await this.app.addCustomer({
$,
uid: this.uid,
data: {
accounts: [
{
id: this.customerId,
},
],
},
});
$.export("$summary", "add-customer executed successfully");
return response;
+ } catch (error) {
+ throw new Error(`Failed to add customer: ${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.
| async run({ $ }) { | |
| const response = await this.app.addCustomer({ | |
| $, | |
| uid: this.uid, | |
| data: { | |
| accounts: [ | |
| { | |
| id: this.customerId, | |
| }, | |
| ], | |
| }, | |
| }); | |
| $.export("$summary", "add-customer executed successfully"); | |
| return response; | |
| }, | |
| async run({ $ }) { | |
| if (!this.uid || !this.customerId) { | |
| throw new Error("User ID and Customer ID are required"); | |
| } | |
| try { | |
| const response = await this.app.addCustomer({ | |
| $, | |
| uid: this.uid, | |
| data: { | |
| accounts: [ | |
| { | |
| id: this.customerId, | |
| }, | |
| ], | |
| }, | |
| }); | |
| $.export("$summary", "add-customer executed successfully"); | |
| return response; | |
| } catch (error) { | |
| throw new Error(`Failed to add customer: ${error.message}`); | |
| } | |
| }, |
| key: "engage-add-event", | ||
| name: "Add Event", | ||
| description: "Adds user events to Engage. [See the documentation](https://docs.engage.so/en-us/a/62bbdd015bfea4dca4834042-users#track-user-event)", | ||
| version: "0.0.21", |
There was a problem hiding this comment.
| version: "0.0.21", | |
| version: "0.0.1", |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
components/engage/engage.app.mjs (6)
37-51: Consider renaminguidproperty for clarity.The property is labeled as "User ID", which is duplicative with the
userIdproperty defined earlier. Consider renaming touserUidto better differentiate between these identifiers and improve code readability.
72-77: Add timestamp format validation.The
timestampis defined as a string without any format validation. Consider adding a description of the expected format (e.g., ISO 8601) and potentially adding validation to ensure the timestamp is properly formatted.
118-126: Validate uid parameter in addCustomer method.The
addCustomermethod doesn't validate that theuidparameter is provided before attempting to use it in the path. Consider adding validation to ensure the required parameter is present.async addCustomer({ uid, ...args }) { + if (!uid) { + throw new Error("User ID is required"); + } return this._makeRequest({ path: `/users/${uid}/accounts`, method: "post", ...args, }); }
127-135: Validate uid parameter in addEvent method.Similar to the
addCustomermethod, theaddEventmethod should validate theuidparameter before using it in the API path.async addEvent({ uid, ...args }) { + if (!uid) { + throw new Error("User ID is required"); + } return this._makeRequest({ path: `/users/${uid}/events`, method: "post", ...args, }); }
111-117: Consider adding data validation in createUser method.The
createUsermethod forwards all arguments directly to_makeRequestwithout any validation. Consider adding validation for required fields such as email or user ID.async createUser(args = {}) { + const { data } = args; + if (data && (!data.email && !data.user_id)) { + throw new Error("Either email or user_id is required"); + } return this._makeRequest({ path: "/users", method: "post", ...args, }); }
136-141: Consider adding pagination handling in getUsers method.The
getUsersmethod doesn't handle pagination, which could be an issue if there are many users. Consider adding pagination support to fetch all users if needed.async getUsers(args = {}) { return this._makeRequest({ path: "/users", + params: { + limit: 100, + ...args.params, + }, ...args, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/engage/actions/add-event/add-event.mjs(1 hunks)components/engage/engage.app.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/engage/actions/add-event/add-event.mjs
⏰ 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 (4)
components/engage/engage.app.mjs (4)
1-6: Module structure looks good.The app is correctly defined with the appropriate imports and configuration. The module type and app name are properly set up for the Engage integration.
52-66: Verify customerId implementation.The options function for
customerIdfetches and returns user IDs (uids), not customer IDs, despite the property description stating "ID of the customer that will be added to the user". This could lead to confusion or incorrect data being passed to the API.Either:
- Update the options implementation to fetch actual customer IDs, or
- Clarify in the description that this represents a user ID that will be linked as a customer.
78-83: Add schema validation for properties object.The
propertiesfield accepts any object structure without validation. Consider adding schema validation to ensure the properties match the expected format required by the Engage API.According to Engage.so documentation, event properties should be key-value pairs with values limited to strings, numbers, booleans, or timestamps. Consider adding validation to enforce this structure.
94-110: Add error handling and request validation to _makeRequest method.The
_makeRequestmethod lacks error handling for network failures, request validation, and rate limiting considerations as previously suggested.Add path validation and try-catch block to handle potential errors:
async _makeRequest(opts = {}) { const { $ = this, path, auth, ...otherOpts } = opts; + if (!path) { + throw new Error("Path is required"); + } + try { return axios($, { ...otherOpts, url: this._baseUrl() + path, auth: { ...auth, username: `${this.$auth.public_key}`, password: `${this.$auth.secret_key}`, }, }); + } catch (error) { + const errorMessage = error.response?.data?.message || error.message; + throw new Error(`Request failed: ${errorMessage}`); + } }
|
/approve |
WHY
Summary by CodeRabbit
New Features
Chores