feat(sdk-package): call apis directly from client sdk #14699
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe pull request includes significant updates to the 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 (
|
| token: string; | ||
| token?: string; |
There was a problem hiding this comment.
Do we want to remove this and require the token be configured when the client is initialized?
This would be a breaking change.
| } catch (err) { | ||
| opts.onError?.(err as ConnectError); | ||
| } | ||
| this.refreshToken(); // token expires once it's used to create a connected account. We need to get a new token for the next requests. |
There was a problem hiding this comment.
This hack of refreshing tokens isn't the best but it's needed based on the existing implementation. Ideally, we could stop deleting the tokens.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (28)
packages/sdk/tsconfig.browser.json (1)
5-5: LGTM! Balanced approach to TypeScript library inclusion.The addition of es2017 to libs while maintaining ES5 and DOM is a good approach. It provides modern feature typings while ensuring backward compatibility.
Consider documenting the minimum browser version requirements in the SDK's README to help users understand compatibility requirements.
packages/sdk/src/browser/async.ts (1)
4-7: Consider adding URL validation for apiHostWhile the type definition is clean, consider adding runtime validation to ensure
apiHostis a valid hostname before using it to construct WebSocket URLs.Example validation helper:
function validateApiHost(host: string): void { try { new URL(`https://${host}`); } catch { throw new Error('Invalid apiHost format'); } }packages/sdk/src/server/__tests__/server.test.ts (1)
302-310: Consider adding a test case for empty accounts list.While the updated response structure and assertion are correct, it would be beneficial to add a test case that verifies the behavior when the API returns an empty accounts list.
describe("getAccounts", () => { it("should retrieve accounts", async () => { // ... existing test ... }); + it("should handle empty accounts list", async () => { + fetchMock.mockResponse( + JSON.stringify({ + data: [], + }), + { + headers: { + "Content-Type": "application/json", + }, + }, + ); + + const result = await client.getAccounts({ + include_credentials: true, + }); + + expect(result.data).toEqual([]); + expect(fetchMock).toHaveBeenCalledWith( + `https://api.pipedream.com/v1/connect/${projectId}/accounts?include_credentials=true`, + expect.any(Object), + ); + }); });Also applies to: 322-322
packages/sdk/src/server/async.ts (1)
19-21: Add braces to the if statement for clarity and maintainabilityWhile it's acceptable to omit braces for single-line statements, adding braces improves readability and reduces the risk of errors when modifying the code in the future.
Modify the code as follows:
if (typeof adapters.WebSocket === "undefined") - adapters.WebSocket = WS; + { + adapters.WebSocket = WS; + }packages/sdk/src/shared/component.ts (2)
5-5: Address the TODO and XXX commentsThere are several
TODOandXXXcomments indicating areas that need attention:
- Line 5:
// XXX don't actually apply to all, fix- Line 16:
// XXX fix duplicating mapping to value type here and with PropValue- Line 21:
// TODO check the types- Line 46:
// TODO is this supportedThese comments suggest that certain properties may not apply to all configurable props and that there might be duplication in type mappings. Would you like assistance in refining these type definitions to address these concerns? I can help:
- Refactor
BaseConfigurablePropto accurately represent applicable properties.- Resolve duplication between
Defaultable<T>andPropValue.- Verify and update the
alertTypeandsecretproperty types.Also applies to: 16-16, 21-21, 46-46
85-87: Review optional properties inConfiguredProps<T>The
ConfiguredProps<T>type defines properties as optional. Depending on your application's requirements, you might want to enforce certain properties to be required, especially if they are marked as non-optional in their respective type definitions.Consider adjusting the type to reflect the
optionalattribute of eachConfigurableProp. This could enhance type safety and prevent runtime errors due to missing required properties.packages/sdk/src/shared/async.ts (2)
24-29: Use generics to enhance type safety in theHandletype.Currently, the
Handletype usesany, which can be avoided by using generics for better type safety and clarity.Here's how you might update the
Handletype and related code:type Handle<T> = { resolve: (value: T) => void; reject: (reason: string) => void; promise: Promise<T>; };Update the
createHandlefunction:const createHandle = <T>(): Handle<T> => { let resolve!: (value: T) => void; let reject!: (reason: string) => void; const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; }); return { resolve, reject, promise }; };Adjust usage in the class methods accordingly.
31-38: Simplify thecreateHandlefunction to avoid type assertions.By initializing
resolveandrejectbefore creating thePromise, you can eliminate the need forPartial<Handle>and type casting.Refactored
createHandlefunction:const createHandle = <T>(): Handle<T> => { let resolve!: (value: T) => void; let reject!: (reason: string) => void; const promise = new Promise<T>((res, rej) => { resolve = res; reject = rej; }); return { resolve, reject, promise }; };packages/sdk/src/server/cli.ts (9)
4-4: Adjust object destructuring to match style guidelinesStatic analysis indicates that object destructuring should include line breaks after the opening brace and before the closing brace for better readability.
Apply this diff:
-const { CLIENT_ID, CLIENT_SECRET, PROJECT_ID, API_HOST } = process.env; +const { + CLIENT_ID, + CLIENT_SECRET, + PROJECT_ID, + API_HOST, +} = process.env;🧰 Tools
🪛 eslint
[error] 4-4: Expected a line break after this opening brace.
(object-curly-newline)
[error] 4-4: Expected a line break before this closing brace.
(object-curly-newline)
17-17: Add missing trailing commaA trailing comma is expected after the last property in an object literal to comply with style guidelines.
Apply this diff:
apiHost: API_HOST +,🧰 Tools
🪛 eslint
[error] 17-18: Missing trailing comma.
(comma-dangle)
95-95: Format ternary expressions for readabilityStatic analysis suggests adding newlines between the test, consequent, and alternate of ternary expressions.
Apply this diff:
-allowed_origins: options.allowedOrigins ? options.allowedOrigins.split(",") : undefined, +allowed_origins: options.allowedOrigins + ? options.allowedOrigins.split(",") + : undefined,🧰 Tools
🪛 eslint
[error] 95-95: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 95-95: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
109-109: Format ternary expressions and object literals for clarityTo enhance readability, format the ternary expression and add line breaks in the object literal.
Apply this diff:
-const params = options.includeCredentials ? { include_credentials: options.includeCredentials } : {}; +const params = options.includeCredentials + ? { + include_credentials: options.includeCredentials, + } + : {};🧰 Tools
🪛 eslint
[error] 109-109: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 109-109: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 109-109: Expected a line break after this opening brace.
(object-curly-newline)
[error] 109-109: Expected a line break before this closing brace.
(object-curly-newline)
135-135: Add line breaks in object literalsStatic analysis recommends adding line breaks after the opening brace and before the closing brace in object literals.
Apply this diff:
-const apps = await client.apps({ q: options.query }); +const apps = await client.apps({ + q: options.query, +});🧰 Tools
🪛 eslint
[error] 135-135: Expected a line break after this opening brace.
(object-curly-newline)
[error] 135-135: Expected a line break before this closing brace.
(object-curly-newline)
178-178: Adjust object literals to follow style guidelinesFor consistency and readability, include line breaks in object literals as per static analysis hints.
Apply this diff:
-const component = await client.component({ key }); +const component = await client.component({ + key, +});🧰 Tools
🪛 eslint
[error] 178-178: Expected a line break after this opening brace.
(object-curly-newline)
[error] 178-178: Expected a line break before this closing brace.
(object-curly-newline)
109-109: Ensure consistent use of trailing commasAccording to style guidelines, include trailing commas after the last property in object literals.
Apply this diff:
+// For line 109 +include_credentials: options.includeCredentials, +// For line 135 +q: options.query, +// For line 178 +key,Also applies to: 135-135, 178-178
🧰 Tools
🪛 eslint
[error] 109-109: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 109-109: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 109-109: Expected a line break after this opening brace.
(object-curly-newline)
[error] 109-109: Expected a line break before this closing brace.
(object-curly-newline)
195-195: Enhance error handling for JSON parsingWhen parsing JSON strings from command-line options, invalid JSON inputs can cause the application to crash. Consider adding specific error handling to provide clear feedback to the user.
Example modification:
try { - const configuredProps = JSON.parse(options.configuredProps); + let configuredProps; + try { + configuredProps = JSON.parse(options.configuredProps); + } catch (parseError) { + console.error("Invalid JSON provided for configured properties:", parseError.message); + process.exit(1); + }Repeat similar error handling for other instances where
JSON.parseis used.Also applies to: 218-218, 240-240
25-31: Consider logging complete error details for better debuggingIn the
handleErrorfunction, you might want to log the entire error stack to aid in troubleshooting.Apply this diff:
if (error instanceof Error) { - console.error(`${message}:`, error.message); + console.error(`${message}:`, error.stack || error.message); } else {packages/sdk/src/server/index.ts (4)
8-8: Remove unused import 'GetAccountOpts'The import
GetAccountOptsis not used anywhere in the code. Consider removing it to clean up the imports and prevent potential warnings.Apply this change:
-import { Account, BaseClient, GetAccountOpts, type AppInfo, type ConnectTokenResponse } from "../shared"; +import { Account, BaseClient, type AppInfo, type ConnectTokenResponse } from "../shared";🧰 Tools
🪛 eslint
[error] 8-8: Expected a line break after this opening brace.
(object-curly-newline)
[error] 8-8: 'GetAccountOpts' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 8-8: Expected a line break before this closing brace.
(object-curly-newline)
10-10: Use double quotes in export statementThe export statement should use double quotes for string literals to comply with the project's coding style and fix the eslint warning.
Apply this change:
-export * from '../shared' +export * from "../shared"🧰 Tools
🪛 eslint
[error] 10-10: Strings must use doublequote.
(quotes)
[error] 10-11: Missing semicolon.
(@typescript-eslint/semi)
256-257: Add missing trailing commaAdd a trailing comma after the last parameter to comply with code style guidelines and fix the eslint error.
Apply this change:
opts: ConnectTokenCreateOpts - ): Promise<ConnectTokenResponse> { + ,): Promise<ConnectTokenResponse> {🧰 Tools
🪛 eslint
[error] 256-257: Missing trailing comma.
(comma-dangle)
282-283: Add missing trailing commaAdd a trailing comma after the last parameter to comply with code style guidelines and fix the eslint error.
Apply this change:
params: GetAccountByIdOpts = {} - ): Promise<Account> { + ,): Promise<Account> {🧰 Tools
🪛 eslint
[error] 282-283: Missing trailing comma.
(comma-dangle)
packages/sdk/src/shared/index.ts (4)
401-401: Simplify conditional check using optional chaining.The conditional can be simplified by using optional chaining to make the code more concise.
Apply this diff:
-if (contentType && contentType.includes("application/json")) { +if (contentType?.includes("application/json")) {🧰 Tools
🪛 Biome
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
826-826: Address the TODO: Test token authentication for external workflow invocation.The TODO comment indicates that testing is needed to ensure token authentication works when invoking workflows for external users. Addressing this will help confirm that the authentication mechanism functions as expected.
Would you like assistance in writing tests for this functionality or opening a GitHub issue to track this task?
507-507: Resolve 'XXX' comments to improve code quality and clarity.There are 'XXX' comments in the code indicating areas that may need attention:
- Line 507:
// XXX only here while need project auth- Line 531:
// XXX only here while need project auth- Line 543:
// XXX can just use /components and ?type instead when supportedConsider addressing these comments to enhance maintainability and code clarity.
Would you like assistance in resolving these issues or creating GitHub issues to track them?
Also applies to: 531-531, 543-543
758-758: Address the TODO: Handle authentication in client-side calls.The TODO comment suggests uncertainty about handling authentication in client-side calls, particularly when the
authTypeis not OAuth. Clarifying and implementing the correct authentication handling will ensure secure API interactions.Would you like assistance in refining this authentication logic or opening a GitHub issue to track this task?
packages/sdk/src/browser/index.ts (3)
Line range hint
233-248: Ensure proper cleanup after message handlingThe
cleanupfunction is only called when the message type is"close". To prevent potential memory leaks and unintended behavior,cleanupshould also be called after handling"success"and"error"messages to remove the event listener and iframe.Apply this diff to call
cleanupafter handling all terminal message types:const onMessage = (e: MessageEvent) => { switch (e.data?.type) { case "success": opts.onSuccess?.({ id: e.data?.authProvisionId, }); + this.cleanup(onMessage); break; case "error": opts.onError?.(new ConnectError(e.data.error)); + this.cleanup(onMessage); break; case "close": this.cleanup(onMessage); break; default: break; } };
205-206: Clarify token refresh behaviorThe
refreshTokenmethod unconditionally sets_tokentoundefined. InconnectAccount,refreshTokenis called regardless of whether the token was consumed or if an error occurred. This could lead to unnecessary token refreshes.Consider modifying
refreshTokento conditionally refresh the token only when necessary, or moverefreshTokeninside the try block:public async connectAccount(opts: StartConnectOpts) { // ... existing code ... try { await this.createIframe(opts); + this.refreshToken(); // Token expires after successful account connection; get a new token for subsequent requests. } catch (err) { opts.onError?.(err as ConnectError); } - this.refreshToken(); // Move inside the try block }
Line range hint
276-308: Ensure iframe reference is always setThere is a possibility that
this.iframemay not be set if theiframe.onloadevent doesn't fire before cleanup. This could cause issues in thecleanupmethod when attempting to remove the iframe.Assign
this.iframeimmediately after creating the iframe to ensure it's always available:private async createIframe(opts: StartConnectOpts) { // ... existing code ... const iframe = document.createElement("iframe"); iframe.id = `pipedream-connect-iframe-${this.iframeId++}`; iframe.title = "Pipedream Connect"; iframe.src = `${this.iframeURL}?${qp.toString()}`; iframe.style.cssText = "position:fixed;inset:0;z-index:2147483647;border:0;display:block;overflow:hidden auto"; iframe.width = "100%"; iframe.height = "100%"; - iframe.onload = () => { - this.iframe = iframe; - }; + this.iframe = iframe; document.body.appendChild(iframe); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/sdk/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
packages/sdk/package.json(2 hunks)packages/sdk/src/browser/async.ts(1 hunks)packages/sdk/src/browser/index.ts(9 hunks)packages/sdk/src/server/__tests__/server.test.ts(3 hunks)packages/sdk/src/server/async.ts(1 hunks)packages/sdk/src/server/cli.ts(1 hunks)packages/sdk/src/server/index.ts(11 hunks)packages/sdk/src/shared/async.ts(1 hunks)packages/sdk/src/shared/component.ts(1 hunks)packages/sdk/src/shared/index.ts(1 hunks)packages/sdk/tsconfig.browser.json(1 hunks)packages/sdk/tsconfig.node.json(1 hunks)
🧰 Additional context used
🪛 eslint
packages/sdk/src/server/cli.ts
[error] 4-4: Expected a line break after this opening brace.
(object-curly-newline)
[error] 4-4: Expected a line break before this closing brace.
(object-curly-newline)
[error] 17-18: Missing trailing comma.
(comma-dangle)
[error] 95-95: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 95-95: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 109-109: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 109-109: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 109-109: Expected a line break after this opening brace.
(object-curly-newline)
[error] 109-109: Expected a line break before this closing brace.
(object-curly-newline)
[error] 135-135: Expected a line break after this opening brace.
(object-curly-newline)
[error] 135-135: Expected a line break before this closing brace.
(object-curly-newline)
[error] 178-178: Expected a line break after this opening brace.
(object-curly-newline)
[error] 178-178: Expected a line break before this closing brace.
(object-curly-newline)
packages/sdk/src/server/index.ts
[error] 8-8: Expected a line break after this opening brace.
(object-curly-newline)
[error] 8-8: 'GetAccountOpts' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 8-8: Expected a line break before this closing brace.
(object-curly-newline)
[error] 10-10: Strings must use doublequote.
(quotes)
[error] 171-171: A linebreak is required after '['.
(array-bracket-newline)
[error] 171-171: There should be a linebreak after this element.
(array-element-newline)
[error] 171-171: A linebreak is required before ']'.
(array-bracket-newline)
[error] 173-174: Missing trailing comma.
(comma-dangle)
[error] 179-179: Expected a line break after this opening brace.
(object-curly-newline)
[error] 179-179: Expected a line break before this closing brace.
(object-curly-newline)
[error] 256-257: Missing trailing comma.
(comma-dangle)
[error] 282-283: Missing trailing comma.
(comma-dangle)
packages/sdk/src/shared/component.ts
[error] 2-2: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 3-3: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 6-6: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 7-7: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 8-8: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 9-9: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 10-10: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 11-11: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 12-12: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 13-13: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 14-15: Missing semicolon.
(@typescript-eslint/semi)
[error] 17-17: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 17-18: Missing semicolon.
(@typescript-eslint/semi)
[error] 20-20: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 21-21: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 22-22: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 23-24: Missing semicolon.
(@typescript-eslint/semi)
[error] 25-25: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 26-27: Missing semicolon.
(@typescript-eslint/semi)
[error] 28-28: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 29-29: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 30-31: Missing semicolon.
(@typescript-eslint/semi)
[error] 31-31: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 31-32: Missing semicolon.
(@typescript-eslint/semi)
[error] 33-33: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 34-34: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 35-35: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 36-37: Missing semicolon.
(@typescript-eslint/semi)
[error] 38-38: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 39-40: Missing semicolon.
(@typescript-eslint/semi)
[error] 41-41: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 42-42: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 43-44: Missing semicolon.
(@typescript-eslint/semi)
[error] 45-45: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 46-46: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 47-47: Missing semicolon.
(@typescript-eslint/semi)
[error] 63-63: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 63-64: Missing semicolon.
(@typescript-eslint/semi)
[error] 65-66: Missing semicolon.
(@typescript-eslint/semi)
[error] 72-72: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 83-84: Missing semicolon.
(@typescript-eslint/semi)
[error] 87-88: Missing semicolon.
(@typescript-eslint/semi)
[error] 91-91: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 92-92: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 93-93: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
[error] 94-94: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
packages/sdk/src/shared/index.ts
[error] 595-595: Don't use {} as a type. {} actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you want a type meaning "empty object", you probably want
Record<string, never>instead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
🪛 Biome
packages/sdk/src/shared/index.ts
[error] 49-53: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
[error] 237-241: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.
(lint/suspicious/noConstEnum)
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 595-595: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (15)
packages/sdk/tsconfig.browser.json (1)
4-4: LGTM! Upgrading to es2017 enables better async operations.
The target upgrade to es2017 is appropriate for the SDK's new direct API calling capabilities, as it provides native support for async/await and other modern features. This version has excellent browser support (>95% globally).
Let's verify the browser support for es2017 features:
packages/sdk/tsconfig.node.json (1)
4-7: LGTM! Appropriate upgrade to ES2017 for better async/await support.
The upgrade from ES6 to ES2017 is a good choice as it enables better async/await patterns, which aligns well with the PR's goal of improving API call handling. ES2017 is well-supported in Node.js environments.
Let's verify the Node.js version requirements in package.json:
✅ Verification successful
ES2017 target is compatible with Node.js requirements
The upgrade to ES2017 in the TypeScript configuration is well-supported by the Node.js version requirement (>=18.0.0) specified in package.json. Node.js 18+ has full support for ES2017 features including async/await.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Node.js version requirements to ensure ES2017 compatibility
# Look for engine requirements in package.json
fd -t f "package.json" -x jq -r '.engines.node // "Not specified"'
Length of output: 31085
Script:
#!/bin/bash
# Let's specifically check the package.json in the SDK package directory
fd -t f "package.json" -x realpath {} \; -x jq -r '. | {name: .name, engines: .engines}' {} \; | grep -B 1 "sdk"
Length of output: 88756
Script:
#!/bin/bash
# Let's try a simpler command to check package.json in the SDK directory
fd -t f "package.json" --exec cat {} \; | grep -A 5 -B 5 '"engines"'
Length of output: 498
packages/sdk/src/browser/async.ts (2)
1-2: LGTM! Clean and focused imports
The imports are minimal and appropriate for the functionality needed.
9-24: Verify integration with AsyncResponseManager and BrowserClient
The implementation looks solid, but let's verify the integration points:
✅ Verification successful
Let me try a different approach to verify the integration points.
Integration with AsyncResponseManager and BrowserClient is correctly implemented
The verification shows that:
BrowserAsyncResponseManagercorrectly extends the abstractAsyncResponseManagerclass and implements the required abstractgetOpts()method with the correct return typeAsyncResponseManagerOptsBrowserClientproperly initializes and usesBrowserAsyncResponseManagerwith the required configuration- The implementation maintains type safety and interface compatibility across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check AsyncResponseManager base class interface
echo "Checking AsyncResponseManager base class..."
ast-grep --pattern 'class AsyncResponseManager {
$$$
protected getOpts(): Promise<AsyncResponseManagerOpts> {
$$$
}
$$$
}'
# Check BrowserClient usage
echo "Checking BrowserClient usage..."
ast-grep --pattern 'class BrowserClient {
$$$
private asyncResponseManager: BrowserAsyncResponseManager;
$$$
}'
Length of output: 426
Script:
#!/bin/bash
# Search for AsyncResponseManager class definition and its abstract methods
echo "Searching for AsyncResponseManager class..."
rg "class AsyncResponseManager" -A 10
# Search for BrowserAsyncResponseManager usage
echo "Searching for BrowserAsyncResponseManager usage..."
rg "BrowserAsyncResponseManager" -A 3
# Search for AsyncResponseManagerOpts type
echo "Searching for AsyncResponseManagerOpts type..."
rg "AsyncResponseManagerOpts" -A 3
Length of output: 5816
packages/sdk/package.json (3)
11-24: Well-structured exports configuration!
The exports field properly defines separate entry points for browser and server environments, with appropriate type definitions. This configuration follows Node.js best practices for dual-package hazard prevention.
38-38: Verify CLI functionality and consider adding documentation
The addition of the CLI script suggests new command-line functionality. Since this is a significant new feature:
- Ensure the CLI entry point (
cli.js) is properly built during the build process - Consider adding CLI usage documentation to the README
Also applies to: 44-45
53-54: Review architectural implications of WebSocket support
The addition of @rails/actioncable and ws dependencies suggests implementation of WebSocket functionality. Given that this PR aims to enable direct API calls:
- Ensure WebSocket connections are properly secured for client-side usage
- Consider documenting any rate limiting or connection pooling strategies
- Verify browser compatibility of the WebSocket implementation
Also applies to: 56-56, 64-67
✅ Verification successful
Let me gather more information about the WebSocket implementation and security measures.
Let me check the actual implementation details of the WebSocket connection setup and security measures.
WebSocket implementation is secure and well-structured
The implementation shows proper security measures and architectural considerations:
- Uses secure WebSocket (
wss://) protocol by default - Implements authentication via tokens (OAuth for server, connect tokens for browser)
- Handles connection lifecycle properly (connect, disconnect, reconnect)
- Separates browser and server implementations with appropriate security contexts
- Includes proper error handling and cleanup mechanisms
No architectural concerns found regarding:
- Security: Authentication is enforced and uses secure protocols
- Browser compatibility: Uses standard WebSocket APIs through ActionCable
- Connection management: Implements proper lifecycle hooks and cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for WebSocket security configurations and browser compatibility checks
rg -A 5 'WebSocket|actioncable' packages/sdk/src
Length of output: 1487
Script:
#!/bin/bash
# Look for WebSocket connection setup and security configurations
ast-grep --pattern 'createConsumer($$$)'
# Check for any SSL/TLS or security-related configurations
rg -l 'wss:|https:|ssl|tls|secure' packages/sdk/src/
# Look for any browser compatibility checks
rg -l 'browser|userAgent|navigator' packages/sdk/src/
Length of output: 587
Script:
#!/bin/bash
# Check WebSocket connection setup in async.ts
cat packages/sdk/src/shared/async.ts
# Check security configurations in browser implementation
cat packages/sdk/src/browser/async.ts
# Check server-side implementation
cat packages/sdk/src/server/async.ts
Length of output: 5428
packages/sdk/src/server/__tests__/server.test.ts (1)
252-252: LGTM! Environment header addition is well-placed.
The addition of the "X-PD-Environment" header is a good practice for environment-aware API calls, which aligns with the PR's objective of enabling direct client-side API calls.
packages/sdk/src/shared/component.ts (2)
46-46: Confirm support for secret in ConfigurablePropStringArray
The comment // TODO is this supported indicates uncertainty about the secret property in ConfigurablePropStringArray. It's important to verify whether the secret attribute is valid for this type.
I can assist in verifying if the secret property is supported for string[] types. Let me know if you'd like me to look into this or open a GitHub issue to track it.
🧰 Tools
🪛 eslint
[error] 46-46: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
21-21: Verify the alertType property values
The alertType property includes a // TODO check the types comment. It's essential to confirm that the specified string literals match the allowed alert types in your application or UI library.
I can help verify the correct alertType values. Would you like me to check the documentation or source code to confirm the valid options?
Alternatively, run the following script to identify the allowed alertType values in your codebase:
✅ Verification successful
The alertType values are correctly defined
The verification confirms that the string literals "info" | "neutral" | "warning" | "error" in the type definition match the actual usage patterns across the codebase. All these values are actively used:
"info": Most common alert type, used extensively across components"neutral": Used in google_sheets component"warning": Used in multiple components including certifier, dropbox, github"error": Used in components like canva_enterprise, youtube_data_api
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for all usages of `alertType` to determine valid values.
# Search for `alertType` assignments and log the unique values.
rg 'alertType:\s*"(\w+)"' -o --replace '$1' | sort | uniq
Length of output: 6438
🧰 Tools
🪛 eslint
[error] 21-21: Expected a semicolon.
(@typescript-eslint/member-delimiter-style)
packages/sdk/src/shared/async.ts (3)
1-5: Imports are correctly defined.
The necessary modules and types are properly imported from @rails/actioncable.
40-45: Consider using cryptographically secure random string generation.
For generating async handles, it's advisable to use a cryptographically secure method to prevent potential collisions or security issues.
For browser environments:
function randomString(n: number) {
const array = new Uint8Array(n);
crypto.getRandomValues(array);
const alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
return Array.from(array, (byte) => alphabet[byte % alphabet.length]).join('');
}For Node.js environments:
import { randomBytes } from 'crypto';
function randomString(n: number) {
const bytes = randomBytes(n);
const alphabet = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
return Array.from(bytes, (byte) => alphabet[byte % alphabet.length]).join('');
}Ensure compatibility with your target environments and that the change meets security requirements.
109-109: Ensure all subclasses implement the getOpts method.
Since getOpts is an abstract method, verify that all subclasses provide a concrete implementation.
Review subclasses like BrowserAsyncResponseManager and ServerAsyncResponseManager to confirm they correctly implement getOpts.
packages/sdk/src/server/index.ts (1)
28-28: 🛠️ Refactor suggestion
Rename type to 'ServerClientOpts'
The type BackendClientOpts should be renamed to ServerClientOpts to reflect the change from BackendClient to ServerClient.
Apply this change:
-export type BackendClientOpts = {
+export type ServerClientOpts = {Likely invalid or redundant comment.
packages/sdk/src/browser/index.ts (1)
166-203: Potential race condition in token retrieval
The token() method may have a race condition if multiple concurrent calls are made before _tokenRequest is set. Ensuring that token requests are properly serialized can prevent unexpected behaviors.
Consider using a locking mechanism or adjusting the logic to handle concurrent token requests safely. This ensures that only one token request is in flight and subsequent calls await the same promise.
private async token() {
if (
this._token &&
this._tokenExpiresAt &&
this._tokenExpiresAt > new Date()
) {
return this._token;
}
if (this._tokenRequest) {
return this._tokenRequest;
}
const tokenCallback = this.tokenCallback;
const externalUserId = this.externalUserId;
if (!tokenCallback) {
throw new Error("No token callback provided");
}
if (!externalUserId) {
throw new Error("No external user ID provided");
}
// Ensure only one token request is in-flight at a time.
this._tokenRequest = (async () => {
try {
const { token, expires_at } = await tokenCallback({
externalUserId: externalUserId,
});
this._token = token;
this._tokenExpiresAt = new Date(expires_at);
return token;
} finally {
this._tokenRequest = undefined;
}
})();
return this._tokenRequest;
}Ensure that concurrent calls to token() are properly synchronized.
| protected override async getOpts(): Promise<AsyncResponseManagerOpts> { | ||
| const token = await this.browserOpts.getConnectToken(); | ||
| const url = `wss://${this.browserOpts.apiHost}/websocket?ctok=${token}`; | ||
| return { | ||
| url, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add error handling and security enhancements
The WebSocket URL construction and token handling could benefit from additional security measures and error handling:
- Token retrieval should have proper error handling
- Consider moving token to WebSocket headers instead of URL query parameters to prevent logging exposure
- Add token format validation before use
Here's a suggested implementation:
protected override async getOpts(): Promise<AsyncResponseManagerOpts> {
- const token = await this.browserOpts.getConnectToken();
- const url = `wss://${this.browserOpts.apiHost}/websocket?ctok=${token}`;
+ let token: string;
+ try {
+ token = await this.browserOpts.getConnectToken();
+ if (!token?.trim()) {
+ throw new Error('Invalid token received');
+ }
+ } catch (error) {
+ throw new Error(`Failed to retrieve connect token: ${error.message}`);
+ }
+
+ const url = new URL(`wss://${this.browserOpts.apiHost}/websocket`);
return {
url,
+ headers: {
+ 'Authorization': `Bearer ${token}`
+ }
};
}Committable suggestion skipped: line range outside the PR's diff.
| if (typeof adapters.WebSocket === "undefined") | ||
| adapters.WebSocket == WS; |
There was a problem hiding this comment.
Fix assignment operator in WebSocket adapter setup
There's an incorrect use of the equality operator == instead of the assignment operator = when setting adapters.WebSocket. This will result in a comparison rather than assigning the WS module to adapters.WebSocket.
Apply this diff to correct the issue:
if (typeof adapters.WebSocket === "undefined")
- adapters.WebSocket == WS;
+ adapters.WebSocket = WS;📝 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 (typeof adapters.WebSocket === "undefined") | |
| adapters.WebSocket == WS; | |
| if (typeof adapters.WebSocket === "undefined") | |
| adapters.WebSocket = WS; |
| const token = await this.serverOpts.getOauthToken(); | ||
| const projectId = await this.serverOpts.getProjectId(); | ||
| return { | ||
| url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${token}`, |
There was a problem hiding this comment.
Extract access token string from AccessToken object before use
The getOauthToken() function returns an AccessToken object from simple-oauth2, but you're using it directly in the WebSocket URL as if it's a string. You need to extract the access token string from this object before using it.
Modify your code to extract the access token string:
const token = await this.serverOpts.getOauthToken();
+ const accessToken = token.token.access_token;Then use accessToken in the URL:
- url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${token}`,
+ url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${accessToken}`,Also, please consider the security implications of passing tokens in the URL as mentioned in the next comment.
📝 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 token = await this.serverOpts.getOauthToken(); | |
| const projectId = await this.serverOpts.getProjectId(); | |
| return { | |
| url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${token}`, | |
| const token = await this.serverOpts.getOauthToken(); | |
| const accessToken = token.token.access_token; | |
| const projectId = await this.serverOpts.getProjectId(); | |
| return { | |
| url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${accessToken}`, |
| const token = await this.serverOpts.getOauthToken(); | ||
| const projectId = await this.serverOpts.getProjectId(); | ||
| return { | ||
| url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${token}`, |
There was a problem hiding this comment.
Avoid passing OAuth token in WebSocket URL to prevent security risks
Passing the OAuth token as a query parameter in the WebSocket URL can expose the token in server logs or browser history, posing a security risk. It's more secure to authenticate WebSocket connections using headers or through the subscription parameters.
Consider modifying the code to pass the token within the subscriptionParams:
- url: `wss://${this.serverOpts.apiHost}/websocket?oauth_token=${accessToken}`,
+ url: `wss://${this.serverOpts.apiHost}/websocket`,And include the token in subscriptionParams:
subscriptionParams: {
project_id: projectId,
+ oauth_token: accessToken,
},Ensure that the server-side WebSocket implementation supports token authentication via subscription parameters.
Committable suggestion skipped: line range outside the PR's diff.
packages/sdk/src/shared/index.ts
Outdated
| dynamic_props_id: opts.dynamicPropsId, | ||
| environment: this.environment, | ||
| }; | ||
| return await this.makeConnectRequestAsync<{}>("/components/props", { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using '{}' as a type; specify a more precise type.
Using {} as a type means "any non-nullish value," which may not be the intended usage. Consider specifying a more precise type or using unknown or object if applicable.
Apply this diff:
-return await this.makeConnectRequestAsync<{}>("/components/props", {
+return await this.makeConnectRequestAsync<Record<string, never>>("/components/props", {Or, if the expected response type is unknown:
-return await this.makeConnectRequestAsync<{}>("/components/props", {
+return await this.makeConnectRequestAsync<unknown>("/components/props", {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome
[error] 595-595: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 eslint
[error] 595-595: Don't use {} as a type. {} actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you want a type meaning "empty object", you probably want
Record<string, never>instead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
| GetAccountOpts, | ||
| type ConnectTokenResponse, | ||
| } from "../shared"; | ||
| export type * from "../shared"; |
There was a problem hiding this comment.
Incorrect TypeScript export syntax
The line export type * from "../shared"; is not valid TypeScript syntax. To re-export all types from a module, you should explicitly list the types or use export * if you intend to export all exports (both types and values).
Apply this diff to fix the export statement:
-export type * from "../shared";
+export * from "../shared";Or, if you intend to export only types, list them explicitly:
export type { TypeA, TypeB } from "../shared";| protected async authHeaders(): Promise<string> { | ||
| if (!(await this.token())) { | ||
| throw new Error("No token provided"); | ||
| } | ||
| return `Bearer ${await this.token()}`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate asynchronous calls to this.token()
In the authHeaders method, await this.token() is called twice, which can lead to unnecessary asynchronous operations and potential performance issues.
Apply this diff to store the token in a variable and reuse it:
protected async authHeaders(): Promise<string> {
- if (!(await this.token())) {
+ const token = await this.token();
+ if (!token) {
throw new Error("No token provided");
}
- return `Bearer ${await this.token()}`;
+ return `Bearer ${token}`;
}📝 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.
| protected async authHeaders(): Promise<string> { | |
| if (!(await this.token())) { | |
| throw new Error("No token provided"); | |
| } | |
| return `Bearer ${await this.token()}`; | |
| } | |
| protected async authHeaders(): Promise<string> { | |
| const token = await this.token(); | |
| if (!token) { | |
| throw new Error("No token provided"); | |
| } | |
| return `Bearer ${token}`; | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (15)
packages/sdk/CHANGELOG.md (1)
4-4: Consider bumping to version 2.0.0The introduction of direct client API calls represents a significant architectural change that might affect backward compatibility. According to semantic versioning principles, this warrants a major version bump.
packages/sdk/src/shared/component.ts (1)
90-90: Reconsider usinganyas default type parameterUsing
anyas the default type parameter forV1Componentreduces type safety. Consider using a more restrictive default type.export type V1Component<T extends ConfigurableProps = ConfigurableProps> = { name: string; key: string; version: string; configurable_props: T; };packages/sdk/src/browser/index.ts (3)
164-201: Consider adding synchronization to token refreshWhile the token management implementation is robust, there's a potential race condition between checking token expiration and setting the new token. If multiple calls occur simultaneously when the token is about to expire, they might all trigger token refresh.
Consider this safer implementation:
private async token() { + // Acquire lock first to prevent race conditions + if (this._tokenRequest) { + return this._tokenRequest; + } + + // Create the request promise before checking expiration + // to ensure consistent state + this._tokenRequest = (async () => { if ( this._token && this._tokenExpiresAt && this._tokenExpiresAt > new Date() ) { - return this._token; + const token = this._token; + this._tokenRequest = undefined; + return token; } - if (this._tokenRequest) { - return this._tokenRequest; - } - const tokenCallback = this.tokenCallback; const externalUserId = this.externalUserId; if (!tokenCallback) { throw new Error("No token callback provided"); } if (!externalUserId) { throw new Error("No external user ID provided"); } - // Ensure only one token request is in-flight at a time. - this._tokenRequest = (async () => { - const { - token, expires_at, - } = await tokenCallback({ - externalUserId: externalUserId, - }); - this._token = token; - this._tokenExpiresAt = new Date(expires_at); - this._tokenRequest = undefined; - return token; - })(); + const { + token, expires_at, + } = await tokenCallback({ + externalUserId: externalUserId, + }); + this._token = token; + this._tokenExpiresAt = new Date(expires_at); + this._tokenRequest = undefined; + return token; + })(); return this._tokenRequest; }
Line range hint
226-252: Consider improving the connection flowThe current implementation has a few areas that could be enhanced:
- The token refresh after connection is handled implicitly
- Error handling could be more specific
- The cleanup process could be more robust
Consider these improvements:
public async connectAccount(opts: StartConnectOpts) { + let cleanup: (() => void) | undefined; + const onMessage = (e: MessageEvent) => { switch (e.data?.type) { case "success": opts.onSuccess?.({ id: e.data?.authProvisionId, }); + // Explicitly handle token refresh + this.refreshToken(); break; case "error": + // Provide more specific error information + const errorMessage = e.data.error || "Unknown connection error"; - opts.onError?.(new ConnectError(e.data.error)); + opts.onError?.(new ConnectError(errorMessage)); break; case "close": - this.cleanup(onMessage); + if (cleanup) cleanup(); break; default: break; } }; window.addEventListener("message", onMessage); + cleanup = () => { + this.iframe?.remove(); + window.removeEventListener("message", onMessage); + cleanup = undefined; + }; try { await this.createIframe(opts); } catch (err) { opts.onError?.(err as ConnectError); + if (cleanup) cleanup(); } - this.refreshToken(); // token expires once it's used to create a connected account }
Line range hint
274-304: Enhance iframe security measuresWhile the iframe implementation follows basic security practices, consider adding additional security measures:
Apply these security enhancements:
private async createIframe(opts: StartConnectOpts) { const token = opts.token || (await this.token()); const qp = new URLSearchParams({ token, }); if (typeof opts.app === "string") { qp.set("app", opts.app); } else { throw new ConnectError("Object app not yet supported"); } if (opts.oauthAppId) { qp.set("oauthAppId", opts.oauthAppId); } const iframe = document.createElement("iframe"); iframe.id = `pipedream-connect-iframe-${this.iframeId++}`; iframe.title = "Pipedream Connect"; iframe.src = `${this.iframeURL}?${qp.toString()}`; + // Add security attributes + iframe.sandbox = "allow-scripts allow-same-origin allow-forms"; + iframe.referrerPolicy = "strict-origin"; iframe.style.cssText = "position:fixed;inset:0;z-index:2147483647;border:0;display:block;overflow:hidden auto"; iframe.width = "100%"; iframe.height = "100%"; iframe.onload = () => { this.iframe = iframe; + // Verify origin before storing iframe reference + const iframeOrigin = new URL(iframe.src).origin; + if (iframeOrigin !== new URL(this.baseURL).origin) { + iframe.remove(); + throw new ConnectError("Invalid iframe origin"); + } }; document.body.appendChild(iframe); }packages/sdk/src/server/index.ts (3)
9-9: Remove unused import 'GetAccountOpts'The 'GetAccountOpts' type is imported but never used in this file.
- Account, BaseClient, GetAccountOpts, type AppInfo, type ConnectTokenResponse, + Account, BaseClient, type AppInfo, type ConnectTokenResponse,🧰 Tools
🪛 eslint
[error] 9-9: 'GetAccountOpts' is defined but never used.
(@typescript-eslint/no-unused-vars)
158-169: Enhance WebSocket error handling in asyncResponseManagerWhile the WebSocket implementation is good, consider adding error handling for connection failures and reconnection logic.
Consider adding error handling like this:
this.asyncResponseManager = new ServerAsyncResponseManager({ apiHost: this.apiHost, getOauthToken: async () => { await this.ensureValidOauthToken(); return this.oauthToken as AccessToken; }, getProjectId: () => { if (!this.projectId) throw "Attempted to connect to websocket without a valid Project id"; return this.projectId; }, + onError: (error) => { + console.error('WebSocket connection error:', error); + // Implement reconnection logic here + }, });
224-226: Enhance OAuth error messageThe current error message could be more informative by including the HTTP status code and response body when available.
- throw new Error(`Failed to obtain OAuth token: ${error.message}`); + throw new Error(`Failed to obtain OAuth token: ${error.message}${ + error.response ? ` (Status: ${error.response.status}, Body: ${JSON.stringify(error.response.data)})` : '' + }`);packages/sdk/src/shared/index.ts (3)
392-397: Enhance error handling structureConsider improving error handling by creating a custom error class that preserves HTTP status codes and structured error responses.
+class APIError extends Error { + constructor( + public status: number, + public body: string, + public structuredError?: unknown + ) { + super(`HTTP error! status: ${status}, body: ${body}`); + this.name = 'APIError'; + } +} if (!response.ok) { const errorBody = await response.text(); - throw new Error( - `HTTP error! status: ${response.status}, body: ${errorBody}`, - ); + let structuredError; + try { + structuredError = JSON.parse(errorBody); + } catch { + // If parsing fails, use raw error body + } + throw new APIError(response.status, errorBody, structuredError); }
507-507: Address TODO/XXX commentsThere are several TODO/XXX comments indicating temporary code or missing functionality:
- Line 507: "XXX only here while need project auth"
- Line 531: "XXX only here while need project auth"
- Line 542: "XXX can just use /components and ?type instead when supported"
Would you like me to help implement these missing features or create GitHub issues to track them?
Also applies to: 531-531, 542-542
533-535: Make the limit parameter configurableThe limit parameter is hardcoded to "20". Consider making this configurable through the options parameter.
const params: Record<string, string> = { - limit: "20", + limit: opts?.limit?.toString() || "20", };And update the type definition:
export type GetComponentOpts = { q?: string; app?: string; componentType?: "trigger" | "action"; + limit?: number; };packages/sdk/src/server/cli.ts (4)
110-117: Refine the '--include-credentials' option to be a boolean flagThe
--include-credentialsoption currently requires a value, which might not be intuitive for users expecting it to be a simple flag. Consider changing it to a boolean flag that, when present, includes credentials in the response.Apply this diff to adjust the option:
.command("get-accounts") .description("Retrieve the list of accounts associated with the project.") - .option("--include-credentials <include>", "Include credentials in the response") + .option("--include-credentials", "Include credentials in the response") .action(async (options) => { try { - const params = options.includeCredentials + const params = options.includeCredentials ? { - include_credentials: options.includeCredentials, + include_credentials: true, } : {}; const accounts = await client.getAccounts(params);
207-218: Enhance error handling for JSON parsingWhen parsing
configuredProps, if the JSON is invalid, the error message might not clearly indicate the problem. Consider adding specific error handling for JSON parsing to provide more informative feedback to the user.Apply this diff to improve error handling:
try { const configuredProps = JSON.parse(options.configuredProps); +} catch (parseError) { + console.error("Failed to parse 'configured-props'. Please ensure it is a valid JSON string."); + process.exit(1); } const response = await client.componentConfigure({
230-241: Improve JSON parsing error feedbackSimilar to the previous comment, enhancing the error handling for parsing
configuredPropswill help users diagnose issues more effectively.Apply this diff:
try { const configuredProps = JSON.parse(options.configuredProps); +} catch (parseError) { + console.error("Failed to parse 'configured-props'. Please ensure it is a valid JSON string."); + process.exit(1); } const response = await client.componentReloadProps({
252-263: Add specific error message for invalid JSON inputTo aid in troubleshooting, provide a clear error message if
configuredPropscannot be parsed.Apply this diff:
try { const configuredProps = JSON.parse(options.configuredProps); +} catch (parseError) { + console.error("Failed to parse 'configured-props'. Please ensure it is a valid JSON string."); + process.exit(1); } const response = await client.actionRun({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/package.json(2 hunks)packages/sdk/src/browser/index.ts(9 hunks)packages/sdk/src/server/cli.ts(1 hunks)packages/sdk/src/server/index.ts(10 hunks)packages/sdk/src/shared/component.ts(1 hunks)packages/sdk/src/shared/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/package.json
🧰 Additional context used
🪛 eslint
packages/sdk/src/server/index.ts
[error] 9-9: 'GetAccountOpts' is defined but never used.
(@typescript-eslint/no-unused-vars)
packages/sdk/src/shared/index.ts
[error] 595-595: Don't use {} as a type. {} actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you want a type meaning "empty object", you probably want
Record<string, never>instead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
🪛 Biome
packages/sdk/src/shared/index.ts
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 595-595: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (10)
packages/sdk/CHANGELOG.md (1)
8-8: Verify security implications of direct client API calls
The change to allow direct API calls from the client requires careful security consideration.
Let's verify the security measures in place:
✅ Verification successful
Let me gather more specific information about the client token implementation.
Based on the search results, I can now generate a final response regarding the security implications of direct client API calls.
Client tokens have appropriate security controls in place
The implementation shows several security measures for client API tokens:
- Tokens are short-lived and expire after 4 hours
- Tokens are scoped to specific users and environments
- Tokens are salted, hashed and encrypted at rest
- Tokens can only initiate auth connection flows and have no permissions to access credentials
- CORS headers are properly configured to control cross-origin requests
- Rate limiting is implemented to prevent abuse
The change to allow direct client API calls appears to have been implemented with appropriate security controls. The token-based authentication system follows security best practices including:
- Limited token scope and permissions
- Short expiration times
- Secure token storage
- Rate limiting protection
- Environment isolation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security-related code patterns in the SDK
# Look for token validation patterns
echo "Checking token validation patterns..."
rg -A 5 "validateToken|verifyToken|tokenValidation"
# Check for rate limiting implementation
echo "Checking rate limiting patterns..."
rg -A 5 "rateLimit|rateLimiting|throttle"
# Look for CORS configuration
echo "Checking CORS configuration..."
rg -A 5 "cors.config|cors.options|Access-Control-Allow"
# Check for any security-related tests
echo "Checking security tests..."
rg -A 5 "test.*security|test.*authentication|test.*authorization"
Length of output: 17063
Script:
#!/bin/bash
# Search for client token related code and implementation
echo "Checking client token implementation..."
rg -A 10 "client.*token|connect.*token"
# Search for API endpoint security measures
echo "Checking API endpoint security..."
rg -A 10 "api\..*client|client\..*api"
# Look for authentication/authorization mechanisms
echo "Checking auth mechanisms..."
ast-grep --pattern 'function $FNAME($$$) {
$$$
authenticate($$$)
$$$
}'
Length of output: 79846
packages/sdk/src/shared/component.ts (5)
94-94: Consider consistent naming convention
The property configurable_props uses snake_case while the rest of the codebase appears to use camelCase. This inconsistency might indicate an external API contract.
Let's verify the naming convention:
If this represents an API contract, consider:
- Adding a comment documenting this is an API contract
- Using a type alias with camelCase for internal use
21-21: Verify alert types completeness
The TODO comment suggests uncertainty about the alert types. This needs verification to ensure all possible types are covered.
Let's search for alert type usage:
✅ Verification successful
Alert types are correctly defined and consistently used
Based on the codebase analysis, the alert types defined in the component ("info" | "neutral" | "warning" | "error") are the standard types used throughout the codebase. This is evidenced by:
- Consistent usage across multiple components showing these exact four types
- The types align with the API documentation (docs-v2/pages/components/api.mdx) which specifies the same set of values
- No instances found of other alert types being used that aren't covered by this definition
The TODO comment can be safely removed as the types are complete and verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for alert type usage in the codebase
rg -A 2 'alertType["\s]*:' --type typescript
Length of output: 81
Script:
#!/bin/bash
# Search for alert type usage in the codebase using ripgrep
rg -A 2 "alertType"
# Also search for any enum or type definitions related to alerts
rg -A 4 "type.*Alert|enum.*Alert"
Length of output: 38943
5-13: Review and document property applicability
The comment "XXX don't actually apply to all, fix" suggests that some optional properties might not be universally applicable. This needs to be addressed to prevent potential type safety issues.
Let's verify which properties apply to which types:
Consider:
- Creating separate interfaces for different property groups
- Adding documentation about which properties apply to which types
- Using TypeScript's utility types to make certain properties optional for specific types
82-82: Review array type handling strategy
The comment "XXX support arrays differently?" suggests uncertainty about array type handling. This needs to be addressed before the implementation becomes widely used.
Consider:
- Whether arrays should be handled via a generic array type
- If there are specific validation requirements for arrays
- Whether we need to support nested arrays or arrays of other types
Let's verify current array type usage:
41-43:
Review security implications of secret fields
The secret field in string types has security implications that need careful consideration:
- Ensure proper handling of secret values in the UI
- Verify secure storage and transmission
- Consider adding validation for secret values
Let's check how secrets are handled:
Also applies to: 45-47
packages/sdk/src/browser/index.ts (1)
38-54: LGTM: Well-structured type definitions for token management
The new types TokenCallback and token-related options in CreateBrowserClientOpts are well-documented and properly structured for managing client-side tokens.
packages/sdk/src/server/index.ts (1)
Line range hint 262-271: Verify impact of Connect token changes
The token creation has been updated with new options. Let's ensure all consumers are updated accordingly.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to ensure we have a complete picture.
The additional scripts will help us:
- Understand the new options type structure
- Verify if there are any lingering uses of the deprecated
environment_namefield
Based on the results, I can now generate the final response as we have sufficient information about the token changes and their impact.
Connect token changes are properly implemented
The changes to the Connect token creation are consistent across the codebase:
- All usages of
createConnectToken()have been updated to use the newConnectTokenCreateOptstype - The deprecated
environment_namefield has been removed with no remaining usage - All consumers (CLI, tests, and documentation) are using the correct parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createConnectToken usage in the codebase
rg -A 5 "createConnectToken\("
# Search for any remaining uses of the old options type
rg "ConnectTokenOpts"
Length of output: 2763
Script:
#!/bin/bash
# Search for the new ConnectTokenCreateOpts type definition and its usage
rg "ConnectTokenCreateOpts" -A 5
# Search for environment_name usage in token creation
rg -A 5 "environment_name.*createConnectToken|createConnectToken.*environment_name"
Length of output: 1207
packages/sdk/src/shared/index.ts (2)
595-595: 🛠️ Refactor suggestion
Replace {} type with a more specific type
The {} type means "any non-nullish value". Consider using a more specific type.
-return await this.makeConnectRequestAsync<{}>("/components/props", {
+return await this.makeConnectRequestAsync<Record<string, never>>("/components/props", {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 595-595: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🪛 eslint
[error] 595-595: Don't use {} as a type. {} actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want
objectinstead. - If you want a type meaning "any value", you probably want
unknowninstead. - If you want a type meaning "empty object", you probably want
Record<string, never>instead. - If you really want a type meaning "any non-nullish value", you probably want
NonNullable<unknown>instead.
(@typescript-eslint/ban-types)
826-826: Verify token authentication functionality
There's a TODO comment about testing token authentication. This is critical for security.
| - Use client Connect tokens to make api calls directly from the client. | ||
| - Deprecated the `environments` property on `createFrontendClient` since it is now | ||
| stored in the token | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the deprecation notice with migration guidance
The deprecation notice for the environments property should include:
- The rationale behind the deprecation
- Migration steps for existing users
- Timeline for removal (if planned)
Consider updating the changelog entry like this:
- Deprecated the `environments` property on `createFrontendClient` since it is now
stored in the token
+ Deprecated the `environments` property on `createFrontendClient`. The environment
+ configuration is now embedded in the Connect token for improved security and
+ simplified setup. To migrate:
+ 1. Remove the environments property from createFrontendClient calls
+ 2. Ensure your Connect tokens are configured with the correct environment
+ This property will be removed in version 2.0.0📝 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.
| - Use client Connect tokens to make api calls directly from the client. | |
| - Deprecated the `environments` property on `createFrontendClient` since it is now | |
| stored in the token | |
| - Use client Connect tokens to make api calls directly from the client. | |
| - Deprecated the `environments` property on `createFrontendClient`. The environment | |
| configuration is now embedded in the Connect token for improved security and | |
| simplified setup. To migrate: | |
| 1. Remove the environments property from createFrontendClient calls | |
| 2. Ensure your Connect tokens are configured with the correct environment | |
| This property will be removed in version 2.0.0 | |
| // XXX fix duplicating mapping to value type here and with PropValue | ||
| type Defaultable<T> = { default?: T; options?: T[]; }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Resolve type mapping duplication
There appears to be duplicate type mapping between Defaultable<T> and PropValue. This could lead to maintenance issues if the mappings diverge.
Consider consolidating the type mappings:
// Define a single source of truth for value types
type ValueTypes = {
alert: never;
any: any;
app: { authProvisionId: string };
boolean: boolean;
integer: number;
object: object;
string: string;
"string[]": string[];
};
// Use it in both places
type Defaultable<T> = {
default?: T;
options?: T[];
};
type PropValue<T extends keyof ValueTypes> = ValueTypes[T];| * Specify which origins can use the token to call the Pipedream API. | ||
| */ | ||
| name_slug: string; | ||
| allowed_origins?: string[]; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the searches performed, I can now generate the final response:
allowed_origins implementation needs validation
While the allowed_origins parameter is defined in the API token configuration, I couldn't find any implementation that validates incoming requests against these allowed origins. This security feature appears to be incomplete.
packages/sdk/src/server/index.ts: Defines theallowed_originsconfigurationpackages/sdk/src/server/cli.ts: Only handles passing the origins during token creation- No evidence of CORS or origin validation middleware that enforces this restriction
🔗 Analysis chain
Security enhancement with allowed_origins
The addition of allowed_origins is a crucial security feature that helps prevent unauthorized cross-origin requests.
Let's verify if this is properly validated:
Let me search for more context about the validation implementation.
Let me search for any token validation or middleware code.
Let me try one final search to find any token or origin validation in the server implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for allowed_origins validation in the codebase
rg -A 5 "allowed_origins"
Length of output: 718
Script:
#!/bin/bash
# Search for CORS or origin validation in the codebase
rg -A 10 -i "origin.*valid|cors.*origin"
# Search for any middleware or request handling related to origins
ast-grep --pattern 'function $FUNC($REQ, $RES, $$$) {
$$$
origin
$$$
}'
Length of output: 1141
Script:
#!/bin/bash
# Search for token validation or middleware code
rg -A 15 "validateToken|verifyToken|checkToken|middleware.*token"
# Search for any Express.js or HTTP server setup code
ast-grep --pattern 'app.$METHOD($PATH, $$$)'
Length of output: 1676
Script:
#!/bin/bash
# Search for token or origin validation in server implementation
rg -A 20 "class.*Server" packages/sdk/src/server/
# Look for any authentication or request handling middleware
ast-grep --pattern 'async function $FUNC($REQ, $RES, $NEXT) {
$$$
}'
Length of output: 8316
| program | ||
| .command("delete-account <accountId>") | ||
| .description("Delete an account by its ID.") | ||
| .action(async (accountId) => { | ||
| try { | ||
| await client.deleteAccount(accountId); | ||
| console.log(`Account with ID ${accountId} has been deleted.`); | ||
| } catch (error) { | ||
| handleError(error, "Failed to delete account"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation prompt before deleting an account
Deleting an account is a destructive action that cannot be undone. To prevent accidental deletions, consider adding a confirmation prompt or requiring a --force flag before executing the deletion.
Apply this diff to implement a --force option:
program
.command("delete-account <accountId>")
.description("Delete an account by its ID.")
+ .option("--force", "Force deletion without confirmation")
.action(async (accountId, options) => {
try {
+ if (!options.force) {
+ const prompt = require('prompt-sync')();
+ const confirmation = prompt(`Are you sure you want to delete account ${accountId}? This action cannot be undone. (yes/no): `);
+ if (confirmation.toLowerCase() !== 'yes') {
+ console.log("Account deletion canceled.");
+ return;
+ }
+ }
await client.deleteAccount(accountId);
console.log(`Account with ID ${accountId} has been deleted.`);
} catch (error) {
handleError(error, "Failed to delete account");
}
});📝 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.
| program | |
| .command("delete-account <accountId>") | |
| .description("Delete an account by its ID.") | |
| .action(async (accountId) => { | |
| try { | |
| await client.deleteAccount(accountId); | |
| console.log(`Account with ID ${accountId} has been deleted.`); | |
| } catch (error) { | |
| handleError(error, "Failed to delete account"); | |
| } | |
| }); | |
| program | |
| .command("delete-account <accountId>") | |
| .description("Delete an account by its ID.") | |
| .option("--force", "Force deletion without confirmation") | |
| .action(async (accountId, options) => { | |
| try { | |
| if (!options.force) { | |
| const prompt = require('prompt-sync')(); | |
| const confirmation = prompt(`Are you sure you want to delete account ${accountId}? This action cannot be undone. (yes/no): `); | |
| if (confirmation.toLowerCase() !== 'yes') { | |
| console.log("Account deletion canceled."); | |
| return; | |
| } | |
| } | |
| await client.deleteAccount(accountId); | |
| console.log(`Account with ID ${accountId} has been deleted.`); | |
| } catch (error) { | |
| handleError(error, "Failed to delete account"); | |
| } | |
| }); |
| program | ||
| .command("delete-accounts-by-app <appId>") | ||
| .description("Delete all accounts associated with a specific app.") | ||
| .action(async (appId) => { | ||
| try { | ||
| await client.deleteAccountsByApp(appId); | ||
| console.log(`All accounts associated with app ID ${appId} have been deleted.`); | ||
| } catch (error) { | ||
| handleError(error, "Failed to delete accounts by app"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement confirmation step before mass deletion of accounts
Deleting all accounts associated with a specific app is a significant action that can affect multiple users. To avoid unintended data loss, it is advisable to include a confirmation prompt or require an explicit --force flag.
Apply this diff to add a confirmation prompt:
program
.command("delete-accounts-by-app <appId>")
.description("Delete all accounts associated with a specific app.")
+ .option("--force", "Force deletion without confirmation")
.action(async (appId, options) => {
try {
+ if (!options.force) {
+ const prompt = require('prompt-sync')();
+ const confirmation = prompt(`Are you sure you want to delete all accounts associated with app ID ${appId}? This action cannot be undone. (yes/no): `);
+ if (confirmation.toLowerCase() !== 'yes') {
+ console.log("Bulk account deletion canceled.");
+ return;
+ }
+ }
await client.deleteAccountsByApp(appId);
console.log(`All accounts associated with app ID ${appId} have been deleted.`);
} catch (error) {
handleError(error, "Failed to delete accounts by app");
}
});Committable suggestion skipped: line range outside the PR's diff.
| program | ||
| .command("delete-external-user <externalId>") | ||
| .description("Delete all accounts associated with a specific external ID.") | ||
| .action(async (externalId) => { | ||
| try { | ||
| await client.deleteExternalUser(externalId); | ||
| console.log(`All accounts associated with external ID ${externalId} have been deleted.`); | ||
| } catch (error) { | ||
| handleError(error, "Failed to delete external user"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation prompt before deleting accounts of an external user
Deleting all accounts associated with an external user ID can have wide-reaching consequences. To prevent accidental deletions, include a confirmation prompt or require a --force option before proceeding.
Apply this diff to implement a confirmation step:
program
.command("delete-external-user <externalId>")
.description("Delete all accounts associated with a specific external ID.")
+ .option("--force", "Force deletion without confirmation")
.action(async (externalId, options) => {
try {
+ if (!options.force) {
+ const prompt = require('prompt-sync')();
+ const confirmation = prompt(`Are you sure you want to delete all accounts associated with external ID ${externalId}? This action cannot be undone. (yes/no): `);
+ if (confirmation.toLowerCase() !== 'yes') {
+ console.log("External user account deletion canceled.");
+ return;
+ }
+ }
await client.deleteExternalUser(externalId);
console.log(`All accounts associated with external ID ${externalId} have been deleted.`);
} catch (error) {
handleError(error, "Failed to delete external user");
}
});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/server/index.ts (2)
158-169: Improve error handling in async response managerThe error message in
getProjectIdcould be more descriptive and follow error handling best practices.Consider this improvement:
getProjectId: () => { if (!this.projectId) - throw "Attempted to connect to websocket without a valid Project id"; + throw new Error("Cannot establish websocket connection: Project ID is required"); return this.projectId; },
Line range hint
210-233: Enhance OAuth token refresh error handlingThe token refresh implementation has good retry logic but could benefit from more specific error handling.
Consider these improvements:
try { this.oauthToken = await this.oauthClient.getToken({}); - } catch (error: any) { - throw new Error(`Failed to obtain OAuth token: ${error.message}`); + } catch (error) { + const message = error instanceof Error ? error.message : 'Unknown error'; + throw new Error(`OAuth token refresh failed: ${message}`, { cause: error }); }packages/sdk/src/shared/index.ts (4)
595-595: Specify a more precise type for the API response.Using
Record<string, any>is too permissive and loses type safety. Consider defining a specific interface for the expected response structure.
596-596: Address the TODO comment.The TODO comment indicates that trigger functionality needs to be implemented.
Would you like me to help implement the trigger functionality or create a GitHub issue to track this task?
826-826: Address the TODO comment about token auth testing.The TODO comment suggests that token authentication testing needs to be implemented.
Would you like me to help create test cases for token authentication or create a GitHub issue to track this task?
739-776: Consider adding rate limiting protection.The
invokeWorkflowmethod might benefit from rate limiting to prevent abuse. Consider implementing a rate limiter or using a rate limiting middleware.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/src/server/index.ts(10 hunks)packages/sdk/src/shared/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/src/shared/index.ts
[error] 401-401: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
packages/sdk/src/server/index.ts (3)
139-139: Class name should be 'ServerClient'
The class is still named BackendClient but should be renamed to ServerClient to match the recent changes and maintain consistency with the AI summary and documentation.
88-90: allowed_origins implementation needs validation
While the allowed_origins parameter is defined in the API token configuration, there's no implementation that validates incoming requests against these allowed origins.
Line range hint 1-350: Verify impact of removed types
Several types have been removed (OAuthCredentials, ProjectEnvironment, ProjectInfoResponse, GetAccountByIdOpts). Please ensure these removals don't affect any existing code that might be importing these types.
packages/sdk/src/shared/index.ts (3)
656-705: Excellent security implementation in URL building!
The buildWorkflowUrl method implements robust security measures:
- Input sanitization to prevent injection attacks
- Proper URL validation and parsing
- Domain validation to prevent DNS rebinding attacks
- Clear error messages for invalid inputs
811-843: Comprehensive validation in invokeWorkflowForExternalUser.
The method includes thorough input validation and proper error handling:
- Validates external user ID
- Validates workflow URL
- Verifies authentication headers
- Properly sets required headers for external user identification
237-241: 🛠️ Refactor suggestion
Replace 'const enum' with 'enum' for better compatibility.
Using enum instead of const enum will ensure better compatibility with bundlers and TypeScript's 'isolatedModules' mode.
Apply this diff:
-export enum HTTPAuthType {
+export enum HTTPAuthType {
None = "none",
StaticBearer = "static_bearer_token",
OAuth = "oauth",
}Likely invalid or redundant comment.
WHY
Currently, the Pipedream Connect SDK only supports calling API endpoints from the server. This means that all client side calls need to be proxied by a trusted server.
This PR adds support for calling most API endpoints directly from the client.
Summary by CodeRabbit
Release Notes
New Features
BrowserAsyncResponseManagerandServerAsyncResponseManagerclasses for improved asynchronous response handling in browser and server contexts.Improvements
BrowserClientwith new methods for token retrieval and caching.environmentsproperty on thecreateFrontendClientmethod, streamlining client configuration.Bug Fixes
Documentation