Cloudinary usability improvements #15091
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe pull request introduces comprehensive updates to the Cloudinary integration, focusing on improving the usability and functionality of various actions. The changes span multiple files, including actions for account usage, resource retrieval, image and resource transformations, and media asset uploads. Key modifications include enhanced error handling, clearer property descriptions, updated version numbers, and streamlined configuration options. The package dependency has also been updated to a newer version of the Cloudinary library. Changes
Assessment against linked issues
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: 1
🧹 Nitpick comments (9)
components/cloudinary/actions/resource-transformation/resource-transformation.mjs (4)
17-21: Consider validating thewidthvalue
Currently, there's no minimum value. Negative or zero widths might cause unexpected results.width: { type: "integer", label: "Width", + min: 1, description: "The new width of the video, e.g. `854`", optional: true, },
23-28: Consider validating theheightvalue
Similarly, you might want to enforce a positive integer to avoid invalid transformations.
29-32: Add a minimum or range forduration
Consider rejecting negative or excessively large values to prevent errors or resource misuse.
52-54: Rename summary for clarity
Currently logs "Successfully transformed video," but the action also covers audio. Consider a more inclusive message.- $.export("$summary", "Successfully transformed video"); + $.export("$summary", "Successfully transformed media asset");components/cloudinary/actions/image-transformation/image-transformation.mjs (2)
17-22: Validatewidth
Similar to the video transformation, consider a minimum or range for more robust handling.
23-28: Validateheight
Likewise, ensureheightremains positive or within expected bounds.components/cloudinary/actions/upload-media-asset/upload-media-asset.mjs (2)
69-72: Support advanced parameters inadditionalOptions
Consider stronger validations or typed approaches to reduce the risk of passing invalid data.
86-101: Graceful handling of partially valid JSON
Parsing eachadditionalOptionsvalue individually is flexible, but be aware that partial parse errors revert to raw strings, possibly leading to ambiguous states.components/cloudinary/actions/get-resources/get-resources.mjs (1)
76-97: Robust error handling and pagination implementationThe changes improve error handling and implement proper cursor-based pagination with respect to maxResults. However, consider adding specific error handling for common Cloudinary API errors.
Consider expanding the error handling to provide more specific error messages for common Cloudinary API errors:
catch (err) { - throw new Error(`Cloudinary error response: ${err.error?.message ?? JSON.stringify(err)}`); + const errorMessage = err.error?.message ?? JSON.stringify(err); + if (err.http_code === 401) { + throw new Error(`Authentication failed. Please verify your Cloudinary credentials. Details: ${errorMessage}`); + } + if (err.http_code === 429) { + throw new Error(`Rate limit exceeded. Please try again later. Details: ${errorMessage}`); + } + throw new Error(`Cloudinary error response: ${errorMessage}`); }
📜 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/cloudinary/actions/get-account-usage-details/get-account-usage-details.mjs(2 hunks)components/cloudinary/actions/get-resources/get-resources.mjs(3 hunks)components/cloudinary/actions/image-transformation/image-transformation.mjs(1 hunks)components/cloudinary/actions/resource-transformation/resource-transformation.mjs(1 hunks)components/cloudinary/actions/upload-media-asset/upload-media-asset.mjs(4 hunks)components/cloudinary/cloudinary.app.mjs(3 hunks)components/cloudinary/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/cloudinary/package.json
🔇 Additional comments (34)
components/cloudinary/actions/resource-transformation/resource-transformation.mjs (5)
5-7: Clearer naming and version bump
These changes appropriately clarify that the action targets video or audio, and the version bump is well-aligned with the new focus.
11-14: Verify references to the replaced resourceType
Ensure that any prior references to resourceType have been fully replaced by assetId throughout the codebase.
35-40: Flexible transformations property
Allowing custom transformations is powerful. Continue to ensure that conflicting options (e.g., width in both options and transformations) are handled consistently.
43-50: Watch for key collisions in spread objects
Merging options and transformations might lead to overwriting. Confirm that overwriting is intentional when names collide.
56-59: Error rethrow approach
This enhanced error message is consistent. No further issues found.
components/cloudinary/actions/image-transformation/image-transformation.mjs (9)
5-7: Revised name, description, and version
The downgrade to 0.1.0 might be intentional, but confirm alignment with other dependent actions if any rely on a higher version.
11-16: Property assetId
Switching from a custom image source property to assetId is cleaner. Confirm that all prior references to the old property are updated.
29-33: Background color usage
This is an optional string. Looks good. No further issues.
35-42: Opacity property with boundaries
Defining min: 0 and max: 100 is a solid safety measure.
43-47: transformations object
This property is consistent with the new approach. Looks good.
51-53: Potential collision between options and transformations
As with the resource transformation, confirm overwriting is intended.
55-58: Consolidated transform call
Using transformAsset is consistent with the new unified design.
60-62: Accurate success message
"Successfully transformed image" is appropriate here.
64-67: Enhanced error reporting
This matches the error handling in other files for consistency.
components/cloudinary/actions/upload-media-asset/upload-media-asset.mjs (7)
6-7: Updated description and major version
Increasing to 1.0.0 acknowledges backward-incompatible changes or a new stable release.
11-15: infoAlert for user guidance
This is helpful messaging, ensuring users know where to find advanced options.
18-19: Clarified file path or URL
Allowing both local and remote sources is flexible.
24-24: Simpler description for publicId
Good clarity for referencing the asset in Cloudinary.
52-52: Changed tags to string[]
This improves type safety for multiple tags.
59-60: Optional format conversion
Clear instructions for converting assets before storing.
110-111: Consistent error structure
Error message matches the pattern in other actions.
components/cloudinary/actions/get-account-usage-details/get-account-usage-details.mjs (4)
6-7: Refreshed description and version
This better conveys what is retrieved.
11-15: dateInfo alert
Helpful note for users about default behavior.
19-19: Stricter date format
Requiring yyyy-mm-dd is clearer and aligns with typical APIs.
28-39: Improved error handling
Wrapping the call in try-catch ensures user-friendly error responses.
components/cloudinary/cloudinary.app.mjs (7)
11-11: Expanded description for resourceType
Noting .mp3 as a video resource is valuable clarification.
34-35: Label change to "Filter by Type"
More accurately reflects its usage.
43-43: Extended uploadDeliveryType description
Clarifies usage of private or authenticated modes.
51-51: accessMode property
Helps configure assets to behave like authenticated or public. Looks good.
55-59: New assetId property
Allows referencing the asset's public ID consistently across multiple actions.
60-64: Added transformations property
This centralizes transformation parameters, making them reusable.
82-83: transformAsset returns a URL
Returning a transformation URL is a common Cloudinary pattern. Confirm that calling actions handle this as expected, since no server-side transformation occurs until the resource is accessed.
components/cloudinary/actions/get-resources/get-resources.mjs (2)
7-7: Version bump is appropriate
The minor version increment (0.0.1 → 0.1.0) correctly reflects the addition of new features and improvements while maintaining backward compatibility.
25-26: Property documentation improvements enhance usability
The updated labels and descriptions are more intuitive and provide clearer guidance to users. The consistent pattern of "Include X" for boolean flags and "Filter by X" for filtering options improves the API's usability.
Also applies to: 31-32, 38-41, 52-53
| metadata: { | ||
| type: "boolean", | ||
| label: "Include Metadata", | ||
| description: "Whether to include the structured metadata fields and values assigned to each asset.", | ||
| default: false, | ||
| optional: true, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add metadata property to options object
The review comment is correct. The metadata property is defined in the component's props but not included in the options object passed to the Cloudinary API call. Here are the specific locations that need attention:
- In
components/cloudinary/actions/get-resources/get-resources.mjs, add themetadataproperty to the options object in therunmethod:
const options = {
resource_type: this.resourceType,
type: this.type,
prefix: this.prefix,
tags: this.tags,
context: this.context,
moderation: this.moderation,
metadata: this.metadata,
};🔗 Analysis chain
Verify metadata property integration with Cloudinary API
The new metadata property is well-documented, but it needs to be included in the options object passed to the Cloudinary API.
Add the metadata property to the options object in the run method:
const options = {
resource_type: this.resourceType,
type: this.type,
prefix: this.prefix,
tags: this.tags,
context: this.context,
moderation: this.moderation,
+ metadata: this.metadata,
};Let me verify if the metadata property is properly integrated with the Cloudinary API by checking the implementation.
Let me fix the regex and try again.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the implementation of the run method in the get-resources component
rg -A 20 "run\s*\(\s*\)\s*{" components/cloudinary/actions/get-resources/get-resources.mjs
Length of output: 190
Script:
#!/bin/bash
# Check the implementation of the run method in the get-resources component
rg "async run" -A 20 components/cloudinary/actions/get-resources/get-resources.mjs
Length of output: 674
There was a problem hiding this comment.
Hi @GTFalcao, I just added a suggestion about a version but I'm moving it to Ready For QA anyway
| version: "0.1.2", | ||
| name: "Transform Image", | ||
| description: "Transform an image asset on-the-fly with several options. [See the documentation](https://cloudinary.com/documentation/image_transformations)", | ||
| version: "0.1.0", |
There was a problem hiding this comment.
| version: "0.1.0", | |
| version: "0.2.0", |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/cloudinary/cloudinary.app.mjs (1)
107-108: Consider renaming the parameter for clarity and consistency.Currently, the method signature is
async transformAsset(imageSource, options). Since it can handle images, videos, and audio, renamingimageSourceto something likeassetIdorpublicIdimproves code clarity and consistency with the newassetIdprop.Proposed change:
- async transformAsset(imageSource, options) { - return this._client().url(imageSource, options); + async transformAsset(assetId, options) { + return this._client().url(assetId, options); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/cloudinary/actions/image-transformation/image-transformation.mjs(0 hunks)components/cloudinary/actions/resource-transformation/resource-transformation.mjs(0 hunks)components/cloudinary/actions/transform-resource/transform-resource.mjs(1 hunks)components/cloudinary/cloudinary.app.mjs(3 hunks)
💤 Files with no reviewable changes (2)
- components/cloudinary/actions/resource-transformation/resource-transformation.mjs
- components/cloudinary/actions/image-transformation/image-transformation.mjs
🧰 Additional context used
🪛 GitHub Check: Lint Code Base
components/cloudinary/actions/transform-resource/transform-resource.mjs
[warning] 18-18:
Component prop info must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 18-18:
Component prop info must have a description. See https://pipedream.com/docs/components/guidelines/#props
🔇 Additional comments (6)
components/cloudinary/actions/transform-resource/transform-resource.mjs (3)
38-45: Validation logic appears correct.Throwing a
ConfigurationErrorif neither namedTransformation nor transformationString is supplied ensures proper user guidance. This logic aligns with the stated requirement.
48-54: Clear handling of named vs. raw transformations.The code correctly prioritizes the named transformation if both are provided, matching the documentation in the
infoprop. This is consistent and well-implemented.
61-63: All newly introduced props are consistent with Cloudinary’s transformation approach.The usage of inline references to Cloudinary property definitions and dynamic options retrieval for named transformations is well-designed and effectively implemented. Good job!
Also applies to: 66-70, 72-89
components/cloudinary/cloudinary.app.mjs (3)
11-11: Enhanced property descriptions look good.The updates provide clearer guidance for users, especially around resource types for video/audio assets and specialized delivery types.
Also applies to: 34-35, 43-43, 51-51
55-59: New properties for transformations are well-defined.These additions properly align with the transformation logic in the new “Transform Resource” action, providing a versatile approach to Cloudinary transformations.
Also applies to: 60-70, 71-89
113-115: Retrieving named transformations adds valuable configurability.The new
getTransformationsmethod is a direct approach for listing transformations. This approach is clean and maintains alignment with Cloudinary’s API.
| info: { | ||
| type: "alert", | ||
| alertType: "info", | ||
| content: `You can either select a pre-configured transformation or pass a transformation string. Both can be managed in the [Cloudinary Transformation Builder](https://tx.cloudinary.com/). | ||
| \\ | ||
| If both are specified, the transformation string will be ignored.`, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the info prop follows Pipedream guidelines.
Static analysis indicates that component props of type alert must have a label and a description. See docs.
Below is a suggested fix:
info: {
type: "alert",
alertType: "info",
+ label: "Information",
+ description: "Guidance on selecting a named transformation or providing a transformation string.",
content: `You can either select a pre-configured transformation...
If both are specified, the transformation string will be ignored.`,
},📝 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.
| info: { | |
| type: "alert", | |
| alertType: "info", | |
| content: `You can either select a pre-configured transformation or pass a transformation string. Both can be managed in the [Cloudinary Transformation Builder](https://tx.cloudinary.com/). | |
| \\ | |
| If both are specified, the transformation string will be ignored.`, | |
| }, | |
| info: { | |
| type: "alert", | |
| alertType: "info", | |
| label: "Information", | |
| description: "Guidance on selecting a named transformation or providing a transformation string.", | |
| content: `You can either select a pre-configured transformation or pass a transformation string. Both can be managed in the [Cloudinary Transformation Builder](https://tx.cloudinary.com/). | |
| \\ | |
| If both are specified, the transformation string will be ignored.`, | |
| }, |
🧰 Tools
🪛 GitHub Check: Lint Code Base
[warning] 18-18:
Component prop info must have a label. See https://pipedream.com/docs/components/guidelines/#props
[warning] 18-18:
Component prop info must have a description. See https://pipedream.com/docs/components/guidelines/#props
|
/approve |
Closes #14875
Summary by CodeRabbit
New Features
infoAlertandadditionalOptionsfor improved asset upload functionality.transformAssetmethod.Bug Fixes
Version Updates