Pipedream SDK #12685
Pipedream SDK #12685
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe recent update enhances the SDK by introducing structured environment configuration, refining user authentication, and streamlining API interactions within a Next.js application. Key additions include a detailed Changes
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 as PR comments)
Additionally, you can add 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: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (6)
packages/sdk/examples/next-app/app/favicon.icois excluded by!**/*.icopackages/sdk/examples/next-app/package-lock.jsonis excluded by!**/package-lock.jsonpackages/sdk/examples/next-app/public/next.svgis excluded by!**/*.svgpackages/sdk/examples/next-app/public/vercel.svgis excluded by!**/*.svgpackages/sdk/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (23)
- components/backblaze/backblaze.app.mjs (1 hunks)
- components/control_d/control_d.app.mjs (1 hunks)
- components/deftship/deftship.app.mjs (1 hunks)
- components/faceup/faceup.app.mjs (1 hunks)
- components/hypeauditor/hypeauditor.app.mjs (1 hunks)
- components/smartymeet/smartymeet.app.mjs (1 hunks)
- components/watsonx_ai/watsonx_ai.app.mjs (1 hunks)
- helpers/package.json (1 hunks)
- packages/sdk/examples/next-app/.gitignore (1 hunks)
- packages/sdk/examples/next-app/README.md (1 hunks)
- packages/sdk/examples/next-app/app/globals.css (1 hunks)
- packages/sdk/examples/next-app/app/layout.tsx (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/examples/next-app/next.config.mjs (1 hunks)
- packages/sdk/examples/next-app/package.json (1 hunks)
- packages/sdk/examples/next-app/postcss.config.js (1 hunks)
- packages/sdk/examples/next-app/tailwind.config.ts (1 hunks)
- packages/sdk/examples/next-app/tsconfig.json (1 hunks)
- packages/sdk/package.json (1 hunks)
- packages/sdk/src/browser/index.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
- packages/sdk/tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (15)
- components/backblaze/backblaze.app.mjs
- components/deftship/deftship.app.mjs
- components/faceup/faceup.app.mjs
- components/hypeauditor/hypeauditor.app.mjs
- components/smartymeet/smartymeet.app.mjs
- components/watsonx_ai/watsonx_ai.app.mjs
- helpers/package.json
- packages/sdk/examples/next-app/.gitignore
- packages/sdk/examples/next-app/README.md
- packages/sdk/examples/next-app/app/globals.css
- packages/sdk/examples/next-app/next.config.mjs
- packages/sdk/examples/next-app/package.json
- packages/sdk/examples/next-app/postcss.config.js
- packages/sdk/examples/next-app/tsconfig.json
- packages/sdk/tsconfig.json
Additional context used
Biome
packages/sdk/src/browser/index.ts
[error] 12-14: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
packages/sdk/examples/next-app/app/page.tsx
[error] 75-75: Provide a text alternative through the alt, aria-label or aria-labelledby attribute
Meaningful alternative text on elements helps users relying on screen readers to understand content's purpose within a page.
(lint/a11y/useAltText)
[error] 67-67: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 69-69: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 72-72: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 76-76: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
[error] 78-78: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
Additional comments not posted (17)
components/control_d/control_d.app.mjs (1)
11-11: LGTM!The addition of a newline at the end of the file is a good practice for consistency.
packages/sdk/examples/next-app/app/layout.tsx (1)
1-22: LGTM!The RootLayout component is well-structured and follows Next.js conventions. The metadata object and the use of the Inter font are correctly implemented.
packages/sdk/examples/next-app/tailwind.config.ts (1)
1-20: LGTM!The Tailwind CSS configuration is correctly set up. The content paths are comprehensive, and the custom gradients are appropriately defined.
packages/sdk/package.json (1)
1-31: LGTM! Verify the LICENSE file.The package configuration looks correct. The license field update is appropriate if the license details are provided in a separate LICENSE file.
Verification successful
LGTM! Verify the LICENSE file.
The package configuration looks correct. The license field update is appropriate if the license details are provided in a separate LICENSE file.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of the LICENSE file. # Test: Check if the LICENSE file exists. Expect: LICENSE file to be present. if [[ -f "LICENSE" ]]; then echo "LICENSE file is present." else echo "LICENSE file is missing." exit 1 fiLength of output: 77
packages/sdk/examples/next-app/app/server.ts (2)
5-14: Verify the availability offetchin the environment.Ensure that
fetchis available in the server environment where this code will run. If not, consider using a library likenode-fetch.
23-35: Verify error handling ingetSpotifyDataandgetSlackData.Ensure that the
getSpotifyDataandgetSlackDatafunctions properly handle errors and edge cases.packages/sdk/src/index.ts (5)
1-4: LGTM!The type definition for
CreateServerClientOptsis correct.
6-8: LGTM!The type definition for
ConnectTokenCreateOptsis correct.
11-14: LGTM!The type definition for
AccountKeyFieldsis correct.
15-15: LGTM!The type definition for
AccountKeyis correct.
17-19: Verify the implementation of theServerClientclass.Ensure that the
ServerClientclass is correctly implemented.Verification successful
Verified the implementation of the
ServerClientclass.The
ServerClientclass is correctly implemented with necessary methods and proper TypeScript syntax.
- Location:
packages/sdk/src/index.ts- Methods:
constructor,_authorizonHeader,connectTokenCreate,getAccountScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `ServerClient` class. # Test: Search for the `ServerClient` class implementation. Expect: Proper implementation. rg --type ts 'class ServerClient' --files-with-matchesLength of output: 82
Script:
#!/bin/bash # Extract the implementation of the `ServerClient` class from the `packages/sdk/src/index.ts` file. ast-grep --lang typescript --pattern $'class ServerClient { $$$ }' packages/sdk/src/index.tsLength of output: 4052
packages/sdk/src/browser/index.ts (5)
1-4: LGTM!The type definition for
CreateBrowserClientOptsis correct.
8-10: LGTM!The type definition for
StartConnectAppis correct.
16-16: LGTM!The class definition for
ConnectErroris correct.
44-93: Verify error handling and iframe management instartConnect.Ensure that errors are properly handled and the iframe is securely managed.
Verification successful
Verification successful: Error handling and iframe management in
startConnectare properly implemented.
- Error handling is present and correctly invokes
opts.onErroror throws errors.- The iframe is securely managed by setting appropriate attributes, appending it to the document body, and removing it upon receiving a "close" message.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and iframe management in `startConnect`. # Test: Search for error handling and iframe management in the `startConnect` method. Expect: Proper error handling and secure iframe management. rg --type ts 'startConnect' --files-with-matches | xargs rg 'throw|catch|error|iframe'Length of output: 1295
29-31: Verify the implementation of theBrowserClientclass.Ensure that the
BrowserClientclass is correctly implemented.Verification successful
The implementation of the
BrowserClientclass is correct.The
BrowserClientclass is properly implemented with a constructor that initializes necessary properties and a method (startConnect) that handles iframe creation and communication.
- The constructor correctly initializes the
environment,publicKey, and other properties.- The
startConnectmethod includes proper event listeners, URL parameter handling, and error handling.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `BrowserClient` class. # Test: Search for the `BrowserClient` class implementation. Expect: Proper implementation. rg --type ts 'class BrowserClient' --files-with-matchesLength of output: 91
Script:
#!/bin/bash # Extract the implementation of the `BrowserClient` class from the `packages/sdk/src/browser/index.ts` file. # Use `ast-grep` to get the implementation of the `BrowserClient` class. ast-grep --lang typescript --pattern $'class BrowserClient { $$$ }' packages/sdk/src/browser/index.tsLength of output: 4280
packages/sdk/examples/next-app/app/page.tsx (1)
7-14: LGTM!The environment variable check and client creation are correctly implemented.
| export async function getSpotifyData(clientUserId: string) { | ||
| const data = await pd.getAccount({ | ||
| app: "spotify", | ||
| clientUserId, | ||
| }, { | ||
| includeCredentials: true, | ||
| }); | ||
| if (!data) { | ||
| return null; | ||
| } | ||
| const resp = await fetch("https://api.spotify.com/v1/me/top/tracks?limit=1", { | ||
| headers: { | ||
| Authorization: `Bearer ${data.credentials.oauth_access_token}`, | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
| const item = res.items[0]; | ||
| if (!item) { | ||
| return null; | ||
| } | ||
| return { | ||
| name: item.album.name, | ||
| artist: item.album.artists[0].name, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Ensure proper error handling for Spotify API call.
The current implementation of the getSpotifyData function does not include error handling for the API call to Spotify. It's important to handle potential errors such as network issues, invalid responses, or API rate limits to ensure the function behaves reliably.
- Add try-catch blocks around the
fetchcall to catch and handle errors. - Consider logging the errors for debugging purposes.
- Return a meaningful error message or null in case of failure.
Analysis chain
Ensure proper error handling for Spotify API call.
Verify that the API call to Spotify is properly handled and errors are managed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in the Spotify API call.
# Test: Search for error handling in the Spotify API call. Expect: Proper error handling.
rg --type ts 'fetch\("https://api.spotify.com/v1/me/top/tracks' --files-with-matches | xargs rg 'throw|catch|error'
Length of output: 191
| export async function getSlackData(clientUserId: string) { | ||
| const data = await pd.getAccount({ | ||
| app: "slack", | ||
| clientUserId, | ||
| }, { | ||
| includeCredentials: true, | ||
| }); | ||
| if (!data) { | ||
| return null; | ||
| } | ||
| const resp = await fetch("https://slack.com/api/users.profile.get", { | ||
| headers: { | ||
| Authorization: `Bearer ${data.credentials.oauth_access_token}`, | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
| return { | ||
| display_name: res.profile.display_name, | ||
| image_original: res.profile.image_original, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Add error handling for Slack API call.
The getSlackData function does not currently handle errors for the fetch call to the Slack API. Consider adding error handling to manage potential failures and improve robustness.
- File:
packages/sdk/examples/next-app/app/server.ts - Lines: 63-83
Analysis chain
Ensure proper error handling for Slack API call.
Verify that the API call to Slack is properly handled and errors are managed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in the Slack API call.
# Test: Search for error handling in the Slack API call. Expect: Proper error handling.
rg --type ts 'fetch\("https://slack.com/api/users.profile.get' --files-with-matches | xargs rg 'throw|catch|error'
Length of output: 190
| export async function serverConnectTokenCreate(clientUserId: string) { | ||
| return pd.connectTokenCreate({ | ||
| clientName: "Next.js Example App", | ||
| clientUserId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Verify error handling in connectTokenCreate.
The connectTokenCreate method in the ServerClient class lacks proper error handling. Specifically, it does not handle potential errors from the fetch call or the response parsing. Consider adding error handling mechanisms to ensure robustness.
- Location:
packages/sdk/src/index.ts- Lines: 37-65
Analysis chain
Verify error handling in connectTokenCreate.
Ensure that the connectTokenCreate method properly handles errors and edge cases.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in the `connectTokenCreate` method.
# Test: Search for the `connectTokenCreate` method implementation. Expect: Proper error handling.
rg --type ts 'connectTokenCreate' --files-with-matches | xargs rg 'throw|catch|error'
Length of output: 259
Script:
#!/bin/bash
# Description: Locate and inspect the implementation of the `connectTokenCreate` method.
# Find the file containing the implementation of `connectTokenCreate`.
rg --type ts 'function connectTokenCreate' --files-with-matches | xargs rg 'function connectTokenCreate'
# Extract the implementation of `connectTokenCreate` for review.
ast-grep --lang typescript --pattern 'function connectTokenCreate($_) { $$$ }'
Length of output: 187
Script:
#!/bin/bash
# Description: Search for various contexts in which `connectTokenCreate` might be defined or imported.
# Search for `connectTokenCreate` in import statements.
rg --type ts 'import .*connectTokenCreate'
# Search for `connectTokenCreate` as an object property or method.
rg --type ts 'connectTokenCreate:'
# Search for `connectTokenCreate` in class definitions.
rg --type ts 'class .* {.*connectTokenCreate.*}'
Length of output: 256
Script:
#!/bin/bash
# Description: Search for various contexts in which `connectTokenCreate` might be defined or imported.
# Search for `connectTokenCreate` in import statements.
rg --type ts 'import .*connectTokenCreate'
# Search for `connectTokenCreate` as an object property or method.
rg --type ts 'connectTokenCreate:'
# Search for `connectTokenCreate` in class definitions, correcting the regex error.
rg --type ts 'class .* {.*connectTokenCreate.*}'
# Search for `connectTokenCreate` as an arrow function.
rg --type ts 'const connectTokenCreate ='
# Search for `connectTokenCreate` within object literals.
rg --type ts '{.*connectTokenCreate.*}'
Length of output: 442
Script:
#!/bin/bash
# Description: Search for various contexts in which `connectTokenCreate` might be defined or imported.
# Search for `connectTokenCreate` in import statements.
rg --type ts 'import .*connectTokenCreate'
# Search for `connectTokenCreate` as an object property or method.
rg --type ts 'connectTokenCreate:'
# Corrected regex: Search for `connectTokenCreate` in class definitions.
rg --type ts 'class [^{]*{[^}]*connectTokenCreate[^}]*}'
# Search for `connectTokenCreate` as an arrow function.
rg --type ts 'const connectTokenCreate ='
# Corrected regex: Search for `connectTokenCreate` within object literals.
rg --type ts '\{[^}]*connectTokenCreate[^}]*\}'
# Search for the `pd` object to trace the `connectTokenCreate` method.
rg --type ts 'pd\.connectTokenCreate'
Length of output: 486
Script:
#!/bin/bash
# Description: Locate the definition or import of the `pd` object to trace the `connectTokenCreate` method.
# Search for the definition of `pd`.
rg --type ts 'const pd ='
# Search for the import of `pd`.
rg --type ts 'import .*pd'
Length of output: 900
Script:
#!/bin/bash
# Description: Locate and inspect the `createClient` function to trace the `connectTokenCreate` method.
# Find the file containing the implementation of `createClient`.
rg --type ts 'function createClient' --files-with-matches | xargs rg 'function createClient'
# Extract the implementation of `createClient` for review.
ast-grep --lang typescript --pattern 'function createClient($_) { $$$ }'
Length of output: 756
Script:
#!/bin/bash
# Description: Locate and inspect the `ServerClient` class and its `connectTokenCreate` method.
# Find the file containing the implementation of `ServerClient`.
rg --type ts 'class ServerClient' --files-with-matches | xargs rg 'class ServerClient'
# Extract the implementation of `ServerClient` for review.
ast-grep --lang typescript --pattern 'class ServerClient { $$$ }'
# Search for the `connectTokenCreate` method within `ServerClient`.
ast-grep --lang typescript --pattern 'class ServerClient { $$$ connectTokenCreate($_) { $$$ } $$$ }'
Length of output: 4379
packages/sdk/src/index.ts
Outdated
| // XXX move to REST API endpoint | ||
| async connectTokenCreate(opts: ConnectTokenCreateOpts): Promise<string> { | ||
| const resp = await fetch(`${API_BASE_URL}/graphql`, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| query: `mutation sdkConnectTokenCreate( | ||
| $secretKey: String! | ||
| $clientName: String! | ||
| $clientUserId: String! | ||
| ) { | ||
| connectTokenCreate( | ||
| secretKey: $secretKey | ||
| clientName: $clientName | ||
| clientUserId: $clientUserId | ||
| ) { | ||
| token | ||
| } | ||
| }`, | ||
| variables: { | ||
| secretKey: this.secretKey, | ||
| clientUserId: opts.clientUserId, | ||
| }, | ||
| }), | ||
| }); | ||
| const res = await resp.json(); | ||
| // XXX expose error here | ||
| return res.data?.connectTokenCreate?.token; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Implement error handling in connectTokenCreate method.
The connectTokenCreate method lacks proper error handling. Ensure that errors are caught and handled appropriately. The GraphQL endpoint appears to be correctly set.
- Location:
packages/sdk/src/index.ts, line 66 (// XXX expose error here)
Analysis chain
Verify error handling and GraphQL endpoint for connectTokenCreate.
Ensure that errors are properly handled and the GraphQL endpoint is correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling and GraphQL endpoint for `connectTokenCreate`.
# Test: Search for error handling and GraphQL endpoint in the `connectTokenCreate` method. Expect: Proper error handling and correct endpoint.
rg --type ts 'connectTokenCreate' --files-with-matches | xargs rg 'throw|catch|error|graphql'
Length of output: 351
packages/sdk/src/index.ts
Outdated
| async getAccount(key: AccountKey, opts?: { includeCredentials?: boolean; }) { | ||
| let url: string; | ||
| let id: string | undefined; | ||
| if (typeof key === "string") { | ||
| id = key; | ||
| url = `${API_BASE_URL}/v1/accounts/${id}`; | ||
| } else { | ||
| url = `${API_BASE_URL}/v1/accounts?app=${key.app}&client_user_id=${key.clientUserId}`; | ||
| } | ||
| if (opts?.includeCredentials) { | ||
| url += `${id | ||
| ? "?" | ||
| : "&"}include_credentials=1`; | ||
| } | ||
| const resp = await fetch(url, { | ||
| headers: { | ||
| Authorization: this._authorizonHeader(), | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
| if (id) { | ||
| return res.data; | ||
| } | ||
| return res.data.accounts[0]; | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
Add error handling in getAccount method.
The getAccount method currently lacks explicit error handling (try/catch/throw). It is recommended to add proper error handling to ensure robustness.
- File:
packages/sdk/src/index.ts - Lines: 68-92
Analysis chain
Verify error handling and API endpoint for getAccount.
Ensure that errors are properly handled and the API endpoint is correct.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling and API endpoint for `getAccount`.
# Test: Search for error handling and API endpoint in the `getAccount` method. Expect: Proper error handling and correct endpoint.
rg --type ts 'getAccount' --files-with-matches | xargs rg 'throw|catch|error|api'
Length of output: 3283
Script:
#!/bin/bash
# Description: Verify error handling and API endpoint for `getAccount`.
# Test: Check if `getAccount` method in `packages/sdk/src/index.ts` has any error handling mechanisms.
rg --type ts 'getAccount' packages/sdk/src/index.ts | xargs rg 'try|catch|throw'
# Test: Verify the correctness of the API endpoint used in `getAccount` method.
rg --type ts 'API_BASE_URL' packages/sdk/src/index.ts
Length of output: 837
Better error handling, improvements to example app and SDK.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/src/browser/index.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- packages/sdk/examples/next-app/app/page.tsx
- packages/sdk/examples/next-app/app/server.ts
- packages/sdk/src/index.ts
Additional context used
Biome
packages/sdk/src/browser/index.ts
[error] 13-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Additional comments not posted (6)
packages/sdk/src/browser/index.ts (6)
1-5: LGTM!The type declaration
CreateBrowserClientOptsis well-defined.
7-7: LGTM!The type declaration
AppIdis well-defined.
9-11: LGTM!The type declaration
StartConnectAppis well-defined.
19-24: LGTM!The type declaration
StartConnectOptsis well-defined.
26-28: LGTM!The function
createClientis well-defined.
17-17: LGTM!The class
ConnectErroris well-defined.
packages/sdk/src/browser/index.ts
Outdated
| this.iframeURL = `${this.baseURL}/_static/connect.html`; | ||
| } | ||
|
|
||
| startConnect(opts: StartConnectOpts) { |
There was a problem hiding this comment.
Add error handling for missing token and app.
Ensure that the token and app properties are provided in the StartConnectOpts object.
+ if (!opts.token) {
+ throw new ConnectError("Token is required.");
+ }
+ if (!opts.app) {
+ throw new ConnectError("App is required.");
+ }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.
| startConnect(opts: StartConnectOpts) { | |
| startConnect(opts: StartConnectOpts) { | |
| if (!opts.token) { | |
| throw new ConnectError("Token is required."); | |
| } | |
| if (!opts.app) { | |
| throw new ConnectError("App is required."); | |
| } |
packages/sdk/src/browser/index.ts
Outdated
| const onMessage = (e: MessageEvent) => { | ||
| if (e.origin !== this.baseURL) { | ||
| console.warn("Received message from unauthorized origin:", e.origin); | ||
| return; | ||
| } | ||
| if (e.data?.type === "close") { | ||
| this.iframe?.remove(); | ||
| window.removeEventListener("message", onMessage); | ||
| } else if (e.data?.type === "success") { | ||
| opts.onSuccess?.({ | ||
| id: e.data.authProvisionId, | ||
| }); | ||
| } else if (e.data?.type === "error") { | ||
| opts.onError?.(new ConnectError(e.data.error)); | ||
| } |
There was a problem hiding this comment.
Move onMessage function outside startConnect for better readability.
Consider moving the onMessage function outside the startConnect method to improve readability and maintainability.
- startConnect(opts: StartConnectOpts) {
- console.log("startConnect", opts);
- const onMessage = (e: MessageEvent) => {
+ private onMessage = (e: MessageEvent) => {
+ if (e.origin !== this.baseURL) {
+ console.warn("Received message from unauthorized origin:", e.origin);
+ return;
+ }
+ if (e.data?.type === "close") {
+ this.iframe?.remove();
+ window.removeEventListener("message", this.onMessage);
+ } else if (e.data?.type === "success") {
+ this.currentOpts?.onSuccess?.({
+ id: e.data.authProvisionId,
+ });
+ } else if (e.data?.type === "error") {
+ this.currentOpts?.onError?.(new ConnectError(e.data.error));
+ }
+ };
+ startConnect(opts: StartConnectOpts) {
+ console.log("startConnect", opts);
+ this.currentOpts = opts;
+ window.addEventListener("message", this.onMessage);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 onMessage = (e: MessageEvent) => { | |
| if (e.origin !== this.baseURL) { | |
| console.warn("Received message from unauthorized origin:", e.origin); | |
| return; | |
| } | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", onMessage); | |
| } else if (e.data?.type === "success") { | |
| opts.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| opts.onError?.(new ConnectError(e.data.error)); | |
| } | |
| private onMessage = (e: MessageEvent) => { | |
| if (e.origin !== this.baseURL) { | |
| console.warn("Received message from unauthorized origin:", e.origin); | |
| return; | |
| } | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", this.onMessage); | |
| } else if (e.data?.type === "success") { | |
| this.currentOpts?.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| this.currentOpts?.onError?.(new ConnectError(e.data.error)); | |
| } | |
| }; | |
| startConnect(opts: StartConnectOpts) { | |
| console.log("startConnect", opts); | |
| this.currentOpts = opts; | |
| window.addEventListener("message", this.onMessage); |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/src/browser/index.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/examples/next-app/app/page.tsx
- packages/sdk/src/index.ts
Additional context used
Biome
packages/sdk/src/browser/index.ts
[error] 13-15: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Additional comments not posted (1)
packages/sdk/src/browser/index.ts (1)
45-45: Add error handling for missing token and app.Ensure that the
tokenandappproperties are provided in theStartConnectOptsobject.+ if (!opts.token) { + throw new ConnectError("Token is required."); + } + if (!opts.app) { + throw new ConnectError("App is required."); + }
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/examples/next-app/app/page.tsx
Additional comments not posted (1)
packages/sdk/examples/next-app/app/server.ts (1)
1-20: Environment variable checks and client creation are correctly implemented.The code properly checks for the presence of required environment variables and creates a Pipedream client using the secret key and API host.
| export async function serverConnectTokenCreate(clientUserId: string) { | ||
| return pd.connectTokenCreate({ | ||
| clientUserId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add error handling to serverConnectTokenCreate.
The function does not handle potential errors from the pd.connectTokenCreate call. Consider adding error handling to ensure robustness.
export async function serverConnectTokenCreate(clientUserId: string) {
- return pd.connectTokenCreate({
- clientUserId,
- });
+ try {
+ return await pd.connectTokenCreate({ clientUserId });
+ } catch (error) {
+ console.error("Error creating connection token:", error);
+ throw new Error("Failed to create connection 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.
| export async function serverConnectTokenCreate(clientUserId: string) { | |
| return pd.connectTokenCreate({ | |
| clientUserId, | |
| }); | |
| } | |
| export async function serverConnectTokenCreate(clientUserId: string) { | |
| try { | |
| return await pd.connectTokenCreate({ clientUserId }); | |
| } catch (error) { | |
| console.error("Error creating connection token:", error); | |
| throw new Error("Failed to create connection token"); | |
| } | |
| } |
| export async function getGithubData(clientUserId: string) { | ||
| if (!NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID) { | ||
| throw new Error("NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID not set in environment"); | ||
| } | ||
| const data = await pd.getAccount({ | ||
| app: NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID, | ||
| clientUserId, | ||
| }, { | ||
| includeCredentials: true, | ||
| }); | ||
| if (!data) { | ||
| return null; | ||
| } | ||
| const resp = await fetch("https://api.github.com/user", { | ||
| headers: { | ||
| Authorization: `Bearer ${data.credentials.oauth_access_token}`, | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
| return { | ||
| login: res.login, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add error handling to getGithubData and improve null check.
The function does not handle potential errors from the pd.getAccount and fetch calls. Additionally, the null check for data can be improved.
export async function getGithubData(clientUserId: string) {
if (!NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID) {
throw new Error("NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID not set in environment");
}
- const data = await pd.getAccount({
- app: NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID,
- clientUserId,
- }, {
- includeCredentials: true,
- });
- if (!data) {
- return null;
- }
+ let data;
+ try {
+ data = await pd.getAccount({
+ app: NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID,
+ clientUserId,
+ }, {
+ includeCredentials: true,
+ });
+ } catch (error) {
+ console.error("Error retrieving account data:", error);
+ throw new Error("Failed to retrieve account data");
+ }
+ if (!data || !data.credentials || !data.credentials.oauth_access_token) {
+ return null;
+ }
const resp = await fetch("https://api.github.com/user", {
headers: {
Authorization: `Bearer ${data.credentials.oauth_access_token}`,
},
});
const res = await resp.json();
return {
login: res.login,
};
}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.
| export async function getGithubData(clientUserId: string) { | |
| if (!NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID) { | |
| throw new Error("NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID not set in environment"); | |
| } | |
| const data = await pd.getAccount({ | |
| app: NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID, | |
| clientUserId, | |
| }, { | |
| includeCredentials: true, | |
| }); | |
| if (!data) { | |
| return null; | |
| } | |
| const resp = await fetch("https://api.github.com/user", { | |
| headers: { | |
| Authorization: `Bearer ${data.credentials.oauth_access_token}`, | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| return { | |
| login: res.login, | |
| }; | |
| } | |
| export async function getGithubData(clientUserId: string) { | |
| if (!NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID) { | |
| throw new Error("NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID not set in environment"); | |
| } | |
| let data; | |
| try { | |
| data = await pd.getAccount({ | |
| app: NEXT_PUBLIC_PIPEDREAM_TEST_APP_ID, | |
| clientUserId, | |
| }, { | |
| includeCredentials: true, | |
| }); | |
| } catch (error) { | |
| console.error("Error retrieving account data:", error); | |
| throw new Error("Failed to retrieve account data"); | |
| } | |
| if (!data || !data.credentials || !data.credentials.oauth_access_token) { | |
| return null; | |
| } | |
| const resp = await fetch("https://api.github.com/user", { | |
| headers: { | |
| Authorization: `Bearer ${data.credentials.oauth_access_token}`, | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| return { | |
| login: res.login, | |
| }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/examples/next-app/.tool-versions (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk/examples/next-app/.tool-versions
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/examples/next-app/app/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- packages/sdk/examples/next-app/.vscode/launch.json (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk/examples/next-app/.vscode/launch.json
Files skipped from review as they are similar to previous changes (3)
- packages/sdk/examples/next-app/app/page.tsx
- packages/sdk/examples/next-app/app/server.ts
- packages/sdk/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/examples/next-app/app/page.tsx
- packages/sdk/examples/next-app/app/server.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/examples/next-app/app/server.ts
- packages/sdk/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
packages/sdk/examples/next-app/app/server.ts (1)
3-14: Ensure all required environment variables are checked.The current implementation checks for
PIPEDREAM_PROJECT_SECRET_KEYandNEXT_PUBLIC_PIPEDREAM_APP_SLUG. However, the following environment variables are also used but not checked:
PIPEDREAM_API_HOSTNEXT_PUBLIC_PIPEDREAM_PROJECT_PUBLIC_KEYPlease add checks for these variables to avoid runtime errors.
Analysis chain
Ensure all required environment variables are checked.
The current implementation checks for
PIPEDREAM_PROJECT_SECRET_KEYandNEXT_PUBLIC_PIPEDREAM_APP_SLUG. Ensure all other critical environment variables are also checked to avoid runtime errors.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all required environment variables are checked. # Test: Search for environment variable usage. Expect: All critical variables are checked. rg --type js 'process.env' packages/sdk/examples/next-app/app/server.tsLength of output: 198
packages/sdk/src/index.ts (1)
1-19: Ensure type definitions are comprehensive.The type definitions seem accurate. However, consider adding comments to describe each type for better readability and maintainability.
type CreateServerClientOpts = { environment?: string; // The environment in which the client is running secretKey: string; // The secret key for authentication apiHost?: string; // The API host URL //projectId?: string; // The project ID (currently commented out) publicKey: string; // The public key for authentication }; type ConnectTokenCreateOpts = { clientUserId: string; // The user ID for the client }; type AccountId = string; // The account ID as a string type AccountKeyFields = { clientUserId: string; // The user ID for the client app: string; // The app identifier }; type AccountKey = AccountId | AccountKeyFields; // The account key can be either an ID or key fields
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Additional comments not posted (3)
packages/sdk/examples/next-app/app/server.ts (1)
22-26: Add error handling toserverConnectTokenCreate.The function does not handle potential errors from the
pd.connectTokenCreatecall. Consider adding error handling to ensure robustness.export async function serverConnectTokenCreate(clientUserId: string) { - return pd.connectTokenCreate({ - clientUserId, - }); + try { + return await pd.connectTokenCreate({ clientUserId }); + } catch (error) { + console.error("Error creating connection token:", error); + throw new Error("Failed to create connection token"); + } }Likely invalid or redundant comment.
packages/sdk/src/index.ts (2)
20-22: LGTM!The
createClientfunction correctly initializes a new instance ofServerClient.
24-37: LGTM!The
ServerClientclass constructor correctly initializes the instance with the provided options.
| export async function getAppsData(clientUserId: string) { | ||
| const [ | ||
| github, | ||
| ] = await Promise.all([ | ||
| getGithubData(clientUserId), | ||
| ]); | ||
| return { | ||
| github, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add error handling to getAppsData.
The function does not handle potential errors from the Promise.all call. Consider adding error handling to ensure robustness.
export async function getAppsData(clientUserId: string) {
- const [ github ] = await Promise.all([ getGithubData(clientUserId) ]);
- return { github };
+ try {
+ const [ github ] = await Promise.all([ getGithubData(clientUserId) ]);
+ return { github };
+ } catch (error) {
+ console.error("Error retrieving apps data:", error);
+ throw new Error("Failed to retrieve apps data");
+ }
}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.
| export async function getAppsData(clientUserId: string) { | |
| const [ | |
| github, | |
| ] = await Promise.all([ | |
| getGithubData(clientUserId), | |
| ]); | |
| return { | |
| github, | |
| }; | |
| } | |
| export async function getAppsData(clientUserId: string) { | |
| try { | |
| const [ github ] = await Promise.all([ getGithubData(clientUserId) ]); | |
| return { github }; | |
| } catch (error) { | |
| console.error("Error retrieving apps data:", error); | |
| throw new Error("Failed to retrieve apps data"); | |
| } | |
| } |
| export async function getGithubData(clientUserId: string) { | ||
| if (!NEXT_PUBLIC_PIPEDREAM_APP_SLUG) { | ||
| throw new Error("NEXT_PUBLIC_PIPEDREAM_APP_SLUG not set in environment"); | ||
| } | ||
| const data = await pd.getAccount({ | ||
| app: NEXT_PUBLIC_PIPEDREAM_APP_SLUG, | ||
| clientUserId, | ||
| }, { | ||
| includeCredentials: true, | ||
| }); | ||
| if (!data?.accounts.length) { | ||
| return null; | ||
| } | ||
| const account = data.accounts[data.accounts.length - 1] | ||
| const resp = await fetch("https://api.github.com/user", { | ||
| headers: { | ||
| Authorization: `Bearer ${account.credentials.oauth_access_token}`, | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
|
|
||
| return res; |
There was a problem hiding this comment.
Add error handling to getGithubData and improve null check.
The function does not handle potential errors from the pd.getAccount and fetch calls. Additionally, the null check for data can be improved.
export async function getGithubData(clientUserId: string) {
if (!NEXT_PUBLIC_PIPEDREAM_APP_SLUG) {
throw new Error("NEXT_PUBLIC_PIPEDREAM_APP_SLUG not set in environment");
}
- const data = await pd.getAccount({
- app: NEXT_PUBLIC_PIPEDREAM_APP_SLUG,
- clientUserId,
- }, {
- includeCredentials: true,
- });
- if (!data?.accounts.length) {
- return null;
- }
+ let data;
+ try {
+ data = await pd.getAccount({
+ app: NEXT_PUBLIC_PIPEDREAM_APP_SLUG,
+ clientUserId,
+ }, {
+ includeCredentials: true,
+ });
+ } catch (error) {
+ console.error("Error retrieving account data:", error);
+ throw new Error("Failed to retrieve account data");
+ }
+ if (!data || !data.accounts.length) {
+ return null;
+ }
const account = data.accounts[data.accounts.length - 1]
const resp = await fetch("https://api.github.com/user", {
headers: {
Authorization: `Bearer ${account.credentials.oauth_access_token}`,
},
});
const res = await resp.json();
return res;
}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.
| export async function getGithubData(clientUserId: string) { | |
| if (!NEXT_PUBLIC_PIPEDREAM_APP_SLUG) { | |
| throw new Error("NEXT_PUBLIC_PIPEDREAM_APP_SLUG not set in environment"); | |
| } | |
| const data = await pd.getAccount({ | |
| app: NEXT_PUBLIC_PIPEDREAM_APP_SLUG, | |
| clientUserId, | |
| }, { | |
| includeCredentials: true, | |
| }); | |
| if (!data?.accounts.length) { | |
| return null; | |
| } | |
| const account = data.accounts[data.accounts.length - 1] | |
| const resp = await fetch("https://api.github.com/user", { | |
| headers: { | |
| Authorization: `Bearer ${account.credentials.oauth_access_token}`, | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| return res; | |
| export async function getGithubData(clientUserId: string) { | |
| if (!NEXT_PUBLIC_PIPEDREAM_APP_SLUG) { | |
| throw new Error("NEXT_PUBLIC_PIPEDREAM_APP_SLUG not set in environment"); | |
| } | |
| let data; | |
| try { | |
| data = await pd.getAccount({ | |
| app: NEXT_PUBLIC_PIPEDREAM_APP_SLUG, | |
| clientUserId, | |
| }, { | |
| includeCredentials: true, | |
| }); | |
| } catch (error) { | |
| console.error("Error retrieving account data:", error); | |
| throw new Error("Failed to retrieve account data"); | |
| } | |
| if (!data || !data.accounts.length) { | |
| return null; | |
| } | |
| const account = data.accounts[data.accounts.length - 1] | |
| const resp = await fetch("https://api.github.com/user", { | |
| headers: { | |
| Authorization: `Bearer ${account.credentials.oauth_access_token}`, | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| return res; | |
| } |
packages/sdk/src/index.ts
Outdated
| async getAccount(key: AccountKey, opts?: { includeCredentials?: boolean; }) { | ||
| let url: string; | ||
| let id: string | undefined; | ||
| const baseAccountURL = `${this.baseURL}/v1/accounts`; | ||
| if (typeof key === "string") { | ||
| id = key; | ||
| url = `${baseAccountURL}/${id}`; | ||
| } else { | ||
| url = `${baseAccountURL}?app=${key.app}&limit=100&external_id=${key.clientUserId}`; | ||
| } | ||
| if (opts?.includeCredentials) { | ||
| url += `${id | ||
| ? "?" | ||
| : "&"}include_credentials=1`; | ||
| } | ||
| const resp = await fetch(url, { | ||
| headers: { | ||
| Authorization: this._authorizonHeader(), | ||
| }, | ||
| }); | ||
| const res = await resp.json(); | ||
| console.log('res :>> ', res); | ||
| const { | ||
| data, error, | ||
| } = res; | ||
| if (error) { | ||
| if (error === "record not found") { | ||
| return null; | ||
| } | ||
| throw new Error(error); | ||
| } | ||
| return data; |
There was a problem hiding this comment.
Add error handling to getAccount method.
The getAccount method currently lacks explicit error handling (try/catch/throw). It is recommended to add proper error handling to ensure robustness.
async getAccount(key: AccountKey, opts?: { includeCredentials?: boolean; }) {
let url: string;
let id: string | undefined;
const baseAccountURL = `${this.baseURL}/v1/accounts`;
if (typeof key === "string") {
id = key;
url = `${baseAccountURL}/${id}`;
} else {
url = `${baseAccountURL}?app=${key.app}&limit=100&external_id=${key.clientUserId}`;
}
if (opts?.includeCredentials) {
url += `${id
? "?"
: "&"}include_credentials=1`;
}
try {
const resp = await fetch(url, {
headers: {
Authorization: this._authorizonHeader(),
},
});
const res = await resp.json();
console.log('res :>> ', res);
const {
data, error,
} = res;
if (error) {
if (error === "record not found") {
return null;
}
throw new Error(error);
}
return data;
} catch (error) {
console.error("Error retrieving account data:", error);
throw new Error("Failed to retrieve account data");
}
}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.
| async getAccount(key: AccountKey, opts?: { includeCredentials?: boolean; }) { | |
| let url: string; | |
| let id: string | undefined; | |
| const baseAccountURL = `${this.baseURL}/v1/accounts`; | |
| if (typeof key === "string") { | |
| id = key; | |
| url = `${baseAccountURL}/${id}`; | |
| } else { | |
| url = `${baseAccountURL}?app=${key.app}&limit=100&external_id=${key.clientUserId}`; | |
| } | |
| if (opts?.includeCredentials) { | |
| url += `${id | |
| ? "?" | |
| : "&"}include_credentials=1`; | |
| } | |
| const resp = await fetch(url, { | |
| headers: { | |
| Authorization: this._authorizonHeader(), | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| console.log('res :>> ', res); | |
| const { | |
| data, error, | |
| } = res; | |
| if (error) { | |
| if (error === "record not found") { | |
| return null; | |
| } | |
| throw new Error(error); | |
| } | |
| return data; | |
| async getAccount(key: AccountKey, opts?: { includeCredentials?: boolean; }) { | |
| let url: string; | |
| let id: string | undefined; | |
| const baseAccountURL = `${this.baseURL}/v1/accounts`; | |
| if (typeof key === "string") { | |
| id = key; | |
| url = `${baseAccountURL}/${id}`; | |
| } else { | |
| url = `${baseAccountURL}?app=${key.app}&limit=100&external_id=${key.clientUserId}`; | |
| } | |
| if (opts?.includeCredentials) { | |
| url += `${id | |
| ? "?" | |
| : "&"}include_credentials=1`; | |
| } | |
| try { | |
| const resp = await fetch(url, { | |
| headers: { | |
| Authorization: this._authorizonHeader(), | |
| }, | |
| }); | |
| const res = await resp.json(); | |
| console.log('res :>> ', res); | |
| const { | |
| data, error, | |
| } = res; | |
| if (error) { | |
| if (error === "record not found") { | |
| return null; | |
| } | |
| throw new Error(error); | |
| } | |
| return data; | |
| } catch (error) { | |
| console.error("Error retrieving account data:", error); | |
| throw new Error("Failed to retrieve account data"); | |
| } | |
| } |
packages/sdk/src/index.ts
Outdated
| // XXX move to REST API endpoint | ||
| async connectTokenCreate(opts: ConnectTokenCreateOpts): Promise<string> { | ||
| const resp = await fetch(`${this.baseURL}/graphql`, { | ||
| method: "POST", | ||
| headers: { | ||
| "content-type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| query: `mutation sdkConnectTokenCreate( | ||
| $publicKey: String! | ||
| $secretKey: String! | ||
| $externalId: String! | ||
| $oauthAppId: String! | ||
| ) { | ||
| connectTokenCreate( | ||
| publicKey: $publicKey | ||
| secretKey: $secretKey | ||
| externalId: $externalId | ||
| oauthAppId: $oauthAppId | ||
| ) { | ||
| token | ||
| } | ||
| }`, | ||
| variables: { | ||
| publicKey: this.publicKey, | ||
| secretKey: this.secretKey, | ||
| externalId: opts.clientUserId, | ||
| oauthAppId: "oa_aLZiMJ", | ||
| }, | ||
| }), | ||
| }); | ||
| const res = await resp.json(); | ||
| // XXX expose error here | ||
| console.log("connect token create result") | ||
| console.log(res) | ||
| return res.data?.connectTokenCreate?.token; | ||
| } |
There was a problem hiding this comment.
Implement error handling in connectTokenCreate method.
The connectTokenCreate method lacks proper error handling. Ensure that errors are caught and handled appropriately.
async connectTokenCreate(opts: ConnectTokenCreateOpts): Promise<string> {
try {
const resp = await fetch(`${this.baseURL}/graphql`, {
method: "POST",
headers: {
"content-type": "application/json",
},
body: JSON.stringify({
query: `mutation sdkConnectTokenCreate(
$publicKey: String!
$secretKey: String!
$externalId: String!
$oauthAppId: String!
) {
connectTokenCreate(
publicKey: $publicKey
secretKey: $secretKey
externalId: $externalId
oauthAppId: $oauthAppId
) {
token
}
}`,
variables: {
publicKey: this.publicKey,
secretKey: this.secretKey,
externalId: opts.clientUserId,
oauthAppId: "oa_aLZiMJ",
},
}),
});
const res = await resp.json();
if (!res.data || !res.data.connectTokenCreate) {
throw new Error("Failed to create connection token");
}
return res.data.connectTokenCreate.token;
} catch (error) {
console.error("Error creating connection token:", error);
throw new Error("Failed to create connection 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.
| // XXX move to REST API endpoint | |
| async connectTokenCreate(opts: ConnectTokenCreateOpts): Promise<string> { | |
| const resp = await fetch(`${this.baseURL}/graphql`, { | |
| method: "POST", | |
| headers: { | |
| "content-type": "application/json", | |
| }, | |
| body: JSON.stringify({ | |
| query: `mutation sdkConnectTokenCreate( | |
| $publicKey: String! | |
| $secretKey: String! | |
| $externalId: String! | |
| $oauthAppId: String! | |
| ) { | |
| connectTokenCreate( | |
| publicKey: $publicKey | |
| secretKey: $secretKey | |
| externalId: $externalId | |
| oauthAppId: $oauthAppId | |
| ) { | |
| token | |
| } | |
| }`, | |
| variables: { | |
| publicKey: this.publicKey, | |
| secretKey: this.secretKey, | |
| externalId: opts.clientUserId, | |
| oauthAppId: "oa_aLZiMJ", | |
| }, | |
| }), | |
| }); | |
| const res = await resp.json(); | |
| // XXX expose error here | |
| console.log("connect token create result") | |
| console.log(res) | |
| return res.data?.connectTokenCreate?.token; | |
| } | |
| async connectTokenCreate(opts: ConnectTokenCreateOpts): Promise<string> { | |
| try { | |
| const resp = await fetch(`${this.baseURL}/graphql`, { | |
| method: "POST", | |
| headers: { | |
| "content-type": "application/json", | |
| }, | |
| body: JSON.stringify({ | |
| query: `mutation sdkConnectTokenCreate( | |
| $publicKey: String! | |
| $secretKey: String! | |
| $externalId: String! | |
| $oauthAppId: String! | |
| ) { | |
| connectTokenCreate( | |
| publicKey: $publicKey | |
| secretKey: $secretKey | |
| externalId: $externalId | |
| oauthAppId: $oauthAppId | |
| ) { | |
| token | |
| } | |
| }`, | |
| variables: { | |
| publicKey: this.publicKey, | |
| secretKey: this.secretKey, | |
| externalId: opts.clientUserId, | |
| oauthAppId: "oa_aLZiMJ", | |
| }, | |
| }), | |
| }); | |
| const res = await resp.json(); | |
| if (!res.data || !res.data.connectTokenCreate) { | |
| throw new Error("Failed to create connection token"); | |
| } | |
| return res.data.connectTokenCreate.token; | |
| } catch (error) { | |
| console.error("Error creating connection token:", error); | |
| throw new Error("Failed to create connection token"); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/_1crm/_1crm.app.mjs (1 hunks)
- components/_8x8_connect/_8x8_connect.app.mjs (1 hunks)
- components/cloudpresenter/cloudpresenter.app.mjs (1 hunks)
- components/d7_darwin/d7_darwin.app.mjs (1 hunks)
- components/dart/dart.app.mjs (1 hunks)
- components/jooto/jooto.app.mjs (1 hunks)
- components/lambdatest/lambdatest.app.mjs (1 hunks)
- components/looker_studio/looker_studio.app.mjs (1 hunks)
Files skipped from review due to trivial changes (8)
- components/_1crm/_1crm.app.mjs
- components/_8x8_connect/_8x8_connect.app.mjs
- components/cloudpresenter/cloudpresenter.app.mjs
- components/d7_darwin/d7_darwin.app.mjs
- components/dart/dart.app.mjs
- components/jooto/jooto.app.mjs
- components/lambdatest/lambdatest.app.mjs
- components/looker_studio/looker_studio.app.mjs
* Switch to the REST API to create a Connect token * Rename "client user ID" to "external ID" * Moved some env vars to the server * Use public key instead of project ID * Switch from app's name slug to ID
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (1)
packages/sdk/src/browser/index.ts (1)
12-14: Replace placeholder with an explicit type definition.Avoid using
{}as a type. Explicitly define the shape ofConnectResult.- type ConnectResult = { - // TODO - }; + type ConnectResult = { + id: string; + success: boolean; + data?: any; + error?: string; + };Tools
Biome
[error] 12-14: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/sdk/examples/next-app/.env.example (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/src/browser/index.ts (1 hunks)
- packages/sdk/src/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk/examples/next-app/.env.example
Files skipped from review as they are similar to previous changes (2)
- packages/sdk/examples/next-app/app/server.ts
- packages/sdk/src/index.ts
Additional context used
Biome
packages/sdk/src/browser/index.ts
[error] 12-14: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
Additional comments not posted (7)
packages/sdk/src/browser/index.ts (3)
16-16: LGTM!The class
ConnectErroris correctly defined.
25-27: LGTM!The function
createClientis correctly defined.
29-40: LGTM!The constructor of the class
BrowserClientis correctly defined.packages/sdk/examples/next-app/app/page.tsx (4)
10-11: LGTM!The environment variable check is correctly implemented.
13-19: LGTM!The state management hooks are correctly implemented.
37-43: LGTM!The
signInandsignOutfunctions are correctly implemented.
64-108: LGTM!The JSX structure is correctly implemented.
packages/sdk/src/browser/index.ts
Outdated
| const iframe = document.createElement("iframe"); | ||
| iframe.id = `pipedream-connect-iframe-${this.iframeId++}`; | ||
| iframe.title = "Pipedream Connect"; | ||
| iframe.src = `${this.iframeURL}?${qp.toString()}`; | ||
| iframe.setAttribute( | ||
| "style", | ||
| "position:fixed;inset:0;z-index:2147483647;border:0;display:block;overflow:hidden auto", | ||
| ); | ||
| iframe.setAttribute("height", "100%"); | ||
| iframe.setAttribute("width", "100%"); | ||
| document.body.appendChild(iframe); | ||
| this.iframe = iframe; |
There was a problem hiding this comment.
Use a more readable style definition for the iframe.
Instead of setting the style as a single string, consider using an object to define the styles for better readability.
- iframe.setAttribute(
- "style",
- "position:fixed;inset:0;z-index:2147483647;border:0;display:block;overflow:hidden auto",
- );
+ Object.assign(iframe.style, {
+ position: "fixed",
+ inset: "0",
+ zIndex: "2147483647",
+ border: "0",
+ display: "block",
+ overflow: "hidden auto"
+ });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 iframe = document.createElement("iframe"); | |
| iframe.id = `pipedream-connect-iframe-${this.iframeId++}`; | |
| iframe.title = "Pipedream Connect"; | |
| iframe.src = `${this.iframeURL}?${qp.toString()}`; | |
| iframe.setAttribute( | |
| "style", | |
| "position:fixed;inset:0;z-index:2147483647;border:0;display:block;overflow:hidden auto", | |
| ); | |
| iframe.setAttribute("height", "100%"); | |
| iframe.setAttribute("width", "100%"); | |
| document.body.appendChild(iframe); | |
| this.iframe = iframe; | |
| const iframe = document.createElement("iframe"); | |
| iframe.id = `pipedream-connect-iframe-${this.iframeId++}`; | |
| iframe.title = "Pipedream Connect"; | |
| iframe.src = `${this.iframeURL}?${qp.toString()}`; | |
| Object.assign(iframe.style, { | |
| position: "fixed", | |
| inset: "0", | |
| zIndex: "2147483647", | |
| border: "0", | |
| display: "block", | |
| overflow: "hidden auto" | |
| }); | |
| iframe.setAttribute("height", "100%"); | |
| iframe.setAttribute("width", "100%"); | |
| document.body.appendChild(iframe); | |
| this.iframe = iframe; |
packages/sdk/src/browser/index.ts
Outdated
| startConnect(opts: StartConnectOpts) { | ||
| const onMessage = (e: MessageEvent) => { | ||
| if (e.data?.type === "close") { | ||
| this.iframe?.remove(); | ||
| window.removeEventListener("message", onMessage); | ||
| } else if (e.data?.type === "success") { | ||
| opts.onSuccess?.({ | ||
| id: e.data.authProvisionId, | ||
| }); | ||
| } else if (e.data?.type === "error") { | ||
| opts.onError?.(new ConnectError(e.data.error)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling for missing token and app.
Ensure that the token and app properties are provided in the StartConnectOpts object.
+ if (!opts.token) {
+ throw new ConnectError("Token is required.");
+ }
+ if (!opts.app) {
+ throw new ConnectError("App is required.");
+ }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.
| startConnect(opts: StartConnectOpts) { | |
| const onMessage = (e: MessageEvent) => { | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", onMessage); | |
| } else if (e.data?.type === "success") { | |
| opts.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| opts.onError?.(new ConnectError(e.data.error)); | |
| } | |
| }; | |
| startConnect(opts: StartConnectOpts) { | |
| if (!opts.token) { | |
| throw new ConnectError("Token is required."); | |
| } | |
| if (!opts.app) { | |
| throw new ConnectError("App is required."); | |
| } | |
| const onMessage = (e: MessageEvent) => { | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", onMessage); | |
| } else if (e.data?.type === "success") { | |
| opts.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| opts.onError?.(new ConnectError(e.data.error)); | |
| } | |
| }; |
Move onMessage function outside startConnect for better readability.
Consider moving the onMessage function outside the startConnect method to improve readability and maintainability.
- startConnect(opts: StartConnectOpts) {
- const onMessage = (e: MessageEvent) => {
+ private onMessage = (e: MessageEvent) => {
+ if (e.origin !== this.baseURL) {
+ console.warn("Received message from unauthorized origin:", e.origin);
+ return;
+ }
+ if (e.data?.type === "close") {
+ this.iframe?.remove();
+ window.removeEventListener("message", this.onMessage);
+ } else if (e.data?.type === "success") {
+ this.currentOpts?.onSuccess?.({
+ id: e.data.authProvisionId,
+ });
+ } else if (e.data?.type === "error") {
+ this.currentOpts?.onError?.(new ConnectError(e.data.error));
+ }
+ };
+ startConnect(opts: StartConnectOpts) {
+ this.currentOpts = opts;
+ window.addEventListener("message", this.onMessage);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.
| startConnect(opts: StartConnectOpts) { | |
| const onMessage = (e: MessageEvent) => { | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", onMessage); | |
| } else if (e.data?.type === "success") { | |
| opts.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| opts.onError?.(new ConnectError(e.data.error)); | |
| } | |
| }; | |
| private onMessage = (e: MessageEvent) => { | |
| if (e.origin !== this.baseURL) { | |
| console.warn("Received message from unauthorized origin:", e.origin); | |
| return; | |
| } | |
| if (e.data?.type === "close") { | |
| this.iframe?.remove(); | |
| window.removeEventListener("message", this.onMessage); | |
| } else if (e.data?.type === "success") { | |
| this.currentOpts?.onSuccess?.({ | |
| id: e.data.authProvisionId, | |
| }); | |
| } else if (e.data?.type === "error") { | |
| this.currentOpts?.onError?.(new ConnectError(e.data.error)); | |
| } | |
| }; | |
| startConnect(opts: StartConnectOpts) { | |
| this.currentOpts = opts; | |
| window.addEventListener("message", this.onMessage); |
| const connectApp = (app: string) => { | ||
| if (!externalUserId || !token) return | ||
| setOauthAppId(app) | ||
| pd.startConnect({ | ||
| app, | ||
| token, | ||
| onSuccess: ({ id: authProvisionId }: any) => { | ||
| setAuthProvisionId(authProvisionId as string) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add error handling for missing externalUserId and token.
Ensure that the externalUserId and token are provided before calling startConnect.
- if (!externalUserId || !token) return
+ if (!externalUserId) {
+ throw new Error("External user ID is required.");
+ }
+ if (!token) {
+ throw new Error("Token is required.");
+ }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 connectApp = (app: string) => { | |
| if (!externalUserId || !token) return | |
| setOauthAppId(app) | |
| pd.startConnect({ | |
| app, | |
| token, | |
| onSuccess: ({ id: authProvisionId }: any) => { | |
| setAuthProvisionId(authProvisionId as string) | |
| } | |
| }) | |
| } | |
| const connectApp = (app: string) => { | |
| if (!externalUserId) { | |
| throw new Error("External user ID is required."); | |
| } | |
| if (!token) { | |
| throw new Error("Token is required."); | |
| } | |
| setOauthAppId(app) | |
| pd.startConnect({ | |
| app, | |
| token, | |
| onSuccess: ({ id: authProvisionId }: any) => { | |
| setAuthProvisionId(authProvisionId as string) | |
| } | |
| }) | |
| } |
There was a problem hiding this comment.
@dylburger, great to hear that the issue has been fixed! If there's anything else you need help with, feel free to ask.
| // Reactive Effects | ||
| useEffect(() => { | ||
| if (!externalUserId) { | ||
| setToken(null) | ||
| setGithubData(null) | ||
| setOauthAppId(null) | ||
| setAuthProvisionId(null) | ||
| } else { | ||
| serverConnectTokenCreate({ | ||
| app_id: oauthAppId, | ||
| external_id: externalUserId | ||
| }).then((t) => setToken(t)) | ||
| getAppsData(externalUserId).then((d) => { | ||
| setGithubData(d.github) | ||
| }) | ||
| } | ||
| }, [externalUserId]) |
There was a problem hiding this comment.
Add error handling for promises.
Ensure that the promises in the useEffect hook are handled properly.
- serverConnectTokenCreate({
- app_id: oauthAppId,
- external_id: externalUserId
- }).then((t) => setToken(t))
- getAppsData(externalUserId).then((d) => {
- setGithubData(d.github)
- })
+ serverConnectTokenCreate({
+ app_id: oauthAppId,
+ external_id: externalUserId
+ }).then((t) => setToken(t)).catch((err) => {
+ console.error("Error creating connect token:", err);
+ setToken(null);
+ });
+ getAppsData(externalUserId).then((d) => {
+ setGithubData(d.github);
+ }).catch((err) => {
+ console.error("Error fetching apps data:", err);
+ setGithubData(null);
+ });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.
| // Reactive Effects | |
| useEffect(() => { | |
| if (!externalUserId) { | |
| setToken(null) | |
| setGithubData(null) | |
| setOauthAppId(null) | |
| setAuthProvisionId(null) | |
| } else { | |
| serverConnectTokenCreate({ | |
| app_id: oauthAppId, | |
| external_id: externalUserId | |
| }).then((t) => setToken(t)) | |
| getAppsData(externalUserId).then((d) => { | |
| setGithubData(d.github) | |
| }) | |
| } | |
| }, [externalUserId]) | |
| // Reactive Effects | |
| useEffect(() => { | |
| if (!externalUserId) { | |
| setToken(null) | |
| setGithubData(null) | |
| setOauthAppId(null) | |
| setAuthProvisionId(null) | |
| } else { | |
| serverConnectTokenCreate({ | |
| app_id: oauthAppId, | |
| external_id: externalUserId | |
| }).then((t) => setToken(t)).catch((err) => { | |
| console.error("Error creating connect token:", err); | |
| setToken(null); | |
| }); | |
| getAppsData(externalUserId).then((d) => { | |
| setGithubData(d.github); | |
| }).catch((err) => { | |
| console.error("Error fetching apps data:", err); | |
| setGithubData(null); | |
| }); | |
| } | |
| }, [externalUserId]) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/sdk/examples/next-app/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (6)
- packages/sdk/examples/next-app/.eslintignore (1 hunks)
- packages/sdk/examples/next-app/.eslintrc.js (1 hunks)
- packages/sdk/examples/next-app/app/CodePanel.tsx (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/package.json (1 hunks)
- packages/sdk/examples/next-app/tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/sdk/examples/next-app/.eslintignore
- packages/sdk/examples/next-app/.eslintrc.js
- packages/sdk/examples/next-app/package.json
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/examples/next-app/app/page.tsx
Additional context used
Learnings (1)
packages/sdk/examples/next-app/app/CodePanel.tsx (1)
Learnt from: dylburger PR: PipedreamHQ/pipedream#12685 File: packages/sdk/examples/next-app/app/CodePanel.tsx:42-42 Timestamp: 2024-08-14T17:26:51.614Z Learning: In React components, use `DOMPurify.sanitize` to sanitize HTML content instead of using `dangerouslySetInnerHTML` to prevent XSS attacks.
Biome
packages/sdk/examples/next-app/app/CodePanel.tsx
[error] 43-43: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
Additional comments not posted (2)
packages/sdk/examples/next-app/tsconfig.json (1)
1-27: TypeScript configuration looks good.The
tsconfig.jsonfile is well-configured for a Next.js project, with appropriate compiler options and module resolutions.packages/sdk/examples/next-app/app/CodePanel.tsx (1)
1-51: React component implementation is secure and well-structured.The
CodePanelcomponent correctly usesDOMPurify.sanitizeto prevent XSS attacks, and theuseEffecthook is properly configured with dependencies. The component is well-structured and follows React best practices.Tools
Biome
[error] 43-43: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
packages/sdk/dist/index.js
Outdated
| this.environment = opts.environment; | ||
| this.secretKey = opts.secretKey; | ||
| this.publicKey = opts.publicKey; | ||
| const { apiHost = "pipedream.com" } = opts; |
There was a problem hiding this comment.
Needs to be changed to api.pipedream.com
There was a problem hiding this comment.
done in new branch
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/examples/next-app/app/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/sdk/src/browser/index.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/sdk/src/browser/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
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 (3)
- components/activecalculator/activecalculator.app.mjs (1 hunks)
- components/aryn/aryn.app.mjs (1 hunks)
- packages/sdk/.gitignore (1 hunks)
Files skipped from review due to trivial changes (3)
- components/activecalculator/activecalculator.app.mjs
- components/aryn/aryn.app.mjs
- packages/sdk/.gitignore
Fixing builds, improving TypeScript defs, simplifying DX based on API changes. LOTS of new docs. --------- Co-authored-by: Danny Roosevelt <[email protected]>
| @@ -8,4 +8,4 @@ export default { | |||
| console.log(Object.keys(this.$auth)); | |||
| }, | |||
| }, | |||
| }; | |||
There was a problem hiding this comment.
Ignore all of these component changes. We should fix the underlying ESLint config so this doesn't happen, not urgent for this PR
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (6)
docs-v2/pages/connect/use-cases.mdx (2)
11-11: Consider using synonyms for clarity.The word "hard" is repeated multiple times, which might affect the readability. Consider using synonyms for variety and clarity.
- But it's hard to justify the engineering effort required for these integrations. They're a distraction from the core product. Once built, they're hard to maintain. + But it's challenging to justify the engineering effort required for these integrations. They're a distraction from the core product. Once built, they're difficult to maintain.Tools
LanguageTool
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...cts and Enterprise customers. But it's hard to justify the engineering effort requi...(HARD_TO)
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...m the core product. Once built, they're hard to maintain. You have to securely manag...(HARD_TO)
25-25: Simplify phrasing for clarity.The phrase "a variety of" can be simplified to improve clarity.
- But most teams work in Slack, Discord, Microsoft Teams, or a variety of other messaging apps. + But most teams work in Slack, Discord, Microsoft Teams, or other messaging apps.Tools
LanguageTool
[style] ~25-~25: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... in Slack, Discord, Microsoft Teams, or a variety of other messaging apps. Sometimes you wan...(A_VARIETY_OF)
[style] ~25-~25: To elevate your writing, try using a synonym here.
Context: ...ges via SMS or push notifications. It's hard to maintain integrations for all the ap...(HARD_TO)
docs-v2/pages/connect/index.mdx (1)
63-63: Correct grammatical issue.There is a noun/verb agreement issue in the security section. Consider revising for grammatical accuracy.
- Use secure, session-based auth between your client and server + Use secure, session-based authentication between your client and serverTools
LanguageTool
[grammar] ~63-~63: There seems to be a noun/verb agreement error. Did you mean “secures” or “secured”?
Context: ...omatically redirected to HTTPS. - **Use secure, session-based auth between your client...(SINGULAR_NOUN_VERB_AGREEMENT)
docs-v2/pages/connect/api.mdx (1)
125-125: Ensure complete sentences.The description for
:app_idshould be a complete sentence for clarity.- `:app_id` can be `oauth_app_id` for [OAuth apps](/connect/quickstart#creating-a-custom-oauth-client) or [name slug](/connect/quickstart/#find-your-apps-name-slug) for key-based apps + The `:app_id` parameter can be either `oauth_app_id` for [OAuth apps](/connect/quickstart#creating-a-custom-oauth-client) or the [name slug](/connect/quickstart/#find-your-apps-name-slug) for key-based apps.Tools
LanguageTool
[style] ~125-~125: To form a complete sentence, be sure to include a subject.
Context: ...apps/:app_id/accounts ``` -:app_idcan be `oauth_app_id` for [OAuth apps](/con...(MISSING_IT_THERE)
docs-v2/pages/connect/quickstart.mdx (2)
18-23: Remove unnecessary comma for clarity.Consider removing the comma before "as well" to improve readability.
- ...-oauth-client) and pass the OAuth app ID, as well. + ...-oauth-client) and pass the OAuth app ID as well.Tools
LanguageTool
[typographical] ~23-~23: A comma before “as well” is not needed unless you want to explicitly mark a pause in speech.
Context: ...-oauth-client) and pass the OAuth app ID, as well. #### Find your app's name slu...(AS_WELL_UNNECESSARY_COMMA)
92-102: Consider varying phrasing for repeated sentences.The section is comprehensive and provides clear instructions. Consider varying the phrasing for repeated sentences to improve readability.
Tools
LanguageTool
[style] ~99-~99: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...pecific operations, for example: - You need to initiate the account connection flow fo...(REP_NEED_TO_VB)
[style] ~100-~100: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...'d be charged for those accounts. - You need to retrieve account credentials for your e...(REP_NEED_TO_VB)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/sdk/examples/next-app/package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (19)
- docs-v2/pages/_meta.json (1 hunks)
- docs-v2/pages/connect/_meta.json (1 hunks)
- docs-v2/pages/connect/api.mdx (1 hunks)
- docs-v2/pages/connect/index.mdx (1 hunks)
- docs-v2/pages/connect/quickstart.mdx (1 hunks)
- docs-v2/pages/connect/use-cases.mdx (1 hunks)
- docs-v2/pages/connected-accounts/index.mdx (1 hunks)
- packages/sdk/.gitignore (1 hunks)
- packages/sdk/README.md (1 hunks)
- packages/sdk/examples/next-app/.env.example (1 hunks)
- packages/sdk/examples/next-app/README.md (1 hunks)
- packages/sdk/examples/next-app/app/page.tsx (1 hunks)
- packages/sdk/examples/next-app/app/server.ts (1 hunks)
- packages/sdk/examples/next-app/package.json (1 hunks)
- packages/sdk/package.json (1 hunks)
- packages/sdk/src/browser/index.ts (1 hunks)
- packages/sdk/src/server/index.ts (1 hunks)
- packages/sdk/tsconfig.browser.json (1 hunks)
- packages/sdk/tsconfig.node.json (1 hunks)
Files skipped from review due to trivial changes (7)
- docs-v2/pages/_meta.json
- packages/sdk/.gitignore
- packages/sdk/README.md
- packages/sdk/examples/next-app/README.md
- packages/sdk/examples/next-app/package.json
- packages/sdk/tsconfig.browser.json
- packages/sdk/tsconfig.node.json
Files skipped from review as they are similar to previous changes (5)
- packages/sdk/examples/next-app/.env.example
- packages/sdk/examples/next-app/app/page.tsx
- packages/sdk/examples/next-app/app/server.ts
- packages/sdk/package.json
- packages/sdk/src/browser/index.ts
Additional context used
LanguageTool
docs-v2/pages/connect/use-cases.mdx
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...cts and Enterprise customers. But it's hard to justify the engineering effort requi...(HARD_TO)
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...m the core product. Once built, they're hard to maintain. You have to securely manag...(HARD_TO)
[style] ~25-~25: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... in Slack, Discord, Microsoft Teams, or a variety of other messaging apps. Sometimes you wan...(A_VARIETY_OF)
[style] ~25-~25: To elevate your writing, try using a synonym here.
Context: ...ges via SMS or push notifications. It's hard to maintain integrations for all the ap...(HARD_TO)
docs-v2/pages/connect/index.mdx
[grammar] ~63-~63: There seems to be a noun/verb agreement error. Did you mean “secures” or “secured”?
Context: ...omatically redirected to HTTPS. - **Use secure, session-based auth between your client...(SINGULAR_NOUN_VERB_AGREEMENT)
docs-v2/pages/connect/api.mdx
[style] ~125-~125: To form a complete sentence, be sure to include a subject.
Context: ...apps/:app_id/accounts ``` -:app_idcan be `oauth_app_id` for [OAuth apps](/con...(MISSING_IT_THERE)
[style] ~373-~373: To form a complete sentence, be sure to include a subject.
Context: .../apps/:app_id/accounts ``` -:app_idcan be `oauth_app_id` for [OAuth apps](/con...(MISSING_IT_THERE)
docs-v2/pages/connect/quickstart.mdx
[typographical] ~23-~23: A comma before “as well” is not needed unless you want to explicitly mark a pause in speech.
Context: ...-oauth-client) and pass the OAuth app ID, as well. #### Find your app's name slu...(AS_WELL_UNNECESSARY_COMMA)
[style] ~99-~99: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...pecific operations, for example: - You need to initiate the account connection flow fo...(REP_NEED_TO_VB)
[style] ~100-~100: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...'d be charged for those accounts. - You need to retrieve account credentials for your e...(REP_NEED_TO_VB)
Additional comments not posted (15)
docs-v2/pages/connect/_meta.json (2)
3-3: Improved title clarity.Changing the title from "Pipedream Connect" to "Overview" provides a clearer introduction to the content.
5-12: Enhanced documentation structure.The addition of "use-cases," "quickstart," and "api" sections improves the organization and accessibility of the documentation. This change helps users find relevant information more easily.
docs-v2/pages/connect/use-cases.mdx (1)
1-41: Informative use cases.The use cases are well-articulated, showcasing the versatility of Pipedream Connect in various applications. This section effectively highlights the core value to developers and customers.
Tools
LanguageTool
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...cts and Enterprise customers. But it's hard to justify the engineering effort requi...(HARD_TO)
[style] ~11-~11: To elevate your writing, try using a synonym here.
Context: ...m the core product. Once built, they're hard to maintain. You have to securely manag...(HARD_TO)
[style] ~25-~25: The phrase “a variety of” may be wordy. To make your writing clearer, consider replacing it.
Context: ... in Slack, Discord, Microsoft Teams, or a variety of other messaging apps. Sometimes you wan...(A_VARIETY_OF)
[style] ~25-~25: To elevate your writing, try using a synonym here.
Context: ...ges via SMS or push notifications. It's hard to maintain integrations for all the ap...(HARD_TO)
packages/sdk/src/server/index.ts (2)
1-6: Well-defined types for server client options.The types for
CreateServerClientOptsare clearly defined, enhancing type safety and code readability.
98-152: Comprehensive API request handling.The
_makeConnectRequestmethod is well-structured for handling API requests, including parameter appending and header management. Consider adding more detailed comments for maintainability.docs-v2/pages/connect/index.mdx (1)
2-3: Enhanced interactivity with new components.The addition of
TabsandImagecomponents enhances the interactivity and visual appeal of the documentation, likely improving user engagement.docs-v2/pages/connect/api.mdx (2)
1-7: Comprehensive API documentation.The API documentation is detailed and informative, providing clear guidance on authentication and endpoint usage.
373-373: Ensure complete sentences.The description for
:app_idshould be a complete sentence for clarity.Tools
LanguageTool
[style] ~373-~373: To form a complete sentence, be sure to include a subject.
Context: .../apps/:app_id/accounts ``` -:app_idcan be `oauth_app_id` for [OAuth apps](/con...(MISSING_IT_THERE)
docs-v2/pages/connected-accounts/index.mdx (1)
236-236: Update approved: Reference to "Pipedream Connect."The update from "Nango" to "Pipedream Connect" reflects the current recommendations and is consistent with the document's context.
docs-v2/pages/connect/quickstart.mdx (6)
12-14: Section approved: Enable the feature flag.The instructions for enabling the feature flag for Pipedream Connect are clear and concise.
25-33: Section approved: Find your app's name slug.The instructions for finding the app's name slug are clear and the examples are helpful.
38-47: Section approved: Creating a custom OAuth client.The instructions for creating a custom OAuth client are clear, and the warning callout effectively highlights important information.
53-61: Section approved: Get your project keys.The instructions for obtaining project keys are clear and concise.
63-90: Section approved: Run the Pipedream demo app, or configure your own.The instructions for running the demo app or configuring your own are clear and detailed.
551-587: Section approved: Connect your account from the frontend.The instructions for connecting an account from the frontend are clear, and the code example is practical.
| class ServerClient { | ||
| environment?: string; | ||
| secretKey: string; | ||
| publicKey: string; | ||
| baseURL: string; | ||
|
|
||
| constructor(opts: CreateServerClientOpts) { | ||
| this.environment = opts.environment; | ||
| this.secretKey = opts.secretKey; | ||
| this.publicKey = opts.publicKey; | ||
|
|
||
| const { apiHost = "api.pipedream.com" } = opts; | ||
| this.baseURL = `https://${apiHost}/v1`; | ||
| } |
There was a problem hiding this comment.
Consider enhancing error handling.
The constructor sets up the base URL and credentials, but consider adding validation to ensure that secretKey and publicKey are not empty or undefined. This can prevent runtime errors.
constructor(opts: CreateServerClientOpts) {
if (!opts.secretKey || !opts.publicKey) {
throw new Error("Both secretKey and publicKey are required.");
}
this.environment = opts.environment;
this.secretKey = opts.secretKey;
this.publicKey = opts.publicKey;
const { apiHost = "api.pipedream.com" } = opts;
this.baseURL = `https://${apiHost}/v1`;
}| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`); | ||
| } |
There was a problem hiding this comment.
Improve error message clarity.
The error message in the if (!response.ok) block could be more descriptive by including the response body or status text to aid in debugging.
if (!response.ok) {
const errorText = await response.text();
throw new Error(`HTTP error! status: ${response.status}, response: ${errorText}`);
}
WHY
Summary by CodeRabbit
New Features
.envfile to aid in environment variable configuration.Bug Fixes
Documentation
.envfile to assist in environment variable configuration.