Add "selectAll" parameter to extract and observe methods#2081
Open
NahomAgidew wants to merge 4 commits intobrowserbase:mainfrom
Open
Add "selectAll" parameter to extract and observe methods#2081NahomAgidew wants to merge 4 commits intobrowserbase:mainfrom
NahomAgidew wants to merge 4 commits intobrowserbase:mainfrom
Conversation
|
Contributor
|
This PR is from an external contributor and must be approved by a stagehand team member with write access before CI can run. |
github-actions
Bot
added
external-contributor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 14 files
Confidence score: 4/5
- This PR looks safe to merge overall, with one moderate-risk issue rather than a clear merge blocker.
- In
packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts, exception handling appears to drop already-resolvedobjectIds, which can lose partial results and reduce snapshot completeness when failures occur. - Given the 5/10 severity (high confidence), this is a targeted correctness concern but not strong evidence of broad breakage across the PR.
- Pay close attention to
packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts- preserve previously resolved IDs when exceptions are raised.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts:318">
P2: Exception handling discards already-resolved objectIds instead of preserving partial results.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant API as Stagehand API Client
participant V3 as V3 Core
participant Handler as ExtractHandler / ObserveHandler
participant Snapshot as Snapshot Capture
participant A11y as A11y Processor
participant CDP as Chrome DevTools Protocol
Note over Client,CDP: NEW: selectAll parameter flow for extract() and observe()
Client->>API: extract() / observe() with options.selectAll = true
API->>V3: Forward instruction, selector, selectAll
V3->>Handler: Pass selectAll in params
alt selectAll is true
Handler->>Snapshot: captureHybridSnapshot(selectAll: true)
Snapshot->>A11y: tryScopedSnapshot(focusSelector, selectAll: true)
A11y->>CDP: DOM.getDocument for each matched element
A11y->>CDP: resolveObjectIdsForCss(".card") or resolveObjectIdsForXPath("//a")
Note over A11y,CDP: NEW: returns array of all matching objectIds
CDP-->>A11y: ["object-1", "object-2", ...]
A11y->>CDP: DOM.describeNode for each objectId
CDP-->>A11y: backendDOMNodeId for each match
A11y->>A11y: Build set of top-level target backend nodes
A11y->>A11y: Filter a11y tree to keep subtree of all targets
A11y-->>Snapshot: scoped accessibility tree with multiple roots
Snapshot-->>Handler: snapshot scoped to all matching elements
else selectAll is false or omitted (default)
Handler->>Snapshot: captureHybridSnapshot(selectAll: false)
Snapshot->>A11y: tryScopedSnapshot(focusSelector, selectAll: false)
A11y->>CDP: resolveObjectIdForCss(".card") or resolveObjectIdForXPath("//a")
Note over A11y,CDP: Existing: single match path
CDP-->>A11y: single objectId
A11y->>CDP: DOM.describeNode
CDP-->>A11y: single backendDOMNodeId
A11y->>A11y: Filter tree to single root subtree
A11y-->>Snapshot: scoped tree with one root
Snapshot-->>Handler: snapshot scoped to first match only
end
alt Extract flow
Handler->>Handler: LLM extraction using scoped snapshot
Handler-->>V3: Extracted data
V3-->>API: Result
API-->>Client: Response
else Observe flow
Handler->>Handler: LLM observation using scoped snapshot
alt selectAll is true
Handler->>Handler: Resolve all matched element selectors
else selectAll is false
Handler->>Handler: Resolve only first matched element selector
end
Handler-->>V3: Observed actions
V3-->>API: Result
API-->>Client: Response
end
Note over Client,CDP: Key Branches for resolveObjectIdsForCss/XPath
loop For index 0..N while objectId found
A11y->>CDP: Runtime.evaluate(resolver(index))
CDP-->>A11y: objectId or null
alt objectId returned
A11y->>A11y: Add to array
else null returned
A11y->>A11y: Stop iteration
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Collaborator
Contributor
|
@miguelg719 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
3 issues found across 14 files
Confidence score: 3/5
- There is moderate regression risk in
packages/core/lib/v3/understudy/a11y/snapshot/a11yTree.ts: if oneDOM.describeNodecall fails,selectAllcan fall back to fully unscoped output, which may change user-visible selector results. packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.tsappears to return multiple CDP object handles without a clear release path, so unresolvedobjectIds may accumulate and impact renderer stability over time.packages/core/lib/v3/types/public/api.tsintroduces a new publicselectAllrequest option, but the noted missing integration coverage for Stagehand REST API changes increases uncertainty around client/server compatibility.- Pay close attention to
packages/core/lib/v3/understudy/a11y/snapshot/a11yTree.ts,packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts, andpackages/core/lib/v3/types/public/api.ts- scoping fallback behavior, CDP handle lifecycle, and API compatibility testing are the main merge risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/understudy/a11y/snapshot/a11yTree.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/a11yTree.ts:85">
P1: `selectAll` scoping is fragile: one failing `DOM.describeNode` causes fallback to fully unscoped output, discarding partial successful matches.</violation>
</file>
<file name="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts">
<violation number="1" location="packages/core/lib/v3/understudy/a11y/snapshot/focusSelectors.ts:249">
P2: Multi-object CDP handles are returned without a corresponding release path, so resolved objectIds can leak in the renderer.</violation>
</file>
<file name="packages/core/lib/v3/types/public/api.ts">
<violation number="1" location="packages/core/lib/v3/types/public/api.ts:508">
P2: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
New public request option `selectAll` is added without server integration-test coverage for the changed API path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as External Client
participant API as API Route / SDK
participant ExtHandler as ExtractHandler
participant ObsHandler as ObserveHandler
participant V3 as V3 Orchestrator
participant Snapshot as tryScopedSnapshot
participant A11y as a11yForFrame
participant Selector as resolveObjectIdsForCss/XPath
participant CDPSession as CDP Session
Note over Client,CDPSession: NEW: selectAll parameter flow for extract/observe
Client->>API: extract/observe with selectAll=true
API->>V3: forward selectAll in params
alt Extract path
V3->>ExtHandler: extract({ selectAll, selector, ... })
ExtHandler->>Snapshot: captureHybridSnapshot({ focusSelector, selectAll })
else Observe path
V3->>ObsHandler: observe({ selectAll, selector, ... })
ObsHandler->>Snapshot: captureHybridSnapshot({ focusSelector, selectAll })
end
Snapshot->>A11y: tryScopedSnapshot({ focusSelector, selectAll })
alt selectAll is true
A11y->>Selector: resolveObjectIdsForCss or resolveObjectIdsForXPath (no limit)
Selector->>CDPSession: Loop Runtime.evaluate index=0,1,2... until null/error
CDPSession-->>Selector: objectId at each index
Selector-->>A11y: array of all matching objectIds
loop For each objectId
A11y->>CDPSession: DOM.describeNode
CDPSession-->>A11y: backendNodeId
end
A11y->>A11y: Filter to top-level targets (no ancestor in match set)
A11y->>A11y: BFS include all children of top-level targets
A11y-->>Snapshot: scoped accessibility tree with multiple roots
else selectAll is false/undefined
A11y->>Selector: resolveObjectIdForCss or resolveObjectIdForXPath (limit=1)
Selector->>CDPSession: Runtime.evaluate index=0
CDPSession-->>Selector: single objectId
Selector-->>A11y: first match only
A11y->>A11y: Single root, include children
A11y-->>Snapshot: scoped accessibility tree (single root)
end
Snapshot-->>ExtHandler/ObsHandler: hybrid snapshot
alt Observe path - element resolution
ObsHandler->>ObsHandler: Map elementIds -> xpaths
Note over ObsHandler: dragAndDrop target lookup may fail
alt dragAndDrop target lookup successful
ObsHandler->>ObsHandler: Resolve target xpath
else dragAndDrop target lookup fails
ObsHandler->>ObsHandler: Filter out action, log warning
end
ObsHandler-->>API: Action[]
else Extract path
ExtHandler-->>API: Extracted data
end
API-->>Client: Response with multi-element scope results
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| example: 30000, | ||
| }), | ||
| selector: z.string().optional().meta({ | ||
| description: "CSS selector to scope extraction to a specific element", |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test
New public request option selectAll is added without server integration-test coverage for the changed API path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/types/public/api.ts, line 508:
<comment>New public request option `selectAll` is added without server integration-test coverage for the changed API path.</comment>
<file context>
@@ -505,6 +505,11 @@ export const ExtractOptionsSchema = z
description: "CSS selector to scope extraction to a specific element",
example: "#main-content",
}),
+ selectAll: z.boolean().optional().meta({
+ description:
+ "When true, apply selector scoping to all matching elements instead of only the first match",
</file context>
Author
|
@miguelg719 I have addressed the additional cubic comments. |
Collaborator
|
hey @NahomAgidew we merged an |
…le, and it always releases Runtime objects
NahomAgidew
force-pushed
the
nahom/issues/1904
branch
from
27c0b4c to
a402a20
Compare
Author
@miguelg719 done! |
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
2 participants
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.
why
what changed
selectAllparameter toextract()andobserve()so selector scoping can include all matching elements’ subtrees instead of only the first match; default behavior is unchanged when omitted or falsetest plan
public-types.test.ts: Extended the expected ExtractOptions / ObserveOptions type shapes to include the new optional selectAll?: boolean field.api-client-observe-variables.test.ts: Verify observe/extract wire requests preserve selectAll in optionssnapshot-a11y-resolvers.test.ts: Cover multi-match scoping (selectAll: true), ensure single-match path doesn’t call bulk resolvers, addresolveObjectIdsForCsscoverage, and asserttryScopedSnapshotforwardsselectAllintoa11yForFrameSummary by cubic
Adds an optional
selectAllflag toextract()andobserve()to scope snapshots to all matching elements, not just the first. Improves stability by keeping partial results when candidates go stale, releasing CDP Runtime objects, and filtering invaliddragAndDroptargets (addresses #1904).New Features
selectAll?: booleantoExtractOptionsandObserveOptions; scoping honors it for CSS and XPath viaresolveObjectIdsForCss/resolveObjectIdsForXPathand returns all top-level matches.tryScopedSnapshot; updated public types andopenapi.v3.yaml; tests cover extract/observe request serialization and server integration.Bug Fixes
DOM.describeNodecalls fail and always releases Runtime objects.dragAndDroptarget IDs and logs details instead of failing.Written for commit a402a20. Summary will update on new commits.