Conversation
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 4 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client
participant API as API Server (v4)
participant Val as Validation (Zod)
participant Engine as Browser Engine
Note over Client,Engine: Action Request Flow (Scroll)
Client->>API: POST /page/scroll { params }
API->>Val: NEW: Validate 3-way Discriminated Union
alt Mode: PageScrollByOffset
Val-->>API: Validated { offset: {x, y}, cursorPosition? }
else Mode: PageScrollByPages
Val-->>API: Validated { pages: N, delayBetweenMs?, cursorPosition? }
else Mode: PageScrollToTarget
Val-->>API: Validated { target: Selector }
end
opt NEW: cursorPosition provided
API->>Engine: Move mouse to cursorPosition selector
Note right of Engine: Focuses specific scrollable container
end
API->>Engine: Execute Scroll Logic (Offset, Pages, or Target)
Engine-->>API: NEW: Return final scroll coordinates { x, y }
API-->>Client: 200 OK with PageScrollResult { x, y }
Note over Client,Engine: Unified Action Result Flow (Click/Hover/Drag)
Client->>API: POST /page/click { selector, returnSelector: true }
API->>Engine: Perform Action
Engine-->>API: Return interaction details
API->>API: NEW: Construct ResultSelector (xpath, css, text, coords)
API-->>Client: 200 OK with CHANGED: PageActionOutput (includes ResultSelector)
| cursorPosition: SelectorSchema.optional(), | ||
| offset: z | ||
| .object({ | ||
| x: z.number(), |
There was a problem hiding this comment.
this should be optional or at least default to 0
| x: z.number(), | |
| x: z.number().default(0), |
| selector: ElementSelectorSchema, | ||
| percentage: z.number().min(0).max(100), | ||
| export const PageScrollByOffsetParamsSchema = PageWithPageIdSchema.extend({ | ||
| cursorPosition: SelectorSchema.optional(), |
There was a problem hiding this comment.
| cursorPosition: SelectorSchema.optional(), | |
| element: SelectorSchema.optional(), |
I would really call this element or container
|
|
||
| export const PageScrollByPagesParamsSchema = PageWithPageIdSchema.extend({ | ||
| cursorPosition: SelectorSchema.optional(), | ||
| pages: z.number(), |
There was a problem hiding this comment.
| pages: z.number(), | |
| pages: z.number().lte(100).gte(-100).default(1), |
I would define min and max values here and have the min allow negative values so agents reading the api spec can tell that negatives and floats are allowed.
| deltaX: z.number().optional(), | ||
| deltaY: z.number(), | ||
| export const PageScrollToTargetParamsSchema = PageWithPageIdSchema.extend({ | ||
| target: SelectorSchema, |
There was a problem hiding this comment.
| target: SelectorSchema, | |
| target: SelectorSchema, | |
| position: z.enum(['center', 'top', 'bottom']).default('center'), |
pirate
left a comment
pirate
left a comment
There was a problem hiding this comment.
Approved but ideally I would change the shapes slightly (see comments)
seanmcguire12
force-pushed
the
seanmcguire/stg-1644-unify-scroll-scrollto
branch
from
2adf994 to
10c534e
Compare
2 participants
why
PageScrollElementParams(element selector + percentage) andPageScrollCoordinateParams(coordinate selector + deltaX/deltaY).what changed
PageScrollByOffsetParams: scroll by pixel delta ({ cursorPosition?, offset: {x, y} })PageScrollByPagesParams: scroll by viewport heights ({ cursorPosition?, pages: number, delayBetweenMs? })PageScrollToTargetParams: scroll until a target is visible ({ target: Selector })cursorPositionis an optionalSelectorthat determines where the mouse cursor is placed before scrolling. this controls which scrollable container receives the event.test plan
Summary by cubic
Refactors the v4
/page/scrollroute to a cleaner 3‑mode API and updates the OpenAPI to return detailed scroll action outputs. Adds apositionoption for target scrolls and adelayBetweenMsoption for page-based scrolling, aligning with Linear STG-1644.Refactors
{offset:{x,y}}, by pages{pages, delayBetweenMs?}, or to target{target, position?}; optional{cursorPosition}picks the scroll container.{x, y}; OpenAPI addsPageScrollActionOutputand...ParamsOutputtypes.Migration
selector + percentageandselector + deltaX/deltaY.{offset:{x,y}},{pages, delayBetweenMs?, cursorPosition?}, or{target, position?}.{x,y}from/scroll.returnSelector: trueto receive aResultSelector.Written for commit 10c534e. Summary will update on new commits. Review in cubic