fixed deepLocator() pathing bug #880
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR fixes a critical bug in the deepLocator() function's XPath handling. The original implementation incorrectly consolidated multiple forward slashes into single ones, which broke XPath queries using the descendant-or-self axis (//). For example, a query like //*[@id='myID'] would incorrectly become /*[@id='myID'], changing the semantic meaning from "find anywhere in the document" to "only look at root level". The fix preserves the number of slashes in XPath expressions by using a more precise string splitting approach.
PR Description Notes:
- The test plan could be more specific about which test cases should be added
Confidence score: 4/5
- Safe to merge after proper testing, as this fixes a bug rather than introducing new features
- High confidence due to the clear bug fix and preservation of XPath semantics, but lacks comprehensive test coverage
- Files needing attention:
- lib/handlers/handlerUtils/actHandlerUtils.ts - Needs additional test cases for non-direct descendant XPath queries
1 file reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
| if (buffer.length === 0) { | ||
| return (ctx as Page | FrameLocator).locator("xpath=/*"); | ||
| } |
There was a problem hiding this comment.
logic: Potentially incorrect fallback for empty buffer case - returning '/*' will only match root level elements, not all elements in the frame
why
The logic in deepLocator was consolidating all the double // -> single /s, meaning that if I tried to do any sort of XPATH without specifying direct descendants, the query would break. So for example when I set my selector to `selector: xpath=//[@id='myID'] , it turns the locator into 'xpath=/[@id='myID']' removing the 2 slashes. This breaks my search because it's only looking for the ID in the root element, not across the descendants of the whole document (which is what I wanted)
what changed
I changed the logic so that it preserves the number of slashes in the xpath instead of splitting on "/", dropping all the slashes and then re-adding them
test plan
I tested locally but the stagehand team should verify that this works on whatever suite of tests they typically try. They should also probably add a test to catch this error like a search on non-direct descendants.