[adr] script and logging async/event API on driver.script - #17776
Open
titusfortner wants to merge 5 commits into
Open
[adr] script and logging async/event API on driver.script#17776titusfortner wants to merge 5 commits into
titusfortner wants to merge 5 commits into
Conversation
Contributor
PR Summary by QodoAdd ADR defining the driver.script async script & event API surface
AI Description
High-Level Assessment
Files changed (1)
|
titusfortner
force-pushed
the
c/bold-archimedes-8235d8
branch
from
July 13, 2026 21:52
0cca0fc to
c0cfe40
Compare
Contributor
Code Review by Qodo
1. Old/new values undefined
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new Architecture/Design Record (ADR) that standardizes the cross-binding, protocol-neutral driver.script surface for script execution/pinning and for console/JS-error/DOM-mutation event subscriptions, intended to guide Selenium 5 convergence work.
Changes:
- Defines the
driver.scriptAPI responsibilities and normative behavior forexecute,pin/unpin, andpreload/removal. - Specifies handler registration/removal semantics (including returning registration objects) and the expected shaped payloads for console/error/mutation events.
- Documents current per-binding divergence and the intended convergence outcomes.
Contributor
|
Code review by qodo was updated up to the latest commit 5905691 |
Contributor
|
Code review by qodo was updated up to the latest commit 309ef00 |
Contributor
Code Review by Qodo
1. Realm/args ambiguity
|
Comment on lines
+87
to
+89
| specialization of, exposed for events no binding pre-defines. The handler runs the script now and, | ||
| unless limited to the current context, keeps it on every future load so it survives navigation; | ||
| `remove_event_handler` detaches the subscription and stops future loads. |
Contributor
There was a problem hiding this comment.
2. Handler persistence unclear 🐞 Bug ≡ Correctness
Decision 4 says add_event_handler “runs the script now” and also persists on “every future load” unless “limited to the current context,” but the ADR never defines how a caller requests document-only vs navigation-persistent behavior (Decision 8 only defines browsing/user context scoping).
Agent Prompt
### Issue description
The ADR introduces two lifetimes for event handlers (immediate + persistent across future loads vs limited) but does not define the API surface or semantics for choosing the lifetime.
### Issue Context
- Decision 4 uses “unless limited to the current context” but provides no parameter/flag describing that limitation.
- Decision 8 defines *which* browsing/user context is targeted, not whether the handler persists across navigations within that context.
- Consequences later clarify removal prevents future loads but doesn’t undo already-injected observers; that still leaves registration mode undefined.
### Fix Focus Areas
- docs/decisions/17776-bidi-script.md[87-89]
- docs/decisions/17776-bidi-script.md[150-155]
- docs/decisions/17776-bidi-script.md[200-203]
### Suggested direction
Add explicit contract text (and/or a named parameter) that defines:
- Default lifetime (e.g., persists across navigations in the scoped browsing context).
- How to request "current document only" behavior.
- Interaction between lifetime and browsing-context vs user-context scoping.
- Confirm whether removal is required to teardown the current document or only prevent re-installation on future loads (and align Decision 4 wording accordingly).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+161
to
+162
| driver.script().addConsoleMessageHandler(tab, m -> log(m.getText())); // tab: a window handle | ||
| driver.script().execute("return self.location.href", worker); // worker: a frame/worker/tab realm |
Contributor
There was a problem hiding this comment.
1. Realm/args ambiguity 🐞 Bug ≡ Correctness
Decision 1 defines execute(script, *args), but Decision 8 adds an optional realm and the Java
example execute("…", worker) is indistinguishable from passing worker as the first script
argument. This underspecifies the API shape and will likely produce incompatible binding signatures
and/or incorrect execution context selection.
Agent Prompt
### Issue description
The ADR introduces an optional realm for `execute`, but doesn’t specify how that realm is passed without colliding with `*args` in bindings that use positional arguments (notably Java). The Java example currently implies the realm is the second positional argument, which conflicts with the earlier `execute(script, *args)` contract.
### Issue Context
This is an ADR intended to drive convergent API design across bindings; ambiguity here will cause divergent method signatures and user confusion.
### Fix Focus Areas
- docs/decisions/17776-bidi-script.md[150-154]
- docs/decisions/17776-bidi-script.md[161-162]
- docs/decisions/17776-bidi-script.md[194-196]
### What to change (one of these, explicitly stated in the ADR)
- Define a required options/config object for non-keyword-arg languages (e.g., `ExecuteOptions{ realm, args }`) and update the Java example accordingly; **or**
- Define a Java-specific overload ordering that removes ambiguity (e.g., `execute(String script, Realm realm, Object... args)`), and update examples to show realm-first (and how to pass script args simultaneously); **or**
- Reserve a distinct type/wrapper for realm so it can’t be confused with normal args, and show the invocation pattern.
Update the Java snippet so it cannot be read as passing `worker` into the script’s arguments.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
There was a problem hiding this comment.
I think this should be clarified as well.
Contributor
|
Code review by qodo was updated up to the latest commit 67d8d4e |
diemol
reviewed
Aug 15, 2026
| driver.script().execute("return document.title"); | ||
| ``` | ||
|
|
||
| 2. **`pin(source)` registers a script the browser keeps on every future page** — run before the |
Member
There was a problem hiding this comment.
What if a user wants to pin a script only for the current browser context? Is that a scenario we should consider?
Comment on lines
+161
to
+162
| driver.script().addConsoleMessageHandler(tab, m -> log(m.getText())); // tab: a window handle | ||
| driver.script().execute("return self.location.href", worker); // worker: a frame/worker/tab realm |
Member
There was a problem hiding this comment.
I think this should be clarified as well.
|
|
||
| **Scoping** | ||
|
|
||
| 8. **Events attach to a window handle; `execute` runs in a realm.** A handler defaults to the current |
Member
There was a problem hiding this comment.
Shouldn't we add pin to this scoping? Would this allow a user to better scope the scripts they pin in case they need that?
Comment on lines
+192
to
+193
| - `execute` overlaps the existing `execute_script`; whether it supersedes that method is a separate | ||
| migration decision, not settled here. |
Member
There was a problem hiding this comment.
Wouldn't this just be applying the deprecation policy to execute_script?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📄 The decision, its rationale, considered options, and consequences are in the record file this
PR adds (
docs/decisions/17776-bidi-script.md); read it there. The sections below are proposalnotes and review logistics.
🔗 Related
pin/unpin/execute, andadd/remove for console-message, JavaScript-error, and DOM-mutation handlers under
driver.script), and where the per-binding implementation history is tracked.driver.scriptis the protocol-neutralsurface consistent with that boundary.
📝 Proposal notes
the script and logging async/event API; on acceptance the charter links back to it.
driver.scriptaccessor; placing the
log-domain console and error events underscript;pin/unpin/execute; and separate add/remove handlers for console messages, JavaScript errors, and DOMmutations.
in two ways; the rest of its calls fill in details [🚀 Feature]: Implement high level BiDi script commands #13992 left unspecified and are reasoned in the
record:
preloadout ofpin— the summit had a singlepin; this separates run-by-handle(
pin) from run-on-every-navigation (preload). Naming both lets the record classify eachbinding's current
pinas one or the other;add_*, not an integer id.script.execute(source)overlaps the existingexecute_script(andexecuteAsyncScript); whetherscript.executesupersedes and deprecates them is a largermigration decision this record does not make.
stores pinned scripts, routes
log.entryAdded, and keys handler removal is left to the adoptingPRs.
🗣 Discussion
Discussed at the TLC meetings below; see the minutes for the full discussion and attribution.
pinandpreloadare oneconcept, not two; console and JavaScript error handlers stay separate. Related:
mapping events back to contexts, where
event payloads not identifying the firing context was raised as a spec gap.
window handle rather than a context, since events attach to a top-level navigable.
📌 Tracking
Tracking issue: (linked on acceptance)