[fix]: extend support for xpaths containing predicates#1683
Open
seanmcguire12 wants to merge 3 commits intomainfrom
Open
[fix]: extend support for xpaths containing predicates#1683seanmcguire12 wants to merge 3 commits intomainfrom
seanmcguire12 wants to merge 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2aa61e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Greptile OverviewGreptile SummaryUnifies XPath resolution into a shared resolver module ( Key improvements:
Testing:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Locator API
participant Selector as selectors.ts
participant Count as counts.ts
participant Wait as waitForSelector.ts
participant Resolver as xpathResolver.ts
participant Parser as xpathParser.ts
participant DOM as Shadow DOM
Client->>Selector: resolveXPathMainWorld(xpath, index)
Selector->>Resolver: resolveXPathAtIndex(xpath, index)
Resolver->>Parser: parseXPathSteps(xpath)
Parser-->>Resolver: steps with predicates
Resolver->>Resolver: getShadowContext()
Resolver->>Resolver: resolveXPathComposedMatches(xpath)
Resolver->>DOM: composedChildren/composedDescendants
DOM-->>Resolver: elements from light and shadow
Resolver->>Parser: applyPredicates(elements, predicates)
Parser-->>Resolver: filtered elements
Resolver-->>Selector: element at index
Selector-->>Client: Element
Client->>Count: countXPathMatchesMainWorld(xpath)
Count->>Resolver: countXPathMatches(xpath)
Resolver->>Parser: parseXPathSteps(xpath)
Parser-->>Resolver: steps
Resolver->>Resolver: resolveXPathComposedMatches(xpath)
Resolver->>DOM: traverse composed tree
DOM-->>Resolver: all matches
Resolver-->>Count: count
Count-->>Client: number
Client->>Wait: waitForSelector(xpath)
Wait->>Resolver: resolveXPathFirst(xpath)
Resolver->>Resolver: resolveXPathAtIndex(xpath, 0)
Resolver-->>Wait: first element or null
Last reviewed commit: 955f87b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
1 issue found across 12 files
Confidence score: 4/5
- This PR looks generally safe to merge, with one moderate parser edge case noted as the main risk driver.
- In
packages/core/lib/v3/dom/locatorScripts/xpathParser.ts,isBoundaryonly treats whitespace as a word boundary, so XPath keywords likeand/oradjacent to parentheses may be misparsed and could affect certain selector expressions. - Pay close attention to
packages/core/lib/v3/dom/locatorScripts/xpathParser.ts- keyword boundary handling around parentheses may break valid XPath operators without spaces.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core/lib/v3/dom/locatorScripts/xpathParser.ts">
<violation number="1" location="packages/core/lib/v3/dom/locatorScripts/xpathParser.ts:349">
P2: `isBoundary` only considers whitespace as a word boundary, so `and`/`or` keywords adjacent to parentheses (e.g., `not(@x)and @y='z'`) won't be split. XPath allows operators without surrounding spaces. Consider treating any non-word character as a boundary instead.</violation>
</file>
Linked issue analysis
Linked issue: STG-1373: add shared xpath resolver + policy
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add shared resolver (e.g. xpathResolver.ts) | Added xpathResolver.ts with exported resolver functions |
| ✅ | Resolver owns native document.evaluate resolution vs composed-tree resolution | Resolver tries native evaluate then falls back to composed traversal |
| ✅ | Resolver centralizes nth (.nth) vs count (.count) semantics | Provides resolveXPathAtIndex(index) and countXPathMatches() APIs |
| ✅ | Ordering: use DFS (document-order) instead of BFS for composed traversal | composedDescendants uses stack + reversed push to produce DFS order |
| ✅ | Make all call sites use the shared resolver (selectors, counts, waitForSelector) | Selectors/counts/waitForSelector now call resolver functions |
Architecture diagram
sequenceDiagram
participant Script as Entry Point (selectors/counts)
participant Resolver as NEW: xpathResolver.ts
participant Parser as CHANGED: xpathParser.ts
participant DOM as Browser DOM (Light + Shadow)
participant Backdoor as window.__stagehandV3__
Note over Script,Backdoor: Unified XPath Resolution Flow
Script->>Resolver: NEW: resolveXPathAtIndex / countXPathMatches
Resolver->>Resolver: normalizeXPath()
Resolver->>Parser: NEW: parseXPathSteps(xpath)
Note right of Parser: Parses complex predicates:<br/>and/or/not, contains(),<br/>normalize-space(), text()
Parser-->>Resolver: Array of XPathStep objects
alt Pierce Shadow (Default)
Resolver->>Backdoor: getShadowContext()
Backdoor-->>Resolver: stats & getClosedRoot helper
loop For each XPath Step
Resolver->>DOM: Get composed children/descendants
Note right of Resolver: DFS traversal across<br/>ShadowRoot boundaries
loop For each candidate Element
Resolver->>Parser: NEW: applyPredicates(element, predicates)
Parser->>DOM: CHANGED: Check attributes/text
Parser-->>Resolver: boolean (matches)
end
end
else Native Fallback (Light DOM only)
Resolver->>DOM: document.evaluate(xpath, ...)
DOM-->>Resolver: XPathResult
end
Resolver-->>Script: Final Element(s) or Match Count
Script-->>Script: Return result to Main World/Playwright
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
Author
Contributor
|
@seanmcguire12 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 12 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Linked issue analysis
Linked issue: STG-1373: add shared xpath resolver + policy
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Add a shared resolver (xpathResolver.ts) that owns native document.evaluate vs composed tree resolution | New xpathResolver.ts implements native vs composed logic |
| ✅ | Ensure .count()/.nth() include shadow DOM matches (nth vs count semantics) | count/resolve call into resolver with pierceShadow by default |
| ✅ | Ordering: use DFS instead of BFS for composed traversal (document-order) | composedDescendants uses stack (DFS) and reversed push |
| ✅ | Make all call sites use the shared resolver | selectors, counts and waitForSelector now call resolver APIs |
| ✅ | Expand composed predicate parsing/evaluation (attr exists, contains, starts-with, text(), normalize-space, and/or/not) | Parser adds predicate types and evaluator handles these cases |
| ✅ | Route xpath entrypoints through the resolver (selectors.ts, counts.ts, waitForSelector.ts) | Entrypoints now delegate to resolver functions |
| ✅ | Remove unused injected helper resolveSimpleXPath from piercer.runtime.ts & update global.d.ts | resolveSimpleXPath removed and backdoor type updated |
| ✅ | Add tests covering parser and resolver (shadow DOM inclusion and ordering) | Parser tests expanded and resolver tests added with jsdom env |
| ✅ | Use native document.evaluate when possible and fallback to composed traversal when needed | Resolver tries native evaluate and falls back on error/hasShadow |
| ✅ | Ensure nth uses consistent document-order traversal and indexing semantics (including positional predicates) | Index predicate handled and resolver returns composed[targetIndex] |
Architecture diagram
sequenceDiagram
participant UI as User / Test Script
participant Entry as Entry Points (selectors/counts/waitFor)
participant Res as NEW: xpathResolver
participant Parser as CHANGED: xpathParser
participant Backdoor as Shadow Piercer Backdoor
participant DOM as Browser DOM (Light + Shadow)
Note over UI,DOM: Unified XPath Resolution Flow (Shadow-Inclusive)
UI->>Entry: resolveXPath / countXPath (rawXp)
Entry->>Res: CHANGED: resolveXPathAtIndex / countXPathMatches
Res->>Parser: parseXPathSteps(rawXp)
Note right of Parser: NEW: Parses predicates (attr, text(),<br/>normalize-space, and/or/not)
Parser-->>Res: XPathStep[] (structured axes + predicates)
alt NEW: Pierce Shadow = false OR No Shadow DOM detected
Res->>DOM: document.evaluate (native)
DOM-->>Res: XPathResult
else Composed Tree Traversal (Pierce Shadow = true)
loop For each XPathStep
Res->>DOM: Get children/descendants (DFS)
opt Element is Shadow Host
Res->>DOM: Access open shadowRoot
Res->>Backdoor: NEW: getClosedRoot(element)
Backdoor-->>Res: Return closed ShadowRoot
end
Res->>Parser: NEW: applyPredicates(elements, step.predicates)
Note right of Parser: Evaluates attr existence/contains,<br/>text equality, and boolean logic
Parser-->>Res: Filtered match list
end
end
alt Indexing / Results
Res->>Res: Order results by Document DFS
Res-->>Entry: Target element(s) or count
else Error / No Match
Res-->>Entry: null / 0
end
Entry-->>UI: Return Result (piercing shadow boundaries)
Note over Res,DOM: DFS ensures .nth(n) matches order across shadow boundaries
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
1 participant
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
.count()/.nth()include shadow DOM matcheswhat changed
packages/core/lib/v3/dom/locatorScripts/xpathResolver.tspackages/core/lib/v3/dom/locatorScripts/xpathParser.ts(attr existence, contains/starts-with, text(), normalize-space, and/or/not)packages/core/lib/v3/dom/locatorScripts/selectors.ts(resolveXPathMainWorld)packages/core/lib/v3/dom/locatorScripts/counts.ts(countXPathMatchesMainWorld)packages/core/lib/v3/dom/locatorScripts/waitForSelector.ts(xpath path now uses resolver)resolveSimpleXPathfrompackages/core/lib/v3/dom/piercer.runtime.ts& updatedpackages/core/lib/v3/dom/global.d.tstest plan
packages/core/tests/xpath-parser.test.ts(expanded predicate parsing + evaluation coverage)packages/core/tests/xpath-resolver.test.ts(shadow-inclusive count/nth ordering across composed tree)Summary by cubic
Extended XPath support to handle predicates and shadow DOM, and unified resolution behind a shared resolver. Fixes .count() and .nth() to include shadow DOM with consistent document-order DFS, aligning with Linear STG-1373.
New Features
Refactors
Written for commit 2aa61e5. Summary will update on new commits. Review in cubic