[Components] cloudflare_browser_rendering - new components #15937
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis pull request introduces several new modules for Cloudflare browser rendering that support multiple actions including fetching HTML, generating PDFs, capturing screenshots, and scraping content. In addition, the app configuration has been expanded to include new browser rendering properties and helper methods for account and request handling. New utility functions and API constants have also been defined, and the package version has been updated with a dependency addition. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GetHTMLContent
participant Endpoint
Client->>GetHTMLContent: Provide HTML/URL & options
GetHTMLContent->>GetHTMLContent: Validate input parameters
GetHTMLContent->>Endpoint: POST /content with payload
Endpoint-->>GetHTMLContent: Return HTML response
GetHTMLContent->>Client: Return summary result
sequenceDiagram
participant Action
participant CloudflareApp
Action->>CloudflareApp: Call post() method
CloudflareApp->>CloudflareApp: Retrieve account ID (getAccountId)
CloudflareApp->>CloudflareApp: Construct URL (getUrl) & get headers (getHeaders)
CloudflareApp->>CloudflareApp: Execute request via _makeRequest
CloudflareApp-->>Action: Return response payload
Action->>Client: Deliver processed result
Assessment against linked issues
Suggested labels
Suggested reviewers
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/cloudflare_browser_rendering/actions/common/base.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs components/cloudflare_browser_rendering/actions/scrape/scrape.mjsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs
✨ Finishing Touches
🪧 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 (4)
components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs (1)
75-81: Consider adding error handling for file operations.The PDF file writing operation lacks try/catch error handling, which could lead to uncaught exceptions if the file system operations fail.
-const pdfPath = path.join("/tmp", fileName); -fs.writeFileSync(pdfPath, response, "binary"); +const pdfPath = path.join("/tmp", fileName); +try { + fs.writeFileSync(pdfPath, response, "binary"); +} catch (error) { + throw new Error(`Failed to write PDF file: ${error.message}`); +}components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs (1)
43-63: Ensure graceful handling of malformed JSON inadditionalSettings.While
utils.parseJsonpresumably throws or returns safe values on parse errors, consider clarifying or documenting how it handles invalid JSON strings. It may be worth catching or explicitly logging parse failures to aid users.components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (1)
187-193: Saving screenshot as a file in /tmp.Writing the file in binary mode looks correct. Consider adding error handling in case the API call returns non-image data or fails. Exporting the file path with a meaningful summary is helpful for referencing in subsequent steps.
components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (1)
7-68: Comprehensive property definitions with clear descriptionsThe property definitions are well-structured with clear labels and descriptions. However, I noticed both
htmlandurlare marked as optional, but the descriptions indicate that at least one must be provided.Consider adding runtime validation to ensure either
htmlorurlis provided when these properties are used. This could be implemented in action methods that use these properties:// Example validation in an action method executeAction(params) { + if (!params.html && !params.url) { + throw new Error("Either HTML or URL must be provided"); + } // Rest of the method }
📜 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 (9)
components/cloudflare_browser_rendering/actions/common/base.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs(1 hunks)components/cloudflare_browser_rendering/actions/scrape/scrape.mjs(1 hunks)components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs(1 hunks)components/cloudflare_browser_rendering/common/constants.mjs(1 hunks)components/cloudflare_browser_rendering/common/utils.mjs(1 hunks)components/cloudflare_browser_rendering/package.json(2 hunks)
⏰ 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 (21)
components/cloudflare_browser_rendering/package.json (2)
3-3: Version bump looks appropriate.The version change from 0.0.1 to 0.1.0 follows semantic versioning principles, correctly indicating the addition of new features while maintaining backward compatibility.
14-17: Dependency addition looks good.The addition of @pipedream/platform as a dependency is appropriate for the new functionality being introduced in the Cloudflare browser rendering components.
components/cloudflare_browser_rendering/actions/common/base.mjs (1)
1-73: Well-organized common props structure.This base module creates a reusable set of properties for Cloudflare Browser Rendering actions, promoting consistency and reducing duplication across components. The eslint-disable comment is appropriately used for the info property that doesn't require standard label and description attributes.
components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs (3)
7-30: Action definition and PDF method look good.The action is well-defined with appropriate properties and a clear method for interacting with the Cloudflare PDF endpoint.
31-53: Validation logic is solid.The validation checks ensure either HTML or URL is provided and that viewport dimensions are properly specified when used.
54-74: API request structure looks correct.Good use of conditional spreading for viewport properties and parsing of additional settings.
components/cloudflare_browser_rendering/actions/scrape/scrape.mjs (4)
5-24: Action definition and props look good.The scrape action is well-defined with appropriate properties. The default for the elements prop (selecting the body element) provides a sensible starting point for users.
25-31: Scrape method implementation is correct.The method properly interacts with the Cloudflare scrape endpoint.
33-56: Validation logic is consistent with other actions.The validation ensures required parameters are provided and properly formatted, maintaining consistency with the PDF action.
57-80: API request and response handling look good.The request properly formats the elements array using utils.parseArray and correctly structures the viewport parameters. The summary export provides clear feedback to the user.
components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs (4)
5-19: Code structure is clear and modular.Merging the
commonobject inside the default export effectively centralizes configuration, while thegetHtmlContentmethod cleanly abstracts your POST request. This pattern is consistent and maintainable.
35-37: Good input validation for HTML or URL requirement.Throwing a
ConfigurationErrorhelps to notify users that one of these inputs must be specified. This early validation prevents unnecessary API calls.
39-41: Viewport dimensions must be fully defined.Requiring both
viewportHeightandviewportWidthensures consistent rendering. This approach is sensible to avoid partial configuration issues.
1-3:❓ Verification inconclusive
Validate external documentation and error handling imports.
The imports and configuration error mechanism look correct. However, ensure that the linked Cloudflare documentation accurately points to the intended resource and that any potential HTTP failure scenarios are properly handled or surfaced if needed.
Would you like to confirm that the documented endpoint path is correct and fully supported by Cloudflare’s latest browser rendering APIs?
🏁 Script executed:
#!/bin/bash rg -A 3 "cloudflare_browser_rendering"Length of output: 4463
Action: Confirm Cloudflare Endpoint & Error Handling
The import statements and error-handling configuration (using
ConfigurationErrorfrom@pipedream/platform) appear correct. Theget-html-contentaction is documented with the endpoint URL:
https://developers.cloudflare.com/api/resources/browser_rendering/subresources/content/
which aligns with similar actions in the codebase.Please verify that this endpoint is indeed current and fully supported by Cloudflare’s latest browser rendering APIs and double-check that any HTTP failure scenarios are handled or surfaced appropriately.
components/cloudflare_browser_rendering/common/constants.mjs (1)
1-11: Centralizing constants for browser rendering is a good practice.The placeholders and paths look correct for current Cloudflare APIs. If Cloudflare modifies its API in the future, updating them here ensures changes remain localized.
components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (2)
14-98: Comprehensive screenshot configuration.Providing defaults (e.g.,
screenshotOptionsClip) is beneficial, but ensure users can fully override them if they require different clipping. Also, consider whether large or unexpected values might cause timeouts or memory usage spikes in certain scenarios.
133-139: Consistent validation for HTML or URL and viewport.The repeated
Either **HTML** or **URL**check parallels other actions, promoting consistency and clarity. Maintaining uniform input constraints across all actions helps reduce confusion.components/cloudflare_browser_rendering/common/utils.mjs (2)
26-47: Robust array parsing with clear error messagesThe
parseArrayfunction provides thorough validation with appropriate error handling. The use of a specificConfigurationErrorwith a user-friendly message is excellent for improving the developer experience.
49-52: Clean export with value-added wrapperThe export approach enhances the
parseArrayfunction by automatically applyingparseJsonto each array element, which is a nice touch that will save developers from having to write this logic themselves.components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (2)
70-90: Well-organized authentication and request preparation methodsThe account and request preparation methods follow good practices by centralizing common logic. The methods are focused and handle their specific responsibilities well, making the code more maintainable.
91-106: Robust request handling implementationThe
_makeRequestandpostmethods provide a clean abstraction for making API requests. The design follows good patterns:
- Centralized request configuration
- Reuse of helper methods
- Flexibility through parameter customization
- Default context handling with
$ = this
jcortes
force-pushed
the
cloudflare-browser-rendering-new-components
branch
from
694bd6d to
33f47d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
components/cloudflare_browser_rendering/actions/scrape/scrape.mjs (2)
14-23: Consider using an array of objects forelementsinstead of an array of JSON strings.
Currently, this property is defined as astring[]with each item containing JSON. Converting it to an array of objects would improve clarity and reduce the need for manual JSON parsing.Below is an illustrative diff showing how you might change the property definition and its usage:
- elements: { - type: "string[]", - label: "Elements", - description: "List of elements to scrape where each element is an object with selector. Eg. `{ selector: \"selector\" }`.", - default: [ JSON.stringify({ selector: "body" }) ], - }, + elements: { + type: "object[]", + label: "Elements", + description: "List of elements to scrape, each containing a selector, e.g. `{ selector: \"body\" }`.", + default: [ + { selector: "body" }, + ], + },And in your request construction:
- elements: utils.parseArray(elements), + elements,
57-75: Optional: Add a try/catch block to handle potential request failures.
Currently, any error thrown bythis.app.postwill bubble up unhandled. Wrapping the call in a try/catch would allow you to customize error messages or handle retries.components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs (1)
43-65: Optional: Consider adding explicit error handling for request failures.
While the platform may handleaxioserrors, adding a try/catch aroundgetHtmlContentcalls can provide more control over error messages or fallback logic.components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (2)
22-34: Defaulting clip values as strings may lead to confusion.
Properties such asheightandwidthare typically numeric. Consider storing them as numbers for clarity and consistency:- default: { - height: "800", - width: "600", - x: "0", - y: "0", - scale: "1", - }, + default: { + height: 800, + width: 600, + x: 0, + y: 0, + scale: 1, + },
187-192: Optional: Add error handling when writing the screenshot file.
Currently, the code writes the image to/tmpwithout any catch for I/O errors. Wrap this operation in a try/catch if you want more robust handling.components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (1)
91-99: Suggestion: Consider more robust error handling in_makeRequest.
If the request fails, capturing and rethrowing a custom error message might help users debug issues faster.
📜 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 (9)
components/cloudflare_browser_rendering/actions/common/base.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs(1 hunks)components/cloudflare_browser_rendering/actions/scrape/scrape.mjs(1 hunks)components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs(1 hunks)components/cloudflare_browser_rendering/common/constants.mjs(1 hunks)components/cloudflare_browser_rendering/common/utils.mjs(1 hunks)components/cloudflare_browser_rendering/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs
- components/cloudflare_browser_rendering/package.json
- components/cloudflare_browser_rendering/actions/common/base.mjs
- components/cloudflare_browser_rendering/common/constants.mjs
- components/cloudflare_browser_rendering/common/utils.mjs
⏰ 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/cloudflare_browser_rendering/actions/scrape/scrape.mjs (1)
49-51: Good validation for requiring either HTML or URL.
It's a best practice to ensure one of these fields is provided before executing the logic.components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs (1)
35-41: Great job implementing validation for HTML/URL and viewport dimensions.
Requiring eitherhtmlorurl, and ensuring bothviewportHeightandviewportWidthare set together, helps reduce invalid configurations.components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (2)
133-135: Excellent validation on requiring either HTML or URL.
This ensures that the action always has a valid input source.
137-139: Appropriate viewport dimension checks.
Forcing users to specify both height and width helps avoid partial or inconsistent viewport configurations.components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (1)
70-90: Configuration and parameter handling look solid.
Methods likegetAccountId,getUrl,getHeaders, andgetParamsare clearly structured to set up requests with minimal duplication.
jcortes
force-pushed
the
cloudflare-browser-rendering-new-components
branch
from
33f47d6 to
007bac0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (6)
11-11: Fix documentation link in descriptionThe current documentation link points to the PDF creation endpoint, but this action is for taking screenshots. Please update the link to the correct screenshot documentation URL.
- description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/pdf/methods/create/)", + description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/screenshot/methods/create/)",
27-33: Avoid redundant JSON parsing of the clip parameterThe
screenshotOptionsClipis being passed throughutils.parseJsonon line 156, but it's already defined as an object in the default value (lines 27-33). This might cause issues if the parsing function isn't designed to handle objects as input.- clip: utils.parseJson(screenshotOptionsClip), + clip: screenshotOptionsClip,Also applies to: 156-156
100-103: Improve error handling in the getScreenshot methodThe current error handling catches errors but creates a new Error with just the string representation of the response data. This might lose valuable error details and context.
} catch (error) { - throw new Error(error.response.data.toString()); + const errorMessage = error.response?.data + ? error.response.data.toString() + : error.message; + throw new Error(`Screenshot request failed: ${errorMessage}`); }
184-186: Add file system error handling for screenshot savingThe current implementation writes the screenshot to a file without any error handling. Adding a try/catch block would make the code more robust.
- const imagePath = path.join("/tmp", `screenshot.${screenshotOptionsType || "png"}`); - fs.writeFileSync(imagePath, response); + const imagePath = path.join("/tmp", `screenshot.${screenshotOptionsType || "png"}`); + try { + fs.writeFileSync(imagePath, response); + } catch (error) { + throw new Error(`Failed to save screenshot: ${error.message}`); + }
139-182: Consider refactoring the parameter construction for better readabilityThe current parameter construction in the
getScreenshotcall is quite complex and nested. Consider refactoring this section to improve readability, perhaps by creating helper methods to construct specific parts of the payload.You could create methods to build the screenshot options and viewport parameters:
methods: { // ... existing methods buildScreenshotOptions() { const { screenshotOptionsCaptureBeyondViewport, screenshotOptionsClip, screenshotOptionsEncoding, screenshotOptionsFromSurface, screenshotOptionsFullPage, screenshotOptionsOmitBackground, screenshotOptionsOptimizeForSpeed, screenshotOptionsQuality, screenshotOptionsType, } = this; if (!screenshotOptionsCaptureBeyondViewport && !screenshotOptionsClip && !screenshotOptionsEncoding && !screenshotOptionsFromSurface && !screenshotOptionsFullPage && !screenshotOptionsOmitBackground && !screenshotOptionsOptimizeForSpeed && !screenshotOptionsQuality && !screenshotOptionsType) { return {}; } return { screenshotOptions: { captureBeyondViewport: screenshotOptionsCaptureBeyondViewport, clip: screenshotOptionsClip, encoding: screenshotOptionsEncoding, fromSurface: screenshotOptionsFromSurface, fullPage: screenshotOptionsFullPage, omitBackground: screenshotOptionsOmitBackground, optimizeForSpeed: screenshotOptionsOptimizeForSpeed, quality: screenshotOptionsQuality, type: screenshotOptionsType, }, }; }, buildViewportSettings() { const { viewportHeight, viewportWidth, viewportDeviceScaleFactor, viewportHasTouch, viewportIsLandscape, viewportIsMobile, } = this; if (!viewportHeight || !viewportWidth) { return {}; } return { viewport: { height: viewportHeight, width: viewportWidth, deviceScaleFactor: viewportDeviceScaleFactor, hasTouch: viewportHasTouch, isLandscape: viewportIsLandscape, isMobile: viewportIsMobile, }, }; }, }, // Then in your run method: const response = await getScreenshot({ $, data: { html, url, ...this.buildScreenshotOptions(), selector, ...this.buildViewportSettings(), userAgent, ...utils.parseJson(additionalSettings), }, });
80-85: Consider validating the screenshot type before file extension selectionWhen setting the image file extension on line 184, the code uses
screenshotOptionsType || "png"but doesn't validate thatscreenshotOptionsTypeis one of the supported types. Adding validation would make the code more robust.You could add a validation method and use it both for prop validation and when determining the file extension:
methods: { // ... existing methods getValidScreenshotType(type) { const validTypes = ["png", "jpeg", "webp"]; return validTypes.includes(type) ? type : "png"; }, }, // Then in your run method: const fileType = this.getValidScreenshotType(screenshotOptionsType); const imagePath = path.join("/tmp", `screenshot.${fileType}`);components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (5)
74-77: Add improved error handling for URL constructionThe URL construction in
getUrldoesn't handle potential issues like missing path segments or undefinedconstants. Consider adding validation and error handling.getUrl(path) { + if (!this.getAccountId()) { + throw new Error("Account ID is required for URL construction"); + } + if (!path) { + throw new Error("Path parameter is required for URL construction"); + } const baseUrl = `${constants.BASE_URL}${constants.VERSION_PATH}${constants.BROWSER_RENDERING_PATH}` .replace(constants.ACCOUNT_PLACEHOLDER, this.getAccountId()); return `${baseUrl}${path}`; },
91-100: Enhance _makeRequest with more comprehensive error handlingThe
_makeRequestmethod doesn't include error handling, which might lead to unhelpful error messages when API calls fail. Consider adding a try/catch block with more detailed error reporting._makeRequest({ $ = this, path, params, headers, ...args } = {}) { + try { return axios($, { ...args, url: this.getUrl(path), headers: this.getHeaders(headers), params: this.getParams(params), }); + } catch (error) { + const statusCode = error.response?.status; + const errorData = error.response?.data; + const message = `Request failed with status ${statusCode}: ${JSON.stringify(errorData) || error.message}`; + throw new Error(message); + } },
70-72: Add validation for getAccountId methodThe
getAccountIdmethod doesn't validate that the account ID is present in the authentication data. Consider adding validation to provide clearer error messages when authentication is misconfigured.getAccountId() { - return this.$auth.account_id; + const accountId = this.$auth.account_id; + if (!accountId) { + throw new Error("Account ID not found in authentication data. Please check your Cloudflare Browser Rendering app configuration."); + } + return accountId; },
79-84: Enhance getHeaders method with token validationThe
getHeadersmethod doesn't validate that the API token is present in the authentication data. Consider adding validation to provide clearer error messages when authentication is misconfigured.getHeaders(headers) { + const apiToken = this.$auth.api_token; + if (!apiToken) { + throw new Error("API token not found in authentication data. Please check your Cloudflare Browser Rendering app configuration."); + } return { "Content-Type": "application/json", - "Authorization": `Bearer ${this.$auth.api_token}`, + "Authorization": `Bearer ${apiToken}`, ...headers, }; },
101-106: Add type safety to the post method parametersThe
postmethod accepts an args parameter with a default empty object, but doesn't validate that the path is provided, which is required for making a request. Consider adding validation.post(args = {}) { + if (!args.path) { + throw new Error("Path is required for POST requests"); + } return this._makeRequest({ method: "POST", ...args, }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
components/cloudflare_browser_rendering/actions/common/base.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs(1 hunks)components/cloudflare_browser_rendering/actions/scrape/scrape.mjs(1 hunks)components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs(1 hunks)components/cloudflare_browser_rendering/common/constants.mjs(1 hunks)components/cloudflare_browser_rendering/common/utils.mjs(1 hunks)components/cloudflare_browser_rendering/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs
- components/cloudflare_browser_rendering/actions/scrape/scrape.mjs
- components/cloudflare_browser_rendering/actions/common/base.mjs
- components/cloudflare_browser_rendering/package.json
- components/cloudflare_browser_rendering/common/constants.mjs
- components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs
- components/cloudflare_browser_rendering/common/utils.mjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
jcortes
force-pushed
the
cloudflare-browser-rendering-new-components
branch
from
007bac0 to
a2319ec
Compare
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
components/cloudflare_browser_rendering/actions/scrape/scrape.mjs (3)
9-9: Update the description link for clarity and readability.The documentation link in the description is included in the label text rather than being made clickable separately. Consider using markdown formatting to make it clearer.
- description: "Get meta attributes like height, width, text and others of selected elements. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/scrape/methods/create/)", + description: "Get meta attributes like height, width, text and others of selected elements. See the [documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/scrape/methods/create/).",
14-23: Clarify the elements prop description to match actual implementation.The description states each element is "an object with selector" but the implementation expects JSON strings. This could confuse users trying to pass actual objects.
elements: { type: "string[]", label: "Elements", - description: "List of elements to scrape where each element is an object with selector. Eg. `{ selector: \"selector\" }`.", + description: "List of JSON-formatted strings representing elements to scrape, each containing a selector. Eg. `{\"selector\": \"body\"}`.", default: [ JSON.stringify({ selector: "body", }), ], },
78-79: Consider enhancing the success message with more detail.The current summary message is generic. Adding more details about what was scraped could be helpful for users.
- $.export("$summary", "Successfully scraped elements"); + $.export("$summary", `Successfully scraped ${utils.parseArray(elements).length} element(s)`);components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (2)
27-33: Use numeric values instead of strings for numeric properties.The default values for the clip object are strings, but they represent numeric values. Using numbers would be more appropriate.
default: { - height: "800", - width: "600", - x: "0", - y: "0", - scale: "1", + height: 800, + width: 600, + x: 0, + y: 0, + scale: 1, },
184-185: Potential file overwrite issue with fixed file path.Using a fixed path for the screenshot file could lead to overwriting if multiple screenshots are taken in parallel.
- const imagePath = path.join("/tmp", `screenshot.${screenshotOptionsType || "png"}`); + const timestamp = Date.now(); + const imagePath = path.join("/tmp", `screenshot_${timestamp}.${screenshotOptionsType || "png"}`); fs.writeFileSync(imagePath, response);components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (1)
1-108: Consider adding HTTP GET method for future use cases.The current implementation only has a POST method, but you might need GET for other CloudFlare API endpoints in the future.
post(args = {}) { return this._makeRequest({ method: "POST", ...args, }); }, + get(args = {}) { + return this._makeRequest({ + method: "GET", + ...args, + }); + },
📜 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 (9)
components/cloudflare_browser_rendering/actions/common/base.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs(1 hunks)components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs(1 hunks)components/cloudflare_browser_rendering/actions/scrape/scrape.mjs(1 hunks)components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs(1 hunks)components/cloudflare_browser_rendering/common/constants.mjs(1 hunks)components/cloudflare_browser_rendering/common/utils.mjs(1 hunks)components/cloudflare_browser_rendering/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- components/cloudflare_browser_rendering/package.json
- components/cloudflare_browser_rendering/actions/common/base.mjs
- components/cloudflare_browser_rendering/actions/get-pdf/get-pdf.mjs
- components/cloudflare_browser_rendering/actions/get-html-content/get-html-content.mjs
- components/cloudflare_browser_rendering/common/constants.mjs
- components/cloudflare_browser_rendering/common/utils.mjs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (9)
components/cloudflare_browser_rendering/actions/scrape/scrape.mjs (3)
49-51: LGTM: Input validation is properly implemented.Good check to ensure either HTML or URL is provided, with a clear error message.
53-55: LGTM: Viewport dimension validation is well-implemented.Good validation to ensure both height and width are provided when either one is specified.
57-76: LGTM: The request payload construction is thorough and well-structured.The implementation properly handles all required and optional parameters, with good conditional logic for viewport settings and additional parameters.
components/cloudflare_browser_rendering/actions/get-screenshot/get-screenshot.mjs (2)
94-104: LGTM: Good error handling in getScreenshot method.The try-catch block properly handles binary response errors by converting them to string format for better error reporting.
144-166: Good conditional screenshot options construction.The complex conditional logic ensures that screenshot options are only included when at least one option is provided, which is efficient.
components/cloudflare_browser_rendering/cloudflare_browser_rendering.app.mjs (4)
7-68: LGTM: Comprehensive property definitions for browser rendering options.The propDefinitions section provides a well-structured set of properties with clear labels, descriptions, and appropriate types for configuring browser rendering actions.
70-77: LGTM: Well-structured account and URL handling methods.The methods for retrieving the account ID and constructing URLs are clear and maintainable, making good use of constants from an external file.
78-90: LGTM: Clean implementation of headers and params methods.The methods for preparing headers and parameters are well-implemented, ensuring proper authorization and account ID inclusion.
91-106: LGTM: Centralized request handling with proper delegation.The _makeRequest and post methods provide a clean API for making HTTP requests while handling common concerns consistently.
| ...common, | ||
| key: "cloudflare_browser_rendering-get-screenshot", | ||
| name: "Get Screenshot", | ||
| description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/pdf/methods/create/)", |
There was a problem hiding this comment.
Incorrect documentation link in description.
The documentation link points to the PDF endpoint rather than the screenshot endpoint, which is misleading.
- description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/pdf/methods/create/)",
+ description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/screenshot/methods/create/)",📝 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.
| description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/pdf/methods/create/)", | |
| description: "Takes a screenshot of a webpage from provided URL or HTML. [See the documentation](https://developers.cloudflare.com/api/resources/browser_rendering/subresources/screenshot/methods/create/)", |
WHY
Resolves #15795
Summary by CodeRabbit
New Features
Chores