[Components] scalr #13481 #16663
[Components] scalr #13481 #16663
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe changes replace the previous minimal Scalr app integration with a new implementation featuring API methods for webhook creation, removal, and account retrieval. A common webhook management module is added, along with three new source components that emit events on specific run statuses. The package metadata is updated, and the old app file and Changes
Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 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)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/scalr/package.json (1)
15-17: Add runtime dependency on @pipedream/platform.
This dependency is required for the component runtime. If you expect the host to provide@pipedream/platform, you may want to declare it as apeerDependencyinstead of bundling it.Here’s a diff to switch it to a peerDependency:
- "dependencies": { - "@pipedream/platform": "^3.0.3" - } + "peerDependencies": { + "@pipedream/platform": "^3.0.3" + }
📜 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 (2)
components/scalr/app/scalr.app.ts(0 hunks)components/scalr/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- components/scalr/app/scalr.app.ts
⏰ 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 (3)
components/scalr/package.json (3)
14-14: PublishConfig block closure restored.
The missing closing brace for thepublishConfigobject has been added correctly.
5-5:❓ Verification inconclusive
Ensure the new entry point exists post-build.
The"main"field now points toscalr.app.mjsat the package root—verify that your build step emits this file in the package, and consider adding a"files"array to restrict published contents.You can confirm its presence with:
🏁 Script executed:
fd scalr.app.mjsLength of output: 18
Verify new entry point and locked‐down package contents
We weren’t able to locatescalr.app.mjsin the repo—please ensure your build step emits this file at the package root (or update"main"to the correct path if it lands elsewhere). Also consider adding a"files"array to limit what’s published.• Run your build and confirm the file exists:
npm run build fd scalr.app.mjs• If the output lands in a subfolder (e.g.
dist/), update the"main"field accordingly.
• Add a"files"entry tocomponents/scalr/package.json, for example:"files": [ "scalr.app.mjs", "dist/" ]
3-3:❓ Verification inconclusive
Confirm the version downgrade intent.
The version was changed from0.0.2to0.0.1—ensure this aligns with your release strategy and doesn’t accidentally overwrite a newer release on npm.Run the following to list published versions:
🏁 Script executed:
npm view @pipedream/scalr versionsLength of output: 57
Confirm version downgrade intent
The published versions for@pipedream/scalrare0.0.1and0.0.2. The version in components/scalr/package.json has been downgraded from0.0.2to0.0.1. Please confirm this is intentional and won’t accidentally overwrite a newer release on npm.
There was a problem hiding this comment.
Hi @lcaresia, I think you forgot to include the components in this PR.
Thanks @michelle0927! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
components/scalr/sources/new-run-completed/new-run-completed.mjs (1)
3-28: Component implementation looks good with room for data validationThe component correctly extends the common module and properly implements the required methods. The emission logic with unique ID generation and summary creation is well-structured.
Consider adding validation for the data structure before accessing properties:
async emitEvent(data) { + if (!data?.run) { + console.log("Received invalid webhook data", data); + return; + } this.$emit(data, { id: data.run.id, summary: `New run completed with ID ${data.run.id}`, ts: Date.parse(data.run["created-at"]), }); },This would prevent potential errors if the webhook payload doesn't match the expected structure.
components/scalr/sources/common/common.mjs (4)
3-70: Well-structured common module with appropriate abstractionThe common module provides a solid foundation for Scalr webhook-based components with proper lifecycle management and abstract methods that enforce implementation.
A few observations:
- The webhook naming pattern
"Webhook Pipedream - " + new Date().toISOString()ensures uniqueness but will create many webhooks if components are reactivated frequently- The error message in
emitEventmethod includes the event parameter but doesn't use it correctlyFix the error message in the
emitEventmethod:emitEvent(event) { - throw new Error("emitEvent is not implemented", event); + throw new Error("emitEvent is not implemented"); },The second argument to Error constructor is not used - JavaScript Error doesn't accept the data as a second parameter.
30-61: Webhook creation looks good but could benefit from error handlingThe webhook creation logic correctly sets up the webhook with the Scalr API and stores the webhook ID.
Consider adding error handling to the webhook creation process:
async activate() { - const { data } = await this.scalr.createWebhook({ - data: { - "data": { - "attributes": { - "max-attempts": 3, - "timeout": 15, - "name": "Webhook Pipedream - " + new Date().toISOString(), - "url": this.http.endpoint, - }, - "relationships": { - "account": { - "data": { - "type": "accounts", - "id": this.accountId, - }, - }, - "events": { - "data": [ - { - "type": "event-definitions", - "id": this.getWebhookEventType(), - }, - ], - }, - }, - "type": "webhook-integrations", - }, - }, - }); - this._setWebhookId(data.id); + try { + const { data } = await this.scalr.createWebhook({ + data: { + "data": { + "attributes": { + "max-attempts": 3, + "timeout": 15, + "name": "Webhook Pipedream - " + new Date().toISOString(), + "url": this.http.endpoint, + }, + "relationships": { + "account": { + "data": { + "type": "accounts", + "id": this.accountId, + }, + }, + "events": { + "data": [ + { + "type": "event-definitions", + "id": this.getWebhookEventType(), + }, + ], + }, + }, + "type": "webhook-integrations", + }, + }, + }); + this._setWebhookId(data.id); + } catch (err) { + console.error("Failed to create webhook", err); + throw err; + } },
62-65: Webhook deactivation should include error handlingThe deactivation logic correctly retrieves and removes the webhook, but lacks error handling.
Add error handling to gracefully handle webhook removal failures:
async deactivate() { const webhookId = this._getWebhookId(); - await this.scalr.removeWebhook(webhookId); + if (!webhookId) { + console.log("No webhook ID found to remove"); + return; + } + try { + await this.scalr.removeWebhook(webhookId); + } catch (err) { + console.error("Failed to remove webhook", err); + // Don't throw the error to allow component deactivation to complete + } },
67-69: Run method should include error handlingThe run method passes the webhook payload to the emitEvent method but doesn't include error handling.
Add error handling to prevent component crashes:
async run(event) { - await this.emitEvent(event.body); + try { + await this.emitEvent(event.body); + } catch (err) { + console.error("Failed to emit event", err); + } },components/scalr/scalr.app.mjs (2)
21-40: API request handling is properly structuredThe base URL construction and request handling are well-implemented with proper authentication and content type headers.
One small improvement would be to add error handling and more detailed error information:
async _makeRequest(opts = {}) { const { $ = this, path, headers, ...otherOpts } = opts; - return axios($, { - ...otherOpts, - url: this._baseUrl() + path, - headers: { - "Authorization": `Bearer ${this.$auth.api_token}`, - "Content-type": "application/vnd.api+json", - ...headers, - }, - }); + try { + return await axios($, { + ...otherOpts, + url: this._baseUrl() + path, + headers: { + "Authorization": `Bearer ${this.$auth.api_token}`, + "Content-type": "application/vnd.api+json", + ...headers, + }, + }); + } catch (error) { + const statusCode = error.response?.status; + const statusText = error.response?.statusText; + const responseData = error.response?.data; + throw new Error(`Scalr API request failed: ${statusCode} ${statusText}. Path: ${path}. ${JSON.stringify(responseData)}`); + } },
41-72: API methods are well-organized and consistentThe API methods for webhook management, account fetching, and event retrieval are correctly implemented and follow a consistent pattern.
A minor enhancement would be to add JSDoc comments for better code documentation:
Add JSDoc comments to document method parameters and return values:
+ /** + * Create a new webhook integration + * @param {Object} args - Request arguments + * @returns {Promise<Object>} Response containing the created webhook data + */ async createWebhook({ ...args }) { return this._makeRequest({ path: "/integrations/webhooks", method: "post", ...args, }); }, + /** + * Remove a webhook integration by ID + * @param {string} webhookId - The ID of the webhook to remove + * @returns {Promise<Object>} Response from the delete operation + */ async removeWebhook(webhookId) { return this._makeRequest({ path: `/integrations/webhooks/${webhookId}`, method: "delete", }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
components/scalr/.gitignore(0 hunks)components/scalr/scalr.app.mjs(1 hunks)components/scalr/sources/common/common.mjs(1 hunks)components/scalr/sources/new-run-completed/new-run-completed.mjs(1 hunks)components/scalr/sources/new-run-errored/new-run-errored.mjs(1 hunks)components/scalr/sources/new-run-needs-attention/new-run-needs-attention.mjs(1 hunks)
💤 Files with no reviewable changes (1)
- components/scalr/.gitignore
✅ Files skipped from review due to trivial changes (2)
- components/scalr/sources/new-run-errored/new-run-errored.mjs
- components/scalr/sources/new-run-needs-attention/new-run-needs-attention.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 (1)
components/scalr/scalr.app.mjs (1)
3-19: App definition and account option fetching are well-implementedThe Scalr app integration correctly defines its type, identifier, and properties. The accountId property with dynamic options fetching provides a good user experience.
components/scalr/sources/new-run-completed/new-run-completed.mjs
Outdated
Show resolved
Hide resolved
components/scalr/sources/new-run-needs-attention/new-run-needs-attention.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/scalr/scalr.app.mjs (2)
45-51: Consider adding input validation for webhook creation.While the implementation works, consider adding validation for required webhook parameters to prevent API errors.
async createWebhook({ ...args }) { + // Validate required fields for webhook creation + const { data } = args; + if (!data || !data.attributes || !data.attributes.url) { + throw new Error("Webhook URL is required"); + } return this._makeRequest({ path: "/integrations/webhooks", method: "post", ...args, }); }
52-57: Add error handling for webhook removal.Consider adding basic error handling to catch and provide meaningful feedback if the webhook deletion fails.
async removeWebhook(webhookId) { + if (!webhookId) { + throw new Error("Webhook ID is required"); + } return this._makeRequest({ path: `/integrations/webhooks/${webhookId}`, method: "delete", }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/scalr/scalr.app.mjs(1 hunks)components/scalr/sources/new-run-completed/new-run-completed.mjs(1 hunks)components/scalr/sources/new-run-errored/new-run-errored.mjs(1 hunks)components/scalr/sources/new-run-needs-attention/new-run-needs-attention.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/scalr/sources/new-run-errored/new-run-errored.mjs
- components/scalr/sources/new-run-completed/new-run-completed.mjs
- components/scalr/sources/new-run-needs-attention/new-run-needs-attention.mjs
🔇 Additional comments (5)
components/scalr/scalr.app.mjs (5)
11-21: Pagination implementation in options looks good.The implementation correctly uses pagination with the
page[number]parameter for fetching accounts, which aligns with the Scalr API documentation. This ensures efficient data loading when there are many accounts.
25-27: Appropriate dynamic base URL construction.The
_baseUrlmethod correctly constructs the API URL using the authenticated domain, making the integration flexible across different Scalr environments.
28-44: Well-structured request method with appropriate headers.The
_makeRequestmethod is well-implemented with:
- Proper authorization header using the API token
- Correct content type for JSON API
- Flexible parameter handling allowing for customization
This provides a solid foundation for all API interactions.
3-65: Overall well-structured Scalr app implementation.The app implementation provides a good foundation for Scalr integration with:
- Clear property definitions
- Consistent API interaction methods
- Proper authentication handling
A few minor suggestions for improvements have been noted in specific sections.
58-63:❓ Verification inconclusive
Consider adding a method to retrieve webhooks.
There's no method to get existing webhooks. If this functionality is needed elsewhere in the component, consider adding a
getWebhooksmethod.
🏁 Script executed:
#!/bin/bash # Check if getWebhooks method is needed by searching for references in component files echo "Searching for potential uses of getWebhooks in other files..." rg -A 3 -B 3 "getWebhooks" --glob "components/scalr/**/*.{mjs,js,ts,tsx}" echo "Searching for webhook retrieval patterns that might need this method..." rg -A 3 -B 3 "\.get.*webhook" --glob "components/scalr/**/*.{mjs,js,ts,tsx}"Length of output: 899
No existing
getWebhooksHTTP methodI didn’t find any
getWebhooksimplementation in components/scalr/scalr.app.mjs (or references elsewhere)—only the internal_getWebhookId/_setWebhookIdhelpers in components/scalr/sources/common/common.mjs. If your integration needs to list configured webhooks via the Scalr API, consider adding agetWebhooksmethod inscalr.app.mjs.
3 participants
WHY
Summary by CodeRabbit