[v4]: more page route stubs for locator functions#1882
[v4]: more page route stubs for locator functions#1882seanmcguire12 wants to merge 3 commits intomainfrom
Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files
Confidence score: 4/5
- This PR appears safe to merge overall, with only a low-to-moderate risk note rather than a clear functional defect.
- The main concern is in
packages/server-v4/src/routes/v4/page/highlight.ts: missing unit tests for the new/page/highlightbehavior (includingreturnSelector) could allow regressions to slip through unnoticed. - Given the issue’s moderate confidence and non-blocking nature, this is best treated as follow-up test hardening rather than a merge blocker.
- Pay close attention to
packages/server-v4/src/routes/v4/page/highlight.ts- new endpoint behavior needs test coverage to prevent silent regressions.
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/routes/v4/page/highlight.ts">
<violation number="1" location="packages/server-v4/src/routes/v4/page/highlight.ts:15">
P2: Add unit tests covering the new /page/highlight route (including returnSelector behavior) so this new endpoint doesn’t regress silently.
(Based on your team's feedback about adding unit tests for new behavior.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client
participant Router as Fastify Router
participant Schema as Zod / OpenAPI Schema
participant Handler as Page Action Handler
participant Exec as Action Execution (Stub)
Note over Client,Exec: NEW & UPDATED Locator Action Flow
Client->>Router: POST /v4/page/{action}
Note right of Client: action = fill, highlight, selectOption, setInputFiles, etc.
Router->>Schema: Validate Request Body
Note over Schema: CHANGED: returnSelector defaults to 'false'<br/>for click, hover, dragAndDrop, and new actions.
alt Validation Fails
Schema-->>Router: 400 Bad Request
Router-->>Client: Error Response
else Validation Success
Schema-->>Router: Validated Params
end
Router->>Handler: NEW: createPageActionHandler(method)
Handler->>Exec: execute()
Note over Exec: NEW: Returns stubbed results<br/>(e.g., PageFillResult, PageHighlightResult)
Exec-->>Handler: Stubbed Result Object
Handler->>Schema: Parse & Validate Result
Note over Schema: NEW: ResultSelector is now required in result schemas
Schema-->>Handler: Validated Action Object
Handler-->>Router: Action Response
Router-->>Client: 200 OK (PageActionResponse)
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const highlightRoute: RouteOptions = { | ||
| method: "POST", | ||
| url: "/page/highlight", |
There was a problem hiding this comment.
P2: Add unit tests covering the new /page/highlight route (including returnSelector behavior) so this new endpoint doesn’t regress silently.
(Based on your team's feedback about adding unit tests for new behavior.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v4/src/routes/v4/page/highlight.ts, line 15:
<comment>Add unit tests covering the new /page/highlight route (including returnSelector behavior) so this new endpoint doesn’t regress silently.
(Based on your team's feedback about adding unit tests for new behavior.) </comment>
<file context>
@@ -0,0 +1,38 @@
+
+const highlightRoute: RouteOptions = {
+ method: "POST",
+ url: "/page/highlight",
+ schema: {
+ operationId: "PageHighlight",
</file context>
| export const PageHoverParamsSchema = PageWithPageIdSchema.extend({ | ||
| selector: SelectorSchema, | ||
| returnSelector: z.boolean().optional(), | ||
| returnSelector: z.boolean().default(false).optional(), |
There was a problem hiding this comment.
Why not just always return the selector?
There was a problem hiding this comment.
populating the selector schema can be expensive, so the user should be able to decide whether they want it
|
|
||
| export const PageFillParamsSchema = PageWithPageIdSchema.extend({ | ||
| selector: SelectorSchema, | ||
| value: z.string(), |
There was a problem hiding this comment.
| value: z.string(), | |
| value: z.union([z.string(), z.number(), z.boolean(), ...]), |
Shouldnt this support more types? form input fields can be checkboxes, radio selectors, select dropdowns, datepickers, etc.
There was a problem hiding this comment.
following the playwright api here https://playwright.dev/docs/api/class-locator#locator-fill
|
|
||
| export const PageHighlightParamsSchema = PageWithPageIdSchema.extend({ | ||
| selector: SelectorSchema, | ||
| color: z.string().optional(), |
There was a problem hiding this comment.
How do we control the color lol? Doesn't this just mean "select this text on the page with the cursor?
Or are we actually modifying the CSS to color the element somehow?
If it's the latter I wouldnt expose just color, I would expose an endpoint to apply arbitrary CSS to any element and let the user decide how to color (e.g. background-color, color, opacity, etc.).
There was a problem hiding this comment.
the latter. open to exposing an applyCSS route though
|
|
||
| export const PageSelectOptionParamsSchema = PageWithPageIdSchema.extend({ | ||
| selector: SelectorSchema, | ||
| values: z.array(z.string()), |
There was a problem hiding this comment.
| values: z.array(z.string()), | |
| values: z.array(z.string()).description('Values or labels for the <option>s you want to select'), |
Agents very often will try to pass the text label (what they see on the page via screenshot) and not the actual value attr, so we need to support both and fallback to substring/fuzzy matching. Select option text is often truncated, slightly occluded, or contains weird characters/whitespace that make exact matches hard.
Keep in mind this endpoint also needs to support aria-based selectors that are custom HTML and not <select> elements (which are super common on modern sites). Returning a single selector doesn't always make sense, a multi-select might be built out of 8 sibling dom elements for example. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Attributes/aria-selected
There was a problem hiding this comment.
- can add the description. for clarity, the existing
locator.selectOption()does support labels & values. - we can try to do fuzzy matching when this route actually gets implemented
- this route is modeled after the existing
locator.selectOption()function, & therefore does not support non-select elements. the single selector that is returned will point to the outer<select>element
| .meta({ id: "PageSelectOptionParams" }); | ||
|
|
||
| export const PageSetInputFilesParamsSchema = PageWithPageIdSchema.extend({ | ||
| selector: SelectorSchema, |
There was a problem hiding this comment.
| selector: SelectorSchema, | |
| selector: SelectorSchema.optional(), |
selector should be optional imo, you should be able to set default upload files for a page, then any click on an <input type="file"> should autopopulate the files and close the file selector popup.
| export const PageFillResultSchema = z | ||
| .object({ | ||
| selector: ResultSelectorSchema, | ||
| value: z.string(), |
There was a problem hiding this comment.
| value: z.string(), | |
| value: z.union([z.string(), ...]), |
Needs more types for more <input> types.
| export const PageHighlightResultSchema = z | ||
| .object({ | ||
| selector: ResultSelectorSchema, | ||
| highlighted: z.boolean(), |
There was a problem hiding this comment.
| highlighted: z.boolean(), |
Route should just return an error if highlight CSS applying fails.
| selector: ResultSelectorSchema, | ||
| selected: z.array(z.string()), |
There was a problem hiding this comment.
| selector: ResultSelectorSchema, | |
| selected: z.array(z.string()), | |
| selector: ResultSelectorSchema, | |
| selected: z.array(z.string()), | |
| isMultiSelect: z.boolean(), |
we should clearly indicate when they try to select multiple options but the select field only supports one / vice versa
|
|
||
| export const PageSetInputFilesResultSchema = z | ||
| .object({ | ||
| selector: ResultSelectorSchema, |
There was a problem hiding this comment.
| selector: ResultSelectorSchema, | |
| selector: ResultSelectorSchema.optional(), |
There was a problem hiding this comment.
to reduce union types, i want to always return the ResultSelectorSchema shape, but it is just {} if the user didn't set returnSelector: true
pirate
left a comment
pirate
left a comment
There was a problem hiding this comment.
Lots of small comments but the biggest items are:
- I think we should replace
highlightwith a genericapplyCssendpoint, because highlighting should not only supportcolor - I dont think we need
returnSelectoron every route, we should just always return the selector but omit any expensive fields fillneeds to support more types (slider, radio, checkbox, date, time, color, number, etc.), ideally we should merge SelectOption into fill imo so it supports truly all fields
2 participants
why
what changed
/fill/highlight/selectOption/setInputFilesreturnSelector: true/click,/hover, and/dragAndDroproutes to defaultreturnSelectortofalsescreenshot:
test plan
Summary by cubic
Adds v4 page route stubs for locator actions: /v4/page/fill, /v4/page/highlight, /v4/page/selectOption, and /v4/page/setInputFiles. Addresses STG-1646 and updates OpenAPI and schemas; routes return placeholder results with optional selector output.
New Features
Refactors
Written for commit f62f42e. Summary will update on new commits. Review in cubic