-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Always treat local organizations as billing-eligible #4124
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: main
Are you sure you want to change the base?
Conversation
## Summary - short-circuit `is_billing_organization` when `ENV="local"` so local development always passes the billing check - keep the existing allowlist/database logic for non-local environments <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > Short-circuit `is_billing_organization()` for local environments to always return `True`, bypassing billing checks. > > - **Behavior**: > - In `is_billing_organization()` in `organization_pricing_service.py`, return `True` if `settings.ENV` is `local`, bypassing billing checks for local development. > - Retain existing billing logic for non-local environments. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for baaa5bf77b5145cf60ad859058e165f40fce7bbb. You can [customize](https://app.ellipsis.dev/Skyvern-AI/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
WalkthroughNavigation-triggered re-exfiltration and re-decoration logic removed from CDP event handlers. Mouse follower creation in JavaScript simplified by removing duplicate-detection guards and early return logic, allowing unconditional function invocation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Important
Looks good to me! 👍
Reviewed 21e402c in 1 minute and 21 seconds. Click for details.
- Reviewed
57lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/forge/sdk/routes/streaming/channels/exfiltration.py:89
- Draft comment:
Removed re-application of exfiltration/decoration on navigation. Confirm that skipping re-initialization for 'frame_navigated' and 'navigated_within_document' events is intended and won’t break page behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. skyvern/forge/sdk/routes/streaming/channels/js/decorate.js:5
- Draft comment:
Removed duplicate element check in __skyvern_create_mouse_follower. Without checking for an existing follower element, repeated calls may create duplicates. Confirm if this behavior is desired. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_7uxxNumEU3APM5qY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed everything up to 21e402c in 1 minute and 28 seconds. Click for details.
- Reviewed
57lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/forge/sdk/routes/streaming/channels/exfiltration.py:89
- Draft comment:
Removed reapplication of exfiltration and decoration on navigation events. This could lead to missing listener reinitialization on page navigations. Confirm if this change is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. skyvern/forge/sdk/routes/streaming/channels/js/decorate.js:5
- Draft comment:
Removed duplicate check for an existing mouse follower element. Without this check, multiple elements with the same ID may be created if the function is invoked more than once. Verify that this behavior is acceptable. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_27NKQlQv0b6qfJV8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
this pr became a mess as it was sync'd while somebody else was in the middle of the process |
This reverts commit 21e402c.
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.
Important
Looks good to me! 👍
Reviewed 67f6705 in 2 minutes and 13 seconds. Click for details.
- Reviewed
57lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern/forge/sdk/routes/streaming/channels/exfiltration.py:92
- Draft comment:
PR metadata mismatch: The title/description mention billing changes, but the code only re-applies exfiltration/decoration. Please update the PR metadata to reflect the actual changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to update the PR description, which is against the rules. It does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.
2. skyvern/forge/sdk/routes/streaming/channels/exfiltration.py:98
- Draft comment:
Be cautious of potential duplicate event listeners: Repeated calls to exfiltrate() register the console event handler each time. Confirm these operations are truly idempotent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment identifies a legitimate concern:page.on()in Playwright typically adds event listeners without removing previous ones, so callingexfiltrate()multiple times on the same page would register the console handler multiple times, causing_handle_console_eventto be called multiple times for each console message. This contradicts the code comment claiming the operations are idempotent. However, the comment uses cautious language like "Be cautious" and "Confirm these operations are truly idempotent" which sounds like it's asking the author to verify rather than stating a definite issue. This violates the rule about not asking authors to confirm or verify things. The comment might be identifying a real bug rather than just asking for confirmation. Ifpage.on()does accumulate listeners, this is a concrete issue that needs fixing (e.g., usingpage.once()or removing old listeners first). The phrasing might be cautious, but the underlying technical concern could be valid and actionable. While the technical concern may be valid, the comment's phrasing ("Be cautious", "Confirm these operations") is asking the author to verify rather than clearly stating an issue and suggesting a fix. A good comment would say something like "This will register duplicate console event handlers. Use page.once() or remove existing listeners first." The current phrasing is too speculative and verification-seeking. The comment should be deleted because it asks the author to "confirm" rather than clearly stating the issue and providing an actionable fix. While it may identify a real concern, the phrasing violates the rule against asking authors to verify or confirm things.
3. skyvern/forge/sdk/routes/streaming/channels/js/decorate.js:7
- Draft comment:
Good idempotency check: Verifying existence of '__skyvern_mouse_follower' prevents duplicate element creation. Ensure that this logic fits with event listener attachment requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%<= threshold50%The comment suggests ensuring that the logic fits with event listener attachment requirements. This is a bit vague and leans towards asking the author to ensure something, which is against the rules. However, it does point out a specific logic check regarding__skyvern_mouse_follower, which could be useful if it were more specific.
4. skyvern/forge/sdk/routes/streaming/channels/js/decorate.js:8
- Draft comment:
Unnecessary trailing comma in the getElementById argument; consider removing it for cleaner style. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_MIjSf7zRwHFkpO59
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Summary
is_billing_organizationwhenENV="local"so local development always passes the billing checkImportant
Simplifies navigation event handling and mouse follower creation by removing redundant operations and checks.
exfiltration.py.decorate.jsfor mouse follower creation.This description was created by
for 67f6705. You can customize this summary. It will automatically update as commits are pushed.
🔍 Detailed Analysis
Key Changes
exfiltration.pydecorate.jsTechnical Implementation
flowchart TD A[Navigation Event] --> B{Before: Check Event Type} B -->|frame_navigated/navigated_within_document| C[Re-apply Exfiltration & Decoration] C --> D[Create Tasks for All Pages] E[Navigation Event] --> F[After: Direct Event Handling] F --> G[No Automatic Re-application] H[Mouse Follower Creation] --> I{Before: Check Existing} I -->|Exists| J[Return False] I -->|Not Exists| K[Create Element] K --> L[Return True] M[Mouse Follower Creation] --> N[After: Direct Creation] N --> O[Always Create Element]Impact
The PR title "Always treat local organizations as billing-eligible" and description mention billing organization changes, but the actual code changes are completely unrelated to billing functionality. This appears to be a mismatch between the PR metadata and the actual implementation.
Created with Palmier
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.