Conversation
|
Greptile SummaryThis PR unifies all selector types across the v4 page API into a single discriminated union ( Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class PageSelector {
<<anyOf>>
}
class PageElementSelector {
<<anyOf>>
}
class PageXPathSelector {
+xpath: string
}
class PageCssSelector {
+css: string
}
class PageTextSelector {
+text: string
}
class PageCoordinateSelector {
+x: number
+y: number
}
PageSelector --> PageXPathSelector
PageSelector --> PageCssSelector
PageSelector --> PageTextSelector
PageSelector --> PageCoordinateSelector
PageElementSelector --> PageXPathSelector
PageElementSelector --> PageCssSelector
PageElementSelector --> PageTextSelector
class PageClickParams {
+selector: PageSelector
+button?: MouseButton
+clickCount?: number
}
class PageHoverParams {
+selector: PageSelector
}
class PageScrollElementParams {
+selector: PageElementSelector
+percentage: number
}
class PageScrollCoordinateParams {
+selector: PageCoordinateSelector
+deltaX?: number
+deltaY: number
}
class PageDragAndDropParams {
+from: PageSelector
+to: PageSelector
+button?: MouseButton
+steps?: number
+delay?: number
}
class PageWaitForSelectorParams {
+selector: PageElementSelector
+state?: WaitForSelectorState
+timeout?: number
}
PageClickParams --> PageSelector
PageHoverParams --> PageSelector
PageScrollElementParams --> PageElementSelector
PageScrollCoordinateParams --> PageCoordinateSelector
PageDragAndDropParams --> PageSelector
PageWaitForSelectorParams --> PageElementSelector
|
There was a problem hiding this comment.
1 issue found across 8 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported concern is a low-to-moderate severity API/schema hygiene issue (4/10) rather than a clear runtime break.
- The key issue is in
packages/server-v4/src/schemas/v4/page.ts: internal schemas are being exposed viapageOpenApiComponents, which can leak private implementation details instead of keeping them scoped toPageScrollParamsSchema. - Pay close attention to
packages/server-v4/src/schemas/v4/page.ts- ensure internal-only schemas are removed frompageOpenApiComponentsto avoid unintended public surface area.
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/server-v4/src/schemas/v4/page.ts">
<violation number="1" location="packages/server-v4/src/schemas/v4/page.ts:1415">
P2: Remove these internal schemas from `pageOpenApiComponents`. Since they are not exported, they should remain private implementation details of `PageScrollParamsSchema`.\n\n(Based on your team's feedback about avoiding exposing internal types as public APIs.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Validator as OpenAPI / Zod Validator
participant Handler as Route Handler (v4)
participant Shared as Shared Utils (normalizeXPath)
participant Browser as Stagehand Core
Note over Client,Browser: Unified Page Action Flow (Click, Hover, Drag, Scroll)
Client->>Validator: POST /v4/page/[action]
Note right of Client: CHANGED: Request now uses unified 'selector' field<br/>instead of top-level x/y coordinates
activate Validator
Validator->>Validator: CHANGED: Match PageSelector Union
alt Selector is XPath/CSS/Text
Validator-->>Handler: PageElementSelector matched
else Selector is Coordinate
Validator-->>Handler: PageCoordinateSelector matched (x, y)
end
deactivate Validator
activate Handler
Handler->>Handler: NEW: Extract specific key from selector union
alt Action is 'scroll'
alt NEW: PageElementSelector used
Handler->>Handler: Require 'percentage' param
else NEW: PageCoordinateSelector used
Handler->>Handler: Require 'deltaY' (and optional 'deltaX')
end
else Action is 'dragAndDrop'
Note right of Handler: NEW: 'from' and 'to' can be<br/>different selector types (e.g. xpath to coord)
else Action is 'waitForSelector'
Note right of Handler: NEW: Restricted to PageElementSelector<br/>(Coordinates not allowed)
end
Handler->>Shared: normalizeXPath(params.selector)
Shared-->>Handler: xpath (real or stub)
Handler->>Browser: Execute Stagehand action
Browser-->>Handler: Action Result
Handler-->>Client: 200 OK (with PageAction result)
deactivate Handler
Note over Client,Validator: Unhappy Path: Validation Error
Client->>Validator: POST /v4/page/click { selector: { invalid: 'type' } }
Validator-->>Client: 400 Bad Request (ValidationErrorResponse)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| .meta({ id: "PageCoordinateSelector" }); | ||
|
|
||
| // Full union (all 4 types) | ||
| export const PageSelectorSchema = z |
There was a problem hiding this comment.
Should we just call this SelectorSchema or ElementSelectorSchema or SuperSelectorSchema? Having page in the name makes me think it's a selector to select a page out of the list of all pages.
pirate
left a comment
There was a problem hiding this comment.
Approved either way but see my comment about naming.
why
selectorinto a single field across page request bodieswhat changed
PageSelectorSchema(xpath-only object) with a discriminated union of 4 selector types:PageXPathSelectorSchema,PageCssSelectorSchema,PageTextSelectorSchema,PageCoordinateSelectorSchemaPageElementSelectorSchema(xpath/css/text, no coordinates) forwaitForSelector*SelectorParams/*CoordinatesParamsschema pairs for click, hover, scroll, and dragAndDrop into single schemas with a unifiedselectorfieldexample
/hoverbefore:example
/hoverafter:test plan
Summary by cubic
Unifies the v4 page selector into a single
selectorunion and adds css, text, and coordinate options. This simplifies request bodies and makes click, hover, scroll, and drag‑and‑drop consistent.New Features
selectorunion for all page actions: xpath, css, text, or coordinates.SelectorandElementSelectorunions; split formerPageSelectorintoXPathSelector,CssSelector,TextSelector, andCoordinateSelector.ElementSelector(xpath/css/text) for element‑only cases likewaitForSelectorand scroll‑by‑percentage; exportedPageScrollElementParamsandPageScrollCoordinateParams.dragAndDropsupports mixing selector types (e.g., xpath → coordinates).Migration
{ selector: { xpath|css|text|x,y } }. Remove top‑levelx/y.percentage, or coordinate selector +deltaY.from/tonow use the unified selector (elements or coordinates).ElementSelector(no coordinates).Written for commit b0b15f5. Summary will update on new commits. Review in cubic