-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Apify_oauth - update to import Apify changes #18451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughBumps versions for multiple Apify OAuth actions and sources, updates apify_oauth package.json (version, dependencies), refactors apify_oauth app to use ApifyClient with a request interceptor instead of a headers method, and adds trailing newlines to two unrelated app files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Apify OAuth App
participant Client as ApifyClient (with interceptor)
participant API as Apify API
User->>App: Invoke action/source
App->>App: _client() creates ApifyClient<br/>with OAuth token
Note over App,Client: Registers request interceptor<br/>to add x-apify-integration-platform
App->>Client: client.request(...)
Client->>API: HTTP request with Authorization<br/>+ x-apify-integration-platform
API-->>Client: Response
Client-->>App: Response
App-->>User: Result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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. Comment |
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michelle0927, LGTM! Ready for QA!
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/apify_oauth/apify_oauth.app.mjs (2)
12-24
: Consider memoizing the client and validating token.
Prevents repeated instantiation and fails fast when token is missing.Apply:
- _client() { - return new ApifyClient({ - token: this.$auth.oauth_access_token, - requestInterceptors: [ - (config) => ({ - ...config, - headers: { - ...(config.headers || {}), - "x-apify-integration-platform": "pipedream", - }, - }), - ], - }); - }, + _client() { + if (this._apifyClient) return this._apifyClient; + const token = this.$auth?.oauth_access_token; + if (!token) throw new Error("Missing Apify OAuth token"); + this._apifyClient = new ApifyClient({ + token, + requestInterceptors: [ + (config) => ({ + ...config, + headers: { + ...(config.headers || {}), + "x-apify-integration-platform": "pipedream", + }, + }), + ], + }); + return this._apifyClient; + },
12-24
: Add retry/timeout tuning when doing many calls.
Optional: pass maxRetries/minDelayBetweenRetriesMillis for resilience with Apify API.For example (tune as needed):
- this._apifyClient = new ApifyClient({ + this._apifyClient = new ApifyClient({ token, + maxRetries: 3, + minDelayBetweenRetriesMillis: 500, requestInterceptors: [Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
components/apify_oauth/apify_oauth.app.mjs
(2 hunks)components/apify_oauth/package.json
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/apify_oauth/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pnpm publish
🔇 Additional comments (3)
components/apify_oauth/apify_oauth.app.mjs (3)
2-2
: LGTM: correct ApifyClient import.
Named ESM import matches apify-client v2 API.
12-24
: No stale _headers() references remain
components/apify_oauth/apify_oauth.app.mjs only defines and uses_client()
, with no_headers()
calls found.
12-24
: Correct requestInterceptors usage. ApifyClient v2 supports arequestInterceptors
array and interceptor functions may return a modified config object as shown here.
Hi everyone, all test cases are passed! Ready for release! Test report |
/approve |
Updates versions to import the changes from #18308
Summary by CodeRabbit