-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Browserless - update base_url #17880
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe updates refactor the Browserless integration to dynamically use the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConvertHtmlToPdfAction
participant BrowserlessApp
participant FileSystem
User->>ConvertHtmlToPdfAction: Provide HTML and options (downloadPath, etc.)
ConvertHtmlToPdfAction->>BrowserlessApp: convertHtmlToPdf(opts)
BrowserlessApp->>BrowserlessApp: POST to {base_url}/pdf
BrowserlessApp-->>ConvertHtmlToPdfAction: PDF binary (arraybuffer)
alt downloadPath specified
ConvertHtmlToPdfAction->>FileSystem: Save PDF to /tmp or syncDir
FileSystem-->>ConvertHtmlToPdfAction: File path
end
ConvertHtmlToPdfAction-->>User: Return base64 PDF (and file path if saved)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
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 (
|
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: 0
🧹 Nitpick comments (2)
components/browserless/actions/convert-html-to-pdf/convert-html-to-pdf.mjs (2)
29-37
: Minor suggestion: Update parameter name for clarity.The method implementation is correct and follows the established pattern from other actions. However, consider renaming the parameter from
screenshot
todata
orpdfData
for better clarity since this method handles PDF data.- async downloadToTMP(screenshot) { + async downloadToTMP(data) { const path = this.downloadPath.includes("/tmp") ? this.downloadPath : `/tmp/${this.downloadPath}`; - fs.writeFileSync(path, screenshot); + fs.writeFileSync(path, data); return path; },
41-61
: LGTM: Successfully implements PR objectives with proper functionality.The refactoring from direct axios calls to using
this.browserless.convertHtmlToPdf
successfully addresses the PR objective of using dynamic base_url handling instead of hardcoded endpoints. The implementation correctly:
- Leverages the app method for proper base_url handling
- Converts binary data to base64 for API response
- Conditionally saves files when downloadPath is provided
- Exports appropriate success summary
Consider adding explicit error handling around the app method call and file operations for better robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
components/browserless/actions/convert-html-to-pdf/convert-html-to-pdf.mjs
(1 hunks)components/browserless/actions/scrape-url-list/scrape-url-list.mjs
(1 hunks)components/browserless/actions/scrape-url/scrape-url.mjs
(1 hunks)components/browserless/actions/take-screenshot/take-screenshot.mjs
(1 hunks)components/browserless/browserless.app.mjs
(2 hunks)components/browserless/package.json
(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
components/browserless/package.json (1)
Learnt from: jcortes
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.
components/browserless/actions/convert-html-to-pdf/convert-html-to-pdf.mjs (1)
Learnt from: js07
PR: #17375
File: components/tinypng/actions/compress-image/compress-image.mjs:18-23
Timestamp: 2025-07-01T17:01:46.327Z
Learning: In TinyPNG compress-image action (components/tinypng/actions/compress-image/compress-image.mjs), the syncDir property uses accessMode: "read" because this action only reads input files and returns API responses without writing files to /tmp, unlike other TinyPNG actions that save processed files to disk.
🧬 Code Graph Analysis (1)
components/browserless/actions/convert-html-to-pdf/convert-html-to-pdf.mjs (1)
components/browserless/actions/take-screenshot/take-screenshot.mjs (3)
screenshot
(45-53)path
(37-39)filePath
(61-61)
🔇 Additional comments (10)
components/browserless/actions/scrape-url-list/scrape-url-list.mjs (1)
7-7
: LGTM: Version increment aligns with coordinated release.The version bump from "0.0.2" to "0.0.3" is appropriate for this coordinated release that addresses the base URL issue.
components/browserless/actions/take-screenshot/take-screenshot.mjs (1)
8-8
: LGTM: Appropriate patch version increment.The version bump from "0.5.4" to "0.5.5" correctly follows semantic versioning for this coordinated release.
components/browserless/browserless.app.mjs (2)
9-9
: Excellent fix: Dynamic base URL resolves the legacy endpoint issue.This change directly addresses the PR objective by making the base URL dynamic from the authentication configuration (
this.$auth.base_url
) instead of using a hardcoded URL. This prevents the 403 errors caused by routing to the legacy endpoint.
53-60
: LGTM: Well-implemented convertHtmlToPdf method.The new method follows the established pattern in this class:
- Uses the
_makeRequest
helper consistently- Sets appropriate
responseType: "arraybuffer"
for PDF content- Allows options to be passed through and merged properly
components/browserless/actions/scrape-url/scrape-url.mjs (1)
7-7
: LGTM: Consistent version increment.The version bump from "0.0.2" to "0.0.3" is consistent with the other actions in this coordinated release.
components/browserless/package.json (2)
3-3
: LGTM: Package version increment aligns with changes.The version bump to 0.1.4 appropriately reflects the new convertHtmlToPdf method and base URL fix.
16-16
: Verify compatibility with major @pipedream/platform update in browserless componentWe bumped
@pipedream/platform
from^1.5.1
to^3.1.0
in components/browserless/package.json. Major releases may include breaking changes to core APIs (defineComponent signature, built-in utilities likeaxios
,$send
,$checkpoint
,$render
, orthis.$auth
). Please:
- Search for every import/use of
@pipedreamhq/platform
incomponents/browserless
(bothrequire
and ESimport
).- Verify
defineComponent
usage still matches the new signature.- Ensure any calls to platform utilities (
axios
,$send
,$checkpoint
,$render
,this.$auth
) conform to v3 behavior.- Run the browserless component’s tests and confirm there are no failures.
Example commands:
rg "@pipedreamhq/platform" -n components/browserless rg "defineComponent" -n components/browserless npm run test -- components/browserlesscomponents/browserless/actions/convert-html-to-pdf/convert-html-to-pdf.mjs (3)
1-2
: LGTM: Import changes support the refactoring objective.The switch from axios to the browserless app import and addition of fs for file operations aligns with the PR's goal of using dynamic base_url handling instead of hardcoded endpoints.
8-8
: LGTM: Appropriate version increment.The minor version bump from 0.4.1 to 0.4.2 is appropriate for the refactoring and new download functionality added.
11-11
: LGTM: Props correctly configured for new functionality.The browserless app prop enables proper base_url handling, and the new download-related props are well-configured:
downloadPath
is appropriately optionalsyncDir
correctly usesaccessMode: "write"
for file operations- Descriptions are clear and accurate
Also applies to: 17-28
/approve |
Resolves #17856
Summary by CodeRabbit
New Features
Improvements
Chores