Skip to content

Conversation

@celalzamanoglu
Copy link
Contributor

@celalzamanoglu celalzamanoglu commented Nov 28, 2025

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

Important

Simplifies navigation event handling and mouse follower creation by removing redundant operations and checks.

  • Navigation Event Handling:
    • Removed automatic re-application of exfiltration and decoration on navigation events in exfiltration.py.
    • Simplified logic to handle events directly without redundant operations.
  • Mouse Follower UI:
    • Removed duplicate element checks in decorate.js for mouse follower creation.
    • Streamlined creation logic to always create the element without checking for existence.
  • Code Cleanup:
    • Eliminated redundant conditional logic and return value handling in navigation and UI code.

This description was created by Ellipsis for 67f6705. You can customize this summary. It will automatically update as commits are pushed.



⚠️ Mismatch Alert: The PR title indicates billing-related changes for local organizations, but the actual patch only contains unrelated streaming and UI decoration code removals.

🔍 Detailed Analysis

Key Changes

  • Streaming Channel: Removed automatic re-application of exfiltration and decoration on navigation events in exfiltration.py
  • Mouse Follower UI: Simplified mouse follower creation logic by removing duplicate element checks in decorate.js
  • Code Cleanup: Eliminated redundant conditional logic and return value handling

Technical 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]
Loading

Impact

  • Performance: Reduces unnecessary DOM operations by removing automatic re-application of decorations on navigation
  • Simplification: Streamlines mouse follower creation by removing redundant existence checks
  • Behavioral Change: Navigation events no longer trigger automatic exfiltration/decoration re-application

⚠️ Critical Issue

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

  • Refactor
    • Simplified navigation event handling by removing redundant exfiltration and decoration operations during page navigation.
    • Updated mouse follower initialization logic to allow unconditional creation without duplicate prevention checks.

✏️ Tip: You can customize this high-level summary in your review settings.

## 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 -->
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 28, 2025

Walkthrough

Navigation-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

Cohort / File(s) Summary
CDP Event Handling
skyvern/forge/sdk/routes/streaming/channels/exfiltration.py
Removed re-application of exfiltration and decoration on frame_navigated and navigated_within_document CDP events; navigation event handling no longer iterates pages and invokes async exfiltrate/decorate tasks.
Mouse Follower Creation
skyvern/forge/sdk/routes/streaming/channels/js/decorate.js
Removed duplicate-detection guard for element with id __skyvern_mouse_follower; removed early return logic from creation function; creation now invoked unconditionally. Method signature changed from returning boolean to void.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Duplicate-check removal in decorate.js: Verify the rationale for allowing multiple followers with the same id; assess whether this could introduce unintended visual or functional issues.
  • Navigation event handling removal: Confirm that removing re-exfiltration/re-decoration on navigation does not break state consistency or data freshness expectations in browser contexts.
  • Method signature change: Ensure all callers of window.__skyvern_create_mouse_follower() do not depend on the boolean return value.

Possibly related PRs

Poem

🐰 No guards to check, the mouse now dances free,
Creation flows unconditional, wild as can be,
Navigation's whispers fade to quiet night,
Simplicity wins—one less thing to fight!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'Always treat local organizations as billing-eligible' describes billing logic changes, but the actual changeset removes navigation event handling from exfiltration and duplicate guard logic from mouse follower creation—completely unrelated to billing. Update the title to accurately reflect the main changes: focus on exfiltration channel navigation event handling removal or mouse follower duplicate detection removal, whichever is the primary intent.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch celal/fix-credit-check-on-local

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 57 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 57 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft 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 Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@celalzamanoglu
Copy link
Contributor Author

this pr became a mess as it was sync'd while somebody else was in the middle of the process

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 57 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft 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% <= threshold 50% 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 calling exfiltrate() multiple times on the same page would register the console handler multiple times, causing _handle_console_event to 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. If page.on() does accumulate listeners, this is a concrete issue that needs fixing (e.g., using page.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% <= threshold 50% 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% <= threshold 50% None

Workflow ID: wflow_MIjSf7zRwHFkpO59

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants