feat(sdk): add x-pd-sdk-version, triggerDeploy (1.0.9) #14836
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
Warning Rate limit exceeded@tjk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce a new Changes
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: 5
🧹 Outside diff range and nitpick comments (4)
packages/sdk/examples/browser/Makefile (1)
1-8: Add documentation and phony targetsAdd documentation and declare phony targets to improve maintainability.
+# Configuration +PORT ?= 8000 +DELAY ?= 1 + +# Declare phony targets +.PHONY: default server delayed-open stop-server + +# Start server and open browser default: make server & make delayed-open +# Start HTTP server in examples/browser directory server: cd examples/browser && python -m http.server $(PORT) +# Open browser after a delay delayed-open: sleep $(DELAY) && $(OPENER) http://localhost:$(PORT)/examples/browser/index.html || echo "Failed to open browser"packages/sdk/src/shared/index.ts (2)
Line range hint
571-571: Address TODO comment about client-side auth handling.The TODO comment indicates that client-side auth handling needs to be implemented. This should be addressed to ensure proper authentication flow.
Would you like me to help create a GitHub issue to track the implementation of client-side auth handling?
Line range hint
590-590: Consider using more specific types instead ofany.The use of
eslint-disable-line @typescript-eslint/no-explicit-anysuggests that we could improve type safety by using more specific types.Consider defining an interface for the configuration properties:
interface ConfiguredProps { // Add specific property types based on your requirements [key: string]: string | number | boolean | object; }Also applies to: 591-591, 592-592
packages/sdk/package.json (1)
38-40: Consider improving build script robustness.The build script changes look good, but consider these improvements:
- Add cross-platform compatibility for
rm -rf- Ensure clean exit on prebuild failure
{ "scripts": { "prepublish": "pnpm run build", "prebuild": "node scripts/updateVersion.mjs", - "build": "rm -rf dist && pnpm run prebuild && pnpm run build:node && pnpm run build:browser", + "clean": "rimraf dist", + "build": "pnpm run clean && pnpm run prebuild && pnpm run build:node && pnpm run build:browser", ... }, "devDependencies": { + "rimraf": "^5.0.0", ... } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/sdk/examples/browser/Makefile(1 hunks)packages/sdk/examples/browser/index.html(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/scripts/updateVersion.mjs(1 hunks)packages/sdk/src/browser/async.ts(1 hunks)packages/sdk/src/browser/index.ts(1 hunks)packages/sdk/src/shared/index.ts(3 hunks)packages/sdk/src/version.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/sdk/src/version.ts
- packages/sdk/examples/browser/index.html
- packages/sdk/src/browser/index.ts
- packages/sdk/src/browser/async.ts
🔇 Additional comments (5)
packages/sdk/examples/browser/Makefile (1)
1-8: Verify port availability before starting server
Add a check to verify if the port is already in use.
packages/sdk/src/shared/index.ts (3)
2-8: LGTM! Import statements updated for ES modules compatibility.
The addition of .js extensions to imports and the new version import are appropriate changes that align with ES modules requirements.
356-356: LGTM! SDK version header correctly implemented.
The addition of the "X-PD-SDK-Version" header in the makeRequest method ensures proper version tracking for all API requests, which aligns with the PR objectives.
294-294: Verify version initialization and updates.
The version property is correctly added to track SDK versions. However, let's verify the version update mechanism.
✅ Verification successful
Let me gather more information about the version initialization by checking the imports and usage.
Version initialization and usage is properly implemented
The version property is correctly initialized and used in the SDK:
- Version is imported from
version.tsassdkVersionand assigned to theversionproperty - The version is properly utilized in API requests via the
X-PD-SDK-Versionheader - Version updates are managed through the source file at
src/version.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version initialization and update mechanism
# Check if version.ts exists and contains the version constant
fd -e ts -e js "version" -x cat {} \; | grep -A 1 "export.*version"
# Check for any version update scripts in package.json
fd "package.json" -x jq -r '.scripts | select(. != null) | to_entries[] | select(.key | contains("version"))' {}
Length of output: 262
Script:
#!/bin/bash
# Check the sdkVersion import and its source
rg "sdkVersion" -A 2 -B 2
# Check version-related constants and their usage
ast-grep --pattern 'const $_ = "$_"' | grep -i "version"
# Check BaseClient class implementation
ast-grep --pattern 'class BaseClient {
$$$
version = $_
$$$
}'
Length of output: 13208
packages/sdk/package.json (1)
38-40: Verify the version header implementation.
Since this PR's objective is to add an x-pd-sdk-version header, let's verify that the version from package.json is correctly propagated to the SDK's BaseClient.
✅ Verification successful
The version header implementation is correctly set up
The verification shows that:
- The SDK version is correctly used in the
X-PD-SDK-Versionheader inpackages/sdk/src/shared/index.ts - The version is imported from
version.tswhich is updated at build time byupdateVersion.mjsscript - The
prebuildscript inpackage.jsonensures the version is updated before the build
The implementation follows a proper pattern where:
- The version from package.json is propagated to
version.tsduring build - The version is imported and used in the BaseClient class
- The version is included in all API requests via the
X-PD-SDK-Versionheader
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the version is correctly used in BaseClient for the header
# Check if BaseClient uses the version for the header
ast-grep --pattern 'class BaseClient {
$$$
headers($$$) {
$$$
"x-pd-sdk-version"
$$$
}
$$$
}'
# Check if version is imported in BaseClient
rg -l "import.*version.*from.*version" src/
Length of output: 206
Script:
#!/bin/bash
# Let's first find the correct path to source files
fd -t f "BaseClient"
# Then search for version-related imports and headers in those files
fd -t f "BaseClient" --exec rg -l "version"
# Look for any file that might contain the header implementation
rg -l "x-pd-sdk-version"
# Check the content of updateVersion.mjs to understand version propagation
fd -t f "updateVersion.mjs" --exec cat {}
Length of output: 646
Script:
#!/bin/bash
# Let's check the content of version.ts and any files importing it
fd -t f "version.ts" --exec cat {}
# Search for files that import version.ts
rg -l "from.*version"
# Let's also check the actual implementation files in packages/sdk
fd -t f . -p "packages/sdk/src" --exec cat {}
Length of output: 74741
Script:
#!/bin/bash
# Let's check if BaseClient class uses the version for the header
rg -A 2 "X-PD-SDK-Version" packages/sdk/src
# Let's verify how version is imported in BaseClient
rg -A 2 "import.*version" packages/sdk/src/shared/base.ts
# Let's check if version is correctly propagated from package.json to version.ts
cat packages/sdk/src/version.ts
Length of output: 462
| server: | ||
| cd ../.. && python -m http.server |
There was a problem hiding this comment.
Enhance server security and configuration
The current server implementation has several issues:
- Serves the entire parent directory, potentially exposing sensitive files
- No port configuration
- No graceful shutdown mechanism
+PORT ?= 8000
+
server:
- cd ../.. && python -m http.server
+ cd examples/browser && python -m http.server $(PORT)Also consider adding a target for stopping the server:
.PHONY: stop-server
stop-server:
lsof -ti:$(PORT) | xargs kill -9 2>/dev/null || true| default: | ||
| make -j2 server delayed-open |
There was a problem hiding this comment.
Remove parallel execution to prevent race conditions
The -j2 flag runs targets in parallel which could cause the browser to open before the server is ready. The server needs to be fully started before opening the browser.
default:
- make -j2 server delayed-open
+ make server & make delayed-openCommittable suggestion skipped: line range outside the PR's diff.
| delayed-open: | ||
| sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html |
There was a problem hiding this comment.
Improve cross-platform compatibility and error handling
The current implementation has platform-specific issues:
xdg-openis Linux-specific- Hard-coded port number
- Fixed delay might not be sufficient on slower systems
+PORT ?= 8000
+DELAY ?= 1
+
+ifeq ($(OS),Windows_NT)
+ OPENER := start
+else
+ UNAME := $(shell uname -s)
+ ifeq ($(UNAME),Darwin)
+ OPENER := open
+ else
+ OPENER := xdg-open
+ endif
+endif
+
delayed-open:
- sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html
+ sleep $(DELAY) && $(OPENER) http://localhost:$(PORT)/examples/browser/index.html || echo "Failed to open browser"📝 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.
| delayed-open: | |
| sleep 1 && xdg-open http://localhost:8000/examples/browser/index.html | |
| PORT ?= 8000 | |
| DELAY ?= 1 | |
| ifeq ($(OS),Windows_NT) | |
| OPENER := start | |
| else | |
| UNAME := $(shell uname -s) | |
| ifeq ($(UNAME),Darwin) | |
| OPENER := open | |
| else | |
| OPENER := xdg-open | |
| endif | |
| endif | |
| delayed-open: | |
| sleep $(DELAY) && $(OPENER) http://localhost:$(PORT)/examples/browser/index.html || echo "Failed to open browser" |
| if (!process.env.CI) { | ||
| // make sure people building locally automatically do not track changes to version file | ||
| cp.execSync("git update-index --skip-worktree src/version.ts"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for git command.
The git command execution should be wrapped in a try-catch block to handle potential errors gracefully, especially if git is not installed or the repository is not initialized.
if (!process.env.CI) {
// make sure people building locally automatically do not track changes to version file
- cp.execSync("git update-index --skip-worktree src/version.ts");
+ try {
+ cp.execSync("git update-index --skip-worktree src/version.ts");
+ } catch (error) {
+ console.warn("Warning: Could not update git index. This may be due to git not being installed or repository not being initialized.");
+ }
}📝 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.
| if (!process.env.CI) { | |
| // make sure people building locally automatically do not track changes to version file | |
| cp.execSync("git update-index --skip-worktree src/version.ts"); | |
| } | |
| if (!process.env.CI) { | |
| // make sure people building locally automatically do not track changes to version file | |
| try { | |
| cp.execSync("git update-index --skip-worktree src/version.ts"); | |
| } catch (error) { | |
| console.warn("Warning: Could not update git index. This may be due to git not being installed or repository not being initialized."); | |
| } | |
| } |
| const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8"))) | ||
| const versionTsPath = "./src/version.ts"; | ||
| const data = String(fs.readFileSync(versionTsPath, "utf8")); | ||
| const newData = data.replace(/"(.*)"/, `"${pkg.version}"`); | ||
| fs.writeFileSync(versionTsPath, newData); |
There was a problem hiding this comment.
Add robust error handling and version validation.
The version update logic needs improvements in several areas:
- Error handling for file operations
- Version format validation
- Backup mechanism
- More precise version replacement regex
-const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")))
-const versionTsPath = "./src/version.ts";
-const data = String(fs.readFileSync(versionTsPath, "utf8"));
-const newData = data.replace(/"(.*)"/, `"${pkg.version}"`);
-fs.writeFileSync(versionTsPath, newData);
+try {
+ const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")));
+
+ // Validate version format
+ if (!/^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$/.test(pkg.version)) {
+ throw new Error(`Invalid version format: ${pkg.version}`);
+ }
+
+ const versionTsPath = "./src/version.ts";
+ const data = String(fs.readFileSync(versionTsPath, "utf8"));
+
+ // Create backup
+ fs.writeFileSync(`${versionTsPath}.backup`, data);
+
+ // More precise version replacement
+ const newData = data.replace(/export const version = "(.*?)";/, `export const version = "${pkg.version}";`);
+
+ // Verify replacement occurred
+ if (newData === data) {
+ throw new Error("Version string not found in version.ts");
+ }
+
+ fs.writeFileSync(versionTsPath, newData);
+} catch (error) {
+ console.error("Error updating version:", error.message);
+ process.exit(1);
+}📝 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.
| const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8"))) | |
| const versionTsPath = "./src/version.ts"; | |
| const data = String(fs.readFileSync(versionTsPath, "utf8")); | |
| const newData = data.replace(/"(.*)"/, `"${pkg.version}"`); | |
| fs.writeFileSync(versionTsPath, newData); | |
| try { | |
| const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8"))); | |
| // Validate version format | |
| if (!/^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$/.test(pkg.version)) { | |
| throw new Error(`Invalid version format: ${pkg.version}`); | |
| } | |
| const versionTsPath = "./src/version.ts"; | |
| const data = String(fs.readFileSync(versionTsPath, "utf8")); | |
| // Create backup | |
| fs.writeFileSync(`${versionTsPath}.backup`, data); | |
| // More precise version replacement | |
| const newData = data.replace(/export const version = "(.*?)";/, `export const version = "${pkg.version}";`); | |
| // Verify replacement occurred | |
| if (newData === data) { | |
| throw new Error("Version string not found in version.ts"); | |
| } | |
| fs.writeFileSync(versionTsPath, newData); | |
| } catch (error) { | |
| console.error("Error updating version:", error.message); | |
| process.exit(1); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/sdk/examples/server/index.mjs (2)
3-9: Consider enhancing the example configurationWhile this is an example file, consider the following improvements:
- Add comments explaining this is example/test credentials
- Consider using environment variables with clear defaults
- Document the available environment options
+// Example configuration for development purposes only +// In production, use environment variables for credentials const client = createBackendClient({ - environment: "development", + environment: process.env.PD_ENV || "development", credentials: { - clientId: "not-empty", - clientSecret: "not-empty", + clientId: process.env.PD_CLIENT_ID || "example-client-id", + clientSecret: process.env.PD_CLIENT_SECRET || "example-client-secret", }, });
10-10: Add more comprehensive version information loggingSince this example demonstrates the new SDK version feature, consider enhancing the logging to be more informative.
-console.log("sdk version: " + client.version); +console.log("Pipedream SDK Version:", client.version); +console.log("Note: This version will be sent in the x-pd-sdk-version header for all API requests");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/examples/server/Makefile(1 hunks)packages/sdk/examples/server/index.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sdk/examples/server/Makefile
🔇 Additional comments (1)
packages/sdk/examples/server/index.mjs (1)
1-1: Verify the import path structure
The import path ../../dist/server/server/index.js contains a duplicated "server" directory which seems unusual.
✅ Verification successful
Let me gather more information about the build output structure and the actual import paths.
The import path is correct as per package configuration
Looking at the package.json configuration, the duplicated "server" in the import path ../../dist/server/server/index.js is actually correct. This is confirmed by the package.json's main, module, and exports fields which all specify the same path structure. The path aligns with the package's build output configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the correct import path structure
# Expected: Find the actual location of server/index.js to confirm the path
# Search for the file
fd --type f "index.js" packages/sdk/dist
# Verify the import in the TypeScript source
fd --type f "index.ts" packages/sdk/src
Length of output: 285
Script:
#!/bin/bash
# Check if the dist directory exists and its structure
fd --type d "dist" packages/sdk
# Check package.json for build output configuration
cat packages/sdk/package.json
# Look for other example files that might import from dist
rg "from.*dist.*server" packages/sdk/examples
Length of output: 2208
| import { createBackendClient } from "../../dist/server/server/index.js"; | ||
|
|
||
| const client = createBackendClient({ | ||
| environment: "development", | ||
| credentials: { | ||
| clientId: "not-empty", | ||
| clientSecret: "not-empty", | ||
| }, | ||
| }); | ||
| console.log("sdk version: " + client.version); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and API example
The example could be more comprehensive by demonstrating error handling and an actual API call to show the x-pd-sdk-version header in action.
Here's a suggested enhancement:
import { createBackendClient } from "../../dist/server/server/index.js";
+async function main() {
+ try {
const client = createBackendClient({
environment: "development",
credentials: {
clientId: "not-empty",
clientSecret: "not-empty",
},
});
console.log("sdk version: " + client.version);
+
+ // Example API call to demonstrate x-pd-sdk-version header
+ const response = await client.makeRequest();
+ console.log("API Response:", response);
+ } catch (error) {
+ console.error("Error:", error.message);
+ process.exit(1);
+ }
+}
+
+main();Committable suggestion skipped: line range outside the PR's diff.
| const data = String(fs.readFileSync(versionTsPath, "utf8")); | ||
| const newData = data.replace(/"(.*)"/, `"${pkg.version}"`); | ||
| fs.writeFileSync(versionTsPath, newData); |
There was a problem hiding this comment.
So that we don't need to keep the src/version.ts file around, we can just generate it on the fly perhaps?
| const data = String(fs.readFileSync(versionTsPath, "utf8")); | |
| const newData = data.replace(/"(.*)"/, `"${pkg.version}"`); | |
| fs.writeFileSync(versionTsPath, newData); | |
| const newData = `export const PD_SDK_VERSION = "${pkg.version}";` | |
| fs.writeFileSync(versionTsPath, newData); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/sdk/scripts/updateVersion.mjs (1)
1-11: Consider alternative approaches for version injection.The current approach of generating a source file at build time has several drawbacks:
- Requires git worktree manipulation
- Creates a source file that might be accidentally modified
- Adds complexity to the build process
Consider these alternatives:
- Use build tool plugins to inject version during bundling
- Use environment variables with defaults from package.json
- Use a proper asset pipeline for build-time constants
Would you like me to provide examples of these alternative implementations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/scripts/updateVersion.mjs(1 hunks)packages/sdk/src/version.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/version.ts
🔇 Additional comments (3)
packages/sdk/scripts/updateVersion.mjs (3)
1-2: LGTM! Imports are appropriate for the required functionality.
4-7:
Add error handling for git command execution.
The git command should be wrapped in a try-catch block to handle potential errors gracefully.
if (!process.env.CI) {
// make sure people building locally automatically do not track changes to version file
- cp.execSync("git update-index --skip-worktree src/version.ts");
+ try {
+ cp.execSync("git update-index --skip-worktree src/version.ts");
+ } catch (error) {
+ console.warn("Warning: Could not update git index. This may be due to git not being installed or repository not being initialized.");
+ }
}9-11: 🛠️ Refactor suggestion
Enhance version update implementation with validation and better naming.
- The constant name should be
PD_SDK_VERSIONto align with the header name. - The implementation needs proper error handling and version validation.
-const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")))
-fs.writeFileSync("./src/version.ts", `// DO NOT EDIT, SET AT BUILD TIME
-export const version = "${pkg.version}";`);
+try {
+ const pkg = JSON.parse(String(fs.readFileSync("./package.json", "utf8")));
+
+ // Validate version format
+ if (!/^\d+\.\d+\.\d+(?:-[\w.-]+)?(?:\+[\w.-]+)?$/.test(pkg.version)) {
+ throw new Error(`Invalid version format: ${pkg.version}`);
+ }
+
+ const versionContent = `// DO NOT EDIT, SET AT BUILD TIME
+export const PD_SDK_VERSION = "${pkg.version}";
+`;
+
+ fs.writeFileSync("./src/version.ts", versionContent);
+} catch (error) {
+ console.error("Error updating version:", error.message);
+ process.exit(1);
+}This implementation:
- Uses a more descriptive constant name that matches the header
- Adds proper error handling for file operations
- Validates the version format
- Maintains clean code structure with template literals
Likely invalid or redundant comment.
This reverts commit 5cc123b.
tjk
changed the title
tjk
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
eslint.config.mjs(3 hunks)package.json(1 hunks)packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/scripts/updateVersion.mjs(1 hunks)packages/sdk/src/shared/index.ts(4 hunks)packages/sdk/src/version.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk/src/version.ts
- packages/sdk/scripts/updateVersion.mjs
- packages/sdk/package.json
🔇 Additional comments (6)
packages/sdk/CHANGELOG.md (1)
4-9: LGTM! Changelog entry is clear and well-formatted.
The changelog entry properly documents the addition of the x-pd-sdk-version header and follows the established format.
package.json (1)
49-49: LGTM! Appropriate dev dependency addition.
The eslint-plugin-import addition is correctly placed as a dev dependency with a reasonable version constraint.
eslint.config.mjs (2)
5-5: LGTM! Plugin import and registration is correct.
The eslint-plugin-import is properly imported and registered in the plugins configuration.
Also applies to: 56-56
318-330: LGTM! Well-scoped rule configuration.
The import/extensions rule is appropriately scoped to SDK TypeScript files and enforces consistent import patterns.
packages/sdk/src/shared/index.ts (2)
3-9: LGTM! Import paths updated for ES modules compatibility.
The addition of .js extensions to import paths and the new sdkVersion import are correctly implemented.
295-295: LGTM! Version tracking properly implemented.
The addition of the version property and X-PD-SDK-Version header enhances debugging capabilities and aligns with the PR objectives.
Also applies to: 357-357
packages/sdk/src/shared/index.ts
Outdated
| public async triggerDeploy(opts: { | ||
| userId: string; | ||
| triggerId: string; | ||
| configuredProps: Record<string, any>; // eslint-disable-line @typescript-eslint/no-explicit-any | ||
| dynamicPropsId?: string; | ||
| webhookUrl?: string; | ||
| }) { | ||
| const body = { | ||
| async_handle: this.asyncResponseManager.createAsyncHandle(), | ||
| external_user_id: opts.userId, | ||
| id: opts.triggerId, | ||
| configured_props: opts.configuredProps, | ||
| dynamic_props_id: opts.dynamicPropsId, | ||
| webhook_url: opts.webhookUrl, | ||
| } | ||
| return await this.makeConnectRequestAsync("/triggers/deploy", { | ||
| method: "POST", | ||
| body, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add return type and documentation for triggerDeploy method.
The new method implementation looks functionally correct, but could benefit from the following improvements:
- Add a return type specification for better type safety
- Add JSDoc documentation following the pattern used in other methods
- Consider defining a proper type for configuredProps instead of using Record<string, any>
Here's the suggested implementation:
+ /**
+ * Deploys a trigger for a specified user
+ *
+ * @param opts - The options for deploying the trigger
+ * @param opts.userId - The ID of the user deploying the trigger
+ * @param opts.triggerId - The ID of the trigger to deploy
+ * @param opts.configuredProps - The configured properties for the trigger
+ * @param opts.dynamicPropsId - Optional dynamic properties ID
+ * @param opts.webhookUrl - Optional webhook URL for the trigger
+ * @returns A promise resolving to the deployment response
+ */
public async triggerDeploy(opts: {
userId: string;
triggerId: string;
- configuredProps: Record<string, any>; // eslint-disable-line @typescript-eslint/no-explicit-any
+ configuredProps: Record<string, unknown>;
dynamicPropsId?: string;
webhookUrl?: string;
- }) {
+ }): Promise<{ success: boolean }> {
const body = {
async_handle: this.asyncResponseManager.createAsyncHandle(),
external_user_id: opts.userId,
id: opts.triggerId,
configured_props: opts.configuredProps,
dynamic_props_id: opts.dynamicPropsId,
webhook_url: opts.webhookUrl,
}
return await this.makeConnectRequestAsync("/triggers/deploy", {
method: "POST",
body,
});
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/sdk/src/shared/component.ts (3)
98-110: Consider enhancing type safety for timestamps and callback_observationsThe type definition looks good overall, but could benefit from some improvements:
Consider these type safety enhancements:
- created_at: number; - updated_at: number; + created_at: UnixTimestamp; // Define a type alias for clarity + updated_at: UnixTimestamp; - callback_observations?: unknown; + callback_observations?: Record<string, unknown>; // Or a more specific typeAlso, consider adding JSDoc comments to document the purpose of each field, especially for fields like
callback_observationswhose purpose isn't immediately clear from the name.
Line range hint
1-96: Address existing TODO and XXX commentsThere are several TODO and XXX comments in the existing code that indicate areas needing improvement:
Would you like me to help address any of these items?
- Refactor
BaseConfigurablePropto separate optional properties- Fix the duplicate type mapping in
Defaultable<T>- Verify and document supported alert types
- Confirm if
secretis supported for string arraysI can help create GitHub issues to track these improvements or provide specific implementation suggestions.
97-110: Consider architectural improvements for better type safetyThe new type looks good, but there are opportunities for broader improvements:
- Consider creating a separate types directory to better organize these definitions
- Replace
anytypes with more specific types where possible:// Example replacement for the generic parameter export type V1DeployedComponent<T extends ConfigurableProps = ConfigurableProps>
- Consider using branded types for IDs to prevent mixing different ID types:
type ID = string & { readonly brand: unique symbol }; type ComponentId = ID; type OwnerId = ID;packages/sdk/src/server/async.ts (1)
WebSocket security and error handling improvements needed
Based on the codebase analysis:
- The OAuth token in WebSocket URL is a security concern as it's the only implementation (
wss://${this.serverOpts.apiHost}/websocket?oauth_token=${token}).- No WebSocket error handling or reconnection logic was found in the codebase, making the connection potentially fragile.
- OAuth token validation exists but is minimal, with the same generic error message used across the codebase.
Recommendations:
- Move the OAuth token to WebSocket protocol headers instead of URL parameters
- Implement WebSocket error handling with reconnection logic, similar to the retry patterns found in other parts of the codebase (e.g.,
shortcut.app.js)- Enhance OAuth token validation with more specific error messages
🔗 Analysis chain
Line range hint
32-46: Consider security and error handling improvements.While the OAuth token validation is good, there are some architectural considerations:
- The OAuth token is exposed in the WebSocket URL query parameter, which might appear in logs. Consider using headers or a more secure token transmission method.
- The error message "Invalid OAuth token structure" could be more specific about what's missing.
- Consider adding WebSocket connection error handling and reconnection logic.
Let's check for similar patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check WebSocket security patterns # Look for WebSocket URL construction with tokens rg -t typescript "wss?://.*oauth_token" # Check for other OAuth token validations ast-grep --pattern 'if (!$_?.token?.access_token)'Length of output: 129
Script:
#!/bin/bash # Look for WebSocket URL construction patterns with tokens, using proper file extensions rg -t ts -t js "wss?://.*oauth_token" # Check for WebSocket connection handling patterns rg -t ts -t js "new WebSocket|WebSocket\(" -A 3 # Look for OAuth token validations and error handling rg -t ts -t js "token\.access_token" -A 2 # Find similar error messages related to OAuth rg -t ts -t js "Invalid OAuth"Length of output: 770
Script:
#!/bin/bash # Check for WebSocket connection error handling patterns rg -t ts -t js "WebSocket.*error|onerror" -A 3 # Look for reconnection logic rg -t ts -t js "reconnect|retry" -A 3 # Check the base class or interface implementation fd -e ts -e js AsyncResponseManager --exec cat {}Length of output: 2494
packages/sdk/tsconfig.node.json (1)
Line range hint
13-16: Consider documenting the module resolution strategy.Given that these configuration changes affect module resolution in both browser and node environments, it would be helpful to:
- Document the reasoning behind these changes in relation to the SDK version header feature
- Add a comment in both config files explaining the module resolution strategy
- Consider creating a shared base tsconfig.json for common settings
This will help maintain consistency as the SDK evolves.
Also applies to: 23-25
.github/workflows/pipedream-sdk-test.yaml (1)
Line range hint
1-47: Verify test coverage for Node.js version compatibilityThe workflow only tests against a single Node.js version. Consider adding a matrix strategy to test against multiple supported Node.js versions (18 and 20) to ensure compatibility across versions.
Here's a suggested enhancement:
jobs: test: runs-on: ubuntu-latest + strategy: + matrix: + node-version: ['18', '20'] steps: - name: Checkout code uses: actions/checkout@v4 # ... other steps ... - name: Set up Node.js uses: actions/setup-node@v3 with: - node-version: '22' + node-version: ${{ matrix.node-version }}🧰 Tools
🪛 actionlint (1.7.4)
37-37: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.github/workflows/pipedream-sdk-test.yaml(1 hunks)packages/sdk/src/server/async.ts(1 hunks)packages/sdk/src/shared/component.ts(1 hunks)packages/sdk/src/shared/index.ts(4 hunks)packages/sdk/tsconfig.browser.json(1 hunks)packages/sdk/tsconfig.node.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/shared/index.ts
🔇 Additional comments (4)
packages/sdk/src/server/async.ts (1)
29-29: Verify WebSocket type compatibility and consider alternatives.
The type assertion as unknown as typeof adapters.WebSocket bypasses TypeScript's type checking. While this works, consider these alternatives for better type safety:
- Create a proper type adapter interface
- Use declaration merging to extend the ActionCable types
Let's verify the WebSocket usage across the codebase:
✅ Verification successful
Type assertion is necessary and correctly implemented
The type assertion is a valid implementation based on the following findings:
- The code bridges Node.js
wspackage's WebSocket with Rails ActionCable adapters - This is the only WebSocket adapter configuration in the codebase
- The CHANGELOG indicates this is a deliberate fix for the new components API
- The type assertion is necessary because ActionCable expects a browser-compatible WebSocket, while we're using Node's
wsimplementation
The current implementation is the most pragmatic solution as:
- The code runs in a server environment where
wspackage provides WebSocket functionality - ActionCable adapters require browser-compatible WebSocket interface
- The type assertion safely bridges this compatibility gap
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check WebSocket adapter usage patterns
# Look for other WebSocket type assertions or adapter patterns
rg -t typescript "WebSocket.*as.*unknown"
# Check for other ActionCable adapter configurations
ast-grep --pattern 'adapters.WebSocket = $_'
Length of output: 239
Script:
#!/bin/bash
# Let's check for WebSocket usage patterns more broadly
rg "WebSocket" -A 3 -B 3
# Look for adapter patterns and type definitions
fd -e ts -e d.ts --exec grep -l "WebSocket"
# Check for ActionCable related types and configurations
rg "ActionCable|adapters\." -A 2 -B 2
Length of output: 7039
packages/sdk/tsconfig.browser.json (1)
14-16: Technically sound, but please clarify the necessity.
The addition of paths mapping to resolve .js files to .ts files is technically correct and follows TypeScript best practices. However, could you clarify how this change relates to the PR's primary objectives of adding the x-pd-sdk-version header and triggerDeploy feature?
Let's check if there are any new imports that specifically require this mapping:
✅ Verification successful
The paths mapping is necessary for ES modules compatibility
The codebase shows consistent usage of .js extensions in import statements throughout the browser and shared components (e.g., "./async.js", "../shared/index.js"). This paths mapping in tsconfig.browser.json is necessary to properly resolve these ES module imports while working with TypeScript source files. Without it, the TypeScript compiler would fail to resolve these imports correctly in a browser environment.
Examples of imports requiring this mapping:
packages/sdk/src/shared/index.ts:import { version as sdkVersion } from "../version.js"packages/sdk/src/browser/index.ts:import { BrowserAsyncResponseManager } from "./async.js"packages/sdk/src/browser/async.ts:import { AsyncResponseManager } from "../shared/async.js"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for .js imports in TypeScript files
rg -t ts '\.js["\']' packages/sdk/src/
Length of output: 116
Script:
#!/bin/bash
# Search for .js imports in TypeScript files (fixing the syntax)
rg "\.js['\"]" packages/sdk/src/
# Also check for any import statements that might need this mapping
rg "^import.*from" packages/sdk/src/
# Let's also check the PR description or related files for context
fd -t f "CHANGELOG|README" packages/sdk/
Length of output: 2156
packages/sdk/tsconfig.node.json (1)
23-25: Consistent with browser config, but verify module resolution.
The paths mapping addition maintains consistency with the browser configuration, which is good. However, there's a potential consideration:
- Browser config uses
"moduleResolution": "node" - Node config uses
"moduleResolution": "NodeNext"
These different module resolution strategies might handle the paths mapping differently.
Let's verify if there are any module resolution conflicts:
.github/workflows/pipedream-sdk-test.yaml (1)
39-39:
Consider using an LTS version of Node.js instead of 22
Node.js 22 is very recent and not yet in LTS status, which could lead to stability issues. Consider using Node.js 20 (LTS) instead, as it provides a better balance of features and stability for CI environments.
Let's verify the SDK's dependencies compatibility with Node.js 22:
Suggested fix:
- node-version: '22'
+ node-version: '20'
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
jest.config.js (1)
16-23: LGTM! Consider adding a comment for the transform configThe transform configuration is well-structured and correctly moves the ESM settings from globals to transform. This is the recommended approach for ts-jest.
Consider adding a comment explaining the ESM configuration:
transform: { + // Configure ts-jest for ESM support "\\.[jt]s?$": [ "ts-jest", { "useESM": true, }, ], },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
jest.config.js(1 hunks)
🔇 Additional comments (1)
jest.config.js (1)
11-11: Verify the impact of broader moduleNameMapper pattern
The new pattern ^(.+)\\.js$ is more permissive than the previous one that only matched relative paths. While this supports the ESM changes, we should verify it doesn't unexpectedly affect node_modules resolution.
✅ Verification successful
The moduleNameMapper pattern change is safe to use
The new pattern ^(.+)\\.js$ is used correctly in this context. The codebase scan shows that:
- Most .js imports are from node_modules (like lodash) which Jest doesn't process
- The SDK's own imports use explicit paths (e.g.,
"../../dist/server/server/index.js") - The pattern aligns with the ESM module system being implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for .js imports in test files that might be affected
# Look for import statements in test files to understand the impact
# Search for .js imports in test files
rg -t ts -t js "import.*from.*\.js[\"']" "**/*test*"
# List test files that might be affected
fd -e test.ts -e test.js
Length of output: 206
Script:
#!/bin/bash
# Let's check the actual imports in the test files and their patterns
echo "=== Content of types/src/types.test.ts ==="
cat types/src/types.test.ts
echo -e "\n=== Content of packages/sdk/src/server/__tests__/server.test.ts ==="
cat packages/sdk/src/server/__tests__/server.test.ts
# Also check the imports in the entire codebase to understand the pattern usage
echo -e "\n=== All .js imports in the codebase ==="
rg "import.*from.*\.js['\"]"
Length of output: 42622
Summary by CodeRabbit
Release Notes
New Features
triggerDeployin the SDK for deploying triggers with specified parameters.Improvements
eslint-plugin-importfor better import management.Bug Fixes
These updates enhance the SDK's usability and maintainability, providing a better experience for developers.