Skip to content

Add high-level BiDi script pin and unpin commands - #17801

Open
mayank-at-sauce wants to merge 3 commits into
SeleniumHQ:trunkfrom
mayank-at-sauce:mayankbhandari/feature/add-web-first-retry
Open

Add high-level BiDi script pin and unpin commands#17801
mayank-at-sauce wants to merge 3 commits into
SeleniumHQ:trunkfrom
mayank-at-sauce:mayankbhandari/feature/add-web-first-retry

Conversation

@mayank-at-sauce

Copy link
Copy Markdown
Contributor

Part of #13992 — implements the pin / unpin cells for the Ruby binding.

What does this PR do?
Adds driver.script.pin and driver.script.unpin to the Ruby bindings, bringing the high-level BiDi Script API closer to parity with Python/Java/JS (which already have these).

pin takes a function declaration and registers it as a BiDi preload script, so it runs on every new browsing context before the page's own scripts — handy for injecting helpers, polyfills, or instrumentation. It returns an id that you pass to unpin to remove it later:

id = driver.script.pin('() => { window.helper = () => 42; }')
driver.navigate.to("https://example.com")

window.helper is available on the page

driver.script.unpin(id)

Implementation Notes
Under the hood these are thin wrappers over the existing script.addPreloadScript / script.removePreloadScript BiDi commands, sent via @bidi.send_cmd — the same pattern the network handlers already use. I kept the surface deliberately minimal (return the raw preload-script id rather than a wrapper object) to match how the JS binding exposes it. Script#initialize now keeps a reference to the bidi object so both the log handlers and these commands can share it; behaviour when BiDi isn't enabled is unchanged (it still raises on driver.script). Added RBS signatures and integration specs covering both the pin path (script runs on a freshly navigated document) and the unpin path (script no longer runs).

@selenium-ci selenium-ci added C-rb Ruby Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 20, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add Ruby BiDi Script#pin/#unpin wrappers for preload scripts

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Add Script#pin/#unpin to manage BiDi preload scripts from Ruby.
• Persist BiDi connection on Script to reuse across handlers and commands.
• Add RBS signatures and integration specs for pin and unpin behavior.
Diagram

graph TD
  A["Ruby: Driver#script"] --> B["Script (common)"] --> C(["BiDi session (@bidi)"]) --> D(("Browser"))
  B --> E["BiDi LogHandler"]
  C --> F["BiDi cmd: add/removePreloadScript"]

  subgraph Legend
    direction LR
    _api["API/Object"] ~~~ _svc(["Session/Transport"]) ~~~ _ext(("External Runtime"))
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Return a typed PreloadScript wrapper object
  • ➕ Allows richer API later (e.g., metadata, automatic cleanup via finalizers/blocks).
  • ➕ Improves type safety and discoverability over a raw string id.
  • ➖ More surface area and maintenance burden.
  • ➖ Potential mismatch with other bindings if they return raw ids.
2. Expose a more complete BiDi Script API (options/sandbox/contexts)
  • ➕ Avoids future API churn by supporting the full command payload early.
  • ➕ Better supports advanced use cases (scoped preload scripts, configuration options).
  • ➖ Higher complexity for a first iteration.
  • ➖ May require additional compatibility decisions across browsers/implementations.

Recommendation: The current approach (thin wrappers returning the raw preload-script id) is a good fit for a parity-driven addition: minimal surface, leverages existing @bidi.send_cmd patterns, and is backed by integration specs. Consider a wrapper object only if future features (scoping/options/lifecycle management) become a near-term roadmap item.

Files changed (3) +41 / -1

Enhancement (1) +21 / -1
script.rbAdd Script#pin and #unpin using BiDi preload script commands +21/-1

Add Script#pin and #unpin using BiDi preload script commands

• Stores the BiDi session on Script initialization and reuses it for both the existing LogHandler and new commands. Adds pin(script) as a wrapper over script.addPreloadScript (returning the preload script id) and unpin(script_id) over script.removePreloadScript.

rb/lib/selenium/webdriver/common/script.rb

Tests (1) +16 / -0
script_spec.rbAdd integration specs verifying pin runs and unpin stops running +16/-0

Add integration specs verifying pin runs and unpin stops running

• Adds a spec asserting a pinned preload script runs on a freshly navigated document and another asserting unpin prevents the script from running on subsequent navigations.

rb/spec/integration/selenium/webdriver/bidi/script_spec.rb

Other (1) +4 / -0
script.rbsAdd RBS signatures for Script#pin and #unpin +4/-0

Add RBS signatures for Script#pin and #unpin

• Extends the Script RBS to include pin(String) -> String and unpin(String) -> untyped, matching the new Ruby API surface.

rb/sig/lib/selenium/webdriver/common/script.rbs

@qodo-code-review

qodo-code-review Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 19 rules

Grey Divider


Action required

1. unpin missing @return tag ✓ Resolved 📘 Rule violation ✧ Quality
Description
The newly added public method unpin has YARD documentation but lacks an @return tag, so the
public API contract is incomplete for consumers and generated docs. This violates the requirement to
document public API methods with full YARD tags.
Code

rb/lib/selenium/webdriver/common/script.rb[R57-62]

+      # Unpins a previously pinned script so it no longer runs on new pages.
+      #
+      # @param [String] script_id the id returned by #pin
+      def unpin(script_id)
+        @bidi.send_cmd('script.removePreloadScript', script: script_id)
+      end
Evidence
PR Compliance ID 389240 requires public API methods to include YARD documentation with @param tags
for parameters and an @return tag for the return value. The new unpin method has a description
and @param but no @return in its YARD block.

Rule 389240: Document public API methods with YARD tags
rb/lib/selenium/webdriver/common/script.rb[57-62]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The public method `unpin` is missing a YARD `@return` tag in its doc block.

## Issue Context
This PR adds `Script#pin` and `Script#unpin` as user-facing Ruby binding APIs, so both methods must be fully documented per the project’s public API YARD requirements.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/script.rb[57-62]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread rb/lib/selenium/webdriver/common/script.rb
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit b895acc

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 2e6f2ba

@titusfortner

Copy link
Copy Markdown
Member

We need to clarify the behavior we expect from this, since it is inconsistent between bindings right now.
That means deciding this first #17776

@titusfortner

Copy link
Copy Markdown
Member

Also, we're migrating things to implement off of the newly generated BiDi::Protocol classes. I haven't dug too deep into your implementation, but I'd like to get rid of the BiDi class entirely in the near future. Are you on Slack? Can we discuss it there?

@mayank-at-sauce

Copy link
Copy Markdown
Contributor Author

Hi @titusfortner sure, let me connect with you on slack. Thanks!

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

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-rb Ruby Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants