Je/add scopes to connect sdk #17452
Je/add scopes to connect sdk #17452
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThis update introduces an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BackendClient
participant OAuthServer
User->>BackendClient: Create BackendClient({ scope })
BackendClient->>OAuthServer: Request token (with optional scope)
OAuthServer-->>BackendClient: Respond with token or error
alt Token retrieval fails (up to 3 attempts)
BackendClient->>OAuthServer: Retry with scope
OAuthServer-->>BackendClient: Respond with error (status, www-authenticate)
BackendClient-->>User: Throw detailed error
else Token retrieval succeeds
BackendClient-->>User: Return token
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/sdk/src/server/index.tsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'jsonc-eslint-parser' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/sdk/src/server/index.ts (3)
59-64: OAuth scope implementation looks good.The scope parameter is correctly implemented as an optional string array and properly passed to the OAuth token request. Consider adding validation for scope values in the future to ensure they match expected formats.
Also applies to: 205-205, 218-218, 311-313
305-308: Consider increasing the retry delay.A 100ms delay between OAuth token request retries might be too short, especially if the OAuth server is experiencing temporary issues.
- await new Promise((resolve) => setTimeout(resolve, 100)); + await new Promise((resolve) => setTimeout(resolve, 500 + attempts * 500)); // Progressive backoff
336-352: Improve type safety in error handling.The error handling uses
(e as any)which bypasses TypeScript's type checking. Consider defining a proper error type or using type guards.- if (e && typeof e === 'object' && 'response' in e) { - const errorResponse = (e as any).response; + interface OAuthError extends Error { + response?: { + status?: number; + headers?: Headers | Record<string, string>; + }; + } + + if (e && typeof e === 'object' && 'response' in e) { + const errorResponse = (e as OAuthError).response;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
components/bitget/bitget.app.mjs(1 hunks)components/explorium/explorium.app.mjs(1 hunks)components/hana/hana.app.mjs(1 hunks)components/indiefunnels/indiefunnels.app.mjs(1 hunks)components/joggai/joggai.app.mjs(1 hunks)components/mumble/mumble.app.mjs(1 hunks)components/sign_plus/sign_plus.app.mjs(1 hunks)components/webscrape_ai/webscrape_ai.app.mjs(1 hunks)packages/sdk/CHANGELOG.md(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/server/index.ts(5 hunks)packages/sdk/src/shared/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/sdk/package.json (1)
Learnt from: jcortes
PR: PipedreamHQ/pipedream#14935
File: components/sailpoint/package.json:15-18
Timestamp: 2024-12-12T19:23:09.039Z
Learning: When developing Pipedream components, do not add built-in Node.js modules like `fs` to `package.json` dependencies, as they are native modules provided by the Node.js runtime.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: pnpm publish
- GitHub Check: test
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (11)
components/joggai/joggai.app.mjs (1)
11-11: Trailing newline added – LGTMAdding a newline at EOF brings the file in line with POSIX conventions and prevents unnecessary diffs in future edits. No functional impact.
components/webscrape_ai/webscrape_ai.app.mjs (1)
11-11: Consistent EOF newline – looks goodThe added newline improves formatting consistency across the codebase. No further action needed.
components/sign_plus/sign_plus.app.mjs (1)
11-11: EOF newline addition – approvedMaintains standard formatting; no logical changes introduced.
components/hana/hana.app.mjs (1)
11-11: Formatting fix acknowledgedTrailing newline ensures clean diffs and editor compatibility. All good here.
components/indiefunnels/indiefunnels.app.mjs (1)
11-11: EOF newline – fineKeeps formatting uniform; nothing else to note.
components/explorium/explorium.app.mjs (1)
11-11: Trailing newline LGTMConsistent newline at EOF improves POSIX-tool compatibility and aligns with repo formatting conventions.
components/bitget/bitget.app.mjs (1)
11-11: EOF newline added – all goodKeeps formatting uniform with the rest of the codebase.
components/mumble/mumble.app.mjs (1)
11-11: Formatting only, no functional impactNothing else to add.
packages/sdk/CHANGELOG.md (1)
5-10: Changelog entry clear and matches implementationVersion
1.7.0notes the new optionalscopeparameter—concise and complete. ✔️packages/sdk/package.json (1)
4-4: Version bump matches changelogPackage version updated to
1.7.0; no other metadata changes. Synchronization looks correct.packages/sdk/src/shared/index.ts (1)
868-876: LGTM! Useful additions for enhanced error diagnostics.The new optional properties
wwwAuthenticateandheaderson theErrorResponsetype will help capture important error context from HTTP responses, particularly useful for OAuth authentication flows.
| const maxAttempts = 2; | ||
|
|
||
| while (!this.oauthAccessToken || this.oauthAccessToken.expiresAt - Date.now() < 1000) { | ||
| if (attempts > maxAttempts) { | ||
| throw new Error("ran out of attempts trying to retrieve oauth access token"); | ||
| } | ||
| if (attempts > 0) { | ||
| // Wait for a short duration before retrying to avoid rapid retries | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
| for (let attempts = 0; attempts <= maxAttempts; attempts++) { |
There was a problem hiding this comment.
Clarify the retry attempt count.
The variable is named maxAttempts = 2 but the loop condition attempts <= maxAttempts means it will actually attempt 3 times (0, 1, 2). This is confusing.
Either rename the variable or adjust the loop condition:
- const maxAttempts = 2;
+ const maxAttempts = 3; // Total attempts including the initial oneOr:
- for (let attempts = 0; attempts <= maxAttempts; attempts++) {
+ for (let attempts = 0; attempts < maxAttempts; attempts++) {📝 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 maxAttempts = 2; | |
| while (!this.oauthAccessToken || this.oauthAccessToken.expiresAt - Date.now() < 1000) { | |
| if (attempts > maxAttempts) { | |
| throw new Error("ran out of attempts trying to retrieve oauth access token"); | |
| } | |
| if (attempts > 0) { | |
| // Wait for a short duration before retrying to avoid rapid retries | |
| await new Promise((resolve) => setTimeout(resolve, 100)); | |
| for (let attempts = 0; attempts <= maxAttempts; attempts++) { | |
| const maxAttempts = 2; | |
| while (!this.oauthAccessToken || this.oauthAccessToken.expiresAt - Date.now() < 1000) { | |
| for (let attempts = 0; attempts < maxAttempts; attempts++) { |
🤖 Prompt for AI Agents
In packages/sdk/src/server/index.ts around lines 301 to 304, the retry loop uses
maxAttempts = 2 but the loop condition attempts <= maxAttempts causes three
attempts (0, 1, 2), which is confusing. To fix this, either rename maxAttempts
to maxAttemptIndex or change the loop condition to attempts < maxAttempts so the
number of attempts matches the variable name and intent.
WHY
Summary by CodeRabbit
Chores
New Features
Bug Fixes