-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[Components] crowdpower #13454 #19471
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
base: master
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
WalkthroughAdds three Crowdpower action modules (create charge, create event, upsert customer), extends the Crowdpower app with HTTP request helpers and API wrapper methods, and updates package metadata (version and dependency). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-12-12T19:23:09.039ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
components/crowdpower/actions/create-customer-charge/create-customer-charge.mjs(1 hunks)components/crowdpower/actions/create-customer-event/create-customer-event.mjs(1 hunks)components/crowdpower/actions/upsert-customer/upsert-customer.mjs(1 hunks)components/crowdpower/crowdpower.app.mjs(1 hunks)components/crowdpower/package.json(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-12-12T19:23:09.039Z
Learnt from: jcortes
Repo: PipedreamHQ/pipedream PR: 14935
File: components/sailpoint/package.json:15-18
Timestamp: 2024-12-12T19:23:09.039Z
Learning: When developing Pipedream components, do not add built-in Node.js modules like `fs` to `package.json` dependencies, as they are native modules provided by the Node.js runtime.
Applied to files:
components/crowdpower/package.json
📚 Learning: 2024-10-30T15:24:39.294Z
Learnt from: jcortes
Repo: PipedreamHQ/pipedream PR: 14467
File: components/gainsight_px/actions/create-account/create-account.mjs:4-6
Timestamp: 2024-10-30T15:24:39.294Z
Learning: In `components/gainsight_px/actions/create-account/create-account.mjs`, the action name should be "Create Account" instead of "Create Memory".
Applied to files:
components/crowdpower/actions/create-customer-charge/create-customer-charge.mjscomponents/crowdpower/actions/create-customer-event/create-customer-event.mjs
📚 Learning: 2025-06-04T17:52:05.780Z
Learnt from: GTFalcao
Repo: PipedreamHQ/pipedream PR: 16954
File: components/salesloft/salesloft.app.mjs:14-23
Timestamp: 2025-06-04T17:52:05.780Z
Learning: In the Salesloft API integration (components/salesloft/salesloft.app.mjs), the _makeRequest method returns response.data which directly contains arrays for list endpoints like listPeople, listCadences, listUsers, and listAccounts. The propDefinitions correctly call .map() directly on these responses without needing to destructure a nested data property.
Applied to files:
components/crowdpower/crowdpower.app.mjs
📚 Learning: 2025-09-15T22:01:11.472Z
Learnt from: GTFalcao
Repo: PipedreamHQ/pipedream PR: 18362
File: components/leonardo_ai/actions/generate-image/generate-image.mjs:103-105
Timestamp: 2025-09-15T22:01:11.472Z
Learning: In Pipedream components, pipedream/platform's axios implementation automatically excludes undefined values from HTTP requests, so there's no need to manually check for truthiness before including properties in request payloads.
Applied to files:
components/crowdpower/crowdpower.app.mjs
🧬 Code graph analysis (2)
components/crowdpower/actions/upsert-customer/upsert-customer.mjs (2)
components/crowdpower/actions/create-customer-charge/create-customer-charge.mjs (1)
response(30-36)components/crowdpower/actions/create-customer-event/create-customer-event.mjs (1)
response(37-44)
components/crowdpower/actions/create-customer-charge/create-customer-charge.mjs (2)
components/crowdpower/actions/create-customer-event/create-customer-event.mjs (1)
response(37-44)components/crowdpower/actions/upsert-customer/upsert-customer.mjs (1)
response(43-51)
🪛 GitHub Actions: Components Checks
components/crowdpower/package.json
[error] 1-1: Lockfile specifiers do not match package.json: the lockfile has {} while package.json requires "@pipedream/platform":"^1.6.8".
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pnpm publish
🔇 Additional comments (3)
components/crowdpower/package.json (1)
3-3: Version bump OK, but ensure it matches your release intent (0.0.4 → 0.1.0).Confirm this is intended as a minor bump (new actions + app surface). Also confirm any internal component registry/versioning expectations for Pipedream components.
components/crowdpower/crowdpower.app.mjs (1)
42-57: Confirm@pipedream/platformaxios behavior (return shape + non-2xx errors) matches action expectations.Actions rely on
response.success/response.code. Please verifyaxios($, config)returns the API JSON body (not an AxiosResponse wrapper), and whether it throws on non-2xx (in which case action summaries won’t run unless wrapped).components/crowdpower/actions/upsert-customer/upsert-customer.mjs (1)
16-22: The current implementation is correct as-is. TheuserIdparameter cannot be optional because it is embedded in the API endpoint path:PUT /customers/${user_id}. WithoutuserId, the API call would fail with an invalid endpoint (e.g.,/customers/undefined).The review comment misunderstands the action's purpose. This action is designed for updating/upserting an existing customer identified by their
userId. If the Crowdpower API supports true create-or-update semantics via thePUT /customers/{userId}endpoint, thenuserIdremains required. If the intended use case is to create customers without pre-existing IDs, a separate endpoint or action logic would be needed, but the current implementation correctly uses the endpoint as designed.
components/crowdpower/actions/create-customer-charge/create-customer-charge.mjs
Show resolved
Hide resolved
components/crowdpower/actions/create-customer-event/create-customer-event.mjs
Show resolved
Hide resolved
| _baseUrl() { | ||
| return "https://beacon.crowdpower.io/"; | ||
| }, | ||
| async _makeRequest(opts = {}) { | ||
| const { | ||
| $ = this, | ||
| path, | ||
| headers, | ||
| ...otherOpts | ||
| } = opts; | ||
| return axios($, { | ||
| ...otherOpts, | ||
| url: this._baseUrl() + path, | ||
| headers: { | ||
| "Authorization": `Bearer ${this.$auth.application_key}`, | ||
| ...headers, | ||
| }, | ||
| }); |
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.
Avoid https://...//path (double slash) by fixing base URL / path join.
Right now _baseUrl() ends with / and path begins with /, producing // in the final URL. Safer to join with new URL() (and drop the trailing slash).
_baseUrl() {
- return "https://beacon.crowdpower.io/";
+ return "https://beacon.crowdpower.io";
},
async _makeRequest(opts = {}) {
const {
$ = this,
path,
headers,
...otherOpts
} = opts;
+ if (!path) {
+ throw new Error("Missing required `path` in _makeRequest()");
+ }
return axios($, {
...otherOpts,
- url: this._baseUrl() + path,
+ url: new URL(path, this._baseUrl()).toString(),
headers: {
"Authorization": `Bearer ${this.$auth.application_key}`,
...headers,
},
});
},📝 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.
| _baseUrl() { | |
| return "https://beacon.crowdpower.io/"; | |
| }, | |
| async _makeRequest(opts = {}) { | |
| const { | |
| $ = this, | |
| path, | |
| headers, | |
| ...otherOpts | |
| } = opts; | |
| return axios($, { | |
| ...otherOpts, | |
| url: this._baseUrl() + path, | |
| headers: { | |
| "Authorization": `Bearer ${this.$auth.application_key}`, | |
| ...headers, | |
| }, | |
| }); | |
| _baseUrl() { | |
| return "https://beacon.crowdpower.io"; | |
| }, | |
| async _makeRequest(opts = {}) { | |
| const { | |
| $ = this, | |
| path, | |
| headers, | |
| ...otherOpts | |
| } = opts; | |
| if (!path) { | |
| throw new Error("Missing required `path` in _makeRequest()"); | |
| } | |
| return axios($, { | |
| ...otherOpts, | |
| url: new URL(path, this._baseUrl()).toString(), | |
| headers: { | |
| "Authorization": `Bearer ${this.$auth.application_key}`, | |
| ...headers, | |
| }, | |
| }); | |
| }, |
🤖 Prompt for AI Agents
In components/crowdpower/crowdpower.app.mjs around lines 39 to 56, the
_baseUrl() returns a string with a trailing slash and _makeRequest concatenates
it with path which may start with a leading slash, producing a double-slash in
the final URL; fix by returning the base URL without the trailing slash or by
normalizing the join using new URL(path, this._baseUrl()) so the path and base
are combined safely (or strip a leading slash from path before concatenation)
and ensure axios receives a single well-formed URL.
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.
Btw the CodeRabbit comment here is correct, please fix the base URL or the endpoints, they're generating a double slash.
7246007 to
1bec5cc
Compare
GTFalcao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But I did leave one comment regarding summaries that may need to be improved.
| $.export("$summary", response.success | ||
| ? `Request succeeded with code ${response.code}` | ||
| : `Request failed with code ${response.code}`); |
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.
Are the summaries intended to be like this? Usually we use more specific wording in each action, rather than a generic "request succeeded" or "request failed"
WHY
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.